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

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

[Laurence de Bruxelles]

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.

Signed-off-by: Laurence de Bruxelles <lfdebrux@gmail.com>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Fixes: ee0c8e494cc3 ("backlight: corgi: Convert to use GPIO descriptors")
---
 arch/arm/mach-pxa/spitz.c |  8 ++++----
 arch/arm/mach-pxa/spitz.h | 34 +++++++++++++++++-----------------
 2 files changed, 21 insertions(+), 21 deletions(-)

diff --git a/arch/arm/mach-pxa/spitz.c b/arch/arm/mach-pxa/spitz.c
index dd88953adc9d..ede3727fc91d 100644
--- a/arch/arm/mach-pxa/spitz.c
+++ b/arch/arm/mach-pxa/spitz.c
@@ -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),
 		{ },
 	},
@@ -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),
 		{ },
 	},
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
 
 /* 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
 
 /* Spitz IRQ Definitions */
 
-- 
2.36.1

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

[Arnd Bergmann]

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

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

[Laurence de Bruxelles]

On Mon Jul 4, 2022 at 1:50 PM BST, Arnd Bergmann wrote:
>
> Thank you for the patch! Since I recently worked on those files,
> I tried to have a closer look myself

Thanks for the comments! Sorry for the late reply, I got stuck in a
rabbit hole trying to understand some things.

>
> > @@ -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.

Good question. I spent a while trying to figure this out at the time, in
the end I just plugged in the i2c address I found from looking in sysfs,
and it worked. It does seem a bit opaque, but as far as I can tell from
reading the GPIO API manual and the i2c GPIO driver that's what it is
supposed to be:

key is ... the label of the gpiod_chip instance providing the GPIO

(from https://www.kernel.org/doc/html/latest/driver-api/gpio/board.html).

>
> > 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.
>

Good catch, I'm not sure how I missed your patch. I think having the
GPIO_BASE included in the index is part of the legacy API, or that is
what the API docs suggest, so we shouldn't need to add it or subtract it
anywhere.

I'd like to make that change, butg I'm not sure whether to submit that
as another patch in a new V3 series, or as a separate patch, or just
roll it into one patch... I'll probably make a V3 patch series unless
you suggest otherwise.

> >  /* 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.
>

Yes, I think so, when I run the kernel with this change with akita on
QEMU I get the error

spitz-audio spitz-audio: cannot find GPIO chip i2c-max7310, deferring

It's a shame, because it is more readable I think. I will change that in
the followup patch I mentioned. 

But to be honest I'm not fully able to test any changes to the audio
support for this hardware. I spent a little while trying to get audio
working on my device with a version of kernel 5.15, which doesn't have
your audio patch, but it was broken. I reverted a change to the pxa
audio driver to fix a DMA preallocation issue, but it was still broken.
So I don't have a working audio setup just yet.

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

[Arnd Bergmann]

On Tue, Jul 12, 2022 at 1:34 AM Laurence de Bruxelles
<lfdebrux@gmail.com> wrote:
> On Mon Jul 4, 2022 at 1:50 PM BST, Arnd Bergmann wrote:
> > > @@ -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.
>
> Good question. I spent a while trying to figure this out at the time, in
> the end I just plugged in the i2c address I found from looking in sysfs,
> and it worked. It does seem a bit opaque, but as far as I can tell from
> reading the GPIO API manual and the i2c GPIO driver that's what it is
> supposed to be:
>
> key is ... the label of the gpiod_chip instance providing the GPIO
>
> (from https://www.kernel.org/doc/html/latest/driver-api/gpio/board.html).

It clearly has to be the label that is associated with the i2c-gpio driver,
I'm just surprised that this label is just the i2c address and nor something
more descriptive. I see that pca953x_setup_gpio() sets the label as

     gc->label = dev_name(&chip->client->dev);

which may or may not be the right thing to do here. Maybe Linus has
a suggestion if this is expected or the name can be set to something else
instead.

> > 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.
> >
>
> Good catch, I'm not sure how I missed your patch. I think having the
> GPIO_BASE included in the index is part of the legacy API, or that is
> what the API docs suggest, so we shouldn't need to add it or subtract it
> anywhere.
>
> I'd like to make that change, butg I'm not sure whether to submit that
> as another patch in a new V3 series, or as a separate patch, or just
> roll it into one patch... I'll probably make a V3 patch series unless
> you suggest otherwise.

I think it's fine either way. As a rule, I try to decide based on the changelog
text: if I end up describing two different things in a commit log, then
it's time to split it up, but if I find a coherent way to describe the change
then it can stay as a single patch. If you end up splitting it, I would
recommend to keep changes across multiple boards together when they
do the same thing, but split it into logical changes that build on top
of one another.

> But to be honest I'm not fully able to test any changes to the audio
> support for this hardware. I spent a little while trying to get audio
> working on my device with a version of kernel 5.15, which doesn't have
> your audio patch, but it was broken. I reverted a change to the pxa
> audio driver to fix a DMA preallocation issue, but it was still broken.
> So I don't have a working audio setup just yet.

Ok, I see. Don't spend too much time on that yet then. It should be
possible to figure out what broke it using git-bisect, but you can do
that once you work on the DT conversion.

      Arnd