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%)
--
2.17.1
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 <[email protected]>
---
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 *
--
2.17.1
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 <[email protected]>
---
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 <[email protected]>
R: Tianshu Qiu <[email protected]>
L: [email protected]
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
From: Heikki Krogerus <[email protected]>
This implements the remaining .graph_* callbacks in the
fwnode operations vector for the software nodes. That makes
the fwnode_graph*() functions available in the drivers also
when software nodes are used.
The implementation tries to mimic the "OF graph" as much as
possible, but there is no support for the "reg" device
property. The ports will need to have the index in their
name which starts with "port" (for example "port0", "port1",
...) and endpoints will use the index of the software node
that is given to them during creation. The port nodes can
also be grouped under a specially named "ports" subnode,
just like in DT, if necessary.
The remote-endpoints are reference properties under the
endpoint nodes that are named "remote-endpoint".
Signed-off-by: Heikki Krogerus <[email protected]>
Co-developed-by: Daniel Scally <[email protected]>
Signed-off-by: Daniel Scally <[email protected]>
---
changes in v3:
- removed software_node_device_is_available
- moved the change to software_node_get_next_child to a separate
patch
- switched to use fwnode_handle_put() in graph_get_next_endpoint()
instead of software_node_put()
changes in v2:
- added software_node_device_is_available
- altered software_node_get_next_child to get references
- altered software_node_get_next_endpoint to release references
to ports and avoid passing invalid combinations of swnodes to
software_node_get_next_child
- altered swnode_graph_find_next_port to release port rather than
old
drivers/base/swnode.c | 120 +++++++++++++++++++++++++++++++++++++++++-
1 file changed, 119 insertions(+), 1 deletion(-)
diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
index 741149b90..3732530ce 100644
--- a/drivers/base/swnode.c
+++ b/drivers/base/swnode.c
@@ -536,6 +536,120 @@ software_node_get_reference_args(const struct fwnode_handle *fwnode,
return 0;
}
+static struct fwnode_handle *
+swnode_graph_find_next_port(const struct fwnode_handle *parent,
+ struct fwnode_handle *port)
+{
+ struct fwnode_handle *old = port;
+
+ while ((port = software_node_get_next_child(parent, old))) {
+ if (!strncmp(to_swnode(port)->node->name, "port", 4))
+ return port;
+ fwnode_handle_put(port);
+ old = port;
+ }
+
+ return NULL;
+}
+
+static struct fwnode_handle *
+software_node_graph_get_next_endpoint(const struct fwnode_handle *fwnode,
+ struct fwnode_handle *endpoint)
+{
+ struct swnode *swnode = to_swnode(fwnode);
+ struct fwnode_handle *old = endpoint;
+ struct fwnode_handle *parent_of_old;
+ struct fwnode_handle *parent;
+ struct fwnode_handle *port;
+
+ if (!swnode)
+ return NULL;
+
+ if (endpoint) {
+ port = software_node_get_parent(endpoint);
+ parent = software_node_get_parent(port);
+ } else {
+ parent = software_node_get_named_child_node(fwnode, "ports");
+ if (!parent)
+ parent = software_node_get(&swnode->fwnode);
+
+ port = swnode_graph_find_next_port(parent, NULL);
+ }
+
+ for (; port; port = swnode_graph_find_next_port(parent, port)) {
+ if (old) {
+ parent_of_old = software_node_get_parent(old);
+
+ if (parent_of_old != port)
+ old = NULL;
+
+ fwnode_handle_put(parent_of_old);
+ }
+
+ endpoint = software_node_get_next_child(port, old);
+ fwnode_handle_put(old);
+ if (endpoint)
+ break;
+
+ fwnode_handle_put(port);
+ }
+
+ fwnode_handle_put(port);
+ fwnode_handle_put(parent);
+
+ return endpoint;
+}
+
+static struct fwnode_handle *
+software_node_graph_get_remote_endpoint(const struct fwnode_handle *fwnode)
+{
+ struct swnode *swnode = to_swnode(fwnode);
+ const struct software_node_ref_args *ref;
+ const struct property_entry *prop;
+
+ if (!swnode)
+ return NULL;
+
+ prop = property_entry_get(swnode->node->properties, "remote-endpoint");
+ if (!prop || prop->type != DEV_PROP_REF || prop->is_inline)
+ return NULL;
+
+ ref = prop->pointer;
+
+ return software_node_get(software_node_fwnode(ref[0].node));
+}
+
+static struct fwnode_handle *
+software_node_graph_get_port_parent(struct fwnode_handle *fwnode)
+{
+ struct swnode *swnode = to_swnode(fwnode);
+ struct fwnode_handle *parent;
+
+ if (!strcmp(swnode->parent->node->name, "ports"))
+ parent = &swnode->parent->parent->fwnode;
+ else
+ parent = &swnode->parent->fwnode;
+
+ return software_node_get(parent);
+}
+
+static int
+software_node_graph_parse_endpoint(const struct fwnode_handle *fwnode,
+ struct fwnode_endpoint *endpoint)
+{
+ struct swnode *swnode = to_swnode(fwnode);
+ int ret;
+
+ ret = kstrtou32(swnode->parent->node->name + 4, 10, &endpoint->port);
+ if (ret)
+ return ret;
+
+ endpoint->id = swnode->id;
+ endpoint->local_fwnode = fwnode;
+
+ return 0;
+}
+
static const struct fwnode_operations software_node_ops = {
.get = software_node_get,
.put = software_node_put,
@@ -547,7 +661,11 @@ static const struct fwnode_operations software_node_ops = {
.get_parent = software_node_get_parent,
.get_next_child_node = software_node_get_next_child,
.get_named_child_node = software_node_get_named_child_node,
- .get_reference_args = software_node_get_reference_args
+ .get_reference_args = software_node_get_reference_args,
+ .graph_get_next_endpoint = software_node_graph_get_next_endpoint,
+ .graph_get_remote_endpoint = software_node_graph_get_remote_endpoint,
+ .graph_get_port_parent = software_node_graph_get_port_parent,
+ .graph_parse_endpoint = software_node_graph_parse_endpoint,
};
/* -------------------------------------------------------------------------- */
--
2.17.1
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 <[email protected]>
Signed-off-by: Daniel Scally <[email protected]>
---
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.
+ */
+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]);
+}
+EXPORT_SYMBOL_GPL(software_node_unregister_nodes_reverse);
+
/**
* software_node_register_node_group - Register a group of software nodes
* @node_group: NULL terminated array of software node pointers to be registered
diff --git a/include/linux/property.h b/include/linux/property.h
index 9f805c442..b6410258b 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -444,6 +444,7 @@ software_node_find_by_name(const struct software_node *parent,
int software_node_register_nodes(const struct software_node *nodes);
void software_node_unregister_nodes(const struct software_node *nodes);
+void software_node_unregister_nodes_reverse(const struct software_node *nodes);
int software_node_register_node_group(const struct software_node **node_group);
void software_node_unregister_node_group(const struct software_node **node_group);
--
2.17.1
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
Signed-off-by: Daniel Scally <[email protected]>
---
Changes in v3:
- patch introduced
drivers/media/pci/intel/ipu3/ipu3-cio2-main.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c b/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c
index 4e598e937..f68ef0f6b 100644
--- a/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c
+++ b/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c
@@ -1466,6 +1466,7 @@ static const struct v4l2_async_notifier_operations cio2_async_ops = {
static int cio2_parse_firmware(struct cio2_device *cio2)
{
+ unsigned long allow_disabled;
unsigned int i;
int ret;
@@ -1474,11 +1475,15 @@ static int cio2_parse_firmware(struct cio2_device *cio2)
.bus_type = V4L2_MBUS_CSI2_DPHY
};
struct sensor_async_subdev *s_asd = NULL;
+ struct fwnode_handle *fwnode;
struct fwnode_handle *ep;
+ fwnode = dev_fwnode(&cio2->pci_dev->dev);
+ allow_disabled = is_software_node(fwnode) ? FWNODE_GRAPH_DEVICE_DISABLED : 0;
+
ep = fwnode_graph_get_endpoint_by_id(
- dev_fwnode(&cio2->pci_dev->dev), i, 0,
- FWNODE_GRAPH_ENDPOINT_NEXT);
+ fwnode, i, 0,
+ FWNODE_GRAPH_ENDPOINT_NEXT | allow_disabled);
if (!ep)
continue;
--
2.17.1
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 <[email protected]>
> Signed-off-by: Daniel Scally <[email protected]>
> ---
> 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 <[email protected]>
> R: Tianshu Qiu <[email protected]>
> L: [email protected]
> 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
>
--
With Best Regards,
Andy Shevchenko
Hi Daniel, Heikki,
Thanks for the patch.
On Mon, Oct 19, 2020 at 11:58:58PM +0100, Daniel Scally wrote:
> From: Heikki Krogerus <[email protected]>
>
> This implements the remaining .graph_* callbacks in the
> fwnode operations vector for the software nodes. That makes
> the fwnode_graph*() functions available in the drivers also
> when software nodes are used.
>
> The implementation tries to mimic the "OF graph" as much as
> possible, but there is no support for the "reg" device
> property. The ports will need to have the index in their
> name which starts with "port" (for example "port0", "port1",
> ...) and endpoints will use the index of the software node
> that is given to them during creation. The port nodes can
> also be grouped under a specially named "ports" subnode,
> just like in DT, if necessary.
>
> The remote-endpoints are reference properties under the
> endpoint nodes that are named "remote-endpoint".
>
> Signed-off-by: Heikki Krogerus <[email protected]>
> Co-developed-by: Daniel Scally <[email protected]>
> Signed-off-by: Daniel Scally <[email protected]>
> ---
> changes in v3:
> - removed software_node_device_is_available
> - moved the change to software_node_get_next_child to a separate
> patch
> - switched to use fwnode_handle_put() in graph_get_next_endpoint()
> instead of software_node_put()
>
> changes in v2:
> - added software_node_device_is_available
> - altered software_node_get_next_child to get references
> - altered software_node_get_next_endpoint to release references
> to ports and avoid passing invalid combinations of swnodes to
> software_node_get_next_child
> - altered swnode_graph_find_next_port to release port rather than
> old
> drivers/base/swnode.c | 120 +++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 119 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
> index 741149b90..3732530ce 100644
> --- a/drivers/base/swnode.c
> +++ b/drivers/base/swnode.c
> @@ -536,6 +536,120 @@ software_node_get_reference_args(const struct fwnode_handle *fwnode,
> return 0;
> }
>
> +static struct fwnode_handle *
> +swnode_graph_find_next_port(const struct fwnode_handle *parent,
> + struct fwnode_handle *port)
> +{
> + struct fwnode_handle *old = port;
> +
> + while ((port = software_node_get_next_child(parent, old))) {
software_node_get_next_child() doesn't drop the reference of the old child
nor gets a reference to the returned node. Should it?
The function to get a named child node does.
It'd be nice if this was aligned. Or am I missing something?
This isn't really a comment on this patch though.
--
Kind regards,
Sakari Ailus
On Tue, Oct 20, 2020 at 03:35:56PM +0300, Sakari Ailus wrote:
> Hi Daniel, Heikki,
>
> Thanks for the patch.
>
> On Mon, Oct 19, 2020 at 11:58:58PM +0100, Daniel Scally wrote:
> > From: Heikki Krogerus <[email protected]>
> >
> > This implements the remaining .graph_* callbacks in the
> > fwnode operations vector for the software nodes. That makes
> > the fwnode_graph*() functions available in the drivers also
> > when software nodes are used.
> >
> > The implementation tries to mimic the "OF graph" as much as
> > possible, but there is no support for the "reg" device
> > property. The ports will need to have the index in their
> > name which starts with "port" (for example "port0", "port1",
> > ...) and endpoints will use the index of the software node
> > that is given to them during creation. The port nodes can
> > also be grouped under a specially named "ports" subnode,
> > just like in DT, if necessary.
> >
> > The remote-endpoints are reference properties under the
> > endpoint nodes that are named "remote-endpoint".
> >
> > Signed-off-by: Heikki Krogerus <[email protected]>
> > Co-developed-by: Daniel Scally <[email protected]>
> > Signed-off-by: Daniel Scally <[email protected]>
> > ---
> > changes in v3:
> > - removed software_node_device_is_available
> > - moved the change to software_node_get_next_child to a separate
> > patch
> > - switched to use fwnode_handle_put() in graph_get_next_endpoint()
> > instead of software_node_put()
> >
> > changes in v2:
> > - added software_node_device_is_available
> > - altered software_node_get_next_child to get references
> > - altered software_node_get_next_endpoint to release references
> > to ports and avoid passing invalid combinations of swnodes to
> > software_node_get_next_child
> > - altered swnode_graph_find_next_port to release port rather than
> > old
> > drivers/base/swnode.c | 120 +++++++++++++++++++++++++++++++++++++++++-
> > 1 file changed, 119 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
> > index 741149b90..3732530ce 100644
> > --- a/drivers/base/swnode.c
> > +++ b/drivers/base/swnode.c
> > @@ -536,6 +536,120 @@ software_node_get_reference_args(const struct fwnode_handle *fwnode,
> > return 0;
> > }
> >
> > +static struct fwnode_handle *
> > +swnode_graph_find_next_port(const struct fwnode_handle *parent,
> > + struct fwnode_handle *port)
> > +{
> > + struct fwnode_handle *old = port;
> > +
> > + while ((port = software_node_get_next_child(parent, old))) {
>
> software_node_get_next_child() doesn't drop the reference of the old child
> nor gets a reference to the returned node. Should it?
>
> The function to get a named child node does.
>
> It'd be nice if this was aligned. Or am I missing something?
>
> This isn't really a comment on this patch though.
I didn't get this patch to my @linux.intel.com account so I guess it's our
mail servers again...
Anyway, please see my comments on that and ignore this one.
--
Sakari Ailus
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 <[email protected]>
> ---
> 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 *
--
Kind regards,
Sakari Ailus
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 <[email protected]>
>> ---
>> 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?
--
With Best Regards,
Andy Shevchenko
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 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.
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 <[email protected]>
Reviewed-by: Laurent Pinchart <[email protected]>
> ---
> 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 <[email protected]>
> R: Tianshu Qiu <[email protected]>
> L: [email protected]
> 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