Hi Laurent,
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@xxxxxxxxxxxxxxxx>
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@xxxxxxxxxxxxxxxx>
> ---
> 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.
> + 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 ;-)
> + }
> +
> + return true;
> }
>
> static bool match_custom(struct v4l2_async_notifier *notifier,
>
