On Fri, Jan 17, 2014 at 9:53 PM, Russell King - ARM Linux
<[email protected]> wrote:
> On Fri, Jan 17, 2014 at 01:42:44PM -0700, Stephen Warren wrote:
>> I believe you want gpio_get_value() to return either the driven or
>> actual pin value where it can on the current HW, but just e.g. hard-code
>> 0 on other HW. That would introduce a core feature that works some
>> places but not others, and hence make drivers that relied on the feature
>> less portable between HW with different actual features.
>
> I can buy that argument, but there's an issue which stands squarely in
> its way, and that is open-drain GPIOs.
>
> These are modelled just as any other GPIO, mainly so that both
> gpio_set_value(gpio, 1) and gpio_direction_input(gpio) both result in
> the signal being high. The only combination which results in the
> signal being driven low is outputting zero - and the state of the signal
> can aways be read back.
>
> The problem here is that such gpios are implemented in things like the
> I2C driver such that they're _always_ outputs, and gpio_set_value() is
> used to pull the signal down. gpio_get_value() is used to read its
> current state.
>
> So, if we say that gpio_get_value() is undefined, we force such
> subsystems to always jump through the non-open-drain paths (using
> gpio_direction_input() to set the line high and
> gpio_direction_output(gpio, 0) to drive it low.)
Incidentally that is what gpiolib is doing internally in
gpiod_direction_output().
You're absolutely right that it makes no sense to have open
drain (or open source) unless the signal can be read back from
the hardware.
I'm thinking something like if the driver manages to obtain a
GPIO with
gpio_request_one(gpio, GPIOF_OPEN_DRAIN |
GPIOF_OUT_INIT_HIGH);
As the I2C core does, and then when that call succeeds, it can
expect that whatever comes back from gpio_get_value() is
always what is actually on the line. If the driver cannot determine
this it should not have allowed that flag to succeed in the first
place, so this might be something we want to enforce.
There are two white spots on the map here:
1. Today this OPEN_DRAIN flag is not even passed down to
the driver so how could it say anything about it :-( it's a pure gpiolib
internal flag. We don't know if the hardware can actually even
do open drain, we just assume it can.
What it should really do - in the best of worlds - is to check if
it can cross-reference the GPIO line to a pin in the pin control
subsystem, and if that is possible, then ask the pin if it
is supporting open drain and set it. It currently has no such
cross-calls, it is just assumed that the configuration is consistent,
and the actual pin is set up as open drain. But it would make
sense to add more cross-calls here, since GPIO is accepting
these flags (OPEN_DRAIN/OPEN_SOURCE).
Like:
int pinctrl_gpio_set_flags(unsigned gpio, unsigned long flags);
Where the pinctrl subsystem would attempt to cross reference
and set the flag, and the pin controller backend will then have
the option to return an error code.
We could atleast support that for the select pin controllers
that use generic pin config. i.MX is another story, but I'm open
to compromises.
2. In the new descriptor API this open drain setting would
be set from the lookup table and be a property on the line,
meaning this flag is not requested explicitly by the consumer,
and the consumer needs to inspect the obtained descriptor
to figure out if it is set to open drain.
Alexandre: do you have plans for how to handle a dynamic
consumer passing flags to its gpio request in the gpiod API?
I noticed that API missing now, there is exactly one user in the
entire kernel, in drivers/i2c/i2c-core.c but a very important one.
I guess to switch the I2C core over to descriptors I could
think of an API like this:
int gpiod_get_flags(const struct gpio_desc *desc);
If the OPEN_DRAIN flag is set on that descriptor we should
always be able to read the input. But as this is not really what the
I2C core wants to know (it really would prefer not to bother with
such GPIO flag details) so is it better if we add a special call to
figure out if the input can be read? Like:
bool gpiod_input_always_valid(const struct gpio_desc *desc);
And leave it up to the core to look at flags, driver characteristics
etc and determine whether the input can be trusted?
Yours,
Linus Walleij
On Sat, Jan 18, 2014 at 7:43 AM, Linus Walleij <[email protected]> wrote:
> On Fri, Jan 17, 2014 at 9:53 PM, Russell King - ARM Linux
> <[email protected]> wrote:
>> On Fri, Jan 17, 2014 at 01:42:44PM -0700, Stephen Warren wrote:
>
>>> I believe you want gpio_get_value() to return either the driven or
>>> actual pin value where it can on the current HW, but just e.g. hard-code
>>> 0 on other HW. That would introduce a core feature that works some
>>> places but not others, and hence make drivers that relied on the feature
>>> less portable between HW with different actual features.
>>
>> I can buy that argument, but there's an issue which stands squarely in
>> its way, and that is open-drain GPIOs.
>>
>> These are modelled just as any other GPIO, mainly so that both
>> gpio_set_value(gpio, 1) and gpio_direction_input(gpio) both result in
>> the signal being high. The only combination which results in the
>> signal being driven low is outputting zero - and the state of the signal
>> can aways be read back.
>>
>> The problem here is that such gpios are implemented in things like the
>> I2C driver such that they're _always_ outputs, and gpio_set_value() is
>> used to pull the signal down. gpio_get_value() is used to read its
>> current state.
>>
>> So, if we say that gpio_get_value() is undefined, we force such
>> subsystems to always jump through the non-open-drain paths (using
>> gpio_direction_input() to set the line high and
>> gpio_direction_output(gpio, 0) to drive it low.)
>
> Incidentally that is what gpiolib is doing internally in
> gpiod_direction_output().
>
> You're absolutely right that it makes no sense to have open
> drain (or open source) unless the signal can be read back from
> the hardware.
>
> I'm thinking something like if the driver manages to obtain a
> GPIO with
>
> gpio_request_one(gpio, GPIOF_OPEN_DRAIN |
> GPIOF_OUT_INIT_HIGH);
>
> As the I2C core does, and then when that call succeeds, it can
> expect that whatever comes back from gpio_get_value() is
> always what is actually on the line. If the driver cannot determine
> this it should not have allowed that flag to succeed in the first
> place, so this might be something we want to enforce.
>
> There are two white spots on the map here:
>
> 1. Today this OPEN_DRAIN flag is not even passed down to
> the driver so how could it say anything about it :-( it's a pure gpiolib
> internal flag. We don't know if the hardware can actually even
> do open drain, we just assume it can.
>
> What it should really do - in the best of worlds - is to check if
> it can cross-reference the GPIO line to a pin in the pin control
> subsystem, and if that is possible, then ask the pin if it
> is supporting open drain and set it. It currently has no such
> cross-calls, it is just assumed that the configuration is consistent,
> and the actual pin is set up as open drain. But it would make
> sense to add more cross-calls here, since GPIO is accepting
> these flags (OPEN_DRAIN/OPEN_SOURCE).
This would definitely work in the case of pinctrl-backed GPIOs, but
would not cover all GPIO chips. If we want to cover all cases we
should give drivers a way to way to report or enforce this capability,
and make the pinctrl cross-reference one of its implementations where
it can be done.
>
> Like:
> int pinctrl_gpio_set_flags(unsigned gpio, unsigned long flags);
>
> Where the pinctrl subsystem would attempt to cross reference
> and set the flag, and the pin controller backend will then have
> the option to return an error code.
>
> We could atleast support that for the select pin controllers
> that use generic pin config. i.MX is another story, but I'm open
> to compromises.
>
> 2. In the new descriptor API this open drain setting would
> be set from the lookup table and be a property on the line,
> meaning this flag is not requested explicitly by the consumer,
> and the consumer needs to inspect the obtained descriptor
> to figure out if it is set to open drain.
>
> Alexandre: do you have plans for how to handle a dynamic
> consumer passing flags to its gpio request in the gpiod API?
Do you mean like passing OPEN_DRAIN or OPEN_SOURCE flags to
gpiod_get(), similarly to what is done for e.g. gpio_request_one()?
In the case of the gpiod API I would rather see these flags defined in
the GPIO mapping if possible. For platform data it is already possible
to specify open drain/open source, for DT this is trivial to add. ACPI
would be more of a problem here, but I'm not sure whether the problem
is relevant for ACPI GPIOs.
So the way I see it coming into shape would be something like:
1) GPIO drivers' request() function get an extra flags argument that
is passed by the GPIO core with the flags of the mapping. There we can
define all the range of properties that gpio_request_one() supported.
The driver's request() will fail it if cannot satisfy these
properties. That's where the pinctrl cross-reference would take place.
2) All properties accepted by gpio_request_one() can also be passed
through GPIO mappings. That is, probably platform_data and DT. I don't
know if we ever get to use open drain GPIOs provided by ACPI, if we do
then this might be a problem.
This does not address the initial problem, which is the uncertainty of
values returned by input GPIOs. For this either we enforce some more
strict rules, or we provide a function to allow drivers to check how
the value is reported, which is what you proposed below:
> I noticed that API missing now, there is exactly one user in the
> entire kernel, in drivers/i2c/i2c-core.c but a very important one.
>
> I guess to switch the I2C core over to descriptors I could
> think of an API like this:
>
> int gpiod_get_flags(const struct gpio_desc *desc);
>
> If the OPEN_DRAIN flag is set on that descriptor we should
> always be able to read the input. But as this is not really what the
> I2C core wants to know (it really would prefer not to bother with
> such GPIO flag details) so is it better if we add a special call to
> figure out if the input can be read? Like:
>
> bool gpiod_input_always_valid(const struct gpio_desc *desc);
>
> And leave it up to the core to look at flags, driver characteristics
> etc and determine whether the input can be trusted?
I am personally a little bit scared by the number of exported
functions in the GPIO framework. It is a pretty large number for
something that is supposed to be simple, so I'd like to avoid adding
more. :) How about the following:
1) GPIOs configured as output without the open drain or open source
flag either return -EINVAL on gpiod_get_value(), or a cached value
tracked by gpiolib for consistency (probably the latter).
2) GPIOs configured as open drain or open source report the actual
value read on the pin, like i2c-core needs. This requires that, for
each GPIO that can be set open drain or open source,
gpiod_input_always_valid() == true.
This is probably naive and needs to be refined, but I wonder if we
could not come with a relatively simple behavior that would lift
ambiguities without complexifying the API. Whatever we come with, we
will also need to think about how we can make the change without
breaking too many users.
Alex.
Hi,
Alexandre Courbot wrote:
> On Sat, Jan 18, 2014 at 7:43 AM, Linus Walleij <[email protected]> wrote:
> > On Fri, Jan 17, 2014 at 9:53 PM, Russell King - ARM Linux
> > <[email protected]> wrote:
> >> On Fri, Jan 17, 2014 at 01:42:44PM -0700, Stephen Warren wrote:
> >
[...]
> > If the OPEN_DRAIN flag is set on that descriptor we should
> > always be able to read the input. But as this is not really what the
> > I2C core wants to know (it really would prefer not to bother with
> > such GPIO flag details) so is it better if we add a special call to
> > figure out if the input can be read? Like:
> >
> > bool gpiod_input_always_valid(const struct gpio_desc *desc);
> >
> > And leave it up to the core to look at flags, driver characteristics
> > etc and determine whether the input can be trusted?
>
> I am personally a little bit scared by the number of exported
> functions in the GPIO framework. It is a pretty large number for
> something that is supposed to be simple, so I'd like to avoid adding
> more. :) How about the following:
>
> 1) GPIOs configured as output without the open drain or open source
> flag either return -EINVAL on gpiod_get_value(), or a cached value
> tracked by gpiolib for consistency (probably the latter).
> 2) GPIOs configured as open drain or open source report the actual
> value read on the pin, like i2c-core needs. This requires that, for
> each GPIO that can be set open drain or open source,
> gpiod_input_always_valid() == true.
>
I would not bind this to the open drain configuration. Any GPIO output
pin may actually be in a different state than programmed when the
output is forcefully driven by another source (shortcut).
So it makes sense to be able to read back the real state of the pad
even for push pull outputs.
Lothar Waßmann
--
___________________________________________________________
Ka-Ro electronics GmbH | Pascalstraße 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Geschäftsführer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996
http://www.karo-electronics.de | [email protected]
___________________________________________________________
On Tue, Jan 21, 2014 at 3:55 AM, Alexandre Courbot <[email protected]> wrote:
> On Sat, Jan 18, 2014 at 7:43 AM, Linus Walleij <[email protected]> wrote:
>> 1. Today this OPEN_DRAIN flag is not even passed down to
>> the driver so how could it say anything about it :-( it's a pure gpiolib
>> internal flag. We don't know if the hardware can actually even
>> do open drain, we just assume it can.
>>
>> What it should really do - in the best of worlds - is to check if
>> it can cross-reference the GPIO line to a pin in the pin control
>> subsystem, and if that is possible, then ask the pin if it
>> is supporting open drain and set it. It currently has no such
>> cross-calls, it is just assumed that the configuration is consistent,
>> and the actual pin is set up as open drain. But it would make
>> sense to add more cross-calls here, since GPIO is accepting
>> these flags (OPEN_DRAIN/OPEN_SOURCE).
>
> This would definitely work in the case of pinctrl-backed GPIOs, but
> would not cover all GPIO chips. If we want to cover all cases we
> should give drivers a way to way to report or enforce this capability,
> and make the pinctrl cross-reference one of its implementations where
> it can be done.
It can never be done in all cases, since in some cases the
open drain config is just a property of the line and not controlled
by software at all, and the datasheet just says this line is open
drain.
But we should cover the cases where we deal with pure
SW-controlled configs as far as possible.
>> Alexandre: do you have plans for how to handle a dynamic
>> consumer passing flags to its gpio request in the gpiod API?
>
> Do you mean like passing OPEN_DRAIN or OPEN_SOURCE flags to
> gpiod_get(), similarly to what is done for e.g. gpio_request_one()?
Yes...
> In the case of the gpiod API I would rather see these flags defined in
> the GPIO mapping if possible. For platform data it is already possible
> to specify open drain/open source, for DT this is trivial to add.
I figured so much. But we have a consumer in i2c-core.c doing
this for a valid reason.
> ACPI
> would be more of a problem here, but I'm not sure whether the problem
> is relevant for ACPI GPIOs.
ACPI has an open drain/source flag for some GPIO lines IIRC.
> 1) GPIO drivers' request() function get an extra flags argument that
> is passed by the GPIO core with the flags of the mapping. There we can
> define all the range of properties that gpio_request_one() supported.
> The driver's request() will fail it if cannot satisfy these
> properties. That's where the pinctrl cross-reference would take place.
I think this doesn't need to go all the way down into the driver
actually. We can call to pinctrl in the gpiolib core and
keep the gpiochip API simple. The GPIO driver doesn't even need
to know this.
> 2) All properties accepted by gpio_request_one() can also be passed
> through GPIO mappings. That is, probably platform_data and DT. I don't
> know if we ever get to use open drain GPIOs provided by ACPI, if we do
> then this might be a problem.
I think we need that.
>> Like:
>>
>> bool gpiod_input_always_valid(const struct gpio_desc *desc);
>>
>> And leave it up to the core to look at flags, driver characteristics
>> etc and determine whether the input can be trusted?
>
> I am personally a little bit scared by the number of exported
> functions in the GPIO framework. It is a pretty large number for
> something that is supposed to be simple, so I'd like to avoid adding
> more. :)
I objected already when the OPEN_DRAIN et al were added,
but to no avail...
> How about the following:
>
> 1) GPIOs configured as output without the open drain or open source
> flag either return -EINVAL on gpiod_get_value(), or a cached value
> tracked by gpiolib for consistency (probably the latter).
Make sense.
> 2) GPIOs configured as open drain or open source report the actual
> value read on the pin, like i2c-core needs. This requires that, for
> each GPIO that can be set open drain or open source,
> gpiod_input_always_valid() == true.
Yeah, but as seen from the I2C core, the algorithm there needs
to know if the input is always valid or not, and take different
execution paths depending on that. :-/
Yours,
Linus Walleij