LinuxLists.cc - [PATCH v3 0/2] media: i2c: ov772x: Enable BT656 mode and test pattern support

[permalink] [raw]

Subject: 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 <[email protected]>
> Reviewed-by: Biju Das <[email protected]>

Looks good to me
Reviewed-by: Jacopo Mondi <[email protected]>

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 <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
> + 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(&client->dev),
> + NULL);
> + if (!endpoint) {
> + dev_err(&client->dev, "endpoint node not found\n");
> + ret = -EINVAL;
> + goto error_clk_put;
> + }
> +
> + ret = v4l2_fwnode_endpoint_parse(endpoint, &priv->ep);
> + fwnode_handle_put(endpoint);
> + if (ret) {
> + dev_err(&client->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(&client->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
>

2020-09-03 16:10:37

[permalink] [raw]

Subject: Re: [PATCH v3 2/2] media: i2c: ov772x: Add test pattern control

On Mon, Aug 24, 2020 at 08:04:06PM +0100, Lad Prabhakar wrote:
> Add support for test pattern control supported by the sensor.
>
> Signed-off-by: Lad Prabhakar <[email protected]>
> Reviewed-by: Biju Das <[email protected]>
> ---
> drivers/media/i2c/ov772x.c | 25 ++++++++++++++++++++++++-
> include/media/i2c/ov772x.h | 1 +
> 2 files changed, 25 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c
> index 67764d647526..f267d8abd742 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;
> /* 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
> */
> @@ -762,6 +768,13 @@ static int ov772x_s_frame_interval(struct v4l2_subdev *sd,
> return ret;
> }
>
> +static int ov772x_enable_test_pattern(struct ov772x_priv *priv, u32 pattern)
> +{
> + priv->test_pattern = pattern;
> + return regmap_update_bits(priv->regmap, COM3, SCOLOR_TEST,
> + pattern ? SCOLOR_TEST : 0x00);
> +}
> +
> static int ov772x_s_ctrl(struct v4l2_ctrl *ctrl)
> {
> struct ov772x_priv *priv = container_of(ctrl->handler,
> @@ -809,6 +822,8 @@ static int ov772x_s_ctrl(struct v4l2_ctrl *ctrl)
> }
>
> return ret;
> + case V4L2_CID_TEST_PATTERN:
> + return ov772x_enable_test_pattern(priv, ctrl->val);

I think you should rather save ctrl->val in priv->test_patter here,
and then apply it at set_params() time (which is called at power on).
But I see the driver refusing to s_ctrl() and not calling
__v4l2_ctrl_handler_setup() at power up time, so I think the idea is
just to ignore controls set when the sensor is powered down..

> }
>
> return -EINVAL;
> @@ -1103,10 +1118,14 @@ static int ov772x_set_params(struct ov772x_priv *priv,
> val |= VFLIP_IMG;
> if (priv->info && (priv->info->flags & OV772X_FLAG_HFLIP))
> val |= HFLIP_IMG;
> + if (priv->info && (priv->info->flags & OV772X_FLAG_TEST_PATTERN))
> + val |= SCOLOR_TEST;
> if (priv->vflip_ctrl->val)
> val ^= VFLIP_IMG;
> if (priv->hflip_ctrl->val)
> val ^= HFLIP_IMG;
> + if (priv->test_pattern)
> + val ^= SCOLOR_TEST;

I'm not sure this is required to be honest.

For hflip/vflip the =^ serves to invert the v4l2-control meaning, as
the image is said to be already H/V flipped by the platform data (if I
got this part right).

For test pattern, do we want the same behaviour ? If enabled by
platform data then selecting "Vertical Color Bar Type 1" then disables
it ? I don't think so...

Anyway, minor issue. With this addressed
Reviewed-by: Jacopo Mondi <[email protected]>

Thanks
j

>
> ret = regmap_update_bits(priv->regmap, COM3, SWAP_MASK | IMG_MASK, val);
> if (ret < 0)
> @@ -1404,6 +1423,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;
> diff --git a/include/media/i2c/ov772x.h b/include/media/i2c/ov772x.h
> index a1702d420087..65e6f8d2f4bb 100644
> --- a/include/media/i2c/ov772x.h
> +++ b/include/media/i2c/ov772x.h
> @@ -12,6 +12,7 @@
> /* for flags */
> #define OV772X_FLAG_VFLIP (1 << 0) /* Vertical flip image */
> #define OV772X_FLAG_HFLIP (1 << 1) /* Horizontal flip image */
> +#define OV772X_FLAG_TEST_PATTERN (1 << 2) /* Test pattern */
>
> /*
> * for Edge ctrl
> --
> 2.17.1
>

2020-09-04 01:22:33

[permalink] [raw]

Subject: 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 <[email protected]>
> Reviewed-by: Biju Das <[email protected]>
> ---
> 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 <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
> + 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(&client->dev),
> + NULL);
> + if (!endpoint) {
> + dev_err(&client->dev, "endpoint node not found\n");
> + ret = -EINVAL;
> + goto error_clk_put;
> + }
> +
> + ret = v4l2_fwnode_endpoint_parse(endpoint, &priv->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 <[email protected]>

> + fwnode_handle_put(endpoint);
> + if (ret) {
> + dev_err(&client->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(&client->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

2020-09-04 01:30:34

[permalink] [raw]

Subject: Re: [PATCH v3 2/2] media: i2c: ov772x: Add test pattern control

Hi Prabhakar,

Thank you for the patch.

On Mon, Aug 24, 2020 at 08:04:06PM +0100, Lad Prabhakar wrote:
> Add support for test pattern control supported by the sensor.
>
> Signed-off-by: Lad Prabhakar <[email protected]>
> Reviewed-by: Biju Das <[email protected]>
> ---
> drivers/media/i2c/ov772x.c | 25 ++++++++++++++++++++++++-
> include/media/i2c/ov772x.h | 1 +
> 2 files changed, 25 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c
> index 67764d647526..f267d8abd742 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;
> /* 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
> */
> @@ -762,6 +768,13 @@ static int ov772x_s_frame_interval(struct v4l2_subdev *sd,
> return ret;
> }
>
> +static int ov772x_enable_test_pattern(struct ov772x_priv *priv, u32 pattern)
> +{
> + priv->test_pattern = pattern;
> + return regmap_update_bits(priv->regmap, COM3, SCOLOR_TEST,
> + pattern ? SCOLOR_TEST : 0x00);
> +}
> +
> static int ov772x_s_ctrl(struct v4l2_ctrl *ctrl)
> {
> struct ov772x_priv *priv = container_of(ctrl->handler,
> @@ -809,6 +822,8 @@ static int ov772x_s_ctrl(struct v4l2_ctrl *ctrl)
> }
>
> return ret;
> + case V4L2_CID_TEST_PATTERN:
> + return ov772x_enable_test_pattern(priv, ctrl->val);
> }
>
> return -EINVAL;
> @@ -1103,10 +1118,14 @@ static int ov772x_set_params(struct ov772x_priv *priv,
> val |= VFLIP_IMG;
> if (priv->info && (priv->info->flags & OV772X_FLAG_HFLIP))
> val |= HFLIP_IMG;
> + if (priv->info && (priv->info->flags & OV772X_FLAG_TEST_PATTERN))
> + val |= SCOLOR_TEST;

Why do you need this new flag ? First of all flags are only used on
systems that use platform data, which has been deprecated in favour of
DT for a long time, and then I don't see a need to enable the test
pattern from platform data.

> if (priv->vflip_ctrl->val)
> val ^= VFLIP_IMG;
> if (priv->hflip_ctrl->val)
> val ^= HFLIP_IMG;
> + if (priv->test_pattern)
> + val ^= SCOLOR_TEST;

It would be nice if we could replace the manual handling of controls in
this function by a call to v4l2_ctrl_handler_setup(), but that's a
candidate for a separate patch.

>
> ret = regmap_update_bits(priv->regmap, COM3, SWAP_MASK | IMG_MASK, val);
> if (ret < 0)
> @@ -1404,6 +1423,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;
> diff --git a/include/media/i2c/ov772x.h b/include/media/i2c/ov772x.h
> index a1702d420087..65e6f8d2f4bb 100644
> --- a/include/media/i2c/ov772x.h
> +++ b/include/media/i2c/ov772x.h
> @@ -12,6 +12,7 @@
> /* for flags */
> #define OV772X_FLAG_VFLIP (1 << 0) /* Vertical flip image */
> #define OV772X_FLAG_HFLIP (1 << 1) /* Horizontal flip image */
> +#define OV772X_FLAG_TEST_PATTERN (1 << 2) /* Test pattern */
>
> /*
> * for Edge ctrl

--
Regards,

Laurent Pinchart

2020-09-04 07:54:39

[permalink] [raw]

Subject: 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 <[email protected]>
> > Reviewed-by: Biju Das <[email protected]>
> > ---
> > 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 <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
> > + 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(&client->dev),
> > + NULL);
> > + if (!endpoint) {
> > + dev_err(&client->dev, "endpoint node not found\n");
> > + ret = -EINVAL;
> > + goto error_clk_put;
> > + }
> > +
> > + ret = v4l2_fwnode_endpoint_parse(endpoint, &priv->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 <[email protected]>
>
> > + fwnode_handle_put(endpoint);
> > + if (ret) {
> > + dev_err(&client->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(&client->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

2020-09-04 08:23:21

[permalink] [raw]

Subject: 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 <[email protected]>
> > > Reviewed-by: Biju Das <[email protected]>
> > > ---
> > > 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 <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
> > > + 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(&client->dev),
> > > + NULL);
> > > + if (!endpoint) {
> > > + dev_err(&client->dev, "endpoint node not found\n");
> > > + ret = -EINVAL;
> > > + goto error_clk_put;
> > > + }
> > > +
> > > + ret = v4l2_fwnode_endpoint_parse(endpoint, &priv->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 it.

>
>
> >
> > Reviewed-by: Laurent Pinchart <[email protected]>
> >
> > > + fwnode_handle_put(endpoint);
> > > + if (ret) {
> > > + dev_err(&client->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(&client->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

--
Sakari Ailus

2020-09-04 09:20:54

[permalink] [raw]

Subject: 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 <[email protected]>
> > > > Reviewed-by: Biju Das <[email protected]>
> > > > ---
> > > > 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 <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
> > > > + 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(&client->dev),
> > > > + NULL);
> > > > + if (!endpoint) {
> > > > + dev_err(&client->dev, "endpoint node not found\n");
> > > > + ret = -EINVAL;
> > > > + goto error_clk_put;
> > > > + }
> > > > +
> > > > + ret = v4l2_fwnode_endpoint_parse(endpoint, &priv->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 it.

For reference:
https://patchwork.linuxtv.org/project/linux-media/patch/[email protected]/

But yes, we might have DTBs in the wild without bus-type specified :(

>
> >
> >
> > >
> > > Reviewed-by: Laurent Pinchart <[email protected]>
> > >
> > > > + fwnode_handle_put(endpoint);
> > > > + if (ret) {
> > > > + dev_err(&client->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(&client->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
>
> --
> Sakari Ailus

2020-09-04 09:40:00

[permalink] [raw]

Subject: 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 <[email protected]>
> > > > > Reviewed-by: Biju Das <[email protected]>
> > > > > ---
> > > > > 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 <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
> > > > > + 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(&client->dev),
> > > > > + NULL);
> > > > > + if (!endpoint) {
> > > > > + dev_err(&client->dev, "endpoint node not found\n");
> > > > > + ret = -EINVAL;
> > > > > + goto error_clk_put;
> > > > > + }
> > > > > +
> > > > > + ret = v4l2_fwnode_endpoint_parse(endpoint, &priv->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 it.
>
> For reference:
> https://patchwork.linuxtv.org/project/linux-media/patch/[email protected]/
>
> But yes, we might have DTBs in the wild without bus-type specified :(

Shouldn't that be then that the bus-type is optional and defaults to
parallel?

--
Sakari Ailus

2020-09-04 10:34:30

[permalink] [raw]

Subject: 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 <[email protected]>
> > > > > > Reviewed-by: Biju Das <[email protected]>
> > > > > > ---
> > > > > > 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 <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
> > > > > > + 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(&client->dev),
> > > > > > + NULL);
> > > > > > + if (!endpoint) {
> > > > > > + dev_err(&client->dev, "endpoint node not found\n");
> > > > > > + ret = -EINVAL;
> > > > > > + goto error_clk_put;
> > > > > > + }
> > > > > > +
> > > > > > + ret = v4l2_fwnode_endpoint_parse(endpoint, &priv->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 it.
> >
> > For reference:
> > https://patchwork.linuxtv.org/project/linux-media/patch/[email protected]/
> >
> > But yes, we might have DTBs in the wild without bus-type specified :(
>
> Shouldn't that be then that the bus-type is optional and defaults to
> parallel?

I think going forward we want to make it mandatory, don't we ? The
older dts will fail at dt validation time against the new yaml bindings, but
my understanding is that this is not a problem.

Binary compatibility, with the introduction of BT.656 support becomes
more complex instead :/

Before this series parallel was the only supported bus type and no
endpoint properties were required. The driver picked the default
settings for signal polarities and that was it.

With the introduction of BT.656 no signal polarity properties means
BT.656 when autoguess is in use. So going forward the bus-type shall
be explicitly set, but we might receive old DTBs with no bus-type and
no endpoint properties which assumes 'parallel' is in use.

One possible way forward could be:
- verify if bus-type is present in the fwnode
- if it is, we have a new DTB and we can rely on autoguess
- if it's not assume we have an old DTB that assumed 'parallel'. Parse
the fwnode and if any relevant V4L2_MBUS_ flag is set use it,
otherwise use the defaults.

If we make bus-type optional in new bindings, the old DTB with no
parallel endpoint properties would be identified as BT.656 breaking
capture operation, am I wrong ?

This might require a bit more work from Prabhakar I'm sorry. The old
bindings were clearly falling short once BT.656 becomes supported.

2020-09-04 11:04:44

[permalink] [raw]

Subject: 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 <[email protected]>
> > > > > > > Reviewed-by: Biju Das <[email protected]>
> > > > > > > ---
> > > > > > > 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 <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
> > > > > > > + 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(&client->dev),
> > > > > > > + NULL);
> > > > > > > + if (!endpoint) {
> > > > > > > + dev_err(&client->dev, "endpoint node not found\n");
> > > > > > > + ret = -EINVAL;
> > > > > > > + goto error_clk_put;
> > > > > > > + }
> > > > > > > +
> > > > > > > + ret = v4l2_fwnode_endpoint_parse(endpoint, &priv->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 it.
> > >
> > > For reference:
> > > https://patchwork.linuxtv.org/project/linux-media/patch/[email protected]/
> > >
> > > But yes, we might have DTBs in the wild without bus-type specified :(
> >
> > Shouldn't that be then that the bus-type is optional and defaults to
> > parallel?
>
> I think going forward we want to make it mandatory, don't we ? The
> older dts will fail at dt validation time against the new yaml bindings, but
> my understanding is that this is not a problem.

For new devices, yes. I still wouldn't make DT binding changes that render
the old DT source invalid, at least unless it's absolutely mandatory. And
that is not the case here.

I guess it may be a bit grey area. At least leave a comment in the driver
on how the old bindings were so the code isn't accidentally "fixed".

>
> Binary compatibility, with the introduction of BT.656 support becomes
> more complex instead :/
>
> Before this series parallel was the only supported bus type and no
> endpoint properties were required. The driver picked the default
> settings for signal polarities and that was it.
>
> With the introduction of BT.656 no signal polarity properties means
> BT.656 when autoguess is in use. So going forward the bus-type shall
> be explicitly set, but we might receive old DTBs with no bus-type and
> no endpoint properties which assumes 'parallel' is in use.
>
> One possible way forward could be:
> - verify if bus-type is present in the fwnode
> - if it is, we have a new DTB and we can rely on autoguess
> - if it's not assume we have an old DTB that assumed 'parallel'. Parse
> the fwnode and if any relevant V4L2_MBUS_ flag is set use it,
> otherwise use the defaults.
>
> If we make bus-type optional in new bindings, the old DTB with no
> parallel endpoint properties would be identified as BT.656 breaking
> capture operation, am I wrong ?

There's no technical reason why it has to be so.

You simply try endpoint parsing with parallel bus first, with the old
defaults, and if that succeeds, then you don't attempt to parse it as
Bt.656 anymore.

>
> This might require a bit more work from Prabhakar I'm sorry. The old
> bindings were clearly falling short once BT.656 becomes supported.

--
Kind regards,

Sakari Ailus

2020-09-04 13:43:00

[permalink] [raw]

Subject: 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 <[email protected]>
> > > > Reviewed-by: Biju Das <[email protected]>
> > > > ---
> > > > 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 <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
> > > > + 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(&client->dev),
> > > > + NULL);
> > > > + if (!endpoint) {
> > > > + dev_err(&client->dev, "endpoint node not found\n");
> > > > + ret = -EINVAL;
> > > > + goto error_clk_put;
> > > > + }
> > > > +
> > > > + ret = v4l2_fwnode_endpoint_parse(endpoint, &priv->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, too.
>
> I can send the patches.

This patch shows a common use case, with a source supporting both
parallel and BT.656. Having to call v4l2_fwnode_endpoint_parse() twice,
once for each bus type, isn't a very good API in my opinion. I'd like
v4l2_fwnode_endpoint_parse() to be able to determine the bus type (using
the bus-type property) *and* support defaults with a single call.

Speaking of defaults, I wonder if it's a good idea to let that up to
drivers, or if we should standardize on one set of defaults that would
be shared by all DT bindings.

> > 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 it.
>
> > > Reviewed-by: Laurent Pinchart <[email protected]>
> > >
> > > > + fwnode_handle_put(endpoint);
> > > > + if (ret) {
> > > > + dev_err(&client->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(&client->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

2020-09-04 13:56:08

[permalink] [raw]

Subject: 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 <[email protected]>
> > > > > > > > Reviewed-by: Biju Das <[email protected]>
> > > > > > > > ---
> > > > > > > > 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 <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
> > > > > > > > + 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(&client->dev),
> > > > > > > > + NULL);
> > > > > > > > + if (!endpoint) {
> > > > > > > > + dev_err(&client->dev, "endpoint node not found\n");
> > > > > > > > + ret = -EINVAL;
> > > > > > > > + goto error_clk_put;
> > > > > > > > + }
> > > > > > > > +
> > > > > > > > + ret = v4l2_fwnode_endpoint_parse(endpoint, &priv->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 it.
> > > >
> > > > For reference:
> > > > https://patchwork.linuxtv.org/project/linux-media/patch/[email protected]/
> > > >
> > > > But yes, we might have DTBs in the wild without bus-type specified :(
> > >
> > > Shouldn't that be then that the bus-type is optional and defaults to
> > > parallel?
> >
> > I think going forward we want to make it mandatory, don't we ? The
> > older dts will fail at dt validation time against the new yaml bindings, but
> > my understanding is that this is not a problem.
>
> For new devices, yes. I still wouldn't make DT binding changes that render
> the old DT source invalid, at least unless it's absolutely mandatory. And
> that is not the case here.
>
> I guess it may be a bit grey area. At least leave a comment in the driver
> on how the old bindings were so the code isn't accidentally "fixed".
>
> > Binary compatibility, with the introduction of BT.656 support becomes
> > more complex instead :/
> >
> > Before this series parallel was the only supported bus type and no
> > endpoint properties were required. The driver picked the default
> > settings for signal polarities and that was it.
> >
> > With the introduction of BT.656 no signal polarity properties means
> > BT.656 when autoguess is in use. So going forward the bus-type shall
> > be explicitly set, but we might receive old DTBs with no bus-type and
> > no endpoint properties which assumes 'parallel' is in use.
> >
> > One possible way forward could be:
> > - verify if bus-type is present in the fwnode
> > - if it is, we have a new DTB and we can rely on autoguess

It's not guessing if the bus type is specified :-)

> > - if it's not assume we have an old DTB that assumed 'parallel'. Parse
> > the fwnode and if any relevant V4L2_MBUS_ flag is set use it,
> > otherwise use the defaults.
> >
> > If we make bus-type optional in new bindings, the old DTB with no
> > parallel endpoint properties would be identified as BT.656 breaking
> > capture operation, am I wrong ?
>
> There's no technical reason why it has to be so.
>
> You simply try endpoint parsing with parallel bus first, with the old
> defaults, and if that succeeds, then you don't attempt to parse it as
> Bt.656 anymore.

If bus-type is optional with new bindings,
v4l2_fwnode_endpoint_parse(V4L2_MBUS_PARALLEL) will always succeed if
the bus-type DT property isn't set.

> > This might require a bit more work from Prabhakar I'm sorry. The old
> > bindings were clearly falling short once BT.656 becomes supported.

--
Regards,

Laurent Pinchart

2020-09-04 19:53:22