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

[Bartosz Golaszewski <bgolaszewski@xxxxxxxxxxxx>]

pon., 4 lis 2019 o 12:12 Kent Gibson <warthog618@xxxxxxxxx> napisał(a):
>
> On Mon, Nov 04, 2019 at 11:14:56AM +0100, Bartosz Golaszewski wrote:
> > pon., 4 lis 2019 o 02:07 Kent Gibson <warthog618@xxxxxxxxx> napisał(a):
> > >
> > > On Mon, Nov 04, 2019 at 01:26:54AM +0100, Linus Walleij wrote:
> > > > On Thu, Oct 31, 2019 at 8:10 AM Bartosz Golaszewski
> > > > <bgolaszewski@xxxxxxxxxxxx> wrote:
> > > >
> > > > > [Kent]
> > > > > > 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.
> > >
> > > > 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.
> >
>
> Will do.
>
> > > 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.
>

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.

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

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

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.

Bart

> Cheers,
> Kent.
>
> > Bartosz
> >
> > > 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.
> > >
> > > > 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