Hi Hans,
Thank you for the review.
On Thu, Apr 04, 2019 at 03:37:44PM +0200, Hans Verkuil wrote:
...
> > diff --git a/drivers/media/platform/davinci/vpif_capture.c
> > b/drivers/media/platform/davinci/vpif_capture.c
> > index 6216b7ac6875..bb4c9cb9f2ad 100644
> > --- a/drivers/media/platform/davinci/vpif_capture.c
> > +++ b/drivers/media/platform/davinci/vpif_capture.c
> > @@ -1541,7 +1541,6 @@ vpif_capture_get_pdata(struct platform_device *pdev)
> >
> > for (i = 0; i < VPIF_CAPTURE_NUM_CHANNELS; i++) {
> > struct v4l2_fwnode_endpoint bus_cfg = { .bus_type = 0 };
> > - struct device_node *rem;
> > unsigned int flags;
> > int err;
> >
> > @@ -1550,14 +1549,6 @@ vpif_capture_get_pdata(struct platform_device *pdev)
> > if (!endpoint)
> > break;
> >
> > - rem = of_graph_get_remote_port_parent(endpoint);
> > - if (!rem) {
> > - dev_dbg(&pdev->dev, "Remote device at %pOF not found\n",
> > - endpoint);
> > - of_node_put(endpoint);
> > - goto done;
> > - }
> > -
> > sdinfo = &pdata->subdev_info[i];
> > chan = &pdata->chan_config[i];
> > chan->inputs = devm_kcalloc(&pdev->dev,
> > @@ -1565,7 +1556,6 @@ vpif_capture_get_pdata(struct platform_device *pdev)
> > sizeof(*chan->inputs),
> > GFP_KERNEL);
> > if (!chan->inputs) {
> > - of_node_put(rem);
> > of_node_put(endpoint);
> > goto err_cleanup;
> > }
> > @@ -1577,10 +1567,8 @@ vpif_capture_get_pdata(struct platform_device *pdev)
> >
> > err = v4l2_fwnode_endpoint_parse(of_fwnode_handle(endpoint),
> > &bus_cfg);
> > - of_node_put(endpoint);
>
> I don't think you can delete this line, seems to be an accidental deletion.
Right. of_graph_get_next_endpoint() will put the endpoint node on the next
iteration, so if the loop is executed again, the endpoint node musn't be
put here. I think I'll properly address this in a separate patch (before
this one).
>
> > if (err) {
> > dev_err(&pdev->dev, "Could not parse the endpoint\n");
> > - of_node_put(rem);
> > goto done;
> > }
> >
> > @@ -1599,7 +1587,7 @@ vpif_capture_get_pdata(struct platform_device *pdev)
> > sdinfo->name = rem->full_name;
> >
> > pdata->asd[i] = v4l2_async_notifier_add_fwnode_subdev(
> > - &vpif_obj.notifier, of_fwnode_handle(rem),
> > + &vpif_obj.notifier, of_fwnode_handle(endpoint),
> > sizeof(struct v4l2_async_subdev));
> > if (IS_ERR(pdata->asd[i])) {
> > of_node_put(rem);
...
> > diff --git a/drivers/media/v4l2-core/v4l2-async.c
> > b/drivers/media/v4l2-core/v4l2-async.c
> > index 15b0c44a76e7..4cb49d5f8c03 100644
> > --- a/drivers/media/v4l2-core/v4l2-async.c
> > +++ b/drivers/media/v4l2-core/v4l2-async.c
> > @@ -670,8 +670,12 @@ int v4l2_async_register_subdev(struct v4l2_subdev *sd)
> > * (struct v4l2_subdev.dev), and async sub-device does not
> > * exist independently of the device at any point of time.
> > */
> > - if (!sd->fwnode && sd->dev)
> > - sd->fwnode = dev_fwnode(sd->dev);
> > + if (!sd->fwnode && sd->dev) {
> > + sd->fwnode = fwnode_graph_get_next_endpoint(
> > + dev_fwnode(sd->dev), NULL);
>
> Doesn't this take a reference? As opposed to the assignment below?
>
> Shouldn't you call 'fwnode_handle_put(sd->fwnode);'?
Yes. I'll fix this for the next version.
>
> > + if (!sd->fwnode)
> > + sd->fwnode = dev_fwnode(sd->dev);
> > + }
> >
> > mutex_lock(&list_lock);
> >
--
Sakari Ailus
sakari.ai...@linux.intel.com
