Re: [PATCH v4 0/5] gpio: expose line bias flags to userspace

[Bartosz Golaszewski <bgolaszewski@xxxxxxxxxxxx>]

pon., 28 paź 2019 o 08:37 Kent Gibson <warthog618@xxxxxxxxx> napisał(a):
>
> 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?

Thomas: is there any reason not to check the return value of these
calls for errors other than -EOPNOTSUPP?

>
> 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

I think this starts to look good. There are some nits and I'd like to
also hear an opinion from Linus on that, but I personally like that
and think it'll be useful.

Bart