linux-kernel - Re: [PATCH v2] gpio: add support for FTDI's MPSSE as GPIO

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CACRpkdZ6WuC6Ak3n2Dg8VFrv2-kpiuaMYeFaiSeAX9HEiNcf0A@mail.gmail.com>
Date: Sun, 22 Sep 2024 22:30:51 +0200
From: Linus Walleij <linus.walleij@...aro.org>
To: Mary Strodl <mstrodl@....rit.edu>
Cc: linux-kernel@...r.kernel.org, brgl@...ev.pl, linux-gpio@...r.kernel.org
Subject: Re: [PATCH v2] gpio: add support for FTDI's MPSSE as GPIO

Hi Mary,

thanks for your patch!

On Thu, Sep 19, 2024 at 4:10 PM Mary Strodl <mstrodl@....rit.edu> wrote:

> FTDI FT2232H is a USB to GPIO chip. Sealevel produces some devices
> with this chip. FT2232H presents itself as a composite device with two
> interfaces (each is an "MPSSE"). Each MPSSE has two banks (high and low)
> of 8 GPIO each. I believe some MPSSE's have only one bank, but I don't
> know how to identify them (I don't have any for testing) and as a result
> are unsupported for the time being.
>
> Additionally, this driver provides software polling-based interrupts for
> edge detection. For the Sealevel device I have to test with, this works
> well because there is hardware debouncing. From talking to Sealevel's
> people, this is their preferred way to do edge detection.
>
> Signed-off-by: Mary Strodl <mstrodl@....rit.edu>

Interesting device! Is it working nicely with hotplug
and using libgpiod for userspace access?

Overall this looks very good, some comments:

> +static int gpio_mpsse_get_direction(struct gpio_chip *chip,
> +                                   unsigned int offset)
> +{
> +       int ret;
> +       int bank = (offset & 8) >> 3;
> +       int bank_offset = offset & 7;
> +       struct mpsse_priv *priv = gpiochip_get_data(chip);
> +
> +       mutex_lock(&priv->io_mutex);

In all instances of mutex, I suggest using the new guarded
mutexes from <linux/cleanup.h>. I this case just

guard(mutex)(&priv->io_mutex);

And then the mutex will be dropped when the function exits.
Tighter scopes are possible, just:
git grep guard drivers/gpio/
for a ton of examples.

> +       /* MPSSE directions are inverted */
> +       if (priv->gpio_dir[bank] & BIT(bank_offset))
> +               ret = 0;
> +       else
> +               ret = 1;

Please use the new defines from <linux/gpio/driver.h>:
#define GPIO_LINE_DIRECTION_IN  1
#define GPIO_LINE_DIRECTION_OUT 0

With the above addressed:
Reviewed-by: Linus Walleij <linus.walleij@...aro.org>

Yours,
Linus Walleij