| Message ID | 20201002165656.16744-1-prabhakar.mahadev-lad.rj@bp.renesas.com |
|---|---|
| Headers | show |
| Series | media: i2c: ov772x: Enable BT.656 mode and test pattern support | expand |
Hi Prabhakar, On Fri, Oct 02, 2020 at 05:56:54PM +0100, Lad Prabhakar wrote: > Parse endpoint properties using v4l2_fwnode_endpoint_alloc_parse() > to determine the bus type and store it in the driver structure. > > Set bus_type to V4L2_MBUS_PARALLEL as it's the only supported one > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > --- > drivers/media/i2c/ov772x.c | 33 +++++++++++++++++++++++++++++++++ > 1 file changed, 33 insertions(+) > > diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c > index 2cc6a678069a..b56f8d7609e6 100644 > --- a/drivers/media/i2c/ov772x.c > +++ b/drivers/media/i2c/ov772x.c > @@ -31,6 +31,7 @@ > #include <media/v4l2-ctrls.h> > #include <media/v4l2-device.h> > #include <media/v4l2-event.h> > +#include <media/v4l2-fwnode.h> > #include <media/v4l2-image-sizes.h> > #include <media/v4l2-subdev.h> > > @@ -434,6 +435,7 @@ struct ov772x_priv { > #ifdef CONFIG_MEDIA_CONTROLLER > struct media_pad pad; > #endif > + enum v4l2_mbus_type bus_type; > }; > > /* > @@ -1348,6 +1350,33 @@ static const struct v4l2_subdev_ops ov772x_subdev_ops = { > .pad = &ov772x_subdev_pad_ops, > }; > > +static int ov772x_parse_dt(struct i2c_client *client, > + struct ov772x_priv *priv) > +{ > + struct v4l2_fwnode_endpoint bus_cfg = { .bus_type = V4L2_MBUS_PARALLEL }; This one gets over 80. > + struct fwnode_handle *ep; > + int ret; > + > + ep = fwnode_graph_get_next_endpoint(dev_fwnode(&client->dev), > + NULL); And this needs no newline. > + if (!ep) { > + dev_err(&client->dev, "Endpoint node not found\n"); > + return -EINVAL; > + } > + > + ret = v4l2_fwnode_endpoint_alloc_parse(ep, &bus_cfg); > + if (ret) > + goto error_fwnode_put; > + > + priv->bus_type = bus_cfg.bus_type; > + v4l2_fwnode_endpoint_free(&bus_cfg); > + > +error_fwnode_put: > + fwnode_handle_put(ep); > + > + return ret; > +} > + > /* > * i2c_driver function > */ > @@ -1415,6 +1444,10 @@ static int ov772x_probe(struct i2c_client *client) > goto error_clk_put; > } > > + ret = ov772x_parse_dt(client, priv); > + if (ret) > + goto error_clk_put; > + > ret = ov772x_video_probe(priv); > if (ret < 0) > goto error_gpio_put; -- Sakari Ailus
On Fri, Oct 02, 2020 at 05:56:56PM +0100, Lad Prabhakar wrote: > Add support for test pattern control supported by the sensor. > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com> > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > --- > drivers/media/i2c/ov772x.c | 17 ++++++++++++++++- > 1 file changed, 16 insertions(+), 1 deletion(-) > > diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c > index 6b46ad493bf7..b7e10c34ef59 100644 > --- a/drivers/media/i2c/ov772x.c > +++ b/drivers/media/i2c/ov772x.c > @@ -227,7 +227,7 @@ > > /* COM3 */ > #define SWAP_MASK (SWAP_RGB | SWAP_YUV | SWAP_ML) > -#define IMG_MASK (VFLIP_IMG | HFLIP_IMG) > +#define IMG_MASK (VFLIP_IMG | HFLIP_IMG | SCOLOR_TEST) > > #define VFLIP_IMG 0x80 /* Vertical flip image ON/OFF selection */ > #define HFLIP_IMG 0x40 /* Horizontal mirror image ON/OFF selection */ > @@ -425,6 +425,7 @@ struct ov772x_priv { > const struct ov772x_win_size *win; > struct v4l2_ctrl *vflip_ctrl; > struct v4l2_ctrl *hflip_ctrl; > + unsigned int test_pattern; Alignment. You can get away with one or possibly two but three is too many in such a small number of lines. :-) > /* band_filter = COM8[5] ? 256 - BDBASE : 0 */ > struct v4l2_ctrl *band_filter_ctrl; > unsigned int fps; > @@ -540,6 +541,11 @@ static const struct ov772x_win_size ov772x_win_sizes[] = { > }, > }; > > +static const char * const ov772x_test_pattern_menu[] = { > + "Disabled", > + "Vertical Color Bar Type 1", > +}; > + > /* > * frame rate settings lists > */ > @@ -809,6 +815,9 @@ static int ov772x_s_ctrl(struct v4l2_ctrl *ctrl) > } > > return ret; > + case V4L2_CID_TEST_PATTERN: > + priv->test_pattern = ctrl->val; > + return 0; > } > > return -EINVAL; > @@ -1107,6 +1116,8 @@ static int ov772x_set_params(struct ov772x_priv *priv, > val ^= VFLIP_IMG; > if (priv->hflip_ctrl->val) > val ^= HFLIP_IMG; > + if (priv->test_pattern) > + val |= SCOLOR_TEST; > > ret = regmap_update_bits(priv->regmap, COM3, SWAP_MASK | IMG_MASK, val); > if (ret < 0) > @@ -1442,6 +1453,10 @@ static int ov772x_probe(struct i2c_client *client) > priv->band_filter_ctrl = v4l2_ctrl_new_std(&priv->hdl, &ov772x_ctrl_ops, > V4L2_CID_BAND_STOP_FILTER, > 0, 256, 1, 0); > + v4l2_ctrl_new_std_menu_items(&priv->hdl, &ov772x_ctrl_ops, > + V4L2_CID_TEST_PATTERN, > + ARRAY_SIZE(ov772x_test_pattern_menu) - 1, > + 0, 0, ov772x_test_pattern_menu); > priv->subdev.ctrl_handler = &priv->hdl; > if (priv->hdl.error) { > ret = priv->hdl.error;
Hi Sakari, Thank you for the review. On Fri, Oct 2, 2020 at 10:13 PM Sakari Ailus <sakari.ailus@linux.intel.com> wrote: > > On Fri, Oct 02, 2020 at 05:56:56PM +0100, Lad Prabhakar wrote: > > Add support for test pattern control supported by the sensor. > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com> > > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > drivers/media/i2c/ov772x.c | 17 ++++++++++++++++- > > 1 file changed, 16 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c > > index 6b46ad493bf7..b7e10c34ef59 100644 > > --- a/drivers/media/i2c/ov772x.c > > +++ b/drivers/media/i2c/ov772x.c > > @@ -227,7 +227,7 @@ > > > > /* COM3 */ > > #define SWAP_MASK (SWAP_RGB | SWAP_YUV | SWAP_ML) > > -#define IMG_MASK (VFLIP_IMG | HFLIP_IMG) > > +#define IMG_MASK (VFLIP_IMG | HFLIP_IMG | SCOLOR_TEST) > > > > #define VFLIP_IMG 0x80 /* Vertical flip image ON/OFF selection */ > > #define HFLIP_IMG 0x40 /* Horizontal mirror image ON/OFF selection */ > > @@ -425,6 +425,7 @@ struct ov772x_priv { > > const struct ov772x_win_size *win; > > struct v4l2_ctrl *vflip_ctrl; > > struct v4l2_ctrl *hflip_ctrl; > > + unsigned int test_pattern; > > Alignment. > > You can get away with one or possibly two but three is too many in such a > small number of lines. :-) > It's aligned as per structure members (non pointers) Cheers Prabhakar > > /* band_filter = COM8[5] ? 256 - BDBASE : 0 */ > > struct v4l2_ctrl *band_filter_ctrl; > > unsigned int fps; > > @@ -540,6 +541,11 @@ static const struct ov772x_win_size ov772x_win_sizes[] = { > > }, > > }; > > > > +static const char * const ov772x_test_pattern_menu[] = { > > + "Disabled", > > + "Vertical Color Bar Type 1", > > +}; > > + > > /* > > * frame rate settings lists > > */ > > @@ -809,6 +815,9 @@ static int ov772x_s_ctrl(struct v4l2_ctrl *ctrl) > > } > > > > return ret; > > + case V4L2_CID_TEST_PATTERN: > > + priv->test_pattern = ctrl->val; > > + return 0; > > } > > > > return -EINVAL; > > @@ -1107,6 +1116,8 @@ static int ov772x_set_params(struct ov772x_priv *priv, > > val ^= VFLIP_IMG; > > if (priv->hflip_ctrl->val) > > val ^= HFLIP_IMG; > > + if (priv->test_pattern) > > + val |= SCOLOR_TEST; > > > > ret = regmap_update_bits(priv->regmap, COM3, SWAP_MASK | IMG_MASK, val); > > if (ret < 0) > > @@ -1442,6 +1453,10 @@ static int ov772x_probe(struct i2c_client *client) > > priv->band_filter_ctrl = v4l2_ctrl_new_std(&priv->hdl, &ov772x_ctrl_ops, > > V4L2_CID_BAND_STOP_FILTER, > > 0, 256, 1, 0); > > + v4l2_ctrl_new_std_menu_items(&priv->hdl, &ov772x_ctrl_ops, > > + V4L2_CID_TEST_PATTERN, > > + ARRAY_SIZE(ov772x_test_pattern_menu) - 1, > > + 0, 0, ov772x_test_pattern_menu); > > priv->subdev.ctrl_handler = &priv->hdl; > > if (priv->hdl.error) { > > ret = priv->hdl.error; > > -- > Sakari Ailus
Hi Sakari, Thank you for the review. On Fri, Oct 2, 2020 at 10:12 PM Sakari Ailus <sakari.ailus@linux.intel.com> wrote: > > Hi Prabhakar, > > On Fri, Oct 02, 2020 at 05:56:54PM +0100, Lad Prabhakar wrote: > > Parse endpoint properties using v4l2_fwnode_endpoint_alloc_parse() > > to determine the bus type and store it in the driver structure. > > > > Set bus_type to V4L2_MBUS_PARALLEL as it's the only supported one > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > > --- > > drivers/media/i2c/ov772x.c | 33 +++++++++++++++++++++++++++++++++ > > 1 file changed, 33 insertions(+) > > > > diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c > > index 2cc6a678069a..b56f8d7609e6 100644 > > --- a/drivers/media/i2c/ov772x.c > > +++ b/drivers/media/i2c/ov772x.c > > @@ -31,6 +31,7 @@ > > #include <media/v4l2-ctrls.h> > > #include <media/v4l2-device.h> > > #include <media/v4l2-event.h> > > +#include <media/v4l2-fwnode.h> > > #include <media/v4l2-image-sizes.h> > > #include <media/v4l2-subdev.h> > > > > @@ -434,6 +435,7 @@ struct ov772x_priv { > > #ifdef CONFIG_MEDIA_CONTROLLER > > struct media_pad pad; > > #endif > > + enum v4l2_mbus_type bus_type; > > }; > > > > /* > > @@ -1348,6 +1350,33 @@ static const struct v4l2_subdev_ops ov772x_subdev_ops = { > > .pad = &ov772x_subdev_pad_ops, > > }; > > > > +static int ov772x_parse_dt(struct i2c_client *client, > > + struct ov772x_priv *priv) > > +{ > > + struct v4l2_fwnode_endpoint bus_cfg = { .bus_type = V4L2_MBUS_PARALLEL }; > > This one gets over 80. > Argh I need to adjust my checkpatch script > > + struct fwnode_handle *ep; > > + int ret; > > + > > + ep = fwnode_graph_get_next_endpoint(dev_fwnode(&client->dev), > > + NULL); > > And this needs no newline. > Agreed. Cheers, Prabhakar
On Fri, Oct 02, 2020 at 10:32:05PM +0100, Lad, Prabhakar wrote: > Hi Sakari, > > Thank you for the review. > > On Fri, Oct 2, 2020 at 10:13 PM Sakari Ailus > <sakari.ailus@linux.intel.com> wrote: > > > > On Fri, Oct 02, 2020 at 05:56:56PM +0100, Lad Prabhakar wrote: > > > Add support for test pattern control supported by the sensor. > > > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com> > > > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > > > --- > > > drivers/media/i2c/ov772x.c | 17 ++++++++++++++++- > > > 1 file changed, 16 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c > > > index 6b46ad493bf7..b7e10c34ef59 100644 > > > --- a/drivers/media/i2c/ov772x.c > > > +++ b/drivers/media/i2c/ov772x.c > > > @@ -227,7 +227,7 @@ > > > > > > /* COM3 */ > > > #define SWAP_MASK (SWAP_RGB | SWAP_YUV | SWAP_ML) > > > -#define IMG_MASK (VFLIP_IMG | HFLIP_IMG) > > > +#define IMG_MASK (VFLIP_IMG | HFLIP_IMG | SCOLOR_TEST) > > > > > > #define VFLIP_IMG 0x80 /* Vertical flip image ON/OFF selection */ > > > #define HFLIP_IMG 0x40 /* Horizontal mirror image ON/OFF selection */ > > > @@ -425,6 +425,7 @@ struct ov772x_priv { > > > const struct ov772x_win_size *win; > > > struct v4l2_ctrl *vflip_ctrl; > > > struct v4l2_ctrl *hflip_ctrl; > > > + unsigned int test_pattern; > > > > Alignment. > > > > You can get away with one or possibly two but three is too many in such a > > small number of lines. :-) > > > It's aligned as per structure members (non pointers) What a weird practice. Oh well. Keep as-is then. checkpatch.pl no longer seems to complain about lines over 80. That keeps the number of warnings lower but may lead to unintentional long lines when you don't need them. -- Sakari Ailus
Hi All, This patch series adds support for BT.656 mode in the ov772x sensor and also enables color bar test pattern control. Cheers, Prabhakar v6->v7 * Fixed review comments pointed by Sakari * Included Ack from Jacopo v5->v6 * Introduced new function ov772x_parse_dt() * Moved the backward compatibility comment from 1/3 to 2/3 v4->v5: * Put the ep instance back using fwnode_handle_put() * Renamed BT656 to BT.656 * Correctly handled backward compatibility case falling back to parallel mode. v3->v4: * New patch 1/3 to fallback in parallel mode. * Switched to v4l2_fwnode_endpoint_alloc_parse() for parsing the ep. * Dropped support for pdat for test pattern control. * DT documentation patches [1]. v2->v3: * Dropped DT binding documentation patch as this is handled by Jacopo. * Fixed review comments pointed by Jacopo. v2: https://patchwork.kernel.org/project/linux-renesas-soc/ list/?series=328133 v1: https://patchwork.kernel.org/project/linux-renesas-soc/ list/?series=323807 [1] https://patchwork.kernel.org/project/ linux-renesas-soc/list/?series=346809 Lad Prabhakar (3): media: i2c: ov772x: Parse endpoint properties media: i2c: ov772x: Add support for BT.656 mode media: i2c: ov772x: Add test pattern control drivers/media/i2c/ov772x.c | 69 +++++++++++++++++++++++++++++++++++++- 1 file changed, 68 insertions(+), 1 deletion(-)