| 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: <CAMRc=McWhREAapUO1C26c7MxFhwsq9=HH7Z8BZabwbB1L5UoBw@mail.gmail.com>
Date: Sat, 13 Dec 2025 19:54:22 -0500
From: Bartosz Golaszewski <brgl@...nel.org>
To: Ramiro Oliveira <ramiro.oliveira@...antech.com>
Cc: linux-kernel@...r.kernel.org, linux-gpio@...r.kernel.org,
linux-hwmon@...r.kernel.org, linux-i2c@...r.kernel.org,
dri-devel@...ts.freedesktop.org, linux-fbdev@...r.kernel.org,
linux-watchdog@...r.kernel.org, linux-pm@...r.kernel.org,
Wenkai Chung <wenkai.chung@...antech.com.tw>,
Francisco Aragon-Trivino <francisco.aragon-trivino@...antech.com>,
Hongzhi Wang <hongzhi.wang@...antech.com>,
Mikhail Tsukerman <mikhail.tsukerman@...antech.com>,
Thomas Kastner <thomas.kastner@...antech.com>, Lee Jones <lee@...nel.org>,
Linus Walleij <linusw@...nel.org>, Bartosz Golaszewski <brgl@...nel.org>, Guenter Roeck <linux@...ck-us.net>,
Andi Shyti <andi.shyti@...nel.org>, Daniel Thompson <danielt@...nel.org>,
Jingoo Han <jingoohan1@...il.com>, Helge Deller <deller@....de>,
Wim Van Sebroeck <wim@...ux-watchdog.org>, "Rafael J. Wysocki" <rafael@...nel.org>,
Daniel Lezcano <daniel.lezcano@...aro.org>, Zhang Rui <rui.zhang@...el.com>,
Lukasz Luba <lukasz.luba@....com>
Subject: Re: [PATCH 2/8] Add Advantech EIO GPIO driver
On Fri, 12 Dec 2025 17:40:53 +0100, Ramiro Oliveira
<ramiro.oliveira@...antech.com> said:
> This driver controls the GPIO component of the Advantech EIO chip.
>
> Signed-off-by: Ramiro Oliveira <ramiro.oliveira@...antech.com>
> ---
> MAINTAINERS | 1 +
> drivers/gpio/Kconfig | 6 ++
> drivers/gpio/Makefile | 1 +
> drivers/gpio/gpio-eio.c | 273 ++++++++++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 281 insertions(+)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index bd9279796c2f..359d4a13f212 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -619,6 +619,7 @@ F: drivers/platform/x86/adv_swbutton.c
> ADVANTECH EIO DRIVER
> M: Ramiro Oliveira <ramiro.oliveira@...antech.com>
> S: Maintained
> +F: drivers/gpio/gpio-eio.c
Instead of churning MAINTAINERS in every patch of the series, I suggest you
add a separate patch adding the full entry at the end.
> F: drivers/mfd/eio_core.c
> F: include/linux/mfd/eio.h
>
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index bd185482a7fd..628a914842bd 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -277,6 +277,12 @@ config GPIO_DWAPB
> Say Y or M here to build support for the Synopsys DesignWare APB
> GPIO block.
>
> +config GPIO_EIO
> + tristate "Advantech EIO GPIO"
> + depends on MFD_EIO
> + help
> + Say Y or M to build support for Advantech EIO GPIO block.
> +
> config GPIO_EIC_SPRD
> tristate "Spreadtrum EIC support"
> depends on ARCH_SPRD || COMPILE_TEST
> diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> index 2421a8fd3733..ba3883d5e4a0 100644
> --- a/drivers/gpio/Makefile
> +++ b/drivers/gpio/Makefile
> @@ -64,6 +64,7 @@ obj-$(CONFIG_GPIO_DLN2) += gpio-dln2.o
> obj-$(CONFIG_GPIO_DS4520) += gpio-ds4520.o
> obj-$(CONFIG_GPIO_DWAPB) += gpio-dwapb.o
> obj-$(CONFIG_GPIO_EIC_SPRD) += gpio-eic-sprd.o
> +obj-$(CONFIG_GPIO_EIO) += gpio-eio.o
> obj-$(CONFIG_GPIO_ELKHARTLAKE) += gpio-elkhartlake.o
> obj-$(CONFIG_GPIO_EM) += gpio-em.o
> obj-$(CONFIG_GPIO_EN7523) += gpio-en7523.o
> diff --git a/drivers/gpio/gpio-eio.c b/drivers/gpio/gpio-eio.c
> new file mode 100644
> index 000000000000..50f66a325e8f
> --- /dev/null
> +++ b/drivers/gpio/gpio-eio.c
> @@ -0,0 +1,273 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * GPIO driver for Advantech EIO Embedded controller.
> + *
> + * Copyright (C) 2025 Advantech Corporation. All rights reserved.
> + */
> +
> +#include <linux/errno.h>
> +#include <linux/gpio.h>
Don't include this, it's a legacy header as per one of the first lines in this
file.
> +#include <linux/gpio/driver.h>
> +#include <linux/mfd/core.h>
> +#include <linux/mfd/eio.h>
> +#include <linux/module.h>
> +
> +#define GPIO_MAX_PINS 48
> +#define GPIO_WRITE 0x18
> +#define GPIO_READ 0x19
> +
> +struct eio_gpio_dev {
> + u64 avail;
> + int max;
> + struct gpio_chip chip;
> + struct device *dev;
> +};
> +
> +struct {
> + int size;
> + bool write;
> +} ctrl_para[] = {
> + { 0x01, false }, { 0x00, false }, { 0x00, false }, { 0x02, false },
> + { 0x01, false }, { 0x00, false }, { 0x00, false }, { 0x00, false },
> + { 0x00, false }, { 0x00, false }, { 0x00, false }, { 0x00, false },
> + { 0x00, false }, { 0x00, false }, { 0x00, false }, { 0x00, false },
> + { 0x01, true }, { 0x01, true }, { 0x02, true }, { 0x02, true },
> + { 0x02, false }, { 0x10, false }
> +};
This should be static.
> +
> +enum {
> + GPIO_STATUS = 0,
> + GPIO_GROUP_AVAIL = 3,
> + GPIO_ERROR = 0x04,
> + GPIO_PIN_DIR = 0x10,
> + GPIO_PIN_LEVEL = 0x11,
> + GPIO_GROUP_DIR = 0x12,
> + GPIO_GROUP_LEVEL = 0x13,
> + GPIO_MAPPING = 0x14,
> + GPIO_NAME = 0x15
> +} gpio_ctrl;
Do enum gpio_ctrl {. But also use a common prefix for all symbols in
this driver.
> +
> +struct {
> + int group;
> + int port;
> +} group_map[] = {
> + { 0, 0 }, { 0, 1 },
> + { 1, 0 }, { 1, 1 },
> + { 2, 0 }, { 2, 1 },
> + { 3, 0 }, { 3, 1 },
> + { 3, 2 }, { 3, 3 },
> + { 3, 4 }, { 3, 5 },
> + { 3, 6 }, { 3, 7 }
> +};
> +
> +static int timeout;
> +module_param(timeout, int, 0444);
> +MODULE_PARM_DESC(timeout, "Set PMC command timeout value.\n");
> +
> +static int pmc_write(struct device *mfd_dev, u8 ctrl, u8 dev_id, void *data)
> +{
> + struct pmc_op op = {
> + .cmd = GPIO_WRITE,
> + .control = ctrl,
> + .device_id = dev_id,
> + .payload = (u8 *)data,
> + .timeout = timeout,
> + };
> +
> + if (ctrl > ARRAY_SIZE(ctrl_para))
> + return -ENOMEM;
> +
> + if (!ctrl_para[ctrl].write)
> + return -EINVAL;
> +
> + op.size = ctrl_para[ctrl].size;
> +
> + return eio_core_pmc_operation(mfd_dev, &op);
> +}
> +
> +static int pmc_read(struct device *mfd_dev, u8 ctrl, u8 dev_id, void *data)
> +{
> + struct pmc_op op = {
> + .cmd = GPIO_READ,
> + .control = ctrl,
> + .device_id = dev_id,
> + .payload = (u8 *)data,
> + .timeout = timeout,
> + };
> +
> + if (ctrl > ARRAY_SIZE(ctrl_para))
> + return -ENOMEM;
> +
> + op.size = ctrl_para[ctrl].size;
> +
> + return eio_core_pmc_operation(mfd_dev, &op);
> +}
> +
> +static int get_dir(struct gpio_chip *chip, unsigned int offset)
> +{
> + u8 dir;
> + int ret;
> +
> + ret = pmc_read(chip->parent, GPIO_PIN_DIR, offset, &dir);
> + if (ret)
> + return ret;
> +
> + return dir ? 0 : 1;
> +}
> +
> +static int dir_input(struct gpio_chip *chip, unsigned int offset)
> +{
> + u8 dir = 0;
> +
> + return pmc_write(chip->parent, GPIO_PIN_DIR, offset, &dir);
> +}
> +
> +static int dir_output(struct gpio_chip *chip, unsigned int offset, int value)
> +{
> + u8 dir = 1;
> + u8 val = value;
> +
> + pmc_write(chip->parent, GPIO_PIN_DIR, offset, &dir);
> +
> + return pmc_write(chip->parent, GPIO_PIN_LEVEL, offset, &val);
> +}
> +
> +static int gpio_get(struct gpio_chip *chip, unsigned int offset)
> +{
> + u8 level;
> + int ret;
> +
> + ret = pmc_read(chip->parent, GPIO_PIN_LEVEL, offset, &level);
> + if (ret)
> + return ret;
> +
> + return level;
> +}
> +
> +static int gpio_set(struct gpio_chip *chip, unsigned int offset, int value)
> +{
> + u8 val = value;
> +
> + pmc_write(chip->parent, GPIO_PIN_LEVEL, offset, &val);
> +
> + return 0;
> +}
> +
> +static int check_support(struct device *dev)
> +{
> + u8 data;
> + int ret;
> +
> + ret = pmc_read(dev, GPIO_STATUS, 0, &data);
> + if (!ret)
> + return ret;
> +
> + if ((data & 0x01) == 0)
> + return -EOPNOTSUPP;
> +
> + return 0;
> +}
> +
> +static int check_pin(struct device *dev, int pin)
> +{
> + int ret;
> + int group, bit;
> + u16 data;
> +
> + /* Get pin mapping */
> + ret = pmc_read(dev, GPIO_MAPPING, pin, &data);
> + if (ret)
> + return ret;
> +
> + if ((data & 0xFF) > ARRAY_SIZE(group_map))
> + return -EINVAL;
> +
> + group = group_map[data & 0xFF].group;
> + bit = data >> 8;
> +
> + /* Check mapped pin */
> + ret = pmc_read(dev, GPIO_GROUP_AVAIL, group, &data);
> + if (ret)
> + return ret;
> +
> + return data & BIT(bit) ? 0 : -EOPNOTSUPP;
> +}
> +
> +static int gpio_init(struct device *mfd, struct eio_gpio_dev *eio_gpio)
> +{
> + int ret;
> + int i;
Keep these on the same line.
> + char str[GPIO_MAX_PINS + 1];
> +
> + memset(str, 0x30, sizeof(str));
> +
> + ret = check_support(mfd);
> + if (ret) {
> + dev_err(eio_gpio->dev, "GPIO not supported (%d)\n", ret);
return dev_err_probe()
> + return ret;
> + }
> +
> + eio_gpio->avail = 0;
> +
> + for (i = 0 ; i < GPIO_MAX_PINS ; i++) {
> + ret = check_pin(mfd, i);
> + if (ret)
> + continue;
> +
> + eio_gpio->avail |= BIT(i);
> + eio_gpio->max = i + 1;
> + str[GPIO_MAX_PINS - i] = '1';
> + }
> +
> + dev_info(eio_gpio->dev, "GPIO pins=%s\n", str);
> +
No need to print anything here.
> + return eio_gpio->max ? 0 : -EOPNOTSUPP;
> +}
> +
> +static const struct gpio_chip eio_gpio_chip = {
> + .label = KBUILD_MODNAME,
> + .owner = THIS_MODULE,
> + .direction_input = dir_input,
> + .get = gpio_get,
> + .direction_output = dir_output,
> + .set = gpio_set,
> + .get_direction = get_dir,
> + .base = -1,
> + .can_sleep = true,
> +};
Instead of having an unnecessary copy of the chip in .rodata, just use compound
literals when initiating it in probe().
> +
> +static int gpio_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct eio_gpio_dev *eio_gpio;
> + struct eio_dev *eio_dev = dev_get_drvdata(dev->parent);
> +
> + if (!eio_dev) {
> + dev_err(dev, "Error contact eio_core\n");
> + return -ENODEV;
> + }
return dev_err_probe()
> +
> + eio_gpio = devm_kzalloc(dev, sizeof(*eio_gpio), GFP_KERNEL);
This can fail, please check the return value.
> + eio_gpio->dev = dev;
> +
> + if (gpio_init(dev->parent, eio_gpio))
> + return -EIO;
> +
> + eio_gpio->chip = eio_gpio_chip;
Don't use tabs like that please. Just stick to single spaces.
> + eio_gpio->chip.parent = dev->parent;
> + eio_gpio->chip.ngpio = eio_gpio->max;
> +
> + return devm_gpiochip_add_data(dev, &eio_gpio->chip, eio_gpio);
> +}
> +
> +static struct platform_driver gpio_driver = {
> + .probe = gpio_probe,
> + .driver = { .name = KBUILD_MODNAME, },
> +};
> +
> +module_platform_driver(gpio_driver);
> +
> +MODULE_AUTHOR("Wenkai Chung <wenkai.chung@...antech.com.tw>");
> +MODULE_AUTHOR("Ramiro Oliveira <ramiro.oliveira@...antech.com>");
> +MODULE_DESCRIPTION("GPIO driver for Advantech EIO embedded controller");
> +MODULE_LICENSE("GPL");
>
> --
> 2.43.0
>
>
Powered by blists - more mailing lists