On Thu, Jul 14, 2022 at 2:29 PM Tomer Maimon <tmaimon77@xxxxxxxxx> 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
