Hello all
v3:
https://lore.kernel.org/linux-media/[email protected]/T/#m37b831bb2b406917d6db5da9acf9ed35df65d72d
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
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:
- Incorporated Andy's patch fixing the ipu3-cio2 header
More details of changes on each patch.
Thanks
Dan
Andy Shevchenko (1):
media: ipu3-cio2: Add headers that ipu3-cio2.h is direct user of
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 | 181 +++++++++--
drivers/media/pci/intel/ipu3/Kconfig | 18 ++
drivers/media/pci/intel/ipu3/Makefile | 3 +
drivers/media/pci/intel/ipu3/cio2-bridge.c | 302 ++++++++++++++++++
drivers/media/pci/intel/ipu3/cio2-bridge.h | 125 ++++++++
.../ipu3/{ipu3-cio2.c => ipu3-cio2-main.c} | 33 ++
drivers/media/pci/intel/ipu3/ipu3-cio2.h | 24 ++
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 | 7 +
include/media/v4l2-fwnode.h | 22 ++
lib/test_printf.c | 4 +-
16 files changed, 754 insertions(+), 38 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 v4:
- 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
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 v4:
- 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
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]>
Reviewed-by: Andy Shevchenko <[email protected]>
Reviewed-by: Laurent Pinchart <[email protected]>
Signed-off-by: Daniel Scally <[email protected]>
---
Changes in v4:
- FORMAT -> FMT
- Dropped the *_LEN macros, since we settled on using
strlen("port@") instead
include/linux/fwnode.h | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/include/linux/fwnode.h b/include/linux/fwnode.h
index 9506f8ec0974..72d36d46287d 100644
--- a/include/linux/fwnode.h
+++ b/include/linux/fwnode.h
@@ -32,6 +32,13 @@ struct fwnode_endpoint {
const struct fwnode_handle *local_fwnode;
};
+/*
+ * ports and endpoints defined as software_nodes should all follow a common
+ * naming scheme; use these macros to ensure commonality.
+ */
+#define SWNODE_GRAPH_PORT_NAME_FMT "port@%u"
+#define SWNODE_GRAPH_ENDPOINT_NAME_FMT "endpoint@%u"
+
#define NR_FWNODE_REFERENCE_ARGS 8
/**
--
2.25.1
Registering software_nodes with the .parent member set to point to a
currently unregistered software_node has the potential for problems,
so enforce parent -> child ordering in arrays passed in to
software_node_register_nodes().
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, reverse the order in which
software_node_unregister_nodes() unregisters software_nodes.
Suggested-by: Andy Shevchenko <[email protected]>
Reviewed-by: Laurent Pinchart <[email protected]>
Signed-off-by: Daniel Scally <[email protected]>
---
Changes in v4:
- None
drivers/base/swnode.c | 42 ++++++++++++++++++++++++++++++------------
1 file changed, 30 insertions(+), 12 deletions(-)
diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
index 615a0c93e116..ade49173ff8d 100644
--- a/drivers/base/swnode.c
+++ b/drivers/base/swnode.c
@@ -692,7 +692,11 @@ swnode_register(const struct software_node *node, struct swnode *parent,
* software_node_register_nodes - Register an array of software nodes
* @nodes: Zero terminated array of software nodes to be registered
*
- * Register multiple software nodes at once.
+ * Register multiple software nodes at once. If any node in the array
+ * has its .parent pointer set (which can only be to another software_node),
+ * then its parent **must** have been registered before it is; either outside
+ * of this function or by ordering the array such that parent comes before
+ * child.
*/
int software_node_register_nodes(const struct software_node *nodes)
{
@@ -700,14 +704,23 @@ int software_node_register_nodes(const struct software_node *nodes)
int i;
for (i = 0; nodes[i].name; i++) {
- ret = software_node_register(&nodes[i]);
- if (ret) {
- software_node_unregister_nodes(nodes);
- return ret;
+ const struct software_node *parent = nodes[i].parent;
+
+ if (parent && !software_node_to_swnode(parent)) {
+ ret = -EINVAL;
+ goto err_unregister_nodes;
}
+
+ ret = software_node_register(&nodes[i]);
+ if (ret)
+ goto err_unregister_nodes;
}
return 0;
+
+err_unregister_nodes:
+ software_node_unregister_nodes(nodes);
+ return ret;
}
EXPORT_SYMBOL_GPL(software_node_register_nodes);
@@ -715,18 +728,23 @@ EXPORT_SYMBOL_GPL(software_node_register_nodes);
* software_node_unregister_nodes - Unregister an array of software nodes
* @nodes: Zero terminated array of software nodes to be unregistered
*
- * Unregister multiple software nodes at once.
+ * Unregister multiple software nodes at once. If parent pointers are set up
+ * in any of the software nodes then the array **must** be ordered such that
+ * parents come before their children.
*
- * NOTE: Be careful using this call if the nodes had parent pointers set up in
- * them before registering. If so, it is wiser to remove the nodes
- * individually, in the correct order (child before parent) instead of relying
- * on the sequential order of the list of nodes in the array.
+ * NOTE: If you are uncertain whether the array is ordered such that
+ * parents will be unregistered before their children, it is wiser to
+ * remove the nodes individually, in the correct order (child before
+ * parent).
*/
void software_node_unregister_nodes(const struct software_node *nodes)
{
- int i;
+ unsigned int i = 0;
+
+ while (nodes[i].name)
+ i++;
- for (i = 0; nodes[i].name; i++)
+ while (i--)
software_node_unregister(&nodes[i]);
}
EXPORT_SYMBOL_GPL(software_node_unregister_nodes);
--
2.25.1
This function is used to find fwnode endpoints against a device. In
some instances those endpoints are software nodes which are children of
fwnode->secondary. Add support to fwnode_graph_get_endpoint_by_id() to
find those endpoints by recursively calling itself passing the ptr to
fwnode->secondary in the event no endpoint is found for the primary.
Reviewed-by: Andy Shevchenko <[email protected]>
Reviewed-by: Laurent Pinchart <[email protected]>
Acked-by: Sakari Ailus <[email protected]>
Signed-off-by: Daniel Scally <[email protected]>
---
Changes in v4:
- None
drivers/base/property.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/base/property.c b/drivers/base/property.c
index bc9c634df6df..ddba75d90af2 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -1163,7 +1163,14 @@ fwnode_graph_get_endpoint_by_id(const struct fwnode_handle *fwnode,
best_ep_id = fwnode_ep.id;
}
- return best_ep;
+ if (best_ep)
+ return best_ep;
+
+ if (fwnode && !IS_ERR_OR_NULL(fwnode->secondary))
+ return fwnode_graph_get_endpoint_by_id(fwnode->secondary, port,
+ endpoint, flags);
+
+ return NULL;
}
EXPORT_SYMBOL_GPL(fwnode_graph_get_endpoint_by_id);
--
2.25.1
V4L2 fwnode bus types are enumerated in v4l2-fwnode.c, meaning they aren't
available to the rest of the kernel. Move the enum to the corresponding
header so that I can use the label to refer to those values.
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 v4:
- Dropped the trailing comma from NR_OF_V4L2_FWNODE_BUS_TYPE
drivers/media/v4l2-core/v4l2-fwnode.c | 11 -----------
include/media/v4l2-fwnode.h | 22 ++++++++++++++++++++++
2 files changed, 22 insertions(+), 11 deletions(-)
diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
index 5353e37eb950..c1c2b3060532 100644
--- a/drivers/media/v4l2-core/v4l2-fwnode.c
+++ b/drivers/media/v4l2-core/v4l2-fwnode.c
@@ -28,17 +28,6 @@
#include <media/v4l2-fwnode.h>
#include <media/v4l2-subdev.h>
-enum v4l2_fwnode_bus_type {
- V4L2_FWNODE_BUS_TYPE_GUESS = 0,
- V4L2_FWNODE_BUS_TYPE_CSI2_CPHY,
- V4L2_FWNODE_BUS_TYPE_CSI1,
- V4L2_FWNODE_BUS_TYPE_CCP2,
- V4L2_FWNODE_BUS_TYPE_CSI2_DPHY,
- V4L2_FWNODE_BUS_TYPE_PARALLEL,
- V4L2_FWNODE_BUS_TYPE_BT656,
- NR_OF_V4L2_FWNODE_BUS_TYPE,
-};
-
static const struct v4l2_fwnode_bus_conv {
enum v4l2_fwnode_bus_type fwnode_bus_type;
enum v4l2_mbus_type mbus_type;
diff --git a/include/media/v4l2-fwnode.h b/include/media/v4l2-fwnode.h
index 4365430eea6f..77fd6a3ec308 100644
--- a/include/media/v4l2-fwnode.h
+++ b/include/media/v4l2-fwnode.h
@@ -213,6 +213,28 @@ struct v4l2_fwnode_connector {
} connector;
};
+/**
+ * enum v4l2_fwnode_bus_type - Video bus types defined by firmware properties
+ * @V4L2_FWNODE_BUS_TYPE_GUESS: Default value if no bus-type fwnode property
+ * @V4L2_FWNODE_BUS_TYPE_CSI2_CPHY: MIPI CSI-2 bus, C-PHY physical layer
+ * @V4L2_FWNODE_BUS_TYPE_CSI1: MIPI CSI-1 bus
+ * @V4L2_FWNODE_BUS_TYPE_CCP2: SMIA Compact Camera Port 2 bus
+ * @V4L2_FWNODE_BUS_TYPE_CSI2_DPHY: MIPI CSI-2 bus, D-PHY physical layer
+ * @V4L2_FWNODE_BUS_TYPE_PARALLEL: Camera Parallel Interface bus
+ * @V4L2_FWNODE_BUS_TYPE_BT656: BT.656 video format bus-type
+ * @NR_OF_V4L2_FWNODE_BUS_TYPE: Number of bus-types
+ */
+enum v4l2_fwnode_bus_type {
+ V4L2_FWNODE_BUS_TYPE_GUESS = 0,
+ V4L2_FWNODE_BUS_TYPE_CSI2_CPHY,
+ V4L2_FWNODE_BUS_TYPE_CSI1,
+ V4L2_FWNODE_BUS_TYPE_CCP2,
+ V4L2_FWNODE_BUS_TYPE_CSI2_DPHY,
+ V4L2_FWNODE_BUS_TYPE_PARALLEL,
+ V4L2_FWNODE_BUS_TYPE_BT656,
+ NR_OF_V4L2_FWNODE_BUS_TYPE
+};
+
/**
* v4l2_fwnode_endpoint_parse() - parse all fwnode node properties
* @fwnode: pointer to the endpoint's fwnode handle
--
2.25.1
To ensure we handle situations in which multiple sensors of the same
model (and therefore _HID) are present in a system, we need to be able
to iterate over devices matching a known _HID but unknown _UID and _HRV
- add acpi_dev_get_next_match_dev() to accommodate that possibility and
change acpi_dev_get_first_match_dev() to simply call the new function
with a NULL starting point. Add an iterator macro for convenience.
Reviewed-by: Andy Shevchenko <[email protected]>
Reviewed-by: Sakari Ailus <[email protected]>
Suggested-by: Andy Shevchenko <[email protected]>
Signed-off-by: Daniel Scally <[email protected]>
---
Changes in v4:
- None
drivers/acpi/utils.c | 30 ++++++++++++++++++++++++++----
include/acpi/acpi_bus.h | 7 +++++++
2 files changed, 33 insertions(+), 4 deletions(-)
diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c
index d5411a166685..ddca1550cce6 100644
--- a/drivers/acpi/utils.c
+++ b/drivers/acpi/utils.c
@@ -843,12 +843,13 @@ bool acpi_dev_present(const char *hid, const char *uid, s64 hrv)
EXPORT_SYMBOL(acpi_dev_present);
/**
- * acpi_dev_get_first_match_dev - Return the first match of ACPI device
+ * acpi_dev_get_next_match_dev - Return the next match of ACPI device
+ * @adev: Pointer to the previous acpi_device matching this @hid, @uid and @hrv
* @hid: Hardware ID of the device.
* @uid: Unique ID of the device, pass NULL to not check _UID
* @hrv: Hardware Revision of the device, pass -1 to not check _HRV
*
- * Return the first match of ACPI device if a matching device was present
+ * Return the next match of ACPI device if another matching device was present
* at the moment of invocation, or NULL otherwise.
*
* The caller is responsible to call put_device() on the returned device.
@@ -856,8 +857,9 @@ EXPORT_SYMBOL(acpi_dev_present);
* See additional information in acpi_dev_present() as well.
*/
struct acpi_device *
-acpi_dev_get_first_match_dev(const char *hid, const char *uid, s64 hrv)
+acpi_dev_get_next_match_dev(struct acpi_device *adev, const char *hid, const char *uid, s64 hrv)
{
+ struct device *start = adev ? &adev->dev : NULL;
struct acpi_dev_match_info match = {};
struct device *dev;
@@ -865,9 +867,29 @@ acpi_dev_get_first_match_dev(const char *hid, const char *uid, s64 hrv)
match.uid = uid;
match.hrv = hrv;
- dev = bus_find_device(&acpi_bus_type, NULL, &match, acpi_dev_match_cb);
+ dev = bus_find_device(&acpi_bus_type, start, &match, acpi_dev_match_cb);
return dev ? to_acpi_device(dev) : NULL;
}
+EXPORT_SYMBOL(acpi_dev_get_next_match_dev);
+
+/**
+ * acpi_dev_get_first_match_dev - Return the first match of ACPI device
+ * @hid: Hardware ID of the device.
+ * @uid: Unique ID of the device, pass NULL to not check _UID
+ * @hrv: Hardware Revision of the device, pass -1 to not check _HRV
+ *
+ * Return the first match of ACPI device if a matching device was present
+ * at the moment of invocation, or NULL otherwise.
+ *
+ * The caller is responsible to call put_device() on the returned device.
+ *
+ * See additional information in acpi_dev_present() as well.
+ */
+struct acpi_device *
+acpi_dev_get_first_match_dev(const char *hid, const char *uid, s64 hrv)
+{
+ return acpi_dev_get_next_match_dev(NULL, hid, uid, hrv);
+}
EXPORT_SYMBOL(acpi_dev_get_first_match_dev);
/*
diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
index a3abcc4b7d9f..0a028ba967d3 100644
--- a/include/acpi/acpi_bus.h
+++ b/include/acpi/acpi_bus.h
@@ -688,9 +688,16 @@ static inline bool acpi_device_can_poweroff(struct acpi_device *adev)
bool acpi_dev_hid_uid_match(struct acpi_device *adev, const char *hid2, const char *uid2);
+struct acpi_device *
+acpi_dev_get_next_match_dev(struct acpi_device *adev, const char *hid, const char *uid, s64 hrv);
struct acpi_device *
acpi_dev_get_first_match_dev(const char *hid, const char *uid, s64 hrv);
+#define for_each_acpi_dev_match(adev, hid, uid, hrv) \
+ for (adev = acpi_dev_get_first_match_dev(hid, uid, hrv); \
+ adev; \
+ adev = acpi_dev_get_next_match_dev(adev, hid, uid, hrv))
+
static inline void acpi_dev_put(struct acpi_device *adev)
{
put_device(&adev->dev);
--
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".
Reviewed-by: Laurent Pinchart <[email protected]>
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 v4:
- Replaced the FWNODE_GRAPH_PORT_NAME_PREFIX_LEN macro with
strlen("port@") throughout
- Added a check to software_node_graph_parse_endpoint() to ensure
the name of the endpoint's parent matches the expected port@n
format
drivers/base/swnode.c | 116 +++++++++++++++++++++++++++++++++++++++++-
1 file changed, 115 insertions(+), 1 deletion(-)
diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
index 1f43c51b431e..82f9d6326110 100644
--- a/drivers/base/swnode.c
+++ b/drivers/base/swnode.c
@@ -540,6 +540,116 @@ 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))) {
+ /*
+ * fwnode ports have naming style "port@", so we search for any
+ * children that follow that convention.
+ */
+ if (!strncmp(to_swnode(port)->node->name, "port@",
+ strlen("port@")))
+ 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);
+ const char *parent_name = swnode->parent->node->name;
+ int ret;
+
+ if (!(strlen(parent_name) > strlen("port@")) ||
+ strncmp(parent_name, "port@", strlen("port@")))
+ return -EINVAL;
+
+ /* Ports have naming style "port@n", we need to select the n */
+ ret = kstrtou32(parent_name + strlen("port@"),
+ 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 +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.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]>
Reviewed-by: Sakari Ailus <[email protected]>
Suggested-by: Andy Shevchenko <[email protected]>
Signed-off-by: Daniel Scally <[email protected]>
---
Changes in v4:
- Changed the language of the comment to be easier to follow
drivers/base/swnode.c | 15 +++++++++++----
1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
index ade49173ff8d..1f43c51b431e 100644
--- a/drivers/base/swnode.c
+++ b/drivers/base/swnode.c
@@ -779,16 +779,23 @@ 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 members of the array are
+ * children of another member then the children must appear later in the list such
+ * that they are unregistered first.
*/
-void software_node_unregister_node_group(const struct software_node **node_group)
+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 Sun, Jan 03, 2021 at 11:12:28PM +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".
Couple of nitpicks below (can be considered as follow up, depends on yours and
maintainer wishes).
> Reviewed-by: Laurent Pinchart <[email protected]>
> 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 v4:
>
> - Replaced the FWNODE_GRAPH_PORT_NAME_PREFIX_LEN macro with
> strlen("port@") throughout
> - Added a check to software_node_graph_parse_endpoint() to ensure
> the name of the endpoint's parent matches the expected port@n
> format
>
> drivers/base/swnode.c | 116 +++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 115 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
> index 1f43c51b431e..82f9d6326110 100644
> --- a/drivers/base/swnode.c
> +++ b/drivers/base/swnode.c
> @@ -540,6 +540,116 @@ 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))) {
> + /*
> + * fwnode ports have naming style "port@", so we search for any
> + * children that follow that convention.
> + */
> + if (!strncmp(to_swnode(port)->node->name, "port@",
> + strlen("port@")))
> + 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);
> + const char *parent_name = swnode->parent->node->name;
> + int ret;
> +
> + if (!(strlen(parent_name) > strlen("port@")) ||
A nit:
if (strlen("port@") >= strlen(parent_name) ||
better to read
> + strncmp(parent_name, "port@", strlen("port@")))
> + return -EINVAL;
> +
> + /* Ports have naming style "port@n", we need to select the n */
> + ret = kstrtou32(parent_name + strlen("port@"),
> + 10, &endpoint->port);
A nit:
ret = kstrtou32(parent_name + strlen("port@"), 10, &endpoint->port);
(perhaps you need to adjust your editor settings, this still fits 80)
> + 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 +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.25.1
>
--
With Best Regards,
Andy Shevchenko
Hi Andy
On 04/01/2021 10:22, Andy Shevchenko wrote:
> On Sun, Jan 03, 2021 at 11:12:28PM +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".
> Couple of nitpicks below (can be considered as follow up, depends on yours and
> maintainer wishes).
>
Thanks
> +static int
> +software_node_graph_parse_endpoint(const struct fwnode_handle *fwnode,
> + struct fwnode_endpoint *endpoint)
> +{
> + struct swnode *swnode = to_swnode(fwnode);
> + const char *parent_name = swnode->parent->node->name;
> + int ret;
> +
> + if (!(strlen(parent_name) > strlen("port@")) ||
> A nit:
>
> if (strlen("port@") >= strlen(parent_name) ||
>
> better to read
yeah agreed
>
>> + strncmp(parent_name, "port@", strlen("port@")))
>> + return -EINVAL;
>> +
>> + /* Ports have naming style "port@n", we need to select the n */
>> + ret = kstrtou32(parent_name + strlen("port@"),
>> + 10, &endpoint->port);
> A nit:
>
> ret = kstrtou32(parent_name + strlen("port@"), 10, &endpoint->port);
>
> (perhaps you need to adjust your editor settings, this still fits 80)
Ah - my bad. Originally instead of parent_name there was
swnode->parent->node->name, which didn't fit. When I added the temp
variable I forgot to fix the line break - thanks.
On Sun, Jan 03, 2021 at 11:12:33PM +0000, Daniel Scally wrote:
> To ensure we handle situations in which multiple sensors of the same
> model (and therefore _HID) are present in a system, we need to be able
> to iterate over devices matching a known _HID but unknown _UID and _HRV
> - add acpi_dev_get_next_match_dev() to accommodate that possibility and
> change acpi_dev_get_first_match_dev() to simply call the new function
> with a NULL starting point. Add an iterator macro for convenience.
I guess we need Rafael's blessing on this.
> Reviewed-by: Andy Shevchenko <[email protected]>
> Reviewed-by: Sakari Ailus <[email protected]>
> Suggested-by: Andy Shevchenko <[email protected]>
> Signed-off-by: Daniel Scally <[email protected]>
> ---
> Changes in v4:
>
> - None
>
> drivers/acpi/utils.c | 30 ++++++++++++++++++++++++++----
> include/acpi/acpi_bus.h | 7 +++++++
> 2 files changed, 33 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c
> index d5411a166685..ddca1550cce6 100644
> --- a/drivers/acpi/utils.c
> +++ b/drivers/acpi/utils.c
> @@ -843,12 +843,13 @@ bool acpi_dev_present(const char *hid, const char *uid, s64 hrv)
> EXPORT_SYMBOL(acpi_dev_present);
>
> /**
> - * acpi_dev_get_first_match_dev - Return the first match of ACPI device
> + * acpi_dev_get_next_match_dev - Return the next match of ACPI device
> + * @adev: Pointer to the previous acpi_device matching this @hid, @uid and @hrv
> * @hid: Hardware ID of the device.
> * @uid: Unique ID of the device, pass NULL to not check _UID
> * @hrv: Hardware Revision of the device, pass -1 to not check _HRV
> *
> - * Return the first match of ACPI device if a matching device was present
> + * Return the next match of ACPI device if another matching device was present
> * at the moment of invocation, or NULL otherwise.
> *
> * The caller is responsible to call put_device() on the returned device.
> @@ -856,8 +857,9 @@ EXPORT_SYMBOL(acpi_dev_present);
> * See additional information in acpi_dev_present() as well.
> */
> struct acpi_device *
> -acpi_dev_get_first_match_dev(const char *hid, const char *uid, s64 hrv)
> +acpi_dev_get_next_match_dev(struct acpi_device *adev, const char *hid, const char *uid, s64 hrv)
> {
> + struct device *start = adev ? &adev->dev : NULL;
> struct acpi_dev_match_info match = {};
> struct device *dev;
>
> @@ -865,9 +867,29 @@ acpi_dev_get_first_match_dev(const char *hid, const char *uid, s64 hrv)
> match.uid = uid;
> match.hrv = hrv;
>
> - dev = bus_find_device(&acpi_bus_type, NULL, &match, acpi_dev_match_cb);
> + dev = bus_find_device(&acpi_bus_type, start, &match, acpi_dev_match_cb);
> return dev ? to_acpi_device(dev) : NULL;
> }
> +EXPORT_SYMBOL(acpi_dev_get_next_match_dev);
> +
> +/**
> + * acpi_dev_get_first_match_dev - Return the first match of ACPI device
> + * @hid: Hardware ID of the device.
> + * @uid: Unique ID of the device, pass NULL to not check _UID
> + * @hrv: Hardware Revision of the device, pass -1 to not check _HRV
> + *
> + * Return the first match of ACPI device if a matching device was present
> + * at the moment of invocation, or NULL otherwise.
> + *
> + * The caller is responsible to call put_device() on the returned device.
> + *
> + * See additional information in acpi_dev_present() as well.
> + */
> +struct acpi_device *
> +acpi_dev_get_first_match_dev(const char *hid, const char *uid, s64 hrv)
> +{
> + return acpi_dev_get_next_match_dev(NULL, hid, uid, hrv);
> +}
> EXPORT_SYMBOL(acpi_dev_get_first_match_dev);
>
> /*
> diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
> index a3abcc4b7d9f..0a028ba967d3 100644
> --- a/include/acpi/acpi_bus.h
> +++ b/include/acpi/acpi_bus.h
> @@ -688,9 +688,16 @@ static inline bool acpi_device_can_poweroff(struct acpi_device *adev)
>
> bool acpi_dev_hid_uid_match(struct acpi_device *adev, const char *hid2, const char *uid2);
>
> +struct acpi_device *
> +acpi_dev_get_next_match_dev(struct acpi_device *adev, const char *hid, const char *uid, s64 hrv);
> struct acpi_device *
> acpi_dev_get_first_match_dev(const char *hid, const char *uid, s64 hrv);
>
> +#define for_each_acpi_dev_match(adev, hid, uid, hrv) \
> + for (adev = acpi_dev_get_first_match_dev(hid, uid, hrv); \
> + adev; \
> + adev = acpi_dev_get_next_match_dev(adev, hid, uid, hrv))
> +
> static inline void acpi_dev_put(struct acpi_device *adev)
> {
> put_device(&adev->dev);
> --
> 2.25.1
>
--
With Best Regards,
Andy Shevchenko
On 04/01/2021 12:42, Andy Shevchenko wrote:
> On Sun, Jan 03, 2021 at 11:12:33PM +0000, Daniel Scally wrote:
>> To ensure we handle situations in which multiple sensors of the same
>> model (and therefore _HID) are present in a system, we need to be able
>> to iterate over devices matching a known _HID but unknown _UID and _HRV
>> - add acpi_dev_get_next_match_dev() to accommodate that possibility and
>> change acpi_dev_get_first_match_dev() to simply call the new function
>> with a NULL starting point. Add an iterator macro for convenience.
> I guess we need Rafael's blessing on this.
For this one yes
On Sun, Jan 03, 2021 at 11:12:34PM +0000, Daniel Scally wrote:
> V4L2 fwnode bus types are enumerated in v4l2-fwnode.c, meaning they aren't
> available to the rest of the kernel. Move the enum to the corresponding
> header so that I can use the label to refer to those values.
Since it seems v5 will be the case, can you drop 'include: ' prefix in the
Subject line?
--
With Best Regards,
Andy Shevchenko
On Sun, Jan 03, 2021 at 11:12:27PM +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.
Since v5, can you modify subject prefix to be "device property: "?
Same for patches 3 and 4, please.
--
With Best Regards,
Andy Shevchenko
On 04/01/2021 14:24, Andy Shevchenko wrote:
> On Sun, Jan 03, 2021 at 11:12:27PM +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.
> Since v5, can you modify subject prefix to be "device property: "?
> Same for patches 3 and 4, please.
>
Will do, and for your last email too.
On Sun, Jan 03, 2021 at 11:12:33PM +0000, Daniel Scally wrote:
> To ensure we handle situations in which multiple sensors of the same
> model (and therefore _HID) are present in a system, we need to be able
> to iterate over devices matching a known _HID but unknown _UID and _HRV
> - add acpi_dev_get_next_match_dev() to accommodate that possibility and
> change acpi_dev_get_first_match_dev() to simply call the new function
> with a NULL starting point. Add an iterator macro for convenience.
Since v5, also please update prefix in the Subject here to be "ACPI / bus: ".
--
With Best Regards,
Andy Shevchenko