Message ID | 20190921102522.8970-1-drew@pdp7.com |
---|---|
State | New |
Headers | show |
Series | [RFC] gpio: expose pull-up/pull-down line flags to userspace | expand |
sob., 21 wrz 2019 o 12:27 Drew Fustini <drew@pdp7.com> napisał(a): > > Add pull-up/pull-down flags to the gpio line get and > set ioctl() calls. Use cases include a push button > that does not have an external resistor. > > Addition use cases described by Limor Fried (ladyada) of > Adafruit in this PR for Adafruit_Blinka Python lib: > https://github.com/adafruit/Adafruit_Blinka/pull/59 > > Signed-off-by: Drew Fustini <drew@pdp7.com> > --- > drivers/gpio/gpiolib.c | 12 ++++++++++++ > include/uapi/linux/gpio.h | 4 ++++ > 2 files changed, 16 insertions(+) > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c > index d9074191edef..9da1093cc7f5 100644 > --- a/drivers/gpio/gpiolib.c > +++ b/drivers/gpio/gpiolib.c > @@ -427,6 +427,8 @@ struct linehandle_state { > (GPIOHANDLE_REQUEST_INPUT | \ > GPIOHANDLE_REQUEST_OUTPUT | \ > GPIOHANDLE_REQUEST_ACTIVE_LOW | \ > + GPIOHANDLE_REQUEST_PULL_UP | \ > + GPIOHANDLE_REQUEST_PULL_DOWN | \ > GPIOHANDLE_REQUEST_OPEN_DRAIN | \ > GPIOHANDLE_REQUEST_OPEN_SOURCE) > > @@ -598,6 +600,10 @@ static int linehandle_create(struct gpio_device *gdev, void __user *ip) > set_bit(FLAG_OPEN_DRAIN, &desc->flags); > if (lflags & GPIOHANDLE_REQUEST_OPEN_SOURCE) > set_bit(FLAG_OPEN_SOURCE, &desc->flags); > + if (lflags & GPIOHANDLE_REQUEST_PULL_DOWN) > + set_bit(FLAG_PULL_DOWN, &desc->flags); > + if (lflags & GPIOHANDLE_REQUEST_PULL_UP) > + set_bit(FLAG_PULL_UP, &desc->flags); > > ret = gpiod_set_transitory(desc, false); > if (ret < 0) > @@ -1102,6 +1108,10 @@ static long gpio_ioctl(struct file *filp, unsigned int cmd, unsigned long arg) > if (test_bit(FLAG_OPEN_SOURCE, &desc->flags)) > lineinfo.flags |= (GPIOLINE_FLAG_OPEN_SOURCE | > GPIOLINE_FLAG_IS_OUT); > + if (test_bit(FLAG_PULL_DOWN, &desc->flags)) > + lineinfo.flags |= GPIOLINE_FLAG_PULL_DOWN; > + if (test_bit(FLAG_PULL_UP, &desc->flags)) > + lineinfo.flags |= GPIOLINE_FLAG_PULL_UP; > > if (copy_to_user(ip, &lineinfo, sizeof(lineinfo))) > return -EFAULT; > @@ -2475,6 +2485,8 @@ static bool gpiod_free_commit(struct gpio_desc *desc) > clear_bit(FLAG_REQUESTED, &desc->flags); > clear_bit(FLAG_OPEN_DRAIN, &desc->flags); > clear_bit(FLAG_OPEN_SOURCE, &desc->flags); > + clear_bit(FLAG_PULL_UP, &desc->flags); > + clear_bit(FLAG_PULL_DOWN, &desc->flags); > clear_bit(FLAG_IS_HOGGED, &desc->flags); > ret = true; > } > diff --git a/include/uapi/linux/gpio.h b/include/uapi/linux/gpio.h > index 4ebfe0ac6c5b..c2d1f7d908d6 100644 > --- a/include/uapi/linux/gpio.h > +++ b/include/uapi/linux/gpio.h > @@ -33,6 +33,8 @@ struct gpiochip_info { > #define GPIOLINE_FLAG_ACTIVE_LOW (1UL << 2) > #define GPIOLINE_FLAG_OPEN_DRAIN (1UL << 3) > #define GPIOLINE_FLAG_OPEN_SOURCE (1UL << 4) > +#define GPIOLINE_FLAG_PULL_UP (1UL << 5) > +#define GPIOLINE_FLAG_PULL_DOWN (1UL << 6) > > /** > * struct gpioline_info - Information about a certain GPIO line > @@ -62,6 +64,8 @@ struct gpioline_info { > #define GPIOHANDLE_REQUEST_ACTIVE_LOW (1UL << 2) > #define GPIOHANDLE_REQUEST_OPEN_DRAIN (1UL << 3) > #define GPIOHANDLE_REQUEST_OPEN_SOURCE (1UL << 4) > +#define GPIOHANDLE_REQUEST_PULL_UP (1UL << 5) > +#define GPIOHANDLE_REQUEST_PULL_DOWN (1UL << 6) > > /** > * struct gpiohandle_request - Information about a GPIO handle request > -- > 2.20.1 > Hi Drew, I remember discussing it with Linus some time ago. This may not be as straightforward as simply adding new flags. Since PULL-UP/DOWN resistors can - among other parameters - also have configurable resistance, we'll probably need some kind of a structure for this ioctl() to pass any additional information to the kernel. Since we can't change ABI this may require adding a whole new ioctl() for extended configuration. This in turn has to be as future-proof as possible - if someone asks for user-space-configurable drive-strength, the new ioctl() should be ready for it. I should have some bandwidth in the coming days, so I'll try to give it a try. Bart
On Mon, Sep 23, 2019 at 10:38 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > I remember discussing it with Linus some time ago. This may not be as > straightforward as simply adding new flags. Since PULL-UP/DOWN > resistors can - among other parameters - also have configurable > resistance, we'll probably need some kind of a structure for this > ioctl() to pass any additional information to the kernel. Since we > can't change ABI this may require adding a whole new ioctl() for > extended configuration. This in turn has to be as future-proof as > possible - if someone asks for user-space-configurable drive-strength, > the new ioctl() should be ready for it. Thanks for the insight, Bartosz. Would you be able to point me to the discussion if is available in an archive? I would like to improve my understanding. What do you think of adding pull-up/down flags first, and then implementing the extended configuration ioctl() later if necessary? Thank you, Drew
On Mon, Sep 23, 2019 at 10:38 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > I remember discussing it with Linus some time ago. This may not be as > straightforward as simply adding new flags. Since PULL-UP/DOWN > resistors can - among other parameters - also have configurable > resistance, we'll probably need some kind of a structure for this > ioctl() to pass any additional information to the kernel. Since we > can't change ABI this may require adding a whole new ioctl() for > extended configuration. This in turn has to be as future-proof as > possible - if someone asks for user-space-configurable drive-strength, > the new ioctl() should be ready for it. > > I should have some bandwidth in the coming days, so I'll try to give it a try. What we did for the in-kernel API and the Device Tree ABI was to simply say that if you need such elaborate control over the line, it needs to be done with a proper pin control driver. So for lines that just have the GPIO_PULL_UP or GPIO_PULL_DOWN set as a (one-bit) flag, what you will get is "typical" pull down/up (whatever the hardware default is, or what the driver thinks is default, which should be safe so the highest possible pull resistance I suppose). So one option is to just go with these flags and explicitly say that it will give a "system default (high resistance) pull up/down". That said, the pin controller back-end is fully capable of accepting more elaborate configuration, so if we prefer then we can make the more complex userspace ABI that can set it to a desired-or-default resistance. I lean toward simplicity here. I haven't seen that these userspace consumers need very elaborate control of this resistance, they are for one-off hacks and as such should be fine with just default pull up/down I think? I think that specifying "this line will use pull up/down" at request time and having the driver set a safe default pull-up/down as response, (and pretty much what this patch does) and then add another explicit ioctl to refine it the day we need it is a viable way forward. in the future something like: #define GPIOHANDLE_SET_LINE_CONFIG_IOCTL _IOWR(0xB4, 0x0a, struct gpiohandle_config) And then, when we need it, try to come up with some really flexible ABI for the config, based on include/linux/pinctrl/pinconf-generic.h But no upfront code for that right now as it is not needed. A practical usecase must come first. Yours, Linus Walleij
czw., 3 paź 2019 o 14:47 Linus Walleij <linus.walleij@linaro.org> napisał(a): > > On Mon, Sep 23, 2019 at 10:38 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > > I remember discussing it with Linus some time ago. This may not be as > > straightforward as simply adding new flags. Since PULL-UP/DOWN > > resistors can - among other parameters - also have configurable > > resistance, we'll probably need some kind of a structure for this > > ioctl() to pass any additional information to the kernel. Since we > > can't change ABI this may require adding a whole new ioctl() for > > extended configuration. This in turn has to be as future-proof as > > possible - if someone asks for user-space-configurable drive-strength, > > the new ioctl() should be ready for it. > > > > I should have some bandwidth in the coming days, so I'll try to give it a try. > > What we did for the in-kernel API and the Device Tree ABI > was to simply say that if you need such elaborate control over > the line, it needs to be done with a proper pin control driver. > > So for lines that just have the GPIO_PULL_UP or > GPIO_PULL_DOWN set as a (one-bit) flag, what you will > get is "typical" pull down/up (whatever the hardware default > is, or what the driver thinks is default, which should be safe > so the highest possible pull resistance I suppose). > > So one option is to just go with these flags and explicitly > say that it will give a "system default (high resistance) > pull up/down". > > That said, the pin controller back-end is fully capable of > accepting more elaborate configuration, so if we prefer then > we can make the more complex userspace ABI that can > set it to a desired-or-default resistance. > > I lean toward simplicity here. I haven't seen that these > userspace consumers need very elaborate control of this > resistance, they are for one-off hacks and as such should > be fine with just default pull up/down I think? > > I think that specifying "this line will use pull up/down" > at request time and having the driver set a safe default > pull-up/down as response, (and pretty much what this > patch does) and then add another explicit > ioctl to refine it the day we need it is a viable way forward. > > in the future something like: > #define GPIOHANDLE_SET_LINE_CONFIG_IOCTL _IOWR(0xB4, 0x0a, struct > gpiohandle_config) > > And then, when we need it, try to come up with some > really flexible ABI for the config, based on > include/linux/pinctrl/pinconf-generic.h > Thanks for your input Linus. I'm good with that. The config ioctl (or something similar) you're mentioning may appear sooner actually - users of libgpiod have been requesting a way of changing the direction of a line without releasing it - something that's possible in the kernel, but not from user-space at the moment. I'll submit something that allows to change the configuration of a requested line soon. Bart > But no upfront code for that right now as it is not needed. > A practical usecase must come first. > > Yours, > Linus Walleij
pt., 4 paź 2019 o 09:22 Bartosz Golaszewski <brgl@bgdev.pl> napisał(a): > > czw., 3 paź 2019 o 14:47 Linus Walleij <linus.walleij@linaro.org> napisał(a): > > > > On Mon, Sep 23, 2019 at 10:38 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > > > > I remember discussing it with Linus some time ago. This may not be as > > > straightforward as simply adding new flags. Since PULL-UP/DOWN > > > resistors can - among other parameters - also have configurable > > > resistance, we'll probably need some kind of a structure for this > > > ioctl() to pass any additional information to the kernel. Since we > > > can't change ABI this may require adding a whole new ioctl() for > > > extended configuration. This in turn has to be as future-proof as > > > possible - if someone asks for user-space-configurable drive-strength, > > > the new ioctl() should be ready for it. > > > > > > I should have some bandwidth in the coming days, so I'll try to give it a try. > > > > What we did for the in-kernel API and the Device Tree ABI > > was to simply say that if you need such elaborate control over > > the line, it needs to be done with a proper pin control driver. > > > > So for lines that just have the GPIO_PULL_UP or > > GPIO_PULL_DOWN set as a (one-bit) flag, what you will > > get is "typical" pull down/up (whatever the hardware default > > is, or what the driver thinks is default, which should be safe > > so the highest possible pull resistance I suppose). > > > > So one option is to just go with these flags and explicitly > > say that it will give a "system default (high resistance) > > pull up/down". > > > > That said, the pin controller back-end is fully capable of > > accepting more elaborate configuration, so if we prefer then > > we can make the more complex userspace ABI that can > > set it to a desired-or-default resistance. > > > > I lean toward simplicity here. I haven't seen that these > > userspace consumers need very elaborate control of this > > resistance, they are for one-off hacks and as such should > > be fine with just default pull up/down I think? > > > > I think that specifying "this line will use pull up/down" > > at request time and having the driver set a safe default > > pull-up/down as response, (and pretty much what this > > patch does) and then add another explicit > > ioctl to refine it the day we need it is a viable way forward. > > > > in the future something like: > > #define GPIOHANDLE_SET_LINE_CONFIG_IOCTL _IOWR(0xB4, 0x0a, struct > > gpiohandle_config) > > > > And then, when we need it, try to come up with some > > really flexible ABI for the config, based on > > include/linux/pinctrl/pinconf-generic.h > > > > Thanks for your input Linus. I'm good with that. The config ioctl (or > something similar) you're mentioning may appear sooner actually - > users of libgpiod have been requesting a way of changing the direction > of a line without releasing it - something that's possible in the > kernel, but not from user-space at the moment. I'll submit something > that allows to change the configuration of a requested line soon. > > Bart Cc'ing Kent, as he might be interested in this. I'll be travelling tomorrow and will have a couple hours on the plane, so I'll try to cook up an additional ioctl for configuration changes of requested lines with additional padding for future extension. Bart > > > But no upfront code for that right now as it is not needed. > > A practical usecase must come first. > > > > Yours, > > Linus Walleij
On Fri, Oct 4, 2019 at 9:22 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > The config ioctl (or > something similar) you're mentioning may appear sooner actually - > users of libgpiod have been requesting a way of changing the direction > of a line without releasing it - something that's possible in the > kernel, but not from user-space at the moment. I'll submit something > that allows to change the configuration of a requested line soon. Hm! I guess I assumed that userspace users would be using the lines for either input or output, not complex use cases like that, reversing direction and what not. What kind of usecase is this? I certainly hope nothing like doing userspace drivers for complex hardware ... those should be in the kernel... the current ABI is a bit oriented around industrial automation and prototyping use cases. Yours, Linus Walleij
On Sat, Oct 05, 2019 at 07:02:58PM +0200, Linus Walleij wrote: > On Fri, Oct 4, 2019 at 9:22 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > > The config ioctl (or > > something similar) you're mentioning may appear sooner actually - > > users of libgpiod have been requesting a way of changing the direction > > of a line without releasing it - something that's possible in the > > kernel, but not from user-space at the moment. I'll submit something > > that allows to change the configuration of a requested line soon. > > Hm! I guess I assumed that userspace users would be using the lines > for either input or output, not complex use cases like that, reversing > direction and what not. > > What kind of usecase is this? I certainly hope nothing like doing > userspace drivers for complex hardware ... those should be in > the kernel... the current ABI is a bit oriented around industrial > automation and prototyping use cases. > I'm not the only one asking for this, and I can't speak to others' use cases, but in my case I'm prototyping and bit bashing a SPI driver for an ADC where the MOSI/MISO lines can be tied to save a pin. I need to be able to switch line direction without glitching the line and that can't be guaranteed with the current UAPI. Would you consider that "complex hardware"? Kent.
On Sun, Oct 6, 2019 at 5:12 AM Kent Gibson <warthog618@gmail.com> wrote: > On Sat, Oct 05, 2019 at 07:02:58PM +0200, Linus Walleij wrote: > > On Fri, Oct 4, 2019 at 9:22 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > > > > The config ioctl (or > > > something similar) you're mentioning may appear sooner actually - > > > users of libgpiod have been requesting a way of changing the direction > > > of a line without releasing it - something that's possible in the > > > kernel, but not from user-space at the moment. I'll submit something > > > that allows to change the configuration of a requested line soon. > > > > Hm! I guess I assumed that userspace users would be using the lines > > for either input or output, not complex use cases like that, reversing > > direction and what not. > > > > What kind of usecase is this? I certainly hope nothing like doing > > userspace drivers for complex hardware ... those should be in > > the kernel... the current ABI is a bit oriented around industrial > > automation and prototyping use cases. > > I'm not the only one asking for this, and I can't speak to others' use > cases, but in my case I'm prototyping and bit bashing a SPI driver for > an ADC where the MOSI/MISO lines can be tied to save a pin. I need to > be able to switch line direction without glitching the line and that > can't be guaranteed with the current UAPI. > > Would you consider that "complex hardware"? I see, so in the Linux kernel this is called 3-wire SPI, and we would drive the SPI bit-banged bus using drivers/spi/spi-gpio.c and model ADC "foo" it as an ADC in drivers/iio/adc/foo.c. The device tree would look something like so: spi { compatible = "spi-gpio"; #address-cells = <1>; #size-cells = <0>; gpio-sck = <&gpio0 5 GPIO_ACTIVE_HIGH>; gpio-mosi = <&gpio0 8 GPIO_ACTIVE_HIGH>; cs-gpios = <&gpio0 20 GPIO_ACTIVE_LOW>; num-chipselects = <1>; adc { compatible = "foo"; reg = <0>; spi-3wire; }; }; (There are ways to define devices without explicit chipselect as well, yielding a 2-wire bus in the end.) The goal of the bit-banged SPI GPIO driver is to have one driver that can be reused for any GPIO bit-banged SPI. The goal of IIO ADC is to present a uniform and scaled ADC interface to userspace, such that userspace need not worry which ADC it is using because they all present the same user API. IIO further provides precise triggers using timers or interrupt generators to provide a continous stream of AD-converted values using a ring buffer. So in summary the kernels SPI, GPIO and IIO subsystems want to abstract hardware and make userspace simpler, and the kernel sees userspace as having no business driving hardware. This might be seen as a bit imperialistic, but alas, the explicit goal of kernel development is hardware abstraction, and what happens sometimes in the maker community in particular is to reimplement what the kernel does in userspace. What we are especially worried about is that for systems that are not one-offs and lab prototypes, something that is getting productified, the userspace tinkering and hackering is just preserved in resin and no transition to proper Linux drivers happens. So we want userspace GPIO access to be an enabler and not a competitor to the Linux kernel. There are other mechanisms in the kernel with this problem, such as UIO. There is a problem with the biases here, such that some of the kernel people always think they are dealing with companies and that kind of larger organizations, and I am one of those that don't think like that. When it does come to companies, I think there is this quote by Fred Brooks that you should "always plan to throw one away" so they might write a userspace prototype, then throw that away and write a kernel driver as one gained knowledge of how the hardware works. It's one of the ways to spin it. And we want to encourage that way of doing some userspace prototyping. And if the usecase gets complex enough it will lean toward: what about just writing a kernel driver in the first place and refine that. If we are thinking a hobbyist, they are thinking to drive this thing on their table for the next two hours or as an experiment or for fun, or semi business (a few devices, not really mass production), like to set up a factory line or a lightshow, that is something that should stay in userspace and has no business in the kernel. And we just need to know what usecase it is here. But I need to understand. Yours, Linus Walleij
On Sun, Oct 06, 2019 at 11:06:21PM +0200, Linus Walleij wrote: > On Sun, Oct 6, 2019 at 5:12 AM Kent Gibson <warthog618@gmail.com> wrote: > > On Sat, Oct 05, 2019 at 07:02:58PM +0200, Linus Walleij wrote: > > > On Fri, Oct 4, 2019 at 9:22 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > > > > > > The config ioctl (or > > > > something similar) you're mentioning may appear sooner actually - > > > > users of libgpiod have been requesting a way of changing the direction > > > > of a line without releasing it - something that's possible in the > > > > kernel, but not from user-space at the moment. I'll submit something > > > > that allows to change the configuration of a requested line soon. > > > > > > Hm! I guess I assumed that userspace users would be using the lines > > > for either input or output, not complex use cases like that, reversing > > > direction and what not. > > > > > > What kind of usecase is this? I certainly hope nothing like doing > > > userspace drivers for complex hardware ... those should be in > > > the kernel... the current ABI is a bit oriented around industrial > > > automation and prototyping use cases. > > > > I'm not the only one asking for this, and I can't speak to others' use > > cases, but in my case I'm prototyping and bit bashing a SPI driver for > > an ADC where the MOSI/MISO lines can be tied to save a pin. I need to > > be able to switch line direction without glitching the line and that > > can't be guaranteed with the current UAPI. > > > > Would you consider that "complex hardware"? > > I see, so in the Linux kernel this is called 3-wire SPI, and we would > drive the SPI bit-banged bus using drivers/spi/spi-gpio.c and > model ADC "foo" it as an ADC in drivers/iio/adc/foo.c. > > The device tree would look something like so: > > spi { > compatible = "spi-gpio"; > #address-cells = <1>; > #size-cells = <0>; > > gpio-sck = <&gpio0 5 GPIO_ACTIVE_HIGH>; > gpio-mosi = <&gpio0 8 GPIO_ACTIVE_HIGH>; > cs-gpios = <&gpio0 20 GPIO_ACTIVE_LOW>; > num-chipselects = <1>; > > adc { > compatible = "foo"; > reg = <0>; > spi-3wire; > }; > }; > > (There are ways to define devices without explicit chipselect > as well, yielding a 2-wire bus in the end.) > > The goal of the bit-banged SPI GPIO driver is to have one > driver that can be reused for any GPIO bit-banged SPI. > > The goal of IIO ADC is to present a uniform and scaled > ADC interface to userspace, such that userspace need not > worry which ADC it is using because they all present the > same user API. IIO further provides precise triggers > using timers or interrupt generators to provide a > continous stream of AD-converted values using a > ring buffer. > > So in summary the kernels SPI, GPIO and IIO subsystems > want to abstract hardware and make userspace simpler, > and the kernel sees userspace as having no business > driving hardware. > > This might be seen as a bit imperialistic, but alas, the > explicit goal of kernel development is hardware abstraction, > and what happens sometimes in the maker community > in particular is to reimplement what the kernel does in > userspace. > > What we are especially worried about is that for systems > that are not one-offs and lab prototypes, something that is > getting productified, the userspace tinkering and hackering > is just preserved in resin and no transition to proper Linux > drivers happens. > > So we want userspace GPIO access to be an enabler and > not a competitor to the Linux kernel. > > There are other mechanisms in the kernel with this problem, > such as UIO. > > There is a problem with the biases here, such that some of the > kernel people always think they are dealing with companies > and that kind of larger organizations, and I am one of those > that don't think like that. > > When it does come to companies, I think > there is this quote by Fred Brooks that you should > "always plan to throw one away" so they might write a > userspace prototype, then > throw that away and write a kernel driver as one gained > knowledge of how the hardware works. It's one of the ways > to spin it. And we want to encourage that way of doing > some userspace prototyping. And if the usecase gets > complex enough it will lean toward: what about just writing > a kernel driver in the first place and refine that. > > If we are thinking a hobbyist, they are thinking to drive this > thing on their table for the next two hours or as an experiment > or for fun, or semi business (a few devices, not really mass > production), like to set up a factory line or a lightshow, that > is something that should stay in userspace and has no business > in the kernel. > > And we just need to know what usecase it is here. But I need > to understand. > Most definitely thinking hobbyist applications, so stepping up to DT/SPI/IIO would be massive overkill. While I understand the desire that those kernel features be used where appropriate, there are still cases where just being able to drive a GPIO pin from userspace is the best solution. I've been working with the Raspberry Pi where the accepted approach to userspace GPIO is based on gpiomem - a kernel module that provides memory mapped access to the bcm2835 registers. For reasons that I hope are obvious, I would like to move away from that to standard kernel APIs, so the GPIO UAPI. The changes to the GPIO UAPI that I've been discussing with Bart, (the direction change and pull up/down support) are to cover reasonably common operations that currently only have kernel space solutions. I'm happy to implement those changes, btw, but want to ensure that I'm on the right track so I don't go wasting anyone's time. Cheers, Kent.
On Sat, Sep 21, 2019 at 12:25:23PM +0200, Drew Fustini wrote: > Add pull-up/pull-down flags to the gpio line get and > set ioctl() calls. Use cases include a push button > that does not have an external resistor. > > Addition use cases described by Limor Fried (ladyada) of > Adafruit in this PR for Adafruit_Blinka Python lib: > https://github.com/adafruit/Adafruit_Blinka/pull/59 > > Signed-off-by: Drew Fustini <drew@pdp7.com> > --- > drivers/gpio/gpiolib.c | 12 ++++++++++++ > include/uapi/linux/gpio.h | 4 ++++ > 2 files changed, 16 insertions(+) > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c > index d9074191edef..9da1093cc7f5 100644 > --- a/drivers/gpio/gpiolib.c > +++ b/drivers/gpio/gpiolib.c > @@ -427,6 +427,8 @@ struct linehandle_state { > (GPIOHANDLE_REQUEST_INPUT | \ > GPIOHANDLE_REQUEST_OUTPUT | \ > GPIOHANDLE_REQUEST_ACTIVE_LOW | \ > + GPIOHANDLE_REQUEST_PULL_UP | \ > + GPIOHANDLE_REQUEST_PULL_DOWN | \ > GPIOHANDLE_REQUEST_OPEN_DRAIN | \ > GPIOHANDLE_REQUEST_OPEN_SOURCE) > > @@ -598,6 +600,10 @@ static int linehandle_create(struct gpio_device *gdev, void __user *ip) > set_bit(FLAG_OPEN_DRAIN, &desc->flags); > if (lflags & GPIOHANDLE_REQUEST_OPEN_SOURCE) > set_bit(FLAG_OPEN_SOURCE, &desc->flags); > + if (lflags & GPIOHANDLE_REQUEST_PULL_DOWN) > + set_bit(FLAG_PULL_DOWN, &desc->flags); > + if (lflags & GPIOHANDLE_REQUEST_PULL_UP) > + set_bit(FLAG_PULL_UP, &desc->flags); > > ret = gpiod_set_transitory(desc, false); > if (ret < 0) > @@ -1102,6 +1108,10 @@ static long gpio_ioctl(struct file *filp, unsigned int cmd, unsigned long arg) > if (test_bit(FLAG_OPEN_SOURCE, &desc->flags)) > lineinfo.flags |= (GPIOLINE_FLAG_OPEN_SOURCE | > GPIOLINE_FLAG_IS_OUT); > + if (test_bit(FLAG_PULL_DOWN, &desc->flags)) > + lineinfo.flags |= GPIOLINE_FLAG_PULL_DOWN; > + if (test_bit(FLAG_PULL_UP, &desc->flags)) > + lineinfo.flags |= GPIOLINE_FLAG_PULL_UP; > > if (copy_to_user(ip, &lineinfo, sizeof(lineinfo))) > return -EFAULT; > @@ -2475,6 +2485,8 @@ static bool gpiod_free_commit(struct gpio_desc *desc) > clear_bit(FLAG_REQUESTED, &desc->flags); > clear_bit(FLAG_OPEN_DRAIN, &desc->flags); > clear_bit(FLAG_OPEN_SOURCE, &desc->flags); > + clear_bit(FLAG_PULL_UP, &desc->flags); > + clear_bit(FLAG_PULL_DOWN, &desc->flags); > clear_bit(FLAG_IS_HOGGED, &desc->flags); > ret = true; > } > diff --git a/include/uapi/linux/gpio.h b/include/uapi/linux/gpio.h > index 4ebfe0ac6c5b..c2d1f7d908d6 100644 > --- a/include/uapi/linux/gpio.h > +++ b/include/uapi/linux/gpio.h > @@ -33,6 +33,8 @@ struct gpiochip_info { > #define GPIOLINE_FLAG_ACTIVE_LOW (1UL << 2) > #define GPIOLINE_FLAG_OPEN_DRAIN (1UL << 3) > #define GPIOLINE_FLAG_OPEN_SOURCE (1UL << 4) > +#define GPIOLINE_FLAG_PULL_UP (1UL << 5) > +#define GPIOLINE_FLAG_PULL_DOWN (1UL << 6) > > /** > * struct gpioline_info - Information about a certain GPIO line > @@ -62,6 +64,8 @@ struct gpioline_info { > #define GPIOHANDLE_REQUEST_ACTIVE_LOW (1UL << 2) > #define GPIOHANDLE_REQUEST_OPEN_DRAIN (1UL << 3) > #define GPIOHANDLE_REQUEST_OPEN_SOURCE (1UL << 4) > +#define GPIOHANDLE_REQUEST_PULL_UP (1UL << 5) > +#define GPIOHANDLE_REQUEST_PULL_DOWN (1UL << 6) > > /** > * struct gpiohandle_request - Information about a GPIO handle request > -- > 2.20.1 > Sorry for the late feedback, but I'm still not sure whether this patch is on track to be applied or not. I had thought not, at least not yet, but just in case... You have added the flags to linehandle_create, but not lineevent_create. Given that your primary use case is push buttons, isn't the event request at least as important as the handle request? Even ignoring your use case, I'd add them to lineevent_create just to maintain symmetry. Also, is it valid to set PULL_UP and PULL_DOWN simulaneously? I would think not, but I could be wrong. If not then that combination should be rejected with EINVAL. Cheers, Kent.
wt., 8 paź 2019 o 08:15 Kent Gibson <warthog618@gmail.com> napisał(a): > > On Sat, Sep 21, 2019 at 12:25:23PM +0200, Drew Fustini wrote: > > Add pull-up/pull-down flags to the gpio line get and > > set ioctl() calls. Use cases include a push button > > that does not have an external resistor. > > > > Addition use cases described by Limor Fried (ladyada) of > > Adafruit in this PR for Adafruit_Blinka Python lib: > > https://github.com/adafruit/Adafruit_Blinka/pull/59 > > > > Signed-off-by: Drew Fustini <drew@pdp7.com> > > --- > > drivers/gpio/gpiolib.c | 12 ++++++++++++ > > include/uapi/linux/gpio.h | 4 ++++ > > 2 files changed, 16 insertions(+) > > > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c > > index d9074191edef..9da1093cc7f5 100644 > > --- a/drivers/gpio/gpiolib.c > > +++ b/drivers/gpio/gpiolib.c > > @@ -427,6 +427,8 @@ struct linehandle_state { > > (GPIOHANDLE_REQUEST_INPUT | \ > > GPIOHANDLE_REQUEST_OUTPUT | \ > > GPIOHANDLE_REQUEST_ACTIVE_LOW | \ > > + GPIOHANDLE_REQUEST_PULL_UP | \ > > + GPIOHANDLE_REQUEST_PULL_DOWN | \ > > GPIOHANDLE_REQUEST_OPEN_DRAIN | \ > > GPIOHANDLE_REQUEST_OPEN_SOURCE) > > > > @@ -598,6 +600,10 @@ static int linehandle_create(struct gpio_device *gdev, void __user *ip) > > set_bit(FLAG_OPEN_DRAIN, &desc->flags); > > if (lflags & GPIOHANDLE_REQUEST_OPEN_SOURCE) > > set_bit(FLAG_OPEN_SOURCE, &desc->flags); > > + if (lflags & GPIOHANDLE_REQUEST_PULL_DOWN) > > + set_bit(FLAG_PULL_DOWN, &desc->flags); > > + if (lflags & GPIOHANDLE_REQUEST_PULL_UP) > > + set_bit(FLAG_PULL_UP, &desc->flags); > > > > ret = gpiod_set_transitory(desc, false); > > if (ret < 0) > > @@ -1102,6 +1108,10 @@ static long gpio_ioctl(struct file *filp, unsigned int cmd, unsigned long arg) > > if (test_bit(FLAG_OPEN_SOURCE, &desc->flags)) > > lineinfo.flags |= (GPIOLINE_FLAG_OPEN_SOURCE | > > GPIOLINE_FLAG_IS_OUT); > > + if (test_bit(FLAG_PULL_DOWN, &desc->flags)) > > + lineinfo.flags |= GPIOLINE_FLAG_PULL_DOWN; > > + if (test_bit(FLAG_PULL_UP, &desc->flags)) > > + lineinfo.flags |= GPIOLINE_FLAG_PULL_UP; > > > > if (copy_to_user(ip, &lineinfo, sizeof(lineinfo))) > > return -EFAULT; > > @@ -2475,6 +2485,8 @@ static bool gpiod_free_commit(struct gpio_desc *desc) > > clear_bit(FLAG_REQUESTED, &desc->flags); > > clear_bit(FLAG_OPEN_DRAIN, &desc->flags); > > clear_bit(FLAG_OPEN_SOURCE, &desc->flags); > > + clear_bit(FLAG_PULL_UP, &desc->flags); > > + clear_bit(FLAG_PULL_DOWN, &desc->flags); > > clear_bit(FLAG_IS_HOGGED, &desc->flags); > > ret = true; > > } > > diff --git a/include/uapi/linux/gpio.h b/include/uapi/linux/gpio.h > > index 4ebfe0ac6c5b..c2d1f7d908d6 100644 > > --- a/include/uapi/linux/gpio.h > > +++ b/include/uapi/linux/gpio.h > > @@ -33,6 +33,8 @@ struct gpiochip_info { > > #define GPIOLINE_FLAG_ACTIVE_LOW (1UL << 2) > > #define GPIOLINE_FLAG_OPEN_DRAIN (1UL << 3) > > #define GPIOLINE_FLAG_OPEN_SOURCE (1UL << 4) > > +#define GPIOLINE_FLAG_PULL_UP (1UL << 5) > > +#define GPIOLINE_FLAG_PULL_DOWN (1UL << 6) > > > > /** > > * struct gpioline_info - Information about a certain GPIO line > > @@ -62,6 +64,8 @@ struct gpioline_info { > > #define GPIOHANDLE_REQUEST_ACTIVE_LOW (1UL << 2) > > #define GPIOHANDLE_REQUEST_OPEN_DRAIN (1UL << 3) > > #define GPIOHANDLE_REQUEST_OPEN_SOURCE (1UL << 4) > > +#define GPIOHANDLE_REQUEST_PULL_UP (1UL << 5) > > +#define GPIOHANDLE_REQUEST_PULL_DOWN (1UL << 6) > > > > /** > > * struct gpiohandle_request - Information about a GPIO handle request > > -- > > 2.20.1 > > > Sorry for the late feedback, but I'm still not sure whether this patch > is on track to be applied or not. I had thought not, at least not yet, > but just in case... > It still needs some fixes, but Linus seems to be fine with the general idea. > You have added the flags to linehandle_create, but not lineevent_create. > Given that your primary use case is push buttons, isn't the event request > at least as important as the handle request? Even ignoring your use > case, I'd add them to lineevent_create just to maintain symmetry. > > Also, is it valid to set PULL_UP and PULL_DOWN simulaneously? > I would think not, but I could be wrong. > If not then that combination should be rejected with EINVAL. > Yes, some validity checks of the flags must be added. I even did it in my local branch[1]. Re: direction and configuration changes on requested lines: I think it's quite useful to add in the form of a new ioctl(): GPIOHANDLE_SET_CONFIG_IOCTL or something. I started hacking on this but eventually got more busy this week than I anticipated. I'm still planning on sending an RFC by the end of the week though. Bart [1] https://github.com/brgl/linux/commit/82fc38f6ab599ee03f7a8ed078de8abb41e6e611 > Cheers, > Kent.
On Tue, Oct 08, 2019 at 10:56:18PM +0200, Bartosz Golaszewski wrote: > wt., 8 paź 2019 o 08:15 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > On Sat, Sep 21, 2019 at 12:25:23PM +0200, Drew Fustini wrote: > > > Add pull-up/pull-down flags to the gpio line get and > > > set ioctl() calls. Use cases include a push button > > > that does not have an external resistor. > > > > > > Addition use cases described by Limor Fried (ladyada) of > > > Adafruit in this PR for Adafruit_Blinka Python lib: > > > https://github.com/adafruit/Adafruit_Blinka/pull/59 > > > > > > Signed-off-by: Drew Fustini <drew@pdp7.com> > > > --- > > > drivers/gpio/gpiolib.c | 12 ++++++++++++ > > > include/uapi/linux/gpio.h | 4 ++++ > > > 2 files changed, 16 insertions(+) > > > > > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c > > > index d9074191edef..9da1093cc7f5 100644 > > > --- a/drivers/gpio/gpiolib.c > > > +++ b/drivers/gpio/gpiolib.c > > > @@ -427,6 +427,8 @@ struct linehandle_state { > > > (GPIOHANDLE_REQUEST_INPUT | \ > > > GPIOHANDLE_REQUEST_OUTPUT | \ > > > GPIOHANDLE_REQUEST_ACTIVE_LOW | \ > > > + GPIOHANDLE_REQUEST_PULL_UP | \ > > > + GPIOHANDLE_REQUEST_PULL_DOWN | \ > > > GPIOHANDLE_REQUEST_OPEN_DRAIN | \ > > > GPIOHANDLE_REQUEST_OPEN_SOURCE) > > > > > > @@ -598,6 +600,10 @@ static int linehandle_create(struct gpio_device *gdev, void __user *ip) > > > set_bit(FLAG_OPEN_DRAIN, &desc->flags); > > > if (lflags & GPIOHANDLE_REQUEST_OPEN_SOURCE) > > > set_bit(FLAG_OPEN_SOURCE, &desc->flags); > > > + if (lflags & GPIOHANDLE_REQUEST_PULL_DOWN) > > > + set_bit(FLAG_PULL_DOWN, &desc->flags); > > > + if (lflags & GPIOHANDLE_REQUEST_PULL_UP) > > > + set_bit(FLAG_PULL_UP, &desc->flags); > > > > > > ret = gpiod_set_transitory(desc, false); > > > if (ret < 0) > > > @@ -1102,6 +1108,10 @@ static long gpio_ioctl(struct file *filp, unsigned int cmd, unsigned long arg) > > > if (test_bit(FLAG_OPEN_SOURCE, &desc->flags)) > > > lineinfo.flags |= (GPIOLINE_FLAG_OPEN_SOURCE | > > > GPIOLINE_FLAG_IS_OUT); > > > + if (test_bit(FLAG_PULL_DOWN, &desc->flags)) > > > + lineinfo.flags |= GPIOLINE_FLAG_PULL_DOWN; > > > + if (test_bit(FLAG_PULL_UP, &desc->flags)) > > > + lineinfo.flags |= GPIOLINE_FLAG_PULL_UP; > > > > > > if (copy_to_user(ip, &lineinfo, sizeof(lineinfo))) > > > return -EFAULT; > > > @@ -2475,6 +2485,8 @@ static bool gpiod_free_commit(struct gpio_desc *desc) > > > clear_bit(FLAG_REQUESTED, &desc->flags); > > > clear_bit(FLAG_OPEN_DRAIN, &desc->flags); > > > clear_bit(FLAG_OPEN_SOURCE, &desc->flags); > > > + clear_bit(FLAG_PULL_UP, &desc->flags); > > > + clear_bit(FLAG_PULL_DOWN, &desc->flags); > > > clear_bit(FLAG_IS_HOGGED, &desc->flags); > > > ret = true; > > > } > > > diff --git a/include/uapi/linux/gpio.h b/include/uapi/linux/gpio.h > > > index 4ebfe0ac6c5b..c2d1f7d908d6 100644 > > > --- a/include/uapi/linux/gpio.h > > > +++ b/include/uapi/linux/gpio.h > > > @@ -33,6 +33,8 @@ struct gpiochip_info { > > > #define GPIOLINE_FLAG_ACTIVE_LOW (1UL << 2) > > > #define GPIOLINE_FLAG_OPEN_DRAIN (1UL << 3) > > > #define GPIOLINE_FLAG_OPEN_SOURCE (1UL << 4) > > > +#define GPIOLINE_FLAG_PULL_UP (1UL << 5) > > > +#define GPIOLINE_FLAG_PULL_DOWN (1UL << 6) > > > > > > /** > > > * struct gpioline_info - Information about a certain GPIO line > > > @@ -62,6 +64,8 @@ struct gpioline_info { > > > #define GPIOHANDLE_REQUEST_ACTIVE_LOW (1UL << 2) > > > #define GPIOHANDLE_REQUEST_OPEN_DRAIN (1UL << 3) > > > #define GPIOHANDLE_REQUEST_OPEN_SOURCE (1UL << 4) > > > +#define GPIOHANDLE_REQUEST_PULL_UP (1UL << 5) > > > +#define GPIOHANDLE_REQUEST_PULL_DOWN (1UL << 6) > > > > > > /** > > > * struct gpiohandle_request - Information about a GPIO handle request > > > -- > > > 2.20.1 > > > > > Sorry for the late feedback, but I'm still not sure whether this patch > > is on track to be applied or not. I had thought not, at least not yet, > > but just in case... > > > > It still needs some fixes, but Linus seems to be fine with the general idea. > > > You have added the flags to linehandle_create, but not lineevent_create. > > Given that your primary use case is push buttons, isn't the event request > > at least as important as the handle request? Even ignoring your use > > case, I'd add them to lineevent_create just to maintain symmetry. > > > > Also, is it valid to set PULL_UP and PULL_DOWN simulaneously? > > I would think not, but I could be wrong. > > If not then that combination should be rejected with EINVAL. > > > > Yes, some validity checks of the flags must be added. I even did it in > my local branch[1]. > Your changes for linehandle_create look ok, but for lineevent_create you explicitly disabled PULL_UP and PULL_DOWN, and in a block labelled "This is just wrong: we don't look for events on output lines" at that. > Re: direction and configuration changes on requested lines: I think > it's quite useful to add in the form of a new ioctl(): > GPIOHANDLE_SET_CONFIG_IOCTL or something. I started hacking on this > but eventually got more busy this week than I anticipated. I'm still > planning on sending an RFC by the end of the week though. > I have the reverse problem - bored and looking for something to do, so more than willing to help out - if you want it. And while we're talking, does the gpio-mockup support pull up/down being set from the kernel side? Cheers, Kent. > > [1] https://github.com/brgl/linux/commit/82fc38f6ab599ee03f7a8ed078de8abb41e6e611 > > > Cheers, > > Kent.
śr., 9 paź 2019 o 01:21 Kent Gibson <warthog618@gmail.com> napisał(a): > > On Tue, Oct 08, 2019 at 10:56:18PM +0200, Bartosz Golaszewski wrote: > > wt., 8 paź 2019 o 08:15 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > > > On Sat, Sep 21, 2019 at 12:25:23PM +0200, Drew Fustini wrote: > > > > Add pull-up/pull-down flags to the gpio line get and > > > > set ioctl() calls. Use cases include a push button > > > > that does not have an external resistor. > > > > > > > > Addition use cases described by Limor Fried (ladyada) of > > > > Adafruit in this PR for Adafruit_Blinka Python lib: > > > > https://github.com/adafruit/Adafruit_Blinka/pull/59 > > > > > > > > Signed-off-by: Drew Fustini <drew@pdp7.com> > > > > --- > > > > drivers/gpio/gpiolib.c | 12 ++++++++++++ > > > > include/uapi/linux/gpio.h | 4 ++++ > > > > 2 files changed, 16 insertions(+) > > > > > > > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c > > > > index d9074191edef..9da1093cc7f5 100644 > > > > --- a/drivers/gpio/gpiolib.c > > > > +++ b/drivers/gpio/gpiolib.c > > > > @@ -427,6 +427,8 @@ struct linehandle_state { > > > > (GPIOHANDLE_REQUEST_INPUT | \ > > > > GPIOHANDLE_REQUEST_OUTPUT | \ > > > > GPIOHANDLE_REQUEST_ACTIVE_LOW | \ > > > > + GPIOHANDLE_REQUEST_PULL_UP | \ > > > > + GPIOHANDLE_REQUEST_PULL_DOWN | \ > > > > GPIOHANDLE_REQUEST_OPEN_DRAIN | \ > > > > GPIOHANDLE_REQUEST_OPEN_SOURCE) > > > > > > > > @@ -598,6 +600,10 @@ static int linehandle_create(struct gpio_device *gdev, void __user *ip) > > > > set_bit(FLAG_OPEN_DRAIN, &desc->flags); > > > > if (lflags & GPIOHANDLE_REQUEST_OPEN_SOURCE) > > > > set_bit(FLAG_OPEN_SOURCE, &desc->flags); > > > > + if (lflags & GPIOHANDLE_REQUEST_PULL_DOWN) > > > > + set_bit(FLAG_PULL_DOWN, &desc->flags); > > > > + if (lflags & GPIOHANDLE_REQUEST_PULL_UP) > > > > + set_bit(FLAG_PULL_UP, &desc->flags); > > > > > > > > ret = gpiod_set_transitory(desc, false); > > > > if (ret < 0) > > > > @@ -1102,6 +1108,10 @@ static long gpio_ioctl(struct file *filp, unsigned int cmd, unsigned long arg) > > > > if (test_bit(FLAG_OPEN_SOURCE, &desc->flags)) > > > > lineinfo.flags |= (GPIOLINE_FLAG_OPEN_SOURCE | > > > > GPIOLINE_FLAG_IS_OUT); > > > > + if (test_bit(FLAG_PULL_DOWN, &desc->flags)) > > > > + lineinfo.flags |= GPIOLINE_FLAG_PULL_DOWN; > > > > + if (test_bit(FLAG_PULL_UP, &desc->flags)) > > > > + lineinfo.flags |= GPIOLINE_FLAG_PULL_UP; > > > > > > > > if (copy_to_user(ip, &lineinfo, sizeof(lineinfo))) > > > > return -EFAULT; > > > > @@ -2475,6 +2485,8 @@ static bool gpiod_free_commit(struct gpio_desc *desc) > > > > clear_bit(FLAG_REQUESTED, &desc->flags); > > > > clear_bit(FLAG_OPEN_DRAIN, &desc->flags); > > > > clear_bit(FLAG_OPEN_SOURCE, &desc->flags); > > > > + clear_bit(FLAG_PULL_UP, &desc->flags); > > > > + clear_bit(FLAG_PULL_DOWN, &desc->flags); > > > > clear_bit(FLAG_IS_HOGGED, &desc->flags); > > > > ret = true; > > > > } > > > > diff --git a/include/uapi/linux/gpio.h b/include/uapi/linux/gpio.h > > > > index 4ebfe0ac6c5b..c2d1f7d908d6 100644 > > > > --- a/include/uapi/linux/gpio.h > > > > +++ b/include/uapi/linux/gpio.h > > > > @@ -33,6 +33,8 @@ struct gpiochip_info { > > > > #define GPIOLINE_FLAG_ACTIVE_LOW (1UL << 2) > > > > #define GPIOLINE_FLAG_OPEN_DRAIN (1UL << 3) > > > > #define GPIOLINE_FLAG_OPEN_SOURCE (1UL << 4) > > > > +#define GPIOLINE_FLAG_PULL_UP (1UL << 5) > > > > +#define GPIOLINE_FLAG_PULL_DOWN (1UL << 6) > > > > > > > > /** > > > > * struct gpioline_info - Information about a certain GPIO line > > > > @@ -62,6 +64,8 @@ struct gpioline_info { > > > > #define GPIOHANDLE_REQUEST_ACTIVE_LOW (1UL << 2) > > > > #define GPIOHANDLE_REQUEST_OPEN_DRAIN (1UL << 3) > > > > #define GPIOHANDLE_REQUEST_OPEN_SOURCE (1UL << 4) > > > > +#define GPIOHANDLE_REQUEST_PULL_UP (1UL << 5) > > > > +#define GPIOHANDLE_REQUEST_PULL_DOWN (1UL << 6) > > > > > > > > /** > > > > * struct gpiohandle_request - Information about a GPIO handle request > > > > -- > > > > 2.20.1 > > > > > > > Sorry for the late feedback, but I'm still not sure whether this patch > > > is on track to be applied or not. I had thought not, at least not yet, > > > but just in case... > > > > > > > It still needs some fixes, but Linus seems to be fine with the general idea. > > > > > You have added the flags to linehandle_create, but not lineevent_create. > > > Given that your primary use case is push buttons, isn't the event request > > > at least as important as the handle request? Even ignoring your use > > > case, I'd add them to lineevent_create just to maintain symmetry. > > > > > > Also, is it valid to set PULL_UP and PULL_DOWN simulaneously? > > > I would think not, but I could be wrong. > > > If not then that combination should be rejected with EINVAL. > > > > > > > Yes, some validity checks of the flags must be added. I even did it in > > my local branch[1]. > > > Your changes for linehandle_create look ok, but for lineevent_create > you explicitly disabled PULL_UP and PULL_DOWN, and in a block labelled > "This is just wrong: we don't look for events on output lines" at that. > Yeah, feel free to change it. > > Re: direction and configuration changes on requested lines: I think > > it's quite useful to add in the form of a new ioctl(): > > GPIOHANDLE_SET_CONFIG_IOCTL or something. I started hacking on this > > but eventually got more busy this week than I anticipated. I'm still > > planning on sending an RFC by the end of the week though. > > > I have the reverse problem - bored and looking for something to do, so > more than willing to help out - if you want it. > Sure, in that case let's just wait for your patches. You can just extend what I started, if you wish. > And while we're talking, does the gpio-mockup support pull up/down > being set from the kernel side? > No, but feel free to add it. Bart > Cheers, > Kent. > > > > [1] https://github.com/brgl/linux/commit/82fc38f6ab599ee03f7a8ed078de8abb41e6e611 > > > > > Cheers, > > > Kent.
On Wed, Oct 09, 2019 at 01:30:18AM +0200, Bartosz Golaszewski wrote: > śr., 9 paź 2019 o 01:21 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > On Tue, Oct 08, 2019 at 10:56:18PM +0200, Bartosz Golaszewski wrote: > > > wt., 8 paź 2019 o 08:15 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > > > > > On Sat, Sep 21, 2019 at 12:25:23PM +0200, Drew Fustini wrote: > > > > > Add pull-up/pull-down flags to the gpio line get and > > > > > set ioctl() calls. Use cases include a push button > > > > > that does not have an external resistor. > > > > > > > > > > Addition use cases described by Limor Fried (ladyada) of > > > > > Adafruit in this PR for Adafruit_Blinka Python lib: > > > > > https://github.com/adafruit/Adafruit_Blinka/pull/59 > > > > > > > > > > Signed-off-by: Drew Fustini <drew@pdp7.com> > > > > > --- > > > > > drivers/gpio/gpiolib.c | 12 ++++++++++++ > > > > > include/uapi/linux/gpio.h | 4 ++++ > > > > > 2 files changed, 16 insertions(+) > > > > > > > > > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c > > > > > index d9074191edef..9da1093cc7f5 100644 > > > > > --- a/drivers/gpio/gpiolib.c > > > > > +++ b/drivers/gpio/gpiolib.c > > > > > @@ -427,6 +427,8 @@ struct linehandle_state { > > > > > (GPIOHANDLE_REQUEST_INPUT | \ > > > > > GPIOHANDLE_REQUEST_OUTPUT | \ > > > > > GPIOHANDLE_REQUEST_ACTIVE_LOW | \ > > > > > + GPIOHANDLE_REQUEST_PULL_UP | \ > > > > > + GPIOHANDLE_REQUEST_PULL_DOWN | \ > > > > > GPIOHANDLE_REQUEST_OPEN_DRAIN | \ > > > > > GPIOHANDLE_REQUEST_OPEN_SOURCE) > > > > > > > > > > @@ -598,6 +600,10 @@ static int linehandle_create(struct gpio_device *gdev, void __user *ip) > > > > > set_bit(FLAG_OPEN_DRAIN, &desc->flags); > > > > > if (lflags & GPIOHANDLE_REQUEST_OPEN_SOURCE) > > > > > set_bit(FLAG_OPEN_SOURCE, &desc->flags); > > > > > + if (lflags & GPIOHANDLE_REQUEST_PULL_DOWN) > > > > > + set_bit(FLAG_PULL_DOWN, &desc->flags); > > > > > + if (lflags & GPIOHANDLE_REQUEST_PULL_UP) > > > > > + set_bit(FLAG_PULL_UP, &desc->flags); > > > > > > > > > > ret = gpiod_set_transitory(desc, false); > > > > > if (ret < 0) > > > > > @@ -1102,6 +1108,10 @@ static long gpio_ioctl(struct file *filp, unsigned int cmd, unsigned long arg) > > > > > if (test_bit(FLAG_OPEN_SOURCE, &desc->flags)) > > > > > lineinfo.flags |= (GPIOLINE_FLAG_OPEN_SOURCE | > > > > > GPIOLINE_FLAG_IS_OUT); > > > > > + if (test_bit(FLAG_PULL_DOWN, &desc->flags)) > > > > > + lineinfo.flags |= GPIOLINE_FLAG_PULL_DOWN; > > > > > + if (test_bit(FLAG_PULL_UP, &desc->flags)) > > > > > + lineinfo.flags |= GPIOLINE_FLAG_PULL_UP; > > > > > > > > > > if (copy_to_user(ip, &lineinfo, sizeof(lineinfo))) > > > > > return -EFAULT; > > > > > @@ -2475,6 +2485,8 @@ static bool gpiod_free_commit(struct gpio_desc *desc) > > > > > clear_bit(FLAG_REQUESTED, &desc->flags); > > > > > clear_bit(FLAG_OPEN_DRAIN, &desc->flags); > > > > > clear_bit(FLAG_OPEN_SOURCE, &desc->flags); > > > > > + clear_bit(FLAG_PULL_UP, &desc->flags); > > > > > + clear_bit(FLAG_PULL_DOWN, &desc->flags); > > > > > clear_bit(FLAG_IS_HOGGED, &desc->flags); > > > > > ret = true; > > > > > } > > > > > diff --git a/include/uapi/linux/gpio.h b/include/uapi/linux/gpio.h > > > > > index 4ebfe0ac6c5b..c2d1f7d908d6 100644 > > > > > --- a/include/uapi/linux/gpio.h > > > > > +++ b/include/uapi/linux/gpio.h > > > > > @@ -33,6 +33,8 @@ struct gpiochip_info { > > > > > #define GPIOLINE_FLAG_ACTIVE_LOW (1UL << 2) > > > > > #define GPIOLINE_FLAG_OPEN_DRAIN (1UL << 3) > > > > > #define GPIOLINE_FLAG_OPEN_SOURCE (1UL << 4) > > > > > +#define GPIOLINE_FLAG_PULL_UP (1UL << 5) > > > > > +#define GPIOLINE_FLAG_PULL_DOWN (1UL << 6) > > > > > > > > > > /** > > > > > * struct gpioline_info - Information about a certain GPIO line > > > > > @@ -62,6 +64,8 @@ struct gpioline_info { > > > > > #define GPIOHANDLE_REQUEST_ACTIVE_LOW (1UL << 2) > > > > > #define GPIOHANDLE_REQUEST_OPEN_DRAIN (1UL << 3) > > > > > #define GPIOHANDLE_REQUEST_OPEN_SOURCE (1UL << 4) > > > > > +#define GPIOHANDLE_REQUEST_PULL_UP (1UL << 5) > > > > > +#define GPIOHANDLE_REQUEST_PULL_DOWN (1UL << 6) > > > > > > > > > > /** > > > > > * struct gpiohandle_request - Information about a GPIO handle request > > > > > -- > > > > > 2.20.1 > > > > > > > > > Sorry for the late feedback, but I'm still not sure whether this patch > > > > is on track to be applied or not. I had thought not, at least not yet, > > > > but just in case... > > > > > > > > > > It still needs some fixes, but Linus seems to be fine with the general idea. > > > > > > > You have added the flags to linehandle_create, but not lineevent_create. > > > > Given that your primary use case is push buttons, isn't the event request > > > > at least as important as the handle request? Even ignoring your use > > > > case, I'd add them to lineevent_create just to maintain symmetry. > > > > > > > > Also, is it valid to set PULL_UP and PULL_DOWN simulaneously? > > > > I would think not, but I could be wrong. > > > > If not then that combination should be rejected with EINVAL. > > > > > > > > > > Yes, some validity checks of the flags must be added. I even did it in > > > my local branch[1]. > > > > > Your changes for linehandle_create look ok, but for lineevent_create > > you explicitly disabled PULL_UP and PULL_DOWN, and in a block labelled > > "This is just wrong: we don't look for events on output lines" at that. > > > > Yeah, feel free to change it. > Yeah, you see I didn't feel free to change it, as I thought the appropriate etiquette was to send comments to the original author. At least until the patch has been applied. Or are RFCs a special case? > > > Re: direction and configuration changes on requested lines: I think > > > it's quite useful to add in the form of a new ioctl(): > > > GPIOHANDLE_SET_CONFIG_IOCTL or something. I started hacking on this > > > but eventually got more busy this week than I anticipated. I'm still > > > planning on sending an RFC by the end of the week though. > > > > > I have the reverse problem - bored and looking for something to do, so > > more than willing to help out - if you want it. > > > > Sure, in that case let's just wait for your patches. You can just > extend what I started, if you wish. > Yep, you sound tired. Are we talking about the SET_CONFIG here? If you have a link to what you've started I'll take a look, but as I've already said - I'm not about to go writing new ioctls without approval and guidance from you and Linus. If you have the ioctl defined I'm happy to take it from there. Well happy-ish. > > And while we're talking, does the gpio-mockup support pull up/down > > being set from the kernel side? > > > > No, but feel free to add it. > Ok, will have a look - cos I would like to add tests for the above and that will require the mockup supporting it as well. Cheers, Kent.
śr., 9 paź 2019 o 01:56 Kent Gibson <warthog618@gmail.com> napisał(a): > > On Wed, Oct 09, 2019 at 01:30:18AM +0200, Bartosz Golaszewski wrote: > > śr., 9 paź 2019 o 01:21 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > > > On Tue, Oct 08, 2019 at 10:56:18PM +0200, Bartosz Golaszewski wrote: > > > > wt., 8 paź 2019 o 08:15 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > > > > > > > On Sat, Sep 21, 2019 at 12:25:23PM +0200, Drew Fustini wrote: > > > > > > Add pull-up/pull-down flags to the gpio line get and > > > > > > set ioctl() calls. Use cases include a push button > > > > > > that does not have an external resistor. > > > > > > > > > > > > Addition use cases described by Limor Fried (ladyada) of > > > > > > Adafruit in this PR for Adafruit_Blinka Python lib: > > > > > > https://github.com/adafruit/Adafruit_Blinka/pull/59 > > > > > > > > > > > > Signed-off-by: Drew Fustini <drew@pdp7.com> > > > > > > --- > > > > > > drivers/gpio/gpiolib.c | 12 ++++++++++++ > > > > > > include/uapi/linux/gpio.h | 4 ++++ > > > > > > 2 files changed, 16 insertions(+) > > > > > > > > > > > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c > > > > > > index d9074191edef..9da1093cc7f5 100644 > > > > > > --- a/drivers/gpio/gpiolib.c > > > > > > +++ b/drivers/gpio/gpiolib.c > > > > > > @@ -427,6 +427,8 @@ struct linehandle_state { > > > > > > (GPIOHANDLE_REQUEST_INPUT | \ > > > > > > GPIOHANDLE_REQUEST_OUTPUT | \ > > > > > > GPIOHANDLE_REQUEST_ACTIVE_LOW | \ > > > > > > + GPIOHANDLE_REQUEST_PULL_UP | \ > > > > > > + GPIOHANDLE_REQUEST_PULL_DOWN | \ > > > > > > GPIOHANDLE_REQUEST_OPEN_DRAIN | \ > > > > > > GPIOHANDLE_REQUEST_OPEN_SOURCE) > > > > > > > > > > > > @@ -598,6 +600,10 @@ static int linehandle_create(struct gpio_device *gdev, void __user *ip) > > > > > > set_bit(FLAG_OPEN_DRAIN, &desc->flags); > > > > > > if (lflags & GPIOHANDLE_REQUEST_OPEN_SOURCE) > > > > > > set_bit(FLAG_OPEN_SOURCE, &desc->flags); > > > > > > + if (lflags & GPIOHANDLE_REQUEST_PULL_DOWN) > > > > > > + set_bit(FLAG_PULL_DOWN, &desc->flags); > > > > > > + if (lflags & GPIOHANDLE_REQUEST_PULL_UP) > > > > > > + set_bit(FLAG_PULL_UP, &desc->flags); > > > > > > > > > > > > ret = gpiod_set_transitory(desc, false); > > > > > > if (ret < 0) > > > > > > @@ -1102,6 +1108,10 @@ static long gpio_ioctl(struct file *filp, unsigned int cmd, unsigned long arg) > > > > > > if (test_bit(FLAG_OPEN_SOURCE, &desc->flags)) > > > > > > lineinfo.flags |= (GPIOLINE_FLAG_OPEN_SOURCE | > > > > > > GPIOLINE_FLAG_IS_OUT); > > > > > > + if (test_bit(FLAG_PULL_DOWN, &desc->flags)) > > > > > > + lineinfo.flags |= GPIOLINE_FLAG_PULL_DOWN; > > > > > > + if (test_bit(FLAG_PULL_UP, &desc->flags)) > > > > > > + lineinfo.flags |= GPIOLINE_FLAG_PULL_UP; > > > > > > > > > > > > if (copy_to_user(ip, &lineinfo, sizeof(lineinfo))) > > > > > > return -EFAULT; > > > > > > @@ -2475,6 +2485,8 @@ static bool gpiod_free_commit(struct gpio_desc *desc) > > > > > > clear_bit(FLAG_REQUESTED, &desc->flags); > > > > > > clear_bit(FLAG_OPEN_DRAIN, &desc->flags); > > > > > > clear_bit(FLAG_OPEN_SOURCE, &desc->flags); > > > > > > + clear_bit(FLAG_PULL_UP, &desc->flags); > > > > > > + clear_bit(FLAG_PULL_DOWN, &desc->flags); > > > > > > clear_bit(FLAG_IS_HOGGED, &desc->flags); > > > > > > ret = true; > > > > > > } > > > > > > diff --git a/include/uapi/linux/gpio.h b/include/uapi/linux/gpio.h > > > > > > index 4ebfe0ac6c5b..c2d1f7d908d6 100644 > > > > > > --- a/include/uapi/linux/gpio.h > > > > > > +++ b/include/uapi/linux/gpio.h > > > > > > @@ -33,6 +33,8 @@ struct gpiochip_info { > > > > > > #define GPIOLINE_FLAG_ACTIVE_LOW (1UL << 2) > > > > > > #define GPIOLINE_FLAG_OPEN_DRAIN (1UL << 3) > > > > > > #define GPIOLINE_FLAG_OPEN_SOURCE (1UL << 4) > > > > > > +#define GPIOLINE_FLAG_PULL_UP (1UL << 5) > > > > > > +#define GPIOLINE_FLAG_PULL_DOWN (1UL << 6) > > > > > > > > > > > > /** > > > > > > * struct gpioline_info - Information about a certain GPIO line > > > > > > @@ -62,6 +64,8 @@ struct gpioline_info { > > > > > > #define GPIOHANDLE_REQUEST_ACTIVE_LOW (1UL << 2) > > > > > > #define GPIOHANDLE_REQUEST_OPEN_DRAIN (1UL << 3) > > > > > > #define GPIOHANDLE_REQUEST_OPEN_SOURCE (1UL << 4) > > > > > > +#define GPIOHANDLE_REQUEST_PULL_UP (1UL << 5) > > > > > > +#define GPIOHANDLE_REQUEST_PULL_DOWN (1UL << 6) > > > > > > > > > > > > /** > > > > > > * struct gpiohandle_request - Information about a GPIO handle request > > > > > > -- > > > > > > 2.20.1 > > > > > > > > > > > Sorry for the late feedback, but I'm still not sure whether this patch > > > > > is on track to be applied or not. I had thought not, at least not yet, > > > > > but just in case... > > > > > > > > > > > > > It still needs some fixes, but Linus seems to be fine with the general idea. > > > > > > > > > You have added the flags to linehandle_create, but not lineevent_create. > > > > > Given that your primary use case is push buttons, isn't the event request > > > > > at least as important as the handle request? Even ignoring your use > > > > > case, I'd add them to lineevent_create just to maintain symmetry. > > > > > > > > > > Also, is it valid to set PULL_UP and PULL_DOWN simulaneously? > > > > > I would think not, but I could be wrong. > > > > > If not then that combination should be rejected with EINVAL. > > > > > > > > > > > > > Yes, some validity checks of the flags must be added. I even did it in > > > > my local branch[1]. > > > > > > > Your changes for linehandle_create look ok, but for lineevent_create > > > you explicitly disabled PULL_UP and PULL_DOWN, and in a block labelled > > > "This is just wrong: we don't look for events on output lines" at that. > > > > > > > Yeah, feel free to change it. > > > Yeah, you see I didn't feel free to change it, as I thought the > appropriate etiquette was to send comments to the original author. > At least until the patch has been applied. > Or are RFCs a special case? > So either you review it and the author changes it and resends, or - if for example you want to build on top of someone else's changes - you take the patch, keep the original authorship, modify it and add your signed-off-by tag with a small note on your changes in square brackets. > > > > Re: direction and configuration changes on requested lines: I think > > > > it's quite useful to add in the form of a new ioctl(): > > > > GPIOHANDLE_SET_CONFIG_IOCTL or something. I started hacking on this > > > > but eventually got more busy this week than I anticipated. I'm still > > > > planning on sending an RFC by the end of the week though. > > > > > > > I have the reverse problem - bored and looking for something to do, so > > > more than willing to help out - if you want it. > > > > > > > Sure, in that case let's just wait for your patches. You can just > > extend what I started, if you wish. > > > Yep, you sound tired. > Are we talking about the SET_CONFIG here? If you have a link to what > you've started I'll take a look, but as I've already said - I'm not > about to go writing new ioctls without approval and guidance from you > and Linus. If you have the ioctl defined I'm happy to take it from > there. Well happy-ish. > But there's absolutely nothing wrong with writing code even if you're not sure about the solution - it's even encouraged[1]. It's much easier to discuss existing code than potential solutions. [1] https://lkml.org/lkml/2000/8/25/132 > > > And while we're talking, does the gpio-mockup support pull up/down > > > being set from the kernel side? > > > > > > > No, but feel free to add it. > > > Ok, will have a look - cos I would like to add tests for the above and > that will require the mockup supporting it as wel Do you also plan on extending libgpiod by any chance? Bart > > Cheers, > Kent. >
On Wed, Oct 09, 2019 at 02:03:58AM +0200, Bartosz Golaszewski wrote: > śr., 9 paź 2019 o 01:56 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > On Wed, Oct 09, 2019 at 01:30:18AM +0200, Bartosz Golaszewski wrote: > > > śr., 9 paź 2019 o 01:21 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > > > > > On Tue, Oct 08, 2019 at 10:56:18PM +0200, Bartosz Golaszewski wrote: > > > > > wt., 8 paź 2019 o 08:15 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > > > > > > > > > On Sat, Sep 21, 2019 at 12:25:23PM +0200, Drew Fustini wrote: > > > > > > > Add pull-up/pull-down flags to the gpio line get and > > > > > > > set ioctl() calls. Use cases include a push button > > > > > > > that does not have an external resistor. > > > > > > > > > > > > > > Addition use cases described by Limor Fried (ladyada) of > > > > > > > Adafruit in this PR for Adafruit_Blinka Python lib: > > > > > > > https://github.com/adafruit/Adafruit_Blinka/pull/59 > > > > > > > > > > > > > > Signed-off-by: Drew Fustini <drew@pdp7.com> > > > > > > > --- > > > > > > > drivers/gpio/gpiolib.c | 12 ++++++++++++ > > > > > > > include/uapi/linux/gpio.h | 4 ++++ > > > > > > > 2 files changed, 16 insertions(+) > > > > > > > > > > > > > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c > > > > > > > index d9074191edef..9da1093cc7f5 100644 > > > > > > > --- a/drivers/gpio/gpiolib.c > > > > > > > +++ b/drivers/gpio/gpiolib.c > > > > > > > @@ -427,6 +427,8 @@ struct linehandle_state { > > > > > > > (GPIOHANDLE_REQUEST_INPUT | \ > > > > > > > GPIOHANDLE_REQUEST_OUTPUT | \ > > > > > > > GPIOHANDLE_REQUEST_ACTIVE_LOW | \ > > > > > > > + GPIOHANDLE_REQUEST_PULL_UP | \ > > > > > > > + GPIOHANDLE_REQUEST_PULL_DOWN | \ > > > > > > > GPIOHANDLE_REQUEST_OPEN_DRAIN | \ > > > > > > > GPIOHANDLE_REQUEST_OPEN_SOURCE) > > > > > > > > > > > > > > @@ -598,6 +600,10 @@ static int linehandle_create(struct gpio_device *gdev, void __user *ip) > > > > > > > set_bit(FLAG_OPEN_DRAIN, &desc->flags); > > > > > > > if (lflags & GPIOHANDLE_REQUEST_OPEN_SOURCE) > > > > > > > set_bit(FLAG_OPEN_SOURCE, &desc->flags); > > > > > > > + if (lflags & GPIOHANDLE_REQUEST_PULL_DOWN) > > > > > > > + set_bit(FLAG_PULL_DOWN, &desc->flags); > > > > > > > + if (lflags & GPIOHANDLE_REQUEST_PULL_UP) > > > > > > > + set_bit(FLAG_PULL_UP, &desc->flags); > > > > > > > > > > > > > > ret = gpiod_set_transitory(desc, false); > > > > > > > if (ret < 0) > > > > > > > @@ -1102,6 +1108,10 @@ static long gpio_ioctl(struct file *filp, unsigned int cmd, unsigned long arg) > > > > > > > if (test_bit(FLAG_OPEN_SOURCE, &desc->flags)) > > > > > > > lineinfo.flags |= (GPIOLINE_FLAG_OPEN_SOURCE | > > > > > > > GPIOLINE_FLAG_IS_OUT); > > > > > > > + if (test_bit(FLAG_PULL_DOWN, &desc->flags)) > > > > > > > + lineinfo.flags |= GPIOLINE_FLAG_PULL_DOWN; > > > > > > > + if (test_bit(FLAG_PULL_UP, &desc->flags)) > > > > > > > + lineinfo.flags |= GPIOLINE_FLAG_PULL_UP; > > > > > > > > > > > > > > if (copy_to_user(ip, &lineinfo, sizeof(lineinfo))) > > > > > > > return -EFAULT; > > > > > > > @@ -2475,6 +2485,8 @@ static bool gpiod_free_commit(struct gpio_desc *desc) > > > > > > > clear_bit(FLAG_REQUESTED, &desc->flags); > > > > > > > clear_bit(FLAG_OPEN_DRAIN, &desc->flags); > > > > > > > clear_bit(FLAG_OPEN_SOURCE, &desc->flags); > > > > > > > + clear_bit(FLAG_PULL_UP, &desc->flags); > > > > > > > + clear_bit(FLAG_PULL_DOWN, &desc->flags); > > > > > > > clear_bit(FLAG_IS_HOGGED, &desc->flags); > > > > > > > ret = true; > > > > > > > } > > > > > > > diff --git a/include/uapi/linux/gpio.h b/include/uapi/linux/gpio.h > > > > > > > index 4ebfe0ac6c5b..c2d1f7d908d6 100644 > > > > > > > --- a/include/uapi/linux/gpio.h > > > > > > > +++ b/include/uapi/linux/gpio.h > > > > > > > @@ -33,6 +33,8 @@ struct gpiochip_info { > > > > > > > #define GPIOLINE_FLAG_ACTIVE_LOW (1UL << 2) > > > > > > > #define GPIOLINE_FLAG_OPEN_DRAIN (1UL << 3) > > > > > > > #define GPIOLINE_FLAG_OPEN_SOURCE (1UL << 4) > > > > > > > +#define GPIOLINE_FLAG_PULL_UP (1UL << 5) > > > > > > > +#define GPIOLINE_FLAG_PULL_DOWN (1UL << 6) > > > > > > > > > > > > > > /** > > > > > > > * struct gpioline_info - Information about a certain GPIO line > > > > > > > @@ -62,6 +64,8 @@ struct gpioline_info { > > > > > > > #define GPIOHANDLE_REQUEST_ACTIVE_LOW (1UL << 2) > > > > > > > #define GPIOHANDLE_REQUEST_OPEN_DRAIN (1UL << 3) > > > > > > > #define GPIOHANDLE_REQUEST_OPEN_SOURCE (1UL << 4) > > > > > > > +#define GPIOHANDLE_REQUEST_PULL_UP (1UL << 5) > > > > > > > +#define GPIOHANDLE_REQUEST_PULL_DOWN (1UL << 6) > > > > > > > > > > > > > > /** > > > > > > > * struct gpiohandle_request - Information about a GPIO handle request > > > > > > > -- > > > > > > > 2.20.1 > > > > > > > > > > > > > Sorry for the late feedback, but I'm still not sure whether this patch > > > > > > is on track to be applied or not. I had thought not, at least not yet, > > > > > > but just in case... > > > > > > > > > > > > > > > > It still needs some fixes, but Linus seems to be fine with the general idea. > > > > > > > > > > > You have added the flags to linehandle_create, but not lineevent_create. > > > > > > Given that your primary use case is push buttons, isn't the event request > > > > > > at least as important as the handle request? Even ignoring your use > > > > > > case, I'd add them to lineevent_create just to maintain symmetry. > > > > > > > > > > > > Also, is it valid to set PULL_UP and PULL_DOWN simulaneously? > > > > > > I would think not, but I could be wrong. > > > > > > If not then that combination should be rejected with EINVAL. > > > > > > > > > > > > > > > > Yes, some validity checks of the flags must be added. I even did it in > > > > > my local branch[1]. > > > > > > > > > Your changes for linehandle_create look ok, but for lineevent_create > > > > you explicitly disabled PULL_UP and PULL_DOWN, and in a block labelled > > > > "This is just wrong: we don't look for events on output lines" at that. > > > > > > > > > > Yeah, feel free to change it. > > > > > Yeah, you see I didn't feel free to change it, as I thought the > > appropriate etiquette was to send comments to the original author. > > At least until the patch has been applied. > > Or are RFCs a special case? > > > > So either you review it and the author changes it and resends, or - if > for example you want to build on top of someone else's changes - you > take the patch, keep the original authorship, modify it and add your > signed-off-by tag with a small note on your changes in square > brackets. > Ok, but I'd still expect some feedback to the author to that effect. Maybe I missed it. > > > > > Re: direction and configuration changes on requested lines: I think > > > > > it's quite useful to add in the form of a new ioctl(): > > > > > GPIOHANDLE_SET_CONFIG_IOCTL or something. I started hacking on this > > > > > but eventually got more busy this week than I anticipated. I'm still > > > > > planning on sending an RFC by the end of the week though. > > > > > > > > > I have the reverse problem - bored and looking for something to do, so > > > > more than willing to help out - if you want it. > > > > > > > > > > Sure, in that case let's just wait for your patches. You can just > > > extend what I started, if you wish. > > > > > Yep, you sound tired. > > Are we talking about the SET_CONFIG here? If you have a link to what > > you've started I'll take a look, but as I've already said - I'm not > > about to go writing new ioctls without approval and guidance from you > > and Linus. If you have the ioctl defined I'm happy to take it from > > there. Well happy-ish. > > > > But there's absolutely nothing wrong with writing code even if you're > not sure about the solution - it's even encouraged[1]. It's much > easier to discuss existing code than potential solutions. > > [1] https://lkml.org/lkml/2000/8/25/132 > Exactly - show me your code. > > > > And while we're talking, does the gpio-mockup support pull up/down > > > > being set from the kernel side? > > > > > > > > > > No, but feel free to add it. > > > > > Ok, will have a look - cos I would like to add tests for the above and > > that will require the mockup supporting it as wel > > Do you also plan on extending libgpiod by any chance? > Well I thought that was your baby and was going to extend my Go library, and leave libgpiod to you. But I can help out with libgpiod as well. I suppose you'll want the Python and C++ bindings updated to match? Cheers, Kent.
On Wed, Oct 09, 2019 at 08:22:11AM +0800, Kent Gibson wrote: > On Wed, Oct 09, 2019 at 02:03:58AM +0200, Bartosz Golaszewski wrote: > > śr., 9 paź 2019 o 01:56 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > But there's absolutely nothing wrong with writing code even if you're > > not sure about the solution - it's even encouraged[1]. It's much > > easier to discuss existing code than potential solutions. > > > > [1] https://lkml.org/lkml/2000/8/25/132 > > > Exactly - show me your code. > Safe to assume your code is in your topic/gpio-uapi-config branch? I hope so, cos I've forked from that into github.com/warthog618/linux and have started working on it. So far I've added pull up/down support to lineevent_create, and added default_values to the gpiohandle_config (for setting outputs). Currently looking at implementing the actual set config, and sucking on whether that is best done by refactoring bit out of linehandle_create. Will sort that into patches once it is in a fit state. Feel free to poke me if I'm going off track. Cheers, Kent.
set pull-up/down flags in lineevent_create and add sanity checks
Check the pull-up/down flags in lineevent_create() and set the
corresponding bits.
Add sanity checks to make pull-up and pull-down flags mutually
exclusive and only valid when the line is an input.
Signed-off-by: Drew Fustini <drew@pdp7.com>
---
drivers/gpio/gpiolib.c | 36 ++++++++++++++++++++++++++++++++++++
1 file changed, 36 insertions(+)
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 646ae4cffe26..babc26267561 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -554,6 +554,20 @@ static int linehandle_create(struct gpio_device *gdev, void __user *ip)
(lflags & GPIOHANDLE_REQUEST_OPEN_SOURCE)))
return -EINVAL;
+ /*
+ * Do not allow PULL_UP & PULL_DOWN flags to be set as they are
+ * contradictory.
+ */
+ if ((lflags & GPIOHANDLE_REQUEST_PULL_UP) &&
+ (lflags & GPIOHANDLE_REQUEST_PULL_DOWN))
+ return -EINVAL;
+
+ /* PULL_UP and PULL_DOWN flags only make sense for input mode. */
+ if (!(lflags & GPIOHANDLE_REQUEST_INPUT) &&
+ ((lflags & GPIOHANDLE_REQUEST_PULL_UP) ||
+ (lflags & GPIOHANDLE_REQUEST_PULL_DOWN)))
+ return -EINVAL;
+
lh = kzalloc(sizeof(*lh), GFP_KERNEL);
if (!lh)
return -ENOMEM;
@@ -941,6 +955,24 @@ static int lineevent_create(struct gpio_device *gdev, void __user *ip)
goto out_free_label;
}
+ /*
+ * Do not allow PULL_UP & PULL_DOWN flags to be set as they are
+ * contradictory.
+ */
+ if ((lflags & GPIOHANDLE_REQUEST_PULL_UP) &&
+ (lflags & GPIOHANDLE_REQUEST_PULL_DOWN)) {
+ ret = -EINVAL;
+ goto out_free_label;
+ }
+
+ /* PULL_UP and PULL_DOWN flags only make sense for input mode. */
+ if (!(lflags & GPIOHANDLE_REQUEST_INPUT) &&
+ ((lflags & GPIOHANDLE_REQUEST_PULL_UP) ||
+ (lflags & GPIOHANDLE_REQUEST_PULL_DOWN))) {
+ ret = -EINVAL;
+ goto out_free_label;
+ }
+
desc = &gdev->descs[offset];
ret = gpiod_request(desc, le->label);
if (ret)
@@ -950,6 +982,10 @@ static int lineevent_create(struct gpio_device *gdev, void __user *ip)
if (lflags & GPIOHANDLE_REQUEST_ACTIVE_LOW)
set_bit(FLAG_ACTIVE_LOW, &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);
ret = gpiod_direction_input(desc);
if (ret)
On Tue, Oct 8, 2019 at 8:15 AM Kent Gibson <warthog618@gmail.com> wrote: > Sorry for the late feedback, but I'm still not sure whether this patch > is on track to be applied or not. I had thought not, at least not yet, > but just in case... > > You have added the flags to linehandle_create, but not lineevent_create. > Given that your primary use case is push buttons, isn't the event request > at least as important as the handle request? Even ignoring your use > case, I'd add them to lineevent_create just to maintain symmetry. > > Also, is it valid to set PULL_UP and PULL_DOWN simulaneously? > I would think not, but I could be wrong. > If not then that combination should be rejected with EINVAL. Kent and Bart - thanks for the review. I've added setting the pull-up/down flags in lineevent_create(). And I have added sanity checks to make pull-up and pull-down flags mutually exclusive and only valid when the line is an input. I tried to use "git send-email" to reply to your message but it doesn't work seem to have threaded correctly, despite using "--in-reply-to" I have only submitted kernel patches before when I was completing the Eudyptula Challenge, so I wasn't sure how to best post a follow-up patch to address review feedback. Maybe starting a new thread with v2 of the RFC? thanks, drew
On Wed, Oct 09, 2019 at 02:55:24PM +0800, Kent Gibson wrote: > Safe to assume your code is in your topic/gpio-uapi-config branch? > I hope so, cos I've forked from that into github.com/warthog618/linux > and have started working on it. > > So far I've added pull up/down support to lineevent_create, and > added default_values to the gpiohandle_config (for setting outputs). > Currently looking at implementing the actual set config, and sucking > on whether that is best done by refactoring bit out of > linehandle_create. > > Will sort that into patches once it is in a fit state. > Feel free to poke me if I'm going off track. > > Cheers, > Kent. Great to see that work on this capability is moving foward. I also have spare time so I'm eager to help however I can. I had been working from the kernel.org torvalds/linux.git master to create a patch and used 'git send-email' to post a patch. It is interesting that Bartosz and you have forks of linux where you created topic branches. I just added warthog618/linux as a remote so I can view and try out your commits. thanks, drew
On Wed, Oct 09, 2019 at 02:57:27PM +0200, Drew Fustini wrote: > > On Wed, Oct 09, 2019 at 02:55:24PM +0800, Kent Gibson wrote: > > Safe to assume your code is in your topic/gpio-uapi-config branch? > > I hope so, cos I've forked from that into github.com/warthog618/linux > > and have started working on it. > > > > So far I've added pull up/down support to lineevent_create, and > > added default_values to the gpiohandle_config (for setting outputs). > > Currently looking at implementing the actual set config, and sucking > > on whether that is best done by refactoring bit out of > > linehandle_create. > > > > Will sort that into patches once it is in a fit state. > > Feel free to poke me if I'm going off track. > > > > Cheers, > > Kent. > > Great to see that work on this capability is moving foward. I also have > spare time so I'm eager to help however I can. > > I had been working from the kernel.org torvalds/linux.git master to > create a patch and used 'git send-email' to post a patch. It is > interesting that Bartosz and you have forks of linux where you created > topic branches. I just added warthog618/linux as a remote so I can view > and try out your commits. > Sorry that the branch is a munge of the pull up/down and set config changes. And I have no idea how I'm going to sort this into a reasonable patch sequence, but I'll burn that bridge when I get to it. I'm reasonably happy with the state of the pull up/down. That is passing all my tests on gpio-mockup - other than the actual value being pulled - but that is because gpio-mockup needs to be updated. Which is next on the todo list. Along with building a Pi kernel to test real hardware. I'd steer clear of the SET_CONFIG changes for the moment - those are totally untested. But I guess you don't have any code to call the ioctl yet anyway... That is slightly further down the todo list. Cheers, Kent.
On Wed, Oct 09, 2019 at 02:55:24PM +0800, Kent Gibson wrote: > Safe to assume your code is in your topic/gpio-uapi-config branch? > I hope so, cos I've forked from that into github.com/warthog618/linux > and have started working on it. Do you also have a fork of libgpiod that you are working in? In case it is of any use, I just posted the libgpiod patch for pull-up/down flags that I had been using to test with. I help maintain Adafruit_Blinka [1] so I would like try testing pull-up/down. I already have a Raspberry Pi 3 booting a cross-compiled kernel with my (now outdated) patch applied and a patched libgpiod. [1] https://github.com/adafruit/Adafruit_Blinka
On Wed, Oct 09, 2019 at 07:21:20AM +0800, Kent Gibson wrote: > And while we're talking, does the gpio-mockup support pull up/down > being set from the kernel side? I was curious where this refers to the kernel or libgpiod? I see there is: libgpiod/tests/mockup/gpio-mockup.c and: linux/drivers/gpio/gpio-mockup.c thanks, drew
On Wed, Oct 09, 2019 at 03:57:38PM +0200, Drew Fustini wrote: > On Wed, Oct 09, 2019 at 07:21:20AM +0800, Kent Gibson wrote: > > And while we're talking, does the gpio-mockup support pull up/down > > being set from the kernel side? > > I was curious where this refers to the kernel or libgpiod? > > I see there is: > libgpiod/tests/mockup/gpio-mockup.c > > and: > linux/drivers/gpio/gpio-mockup.c > The kernel one. I was referring to being able to change the pull on a line using the pull up/down flags. It can be set using debugfs, but the point here is to show that it can be set via the new flags as well. Cheers, Kent.
On Wed, Oct 09, 2019 at 03:30:37PM +0200, Drew Fustini wrote: > On Wed, Oct 09, 2019 at 02:55:24PM +0800, Kent Gibson wrote: > > Safe to assume your code is in your topic/gpio-uapi-config branch? > > I hope so, cos I've forked from that into github.com/warthog618/linux > > and have started working on it. > > Do you also have a fork of libgpiod that you are working in? > Actually not yet - I'm using my Go equivalent of libgpiod to drive the kernel for my tests. That is also on github - it is in the uapi directory of the feature/pud branch of my gpiod repo. > In case it is of any use, I just posted the libgpiod patch for pull-up/down > flags that I had been using to test with. > > I help maintain Adafruit_Blinka [1] so I would like try testing pull-up/down. > I already have a Raspberry Pi 3 booting a cross-compiled kernel with my (now > outdated) patch applied and a patched libgpiod. > Go nuts. I hope to be testing on one of Pis, but I don't think I have a recent kernel build handy, so I'll be a while. Cheers, Kent.
śr., 9 paź 2019 o 16:11 Kent Gibson <warthog618@gmail.com> napisał(a): > > On Wed, Oct 09, 2019 at 03:30:37PM +0200, Drew Fustini wrote: > > On Wed, Oct 09, 2019 at 02:55:24PM +0800, Kent Gibson wrote: > > > Safe to assume your code is in your topic/gpio-uapi-config branch? > > > I hope so, cos I've forked from that into github.com/warthog618/linux > > > and have started working on it. > > > > Do you also have a fork of libgpiod that you are working in? > > > Actually not yet - I'm using my Go equivalent of libgpiod to drive the > kernel for my tests. That is also on github - it is in the uapi > directory of the feature/pud branch of my gpiod repo. > > > In case it is of any use, I just posted the libgpiod patch for pull-up/down > > flags that I had been using to test with. > > > > I help maintain Adafruit_Blinka [1] so I would like try testing pull-up/down. > > I already have a Raspberry Pi 3 booting a cross-compiled kernel with my (now > > outdated) patch applied and a patched libgpiod. > > > Go nuts. I hope to be testing on one of Pis, but I don't think I have a > recent kernel build handy, so I'll be a while. > > Cheers, > Kent. Hey guys, just one thing: don't send patches as replies - just send a new version as a separate series. Also: please aggregate all the patches (pull-up/down, set config, gpio-mockup changes if any) into a single series. Bart
On Wed, Oct 09, 2019 at 05:50:00PM +0200, Bartosz Golaszewski wrote: > śr., 9 paź 2019 o 16:11 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > On Wed, Oct 09, 2019 at 03:30:37PM +0200, Drew Fustini wrote: > > > On Wed, Oct 09, 2019 at 02:55:24PM +0800, Kent Gibson wrote: > > > > Safe to assume your code is in your topic/gpio-uapi-config branch? > > > > I hope so, cos I've forked from that into github.com/warthog618/linux > > > > and have started working on it. > > > > > > Do you also have a fork of libgpiod that you are working in? > > > > > Actually not yet - I'm using my Go equivalent of libgpiod to drive the > > kernel for my tests. That is also on github - it is in the uapi > > directory of the feature/pud branch of my gpiod repo. > > > > > In case it is of any use, I just posted the libgpiod patch for pull-up/down > > > flags that I had been using to test with. > > > > > > I help maintain Adafruit_Blinka [1] so I would like try testing pull-up/down. > > > I already have a Raspberry Pi 3 booting a cross-compiled kernel with my (now > > > outdated) patch applied and a patched libgpiod. > > > > > Go nuts. I hope to be testing on one of Pis, but I don't think I have a > > recent kernel build handy, so I'll be a while. > > > > Cheers, > > Kent. > > Hey guys, just one thing: don't send patches as replies - just send a > new version as a separate series. Also: please aggregate all the > patches (pull-up/down, set config, gpio-mockup changes if any) into a > single series. > I think the only patch I've sent you was direct to you for feedback, not to the list, and so not intended to be applied. And that was only cos I thought you might prefer that to looking at the diffs on github. Would it be ok to create two patch series - one for pull up/down and the other for set config, if one still needs a lot more work? And is it ok to base them on your lineevent_create gotos tidy up (f6cfbbe2950b2)? Cheers, Kent.
On Wed, Oct 09, 2019 at 03:30:37PM +0200, Drew Fustini wrote: > On Wed, Oct 09, 2019 at 02:55:24PM +0800, Kent Gibson wrote: > > Safe to assume your code is in your topic/gpio-uapi-config branch? > > I hope so, cos I've forked from that into github.com/warthog618/linux > > and have started working on it. > > Do you also have a fork of libgpiod that you are working in? > > In case it is of any use, I just posted the libgpiod patch for pull-up/down > flags that I had been using to test with. > > I help maintain Adafruit_Blinka [1] so I would like try testing pull-up/down. > I already have a Raspberry Pi 3 booting a cross-compiled kernel with my (now > outdated) patch applied and a patched libgpiod. > It is basically working for me on my Pi4: pi@quoll:~ $ ./gpiodctl get gpiochip0 7 0 pi@quoll:~ $ ./gpiodctl get -u gpiochip0 7 1 pi@quoll:~ $ ./gpiodctl get gpiochip0 7 1 pi@quoll:~ $ ./gpiodctl get -d gpiochip0 7 0 pi@quoll:~ $ ./gpiodctl get gpiochip0 7 0 That is using the gpiodctl tool from my gpiod library. My gpiod test suite also passes, but it doesn't do much to exercise the UAPI. I was intending to run my uapi test suite, which is more thorough, but it turns out that only targets gpio-mockup, whereas my gpiod test suite can target either. Something else for the todo list. Hopefully it is obvious what gpiodctl is doing. (-u sets the pull-up flag, -d sets the pull-down flag) Looks like the pulls stick when the line is released, and the subsequent get, without pull-up set, either doesn't clear the pull-up/down or the line stays floating at the old pull level. More investigation required, but that will have to wait til I get back to this later in the day. Oh, and that is running on the rpi-5.3.3 kernel patched with everything on my topic/gpio-uapi-config branch from 5.4-rc2 onward. Cheers, Kent.
On Thu, Oct 10, 2019 at 1:59 AM Kent Gibson <warthog618@gmail.com> wrote: > It is basically working for me on my Pi4: > > pi@quoll:~ $ ./gpiodctl get gpiochip0 7 > 0 > pi@quoll:~ $ ./gpiodctl get -u gpiochip0 7 > 1 > pi@quoll:~ $ ./gpiodctl get gpiochip0 7 > 1 > pi@quoll:~ $ ./gpiodctl get -d gpiochip0 7 > 0 > pi@quoll:~ $ ./gpiodctl get gpiochip0 7 > 0 > > That is using the gpiodctl tool from my gpiod library. > My gpiod test suite also passes, but it doesn't do much to > exercise the UAPI. > I was intending to run my uapi test suite, which is more thorough, > but it turns out that only targets gpio-mockup, whereas my gpiod > test suite can target either. > Something else for the todo list. > > Hopefully it is obvious what gpiodctl is doing. (-u sets the > pull-up flag, -d sets the pull-down flag) > Looks like the pulls stick when the line is released, and the > subsequent get, without pull-up set, either doesn't clear the > pull-up/down or the line stays floating at the old pull level. > More investigation required, but that will have to wait til > I get back to this later in the day. > > Oh, and that is running on the rpi-5.3.3 kernel patched with everything > on my topic/gpio-uapi-config branch from 5.4-rc2 onward. Thanks for sharing your results. My Pi 3 had been running 5.3.0-v7+ from September 20th with my pull-up/down patch (82fc38f6ab59). I removed that patch and just cross-compiled 5.4-rc2 with multi_v7_defconfig for the Pi3. Are these the commits that I should apply from your topic branch? bdc9696a27ed pull up/down requires explicit input mode in lineevent_create 14ee636232d4 disallow pull up/down on outputs ce03bf5af1ec implement SET_CONFIG_IOCTL f38b7554eb52 pull common validation code into linehandle_validate_flags 31c0aa53ffc3 Add default values for setting output 3c7ec03efcd9 add support for pull up/down to lineevent_create 99b85d1c26ea gpio: add new ioctl() to gpio chardev 82fc38f6ab59 gpio: expose pull-up/pull-down line flags to userspace f6cfbbe2950b gpiolib: sanitize flags before allocating memory in lineevent_create() Thanks, Drew
On Thu, Oct 10, 2019 at 09:47:35AM +0200, Drew Fustini wrote: > On Thu, Oct 10, 2019 at 1:59 AM Kent Gibson <warthog618@gmail.com> wrote: > > It is basically working for me on my Pi4: > > > > pi@quoll:~ $ ./gpiodctl get gpiochip0 7 > > 0 > > pi@quoll:~ $ ./gpiodctl get -u gpiochip0 7 > > 1 > > pi@quoll:~ $ ./gpiodctl get gpiochip0 7 > > 1 > > pi@quoll:~ $ ./gpiodctl get -d gpiochip0 7 > > 0 > > pi@quoll:~ $ ./gpiodctl get gpiochip0 7 > > 0 > > > > That is using the gpiodctl tool from my gpiod library. > > My gpiod test suite also passes, but it doesn't do much to > > exercise the UAPI. > > I was intending to run my uapi test suite, which is more thorough, > > but it turns out that only targets gpio-mockup, whereas my gpiod > > test suite can target either. > > Something else for the todo list. > > > > Hopefully it is obvious what gpiodctl is doing. (-u sets the > > pull-up flag, -d sets the pull-down flag) > > Looks like the pulls stick when the line is released, and the > > subsequent get, without pull-up set, either doesn't clear the > > pull-up/down or the line stays floating at the old pull level. > > More investigation required, but that will have to wait til > > I get back to this later in the day. > > > > Oh, and that is running on the rpi-5.3.3 kernel patched with everything > > on my topic/gpio-uapi-config branch from 5.4-rc2 onward. > > Thanks for sharing your results. > > My Pi 3 had been running 5.3.0-v7+ from September 20th with my > pull-up/down patch (82fc38f6ab59). > > I removed that patch and just cross-compiled 5.4-rc2 with > multi_v7_defconfig for the Pi3. > > Are these the commits that I should apply from your topic branch? > > bdc9696a27ed pull up/down requires explicit input mode in lineevent_create > 14ee636232d4 disallow pull up/down on outputs > ce03bf5af1ec implement SET_CONFIG_IOCTL > f38b7554eb52 pull common validation code into linehandle_validate_flags > 31c0aa53ffc3 Add default values for setting output > 3c7ec03efcd9 add support for pull up/down to lineevent_create > 99b85d1c26ea gpio: add new ioctl() to gpio chardev > 82fc38f6ab59 gpio: expose pull-up/pull-down line flags to userspace > f6cfbbe2950b gpiolib: sanitize flags before allocating memory in > lineevent_create() > Those are the ones. Plus there are now another 3: 625cd0a0df3ad9 actively disable bias on outputs 9d1f9db81b4dc4 actively disable bias on inputs when pull up/down not set c6d4bf32c05189 add set_config to support pull up/down Those add pull up/down support to gpio-mockup, and fix the stuck pulls I noted earlier - though they still remain applied until the next time the line is requested. 9d1f9db81b4dc4 doesn't like being applied to the rpi kernel, I assume as there have been conflicting changes between 5.3.3 and 5.4-rc2, so I patched that one manually. Cheers, Kent.
On Thu, Oct 10, 2019 at 06:14:21PM +0800, Kent Gibson wrote: > On Thu, Oct 10, 2019 at 09:47:35AM +0200, Drew Fustini wrote: > > On Thu, Oct 10, 2019 at 1:59 AM Kent Gibson <warthog618@gmail.com> wrote: > > > It is basically working for me on my Pi4: > > > > > > pi@quoll:~ $ ./gpiodctl get gpiochip0 7 > > > 0 > > > pi@quoll:~ $ ./gpiodctl get -u gpiochip0 7 > > > 1 > > > pi@quoll:~ $ ./gpiodctl get gpiochip0 7 > > > 1 > > > pi@quoll:~ $ ./gpiodctl get -d gpiochip0 7 > > > 0 > > > pi@quoll:~ $ ./gpiodctl get gpiochip0 7 > > > 0 > > > > > > That is using the gpiodctl tool from my gpiod library. > > > My gpiod test suite also passes, but it doesn't do much to > > > exercise the UAPI. > > > I was intending to run my uapi test suite, which is more thorough, > > > but it turns out that only targets gpio-mockup, whereas my gpiod > > > test suite can target either. > > > Something else for the todo list. > > > > > > Hopefully it is obvious what gpiodctl is doing. (-u sets the > > > pull-up flag, -d sets the pull-down flag) > > > Looks like the pulls stick when the line is released, and the > > > subsequent get, without pull-up set, either doesn't clear the > > > pull-up/down or the line stays floating at the old pull level. > > > More investigation required, but that will have to wait til > > > I get back to this later in the day. > > > > > > Oh, and that is running on the rpi-5.3.3 kernel patched with everything > > > on my topic/gpio-uapi-config branch from 5.4-rc2 onward. > > > > Thanks for sharing your results. > > > > My Pi 3 had been running 5.3.0-v7+ from September 20th with my > > pull-up/down patch (82fc38f6ab59). > > > > I removed that patch and just cross-compiled 5.4-rc2 with > > multi_v7_defconfig for the Pi3. > > > > Are these the commits that I should apply from your topic branch? > > > > bdc9696a27ed pull up/down requires explicit input mode in lineevent_create > > 14ee636232d4 disallow pull up/down on outputs > > ce03bf5af1ec implement SET_CONFIG_IOCTL > > f38b7554eb52 pull common validation code into linehandle_validate_flags > > 31c0aa53ffc3 Add default values for setting output > > 3c7ec03efcd9 add support for pull up/down to lineevent_create > > 99b85d1c26ea gpio: add new ioctl() to gpio chardev > > 82fc38f6ab59 gpio: expose pull-up/pull-down line flags to userspace > > f6cfbbe2950b gpiolib: sanitize flags before allocating memory in > > lineevent_create() > > > Those are the ones. > > Plus there are now another 3: > 625cd0a0df3ad9 actively disable bias on outputs > 9d1f9db81b4dc4 actively disable bias on inputs when pull up/down not set > c6d4bf32c05189 add set_config to support pull up/down > > Those add pull up/down support to gpio-mockup, and fix the stuck pulls > I noted earlier - though they still remain applied until the next time > the line is requested. > > 9d1f9db81b4dc4 doesn't like being applied to the rpi kernel, I assume as > there have been conflicting changes between 5.3.3 and 5.4-rc2, so I > patched that one manually. > The disable bias changes I mentioned above are problematic - they break ABI backward compatibility by explicitly disabling bias when neither PULL_UP nor PULL_DOWN are set - which will be the case for legacy apps. We really need to handle 4 possible bias states - None, Pull Up, Pull Down and As Is, with the zero default being As Is. So I intend to reinterpret the pull up and down flags as a two bit field encoding the following: 0 - AsIs 1 - PullUp 2 - PullDown 3 - PullNone Any objections? Cheers, Kent.
Hi Kent, that makes sense to me. Hopefully, Bartosz and/or Linus will advise if this is an issue. thanks, drew On Thu, Oct 10, 2019 at 1:17 PM Kent Gibson <warthog618@gmail.com> wrote: > > On Thu, Oct 10, 2019 at 06:14:21PM +0800, Kent Gibson wrote: > > On Thu, Oct 10, 2019 at 09:47:35AM +0200, Drew Fustini wrote: > > > On Thu, Oct 10, 2019 at 1:59 AM Kent Gibson <warthog618@gmail.com> wrote: > > > > It is basically working for me on my Pi4: > > > > > > > > pi@quoll:~ $ ./gpiodctl get gpiochip0 7 > > > > 0 > > > > pi@quoll:~ $ ./gpiodctl get -u gpiochip0 7 > > > > 1 > > > > pi@quoll:~ $ ./gpiodctl get gpiochip0 7 > > > > 1 > > > > pi@quoll:~ $ ./gpiodctl get -d gpiochip0 7 > > > > 0 > > > > pi@quoll:~ $ ./gpiodctl get gpiochip0 7 > > > > 0 > > > > > > > > That is using the gpiodctl tool from my gpiod library. > > > > My gpiod test suite also passes, but it doesn't do much to > > > > exercise the UAPI. > > > > I was intending to run my uapi test suite, which is more thorough, > > > > but it turns out that only targets gpio-mockup, whereas my gpiod > > > > test suite can target either. > > > > Something else for the todo list. > > > > > > > > Hopefully it is obvious what gpiodctl is doing. (-u sets the > > > > pull-up flag, -d sets the pull-down flag) > > > > Looks like the pulls stick when the line is released, and the > > > > subsequent get, without pull-up set, either doesn't clear the > > > > pull-up/down or the line stays floating at the old pull level. > > > > More investigation required, but that will have to wait til > > > > I get back to this later in the day. > > > > > > > > Oh, and that is running on the rpi-5.3.3 kernel patched with everything > > > > on my topic/gpio-uapi-config branch from 5.4-rc2 onward. > > > > > > Thanks for sharing your results. > > > > > > My Pi 3 had been running 5.3.0-v7+ from September 20th with my > > > pull-up/down patch (82fc38f6ab59). > > > > > > I removed that patch and just cross-compiled 5.4-rc2 with > > > multi_v7_defconfig for the Pi3. > > > > > > Are these the commits that I should apply from your topic branch? > > > > > > bdc9696a27ed pull up/down requires explicit input mode in lineevent_create > > > 14ee636232d4 disallow pull up/down on outputs > > > ce03bf5af1ec implement SET_CONFIG_IOCTL > > > f38b7554eb52 pull common validation code into linehandle_validate_flags > > > 31c0aa53ffc3 Add default values for setting output > > > 3c7ec03efcd9 add support for pull up/down to lineevent_create > > > 99b85d1c26ea gpio: add new ioctl() to gpio chardev > > > 82fc38f6ab59 gpio: expose pull-up/pull-down line flags to userspace > > > f6cfbbe2950b gpiolib: sanitize flags before allocating memory in > > > lineevent_create() > > > > > Those are the ones. > > > > Plus there are now another 3: > > 625cd0a0df3ad9 actively disable bias on outputs > > 9d1f9db81b4dc4 actively disable bias on inputs when pull up/down not set > > c6d4bf32c05189 add set_config to support pull up/down > > > > Those add pull up/down support to gpio-mockup, and fix the stuck pulls > > I noted earlier - though they still remain applied until the next time > > the line is requested. > > > > 9d1f9db81b4dc4 doesn't like being applied to the rpi kernel, I assume as > > there have been conflicting changes between 5.3.3 and 5.4-rc2, so I > > patched that one manually. > > > The disable bias changes I mentioned above are problematic - they break > ABI backward compatibility by explicitly disabling bias when neither > PULL_UP nor PULL_DOWN are set - which will be the case for legacy apps. > > We really need to handle 4 possible bias states - None, Pull Up, Pull > Down and As Is, with the zero default being As Is. > So I intend to reinterpret the pull up and down flags as a two bit field > encoding the following: > > 0 - AsIs > 1 - PullUp > 2 - PullDown > 3 - PullNone > > Any objections? > > Cheers, > Kent.
Hi Kent, I noticed there are some additional commits in your branch including: 7f0bcb88f583 pull up/down on output is actually a valid option Do you know what the result would be when the line is an output? I seem to recall some chips might do High-Z when some pull-up/down is set and direction is out. thanks, drew On Thu, Oct 10, 2019 at 12:14 PM Kent Gibson <warthog618@gmail.com> wrote: > > On Thu, Oct 10, 2019 at 09:47:35AM +0200, Drew Fustini wrote: > > On Thu, Oct 10, 2019 at 1:59 AM Kent Gibson <warthog618@gmail.com> wrote: > > > It is basically working for me on my Pi4: > > > > > > pi@quoll:~ $ ./gpiodctl get gpiochip0 7 > > > 0 > > > pi@quoll:~ $ ./gpiodctl get -u gpiochip0 7 > > > 1 > > > pi@quoll:~ $ ./gpiodctl get gpiochip0 7 > > > 1 > > > pi@quoll:~ $ ./gpiodctl get -d gpiochip0 7 > > > 0 > > > pi@quoll:~ $ ./gpiodctl get gpiochip0 7 > > > 0 > > > > > > That is using the gpiodctl tool from my gpiod library. > > > My gpiod test suite also passes, but it doesn't do much to > > > exercise the UAPI. > > > I was intending to run my uapi test suite, which is more thorough, > > > but it turns out that only targets gpio-mockup, whereas my gpiod > > > test suite can target either. > > > Something else for the todo list. > > > > > > Hopefully it is obvious what gpiodctl is doing. (-u sets the > > > pull-up flag, -d sets the pull-down flag) > > > Looks like the pulls stick when the line is released, and the > > > subsequent get, without pull-up set, either doesn't clear the > > > pull-up/down or the line stays floating at the old pull level. > > > More investigation required, but that will have to wait til > > > I get back to this later in the day. > > > > > > Oh, and that is running on the rpi-5.3.3 kernel patched with everything > > > on my topic/gpio-uapi-config branch from 5.4-rc2 onward. > > > > Thanks for sharing your results. > > > > My Pi 3 had been running 5.3.0-v7+ from September 20th with my > > pull-up/down patch (82fc38f6ab59). > > > > I removed that patch and just cross-compiled 5.4-rc2 with > > multi_v7_defconfig for the Pi3. > > > > Are these the commits that I should apply from your topic branch? > > > > bdc9696a27ed pull up/down requires explicit input mode in lineevent_create > > 14ee636232d4 disallow pull up/down on outputs > > ce03bf5af1ec implement SET_CONFIG_IOCTL > > f38b7554eb52 pull common validation code into linehandle_validate_flags > > 31c0aa53ffc3 Add default values for setting output > > 3c7ec03efcd9 add support for pull up/down to lineevent_create > > 99b85d1c26ea gpio: add new ioctl() to gpio chardev > > 82fc38f6ab59 gpio: expose pull-up/pull-down line flags to userspace > > f6cfbbe2950b gpiolib: sanitize flags before allocating memory in > > lineevent_create() > > > Those are the ones. > > Plus there are now another 3: > 625cd0a0df3ad9 actively disable bias on outputs > 9d1f9db81b4dc4 actively disable bias on inputs when pull up/down not set > c6d4bf32c05189 add set_config to support pull up/down > > Those add pull up/down support to gpio-mockup, and fix the stuck pulls > I noted earlier - though they still remain applied until the next time > the line is requested. > > 9d1f9db81b4dc4 doesn't like being applied to the rpi kernel, I assume as > there have been conflicting changes between 5.3.3 and 5.4-rc2, so I > patched that one manually. > > Cheers, > Kent.
On Fri, Oct 11, 2019 at 03:06:41PM +0200, Drew Fustini wrote: > Hi Kent, > > I noticed there are some additional commits in your branch including: > 7f0bcb88f583 pull up/down on output is actually a valid option > Yeah pulls make no sense for push pull lines, but while looking into disabling bias I discovered that there are some chips that support pull-up/down for open drain/source outputs. It seemed reasonable to support them rather than reject them outright, hence the reversal. > Do you know what the result would be when the line is an output? > Behaviour would depend on the driver and chip. I'm assuming the user would only request combinations that make sense and that the driver would reject any combinations that might have unfortunate results. I'm working on improving my git-fu to collect the pull changes from that branch, and hope to mail out a patch set shortly. Cheers, Kent.
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index d9074191edef..9da1093cc7f5 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -427,6 +427,8 @@ struct linehandle_state { (GPIOHANDLE_REQUEST_INPUT | \ GPIOHANDLE_REQUEST_OUTPUT | \ GPIOHANDLE_REQUEST_ACTIVE_LOW | \ + GPIOHANDLE_REQUEST_PULL_UP | \ + GPIOHANDLE_REQUEST_PULL_DOWN | \ GPIOHANDLE_REQUEST_OPEN_DRAIN | \ GPIOHANDLE_REQUEST_OPEN_SOURCE) @@ -598,6 +600,10 @@ static int linehandle_create(struct gpio_device *gdev, void __user *ip) set_bit(FLAG_OPEN_DRAIN, &desc->flags); if (lflags & GPIOHANDLE_REQUEST_OPEN_SOURCE) set_bit(FLAG_OPEN_SOURCE, &desc->flags); + if (lflags & GPIOHANDLE_REQUEST_PULL_DOWN) + set_bit(FLAG_PULL_DOWN, &desc->flags); + if (lflags & GPIOHANDLE_REQUEST_PULL_UP) + set_bit(FLAG_PULL_UP, &desc->flags); ret = gpiod_set_transitory(desc, false); if (ret < 0) @@ -1102,6 +1108,10 @@ static long gpio_ioctl(struct file *filp, unsigned int cmd, unsigned long arg) if (test_bit(FLAG_OPEN_SOURCE, &desc->flags)) lineinfo.flags |= (GPIOLINE_FLAG_OPEN_SOURCE | GPIOLINE_FLAG_IS_OUT); + if (test_bit(FLAG_PULL_DOWN, &desc->flags)) + lineinfo.flags |= GPIOLINE_FLAG_PULL_DOWN; + if (test_bit(FLAG_PULL_UP, &desc->flags)) + lineinfo.flags |= GPIOLINE_FLAG_PULL_UP; if (copy_to_user(ip, &lineinfo, sizeof(lineinfo))) return -EFAULT; @@ -2475,6 +2485,8 @@ static bool gpiod_free_commit(struct gpio_desc *desc) clear_bit(FLAG_REQUESTED, &desc->flags); clear_bit(FLAG_OPEN_DRAIN, &desc->flags); clear_bit(FLAG_OPEN_SOURCE, &desc->flags); + clear_bit(FLAG_PULL_UP, &desc->flags); + clear_bit(FLAG_PULL_DOWN, &desc->flags); clear_bit(FLAG_IS_HOGGED, &desc->flags); ret = true; } diff --git a/include/uapi/linux/gpio.h b/include/uapi/linux/gpio.h index 4ebfe0ac6c5b..c2d1f7d908d6 100644 --- a/include/uapi/linux/gpio.h +++ b/include/uapi/linux/gpio.h @@ -33,6 +33,8 @@ struct gpiochip_info { #define GPIOLINE_FLAG_ACTIVE_LOW (1UL << 2) #define GPIOLINE_FLAG_OPEN_DRAIN (1UL << 3) #define GPIOLINE_FLAG_OPEN_SOURCE (1UL << 4) +#define GPIOLINE_FLAG_PULL_UP (1UL << 5) +#define GPIOLINE_FLAG_PULL_DOWN (1UL << 6) /** * struct gpioline_info - Information about a certain GPIO line @@ -62,6 +64,8 @@ struct gpioline_info { #define GPIOHANDLE_REQUEST_ACTIVE_LOW (1UL << 2) #define GPIOHANDLE_REQUEST_OPEN_DRAIN (1UL << 3) #define GPIOHANDLE_REQUEST_OPEN_SOURCE (1UL << 4) +#define GPIOHANDLE_REQUEST_PULL_UP (1UL << 5) +#define GPIOHANDLE_REQUEST_PULL_DOWN (1UL << 6) /** * struct gpiohandle_request - Information about a GPIO handle request
Add pull-up/pull-down flags to the gpio line get and set ioctl() calls. Use cases include a push button that does not have an external resistor. Addition use cases described by Limor Fried (ladyada) of Adafruit in this PR for Adafruit_Blinka Python lib: https://github.com/adafruit/Adafruit_Blinka/pull/59 Signed-off-by: Drew Fustini <drew@pdp7.com> --- drivers/gpio/gpiolib.c | 12 ++++++++++++ include/uapi/linux/gpio.h | 4 ++++ 2 files changed, 16 insertions(+)