Re: [PATCH] ARM: pxa: Fix gpio numbers for backlight controls

[Arnd Bergmann <arnd@arndb.de>]

On Sat, Jul 2, 2022 at 9:54 AM Laurence de Bruxelles <lfdebrux@gmail.com> wrote:
>
> With recent stable kernels on my Zaurus Akita I am unable to change the
> backlight brightness, as the necessary files are not in the sys
> filesystem, and at startup I get the following errors:
>
> corgi-lcd spi2.1: requested GPIO 0 (207) is out of range [0..120] for
> chip gpio-pxa
> corgi-lcd: probe of spi2.1 failed with error -22
>
> Looking at recent changes to the Corgi backlight driver, I found that
> commit ee0c8e494cc3 ("backlight: corgi: Convert to use GPIO
> descriptors") replaced the use of the deprecated integer-based GPIO
> interface, but didn't convert the GPIO numbers in the platform header
> to match this. I think this is what causes the error, as the GPIOs for
> the backlight are part of an expander chip with a GPIO base number
> larger than the number of GPIOs for the gpio-pxa device.
>
> This patch fixes things by updating the GPIO numbers for the backlight,
> simply by dropping the GPIO base. It also updates the GPIO lookups to
> look in the table for the appropriate expander. For the Akita platform
> the expander with i2c address 0-0018 is used, for Spitz and Borzoi
> the second Sharp SCOOP expander is used.
>
> I've tested this patch with real Akita and Spitz handhelds, as well as
> with QEMU.

Thank you for the patch! Since I recently worked on those files,
I tried to have a closer look myself

> @@ -524,9 +524,9 @@ static void spitz_bl_kick_battery(void)
>  static struct gpiod_lookup_table spitz_lcdcon_gpio_table = {
>         .dev_id = "spi2.1",
>         .table = {
> -               GPIO_LOOKUP("gpio-pxa", SPITZ_GPIO_BACKLIGHT_CONT,
> +               GPIO_LOOKUP("sharp-scoop.1", SPITZ_GPIO_BACKLIGHT_CONT,
>                             "BL_CONT", GPIO_ACTIVE_LOW),
> -               GPIO_LOOKUP("gpio-pxa", SPITZ_GPIO_BACKLIGHT_ON,
> +               GPIO_LOOKUP("sharp-scoop.1", SPITZ_GPIO_BACKLIGHT_ON,
>                             "BL_ON", GPIO_ACTIVE_HIGH),
>                 { },
>         },

This part looks fine.

> @@ -535,9 +535,9 @@ static struct gpiod_lookup_table spitz_lcdcon_gpio_table = {
>  static struct gpiod_lookup_table akita_lcdcon_gpio_table = {
>         .dev_id = "spi2.1",
>         .table = {
> -               GPIO_LOOKUP("gpio-pxa", AKITA_GPIO_BACKLIGHT_CONT,
> +               GPIO_LOOKUP("0-0018", AKITA_GPIO_BACKLIGHT_CONT,
>                             "BL_CONT", GPIO_ACTIVE_LOW),
> -               GPIO_LOOKUP("gpio-pxa", AKITA_GPIO_BACKLIGHT_ON,
> +               GPIO_LOOKUP("0-0018", AKITA_GPIO_BACKLIGHT_ON,
>                             "BL_ON", GPIO_ACTIVE_HIGH),
>                 { },
>         },

I assume this is correct as well, but I can't see where you got the "0-0018"
name from, or if we need to change others the same way.

> diff --git a/arch/arm/mach-pxa/spitz.h b/arch/arm/mach-pxa/spitz.h
> index 04828d8918aa..4def79ee6df0 100644
> --- a/arch/arm/mach-pxa/spitz.h
> +++ b/arch/arm/mach-pxa/spitz.h
> @@ -137,26 +137,26 @@
>  #define SPITZ_SCP2_SUS_SET  (SPITZ_SCP2_IR_ON | SPITZ_SCP2_RESERVED_1)
>
>  #define SPITZ_SCP2_GPIO_BASE           (PXA_NR_BUILTIN_GPIO + 12)
> -#define SPITZ_GPIO_IR_ON               (SPITZ_SCP2_GPIO_BASE + 0)
> -#define SPITZ_GPIO_AKIN_PULLUP         (SPITZ_SCP2_GPIO_BASE + 1)
> -#define SPITZ_GPIO_RESERVED_1          (SPITZ_SCP2_GPIO_BASE + 2)
> -#define SPITZ_GPIO_RESERVED_2          (SPITZ_SCP2_GPIO_BASE + 3)
> -#define SPITZ_GPIO_RESERVED_3          (SPITZ_SCP2_GPIO_BASE + 4)
> -#define SPITZ_GPIO_RESERVED_4          (SPITZ_SCP2_GPIO_BASE + 5)
> -#define SPITZ_GPIO_BACKLIGHT_CONT      (SPITZ_SCP2_GPIO_BASE + 6)
> -#define SPITZ_GPIO_BACKLIGHT_ON                (SPITZ_SCP2_GPIO_BASE + 7)
> -#define SPITZ_GPIO_MIC_BIAS            (SPITZ_SCP2_GPIO_BASE + 8)
> +#define SPITZ_GPIO_IR_ON               0
> +#define SPITZ_GPIO_AKIN_PULLUP         1
> +#define SPITZ_GPIO_RESERVED_1          2
> +#define SPITZ_GPIO_RESERVED_2          3
> +#define SPITZ_GPIO_RESERVED_3          4
> +#define SPITZ_GPIO_RESERVED_4          5
> +#define SPITZ_GPIO_BACKLIGHT_CONT      6
> +#define SPITZ_GPIO_BACKLIGHT_ON                7
> +#define SPITZ_GPIO_MIC_BIAS            8

This probably breaks the audio support, at least the SPITZ_GPIO_MIC_BIAS
one:

arch/arm/mach-pxa/spitz.c:              GPIO_LOOKUP("sharp-scoop.0",
SPITZ_GPIO_MUTE_L - SPITZ_SCP_GPIO_BASE,
arch/arm/mach-pxa/spitz.c:              GPIO_LOOKUP("sharp-scoop.0",
SPITZ_GPIO_MUTE_R - SPITZ_SCP_GPIO_BASE,
arch/arm/mach-pxa/spitz.c:              GPIO_LOOKUP("sharp-scoop.1",
SPITZ_GPIO_MIC_BIAS - SPITZ_SCP2_GPIO_BASE,
arch/arm/mach-pxa/spitz.c:              GPIO_LOOKUP("sharp-scoop.0",
SPITZ_GPIO_MUTE_L - SPITZ_SCP_GPIO_BASE,
arch/arm/mach-pxa/spitz.c:              GPIO_LOOKUP("sharp-scoop.0",
SPITZ_GPIO_MUTE_R - SPITZ_SCP_GPIO_BASE,

I think we'd probably want to handle the SCP and SCP2 GPIOs the same
way, either adding/subtracting the GPIO_BASE everywhere, or dropping that.

>  /* Akita IO Expander GPIOs */
>  #define AKITA_IOEXP_GPIO_BASE          (PXA_NR_BUILTIN_GPIO + 12)
> -#define AKITA_GPIO_RESERVED_0          (AKITA_IOEXP_GPIO_BASE + 0)
> -#define AKITA_GPIO_RESERVED_1          (AKITA_IOEXP_GPIO_BASE + 1)
> -#define AKITA_GPIO_MIC_BIAS            (AKITA_IOEXP_GPIO_BASE + 2)
> -#define AKITA_GPIO_BACKLIGHT_ON                (AKITA_IOEXP_GPIO_BASE + 3)
> -#define AKITA_GPIO_BACKLIGHT_CONT      (AKITA_IOEXP_GPIO_BASE + 4)
> -#define AKITA_GPIO_AKIN_PULLUP         (AKITA_IOEXP_GPIO_BASE + 5)
> -#define AKITA_GPIO_IR_ON               (AKITA_IOEXP_GPIO_BASE + 6)
> -#define AKITA_GPIO_RESERVED_7          (AKITA_IOEXP_GPIO_BASE + 7)
> +#define AKITA_GPIO_RESERVED_0          0
> +#define AKITA_GPIO_RESERVED_1          1
> +#define AKITA_GPIO_MIC_BIAS            2
> +#define AKITA_GPIO_BACKLIGHT_ON                3
> +#define AKITA_GPIO_BACKLIGHT_CONT      4
> +#define AKITA_GPIO_AKIN_PULLUP         5
> +#define AKITA_GPIO_IR_ON               6
> +#define AKITA_GPIO_RESERVED_7          7

I see that this line

arch/arm/mach-pxa/spitz.c:              GPIO_LOOKUP("i2c-max7310",
AKITA_GPIO_MIC_BIAS - AKITA_IOEXP_GPIO_BASE,

is now wrong, and presumably the "i2c-max7310" string also needs to be changed
to "0-0018" now.

         Arnd