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

[Yixun Lan <dlan@xxxxxxxxxx>]

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@xxxxxxxxxx> 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