linux-kernel - Re: [PATCH v2 1/2] Support NVIDIA BlueField-3 GPIO controller

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>] [day] [month] [year] [list]

Message-ID: <CACRpkdZ_sPCa+Q6MWUKj1ytCe5AxTp--bMmbRTUfsNO0kZTMpQ@mail.gmail.com>
Date:   Thu, 9 Feb 2023 11:53:50 +0100
From:   Linus Walleij <linus.walleij@...aro.org>
To:     Asmaa Mnebhi <asmaa@...dia.com>
Cc:     linux-gpio@...r.kernel.org, linux-kernel@...r.kernel.org,
        andy.shevchenko@...il.com, bgolaszewski@...libre.com,
        linux-acpi@...r.kernel.org
Subject: Re: [PATCH v2 1/2] Support NVIDIA BlueField-3 GPIO controller

Hi Asmaa,

thanks for your patch!

On Wed, Feb 8, 2023 at 7:57 PM Asmaa Mnebhi <asmaa@...dia.com> wrote:

> This patch adds support for the BlueField-3 SoC GPIO driver
> which allows:
> - setting certain GPIOs as interrupts from other dependent drivers
> - ability to manipulate certain GPIO pins via libgpiod tools for instance
>
> BlueField-3 has 56 GPIOs but the user is only allowed to change some
> of them into GPIO mode. Use valid_mask to make it impossible to alter
> the rest of the GPIOs.
>
> Signed-off-by: Asmaa Mnebhi <asmaa@...dia.com>

(...)

> +struct mlxbf3_gpio_context {
> +       struct gpio_chip gc;
> +       struct irq_chip irq_chip;
> +
> +       /* YU GPIO block address */
> +       void __iomem *gpio_io;
> +
> +       /* YU GPIO cause block address */
> +       void __iomem *gpio_cause_io;

I especially like that you use the word "cause" (as in cause-and-effect)
over the imprecise and overused word "reason" (brrrr)

> +static int mlxbf3_gpio_direction_input(struct gpio_chip *chip,
> +                                      unsigned int offset)
> +{
> +       struct mlxbf3_gpio_context *gs = gpiochip_get_data(chip);
> +       unsigned long flags;
> +
> +       raw_spin_lock_irqsave(&gs->gc.bgpio_lock, flags);
> +
> +       writel(BIT(offset), gs->gpio_io + MLXBF_GPIO_FW_OUTPUT_ENABLE_CLEAR);
> +
> +       raw_spin_unlock_irqrestore(&gs->gc.bgpio_lock, flags);
> +
> +       return 0;
> +}
> +
> +static int mlxbf3_gpio_direction_output(struct gpio_chip *chip,
> +                                       unsigned int offset,
> +                                       int value)
> +{
> +       struct mlxbf3_gpio_context *gs = gpiochip_get_data(chip);
> +       unsigned long flags;
> +
> +       raw_spin_lock_irqsave(&gs->gc.bgpio_lock, flags);
> +
> +       writel(BIT(offset), gs->gpio_io + MLXBF_GPIO_FW_OUTPUT_ENABLE_SET);
> +
> +       if (value)
> +               writel(BIT(offset), gs->gpio_io + MLXBF_GPIO_FW_DATA_OUT_SET);
> +       else
> +               writel(BIT(offset), gs->gpio_io + MLXBF_GPIO_FW_DATA_OUT_CLEAR);
> +
> +       raw_spin_unlock_irqrestore(&gs->gc.bgpio_lock, flags);
> +
> +       return 0;
> +}

GPIO_GENERIC / bgpio should be able to handle these too.

> +       ret = bgpio_init(gc, dev, 4,
> +                       gs->gpio_io + MLXBF_GPIO_READ_DATA_IN,
> +                       gs->gpio_io + MLXBF_GPIO_FW_DATA_OUT_SET,
> +                       gs->gpio_io + MLXBF_GPIO_FW_DATA_OUT_CLEAR,
> +                       NULL, NULL, 0);

Instead of NULL, NULL, 0

use

gs->gpio_io + MLXBF_GPIO_FW_OUTPUT_ENABLE_SET,
gs->gpio_io + MLXBF_GPIO_FW_OUTPUT_ENABLE_CLEAR,
0);

The generic library will make sure to also set the output value when
changing direction to output.

Yours,
Linus Walleij