Re: [PATCH v1 2/9] gpio: Add Nuvoton NCT6694 GPIO support

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Sorry, resending this email in plain text format.

Dear Bart,

Thank you for your comments.

Bartosz Golaszewski <brgl@xxxxxxxx> 於 2024年10月24日 週四 下午5:47寫道:
>
> On Thu, Oct 24, 2024 at 10:59 AM Ming Yu <a0282524688@xxxxxxxxx> wrote:
> >
> > This driver supports GPIO and IRQ functionality for NCT6694 MFD
> > device based on USB interface.
> >
> > Signed-off-by: Ming Yu <tmyu0@xxxxxxxxxxx>
> > ---
> >  MAINTAINERS                 |   1 +
> >  drivers/gpio/Kconfig        |  12 +
> >  drivers/gpio/Makefile       |   1 +
> >  drivers/gpio/gpio-nct6694.c | 489 ++++++++++++++++++++++++++++++++++++
> >  4 files changed, 503 insertions(+)
> >  create mode 100644 drivers/gpio/gpio-nct6694.c
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 30157ca95cf3..2c86d5dab3f1 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -16438,6 +16438,7 @@ NUVOTON NCT6694 MFD DRIVER
> >  M:     Ming Yu <tmyu0@xxxxxxxxxxx>
> >  L:     linux-kernel@xxxxxxxxxxxxxxx
> >  S:     Supported
> > +F:     drivers/gpio/gpio-nct6694.c
> >  F:     drivers/mfd/nct6694.c
> >  F:     include/linux/mfd/nct6694.h
> >
> > diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> > index d93cd4f722b4..aa78ad9ff4ac 100644
> > --- a/drivers/gpio/Kconfig
> > +++ b/drivers/gpio/Kconfig
> > @@ -1450,6 +1450,18 @@ config GPIO_MAX77650
> >           GPIO driver for MAX77650/77651 PMIC from Maxim Semiconductor.
> >           These chips have a single pin that can be configured as GPIO.
> >
> > +config GPIO_NCT6694
> > +       tristate "Nuvoton NCT6694 GPIO controller support"
> > +       depends on MFD_NCT6694
> > +       select GENERIC_IRQ_CHIP
> > +       select GPIOLIB_IRQCHIP
> > +       help
> > +         This driver supports 8 GPIO pins per bank that can all be interrupt
> > +         sources.
> > +
> > +         This driver can also be built as a module. If so, the module will be
> > +         called gpio-nct6694.
> > +
> >  config GPIO_PALMAS
> >         bool "TI PALMAS series PMICs GPIO"
> >         depends on MFD_PALMAS
> > diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> > index 1429e8c0229b..02c94aa28017 100644
> > --- a/drivers/gpio/Makefile
> > +++ b/drivers/gpio/Makefile
> > @@ -121,6 +121,7 @@ obj-$(CONFIG_GPIO_MXC)                      += gpio-mxc.o
> >  obj-$(CONFIG_GPIO_MXS)                 += gpio-mxs.o
> >  obj-$(CONFIG_GPIO_NOMADIK)             += gpio-nomadik.o
> >  obj-$(CONFIG_GPIO_NPCM_SGPIO)          += gpio-npcm-sgpio.o
> > +obj-$(CONFIG_GPIO_NCT6694)             += gpio-nct6694.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-nct6694.c b/drivers/gpio/gpio-nct6694.c
> > new file mode 100644
> > index 000000000000..42c0e6e76730
> > --- /dev/null
> > +++ b/drivers/gpio/gpio-nct6694.c
> > @@ -0,0 +1,489 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Nuvoton NCT6694 GPIO controller driver based on USB interface.
> > + *
> > + * Copyright (C) 2024 Nuvoton Technology Corp.
> > + */
> > +
> > +#include <linux/gpio.h>
>
> Don't include this header. It's documented as obsolete.

[Ming] Okay! I'll drop it in the next patch.

>
> > +#include <linux/gpio/driver.h>
> > +#include <linux/module.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/mfd/core.h>
> > +#include <linux/mfd/nct6694.h>
> > +
>
> You only use it once, drop it.

[Ming] That line is blank, did you mean #include <linux/gpio.h>?

>
> > +#define DRVNAME "nct6694-gpio"
> > +
> > +/* Host interface */
> > +#define REQUEST_GPIO_MOD               0xFF
> > +#define REQUEST_GPIO_LEN               0x01
> > +
> > +/* Report Channel */
> > +#define GPIO_VER_REG                   0x90
> > +#define GPIO_VALID_REG                 0x110
> > +#define GPI_DATA_REG                   0x120
> > +#define GPO_DIR_REG                    0x170
> > +#define GPO_TYPE_REG                   0x180
> > +#define GPO_DATA_REG                   0x190
> > +
> > +#define GPI_STS_REG                    0x130
> > +#define GPI_CLR_REG                    0x140
> > +#define GPI_FALLING_REG                        0x150
> > +#define GPI_RISING_REG                 0x160
> > +
>
> Please use the NCT6694 prefix for these defines, otherwise it's not
> clear whether they come from the driver or from GPIO core.
>
> []

[Ming] Okay! I'll add the prefix to the defines in the next patch.

>
> > +
> > +static const char * const nct6694_gpio_name[] = {
> > +       "NCT6694-GPIO0",
> > +       "NCT6694-GPIO1",
> > +       "NCT6694-GPIO2",
> > +       "NCT6694-GPIO3",
> > +       "NCT6694-GPIO4",
> > +       "NCT6694-GPIO5",
> > +       "NCT6694-GPIO6",
> > +       "NCT6694-GPIO7",
> > +       "NCT6694-GPIO8",
> > +       "NCT6694-GPIO9",
> > +       "NCT6694-GPIOA",
> > +       "NCT6694-GPIOB",
> > +       "NCT6694-GPIOC",
> > +       "NCT6694-GPIOD",
> > +       "NCT6694-GPIOE",
> > +       "NCT6694-GPIOF",
> > +};
>
> This looks like it corresponds with the MFD cells and makes me wonder:
> am I getting that wrong or do you want to register 0xf GPIO chips? Or
> a single GPIO chip with 0xf lines? What is the topology?

[Ming] Yes, it corresponds to the MFD cells.
I would like to register 16 GPIO chips, each with 8 lines.
The chip has 128 pins totally, the core can check if the pin is valid through
the init_valid_mask() callback.

>
> > +
> > +static int nct6694_gpio_probe(struct platform_device *pdev)
> > +{
> > +       const struct mfd_cell *cell = mfd_get_cell(pdev);
> > +       struct nct6694 *nct6694 = dev_get_drvdata(pdev->dev.parent);
> > +       struct nct6694_gpio_data *data;
> > +       struct gpio_irq_chip *girq;
> > +       int ret;
> > +
> > +       data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
> > +       if (!data)
> > +               return -ENOMEM;
> > +
> > +       data->nct6694 = nct6694;
> > +       data->group = cell->id;
> > +
> > +       data->gpio.label                = nct6694_gpio_name[cell->id];
> > +       data->gpio.direction_input      = nct6694_direction_input;
> > +       data->gpio.get                  = nct6694_get_value;
> > +       data->gpio.direction_output     = nct6694_direction_output;
> > +       data->gpio.set                  = nct6694_set_value;
> > +       data->gpio.get_direction        = nct6694_get_direction;
> > +       data->gpio.set_config           = nct6694_set_config;
> > +       data->gpio.init_valid_mask      = nct6694_init_valid_mask;
> > +       data->gpio.base                 = -1;
> > +       data->gpio.can_sleep            = false;
> > +       data->gpio.owner                = THIS_MODULE;
> > +       data->gpio.ngpio                = 8;
> > +
> > +       INIT_WORK(&data->irq_work, nct6694_irq);
> > +       INIT_WORK(&data->irq_trig_work, nct6694_irq_trig);
> > +       mutex_init(&data->irq_lock);
> > +
> > +       ret = nct6694_register_handler(nct6694, GPIO_IRQ_STATUS,
> > +                                      nct6694_gpio_handler, data);
> > +       if (ret) {
> > +               dev_err(&pdev->dev, "%s:  Failed to register handler: %pe\n",
> > +                       __func__, ERR_PTR(ret));
> > +               return ret;
> > +       }
> > +
> > +       platform_set_drvdata(pdev, data);
> > +
> > +       ret = nct6694_get_irq_trig(data);
> > +       if (ret)
> > +               return ret;
> > +
> > +       /* Register gpio chip to GPIO framework */
> > +       girq = &data->gpio.irq;
> > +       gpio_irq_chip_set_chip(girq, &nct6694_irq_chip);
> > +       girq->parent_handler = NULL;
> > +       girq->num_parents = 0;
> > +       girq->parents = NULL;
> > +       girq->default_type = IRQ_TYPE_NONE;
> > +       girq->handler = handle_level_irq;
> > +       girq->threaded = true;
> > +
> > +       ret = gpiochip_add_data(&data->gpio, data);
> > +       if (ret) {
> > +               dev_err(&pdev->dev, "%s: Failed to register GPIO chip: %pe",
> > +                       __func__, ERR_PTR(ret));
> > +               return ret;
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> > +static void nct6694_gpio_remove(struct platform_device *pdev)
> > +{
> > +       struct nct6694_gpio_data *data = platform_get_drvdata(pdev);
> > +
> > +       gpiochip_remove(&data->gpio);
>
> This should be dropped in favor of using devm_gpiochip_add_data().
> Especially since you probably want to cancel the irq_work before
> removing the chip.

[Ming] Okay! I'll change it in the next patch.

>
> > +       cancel_work(&data->irq_work);
> > +       cancel_work(&data->irq_trig_work);
> > +}
> > +
> > +static struct platform_driver nct6694_gpio_driver = {
> > +       .driver = {
> > +               .name   = DRVNAME,
> > +       },
> > +       .probe          = nct6694_gpio_probe,
> > +       .remove         = nct6694_gpio_remove,
> > +};
> > +
> > +static int __init nct6694_init(void)
> > +{
> > +       int err;
> > +
> > +       err = platform_driver_register(&nct6694_gpio_driver);
> > +       if (!err) {
> > +               if (err)
>
> If err is equal to 0, check if it's not equal to zero?
>
> > +                       platform_driver_unregister(&nct6694_gpio_driver);
>
> If platform_driver_register() failed, then the device was never registered.
>
> > +       }
> > +
> > +       return err;
> > +}
> > +subsys_initcall(nct6694_init);
>
> Any reason why this must be initialized earlier? It's a USB driver after all.

[Ming] For platform driver registration, I'll change it to
module_platform_driver()
in the next patch.

>
> > +
> > +static void __exit nct6694_exit(void)
> > +{
> > +       platform_driver_unregister(&nct6694_gpio_driver);
> > +}
> > +module_exit(nct6694_exit);
> > +
> > +MODULE_DESCRIPTION("USB-GPIO controller driver for NCT6694");
> > +MODULE_AUTHOR("Ming Yu <tmyu0@xxxxxxxxxxx>");
> > +MODULE_LICENSE("GPL");
> > --
> > 2.34.1
> >
>
> Bart





[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [eCos]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux