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

[Aapo Vienamo]

Hi Linus,

Thanks for the review!

On Fri, Apr 19, 2024 at 03:11:23PM GMT, Linus Walleij wrote:
> On Fri, Apr 19, 2024 at 10:07 AM Aapo Vienamo
> > +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()?

I do agree that the pad part of the name maybe isn't the best, though
this function isn't just for direction control, since it's used for
setting the pin output state as well in gnr_gpio_set(). The idea is that
locking and masking of the register accesses is factored out of the gpio
callbacks and implemented in this function.

Maybe gnr_gpio_configure_pin()?

Best regards,
Aapo