Re: [PATCH v2 3/4] media: v4l2-async: Log message in case of heterogenous fwnode match

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

Hi Kieran,

On Wed, Mar 18, 2020 at 09:16:01AM +0000, Kieran Bingham wrote:
> On 18/03/2020 00:25, Laurent Pinchart wrote:
> > When a notifier supplies a device fwnode and a subdev supplies an
> > endpoint fwnode, incorrect matches may occur if multiple subdevs
> > correspond to the same device fwnode. This can't be handled
> > transparently in the framework, and requires the notifier to switch to
> > endpoint fwnodes. Log a message to notify of this problem. A second
> > message is added to help accelerating the transition to endpoint
> > matching.
> 
> Only minor comments and discussion below:
> 
> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > ---
> >  drivers/media/v4l2-core/v4l2-async.c | 24 +++++++++++++++++++++++-
> >  1 file changed, 23 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> > index 224b39a7aeb1..9f393a7be455 100644
> > --- a/drivers/media/v4l2-core/v4l2-async.c
> > +++ b/drivers/media/v4l2-core/v4l2-async.c
> > @@ -77,6 +77,7 @@ static bool match_fwnode(struct v4l2_async_notifier *notifier,
> >  	struct fwnode_handle *dev_fwnode;
> >  	bool asd_fwnode_is_ep;
> >  	bool sd_fwnode_is_ep;
> > +	struct device *dev;
> >  	const char *name;
> >  
> >  	/*
> > @@ -113,7 +114,28 @@ static bool match_fwnode(struct v4l2_async_notifier *notifier,
> >  
> >  	fwnode_handle_put(dev_fwnode);
> >  
> > -	return dev_fwnode == other_fwnode;
> > +	if (dev_fwnode != other_fwnode)
> > +		return false;
> > +
> > +	/*
> > +	 * We have an heterogenous match. Retrieve the struct device of the
> 
> s/an/a/
> 
> s/heterogenous/heterogeneous/ (and that's not an en-gb/en-us thing)
> Also in $SUBJECT
> 
> > +	 * side that matched on a device fwnode to print its driver name.
> > +	 */
> > +	if (sd_fwnode_is_ep)
> > +		dev = notifier->v4l2_dev ? notifier->v4l2_dev->dev
> > +		    : notifier->sd->dev;
> 
> Eugh ... I guess if this gets needed elsewhere, notifiers need a helper
> to get the appropriate dev out... but if this is the only place, then so
> be it.

I'll let the next person who needs this create a helper :-)

> > +	else
> > +		dev = sd->dev;
> > +
> > +	if (dev && dev->driver) {
> > +		if (sd_fwnode_is_ep)
> > +			dev_info(dev, "Driver %s uses device fwnode, incorrect match may occur\n",
> > +				 dev->driver->name);
> > +		dev_info(dev, "Consider updating driver %s to match on endpoints\n",
> > +			 dev->driver->name);
> 
> I think I interpret that in the case that existing drivers match on
> dev->dev (i.e. no endpoints involved) then this will not print, as we
> would already have matched and returned earlier in the function.
> 
> I don't think that's a problem, but it means people will not be
> 'encouraged' to move to endpoint matching until they encounter a device
> which uses endpoints.
> 
> Perhaps that's ok ... but I was almost thinking of being more 'pushy'
> and guiding device matches to move to endpoints too ;-)

I don't think we can do that, as not all devices have endpoints. For
instance, a lens controller or a flash controller will be referred to by
phandle properties, without using OF graph. We still need to support
matching them.

> > +	}
> > +
> > +	return true;
> >  }
> >  
> >  static bool match_custom(struct v4l2_async_notifier *notifier,

-- 
Regards,

Laurent Pinchart