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

[Rob Herring]

On Fri, Aug 8, 2025 at 11:08 AM Luca Ceresoli <luca.ceresoli@xxxxxxxxxxx> wrote:
>
> Hello Rob, Wolfram,
>
> [this e-mail is co-written with Hervé]
>
> Rob, Wolfram: this e-mail mentions DT description together with
> implementation ideas, but both of your opinions are very relevant here.
>
> On Fri, 1 Aug 2025 13:09:54 -0500
> Rob Herring <robh@xxxxxxxxxx> wrote:
>
> > On Wed, Jun 18, 2025 at 3:23 AM Herve Codina <herve.codina@xxxxxxxxxxx> 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,foo";
> > >       i2c-bus-extension@0 {
> >
> > This is at I2C bus address 0? No. You are mixing 2 different address
> > spaces. Don't do that.
> >
> > You could solve this with just a property in the parent. If there's
> > more than 1, then it's just multiple phandles. However I don't think
> > you need this at all. You can just search the DT for 'i2c-parent' and
> > find phandles that match the i2c controller node. But why does the
> > controller driver need to know about connectors? Shouldn't the
> > connector driver drive this and tell the controller there's more
> > devices?
>
> These were the concerns you had raised last April [0], so let's continue
> from there and Hervé's follow-up.
>
> Hervé's position was that a double-reference is more effective as every
> interested party (the i2c adapter and the i2c extension node) have an
> explicit description of the connected party/parties. Besides, this looks
> consistent with the bidirectional links in remote-endpoints, which has
> similarities.

In a complex graph, you need to be able to walk from any node to
another node. I don't think that applies here.

> OTOH I agree a single-direction link (i2c extension node to i2c
> adapter) should work, in principle. However there are issues, mainly in
> how an adapter finds out about the extensions when it is probed after
> them.

We've had other cases such as i2c-parent and ddc-i2c-bus for years.
Wouldn't those have the same issue? Maybe they do and just ignored it.
If so, too late to change their DT and the kernel has to solve the
problems there anyways...

> Your proposal of searching the whole tree involves a lot of searches in
> a potentially large tree, even though I don't expect it to happen often
> at least for the use cases I'm aware of.

Right. And the need to do that entirely depends on the I2C core
needing to scan the whole tree. Surely that's not actually the case.
How does adding a device by overlay (as a direct child) work? If the
I2C core just scans the child nodes again, don't we just need to
change it to scan somewhere else? Seems like that would be simple
enough change.

> But, more important, Hervé pointed out an "i2c-parent" property is
> already used for different purposes (i2c muxes). We could choose
> another string but that would be fragile too.

It's the same purpose. Linking an i2c bus to somewhere else in the
tree. I don't see why a connector/overlay is special here.

> One idea is to unambiguously mark i2c bus extension nodes with a
> compatible string:
>
>   connector {
>       i2c_ctrl: i2c-ctrl {
>           compatible = "i2c-bus-extension"; // <-----
>           i2c-parent = <&i2c1>;
>           #address-cells = <1>;
>           #size-cells = <0>;
>       };
>   };
>
> And then implementing a device driver for the i2c bus extension.

I generally will never argue against adding a compatible...

> This would allow to ensure extensions are probed after their adapter
> (thanks to probe deferral) and at that point can register themselves at
> the adapter. In other words the device driver core would provide the
> synchronization between adapter, bus extension, and devices on the bus
> extension.

Can't the parent (the connector node) driver do the same thing and
defer if any i2c-parent phandles drivers aren't probed?

> A different option is to only have the "i2c-parent" phandle in the
> extension node and nothing else in DT (no bidirectional link, no
> compatible string), without any full-tree searches.
>
> On the implementation side, the connector driver when probing would
> register the extension nodes at the I2C core, which would maintain a
> list of extension nodes. This is important when the connector probes
> first. Then when any adapter probes the core would iterate over the
> list to check whether the newly-probed adapter is pointed to by one of
> the registered bus extensions, and then start populating the devices on
> the matching bus extension(s).
>
> A lot of care would have to be put in the disconnection path and while
> removing any bus extension from the global list, which could race with
> the I2C core using the list itself. The drive core wouldn't do it for
> us for free.

I'll defer to Wolfram on I2C core implementation...


Rob