Re: [PATCH 3/3] dt-bindings: i2c: realtek,rtl9301-i2c: extend for RTL9310 support

[Jonas Jelonek <jelonek.jonas@gmail.com>]

(sorry for double sending, previous one had HTML)

Hi Krzysztof,

On 02.07.2025 09:49, Krzysztof Kozlowski wrote:
> If people sent complete bindings, they would see that you are now in
> tricky spot and this maybe has to be redone to standard approach - I2C
> is not a child of syscon block, but separate device. When it is a
> separate device we solve it (plenty of examples) with phandle to syscon
> with offset or value argument.

Was this assumption probably made based on that the I2C peripherals are
controlled via mapped registers within the address space that is
designated to that 'switchcore'/syscon node?

If using a phandle instead solves this, I'd be fine. Moving the I2C
controller out of the syscon shouldn't be an issue. Is changing this
behavior fine with changing the dt-bindings then? IIRC there's a rule
that dt-bindings must not be changed once they are published.

>
> But no! Some incomplete hardware description was sent, stuffing
> everything into syscon and claiming that everything is child of syscon,
> and now you are stuck with this:
>
> system controller
>         |
>         |
>    i2c-controller-for-multiple-SDA
>            |
>            |
>        i2c-controllers-for-each-SDA
>
> This is not only just confusing but maybe even not correct.

At least I'm confused already.

To make sure we're talking about the same hardware architecture:
RTL93xx have two I2C controllers each having a hardwired SCL and being
able to use any of the 8/12 SDA lines.

As far as I understood, the 'i2c-controller-for-each-SDA' you
mention is meant to handle the muxing behavior (being able to use each
SDA on either controller). Being more like a channel than a dedicated
controller.

In comparison, downstream in OpenWrt there's currently a separate
driver i2c-rtl9300-mux. The child nodes belong to the Mux node there
instead of to the I2C controller. The mux node has a reference to the
I2C controller it uses.

> I understand that either i2c controller can take any SCL line. If so,
> that could be a pinctrl, but again this is child of that device, so
> pinctrl to parent would be odd.

Either I2C controller can take any SDA line, SCL line is fixed.
I see that adding the property implies the opposite, so I agree
if this is not acceptable.

>
> Vendor (not generic) property seems the only solution, but then this
> should not be part of the existing binding or you should clearly narrow
> this per variant. 

Based on the previous comment, I could completely omit this property
and somehow hardcode it in the driver based on e.g. the address of the
I2C controller. But I need advice in this case. Either use a property
to explicitly mention this in the device tree or infer it from an
already used property. 

> It was made very clear that rtl9301-family has only
> one SCL per controller and you cannot choose.

This hasn't changed in RTL9310 family.

And to clarify since this is already confusing for me in how the
compatibles are named: there is no rtl9301-family, RTL9301X SoCs (among
RTL9302X and RTL9303X) are part of the RTL9300 / longan family.Similarly, RTL9311X, RTL9312X and RTL9313X SoCs are part of the RTL9310 / mango family.
> Best regards,
> Krzysztof

Best regards,
Jonas