Re: [PATCH 2/2] schemas: i2c: Introduce I2C bus extensions

Hi Rob, device-tree maintainers,

This discussion started a few weeks ago and hasn't reached any conclusion.
I answered questions and comments but didn't receive any feedback.

In order to move forward on that topic, can we revive this discussion?

As a reminder, topics started in the discussion were the following:
 - i2c-bus-extension@0: usage of a subnode with unit address
 - Presence of phandles in both direction (double linked list)

Best regards,
Hervé

On Wed, 2 Apr 2025 10:21:23 +0200
Herve Codina <[email protected]> wrote:

> Hi Rob,
> 
> On Tue, 1 Apr 2025 09:03:23 -0500
> Rob Herring <[email protected]> wrote:
> 
> > On Tue, Apr 1, 2025 at 3:11 AM Herve Codina <[email protected]> wrote:  
> > >
> > > An I2C bus can be wired to the connector and allows an add-on board to
> > > connect additional I2C devices to this bus.
> > >
> > > Those additional I2C devices could be described as sub-nodes of the I2C
> > > bus controller node however for hotplug connectors described via device
> > > tree overlays 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 {    
> > 
> > What does 0 represent? Some fake I2C address?
> > 
> > Why do you even need a child node here?  
> 
> 0 represent the extension number. Multiple extensions can be connected
> to a single i2c controller:
>                                       +-------------+
>   +------------+                 .----+ Connector 1 |
>   |   i2c      |                 |    +-------------+
>   | controller +---- i2c bus ----+
>   +------------+                 |    +-------------+
>                                  '----+ Connector 2 |
>                                       +-------------+
> 
> I need a child node because extensions don't modify existing hardware (adding/removing
> a property) but add an entry point, the extension for a new set of devices.
> As it is not a existing hardware modification it is better represented as a node.
> 
> 
> Those extensions can be chained:
>   +-----------------------------------+         +-------------------+
>   | Base board                        |         | addon board       |
>   |                                   |         |    +------------+ |
>   | +------------+                    |         | .--+ i2c device | |
>   | |   i2c      |             +-------------+  | |  +------------+ |
>   | | controller +-- i2c bus --+ connector A +----+                 |
>   | +------------+             +-------------+  | |          +-------------+
>   +-----------------------------------+         | '----------+ connector B |
>                                                 |            +-------------+
>                                                 +-------------------+
> The addon board is described using an overlay.
> 
> In that case, we have:
> - base-board.dts:
>     i2c0: i2c@cafe0000 {
>         compatible = "xyz,i2c-ctrl";
>         #address-cells = <1>;
>         #size-cells = <0>;
> 
>         i2c-bus-extension@0 {
>             reg = <0>;
>             i2c-bus = <&i2c_connector_a>;
>         };
>         ...
>     };
> 
>     connector-a {
>         devices {
>            /* Entry point for devices available on the addon
>             * board that are not connected to a bus such as
>             * fixed-clock, fixed-regulator, connectors, ...
>             */
>         };
>         i2c_connector_a: i2c-connector-a {
>             /* The i2c available at connector */
>             #address-cells = <1>;
>             #size-cells = <0>;
>             i2c_parent <&i2c0>;
>        };
>     };
> 
> - addon-board.dtso
>     __overlay__ { /* This is applied at connector_a node */
>         i2c_connector_a: i2c-connector-a {
>             /* We do not modify the existing device i2c_connector_a
>              * by changing, adding or removing its properties but
>              * we add new devices (sub-nodes)
>              */
> 
>             /* The i2c device available in the addon-board */
>             i2c-device@0x10 {
>                 compatible = "foo,bar";
>                 reg = 0x10;
>             };
> 
>             /* The i2c extension forwarding the i2c bus */
>             i2c-bus-extension@0 {
> 	        reg = <0>;
>                 i2c-bus = <&i2c_connector_b>;
>             };
>        };
>        
>        devices {
>           /* addon-board connector b */
>           connector_b {
>               i2c_connector_b: i2c_connector_b {
>               /* The i2c available at connector */
>               #address-cells = <1>;
>               #size-cells = <0>;
>               i2c_parent = <&i2c_connector_a>;
>           };
>        };
>    };
> 
> Without a child node for i2c-bus-extension, we need to add
> properties on already existing node (i2c-connector-a) to add
> the bus extension and adding/modifying/removing a property
> on a device-tree node correspond to modifying the device
> itself (description changed) whereas adding/removing sub-nodes
> correspond to adding/removing devices handled by the parent
> parent node of those sub-nodes.
> 
> From the controller point of view, an extension is "a collection of
> devices described somewhere else in the device-tree and connected
> to the I2C SDA/SCL pins". Having that described as a sub-node seems
> correct.
> 
> >   
> > >           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.
> > >
> > > The extension node is linked to the I2C bus controller in two ways. The
> > > first one with the i2c-bus-extension available in I2C controller
> > > sub-node and the second one with the i2c-parent property available in
> > > the extension node itself.
> > >
> > > The purpose of those two links is to provide the link in both direction
> > > from the I2C controller to the I2C extension and from the I2C extension
> > > to the I2C controller.    
> > 
> > Why do you need both directions? An i2c controller can search the tree
> > for i2c-parent and find the one's that belong to it. Or the connector
> > can register with the I2C controller somehow.  
> 
> Yes, but this is sub-optimal compare to the double-link references.
> 
> I discarded any kind of registering from the connector which implies
> extra complexity compared to a simple double-link reference. In the I2C
> path, the connector is really a passive component and fully transparent.
> It should be transparent as well in the implementation.
> 
> Using only i2c-parent (i.e. the reference from extension to i2c controller)
> works when the walk from extension to i2c controller but using it on the
> other direction is not as trivial as it could be.
> 
> Indeed, starting from the i2c controller, we have to search for the entire
> tree any i2c-parent that references the i2c controller.
> Those i2c-parent can exist in node that are not at all related to i2c
> extensions.
> 
> i2c-parent in all cases represents an i2c parent bus but not only for i2c
> extensions. I2C muxes and some other devices use this property.
> 
> Here also, searching the entire tree for i2c-parent and being sure that
> the property found is related to an I2C extension adds extra complexity
> that is simply not present with the double-link references.
> 
> Best regards,
> Hervé