Re: [PATCH 2/2] rcar-csi2: Use V4L2 async helpers to create the notifier

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Niklas,

Thank you for the patch.

On Fri, Aug 07, 2020 at 01:16:19PM +0200, Niklas Söderlund wrote:
> The V4L2 async framework helpers now populates the async notifier with
> endpoint matching information and there is no need to do this manually
> in the R-Car CSI-2 driver, switch to using the provided helper.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  drivers/media/platform/rcar-vin/rcar-csi2.c | 48 +++++----------------
>  1 file changed, 11 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
> index c6cc4f473a077899..f0067ff21d5d9d33 100644
> --- a/drivers/media/platform/rcar-vin/rcar-csi2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> @@ -362,7 +362,6 @@ struct rcar_csi2 {
>  	struct media_pad pads[NR_OF_RCAR_CSI2_PAD];
>  
>  	struct v4l2_async_notifier notifier;
> -	struct v4l2_async_subdev asd;
>  	struct v4l2_subdev *remote;
>  
>  	struct v4l2_mbus_framefmt mf;
> @@ -774,9 +773,11 @@ static const struct v4l2_async_notifier_operations rcar_csi2_notify_ops = {
>  	.unbind = rcsi2_notify_unbind,
>  };
>  
> -static int rcsi2_parse_v4l2(struct rcar_csi2 *priv,
> -			    struct v4l2_fwnode_endpoint *vep)
> +static int rcar_csi2_parse_v4l2(struct device *dev,
> +				struct v4l2_fwnode_endpoint *vep,
> +				struct v4l2_async_subdev *asd)
>  {
> +	struct rcar_csi2 *priv = dev_get_drvdata(dev);
>  	unsigned int i;
>  
>  	/* Only port 0 endpoint 0 is valid. */
> @@ -806,53 +807,26 @@ static int rcsi2_parse_v4l2(struct rcar_csi2 *priv,
>  		}
>  	}
>  
> +	dev_dbg(priv->dev, "Found '%pOF'\n", to_of_node(asd->match.fwnode));
> +
>  	return 0;
>  }
>  
>  static int rcsi2_parse_dt(struct rcar_csi2 *priv)
>  {
> -	struct device_node *ep;
> -	struct v4l2_fwnode_endpoint v4l2_ep = { .bus_type = 0 };
>  	int ret;
>  
> -	ep = of_graph_get_endpoint_by_regs(priv->dev->of_node, 0, 0);
> -	if (!ep) {
> -		dev_err(priv->dev, "Not connected to subdevice\n");
> -		return -EINVAL;
> -	}
> -
> -	ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(ep), &v4l2_ep);
> -	if (ret) {
> -		dev_err(priv->dev, "Could not parse v4l2 endpoint\n");
> -		of_node_put(ep);
> -		return -EINVAL;
> -	}
> -
> -	ret = rcsi2_parse_v4l2(priv, &v4l2_ep);
> -	if (ret) {
> -		of_node_put(ep);
> -		return ret;
> -	}
> -
> -	priv->asd.match.fwnode =
> -		fwnode_graph_get_remote_endpoint(of_fwnode_handle(ep));
> -	priv->asd.match_type = V4L2_ASYNC_MATCH_FWNODE;
> -
> -	of_node_put(ep);
> -
>  	v4l2_async_notifier_init(&priv->notifier);
>  
> -	ret = v4l2_async_notifier_add_subdev(&priv->notifier, &priv->asd);
> -	if (ret) {
> -		fwnode_handle_put(priv->asd.match.fwnode);
> +	ret = v4l2_async_notifier_parse_fwnode_endpoints_by_port(
> +			priv->dev, &priv->notifier,
> +			sizeof(struct v4l2_async_subdev), 0,
> +			rcar_csi2_parse_v4l2);

It seems a bit expensive to go through a complex logic of iterating over
all endpoints when rcar_csi2_parse_v4l2() will only accept port 0,
endpoint 0, but there's a clear benefit from not having to maintain this
code in the driver.

With Jacopo's comments addressed,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

If anyone has time to spend on it, I think
__v4l2_async_notifier_parse_fwnode_ep() should be duplicated in
v4l2_async_notifier_parse_fwnode_endpoints() and
v4l2_async_notifier_parse_fwnode_endpoints_by_port(), with the latter
only iterating over the endpoints of the requested port.

I also wonder if the !is_available check needs to be inside the loop:

	fwnode_graph_for_each_endpoint(dev_fwnode(dev), fwnode) {
		struct fwnode_handle *dev_fwnode;
		bool is_available;

		dev_fwnode = fwnode_graph_get_port_parent(fwnode);
		is_available = fwnode_device_is_available(dev_fwnode);
		fwnode_handle_put(dev_fwnode);
		if (!is_available)
			continue;

		...
	}

Isn't dev_fwnode == dev_fwnode(dev) and thus constant ?

> +	if (ret)
>  		return ret;
> -	}
>  
>  	priv->notifier.ops = &rcar_csi2_notify_ops;
>  
> -	dev_dbg(priv->dev, "Found '%pOF'\n",
> -		to_of_node(priv->asd.match.fwnode));
> -
>  	ret = v4l2_async_subdev_notifier_register(&priv->subdev,
>  						  &priv->notifier);
>  	if (ret)

-- 
Regards,

Laurent Pinchart