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

["Laurence de Bruxelles" <lfdebrux@gmail.com>]

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.