Re: [PATCH 06/11] gpio: Add NVIDIA LSTP GPIO driver

[Linus Walleij]

Hi Adrian,

thanks for your patch!

On Mon, Oct 12, 2026 at 7:37 PM Adrian Ambrozewicz
<aambrozewicz@xxxxxxxxxx> wrote:

> Add a driver for the GPIO channels of NVIDIA LSTP USB adapters. Each
> channel is registered as a GPIO chip with an interrupt controller for
> its pins; every line access is a USB transaction, so the chip can
> sleep. Pins the firmware reports as LEDs are not GPIO lines: the driver
> hands them to the LED driver as an auxiliary device.
>
> At probe the driver reads each pin's interrupt configuration from the
> device. A reply that misses the response timeout can answer a later
> request and shift every following pin's state by one, so a single read
> cannot simply be retried. After a timeout the driver reads all pins
> again, alternating ascending and descending order, until two
> consecutive passes without a timeout agree: at most five passes, ending
> within 10 s of the first timeout, after which the probe fails as
> before. The probe runs in the context that added the device, often the
> hub's worker, hence the bound.
>
> Signed-off-by: Adrian Ambrozewicz <aambrozewicz@xxxxxxxxxx>
(...)
> +++ b/drivers/gpio/Kconfig
> @@ -2051,6 +2051,20 @@ config GPIO_USBIO
>           This driver can also be built as a module. If so, the module
>           will be called gpio_usbio.
>
> +config GPIO_LSTP

GPIO_USBIO ... GPIO_LSTP?

These should be in alphabetical order.

> +#include <linux/atomic.h>
> +#include <linux/bitmap.h>
> +#include <linux/build_bug.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/gpio/driver.h>
> +#include <linux/irq.h>
> +#include <linux/jiffies.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/seq_file.h>
> +#include <linux/slab.h>
> +#include <linux/string.h>
> +#include <linux/wait_bit.h>
> +#include <linux/workqueue.h>
> +#include <uapi/linux/gpio.h>

Do you really need all this?
Certainly <uapi/linux/gpio.h> is a mistake, look over all of these.

> +/* clang-format off */

I beg your pardon? What is this?

> +enum lstp_gpio_direction {
> +       LSTP_GPIO_OUTPUT = 0x00,
> +       LSTP_GPIO_INPUT = 0x01
> +} __packed;
> +enum lstp_gpio_pin_state {
> +       LSTP_GPIO_LOW = 0x00,
> +       LSTP_GPIO_HIGH = 0x01
> +} __packed;
> +enum lstp_gpio_drive_config {
> +       LSTP_GPIO_PUSH_PULL = 0x00,
> +       LSTP_GPIO_OPEN_DRAIN = 0x01,
> +       LSTP_GPIO_OPEN_SOURCE = 0x02
> +} __packed;
> +enum lstp_gpio_bias_config {
> +       LSTP_GPIO_NO_PULL = 0x00,
> +       LSTP_GPIO_PULL_UP = 0x01,
> +       LSTP_GPIO_PULL_DOWN = 0x02
> +} __packed;
> +enum lstp_gpio_led_support {
> +       LSTP_GPIO_NOT_LED = 0x00,
> +       LSTP_GPIO_LED_MODE_HOLD = 0x01, /* pattern brightness jumps between steps */
> +       LSTP_GPIO_LED_MODE_INTERPOLATE = 0x02, /* pattern brightness interpolates between steps */
> +} __packed; /* clang-format on */

This  looks very dense too..

At least a newline between the enums?

What's with the clang-format thing? I don't get it.

> +struct lstp_gpio_priv {

Just call it lstp_gpio

> +       struct lstp_channel *ch;
> +       struct gpio_chip gc;

Why? Normally drivers get the state container from the
chip not the other way around.

> +static int lstp_gpio_get_direction(struct gpio_chip *gc, unsigned int pin)
> +{
> +       struct lstp_gpio_priv *priv = gpiochip_get_data(gc);
> +       u8 direction;
> +
> +       int ret = lstp_gpio_read_direction(priv, pin, &direction);
> +
> +       if (ret)
> +               return ret;
> +
> +       if (direction == LSTP_GPIO_INPUT)
> +               return GPIO_LINE_DIRECTION_IN;
> +
> +       return GPIO_LINE_DIRECTION_OUT;
> +}
> +
> +static int lstp_gpio_direction_input(struct gpio_chip *gc, unsigned int pin)
> +{
> +       struct lstp_gpio_priv *priv = gpiochip_get_data(gc);
> +
> +       return lstp_gpio_require_direction(priv, pin, LSTP_GPIO_INPUT);
> +}
> +
> +static int lstp_gpio_direction_output(struct gpio_chip *gc, unsigned int pin, int value)
> +{
> +       struct lstp_gpio_priv *priv = gpiochip_get_data(gc);
> +       int ret = lstp_gpio_require_direction(priv, pin, LSTP_GPIO_OUTPUT);
> +
> +       if (ret)
> +               return ret;
> +
> +       return lstp_gpio_set_value(gc, pin, value);
> +}

It seems some pins are only input and some are only output.

Have you considered using serialized register access over
regmap and use regmap-gpio with fixed direction bitmaps?

Yours,
Linus Walleij