On Fri, Oct 25, 2024 at 9:39 AM 游子民 <a0282524688@xxxxxxxxx> wrote:
>
> 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"
I meant this line. Just put the driver name in the driver struct
definition directly.
> > > +
> > > +/* 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.
>
Ok, that's fine but the GPIO chip names should be in the MFD driver
only, it doesn't make sense to have them here. It's the MFD core that
will register the GPIO platform devices.
No for line names - as this is a dynamic USB expander, I'd suggest to
have them in the driver and assign to gc->names.
> >
> > > +
> > > +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.
>
Thanks,
Bartosz
> >
> > > +
> > > +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
