[BUG] LPC32xx gpio driver broken by commit 762c2e46 in 4.9-rc1

[BUG] LPC32xx gpio driver broken by commit 762c2e46 in 4.9-rc1

[Sylvain Lemieux]

Vladimir, Linus, Alexandre,

the current LPC32xx GPIO driver is broken by commit 762c2e46
(gpio: of: remove of_gpiochip_and_xlate() and struct gg_data).

A call to "of_get_named_gpio" to retrieve the GPIO will
always return -EINVAL, except for the first GPIO bank.

Prior to this commit, the driver was working properly
because of the side-effect of the match function called by
"gpiochip_find" inside "of_get_named_gpiod_flags" function.

I think, the proper long-term solution is to replace the
LPC32xx GPIO driver; an initial version was previously
submitted, by Vladimir Zapolskiy, to the mailing list:
http://www.spinics.net/lists/linux-gpio/msg09746.html

Is there any short-term solution that can be done with
the existing driver to keep the LPC32xx platform working
properly in the 4.9 mainline kernel?


Regards,
Sylvain Lemieux


 

Re: [BUG] LPC32xx gpio driver broken by commit 762c2e46 in 4.9-rc1

[Vladimir Zapolskiy]

Hi Sylvain,

On 18.10.2016 19:23, Sylvain Lemieux wrote:
> Vladimir, Linus, Alexandre,
> 
> the current LPC32xx GPIO driver is broken by commit 762c2e46
> (gpio: of: remove of_gpiochip_and_xlate() and struct gg_data).

I do confirm, as well I've noticed that the driver is broken on v4.9, 
however I didn't find time to bisect the problematic commit, thank
you to pinning it out.

> A call to "of_get_named_gpio" to retrieve the GPIO will
> always return -EINVAL, except for the first GPIO bank.
> 
> Prior to this commit, the driver was working properly
> because of the side-effect of the match function called by
> "gpiochip_find" inside "of_get_named_gpiod_flags" function.
> 
> I think, the proper long-term solution is to replace the
> LPC32xx GPIO driver; an initial version was previously
> submitted, by Vladimir Zapolskiy, to the mailing list:
> http://www.spinics.net/lists/linux-gpio/msg09746.html

I still cherish a hope for submitting v2 for v4.10, the difference
from v1 is expected to be relatively big (e.g. there will be 5
banks instead of 6, on hardware level banks P0 and P1 are on the
single controller, there will be other lesser differences also).

> Is there any short-term solution that can be done with
> the existing driver to keep the LPC32xx platform working
> properly in the 4.9 mainline kernel?

Unfortunately I didn't spend enough time to fix the problem,
but in two words the root cause is that from the OF description
there is only one on-SoC GPIO controller, but the GPIO controller
driver registers multiple gpiochips (6 in this particular case),
consumers specify a bank as a value in the first cell.
The referenced commit simplifies the matter by assuming that
a number of gpiochips for consumers is the same as the number
of registered GPIO controllers from OF description.

I don't think that the problem is specific only to the legacy
LPC32xx GPIO controller driver, but at the moment I don't have
any more examples to share. Probably another 3-cell GPIO
controller driver gpio-etraxfs.c is also broken, a good enough
implicit indicator for potentially broken drivers might be if
you see gpiochip_add_data() call inside a loop:
* gpio-sch311x.c
* gpio-ml-ioh.c
* gpio-etraxfs.c
* gpio-htc-egpio.c
* gpio-davinci.c
* gpio-lpc32xx.c

--
With best wishes,
Vladimir

Re: [BUG] LPC32xx gpio driver broken by commit 762c2e46 in 4.9-rc1

[Sylvain Lemieux]

Hi Vladimir,

On Tue, 2016-10-18 at 21:06 +0300, Vladimir Zapolskiy wrote:
> Hi Sylvain,
> 
> On 18.10.2016 19:23, Sylvain Lemieux wrote:
> > Vladimir, Linus, Alexandre,
> > 
> > the current LPC32xx GPIO driver is broken by commit 762c2e46
> > (gpio: of: remove of_gpiochip_and_xlate() and struct gg_data).
> 
> I do confirm, as well I've noticed that the driver is broken on v4.9, 
> however I didn't find time to bisect the problematic commit, thank
> you to pinning it out.
> 
> > A call to "of_get_named_gpio" to retrieve the GPIO will
> > always return -EINVAL, except for the first GPIO bank.
> > 
> > Prior to this commit, the driver was working properly
> > because of the side-effect of the match function called by
> > "gpiochip_find" inside "of_get_named_gpiod_flags" function.
> > 
> > I think, the proper long-term solution is to replace the
> > LPC32xx GPIO driver; an initial version was previously
> > submitted, by Vladimir Zapolskiy, to the mailing list:
> > http://www.spinics.net/lists/linux-gpio/msg09746.html
> 
> I still cherish a hope for submitting v2 for v4.10, the difference
> from v1 is expected to be relatively big (e.g. there will be 5
> banks instead of 6, on hardware level banks P0 and P1 are on the
> single controller, there will be other lesser differences also).
> 
I will be available to test the new driver, once submitted
on the mailing list.

> > Is there any short-term solution that can be done with
> > the existing driver to keep the LPC32xx platform working
> > properly in the 4.9 mainline kernel?
> 
> Unfortunately I didn't spend enough time to fix the problem,
> but in two words the root cause is that from the OF description
> there is only one on-SoC GPIO controller, but the GPIO controller
> driver registers multiple gpiochips (6 in this particular case),
> consumers specify a bank as a value in the first cell.
> The referenced commit simplifies the matter by assuming that
> a number of gpiochips for consumers is the same as the number
> of registered GPIO controllers from OF description.
> 
> I don't think that the problem is specific only to the legacy
> LPC32xx GPIO controller driver, but at the moment I don't have
> any more examples to share. Probably another 3-cell GPIO
> controller driver gpio-etraxfs.c is also broken, a good enough
> implicit indicator for potentially broken drivers might be if
> you see gpiochip_add_data() call inside a loop:
> * gpio-sch311x.c
> * gpio-ml-ioh.c
> * gpio-etraxfs.c
> * gpio-htc-egpio.c
> * gpio-davinci.c
> * gpio-lpc32xx.c
> 
As a temporary solution, locally I reverted the following
commits to be able to have a working platform on 4.9-rc1:
* "gpio: of: factor out common code to a new helper function"
  (99468c1af913bb5662c223b68e783b4bf9200184)
* "gpio: of: remove of_gpiochip_and_xlate() and struct gg_data"
  (762c2e46c0591d207289105c8718e4adf29b2b34)


Regards,
Sylvain

Re: [BUG] LPC32xx gpio driver broken by commit 762c2e46 in 4.9-rc1

[Linus Walleij]

On Tue, Oct 18, 2016 at 6:23 PM, Sylvain Lemieux
<slemieux.tyco@gmail.com> wrote:

> the current LPC32xx GPIO driver is broken by commit 762c2e46
> (gpio: of: remove of_gpiochip_and_xlate() and struct gg_data).
>
> A call to "of_get_named_gpio" to retrieve the GPIO will
> always return -EINVAL, except for the first GPIO bank.
>
> Prior to this commit, the driver was working properly
> because of the side-effect of the match function called by
> "gpiochip_find" inside "of_get_named_gpiod_flags" function.
(...)
> Is there any short-term solution that can be done with
> the existing driver to keep the LPC32xx platform working
> properly in the 4.9 mainline kernel?

Masahiro, what do you think is the best course to proceed here?
Can we
- Restore the behaviour prior to the patches or
- Fix up all users or
- Do we have to revert these two patches?

I would prefer not to revert, because I liked the cleanup a lot ...

Yours,
Linus Walleij

Re: [BUG] LPC32xx gpio driver broken by commit 762c2e46 in 4.9-rc1

[Masahiro Yamada]

Hi.

2016-10-24 9:46 GMT+09:00 Linus Walleij <linus.walleij@linaro.org>:
> On Tue, Oct 18, 2016 at 6:23 PM, Sylvain Lemieux
> <slemieux.tyco@gmail.com> wrote:
>
>> the current LPC32xx GPIO driver is broken by commit 762c2e46
>> (gpio: of: remove of_gpiochip_and_xlate() and struct gg_data).
>>
>> A call to "of_get_named_gpio" to retrieve the GPIO will
>> always return -EINVAL, except for the first GPIO bank.
>>
>> Prior to this commit, the driver was working properly
>> because of the side-effect of the match function called by
>> "gpiochip_find" inside "of_get_named_gpiod_flags" function.
> (...)
>> Is there any short-term solution that can be done with
>> the existing driver to keep the LPC32xx platform working
>> properly in the 4.9 mainline kernel?
>
> Masahiro, what do you think is the best course to proceed here?
> Can we
> - Restore the behaviour prior to the patches or
> - Fix up all users or
> - Do we have to revert these two patches?
>
> I would prefer not to revert, because I liked the cleanup a lot ...
>


Personally, I do not want to revert, either.

I guess, this discussion comes down to
"is it justified to register multiple chips
associated to a single DT node?"


I feel like, DT properties such as
"gpio-hog", "gpio-ranges" assume one gpio_chip for one node.

We can register multi gpio_chip if we like, but
it looks odd to parse the same DT properties over and over again
looping gpio_chips.


If we move forward to single gpio_chip solution,
please check my RFC
"gpio: of: fix GPIO drivers with multiple gpio_chip for a single node"
as a long-term (but not too long) solution.



-- 
Best Regards
Masahiro Yamada