Re: [PATCH v3 05/13] media: v4l2-fwnode: Add a convenience function for registering subdevs with notifiers
Hi Steve, On Tue, May 08, 2018 at 08:55:04PM -0700, Steve Longerbeam wrote: > > > On 05/08/2018 03:28 AM, Sakari Ailus wrote: > > Hi Steve, > > > > Again, sorry about the delay. This thread got buried in my inbox. :-( > > Please see my reply below. > > > > On Mon, Apr 23, 2018 at 11:00:22AM -0700, Steve Longerbeam wrote: > > > > > > On 04/23/2018 12:14 AM, Sakari Ailus wrote: > > > > Hi Steve, > > > > > > > > On Tue, Mar 20, 2018 at 05:37:21PM -0700, Steve Longerbeam wrote: > > > > > Adds v4l2_async_register_fwnode_subdev(), which is a convenience > > > > > function > > > > > for parsing a sub-device's fwnode port endpoints for connected remote > > > > > sub-devices, registering a sub-device notifier, and then registering > > > > > the sub-device itself. > > > > > > > > > > Signed-off-by: Steve Longerbeam > > > > > --- > > > > > Changes since v2: > > > > > - fix error-out path in v4l2_async_register_fwnode_subdev() that > > > > > forgot > > > > > to put device. > > > > > Changes since v1: > > > > > - add #include to v4l2-fwnode.h for > > > > > 'struct v4l2_subdev' declaration. > > > > > --- > > > > >drivers/media/v4l2-core/v4l2-fwnode.c | 101 > > > > > ++ > > > > >include/media/v4l2-fwnode.h | 43 +++ > > > > >2 files changed, 144 insertions(+) > > > > > > > > > > diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c > > > > > b/drivers/media/v4l2-core/v4l2-fwnode.c > > > > > index 99198b9..d42024d 100644 > > > > > --- a/drivers/media/v4l2-core/v4l2-fwnode.c > > > > > +++ b/drivers/media/v4l2-core/v4l2-fwnode.c > > > > > @@ -880,6 +880,107 @@ int > > > > > v4l2_async_register_subdev_sensor_common(struct v4l2_subdev *sd) > > > > >} > > > > >EXPORT_SYMBOL_GPL(v4l2_async_register_subdev_sensor_common); > > > > > +int v4l2_async_register_fwnode_subdev( > > > > > + struct v4l2_subdev *sd, size_t asd_struct_size, > > > > > + unsigned int *ports, unsigned int num_ports, > > > > > + int (*parse_endpoint)(struct device *dev, > > > > > + struct v4l2_fwnode_endpoint *vep, > > > > > + struct v4l2_async_subdev *asd)) > > > > > +{ > > > > > + struct v4l2_async_notifier *notifier; > > > > > + struct device *dev = sd->dev; > > > > > + struct fwnode_handle *fwnode; > > > > > + unsigned int subdev_port; > > > > > + bool is_port; > > > > > + int ret; > > > > > + > > > > > + if (WARN_ON(!dev)) > > > > > + return -ENODEV; > > > > > + > > > > > + fwnode = dev_fwnode(dev); > > > > > + if (!fwnode_device_is_available(fwnode)) > > > > > + return -ENODEV; > > > > > + > > > > > + is_port = (is_of_node(fwnode) && > > > > > +of_node_cmp(to_of_node(fwnode)->name, "port") == 0); > > > > What's the intent of this and the code below? You may not parse the > > > > graph > > > > data structure here, it should be done in the actual firmware > > > > implementation instead. > > > The i.MX6 CSI sub-device registers itself from a port fwnode, so > > > the intent of the is_port code below is to support the i.MX6 CSI. > > > > > > I can remove the is_port checks, but it means > > > v4l2_async_register_fwnode_subdev() won't be usable by the CSI > > > sub-device. > > This won't scale. > > The vast majority of sub-devices register themselves as > port parent nodes. So for now at least, I think > v4l2_async_register_fwnode_subdev() could be useful to many > platforms. It's because the graph bindings define that the port nodes are sub-nodes of a device node (see Documentation/devicetree/bindings/graph.txt). It's not exactly the same as doing this in DT but still the kernel implementation pretty much assumes the same. > > > Instead, I think we'd need to separate registering > > sub-devices (through async sub-devices) and binding them with the driver > > that registered the notifier. Or at least change how that process works: a > > single sub-device can well be bound to multiple notifiers, > > Ok, that is certainly not the case now, a sub-device can only > be bound to a single notifier. > > > or multiple > > times to the same notifier while it may be registered only once. > > Anyway, this is a future generalization if I understand you > correctly. Not something to deal with here. Indeed; just FYI. > > > > > > > Also, sub-devices generally do not match ports. > > > Yes that's generally true, sub-devices generally match to port parent > > > nodes. But I do know of one other sub-device that buck that trend. > > > The ADV748x CSI-2 output sub-devices match against endpoint nodes. > > Endpoints, yes, but not ports. > > Well, the imx CSI registers from a port node. > > > > > > >How sub-devices generally > > > > correspond to fwnodes is up to the device. > > > What do you think of adding a v4l2_async_register_port_fwnode_subdev(), > > > and a v4l2_async_register_endpoint_fwnode_subdev() to support such >
Re: [PATCH v3 05/13] media: v4l2-fwnode: Add a convenience function for registering subdevs with notifiers
On 06/26/2018 12:45 AM, Sakari Ailus wrote: On Tue, May 08, 2018 at 08:55:04PM -0700, Steve Longerbeam wrote: On 05/08/2018 03:28 AM, Sakari Ailus wrote: Hi Steve, Again, sorry about the delay. This thread got buried in my inbox. :-( Please see my reply below. On Mon, Apr 23, 2018 at 11:00:22AM -0700, Steve Longerbeam wrote: On 04/23/2018 12:14 AM, Sakari Ailus wrote: Hi Steve, On Tue, Mar 20, 2018 at 05:37:21PM -0700, Steve Longerbeam wrote: Adds v4l2_async_register_fwnode_subdev(), which is a convenience function for parsing a sub-device's fwnode port endpoints for connected remote sub-devices, registering a sub-device notifier, and then registering the sub-device itself. Signed-off-by: Steve Longerbeam --- Changes since v2: - fix error-out path in v4l2_async_register_fwnode_subdev() that forgot to put device. Changes since v1: - add #include to v4l2-fwnode.h for 'struct v4l2_subdev' declaration. --- drivers/media/v4l2-core/v4l2-fwnode.c | 101 ++ include/media/v4l2-fwnode.h | 43 +++ 2 files changed, 144 insertions(+) diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c index 99198b9..d42024d 100644 --- a/drivers/media/v4l2-core/v4l2-fwnode.c +++ b/drivers/media/v4l2-core/v4l2-fwnode.c @@ -880,6 +880,107 @@ int v4l2_async_register_subdev_sensor_common(struct v4l2_subdev *sd) } EXPORT_SYMBOL_GPL(v4l2_async_register_subdev_sensor_common); +int v4l2_async_register_fwnode_subdev( + struct v4l2_subdev *sd, size_t asd_struct_size, + unsigned int *ports, unsigned int num_ports, + int (*parse_endpoint)(struct device *dev, + struct v4l2_fwnode_endpoint *vep, + struct v4l2_async_subdev *asd)) +{ + struct v4l2_async_notifier *notifier; + struct device *dev = sd->dev; + struct fwnode_handle *fwnode; + unsigned int subdev_port; + bool is_port; + int ret; + + if (WARN_ON(!dev)) + return -ENODEV; + + fwnode = dev_fwnode(dev); + if (!fwnode_device_is_available(fwnode)) + return -ENODEV; + + is_port = (is_of_node(fwnode) && + of_node_cmp(to_of_node(fwnode)->name, "port") == 0); What's the intent of this and the code below? You may not parse the graph data structure here, it should be done in the actual firmware implementation instead. The i.MX6 CSI sub-device registers itself from a port fwnode, so the intent of the is_port code below is to support the i.MX6 CSI. I can remove the is_port checks, but it means v4l2_async_register_fwnode_subdev() won't be usable by the CSI sub-device. This won't scale. The vast majority of sub-devices register themselves as port parent nodes. So for now at least, I think v4l2_async_register_fwnode_subdev() could be useful to many platforms. Instead, I think we'd need to separate registering sub-devices (through async sub-devices) and binding them with the driver that registered the notifier. Or at least change how that process works: a single sub-device can well be bound to multiple notifiers, Ok, that is certainly not the case now, a sub-device can only be bound to a single notifier. or multiple times to the same notifier while it may be registered only once. Anyway, this is a future generalization if I understand you correctly. Not something to deal with here. Also, sub-devices generally do not match ports. Yes that's generally true, sub-devices generally match to port parent nodes. But I do know of one other sub-device that buck that trend. The ADV748x CSI-2 output sub-devices match against endpoint nodes. Endpoints, yes, but not ports. Well, the imx CSI registers from a port node. I looked at the imx driver and it appears to be parsing DT without much help from the frameworks; graph or V4L2 fwnode. Is there a reason to do so, other than it's a staging driver? :-) That's the whole point of this patchset. It gets rid of the code in imx that discovers subdevices by recursively walking the graph, replacing with the subdev notifier framework. Do you happen to have any DT source (or bindings) for this? Documentation/devicetree/bindings/media/imx.txt. For example DT source, it's all in the imx6 reference boards, see imx6qdl-sabresd.dtsi for example. It'd help to understand what is the aim here. The aim of what? Of this specific commit? I'll reprint it here: Adds v4l2_async_register_fwnode_subdev(), which is a convenience function for parsing a sub-device's fwnode port endpoints for connected remote sub-devices, registering a sub-device notifier, and then registering the sub-device itself. Steve
Re: [PATCH v3 05/13] media: v4l2-fwnode: Add a convenience function for registering subdevs with notifiers
On Tue, May 08, 2018 at 08:55:04PM -0700, Steve Longerbeam wrote: > > > On 05/08/2018 03:28 AM, Sakari Ailus wrote: > > Hi Steve, > > > > Again, sorry about the delay. This thread got buried in my inbox. :-( > > Please see my reply below. > > > > On Mon, Apr 23, 2018 at 11:00:22AM -0700, Steve Longerbeam wrote: > > > > > > On 04/23/2018 12:14 AM, Sakari Ailus wrote: > > > > Hi Steve, > > > > > > > > On Tue, Mar 20, 2018 at 05:37:21PM -0700, Steve Longerbeam wrote: > > > > > Adds v4l2_async_register_fwnode_subdev(), which is a convenience > > > > > function > > > > > for parsing a sub-device's fwnode port endpoints for connected remote > > > > > sub-devices, registering a sub-device notifier, and then registering > > > > > the sub-device itself. > > > > > > > > > > Signed-off-by: Steve Longerbeam > > > > > --- > > > > > Changes since v2: > > > > > - fix error-out path in v4l2_async_register_fwnode_subdev() that > > > > > forgot > > > > > to put device. > > > > > Changes since v1: > > > > > - add #include to v4l2-fwnode.h for > > > > > 'struct v4l2_subdev' declaration. > > > > > --- > > > > >drivers/media/v4l2-core/v4l2-fwnode.c | 101 > > > > > ++ > > > > >include/media/v4l2-fwnode.h | 43 +++ > > > > >2 files changed, 144 insertions(+) > > > > > > > > > > diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c > > > > > b/drivers/media/v4l2-core/v4l2-fwnode.c > > > > > index 99198b9..d42024d 100644 > > > > > --- a/drivers/media/v4l2-core/v4l2-fwnode.c > > > > > +++ b/drivers/media/v4l2-core/v4l2-fwnode.c > > > > > @@ -880,6 +880,107 @@ int > > > > > v4l2_async_register_subdev_sensor_common(struct v4l2_subdev *sd) > > > > >} > > > > >EXPORT_SYMBOL_GPL(v4l2_async_register_subdev_sensor_common); > > > > > +int v4l2_async_register_fwnode_subdev( > > > > > + struct v4l2_subdev *sd, size_t asd_struct_size, > > > > > + unsigned int *ports, unsigned int num_ports, > > > > > + int (*parse_endpoint)(struct device *dev, > > > > > + struct v4l2_fwnode_endpoint *vep, > > > > > + struct v4l2_async_subdev *asd)) > > > > > +{ > > > > > + struct v4l2_async_notifier *notifier; > > > > > + struct device *dev = sd->dev; > > > > > + struct fwnode_handle *fwnode; > > > > > + unsigned int subdev_port; > > > > > + bool is_port; > > > > > + int ret; > > > > > + > > > > > + if (WARN_ON(!dev)) > > > > > + return -ENODEV; > > > > > + > > > > > + fwnode = dev_fwnode(dev); > > > > > + if (!fwnode_device_is_available(fwnode)) > > > > > + return -ENODEV; > > > > > + > > > > > + is_port = (is_of_node(fwnode) && > > > > > +of_node_cmp(to_of_node(fwnode)->name, "port") == 0); > > > > What's the intent of this and the code below? You may not parse the > > > > graph > > > > data structure here, it should be done in the actual firmware > > > > implementation instead. > > > The i.MX6 CSI sub-device registers itself from a port fwnode, so > > > the intent of the is_port code below is to support the i.MX6 CSI. > > > > > > I can remove the is_port checks, but it means > > > v4l2_async_register_fwnode_subdev() won't be usable by the CSI > > > sub-device. > > This won't scale. > > The vast majority of sub-devices register themselves as > port parent nodes. So for now at least, I think > v4l2_async_register_fwnode_subdev() could be useful to many > platforms. > > > Instead, I think we'd need to separate registering > > sub-devices (through async sub-devices) and binding them with the driver > > that registered the notifier. Or at least change how that process works: a > > single sub-device can well be bound to multiple notifiers, > > Ok, that is certainly not the case now, a sub-device can only > be bound to a single notifier. > > > or multiple > > times to the same notifier while it may be registered only once. > > Anyway, this is a future generalization if I understand you > correctly. Not something to deal with here. > > > > > > > Also, sub-devices generally do not match ports. > > > Yes that's generally true, sub-devices generally match to port parent > > > nodes. But I do know of one other sub-device that buck that trend. > > > The ADV748x CSI-2 output sub-devices match against endpoint nodes. > > Endpoints, yes, but not ports. > > Well, the imx CSI registers from a port node. I looked at the imx driver and it appears to be parsing DT without much help from the frameworks; graph or V4L2 fwnode. Is there a reason to do so, other than it's a staging driver? :-) Do you happen to have any DT source (or bindings) for this? It'd help to understand what is the aim here. > > > > > > >How sub-devices generally > > > > correspond to fwnodes is up to the device. > > > What do you think of adding a v4l2_async_register_port_fwnode_subdev(), > > > and a v4l2_async_register_endpoint_fwnode_subdev() to suppo
Re: [PATCH v3 05/13] media: v4l2-fwnode: Add a convenience function for registering subdevs with notifiers
On 05/08/2018 03:28 AM, Sakari Ailus wrote: Hi Steve, Again, sorry about the delay. This thread got buried in my inbox. :-( Please see my reply below. On Mon, Apr 23, 2018 at 11:00:22AM -0700, Steve Longerbeam wrote: On 04/23/2018 12:14 AM, Sakari Ailus wrote: Hi Steve, On Tue, Mar 20, 2018 at 05:37:21PM -0700, Steve Longerbeam wrote: Adds v4l2_async_register_fwnode_subdev(), which is a convenience function for parsing a sub-device's fwnode port endpoints for connected remote sub-devices, registering a sub-device notifier, and then registering the sub-device itself. Signed-off-by: Steve Longerbeam --- Changes since v2: - fix error-out path in v4l2_async_register_fwnode_subdev() that forgot to put device. Changes since v1: - add #include to v4l2-fwnode.h for 'struct v4l2_subdev' declaration. --- drivers/media/v4l2-core/v4l2-fwnode.c | 101 ++ include/media/v4l2-fwnode.h | 43 +++ 2 files changed, 144 insertions(+) diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c index 99198b9..d42024d 100644 --- a/drivers/media/v4l2-core/v4l2-fwnode.c +++ b/drivers/media/v4l2-core/v4l2-fwnode.c @@ -880,6 +880,107 @@ int v4l2_async_register_subdev_sensor_common(struct v4l2_subdev *sd) } EXPORT_SYMBOL_GPL(v4l2_async_register_subdev_sensor_common); +int v4l2_async_register_fwnode_subdev( + struct v4l2_subdev *sd, size_t asd_struct_size, + unsigned int *ports, unsigned int num_ports, + int (*parse_endpoint)(struct device *dev, + struct v4l2_fwnode_endpoint *vep, + struct v4l2_async_subdev *asd)) +{ + struct v4l2_async_notifier *notifier; + struct device *dev = sd->dev; + struct fwnode_handle *fwnode; + unsigned int subdev_port; + bool is_port; + int ret; + + if (WARN_ON(!dev)) + return -ENODEV; + + fwnode = dev_fwnode(dev); + if (!fwnode_device_is_available(fwnode)) + return -ENODEV; + + is_port = (is_of_node(fwnode) && + of_node_cmp(to_of_node(fwnode)->name, "port") == 0); What's the intent of this and the code below? You may not parse the graph data structure here, it should be done in the actual firmware implementation instead. The i.MX6 CSI sub-device registers itself from a port fwnode, so the intent of the is_port code below is to support the i.MX6 CSI. I can remove the is_port checks, but it means v4l2_async_register_fwnode_subdev() won't be usable by the CSI sub-device. This won't scale. The vast majority of sub-devices register themselves as port parent nodes. So for now at least, I think v4l2_async_register_fwnode_subdev() could be useful to many platforms. Instead, I think we'd need to separate registering sub-devices (through async sub-devices) and binding them with the driver that registered the notifier. Or at least change how that process works: a single sub-device can well be bound to multiple notifiers, Ok, that is certainly not the case now, a sub-device can only be bound to a single notifier. or multiple times to the same notifier while it may be registered only once. Anyway, this is a future generalization if I understand you correctly. Not something to deal with here. Also, sub-devices generally do not match ports. Yes that's generally true, sub-devices generally match to port parent nodes. But I do know of one other sub-device that buck that trend. The ADV748x CSI-2 output sub-devices match against endpoint nodes. Endpoints, yes, but not ports. Well, the imx CSI registers from a port node. How sub-devices generally correspond to fwnodes is up to the device. What do you think of adding a v4l2_async_register_port_fwnode_subdev(), and a v4l2_async_register_endpoint_fwnode_subdev() to support such sub-devices? The endpoint is more specific than a port, so why the port and not the endpoint? Do you mean there should be a v4l2_async_register_endpoint_fwnode_subdev() but not v4l2_async_register_endpoint_port_subdev()? Steve
Re: [PATCH v3 05/13] media: v4l2-fwnode: Add a convenience function for registering subdevs with notifiers
Hi Steve, Again, sorry about the delay. This thread got buried in my inbox. :-( Please see my reply below. On Mon, Apr 23, 2018 at 11:00:22AM -0700, Steve Longerbeam wrote: > > > On 04/23/2018 12:14 AM, Sakari Ailus wrote: > > Hi Steve, > > > > On Tue, Mar 20, 2018 at 05:37:21PM -0700, Steve Longerbeam wrote: > > > Adds v4l2_async_register_fwnode_subdev(), which is a convenience function > > > for parsing a sub-device's fwnode port endpoints for connected remote > > > sub-devices, registering a sub-device notifier, and then registering > > > the sub-device itself. > > > > > > Signed-off-by: Steve Longerbeam > > > --- > > > Changes since v2: > > > - fix error-out path in v4l2_async_register_fwnode_subdev() that forgot > > >to put device. > > > Changes since v1: > > > - add #include to v4l2-fwnode.h for > > >'struct v4l2_subdev' declaration. > > > --- > > > drivers/media/v4l2-core/v4l2-fwnode.c | 101 > > > ++ > > > include/media/v4l2-fwnode.h | 43 +++ > > > 2 files changed, 144 insertions(+) > > > > > > diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c > > > b/drivers/media/v4l2-core/v4l2-fwnode.c > > > index 99198b9..d42024d 100644 > > > --- a/drivers/media/v4l2-core/v4l2-fwnode.c > > > +++ b/drivers/media/v4l2-core/v4l2-fwnode.c > > > @@ -880,6 +880,107 @@ int v4l2_async_register_subdev_sensor_common(struct > > > v4l2_subdev *sd) > > > } > > > EXPORT_SYMBOL_GPL(v4l2_async_register_subdev_sensor_common); > > > +int v4l2_async_register_fwnode_subdev( > > > + struct v4l2_subdev *sd, size_t asd_struct_size, > > > + unsigned int *ports, unsigned int num_ports, > > > + int (*parse_endpoint)(struct device *dev, > > > + struct v4l2_fwnode_endpoint *vep, > > > + struct v4l2_async_subdev *asd)) > > > +{ > > > + struct v4l2_async_notifier *notifier; > > > + struct device *dev = sd->dev; > > > + struct fwnode_handle *fwnode; > > > + unsigned int subdev_port; > > > + bool is_port; > > > + int ret; > > > + > > > + if (WARN_ON(!dev)) > > > + return -ENODEV; > > > + > > > + fwnode = dev_fwnode(dev); > > > + if (!fwnode_device_is_available(fwnode)) > > > + return -ENODEV; > > > + > > > + is_port = (is_of_node(fwnode) && > > > +of_node_cmp(to_of_node(fwnode)->name, "port") == 0); > > What's the intent of this and the code below? You may not parse the graph > > data structure here, it should be done in the actual firmware > > implementation instead. > > The i.MX6 CSI sub-device registers itself from a port fwnode, so > the intent of the is_port code below is to support the i.MX6 CSI. > > I can remove the is_port checks, but it means > v4l2_async_register_fwnode_subdev() won't be usable by the CSI > sub-device. This won't scale. Instead, I think we'd need to separate registering sub-devices (through async sub-devices) and binding them with the driver that registered the notifier. Or at least change how that process works: a single sub-device can well be bound to multiple notifiers, or multiple times to the same notifier while it may be registered only once. > > > > > Also, sub-devices generally do not match ports. > > Yes that's generally true, sub-devices generally match to port parent > nodes. But I do know of one other sub-device that buck that trend. > The ADV748x CSI-2 output sub-devices match against endpoint nodes. Endpoints, yes, but not ports. > > > How sub-devices generally > > correspond to fwnodes is up to the device. > > What do you think of adding a v4l2_async_register_port_fwnode_subdev(), > and a v4l2_async_register_endpoint_fwnode_subdev() to support such > sub-devices? The endpoint is more specific than a port, so why the port and not the endpoint? -- Sakari Ailus sakari.ai...@linux.intel.com
Re: [PATCH v3 05/13] media: v4l2-fwnode: Add a convenience function for registering subdevs with notifiers
On 04/23/2018 12:14 AM, Sakari Ailus wrote: Hi Steve, On Tue, Mar 20, 2018 at 05:37:21PM -0700, Steve Longerbeam wrote: Adds v4l2_async_register_fwnode_subdev(), which is a convenience function for parsing a sub-device's fwnode port endpoints for connected remote sub-devices, registering a sub-device notifier, and then registering the sub-device itself. Signed-off-by: Steve Longerbeam --- Changes since v2: - fix error-out path in v4l2_async_register_fwnode_subdev() that forgot to put device. Changes since v1: - add #include to v4l2-fwnode.h for 'struct v4l2_subdev' declaration. --- drivers/media/v4l2-core/v4l2-fwnode.c | 101 ++ include/media/v4l2-fwnode.h | 43 +++ 2 files changed, 144 insertions(+) diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c index 99198b9..d42024d 100644 --- a/drivers/media/v4l2-core/v4l2-fwnode.c +++ b/drivers/media/v4l2-core/v4l2-fwnode.c @@ -880,6 +880,107 @@ int v4l2_async_register_subdev_sensor_common(struct v4l2_subdev *sd) } EXPORT_SYMBOL_GPL(v4l2_async_register_subdev_sensor_common); +int v4l2_async_register_fwnode_subdev( + struct v4l2_subdev *sd, size_t asd_struct_size, + unsigned int *ports, unsigned int num_ports, + int (*parse_endpoint)(struct device *dev, + struct v4l2_fwnode_endpoint *vep, + struct v4l2_async_subdev *asd)) +{ + struct v4l2_async_notifier *notifier; + struct device *dev = sd->dev; + struct fwnode_handle *fwnode; + unsigned int subdev_port; + bool is_port; + int ret; + + if (WARN_ON(!dev)) + return -ENODEV; + + fwnode = dev_fwnode(dev); + if (!fwnode_device_is_available(fwnode)) + return -ENODEV; + + is_port = (is_of_node(fwnode) && + of_node_cmp(to_of_node(fwnode)->name, "port") == 0); What's the intent of this and the code below? You may not parse the graph data structure here, it should be done in the actual firmware implementation instead. The i.MX6 CSI sub-device registers itself from a port fwnode, so the intent of the is_port code below is to support the i.MX6 CSI. I can remove the is_port checks, but it means v4l2_async_register_fwnode_subdev() won't be usable by the CSI sub-device. Also, sub-devices generally do not match ports. Yes that's generally true, sub-devices generally match to port parent nodes. But I do know of one other sub-device that buck that trend. The ADV748x CSI-2 output sub-devices match against endpoint nodes. How sub-devices generally correspond to fwnodes is up to the device. What do you think of adding a v4l2_async_register_port_fwnode_subdev(), and a v4l2_async_register_endpoint_fwnode_subdev() to support such sub-devices? Steve + + /* +* If the sub-device is a port, only parse fwnode endpoints from +* this sub-device's single port id. +*/ + if (is_port) { + /* verify the caller did not provide a ports array */ + if (ports) + return -EINVAL; + + ret = fwnode_property_read_u32(fwnode, "reg", &subdev_port); + if (ret < 0) + return ret; + + /* +* the device given to the fwnode endpoint parsing +* must be the port sub-device's parent. +*/ + dev = get_device(sd->dev->parent); + + if (WARN_ON(!dev)) + return -ENODEV; + + ports = &subdev_port; + num_ports = 1; + } + + notifier = kzalloc(sizeof(*notifier), GFP_KERNEL); + if (!notifier) { + ret = -ENOMEM; + goto out_putdev; + } + + if (!ports) { + ret = v4l2_async_notifier_parse_fwnode_endpoints( + dev, notifier, asd_struct_size, parse_endpoint); + if (ret < 0) + goto out_cleanup; + } else { + unsigned int i; + + for (i = 0; i < num_ports; i++) { + ret = v4l2_async_notifier_parse_fwnode_endpoints_by_port( + dev, notifier, asd_struct_size, + ports[i], parse_endpoint); + if (ret < 0) + goto out_cleanup; + } + } + + ret = v4l2_async_subdev_notifier_register(sd, notifier); + if (ret < 0) + goto out_cleanup; + + ret = v4l2_async_register_subdev(sd); + if (ret < 0) + goto out_unregister; + + sd->subdev_notifier = notifier; + + if (is_port) + put_device(dev); + + return 0; + +out_unregister: + v4l2_async_notifier_unregister(notifier); +out_cleanup: + v4l2_async_notifier_cleanup(notifier);
Re: [PATCH v3 05/13] media: v4l2-fwnode: Add a convenience function for registering subdevs with notifiers
Hi Steve, On Tue, Mar 20, 2018 at 05:37:21PM -0700, Steve Longerbeam wrote: > Adds v4l2_async_register_fwnode_subdev(), which is a convenience function > for parsing a sub-device's fwnode port endpoints for connected remote > sub-devices, registering a sub-device notifier, and then registering > the sub-device itself. > > Signed-off-by: Steve Longerbeam > --- > Changes since v2: > - fix error-out path in v4l2_async_register_fwnode_subdev() that forgot > to put device. > Changes since v1: > - add #include to v4l2-fwnode.h for > 'struct v4l2_subdev' declaration. > --- > drivers/media/v4l2-core/v4l2-fwnode.c | 101 > ++ > include/media/v4l2-fwnode.h | 43 +++ > 2 files changed, 144 insertions(+) > > diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c > b/drivers/media/v4l2-core/v4l2-fwnode.c > index 99198b9..d42024d 100644 > --- a/drivers/media/v4l2-core/v4l2-fwnode.c > +++ b/drivers/media/v4l2-core/v4l2-fwnode.c > @@ -880,6 +880,107 @@ int v4l2_async_register_subdev_sensor_common(struct > v4l2_subdev *sd) > } > EXPORT_SYMBOL_GPL(v4l2_async_register_subdev_sensor_common); > > +int v4l2_async_register_fwnode_subdev( > + struct v4l2_subdev *sd, size_t asd_struct_size, > + unsigned int *ports, unsigned int num_ports, > + int (*parse_endpoint)(struct device *dev, > + struct v4l2_fwnode_endpoint *vep, > + struct v4l2_async_subdev *asd)) > +{ > + struct v4l2_async_notifier *notifier; > + struct device *dev = sd->dev; > + struct fwnode_handle *fwnode; > + unsigned int subdev_port; > + bool is_port; > + int ret; > + > + if (WARN_ON(!dev)) > + return -ENODEV; > + > + fwnode = dev_fwnode(dev); > + if (!fwnode_device_is_available(fwnode)) > + return -ENODEV; > + > + is_port = (is_of_node(fwnode) && > +of_node_cmp(to_of_node(fwnode)->name, "port") == 0); What's the intent of this and the code below? You may not parse the graph data structure here, it should be done in the actual firmware implementation instead. Also, sub-devices generally do not match ports. How sub-devices generally correspond to fwnodes is up to the device. > + > + /* > + * If the sub-device is a port, only parse fwnode endpoints from > + * this sub-device's single port id. > + */ > + if (is_port) { > + /* verify the caller did not provide a ports array */ > + if (ports) > + return -EINVAL; > + > + ret = fwnode_property_read_u32(fwnode, "reg", &subdev_port); > + if (ret < 0) > + return ret; > + > + /* > + * the device given to the fwnode endpoint parsing > + * must be the port sub-device's parent. > + */ > + dev = get_device(sd->dev->parent); > + > + if (WARN_ON(!dev)) > + return -ENODEV; > + > + ports = &subdev_port; > + num_ports = 1; > + } > + > + notifier = kzalloc(sizeof(*notifier), GFP_KERNEL); > + if (!notifier) { > + ret = -ENOMEM; > + goto out_putdev; > + } > + > + if (!ports) { > + ret = v4l2_async_notifier_parse_fwnode_endpoints( > + dev, notifier, asd_struct_size, parse_endpoint); > + if (ret < 0) > + goto out_cleanup; > + } else { > + unsigned int i; > + > + for (i = 0; i < num_ports; i++) { > + ret = > v4l2_async_notifier_parse_fwnode_endpoints_by_port( > + dev, notifier, asd_struct_size, > + ports[i], parse_endpoint); > + if (ret < 0) > + goto out_cleanup; > + } > + } > + > + ret = v4l2_async_subdev_notifier_register(sd, notifier); > + if (ret < 0) > + goto out_cleanup; > + > + ret = v4l2_async_register_subdev(sd); > + if (ret < 0) > + goto out_unregister; > + > + sd->subdev_notifier = notifier; > + > + if (is_port) > + put_device(dev); > + > + return 0; > + > +out_unregister: > + v4l2_async_notifier_unregister(notifier); > +out_cleanup: > + v4l2_async_notifier_cleanup(notifier); > + kfree(notifier); > +out_putdev: > + if (is_port) > + put_device(dev); > + > + return ret; > +} > +EXPORT_SYMBOL_GPL(v4l2_async_register_fwnode_subdev); > + > MODULE_LICENSE("GPL"); > MODULE_AUTHOR("Sakari Ailus "); > MODULE_AUTHOR("Sylwester Nawrocki "); > diff --git a/include/media/v4l2-fwnode.h b/include/media/v4l2-fwnode.h > index 9a4b3f8..4de0ac2 100644 > --- a/include/media/v4l2-fwnode.h > +++ b/include/media/v4l2-fwnode.h > @@ -23,6 +23,7 @@ > #include > > #include > +#include > > struct fwnode_handle; > struct v4l2_async_notifier; >
[PATCH v3 05/13] media: v4l2-fwnode: Add a convenience function for registering subdevs with notifiers
Adds v4l2_async_register_fwnode_subdev(), which is a convenience function for parsing a sub-device's fwnode port endpoints for connected remote sub-devices, registering a sub-device notifier, and then registering the sub-device itself. Signed-off-by: Steve Longerbeam --- Changes since v2: - fix error-out path in v4l2_async_register_fwnode_subdev() that forgot to put device. Changes since v1: - add #include to v4l2-fwnode.h for 'struct v4l2_subdev' declaration. --- drivers/media/v4l2-core/v4l2-fwnode.c | 101 ++ include/media/v4l2-fwnode.h | 43 +++ 2 files changed, 144 insertions(+) diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c index 99198b9..d42024d 100644 --- a/drivers/media/v4l2-core/v4l2-fwnode.c +++ b/drivers/media/v4l2-core/v4l2-fwnode.c @@ -880,6 +880,107 @@ int v4l2_async_register_subdev_sensor_common(struct v4l2_subdev *sd) } EXPORT_SYMBOL_GPL(v4l2_async_register_subdev_sensor_common); +int v4l2_async_register_fwnode_subdev( + struct v4l2_subdev *sd, size_t asd_struct_size, + unsigned int *ports, unsigned int num_ports, + int (*parse_endpoint)(struct device *dev, + struct v4l2_fwnode_endpoint *vep, + struct v4l2_async_subdev *asd)) +{ + struct v4l2_async_notifier *notifier; + struct device *dev = sd->dev; + struct fwnode_handle *fwnode; + unsigned int subdev_port; + bool is_port; + int ret; + + if (WARN_ON(!dev)) + return -ENODEV; + + fwnode = dev_fwnode(dev); + if (!fwnode_device_is_available(fwnode)) + return -ENODEV; + + is_port = (is_of_node(fwnode) && + of_node_cmp(to_of_node(fwnode)->name, "port") == 0); + + /* +* If the sub-device is a port, only parse fwnode endpoints from +* this sub-device's single port id. +*/ + if (is_port) { + /* verify the caller did not provide a ports array */ + if (ports) + return -EINVAL; + + ret = fwnode_property_read_u32(fwnode, "reg", &subdev_port); + if (ret < 0) + return ret; + + /* +* the device given to the fwnode endpoint parsing +* must be the port sub-device's parent. +*/ + dev = get_device(sd->dev->parent); + + if (WARN_ON(!dev)) + return -ENODEV; + + ports = &subdev_port; + num_ports = 1; + } + + notifier = kzalloc(sizeof(*notifier), GFP_KERNEL); + if (!notifier) { + ret = -ENOMEM; + goto out_putdev; + } + + if (!ports) { + ret = v4l2_async_notifier_parse_fwnode_endpoints( + dev, notifier, asd_struct_size, parse_endpoint); + if (ret < 0) + goto out_cleanup; + } else { + unsigned int i; + + for (i = 0; i < num_ports; i++) { + ret = v4l2_async_notifier_parse_fwnode_endpoints_by_port( + dev, notifier, asd_struct_size, + ports[i], parse_endpoint); + if (ret < 0) + goto out_cleanup; + } + } + + ret = v4l2_async_subdev_notifier_register(sd, notifier); + if (ret < 0) + goto out_cleanup; + + ret = v4l2_async_register_subdev(sd); + if (ret < 0) + goto out_unregister; + + sd->subdev_notifier = notifier; + + if (is_port) + put_device(dev); + + return 0; + +out_unregister: + v4l2_async_notifier_unregister(notifier); +out_cleanup: + v4l2_async_notifier_cleanup(notifier); + kfree(notifier); +out_putdev: + if (is_port) + put_device(dev); + + return ret; +} +EXPORT_SYMBOL_GPL(v4l2_async_register_fwnode_subdev); + MODULE_LICENSE("GPL"); MODULE_AUTHOR("Sakari Ailus "); MODULE_AUTHOR("Sylwester Nawrocki "); diff --git a/include/media/v4l2-fwnode.h b/include/media/v4l2-fwnode.h index 9a4b3f8..4de0ac2 100644 --- a/include/media/v4l2-fwnode.h +++ b/include/media/v4l2-fwnode.h @@ -23,6 +23,7 @@ #include #include +#include struct fwnode_handle; struct v4l2_async_notifier; @@ -360,4 +361,46 @@ int v4l2_async_notifier_parse_fwnode_endpoints_by_port( int v4l2_async_notifier_parse_fwnode_sensor_common( struct device *dev, struct v4l2_async_notifier *notifier); +/** + * v4l2_async_register_fwnode_subdev - registers a sub-device to the + * asynchronous sub-device framework + * and parses fwnode endpoints + * + * @sd: pointer to struct &v4l2_subdev + * @asd_struct_size: size of the driver'