| Message ID | 20200318002507.30336-4-laurent.pinchart+renesas@ideasonboard.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series | media: v4l2-async: Accept endpoints and devices for fwnode matching | expand |
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@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. > + 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, >
Hi Laurent, On Wed, Mar 18, 2020 at 1:26 AM Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> 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. > > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> Thanks for your patch! > --- a/drivers/media/v4l2-core/v4l2-async.c > +++ b/drivers/media/v4l2-core/v4l2-async.c > @@ -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 > + * 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; > + 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); I think that deserves a dev_warn(). > + dev_info(dev, "Consider updating driver %s to match on endpoints\n", > + dev->driver->name); And dev_notice(), while at it ;-) > + } > + > + return true; > } Gr{oetje,eeting}s, Geert
Hi Laurent, On Wed, Mar 18, 2020 at 02:25:06AM +0200, 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. > > 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 > + * 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; Have you considered passing the device directly ? seems notifier is only used for that... Apart this small nit, for the series Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org> Thanks j > + 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); > + } > + > + return true; > } > > static bool match_custom(struct v4l2_async_notifier *notifier, > -- > Regards, > > Laurent Pinchart >
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,
Hi Geert, On Wed, Mar 18, 2020 at 10:16:37AM +0100, Geert Uytterhoeven wrote: > On Wed, Mar 18, 2020 at 1:26 AM 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. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> > > Thanks for your patch! > > > --- a/drivers/media/v4l2-core/v4l2-async.c > > +++ b/drivers/media/v4l2-core/v4l2-async.c > > @@ -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 > > + * 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; > > + 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); > > I think that deserves a dev_warn(). > > > + dev_info(dev, "Consider updating driver %s to match on endpoints\n", > > + dev->driver->name); > > And dev_notice(), while at it ;-) A rarely used, but totally appropriate level. I'll fix both. > > + } > > + > > + return true; > > }
Hi Jacopo, On Wed, Mar 18, 2020 at 03:03:26PM +0100, Jacopo Mondi wrote: > On Wed, Mar 18, 2020 at 02:25:06AM +0200, 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. > > > > 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 > > + * 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; > > Have you considered passing the device directly ? seems notifier is > only used for that... Yes, but I thought that match functions could use the notifier for other purposes in the future, so I think this approach is more versatile. > Apart this small nit, for the series > Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > > > + 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); > > + } > > + > > + return true; > > } > > > > static bool match_custom(struct v4l2_async_notifier *notifier,
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 + * 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; + 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); + } + + return true; } static bool match_custom(struct v4l2_async_notifier *notifier,
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. 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(-)