Re: 回复: [PATCH 2/3] dt-bindings: pinctrl: Add cix,sky1-pinctrl

[Linus Walleij <linus.walleij@linaro.org>]

Hi Gary,

thanks for your patch!

On Thu, Aug 28, 2025 at 10:58 AM Gary Yang <gary.yang@cixtech.com> wrote:
> > On 28/08/2025 07:37, Gary Yang wrote:

> > >> Whats the difference between? You have entire description field to
> > >> explain this but instead you said something obvious there.
> > >>
> > > Cix sky1 has three power states. S0 means work state. S3 means STR state.
> > S5 means SD state.
> > >
> > > The pin-controller on sky1 has two power states. They are S0 and S5.
> >
> >
> > State != device. Please create bindings for devices, not states.
> >
>
> Sorry, maybe I didn't explain it correctly before, and then make you misunderstand
>
> There are two pin-controller on sky1. One is used under s0 state, other is used under s5 state.
>
> They are two devices

Just explain this in the description: and everyone will understand what
is going on. Since "S0" and "S5" can be easy to confuse for "states"
it is extra helpful with some extended descriptions.

> > >>> +    properties:
> > >>> +      cix,pins:
> > >>
> > >> No, use generic properties from pinmux schema.
> > >>
> > >> You should also reference it.
> > >
> > > Did you suggest us to refer to
> > Documentation/devicetree/bindings/pinctrl/pincfg-node.yaml?
> > >
> > > Make us support drive-strength, bias-pull-down properties?
> >
> > and pinmux. There is a standard pins property.
>
> Ok, I see, try our best to support standard

Unfortunately many pin controllers have forged ahead
with custom foo,pins = <....>; settings where they set up
mux and electrical config by OR:in together different bits,
and then they just poke this into some registers.

This isn't very helpful for users.

I initially wanted all functions and groups to be strings
and then to associate groups with functions using
strings in the device tree.

But I have realized (though much pain) that many developers
don't like this. They want a magic number to write to
a register to configure a pin, because their hardware
has one (or several) register for each pin.

So nowadays the most common is to use a compromise.

A magic number in the pinmux property to set up the muxing.

For example:

arch/arm/boot/dts/mediatek/mt7623.dtsi:
pinmux = <MT7623_PIN_75_SDA0_FUNC_SDA0>,
               <MT7623_PIN_76_SCL0_FUNC_SCL0>;

Then the electric properties like bias-pull-down; to set
these on the state:

        i2c0_pins_a: i2c0-default {
                pins-i2c0 {
                        pinmux = <MT7623_PIN_75_SDA0_FUNC_SDA0>,
                                 <MT7623_PIN_76_SCL0_FUNC_SCL0>;
                        bias-disable;
                };
        };

This is a good compromis becaus it looks similar on all
SoCs and you see immediately what is going on: we enable
SDA0 And SCL0 and disable bias, so there must be external
pull-up resistors on this bus since I2C is open drain. Very
easy for an electronics engineer to grasp, they don't need
to be computer engineers or device tree experts.

Yours,
Linus Walleij