| Message ID | 20201019225903.14276-1-djrscally@gmail.com |
|---|---|
| Headers | show |
| Series | Add functionality to ipu3-cio2 driver allowing software_node connections to sensors on platforms designed for Windows | expand |
On Mon, Oct 19, 2020 at 11:59:00PM +0100, Daniel Scally wrote: > ipu3-cio2 driver needs extending with multiple files; rename the main > source file and specify the renamed file in Makefile to accommodate that. Suggested-by? Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> after addressing below comment. > Signed-off-by: Daniel Scally <djrscally@gmail.com> > --- > Changes in v3: > - patch introduced > > drivers/media/pci/intel/ipu3/Makefile | 2 ++ > drivers/media/pci/intel/ipu3/{ipu3-cio2.c => ipu3-cio2-main.c} | 0 > 2 files changed, 2 insertions(+) > rename drivers/media/pci/intel/ipu3/{ipu3-cio2.c => ipu3-cio2-main.c} (100%) > > diff --git a/drivers/media/pci/intel/ipu3/Makefile b/drivers/media/pci/intel/ipu3/Makefile > index 98ddd5bea..b4e3266d9 100644 > --- a/drivers/media/pci/intel/ipu3/Makefile > +++ b/drivers/media/pci/intel/ipu3/Makefile > @@ -1,2 +1,4 @@ > # SPDX-License-Identifier: GPL-2.0-only > obj-$(CONFIG_VIDEO_IPU3_CIO2) += ipu3-cio2.o > + > +ipu3-cio2-y += ipu3-cio2-main.o > \ No newline at end of file Don't forget to add \n at the end of above line. > diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c b/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c > similarity index 100% > rename from drivers/media/pci/intel/ipu3/ipu3-cio2.c > rename to drivers/media/pci/intel/ipu3/ipu3-cio2-main.c > -- > 2.17.1 > -- With Best Regards, Andy Shevchenko
On Mon, Oct 19, 2020 at 11:58:59PM +0100, Daniel Scally wrote: > Development for the ipu3-cio2 driver is taking place in media_tree, but > there's no T: entry in MAINTAINERS to denote that - rectify that oversight Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Signed-off-by: Daniel Scally <djrscally@gmail.com> > --- > Changes in v3: > - patch introduced. > > MAINTAINERS | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/MAINTAINERS b/MAINTAINERS > index 43a025039..5d768d5ad 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -8851,6 +8851,7 @@ M: Bingbu Cao <bingbu.cao@intel.com> > R: Tianshu Qiu <tian.shu.qiu@intel.com> > L: linux-media@vger.kernel.org > S: Maintained > +T: git git://linuxtv.org/media_tree.git > F: Documentation/userspace-api/media/v4l/pixfmt-srggb10-ipu3.rst > F: drivers/media/pci/intel/ipu3/ > > -- > 2.17.1 >
On Mon, Oct 19, 2020 at 11:59:01PM +0100, Daniel Scally wrote: > fwnode_graph_get_endpoint_by_id() will optionally parse enabled devices > only; that status being determined through the .device_is_available() op > of the device's fwnode. As software_nodes don't have that operation and > adding it is meaningless, we instead need to check if the device's fwnode > is a software_node and if so pass the appropriate flag to disable that > check Period. I'm wondering if actually this can be hidden in fwnode_graph_get_endpoint_by_id().
On Mon, Oct 19, 2020 at 11:58:55PM +0100, Daniel Scally wrote: > Software nodes that are children of another software node should be > unregistered before their parent. To allow easy unregistering of an array > of software_nodes ordered parent to child, add a helper function to loop > over and unregister nodes in such an array in reverse order. > Suggested-by: Andriy Shevchenko <andriy.shevchenko@linux.intel.com> For all patches Andriy -> Andy (email stays as above!). ... > +/** > + * software_node_unregister_nodes_reverse - Unregister an array of software > + * nodes in reverse order. Can you shrink this to one line? Something like dropping ' in reverse order.' and adding it below... > + * @nodes: Array of software nodes to be unregistered. > + * ...like here to explain reversed order? > + * NOTE: The same warning applies as with software_node_unregister_nodes. software_node_unregister_nodes() > + * Unless you are _sure_ that the array of nodes is ordered parent to child > + * it is wiser to remove them individually in the correct order. > + */ > +void software_node_unregister_nodes_reverse(const struct software_node *nodes) > +{ > + unsigned int i = 0; > + > + while (nodes[i].name) > + i++; > + > + while (i--) > + software_node_unregister(&nodes[i]); > +}
On Mon, Oct 19, 2020 at 11:58:54PM +0100, Daniel Scally wrote: > Hello all > > This series adds support to the ipu3-cio2 driver for fwnode connections > between cio2 and sensors to be defined via software_nodes. The final patch > in the series deals wholly with those changes - the preceding patches are > either supporting changes to accommodate that or incidental fixes along > the way: > > 1/9 adds a function to drivers/base/swnode.c unwinding arrays of software > nodes in reverse order > > 2/9 uses that function in lib/test_printf.c > > 3/9 fixes what seems to me to be a bug in the existing swnode.c code in > that software_node_get_next_child() does not increase the refcount of the > returned node (in contrast to, for example, of_get_next_child_node() which > does increase the count) > > 4/9 adds the fwnode_graph*() family of functions to the software_node > implementation > > 5/9 adds a T: entry to MAINTAINERS for the ipu3-cio2 driver > > 6/9 renames the ipu3-cio2.c file to ipu3-cio2-main.c and fixes Makefile > to accommodate that change > > 7/9 alters the ipu3-cio2 driver to check if the pci_dev's fwnode is a > software_node and pass flags to fwnode_graph_get_endpoint_by_id() if so > > 8/9 alters match_fwnode() in v4l2-async.c to additionally try to match on > a fwnode_handle's secondary if the primary doesn't match > > 9/9 alters the ipu3-cio2 driver to do the actual building of software_node > connections between the sensor devices and the cio2 device. Thanks! I have mostly cosmetic comments. > This is still not ready for integration - hence the RFC label - as there > is ongoing work to extend the ipu3-cio2 driver further to parse ACPI > to discover resources such as regulators and GPIOs that are defined there > in unusual ways and map them to the sensor devices so that their drivers > can consume them transparently through the usual frameworks. Given this > has changed quite extensively from v2 though, I wanted to submit it for > feedback at this point in case it needs further large scale change. From my point of view it's a good start to drop RFC from next version (RFC v3 -> v1). But let's wait couple of weeks for people to comment (now is a second week of merge window, not everybody looking into the mailing lists). > Daniel Scally (8): > software_node: Add helper function to unregister arrays of > software_nodes ordered parent to child > lib/test_printf.c: Use helper function to unwind array of > software_nodes > software_node: Fix failure to hold refcount in > software_node_get_next_child > ipu3-cio2: Add T: entry to MAINTAINERS > ipu3-cio2: Rename ipu3-cio2.c to allow module to be built from > multiple sources files retaining ipu3-cio2 name > ipu3-cio2: Check if pci_dev->dev's fwnode is a software_node in > cio2_parse_firmware() and set FWNODE_GRAPH_DEVICE_DISABLED if so > media: v4l2-core: v4l2-async: Check possible match in match_fwnode > based on sd->fwnode->secondary > ipu3-cio2: Add functionality allowing software_node connections to > sensors on platforms designed for Windows > > Heikki Krogerus (1): > software_node: Add support for fwnode_graph*() family of functions > > MAINTAINERS | 2 + > drivers/base/swnode.c | 143 +++++++- > drivers/media/pci/intel/ipu3/Kconfig | 18 + > drivers/media/pci/intel/ipu3/Makefile | 3 + > drivers/media/pci/intel/ipu3/cio2-bridge.c | 327 ++++++++++++++++++ > drivers/media/pci/intel/ipu3/cio2-bridge.h | 94 +++++ > .../ipu3/{ipu3-cio2.c => ipu3-cio2-main.c} | 30 +- > drivers/media/pci/intel/ipu3/ipu3-cio2.h | 9 + > drivers/media/v4l2-core/v4l2-async.c | 8 + > include/linux/property.h | 1 + > lib/test_printf.c | 4 +- > 11 files changed, 632 insertions(+), 7 deletions(-) > create mode 100644 drivers/media/pci/intel/ipu3/cio2-bridge.c > create mode 100644 drivers/media/pci/intel/ipu3/cio2-bridge.h > rename drivers/media/pci/intel/ipu3/{ipu3-cio2.c => ipu3-cio2-main.c} (98%) > > -- > 2.17.1 > -- With Best Regards, Andy Shevchenko
Hi Daniel, Andy, On Mon, Oct 19, 2020 at 11:58:55PM +0100, Daniel Scally wrote: > Software nodes that are children of another software node should be > unregistered before their parent. To allow easy unregistering of an array > of software_nodes ordered parent to child, add a helper function to loop > over and unregister nodes in such an array in reverse order. > > Suggested-by: Andriy Shevchenko <andriy.shevchenko@linux.intel.com> > Signed-off-by: Daniel Scally <djrscally@gmail.com> > --- > Changes in v3: > - patch introduced. > > drivers/base/swnode.c | 21 +++++++++++++++++++++ > include/linux/property.h | 1 + > 2 files changed, 22 insertions(+) > > diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c > index 010828fc7..f01b1cc61 100644 > --- a/drivers/base/swnode.c > +++ b/drivers/base/swnode.c > @@ -727,6 +727,27 @@ void software_node_unregister_nodes(const struct software_node *nodes) > } > EXPORT_SYMBOL_GPL(software_node_unregister_nodes); > > +/** > + * software_node_unregister_nodes_reverse - Unregister an array of software > + * nodes in reverse order. > + * @nodes: Array of software nodes to be unregistered. > + * > + * NOTE: The same warning applies as with software_node_unregister_nodes. > + * Unless you are _sure_ that the array of nodes is ordered parent to child > + * it is wiser to remove them individually in the correct order. Could the default order in software_node_unregister_nodes() be reversed instead? There are no users so this should be easy to change. Doing this only one way may require enforcing the registration order in software_node_register_nodes(), but the end result would be safer. What do you think?
On Tue, Oct 20, 2020 at 01:05:10PM +0300, Sakari Ailus wrote: > On Mon, Oct 19, 2020 at 11:58:55PM +0100, Daniel Scally wrote: > > Software nodes that are children of another software node should be > > unregistered before their parent. To allow easy unregistering of an array > > of software_nodes ordered parent to child, add a helper function to loop > > over and unregister nodes in such an array in reverse order. ... > > + * software_node_unregister_nodes_reverse - Unregister an array of software > > + * nodes in reverse order. > > + * @nodes: Array of software nodes to be unregistered. > > + * > > + * NOTE: The same warning applies as with software_node_unregister_nodes. > > + * Unless you are _sure_ that the array of nodes is ordered parent to child > > + * it is wiser to remove them individually in the correct order. > > Could the default order in software_node_unregister_nodes() be reversed > instead? There are no users so this should be easy to change. > > Doing this only one way may require enforcing the registration order in > software_node_register_nodes(), but the end result would be safer. > > What do you think? Will work for me (I would also hear Heikki). But in such case let's change the order of software_node_unregister_node_group() for the sake of consistency. -- With Best Regards, Andy Shevchenko
On Tue, Oct 20, 2020 at 02:01:55PM +0300, Andy Shevchenko wrote: > On Tue, Oct 20, 2020 at 01:05:10PM +0300, Sakari Ailus wrote: > > On Mon, Oct 19, 2020 at 11:58:55PM +0100, Daniel Scally wrote: > > > Software nodes that are children of another software node should be > > > unregistered before their parent. To allow easy unregistering of an array > > > of software_nodes ordered parent to child, add a helper function to loop > > > over and unregister nodes in such an array in reverse order. > > ... > > > > + * software_node_unregister_nodes_reverse - Unregister an array of software > > > + * nodes in reverse order. > > > + * @nodes: Array of software nodes to be unregistered. > > > + * > > > + * NOTE: The same warning applies as with software_node_unregister_nodes. > > > + * Unless you are _sure_ that the array of nodes is ordered parent to child > > > + * it is wiser to remove them individually in the correct order. > > > > Could the default order in software_node_unregister_nodes() be reversed > > instead? There are no users so this should be easy to change. > > > > Doing this only one way may require enforcing the registration order in > > software_node_register_nodes(), but the end result would be safer. > > > > What do you think? > > Will work for me (I would also hear Heikki). > > But in such case let's change the order of > software_node_unregister_node_group() for the sake of consistency. But either way we will need a note to describe the ordering.
On Tue, Oct 20, 2020 at 02:01:55PM +0300, Andy Shevchenko wrote: > On Tue, Oct 20, 2020 at 01:05:10PM +0300, Sakari Ailus wrote: > > On Mon, Oct 19, 2020 at 11:58:55PM +0100, Daniel Scally wrote: > > > Software nodes that are children of another software node should be > > > unregistered before their parent. To allow easy unregistering of an array > > > of software_nodes ordered parent to child, add a helper function to loop > > > over and unregister nodes in such an array in reverse order. > > ... > > > > + * software_node_unregister_nodes_reverse - Unregister an array of software > > > + * nodes in reverse order. > > > + * @nodes: Array of software nodes to be unregistered. > > > + * > > > + * NOTE: The same warning applies as with software_node_unregister_nodes. > > > + * Unless you are _sure_ that the array of nodes is ordered parent to child > > > + * it is wiser to remove them individually in the correct order. > > > > Could the default order in software_node_unregister_nodes() be reversed > > instead? There are no users so this should be easy to change. > > > > Doing this only one way may require enforcing the registration order in > > software_node_register_nodes(), but the end result would be safer. > > > > What do you think? > > Will work for me (I would also hear Heikki). Sounds reasonable to me. thanks, -- heikki
Hi Andy, On Tue, Oct 20, 2020 at 12:19:58PM +0300, Andy Shevchenko wrote: > On Mon, Oct 19, 2020 at 11:59:01PM +0100, Daniel Scally wrote: > > fwnode_graph_get_endpoint_by_id() will optionally parse enabled devices > > only; that status being determined through the .device_is_available() op > > of the device's fwnode. As software_nodes don't have that operation and > > adding it is meaningless, we instead need to check if the device's fwnode > > is a software_node and if so pass the appropriate flag to disable that > > check > > Period. > > I'm wondering if actually this can be hidden in fwnode_graph_get_endpoint_by_id(). The device availability test is actually there for a reason. Some firmware implementations put all the potential devices in the tables and only one (of some) of them are available. Could this be implemented so that if the node is a software node, then get its parent and then see if that is available? I guess that could be implemented in software node ops. Any opinions?
On Tue, Oct 20, 2020 at 12:59 AM Daniel Scally <djrscally@gmail.com> wrote: > > The software_node_get_next_child() function currently does not hold a kref > to the child software_node; fix that. > > Fixes: 59abd83672f7 ("drivers: base: Introducing software nodes to the firmware node framework") > Signed-off-by: Daniel Scally <djrscally@gmail.com> > --- > Changes in v3: > - split out from the full software_node_graph*() patch > > drivers/base/swnode.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c > index f01b1cc61..741149b90 100644 > --- a/drivers/base/swnode.c > +++ b/drivers/base/swnode.c > @@ -450,7 +450,7 @@ software_node_get_next_child(const struct fwnode_handle *fwnode, > c = list_next_entry(c, entry); > else > c = list_first_entry(&p->children, struct swnode, entry); > - return &c->fwnode; > + return software_node_get(&c->fwnode); > } > > static struct fwnode_handle * > -- This should be sent as a separate fix AFAICS.
Hi Daniel, On Mon, Oct 19, 2020 at 11:58:57PM +0100, Daniel Scally wrote: > The software_node_get_next_child() function currently does not hold a kref > to the child software_node; fix that. > > Fixes: 59abd83672f7 ("drivers: base: Introducing software nodes to the firmware node framework") > Signed-off-by: Daniel Scally <djrscally@gmail.com> > --- > Changes in v3: > - split out from the full software_node_graph*() patch > > drivers/base/swnode.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c > index f01b1cc61..741149b90 100644 > --- a/drivers/base/swnode.c > +++ b/drivers/base/swnode.c > @@ -450,7 +450,7 @@ software_node_get_next_child(const struct fwnode_handle *fwnode, > c = list_next_entry(c, entry); > else > c = list_first_entry(&p->children, struct swnode, entry); > - return &c->fwnode; > + return software_node_get(&c->fwnode); I believe similarly, the function should drop the reference to the previous node, and not expect the caller to do this. The OF equivalent does the same. > } > > static struct fwnode_handle *
[Fix the Linus Walleij's address.] On Tue, Oct 20, 2020 at 12:59 AM Daniel Scally <djrscally@gmail.com> wrote: > > Hello all > > This series adds support to the ipu3-cio2 driver for fwnode connections > between cio2 and sensors to be defined via software_nodes. The final patch > in the series deals wholly with those changes - the preceding patches are > either supporting changes to accommodate that or incidental fixes along > the way: > > 1/9 adds a function to drivers/base/swnode.c unwinding arrays of software > nodes in reverse order > > 2/9 uses that function in lib/test_printf.c > > 3/9 fixes what seems to me to be a bug in the existing swnode.c code in > that software_node_get_next_child() does not increase the refcount of the > returned node (in contrast to, for example, of_get_next_child_node() which > does increase the count) > > 4/9 adds the fwnode_graph*() family of functions to the software_node > implementation > > 5/9 adds a T: entry to MAINTAINERS for the ipu3-cio2 driver > > 6/9 renames the ipu3-cio2.c file to ipu3-cio2-main.c and fixes Makefile > to accommodate that change > > 7/9 alters the ipu3-cio2 driver to check if the pci_dev's fwnode is a > software_node and pass flags to fwnode_graph_get_endpoint_by_id() if so > > 8/9 alters match_fwnode() in v4l2-async.c to additionally try to match on > a fwnode_handle's secondary if the primary doesn't match > > 9/9 alters the ipu3-cio2 driver to do the actual building of software_node > connections between the sensor devices and the cio2 device. > > This is still not ready for integration - hence the RFC label - as there > is ongoing work to extend the ipu3-cio2 driver further to parse ACPI > to discover resources such as regulators and GPIOs that are defined there > in unusual ways and map them to the sensor devices so that their drivers > can consume them transparently through the usual frameworks. Given this > has changed quite extensively from v2 though, I wanted to submit it for > feedback at this point in case it needs further large scale change. I would appreciate it if you posted the next version of this series (all patches) to linux-acpi@vger.kernel.org for easier review. Thanks!
Hi Sakari On 20/10/2020 13:06, Sakari Ailus wrote: > Hi Andy, > > On Tue, Oct 20, 2020 at 12:19:58PM +0300, Andy Shevchenko wrote: >> On Mon, Oct 19, 2020 at 11:59:01PM +0100, Daniel Scally wrote: >>> fwnode_graph_get_endpoint_by_id() will optionally parse enabled devices >>> only; that status being determined through the .device_is_available() op >>> of the device's fwnode. As software_nodes don't have that operation and >>> adding it is meaningless, we instead need to check if the device's fwnode >>> is a software_node and if so pass the appropriate flag to disable that >>> check >> Period. >> >> I'm wondering if actually this can be hidden in fwnode_graph_get_endpoint_by_id(). > The device availability test is actually there for a reason. Some firmware > implementations put all the potential devices in the tables and only one > (of some) of them are available. > > Could this be implemented so that if the node is a software node, then get > its parent and then see if that is available? > > I guess that could be implemented in software node ops. Any opinions? Actually when considering the cio2 device, it seems that set_secondary_fwnode() actually overwrites the _primary_, given fwnode_is_primary(dev->fwnode) returns false. So in at least some cases, this wouldn't work.
On 20/10/2020 10:15, Andy Shevchenko wrote: > On Mon, Oct 19, 2020 at 11:59:00PM +0100, Daniel Scally wrote: >> ipu3-cio2 driver needs extending with multiple files; rename the main >> source file and specify the renamed file in Makefile to accommodate that. > Suggested-by? Oops - yes of course, will add that next version. > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > after addressing below comment. > >> Signed-off-by: Daniel Scally <djrscally@gmail.com> >> --- >> Changes in v3: >> - patch introduced >> >> drivers/media/pci/intel/ipu3/Makefile | 2 ++ >> drivers/media/pci/intel/ipu3/{ipu3-cio2.c => ipu3-cio2-main.c} | 0 >> 2 files changed, 2 insertions(+) >> rename drivers/media/pci/intel/ipu3/{ipu3-cio2.c => ipu3-cio2-main.c} (100%) >> >> diff --git a/drivers/media/pci/intel/ipu3/Makefile b/drivers/media/pci/intel/ipu3/Makefile >> index 98ddd5bea..b4e3266d9 100644 >> --- a/drivers/media/pci/intel/ipu3/Makefile >> +++ b/drivers/media/pci/intel/ipu3/Makefile >> @@ -1,2 +1,4 @@ >> # SPDX-License-Identifier: GPL-2.0-only >> obj-$(CONFIG_VIDEO_IPU3_CIO2) += ipu3-cio2.o >> + >> +ipu3-cio2-y += ipu3-cio2-main.o >> \ No newline at end of file > Don't forget to add \n at the end of above line. > >> diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c b/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c >> similarity index 100% >> rename from drivers/media/pci/intel/ipu3/ipu3-cio2.c >> rename to drivers/media/pci/intel/ipu3/ipu3-cio2-main.c >> -- >> 2.17.1 >>
Hi Dan, On Tue, Oct 20, 2020 at 08:56:07PM +0100, Dan Scally wrote: > Hi Sakari > > On 20/10/2020 13:06, Sakari Ailus wrote: > > Hi Andy, > > > > On Tue, Oct 20, 2020 at 12:19:58PM +0300, Andy Shevchenko wrote: > >> On Mon, Oct 19, 2020 at 11:59:01PM +0100, Daniel Scally wrote: > >>> fwnode_graph_get_endpoint_by_id() will optionally parse enabled devices > >>> only; that status being determined through the .device_is_available() op > >>> of the device's fwnode. As software_nodes don't have that operation and > >>> adding it is meaningless, we instead need to check if the device's fwnode > >>> is a software_node and if so pass the appropriate flag to disable that > >>> check > >> Period. > >> > >> I'm wondering if actually this can be hidden in fwnode_graph_get_endpoint_by_id(). > > The device availability test is actually there for a reason. Some firmware > > implementations put all the potential devices in the tables and only one > > (of some) of them are available. > > > > Could this be implemented so that if the node is a software node, then get > > its parent and then see if that is available? > > > > I guess that could be implemented in software node ops. Any opinions? > Actually when considering the cio2 device, it seems that > set_secondary_fwnode() actually overwrites the _primary_, given > fwnode_is_primary(dev->fwnode) returns false. So in at least some cases, > this wouldn't work. Ouch. I wonder when this happens --- have you checked what's the primary there? I guess it might be if it's a PCI device without the corresponding ACPI device node? I remember you had an is_available implementation that just returned true for software nodes in an early version of the set? I think it would still be a lesser bad in this case. -- Regards, Sakari Ailus
Hi Sakari On 20/10/2020 11:05, Sakari Ailus wrote: > Hi Daniel, Andy, > > On Mon, Oct 19, 2020 at 11:58:55PM +0100, Daniel Scally wrote: >> Software nodes that are children of another software node should be >> unregistered before their parent. To allow easy unregistering of an array >> of software_nodes ordered parent to child, add a helper function to loop >> over and unregister nodes in such an array in reverse order. >> >> Suggested-by: Andriy Shevchenko <andriy.shevchenko@linux.intel.com> >> Signed-off-by: Daniel Scally <djrscally@gmail.com> >> --- >> Changes in v3: >> - patch introduced. >> >> drivers/base/swnode.c | 21 +++++++++++++++++++++ >> include/linux/property.h | 1 + >> 2 files changed, 22 insertions(+) >> >> diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c >> index 010828fc7..f01b1cc61 100644 >> --- a/drivers/base/swnode.c >> +++ b/drivers/base/swnode.c >> @@ -727,6 +727,27 @@ void software_node_unregister_nodes(const struct software_node *nodes) >> } >> EXPORT_SYMBOL_GPL(software_node_unregister_nodes); >> >> +/** >> + * software_node_unregister_nodes_reverse - Unregister an array of software >> + * nodes in reverse order. >> + * @nodes: Array of software nodes to be unregistered. >> + * >> + * NOTE: The same warning applies as with software_node_unregister_nodes. >> + * Unless you are _sure_ that the array of nodes is ordered parent to child >> + * it is wiser to remove them individually in the correct order. > Could the default order in software_node_unregister_nodes() be reversed > instead? There are no users so this should be easy to change. > > Doing this only one way may require enforcing the registration order in > software_node_register_nodes(), but the end result would be safer. > > What do you think? Yeah fine by me. We can just use software_node_to_swnode(node->parent) within software_node_unregister_nodes() to check that children come after their parents have already been processed. I'll add a patch to do that in the next version of this series, and another changing the ordering of software_node_unregister_node_group() as Andy suggests for consistency.
Hi Sakari On 20/10/2020 23:49, Sakari Ailus wrote: > Hi Dan, > > On Tue, Oct 20, 2020 at 08:56:07PM +0100, Dan Scally wrote: >> Hi Sakari >> >> On 20/10/2020 13:06, Sakari Ailus wrote: >>> Hi Andy, >>> >>> On Tue, Oct 20, 2020 at 12:19:58PM +0300, Andy Shevchenko wrote: >>>> On Mon, Oct 19, 2020 at 11:59:01PM +0100, Daniel Scally wrote: >>>>> fwnode_graph_get_endpoint_by_id() will optionally parse enabled devices >>>>> only; that status being determined through the .device_is_available() op >>>>> of the device's fwnode. As software_nodes don't have that operation and >>>>> adding it is meaningless, we instead need to check if the device's fwnode >>>>> is a software_node and if so pass the appropriate flag to disable that >>>>> check >>>> Period. >>>> >>>> I'm wondering if actually this can be hidden in fwnode_graph_get_endpoint_by_id(). >>> The device availability test is actually there for a reason. Some firmware >>> implementations put all the potential devices in the tables and only one >>> (of some) of them are available. >>> >>> Could this be implemented so that if the node is a software node, then get >>> its parent and then see if that is available? >>> >>> I guess that could be implemented in software node ops. Any opinions? >> Actually when considering the cio2 device, it seems that >> set_secondary_fwnode() actually overwrites the _primary_, given >> fwnode_is_primary(dev->fwnode) returns false. So in at least some cases, >> this wouldn't work. > Ouch. I wonder when this happens --- have you checked what's the primary > there? I guess it might be if it's a PCI device without the corresponding > ACPI device node? Yes; it's null, and I think that diagnosis is correct. > I remember you had an is_available implementation that just returned true > for software nodes in an early version of the set? I think it would still > be a lesser bad in this case. Yep - I can put that back in and just drop this patch then; fine for me.
Hi Sakari On 20/10/2020 14:31, Sakari Ailus wrote: > Hi Daniel, > > On Mon, Oct 19, 2020 at 11:58:57PM +0100, Daniel Scally wrote: >> The software_node_get_next_child() function currently does not hold a kref >> to the child software_node; fix that. >> >> Fixes: 59abd83672f7 ("drivers: base: Introducing software nodes to the firmware node framework") >> Signed-off-by: Daniel Scally <djrscally@gmail.com> >> --- >> Changes in v3: >> - split out from the full software_node_graph*() patch >> >> drivers/base/swnode.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c >> index f01b1cc61..741149b90 100644 >> --- a/drivers/base/swnode.c >> +++ b/drivers/base/swnode.c >> @@ -450,7 +450,7 @@ software_node_get_next_child(const struct fwnode_handle *fwnode, >> c = list_next_entry(c, entry); >> else >> c = list_first_entry(&p->children, struct swnode, entry); >> - return &c->fwnode; >> + return software_node_get(&c->fwnode); > I believe similarly, the function should drop the reference to the previous > node, and not expect the caller to do this. The OF equivalent does the > same. I think I prefer it this way myself, since the alternative is having to explicitly call *_node_get() on a returned child if you want to keep it but also keep iterating. But I agree that it's important to take a consistent approach. I'll add that too; this will mean swnode_graph_find_next_port() and software_node_graph_get_next_endpoint() in patch 4 of this series will need changing slightly to square away their references.
On Wed, Oct 21, 2020 at 12:25:28AM +0100, Dan Scally wrote: > On 20/10/2020 14:31, Sakari Ailus wrote: > > On Mon, Oct 19, 2020 at 11:58:57PM +0100, Daniel Scally wrote: > >> + return software_node_get(&c->fwnode); > > I believe similarly, the function should drop the reference to the previous > > node, and not expect the caller to do this. The OF equivalent does the > > same. > > I think I prefer it this way myself, since the alternative is having to > explicitly call *_node_get() on a returned child if you want to keep it > but also keep iterating. But I agree that it's important to take a > consistent approach. I'll add that too; this will mean > swnode_graph_find_next_port() and > software_node_graph_get_next_endpoint() in patch 4 of this series will > need changing slightly to square away their references. What about ACPI case? Does it square?
On Wed, Oct 21, 2020 at 12:33:21PM +0300, Andy Shevchenko wrote: > On Wed, Oct 21, 2020 at 12:25:28AM +0100, Dan Scally wrote: > > On 20/10/2020 14:31, Sakari Ailus wrote: > > > On Mon, Oct 19, 2020 at 11:58:57PM +0100, Daniel Scally wrote: > > > >> + return software_node_get(&c->fwnode); > > > I believe similarly, the function should drop the reference to the previous > > > node, and not expect the caller to do this. The OF equivalent does the > > > same. > > > > I think I prefer it this way myself, since the alternative is having to > > explicitly call *_node_get() on a returned child if you want to keep it > > but also keep iterating. But I agree that it's important to take a > > consistent approach. I'll add that too; this will mean > > swnode_graph_find_next_port() and > > software_node_graph_get_next_endpoint() in patch 4 of this series will > > need changing slightly to square away their references. > > What about ACPI case? Does it square? In ACPI, we seem to assume these nodes are always there and thus don't need reference counting. -- Sakari Ailus
On Tue, Oct 20, 2020 at 11:52:56PM +0100, Dan Scally wrote: > On 20/10/2020 11:05, Sakari Ailus wrote: > > On Mon, Oct 19, 2020 at 11:58:55PM +0100, Daniel Scally wrote: > >> Software nodes that are children of another software node should be > >> unregistered before their parent. To allow easy unregistering of an array > >> of software_nodes ordered parent to child, add a helper function to loop > >> over and unregister nodes in such an array in reverse order. ... > >> + * software_node_unregister_nodes_reverse - Unregister an array of software > >> + * nodes in reverse order. > >> + * @nodes: Array of software nodes to be unregistered. > >> + * > >> + * NOTE: The same warning applies as with software_node_unregister_nodes. > >> + * Unless you are _sure_ that the array of nodes is ordered parent to child > >> + * it is wiser to remove them individually in the correct order. > > Could the default order in software_node_unregister_nodes() be reversed > > instead? There are no users so this should be easy to change. > > > > Doing this only one way may require enforcing the registration order in > > software_node_register_nodes(), but the end result would be safer. > > > > What do you think? > > Yeah fine by me. We can just use software_node_to_swnode(node->parent) > within software_node_unregister_nodes() to check that children come > after their parents have already been processed. I'll add a patch to do > that in the next version of this series, and another changing the > ordering of software_node_unregister_node_group() as Andy suggests for > consistency. I remember it was a big discussion between Rafael, Heikki and Greg KH about child-parent release in kobjects. That ended up with few patches to device object handling along with one that reversed the order of swnode unregistering in test_printf.c. So here is the question who is maintaining the order: a kref (via kobject) or a caller? -- With Best Regards, Andy Shevchenko
On 21/10/2020 10:40, Andy Shevchenko wrote: > On Tue, Oct 20, 2020 at 11:52:56PM +0100, Dan Scally wrote: >> On 20/10/2020 11:05, Sakari Ailus wrote: >>> On Mon, Oct 19, 2020 at 11:58:55PM +0100, Daniel Scally wrote: >>>> Software nodes that are children of another software node should be >>>> unregistered before their parent. To allow easy unregistering of an array >>>> of software_nodes ordered parent to child, add a helper function to loop >>>> over and unregister nodes in such an array in reverse order. > ... > >>>> + * software_node_unregister_nodes_reverse - Unregister an array of software >>>> + * nodes in reverse order. >>>> + * @nodes: Array of software nodes to be unregistered. >>>> + * >>>> + * NOTE: The same warning applies as with software_node_unregister_nodes. >>>> + * Unless you are _sure_ that the array of nodes is ordered parent to child >>>> + * it is wiser to remove them individually in the correct order. >>> Could the default order in software_node_unregister_nodes() be reversed >>> instead? There are no users so this should be easy to change. >>> >>> Doing this only one way may require enforcing the registration order in >>> software_node_register_nodes(), but the end result would be safer. >>> >>> What do you think? >> Yeah fine by me. We can just use software_node_to_swnode(node->parent) >> within software_node_unregister_nodes() to check that children come >> after their parents have already been processed. I'll add a patch to do >> that in the next version of this series, and another changing the >> ordering of software_node_unregister_node_group() as Andy suggests for >> consistency. > I remember it was a big discussion between Rafael, Heikki and Greg KH about > child-parent release in kobjects. That ended up with few patches to device > object handling along with one that reversed the order of swnode unregistering > in test_printf.c. So here is the question who is maintaining the order: a kref > (via kobject) or a caller? I would expect the caller to maintain the order correctly, and just have the register() function validate that the ordering is good and complain if not.
On 21/10/2020 10:33, Andy Shevchenko wrote: > On Wed, Oct 21, 2020 at 12:25:28AM +0100, Dan Scally wrote: >> On 20/10/2020 14:31, Sakari Ailus wrote: >>> On Mon, Oct 19, 2020 at 11:58:57PM +0100, Daniel Scally wrote: >>>> + return software_node_get(&c->fwnode); >>> I believe similarly, the function should drop the reference to the previous >>> node, and not expect the caller to do this. The OF equivalent does the >>> same. >> I think I prefer it this way myself, since the alternative is having to >> explicitly call *_node_get() on a returned child if you want to keep it >> but also keep iterating. But I agree that it's important to take a >> consistent approach. I'll add that too; this will mean >> swnode_graph_find_next_port() and >> software_node_graph_get_next_endpoint() in patch 4 of this series will >> need changing slightly to square away their references. > What about ACPI case? Does it square? ACPI version doesn't handle references at all; neither puts() the old nor gets() the new child node.
On 20/10/2020 14:38, Rafael J. Wysocki wrote: > [Fix the Linus Walleij's address.] Thanks - much appreciated > On Tue, Oct 20, 2020 at 12:59 AM Daniel Scally <djrscally@gmail.com> wrote: >> Hello all >> >> This series adds support to the ipu3-cio2 driver for fwnode connections >> between cio2 and sensors to be defined via software_nodes. The final patch >> in the series deals wholly with those changes - the preceding patches are >> either supporting changes to accommodate that or incidental fixes along >> the way: >> >> 1/9 adds a function to drivers/base/swnode.c unwinding arrays of software >> nodes in reverse order >> >> 2/9 uses that function in lib/test_printf.c >> >> 3/9 fixes what seems to me to be a bug in the existing swnode.c code in >> that software_node_get_next_child() does not increase the refcount of the >> returned node (in contrast to, for example, of_get_next_child_node() which >> does increase the count) >> >> 4/9 adds the fwnode_graph*() family of functions to the software_node >> implementation >> >> 5/9 adds a T: entry to MAINTAINERS for the ipu3-cio2 driver >> >> 6/9 renames the ipu3-cio2.c file to ipu3-cio2-main.c and fixes Makefile >> to accommodate that change >> >> 7/9 alters the ipu3-cio2 driver to check if the pci_dev's fwnode is a >> software_node and pass flags to fwnode_graph_get_endpoint_by_id() if so >> >> 8/9 alters match_fwnode() in v4l2-async.c to additionally try to match on >> a fwnode_handle's secondary if the primary doesn't match >> >> 9/9 alters the ipu3-cio2 driver to do the actual building of software_node >> connections between the sensor devices and the cio2 device. >> >> This is still not ready for integration - hence the RFC label - as there >> is ongoing work to extend the ipu3-cio2 driver further to parse ACPI >> to discover resources such as regulators and GPIOs that are defined there >> in unusual ways and map them to the sensor devices so that their drivers >> can consume them transparently through the usual frameworks. Given this >> has changed quite extensively from v2 though, I wanted to submit it for >> feedback at this point in case it needs further large scale change. > I would appreciate it if you posted the next version of this series > (all patches) to linux-acpi@vger.kernel.org for easier review. > > Thanks! Sure thing, I'll make sure to add that list to next version
Hi Daniel, Thank you for the patch. On Mon, Oct 19, 2020 at 11:58:59PM +0100, Daniel Scally wrote: > Development for the ipu3-cio2 driver is taking place in media_tree, but > there's no T: entry in MAINTAINERS to denote that - rectify that oversight > > Signed-off-by: Daniel Scally <djrscally@gmail.com> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > Changes in v3: > - patch introduced. > > MAINTAINERS | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/MAINTAINERS b/MAINTAINERS > index 43a025039..5d768d5ad 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -8851,6 +8851,7 @@ M: Bingbu Cao <bingbu.cao@intel.com> > R: Tianshu Qiu <tian.shu.qiu@intel.com> > L: linux-media@vger.kernel.org > S: Maintained > +T: git git://linuxtv.org/media_tree.git > F: Documentation/userspace-api/media/v4l/pixfmt-srggb10-ipu3.rst > F: drivers/media/pci/intel/ipu3/ -- Regards, Laurent Pinchart
Hi Daniel, Thank you for the patch. On Mon, Oct 19, 2020 at 11:59:00PM +0100, Daniel Scally wrote: > ipu3-cio2 driver needs extending with multiple files; rename the main > source file and specify the renamed file in Makefile to accommodate that. > > Signed-off-by: Daniel Scally <djrscally@gmail.com> > --- > Changes in v3: > - patch introduced > > drivers/media/pci/intel/ipu3/Makefile | 2 ++ > drivers/media/pci/intel/ipu3/{ipu3-cio2.c => ipu3-cio2-main.c} | 0 > 2 files changed, 2 insertions(+) > rename drivers/media/pci/intel/ipu3/{ipu3-cio2.c => ipu3-cio2-main.c} (100%) > > diff --git a/drivers/media/pci/intel/ipu3/Makefile b/drivers/media/pci/intel/ipu3/Makefile > index 98ddd5bea..b4e3266d9 100644 > --- a/drivers/media/pci/intel/ipu3/Makefile > +++ b/drivers/media/pci/intel/ipu3/Makefile > @@ -1,2 +1,4 @@ > # SPDX-License-Identifier: GPL-2.0-only > obj-$(CONFIG_VIDEO_IPU3_CIO2) += ipu3-cio2.o > + > +ipu3-cio2-y += ipu3-cio2-main.o > \ No newline at end of file I would have sworn the usual naming for this kind of case was -drv.c, but it seems -main.c is more common (I've probably been mistaken by focussing quite a bit on drivers/gpu/drm/ in the past few years). -core.c wins over both though :-) Anyway, enough bikeshedding, with the newline fixed, Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c b/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c > similarity index 100% > rename from drivers/media/pci/intel/ipu3/ipu3-cio2.c > rename to drivers/media/pci/intel/ipu3/ipu3-cio2-main.c
Hi Sakari On Wed, Oct 21, 2020 at 01:49:10AM +0300, Sakari Ailus wrote: > On Tue, Oct 20, 2020 at 08:56:07PM +0100, Dan Scally wrote: > > On 20/10/2020 13:06, Sakari Ailus wrote: > > > On Tue, Oct 20, 2020 at 12:19:58PM +0300, Andy Shevchenko wrote: > > >> On Mon, Oct 19, 2020 at 11:59:01PM +0100, Daniel Scally wrote: > > >>> fwnode_graph_get_endpoint_by_id() will optionally parse enabled devices > > >>> only; that status being determined through the .device_is_available() op > > >>> of the device's fwnode. As software_nodes don't have that operation and > > >>> adding it is meaningless, we instead need to check if the device's fwnode > > >>> is a software_node and if so pass the appropriate flag to disable that > > >>> check > > >> Period. > > >> > > >> I'm wondering if actually this can be hidden in fwnode_graph_get_endpoint_by_id(). > > > The device availability test is actually there for a reason. Some firmware > > > implementations put all the potential devices in the tables and only one > > > (of some) of them are available. > > > > > > Could this be implemented so that if the node is a software node, then get > > > its parent and then see if that is available? > > > > > > I guess that could be implemented in software node ops. Any opinions? > > Actually when considering the cio2 device, it seems that > > set_secondary_fwnode() actually overwrites the _primary_, given > > fwnode_is_primary(dev->fwnode) returns false. So in at least some cases, > > this wouldn't work. > > Ouch. I wonder when this happens --- have you checked what's the primary > there? I guess it might be if it's a PCI device without the corresponding > ACPI device node? > > I remember you had an is_available implementation that just returned true > for software nodes in an early version of the set? I think it would still > be a lesser bad in this case. How about the following ? diff --git a/drivers/base/property.c b/drivers/base/property.c index 81bd01ed4042..ea44ba846299 100644 --- a/drivers/base/property.c +++ b/drivers/base/property.c @@ -706,9 +706,14 @@ EXPORT_SYMBOL_GPL(fwnode_handle_put); /** * fwnode_device_is_available - check if a device is available for use * @fwnode: Pointer to the fwnode of the device. + * + * For fwnode node types that don't implement the .device_is_available() + * operation, such as software nodes, this function returns true. */ bool fwnode_device_is_available(const struct fwnode_handle *fwnode) { + if (!fwnode_has_op(fwnode, device_is_available)) + return true; return fwnode_call_bool_op(fwnode, device_is_available); } EXPORT_SYMBOL_GPL(fwnode_device_is_available); -- Regards, Laurent Pinchart
On Sat, Oct 24, 2020 at 03:39:55AM +0300, Laurent Pinchart wrote: > Hi Sakari > > On Wed, Oct 21, 2020 at 01:49:10AM +0300, Sakari Ailus wrote: > > On Tue, Oct 20, 2020 at 08:56:07PM +0100, Dan Scally wrote: > > > On 20/10/2020 13:06, Sakari Ailus wrote: > > > > On Tue, Oct 20, 2020 at 12:19:58PM +0300, Andy Shevchenko wrote: > > > >> On Mon, Oct 19, 2020 at 11:59:01PM +0100, Daniel Scally wrote: > > > >>> fwnode_graph_get_endpoint_by_id() will optionally parse enabled devices > > > >>> only; that status being determined through the .device_is_available() op > > > >>> of the device's fwnode. As software_nodes don't have that operation and > > > >>> adding it is meaningless, we instead need to check if the device's fwnode > > > >>> is a software_node and if so pass the appropriate flag to disable that > > > >>> check > > > >> Period. > > > >> > > > >> I'm wondering if actually this can be hidden in fwnode_graph_get_endpoint_by_id(). > > > > The device availability test is actually there for a reason. Some firmware > > > > implementations put all the potential devices in the tables and only one > > > > (of some) of them are available. > > > > > > > > Could this be implemented so that if the node is a software node, then get > > > > its parent and then see if that is available? > > > > > > > > I guess that could be implemented in software node ops. Any opinions? > > > Actually when considering the cio2 device, it seems that > > > set_secondary_fwnode() actually overwrites the _primary_, given > > > fwnode_is_primary(dev->fwnode) returns false. So in at least some cases, > > > this wouldn't work. > > > > Ouch. I wonder when this happens --- have you checked what's the primary > > there? I guess it might be if it's a PCI device without the corresponding > > ACPI device node? > > > > I remember you had an is_available implementation that just returned true > > for software nodes in an early version of the set? I think it would still > > be a lesser bad in this case. > > How about the following ? Looks good to me. > > diff --git a/drivers/base/property.c b/drivers/base/property.c > index 81bd01ed4042..ea44ba846299 100644 > --- a/drivers/base/property.c > +++ b/drivers/base/property.c > @@ -706,9 +706,14 @@ EXPORT_SYMBOL_GPL(fwnode_handle_put); > /** > * fwnode_device_is_available - check if a device is available for use > * @fwnode: Pointer to the fwnode of the device. > + * > + * For fwnode node types that don't implement the .device_is_available() > + * operation, such as software nodes, this function returns true. > */ > bool fwnode_device_is_available(const struct fwnode_handle *fwnode) > { > + if (!fwnode_has_op(fwnode, device_is_available)) > + return true; > return fwnode_call_bool_op(fwnode, device_is_available); > } > EXPORT_SYMBOL_GPL(fwnode_device_is_available); >
On 24/10/2020 15:29, Sakari Ailus wrote: > On Sat, Oct 24, 2020 at 03:39:55AM +0300, Laurent Pinchart wrote: >> Hi Sakari >> >> On Wed, Oct 21, 2020 at 01:49:10AM +0300, Sakari Ailus wrote: >>> On Tue, Oct 20, 2020 at 08:56:07PM +0100, Dan Scally wrote: >>>> On 20/10/2020 13:06, Sakari Ailus wrote: >>>>> On Tue, Oct 20, 2020 at 12:19:58PM +0300, Andy Shevchenko wrote: >>>>>> On Mon, Oct 19, 2020 at 11:59:01PM +0100, Daniel Scally wrote: >>>>>>> fwnode_graph_get_endpoint_by_id() will optionally parse enabled devices >>>>>>> only; that status being determined through the .device_is_available() op >>>>>>> of the device's fwnode. As software_nodes don't have that operation and >>>>>>> adding it is meaningless, we instead need to check if the device's fwnode >>>>>>> is a software_node and if so pass the appropriate flag to disable that >>>>>>> check >>>>>> Period. >>>>>> >>>>>> I'm wondering if actually this can be hidden in fwnode_graph_get_endpoint_by_id(). >>>>> The device availability test is actually there for a reason. Some firmware >>>>> implementations put all the potential devices in the tables and only one >>>>> (of some) of them are available. >>>>> >>>>> Could this be implemented so that if the node is a software node, then get >>>>> its parent and then see if that is available? >>>>> >>>>> I guess that could be implemented in software node ops. Any opinions? >>>> Actually when considering the cio2 device, it seems that >>>> set_secondary_fwnode() actually overwrites the _primary_, given >>>> fwnode_is_primary(dev->fwnode) returns false. So in at least some cases, >>>> this wouldn't work. >>> Ouch. I wonder when this happens --- have you checked what's the primary >>> there? I guess it might be if it's a PCI device without the corresponding >>> ACPI device node? >>> >>> I remember you had an is_available implementation that just returned true >>> for software nodes in an early version of the set? I think it would still >>> be a lesser bad in this case. >> How about the following ? > Looks good to me. If we're agreed on this (and it's fine by me too), do you want me to include it in the next set, or are you going to do it separately Laurent? >> diff --git a/drivers/base/property.c b/drivers/base/property.c >> index 81bd01ed4042..ea44ba846299 100644 >> --- a/drivers/base/property.c >> +++ b/drivers/base/property.c >> @@ -706,9 +706,14 @@ EXPORT_SYMBOL_GPL(fwnode_handle_put); >> /** >> * fwnode_device_is_available - check if a device is available for use >> * @fwnode: Pointer to the fwnode of the device. >> + * >> + * For fwnode node types that don't implement the .device_is_available() >> + * operation, such as software nodes, this function returns true. >> */ >> bool fwnode_device_is_available(const struct fwnode_handle *fwnode) >> { >> + if (!fwnode_has_op(fwnode, device_is_available)) >> + return true; >> return fwnode_call_bool_op(fwnode, device_is_available); >> } >> EXPORT_SYMBOL_GPL(fwnode_device_is_available); >>
Hi Dan, On Sat, Oct 24, 2020 at 05:33:32PM +0100, Dan Scally wrote: > On 24/10/2020 15:29, Sakari Ailus wrote: > > On Sat, Oct 24, 2020 at 03:39:55AM +0300, Laurent Pinchart wrote: > >> On Wed, Oct 21, 2020 at 01:49:10AM +0300, Sakari Ailus wrote: > >>> On Tue, Oct 20, 2020 at 08:56:07PM +0100, Dan Scally wrote: > >>>> On 20/10/2020 13:06, Sakari Ailus wrote: > >>>>> On Tue, Oct 20, 2020 at 12:19:58PM +0300, Andy Shevchenko wrote: > >>>>>> On Mon, Oct 19, 2020 at 11:59:01PM +0100, Daniel Scally wrote: > >>>>>>> fwnode_graph_get_endpoint_by_id() will optionally parse enabled devices > >>>>>>> only; that status being determined through the .device_is_available() op > >>>>>>> of the device's fwnode. As software_nodes don't have that operation and > >>>>>>> adding it is meaningless, we instead need to check if the device's fwnode > >>>>>>> is a software_node and if so pass the appropriate flag to disable that > >>>>>>> check > >>>>>> > >>>>>> Period. > >>>>>> > >>>>>> I'm wondering if actually this can be hidden in fwnode_graph_get_endpoint_by_id(). > >>>>> > >>>>> The device availability test is actually there for a reason. Some firmware > >>>>> implementations put all the potential devices in the tables and only one > >>>>> (of some) of them are available. > >>>>> > >>>>> Could this be implemented so that if the node is a software node, then get > >>>>> its parent and then see if that is available? > >>>>> > >>>>> I guess that could be implemented in software node ops. Any opinions? > >>>> > >>>> Actually when considering the cio2 device, it seems that > >>>> set_secondary_fwnode() actually overwrites the _primary_, given > >>>> fwnode_is_primary(dev->fwnode) returns false. So in at least some cases, > >>>> this wouldn't work. > >>> > >>> Ouch. I wonder when this happens --- have you checked what's the primary > >>> there? I guess it might be if it's a PCI device without the corresponding > >>> ACPI device node? > >>> > >>> I remember you had an is_available implementation that just returned true > >>> for software nodes in an early version of the set? I think it would still > >>> be a lesser bad in this case. > >> > >> How about the following ? > > > > Looks good to me. > > If we're agreed on this (and it's fine by me too), do you want me to > include it in the next set, or are you going to do it separately Laurent? Feel free to include it in the next version, but I can send a patch if you prefer. > >> diff --git a/drivers/base/property.c b/drivers/base/property.c > >> index 81bd01ed4042..ea44ba846299 100644 > >> --- a/drivers/base/property.c > >> +++ b/drivers/base/property.c > >> @@ -706,9 +706,14 @@ EXPORT_SYMBOL_GPL(fwnode_handle_put); > >> /** > >> * fwnode_device_is_available - check if a device is available for use > >> * @fwnode: Pointer to the fwnode of the device. > >> + * > >> + * For fwnode node types that don't implement the .device_is_available() > >> + * operation, such as software nodes, this function returns true. > >> */ > >> bool fwnode_device_is_available(const struct fwnode_handle *fwnode) > >> { > >> + if (!fwnode_has_op(fwnode, device_is_available)) > >> + return true; > >> return fwnode_call_bool_op(fwnode, device_is_available); > >> } > >> EXPORT_SYMBOL_GPL(fwnode_device_is_available);
Hello all This series adds support to the ipu3-cio2 driver for fwnode connections between cio2 and sensors to be defined via software_nodes. The final patch in the series deals wholly with those changes - the preceding patches are either supporting changes to accommodate that or incidental fixes along the way: 1/9 adds a function to drivers/base/swnode.c unwinding arrays of software nodes in reverse order 2/9 uses that function in lib/test_printf.c 3/9 fixes what seems to me to be a bug in the existing swnode.c code in that software_node_get_next_child() does not increase the refcount of the returned node (in contrast to, for example, of_get_next_child_node() which does increase the count) 4/9 adds the fwnode_graph*() family of functions to the software_node implementation 5/9 adds a T: entry to MAINTAINERS for the ipu3-cio2 driver 6/9 renames the ipu3-cio2.c file to ipu3-cio2-main.c and fixes Makefile to accommodate that change 7/9 alters the ipu3-cio2 driver to check if the pci_dev's fwnode is a software_node and pass flags to fwnode_graph_get_endpoint_by_id() if so 8/9 alters match_fwnode() in v4l2-async.c to additionally try to match on a fwnode_handle's secondary if the primary doesn't match 9/9 alters the ipu3-cio2 driver to do the actual building of software_node connections between the sensor devices and the cio2 device. This is still not ready for integration - hence the RFC label - as there is ongoing work to extend the ipu3-cio2 driver further to parse ACPI to discover resources such as regulators and GPIOs that are defined there in unusual ways and map them to the sensor devices so that their drivers can consume them transparently through the usual frameworks. Given this has changed quite extensively from v2 though, I wanted to submit it for feedback at this point in case it needs further large scale change. Thanks Dan Daniel Scally (8): software_node: Add helper function to unregister arrays of software_nodes ordered parent to child lib/test_printf.c: Use helper function to unwind array of software_nodes software_node: Fix failure to hold refcount in software_node_get_next_child ipu3-cio2: Add T: entry to MAINTAINERS ipu3-cio2: Rename ipu3-cio2.c to allow module to be built from multiple sources files retaining ipu3-cio2 name ipu3-cio2: Check if pci_dev->dev's fwnode is a software_node in cio2_parse_firmware() and set FWNODE_GRAPH_DEVICE_DISABLED if so media: v4l2-core: v4l2-async: Check possible match in match_fwnode based on sd->fwnode->secondary ipu3-cio2: Add functionality allowing software_node connections to sensors on platforms designed for Windows Heikki Krogerus (1): software_node: Add support for fwnode_graph*() family of functions MAINTAINERS | 2 + drivers/base/swnode.c | 143 +++++++- drivers/media/pci/intel/ipu3/Kconfig | 18 + drivers/media/pci/intel/ipu3/Makefile | 3 + drivers/media/pci/intel/ipu3/cio2-bridge.c | 327 ++++++++++++++++++ drivers/media/pci/intel/ipu3/cio2-bridge.h | 94 +++++ .../ipu3/{ipu3-cio2.c => ipu3-cio2-main.c} | 30 +- drivers/media/pci/intel/ipu3/ipu3-cio2.h | 9 + drivers/media/v4l2-core/v4l2-async.c | 8 + include/linux/property.h | 1 + lib/test_printf.c | 4 +- 11 files changed, 632 insertions(+), 7 deletions(-) create mode 100644 drivers/media/pci/intel/ipu3/cio2-bridge.c create mode 100644 drivers/media/pci/intel/ipu3/cio2-bridge.h rename drivers/media/pci/intel/ipu3/{ipu3-cio2.c => ipu3-cio2-main.c} (98%)