| Message ID | 20190103164102.31437-4-thomas.petazzoni@bootlin.com |
|---|---|
| State | New |
| Headers | show |
| Series | Proposal to support pull-up/pull-down GPIO configuration | expand |
> diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c > index 7f1260c78270..6518dc8c7c4c 100644 > --- a/drivers/gpio/gpiolib-of.c > +++ b/drivers/gpio/gpiolib-of.c > @@ -323,6 +323,11 @@ struct gpio_desc *of_find_gpio(struct > device *dev, const char *con_id, > if (of_flags & OF_GPIO_TRANSITORY) > *flags |= GPIO_TRANSITORY; > > + if (of_flags & OF_GPIO_PULL_UP) > + *flags |= GPIO_PULL_UP; > + else if (of_flags & OF_GPIO_PULL_DOWN) > + *flags |= GPIO_PULL_DOWN; > + > return desc; > } > Hi Thomas, my recommendation is to add an explicit "error handling" code here to warn when the DT specifies both pull-up and pull-down bits. It's outside of any hot path, and it will help identify mistakes instead of silently prefering a random bit choice. > +/* Bit 4 express pull up */ > +#define GPIO_PULL_UP 16 > + > +/* Bit 5 express pull down */ > +#define GPIO_PULL_DOWN 32 > + > + GPIO_PULL_UP = (1 << 4), > + GPIO_PULL_DOWN = (1 << 5), > + OF_GPIO_PULL_UP = 0x10, > + OF_GPIO_PULL_DOWN = 0x20, I understand that it's already there, but I wonder if this duplication can be removed. Am I missing something, perhaps a reason why include/linux/of_gpio.h and include/dt-bindings/gpio/gpio.h are separate files? With kind regards, Jan
Hello Jan, Thanks for your feedback. On Tue, 08 Jan 2019 15:00:22 +0100, Jan Kundrát wrote: > > diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c > > index 7f1260c78270..6518dc8c7c4c 100644 > > --- a/drivers/gpio/gpiolib-of.c > > +++ b/drivers/gpio/gpiolib-of.c > > @@ -323,6 +323,11 @@ struct gpio_desc *of_find_gpio(struct > > device *dev, const char *con_id, > > if (of_flags & OF_GPIO_TRANSITORY) > > *flags |= GPIO_TRANSITORY; > > > > + if (of_flags & OF_GPIO_PULL_UP) > > + *flags |= GPIO_PULL_UP; > > + else if (of_flags & OF_GPIO_PULL_DOWN) > > + *flags |= GPIO_PULL_DOWN; > > + > > return desc; > > } > > > > Hi Thomas, > my recommendation is to add an explicit "error handling" code here to warn > when the DT specifies both pull-up and pull-down bits. It's outside of any > hot path, and it will help identify mistakes instead of silently prefering > a random bit choice. Sure. > > +/* Bit 4 express pull up */ > > +#define GPIO_PULL_UP 16 > > + > > +/* Bit 5 express pull down */ > > +#define GPIO_PULL_DOWN 32 > > + > > > + GPIO_PULL_UP = (1 << 4), > > + GPIO_PULL_DOWN = (1 << 5), > > > + OF_GPIO_PULL_UP = 0x10, > > + OF_GPIO_PULL_DOWN = 0x20, > > I understand that it's already there, but I wonder if this duplication can > be removed. Am I missing something, perhaps a reason why > include/linux/of_gpio.h and include/dt-bindings/gpio/gpio.h are separate > files? I also wondered why there was such duplication when writing the patches. I've assumed it was done so that include/dt-bindings/gpio/gpio.h is "clean" enough to be included in DT, while include/linux/of_gpio.h some more elaborate definitions. But indeed <linux/of_gpio.h> could include <dt-bindings/gpio/gpio.h>. Perhaps there's a good reason for this duplication? Let's see the feedback of GPIO maintainers about this. Best regards, Thomas
On Thu, Jan 3, 2019 at 5:41 PM Thomas Petazzoni <thomas.petazzoni@bootlin.com> wrote: > This commit adds support for configuring the pull-up and pull-down > resistors available in some GPIO controllers. While configuring > pull-up/pull-down is already possible through the pinctrl subsystem, > some GPIO controllers, especially simple ones such as GPIO expanders > on I2C, don't have any pinmuxing capability and therefore do not use > the pinctrl subsystem. > > This commit implements the GPIO_PULL_UP and GPIO_PULL_DOWN flags, > which can be used from the Device Tree, to enable a pull-up or > pull-down resistor on a given GPIO. > > The flag is simply propagated all the way to the core GPIO subsystem, > where it is used to call the gpio_chip ->set_config callback with the > appropriate existing PIN_CONFIG_BIAS_* values. > > Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com> I'm overall happy with this approach. > +/* Bit 4 express pull up */ > +#define GPIO_PULL_UP 16 > + > +/* Bit 5 express pull down */ > +#define GPIO_PULL_DOWN 32 Please use #define GPIO_PULL_UP BIT(5) #define GPIO_PULL_DOWN BIT(6) > @@ -12,6 +12,8 @@ enum gpio_lookup_flags { > GPIO_OPEN_SOURCE = (1 << 2), > GPIO_PERSISTENT = (0 << 3), > GPIO_TRANSITORY = (1 << 3), > + GPIO_PULL_UP = (1 << 4), > + GPIO_PULL_DOWN = (1 << 5), You can keep this to follow the pattern set by the others. > @@ -28,6 +28,8 @@ enum of_gpio_flags { > OF_GPIO_SINGLE_ENDED = 0x2, > OF_GPIO_OPEN_DRAIN = 0x4, > OF_GPIO_TRANSITORY = 0x8, > + OF_GPIO_PULL_UP = 0x10, > + OF_GPIO_PULL_DOWN = 0x20, Same here: this is fine. Yours, Linus Walleij
On Tue, Jan 8, 2019 at 3:04 PM Thomas Petazzoni <thomas.petazzoni@bootlin.com> wrote: > > > +/* Bit 4 express pull up */ > > > +#define GPIO_PULL_UP 16 > > > + > > > +/* Bit 5 express pull down */ > > > +#define GPIO_PULL_DOWN 32 > > > + > > > > > + GPIO_PULL_UP = (1 << 4), > > > + GPIO_PULL_DOWN = (1 << 5), > > > > > + OF_GPIO_PULL_UP = 0x10, > > > + OF_GPIO_PULL_DOWN = 0x20, > > > > I understand that it's already there, but I wonder if this duplication can > > be removed. Am I missing something, perhaps a reason why > > include/linux/of_gpio.h and include/dt-bindings/gpio/gpio.h are separate > > files? > > I also wondered why there was such duplication when writing the > patches. I've assumed it was done so that > include/dt-bindings/gpio/gpio.h is "clean" enough to be included in DT, > while include/linux/of_gpio.h some more elaborate definitions. But > indeed <linux/of_gpio.h> could include <dt-bindings/gpio/gpio.h>. > Perhaps there's a good reason for this duplication? Let's see the > feedback of GPIO maintainers about this. There is a good reason for this. The reason is that DT ABI is written in clay, userspace ABI is written in stone and kernel internal API is written in water. For the last one see Documentation/process/stable-api-nonsense.rst For the DT the clay tablet hardens when devices are mass produced and shipped with a DT in flash. For the userspace ABI, that will never change until the chardevice disappears. So these ABIs are essentially versioned according to completely different rules and for this reason they are kept separate. They overlap considerably, but for this reason we are keeping the definitions separately and also explicitly translating between them so that we can still change the internal kernel representation of these flags if we need. Yours, Linus Walleij
Hello Linus, On Fri, 11 Jan 2019 11:05:03 +0100, Linus Walleij wrote: > > The flag is simply propagated all the way to the core GPIO subsystem, > > where it is used to call the gpio_chip ->set_config callback with the > > appropriate existing PIN_CONFIG_BIAS_* values. > > > > Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com> > > I'm overall happy with this approach. Thanks for this review and feedback, very useful. > > > +/* Bit 4 express pull up */ > > +#define GPIO_PULL_UP 16 > > + > > +/* Bit 5 express pull down */ > > +#define GPIO_PULL_DOWN 32 > > Please use > #define GPIO_PULL_UP BIT(5) > #define GPIO_PULL_DOWN BIT(6) Here, I did exactly like below: keep the existing approach used in the file. Today it has: /* Bit 0 express polarity */ #define GPIO_ACTIVE_HIGH 0 #define GPIO_ACTIVE_LOW 1 /* Bit 1 express single-endedness */ #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 [...] /* Bit 3 express GPIO suspend/resume and reset persistence */ #define GPIO_PERSISTENT 0 #define GPIO_TRANSITORY 8 So I kept the same logic and used 16 and 32 to define the GPIO_PULL_UP/GPIO_PULL_DOWN values instead of BIT(x). BIT(x) would have been more logical. However, we are in the dt-bindings includes here, and apparently, there is no definition for the BIT() macro in those headers. Best regards, Thomas
On Fri, Jan 11, 2019 at 1:58 PM Thomas Petazzoni <thomas.petazzoni@bootlin.com> wrote: > On Fri, 11 Jan 2019 11:05:03 +0100, Linus Walleij wrote: > > > +/* Bit 4 express pull up */ > > > +#define GPIO_PULL_UP 16 > > > + > > > +/* Bit 5 express pull down */ > > > +#define GPIO_PULL_DOWN 32 > > > > Please use > > #define GPIO_PULL_UP BIT(5) > > #define GPIO_PULL_DOWN BIT(6) (...) > However, we are in the dt-bindings includes here, and apparently, there > is no definition for the BIT() macro in those headers. Ah you nailed it :D OK keep it like this. Yours, Linus Walleij
On Thu, Jan 03, 2019 at 05:41:01PM +0100, Thomas Petazzoni wrote: > This commit adds support for configuring the pull-up and pull-down > resistors available in some GPIO controllers. While configuring > pull-up/pull-down is already possible through the pinctrl subsystem, > some GPIO controllers, especially simple ones such as GPIO expanders > on I2C, don't have any pinmuxing capability and therefore do not use > the pinctrl subsystem. > > This commit implements the GPIO_PULL_UP and GPIO_PULL_DOWN flags, > which can be used from the Device Tree, to enable a pull-up or > pull-down resistor on a given GPIO. > > The flag is simply propagated all the way to the core GPIO subsystem, > where it is used to call the gpio_chip ->set_config callback with the > appropriate existing PIN_CONFIG_BIAS_* values. > > Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com> > --- > drivers/gpio/gpiolib-of.c | 5 +++++ > drivers/gpio/gpiolib.c | 12 ++++++++++++ > drivers/gpio/gpiolib.h | 2 ++ > include/dt-bindings/gpio/gpio.h | 6 ++++++ It's fine, but really this one should be part of the binding patch. > include/linux/gpio/machine.h | 2 ++ > include/linux/of_gpio.h | 2 ++ > 6 files changed, 29 insertions(+) Acked-by: Rob Herring <robh@kernel.org>
diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c index 7f1260c78270..6518dc8c7c4c 100644 --- a/drivers/gpio/gpiolib-of.c +++ b/drivers/gpio/gpiolib-of.c @@ -323,6 +323,11 @@ struct gpio_desc *of_find_gpio(struct device *dev, const char *con_id, if (of_flags & OF_GPIO_TRANSITORY) *flags |= GPIO_TRANSITORY; + if (of_flags & OF_GPIO_PULL_UP) + *flags |= GPIO_PULL_UP; + else if (of_flags & OF_GPIO_PULL_DOWN) + *flags |= GPIO_PULL_DOWN; + return desc; } diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index b07e4f6ee641..20887c62fbb3 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -2585,6 +2585,13 @@ int gpiod_direction_input(struct gpio_desc *desc) if (status == 0) clear_bit(FLAG_IS_OUT, &desc->flags); + if (test_bit(FLAG_PULL_UP, &desc->flags)) + gpio_set_config(chip, gpio_chip_hwgpio(desc), + PIN_CONFIG_BIAS_PULL_UP); + else if (test_bit(FLAG_PULL_DOWN, &desc->flags)) + gpio_set_config(chip, gpio_chip_hwgpio(desc), + PIN_CONFIG_BIAS_PULL_DOWN); + trace_gpio_direction(desc_to_gpio(desc), 1, status); return status; @@ -4054,6 +4061,11 @@ int gpiod_configure_flags(struct gpio_desc *desc, const char *con_id, if (lflags & GPIO_OPEN_SOURCE) set_bit(FLAG_OPEN_SOURCE, &desc->flags); + if (lflags & GPIO_PULL_UP) + set_bit(FLAG_PULL_UP, &desc->flags); + else if (lflags & GPIO_PULL_DOWN) + set_bit(FLAG_PULL_DOWN, &desc->flags); + status = gpiod_set_transitory(desc, (lflags & GPIO_TRANSITORY)); if (status < 0) return status; diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h index 087d865286a0..0b0cf213c45a 100644 --- a/drivers/gpio/gpiolib.h +++ b/drivers/gpio/gpiolib.h @@ -225,6 +225,8 @@ struct gpio_desc { #define FLAG_IRQ_IS_ENABLED 10 /* GPIO is connected to an enabled IRQ */ #define FLAG_IS_HOGGED 11 /* GPIO is hogged */ #define FLAG_TRANSITORY 12 /* GPIO may lose value in sleep or reset */ +#define FLAG_PULL_UP 13 /* GPIO has pull up enabled */ +#define FLAG_PULL_DOWN 14 /* GPIO has pull down enabled */ /* Connection label */ const char *label; diff --git a/include/dt-bindings/gpio/gpio.h b/include/dt-bindings/gpio/gpio.h index 2cc10ae4bbb7..c029467e828b 100644 --- a/include/dt-bindings/gpio/gpio.h +++ b/include/dt-bindings/gpio/gpio.h @@ -33,4 +33,10 @@ #define GPIO_PERSISTENT 0 #define GPIO_TRANSITORY 8 +/* Bit 4 express pull up */ +#define GPIO_PULL_UP 16 + +/* Bit 5 express pull down */ +#define GPIO_PULL_DOWN 32 + #endif diff --git a/include/linux/gpio/machine.h b/include/linux/gpio/machine.h index daa44eac9241..69673be10213 100644 --- a/include/linux/gpio/machine.h +++ b/include/linux/gpio/machine.h @@ -12,6 +12,8 @@ enum gpio_lookup_flags { GPIO_OPEN_SOURCE = (1 << 2), GPIO_PERSISTENT = (0 << 3), GPIO_TRANSITORY = (1 << 3), + GPIO_PULL_UP = (1 << 4), + GPIO_PULL_DOWN = (1 << 5), }; /** diff --git a/include/linux/of_gpio.h b/include/linux/of_gpio.h index 163b79ecd01a..f9737dea9d1f 100644 --- a/include/linux/of_gpio.h +++ b/include/linux/of_gpio.h @@ -28,6 +28,8 @@ enum of_gpio_flags { OF_GPIO_SINGLE_ENDED = 0x2, OF_GPIO_OPEN_DRAIN = 0x4, OF_GPIO_TRANSITORY = 0x8, + OF_GPIO_PULL_UP = 0x10, + OF_GPIO_PULL_DOWN = 0x20, }; #ifdef CONFIG_OF_GPIO
This commit adds support for configuring the pull-up and pull-down resistors available in some GPIO controllers. While configuring pull-up/pull-down is already possible through the pinctrl subsystem, some GPIO controllers, especially simple ones such as GPIO expanders on I2C, don't have any pinmuxing capability and therefore do not use the pinctrl subsystem. This commit implements the GPIO_PULL_UP and GPIO_PULL_DOWN flags, which can be used from the Device Tree, to enable a pull-up or pull-down resistor on a given GPIO. The flag is simply propagated all the way to the core GPIO subsystem, where it is used to call the gpio_chip ->set_config callback with the appropriate existing PIN_CONFIG_BIAS_* values. Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com> --- drivers/gpio/gpiolib-of.c | 5 +++++ drivers/gpio/gpiolib.c | 12 ++++++++++++ drivers/gpio/gpiolib.h | 2 ++ include/dt-bindings/gpio/gpio.h | 6 ++++++ include/linux/gpio/machine.h | 2 ++ include/linux/of_gpio.h | 2 ++ 6 files changed, 29 insertions(+)