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

[Kent Gibson]

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@xxxxxxxxxx> wrote:
> >
> > On Wed, Sep 7, 2022 at 12:41 AM Dmitry Torokhov
> > <dmitry.torokhov@xxxxxxxxx> 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.
> >
> 
> ... 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.
> 

After sleeping on this, I'm even more in disagreement with renaming
value to state.

AIUI, the confusion here is distinguishing between the physical and
logical cases.  libgpiod doesn't have this problem, as it only deals
with logical.

By convention, gpiolib uses _raw in the function name when referring to
physical, and otherwise deals with logical. e.g. gpiod_set_value() and
gpiod_set_raw_value().  Changing "value" to "state" is orthogonal to the
actual problem.

Further, "state" is a more broad term than "value", i.e. state may
include a number of attributes, whereas value indicates an individial
attribute.

For lines, state and value are typically synonymous, as the line level
(just to throw in another term for it) is what usually springs to
mind when considering a line.  But a line's state could also be
interpreted to include direction, bias, drive,...
You may argue that those form the configuration state, and I would agree
with you - the "configuration" indicating a subset of the overall line
state. i.e. "state" is a broad term.

If you are trying to reduce confusion, switching from a more specific
to a more broad term is going in the wrong direction.

OTOH, I totally agree with the addition of GPIOD_ACTIVE/INACTIVE to be
used for the logical cases, and even a script to apply it globally.
Ideally that script would change both the calls to the logical functions
to use ACTIVE/INACTIVE, and the physical to HIGH/LOW.

Introducing enums for those, and changing the function signatures to
use those rather than int makes sense to me too.  Though I'm not sure
why that has to wait until after all users are changed to the new macros.
Would that generate lint warnings?

Cheers,
Kent.

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