| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAOoeyxXRrDuKJRMb3O2h3BF1vC=pwNN3DKfnEN9LnA+jiBCTQg@mail.gmail.com>
Date: Mon, 28 Oct 2024 16:56:51 +0800
From: Ming Yu <a0282524688@...il.com>
To: Bartosz Golaszewski <brgl@...ev.pl>
Cc: tmyu0@...oton.com, lee@...nel.org, linus.walleij@...aro.org,
andi.shyti@...nel.org, mkl@...gutronix.de, mailhol.vincent@...adoo.fr,
andrew+netdev@...n.ch, davem@...emloft.net, edumazet@...gle.com,
kuba@...nel.org, pabeni@...hat.com, wim@...ux-watchdog.org,
linux@...ck-us.net, jdelvare@...e.com, jic23@...nel.org, lars@...afoo.de,
ukleinek@...nel.org, alexandre.belloni@...tlin.com,
linux-kernel@...r.kernel.org, linux-gpio@...r.kernel.org,
linux-i2c@...r.kernel.org, linux-can@...r.kernel.org, netdev@...r.kernel.org,
linux-watchdog@...r.kernel.org, linux-hwmon@...r.kernel.org,
linux-iio@...r.kernel.org, linux-pwm@...r.kernel.org,
linux-rtc@...r.kernel.org
Subject: Re: [PATCH v1 2/9] gpio: Add Nuvoton NCT6694 GPIO support
Dear Bartosz,
Thank you for your comments,
Bartosz Golaszewski <brgl@...ev.pl> 於 2024年10月25日 週五 下午3:47寫道：
>
> On Fri, Oct 25, 2024 at 9:39 AM 游子民 <a0282524688@...il.com> wrote:
> >
> > Sorry, resending this email in plain text format.
> >
> > Dear Bart,
> >
> > Thank you for your comments.
> >
> > Bartosz Golaszewski <brgl@...ev.pl> 於 2024年10月24日 週四 下午5:47寫道：
> > >
> > > On Thu, Oct 24, 2024 at 10:59 AM Ming Yu <a0282524688@...il.com> wrote:
> > > >
> > > > This driver supports GPIO and IRQ functionality for NCT6694 MFD
> > > > device based on USB interface.
> > > >
> > > > Signed-off-by: Ming Yu <tmyu0@...oton.com>
> > > > ---
> > > > 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@...oton.com>
> > > > L: linux-kernel@...r.kernel.org
> > > > 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.
I understand. I will remove it.
>
> 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.
Could I create an array to map each of the GPIO pins?
>
> > >
> > > > +
> > > > +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@...oton.com>");
> > > > +MODULE_LICENSE("GPL");
> > > > --
> > > > 2.34.1
> > > >
> > >
> > > Bart
Best regards,
Ming
Powered by blists - more mailing lists