| Message ID | 1491485752-28030-1-git-send-email-ldewangan@nvidia.com |
|---|---|
| State | New |
| Headers | show |
On Thu, Apr 6, 2017 at 4:35 PM, Laxman Dewangan <ldewangan@nvidia.com> wrote: > Currently, the GPIO interface is said to Open Drain if it is Single > Ended and active LOW. Similarly, it is said as Open Source if it is > Single Ended and active HIGH. > > The active HIGH/LOW is used in the interface for setting the pin > state to HIGH or LOW when enabling/disabling the interface. > > In Open Drain interface, pin is set to HIGH by putting pin in > high impedance and LOW by driving to the LOW. > > In Open Source interface, pin is set to HIGH by driving pin to > HIGH and set to LOW by putting pin in high impedance. > > With above, the Open Drain/Source is unrelated to the active LOW/HIGH > in interface. There is interface where the enable/disable of interface > is ether active LOW or HIGH but it is Open Drain type. > > Hence decouple the Open Drain with Single Ended + Active LOW and > Open Source with Single Ended + Active HIGH. > > Adding different flag for the Open Drain/Open Source which is valid > only when Single ended flag is enabled. > if (single_ended) { > - if (active_low) > + if (open_drain) This breaks ACPI case, right?
On Thursday 06 April 2017 09:40 PM, Andy Shevchenko wrote: > On Thu, Apr 6, 2017 at 4:35 PM, Laxman Dewangan <ldewangan@nvidia.com> wrote: >> Currently, the GPIO interface is said to Open Drain if it is Single >> Ended and active LOW. Similarly, it is said as Open Source if it is >> Single Ended and active HIGH. >> >> The active HIGH/LOW is used in the interface for setting the pin >> state to HIGH or LOW when enabling/disabling the interface. >> >> In Open Drain interface, pin is set to HIGH by putting pin in >> high impedance and LOW by driving to the LOW. >> >> In Open Source interface, pin is set to HIGH by driving pin to >> HIGH and set to LOW by putting pin in high impedance. >> >> With above, the Open Drain/Source is unrelated to the active LOW/HIGH >> in interface. There is interface where the enable/disable of interface >> is ether active LOW or HIGH but it is Open Drain type. >> >> Hence decouple the Open Drain with Single Ended + Active LOW and >> Open Source with Single Ended + Active HIGH. >> >> Adding different flag for the Open Drain/Open Source which is valid >> only when Single ended flag is enabled. >> if (single_ended) { >> - if (active_low) >> + if (open_drain) > This breaks ACPI case, right? > In acpi case, single_ended is not handled. It only handles the active LOW. From code: bool active_low = false; bool single_ended = false; int ret; if (!fwnode) return ERR_PTR(-EINVAL); if (is_of_node(fwnode)) { enum of_gpio_flags flags; desc = of_get_named_gpiod_flags(to_of_node(fwnode), propname, index, &flags); if (!IS_ERR(desc)) { active_low = flags & OF_GPIO_ACTIVE_LOW; single_ended = flags & OF_GPIO_SINGLE_ENDED; } } else if (is_acpi_node(fwnode)) { struct acpi_gpio_info info; desc = acpi_node_get_gpiod(fwnode, propname, index, &info); if (!IS_ERR(desc)) active_low = info.polarity == GPIO_ACTIVE_LOW; } -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Apr 6, 2017 at 7:56 PM, Laxman Dewangan <ldewangan@nvidia.com> wrote: > On Thursday 06 April 2017 09:40 PM, Andy Shevchenko wrote: >> On Thu, Apr 6, 2017 at 4:35 PM, Laxman Dewangan <ldewangan@nvidia.com> >> wrote: >>> Adding different flag for the Open Drain/Open Source which is valid >>> only when Single ended flag is enabled. >>> if (single_ended) { >>> - if (active_low) >>> + if (open_drain) >> >> This breaks ACPI case, right? > In acpi case, single_ended is not handled. It only handles the active LOW. > > From code: Fair enough. I would look into ACPI spec for possibility to add this kind of functionality. Thanks.
On Friday 07 April 2017 03:55 PM, Linus Walleij wrote: > On Thu, Apr 6, 2017 at 3:35 PM, Laxman Dewangan <ldewangan@nvidia.com> wrote: > >> Currently, the GPIO interface is said to Open Drain if it is Single >> Ended and active LOW. Similarly, it is said as Open Source if it is >> Single Ended and active HIGH. >> >> The active HIGH/LOW is used in the interface for setting the pin >> state to HIGH or LOW when enabling/disabling the interface. >> >> In Open Drain interface, pin is set to HIGH by putting pin in >> high impedance and LOW by driving to the LOW. >> >> In Open Source interface, pin is set to HIGH by driving pin to >> HIGH and set to LOW by putting pin in high impedance. >> >> With above, the Open Drain/Source is unrelated to the active LOW/HIGH >> in interface. There is interface where the enable/disable of interface >> is ether active LOW or HIGH but it is Open Drain type. >> >> Hence decouple the Open Drain with Single Ended + Active LOW and >> Open Source with Single Ended + Active HIGH. >> >> Adding different flag for the Open Drain/Open Source which is valid >> only when Single ended flag is enabled. >> >> Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com> > Patch applied. > > Good that you found this and fixed it before someone git hurt. Thanks you very much. Now we are going to use the descriptor based GPIO APIs as this is very powerful and passing this info from DT is very useful. -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Apr 6, 2017 at 3:35 PM, Laxman Dewangan <ldewangan@nvidia.com> wrote: > Currently, the GPIO interface is said to Open Drain if it is Single > Ended and active LOW. Similarly, it is said as Open Source if it is > Single Ended and active HIGH. > > The active HIGH/LOW is used in the interface for setting the pin > state to HIGH or LOW when enabling/disabling the interface. > > In Open Drain interface, pin is set to HIGH by putting pin in > high impedance and LOW by driving to the LOW. > > In Open Source interface, pin is set to HIGH by driving pin to > HIGH and set to LOW by putting pin in high impedance. > > With above, the Open Drain/Source is unrelated to the active LOW/HIGH > in interface. There is interface where the enable/disable of interface > is ether active LOW or HIGH but it is Open Drain type. > > Hence decouple the Open Drain with Single Ended + Active LOW and > Open Source with Single Ended + Active HIGH. > > Adding different flag for the Open Drain/Open Source which is valid > only when Single ended flag is enabled. > > Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com> Patch applied. Good that you found this and fixed it before someone git hurt. Sorry for screwing it up, my fault. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Apr 07, 2017 at 12:25:49PM +0200, Linus Walleij wrote: > On Thu, Apr 6, 2017 at 3:35 PM, Laxman Dewangan <ldewangan@nvidia.com> wrote: > > > Currently, the GPIO interface is said to Open Drain if it is Single > > Ended and active LOW. Similarly, it is said as Open Source if it is > > Single Ended and active HIGH. > > > > The active HIGH/LOW is used in the interface for setting the pin > > state to HIGH or LOW when enabling/disabling the interface. > > > > In Open Drain interface, pin is set to HIGH by putting pin in > > high impedance and LOW by driving to the LOW. > > > > In Open Source interface, pin is set to HIGH by driving pin to > > HIGH and set to LOW by putting pin in high impedance. > > > > With above, the Open Drain/Source is unrelated to the active LOW/HIGH > > in interface. There is interface where the enable/disable of interface > > is ether active LOW or HIGH but it is Open Drain type. > > > > Hence decouple the Open Drain with Single Ended + Active LOW and > > Open Source with Single Ended + Active HIGH. > > > > Adding different flag for the Open Drain/Open Source which is valid > > only when Single ended flag is enabled. > > > > Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com> > > Patch applied. > > Good that you found this and fixed it before someone git hurt. Well, while decoupling single-endedness from polarity was the right thing to do, this change did actually break the DT binary interface. If you have an old compiled dtb whose source used GPIO_OPEN_DRAIN, you now instead get *open-source* behaviour on 4.12: GPIO_OPEN_DRAIN = GPIO_SINGLE_ENDED | GPIO_ACTIVE_LOW => active-low, but *open source* while if you recompile that source against 4.12 you do get the expected open-drain behaviour, but now with inverted polarity: GPIO_OPEN_DRAIN = GPIO_SINGLE_ENDED | GPIO_LINE_OPEN_DRAIN => open drain, but *active high* requiring the device tree to be updated by specifying (GPIO_OPEN_DRAIN | GPIO_ACTIVE_LOW) I guess the latter is fine, even if it is likely to amount to a fair bit of debugging world wide. Perhaps all this can still be avoided by adding further flags and deprecating others before people start migrating to 4.12 (after all, GPIO_OPEN_DRAIN has been around since 4.4 even if there are no in-kernel users). Or we accept the binary interface breakage -- it probably is pretty rare that people update the kernel without updating the dtb. I can just update the dts on the system that broke for me, and hopefully anyone debugging this issue while updating to 4.12 will find this mail quickly. Johan -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday 19 July 2017 06:55 PM, Johan Hovold wrote: > On Fri, Apr 07, 2017 at 12:25:49PM +0200, Linus Walleij wrote: >> On Thu, Apr 6, 2017 at 3:35 PM, Laxman Dewangan <ldewangan@nvidia.com> wrote: >> >>> Currently, the GPIO interface is said to Open Drain if it is Single >>> Ended and active LOW. Similarly, it is said as Open Source if it is >>> Single Ended and active HIGH. >>> >>> The active HIGH/LOW is used in the interface for setting the pin >>> state to HIGH or LOW when enabling/disabling the interface. >>> >>> In Open Drain interface, pin is set to HIGH by putting pin in >>> high impedance and LOW by driving to the LOW. >>> >>> In Open Source interface, pin is set to HIGH by driving pin to >>> HIGH and set to LOW by putting pin in high impedance. >>> >>> With above, the Open Drain/Source is unrelated to the active LOW/HIGH >>> in interface. There is interface where the enable/disable of interface >>> is ether active LOW or HIGH but it is Open Drain type. >>> >>> Hence decouple the Open Drain with Single Ended + Active LOW and >>> Open Source with Single Ended + Active HIGH. >>> >>> Adding different flag for the Open Drain/Open Source which is valid >>> only when Single ended flag is enabled. >>> >>> Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com> >> Patch applied. >> >> Good that you found this and fixed it before someone git hurt. > Well, while decoupling single-endedness from polarity was the right > thing to do, this change did actually break the DT binary interface. > > If you have an old compiled dtb whose source used GPIO_OPEN_DRAIN, you > now instead get *open-source* behaviour on 4.12: > > GPIO_OPEN_DRAIN = GPIO_SINGLE_ENDED | GPIO_ACTIVE_LOW > > => active-low, but *open source* > > while if you recompile that source against 4.12 you do get the expected > open-drain behaviour, but now with inverted polarity: > > GPIO_OPEN_DRAIN = GPIO_SINGLE_ENDED | GPIO_LINE_OPEN_DRAIN > > => open drain, but *active high* > > requiring the device tree to be updated by specifying > > (GPIO_OPEN_DRAIN | GPIO_ACTIVE_LOW) > > I guess the latter is fine, even if it is likely to amount to a fair bit > of debugging world wide. > > Perhaps all this can still be avoided by adding further flags and > deprecating others before people start migrating to 4.12 (after all, > GPIO_OPEN_DRAIN has been around since 4.4 even if there are no in-kernel > users). > > Or we accept the binary interface breakage -- it probably is pretty rare > that people update the kernel without updating the dtb. I can just > update the dts on the system that broke for me, and hopefully anyone > debugging this issue while updating to 4.12 will find this mail quickly. > Yes, it breaks the older DTS with new kernel. However, this point was discussed before sending patch. As there was no user in the mainline DTs for these macros, we made change. -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jul 19, 2017 at 08:29:08PM +0530, Laxman Dewangan wrote: > >> Good that you found this and fixed it before someone git hurt. > > Well, while decoupling single-endedness from polarity was the right > > thing to do, this change did actually break the DT binary interface. > > > > If you have an old compiled dtb whose source used GPIO_OPEN_DRAIN, you > > now instead get *open-source* behaviour on 4.12: > > > > GPIO_OPEN_DRAIN = GPIO_SINGLE_ENDED | GPIO_ACTIVE_LOW > > > > => active-low, but *open source* > > > > while if you recompile that source against 4.12 you do get the expected > > open-drain behaviour, but now with inverted polarity: > > > > GPIO_OPEN_DRAIN = GPIO_SINGLE_ENDED | GPIO_LINE_OPEN_DRAIN > > > > => open drain, but *active high* > > > > requiring the device tree to be updated by specifying > > > > (GPIO_OPEN_DRAIN | GPIO_ACTIVE_LOW) > > > > I guess the latter is fine, even if it is likely to amount to a fair bit > > of debugging world wide. > > > > Perhaps all this can still be avoided by adding further flags and > > deprecating others before people start migrating to 4.12 (after all, > > GPIO_OPEN_DRAIN has been around since 4.4 even if there are no in-kernel > > users). > > > > Or we accept the binary interface breakage -- it probably is pretty rare > > that people update the kernel without updating the dtb. I can just > > update the dts on the system that broke for me, and hopefully anyone > > debugging this issue while updating to 4.12 will find this mail quickly. > > > > Yes, it breaks the older DTS with new kernel. However, this point was > discussed before sending patch. As there was no user in the mainline DTs > for these macros, we made change. These are generic gpio flags that can be used with a multitude of devices (e.g. regulators, reset-signals for all sorts of ICs, etc.), and whether there are any in-kernel users (dts) should probably not carry much weight. Some people are still stuck with 4.4 or 4.9 and it may still be a while before they update to, say, the next LTS kernel (4.13) and get bitten by this. Thanks, Johan -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jul 19, 2017 at 4:59 PM, Laxman Dewangan <ldewangan@nvidia.com> wrote: > On Wednesday 19 July 2017 06:55 PM, Johan Hovold wrote: >> I guess the latter is fine, even if it is likely to amount to a fair bit >> of debugging world wide. >> >> Perhaps all this can still be avoided by adding further flags and >> deprecating others before people start migrating to 4.12 (after all, >> GPIO_OPEN_DRAIN has been around since 4.4 even if there are no in-kernel >> users). >> >> Or we accept the binary interface breakage -- it probably is pretty rare >> that people update the kernel without updating the dtb. I can just >> update the dts on the system that broke for me, and hopefully anyone >> debugging this issue while updating to 4.12 will find this mail quickly. > > Yes, it breaks the older DTS with new kernel. However, this point was > discussed before sending patch. As there was no user in the mainline DTs for > these macros, we made change. My operating assumption is usually "rough consensus and running code". If the code doesn't run, i.e. if there are regressions then we need to fix them. I am not aware of any systems having picked up the flags as they were set before this patch, but if they exist we need to patch it of course. On the other hand we don't patch theoretical compatibility issues either, only those that occure in practice. It falls back on when a DT binding is really standardized. When it is starting to get discussed? When the final binding is merged to the kernel tree? When the devicetree.org people publish it? When a big enough vendor ships more than 100 devices using it no matter what happened discussion-wise? All these are a bit fluid concepts, so as a result, the handling of bindings is a bit fluid. If we really want DT bindings to have a point when they are "set in stone" then we need to talk to devicetree.org about that. But in reality I think all standards ever are a bit fuzzy around the edges. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Aug 03, 2017 at 10:14:32AM +0200, Linus Walleij wrote: > On Wed, Jul 19, 2017 at 4:59 PM, Laxman Dewangan <ldewangan@nvidia.com> wrote: > > On Wednesday 19 July 2017 06:55 PM, Johan Hovold wrote: > > >> I guess the latter is fine, even if it is likely to amount to a fair bit > >> of debugging world wide. > >> > >> Perhaps all this can still be avoided by adding further flags and > >> deprecating others before people start migrating to 4.12 (after all, > >> GPIO_OPEN_DRAIN has been around since 4.4 even if there are no in-kernel > >> users). > >> > >> Or we accept the binary interface breakage -- it probably is pretty rare > >> that people update the kernel without updating the dtb. I can just > >> update the dts on the system that broke for me, and hopefully anyone > >> debugging this issue while updating to 4.12 will find this mail quickly. > > > > Yes, it breaks the older DTS with new kernel. However, this point was > > discussed before sending patch. As there was no user in the mainline DTs for > > these macros, we made change. > > My operating assumption is usually "rough consensus and running code". > > If the code doesn't run, i.e. if there are regressions then we need to fix > them. > > I am not aware of any systems having picked up the flags as they were > set before this patch, but if they exist we need to patch it of course. Eight kernel releases, including two long-term stable kernels (4.4 and 4.9), is a long time for people start using these flags and without us finding out about it for a while still. > On the other hand we don't patch theoretical compatibility issues either, > only those that occure in practice. I have a customer who was bitten by this after updating to 4.12, and I'm sure there will be others. So I wouldn't simply dismiss it as theoretical. > It falls back on when a DT binding is really standardized. When it is > starting to get discussed? When the final binding is merged to the kernel > tree? When the devicetree.org people publish it? When a big enough > vendor ships more than 100 devices using it no matter what happened > discussion-wise? > > All these are a bit fluid concepts, so as a result, the handling of bindings > is a bit fluid. > > If we really want DT bindings to have a point when they are "set in stone" > then we need to talk to devicetree.org about that. But in reality I think all > standards ever are a bit fuzzy around the edges. I agree. And the interface needed to be fixed. But for next time, perhaps being more explicit about the implications for current users would help people notice such changes when they do happen. Backporting a fix for a broken interface could also be considered (at least if discovered in the next kernel release cycle or so). And in this particular case, it looks like it might have been possible to pick flags in such a way that the breakage could have be avoided in the first place. Johan -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" 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/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c index 975b9f6..b13b7c7 100644 --- a/drivers/gpio/gpiolib-of.c +++ b/drivers/gpio/gpiolib-of.c @@ -147,7 +147,7 @@ struct gpio_desc *of_find_gpio(struct device *dev, const char *con_id, *flags |= GPIO_ACTIVE_LOW; if (of_flags & OF_GPIO_SINGLE_ENDED) { - if (of_flags & OF_GPIO_ACTIVE_LOW) + if (of_flags & OF_GPIO_OPEN_DRAIN) *flags |= GPIO_OPEN_DRAIN; else *flags |= GPIO_OPEN_SOURCE; diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index 4aa1e78..c22e572 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -3333,6 +3333,7 @@ struct gpio_desc *fwnode_get_named_gpiod(struct fwnode_handle *fwnode, unsigned long lflags = 0; bool active_low = false; bool single_ended = false; + bool open_drain = false; int ret; if (!fwnode) @@ -3346,6 +3347,7 @@ struct gpio_desc *fwnode_get_named_gpiod(struct fwnode_handle *fwnode, if (!IS_ERR(desc)) { active_low = flags & OF_GPIO_ACTIVE_LOW; single_ended = flags & OF_GPIO_SINGLE_ENDED; + open_drain = flags & OF_GPIO_OPEN_DRAIN; } } else if (is_acpi_node(fwnode)) { struct acpi_gpio_info info; @@ -3366,7 +3368,7 @@ struct gpio_desc *fwnode_get_named_gpiod(struct fwnode_handle *fwnode, lflags |= GPIO_ACTIVE_LOW; if (single_ended) { - if (active_low) + if (open_drain) lflags |= GPIO_OPEN_DRAIN; else lflags |= GPIO_OPEN_SOURCE; diff --git a/include/dt-bindings/gpio/gpio.h b/include/dt-bindings/gpio/gpio.h index c673d2c..b4f54da 100644 --- a/include/dt-bindings/gpio/gpio.h +++ b/include/dt-bindings/gpio/gpio.h @@ -17,11 +17,15 @@ #define GPIO_PUSH_PULL 0 #define GPIO_SINGLE_ENDED 2 +/* Bit 2 express Open drain or open source */ +#define GPIO_LINE_OPEN_SOURCE 0 +#define GPIO_LINE_OPEN_DRAIN 4 + /* - * Open Drain/Collector is the combination of single-ended active low, - * Open Source/Emitter is the combination of single-ended active high. + * Open Drain/Collector is the combination of single-ended open drain interface. + * Open Source/Emitter is the combination of single-ended open source interface. */ -#define GPIO_OPEN_DRAIN (GPIO_SINGLE_ENDED | GPIO_ACTIVE_LOW) -#define GPIO_OPEN_SOURCE (GPIO_SINGLE_ENDED | GPIO_ACTIVE_HIGH) +#define GPIO_OPEN_DRAIN (GPIO_SINGLE_ENDED | GPIO_LINE_OPEN_DRAIN) +#define GPIO_OPEN_SOURCE (GPIO_SINGLE_ENDED | GPIO_LINE_OPEN_SOURCE) #endif diff --git a/include/linux/of_gpio.h b/include/linux/of_gpio.h index 3f87ea5..1e089d5 100644 --- a/include/linux/of_gpio.h +++ b/include/linux/of_gpio.h @@ -30,6 +30,7 @@ struct device_node; enum of_gpio_flags { OF_GPIO_ACTIVE_LOW = 0x1, OF_GPIO_SINGLE_ENDED = 0x2, + OF_GPIO_OPEN_DRAIN = 0x4, }; #ifdef CONFIG_OF_GPIO
Currently, the GPIO interface is said to Open Drain if it is Single Ended and active LOW. Similarly, it is said as Open Source if it is Single Ended and active HIGH. The active HIGH/LOW is used in the interface for setting the pin state to HIGH or LOW when enabling/disabling the interface. In Open Drain interface, pin is set to HIGH by putting pin in high impedance and LOW by driving to the LOW. In Open Source interface, pin is set to HIGH by driving pin to HIGH and set to LOW by putting pin in high impedance. With above, the Open Drain/Source is unrelated to the active LOW/HIGH in interface. There is interface where the enable/disable of interface is ether active LOW or HIGH but it is Open Drain type. Hence decouple the Open Drain with Single Ended + Active LOW and Open Source with Single Ended + Active HIGH. Adding different flag for the Open Drain/Open Source which is valid only when Single ended flag is enabled. Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com> --- drivers/gpio/gpiolib-of.c | 2 +- drivers/gpio/gpiolib.c | 4 +++- include/dt-bindings/gpio/gpio.h | 12 ++++++++---- include/linux/of_gpio.h | 1 + 4 files changed, 13 insertions(+), 6 deletions(-)