Received-SPF: pass (google.com: domain of linux-kernel+bounces-151596-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Fri, 19 Apr 2024 17:42:59 +0300
From: Aapo Vienamo <aapo.vienamo@linux.intel.com>
To: Linus Walleij <linus.walleij@linaro.org>
Cc: Bartosz Golaszewski <brgl@bgdev.pl>, Andy Shevchenko <andy@kernel.org>, 
	linux-kernel@vger.kernel.org, linux-gpio@vger.kernel.org, 
	Mika Westerberg <mika.westerberg@linux.intel.com>, Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Subject: Re: [PATCH] gpio: Add Intel Granite Rapids-D vGPIO driver
Message-ID: <ljyjvdtzhgug7frkiwbrvobbusnzqu5gpn345n5bjsmbuw5gjd@xex3dznz5jov>
References: <20240419080555.97343-1-aapo.vienamo@linux.intel.com>
 <CACRpkdbSB+JTdhGXViWs-SmR3nUnm6dVXt3WzK-d4zFSz63XxQ@mail.gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CACRpkdbSB+JTdhGXViWs-SmR3nUnm6dVXt3WzK-d4zFSz63XxQ@mail.gmail.com>

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