On Thu, 2011-01-06 at 13:54 -0600, Peter Tyser wrote:
> Add a new get_direction() function to the gpio_chip structure. This is
> useful so that the direction of a pin can be determined when its
> exported. Previously, the direction defaulted to 'in' regardless of the
> actual configuration of the GPIO pin which resulted in the 'direction'
> sysfs file often being incorrect.
>
> If a GPIO driver implements get_direction(), it is called in
> gpio_request() to set the initial direction of the pin accurately.
I see Grant was just added as a GPIO maintainer, so added him on CC.
Anything gating getting these 3 patches being picked up?
Thanks,
Peter
On Mon, Feb 14, 2011 at 8:48 AM, Peter Tyser <[email protected]> wrote:
> On Thu, 2011-01-06 at 13:54 -0600, Peter Tyser wrote:
>> Add a new get_direction() function to the gpio_chip structure. ?This is
>> useful so that the direction of a pin can be determined when its
>> exported. ?Previously, the direction defaulted to 'in' regardless of the
>> actual configuration of the GPIO pin which resulted in the 'direction'
>> sysfs file often being incorrect.
>>
>> If a GPIO driver implements get_direction(), it is called in
>> gpio_request() to set the initial direction of the pin accurately.
>
> I see Grant was just added as a GPIO maintainer, so added him on CC.
>
> Anything gating getting these 3 patches being picked up?
I'll take a look at them later today.
g.
> > If a GPIO driver implements get_direction(), it is called in
> > gpio_request() to set the initial direction of the pin accurately.
>
> I see Grant was just added as a GPIO maintainer, so added him on CC.
>
> Anything gating getting these 3 patches being picked up?
We need four states for a gpio pin direction though. A pin can be
- input
- output
- unknown (hardware lacks get functionality and it has not been set by
software yet)
- alt_func (pin is in use for some other purpose)
(and being able to set them alt_func was proposed a while ago and I think
wants revisiting judging by the number of platforms which use gpio, and
in their own arch code are privately handling alt_func stuff)
Alan
On Mon, Feb 14, 2011 at 10:08 AM, Alan Cox <[email protected]> wrote:
>> > If a GPIO driver implements get_direction(), it is called in
>> > gpio_request() to set the initial direction of the pin accurately.
>>
>> I see Grant was just added as a GPIO maintainer, so added him on CC.
>>
>> Anything gating getting these 3 patches being picked up?
>
> We need four states for a gpio pin direction though. A pin can be
>
> - input
> - output
There are actually multiple output modes that a specific gpio
controller could implement, but the gpio api only has a boolean
understanding of output. I don't know if it is really worthwhile to
try and encode all the possible configurations in this API.
> - unknown (hardware lacks get functionality and it has not been set by
> ?software yet)
> - alt_func (pin is in use for some other purpose)
What is the use-case for alt_func? From the point of view of a GPIO
driver, I don't think it cares if the pin has been dedicated to
something else. It can twiddle all it wants, but if the pin is routed
to something else then it won't have any external effects (pin mux is
often a separate logic block from the gpio controller). Also with
GPIOs, the engineers fiddling with them *really* needs to know what
the gpios are routed to. It is highly unlikely to have any kind of
automatic configuration of gpios; ie. if it isn't wired as a gpio,
then don't go twiddling it.
>
> (and being able to set them alt_func was proposed a while ago and I think
> wants revisiting judging by the number of platforms which use gpio, and
> in their own arch code are privately handling alt_func stuff)
Fair enough; convince me on alt_func. What is the use case that I'm missing?
g.
On Mon, Feb 14, 2011 at 10:26:18AM -0700, Grant Likely wrote:
> On Mon, Feb 14, 2011 at 10:08 AM, Alan Cox <[email protected]> wrote:
> > - input
> > - output
> There are actually multiple output modes that a specific gpio
> controller could implement, but the gpio api only has a boolean
> understanding of output. I don't know if it is really worthwhile to
> try and encode all the possible configurations in this API.
Ditto for inputs, and of course there's all the pad configuration stuff.
> > - alt_func (pin is in use for some other purpose)
> What is the use-case for alt_func? From the point of view of a GPIO
> driver, I don't think it cares if the pin has been dedicated to
> something else. It can twiddle all it wants, but if the pin is routed
> to something else then it won't have any external effects (pin mux is
> often a separate logic block from the gpio controller). Also with
Not always entirely true, some of the controllers I've worked with have
input and output both as alternate functions among the others there so
selecting input or output mode will tend to force the mode but...
> GPIOs, the engineers fiddling with them *really* needs to know what
> the gpios are routed to. It is highly unlikely to have any kind of
> automatic configuration of gpios; ie. if it isn't wired as a gpio,
> then don't go twiddling it.
...it doesn't make much difference for the reasons you mention.
> > We need four states for a gpio pin direction though. A pin can be
> >
> > - input
> > - output
>
> There are actually multiple output modes that a specific gpio
> controller could implement, but the gpio api only has a boolean
> understanding of output. I don't know if it is really worthwhile to
> try and encode all the possible configurations in this API.
>
> > - unknown (hardware lacks get functionality and it has not been set by
> > software yet)
I'm not sure how we could handle unknown directions in a better way. We
really should know the direction by this point for most (all?) GPIO
chips. I'd think the proper fix would be to make sure we can detect a
direction for all chips - either by reading hardware bits or by having
the platform code let us know (eg pdata->n_latch in pcf857x.c). If you
have a suggestion about how unknown pins should be used, I can look into
it and submit a follow up patch.
> > - alt_func (pin is in use for some other purpose)
>
> What is the use-case for alt_func? From the point of view of a GPIO
> driver, I don't think it cares if the pin has been dedicated to
> something else. It can twiddle all it wants, but if the pin is routed
> to something else then it won't have any external effects (pin mux is
> often a separate logic block from the gpio controller). Also with
> GPIOs, the engineers fiddling with them *really* needs to know what
> the gpios are routed to. It is highly unlikely to have any kind of
> automatic configuration of gpios; ie. if it isn't wired as a gpio,
> then don't go twiddling it.
Additionally, for this case I thought the low level GPIO driver should
implement a request() function to prevent a non-GPIO pin from being used
in the first place. Eg like chip_gpio_request() in cs5535-gpio.c, or
ichx_gpio_request() in patch 3 of this series.
> > (and being able to set them alt_func was proposed a while ago and I think
> > wants revisiting judging by the number of platforms which use gpio, and
> > in their own arch code are privately handling alt_func stuff)
>
> Fair enough; convince me on alt_func. What is the use case that I'm missing?
Peter
On Mon, Feb 14, 2011 at 10:45 AM, Peter Tyser <[email protected]> wrote:
>> > We need four states for a gpio pin direction though. A pin can be
>> >
>> > - input
>> > - output
>>
>> There are actually multiple output modes that a specific gpio
>> controller could implement, but the gpio api only has a boolean
>> understanding of output. ?I don't know if it is really worthwhile to
>> try and encode all the possible configurations in this API.
>>
>> > - unknown (hardware lacks get functionality and it has not been set by
>> > ?software yet)
>
> I'm not sure how we could handle unknown directions in a better way. ?We
> really should know the direction by this point for most (all?) GPIO
> chips. ?I'd think the proper fix would be to make sure we can detect a
> direction for all chips - either by reading hardware bits or by having
> the platform code let us know (eg pdata->n_latch in pcf857x.c). ?If you
> have a suggestion about how unknown pins should be used, I can look into
> it and submit a follow up patch.
Does it really matter? Sure it's *nice* to have the current status
information if the driver can easily get it, but it probably isn't
critical. Any user of the pin should be putting the pin in the mode
it needs regardless of the initial state.
>
>> > - alt_func (pin is in use for some other purpose)
>>
>> What is the use-case for alt_func? ?From the point of view of a GPIO
>> driver, I don't think it cares if the pin has been dedicated to
>> something else. ?It can twiddle all it wants, but if the pin is routed
>> to something else then it won't have any external effects (pin mux is
>> often a separate logic block from the gpio controller). ?Also with
>> GPIOs, the engineers fiddling with them *really* needs to know what
>> the gpios are routed to. ?It is highly unlikely to have any kind of
>> automatic configuration of gpios; ie. if it isn't wired as a gpio,
>> then don't go twiddling it.
>
> Additionally, for this case I thought the low level GPIO driver should
> implement a request() function to prevent a non-GPIO pin from being used
> in the first place. ?Eg like chip_gpio_request() in cs5535-gpio.c, or
> ichx_gpio_request() in patch 3 of this series.
Yes, *if* a driver has the knowledge that a pin is unusable, then it
should not allow the pin to be requested.
g.
On Mon, 2011-02-14 at 11:04 -0700, Grant Likely wrote:
> On Mon, Feb 14, 2011 at 10:45 AM, Peter Tyser <[email protected]> wrote:
> >> > We need four states for a gpio pin direction though. A pin can be
> >> >
> >> > - input
> >> > - output
> >>
> >> There are actually multiple output modes that a specific gpio
> >> controller could implement, but the gpio api only has a boolean
> >> understanding of output. I don't know if it is really worthwhile to
> >> try and encode all the possible configurations in this API.
> >>
> >> > - unknown (hardware lacks get functionality and it has not been set by
> >> > software yet)
> >
> > I'm not sure how we could handle unknown directions in a better way. We
> > really should know the direction by this point for most (all?) GPIO
> > chips. I'd think the proper fix would be to make sure we can detect a
> > direction for all chips - either by reading hardware bits or by having
> > the platform code let us know (eg pdata->n_latch in pcf857x.c). If you
> > have a suggestion about how unknown pins should be used, I can look into
> > it and submit a follow up patch.
>
> Does it really matter? Sure it's *nice* to have the current status
> information if the driver can easily get it, but it probably isn't
> critical. Any user of the pin should be putting the pin in the mode
> it needs regardless of the initial state.
I don't think it matters too much since I haven't encountered a driver
where you can't determine a pin direction yet. But if it were
impossible to determine a direction, it would throw people off. They'd
have to know that the pin direction couldn't be trusted initially, which
is a big leap (this is the bug I'm trying to fix with this patch). Eg
if someone sees a pin direction as "input" via sysfs, what are the odds
that they'll run "echo input > direction" just to make sure...
Anyway, I don't think its a critical issue either since its a corner
case with an easy workaround. If we did want to add support for
allowing a direction of "unknown", it could be in a follow up patch
since its technically a bigger issue than this patch.
Peter
On Mon, Feb 14, 2011 at 09:02:02AM -0700, Grant Likely wrote:
> On Mon, Feb 14, 2011 at 8:48 AM, Peter Tyser <[email protected]> wrote:
> > On Thu, 2011-01-06 at 13:54 -0600, Peter Tyser wrote:
> >> Add a new get_direction() function to the gpio_chip structure. ?This is
> >> useful so that the direction of a pin can be determined when its
> >> exported. ?Previously, the direction defaulted to 'in' regardless of the
> >> actual configuration of the GPIO pin which resulted in the 'direction'
> >> sysfs file often being incorrect.
> >>
> >> If a GPIO driver implements get_direction(), it is called in
> >> gpio_request() to set the initial direction of the pin accurately.
> >
> > I see Grant was just added as a GPIO maintainer, so added him on CC.
> >
> > Anything gating getting these 3 patches being picked up?
>
> I'll take a look at them later today.
What are the other two patches? I only see this one. Can you repost?
g.
> What is the use-case for alt_func? From the point of view of a GPIO
> driver, I don't think it cares if the pin has been dedicated to
Currently it doesn't.
However the moment it starts setting input and output itself on requests
then it does because it may kick the pin out of alt_func mode when you
merely want to request it so you know which pin to stick into alt_func
mode, or to find a mapping. The current situation is an "ignorance is
bliss" one, but making it smarter backfires. We have the same problem
with unknown state - if I have a set of pins some of whose state is known
at boot time then I can't now provide a get_direction interface because
of the lack of states. At the very least we need input/output/godknows
where the latter means the gpio_request code keeps its nose out.
reconfigure_resource();
see_if_we_own_it()
is simply the wrong order for a resource.
The second problem is that in many cases you need to call gpio_request to
know you have the pin yourself before you can set the direction. That
means forcing the direction in gpio_request is daft - you force an
undefined value that cannot yet safely be set in all cases.
At the moment the lack of alt_func also has some strange effects and you
end up with code like
foo_firmware_update()
{
/* Reserve the line for alt_func use for the moment */
gpio_request(GPIO_FOO, "blah");
random_gpio_driver_specific_altfunc_foo();
do stuff();
random_gpio_driver_specific_altfunc_foo();
gpio_free(GPIO_FOO);
/* Now available again for its normal GPIO use */
}
and that means you can't generalise dynamic access to a shared GPIO pin
without extra hardcoded knowledge.
In the case of a fixed pin its easy as you can either a) not register it
or b) just gpio_request it at boot and whack it in arch code, but if you
want to go beyond that it gets interesting.
Alan
On Mon, 2011-02-14 at 12:14 -0700, Grant Likely wrote:
> On Mon, Feb 14, 2011 at 09:02:02AM -0700, Grant Likely wrote:
> > On Mon, Feb 14, 2011 at 8:48 AM, Peter Tyser <[email protected]> wrote:
> > > On Thu, 2011-01-06 at 13:54 -0600, Peter Tyser wrote:
> > >> Add a new get_direction() function to the gpio_chip structure. This is
> > >> useful so that the direction of a pin can be determined when its
> > >> exported. Previously, the direction defaulted to 'in' regardless of the
> > >> actual configuration of the GPIO pin which resulted in the 'direction'
> > >> sysfs file often being incorrect.
> > >>
> > >> If a GPIO driver implements get_direction(), it is called in
> > >> gpio_request() to set the initial direction of the pin accurately.
> > >
> > > I see Grant was just added as a GPIO maintainer, so added him on CC.
> > >
> > > Anything gating getting these 3 patches being picked up?
> >
> > I'll take a look at them later today.
>
> What are the other two patches? I only see this one. Can you repost?
I just resent the 3 patches to you off-list. They can also be found at:
https://patchwork.kernel.org/patch/460411/
https://patchwork.kernel.org/patch/460321/
https://patchwork.kernel.org/patch/460331/
Peter
On Mon, 2011-02-14 at 19:35 +0000, Alan Cox wrote:
> > What is the use-case for alt_func? From the point of view of a GPIO
> > driver, I don't think it cares if the pin has been dedicated to
>
> Currently it doesn't.
>
> However the moment it starts setting input and output itself on requests
> then it does because it may kick the pin out of alt_func mode when you
> merely want to request it so you know which pin to stick into alt_func
> mode, or to find a mapping. The current situation is an "ignorance is
> bliss" one, but making it smarter backfires. We have the same problem
> with unknown state - if I have a set of pins some of whose state is known
> at boot time then I can't now provide a get_direction interface because
> of the lack of states. At the very least we need input/output/godknows
> where the latter means the gpio_request code keeps its nose out.
>
> reconfigure_resource();
> see_if_we_own_it()
>
> is simply the wrong order for a resource.
It looks like most drivers that implement a chip-specific request()
function do check if the gpio pin is available for use prior to
reconfiguring it. If a pin is reserved for an alt_func, the request
fails and the pin is not touched. Seems like adding a well coded
chip-specific request() function to drivers where necessary would
resolve your concern about reconfiguring pins before checking ownership?
> The second problem is that in many cases you need to call gpio_request to
> know you have the pin yourself before you can set the direction. That
> means forcing the direction in gpio_request is daft - you force an
> undefined value that cannot yet safely be set in all cases.
My understanding is that gpio_request() doesn't generally force a
direction, it just sets up the pin for use, or checks to make sure the
pin could be used. Most chip's don't have their own request() function
and thus don't touch hardware at all. For those that do have their own
request() function, they don't appear to force a value in many cases.
Also, in most cases I'd think that the BIOS/U-Boot/firmware should have
configured the GPIO pins appropriately, which Linux should inherit in
general. Linux currently inherits GPIO states that were set in firmware
when a GPIO is requested, but it doesn't properly report those values
via sysfs - that's the only bug I'm trying to fix.
> At the moment the lack of alt_func also has some strange effects and you
> end up with code like
>
> foo_firmware_update()
> {
> /* Reserve the line for alt_func use for the moment */
> gpio_request(GPIO_FOO, "blah");
> random_gpio_driver_specific_altfunc_foo();
> do stuff();
> random_gpio_driver_specific_altfunc_foo();
> gpio_free(GPIO_FOO);
>
> /* Now available again for its normal GPIO use */
> }
>
>
> and that means you can't generalise dynamic access to a shared GPIO pin
> without extra hardcoded knowledge.
Are there many cases where people need to swap a pin from GPIO to alt
functionality, and back again in Linux? I have never seen them used
like that; generally they are either wired up to an alt_func device (I2C
pins, serial pins, etc), or as a GPIO - not both dynamically. If some
hardware does need to do that, isn't it very chipset/board specific? I
guess I'm just not really grasping the big advantage of the alt_func
feature, or how it'd be implemented as common code. It looks like there
was a thread about it back in 2009 that didn't go anywhere:
http://thread.gmane.org/gmane.linux.kernel/851818
The current GPIO implementation has worked well for my usage scenarios,
so you may have to be blunt with me as far as what your looking for:)
Thanks,
Peter
> fails and the pin is not touched. Seems like adding a well coded
> chip-specific request() function to drivers where necessary would
> resolve your concern about reconfiguring pins before checking ownership?
That seems sensible and it keeps the knowledge out of gpiolib.
> Also, in most cases I'd think that the BIOS/U-Boot/firmware should have
> configured the GPIO pins appropriately, which Linux should inherit in
> general. Linux currently inherits GPIO states that were set in firmware
> when a GPIO is requested, but it doesn't properly report those values
> via sysfs - that's the only bug I'm trying to fix.
Yes - however you can't fix it unless you are prepared to admit that the
gpio has multiple states. At minimum you need to be able to report
input/output/unknown/unavailable
if you want to generalise it. Otherwise you don't solve the problem
because you are asking a question that driver cannot answer correctly.
> Are there many cases where people need to swap a pin from GPIO to alt
> functionality, and back again in Linux? I have never seen them used
> like that
Well I'm not surprised - you can't currently do it that way. The code
doesn't support it, instead such things are buried in arch code and
driver specific goo. I question whether it should be buried away like
that.
That's really about Grant's why do we need it comment rather than about
your bits. From the point of view of direction reporting (which I'm fully
in favour of) the only problem that matters is the fact a pin has a
minimum of four states to report. From that point of view "unavailable"
is probably a better way to report it than alt_func as it covers all the
other "unavailable" cases that may exist and haven't yet been thought of.
Alan
> > Also, in most cases I'd think that the BIOS/U-Boot/firmware should have
> > configured the GPIO pins appropriately, which Linux should inherit in
> > general. Linux currently inherits GPIO states that were set in firmware
> > when a GPIO is requested, but it doesn't properly report those values
> > via sysfs - that's the only bug I'm trying to fix.
>
> Yes - however you can't fix it unless you are prepared to admit that the
> gpio has multiple states. At minimum you need to be able to report
>
> input/output/unknown/unavailable
>
> if you want to generalise it. Otherwise you don't solve the problem
> because you are asking a question that driver cannot answer correctly.
As far as the "unknown" state, I can update the patch to have the logic:
+ if (chip->get_direction) {
+ /* chip->get_direction may sleep */
+ spin_unlock_irqrestore(&gpio_lock, flags);
+ if (chip->get_direction(chip, gpio - chip->base) > 0)
+ set_bit(FLAG_IS_OUT, &desc->flags);
+ spin_lock_irqsave(&gpio_lock, flags);
+ } else {
+ set_bit(FLAG_IS_UNKNOWN, &desc->flags);
+ }
This would have the side effect of having nearly all GPIO drivers
default to an "unknown" direction until they implement the new
get_direction() function, which I think is an improvement over the
current system where they are all unconditionally shown as "input",
often incorrectly. Are you OK with this Grant?
For the "unavailable" state, I didn't think it would be necessary. As
is, if someone calls gpio_request() on an invalid or alt_use pin, they
shouldn't get access to the GPIO, which makes the "unavailable value
moot since they couldn't access the GPIO in the first place.
Changing the logic to allow "unavailable" GPIO pins to be
requested/exported would require larger changes to the code, and
wouldn't provide much benefit without additional changes (eg an alt_func
feature). So I'd vote to not add support for "unavailable" pins in this
patch and rather wait until someone has a specific use for it, and add
it then.
Peter
> + if (chip->get_direction) {
> + /* chip->get_direction may sleep */
> + spin_unlock_irqrestore(&gpio_lock, flags);
> + if (chip->get_direction(chip, gpio - chip->base) > 0)
> + set_bit(FLAG_IS_OUT, &desc->flags);
> + spin_lock_irqsave(&gpio_lock, flags);
> + } else {
> + set_bit(FLAG_IS_UNKNOWN, &desc->flags);
> + }
>
> This would have the side effect of having nearly all GPIO drivers
> default to an "unknown" direction until they implement the new
> get_direction() function, which I think is an improvement over the
This doesn't solve anything. If the hardware supports alt_func state then
it now can't implement get_direction, so that's useless.
> For the "unavailable" state, I didn't think it would be necessary. As
> is, if someone calls gpio_request() on an invalid or alt_use pin, they
> shouldn't get access to the GPIO, which makes the "unavailable value
> moot since they couldn't access the GPIO in the first place.
In a word 'sysfs'
We need FLAG_IS_UNKNOWN (or saner would be FLAG_IS_IN to go with
FLAG_IS_OUT) to make the sysfs code report properly (and some other spots
fixing to make it work right)
If you add FLAG_IS_UNKNOWN then the other change you need is in
gpio_direction_show() which needs to also check the UNKNOWN bit and
report appropriately. That would fix that problem and at least allow the
reporting side of GPIO in use for something else to be handled as a
platform thing even though it can't be handled properly.
The combination of RESERVED & EXPORTED is also busted but is actually
sensible - that I gues should report reserved and fail the set operations.
On Tue, 2011-02-15 at 17:19 +0000, Alan Cox wrote:
> > + if (chip->get_direction) {
> > + /* chip->get_direction may sleep */
> > + spin_unlock_irqrestore(&gpio_lock, flags);
> > + if (chip->get_direction(chip, gpio - chip->base) > 0)
> > + set_bit(FLAG_IS_OUT, &desc->flags);
> > + spin_lock_irqsave(&gpio_lock, flags);
> > + } else {
> > + set_bit(FLAG_IS_UNKNOWN, &desc->flags);
> > + }
> >
> > This would have the side effect of having nearly all GPIO drivers
> > default to an "unknown" direction until they implement the new
> > get_direction() function, which I think is an improvement over the
>
> This doesn't solve anything. If the hardware supports alt_func state then
> it now can't implement get_direction, so that's useless.
I don't follow. If a pin is configured for some alternate function,
then requesting it for GPIO should fail, thus it doesn't matter if it
implements get_direction()? Since we can't easily toggle back and forth
between GPIO and alt_func, I'd think we shouldn't be able to request
alt_func pins for GPIO - they should be off-limits to the GPIO subsystem
altogether.
My understanding is that currently if some platform wants to toggle pins
back and forth between alt_func and GPIO, it needs to handle that logic
itself. If platform code is handling that toggling, I'd think the GPIO
code should not touch pins configured as alt_func. If the platform is
no longer using them as alt_func, then it should poke the appropriate
registers to make them not alt_func so that they can then be used by the
GPIO subsystem.
Maybe we disagree on the above point, which is adding to the confusion?
> > For the "unavailable" state, I didn't think it would be necessary. As
> > is, if someone calls gpio_request() on an invalid or alt_use pin, they
> > shouldn't get access to the GPIO, which makes the "unavailable value
> > moot since they couldn't access the GPIO in the first place.
>
> In a word 'sysfs'
>
> We need FLAG_IS_UNKNOWN (or saner would be FLAG_IS_IN to go with
> FLAG_IS_OUT) to make the sysfs code report properly (and some other spots
> fixing to make it work right)
Agreed.
> If you add FLAG_IS_UNKNOWN then the other change you need is in
>
> gpio_direction_show() which needs to also check the UNKNOWN bit and
> report appropriately.
Agreed.
> That would fix that problem and at least allow the
> reporting side of GPIO in use for something else to be handled as a
> platform thing even though it can't be handled properly.
I don't follow. I don't think I'm grasping what you want for alt_func
pins in the short term. Do you want them to be exported to the GPIO
sysfs filesystem and shown as "unavailable"? If so, what advantage does
that have over not allowing them to be exported/reserved in the first
place?
Peter
> My understanding is that currently if some platform wants to toggle pins
> back and forth between alt_func and GPIO, it needs to handle that logic
> itself. If platform code is handling that toggling, I'd think the GPIO
Yes - which is broken as you can't abstract it to the drivers which is
the whole point of GPIO. Anyway put that bit aside - for the get method
it's not actually important since we need an unknown state anyway and
alt_func is unknown or similar.
> > That would fix that problem and at least allow the
> > reporting side of GPIO in use for something else to be handled as a
> > platform thing even though it can't be handled properly.
>
> I don't follow. I don't think I'm grasping what you want for alt_func
> pins in the short term. Do you want them to be exported to the GPIO
> sysfs filesystem and shown as "unavailable"? If so, what advantage does
> that have over not allowing them to be exported/reserved in the first
> place?
You can see what state they are in. Otherwise you have to clone the GPIO
sysfs to expose private platform specific magic to indicate that.
Anyway first step is to allow 'Unknown' to be reported by a get_direction
method. The direction of a pin in some magic platform owned state is
defacto 'unknown'.
Not having a get_direction method doesn't help as we don't support
changing the methods on the fly (which is horrid for locking and best
kept that way) so we need a way to return input/output/beats me
On Mon, Feb 14, 2011 at 05:35:02PM -0600, Peter Tyser wrote:
> Also, in most cases I'd think that the BIOS/U-Boot/firmware should have
> configured the GPIO pins appropriately, which Linux should inherit in
> general. Linux currently inherits GPIO states that were set in firmware
> when a GPIO is requested, but it doesn't properly report those values
> via sysfs - that's the only bug I'm trying to fix.
That's one model for these things but it's *far* from universal.
Another model which is at least as common is that the bootloader does
the minimum possible to transfer control to Linux which then does the
actual configuration for the system.
> Are there many cases where people need to swap a pin from GPIO to alt
> functionality, and back again in Linux? I have never seen them used
> like that; generally they are either wired up to an alt_func device (I2C
> pins, serial pins, etc), or as a GPIO - not both dynamically. If some
> hardware does need to do that, isn't it very chipset/board specific? I
No, it's very rare. One example we have in the kernel is the PXA27x
AC'97 driver - the controller doesn't generate one of the resets
correctly so the driver puts the signals into GPIO mode and manually
generates the reset on the bus for the CODEC when it needs to do so.
> guess I'm just not really grasping the big advantage of the alt_func
> feature, or how it'd be implemented as common code. It looks like there
> was a thread about it back in 2009 that didn't go anywhere:
> http://thread.gmane.org/gmane.linux.kernel/851818
I tend to agree; I strongly expect that any alternate function code
would just wind up feeding at best device specific if not system
specific constants through and give us no more generality than we have
now.
On Tue, Feb 15, 2011 at 11:49:11AM -0600, Peter Tyser wrote:
> On Tue, 2011-02-15 at 17:19 +0000, Alan Cox wrote:
> > > + if (chip->get_direction) {
> > > + /* chip->get_direction may sleep */
> > > + spin_unlock_irqrestore(&gpio_lock, flags);
> > > + if (chip->get_direction(chip, gpio - chip->base) > 0)
> > > + set_bit(FLAG_IS_OUT, &desc->flags);
> > > + spin_lock_irqsave(&gpio_lock, flags);
> > > + } else {
> > > + set_bit(FLAG_IS_UNKNOWN, &desc->flags);
> > > + }
> > >
> > > This would have the side effect of having nearly all GPIO drivers
> > > default to an "unknown" direction until they implement the new
> > > get_direction() function, which I think is an improvement over the
> >
> > This doesn't solve anything. If the hardware supports alt_func state then
> > it now can't implement get_direction, so that's useless.
>
> I don't follow. If a pin is configured for some alternate function,
> then requesting it for GPIO should fail, thus it doesn't matter if it
> implements get_direction()? Since we can't easily toggle back and forth
> between GPIO and alt_func, I'd think we shouldn't be able to request
> alt_func pins for GPIO - they should be off-limits to the GPIO subsystem
> altogether.
hmm, I'm not sure. Letting gpio_request fail looks good from the POV of
an uninformed driver. But for some platform code, it seems more natural
to do:
gpio_request(mygpio);
myplatform_iomux_setup(pad_for_altfunc);
do_something_special();
/*
* the controler is unable to reset some component, so use
* bitbanging for that
*/
myplatform_iomux_setup(pad_for_gpio);
gpio_direction_output(mygpio, 0);
usleep(100);
myplatform_iomux_setup(pad_for_altfunc);
...
instead of only being able to gpio_request after
myplatform_iomux_setup(pad_for_gpio). (And so in theory take that risk
that another process grabs the gpio between mux-for-gpio and
gpio_request.) So if you ask me, it's gpio_direction_{in,out}put that
should fail, not gpio_request. But I'm not that sure about it to
already know now to keep this opinion until after this discussion is
over.
> My understanding is that currently if some platform wants to toggle pins
> back and forth between alt_func and GPIO, it needs to handle that logic
> itself. If platform code is handling that toggling, I'd think the GPIO
> code should not touch pins configured as alt_func. If the platform is
> no longer using them as alt_func, then it should poke the appropriate
> registers to make them not alt_func so that they can then be used by the
> GPIO subsystem.
.. or at least make the usage via the gpio subsystem fail using it.
OTOH on arm/plat-mxc (at least the newer chips) there is no easy mapping
between pads and gpios. So currently we do: gpio_request and
gpio_direction_{in,out}put yield 0, but it depends on the pin muxing if
the gpio is "visible" anywhere. I don't like that much, but I agree
that it's not worth to setup a huge table to map gpios to pads and back
just to return -ESOMETHING in the gpio functions.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
On Mon, Feb 14, 2011 at 07:35:02PM +0000, Alan Cox wrote:
> > What is the use-case for alt_func? From the point of view of a GPIO
> > driver, I don't think it cares if the pin has been dedicated to
>
> Currently it doesn't.
>
> However the moment it starts setting input and output itself on requests
> then it does because it may kick the pin out of alt_func mode when you
> merely want to request it so you know which pin to stick into alt_func
> mode, or to find a mapping. The current situation is an "ignorance is
> bliss" one, but making it smarter backfires. We have the same problem
> with unknown state - if I have a set of pins some of whose state is known
> at boot time then I can't now provide a get_direction interface because
> of the lack of states. At the very least we need input/output/godknows
> where the latter means the gpio_request code keeps its nose out.
Not quite; the gpio api is only about discrete gpios. If a particular
pin is dedicated to another non-gpio purpose, then from the POV of the
gpio api, the pin is disconnected from the outside world and any
twiddling of it just won't do anything. If an alt_func has any driver
behaviour impact, then it needs to be handled internal to the driver.
>
> reconfigure_resource();
> see_if_we_own_it()
>
> is simply the wrong order for a resource.
Yes, this is broken. gpio_request() should not change the state of
the resource. I don't see anything in gpiolib that currently does
this.
> The second problem is that in many cases you need to call gpio_request to
> know you have the pin yourself before you can set the direction. That
> means forcing the direction in gpio_request is daft - you force an
> undefined value that cannot yet safely be set in all cases.
>
> At the moment the lack of alt_func also has some strange effects and you
> end up with code like
>
> foo_firmware_update()
> {
> /* Reserve the line for alt_func use for the moment */
> gpio_request(GPIO_FOO, "blah");
> random_gpio_driver_specific_altfunc_foo();
> do stuff();
> random_gpio_driver_specific_altfunc_foo();
> gpio_free(GPIO_FOO);
>
> /* Now available again for its normal GPIO use */
> }
>
>
> and that means you can't generalise dynamic access to a shared GPIO pin
> without extra hardcoded knowledge.
I don't follow the argument. Of course you have to do weird hardcoded
things when a gpio pin has to be shared between multiple purposes, but
that is also a weird corner case that won't fit any kind of common
pattern. I don't see the above code snippet as a problem.
g.
On Tue, Feb 15, 2011 at 11:05:48AM -0600, Peter Tyser wrote:
> > > Also, in most cases I'd think that the BIOS/U-Boot/firmware should have
> > > configured the GPIO pins appropriately, which Linux should inherit in
> > > general. Linux currently inherits GPIO states that were set in firmware
> > > when a GPIO is requested, but it doesn't properly report those values
> > > via sysfs - that's the only bug I'm trying to fix.
> >
> > Yes - however you can't fix it unless you are prepared to admit that the
> > gpio has multiple states. At minimum you need to be able to report
> >
> > input/output/unknown/unavailable
> >
> > if you want to generalise it. Otherwise you don't solve the problem
> > because you are asking a question that driver cannot answer correctly.
>
> As far as the "unknown" state, I can update the patch to have the logic:
> + if (chip->get_direction) {
> + /* chip->get_direction may sleep */
> + spin_unlock_irqrestore(&gpio_lock, flags);
> + if (chip->get_direction(chip, gpio - chip->base) > 0)
> + set_bit(FLAG_IS_OUT, &desc->flags);
> + spin_lock_irqsave(&gpio_lock, flags);
> + } else {
> + set_bit(FLAG_IS_UNKNOWN, &desc->flags);
> + }
>
> This would have the side effect of having nearly all GPIO drivers
> default to an "unknown" direction until they implement the new
> get_direction() function, which I think is an improvement over the
> current system where they are all unconditionally shown as "input",
> often incorrectly. Are you OK with this Grant?
Not really, no. Defaulting to "input" may be incorrect, but it is
always safe, it it should only be a minor inconvenience to human users
of the sysfs interface. Actual usage of a gpio pin must always be to
explicitly set the direction before using a pin.
> Changing the logic to allow "unavailable" GPIO pins to be
> requested/exported would require larger changes to the code, and
> wouldn't provide much benefit without additional changes (eg an alt_func
> feature). So I'd vote to not add support for "unavailable" pins in this
> patch and rather wait until someone has a specific use for it, and add
> it then.
Agreed.
g.