[PATCH 0/6] introduce fwnode in the I2C subsystem

[PATCH 0/6] 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.

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/

Clément Léger (6):
  property: add 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
  net: sfp: add support for fwnode

 drivers/base/property.c             | 48 +++++++++++++++++++++++++++++
 drivers/i2c/Makefile                |  1 +
 drivers/i2c/i2c-core-fwnode.c       | 41 ++++++++++++++++++++++++
 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 | 21 +++++++------
 drivers/net/phy/sfp.c               | 46 +++++++++------------------
 include/linux/i2c.h                 |  7 ++++-
 include/linux/property.h            |  3 ++
 10 files changed, 142 insertions(+), 95 deletions(-)
 create mode 100644 drivers/i2c/i2c-core-fwnode.c

-- 
2.34.1

[PATCH 1/6] property: add fwnode_property_read_string_index()

[Clément Léger]

Add fwnode_property_read_string_index() function which allows to
retrieve a 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  | 48 ++++++++++++++++++++++++++++++++++++++++
 include/linux/property.h |  3 +++
 2 files changed, 51 insertions(+)

diff --git a/drivers/base/property.c b/drivers/base/property.c
index e6497f6877ee..67c33c11f00c 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -451,6 +451,54 @@ int fwnode_property_match_string(const struct fwnode_handle *fwnode,
 }
 EXPORT_SYMBOL_GPL(fwnode_property_match_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 property 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)
+{
+	const char **values;
+	int nval, ret;
+
+	nval = fwnode_property_read_string_array(fwnode, propname, NULL, 0);
+	if (nval < 0)
+		return nval;
+
+	if (index >= nval)
+		return -EINVAL;
+
+	if (nval == 0)
+		return -ENODATA;
+
+	values = kcalloc(nval, sizeof(*values), GFP_KERNEL);
+	if (!values)
+		return -ENOMEM;
+
+	ret = fwnode_property_read_string_array(fwnode, propname, values, nval);
+	if (ret < 0)
+		goto out;
+
+	*string = values[index];
+out:
+	kfree(values);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(fwnode_property_read_string_index);
+
 /**
  * fwnode_property_get_reference_args() - Find a reference with arguments
  * @fwnode:	Firmware node where to look for the reference
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 2/6] 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.

Signed-off-by: Clément Léger <clement.leger@bootlin.com>
---
 drivers/i2c/Makefile          |  1 +
 drivers/i2c/i2c-core-fwnode.c | 41 +++++++++++++++++++++++++++++++++++
 include/linux/i2c.h           |  2 ++
 3 files changed, 44 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..2404c2477a80
--- /dev/null
+++ b/drivers/i2c/i2c-core-fwnode.c
@@ -0,0 +1,41 @@
+// 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, we do not want to match the parent */
+	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 *node)
+{
+	struct device *dev;
+	struct i2c_adapter *adapter;
+
+	dev = bus_find_device(&i2c_bus_type, NULL, node,
+			      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..9b480a8b0a76 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -967,6 +967,8 @@ 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 *node);
+
 #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 3/6] i2c: of: use fwnode_get_i2c_adapter_by_node()

[Clément Léger]

Since the new fwnode function does the same check that was done by
of_get_i2c_adapter_by_node(), call this one 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 9b480a8b0a76..d1f384b805ad 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -974,7 +974,10 @@ struct i2c_adapter *fwnode_find_i2c_adapter_by_node(struct fwnode_handle *node);
 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 4/6] i2c: mux: pinctrl: remove CONFIG_OF dependency and use fwnode API

[Clément Léger]

In order to use i2c muxes with software_node when added with a struct
mfd_cell, switch to fwnode API. The fwnode layer will allow to use this
with both device_node and software_node.

Signed-off-by: Clément Léger <clement.leger@bootlin.com>
---
 drivers/i2c/muxes/Kconfig           |  1 -
 drivers/i2c/muxes/i2c-mux-pinctrl.c | 21 +++++++++++----------
 2 files changed, 11 insertions(+), 11 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..200890d7a625 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_np;
 	struct i2c_adapter *parent;
 
-	parent_np = of_parse_phandle(np, "i2c-parent", 0);
+	parent_np = fwnode_find_reference(fwnode, "i2c-parent", 0);
 	if (!parent_np) {
 		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_np);
+	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 *np = 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(np, "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(np, "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 5/6] i2c: mux: add support for fwnode

[Clément Léger]

Modify i2c_mux_add_adapter() to use with fwnode API to allow creating
mux adapters with fwnode based devices. This allows to have a node
independent 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..93c916220da5 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 *np = 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(np);
 		kfree(priv);
 	}
 }
-- 
2.34.1

[PATCH 6/6] net: sfp: add support for fwnode

[Clément Léger]

Add support to retrieve a i2c bus in sfp with a fwnode. This support
is using the fwnode API which also works with device-tree and ACPI.
For this purpose, the device-tree and ACPI code handling the i2c
adapter retrieval was factorized with the new code. This also allows
i2c devices using a software_node description to be used by sfp code.

Signed-off-by: Clément Léger <clement.leger@bootlin.com>
---
 drivers/net/phy/sfp.c | 46 +++++++++++++------------------------------
 1 file changed, 14 insertions(+), 32 deletions(-)

diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c
index 4720b24ca51b..3f57b1fbf413 100644
--- a/drivers/net/phy/sfp.c
+++ b/drivers/net/phy/sfp.c
@@ -2483,7 +2483,7 @@ static void sfp_cleanup(void *data)
 static int sfp_probe(struct platform_device *pdev)
 {
 	const struct sff_data *sff;
-	struct i2c_adapter *i2c;
+	struct i2c_adapter *i2c = NULL;
 	char *sfp_irq_name;
 	struct sfp *sfp;
 	int err, i;
@@ -2499,43 +2499,25 @@ static int sfp_probe(struct platform_device *pdev)
 		return err;
 
 	sff = sfp->type = &sfp_data;
+	if (dev_fwnode(&pdev->dev)) {
+		struct fwnode_handle *fwnode = dev_fwnode(&pdev->dev);
+		struct fwnode_handle *np;
 
-	if (pdev->dev.of_node) {
-		struct device_node *node = pdev->dev.of_node;
-		const struct of_device_id *id;
-		struct device_node *np;
-
-		id = of_match_node(sfp_of_match, node);
-		if (WARN_ON(!id))
-			return -EINVAL;
+		if (!is_acpi_device_node(fwnode)) {
+			sff = device_get_match_data(&pdev->dev);
+			if (WARN_ON(!sff))
+				return -EINVAL;
 
-		sff = sfp->type = id->data;
-
-		np = of_parse_phandle(node, "i2c-bus", 0);
-		if (!np) {
-			dev_err(sfp->dev, "missing 'i2c-bus' property\n");
-			return -ENODEV;
+			sfp->type = sff;
 		}
 
-		i2c = of_find_i2c_adapter_by_node(np);
-		of_node_put(np);
-	} else if (has_acpi_companion(&pdev->dev)) {
-		struct acpi_device *adev = ACPI_COMPANION(&pdev->dev);
-		struct fwnode_handle *fw = acpi_fwnode_handle(adev);
-		struct fwnode_reference_args args;
-		struct acpi_handle *acpi_handle;
-		int ret;
-
-		ret = acpi_node_get_property_reference(fw, "i2c-bus", 0, &args);
-		if (ret || !is_acpi_device_node(args.fwnode)) {
-			dev_err(&pdev->dev, "missing 'i2c-bus' property\n");
+		np = fwnode_find_reference(fwnode, "i2c-bus", 0);
+		if (!np) {
+			dev_err(&pdev->dev, "Cannot parse i2c-bus\n");
 			return -ENODEV;
 		}
-
-		acpi_handle = ACPI_HANDLE_FWNODE(args.fwnode);
-		i2c = i2c_acpi_find_adapter_by_handle(acpi_handle);
-	} else {
-		return -EINVAL;
+		i2c = fwnode_find_i2c_adapter_by_node(np);
+		fwnode_handle_put(np);
 	}
 
 	if (!i2c)
-- 
2.34.1

[PATCH 0/6] 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.

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/

Clément Léger (6):
  property: add 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
  net: sfp: add support for fwnode

 drivers/base/property.c             | 48 +++++++++++++++++++++++++++++
 drivers/i2c/Makefile                |  1 +
 drivers/i2c/i2c-core-fwnode.c       | 41 ++++++++++++++++++++++++
 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 | 21 +++++++------
 drivers/net/phy/sfp.c               | 46 +++++++++------------------
 include/linux/i2c.h                 |  7 ++++-
 include/linux/property.h            |  3 ++
 10 files changed, 142 insertions(+), 95 deletions(-)
 create mode 100644 drivers/i2c/i2c-core-fwnode.c

-- 
2.34.1

[PATCH 1/6] property: add fwnode_property_read_string_index()

[Clément Léger]

Add fwnode_property_read_string_index() function which allows to
retrieve a 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  | 48 ++++++++++++++++++++++++++++++++++++++++
 include/linux/property.h |  3 +++
 2 files changed, 51 insertions(+)

diff --git a/drivers/base/property.c b/drivers/base/property.c
index e6497f6877ee..67c33c11f00c 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -451,6 +451,54 @@ int fwnode_property_match_string(const struct fwnode_handle *fwnode,
 }
 EXPORT_SYMBOL_GPL(fwnode_property_match_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 property 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)
+{
+	const char **values;
+	int nval, ret;
+
+	nval = fwnode_property_read_string_array(fwnode, propname, NULL, 0);
+	if (nval < 0)
+		return nval;
+
+	if (index >= nval)
+		return -EINVAL;
+
+	if (nval == 0)
+		return -ENODATA;
+
+	values = kcalloc(nval, sizeof(*values), GFP_KERNEL);
+	if (!values)
+		return -ENOMEM;
+
+	ret = fwnode_property_read_string_array(fwnode, propname, values, nval);
+	if (ret < 0)
+		goto out;
+
+	*string = values[index];
+out:
+	kfree(values);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(fwnode_property_read_string_index);
+
 /**
  * fwnode_property_get_reference_args() - Find a reference with arguments
  * @fwnode:	Firmware node where to look for the reference
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 2/6] 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.

Signed-off-by: Clément Léger <clement.leger@bootlin.com>
---
 drivers/i2c/Makefile          |  1 +
 drivers/i2c/i2c-core-fwnode.c | 41 +++++++++++++++++++++++++++++++++++
 include/linux/i2c.h           |  2 ++
 3 files changed, 44 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..2404c2477a80
--- /dev/null
+++ b/drivers/i2c/i2c-core-fwnode.c
@@ -0,0 +1,41 @@
+// 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, we do not want to match the parent */
+	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 *node)
+{
+	struct device *dev;
+	struct i2c_adapter *adapter;
+
+	dev = bus_find_device(&i2c_bus_type, NULL, node,
+			      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..9b480a8b0a76 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -967,6 +967,8 @@ 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 *node);
+
 #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 3/6] i2c: of: use fwnode_get_i2c_adapter_by_node()

[Clément Léger]

Since the new fwnode function does the same check that was done by
of_get_i2c_adapter_by_node(), call this one 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 9b480a8b0a76..d1f384b805ad 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -974,7 +974,10 @@ struct i2c_adapter *fwnode_find_i2c_adapter_by_node(struct fwnode_handle *node);
 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 4/6] i2c: mux: pinctrl: remove CONFIG_OF dependency and use fwnode API

[Clément Léger]

In order to use i2c muxes with software_node when added with a struct
mfd_cell, switch to fwnode API. The fwnode layer will allow to use this
with both device_node and software_node.

Signed-off-by: Clément Léger <clement.leger@bootlin.com>
---
 drivers/i2c/muxes/Kconfig           |  1 -
 drivers/i2c/muxes/i2c-mux-pinctrl.c | 21 +++++++++++----------
 2 files changed, 11 insertions(+), 11 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..200890d7a625 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_np;
 	struct i2c_adapter *parent;
 
-	parent_np = of_parse_phandle(np, "i2c-parent", 0);
+	parent_np = fwnode_find_reference(fwnode, "i2c-parent", 0);
 	if (!parent_np) {
 		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_np);
+	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 *np = 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(np, "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(np, "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 5/6] i2c: mux: add support for fwnode

[Clément Léger]

Modify i2c_mux_add_adapter() to use with fwnode API to allow creating
mux adapters with fwnode based devices. This allows to have a node
independent 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..93c916220da5 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 *np = 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(np);
 		kfree(priv);
 	}
 }
-- 
2.34.1

[PATCH 6/6] net: sfp: add support for fwnode

[Clément Léger]

Add support to retrieve a i2c bus in sfp with a fwnode. This support
is using the fwnode API which also works with device-tree and ACPI.
For this purpose, the device-tree and ACPI code handling the i2c
adapter retrieval was factorized with the new code. This also allows
i2c devices using a software_node description to be used by sfp code.

Signed-off-by: Clément Léger <clement.leger@bootlin.com>
---
 drivers/net/phy/sfp.c | 46 +++++++++++++------------------------------
 1 file changed, 14 insertions(+), 32 deletions(-)

diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c
index 4720b24ca51b..3f57b1fbf413 100644
--- a/drivers/net/phy/sfp.c
+++ b/drivers/net/phy/sfp.c
@@ -2483,7 +2483,7 @@ static void sfp_cleanup(void *data)
 static int sfp_probe(struct platform_device *pdev)
 {
 	const struct sff_data *sff;
-	struct i2c_adapter *i2c;
+	struct i2c_adapter *i2c = NULL;
 	char *sfp_irq_name;
 	struct sfp *sfp;
 	int err, i;
@@ -2499,43 +2499,25 @@ static int sfp_probe(struct platform_device *pdev)
 		return err;
 
 	sff = sfp->type = &sfp_data;
+	if (dev_fwnode(&pdev->dev)) {
+		struct fwnode_handle *fwnode = dev_fwnode(&pdev->dev);
+		struct fwnode_handle *np;
 
-	if (pdev->dev.of_node) {
-		struct device_node *node = pdev->dev.of_node;
-		const struct of_device_id *id;
-		struct device_node *np;
-
-		id = of_match_node(sfp_of_match, node);
-		if (WARN_ON(!id))
-			return -EINVAL;
+		if (!is_acpi_device_node(fwnode)) {
+			sff = device_get_match_data(&pdev->dev);
+			if (WARN_ON(!sff))
+				return -EINVAL;
 
-		sff = sfp->type = id->data;
-
-		np = of_parse_phandle(node, "i2c-bus", 0);
-		if (!np) {
-			dev_err(sfp->dev, "missing 'i2c-bus' property\n");
-			return -ENODEV;
+			sfp->type = sff;
 		}
 
-		i2c = of_find_i2c_adapter_by_node(np);
-		of_node_put(np);
-	} else if (has_acpi_companion(&pdev->dev)) {
-		struct acpi_device *adev = ACPI_COMPANION(&pdev->dev);
-		struct fwnode_handle *fw = acpi_fwnode_handle(adev);
-		struct fwnode_reference_args args;
-		struct acpi_handle *acpi_handle;
-		int ret;
-
-		ret = acpi_node_get_property_reference(fw, "i2c-bus", 0, &args);
-		if (ret || !is_acpi_device_node(args.fwnode)) {
-			dev_err(&pdev->dev, "missing 'i2c-bus' property\n");
+		np = fwnode_find_reference(fwnode, "i2c-bus", 0);
+		if (!np) {
+			dev_err(&pdev->dev, "Cannot parse i2c-bus\n");
 			return -ENODEV;
 		}
-
-		acpi_handle = ACPI_HANDLE_FWNODE(args.fwnode);
-		i2c = i2c_acpi_find_adapter_by_handle(acpi_handle);
-	} else {
-		return -EINVAL;
+		i2c = fwnode_find_i2c_adapter_by_node(np);
+		fwnode_handle_put(np);
 	}
 
 	if (!i2c)
-- 
2.34.1

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

[Andy Shevchenko]

On Fri, Mar 18, 2022 at 05:00:46PM +0100, Clément Léger wrote:
> 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.
> 
> 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/

I have got two copies (?) of the series. I have no idea which one is correct.
So, please take care of it and send the version we are supposed to review.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/6] property: add fwnode_property_read_string_index()

[Andy Shevchenko]

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

...

> +	values = kcalloc(nval, sizeof(*values), GFP_KERNEL);
> +	if (!values)
> +		return -ENOMEM;
> +
> +	ret = fwnode_property_read_string_array(fwnode, propname, values, nval);
> +	if (ret < 0)
> +		goto out;
> +
> +	*string = values[index];
> +out:
> +	kfree(values);

Here is UAF (use after free). How is it supposed to work?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 4/6] i2c: mux: pinctrl: remove CONFIG_OF dependency and use fwnode API

[Andy Shevchenko]

On Fri, Mar 18, 2022 at 05:00:50PM +0100, Clément Léger wrote:
> In order to use i2c muxes with software_node when added with a struct
> mfd_cell, switch to fwnode API. The fwnode layer will allow to use this
> with both device_node and software_node.

> -	struct device_node *np = dev->of_node;
> +	struct fwnode_handle *np = dev_fwnode(dev);

np is now a misleading name. Use fwnode.

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Fri, Mar 18, 2022 at 05:00:48PM +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 device node, we do not want to match the parent */

Why?
Neither commit message nor this comment does not answer to this question.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 6/6] net: sfp: add support for fwnode

[Andy Shevchenko]

On Fri, Mar 18, 2022 at 05:00:52PM +0100, Clément Léger wrote:
> Add support to retrieve a i2c bus in sfp with a fwnode. This support
> is using the fwnode API which also works with device-tree and ACPI.
> For this purpose, the device-tree and ACPI code handling the i2c
> adapter retrieval was factorized with the new code. This also allows
> i2c devices using a software_node description to be used by sfp code.

> +		i2c = fwnode_find_i2c_adapter_by_node(np);

Despite using this, I believe you may split this patch to at least two where
first one is preparatory (converting whatever parts is possible to fwnode
(looks like ACPI case may be optimized) followed by this change.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/6] property: add fwnode_property_read_string_index()

[Clément Léger]

Le Fri, 18 Mar 2022 18:26:00 +0200,
Andy Shevchenko <andriy.shevchenko@linux.intel.com> a écrit :

> On Fri, Mar 18, 2022 at 05:00:47PM +0100, Clément Léger wrote:
> > Add fwnode_property_read_string_index() function which allows to
> > retrieve a string from an array by its index. This function is the
> > equivalent of of_property_read_string_index() but for fwnode support.  
> 
> ...
> 
> > +	values = kcalloc(nval, sizeof(*values), GFP_KERNEL);
> > +	if (!values)
> > +		return -ENOMEM;
> > +
> > +	ret = fwnode_property_read_string_array(fwnode, propname, values, nval);
> > +	if (ret < 0)
> > +		goto out;
> > +
> > +	*string = values[index];
> > +out:
> > +	kfree(values);  
> 
> Here is UAF (use after free). How is it supposed to work?
> 

values is an array of pointers. I'm only retrieving a pointer out of
it.

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

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

[Clément Léger]

Le Fri, 18 Mar 2022 18:29:39 +0200,
Andy Shevchenko <andriy.shevchenko@linux.intel.com> a écrit :

> On Fri, Mar 18, 2022 at 05:00:48PM +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 device node, we do not want to match the parent */  
> 
> Why?
> Neither commit message nor this comment does not answer to this question.
> 

Yes you are right. This is done to keep the existing behavior that is
applied by i2c_acpi_find_adapter_by_handle() which only checks the
device node and not the parent one. Using the same behavior than for DT
would add some unwanted behavior in I2C device lookup for ACPI.

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

Re: [PATCH 4/6] i2c: mux: pinctrl: remove CONFIG_OF dependency and use fwnode API

[Clément Léger]

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

> On Fri, Mar 18, 2022 at 05:00:50PM +0100, Clément Léger wrote:
> > In order to use i2c muxes with software_node when added with a struct
> > mfd_cell, switch to fwnode API. The fwnode layer will allow to use this
> > with both device_node and software_node.  
> 
> > -	struct device_node *np = dev->of_node;
> > +	struct fwnode_handle *np = dev_fwnode(dev);  
> 
> np is now a misleading name. Use fwnode.
> 

Ok I thought np was meaning "node pointer" and it looked like okay to
avoid avoid a diff that is too huge. But agreed, I'll rename that.

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

Re: [PATCH 6/6] net: sfp: add support for fwnode

[Clément Léger]

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

> On Fri, Mar 18, 2022 at 05:00:52PM +0100, Clément Léger wrote:
> > Add support to retrieve a i2c bus in sfp with a fwnode. This support
> > is using the fwnode API which also works with device-tree and ACPI.
> > For this purpose, the device-tree and ACPI code handling the i2c
> > adapter retrieval was factorized with the new code. This also allows
> > i2c devices using a software_node description to be used by sfp code.  
> 
> > +		i2c = fwnode_find_i2c_adapter_by_node(np);  
> 
> Despite using this, I believe you may split this patch to at least two where
> first one is preparatory (converting whatever parts is possible to fwnode
> (looks like ACPI case may be optimized) followed by this change.
> 

Indeed, I had hesitate to do so but is seems more correct not to mix
everything.

Thanks for the review.


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

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

[Clément Léger]

Le Fri, 18 Mar 2022 18:24:05 +0200,
Andy Shevchenko <andriy.shevchenko@linux.intel.com> a écrit :

> On Fri, Mar 18, 2022 at 05:00:46PM +0100, Clément Léger wrote:
> > 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.
> > 
> > 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/  
> 
> I have got two copies (?) of the series. I have no idea which one is correct.
> So, please take care of it and send the version we are supposed to review.
> 

Sorry for that, both are identical and were unfortunately provided
twice to git send-email...

I'll be more cautious next time.


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

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

[Jakub Kicinski]

On Fri, 18 Mar 2022 17:00:46 +0100 Clément Léger wrote:
> 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.
> 
> 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/

Sorry to jump ahead but would be great to split it up so that every
subsystem could apply its patches without risking conflicts, once
consensus has been reached.

Re: [PATCH 1/6] property: add fwnode_property_read_string_index()

[Andy Shevchenko]

On Fri, Mar 18, 2022 at 05:49:12PM +0100, Clément Léger wrote:
> Le Fri, 18 Mar 2022 18:26:00 +0200,
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> a écrit :
> > On Fri, Mar 18, 2022 at 05:00:47PM +0100, Clément Léger wrote:
> > > Add fwnode_property_read_string_index() function which allows to
> > > retrieve a string from an array by its index. This function is the
> > > equivalent of of_property_read_string_index() but for fwnode support.  

...

> > > +	values = kcalloc(nval, sizeof(*values), GFP_KERNEL);
> > > +	if (!values)
> > > +		return -ENOMEM;
> > > +
> > > +	ret = fwnode_property_read_string_array(fwnode, propname, values, nval);
> > > +	if (ret < 0)
> > > +		goto out;
> > > +
> > > +	*string = values[index];
> > > +out:
> > > +	kfree(values);  
> > 
> > Here is UAF (use after free). How is it supposed to work?
> 
> values is an array of pointers. I'm only retrieving a pointer out of
> it.

I see, thanks for pointing out.

Nevertheless, I don't like the idea of allocating memory in this case.
Can we rather add a new callback that will provide us the necessary
property directly?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 4/6] i2c: mux: pinctrl: remove CONFIG_OF dependency and use fwnode API

[Andy Shevchenko]

On Fri, Mar 18, 2022 at 05:56:30PM +0100, Clément Léger wrote:
> Le Fri, 18 Mar 2022 18:28:45 +0200,
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> a écrit :
> > On Fri, Mar 18, 2022 at 05:00:50PM +0100, Clément Léger wrote:
> > > In order to use i2c muxes with software_node when added with a struct
> > > mfd_cell, switch to fwnode API. The fwnode layer will allow to use this
> > > with both device_node and software_node.  
> > 
> > > -	struct device_node *np = dev->of_node;
> > > +	struct fwnode_handle *np = dev_fwnode(dev);  
> > 
> > np is now a misleading name. Use fwnode.
> 
> Ok I thought np was meaning "node pointer" and it looked like okay to
> avoid avoid a diff that is too huge. But agreed, I'll rename that.

It's rather "in practice", np stands for "OF node pointer", while fwnode
stands for "firmware node handle".

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/6] property: add fwnode_property_read_string_index()

[Clément Léger]

Le Fri, 18 Mar 2022 20:09:37 +0200,
Andy Shevchenko <andriy.shevchenko@linux.intel.com> a écrit :

> On Fri, Mar 18, 2022 at 05:49:12PM +0100, Clément Léger wrote:
> > Le Fri, 18 Mar 2022 18:26:00 +0200,
> > Andy Shevchenko <andriy.shevchenko@linux.intel.com> a écrit :  
> > > On Fri, Mar 18, 2022 at 05:00:47PM +0100, Clément Léger wrote:  
> > > > Add fwnode_property_read_string_index() function which allows to
> > > > retrieve a string from an array by its index. This function is the
> > > > equivalent of of_property_read_string_index() but for fwnode support.    
> 
> ...
> 
> > > > +	values = kcalloc(nval, sizeof(*values), GFP_KERNEL);
> > > > +	if (!values)
> > > > +		return -ENOMEM;
> > > > +
> > > > +	ret = fwnode_property_read_string_array(fwnode, propname, values, nval);
> > > > +	if (ret < 0)
> > > > +		goto out;
> > > > +
> > > > +	*string = values[index];
> > > > +out:
> > > > +	kfree(values);    
> > > 
> > > Here is UAF (use after free). How is it supposed to work?  
> > 
> > values is an array of pointers. I'm only retrieving a pointer out of
> > it.  
> 
> I see, thanks for pointing out.
> 
> Nevertheless, I don't like the idea of allocating memory in this case.
> Can we rather add a new callback that will provide us the necessary
> property directly?
> 

IMHO, it would indeed be better. However,
fwnode_property_match_string() also allocates memory to do the same
kind of operation. Would you also like a callback for this one ?

Thanks,

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

Re: [PATCH 1/6] property: add fwnode_property_read_string_index()

[Andy Shevchenko]

On Mon, Mar 21, 2022 at 08:49:21AM +0100, Clément Léger wrote:
> Le Fri, 18 Mar 2022 20:09:37 +0200,
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> a écrit :
> > On Fri, Mar 18, 2022 at 05:49:12PM +0100, Clément Léger wrote:
> > > Le Fri, 18 Mar 2022 18:26:00 +0200,
> > > Andy Shevchenko <andriy.shevchenko@linux.intel.com> a écrit :  
> > > > On Fri, Mar 18, 2022 at 05:00:47PM +0100, Clément Léger wrote:  

...

> > > > > +	values = kcalloc(nval, sizeof(*values), GFP_KERNEL);
> > > > > +	if (!values)
> > > > > +		return -ENOMEM;
> > > > > +
> > > > > +	ret = fwnode_property_read_string_array(fwnode, propname, values, nval);
> > > > > +	if (ret < 0)
> > > > > +		goto out;
> > > > > +
> > > > > +	*string = values[index];
> > > > > +out:
> > > > > +	kfree(values);    
> > > > 
> > > > Here is UAF (use after free). How is it supposed to work?  
> > > 
> > > values is an array of pointers. I'm only retrieving a pointer out of
> > > it.  
> > 
> > I see, thanks for pointing out.
> > 
> > Nevertheless, I don't like the idea of allocating memory in this case.
> > Can we rather add a new callback that will provide us the necessary
> > property directly?
> > 
> 
> IMHO, it would indeed be better. However,
> fwnode_property_match_string() also allocates memory to do the same
> kind of operation. Would you also like a callback for this one ?

But matching string will need all of them to cover all possible cases.
So, it doesn't rely on the certain index and needs allocation anyway.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/6] property: add fwnode_property_read_string_index()

[Clément Léger]

Le Mon, 21 Mar 2022 12:31:58 +0200,
Andy Shevchenko <andriy.shevchenko@linux.intel.com> a écrit :

> > 
> > IMHO, it would indeed be better. However,
> > fwnode_property_match_string() also allocates memory to do the same
> > kind of operation. Would you also like a callback for this one ?  
> 
> But matching string will need all of them to cover all possible cases.
> So, it doesn't rely on the certain index and needs allocation anyway.

Acked, it makes sense to keep it that way.

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

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

[Clément Léger]

Le Fri, 18 Mar 2022 10:02:01 -0700,
Jakub Kicinski <kuba@kernel.org> a écrit :

> On Fri, 18 Mar 2022 17:00:46 +0100 Clément Léger wrote:
> > 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.
> > 
> > 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/  
> 
> Sorry to jump ahead but would be great to split it up so that every
> subsystem could apply its patches without risking conflicts, once
> consensus has been reached.

Hi Jakub,

Ok, to be clear, you would like a series which contains all the
"base" fwnode functions that I'm going to add to be sent separately
right ? And then also split i2c/net stuff that was sent in this series ?

Thanks,

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

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

[Jakub Kicinski]

On Mon, 21 Mar 2022 11:56:34 +0100 Clément Léger wrote:
> Le Fri, 18 Mar 2022 10:02:01 -0700,
> Jakub Kicinski <kuba@kernel.org> a écrit :
> 
> > On Fri, 18 Mar 2022 17:00:46 +0100 Clément Léger wrote:  
> > > 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.
> > > 
> > > 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/    
> > 
> > Sorry to jump ahead but would be great to split it up so that every
> > subsystem could apply its patches without risking conflicts, once
> > consensus has been reached.  
> 
> Hi Jakub,
> 
> Ok, to be clear, you would like a series which contains all the
> "base" fwnode functions that I'm going to add to be sent separately
> right ? And then also split i2c/net stuff that was sent in this series ?

I'm mostly concerned about conflicts, so if you can get the entire
series into 5.18 before the merge window is over then consider it 
acked. If it doesn't make 5.18 looks like you'd need to send patches 
1 and 2 as a PR so that both the i2c and net trees can pull it. 
Once pulled send patch 6 out to net-next. Does that make sense?

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

[Clément Léger]

Le Mon, 21 Mar 2022 11:36:34 -0700,
Jakub Kicinski <kuba@kernel.org> a écrit :

> > Hi Jakub,
> > 
> > Ok, to be clear, you would like a series which contains all the
> > "base" fwnode functions that I'm going to add to be sent separately
> > right ? And then also split i2c/net stuff that was sent in this series ?  
> 
> I'm mostly concerned about conflicts, so if you can get the entire
> series into 5.18 before the merge window is over then consider it 
> acked. If it doesn't make 5.18 looks like you'd need to send patches 
> 1 and 2 as a PR so that both the i2c and net trees can pull it. 
> Once pulled send patch 6 out to net-next. Does that make sense?

Yes totally, I guess I'll go for I2C only and then I'll move on with
next patches individually later. No need to hurry.


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