[alsa-devel] [PATCH 00/62] Add definition for GPIO direction

[Vaittinen, Matti]

On Tue, 2019-11-05 at 15:11 +0200, andriy.shevchenko at linux.intel.com
wrote:
> On Tue, Nov 05, 2019 at 12:54:55PM +0000, Vaittinen, Matti wrote:
> > On Tue, 2019-11-05 at 14:20 +0200, Andy Shevchenko wrote:
> > > On Tue, Nov 05, 2019 at 12:09:10PM +0200, Matti Vaittinen wrote:
> > > > The patch series adds definitions for GPIO line directions.
> > > > 
> > > > For occasional GPIO contributor like me it is always a pain to
> > > > remember
> > > > whether 1 or 0 was used for GPIO direction INPUT/OUTPUT.
> > > > Judging
> > > > the
> > > > fact that I removed few comments like:
> > > > 
> > > > /* Return 0 if output, 1 if input */
> > > > /* This means "out" */
> > > > return 1; /* input */
> > > > return 0; /* output */
> > > > 
> > > > it seems at least some others may find it hard to remember too.
> > > > Adding
> > > > defines for these values helps us who really have good - but
> > > > short
> > > > duration - memory :]
> > > > 
> > > > Please also see the last patch. It adds warning prints to be
> > > > emitted
> > > > from gpiolib if other than defined values are used. This patch
> > > > can
> > > > be
> > > > dropped out if there is a reason for that to still be allowed.
> > > > 
> > > > This idea comes from RFC series for ROHM BD71828 PMIC and was
> > > > initially
> > > > discussed with Linus Walleij here:
> > > > https://lore.kernel.org/lkml/c06725c3dd34118a324907137758d8b85b3d4043.camel@fi.rohmeurope.com/
> > > > but as this has no dependencies to BD71828 work (which probably
> > > > takes a
> > > > while) I decided to make it independent series.
> > > > 
> > > > Patches are compile-tested only. I have no HW to really test
> > > > them.
> > > > Thus I'd
> > > > appreciate carefull review. This work is mainly about
> > > > converting
> > > > zeros
> > > > and ones to the new defines but it wouldn't be first time I get
> > > > it
> > > > wrong
> > > > in one of the patches :)
> > > 
> > > For all patches I have been Cc'ed to, NAK.
> > > 
> > > I don't see the advantages now since all known hardware uses the
> > > single bit to
> > > describe direction (even for Intel where we have separate gating
> > > for
> > > in and out
> > > buffers the direction we basically rely on the bit that enables
> > > out
> > > buffer).
> > > 
> > > So, that said the direction is always 1 bit and basically 0/1
> > > value. 
> > 
> > Yes. At least for now. And this patch didn't change that although
> > it
> > makes it possible to do so if required.
> > 
> > > Which one
> > > is in and which one is out just a matter of an agreement we did.
> > 
> > This one is exactly the problem patch series was created for. Which
> > one
> > is IN and which is OUT is indeed a matter of an agreement - but
> > this
> > agreement should be clearly visible, well defined and same for all.
> > 
> > It's *annoying* to try finding out whether it was 1 or 0 one should
> > return from get_direction callback for OUTPUT. This is probably the
> > reason we have comments like
> > 
> > return 1; /* input */
> > 
> > there.
> > 
> > As this is global agreement for all GPIO framework users - not
> > something that can be agreed per driver basis - we should have GPIO
> > framework wide definitions for this. We can just add definitions
> > for
> > new drivers to benefit - but I think adding them also for existing
> > drivers improves readability significantly (see below).
> > 
> > > I would also like to see bloat-o-meter statistics before and
> > > after
> > > your patch.
> > > My guts tell me that the result will be not in the favour of
> > > yours
> > > solution.
> > 
> > Can you please tell me what type of stats you hope to see? 
> 
> bloat-o-meter. It's a script that compares two object files to see
> which
> functions got fatter, and which are slimmer. You may find it in the
> kernel tree
> sources (scripts/ folder).

Right. Uwe explained that to me.

> > I can try
> > generating what you are after. The cover letter contained typical
> > +/-
> > change stats from git and summary:
> > 
> > 62 files changed, 228 insertions(+), 104 deletions(-)
> > 
> > It sure shows that amount of lines was increased (roughly 2 lines
> > more
> > / changed file)
> 
> Which is making unneeded churn.

Not unneeded. A few of us see the value of naming the 1 and 0.

> > - but I guess that was expected as I don't like
> > ternary.
> 
> And I like them, so, what to do?

Well, if you maintain those files and like ternary, then they can be
ternary. I don't really care as long as I don't need to be the one
maintaining them. Not really a battle I want to invest in. I can even
go and drop the patches for files you are maintaining if you see that's
the way to go. It's easier for me.

As I said, I don't plan to change the meaning of 1 and 0 - although I
thought that allowing it might help in the future. Main goal was to
name the 1 and 0. Naming can be done even if all users were not
converted to use the names - as long as values behind names are not
changed. Changing the values is a burden that can be carried by others
when it is needed - we can now just make it easier or harder.

So as to what to do - please just give me the final decision so that I
know if I should just drop the intel patches or use the ternary.
Unfortunately I don't right now have the time to waste arguing over it
;)

Br,
	Matti