* [PATCH v2 0/1] gpio-f7188x: fix base values conflicts with other gpio pins @ 2023-05-29 2:50 xingtong_wu 2023-05-29 2:50 ` [PATCH v2 1/1] " xingtong_wu 0 siblings, 1 reply; 20+ messages in thread From: xingtong_wu @ 2023-05-29 2:50 UTC (permalink / raw) To: linus.walleij, brgl, linux-gpio, linux-kernel; +Cc: henning.schild, xingtong.wu Switch pin base from static to automatic allocation to avoid conflicts and align with other gpio chip drivers Based on: [PATCH v2 1/1] gpio-f7188x: fix chip name and pin count on Nuvoton chip which was recently pulled by maintainer ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v2 1/1] gpio-f7188x: fix base values conflicts with other gpio pins 2023-05-29 2:50 [PATCH v2 0/1] gpio-f7188x: fix base values conflicts with other gpio pins xingtong_wu @ 2023-05-29 2:50 ` xingtong_wu 2023-05-29 12:26 ` simon.guinot ` (2 more replies) 0 siblings, 3 replies; 20+ messages in thread From: xingtong_wu @ 2023-05-29 2:50 UTC (permalink / raw) To: linus.walleij, brgl, linux-gpio, linux-kernel; +Cc: henning.schild, xingtong.wu From: "xingtong.wu" <xingtong.wu@siemens.com> switch pin base from static to automatic allocation to avoid conflicts and align with other gpio chip drivers Signed-off-by: xingtong.wu <xingtong.wu@siemens.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 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH v2 1/1] gpio-f7188x: fix base values conflicts with other gpio pins 2023-05-29 2:50 ` [PATCH v2 1/1] " xingtong_wu @ 2023-05-29 12:26 ` simon.guinot 2023-05-29 13:03 ` Linus Walleij 2023-05-29 13:02 ` Linus Walleij 2023-09-11 7:04 ` Bartosz Golaszewski 2 siblings, 1 reply; 20+ messages in thread From: simon.guinot @ 2023-05-29 12:26 UTC (permalink / raw) To: xingtong_wu Cc: linus.walleij, brgl, linux-gpio, linux-kernel, henning.schild, xingtong.wu [-- Attachment #1: Type: text/plain, Size: 9263 bytes --] On Mon, May 29, 2023 at 10:50:12AM +0800, xingtong_wu@163.com wrote: > From: "xingtong.wu" <xingtong.wu@siemens.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@siemens.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 [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 1/1] gpio-f7188x: fix base values conflicts with other gpio pins 2023-05-29 12:26 ` simon.guinot @ 2023-05-29 13:03 ` Linus Walleij 2023-05-29 13:54 ` simon.guinot 0 siblings, 1 reply; 20+ messages in thread From: Linus Walleij @ 2023-05-29 13:03 UTC (permalink / raw) To: simon.guinot Cc: xingtong_wu, brgl, linux-gpio, linux-kernel, henning.schild, xingtong.wu On Mon, May 29, 2023 at 2:27 PM <simon.guinot@sequanux.org> wrote: > 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. We actively discourage this kind of mapping because of reasons stated in drivers/gpio/TODO: we want dynamic number allocation to be the norm. Yours, Linus Walleij ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 1/1] gpio-f7188x: fix base values conflicts with other gpio pins 2023-05-29 13:03 ` Linus Walleij @ 2023-05-29 13:54 ` simon.guinot 2023-05-29 22:24 ` andy.shevchenko ` (2 more replies) 0 siblings, 3 replies; 20+ messages in thread From: simon.guinot @ 2023-05-29 13:54 UTC (permalink / raw) To: Linus Walleij Cc: xingtong_wu, brgl, linux-gpio, linux-kernel, henning.schild, xingtong.wu [-- Attachment #1: Type: text/plain, Size: 818 bytes --] On Mon, May 29, 2023 at 03:03:28PM +0200, Linus Walleij wrote: > On Mon, May 29, 2023 at 2:27 PM <simon.guinot@sequanux.org> wrote: > > > 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. > > We actively discourage this kind of mapping because of reasons stated > in drivers/gpio/TODO: we want dynamic number allocation to be the > norm. Hi Linus, Sure but it would be nice to have a dynamic base applied to a controller (and not to each chip of this controller), and to respect the interval between the chips (as stated in the controllers datasheets). This way the assignation would be dynamic and the pin numbers found in controller datasheet would be meaningful as well. Simon [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 1/1] gpio-f7188x: fix base values conflicts with other gpio pins 2023-05-29 13:54 ` simon.guinot @ 2023-05-29 22:24 ` andy.shevchenko 2023-05-30 6:27 ` xingtong.wu 2023-05-30 17:53 ` simon.guinot 2023-05-30 10:57 ` Henning Schild 2023-05-30 11:40 ` Linus Walleij 2 siblings, 2 replies; 20+ messages in thread From: andy.shevchenko @ 2023-05-29 22:24 UTC (permalink / raw) To: simon.guinot Cc: Linus Walleij, xingtong_wu, brgl, linux-gpio, linux-kernel, henning.schild, xingtong.wu Mon, May 29, 2023 at 03:54:36PM +0200, simon.guinot@sequanux.org kirjoitti: > On Mon, May 29, 2023 at 03:03:28PM +0200, Linus Walleij wrote: > > On Mon, May 29, 2023 at 2:27 PM <simon.guinot@sequanux.org> wrote: > > > > > 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. > > > > We actively discourage this kind of mapping because of reasons stated > > in drivers/gpio/TODO: we want dynamic number allocation to be the > > norm. > > Sure but it would be nice to have a dynamic base applied to a controller > (and not to each chip of this controller), and to respect the interval > between the chips (as stated in the controllers datasheets). What you want is against the architecture. To fix this, you might change the architecture of the driver to have one chip for the controller, but it's quite questionable change. Also how can you guarantee ordering of the enumeration? You probably need to *disable* SMP on the boot time. This will still be fragile as long as GPIO chip can be unbound at run time. Order can be changed. So, the patch is good and the correct way to go. P.S. The root cause is that hardware engineers and documentation writers do not consider their hardware in the multi-tasking, multi-user general purpose operating system, such as Linux. I believe the ideal fix is to fix the documentation (datasheet). -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 1/1] gpio-f7188x: fix base values conflicts with other gpio pins 2023-05-29 22:24 ` andy.shevchenko @ 2023-05-30 6:27 ` xingtong.wu 2023-05-30 10:53 ` andy.shevchenko 2023-05-30 17:53 ` simon.guinot 1 sibling, 1 reply; 20+ messages in thread From: xingtong.wu @ 2023-05-30 6:27 UTC (permalink / raw) To: andy.shevchenko, simon.guinot Cc: Linus Walleij, brgl, linux-gpio, linux-kernel, henning.schild, xingtong.wu On 2023/5/30 06:24, andy.shevchenko@gmail.com wrote: > Mon, May 29, 2023 at 03:54:36PM +0200, simon.guinot@sequanux.org kirjoitti: >> On Mon, May 29, 2023 at 03:03:28PM +0200, Linus Walleij wrote: >>> On Mon, May 29, 2023 at 2:27 PM <simon.guinot@sequanux.org> wrote: >>> >>>> 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. >>> >>> We actively discourage this kind of mapping because of reasons stated >>> in drivers/gpio/TODO: we want dynamic number allocation to be the >>> norm. >> >> Sure but it would be nice to have a dynamic base applied to a controller >> (and not to each chip of this controller), and to respect the interval >> between the chips (as stated in the controllers datasheets). > > What you want is against the architecture. To fix this, you might change > the architecture of the driver to have one chip for the controller, but > it's quite questionable change. Also how can you guarantee ordering of > the enumeration? You probably need to *disable* SMP on the boot time. > This will still be fragile as long as GPIO chip can be unbound at run > time. Order can be changed. > > So, the patch is good and the correct way to go. > > P.S. The root cause is that hardware engineers and documentation writers > do not consider their hardware in the multi-tasking, multi-user general > purpose operating system, such as Linux. I believe the ideal fix is to fix the > documentation (datasheet). > Hi, Thanks for your review. The direct reason of this patch is that when "modprobe gpio-f7188x", it conflicts with INT34C6. I met this issue on an older kernel, but could not remember which version exactly. The error message is as the link below: https://elixir.bootlin.com/linux/v6.3.2/source/drivers/gpio/gpiolib.c#L798 - XingTong Wu ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 1/1] gpio-f7188x: fix base values conflicts with other gpio pins 2023-05-30 6:27 ` xingtong.wu @ 2023-05-30 10:53 ` andy.shevchenko 2023-05-30 11:10 ` andy.shevchenko 0 siblings, 1 reply; 20+ messages in thread From: andy.shevchenko @ 2023-05-30 10:53 UTC (permalink / raw) To: xingtong.wu Cc: andy.shevchenko, simon.guinot, Linus Walleij, brgl, linux-gpio, linux-kernel, henning.schild, xingtong.wu Tue, May 30, 2023 at 02:27:09PM +0800, xingtong.wu kirjoitti: > On 2023/5/30 06:24, andy.shevchenko@gmail.com wrote: > > Mon, May 29, 2023 at 03:54:36PM +0200, simon.guinot@sequanux.org kirjoitti: > >> On Mon, May 29, 2023 at 03:03:28PM +0200, Linus Walleij wrote: > >>> On Mon, May 29, 2023 at 2:27 PM <simon.guinot@sequanux.org> wrote: > >>> > >>>> 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. > >>> > >>> We actively discourage this kind of mapping because of reasons stated > >>> in drivers/gpio/TODO: we want dynamic number allocation to be the > >>> norm. > >> > >> Sure but it would be nice to have a dynamic base applied to a controller > >> (and not to each chip of this controller), and to respect the interval > >> between the chips (as stated in the controllers datasheets). > > > > What you want is against the architecture. To fix this, you might change > > the architecture of the driver to have one chip for the controller, but > > it's quite questionable change. Also how can you guarantee ordering of > > the enumeration? You probably need to *disable* SMP on the boot time. > > This will still be fragile as long as GPIO chip can be unbound at run > > time. Order can be changed. > > > > So, the patch is good and the correct way to go. > > > > P.S. The root cause is that hardware engineers and documentation writers > > do not consider their hardware in the multi-tasking, multi-user general > > purpose operating system, such as Linux. I believe the ideal fix is to fix the > > documentation (datasheet). > > Thanks for your review. > > The direct reason of this patch is that when "modprobe gpio-f7188x", > it conflicts with INT34C6. I met this issue on an older kernel, but > could not remember which version exactly. This is interesting. But what I have noticed the v6.3.2 missing this https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/gpio/gpiolib.c?id=7dd3d9bd873f138675cb727eaa51a498d99f0e89 change. Can you apply and retest? If this does not help, please share more details, exact steps of reproducing the issue, including respective `dmesg` output, etc. (maybe via creating a kernel bugzilla report). > The error message is as the link below: > https://elixir.bootlin.com/linux/v6.3.2/source/drivers/gpio/gpiolib.c#L798 -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 1/1] gpio-f7188x: fix base values conflicts with other gpio pins 2023-05-30 10:53 ` andy.shevchenko @ 2023-05-30 11:10 ` andy.shevchenko 0 siblings, 0 replies; 20+ messages in thread From: andy.shevchenko @ 2023-05-30 11:10 UTC (permalink / raw) To: andy.shevchenko Cc: xingtong.wu, simon.guinot, Linus Walleij, brgl, linux-gpio, linux-kernel, henning.schild, xingtong.wu Tue, May 30, 2023 at 01:53:47PM +0300, andy.shevchenko@gmail.com kirjoitti: > Tue, May 30, 2023 at 02:27:09PM +0800, xingtong.wu kirjoitti: > > On 2023/5/30 06:24, andy.shevchenko@gmail.com wrote: > > > Mon, May 29, 2023 at 03:54:36PM +0200, simon.guinot@sequanux.org kirjoitti: > > >> On Mon, May 29, 2023 at 03:03:28PM +0200, Linus Walleij wrote: > > >>> On Mon, May 29, 2023 at 2:27 PM <simon.guinot@sequanux.org> wrote: > > >>> > > >>>> 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. > > >>> > > >>> We actively discourage this kind of mapping because of reasons stated > > >>> in drivers/gpio/TODO: we want dynamic number allocation to be the > > >>> norm. > > >> > > >> Sure but it would be nice to have a dynamic base applied to a controller > > >> (and not to each chip of this controller), and to respect the interval > > >> between the chips (as stated in the controllers datasheets). > > > > > > What you want is against the architecture. To fix this, you might change > > > the architecture of the driver to have one chip for the controller, but > > > it's quite questionable change. Also how can you guarantee ordering of > > > the enumeration? You probably need to *disable* SMP on the boot time. > > > This will still be fragile as long as GPIO chip can be unbound at run > > > time. Order can be changed. > > > > > > So, the patch is good and the correct way to go. > > > > > > P.S. The root cause is that hardware engineers and documentation writers > > > do not consider their hardware in the multi-tasking, multi-user general > > > purpose operating system, such as Linux. I believe the ideal fix is to fix the > > > documentation (datasheet). > > > > Thanks for your review. > > > > The direct reason of this patch Oh, It seems I misread this as the cause of the patch, please ignore my previous reply. > > is that when "modprobe gpio-f7188x", > > it conflicts with INT34C6. I met this issue on an older kernel, but > > could not remember which version exactly. > > This is interesting. But what I have noticed the v6.3.2 missing this > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/gpio/gpiolib.c?id=7dd3d9bd873f138675cb727eaa51a498d99f0e89 > change. Can you apply and retest? > > If this does not help, please share more details, exact steps of reproducing > the issue, including respective `dmesg` output, etc. (maybe via creating a > kernel bugzilla report). > > > The error message is as the link below: > > https://elixir.bootlin.com/linux/v6.3.2/source/drivers/gpio/gpiolib.c#L798 -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 1/1] gpio-f7188x: fix base values conflicts with other gpio pins 2023-05-29 22:24 ` andy.shevchenko 2023-05-30 6:27 ` xingtong.wu @ 2023-05-30 17:53 ` simon.guinot 2023-05-30 21:42 ` Andy Shevchenko 1 sibling, 1 reply; 20+ messages in thread From: simon.guinot @ 2023-05-30 17:53 UTC (permalink / raw) To: andy.shevchenko Cc: Linus Walleij, xingtong_wu, brgl, linux-gpio, linux-kernel, henning.schild, xingtong.wu [-- Attachment #1: Type: text/plain, Size: 2523 bytes --] On Tue, May 30, 2023 at 01:24:30AM +0300, andy.shevchenko@gmail.com wrote: > Mon, May 29, 2023 at 03:54:36PM +0200, simon.guinot@sequanux.org kirjoitti: > > On Mon, May 29, 2023 at 03:03:28PM +0200, Linus Walleij wrote: > > > On Mon, May 29, 2023 at 2:27 PM <simon.guinot@sequanux.org> wrote: > > > > > > > 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. > > > > > > We actively discourage this kind of mapping because of reasons stated > > > in drivers/gpio/TODO: we want dynamic number allocation to be the > > > norm. > > > > Sure but it would be nice to have a dynamic base applied to a controller > > (and not to each chip of this controller), and to respect the interval > > between the chips (as stated in the controllers datasheets). > > What you want is against the architecture. To fix this, you might change > the architecture of the driver to have one chip for the controller, but > it's quite questionable change. Also how can you guarantee ordering of > the enumeration? You probably need to *disable* SMP on the boot time. > This will still be fragile as long as GPIO chip can be unbound at run > time. Order can be changed. > > So, the patch is good and the correct way to go. > > P.S. The root cause is that hardware engineers and documentation writers > do not consider their hardware in the multi-tasking, multi-user general > purpose operating system, such as Linux. I believe the ideal fix is to fix the > documentation (datasheet). Some GPIO controllers (as Super-I/O) are multifunctional devices and pins are multiplexed. Some can be configured to act as GPIOs and some cannot. So there are holes. It is an hardware reality and not only an issue due to poorly written documents (even if there are issues with them too). Today we work around these holes by splitting the GPIOs between several chips. As a consequence "hardware" GPIO numbers don't exist in Linux. It requires some work from a user to first find the chip a GPIO belongs to and then compute the number. It is not terrible. But on some machines with a lot of GPIO controllers and chips it can be quite challenging (especially when ACPI is involved). I am only saying it would be nice for Linux users if they could use hardware GPIO numbers (i.e. as read in hardware documents). But I understand everything that has been said by everyone and I agree. Simon [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 1/1] gpio-f7188x: fix base values conflicts with other gpio pins 2023-05-30 17:53 ` simon.guinot @ 2023-05-30 21:42 ` Andy Shevchenko 0 siblings, 0 replies; 20+ messages in thread From: Andy Shevchenko @ 2023-05-30 21:42 UTC (permalink / raw) To: simon.guinot Cc: Linus Walleij, xingtong_wu, brgl, linux-gpio, linux-kernel, henning.schild, xingtong.wu On Tue, May 30, 2023 at 8:56 PM <simon.guinot@sequanux.org> wrote: > On Tue, May 30, 2023 at 01:24:30AM +0300, andy.shevchenko@gmail.com wrote: > > Mon, May 29, 2023 at 03:54:36PM +0200, simon.guinot@sequanux.org kirjoitti: > > > On Mon, May 29, 2023 at 03:03:28PM +0200, Linus Walleij wrote: > > > > On Mon, May 29, 2023 at 2:27 PM <simon.guinot@sequanux.org> wrote: > > > > > > > > > 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. > > > > > > > > We actively discourage this kind of mapping because of reasons stated > > > > in drivers/gpio/TODO: we want dynamic number allocation to be the > > > > norm. > > > > > > Sure but it would be nice to have a dynamic base applied to a controller > > > (and not to each chip of this controller), and to respect the interval > > > between the chips (as stated in the controllers datasheets). > > > > What you want is against the architecture. To fix this, you might change > > the architecture of the driver to have one chip for the controller, but > > it's quite questionable change. Also how can you guarantee ordering of > > the enumeration? You probably need to *disable* SMP on the boot time. > > This will still be fragile as long as GPIO chip can be unbound at run > > time. Order can be changed. > > > > So, the patch is good and the correct way to go. > > > > P.S. The root cause is that hardware engineers and documentation writers > > do not consider their hardware in the multi-tasking, multi-user general > > purpose operating system, such as Linux. I believe the ideal fix is to fix the > > documentation (datasheet). > > Some GPIO controllers (as Super-I/O) are multifunctional devices and > pins are multiplexed. Some can be configured to act as GPIOs and some > cannot. So there are holes. It is an hardware reality and not only an > issue due to poorly written documents (even if there are issues with > them too). So, this is done with GPIO to pin mapping (and yes, pin control has to be present). In simpler cases the valid mask is enough. > Today we work around these holes by splitting the GPIOs between several > chips. What you are saying seems like a broken architecture of the certain driver, i.e. exposing hardware not in the correct representation (wrong mapping). Maybe I'm missing something... > As a consequence "hardware" GPIO numbers don't exist in Linux. It > requires some work from a user to first find the chip a GPIO belongs to > and then compute the number. It is not terrible. But on some machines > with a lot of GPIO controllers and chips it can be quite challenging > (especially when ACPI is involved). Not sure how ACPI makes things worse (except the number space used for GpioIo() and GpioInt() resources, which in case of existing pin control may be different to the pin numbering). In any case the pin control case is covered nowadays in debugfs and one may look at that to find the mapping and pin naming. > I am only saying it would be nice for Linux users if they could use > hardware GPIO numbers (i.e. as read in hardware documents). It's impossible. I can make an example which is the UP board (or UP² a.k.a. UP Square) where GPIO from SoC goes through CPLD and becomes completely non-related in the documentation. AFAIU all the same for Raspberry Pi. Besides that, if a board has an I²C expander, and other I²C buses available to connect anything, it will always be ambiguous. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 1/1] gpio-f7188x: fix base values conflicts with other gpio pins 2023-05-29 13:54 ` simon.guinot 2023-05-29 22:24 ` andy.shevchenko @ 2023-05-30 10:57 ` Henning Schild 2023-05-30 17:57 ` simon.guinot 2023-05-30 11:40 ` Linus Walleij 2 siblings, 1 reply; 20+ messages in thread From: Henning Schild @ 2023-05-30 10:57 UTC (permalink / raw) To: simon.guinot Cc: Linus Walleij, xingtong_wu, brgl, linux-gpio, linux-kernel, xingtong.wu Am Mon, 29 May 2023 15:54:36 +0200 schrieb simon.guinot@sequanux.org: > On Mon, May 29, 2023 at 03:03:28PM +0200, Linus Walleij wrote: > > On Mon, May 29, 2023 at 2:27 PM <simon.guinot@sequanux.org> wrote: > > > > > 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. > > > > We actively discourage this kind of mapping because of reasons > > stated in drivers/gpio/TODO: we want dynamic number allocation to > > be the norm. > > Hi Linus, > > Sure but it would be nice to have a dynamic base applied to a > controller (and not to each chip of this controller), and to respect > the interval between the chips (as stated in the controllers > datasheets). You mentioned yourself that there are the holes to take care of. And the symbols/names from the SPECs seem to be octal numbers to me. While humans might prefer decimal and the code seems to be hexadecimal. Not sure the numbers have ever been too useful for humans. And once we change one base (bank0) we actually already break user-land that so far failed to discover the base from sysfs (bug in that user-land code, not our problem). I am with Linus on that one, we should try. Henning > This way the assignation would be dynamic and the pin numbers found in > controller datasheet would be meaningful as well. > > Simon ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 1/1] gpio-f7188x: fix base values conflicts with other gpio pins 2023-05-30 10:57 ` Henning Schild @ 2023-05-30 17:57 ` simon.guinot 0 siblings, 0 replies; 20+ messages in thread From: simon.guinot @ 2023-05-30 17:57 UTC (permalink / raw) To: Henning Schild Cc: Linus Walleij, xingtong_wu, brgl, linux-gpio, linux-kernel, xingtong.wu [-- Attachment #1: Type: text/plain, Size: 1480 bytes --] On Tue, May 30, 2023 at 12:57:27PM +0200, Henning Schild wrote: > Am Mon, 29 May 2023 15:54:36 +0200 > schrieb simon.guinot@sequanux.org: > > > On Mon, May 29, 2023 at 03:03:28PM +0200, Linus Walleij wrote: > > > On Mon, May 29, 2023 at 2:27 PM <simon.guinot@sequanux.org> wrote: > > > > > > > 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. > > > > > > We actively discourage this kind of mapping because of reasons > > > stated in drivers/gpio/TODO: we want dynamic number allocation to > > > be the norm. > > > > Hi Linus, > > > > Sure but it would be nice to have a dynamic base applied to a > > controller (and not to each chip of this controller), and to respect > > the interval between the chips (as stated in the controllers > > datasheets). > > You mentioned yourself that there are the holes to take care of. And > the symbols/names from the SPECs seem to be octal numbers to me. While > humans might prefer decimal and the code seems to be hexadecimal. > > Not sure the numbers have ever been too useful for humans. And once we > change one base (bank0) we actually already break user-land that so far > failed to discover the base from sysfs (bug in that user-land code, not > our problem). > > I am with Linus on that one, we should try. I am also in the Linus and "everybody but me" team too :) [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 1/1] gpio-f7188x: fix base values conflicts with other gpio pins 2023-05-29 13:54 ` simon.guinot 2023-05-29 22:24 ` andy.shevchenko 2023-05-30 10:57 ` Henning Schild @ 2023-05-30 11:40 ` Linus Walleij 2 siblings, 0 replies; 20+ messages in thread From: Linus Walleij @ 2023-05-30 11:40 UTC (permalink / raw) To: simon.guinot Cc: xingtong_wu, brgl, linux-gpio, linux-kernel, henning.schild, xingtong.wu On Mon, May 29, 2023 at 3:56 PM <simon.guinot@sequanux.org> wrote: > This way the assignation would be dynamic and the pin numbers found in > controller datasheet would be meaningful as well. I always had in my mind that this is what you should use the .dbg_show() callback in struct gpio_chip for. It is for debugging, not ABI and cross-referencing datasheets is definitely debugging, so use that and look in /proc/sys/kernel/debug/gpio for the data you want for cross-referencing. Yours, Linus Walleij ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 1/1] gpio-f7188x: fix base values conflicts with other gpio pins 2023-05-29 2:50 ` [PATCH v2 1/1] " xingtong_wu 2023-05-29 12:26 ` simon.guinot @ 2023-05-29 13:02 ` Linus Walleij 2023-03-02 13:48 ` xingtong.wu ` (2 more replies) 2023-09-11 7:04 ` Bartosz Golaszewski 2 siblings, 3 replies; 20+ messages in thread From: Linus Walleij @ 2023-05-29 13:02 UTC (permalink / raw) To: xingtong_wu; +Cc: brgl, linux-gpio, linux-kernel, henning.schild, xingtong.wu On Mon, May 29, 2023 at 4:55 AM <xingtong_wu@163.com> wrote: > From: "xingtong.wu" <xingtong.wu@siemens.com> > > switch pin base from static to automatic allocation to > avoid conflicts and align with other gpio chip drivers > > Signed-off-by: xingtong.wu <xingtong.wu@siemens.com> Reviewed-by: Linus Walleij <linus.walleij@linaro.org> If this platform does not have a ton of userspace using the obsolete sysfs this should be fine to apply. I say let's apply and see what happens. Yours, Linus Walleij ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 1/1] gpio-f7188x: fix base values conflicts with other gpio pins 2023-05-29 13:02 ` Linus Walleij @ 2023-03-02 13:48 ` xingtong.wu 2023-06-16 7:53 ` Henning Schild 2023-08-31 7:28 ` xingtong.wu 2 siblings, 0 replies; 20+ messages in thread From: xingtong.wu @ 2023-03-02 13:48 UTC (permalink / raw) To: Linus Walleij; +Cc: brgl, linux-gpio, linux-kernel, henning.schild, xingtong.wu On 2023/5/29 21:02, Linus Walleij wrote: > On Mon, May 29, 2023 at 4:55 AM <xingtong_wu@163.com> wrote: > >> From: "xingtong.wu" <xingtong.wu@siemens.com> >> >> switch pin base from static to automatic allocation to >> avoid conflicts and align with other gpio chip drivers >> >> Signed-off-by: xingtong.wu <xingtong.wu@siemens.com> > > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> > > If this platform does not have a ton of userspace using the obsolete > sysfs this should be fine to apply. I say let's apply and see what happens. > > Yours, > Linus Walleij Hi Seems it has been a while since our last discussion, I guess it might be fit in. -- XingTong Wu ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 1/1] gpio-f7188x: fix base values conflicts with other gpio pins 2023-05-29 13:02 ` Linus Walleij 2023-03-02 13:48 ` xingtong.wu @ 2023-06-16 7:53 ` Henning Schild 2023-08-31 7:28 ` xingtong.wu 2 siblings, 0 replies; 20+ messages in thread From: Henning Schild @ 2023-06-16 7:53 UTC (permalink / raw) To: Linus Walleij; +Cc: xingtong_wu, brgl, linux-gpio, linux-kernel, xingtong.wu Am Mon, 29 May 2023 15:02:08 +0200 schrieb Linus Walleij <linus.walleij@linaro.org>: > On Mon, May 29, 2023 at 4:55 AM <xingtong_wu@163.com> wrote: > > > From: "xingtong.wu" <xingtong.wu@siemens.com> > > > > switch pin base from static to automatic allocation to > > avoid conflicts and align with other gpio chip drivers > > > > Signed-off-by: xingtong.wu <xingtong.wu@siemens.com> > > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> > > If this platform does not have a ton of userspace using the obsolete > sysfs this should be fine to apply. I say let's apply and see what > happens. Seems after some discussion we concluded to merge this. I guess it might already be a little late for 6.4. Henning > Yours, > Linus Walleij ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 1/1] gpio-f7188x: fix base values conflicts with other gpio pins 2023-05-29 13:02 ` Linus Walleij 2023-03-02 13:48 ` xingtong.wu 2023-06-16 7:53 ` Henning Schild @ 2023-08-31 7:28 ` xingtong.wu 2023-09-01 9:10 ` Bartosz Golaszewski 2 siblings, 1 reply; 20+ messages in thread From: xingtong.wu @ 2023-08-31 7:28 UTC (permalink / raw) To: Linus Walleij Cc: brgl, linux-gpio, linux-kernel, xingtong.wu, andy.shevchenko, simon.guinot, Schaffner, Tobias, Haeussler, Gerd On 2023/5/29 21:02, Linus Walleij wrote: > On Mon, May 29, 2023 at 4:55 AM <xingtong_wu@163.com> wrote: > >> From: "xingtong.wu" <xingtong.wu@siemens.com> >> >> switch pin base from static to automatic allocation to >> avoid conflicts and align with other gpio chip drivers >> >> Signed-off-by: xingtong.wu <xingtong.wu@siemens.com> > > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> > > If this platform does not have a ton of userspace using the obsolete > sysfs this should be fine to apply. I say let's apply and see what happens. > > Yours, > Linus Walleij Hi Seems the issue happened again, the module "gpio-f7188x" register gpio_chip failed because of the base value conflict. I hope the patch can be merged soon, I'm afraid that you forgot it... The log is below: [ 6.872049] gpio-f7188x: Unsupported Fintek device 0x0303 [ 6.872137] gpio-f7188x: Found nct6126d at 0x4e [ 6.899965] gpiochip_find_base: cannot find free range [ 6.899967] gpiochip_add_data_with_key: GPIOs 0..7 (gpio-f7188x-6) failed to register, -28 [ 6.899970] gpio-f7188x gpio-f7188x: Failed to register gpiochip 6: -28 [ 6.903329] simatic_ipc_batt simatic_ipc_batt: cannot find GPIO chip gpio-f7188x-6, deferring There is a gpio_chip created by "pinctrl-tigerlake": /sys/class/gpio/gpiochip49# ls -l total 0 -r--r--r--. 1 root root 4096 Aug 31 06:40 base lrwxrwxrwx. 1 root root 0 Aug 31 06:40 device -> ../../../INT34C6:00 -r--r--r--. 1 root root 4096 Aug 31 06:40 label -r--r--r--. 1 root root 4096 Aug 31 06:40 ngpio drwxr-xr-x. 2 root root 0 Aug 31 06:40 power lrwxrwxrwx. 1 root root 0 Aug 31 06:38 subsystem -> ../../../../../class/gpio -rw-r--r--. 1 root root 4096 Aug 31 06:38 uevent The base value is 49, label = INT34C6:00, ngpio = 463 The issue arose by chance, because the driver "pinctrl-tigerlake" apply gpio_chip->base randomly, this time it apply the base value 49, so it have conflict to here: https://github.com/torvalds/linux/blob/master/drivers/gpio/gpio-f7188x.c#L283 But sometime it apply other base values, so the issue do not happen. BRs, Xing Tong Wu ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 1/1] gpio-f7188x: fix base values conflicts with other gpio pins 2023-08-31 7:28 ` xingtong.wu @ 2023-09-01 9:10 ` Bartosz Golaszewski 0 siblings, 0 replies; 20+ messages in thread From: Bartosz Golaszewski @ 2023-09-01 9:10 UTC (permalink / raw) To: xingtong.wu Cc: Linus Walleij, linux-gpio, linux-kernel, xingtong.wu, andy.shevchenko, simon.guinot, Schaffner, Tobias, Haeussler, Gerd On Thu, Aug 31, 2023 at 9:28 AM xingtong.wu <xingtong_wu@163.com> wrote: > > On 2023/5/29 21:02, Linus Walleij wrote: > > On Mon, May 29, 2023 at 4:55 AM <xingtong_wu@163.com> wrote: > > > >> From: "xingtong.wu" <xingtong.wu@siemens.com> > >> > >> switch pin base from static to automatic allocation to > >> avoid conflicts and align with other gpio chip drivers > >> > >> Signed-off-by: xingtong.wu <xingtong.wu@siemens.com> > > > > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> > > > > If this platform does not have a ton of userspace using the obsolete > > sysfs this should be fine to apply. I say let's apply and see what happens. > > > > Yours, > > Linus Walleij > > Hi > > Seems the issue happened again, the module "gpio-f7188x" register > gpio_chip failed because of the base value conflict. I hope the patch > can be merged soon, I'm afraid that you forgot it... > > The log is below: > [ 6.872049] gpio-f7188x: Unsupported Fintek device 0x0303 > [ 6.872137] gpio-f7188x: Found nct6126d at 0x4e > [ 6.899965] gpiochip_find_base: cannot find free range > [ 6.899967] gpiochip_add_data_with_key: GPIOs 0..7 (gpio-f7188x-6) failed to register, -28 > [ 6.899970] gpio-f7188x gpio-f7188x: Failed to register gpiochip 6: -28 > [ 6.903329] simatic_ipc_batt simatic_ipc_batt: cannot find GPIO chip gpio-f7188x-6, deferring > > There is a gpio_chip created by "pinctrl-tigerlake": > /sys/class/gpio/gpiochip49# ls -l > total 0 > -r--r--r--. 1 root root 4096 Aug 31 06:40 base > lrwxrwxrwx. 1 root root 0 Aug 31 06:40 device -> ../../../INT34C6:00 > -r--r--r--. 1 root root 4096 Aug 31 06:40 label > -r--r--r--. 1 root root 4096 Aug 31 06:40 ngpio > drwxr-xr-x. 2 root root 0 Aug 31 06:40 power > lrwxrwxrwx. 1 root root 0 Aug 31 06:38 subsystem -> ../../../../../class/gpio > -rw-r--r--. 1 root root 4096 Aug 31 06:38 uevent > > The base value is 49, label = INT34C6:00, ngpio = 463 > > The issue arose by chance, because the driver "pinctrl-tigerlake" apply gpio_chip->base > randomly, this time it apply the base value 49, so it have conflict to here: > https://github.com/torvalds/linux/blob/master/drivers/gpio/gpio-f7188x.c#L283 > > But sometime it apply other base values, so the issue do not happen. > > BRs, > Xing Tong Wu > Ah, it fell through the cracks. I will queue it right after the merge window. Bart ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 1/1] gpio-f7188x: fix base values conflicts with other gpio pins 2023-05-29 2:50 ` [PATCH v2 1/1] " xingtong_wu 2023-05-29 12:26 ` simon.guinot 2023-05-29 13:02 ` Linus Walleij @ 2023-09-11 7:04 ` Bartosz Golaszewski 2 siblings, 0 replies; 20+ messages in thread From: Bartosz Golaszewski @ 2023-09-11 7:04 UTC (permalink / raw) To: xingtong_wu Cc: linus.walleij, linux-gpio, linux-kernel, henning.schild, xingtong.wu On Mon, May 29, 2023 at 4:55 AM <xingtong_wu@163.com> wrote: > > From: "xingtong.wu" <xingtong.wu@siemens.com> > > switch pin base from static to automatic allocation to > avoid conflicts and align with other gpio chip drivers > > Signed-off-by: xingtong.wu <xingtong.wu@siemens.com> > --- Applied, thanks! Bart ^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2023-09-11 7:04 UTC | newest] Thread overview: 20+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-05-29 2:50 [PATCH v2 0/1] gpio-f7188x: fix base values conflicts with other gpio pins xingtong_wu 2023-05-29 2:50 ` [PATCH v2 1/1] " xingtong_wu 2023-05-29 12:26 ` simon.guinot 2023-05-29 13:03 ` Linus Walleij 2023-05-29 13:54 ` simon.guinot 2023-05-29 22:24 ` andy.shevchenko 2023-05-30 6:27 ` xingtong.wu 2023-05-30 10:53 ` andy.shevchenko 2023-05-30 11:10 ` andy.shevchenko 2023-05-30 17:53 ` simon.guinot 2023-05-30 21:42 ` Andy Shevchenko 2023-05-30 10:57 ` Henning Schild 2023-05-30 17:57 ` simon.guinot 2023-05-30 11:40 ` Linus Walleij 2023-05-29 13:02 ` Linus Walleij 2023-03-02 13:48 ` xingtong.wu 2023-06-16 7:53 ` Henning Schild 2023-08-31 7:28 ` xingtong.wu 2023-09-01 9:10 ` Bartosz Golaszewski 2023-09-11 7:04 ` Bartosz Golaszewski
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).