Received-SPF: pass (google.com: domain of linux-kernel+bounces-86723-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Precedence: bulk
MIME-Version: 1.0
References: <20240220064246.467216-1-yuklin.soo@starfivetech.com> <20240220064246.467216-3-yuklin.soo@starfivetech.com>
In-Reply-To: <20240220064246.467216-3-yuklin.soo@starfivetech.com>
From: Linus Walleij <linus.walleij@linaro.org>
Date: Thu, 29 Feb 2024 14:03:14 +0100
Message-ID: <CACRpkdZd9WuLmvBEjEOF5R+R8Yrva_KiEPRCOXU98yLDkS=+ZQ@mail.gmail.com>
Subject: Re: [RFC PATCH v2 2/6] pinctrl: starfive: jh8100: add main and
 sys_east driver
To: Alex Soo <yuklin.soo@starfivetech.com>
Cc: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>, Hal Feng <hal.feng@starfivetech.com>, 
	Ley Foon Tan <leyfoon.tan@starfivetech.com>, 
	Jianlong Huang <jianlong.huang@starfivetech.com>, Emil Renner Berthing <kernel@esmil.dk>, 
	Rob Herring <robh@kernel.org>, Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>, 
	Conor Dooley <conor+dt@kernel.org>, Drew Fustini <drew@beagleboard.org>, linux-gpio@vger.kernel.org, 
	linux-kernel@vger.kernel.org, devicetree@vger.kernel.org, 
	linux-riscv@lists.infradead.org, Paul Walmsley <paul.walmsley@sifive.com>, 
	Palmer Dabbelt <palmer@dabbelt.com>, Albert Ou <aou@eecs.berkeley.edu>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Thanks Alex,

this new version is much improved!

On Tue, Feb 20, 2024 at 7:43=E2=80=AFAM Alex Soo <yuklin.soo@starfivetech.c=
om> wrote:

> Add JH8100 pinctrl main and sys_east domain driver.
>
> Signed-off-by: Alex Soo <yuklin.soo@starfivetech.com>

This commit message should at least explain what we are adding here,
that it's a core driver that will be used by all the domains, what the
SoC is etc etc.

> +       select GPIOLIB_IRQCHIP
(...)
> +#include "../core.h"
> +#include "../pinmux.h"
> +#include "../pinconf.h"

Do you really need to poke around in the internals like this?

Please explain for each cross-include *why* you need to do this.

> +++ b/drivers/pinctrl/starfive/pinctrl-starfive-jh8100.c
(...)
> +#include <linux/of_gpio.h>

Never use this include. It is legacy and you should not be using
it. Use <linux/gpio/consumer.h> solely. See comments below.

> +#include <linux/pinctrl/consumer.h>

Why?

> +#include "../core.h"
> +#include "../pinctrl-utils.h"
> +#include "../pinmux.h"
> +#include "../pinconf.h"

Again all this. Explain for each one exactly why you need this.

> +static int jh8100_gpio_irq_setup(struct device *dev, struct jh8100_pinct=
rl *sfp)
> +{
> +       struct device_node *np =3D dev->of_node;
> +       struct gpio_irq_chip *girq =3D &sfp->gc.irq;
> +       struct gpio_desc *gpiod;
> +       struct irq_desc *desc;
> +       irq_hw_number_t hwirq;
> +       int i, ret;
> +       int dir;
> +
> +       if (!girq->domain) {
> +               sfp->irq_domain =3D irq_domain_add_linear(np, sfp->gc.ngp=
io,
> +                                                       &irq_domain_simpl=
e_ops,
> +                                                       sfp);

When would this happen? I don't quite get it.

It looks like a probe order issue or something.

> +       } else {
> +               sfp->irq_domain =3D girq->domain;
> +       }
> +
> +       if (!sfp->irq_domain) {
> +               dev_err(dev, "Couldn't allocate IRQ domain\n");
> +               return -ENXIO;
> +       }
> +
> +       for (i =3D 0; i < sfp->gc.ngpio; i++) {
> +               int virq =3D irq_create_mapping(sfp->irq_domain, i);
> +
> +               irq_set_chip_and_handler(virq, &jh8100_irq_chip,
> +                                        handle_edge_irq);
> +               irq_set_chip_data(virq, &sfp->gc);
> +       }

This duplicates core gpiolib irqchip handling, which you select using
select GPIOLIB_IRQCHIP.

Can you please look into just using the gpiolib irqchip handling?

> +       sfp->wakeup_gpio =3D of_get_named_gpio(np, "wakeup-gpios", 0);

No using <linux/of_gpio.h> please.

Use just <linux/gpio/consumer.h> and something like:

struct gpio_desc *wakeup;
wakeup =3D devm_gpiod_get_optional(dev, "wakeup", GPIOD_IN);

> +       if (gpio_is_valid(sfp->wakeup_gpio)) {
> +               hwirq =3D pin_to_hwirq(sfp);
> +               sfp->wakeup_irq =3D irq_find_mapping(sfp->irq_domain, hwi=
rq);
> +               desc =3D irq_to_desc(sfp->wakeup_irq);

if (wakeup) {
     irq =3D gpiod_to_irq(wakeup);
     .. convert to irq descriptor etc...

Actually: is this wakeup handling something we would like to add
to the gpiolib irqchip so everyone can reuse it?
In gpiochip_add_irqchip()?
At least give it a thought.

Yours,
Linus Walleij