* [PATCH v4 0/5] add support for another simatic board @ 2022-08-23 10:23 Henning Schild 2022-08-23 10:23 ` [PATCH v4 1/5] gpio-f7188x: Add GPIO support for Nuvoton NCT6116 Henning Schild ` (5 more replies) 0 siblings, 6 replies; 16+ messages in thread From: Henning Schild @ 2022-08-23 10:23 UTC (permalink / raw) To: Linus Walleij, Bartosz Golaszewski, Pavel Machek, Hans de Goede, Mark Gross, Andy Shevchenko, Lee Jones, linux-gpio, linux-kernel, linux-leds, platform-driver-x86 Cc: Sheng-Yuan Huang, Tasanakorn Phaipool, simon.guinot, Henning Schild changes since v3: - update Kconfig as well - drop chip names from comment in driver header - add manufacturer check for Fintek again, Nuvoton not possible - drop revision printing for Nuvoton - restructure defines again - add new model 427G changes since v2: (p1 only) - rename macros that change behavior - use chip type not device id in the macros - reorder defines a bit changes since v1: - remove unused define - fix bug where (base + 2) was used as second data bit - add macros for "inverted" and "single data bit" 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 (5): 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 platform/x86: simatic-ipc: add new model 427G drivers/gpio/Kconfig | 3 +- drivers/gpio/gpio-f7188x.c | 229 +++++++++++------- drivers/leds/simple/simatic-ipc-leds-gpio.c | 42 +++- drivers/platform/x86/simatic-ipc.c | 10 +- .../platform_data/x86/simatic-ipc-base.h | 1 + include/linux/platform_data/x86/simatic-ipc.h | 2 + 6 files changed, 194 insertions(+), 93 deletions(-) -- 2.35.1 ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v4 1/5] gpio-f7188x: Add GPIO support for Nuvoton NCT6116 2022-08-23 10:23 [PATCH v4 0/5] add support for another simatic board Henning Schild @ 2022-08-23 10:23 ` Henning Schild 2022-08-23 14:47 ` Andy Shevchenko 2022-08-23 10:23 ` [PATCH v4 2/5] gpio-f7188x: use unique labels for banks/chips Henning Schild ` (4 subsequent siblings) 5 siblings, 1 reply; 16+ messages in thread From: Henning Schild @ 2022-08-23 10:23 UTC (permalink / raw) To: Linus Walleij, Bartosz Golaszewski, Pavel Machek, Hans de Goede, Mark Gross, Andy Shevchenko, Lee Jones, linux-gpio, linux-kernel, linux-leds, platform-driver-x86 Cc: Sheng-Yuan Huang, Tasanakorn Phaipool, simon.guinot, Henning Schild 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. On a chip level we do not have a manufacturer ID to check and also no revision. Signed-off-by: Henning Schild <henning.schild@siemens.com> --- drivers/gpio/Kconfig | 3 +- drivers/gpio/gpio-f7188x.c | 107 ++++++++++++++++++++++++++++--------- 2 files changed, 84 insertions(+), 26 deletions(-) diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig index 0642f579196f..3f64345fe40b 100644 --- a/drivers/gpio/Kconfig +++ b/drivers/gpio/Kconfig @@ -874,10 +874,11 @@ config GPIO_104_IDI_48 module parameter. config GPIO_F7188X - tristate "F71869, F71869A, F71882FG, F71889F and F81866 GPIO support" + tristate "Fintek and Nuvoton Super-I/O GPIO support" help This option enables support for GPIOs found on Fintek Super-I/O chips F71869, F71869A, F71882FG, F71889F and F81866. + As well as Nuvoton Super-I/O chip NCT6116D. To compile this driver as a module, choose M here: the module will be called f7188x-gpio. diff --git a/drivers/gpio/gpio-f7188x.c b/drivers/gpio/gpio-f7188x.c index 18a3147f5a42..820ac5d60fda 100644 --- a/drivers/gpio/gpio-f7188x.c +++ b/drivers/gpio/gpio-f7188x.c @@ -1,6 +1,6 @@ // SPDX-License-Identifier: GPL-2.0-or-later /* - * GPIO driver for Fintek Super-I/O F71869, F71869A, F71882, F71889 and F81866 + * GPIO driver for Fintek and Nuvoton Super-I/O chips * * Copyright (C) 2010-2013 LaCie * @@ -21,23 +21,36 @@ */ #define SIO_LDSEL 0x07 /* Logical device select */ #define SIO_DEVID 0x20 /* Device ID (2 bytes) */ -#define SIO_DEVREV 0x22 /* Device revision */ -#define SIO_MANID 0x23 /* Fintek ID (2 bytes) */ -#define SIO_LD_GPIO 0x06 /* GPIO logical device */ #define SIO_UNLOCK_KEY 0x87 /* Key to enable Super-I/O */ #define SIO_LOCK_KEY 0xAA /* Key to disable Super-I/O */ -#define SIO_FINTEK_ID 0x1934 /* Manufacturer ID */ +/* + * Fintek devices. + */ +#define SIO_FINTEK_DEVREV 0x22 /* Fintek Device revision */ +#define SIO_FINTEK_MANID 0x23 /* Fintek ID (2 bytes) */ + +#define SIO_FINTEK_ID 0x1934 /* Manufacturer ID */ + #define SIO_F71869_ID 0x0814 /* F71869 chipset ID */ #define SIO_F71869A_ID 0x1007 /* F71869A chipset ID */ #define SIO_F71882_ID 0x0541 /* F71882 chipset ID */ #define SIO_F71889_ID 0x0909 /* F71889 chipset ID */ #define SIO_F71889A_ID 0x1005 /* F71889A chipset ID */ #define SIO_F81866_ID 0x1010 /* F81866 chipset ID */ -#define SIO_F81804_ID 0x1502 /* F81804 chipset ID, same for f81966 */ +#define SIO_F81804_ID 0x1502 /* F81804 chipset ID, same for F81966 */ #define SIO_F81865_ID 0x0704 /* F81865 chipset ID */ +#define SIO_LD_GPIO_FINTEK 0x06 /* GPIO logical device */ + +/* + * Nuvoton devices. + */ +#define SIO_NCT6116D_ID 0xD283 /* NCT6116D chipset ID */ + +#define SIO_LD_GPIO_NUVOTON 0x07 /* GPIO logical device */ + enum chips { f71869, @@ -48,6 +61,7 @@ enum chips { f81866, f81804, f81865, + nct6116d, }; static const char * const f7188x_names[] = { @@ -59,10 +73,12 @@ static const char * const f7188x_names[] = { "f81866", "f81804", "f81865", + "nct6116d", }; struct f7188x_sio { int addr; + int device; enum chips type; }; @@ -170,6 +186,9 @@ static int f7188x_gpio_set_config(struct gpio_chip *chip, unsigned offset, /* Output mode register (0:open drain 1:push-pull). */ #define gpio_out_mode(base) (base + 3) +#define gpio_dir_invert(type) ((type) == nct6116d) +#define gpio_data_single(type) ((type) == nct6116d) + static struct f7188x_gpio_bank f71869_gpio_bank[] = { F7188X_GPIO_BANK(0, 6, 0xF0), F7188X_GPIO_BANK(10, 8, 0xE0), @@ -254,6 +273,17 @@ static struct f7188x_gpio_bank f81865_gpio_bank[] = { F7188X_GPIO_BANK(60, 5, 0x90), }; +static struct f7188x_gpio_bank nct6116d_gpio_bank[] = { + F7188X_GPIO_BANK(0, 8, 0xE0), + F7188X_GPIO_BANK(10, 8, 0xE4), + F7188X_GPIO_BANK(20, 8, 0xE8), + F7188X_GPIO_BANK(30, 8, 0xEC), + F7188X_GPIO_BANK(40, 8, 0xF0), + F7188X_GPIO_BANK(50, 8, 0xF4), + F7188X_GPIO_BANK(60, 8, 0xF8), + F7188X_GPIO_BANK(70, 1, 0xFC), +}; + static int f7188x_gpio_get_direction(struct gpio_chip *chip, unsigned offset) { int err; @@ -264,13 +294,16 @@ static int f7188x_gpio_get_direction(struct gpio_chip *chip, unsigned offset) err = superio_enter(sio->addr); if (err) return err; - superio_select(sio->addr, SIO_LD_GPIO); + superio_select(sio->addr, sio->device); dir = superio_inb(sio->addr, gpio_dir(bank->regbase)); superio_exit(sio->addr); - if (dir & 1 << offset) + if (gpio_dir_invert(sio->type)) + dir = ~dir; + + if (dir & BIT(offset)) return GPIO_LINE_DIRECTION_OUT; return GPIO_LINE_DIRECTION_IN; @@ -286,10 +319,14 @@ static int f7188x_gpio_direction_in(struct gpio_chip *chip, unsigned offset) err = superio_enter(sio->addr); if (err) return err; - superio_select(sio->addr, SIO_LD_GPIO); + superio_select(sio->addr, sio->device); dir = superio_inb(sio->addr, gpio_dir(bank->regbase)); - dir &= ~BIT(offset); + + if (gpio_dir_invert(sio->type)) + dir |= BIT(offset); + else + dir &= ~BIT(offset); superio_outb(sio->addr, gpio_dir(bank->regbase), dir); superio_exit(sio->addr); @@ -307,11 +344,11 @@ static int f7188x_gpio_get(struct gpio_chip *chip, unsigned offset) err = superio_enter(sio->addr); if (err) return err; - superio_select(sio->addr, SIO_LD_GPIO); + superio_select(sio->addr, sio->device); dir = superio_inb(sio->addr, gpio_dir(bank->regbase)); dir = !!(dir & BIT(offset)); - if (dir) + if (gpio_data_single(sio->type) || dir) data = superio_inb(sio->addr, gpio_data_out(bank->regbase)); else data = superio_inb(sio->addr, gpio_data_in(bank->regbase)); @@ -332,7 +369,7 @@ static int f7188x_gpio_direction_out(struct gpio_chip *chip, err = superio_enter(sio->addr); if (err) return err; - superio_select(sio->addr, SIO_LD_GPIO); + superio_select(sio->addr, sio->device); data_out = superio_inb(sio->addr, gpio_data_out(bank->regbase)); if (value) @@ -342,7 +379,10 @@ static int f7188x_gpio_direction_out(struct gpio_chip *chip, superio_outb(sio->addr, gpio_data_out(bank->regbase), data_out); dir = superio_inb(sio->addr, gpio_dir(bank->regbase)); - dir |= BIT(offset); + if (gpio_dir_invert(sio->type)) + dir &= ~BIT(offset); + else + dir |= BIT(offset); superio_outb(sio->addr, gpio_dir(bank->regbase), dir); superio_exit(sio->addr); @@ -360,7 +400,7 @@ static void f7188x_gpio_set(struct gpio_chip *chip, unsigned offset, int value) err = superio_enter(sio->addr); if (err) return; - superio_select(sio->addr, SIO_LD_GPIO); + superio_select(sio->addr, sio->device); data_out = superio_inb(sio->addr, gpio_data_out(bank->regbase)); if (value) @@ -388,7 +428,7 @@ static int f7188x_gpio_set_config(struct gpio_chip *chip, unsigned offset, err = superio_enter(sio->addr); if (err) return err; - superio_select(sio->addr, SIO_LD_GPIO); + superio_select(sio->addr, sio->device); data = superio_inb(sio->addr, gpio_out_mode(bank->regbase)); if (param == PIN_CONFIG_DRIVE_OPEN_DRAIN) @@ -449,6 +489,10 @@ static int f7188x_gpio_probe(struct platform_device *pdev) data->nr_bank = ARRAY_SIZE(f81865_gpio_bank); data->bank = f81865_gpio_bank; break; + case nct6116d: + data->nr_bank = ARRAY_SIZE(nct6116d_gpio_bank); + data->bank = nct6116d_gpio_bank; + break; default: return -ENODEV; } @@ -479,18 +523,15 @@ static int __init f7188x_find(int addr, struct f7188x_sio *sio) { int err; u16 devid; + u16 manid; err = superio_enter(addr); if (err) return err; err = -ENODEV; - devid = superio_inw(addr, SIO_MANID); - if (devid != SIO_FINTEK_ID) { - pr_debug(DRVNAME ": Not a Fintek device at 0x%08x\n", addr); - goto err; - } + sio->device = SIO_LD_GPIO_FINTEK; devid = superio_inw(addr, SIO_DEVID); switch (devid) { case SIO_F71869_ID: @@ -517,17 +558,33 @@ static int __init f7188x_find(int addr, struct f7188x_sio *sio) case SIO_F81865_ID: sio->type = f81865; break; + case SIO_NCT6116D_ID: + sio->device = SIO_LD_GPIO_NUVOTON; + sio->type = nct6116d; + break; default: - pr_info(DRVNAME ": Unsupported Fintek device 0x%04x\n", devid); + pr_info(DRVNAME ": Unsupported device 0x%04x\n", devid); goto err; } + + /* double check manufacturer where possible */ + if (sio->type != nct6116d) { + manid = superio_inw(addr, SIO_FINTEK_MANID); + if (manid != SIO_FINTEK_ID) { + pr_debug(DRVNAME ": Not a Fintek device at 0x%08x\n", addr); + goto err; + } + } + sio->addr = addr; err = 0; - pr_info(DRVNAME ": Found %s at %#x, revision %d\n", + pr_info(DRVNAME ": Found %s at %#x\n", f7188x_names[sio->type], - (unsigned int) addr, - (int) superio_inb(addr, SIO_DEVREV)); + (unsigned int)addr); + if (sio->type != nct6116d) + pr_info(DRVNAME ": revision %d\n", + (int)superio_inb(addr, SIO_FINTEK_DEVREV)); err: superio_exit(addr); -- 2.35.1 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v4 1/5] gpio-f7188x: Add GPIO support for Nuvoton NCT6116 2022-08-23 10:23 ` [PATCH v4 1/5] gpio-f7188x: Add GPIO support for Nuvoton NCT6116 Henning Schild @ 2022-08-23 14:47 ` Andy Shevchenko 2022-08-23 14:54 ` Henning Schild 0 siblings, 1 reply; 16+ messages in thread From: Andy Shevchenko @ 2022-08-23 14:47 UTC (permalink / raw) To: Henning Schild Cc: Linus Walleij, Bartosz Golaszewski, Pavel Machek, Hans de Goede, Mark Gross, Lee Jones, linux-gpio, linux-kernel, linux-leds, platform-driver-x86, Sheng-Yuan Huang, Tasanakorn Phaipool, simon.guinot On Tue, Aug 23, 2022 at 12:23:40PM +0200, Henning Schild 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. > > On a chip level we do not have a manufacturer ID to check and also no > revision. ... > - * GPIO driver for Fintek Super-I/O F71869, F71869A, F71882, F71889 and F81866 > + * GPIO driver for Fintek and Nuvoton Super-I/O chips I'm not sure it's good idea to drop it from here. It means reader has to get this info in a hard way. ... > +#define gpio_dir_invert(type) ((type) == nct6116d) > +#define gpio_data_single(type) ((type) == nct6116d) What's prevents us to add a proper prefix to these? I don't like the idea of them having "gpio" prefix. ... > + pr_info(DRVNAME ": Unsupported device 0x%04x\n", devid); > + pr_debug(DRVNAME ": Not a Fintek device at 0x%08x\n", addr); > + pr_info(DRVNAME ": Found %s at %#x\n", > + pr_info(DRVNAME ": revision %d\n", Can we, please, utilize pr_fmt()? > + (int)superio_inb(addr, SIO_FINTEK_DEVREV)); Explicit casting in printf() means wrong specifier in 99% of cases. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v4 1/5] gpio-f7188x: Add GPIO support for Nuvoton NCT6116 2022-08-23 14:47 ` Andy Shevchenko @ 2022-08-23 14:54 ` Henning Schild 2022-08-24 13:10 ` simon.guinot 0 siblings, 1 reply; 16+ messages in thread From: Henning Schild @ 2022-08-23 14:54 UTC (permalink / raw) To: Andy Shevchenko Cc: Linus Walleij, Bartosz Golaszewski, Pavel Machek, Hans de Goede, Mark Gross, Lee Jones, linux-gpio, linux-kernel, linux-leds, platform-driver-x86, Sheng-Yuan Huang, Tasanakorn Phaipool, simon.guinot Am Tue, 23 Aug 2022 17:47:38 +0300 schrieb Andy Shevchenko <andriy.shevchenko@linux.intel.com>: > On Tue, Aug 23, 2022 at 12:23:40PM +0200, Henning Schild 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. > > > > On a chip level we do not have a manufacturer ID to check and also > > no revision. > > ... > > > - * GPIO driver for Fintek Super-I/O F71869, F71869A, F71882, > > F71889 and F81866 > > + * GPIO driver for Fintek and Nuvoton Super-I/O chips > > I'm not sure it's good idea to drop it from here. It means reader has > to get this info in a hard way. > > ... Let us see what others say. I wanted to keep this in line with what Kconfig says and the oneliner in the Kconfig was getting pretty longish. Hence i decided to shorten that. Other drivers also seem to not list all the possible chips in many places, it is all maint effort when a new chips is added and the list is in like 5 places. > > +#define gpio_dir_invert(type) ((type) == nct6116d) > > +#define gpio_data_single(type) ((type) == nct6116d) > > What's prevents us to add a proper prefix to these? I don't like the > idea of them having "gpio" prefix. > > ... > > > + pr_info(DRVNAME ": Unsupported device 0x%04x\n", > > devid); > > + pr_debug(DRVNAME ": Not a Fintek device at > > 0x%08x\n", addr); > > + pr_info(DRVNAME ": Found %s at %#x\n", > > + pr_info(DRVNAME ": revision %d\n", > > Can we, please, utilize pr_fmt()? > > > + (int)superio_inb(addr, > > SIO_FINTEK_DEVREV)); > > Explicit casting in printf() means wrong specifier in 99% of cases. > For all the other comments i will wait for a second opinion. I specifically did not change existing code for more than the functional changes needed. And a bit of checkpatch.pl fixing. Beautification could be done on the way but would only cause inconsistency. That driver is what it is, if someone wants to overhaul the style ... that should be another patch. One likely not coming from me. Henning ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v4 1/5] gpio-f7188x: Add GPIO support for Nuvoton NCT6116 2022-08-23 14:54 ` Henning Schild @ 2022-08-24 13:10 ` simon.guinot 2022-08-24 13:50 ` Henning Schild 0 siblings, 1 reply; 16+ messages in thread From: simon.guinot @ 2022-08-24 13:10 UTC (permalink / raw) To: Henning Schild Cc: Andy Shevchenko, Linus Walleij, Bartosz Golaszewski, Pavel Machek, Hans de Goede, Mark Gross, Lee Jones, linux-gpio, linux-kernel, linux-leds, platform-driver-x86, Sheng-Yuan Huang, Tasanakorn Phaipool [-- Attachment #1: Type: text/plain, Size: 3006 bytes --] On Tue, Aug 23, 2022 at 04:54:59PM +0200, Henning Schild wrote: > Am Tue, 23 Aug 2022 17:47:38 +0300 > schrieb Andy Shevchenko <andriy.shevchenko@linux.intel.com>: Hi Andy, Thanks for this new version. It is looking good to me. > > > On Tue, Aug 23, 2022 at 12:23:40PM +0200, Henning Schild 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. > > > > > > On a chip level we do not have a manufacturer ID to check and also > > > no revision. > > > > ... > > > > > - * GPIO driver for Fintek Super-I/O F71869, F71869A, F71882, > > > F71889 and F81866 > > > + * GPIO driver for Fintek and Nuvoton Super-I/O chips > > > > I'm not sure it's good idea to drop it from here. It means reader has > > to get this info in a hard way. > > > > ... > > Let us see what others say. I wanted to keep this in line with what > Kconfig says and the oneliner in the Kconfig was getting pretty > longish. Hence i decided to shorten that. Other drivers also seem to > not list all the possible chips in many places, it is all maint effort > when a new chips is added and the list is in like 5 places. I agree with you that we can drop this line. It was already incomplete and the information is quite readable a few lines below in both the define list and the chip enumeration. > > > > +#define gpio_dir_invert(type) ((type) == nct6116d) > > > +#define gpio_data_single(type) ((type) == nct6116d) > > > > What's prevents us to add a proper prefix to these? I don't like the > > idea of them having "gpio" prefix. > > > > ... > > > > > + pr_info(DRVNAME ": Unsupported device 0x%04x\n", > > > devid); > > > + pr_debug(DRVNAME ": Not a Fintek device at > > > 0x%08x\n", addr); > > > + pr_info(DRVNAME ": Found %s at %#x\n", > > > + pr_info(DRVNAME ": revision %d\n", > > > > Can we, please, utilize pr_fmt()? > > > > > + (int)superio_inb(addr, > > > SIO_FINTEK_DEVREV)); > > > > Explicit casting in printf() means wrong specifier in 99% of cases. > > > > For all the other comments i will wait for a second opinion. I > specifically did not change existing code for more than the functional > changes needed. And a bit of checkpatch.pl fixing. > Beautification could be done on the way but would only cause > inconsistency. That driver is what it is, if someone wants to overhaul > the style ... that should be another patch. One likely not coming from > me. About the int cast, I think you can drop it while you are updating this line. It is unneeded. I have no opinion on the other comments and I am OK with the rest of the patch. Simon [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v4 1/5] gpio-f7188x: Add GPIO support for Nuvoton NCT6116 2022-08-24 13:10 ` simon.guinot @ 2022-08-24 13:50 ` Henning Schild 2022-08-24 13:54 ` Hans de Goede 0 siblings, 1 reply; 16+ messages in thread From: Henning Schild @ 2022-08-24 13:50 UTC (permalink / raw) To: simon.guinot Cc: Andy Shevchenko, Linus Walleij, Bartosz Golaszewski, Pavel Machek, Hans de Goede, Mark Gross, Lee Jones, linux-gpio, linux-kernel, linux-leds, platform-driver-x86, Sheng-Yuan Huang, Tasanakorn Phaipool Am Wed, 24 Aug 2022 15:10:55 +0200 schrieb simon.guinot@sequanux.org: > On Tue, Aug 23, 2022 at 04:54:59PM +0200, Henning Schild wrote: > > Am Tue, 23 Aug 2022 17:47:38 +0300 > > schrieb Andy Shevchenko <andriy.shevchenko@linux.intel.com>: > > Hi Andy, > > Thanks for this new version. It is looking good to me. > > > > > > On Tue, Aug 23, 2022 at 12:23:40PM +0200, Henning Schild 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. > > > > > > > > On a chip level we do not have a manufacturer ID to check and > > > > also no revision. > > > > > > ... > > > > > > > - * GPIO driver for Fintek Super-I/O F71869, F71869A, F71882, > > > > F71889 and F81866 > > > > + * GPIO driver for Fintek and Nuvoton Super-I/O chips > > > > > > I'm not sure it's good idea to drop it from here. It means reader > > > has to get this info in a hard way. > > > > > > ... > > > > Let us see what others say. I wanted to keep this in line with what > > Kconfig says and the oneliner in the Kconfig was getting pretty > > longish. Hence i decided to shorten that. Other drivers also seem to > > not list all the possible chips in many places, it is all maint > > effort when a new chips is added and the list is in like 5 places. > > I agree with you that we can drop this line. It was already incomplete > and the information is quite readable a few lines below in both the > define list and the chip enumeration. > > > > > > > +#define gpio_dir_invert(type) ((type) == nct6116d) > > > > +#define gpio_data_single(type) ((type) == nct6116d) > > > > > > What's prevents us to add a proper prefix to these? I don't like > > > the idea of them having "gpio" prefix. > > > > > > ... > > > > > > > + pr_info(DRVNAME ": Unsupported device > > > > 0x%04x\n", devid); > > > > + pr_debug(DRVNAME ": Not a Fintek > > > > device at 0x%08x\n", addr); > > > > + pr_info(DRVNAME ": Found %s at %#x\n", > > > > + pr_info(DRVNAME ": revision %d\n", > > > > > > Can we, please, utilize pr_fmt()? > > > > > > > + (int)superio_inb(addr, > > > > SIO_FINTEK_DEVREV)); > > > > > > Explicit casting in printf() means wrong specifier in 99% of > > > cases. > > > > For all the other comments i will wait for a second opinion. I > > specifically did not change existing code for more than the > > functional changes needed. And a bit of checkpatch.pl fixing. > > Beautification could be done on the way but would only cause > > inconsistency. That driver is what it is, if someone wants to > > overhaul the style ... that should be another patch. One likely not > > coming from me. > > About the int cast, I think you can drop it while you are updating > this line. It is unneeded. Ok two voices for doing that one fix along the way. I will send a v5 and hope nobody insists on me fixing the other findings in code i never wrote. regards, Henning > I have no opinion on the other comments and I am OK with the rest of > the patch. > > Simon ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v4 1/5] gpio-f7188x: Add GPIO support for Nuvoton NCT6116 2022-08-24 13:50 ` Henning Schild @ 2022-08-24 13:54 ` Hans de Goede 2022-08-24 14:17 ` Henning Schild 0 siblings, 1 reply; 16+ messages in thread From: Hans de Goede @ 2022-08-24 13:54 UTC (permalink / raw) To: Henning Schild, simon.guinot Cc: Andy Shevchenko, Linus Walleij, Bartosz Golaszewski, Pavel Machek, Mark Gross, Lee Jones, linux-gpio, linux-kernel, linux-leds, platform-driver-x86, Sheng-Yuan Huang, Tasanakorn Phaipool Hi Henning, On 8/24/22 15:50, Henning Schild wrote: > Am Wed, 24 Aug 2022 15:10:55 +0200 > schrieb simon.guinot@sequanux.org: > >> On Tue, Aug 23, 2022 at 04:54:59PM +0200, Henning Schild wrote: >>> Am Tue, 23 Aug 2022 17:47:38 +0300 >>> schrieb Andy Shevchenko <andriy.shevchenko@linux.intel.com>: >> >> Hi Andy, >> >> Thanks for this new version. It is looking good to me. >> >>> >>>> On Tue, Aug 23, 2022 at 12:23:40PM +0200, Henning Schild 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. >>>>> >>>>> On a chip level we do not have a manufacturer ID to check and >>>>> also no revision. >>>> >>>> ... >>>> >>>>> - * GPIO driver for Fintek Super-I/O F71869, F71869A, F71882, >>>>> F71889 and F81866 >>>>> + * GPIO driver for Fintek and Nuvoton Super-I/O chips >>>> >>>> I'm not sure it's good idea to drop it from here. It means reader >>>> has to get this info in a hard way. >>>> >>>> ... >>> >>> Let us see what others say. I wanted to keep this in line with what >>> Kconfig says and the oneliner in the Kconfig was getting pretty >>> longish. Hence i decided to shorten that. Other drivers also seem to >>> not list all the possible chips in many places, it is all maint >>> effort when a new chips is added and the list is in like 5 places. >> >> I agree with you that we can drop this line. It was already incomplete >> and the information is quite readable a few lines below in both the >> define list and the chip enumeration. >> >>> >>>>> +#define gpio_dir_invert(type) ((type) == nct6116d) >>>>> +#define gpio_data_single(type) ((type) == nct6116d) >>>> >>>> What's prevents us to add a proper prefix to these? I don't like >>>> the idea of them having "gpio" prefix. >>>> >>>> ... >>>> >>>>> + pr_info(DRVNAME ": Unsupported device >>>>> 0x%04x\n", devid); >>>>> + pr_debug(DRVNAME ": Not a Fintek >>>>> device at 0x%08x\n", addr); >>>>> + pr_info(DRVNAME ": Found %s at %#x\n", >>>>> + pr_info(DRVNAME ": revision %d\n", >>>> >>>> Can we, please, utilize pr_fmt()? >>>> >>>>> + (int)superio_inb(addr, >>>>> SIO_FINTEK_DEVREV)); >>>> >>>> Explicit casting in printf() means wrong specifier in 99% of >>>> cases. >>> >>> For all the other comments i will wait for a second opinion. I >>> specifically did not change existing code for more than the >>> functional changes needed. And a bit of checkpatch.pl fixing. >>> Beautification could be done on the way but would only cause >>> inconsistency. That driver is what it is, if someone wants to >>> overhaul the style ... that should be another patch. One likely not >>> coming from me. >> >> About the int cast, I think you can drop it while you are updating >> this line. It is unneeded. > > Ok two voices for doing that one fix along the way. I will send a v5 > and hope nobody insists on me fixing the other findings in code i never > wrote. You did not write it, but you are using it to do hw-enablement for your company's products. So being asked to also some touch-ups left and right while you are at it really is not unexpected IMHO. Regards, Hans ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v4 1/5] gpio-f7188x: Add GPIO support for Nuvoton NCT6116 2022-08-24 13:54 ` Hans de Goede @ 2022-08-24 14:17 ` Henning Schild 2022-08-24 14:24 ` Hans de Goede 2022-08-26 13:30 ` Linus Walleij 0 siblings, 2 replies; 16+ messages in thread From: Henning Schild @ 2022-08-24 14:17 UTC (permalink / raw) To: Hans de Goede Cc: simon.guinot, Andy Shevchenko, Linus Walleij, Bartosz Golaszewski, Pavel Machek, Mark Gross, Lee Jones, linux-gpio, linux-kernel, linux-leds, platform-driver-x86, Sheng-Yuan Huang, Tasanakorn Phaipool Am Wed, 24 Aug 2022 15:54:28 +0200 schrieb Hans de Goede <hdegoede@redhat.com>: > Hi Henning, > > On 8/24/22 15:50, Henning Schild wrote: > > Am Wed, 24 Aug 2022 15:10:55 +0200 > > schrieb simon.guinot@sequanux.org: > > > >> On Tue, Aug 23, 2022 at 04:54:59PM +0200, Henning Schild wrote: > >>> Am Tue, 23 Aug 2022 17:47:38 +0300 > >>> schrieb Andy Shevchenko <andriy.shevchenko@linux.intel.com>: > >> > >> Hi Andy, > >> > >> Thanks for this new version. It is looking good to me. > >> > >>> > >>>> On Tue, Aug 23, 2022 at 12:23:40PM +0200, Henning Schild 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. > >>>>> > >>>>> On a chip level we do not have a manufacturer ID to check and > >>>>> also no revision. > >>>> > >>>> ... > >>>> > >>>>> - * GPIO driver for Fintek Super-I/O F71869, F71869A, F71882, > >>>>> F71889 and F81866 > >>>>> + * GPIO driver for Fintek and Nuvoton Super-I/O chips > >>>> > >>>> I'm not sure it's good idea to drop it from here. It means reader > >>>> has to get this info in a hard way. > >>>> > >>>> ... > >>> > >>> Let us see what others say. I wanted to keep this in line with > >>> what Kconfig says and the oneliner in the Kconfig was getting > >>> pretty longish. Hence i decided to shorten that. Other drivers > >>> also seem to not list all the possible chips in many places, it > >>> is all maint effort when a new chips is added and the list is in > >>> like 5 places. > >> > >> I agree with you that we can drop this line. It was already > >> incomplete and the information is quite readable a few lines below > >> in both the define list and the chip enumeration. > >> > >>> > >>>>> +#define gpio_dir_invert(type) ((type) == nct6116d) > >>>>> +#define gpio_data_single(type) ((type) == nct6116d) > >>>>> > >>>> > >>>> What's prevents us to add a proper prefix to these? I don't like > >>>> the idea of them having "gpio" prefix. > >>>> > >>>> ... > >>>> > >>>>> + pr_info(DRVNAME ": Unsupported device > >>>>> 0x%04x\n", devid); > >>>>> + pr_debug(DRVNAME ": Not a Fintek > >>>>> device at 0x%08x\n", addr); > >>>>> + pr_info(DRVNAME ": Found %s at %#x\n", > >>>>> + pr_info(DRVNAME ": revision %d\n", > >>>> > >>>> Can we, please, utilize pr_fmt()? > >>>> > >>>>> + (int)superio_inb(addr, > >>>>> SIO_FINTEK_DEVREV)); > >>>> > >>>> Explicit casting in printf() means wrong specifier in 99% of > >>>> cases. > >>> > >>> For all the other comments i will wait for a second opinion. I > >>> specifically did not change existing code for more than the > >>> functional changes needed. And a bit of checkpatch.pl fixing. > >>> Beautification could be done on the way but would only cause > >>> inconsistency. That driver is what it is, if someone wants to > >>> overhaul the style ... that should be another patch. One likely > >>> not coming from me. > >> > >> About the int cast, I think you can drop it while you are updating > >> this line. It is unneeded. > > > > Ok two voices for doing that one fix along the way. I will send a v5 > > and hope nobody insists on me fixing the other findings in code i > > never wrote. > > You did not write it, but you are using it to do hw-enablement for > your company's products. So being asked to also some touch-ups > left and right while you are at it really is not unexpected IMHO. Sure thing. Dropping a few characters from a line i touch anyhow is easy enough. But i.e a refactoring to pr_fmt would feel like asking too much in my book. That feels like work of the author or maintainer. In fact i am just doing the homework of what i think should have long been done by Nuvoton. I hope that v5 will be acceptable. Henning > Regards, > > Hans > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v4 1/5] gpio-f7188x: Add GPIO support for Nuvoton NCT6116 2022-08-24 14:17 ` Henning Schild @ 2022-08-24 14:24 ` Hans de Goede 2022-08-24 16:02 ` simon.guinot 2022-08-26 13:30 ` Linus Walleij 1 sibling, 1 reply; 16+ messages in thread From: Hans de Goede @ 2022-08-24 14:24 UTC (permalink / raw) To: Henning Schild Cc: simon.guinot, Andy Shevchenko, Linus Walleij, Bartosz Golaszewski, Pavel Machek, Mark Gross, Lee Jones, linux-gpio, linux-kernel, linux-leds, platform-driver-x86, Sheng-Yuan Huang, Tasanakorn Phaipool Hi, On 8/24/22 16:17, Henning Schild wrote: > Am Wed, 24 Aug 2022 15:54:28 +0200 > schrieb Hans de Goede <hdegoede@redhat.com>: > >> Hi Henning, >> >> On 8/24/22 15:50, Henning Schild wrote: >>> Am Wed, 24 Aug 2022 15:10:55 +0200 >>> schrieb simon.guinot@sequanux.org: >>> >>>> On Tue, Aug 23, 2022 at 04:54:59PM +0200, Henning Schild wrote: >>>>> Am Tue, 23 Aug 2022 17:47:38 +0300 >>>>> schrieb Andy Shevchenko <andriy.shevchenko@linux.intel.com>: >>>> >>>> Hi Andy, >>>> >>>> Thanks for this new version. It is looking good to me. >>>> >>>>> >>>>>> On Tue, Aug 23, 2022 at 12:23:40PM +0200, Henning Schild 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. >>>>>>> >>>>>>> On a chip level we do not have a manufacturer ID to check and >>>>>>> also no revision. >>>>>> >>>>>> ... >>>>>> >>>>>>> - * GPIO driver for Fintek Super-I/O F71869, F71869A, F71882, >>>>>>> F71889 and F81866 >>>>>>> + * GPIO driver for Fintek and Nuvoton Super-I/O chips >>>>>> >>>>>> I'm not sure it's good idea to drop it from here. It means reader >>>>>> has to get this info in a hard way. >>>>>> >>>>>> ... >>>>> >>>>> Let us see what others say. I wanted to keep this in line with >>>>> what Kconfig says and the oneliner in the Kconfig was getting >>>>> pretty longish. Hence i decided to shorten that. Other drivers >>>>> also seem to not list all the possible chips in many places, it >>>>> is all maint effort when a new chips is added and the list is in >>>>> like 5 places. >>>> >>>> I agree with you that we can drop this line. It was already >>>> incomplete and the information is quite readable a few lines below >>>> in both the define list and the chip enumeration. >>>> >>>>> >>>>>>> +#define gpio_dir_invert(type) ((type) == nct6116d) >>>>>>> +#define gpio_data_single(type) ((type) == nct6116d) >>>>>>> >>>>>> >>>>>> What's prevents us to add a proper prefix to these? I don't like >>>>>> the idea of them having "gpio" prefix. >>>>>> >>>>>> ... >>>>>> >>>>>>> + pr_info(DRVNAME ": Unsupported device >>>>>>> 0x%04x\n", devid); >>>>>>> + pr_debug(DRVNAME ": Not a Fintek >>>>>>> device at 0x%08x\n", addr); >>>>>>> + pr_info(DRVNAME ": Found %s at %#x\n", >>>>>>> + pr_info(DRVNAME ": revision %d\n", >>>>>> >>>>>> Can we, please, utilize pr_fmt()? >>>>>> >>>>>>> + (int)superio_inb(addr, >>>>>>> SIO_FINTEK_DEVREV)); >>>>>> >>>>>> Explicit casting in printf() means wrong specifier in 99% of >>>>>> cases. >>>>> >>>>> For all the other comments i will wait for a second opinion. I >>>>> specifically did not change existing code for more than the >>>>> functional changes needed. And a bit of checkpatch.pl fixing. >>>>> Beautification could be done on the way but would only cause >>>>> inconsistency. That driver is what it is, if someone wants to >>>>> overhaul the style ... that should be another patch. One likely >>>>> not coming from me. >>>> >>>> About the int cast, I think you can drop it while you are updating >>>> this line. It is unneeded. >>> >>> Ok two voices for doing that one fix along the way. I will send a v5 >>> and hope nobody insists on me fixing the other findings in code i >>> never wrote. >> >> You did not write it, but you are using it to do hw-enablement for >> your company's products. So being asked to also some touch-ups >> left and right while you are at it really is not unexpected IMHO. > > Sure thing. Dropping a few characters from a line i touch anyhow is > easy enough. But i.e a refactoring to pr_fmt would feel like asking too > much in my book. That feels like work of the author or maintainer. Right, but that assumes that the original author / maintainer is still around and actively contributing which unfortunately is not always the case. Regards, Hans ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v4 1/5] gpio-f7188x: Add GPIO support for Nuvoton NCT6116 2022-08-24 14:24 ` Hans de Goede @ 2022-08-24 16:02 ` simon.guinot 0 siblings, 0 replies; 16+ messages in thread From: simon.guinot @ 2022-08-24 16:02 UTC (permalink / raw) To: Hans de Goede Cc: Henning Schild, Andy Shevchenko, Linus Walleij, Bartosz Golaszewski, Pavel Machek, Mark Gross, Lee Jones, linux-gpio, linux-kernel, linux-leds, platform-driver-x86, Sheng-Yuan Huang, Tasanakorn Phaipool [-- Attachment #1: Type: text/plain, Size: 4935 bytes --] On Wed, Aug 24, 2022 at 04:24:46PM +0200, Hans de Goede wrote: > Hi, > > On 8/24/22 16:17, Henning Schild wrote: > > Am Wed, 24 Aug 2022 15:54:28 +0200 > > schrieb Hans de Goede <hdegoede@redhat.com>: > > > >> Hi Henning, > >> > >> On 8/24/22 15:50, Henning Schild wrote: > >>> Am Wed, 24 Aug 2022 15:10:55 +0200 > >>> schrieb simon.guinot@sequanux.org: > >>> > >>>> On Tue, Aug 23, 2022 at 04:54:59PM +0200, Henning Schild wrote: > >>>>> Am Tue, 23 Aug 2022 17:47:38 +0300 > >>>>> schrieb Andy Shevchenko <andriy.shevchenko@linux.intel.com>: > >>>> > >>>> Hi Andy, > >>>> > >>>> Thanks for this new version. It is looking good to me. > >>>> > >>>>> > >>>>>> On Tue, Aug 23, 2022 at 12:23:40PM +0200, Henning Schild 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. > >>>>>>> > >>>>>>> On a chip level we do not have a manufacturer ID to check and > >>>>>>> also no revision. > >>>>>> > >>>>>> ... > >>>>>> > >>>>>>> - * GPIO driver for Fintek Super-I/O F71869, F71869A, F71882, > >>>>>>> F71889 and F81866 > >>>>>>> + * GPIO driver for Fintek and Nuvoton Super-I/O chips > >>>>>> > >>>>>> I'm not sure it's good idea to drop it from here. It means reader > >>>>>> has to get this info in a hard way. > >>>>>> > >>>>>> ... > >>>>> > >>>>> Let us see what others say. I wanted to keep this in line with > >>>>> what Kconfig says and the oneliner in the Kconfig was getting > >>>>> pretty longish. Hence i decided to shorten that. Other drivers > >>>>> also seem to not list all the possible chips in many places, it > >>>>> is all maint effort when a new chips is added and the list is in > >>>>> like 5 places. > >>>> > >>>> I agree with you that we can drop this line. It was already > >>>> incomplete and the information is quite readable a few lines below > >>>> in both the define list and the chip enumeration. > >>>> > >>>>> > >>>>>>> +#define gpio_dir_invert(type) ((type) == nct6116d) > >>>>>>> +#define gpio_data_single(type) ((type) == nct6116d) > >>>>>>> > >>>>>> > >>>>>> What's prevents us to add a proper prefix to these? I don't like > >>>>>> the idea of them having "gpio" prefix. > >>>>>> > >>>>>> ... > >>>>>> > >>>>>>> + pr_info(DRVNAME ": Unsupported device > >>>>>>> 0x%04x\n", devid); > >>>>>>> + pr_debug(DRVNAME ": Not a Fintek > >>>>>>> device at 0x%08x\n", addr); > >>>>>>> + pr_info(DRVNAME ": Found %s at %#x\n", > >>>>>>> + pr_info(DRVNAME ": revision %d\n", > >>>>>> > >>>>>> Can we, please, utilize pr_fmt()? > >>>>>> > >>>>>>> + (int)superio_inb(addr, > >>>>>>> SIO_FINTEK_DEVREV)); > >>>>>> > >>>>>> Explicit casting in printf() means wrong specifier in 99% of > >>>>>> cases. > >>>>> > >>>>> For all the other comments i will wait for a second opinion. I > >>>>> specifically did not change existing code for more than the > >>>>> functional changes needed. And a bit of checkpatch.pl fixing. > >>>>> Beautification could be done on the way but would only cause > >>>>> inconsistency. That driver is what it is, if someone wants to > >>>>> overhaul the style ... that should be another patch. One likely > >>>>> not coming from me. > >>>> > >>>> About the int cast, I think you can drop it while you are updating > >>>> this line. It is unneeded. > >>> > >>> Ok two voices for doing that one fix along the way. I will send a v5 > >>> and hope nobody insists on me fixing the other findings in code i > >>> never wrote. > >> > >> You did not write it, but you are using it to do hw-enablement for > >> your company's products. So being asked to also some touch-ups > >> left and right while you are at it really is not unexpected IMHO. > > > > Sure thing. Dropping a few characters from a line i touch anyhow is > > easy enough. But i.e a refactoring to pr_fmt would feel like asking too > > much in my book. That feels like work of the author or maintainer. > > Right, but that assumes that the original author / maintainer is still > around and actively contributing which unfortunately is not always > the case. Actually the original author is not active but he is still keeping an eye on the driver :) I still review and test the patches I catch on the MLs. And I am ready to do some maintenance work if needed. Henning, I think you could have done the pr_fmt conversion. It is not a big deal and it would have been nice. But indeed, you don't have to... Simon [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v4 1/5] gpio-f7188x: Add GPIO support for Nuvoton NCT6116 2022-08-24 14:17 ` Henning Schild 2022-08-24 14:24 ` Hans de Goede @ 2022-08-26 13:30 ` Linus Walleij 1 sibling, 0 replies; 16+ messages in thread From: Linus Walleij @ 2022-08-26 13:30 UTC (permalink / raw) To: Henning Schild Cc: Hans de Goede, simon.guinot, Andy Shevchenko, Bartosz Golaszewski, Pavel Machek, Mark Gross, Lee Jones, linux-gpio, linux-kernel, linux-leds, platform-driver-x86, Sheng-Yuan Huang, Tasanakorn Phaipool On Wed, Aug 24, 2022 at 4:18 PM Henning Schild <henning.schild@siemens.com> wrote: > > You did not write it, but you are using it to do hw-enablement for > > your company's products. So being asked to also some touch-ups > > left and right while you are at it really is not unexpected IMHO. > > Sure thing. Dropping a few characters from a line i touch anyhow is > easy enough. But i.e a refactoring to pr_fmt would feel like asking too > much in my book. That feels like work of the author or maintainer. > > In fact i am just doing the homework of what i think should have long > been done by Nuvoton. A lot of vendors don't have much active upstream participation, they outsource that work to people like yourself by just ignoring it. > I hope that v5 will be acceptable. Bartosz is applying GPIO patches now, but my principle was that when I feel a patch makes the kernel look better after than before the patch and no new version is coming, I just apply the patch. This is how we deal with "perfect is the enemy of good" in practice. That said, we are all grateful for any improvements you manage to sneak in! Yours, Linus Walleij ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v4 2/5] gpio-f7188x: use unique labels for banks/chips 2022-08-23 10:23 [PATCH v4 0/5] add support for another simatic board Henning Schild 2022-08-23 10:23 ` [PATCH v4 1/5] gpio-f7188x: Add GPIO support for Nuvoton NCT6116 Henning Schild @ 2022-08-23 10:23 ` Henning Schild 2022-08-23 10:23 ` [PATCH v4 3/5] leds: simatic-ipc-leds-gpio: add new model 227G Henning Schild ` (3 subsequent siblings) 5 siblings, 0 replies; 16+ messages in thread From: Henning Schild @ 2022-08-23 10:23 UTC (permalink / raw) To: Linus Walleij, Bartosz Golaszewski, Pavel Machek, Hans de Goede, Mark Gross, Andy Shevchenko, Lee Jones, linux-gpio, linux-kernel, linux-leds, platform-driver-x86 Cc: Sheng-Yuan Huang, Tasanakorn Phaipool, simon.guinot, Henning Schild So that drivers building on top can find those pins with GPIO_LOOKUP helpers. Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Acked-by: Simon Guinot <simon.guinot@sequanux.org> Signed-off-by: Henning Schild <henning.schild@siemens.com> --- drivers/gpio/gpio-f7188x.c | 138 ++++++++++++++++++------------------- 1 file changed, 69 insertions(+), 69 deletions(-) diff --git a/drivers/gpio/gpio-f7188x.c b/drivers/gpio/gpio-f7188x.c index 820ac5d60fda..c9b18c6d8464 100644 --- a/drivers/gpio/gpio-f7188x.c +++ b/drivers/gpio/gpio-f7188x.c @@ -162,10 +162,10 @@ static void f7188x_gpio_set(struct gpio_chip *chip, unsigned offset, int value); static int f7188x_gpio_set_config(struct gpio_chip *chip, unsigned offset, unsigned long config); -#define F7188X_GPIO_BANK(_base, _ngpio, _regbase) \ +#define F7188X_GPIO_BANK(_base, _ngpio, _regbase, _label) \ { \ .chip = { \ - .label = DRVNAME, \ + .label = _label, \ .owner = THIS_MODULE, \ .get_direction = f7188x_gpio_get_direction, \ .direction_input = f7188x_gpio_direction_in, \ @@ -190,98 +190,98 @@ static int f7188x_gpio_set_config(struct gpio_chip *chip, unsigned offset, #define gpio_data_single(type) ((type) == nct6116d) static struct f7188x_gpio_bank f71869_gpio_bank[] = { - F7188X_GPIO_BANK(0, 6, 0xF0), - F7188X_GPIO_BANK(10, 8, 0xE0), - F7188X_GPIO_BANK(20, 8, 0xD0), - F7188X_GPIO_BANK(30, 8, 0xC0), - F7188X_GPIO_BANK(40, 8, 0xB0), - F7188X_GPIO_BANK(50, 5, 0xA0), - F7188X_GPIO_BANK(60, 6, 0x90), + F7188X_GPIO_BANK(0, 6, 0xF0, DRVNAME "-0"), + F7188X_GPIO_BANK(10, 8, 0xE0, DRVNAME "-1"), + F7188X_GPIO_BANK(20, 8, 0xD0, DRVNAME "-2"), + F7188X_GPIO_BANK(30, 8, 0xC0, DRVNAME "-3"), + F7188X_GPIO_BANK(40, 8, 0xB0, DRVNAME "-4"), + F7188X_GPIO_BANK(50, 5, 0xA0, DRVNAME "-5"), + F7188X_GPIO_BANK(60, 6, 0x90, DRVNAME "-6"), }; static struct f7188x_gpio_bank f71869a_gpio_bank[] = { - F7188X_GPIO_BANK(0, 6, 0xF0), - F7188X_GPIO_BANK(10, 8, 0xE0), - F7188X_GPIO_BANK(20, 8, 0xD0), - F7188X_GPIO_BANK(30, 8, 0xC0), - F7188X_GPIO_BANK(40, 8, 0xB0), - F7188X_GPIO_BANK(50, 5, 0xA0), - F7188X_GPIO_BANK(60, 8, 0x90), - F7188X_GPIO_BANK(70, 8, 0x80), + F7188X_GPIO_BANK(0, 6, 0xF0, DRVNAME "-0"), + F7188X_GPIO_BANK(10, 8, 0xE0, DRVNAME "-1"), + F7188X_GPIO_BANK(20, 8, 0xD0, DRVNAME "-2"), + F7188X_GPIO_BANK(30, 8, 0xC0, DRVNAME "-3"), + F7188X_GPIO_BANK(40, 8, 0xB0, DRVNAME "-4"), + F7188X_GPIO_BANK(50, 5, 0xA0, DRVNAME "-5"), + F7188X_GPIO_BANK(60, 8, 0x90, DRVNAME "-6"), + F7188X_GPIO_BANK(70, 8, 0x80, DRVNAME "-7"), }; static struct f7188x_gpio_bank f71882_gpio_bank[] = { - F7188X_GPIO_BANK(0, 8, 0xF0), - F7188X_GPIO_BANK(10, 8, 0xE0), - F7188X_GPIO_BANK(20, 8, 0xD0), - F7188X_GPIO_BANK(30, 4, 0xC0), - F7188X_GPIO_BANK(40, 4, 0xB0), + F7188X_GPIO_BANK(0, 8, 0xF0, DRVNAME "-0"), + F7188X_GPIO_BANK(10, 8, 0xE0, DRVNAME "-1"), + F7188X_GPIO_BANK(20, 8, 0xD0, DRVNAME "-2"), + F7188X_GPIO_BANK(30, 4, 0xC0, DRVNAME "-3"), + F7188X_GPIO_BANK(40, 4, 0xB0, DRVNAME "-4"), }; static struct f7188x_gpio_bank f71889a_gpio_bank[] = { - F7188X_GPIO_BANK(0, 7, 0xF0), - F7188X_GPIO_BANK(10, 7, 0xE0), - F7188X_GPIO_BANK(20, 8, 0xD0), - F7188X_GPIO_BANK(30, 8, 0xC0), - F7188X_GPIO_BANK(40, 8, 0xB0), - F7188X_GPIO_BANK(50, 5, 0xA0), - F7188X_GPIO_BANK(60, 8, 0x90), - F7188X_GPIO_BANK(70, 8, 0x80), + F7188X_GPIO_BANK(0, 7, 0xF0, DRVNAME "-0"), + F7188X_GPIO_BANK(10, 7, 0xE0, DRVNAME "-1"), + F7188X_GPIO_BANK(20, 8, 0xD0, DRVNAME "-2"), + F7188X_GPIO_BANK(30, 8, 0xC0, DRVNAME "-3"), + F7188X_GPIO_BANK(40, 8, 0xB0, DRVNAME "-4"), + F7188X_GPIO_BANK(50, 5, 0xA0, DRVNAME "-5"), + F7188X_GPIO_BANK(60, 8, 0x90, DRVNAME "-6"), + F7188X_GPIO_BANK(70, 8, 0x80, DRVNAME "-7"), }; static struct f7188x_gpio_bank f71889_gpio_bank[] = { - F7188X_GPIO_BANK(0, 7, 0xF0), - F7188X_GPIO_BANK(10, 7, 0xE0), - F7188X_GPIO_BANK(20, 8, 0xD0), - F7188X_GPIO_BANK(30, 8, 0xC0), - F7188X_GPIO_BANK(40, 8, 0xB0), - F7188X_GPIO_BANK(50, 5, 0xA0), - F7188X_GPIO_BANK(60, 8, 0x90), - F7188X_GPIO_BANK(70, 8, 0x80), + F7188X_GPIO_BANK(0, 7, 0xF0, DRVNAME "-0"), + F7188X_GPIO_BANK(10, 7, 0xE0, DRVNAME "-1"), + F7188X_GPIO_BANK(20, 8, 0xD0, DRVNAME "-2"), + F7188X_GPIO_BANK(30, 8, 0xC0, DRVNAME "-3"), + F7188X_GPIO_BANK(40, 8, 0xB0, DRVNAME "-4"), + F7188X_GPIO_BANK(50, 5, 0xA0, DRVNAME "-5"), + F7188X_GPIO_BANK(60, 8, 0x90, DRVNAME "-6"), + F7188X_GPIO_BANK(70, 8, 0x80, DRVNAME "-7"), }; static struct f7188x_gpio_bank f81866_gpio_bank[] = { - F7188X_GPIO_BANK(0, 8, 0xF0), - F7188X_GPIO_BANK(10, 8, 0xE0), - F7188X_GPIO_BANK(20, 8, 0xD0), - F7188X_GPIO_BANK(30, 8, 0xC0), - F7188X_GPIO_BANK(40, 8, 0xB0), - F7188X_GPIO_BANK(50, 8, 0xA0), - F7188X_GPIO_BANK(60, 8, 0x90), - F7188X_GPIO_BANK(70, 8, 0x80), - F7188X_GPIO_BANK(80, 8, 0x88), + F7188X_GPIO_BANK(0, 8, 0xF0, DRVNAME "-0"), + F7188X_GPIO_BANK(10, 8, 0xE0, DRVNAME "-1"), + F7188X_GPIO_BANK(20, 8, 0xD0, DRVNAME "-2"), + F7188X_GPIO_BANK(30, 8, 0xC0, DRVNAME "-3"), + F7188X_GPIO_BANK(40, 8, 0xB0, DRVNAME "-4"), + F7188X_GPIO_BANK(50, 8, 0xA0, DRVNAME "-5"), + F7188X_GPIO_BANK(60, 8, 0x90, DRVNAME "-6"), + F7188X_GPIO_BANK(70, 8, 0x80, DRVNAME "-7"), + F7188X_GPIO_BANK(80, 8, 0x88, DRVNAME "-8"), }; static struct f7188x_gpio_bank f81804_gpio_bank[] = { - F7188X_GPIO_BANK(0, 8, 0xF0), - F7188X_GPIO_BANK(10, 8, 0xE0), - F7188X_GPIO_BANK(20, 8, 0xD0), - F7188X_GPIO_BANK(50, 8, 0xA0), - F7188X_GPIO_BANK(60, 8, 0x90), - F7188X_GPIO_BANK(70, 8, 0x80), - F7188X_GPIO_BANK(90, 8, 0x98), + F7188X_GPIO_BANK(0, 8, 0xF0, DRVNAME "-0"), + F7188X_GPIO_BANK(10, 8, 0xE0, DRVNAME "-1"), + F7188X_GPIO_BANK(20, 8, 0xD0, DRVNAME "-2"), + F7188X_GPIO_BANK(50, 8, 0xA0, DRVNAME "-3"), + F7188X_GPIO_BANK(60, 8, 0x90, DRVNAME "-4"), + F7188X_GPIO_BANK(70, 8, 0x80, DRVNAME "-5"), + F7188X_GPIO_BANK(90, 8, 0x98, DRVNAME "-6"), }; static struct f7188x_gpio_bank f81865_gpio_bank[] = { - F7188X_GPIO_BANK(0, 8, 0xF0), - F7188X_GPIO_BANK(10, 8, 0xE0), - F7188X_GPIO_BANK(20, 8, 0xD0), - F7188X_GPIO_BANK(30, 8, 0xC0), - F7188X_GPIO_BANK(40, 8, 0xB0), - F7188X_GPIO_BANK(50, 8, 0xA0), - F7188X_GPIO_BANK(60, 5, 0x90), + F7188X_GPIO_BANK(0, 8, 0xF0, DRVNAME "-0"), + F7188X_GPIO_BANK(10, 8, 0xE0, DRVNAME "-1"), + F7188X_GPIO_BANK(20, 8, 0xD0, DRVNAME "-2"), + F7188X_GPIO_BANK(30, 8, 0xC0, DRVNAME "-3"), + F7188X_GPIO_BANK(40, 8, 0xB0, DRVNAME "-4"), + F7188X_GPIO_BANK(50, 8, 0xA0, DRVNAME "-5"), + F7188X_GPIO_BANK(60, 5, 0x90, DRVNAME "-6"), }; static struct f7188x_gpio_bank nct6116d_gpio_bank[] = { - F7188X_GPIO_BANK(0, 8, 0xE0), - F7188X_GPIO_BANK(10, 8, 0xE4), - F7188X_GPIO_BANK(20, 8, 0xE8), - F7188X_GPIO_BANK(30, 8, 0xEC), - F7188X_GPIO_BANK(40, 8, 0xF0), - F7188X_GPIO_BANK(50, 8, 0xF4), - F7188X_GPIO_BANK(60, 8, 0xF8), - F7188X_GPIO_BANK(70, 1, 0xFC), + F7188X_GPIO_BANK(0, 8, 0xE0, DRVNAME "-0"), + F7188X_GPIO_BANK(10, 8, 0xE4, DRVNAME "-1"), + F7188X_GPIO_BANK(20, 8, 0xE8, DRVNAME "-2"), + F7188X_GPIO_BANK(30, 8, 0xEC, DRVNAME "-3"), + F7188X_GPIO_BANK(40, 8, 0xF0, DRVNAME "-4"), + F7188X_GPIO_BANK(50, 8, 0xF4, DRVNAME "-5"), + F7188X_GPIO_BANK(60, 8, 0xF8, DRVNAME "-6"), + F7188X_GPIO_BANK(70, 1, 0xFC, DRVNAME "-7"), }; static int f7188x_gpio_get_direction(struct gpio_chip *chip, unsigned offset) -- 2.35.1 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v4 3/5] leds: simatic-ipc-leds-gpio: add new model 227G 2022-08-23 10:23 [PATCH v4 0/5] add support for another simatic board Henning Schild 2022-08-23 10:23 ` [PATCH v4 1/5] gpio-f7188x: Add GPIO support for Nuvoton NCT6116 Henning Schild 2022-08-23 10:23 ` [PATCH v4 2/5] gpio-f7188x: use unique labels for banks/chips Henning Schild @ 2022-08-23 10:23 ` Henning Schild 2022-08-23 10:23 ` [PATCH v4 4/5] platform/x86: simatic-ipc: enable watchdog for 227G Henning Schild ` (2 subsequent siblings) 5 siblings, 0 replies; 16+ messages in thread From: Henning Schild @ 2022-08-23 10:23 UTC (permalink / raw) To: Linus Walleij, Bartosz Golaszewski, Pavel Machek, Hans de Goede, Mark Gross, Andy Shevchenko, Lee Jones, linux-gpio, linux-kernel, linux-leds, platform-driver-x86 Cc: Sheng-Yuan Huang, Tasanakorn Phaipool, simon.guinot, Henning Schild This adds support of the Siemens Simatic IPC227G. Its LEDs are connected to GPIO pins provided by the gpio-f7188x module. We make sure that gets loaded, if not enabled in the kernel config no LED support will be available. Reviewed-by: Hans de Goede <hdegoede@redhat.com> Signed-off-by: Henning Schild <henning.schild@siemens.com> --- drivers/leds/simple/simatic-ipc-leds-gpio.c | 42 ++++++++++++++++--- drivers/platform/x86/simatic-ipc.c | 4 +- .../platform_data/x86/simatic-ipc-base.h | 1 + include/linux/platform_data/x86/simatic-ipc.h | 1 + 4 files changed, 42 insertions(+), 6 deletions(-) diff --git a/drivers/leds/simple/simatic-ipc-leds-gpio.c b/drivers/leds/simple/simatic-ipc-leds-gpio.c index 4c9e663a90ba..0d73dcbeec2d 100644 --- a/drivers/leds/simple/simatic-ipc-leds-gpio.c +++ b/drivers/leds/simple/simatic-ipc-leds-gpio.c @@ -13,28 +13,45 @@ #include <linux/leds.h> #include <linux/module.h> #include <linux/platform_device.h> +#include <linux/platform_data/x86/simatic-ipc-base.h> -static struct gpiod_lookup_table simatic_ipc_led_gpio_table = { +struct gpiod_lookup_table *simatic_ipc_led_gpio_table; + +static struct gpiod_lookup_table simatic_ipc_led_gpio_table_127e = { .dev_id = "leds-gpio", .table = { - GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 51, NULL, 0, GPIO_ACTIVE_LOW), GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 52, NULL, 1, GPIO_ACTIVE_LOW), GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 53, NULL, 2, GPIO_ACTIVE_LOW), GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 57, NULL, 3, GPIO_ACTIVE_LOW), GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 58, NULL, 4, GPIO_ACTIVE_LOW), GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 60, NULL, 5, GPIO_ACTIVE_LOW), + GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 51, NULL, 0, GPIO_ACTIVE_LOW), GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 56, NULL, 6, GPIO_ACTIVE_LOW), GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 59, NULL, 7, GPIO_ACTIVE_HIGH), }, }; +static struct gpiod_lookup_table simatic_ipc_led_gpio_table_227g = { + .dev_id = "leds-gpio", + .table = { + GPIO_LOOKUP_IDX("gpio-f7188x-2", 0, NULL, 0, GPIO_ACTIVE_LOW), + GPIO_LOOKUP_IDX("gpio-f7188x-2", 1, NULL, 1, GPIO_ACTIVE_LOW), + GPIO_LOOKUP_IDX("gpio-f7188x-2", 2, NULL, 2, GPIO_ACTIVE_LOW), + GPIO_LOOKUP_IDX("gpio-f7188x-2", 3, NULL, 3, GPIO_ACTIVE_LOW), + GPIO_LOOKUP_IDX("gpio-f7188x-2", 4, NULL, 4, GPIO_ACTIVE_LOW), + GPIO_LOOKUP_IDX("gpio-f7188x-2", 5, NULL, 5, GPIO_ACTIVE_LOW), + GPIO_LOOKUP_IDX("gpio-f7188x-3", 6, NULL, 6, GPIO_ACTIVE_HIGH), + GPIO_LOOKUP_IDX("gpio-f7188x-3", 7, NULL, 7, GPIO_ACTIVE_HIGH), + } +}; + static const struct gpio_led simatic_ipc_gpio_leds[] = { - { .name = "green:" LED_FUNCTION_STATUS "-3" }, { .name = "red:" LED_FUNCTION_STATUS "-1" }, { .name = "green:" LED_FUNCTION_STATUS "-1" }, { .name = "red:" LED_FUNCTION_STATUS "-2" }, { .name = "green:" LED_FUNCTION_STATUS "-2" }, { .name = "red:" LED_FUNCTION_STATUS "-3" }, + { .name = "green:" LED_FUNCTION_STATUS "-3" }, }; static const struct gpio_led_platform_data simatic_ipc_gpio_leds_pdata = { @@ -46,7 +63,7 @@ static struct platform_device *simatic_leds_pdev; static int simatic_ipc_leds_gpio_remove(struct platform_device *pdev) { - gpiod_remove_lookup_table(&simatic_ipc_led_gpio_table); + gpiod_remove_lookup_table(simatic_ipc_led_gpio_table); platform_device_unregister(simatic_leds_pdev); return 0; @@ -54,10 +71,25 @@ static int simatic_ipc_leds_gpio_remove(struct platform_device *pdev) static int simatic_ipc_leds_gpio_probe(struct platform_device *pdev) { + const struct simatic_ipc_platform *plat = pdev->dev.platform_data; struct gpio_desc *gpiod; int err; - gpiod_add_lookup_table(&simatic_ipc_led_gpio_table); + switch (plat->devmode) { + case SIMATIC_IPC_DEVICE_127E: + simatic_ipc_led_gpio_table = &simatic_ipc_led_gpio_table_127e; + break; + case SIMATIC_IPC_DEVICE_227G: + if (!IS_ENABLED(CONFIG_GPIO_F7188X)) + return -ENODEV; + request_module("gpio-f7188x"); + simatic_ipc_led_gpio_table = &simatic_ipc_led_gpio_table_227g; + break; + default: + return -ENODEV; + } + + gpiod_add_lookup_table(simatic_ipc_led_gpio_table); simatic_leds_pdev = platform_device_register_resndata(NULL, "leds-gpio", PLATFORM_DEVID_NONE, NULL, 0, &simatic_ipc_gpio_leds_pdata, diff --git a/drivers/platform/x86/simatic-ipc.c b/drivers/platform/x86/simatic-ipc.c index ca3647b751d5..1825ef21a86d 100644 --- a/drivers/platform/x86/simatic-ipc.c +++ b/drivers/platform/x86/simatic-ipc.c @@ -41,6 +41,7 @@ static struct { {SIMATIC_IPC_IPC127E, SIMATIC_IPC_DEVICE_127E, SIMATIC_IPC_DEVICE_NONE}, {SIMATIC_IPC_IPC227D, SIMATIC_IPC_DEVICE_227D, SIMATIC_IPC_DEVICE_NONE}, {SIMATIC_IPC_IPC227E, SIMATIC_IPC_DEVICE_427E, SIMATIC_IPC_DEVICE_227E}, + {SIMATIC_IPC_IPC227G, SIMATIC_IPC_DEVICE_227G, SIMATIC_IPC_DEVICE_NONE}, {SIMATIC_IPC_IPC277E, SIMATIC_IPC_DEVICE_NONE, SIMATIC_IPC_DEVICE_227E}, {SIMATIC_IPC_IPC427D, SIMATIC_IPC_DEVICE_427E, SIMATIC_IPC_DEVICE_NONE}, {SIMATIC_IPC_IPC427E, SIMATIC_IPC_DEVICE_427E, SIMATIC_IPC_DEVICE_427E}, @@ -65,7 +66,8 @@ static int register_platform_devices(u32 station_id) } if (ledmode != SIMATIC_IPC_DEVICE_NONE) { - if (ledmode == SIMATIC_IPC_DEVICE_127E) + if (ledmode == SIMATIC_IPC_DEVICE_127E || + ledmode == SIMATIC_IPC_DEVICE_227G) pdevname = KBUILD_MODNAME "_leds_gpio"; platform_data.devmode = ledmode; ipc_led_platform_device = diff --git a/include/linux/platform_data/x86/simatic-ipc-base.h b/include/linux/platform_data/x86/simatic-ipc-base.h index 39fefd48cf4d..57d6a10dfc9e 100644 --- a/include/linux/platform_data/x86/simatic-ipc-base.h +++ b/include/linux/platform_data/x86/simatic-ipc-base.h @@ -19,6 +19,7 @@ #define SIMATIC_IPC_DEVICE_427E 2 #define SIMATIC_IPC_DEVICE_127E 3 #define SIMATIC_IPC_DEVICE_227E 4 +#define SIMATIC_IPC_DEVICE_227G 5 struct simatic_ipc_platform { u8 devmode; diff --git a/include/linux/platform_data/x86/simatic-ipc.h b/include/linux/platform_data/x86/simatic-ipc.h index f3b76b39776b..7a2e79f3be0b 100644 --- a/include/linux/platform_data/x86/simatic-ipc.h +++ b/include/linux/platform_data/x86/simatic-ipc.h @@ -31,6 +31,7 @@ enum simatic_ipc_station_ids { SIMATIC_IPC_IPC427E = 0x00000A01, SIMATIC_IPC_IPC477E = 0x00000A02, SIMATIC_IPC_IPC127E = 0x00000D01, + SIMATIC_IPC_IPC227G = 0x00000F01, }; static inline u32 simatic_ipc_get_station_id(u8 *data, int max_len) -- 2.35.1 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v4 4/5] platform/x86: simatic-ipc: enable watchdog for 227G 2022-08-23 10:23 [PATCH v4 0/5] add support for another simatic board Henning Schild ` (2 preceding siblings ...) 2022-08-23 10:23 ` [PATCH v4 3/5] leds: simatic-ipc-leds-gpio: add new model 227G Henning Schild @ 2022-08-23 10:23 ` Henning Schild 2022-08-23 10:23 ` [PATCH v4 5/5] platform/x86: simatic-ipc: add new model 427G Henning Schild 2022-08-23 14:49 ` [PATCH v4 0/5] add support for another simatic board Andy Shevchenko 5 siblings, 0 replies; 16+ messages in thread From: Henning Schild @ 2022-08-23 10:23 UTC (permalink / raw) To: Linus Walleij, Bartosz Golaszewski, Pavel Machek, Hans de Goede, Mark Gross, Andy Shevchenko, Lee Jones, linux-gpio, linux-kernel, linux-leds, platform-driver-x86 Cc: Sheng-Yuan Huang, Tasanakorn Phaipool, simon.guinot, Henning Schild Just load the watchdog module, after having identified that machine. That watchdog module does not have any autoloading support. Signed-off-by: Henning Schild <henning.schild@siemens.com> --- drivers/platform/x86/simatic-ipc.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/platform/x86/simatic-ipc.c b/drivers/platform/x86/simatic-ipc.c index 1825ef21a86d..8dd686d1c9f1 100644 --- a/drivers/platform/x86/simatic-ipc.c +++ b/drivers/platform/x86/simatic-ipc.c @@ -96,6 +96,9 @@ static int register_platform_devices(u32 station_id) ipc_wdt_platform_device->name); } + if (station_id == SIMATIC_IPC_IPC227G) + request_module("w83627hf_wdt"); + if (ledmode == SIMATIC_IPC_DEVICE_NONE && wdtmode == SIMATIC_IPC_DEVICE_NONE) { pr_warn("unsupported IPC detected, station id=%08x\n", -- 2.35.1 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v4 5/5] platform/x86: simatic-ipc: add new model 427G 2022-08-23 10:23 [PATCH v4 0/5] add support for another simatic board Henning Schild ` (3 preceding siblings ...) 2022-08-23 10:23 ` [PATCH v4 4/5] platform/x86: simatic-ipc: enable watchdog for 227G Henning Schild @ 2022-08-23 10:23 ` Henning Schild 2022-08-23 14:49 ` [PATCH v4 0/5] add support for another simatic board Andy Shevchenko 5 siblings, 0 replies; 16+ messages in thread From: Henning Schild @ 2022-08-23 10:23 UTC (permalink / raw) To: Linus Walleij, Bartosz Golaszewski, Pavel Machek, Hans de Goede, Mark Gross, Andy Shevchenko, Lee Jones, linux-gpio, linux-kernel, linux-leds, platform-driver-x86 Cc: Sheng-Yuan Huang, Tasanakorn Phaipool, simon.guinot, Henning Schild This adds support for the Siemens Simatic IPC427G. A board which basically works like the 227G added in a previous patch. So all there is to do is to add the station_id and make it take all the 227G branches. Signed-off-by: Henning Schild <henning.schild@siemens.com> --- drivers/platform/x86/simatic-ipc.c | 11 +++++++---- include/linux/platform_data/x86/simatic-ipc.h | 1 + 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/drivers/platform/x86/simatic-ipc.c b/drivers/platform/x86/simatic-ipc.c index 8dd686d1c9f1..ca76076fc706 100644 --- a/drivers/platform/x86/simatic-ipc.c +++ b/drivers/platform/x86/simatic-ipc.c @@ -41,11 +41,12 @@ static struct { {SIMATIC_IPC_IPC127E, SIMATIC_IPC_DEVICE_127E, SIMATIC_IPC_DEVICE_NONE}, {SIMATIC_IPC_IPC227D, SIMATIC_IPC_DEVICE_227D, SIMATIC_IPC_DEVICE_NONE}, {SIMATIC_IPC_IPC227E, SIMATIC_IPC_DEVICE_427E, SIMATIC_IPC_DEVICE_227E}, - {SIMATIC_IPC_IPC227G, SIMATIC_IPC_DEVICE_227G, SIMATIC_IPC_DEVICE_NONE}, + {SIMATIC_IPC_IPC227G, SIMATIC_IPC_DEVICE_227G, SIMATIC_IPC_DEVICE_227G}, {SIMATIC_IPC_IPC277E, SIMATIC_IPC_DEVICE_NONE, SIMATIC_IPC_DEVICE_227E}, {SIMATIC_IPC_IPC427D, SIMATIC_IPC_DEVICE_427E, SIMATIC_IPC_DEVICE_NONE}, {SIMATIC_IPC_IPC427E, SIMATIC_IPC_DEVICE_427E, SIMATIC_IPC_DEVICE_427E}, {SIMATIC_IPC_IPC477E, SIMATIC_IPC_DEVICE_NONE, SIMATIC_IPC_DEVICE_427E}, + {SIMATIC_IPC_IPC427G, SIMATIC_IPC_DEVICE_227G, SIMATIC_IPC_DEVICE_227G}, }; static int register_platform_devices(u32 station_id) @@ -82,6 +83,11 @@ static int register_platform_devices(u32 station_id) ipc_led_platform_device->name); } + if (wdtmode == SIMATIC_IPC_DEVICE_227G) { + request_module("w83627hf_wdt"); + return 0; + } + if (wdtmode != SIMATIC_IPC_DEVICE_NONE) { platform_data.devmode = wdtmode; ipc_wdt_platform_device = @@ -96,9 +102,6 @@ static int register_platform_devices(u32 station_id) ipc_wdt_platform_device->name); } - if (station_id == SIMATIC_IPC_IPC227G) - request_module("w83627hf_wdt"); - if (ledmode == SIMATIC_IPC_DEVICE_NONE && wdtmode == SIMATIC_IPC_DEVICE_NONE) { pr_warn("unsupported IPC detected, station id=%08x\n", diff --git a/include/linux/platform_data/x86/simatic-ipc.h b/include/linux/platform_data/x86/simatic-ipc.h index 7a2e79f3be0b..632320ec8f08 100644 --- a/include/linux/platform_data/x86/simatic-ipc.h +++ b/include/linux/platform_data/x86/simatic-ipc.h @@ -32,6 +32,7 @@ enum simatic_ipc_station_ids { SIMATIC_IPC_IPC477E = 0x00000A02, SIMATIC_IPC_IPC127E = 0x00000D01, SIMATIC_IPC_IPC227G = 0x00000F01, + SIMATIC_IPC_IPC427G = 0x00001001, }; static inline u32 simatic_ipc_get_station_id(u8 *data, int max_len) -- 2.35.1 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v4 0/5] add support for another simatic board 2022-08-23 10:23 [PATCH v4 0/5] add support for another simatic board Henning Schild ` (4 preceding siblings ...) 2022-08-23 10:23 ` [PATCH v4 5/5] platform/x86: simatic-ipc: add new model 427G Henning Schild @ 2022-08-23 14:49 ` Andy Shevchenko 5 siblings, 0 replies; 16+ messages in thread From: Andy Shevchenko @ 2022-08-23 14:49 UTC (permalink / raw) To: Henning Schild Cc: Linus Walleij, Bartosz Golaszewski, Pavel Machek, Hans de Goede, Mark Gross, Lee Jones, linux-gpio, linux-kernel, linux-leds, platform-driver-x86, Sheng-Yuan Huang, Tasanakorn Phaipool, simon.guinot On Tue, Aug 23, 2022 at 12:23:39PM +0200, Henning Schild wrote: > changes since v3: > - update Kconfig as well > - drop chip names from comment in driver header > - add manufacturer check for Fintek again, Nuvoton not possible > - drop revision printing for Nuvoton > - restructure defines again > - add new model 427G > > changes since v2: (p1 only) > - rename macros that change behavior > - use chip type not device id in the macros > - reorder defines a bit > > changes since v1: > - remove unused define > - fix bug where (base + 2) was used as second data bit > - add macros for "inverted" and "single data bit" For the non-commented patches: Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.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" No need, it's upstream (v6.0-rc1 onwards). > Henning Schild (5): > 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 > platform/x86: simatic-ipc: add new model 427G > > drivers/gpio/Kconfig | 3 +- > drivers/gpio/gpio-f7188x.c | 229 +++++++++++------- > drivers/leds/simple/simatic-ipc-leds-gpio.c | 42 +++- > drivers/platform/x86/simatic-ipc.c | 10 +- > .../platform_data/x86/simatic-ipc-base.h | 1 + > include/linux/platform_data/x86/simatic-ipc.h | 2 + > 6 files changed, 194 insertions(+), 93 deletions(-) > > -- > 2.35.1 > -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2022-08-26 13:30 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-08-23 10:23 [PATCH v4 0/5] add support for another simatic board Henning Schild 2022-08-23 10:23 ` [PATCH v4 1/5] gpio-f7188x: Add GPIO support for Nuvoton NCT6116 Henning Schild 2022-08-23 14:47 ` Andy Shevchenko 2022-08-23 14:54 ` Henning Schild 2022-08-24 13:10 ` simon.guinot 2022-08-24 13:50 ` Henning Schild 2022-08-24 13:54 ` Hans de Goede 2022-08-24 14:17 ` Henning Schild 2022-08-24 14:24 ` Hans de Goede 2022-08-24 16:02 ` simon.guinot 2022-08-26 13:30 ` Linus Walleij 2022-08-23 10:23 ` [PATCH v4 2/5] gpio-f7188x: use unique labels for banks/chips Henning Schild 2022-08-23 10:23 ` [PATCH v4 3/5] leds: simatic-ipc-leds-gpio: add new model 227G Henning Schild 2022-08-23 10:23 ` [PATCH v4 4/5] platform/x86: simatic-ipc: enable watchdog for 227G Henning Schild 2022-08-23 10:23 ` [PATCH v4 5/5] platform/x86: simatic-ipc: add new model 427G Henning Schild 2022-08-23 14:49 ` [PATCH v4 0/5] add support for another simatic board Andy Shevchenko
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).