Re: [PATCH] ASoC: simple-card: Add mic and hp detect gpios.

Re: [PATCH] ASoC: simple-card: Add mic and hp detect gpios.

[Geert Uytterhoeven]

(re-added some context, CC Linus, Alexandre, linux-gpio)

On Tue, Oct 7, 2014 at 2:38 PM, Mark Brown <broonie@kernel.org> wrote:
> On Tue, Oct 07, 2014 at 02:32:57PM +0200, Geert Uytterhoeven wrote:
>> Unfortunately there's no equivalent code for platform data, and the
>> uninitialized default of 0 for gpio_hp_det and gpio_mic_det doesn't
>> play well with asm-generic's gpio_is_valid():
>>
>> static inline bool gpio_is_valid(int number)
>> {
>>         return number >= 0 && number < ARCH_NR_GPIOS;
>> }
>>
>> Hence on r8a7740/armadillo-legacy, which uses platform devices instead of DT:
>
>>     sh-mobile-hdmi sh-mobile-hdmi: SH Mobile HDMI Audio Codec
>>     sh-mobile-hdmi sh-mobile-hdmi: ASoC: DAPM unknown pin Headphones
>>     sh-mobile-hdmi sh-mobile-hdmi: ASoC: DAPM unknown pin Mic Jack
>
>> After that the kernel log is spammed ca. 7 times per second with:
>
>>     sh-mobile-hdmi sh-mobile-hdmi: ASoC: DAPM unknown pin Headphones
>
>> Reverting commit 3fe240326cc395c66 ("ASoC: simple-card: Add mic and
>> hp detect gpios.") fixes this.
>
> The fix here is to not allow 0 as a GPIO in the core code (which
> should've been there already).

Unfortunately it's not there.

And it's not as simple as changing the definition of gpio_is_valid()
(crash in gpio_get_value()):

    gpiochip_add: GPIOs 0..211 (r8a7740_pfc) failed to register
    sh-pfc pfc-r8a7740: failed to init GPIO chip, ignoring...
    sh-pfc pfc-r8a7740: r8a7740_pfc support registered
    Unable to handle kernel NULL pointer dereference at virtual address 0000004c

Quoting Linus (https://lkml.org/lkml/2014/9/4/464):
"Fixing the old global GPIO numberspace API is a waste of time IMO".

Hence I've just sent a patch to initialize the GPIO numbers with -ENOENT.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] ASoC: simple-card: Add mic and hp detect gpios.

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1089 bytes --]

On Tue, Oct 07, 2014 at 03:10:01PM +0200, Geert Uytterhoeven wrote:
> On Tue, Oct 7, 2014 at 2:38 PM, Mark Brown <broonie@kernel.org> wrote:

> > The fix here is to not allow 0 as a GPIO in the core code (which
> > should've been there already).

> Unfortunately it's not there.

> And it's not as simple as changing the definition of gpio_is_valid()
> (crash in gpio_get_value()):

>     gpiochip_add: GPIOs 0..211 (r8a7740_pfc) failed to register
>     sh-pfc pfc-r8a7740: failed to init GPIO chip, ignoring...
>     sh-pfc pfc-r8a7740: r8a7740_pfc support registered
>     Unable to handle kernel NULL pointer dereference at virtual address 0000004c

Right, obviously it's not going to work if the platform actually uses 0
as a valid GPIO.

> Quoting Linus (https://lkml.org/lkml/2014/9/4/464):
> "Fixing the old global GPIO numberspace API is a waste of time IMO".

> Hence I've just sent a patch to initialize the GPIO numbers with -ENOENT.

I do think it's worth renumbering the platforms since it's *relatively*
little work per platform compared to completing the gpiod transition.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH] ASoC: simple-card: Add mic and hp detect gpios.

[Alexandre Courbot]

On Wed, Oct 8, 2014 at 1:36 AM, Mark Brown <broonie@kernel.org> wrote:
> On Tue, Oct 07, 2014 at 03:10:01PM +0200, Geert Uytterhoeven wrote:
>> On Tue, Oct 7, 2014 at 2:38 PM, Mark Brown <broonie@kernel.org> wrote:
>
>> > The fix here is to not allow 0 as a GPIO in the core code (which
>> > should've been there already).
>
>> Unfortunately it's not there.
>
>> And it's not as simple as changing the definition of gpio_is_valid()
>> (crash in gpio_get_value()):
>
>>     gpiochip_add: GPIOs 0..211 (r8a7740_pfc) failed to register
>>     sh-pfc pfc-r8a7740: failed to init GPIO chip, ignoring...
>>     sh-pfc pfc-r8a7740: r8a7740_pfc support registered
>>     Unable to handle kernel NULL pointer dereference at virtual address 0000004c
>
> Right, obviously it's not going to work if the platform actually uses 0
> as a valid GPIO.
>
>> Quoting Linus (https://lkml.org/lkml/2014/9/4/464):
>> "Fixing the old global GPIO numberspace API is a waste of time IMO".
>
>> Hence I've just sent a patch to initialize the GPIO numbers with -ENOENT.
>
> I do think it's worth renumbering the platforms since it's *relatively*
> little work per platform compared to completing the gpiod transition.

But transition to gpiod is the way to ultimately fix this issue, as
well as many others. Not to mention that renumbering GPIOs will
certainly make a few users of the GPIO sysfs (another abomination,
agreed) unhappy. I can only recommend switching drivers to gpiod when
such issues are spotted.

Re: [PATCH] ASoC: simple-card: Add mic and hp detect gpios.

[Linus Walleij]

On Wed, Oct 8, 2014 at 9:05 AM, Alexandre Courbot <gnurou@gmail.com> wrote:
> On Wed, Oct 8, 2014 at 1:36 AM, Mark Brown <broonie@kernel.org> wrote:
>> On Tue, Oct 07, 2014 at 03:10:01PM +0200, Geert Uytterhoeven wrote:

>> Right, obviously it's not going to work if the platform actually uses 0
>> as a valid GPIO.
>>
>>> Quoting Linus (https://lkml.org/lkml/2014/9/4/464):
>>> "Fixing the old global GPIO numberspace API is a waste of time IMO".
>>
>>> Hence I've just sent a patch to initialize the GPIO numbers with -ENOENT.
>>
>> I do think it's worth renumbering the platforms since it's *relatively*
>> little work per platform compared to completing the gpiod transition.
>
> But transition to gpiod is the way to ultimately fix this issue, as
> well as many others. Not to mention that renumbering GPIOs will
> certainly make a few users of the GPIO sysfs (another abomination,
> agreed) unhappy. I can only recommend switching drivers to gpiod when
> such issues are spotted.

Yeah that is another issue ... we end up in catch 22 situations like
that, renumber the GPIOs, OK, then we break the ABI.

Admittedly that "ABI" is something people break all the time,
/sys/*gpioN just isnt what it should be, not stable at all.

I'm not against renumbering GPIO if it's minor effort, but if it
start to consume hundreds of hours and regressions and what not,
that time is better spent focusing on the gpiod transition.

Yours,
Linus Walleij

Re: [PATCH] ASoC: simple-card: Add mic and hp detect gpios.

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1172 bytes --]

On Wed, Oct 08, 2014 at 10:50:00AM +0200, Linus Walleij wrote:
> On Wed, Oct 8, 2014 at 9:05 AM, Alexandre Courbot <gnurou@gmail.com> wrote:

> > But transition to gpiod is the way to ultimately fix this issue, as
> > well as many others. Not to mention that renumbering GPIOs will
> > certainly make a few users of the GPIO sysfs (another abomination,
> > agreed) unhappy. I can only recommend switching drivers to gpiod when
> > such issues are spotted.

> Yeah that is another issue ... we end up in catch 22 situations like
> that, renumber the GPIOs, OK, then we break the ABI.

> Admittedly that "ABI" is something people break all the time,
> /sys/*gpioN just isnt what it should be, not stable at all.

> I'm not against renumbering GPIO if it's minor effort, but if it
> start to consume hundreds of hours and regressions and what not,
> that time is better spent focusing on the gpiod transition.

My guess is that it's relatively little work for most platforms - with
the systems I've done enough work on to notice everything is keyed off a
few defines in the header file.  Things tend to be worse in out of tree
code but mainline's generally been pretty good.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]