Hello all
Previous version:
https://lore.kernel.org/linux-media/[email protected]/
This series aims to add support for webcams on laptops with ACPI tables
designed for use with CIO2 on Windows. There are two main parts; the
first is extending the ipu3-cio2 driver to allow for patching the
firmware via software_nodes if endpoints aren't defined by ACPI. Patch #13
deals directly with that, all preceding patches are either supplemental
changes or incidental fixes along the way.
The second main part is a way to handle the unusual definition of resource
destined for the sensors in these ACPI tables; regulators and GPIO lines
that are supposed to be consumed by the sensor are lumped in the _CRS of
a dummy ACPI device upon which the sensor is dependent. Patch 18 defines a
new driver to handle those dummy devices and map the resources to the
sensor instead. 14-17 are supporting changes for that driver.
Changelogs mostly in the individual patches, but a broad summary:
- Altered fwnode_device_is_available() to return true if the
fwnode_handle doesn't implement that operation.
- Altered fwnode_graph_get_endpoint_by_id() to parse secondary
if no endpoint found on primary.
- Enforce parent->child ordering of software_nodes on registration
- Added a function to get the next ACPI device with matching _HID,
plus an iterator macro
- Altered cio2-bridge.c to store the bridge struct (and basically
everything else) in heap, plus removed the requirement to delay
ipu3-cio2 probe until after the i2c devices were instantiated.
Also now ensured we handle multiple sensors with the same _HID.
- Added a function to get devices _dependent_ on a given ACPI dev,
according to their _DEP entries.
- Added a function to explicitly construct the name of an I2C dev
created from an ACPI dev.
- Added a driver to handle the dummy ACPI devices discussed above.
Comments very welcome!
Dan Scally (1):
i2c: i2c-core-base: Use the new i2c_acpi_dev_name() in
i2c_set_dev_name()
Daniel Scally (16):
property: Return true in fwnode_device_is_available for node types
that do not implement this operation
property: Add support for calling fwnode_graph_get_endpoint_by_id()
for fwnode->secondary
software_node: Fix failure to put() and get() references to children
in software_node_get_next_child()
software_node: Enforce parent before child ordering of nodes array for
software_node_register_nodes()
software_node: Alter software_node_unregister_nodes() to unregister
the array in reverse order
software_node: amend software_node_unregister_node_group() to perform
unregistration of array in reverse order to be consistent with
software_node_unregister_nodes()
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 to allow module to be built from
multiple source files retaining ipu3-cio2 name
media: v4l2-core: v4l2-async: Check possible match in match_fwnode
based on sd->fwnode->secondary
acpi: Add acpi_dev_get_next_match_dev() and macro to iterate through
acpi_devices matching a given _HID
ipu3-cio2: Add functionality allowing software_node connections to
sensors on platforms designed for Windows
acpi: utils: Add function to fetch dependent acpi_devices
i2c: i2c-core-acpi: Add i2c_acpi_dev_name()
gpio: gpiolib-acpi: Export acpi_get_gpiod()
ipu3: Add driver for dummy INT3472 ACPI device
Heikki Krogerus (1):
software_node: Add support for fwnode_graph*() family of functions
MAINTAINERS | 9 +
drivers/acpi/utils.c | 98 ++++-
drivers/base/property.c | 9 +
drivers/base/swnode.c | 157 +++++++-
drivers/gpio/gpiolib-acpi.c | 3 +-
drivers/i2c/i2c-core-acpi.c | 14 +
drivers/i2c/i2c-core-base.c | 2 +-
drivers/media/pci/intel/ipu3/Kconfig | 32 ++
drivers/media/pci/intel/ipu3/Makefile | 4 +
drivers/media/pci/intel/ipu3/cio2-bridge.c | 260 ++++++++++++
drivers/media/pci/intel/ipu3/cio2-bridge.h | 108 +++++
drivers/media/pci/intel/ipu3/int3472.c | 381 ++++++++++++++++++
drivers/media/pci/intel/ipu3/int3472.h | 96 +++++
.../ipu3/{ipu3-cio2.c => ipu3-cio2-main.c} | 27 ++
drivers/media/pci/intel/ipu3/ipu3-cio2.h | 6 +
drivers/media/v4l2-core/v4l2-async.c | 8 +
include/acpi/acpi_bus.h | 9 +
include/linux/acpi.h | 5 +
include/linux/i2c.h | 5 +
lib/test_printf.c | 4 +-
20 files changed, 1213 insertions(+), 24 deletions(-)
create mode 100644 drivers/media/pci/intel/ipu3/cio2-bridge.c
create mode 100644 drivers/media/pci/intel/ipu3/cio2-bridge.h
create mode 100644 drivers/media/pci/intel/ipu3/int3472.c
create mode 100644 drivers/media/pci/intel/ipu3/int3472.h
rename drivers/media/pci/intel/ipu3/{ipu3-cio2.c => ipu3-cio2-main.c} (98%)
--
2.25.1
From: Dan Scally <[email protected]>
To make sure the new i2c_acpi_dev_name() always reflects the name of i2c
devices sourced from ACPI, use it in i2c_set_dev_name().
Signed-off-by: Dan Scally <[email protected]>
---
Changes since RFC v3:
- Patch introduced
drivers/i2c/i2c-core-base.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 573b5da145d1..a6d4ceb01077 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -814,7 +814,7 @@ static void i2c_dev_set_name(struct i2c_adapter *adap,
}
if (adev) {
- dev_set_name(&client->dev, "i2c-%s", acpi_dev_name(adev));
+ dev_set_name(&client->dev, i2c_acpi_dev_name(adev));
return;
}
--
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 to this function.
Suggested-by: Andy Shevchenko <[email protected]>
Signed-off-by: Daniel Scally <[email protected]>
---
Changes since RFC v3:
Patch introduced
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 615a0c93e116..af7930b3679e 100644
--- a/drivers/base/swnode.c
+++ b/drivers/base/swnode.c
@@ -700,14 +700,21 @@ int software_node_register_nodes(const struct software_node *nodes)
int i;
for (i = 0; nodes[i].name; i++) {
+ if (nodes[i].parent)
+ if (!software_node_to_swnode(nodes[i].parent)) {
+ ret = -EINVAL;
+ goto err_unregister_nodes;
+ }
+
ret = software_node_register(&nodes[i]);
- if (ret) {
- software_node_unregister_nodes(nodes);
- return ret;
- }
+ 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);
--
2.25.1
Some types of fwnode_handle do not implement the device_is_available()
check, such as those created by software_nodes. There isn't really a
meaningful way to check for the availability of a device that doesn't
actually exist, so if the check isn't implemented just assume that the
"device" is present.
Suggested-by: Laurent Pinchart <[email protected]>
Signed-off-by: Daniel Scally <[email protected]>
---
Changes since RFC v3:
patch introduced
drivers/base/property.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/base/property.c b/drivers/base/property.c
index 4c43d30145c6..a5ca2306796f 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -785,9 +785,14 @@ EXPORT_SYMBOL_GPL(fwnode_handle_put);
/**
* fwnode_device_is_available - check if a device is available for use
* @fwnode: Pointer to the fwnode of the device.
+ *
+ * For fwnode node types that don't implement the .device_is_available()
+ * operation, this function returns true.
*/
bool fwnode_device_is_available(const struct fwnode_handle *fwnode)
{
+ if (!fwnode_has_op(fwnode, device_is_available))
+ return true;
return fwnode_call_bool_op(fwnode, device_is_available);
}
EXPORT_SYMBOL_GPL(fwnode_device_is_available);
--
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.
Suggested-by: Andy Shevchenko <[email protected]>
Signed-off-by: Daniel Scally <[email protected]>
---
Changes since v3:
Patch introduced
drivers/base/swnode.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
index d39e1c76d98d..9bd0bb77ad5b 100644
--- a/drivers/base/swnode.c
+++ b/drivers/base/swnode.c
@@ -782,7 +782,10 @@ void software_node_unregister_node_group(const struct software_node **node_group
if (!node_group)
return;
- for (i = 0; node_group[i]; i++)
+ while (node_group[i]->name)
+ i++;
+
+ while (i--)
software_node_unregister(node_group[i]);
}
EXPORT_SYMBOL_GPL(software_node_unregister_node_group);
--
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: Andy Shevchenko <[email protected]>
Reviewed-by: Laurent Pinchart <[email protected]>
Signed-off-by: Daniel Scally <[email protected]>
---
Changes since RFC v3:
- None
MAINTAINERS | 1 +
1 file changed, 1 insertion(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index e3a828249c8c..9702b886d6a4 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -8930,6 +8930,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
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]>
Suggested-by: Andy Shevchenko <[email protected]>
Signed-off-by: Daniel Scally <[email protected]>
---
Changes since RFC v3:
Changed the called function name - didn't drop the tags since it's
such a trivial change, hope that's alright!
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
From: Heikki Krogerus <[email protected]>
From: Heikki Krogerus <[email protected]>
This implements the remaining .graph_* callbacks in the
fwnode operations vector for the software nodes. That makes
the fwnode_graph*() functions available in the drivers also
when software nodes are used.
The implementation tries to mimic the "OF graph" as much as
possible, but there is no support for the "reg" device
property. The ports will need to have the index in their
name which starts with "port" (for example "port0", "port1",
...) and endpoints will use the index of the software node
that is given to them during creation. The port nodes can
also be grouped under a specially named "ports" subnode,
just like in DT, if necessary.
The remote-endpoints are reference properties under the
endpoint nodes that are named "remote-endpoint".
Signed-off-by: Heikki Krogerus <[email protected]>
Co-developed-by: Daniel Scally <[email protected]>
Signed-off-by: Daniel Scally <[email protected]>
---
changes since RFC v3:
- Simplified software_node_get_next_endpoint() a little
- Squared away references in software_node_get_next_endpoint()
and swnode_graph_find_next_port(), since they were affected by
the change to the earlier patch that had *get_next_child() also
put the previous reference.
- Dropped Andy's R-b, since the code changed.
changes in RFC v3:
- removed software_node_device_is_available
- moved the change to software_node_get_next_child to a separate
patch
- switched to use fwnode_handle_put() in graph_get_next_endpoint()
instead of software_node_put()
changes in RFC v2:
- added software_node_device_is_available
- altered software_node_get_next_child to get references
- altered software_node_get_next_endpoint to release references
to ports and avoid passing invalid combinations of swnodes to
software_node_get_next_child
- altered swnode_graph_find_next_port to release port rather than
old
drivers/base/swnode.c | 110 +++++++++++++++++++++++++++++++++++++++++-
1 file changed, 109 insertions(+), 1 deletion(-)
diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
index 9bd0bb77ad5b..0c7a8d6b9ea8 100644
--- a/drivers/base/swnode.c
+++ b/drivers/base/swnode.c
@@ -540,6 +540,110 @@ software_node_get_reference_args(const struct fwnode_handle *fwnode,
return 0;
}
+static struct fwnode_handle *
+swnode_graph_find_next_port(const struct fwnode_handle *parent,
+ struct fwnode_handle *port)
+{
+ struct fwnode_handle *old = port;
+
+ while ((port = software_node_get_next_child(parent, old))) {
+ if (!strncmp(to_swnode(port)->node->name, "port", 4))
+ return port;
+ old = port;
+ }
+
+ return NULL;
+}
+
+static struct fwnode_handle *
+software_node_graph_get_next_endpoint(const struct fwnode_handle *fwnode,
+ struct fwnode_handle *endpoint)
+{
+ struct swnode *swnode = to_swnode(fwnode);
+ struct fwnode_handle *old = endpoint;
+ struct fwnode_handle *parent;
+ 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, old);
+ if (endpoint) {
+ fwnode_handle_put(port);
+ break;
+ }
+
+ /* No more endpoints for that port, so stop passing old */
+ old = NULL;
+ }
+
+ fwnode_handle_put(parent);
+
+ return endpoint;
+}
+
+static struct fwnode_handle *
+software_node_graph_get_remote_endpoint(const struct fwnode_handle *fwnode)
+{
+ struct swnode *swnode = to_swnode(fwnode);
+ const struct software_node_ref_args *ref;
+ const struct property_entry *prop;
+
+ if (!swnode)
+ return NULL;
+
+ prop = property_entry_get(swnode->node->properties, "remote-endpoint");
+ if (!prop || prop->type != DEV_PROP_REF || prop->is_inline)
+ return NULL;
+
+ ref = prop->pointer;
+
+ return software_node_get(software_node_fwnode(ref[0].node));
+}
+
+static struct fwnode_handle *
+software_node_graph_get_port_parent(struct fwnode_handle *fwnode)
+{
+ struct swnode *swnode = to_swnode(fwnode);
+ struct fwnode_handle *parent;
+
+ if (!strcmp(swnode->parent->node->name, "ports"))
+ parent = &swnode->parent->parent->fwnode;
+ else
+ parent = &swnode->parent->fwnode;
+
+ return software_node_get(parent);
+}
+
+static int
+software_node_graph_parse_endpoint(const struct fwnode_handle *fwnode,
+ struct fwnode_endpoint *endpoint)
+{
+ struct swnode *swnode = to_swnode(fwnode);
+ int ret;
+
+ ret = kstrtou32(swnode->parent->node->name + 4, 10, &endpoint->port);
+ if (ret)
+ return ret;
+
+ endpoint->id = swnode->id;
+ endpoint->local_fwnode = fwnode;
+
+ return 0;
+}
+
static const struct fwnode_operations software_node_ops = {
.get = software_node_get,
.put = software_node_put,
@@ -551,7 +655,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
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.
Signed-off-by: Daniel Scally <[email protected]>
---
Changes since RFC 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..6486dbde784f 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
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: Andy Shevchenko <[email protected]>
Reviewed-by: Laurent Pinchart <[email protected]>
Signed-off-by: Daniel Scally <[email protected]>
---
Changes since RFC 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
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.
Signed-off-by: Daniel Scally <[email protected]>
---
Changes since RFC v3:
Patch introduced. In discussion in the last submission I noted
that the CIO2 device doesn't have an ACPI fwnode - that turns
out to be true for _some_ devices but not others, so we need
this function to check the secondary too.
drivers/base/property.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/base/property.c b/drivers/base/property.c
index a5ca2306796f..4ece6b086e36 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -1162,6 +1162,10 @@ fwnode_graph_get_endpoint_by_id(const struct fwnode_handle *fwnode,
best_ep_id = fwnode_ep.id;
}
+ if (!best_ep && fwnode && !IS_ERR_OR_NULL(fwnode->secondary))
+ return fwnode_graph_get_endpoint_by_id(fwnode->secondary, port,
+ endpoint, flags);
+
return best_ep;
}
EXPORT_SYMBOL_GPL(fwnode_graph_get_endpoint_by_id);
--
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.
Suggested-by: Andy Shevchenko <[email protected]>
Signed-off-by: Daniel Scally <[email protected]>
---
Changes since RFC v3:
- Patch introduced
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..c177165c8db2 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
Some places in the kernel allow users to map resources to a device
using device name (for example, gpiod_lookup_table). Currently
this involves waiting for the i2c_client to have been registered so we
can use dev_name(&client->dev). Adding this function means that we can
achieve the same thing without having to wait to the i2c device.
Signed-off-by: Daniel Scally <[email protected]>
---
Changes since RFC v3:
- Patch introduced
drivers/i2c/i2c-core-acpi.c | 14 ++++++++++++++
include/linux/i2c.h | 5 +++++
2 files changed, 19 insertions(+)
diff --git a/drivers/i2c/i2c-core-acpi.c b/drivers/i2c/i2c-core-acpi.c
index 37c510d9347a..d3a653eac79e 100644
--- a/drivers/i2c/i2c-core-acpi.c
+++ b/drivers/i2c/i2c-core-acpi.c
@@ -497,6 +497,20 @@ struct i2c_client *i2c_acpi_new_device(struct device *dev, int index,
}
EXPORT_SYMBOL_GPL(i2c_acpi_new_device);
+/**
+ * i2c_acpi_dev_name - Construct i2c device name for devs sourced from ACPI
+ * @adev: ACPI device to construct the name for
+ *
+ * Prefixes "i2c-" to the ACPI device name, for use in i2c_dev_set_name() and
+ * also anywhere else in the kernel that needs to refer to an i2c device by
+ * name but before they have been instantiated.
+ */
+char *i2c_acpi_dev_name(struct acpi_device *adev)
+{
+ return kasprintf(GFP_KERNEL, "i2c-%s", acpi_dev_name(adev));
+}
+EXPORT_SYMBOL_GPL(i2c_acpi_dev_name);
+
#ifdef CONFIG_ACPI_I2C_OPREGION
static int acpi_gsb_i2c_read_bytes(struct i2c_client *client,
u8 cmd, u8 *data, u8 data_len)
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 56622658b215..ab0e505b2ca6 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -995,6 +995,7 @@ bool i2c_acpi_get_i2c_resource(struct acpi_resource *ares,
u32 i2c_acpi_find_bus_speed(struct device *dev);
struct i2c_client *i2c_acpi_new_device(struct device *dev, int index,
struct i2c_board_info *info);
+char *i2c_acpi_dev_name(struct acpi_device *adev);
struct i2c_adapter *i2c_acpi_find_adapter_by_handle(acpi_handle handle);
#else
static inline bool i2c_acpi_get_i2c_resource(struct acpi_resource *ares,
@@ -1011,6 +1012,10 @@ static inline struct i2c_client *i2c_acpi_new_device(struct device *dev,
{
return ERR_PTR(-ENODEV);
}
+static inline char *i2c_acpi_dev_name(struct acpi_device *adev)
+{
+ return NULL;
+}
static inline struct i2c_adapter *i2c_acpi_find_adapter_by_handle(acpi_handle handle)
{
return NULL;
--
2.25.1
ACPI devices declare themselves dependent on other devices via the _DEP
buffer. Fetching the dependee from dependent is a matter of parsing
_DEP, but currently there's no method to fetch dependent from dependee.
Add one, so we can parse sensors dependent on a PMIC from the PMIC's
acpi_driver.
Signed-off-by: Daniel Scally <[email protected]>
---
Changes since RFC v3:
- Patch introduced
drivers/acpi/utils.c | 68 +++++++++++++++++++++++++++++++++++++++++
include/acpi/acpi_bus.h | 2 ++
2 files changed, 70 insertions(+)
diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c
index c177165c8db2..7099529121db 100644
--- a/drivers/acpi/utils.c
+++ b/drivers/acpi/utils.c
@@ -807,6 +807,52 @@ static int acpi_dev_match_cb(struct device *dev, const void *data)
return hrv == match->hrv;
}
+static int acpi_dev_match_by_dep(struct device *dev, const void *data)
+{
+ struct acpi_device *adev = to_acpi_device(dev);
+ const struct acpi_device *dependee = data;
+ struct acpi_handle_list dep_handles;
+ struct acpi_device *candidate;
+ acpi_handle handle;
+ acpi_status status;
+ unsigned int i;
+ int ret;
+
+ handle = adev->handle;
+
+ if (!acpi_has_method(handle, "_DEP"))
+ return 0;
+
+ status = acpi_evaluate_reference(handle, "_DEP", NULL, &dep_handles);
+ if (ACPI_FAILURE(status))
+ return 0;
+
+ for (i = 0; i < dep_handles.count; i++) {
+ struct acpi_device_info *info;
+
+ status = acpi_get_object_info(dep_handles.handles[i], &info);
+ if (ACPI_FAILURE(status))
+ continue;
+
+ if (info->valid & ACPI_VALID_HID) {
+ ret = acpi_bus_get_device(dep_handles.handles[i], &candidate);
+ if (ret || !candidate) {
+ kfree(info);
+ continue;
+ }
+
+ if (candidate == dependee) {
+ acpi_dev_put(candidate);
+ kfree(info);
+ return 1;
+ }
+
+ kfree(info);
+ }
+ }
+ return 0;
+}
+
/**
* acpi_dev_present - Detect that a given ACPI device is present
* @hid: Hardware ID of the device.
@@ -842,6 +888,28 @@ bool acpi_dev_present(const char *hid, const char *uid, s64 hrv)
}
EXPORT_SYMBOL(acpi_dev_present);
+/**
+ * acpi_dev_get_next_dep_dev - Return next ACPI device dependent on input dev
+ * @adev: Pointer to the dependee device
+ * @prev: Pointer to the previous dependent device (or NULL for first match)
+ *
+ * Return the next ACPI device which declares itself dependent on @adev in
+ * the _DEP buffer.
+ *
+ * The caller is responsible to call put_device() on the returned device.
+ */
+struct acpi_device *acpi_dev_get_next_dep_dev(struct acpi_device *adev,
+ struct acpi_device *prev)
+{
+ struct device *start = prev ? &prev->dev : NULL;
+ struct device *dev;
+
+ dev = bus_find_device(&acpi_bus_type, start, adev, acpi_dev_match_by_dep);
+
+ return dev ? to_acpi_device(dev) : NULL;
+}
+EXPORT_SYMBOL(acpi_dev_get_next_dep_dev);
+
/**
* 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
diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
index 0a028ba967d3..f5dfeb030b9c 100644
--- a/include/acpi/acpi_bus.h
+++ b/include/acpi/acpi_bus.h
@@ -688,6 +688,8 @@ 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_dep_dev(struct acpi_device *adev, struct acpi_device *prev);
struct acpi_device *
acpi_dev_get_next_match_dev(struct acpi_device *adev, const char *hid, const char *uid, s64 hrv);
struct acpi_device *
--
2.25.1
Software nodes that are children of another software node should be
unregistered before their parent. To allow easy unregistering of an array
of software_nodes ordered parent to child, reverse the order in which
this function unregisters software_nodes.
Suggested-by: Andy Shevchenko <[email protected]>
Signed-off-by: Daniel Scally <[email protected]>
---
Changes since RFC v3:
Switched this functionality from a new function to replacing
the existing software_nodes_unregister_nodes()
drivers/base/swnode.c | 19 ++++++++++++-------
1 file changed, 12 insertions(+), 7 deletions(-)
diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
index af7930b3679e..d39e1c76d98d 100644
--- a/drivers/base/swnode.c
+++ b/drivers/base/swnode.c
@@ -720,20 +720,25 @@ 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
+ * @nodes: Zero terminated array of software nodes to be unregistered. 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.
*
* Unregister multiple software nodes at once.
*
- * 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
I need to be able to translate GPIO resources in an acpi_device's _CRS
into gpio_descs. Those are represented in _CRS as a pathname to a GPIO
device plus the pin's index number: this function is perfect for that
purpose.
Signed-off-by: Daniel Scally <[email protected]>
---
Changes since RFC v3:
- Patch introduced
drivers/gpio/gpiolib-acpi.c | 3 ++-
include/linux/acpi.h | 5 +++++
2 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index 834a12f3219e..cfadbc263475 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c
@@ -111,7 +111,7 @@ static int acpi_gpiochip_find(struct gpio_chip *gc, void *data)
* controller does not have GPIO chip registered at the moment. This is to
* support probe deferral.
*/
-static struct gpio_desc *acpi_get_gpiod(char *path, int pin)
+struct gpio_desc *acpi_get_gpiod(char *path, int pin)
{
struct gpio_chip *chip;
acpi_handle handle;
@@ -127,6 +127,7 @@ static struct gpio_desc *acpi_get_gpiod(char *path, int pin)
return gpiochip_get_desc(chip, pin);
}
+EXPORT_SYMBOL_GPL(acpi_get_gpiod);
static irqreturn_t acpi_gpio_irq_handler(int irq, void *data)
{
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 39263c6b52e1..737115a93138 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -1066,6 +1066,7 @@ void __acpi_handle_debug(struct _ddebug *descriptor, acpi_handle handle, const c
bool acpi_gpio_get_irq_resource(struct acpi_resource *ares,
struct acpi_resource_gpio **agpio);
int acpi_dev_gpio_irq_get(struct acpi_device *adev, int index);
+struct gpio_desc *acpi_get_gpiod(char *path, int pin);
#else
static inline bool acpi_gpio_get_irq_resource(struct acpi_resource *ares,
struct acpi_resource_gpio **agpio)
@@ -1076,6 +1077,10 @@ static inline int acpi_dev_gpio_irq_get(struct acpi_device *adev, int index)
{
return -ENXIO;
}
+struct gpio_desc *acpi_get_gpiod(char *path, int pin)
+{
+ return NULL;
+}
#endif
/* Device properties */
--
2.25.1
On (20/11/30 13:31), Daniel Scally wrote:
>
> 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]>
> Suggested-by: Andy Shevchenko <[email protected]>
> Signed-off-by: Daniel Scally <[email protected]>
Reviewed-by: Sergey Senozhatsky <[email protected]>
-ss
Hi Daniel,
Thank you for the patch.
On Mon, Nov 30, 2020 at 01:31:12PM +0000, Daniel Scally wrote:
> Some types of fwnode_handle do not implement the device_is_available()
> check, such as those created by software_nodes. There isn't really a
> meaningful way to check for the availability of a device that doesn't
> actually exist, so if the check isn't implemented just assume that the
> "device" is present.
>
> Suggested-by: Laurent Pinchart <[email protected]>
> Signed-off-by: Daniel Scally <[email protected]>
Reviewed-by: Laurent Pinchart <[email protected]>
> ---
> Changes since RFC v3:
>
> patch introduced
>
> drivers/base/property.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/drivers/base/property.c b/drivers/base/property.c
> index 4c43d30145c6..a5ca2306796f 100644
> --- a/drivers/base/property.c
> +++ b/drivers/base/property.c
> @@ -785,9 +785,14 @@ EXPORT_SYMBOL_GPL(fwnode_handle_put);
> /**
> * fwnode_device_is_available - check if a device is available for use
> * @fwnode: Pointer to the fwnode of the device.
> + *
> + * For fwnode node types that don't implement the .device_is_available()
> + * operation, this function returns true.
> */
> bool fwnode_device_is_available(const struct fwnode_handle *fwnode)
> {
> + if (!fwnode_has_op(fwnode, device_is_available))
> + return true;
> return fwnode_call_bool_op(fwnode, device_is_available);
> }
> EXPORT_SYMBOL_GPL(fwnode_device_is_available);
--
Regards,
Laurent Pinchart
Hi Daniel,
Thank you for the patch.
On Mon, Nov 30, 2020 at 01:31:15PM +0000, Daniel Scally wrote:
> 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 to this function.
>
> Suggested-by: Andy Shevchenko <[email protected]>
> Signed-off-by: Daniel Scally <[email protected]>
> ---
> Changes since RFC v3:
>
> Patch introduced
>
> 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 615a0c93e116..af7930b3679e 100644
> --- a/drivers/base/swnode.c
> +++ b/drivers/base/swnode.c
> @@ -700,14 +700,21 @@ int software_node_register_nodes(const struct software_node *nodes)
> int i;
>
> for (i = 0; nodes[i].name; i++) {
> + if (nodes[i].parent)
> + if (!software_node_to_swnode(nodes[i].parent)) {
> + ret = -EINVAL;
> + goto err_unregister_nodes;
> + }
> +
> ret = software_node_register(&nodes[i]);
> - if (ret) {
> - software_node_unregister_nodes(nodes);
> - return ret;
> - }
> + if (ret)
> + goto err_unregister_nodes;
> }
>
> return 0;
I'd add a blank line here.
Reviewed-by: Laurent Pinchart <[email protected]>
> +err_unregister_nodes:
> + software_node_unregister_nodes(nodes);
> + return ret;
> }
> EXPORT_SYMBOL_GPL(software_node_register_nodes);
>
--
Regards,
Laurent Pinchart
On Mon, Nov 30, 2020 at 06:11:52PM +0200, Laurent Pinchart wrote:
> Hi Daniel,
>
> Thank you for the patch.
>
> On Mon, Nov 30, 2020 at 01:31:15PM +0000, Daniel Scally wrote:
> > 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 to this function.
> >
> > Suggested-by: Andy Shevchenko <[email protected]>
> > Signed-off-by: Daniel Scally <[email protected]>
> > ---
> > Changes since RFC v3:
> >
> > Patch introduced
> >
> > 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 615a0c93e116..af7930b3679e 100644
> > --- a/drivers/base/swnode.c
> > +++ b/drivers/base/swnode.c
> > @@ -700,14 +700,21 @@ int software_node_register_nodes(const struct software_node *nodes)
> > int i;
> >
> > for (i = 0; nodes[i].name; i++) {
> > + if (nodes[i].parent)
> > + if (!software_node_to_swnode(nodes[i].parent)) {
> > + ret = -EINVAL;
> > + goto err_unregister_nodes;
> > + }
> > +
> > ret = software_node_register(&nodes[i]);
> > - if (ret) {
> > - software_node_unregister_nodes(nodes);
> > - return ret;
> > - }
> > + if (ret)
> > + goto err_unregister_nodes;
> > }
> >
> > return 0;
>
> I'd add a blank line here.
>
> Reviewed-by: Laurent Pinchart <[email protected]>
I spoke a bit too soon. Could you update the documentation of the
function to explain this new requirement ?
> > +err_unregister_nodes:
> > + software_node_unregister_nodes(nodes);
> > + return ret;
> > }
> > EXPORT_SYMBOL_GPL(software_node_register_nodes);
> >
>
> --
> Regards,
>
> Laurent Pinchart
--
Regards,
Laurent Pinchart
Hi Daniel,
Thank you for the patch.
On Mon, Nov 30, 2020 at 01:31:16PM +0000, Daniel Scally wrote:
> Software nodes that are children of another software node should be
> unregistered before their parent. To allow easy unregistering of an array
> of software_nodes ordered parent to child, reverse the order in which
> this function unregisters software_nodes.
>
> Suggested-by: Andy Shevchenko <[email protected]>
> Signed-off-by: Daniel Scally <[email protected]>
Reviewed-by: Laurent Pinchart <[email protected]>
> ---
> Changes since RFC v3:
>
> Switched this functionality from a new function to replacing
> the existing software_nodes_unregister_nodes()
>
> drivers/base/swnode.c | 19 ++++++++++++-------
> 1 file changed, 12 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
> index af7930b3679e..d39e1c76d98d 100644
> --- a/drivers/base/swnode.c
> +++ b/drivers/base/swnode.c
> @@ -720,20 +720,25 @@ 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
> + * @nodes: Zero terminated array of software nodes to be unregistered. 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.
> *
> * Unregister multiple software nodes at once.
> *
> - * 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);
--
Regards,
Laurent Pinchart
Hi Daniel,
Thank you for the patch.
The subject line is very long. We try to keep it within a 72 characters
limit in the kernel. That can be a challenge sometimes, and expections
can be accepted, but this one is reaaaally long.
(The same comment holds for other patches in the series)
On Mon, Nov 30, 2020 at 01:31:17PM +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.
>
> Suggested-by: Andy Shevchenko <[email protected]>
> Signed-off-by: Daniel Scally <[email protected]>
I"d squash this with the previous patch to avoid introducing an
inconsistency.
Reviewed-by: Laurent Pinchart <[email protected]>
> ---
> Changes since v3:
>
> Patch introduced
>
> drivers/base/swnode.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
> index d39e1c76d98d..9bd0bb77ad5b 100644
> --- a/drivers/base/swnode.c
> +++ b/drivers/base/swnode.c
> @@ -782,7 +782,10 @@ void software_node_unregister_node_group(const struct software_node **node_group
> if (!node_group)
> return;
>
> - for (i = 0; node_group[i]; i++)
> + while (node_group[i]->name)
> + i++;
> +
> + while (i--)
> software_node_unregister(node_group[i]);
> }
> EXPORT_SYMBOL_GPL(software_node_unregister_node_group);
--
Regards,
Laurent Pinchart
Hi Daniel and Heikki,
Thank you for the patch.
On Mon, Nov 30, 2020 at 01:31:18PM +0000, Daniel Scally wrote:
> From: Heikki Krogerus <[email protected]>
>
> From: Heikki Krogerus <[email protected]>
>
There seems to be one From: line too many. You can drop the one in the
commit message, git-format-patch will add it automatically.
> This implements the remaining .graph_* callbacks in the
> fwnode operations vector for the software nodes. That makes
s/vector/structure/ ?
> the fwnode_graph*() functions available in the drivers also
> when software nodes are used.
>
> The implementation tries to mimic the "OF graph" as much as
> possible, but there is no support for the "reg" device
> property. The ports will need to have the index in their
> name which starts with "port" (for example "port0", "port1",
> ...) and endpoints will use the index of the software node
> that is given to them during creation. The port nodes can
> also be grouped under a specially named "ports" subnode,
> just like in DT, if necessary.
I'm not very familiar with swnodes, but could we name ports port@n
instead of portn to mimic OF nodes ?
> 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 since RFC v3:
> - Simplified software_node_get_next_endpoint() a little
> - Squared away references in software_node_get_next_endpoint()
> and swnode_graph_find_next_port(), since they were affected by
> the change to the earlier patch that had *get_next_child() also
> put the previous reference.
> - Dropped Andy's R-b, since the code changed.
> changes in RFC v3:
> - removed software_node_device_is_available
> - moved the change to software_node_get_next_child to a separate
> patch
> - switched to use fwnode_handle_put() in graph_get_next_endpoint()
> instead of software_node_put()
>
> changes in RFC v2:
> - added software_node_device_is_available
> - altered software_node_get_next_child to get references
> - altered software_node_get_next_endpoint to release references
> to ports and avoid passing invalid combinations of swnodes to
> software_node_get_next_child
> - altered swnode_graph_find_next_port to release port rather than
> old
>
> drivers/base/swnode.c | 110 +++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 109 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
> index 9bd0bb77ad5b..0c7a8d6b9ea8 100644
> --- a/drivers/base/swnode.c
> +++ b/drivers/base/swnode.c
> @@ -540,6 +540,110 @@ software_node_get_reference_args(const struct fwnode_handle *fwnode,
> return 0;
> }
>
> +static struct fwnode_handle *
> +swnode_graph_find_next_port(const struct fwnode_handle *parent,
> + struct fwnode_handle *port)
> +{
> + struct fwnode_handle *old = port;
> +
> + while ((port = software_node_get_next_child(parent, old))) {
> + if (!strncmp(to_swnode(port)->node->name, "port", 4))
> + return port;
> + old = port;
> + }
> +
> + return NULL;
> +}
> +
> +static struct fwnode_handle *
> +software_node_graph_get_next_endpoint(const struct fwnode_handle *fwnode,
> + struct fwnode_handle *endpoint)
> +{
> + struct swnode *swnode = to_swnode(fwnode);
> + struct fwnode_handle *old = endpoint;
> + struct fwnode_handle *parent;
> + 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, old);
> + if (endpoint) {
> + fwnode_handle_put(port);
> + break;
> + }
> +
> + /* No more endpoints for that port, so stop passing old */
> + old = NULL;
> + }
> +
> + fwnode_handle_put(parent);
> +
> + return endpoint;
> +}
> +
> +static struct fwnode_handle *
> +software_node_graph_get_remote_endpoint(const struct fwnode_handle *fwnode)
> +{
> + struct swnode *swnode = to_swnode(fwnode);
> + const struct software_node_ref_args *ref;
> + const struct property_entry *prop;
> +
> + if (!swnode)
> + return NULL;
> +
> + prop = property_entry_get(swnode->node->properties, "remote-endpoint");
> + if (!prop || prop->type != DEV_PROP_REF || prop->is_inline)
> + return NULL;
> +
> + ref = prop->pointer;
> +
> + return software_node_get(software_node_fwnode(ref[0].node));
> +}
> +
> +static struct fwnode_handle *
> +software_node_graph_get_port_parent(struct fwnode_handle *fwnode)
> +{
> + struct swnode *swnode = to_swnode(fwnode);
> + struct fwnode_handle *parent;
> +
> + if (!strcmp(swnode->parent->node->name, "ports"))
> + parent = &swnode->parent->parent->fwnode;
> + else
> + parent = &swnode->parent->fwnode;
> +
> + return software_node_get(parent);
> +}
> +
> +static int
> +software_node_graph_parse_endpoint(const struct fwnode_handle *fwnode,
> + struct fwnode_endpoint *endpoint)
> +{
> + struct swnode *swnode = to_swnode(fwnode);
> + int ret;
> +
> + ret = kstrtou32(swnode->parent->node->name + 4, 10, &endpoint->port);
If we use port@, s/4/5/. But I suppose we also want to support the case
where a single port is used, with its name set to "port" ? The logic
would then need to be a tad more complex. Not sure if the consistency is
worth the additional complexity, up to you.
> + 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 +655,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 Daniel,
Thank you for the patch.
On Mon, Nov 30, 2020 at 01:31:19PM +0000, Daniel Scally wrote:
> 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]>
> Suggested-by: Andy Shevchenko <[email protected]>
> Signed-off-by: Daniel Scally <[email protected]>
Reviewed-by: Laurent Pinchart <[email protected]>
> ---
> Changes since RFC v3:
>
> Changed the called function name - didn't drop the tags since it's
> such a trivial change, hope that's alright!
>
> 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
--
Regards,
Laurent Pinchart
Hi Daniel,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on linuxtv-media/master]
[also build test ERROR on driver-core/driver-core-testing pm/linux-next v5.10-rc6 next-20201130]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Daniel-Scally/Add-functionality-to-ipu3-cio2-driver-allowing-software_node-connections-to-sensors-on-platforms-designed-for-Windows/20201130-214014
base: git://linuxtv.org/media_tree.git master
config: s390-randconfig-p001-20201130 (attached as .config)
compiler: s390-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/eb1854ac694a8e59c0ea703e46fe2ee7e3118b42
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Daniel-Scally/Add-functionality-to-ipu3-cio2-driver-allowing-software_node-connections-to-sensors-on-platforms-designed-for-Windows/20201130-214014
git checkout eb1854ac694a8e59c0ea703e46fe2ee7e3118b42
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=s390
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>
All errors (new ones prefixed by >>):
s390-linux-ld: kernel/sysctl.o: in function `acpi_get_gpiod':
>> sysctl.c:(.text+0x2bb8): multiple definition of `acpi_get_gpiod'; init/main.o:main.c:(.text+0x0): first defined here
s390-linux-ld: kernel/irq/irqdomain.o: in function `acpi_get_gpiod':
irqdomain.c:(.text+0x378): multiple definition of `acpi_get_gpiod'; init/main.o:main.c:(.text+0x0): first defined here
s390-linux-ld: kernel/dma/mapping.o: in function `acpi_get_gpiod':
mapping.c:(.text+0xb0): multiple definition of `acpi_get_gpiod'; init/main.o:main.c:(.text+0x0): first defined here
s390-linux-ld: drivers/gpio/gpiolib.o: in function `acpi_get_gpiod':
gpiolib.c:(.text+0x1c00): multiple definition of `acpi_get_gpiod'; init/main.o:main.c:(.text+0x0): first defined here
s390-linux-ld: drivers/gpio/gpio-pca953x.o: in function `acpi_get_gpiod':
gpio-pca953x.c:(.text+0x29e0): multiple definition of `acpi_get_gpiod'; init/main.o:main.c:(.text+0x0): first defined here
s390-linux-ld: drivers/gpio/gpio-pca9570.o: in function `acpi_get_gpiod':
gpio-pca9570.c:(.text+0x338): multiple definition of `acpi_get_gpiod'; init/main.o:main.c:(.text+0x0): first defined here
s390-linux-ld: drivers/gpio/gpio-pcf857x.o: in function `acpi_get_gpiod':
gpio-pcf857x.c:(.text+0xcb0): multiple definition of `acpi_get_gpiod'; init/main.o:main.c:(.text+0x0): first defined here
s390-linux-ld: drivers/gpio/gpio-tpic2810.o: in function `acpi_get_gpiod':
gpio-tpic2810.c:(.text+0x418): multiple definition of `acpi_get_gpiod'; init/main.o:main.c:(.text+0x0): first defined here
s390-linux-ld: drivers/pwm/core.o: in function `acpi_get_gpiod':
core.c:(.text+0x660): multiple definition of `acpi_get_gpiod'; init/main.o:main.c:(.text+0x0): first defined here
s390-linux-ld: drivers/pwm/pwm-pca9685.o: in function `acpi_get_gpiod':
pwm-pca9685.c:(.text+0x10d8): multiple definition of `acpi_get_gpiod'; init/main.o:main.c:(.text+0x0): first defined here
s390-linux-ld: drivers/dma/dmaengine.o: in function `acpi_get_gpiod':
dmaengine.c:(.text+0x1f30): multiple definition of `acpi_get_gpiod'; init/main.o:main.c:(.text+0x0): first defined here
s390-linux-ld: drivers/dma/dw/platform.o: in function `acpi_get_gpiod':
platform.c:(.text+0x420): multiple definition of `acpi_get_gpiod'; init/main.o:main.c:(.text+0x0): first defined here
s390-linux-ld: drivers/dma/qcom/hidma.o: in function `acpi_get_gpiod':
hidma.c:(.text+0x28e8): multiple definition of `acpi_get_gpiod'; init/main.o:main.c:(.text+0x0): first defined here
s390-linux-ld: drivers/regulator/88pg86x.o: in function `acpi_get_gpiod':
88pg86x.c:(.text+0x198): multiple definition of `acpi_get_gpiod'; init/main.o:main.c:(.text+0x0): first defined here
s390-linux-ld: drivers/regulator/ad5398.o: in function `acpi_get_gpiod':
ad5398.c:(.text+0x780): multiple definition of `acpi_get_gpiod'; init/main.o:main.c:(.text+0x0): first defined here
s390-linux-ld: drivers/regulator/da9210-regulator.o: in function `acpi_get_gpiod':
da9210-regulator.c:(.text+0x628): multiple definition of `acpi_get_gpiod'; init/main.o:main.c:(.text+0x0): first defined here
s390-linux-ld: drivers/regulator/isl6271a-regulator.o: in function `acpi_get_gpiod':
isl6271a-regulator.c:(.text+0x3b8): multiple definition of `acpi_get_gpiod'; init/main.o:main.c:(.text+0x0): first defined here
s390-linux-ld: drivers/regulator/isl9305.o: in function `acpi_get_gpiod':
isl9305.c:(.text+0x1a0): multiple definition of `acpi_get_gpiod'; init/main.o:main.c:(.text+0x0): first defined here
s390-linux-ld: drivers/regulator/lp872x.o: in function `acpi_get_gpiod':
lp872x.c:(.text+0x11c0): multiple definition of `acpi_get_gpiod'; init/main.o:main.c:(.text+0x0): first defined here
s390-linux-ld: drivers/regulator/lp8755.o: in function `acpi_get_gpiod':
lp8755.c:(.text+0x1148): multiple definition of `acpi_get_gpiod'; init/main.o:main.c:(.text+0x0): first defined here
s390-linux-ld: drivers/regulator/ltc3589.o: in function `acpi_get_gpiod':
ltc3589.c:(.text+0x8c8): multiple definition of `acpi_get_gpiod'; init/main.o:main.c:(.text+0x0): first defined here
s390-linux-ld: drivers/regulator/max8649.o: in function `acpi_get_gpiod':
max8649.c:(.text+0x7a0): multiple definition of `acpi_get_gpiod'; init/main.o:main.c:(.text+0x0): first defined here
s390-linux-ld: drivers/regulator/max8660.o: in function `acpi_get_gpiod':
max8660.c:(.text+0xe28): multiple definition of `acpi_get_gpiod'; init/main.o:main.c:(.text+0x0): first defined here
s390-linux-ld: drivers/regulator/max77826-regulator.o: in function `acpi_get_gpiod':
max77826-regulator.c:(.text+0x230): multiple definition of `acpi_get_gpiod'; init/main.o:main.c:(.text+0x0): first defined here
s390-linux-ld: drivers/regulator/mp8859.o: in function `acpi_get_gpiod':
mp8859.c:(.text+0x2e0): multiple definition of `acpi_get_gpiod'; init/main.o:main.c:(.text+0x0): first defined here
s390-linux-ld: drivers/regulator/mt6311-regulator.o: in function `acpi_get_gpiod':
mt6311-regulator.c:(.text+0x248): multiple definition of `acpi_get_gpiod'; init/main.o:main.c:(.text+0x0): first defined here
s390-linux-ld: drivers/regulator/pca9450-regulator.o: in function `acpi_get_gpiod':
pca9450-regulator.c:(.text+0x790): multiple definition of `acpi_get_gpiod'; init/main.o:main.c:(.text+0x0): first defined here
s390-linux-ld: drivers/regulator/pfuze100-regulator.o: in function `acpi_get_gpiod':
pfuze100-regulator.c:(.text+0xaa8): multiple definition of `acpi_get_gpiod'; init/main.o:main.c:(.text+0x0): first defined here
s390-linux-ld: drivers/regulator/pv88060-regulator.o: in function `acpi_get_gpiod':
pv88060-regulator.c:(.text+0x870): multiple definition of `acpi_get_gpiod'; init/main.o:main.c:(.text+0x0): first defined here
s390-linux-ld: drivers/regulator/pv88080-regulator.o: in function `acpi_get_gpiod':
pv88080-regulator.c:(.text+0xb38): multiple definition of `acpi_get_gpiod'; init/main.o:main.c:(.text+0x0): first defined here
s390-linux-ld: drivers/regulator/pv88090-regulator.o: in function `acpi_get_gpiod':
pv88090-regulator.c:(.text+0x960): multiple definition of `acpi_get_gpiod'; init/main.o:main.c:(.text+0x0): first defined here
s390-linux-ld: drivers/regulator/rt4801-regulator.o: in function `acpi_get_gpiod':
rt4801-regulator.c:(.text+0x6a0): multiple definition of `acpi_get_gpiod'; init/main.o:main.c:(.text+0x0): first defined here
s390-linux-ld: drivers/regulator/rtmv20-regulator.o: in function `acpi_get_gpiod':
rtmv20-regulator.c:(.text+0xb20): multiple definition of `acpi_get_gpiod'; init/main.o:main.c:(.text+0x0): first defined here
s390-linux-ld: drivers/regulator/slg51000-regulator.o: in function `acpi_get_gpiod':
slg51000-regulator.c:(.text+0xb28): multiple definition of `acpi_get_gpiod'; init/main.o:main.c:(.text+0x0): first defined here
s390-linux-ld: drivers/regulator/tps62360-regulator.o: in function `acpi_get_gpiod':
tps62360-regulator.c:(.text+0xe68): multiple definition of `acpi_get_gpiod'; init/main.o:main.c:(.text+0x0): first defined here
s390-linux-ld: drivers/regulator/tps65132-regulator.o: in function `acpi_get_gpiod':
tps65132-regulator.c:(.text+0x728): multiple definition of `acpi_get_gpiod'; init/main.o:main.c:(.text+0x0): first defined here
s390-linux-ld: drivers/base/core.o: in function `acpi_get_gpiod':
core.c:(.text+0x27f8): multiple definition of `acpi_get_gpiod'; init/main.o:main.c:(.text+0x0): first defined here
s390-linux-ld: drivers/base/platform.o: in function `acpi_get_gpiod':
platform.c:(.text+0x798): multiple definition of `acpi_get_gpiod'; init/main.o:main.c:(.text+0x0): first defined here
s390-linux-ld: drivers/base/cpu.o: in function `acpi_get_gpiod':
cpu.c:(.text+0x8c8): multiple definition of `acpi_get_gpiod'; init/main.o:main.c:(.text+0x0): first defined here
s390-linux-ld: drivers/base/property.o: in function `acpi_get_gpiod':
property.c:(.text+0x2f8): multiple definition of `acpi_get_gpiod'; init/main.o:main.c:(.text+0x0): first defined here
s390-linux-ld: drivers/base/cacheinfo.o: in function `acpi_get_gpiod':
cacheinfo.c:(.text+0xca8): multiple definition of `acpi_get_gpiod'; init/main.o:main.c:(.text+0x0): first defined here
s390-linux-ld: drivers/base/regmap/regmap-i2c.o: in function `acpi_get_gpiod':
regmap-i2c.c:(.text+0xcb0): multiple definition of `acpi_get_gpiod'; init/main.o:main.c:(.text+0x0): first defined here
s390-linux-ld: drivers/misc/ad525x_dpot-i2c.o: in function `acpi_get_gpiod':
ad525x_dpot-i2c.c:(.text+0x278): multiple definition of `acpi_get_gpiod'; init/main.o:main.c:(.text+0x0): first defined here
s390-linux-ld: drivers/misc/bh1770glc.o: in function `acpi_get_gpiod':
bh1770glc.c:(.text+0x28a8): multiple definition of `acpi_get_gpiod'; init/main.o:main.c:(.text+0x0): first defined here
s390-linux-ld: drivers/misc/apds990x.o: in function `acpi_get_gpiod':
apds990x.c:(.text+0x23a8): multiple definition of `acpi_get_gpiod'; init/main.o:main.c:(.text+0x0): first defined here
s390-linux-ld: drivers/misc/isl29003.o: in function `acpi_get_gpiod':
isl29003.c:(.text+0xad8): multiple definition of `acpi_get_gpiod'; init/main.o:main.c:(.text+0x0): first defined here
s390-linux-ld: drivers/misc/isl29020.o: in function `acpi_get_gpiod':
isl29020.c:(.text+0x5e8): multiple definition of `acpi_get_gpiod'; init/main.o:main.c:(.text+0x0): first defined here
s390-linux-ld: drivers/misc/tsl2550.o: in function `acpi_get_gpiod':
tsl2550.c:(.text+0x9d8): multiple definition of `acpi_get_gpiod'; init/main.o:main.c:(.text+0x0): first defined here
s390-linux-ld: drivers/misc/ds1682.o: in function `acpi_get_gpiod':
ds1682.c:(.text+0x578): multiple definition of `acpi_get_gpiod'; init/main.o:main.c:(.text+0x0): first defined here
s390-linux-ld: drivers/misc/hmc6352.o: in function `acpi_get_gpiod':
hmc6352.c:(.text+0x568): multiple definition of `acpi_get_gpiod'; init/main.o:main.c:(.text+0x0): first defined here
s390-linux-ld: drivers/misc/eeprom/eeprom.o: in function `acpi_get_gpiod':
eeprom.c:(.text+0x7a0): multiple definition of `acpi_get_gpiod'; init/main.o:main.c:(.text+0x0): first defined here
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]
Hi Daniel,
Thank you for the patch.
On Mon, Nov 30, 2020 at 01:31:26PM +0000, Daniel Scally wrote:
> Some places in the kernel allow users to map resources to a device
> using device name (for example, gpiod_lookup_table). Currently
> this involves waiting for the i2c_client to have been registered so we
> can use dev_name(&client->dev). Adding this function means that we can
> achieve the same thing without having to wait to the i2c device.
>
> Signed-off-by: Daniel Scally <[email protected]>
> ---
> Changes since RFC v3:
>
> - Patch introduced
>
> drivers/i2c/i2c-core-acpi.c | 14 ++++++++++++++
> include/linux/i2c.h | 5 +++++
> 2 files changed, 19 insertions(+)
>
> diff --git a/drivers/i2c/i2c-core-acpi.c b/drivers/i2c/i2c-core-acpi.c
> index 37c510d9347a..d3a653eac79e 100644
> --- a/drivers/i2c/i2c-core-acpi.c
> +++ b/drivers/i2c/i2c-core-acpi.c
> @@ -497,6 +497,20 @@ struct i2c_client *i2c_acpi_new_device(struct device *dev, int index,
> }
> EXPORT_SYMBOL_GPL(i2c_acpi_new_device);
>
> +/**
> + * i2c_acpi_dev_name - Construct i2c device name for devs sourced from ACPI
> + * @adev: ACPI device to construct the name for
> + *
> + * Prefixes "i2c-" to the ACPI device name, for use in i2c_dev_set_name() and
> + * also anywhere else in the kernel that needs to refer to an i2c device by
> + * name but before they have been instantiated.
The documentation should state that the caller must free the return
value.
> + */
> +char *i2c_acpi_dev_name(struct acpi_device *adev)
> +{
> + return kasprintf(GFP_KERNEL, "i2c-%s", acpi_dev_name(adev));
> +}
> +EXPORT_SYMBOL_GPL(i2c_acpi_dev_name);
> +
> #ifdef CONFIG_ACPI_I2C_OPREGION
> static int acpi_gsb_i2c_read_bytes(struct i2c_client *client,
> u8 cmd, u8 *data, u8 data_len)
> diff --git a/include/linux/i2c.h b/include/linux/i2c.h
> index 56622658b215..ab0e505b2ca6 100644
> --- a/include/linux/i2c.h
> +++ b/include/linux/i2c.h
> @@ -995,6 +995,7 @@ bool i2c_acpi_get_i2c_resource(struct acpi_resource *ares,
> u32 i2c_acpi_find_bus_speed(struct device *dev);
> struct i2c_client *i2c_acpi_new_device(struct device *dev, int index,
> struct i2c_board_info *info);
> +char *i2c_acpi_dev_name(struct acpi_device *adev);
> struct i2c_adapter *i2c_acpi_find_adapter_by_handle(acpi_handle handle);
> #else
> static inline bool i2c_acpi_get_i2c_resource(struct acpi_resource *ares,
> @@ -1011,6 +1012,10 @@ static inline struct i2c_client *i2c_acpi_new_device(struct device *dev,
> {
> return ERR_PTR(-ENODEV);
> }
> +static inline char *i2c_acpi_dev_name(struct acpi_device *adev)
> +{
> + return NULL;
> +}
> static inline struct i2c_adapter *i2c_acpi_find_adapter_by_handle(acpi_handle handle)
> {
> return NULL;
--
Regards,
Laurent Pinchart
Hi Daniel,
Thank you for the patch.
On Mon, Nov 30, 2020 at 01:31:27PM +0000, Daniel Scally wrote:
> From: Dan Scally <[email protected]>
>
> To make sure the new i2c_acpi_dev_name() always reflects the name of i2c
> devices sourced from ACPI, use it in i2c_set_dev_name().
>
> Signed-off-by: Dan Scally <[email protected]>
I'd squash this with 15/18, which would make it clear there's a memory
leak :-)
> ---
> Changes since RFC v3:
>
> - Patch introduced
>
> drivers/i2c/i2c-core-base.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> index 573b5da145d1..a6d4ceb01077 100644
> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c
> @@ -814,7 +814,7 @@ static void i2c_dev_set_name(struct i2c_adapter *adap,
> }
>
> if (adev) {
> - dev_set_name(&client->dev, "i2c-%s", acpi_dev_name(adev));
> + dev_set_name(&client->dev, i2c_acpi_dev_name(adev));
> return;
> }
>
--
Regards,
Laurent Pinchart
Hi Daniel,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on linuxtv-media/master]
[also build test WARNING on driver-core/driver-core-testing pm/linux-next v5.10-rc6 next-20201130]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Daniel-Scally/Add-functionality-to-ipu3-cio2-driver-allowing-software_node-connections-to-sensors-on-platforms-designed-for-Windows/20201130-214014
base: git://linuxtv.org/media_tree.git master
config: powerpc-randconfig-r031-20201130 (attached as .config)
compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project dfcf1acf13226be0f599a7ab6b395b66dc9545d6)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install powerpc cross compiling tool for clang build
# apt-get install binutils-powerpc-linux-gnu
# https://github.com/0day-ci/linux/commit/7c7577c82672f0a0775ac1ad85358e2dc17b2c91
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Daniel-Scally/Add-functionality-to-ipu3-cio2-driver-allowing-software_node-connections-to-sensors-on-platforms-designed-for-Windows/20201130-214014
git checkout 7c7577c82672f0a0775ac1ad85358e2dc17b2c91
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=powerpc
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>
All warnings (new ones prefixed by >>):
>> drivers/base/swnode.c:785:20: warning: variable 'i' is uninitialized when used here [-Wuninitialized]
while (node_group[i]->name)
^
drivers/base/swnode.c:780:16: note: initialize the variable 'i' to silence this warning
unsigned int i;
^
= 0
1 warning generated.
vim +/i +785 drivers/base/swnode.c
771
772 /**
773 * software_node_unregister_node_group - Unregister a group of software nodes
774 * @node_group: NULL terminated array of software node pointers to be unregistered
775 *
776 * Unregister multiple software nodes at once.
777 */
778 void software_node_unregister_node_group(const struct software_node **node_group)
779 {
780 unsigned int i;
781
782 if (!node_group)
783 return;
784
> 785 while (node_group[i]->name)
786 i++;
787
788 while (i--)
789 software_node_unregister(node_group[i]);
790 }
791 EXPORT_SYMBOL_GPL(software_node_unregister_node_group);
792
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]
On Mon, Nov 30, 2020 at 01:31:12PM +0000, Daniel Scally wrote:
> Some types of fwnode_handle do not implement the device_is_available()
> check, such as those created by software_nodes. There isn't really a
> meaningful way to check for the availability of a device that doesn't
> actually exist, so if the check isn't implemented just assume that the
> "device" is present.
Reviewed-by: Andy Shevchenko <[email protected]>
> Suggested-by: Laurent Pinchart <[email protected]>
> Signed-off-by: Daniel Scally <[email protected]>
> ---
> Changes since RFC v3:
>
> patch introduced
>
> drivers/base/property.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/drivers/base/property.c b/drivers/base/property.c
> index 4c43d30145c6..a5ca2306796f 100644
> --- a/drivers/base/property.c
> +++ b/drivers/base/property.c
> @@ -785,9 +785,14 @@ EXPORT_SYMBOL_GPL(fwnode_handle_put);
> /**
> * fwnode_device_is_available - check if a device is available for use
> * @fwnode: Pointer to the fwnode of the device.
> + *
> + * For fwnode node types that don't implement the .device_is_available()
> + * operation, this function returns true.
> */
> bool fwnode_device_is_available(const struct fwnode_handle *fwnode)
> {
> + if (!fwnode_has_op(fwnode, device_is_available))
> + return true;
> return fwnode_call_bool_op(fwnode, device_is_available);
> }
> EXPORT_SYMBOL_GPL(fwnode_device_is_available);
> --
> 2.25.1
>
--
With Best Regards,
Andy Shevchenko
On Mon, Nov 30, 2020 at 01:31:13PM +0000, Daniel Scally wrote:
> 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.
One nit below, after addressing:
Reviewed-by: Andy Shevchenko <[email protected]>
...
> + if (!best_ep && fwnode && !IS_ERR_OR_NULL(fwnode->secondary))
> + return fwnode_graph_get_endpoint_by_id(fwnode->secondary, port,
> + endpoint, flags);
> return best_ep;
Can we, please, do
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;
?
This 'if (fwnode && !IS_ERR_OR_NULL(fwnode->secondary))' becomes kinda
idiomatic to the cases when we need to proceed primary followed by the
secondary in cases where it's not already done.
> }
> EXPORT_SYMBOL_GPL(fwnode_graph_get_endpoint_by_id);
--
With Best Regards,
Andy Shevchenko
On Mon, Nov 30, 2020 at 01:31:15PM +0000, Daniel Scally wrote:
> 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 to this function.
I agree with Laurent.
...
> for (i = 0; nodes[i].name; i++) {
> + if (nodes[i].parent)
> + if (!software_node_to_swnode(nodes[i].parent)) {
> + ret = -EINVAL;
> + goto err_unregister_nodes;
> + }
> +
Besides that can we pack these conditionals together?
if (nodes[i].parent && !software_node_to_swnode(nodes[i].parent)) {
Do we have sane ordering in software_node_unregister_nodes()?
--
With Best Regards,
Andy Shevchenko
On Mon, Nov 30, 2020 at 07:35:30PM +0200, Andy Shevchenko wrote:
> On Mon, Nov 30, 2020 at 01:31:15PM +0000, Daniel Scally wrote:
...
> > for (i = 0; nodes[i].name; i++) {
> > + if (nodes[i].parent)
> > + if (!software_node_to_swnode(nodes[i].parent)) {
> > + ret = -EINVAL;
> > + goto err_unregister_nodes;
> > + }
> > +
>
> Besides that can we pack these conditionals together?
>
> if (nodes[i].parent && !software_node_to_swnode(nodes[i].parent)) {
For being it shorter you may use temporary variable:
software_node *parent;
parent = nodes[i].parent;
if (parent && !software_node_to_swnode(parent)) {
--
With Best Regards,
Andy Shevchenko
On Mon, Nov 30, 2020 at 01:31:16PM +0000, Daniel Scally wrote:
> Software nodes that are children of another software node should be
> unregistered before their parent. To allow easy unregistering of an array
> of software_nodes ordered parent to child, reverse the order in which
> this function unregisters software_nodes.
Should be folded in the previous patch. Otherwise we will have a history point
where register() behaves differently to unregister().
...
> + * @nodes: Zero terminated array of software nodes to be unregistered. 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.
Please, leave field description short. Rather add another note to the
Description below.
> *
> * Unregister multiple software nodes at once.
> *
> - * 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).
> */
--
With Best Regards,
Andy Shevchenko
On Mon, Nov 30, 2020 at 06:17:16PM +0200, Laurent Pinchart wrote:
> Hi Daniel,
>
> Thank you for the patch.
>
> The subject line is very long. We try to keep it within a 72 characters
> limit in the kernel. That can be a challenge sometimes, and expections
> can be accepted, but this one is reaaaally long.
>
> (The same comment holds for other patches in the series)
+1.
> On Mon, Nov 30, 2020 at 01:31:17PM +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.
> >
> > Suggested-by: Andy Shevchenko <[email protected]>
> > Signed-off-by: Daniel Scally <[email protected]>
>
> I"d squash this with the previous patch to avoid introducing an
> inconsistency.
It's different to previous. It touches not complementary API, but different
one. However, I would follow your comment about documenting the behaviour of
these two APIs as well…
--
With Best Regards,
Andy Shevchenko
On Mon, Nov 30, 2020 at 07:28:57PM +0200, Laurent Pinchart wrote:
> On Mon, Nov 30, 2020 at 07:29:00PM +0200, Andy Shevchenko wrote:
> > On Mon, Nov 30, 2020 at 01:31:13PM +0000, Daniel Scally wrote:
...
> > > + if (!best_ep && fwnode && !IS_ERR_OR_NULL(fwnode->secondary))
> > > + return fwnode_graph_get_endpoint_by_id(fwnode->secondary, port,
> > > + endpoint, flags);
> >
> > > return best_ep;
> >
> > Can we, please, do
> >
> > 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;
> >
> > ?
> >
> > This 'if (fwnode && !IS_ERR_OR_NULL(fwnode->secondary))' becomes kinda
> > idiomatic to the cases when we need to proceed primary followed by the
> > secondary in cases where it's not already done.
>
> We could also move the !fwnode check to the beginning of the function.
It's already there (1). What did I miss?
1) via fwnode_graph_get_next_endpoint() -> fwnode_call_ptr_op()
--
With Best Regards,
Andy Shevchenko
On Mon, Nov 30, 2020 at 01:31:22PM +0000, Daniel Scally wrote:
> 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.
One nit below.
Reviewed-by: Andy Shevchenko <[email protected]>
> Signed-off-by: Daniel Scally <[email protected]>
> ---
> Changes since RFC 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..6486dbde784f 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)) &&
Too many parentheses.
> + 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
>
--
With Best Regards,
Andy Shevchenko
On Mon, Nov 30, 2020 at 01:31:23PM +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.
Reviewed-by: Andy Shevchenko <[email protected]>
> Suggested-by: Andy Shevchenko <[email protected]>
> Signed-off-by: Daniel Scally <[email protected]>
> ---
> Changes since RFC v3:
>
> - Patch introduced
>
> 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..c177165c8db2 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
Hi Daniel,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on linuxtv-media/master]
[also build test ERROR on driver-core/driver-core-testing pm/linux-next v5.10-rc6 next-20201130]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Daniel-Scally/Add-functionality-to-ipu3-cio2-driver-allowing-software_node-connections-to-sensors-on-platforms-designed-for-Windows/20201130-214014
base: git://linuxtv.org/media_tree.git master
config: ia64-defconfig (attached as .config)
compiler: ia64-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/eb1854ac694a8e59c0ea703e46fe2ee7e3118b42
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Daniel-Scally/Add-functionality-to-ipu3-cio2-driver-allowing-software_node-connections-to-sensors-on-platforms-designed-for-Windows/20201130-214014
git checkout eb1854ac694a8e59c0ea703e46fe2ee7e3118b42
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=ia64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>
All errors (new ones prefixed by >>):
ia64-linux-ld: drivers/usb/core/hub.o: in function `acpi_get_gpiod':
>> (.text+0x3060): multiple definition of `acpi_get_gpiod'; drivers/usb/core/usb.o:(.text+0x2020): first defined here
ia64-linux-ld: drivers/usb/core/hcd.o: in function `acpi_get_gpiod':
(.text+0x7480): multiple definition of `acpi_get_gpiod'; drivers/usb/core/usb.o:(.text+0x2020): first defined here
ia64-linux-ld: drivers/usb/core/message.o: in function `acpi_get_gpiod':
(.text+0x3f20): multiple definition of `acpi_get_gpiod'; drivers/usb/core/usb.o:(.text+0x2020): first defined here
ia64-linux-ld: drivers/usb/core/driver.o: in function `acpi_get_gpiod':
(.text+0x1c00): multiple definition of `acpi_get_gpiod'; drivers/usb/core/usb.o:(.text+0x2020): first defined here
ia64-linux-ld: drivers/usb/core/config.o: in function `acpi_get_gpiod':
config.c:(.text+0x3420): multiple definition of `acpi_get_gpiod'; drivers/usb/core/usb.o:(.text+0x2020): first defined here
ia64-linux-ld: drivers/usb/core/file.o: in function `acpi_get_gpiod':
(.text+0xe00): multiple definition of `acpi_get_gpiod'; drivers/usb/core/usb.o:(.text+0x2020): first defined here
ia64-linux-ld: drivers/usb/core/sysfs.o: in function `acpi_get_gpiod':
sysfs.c:(.text+0x28c0): multiple definition of `acpi_get_gpiod'; drivers/usb/core/usb.o:(.text+0x2020): first defined here
ia64-linux-ld: drivers/usb/core/endpoint.o: in function `acpi_get_gpiod':
endpoint.c:(.text+0x7a0): multiple definition of `acpi_get_gpiod'; drivers/usb/core/usb.o:(.text+0x2020): first defined here
ia64-linux-ld: drivers/usb/core/devio.o: in function `acpi_get_gpiod':
devio.c:(.text+0xcae0): multiple definition of `acpi_get_gpiod'; drivers/usb/core/usb.o:(.text+0x2020): first defined here
ia64-linux-ld: drivers/usb/core/notify.o: in function `acpi_get_gpiod':
(.text+0xc0): multiple definition of `acpi_get_gpiod'; drivers/usb/core/usb.o:(.text+0x2020): first defined here
ia64-linux-ld: drivers/usb/core/generic.o: in function `acpi_get_gpiod':
(.text+0xa60): multiple definition of `acpi_get_gpiod'; drivers/usb/core/usb.o:(.text+0x2020): first defined here
ia64-linux-ld: drivers/usb/core/quirks.o: in function `acpi_get_gpiod':
quirks.c:(.text+0xac0): multiple definition of `acpi_get_gpiod'; drivers/usb/core/usb.o:(.text+0x2020): first defined here
ia64-linux-ld: drivers/usb/core/devices.o: in function `acpi_get_gpiod':
devices.c:(.text+0x1ca0): multiple definition of `acpi_get_gpiod'; drivers/usb/core/usb.o:(.text+0x2020): first defined here
ia64-linux-ld: drivers/usb/core/port.o: in function `acpi_get_gpiod':
port.c:(.text+0xcc0): multiple definition of `acpi_get_gpiod'; drivers/usb/core/usb.o:(.text+0x2020): first defined here
ia64-linux-ld: drivers/usb/core/hcd-pci.o: in function `acpi_get_gpiod':
(.text+0x1240): multiple definition of `acpi_get_gpiod'; drivers/usb/core/usb.o:(.text+0x2020): first defined here
ia64-linux-ld: drivers/usb/core/usb-acpi.o: in function `acpi_get_gpiod':
(.text+0x900): multiple definition of `acpi_get_gpiod'; drivers/usb/core/usb.o:(.text+0x2020): first defined here
--
ia64-linux-ld: drivers/video/fbdev/core/fbmon.o: in function `acpi_get_gpiod':
>> (.text+0x4a0): multiple definition of `acpi_get_gpiod'; drivers/video/fbdev/core/fbmem.o:(.text+0x4b20): first defined here
ia64-linux-ld: drivers/video/fbdev/core/fbcmap.o: in function `acpi_get_gpiod':
(.text+0x960): multiple definition of `acpi_get_gpiod'; drivers/video/fbdev/core/fbmem.o:(.text+0x4b20): first defined here
ia64-linux-ld: drivers/video/fbdev/core/fbsysfs.o: in function `acpi_get_gpiod':
(.text+0x1400): multiple definition of `acpi_get_gpiod'; drivers/video/fbdev/core/fbmem.o:(.text+0x4b20): first defined here
ia64-linux-ld: drivers/video/fbdev/core/modedb.o: in function `acpi_get_gpiod':
(.text+0x22e0): multiple definition of `acpi_get_gpiod'; drivers/video/fbdev/core/fbmem.o:(.text+0x4b20): first defined here
ia64-linux-ld: drivers/video/fbdev/core/fbcvt.o: in function `acpi_get_gpiod':
fbcvt.c:(.text+0x0): multiple definition of `acpi_get_gpiod'; drivers/video/fbdev/core/fbmem.o:(.text+0x4b20): first defined here
ia64-linux-ld: drivers/video/fbdev/core/fb_defio.o: in function `acpi_get_gpiod':
(.text+0xf40): multiple definition of `acpi_get_gpiod'; drivers/video/fbdev/core/fbmem.o:(.text+0x4b20): first defined here
ia64-linux-ld: drivers/video/fbdev/core/fbcon.o: in function `acpi_get_gpiod':
(.text+0xcfa0): multiple definition of `acpi_get_gpiod'; drivers/video/fbdev/core/fbmem.o:(.text+0x4b20): first defined here
ia64-linux-ld: drivers/video/fbdev/core/bitblit.o: in function `acpi_get_gpiod':
(.text+0x29a0): multiple definition of `acpi_get_gpiod'; drivers/video/fbdev/core/fbmem.o:(.text+0x4b20): first defined here
ia64-linux-ld: drivers/video/fbdev/core/softcursor.o: in function `acpi_get_gpiod':
(.text+0x800): multiple definition of `acpi_get_gpiod'; drivers/video/fbdev/core/fbmem.o:(.text+0x4b20): first defined here
--
ia64-linux-ld: drivers/acpi/processor_idle.o: in function `acpi_get_gpiod':
>> processor_idle.c:(.text+0x3000): multiple definition of `acpi_get_gpiod'; drivers/acpi/processor_driver.o:processor_driver.c:(.text+0xd20): first defined here
ia64-linux-ld: drivers/acpi/processor_throttling.o: in function `acpi_get_gpiod':
processor_throttling.c:(.text+0x27e0): multiple definition of `acpi_get_gpiod'; drivers/acpi/processor_driver.o:processor_driver.c:(.text+0xd20): first defined here
ia64-linux-ld: drivers/acpi/processor_thermal.o: in function `acpi_get_gpiod':
processor_thermal.c:(.text+0x2c0): multiple definition of `acpi_get_gpiod'; drivers/acpi/processor_driver.o:processor_driver.c:(.text+0xd20): first defined here
--
ia64-linux-ld: drivers/gpu/drm/drm_crtc_helper.o: in function `acpi_get_gpiod':
>> (.text+0x22e0): multiple definition of `acpi_get_gpiod'; drivers/gpu/drm/drm_bridge_connector.o:(.text+0xb80): first defined here
ia64-linux-ld: drivers/gpu/drm/drm_dp_helper.o: in function `acpi_get_gpiod':
(.text+0x5460): multiple definition of `acpi_get_gpiod'; drivers/gpu/drm/drm_bridge_connector.o:(.text+0xb80): first defined here
ia64-linux-ld: drivers/gpu/drm/drm_dsc.o: in function `acpi_get_gpiod':
(.text+0x1220): multiple definition of `acpi_get_gpiod'; drivers/gpu/drm/drm_bridge_connector.o:(.text+0xb80): first defined here
ia64-linux-ld: drivers/gpu/drm/drm_probe_helper.o: in function `acpi_get_gpiod':
(.text+0x1340): multiple definition of `acpi_get_gpiod'; drivers/gpu/drm/drm_bridge_connector.o:(.text+0xb80): first defined here
ia64-linux-ld: drivers/gpu/drm/drm_plane_helper.o: in function `acpi_get_gpiod':
(.text+0x920): multiple definition of `acpi_get_gpiod'; drivers/gpu/drm/drm_bridge_connector.o:(.text+0xb80): first defined here
ia64-linux-ld: drivers/gpu/drm/drm_dp_mst_topology.o: in function `acpi_get_gpiod':
(.text+0xaa60): multiple definition of `acpi_get_gpiod'; drivers/gpu/drm/drm_bridge_connector.o:(.text+0xb80): first defined here
ia64-linux-ld: drivers/gpu/drm/drm_atomic_helper.o: in function `acpi_get_gpiod':
(.text+0xed00): multiple definition of `acpi_get_gpiod'; drivers/gpu/drm/drm_bridge_connector.o:(.text+0xb80): first defined here
ia64-linux-ld: drivers/gpu/drm/drm_kms_helper_common.o: in function `acpi_get_gpiod':
drm_kms_helper_common.c:(.text+0x0): multiple definition of `acpi_get_gpiod'; drivers/gpu/drm/drm_bridge_connector.o:(.text+0xb80): first defined here
ia64-linux-ld: drivers/gpu/drm/drm_dp_dual_mode_helper.o: in function `acpi_get_gpiod':
(.text+0x1280): multiple definition of `acpi_get_gpiod'; drivers/gpu/drm/drm_bridge_connector.o:(.text+0xb80): first defined here
ia64-linux-ld: drivers/gpu/drm/drm_simple_kms_helper.o: in function `acpi_get_gpiod':
(.text+0xb60): multiple definition of `acpi_get_gpiod'; drivers/gpu/drm/drm_bridge_connector.o:(.text+0xb80): first defined here
ia64-linux-ld: drivers/gpu/drm/drm_modeset_helper.o: in function `acpi_get_gpiod':
(.text+0x8a0): multiple definition of `acpi_get_gpiod'; drivers/gpu/drm/drm_bridge_connector.o:(.text+0xb80): first defined here
ia64-linux-ld: drivers/gpu/drm/drm_scdc_helper.o: in function `acpi_get_gpiod':
(.text+0x620): multiple definition of `acpi_get_gpiod'; drivers/gpu/drm/drm_bridge_connector.o:(.text+0xb80): first defined here
ia64-linux-ld: drivers/gpu/drm/drm_gem_framebuffer_helper.o: in function `acpi_get_gpiod':
(.text+0x11c0): multiple definition of `acpi_get_gpiod'; drivers/gpu/drm/drm_bridge_connector.o:(.text+0xb80): first defined here
ia64-linux-ld: drivers/gpu/drm/drm_atomic_state_helper.o: in function `acpi_get_gpiod':
(.text+0x1b80): multiple definition of `acpi_get_gpiod'; drivers/gpu/drm/drm_bridge_connector.o:(.text+0xb80): first defined here
ia64-linux-ld: drivers/gpu/drm/drm_damage_helper.o: in function `acpi_get_gpiod':
(.text+0x1380): multiple definition of `acpi_get_gpiod'; drivers/gpu/drm/drm_bridge_connector.o:(.text+0xb80): first defined here
ia64-linux-ld: drivers/gpu/drm/drm_self_refresh_helper.o: in function `acpi_get_gpiod':
(.text+0xc00): multiple definition of `acpi_get_gpiod'; drivers/gpu/drm/drm_bridge_connector.o:(.text+0xb80): first defined here
ia64-linux-ld: drivers/gpu/drm/bridge/panel.o: in function `acpi_get_gpiod':
(.text+0xa20): multiple definition of `acpi_get_gpiod'; drivers/gpu/drm/drm_bridge_connector.o:(.text+0xb80): first defined here
ia64-linux-ld: drivers/gpu/drm/drm_fb_helper.o: in function `acpi_get_gpiod':
(.text+0x7d80): multiple definition of `acpi_get_gpiod'; drivers/gpu/drm/drm_bridge_connector.o:(.text+0xb80): first defined here
--
ia64-linux-ld: drivers/gpu/drm/drm_ioctl.o: in function `acpi_get_gpiod':
>> (.text+0x2540): multiple definition of `acpi_get_gpiod'; drivers/gpu/drm/drm_file.o:(.text+0x19c0): first defined here
ia64-linux-ld: drivers/gpu/drm/drm_drv.o: in function `acpi_get_gpiod':
(.text+0x2940): multiple definition of `acpi_get_gpiod'; drivers/gpu/drm/drm_file.o:(.text+0x19c0): first defined here
ia64-linux-ld: drivers/gpu/drm/drm_sysfs.o: in function `acpi_get_gpiod':
(.text+0xe40): multiple definition of `acpi_get_gpiod'; drivers/gpu/drm/drm_file.o:(.text+0x19c0): first defined here
ia64-linux-ld: drivers/gpu/drm/drm_crtc.o: in function `acpi_get_gpiod':
(.text+0x1340): multiple definition of `acpi_get_gpiod'; drivers/gpu/drm/drm_file.o:(.text+0x19c0): first defined here
ia64-linux-ld: drivers/gpu/drm/drm_modes.o: in function `acpi_get_gpiod':
(.text+0x4f60): multiple definition of `acpi_get_gpiod'; drivers/gpu/drm/drm_file.o:(.text+0x19c0): first defined here
ia64-linux-ld: drivers/gpu/drm/drm_edid.o: in function `acpi_get_gpiod':
(.text+0xa7a0): multiple definition of `acpi_get_gpiod'; drivers/gpu/drm/drm_file.o:(.text+0x19c0): first defined here
ia64-linux-ld: drivers/gpu/drm/drm_encoder_slave.o: in function `acpi_get_gpiod':
(.text+0x760): multiple definition of `acpi_get_gpiod'; drivers/gpu/drm/drm_file.o:(.text+0x19c0): first defined here
ia64-linux-ld: drivers/gpu/drm/drm_modeset_lock.o: in function `acpi_get_gpiod':
(.text+0x1a40): multiple definition of `acpi_get_gpiod'; drivers/gpu/drm/drm_file.o:(.text+0x19c0): first defined here
ia64-linux-ld: drivers/gpu/drm/drm_atomic.o: in function `acpi_get_gpiod':
(.text+0x63c0): multiple definition of `acpi_get_gpiod'; drivers/gpu/drm/drm_file.o:(.text+0x19c0): first defined here
ia64-linux-ld: drivers/gpu/drm/drm_bridge.o: in function `acpi_get_gpiod':
(.text+0x2940): multiple definition of `acpi_get_gpiod'; drivers/gpu/drm/drm_file.o:(.text+0x19c0): first defined here
ia64-linux-ld: drivers/gpu/drm/drm_framebuffer.o: in function `acpi_get_gpiod':
(.text+0x1260): multiple definition of `acpi_get_gpiod'; drivers/gpu/drm/drm_file.o:(.text+0x19c0): first defined here
ia64-linux-ld: drivers/gpu/drm/drm_connector.o: in function `acpi_get_gpiod':
(.text+0x48a0): multiple definition of `acpi_get_gpiod'; drivers/gpu/drm/drm_file.o:(.text+0x19c0): first defined here
ia64-linux-ld: drivers/gpu/drm/drm_blend.o: in function `acpi_get_gpiod':
(.text+0x1100): multiple definition of `acpi_get_gpiod'; drivers/gpu/drm/drm_file.o:(.text+0x19c0): first defined here
ia64-linux-ld: drivers/gpu/drm/drm_encoder.o: in function `acpi_get_gpiod':
(.text+0x520): multiple definition of `acpi_get_gpiod'; drivers/gpu/drm/drm_file.o:(.text+0x19c0): first defined here
ia64-linux-ld: drivers/gpu/drm/drm_mode_object.o: in function `acpi_get_gpiod':
(.text+0xb20): multiple definition of `acpi_get_gpiod'; drivers/gpu/drm/drm_file.o:(.text+0x19c0): first defined here
ia64-linux-ld: drivers/gpu/drm/drm_property.o: in function `acpi_get_gpiod':
(.text+0x17a0): multiple definition of `acpi_get_gpiod'; drivers/gpu/drm/drm_file.o:(.text+0x19c0): first defined here
ia64-linux-ld: drivers/gpu/drm/drm_plane.o: in function `acpi_get_gpiod':
(.text+0x1680): multiple definition of `acpi_get_gpiod'; drivers/gpu/drm/drm_file.o:(.text+0x19c0): first defined here
ia64-linux-ld: drivers/gpu/drm/drm_color_mgmt.o: in function `acpi_get_gpiod':
(.text+0xb00): multiple definition of `acpi_get_gpiod'; drivers/gpu/drm/drm_file.o:(.text+0x19c0): first defined here
ia64-linux-ld: drivers/gpu/drm/drm_mode_config.o: in function `acpi_get_gpiod':
(.text+0x1760): multiple definition of `acpi_get_gpiod'; drivers/gpu/drm/drm_file.o:(.text+0x19c0): first defined here
ia64-linux-ld: drivers/gpu/drm/drm_vblank.o: in function `acpi_get_gpiod':
(.text+0x40c0): multiple definition of `acpi_get_gpiod'; drivers/gpu/drm/drm_file.o:(.text+0x19c0): first defined here
ia64-linux-ld: drivers/gpu/drm/drm_lease.o: in function `acpi_get_gpiod':
drm_lease.c:(.text+0x9a0): multiple definition of `acpi_get_gpiod'; drivers/gpu/drm/drm_file.o:(.text+0x19c0): first defined here
ia64-linux-ld: drivers/gpu/drm/drm_writeback.o: in function `acpi_get_gpiod':
(.text+0xec0): multiple definition of `acpi_get_gpiod'; drivers/gpu/drm/drm_file.o:(.text+0x19c0): first defined here
ia64-linux-ld: drivers/gpu/drm/drm_client.o: in function `acpi_get_gpiod':
(.text+0x1200): multiple definition of `acpi_get_gpiod'; drivers/gpu/drm/drm_file.o:(.text+0x19c0): first defined here
ia64-linux-ld: drivers/gpu/drm/drm_client_modeset.o: in function `acpi_get_gpiod':
(.text+0x52a0): multiple definition of `acpi_get_gpiod'; drivers/gpu/drm/drm_file.o:(.text+0x19c0): first defined here
ia64-linux-ld: drivers/gpu/drm/drm_atomic_uapi.o: in function `acpi_get_gpiod':
(.text+0x1120): multiple definition of `acpi_get_gpiod'; drivers/gpu/drm/drm_file.o:(.text+0x19c0): first defined here
ia64-linux-ld: drivers/gpu/drm/drm_vblank_work.o: in function `acpi_get_gpiod':
(.text+0xb20): multiple definition of `acpi_get_gpiod'; drivers/gpu/drm/drm_file.o:(.text+0x19c0): first defined here
ia64-linux-ld: drivers/gpu/drm/drm_panel.o: in function `acpi_get_gpiod':
(.text+0xa40): multiple definition of `acpi_get_gpiod'; drivers/gpu/drm/drm_file.o:(.text+0x19c0): first defined here
--
ia64-linux-ld: drivers/gpu/drm/radeon/radeon_device.o: in function `acpi_get_gpiod':
>> radeon_device.c:(.text+0xe00): multiple definition of `acpi_get_gpiod'; drivers/gpu/drm/radeon/radeon_drv.o:radeon_drv.c:(.text+0xb80): first defined here
ia64-linux-ld: drivers/gpu/drm/radeon/radeon_asic.o: in function `acpi_get_gpiod':
radeon_asic.c:(.text+0x140): multiple definition of `acpi_get_gpiod'; drivers/gpu/drm/radeon/radeon_drv.o:radeon_drv.c:(.text+0xb80): first defined here
ia64-linux-ld: drivers/gpu/drm/radeon/radeon_kms.o: in function `acpi_get_gpiod':
radeon_kms.c:(.text+0x1d80): multiple definition of `acpi_get_gpiod'; drivers/gpu/drm/radeon/radeon_drv.o:radeon_drv.c:(.text+0xb80): first defined here
ia64-linux-ld: drivers/gpu/drm/radeon/radeon_atombios.o: in function `acpi_get_gpiod':
radeon_atombios.c:(.text+0x19c0): multiple definition of `acpi_get_gpiod'; drivers/gpu/drm/radeon/radeon_drv.o:radeon_drv.c:(.text+0xb80): first defined here
ia64-linux-ld: drivers/gpu/drm/radeon/radeon_agp.o: in function `acpi_get_gpiod':
radeon_agp.c:(.text+0x0): multiple definition of `acpi_get_gpiod'; drivers/gpu/drm/radeon/radeon_drv.o:radeon_drv.c:(.text+0xb80): first defined here
ia64-linux-ld: drivers/gpu/drm/radeon/atombios_crtc.o: in function `acpi_get_gpiod':
atombios_crtc.c:(.text+0xb200): multiple definition of `acpi_get_gpiod'; drivers/gpu/drm/radeon/radeon_drv.o:radeon_drv.c:(.text+0xb80): first defined here
ia64-linux-ld: drivers/gpu/drm/radeon/radeon_combios.o: in function `acpi_get_gpiod':
radeon_combios.c:(.text+0x2320): multiple definition of `acpi_get_gpiod'; drivers/gpu/drm/radeon/radeon_drv.o:radeon_drv.c:(.text+0xb80): first defined here
ia64-linux-ld: drivers/gpu/drm/radeon/atom.o: in function `acpi_get_gpiod':
atom.c:(.text+0xb440): multiple definition of `acpi_get_gpiod'; drivers/gpu/drm/radeon/radeon_drv.o:radeon_drv.c:(.text+0xb80): first defined here
ia64-linux-ld: drivers/gpu/drm/radeon/radeon_fence.o: in function `acpi_get_gpiod':
radeon_fence.c:(.text+0x1740): multiple definition of `acpi_get_gpiod'; drivers/gpu/drm/radeon/radeon_drv.o:radeon_drv.c:(.text+0xb80): first defined here
ia64-linux-ld: drivers/gpu/drm/radeon/radeon_ttm.o: in function `acpi_get_gpiod':
radeon_ttm.c:(.text+0x2d20): multiple definition of `acpi_get_gpiod'; drivers/gpu/drm/radeon/radeon_drv.o:radeon_drv.c:(.text+0xb80): first defined here
ia64-linux-ld: drivers/gpu/drm/radeon/radeon_object.o: in function `acpi_get_gpiod':
radeon_object.c:(.text+0x5a0): multiple definition of `acpi_get_gpiod'; drivers/gpu/drm/radeon/radeon_drv.o:radeon_drv.c:(.text+0xb80): first defined here
ia64-linux-ld: drivers/gpu/drm/radeon/radeon_gart.o: in function `acpi_get_gpiod':
radeon_gart.c:(.text+0x0): multiple definition of `acpi_get_gpiod'; drivers/gpu/drm/radeon/radeon_drv.o:radeon_drv.c:(.text+0xb80): first defined here
ia64-linux-ld: drivers/gpu/drm/radeon/radeon_legacy_crtc.o: in function `acpi_get_gpiod':
radeon_legacy_crtc.c:(.text+0x3440): multiple definition of `acpi_get_gpiod'; drivers/gpu/drm/radeon/radeon_drv.o:radeon_drv.c:(.text+0xb80): first defined here
ia64-linux-ld: drivers/gpu/drm/radeon/radeon_legacy_encoders.o: in function `acpi_get_gpiod':
radeon_legacy_encoders.c:(.text+0x7760): multiple definition of `acpi_get_gpiod'; drivers/gpu/drm/radeon/radeon_drv.o:radeon_drv.c:(.text+0xb80): first defined here
ia64-linux-ld: drivers/gpu/drm/radeon/radeon_connectors.o: in function `acpi_get_gpiod':
radeon_connectors.c:(.text+0x6380): multiple definition of `acpi_get_gpiod'; drivers/gpu/drm/radeon/radeon_drv.o:radeon_drv.c:(.text+0xb80): first defined here
ia64-linux-ld: drivers/gpu/drm/radeon/radeon_encoders.o: in function `acpi_get_gpiod':
radeon_encoders.c:(.text+0x0): multiple definition of `acpi_get_gpiod'; drivers/gpu/drm/radeon/radeon_drv.o:radeon_drv.c:(.text+0xb80): first defined here
ia64-linux-ld: drivers/gpu/drm/radeon/radeon_display.o: in function `acpi_get_gpiod':
radeon_display.c:(.text+0x4580): multiple definition of `acpi_get_gpiod'; drivers/gpu/drm/radeon/radeon_drv.o:radeon_drv.c:(.text+0xb80): first defined here
ia64-linux-ld: drivers/gpu/drm/radeon/radeon_cursor.o: in function `acpi_get_gpiod':
radeon_cursor.c:(.text+0x1a80): multiple definition of `acpi_get_gpiod'; drivers/gpu/drm/radeon/radeon_drv.o:radeon_drv.c:(.text+0xb80): first defined here
ia64-linux-ld: drivers/gpu/drm/radeon/radeon_i2c.o: in function `acpi_get_gpiod':
radeon_i2c.c:(.text+0x6060): multiple definition of `acpi_get_gpiod'; drivers/gpu/drm/radeon/radeon_drv.o:radeon_drv.c:(.text+0xb80): first defined here
ia64-linux-ld: drivers/gpu/drm/radeon/radeon_clocks.o: in function `acpi_get_gpiod':
radeon_clocks.c:(.text+0x0): multiple definition of `acpi_get_gpiod'; drivers/gpu/drm/radeon/radeon_drv.o:radeon_drv.c:(.text+0xb80): first defined here
ia64-linux-ld: drivers/gpu/drm/radeon/radeon_fb.o: in function `acpi_get_gpiod':
radeon_fb.c:(.text+0x360): multiple definition of `acpi_get_gpiod'; drivers/gpu/drm/radeon/radeon_drv.o:radeon_drv.c:(.text+0xb80): first defined here
ia64-linux-ld: drivers/gpu/drm/radeon/radeon_gem.o: in function `acpi_get_gpiod':
radeon_gem.c:(.text+0x60): multiple definition of `acpi_get_gpiod'; drivers/gpu/drm/radeon/radeon_drv.o:radeon_drv.c:(.text+0xb80): first defined here
ia64-linux-ld: drivers/gpu/drm/radeon/radeon_ring.o: in function `acpi_get_gpiod':
radeon_ring.c:(.text+0x0): multiple definition of `acpi_get_gpiod'; drivers/gpu/drm/radeon/radeon_drv.o:radeon_drv.c:(.text+0xb80): first defined here
ia64-linux-ld: drivers/gpu/drm/radeon/radeon_irq_kms.o: in function `acpi_get_gpiod':
radeon_irq_kms.c:(.text+0x1e0): multiple definition of `acpi_get_gpiod'; drivers/gpu/drm/radeon/radeon_drv.o:radeon_drv.c:(.text+0xb80): first defined here
ia64-linux-ld: drivers/gpu/drm/radeon/radeon_cs.o: in function `acpi_get_gpiod':
radeon_cs.c:(.text+0x2080): multiple definition of `acpi_get_gpiod'; drivers/gpu/drm/radeon/radeon_drv.o:radeon_drv.c:(.text+0xb80): first defined here
ia64-linux-ld: drivers/gpu/drm/radeon/radeon_bios.o: in function `acpi_get_gpiod':
radeon_bios.c:(.text+0x420): multiple definition of `acpi_get_gpiod'; drivers/gpu/drm/radeon/radeon_drv.o:radeon_drv.c:(.text+0xb80): first defined here
ia64-linux-ld: drivers/gpu/drm/radeon/radeon_benchmark.o: in function `acpi_get_gpiod':
radeon_benchmark.c:(.text+0xf80): multiple definition of `acpi_get_gpiod'; drivers/gpu/drm/radeon/radeon_drv.o:radeon_drv.c:(.text+0xb80): first defined here
ia64-linux-ld: drivers/gpu/drm/radeon/r100.o: in function `acpi_get_gpiod':
r100.c:(.text+0x1640): multiple definition of `acpi_get_gpiod'; drivers/gpu/drm/radeon/radeon_drv.o:radeon_drv.c:(.text+0xb80): first defined here
ia64-linux-ld: drivers/gpu/drm/radeon/r300.o: in function `acpi_get_gpiod':
r300.c:(.text+0x2b00): multiple definition of `acpi_get_gpiod'; drivers/gpu/drm/radeon/radeon_drv.o:radeon_drv.c:(.text+0xb80): first defined here
ia64-linux-ld: drivers/gpu/drm/radeon/r420.o: in function `acpi_get_gpiod':
r420.c:(.text+0x1e0): multiple definition of `acpi_get_gpiod'; drivers/gpu/drm/radeon/radeon_drv.o:radeon_drv.c:(.text+0xb80): first defined here
ia64-linux-ld: drivers/gpu/drm/radeon/rs400.o: in function `acpi_get_gpiod':
rs400.c:(.text+0x0): multiple definition of `acpi_get_gpiod'; drivers/gpu/drm/radeon/radeon_drv.o:radeon_drv.c:(.text+0xb80): first defined here
ia64-linux-ld: drivers/gpu/drm/radeon/rs600.o: in function `acpi_get_gpiod':
rs600.c:(.text+0x320): multiple definition of `acpi_get_gpiod'; drivers/gpu/drm/radeon/radeon_drv.o:radeon_drv.c:(.text+0xb80): first defined here
ia64-linux-ld: drivers/gpu/drm/radeon/rs690.o: in function `acpi_get_gpiod':
rs690.c:(.text+0x1400): multiple definition of `acpi_get_gpiod'; drivers/gpu/drm/radeon/radeon_drv.o:radeon_drv.c:(.text+0xb80): first defined here
ia64-linux-ld: drivers/gpu/drm/radeon/rv515.o: in function `acpi_get_gpiod':
rv515.c:(.text+0x1180): multiple definition of `acpi_get_gpiod'; drivers/gpu/drm/radeon/radeon_drv.o:radeon_drv.c:(.text+0xb80): first defined here
ia64-linux-ld: drivers/gpu/drm/radeon/r520.o: in function `acpi_get_gpiod':
r520.c:(.text+0x0): multiple definition of `acpi_get_gpiod'; drivers/gpu/drm/radeon/radeon_drv.o:radeon_drv.c:(.text+0xb80): first defined here
ia64-linux-ld: drivers/gpu/drm/radeon/r600.o: in function `acpi_get_gpiod':
r600.c:(.text+0x26a0): multiple definition of `acpi_get_gpiod'; drivers/gpu/drm/radeon/radeon_drv.o:radeon_drv.c:(.text+0xb80): first defined here
ia64-linux-ld: drivers/gpu/drm/radeon/rv770.o: in function `acpi_get_gpiod':
rv770.c:(.text+0x3500): multiple definition of `acpi_get_gpiod'; drivers/gpu/drm/radeon/radeon_drv.o:radeon_drv.c:(.text+0xb80): first defined here
ia64-linux-ld: drivers/gpu/drm/radeon/radeon_test.o: in function `acpi_get_gpiod':
radeon_test.c:(.text+0x1a20): multiple definition of `acpi_get_gpiod'; drivers/gpu/drm/radeon/radeon_drv.o:radeon_drv.c:(.text+0xb80): first defined here
ia64-linux-ld: drivers/gpu/drm/radeon/r200.o: in function `acpi_get_gpiod':
r200.c:(.text+0x0): multiple definition of `acpi_get_gpiod'; drivers/gpu/drm/radeon/radeon_drv.o:radeon_drv.c:(.text+0xb80): first defined here
ia64-linux-ld: drivers/gpu/drm/radeon/radeon_legacy_tv.o: in function `acpi_get_gpiod':
radeon_legacy_tv.c:(.text+0x600): multiple definition of `acpi_get_gpiod'; drivers/gpu/drm/radeon/radeon_drv.o:radeon_drv.c:(.text+0xb80): first defined here
ia64-linux-ld: drivers/gpu/drm/radeon/r600_cs.o: in function `acpi_get_gpiod':
r600_cs.c:(.text+0x4bc0): multiple definition of `acpi_get_gpiod'; drivers/gpu/drm/radeon/radeon_drv.o:radeon_drv.c:(.text+0xb80): first defined here
ia64-linux-ld: drivers/gpu/drm/radeon/radeon_pm.o: in function `acpi_get_gpiod':
radeon_pm.c:(.text+0x4660): multiple definition of `acpi_get_gpiod'; drivers/gpu/drm/radeon/radeon_drv.o:radeon_drv.c:(.text+0xb80): first defined here
ia64-linux-ld: drivers/gpu/drm/radeon/atombios_dp.o: in function `acpi_get_gpiod':
atombios_dp.c:(.text+0x13a0): multiple definition of `acpi_get_gpiod'; drivers/gpu/drm/radeon/radeon_drv.o:radeon_drv.c:(.text+0xb80): first defined here
ia64-linux-ld: drivers/gpu/drm/radeon/r600_hdmi.o: in function `acpi_get_gpiod':
r600_hdmi.c:(.text+0x4a0): multiple definition of `acpi_get_gpiod'; drivers/gpu/drm/radeon/radeon_drv.o:radeon_drv.c:(.text+0xb80): first defined here
ia64-linux-ld: drivers/gpu/drm/radeon/dce3_1_afmt.o: in function `acpi_get_gpiod':
dce3_1_afmt.c:(.text+0x0): multiple definition of `acpi_get_gpiod'; drivers/gpu/drm/radeon/radeon_drv.o:radeon_drv.c:(.text+0xb80): first defined here
ia64-linux-ld: drivers/gpu/drm/radeon/evergreen.o: in function `acpi_get_gpiod':
evergreen.c:(.text+0x99a0): multiple definition of `acpi_get_gpiod'; drivers/gpu/drm/radeon/radeon_drv.o:radeon_drv.c:(.text+0xb80): first defined here
ia64-linux-ld: drivers/gpu/drm/radeon/evergreen_cs.o: in function `acpi_get_gpiod':
evergreen_cs.c:(.text+0xe240): multiple definition of `acpi_get_gpiod'; drivers/gpu/drm/radeon/radeon_drv.o:radeon_drv.c:(.text+0xb80): first defined here
ia64-linux-ld: drivers/gpu/drm/radeon/evergreen_hdmi.o: in function `acpi_get_gpiod':
evergreen_hdmi.c:(.text+0x0): multiple definition of `acpi_get_gpiod'; drivers/gpu/drm/radeon/radeon_drv.o:radeon_drv.c:(.text+0xb80): first defined here
ia64-linux-ld: drivers/gpu/drm/radeon/radeon_trace_points.o: in function `acpi_get_gpiod':
radeon_trace_points.c:(.text+0x0): multiple definition of `acpi_get_gpiod'; drivers/gpu/drm/radeon/radeon_drv.o:radeon_drv.c:(.text+0xb80): first defined here
ia64-linux-ld: drivers/gpu/drm/radeon/ni.o: in function `acpi_get_gpiod':
ni.c:(.text+0x85c0): multiple definition of `acpi_get_gpiod'; drivers/gpu/drm/radeon/radeon_drv.o:radeon_drv.c:(.text+0xb80): first defined here
ia64-linux-ld: drivers/gpu/drm/radeon/atombios_encoders.o: in function `acpi_get_gpiod':
atombios_encoders.c:(.text+0x18e0): multiple definition of `acpi_get_gpiod'; drivers/gpu/drm/radeon/radeon_drv.o:radeon_drv.c:(.text+0xb80): first defined here
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]
On Mon, Nov 30, 2020 at 01:31:25PM +0000, Daniel Scally wrote:
> ACPI devices declare themselves dependent on other devices via the _DEP
> buffer. Fetching the dependee from dependent is a matter of parsing
> _DEP, but currently there's no method to fetch dependent from dependee.
> Add one, so we can parse sensors dependent on a PMIC from the PMIC's
> acpi_driver.
Do I understand correctly that it's an existing table provided by firmware that
(ab)uses _DEP in such way? Note, the specification doesn't tell we may use it
in this way, OTOH I don't remember if it strictly forbids such use.
So, please elaborate in the commit message why you need this and pint out to
the 6.5.8 "_DEP (Operation Region Dependencies)" which clearly says about
OpRegions and that part already supported by ACPI in the Linux, if I'm not
mistaken, need to refresh my memory.
...
> + handle = adev->handle;
> +
> + if (!acpi_has_method(handle, "_DEP"))
> + return 0;
> +
> + status = acpi_evaluate_reference(handle, "_DEP", NULL, &dep_handles);
> + if (ACPI_FAILURE(status))
> + return 0;
> +
> + for (i = 0; i < dep_handles.count; i++) {
> + struct acpi_device_info *info;
> +
> + status = acpi_get_object_info(dep_handles.handles[i], &info);
> + if (ACPI_FAILURE(status))
> + continue;
> +
> + if (info->valid & ACPI_VALID_HID) {
> + ret = acpi_bus_get_device(dep_handles.handles[i], &candidate);
> + if (ret || !candidate) {
> + kfree(info);
> + continue;
> + }
> +
> + if (candidate == dependee) {
> + acpi_dev_put(candidate);
> + kfree(info);
> + return 1;
> + }
> +
> + kfree(info);
> + }
> + }
Can you utilize (by moving to here and export for ACPI layer the
acpi_lpss_dep()?
--
With Best Regards,
Andy Shevchenko
Hi Andy,
On Mon, Nov 30, 2020 at 07:53:19PM +0200, Andy Shevchenko wrote:
> On Mon, Nov 30, 2020 at 07:28:57PM +0200, Laurent Pinchart wrote:
> > On Mon, Nov 30, 2020 at 07:29:00PM +0200, Andy Shevchenko wrote:
> > > On Mon, Nov 30, 2020 at 01:31:13PM +0000, Daniel Scally wrote:
>
> ...
>
> > > > + if (!best_ep && fwnode && !IS_ERR_OR_NULL(fwnode->secondary))
> > > > + return fwnode_graph_get_endpoint_by_id(fwnode->secondary, port,
> > > > + endpoint, flags);
> > >
> > > > return best_ep;
> > >
> > > Can we, please, do
> > >
> > > 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;
> > >
> > > ?
> > >
> > > This 'if (fwnode && !IS_ERR_OR_NULL(fwnode->secondary))' becomes kinda
> > > idiomatic to the cases when we need to proceed primary followed by the
> > > secondary in cases where it's not already done.
> >
> > We could also move the !fwnode check to the beginning of the function.
>
> It's already there (1). What did I miss?
It is, but as we need an explicitly check at the end, it feels cleaner
to move it to the beginning. No big deal though.
> 1) via fwnode_graph_get_next_endpoint() -> fwnode_call_ptr_op()
--
Regards,
Laurent Pinchart
Hi Andy,
On Mon, Nov 30, 2020 at 08:23:54PM +0200, Andy Shevchenko wrote:
> On Mon, Nov 30, 2020 at 01:31:25PM +0000, Daniel Scally wrote:
> > ACPI devices declare themselves dependent on other devices via the _DEP
> > buffer. Fetching the dependee from dependent is a matter of parsing
> > _DEP, but currently there's no method to fetch dependent from dependee.
> > Add one, so we can parse sensors dependent on a PMIC from the PMIC's
> > acpi_driver.
>
> Do I understand correctly that it's an existing table provided by firmware that
> (ab)uses _DEP in such way? Note, the specification doesn't tell we may use it
> in this way, OTOH I don't remember if it strictly forbids such use.
The ACPI "bindings" (I come from the DT world, is there a standard term
to describe this in ACPI ?) for the camera in Windows-based Kaby Lake
machines could be used as textbook examples of how to abuse ACPI, in
many different ways :-) I'm sure that applies to ACPI in general
though...
Depending on the device, camera sensors are controlled by a PMIC that
provides regulators, clocks and GPIOs (for the reset and power down
signals), or directly by GPIOs that control discrete regulators and
sensor signals. In the first case an INT3472 device models the
regulator, which can be a TI TPS68470 or a uPI Semi uP6641Q (two
completely different devices with a single HID...). The device model is
specified in the CLDB, a custom data table for INT3472.
In the latter case, Intel has created ACPI bindings for a "discrete
PMIC". It uses an ACPI device object with HID set to INT3472 as well,
also with a CLDB whose type field indicate the PMIC is "discrete". The
ACPI device is only used to reference up to 4 GPIOs (provided by the
Kaby Lake GPIO controller, the LPSS) in the _CRS. There's also a _DSM
that reports, for each GPIO, its function. All this information should
have been stored in the camera sensor ACPI device object, but that would
have been too simple...
In both cases, the PMIC device object is referenced by the _DEP data. We
need to access it to dig up the GPIOs, look up their type, and register
fixed regulators, supply mappings and GPIO mappings for the sensor.
> So, please elaborate in the commit message why you need this and pint out to
> the 6.5.8 "_DEP (Operation Region Dependencies)" which clearly says about
> OpRegions and that part already supported by ACPI in the Linux, if I'm not
> mistaken, need to refresh my memory.
>
> ...
>
> > + handle = adev->handle;
> > +
> > + if (!acpi_has_method(handle, "_DEP"))
> > + return 0;
> > +
> > + status = acpi_evaluate_reference(handle, "_DEP", NULL, &dep_handles);
> > + if (ACPI_FAILURE(status))
> > + return 0;
> > +
> > + for (i = 0; i < dep_handles.count; i++) {
> > + struct acpi_device_info *info;
> > +
> > + status = acpi_get_object_info(dep_handles.handles[i], &info);
> > + if (ACPI_FAILURE(status))
> > + continue;
> > +
> > + if (info->valid & ACPI_VALID_HID) {
> > + ret = acpi_bus_get_device(dep_handles.handles[i], &candidate);
> > + if (ret || !candidate) {
> > + kfree(info);
> > + continue;
> > + }
> > +
> > + if (candidate == dependee) {
> > + acpi_dev_put(candidate);
> > + kfree(info);
> > + return 1;
> > + }
> > +
> > + kfree(info);
> > + }
> > + }
>
> Can you utilize (by moving to here and export for ACPI layer the
> acpi_lpss_dep()?
--
Regards,
Laurent Pinchart
On Mon, Nov 30, 2020 at 07:12:41PM +0200, Laurent Pinchart wrote:
> On Mon, Nov 30, 2020 at 01:31:27PM +0000, Daniel Scally wrote:
> > From: Dan Scally <[email protected]>
> >
> > To make sure the new i2c_acpi_dev_name() always reflects the name of i2c
> > devices sourced from ACPI, use it in i2c_set_dev_name().
> >
> > Signed-off-by: Dan Scally <[email protected]>
>
> I'd squash this with 15/18, which would make it clear there's a memory
> leak :-)
...
> > if (adev) {
> > - dev_set_name(&client->dev, "i2c-%s", acpi_dev_name(adev));
> > + dev_set_name(&client->dev, i2c_acpi_dev_name(adev));
> > return;
But you split pattern used in i2c_dev_set_name().
What you need is to provide something like this
#define I2C_DEV_NAME_FORMAT "i2c-%s"
const char *i2c_acpi_get_dev_name(...)
{
return kasprintf(..., I2C_DEV_NAME_FORMAT, ...);
}
(Possible in the future if anybody needs
const char *i2c_dev_get_name_by_bus_and_addr(int bus, unsigned short addr)
)
And here
- dev_set_name(&client->dev, "i2c-%s", info->dev_name);
+ dev_set_name(&client->dev, I2C_DEV_NAME_FORMAT, info->dev_name);
- dev_set_name(&client->dev, "i2c-%s", acpi_dev_name(adev));
+ dev_set_name(&client->dev, I2C_DEV_NAME_FORMAT, acpi_dev_name(adev));
--
With Best Regards,
Andy Shevchenko;
Hi Laurent
On 30/11/2020 16:12, Laurent Pinchart wrote:
> On Mon, Nov 30, 2020 at 06:11:52PM +0200, Laurent Pinchart wrote:
>> Hi Daniel,
>>
>> Thank you for the patch.
>>
>> On Mon, Nov 30, 2020 at 01:31:15PM +0000, Daniel Scally wrote:
>>> 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 to this function.
>>>
>>> Suggested-by: Andy Shevchenko <[email protected]>
>>> Signed-off-by: Daniel Scally <[email protected]>
>>> ---
>>> Changes since RFC v3:
>>>
>>> Patch introduced
>>>
>>> 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 615a0c93e116..af7930b3679e 100644
>>> --- a/drivers/base/swnode.c
>>> +++ b/drivers/base/swnode.c
>>> @@ -700,14 +700,21 @@ int software_node_register_nodes(const struct software_node *nodes)
>>> int i;
>>>
>>> for (i = 0; nodes[i].name; i++) {
>>> + if (nodes[i].parent)
>>> + if (!software_node_to_swnode(nodes[i].parent)) {
>>> + ret = -EINVAL;
>>> + goto err_unregister_nodes;
>>> + }
>>> +
>>> ret = software_node_register(&nodes[i]);
>>> - if (ret) {
>>> - software_node_unregister_nodes(nodes);
>>> - return ret;
>>> - }
>>> + if (ret)
>>> + goto err_unregister_nodes;
>>> }
>>>
>>> return 0;
>> I'd add a blank line here.
>>
>> Reviewed-by: Laurent Pinchart <[email protected]>
> I spoke a bit too soon. Could you update the documentation of the
> function to explain this new requirement ?
Oops - of course, will do
>>> +err_unregister_nodes:
>>> + software_node_unregister_nodes(nodes);
>>> + return ret;
>>> }
>>> EXPORT_SYMBOL_GPL(software_node_register_nodes);
>>>
>> --
>> Regards,
>>
>> Laurent Pinchart
Hi Laurent
On 30/11/2020 16:25, Laurent Pinchart wrote:
> Hi Daniel and Heikki,
>
> Thank you for the patch.
>
> On Mon, Nov 30, 2020 at 01:31:18PM +0000, Daniel Scally wrote:
>> From: Heikki Krogerus <[email protected]>
>>
>> From: Heikki Krogerus <[email protected]>
>>
> There seems to be one From: line too many. You can drop the one in the
> commit message, git-format-patch will add it automatically.
Ah! Thanks, sorry about that
>> This implements the remaining .graph_* callbacks in the
>> fwnode operations vector for the software nodes. That makes
> s/vector/structure/ ?
Yeah, sure.
>> the fwnode_graph*() functions available in the drivers also
>> when software nodes are used.
>>
>> The implementation tries to mimic the "OF graph" as much as
>> possible, but there is no support for the "reg" device
>> property. The ports will need to have the index in their
>> name which starts with "port" (for example "port0", "port1",
>> ...) and endpoints will use the index of the software node
>> that is given to them during creation. The port nodes can
>> also be grouped under a specially named "ports" subnode,
>> just like in DT, if necessary.
> I'm not very familiar with swnodes, but could we name ports port@n
> instead of portn to mimic OF nodes ?
Yes, I don't see any reason why not.
>
>> 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 since RFC v3:
>> - Simplified software_node_get_next_endpoint() a little
>> - Squared away references in software_node_get_next_endpoint()
>> and swnode_graph_find_next_port(), since they were affected by
>> the change to the earlier patch that had *get_next_child() also
>> put the previous reference.
>> - Dropped Andy's R-b, since the code changed.
>> changes in RFC v3:
>> - removed software_node_device_is_available
>> - moved the change to software_node_get_next_child to a separate
>> patch
>> - switched to use fwnode_handle_put() in graph_get_next_endpoint()
>> instead of software_node_put()
>>
>> changes in RFC v2:
>> - added software_node_device_is_available
>> - altered software_node_get_next_child to get references
>> - altered software_node_get_next_endpoint to release references
>> to ports and avoid passing invalid combinations of swnodes to
>> software_node_get_next_child
>> - altered swnode_graph_find_next_port to release port rather than
>> old
>>
>> drivers/base/swnode.c | 110 +++++++++++++++++++++++++++++++++++++++++-
>> 1 file changed, 109 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
>> index 9bd0bb77ad5b..0c7a8d6b9ea8 100644
>> --- a/drivers/base/swnode.c
>> +++ b/drivers/base/swnode.c
>> @@ -540,6 +540,110 @@ software_node_get_reference_args(const struct fwnode_handle *fwnode,
>> return 0;
>> }
>>
>> +static struct fwnode_handle *
>> +swnode_graph_find_next_port(const struct fwnode_handle *parent,
>> + struct fwnode_handle *port)
>> +{
>> + struct fwnode_handle *old = port;
>> +
>> + while ((port = software_node_get_next_child(parent, old))) {
>> + if (!strncmp(to_swnode(port)->node->name, "port", 4))
>> + return port;
>> + old = port;
>> + }
>> +
>> + return NULL;
>> +}
>> +
>> +static struct fwnode_handle *
>> +software_node_graph_get_next_endpoint(const struct fwnode_handle *fwnode,
>> + struct fwnode_handle *endpoint)
>> +{
>> + struct swnode *swnode = to_swnode(fwnode);
>> + struct fwnode_handle *old = endpoint;
>> + struct fwnode_handle *parent;
>> + 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, old);
>> + if (endpoint) {
>> + fwnode_handle_put(port);
>> + break;
>> + }
>> +
>> + /* No more endpoints for that port, so stop passing old */
>> + old = NULL;
>> + }
>> +
>> + fwnode_handle_put(parent);
>> +
>> + return endpoint;
>> +}
>> +
>> +static struct fwnode_handle *
>> +software_node_graph_get_remote_endpoint(const struct fwnode_handle *fwnode)
>> +{
>> + struct swnode *swnode = to_swnode(fwnode);
>> + const struct software_node_ref_args *ref;
>> + const struct property_entry *prop;
>> +
>> + if (!swnode)
>> + return NULL;
>> +
>> + prop = property_entry_get(swnode->node->properties, "remote-endpoint");
>> + if (!prop || prop->type != DEV_PROP_REF || prop->is_inline)
>> + return NULL;
>> +
>> + ref = prop->pointer;
>> +
>> + return software_node_get(software_node_fwnode(ref[0].node));
>> +}
>> +
>> +static struct fwnode_handle *
>> +software_node_graph_get_port_parent(struct fwnode_handle *fwnode)
>> +{
>> + struct swnode *swnode = to_swnode(fwnode);
>> + struct fwnode_handle *parent;
>> +
>> + if (!strcmp(swnode->parent->node->name, "ports"))
>> + parent = &swnode->parent->parent->fwnode;
>> + else
>> + parent = &swnode->parent->fwnode;
>> +
>> + return software_node_get(parent);
>> +}
>> +
>> +static int
>> +software_node_graph_parse_endpoint(const struct fwnode_handle *fwnode,
>> + struct fwnode_endpoint *endpoint)
>> +{
>> + struct swnode *swnode = to_swnode(fwnode);
>> + int ret;
>> +
>> + ret = kstrtou32(swnode->parent->node->name + 4, 10, &endpoint->port);
> If we use port@, s/4/5/. But I suppose we also want to support the case
> where a single port is used, with its name set to "port" ? The logic
> would then need to be a tad more complex. Not sure if the consistency is
> worth the additional complexity, up to you.
I'm conflicted; consistency is good but in my mind keeping the name as
"port@0" for a single port rather than dropping the suffix is the better
approach anyway.
>> + 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 +655,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,
>> };
>>
>> /* -------------------------------------------------------------------------- */
Hi Andy
On 30/11/2020 18:23, Andy Shevchenko wrote:
> On Mon, Nov 30, 2020 at 01:31:25PM +0000, Daniel Scally wrote:
>> ACPI devices declare themselves dependent on other devices via the _DEP
>> buffer. Fetching the dependee from dependent is a matter of parsing
>> _DEP, but currently there's no method to fetch dependent from dependee.
>> Add one, so we can parse sensors dependent on a PMIC from the PMIC's
>> acpi_driver.
> Do I understand correctly that it's an existing table provided by firmware that
> (ab)uses _DEP in such way? Note, the specification doesn't tell we may use it
> in this way, OTOH I don't remember if it strictly forbids such use.
>
> So, please elaborate in the commit message why you need this and pint out to
> the 6.5.8 "_DEP (Operation Region Dependencies)" which clearly says about
> OpRegions and that part already supported by ACPI in the Linux, if I'm not
> mistaken, need to refresh my memory.
Laurent's reply is good explanation, but for example see my Lenovo Miix
510's DSDT:
https://gist.githubusercontent.com/djrscally/e64d112180517352fa3392878b0f4a7d/raw/88b90b3ea4204fd7845257b6666fdade47cc2981/dsdt.dsl
Search OVTI2680 and OVTI5648 for the cameras. Both are dependent on
IN3472 devices (PMI0 and PMI1) which are the discrete type that we're
attempting to handle here.
>
> ...
>
>> + handle = adev->handle;
>> +
>> + if (!acpi_has_method(handle, "_DEP"))
>> + return 0;
>> +
>> + status = acpi_evaluate_reference(handle, "_DEP", NULL, &dep_handles);
>> + if (ACPI_FAILURE(status))
>> + return 0;
>> +
>> + for (i = 0; i < dep_handles.count; i++) {
>> + struct acpi_device_info *info;
>> +
>> + status = acpi_get_object_info(dep_handles.handles[i], &info);
>> + if (ACPI_FAILURE(status))
>> + continue;
>> +
>> + if (info->valid & ACPI_VALID_HID) {
>> + ret = acpi_bus_get_device(dep_handles.handles[i], &candidate);
>> + if (ret || !candidate) {
>> + kfree(info);
>> + continue;
>> + }
>> +
>> + if (candidate == dependee) {
>> + acpi_dev_put(candidate);
>> + kfree(info);
>> + return 1;
>> + }
>> +
>> + kfree(info);
>> + }
>> + }
> Can you utilize (by moving to here and export for ACPI layer the
> acpi_lpss_dep()?
oooh, yes, I think I can. Thank you!
Hi Daniel,
Thank you for the patch.
On Mon, Nov 30, 2020 at 01:31:13PM +0000, Daniel Scally wrote:
> 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.
>
> Signed-off-by: Daniel Scally <[email protected]>
Reviewed-by: Laurent Pinchart <[email protected]>
> ---
> Changes since RFC v3:
>
> Patch introduced. In discussion in the last submission I noted
> that the CIO2 device doesn't have an ACPI fwnode - that turns
> out to be true for _some_ devices but not others, so we need
> this function to check the secondary too.
>
> drivers/base/property.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/base/property.c b/drivers/base/property.c
> index a5ca2306796f..4ece6b086e36 100644
> --- a/drivers/base/property.c
> +++ b/drivers/base/property.c
> @@ -1162,6 +1162,10 @@ fwnode_graph_get_endpoint_by_id(const struct fwnode_handle *fwnode,
> best_ep_id = fwnode_ep.id;
> }
>
> + if (!best_ep && fwnode && !IS_ERR_OR_NULL(fwnode->secondary))
> + return fwnode_graph_get_endpoint_by_id(fwnode->secondary, port,
> + endpoint, flags);
> +
> return best_ep;
> }
> EXPORT_SYMBOL_GPL(fwnode_graph_get_endpoint_by_id);
--
Regards,
Laurent Pinchart
Hi Daniel,
Thank you for the patch.
On Mon, Nov 30, 2020 at 01:31:22PM +0000, Daniel Scally wrote:
> 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.
>
> Signed-off-by: Daniel Scally <[email protected]>
Reviewed-by: Laurent Pinchart <[email protected]>
> ---
> Changes since RFC 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..6486dbde784f 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.
--
Regards,
Laurent Pinchart
On Mon, Nov 30, 2020 at 07:29:00PM +0200, Andy Shevchenko wrote:
> On Mon, Nov 30, 2020 at 01:31:13PM +0000, Daniel Scally wrote:
> > 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.
>
> One nit below, after addressing:
> Reviewed-by: Andy Shevchenko <[email protected]>
>
> ...
>
> > + if (!best_ep && fwnode && !IS_ERR_OR_NULL(fwnode->secondary))
> > + return fwnode_graph_get_endpoint_by_id(fwnode->secondary, port,
> > + endpoint, flags);
>
> > return best_ep;
>
> Can we, please, do
>
> 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;
>
> ?
>
> This 'if (fwnode && !IS_ERR_OR_NULL(fwnode->secondary))' becomes kinda
> idiomatic to the cases when we need to proceed primary followed by the
> secondary in cases where it's not already done.
We could also move the !fwnode check to the beginning of the function.
> > }
> > EXPORT_SYMBOL_GPL(fwnode_graph_get_endpoint_by_id);
--
Regards,
Laurent Pinchart
On Mon, Nov 30, 2020 at 08:41:41PM +0200, Laurent Pinchart wrote:
> On Mon, Nov 30, 2020 at 07:53:19PM +0200, Andy Shevchenko wrote:
> > On Mon, Nov 30, 2020 at 07:28:57PM +0200, Laurent Pinchart wrote:
> > > On Mon, Nov 30, 2020 at 07:29:00PM +0200, Andy Shevchenko wrote:
...
> > > We could also move the !fwnode check to the beginning of the function.
> >
> > It's already there (1). What did I miss?
>
> It is, but as we need an explicitly check at the end, it feels cleaner
> to move it to the beginning. No big deal though.
I prefer to stick with a pattern I mentioned because we may easily to find and
unify these ones somehow.
> > 1) via fwnode_graph_get_next_endpoint() -> fwnode_call_ptr_op()
--
With Best Regards,
Andy Shevchenko
On Mon, Nov 30, 2020 at 01:31:28PM +0000, Daniel Scally wrote:
> I need to be able to translate GPIO resources in an acpi_device's _CRS
> into gpio_descs. Those are represented in _CRS as a pathname to a GPIO
> device plus the pin's index number: this function is perfect for that
> purpose.
Destiny of this patch depends on the review of the next one.
--
With Best Regards,
Andy Shevchenko
Daniel, thanks for your patch.
On 11/30/20 9:31 PM, Daniel Scally wrote:
> Some types of fwnode_handle do not implement the device_is_available()
> check, such as those created by software_nodes. There isn't really a
> meaningful way to check for the availability of a device that doesn't
> actually exist, so if the check isn't implemented just assume that the
> "device" is present.
>
> Suggested-by: Laurent Pinchart <[email protected]>
> Signed-off-by: Daniel Scally <[email protected]>
> ---
> Changes since RFC v3:
>
> patch introduced
>
> drivers/base/property.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/drivers/base/property.c b/drivers/base/property.c
> index 4c43d30145c6..a5ca2306796f 100644
> --- a/drivers/base/property.c
> +++ b/drivers/base/property.c
> @@ -785,9 +785,14 @@ EXPORT_SYMBOL_GPL(fwnode_handle_put);
> /**
> * fwnode_device_is_available - check if a device is available for use
> * @fwnode: Pointer to the fwnode of the device.
> + *
> + * For fwnode node types that don't implement the .device_is_available()
> + * operation, this function returns true.
> */
> bool fwnode_device_is_available(const struct fwnode_handle *fwnode)
> {
> + if (!fwnode_has_op(fwnode, device_is_available))
> + return true;
blank line here?
> return fwnode_call_bool_op(fwnode, device_is_available);
> }
> EXPORT_SYMBOL_GPL(fwnode_device_is_available);
>
--
Best regards,
Bingbu Cao
I see there will be multiple files, but there will be no conflict if keep as the main
file name unchanged, right? If so, I prefer keep as it was.
On 11/30/20 9:31 PM, Daniel Scally wrote:
> ipu3-cio2 driver needs extending with multiple files; rename the main
> source file and specify the renamed file in Makefile to accommodate that.
>
> Suggested-by: Andy Shevchenko <[email protected]>
> Reviewed-by: Andy Shevchenko <[email protected]>
> Reviewed-by: Laurent Pinchart <[email protected]>
> Signed-off-by: Daniel Scally <[email protected]>
> ---
> Changes since RFC 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
>
--
Best regards,
Bingbu Cao
Hi Bingbu
On 01/12/2020 03:12, Bingbu Cao wrote:
> Daniel, thanks for your patch.
>
> On 11/30/20 9:31 PM, Daniel Scally wrote:
>> Some types of fwnode_handle do not implement the device_is_available()
>> check, such as those created by software_nodes. There isn't really a
>> meaningful way to check for the availability of a device that doesn't
>> actually exist, so if the check isn't implemented just assume that the
>> "device" is present.
>>
>> Suggested-by: Laurent Pinchart <[email protected]>
>> Signed-off-by: Daniel Scally <[email protected]>
>> ---
>> Changes since RFC v3:
>>
>> patch introduced
>>
>> drivers/base/property.c | 5 +++++
>> 1 file changed, 5 insertions(+)
>>
>> diff --git a/drivers/base/property.c b/drivers/base/property.c
>> index 4c43d30145c6..a5ca2306796f 100644
>> --- a/drivers/base/property.c
>> +++ b/drivers/base/property.c
>> @@ -785,9 +785,14 @@ EXPORT_SYMBOL_GPL(fwnode_handle_put);
>> /**
>> * fwnode_device_is_available - check if a device is available for use
>> * @fwnode: Pointer to the fwnode of the device.
>> + *
>> + * For fwnode node types that don't implement the .device_is_available()
>> + * operation, this function returns true.
>> */
>> bool fwnode_device_is_available(const struct fwnode_handle *fwnode)
>> {
>> + if (!fwnode_has_op(fwnode, device_is_available))
>> + return true;
> blank line here?
Sure thing - I'll add one in
>> return fwnode_call_bool_op(fwnode, device_is_available);
>> }
>> EXPORT_SYMBOL_GPL(fwnode_device_is_available);
>>
On Mon, Nov 30, 2020 at 11:54:44PM +0000, Dan Scally wrote:
> Hi Andy
>
> On 30/11/2020 18:23, Andy Shevchenko wrote:
> > On Mon, Nov 30, 2020 at 01:31:25PM +0000, Daniel Scally wrote:
> >> ACPI devices declare themselves dependent on other devices via the _DEP
> >> buffer. Fetching the dependee from dependent is a matter of parsing
> >> _DEP, but currently there's no method to fetch dependent from dependee.
> >> Add one, so we can parse sensors dependent on a PMIC from the PMIC's
> >> acpi_driver.
> > Do I understand correctly that it's an existing table provided by firmware that
> > (ab)uses _DEP in such way? Note, the specification doesn't tell we may use it
> > in this way, OTOH I don't remember if it strictly forbids such use.
> >
> > So, please elaborate in the commit message why you need this and pint out to
> > the 6.5.8 "_DEP (Operation Region Dependencies)" which clearly says about
> > OpRegions and that part already supported by ACPI in the Linux, if I'm not
> > mistaken, need to refresh my memory.
>
>
> Laurent's reply is good explanation, but for example see my Lenovo Miix
> 510's DSDT:
>
>
> https://gist.githubusercontent.com/djrscally/e64d112180517352fa3392878b0f4a7d/raw/88b90b3ea4204fd7845257b6666fdade47cc2981/dsdt.dsl
>
>
> Search OVTI2680 and OVTI5648 for the cameras. Both are dependent on
> IN3472 devices (PMI0 and PMI1) which are the discrete type that we're
> attempting to handle here.
Yes, it seems since PMIC is kinda "power resource" (don't mix with real power
resource as by ACPI specifications) and that's why they decided to include it
into _DEP. So, it seems a de facto common practice. Thus, it would be nice to
have the above in the commit message in some form. Can you do it?
--
With Best Regards,
Andy Shevchenko
Hi Daniel,
url: https://github.com/0day-ci/linux/commits/Daniel-Scally/Add-functionality-to-ipu3-cio2-driver-allowing-software_node-connections-to-sensors-on-platforms-designed-for-Windows/20201130-214014
base: git://linuxtv.org/media_tree.git master
config: i386-randconfig-m021-20201130 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>
Reported-by: Dan Carpenter <[email protected]>
smatch warnings:
drivers/base/swnode.c:785 software_node_unregister_node_group() error: uninitialized symbol 'i'.
vim +/i +785 drivers/base/swnode.c
02094d54870590a Andy Shevchenko 2020-04-08 778 void software_node_unregister_node_group(const struct software_node **node_group)
02094d54870590a Andy Shevchenko 2020-04-08 779 {
02094d54870590a Andy Shevchenko 2020-04-08 780 unsigned int i;
02094d54870590a Andy Shevchenko 2020-04-08 781
02094d54870590a Andy Shevchenko 2020-04-08 782 if (!node_group)
02094d54870590a Andy Shevchenko 2020-04-08 783 return;
02094d54870590a Andy Shevchenko 2020-04-08 784
7c7577c82672f0a Daniel Scally 2020-11-30 @785 while (node_group[i]->name)
^
The "i" is never initialized.
7c7577c82672f0a Daniel Scally 2020-11-30 786 i++;
7c7577c82672f0a Daniel Scally 2020-11-30 787
7c7577c82672f0a Daniel Scally 2020-11-30 788 while (i--)
9dcbac84244f32e Andy Shevchenko 2020-06-22 789 software_node_unregister(node_group[i]);
It's a strange thing that they can only be unregistered in reverse
order... Walter Harms is right when he points out that programmers are
notoriously bad at counting backwards.
02094d54870590a Andy Shevchenko 2020-04-08 790 }
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]
On 12/1/20 2:56 PM, Bingbu Cao wrote:
> I see there will be multiple files, but there will be no conflict if keep as the main
> file name unchanged, right? If so, I prefer keep as it was.
Oops, I notice you try to build all the files into single module, so please ignore my
comment above.
>
> On 11/30/20 9:31 PM, Daniel Scally wrote:
>> ipu3-cio2 driver needs extending with multiple files; rename the main
>> source file and specify the renamed file in Makefile to accommodate that.
>>
>> Suggested-by: Andy Shevchenko <[email protected]>
>> Reviewed-by: Andy Shevchenko <[email protected]>
>> Reviewed-by: Laurent Pinchart <[email protected]>
>> Signed-off-by: Daniel Scally <[email protected]>
>> ---
>> Changes since RFC 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
>>
>
--
Best regards,
Bingbu Cao
On 01/12/2020 15:10, Andy Shevchenko wrote:
> On Mon, Nov 30, 2020 at 11:54:44PM +0000, Dan Scally wrote:
>> Hi Andy
>>
>> On 30/11/2020 18:23, Andy Shevchenko wrote:
>>> On Mon, Nov 30, 2020 at 01:31:25PM +0000, Daniel Scally wrote:
>>>> ACPI devices declare themselves dependent on other devices via the _DEP
>>>> buffer. Fetching the dependee from dependent is a matter of parsing
>>>> _DEP, but currently there's no method to fetch dependent from dependee.
>>>> Add one, so we can parse sensors dependent on a PMIC from the PMIC's
>>>> acpi_driver.
>>> Do I understand correctly that it's an existing table provided by firmware that
>>> (ab)uses _DEP in such way? Note, the specification doesn't tell we may use it
>>> in this way, OTOH I don't remember if it strictly forbids such use.
>>>
>>> So, please elaborate in the commit message why you need this and pint out to
>>> the 6.5.8 "_DEP (Operation Region Dependencies)" which clearly says about
>>> OpRegions and that part already supported by ACPI in the Linux, if I'm not
>>> mistaken, need to refresh my memory.
>>
>> Laurent's reply is good explanation, but for example see my Lenovo Miix
>> 510's DSDT:
>>
>>
>> https://gist.githubusercontent.com/djrscally/e64d112180517352fa3392878b0f4a7d/raw/88b90b3ea4204fd7845257b6666fdade47cc2981/dsdt.dsl
>>
>>
>> Search OVTI2680 and OVTI5648 for the cameras. Both are dependent on
>> IN3472 devices (PMI0 and PMI1) which are the discrete type that we're
>> attempting to handle here.
> Yes, it seems since PMIC is kinda "power resource" (don't mix with real power
> resource as by ACPI specifications) and that's why they decided to include it
> into _DEP. So, it seems a de facto common practice. Thus, it would be nice to
> have the above in the commit message in some form. Can you do it?
>
Sure, no problem. I'll include that in the next version
Hi Andy
On 30/11/2020 17:45, Andy Shevchenko wrote:
> On Mon, Nov 30, 2020 at 01:31:16PM +0000, Daniel Scally wrote:
>> Software nodes that are children of another software node should be
>> unregistered before their parent. To allow easy unregistering of an array
>> of software_nodes ordered parent to child, reverse the order in which
>> this function unregisters software_nodes.
>
> Should be folded in the previous patch. Otherwise we will have a history point
> where register() behaves differently to unregister().
OK sure, I'll squash them - and thanks for your comments on the previous
patch, I condensed the conditionals as you suggest
> ...
>
>> + * @nodes: Zero terminated array of software nodes to be unregistered. 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.
>
> Please, leave field description short. Rather add another note to the
> Description below.
Ack
On 30/11/2020 17:12, Laurent Pinchart wrote:
> Hi Daniel,
>
> Thank you for the patch.
>
> On Mon, Nov 30, 2020 at 01:31:27PM +0000, Daniel Scally wrote:
>> From: Dan Scally <[email protected]>
>>
>> To make sure the new i2c_acpi_dev_name() always reflects the name of i2c
>> devices sourced from ACPI, use it in i2c_set_dev_name().
>>
>> Signed-off-by: Dan Scally <[email protected]>
>
> I'd squash this with 15/18, which would make it clear there's a memory
> leak :-)
Ah - that was sloppy...switched from devm_ and forgot to go fix that.
I'll add the kfree into i2c_unregister_device() and squash to 15/18
>> ---
>> Changes since RFC v3:
>>
>> - Patch introduced
>>
>> drivers/i2c/i2c-core-base.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
>> index 573b5da145d1..a6d4ceb01077 100644
>> --- a/drivers/i2c/i2c-core-base.c
>> +++ b/drivers/i2c/i2c-core-base.c
>> @@ -814,7 +814,7 @@ static void i2c_dev_set_name(struct i2c_adapter *adap,
>> }
>>
>> if (adev) {
>> - dev_set_name(&client->dev, "i2c-%s", acpi_dev_name(adev));
>> + dev_set_name(&client->dev, i2c_acpi_dev_name(adev));
>> return;
>> }
>>
>
Dan, does this mail among other my replies reach you?
It seems you answered to Laurent's mails and leaving mine ignored. Just
wondering if our servers have an issue again...
On Mon, Nov 30, 2020 at 09:18:56PM +0200, Andy Shevchenko wrote:
> On Mon, Nov 30, 2020 at 07:12:41PM +0200, Laurent Pinchart wrote:
> > On Mon, Nov 30, 2020 at 01:31:27PM +0000, Daniel Scally wrote:
> > > From: Dan Scally <[email protected]>
> > >
> > > To make sure the new i2c_acpi_dev_name() always reflects the name of i2c
> > > devices sourced from ACPI, use it in i2c_set_dev_name().
> > >
> > > Signed-off-by: Dan Scally <[email protected]>
> >
> > I'd squash this with 15/18, which would make it clear there's a memory
> > leak :-)
>
> ...
>
> > > if (adev) {
> > > - dev_set_name(&client->dev, "i2c-%s", acpi_dev_name(adev));
> > > + dev_set_name(&client->dev, i2c_acpi_dev_name(adev));
> > > return;
>
> But you split pattern used in i2c_dev_set_name().
> What you need is to provide something like this
>
> #define I2C_DEV_NAME_FORMAT "i2c-%s"
>
> const char *i2c_acpi_get_dev_name(...)
> {
> return kasprintf(..., I2C_DEV_NAME_FORMAT, ...);
> }
>
> (Possible in the future if anybody needs
> const char *i2c_dev_get_name_by_bus_and_addr(int bus, unsigned short addr)
> )
>
> And here
>
> - dev_set_name(&client->dev, "i2c-%s", info->dev_name);
> + dev_set_name(&client->dev, I2C_DEV_NAME_FORMAT, info->dev_name);
>
> - dev_set_name(&client->dev, "i2c-%s", acpi_dev_name(adev));
> + dev_set_name(&client->dev, I2C_DEV_NAME_FORMAT, acpi_dev_name(adev));
>
> --
> With Best Regards,
> Andy Shevchenko;
>
>
--
With Best Regards,
Andy Shevchenko
On 02/12/2020 09:35, Andy Shevchenko wrote:
> Dan, does this mail among other my replies reach you?
> It seems you answered to Laurent's mails and leaving mine ignored. Just
> wondering if our servers have an issue again...
Morning - I got it, sorry. I just read Laurent's first and then called
it a night
> On Mon, Nov 30, 2020 at 09:18:56PM +0200, Andy Shevchenko wrote:
>> On Mon, Nov 30, 2020 at 07:12:41PM +0200, Laurent Pinchart wrote:
>>> On Mon, Nov 30, 2020 at 01:31:27PM +0000, Daniel Scally wrote:
>>>> From: Dan Scally <[email protected]>
>>>>
>>>> To make sure the new i2c_acpi_dev_name() always reflects the name of i2c
>>>> devices sourced from ACPI, use it in i2c_set_dev_name().
>>>>
>>>> Signed-off-by: Dan Scally <[email protected]>
>>> I'd squash this with 15/18, which would make it clear there's a memory
>>> leak :-)
>> ...
>>
>>>> if (adev) {
>>>> - dev_set_name(&client->dev, "i2c-%s", acpi_dev_name(adev));
>>>> + dev_set_name(&client->dev, i2c_acpi_dev_name(adev));
>>>> return;
>> But you split pattern used in i2c_dev_set_name().
>> What you need is to provide something like this
>>
>> #define I2C_DEV_NAME_FORMAT "i2c-%s"
>>
>> const char *i2c_acpi_get_dev_name(...)
>> {
>> return kasprintf(..., I2C_DEV_NAME_FORMAT, ...);
>> }
>>
>> (Possible in the future if anybody needs
>> const char *i2c_dev_get_name_by_bus_and_addr(int bus, unsigned short addr)
>> )
>>
>> And here
>>
>> - dev_set_name(&client->dev, "i2c-%s", info->dev_name);
>> + dev_set_name(&client->dev, I2C_DEV_NAME_FORMAT, info->dev_name);
>>
>> - dev_set_name(&client->dev, "i2c-%s", acpi_dev_name(adev));
>> + dev_set_name(&client->dev, I2C_DEV_NAME_FORMAT, acpi_dev_name(adev));
>>
Yeah ok, I like this approach much better, I'll switch to that.
On 30/11/2020 17:47, Andy Shevchenko wrote:
> On Mon, Nov 30, 2020 at 06:17:16PM +0200, Laurent Pinchart wrote:
>> Hi Daniel,
>>
>> Thank you for the patch.
>>
>> The subject line is very long. We try to keep it within a 72 characters
>> limit in the kernel. That can be a challenge sometimes, and expections
>> can be accepted, but this one is reaaaally long.
>>
>> (The same comment holds for other patches in the series)
>
> +1.
My bad; I'll go through the series and condense them down as much as
possible.
>> On Mon, Nov 30, 2020 at 01:31:17PM +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.
>>>
>>> Suggested-by: Andy Shevchenko <[email protected]>
>>> Signed-off-by: Daniel Scally <[email protected]>
>>
>> I"d squash this with the previous patch to avoid introducing an
>> inconsistency.
>
> It's different to previous. It touches not complementary API, but different
> one. However, I would follow your comment about documenting the behaviour of
> these two APIs as well…
I'll update the documentation for this function too.
On 30/11/2020 17:29, Andy Shevchenko wrote:
> On Mon, Nov 30, 2020 at 01:31:13PM +0000, Daniel Scally wrote:
>> 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.
>
> One nit below, after addressing:
> Reviewed-by: Andy Shevchenko <[email protected]>
>
> ...
>
>> + if (!best_ep && fwnode && !IS_ERR_OR_NULL(fwnode->secondary))
>> + return fwnode_graph_get_endpoint_by_id(fwnode->secondary, port,
>> + endpoint, flags);
>
>> return best_ep;
>
> Can we, please, do
>
> 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;
>
> ?
>
> This 'if (fwnode && !IS_ERR_OR_NULL(fwnode->secondary))' becomes kinda
> idiomatic to the cases when we need to proceed primary followed by the
> secondary in cases where it's not already done.
Thanks - I made this change too
On 30/11/2020 17:11, Laurent Pinchart wrote:
> Hi Daniel,
>
> Thank you for the patch.
>
> On Mon, Nov 30, 2020 at 01:31:26PM +0000, Daniel Scally wrote:
>> Some places in the kernel allow users to map resources to a device
>> using device name (for example, gpiod_lookup_table). Currently
>> this involves waiting for the i2c_client to have been registered so we
>> can use dev_name(&client->dev). Adding this function means that we can
>> achieve the same thing without having to wait to the i2c device.
>>
>> Signed-off-by: Daniel Scally <[email protected]>
>> ---
>> Changes since RFC v3:
>>
>> - Patch introduced
>>
>> drivers/i2c/i2c-core-acpi.c | 14 ++++++++++++++
>> include/linux/i2c.h | 5 +++++
>> 2 files changed, 19 insertions(+)
>>
>> diff --git a/drivers/i2c/i2c-core-acpi.c b/drivers/i2c/i2c-core-acpi.c
>> index 37c510d9347a..d3a653eac79e 100644
>> --- a/drivers/i2c/i2c-core-acpi.c
>> +++ b/drivers/i2c/i2c-core-acpi.c
>> @@ -497,6 +497,20 @@ struct i2c_client *i2c_acpi_new_device(struct device *dev, int index,
>> }
>> EXPORT_SYMBOL_GPL(i2c_acpi_new_device);
>>
>> +/**
>> + * i2c_acpi_dev_name - Construct i2c device name for devs sourced from ACPI
>> + * @adev: ACPI device to construct the name for
>> + *
>> + * Prefixes "i2c-" to the ACPI device name, for use in i2c_dev_set_name() and
>> + * also anywhere else in the kernel that needs to refer to an i2c device by
>> + * name but before they have been instantiated.
>
> The documentation should state that the caller must free the return
> value.
Updated to include that - thanks
>> + */
>> +char *i2c_acpi_dev_name(struct acpi_device *adev)
>> +{
>> + return kasprintf(GFP_KERNEL, "i2c-%s", acpi_dev_name(adev));
>> +}
>> +EXPORT_SYMBOL_GPL(i2c_acpi_dev_name);
>> +
>> #ifdef CONFIG_ACPI_I2C_OPREGION
>> static int acpi_gsb_i2c_read_bytes(struct i2c_client *client,
>> u8 cmd, u8 *data, u8 data_len)
>> diff --git a/include/linux/i2c.h b/include/linux/i2c.h
>> index 56622658b215..ab0e505b2ca6 100644
>> --- a/include/linux/i2c.h
>> +++ b/include/linux/i2c.h
>> @@ -995,6 +995,7 @@ bool i2c_acpi_get_i2c_resource(struct acpi_resource *ares,
>> u32 i2c_acpi_find_bus_speed(struct device *dev);
>> struct i2c_client *i2c_acpi_new_device(struct device *dev, int index,
>> struct i2c_board_info *info);
>> +char *i2c_acpi_dev_name(struct acpi_device *adev);
>> struct i2c_adapter *i2c_acpi_find_adapter_by_handle(acpi_handle handle);
>> #else
>> static inline bool i2c_acpi_get_i2c_resource(struct acpi_resource *ares,
>> @@ -1011,6 +1012,10 @@ static inline struct i2c_client *i2c_acpi_new_device(struct device *dev,
>> {
>> return ERR_PTR(-ENODEV);
>> }
>> +static inline char *i2c_acpi_dev_name(struct acpi_device *adev)
>> +{
>> + return NULL;
>> +}
>> static inline struct i2c_adapter *i2c_acpi_find_adapter_by_handle(acpi_handle handle)
>> {
>> return NULL;
>