linux-kernel - Re: [PATCH v3 2/3] gpio: spacemit: add support for K1 SoC

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 for Android: free password hash cracker in your pocket

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

Message-ID: <CACRpkdYgGho=VQabonq4HccEiXBH2qM76K45oDaV1Jyi0xZ-YA@mail.gmail.com>
Date: Fri, 27 Dec 2024 17:28:53 +0100
From: Linus Walleij <linus.walleij@...aro.org>
To: Yixun Lan <dlan@...too.org>
Cc: Bartosz Golaszewski <brgl@...ev.pl>, Rob Herring <robh@...nel.org>, 
	Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, 
	Conor Dooley <conor@...nel.org>, Paul Walmsley <paul.walmsley@...ive.com>, 
	Palmer Dabbelt <palmer@...belt.com>, Yangyu Chen <cyy@...self.name>, 
	Jisheng Zhang <jszhang@...nel.org>, Jesse Taube <mr.bossman075@...il.com>, 
	Inochi Amaoto <inochiama@...look.com>, Icenowy Zheng <uwu@...nowy.me>, 
	Meng Zhang <zhangmeng.kevin@...ux.spacemit.com>, linux-gpio@...r.kernel.org, 
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-riscv@...ts.infradead.org
Subject: Re: [PATCH v3 2/3] gpio: spacemit: add support for K1 SoC

Hi Yixun,

thanks for your patch!

Some comments below:

On Wed, Dec 25, 2024 at 1:33 AM Yixun Lan <dlan@...too.org> wrote:

> Implement GPIO functionality which capable of setting pin as
> input, output. Also, each pin can be used as interrupt which
> support rising/failing edge type trigger, or both.
>
> Signed-off-by: Yixun Lan <dlan@...too.org>
(...)
> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/init.h>
> +#include <linux/irq.h>
> +#include <linux/interrupt.h>
> +#include <linux/clk.h>
> +#include <linux/gpio.h>

Please drop this legacy include. It should not be needed.

> +#include <linux/gpio/driver.h>

This should be enough for any driver.

> +/* register offset */
> +#define GPLR           0x00
> +#define GPDR           0x0c
> +#define GPSR           0x18
> +#define GPCR           0x24
> +#define GRER           0x30
> +#define GFER           0x3c
> +#define GEDR           0x48
> +#define GSDR           0x54
> +#define GCDR           0x60
> +#define GSRER          0x6c
> +#define GCRER          0x78
> +#define GSFER          0x84
> +#define GCFER          0x90
> +#define GAPMASK                0x9c
> +#define GCPMASK                0xa8

This looks like the archetype of a memory-mapped GPIO chip.

> +#define spacemit_gpio_to_bank_idx(gpio)                ((gpio) / K1_BANK_GPIO_NUMBER)
> +#define spacemit_gpio_to_bank_offset(gpio)     ((gpio) & BANK_GPIO_MASK)
> +#define spacemit_bank_to_gpio(idx, offset)     \
> +       (((idx) * K1_BANK_GPIO_NUMBER) | ((offset) & BANK_GPIO_MASK))
> +
> +static u32 k1_gpio_bank_offset[] = { 0x0, 0x4, 0x8, 0x100 };

Yet this is not registered on a per-bank basis, instead all four banks
are hammered into one single chip. Why?

Can you please split this into 4 instances, also in the device
tree. It makes everything much simpler.

> +struct spacemit_gpio_chip {
> +       struct gpio_chip                chip;
> +       struct irq_domain               *domain;

If you're using GPIOLIB_IRQCHIP you should not
also define youe own irq_domain, the gpiolib handles this.

> +       struct spacemit_gpio_bank       *banks;
> +       void __iomem                    *reg_base;
> +       unsigned int                    ngpio;

Is this ever used after initialization?

> +       unsigned int                    nbank;
> +       int                             irq;

Or this?

> +static int spacemit_gpio_to_irq(struct gpio_chip *chip, unsigned int offset)
> +{
> +       struct spacemit_gpio_chip *schip = to_spacemit_gpio_chip(chip);
> +
> +       return irq_create_mapping(schip->domain, offset);
> +}

Should not be needed when using GPIOLIB_IRQCHIP.

> +       schip = to_spacemit_gpio_chip(chip);
> +       bank = spacemit_gpio_get_bank(schip, offset);
> +       bit = BIT(spacemit_gpio_to_bank_offset(offset));

#include <linux/bits.h> to use the BIT() macro.

This driver becomes unnecessarily complex due to collecting 4 GPIO chips
into one.

Please split it into 4 instances of the same driver, then look at other
drivers using select GPIO_GENERIC such as
drivers/gpio/gpio-pl061.c
drivers/gpio/gpio-ftgpio010.c
for examples of how to create a very compact and simple driver
reusing the generic GPIO helpers, this will also provide
get/set_multiple implementations and other useful things.

Yours,
Linus Walleij