Re: [PATCH v10 1/2] dt-bindings: pinctrl: mt8195: add rsel define

[Chen-Yu Tsai <wenst@chromium.org>]

On Tue, Aug 17, 2021 at 10:21 AM zhiyong.tao <zhiyong.tao@mediatek.com> wrote:
>
> On Tue, 2021-08-17 at 01:00 +0200, Linus Walleij wrote:
> > On Mon, Aug 16, 2021 at 5:38 PM Chen-Yu Tsai <wenst@chromium.org>
> > wrote:
> > > On Mon, Aug 16, 2021 at 6:48 PM zhiyong.tao <
> > > zhiyong.tao@mediatek.com> wrote:
> > > > > I'll take that as "use SI units whenever possible and
> > > > > reasonable".
> > > >
> > > > ==> so It doesn't need to change the define, is it right?
> > > > we will keep the common define.
> > >
> > > Actually I think it would be possible and reasonable to use SI
> > > units
> > > in this case, since you are the vendor and have the resistor values
> > > to implement the support. Having different sets of values for
> > > different
> > > chips is nothing out of the ordinary. We already have to account
> > > for
> > > different number of pins and different pin functions. That is what
> > > compatible strings are for.
> >
> > I fully agree with Chen-Yu's analysis here.
> >
> > Zhiyong can you make an attempt to use SI units (Ohms) and see
> > what it will look like? I think it will look better for users and it
> > will
> > be less risk to make mistakes.
> >
> > Yours,
> > Linus Walleij
>
> Hi Linus & chen-yu,
>
> The rsel actual bias resistance of each setting is different in
> different IC. For example, in mt8195, the rsel actual bias resistance
> setting like as:
> MTK_PULL_SET_RSEL_000:75K in PU, 75k in PD;
> MTK_PULL_SET
> _RSEL_001:10k in PU, 5k in PD;
> MTK_PULL_SET_RSEL_010:5k in PU, 75k in
> PD;
> MTK_PULL_SET_RSEL_011:4k in PU, 5K in PD;
> MTK_PULL_SET_RSEL_100:3k in
> PU, 75k in PD;
> MTK_PULL_SET_RSEL_101:2k in PU, 5K in PD;
> MTK_PULL_SET_RSE
> L_110:1.5k in PU, 75k in PD;
> MTK_PULL_SET_RSEL_111:1k in PU, 5k in PD.
>
> but in mt8192, the rsel actual bias resistance setting like as:
> MTK_PULL_SET_RSEL_000:75K in PU, 75k in PD;
> MTK_PULL_SET_RSEL_001:3k in PU, 5k in PD;
> MTK_PULL_SET_RSEL_010:10k in PU, 75k in PD;
> MTK_PULL_SET_RSEL_011:1k in PU, 5K in PD;
>
> Can you help me to provide a suggestion common define for the all
> different IC?
> It seems that we should add a new define, if we upstream a new IC
> pinctrl driver in the future.

I assume you mean the macros used in the device tree?

The point of using SI units is to get rid of the macros. Instead of:

    bias-pull-up = <MTK_PULL_SET_RSEL_000>;

and

    bias-pull-down = <MTK_PULL_SET_RSEL_011>;

We want:

    bias-pull-up = <75000>;

and

    bias-pull-down = <5000>;

And the pinctrl driver then converts the real values in the device tree
into register values using some lookup table.

The DT schema could then enumerate all the valid resistor values,
and get proper validity checking.

Now if you really wanted to keep some symbols for mapping hardware
register values to resistor values, you could have

    #define MT8192_PULL_UP_RSEL_001      75000
    #define MT8192_PULL_DOWN_RSEL_001     5000

or have them all named MTK_PULL_{UP,DOWN}_RSEL_NNN, but split into
different header files, one per SoC.

Personally I think having the macros is a bad idea if proper values
are available. It just adds another layer of indirection, and another
area where errors can creep in.


Regards
ChenYu