[RFC PATCH 0/3] i2c: Introduce i2c bus extensions

[RFC PATCH 0/3] i2c: Introduce i2c bus extensions

[Herve Codina]

The big picture behind this RFC series is to support a Linux device
with a connector to physically add and remove an add-on to/from the
main device to augment its features at runtime, adding devices on
non-discoverable busses, using device tree overlays.

The related big picture has been already presented in
  - the 'Add support for GE SUNH hot-pluggable connector' series [0]
  - the 'Runtime hotplug on non-discoverable busses with device tree
    overlays' talk at Linux Plumbers Conference 2024 [1].

This series focuses on the i2c related part.

An i2c bus is wired to the connector and allows an add-on board to
connect additional i2c devices to this bus.

When device tree nodes are added, the I2C core tries to probe client
devices based on the classic DT structure:

  i2c@abcd0000 {
      some-client@42 { compatible = "xyz,blah"; ... };
  };

However for hotplug connectors described via device tree overlays [0]
there is additional level of indirection, which is needed to decouple
the overlay and the base tree:

  --- base device tree ---

  i2c1: i2c@abcd0000 {
      compatible = "xyz,i2c-ctrl";
      i2c-bus-extension@0 {
          i2c-bus = <&i2c_ctrl>;
      };
      ...
  };

  i2c5: i2c@cafe0000 {
      compatible = "xyz,i2c-ctrl";
      i2c-bus-extension@0 {
          i2c-bus = <&i2c-sensors>;
      };
      ...
  };

  connector {
      i2c_ctrl: i2c-ctrl {
          i2c-parent = <&i2c1>;
          #address-cells = <1>;
          #size-cells = <0>;
      };

      i2c-sensors {
          i2c-parent = <&i2c5>;
          #address-cells = <1>;
          #size-cells = <0>;
      };
  };

  --- device tree overlay ---

  ...
  // This node will overlay on the i2c-ctrl node of the base tree
  i2c-ctrl {
      eeprom@50 { compatible = "atmel,24c64"; ... };
  };
  ...

  --- resulting device tree ---

  i2c1: i2c@abcd0000 {
      compatible = "xyz,i2c-ctrl";
      i2c-bus-extension@0 {
          i2c-bus = <&i2c_ctrl>;
      };
      ...
  };

  i2c5: i2c@cafe0000 {
      compatible = "xyz,i2c-ctrl";
      i2c-bus-extension@0 {
          i2c-bus = <&i2c-sensors>;
      };
      ...
  };

  connector {
      i2c-ctrl {
          i2c-parent = <&i2c1>;
          #address-cells = <1>;
          #size-cells = <0>;

          eeprom@50 { compatible = "atmel,24c64"; ... };
      };

      i2c-sensors {
          i2c-parent = <&i2c5>;
          #address-cells = <1>;
          #size-cells = <0>;
      };
  };

Here i2c-ctrl (same goes for i2c-sensors) represent the part of I2C bus
that is on the hot-pluggable add-on. On hot-plugging it will physically
connect to the I2C adapter on the base board. Let's call the 'i2c-ctrl'
node an "extension node".

In order to decouple the overlay from the base tree, the I2C adapter
(i2c@abcd0000) and the extension node (i2c-ctrl) are separate nodes.
Rightfully, only the former will probe into an I2C adapter, and it will
do that perhaps during boot, long before overlay insertion or after the
overlay has been inserted for instance if the I2C adapter is remove and
re-probed for whatever reason after the overlay insertion.

The extension node won't probe into an I2C adapter or any other device
or bus, so its subnodes ('eeprom@50') won't be interpreted as I2C
clients by current I2C core code.

The extension node is linked to the adapter node in two ways. The first
one with the i2c-bus-extension adapter sub-node and the second one with
the i2c-parent property in the extension node itself.

The purpose of those two links is to handle device probing in several
cases.

- First case: Adapter already probed when add-on devices are added

When devices are added by the overlay, an OF change notification is
triggered so that busses can support those new devices.

Going from a newly added device node, the i2c-parent property allows to
find the corresponding I2C adapter and register the new I2C client with
this adapter.

The patch 1 in this series proposes modification to handle this case
and, by the nature of the modification, all cases where a phandle refers
an extension node instead of the adapter node itself.

- Second case: Add-on devices already present in device-tree when
  adapter is probed

In this case, everything is already described in the device-tree and
then the adapter is probed.

OF change notifications don't play a role in this case either because
they were never triggered (the overlay was applied by the bootloader)
or they were triggered before the adapter is probed and so were
missed/ignored.

The adapter probe process registers device already described at the
adapter node level (current code) and, thanks to i2c-bus-extension
adapter sub-node and its i2c-bus property, it can also follow the
extension and registers devices described in those extension nodes.

The patch 2 and 3 in this series proposes modifications to handle this
case.

I know device-tree bindings for i2c-bus-extension and i2c-parent are not
yet provided in this RFC series.

I would like to discuss the proposal before going further and write
those needed bindinds (i2c-bus-extension needs to be added in
i2c-controller.yaml available in dt-schema repository [2]).

Best regards,
Hervé Codina

[0] https://lore.kernel.org/all/20240917-hotplug-drm-bridge-v4-0-bc4dfee61be6@bootlin.com/
[1] https://lpc.events/event/18/contributions/1696/
[2] https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/i2c/i2c-controller.yaml

Herve Codina (3):
  i2c: core: Follow i2c-parent when retrieving an adapter from node
  i2c: i2c-core-of: Move children registration in a dedicated function
  i2c: i2c-core-of: Handle i2c bus extensions

 drivers/i2c/i2c-core-base.c | 43 ++++++++++++++++++++++++++++-
 drivers/i2c/i2c-core-of.c   | 54 +++++++++++++++++++++++++++++--------
 2 files changed, 85 insertions(+), 12 deletions(-)

-- 
2.47.1

[RFC PATCH 1/3] i2c: core: Follow i2c-parent when retrieving an adapter from node

[Herve Codina]

i2c bus extensions were introduced to decouple i2c busses when they are
wired to connectors. Combined with devicetree overlays, they introduce
an additional level of indirection, which is needed to decouple the
overlay (describing the hardware available on addon baord) and the base
tree (describing resources provided to the addon board).

For instance, the following devicetree fragment, available once
overlays are applied, is legit:

    i2c1: i2c@abcd0000 {
        compatible = "xyz,i2c-ctrl";
        i2c-bus-extension@0 {
            reg = <0>;
            i2c-bus = <&i2c-ctrl>;
        };
        ...
    };

    connector {
        i2c-ctrl {
            i2c-parent = <&i2c1>;
            #address-cells = <1>;
            #size-cells = <0>;

            i2c-bus-extension@0 {
                reg = <0>;
                i2c-bus = <&i2c-other-connector>;
            };

            device@10 {
                compatible = "xyz,foo";
                reg = <0x10>;
            };
        };

        devices {
            other-connector {
                i2c-at-other-connector {
                    i2c-parent = <&i2c-ctrl>;
                    #address-cells = <1>;
                    #size-cells = <0>;

                    device@20 {
                       compatible = "xyz,bar";
                       reg = <0x20>;
                    };
                };
            };
        };
    };

Current implementation of i2c_find_adapter_by_fwnode() supposes that the
fwnode parameter correspond to the adapter.

This assumption is no more valid with i2c bus extensions. Indeed, the
fwnode parameter can reference an i2c bus extension node and not the
related adapter.

In the example, i2c-ctrl and i2c-at-other-connector nodes are chained
bus extensions and those node can be passed to
i2c_find_adapter_by_fwnode() in order to get the related adapter (i.e
the adapter handling the bus and its extensions: i2c@abcd0000).

Extend i2c_find_adapter_by_fwnode() to perform the walking from the
given fwnode through i2c-parent references up to the adapter.

Signed-off-by: Herve Codina <herve.codina@bootlin.com>
---
 drivers/i2c/i2c-core-base.c | 43 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 42 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 5546184df05f..cb7adc5c1285 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -1827,14 +1827,55 @@ static int i2c_dev_or_parent_fwnode_match(struct device *dev, const void *data)
  */
 struct i2c_adapter *i2c_find_adapter_by_fwnode(struct fwnode_handle *fwnode)
 {
+	struct fwnode_reference_args args;
+	struct fwnode_handle *adap_fwnode;
 	struct i2c_adapter *adapter;
 	struct device *dev;
+	int err;
 
 	if (!fwnode)
 		return NULL;
 
-	dev = bus_find_device(&i2c_bus_type, NULL, fwnode,
+	adap_fwnode = fwnode_handle_get(fwnode);
+
+	/* Walk extension busses (through i2c-parent) up to the adapter node */
+	while (fwnode_property_present(adap_fwnode, "i2c-parent")) {
+		/*
+		 * A specific case exists for the i2c demux pinctrl. The i2c bus
+		 * node related this component (the i2c demux pinctrl node
+		 * itself) has an i2c-parent property set. This property is used
+		 * by the i2c demux pinctrl component for the demuxing purpose
+		 * and is not related to the extension bus feature.
+		 *
+		 * In this current i2c-parent walking, the i2c demux pinctrl
+		 * node has to be considered as an adapter node and so, if
+		 * the adap_fwnode node is an i2c demux pinctrl node, simply
+		 * stop the i2c-parent walking.
+		 */
+		if (fwnode_property_match_string(adap_fwnode, "compatible",
+						 "i2c-demux-pinctrl") >= 0)
+			break;
+
+		/*
+		 * i2c-parent property available in a i2c bus node means that
+		 * this node is an extension bus node. In that case,
+		 * continue i2c-parent walking up to the adapter node.
+		 */
+		err = fwnode_property_get_reference_args(adap_fwnode, "i2c-parent",
+							 NULL, 0, 0, &args);
+		if (err)
+			break;
+
+		pr_debug("Find adapter for %pfw, use parent: %pfw\n", fwnode,
+			 args.fwnode);
+
+		fwnode_handle_put(adap_fwnode);
+		adap_fwnode = args.fwnode;
+	}
+
+	dev = bus_find_device(&i2c_bus_type, NULL, adap_fwnode,
 			      i2c_dev_or_parent_fwnode_match);
+	fwnode_handle_put(adap_fwnode);
 	if (!dev)
 		return NULL;
 
-- 
2.47.1

[RFC PATCH 2/3] i2c: i2c-core-of: Move children registration in a dedicated function

[Herve Codina]

of_i2c_register_devices() perform the loop for registering child devices.

In order to prepare the support for i2c bus extensions, extract this
registration loop and move it in a dedicated function.

Signed-off-by: Herve Codina <herve.codina@bootlin.com>
---
 drivers/i2c/i2c-core-of.c | 30 +++++++++++++++++++-----------
 1 file changed, 19 insertions(+), 11 deletions(-)

diff --git a/drivers/i2c/i2c-core-of.c b/drivers/i2c/i2c-core-of.c
index a6c407d36800..b4c9db137f5a 100644
--- a/drivers/i2c/i2c-core-of.c
+++ b/drivers/i2c/i2c-core-of.c
@@ -82,21 +82,15 @@ static struct i2c_client *of_i2c_register_device(struct i2c_adapter *adap,
 	return client;
 }
 
-void of_i2c_register_devices(struct i2c_adapter *adap)
+static void of_i2c_register_children(struct i2c_adapter *adap,
+				     struct device_node *bus)
 {
-	struct device_node *bus, *node;
 	struct i2c_client *client;
+	struct device_node *node;
 
-	/* Only register child devices if the adapter has a node pointer set */
-	if (!adap->dev.of_node)
-		return;
-
-	dev_dbg(&adap->dev, "of_i2c: walking child nodes\n");
-
-	bus = of_get_child_by_name(adap->dev.of_node, "i2c-bus");
-	if (!bus)
-		bus = of_node_get(adap->dev.of_node);
+	dev_dbg(&adap->dev, "of_i2c: walking child nodes from %pOF\n", bus);
 
+	/* Register device directly attached to this bus */
 	for_each_available_child_of_node(bus, node) {
 		if (of_node_test_and_set_flag(node, OF_POPULATED))
 			continue;
@@ -109,7 +103,21 @@ void of_i2c_register_devices(struct i2c_adapter *adap)
 			of_node_clear_flag(node, OF_POPULATED);
 		}
 	}
+}
+
+void of_i2c_register_devices(struct i2c_adapter *adap)
+{
+	struct device_node *bus;
+
+	/* Only register child devices if the adapter has a node pointer set */
+	if (!adap->dev.of_node)
+		return;
+
+	bus = of_get_child_by_name(adap->dev.of_node, "i2c-bus");
+	if (!bus)
+		bus = of_node_get(adap->dev.of_node);
 
+	of_i2c_register_children(adap, bus);
 	of_node_put(bus);
 }
 
-- 
2.47.1

[RFC PATCH 3/3] i2c: i2c-core-of: Handle i2c bus extensions

[Herve Codina]

i2c bus extensions were introduced to decouple i2c busses when they are
wired to connectors. Combined with devicetree overlays, they introduce
an additional level of indirection, which is needed to decouple the
overlay (describing the hardware available on addon baord) and the base
tree (describing resources provided to the addon board).

For instance, the following devicetree fragment, available once
overlays are applied, is legit:

    i2c1: i2c@abcd0000 {
        compatible = "xyz,i2c-ctrl";
        i2c-bus-extension@0 {
            reg = <0>;
            i2c-bus = <&i2c-ctrl>;
        };
        ...
    };

    connector {
        i2c-ctrl {
            i2c-parent = <&i2c1>;
            #address-cells = <1>;
            #size-cells = <0>;

            i2c-bus-extension@0 {
                reg = <0>;
                i2c-bus = <&i2c-other-connector>;
            };

            device@10 {
                compatible = "xyz,foo";
                reg = <0x10>;
            };
        };

        devices {
            other-connector {
                i2c-at-other-connector {
                    i2c-parent = <&i2c-ctrl>;
                    #address-cells = <1>;
                    #size-cells = <0>;

                    device@20 {
                       compatible = "xyz,bar";
                       reg = <0x20>;
                    };
                };
            };
        };
    };

Current processing done when a i2c adapter is registered registers
i2c clients described at the adapter node level.

With i2c bus extensions, the process needs to look also at
extensions to register devices described in those extensions and so
connected to the adapter.

Extend of_i2c_register_children() to look recursively at those
i2c bus extensions.

Signed-off-by: Herve Codina <herve.codina@bootlin.com>
---
 drivers/i2c/i2c-core-of.c | 28 ++++++++++++++++++++++++++--
 1 file changed, 26 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/i2c-core-of.c b/drivers/i2c/i2c-core-of.c
index b4c9db137f5a..406565beabbe 100644
--- a/drivers/i2c/i2c-core-of.c
+++ b/drivers/i2c/i2c-core-of.c
@@ -85,13 +85,20 @@ static struct i2c_client *of_i2c_register_device(struct i2c_adapter *adap,
 static void of_i2c_register_children(struct i2c_adapter *adap,
 				     struct device_node *bus)
 {
+	struct device_node *node, *extension;
 	struct i2c_client *client;
-	struct device_node *node;
 
 	dev_dbg(&adap->dev, "of_i2c: walking child nodes from %pOF\n", bus);
 
-	/* Register device directly attached to this bus */
+	/*
+	 * Register device directly described in this bus node before looking
+	 * at extensions.
+	 */
 	for_each_available_child_of_node(bus, node) {
+		/* Filter out extension node */
+		if (of_node_name_eq(node, "i2c-bus-extension"))
+			continue;
+
 		if (of_node_test_and_set_flag(node, OF_POPULATED))
 			continue;
 
@@ -103,6 +110,23 @@ static void of_i2c_register_children(struct i2c_adapter *adap,
 			of_node_clear_flag(node, OF_POPULATED);
 		}
 	}
+
+	/* Look at extensions */
+	for_each_available_child_of_node(bus, node) {
+		if (!of_node_name_eq(node, "i2c-bus-extension"))
+			continue;
+
+		extension = of_parse_phandle(node, "i2c-bus", 0);
+		if (!extension)
+			continue;
+
+		/*
+		 * Register children available at this extension possibly
+		 * walking other chained extensions.
+		 */
+		of_i2c_register_children(adap, extension);
+		of_node_put(extension);
+	}
 }
 
 void of_i2c_register_devices(struct i2c_adapter *adap)
-- 
2.47.1

Re: [RFC PATCH 3/3] i2c: i2c-core-of: Handle i2c bus extensions

[Krzysztof Kozlowski]

On 05/02/2025 18:39, Herve Codina wrote:
>  
>  	dev_dbg(&adap->dev, "of_i2c: walking child nodes from %pOF\n", bus);
>  
> -	/* Register device directly attached to this bus */
> +	/*
> +	 * Register device directly described in this bus node before looking
> +	 * at extensions.
> +	 */
>  	for_each_available_child_of_node(bus, node) {
> +		/* Filter out extension node */
> +		if (of_node_name_eq(node, "i2c-bus-extension"))

Where is the ABI documented?

> +			continue;
> +
>  		if (of_node_test_and_set_flag(node, OF_POPULATED))
>  			continue;
>  
> @@ -103,6 +110,23 @@ static void of_i2c_register_children(struct i2c_adapter *adap,
>  			of_node_clear_flag(node, OF_POPULATED);
>  		}
>  	}
> +
> +	/* Look at extensions */
> +	for_each_available_child_of_node(bus, node) {
> +		if (!of_node_name_eq(node, "i2c-bus-extension"))
> +			continue;
> +
> +		extension = of_parse_phandle(node, "i2c-bus", 0);

And this?

> +		if (!extension)
> +			continue;
> +
Best regards,
Krzysztof

Re: [RFC PATCH 3/3] i2c: i2c-core-of: Handle i2c bus extensions

[Herve Codina]

Hi Krzysztof,

On Wed, 12 Feb 2025 06:54:19 +0100
Krzysztof Kozlowski <krzk@kernel.org> wrote:

> On 05/02/2025 18:39, Herve Codina wrote:
> >  
> >  	dev_dbg(&adap->dev, "of_i2c: walking child nodes from %pOF\n", bus);
> >  
> > -	/* Register device directly attached to this bus */
> > +	/*
> > +	 * Register device directly described in this bus node before looking
> > +	 * at extensions.
> > +	 */
> >  	for_each_available_child_of_node(bus, node) {
> > +		/* Filter out extension node */
> > +		if (of_node_name_eq(node, "i2c-bus-extension"))  
> 
> Where is the ABI documented?
> 
> > +			continue;
> > +
> >  		if (of_node_test_and_set_flag(node, OF_POPULATED))
> >  			continue;
> >  
> > @@ -103,6 +110,23 @@ static void of_i2c_register_children(struct i2c_adapter *adap,
> >  			of_node_clear_flag(node, OF_POPULATED);
> >  		}
> >  	}
> > +
> > +	/* Look at extensions */
> > +	for_each_available_child_of_node(bus, node) {
> > +		if (!of_node_name_eq(node, "i2c-bus-extension"))
> > +			continue;
> > +
> > +		extension = of_parse_phandle(node, "i2c-bus", 0);  
> 
> And this?
> 
> > +		if (!extension)
> > +			continue;
> > +  

I know the binding is not present in this RFC series.

As I mentioned in my cover letter, the binding that needs to be updated is
available in dt-schema repo [0].

When the binding is be accepted in dt-schema repo, I will not be able to
change it and because two repos are involved, I cannot send the binding and
the implementation in the same series.

Before sending a patch to update the binding in dt-schema repo, I would
like first to discuss the proposed i2c bus extension idea in terms of:
  1) DT properties naming and purpose
  2) implementation

[0] https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/i2c/i2c-controller.yaml

Best regards,
Hervé

Re: [RFC PATCH 0/3] i2c: Introduce i2c bus extensions

[Herve Codina]

Hi Wolfram,

In order for me to move forward on this new feature sending a patch for the
binding or reworking my proposal can you provide feedback on the topic and
the current implementation available in this RFC.

Best regards,
Hervé

On Wed,  5 Feb 2025 18:39:13 +0100
Herve Codina <herve.codina@bootlin.com> wrote:

> The big picture behind this RFC series is to support a Linux device
> with a connector to physically add and remove an add-on to/from the
> main device to augment its features at runtime, adding devices on
> non-discoverable busses, using device tree overlays.
> 
> The related big picture has been already presented in
>   - the 'Add support for GE SUNH hot-pluggable connector' series [0]
>   - the 'Runtime hotplug on non-discoverable busses with device tree
>     overlays' talk at Linux Plumbers Conference 2024 [1].
> 
> This series focuses on the i2c related part.
> 
> An i2c bus is wired to the connector and allows an add-on board to
> connect additional i2c devices to this bus.
> 
> When device tree nodes are added, the I2C core tries to probe client
> devices based on the classic DT structure:
> 
>   i2c@abcd0000 {
>       some-client@42 { compatible = "xyz,blah"; ... };
>   };
> 
> However for hotplug connectors described via device tree overlays [0]
> there is additional level of indirection, which is needed to decouple
> the overlay and the base tree:
> 
>   --- base device tree ---
> 
>   i2c1: i2c@abcd0000 {
>       compatible = "xyz,i2c-ctrl";
>       i2c-bus-extension@0 {
>           i2c-bus = <&i2c_ctrl>;
>       };
>       ...
>   };
> 
>   i2c5: i2c@cafe0000 {
>       compatible = "xyz,i2c-ctrl";
>       i2c-bus-extension@0 {
>           i2c-bus = <&i2c-sensors>;
>       };
>       ...
>   };
> 
>   connector {
>       i2c_ctrl: i2c-ctrl {
>           i2c-parent = <&i2c1>;
>           #address-cells = <1>;
>           #size-cells = <0>;
>       };
> 
>       i2c-sensors {
>           i2c-parent = <&i2c5>;
>           #address-cells = <1>;
>           #size-cells = <0>;
>       };
>   };
> 
>   --- device tree overlay ---
> 
>   ...
>   // This node will overlay on the i2c-ctrl node of the base tree
>   i2c-ctrl {
>       eeprom@50 { compatible = "atmel,24c64"; ... };
>   };
>   ...
> 
>   --- resulting device tree ---
> 
>   i2c1: i2c@abcd0000 {
>       compatible = "xyz,i2c-ctrl";
>       i2c-bus-extension@0 {
>           i2c-bus = <&i2c_ctrl>;
>       };
>       ...
>   };
> 
>   i2c5: i2c@cafe0000 {
>       compatible = "xyz,i2c-ctrl";
>       i2c-bus-extension@0 {
>           i2c-bus = <&i2c-sensors>;
>       };
>       ...
>   };
> 
>   connector {
>       i2c-ctrl {
>           i2c-parent = <&i2c1>;
>           #address-cells = <1>;
>           #size-cells = <0>;
> 
>           eeprom@50 { compatible = "atmel,24c64"; ... };
>       };
> 
>       i2c-sensors {
>           i2c-parent = <&i2c5>;
>           #address-cells = <1>;
>           #size-cells = <0>;
>       };
>   };
> 
> Here i2c-ctrl (same goes for i2c-sensors) represent the part of I2C bus
> that is on the hot-pluggable add-on. On hot-plugging it will physically
> connect to the I2C adapter on the base board. Let's call the 'i2c-ctrl'
> node an "extension node".
> 
> In order to decouple the overlay from the base tree, the I2C adapter
> (i2c@abcd0000) and the extension node (i2c-ctrl) are separate nodes.
> Rightfully, only the former will probe into an I2C adapter, and it will
> do that perhaps during boot, long before overlay insertion or after the
> overlay has been inserted for instance if the I2C adapter is remove and
> re-probed for whatever reason after the overlay insertion.
> 
> The extension node won't probe into an I2C adapter or any other device
> or bus, so its subnodes ('eeprom@50') won't be interpreted as I2C
> clients by current I2C core code.
> 
> The extension node is linked to the adapter node in two ways. The first
> one with the i2c-bus-extension adapter sub-node and the second one with
> the i2c-parent property in the extension node itself.
> 
> The purpose of those two links is to handle device probing in several
> cases.
> 
> - First case: Adapter already probed when add-on devices are added
> 
> When devices are added by the overlay, an OF change notification is
> triggered so that busses can support those new devices.
> 
> Going from a newly added device node, the i2c-parent property allows to
> find the corresponding I2C adapter and register the new I2C client with
> this adapter.
> 
> The patch 1 in this series proposes modification to handle this case
> and, by the nature of the modification, all cases where a phandle refers
> an extension node instead of the adapter node itself.
> 
> - Second case: Add-on devices already present in device-tree when
>   adapter is probed
> 
> In this case, everything is already described in the device-tree and
> then the adapter is probed.
> 
> OF change notifications don't play a role in this case either because
> they were never triggered (the overlay was applied by the bootloader)
> or they were triggered before the adapter is probed and so were
> missed/ignored.
> 
> The adapter probe process registers device already described at the
> adapter node level (current code) and, thanks to i2c-bus-extension
> adapter sub-node and its i2c-bus property, it can also follow the
> extension and registers devices described in those extension nodes.
> 
> The patch 2 and 3 in this series proposes modifications to handle this
> case.
> 
> I know device-tree bindings for i2c-bus-extension and i2c-parent are not
> yet provided in this RFC series.
> 
> I would like to discuss the proposal before going further and write
> those needed bindinds (i2c-bus-extension needs to be added in
> i2c-controller.yaml available in dt-schema repository [2]).
> 
> Best regards,
> Hervé Codina
> 
> [0] https://lore.kernel.org/all/20240917-hotplug-drm-bridge-v4-0-bc4dfee61be6@bootlin.com/
> [1] https://lpc.events/event/18/contributions/1696/
> [2] https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/i2c/i2c-controller.yaml
> 
> Herve Codina (3):
>   i2c: core: Follow i2c-parent when retrieving an adapter from node
>   i2c: i2c-core-of: Move children registration in a dedicated function
>   i2c: i2c-core-of: Handle i2c bus extensions
> 
>  drivers/i2c/i2c-core-base.c | 43 ++++++++++++++++++++++++++++-
>  drivers/i2c/i2c-core-of.c   | 54 +++++++++++++++++++++++++++++--------
>  2 files changed, 85 insertions(+), 12 deletions(-)
> 

Re: [RFC PATCH 0/3] i2c: Introduce i2c bus extensions

[Wolfram Sang]

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

Hi Herve,

> The related big picture has been already presented in
>   - the 'Add support for GE SUNH hot-pluggable connector' series [0]
>   - the 'Runtime hotplug on non-discoverable busses with device tree
>     overlays' talk at Linux Plumbers Conference 2024 [1].

Any outcome of the Plumbers meetup? Was this "double-link" solution
agreed on or so? I mean the code is the easy part here, but I would like
to have an agreed approach for handling all kinds of non-probable
busses. I really don't want an island solution for I2C. So, the key
question here is what do DT maintainers think?

You sent code without bindings, but I'd think the other way around would
be better for the discussion.

Makes sense?

Happy hacking,

   Wolfram


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

Re: [RFC PATCH 0/3] i2c: Introduce i2c bus extensions

[Luca Ceresoli]

Hi Wolfram,

On Thu, 20 Mar 2025 13:49:53 +0100
Wolfram Sang <wsa+renesas@sang-engineering.com> wrote:

> Hi Herve,
> 
> > The related big picture has been already presented in
> >   - the 'Add support for GE SUNH hot-pluggable connector' series [0]
> >   - the 'Runtime hotplug on non-discoverable busses with device tree
> >     overlays' talk at Linux Plumbers Conference 2024 [1].  
> 
> Any outcome of the Plumbers meetup? Was this "double-link" solution
> agreed on or so?

The i2c-parent was proposed by Rob [0]. The need for the double link
is what you, Hervé and I had agreed during our discussion after LPC,
based on having realized that the forward link is insufficient for some
cases (see "Second case" in the cover letter).

> I mean the code is the easy part here, but I would like
> to have an agreed approach for handling all kinds of non-probable
> busses. I really don't want an island solution for I2C. So, the key
> question here is what do DT maintainers think?
> 
> You sent code without bindings, but I'd think the other way around would
> be better for the discussion.

[0] https://lore.kernel.org/lkml/20240510163625.GA336987-robh@kernel.org/

Luca

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [RFC PATCH 0/3] i2c: Introduce i2c bus extensions

[Wolfram Sang]

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


> The i2c-parent was proposed by Rob [0]. The need for the double link
> is what you, Hervé and I had agreed during our discussion after LPC,
> based on having realized that the forward link is insufficient for some
> cases (see "Second case" in the cover letter).

Ah, we talked *after* LPC. Sorry, I remembered it wrong and thought we
talked before it. Will check the patches.


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

Re: [RFC PATCH 1/3] i2c: core: Follow i2c-parent when retrieving an adapter from node

[Wolfram Sang]

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


> Extend i2c_find_adapter_by_fwnode() to perform the walking from the
> given fwnode through i2c-parent references up to the adapter.

Even with the review of the schema going on, here are some comments
already.

> 
> Signed-off-by: Herve Codina <herve.codina@bootlin.com>
> ---
>  drivers/i2c/i2c-core-base.c | 43 ++++++++++++++++++++++++++++++++++++-
>  1 file changed, 42 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> index 5546184df05f..cb7adc5c1285 100644
> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c
> @@ -1827,14 +1827,55 @@ static int i2c_dev_or_parent_fwnode_match(struct device *dev, const void *data)
>   */
>  struct i2c_adapter *i2c_find_adapter_by_fwnode(struct fwnode_handle *fwnode)
>  {
> +	struct fwnode_reference_args args;
> +	struct fwnode_handle *adap_fwnode;
>  	struct i2c_adapter *adapter;
>  	struct device *dev;
> +	int err;
>  
>  	if (!fwnode)
>  		return NULL;
>  
> -	dev = bus_find_device(&i2c_bus_type, NULL, fwnode,
> +	adap_fwnode = fwnode_handle_get(fwnode);
> +
> +	/* Walk extension busses (through i2c-parent) up to the adapter node */
> +	while (fwnode_property_present(adap_fwnode, "i2c-parent")) {
> +		/*
> +		 * A specific case exists for the i2c demux pinctrl. The i2c bus
> +		 * node related this component (the i2c demux pinctrl node
> +		 * itself) has an i2c-parent property set. This property is used
> +		 * by the i2c demux pinctrl component for the demuxing purpose
> +		 * and is not related to the extension bus feature.
> +		 *
> +		 * In this current i2c-parent walking, the i2c demux pinctrl
> +		 * node has to be considered as an adapter node and so, if
> +		 * the adap_fwnode node is an i2c demux pinctrl node, simply
> +		 * stop the i2c-parent walking.
> +		 */
> +		if (fwnode_property_match_string(adap_fwnode, "compatible",
> +						 "i2c-demux-pinctrl") >= 0)
> +			break;

I understand the unlikeliness of another demux driver showing up, yet
relying on compatible-values here is too easy to get stale. What about
checking if the i2c-parent property has more than one entry? That should
be only true for demuxers.

> +
> +		/*
> +		 * i2c-parent property available in a i2c bus node means that
> +		 * this node is an extension bus node. In that case,
> +		 * continue i2c-parent walking up to the adapter node.
> +		 */
> +		err = fwnode_property_get_reference_args(adap_fwnode, "i2c-parent",
> +							 NULL, 0, 0, &args);
> +		if (err)
> +			break;
> +
> +		pr_debug("Find adapter for %pfw, use parent: %pfw\n", fwnode,
> +			 args.fwnode);

Is this useful when creating the overlays? I tend to ask you to remove
it one RFC phase is over. If it is useful, it should be at least
dev_dbg?

> +
> +		fwnode_handle_put(adap_fwnode);
> +		adap_fwnode = args.fwnode;
> +	}
> +
> +	dev = bus_find_device(&i2c_bus_type, NULL, adap_fwnode,
>  			      i2c_dev_or_parent_fwnode_match);
> +	fwnode_handle_put(adap_fwnode);
>  	if (!dev)
>  		return NULL;
>  
> -- 
> 2.47.1
> 

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

Re: [RFC PATCH 2/3] i2c: i2c-core-of: Move children registration in a dedicated function

[Wolfram Sang]

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


> -void of_i2c_register_devices(struct i2c_adapter *adap)
> +static void of_i2c_register_children(struct i2c_adapter *adap,
> +				     struct device_node *bus)

Could you kindly add a little kdoc so it is easy to understand the
difference between of_i2c_register_children() and
of_i2c_register_devices()? It is not too obvious from the names alone.


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

Re: [RFC PATCH 3/3] i2c: i2c-core-of: Handle i2c bus extensions

[Wolfram Sang]

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

On Wed, Feb 05, 2025 at 06:39:16PM +0100, Herve Codina wrote:
> i2c bus extensions were introduced to decouple i2c busses when they are
> wired to connectors. Combined with devicetree overlays, they introduce
> an additional level of indirection, which is needed to decouple the
> overlay (describing the hardware available on addon baord) and the base
> tree (describing resources provided to the addon board).

No comments on this one.


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

Re: [RFC PATCH 0/3] i2c: Introduce i2c bus extensions

[Wolfram Sang]

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

On Wed, Feb 05, 2025 at 06:39:13PM +0100, Herve Codina wrote:
> The big picture behind this RFC series is to support a Linux device
> with a connector to physically add and remove an add-on to/from the
> main device to augment its features at runtime, adding devices on
> non-discoverable busses, using device tree overlays.

I didn't test the actual feature, but I did test this on my Renesas
Lager board (R-Car H2) including its use of the notorious
i2c-demux-pinctrl driver (Thanks for taking care of it!). No regressions
discovered, so for that part:

Tested-by: Wolfram Sang <wsa+renesas@sang-engineering.com>


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

Re: [RFC PATCH 1/3] i2c: core: Follow i2c-parent when retrieving an adapter from node

[Herve Codina]

Hi Wolfram,

On Thu, 3 Apr 2025 11:03:27 +0200
Wolfram Sang <wsa+renesas@sang-engineering.com> wrote:

> > Extend i2c_find_adapter_by_fwnode() to perform the walking from the
> > given fwnode through i2c-parent references up to the adapter.  
> 
> Even with the review of the schema going on, here are some comments
> already.

Yes. Of course, depending on this review, things could be changed in the
implementation but every things already discussed here make the topic
moving forward. Thanks for that!

...

> > +
> > +	/* Walk extension busses (through i2c-parent) up to the adapter node */
> > +	while (fwnode_property_present(adap_fwnode, "i2c-parent")) {
> > +		/*
> > +		 * A specific case exists for the i2c demux pinctrl. The i2c bus
> > +		 * node related this component (the i2c demux pinctrl node
> > +		 * itself) has an i2c-parent property set. This property is used
> > +		 * by the i2c demux pinctrl component for the demuxing purpose
> > +		 * and is not related to the extension bus feature.
> > +		 *
> > +		 * In this current i2c-parent walking, the i2c demux pinctrl
> > +		 * node has to be considered as an adapter node and so, if
> > +		 * the adap_fwnode node is an i2c demux pinctrl node, simply
> > +		 * stop the i2c-parent walking.
> > +		 */
> > +		if (fwnode_property_match_string(adap_fwnode, "compatible",
> > +						 "i2c-demux-pinctrl") >= 0)
> > +			break;  
> 
> I understand the unlikeliness of another demux driver showing up, yet
> relying on compatible-values here is too easy to get stale. What about
> checking if the i2c-parent property has more than one entry? That should
> be only true for demuxers.

Indeed, this is better.
I will stop the walking based on this number of entries in the i2c-parent
property.

> 
> > +
> > +		/*
> > +		 * i2c-parent property available in a i2c bus node means that
> > +		 * this node is an extension bus node. In that case,
> > +		 * continue i2c-parent walking up to the adapter node.
> > +		 */
> > +		err = fwnode_property_get_reference_args(adap_fwnode, "i2c-parent",
> > +							 NULL, 0, 0, &args);
> > +		if (err)
> > +			break;
> > +
> > +		pr_debug("Find adapter for %pfw, use parent: %pfw\n", fwnode,
> > +			 args.fwnode);  
> 
> Is this useful when creating the overlays? I tend to ask you to remove
> it one RFC phase is over. If it is useful, it should be at least
> dev_dbg?

Using dev_dbg could lead to issues. Indeed, dev is not given as an argument
of i2c_find_adapter_by_fwnode() and there is no reason to add it (except
for this debug message).

Without a dev given as argument we have to retrieve it from the given
fw_node argument. This given fw_node may have its dev field not already set.
Indeed, the dev instanciation could be done later when the bus this fw_node
is connected to will probe().

For instance
  https://elixir.bootlin.com/linux/v6.14-rc6/source/drivers/gpu/drm/panel/panel-simple.c#L606
The panel driver can call of_find_i2c_adapter_by_node() which in turn call
i2c_find_adapter_by_fwnode() without having the I2C controller related to the
adapter already present. the panel driver will return a legit EPROBE_DEFER.

So back to our debug message in i2c_find_adapter_by_fwnode(), either I keep
pr_debug() or I fully remove the message but I don't thing I should change
the i2c_find_adapter_by_fwnode() prototype and update all the callers just
for this debug message.

The debug message can be interesting when things went wrong and we want
to investigate potential issue with i2c-parent chain from the last device
up to the adapter.

I don't have a strong opinion about the need of this message and I can
simply remove it.

What is your preference ?

Best regards,
Hervé

Re: [RFC PATCH 2/3] i2c: i2c-core-of: Move children registration in a dedicated function

[Herve Codina]

Hi Wolfram,

On Thu, 3 Apr 2025 11:07:15 +0200
Wolfram Sang <wsa+renesas@sang-engineering.com> wrote:

> > -void of_i2c_register_devices(struct i2c_adapter *adap)
> > +static void of_i2c_register_children(struct i2c_adapter *adap,
> > +				     struct device_node *bus)  
> 
> Could you kindly add a little kdoc so it is easy to understand the
> difference between of_i2c_register_children() and
> of_i2c_register_devices()? It is not too obvious from the names alone.
> 

Of course, I Will do.

Best regards,
Hervé

Re: [RFC PATCH 1/3] i2c: core: Follow i2c-parent when retrieving an adapter from node

[Wolfram Sang]

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


> The debug message can be interesting when things went wrong and we want
> to investigate potential issue with i2c-parent chain from the last device
> up to the adapter.

I thought so but couldn't estimate how often this is useful in reality.
I agree that introducing 'dev' is too much hazzle, yet I think the
message should have some id to disitnguish potential different adapter
chains. Either that, or...

> I don't have a strong opinion about the need of this message and I can
> simply remove it.

... we just remove it and let people add their debug stuff while
developing.

> What is your preference ?

A tad more 'removing'.


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

Re: [RFC PATCH 1/3] i2c: core: Follow i2c-parent when retrieving an adapter from node

[Herve Codina]

Hi Wolfram,

On Thu, 3 Apr 2025 13:20:05 +0200
Wolfram Sang <wsa+renesas@sang-engineering.com> wrote:

> > The debug message can be interesting when things went wrong and we want
> > to investigate potential issue with i2c-parent chain from the last device
> > up to the adapter.  
> 
> I thought so but couldn't estimate how often this is useful in reality.
> I agree that introducing 'dev' is too much hazzle, yet I think the
> message should have some id to disitnguish potential different adapter
> chains. Either that, or...
> 
> > I don't have a strong opinion about the need of this message and I can
> > simply remove it.  
> 
> ... we just remove it and let people add their debug stuff while
> developing.

I agree.

> 
> > What is your preference ?  
> 
> A tad more 'removing'.
> 

Will be removed.

Hervé

Re: [RFC PATCH 0/3] i2c: Introduce i2c bus extensions

[Ayush Singh]

I have tested this patch series for use with pocketbeagle 2 connector 
driver [0]. To get a better idea how it looks in real devicetree, see 
the base tree [1] and the overlay [2]. Since it also used gpio and pwm 
nexus nodes, along with providing pinmux for pins, it can provide a 
better picture of how the different pieces (export-symbols, nexus nodes, 
etc) look when combined.


I also have a question for Herve. Do you already have any working 
patches for similar extension for SPI and UART in some private tree?


[0]: https://github.com/Ayush1325/linux/tree/beagle-cape-v1

[1]: 
https://github.com/Ayush1325/BeagleBoard-DeviceTrees/commit/bf9d981ebf5f1a5704df1e7deba2188c70eb5d6f

[2]: 
https://github.com/Ayush1325/linux/commit/4ebc8467c98b5df3c30935e1d3736f9a64c1b08d


Tested-by: Ayush Singh <ayush@beagleboard.org>

Re: [RFC PATCH 0/3] i2c: Introduce i2c bus extensions

[Herve Codina]

Hi Ayush,

On Thu, 12 Jun 2025 13:22:45 +0530
Ayush Singh <ayush@beagleboard.org> wrote:

> I have tested this patch series for use with pocketbeagle 2 connector 
> driver [0]. To get a better idea how it looks in real devicetree, see 
> the base tree [1] and the overlay [2]. Since it also used gpio and pwm 
> nexus nodes, along with providing pinmux for pins, it can provide a 
> better picture of how the different pieces (export-symbols, nexus nodes, 
> etc) look when combined.

Nice. Happy to see that I am no more alone with a system using these
features.

> 
> 
> I also have a question for Herve. Do you already have any working 
> patches for similar extension for SPI and UART in some private tree?

No, I didn't do anything related to SPI nor UART.

On my system, no SPI nor UART are wired to my connector and so, I haven't
got any needs to implement extension busses for SPI an UART (serial dev bus)
nor any support for nexus nodes for other kind of components.

Best regards,
Hervé

Re: [RFC PATCH 0/3] i2c: Introduce i2c bus extensions

[Ayush Singh]

On 6/13/25 13:00, Herve Codina wrote:

> Hi Ayush,
>
> On Thu, 12 Jun 2025 13:22:45 +0530
> Ayush Singh <ayush@beagleboard.org> wrote:
>
>> I have tested this patch series for use with pocketbeagle 2 connector
>> driver [0]. To get a better idea how it looks in real devicetree, see
>> the base tree [1] and the overlay [2]. Since it also used gpio and pwm
>> nexus nodes, along with providing pinmux for pins, it can provide a
>> better picture of how the different pieces (export-symbols, nexus nodes,
>> etc) look when combined.
> Nice. Happy to see that I am no more alone with a system using these
> features.
>
>>
>> I also have a question for Herve. Do you already have any working
>> patches for similar extension for SPI and UART in some private tree?
> No, I didn't do anything related to SPI nor UART.
>
> On my system, no SPI nor UART are wired to my connector and so, I haven't
> got any needs to implement extension busses for SPI an UART (serial dev bus)
> nor any support for nexus nodes for other kind of components.
>
> Best regards,
> Hervé


I have added SPI bus extension to my kernel tree [0]. Now, the techlab 
cape (other than mikrobus port) works using export-symbols + i2c and spi 
bus extension + eeprom auto detection.


Here is a list of everything currently working on the tree:

1. EEPROM based auto-detection.

2. SPI

3. I2C

4. PWM

5. GPIO


Missing:

1. UART (Don't have a cape that has something using the UART yet. Maybe 
need to experiment with MikroBUS).


Not quite sure what else to do to move things forward.


Best Regards,

Ayush Singh


[0]: https://github.com/Ayush1325/linux/tree/beagle-cape-v1

Re: [RFC PATCH 0/3] i2c: Introduce i2c bus extensions

[Herve Codina]

Hi Ayush,

On Thu, 3 Jul 2025 16:56:20 +0530
Ayush Singh <ayush@beagleboard.org> wrote:

> On 6/13/25 13:00, Herve Codina wrote:
> 
> > Hi Ayush,
> >
> > On Thu, 12 Jun 2025 13:22:45 +0530
> > Ayush Singh <ayush@beagleboard.org> wrote:
> >  
> >> I have tested this patch series for use with pocketbeagle 2 connector
> >> driver [0]. To get a better idea how it looks in real devicetree, see
> >> the base tree [1] and the overlay [2]. Since it also used gpio and pwm
> >> nexus nodes, along with providing pinmux for pins, it can provide a
> >> better picture of how the different pieces (export-symbols, nexus nodes,
> >> etc) look when combined.  
> > Nice. Happy to see that I am no more alone with a system using these
> > features.
> >  
> >>
> >> I also have a question for Herve. Do you already have any working
> >> patches for similar extension for SPI and UART in some private tree?  
> > No, I didn't do anything related to SPI nor UART.
> >
> > On my system, no SPI nor UART are wired to my connector and so, I haven't
> > got any needs to implement extension busses for SPI an UART (serial dev bus)
> > nor any support for nexus nodes for other kind of components.
> >
> > Best regards,
> > Hervé  
> 
> 
> I have added SPI bus extension to my kernel tree [0]. Now, the techlab 
> cape (other than mikrobus port) works using export-symbols + i2c and spi 
> bus extension + eeprom auto detection.
> 
> 
> Here is a list of everything currently working on the tree:
> 
> 1. EEPROM based auto-detection.
> 
> 2. SPI
> 
> 3. I2C
> 
> 4. PWM
> 
> 5. GPIO
> 
> 
> Missing:
> 
> 1. UART (Don't have a cape that has something using the UART yet. Maybe 
> need to experiment with MikroBUS).
> 
> 
> Not quite sure what else to do to move things forward.
> 
> 
> Best Regards,
> 
> Ayush Singh
> 
> 
> [0]: https://github.com/Ayush1325/linux/tree/beagle-cape-v1
> 

I've just looked at your code related to SPI. It is closed to the I2C code
and that's pretty nice!

I think to move forward we have to wrote the SPI bus extension binding and
propose the binding + the code upstream.

Compared to I2C bus extension, only one repo is involved for SPI, the Linux
kernel repo. On I2C bus extension, I am stuck on the binding which is a
modification on the dtschema repo [0].

The SPI binding modifications for SPI bus extension will probably take place
in spi-controller.yaml [1] and should be pretty close to modifications done
for the I2C binding.

When one of the two series (I2C or SPI) is accepted, it will be easier
for the other one to follow (Same concept, same kind of binding, same kind
of code).

The advantage of the SPI series, I think, is that only one repo is involved.

Best regards,
Hervé

[0]: https://lore.kernel.org/all/20250618082313.549140-1-herve.codina@bootlin.com/
[1]: https://elixir.bootlin.com/linux/v6.16-rc4/source/Documentation/devicetree/bindings/spi/spi-controller.yaml