linux-kernel - Re: [PATCH v2 1/1] gpio-f7188x: fix base values conflicts with other gpio pins

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]
Date:   Mon, 29 May 2023 14:26:29 +0200
From:   simon.guinot@...uanux.org
To:     xingtong_wu@....com
Cc:     linus.walleij@...aro.org, brgl@...ev.pl,
        linux-gpio@...r.kernel.org, linux-kernel@...r.kernel.org,
        henning.schild@...mens.com, xingtong.wu@...mens.com
Subject: Re: [PATCH v2 1/1] gpio-f7188x: fix base values conflicts with other
 gpio pins

On Mon, May 29, 2023 at 10:50:12AM +0800, xingtong_wu@....com wrote:
> From: "xingtong.wu" <xingtong.wu@...mens.com>
> 
> switch pin base from static to automatic allocation to
> avoid conflicts and align with other gpio chip drivers

Hi xingtong,

I suppose this patch is correct but I am not a big fan.

It would be nice if a pin number found in the device datasheet could
still be converted into a Linux GPIO number by adding the base of the
first bank.

With this patch it is no longer possible. All the F7188x controllers
have holes in their pin namespace (between the banks/chips). And a base
number assigned dynamically to each chip won't take these holes into
account.

Simon

> 
> Signed-off-by: xingtong.wu <xingtong.wu@...mens.com>
> ---
>  drivers/gpio/gpio-f7188x.c | 138 ++++++++++++++++++-------------------
>  1 file changed, 69 insertions(+), 69 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-f7188x.c b/drivers/gpio/gpio-f7188x.c
> index f54ca5a1775e..3875fd940ccb 100644
> --- a/drivers/gpio/gpio-f7188x.c
> +++ b/drivers/gpio/gpio-f7188x.c
> @@ -163,7 +163,7 @@ static void f7188x_gpio_set(struct gpio_chip *chip, unsigned offset, int value);
>  static int f7188x_gpio_set_config(struct gpio_chip *chip, unsigned offset,
>  				  unsigned long config);
>  
> -#define F7188X_GPIO_BANK(_base, _ngpio, _regbase, _label)			\
> +#define F7188X_GPIO_BANK(_ngpio, _regbase, _label)			\
>  	{								\
>  		.chip = {						\
>  			.label            = _label,			\
> @@ -174,7 +174,7 @@ static int f7188x_gpio_set_config(struct gpio_chip *chip, unsigned offset,
>  			.direction_output = f7188x_gpio_direction_out,	\
>  			.set              = f7188x_gpio_set,		\
>  			.set_config	  = f7188x_gpio_set_config,	\
> -			.base             = _base,			\
> +			.base             = -1,				\
>  			.ngpio            = _ngpio,			\
>  			.can_sleep        = true,			\
>  		},							\
> @@ -191,98 +191,98 @@ static int f7188x_gpio_set_config(struct gpio_chip *chip, unsigned offset,
>  #define f7188x_gpio_data_single(type)	((type) == nct6126d)
>  
>  static struct f7188x_gpio_bank f71869_gpio_bank[] = {
> -	F7188X_GPIO_BANK(0, 6, 0xF0, DRVNAME "-0"),
> -	F7188X_GPIO_BANK(10, 8, 0xE0, DRVNAME "-1"),
> -	F7188X_GPIO_BANK(20, 8, 0xD0, DRVNAME "-2"),
> -	F7188X_GPIO_BANK(30, 8, 0xC0, DRVNAME "-3"),
> -	F7188X_GPIO_BANK(40, 8, 0xB0, DRVNAME "-4"),
> -	F7188X_GPIO_BANK(50, 5, 0xA0, DRVNAME "-5"),
> -	F7188X_GPIO_BANK(60, 6, 0x90, DRVNAME "-6"),
> +	F7188X_GPIO_BANK(6, 0xF0, DRVNAME "-0"),
> +	F7188X_GPIO_BANK(8, 0xE0, DRVNAME "-1"),
> +	F7188X_GPIO_BANK(8, 0xD0, DRVNAME "-2"),
> +	F7188X_GPIO_BANK(8, 0xC0, DRVNAME "-3"),
> +	F7188X_GPIO_BANK(8, 0xB0, DRVNAME "-4"),
> +	F7188X_GPIO_BANK(5, 0xA0, DRVNAME "-5"),
> +	F7188X_GPIO_BANK(6, 0x90, DRVNAME "-6"),
>  };
>  
>  static struct f7188x_gpio_bank f71869a_gpio_bank[] = {
> -	F7188X_GPIO_BANK(0, 6, 0xF0, DRVNAME "-0"),
> -	F7188X_GPIO_BANK(10, 8, 0xE0, DRVNAME "-1"),
> -	F7188X_GPIO_BANK(20, 8, 0xD0, DRVNAME "-2"),
> -	F7188X_GPIO_BANK(30, 8, 0xC0, DRVNAME "-3"),
> -	F7188X_GPIO_BANK(40, 8, 0xB0, DRVNAME "-4"),
> -	F7188X_GPIO_BANK(50, 5, 0xA0, DRVNAME "-5"),
> -	F7188X_GPIO_BANK(60, 8, 0x90, DRVNAME "-6"),
> -	F7188X_GPIO_BANK(70, 8, 0x80, DRVNAME "-7"),
> +	F7188X_GPIO_BANK(6, 0xF0, DRVNAME "-0"),
> +	F7188X_GPIO_BANK(8, 0xE0, DRVNAME "-1"),
> +	F7188X_GPIO_BANK(8, 0xD0, DRVNAME "-2"),
> +	F7188X_GPIO_BANK(8, 0xC0, DRVNAME "-3"),
> +	F7188X_GPIO_BANK(8, 0xB0, DRVNAME "-4"),
> +	F7188X_GPIO_BANK(5, 0xA0, DRVNAME "-5"),
> +	F7188X_GPIO_BANK(8, 0x90, DRVNAME "-6"),
> +	F7188X_GPIO_BANK(8, 0x80, DRVNAME "-7"),
>  };
>  
>  static struct f7188x_gpio_bank f71882_gpio_bank[] = {
> -	F7188X_GPIO_BANK(0, 8, 0xF0, DRVNAME "-0"),
> -	F7188X_GPIO_BANK(10, 8, 0xE0, DRVNAME "-1"),
> -	F7188X_GPIO_BANK(20, 8, 0xD0, DRVNAME "-2"),
> -	F7188X_GPIO_BANK(30, 4, 0xC0, DRVNAME "-3"),
> -	F7188X_GPIO_BANK(40, 4, 0xB0, DRVNAME "-4"),
> +	F7188X_GPIO_BANK(8, 0xF0, DRVNAME "-0"),
> +	F7188X_GPIO_BANK(8, 0xE0, DRVNAME "-1"),
> +	F7188X_GPIO_BANK(8, 0xD0, DRVNAME "-2"),
> +	F7188X_GPIO_BANK(4, 0xC0, DRVNAME "-3"),
> +	F7188X_GPIO_BANK(4, 0xB0, DRVNAME "-4"),
>  };
>  
>  static struct f7188x_gpio_bank f71889a_gpio_bank[] = {
> -	F7188X_GPIO_BANK(0, 7, 0xF0, DRVNAME "-0"),
> -	F7188X_GPIO_BANK(10, 7, 0xE0, DRVNAME "-1"),
> -	F7188X_GPIO_BANK(20, 8, 0xD0, DRVNAME "-2"),
> -	F7188X_GPIO_BANK(30, 8, 0xC0, DRVNAME "-3"),
> -	F7188X_GPIO_BANK(40, 8, 0xB0, DRVNAME "-4"),
> -	F7188X_GPIO_BANK(50, 5, 0xA0, DRVNAME "-5"),
> -	F7188X_GPIO_BANK(60, 8, 0x90, DRVNAME "-6"),
> -	F7188X_GPIO_BANK(70, 8, 0x80, DRVNAME "-7"),
> +	F7188X_GPIO_BANK(7, 0xF0, DRVNAME "-0"),
> +	F7188X_GPIO_BANK(7, 0xE0, DRVNAME "-1"),
> +	F7188X_GPIO_BANK(8, 0xD0, DRVNAME "-2"),
> +	F7188X_GPIO_BANK(8, 0xC0, DRVNAME "-3"),
> +	F7188X_GPIO_BANK(8, 0xB0, DRVNAME "-4"),
> +	F7188X_GPIO_BANK(5, 0xA0, DRVNAME "-5"),
> +	F7188X_GPIO_BANK(8, 0x90, DRVNAME "-6"),
> +	F7188X_GPIO_BANK(8, 0x80, DRVNAME "-7"),
>  };
>  
>  static struct f7188x_gpio_bank f71889_gpio_bank[] = {
> -	F7188X_GPIO_BANK(0, 7, 0xF0, DRVNAME "-0"),
> -	F7188X_GPIO_BANK(10, 7, 0xE0, DRVNAME "-1"),
> -	F7188X_GPIO_BANK(20, 8, 0xD0, DRVNAME "-2"),
> -	F7188X_GPIO_BANK(30, 8, 0xC0, DRVNAME "-3"),
> -	F7188X_GPIO_BANK(40, 8, 0xB0, DRVNAME "-4"),
> -	F7188X_GPIO_BANK(50, 5, 0xA0, DRVNAME "-5"),
> -	F7188X_GPIO_BANK(60, 8, 0x90, DRVNAME "-6"),
> -	F7188X_GPIO_BANK(70, 8, 0x80, DRVNAME "-7"),
> +	F7188X_GPIO_BANK(7, 0xF0, DRVNAME "-0"),
> +	F7188X_GPIO_BANK(7, 0xE0, DRVNAME "-1"),
> +	F7188X_GPIO_BANK(8, 0xD0, DRVNAME "-2"),
> +	F7188X_GPIO_BANK(8, 0xC0, DRVNAME "-3"),
> +	F7188X_GPIO_BANK(8, 0xB0, DRVNAME "-4"),
> +	F7188X_GPIO_BANK(5, 0xA0, DRVNAME "-5"),
> +	F7188X_GPIO_BANK(8, 0x90, DRVNAME "-6"),
> +	F7188X_GPIO_BANK(8, 0x80, DRVNAME "-7"),
>  };
>  
>  static struct f7188x_gpio_bank f81866_gpio_bank[] = {
> -	F7188X_GPIO_BANK(0, 8, 0xF0, DRVNAME "-0"),
> -	F7188X_GPIO_BANK(10, 8, 0xE0, DRVNAME "-1"),
> -	F7188X_GPIO_BANK(20, 8, 0xD0, DRVNAME "-2"),
> -	F7188X_GPIO_BANK(30, 8, 0xC0, DRVNAME "-3"),
> -	F7188X_GPIO_BANK(40, 8, 0xB0, DRVNAME "-4"),
> -	F7188X_GPIO_BANK(50, 8, 0xA0, DRVNAME "-5"),
> -	F7188X_GPIO_BANK(60, 8, 0x90, DRVNAME "-6"),
> -	F7188X_GPIO_BANK(70, 8, 0x80, DRVNAME "-7"),
> -	F7188X_GPIO_BANK(80, 8, 0x88, DRVNAME "-8"),
> +	F7188X_GPIO_BANK(8, 0xF0, DRVNAME "-0"),
> +	F7188X_GPIO_BANK(8, 0xE0, DRVNAME "-1"),
> +	F7188X_GPIO_BANK(8, 0xD0, DRVNAME "-2"),
> +	F7188X_GPIO_BANK(8, 0xC0, DRVNAME "-3"),
> +	F7188X_GPIO_BANK(8, 0xB0, DRVNAME "-4"),
> +	F7188X_GPIO_BANK(8, 0xA0, DRVNAME "-5"),
> +	F7188X_GPIO_BANK(8, 0x90, DRVNAME "-6"),
> +	F7188X_GPIO_BANK(8, 0x80, DRVNAME "-7"),
> +	F7188X_GPIO_BANK(8, 0x88, DRVNAME "-8"),
>  };
>  
>  
>  static struct f7188x_gpio_bank f81804_gpio_bank[] = {
> -	F7188X_GPIO_BANK(0, 8, 0xF0, DRVNAME "-0"),
> -	F7188X_GPIO_BANK(10, 8, 0xE0, DRVNAME "-1"),
> -	F7188X_GPIO_BANK(20, 8, 0xD0, DRVNAME "-2"),
> -	F7188X_GPIO_BANK(50, 8, 0xA0, DRVNAME "-3"),
> -	F7188X_GPIO_BANK(60, 8, 0x90, DRVNAME "-4"),
> -	F7188X_GPIO_BANK(70, 8, 0x80, DRVNAME "-5"),
> -	F7188X_GPIO_BANK(90, 8, 0x98, DRVNAME "-6"),
> +	F7188X_GPIO_BANK(8, 0xF0, DRVNAME "-0"),
> +	F7188X_GPIO_BANK(8, 0xE0, DRVNAME "-1"),
> +	F7188X_GPIO_BANK(8, 0xD0, DRVNAME "-2"),
> +	F7188X_GPIO_BANK(8, 0xA0, DRVNAME "-3"),
> +	F7188X_GPIO_BANK(8, 0x90, DRVNAME "-4"),
> +	F7188X_GPIO_BANK(8, 0x80, DRVNAME "-5"),
> +	F7188X_GPIO_BANK(8, 0x98, DRVNAME "-6"),
>  };
>  
>  static struct f7188x_gpio_bank f81865_gpio_bank[] = {
> -	F7188X_GPIO_BANK(0, 8, 0xF0, DRVNAME "-0"),
> -	F7188X_GPIO_BANK(10, 8, 0xE0, DRVNAME "-1"),
> -	F7188X_GPIO_BANK(20, 8, 0xD0, DRVNAME "-2"),
> -	F7188X_GPIO_BANK(30, 8, 0xC0, DRVNAME "-3"),
> -	F7188X_GPIO_BANK(40, 8, 0xB0, DRVNAME "-4"),
> -	F7188X_GPIO_BANK(50, 8, 0xA0, DRVNAME "-5"),
> -	F7188X_GPIO_BANK(60, 5, 0x90, DRVNAME "-6"),
> +	F7188X_GPIO_BANK(8, 0xF0, DRVNAME "-0"),
> +	F7188X_GPIO_BANK(8, 0xE0, DRVNAME "-1"),
> +	F7188X_GPIO_BANK(8, 0xD0, DRVNAME "-2"),
> +	F7188X_GPIO_BANK(8, 0xC0, DRVNAME "-3"),
> +	F7188X_GPIO_BANK(8, 0xB0, DRVNAME "-4"),
> +	F7188X_GPIO_BANK(8, 0xA0, DRVNAME "-5"),
> +	F7188X_GPIO_BANK(5, 0x90, DRVNAME "-6"),
>  };
>  
>  static struct f7188x_gpio_bank nct6126d_gpio_bank[] = {
> -	F7188X_GPIO_BANK(0, 8, 0xE0, DRVNAME "-0"),
> -	F7188X_GPIO_BANK(10, 8, 0xE4, DRVNAME "-1"),
> -	F7188X_GPIO_BANK(20, 8, 0xE8, DRVNAME "-2"),
> -	F7188X_GPIO_BANK(30, 8, 0xEC, DRVNAME "-3"),
> -	F7188X_GPIO_BANK(40, 8, 0xF0, DRVNAME "-4"),
> -	F7188X_GPIO_BANK(50, 8, 0xF4, DRVNAME "-5"),
> -	F7188X_GPIO_BANK(60, 8, 0xF8, DRVNAME "-6"),
> -	F7188X_GPIO_BANK(70, 8, 0xFC, DRVNAME "-7"),
> +	F7188X_GPIO_BANK(8, 0xE0, DRVNAME "-0"),
> +	F7188X_GPIO_BANK(8, 0xE4, DRVNAME "-1"),
> +	F7188X_GPIO_BANK(8, 0xE8, DRVNAME "-2"),
> +	F7188X_GPIO_BANK(8, 0xEC, DRVNAME "-3"),
> +	F7188X_GPIO_BANK(8, 0xF0, DRVNAME "-4"),
> +	F7188X_GPIO_BANK(8, 0xF4, DRVNAME "-5"),
> +	F7188X_GPIO_BANK(8, 0xF8, DRVNAME "-6"),
> +	F7188X_GPIO_BANK(8, 0xFC, DRVNAME "-7"),
>  };
>  
>  static int f7188x_gpio_get_direction(struct gpio_chip *chip, unsigned offset)
> -- 
> 2.25.1

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)