Re: More GPIO madness on iMX6 - and the crappy ARM port of Linux
On Tue, Jan 21, 2014 at 3:55 AM, Alexandre Courbot wrote: > On Sat, Jan 18, 2014 at 7:43 AM, Linus Walleij > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: More GPIO madness on iMX6 - and the crappy ARM port of Linux
On Tue, Jan 21, 2014 at 3:55 AM, Alexandre Courbot gnu...@gmail.com wrote: On Sat, Jan 18, 2014 at 7:43 AM, Linus Walleij linus.wall...@linaro.org 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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: More GPIO madness on iMX6 - and the crappy ARM port of Linux
Hi, Alexandre Courbot wrote: > On Sat, Jan 18, 2014 at 7:43 AM, Linus Walleij > wrote: > > On Fri, Jan 17, 2014 at 9:53 PM, Russell King - ARM Linux > > 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 www.karo-electronics.de | i...@karo-electronics.de ___ -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: More GPIO madness on iMX6 - and the crappy ARM port of Linux
On Sat, Jan 18, 2014 at 7:43 AM, Linus Walleij wrote: > On Fri, Jan 17, 2014 at 9:53 PM, Russell King - ARM Linux > 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
Re: More GPIO madness on iMX6 - and the crappy ARM port of Linux
On Sat, Jan 18, 2014 at 7:43 AM, Linus Walleij linus.wall...@linaro.org wrote: On Fri, Jan 17, 2014 at 9:53 PM, Russell King - ARM Linux li...@arm.linux.org.uk 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
Re: More GPIO madness on iMX6 - and the crappy ARM port of Linux
Hi, Alexandre Courbot wrote: On Sat, Jan 18, 2014 at 7:43 AM, Linus Walleij linus.wall...@linaro.org wrote: On Fri, Jan 17, 2014 at 9:53 PM, Russell King - ARM Linux li...@arm.linux.org.uk 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 www.karo-electronics.de | i...@karo-electronics.de ___ -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: More GPIO madness on iMX6 - and the crappy ARM port of Linux
On Fri, Jan 17, 2014 at 9:53 PM, Russell King - ARM Linux 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: More GPIO madness on iMX6 - and the crappy ARM port of Linux
On Fri, Jan 17, 2014 at 9:53 PM, Russell King - ARM Linux li...@arm.linux.org.uk 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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
