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

[Bartosz Golaszewski <bgolaszewski@xxxxxxxxxxxx>]

wt., 5 lis 2019 o 17:24 Bartosz Golaszewski
<bgolaszewski@xxxxxxxxxxxx> napisał(a):
>
> wt., 5 lis 2019 o 16:26 Kent Gibson <warthog618@xxxxxxxxx> 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.