gpio: expose line bias flags to userspace

Add support for pull up/down to lineevent_create. Use cases include receiving asynchronous presses from a push button without an external pull up/down.

Also restrict the application of bias to lines explicitly requested as inputs to prevent bias being applied to as-is line requests.

Signed-off-by Kent Gibson <[email protected]>

---
 drivers/gpio/gpiolib.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

drivers/gpio/gpiolib.c +0 -0

index e80e689be2cc..7dfbb3676ee0 100644

drivers/gpio/gpiolib.c +0 -1

        ret = gpiod_direction_input(desc);
        if (ret)
--

2.23.0

pon., 28 paź 2019 o 08:38 Kent Gibson <[email protected]> napisał(a):

Quoted 22 lines

Add support for pull up/down to lineevent_create. Use cases include receiving asynchronous presses from a push button without an external pull up/down.

Also restrict the application of bias to lines explicitly requested as inputs to prevent bias being applied to as-is line requests.

Signed-off-by Kent Gibson <[email protected]>

---
 drivers/gpio/gpiolib.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

drivers/gpio/gpiolib.c +0 -0

index e80e689be2cc..7dfbb3676ee0 100644
@@ -554,6 +554,12 @@ static int linehandle_create(struct gpio_device *gdev, void __user *ip)
             (lflags & GPIOHANDLE_REQUEST_OPEN_SOURCE)))
                return -EINVAL;
 

drivers/gpio/gpiolib.c static int linehandle_create(struct gpio_device *gdev, void __user *ip) · line 562

Quoted 6 lines

drivers/gpio/gpiolib.c +6 -0

+       /* PULL_UP and PULL_DOWN flags only make sense for input mode. */
+       if (!(lflags & GPIOHANDLE_REQUEST_INPUT) &&
+           ((lflags & GPIOHANDLE_REQUEST_BIAS_PULL_UP) ||
+            (lflags & GPIOHANDLE_REQUEST_BIAS_PULL_DOWN)))
+               return -EINVAL;
+

This is not part of this patch - this must go into 1/5.

The major changes from v3 is the renaming of flags in patch 1, rather than in later patches, and the addition of sanity checking on bias flag combinations - only allowing one bias flag to be set at a time.

There are still some deficiencies that I'm uncertain on how to best resolve:

The setting of bias is performed by gpio_set_bias, which is hooked into gpiod_direction_input and gpiod_direction_output. It extends the setting of bias that was already present in gpiod_direction_input.
In keeping with the existing behaviour, the bias is set on a best-effort basis - no error is returned to the user if an error is returned by the pinctrl driver. Returning an error makes sense to me, particularly for the uAPI, but that conflicts with the existing gpiod_direction_input behaviour. So leave as best-effort, change gpiod_direction_input behaviour, or restructure to support both behaviours?

Also, the gpio_set_bias is hooked into gpiod_direction_output, which is fine for the uAPI, but perhaps it should be hooked into gpiod_direction_output_raw_commit? Or the setting of direction and bias should be decoupled?

And now the actual blurb...

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.

Patches are against Bart's gpio/for-next branch[1].

The patch has been successfully tested against gpio-mockup, and on a Raspberry Pi, in both cases using the feature/pud branch of my Go gpiod library[2], as well as with my feature/pud development branch of libgpiod[3].

Patch 1 adds support to line handle requests.
Patch 2 adds support to line event requests and restricts bias settings to lines explicitly requested as inputs.
Patch 3 adds pull-up/down support to the gpio-mockup for uAPI testing. Patch 4 adds support for disabling bias.
Patch 5 adds support for setting bias on output lines.

Drew Fustini (1):
gpio: expose pull-up/pull-down line flags to userspace

Kent Gibson (4):
gpiolib: add support for pull up/down to lineevent_create
gpio: mockup: add set_config to support pull up/down
gpiolib: add support for disabling line bias
gpiolib: add support for biasing output lines

 drivers/gpio/gpio-mockup.c | 94 ++++++++++++++++++++++++--------------
 drivers/gpio/gpiolib.c     | 81 +++++++++++++++++++++++++++++---
 drivers/gpio/gpiolib.h     |  1 +
 include/uapi/linux/gpio.h  |  6 +++
 4 files changed, 142 insertions(+), 40 deletions(-)

--
2.23.0

[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

[Kent]

Quoted 3 lines

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.

Quoted 4 lines · Bartosz Golaszewski

[Kent]

Quoted 3 lines

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.

Right, excellent and persistent work here, much appreciated!

As long as I get Bartosz's blanket ACK on v5 I think it is ready to merge. His consent is required for this.

It looks pretty much as I imagined it when I discussed it with Drew some while back, with some gritty details fixed up.

Yours,
Linus Walleij

Quoted 10 lines

Quoted 4 lines · Bartosz Golaszewski

[Kent]

Quoted 3 lines

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.

Right, excellent and persistent work here, much appreciated!

No problem - hopefully I haven't irritated too many people in the process.

Quoted 3 lines

As long as I get Bartosz's blanket ACK on v5 I think it is ready to merge. His consent is required for this.

I'm still waiting on open questions from v4 before submitting v5:

One, handling of errors when setting bias, Bart has referred to Thomas, so waiting for feedback on that.

The other, where gpio_set_bias is hooked into gpiod_direction_output, is fine as is for the UAPI - it can always be relocated subsequently if other APIs need to set bias. On the other hand, if decoupling setting direction and bias is in order then that really should be done now. Can I get an an ACK on that either way?

I've also made a couple of minor changes myself while reviewing v4 - reordering the patches to group the gpiolib.c ones and leaving the gpio-mockup til last, and removing the "bias requires input mode" check from lineevent_create as the line is assumed to be input for events regardless of the input flag - there is no such thing as as-is for event requests.
Only mentioning here in case such changes are clearly wrong...

Cheers,
Kent.

Quoted 5 lines

It looks pretty much as I imagined it when I discussed it with Drew some while back, with some gritty details fixed up.

Yours,
Linus Walleij

pon., 4 lis 2019 o 02:07 Kent Gibson <[email protected]> napisał(a):

Quoted 15 lines · Linus Walleij

Quoted 10 lines

Quoted 4 lines · Bartosz Golaszewski

[Kent]

Quoted 3 lines

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.

Right, excellent and persistent work here, much appreciated!

No problem - hopefully I haven't irritated too many people in the process.

Quoted 9 lines

Quoted 3 lines

As long as I get Bartosz's blanket ACK on v5 I think it is ready to merge. His consent is required for this.

I'm still waiting on open questions from v4 before submitting v5:

One, handling of errors when setting bias, Bart has referred to Thomas, so waiting for feedback on that.

If we can get it merged for v5.5, then I don't want to block it waiting for answers. Looking at the code I think we should only ignore the EOPNOTSUPP error and propagate all other codes. Can you add a patch changing that and then fix the other nits I pointed out? Also:
please Cc Thomas Petazzoni so that he gets the chance to yell at us if it breaks something.

Quoted 6 lines

The other, where gpio_set_bias is hooked into gpiod_direction_output, is fine as is for the UAPI - it can always be relocated subsequently if other APIs need to set bias. On the other hand, if decoupling setting direction and bias is in order then that really should be done now. Can I get an an ACK on that either way?

This is in line with what you did for input. I don't think it should be decoupled (any particular reason for that? There is none mentioned in the cover letter), so I propose to leave it as it is in patch 5/5.

One more thing - since we all want this to make it for v5.5 - can you make the set config patches part of this series (simply bunch it all together)? This will make it easy to review and merge everything.

Thanks in advance and great job!

Bartosz

Quoted 16 lines

I've also made a couple of minor changes myself while reviewing v4 - reordering the patches to group the gpiolib.c ones and leaving the gpio-mockup til last, and removing the "bias requires input mode" check from lineevent_create as the line is assumed to be input for events regardless of the input flag - there is no such thing as as-is for event requests.
Only mentioning here in case such changes are clearly wrong...

Cheers,
Kent.

Quoted 5 lines

It looks pretty much as I imagined it when I discussed it with Drew some while back, with some gritty details fixed up.

Yours,
Linus Walleij

Quoted 26 lines

pon., 4 lis 2019 o 02:07 Kent Gibson <[email protected]> napisał(a):

Quoted 15 lines · Linus Walleij

Quoted 10 lines

Quoted 4 lines · Bartosz Golaszewski

[Kent]

Quoted 3 lines

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.

Right, excellent and persistent work here, much appreciated!

No problem - hopefully I haven't irritated too many people in the process.

Quoted 9 lines

Quoted 3 lines

As long as I get Bartosz's blanket ACK on v5 I think it is ready to merge. His consent is required for this.

I'm still waiting on open questions from v4 before submitting v5:

One, handling of errors when setting bias, Bart has referred to Thomas, so waiting for feedback on that.

Quoted 7 lines

If we can get it merged for v5.5, then I don't want to block it waiting for answers. Looking at the code I think we should only ignore the EOPNOTSUPP error and propagate all other codes. Can you add a patch changing that and then fix the other nits I pointed out? Also:
please Cc Thomas Petazzoni so that he gets the chance to yell at us if it breaks something.

Will do.

Quoted 11 lines

Quoted 6 lines

The other, where gpio_set_bias is hooked into gpiod_direction_output, is fine as is for the UAPI - it can always be relocated subsequently if other APIs need to set bias. On the other hand, if decoupling setting direction and bias is in order then that really should be done now. Can I get an an ACK on that either way?

This is in line with what you did for input. I don't think it should be decoupled (any particular reason for that? There is none mentioned in the cover letter), so I propose to leave it as it is in patch 5/5.

Wrt decoupling, I was thinking of both input and output, not just output, though it was the output that got me onto that line of thought as gpiod_direction_output sets bias, but gpiod_direction_output_raw doesn't. That seemed totally arbitrary.

That lead to thinking that the gpiod_direction_output (and _input) is now doing more than implied by the name, and by the documentation for that matter. So perhaps it should be split out and promote gpio_set_bias to gpiod_set_bias? Anyway, that was the line of thought.
The problem there being some callers of gpiod_direction_input already expect it to set bias, at least on a best effort basis, and they would have to be updated to call gpiod_set_bias.

Maybe just update the documentation for exported functions that do set bias?

Quoted 5 lines

One more thing - since we all want this to make it for v5.5 - can you make the set config patches part of this series (simply bunch it all together)? This will make it easy to review and merge everything.

May as well - I've got it all in the one branch anyway.

Quoted 1 line

Thanks in advance and great job!

I was about to start updating libgpiod to add set_config, after adding the equivalent to my gpiod library (nearly finished writing the tests for that), but I'll put all that on the back burner and get v5, including in the set_config patches, out as soon as I can.

Cheers,
Kent.

Quoted 18 lines

Bartosz

Quoted 16 lines

I've also made a couple of minor changes myself while reviewing v4 - reordering the patches to group the gpiolib.c ones and leaving the gpio-mockup til last, and removing the "bias requires input mode" check from lineevent_create as the line is assumed to be input for events regardless of the input flag - there is no such thing as as-is for event requests.
Only mentioning here in case such changes are clearly wrong...

Cheers,
Kent.

Quoted 5 lines

It looks pretty much as I imagined it when I discussed it with Drew some while back, with some gritty details fixed up.

Yours,
Linus Walleij

pon., 4 lis 2019 o 12:12 Kent Gibson <[email protected]> napisał(a):

Quoted 36 lines · Bartosz Golaszewski

Quoted 33 lines

pon., 4 lis 2019 o 02:07 Kent Gibson <[email protected]> napisał(a):

Quoted 15 lines · Linus Walleij

Quoted 10 lines

Quoted 4 lines · Bartosz Golaszewski

[Kent]

Quoted 3 lines

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.

Right, excellent and persistent work here, much appreciated!

No problem - hopefully I haven't irritated too many people in the process.

Quoted 9 lines

Quoted 3 lines

As long as I get Bartosz's blanket ACK on v5 I think it is ready to merge. His consent is required for this.

I'm still waiting on open questions from v4 before submitting v5:

One, handling of errors when setting bias, Bart has referred to Thomas, so waiting for feedback on that.

If we can get it merged for v5.5, then I don't want to block it waiting for answers. Looking at the code I think we should only ignore the EOPNOTSUPP error and propagate all other codes. Can you add a patch changing that and then fix the other nits I pointed out? Also:
please Cc Thomas Petazzoni so that he gets the chance to yell at us if it breaks something.

Quoted 19 lines

Will do.

Quoted 11 lines

Quoted 6 lines

The other, where gpio_set_bias is hooked into gpiod_direction_output, is fine as is for the UAPI - it can always be relocated subsequently if other APIs need to set bias. On the other hand, if decoupling setting direction and bias is in order then that really should be done now. Can I get an an ACK on that either way?

This is in line with what you did for input. I don't think it should be decoupled (any particular reason for that? There is none mentioned in the cover letter), so I propose to leave it as it is in patch 5/5.

Wrt decoupling, I was thinking of both input and output, not just output, though it was the output that got me onto that line of thought as gpiod_direction_output sets bias, but gpiod_direction_output_raw doesn't. That seemed totally arbitrary.

Quoted 8 lines

That lead to thinking that the gpiod_direction_output (and _input) is now doing more than implied by the name, and by the documentation for that matter. So perhaps it should be split out and promote gpio_set_bias to gpiod_set_bias? Anyway, that was the line of thought.
The problem there being some callers of gpiod_direction_input already expect it to set bias, at least on a best effort basis, and they would have to be updated to call gpiod_set_bias.

I see. I think that in this case, the _raw variants should stay as simple as possible (hence the name) while the bias *should* be set in the regular gpiod_direction_intput()/output(). For now I don't think we need an exported gpiod_set_bias(), but if someone should request it in the future it will be straightforward to add.

Quoted 2 lines

Maybe just update the documentation for exported functions that do set bias?

Sure, sounds good. You can even extend the doc to include other flags these functions handle.

Quoted 10 lines

Quoted 5 lines

One more thing - since we all want this to make it for v5.5 - can you make the set config patches part of this series (simply bunch it all together)? This will make it easy to review and merge everything.

May as well - I've got it all in the one branch anyway.

Quoted 1 line

Thanks in advance and great job!

Quoted 5 lines

I was about to start updating libgpiod to add set_config, after adding the equivalent to my gpiod library (nearly finished writing the tests for that), but I'll put all that on the back burner and get v5, including in the set_config patches, out as soon as I can.

Let me know if you still want to do it and you'll have patches ready before v5.5 is released. Otherwise I can do it myself too if needed.

Quoted 37 lines

pon., 4 lis 2019 o 12:12 Kent Gibson <[email protected]> napisał(a):

Quoted 36 lines · Bartosz Golaszewski

Quoted 33 lines

pon., 4 lis 2019 o 02:07 Kent Gibson <[email protected]> napisał(a):

Quoted 15 lines · Linus Walleij

Quoted 10 lines

Quoted 4 lines · Bartosz Golaszewski

[Kent]

Quoted 3 lines

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.

Right, excellent and persistent work here, much appreciated!

No problem - hopefully I haven't irritated too many people in the process.

Quoted 9 lines

Quoted 3 lines

As long as I get Bartosz's blanket ACK on v5 I think it is ready to merge. His consent is required for this.

I'm still waiting on open questions from v4 before submitting v5:

One, handling of errors when setting bias, Bart has referred to Thomas, so waiting for feedback on that.

If we can get it merged for v5.5, then I don't want to block it waiting for answers. Looking at the code I think we should only ignore the EOPNOTSUPP error and propagate all other codes. Can you add a patch changing that and then fix the other nits I pointed out? Also:
please Cc Thomas Petazzoni so that he gets the chance to yell at us if it breaks something.

Can you just confirm if it is EOPNOTSUPP or ENOTSUPP that you want ignored?

Quoted 28 lines

Quoted 19 lines

Will do.

Quoted 11 lines

Quoted 6 lines

The other, where gpio_set_bias is hooked into gpiod_direction_output, is fine as is for the UAPI - it can always be relocated subsequently if other APIs need to set bias. On the other hand, if decoupling setting direction and bias is in order then that really should be done now. Can I get an an ACK on that either way?

This is in line with what you did for input. I don't think it should be decoupled (any particular reason for that? There is none mentioned in the cover letter), so I propose to leave it as it is in patch 5/5.

Wrt decoupling, I was thinking of both input and output, not just output, though it was the output that got me onto that line of thought as gpiod_direction_output sets bias, but gpiod_direction_output_raw doesn't. That seemed totally arbitrary.

Quoted 8 lines

That lead to thinking that the gpiod_direction_output (and _input) is now doing more than implied by the name, and by the documentation for that matter. So perhaps it should be split out and promote gpio_set_bias to gpiod_set_bias? Anyway, that was the line of thought.
The problem there being some callers of gpiod_direction_input already expect it to set bias, at least on a best effort basis, and they would have to be updated to call gpiod_set_bias.

Quoted 6 lines

I see. I think that in this case, the _raw variants should stay as simple as possible (hence the name) while the bias *should* be set in the regular gpiod_direction_intput()/output(). For now I don't think we need an exported gpiod_set_bias(), but if someone should request it in the future it will be straightforward to add.

OK.

Quoted 6 lines

Quoted 2 lines

Maybe just update the documentation for exported functions that do set bias?

Sure, sounds good. You can even extend the doc to include other flags these functions handle.

OK to add this later, or does it need to be in this series?

Quoted 16 lines

Quoted 15 lines

Quoted 5 lines

One more thing - since we all want this to make it for v5.5 - can you make the set config patches part of this series (simply bunch it all together)? This will make it easy to review and merge everything.

May as well - I've got it all in the one branch anyway.

Quoted 1 line

Thanks in advance and great job!

I was about to start updating libgpiod to add set_config, after adding the equivalent to my gpiod library (nearly finished writing the tests for that), but I'll put all that on the back burner and get v5, including in the set_config patches, out as soon as I can.

Quoted 3 lines

Let me know if you still want to do it and you'll have patches ready before v5.5 is released. Otherwise I can do it myself too if needed.

When does the window close for v5.5?
I've got an updated series ready - other than the doc updates mentioned above.

Cheers,
Kent.

Linus mentioned during the ELCE that this release may take 8 release candidates, so we should be good, but it would be great to have it in for-next by the end of this week. But I was actually referring to related libgpiod changes - in this case I'd like to do a release as soon as v5.5 is out which will be in around 3 months anyway, so we have time.