[PATCH RFC v2 0/2] Add I2C fwnode lookup/get interfaces

[PATCH RFC v2 0/2] Add I2C fwnode lookup/get interfaces

[Russell King (Oracle)]

Hi,

This RFC series is intended for the next merge window, but we will need
to decide how to merge it as it is split across two subsystems. These
patches have been generated against the net-next, since patch 2 depends
on a recently merged patch in that tree (which is now in mainline.)

Currently, the SFP code attempts to work out what kind of fwnode we
found when looking up the I2C bus for the SFP cage, converts the fwnode
to the appropriate firmware specific representation to then call the
appropriate I2C layer function. This is inefficient, since the device
model provides a way to locate items on a bus_type by fwnode.

In order to reduce this complexity, this series adds fwnode interfaces
to the I2C subsystem to allow I2C adapters to be looked up. I also
accidentally also converted the I2C clients to also be looked up, so
I've left that in patch 1 if people think that could be useful - if
not, I'll remove it.

We could also convert the of_* functions to be inline in i2c.h and
remove the stub of_* functions and exports.

Do we want these to live in i2c-core-fwnode.c ? I don't see a Kconfig
symbol that indicates whether we want fwnode support, and I know there
are people looking to use software nodes to lookup the SFP I2C bus
(which is why the manual firmware-specific code in sfp.c is a problem.)

Thanks!

v2: updated patch 1 with docbook comments.

 drivers/i2c/i2c-core-acpi.c | 13 +-----
 drivers/i2c/i2c-core-base.c | 98 +++++++++++++++++++++++++++++++++++++++++++++
 drivers/i2c/i2c-core-of.c   | 51 ++---------------------
 drivers/net/phy/sfp.c       | 13 +-----
 include/linux/i2c.h         |  9 +++++
 5 files changed, 112 insertions(+), 72 deletions(-)

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

[PATCH RFC net-next v2 1/2] i2c: add fwnode APIs

[Russell King (Oracle)]

Add fwnode APIs for finding and getting I2C adapters, which will be
used by the SFP code. These are passed the fwnode corresponding to
the adapter, and return the I2C adapter. It is the responsibility of
the caller to find the appropriate fwnode.

We keep the DT and ACPI interfaces, but where appropriate, recode them
to use the fwnode interfaces internally.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
v2: update docbook comments

 drivers/i2c/i2c-core-acpi.c | 13 +----
 drivers/i2c/i2c-core-base.c | 98 +++++++++++++++++++++++++++++++++++++
 drivers/i2c/i2c-core-of.c   | 51 ++-----------------
 include/linux/i2c.h         |  9 ++++
 4 files changed, 111 insertions(+), 60 deletions(-)

diff --git a/drivers/i2c/i2c-core-acpi.c b/drivers/i2c/i2c-core-acpi.c
index 4dd777cc0c89..d6037a328669 100644
--- a/drivers/i2c/i2c-core-acpi.c
+++ b/drivers/i2c/i2c-core-acpi.c
@@ -442,18 +442,7 @@ EXPORT_SYMBOL_GPL(i2c_acpi_find_adapter_by_handle);
 
 static struct i2c_client *i2c_acpi_find_client_by_adev(struct acpi_device *adev)
 {
-	struct device *dev;
-	struct i2c_client *client;
-
-	dev = bus_find_device_by_acpi_dev(&i2c_bus_type, adev);
-	if (!dev)
-		return NULL;
-
-	client = i2c_verify_client(dev);
-	if (!client)
-		put_device(dev);
-
-	return client;
+	return i2c_find_device_by_fwnode(acpi_fwnode_handle(adev));
 }
 
 static int i2c_acpi_notify(struct notifier_block *nb, unsigned long value,
diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 13fafb74bab8..1395a78d4a22 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -1012,6 +1012,35 @@ void i2c_unregister_device(struct i2c_client *client)
 }
 EXPORT_SYMBOL_GPL(i2c_unregister_device);
 
+/**
+ * i2c_find_device_by_fwnode() - find an i2c_client for the fwnode
+ * @fwnode: &struct fwnode_handle corresponding to the &struct i2c_client
+ *
+ * Look up and return the &struct i2c_client corresponding to the @fwnode.
+ * If no client can be found, or @fwnode is NULL, this returns NULL.
+ *
+ * The user must call put_device(&client->dev) once done with the i2c client.
+ */
+struct i2c_client *i2c_find_device_by_fwnode(struct fwnode_handle *fwnode)
+{
+	struct i2c_client *client;
+	struct device *dev;
+
+	if (!fwnode)
+		return NULL;
+
+	dev = bus_find_device_by_fwnode(&i2c_bus_type, fwnode);
+	if (!dev)
+		return NULL;
+
+	client = i2c_verify_client(dev);
+	if (!client)
+		put_device(dev);
+
+	return client;
+}
+EXPORT_SYMBOL(i2c_find_device_by_fwnode);
+
 
 static const struct i2c_device_id dummy_id[] = {
 	{ "dummy", 0 },
@@ -1762,6 +1791,75 @@ int devm_i2c_add_adapter(struct device *dev, struct i2c_adapter *adapter)
 }
 EXPORT_SYMBOL_GPL(devm_i2c_add_adapter);
 
+static int i2c_dev_or_parent_fwnode_match(struct device *dev, const void *data)
+{
+	if (dev_fwnode(dev) == data)
+		return 1;
+
+	if (dev->parent && dev_fwnode(dev->parent) == data)
+		return 1;
+
+	return 0;
+}
+
+/**
+ * i2c_find_adapter_by_fwnode() - find an i2c_adapter for the fwnode
+ * @fwnode: &struct fwnode_handle corresponding to the &struct i2c_adapter
+ *
+ * Look up and return the &struct i2c_adapter corresponding to the @fwnode.
+ * If no adapter can be found, or @fwnode is NULL, this returns NULL.
+ *
+ * The user must call put_device(&adapter->dev) once done with the i2c adapter.
+ */
+struct i2c_adapter *i2c_find_adapter_by_fwnode(struct fwnode_handle *fwnode)
+{
+	struct i2c_adapter *adapter;
+	struct device *dev;
+
+	if (!fwnode)
+		return NULL;
+
+	dev = bus_find_device(&i2c_bus_type, NULL, fwnode,
+			      i2c_dev_or_parent_fwnode_match);
+	if (!dev)
+		return NULL;
+
+	adapter = i2c_verify_adapter(dev);
+	if (!adapter)
+		put_device(dev);
+
+	return adapter;
+}
+EXPORT_SYMBOL(i2c_find_adapter_by_fwnode);
+
+/**
+ * i2c_get_adapter_by_fwnode() - find an i2c_adapter for the fwnode
+ * @fwnode: &struct fwnode_handle corresponding to the &struct i2c_adapter
+ *
+ * Look up and return the &struct i2c_adapter corresponding to the @fwnode,
+ * and increment the adapter module's use count. If no adapter can be found,
+ * or @fwnode is NULL, this returns NULL.
+ *
+ * The user must call i2c_put_adapter(adapter) once done with the i2c adapter.
+ * Note that this is different from i2c_find_adapter_by_node().
+ */
+struct i2c_adapter *i2c_get_adapter_by_fwnode(struct fwnode_handle *fwnode)
+{
+	struct i2c_adapter *adapter;
+
+	adapter = i2c_find_adapter_by_fwnode(fwnode);
+	if (!adapter)
+		return NULL;
+
+	if (!try_module_get(adapter->owner)) {
+		put_device(&adapter->dev);
+		adapter = NULL;
+	}
+
+	return adapter;
+}
+EXPORT_SYMBOL(i2c_get_adapter_by_fwnode);
+
 static void i2c_parse_timing(struct device *dev, char *prop_name, u32 *cur_val_p,
 			    u32 def_val, bool use_def)
 {
diff --git a/drivers/i2c/i2c-core-of.c b/drivers/i2c/i2c-core-of.c
index 3ed74aa4b44b..c3e565e4bddf 100644
--- a/drivers/i2c/i2c-core-of.c
+++ b/drivers/i2c/i2c-core-of.c
@@ -113,69 +113,24 @@ 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)
 {
-	struct device *dev;
-	struct i2c_client *client;
-
-	dev = bus_find_device_by_of_node(&i2c_bus_type, node);
-	if (!dev)
-		return NULL;
-
-	client = i2c_verify_client(dev);
-	if (!client)
-		put_device(dev);
-
-	return client;
+	return i2c_find_device_by_fwnode(of_fwnode_handle(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;
+	return i2c_find_adapter_by_fwnode(of_fwnode_handle(node));
 }
 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)
 {
-	struct i2c_adapter *adapter;
-
-	adapter = of_find_i2c_adapter_by_node(node);
-	if (!adapter)
-		return NULL;
-
-	if (!try_module_get(adapter->owner)) {
-		put_device(&adapter->dev);
-		adapter = NULL;
-	}
-
-	return adapter;
+	return i2c_get_adapter_by_fwnode(of_fwnode_handle(node));
 }
 EXPORT_SYMBOL(of_get_i2c_adapter_by_node);
 
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index d84e0e99f084..bcee9faaf2e6 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -965,6 +965,15 @@ int i2c_handle_smbus_host_notify(struct i2c_adapter *adap, unsigned short addr);
 
 #endif /* I2C */
 
+/* must call put_device() when done with returned i2c_client device */
+struct i2c_client *i2c_find_device_by_fwnode(struct fwnode_handle *fwnode);
+
+/* must call put_device() when done with returned i2c_adapter device */
+struct i2c_adapter *i2c_find_adapter_by_fwnode(struct fwnode_handle *fwnode);
+
+/* must call i2c_put_adapter() when done with returned i2c_adapter device */
+struct i2c_adapter *i2c_get_adapter_by_fwnode(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.30.2

Re: [PATCH RFC net-next v2 1/2] i2c: add fwnode APIs

[Mika Westerberg]

On Mon, Dec 19, 2022 at 09:52:02AM +0000, Russell King (Oracle) wrote:
> Add fwnode APIs for finding and getting I2C adapters, which will be
> used by the SFP code. These are passed the fwnode corresponding to
> the adapter, and return the I2C adapter. It is the responsibility of
> the caller to find the appropriate fwnode.
> 
> We keep the DT and ACPI interfaces, but where appropriate, recode them
> to use the fwnode interfaces internally.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>

[PATCH RFC net-next v2 2/2] net: sfp: use i2c_get_adapter_by_fwnode()

[Russell King (Oracle)]

Use the newly introduced i2c_get_adapter_by_fwnode() API, so that we
can retrieve the I2C adapter in a firmware independent manner once we
have the fwnode handle for the adapter.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/phy/sfp.c | 13 +------------
 1 file changed, 1 insertion(+), 12 deletions(-)

diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c
index 83b99d95b278..aa2f7ebbdebc 100644
--- a/drivers/net/phy/sfp.c
+++ b/drivers/net/phy/sfp.c
@@ -2644,10 +2644,8 @@ static void sfp_cleanup(void *data)
 
 static int sfp_i2c_get(struct sfp *sfp)
 {
-	struct acpi_handle *acpi_handle;
 	struct fwnode_handle *h;
 	struct i2c_adapter *i2c;
-	struct device_node *np;
 	int err;
 
 	h = fwnode_find_reference(dev_fwnode(sfp->dev), "i2c-bus", 0);
@@ -2656,16 +2654,7 @@ static int sfp_i2c_get(struct sfp *sfp)
 		return -ENODEV;
 	}
 
-	if (is_acpi_device_node(h)) {
-		acpi_handle = ACPI_HANDLE_FWNODE(h);
-		i2c = i2c_acpi_find_adapter_by_handle(acpi_handle);
-	} else if ((np = to_of_node(h)) != NULL) {
-		i2c = of_find_i2c_adapter_by_node(np);
-	} else {
-		err = -EINVAL;
-		goto put;
-	}
-
+	i2c = i2c_get_adapter_by_fwnode(h);
 	if (!i2c) {
 		err = -EPROBE_DEFER;
 		goto put;
-- 
2.30.2

Re: [PATCH RFC net-next v2 2/2] net: sfp: use i2c_get_adapter_by_fwnode()

[Mika Westerberg]

On Mon, Dec 19, 2022 at 09:52:07AM +0000, Russell King (Oracle) wrote:
> Use the newly introduced i2c_get_adapter_by_fwnode() API, so that we
> can retrieve the I2C adapter in a firmware independent manner once we
> have the fwnode handle for the adapter.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Re: [PATCH RFC v2 0/2] Add I2C fwnode lookup/get interfaces

[Russell King (Oracle)]

Hi Wolfram, David, Eric, Paolo,

How would you like to handle merging these patches? I'm not expecting
any changes during this cycle which would conflict with the sfp.c
changes in this series, so the series could be merged through the i2c
tree. However, I am intending to send additional sfp.c changes which
are independent of this.

Thanks.

On Mon, Dec 19, 2022 at 09:50:19AM +0000, Russell King (Oracle) wrote:
> Hi,
> 
> This RFC series is intended for the next merge window, but we will need
> to decide how to merge it as it is split across two subsystems. These
> patches have been generated against the net-next, since patch 2 depends
> on a recently merged patch in that tree (which is now in mainline.)
> 
> Currently, the SFP code attempts to work out what kind of fwnode we
> found when looking up the I2C bus for the SFP cage, converts the fwnode
> to the appropriate firmware specific representation to then call the
> appropriate I2C layer function. This is inefficient, since the device
> model provides a way to locate items on a bus_type by fwnode.
> 
> In order to reduce this complexity, this series adds fwnode interfaces
> to the I2C subsystem to allow I2C adapters to be looked up. I also
> accidentally also converted the I2C clients to also be looked up, so
> I've left that in patch 1 if people think that could be useful - if
> not, I'll remove it.
> 
> We could also convert the of_* functions to be inline in i2c.h and
> remove the stub of_* functions and exports.
> 
> Do we want these to live in i2c-core-fwnode.c ? I don't see a Kconfig
> symbol that indicates whether we want fwnode support, and I know there
> are people looking to use software nodes to lookup the SFP I2C bus
> (which is why the manual firmware-specific code in sfp.c is a problem.)
> 
> Thanks!
> 
> v2: updated patch 1 with docbook comments.
> 
>  drivers/i2c/i2c-core-acpi.c | 13 +-----
>  drivers/i2c/i2c-core-base.c | 98 +++++++++++++++++++++++++++++++++++++++++++++
>  drivers/i2c/i2c-core-of.c   | 51 ++---------------------
>  drivers/net/phy/sfp.c       | 13 +-----
>  include/linux/i2c.h         |  9 +++++
>  5 files changed, 112 insertions(+), 72 deletions(-)
> 
> -- 
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
> 

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH RFC v2 0/2] Add I2C fwnode lookup/get interfaces

[Wolfram Sang]

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

Hi Russell,

thank you for this series!

> This RFC series is intended for the next merge window, but we will need
> to decide how to merge it as it is split across two subsystems. These
> patches have been generated against the net-next, since patch 2 depends
> on a recently merged patch in that tree (which is now in mainline.)

I'd prefer to apply it all to my I2C tree then. I can also provide an
immutable branch for net if that is helpful.

> In order to reduce this complexity, this series adds fwnode interfaces
> to the I2C subsystem to allow I2C adapters to be looked up. I also
> accidentally also converted the I2C clients to also be looked up, so
> I've left that in patch 1 if people think that could be useful - if
> not, I'll remove it.

Because you also converted I2C ACPI to use the new function, I'd say
let's keep it.

> We could also convert the of_* functions to be inline in i2c.h and
> remove the stub of_* functions and exports.

I'd like that.

> Do we want these to live in i2c-core-fwnode.c ? I don't see a Kconfig

I don't think this is enough fwnode-specific code yet for a seperate
source file. I also don't think the helper functions are so large that
there should be an option to compile them out. I am open for other
opinions, but IMHO that part looks good as it is.

> symbol that indicates whether we want fwnode support, and I know there
> are people looking to use software nodes to lookup the SFP I2C bus
> (which is why the manual firmware-specific code in sfp.c is a problem.)

All the best,

   Wolfram


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

Re: [PATCH RFC v2 0/2] Add I2C fwnode lookup/get interfaces

[Russell King (Oracle)]

On Mon, Jan 09, 2023 at 12:48:37PM +0100, Wolfram Sang wrote:
> > This RFC series is intended for the next merge window, but we will need
> > to decide how to merge it as it is split across two subsystems. These
> > patches have been generated against the net-next, since patch 2 depends
> > on a recently merged patch in that tree (which is now in mainline.)
> 
> I'd prefer to apply it all to my I2C tree then. I can also provide an
> immutable branch for net if that is helpful.

If we go for the immutable branch, then patch 2 might as well be
merged via the net tree, if net-next is willing to pull your
immutable branch.

Dave? Jakub? Paolo? Do you have any preferences how you'd like to
handle this?

Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH RFC v2 0/2] Add I2C fwnode lookup/get interfaces

[Jakub Kicinski]

On Tue, 10 Jan 2023 13:02:36 +0000 Russell King (Oracle) wrote:
> On Mon, Jan 09, 2023 at 12:48:37PM +0100, Wolfram Sang wrote:
> > > This RFC series is intended for the next merge window, but we will need
> > > to decide how to merge it as it is split across two subsystems. These
> > > patches have been generated against the net-next, since patch 2 depends
> > > on a recently merged patch in that tree (which is now in mainline.)  
> > 
> > I'd prefer to apply it all to my I2C tree then. I can also provide an
> > immutable branch for net if that is helpful.  
> 
> If we go for the immutable branch, then patch 2 might as well be
> merged via the net tree, if net-next is willing to pull your
> immutable branch.
> 
> Dave? Jakub? Paolo? Do you have any preferences how you'd like to
> handle this?

No strong preference here. Immutable branch works.
Patch 2 will stick out in the diffstat for i2c so may indeed be better
to apply it to net-next only, then again perhaps Wolfram prefers to
have the user merged with the API? We're fine either way.