| Message ID | 1329114588-15430-3-git-send-email-ldewangan@nvidia.com |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On Mon, Feb 13, 2012 at 11:59:47AM +0530, Laxman Dewangan wrote: > Adding details of open drain configuration of the gpio so that > client can set the pin as open drain at the time of gpio request. > > Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com> Implicitly, the gpio api already supports open-drain and several drivers make use of it. Instead of being a separate flag; users who need open drain will set the pin to input. For example, see the i2c-gpio driver. I'm not convinced this is needed; but my opinion could be swayed. Linus mentioned that this should be part of pinctrl instead of the gpio API, but I think there is an argument for making it part of the gpio API, particularly since open-drain is pretty much a universal concept that all gpio controllers can support (unlike driver strength) and as I said above, it is already implicitly supported by gpiolib. The difference with this method is it would allow drivers like the gpio-i2c.c driver to set the flag at gpio request time and then be able to always use gpio_set_value() regardless of the pin mode. However, I'm not thrilled about adding things to the already-horrible sysfs abi. Please drop that hunk entirely or put it into a separate patch so it doesn't block the core functionality. Also, you should include a patch to make i2c-gpio.c use this new functionality as a proof-of-concept. You may not be able to test it, but it will make it a lot easier to justify merging by showing how it cleans up a gpio user. Have you though about support for lines that are pulled low instead of high? Those aren't as common, but it is conceivable that some hardware would need it. g. > --- > Documentation/gpio.txt | 21 +++++++++++++++++++-- > 1 files changed, 19 insertions(+), 2 deletions(-) > > diff --git a/Documentation/gpio.txt b/Documentation/gpio.txt > index 792faa3..b08933c 100644 > --- a/Documentation/gpio.txt > +++ b/Documentation/gpio.txt > @@ -302,6 +302,7 @@ where 'flags' is currently defined to specify the following properties: > > * GPIOF_INIT_LOW - as output, set initial level to LOW > * GPIOF_INIT_HIGH - as output, set initial level to HIGH > + * GPIOF_OD - gpio pin is open drain type. > > since GPIOF_INIT_* are only valid when configured as output, so group valid > combinations as: > @@ -310,8 +311,7 @@ combinations as: > * GPIOF_OUT_INIT_LOW - configured as output, initial level LOW > * GPIOF_OUT_INIT_HIGH - configured as output, initial level HIGH > > -In the future, these flags can be extended to support more properties such > -as open-drain status. > +In the future, these flags can be extended to support more properties. > > Further more, to ease the claim/release of multiple GPIOs, 'struct gpio' is > introduced to encapsulate all three fields as: > @@ -641,6 +641,13 @@ and have the following read/write attributes: > for "rising" and "falling" edges will follow this > setting. > > + "open_drain" ... reads as either 0 (false) or 1 (true). Write > + any nonzero value to make the pin in open drain. > + By setting open drain to true, the output can be set > + to HIGH by external PULL UP and setting direction to input. > + The output will be set to LOW by setting direction to > + output with value is 0. > + > GPIO controllers have paths like /sys/class/gpio/gpiochip42/ (for the > controller implementing GPIOs starting at #42) and have the following > read-only attributes: > @@ -679,6 +686,9 @@ requested using gpio_request(): > /* change the polarity of a GPIO node in sysfs */ > int gpio_sysfs_set_active_low(unsigned gpio, int value); > > + /* change the pin to open drain in sysfs */ > + int gpio_sysfs_set_open_drain(unsigned gpio, int value); > + > After a kernel driver requests a GPIO, it may only be made available in > the sysfs interface by gpio_export(). The driver can control whether the > signal direction may change. This helps drivers prevent userspace code > @@ -698,3 +708,10 @@ differences between boards from user space. This only affects the > sysfs interface. Polarity change can be done both before and after > gpio_export(), and previously enabled poll(2) support for either > rising or falling edge will be reconfigured to follow this setting. > + > +Drivers can use gpio_sysfs_set_open_drain() to enable/disable the open > +drain property of that pins. This only affect the sysfs interface. > +The flag will be set as open drain if thsi function is called with value > +of 1. It is recommended to set the open drain property before setting > +the value in output mode so that pin state cn be set properly based > +on the value. > -- > 1.7.1.1 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Feb 13, 2012 at 02:18:09PM -0700, Grant Likely wrote: > On Mon, Feb 13, 2012 at 11:59:47AM +0530, Laxman Dewangan wrote: > > Adding details of open drain configuration of the gpio so that > > client can set the pin as open drain at the time of gpio request. > Implicitly, the gpio api already supports open-drain and several drivers > make use of it. Instead of being a separate flag; users who need open > drain will set the pin to input. For example, see the i2c-gpio driver. > I'm not convinced this is needed; but my opinion could be swayed. The actual idea is to provide support for doing the switch to input to drivers that just want to set a logic level and don't (at their level) care one way or another if it's a CMOS or open drain output but instead leaves it up to board configuration which mode is used. Laxman posted a patch for doing this to a regulator driver but looking at it the code for emulating open drain while also maintaining support for regular CMOS is fiddly enough that it seemed like it should be factored out of the individual drivers. > Also, you should include a patch to make i2c-gpio.c use this new > functionality as a proof-of-concept. You may not be able to test it, > but it will make it a lot easier to justify merging by showing how it > cleans up a gpio user. The regulator patch is one example - I think things that could be CMOS are probably more interesting here than drivers that always want open drain as they have more of a complexity hit from needing to decide if they'll use the emulation or not. We could also at some later point add support for hardware which can implement open drain mode itself though I'm not sure if there's enough problem with emulating to make it worth the effort.
On Tuesday 14 February 2012 03:36 AM, Mark Brown wrote: > * PGP Signed by an unknown key > >> Implicitly, the gpio api already supports open-drain and several drivers >> make use of it. Instead of being a separate flag; users who need open >> drain will set the pin to input. For example, see the i2c-gpio driver. >> I'm not convinced this is needed; but my opinion could be swayed. > The actual idea is to provide support for doing the switch to input to > drivers that just want to set a logic level and don't (at their level) > care one way or another if it's a CMOS or open drain output but instead > leaves it up to board configuration which mode is used. Laxman posted a > patch for doing this to a regulator driver but looking at it the code > for emulating open drain while also maintaining support for regular CMOS > is fiddly enough that it seemed like it should be factored out of the > individual drivers. > Implementing open-drain handling in every gpio client (like i2c-gpio/regulator/fixed.c) is just duplicating the code everywhere. Also it leaves to have such implementation buggy which is in following piece of code (taken from i2c-gpio.c) if (pdata->sda_is_open_drain) { gpio_direction_output(pdata->sda_pin, 1); bit_data->setsda = i2c_gpio_setsda_val; } else { gpio_direction_input(pdata->sda_pin); bit_data->setsda = i2c_gpio_setsda_dir; } Header says * @sda_is_open_drain: SDA is configured as open drain, i.e. the pin * isn't actively driven high when setting the output value high. * gpio_get_value() must return the actual pin state even if the * pin is configured as an output. And hence the check should be if (!pdata->sda_is_open_drain) { ::::: } All these can be avoided by supporting open-drain flag and handling in gpio library. >> Also, you should include a patch to make i2c-gpio.c use this new >> functionality as a proof-of-concept. You may not be able to test it, >> but it will make it a lot easier to justify merging by showing how it >> cleans up a gpio user. I can do the cleanup in i2c-gpio also which will reduce lots of code if this change in gpiolib is acceptable and fine with everyone. Let me know if patch regulator/fixed.c V1 is sufficient or not to justify. I will create the patch for i2c-gpio also. -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday 14 February 2012 02:48 AM, Grant Likely wrote: > On Mon, Feb 13, 2012 at 11:59:47AM +0530, Laxman Dewangan wrote: >> Adding details of open drain configuration of the gpio so that >> client can set the pin as open drain at the time of gpio request. >> >> Signed-off-by: Laxman Dewangan<ldewangan@nvidia.com> > > Linus mentioned that this should be part of pinctrl instead of the gpio API, > but I think there is an argument for making it part of the gpio API, > particularly since open-drain is pretty much a universal concept that all > gpio controllers can support (unlike driver strength) and as I said above, it > is already implicitly supported by gpiolib. I am not sure about other soc but taking Tegra as example, the gpio controller is almost same for same series of soc. It does not very much about different version of the socs. The pins, number of pins. pin controls, pin is OD or not etc vary from variant of socs and so it is much more depends on the particular soc rather than the major family. Bringing pincontrol information in gpio driver will make the gpio driver complex which need to take care of every variant of chips. > The difference with this > method is it would allow drivers like the gpio-i2c.c driver to set the > flag at gpio request time and then be able to always use gpio_set_value() > regardless of the pin mode. > > However, I'm not thrilled about adding things to the already-horrible sysfs > abi. Please drop that hunk entirely or put it into a separate patch so it > doesn't block the core functionality. > I will split the change in two parts one for core driver and other for the sysfs interface if all changes are fine here. > Have you though about support for lines that are pulled low instead of > high? Those aren't as common, but it is conceivable that some > hardware would need it. I think open drain pin should not be pulled low otherwise it will not be possible to make the pin as HIGH with the assumption that the OD pin should never be driven to HIGH But if it is there in any case then it should be handle differently at client level without letting the gpio driver that it is open-drain. -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Feb 13, 2012 at 10:18 PM, Grant Likely <grant.likely@secretlab.ca> wrote: > Linus mentioned that this should be part of pinctrl instead of the gpio API, > but I think there is an argument for making it part of the gpio API, > particularly since open-drain is pretty much a universal concept that all > gpio controllers can support (unlike driver strength) Actually pinctrl is engineered to be used as back-end for gpiolib so thinking about it I'm pretty happy with this arrangement, the gpiolib driver can very well call down to pinctrl to have the actual setting done if needed. So it's no big deal. It is also a case for making some of the pin config business generic, which I have failed at in the past. > Have you though about support for lines that are pulled low instead of > high? Those aren't as common, but it is conceivable that some > hardware would need it. So if the idea is (if I get it correctly) that this thing is an input sometimes and open drain/collector output sometimes, then open source/emitter for the inverse situation is an equally valid case right? In that case I think it'd be best to add both. The COH901 driver for U300 supports open source/emitter BTW. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thursday 16 February 2012 03:55 AM, Linus Walleij wrote: >> Have you though about support for lines that are pulled low instead of >> high? Those aren't as common, but it is conceivable that some >> hardware would need it. > So if the idea is (if I get it correctly) that this thing is an input > sometimes and open drain/collector output sometimes, then > open source/emitter for the inverse situation is an equally valid > case right? In that case I think it'd be best to add both. > > The COH901 driver for U300 supports open source/emitter > BTW. > Yes, I can add the open source also but like to be in incremental change. Not together with open drain. We can go with: - open drain core driver support. - open drain sysfs interface support - open source core driver support - open source sysfs interface. I have already changes for the first 2 which we can review/apply, Meanwhile I will work on open source support. Does it look good? > Yours, > Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Feb 16, 2012 at 9:28 AM, Laxman Dewangan <ldewangan@nvidia.com> wrote: > Yes, I can add the open source also but like to be in incremental change. > Not together with open drain. > We can go with: > - open drain core driver support. > - open drain sysfs interface support > - open source core driver support > - open source sysfs interface. > > I have already changes for the first 2 which we can review/apply, > Meanwhile I will work on open source support. > > Does it look good? Well as a patch concept it is OK but I have real bad feelings about patch [2/4] and [4/4] adding sysfs. What the GPIO subsystem really needs is to get rid of sysfs interfaces, have them deprecated and replaced with a per-gpiochip /dev/gpioN with ioctl()s or similar instead. The sysfs interface is just ... it won't scale. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Friday 17 February 2012 01:30 AM, Linus Walleij wrote: > On Thu, Feb 16, 2012 at 9:28 AM, Laxman Dewangan<ldewangan@nvidia.com> wrote: > >> Yes, I can add the open source also but like to be in incremental change. >> Not together with open drain. >> We can go with: >> - open drain core driver support. >> - open drain sysfs interface support >> - open source core driver support >> - open source sysfs interface. >> >> I have already changes for the first 2 which we can review/apply, >> Meanwhile I will work on open source support. >> >> Does it look good? > Well as a patch concept it is OK but I have real bad feelings about > patch [2/4] and [4/4] adding sysfs. > > What the GPIO subsystem really needs is to get rid of sysfs > interfaces, have them deprecated and replaced with a > per-gpiochip /dev/gpioN with ioctl()s or similar instead. The sysfs > interface is just ... it won't scale. > Wow, then I will not add 2/4 and 4/4. > Yours, > Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/gpio.txt b/Documentation/gpio.txt index 792faa3..b08933c 100644 --- a/Documentation/gpio.txt +++ b/Documentation/gpio.txt @@ -302,6 +302,7 @@ where 'flags' is currently defined to specify the following properties: * GPIOF_INIT_LOW - as output, set initial level to LOW * GPIOF_INIT_HIGH - as output, set initial level to HIGH + * GPIOF_OD - gpio pin is open drain type. since GPIOF_INIT_* are only valid when configured as output, so group valid combinations as: @@ -310,8 +311,7 @@ combinations as: * GPIOF_OUT_INIT_LOW - configured as output, initial level LOW * GPIOF_OUT_INIT_HIGH - configured as output, initial level HIGH -In the future, these flags can be extended to support more properties such -as open-drain status. +In the future, these flags can be extended to support more properties. Further more, to ease the claim/release of multiple GPIOs, 'struct gpio' is introduced to encapsulate all three fields as: @@ -641,6 +641,13 @@ and have the following read/write attributes: for "rising" and "falling" edges will follow this setting. + "open_drain" ... reads as either 0 (false) or 1 (true). Write + any nonzero value to make the pin in open drain. + By setting open drain to true, the output can be set + to HIGH by external PULL UP and setting direction to input. + The output will be set to LOW by setting direction to + output with value is 0. + GPIO controllers have paths like /sys/class/gpio/gpiochip42/ (for the controller implementing GPIOs starting at #42) and have the following read-only attributes: @@ -679,6 +686,9 @@ requested using gpio_request(): /* change the polarity of a GPIO node in sysfs */ int gpio_sysfs_set_active_low(unsigned gpio, int value); + /* change the pin to open drain in sysfs */ + int gpio_sysfs_set_open_drain(unsigned gpio, int value); + After a kernel driver requests a GPIO, it may only be made available in the sysfs interface by gpio_export(). The driver can control whether the signal direction may change. This helps drivers prevent userspace code @@ -698,3 +708,10 @@ differences between boards from user space. This only affects the sysfs interface. Polarity change can be done both before and after gpio_export(), and previously enabled poll(2) support for either rising or falling edge will be reconfigured to follow this setting. + +Drivers can use gpio_sysfs_set_open_drain() to enable/disable the open +drain property of that pins. This only affect the sysfs interface. +The flag will be set as open drain if thsi function is called with value +of 1. It is recommended to set the open drain property before setting +the value in output mode so that pin state cn be set properly based +on the value.
Adding details of open drain configuration of the gpio so that client can set the pin as open drain at the time of gpio request. Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com> --- Documentation/gpio.txt | 21 +++++++++++++++++++-- 1 files changed, 19 insertions(+), 2 deletions(-)