diff mbox

gpio: omap: make gpio numbering deterministical by using of aliases

Message ID 1465898604-16294-1-git-send-email-u.kleine-koenig@pengutronix.de (mailing list archive)
State New, archived
Headers show

Commit Message

Uwe Kleine-König June 14, 2016, 10:03 a.m. UTC
Traditionally the n-th gpio device probed by the omap gpio driver got
the gpio number range [n*32 .. n*32+31].
When order of the devices probed by the driver changes (which can happen
already now when some devices have a pinctrl and so the first probe
attempt returns -ENODEV) the numbering changes.

To ensure a deterministical numbering use of_alias_get_id to determine
the number base for a given device. If no respective alias exists fall
back to the traditional numbering.

For the unusual case where only a part of the gpio devices have a
matching alias some of them might fail to probe. But if none of them has
an alias or all, there is no conflict which should be good enough to
maintain backward compatibility.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
Hello,

if you're happy with this patch I can follow up and add the respective
aliases to the device trees.

Best regards
Uwe

 drivers/gpio/gpio-omap.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

Comments

Grygorii Strashko June 14, 2016, 11:18 a.m. UTC | #1
Hi Uwe,

On 06/14/2016 01:03 PM, Uwe Kleine-König wrote:
> Traditionally the n-th gpio device probed by the omap gpio driver got
> the gpio number range [n*32 .. n*32+31].
> When order of the devices probed by the driver changes (which can happen
> already now when some devices have a pinctrl and so the first probe
> attempt returns -ENODEV) the numbering changes.
> 
> To ensure a deterministical numbering use of_alias_get_id to determine
> the number base for a given device. If no respective alias exists fall
> back to the traditional numbering.
> 
> For the unusual case where only a part of the gpio devices have a
> matching alias some of them might fail to probe. But if none of them has
> an alias or all, there is no conflict which should be good enough to
> maintain backward compatibility.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
> Hello,
> 
> if you're happy with this patch I can follow up and add the respective
> aliases to the device trees.
> 

Thanks for the patch.
I have no objection to the idea of the patch, but there are some comments.

> 
>   drivers/gpio/gpio-omap.c | 12 ++++++++++++
>   1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
> index b98ede78c9d8..6814245a54aa 100644
> --- a/drivers/gpio/gpio-omap.c
> +++ b/drivers/gpio/gpio-omap.c
> @@ -1034,6 +1034,7 @@ static int omap_gpio_chip_init(struct gpio_bank *bank, struct irq_chip *irqc)
>   	static int gpio;
>   	int irq_base = 0;
>   	int ret;
> +	int gpio_alias_id;
>   
>   	/*
>   	 * REVISIT eventually switch from OMAP-specific gpio structs
> @@ -1056,6 +1057,17 @@ static int omap_gpio_chip_init(struct gpio_bank *bank, struct irq_chip *irqc)
>   		bank->chip.label = "gpio";
>   		bank->chip.base = gpio;

I think, the gpio base correction should be done here

>   	}
> +
> +	/*
> +	 * Traditionally the base is given out in first-come-first-serve order.
> +	 * This might shuffle the numbering of gpios if the probe order changes.
> +	 * So make the base deterministical if the device tree specifies alias
> +	 * ids.
> +	 */
> +	gpio_alias_id = of_alias_get_id(bank->chip.of_node, "gpio");
> +	if (gpio_alias_id >= 0)
> +		bank->chip.base = bank->width * gpio_alias_id;
> +

Unfortunately, this driver is still used by non-DT platforms, so above code will
 break build if !OF && !OF_GPIO

I think, right way would be to get alias_id in .probe(), then calc base and save it
in struct gpio_bank. Then use it here to fixup GPIO base if positive.

>   	bank->chip.ngpio = bank->width;
>   
>   	ret = gpiochip_add_data(&bank->chip, bank);
>
Uwe Kleine-König June 14, 2016, 12:01 p.m. UTC | #2
Hello Grygorii,

On Tue, Jun 14, 2016 at 02:18:08PM +0300, Grygorii Strashko wrote:
> On 06/14/2016 01:03 PM, Uwe Kleine-König wrote:
> > diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
> > index b98ede78c9d8..6814245a54aa 100644
> > --- a/drivers/gpio/gpio-omap.c
> > +++ b/drivers/gpio/gpio-omap.c
> > @@ -1034,6 +1034,7 @@ static int omap_gpio_chip_init(struct gpio_bank *bank, struct irq_chip *irqc)
> >   	static int gpio;
> >   	int irq_base = 0;
> >   	int ret;
> > +	int gpio_alias_id;
> >   
> >   	/*
> >   	 * REVISIT eventually switch from OMAP-specific gpio structs
> > @@ -1056,6 +1057,17 @@ static int omap_gpio_chip_init(struct gpio_bank *bank, struct irq_chip *irqc)
> >   		bank->chip.label = "gpio";
> >   		bank->chip.base = gpio;
> 
> I think, the gpio base correction should be done here

What is the upside of doing it here? It has the downside that it needs
one more indention level. The only difference I see is that it doesn't
apply for mpuio devices then and wonder if that is relevant.
 
> >   	}
> > +
> > +	/*
> > +	 * Traditionally the base is given out in first-come-first-serve order.
> > +	 * This might shuffle the numbering of gpios if the probe order changes.
> > +	 * So make the base deterministical if the device tree specifies alias
> > +	 * ids.
> > +	 */
> > +	gpio_alias_id = of_alias_get_id(bank->chip.of_node, "gpio");
> > +	if (gpio_alias_id >= 0)
> > +		bank->chip.base = bank->width * gpio_alias_id;
> > +
> 
> Unfortunately, this driver is still used by non-DT platforms, so above code will
>  break build if !OF && !OF_GPIO

Without CONFIG_OF of_alias_get_id still exists and returns -ENOSYS which
should make the added code a nop. Does your compiler agree with you that
my patch breaks building the driver?

Best regards
Uwe
Grygorii Strashko June 14, 2016, 3:10 p.m. UTC | #3
On 06/14/2016 03:01 PM, Uwe Kleine-König wrote:
> Hello Grygorii,
> 
> On Tue, Jun 14, 2016 at 02:18:08PM +0300, Grygorii Strashko wrote:
>> On 06/14/2016 01:03 PM, Uwe Kleine-König wrote:
>>> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
>>> index b98ede78c9d8..6814245a54aa 100644
>>> --- a/drivers/gpio/gpio-omap.c
>>> +++ b/drivers/gpio/gpio-omap.c
>>> @@ -1034,6 +1034,7 @@ static int omap_gpio_chip_init(struct gpio_bank *bank, struct irq_chip *irqc)
>>>    	static int gpio;
>>>    	int irq_base = 0;
>>>    	int ret;
>>> +	int gpio_alias_id;
>>>    
>>>    	/*
>>>    	 * REVISIT eventually switch from OMAP-specific gpio structs
>>> @@ -1056,6 +1057,17 @@ static int omap_gpio_chip_init(struct gpio_bank *bank, struct irq_chip *irqc)
>>>    		bank->chip.label = "gpio";
>>>    		bank->chip.base = gpio;
>>
>> I think, the gpio base correction should be done here
> 
> What is the upside of doing it here? It has the downside that it needs
> one more indention level. The only difference I see is that it doesn't
> apply for mpuio devices then and wonder if that is relevant.

Personally, I prefer not ti touch mpuio code unless really required.

>   
>>>    	}
>>> +
>>> +	/*
>>> +	 * Traditionally the base is given out in first-come-first-serve order.
>>> +	 * This might shuffle the numbering of gpios if the probe order changes.
>>> +	 * So make the base deterministical if the device tree specifies alias
>>> +	 * ids.
>>> +	 */
>>> +	gpio_alias_id = of_alias_get_id(bank->chip.of_node, "gpio");
>>> +	if (gpio_alias_id >= 0)
>>> +		bank->chip.base = bank->width * gpio_alias_id;
>>> +
>>
>> Unfortunately, this driver is still used by non-DT platforms, so above code will
>>   break build if !OF && !OF_GPIO
> 
> Without CONFIG_OF of_alias_get_id still exists and returns -ENOSYS which
> should make the added code a nop. Does your compiler agree with you that
> my patch breaks building the driver?

make ARCH=arm omap1_defconfig
make ARCH=arm ./drivers/gpio/gpio-omap.o


drivers/gpio/gpio-omap.c: In function 'omap_gpio_chip_init':
drivers/gpio/gpio-omap.c:1066:44: error: 'struct gpio_chip' has no member named 'of_node'
  gpio_alias_id = of_alias_get_id(bank->chip.of_node, "gpio");
                                            ^
make[1]: *** [drivers/gpio/gpio-omap.o] Error 1
make: *** [drivers/gpio/gpio-omap.o] Error 2
kernel test robot June 15, 2016, midnight UTC | #4
Hi,

[auto build test ERROR on gpio/for-next]
[also build test ERROR on v4.7-rc3 next-20160614]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Uwe-Kleine-K-nig/gpio-omap-make-gpio-numbering-deterministical-by-using-of-aliases/20160614-180703
base:   https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-gpio.git for-next
config: arm-omap1_defconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (Debian 5.3.1-8) 5.3.1 20160205
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=arm 

All errors (new ones prefixed by >>):

   drivers/gpio/gpio-omap.c: In function 'omap_gpio_chip_init':
>> drivers/gpio/gpio-omap.c:1067:44: error: 'struct gpio_chip' has no member named 'of_node'
     gpio_alias_id = of_alias_get_id(bank->chip.of_node, "gpio");
                                               ^

vim +1067 drivers/gpio/gpio-omap.c

  1061		/*
  1062		 * Traditionally the base is given out in first-come-first-serve order.
  1063		 * This might shuffle the numbering of gpios if the probe order changes.
  1064		 * So make the base deterministical if the device tree specifies alias
  1065		 * ids.
  1066		 */
> 1067		gpio_alias_id = of_alias_get_id(bank->chip.of_node, "gpio");
  1068		if (gpio_alias_id >= 0)
  1069			bank->chip.base = bank->width * gpio_alias_id;
  1070	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
Linus Walleij June 15, 2016, 6:56 a.m. UTC | #5
On Tue, Jun 14, 2016 at 12:03 PM, Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:

> Traditionally the n-th gpio device probed by the omap gpio driver got
> the gpio number range [n*32 .. n*32+31].
> When order of the devices probed by the driver changes (which can happen
> already now when some devices have a pinctrl and so the first probe
> attempt returns -ENODEV) the numbering changes.
>
> To ensure a deterministical numbering use of_alias_get_id to determine
> the number base for a given device. If no respective alias exists fall
> back to the traditional numbering.

I'm not too happy about this approach.

The patch doesn't mention what practical problems it is trying to
solve.

I am very much suspecting the sysfs ABI to be what you're trying to
get consistent, and that is OBSOLETED, see
fe95046e960b4b76e73dc1486955d93f47276134
"gpio: ABI: mark the sysfs ABI as obsolete"

The GPIO numbering scheme is a matter of Linux internals and
not about hardware description IMO.

The way forward is to use the character device and use gpiochip
devices with offset indexes and look up GPIOs by name from the
character devices. If nothing substantial happens I am merging the
final pieces of the GPIO chardev ABI for v4.8 and that is doing all that
sysfs was doing and then some. I just need to change a small thing
before sending the final version for review.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Uwe Kleine-König June 15, 2016, 7:24 a.m. UTC | #6
Hello Linus,

On Wed, Jun 15, 2016 at 08:56:58AM +0200, Linus Walleij wrote:
> On Tue, Jun 14, 2016 at 12:03 PM, Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> 
> > Traditionally the n-th gpio device probed by the omap gpio driver got
> > the gpio number range [n*32 .. n*32+31].
> > When order of the devices probed by the driver changes (which can happen
> > already now when some devices have a pinctrl and so the first probe
> > attempt returns -ENODEV) the numbering changes.
> >
> > To ensure a deterministical numbering use of_alias_get_id to determine
> > the number base for a given device. If no respective alias exists fall
> > back to the traditional numbering.
> 
> I'm not too happy about this approach.
> 
> The patch doesn't mention what practical problems it is trying to
> solve.

the practical problem is that a (binary-only and hardware specific)
application uses GPIO47 via /sys/class/gpio and this number refers to
the wrong GPIO since I introduced gpio-hogs in the device tree together
with pinctrl stuff to make the gpio-hogs work.

I agree that this doesn't need to be a reason to care for it from a
mainline POV.

> The GPIO numbering scheme is a matter of Linux internals and
> not about hardware description IMO.

Not sure if I should agree here or not. It's very usual that the
"internal" gpio numbers match the hardware reference manual. I know this
from imx, at91, all pre-dt platforms, I'm sure there are more, and I bet
I'm not the only one relying on this for omap.

And this is very usual in the dt world, too:

$ git grep -El 'gpio. = \&gpio' arch/arm/boot/dts | wc -l
37

> The way forward is to use the character device and use gpiochip
> devices with offset indexes and look up GPIOs by name from the
> character devices. If nothing substantial happens I am merging the
> final pieces of the GPIO chardev ABI for v4.8 and that is doing all that
> sysfs was doing and then some. I just need to change a small thing
> before sending the final version for review.

Hmm, so /sys/class/gpio was obsoleted before the substitution was ready?
I'd say an overlapping of several kernel versions would be good as we
cannot expect that userspace changes as fast as the kernel.

Independent of the inclusion of my patch series in the mainline kernel
I'll have to use it on my custom kernel to keep the hardware do what it
should.

Best regards
Uwe
Tony Lindgren June 15, 2016, 8:27 a.m. UTC | #7
* Uwe Kleine-König <u.kleine-koenig@pengutronix.de> [160615 00:27]:
> On Wed, Jun 15, 2016 at 08:56:58AM +0200, Linus Walleij wrote:
> > The way forward is to use the character device and use gpiochip
> > devices with offset indexes and look up GPIOs by name from the
> > character devices. If nothing substantial happens I am merging the
> > final pieces of the GPIO chardev ABI for v4.8 and that is doing all that
> > sysfs was doing and then some. I just need to change a small thing
> > before sending the final version for review.
> 
> Hmm, so /sys/class/gpio was obsoleted before the substitution was ready?
> I'd say an overlapping of several kernel versions would be good as we
> cannot expect that userspace changes as fast as the kernel.

Well the /sys/class/gpio is an interface, and we'll have to maintain
it basically forever as we all know. It also works just fine for simple
things, so let's make sure it is usable. Having it inconsistent can
cause nasty side effects upgrading kernels.

Naturally I don't have any objections for adding a character device :)

Regards,

Tony
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Grygorii Strashko June 15, 2016, 9:56 a.m. UTC | #8
On 06/15/2016 11:27 AM, Tony Lindgren wrote:
> * Uwe Kleine-König <u.kleine-koenig@pengutronix.de> [160615 00:27]:
>> On Wed, Jun 15, 2016 at 08:56:58AM +0200, Linus Walleij wrote:
>>> The way forward is to use the character device and use gpiochip
>>> devices with offset indexes and look up GPIOs by name from the
>>> character devices. If nothing substantial happens I am merging the
>>> final pieces of the GPIO chardev ABI for v4.8 and that is doing all that
>>> sysfs was doing and then some. I just need to change a small thing
>>> before sending the final version for review.
>>
>> Hmm, so /sys/class/gpio was obsoleted before the substitution was ready?
>> I'd say an overlapping of several kernel versions would be good as we
>> cannot expect that userspace changes as fast as the kernel.
> 
> Well the /sys/class/gpio is an interface, and we'll have to maintain
> it basically forever as we all know. It also works just fine for simple
> things, so let's make sure it is usable. Having it inconsistent can
> cause nasty side effects upgrading kernels.
> 

Wouldn't gpio-line-names be helpful for such cases as mentioned in [1],[2]
But I don't know it these properties were exposed in legacy GPIO sysfs interface also.

[1] http://www.spinics.net/lists/linux-gpio/msg15040.html
[2] https://patchwork.ozlabs.org/patch/617713/
Linus Walleij June 18, 2016, 8:25 a.m. UTC | #9
On Wed, Jun 15, 2016 at 9:24 AM, Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
> On Wed, Jun 15, 2016 at 08:56:58AM +0200, Linus Walleij wrote:

>> The GPIO numbering scheme is a matter of Linux internals and
>> not about hardware description IMO.
>
> Not sure if I should agree here or not. It's very usual that the
> "internal" gpio numbers match the hardware reference manual. I know this
> from imx, at91, all pre-dt platforms, I'm sure there are more, and I bet
> I'm not the only one relying on this for omap.

I think it will still match nicely against the chip-local offsets of the
primary gpiochip so it'll be fine with a chardev too. The same was/is
the case of the first interrupts on x86 I think, but with the plethora of
irqchips and dependency on probe order etc the assumption is
nowadays to dangerous.

>
> And this is very usual in the dt world, too:
>
> $ git grep -El 'gpio. = \&gpio' arch/arm/boot/dts | wc -l
> 37

Aha I didn't even know. Well I guess I could allow it for OMAP too
then, but I want an ACK from one of the DT binding maintainers.

>> The way forward is to use the character device and use gpiochip
>> devices with offset indexes and look up GPIOs by name from the
>> character devices. If nothing substantial happens I am merging the
>> final pieces of the GPIO chardev ABI for v4.8 and that is doing all that
>> sysfs was doing and then some. I just need to change a small thing
>> before sending the final version for review.
>
> Hmm, so /sys/class/gpio was obsoleted before the substitution was ready?

Yeah, the sysfs was so broken that it could not be maintained.

> I'd say an overlapping of several kernel versions would be good as we
> cannot expect that userspace changes as fast as the kernel.

This is why it is scheduled for removal in 2020 and not tomorrow.

> Independent of the inclusion of my patch series in the mainline kernel
> I'll have to use it on my custom kernel to keep the hardware do what it
> should.

Nah well, let's discuss it.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Linus Walleij June 18, 2016, 8:29 a.m. UTC | #10
On Wed, Jun 15, 2016 at 10:27 AM, Tony Lindgren <tony@atomide.com> wrote:
> * Uwe Kleine-König <u.kleine-koenig@pengutronix.de> [160615 00:27]:
>> On Wed, Jun 15, 2016 at 08:56:58AM +0200, Linus Walleij wrote:
>> > The way forward is to use the character device and use gpiochip
>> > devices with offset indexes and look up GPIOs by name from the
>> > character devices. If nothing substantial happens I am merging the
>> > final pieces of the GPIO chardev ABI for v4.8 and that is doing all that
>> > sysfs was doing and then some. I just need to change a small thing
>> > before sending the final version for review.
>>
>> Hmm, so /sys/class/gpio was obsoleted before the substitution was ready?
>> I'd say an overlapping of several kernel versions would be good as we
>> cannot expect that userspace changes as fast as the kernel.
>
> Well the /sys/class/gpio is an interface, and we'll have to maintain
> it basically forever as we all know. It also works just fine for simple
> things, so let's make sure it is usable. Having it inconsistent can
> cause nasty side effects upgrading kernels.

The clever approach is that while sysfs GPIO has to be explicitly turned
on with a Kconfig, the character device is mandatory, and people can
rely on it always being present.

This will in my dreams encourage people to move toward using the
chardev.

Any removal of the sysfs depend on real-world users migrating to the
chardev though.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Linus Walleij June 18, 2016, 8:30 a.m. UTC | #11
On Wed, Jun 15, 2016 at 11:56 AM, Grygorii Strashko
<grygorii.strashko@ti.com> wrote:

> Wouldn't gpio-line-names be helpful for such cases as mentioned in [1],[2]
> But I don't know it these properties were exposed in legacy GPIO sysfs interface also.

The line names only expose through the chardev interface because the
sysfs ABI is frozen and should not be extended.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Uwe Kleine-König June 19, 2016, 1:08 a.m. UTC | #12
Hello Linus,

On Sat, Jun 18, 2016 at 10:25:45AM +0200, Linus Walleij wrote:
> On Wed, Jun 15, 2016 at 9:24 AM, Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> > On Wed, Jun 15, 2016 at 08:56:58AM +0200, Linus Walleij wrote:
> 
> >> The GPIO numbering scheme is a matter of Linux internals and
> >> not about hardware description IMO.
> >
> > Not sure if I should agree here or not. It's very usual that the
> > "internal" gpio numbers match the hardware reference manual. I know this
> > from imx, at91, all pre-dt platforms, I'm sure there are more, and I bet
> > I'm not the only one relying on this for omap.
> 
> I think it will still match nicely against the chip-local offsets of the
> primary gpiochip so it'll be fine with a chardev too. The same was/is

I cannot follow. What is the primary gpiochip? The first one? What is a
"chip-local offset". Just 3 for the fourth gpio of a given gpio bank?
I guess the problem is that I didn't follow development of the gpio
chardev.

> the case of the first interrupts on x86 I think, but with the plethora of
> irqchips and dependency on probe order etc the assumption is
> nowadays to dangerous.
> 
> >
> > And this is very usual in the dt world, too:
> >
> > $ git grep -El 'gpio. = \&gpio' arch/arm/boot/dts | wc -l
> > 37
> 
> Aha I didn't even know. Well I guess I could allow it for OMAP too
> then, but I want an ACK from one of the DT binding maintainers.

I added Rob, Frank, Mark and the device tree list to the recipients of
this mail. Can you please comment? There is already a v2 that you can
find at http://thread.gmane.org/gmane.linux.kernel.gpio/17399/ in case
it didn't hit your mailbox. If you tell me that you want it, I can also
bounce you the series.

Best regards
Uwe
Mark Rutland June 22, 2016, 4:16 p.m. UTC | #13
On Sun, Jun 19, 2016 at 03:08:23AM +0200, Uwe Kleine-König wrote:
> Hello Linus,
> 
> On Sat, Jun 18, 2016 at 10:25:45AM +0200, Linus Walleij wrote:
> > On Wed, Jun 15, 2016 at 9:24 AM, Uwe Kleine-König
> > <u.kleine-koenig@pengutronix.de> wrote:
> > > On Wed, Jun 15, 2016 at 08:56:58AM +0200, Linus Walleij wrote:
> > 
> > >> The GPIO numbering scheme is a matter of Linux internals and
> > >> not about hardware description IMO.
> > >
> > > Not sure if I should agree here or not. It's very usual that the
> > > "internal" gpio numbers match the hardware reference manual. I know this
> > > from imx, at91, all pre-dt platforms, I'm sure there are more, and I bet
> > > I'm not the only one relying on this for omap.
> > 
> > I think it will still match nicely against the chip-local offsets of the
> > primary gpiochip so it'll be fine with a chardev too. The same was/is
> 
> I cannot follow. What is the primary gpiochip? The first one? What is a
> "chip-local offset". Just 3 for the fourth gpio of a given gpio bank?
> I guess the problem is that I didn't follow development of the gpio
> chardev.

If I've understood correctly, Linus was on about the id space for GPIOs
under a particular gpiochip. If I've understand correctly, you're trying
to ensure consistent numbering the the *global* ID space shared by all
GPIO chips present in a system?

> > the case of the first interrupts on x86 I think, but with the plethora of
> > irqchips and dependency on probe order etc the assumption is
> > nowadays to dangerous.
> > 
> > > And this is very usual in the dt world, too:
> > >
> > > $ git grep -El 'gpio. = \&gpio' arch/arm/boot/dts | wc -l
> > > 37
> > 
> > Aha I didn't even know. Well I guess I could allow it for OMAP too
> > then, but I want an ACK from one of the DT binding maintainers.

In general, our use of aliases is rather ill-defined. It would be nicer
if we could address devices in a similar manner to disks or partitions,
e.g. by path or uuid, but I don't think we have anything sensible we can
use there.

Given that, I can see the use of an alias to provide a consistent way of
referring to a particular gpiochip (and maybe we need to expose the
alises information somehow to userspace), but IMO that's independent of
any global ID space, probe ordering, etc.

Thanks,
Mark.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Linus Walleij June 23, 2016, 9:04 a.m. UTC | #14
On Wed, Jun 22, 2016 at 6:16 PM, Mark Rutland <mark.rutland@arm.com> wrote:

> In general, our use of aliases is rather ill-defined. It would be nicer
> if we could address devices in a similar manner to disks or partitions,
> e.g. by path or uuid, but I don't think we have anything sensible we can
> use there.
>
> Given that, I can see the use of an alias to provide a consistent way of
> referring to a particular gpiochip (and maybe we need to expose the
> alises information somehow to userspace), but IMO that's independent of
> any global ID space, probe ordering, etc.

From the kernel point of view the way forward to identify and refer to
a particular gpiochip is using /dev/gpiochipN the character device.

If complete topology of the bus placement etc is needed, userspace
can traverse /sys/bus/gpio/*

This solves the big problem with the current global numbering system
in /sys/class/gpio/*

So what this alias should address would be two things:

- Solve the immediate issue of the global number space for the
  legacy sysfs ABI, but also:

- Determine which chip is gpiochip0, gpiochip1, .. etc in the
  new ABI, so the devices get consistent numbering.

The latter is lightly frowned upon by the udev people: they think it
is more proper to traverse /sys to get topological information about
the devices.

I would appreciate if a patch to add alias handling would take care
of both these things if we apply it.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Grygorii Strashko June 23, 2016, 9:38 a.m. UTC | #15
On 06/23/2016 12:04 PM, Linus Walleij wrote:
> On Wed, Jun 22, 2016 at 6:16 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> 
>> In general, our use of aliases is rather ill-defined. It would be nicer
>> if we could address devices in a similar manner to disks or partitions,
>> e.g. by path or uuid, but I don't think we have anything sensible we can
>> use there.
>>
>> Given that, I can see the use of an alias to provide a consistent way of
>> referring to a particular gpiochip (and maybe we need to expose the
>> alises information somehow to userspace), but IMO that's independent of
>> any global ID space, probe ordering, etc.
> 
>  From the kernel point of view the way forward to identify and refer to
> a particular gpiochip is using /dev/gpiochipN the character device.
> 
> If complete topology of the bus placement etc is needed, userspace
> can traverse /sys/bus/gpio/*
> 
> This solves the big problem with the current global numbering system
> in /sys/class/gpio/*

Hm. May be i misunderstood samthing (and sry, if my following question
is dummy as I've not followed closely new GPIO ABI development), but..

- from above description it seems that  global numbering system is not really
solved :( instead it's been moved one level up and now the same happens with
gpiocipX devices :( Wouldn't it be reasonable to add possibility to create named/labeled
gpiocipX devices from the very beginning, like: "/dev/gpiocipX[_name|label], or "/dev/gpiocip[_name|label]", or ..

Actually, struct gpio_chip has label field already.

PS. From my experience, the worst case with dev numbering usually happens after 
few insmod/rmmod (or sometimes after suspend/resume) iterations - ttyX ;..(.

> 
> So what this alias should address would be two things:
> 
> - Solve the immediate issue of the global number space for the
>    legacy sysfs ABI, but also:
> 
> - Determine which chip is gpiochip0, gpiochip1, .. etc in the
>    new ABI, so the devices get consistent numbering.
> 
> The latter is lightly frowned upon by the udev people: they think it
> is more proper to traverse /sys to get topological information about
> the devices.
> 
> I would appreciate if a patch to add alias handling would take care
> of both these things if we apply it.
>
Linus Walleij June 23, 2016, 12:08 p.m. UTC | #16
On Thu, Jun 23, 2016 at 11:38 AM, Grygorii Strashko
<grygorii.strashko@ti.com> wrote:

>> This solves the big problem with the current global numbering system
>> in /sys/class/gpio/*
>
> Hm. May be i misunderstood samthing (and sry, if my following question
> is dummy as I've not followed closely new GPIO ABI development), but..
>
> - from above description it seems that  global numbering system is not really
> solved :( instead it's been moved one level up and now the same happens with
> gpiocipX devices :(

Yes that is true, but also the discovery mechanism is changed so it should
not be a problem: by traversing /sys/bus/gpio/* you know from the topology
information which GPIO chip is which. This is the same method that e.g. USB,
IIO or disks use: you have to complement the device name with bus information
from sysfs.

/sys/class/gpio/* on the other hand has no such information: it is flat.

But as I wrote in the other mail in response to Mark Rutland: if it is desired
to have predictable numbering of the GPIO chips the exact same alias
method can be used for that, and a patch utilizing aliases should fix also
this for the character device usecase.

My own position on these aliases is "eating popcorn", I'm happy with it,
happy without it, the people who care need to argue for it with the DT
maintainers.

> Wouldn't it be reasonable to add possibility to create named/labeled
> gpiocipX devices from the very beginning, like: "/dev/gpiocipX[_name|label], or "/dev/gpiocip[_name|label]", or ..
>
> Actually, struct gpio_chip has label field already.

I don't think that would be nice. There is an ioctl() to get that information
(amongst other stuff) from the device. Check tools/gpio/lsgpio.c

> PS. From my experience, the worst case with dev numbering usually happens after
> few insmod/rmmod (or sometimes after suspend/resume) iterations - ttyX ;..(.

In the GPIO case I think the numbers will be reused as we're using
<linux/idr.h> infrastructure (same method as IIO).

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index b98ede78c9d8..6814245a54aa 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -1034,6 +1034,7 @@  static int omap_gpio_chip_init(struct gpio_bank *bank, struct irq_chip *irqc)
 	static int gpio;
 	int irq_base = 0;
 	int ret;
+	int gpio_alias_id;
 
 	/*
 	 * REVISIT eventually switch from OMAP-specific gpio structs
@@ -1056,6 +1057,17 @@  static int omap_gpio_chip_init(struct gpio_bank *bank, struct irq_chip *irqc)
 		bank->chip.label = "gpio";
 		bank->chip.base = gpio;
 	}
+
+	/*
+	 * Traditionally the base is given out in first-come-first-serve order.
+	 * This might shuffle the numbering of gpios if the probe order changes.
+	 * So make the base deterministical if the device tree specifies alias
+	 * ids.
+	 */
+	gpio_alias_id = of_alias_get_id(bank->chip.of_node, "gpio");
+	if (gpio_alias_id >= 0)
+		bank->chip.base = bank->width * gpio_alias_id;
+
 	bank->chip.ngpio = bank->width;
 
 	ret = gpiochip_add_data(&bank->chip, bank);