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

[Vaittinen, Matti]

Hello Andy,

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? 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) - but I guess that was expected as I don't like
ternary. So pretty much each

return !!foo;

was changed to

if (foo)
	return GPIO_LINE_DIRECTION_IN;

return GPIO_LINE_DIRECTION_OUT;

For me (and hopefully others who don't remember whether 1 is IN or OUT)
this is improved readability while bloat-factor is still pretty low.

From

return !!foo;

one does not know what the foo should be for IN or OUT without reading
some spec or browsing through comments. From latter it is obvious that
if foo is zero the direction is output.

Br,
	Matti