| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CACRpkdYnG+SgrgAWW8+qdiBwO5d+nE8g_31Evyw0pA2dXz3BPw@mail.gmail.com>
Date: Thu, 28 Mar 2024 10:10:33 +0100
From: Linus Walleij <linus.walleij@...aro.org>
To: Jacky Huang <ychuang570808@...il.com>
Cc: robh+dt@...nel.org, krzysztof.kozlowski+dt@...aro.org, conor+dt@...nel.org,
p.zabel@...gutronix.de, j.neuschaefer@....net,
linux-arm-kernel@...ts.infradead.org, linux-gpio@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
ychuang3@...oton.com, schung@...oton.com
Subject: Re: [PATCH v6 3/3] pinctrl: nuvoton: Add ma35d1 pinctrl and GPIO driver
Hi Jacky,
overall this looks very good.
On Wed, Mar 13, 2024 at 4:57 AM Jacky Huang <ychuang570808@...il.com> wrote:
> From: Jacky Huang <ychuang3@...oton.com>
>
> Add common pinctrl and GPIO driver for Nuvoton MA35 series SoC, and
> add support for ma35d1 pinctrl.
>
> Signed-off-by: Jacky Huang <ychuang3@...oton.com>
(...)
> +static int ma35_pinmux_set_mux(struct pinctrl_dev *pctldev, unsigned int selector,
> + unsigned int group)
> +{
> + struct ma35_pinctrl *npctl = pinctrl_dev_get_drvdata(pctldev);
> + struct ma35_pin_group *grp = &npctl->groups[group];
> + struct ma35_pin_setting *setting = grp->settings;
> + u32 i, regval;
> +
> + dev_dbg(npctl->dev, "enable function %s group %s\n",
> + npctl->functions[selector].name, npctl->groups[group].name);
> +
> + for (i = 0; i < grp->npins; i++) {
> + regmap_read(npctl->regmap, setting->offset, ®val);
> + regval &= ~GENMASK(setting->shift + 3, setting->shift);
Add a comment explaining why you add +3
> +static int ma35_gpio_core_direction_in(struct gpio_chip *gc, unsigned int gpio)
> +{
> + struct ma35_pin_bank *bank = gpiochip_get_data(gc);
> + void __iomem *reg_mode = bank->reg_base + MA35_GP_REG_MODE;
> + unsigned long flags;
> + unsigned int regval;
> +
> + spin_lock_irqsave(&bank->lock, flags);
> +
> + regval = readl(reg_mode);
> + regval &= ~GENMASK(gpio * 2 + 1, gpio * 2);
> + regval |= MA35_GP_MODE_INPUT << gpio * 2;
Here the first time you do this magic explain in a comment why you
use *2+1 and *2 overall (I guess two bits per line).
> +static int ma35_gpio_core_get(struct gpio_chip *gc, unsigned int gpio)
> +{
> + struct ma35_pin_bank *bank = gpiochip_get_data(gc);
> +
> + return readl(bank->reg_base + MA35_PIN_MAP_BASE + gpio * 4);
Here add a comment explaining the *4
I guess one 32-bit register per pin?
> +static int ma35_irq_irqtype(struct irq_data *d, unsigned int type)
> +{
> + struct ma35_pin_bank *bank = gpiochip_get_data(irq_data_get_irq_chip_data(d));
> + void __iomem *reg_itype = bank->reg_base + MA35_GP_REG_INTTYPE;
> + void __iomem *reg_ien = bank->reg_base + MA35_GP_REG_INTEN;
> + unsigned int num = (d->hwirq);
> +
> + if (type == IRQ_TYPE_PROBE) {
> + writel(readl(reg_itype) & ~BIT(num), reg_itype);
> + writel(readl(reg_ien) | BIT(num) | BIT(num + 16), reg_ien);
> + bank->irqtype &= ~BIT(num);
> + bank->irqinten |= BIT(num) | BIT(num + 16);
> + return 0;
> + }
> +
> + if (type & IRQ_TYPE_LEVEL_MASK) {
> + writel(readl(reg_itype) | BIT(num), reg_itype);
> + writel(readl(reg_ien) & ~(BIT(num) | BIT(num + 16)), reg_ien);
> + bank->irqtype |= BIT(num);
> + bank->irqinten &= ~(BIT(num) | BIT(num + 16));
> + if (type == IRQ_TYPE_LEVEL_HIGH) {
> + writel(readl(reg_ien) | BIT(num + 16), reg_ien);
> + bank->irqinten |= BIT(num + 16);
> + return 0;
> + }
> +
> + if (type == IRQ_TYPE_LEVEL_LOW) {
> + writel(readl(reg_ien) | BIT(num), reg_ien);
> + bank->irqinten |= BIT(num);
> + return 0;
> + }
> +
> + } else {
> + writel(readl(reg_itype) & ~BIT(num), reg_itype);
> + bank->irqtype &= ~BIT(num);
> +
> + if (type & IRQ_TYPE_EDGE_RISING) {
> + writel(readl(reg_ien) | BIT(num + 16), reg_ien);
> + bank->irqinten |= BIT(num + 16);
> +
> + } else {
> + writel(readl(reg_ien) & ~BIT(num + 16), reg_ien);
> + bank->irqinten &= ~BIT(num + 16);
> + }
> +
> + if (type & IRQ_TYPE_EDGE_FALLING) {
> + writel(readl(reg_ien) | BIT(num), reg_ien);
> + bank->irqinten |= BIT(num);
> +
> + } else {
> + writel(readl(reg_ien) & ~BIT(num), reg_ien);
> + bank->irqinten &= ~BIT(num);
> + }
> + }
> + return 0;
> +}
I don't understand why you don't set the irq_handler:
irq_set_handler_locked(d, handle_edge_irq);
irq_set_handler_locked(d, handle_level_irq);
It seems you are not handling IRQ_TYPE_EDGE_BOTH?
What happens if both rising and falling is specified simultaneously?
The if/else nesting is hard to read.
switch (type) {
case IRQ_TYPE_EDGE_BOTH:
(...)
case IRQ_TYPE_EDGE_RISING:
(...)
See drivers/gpio/gpio-ftgpio010.c for an example.
Have you checked that handling edge and level IRQs really work
as expected?
> +static int ma35_gpiolib_register(struct platform_device *pdev, struct ma35_pinctrl *npctl)
> +{
> + struct ma35_pin_ctrl *ctrl = npctl->ctrl;
> + struct ma35_pin_bank *bank = ctrl->pin_banks;
> + int ret;
> + int i;
> +
> + for (i = 0; i < ctrl->nr_banks; ++i, ++bank) {
> + if (!bank->valid) {
> + dev_warn(&pdev->dev, "bank %s is not valid\n",
> + bank->np->name);
> + continue;
> + }
> + bank->irqtype = 0;
> + bank->irqinten = 0;
> + bank->chip.label = bank->name;
> + bank->chip.of_gpio_n_cells = 2;
> + bank->chip.parent = &pdev->dev;
> + bank->chip.request = ma35_gpio_core_to_request;
> + bank->chip.direction_input = ma35_gpio_core_direction_in;
> + bank->chip.direction_output = ma35_gpio_core_direction_out;
> + bank->chip.get = ma35_gpio_core_get;
> + bank->chip.set = ma35_gpio_core_set;
> + bank->chip.base = -1;
> + bank->chip.ngpio = bank->nr_pins;
> + bank->chip.can_sleep = false;
> + spin_lock_init(&bank->lock);
> +
> + if (bank->irq > 0) {
> + struct gpio_irq_chip *girq;
> +
> + girq = &bank->chip.irq;
> + gpio_irq_chip_set_chip(girq, &ma35_gpio_irqchip);
> + girq->parent_handler = ma35_irq_demux_intgroup;
> + girq->num_parents = 1;
> +
> + girq->parents = devm_kcalloc(&pdev->dev, 1, sizeof(*girq->parents),
> + GFP_KERNEL);
> + if (!girq->parents)
> + return -ENOMEM;
> +
> + girq->parents[0] = bank->irq;
> + girq->default_type = IRQ_TYPE_NONE;
> + girq->handler = handle_level_irq;
Does this really work for the edge IRQs?
I recommend setting this to handle_bad_irq and assign the right
handler in .set_type().
Yours,
Linus Walleij
Powered by blists - more mailing lists