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
