Re: [PATCH v6 0/7] gpio: expose line bias flags to userspace

[Kent Gibson <warthog618@gmail.com>]

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.