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

[Andy Shevchenko <andy.shevchenko@gmail.com>]

On Wed, Jun 2, 2021 at 3:05 PM Jonathan Neuschäfer
<j.neuschaefer@gmx.net> 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?

> +       depends on (ARCH_WPCM450 || COMPILE_TEST) && OF

Is it really OF dependent (see below)?

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

...

> +#include <linux/module.h>

mod_devicetable.h

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

...

> +       /*
> +        * This spinlock protects registers and struct wpcm450_pinctrl fields

spin lock

> +        * against concurrent access.
> +        */

...

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

Useless.

...

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

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

...

> +/* Interrupt support */
> +

Useless besides being in a bad style.

...

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

...

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

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

...

> +static void wpcm450_gpio_fix_evpol(struct wpcm450_pinctrl *pctrl, unsigned long all)
> +{
> +       int bitnum;

Can it be negative?

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

> +
> +                       iowrite32(evpol, pctrl->gpio_base + WPCM450_GPEVPOL);
> +               } while (wpcm450_gpio_get(&pctrl->gc, gpio) != level);
> +       }
> +}

...

> +static int wpcm450_gpio_set_irq_type(struct irq_data *d, unsigned int flow_type)
> +{

Consider to assign handler type here.

> +}

...

> +/* pinmux handing */
> +

Useless.

...

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

...

> +/* pinctrl_ops */

Useless.

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

> +       return ARRAY_SIZE(wpcm450_groups);
> +}

...

> +/* pinmux_ops  */

Useless.

...

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

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

Dead code?

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

Dead code?

> +       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? What about the GPIO library API that
does some additional stuff?

> +}

...

> +/* pinconf_ops */

Useless.

...

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

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

...

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

> +       return 0;
> +}

...

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

Dead code?

...

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

devm_platform_ioremap_resource();

> +       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()?

...

> +       girq->default_type = IRQ_TYPE_NONE;

> +       girq->handler = handle_level_irq;

Use ->irq_set_type() for this. Here should be handle_bad_irq().

> +       for (i = 0; i < WPCM450_NUM_GPIO_IRQS; i++) {

> +               int irq = irq_of_parse_and_map(np, i);

fwnode_get_irq()

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

> +       }

...

> +       pr_info("WPCM450 pinctrl and GPIO driver probed\n");

Besides you have to use dev_*() this is completely useless and noisy message.

...

> +static const struct of_device_id wpcm450_pinctrl_match[] = {
> +       { .compatible = "nuvoton,wpcm450-pinctrl" },

> +       { },

Comma is not needed for terminator line.

> +};

...

> +               .suppress_bind_attrs = true,

Why?

-- 
With Best Regards,
Andy Shevchenko