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. But why? 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 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". 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. Yours, Linus Walleij
