* [PATCH 0/1] add device driver for Nuvoton SIO gpio function @ 2022-07-01 9:14 Henning Schild 2022-07-01 9:14 ` [PATCH 1/1] gpio: nct6116d: add new driver for several Nuvoton super io chips Henning Schild 0 siblings, 1 reply; 5+ messages in thread From: Henning Schild @ 2022-07-01 9:14 UTC (permalink / raw) To: linux-kernel, linux-gpio, Bartosz Golaszewski, Linus Walleij, Tasanakorn Phaipool Cc: Sheng-Yuan Huang, Kuan-Wei Ho, Henning Schild This adds gpio support for several Super IO chips from Nuvoton. The driver was originally developed by Nuvoton and i am just contributing it on behalf, because other patches i will send later will require access to the gpios. The driver is valid on its own. The driver supports several chips, of which i only managed to test one but did not want to drop the others. I hope the original authors will help with the testing and addressing review feedback. The changes i did so far mainly are inspired by similar drivers and some just concern coding style. If more has to be done and the original authors do not jump in, we might start off with just that one chip i can test and add the others later on. Henning Schild (1): gpio: nct6116d: add new driver for several Nuvoton super io chips drivers/gpio/Kconfig | 9 + drivers/gpio/Makefile | 1 + drivers/gpio/gpio-nct6116d.c | 401 +++++++++++++++++++++++++++++++++++ 3 files changed, 411 insertions(+) create mode 100644 drivers/gpio/gpio-nct6116d.c -- 2.35.1 ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH 1/1] gpio: nct6116d: add new driver for several Nuvoton super io chips 2022-07-01 9:14 [PATCH 0/1] add device driver for Nuvoton SIO gpio function Henning Schild @ 2022-07-01 9:14 ` Henning Schild 2022-07-01 10:45 ` Andy Shevchenko 0 siblings, 1 reply; 5+ messages in thread From: Henning Schild @ 2022-07-01 9:14 UTC (permalink / raw) To: linux-kernel, linux-gpio, Bartosz Golaszewski, Linus Walleij, Tasanakorn Phaipool Cc: Sheng-Yuan Huang, Kuan-Wei Ho, Henning Schild This patch adds gpio support for several Nuvoton NCTXXX chips. These super io chips offer multiple functions of which several already have drivers in the kernel, i.e. hwmon and wdt. Signed-off-by: Henning Schild <henning.schild@siemens.com> --- drivers/gpio/Kconfig | 9 + drivers/gpio/Makefile | 1 + drivers/gpio/gpio-nct6116d.c | 401 +++++++++++++++++++++++++++++++++++ 3 files changed, 411 insertions(+) create mode 100644 drivers/gpio/gpio-nct6116d.c diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig index b01961999ced..40f1494b1adc 100644 --- a/drivers/gpio/Kconfig +++ b/drivers/gpio/Kconfig @@ -457,6 +457,15 @@ config GPIO_MXS select GPIO_GENERIC select GENERIC_IRQ_CHIP +config GPIO_NCT6116D + tristate "Nuvoton Super-I/O GPIO support" + help + This option enables support for GPIOs found on Nuvoton Super-I/O + chips NCT5104D, NCT6106D, NCT6116D, NCT6122D. + + To compile this driver as a module, choose M here: the module will + be called gpio-nct6116d. + config GPIO_OCTEON tristate "Cavium OCTEON GPIO" depends on CAVIUM_OCTEON_SOC diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile index 14352f6dfe8e..87f1b0a0cda2 100644 --- a/drivers/gpio/Makefile +++ b/drivers/gpio/Makefile @@ -107,6 +107,7 @@ obj-$(CONFIG_GPIO_MT7621) += gpio-mt7621.o obj-$(CONFIG_GPIO_MVEBU) += gpio-mvebu.o obj-$(CONFIG_GPIO_MXC) += gpio-mxc.o obj-$(CONFIG_GPIO_MXS) += gpio-mxs.o +obj-$(CONFIG_GPIO_NCT6116D) += gpio-nct6116d.o obj-$(CONFIG_GPIO_OCTEON) += gpio-octeon.o obj-$(CONFIG_GPIO_OMAP) += gpio-omap.o obj-$(CONFIG_GPIO_PALMAS) += gpio-palmas.o diff --git a/drivers/gpio/gpio-nct6116d.c b/drivers/gpio/gpio-nct6116d.c new file mode 100644 index 000000000000..bef2121da991 --- /dev/null +++ b/drivers/gpio/gpio-nct6116d.c @@ -0,0 +1,401 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * GPIO driver for Nuvoton Super-I/O chips NCT5104D, NCT6106D, NCT6116D, NCT6122D + * + * Authors: + * Tasanakorn Phaipool <tasanakorn@gmail.com> + * Sheng-Yuan Huang <syhuang3@nuvoton.com> + * Kuan-Wei Ho <cwho@nuvoton.com> + * Henning Schild <henning.schild@siemens.com> + */ + +#include <linux/module.h> +#include <linux/init.h> +#include <linux/platform_device.h> +#include <linux/io.h> +#include <linux/gpio/driver.h> + +#define DRVNAME "gpio-nct6116d" + +/* + * Super-I/O registers + */ +#define SIO_LDSEL 0x07 /* Logical device select */ +#define SIO_CHIPID 0x20 /* Chaip ID (2 bytes) */ +#define SIO_GPIO_ENABLE 0x30 /* GPIO enable */ + +#define SIO_LD_GPIO 0x07 /* 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_ID_MASK 0xFFF0 +#define SIO_NCT5104D_ID 0x1061 +#define SIO_NCT6106D_ID 0xC452 +#define SIO_NCT6116D_ID 0xD282 +#define SIO_NCT6122D_ID 0xD2A3 + +enum chips { + nct5104d, + nct6106d, + nct6116d, + nct6122d, +}; + +static const char * const nct6116d_names[] = { + "nct5104d", + "nct6106d", + "nct6116d", + "nct6122d", +}; + +struct nct6116d_sio { + int addr; + enum chips type; +}; + +struct nct6116d_gpio_bank { + struct gpio_chip chip; + unsigned int regbase; + struct nct6116d_gpio_data *data; +}; + +struct nct6116d_gpio_data { + struct nct6116d_sio *sio; + int nr_bank; + struct nct6116d_gpio_bank *bank; +}; + +/* + * Super-I/O functions. + */ + +static inline int superio_inb(int base, int reg) +{ + outb(reg, base); + return inb(base + 1); +} + +static int superio_inw(int base, int reg) +{ + int val; + + outb(reg++, base); + val = inb(base + 1) << 8; + outb(reg, base); + val |= inb(base + 1); + + return val; +} + +static inline void superio_outb(int base, int reg, int val) +{ + outb(reg, base); + outb(val, base + 1); +} + +static inline int superio_enter(int base) +{ + /* Don't step on other drivers' I/O space by accident. */ + if (!request_muxed_region(base, 2, DRVNAME)) { + pr_err(DRVNAME "I/O address 0x%04x already in use\n", base); + return -EBUSY; + } + + /* According to the datasheet the key must be send twice. */ + outb(SIO_UNLOCK_KEY, base); + outb(SIO_UNLOCK_KEY, base); + + return 0; +} + +static inline void superio_select(int base, int ld) +{ + outb(SIO_LDSEL, base); + outb(ld, base + 1); +} + +static inline void superio_exit(int base) +{ + outb(SIO_LOCK_KEY, base); + release_region(base, 2); +} + +/* + * GPIO chip. + */ + +static int nct6116d_gpio_direction_in(struct gpio_chip *chip, unsigned int offset); +static int nct6116d_gpio_get(struct gpio_chip *chip, unsigned int offset); +static int nct6116d_gpio_direction_out(struct gpio_chip *chip, + unsigned int offset, int value); +static void nct6116d_gpio_set(struct gpio_chip *chip, unsigned int offset, int value); + +#define NCT6116D_GPIO_BANK(_base, _ngpio, _regbase, _label) \ + { \ + .chip = { \ + .label = _label, \ + .owner = THIS_MODULE, \ + .direction_input = nct6116d_gpio_direction_in, \ + .get = nct6116d_gpio_get, \ + .direction_output = nct6116d_gpio_direction_out, \ + .set = nct6116d_gpio_set, \ + .base = _base, \ + .ngpio = _ngpio, \ + .can_sleep = false, \ + }, \ + .regbase = _regbase, \ + } + +#define gpio_dir(base) ((base) + 0) +#define gpio_data(base) ((base) + 1) + +static struct nct6116d_gpio_bank nct6116d_gpio_bank[] = { + NCT6116D_GPIO_BANK(0, 8, 0xE0, DRVNAME "-0"), + NCT6116D_GPIO_BANK(10, 8, 0xE4, DRVNAME "-1"), + NCT6116D_GPIO_BANK(20, 8, 0xE8, DRVNAME "-2"), + NCT6116D_GPIO_BANK(30, 8, 0xEC, DRVNAME "-3"), + NCT6116D_GPIO_BANK(40, 8, 0xF0, DRVNAME "-4"), +}; + +static int nct6116d_gpio_direction_in(struct gpio_chip *chip, unsigned int offset) +{ + int err; + struct nct6116d_gpio_bank *bank = + container_of(chip, struct nct6116d_gpio_bank, chip); + struct nct6116d_sio *sio = bank->data->sio; + u8 dir; + + err = superio_enter(sio->addr); + if (err) + return err; + superio_select(sio->addr, SIO_LD_GPIO); + + dir = superio_inb(sio->addr, gpio_dir(bank->regbase)); + dir |= BIT(offset); + superio_outb(sio->addr, gpio_dir(bank->regbase), dir); + + superio_exit(sio->addr); + + return 0; +} + +static int nct6116d_gpio_get(struct gpio_chip *chip, unsigned int offset) +{ + int err; + struct nct6116d_gpio_bank *bank = + container_of(chip, struct nct6116d_gpio_bank, chip); + struct nct6116d_sio *sio = bank->data->sio; + u8 data; + + err = superio_enter(sio->addr); + if (err) + return err; + superio_select(sio->addr, SIO_LD_GPIO); + + data = superio_inb(sio->addr, gpio_data(bank->regbase)); + + superio_exit(sio->addr); + + return !!(data & BIT(offset)); +} + +static int nct6116d_gpio_direction_out(struct gpio_chip *chip, + unsigned int offset, int value) +{ + int err; + struct nct6116d_gpio_bank *bank = + container_of(chip, struct nct6116d_gpio_bank, chip); + struct nct6116d_sio *sio = bank->data->sio; + u8 dir, data_out; + + err = superio_enter(sio->addr); + if (err) + return err; + superio_select(sio->addr, SIO_LD_GPIO); + + data_out = superio_inb(sio->addr, gpio_data(bank->regbase)); + if (value) + data_out |= BIT(offset); + else + data_out &= ~BIT(offset); + superio_outb(sio->addr, gpio_data(bank->regbase), data_out); + + dir = superio_inb(sio->addr, gpio_dir(bank->regbase)); + dir &= ~BIT(offset); + superio_outb(sio->addr, gpio_dir(bank->regbase), dir); + + superio_exit(sio->addr); + + return 0; +} + +static void nct6116d_gpio_set(struct gpio_chip *chip, unsigned int offset, int value) +{ + int err; + struct nct6116d_gpio_bank *bank = + container_of(chip, struct nct6116d_gpio_bank, chip); + struct nct6116d_sio *sio = bank->data->sio; + u8 data_out; + + err = superio_enter(sio->addr); + if (err) + return; + superio_select(sio->addr, SIO_LD_GPIO); + + data_out = superio_inb(sio->addr, gpio_data(bank->regbase)); + if (value) + data_out |= BIT(offset); + else + data_out &= ~BIT(offset); + superio_outb(sio->addr, gpio_data(bank->regbase), data_out); + + superio_exit(sio->addr); +} + +/* + * Platform device and driver. + */ + +static int nct6116d_gpio_probe(struct platform_device *pdev) +{ + int err; + int i; + struct nct6116d_sio *sio = pdev->dev.platform_data; + struct nct6116d_gpio_data *data; + + data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL); + if (!data) + return -ENOMEM; + + data->nr_bank = ARRAY_SIZE(nct6116d_gpio_bank); + data->bank = nct6116d_gpio_bank; + data->sio = sio; + + platform_set_drvdata(pdev, data); + + /* For each GPIO bank, register a GPIO chip. */ + for (i = 0; i < data->nr_bank; i++) { + struct nct6116d_gpio_bank *bank = &data->bank[i]; + + bank->chip.parent = &pdev->dev; + bank->data = data; + + err = devm_gpiochip_add_data(&pdev->dev, &bank->chip, bank); + if (err) { + dev_err(&pdev->dev, + "Failed to register gpiochip %d: %d\n", + i, err); + return err; + } + } + + return 0; +} + +static int __init nct6116d_find(int addr, struct nct6116d_sio *sio) +{ + int err; + u16 devid; + + err = superio_enter(addr); + if (err) + return err; + + devid = superio_inw(addr, SIO_CHIPID); + switch (devid & SIO_ID_MASK) { + case SIO_NCT5104D_ID & SIO_ID_MASK: + sio->type = nct5104d; + break; + case SIO_NCT6106D_ID & SIO_ID_MASK: + sio->type = nct6106d; + break; + case SIO_NCT6116D_ID & SIO_ID_MASK: + sio->type = nct6116d; + break; + case SIO_NCT6122D_ID & SIO_ID_MASK: + sio->type = nct6122d; + break; + default: + pr_info(DRVNAME ": Unsupported device 0x%04x\n", devid); + superio_exit(addr); + return -ENODEV; + } + sio->addr = addr; + + pr_info(DRVNAME ": Found %s at %#x chip id 0x%04x\n", + nct6116d_names[sio->type], + (unsigned int)addr, + devid); + + superio_exit(addr); + return 0; +} + +static struct platform_device *nct6116d_gpio_pdev; + +static int __init +nct6116d_gpio_device_add(const struct nct6116d_sio *sio) +{ + int err; + + nct6116d_gpio_pdev = platform_device_alloc(DRVNAME, -1); + if (!nct6116d_gpio_pdev) + return -ENOMEM; + + err = platform_device_add_data(nct6116d_gpio_pdev, sio, sizeof(*sio)); + if (err) { + pr_err(DRVNAME "Platform data allocation failed\n"); + goto err; + } + + err = platform_device_add(nct6116d_gpio_pdev); + if (err) { + pr_err(DRVNAME "Device addition failed\n"); + goto err; + } + + return 0; + +err: + platform_device_put(nct6116d_gpio_pdev); + + return err; +} + +static struct platform_driver nct6116d_gpio_driver = { + .driver = { + .name = DRVNAME, + }, + .probe = nct6116d_gpio_probe, +}; + +static int __init nct6116d_gpio_init(void) +{ + int err; + struct nct6116d_sio sio; + + if (nct6116d_find(0x2e, &sio) && + nct6116d_find(0x4e, &sio)) + return -ENODEV; + + err = platform_driver_register(&nct6116d_gpio_driver); + if (!err) { + err = nct6116d_gpio_device_add(&sio); + if (err) + platform_driver_unregister(&nct6116d_gpio_driver); + } + + return err; +} +subsys_initcall(nct6116d_gpio_init); + +static void __exit nct6116d_gpio_exit(void) +{ + platform_device_unregister(nct6116d_gpio_pdev); + platform_driver_unregister(&nct6116d_gpio_driver); +} +module_exit(nct6116d_gpio_exit); + +MODULE_DESCRIPTION("GPIO driver for Nuvoton Super-I/O chips NCT5104D, NCT6106D, NCT6116D, NCT6122D"); +MODULE_AUTHOR("Tasanakorn Phaipool <tasanakorn@gmail.com>"); +MODULE_LICENSE("GPL"); -- 2.35.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 1/1] gpio: nct6116d: add new driver for several Nuvoton super io chips 2022-07-01 9:14 ` [PATCH 1/1] gpio: nct6116d: add new driver for several Nuvoton super io chips Henning Schild @ 2022-07-01 10:45 ` Andy Shevchenko 2022-07-01 11:12 ` Henning Schild 2022-07-04 13:08 ` Henning Schild 0 siblings, 2 replies; 5+ messages in thread From: Andy Shevchenko @ 2022-07-01 10:45 UTC (permalink / raw) To: Henning Schild Cc: Linux Kernel Mailing List, open list:GPIO SUBSYSTEM, Bartosz Golaszewski, Linus Walleij, Tasanakorn Phaipool, Sheng-Yuan Huang, Kuan-Wei Ho On Fri, Jul 1, 2022 at 11:15 AM Henning Schild <henning.schild@siemens.com> wrote: > > This patch adds gpio support for several Nuvoton NCTXXX chips. These super > io chips offer multiple functions of which several already have drivers in Super-I/O (to be consistent with the help in Kconfig, etc). > the kernel, i.e. hwmon and wdt. watchdog ... Since you are talking about authorship in the cover letter, is it possible to get the original authorship to be preserved in the commit and authors / co-developers giving their SoB tags? ... > +#include <linux/module.h> > +#include <linux/init.h> > +#include <linux/platform_device.h> > +#include <linux/io.h> > +#include <linux/gpio/driver.h> Keep it sorted? ... > +#define SIO_ID_MASK 0xFFF0 GENMASK() ? ... > +enum chips { > + nct5104d, > + nct6106d, > + nct6116d, > + nct6122d, > +}; > + > +static const char * const nct6116d_names[] = { > + "nct5104d", > + "nct6106d", > + "nct6116d", > + "nct6122d", It would be slightly more flexible to use enum values as indices here: [nct5104d] = "nct5104d", > +}; ... > + pr_err(DRVNAME "I/O address 0x%04x already in use\n", base); Why not use pr_fmt() properly and drop DRVNAME here and in other pr_*(), if any? ... > +static int nct6116d_gpio_direction_in(struct gpio_chip *chip, unsigned int offset); > +static int nct6116d_gpio_get(struct gpio_chip *chip, unsigned int offset); > +static int nct6116d_gpio_direction_out(struct gpio_chip *chip, > + unsigned int offset, int value); > +static void nct6116d_gpio_set(struct gpio_chip *chip, unsigned int offset, int value); Is it possible to avoid forward declarations? ... > +#define NCT6116D_GPIO_BANK(_base, _ngpio, _regbase, _label) \ > + { \ > + .chip = { \ > + .label = _label, \ > + .owner = THIS_MODULE, \ > + .direction_input = nct6116d_gpio_direction_in, \ > + .get = nct6116d_gpio_get, \ > + .direction_output = nct6116d_gpio_direction_out, \ > + .set = nct6116d_gpio_set, \ .get_direction ? > + .base = _base, \ > + .ngpio = _ngpio, \ > + .can_sleep = false, \ > + }, \ > + .regbase = _regbase, \ > + } ... > + int err; > + struct nct6116d_gpio_bank *bank = > + container_of(chip, struct nct6116d_gpio_bank, chip); Can it be transformed to macro or inliner and then struct nct6116d_gpio_bank *bank = to_nct6116d_gpio_bank(chip); > + struct nct6116d_sio *sio = bank->data->sio; > + u8 dir; Here and everywhere else, perhaps keep the reversed xmas tree order? ... > + err = devm_gpiochip_add_data(&pdev->dev, &bank->chip, bank); > + if (err) { > + dev_err(&pdev->dev, > + "Failed to register gpiochip %d: %d\n", > + i, err); > + return err; return dev_err_probe(...); ... > + pr_info(DRVNAME ": Found %s at %#x chip id 0x%04x\n", > + nct6116d_names[sio->type], > + (unsigned int)addr, Casting in printf() very often means a wrong specifier in use. > + devid); ... > + nct6116d_gpio_pdev = platform_device_alloc(DRVNAME, -1); > + if (!nct6116d_gpio_pdev) > + return -ENOMEM; > + > + err = platform_device_add_data(nct6116d_gpio_pdev, sio, sizeof(*sio)); > + if (err) { > + pr_err(DRVNAME "Platform data allocation failed\n"); > + goto err; > + } > + > + err = platform_device_add(nct6116d_gpio_pdev); > + if (err) { > + pr_err(DRVNAME "Device addition failed\n"); > + goto err; > + } Wonder, don't we have some shortcuts for these? Perhaps platform_device_register_full()? -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/1] gpio: nct6116d: add new driver for several Nuvoton super io chips 2022-07-01 10:45 ` Andy Shevchenko @ 2022-07-01 11:12 ` Henning Schild 2022-07-04 13:08 ` Henning Schild 1 sibling, 0 replies; 5+ messages in thread From: Henning Schild @ 2022-07-01 11:12 UTC (permalink / raw) To: Andy Shevchenko Cc: Linux Kernel Mailing List, open list:GPIO SUBSYSTEM, Bartosz Golaszewski, Linus Walleij, Tasanakorn Phaipool, Sheng-Yuan Huang, Kuan-Wei Ho Am Fri, 1 Jul 2022 12:45:58 +0200 schrieb Andy Shevchenko <andy.shevchenko@gmail.com>: > On Fri, Jul 1, 2022 at 11:15 AM Henning Schild > <henning.schild@siemens.com> wrote: > > > > This patch adds gpio support for several Nuvoton NCTXXX chips. > > These super io chips offer multiple functions of which several > > already have drivers in > > Super-I/O (to be consistent with the help in Kconfig, etc). > > > > the kernel, i.e. hwmon and wdt. > > watchdog > > ... > > Since you are talking about authorship in the cover letter, is it > possible to get the original authorship to be preserved in the commit > and authors / co-developers giving their SoB tags? In fact i am afraid the original authors might stay silent. I do not want to claim to be the author, i am just the one who polished for mainline. And i do want to give the original authors the credit. But i will not spoof git author or add SoBs, unless i get explicit permission ... which might not happen. A lot of the findings in the code below are actually what looks like copied code from drivers/gpio/gpio-f7188x.c and drivers/gpio/gpio-winbond.c but this being a new driver i could apply the improvements here. I would have to look over the proposed changes, would not want to touch too much because i can only test one of the 4 chips. regards, Henning > ... > > > +#include <linux/module.h> > > +#include <linux/init.h> > > +#include <linux/platform_device.h> > > +#include <linux/io.h> > > +#include <linux/gpio/driver.h> > > Keep it sorted? > > ... > > > +#define SIO_ID_MASK 0xFFF0 > > GENMASK() ? > > ... > > > +enum chips { > > + nct5104d, > > + nct6106d, > > + nct6116d, > > + nct6122d, > > +}; > > + > > +static const char * const nct6116d_names[] = { > > + "nct5104d", > > + "nct6106d", > > + "nct6116d", > > + "nct6122d", > > It would be slightly more flexible to use enum values as indices here: > > [nct5104d] = "nct5104d", > > > +}; > > ... > > > + pr_err(DRVNAME "I/O address 0x%04x already in > > use\n", base); > > Why not use pr_fmt() properly and drop DRVNAME here and in other > pr_*(), if any? > > ... > > > +static int nct6116d_gpio_direction_in(struct gpio_chip *chip, > > unsigned int offset); +static int nct6116d_gpio_get(struct > > gpio_chip *chip, unsigned int offset); +static int > > nct6116d_gpio_direction_out(struct gpio_chip *chip, > > + unsigned int offset, int > > value); +static void nct6116d_gpio_set(struct gpio_chip *chip, > > unsigned int offset, int value); > > Is it possible to avoid forward declarations? > > ... > > > +#define NCT6116D_GPIO_BANK(_base, _ngpio, _regbase, _label) > > \ > > + { > > \ > > + .chip = { > > \ > > + .label = _label, > > \ > > + .owner = THIS_MODULE, > > \ > > + .direction_input = > > nct6116d_gpio_direction_in, \ > > + .get = nct6116d_gpio_get, > > \ > > + .direction_output = > > nct6116d_gpio_direction_out, \ > > + .set = nct6116d_gpio_set, > > \ > > .get_direction ? > > > + .base = _base, > > \ > > + .ngpio = _ngpio, > > \ > > + .can_sleep = false, > > \ > > + }, > > \ > > + .regbase = _regbase, > > \ > > + } > > ... > > > + int err; > > + struct nct6116d_gpio_bank *bank = > > + container_of(chip, struct nct6116d_gpio_bank, > > chip); > > Can it be transformed to macro or inliner and then > > struct nct6116d_gpio_bank *bank = to_nct6116d_gpio_bank(chip); > > > + struct nct6116d_sio *sio = bank->data->sio; > > + u8 dir; > > Here and everywhere else, perhaps keep the reversed xmas tree order? > > ... > > > + err = devm_gpiochip_add_data(&pdev->dev, > > &bank->chip, bank); > > + if (err) { > > + dev_err(&pdev->dev, > > + "Failed to register gpiochip %d: > > %d\n", > > + i, err); > > + return err; > > return dev_err_probe(...); > > ... > > > + pr_info(DRVNAME ": Found %s at %#x chip id 0x%04x\n", > > + nct6116d_names[sio->type], > > + (unsigned int)addr, > > Casting in printf() very often means a wrong specifier in use. > > > + devid); > > ... > > > + nct6116d_gpio_pdev = platform_device_alloc(DRVNAME, -1); > > + if (!nct6116d_gpio_pdev) > > + return -ENOMEM; > > + > > + err = platform_device_add_data(nct6116d_gpio_pdev, sio, > > sizeof(*sio)); > > + if (err) { > > + pr_err(DRVNAME "Platform data allocation failed\n"); > > + goto err; > > + } > > + > > + err = platform_device_add(nct6116d_gpio_pdev); > > + if (err) { > > + pr_err(DRVNAME "Device addition failed\n"); > > + goto err; > > + } > > Wonder, don't we have some shortcuts for these? Perhaps > platform_device_register_full()? > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/1] gpio: nct6116d: add new driver for several Nuvoton super io chips 2022-07-01 10:45 ` Andy Shevchenko 2022-07-01 11:12 ` Henning Schild @ 2022-07-04 13:08 ` Henning Schild 1 sibling, 0 replies; 5+ messages in thread From: Henning Schild @ 2022-07-04 13:08 UTC (permalink / raw) To: Andy Shevchenko Cc: Linux Kernel Mailing List, open list:GPIO SUBSYSTEM, Bartosz Golaszewski, Linus Walleij, Tasanakorn Phaipool, Sheng-Yuan Huang, Kuan-Wei Ho Am Fri, 1 Jul 2022 12:45:58 +0200 schrieb Andy Shevchenko <andy.shevchenko@gmail.com>: > On Fri, Jul 1, 2022 at 11:15 AM Henning Schild > <henning.schild@siemens.com> wrote: > > > > This patch adds gpio support for several Nuvoton NCTXXX chips. > > These super io chips offer multiple functions of which several > > already have drivers in > > Super-I/O (to be consistent with the help in Kconfig, etc). > > > > the kernel, i.e. hwmon and wdt. > > watchdog > > ... > > Since you are talking about authorship in the cover letter, is it > possible to get the original authorship to be preserved in the commit > and authors / co-developers giving their SoB tags? > > ... > > > +#include <linux/module.h> > > +#include <linux/init.h> > > +#include <linux/platform_device.h> > > +#include <linux/io.h> > > +#include <linux/gpio/driver.h> > > Keep it sorted? > > ... > > > +#define SIO_ID_MASK 0xFFF0 > > GENMASK() ? > > ... > > > +enum chips { > > + nct5104d, > > + nct6106d, > > + nct6116d, > > + nct6122d, > > +}; > > + > > +static const char * const nct6116d_names[] = { > > + "nct5104d", > > + "nct6106d", > > + "nct6116d", > > + "nct6122d", > > It would be slightly more flexible to use enum values as indices here: > > [nct5104d] = "nct5104d", > > > +}; > > ... > > > + pr_err(DRVNAME "I/O address 0x%04x already in > > use\n", base); > > Why not use pr_fmt() properly and drop DRVNAME here and in other > pr_*(), if any? > > ... > > > +static int nct6116d_gpio_direction_in(struct gpio_chip *chip, > > unsigned int offset); +static int nct6116d_gpio_get(struct > > gpio_chip *chip, unsigned int offset); +static int > > nct6116d_gpio_direction_out(struct gpio_chip *chip, > > + unsigned int offset, int > > value); +static void nct6116d_gpio_set(struct gpio_chip *chip, > > unsigned int offset, int value); > > Is it possible to avoid forward declarations? > > ... > > > +#define NCT6116D_GPIO_BANK(_base, _ngpio, _regbase, _label) > > \ > > + { > > \ > > + .chip = { > > \ > > + .label = _label, > > \ > > + .owner = THIS_MODULE, > > \ > > + .direction_input = > > nct6116d_gpio_direction_in, \ > > + .get = nct6116d_gpio_get, > > \ > > + .direction_output = > > nct6116d_gpio_direction_out, \ > > + .set = nct6116d_gpio_set, > > \ > > .get_direction ? > > > + .base = _base, > > \ > > + .ngpio = _ngpio, > > \ > > + .can_sleep = false, > > \ > > + }, > > \ > > + .regbase = _regbase, > > \ > > + } > > ... > > > + int err; > > + struct nct6116d_gpio_bank *bank = > > + container_of(chip, struct nct6116d_gpio_bank, > > chip); > > Can it be transformed to macro or inliner and then > > struct nct6116d_gpio_bank *bank = to_nct6116d_gpio_bank(chip); > > > + struct nct6116d_sio *sio = bank->data->sio; > > + u8 dir; > > Here and everywhere else, perhaps keep the reversed xmas tree order? > > ... > > > + err = devm_gpiochip_add_data(&pdev->dev, > > &bank->chip, bank); > > + if (err) { > > + dev_err(&pdev->dev, > > + "Failed to register gpiochip %d: > > %d\n", > > + i, err); > > + return err; > > return dev_err_probe(...); > > ... > > > + pr_info(DRVNAME ": Found %s at %#x chip id 0x%04x\n", > > + nct6116d_names[sio->type], > > + (unsigned int)addr, > > Casting in printf() very often means a wrong specifier in use. > > > + devid); > > ... > > > + nct6116d_gpio_pdev = platform_device_alloc(DRVNAME, -1); > > + if (!nct6116d_gpio_pdev) > > + return -ENOMEM; > > + > > + err = platform_device_add_data(nct6116d_gpio_pdev, sio, > > sizeof(*sio)); > > + if (err) { > > + pr_err(DRVNAME "Platform data allocation failed\n"); > > + goto err; > > + } > > + > > + err = platform_device_add(nct6116d_gpio_pdev); > > + if (err) { > > + pr_err(DRVNAME "Device addition failed\n"); > > + goto err; > > + } > > Wonder, don't we have some shortcuts for these? Perhaps > platform_device_register_full()? I ended up doing nothing here, code did not really look simpler. For all the other points, except for the SoBs and authorship i did something in v2. Henning ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-07-04 13:09 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-07-01 9:14 [PATCH 0/1] add device driver for Nuvoton SIO gpio function Henning Schild 2022-07-01 9:14 ` [PATCH 1/1] gpio: nct6116d: add new driver for several Nuvoton super io chips Henning Schild 2022-07-01 10:45 ` Andy Shevchenko 2022-07-01 11:12 ` Henning Schild 2022-07-04 13:08 ` Henning Schild
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.