Re: [PATCH v13 07/25] rcar-vin: Use generic parser for parsing fwnode endpoints

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

Hi Sakari,

Thank you for the patch.

On Friday, 15 September 2017 17:17:06 EEST Sakari Ailus wrote:
> Instead of using driver implementation, use

Same comment as for patch 06/25.

> v4l2_async_notifier_parse_fwnode_endpoints() to parse the fwnode endpoints
> of the device.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> Acked-by: Hans Verkuil <hans.verkuil@cisco.com>
> ---
>  drivers/media/platform/rcar-vin/rcar-core.c | 112 ++++++++-----------------
>  drivers/media/platform/rcar-vin/rcar-dma.c  |  10 +--
>  drivers/media/platform/rcar-vin/rcar-v4l2.c |  14 ++--
>  drivers/media/platform/rcar-vin/rcar-vin.h  |   4 +-
>  4 files changed, 48 insertions(+), 92 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-core.c
> b/drivers/media/platform/rcar-vin/rcar-core.c index
> 142de447aaaa..62b4a94f9a39 100644
> --- a/drivers/media/platform/rcar-vin/rcar-core.c
> +++ b/drivers/media/platform/rcar-vin/rcar-core.c

[snip]

> @@ -120,117 +121,70 @@ static int rvin_digital_notify_bound(struct

[snip]

> -static int rvin_digitial_parse_v4l2(struct rvin_dev *vin,
> -				    struct device_node *ep,
> -				    struct v4l2_mbus_config *mbus_cfg)
> +static int rvin_digital_parse_v4l2(struct device *dev,
> +				   struct v4l2_fwnode_endpoint *vep,
> +				   struct v4l2_async_subdev *asd)
>  {
> -	struct v4l2_fwnode_endpoint v4l2_ep;
> -	int ret;
> +	struct rvin_dev *vin = dev_get_drvdata(dev);

Doesn't this show that we miss a context argument to the callback function ? 
Storing the context in device driver data is probably OK if the driver parsing 
the endpoints controls the struct device, but is that always the case ?

> +	struct rvin_graph_entity *rvge =
> +		container_of(asd, struct rvin_graph_entity, asd);
> 
> -	ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(ep), &v4l2_ep);
> -	if (ret) {
> -		vin_err(vin, "Could not parse v4l2 endpoint\n");
> -		return -EINVAL;
> -	}
> +	if (vep->base.port || vep->base.id)
> +		return -ENOTCONN;
> 
> -	mbus_cfg->type = v4l2_ep.bus_type;
> +	rvge->mbus_cfg.type = vep->bus_type;
> 
> -	switch (mbus_cfg->type) {
> +	switch (rvge->mbus_cfg.type) {
>  	case V4L2_MBUS_PARALLEL:
>  		vin_dbg(vin, "Found PARALLEL media bus\n");
> -		mbus_cfg->flags = v4l2_ep.bus.parallel.flags;
> +		rvge->mbus_cfg.flags = vep->bus.parallel.flags;
>  		break;
>  	case V4L2_MBUS_BT656:
>  		vin_dbg(vin, "Found BT656 media bus\n");
> -		mbus_cfg->flags = 0;
> +		rvge->mbus_cfg.flags = 0;
>  		break;
>  	default:
>  		vin_err(vin, "Unknown media bus type\n");
>  		return -EINVAL;
>  	}
> 
> -	return 0;
> -}
> -
> -static int rvin_digital_graph_parse(struct rvin_dev *vin)
> -{
> -	struct device_node *ep, *np;
> -	int ret;
> -
> -	vin->digital.asd.match.fwnode.fwnode = NULL;
> -	vin->digital.subdev = NULL;
> -
> -	/*
> -	 * Port 0 id 0 is local digital input, try to get it.
> -	 * Not all instances can or will have this, that is OK
> -	 */
> -	ep = of_graph_get_endpoint_by_regs(vin->dev->of_node, 0, 0);
> -	if (!ep)
> -		return 0;
> -
> -	np = of_graph_get_remote_port_parent(ep);
> -	if (!np) {
> -		vin_err(vin, "No remote parent for digital input\n");
> -		of_node_put(ep);
> -		return -EINVAL;
> -	}
> -	of_node_put(np);
> -
> -	ret = rvin_digitial_parse_v4l2(vin, ep, &vin->digital.mbus_cfg);
> -	of_node_put(ep);
> -	if (ret)
> -		return ret;
> -
> -	vin->digital.asd.match.fwnode.fwnode = of_fwnode_handle(np);
> -	vin->digital.asd.match_type = V4L2_ASYNC_MATCH_FWNODE;
> +	vin->digital = rvge;
> 
>  	return 0;
>  }
> 
>  static int rvin_digital_graph_init(struct rvin_dev *vin)
>  {
> -	struct v4l2_async_subdev **subdevs = NULL;
>  	int ret;
> 
> -	ret = rvin_digital_graph_parse(vin);
> +	ret = v4l2_async_notifier_parse_fwnode_endpoints(
> +		vin->dev, &vin->notifier,
> +		sizeof(struct rvin_graph_entity), rvin_digital_parse_v4l2);
>  	if (ret)
>  		return ret;
> 
> -	if (!vin->digital.asd.match.fwnode.fwnode) {
> -		vin_dbg(vin, "No digital subdevice found\n");
> -		return -ENODEV;
> -	}
> -
> -	/* Register the subdevices notifier. */
> -	subdevs = devm_kzalloc(vin->dev, sizeof(*subdevs), GFP_KERNEL);
> -	if (subdevs == NULL)
> -		return -ENOMEM;
> -
> -	subdevs[0] = &vin->digital.asd;
> -
> -	vin_dbg(vin, "Found digital subdevice %pOF\n",
> -		to_of_node(subdevs[0]->match.fwnode.fwnode));
> +	if (vin->digital)
> +		vin_dbg(vin, "Found digital subdevice %pOF\n",
> +			to_of_node(
> +				vin->digital->asd.match.fwnode.fwnode));

Isn't this is a change in behaviour ? The driver currently returns -ENODEV 
when no digital subdev is found.

> -	vin->notifier.num_subdevs = 1;
> -	vin->notifier.subdevs = subdevs;
>  	vin->notifier.bound = rvin_digital_notify_bound;
>  	vin->notifier.unbind = rvin_digital_notify_unbind;
>  	vin->notifier.complete = rvin_digital_notify_complete;
> -
>  	ret = v4l2_async_notifier_register(&vin->v4l2_dev, &vin->notifier);
>  	if (ret < 0) {
>  		vin_err(vin, "Notifier registration failed\n");


[snip]

-- 
Regards,

Laurent Pinchart