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

[Andy Shevchenko <andy.shevchenko@xxxxxxxxx>]

On Sun, Jun 13, 2021 at 2:20 AM Jonathan Neuschäfer
<j.neuschaefer@xxxxxxx> wrote:
> On Wed, Jun 02, 2021 at 03:50:39PM +0300, Andy Shevchenko wrote:
> > On Wed, Jun 2, 2021 at 3:05 PM Jonathan Neuschäfer
> > <j.neuschaefer@xxxxxxx> wrote:

...

> > > +       help
> > > +         Say Y here to enable pin controller and GPIO support
> > > +         for the Nuvoton WPCM450 SoC.
> >
> > >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.)

...

> > > +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);
> >
> > > +               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.

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

...

> > > +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;
> >
> > > +       int ret = 0;
> >
> > Redundant. Can do it without it.
> >
> > > +       spin_lock_irqsave(&pctrl->lock, flags);
> > > +       if (port->cfg0) {
> >
> > > +       } 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.

...

> > 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.

...

> > > +       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