Re: [PATCH v4 5/9] media: rcar-vin: Parse parallel input on Gen3

[jacopo mondi <jacopo@jmondi.org>]

[-- Attachment #1: Type: text/plain, Size: 5509 bytes --]

Hi Niklas,

On Fri, May 25, 2018 at 12:29:44AM +0200, Niklas Söderlund wrote:
> Hi Jacopo,
>
> Thanks for your work.
>
> I really like what you did with this patch in v4.

Thanks for review and suggestions, what's there comes mostly from your
comments and guidance.

>
> On 2018-05-25 00:02:15 +0200, Jacopo Mondi wrote:
> > The rcar-vin driver so far had a mutually exclusive code path for
> > handling parallel and CSI-2 video input subdevices, with only the CSI-2
> > use case supporting media-controller. As we add support for parallel
> > inputs to Gen3 media-controller compliant code path now parse both port@0
> > and port@1, handling the media-controller use case in the parallel
> > bound/unbind notifier operations.
> >
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> >
> > ---
> > v3 -> v4:
> > - Change the mc/parallel initialization order. Initialize mc first, then
> >   parallel
> > - As a consequence no need to delay parallel notifiers registration, the
> >   media controller is set up already when parallel input got parsed,
> >   this greatly simplify the group notifier complete callback.
> > ---
> >  drivers/media/platform/rcar-vin/rcar-core.c | 56 ++++++++++++++++++-----------
> >  1 file changed, 35 insertions(+), 21 deletions(-)
> >
> > diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
> > index a799684..29619c2 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-core.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> > @@ -399,6 +399,11 @@ static int rvin_parallel_subdevice_attach(struct rvin_dev *vin,
> >  	ret = rvin_find_pad(subdev, MEDIA_PAD_FL_SINK);
> >  	vin->parallel->sink_pad = ret < 0 ? 0 : ret;
> >
> > +	if (vin->info->use_mc) {
> > +		vin->parallel->subdev = subdev;
> > +		return 0;
> > +	}
> > +
> >  	/* Find compatible subdevices mbus format */
> >  	vin->mbus_code = 0;
> >  	code.index = 0;
> > @@ -460,10 +465,12 @@ static int rvin_parallel_subdevice_attach(struct rvin_dev *vin,
> >  static void rvin_parallel_subdevice_detach(struct rvin_dev *vin)
> >  {
> >  	rvin_v4l2_unregister(vin);
> > -	v4l2_ctrl_handler_free(&vin->ctrl_handler);
> > -
> > -	vin->vdev.ctrl_handler = NULL;
> >  	vin->parallel->subdev = NULL;
> > +
> > +	if (!vin->info->use_mc) {
> > +		v4l2_ctrl_handler_free(&vin->ctrl_handler);
> > +		vin->vdev.ctrl_handler = NULL;
> > +	}
> >  }
> >
> >  static int rvin_parallel_notify_complete(struct v4l2_async_notifier *notifier)
> > @@ -552,18 +559,18 @@ static int rvin_parallel_parse_v4l2(struct device *dev,
> >  	return 0;
> >  }
> >
> > -static int rvin_parallel_graph_init(struct rvin_dev *vin)
> > +static int rvin_parallel_init(struct rvin_dev *vin)
> >  {
> >  	int ret;
> >
> > -	ret = v4l2_async_notifier_parse_fwnode_endpoints(
> > -		vin->dev, &vin->notifier,
> > -		sizeof(struct rvin_parallel_entity), rvin_parallel_parse_v4l2);
> > +	ret = v4l2_async_notifier_parse_fwnode_endpoints_by_port(
> > +		vin->dev, &vin->notifier, sizeof(struct rvin_parallel_entity),
> > +		0, rvin_parallel_parse_v4l2);
> >  	if (ret)
> >  		return ret;
> >
> >  	if (!vin->parallel)
> > -		return -ENODEV;
> > +		return -ENOTCONN;
>
> I think you still should return -ENODEV here if !vin->info->use_mc to
> preserve Gen2 which runs without media controller behavior. How about:
>
>     return vin->info->use_mc ? -ENOTCONN : -ENODEV;

Right, I wish I had some gen2 board to test. I wonder if that's not
better handled in probe though... I'll see how it looks like.
>
> >
> >  	vin_dbg(vin, "Found parallel subdevice %pOF\n",
> >  		to_of_node(vin->parallel->asd.match.fwnode));
> > @@ -784,14 +791,8 @@ static int rvin_mc_init(struct rvin_dev *vin)
> >  {
> >  	int ret;
> >
> > -	vin->pad.flags = MEDIA_PAD_FL_SINK;
> > -	ret = media_entity_pads_init(&vin->vdev.entity, 1, &vin->pad);
> > -	if (ret)
> > -		return ret;
> > -
> > -	ret = rvin_group_get(vin);
> > -	if (ret)
> > -		return ret;
> > +	if (!vin->info->use_mc)
> > +		return 0;
> >
> >  	ret = rvin_mc_parse_of_graph(vin);
> >  	if (ret)
> > @@ -1074,11 +1075,24 @@ static int rcar_vin_probe(struct platform_device *pdev)
> >  		return ret;
> >
> >  	platform_set_drvdata(pdev, vin);
> > -	if (vin->info->use_mc)
> > -		ret = rvin_mc_init(vin);
> > -	else
> > -		ret = rvin_parallel_graph_init(vin);
> > -	if (ret < 0)
> > +
> > +	if (vin->info->use_mc) {
> > +		vin->pad.flags = MEDIA_PAD_FL_SINK;
> > +		ret = media_entity_pads_init(&vin->vdev.entity, 1, &vin->pad);
> > +		if (ret)
> > +			return ret;
> > +
> > +		ret = rvin_group_get(vin);
> > +		if (ret)
> > +			return ret;
> > +	}
>
> I don't see why you need to move the media pad creation out of
> rvin_mc_init(). With the reorder of the rvin_mc_init()
> rvin_parallel_init() I would keep this in rvin_mc_init().
>

Just because I've been lazy re-structuring that part of code I broke
up in previous versions. I'll move that part back to mc_init() and
possibly also remove the

+	if (!vin->info->use_mc)
+		return 0;

in that function and handle this in probe().

Thanks
   j


> > +
> > +	ret = rvin_mc_init(vin);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = rvin_parallel_init(vin);
> > +	if (ret < 0 && ret != -ENOTCONN)
> >  		goto error;
> >
> >  	pm_suspend_ignore_children(&pdev->dev, true);
> > --
> > 2.7.4
> >
>
> --
> Regards,
> Niklas Söderlund

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]