Messages in this thread | | | From | Andy Shevchenko <> | Date | Thu, 14 Jul 2022 18:59:41 +0200 | Subject | Re: [PATCH v2 2/2] pinctrl: nuvoton: add NPCM8XX pinctrl and GPIO driver |
| |
On Thu, Jul 14, 2022 at 2:29 PM Tomer Maimon <tmaimon77@gmail.com> wrote: > > Add pinctrl and GPIO controller driver support to Arbel BMC NPCM8XX SoC. > > Arbel BMC NPCM8XX pinctrl driver based on Poleg NPCM7XX, except the > pin mux mapping difference the NPCM8XX GPIO supports adjust debounce > period time.
...
> +config PINCTRL_NPCM8XX > + bool "Pinctrl and GPIO driver for Nuvoton NPCM8XX"
Why boolean?
> + depends on (ARCH_NPCM || COMPILE_TEST) && OF
I believe the OF is not compile time dependency, hence you may for it as functional one by
depends on (ARCH_NPCM && OF) || COMPILE_TEST
> + 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 Nuvoton NPCM8XX SoC.
Depends on the answer above, this might need an addition on how module will be called.
...
Missed bits.h.
> +#include <linux/device.h> > +#include <linux/gpio/driver.h> > +#include <linux/interrupt.h> > +#include <linux/irq.h> > +#include <linux/mfd/syscon.h> > +#include <linux/module.h>
Missed mod_devicetable.h.
> +#include <linux/of.h> > +#include <linux/of_address.h> > +#include <linux/of_irq.h>
How are these being used?
> +#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> > +#include <linux/platform_device.h>
+ property.h.
> +#include <linux/regmap.h>
...
> +/* GCR registers */ > +#define NPCM8XX_GCR_PDID 0x00 > +#define NPCM8XX_GCR_SRCNT 0x68 > +#define NPCM8XX_GCR_FLOCKR1 0x74 > +#define NPCM8XX_GCR_DSCNT 0x78 > +#define NPCM8XX_GCR_I2CSEGCTL 0xE4 > +#define NPCM8XX_GCR_I2CSEGSEL 0xE0
Format them with the same width, e.g. 0x0E0. And, btw, why capital letters in the numbers?
> +#define NPCM8XX_GCR_MFSEL1 0x260 > +#define NPCM8XX_GCR_MFSEL2 0x264 > +#define NPCM8XX_GCR_MFSEL3 0x268 > +#define NPCM8XX_GCR_MFSEL4 0x26C > +#define NPCM8XX_GCR_MFSEL5 0x270 > +#define NPCM8XX_GCR_MFSEL6 0x274 > +#define NPCM8XX_GCR_MFSEL7 0x278
...
> +/* GPIO registers */
Ditto.
...
> +#define NPCM8XX_DEBOUNCE_NANOSEC 40
_NSEC is enough.
...
> +#define NPCM8XX_DEBOUNCE_VAL_MASK GENMASK(23, 4) > +#define NPCM8XX_DEBOUNCE_MAX_VAL 0xFFFFF7
How MAX_VAL is different from the MASK ?
...
> +struct npcm8xx_gpio { > + void __iomem *base;
> + struct gpio_chip gc;
Making this first member in the structure may reduce the code base at compile time due to pointer arithmetic. You may confirm that by using bloat-o-meter.
> + struct debounce_time debounce; > + int irqbase; > + int irq; > + struct irq_chip irq_chip; > + u32 pinctrl_id; > + int (*direction_input)(struct gpio_chip *chip, unsigned int offset); > + int (*direction_output)(struct gpio_chip *chip, unsigned int offset, > + int value); > + int (*request)(struct gpio_chip *chip, unsigned int offset); > + void (*free)(struct gpio_chip *chip, unsigned int offset); > +};
...
> + val = ioread32(reg) | pinmask; > + iowrite32(val, reg);
With this kind of indentation you may even reduce codebase with
iowrite32(ioread32(reg) | pinmask, reg);
...
> + val = ioread32(reg) & ~pinmask; > + iowrite32(val, reg);
Ditto.
...
> +static void npcmgpio_dbg_show(struct seq_file *s, struct gpio_chip *chip) > +{ > + seq_printf(s, "-- module %d [gpio%d - %d]\n",
Hmm... Isn't pin range is showed in a separate debugfs node?
> +}
...
> + for_each_set_bit(bit, (const void *)&sts, NPCM8XX_GPIO_PER_BANK)
Why this casting?
> + generic_handle_domain_irq(gc->irq.domain, bit);
...
> + unsigned int gpio = BIT(d->hwirq);
There is a special helper to get an H/W IRQ, which is type of irq_hw_number_t IIRC.
...
> + if (type & (IRQ_TYPE_LEVEL_HIGH | IRQ_TYPE_LEVEL_LOW)) {
IRQ_TYPE_LEVEL_MASK
> + npcm_gpio_clr(&bank->gc, bank->base + NPCM8XX_GP_N_EVTYP, gpio); > + irq_set_handler_locked(d, handle_level_irq);
> + } else if (type & (IRQ_TYPE_EDGE_BOTH | IRQ_TYPE_EDGE_RISING > + | IRQ_TYPE_EDGE_FALLING)) {
Why duplicating RISING and FAILING? Isn't it covered by BOTH?
> + npcm_gpio_set(&bank->gc, bank->base + NPCM8XX_GP_N_EVTYP, gpio); > + irq_set_handler_locked(d, handle_edge_irq); > + }
...
> + unsigned int gpio = d->hwirq;
Read the documentation on how the mask()/unmask() has to be implemented (there are examples): https://www.kernel.org/doc/html/latest/driver-api/gpio/driver.html#infrastructure-helpers-for-gpio-irqchips
...
> +/* > + * pin: name, number > + * group: name, npins, pins > + * function: name, ngroups, groups > + */ > +struct npcm8xx_group { > + const char *name; > + const unsigned int *pins; > + int npins; > +};
NIH struct pingroup.
...
Temporary variable here
...reg = base + OSCR;
> + int gpio = BIT(pin % bank->gc.ngpio); > + > + if (pincfg[pin].flag & SLEW) { > + switch (arg) { > + case 0: > + npcm_gpio_clr(&bank->gc, bank->base + NPCM8XX_GP_N_OSRC, > + gpio); > + return 0; > + case 1: > + npcm_gpio_set(&bank->gc, bank->base + NPCM8XX_GP_N_OSRC, > + gpio);
...will save one LoC in this switch-case.
> + return 0; > + default: > + return -EINVAL; > + } > + }
...
> + int gpio = (pin % bank->gc.ngpio);
Too many parentheses.
...
> + u32 ds = 0;
This assignment is redundant, if...
> + flg = pincfg[pin].flag; > + if (flg & DRIVE_STRENGTH_MASK) {
you use traditional pattern, i.e.
if (error_condition) return an_error;
> + val = ioread32(bank->base + NPCM8XX_GP_N_ODSC) & pinmask; > + ds = val ? DSHI(flg) : DSLO(flg); > + dev_dbg(bank->gc.parent, "pin %d strength %d = %d\n", pin, val, ds); > + return ds; > + } > + > + return -EINVAL;
...
> + v = (pincfg[pin].flag & DRIVE_STRENGTH_MASK);
Too many parentheses.
> + if (!nval || !v) > + return -ENOTSUPP; > + if (DSLO(v) == nval) { > + npcm_gpio_clr(&bank->gc, bank->base + NPCM8XX_GP_N_ODSC, gpio); > + return 0; > + } > + if (DSHI(v) == nval) { > + npcm_gpio_set(&bank->gc, bank->base + NPCM8XX_GP_N_ODSC, gpio); > + return 0; > + } > + > + return -ENOTSUPP;
Traditional pattern:
if (LO == nval) clr() else if (HI == nval) set() else return -ENOTSUPP;
return 0;
...
> +static int npcm8xx_gpio_request_enable(struct pinctrl_dev *pctldev, > + struct pinctrl_gpio_range *range, > + unsigned int offset) > +{ > + struct npcm8xx_pinctrl *npcm = pinctrl_dev_get_drvdata(pctldev);
> + if (!range) { > + dev_err(npcm->dev, "invalid range\n"); > + return -EINVAL; > + }
> + if (!range->gc) { > + dev_err(npcm->dev, "invalid gpiochip\n"); > + return -EINVAL; > + }
I'm wondering when you can have one of these triggered.
> + > + npcm8xx_setfunc(npcm->gcr_regmap, &offset, 1, fn_gpio); > + > + return 0; > +}
...
> +static int debounce_timing_setting(struct npcm8xx_gpio *bank, u32 gpio, > + u32 nanosecs) > +{ > + int gpio_debounce = (gpio % 16) * 2; > + u32 dbncp_val, dbncp_val_mod; > + int DBNCS_offset = gpio / 16;
Can you group it together with gpio%16 line? It would be easier for the reader who knows that on some architectures the both assignments may be done in one assembly instruction.
> + int debounce_select;
This logically would be grouped with above int:s.
u32 dbncp_val, dbncp_val_mod; int gpio_debounce = (gpio % 16) * 2; int DBNCS_offset = gpio / 16; int debounce_select;
...
> + npcm_gpio_set(&bank->gc, bank->base + NPCM8XX_GP_N_DBNCS0 + (DBNCS_offset * 4), debounce_select);
> + npcm_gpio_set(&bank->gc, bank->base + NPCM8XX_GP_N_DBNCS0 + (DBNCS_offset * 4), debounce_select);
We can make this line much shorter with help of a temporary variable.
...
> + iowrite32(0x40, bank->base + NPCM8XX_GP_N_DBNCP0 + (i * 4)); > + iowrite32(0x50, bank->base + NPCM8XX_GP_N_DBNCP0 + (i * 4)); > + iowrite32(0x60, bank->base + NPCM8XX_GP_N_DBNCP0 + (i * 4)); > + iowrite32(0x70, bank->base + NPCM8XX_GP_N_DBNCP0 + (i * 4));
And this lines can be shorter with a helper function, but this is up to you.
...
> + dbncp_val_mod = dbncp_val & 0xF;
GENMASK() ? Or (BIT(x) - 1) if it's a limitation by the hardware in bits, this will show it directly (like 4 bits limit).
> + if (dbncp_val_mod > 0x7)
In similar way.
...
> + int ret = 0;
Redundant assignment.
Such assignments in some cases may hide real bugs.
> + if (nanosecs) { > + ret = debounce_timing_setting(bank, pin % bank->gc.ngpio, > + nanosecs); > + if (!ret) {
Why not positive conditional and in this case aka "traditional pattern": if (error) { ...handle error... return error; }
> + npcm_gpio_set(&bank->gc, bank->base + NPCM8XX_GP_N_DBNC, > + gpio); > + } else {
> + dev_info(npcm->dev, "All four debounce timing values are used, please use one of exist debounce values\n"); > + dev_err(npcm->dev, "Pin %d debounce_timing_setting failed, ret=%d\n", pin, ret);
Too much noise in the messages. Create one error message
> + } > + > + return ret; > + } > + > + npcm_gpio_clr(&bank->gc, bank->base + NPCM8XX_GP_N_DBNC, gpio); > + > + return 0;
...
> + if (param == PIN_CONFIG_BIAS_DISABLE) > + rc = (!pu && !pd); > + else if (param == PIN_CONFIG_BIAS_PULL_UP) > + rc = (pu && !pd); > + else if (param == PIN_CONFIG_BIAS_PULL_DOWN) > + rc = (!pu && pd);
In many places (and not only in this function) you are using too many parentheses, why? Can you clean all them up?
...
> +static int npcm8xx_gpio_of(struct npcm8xx_pinctrl *pctrl)
_fw
...
> + char gpioirqname[30];
How 30 was chosen?
...
> + pctrl->gpio_bank[id].gc.label = devm_kasprintf(dev, GFP_KERNEL, "%pfw", child); > + if (!pctrl->gpio_bank[id].gc.label) > + dev_err_probe(dev, -ENOMEM, "No GPIO label %u\n", id);
-ENOMEM doesn't need an error message.
...
> + dev_set_drvdata(&pdev->dev, pctrl);
platform_set_drvdata();
-- With Best Regards, Andy Shevchenko
| |