From: Andy Shevchenko <andy.shevchenko@gmail.com> To: Tomer Maimon <tmaimon77@gmail.com> Cc: "Avi Fishman" <avifishman70@gmail.com>, "Tali Perry" <tali.perry1@gmail.com>, "Joel Stanley" <joel@jms.id.au>, "Patrick Venture" <venture@google.com>, "Nancy Yuen" <yuenn@google.com>, "Benjamin Fair" <benjaminfair@google.com>, "Linus Walleij" <linus.walleij@linaro.org>, "Rob Herring" <robh+dt@kernel.org>, "Krzysztof Kozlowski" <krzysztof.kozlowski+dt@linaro.org>, "Jonathan Neuschäfer" <j.neuschaefer@gmx.net>, zhengbin13@huawei.com, "OpenBMC Maillist" <openbmc@lists.ozlabs.org>, "open list:GPIO SUBSYSTEM" <linux-gpio@vger.kernel.org>, "Linux Kernel Mailing List" <linux-kernel@vger.kernel.org>, devicetree <devicetree@vger.kernel.org> Subject: Re: [PATCH v2 2/2] pinctrl: nuvoton: add NPCM8XX pinctrl and GPIO driver Date: Thu, 14 Jul 2022 18:59:41 +0200 [thread overview] Message-ID: <CAHp75Vcd6vATJQoJMh_SQ27ijOpiCjMWuSZ04d2OOnExunveqg@mail.gmail.com> (raw) In-Reply-To: <20220714122322.63663-3-tmaimon77@gmail.com> 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
WARNING: multiple messages have this Message-ID (diff)
From: Andy Shevchenko <andy.shevchenko@gmail.com> To: Tomer Maimon <tmaimon77@gmail.com> Cc: devicetree <devicetree@vger.kernel.org>, "Benjamin Fair" <benjaminfair@google.com>, "open list:GPIO SUBSYSTEM" <linux-gpio@vger.kernel.org>, "Avi Fishman" <avifishman70@gmail.com>, "Patrick Venture" <venture@google.com>, "Linus Walleij" <linus.walleij@linaro.org>, "Jonathan Neuschäfer" <j.neuschaefer@gmx.net>, "Tali Perry" <tali.perry1@gmail.com>, zhengbin13@huawei.com, "Rob Herring" <robh+dt@kernel.org>, "Joel Stanley" <joel@jms.id.au>, "Krzysztof Kozlowski" <krzysztof.kozlowski+dt@linaro.org>, "OpenBMC Maillist" <openbmc@lists.ozlabs.org>, "Linux Kernel Mailing List" <linux-kernel@vger.kernel.org> Subject: Re: [PATCH v2 2/2] pinctrl: nuvoton: add NPCM8XX pinctrl and GPIO driver Date: Thu, 14 Jul 2022 18:59:41 +0200 [thread overview] Message-ID: <CAHp75Vcd6vATJQoJMh_SQ27ijOpiCjMWuSZ04d2OOnExunveqg@mail.gmail.com> (raw) In-Reply-To: <20220714122322.63663-3-tmaimon77@gmail.com> 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
next prev parent reply other threads:[~2022-07-14 17:00 UTC|newest] Thread overview: 34+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-07-14 12:23 [PATCH v2 0/2] pinctrl: nuvoton: add pinmux and GPIO driver for NPCM8XX Tomer Maimon 2022-07-14 12:23 ` Tomer Maimon 2022-07-14 12:23 ` [PATCH v2 1/2] dt-binding: pinctrl: Add NPCM8XX pinctrl and GPIO documentation Tomer Maimon 2022-07-14 12:23 ` Tomer Maimon 2022-07-18 21:10 ` Rob Herring 2022-07-18 21:10 ` Rob Herring 2022-09-18 18:28 ` Tomer Maimon 2022-09-18 18:28 ` Tomer Maimon 2022-09-19 6:56 ` Krzysztof Kozlowski 2022-09-19 14:31 ` Tomer Maimon 2022-09-19 14:31 ` Tomer Maimon 2022-09-19 16:06 ` Krzysztof Kozlowski 2022-09-20 7:59 ` Tomer Maimon 2022-09-20 7:59 ` Tomer Maimon 2022-09-20 8:21 ` Krzysztof Kozlowski 2022-09-20 8:32 ` Tomer Maimon 2022-09-20 8:32 ` Tomer Maimon 2022-09-20 8:47 ` Krzysztof Kozlowski 2022-09-20 9:27 ` Tomer Maimon 2022-09-20 9:27 ` Tomer Maimon 2022-09-20 15:16 ` Krzysztof Kozlowski 2022-09-20 17:00 ` Tomer Maimon 2022-09-20 17:00 ` Tomer Maimon 2022-07-14 12:23 ` [PATCH v2 2/2] pinctrl: nuvoton: add NPCM8XX pinctrl and GPIO driver Tomer Maimon 2022-07-14 12:23 ` Tomer Maimon 2022-07-14 12:25 ` Krzysztof Kozlowski 2022-07-14 13:14 ` Andy Shevchenko 2022-07-14 13:14 ` Andy Shevchenko 2022-07-14 16:59 ` Andy Shevchenko [this message] 2022-07-14 16:59 ` Andy Shevchenko 2022-07-18 1:24 ` kernel test robot 2022-07-18 1:24 ` kernel test robot 2022-07-16 16:21 kernel test robot 2022-07-18 13:41 kernel test robot
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=CAHp75Vcd6vATJQoJMh_SQ27ijOpiCjMWuSZ04d2OOnExunveqg@mail.gmail.com \ --to=andy.shevchenko@gmail.com \ --cc=avifishman70@gmail.com \ --cc=benjaminfair@google.com \ --cc=devicetree@vger.kernel.org \ --cc=j.neuschaefer@gmx.net \ --cc=joel@jms.id.au \ --cc=krzysztof.kozlowski+dt@linaro.org \ --cc=linus.walleij@linaro.org \ --cc=linux-gpio@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=openbmc@lists.ozlabs.org \ --cc=robh+dt@kernel.org \ --cc=tali.perry1@gmail.com \ --cc=tmaimon77@gmail.com \ --cc=venture@google.com \ --cc=yuenn@google.com \ --cc=zhengbin13@huawei.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.