Message-ID: <20240902154702-GYA840562@gentoo>
Date: Mon, 2 Sep 2024 15:47:02 +0000
From: Yixun Lan <dlan@...too.org>
To: Linus Walleij <linus.walleij@...aro.org>
Cc: Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Paul Walmsley <paul.walmsley@...ive.com>,
	Palmer Dabbelt <palmer@...belt.com>,
	Albert Ou <aou@...s.berkeley.edu>, Conor Dooley <conor@...nel.org>,
	Yangyu Chen <cyy@...self.name>, Jesse Taube <jesse@...osinc.com>,
	Jisheng Zhang <jszhang@...nel.org>,
	Inochi Amaoto <inochiama@...look.com>,
	Icenowy Zheng <uwu@...nowy.me>,
	Meng Zhang <zhangmeng.kevin@...cemit.com>,
	Meng Zhang <kevin.z.m@...mail.com>, devicetree@...r.kernel.org,
	linux-riscv@...ts.infradead.org, linux-kernel@...r.kernel.org,
	linux-gpio@...r.kernel.org
Subject: Re: [PATCH v3 2/4] pinctrl: spacemit: add support for SpacemiT K1 SoC

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@...too.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

Powered by blists - more mailing lists