Messages in this thread | | | From | Andy Shevchenko <> | Date | Wed, 2 Jun 2021 15:50:39 +0300 | Subject | Re: [PATCH 5/8] pinctrl: nuvoton: Add driver for WPCM450 |
| |
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
| |