Re: [PATCH v3 1/2] media: i2c: ov772x: Add support for BT656 mode
On Fri, Sep 04, 2020 at 04:48:32PM +0300, Laurent Pinchart wrote:
> On Fri, Sep 04, 2020 at 02:00:13PM +0300, Sakari Ailus wrote:
> > On Fri, Sep 04, 2020 at 12:35:50PM +0200, Jacopo Mondi wrote:
> > > On Fri, Sep 04, 2020 at 12:36:26PM +0300, Sakari Ailus wrote:
> > > > On Fri, Sep 04, 2020 at 11:20:49AM +0200, Jacopo Mondi wrote:
> > > > > On Fri, Sep 04, 2020 at 11:21:04AM +0300, Sakari Ailus wrote:
> > > > > > On Fri, Sep 04, 2020 at 09:55:53AM +0200, Jacopo Mondi wrote:
> > > > > > > On Fri, Sep 04, 2020 at 04:20:00AM +0300, Laurent Pinchart wrote:
> > > > > > > > Hi Prabhakar,
> > > > > > > >
> > > > > > > > Thank you for the patch.
> > > > > > > >
> > > > > > > > On Mon, Aug 24, 2020 at 08:04:05PM +0100, Lad Prabhakar wrote:
> > > > > > > > > Add support to read the bus-type and enable BT656 mode if
> > > > > > > > > needed.
> > > > > > > > >
> > > > > > > > > Also fail probe if unsupported bus_type is detected.
> > > > > > > > >
> > > > > > > > > Signed-off-by: Lad Prabhakar
> > > > > > > > >
> > > > > > > > > Reviewed-by: Biju Das
> > > > > > > > > ---
> > > > > > > > > drivers/media/i2c/ov772x.c | 32
> > > > > > > > >
> > > > > > > > > 1 file changed, 32 insertions(+)
> > > > > > > > >
> > > > > > > > > diff --git a/drivers/media/i2c/ov772x.c
> > > > > > > > > b/drivers/media/i2c/ov772x.c
> > > > > > > > > index 2cc6a678069a..67764d647526 100644
> > > > > > > > > --- a/drivers/media/i2c/ov772x.c
> > > > > > > > > +++ b/drivers/media/i2c/ov772x.c
> > > > > > > > > @@ -31,6 +31,7 @@
> > > > > > > > > #include
> > > > > > > > > #include
> > > > > > > > > #include
> > > > > > > > > +#include
> > > > > > > > > #include
> > > > > > > > > #include
> > > > > > > > >
> > > > > > > > > @@ -434,6 +435,7 @@ struct ov772x_priv {
> > > > > > > > > #ifdef CONFIG_MEDIA_CONTROLLER
> > > > > > > > > struct media_pad pad;
> > > > > > > > > #endif
> > > > > > > > > + struct v4l2_fwnode_endpoint ep;
> > > > > > > > > };
> > > > > > > > >
> > > > > > > > > /*
> > > > > > > > > @@ -581,6 +583,13 @@ static int ov772x_s_stream(struct
> > > > > > > > > v4l2_subdev *sd, int enable)
> > > > > > > > > if (priv->streaming == enable)
> > > > > > > > > goto done;
> > > > > > > > >
> > > > > > > > > + if (priv->ep.bus_type == V4L2_MBUS_BT656) {
> > > > > > > > > + ret = regmaup_update_bits(priv->regmap, COM7,
> > > > > > > > > ITU656_ON_OFF,
> > > > > > > > > + enable ? ITU656_ON_OFF
> > > > > > > > > : ~ITU656_ON_OFF);
> > > > > > > > > + if (ret)
> > > > > > > > > + goto done;
> > > > > > > > > + }
> > > > > > > > > +
> > > > > > > > > ret = regmap_update_bits(priv->regmap, COM2,
> > > > > > > > > SOFT_SLEEP_MODE,
> > > > > > > > >enable ? 0 : SOFT_SLEEP_MODE);
> > > > > > > > > if (ret)
> > > > > > > > > @@ -1354,6 +1363,7 @@ static const struct v4l2_subdev_ops
> > > > > > > > > ov772x_subdev_ops = {
> > > > > > > > >
> > > > > > > > > static int ov772x_probe(struct i2c_client *client)
> > > > > > > > > {
> > > > > > > > > + struct fwnode_handle *endpoint;
> > > > > > > > > struct ov772x_priv *priv;
> > > > > > > > > int ret;
> > > > > > > > > static const struct regmap_config ov772x_regmap_config
> > > > > > > > > = {
> > > > > > > > > @@ -1415,6 +1425,28 @@ static int ov772x_probe(struct
> > > > > > > > > i2c_client *client)
> > > > > > > > > goto error_clk_put;
> > > > > > > > > }
> > > > > > > > >
> > > > > > > > > + endpoint =
> > > > > > > > > fwnode_graph_get_next_endpoint(dev_fwnode(>dev),
> > > > > > > > > + NULL);
> > > > > > > > > + if (!endpoint) {
> > > > > > > > > + dev_err(>dev, "endpoint node not
> > > > > > > > > found\n");
> > > > > > > > > + ret = -EINVAL;
> > > > > > > > > + goto error_clk_put;
> > > > > > > > > + }
> > > > > > > > > +
> > > > > > > > > + ret = v4l2_fwnode_endpoint_parse(endpoint, >ep);
> > > > > > > >
> > > > > > > > v4l2_fwnode_endpoint_parse() is deprecated for new drivers,
> > > > > > > > v4l2_fwnode_endpoint_alloc_parse() is recommended instead.
> > > > > > > > Please note
> > > > > > > > that v4l2_fwnode_endpoint_free() then needs to be called in the
> > > > > > > > error
> > > > > > > > path and in remove().
> > > > > > >
> > > > > > > Doesn't alloc_parse() differ from just _parse() as it reserve
> > > > > > > space
> > > > > > > for the 'link-frequencies' array ? As this device does not support
> > > > > > > CSI-2 and the 'link-frequencies' property is not allows in
> > > > > > > bindings,
> > > > > > > isn't using endpoint_parse() better as it saves a call to _free()
> > > > > > > ?
> > > > > >
> > > > > > Yeah. I think the documentation needs to be updated.
> > > > > >
> > > > > > The
Re: [PATCH v3 1/2] media: i2c: ov772x: Add support for BT656 mode
On Fri, Sep 04, 2020 at 02:00:13PM +0300, Sakari Ailus wrote:
> On Fri, Sep 04, 2020 at 12:35:50PM +0200, Jacopo Mondi wrote:
> > On Fri, Sep 04, 2020 at 12:36:26PM +0300, Sakari Ailus wrote:
> > > On Fri, Sep 04, 2020 at 11:20:49AM +0200, Jacopo Mondi wrote:
> > > > On Fri, Sep 04, 2020 at 11:21:04AM +0300, Sakari Ailus wrote:
> > > > > On Fri, Sep 04, 2020 at 09:55:53AM +0200, Jacopo Mondi wrote:
> > > > > > On Fri, Sep 04, 2020 at 04:20:00AM +0300, Laurent Pinchart wrote:
> > > > > > > Hi Prabhakar,
> > > > > > >
> > > > > > > Thank you for the patch.
> > > > > > >
> > > > > > > On Mon, Aug 24, 2020 at 08:04:05PM +0100, Lad Prabhakar wrote:
> > > > > > > > Add support to read the bus-type and enable BT656 mode if
> > > > > > > > needed.
> > > > > > > >
> > > > > > > > Also fail probe if unsupported bus_type is detected.
> > > > > > > >
> > > > > > > > Signed-off-by: Lad Prabhakar
> > > > > > > >
> > > > > > > > Reviewed-by: Biju Das
> > > > > > > > ---
> > > > > > > > drivers/media/i2c/ov772x.c | 32
> > > > > > > >
> > > > > > > > 1 file changed, 32 insertions(+)
> > > > > > > >
> > > > > > > > diff --git a/drivers/media/i2c/ov772x.c
> > > > > > > > b/drivers/media/i2c/ov772x.c
> > > > > > > > index 2cc6a678069a..67764d647526 100644
> > > > > > > > --- a/drivers/media/i2c/ov772x.c
> > > > > > > > +++ b/drivers/media/i2c/ov772x.c
> > > > > > > > @@ -31,6 +31,7 @@
> > > > > > > > #include
> > > > > > > > #include
> > > > > > > > #include
> > > > > > > > +#include
> > > > > > > > #include
> > > > > > > > #include
> > > > > > > >
> > > > > > > > @@ -434,6 +435,7 @@ struct ov772x_priv {
> > > > > > > > #ifdef CONFIG_MEDIA_CONTROLLER
> > > > > > > > struct media_pad pad;
> > > > > > > > #endif
> > > > > > > > + struct v4l2_fwnode_endpoint ep;
> > > > > > > > };
> > > > > > > >
> > > > > > > > /*
> > > > > > > > @@ -581,6 +583,13 @@ static int ov772x_s_stream(struct
> > > > > > > > v4l2_subdev *sd, int enable)
> > > > > > > > if (priv->streaming == enable)
> > > > > > > > goto done;
> > > > > > > >
> > > > > > > > + if (priv->ep.bus_type == V4L2_MBUS_BT656) {
> > > > > > > > + ret = regmaup_update_bits(priv->regmap, COM7,
> > > > > > > > ITU656_ON_OFF,
> > > > > > > > +enable ? ITU656_ON_OFF
> > > > > > > > : ~ITU656_ON_OFF);
> > > > > > > > + if (ret)
> > > > > > > > + goto done;
> > > > > > > > + }
> > > > > > > > +
> > > > > > > > ret = regmap_update_bits(priv->regmap, COM2,
> > > > > > > > SOFT_SLEEP_MODE,
> > > > > > > > enable ? 0 : SOFT_SLEEP_MODE);
> > > > > > > > if (ret)
> > > > > > > > @@ -1354,6 +1363,7 @@ static const struct v4l2_subdev_ops
> > > > > > > > ov772x_subdev_ops = {
> > > > > > > >
> > > > > > > > static int ov772x_probe(struct i2c_client *client)
> > > > > > > > {
> > > > > > > > + struct fwnode_handle *endpoint;
> > > > > > > > struct ov772x_priv *priv;
> > > > > > > > int ret;
> > > > > > > > static const struct regmap_config ov772x_regmap_config
> > > > > > > > = {
> > > > > > > > @@ -1415,6 +1425,28 @@ static int ov772x_probe(struct
> > > > > > > > i2c_client *client)
> > > > > > > > goto error_clk_put;
> > > > > > > > }
> > > > > > > >
> > > > > > > > + endpoint =
> > > > > > > > fwnode_graph_get_next_endpoint(dev_fwnode(>dev),
> > > > > > > > + NULL);
> > > > > > > > + if (!endpoint) {
> > > > > > > > + dev_err(>dev, "endpoint node not
> > > > > > > > found\n");
> > > > > > > > + ret = -EINVAL;
> > > > > > > > + goto error_clk_put;
> > > > > > > > + }
> > > > > > > > +
> > > > > > > > + ret = v4l2_fwnode_endpoint_parse(endpoint, >ep);
> > > > > > >
> > > > > > > v4l2_fwnode_endpoint_parse() is deprecated for new drivers,
> > > > > > > v4l2_fwnode_endpoint_alloc_parse() is recommended instead. Please
> > > > > > > note
> > > > > > > that v4l2_fwnode_endpoint_free() then needs to be called in the
> > > > > > > error
> > > > > > > path and in remove().
> > > > > >
> > > > > > Doesn't alloc_parse() differ from just _parse() as it reserve space
> > > > > > for the 'link-frequencies' array ? As this device does not support
> > > > > > CSI-2 and the 'link-frequencies' property is not allows in bindings,
> > > > > > isn't using endpoint_parse() better as it saves a call to _free() ?
> > > > >
> > > > > Yeah. I think the documentation needs to be updated.
> > > > >
> > > > > The thinking was there would be other variable size properties that
> > > > > drivers
> > > > > would need but that didn't happen. So feel free to continue use
> > > > > v4l2_fwnode_endpoint_parse() where it does the job.
> > > > >
> > > > > >
> > > > > > Or are we
Re: [PATCH v3 1/2] media: i2c: ov772x: Add support for BT656 mode
Hello,
On Fri, Sep 04, 2020 at 11:21:04AM +0300, Sakari Ailus wrote:
> On Fri, Sep 04, 2020 at 09:55:53AM +0200, Jacopo Mondi wrote:
> > On Fri, Sep 04, 2020 at 04:20:00AM +0300, Laurent Pinchart wrote:
> > > On Mon, Aug 24, 2020 at 08:04:05PM +0100, Lad Prabhakar wrote:
> > > > Add support to read the bus-type and enable BT656 mode if needed.
> > > >
> > > > Also fail probe if unsupported bus_type is detected.
> > > >
> > > > Signed-off-by: Lad Prabhakar
> > > > Reviewed-by: Biju Das
> > > > ---
> > > > drivers/media/i2c/ov772x.c | 32
> > > > 1 file changed, 32 insertions(+)
> > > >
> > > > diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c
> > > > index 2cc6a678069a..67764d647526 100644
> > > > --- a/drivers/media/i2c/ov772x.c
> > > > +++ b/drivers/media/i2c/ov772x.c
> > > > @@ -31,6 +31,7 @@
> > > > #include
> > > > #include
> > > > #include
> > > > +#include
> > > > #include
> > > > #include
> > > >
> > > > @@ -434,6 +435,7 @@ struct ov772x_priv {
> > > > #ifdef CONFIG_MEDIA_CONTROLLER
> > > > struct media_pad pad;
> > > > #endif
> > > > + struct v4l2_fwnode_endpoint ep;
> > > > };
> > > >
> > > > /*
> > > > @@ -581,6 +583,13 @@ static int ov772x_s_stream(struct v4l2_subdev *sd,
> > > > int enable)
> > > > if (priv->streaming == enable)
> > > > goto done;
> > > >
> > > > + if (priv->ep.bus_type == V4L2_MBUS_BT656) {
> > > > + ret = regmap_update_bits(priv->regmap, COM7,
> > > > ITU656_ON_OFF,
> > > > +enable ? ITU656_ON_OFF :
> > > > ~ITU656_ON_OFF);
> > > > + if (ret)
> > > > + goto done;
> > > > + }
> > > > +
> > > > ret = regmap_update_bits(priv->regmap, COM2, SOFT_SLEEP_MODE,
> > > > enable ? 0 : SOFT_SLEEP_MODE);
> > > > if (ret)
> > > > @@ -1354,6 +1363,7 @@ static const struct v4l2_subdev_ops
> > > > ov772x_subdev_ops = {
> > > >
> > > > static int ov772x_probe(struct i2c_client *client)
> > > > {
> > > > + struct fwnode_handle *endpoint;
> > > > struct ov772x_priv *priv;
> > > > int ret;
> > > > static const struct regmap_config ov772x_regmap_config = {
> > > > @@ -1415,6 +1425,28 @@ static int ov772x_probe(struct i2c_client
> > > > *client)
> > > > goto error_clk_put;
> > > > }
> > > >
> > > > + endpoint =
> > > > fwnode_graph_get_next_endpoint(dev_fwnode(>dev),
> > > > + NULL);
> > > > + if (!endpoint) {
> > > > + dev_err(>dev, "endpoint node not found\n");
> > > > + ret = -EINVAL;
> > > > + goto error_clk_put;
> > > > + }
> > > > +
> > > > + ret = v4l2_fwnode_endpoint_parse(endpoint, >ep);
> > >
> > > v4l2_fwnode_endpoint_parse() is deprecated for new drivers,
> > > v4l2_fwnode_endpoint_alloc_parse() is recommended instead. Please note
> > > that v4l2_fwnode_endpoint_free() then needs to be called in the error
> > > path and in remove().
> >
> > Doesn't alloc_parse() differ from just _parse() as it reserve space
> > for the 'link-frequencies' array ? As this device does not support
> > CSI-2 and the 'link-frequencies' property is not allows in bindings,
> > isn't using endpoint_parse() better as it saves a call to _free() ?
>
> Yeah. I think the documentation needs to be updated.
>
> The thinking was there would be other variable size properties that drivers
> would need but that didn't happen. So feel free to continue use
> v4l2_fwnode_endpoint_parse() where it does the job.
I thought the goal was to remove v4l2_fwnode_endpoint_parse() in the
future. If Sakari is happy to keep it and see it used for parallel
buses, I have no objection.
> > Or are we deprecating that function unconditionally ? The
> > documentation suggests "please use v4l2_fwnode_endpoint_alloc_parse()
> > in new drivers" but here it doesn't seem required..
> >
> > > On the other hand, not setting .bus_type and letting the parse()
> > > function determine the but type automatically is also deprecated, and I
> > > don't think forcing drivers to call v4l2_fwnode_endpoint_alloc_parse()
> > > once for each bus type until one succeeds is a good API. As change will
> > > be needed in that API, you can ignore v4l2_fwnode_endpoint_alloc_parse()
> > > for the time being if you want.
> >
> > But indeed relying on auto-guessing of the bus type is deprecated since
> > some time now (and the API could be improved, yes). Sorry I missed
> > that yesterday.
>
> There's one case where the bus type does not need to be set: when bindings
> require it *and* at the same time you have no default configuration that
> requires something to be set in the bus specific struct. Bindings where
> bus-type is required were added later so I think the documentation should
> be changed there,
Re: [PATCH v3 1/2] media: i2c: ov772x: Add support for BT656 mode
Hi Jacopo,
On Fri, Sep 04, 2020 at 12:35:50PM +0200, Jacopo Mondi wrote:
> Hi Sakari,
>
> On Fri, Sep 04, 2020 at 12:36:26PM +0300, Sakari Ailus wrote:
> > On Fri, Sep 04, 2020 at 11:20:49AM +0200, Jacopo Mondi wrote:
> > > Hi Sakari,
> > >
> > > On Fri, Sep 04, 2020 at 11:21:04AM +0300, Sakari Ailus wrote:
> > > > Hi Laurent, Jacopo,
> > > >
> > > > On Fri, Sep 04, 2020 at 09:55:53AM +0200, Jacopo Mondi wrote:
> > > > > Hi Laurent,
> > > > >
> > > > > On Fri, Sep 04, 2020 at 04:20:00AM +0300, Laurent Pinchart wrote:
> > > > > > Hi Prabhakar,
> > > > > >
> > > > > > Thank you for the patch.
> > > > > >
> > > > > > On Mon, Aug 24, 2020 at 08:04:05PM +0100, Lad Prabhakar wrote:
> > > > > > > Add support to read the bus-type and enable BT656 mode if needed.
> > > > > > >
> > > > > > > Also fail probe if unsupported bus_type is detected.
> > > > > > >
> > > > > > > Signed-off-by: Lad Prabhakar
> > > > > > >
> > > > > > > Reviewed-by: Biju Das
> > > > > > > ---
> > > > > > > drivers/media/i2c/ov772x.c | 32
> > > > > > > 1 file changed, 32 insertions(+)
> > > > > > >
> > > > > > > diff --git a/drivers/media/i2c/ov772x.c
> > > > > > > b/drivers/media/i2c/ov772x.c
> > > > > > > index 2cc6a678069a..67764d647526 100644
> > > > > > > --- a/drivers/media/i2c/ov772x.c
> > > > > > > +++ b/drivers/media/i2c/ov772x.c
> > > > > > > @@ -31,6 +31,7 @@
> > > > > > > #include
> > > > > > > #include
> > > > > > > #include
> > > > > > > +#include
> > > > > > > #include
> > > > > > > #include
> > > > > > >
> > > > > > > @@ -434,6 +435,7 @@ struct ov772x_priv {
> > > > > > > #ifdef CONFIG_MEDIA_CONTROLLER
> > > > > > > struct media_pad pad;
> > > > > > > #endif
> > > > > > > + struct v4l2_fwnode_endpoint ep;
> > > > > > > };
> > > > > > >
> > > > > > > /*
> > > > > > > @@ -581,6 +583,13 @@ static int ov772x_s_stream(struct
> > > > > > > v4l2_subdev *sd, int enable)
> > > > > > > if (priv->streaming == enable)
> > > > > > > goto done;
> > > > > > >
> > > > > > > + if (priv->ep.bus_type == V4L2_MBUS_BT656) {
> > > > > > > + ret = regmap_update_bits(priv->regmap, COM7,
> > > > > > > ITU656_ON_OFF,
> > > > > > > + enable ? ITU656_ON_OFF :
> > > > > > > ~ITU656_ON_OFF);
> > > > > > > + if (ret)
> > > > > > > + goto done;
> > > > > > > + }
> > > > > > > +
> > > > > > > ret = regmap_update_bits(priv->regmap, COM2, SOFT_SLEEP_MODE,
> > > > > > >enable ? 0 : SOFT_SLEEP_MODE);
> > > > > > > if (ret)
> > > > > > > @@ -1354,6 +1363,7 @@ static const struct v4l2_subdev_ops
> > > > > > > ov772x_subdev_ops = {
> > > > > > >
> > > > > > > static int ov772x_probe(struct i2c_client *client)
> > > > > > > {
> > > > > > > + struct fwnode_handle *endpoint;
> > > > > > > struct ov772x_priv *priv;
> > > > > > > int ret;
> > > > > > > static const struct regmap_config ov772x_regmap_config = {
> > > > > > > @@ -1415,6 +1425,28 @@ static int ov772x_probe(struct i2c_client
> > > > > > > *client)
> > > > > > > goto error_clk_put;
> > > > > > > }
> > > > > > >
> > > > > > > + endpoint =
> > > > > > > fwnode_graph_get_next_endpoint(dev_fwnode(>dev),
> > > > > > > + NULL);
> > > > > > > + if (!endpoint) {
> > > > > > > + dev_err(>dev, "endpoint node not found\n");
> > > > > > > + ret = -EINVAL;
> > > > > > > + goto error_clk_put;
> > > > > > > + }
> > > > > > > +
> > > > > > > + ret = v4l2_fwnode_endpoint_parse(endpoint, >ep);
> > > > > >
> > > > > > v4l2_fwnode_endpoint_parse() is deprecated for new drivers,
> > > > > > v4l2_fwnode_endpoint_alloc_parse() is recommended instead. Please
> > > > > > note
> > > > > > that v4l2_fwnode_endpoint_free() then needs to be called in the
> > > > > > error
> > > > > > path and in remove().
> > > > >
> > > > > Doesn't alloc_parse() differ from just _parse() as it reserve space
> > > > > for the 'link-frequencies' array ? As this device does not support
> > > > > CSI-2 and the 'link-frequencies' property is not allows in bindings,
> > > > > isn't using endpoint_parse() better as it saves a call to _free() ?
> > > >
> > > > Yeah. I think the documentation needs to be updated.
> > > >
> > > > The thinking was there would be other variable size properties that
> > > > drivers
> > > > would need but that didn't happen. So feel free to continue use
> > > > v4l2_fwnode_endpoint_parse() where it does the job.
> > > >
> > > > >
> > > > > Or are we deprecating that function unconditionally ? The
> > > > > documentation suggests "please use v4l2_fwnode_endpoint_alloc_parse()
> > > > > in new drivers" but here it doesn't seem required..
> > > > >
> > > > > >
> > > > > > On the other hand, not setting .bus_type and letting the parse()
> > > > > > function determine the but type automatically is also deprecated,
> > > > > > and I
> > > > > > don't think
Re: [PATCH v3 1/2] media: i2c: ov772x: Add support for BT656 mode
Hi Sakari,
On Fri, Sep 04, 2020 at 12:36:26PM +0300, Sakari Ailus wrote:
> On Fri, Sep 04, 2020 at 11:20:49AM +0200, Jacopo Mondi wrote:
> > Hi Sakari,
> >
> > On Fri, Sep 04, 2020 at 11:21:04AM +0300, Sakari Ailus wrote:
> > > Hi Laurent, Jacopo,
> > >
> > > On Fri, Sep 04, 2020 at 09:55:53AM +0200, Jacopo Mondi wrote:
> > > > Hi Laurent,
> > > >
> > > > On Fri, Sep 04, 2020 at 04:20:00AM +0300, Laurent Pinchart wrote:
> > > > > Hi Prabhakar,
> > > > >
> > > > > Thank you for the patch.
> > > > >
> > > > > On Mon, Aug 24, 2020 at 08:04:05PM +0100, Lad Prabhakar wrote:
> > > > > > Add support to read the bus-type and enable BT656 mode if needed.
> > > > > >
> > > > > > Also fail probe if unsupported bus_type is detected.
> > > > > >
> > > > > > Signed-off-by: Lad Prabhakar
> > > > > >
> > > > > > Reviewed-by: Biju Das
> > > > > > ---
> > > > > > drivers/media/i2c/ov772x.c | 32
> > > > > > 1 file changed, 32 insertions(+)
> > > > > >
> > > > > > diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c
> > > > > > index 2cc6a678069a..67764d647526 100644
> > > > > > --- a/drivers/media/i2c/ov772x.c
> > > > > > +++ b/drivers/media/i2c/ov772x.c
> > > > > > @@ -31,6 +31,7 @@
> > > > > > #include
> > > > > > #include
> > > > > > #include
> > > > > > +#include
> > > > > > #include
> > > > > > #include
> > > > > >
> > > > > > @@ -434,6 +435,7 @@ struct ov772x_priv {
> > > > > > #ifdef CONFIG_MEDIA_CONTROLLER
> > > > > > struct media_pad pad;
> > > > > > #endif
> > > > > > + struct v4l2_fwnode_endpoint ep;
> > > > > > };
> > > > > >
> > > > > > /*
> > > > > > @@ -581,6 +583,13 @@ static int ov772x_s_stream(struct v4l2_subdev
> > > > > > *sd, int enable)
> > > > > > if (priv->streaming == enable)
> > > > > > goto done;
> > > > > >
> > > > > > + if (priv->ep.bus_type == V4L2_MBUS_BT656) {
> > > > > > + ret = regmap_update_bits(priv->regmap, COM7,
> > > > > > ITU656_ON_OFF,
> > > > > > +enable ? ITU656_ON_OFF :
> > > > > > ~ITU656_ON_OFF);
> > > > > > + if (ret)
> > > > > > + goto done;
> > > > > > + }
> > > > > > +
> > > > > > ret = regmap_update_bits(priv->regmap, COM2, SOFT_SLEEP_MODE,
> > > > > > enable ? 0 : SOFT_SLEEP_MODE);
> > > > > > if (ret)
> > > > > > @@ -1354,6 +1363,7 @@ static const struct v4l2_subdev_ops
> > > > > > ov772x_subdev_ops = {
> > > > > >
> > > > > > static int ov772x_probe(struct i2c_client *client)
> > > > > > {
> > > > > > + struct fwnode_handle *endpoint;
> > > > > > struct ov772x_priv *priv;
> > > > > > int ret;
> > > > > > static const struct regmap_config ov772x_regmap_config = {
> > > > > > @@ -1415,6 +1425,28 @@ static int ov772x_probe(struct i2c_client
> > > > > > *client)
> > > > > > goto error_clk_put;
> > > > > > }
> > > > > >
> > > > > > + endpoint =
> > > > > > fwnode_graph_get_next_endpoint(dev_fwnode(>dev),
> > > > > > + NULL);
> > > > > > + if (!endpoint) {
> > > > > > + dev_err(>dev, "endpoint node not found\n");
> > > > > > + ret = -EINVAL;
> > > > > > + goto error_clk_put;
> > > > > > + }
> > > > > > +
> > > > > > + ret = v4l2_fwnode_endpoint_parse(endpoint, >ep);
> > > > >
> > > > > v4l2_fwnode_endpoint_parse() is deprecated for new drivers,
> > > > > v4l2_fwnode_endpoint_alloc_parse() is recommended instead. Please note
> > > > > that v4l2_fwnode_endpoint_free() then needs to be called in the error
> > > > > path and in remove().
> > > >
> > > > Doesn't alloc_parse() differ from just _parse() as it reserve space
> > > > for the 'link-frequencies' array ? As this device does not support
> > > > CSI-2 and the 'link-frequencies' property is not allows in bindings,
> > > > isn't using endpoint_parse() better as it saves a call to _free() ?
> > >
> > > Yeah. I think the documentation needs to be updated.
> > >
> > > The thinking was there would be other variable size properties that
> > > drivers
> > > would need but that didn't happen. So feel free to continue use
> > > v4l2_fwnode_endpoint_parse() where it does the job.
> > >
> > > >
> > > > Or are we deprecating that function unconditionally ? The
> > > > documentation suggests "please use v4l2_fwnode_endpoint_alloc_parse()
> > > > in new drivers" but here it doesn't seem required..
> > > >
> > > > >
> > > > > On the other hand, not setting .bus_type and letting the parse()
> > > > > function determine the but type automatically is also deprecated, and
> > > > > I
> > > > > don't think forcing drivers to call v4l2_fwnode_endpoint_alloc_parse()
> > > > > once for each bus type until one succeeds is a good API. As change
> > > > > will
> > > > > be needed in that API, you can ignore
> > > > > v4l2_fwnode_endpoint_alloc_parse()
> > > > > for the time being if you want.
> > > >
>
Re: [PATCH v3 1/2] media: i2c: ov772x: Add support for BT656 mode
On Fri, Sep 04, 2020 at 11:20:49AM +0200, Jacopo Mondi wrote:
> Hi Sakari,
>
> On Fri, Sep 04, 2020 at 11:21:04AM +0300, Sakari Ailus wrote:
> > Hi Laurent, Jacopo,
> >
> > On Fri, Sep 04, 2020 at 09:55:53AM +0200, Jacopo Mondi wrote:
> > > Hi Laurent,
> > >
> > > On Fri, Sep 04, 2020 at 04:20:00AM +0300, Laurent Pinchart wrote:
> > > > Hi Prabhakar,
> > > >
> > > > Thank you for the patch.
> > > >
> > > > On Mon, Aug 24, 2020 at 08:04:05PM +0100, Lad Prabhakar wrote:
> > > > > Add support to read the bus-type and enable BT656 mode if needed.
> > > > >
> > > > > Also fail probe if unsupported bus_type is detected.
> > > > >
> > > > > Signed-off-by: Lad Prabhakar
> > > > > Reviewed-by: Biju Das
> > > > > ---
> > > > > drivers/media/i2c/ov772x.c | 32
> > > > > 1 file changed, 32 insertions(+)
> > > > >
> > > > > diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c
> > > > > index 2cc6a678069a..67764d647526 100644
> > > > > --- a/drivers/media/i2c/ov772x.c
> > > > > +++ b/drivers/media/i2c/ov772x.c
> > > > > @@ -31,6 +31,7 @@
> > > > > #include
> > > > > #include
> > > > > #include
> > > > > +#include
> > > > > #include
> > > > > #include
> > > > >
> > > > > @@ -434,6 +435,7 @@ struct ov772x_priv {
> > > > > #ifdef CONFIG_MEDIA_CONTROLLER
> > > > > struct media_pad pad;
> > > > > #endif
> > > > > + struct v4l2_fwnode_endpoint ep;
> > > > > };
> > > > >
> > > > > /*
> > > > > @@ -581,6 +583,13 @@ static int ov772x_s_stream(struct v4l2_subdev
> > > > > *sd, int enable)
> > > > > if (priv->streaming == enable)
> > > > > goto done;
> > > > >
> > > > > + if (priv->ep.bus_type == V4L2_MBUS_BT656) {
> > > > > + ret = regmap_update_bits(priv->regmap, COM7,
> > > > > ITU656_ON_OFF,
> > > > > + enable ? ITU656_ON_OFF :
> > > > > ~ITU656_ON_OFF);
> > > > > + if (ret)
> > > > > + goto done;
> > > > > + }
> > > > > +
> > > > > ret = regmap_update_bits(priv->regmap, COM2, SOFT_SLEEP_MODE,
> > > > >enable ? 0 : SOFT_SLEEP_MODE);
> > > > > if (ret)
> > > > > @@ -1354,6 +1363,7 @@ static const struct v4l2_subdev_ops
> > > > > ov772x_subdev_ops = {
> > > > >
> > > > > static int ov772x_probe(struct i2c_client *client)
> > > > > {
> > > > > + struct fwnode_handle *endpoint;
> > > > > struct ov772x_priv *priv;
> > > > > int ret;
> > > > > static const struct regmap_config ov772x_regmap_config = {
> > > > > @@ -1415,6 +1425,28 @@ static int ov772x_probe(struct i2c_client
> > > > > *client)
> > > > > goto error_clk_put;
> > > > > }
> > > > >
> > > > > + endpoint =
> > > > > fwnode_graph_get_next_endpoint(dev_fwnode(>dev),
> > > > > + NULL);
> > > > > + if (!endpoint) {
> > > > > + dev_err(>dev, "endpoint node not found\n");
> > > > > + ret = -EINVAL;
> > > > > + goto error_clk_put;
> > > > > + }
> > > > > +
> > > > > + ret = v4l2_fwnode_endpoint_parse(endpoint, >ep);
> > > >
> > > > v4l2_fwnode_endpoint_parse() is deprecated for new drivers,
> > > > v4l2_fwnode_endpoint_alloc_parse() is recommended instead. Please note
> > > > that v4l2_fwnode_endpoint_free() then needs to be called in the error
> > > > path and in remove().
> > >
> > > Doesn't alloc_parse() differ from just _parse() as it reserve space
> > > for the 'link-frequencies' array ? As this device does not support
> > > CSI-2 and the 'link-frequencies' property is not allows in bindings,
> > > isn't using endpoint_parse() better as it saves a call to _free() ?
> >
> > Yeah. I think the documentation needs to be updated.
> >
> > The thinking was there would be other variable size properties that drivers
> > would need but that didn't happen. So feel free to continue use
> > v4l2_fwnode_endpoint_parse() where it does the job.
> >
> > >
> > > Or are we deprecating that function unconditionally ? The
> > > documentation suggests "please use v4l2_fwnode_endpoint_alloc_parse()
> > > in new drivers" but here it doesn't seem required..
> > >
> > > >
> > > > On the other hand, not setting .bus_type and letting the parse()
> > > > function determine the but type automatically is also deprecated, and I
> > > > don't think forcing drivers to call v4l2_fwnode_endpoint_alloc_parse()
> > > > once for each bus type until one succeeds is a good API. As change will
> > > > be needed in that API, you can ignore v4l2_fwnode_endpoint_alloc_parse()
> > > > for the time being if you want.
> > >
> > > But indeed relying on auto-guessing of the bus type is deprecated since
> > > some time now (and the API could be improved, yes). Sorry I missed
> > > that yesterday.
> >
> > There's one case where the bus type does not need to be set: when bindings
> > require it *and* at the same time you have no
Re: [PATCH v3 1/2] media: i2c: ov772x: Add support for BT656 mode
Hi Sakari,
On Fri, Sep 04, 2020 at 11:21:04AM +0300, Sakari Ailus wrote:
> Hi Laurent, Jacopo,
>
> On Fri, Sep 04, 2020 at 09:55:53AM +0200, Jacopo Mondi wrote:
> > Hi Laurent,
> >
> > On Fri, Sep 04, 2020 at 04:20:00AM +0300, Laurent Pinchart wrote:
> > > Hi Prabhakar,
> > >
> > > Thank you for the patch.
> > >
> > > On Mon, Aug 24, 2020 at 08:04:05PM +0100, Lad Prabhakar wrote:
> > > > Add support to read the bus-type and enable BT656 mode if needed.
> > > >
> > > > Also fail probe if unsupported bus_type is detected.
> > > >
> > > > Signed-off-by: Lad Prabhakar
> > > > Reviewed-by: Biju Das
> > > > ---
> > > > drivers/media/i2c/ov772x.c | 32
> > > > 1 file changed, 32 insertions(+)
> > > >
> > > > diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c
> > > > index 2cc6a678069a..67764d647526 100644
> > > > --- a/drivers/media/i2c/ov772x.c
> > > > +++ b/drivers/media/i2c/ov772x.c
> > > > @@ -31,6 +31,7 @@
> > > > #include
> > > > #include
> > > > #include
> > > > +#include
> > > > #include
> > > > #include
> > > >
> > > > @@ -434,6 +435,7 @@ struct ov772x_priv {
> > > > #ifdef CONFIG_MEDIA_CONTROLLER
> > > > struct media_pad pad;
> > > > #endif
> > > > + struct v4l2_fwnode_endpoint ep;
> > > > };
> > > >
> > > > /*
> > > > @@ -581,6 +583,13 @@ static int ov772x_s_stream(struct v4l2_subdev *sd,
> > > > int enable)
> > > > if (priv->streaming == enable)
> > > > goto done;
> > > >
> > > > + if (priv->ep.bus_type == V4L2_MBUS_BT656) {
> > > > + ret = regmap_update_bits(priv->regmap, COM7,
> > > > ITU656_ON_OFF,
> > > > +enable ? ITU656_ON_OFF :
> > > > ~ITU656_ON_OFF);
> > > > + if (ret)
> > > > + goto done;
> > > > + }
> > > > +
> > > > ret = regmap_update_bits(priv->regmap, COM2, SOFT_SLEEP_MODE,
> > > > enable ? 0 : SOFT_SLEEP_MODE);
> > > > if (ret)
> > > > @@ -1354,6 +1363,7 @@ static const struct v4l2_subdev_ops
> > > > ov772x_subdev_ops = {
> > > >
> > > > static int ov772x_probe(struct i2c_client *client)
> > > > {
> > > > + struct fwnode_handle *endpoint;
> > > > struct ov772x_priv *priv;
> > > > int ret;
> > > > static const struct regmap_config ov772x_regmap_config = {
> > > > @@ -1415,6 +1425,28 @@ static int ov772x_probe(struct i2c_client
> > > > *client)
> > > > goto error_clk_put;
> > > > }
> > > >
> > > > + endpoint =
> > > > fwnode_graph_get_next_endpoint(dev_fwnode(>dev),
> > > > + NULL);
> > > > + if (!endpoint) {
> > > > + dev_err(>dev, "endpoint node not found\n");
> > > > + ret = -EINVAL;
> > > > + goto error_clk_put;
> > > > + }
> > > > +
> > > > + ret = v4l2_fwnode_endpoint_parse(endpoint, >ep);
> > >
> > > v4l2_fwnode_endpoint_parse() is deprecated for new drivers,
> > > v4l2_fwnode_endpoint_alloc_parse() is recommended instead. Please note
> > > that v4l2_fwnode_endpoint_free() then needs to be called in the error
> > > path and in remove().
> >
> > Doesn't alloc_parse() differ from just _parse() as it reserve space
> > for the 'link-frequencies' array ? As this device does not support
> > CSI-2 and the 'link-frequencies' property is not allows in bindings,
> > isn't using endpoint_parse() better as it saves a call to _free() ?
>
> Yeah. I think the documentation needs to be updated.
>
> The thinking was there would be other variable size properties that drivers
> would need but that didn't happen. So feel free to continue use
> v4l2_fwnode_endpoint_parse() where it does the job.
>
> >
> > Or are we deprecating that function unconditionally ? The
> > documentation suggests "please use v4l2_fwnode_endpoint_alloc_parse()
> > in new drivers" but here it doesn't seem required..
> >
> > >
> > > On the other hand, not setting .bus_type and letting the parse()
> > > function determine the but type automatically is also deprecated, and I
> > > don't think forcing drivers to call v4l2_fwnode_endpoint_alloc_parse()
> > > once for each bus type until one succeeds is a good API. As change will
> > > be needed in that API, you can ignore v4l2_fwnode_endpoint_alloc_parse()
> > > for the time being if you want.
> >
> > But indeed relying on auto-guessing of the bus type is deprecated since
> > some time now (and the API could be improved, yes). Sorry I missed
> > that yesterday.
>
> There's one case where the bus type does not need to be set: when bindings
> require it *and* at the same time you have no default configuration that
> requires something to be set in the bus specific struct. Bindings where
> bus-type is required were added later so I think the documentation should
> be changed there, too.
>
> I can send the patches.
>
> >
> > As we
Re: [PATCH v3 1/2] media: i2c: ov772x: Add support for BT656 mode
Hi Laurent, Jacopo,
On Fri, Sep 04, 2020 at 09:55:53AM +0200, Jacopo Mondi wrote:
> Hi Laurent,
>
> On Fri, Sep 04, 2020 at 04:20:00AM +0300, Laurent Pinchart wrote:
> > Hi Prabhakar,
> >
> > Thank you for the patch.
> >
> > On Mon, Aug 24, 2020 at 08:04:05PM +0100, Lad Prabhakar wrote:
> > > Add support to read the bus-type and enable BT656 mode if needed.
> > >
> > > Also fail probe if unsupported bus_type is detected.
> > >
> > > Signed-off-by: Lad Prabhakar
> > > Reviewed-by: Biju Das
> > > ---
> > > drivers/media/i2c/ov772x.c | 32
> > > 1 file changed, 32 insertions(+)
> > >
> > > diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c
> > > index 2cc6a678069a..67764d647526 100644
> > > --- a/drivers/media/i2c/ov772x.c
> > > +++ b/drivers/media/i2c/ov772x.c
> > > @@ -31,6 +31,7 @@
> > > #include
> > > #include
> > > #include
> > > +#include
> > > #include
> > > #include
> > >
> > > @@ -434,6 +435,7 @@ struct ov772x_priv {
> > > #ifdef CONFIG_MEDIA_CONTROLLER
> > > struct media_pad pad;
> > > #endif
> > > + struct v4l2_fwnode_endpoint ep;
> > > };
> > >
> > > /*
> > > @@ -581,6 +583,13 @@ static int ov772x_s_stream(struct v4l2_subdev *sd,
> > > int enable)
> > > if (priv->streaming == enable)
> > > goto done;
> > >
> > > + if (priv->ep.bus_type == V4L2_MBUS_BT656) {
> > > + ret = regmap_update_bits(priv->regmap, COM7, ITU656_ON_OFF,
> > > + enable ? ITU656_ON_OFF :
> > > ~ITU656_ON_OFF);
> > > + if (ret)
> > > + goto done;
> > > + }
> > > +
> > > ret = regmap_update_bits(priv->regmap, COM2, SOFT_SLEEP_MODE,
> > >enable ? 0 : SOFT_SLEEP_MODE);
> > > if (ret)
> > > @@ -1354,6 +1363,7 @@ static const struct v4l2_subdev_ops
> > > ov772x_subdev_ops = {
> > >
> > > static int ov772x_probe(struct i2c_client *client)
> > > {
> > > + struct fwnode_handle *endpoint;
> > > struct ov772x_priv *priv;
> > > int ret;
> > > static const struct regmap_config ov772x_regmap_config = {
> > > @@ -1415,6 +1425,28 @@ static int ov772x_probe(struct i2c_client *client)
> > > goto error_clk_put;
> > > }
> > >
> > > + endpoint = fwnode_graph_get_next_endpoint(dev_fwnode(>dev),
> > > + NULL);
> > > + if (!endpoint) {
> > > + dev_err(>dev, "endpoint node not found\n");
> > > + ret = -EINVAL;
> > > + goto error_clk_put;
> > > + }
> > > +
> > > + ret = v4l2_fwnode_endpoint_parse(endpoint, >ep);
> >
> > v4l2_fwnode_endpoint_parse() is deprecated for new drivers,
> > v4l2_fwnode_endpoint_alloc_parse() is recommended instead. Please note
> > that v4l2_fwnode_endpoint_free() then needs to be called in the error
> > path and in remove().
>
> Doesn't alloc_parse() differ from just _parse() as it reserve space
> for the 'link-frequencies' array ? As this device does not support
> CSI-2 and the 'link-frequencies' property is not allows in bindings,
> isn't using endpoint_parse() better as it saves a call to _free() ?
Yeah. I think the documentation needs to be updated.
The thinking was there would be other variable size properties that drivers
would need but that didn't happen. So feel free to continue use
v4l2_fwnode_endpoint_parse() where it does the job.
>
> Or are we deprecating that function unconditionally ? The
> documentation suggests "please use v4l2_fwnode_endpoint_alloc_parse()
> in new drivers" but here it doesn't seem required..
>
> >
> > On the other hand, not setting .bus_type and letting the parse()
> > function determine the but type automatically is also deprecated, and I
> > don't think forcing drivers to call v4l2_fwnode_endpoint_alloc_parse()
> > once for each bus type until one succeeds is a good API. As change will
> > be needed in that API, you can ignore v4l2_fwnode_endpoint_alloc_parse()
> > for the time being if you want.
>
> But indeed relying on auto-guessing of the bus type is deprecated since
> some time now (and the API could be improved, yes). Sorry I missed
> that yesterday.
There's one case where the bus type does not need to be set: when bindings
require it *and* at the same time you have no default configuration that
requires something to be set in the bus specific struct. Bindings where
bus-type is required were added later so I think the documentation should
be changed there, too.
I can send the patches.
>
> As we support parallel and bt.656 only I must be honest I don't mind
> it here as otherwise the code would be more complex for no real gain,
> but I defer this to Sakari which has been fighting the battle against
> auto-guessing since a long time now :)
I think you should require bus-type property in bindings in that case.
But as it's an existing driver, bus-type will be optional. You'll need to
default to what was supported earlier. This is actually an interesting case
as bindings do not document
Re: [PATCH v3 1/2] media: i2c: ov772x: Add support for BT656 mode
Hi Laurent,
On Fri, Sep 04, 2020 at 04:20:00AM +0300, Laurent Pinchart wrote:
> Hi Prabhakar,
>
> Thank you for the patch.
>
> On Mon, Aug 24, 2020 at 08:04:05PM +0100, Lad Prabhakar wrote:
> > Add support to read the bus-type and enable BT656 mode if needed.
> >
> > Also fail probe if unsupported bus_type is detected.
> >
> > Signed-off-by: Lad Prabhakar
> > Reviewed-by: Biju Das
> > ---
> > drivers/media/i2c/ov772x.c | 32
> > 1 file changed, 32 insertions(+)
> >
> > diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c
> > index 2cc6a678069a..67764d647526 100644
> > --- a/drivers/media/i2c/ov772x.c
> > +++ b/drivers/media/i2c/ov772x.c
> > @@ -31,6 +31,7 @@
> > #include
> > #include
> > #include
> > +#include
> > #include
> > #include
> >
> > @@ -434,6 +435,7 @@ struct ov772x_priv {
> > #ifdef CONFIG_MEDIA_CONTROLLER
> > struct media_pad pad;
> > #endif
> > + struct v4l2_fwnode_endpoint ep;
> > };
> >
> > /*
> > @@ -581,6 +583,13 @@ static int ov772x_s_stream(struct v4l2_subdev *sd, int
> > enable)
> > if (priv->streaming == enable)
> > goto done;
> >
> > + if (priv->ep.bus_type == V4L2_MBUS_BT656) {
> > + ret = regmap_update_bits(priv->regmap, COM7, ITU656_ON_OFF,
> > +enable ? ITU656_ON_OFF :
> > ~ITU656_ON_OFF);
> > + if (ret)
> > + goto done;
> > + }
> > +
> > ret = regmap_update_bits(priv->regmap, COM2, SOFT_SLEEP_MODE,
> > enable ? 0 : SOFT_SLEEP_MODE);
> > if (ret)
> > @@ -1354,6 +1363,7 @@ static const struct v4l2_subdev_ops ov772x_subdev_ops
> > = {
> >
> > static int ov772x_probe(struct i2c_client *client)
> > {
> > + struct fwnode_handle *endpoint;
> > struct ov772x_priv *priv;
> > int ret;
> > static const struct regmap_config ov772x_regmap_config = {
> > @@ -1415,6 +1425,28 @@ static int ov772x_probe(struct i2c_client *client)
> > goto error_clk_put;
> > }
> >
> > + endpoint = fwnode_graph_get_next_endpoint(dev_fwnode(>dev),
> > + NULL);
> > + if (!endpoint) {
> > + dev_err(>dev, "endpoint node not found\n");
> > + ret = -EINVAL;
> > + goto error_clk_put;
> > + }
> > +
> > + ret = v4l2_fwnode_endpoint_parse(endpoint, >ep);
>
> v4l2_fwnode_endpoint_parse() is deprecated for new drivers,
> v4l2_fwnode_endpoint_alloc_parse() is recommended instead. Please note
> that v4l2_fwnode_endpoint_free() then needs to be called in the error
> path and in remove().
Doesn't alloc_parse() differ from just _parse() as it reserve space
for the 'link-frequencies' array ? As this device does not support
CSI-2 and the 'link-frequencies' property is not allows in bindings,
isn't using endpoint_parse() better as it saves a call to _free() ?
Or are we deprecating that function unconditionally ? The
documentation suggests "please use v4l2_fwnode_endpoint_alloc_parse()
in new drivers" but here it doesn't seem required..
>
> On the other hand, not setting .bus_type and letting the parse()
> function determine the but type automatically is also deprecated, and I
> don't think forcing drivers to call v4l2_fwnode_endpoint_alloc_parse()
> once for each bus type until one succeeds is a good API. As change will
> be needed in that API, you can ignore v4l2_fwnode_endpoint_alloc_parse()
> for the time being if you want.
But indeed relying on auto-guessing of the bus type is deprecated since
some time now (and the API could be improved, yes). Sorry I missed
that yesterday.
As we support parallel and bt.656 only I must be honest I don't mind
it here as otherwise the code would be more complex for no real gain,
but I defer this to Sakari which has been fighting the battle against
auto-guessing since a long time now :)
>
> Reviewed-by: Laurent Pinchart
>
> > + fwnode_handle_put(endpoint);
> > + if (ret) {
> > + dev_err(>dev, "Could not parse endpoint\n");
> > + goto error_clk_put;
> > + }
> > +
> > + if (priv->ep.bus_type != V4L2_MBUS_PARALLEL &&
> > + priv->ep.bus_type != V4L2_MBUS_BT656) {
> > + dev_err(>dev, "Unsupported bus type %d\n",
> > + priv->ep.bus_type);
> > + goto error_clk_put;
> > + }
> > +
> > ret = ov772x_video_probe(priv);
> > if (ret < 0)
> > goto error_gpio_put;
>
> --
> Regards,
>
> Laurent Pinchart
Re: [PATCH v3 1/2] media: i2c: ov772x: Add support for BT656 mode
Hi Prabhakar,
Thank you for the patch.
On Mon, Aug 24, 2020 at 08:04:05PM +0100, Lad Prabhakar wrote:
> Add support to read the bus-type and enable BT656 mode if needed.
>
> Also fail probe if unsupported bus_type is detected.
>
> Signed-off-by: Lad Prabhakar
> Reviewed-by: Biju Das
> ---
> drivers/media/i2c/ov772x.c | 32
> 1 file changed, 32 insertions(+)
>
> diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c
> index 2cc6a678069a..67764d647526 100644
> --- a/drivers/media/i2c/ov772x.c
> +++ b/drivers/media/i2c/ov772x.c
> @@ -31,6 +31,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
>
> @@ -434,6 +435,7 @@ struct ov772x_priv {
> #ifdef CONFIG_MEDIA_CONTROLLER
> struct media_pad pad;
> #endif
> + struct v4l2_fwnode_endpoint ep;
> };
>
> /*
> @@ -581,6 +583,13 @@ static int ov772x_s_stream(struct v4l2_subdev *sd, int
> enable)
> if (priv->streaming == enable)
> goto done;
>
> + if (priv->ep.bus_type == V4L2_MBUS_BT656) {
> + ret = regmap_update_bits(priv->regmap, COM7, ITU656_ON_OFF,
> + enable ? ITU656_ON_OFF :
> ~ITU656_ON_OFF);
> + if (ret)
> + goto done;
> + }
> +
> ret = regmap_update_bits(priv->regmap, COM2, SOFT_SLEEP_MODE,
>enable ? 0 : SOFT_SLEEP_MODE);
> if (ret)
> @@ -1354,6 +1363,7 @@ static const struct v4l2_subdev_ops ov772x_subdev_ops =
> {
>
> static int ov772x_probe(struct i2c_client *client)
> {
> + struct fwnode_handle *endpoint;
> struct ov772x_priv *priv;
> int ret;
> static const struct regmap_config ov772x_regmap_config = {
> @@ -1415,6 +1425,28 @@ static int ov772x_probe(struct i2c_client *client)
> goto error_clk_put;
> }
>
> + endpoint = fwnode_graph_get_next_endpoint(dev_fwnode(>dev),
> + NULL);
> + if (!endpoint) {
> + dev_err(>dev, "endpoint node not found\n");
> + ret = -EINVAL;
> + goto error_clk_put;
> + }
> +
> + ret = v4l2_fwnode_endpoint_parse(endpoint, >ep);
v4l2_fwnode_endpoint_parse() is deprecated for new drivers,
v4l2_fwnode_endpoint_alloc_parse() is recommended instead. Please note
that v4l2_fwnode_endpoint_free() then needs to be called in the error
path and in remove().
On the other hand, not setting .bus_type and letting the parse()
function determine the but type automatically is also deprecated, and I
don't think forcing drivers to call v4l2_fwnode_endpoint_alloc_parse()
once for each bus type until one succeeds is a good API. As change will
be needed in that API, you can ignore v4l2_fwnode_endpoint_alloc_parse()
for the time being if you want.
Reviewed-by: Laurent Pinchart
> + fwnode_handle_put(endpoint);
> + if (ret) {
> + dev_err(>dev, "Could not parse endpoint\n");
> + goto error_clk_put;
> + }
> +
> + if (priv->ep.bus_type != V4L2_MBUS_PARALLEL &&
> + priv->ep.bus_type != V4L2_MBUS_BT656) {
> + dev_err(>dev, "Unsupported bus type %d\n",
> + priv->ep.bus_type);
> + goto error_clk_put;
> + }
> +
> ret = ov772x_video_probe(priv);
> if (ret < 0)
> goto error_gpio_put;
--
Regards,
Laurent Pinchart
Re: [PATCH v3 1/2] media: i2c: ov772x: Add support for BT656 mode
Hi Prabhakar,
On Mon, Aug 24, 2020 at 08:04:05PM +0100, Lad Prabhakar wrote:
> Add support to read the bus-type and enable BT656 mode if needed.
>
> Also fail probe if unsupported bus_type is detected.
>
> Signed-off-by: Lad Prabhakar
> Reviewed-by: Biju Das
Looks good to me
Reviewed-by: Jacopo Mondi
Thanks
j
> ---
> drivers/media/i2c/ov772x.c | 32
> 1 file changed, 32 insertions(+)
>
> diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c
> index 2cc6a678069a..67764d647526 100644
> --- a/drivers/media/i2c/ov772x.c
> +++ b/drivers/media/i2c/ov772x.c
> @@ -31,6 +31,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
>
> @@ -434,6 +435,7 @@ struct ov772x_priv {
> #ifdef CONFIG_MEDIA_CONTROLLER
> struct media_pad pad;
> #endif
> + struct v4l2_fwnode_endpoint ep;
> };
>
> /*
> @@ -581,6 +583,13 @@ static int ov772x_s_stream(struct v4l2_subdev *sd, int
> enable)
> if (priv->streaming == enable)
> goto done;
>
> + if (priv->ep.bus_type == V4L2_MBUS_BT656) {
> + ret = regmap_update_bits(priv->regmap, COM7, ITU656_ON_OFF,
> + enable ? ITU656_ON_OFF :
> ~ITU656_ON_OFF);
> + if (ret)
> + goto done;
> + }
> +
> ret = regmap_update_bits(priv->regmap, COM2, SOFT_SLEEP_MODE,
>enable ? 0 : SOFT_SLEEP_MODE);
> if (ret)
> @@ -1354,6 +1363,7 @@ static const struct v4l2_subdev_ops ov772x_subdev_ops =
> {
>
> static int ov772x_probe(struct i2c_client *client)
> {
> + struct fwnode_handle *endpoint;
> struct ov772x_priv *priv;
> int ret;
> static const struct regmap_config ov772x_regmap_config = {
> @@ -1415,6 +1425,28 @@ static int ov772x_probe(struct i2c_client *client)
> goto error_clk_put;
> }
>
> + endpoint = fwnode_graph_get_next_endpoint(dev_fwnode(>dev),
> + NULL);
> + if (!endpoint) {
> + dev_err(>dev, "endpoint node not found\n");
> + ret = -EINVAL;
> + goto error_clk_put;
> + }
> +
> + ret = v4l2_fwnode_endpoint_parse(endpoint, >ep);
> + fwnode_handle_put(endpoint);
> + if (ret) {
> + dev_err(>dev, "Could not parse endpoint\n");
> + goto error_clk_put;
> + }
> +
> + if (priv->ep.bus_type != V4L2_MBUS_PARALLEL &&
> + priv->ep.bus_type != V4L2_MBUS_BT656) {
> + dev_err(>dev, "Unsupported bus type %d\n",
> + priv->ep.bus_type);
> + goto error_clk_put;
> + }
> +
> ret = ov772x_video_probe(priv);
> if (ret < 0)
> goto error_gpio_put;
> --
> 2.17.1
>
