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

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

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

+       help
+         Say Y here to enable pin controller and GPIO support
+         for the Nuvoton WPCM450 SoC.

quoted

From this help it's not clear why user should or shouldn't enable it.

Please, elaborate (and hence fix checkpatch warning).

I'll try something like this, but I'm open for better ideas:

        help
          Say Y or M here to enable pin controller and GPIO support for
          the Nuvoton WPCM450 SoC. This is strongly recommended when
          building a kernel that will run on this chip.

          If this driver is compiled as a module, it will be named
          pinctrl-wpcm450.

This looks good enough.

I could mention some examples of functionality enabled by this driver:
LEDs, host power control, Ethernet.

(LEDs and host power control use GPIOs, at least on the Supermicro X9
 board I've been using. Ethernet MDIO must be enabled through the
 pinctrl driver, unless the bootloader has done so already; on my board
 the bootloader doesn't do this.)

...

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.

quoted

quoted

+       spin_unlock_irqrestore(&pctrl->lock, flags);
+       return dir;
+}

...

quoted

quoted

+static int wpcm450_gpio_direction_output(struct gpio_chip *chip,
+                                        unsigned int offset, int value)
+{
+       struct wpcm450_pinctrl *pctrl = gpiochip_get_data(chip);
+       const struct wpcm450_port *port = to_port(offset);
+       unsigned long flags;
+       u32 dataout, cfg0;

quoted

+       int ret = 0;

Redundant. Can do it without it.

quoted

+       spin_lock_irqsave(&pctrl->lock, flags);
+       if (port->cfg0) {

quoted

+       } else {
+               ret = -EINVAL;
+       }
+       spin_unlock_irqrestore(&pctrl->lock, flags);
+       return ret;
+}

I'll refactor it to return -EINVAL early.

Here a similar approach can be used.

...

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.

...

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.

-- 
With Best Regards,
Andy Shevchenko