| Message ID | 20220728155652.29516-1-henning.schild@siemens.com |
|---|---|
| Headers | show |
| Series | add support for another simatic board | expand |
I wanted to send this series to a wider audience, somehow i messed that up and might have to send again. Maybe the v5, and if there is no changes i might send again as v4. Henning Am Thu, 28 Jul 2022 17:56:48 +0200 schrieb Henning Schild <henning.schild@siemens.com>: > This series first enables a SuperIO GPIO driver to support a chip from > the vendor Nuvoton, the driver is for Fintek devices but those just > are very similar. And in watchdog and hwmon subsystems these SuperIO > drivers also share code and are sometimes called a family. > > In another step the individual banks receive a label to tell them > apart, a step which potentially changes an interface to legacy users > that might rely on all banks having the same label, or an exact > label. But since a later patch wants to use GPIO_LOOKUP unique labels > are needed and i decided to assign them for all supported chips. > > In a following patch the Simatic GPIO LED driver is extended to > provide LEDs in case that SuperIO GPIO driver can be loaded. > > Last but not least the watchdog module of that same SuperIO gets > loaded on a best effort basis. > > Note similar patches have appreared before as > "[PATCH v3 0/1] add device driver for Nuvoton SIO gpio function" > The main difference here is that i added chip support to an existing > driver instead of creating a new one. And that i actually propose all > patches and do not just have the LED patch for Simatic as an example. > Also note that the patches are based on > "[PATCH v6 00/12] platform/x86: introduce p2sb_bar() helper" > > Henning Schild (4): > gpio-f7188x: Add GPIO support for Nuvoton NCT6116 > gpio-f7188x: use unique labels for banks/chips > leds: simatic-ipc-leds-gpio: add new model 227G > platform/x86: simatic-ipc: enable watchdog for 227G > > drivers/gpio/gpio-f7188x.c | 192 > +++++++++++------- drivers/leds/simple/simatic-ipc-leds-gpio.c | > 42 +++- drivers/platform/x86/simatic-ipc.c | 7 +- > .../platform_data/x86/simatic-ipc-base.h | 1 + > include/linux/platform_data/x86/simatic-ipc.h | 1 + > 5 files changed, 158 insertions(+), 85 deletions(-) >
Am Thu, 28 Jul 2022 23:33:36 +0200 schrieb Andy Shevchenko <andy.shevchenko@gmail.com>: > On Thu, Jul 28, 2022 at 5:57 PM Henning Schild > <henning.schild@siemens.com> wrote: > > > > Add GPIO support for Nuvoton NCT6116 chip. Nuvoton SuperIO chips are > > very similar to the ones from Fintek. In other subsystems they also > > share drivers and are called a family of drivers. > > > > For the GPIO subsystem the only difference is that the direction > > bit is reversed and that there is only one data bit per pin. On the > > SuperIO level the logical device is another one. > > ... > > > +#define SIO_GPIO_ENABLE 0x30 /* GPIO enable */ > > I don't see how it's being utilized... (But okay, it might be good to > have as a hint for a reader who has no access to the documentation). Good catch. That is a leftover from code that turned out to be not needed. Will drop. > ... > > > + if (sio->device == SIO_LD_GPIO_NUVOTON) { > > Everywhere else you use `device == SIO_LD_GPIO_FINTEK`, perhaps here > for consistency? However, I would rather see a field that clearly > states that it's an inverted value. Then you can use > > if (sio->dir_inv) > ...do something... Good idea, will look into that. Given we talk about a family of chips there might be more vendor ids that should be mapped onto "inv" in the future. Henning > > + if (dir & BIT(offset)) > > + return GPIO_LINE_DIRECTION_IN; > > + > > + return GPIO_LINE_DIRECTION_OUT; > > + } > > + > > + if (dir & BIT(offset)) > > return GPIO_LINE_DIRECTION_OUT; > > > > return GPIO_LINE_DIRECTION_IN; >
On Thu, Jul 28, 2022 at 5:57 PM Henning Schild <henning.schild@siemens.com> wrote: > In another step the individual banks receive a label to tell them apart, > a step which potentially changes an interface to legacy users that might > rely on all banks having the same label, or an exact label. But since a > later patch wants to use GPIO_LOOKUP unique labels are needed and i > decided to assign them for all supported chips. Since it is all in-kernel users, any "legacy users" should be in the kernel tree and you should then fix them in the patch. If they are not in the kernel tree, they are out-of-tree drivers and them we do not care, we can't fix the whole world. Yours, Linus Walleij