linux-kernel - Re: [PATCH 3/5] gpiolib: add gpiochip_add_pinlist

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: <CACRpkdZi-KP2SK4w_Pj_AjQmcSM2miTpAYb7UzWTxtCoqpquuw@mail.gmail.com>
Date: Fri, 20 Dec 2024 13:31:06 +0100
From: Linus Walleij <linus.walleij@...aro.org>
To: Thomas Richard <thomas.richard@...tlin.com>
Cc: Bartosz Golaszewski <brgl@...ev.pl>, Lee Jones <lee@...nel.org>, Pavel Machek <pavel@....cz>, 
	linux-gpio@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-leds@...r.kernel.org, thomas.petazzoni@...tlin.com, 
	DanieleCleri@...on.eu, GaryWang@...on.com.tw
Subject: Re: [PATCH 3/5] gpiolib: add gpiochip_add_pinlist_range() function

Hi Thomas,

thanks for your patch!

On Wed, Dec 11, 2024 at 5:27 PM Thomas Richard
<thomas.richard@...tlin.com> wrote:

> Add gpiochip_add_pinlist_range() function to add a range for GPIO <-> pin
> mapping, using a list of non consecutive pins.
> Previously, it was only possible to add range of consecutive pins using
> gpiochip_add_pin_range().
>
> The struct pinctrl_gpio_range has a 'pins' member which allows to set a
> list of pins (which can be non consecutive). gpiochip_add_pinlist_range()
> is identical to gpiochip_add_pin_range(), except it set 'pins' member
> instead of 'pin_base' member.
>
> Signed-off-by: Thomas Richard <thomas.richard@...tlin.com>

(...)
> -int gpiochip_add_pin_range(struct gpio_chip *gc, const char *pinctl_name,
> -                          unsigned int gpio_offset, unsigned int pin_offset,
> -                          unsigned int npins)
> +static int __gpiochip_add_pin_range(struct gpio_chip *gc, const char *pinctl_name,
> +                                   unsigned int gpio_offset, unsigned int pin_offset,
> +                                   unsigned int const *pins, unsigned int npins)

Uh this looks messy and I'm not a fan, sadly.

Overall I dislike __inner_function() syntax, so use some name that
describe what is going on please, but I don't think we wanna do
this at all.

Instead of this hack that start to soften the boundary between GPIO
and pin control drivers, I think it is better to do what we often do
and move the whole GPIO driver over into the pin control driver
and have it all inside one single file, since I suspect the hardware
is supposed to be used as one single unit.

Please look at other combined GPIO+pin control drivers
for inspiration, such as:
pinctrl-stmfx.c
pinctrl-sx150x.c

those just access any registers they need as they are in one
driver, but still maintains the separation by just calling the
existing gpiochip_add_pin_range() and be done with it.

This should remove your need for the strange hacks like this
patch and the gpiochip wrapper in the pin control driver.

Yours,
Linus Walleij