Re: [PATCH 2/3] gpio: Add Intel USBIO GPIO driver

Sakari Ailus
2025-08-11T07:07:58Z

Hi Hans, Israel,

On Sat, Aug 09, 2025 at 12:23:25PM +0200, Hans de Goede wrote:
> From: Israel Cepeda <israel.a.cepeda.lopez@intel.com>
>
> Add a a driver for the GPIO auxbus child device of the Intel USBIO USB
> IO-expander used by the MIPI cameras on various new (Meteor Lake and
> later) Intel laptops.
>
> Co-developed-by: Hans de Goede <hansg@kernel.org>
> Signed-off-by: Hans de Goede <hansg@kernel.org>
> Signed-off-by: Israel Cepeda <israel.a.cepeda.lopez@intel.com>
> ---
> MAINTAINERS | 1 +
> drivers/gpio/Kconfig | 11 ++
> drivers/gpio/Makefile | 1 +
> drivers/gpio/gpio-usbio.c | 258 ++++++++++++++++++++++++++++++++++++++
> 4 files changed, 271 insertions(+)
> create mode 100644 drivers/gpio/gpio-usbio.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 1208efe41f9f..81db1457e9d1 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -12522,6 +12522,7 @@ INTEL USBIO USB I/O EXPANDER DRIVERS
> M: Israel Cepeda <israel.a.cepeda.lopez@intel.com>
> M: Hans de Goede <hansg@kernel.org>
> S: Maintained
> +F: drivers/gpio/gpio-usbio.c
> F: drivers/usb/misc/usbio.c
> F: include/linux/usb/usbio.h
>
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index 44f922e10db2..f3f7b3b782a3 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -1436,6 +1436,17 @@ config GPIO_LJCA
> This driver can also be built as a module. If so, the module
> will be called gpio-ljca.
>
> +config GPIO_USBIO
> + tristate "Intel USBIO GPIO support"
> + depends on USB_USBIO
> + default USB_USBIO
> + help
> + Select this option to enable GPIO driver for the INTEL
> + USBIO driver stack.
> +
> + This driver can also be built as a module. If so, the module
> + will be called gpio_usbio.
> +
> config GPIO_LP3943
> tristate "TI/National Semiconductor LP3943 GPIO expander"
> depends on MFD_LP3943
> diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> index 88dedd298256..a2c054ea2d4c 100644
> --- a/drivers/gpio/Makefile
> +++ b/drivers/gpio/Makefile
> @@ -90,6 +90,7 @@ obj-$(CONFIG_GPIO_JANZ_TTL) += gpio-janz-ttl.o
> obj-$(CONFIG_GPIO_KEMPLD) += gpio-kempld.o
> obj-$(CONFIG_GPIO_LATCH) += gpio-latch.o
> obj-$(CONFIG_GPIO_LJCA) += gpio-ljca.o
> +obj-$(CONFIG_GPIO_USBIO) += gpio-usbio.o
> obj-$(CONFIG_GPIO_LOGICVC) += gpio-logicvc.o
> obj-$(CONFIG_GPIO_LOONGSON1) += gpio-loongson1.o
> obj-$(CONFIG_GPIO_LOONGSON) += gpio-loongson.o
> diff --git a/drivers/gpio/gpio-usbio.c b/drivers/gpio/gpio-usbio.c
> new file mode 100644
> index 000000000000..08a1219153f4
> --- /dev/null
> +++ b/drivers/gpio/gpio-usbio.c
> @@ -0,0 +1,258 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2025 Intel Corporation.
> + * Copyright (c) 2025 Red Hat, Inc.
> + */
> +
> +#include <linux/acpi.h>
> +#include <linux/auxiliary_bus.h>
> +#include <linux/dev_printk.h>
> +#include <linux/device.h>
> +#include <linux/gpio/driver.h>
> +#include <linux/types.h>
> +#include <linux/usb/usbio.h>
> +
> +struct usbio_gpio_bank {
> + u8 config[USBIO_GPIOSPERBANK];
> + u32 bitmap;
> +};
> +
> +struct usbio_gpio {
> + struct usbio_gpio_bank banks[USBIO_MAX_GPIOBANKS];
> + struct gpio_chip gc;
> + struct auxiliary_device *adev;
> +};
> +
> +static const struct acpi_device_id usbio_gpio_acpi_hids[] = {
> + { "INTC1007" }, /* MTL */
> + { "INTC10B2" }, /* ARL */
> + { "INTC10B5" }, /* LNL */
> + { "INTC10E2" }, /* PTL */
> + { }
> +};
> +
> +static bool usbio_gpio_get_bank_and_pin(struct gpio_chip *gc, unsigned int offset,
> + struct usbio_gpio_bank **bank_ret, int *pin_ret)
> +{
> + struct usbio_gpio *gpio = gpiochip_get_data(gc);
> + struct device *dev = &gpio->adev->dev;
> + struct usbio_gpio_bank *bank;
> + int pin;

unsigned int pin and for pin_ret?

> +
> + if (offset >= gc->ngpio)
> + return false;
> +
> + bank = &gpio->banks[offset / USBIO_GPIOSPERBANK];
> + pin = offset % USBIO_GPIOSPERBANK;
> + if (~bank->bitmap & BIT(pin)) {
> + /* The FW bitmap sometimes is invalid, warn and continue */
> + dev_warn_once(dev, FW_BUG "GPIO %u is not in FW pins bitmap\n", offset);
> + }
> +
> + *bank_ret = bank;
> + *pin_ret = pin;
> + return true;
> +}
> +
> +static int usbio_gpio_get_direction(struct gpio_chip *gc, unsigned int offset)
> +{
> + struct usbio_gpio_bank *bank;
> + int pin;
> + u8 cfg;
> +
> + if (!usbio_gpio_get_bank_and_pin(gc, offset, &bank, &pin))
> + return -EINVAL;
> +
> + cfg = bank->config[pin] & USBIO_GPIO_PINMOD_MASK;
> + if (cfg == USBIO_GPIO_PINMOD_OUTPUT)
> + return GPIO_LINE_DIRECTION_OUT;
> + else
> + return GPIO_LINE_DIRECTION_IN;

The else branch is not useful. But I think you could do instead:

return cfg == USBIO_GPIO_PINMOD_OUTPUT ?
GPIO_LINE_DIRECTION_OUT : GPIO_LINE_DIRECTION_IN;

> +}
> +
> +static int usbio_gpio_get(struct gpio_chip *gc, unsigned int offset)
> +{
> + struct usbio_gpio *gpio = gpiochip_get_data(gc);
> + struct usbio_gpio_bank *bank;
> + struct usbio_gpio_rw gbuf;
> + int pin, ret;
> +
> + if (!usbio_gpio_get_bank_and_pin(gc, offset, &bank, &pin))
> + return -EINVAL;
> +
> + gbuf.bankid = offset / USBIO_GPIOSPERBANK;
> + gbuf.pincount = 1;
> + gbuf.pin = pin;
> +
> + ret = usbio_control_msg(gpio->adev, USBIO_PKTTYPE_GPIO, USBIO_GPIOCMD_READ,
> + &gbuf, sizeof(gbuf) - sizeof(gbuf.value),
> + &gbuf, sizeof(gbuf));
> + if (ret != sizeof(gbuf))
> + return (ret < 0) ? ret : -EPROTO;

Unneeded parentheses.

> +
> + return (gbuf.value >> pin) & 1;
> +}
> +
> +static void usbio_gpio_set(struct gpio_chip *gc, unsigned int offset,
> + int value)
> +{
> + struct usbio_gpio *gpio = gpiochip_get_data(gc);
> + struct usbio_gpio_bank *bank;
> + struct usbio_gpio_rw gbuf;
> + int pin;
> +
> + if (!usbio_gpio_get_bank_and_pin(gc, offset, &bank, &pin))
> + return;
> +
> + gbuf.bankid = offset / USBIO_GPIOSPERBANK;
> + gbuf.pincount = 1;
> + gbuf.pin = pin;
> + gbuf.value = value << pin;
> +
> + usbio_control_msg(gpio->adev, USBIO_PKTTYPE_GPIO, USBIO_GPIOCMD_WRITE,
> + &gbuf, sizeof(gbuf), NULL, 0);
> +}
> +
> +static int usbio_gpio_update_config(struct gpio_chip *gc, unsigned int offset,
> + u8 mask, u8 value)
> +{
> + struct usbio_gpio *gpio = gpiochip_get_data(gc);
> + struct usbio_gpio_bank *bank;
> + struct usbio_gpio_init gbuf;
> + int pin;
> +
> + if (!usbio_gpio_get_bank_and_pin(gc, offset, &bank, &pin))
> + return -EINVAL;
> +
> + bank->config[pin] &= ~mask;
> + bank->config[pin] |= value;
> +

You could declare gbuf here and assign the fields in initialisation. There
are no additional, uninitialised fields in the struct but still I'd do
that.

> + gbuf.bankid = offset / USBIO_GPIOSPERBANK;
> + gbuf.config = bank->config[pin];
> + gbuf.pincount = 1;
> + gbuf.pin = pin;
> +
> + return usbio_control_msg(gpio->adev, USBIO_PKTTYPE_GPIO, USBIO_GPIOCMD_INIT,
> + &gbuf, sizeof(gbuf), NULL, 0);
> +}
> +
> +static int usbio_gpio_direction_input(struct gpio_chip *gc, unsigned int offset)
> +{
> + return usbio_gpio_update_config(gc, offset, USBIO_GPIO_PINMOD_MASK,
> + USBIO_GPIO_SET_PINMOD(USBIO_GPIO_PINMOD_INPUT));
> +}
> +
> +static int usbio_gpio_direction_output(struct gpio_chip *gc,
> + unsigned int offset, int value)
> +{
> + int ret;
> +
> + ret = usbio_gpio_update_config(gc, offset, USBIO_GPIO_PINMOD_MASK,
> + USBIO_GPIO_SET_PINMOD(USBIO_GPIO_PINMOD_OUTPUT));
> + if (ret)
> + return ret;
> +
> + usbio_gpio_set(gc, offset, value);

Please check the return value (after switching to set_rv(); see below).

> + return 0;
> +}
> +
> +static int usbio_gpio_set_config(struct gpio_chip *gc, unsigned int offset,
> + unsigned long config)
> +{
> + u8 value;
> +
> + switch (pinconf_to_config_param(config)) {
> + case PIN_CONFIG_BIAS_PULL_PIN_DEFAULT:
> + value = USBIO_GPIO_SET_PINCFG(USBIO_GPIO_PINCFG_DEFAULT);
> + break;
> + case PIN_CONFIG_BIAS_PULL_UP:
> + value = USBIO_GPIO_SET_PINCFG(USBIO_GPIO_PINCFG_PULLUP);
> + break;
> + case PIN_CONFIG_BIAS_PULL_DOWN:
> + value = USBIO_GPIO_SET_PINCFG(USBIO_GPIO_PINCFG_PULLDOWN);
> + break;
> + case PIN_CONFIG_DRIVE_PUSH_PULL:
> + value = USBIO_GPIO_SET_PINCFG(USBIO_GPIO_PINCFG_PUSHPULL);
> + break;
> + default:
> + return -ENOTSUPP;
> + }
> +
> + return usbio_gpio_update_config(gc, offset, USBIO_GPIO_PINCFG_MASK, value);
> +}
> +
> +static int usbio_gpio_probe(struct auxiliary_device *adev,
> + const struct auxiliary_device_id *adev_id)
> +{
> + struct usbio_gpio_bank_desc *bank_desc;
> + struct device *dev = &adev->dev;
> + struct usbio_gpio *gpio;
> + int bank, ret;
> +
> + bank_desc = dev_get_platdata(dev);
> + if (!bank_desc)
> + return -EINVAL;
> +
> + gpio = devm_kzalloc(dev, sizeof(*gpio), GFP_KERNEL);
> + if (!gpio)
> + return -ENOMEM;

All devm-allocated memory is released once the device is gone but you may
still have users. Could you release this in the adev's release callback
instead?

> +
> + gpio->adev = adev;
> +
> + usbio_acpi_bind(gpio->adev, usbio_gpio_acpi_hids);
> +
> + for (bank = 0; bank < USBIO_MAX_GPIOBANKS && bank_desc[bank].bmap; bank++)
> + gpio->banks[bank].bitmap = bank_desc[bank].bmap;
> +
> + gpio->gc.label = ACPI_COMPANION(dev) ?
> + acpi_dev_name(ACPI_COMPANION(dev)) : dev_name(dev);
> + gpio->gc.parent = dev;
> + gpio->gc.owner = THIS_MODULE;
> + gpio->gc.get_direction = usbio_gpio_get_direction;
> + gpio->gc.direction_input = usbio_gpio_direction_input;
> + gpio->gc.direction_output = usbio_gpio_direction_output;
> + gpio->gc.get = usbio_gpio_get;
> + gpio->gc.set = usbio_gpio_set;

Please use set_rv() callback; it can return an error. The set callback is
also deprecated nowadays.

> + gpio->gc.set_config = usbio_gpio_set_config;
> + gpio->gc.base = -1;
> + gpio->gc.ngpio = bank * USBIO_GPIOSPERBANK;
> + gpio->gc.can_sleep = true;
> +
> + auxiliary_set_drvdata(adev, gpio);
> +
> + ret = gpiochip_add_data(&gpio->gc, gpio);
> + if (ret)
> + return ret;
> +
> + if (has_acpi_companion(dev))
> + acpi_dev_clear_dependencies(ACPI_COMPANION(dev));
> +
> + return 0;
> +}
> +
> +static void usbio_gpio_remove(struct auxiliary_device *adev)
> +{
> + struct usbio_gpio *gpio = auxiliary_get_drvdata(adev);
> +
> + gpiochip_remove(&gpio->gc);
> +}
> +
> +static const struct auxiliary_device_id usbio_gpio_id_table[] = {
> + { "usbio.usbio-gpio" },
> + { }
> +};
> +MODULE_DEVICE_TABLE(auxiliary, usbio_gpio_id_table);
> +
> +static struct auxiliary_driver usbio_gpio_driver = {
> + .name = USBIO_GPIO_CLIENT,
> + .probe = usbio_gpio_probe,
> + .remove = usbio_gpio_remove,
> + .id_table = usbio_gpio_id_table
> +};
> +module_auxiliary_driver(usbio_gpio_driver);
> +
> +MODULE_DESCRIPTION("Intel USBIO GPIO driver");
> +MODULE_AUTHOR("Israel Cepeda <israel.a.cepeda.lopez@intel.com>");
> +MODULE_AUTHOR("Hans de Goede <hansg@kernel.org>");
> +MODULE_LICENSE("GPL");
> +MODULE_IMPORT_NS("USBIO");

--
Kind regards,

Sakari Ailus