Re: [PATCH 5/8] pinctrl: nuvoton: Add driver for WPCM450

On Sun, Jun 13, 2021 at 01:06:15PM +0300, Andy Shevchenko wrote:

On Sun, Jun 13, 2021 at 2:20 AM Jonathan Neuschäfer [off-list ref] wrote:

quoted

On Wed, Jun 02, 2021 at 03:50:39PM +0300, Andy Shevchenko wrote:

quoted

On Wed, Jun 2, 2021 at 3:05 PM Jonathan Neuschäfer [off-list ref] wrote:

[...]

quoted

quoted

quoted

+static int wpcm450_gpio_get_direction(struct gpio_chip *chip,
+                                     unsigned int offset)
+{
+       struct wpcm450_pinctrl *pctrl = gpiochip_get_data(chip);
+       const struct wpcm450_port *port = to_port(offset);
+       unsigned long flags;
+       u32 cfg0;
+       int dir;
+
+       spin_lock_irqsave(&pctrl->lock, flags);
+       if (port->cfg0) {
+               cfg0 = ioread32(pctrl->gpio_base + port->cfg0);

quoted

+               dir = !(cfg0 & port_mask(port, offset));
+       } else {
+               /* If cfg0 is unavailable, the GPIO is always an input */
+               dir = 1;
+       }

Why above is under spin lock?
Same question for all other similar places in different functions, if any.

My intention was to protect the ioread32. But given that it's just a
single MMIO read, this might be unnecessary.

Sometimes it's necessary and I'm not talking about it. (I put blank
lines around the code I was commenting on)

So, What I meant above is to get something like this

if (port->cfg0) {
  spin lock
  ...
  spin unlock
} else {
  ...
}

or equivalent ideas.

Ah, in other words: Narrowing the scope of the lock as far as possible.
I'll keep it in mind for v2.

quoted

quoted

What about the GPIO library API that does some additional stuff?

I don't know which gpiolib function would be appropriate here, sorry.

When you leave those request and release callbacks untouched the GPIO
library will assign default ones. You may see what they do.

Ah, I see. I'll look into it.

...

quoted

quoted

quoted

+       if (!of_find_property(np, "gpio-controller", NULL))
+               return -ENODEV;

Dead code?

The point here was to check if the node is marked as a GPIO controller,
with the boolean property "gpio-controller" (so device_property_read_bool
would probably be more appropriate).

However, since the gpio-controller property is already defined as
required in the DT binding, I'm not sure it's worth checking here.

Exactly my point.

Alright.


Thanks,
Jonthan Neuschäfer