Received-SPF: pass (google.com: domain of linux-kernel+bounces-122640-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Precedence: bulk
MIME-Version: 1.0
References: <20240313035719.768469-1-ychuang570808@gmail.com> <20240313035719.768469-4-ychuang570808@gmail.com>
In-Reply-To: <20240313035719.768469-4-ychuang570808@gmail.com>
From: Linus Walleij <linus.walleij@linaro.org>
Date: Thu, 28 Mar 2024 10:10:33 +0100
Message-ID: <CACRpkdYnG+SgrgAWW8+qdiBwO5d+nE8g_31Evyw0pA2dXz3BPw@mail.gmail.com>
Subject: Re: [PATCH v6 3/3] pinctrl: nuvoton: Add ma35d1 pinctrl and GPIO driver
To: Jacky Huang <ychuang570808@gmail.com>
Cc: robh+dt@kernel.org, krzysztof.kozlowski+dt@linaro.org, conor+dt@kernel.org, 
	p.zabel@pengutronix.de, j.neuschaefer@gmx.net, 
	linux-arm-kernel@lists.infradead.org, linux-gpio@vger.kernel.org, 
	devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, 
	ychuang3@nuvoton.com, schung@nuvoton.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hi Jacky,

overall this looks very good.

On Wed, Mar 13, 2024 at 4:57=E2=80=AFAM Jacky Huang <ychuang570808@gmail.co=
m> wrote:


> From: Jacky Huang <ychuang3@nuvoton.com>
>
> Add common pinctrl and GPIO driver for Nuvoton MA35 series SoC, and
> add support for ma35d1 pinctrl.
>
> Signed-off-by: Jacky Huang <ychuang3@nuvoton.com>
(...)
> +static int ma35_pinmux_set_mux(struct pinctrl_dev *pctldev, unsigned int=
 selector,
> +                              unsigned int group)
> +{
> +       struct ma35_pinctrl *npctl =3D pinctrl_dev_get_drvdata(pctldev);
> +       struct ma35_pin_group *grp =3D &npctl->groups[group];
> +       struct ma35_pin_setting *setting =3D grp->settings;
> +       u32 i, regval;
> +
> +       dev_dbg(npctl->dev, "enable function %s group %s\n",
> +               npctl->functions[selector].name, npctl->groups[group].nam=
e);
> +
> +       for (i =3D 0; i < grp->npins; i++) {
> +               regmap_read(npctl->regmap, setting->offset, &regval);
> +               regval &=3D ~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 in=
t gpio)
> +{
> +       struct ma35_pin_bank *bank =3D gpiochip_get_data(gc);
> +       void __iomem *reg_mode =3D bank->reg_base + MA35_GP_REG_MODE;
> +       unsigned long flags;
> +       unsigned int regval;
> +
> +       spin_lock_irqsave(&bank->lock, flags);
> +
> +       regval =3D readl(reg_mode);
> +       regval &=3D ~GENMASK(gpio * 2 + 1, gpio * 2);
> +       regval |=3D 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 =3D 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 =3D gpiochip_get_data(irq_data_get_irq=
_chip_data(d));
> +       void __iomem *reg_itype =3D bank->reg_base + MA35_GP_REG_INTTYPE;
> +       void __iomem *reg_ien =3D bank->reg_base + MA35_GP_REG_INTEN;
> +       unsigned int num =3D (d->hwirq);
> +
> +       if (type =3D=3D IRQ_TYPE_PROBE) {
> +               writel(readl(reg_itype) & ~BIT(num), reg_itype);
> +               writel(readl(reg_ien) | BIT(num) | BIT(num + 16), reg_ien=
);
> +               bank->irqtype &=3D ~BIT(num);
> +               bank->irqinten |=3D 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 |=3D BIT(num);
> +               bank->irqinten &=3D ~(BIT(num) | BIT(num + 16));
> +               if (type =3D=3D IRQ_TYPE_LEVEL_HIGH) {
> +                       writel(readl(reg_ien) | BIT(num + 16), reg_ien);
> +                       bank->irqinten |=3D BIT(num + 16);
> +                       return 0;
> +               }
> +
> +               if (type =3D=3D IRQ_TYPE_LEVEL_LOW) {
> +                       writel(readl(reg_ien) | BIT(num), reg_ien);
> +                       bank->irqinten |=3D BIT(num);
> +                       return 0;
> +               }
> +
> +       } else {
> +               writel(readl(reg_itype) & ~BIT(num), reg_itype);
> +               bank->irqtype &=3D ~BIT(num);
> +
> +               if (type & IRQ_TYPE_EDGE_RISING) {
> +                       writel(readl(reg_ien) | BIT(num + 16), reg_ien);
> +                       bank->irqinten |=3D BIT(num + 16);
> +
> +               } else {
> +                       writel(readl(reg_ien) & ~BIT(num + 16), reg_ien);
> +                       bank->irqinten &=3D ~BIT(num + 16);
> +               }
> +
> +               if (type & IRQ_TYPE_EDGE_FALLING) {
> +                       writel(readl(reg_ien) | BIT(num), reg_ien);
> +                       bank->irqinten |=3D BIT(num);
> +
> +               } else {
> +                       writel(readl(reg_ien) & ~BIT(num), reg_ien);
> +                       bank->irqinten &=3D ~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 ma=
35_pinctrl *npctl)
> +{
> +       struct ma35_pin_ctrl *ctrl =3D npctl->ctrl;
> +       struct ma35_pin_bank *bank =3D ctrl->pin_banks;
> +       int ret;
> +       int i;
> +
> +       for (i =3D 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 =3D 0;
> +               bank->irqinten =3D 0;
> +               bank->chip.label =3D bank->name;
> +               bank->chip.of_gpio_n_cells =3D 2;
> +               bank->chip.parent =3D &pdev->dev;
> +               bank->chip.request =3D ma35_gpio_core_to_request;
> +               bank->chip.direction_input =3D ma35_gpio_core_direction_i=
n;
> +               bank->chip.direction_output =3D ma35_gpio_core_direction_=
out;
> +               bank->chip.get =3D ma35_gpio_core_get;
> +               bank->chip.set =3D ma35_gpio_core_set;
> +               bank->chip.base =3D -1;
> +               bank->chip.ngpio =3D bank->nr_pins;
> +               bank->chip.can_sleep =3D false;
> +               spin_lock_init(&bank->lock);
> +
> +               if (bank->irq > 0) {
> +                       struct gpio_irq_chip *girq;
> +
> +                       girq =3D &bank->chip.irq;
> +                       gpio_irq_chip_set_chip(girq, &ma35_gpio_irqchip);
> +                       girq->parent_handler =3D ma35_irq_demux_intgroup;
> +                       girq->num_parents =3D 1;
> +
> +                       girq->parents =3D devm_kcalloc(&pdev->dev, 1, siz=
eof(*girq->parents),
> +                                                    GFP_KERNEL);
> +                       if (!girq->parents)
> +                               return -ENOMEM;
> +
> +                       girq->parents[0] =3D bank->irq;
> +                       girq->default_type =3D IRQ_TYPE_NONE;
> +                       girq->handler =3D 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