Received-SPF: pass (google.com: domain of linux-kernel+bounces-124287-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Message-ID: <c39611b6-25ab-425b-8a16-9f700086e6eb@gmail.com>
Date: Fri, 29 Mar 2024 16:17:40 +0800
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v6 3/3] pinctrl: nuvoton: Add ma35d1 pinctrl and GPIO
 driver
To: Linus Walleij <linus.walleij@linaro.org>
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
References: <20240313035719.768469-1-ychuang570808@gmail.com>
 <20240313035719.768469-4-ychuang570808@gmail.com>
 <CACRpkdYnG+SgrgAWW8+qdiBwO5d+nE8g_31Evyw0pA2dXz3BPw@mail.gmail.com>
Content-Language: en-US
From: Jacky Huang <ychuang570808@gmail.com>
In-Reply-To: <CACRpkdYnG+SgrgAWW8+qdiBwO5d+nE8g_31Evyw0pA2dXz3BPw@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit


Dear Linus,

Thanks for your review.


On 2024/3/28 下午 05:10, Linus Walleij wrote:
> Hi Jacky,
>
> overall this looks very good.
>
> On Wed, Mar 13, 2024 at 4:57 AM Jacky Huang <ychuang570808@gmail.com> 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 = 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, &regval);
>> +               regval &= ~GENMASK(setting->shift + 3, setting->shift);
> Add a comment explaining why you add +3

The pinmux selection is 4 bits. I will use a constant for the bitmask 
width instead.

>
>> +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).

Yes, it is two bits per pin. I will add a comment to explain this.

>> +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?

Yes, it maps one 32-bit register to a gpio pin.
I will add a comment to explain this.

>> +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);

I will add the irq_set_handler_locked().
> 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.

We'll refer to this driver to modify our code.

> Have you checked that handling edge and level IRQs really work
> as expected?

This driver works with edge or level IRQs in linux-5.10, and some 
modifications
have been made for upstream. We'll verify if it also works in linux-6.9.

>> +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

OK, I will fix it.


Best Regards,
Jacky Huang