Re: [PATCH 2/2] PCI: mvebu: switch to using gpiod API

[Kent Gibson <warthog618@gmail.com>]

On Wed, Sep 14, 2022 at 02:10:21PM +0200, Bartosz Golaszewski wrote:
> On Wed, Sep 14, 2022 at 12:35 PM Linus Walleij <linus.walleij@linaro.org> wrote:
> >
> > On Wed, Sep 7, 2022 at 12:41 AM Dmitry Torokhov
> > <dmitry.torokhov@gmail.com> wrote:
> >
> > > Linus, do you think we should introduce GPIOD_OUT_INACTIVE /
> > > GPIOD_OUT_ACTIVE or GPIOD_OUT_DEASSERTED / GPIOD_OUT_ASSERTED and
> > > deprecate existing GPIOD_OUT_LOW and GPIO_OUT_HIGH?
> >
> > They should rather be replaced everywhere in one go.
> >
> > I think it is just a half-measure unless we also add
> > #define GPIOD_ASSERTED 1
> > #define GPIOD_DEASSERTED 0
> > to be used instead of 1/0 in gpiod_set_value().
> >
> 
> We've had that discussion for libgpiod and went with
> GPIOD_VALUE_ACTIVE and GPIOD_VALUE_INACTIVE but...
> 
> > It would also imply changing the signature of the function
> > gpiod_set_value() to gpiod_set_state() as we are not
> > really setting a value but a state.
> >
> 

I might be in the minority here, but in this context value and state mean
the same thing to me, so changing from one to the other provides no
additional clarity and is at best pointless.

The key distinction is whether you are describing a physical or logical
value/state.
High/low should be reserved for physical.
Active/inactive describe logical.

(I personally dislike "deassert" as it is a manufactured word that feels
very awkward.)

So for gpiod_set_value(), which expects a logical value, you would use
ACTIVE/INACTIVE.  For gpiod_set_raw_value(), which expects a physical
value, you would use HIGH/LOW.

Given there are gpiod functions that expect both, there is a case for
both to pairings exist, and for the caller to use the appropriate one
for the call.

Changing gpiod_set_value() to gpiod_set_state(), while leaving
gpiod_set_raw_value() as is, does not reduce confusion - at least not for
me.

Cheers,
Kent.

> ... now that you've mentioned it it does seem like
> GPIOD_STATE_ACTIVE/INACTIVE makes much more sense as well as naming
> the relevant functions gpiod_line_request_set_line_state() etc.
> 
> > I have thought about changing this, but the problem is that I felt
> > it should be accompanied with a change fixing as many users
> > as possible.
> >
> > I think this is one of those occasions where we should merge
> > the new defines, and then send Linus Torvalds a sed script
> > that he can run at the end of the merge window to change all
> > gpiod_set_value(...., 1) -> gpiod_set_state(...., GPIOD_ASSERTED);
> > everywhere.
> >
> > After all users are changed, the GPIOD_ASSERTED/DEASSERTED
> > defined can be turned into an enum.
> >
> > That would be the silver bullet against a lot of confusion IMO.
> >
> > We would need Bartosz' input on this.
> >
> 
> We can also let Linus know that we'll do it ourselves late in the
> merge window and send a separate PR on Saturday before rc1?
> 
> Bart
> 
> > Yours,
> > Linus Walleij