Re: [PATCH 3/8] ARM: dts: aspeed: Add I2C buses

[Andrew Jeffery]

On Thu, 2017-09-28 at 11:35 -0700, Brendan Higgins wrote:
> > On Thu, Sep 28, 2017 at 12:51 AM, Joel Stanley <joel@xxxxxxxxx> wrote:
> > Now with an upstream i2c bus driver, we can addÂÂthe 14 i2c buses that
> > exist in ASPEED G4 and G5 generation SoCs.
> >Â
> > It also adds aliases for the 14 built-in I2C busses to ensure userspace
> > sees the numbering staring from zero and counting up.
> >Â
> > Signed-off-by: Joel Stanley <joel@xxxxxxxxx>
> > ---
>Â
> > Reviewed-by: Brendan Higgins <brendanhiggins@xxxxxxxxxx>
>Â
> nit: can we make the i2c labels and the pinctrl labels match?
>Â
> For example:
>Â
> > +
> > +ÂÂÂÂÂÂÂi2c13: i2c-bus@480 {
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ#address-cells = <1>;
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ#size-cells = <0>;
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ#interrupt-cells = <1>;
> > +
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreg = <0x480 0x40>;
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂcompatible = "aspeed,ast2500-i2c-bus";
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂclocks = <&clk_apb>;
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂbus-frequency = <100000>;
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂinterrupts = <13>;
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂinterrupt-parent = <&i2c_ic>;
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂpinctrl-names = "default";
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂpinctrl-0 = <&pinctrl_i2c14_default>;
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂstatus = "disabled";
> > +ÂÂÂÂÂÂÂ};
>Â
> "i2c13" has a pinctrl-0 of "pinctrl_i2c14_default"
>Â
> I know that pinctrl_i2c14_default is consistent with the function and
> groups it uses, but I would like to see them all be consistent at some
> point in the future.

I doubt we'll be making them consistent: How the i2c devices are aliased by
Linux are independent of how they're labelled in hardware or the datasheet.

For sanity's sake pinmux's functions and groups follow the datasheet (though if
you want to maintain the Aspeed pinctrl drivers, be my guest :D)

We could rename the pinmux nodes to offset them from the function and group,
but it feels less self-contained if the nodes are named at the whim of the
device requesting the function if there's no further configuration on top of
the function and group*. It also makes it harder to auto-explode the function
name into the node definitions as we'd then need some further mangling to make
things line up.

* The pinctrl devicetree bindings allow for definition of nested nodes to
specify collections and combinations of pin function and group, and pin
configuration such as drive-strength. Thus the nodes can be a lot more complex
than what we've defined up until now, but what we've got enables a lot of
pretty straight-forward uses-cases.

We could alias the i2c devices in line with the hardware, but that breaks the
convention of starting device numbering at 0.

Ultimately Joel and I chose to make the break at the point where the i2c device
requests the mux function. It's up to the device to know what hardware mux
configuration it needs and that's what this approach documents. The
inconsistency is irritating but I don't feel like it's the end of the world,
and I think the alternatives are (slightly) worse.

Cheers,

Andrew

Attachment: signature.asc
Description: This is a digitally signed message part