Re: [RFC 1/8] v4l2-async: Use endpoint node, not device node, for fwnode match

[Sakari Ailus <sakari.ailus@linux.intel.com>]

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.ailus@linux.intel.com