[PATCH v3 0/9] introduce fwnode in the I2C subsystem

[PATCH v3 0/9] introduce fwnode in the I2C subsystem

[Clément Léger]

In order to allow the I2C subsystem to be usable with fwnode, add
some functions to retrieve an i2c_adapter from a fwnode and use
these functions in both i2c mux and sfp. ACPI and device-tree are
handled to allow these modifications to work with both descriptions.
I2C mux support has also been modified to support fwnode based
descriptions.

This series is a subset of the one that was first submitted as a larger
series to add swnode support [1]. In this one, it will be focused on
fwnode support only since it seems to have reach a consensus that
adding fwnode to subsystems makes sense.

[1] https://lore.kernel.org/netdev/YhPSkz8+BIcdb72R@smile.fi.intel.com/T/

---------------

Changes in V3:
 - Add index parameter to property_read_string_array()
 - Implement support for devbice-tree and software nodes
 - Restrict index support for ACPI to 0
 - Add unittests for of_property_read_string_array_index()
 - Add unittests for fwnode_property_read_string_index()

Changes in V2:
 - Remove sfp modifications
 - Add property_read_string_index fwnode_operation callback
 - Implement .property_read_string_index for of and swnode
 - Renamed np variable to fwnode

Clément Léger (9):
  of: add of_property_read_string_array_index()
  of: unittests: add tests for of_property_read_string_array_index()
  device property: add index argument to property_read_string_array()
    callback
  device property: add fwnode_property_read_string_index()
  device property: add tests for fwnode_property_read_string_index()
  i2c: fwnode: add fwnode_find_i2c_adapter_by_node()
  i2c: of: use fwnode_get_i2c_adapter_by_node()
  i2c: mux: pinctrl: remove CONFIG_OF dependency and use fwnode API
  i2c: mux: add support for fwnode

 drivers/acpi/property.c                 |  5 ++-
 drivers/base/property.c                 | 37 ++++++++++++++++++--
 drivers/base/swnode.c                   | 21 ++++++++----
 drivers/base/test/property-entry-test.c | 18 ++++++++++
 drivers/i2c/Makefile                    |  1 +
 drivers/i2c/i2c-core-fwnode.c           | 45 +++++++++++++++++++++++++
 drivers/i2c/i2c-core-of.c               | 30 -----------------
 drivers/i2c/i2c-mux.c                   | 39 ++++++++++-----------
 drivers/i2c/muxes/Kconfig               |  1 -
 drivers/i2c/muxes/i2c-mux-pinctrl.c     | 23 +++++++------
 drivers/of/property.c                   |  5 +--
 drivers/of/unittest.c                   | 20 +++++++++++
 include/linux/fwnode.h                  |  7 ++--
 include/linux/i2c.h                     |  8 ++++-
 include/linux/of.h                      | 22 ++++++++++++
 include/linux/property.h                |  3 ++
 16 files changed, 207 insertions(+), 78 deletions(-)
 create mode 100644 drivers/i2c/i2c-core-fwnode.c

-- 
2.34.1

[PATCH v3 1/9] of: add of_property_read_string_array_index()

[Clément Léger]

Add of_property_read_string_array_index() which allows to read a string
array starting at a specific index.

Signed-off-by: Clément Léger <clement.leger@bootlin.com>
---
 include/linux/of.h | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/include/linux/of.h b/include/linux/of.h
index 2dc77430a91a..93f04c530bd1 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -1115,6 +1115,28 @@ static inline int of_property_read_string_array(const struct device_node *np,
 	return of_property_read_string_helper(np, propname, out_strs, sz, 0);
 }
 
+/**
+ * of_property_read_string_array_index() - Read an array of strings from a
+ * multiple strings property starting at a specified index
+ * @np:		device node from which the property value is to be read.
+ * @propname:	name of the property to be searched.
+ * @out_strs:	output array of string pointers.
+ * @sz:		number of array elements to read.
+ * @index:	index to start reading from
+ *
+ * Search for a property in a device tree node and retrieve a list of
+ * terminated string values (pointer to data, not a copy) in that property
+ * starting at specified index.
+ *
+ * Return: If @out_strs is NULL, the number of strings in the property is returned.
+ */
+static inline int of_property_read_string_array_index(const struct device_node *np,
+						      const char *propname,
+						      const char **out_strs,
+						      size_t sz, int index)
+{
+	return of_property_read_string_helper(np, propname, out_strs, sz, index);
+}
 /**
  * of_property_count_strings() - Find and return the number of strings from a
  * multiple strings property.
-- 
2.34.1

[PATCH v3 2/9] of: unittests: add tests for of_property_read_string_array_index()

[Clément Léger]

Add testing for of_property_read_string_array_index() function which
allows to retrieve a string array starting at a specified offset. These
tests are testing some basic use-case for this function.

Signed-off-by: Clément Léger <clement.leger@bootlin.com>
---
 drivers/of/unittest.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
index 2c2fb161b572..d39ec2f5d2c5 100644
--- a/drivers/of/unittest.c
+++ b/drivers/of/unittest.c
@@ -759,6 +759,26 @@ static void __init of_unittest_property_string(void)
 	strings[1] = NULL;
 	rc = of_property_read_string_array(np, "phandle-list-names", strings, 1);
 	unittest(rc == 1 && strings[1] == NULL, "Overwrote end of string array; rc=%i, str='%s'\n", rc, strings[1]);
+
+	/* of_property_read_string_array_index() tests */
+	rc = of_property_read_string_array_index(np, "string-property", strings, 4, 0);
+	unittest(rc == 1, "Incorrect string count; rc=%i\n", rc);
+	rc = of_property_read_string_array_index(np, "string-property", strings, 4, 1);
+	unittest(rc == -ENODATA, "Incorrect return value; rc=%i\n", rc);
+	rc = of_property_read_string_array_index(np, "phandle-list-names", strings, 2, 0);
+	unittest(rc == 2 && !strcmp(strings[0], "first") && !strcmp(strings[1], "second"),
+		 "of_property_read_string_array_index() failure; rc=%i\n", rc);
+	rc = of_property_read_string_array_index(np, "phandle-list-names", strings, 2, 1);
+	unittest(rc == 2 && !strcmp(strings[0], "second") && !strcmp(strings[1], "third"),
+		 "of_property_read_string_array_index() failure; rc=%i\n", rc);
+	rc = of_property_read_string_array_index(np, "phandle-list-names", strings, 4, 1);
+	unittest(rc == 2 && !strcmp(strings[0], "second") && !strcmp(strings[1], "third"),
+		 "of_property_read_string_array_index() failure; rc=%i\n", rc);
+	rc = of_property_read_string_array_index(np, "phandle-list-names", strings, 1, 2);
+	unittest(rc == 1 && !strcmp(strings[0], "third"),
+		 "of_property_read_string_array_index() failure; rc=%i\n", rc);
+	rc = of_property_read_string_array_index(np, "phandle-list-names", strings, 1, 3);
+	unittest(rc == -ENODATA, "Incorrect return value; rc=%i\n", rc);
 }
 
 #define propcmp(p1, p2) (((p1)->length == (p2)->length) && \
-- 
2.34.1

[PATCH v3 3/9] device property: add index argument to property_read_string_array() callback

[Clément Léger]

This will allow to read a string array from a specified offset. Support
for this new parameter has been added in both of through the use of
of_property_read_string_array_index() and in swnode though the existing
property_entry_read_string_array() function. ACPI support has not yet
been added and only index == 0 is accepted.

Signed-off-by: Clément Léger <clement.leger@bootlin.com>
---
 drivers/acpi/property.c |  5 ++++-
 drivers/base/property.c |  4 ++--
 drivers/base/swnode.c   | 21 +++++++++++++++------
 drivers/of/property.c   |  5 +++--
 include/linux/fwnode.h  |  7 ++++---
 5 files changed, 28 insertions(+), 14 deletions(-)

diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
index 12bbfe833609..7847b21c94f7 100644
--- a/drivers/acpi/property.c
+++ b/drivers/acpi/property.c
@@ -1293,8 +1293,11 @@ acpi_fwnode_property_read_int_array(const struct fwnode_handle *fwnode,
 static int
 acpi_fwnode_property_read_string_array(const struct fwnode_handle *fwnode,
 				       const char *propname, const char **val,
-				       size_t nval)
+				       size_t nval, int index)
 {
+	if (index != 0)
+		return -EINVAL;
+
 	return acpi_node_prop_read(fwnode, propname, DEV_PROP_STRING,
 				   val, nval);
 }
diff --git a/drivers/base/property.c b/drivers/base/property.c
index e6497f6877ee..d95c949e0a79 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -372,12 +372,12 @@ int fwnode_property_read_string_array(const struct fwnode_handle *fwnode,
 	int ret;
 
 	ret = fwnode_call_int_op(fwnode, property_read_string_array, propname,
-				 val, nval);
+				 val, nval, 0);
 	if (ret == -EINVAL && !IS_ERR_OR_NULL(fwnode) &&
 	    !IS_ERR_OR_NULL(fwnode->secondary))
 		ret = fwnode_call_int_op(fwnode->secondary,
 					 property_read_string_array, propname,
-					 val, nval);
+					 val, nval, 0);
 	return ret;
 }
 EXPORT_SYMBOL_GPL(fwnode_property_read_string_array);
diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
index 0a482212c7e8..cb80dab041ef 100644
--- a/drivers/base/swnode.c
+++ b/drivers/base/swnode.c
@@ -184,9 +184,10 @@ static int property_entry_read_int_array(const struct property_entry *props,
 
 static int property_entry_read_string_array(const struct property_entry *props,
 					    const char *propname,
-					    const char **strings, size_t nval)
+					    const char **strings, size_t nval,
+					    int index)
 {
-	const void *pointer;
+	const char * const *pointer;
 	size_t length;
 	int array_len;
 
@@ -200,13 +201,20 @@ static int property_entry_read_string_array(const struct property_entry *props,
 	if (!strings)
 		return array_len;
 
-	array_len = min_t(size_t, nval, array_len);
 	length = array_len * sizeof(*strings);
-
 	pointer = property_entry_find(props, propname, length);
 	if (IS_ERR(pointer))
 		return PTR_ERR(pointer);
 
+	if (index >= array_len)
+		return -ENODATA;
+
+	pointer += index;
+	array_len -= index;
+
+	array_len = min_t(size_t, nval, array_len);
+	length = array_len * sizeof(*strings);
+
 	memcpy(strings, pointer, length);
 
 	return array_len;
@@ -400,12 +408,13 @@ static int software_node_read_int_array(const struct fwnode_handle *fwnode,
 
 static int software_node_read_string_array(const struct fwnode_handle *fwnode,
 					   const char *propname,
-					   const char **val, size_t nval)
+					   const char **val, size_t nval,
+					   int index)
 {
 	struct swnode *swnode = to_swnode(fwnode);
 
 	return property_entry_read_string_array(swnode->node->properties,
-						propname, val, nval);
+						propname, val, nval, index);
 }
 
 static const char *
diff --git a/drivers/of/property.c b/drivers/of/property.c
index 8e90071de6ed..77e8df4a6267 100644
--- a/drivers/of/property.c
+++ b/drivers/of/property.c
@@ -906,12 +906,13 @@ static int of_fwnode_property_read_int_array(const struct fwnode_handle *fwnode,
 static int
 of_fwnode_property_read_string_array(const struct fwnode_handle *fwnode,
 				     const char *propname, const char **val,
-				     size_t nval)
+				     size_t nval, int index)
 {
 	const struct device_node *node = to_of_node(fwnode);
 
 	return val ?
-		of_property_read_string_array(node, propname, val, nval) :
+		of_property_read_string_array_index(node, propname, val, nval,
+						    index) :
 		of_property_count_strings(node, propname);
 }
 
diff --git a/include/linux/fwnode.h b/include/linux/fwnode.h
index 3a532ba66f6c..b9e28ba49038 100644
--- a/include/linux/fwnode.h
+++ b/include/linux/fwnode.h
@@ -91,8 +91,9 @@ struct fwnode_reference_args {
  * @property_present: Return true if a property is present.
  * @property_read_int_array: Read an array of integer properties. Return zero on
  *			     success, a negative error code otherwise.
- * @property_read_string_array: Read an array of string properties. Return zero
- *				on success, a negative error code otherwise.
+ * @property_read_string_array: Read an array of string properties starting at
+ *				a specified index. Return zero on success, a
+ *				negative error code otherwise.
  * @get_name: Return the name of an fwnode.
  * @get_name_prefix: Get a prefix for a node (for printing purposes).
  * @get_parent: Return the parent of an fwnode.
@@ -122,7 +123,7 @@ struct fwnode_operations {
 	int
 	(*property_read_string_array)(const struct fwnode_handle *fwnode_handle,
 				      const char *propname, const char **val,
-				      size_t nval);
+				      size_t nval, int index);
 	const char *(*get_name)(const struct fwnode_handle *fwnode);
 	const char *(*get_name_prefix)(const struct fwnode_handle *fwnode);
 	struct fwnode_handle *(*get_parent)(const struct fwnode_handle *fwnode);
-- 
2.34.1

[PATCH v3 4/9] device property: add fwnode_property_read_string_index()

[Clément Léger]

Add fwnode_property_read_string_index() function which allows to
retrieve a single string from an array by its index. This function is
the equivalent of of_property_read_string_index() but for fwnode
support.

Signed-off-by: Clément Léger <clement.leger@bootlin.com>
---
 drivers/base/property.c  | 33 +++++++++++++++++++++++++++++++++
 include/linux/property.h |  3 +++
 2 files changed, 36 insertions(+)

diff --git a/drivers/base/property.c b/drivers/base/property.c
index d95c949e0a79..a2cfdf57b847 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -406,6 +406,39 @@ int fwnode_property_read_string(const struct fwnode_handle *fwnode,
 }
 EXPORT_SYMBOL_GPL(fwnode_property_read_string);
 
+/**
+ * fwnode_property_read_string_index - read a string in an array using an index
+ * @fwnode: Firmware node to get the property of
+ * @propname: Name of the property holding the array
+ * @index: Index of the string to look for
+ * @string: Pointer to the string if found
+ *
+ * Find a string by a given index in a string array and if it is found return
+ * the string value in @string.
+ *
+ * Return: %0 if the string was found (success),
+ *	   %-EINVAL if given arguments are not valid,
+ *	   %-ENODATA if the property does not have a value,
+ *	   %-EPROTO if the property is not an array of strings,
+ *	   %-ENXIO if no suitable firmware interface is present.
+ */
+int fwnode_property_read_string_index(const struct fwnode_handle *fwnode,
+				      const char *propname, int index,
+				      const char **string)
+{
+	int ret;
+
+	ret = fwnode_call_int_op(fwnode, property_read_string_array, propname,
+				 string, 1, index);
+	if (ret == -EINVAL && !IS_ERR_OR_NULL(fwnode) &&
+	    !IS_ERR_OR_NULL(fwnode->secondary))
+		ret = fwnode_call_int_op(fwnode->secondary,
+					 property_read_string_array, propname,
+					 string, 1, index);
+	return ret == 1 ? 0 : ret;
+}
+EXPORT_SYMBOL_GPL(fwnode_property_read_string_index);
+
 /**
  * fwnode_property_match_string - find a string in an array and return index
  * @fwnode: Firmware node to get the property of
diff --git a/include/linux/property.h b/include/linux/property.h
index 7399a0b45f98..a033920eb10a 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -70,6 +70,9 @@ int fwnode_property_read_string_array(const struct fwnode_handle *fwnode,
 				      size_t nval);
 int fwnode_property_read_string(const struct fwnode_handle *fwnode,
 				const char *propname, const char **val);
+int fwnode_property_read_string_index(const struct fwnode_handle *fwnode,
+				      const char *propname, int index,
+				      const char **string);
 int fwnode_property_match_string(const struct fwnode_handle *fwnode,
 				 const char *propname, const char *string);
 int fwnode_property_get_reference_args(const struct fwnode_handle *fwnode,
-- 
2.34.1

[PATCH v3 5/9] device property: add tests for fwnode_property_read_string_index()

[Clément Léger]

Add somes tests to validate fwnode_property_read_string_index().

Signed-off-by: Clément Léger <clement.leger@bootlin.com>
---
 drivers/base/test/property-entry-test.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/drivers/base/test/property-entry-test.c b/drivers/base/test/property-entry-test.c
index 6071d5bc128c..9edbaa53a950 100644
--- a/drivers/base/test/property-entry-test.c
+++ b/drivers/base/test/property-entry-test.c
@@ -318,6 +318,24 @@ static void pe_test_strings(struct kunit *test)
 	KUNIT_EXPECT_EQ(test, error, 0);
 	KUNIT_EXPECT_STREQ(test, str, "string-a");
 
+	error = fwnode_property_read_string_index(node, "str", 0, strs);
+	KUNIT_EXPECT_EQ(test, error, 0);
+	KUNIT_EXPECT_STREQ(test, strs[0], "single");
+
+	error = fwnode_property_read_string_index(node, "strs", 0, strs);
+	KUNIT_EXPECT_EQ(test, error, 0);
+	KUNIT_EXPECT_STREQ(test, strs[0], "string-a");
+
+	error = fwnode_property_read_string_index(node, "strs", 1, strs);
+	KUNIT_EXPECT_EQ(test, error, 0);
+	KUNIT_EXPECT_STREQ(test, strs[0], "string-b");
+
+	error = fwnode_property_read_string_index(node, "str", 1, strs);
+	KUNIT_EXPECT_EQ(test, error, -ENODATA);
+
+	error = fwnode_property_read_string_index(node, "strs", 3, strs);
+	KUNIT_EXPECT_EQ(test, error, -ENODATA);
+
 	fwnode_remove_software_node(node);
 }
 
-- 
2.34.1

[PATCH v3 6/9] i2c: fwnode: add fwnode_find_i2c_adapter_by_node()

[Clément Léger]

Add fwnode_find_i2c_adapter_by_node() which allows to retrieve a i2c
adapter using a fwnode. Since dev_fwnode() uses the fwnode provided by
the of_node member of the device, this will also work for devices were
the of_node has been set and not the fwnode field.
For acpi nodes, the check for parent node is skipped since
i2c_acpi_find_adapter_by_handle() does not check it and we don't want
to change this behavior.

Signed-off-by: Clément Léger <clement.leger@bootlin.com>
---
 drivers/i2c/Makefile          |  1 +
 drivers/i2c/i2c-core-fwnode.c | 45 +++++++++++++++++++++++++++++++++++
 include/linux/i2c.h           |  3 +++
 3 files changed, 49 insertions(+)
 create mode 100644 drivers/i2c/i2c-core-fwnode.c

diff --git a/drivers/i2c/Makefile b/drivers/i2c/Makefile
index c1d493dc9bac..c9c97675163e 100644
--- a/drivers/i2c/Makefile
+++ b/drivers/i2c/Makefile
@@ -6,6 +6,7 @@
 obj-$(CONFIG_I2C_BOARDINFO)	+= i2c-boardinfo.o
 obj-$(CONFIG_I2C)		+= i2c-core.o
 i2c-core-objs 			:= i2c-core-base.o i2c-core-smbus.o
+i2c-core-objs			+= i2c-core-fwnode.o
 i2c-core-$(CONFIG_ACPI)		+= i2c-core-acpi.o
 i2c-core-$(CONFIG_I2C_SLAVE) 	+= i2c-core-slave.o
 i2c-core-$(CONFIG_OF) 		+= i2c-core-of.o
diff --git a/drivers/i2c/i2c-core-fwnode.c b/drivers/i2c/i2c-core-fwnode.c
new file mode 100644
index 000000000000..fb1c820b686e
--- /dev/null
+++ b/drivers/i2c/i2c-core-fwnode.c
@@ -0,0 +1,45 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Linux I2C core fwnode support code
+ *
+ * Copyright (C) 2022 Microchip
+ */
+
+#include <linux/device.h>
+#include <linux/i2c.h>
+
+#include "i2c-core.h"
+
+static int fwnode_dev_or_parent_node_match(struct device *dev, const void *data)
+{
+	if (device_match_fwnode(dev, data))
+		return 1;
+
+	/*
+	 * For ACPI device node, the behavior is to not match the parent (see
+	 * i2c_acpi_find_adapter_by_handle())
+	 */
+	if (!is_acpi_device_node(dev_fwnode(dev)) && dev->parent)
+		return device_match_fwnode(dev->parent, data);
+
+	return 0;
+}
+
+struct i2c_adapter *
+fwnode_find_i2c_adapter_by_node(struct fwnode_handle *fwnode)
+{
+	struct device *dev;
+	struct i2c_adapter *adapter;
+
+	dev = bus_find_device(&i2c_bus_type, NULL, fwnode,
+			      fwnode_dev_or_parent_node_match);
+	if (!dev)
+		return NULL;
+
+	adapter = i2c_verify_adapter(dev);
+	if (!adapter)
+		put_device(dev);
+
+	return adapter;
+}
+EXPORT_SYMBOL(fwnode_find_i2c_adapter_by_node);
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 7d4f52ceb7b5..8dadf8c89fd9 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -967,6 +967,9 @@ int i2c_handle_smbus_host_notify(struct i2c_adapter *adap, unsigned short addr);
 
 #endif /* I2C */
 
+struct i2c_adapter *
+fwnode_find_i2c_adapter_by_node(struct fwnode_handle *fwnode);
+
 #if IS_ENABLED(CONFIG_OF)
 /* must call put_device() when done with returned i2c_client device */
 struct i2c_client *of_find_i2c_device_by_node(struct device_node *node);
-- 
2.34.1

[PATCH v3 7/9] i2c: of: use fwnode_get_i2c_adapter_by_node()

[Clément Léger]

Since the new fwnode based fwnode_find_i2c_adapter_by_node() function
has the same behavior than of_get_i2c_adapter_by_node(), call it to
avoid code duplication.

Signed-off-by: Clément Léger <clement.leger@bootlin.com>
---
 drivers/i2c/i2c-core-of.c | 30 ------------------------------
 include/linux/i2c.h       |  5 ++++-
 2 files changed, 4 insertions(+), 31 deletions(-)

diff --git a/drivers/i2c/i2c-core-of.c b/drivers/i2c/i2c-core-of.c
index 3ed74aa4b44b..be7d66aa0f49 100644
--- a/drivers/i2c/i2c-core-of.c
+++ b/drivers/i2c/i2c-core-of.c
@@ -113,17 +113,6 @@ void of_i2c_register_devices(struct i2c_adapter *adap)
 	of_node_put(bus);
 }
 
-static int of_dev_or_parent_node_match(struct device *dev, const void *data)
-{
-	if (dev->of_node == data)
-		return 1;
-
-	if (dev->parent)
-		return dev->parent->of_node == data;
-
-	return 0;
-}
-
 /* must call put_device() when done with returned i2c_client device */
 struct i2c_client *of_find_i2c_device_by_node(struct device_node *node)
 {
@@ -142,25 +131,6 @@ struct i2c_client *of_find_i2c_device_by_node(struct device_node *node)
 }
 EXPORT_SYMBOL(of_find_i2c_device_by_node);
 
-/* must call put_device() when done with returned i2c_adapter device */
-struct i2c_adapter *of_find_i2c_adapter_by_node(struct device_node *node)
-{
-	struct device *dev;
-	struct i2c_adapter *adapter;
-
-	dev = bus_find_device(&i2c_bus_type, NULL, node,
-			      of_dev_or_parent_node_match);
-	if (!dev)
-		return NULL;
-
-	adapter = i2c_verify_adapter(dev);
-	if (!adapter)
-		put_device(dev);
-
-	return adapter;
-}
-EXPORT_SYMBOL(of_find_i2c_adapter_by_node);
-
 /* must call i2c_put_adapter() when done with returned i2c_adapter device */
 struct i2c_adapter *of_get_i2c_adapter_by_node(struct device_node *node)
 {
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 8dadf8c89fd9..982918fd0093 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -975,7 +975,10 @@ fwnode_find_i2c_adapter_by_node(struct fwnode_handle *fwnode);
 struct i2c_client *of_find_i2c_device_by_node(struct device_node *node);
 
 /* must call put_device() when done with returned i2c_adapter device */
-struct i2c_adapter *of_find_i2c_adapter_by_node(struct device_node *node);
+static inline struct i2c_adapter *of_find_i2c_adapter_by_node(struct device_node *node)
+{
+	return fwnode_find_i2c_adapter_by_node(of_fwnode_handle(node));
+}
 
 /* must call i2c_put_adapter() when done with returned i2c_adapter device */
 struct i2c_adapter *of_get_i2c_adapter_by_node(struct device_node *node);
-- 
2.34.1

[PATCH v3 8/9] i2c: mux: pinctrl: remove CONFIG_OF dependency and use fwnode API

[Clément Léger]

In order to use i2c muxes with all types of nodes, switch to fwnode
API. The fwnode layer will allow to use this with both device_node and
software_node.

This commits is simply replacing the use of "of_" prefixed functions
with there fwnode equivalent.

Signed-off-by: Clément Léger <clement.leger@bootlin.com>
---
 drivers/i2c/muxes/Kconfig           |  1 -
 drivers/i2c/muxes/i2c-mux-pinctrl.c | 23 ++++++++++++-----------
 2 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/i2c/muxes/Kconfig b/drivers/i2c/muxes/Kconfig
index 1708b1a82da2..d9cb15cfba3e 100644
--- a/drivers/i2c/muxes/Kconfig
+++ b/drivers/i2c/muxes/Kconfig
@@ -77,7 +77,6 @@ config I2C_MUX_PCA954x
 config I2C_MUX_PINCTRL
 	tristate "pinctrl-based I2C multiplexer"
 	depends on PINCTRL
-	depends on OF || COMPILE_TEST
 	help
 	  If you say yes to this option, support will be included for an I2C
 	  multiplexer that uses the pinctrl subsystem, i.e. pin multiplexing.
diff --git a/drivers/i2c/muxes/i2c-mux-pinctrl.c b/drivers/i2c/muxes/i2c-mux-pinctrl.c
index f1bb00a11ad6..d9c0241e8790 100644
--- a/drivers/i2c/muxes/i2c-mux-pinctrl.c
+++ b/drivers/i2c/muxes/i2c-mux-pinctrl.c
@@ -53,19 +53,20 @@ static struct i2c_adapter *i2c_mux_pinctrl_root_adapter(
 
 static struct i2c_adapter *i2c_mux_pinctrl_parent_adapter(struct device *dev)
 {
-	struct device_node *np = dev->of_node;
-	struct device_node *parent_np;
+	struct fwnode_handle *fwnode = dev_fwnode(dev);
+	struct fwnode_handle *parent_fwnode;
 	struct i2c_adapter *parent;
 
-	parent_np = of_parse_phandle(np, "i2c-parent", 0);
-	if (!parent_np) {
+	parent_fwnode = fwnode_find_reference(fwnode, "i2c-parent", 0);
+	if (!parent_fwnode) {
 		dev_err(dev, "Cannot parse i2c-parent\n");
 		return ERR_PTR(-ENODEV);
 	}
-	parent = of_find_i2c_adapter_by_node(parent_np);
-	of_node_put(parent_np);
-	if (!parent)
+	parent = fwnode_find_i2c_adapter_by_node(parent_fwnode);
+	if (!parent) {
+		dev_err(dev, "Cannot find i2c-parent\n");
 		return ERR_PTR(-EPROBE_DEFER);
+	}
 
 	return parent;
 }
@@ -73,7 +74,7 @@ static struct i2c_adapter *i2c_mux_pinctrl_parent_adapter(struct device *dev)
 static int i2c_mux_pinctrl_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
-	struct device_node *np = dev->of_node;
+	struct fwnode_handle *fwnode = dev_fwnode(dev);
 	struct i2c_mux_core *muxc;
 	struct i2c_mux_pinctrl *mux;
 	struct i2c_adapter *parent;
@@ -81,7 +82,7 @@ static int i2c_mux_pinctrl_probe(struct platform_device *pdev)
 	int num_names, i, ret;
 	const char *name;
 
-	num_names = of_property_count_strings(np, "pinctrl-names");
+	num_names = fwnode_property_string_array_count(fwnode, "pinctrl-names");
 	if (num_names < 0) {
 		dev_err(dev, "Cannot parse pinctrl-names: %d\n",
 			num_names);
@@ -111,8 +112,8 @@ static int i2c_mux_pinctrl_probe(struct platform_device *pdev)
 	}
 
 	for (i = 0; i < num_names; i++) {
-		ret = of_property_read_string_index(np, "pinctrl-names", i,
-						    &name);
+		ret = fwnode_property_read_string_index(fwnode, "pinctrl-names", i,
+							&name);
 		if (ret < 0) {
 			dev_err(dev, "Cannot parse pinctrl-names: %d\n", ret);
 			goto err_put_parent;
-- 
2.34.1

[PATCH v3 9/9] i2c: mux: add support for fwnode

[Clément Léger]

Modify i2c_mux_add_adapter() to use the fwnode API to create mux
adapters with fwnode based devices. This allows to have a node agnostic
support for i2c muxes.

Signed-off-by: Clément Léger <clement.leger@bootlin.com>
---
 drivers/i2c/i2c-mux.c | 39 ++++++++++++++++++---------------------
 1 file changed, 18 insertions(+), 21 deletions(-)

diff --git a/drivers/i2c/i2c-mux.c b/drivers/i2c/i2c-mux.c
index 774507b54b57..98d735349bd6 100644
--- a/drivers/i2c/i2c-mux.c
+++ b/drivers/i2c/i2c-mux.c
@@ -24,7 +24,7 @@
 #include <linux/i2c-mux.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
-#include <linux/of.h>
+#include <linux/property.h>
 #include <linux/slab.h>
 #include <linux/sysfs.h>
 
@@ -347,38 +347,35 @@ int i2c_mux_add_adapter(struct i2c_mux_core *muxc,
 	else
 		priv->adap.class = class;
 
-	/*
-	 * Try to populate the mux adapter's of_node, expands to
-	 * nothing if !CONFIG_OF.
-	 */
-	if (muxc->dev->of_node) {
-		struct device_node *dev_node = muxc->dev->of_node;
-		struct device_node *mux_node, *child = NULL;
+	/* Try to populate the mux adapter's device node */
+	if (dev_fwnode(muxc->dev) && !has_acpi_companion(muxc->dev)) {
+		struct fwnode_handle *dev_node = dev_fwnode(muxc->dev);
+		struct fwnode_handle *mux_node, *child = NULL;
 		u32 reg;
 
 		if (muxc->arbitrator)
-			mux_node = of_get_child_by_name(dev_node, "i2c-arb");
+			mux_node = fwnode_get_named_child_node(dev_node, "i2c-arb");
 		else if (muxc->gate)
-			mux_node = of_get_child_by_name(dev_node, "i2c-gate");
+			mux_node = fwnode_get_named_child_node(dev_node, "i2c-gate");
 		else
-			mux_node = of_get_child_by_name(dev_node, "i2c-mux");
+			mux_node = fwnode_get_named_child_node(dev_node, "i2c-mux");
 
 		if (mux_node) {
 			/* A "reg" property indicates an old-style DT entry */
-			if (!of_property_read_u32(mux_node, "reg", &reg)) {
-				of_node_put(mux_node);
+			if (!fwnode_property_read_u32(mux_node, "reg", &reg)) {
+				fwnode_handle_put(mux_node);
 				mux_node = NULL;
 			}
 		}
 
 		if (!mux_node)
-			mux_node = of_node_get(dev_node);
+			mux_node = fwnode_handle_get(dev_node);
 		else if (muxc->arbitrator || muxc->gate)
-			child = of_node_get(mux_node);
+			child = fwnode_handle_get(mux_node);
 
 		if (!child) {
-			for_each_child_of_node(mux_node, child) {
-				ret = of_property_read_u32(child, "reg", &reg);
+			fwnode_for_each_child_node(mux_node, child) {
+				ret = fwnode_property_read_u32(child, "reg", &reg);
 				if (ret)
 					continue;
 				if (chan_id == reg)
@@ -386,8 +383,8 @@ int i2c_mux_add_adapter(struct i2c_mux_core *muxc,
 			}
 		}
 
-		priv->adap.dev.of_node = child;
-		of_node_put(mux_node);
+		device_set_node(&priv->adap.dev, child);
+		fwnode_handle_put(mux_node);
 	}
 
 	/*
@@ -444,7 +441,7 @@ void i2c_mux_del_adapters(struct i2c_mux_core *muxc)
 	while (muxc->num_adapters) {
 		struct i2c_adapter *adap = muxc->adapter[--muxc->num_adapters];
 		struct i2c_mux_priv *priv = adap->algo_data;
-		struct device_node *np = adap->dev.of_node;
+		struct fwnode_handle *fwnode = dev_fwnode(&adap->dev);
 
 		muxc->adapter[muxc->num_adapters] = NULL;
 
@@ -454,7 +451,7 @@ void i2c_mux_del_adapters(struct i2c_mux_core *muxc)
 
 		sysfs_remove_link(&priv->adap.dev.kobj, "mux_device");
 		i2c_del_adapter(adap);
-		of_node_put(np);
+		fwnode_handle_put(fwnode);
 		kfree(priv);
 	}
 }
-- 
2.34.1

Re: [PATCH v3 3/9] device property: add index argument to property_read_string_array() callback

[Andy Shevchenko]

On Fri, Mar 25, 2022 at 12:31:42PM +0100, Clément Léger wrote:
> This will allow to read a string array from a specified offset. Support
> for this new parameter has been added in both of through the use of
> of_property_read_string_array_index() and in swnode though the existing
> property_entry_read_string_array() function. ACPI support has not yet
> been added and only index == 0 is accepted.

...

>  static int
>  acpi_fwnode_property_read_string_array(const struct fwnode_handle *fwnode,
>  				       const char *propname, const char **val,
> -				       size_t nval)
> +				       size_t nval, int index)
>  {
> +	if (index != 0)
> +		return -EINVAL;
> +
>  	return acpi_node_prop_read(fwnode, propname, DEV_PROP_STRING,
>  				   val, nval);

I guess it would be nice if some of us (ACPI folks) can provide a proper
prerequisite patch.

...

> -	array_len = min_t(size_t, nval, array_len);
>  	length = array_len * sizeof(*strings);

> -

Stray change.

>  	pointer = property_entry_find(props, propname, length);
>  	if (IS_ERR(pointer))
>  		return PTR_ERR(pointer);

> +	if (index >= array_len)
> +		return -ENODATA;

I was about to ask if we can check this before the property_entry_find() call,
but realized that in such case it will shadow possible errors due to wrong or
absent property.

...

> -		of_property_read_string_array(node, propname, val, nval) :
> +		of_property_read_string_array_index(node, propname, val, nval,
> +						    index) :

Dunno about the style there, but I think it can be one line.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 4/9] device property: add fwnode_property_read_string_index()

[Andy Shevchenko]

On Fri, Mar 25, 2022 at 12:31:43PM +0100, Clément Léger wrote:
> Add fwnode_property_read_string_index() function which allows to
> retrieve a single string from an array by its index. This function is
> the equivalent of of_property_read_string_index() but for fwnode
> support.

...

> +	ret = fwnode_call_int_op(fwnode, property_read_string_array, propname,
> +				 string, 1, index);
> +	if (ret == -EINVAL && !IS_ERR_OR_NULL(fwnode) &&
> +	    !IS_ERR_OR_NULL(fwnode->secondary))
> +		ret = fwnode_call_int_op(fwnode->secondary,
> +					 property_read_string_array, propname,
> +					 string, 1, index);

This is not fully correct. See [1] for the details.
I hope to send the new version just after the merge window ends.

[1]: https://lore.kernel.org/lkml/20220308123712.18613-1-andriy.shevchenko@linux.intel.com/

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 6/9] i2c: fwnode: add fwnode_find_i2c_adapter_by_node()

[Andy Shevchenko]

On Fri, Mar 25, 2022 at 12:31:45PM +0100, Clément Léger wrote:
> Add fwnode_find_i2c_adapter_by_node() which allows to retrieve a i2c
> adapter using a fwnode. Since dev_fwnode() uses the fwnode provided by
> the of_node member of the device, this will also work for devices were
> the of_node has been set and not the fwnode field.
> For acpi nodes, the check for parent node is skipped since
> i2c_acpi_find_adapter_by_handle() does not check it and we don't want
> to change this behavior.

...

> +#include <linux/device.h>
> +#include <linux/i2c.h>

Missed headers so far:
acpi.h

...

> +static int fwnode_dev_or_parent_node_match(struct device *dev, const void *data)
> +{
> +	if (device_match_fwnode(dev, data))
> +		return 1;
> +
> +	/*
> +	 * For ACPI device node, the behavior is to not match the parent (see
> +	 * i2c_acpi_find_adapter_by_handle())
> +	 */

Would it be harmful to drop this check?

> +	if (!is_acpi_device_node(dev_fwnode(dev)) && dev->parent)
> +		return device_match_fwnode(dev->parent, data);
> +
> +	return 0;
> +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 8/9] i2c: mux: pinctrl: remove CONFIG_OF dependency and use fwnode API

[Andy Shevchenko]

On Fri, Mar 25, 2022 at 12:31:47PM +0100, Clément Léger wrote:
> In order to use i2c muxes with all types of nodes, switch to fwnode
> API. The fwnode layer will allow to use this with both device_node and
> software_node.
> 
> This commits is simply replacing the use of "of_" prefixed functions
> with there fwnode equivalent.

What I meant by splitting to the patches is to be able to have first patch of
a such split to be independent of this series. And I believe one or two (if
you split to more logical pieces) may be done this way, means we have already
available APIs.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 6/9] i2c: fwnode: add fwnode_find_i2c_adapter_by_node()

[Clément Léger]

Le Fri, 25 Mar 2022 16:35:52 +0200,
Andy Shevchenko <andriy.shevchenko@linux.intel.com> a écrit :

> On Fri, Mar 25, 2022 at 12:31:45PM +0100, Clément Léger wrote:
> > Add fwnode_find_i2c_adapter_by_node() which allows to retrieve a i2c
> > adapter using a fwnode. Since dev_fwnode() uses the fwnode provided by
> > the of_node member of the device, this will also work for devices were
> > the of_node has been set and not the fwnode field.
> > For acpi nodes, the check for parent node is skipped since
> > i2c_acpi_find_adapter_by_handle() does not check it and we don't want
> > to change this behavior.  
> 
> ...
> 
> > +#include <linux/device.h>
> > +#include <linux/i2c.h>  
> 
> Missed headers so far:
> acpi.h

Indeed, will check that.

> 
> ...
> 
> > +static int fwnode_dev_or_parent_node_match(struct device *dev, const void *data)
> > +{
> > +	if (device_match_fwnode(dev, data))
> > +		return 1;
> > +
> > +	/*
> > +	 * For ACPI device node, the behavior is to not match the parent (see
> > +	 *  did not checked the )
> > +	 */  
> 
> Would it be harmful to drop this check?

Can't tell, I would not want to introduce some behavior wrt to parent
node for ACPI since it was not done this way. Might works in 99% of the
case though.

If ok with that, I can drop it.


-- 
Clément Léger,
Embedded Linux and Kernel engineer at Bootlin
https://bootlin.com

Re: [PATCH v3 4/9] device property: add fwnode_property_read_string_index()

[Clément Léger]

Le Fri, 25 Mar 2022 16:33:54 +0200,
Andy Shevchenko <andriy.shevchenko@linux.intel.com> a écrit :

> On Fri, Mar 25, 2022 at 12:31:43PM +0100, Clément Léger wrote:
> > Add fwnode_property_read_string_index() function which allows to
> > retrieve a single string from an array by its index. This function is
> > the equivalent of of_property_read_string_index() but for fwnode
> > support.  
> 
> ...
> 
> > +	ret = fwnode_call_int_op(fwnode, property_read_string_array, propname,
> > +				 string, 1, index);
> > +	if (ret == -EINVAL && !IS_ERR_OR_NULL(fwnode) &&
> > +	    !IS_ERR_OR_NULL(fwnode->secondary))
> > +		ret = fwnode_call_int_op(fwnode->secondary,
> > +					 property_read_string_array, propname,
> > +					 string, 1, index);  
> 
> This is not fully correct. See [1] for the details.
> I hope to send the new version just after the merge window ends.
> 
> [1]: https://lore.kernel.org/lkml/20220308123712.18613-1-andriy.shevchenko@linux.intel.com/
> 

Ok, I think we can wait for your patch to be applied in that case.

-- 
Clément Léger,
Embedded Linux and Kernel engineer at Bootlin
https://bootlin.com

Re: [PATCH v3 6/9] i2c: fwnode: add fwnode_find_i2c_adapter_by_node()

[Andy Shevchenko]

On Fri, Mar 25, 2022 at 04:09:27PM +0100, Clément Léger wrote:
> Le Fri, 25 Mar 2022 16:35:52 +0200,
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> a écrit :
> > On Fri, Mar 25, 2022 at 12:31:45PM +0100, Clément Léger wrote:

...

> > > +	 * For ACPI device node, the behavior is to not match the parent (see
> > > +	 *  did not checked the )
> > 
> > Would it be harmful to drop this check?
> 
> Can't tell, I would not want to introduce some behavior wrt to parent
> node for ACPI since it was not done this way. Might works in 99% of the
> case though.
> 
> If ok with that, I can drop it.

Let's ask Mika and Jarkko if they know more on this. I think Mika was the
one who introduced that (sorry, if I'm mistaken, haven't looked at the history
carefully).

P.S. Interesting enough that Mika is listed as I2C ACPI maintainer and his
email is not in the Cc. Please, check how you form Cc list for this series
and include all parties next time.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 6/9] i2c: fwnode: add fwnode_find_i2c_adapter_by_node()

[Clément Léger]

Le Fri, 25 Mar 2022 17:56:43 +0200,
Andy Shevchenko <andriy.shevchenko@linux.intel.com> a écrit :

> > 
> > Can't tell, I would not want to introduce some behavior wrt to parent
> > node for ACPI since it was not done this way. Might works in 99% of the
> > case though.
> > 
> > If ok with that, I can drop it.  
> 
> Let's ask Mika and Jarkko if they know more on this. I think Mika was the
> one who introduced that (sorry, if I'm mistaken, haven't looked at the history
> carefully).
> 
> P.S. Interesting enough that Mika is listed as I2C ACPI maintainer and his
> email is not in the Cc. Please, check how you form Cc list for this series
> and include all parties next time.

I'm using get_maintainers.pl which does not return Mika nor Jarkko. I
was using --nogit which probably ruled out some contributors but not
them apparently.

-- 
Clément Léger,
Embedded Linux and Kernel engineer at Bootlin
https://bootlin.com

Re: [PATCH v3 6/9] i2c: fwnode: add fwnode_find_i2c_adapter_by_node()

[Andy Shevchenko]

On Fri, Mar 25, 2022 at 05:04:39PM +0100, Clément Léger wrote:
> Le Fri, 25 Mar 2022 17:56:43 +0200,
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> a écrit :

...

> > P.S. Interesting enough that Mika is listed as I2C ACPI maintainer and his
> > email is not in the Cc. Please, check how you form Cc list for this series
> > and include all parties next time.
> 
> I'm using get_maintainers.pl which does not return Mika nor Jarkko. I
> was using --nogit which probably ruled out some contributors but not
> them apparently.

Yeah, in this case it seems some heuristics has to be used...

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 8/9] i2c: mux: pinctrl: remove CONFIG_OF dependency and use fwnode API

[Peter Rosin]

Hi!

On 2022-03-25 12:31, Clément Léger wrote:
> In order to use i2c muxes with all types of nodes, switch to fwnode
> API. The fwnode layer will allow to use this with both device_node and
> software_node.
> 
> This commits is simply replacing the use of "of_" prefixed functions
> with there fwnode equivalent.
> 
> Signed-off-by: Clément Léger <clement.leger@bootlin.com>
> ---
>  drivers/i2c/muxes/Kconfig           |  1 -
>  drivers/i2c/muxes/i2c-mux-pinctrl.c | 23 ++++++++++++-----------
>  2 files changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/i2c/muxes/Kconfig b/drivers/i2c/muxes/Kconfig
> index 1708b1a82da2..d9cb15cfba3e 100644
> --- a/drivers/i2c/muxes/Kconfig
> +++ b/drivers/i2c/muxes/Kconfig
> @@ -77,7 +77,6 @@ config I2C_MUX_PCA954x
>  config I2C_MUX_PINCTRL
>  	tristate "pinctrl-based I2C multiplexer"
>  	depends on PINCTRL
> -	depends on OF || COMPILE_TEST
>  	help
>  	  If you say yes to this option, support will be included for an I2C
>  	  multiplexer that uses the pinctrl subsystem, i.e. pin multiplexing.
> diff --git a/drivers/i2c/muxes/i2c-mux-pinctrl.c b/drivers/i2c/muxes/i2c-mux-pinctrl.c
> index f1bb00a11ad6..d9c0241e8790 100644
> --- a/drivers/i2c/muxes/i2c-mux-pinctrl.c
> +++ b/drivers/i2c/muxes/i2c-mux-pinctrl.c
> @@ -53,19 +53,20 @@ static struct i2c_adapter *i2c_mux_pinctrl_root_adapter(
>  
>  static struct i2c_adapter *i2c_mux_pinctrl_parent_adapter(struct device *dev)
>  {
> -	struct device_node *np = dev->of_node;
> -	struct device_node *parent_np;
> +	struct fwnode_handle *fwnode = dev_fwnode(dev);
> +	struct fwnode_handle *parent_fwnode;
>  	struct i2c_adapter *parent;
>  
> -	parent_np = of_parse_phandle(np, "i2c-parent", 0);
> -	if (!parent_np) {
> +	parent_fwnode = fwnode_find_reference(fwnode, "i2c-parent", 0);
> +	if (!parent_fwnode) {
>  		dev_err(dev, "Cannot parse i2c-parent\n");
>  		return ERR_PTR(-ENODEV);
>  	}
> -	parent = of_find_i2c_adapter_by_node(parent_np);
> -	of_node_put(parent_np);
> -	if (!parent)
> +	parent = fwnode_find_i2c_adapter_by_node(parent_fwnode);
> +	if (!parent) {
> +		dev_err(dev, "Cannot find i2c-parent\n");

Why do we need to log this as an error?

Cheers,
Peter

>  		return ERR_PTR(-EPROBE_DEFER);
> +	}
>  
>  	return parent;
>  }
> @@ -73,7 +74,7 @@ static struct i2c_adapter *i2c_mux_pinctrl_parent_adapter(struct device *dev)
>  static int i2c_mux_pinctrl_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
> -	struct device_node *np = dev->of_node;
> +	struct fwnode_handle *fwnode = dev_fwnode(dev);
>  	struct i2c_mux_core *muxc;
>  	struct i2c_mux_pinctrl *mux;
>  	struct i2c_adapter *parent;
> @@ -81,7 +82,7 @@ static int i2c_mux_pinctrl_probe(struct platform_device *pdev)
>  	int num_names, i, ret;
>  	const char *name;
>  
> -	num_names = of_property_count_strings(np, "pinctrl-names");
> +	num_names = fwnode_property_string_array_count(fwnode, "pinctrl-names");
>  	if (num_names < 0) {
>  		dev_err(dev, "Cannot parse pinctrl-names: %d\n",
>  			num_names);
> @@ -111,8 +112,8 @@ static int i2c_mux_pinctrl_probe(struct platform_device *pdev)
>  	}
>  
>  	for (i = 0; i < num_names; i++) {
> -		ret = of_property_read_string_index(np, "pinctrl-names", i,
> -						    &name);
> +		ret = fwnode_property_read_string_index(fwnode, "pinctrl-names", i,
> +							&name);
>  		if (ret < 0) {
>  			dev_err(dev, "Cannot parse pinctrl-names: %d\n", ret);
>  			goto err_put_parent;

Re: [PATCH v3 3/9] device property: add index argument to property_read_string_array() callback

[Clément Léger]

Le Fri, 25 Mar 2022 16:30:45 +0200,
Andy Shevchenko <andriy.shevchenko@linux.intel.com> a écrit :

> >  	pointer = property_entry_find(props, propname, length);
> >  	if (IS_ERR(pointer))
> >  		return PTR_ERR(pointer);  
> 
> > +	if (index >= array_len)
> > +		return -ENODATA;  
> 
> I was about to ask if we can check this before the
> property_entry_find() call, but realized that in such case it will
> shadow possible errors due to wrong or absent property.

I think you are actually right, the check can be done after
property_entry_count_elems_of_size() since it already checks for the
property to be present. I'll move that check.

> 
> ...
> 
> > -		of_property_read_string_array(node, propname, val,
> > nval) :
> > +		of_property_read_string_array_index(node,
> > propname, val, nval,
> > +						    index) :  
> 
> Dunno about the style there, but I think it can be one line.

Seems like the complete file is strictly applying the 80 columns rules
so I thought it was better to keep it like this. However, I think the
ternary oeprator is not really readable with such split.

-- 
Clément Léger,
Embedded Linux and Kernel engineer at Bootlin
https://bootlin.com

Re: [PATCH v3 1/9] of: add of_property_read_string_array_index()

[Rob Herring]

On Fri, 25 Mar 2022 12:31:40 +0100, Clément Léger wrote:
> Add of_property_read_string_array_index() which allows to read a string
> array starting at a specific index.
> 
> Signed-off-by: Clément Léger <clement.leger@bootlin.com>
> ---
>  include/linux/of.h | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH v3 2/9] of: unittests: add tests for of_property_read_string_array_index()

[Rob Herring]

On Fri, 25 Mar 2022 12:31:41 +0100, Clément Léger wrote:
> Add testing for of_property_read_string_array_index() function which
> allows to retrieve a string array starting at a specified offset. These
> tests are testing some basic use-case for this function.
> 
> Signed-off-by: Clément Léger <clement.leger@bootlin.com>
> ---
>  drivers/of/unittest.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH v3 8/9] i2c: mux: pinctrl: remove CONFIG_OF dependency and use fwnode API

[Clément Léger]

Le Fri, 25 Mar 2022 17:48:19 +0100,
Peter Rosin <peda@axentia.se> a écrit :

> >  
> > -	parent_np = of_parse_phandle(np, "i2c-parent", 0);
> > -	if (!parent_np) {
> > +	parent_fwnode = fwnode_find_reference(fwnode, "i2c-parent", 0);
> > +	if (!parent_fwnode) {
> >  		dev_err(dev, "Cannot parse i2c-parent\n");
> >  		return ERR_PTR(-ENODEV);
> >  	}
> > -	parent = of_find_i2c_adapter_by_node(parent_np);
> > -	of_node_put(parent_np);
> > -	if (!parent)
> > +	parent = fwnode_find_i2c_adapter_by_node(parent_fwnode);
> > +	if (!parent) {
> > +		dev_err(dev, "Cannot find i2c-parent\n");  
> 
> Why do we need to log this as an error?

Hi Peter, sorry for the late answer, your mail ended up in my SPAM
folder.

Regarding the error logging, you are right, this is not needed. I'll
remove it.

Thanks,

Clément 

-- 
Clément Léger,
Embedded Linux and Kernel engineer at Bootlin
https://bootlin.com

Re: [PATCH v3 3/9] device property: add index argument to property_read_string_array() callback

[Rafael J. Wysocki]

On Mon, Mar 28, 2022 at 4:29 PM Clément Léger <clement.leger@bootlin.com> wrote:
>
> Le Fri, 25 Mar 2022 16:30:45 +0200,
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> a écrit :
>
> > >     pointer = property_entry_find(props, propname, length);
> > >     if (IS_ERR(pointer))
> > >             return PTR_ERR(pointer);
> >
> > > +   if (index >= array_len)
> > > +           return -ENODATA;
> >
> > I was about to ask if we can check this before the
> > property_entry_find() call, but realized that in such case it will
> > shadow possible errors due to wrong or absent property.
>
> I think you are actually right, the check can be done after
> property_entry_count_elems_of_size() since it already checks for the
> property to be present. I'll move that check.
>
> >
> > ...
> >
> > > -           of_property_read_string_array(node, propname, val,
> > > nval) :
> > > +           of_property_read_string_array_index(node,
> > > propname, val, nval,
> > > +                                               index) :
> >
> > Dunno about the style there, but I think it can be one line.
>
> Seems like the complete file is strictly applying the 80 columns rules
> so I thought it was better to keep it like this. However, I think the
> ternary oeprator is not really readable with such split.

So FWIW I would entirely change it to

if (!val)
        return of_property_count_strings(node, propname);

return of_property_read_string_array_index(node, propname, val,

nval, index);

which IMO would be way easier to read.

Re: [PATCH v3 3/9] device property: add index argument to property_read_string_array() callback

[Clément Léger]

Le Tue, 5 Apr 2022 15:22:51 +0200,
"Rafael J. Wysocki" <rafael@kernel.org> a écrit :

> On Mon, Mar 28, 2022 at 4:29 PM Clément Léger <clement.leger@bootlin.com> wrote:
> >
> > Le Fri, 25 Mar 2022 16:30:45 +0200,
> > Andy Shevchenko <andriy.shevchenko@linux.intel.com> a écrit :
> >  
> > > >     pointer = property_entry_find(props, propname, length);
> > > >     if (IS_ERR(pointer))
> > > >             return PTR_ERR(pointer);  
> > >  
> > > > +   if (index >= array_len)
> > > > +           return -ENODATA;  
> > >
> > > I was about to ask if we can check this before the
> > > property_entry_find() call, but realized that in such case it will
> > > shadow possible errors due to wrong or absent property.  
> >
> > I think you are actually right, the check can be done after
> > property_entry_count_elems_of_size() since it already checks for the
> > property to be present. I'll move that check.
> >  
> > >
> > > ...
> > >  
> > > > -           of_property_read_string_array(node, propname, val,
> > > > nval) :
> > > > +           of_property_read_string_array_index(node,
> > > > propname, val, nval,
> > > > +                                               index) :  
> > >
> > > Dunno about the style there, but I think it can be one line.  
> >
> > Seems like the complete file is strictly applying the 80 columns rules
> > so I thought it was better to keep it like this. However, I think the
> > ternary oeprator is not really readable with such split.  
> 
> So FWIW I would entirely change it to
> 
> if (!val)
>         return of_property_count_strings(node, propname);
> 
> return of_property_read_string_array_index(node, propname, val,
> 
> nval, index);
> 
> which IMO would be way easier to read.

Hi Rafael,

Agreed, this is way more readable. I'll modify that.

Thanks,

-- 
Clément Léger,
Embedded Linux and Kernel engineer at Bootlin
https://bootlin.com

Re: [PATCH v3 0/9] introduce fwnode in the I2C subsystem

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 527 bytes --]

O
> This series is a subset of the one that was first submitted as a larger
> series to add swnode support [1]. In this one, it will be focused on
> fwnode support only since it seems to have reach a consensus that
> adding fwnode to subsystems makes sense.

From a high level view, I like this series. Though, it will need Peter's
ack on the I2C mux patches as he is the I2C mux maintainer. Still, I
wonder about the way to upstream the series. Feels like the first 5
patches should not go via I2C but seperately?


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v3 0/9] introduce fwnode in the I2C subsystem

[Peter Rosin]

2022-05-14 at 16:47, Wolfram Sang wrote:
> O
>> This series is a subset of the one that was first submitted as a larger
>> series to add swnode support [1]. In this one, it will be focused on
>> fwnode support only since it seems to have reach a consensus that
>> adding fwnode to subsystems makes sense.
> 
> From a high level view, I like this series. Though, it will need Peter's
> ack on the I2C mux patches as he is the I2C mux maintainer. Still, I
> wonder about the way to upstream the series. Feels like the first 5
> patches should not go via I2C but seperately?

Hi Wolfram,

I also think it looks basically sane. However, there are a couple of
comments plus promises to adjust accordingly. I guess I filed it under
"wait for the next iteration"...

Cheers,
Peter

Re: [PATCH v3 0/9] introduce fwnode in the I2C subsystem

[Clément Léger]

Le Sun, 15 May 2022 23:34:16 +0200,
Peter Rosin <peda@axentia.se> a écrit :

> 2022-05-14 at 16:47, Wolfram Sang wrote:
> > O  
> >> This series is a subset of the one that was first submitted as a larger
> >> series to add swnode support [1]. In this one, it will be focused on
> >> fwnode support only since it seems to have reach a consensus that
> >> adding fwnode to subsystems makes sense.  
> > 
> > From a high level view, I like this series. Though, it will need Peter's
> > ack on the I2C mux patches as he is the I2C mux maintainer. Still, I
> > wonder about the way to upstream the series. Feels like the first 5
> > patches should not go via I2C but seperately?  
> 
> Hi Wolfram,
> 
> I also think it looks basically sane. However, there are a couple of
> comments plus promises to adjust accordingly. I guess I filed it under
> "wait for the next iteration"...
> 
> Cheers,
> Peter

Hi Wolfram & Peter,

While doing the same conversion on the reset subsystem, Rob Herring
stepped in and mention the fact that this could be done using
device-tree overlay (even on system with ACPI) .

The result was that a new serie [1] which add support to create the PCI
devices of_node if not existing, and then allow drivers to applies an
overlay which describe the tree of devices as a child of a specific PCI
device of_node. There are a lot of advantages to this approach (small
patchset working for all susbystems, easier to use, description is
using already existing device-tree). There are still some concerns
about the viability of dynamic overlay but this might be settled soon.

Regards,

[1] https://lore.kernel.org/all/20220509141634.16158c38@xps-bootlin/T/

-- 
Clément Léger,
Embedded Linux and Kernel engineer at Bootlin
https://bootlin.com

Re: [PATCH v3 0/9] introduce fwnode in the I2C subsystem

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 241 bytes --]


> While doing the same conversion on the reset subsystem, Rob Herring
> stepped in and mention the fact that this could be done using
> device-tree overlay (even on system with ACPI) .

Thanks for the heads up, I'll drop this series then.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]