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

[Jonathan Neuschäfer]

Hello,

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:
> >
> > This driver is heavily based on the one for NPCM7xx, because the WPCM450
> > is a predecessor of those SoCs.
> >
> > The biggest difference is in how the GPIO controller works. In the
> > WPCM450, the GPIO registers are not organized in multiple banks, but
> > rather placed continually into the same register block, and the driver
> > reflects this.
> >
> > Some functionality implemented in the hardware was (for now) left unused
> > in the driver, specifically blinking and pull-up/down.
> 
> ...
> 
> > +config PINCTRL_WPCM450
> > +       bool "Pinctrl and GPIO driver for Nuvoton WPCM450"
> 
> Why it can't be a module?

My mistake, it does indeed work as a module. I'll change it to tristate.

> > +       depends on (ARCH_WPCM450 || COMPILE_TEST) && OF
> 
> Is it really OF dependent (see below)?

After switching to the fwnode API, no. I'll drop the dependency.

> > +       select PINMUX
> > +       select PINCONF
> > +       select GENERIC_PINCONF
> > +       select GPIOLIB
> > +       select GPIO_GENERIC
> > +       select GPIOLIB_IRQCHIP
> > +       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.


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


> > +#include <linux/module.h>
> 
> mod_devicetable.h

I'll add it.

> 
> > +#include <linux/of.h>
> > +#include <linux/of_address.h>
> > +#include <linux/of_irq.h>
> 
> > +#include <linux/pinctrl/machine.h>
> > +#include <linux/pinctrl/pinconf.h>
> > +#include <linux/pinctrl/pinconf-generic.h>
> > +#include <linux/pinctrl/pinctrl.h>
> > +#include <linux/pinctrl/pinmux.h>
> 
> Can you move this group...
> 
> > +#include <linux/platform_device.h>
> > +#include <linux/regmap.h>
> 
> ...here?
> 
> It will show the relation with pin control subsystem and separate from
> global / generic headers.

Will do.


> > +        * This spinlock protects registers and struct wpcm450_pinctrl fields
> 
> spin lock

Ok


> > +/* GPIO handling in the pinctrl driver */
> > +
> 
> Useless.

Ok

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

wpcm450_gpio_direction_input and a few other functions implement a
read/modify/write cycle, where the lock makes more sense:

	spin_lock_irqsave(&pctrl->lock, flags);
	if (port->cfg0) {
		cfg0 = ioread32(pctrl->gpio_base + port->cfg0);
		cfg0 &= ~port_mask(port, offset);
		iowrite32(cfg0, pctrl->gpio_base + port->cfg0);
	}
	spin_unlock_irqrestore(&pctrl->lock, flags);

(Here, as above, the locking calls moved into the if block, but I'm not
 sure how much of an improvement that would be.)

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


> > +/* Interrupt support */
> > +
> 
> Useless besides being in a bad style.

Ok, will remove.
> 
> ...
> 
> > +static int event_bitmask(int gpio)
> > +{
> > +       if (gpio >= 0 && gpio < 16)
> > +               return BIT(gpio);
> > +       if (gpio == 24 || gpio == 25)
> > +               return BIT(gpio - 8);
> > +       return -EINVAL;
> > +}
> 
> Can you consider to use bitmap_bitremap()

> > +static int event_bitnum_to_gpio(int num)
> > +{
> > +       if (num >= 0 && num < 16)
> > +               return num;
> > +       if (num == 16 || num == 17)
> > +               return num + 8;
> > +       return -EINVAL;
> > +}
> 
> Ditto.
> 
> See gpio-xilinx.c for example.

I looked at it now. I'm not convinced it'll improve readability here.

> 
> ...
> 
> > +static void wpcm450_gpio_irq_ack(struct irq_data *d)
> > +{
> > +       struct wpcm450_pinctrl *pctrl = gpiochip_get_data(irq_data_get_irq_chip_data(d));
> 
> > +       int mask = event_bitmask(d->hwirq);
> 
> Move the assignment closer to the check.
> Ditto for other same and similar cases in the code.

Will do.

> 
> > +       unsigned long flags;
> > +
> > +       if (mask < 0)
> > +               return;
> 
> > +}
> 
> ...
> 
> > +       int mask = event_bitmask(d->hwirq);
> 
> Use irqd_to_hwirq() (please check that I spelled it correctly).
> Same for all hwirq getters.

Will do.

> 
> ...
> 
> > +static void wpcm450_gpio_fix_evpol(struct wpcm450_pinctrl *pctrl, unsigned long all)
> > +{
> > +       int bitnum;
> 
> Can it be negative?

No. I'll change it to unsigned int.
> 
> > +       for_each_set_bit(bitnum, &all, 32) {
> 
> > +               int gpio = event_bitnum_to_gpio(bitnum);
> > +               u32 mask = BIT(bitnum), evpol;
> 
> unsigned long evpol;
> 
> > +               int level;
> > +
> > +               do {
> > +                       evpol = ioread32(pctrl->gpio_base + WPCM450_GPEVPOL);
> > +                       level = wpcm450_gpio_get(&pctrl->gc, gpio);
> 
> > +                       /* Switch event polarity to the opposite of the current level */
> > +                       if (level)
> > +                               evpol &= ~mask;
> > +                       else
> > +                               evpol |= mask;
> 
> __assign_bit(bitnum, &evpol, level);

Ah, very useful. I'll use __assign_bit in a few places.


> > +static int wpcm450_gpio_set_irq_type(struct irq_data *d, unsigned int flow_type)
> > +{
> 
> Consider to assign handler type here.

Will do.


> > +/* pinmux handing */
> > +
> 
> Useless.

Ok.


> > +/*
> > + * pin:             name, number
> > + * group:    name, npins,   pins
> > + * function: name, ngroups, groups
> > + */
> > +struct wpcm450_group {
> > +       const char *name;
> > +       const unsigned int *pins;
> > +       int npins;
> > +};
> 
> Use struct group_desc from core.h.

Ok.

> > +/* pinctrl_ops */
> 
> Useless.

Ok

> 
> > +static int wpcm450_get_groups_count(struct pinctrl_dev *pctldev)
> > +{
> > +       struct wpcm450_pinctrl *npcm = pinctrl_dev_get_drvdata(pctldev);
> 
> > +       dev_dbg(npcm->dev, "group size: %d\n", ARRAY_SIZE(wpcm450_groups));
> 
> Ditto.

Removing this and similar messages.


> > +/* pinmux_ops  */
> 
> Useless.

Ok

> 
> ...
> 
> > +static int wpcm450_gpio_request_enable(struct pinctrl_dev *pctldev,
> > +                                      struct pinctrl_gpio_range *range,
> > +                                      unsigned int offset)
> > +{
> > +       struct wpcm450_pinctrl *npcm = pinctrl_dev_get_drvdata(pctldev);

Oops, I forgot to change the name to pctrl here.

> 
> > +       if (!range) {
> > +               dev_err(npcm->dev, "invalid range\n");
> > +               return -EINVAL;
> > +       }
> 
> Dead code?

Indeed, the range pointer is checked in pin_request().

> 
> > +       if (!range->gc) {
> > +               dev_err(npcm->dev, "invalid gpiochip\n");
> > +               return -EINVAL;
> > +       }
> 
> Dead code?

... and range->gc isn't used here... I'll remove the check.
> 
> > +       wpcm450_setfunc(npcm->gcr_regmap, &offset, 1, fn_gpio);
> > +
> > +       return 0;
> > +}
> 
> ...
> 
> > +/* Release GPIO back to pinctrl mode */
> > +static void wpcm450_gpio_request_free(struct pinctrl_dev *pctldev,
> > +                                     struct pinctrl_gpio_range *range,
> > +                                     unsigned int offset)
> > +{
> > +       struct wpcm450_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
> > +       int virq;
> > +
> > +       virq = irq_find_mapping(pctrl->domain, offset);
> > +       if (virq)
> > +               irq_dispose_mapping(virq);
> 
> Why it needs to be done here?

I copied this function from the pinctrl-npcm7xx driver; I don't really
see a reason to call irq_dispose_mapping here.

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

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


> > +/* pinconf_ops */
> 
> Useless.

Ok


> > +static int debounce_bitmask(int gpio)
> > +{
> > +       if (gpio >= 0 && gpio < 16)
> > +               return BIT(gpio);
> > +       return -EINVAL;
> > +}
> 
> I don't see users of it except one below, care to inline?

It should have been used in wpcm450_config_set_one, but I missed that.

> > +static int wpcm450_config_get(struct pinctrl_dev *pctldev, unsigned int pin,
> > +                             unsigned long *config)
> > +{
> 
> > +       switch (param) {
> > +       case PIN_CONFIG_INPUT_DEBOUNCE:
> > +               mask = debounce_bitmask(pin);
> > +               if (mask < 0)
> > +                       return mask;
> 
> > +               break;
> > +       default:
> > +               return -ENOTSUPP;
> > +       }
> > +
> > +       return 0;
> > +}
> 
> ...
> 
> > +/* Set multiple configuration settings for a pin */
> 
> Useless.

Ok.

> 
> ...
> 
> > +static int wpcm450_config_set(struct pinctrl_dev *pctldev, unsigned int pin,
> > +                             unsigned long *configs, unsigned int num_configs)
> > +{
> > +       struct wpcm450_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
> 
> > +       int rc;
> 
> Why out of a sudden different (inconsistent) name?

No particular reason. I'll change it to ret.

> 
> > +       return 0;
> > +}
> 
> ...
> 
> > +       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.

> 
> ...
> 
> > +       pctrl->gpio_base = of_iomap(np, 0);
> 
> devm_platform_ioremap_resource();

Will change.
> 
> > +       if (!pctrl->gpio_base) {
> > +               dev_err(pctrl->dev, "Resource fail for GPIO controller\n");
> > +               return -ENOMEM;
> > +       }
> 
> Here leak of resources. See above.
> 
> ...
> 
> > +       pctrl->gc.get_direction = wpcm450_gpio_get_direction;
> > +       pctrl->gc.direction_input = wpcm450_gpio_direction_input;
> > +       pctrl->gc.direction_output = wpcm450_gpio_direction_output;
> > +       pctrl->gc.get = wpcm450_gpio_get;
> > +       pctrl->gc.set = wpcm450_gpio_set;
> 
> No ->set_config()?

Good point, I'll hook it up to wpcm450_config_set_one().

> 
> ...
> 
> > +       girq->default_type = IRQ_TYPE_NONE;
> 
> > +       girq->handler = handle_level_irq;
> 
> Use ->irq_set_type() for this. Here should be handle_bad_irq().

Ok.

> 
> > +       for (i = 0; i < WPCM450_NUM_GPIO_IRQS; i++) {
> 
> > +               int irq = irq_of_parse_and_map(np, i);
> 
> fwnode_get_irq()

Indeed, will switch to fwnode_irq_get.

> 
> > +               if (irq < 0) {
> > +                       dev_err(pctrl->dev, "No IRQ for GPIO controller\n");
> > +                       return irq;
> > +               }
> > +               girq->parents[i] = irq;
> > +       }
> 
> ...
> 
> > +       pctrl->pctldev = devm_pinctrl_register(&pdev->dev,
> > +                                              &wpcm450_pinctrl_desc, pctrl);
> > +       if (IS_ERR(pctrl->pctldev)) {
> 
> > +               dev_err(&pdev->dev, "Failed to register pinctrl device\n");
> > +               return PTR_ERR(pctrl->pctldev);
> 
> Shouldn't it be return dev_err_probe(...); here?

It's so far a rare pattern after devm_pinctrl_register, but it makes
sense. Will change (in both cases above).

(Perhaps I should also use dev_err_probe in wpcm450_gpio_register, since
it's called from the probe function.)

> 
> > +       }
> 
> ...
> 
> > +       pr_info("WPCM450 pinctrl and GPIO driver probed\n");
> 
> Besides you have to use dev_*() this is completely useless and noisy message.

I'll remove this message and review the other messages.

> 
> ...
> 
> > +static const struct of_device_id wpcm450_pinctrl_match[] = {
> > +       { .compatible = "nuvoton,wpcm450-pinctrl" },
> 
> > +       { },
> 
> Comma is not needed for terminator line.

True, will remove.
> 
> > +};
> 
> ...
> 
> > +               .suppress_bind_attrs = true,
> 
> Why?

Copied from pinctrl-npcm7xx, but I see no reason to keep it.

> 
> -- 
> With Best Regards,
> Andy Shevchenko


Thank you for your detailed review,
Jonthan Neuschäfer

Attachment: signature.asc
Description: PGP signature