| Message ID | 20191105020429.18942-1-warthog618@gmail.com |
|---|---|
| Headers | show |
| Series | gpio: expose line bias flags to userspace | expand |
wt., 5 lis 2019 o 03:04 Kent Gibson <warthog618@gmail.com> napisał(a): > > The changes from v5: > - rebased onto Bart's gpio/for-kent branch. > > The changes from v4: > Review changes: > - relocate restriction on applying bias to as-is from patch 2 to patch 1. > - propagate errors, other than ENOTSUPP, from gpio_set_bias. (squashed > into patches 3 and 4). > - include SET_CONFIG patch series v2 (patch 6 and 7 here). > > I've also fixed a few other nits I noticed along the way: > - rework gpio_set_bias as flags are mutually exclusive. > - remove input flag required to set bias restriction from > lineevent_create as events are implicitly assumed inputs anyway. > - reorder patches to group gpiolib bias patches together before the > gpio-mockup changes. > > > This series adds gross control of pull-up/pull-down to the GPIO uAPI. > Gross control means enabling and disabling of bias functionality, > not finer grained control such as setting biasing impedances. > > The support allows both input and output lines to have any one of the > following biases applied as part of the line handle or event request: > 0. As Is - bias is left alone. This is the default for ABI compatibility. > 1. Bias Disable - bias is explicitly disabled. > 2. Pull Down - pull-down bias is enabled. > 3. Pull Up - pull-up bias is enabled. > > The biases are set via three flags, BIAS_DISABLE, BIAS_PULL_DOWN > and BIAS_PULL_UP. These map directly to the similarly named > pinctrl pin_config_param flags. > As Is corresponds to none of the flags being set. > > The setting of biases on output lines may seem odd, but is to allow for > utilisation of internal pull-up/pull-down on open drain and open source > outputs, where supported in hardware. > > The series also adds the GPIOHANDLE_SET_CONFIG_IOCTL to the gpio chardev. > The ioctl allows some of the configuration of a requested handle to be > changed without having to release the line. > The primary use case is the changing of direction for bi-directional > lines. > > Patches are against Bart's gpio/for-kent branch[1]. > > The patch has been successfully tested against gpio-mockup, and > on a Raspberry Pi, in both cases using the feature/pud_set_config > branch of my Go gpiod library[2], as well as with my feature/pud > development branch of libgpiod[3]. Patch 7 has only been tested using > my gpiod library as libgpiod has not yet been updated to support the > SET_CONFIG ioctl. > > Patch 1 adds pull-up/pull-down support to line handle requests. > Patch 2 adds pull-up/pull-down support to line event requests. > Patch 3 adds support for disabling bias. > Patch 4 adds support for setting bias on output lines. > Patch 5 adds pull-up/down support to the gpio-mockup for uAPI testing. > Patch 6 refactors the flag validation from linehandle_create. > Patch 7 adds the SET_CONFIG ioctl. > > Drew Fustini (1): > gpio: expose pull-up/pull-down line flags to userspace > > Kent Gibson (6): > gpiolib: add support for pull up/down to lineevent_create > gpiolib: add support for disabling line bias > gpiolib: add support for biasing output lines > gpio: mockup: add set_config to support pull up/down > gpiolib: move validation of line handle flags into helper function > gpio: add new SET_CONFIG ioctl() to gpio chardev > > drivers/gpio/gpio-mockup.c | 94 ++++++++++------ > drivers/gpio/gpiolib.c | 213 +++++++++++++++++++++++++++++++------ > drivers/gpio/gpiolib.h | 1 + > include/uapi/linux/gpio.h | 24 +++++ > 4 files changed, 264 insertions(+), 68 deletions(-) > > -- > 2.23.0 > Ok, all looks good now and since Linus already said he's fine with this series, I'll take it through my tree for v5.5. Thanks! Bartosz
On Tue, Nov 05, 2019 at 10:04:22AM +0800, Kent Gibson wrote: > Patches are against Bart's gpio/for-kent branch[1]. > > The patch has been successfully tested against gpio-mockup, and > on a Raspberry Pi, in both cases using the feature/pud_set_config > branch of my Go gpiod library[2], as well as with my feature/pud > development branch of libgpiod[3]. Patch 7 has only been tested using > my gpiod library as libgpiod has not yet been updated to support the > SET_CONFIG ioctl. > I've just pushed a first pass at SET_CONFIG support into my libgpiod feature/pud branch. It is causing me a bit of grief. Due to the way the libgpiod API is structured, with the direction flags pulled out into the request type, I thought it would be cleaner to keep changes to direction orthogonal to changes to the other handle flags. So I've added these methods to the API: int gpiod_line_set_config(struct gpiod_line *line, int flags) int gpiod_line_set_direction_input(struct gpiod_line *line) int gpiod_line_set_direction_output(struct gpiod_line *line, int value) along with their bulk equivalents. I've coded that and started adding tests when I tripped over changing bias. The kernel requires a direction to be set, but I'm setting it as-is in gpiod_line_set_config - so that wont work. Open drain/source are in the same boat - they require output mode. I see these options: 1. set the direction as part of gpiod_line_set_config 2. relax the kernel restriction. 3. don't support changing bias or open source/drain. 4. rethink the API. The first option requires caching the value set for outputs which I'm a bit hesitant to do, though I'm not sure why - I've already added caching of the handle flags for the direction functions. Any preferences or suggestions? Cheers, Kent.
wt., 5 lis 2019 o 16:26 Kent Gibson <warthog618@gmail.com> napisał(a): > > On Tue, Nov 05, 2019 at 10:04:22AM +0800, Kent Gibson wrote: > > Patches are against Bart's gpio/for-kent branch[1]. > > > > The patch has been successfully tested against gpio-mockup, and > > on a Raspberry Pi, in both cases using the feature/pud_set_config > > branch of my Go gpiod library[2], as well as with my feature/pud > > development branch of libgpiod[3]. Patch 7 has only been tested using > > my gpiod library as libgpiod has not yet been updated to support the > > SET_CONFIG ioctl. > > > > I've just pushed a first pass at SET_CONFIG support into my libgpiod > feature/pud branch. It is causing me a bit of grief. Due to the way > the libgpiod API is structured, with the direction flags pulled out into > the request type, I thought it would be cleaner to keep changes to direction > orthogonal to changes to the other handle flags. > I'd love to see that branch - is it public? > So I've added these methods to the API: > > int gpiod_line_set_config(struct gpiod_line *line, int flags) > int gpiod_line_set_direction_input(struct gpiod_line *line) > int gpiod_line_set_direction_output(struct gpiod_line *line, > int value) > > along with their bulk equivalents. > > I've coded that and started adding tests when I tripped over changing > bias. The kernel requires a direction to be set, but I'm setting it > as-is in gpiod_line_set_config - so that wont work. > Open drain/source are in the same boat - they require output mode. > Ha! Yes this is a problem - how about this: If the caller of set_config in the kernel doesn't pass any of the direction flags, then we read the current direction, set the right flag in lflags and only then call the function validating the flags? > I see these options: > 1. set the direction as part of gpiod_line_set_config > 2. relax the kernel restriction. Yes, I don't think we should force users to always pass the direction flag in set_config. Good point. > 3. don't support changing bias or open source/drain. No! We definitely want to support it in libgpiod. > 4. rethink the API. As for libgpiod: I think we should have a low-level gpiod_line_set_config() that would set both the direction and other flags (it could for instance only accept two request flags: input and output) and then a higher-level set_flags(), set_direction_input(), set_direction_output() that would call the low-level variant and - for set_flags() without the direction argument - it could simply retrieve the current direction and pass it to gpiod_line_set_config(). But this is only a vague idea - I'd have to actually start looking at the code to be sure. I'd love to see what you came up with so far though! Bart > > The first option requires caching the value set for outputs which I'm a > bit hesitant to do, though I'm not sure why - I've already added caching > of the handle flags for the direction functions. > > Any preferences or suggestions? > > Cheers, > Kent.
wt., 5 lis 2019 o 17:24 Bartosz Golaszewski <bgolaszewski@baylibre.com> napisał(a): > > wt., 5 lis 2019 o 16:26 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > On Tue, Nov 05, 2019 at 10:04:22AM +0800, Kent Gibson wrote: > > > Patches are against Bart's gpio/for-kent branch[1]. > > > > > > The patch has been successfully tested against gpio-mockup, and > > > on a Raspberry Pi, in both cases using the feature/pud_set_config > > > branch of my Go gpiod library[2], as well as with my feature/pud > > > development branch of libgpiod[3]. Patch 7 has only been tested using > > > my gpiod library as libgpiod has not yet been updated to support the > > > SET_CONFIG ioctl. > > > > > > > I've just pushed a first pass at SET_CONFIG support into my libgpiod > > feature/pud branch. It is causing me a bit of grief. Due to the way > > the libgpiod API is structured, with the direction flags pulled out into > > the request type, I thought it would be cleaner to keep changes to direction > > orthogonal to changes to the other handle flags. > > > > I'd love to see that branch - is it public? > > > So I've added these methods to the API: > > > > int gpiod_line_set_config(struct gpiod_line *line, int flags) > > int gpiod_line_set_direction_input(struct gpiod_line *line) > > int gpiod_line_set_direction_output(struct gpiod_line *line, > > int value) > > > > along with their bulk equivalents. > > > > I've coded that and started adding tests when I tripped over changing > > bias. The kernel requires a direction to be set, but I'm setting it > > as-is in gpiod_line_set_config - so that wont work. > > Open drain/source are in the same boat - they require output mode. > > > > Ha! Yes this is a problem - how about this: > > If the caller of set_config in the kernel doesn't pass any of the > direction flags, then we read the current direction, set the right > flag in lflags and only then call the function validating the flags? > After another thought: this would be a bit inconsistent with the rest of the flags. IIRC this was the reason for me to split the request_type and other flags into two separate fields in libgpiod in the first place. When I think about it: the kernel behavior should be as predictable as possible - if we keep the behavior as is in v6, I don't see why we couldn't make userspace cache (or re-read) the current direction when setting flags other than direction? Do you see any trouble in that? That way we'd avoid having the kernel treat different flags in different way. Bart > > I see these options: > > 1. set the direction as part of gpiod_line_set_config > > 2. relax the kernel restriction. > > Yes, I don't think we should force users to always pass the direction > flag in set_config. Good point. > > > 3. don't support changing bias or open source/drain. > > No! We definitely want to support it in libgpiod. > > > 4. rethink the API. > > As for libgpiod: I think we should have a low-level > gpiod_line_set_config() that would set both the direction and other > flags (it could for instance only accept two request flags: input and > output) and then a higher-level set_flags(), set_direction_input(), > set_direction_output() that would call the low-level variant and - for > set_flags() without the direction argument - it could simply retrieve > the current direction and pass it to gpiod_line_set_config(). > > But this is only a vague idea - I'd have to actually start looking at > the code to be sure. I'd love to see what you came up with so far > though! > > Bart > > > > > The first option requires caching the value set for outputs which I'm a > > bit hesitant to do, though I'm not sure why - I've already added caching > > of the handle flags for the direction functions. > > > > Any preferences or suggestions? > > > > Cheers, > > Kent.
On Tue, Nov 05, 2019 at 05:24:44PM +0100, Bartosz Golaszewski wrote: > wt., 5 lis 2019 o 16:26 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > On Tue, Nov 05, 2019 at 10:04:22AM +0800, Kent Gibson wrote: > > > Patches are against Bart's gpio/for-kent branch[1]. > > > > > > The patch has been successfully tested against gpio-mockup, and > > > on a Raspberry Pi, in both cases using the feature/pud_set_config > > > branch of my Go gpiod library[2], as well as with my feature/pud > > > development branch of libgpiod[3]. Patch 7 has only been tested using > > > my gpiod library as libgpiod has not yet been updated to support the > > > SET_CONFIG ioctl. > > > > > > > I've just pushed a first pass at SET_CONFIG support into my libgpiod > > feature/pud branch. It is causing me a bit of grief. Due to the way > > the libgpiod API is structured, with the direction flags pulled out into > > the request type, I thought it would be cleaner to keep changes to direction > > orthogonal to changes to the other handle flags. > > > > I'd love to see that branch - is it public? > Sorry - I forgot to add the references to the bottom of the cover note for v5 and v6. It should've been: [1] git://git.kernel.org/pub/scm/linux/kernel/git/brgl/linux.git [2] https://github.com/warthog618/gpiod.git [3] https://github.com/warthog618/libgpiod.git It is my libgpiod repo on github[3] - in the feature/pud branch. Cheers, Kent.
On Tue, Nov 05, 2019 at 10:07:58PM +0100, Bartosz Golaszewski wrote: > wt., 5 lis 2019 o 17:24 Bartosz Golaszewski > <bgolaszewski@baylibre.com> napisał(a): > > > > wt., 5 lis 2019 o 16:26 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > > > On Tue, Nov 05, 2019 at 10:04:22AM +0800, Kent Gibson wrote: > > > > Patches are against Bart's gpio/for-kent branch[1]. > > > > > > > > The patch has been successfully tested against gpio-mockup, and > > > > on a Raspberry Pi, in both cases using the feature/pud_set_config > > > > branch of my Go gpiod library[2], as well as with my feature/pud > > > > development branch of libgpiod[3]. Patch 7 has only been tested using > > > > my gpiod library as libgpiod has not yet been updated to support the > > > > SET_CONFIG ioctl. > > > > > > > > > > I've just pushed a first pass at SET_CONFIG support into my libgpiod > > > feature/pud branch. It is causing me a bit of grief. Due to the way > > > the libgpiod API is structured, with the direction flags pulled out into > > > the request type, I thought it would be cleaner to keep changes to direction > > > orthogonal to changes to the other handle flags. > > > > > > > I'd love to see that branch - is it public? > > > > > So I've added these methods to the API: > > > > > > int gpiod_line_set_config(struct gpiod_line *line, int flags) > > > int gpiod_line_set_direction_input(struct gpiod_line *line) > > > int gpiod_line_set_direction_output(struct gpiod_line *line, > > > int value) > > > > > > along with their bulk equivalents. > > > > > > I've coded that and started adding tests when I tripped over changing > > > bias. The kernel requires a direction to be set, but I'm setting it > > > as-is in gpiod_line_set_config - so that wont work. > > > Open drain/source are in the same boat - they require output mode. > > > > > > > Ha! Yes this is a problem - how about this: > > > > If the caller of set_config in the kernel doesn't pass any of the > > direction flags, then we read the current direction, set the right > > flag in lflags and only then call the function validating the flags? > > > I was also thinking along that line - the config would be carried over from the initial request and any subsequent SET_CONFIGs. The kernel could overlay the existing config over any field set "as-is" before performing validation. The validation requirement would stand, but you don't need to pass the complete state, possibly including output values, with each SET_CONFIG request. In the bias case above, I create the line as an output and so should be able to set the bias, even if neither INPUT nor OUTPUT is set in the SET_CONFIG request. > After another thought: this would be a bit inconsistent with the rest > of the flags. IIRC this was the reason for me to split the > request_type and other flags into two separate fields in libgpiod in > the first place. > It is a bit inconsisent, so how about changing the rest of the flags to make them consistent? They need to have an as-is state, which corresponds to no flags set, and you then leave that field as is. We currently have four fields in the handle flags - direction, active state, drive, and bias. Of those, direction and bias have as-is states. What we are missing are additional flags so we have an as-is state for active state and drive. Currently: ACTIVE_LOW == 0 => ACTIVE_HIGH, and OPEN_DRAIN == 0 && OPEN_SOURCE == 0 => PUSH_PULL. If we added an ACTIVE_HIGH flag, to counter ACTIVE_LOW, and PUSH_PULL to counter OPEN_DRAIN/OPEN_SOURCE, then we can have SET_CONFIG change the four fields (direction, active state, drive and bias), independently, or not, as the caller sees fit. For backward compatibility, the lines would be created with ACTIVE_HIGH and PUSH_PULL set, should they be requested "as-is", by the new definition. This feels like the right solution to me - as I write this anyway. The biggest downside I can see is that it means pulling v6, or at least the SET_CONFIG patches, pending an update. > When I think about it: the kernel behavior should be as predictable as > possible - if we keep the behavior as is in v6, I don't see why we > couldn't make userspace cache (or re-read) the current direction when > setting flags other than direction? Do you see any trouble in that? > That way we'd avoid having the kernel treat different flags in > different way. > If in userspace then it will have to be cached - the kernel still has issues reading back output values for emulated open drain/source outputs. Fixing that is somewhere down my todo list. I can't think of any concrete problems with caching. It gives me "I have a bad feeling about this" vibe, but I can't pin down why. Maybe wanting to avoid shadowing kernel state in userspace? But, as above, I'd rather fix the flags so we have as-is for all, and caching becomes unnecessary. > Bart > > > > I see these options: > > > 1. set the direction as part of gpiod_line_set_config > > > 2. relax the kernel restriction. > > > > Yes, I don't think we should force users to always pass the direction > > flag in set_config. Good point. > > > > > 3. don't support changing bias or open source/drain. > > > > No! We definitely want to support it in libgpiod. Agreed. > > > > > 4. rethink the API. > > > > As for libgpiod: I think we should have a low-level > > gpiod_line_set_config() that would set both the direction and other > > flags (it could for instance only accept two request flags: input and > > output) and then a higher-level set_flags(), set_direction_input(), > > set_direction_output() that would call the low-level variant and - for > > set_flags() without the direction argument - it could simply retrieve > > the current direction and pass it to gpiod_line_set_config(). > > I agree that there should add be a fully capable low level option. The low level function would look have a signature like this: int gpiod_line_set_config(struct gpiod_line *line, int direction, int flags, const int *default_vals) The existing gpiod_line_set_config would be renamed gpiod_line_set_flags. > > But this is only a vague idea - I'd have to actually start looking at > > the code to be sure. I'd love to see what you came up with so far > > though! > > Indeed - what I had in mind changed radically once I had a closer look at the libgpiod API. And it is still changing. Cheers, Kent.
śr., 6 lis 2019 o 07:48 Kent Gibson <warthog618@gmail.com> napisał(a): > > On Tue, Nov 05, 2019 at 10:07:58PM +0100, Bartosz Golaszewski wrote: > > wt., 5 lis 2019 o 17:24 Bartosz Golaszewski > > <bgolaszewski@baylibre.com> napisał(a): > > > > > > wt., 5 lis 2019 o 16:26 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > > > > > On Tue, Nov 05, 2019 at 10:04:22AM +0800, Kent Gibson wrote: > > > > > Patches are against Bart's gpio/for-kent branch[1]. > > > > > > > > > > The patch has been successfully tested against gpio-mockup, and > > > > > on a Raspberry Pi, in both cases using the feature/pud_set_config > > > > > branch of my Go gpiod library[2], as well as with my feature/pud > > > > > development branch of libgpiod[3]. Patch 7 has only been tested using > > > > > my gpiod library as libgpiod has not yet been updated to support the > > > > > SET_CONFIG ioctl. > > > > > > > > > > > > > I've just pushed a first pass at SET_CONFIG support into my libgpiod > > > > feature/pud branch. It is causing me a bit of grief. Due to the way > > > > the libgpiod API is structured, with the direction flags pulled out into > > > > the request type, I thought it would be cleaner to keep changes to direction > > > > orthogonal to changes to the other handle flags. > > > > > > > > > > I'd love to see that branch - is it public? > > > > > > > So I've added these methods to the API: > > > > > > > > int gpiod_line_set_config(struct gpiod_line *line, int flags) > > > > int gpiod_line_set_direction_input(struct gpiod_line *line) > > > > int gpiod_line_set_direction_output(struct gpiod_line *line, > > > > int value) > > > > > > > > along with their bulk equivalents. > > > > > > > > I've coded that and started adding tests when I tripped over changing > > > > bias. The kernel requires a direction to be set, but I'm setting it > > > > as-is in gpiod_line_set_config - so that wont work. > > > > Open drain/source are in the same boat - they require output mode. > > > > > > > > > > Ha! Yes this is a problem - how about this: > > > > > > If the caller of set_config in the kernel doesn't pass any of the > > > direction flags, then we read the current direction, set the right > > > flag in lflags and only then call the function validating the flags? > > > > > > Cc'ing Linus, I'm not sure when he was dropped from this discussion. > I was also thinking along that line - the config would be carried over > from the initial request and any subsequent SET_CONFIGs. > The kernel could overlay the existing config over any field set > "as-is" before performing validation. > The validation requirement would stand, but you don't need to pass the > complete state, possibly including output values, with each SET_CONFIG > request. > > In the bias case above, I create the line as an output and so should > be able to set the bias, even if neither INPUT nor OUTPUT is set in > the SET_CONFIG request. > > > After another thought: this would be a bit inconsistent with the rest > > of the flags. IIRC this was the reason for me to split the > > request_type and other flags into two separate fields in libgpiod in > > the first place. > > > > It is a bit inconsisent, so how about changing the rest of the flags > to make them consistent? They need to have an as-is state, which > corresponds to no flags set, and you then leave that field as is. > We currently have four fields in the handle flags - direction, active > state, drive, and bias. Of those, direction and bias have as-is states. > What we are missing are additional flags so we have an as-is state for > active state and drive. > > Currently: > ACTIVE_LOW == 0 => ACTIVE_HIGH, and > OPEN_DRAIN == 0 && OPEN_SOURCE == 0 => PUSH_PULL. > > If we added an ACTIVE_HIGH flag, to counter ACTIVE_LOW, and PUSH_PULL > to counter OPEN_DRAIN/OPEN_SOURCE, then we can have SET_CONFIG change > the four fields (direction, active state, drive and bias), independently, > or not, as the caller sees fit. > > For backward compatibility, the lines would be created with ACTIVE_HIGH > and PUSH_PULL set, should they be requested "as-is", by the new > definition. > I'm not in favor of having the same behavior triggered in two different ways: one explicit and one implicit. This API got released and we have to live with it, I'm afraid. We could for instance add a comment to the uAPI header though. I also don't think new AS_IS flags are necessary. We can live fine with certain inconveniences in the ioctl() API as long as user-space libraries make up for it by structuring these calls differently. To summarize: I'd prefer to make the SET_CONFIG ioctl() require specifying the direction and then simply caching it in user-space. > This feels like the right solution to me - as I write this anyway. > The biggest downside I can see is that it means pulling v6, or at least > the SET_CONFIG patches, pending an update. > > > When I think about it: the kernel behavior should be as predictable as > > possible - if we keep the behavior as is in v6, I don't see why we > > couldn't make userspace cache (or re-read) the current direction when > > setting flags other than direction? Do you see any trouble in that? > > That way we'd avoid having the kernel treat different flags in > > different way. > > > > If in userspace then it will have to be cached - the kernel still has > issues reading back output values for emulated open drain/source outputs. > Fixing that is somewhere down my todo list. > Right, but we've lived with problems in this area for a long time and nobody complained - maybe it can wait just a bit more. :) > I can't think of any concrete problems with caching. > It gives me "I have a bad feeling about this" vibe, but I can't pin > down why. Maybe wanting to avoid shadowing kernel state in userspace? > The user-space can always read back the current state with the lineinfo ioctl(), right? Any problems with that? > But, as above, I'd rather fix the flags so we have as-is for all, and > caching becomes unnecessary. > This idea in turn gives me a bad feeling. There's something too implicit in this behavior for me. Sounds like it's too easy to get it wrong from user-space. > > Bart > > > > > > I see these options: > > > > 1. set the direction as part of gpiod_line_set_config > > > > 2. relax the kernel restriction. > > > > > > Yes, I don't think we should force users to always pass the direction > > > flag in set_config. Good point. > > > > > > > 3. don't support changing bias or open source/drain. > > > > > > No! We definitely want to support it in libgpiod. > > Agreed. > > > > > > > > 4. rethink the API. > > > > > > As for libgpiod: I think we should have a low-level > > > gpiod_line_set_config() that would set both the direction and other > > > flags (it could for instance only accept two request flags: input and > > > output) and then a higher-level set_flags(), set_direction_input(), > > > set_direction_output() that would call the low-level variant and - for > > > set_flags() without the direction argument - it could simply retrieve > > > the current direction and pass it to gpiod_line_set_config(). > > > > > I agree that there should add be a fully capable low level option. > > The low level function would look have a signature like this: > > int gpiod_line_set_config(struct gpiod_line *line, int direction, int flags, > const int *default_vals) > > The existing gpiod_line_set_config would be renamed gpiod_line_set_flags. > > > > But this is only a vague idea - I'd have to actually start looking at > > > the code to be sure. I'd love to see what you came up with so far > > > though! > > > > > Indeed - what I had in mind changed radically once I had a closer look > at the libgpiod API. And it is still changing. > Thanks for doing it! It's also great you included some test cases! Bart > Cheers, > Kent. >
On Wed, Nov 06, 2019 at 02:59:33PM +0100, Bartosz Golaszewski wrote: > śr., 6 lis 2019 o 07:48 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > On Tue, Nov 05, 2019 at 10:07:58PM +0100, Bartosz Golaszewski wrote: > > > wt., 5 lis 2019 o 17:24 Bartosz Golaszewski > > > <bgolaszewski@baylibre.com> napisał(a): > > > > > > > > wt., 5 lis 2019 o 16:26 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > > > > > > > On Tue, Nov 05, 2019 at 10:04:22AM +0800, Kent Gibson wrote: > > > > > > Patches are against Bart's gpio/for-kent branch[1]. > > > > > > > > > > > > The patch has been successfully tested against gpio-mockup, and > > > > > > on a Raspberry Pi, in both cases using the feature/pud_set_config > > > > > > branch of my Go gpiod library[2], as well as with my feature/pud > > > > > > development branch of libgpiod[3]. Patch 7 has only been tested using > > > > > > my gpiod library as libgpiod has not yet been updated to support the > > > > > > SET_CONFIG ioctl. > > > > > > > > > > > > > > > > I've just pushed a first pass at SET_CONFIG support into my libgpiod > > > > > feature/pud branch. It is causing me a bit of grief. Due to the way > > > > > the libgpiod API is structured, with the direction flags pulled out into > > > > > the request type, I thought it would be cleaner to keep changes to direction > > > > > orthogonal to changes to the other handle flags. > > > > > > > > > > > > > I'd love to see that branch - is it public? > > > > > > > > > So I've added these methods to the API: > > > > > > > > > > int gpiod_line_set_config(struct gpiod_line *line, int flags) > > > > > int gpiod_line_set_direction_input(struct gpiod_line *line) > > > > > int gpiod_line_set_direction_output(struct gpiod_line *line, > > > > > int value) > > > > > > > > > > along with their bulk equivalents. > > > > > > > > > > I've coded that and started adding tests when I tripped over changing > > > > > bias. The kernel requires a direction to be set, but I'm setting it > > > > > as-is in gpiod_line_set_config - so that wont work. > > > > > Open drain/source are in the same boat - they require output mode. > > > > > > > > > > > > > Ha! Yes this is a problem - how about this: > > > > > > > > If the caller of set_config in the kernel doesn't pass any of the > > > > direction flags, then we read the current direction, set the right > > > > flag in lflags and only then call the function validating the flags? > > > > > > > > > > > Cc'ing Linus, I'm not sure when he was dropped from this discussion. > My bad - I cut it down to you and Drew + list in my initial reply as I thought we'd only be talking libgpiod changes. But then it took a sharp u-turn back to the kernel patch. Anway I think we are now in agreement that the kernel is fine and we're back in libgpiod territory - see below. > > I was also thinking along that line - the config would be carried over > > from the initial request and any subsequent SET_CONFIGs. > > The kernel could overlay the existing config over any field set > > "as-is" before performing validation. > > The validation requirement would stand, but you don't need to pass the > > complete state, possibly including output values, with each SET_CONFIG > > request. > > > > In the bias case above, I create the line as an output and so should > > be able to set the bias, even if neither INPUT nor OUTPUT is set in > > the SET_CONFIG request. > > > > > After another thought: this would be a bit inconsistent with the rest > > > of the flags. IIRC this was the reason for me to split the > > > request_type and other flags into two separate fields in libgpiod in > > > the first place. > > > > > > > It is a bit inconsisent, so how about changing the rest of the flags > > to make them consistent? They need to have an as-is state, which > > corresponds to no flags set, and you then leave that field as is. > > We currently have four fields in the handle flags - direction, active > > state, drive, and bias. Of those, direction and bias have as-is states. > > What we are missing are additional flags so we have an as-is state for > > active state and drive. > > > > Currently: > > ACTIVE_LOW == 0 => ACTIVE_HIGH, and > > OPEN_DRAIN == 0 && OPEN_SOURCE == 0 => PUSH_PULL. > > > > If we added an ACTIVE_HIGH flag, to counter ACTIVE_LOW, and PUSH_PULL > > to counter OPEN_DRAIN/OPEN_SOURCE, then we can have SET_CONFIG change > > the four fields (direction, active state, drive and bias), independently, > > or not, as the caller sees fit. > > > > For backward compatibility, the lines would be created with ACTIVE_HIGH > > and PUSH_PULL set, should they be requested "as-is", by the new > > definition. > > > > I'm not in favor of having the same behavior triggered in two > different ways: one explicit and one implicit. This API got released > and we have to live with it, I'm afraid. We could for instance add a > comment to the uAPI header though. I also don't think new AS_IS flags > are necessary. We can live fine with certain inconveniences in the > ioctl() API as long as user-space libraries make up for it by > structuring these calls differently. > > To summarize: I'd prefer to make the SET_CONFIG ioctl() require > specifying the direction and then simply caching it in user-space. > Agreed. I had a quick play with changing the kernel and it was ugly. Too many conversions between flag layouts and messing with flag masks and bits. It was absolutely hideous. Requiring the userspace to provide the complete config is much simpler. > > This feels like the right solution to me - as I write this anyway. > > The biggest downside I can see is that it means pulling v6, or at least > > the SET_CONFIG patches, pending an update. > > > > > When I think about it: the kernel behavior should be as predictable as > > > possible - if we keep the behavior as is in v6, I don't see why we > > > couldn't make userspace cache (or re-read) the current direction when > > > setting flags other than direction? Do you see any trouble in that? > > > That way we'd avoid having the kernel treat different flags in > > > different way. > > > > > > > If in userspace then it will have to be cached - the kernel still has > > issues reading back output values for emulated open drain/source outputs. > > Fixing that is somewhere down my todo list. > > > > Right, but we've lived with problems in this area for a long time and > nobody complained - maybe it can wait just a bit more. :) > Hey - I complained! I had to skip over some of my tests because of that ;-). > > I can't think of any concrete problems with caching. > > It gives me "I have a bad feeling about this" vibe, but I can't pin > > down why. Maybe wanting to avoid shadowing kernel state in userspace? > > > > The user-space can always read back the current state with the > lineinfo ioctl(), right? Any problems with that? > Only that you have to convert from the info flag layout to the request handle flag layout, or the libgpiod request_type + flag layout. And I've already had enough of that for one day. > > But, as above, I'd rather fix the flags so we have as-is for all, and > > caching becomes unnecessary. > > > > This idea in turn gives me a bad feeling. There's something too > implicit in this behavior for me. Sounds like it's too easy to get it > wrong from user-space. > > > > Bart > > > > > > > > I see these options: > > > > > 1. set the direction as part of gpiod_line_set_config > > > > > 2. relax the kernel restriction. > > > > > > > > Yes, I don't think we should force users to always pass the direction > > > > flag in set_config. Good point. > > > > > > > > > 3. don't support changing bias or open source/drain. > > > > > > > > No! We definitely want to support it in libgpiod. > > > > Agreed. > > > > > > > > > > > 4. rethink the API. > > > > > > > > As for libgpiod: I think we should have a low-level > > > > gpiod_line_set_config() that would set both the direction and other > > > > flags (it could for instance only accept two request flags: input and > > > > output) and then a higher-level set_flags(), set_direction_input(), > > > > set_direction_output() that would call the low-level variant and - for > > > > set_flags() without the direction argument - it could simply retrieve > > > > the current direction and pass it to gpiod_line_set_config(). > > > > > > > > I agree that there should add be a fully capable low level option. > > > > The low level function would look have a signature like this: > > > > int gpiod_line_set_config(struct gpiod_line *line, int direction, int flags, > > const int *default_vals) > > > > The existing gpiod_line_set_config would be renamed gpiod_line_set_flags. > > > > > > But this is only a vague idea - I'd have to actually start looking at > > > > the code to be sure. I'd love to see what you came up with so far > > > > though! > > > > > > > > Indeed - what I had in mind changed radically once I had a closer look > > at the libgpiod API. And it is still changing. > > > > Thanks for doing it! It's also great you included some test cases! > Oh, I always write test cases - manual testing is for monkeys. The few I've added so far are just basic smoke tests - there will be quite a few more added with better coverage once the API settles down. I've pushed some more changes with the updated API we discussed earlier. Those new tests I'd added now pass. Yay. One problem though - gpiod_line_set_config as written has no way to accept an as-is direction. Hopefully I'll have some time to take another look at that tomorrow. Cheers, Kent.
śr., 6 lis 2019 o 17:58 Kent Gibson <warthog618@gmail.com> napisał(a): > > On Wed, Nov 06, 2019 at 02:59:33PM +0100, Bartosz Golaszewski wrote: > > śr., 6 lis 2019 o 07:48 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > > > On Tue, Nov 05, 2019 at 10:07:58PM +0100, Bartosz Golaszewski wrote: > > > > wt., 5 lis 2019 o 17:24 Bartosz Golaszewski > > > > <bgolaszewski@baylibre.com> napisał(a): > > > > > > > > > > wt., 5 lis 2019 o 16:26 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > > > > > > > > > On Tue, Nov 05, 2019 at 10:04:22AM +0800, Kent Gibson wrote: > > > > > > > Patches are against Bart's gpio/for-kent branch[1]. > > > > > > > > > > > > > > The patch has been successfully tested against gpio-mockup, and > > > > > > > on a Raspberry Pi, in both cases using the feature/pud_set_config > > > > > > > branch of my Go gpiod library[2], as well as with my feature/pud > > > > > > > development branch of libgpiod[3]. Patch 7 has only been tested using > > > > > > > my gpiod library as libgpiod has not yet been updated to support the > > > > > > > SET_CONFIG ioctl. > > > > > > > > > > > > > > > > > > > I've just pushed a first pass at SET_CONFIG support into my libgpiod > > > > > > feature/pud branch. It is causing me a bit of grief. Due to the way > > > > > > the libgpiod API is structured, with the direction flags pulled out into > > > > > > the request type, I thought it would be cleaner to keep changes to direction > > > > > > orthogonal to changes to the other handle flags. > > > > > > > > > > > > > > > > I'd love to see that branch - is it public? > > > > > > > > > > > So I've added these methods to the API: > > > > > > > > > > > > int gpiod_line_set_config(struct gpiod_line *line, int flags) > > > > > > int gpiod_line_set_direction_input(struct gpiod_line *line) > > > > > > int gpiod_line_set_direction_output(struct gpiod_line *line, > > > > > > int value) > > > > > > > > > > > > along with their bulk equivalents. > > > > > > > > > > > > I've coded that and started adding tests when I tripped over changing > > > > > > bias. The kernel requires a direction to be set, but I'm setting it > > > > > > as-is in gpiod_line_set_config - so that wont work. > > > > > > Open drain/source are in the same boat - they require output mode. > > > > > > > > > > > > > > > > Ha! Yes this is a problem - how about this: > > > > > > > > > > If the caller of set_config in the kernel doesn't pass any of the > > > > > direction flags, then we read the current direction, set the right > > > > > flag in lflags and only then call the function validating the flags? > > > > > > > > > > > > > > > > Cc'ing Linus, I'm not sure when he was dropped from this discussion. > > > > My bad - I cut it down to you and Drew + list in my initial reply as I > thought we'd only be talking libgpiod changes. > But then it took a sharp u-turn back to the kernel patch. > > Anway I think we are now in agreement that the kernel is fine and we're > back in libgpiod territory - see below. > > > > I was also thinking along that line - the config would be carried over > > > from the initial request and any subsequent SET_CONFIGs. > > > The kernel could overlay the existing config over any field set > > > "as-is" before performing validation. > > > The validation requirement would stand, but you don't need to pass the > > > complete state, possibly including output values, with each SET_CONFIG > > > request. > > > > > > In the bias case above, I create the line as an output and so should > > > be able to set the bias, even if neither INPUT nor OUTPUT is set in > > > the SET_CONFIG request. > > > > > > > After another thought: this would be a bit inconsistent with the rest > > > > of the flags. IIRC this was the reason for me to split the > > > > request_type and other flags into two separate fields in libgpiod in > > > > the first place. > > > > > > > > > > It is a bit inconsisent, so how about changing the rest of the flags > > > to make them consistent? They need to have an as-is state, which > > > corresponds to no flags set, and you then leave that field as is. > > > We currently have four fields in the handle flags - direction, active > > > state, drive, and bias. Of those, direction and bias have as-is states. > > > What we are missing are additional flags so we have an as-is state for > > > active state and drive. > > > > > > Currently: > > > ACTIVE_LOW == 0 => ACTIVE_HIGH, and > > > OPEN_DRAIN == 0 && OPEN_SOURCE == 0 => PUSH_PULL. > > > > > > If we added an ACTIVE_HIGH flag, to counter ACTIVE_LOW, and PUSH_PULL > > > to counter OPEN_DRAIN/OPEN_SOURCE, then we can have SET_CONFIG change > > > the four fields (direction, active state, drive and bias), independently, > > > or not, as the caller sees fit. > > > > > > For backward compatibility, the lines would be created with ACTIVE_HIGH > > > and PUSH_PULL set, should they be requested "as-is", by the new > > > definition. > > > > > > > I'm not in favor of having the same behavior triggered in two > > different ways: one explicit and one implicit. This API got released > > and we have to live with it, I'm afraid. We could for instance add a > > comment to the uAPI header though. I also don't think new AS_IS flags > > are necessary. We can live fine with certain inconveniences in the > > ioctl() API as long as user-space libraries make up for it by > > structuring these calls differently. > > > > To summarize: I'd prefer to make the SET_CONFIG ioctl() require > > specifying the direction and then simply caching it in user-space. > > > > Agreed. I had a quick play with changing the kernel and it was ugly. > Too many conversions between flag layouts and messing with flag masks > and bits. It was absolutely hideous. > Requiring the userspace to provide the complete config is much simpler. > > > > This feels like the right solution to me - as I write this anyway. > > > The biggest downside I can see is that it means pulling v6, or at least > > > the SET_CONFIG patches, pending an update. > > > > > > > When I think about it: the kernel behavior should be as predictable as > > > > possible - if we keep the behavior as is in v6, I don't see why we > > > > couldn't make userspace cache (or re-read) the current direction when > > > > setting flags other than direction? Do you see any trouble in that? > > > > That way we'd avoid having the kernel treat different flags in > > > > different way. > > > > > > > > > > If in userspace then it will have to be cached - the kernel still has > > > issues reading back output values for emulated open drain/source outputs. > > > Fixing that is somewhere down my todo list. > > > > > > > Right, but we've lived with problems in this area for a long time and > > nobody complained - maybe it can wait just a bit more. :) > > > > Hey - I complained! I had to skip over some of my tests because of that ;-). > > > > I can't think of any concrete problems with caching. > > > It gives me "I have a bad feeling about this" vibe, but I can't pin > > > down why. Maybe wanting to avoid shadowing kernel state in userspace? > > > > > > > The user-space can always read back the current state with the > > lineinfo ioctl(), right? Any problems with that? > > > > Only that you have to convert from the info flag layout to the request > handle flag layout, or the libgpiod request_type + flag layout. > And I've already had enough of that for one day. > > > > But, as above, I'd rather fix the flags so we have as-is for all, and > > > caching becomes unnecessary. > > > > > > > This idea in turn gives me a bad feeling. There's something too > > implicit in this behavior for me. Sounds like it's too easy to get it > > wrong from user-space. > > > > > > Bart > > > > > > > > > > I see these options: > > > > > > 1. set the direction as part of gpiod_line_set_config > > > > > > 2. relax the kernel restriction. > > > > > > > > > > Yes, I don't think we should force users to always pass the direction > > > > > flag in set_config. Good point. > > > > > > > > > > > 3. don't support changing bias or open source/drain. > > > > > > > > > > No! We definitely want to support it in libgpiod. > > > > > > Agreed. > > > > > > > > > > > > > > 4. rethink the API. > > > > > > > > > > As for libgpiod: I think we should have a low-level > > > > > gpiod_line_set_config() that would set both the direction and other > > > > > flags (it could for instance only accept two request flags: input and > > > > > output) and then a higher-level set_flags(), set_direction_input(), > > > > > set_direction_output() that would call the low-level variant and - for > > > > > set_flags() without the direction argument - it could simply retrieve > > > > > the current direction and pass it to gpiod_line_set_config(). > > > > > > > > > > > I agree that there should add be a fully capable low level option. > > > > > > The low level function would look have a signature like this: > > > > > > int gpiod_line_set_config(struct gpiod_line *line, int direction, int flags, > > > const int *default_vals) > > > > > > The existing gpiod_line_set_config would be renamed gpiod_line_set_flags. > > > > > > > > But this is only a vague idea - I'd have to actually start looking at > > > > > the code to be sure. I'd love to see what you came up with so far > > > > > though! > > > > > > > > > > > Indeed - what I had in mind changed radically once I had a closer look > > > at the libgpiod API. And it is still changing. > > > > > > > Thanks for doing it! It's also great you included some test cases! > > > > Oh, I always write test cases - manual testing is for monkeys. > The few I've added so far are just basic smoke tests - there will be > quite a few more added with better coverage once the API settles down. > > I've pushed some more changes with the updated API we discussed earlier. > Those new tests I'd added now pass. Yay. > One problem though - gpiod_line_set_config as written has no way to > accept an as-is direction. > Hopefully I'll have some time to take another look at that tomorrow. > This is not something that we need to merge the kernel changes, it won't hit a libgpiod release until v5.5 is tagged anyway. Unless I'm missing something, v6 is still fine to be merged upstream, right? Do we agree on requiring the full config in SET_CONFIG? Bart > Cheers, > Kent. >
On Wed, Nov 06, 2019 at 06:06:12PM +0100, Bartosz Golaszewski wrote: > > This is not something that we need to merge the kernel changes, it > won't hit a libgpiod release until v5.5 is tagged anyway. Unless I'm > missing something, v6 is still fine to be merged upstream, right? Do > we agree on requiring the full config in SET_CONFIG? > I agree that the user needs to pass the full config to SET_CONFIG and therefore that v6 is still good to merge. The corresponding libgpiod work will be ongoing for a while. Cheers, Kent.
On Tue, Nov 5, 2019 at 4:05 PM Bartosz Golaszewski <bgolaszewski@baylibre.com> wrote: > Ok, all looks good now and since Linus already said he's fine with > this series, I'll take it through my tree for v5.5. OK good we get it to -next through your tree and I await a pull request from you for these. Very nice work from everyone involved here! Yours, Linus Walleij
On Thu, Nov 07, 2019 at 12:58:04AM +0800, Kent Gibson wrote: > I've pushed some more changes with the updated API we discussed earlier. > Those new tests I'd added now pass. Yay. > One problem though - gpiod_line_set_config as written has no way to > accept an as-is direction. > Hopefully I'll have some time to take another look at that tomorrow. > I've pushed some more updates to my libgpiod branch[1]. They fix the direction limitation I mentioned (I was using the wrong set flags), and extend the tests to cover all of the SET_CONFIG fields. That completes the C API changes. If that is ok with you then I can take a look at the corresponding changes to the C++ and Python bindings. And I guess we should move this libgpiod discussion to a new thread? Cheers, Kent. [1] https://github.com/warthog618/libgpiod.git
czw., 7 lis 2019 o 11:39 Kent Gibson <warthog618@gmail.com> napisał(a): > > On Thu, Nov 07, 2019 at 12:58:04AM +0800, Kent Gibson wrote: > > I've pushed some more changes with the updated API we discussed earlier. > > Those new tests I'd added now pass. Yay. > > One problem though - gpiod_line_set_config as written has no way to > > accept an as-is direction. > > Hopefully I'll have some time to take another look at that tomorrow. > > > > I've pushed some more updates to my libgpiod branch[1]. They fix the > direction limitation I mentioned (I was using the wrong set flags), > and extend the tests to cover all of the SET_CONFIG fields. > > That completes the C API changes. > If that is ok with you then I can take a look at the corresponding > changes to the C++ and Python bindings. > > And I guess we should move this libgpiod discussion to a new thread? Yes, and better yet - you could simply send these patches for review and we can continue the discussion there. Bart > > Cheers, > Kent. > > [1] https://github.com/warthog618/libgpiod.git >
On Thu, Nov 07, 2019 at 12:28:09PM +0100, Bartosz Golaszewski wrote: > czw., 7 lis 2019 o 11:39 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > On Thu, Nov 07, 2019 at 12:58:04AM +0800, Kent Gibson wrote: > > > I've pushed some more changes with the updated API we discussed earlier. > > > Those new tests I'd added now pass. Yay. > > > One problem though - gpiod_line_set_config as written has no way to > > > accept an as-is direction. > > > Hopefully I'll have some time to take another look at that tomorrow. > > > > > > > I've pushed some more updates to my libgpiod branch[1]. They fix the > > direction limitation I mentioned (I was using the wrong set flags), > > and extend the tests to cover all of the SET_CONFIG fields. > > > > That completes the C API changes. > > If that is ok with you then I can take a look at the corresponding > > changes to the C++ and Python bindings. > > > > And I guess we should move this libgpiod discussion to a new thread? > > Yes, and better yet - you could simply send these patches for review > and we can continue the discussion there. > I was holding off on putting a patch series together until all the functionality was complete - so after the C++ and Python bindings. Cheers, Kent.
wt., 5 lis 2019 o 03:04 Kent Gibson <warthog618@gmail.com> napisał(a): > > The changes from v5: > - rebased onto Bart's gpio/for-kent branch. > > The changes from v4: > Review changes: > - relocate restriction on applying bias to as-is from patch 2 to patch 1. > - propagate errors, other than ENOTSUPP, from gpio_set_bias. (squashed > into patches 3 and 4). > - include SET_CONFIG patch series v2 (patch 6 and 7 here). > > I've also fixed a few other nits I noticed along the way: > - rework gpio_set_bias as flags are mutually exclusive. > - remove input flag required to set bias restriction from > lineevent_create as events are implicitly assumed inputs anyway. > - reorder patches to group gpiolib bias patches together before the > gpio-mockup changes. > > > This series adds gross control of pull-up/pull-down to the GPIO uAPI. > Gross control means enabling and disabling of bias functionality, > not finer grained control such as setting biasing impedances. > > The support allows both input and output lines to have any one of the > following biases applied as part of the line handle or event request: > 0. As Is - bias is left alone. This is the default for ABI compatibility. > 1. Bias Disable - bias is explicitly disabled. > 2. Pull Down - pull-down bias is enabled. > 3. Pull Up - pull-up bias is enabled. > > The biases are set via three flags, BIAS_DISABLE, BIAS_PULL_DOWN > and BIAS_PULL_UP. These map directly to the similarly named > pinctrl pin_config_param flags. > As Is corresponds to none of the flags being set. > > The setting of biases on output lines may seem odd, but is to allow for > utilisation of internal pull-up/pull-down on open drain and open source > outputs, where supported in hardware. > > The series also adds the GPIOHANDLE_SET_CONFIG_IOCTL to the gpio chardev. > The ioctl allows some of the configuration of a requested handle to be > changed without having to release the line. > The primary use case is the changing of direction for bi-directional > lines. > > Patches are against Bart's gpio/for-kent branch[1]. > > The patch has been successfully tested against gpio-mockup, and > on a Raspberry Pi, in both cases using the feature/pud_set_config > branch of my Go gpiod library[2], as well as with my feature/pud > development branch of libgpiod[3]. Patch 7 has only been tested using > my gpiod library as libgpiod has not yet been updated to support the > SET_CONFIG ioctl. > > Patch 1 adds pull-up/pull-down support to line handle requests. > Patch 2 adds pull-up/pull-down support to line event requests. > Patch 3 adds support for disabling bias. > Patch 4 adds support for setting bias on output lines. > Patch 5 adds pull-up/down support to the gpio-mockup for uAPI testing. > Patch 6 refactors the flag validation from linehandle_create. > Patch 7 adds the SET_CONFIG ioctl. > > Drew Fustini (1): > gpio: expose pull-up/pull-down line flags to userspace > > Kent Gibson (6): > gpiolib: add support for pull up/down to lineevent_create > gpiolib: add support for disabling line bias > gpiolib: add support for biasing output lines > gpio: mockup: add set_config to support pull up/down > gpiolib: move validation of line handle flags into helper function > gpio: add new SET_CONFIG ioctl() to gpio chardev > > drivers/gpio/gpio-mockup.c | 94 ++++++++++------ > drivers/gpio/gpiolib.c | 213 +++++++++++++++++++++++++++++++------ > drivers/gpio/gpiolib.h | 1 + > include/uapi/linux/gpio.h | 24 +++++ > 4 files changed, 264 insertions(+), 68 deletions(-) > > -- > 2.23.0 > Applied all patches to for-next. Thanks! Bart
On Tue, Nov 05, 2019 at 10:04:22AM +0800, Kent Gibson wrote: > This series adds gross control o:f pull-up/pull-down to the GPIO uAPI. > Gross control means enabling and disabling of bias functionality, > not finer grained control such as setting biasing impedances. > > The support allows both input and output lines to have any one of the > following biases applied as part of the line handle or event request: > 0. As Is - bias is left alone. This is the default for ABI compatibility. > 1. Bias Disable - bias is explicitly disabled. > 2. Pull Down - pull-down bias is enabled. > 3. Pull Up - pull-up bias is enabled. > > The biases are set via three flags, BIAS_DISABLE, BIAS_PULL_DOWN > and BIAS_PULL_UP. These map directly to the similarly named > pinctrl pin_config_param flags. > As Is corresponds to none of the flags being set. I had been looking at how to make these flags work on the BeagleBone (TI Sitara AM3358 SoC) which uses the gpio-omap driver and pinctrl-single driver. Howeverm, it seems that it is not posssible as the BeagleBone device tree uses compatible of "pinctrl-single" instead of "padconf-single", and thus pcs_pinconf_set() is not called [0]. The bias flags already work on the Raspberry Pi as the Broadcom SoC uses pinctrl-bcm2835.c which uses gpiochip_generic_config() from gpiolib.c for .set_config in bcm2835_gpio_chip. This eventually calls bcm2835_pinconf_set() which handles the PIN_CONFIG_BIAS_* flags. Thus, I started thinking about what other boards I could test the bias flags with, and potentially, find drivers that I could add or fix support. The PIN_CONFIG_BIAS_* flags exist in these GPIO drivers: gpio-aspeed.c: aspeed seems to be used in server BMC so not hardware that I have access to or could easily acquire. gpio-mockup.c: mockup driver that has already been tested and works gpio-pca953x.c: an external I2C GPIO expander, easy for me to acquire, it appears that gpio uapi bias flags should work, but I could test and verify gpiolib.c: like the bmc2835 in the raspberry pi, it seems some drivers in pinctrl directory will define a gpiochip and use gpiochip_generic_config(). Here is a list of other pinctrl drivers which use gpiochip_generic_config: drivers/pinctrl/pinctrl-stmfx.c and drivers/pinctrl/pinctrl-sx150x.c: These are both GPIO expanders which I could probably purchase drivers/pinctrl/intel/pinctrl-intel.c: Maybe this means that I could try this on x86 boards? drivers/pinctrl/sunxi/pinctrl-sunxi.c Maybe the Allwinner boards might work too? Any other hardware that I should take a look at testing? Thanks, Drew [0] https://lore.kernel.org/linux-gpio/20200413123921.GA32586@x1/T/#t