Re: [PATCH] gpio: Add Intel Granite Rapids-D vGPIO driver

[Linus Walleij <linus.walleij@xxxxxxxxxx>]

Hi Aapo,

thanks for your patch!

The code is impeccable, not much to say about that.
>From that PoV the driver is finished.

I have some technical review comments:

On Fri, Apr 19, 2024 at 10:07 AM Aapo Vienamo
<aapo.vienamo@xxxxxxxxxxxxxxx> wrote:

> This driver provides a basic GPIO driver for the Intel Granite Rapids-D
> virtual GPIOs. On SoCs with limited physical pins on the package, the
> physical pins controlled by this driver would be exposed on an external
> device such as a BMC or CPLD.
>
> Signed-off-by: Aapo Vienamo <aapo.vienamo@xxxxxxxxxxxxxxx>
> Reviewed-by: Mika Westerberg <mika.westerberg@xxxxxxxxxxxxxxx>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>

OK I get how it works, but not all the way right? We write these registers,
and somehow that results on pins on a completely different piece of
silicon in a different package driving some lines low/high?

So ... can we write something about how the signal gets over there
from where the driver is running? It needs to happen somehow, right?

> +config GPIO_GRANITERAPIDS
> +       tristate "Intel Granite Rapids-D vGPIO support"
> +       depends on X86 || COMPILE_TEST
> +       select GPIOLIB_IRQCHIP
> +       help
> +         Select this to enable GPIO support on platforms with the following
> +         SoCs:
> +
> +         - Intel Granite Rapids-D
> +
> +         The driver enables basic GPIO functionality and implements interrupt
> +         support.
> +
> +         To compile this driver as a module, choose M here: the module will
> +         be called gpio-graniterapids.

This help text is not as informative as the commit log. Write something
about how the GPIO works here, too.

> +static int gnr_gpio_configure_pad(struct gpio_chip *gc, unsigned int gpio,
> +                                 u32 clear_mask, u32 set_mask)
> +{
> +       struct gnr_gpio *priv = gpiochip_get_data(gc);
> +       void __iomem *addr = gnr_gpio_get_padcfg_addr(priv, gpio);
> +       u32 dw;
> +
> +       if (test_bit(gpio, priv->ro_bitmap))
> +               return -EACCES;
> +
> +       guard(raw_spinlock_irqsave)(&priv->lock);
> +
> +       dw = readl(addr);
> +       dw &= ~clear_mask;
> +       dw |= set_mask;
> +       writel(dw, addr);
> +
> +       return 0;
> +}

Configure pad sounds like pin control so it's a bit of icky name.
What it really does is configure the direction (in or out) for this
GPIO pad. And it's not really the *pad* that is configured, right?
It is the hardware *driver* for the pad, i.e. what is reflected in
the GPIO line control register.

Can you rename this:
gnr_gpio_configure_direction()?

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

Yours,
Linus Walleij