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

[Bartosz Golaszewski <bgolaszewski@baylibre.com>]

pon., 4 lis 2019 o 15:22 Kent Gibson <warthog618@gmail.com> napisał(a):
>
> On Mon, Nov 04, 2019 at 12:48:33PM +0100, Bartosz Golaszewski wrote:
> > pon., 4 lis 2019 o 12:12 Kent Gibson <warthog618@gmail.com> 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@gmail.com> 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@baylibre.com> 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.
> > > >
> > >
>
> Can you just confirm if it is EOPNOTSUPP or ENOTSUPP that you want ignored?
>

Oops, it's -ENOTSUPP of course, the error code returned from
gpio_set_config() by this line:

  return gc->set_config ? gc->set_config(gc, offset, config) : -ENOTSUPP;

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

Yeah, we can do it later.

> > >
> > > > 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.
> >
>
> When does the window close for v5.5?
> I've got an updated series ready - other than the doc updates mentioned
> above.
>

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.

Bart

> Cheers,
> Kent.