| Message ID | 20200315125511.25756-1-laurent.pinchart+renesas@ideasonboard.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: v4l2-async: Accept endpoints and devices for fwnode matching | expand |
Hi Laurent, Thank you for the patch. On Sun, Mar 15, 2020 at 12:55 PM Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> wrote: > > fwnode matching was designed to match on nodes corresponding to a > device. Some drivers, however, needed to match on endpoints, and have > passed endpoint fwnodes to v4l2-async. This works when both the subdev > and the notifier use the same fwnode types (endpoint or device), but > makes drivers that use different types incompatible. > > Fix this by extending the fwnode match to handle fwnodes of different > types. When the types (deduced from the node name) are different, > retrieve the device fwnode for the side that provides an endpoint > fwnode, and compare it with the device fwnode provided by the other > side. This allows interoperability between all drivers, regardless of > which type of fwnode they use for matching. > > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> > --- > This has been compile-tested only. Prabhakar, could you check if it > fixes your issue ? > Yes it does handle all the case. Tested-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> Cheers, --Prabhakar > drivers/media/v4l2-core/v4l2-async.c | 42 +++++++++++++++++++++++++++- > 1 file changed, 41 insertions(+), 1 deletion(-) > > diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c > index 8bde33c21ce4..995e5464cba7 100644 > --- a/drivers/media/v4l2-core/v4l2-async.c > +++ b/drivers/media/v4l2-core/v4l2-async.c > @@ -71,7 +71,47 @@ static bool match_devname(struct v4l2_subdev *sd, > > static bool match_fwnode(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd) > { > - return sd->fwnode == asd->match.fwnode; > + struct fwnode_handle *other_fwnode; > + struct fwnode_handle *dev_fwnode; > + bool asd_fwnode_is_ep; > + bool sd_fwnode_is_ep; > + const char *name; > + > + /* > + * Both the subdev and the async subdev can provide either an endpoint > + * fwnode or a device fwnode. Start with the simple case of direct > + * fwnode matching. > + */ > + if (sd->fwnode == asd->match.fwnode) > + return true; > + > + /* > + * Otherwise, check if the sd fwnode and the asd fwnode refer to an > + * endpoint or a device. If they're of the same type, there's no match. > + */ > + name = fwnode_get_name(sd->fwnode); > + sd_fwnode_is_ep = name && strstarts(name, "endpoint"); > + name = fwnode_get_name(asd->match.fwnode); > + asd_fwnode_is_ep = name && strstarts(name, "endpoint"); > + > + if (sd_fwnode_is_ep == asd_fwnode_is_ep) > + return false; > + > + /* > + * The sd and asd fwnodes are of different types. Get the device fwnode > + * parent of the endpoint fwnode, and compare it with the other fwnode. > + */ > + if (sd_fwnode_is_ep) { > + dev_fwnode = fwnode_graph_get_port_parent(sd->fwnode); > + other_fwnode = asd->match.fwnode; > + } else { > + dev_fwnode = fwnode_graph_get_port_parent(asd->match.fwnode); > + other_fwnode = sd->fwnode; > + } > + > + fwnode_handle_put(dev_fwnode); > + > + return dev_fwnode == other_fwnode; > } > > static bool match_custom(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd) > -- > Regards, > > Laurent Pinchart >
Hi Laurent, On Sun, Mar 15, 2020 at 02:55:11PM +0200, Laurent Pinchart wrote: > fwnode matching was designed to match on nodes corresponding to a > device. Some drivers, however, needed to match on endpoints, and have > passed endpoint fwnodes to v4l2-async. This works when both the subdev > and the notifier use the same fwnode types (endpoint or device), but > makes drivers that use different types incompatible. > > Fix this by extending the fwnode match to handle fwnodes of different > types. When the types (deduced from the node name) are different, > retrieve the device fwnode for the side that provides an endpoint > fwnode, and compare it with the device fwnode provided by the other > side. This allows interoperability between all drivers, regardless of > which type of fwnode they use for matching. > I'm sorry but I'm not sure why would make a difference compared to what Kieran's patch did. https://lore.kernel.org/patchwork/patch/788637/ If the bridge matches on device node, and the remote registers more than one endpoints it is possible to get a false match. I'm not sure how that would happen in practice, as the bridge would be registering the fwnode of the remote device twice, but I think that was the reason kieran's patch has not been collected or am I mistaken ? > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> > --- > This has been compile-tested only. Prabhakar, could you check if it > fixes your issue ? > > drivers/media/v4l2-core/v4l2-async.c | 42 +++++++++++++++++++++++++++- > 1 file changed, 41 insertions(+), 1 deletion(-) > > diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c > index 8bde33c21ce4..995e5464cba7 100644 > --- a/drivers/media/v4l2-core/v4l2-async.c > +++ b/drivers/media/v4l2-core/v4l2-async.c > @@ -71,7 +71,47 @@ static bool match_devname(struct v4l2_subdev *sd, > > static bool match_fwnode(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd) > { > - return sd->fwnode == asd->match.fwnode; > + struct fwnode_handle *other_fwnode; > + struct fwnode_handle *dev_fwnode; > + bool asd_fwnode_is_ep; > + bool sd_fwnode_is_ep; > + const char *name; > + > + /* > + * Both the subdev and the async subdev can provide either an endpoint > + * fwnode or a device fwnode. Start with the simple case of direct > + * fwnode matching. > + */ > + if (sd->fwnode == asd->match.fwnode) > + return true; > + > + /* > + * Otherwise, check if the sd fwnode and the asd fwnode refer to an > + * endpoint or a device. If they're of the same type, there's no match. > + */ > + name = fwnode_get_name(sd->fwnode); > + sd_fwnode_is_ep = name && strstarts(name, "endpoint"); > + name = fwnode_get_name(asd->match.fwnode); > + asd_fwnode_is_ep = name && strstarts(name, "endpoint"); > + > + if (sd_fwnode_is_ep == asd_fwnode_is_ep) > + return false; > + > + /* > + * The sd and asd fwnodes are of different types. Get the device fwnode > + * parent of the endpoint fwnode, and compare it with the other fwnode. > + */ > + if (sd_fwnode_is_ep) { > + dev_fwnode = fwnode_graph_get_port_parent(sd->fwnode); > + other_fwnode = asd->match.fwnode; > + } else { > + dev_fwnode = fwnode_graph_get_port_parent(asd->match.fwnode); > + other_fwnode = sd->fwnode; > + } > + > + fwnode_handle_put(dev_fwnode); > + > + return dev_fwnode == other_fwnode; > } > > static bool match_custom(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd) > -- > Regards, > > Laurent Pinchart >
Hi Jacopo, On Sun, Mar 15, 2020 at 10:47:07PM +0100, Jacopo Mondi wrote: > On Sun, Mar 15, 2020 at 02:55:11PM +0200, Laurent Pinchart wrote: > > fwnode matching was designed to match on nodes corresponding to a > > device. Some drivers, however, needed to match on endpoints, and have > > passed endpoint fwnodes to v4l2-async. This works when both the subdev > > and the notifier use the same fwnode types (endpoint or device), but > > makes drivers that use different types incompatible. > > > > Fix this by extending the fwnode match to handle fwnodes of different > > types. When the types (deduced from the node name) are different, > > retrieve the device fwnode for the side that provides an endpoint > > fwnode, and compare it with the device fwnode provided by the other > > side. This allows interoperability between all drivers, regardless of > > which type of fwnode they use for matching. > > I'm sorry but I'm not sure why would make a difference compared to > what Kieran's patch did. > https://lore.kernel.org/patchwork/patch/788637/ > > If the bridge matches on device node, and the remote registers more > than one endpoints it is possible to get a false match. How so ? If a notifier entry points to a device node, and two subdevs are registered with different endpoint nodes that are both part of the same device node, the notifier will get either of them. Which subdev match the notifier won't be guaranteed, but that's what the notifier asked for if it contains a device node and not an endpoint node: any subdev corresponding to the device node. In practice notifiers will need to move to endpoint matching if they want to get a particular subdev of a device, and this change allows doing so without mass-patching every driver. It allows a notifier to switch to endpoint nodes, while subdevs still use device nodes and are gradually ported. > I'm not sure how that would happen in practice, as the bridge would be > registering the fwnode of the remote device twice, but I think that > was the reason kieran's patch has not been collected or am I > mistaken ? > > > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> > > --- > > This has been compile-tested only. Prabhakar, could you check if it > > fixes your issue ? > > > > drivers/media/v4l2-core/v4l2-async.c | 42 +++++++++++++++++++++++++++- > > 1 file changed, 41 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c > > index 8bde33c21ce4..995e5464cba7 100644 > > --- a/drivers/media/v4l2-core/v4l2-async.c > > +++ b/drivers/media/v4l2-core/v4l2-async.c > > @@ -71,7 +71,47 @@ static bool match_devname(struct v4l2_subdev *sd, > > > > static bool match_fwnode(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd) > > { > > - return sd->fwnode == asd->match.fwnode; > > + struct fwnode_handle *other_fwnode; > > + struct fwnode_handle *dev_fwnode; > > + bool asd_fwnode_is_ep; > > + bool sd_fwnode_is_ep; > > + const char *name; > > + > > + /* > > + * Both the subdev and the async subdev can provide either an endpoint > > + * fwnode or a device fwnode. Start with the simple case of direct > > + * fwnode matching. > > + */ > > + if (sd->fwnode == asd->match.fwnode) > > + return true; > > + > > + /* > > + * Otherwise, check if the sd fwnode and the asd fwnode refer to an > > + * endpoint or a device. If they're of the same type, there's no match. > > + */ > > + name = fwnode_get_name(sd->fwnode); > > + sd_fwnode_is_ep = name && strstarts(name, "endpoint"); > > + name = fwnode_get_name(asd->match.fwnode); > > + asd_fwnode_is_ep = name && strstarts(name, "endpoint"); > > + > > + if (sd_fwnode_is_ep == asd_fwnode_is_ep) > > + return false; > > + > > + /* > > + * The sd and asd fwnodes are of different types. Get the device fwnode > > + * parent of the endpoint fwnode, and compare it with the other fwnode. > > + */ > > + if (sd_fwnode_is_ep) { > > + dev_fwnode = fwnode_graph_get_port_parent(sd->fwnode); > > + other_fwnode = asd->match.fwnode; > > + } else { > > + dev_fwnode = fwnode_graph_get_port_parent(asd->match.fwnode); > > + other_fwnode = sd->fwnode; > > + } > > + > > + fwnode_handle_put(dev_fwnode); > > + > > + return dev_fwnode == other_fwnode; > > } > > > > static bool match_custom(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd)
Hi Laurent, On Mon, Mar 16, 2020 at 08:34:44AM +0200, Laurent Pinchart wrote: > Hi Jacopo, > > On Sun, Mar 15, 2020 at 10:47:07PM +0100, Jacopo Mondi wrote: > > On Sun, Mar 15, 2020 at 02:55:11PM +0200, Laurent Pinchart wrote: > > > fwnode matching was designed to match on nodes corresponding to a > > > device. Some drivers, however, needed to match on endpoints, and have > > > passed endpoint fwnodes to v4l2-async. This works when both the subdev > > > and the notifier use the same fwnode types (endpoint or device), but > > > makes drivers that use different types incompatible. > > > > > > Fix this by extending the fwnode match to handle fwnodes of different > > > types. When the types (deduced from the node name) are different, > > > retrieve the device fwnode for the side that provides an endpoint > > > fwnode, and compare it with the device fwnode provided by the other > > > side. This allows interoperability between all drivers, regardless of > > > which type of fwnode they use for matching. > > > > I'm sorry but I'm not sure why would make a difference compared to > > what Kieran's patch did. > > https://lore.kernel.org/patchwork/patch/788637/ > > > > If the bridge matches on device node, and the remote registers more > > than one endpoints it is possible to get a false match. > > How so ? If a notifier entry points to a device node, and two subdevs > are registered with different endpoint nodes that are both part of the > same device node, the notifier will get either of them. Which subdev > match the notifier won't be guaranteed, but that's what the notifier > asked for if it contains a device node and not an endpoint node: any > subdev corresponding to the device node. > > In practice notifiers will need to move to endpoint matching if they > want to get a particular subdev of a device, and this change allows > doing so without mass-patching every driver. It allows a notifier to > switch to endpoint nodes, while subdevs still use device nodes and are > gradually ported. > In case a device has two CSI-2 receivers, different IPs, different drivers which register different notifiers, and they are connected in DTS to a device like adv748x which registers two async devices for its two CSI-2 transmitter on endpoints, depending on which CSI-2 receiver gets probed first, it matches any of the two CSI-2 Tx. The media graph would complete but it won't be what's described in dts. I agree it's unlikely, and having something like this or what kieran did in is better than the current situation, so I'm not pushing this back, at all. Just pointing possible reasons why we still don't have any solution to this issue in mainline. Thanks j > > I'm not sure how that would happen in practice, as the bridge would be > > registering the fwnode of the remote device twice, but I think that > > was the reason kieran's patch has not been collected or am I > > mistaken ? > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> > > > --- > > > This has been compile-tested only. Prabhakar, could you check if it > > > fixes your issue ? > > > > > > drivers/media/v4l2-core/v4l2-async.c | 42 +++++++++++++++++++++++++++- > > > 1 file changed, 41 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c > > > index 8bde33c21ce4..995e5464cba7 100644 > > > --- a/drivers/media/v4l2-core/v4l2-async.c > > > +++ b/drivers/media/v4l2-core/v4l2-async.c > > > @@ -71,7 +71,47 @@ static bool match_devname(struct v4l2_subdev *sd, > > > > > > static bool match_fwnode(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd) > > > { > > > - return sd->fwnode == asd->match.fwnode; > > > + struct fwnode_handle *other_fwnode; > > > + struct fwnode_handle *dev_fwnode; > > > + bool asd_fwnode_is_ep; > > > + bool sd_fwnode_is_ep; > > > + const char *name; > > > + > > > + /* > > > + * Both the subdev and the async subdev can provide either an endpoint > > > + * fwnode or a device fwnode. Start with the simple case of direct > > > + * fwnode matching. > > > + */ > > > + if (sd->fwnode == asd->match.fwnode) > > > + return true; > > > + > > > + /* > > > + * Otherwise, check if the sd fwnode and the asd fwnode refer to an > > > + * endpoint or a device. If they're of the same type, there's no match. > > > + */ > > > + name = fwnode_get_name(sd->fwnode); > > > + sd_fwnode_is_ep = name && strstarts(name, "endpoint"); > > > + name = fwnode_get_name(asd->match.fwnode); > > > + asd_fwnode_is_ep = name && strstarts(name, "endpoint"); > > > + > > > + if (sd_fwnode_is_ep == asd_fwnode_is_ep) > > > + return false; > > > + > > > + /* > > > + * The sd and asd fwnodes are of different types. Get the device fwnode > > > + * parent of the endpoint fwnode, and compare it with the other fwnode. > > > + */ > > > + if (sd_fwnode_is_ep) { > > > + dev_fwnode = fwnode_graph_get_port_parent(sd->fwnode); > > > + other_fwnode = asd->match.fwnode; > > > + } else { > > > + dev_fwnode = fwnode_graph_get_port_parent(asd->match.fwnode); > > > + other_fwnode = sd->fwnode; > > > + } > > > + > > > + fwnode_handle_put(dev_fwnode); > > > + > > > + return dev_fwnode == other_fwnode; > > > } > > > > > > static bool match_custom(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd) > > -- > Regards, > > Laurent Pinchart
Hi Jacopo, On Mon, Mar 16, 2020 at 09:59:34AM +0100, Jacopo Mondi wrote: > On Mon, Mar 16, 2020 at 08:34:44AM +0200, Laurent Pinchart wrote: > > On Sun, Mar 15, 2020 at 10:47:07PM +0100, Jacopo Mondi wrote: > >> On Sun, Mar 15, 2020 at 02:55:11PM +0200, Laurent Pinchart wrote: > >>> fwnode matching was designed to match on nodes corresponding to a > >>> device. Some drivers, however, needed to match on endpoints, and have > >>> passed endpoint fwnodes to v4l2-async. This works when both the subdev > >>> and the notifier use the same fwnode types (endpoint or device), but > >>> makes drivers that use different types incompatible. > >>> > >>> Fix this by extending the fwnode match to handle fwnodes of different > >>> types. When the types (deduced from the node name) are different, > >>> retrieve the device fwnode for the side that provides an endpoint > >>> fwnode, and compare it with the device fwnode provided by the other > >>> side. This allows interoperability between all drivers, regardless of > >>> which type of fwnode they use for matching. > >> > >> I'm sorry but I'm not sure why would make a difference compared to > >> what Kieran's patch did. > >> https://lore.kernel.org/patchwork/patch/788637/ > >> > >> If the bridge matches on device node, and the remote registers more > >> than one endpoints it is possible to get a false match. > > > > How so ? If a notifier entry points to a device node, and two subdevs > > are registered with different endpoint nodes that are both part of the > > same device node, the notifier will get either of them. Which subdev > > match the notifier won't be guaranteed, but that's what the notifier > > asked for if it contains a device node and not an endpoint node: any > > subdev corresponding to the device node. > > > > In practice notifiers will need to move to endpoint matching if they > > want to get a particular subdev of a device, and this change allows > > doing so without mass-patching every driver. It allows a notifier to > > switch to endpoint nodes, while subdevs still use device nodes and are > > gradually ported. > > In case a device has two CSI-2 receivers, different IPs, different > drivers which register different notifiers, and they are connected in > DTS to a device like adv748x which registers two async > devices for its two CSI-2 transmitter on endpoints, depending on which > CSI-2 receiver gets probed first, it matches any of the two CSI-2 Tx. > The media graph would complete but it won't be what's described in > dts. > > I agree it's unlikely, and having something like this or what kieran > did in is better than the current situation, so I'm not pushing this > back, at all. Just pointing possible reasons why we still don't have > any solution to this issue in mainline. Regardless of whether it's likely or not, to support this correctly, endpoint matching is required, there's no way around that. This change doesn't introduce any regression, and allows migrating subdevs and subdevs users independently from each other, so I see no drawback :-) > >> I'm not sure how that would happen in practice, as the bridge would be > >> registering the fwnode of the remote device twice, but I think that > >> was the reason kieran's patch has not been collected or am I > >> mistaken ? > >> > >>> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> > >>> --- > >>> This has been compile-tested only. Prabhakar, could you check if it > >>> fixes your issue ? > >>> > >>> drivers/media/v4l2-core/v4l2-async.c | 42 +++++++++++++++++++++++++++- > >>> 1 file changed, 41 insertions(+), 1 deletion(-) > >>> > >>> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c > >>> index 8bde33c21ce4..995e5464cba7 100644 > >>> --- a/drivers/media/v4l2-core/v4l2-async.c > >>> +++ b/drivers/media/v4l2-core/v4l2-async.c > >>> @@ -71,7 +71,47 @@ static bool match_devname(struct v4l2_subdev *sd, > >>> > >>> static bool match_fwnode(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd) > >>> { > >>> - return sd->fwnode == asd->match.fwnode; > >>> + struct fwnode_handle *other_fwnode; > >>> + struct fwnode_handle *dev_fwnode; > >>> + bool asd_fwnode_is_ep; > >>> + bool sd_fwnode_is_ep; > >>> + const char *name; > >>> + > >>> + /* > >>> + * Both the subdev and the async subdev can provide either an endpoint > >>> + * fwnode or a device fwnode. Start with the simple case of direct > >>> + * fwnode matching. > >>> + */ > >>> + if (sd->fwnode == asd->match.fwnode) > >>> + return true; > >>> + > >>> + /* > >>> + * Otherwise, check if the sd fwnode and the asd fwnode refer to an > >>> + * endpoint or a device. If they're of the same type, there's no match. > >>> + */ > >>> + name = fwnode_get_name(sd->fwnode); > >>> + sd_fwnode_is_ep = name && strstarts(name, "endpoint"); > >>> + name = fwnode_get_name(asd->match.fwnode); > >>> + asd_fwnode_is_ep = name && strstarts(name, "endpoint"); > >>> + > >>> + if (sd_fwnode_is_ep == asd_fwnode_is_ep) > >>> + return false; > >>> + > >>> + /* > >>> + * The sd and asd fwnodes are of different types. Get the device fwnode > >>> + * parent of the endpoint fwnode, and compare it with the other fwnode. > >>> + */ > >>> + if (sd_fwnode_is_ep) { > >>> + dev_fwnode = fwnode_graph_get_port_parent(sd->fwnode); > >>> + other_fwnode = asd->match.fwnode; > >>> + } else { > >>> + dev_fwnode = fwnode_graph_get_port_parent(asd->match.fwnode); > >>> + other_fwnode = sd->fwnode; > >>> + } > >>> + > >>> + fwnode_handle_put(dev_fwnode); > >>> + > >>> + return dev_fwnode == other_fwnode; > >>> } > >>> > >>> static bool match_custom(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd)
Hi Laurent, Thanks for your work. On 2020-03-15 14:55:11 +0200, Laurent Pinchart wrote: > fwnode matching was designed to match on nodes corresponding to a > device. Some drivers, however, needed to match on endpoints, and have > passed endpoint fwnodes to v4l2-async. This works when both the subdev > and the notifier use the same fwnode types (endpoint or device), but > makes drivers that use different types incompatible. > > Fix this by extending the fwnode match to handle fwnodes of different > types. When the types (deduced from the node name) are different, > retrieve the device fwnode for the side that provides an endpoint > fwnode, and compare it with the device fwnode provided by the other > side. This allows interoperability between all drivers, regardless of > which type of fwnode they use for matching. > > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> I tested this on R-Car CSI-2 and ADV748x without any regressions. As Jacopo already pointed out it's similar to what have been tried before and have the potential problem for new transmitters registering multiple endpoints (like ADV748x) being used together with older receivers who register a single device node in v4l-async. But this do not introduce any regressions and is a good first step to move everything to endpoint matching. Tested-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> Maybe a info message should be logged if a match is made between endpoint and node? It would make it easy to spot if one needs to debug a miss match and would be a clear message one driver should be moved to endpoint matching. Maybe adding such a log message would count as a regression for some. > --- > This has been compile-tested only. Prabhakar, could you check if it > fixes your issue ? > > drivers/media/v4l2-core/v4l2-async.c | 42 +++++++++++++++++++++++++++- > 1 file changed, 41 insertions(+), 1 deletion(-) > > diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c > index 8bde33c21ce4..995e5464cba7 100644 > --- a/drivers/media/v4l2-core/v4l2-async.c > +++ b/drivers/media/v4l2-core/v4l2-async.c > @@ -71,7 +71,47 @@ static bool match_devname(struct v4l2_subdev *sd, > > static bool match_fwnode(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd) > { > - return sd->fwnode == asd->match.fwnode; > + struct fwnode_handle *other_fwnode; > + struct fwnode_handle *dev_fwnode; > + bool asd_fwnode_is_ep; > + bool sd_fwnode_is_ep; > + const char *name; > + > + /* > + * Both the subdev and the async subdev can provide either an endpoint > + * fwnode or a device fwnode. Start with the simple case of direct > + * fwnode matching. > + */ > + if (sd->fwnode == asd->match.fwnode) > + return true; > + > + /* > + * Otherwise, check if the sd fwnode and the asd fwnode refer to an > + * endpoint or a device. If they're of the same type, there's no match. > + */ > + name = fwnode_get_name(sd->fwnode); > + sd_fwnode_is_ep = name && strstarts(name, "endpoint"); > + name = fwnode_get_name(asd->match.fwnode); > + asd_fwnode_is_ep = name && strstarts(name, "endpoint"); > + > + if (sd_fwnode_is_ep == asd_fwnode_is_ep) > + return false; > + > + /* > + * The sd and asd fwnodes are of different types. Get the device fwnode > + * parent of the endpoint fwnode, and compare it with the other fwnode. > + */ > + if (sd_fwnode_is_ep) { > + dev_fwnode = fwnode_graph_get_port_parent(sd->fwnode); > + other_fwnode = asd->match.fwnode; > + } else { > + dev_fwnode = fwnode_graph_get_port_parent(asd->match.fwnode); > + other_fwnode = sd->fwnode; > + } > + > + fwnode_handle_put(dev_fwnode); > + > + return dev_fwnode == other_fwnode; > } > > static bool match_custom(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd) > -- > Regards, > > Laurent Pinchart >
Hi Niklas, On Mon, Mar 16, 2020 at 10:40:12PM +0100, Niklas Söderlund wrote: > On 2020-03-15 14:55:11 +0200, Laurent Pinchart wrote: > > fwnode matching was designed to match on nodes corresponding to a > > device. Some drivers, however, needed to match on endpoints, and have > > passed endpoint fwnodes to v4l2-async. This works when both the subdev > > and the notifier use the same fwnode types (endpoint or device), but > > makes drivers that use different types incompatible. > > > > Fix this by extending the fwnode match to handle fwnodes of different > > types. When the types (deduced from the node name) are different, > > retrieve the device fwnode for the side that provides an endpoint > > fwnode, and compare it with the device fwnode provided by the other > > side. This allows interoperability between all drivers, regardless of > > which type of fwnode they use for matching. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> > > I tested this on R-Car CSI-2 and ADV748x without any regressions. As > Jacopo already pointed out it's similar to what have been tried before > and have the potential problem for new transmitters registering multiple > endpoints (like ADV748x) being used together with older receivers who > register a single device node in v4l-async. But this do not introduce > any regressions and is a good first step to move everything to endpoint > matching. > > Tested-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > > Maybe a info message should be logged if a match is made between > endpoint and node? It would make it easy to spot if one needs to debug a > miss match and would be a clear message one driver should be moved to > endpoint matching. Maybe adding such a log message would count as a > regression for some. WARN("FIX YOUR DRIVER TO USE ENDPOINT MATCHING") ? :-) Jokes aside, something a bit less harsh such as "Matching endpoint with device node, consider fixing driver %s to use endpoints" wouldn't be a bad idea. > > --- > > This has been compile-tested only. Prabhakar, could you check if it > > fixes your issue ? > > > > drivers/media/v4l2-core/v4l2-async.c | 42 +++++++++++++++++++++++++++- > > 1 file changed, 41 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c > > index 8bde33c21ce4..995e5464cba7 100644 > > --- a/drivers/media/v4l2-core/v4l2-async.c > > +++ b/drivers/media/v4l2-core/v4l2-async.c > > @@ -71,7 +71,47 @@ static bool match_devname(struct v4l2_subdev *sd, > > > > static bool match_fwnode(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd) > > { > > - return sd->fwnode == asd->match.fwnode; > > + struct fwnode_handle *other_fwnode; > > + struct fwnode_handle *dev_fwnode; > > + bool asd_fwnode_is_ep; > > + bool sd_fwnode_is_ep; > > + const char *name; > > + > > + /* > > + * Both the subdev and the async subdev can provide either an endpoint > > + * fwnode or a device fwnode. Start with the simple case of direct > > + * fwnode matching. > > + */ > > + if (sd->fwnode == asd->match.fwnode) > > + return true; > > + > > + /* > > + * Otherwise, check if the sd fwnode and the asd fwnode refer to an > > + * endpoint or a device. If they're of the same type, there's no match. > > + */ > > + name = fwnode_get_name(sd->fwnode); > > + sd_fwnode_is_ep = name && strstarts(name, "endpoint"); > > + name = fwnode_get_name(asd->match.fwnode); > > + asd_fwnode_is_ep = name && strstarts(name, "endpoint"); > > + > > + if (sd_fwnode_is_ep == asd_fwnode_is_ep) > > + return false; > > + > > + /* > > + * The sd and asd fwnodes are of different types. Get the device fwnode > > + * parent of the endpoint fwnode, and compare it with the other fwnode. > > + */ > > + if (sd_fwnode_is_ep) { > > + dev_fwnode = fwnode_graph_get_port_parent(sd->fwnode); > > + other_fwnode = asd->match.fwnode; > > + } else { > > + dev_fwnode = fwnode_graph_get_port_parent(asd->match.fwnode); > > + other_fwnode = sd->fwnode; > > + } > > + > > + fwnode_handle_put(dev_fwnode); > > + > > + return dev_fwnode == other_fwnode; > > } > > > > static bool match_custom(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd)
Hi Laurent, On 16/03/2020 21:47, Laurent Pinchart wrote: > Hi Niklas, > > On Mon, Mar 16, 2020 at 10:40:12PM +0100, Niklas Söderlund wrote: >> On 2020-03-15 14:55:11 +0200, Laurent Pinchart wrote: >>> fwnode matching was designed to match on nodes corresponding to a >>> device. Some drivers, however, needed to match on endpoints, and have >>> passed endpoint fwnodes to v4l2-async. This works when both the subdev >>> and the notifier use the same fwnode types (endpoint or device), but >>> makes drivers that use different types incompatible. >>> >>> Fix this by extending the fwnode match to handle fwnodes of different >>> types. When the types (deduced from the node name) are different, >>> retrieve the device fwnode for the side that provides an endpoint >>> fwnode, and compare it with the device fwnode provided by the other >>> side. This allows interoperability between all drivers, regardless of >>> which type of fwnode they use for matching. >>> >>> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> >> >> I tested this on R-Car CSI-2 and ADV748x without any regressions. As >> Jacopo already pointed out it's similar to what have been tried before This was the patch that I had believed was accepted, but ended up stuck in Sakari's tree: https://git.linuxtv.org/sailus/media_tree.git/commit/?h=fwnode-const&id=35c32d99b2c3f5086b911ec817926de9b7bc3b41 (it's already a little bit-rotted though) >> and have the potential problem for new transmitters registering multiple >> endpoints (like ADV748x) being used together with older receivers who >> register a single device node in v4l-async. But this do not introduce So if an 'old' receiver wants to use the 'new' features, it must upgrade to endpoint matching. I think that's fine. >> any regressions and is a good first step to move everything to endpoint >> matching. >> >> Tested-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> >> >> Maybe a info message should be logged if a match is made between >> endpoint and node? It would make it easy to spot if one needs to debug a >> miss match and would be a clear message one driver should be moved to >> endpoint matching. Maybe adding such a log message would count as a >> regression for some. > > WARN("FIX YOUR DRIVER TO USE ENDPOINT MATCHING") ? :-) Indeed, a notification that they need to update their matching would be useful in that scenario. I believe we need to move forward with this somehow, as we have Xilinx trying to use MAX9286 with Xilinx drivers, (endpoint matching subdev with dev node matching receiver) and Renesas trying to use non-endpoint subdevices against an endpoint matched RCar-VIN ...? > Jokes aside, something a bit less harsh such as "Matching endpoint with > device node, consider fixing driver %s to use endpoints" wouldn't be a > bad idea. Yes, Is there anything else we can do? Even if we 'started' converting other receivers to match on endpoints, it would take time - so I think an intermediate stage like this is still very useful. Of course this patch also lets us push the updates back to those who care about those drivers too ... >>> --- >>> This has been compile-tested only. Prabhakar, could you check if it >>> fixes your issue ? >>> >>> drivers/media/v4l2-core/v4l2-async.c | 42 +++++++++++++++++++++++++++- >>> 1 file changed, 41 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c >>> index 8bde33c21ce4..995e5464cba7 100644 >>> --- a/drivers/media/v4l2-core/v4l2-async.c >>> +++ b/drivers/media/v4l2-core/v4l2-async.c >>> @@ -71,7 +71,47 @@ static bool match_devname(struct v4l2_subdev *sd, >>> >>> static bool match_fwnode(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd) >>> { >>> - return sd->fwnode == asd->match.fwnode; >>> + struct fwnode_handle *other_fwnode; >>> + struct fwnode_handle *dev_fwnode; >>> + bool asd_fwnode_is_ep; >>> + bool sd_fwnode_is_ep; >>> + const char *name; >>> + >>> + /* >>> + * Both the subdev and the async subdev can provide either an endpoint >>> + * fwnode or a device fwnode. Start with the simple case of direct >>> + * fwnode matching. >>> + */ >>> + if (sd->fwnode == asd->match.fwnode) >>> + return true; >>> + >>> + /* >>> + * Otherwise, check if the sd fwnode and the asd fwnode refer to an >>> + * endpoint or a device. If they're of the same type, there's no match. >>> + */ >>> + name = fwnode_get_name(sd->fwnode); >>> + sd_fwnode_is_ep = name && strstarts(name, "endpoint"); >>> + name = fwnode_get_name(asd->match.fwnode); >>> + asd_fwnode_is_ep = name && strstarts(name, "endpoint"); >>> + >>> + if (sd_fwnode_is_ep == asd_fwnode_is_ep) >>> + return false; Ok, so this looks like a good safety check for edge cases which would potentially have got through in my version. >>> + >>> + /* >>> + * The sd and asd fwnodes are of different types. Get the device fwnode >>> + * parent of the endpoint fwnode, and compare it with the other fwnode. >>> + */ >>> + if (sd_fwnode_is_ep) { >>> + dev_fwnode = fwnode_graph_get_port_parent(sd->fwnode); >>> + other_fwnode = asd->match.fwnode; >>> + } else { >>> + dev_fwnode = fwnode_graph_get_port_parent(asd->match.fwnode); >>> + other_fwnode = sd->fwnode; >>> + } >>> + >>> + fwnode_handle_put(dev_fwnode); It seems in my implementation these got leaked too :-) I'm sold. This one is better than the old version I had. Hopefully we can get this moving so that we can progress towards endpoint matching throughout next. (Ideally with a warning to convert non-endpoint matching drivers...) Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> >>> + >>> + return dev_fwnode == other_fwnode; >>> } >>> >>> static bool match_custom(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd) >
Hi Laurent, Thanks for the patch. On Sun, Mar 15, 2020 at 02:55:11PM +0200, Laurent Pinchart wrote: > fwnode matching was designed to match on nodes corresponding to a > device. Some drivers, however, needed to match on endpoints, and have > passed endpoint fwnodes to v4l2-async. This works when both the subdev > and the notifier use the same fwnode types (endpoint or device), but > makes drivers that use different types incompatible. > > Fix this by extending the fwnode match to handle fwnodes of different > types. When the types (deduced from the node name) are different, > retrieve the device fwnode for the side that provides an endpoint > fwnode, and compare it with the device fwnode provided by the other > side. This allows interoperability between all drivers, regardless of > which type of fwnode they use for matching. > > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> > --- > This has been compile-tested only. Prabhakar, could you check if it > fixes your issue ? > > drivers/media/v4l2-core/v4l2-async.c | 42 +++++++++++++++++++++++++++- > 1 file changed, 41 insertions(+), 1 deletion(-) > > diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c > index 8bde33c21ce4..995e5464cba7 100644 > --- a/drivers/media/v4l2-core/v4l2-async.c > +++ b/drivers/media/v4l2-core/v4l2-async.c > @@ -71,7 +71,47 @@ static bool match_devname(struct v4l2_subdev *sd, > > static bool match_fwnode(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd) > { > - return sd->fwnode == asd->match.fwnode; > + struct fwnode_handle *other_fwnode; > + struct fwnode_handle *dev_fwnode; > + bool asd_fwnode_is_ep; > + bool sd_fwnode_is_ep; > + const char *name; > + > + /* > + * Both the subdev and the async subdev can provide either an endpoint > + * fwnode or a device fwnode. Start with the simple case of direct > + * fwnode matching. > + */ > + if (sd->fwnode == asd->match.fwnode) > + return true; > + > + /* > + * Otherwise, check if the sd fwnode and the asd fwnode refer to an > + * endpoint or a device. If they're of the same type, there's no match. > + */ > + name = fwnode_get_name(sd->fwnode); > + sd_fwnode_is_ep = name && strstarts(name, "endpoint"); > + name = fwnode_get_name(asd->match.fwnode); > + asd_fwnode_is_ep = name && strstarts(name, "endpoint"); Apart from the fact that you're parsing graph node names here, this looks good. How about checking instead that calling fwnode_graph_get_remote_endpoint(fwnode_graph_get_remote_endpoint()) yields the same node? That would ensure you're dealing with endpoint nodes without explicitly parsing the graph in any way. Just remember to drop the references. > + > + if (sd_fwnode_is_ep == asd_fwnode_is_ep) > + return false; > + > + /* > + * The sd and asd fwnodes are of different types. Get the device fwnode > + * parent of the endpoint fwnode, and compare it with the other fwnode. > + */ > + if (sd_fwnode_is_ep) { > + dev_fwnode = fwnode_graph_get_port_parent(sd->fwnode); > + other_fwnode = asd->match.fwnode; > + } else { > + dev_fwnode = fwnode_graph_get_port_parent(asd->match.fwnode); > + other_fwnode = sd->fwnode; > + } > + > + fwnode_handle_put(dev_fwnode); > + > + return dev_fwnode == other_fwnode; > } > > static bool match_custom(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd)
Hi Sakari, On Tue, Mar 17, 2020 at 02:44:56PM +0200, Sakari Ailus wrote: > On Sun, Mar 15, 2020 at 02:55:11PM +0200, Laurent Pinchart wrote: > > fwnode matching was designed to match on nodes corresponding to a > > device. Some drivers, however, needed to match on endpoints, and have > > passed endpoint fwnodes to v4l2-async. This works when both the subdev > > and the notifier use the same fwnode types (endpoint or device), but > > makes drivers that use different types incompatible. > > > > Fix this by extending the fwnode match to handle fwnodes of different > > types. When the types (deduced from the node name) are different, > > retrieve the device fwnode for the side that provides an endpoint > > fwnode, and compare it with the device fwnode provided by the other > > side. This allows interoperability between all drivers, regardless of > > which type of fwnode they use for matching. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> > > --- > > This has been compile-tested only. Prabhakar, could you check if it > > fixes your issue ? > > > > drivers/media/v4l2-core/v4l2-async.c | 42 +++++++++++++++++++++++++++- > > 1 file changed, 41 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c > > index 8bde33c21ce4..995e5464cba7 100644 > > --- a/drivers/media/v4l2-core/v4l2-async.c > > +++ b/drivers/media/v4l2-core/v4l2-async.c > > @@ -71,7 +71,47 @@ static bool match_devname(struct v4l2_subdev *sd, > > > > static bool match_fwnode(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd) > > { > > - return sd->fwnode == asd->match.fwnode; > > + struct fwnode_handle *other_fwnode; > > + struct fwnode_handle *dev_fwnode; > > + bool asd_fwnode_is_ep; > > + bool sd_fwnode_is_ep; > > + const char *name; > > + > > + /* > > + * Both the subdev and the async subdev can provide either an endpoint > > + * fwnode or a device fwnode. Start with the simple case of direct > > + * fwnode matching. > > + */ > > + if (sd->fwnode == asd->match.fwnode) > > + return true; > > + > > + /* > > + * Otherwise, check if the sd fwnode and the asd fwnode refer to an > > + * endpoint or a device. If they're of the same type, there's no match. > > + */ > > + name = fwnode_get_name(sd->fwnode); > > + sd_fwnode_is_ep = name && strstarts(name, "endpoint"); > > + name = fwnode_get_name(asd->match.fwnode); > > + asd_fwnode_is_ep = name && strstarts(name, "endpoint"); > > Apart from the fact that you're parsing graph node names here, this looks > good. > > How about checking instead that calling > fwnode_graph_get_remote_endpoint(fwnode_graph_get_remote_endpoint()) yields > the same node? That would ensure you're dealing with endpoint nodes without > explicitly parsing the graph in any way. Would it be simpler to check for the presence of an endpoint property ? > Just remember to drop the references. > > > + > > + if (sd_fwnode_is_ep == asd_fwnode_is_ep) > > + return false; > > + > > + /* > > + * The sd and asd fwnodes are of different types. Get the device fwnode > > + * parent of the endpoint fwnode, and compare it with the other fwnode. > > + */ > > + if (sd_fwnode_is_ep) { > > + dev_fwnode = fwnode_graph_get_port_parent(sd->fwnode); > > + other_fwnode = asd->match.fwnode; > > + } else { > > + dev_fwnode = fwnode_graph_get_port_parent(asd->match.fwnode); > > + other_fwnode = sd->fwnode; > > + } > > + > > + fwnode_handle_put(dev_fwnode); > > + > > + return dev_fwnode == other_fwnode; > > } > > > > static bool match_custom(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd)
Hi Kieran, On Tue, Mar 17, 2020 at 12:33:07PM +0000, Kieran Bingham wrote: > On 16/03/2020 21:47, Laurent Pinchart wrote: > > On Mon, Mar 16, 2020 at 10:40:12PM +0100, Niklas Söderlund wrote: > >> On 2020-03-15 14:55:11 +0200, Laurent Pinchart wrote: > >>> fwnode matching was designed to match on nodes corresponding to a > >>> device. Some drivers, however, needed to match on endpoints, and have > >>> passed endpoint fwnodes to v4l2-async. This works when both the subdev > >>> and the notifier use the same fwnode types (endpoint or device), but > >>> makes drivers that use different types incompatible. > >>> > >>> Fix this by extending the fwnode match to handle fwnodes of different > >>> types. When the types (deduced from the node name) are different, > >>> retrieve the device fwnode for the side that provides an endpoint > >>> fwnode, and compare it with the device fwnode provided by the other > >>> side. This allows interoperability between all drivers, regardless of > >>> which type of fwnode they use for matching. > >>> > >>> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> > >> > >> I tested this on R-Car CSI-2 and ADV748x without any regressions. As > >> Jacopo already pointed out it's similar to what have been tried before > > This was the patch that I had believed was accepted, but ended up stuck > in Sakari's tree: > > https://git.linuxtv.org/sailus/media_tree.git/commit/?h=fwnode-const&id=35c32d99b2c3f5086b911ec817926de9b7bc3b41 > > (it's already a little bit-rotted though) Yes, I noticed that. I don't mind dropping this patch if you rebase yours, as long as we merge a fix :-) > >> and have the potential problem for new transmitters registering multiple > >> endpoints (like ADV748x) being used together with older receivers who > >> register a single device node in v4l-async. But this do not introduce > > So if an 'old' receiver wants to use the 'new' features, it must upgrade > to endpoint matching. > > I think that's fine. Yes that's the idea. It will however not have to upgrade all the subdev drivers it uses at the same time. > >> any regressions and is a good first step to move everything to endpoint > >> matching. > >> > >> Tested-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > >> > >> Maybe a info message should be logged if a match is made between > >> endpoint and node? It would make it easy to spot if one needs to debug a > >> miss match and would be a clear message one driver should be moved to > >> endpoint matching. Maybe adding such a log message would count as a > >> regression for some. > > > > WARN("FIX YOUR DRIVER TO USE ENDPOINT MATCHING") ? :-) > > Indeed, a notification that they need to update their matching would be > useful in that scenario. > > I believe we need to move forward with this somehow, as we have Xilinx > trying to use MAX9286 with Xilinx drivers, (endpoint matching subdev > with dev node matching receiver) and Renesas trying to use non-endpoint > subdevices against an endpoint matched RCar-VIN ...? > > > Jokes aside, something a bit less harsh such as "Matching endpoint with > > device node, consider fixing driver %s to use endpoints" wouldn't be a > > bad idea. > > Yes, Is there anything else we can do? Even if we 'started' converting > other receivers to match on endpoints, it would take time - so I think > an intermediate stage like this is still very useful. > > Of course this patch also lets us push the updates back to those who > care about those drivers too ... Exactly :-) > >>> --- > >>> This has been compile-tested only. Prabhakar, could you check if it > >>> fixes your issue ? > >>> > >>> drivers/media/v4l2-core/v4l2-async.c | 42 +++++++++++++++++++++++++++- > >>> 1 file changed, 41 insertions(+), 1 deletion(-) > >>> > >>> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c > >>> index 8bde33c21ce4..995e5464cba7 100644 > >>> --- a/drivers/media/v4l2-core/v4l2-async.c > >>> +++ b/drivers/media/v4l2-core/v4l2-async.c > >>> @@ -71,7 +71,47 @@ static bool match_devname(struct v4l2_subdev *sd, > >>> > >>> static bool match_fwnode(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd) > >>> { > >>> - return sd->fwnode == asd->match.fwnode; > >>> + struct fwnode_handle *other_fwnode; > >>> + struct fwnode_handle *dev_fwnode; > >>> + bool asd_fwnode_is_ep; > >>> + bool sd_fwnode_is_ep; > >>> + const char *name; > >>> + > >>> + /* > >>> + * Both the subdev and the async subdev can provide either an endpoint > >>> + * fwnode or a device fwnode. Start with the simple case of direct > >>> + * fwnode matching. > >>> + */ > >>> + if (sd->fwnode == asd->match.fwnode) > >>> + return true; > >>> + > >>> + /* > >>> + * Otherwise, check if the sd fwnode and the asd fwnode refer to an > >>> + * endpoint or a device. If they're of the same type, there's no match. > >>> + */ > >>> + name = fwnode_get_name(sd->fwnode); > >>> + sd_fwnode_is_ep = name && strstarts(name, "endpoint"); > >>> + name = fwnode_get_name(asd->match.fwnode); > >>> + asd_fwnode_is_ep = name && strstarts(name, "endpoint"); > >>> + > >>> + if (sd_fwnode_is_ep == asd_fwnode_is_ep) > >>> + return false; > > Ok, so this looks like a good safety check for edge cases which would > potentially have got through in my version. > > >>> + > >>> + /* > >>> + * The sd and asd fwnodes are of different types. Get the device fwnode > >>> + * parent of the endpoint fwnode, and compare it with the other fwnode. > >>> + */ > >>> + if (sd_fwnode_is_ep) { > >>> + dev_fwnode = fwnode_graph_get_port_parent(sd->fwnode); > >>> + other_fwnode = asd->match.fwnode; > >>> + } else { > >>> + dev_fwnode = fwnode_graph_get_port_parent(asd->match.fwnode); > >>> + other_fwnode = sd->fwnode; > >>> + } > >>> + > >>> + fwnode_handle_put(dev_fwnode); > > It seems in my implementation these got leaked too :-) > > I'm sold. This one is better than the old version I had. > > Hopefully we can get this moving so that we can progress towards > endpoint matching throughout next. > > (Ideally with a warning to convert non-endpoint matching drivers...) > > Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> Thank you. I'll add a warning and agree with Sakari on the best method to check if a node is an endpoint node, and will then resubmit. > >>> + > >>> + return dev_fwnode == other_fwnode; > >>> } > >>> > >>> static bool match_custom(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd)
Hi Sakari, On Wed, Mar 18, 2020 at 01:04:32AM +0200, Laurent Pinchart wrote: > On Tue, Mar 17, 2020 at 02:44:56PM +0200, Sakari Ailus wrote: > > On Sun, Mar 15, 2020 at 02:55:11PM +0200, Laurent Pinchart wrote: > > > fwnode matching was designed to match on nodes corresponding to a > > > device. Some drivers, however, needed to match on endpoints, and have > > > passed endpoint fwnodes to v4l2-async. This works when both the subdev > > > and the notifier use the same fwnode types (endpoint or device), but > > > makes drivers that use different types incompatible. > > > > > > Fix this by extending the fwnode match to handle fwnodes of different > > > types. When the types (deduced from the node name) are different, > > > retrieve the device fwnode for the side that provides an endpoint > > > fwnode, and compare it with the device fwnode provided by the other > > > side. This allows interoperability between all drivers, regardless of > > > which type of fwnode they use for matching. > > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> > > > --- > > > This has been compile-tested only. Prabhakar, could you check if it > > > fixes your issue ? > > > > > > drivers/media/v4l2-core/v4l2-async.c | 42 +++++++++++++++++++++++++++- > > > 1 file changed, 41 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c > > > index 8bde33c21ce4..995e5464cba7 100644 > > > --- a/drivers/media/v4l2-core/v4l2-async.c > > > +++ b/drivers/media/v4l2-core/v4l2-async.c > > > @@ -71,7 +71,47 @@ static bool match_devname(struct v4l2_subdev *sd, > > > > > > static bool match_fwnode(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd) > > > { > > > - return sd->fwnode == asd->match.fwnode; > > > + struct fwnode_handle *other_fwnode; > > > + struct fwnode_handle *dev_fwnode; > > > + bool asd_fwnode_is_ep; > > > + bool sd_fwnode_is_ep; > > > + const char *name; > > > + > > > + /* > > > + * Both the subdev and the async subdev can provide either an endpoint > > > + * fwnode or a device fwnode. Start with the simple case of direct > > > + * fwnode matching. > > > + */ > > > + if (sd->fwnode == asd->match.fwnode) > > > + return true; > > > + > > > + /* > > > + * Otherwise, check if the sd fwnode and the asd fwnode refer to an > > > + * endpoint or a device. If they're of the same type, there's no match. > > > + */ > > > + name = fwnode_get_name(sd->fwnode); > > > + sd_fwnode_is_ep = name && strstarts(name, "endpoint"); > > > + name = fwnode_get_name(asd->match.fwnode); > > > + asd_fwnode_is_ep = name && strstarts(name, "endpoint"); > > > > Apart from the fact that you're parsing graph node names here, this looks > > good. And why is that an issue btw ? the ACPI fwnode ops seem to provide a .get_name() operation, would it return the ACPI bus ID here ? > > How about checking instead that calling > > fwnode_graph_get_remote_endpoint(fwnode_graph_get_remote_endpoint()) yields > > the same node? That would ensure you're dealing with endpoint nodes without > > explicitly parsing the graph in any way. > > Would it be simpler to check for the presence of an endpoint property ? > > > Just remember to drop the references. > > > > > + > > > + if (sd_fwnode_is_ep == asd_fwnode_is_ep) > > > + return false; > > > + > > > + /* > > > + * The sd and asd fwnodes are of different types. Get the device fwnode > > > + * parent of the endpoint fwnode, and compare it with the other fwnode. > > > + */ > > > + if (sd_fwnode_is_ep) { > > > + dev_fwnode = fwnode_graph_get_port_parent(sd->fwnode); > > > + other_fwnode = asd->match.fwnode; > > > + } else { > > > + dev_fwnode = fwnode_graph_get_port_parent(asd->match.fwnode); > > > + other_fwnode = sd->fwnode; > > > + } > > > + > > > + fwnode_handle_put(dev_fwnode); > > > + > > > + return dev_fwnode == other_fwnode; > > > } > > > > > > static bool match_custom(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd)
Hi Laurent, On Wed, Mar 18, 2020 at 02:17:26AM +0200, Laurent Pinchart wrote: > Hi Sakari, > > On Wed, Mar 18, 2020 at 01:04:32AM +0200, Laurent Pinchart wrote: > > On Tue, Mar 17, 2020 at 02:44:56PM +0200, Sakari Ailus wrote: > > > On Sun, Mar 15, 2020 at 02:55:11PM +0200, Laurent Pinchart wrote: > > > > fwnode matching was designed to match on nodes corresponding to a > > > > device. Some drivers, however, needed to match on endpoints, and have > > > > passed endpoint fwnodes to v4l2-async. This works when both the subdev > > > > and the notifier use the same fwnode types (endpoint or device), but > > > > makes drivers that use different types incompatible. > > > > > > > > Fix this by extending the fwnode match to handle fwnodes of different > > > > types. When the types (deduced from the node name) are different, > > > > retrieve the device fwnode for the side that provides an endpoint > > > > fwnode, and compare it with the device fwnode provided by the other > > > > side. This allows interoperability between all drivers, regardless of > > > > which type of fwnode they use for matching. > > > > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> > > > > --- > > > > This has been compile-tested only. Prabhakar, could you check if it > > > > fixes your issue ? > > > > > > > > drivers/media/v4l2-core/v4l2-async.c | 42 +++++++++++++++++++++++++++- > > > > 1 file changed, 41 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c > > > > index 8bde33c21ce4..995e5464cba7 100644 > > > > --- a/drivers/media/v4l2-core/v4l2-async.c > > > > +++ b/drivers/media/v4l2-core/v4l2-async.c > > > > @@ -71,7 +71,47 @@ static bool match_devname(struct v4l2_subdev *sd, > > > > > > > > static bool match_fwnode(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd) > > > > { > > > > - return sd->fwnode == asd->match.fwnode; > > > > + struct fwnode_handle *other_fwnode; > > > > + struct fwnode_handle *dev_fwnode; > > > > + bool asd_fwnode_is_ep; > > > > + bool sd_fwnode_is_ep; > > > > + const char *name; > > > > + > > > > + /* > > > > + * Both the subdev and the async subdev can provide either an endpoint > > > > + * fwnode or a device fwnode. Start with the simple case of direct > > > > + * fwnode matching. > > > > + */ > > > > + if (sd->fwnode == asd->match.fwnode) > > > > + return true; > > > > + > > > > + /* > > > > + * Otherwise, check if the sd fwnode and the asd fwnode refer to an > > > > + * endpoint or a device. If they're of the same type, there's no match. > > > > + */ > > > > + name = fwnode_get_name(sd->fwnode); > > > > + sd_fwnode_is_ep = name && strstarts(name, "endpoint"); > > > > + name = fwnode_get_name(asd->match.fwnode); > > > > + asd_fwnode_is_ep = name && strstarts(name, "endpoint"); > > > > > > Apart from the fact that you're parsing graph node names here, this looks > > > good. > > And why is that an issue btw ? the ACPI fwnode ops seem to provide a > .get_name() operation, would it return the ACPI bus ID here ? I'd really prefer not to do graph parsing outside the main parser(s), OF, ACPI and property frameworks. Just for an example, the v4l2_fwnode_link_parse() was broken for ACPI for a long time just because it did not use the graph parsing functions, but implemented graph parsing on its own. > > > > How about checking instead that calling > > > fwnode_graph_get_remote_endpoint(fwnode_graph_get_remote_endpoint()) yields > > > the same node? That would ensure you're dealing with endpoint nodes without > > > explicitly parsing the graph in any way. > > > > Would it be simpler to check for the presence of an endpoint property ? There's no endpoint property, apart from an old ACPI definition. There are differences in the implementations and this is not the best place to try to take them all into account.
Hi Sakari, On Wed, Mar 18, 2020 at 09:52:25AM +0200, Sakari Ailus wrote: > On Wed, Mar 18, 2020 at 02:17:26AM +0200, Laurent Pinchart wrote: > > On Wed, Mar 18, 2020 at 01:04:32AM +0200, Laurent Pinchart wrote: > >> On Tue, Mar 17, 2020 at 02:44:56PM +0200, Sakari Ailus wrote: > >>> On Sun, Mar 15, 2020 at 02:55:11PM +0200, Laurent Pinchart wrote: > >>>> fwnode matching was designed to match on nodes corresponding to a > >>>> device. Some drivers, however, needed to match on endpoints, and have > >>>> passed endpoint fwnodes to v4l2-async. This works when both the subdev > >>>> and the notifier use the same fwnode types (endpoint or device), but > >>>> makes drivers that use different types incompatible. > >>>> > >>>> Fix this by extending the fwnode match to handle fwnodes of different > >>>> types. When the types (deduced from the node name) are different, > >>>> retrieve the device fwnode for the side that provides an endpoint > >>>> fwnode, and compare it with the device fwnode provided by the other > >>>> side. This allows interoperability between all drivers, regardless of > >>>> which type of fwnode they use for matching. > >>>> > >>>> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> > >>>> --- > >>>> This has been compile-tested only. Prabhakar, could you check if it > >>>> fixes your issue ? > >>>> > >>>> drivers/media/v4l2-core/v4l2-async.c | 42 +++++++++++++++++++++++++++- > >>>> 1 file changed, 41 insertions(+), 1 deletion(-) > >>>> > >>>> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c > >>>> index 8bde33c21ce4..995e5464cba7 100644 > >>>> --- a/drivers/media/v4l2-core/v4l2-async.c > >>>> +++ b/drivers/media/v4l2-core/v4l2-async.c > >>>> @@ -71,7 +71,47 @@ static bool match_devname(struct v4l2_subdev *sd, > >>>> > >>>> static bool match_fwnode(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd) > >>>> { > >>>> - return sd->fwnode == asd->match.fwnode; > >>>> + struct fwnode_handle *other_fwnode; > >>>> + struct fwnode_handle *dev_fwnode; > >>>> + bool asd_fwnode_is_ep; > >>>> + bool sd_fwnode_is_ep; > >>>> + const char *name; > >>>> + > >>>> + /* > >>>> + * Both the subdev and the async subdev can provide either an endpoint > >>>> + * fwnode or a device fwnode. Start with the simple case of direct > >>>> + * fwnode matching. > >>>> + */ > >>>> + if (sd->fwnode == asd->match.fwnode) > >>>> + return true; > >>>> + > >>>> + /* > >>>> + * Otherwise, check if the sd fwnode and the asd fwnode refer to an > >>>> + * endpoint or a device. If they're of the same type, there's no match. > >>>> + */ > >>>> + name = fwnode_get_name(sd->fwnode); > >>>> + sd_fwnode_is_ep = name && strstarts(name, "endpoint"); > >>>> + name = fwnode_get_name(asd->match.fwnode); > >>>> + asd_fwnode_is_ep = name && strstarts(name, "endpoint"); > >>> > >>> Apart from the fact that you're parsing graph node names here, this looks > >>> good. > > > > And why is that an issue btw ? the ACPI fwnode ops seem to provide a > > .get_name() operation, would it return the ACPI bus ID here ? > > I'd really prefer not to do graph parsing outside the main parser(s), OF, > ACPI and property frameworks. > > Just for an example, the v4l2_fwnode_link_parse() was broken for ACPI for a > long time just because it did not use the graph parsing functions, but > implemented graph parsing on its own. > > >>> How about checking instead that calling > >>> fwnode_graph_get_remote_endpoint(fwnode_graph_get_remote_endpoint()) yields > >>> the same node? That would ensure you're dealing with endpoint nodes without > >>> explicitly parsing the graph in any way. > >> > >> Would it be simpler to check for the presence of an endpoint property ? > > There's no endpoint property, apart from an old ACPI definition. There isn't ? How does it work on ACPI then ? acpi_graph_get_remote_endpoint() starts with ret = acpi_node_get_property_reference(__fwnode, "remote-endpoint", 0, &args); > There are differences in the implementations and this is not the best place > to try to take them all into account. OK, but in that case I think we need an fwnode_graph_is_endpoint().
Hi Laurent, On Wed, Mar 18, 2020 at 01:22:16PM +0200, Laurent Pinchart wrote: > Hi Sakari, > > On Wed, Mar 18, 2020 at 09:52:25AM +0200, Sakari Ailus wrote: > > On Wed, Mar 18, 2020 at 02:17:26AM +0200, Laurent Pinchart wrote: > > > On Wed, Mar 18, 2020 at 01:04:32AM +0200, Laurent Pinchart wrote: > > >> On Tue, Mar 17, 2020 at 02:44:56PM +0200, Sakari Ailus wrote: > > >>> On Sun, Mar 15, 2020 at 02:55:11PM +0200, Laurent Pinchart wrote: > > >>>> fwnode matching was designed to match on nodes corresponding to a > > >>>> device. Some drivers, however, needed to match on endpoints, and have > > >>>> passed endpoint fwnodes to v4l2-async. This works when both the subdev > > >>>> and the notifier use the same fwnode types (endpoint or device), but > > >>>> makes drivers that use different types incompatible. > > >>>> > > >>>> Fix this by extending the fwnode match to handle fwnodes of different > > >>>> types. When the types (deduced from the node name) are different, > > >>>> retrieve the device fwnode for the side that provides an endpoint > > >>>> fwnode, and compare it with the device fwnode provided by the other > > >>>> side. This allows interoperability between all drivers, regardless of > > >>>> which type of fwnode they use for matching. > > >>>> > > >>>> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> > > >>>> --- > > >>>> This has been compile-tested only. Prabhakar, could you check if it > > >>>> fixes your issue ? > > >>>> > > >>>> drivers/media/v4l2-core/v4l2-async.c | 42 +++++++++++++++++++++++++++- > > >>>> 1 file changed, 41 insertions(+), 1 deletion(-) > > >>>> > > >>>> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c > > >>>> index 8bde33c21ce4..995e5464cba7 100644 > > >>>> --- a/drivers/media/v4l2-core/v4l2-async.c > > >>>> +++ b/drivers/media/v4l2-core/v4l2-async.c > > >>>> @@ -71,7 +71,47 @@ static bool match_devname(struct v4l2_subdev *sd, > > >>>> > > >>>> static bool match_fwnode(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd) > > >>>> { > > >>>> - return sd->fwnode == asd->match.fwnode; > > >>>> + struct fwnode_handle *other_fwnode; > > >>>> + struct fwnode_handle *dev_fwnode; > > >>>> + bool asd_fwnode_is_ep; > > >>>> + bool sd_fwnode_is_ep; > > >>>> + const char *name; > > >>>> + > > >>>> + /* > > >>>> + * Both the subdev and the async subdev can provide either an endpoint > > >>>> + * fwnode or a device fwnode. Start with the simple case of direct > > >>>> + * fwnode matching. > > >>>> + */ > > >>>> + if (sd->fwnode == asd->match.fwnode) > > >>>> + return true; > > >>>> + > > >>>> + /* > > >>>> + * Otherwise, check if the sd fwnode and the asd fwnode refer to an > > >>>> + * endpoint or a device. If they're of the same type, there's no match. > > >>>> + */ > > >>>> + name = fwnode_get_name(sd->fwnode); > > >>>> + sd_fwnode_is_ep = name && strstarts(name, "endpoint"); > > >>>> + name = fwnode_get_name(asd->match.fwnode); > > >>>> + asd_fwnode_is_ep = name && strstarts(name, "endpoint"); > > >>> > > >>> Apart from the fact that you're parsing graph node names here, this looks > > >>> good. > > > > > > And why is that an issue btw ? the ACPI fwnode ops seem to provide a > > > .get_name() operation, would it return the ACPI bus ID here ? > > > > I'd really prefer not to do graph parsing outside the main parser(s), OF, > > ACPI and property frameworks. > > > > Just for an example, the v4l2_fwnode_link_parse() was broken for ACPI for a > > long time just because it did not use the graph parsing functions, but > > implemented graph parsing on its own. > > > > >>> How about checking instead that calling > > >>> fwnode_graph_get_remote_endpoint(fwnode_graph_get_remote_endpoint()) yields > > >>> the same node? That would ensure you're dealing with endpoint nodes without > > >>> explicitly parsing the graph in any way. > > >> > > >> Would it be simpler to check for the presence of an endpoint property ? > > > > There's no endpoint property, apart from an old ACPI definition. > > There isn't ? How does it work on ACPI then ? > acpi_graph_get_remote_endpoint() starts with > > ret = acpi_node_get_property_reference(__fwnode, "remote-endpoint", 0, > &args); The remote-endpoint property is used in ACPI, too, yes. But the question was about a property named "endpoint". > > > There are differences in the implementations and this is not the best place > > to try to take them all into account. > > OK, but in that case I think we need an fwnode_graph_is_endpoint(). Thinking about this --- could you check fwnode_graph_get_remote_endpoint() returns non-NULL, and then put the remote? That would be more simple as you are only interested in knowing you're dealing with an endpoint. I don't object having a little helper for this though.
diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c index 8bde33c21ce4..995e5464cba7 100644 --- a/drivers/media/v4l2-core/v4l2-async.c +++ b/drivers/media/v4l2-core/v4l2-async.c @@ -71,7 +71,47 @@ static bool match_devname(struct v4l2_subdev *sd, static bool match_fwnode(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd) { - return sd->fwnode == asd->match.fwnode; + struct fwnode_handle *other_fwnode; + struct fwnode_handle *dev_fwnode; + bool asd_fwnode_is_ep; + bool sd_fwnode_is_ep; + const char *name; + + /* + * Both the subdev and the async subdev can provide either an endpoint + * fwnode or a device fwnode. Start with the simple case of direct + * fwnode matching. + */ + if (sd->fwnode == asd->match.fwnode) + return true; + + /* + * Otherwise, check if the sd fwnode and the asd fwnode refer to an + * endpoint or a device. If they're of the same type, there's no match. + */ + name = fwnode_get_name(sd->fwnode); + sd_fwnode_is_ep = name && strstarts(name, "endpoint"); + name = fwnode_get_name(asd->match.fwnode); + asd_fwnode_is_ep = name && strstarts(name, "endpoint"); + + if (sd_fwnode_is_ep == asd_fwnode_is_ep) + return false; + + /* + * The sd and asd fwnodes are of different types. Get the device fwnode + * parent of the endpoint fwnode, and compare it with the other fwnode. + */ + if (sd_fwnode_is_ep) { + dev_fwnode = fwnode_graph_get_port_parent(sd->fwnode); + other_fwnode = asd->match.fwnode; + } else { + dev_fwnode = fwnode_graph_get_port_parent(asd->match.fwnode); + other_fwnode = sd->fwnode; + } + + fwnode_handle_put(dev_fwnode); + + return dev_fwnode == other_fwnode; } static bool match_custom(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd)
fwnode matching was designed to match on nodes corresponding to a device. Some drivers, however, needed to match on endpoints, and have passed endpoint fwnodes to v4l2-async. This works when both the subdev and the notifier use the same fwnode types (endpoint or device), but makes drivers that use different types incompatible. Fix this by extending the fwnode match to handle fwnodes of different types. When the types (deduced from the node name) are different, retrieve the device fwnode for the side that provides an endpoint fwnode, and compare it with the device fwnode provided by the other side. This allows interoperability between all drivers, regardless of which type of fwnode they use for matching. Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> --- This has been compile-tested only. Prabhakar, could you check if it fixes your issue ? drivers/media/v4l2-core/v4l2-async.c | 42 +++++++++++++++++++++++++++- 1 file changed, 41 insertions(+), 1 deletion(-)