Re: [PATCH v3 05/13] media: v4l2-fwnode: Add a convenience function for registering subdevs with notifiers

[Sakari Ailus]

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

[Steve Longerbeam]

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

[Sakari Ailus]

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 

Re: [PATCH v3 05/13] media: v4l2-fwnode: Add a convenience function for registering subdevs with notifiers

[Steve Longerbeam]

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

[Sakari Ailus]

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

[Steve Longerbeam]

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", _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 = _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:
+   

Re: [PATCH v3 05/13] media: v4l2-fwnode: Add a convenience function for registering subdevs with notifiers

[Sakari Ailus]

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", _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 = _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 
>