Re: [PATCH v5 1/3] media: i2c: ov772x: Parse endpoint properties

[Jacopo Mondi]

Hi Prabhakar,

On Tue, Sep 15, 2020 at 06:42:33PM +0100, Lad Prabhakar wrote:
> Parse endpoint properties using v4l2_fwnode_endpoint_alloc_parse()
> to determine bus-type and store it locally in priv data.
>
> v4l2_fwnode_endpoint_alloc_parse() with bus_type set to
> V4L2_MBUS_PARALLEL falls back to V4L2_MBUS_PARALLEL thus handling
> backward compatibility with existing DT where bus-type isn't specified.


I don't think this is necessary here. This patch does not need to
handle any retrocompatibility, as only PARALLEL is supported.

The 'right' way to put it to me would be
"Parse endpoint properties using v4l2_fwnode_endpoint_alloc_parse()
to determine the bus type and store it in the driver structure.

Set bus_type to V4L2_MBUS_PARALLEL as it's the only supported one"

See comment in the next patch for retrocompatibility

>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
> ---
>  drivers/media/i2c/ov772x.c | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
>
> diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c
> index 2cc6a678069a..4ab4b3c883d0 100644
> --- a/drivers/media/i2c/ov772x.c
> +++ b/drivers/media/i2c/ov772x.c
> @@ -31,6 +31,7 @@
>  #include <media/v4l2-ctrls.h>
>  #include <media/v4l2-device.h>
>  #include <media/v4l2-event.h>
> +#include <media/v4l2-fwnode.h>
>  #include <media/v4l2-image-sizes.h>
>  #include <media/v4l2-subdev.h>
>
> @@ -434,6 +435,7 @@ struct ov772x_priv {
>  #ifdef CONFIG_MEDIA_CONTROLLER
>  	struct media_pad pad;
>  #endif
> +	enum v4l2_mbus_type		  bus_type;
>  };
>
>  /*
> @@ -1354,6 +1356,8 @@ static const struct v4l2_subdev_ops ov772x_subdev_ops = {
>
>  static int ov772x_probe(struct i2c_client *client)
>  {
> +	struct v4l2_fwnode_endpoint bus_cfg;
> +	struct fwnode_handle	*ep;
>  	struct ov772x_priv	*priv;
>  	int			ret;
>  	static const struct regmap_config ov772x_regmap_config = {
> @@ -1415,6 +1419,27 @@ static int ov772x_probe(struct i2c_client *client)
>  		goto error_clk_put;
>  	}
>
> +	ep = fwnode_graph_get_next_endpoint(dev_fwnode(&client->dev),
> +					    NULL);
> +	if (!ep) {
> +		dev_err(&client->dev, "endpoint node not found\n");
> +		ret = -EINVAL;
> +		goto error_clk_put;
> +	}
> +
> +	/* For backward compatibility with the existing DT where
> +	 * bus-type isn't specified v4l2_fwnode_endpoint_alloc_parse()
> +	 * with bus_type set to V4L2_MBUS_PARALLEL falls back to
> +	 * V4L2_MBUS_PARALLEL
> +	 */

You can drop this comment block

> +	bus_cfg.bus_type = V4L2_MBUS_PARALLEL;
> +	ret = v4l2_fwnode_endpoint_alloc_parse(ep, &bus_cfg);
> +	priv->bus_type = bus_cfg.bus_type;

Set this after if (ret)

> +	v4l2_fwnode_endpoint_free(&bus_cfg);
> +	fwnode_handle_put(ep);
> +	if (ret)
> +		goto error_clk_put;
> +
>  	ret = ov772x_video_probe(priv);
>  	if (ret < 0)
>  		goto error_gpio_put;
> --
> 2.17.1
>