Hi Jacopo, Thanks for your work.
I really like what you did with this patch in v4. 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 <[email protected]> > > --- > 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; > > 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(). > + > + 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
