Re: [PATCH v3 2/4] pinctrl: spacemit: add support for SpacemiT K1 SoC

Yixun Lan
2024-09-02T15:47:13Z

Hi Linus:

On 09:57 Mon 02 Sep , Linus Walleij wrote:
> Hi Yixun,
>
> thanks for your patch! Overall the driver looks very good, it's using the
> right helpers and abstractions etc.
>
> There is this thing that needs some elaboration:
>
> On Wed, Aug 28, 2024 at 1:31 PM Yixun Lan <dlan@gentoo.org> wrote:
>
> > +/* pin offset */
> > +#define PINID(x) ((x) + 1)
> > +
> > +#define GPIO_INVAL 0
> > +#define GPIO_00 PINID(0)
> > +#define GPIO_01 PINID(1)
> (...)
>
> So GPIO 00 has pin ID 1 actually etc.
>
yes, in current version

> But why?
>
good question!

from hw perspective, the GPIO_00 pinctrl register start at offset 0x4,
see chap 3.3.1 of "Pin Information" section at [1]

and in this version of patch, we are extracting pinid from register offset,
using the algorithem pinid = (offset >> 2). this idea was something I
borrowed from vendor's driver, and now you remind me this might be wrong..

> If there is no datasheet or other conflicting documentation, just
> begin numbering the GPIOs at 1 instead of 0 to match the
> hardware:
>
> #define GPIO_01 1
> #define GPIO_02 2
>

as current patch version, there will be some non-linear mapping, such as
#define GPIO_98 93
#define GPIO_99 92
..
#define GPIO_110 116
...

I think we could fix this by introducing a pinid_to_register_offset() function,
which should drop the pinid = (offset >> 2) mapping, but instead, doing in the
reverse way, retrive register offset from pinid, so idealy we should get
a linear mapping of GPIO to pinid, like

#define GPIO_00 0
#define GPIO_01 1
..
#define GPIO_127 127

I will work this in next patch version.

> and all is fine.
>
> It's just very uninituitive for developers. I guess that there
> is a reason, such as that the datasheet has stated that the pin
> with pin ID 1 is GPIO 00, then this needs to be explained with
> a comment in the code: "we are doing this because otherwise
> the developers will see an offset of -1 between the number the
> pin has in the datasheet and the number they put into the
> device tree, while the hardware starts the numbering at 1, the
> documentation starts the numbering at 0".
>
see above, a potential solution to fix this

> It is common that engineers from analog electronics background
> start numbering things from 1 while any computer science
> engineer start the numbering at 0. So this is what you get when
> an analog engineer designs the electronics and a computer
> science engineer writes that datasheet and decides to "fix"
> the problem.
>
true, things happens, sometimes there is gap in understanding between
analog engineers and software developers..

Link: https://developer.spacemit.com/documentation?token=An1vwTwKaigaXRkYfwmcznTXned [1]

--
Yixun Lan (dlan)
Gentoo Linux Developer
GPG Key ID AABEFD55