Re: [PATCH 5/8] pinctrl: nuvoton: Add driver for WPCM450
From: Andy Shevchenko
Date: Wed Jun 02 2021 - 08:51:18 EST
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?
> + 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