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

From: simon . guinot
Date: Mon May 29 2023 - 08:27:37 EST

Next message: Claudiu.Beznea: "Re: [PATCH v2 2/4] dt-bindings: timer: atmel,at91sam9260-pit: convert to yaml"
Previous message: Conor Dooley: "Re: [PATCH v2 4/4] dt-bindings: timer: atmel,at91rm9200-st: convert to yaml"
In reply to: xingtong_wu: "[PATCH v2 1/1] gpio-f7188x: fix base values conflicts with other gpio pins"
Next in thread: Linus Walleij: "Re: [PATCH v2 1/1] gpio-f7188x: fix base values conflicts with other gpio pins"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Mon, May 29, 2023 at 10:50:12AM +0800, xingtong_wu@xxxxxxx wrote:
> From: "xingtong.wu" <xingtong.wu@xxxxxxxxxxx>
>
> 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@xxxxxxxxxxx>
> ---
> 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

Attachment: signature.asc
Description: PGP signature

Next message: Claudiu.Beznea: "Re: [PATCH v2 2/4] dt-bindings: timer: atmel,at91sam9260-pit: convert to yaml"
Previous message: Conor Dooley: "Re: [PATCH v2 4/4] dt-bindings: timer: atmel,at91rm9200-st: convert to yaml"
In reply to: xingtong_wu: "[PATCH v2 1/1] gpio-f7188x: fix base values conflicts with other gpio pins"
Next in thread: Linus Walleij: "Re: [PATCH v2 1/1] gpio-f7188x: fix base values conflicts with other gpio pins"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]