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

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: <ljyjvdtzhgug7frkiwbrvobbusnzqu5gpn345n5bjsmbuw5gjd@xex3dznz5jov>
Date: Fri, 19 Apr 2024 17:42:59 +0300
From: Aapo Vienamo <aapo.vienamo@...ux.intel.com>
To: Linus Walleij <linus.walleij@...aro.org>
Cc: Bartosz Golaszewski <brgl@...ev.pl>, Andy Shevchenko <andy@...nel.org>, 
	linux-kernel@...r.kernel.org, linux-gpio@...r.kernel.org, 
	Mika Westerberg <mika.westerberg@...ux.intel.com>, Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Subject: Re: [PATCH] gpio: Add Intel Granite Rapids-D vGPIO driver

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