linux-kernel - Re: [RFC PATCH v2 2/6] pinctrl: starfive: jh8100: add main and sys

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CACRpkdZd9WuLmvBEjEOF5R+R8Yrva_KiEPRCOXU98yLDkS=+ZQ@mail.gmail.com>
Date: Thu, 29 Feb 2024 14:03:14 +0100
From: Linus Walleij <linus.walleij@...aro.org>
To: Alex Soo <yuklin.soo@...rfivetech.com>
Cc: Bartosz Golaszewski <bartosz.golaszewski@...aro.org>, Hal Feng <hal.feng@...rfivetech.com>, 
	Ley Foon Tan <leyfoon.tan@...rfivetech.com>, 
	Jianlong Huang <jianlong.huang@...rfivetech.com>, Emil Renner Berthing <kernel@...il.dk>, 
	Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>, 
	Conor Dooley <conor+dt@...nel.org>, Drew Fustini <drew@...gleboard.org>, linux-gpio@...r.kernel.org, 
	linux-kernel@...r.kernel.org, devicetree@...r.kernel.org, 
	linux-riscv@...ts.infradead.org, Paul Walmsley <paul.walmsley@...ive.com>, 
	Palmer Dabbelt <palmer@...belt.com>, Albert Ou <aou@...s.berkeley.edu>
Subject: Re: [RFC PATCH v2 2/6] pinctrl: starfive: jh8100: add main and
 sys_east driver

Thanks Alex,

this new version is much improved!

On Tue, Feb 20, 2024 at 7:43 AM Alex Soo <yuklin.soo@...rfivetech.com> wrote:

> Add JH8100 pinctrl main and sys_east domain driver.
>
> Signed-off-by: Alex Soo <yuklin.soo@...rfivetech.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_pinctrl *sfp)
> +{
> +       struct device_node *np = dev->of_node;
> +       struct gpio_irq_chip *girq = &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 = irq_domain_add_linear(np, sfp->gc.ngpio,
> +                                                       &irq_domain_simple_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 = girq->domain;
> +       }
> +
> +       if (!sfp->irq_domain) {
> +               dev_err(dev, "Couldn't allocate IRQ domain\n");
> +               return -ENXIO;
> +       }
> +
> +       for (i = 0; i < sfp->gc.ngpio; i++) {
> +               int virq = 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 = 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 = devm_gpiod_get_optional(dev, "wakeup", GPIOD_IN);

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

if (wakeup) {
     irq = 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