| Message ID | 20200915174235.1229-2-prabhakar.mahadev-lad.rj@bp.renesas.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: i2c: ov772x: Enable BT.656 mode and test pattern support | expand |
Hi Prabhakar, On Tue, Sep 15, 2020 at 06:42:33PM +0100, Lad Prabhakar wrote: > Parse endpoint properties using v4l2_fwnode_endpoint_alloc_parse() > to determine bus-type and store it locally in priv data. > > v4l2_fwnode_endpoint_alloc_parse() with bus_type set to > V4L2_MBUS_PARALLEL falls back to V4L2_MBUS_PARALLEL thus handling > backward compatibility with existing DT where bus-type isn't specified. I don't think this is necessary here. This patch does not need to handle any retrocompatibility, as only PARALLEL is supported. The 'right' way to put it to me would be "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" See comment in the next patch for retrocompatibility > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > --- > drivers/media/i2c/ov772x.c | 25 +++++++++++++++++++++++++ > 1 file changed, 25 insertions(+) > > diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c > index 2cc6a678069a..4ab4b3c883d0 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; > }; > > /* > @@ -1354,6 +1356,8 @@ static const struct v4l2_subdev_ops ov772x_subdev_ops = { > > static int ov772x_probe(struct i2c_client *client) > { > + struct v4l2_fwnode_endpoint bus_cfg; > + struct fwnode_handle *ep; > struct ov772x_priv *priv; > int ret; > static const struct regmap_config ov772x_regmap_config = { > @@ -1415,6 +1419,27 @@ static int ov772x_probe(struct i2c_client *client) > goto error_clk_put; > } > > + ep = fwnode_graph_get_next_endpoint(dev_fwnode(&client->dev), > + NULL); > + if (!ep) { > + dev_err(&client->dev, "endpoint node not found\n"); > + ret = -EINVAL; > + goto error_clk_put; > + } > + > + /* For backward compatibility with the existing DT where > + * bus-type isn't specified v4l2_fwnode_endpoint_alloc_parse() > + * with bus_type set to V4L2_MBUS_PARALLEL falls back to > + * V4L2_MBUS_PARALLEL > + */ You can drop this comment block > + bus_cfg.bus_type = V4L2_MBUS_PARALLEL; > + ret = v4l2_fwnode_endpoint_alloc_parse(ep, &bus_cfg); > + priv->bus_type = bus_cfg.bus_type; Set this after if (ret) > + v4l2_fwnode_endpoint_free(&bus_cfg); > + fwnode_handle_put(ep); > + if (ret) > + goto error_clk_put; > + > ret = ov772x_video_probe(priv); > if (ret < 0) > goto error_gpio_put; > -- > 2.17.1 >
Hi Prabhakar, sorry, two more tiny nits On Wed, Sep 16, 2020 at 09:47:37AM +0200, Jacopo Mondi wrote: > Hi Prabhakar, > > On Tue, Sep 15, 2020 at 06:42:33PM +0100, Lad Prabhakar wrote: > > Parse endpoint properties using v4l2_fwnode_endpoint_alloc_parse() > > to determine bus-type and store it locally in priv data. > > > > v4l2_fwnode_endpoint_alloc_parse() with bus_type set to > > V4L2_MBUS_PARALLEL falls back to V4L2_MBUS_PARALLEL thus handling > > backward compatibility with existing DT where bus-type isn't specified. > > > I don't think this is necessary here. This patch does not need to > handle any retrocompatibility, as only PARALLEL is supported. > > The 'right' way to put it to me would be > "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" > > See comment in the next patch for retrocompatibility > > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > --- > > drivers/media/i2c/ov772x.c | 25 +++++++++++++++++++++++++ > > 1 file changed, 25 insertions(+) > > > > diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c > > index 2cc6a678069a..4ab4b3c883d0 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; > > }; > > > > /* > > @@ -1354,6 +1356,8 @@ static const struct v4l2_subdev_ops ov772x_subdev_ops = { > > > > static int ov772x_probe(struct i2c_client *client) > > { > > + struct v4l2_fwnode_endpoint bus_cfg; > > + struct fwnode_handle *ep; > > struct ov772x_priv *priv; > > int ret; > > static const struct regmap_config ov772x_regmap_config = { > > @@ -1415,6 +1419,27 @@ static int ov772x_probe(struct i2c_client *client) > > goto error_clk_put; > > } > > > > + ep = fwnode_graph_get_next_endpoint(dev_fwnode(&client->dev), > > + NULL); > > + if (!ep) { > > + dev_err(&client->dev, "endpoint node not found\n"); Nit: other error messages in the driver start with a capital letter, > > + ret = -EINVAL; > > + goto error_clk_put; > > + } > > + > > + /* For backward compatibility with the existing DT where > > + * bus-type isn't specified v4l2_fwnode_endpoint_alloc_parse() > > + * with bus_type set to V4L2_MBUS_PARALLEL falls back to > > + * V4L2_MBUS_PARALLEL > > + */ > > You can drop this comment block > Or better move it to the next patch. Two nits in the meantime: Use /* * This in place of /* This And I would write it as something like /* * For backward compatibility with older DTS where the * bus-type property was not mandatory, assume * V4L2_MBUS_PARALLEL as it was the only supported bus at the * time. v4l2_fwnode_endpoint_alloc_parse() will not fail if * 'bus-type' is not specified. */ Thanks j > > + bus_cfg.bus_type = V4L2_MBUS_PARALLEL; > > + ret = v4l2_fwnode_endpoint_alloc_parse(ep, &bus_cfg); > > + priv->bus_type = bus_cfg.bus_type; > > Set this after if (ret) > > > + v4l2_fwnode_endpoint_free(&bus_cfg); > > + fwnode_handle_put(ep); > > + if (ret) > > + goto error_clk_put; > > + > > ret = ov772x_video_probe(priv); > > if (ret < 0) > > goto error_gpio_put; > > -- > > 2.17.1 > >
diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c index 2cc6a678069a..4ab4b3c883d0 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; }; /* @@ -1354,6 +1356,8 @@ static const struct v4l2_subdev_ops ov772x_subdev_ops = { static int ov772x_probe(struct i2c_client *client) { + struct v4l2_fwnode_endpoint bus_cfg; + struct fwnode_handle *ep; struct ov772x_priv *priv; int ret; static const struct regmap_config ov772x_regmap_config = { @@ -1415,6 +1419,27 @@ static int ov772x_probe(struct i2c_client *client) goto error_clk_put; } + ep = fwnode_graph_get_next_endpoint(dev_fwnode(&client->dev), + NULL); + if (!ep) { + dev_err(&client->dev, "endpoint node not found\n"); + ret = -EINVAL; + goto error_clk_put; + } + + /* For backward compatibility with the existing DT where + * bus-type isn't specified v4l2_fwnode_endpoint_alloc_parse() + * with bus_type set to V4L2_MBUS_PARALLEL falls back to + * V4L2_MBUS_PARALLEL + */ + bus_cfg.bus_type = V4L2_MBUS_PARALLEL; + ret = v4l2_fwnode_endpoint_alloc_parse(ep, &bus_cfg); + priv->bus_type = bus_cfg.bus_type; + v4l2_fwnode_endpoint_free(&bus_cfg); + fwnode_handle_put(ep); + if (ret) + goto error_clk_put; + ret = ov772x_video_probe(priv); if (ret < 0) goto error_gpio_put;
Parse endpoint properties using v4l2_fwnode_endpoint_alloc_parse() to determine bus-type and store it locally in priv data. v4l2_fwnode_endpoint_alloc_parse() with bus_type set to V4L2_MBUS_PARALLEL falls back to V4L2_MBUS_PARALLEL thus handling backward compatibility with existing DT where bus-type isn't specified. Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> --- drivers/media/i2c/ov772x.c | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+)