Hello all
v2:
https://lore.kernel.org/linux-media/[email protected]/T/#md93fd090009b42a6a98aed892aff0d38cf07e0cd
v1:
https://lore.kernel.org/linux-media/[email protected]/T/#m91934e12e3d033da2e768e952ea3b4a125ee3e67
The RFC version before that:
https://lore.kernel.org/linux-media/[email protected]/
This series is to start adding support for webcams on laptops with ACPI tables
designed for use with CIO2 on Windows. This problem has two main parts; the
first part, which is handled in this series, is extending the ipu3-cio2
driver to allow for patching the firmware via software_nodes if endpoints
aren't defined by ACPI. The second is adding a new driver to handle power,
clocks and GPIO pins defined in DSDT tables in an awkward way. I decided to
split that second part out from this series, and instead give it its own
series (a v2 of which should land "soon"). The reasons for that are:
1. It's a logically separate change anyway
2. The recipients list was getting really long and
3. That probably meant that handling merge for all of this in one go was
going to be impractically awkward.
I'm hopeful that most or all of this series could get picked up for 5.12.
We touch a few different areas (listed below), but I think the easiest
approach would be to merge everything through media tree. Rafael, Greg,
Mauro and Sergey; are you ok with that plan, or would you prefer a
different approach? Mauro; if that plan is ok (and of course assuming that
the rest of the patches are acked by their respective maintainers) could
we get a dedicated feature branch just in case the following series ends
up being ready in time too?
lib (with an ack already)
lib/test_printf.c: Use helper function to unwind array of
software_nodes
base
software_node: Fix refcounts in software_node_get_next_child()
property: Return true in fwnode_device_is_available for NULL ops
property: Call fwnode_graph_get_endpoint_by_id() for fwnode->secondary
software_node: Enforce parent before child ordering of nodes arrays
software_node: unregister software_nodes in reverse order
include: fwnode.h: Define format macros for ports and endpoints
acpi
acpi: Add acpi_dev_get_next_match_dev() and helper macro
media
media: v4l2-core: v4l2-async: Check sd->fwnode->secondary in
match_fwnode()
ipu3-cio2: Add T: entry to MAINTAINERS
ipu3-cio2: Rename ipu3-cio2.c
ipu3-cio2: Add cio2-bridge to ipu3-cio2 driver
include: media: v4l2-fwnode: Include v4l2_fwnode_bus_type
Series-level changelog:
- Added patch 13/14 to define an enum in media/v4l2-fwnode.h
- Added patch 06/14 to define name formats for fwnode graph
ports and endpoints
More details of changes on each patch.
Merry Christmas everyone!
Dan
Daniel Scally (13):
software_node: Fix refcounts in software_node_get_next_child()
property: Return true in fwnode_device_is_available for NULL ops
property: Call fwnode_graph_get_endpoint_by_id() for fwnode->secondary
software_node: Enforce parent before child ordering of nodes arrays
software_node: unregister software_nodes in reverse order
include: fwnode.h: Define format macros for ports and endpoints
lib/test_printf.c: Use helper function to unwind array of
software_nodes
ipu3-cio2: Add T: entry to MAINTAINERS
ipu3-cio2: Rename ipu3-cio2.c
media: v4l2-core: v4l2-async: Check sd->fwnode->secondary in
match_fwnode()
acpi: Add acpi_dev_get_next_match_dev() and helper macro
include: media: v4l2-fwnode: Include v4l2_fwnode_bus_type
ipu3-cio2: Add cio2-bridge to ipu3-cio2 driver
Heikki Krogerus (1):
software_node: Add support for fwnode_graph*() family of functions
MAINTAINERS | 2 +
drivers/acpi/utils.c | 30 +-
drivers/base/property.c | 15 +-
drivers/base/swnode.c | 174 +++++++++--
drivers/media/pci/intel/ipu3/Kconfig | 18 ++
drivers/media/pci/intel/ipu3/Makefile | 3 +
drivers/media/pci/intel/ipu3/cio2-bridge.c | 272 ++++++++++++++++++
drivers/media/pci/intel/ipu3/cio2-bridge.h | 122 ++++++++
.../ipu3/{ipu3-cio2.c => ipu3-cio2-main.c} | 34 +++
drivers/media/pci/intel/ipu3/ipu3-cio2.h | 6 +
drivers/media/v4l2-core/v4l2-async.c | 8 +
drivers/media/v4l2-core/v4l2-fwnode.c | 11 -
include/acpi/acpi_bus.h | 7 +
include/linux/fwnode.h | 13 +
include/media/v4l2-fwnode.h | 22 ++
lib/test_printf.c | 4 +-
16 files changed, 704 insertions(+), 37 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.25.1
Use the software_node_unregister_nodes() helper function to unwind this
array in a cleaner way.
Acked-by: Petr Mladek <[email protected]>
Reviewed-by: Andy Shevchenko <[email protected]>
Reviewed-by: Laurent Pinchart <[email protected]>
Reviewed-by: Sergey Senozhatsky <[email protected]>
Suggested-by: Andy Shevchenko <[email protected]>
Signed-off-by: Daniel Scally <[email protected]>
---
Changes in v3
- None
lib/test_printf.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/lib/test_printf.c b/lib/test_printf.c
index 7ac87f18a10f..7d60f24240a4 100644
--- a/lib/test_printf.c
+++ b/lib/test_printf.c
@@ -644,9 +644,7 @@ static void __init fwnode_pointer(void)
test(second_name, "%pfwP", software_node_fwnode(&softnodes[1]));
test(third_name, "%pfwP", software_node_fwnode(&softnodes[2]));
- software_node_unregister(&softnodes[2]);
- software_node_unregister(&softnodes[1]);
- software_node_unregister(&softnodes[0]);
+ software_node_unregister_nodes(softnodes);
}
static void __init
--
2.25.1
Where the fwnode graph is comprised of software_nodes, these will be
assigned as the secondary to dev->fwnode. Check the v4l2_subdev's fwnode
for a secondary and attempt to match against it during match_fwnode() to
accommodate that possibility.
Reviewed-by: Andy Shevchenko <[email protected]>
Reviewed-by: Laurent Pinchart <[email protected]>
Signed-off-by: Daniel Scally <[email protected]>
---
Changes in v3
- None
drivers/media/v4l2-core/v4l2-async.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
index e3ab003a6c85..9dd896d085ec 100644
--- a/drivers/media/v4l2-core/v4l2-async.c
+++ b/drivers/media/v4l2-core/v4l2-async.c
@@ -87,6 +87,14 @@ static bool match_fwnode(struct v4l2_async_notifier *notifier,
if (sd->fwnode == asd->match.fwnode)
return true;
+ /*
+ * Check the same situation for any possible secondary assigned to the
+ * subdev's fwnode
+ */
+ if (!IS_ERR_OR_NULL(sd->fwnode->secondary) &&
+ sd->fwnode->secondary == asd->match.fwnode)
+ return true;
+
/*
* Otherwise, check if the sd fwnode and the asd fwnode refer to an
* endpoint or a device. If they're of the same type, there's no match.
--
2.25.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
Reviewed-by: Laurent Pinchart <[email protected]>
Reviewed-by: Andy Shevchenko <[email protected]>
Signed-off-by: Daniel Scally <[email protected]>
---
Changes in v3
- None
MAINTAINERS | 1 +
1 file changed, 1 insertion(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index 80881fb36404..16b544624577 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -8946,6 +8946,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.25.1
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: Andy Shevchenko <[email protected]>
Reviewed-by: Laurent Pinchart <[email protected]>
Reviewed-by: Andy Shevchenko <[email protected]>
Signed-off-by: Daniel Scally <[email protected]>
---
Changes in v3
- None
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 98ddd5beafe0..429d516452e4 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
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.25.1
From: Heikki Krogerus <[email protected]>
This implements the remaining .graph_* callbacks in the
fwnode operations structure 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 "port@0", "port@1",
...) 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
- Changed software_node_get_next_endpoint() to drop the variable
named "old"
- Used the macros defined in 06/14 instead of magic numbers
- Added some comments to explain behaviour a little where it's unclear
drivers/base/swnode.c | 112 +++++++++++++++++++++++++++++++++++++++++-
1 file changed, 111 insertions(+), 1 deletion(-)
diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
index 2d07eb04c6c8..ff690029060d 100644
--- a/drivers/base/swnode.c
+++ b/drivers/base/swnode.c
@@ -540,6 +540,112 @@ 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))) {
+ /*
+ * ports have naming style "port@n", so we search for children
+ * that follow that convention (but without assuming anything
+ * about the index number)
+ */
+ if (!strncmp(to_swnode(port)->node->name, "port@",
+ FWNODE_GRAPH_PORT_NAME_PREFIX_LEN))
+ return 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 *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)) {
+ endpoint = software_node_get_next_child(port, endpoint);
+ if (endpoint) {
+ fwnode_handle_put(port);
+ break;
+ }
+ }
+
+ 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);
+
+ swnode = swnode->parent;
+ if (swnode && !strcmp(swnode->node->name, "ports"))
+ swnode = swnode->parent;
+
+ return swnode ? software_node_get(&swnode->fwnode) : NULL;
+}
+
+static int
+software_node_graph_parse_endpoint(const struct fwnode_handle *fwnode,
+ struct fwnode_endpoint *endpoint)
+{
+ struct swnode *swnode = to_swnode(fwnode);
+ int ret;
+
+ /* Ports have naming style "port@n", we need to select the n */
+ ret = kstrtou32(swnode->parent->node->name + FWNODE_GRAPH_PORT_NAME_PREFIX_LEN,
+ 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,
@@ -551,7 +657,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.25.1
OF, ACPI and software_nodes all implement graphs including nodes for ports
and endpoints. These are all intended to be named with a common schema,
as "port@n" and "endpoint@n" where n is an unsigned int representing the
index of the node. To ensure commonality across the subsystems, provide a
set of macros to define the format.
Suggested-by: Andy Shevchenko <[email protected]>
Signed-off-by: Daniel Scally <[email protected]>
---
Changes in v3
- Patch introduced
include/linux/fwnode.h | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/include/linux/fwnode.h b/include/linux/fwnode.h
index 9506f8ec0974..52889efceb7d 100644
--- a/include/linux/fwnode.h
+++ b/include/linux/fwnode.h
@@ -32,6 +32,19 @@ struct fwnode_endpoint {
const struct fwnode_handle *local_fwnode;
};
+/*
+ * ports and endpoints defined in OF, ACPI and as software_nodes should all
+ * follow a common naming scheme; use these macros to ensure commonality across
+ * the subsystems.
+ *
+ * The *PREFIX_LEN macros refer to the length of the "port@" and "endpoint@"
+ * sections of the naming scheme.
+ */
+#define FWNODE_GRAPH_PORT_NAME_FORMAT "port@%u"
+#define FWNODE_GRAPH_PORT_NAME_PREFIX_LEN 5
+#define FWNODE_GRAPH_ENDPOINT_NAME_FORMAT "endpoint@%u"
+#define FWNODE_GRAPH_ENDPOINT_PREFIX_LEN 9
+
#define NR_FWNODE_REFERENCE_ARGS 8
/**
--
2.25.1
To maintain consistency with software_node_unregister_nodes(), reverse
the order in which the software_node_unregister_node_group() function
unregisters nodes.
Reported-by: kernel test robot <[email protected]>
Reported-by: Dan Carpenter <[email protected]>
Reviewed-by: Laurent Pinchart <[email protected]>
Suggested-by: Andy Shevchenko <[email protected]>
Signed-off-by: Daniel Scally <[email protected]>
---
Changes in v3
- Fixed the dereference of the terminating NULL entry
- Comment cleanup
drivers/base/swnode.c | 12 +++++++++---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
index ade49173ff8d..2d07eb04c6c8 100644
--- a/drivers/base/swnode.c
+++ b/drivers/base/swnode.c
@@ -779,16 +779,22 @@ EXPORT_SYMBOL_GPL(software_node_register_node_group);
* software_node_unregister_node_group - Unregister a group of software nodes
* @node_group: NULL terminated array of software node pointers to be unregistered
*
- * Unregister multiple software nodes at once.
+ * Unregister multiple software nodes at once. The array will be unwound in
+ * reverse order (i.e. last entry first) and thus if any member of the array
+ * has its .parent member set then they should appear later in the array such
+ * that they are unregistered first.
*/
void software_node_unregister_node_group(const struct software_node **node_group)
{
- unsigned int i;
+ unsigned int i = 0;
if (!node_group)
return;
- for (i = 0; node_group[i]; i++)
+ while (node_group[i])
+ i++;
+
+ while (i--)
software_node_unregister(node_group[i]);
}
EXPORT_SYMBOL_GPL(software_node_unregister_node_group);
--
2.25.1
On Thu, Dec 24, 2020 at 3:12 AM Daniel Scally <[email protected]> wrote:
>
> To maintain consistency with software_node_unregister_nodes(), reverse
> the order in which the software_node_unregister_node_group() function
> unregisters nodes.
...
> - * Unregister multiple software nodes at once.
> + * Unregister multiple software nodes at once. The array will be unwound in
> + * reverse order (i.e. last entry first) and thus if any member of the array
> + * has its .parent member set then they should appear later in the array such
> + * that they are unregistered first.
I'm, as being not a native speaker, a bit confused by this comment.
The idea is that children are unregistered first. Can you try to make
it more clear maybe?
> */
--
With Best Regards,
Andy Shevchenko
On Thu, Dec 24, 2020 at 3:12 AM Daniel Scally <[email protected]> wrote:
>
> OF, ACPI and software_nodes all implement graphs including nodes for ports
> and endpoints. These are all intended to be named with a common schema,
> as "port@n" and "endpoint@n" where n is an unsigned int representing the
> index of the node. To ensure commonality across the subsystems, provide a
> set of macros to define the format.
Nitpicks below, but in general that's what I meant, thanks!
Reviewed-by: Andy Shevchenko <[email protected]>
(after addressing nitpicks)
> Suggested-by: Andy Shevchenko <[email protected]>
> Signed-off-by: Daniel Scally <[email protected]>
> ---
> Changes in v3
> - Patch introduced
>
> include/linux/fwnode.h | 13 +++++++++++++
> 1 file changed, 13 insertions(+)
>
> diff --git a/include/linux/fwnode.h b/include/linux/fwnode.h
> index 9506f8ec0974..52889efceb7d 100644
> --- a/include/linux/fwnode.h
> +++ b/include/linux/fwnode.h
> @@ -32,6 +32,19 @@ struct fwnode_endpoint {
> const struct fwnode_handle *local_fwnode;
> };
>
> +/*
> + * ports and endpoints defined in OF, ACPI and as software_nodes should all
> + * follow a common naming scheme; use these macros to ensure commonality across
> + * the subsystems.
> + *
> + * The *PREFIX_LEN macros refer to the length of the "port@" and "endpoint@"
*PREFIX_LEN -> *_PREFIX_LEN
> + * sections of the naming scheme.
> + */
> +#define FWNODE_GRAPH_PORT_NAME_FORMAT "port@%u"
> +#define FWNODE_GRAPH_PORT_NAME_PREFIX_LEN 5
> +#define FWNODE_GRAPH_ENDPOINT_NAME_FORMAT "endpoint@%u"
> +#define FWNODE_GRAPH_ENDPOINT_PREFIX_LEN 9
_FORMAT -> _FMT (however, V4L2 guys may correct me, because IIRC _FMT
suffix is also used for other things in v4l2.
> #define NR_FWNODE_REFERENCE_ARGS 8
>
> /**
--
With Best Regards,
Andy Shevchenko
On Thu, Dec 24, 2020 at 3:14 AM Daniel Scally <[email protected]> wrote:
>
> From: Heikki Krogerus <[email protected]>
>
> This implements the remaining .graph_* callbacks in the
.graph_* ==> ->graph_*() ?
> fwnode operations structure for the software nodes. That makes
> the fwnode_graph*() functions available in the drivers also
graph*() -> graph_*() ?
> 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 "port@0", "port@1",
> ...)
Looks not good, perhaps move to the previous line, or move port@1 example here?
> 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".
Few nitpicks here and there, after addressing them,
Reviewed-by: Andy Shevchenko <[email protected]>
> Signed-off-by: Heikki Krogerus <[email protected]>
> Co-developed-by: Daniel Scally <[email protected]>
> Signed-off-by: Daniel Scally <[email protected]>
> ---
> Changes in v3
> - Changed software_node_get_next_endpoint() to drop the variable
> named "old"
> - Used the macros defined in 06/14 instead of magic numbers
> - Added some comments to explain behaviour a little where it's unclear
>
> drivers/base/swnode.c | 112 +++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 111 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
> index 2d07eb04c6c8..ff690029060d 100644
> --- a/drivers/base/swnode.c
> +++ b/drivers/base/swnode.c
> @@ -540,6 +540,112 @@ 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))) {
> + /*
> + * ports have naming style "port@n", so we search for children
> + * that follow that convention (but without assuming anything
> + * about the index number)
> + */
> + if (!strncmp(to_swnode(port)->node->name, "port@",
You may use here corresponding _FMT macro.
> + FWNODE_GRAPH_PORT_NAME_PREFIX_LEN))
> + return 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 *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)) {
> + endpoint = software_node_get_next_child(port, endpoint);
> + if (endpoint) {
> + fwnode_handle_put(port);
> + break;
> + }
> + }
> +
> + 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);
> +
> + swnode = swnode->parent;
> + if (swnode && !strcmp(swnode->node->name, "ports"))
> + swnode = swnode->parent;
> +
> + return swnode ? software_node_get(&swnode->fwnode) : NULL;
> +}
> +
> +static int
> +software_node_graph_parse_endpoint(const struct fwnode_handle *fwnode,
> + struct fwnode_endpoint *endpoint)
> +{
> + struct swnode *swnode = to_swnode(fwnode);
> + int ret;
> +
> + /* Ports have naming style "port@n", we need to select the n */
> + ret = kstrtou32(swnode->parent->node->name + FWNODE_GRAPH_PORT_NAME_PREFIX_LEN,
Maybe a temporary variable?
unsigned int prefix_len = FWNODE_GRAPH_PORT_NAME_PREFIX_LEN;
...
ret = kstrtou32(swnode->parent->node->name + prefix_len,
> + 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,
> @@ -551,7 +657,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.25.1
>
--
With Best Regards,
Andy Shevchenko
Hi Daniel,
Thank you for the patch.
On Thu, Dec 24, 2020 at 02:17:07PM +0200, Andy Shevchenko wrote:
> On Thu, Dec 24, 2020 at 3:12 AM Daniel Scally wrote:
> >
> > OF, ACPI and software_nodes all implement graphs including nodes for ports
> > and endpoints. These are all intended to be named with a common schema,
> > as "port@n" and "endpoint@n" where n is an unsigned int representing the
> > index of the node. To ensure commonality across the subsystems, provide a
> > set of macros to define the format.
>
> Nitpicks below, but in general that's what I meant, thanks!
>
> Reviewed-by: Andy Shevchenko <[email protected]>
> (after addressing nitpicks)
>
> > Suggested-by: Andy Shevchenko <[email protected]>
> > Signed-off-by: Daniel Scally <[email protected]>
> > ---
> > Changes in v3
> > - Patch introduced
> >
> > include/linux/fwnode.h | 13 +++++++++++++
> > 1 file changed, 13 insertions(+)
> >
> > diff --git a/include/linux/fwnode.h b/include/linux/fwnode.h
> > index 9506f8ec0974..52889efceb7d 100644
> > --- a/include/linux/fwnode.h
> > +++ b/include/linux/fwnode.h
> > @@ -32,6 +32,19 @@ struct fwnode_endpoint {
> > const struct fwnode_handle *local_fwnode;
> > };
> >
> > +/*
> > + * ports and endpoints defined in OF, ACPI and as software_nodes should all
> > + * follow a common naming scheme; use these macros to ensure commonality across
> > + * the subsystems.
> > + *
> > + * The *PREFIX_LEN macros refer to the length of the "port@" and "endpoint@"
>
> *PREFIX_LEN -> *_PREFIX_LEN
>
> > + * sections of the naming scheme.
> > + */
> > +#define FWNODE_GRAPH_PORT_NAME_FORMAT "port@%u"
> > +#define FWNODE_GRAPH_PORT_NAME_PREFIX_LEN 5
> > +#define FWNODE_GRAPH_ENDPOINT_NAME_FORMAT "endpoint@%u"
> > +#define FWNODE_GRAPH_ENDPOINT_PREFIX_LEN 9
>
> _FORMAT -> _FMT (however, V4L2 guys may correct me, because IIRC _FMT
> suffix is also used for other things in v4l2.
This isn't related to V4L2, so it doesn't matter much :-) I personally
prefer spelling names out in full when that wouldn't result in too long
lines, but it's really a matter of personal preference, I don't mind
either way.
Reviewed-by: Laurent Pinchart <[email protected]>
> > #define NR_FWNODE_REFERENCE_ARGS 8
> >
> > /**
--
Regards,
Laurent Pinchart
Hi Daniel,
Thank you for the patch.
On Thu, Dec 24, 2020 at 01:09:00AM +0000, Daniel Scally wrote:
> From: Heikki Krogerus <[email protected]>
>
> This implements the remaining .graph_* callbacks in the
> fwnode operations structure 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 "port@0", "port@1",
> ...) 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
> - Changed software_node_get_next_endpoint() to drop the variable
> named "old"
> - Used the macros defined in 06/14 instead of magic numbers
> - Added some comments to explain behaviour a little where it's unclear
>
> drivers/base/swnode.c | 112 +++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 111 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
> index 2d07eb04c6c8..ff690029060d 100644
> --- a/drivers/base/swnode.c
> +++ b/drivers/base/swnode.c
> @@ -540,6 +540,112 @@ 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))) {
> + /*
> + * ports have naming style "port@n", so we search for children
> + * that follow that convention (but without assuming anything
> + * about the index number)
> + */
> + if (!strncmp(to_swnode(port)->node->name, "port@",
> + FWNODE_GRAPH_PORT_NAME_PREFIX_LEN))
I would either add a macro to replace the prefix ("port@"), or drop
FWNODE_GRAPH_PORT_NAME_PREFIX_LEN. I think this is the worst of both
worlds, the string and its length are defined in two different places
:-)
I would personally drop the macro, but I don't mind either way as long
as the string and its length are defined in the same place.
> + return 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 *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)) {
> + endpoint = software_node_get_next_child(port, endpoint);
> + if (endpoint) {
> + fwnode_handle_put(port);
> + break;
> + }
> + }
> +
> + 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);
> +
> + swnode = swnode->parent;
> + if (swnode && !strcmp(swnode->node->name, "ports"))
> + swnode = swnode->parent;
> +
> + return swnode ? software_node_get(&swnode->fwnode) : NULL;
> +}
> +
> +static int
> +software_node_graph_parse_endpoint(const struct fwnode_handle *fwnode,
> + struct fwnode_endpoint *endpoint)
> +{
> + struct swnode *swnode = to_swnode(fwnode);
> + int ret;
> +
> + /* Ports have naming style "port@n", we need to select the n */
> + ret = kstrtou32(swnode->parent->node->name + FWNODE_GRAPH_PORT_NAME_PREFIX_LEN,
> + 10, &endpoint->port);
Same here.
I wonder if we should add a check to ensure parent->node->name is long
enough (and possibly even start with the right prefix), as otherwise the
pointer passed to kstrtou32() may be past the end of the string. Maybe
this is overkill, if we can rely on the fact that software nodes have
correct names.
Reviewed-by: Laurent Pinchart <[email protected]>
> + 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,
> @@ -551,7 +657,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,
> };
>
> /* -------------------------------------------------------------------------- */
--
Regards,
Laurent Pinchart
Hi Andy,
On Thu, Dec 24, 2020 at 02:24:12PM +0200, Andy Shevchenko wrote:
> On Thu, Dec 24, 2020 at 3:14 AM Daniel Scally wrote:
> >
> > From: Heikki Krogerus <[email protected]>
> >
> > This implements the remaining .graph_* callbacks in the
>
> .graph_* ==> ->graph_*() ?
>
> > fwnode operations structure for the software nodes. That makes
> > the fwnode_graph*() functions available in the drivers also
>
> graph*() -> graph_*() ?
>
> > 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 "port@0", "port@1",
>
> > ...)
>
> Looks not good, perhaps move to the previous line, or move port@1 example here?
>
> > 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".
>
> Few nitpicks here and there, after addressing them,
> Reviewed-by: Andy Shevchenko <[email protected]>
>
> > Signed-off-by: Heikki Krogerus <[email protected]>
> > Co-developed-by: Daniel Scally <[email protected]>
> > Signed-off-by: Daniel Scally <[email protected]>
> > ---
> > Changes in v3
> > - Changed software_node_get_next_endpoint() to drop the variable
> > named "old"
> > - Used the macros defined in 06/14 instead of magic numbers
> > - Added some comments to explain behaviour a little where it's unclear
> >
> > drivers/base/swnode.c | 112 +++++++++++++++++++++++++++++++++++++++++-
> > 1 file changed, 111 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
> > index 2d07eb04c6c8..ff690029060d 100644
> > --- a/drivers/base/swnode.c
> > +++ b/drivers/base/swnode.c
> > @@ -540,6 +540,112 @@ 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))) {
> > + /*
> > + * ports have naming style "port@n", so we search for children
> > + * that follow that convention (but without assuming anything
> > + * about the index number)
> > + */
>
> > + if (!strncmp(to_swnode(port)->node->name, "port@",
>
> You may use here corresponding _FMT macro.
>
> > + FWNODE_GRAPH_PORT_NAME_PREFIX_LEN))
> > + return 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 *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)) {
> > + endpoint = software_node_get_next_child(port, endpoint);
> > + if (endpoint) {
> > + fwnode_handle_put(port);
> > + break;
> > + }
> > + }
> > +
> > + 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);
> > +
> > + swnode = swnode->parent;
> > + if (swnode && !strcmp(swnode->node->name, "ports"))
> > + swnode = swnode->parent;
> > +
> > + return swnode ? software_node_get(&swnode->fwnode) : NULL;
> > +}
> > +
> > +static int
> > +software_node_graph_parse_endpoint(const struct fwnode_handle *fwnode,
> > + struct fwnode_endpoint *endpoint)
> > +{
> > + struct swnode *swnode = to_swnode(fwnode);
> > + int ret;
> > +
> > + /* Ports have naming style "port@n", we need to select the n */
>
> > + ret = kstrtou32(swnode->parent->node->name + FWNODE_GRAPH_PORT_NAME_PREFIX_LEN,
>
> Maybe a temporary variable?
>
> unsigned int prefix_len = FWNODE_GRAPH_PORT_NAME_PREFIX_LEN;
> ...
> ret = kstrtou32(swnode->parent->node->name + prefix_len,
Honestly I'm wondering if those macros don't hinder readability. I'd
rather write
+ strlen("port@")
and let the compiler optimize this to a compile-time constant.
> > + 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,
> > @@ -551,7 +657,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,
> > };
> >
> > /* -------------------------------------------------------------------------- */
--
Regards,
Laurent Pinchart
On Thu, Dec 24, 2020 at 2:55 PM Laurent Pinchart
<[email protected]> wrote:
> On Thu, Dec 24, 2020 at 02:24:12PM +0200, Andy Shevchenko wrote:
> > On Thu, Dec 24, 2020 at 3:14 AM Daniel Scally wrote:
...
> > > + if (!strncmp(to_swnode(port)->node->name, "port@",
> >
> > You may use here corresponding _FMT macro.
> >
> > > + FWNODE_GRAPH_PORT_NAME_PREFIX_LEN))
> > > + return port;
...
> > > + /* Ports have naming style "port@n", we need to select the n */
> >
> > > + ret = kstrtou32(swnode->parent->node->name + FWNODE_GRAPH_PORT_NAME_PREFIX_LEN,
> >
> > Maybe a temporary variable?
> >
> > unsigned int prefix_len = FWNODE_GRAPH_PORT_NAME_PREFIX_LEN;
> > ...
> > ret = kstrtou32(swnode->parent->node->name + prefix_len,
>
> Honestly I'm wondering if those macros don't hinder readability. I'd
> rather write
>
> + strlen("port@")
Works for me, since the compiler optimizes this away to be a plain constant.
> and let the compiler optimize this to a compile-time constant.
>
> > > + 10, &endpoint->port);
> > > + if (ret)
> > > + return ret;
--
With Best Regards,
Andy Shevchenko
On 24/12/2020 12:13, Andy Shevchenko wrote:
> On Thu, Dec 24, 2020 at 3:12 AM Daniel Scally <[email protected]> wrote:
>>
>> To maintain consistency with software_node_unregister_nodes(), reverse
>> the order in which the software_node_unregister_node_group() function
>> unregisters nodes.
>
> ...
>
>> - * Unregister multiple software nodes at once.
>> + * Unregister multiple software nodes at once. The array will be unwound in
>> + * reverse order (i.e. last entry first) and thus if any member of the array
>> + * has its .parent member set then they should appear later in the array such
>> + * that they are unregistered first.
>
> I'm, as being not a native speaker, a bit confused by this comment.
> The idea is that children are unregistered first. Can you try to make
> it more clear maybe?
Sure, how about:
The array will be unwound in reverse order (i.e. last entry first). If
any member of the array is a child of another member then the child must
appear later in the array than their parent, so that they are
unregistered first.
?
On Thu, Dec 24, 2020 at 4:00 PM Daniel Scally <[email protected]> wrote:
> On 24/12/2020 12:13, Andy Shevchenko wrote:
> > On Thu, Dec 24, 2020 at 3:12 AM Daniel Scally <[email protected]> wrote:
...
> >> + * Unregister multiple software nodes at once. The array will be unwound in
> >> + * reverse order (i.e. last entry first) and thus if any member of the array
> >> + * has its .parent member set then they should appear later in the array such
> >> + * that they are unregistered first.
> >
> > I'm, as being not a native speaker, a bit confused by this comment.
> > The idea is that children are unregistered first. Can you try to make
> > it more clear maybe?
>
> Sure, how about:
>
> The array will be unwound in reverse order (i.e. last entry first). If
> any member of the array is a child of another member then the child must
children ?
> appear later in the array than their parent, so that they are
> unregistered first.
I think with the above change it will be better, yes.
--
With Best Regards,
Andy Shevchenko
On 24/12/2020 14:12, Andy Shevchenko wrote:
> On Thu, Dec 24, 2020 at 4:00 PM Daniel Scally <[email protected]> wrote:
>> On 24/12/2020 12:13, Andy Shevchenko wrote:
>>> On Thu, Dec 24, 2020 at 3:12 AM Daniel Scally <[email protected]> wrote:
> ...
>
>>>> + * Unregister multiple software nodes at once. The array will be unwound in
>>>> + * reverse order (i.e. last entry first) and thus if any member of the array
>>>> + * has its .parent member set then they should appear later in the array such
>>>> + * that they are unregistered first.
>>> I'm, as being not a native speaker, a bit confused by this comment.
>>> The idea is that children are unregistered first. Can you try to make
>>> it more clear maybe?
>> Sure, how about:
>>
>> The array will be unwound in reverse order (i.e. last entry first). If
>> any member of the array is a child of another member then the child must
> children ?
Yes, you are right of course.
>
>> appear later in the array than their parent, so that they are
>> unregistered first.
> I think with the above change it will be better, yes.
>
Ok, done.
Hi Andy, Laurent
> On Thu, Dec 24, 2020 at 2:55 PM Laurent Pinchart
> <[email protected]> wrote:
>> On Thu, Dec 24, 2020 at 02:24:12PM +0200, Andy Shevchenko wrote:
>>> On Thu, Dec 24, 2020 at 3:14 AM Daniel Scally wrote:
>
> ...
>
>>>> + if (!strncmp(to_swnode(port)->node->name, "port@",
>>>
>>> You may use here corresponding _FMT macro.
>>>
>>>> + FWNODE_GRAPH_PORT_NAME_PREFIX_LEN))
>>>> + return port;
>
> ...
>
>>>> + /* Ports have naming style "port@n", we need to select the n */
>>>
>>>> + ret = kstrtou32(swnode->parent->node->name + FWNODE_GRAPH_PORT_NAME_PREFIX_LEN,
>>>
>>> Maybe a temporary variable?
>>>
>>> unsigned int prefix_len = FWNODE_GRAPH_PORT_NAME_PREFIX_LEN;
>>> ...
>>> ret = kstrtou32(swnode->parent->node->name + prefix_len,
>>
>> Honestly I'm wondering if those macros don't hinder readability. I'd
>> rather write
>>
>> + strlen("port@")
>
> Works for me, since the compiler optimizes this away to be a plain constant.
Well, how about instead of the LEN macro, we have:
#define FWNODE_GRAPH_PORT_NAME_PREFIX "port@"
#define FWNODE_GRAPH_PORT_NAME_FMT FWNODE_GRAPH_PORT_NAME_PREFIX "%u"
And then it's still maintainable in one place but (I think) slightly
less clunky, since we can do strlen(FWNODE_GRAPH_PORT_NAME_PREFIX)
Or we can do strlen("port@"), I'm good either way :)
On 24/12/2020 12:53, Laurent Pinchart wrote:
>> + while ((port = software_node_get_next_child(parent, old))) {
>> + /*
>> + * ports have naming style "port@n", so we search for children
>> + * that follow that convention (but without assuming anything
>> + * about the index number)
>> + */
>> + if (!strncmp(to_swnode(port)->node->name, "port@",
>> + FWNODE_GRAPH_PORT_NAME_PREFIX_LEN))
>
> I would either add a macro to replace the prefix ("port@"), or drop
> FWNODE_GRAPH_PORT_NAME_PREFIX_LEN. I think this is the worst of both
> worlds, the string and its length are defined in two different places
> :-)
>
> I would personally drop the macro, but I don't mind either way as long
> as the string and its length are defined in the same place.
OK, pending outcome of the discussion in the other thread I'll do both
things the same way - whatever the decision there is.
>> +static int
>> +software_node_graph_parse_endpoint(const struct fwnode_handle *fwnode,
>> + struct fwnode_endpoint *endpoint)
>> +{
>> + struct swnode *swnode = to_swnode(fwnode);
>> + int ret;
>> +
>> + /* Ports have naming style "port@n", we need to select the n */
>> + ret = kstrtou32(swnode->parent->node->name + FWNODE_GRAPH_PORT_NAME_PREFIX_LEN,
>> + 10, &endpoint->port);
>
> Same here.
>
> I wonder if we should add a check to ensure parent->node->name is long
> enough (and possibly even start with the right prefix), as otherwise the
> pointer passed to kstrtou32() may be past the end of the string. Maybe
> this is overkill, if we can rely on the fact that software nodes have
> correct names.
Not necessarily actually; ports yes but endpoints no, so I think the
danger is there. I'll add the check.
> Reviewed-by: Laurent Pinchart <[email protected]>
Thanks!
From: Daniel Scally
> Sent: 24 December 2020 14:14
...
> >> The array will be unwound in reverse order (i.e. last entry first). If
> >> any member of the array is a child of another member then the child must
> > children ?
>
> Yes, you are right of course.
The second 'child' is a back-reference to 'any member' so is singular
so 'child' is correct.
'the child' could be replaced by 'it'
You could have:
If any members of the array are children of another member then the
children must appear later in the list.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
Hi Andy
On 24/12/2020 12:24, Andy Shevchenko wrote:
> On Thu, Dec 24, 2020 at 3:14 AM Daniel Scally <[email protected]> wrote:
>>
>> From: Heikki Krogerus <[email protected]>
>>
>> This implements the remaining .graph_* callbacks in the
>
> .graph_* ==> ->graph_*() ?
>
>> fwnode operations structure for the software nodes. That makes
>> the fwnode_graph*() functions available in the drivers also
>
> graph*() -> graph_*() ?
Both done.
>> 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 "port@0", "port@1",
>
>> ...)
>
> Looks not good, perhaps move to the previous line, or move port@1 example here?
Fixed - by expanding the lines to ~75 chars
> Few nitpicks here and there, after addressing them,
> Reviewed-by: Andy Shevchenko <[email protected]>
Thank you!
>> +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))) {
>> + /*
>> + * ports have naming style "port@n", so we search for children
>> + * that follow that convention (but without assuming anything
>> + * about the index number)
>> + */
>
>> + if (!strncmp(to_swnode(port)->node->name, "port@",
>
> You may use here corresponding _FMT macro.
>> +static int
>> +software_node_graph_parse_endpoint(const struct fwnode_handle *fwnode,
>> + struct fwnode_endpoint *endpoint)
>> +{
>> + struct swnode *swnode = to_swnode(fwnode);
>> + int ret;
>> +
>> + /* Ports have naming style "port@n", we need to select the n */
>
>> + ret = kstrtou32(swnode->parent->node->name + FWNODE_GRAPH_PORT_NAME_PREFIX_LEN,
>
> Maybe a temporary variable?
>
> unsigned int prefix_len = FWNODE_GRAPH_PORT_NAME_PREFIX_LEN;
> ...
> ret = kstrtou32(swnode->parent->node->name + prefix_len,
Both discussed after Laurent's reply I think.
On Thu, Dec 24, 2020 at 06:36:10PM +0000, David Laight wrote:
> From: Daniel Scally
> > Sent: 24 December 2020 14:14
> ...
> > >> The array will be unwound in reverse order (i.e. last entry first). If
> > >> any member of the array is a child of another member then the child must
> > > children ?
> >
> > Yes, you are right of course.
>
> The second 'child' is a back-reference to 'any member' so is singular
> so 'child' is correct.
> 'the child' could be replaced by 'it'
>
> You could have:
> If any members of the array are children of another member then the
> children must appear later in the list.
Works for me!
Dan, can you consider David's proposal?
--
With Best Regards,
Andy Shevchenko
Hi Daniel, Andy,
On Thu, Dec 24, 2020 at 01:08:59AM +0000, Daniel Scally wrote:
> OF, ACPI and software_nodes all implement graphs including nodes for ports
> and endpoints. These are all intended to be named with a common schema,
> as "port@n" and "endpoint@n" where n is an unsigned int representing the
> index of the node. To ensure commonality across the subsystems, provide a
> set of macros to define the format.
>
> Suggested-by: Andy Shevchenko <[email protected]>
> Signed-off-by: Daniel Scally <[email protected]>
> ---
> Changes in v3
> - Patch introduced
>
> include/linux/fwnode.h | 13 +++++++++++++
> 1 file changed, 13 insertions(+)
>
> diff --git a/include/linux/fwnode.h b/include/linux/fwnode.h
> index 9506f8ec0974..52889efceb7d 100644
> --- a/include/linux/fwnode.h
> +++ b/include/linux/fwnode.h
> @@ -32,6 +32,19 @@ struct fwnode_endpoint {
> const struct fwnode_handle *local_fwnode;
> };
>
> +/*
> + * ports and endpoints defined in OF, ACPI and as software_nodes should all
> + * follow a common naming scheme; use these macros to ensure commonality across
> + * the subsystems.
> + *
> + * The *PREFIX_LEN macros refer to the length of the "port@" and "endpoint@"
> + * sections of the naming scheme.
> + */
> +#define FWNODE_GRAPH_PORT_NAME_FORMAT "port@%u"
> +#define FWNODE_GRAPH_PORT_NAME_PREFIX_LEN 5
> +#define FWNODE_GRAPH_ENDPOINT_NAME_FORMAT "endpoint@%u"
> +#define FWNODE_GRAPH_ENDPOINT_PREFIX_LEN 9
> +
> #define NR_FWNODE_REFERENCE_ARGS 8
I'd keep such definitions local to the swnode implementation as neither
ACPI nor DT have an apparent need for them. They do use the naming, but
don't appear to format such strings.
--
Regards,
Sakari Ailus
Hi Daniel,
On Thu, Dec 24, 2020 at 01:08:58AM +0000, Daniel Scally wrote:
> To maintain consistency with software_node_unregister_nodes(), reverse
> the order in which the software_node_unregister_node_group() function
> unregisters nodes.
>
> Reported-by: kernel test robot <[email protected]>
> Reported-by: Dan Carpenter <[email protected]>
> Reviewed-by: Laurent Pinchart <[email protected]>
> Suggested-by: Andy Shevchenko <[email protected]>
> Signed-off-by: Daniel Scally <[email protected]>
> ---
> Changes in v3
> - Fixed the dereference of the terminating NULL entry
> - Comment cleanup
>
> drivers/base/swnode.c | 12 +++++++++---
> 1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
> index ade49173ff8d..2d07eb04c6c8 100644
> --- a/drivers/base/swnode.c
> +++ b/drivers/base/swnode.c
> @@ -779,16 +779,22 @@ EXPORT_SYMBOL_GPL(software_node_register_node_group);
> * software_node_unregister_node_group - Unregister a group of software nodes
> * @node_group: NULL terminated array of software node pointers to be unregistered
> *
> - * Unregister multiple software nodes at once.
> + * Unregister multiple software nodes at once. The array will be unwound in
> + * reverse order (i.e. last entry first) and thus if any member of the array
> + * has its .parent member set then they should appear later in the array such
> + * that they are unregistered first.
> */
> void software_node_unregister_node_group(const struct software_node **node_group)
With this line wrapped,
Reviewed-by: Sakari Ailus <[email protected]>
> {
> - unsigned int i;
> + unsigned int i = 0;
>
> if (!node_group)
> return;
>
> - for (i = 0; node_group[i]; i++)
> + while (node_group[i])
> + i++;
> +
> + while (i--)
> software_node_unregister(node_group[i]);
> }
> EXPORT_SYMBOL_GPL(software_node_unregister_node_group);
--
Kind regards,
Sakari Ailus
Hi Daniel,
On Thu, Dec 24, 2020 at 02:21:15PM +0000, Daniel Scally wrote:
> Hi Andy, Laurent
>
> > On Thu, Dec 24, 2020 at 2:55 PM Laurent Pinchart
> > <[email protected]> wrote:
> >> On Thu, Dec 24, 2020 at 02:24:12PM +0200, Andy Shevchenko wrote:
> >>> On Thu, Dec 24, 2020 at 3:14 AM Daniel Scally wrote:
> >
> > ...
> >
> >>>> + if (!strncmp(to_swnode(port)->node->name, "port@",
> >>>
> >>> You may use here corresponding _FMT macro.
> >>>
> >>>> + FWNODE_GRAPH_PORT_NAME_PREFIX_LEN))
> >>>> + return port;
> >
> > ...
> >
> >>>> + /* Ports have naming style "port@n", we need to select the n */
> >>>
> >>>> + ret = kstrtou32(swnode->parent->node->name + FWNODE_GRAPH_PORT_NAME_PREFIX_LEN,
> >>>
> >>> Maybe a temporary variable?
> >>>
> >>> unsigned int prefix_len = FWNODE_GRAPH_PORT_NAME_PREFIX_LEN;
> >>> ...
> >>> ret = kstrtou32(swnode->parent->node->name + prefix_len,
> >>
> >> Honestly I'm wondering if those macros don't hinder readability. I'd
> >> rather write
> >>
> >> + strlen("port@")
> >
> > Works for me, since the compiler optimizes this away to be a plain constant.
>
> Well, how about instead of the LEN macro, we have:
>
> #define FWNODE_GRAPH_PORT_NAME_PREFIX "port@"
> #define FWNODE_GRAPH_PORT_NAME_FMT FWNODE_GRAPH_PORT_NAME_PREFIX "%u"
>
> And then it's still maintainable in one place but (I think) slightly
> less clunky, since we can do strlen(FWNODE_GRAPH_PORT_NAME_PREFIX)
>
> Or we can do strlen("port@"), I'm good either way :)
I'd be in favour of using strlen("port@") here.
At least for now. I think refactoring the use of such strings could be a
separate set at another time, if that's seen as the way to go.
--
Kind regards,
Sakari Ailus
On Mon, Dec 28, 2020 at 06:30:24PM +0200, Sakari Ailus wrote:
> Hi Daniel, Andy,
>
> On Thu, Dec 24, 2020 at 01:08:59AM +0000, Daniel Scally wrote:
> > OF, ACPI and software_nodes all implement graphs including nodes for ports
> > and endpoints. These are all intended to be named with a common schema,
> > as "port@n" and "endpoint@n" where n is an unsigned int representing the
> > index of the node. To ensure commonality across the subsystems, provide a
> > set of macros to define the format.
> >
> > Suggested-by: Andy Shevchenko <[email protected]>
> > Signed-off-by: Daniel Scally <[email protected]>
> > ---
> > Changes in v3
> > - Patch introduced
> >
> > include/linux/fwnode.h | 13 +++++++++++++
> > 1 file changed, 13 insertions(+)
> >
> > diff --git a/include/linux/fwnode.h b/include/linux/fwnode.h
> > index 9506f8ec0974..52889efceb7d 100644
> > --- a/include/linux/fwnode.h
> > +++ b/include/linux/fwnode.h
> > @@ -32,6 +32,19 @@ struct fwnode_endpoint {
> > const struct fwnode_handle *local_fwnode;
> > };
> >
> > +/*
> > + * ports and endpoints defined in OF, ACPI and as software_nodes should all
> > + * follow a common naming scheme; use these macros to ensure commonality across
> > + * the subsystems.
> > + *
> > + * The *PREFIX_LEN macros refer to the length of the "port@" and "endpoint@"
> > + * sections of the naming scheme.
> > + */
> > +#define FWNODE_GRAPH_PORT_NAME_FORMAT "port@%u"
> > +#define FWNODE_GRAPH_PORT_NAME_PREFIX_LEN 5
> > +#define FWNODE_GRAPH_ENDPOINT_NAME_FORMAT "endpoint@%u"
> > +#define FWNODE_GRAPH_ENDPOINT_PREFIX_LEN 9
> > +
> > #define NR_FWNODE_REFERENCE_ARGS 8
>
> I'd keep such definitions local to the swnode implementation as neither
> ACPI nor DT have an apparent need for them. They do use the naming, but
> don't appear to format such strings.
Ah, I noticed these are used by the later patches. Please ignore that
comment then.
But these are still definitions, so I'd use SWNODE prefix rather than
FWNODE. That should also be reflected in the comment.
ACPI does not have a format port concept so the definitions that are being
used can be said to be specific to Linux.
--
Kind regards,
Sakari Ailus
On Mon, Dec 28, 2020 at 06:34:10PM +0200, Sakari Ailus wrote:
> On Thu, Dec 24, 2020 at 01:08:58AM +0000, Daniel Scally wrote:
...
> > void software_node_unregister_node_group(const struct software_node **node_group)
>
> With this line wrapped,
Why? It's only one character behind 80 and wrapping it will decrease
readability. Moreover, documentation has explicit exceptions for such cases.
--
With Best Regards,
Andy Shevchenko
Hi Sakari
On 28/12/2020 16:41, Sakari Ailus wrote:
> Hi Daniel,
>
> On Thu, Dec 24, 2020 at 02:21:15PM +0000, Daniel Scally wrote:
>> Hi Andy, Laurent
>>
>>> On Thu, Dec 24, 2020 at 2:55 PM Laurent Pinchart
>>> <[email protected]> wrote:
>>>> On Thu, Dec 24, 2020 at 02:24:12PM +0200, Andy Shevchenko wrote:
>>>>> On Thu, Dec 24, 2020 at 3:14 AM Daniel Scally wrote:
>>>
>>> ...
>>>
>>>>>> + if (!strncmp(to_swnode(port)->node->name, "port@",
>>>>>
>>>>> You may use here corresponding _FMT macro.
>>>>>
>>>>>> + FWNODE_GRAPH_PORT_NAME_PREFIX_LEN))
>>>>>> + return port;
>>>
>>> ...
>>>
>>>>>> + /* Ports have naming style "port@n", we need to select the n */
>>>>>
>>>>>> + ret = kstrtou32(swnode->parent->node->name + FWNODE_GRAPH_PORT_NAME_PREFIX_LEN,
>>>>>
>>>>> Maybe a temporary variable?
>>>>>
>>>>> unsigned int prefix_len = FWNODE_GRAPH_PORT_NAME_PREFIX_LEN;
>>>>> ...
>>>>> ret = kstrtou32(swnode->parent->node->name + prefix_len,
>>>>
>>>> Honestly I'm wondering if those macros don't hinder readability. I'd
>>>> rather write
>>>>
>>>> + strlen("port@")
>>>
>>> Works for me, since the compiler optimizes this away to be a plain constant.
>>
>> Well, how about instead of the LEN macro, we have:
>>
>> #define FWNODE_GRAPH_PORT_NAME_PREFIX "port@"
>> #define FWNODE_GRAPH_PORT_NAME_FMT FWNODE_GRAPH_PORT_NAME_PREFIX "%u"
>>
>> And then it's still maintainable in one place but (I think) slightly
>> less clunky, since we can do strlen(FWNODE_GRAPH_PORT_NAME_PREFIX)
>>
>> Or we can do strlen("port@"), I'm good either way :)
>
> I'd be in favour of using strlen("port@") here.
>
> At least for now. I think refactoring the use of such strings could be a
> separate set at another time, if that's seen as the way to go.
Alright - thanks. In that case I'll switch to strlen("port@0"), and
FWNODE_GRAPH_PORT_NAME_LEN can be dropped then I think.
Hi Sakari
On 28/12/2020 17:11, Sakari Ailus wrote:
> On Mon, Dec 28, 2020 at 06:30:24PM +0200, Sakari Ailus wrote:
>> Hi Daniel, Andy,
>>
>> On Thu, Dec 24, 2020 at 01:08:59AM +0000, Daniel Scally wrote:
>>> OF, ACPI and software_nodes all implement graphs including nodes for ports
>>> and endpoints. These are all intended to be named with a common schema,
>>> as "port@n" and "endpoint@n" where n is an unsigned int representing the
>>> index of the node. To ensure commonality across the subsystems, provide a
>>> set of macros to define the format.
>>>
>>> Suggested-by: Andy Shevchenko <[email protected]>
>>> Signed-off-by: Daniel Scally <[email protected]>
>>> ---
>>> Changes in v3
>>> - Patch introduced
>>>
>>> include/linux/fwnode.h | 13 +++++++++++++
>>> 1 file changed, 13 insertions(+)
>>>
>>> diff --git a/include/linux/fwnode.h b/include/linux/fwnode.h
>>> index 9506f8ec0974..52889efceb7d 100644
>>> --- a/include/linux/fwnode.h
>>> +++ b/include/linux/fwnode.h
>>> @@ -32,6 +32,19 @@ struct fwnode_endpoint {
>>> const struct fwnode_handle *local_fwnode;
>>> };
>>>
>>> +/*
>>> + * ports and endpoints defined in OF, ACPI and as software_nodes should all
>>> + * follow a common naming scheme; use these macros to ensure commonality across
>>> + * the subsystems.
>>> + *
>>> + * The *PREFIX_LEN macros refer to the length of the "port@" and "endpoint@"
>>> + * sections of the naming scheme.
>>> + */
>>> +#define FWNODE_GRAPH_PORT_NAME_FORMAT "port@%u"
>>> +#define FWNODE_GRAPH_PORT_NAME_PREFIX_LEN 5
>>> +#define FWNODE_GRAPH_ENDPOINT_NAME_FORMAT "endpoint@%u"
>>> +#define FWNODE_GRAPH_ENDPOINT_PREFIX_LEN 9
>>> +
>>> #define NR_FWNODE_REFERENCE_ARGS 8
>>
>> I'd keep such definitions local to the swnode implementation as neither
>> ACPI nor DT have an apparent need for them. They do use the naming, but
>> don't appear to format such strings.
>
> Ah, I noticed these are used by the later patches. Please ignore that
> comment then.
>
> But these are still definitions, so I'd use SWNODE prefix rather than
> FWNODE. That should also be reflected in the comment.
Fine by me; I shall change that.
> ACPI does not have a format port concept so the definitions that are being
> used can be said to be specific to Linux.
>
On 28/12/2020 10:15, Andy Shevchenko wrote:
> On Thu, Dec 24, 2020 at 06:36:10PM +0000, David Laight wrote:
>> From: Daniel Scally
>>> Sent: 24 December 2020 14:14
>> ...
>>>>> The array will be unwound in reverse order (i.e. last entry first). If
>>>>> any member of the array is a child of another member then the child must
>>>> children ?
>>>
>>> Yes, you are right of course.
>>
>> The second 'child' is a back-reference to 'any member' so is singular
>> so 'child' is correct.
>> 'the child' could be replaced by 'it'
>>
>> You could have:
>> If any members of the array are children of another member then the
>> children must appear later in the list.
>
> Works for me!
> Dan, can you consider David's proposal?
Yep - done, thanks David