Re: [PATCH 4.16 269/272] pinctrl: msm: Use dynamic GPIO numbering

[Bjorn Andersson]

On Thu 31 May 04:45 PDT 2018, Greg Kroah-Hartman wrote:

> On Thu, May 31, 2018 at 01:21:56PM +0200, Sebastian Gottschall wrote:
> > Am 28.05.2018 um 12:05 schrieb Greg Kroah-Hartman:
> > > 4.16-stable review patch.  If anyone has any objections, please let me know.
> > > 
> > > ------------------
> > > 
> > > From: Bjorn Andersson <bjorn.andersson@xxxxxxxxxx>
> > > 
> > > [ Upstream commit a7aa75a2a7dba32594291a71c3704000a2fd7089 ]
> > > 
> > > The base of the TLMM gpiochip should not be statically defined as 0, fix
> > > this to not artificially restrict the existence of multiple pinctrl-msm
> > > devices.
> > > 
> > > Fixes: f365be092572 ("pinctrl: Add Qualcomm TLMM driver")
> > > Reported-by: Timur Tabi <timur@xxxxxxxxxxxxxx>
> > > Signed-off-by: Bjorn Andersson <bjorn.andersson@xxxxxxxxxx>
> > > Signed-off-by: Linus Walleij <linus.walleij@xxxxxxxxxx>
> > > Signed-off-by: Sasha Levin <alexander.levin@xxxxxxxxxxxxx>
> > > Signed-off-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> > > ---
> > >   drivers/pinctrl/qcom/pinctrl-msm.c |    2 +-
> > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > --- a/drivers/pinctrl/qcom/pinctrl-msm.c
> > > +++ b/drivers/pinctrl/qcom/pinctrl-msm.c
> > > @@ -818,7 +818,7 @@ static int msm_gpio_init(struct msm_pinc
> > >   		return -EINVAL;
> > >   	chip = &pctrl->chip;
> > > -	chip->base = 0;
> > > +	chip->base = -1;
> > >   	chip->ngpio = ngpio;
> > >   	chip->label = dev_name(pctrl->dev);
> > >   	chip->parent = pctrl->dev;
> > 
> > this patch creates a regression for me. on ipq8064 the systems gpios now
> > start somewhere in the sky
> > 
> > new layout
> > 
> > lrwxrwxrwx    1 root     root             0 Jan  1 00:00 gpiochip373 -> ../../devices/platform/soc/1b700000.pci/pci0001:00/0001:00:00.0/0001:01:00.0/gpio/gpiochip373
> > lrwxrwxrwx    1 root     root             0 Jan  1 00:00 gpiochip408 -> ../../devices/platform/soc/1b500000.pci/pci0000:00/0000:00:00.0/0000:01:00.0/gpio/gpiochip408
> > lrwxrwxrwx    1 root     root             0 Jan  1 00:00 gpiochip443 ->
> > ../../devices/platform/soc/800000.pinmux/gpio/gpiochip443
> > 
> > 
> > before the patch
> > 
> > lrwxrwxrwx    1 root     root             0 Jan  1  1970 gpiochip0 ->
> > ../../devices/platform/soc/800000.pinmux/gpio/gpiochip0
> > lrwxrwxrwx    1 root     root             0 May 31 13:13 gpiochip442 -> ../../devices/platform/soc/1b700000.pci/pci0001:00/0001:00:00.0/0001:01:00.0/gpio/gpiochip442
> > lrwxrwxrwx    1 root     root             0 May 31 13:13 gpiochip477 -> ../../devices/platform/soc/1b500000.pci/pci0000:00/0000:00:00.0/0000:01:00.0/gpio/gpiochip477
> > 
> > 
> > this broke my userspace gpio handling. i can override this, but still it
> > doesnt look correct since there is a hole at the beginng likelly reserved by
> > unused arm gpios
> 
> So does this mean that 4.17-rc is also broken for you?  If so, this
> needs to be reverted there as well.
> 
> Bjorn, any ideas?
> 

The Qualcomm TLMM (pinctrl) driver was introduced long after it was
decided that the kernel should not rely on global/static gpio numbering.

But due to a mistake on my part the dynamic gpio base for this gpiochip
was always 0, which the offending patch corrects.


So writing user space code relying on the gpio numbering for this chip
has always been incorrect, just as it is for the other two gpiochips
listed above.


The original objection for backporting this patch was a touchscreen
driver stopped working on 4.4, but the driver clearly doesn't depend on
global or static numbering of gpios. I don't think this was ever root
caused.


I guess the pragmatic approach would be to come up with something like
Timur's suggested fix that makes the first instance of this driver use
base = 0 and then use dynamic numbering for the rest, to maintain
support for this incorrect usage of the gpio sysfs interface. (Although
that would probably break if Timur's customers move their user space to
the new platform as "the first instance" isn't deterministic).

Regards,
Bjorn