mbox series

[00/62] Add definition for GPIO direction

Message ID cover.1572875541.git.matti.vaittinen@fi.rohmeurope.com (mailing list archive)
Headers show
Series Add definition for GPIO direction | expand

Message

Vaittinen, Matti Nov. 5, 2019, 10:09 a.m. UTC
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 :)

Patch 1:
 - adds the defines
Patches 2 - 61:
 - convert drivers to use new defines
Patch 62:
 - Add warning print if values other than the defines is used for direction.

Patches are created on top of Linux v5.4-rc6.

--

Matti Vaittinen (62):
  gpio: Add definition for GPIO direction
  gpio: gpio-104-dio-48e: Use new GPIO_LINE_DIRECTION
  gpio: add gpio-104-idi-48: Use new GPIO_LINE_DIRECTION
  gpio: gpio-104-idio-16: Use new GPIO_LINE_DIRECTION
  gpio: gpio-74xx-mmio: Use new GPIO_LINE_DIRECTION
  gpio: gpio-amd-fch: Use new GPIO_LINE_DIRECTION
  gpio: gpio-aspeed: Use new GPIO_LINE_DIRECTION
  gpio: gpio-bcm-kona: Use new GPIO_LINE_DIRECTION
  gpio: gpio-bd70528: Use new GPIO_LINE_DIRECTION
  gpio: gpio-bd9571mwv: Use new GPIO_LINE_DIRECTION
  gpio: gpio-dln2: Use new GPIO_LINE_DIRECTION
  gpio: gpio-exar: Use new GPIO_LINE_DIRECTION
  gpio: gpio-f7188x: Use new GPIO_LINE_DIRECTION
  gpio: gpio-gpio-mm: Use new GPIO_LINE_DIRECTION
  gpio: gpio-htc-egpio: Use new GPIO_LINE_DIRECTION
  gpio: gpio-ich: Use new GPIO_LINE_DIRECTION
  gpio: gpio-kempld: Use new GPIO_LINE_DIRECTION
  gpio: gpio-lp873x: Use new GPIO_LINE_DIRECTION
  gpio: gpio-lp87565: Use new GPIO_LINE_DIRECTION
  gpio: gpio-madera: Use new GPIO_LINE_DIRECTION
  gpio: gpio-max3191x: Use new GPIO_LINE_DIRECTION
  gpio: gpio-mmio: Use new GPIO_LINE_DIRECTION
  gpio: gpio-merrifield: Use new GPIO_LINE_DIRECTION
  gpio: gpio-mockup: Use new GPIO_LINE_DIRECTION
  gpio: gpio-moxtet: Use new GPIO_LINE_DIRECTION
  gpio: gpio-mvebu: Use new GPIO_LINE_DIRECTION
  gpio: gpio-mxs: Use new GPIO_LINE_DIRECTION
  gpio: gpio-omap: Use new GPIO_LINE_DIRECTION
  gpio: gpio-pca953x: Use new GPIO_LINE_DIRECTION
  gpio: gpio-pcie-idio-24: Use new GPIO_LINE_DIRECTION
  gpio: gpio-pci-idio-16: Use new GPIO_LINE_DIRECTION
  gpio: gpio-pisosr: Use new GPIO_LINE_DIRECTION
  gpio: gpio-pl061: Use new GPIO_LINE_DIRECTION
  gpio: gpio-raspberrypi-exp: Use new GPIO_LINE_DIRECTION
  gpio: gpio-rcar: Use new GPIO_LINE_DIRECTION
  gpio: gpio-reg: Use new GPIO_LINE_DIRECTION
  gpio: gpio-sama5d2-piobu: Use new GPIO_LINE_DIRECTION
  gpio: gpio-sch311x: Use new GPIO_LINE_DIRECTION
  gpio: gpio-sch: Use new GPIO_LINE_DIRECTION
  gpio: gpio-siox: Use new GPIO_LINE_DIRECTION
  gpio: gpio-stmpe: Use new GPIO_LINE_DIRECTION
  gpio: gpio-tc3589x: Use new GPIO_LINE_DIRECTION
  gpio: gpio-tegra186: Use new GPIO_LINE_DIRECTION
  gpio: gpio-tegra: Use new GPIO_LINE_DIRECTION
  gpio: gpio-thunderx: Use new GPIO_LINE_DIRECTION
  gpio: gpio-tpic2810: Use new GPIO_LINE_DIRECTION
  gpio: gpio-tps65086: Use new GPIO_LINE_DIRECTION
  gpio: gpio-tps65912: Use new GPIO_LINE_DIRECTION
  gpio: gpio-tps68470: Use new GPIO_LINE_DIRECTION
  gpio: gpio-tqmx86: Use new GPIO_LINE_DIRECTION
  gpio: gpio-ts4900: Use new GPIO_LINE_DIRECTION
  gpio: gpio-twl4030: Use new GPIO_LINE_DIRECTION
  gpio: gpio-twl6040: Use new GPIO_LINE_DIRECTION
  gpio: gpio-uniphier: Use new GPIO_LINE_DIRECTION
  gpio: gpio-wcove: Use new GPIO_LINE_DIRECTION
  gpio: gpio-ws16c48: Use new GPIO_LINE_DIRECTION
  gpio: gpio-xgene: Use new GPIO_LINE_DIRECTION
  gpio: gpio-xra1403: Use new GPIO_LINE_DIRECTION
  gpio: gpio-xtensa: Use new GPIO_LINE_DIRECTION
  gpio: gpio-zynq: Use new GPIO_LINE_DIRECTION
  gpio: gpio-sa1100: Use new GPIO_LINE_DIRECTION
  gpiolib: Nag for INPUT direction values other than
    GPIO_LINE_DIRECTION_IN

 drivers/gpio/gpio-104-dio-48e.c     |  5 ++++-
 drivers/gpio/gpio-104-idi-48.c      |  2 +-
 drivers/gpio/gpio-104-idio-16.c     |  4 ++--
 drivers/gpio/gpio-74xx-mmio.c       |  5 ++++-
 drivers/gpio/gpio-amd-fch.c         |  2 +-
 drivers/gpio/gpio-aspeed.c          |  7 +++----
 drivers/gpio/gpio-bcm-kona.c        |  6 +++---
 drivers/gpio/gpio-bd70528.c         |  8 +++++---
 drivers/gpio/gpio-bd9571mwv.c       |  4 +++-
 drivers/gpio/gpio-dln2.c            |  6 +++---
 drivers/gpio/gpio-exar.c            |  5 ++++-
 drivers/gpio/gpio-f7188x.c          |  5 ++++-
 drivers/gpio/gpio-gpio-mm.c         |  5 ++++-
 drivers/gpio/gpio-htc-egpio.c       |  5 ++++-
 drivers/gpio/gpio-ich.c             |  5 ++++-
 drivers/gpio/gpio-kempld.c          |  5 ++++-
 drivers/gpio/gpio-lp873x.c          |  2 +-
 drivers/gpio/gpio-lp87565.c         |  5 ++++-
 drivers/gpio/gpio-madera.c          |  5 ++++-
 drivers/gpio/gpio-max3191x.c        |  2 +-
 drivers/gpio/gpio-merrifield.c      |  5 ++++-
 drivers/gpio/gpio-mmio.c            | 21 +++++++++++++++------
 drivers/gpio/gpio-mockup.c          | 11 +++--------
 drivers/gpio/gpio-moxtet.c          |  4 ++--
 drivers/gpio/gpio-mvebu.c           |  5 ++++-
 drivers/gpio/gpio-mxs.c             |  5 ++++-
 drivers/gpio/gpio-omap.c            |  6 ++++--
 drivers/gpio/gpio-pca953x.c         |  5 ++++-
 drivers/gpio/gpio-pci-idio-16.c     |  4 ++--
 drivers/gpio/gpio-pcie-idio-24.c    |  9 ++++++---
 drivers/gpio/gpio-pisosr.c          |  2 +-
 drivers/gpio/gpio-pl061.c           |  5 ++++-
 drivers/gpio/gpio-raspberrypi-exp.c |  5 ++++-
 drivers/gpio/gpio-rcar.c            |  5 ++++-
 drivers/gpio/gpio-reg.c             |  3 ++-
 drivers/gpio/gpio-sa1100.c          |  5 ++++-
 drivers/gpio/gpio-sama5d2-piobu.c   |  7 ++++---
 drivers/gpio/gpio-sch.c             |  5 ++++-
 drivers/gpio/gpio-sch311x.c         |  5 ++++-
 drivers/gpio/gpio-siox.c            |  4 ++--
 drivers/gpio/gpio-stmpe.c           |  5 ++++-
 drivers/gpio/gpio-tc3589x.c         |  5 ++++-
 drivers/gpio/gpio-tegra.c           |  5 ++++-
 drivers/gpio/gpio-tegra186.c        |  4 ++--
 drivers/gpio/gpio-thunderx.c        |  5 ++++-
 drivers/gpio/gpio-tpic2810.c        |  2 +-
 drivers/gpio/gpio-tps65086.c        |  2 +-
 drivers/gpio/gpio-tps65912.c        |  4 ++--
 drivers/gpio/gpio-tps68470.c        |  6 +++---
 drivers/gpio/gpio-tqmx86.c          |  5 ++++-
 drivers/gpio/gpio-ts4900.c          |  5 ++++-
 drivers/gpio/gpio-twl4030.c         | 10 +++++-----
 drivers/gpio/gpio-twl6040.c         |  3 +--
 drivers/gpio/gpio-uniphier.c        |  5 ++++-
 drivers/gpio/gpio-wcove.c           |  7 +++++--
 drivers/gpio/gpio-ws16c48.c         |  5 ++++-
 drivers/gpio/gpio-xgene.c           |  5 ++++-
 drivers/gpio/gpio-xra1403.c         |  5 ++++-
 drivers/gpio/gpio-xtensa.c          |  4 ++--
 drivers/gpio/gpio-zynq.c            |  7 +++++--
 drivers/gpio/gpiolib.c              | 16 ++++++++++++++--
 include/linux/gpio/driver.h         |  3 +++
 62 files changed, 228 insertions(+), 104 deletions(-)

Comments

Vaittinen, Matti Nov. 5, 2019, 10:54 a.m. UTC | #1
On Tue, 2019-11-05 at 12:09 +0200, Matti Vaittinen wrote:
> The patch series adds definitions for GPIO line directions.

Argh. Sorry peeps. My new cool patch script messed up the message-id
header causing the messages to not be nicely grouped in one thread.
Patches are now all scattered around your (in)boxes :/

I won't resend the series unless requested though - sending 62 mails is
not fun - receiving them may be even less fun... 

Sorry once more
--Matti
Andy Shevchenko Nov. 5, 2019, 12:20 p.m. UTC | #2
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. Which one
is in and which one is out just a matter of an agreement we did.

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.
Vaittinen, Matti Nov. 5, 2019, 12:54 p.m. UTC | #3
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
Uwe Kleine-König Nov. 5, 2019, 1:10 p.m. UTC | #4
Hello,

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:
> > 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(-)

I guess he wants to see

	scripts/bloat-o-meter vmlinuz.old vmlinuz

. I would expect a 0 there. I didn't look in detail, but in general I
like the idea to give 0 and 1 a symbolic name.

Best regards
Uwe
Andy Shevchenko Nov. 5, 2019, 1:11 p.m. UTC | #5
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).

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

> - but I guess that was expected as I don't like
> ternary.

And I like them, so, what to do?
Vaittinen, Matti Nov. 5, 2019, 1:30 p.m. UTC | #6
On Tue, 2019-11-05 at 14:10 +0100, Uwe Kleine-König wrote:
> Hello,
> 
> 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:
> > > 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(-)
> 
> I guess he wants to see
> 
> 	scripts/bloat-o-meter vmlinuz.old vmlinuz
> 
> . I would expect a 0 there. I didn't look in detail, but in general I
> like the idea to give 0 and 1 a symbolic name.

Thanks Uwe. This far I have only cross-compiled the series for arm
which I use for developing the ROHM PMICs. scripts/bloat-o-meter /
tools it uses does not seem to recognize the image format (not a big
surprize as my host is x86_64).

Br,
	Matti Vaittinen
Andy Shevchenko Nov. 5, 2019, 1:30 p.m. UTC | #7
On Tue, Nov 05, 2019 at 02:10:38PM +0100, Uwe Kleine-König 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:
> > > 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(-)
> 
> I guess he wants to see
> 
> 	scripts/bloat-o-meter vmlinuz.old vmlinuz

Yes, but be sure you have compiled them all and build them all in.
Otherwise you might get wrong result.

> . I would expect a 0 there. I didn't look in detail, but in general I
> like the idea to give 0 and 1 a symbolic name.

I'll will be fine with that if and only if maintainers are okay. For now,
I don't like the idea to trade bad for worse.
Uwe Kleine-König Nov. 5, 2019, 1:36 p.m. UTC | #8
On Tue, Nov 05, 2019 at 01:30:20PM +0000, Vaittinen, Matti wrote:
> On Tue, 2019-11-05 at 14:10 +0100, Uwe Kleine-König wrote:
> > Hello,
> > 
> > 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:
> > > > 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(-)
> > 
> > I guess he wants to see
> > 
> > 	scripts/bloat-o-meter vmlinuz.old vmlinuz
> > 
> > . I would expect a 0 there. I didn't look in detail, but in general I
> > like the idea to give 0 and 1 a symbolic name.
> 
> Thanks Uwe. This far I have only cross-compiled the series for arm
> which I use for developing the ROHM PMICs. scripts/bloat-o-meter /
> tools it uses does not seem to recognize the image format (not a big
> surprize as my host is x86_64).

It works for me, I guess that's because my binutils support several
architectures:

	$ nm --help
	...
	nm: supported targets: elf64-x86-64 elf32-i386 elf32-iamcu elf32-x86-64
	pei-i386 pei-x86-64 elf64-l1om elf64-k1om elf64-little elf64-big
	elf32-little elf32-big elf64-littleaarch64 elf64-bigaarch64
	elf32-littleaarch64 elf32-bigaarch64 elf32-littlearm elf32-bigarm
	elf64-alpha ecoff-littlealpha elf32-littlearm-fdpic elf32-bigarm-fdpic
	elf32-hppa-linux elf32-hppa elf64-ia64-little elf64-ia64-big pei-ia64
	elf32-m32r-linux elf32-m32rle-linux elf32-m68k elf32-tradbigmips
	elf32-tradlittlemips ecoff-bigmips ecoff-littlemips elf32-ntradbigmips
	elf64-tradbigmips elf32-ntradlittlemips elf64-tradlittlemips
	elf32-powerpc aixcoff-rs6000 elf32-powerpcle ppcboot elf64-powerpc
	elf64-powerpcle aixcoff64-rs6000 aix5coff64-rs6000 elf64-littleriscv
	elf32-littleriscv elf32-s390 elf64-s390 elf32-sh-linux elf32-shbig-linux
	elf32-sh-fdpic elf32-shbig-fdpic elf32-sparc elf64-sparc pe-x86-64
	pe-bigobj-x86-64 pe-i386 plugin srec symbolsrec verilog tekhex binary
	ihex

(added line breaks for easier reading). I got this by installing
binutils-multiarch (on Debian).

Best regards
Uwe
Uwe Kleine-König Nov. 5, 2019, 1:40 p.m. UTC | #9
On Tue, Nov 05, 2019 at 03:30:25PM +0200, andriy.shevchenko@linux.intel.com wrote:
> On Tue, Nov 05, 2019 at 02:10:38PM +0100, Uwe Kleine-König 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:
> > > > 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(-)
> > 
> > I guess he wants to see
> > 
> > 	scripts/bloat-o-meter vmlinuz.old vmlinuz
> 
> Yes, but be sure you have compiled them all and build them all in.
> Otherwise you might get wrong result.
> 
> > . I would expect a 0 there. I didn't look in detail, but in general I
> > like the idea to give 0 and 1 a symbolic name.
> 
> I'll will be fine with that if and only if maintainers are okay. For now,
> I don't like the idea to trade bad for worse.

I don't see you concern.

	int somefunction(...)
	{
		return 1;
	}

is definitively worse than

	int somefunction(...)
	{
		return GPIO_LINE_DIRECTION_IN;
	}

and after cpp had its go on the source the compiler sees the exact same
thing, so I don't expect any size changes. The only change is that to
write (or understand) the above code, you have to know that 1
corresponds to GPIO input (or was it output?) while in the later
function it is obvious that we're talking about an input.

Best regards
Uwe
Vaittinen, Matti Nov. 5, 2019, 1:54 p.m. UTC | #10
On Tue, 2019-11-05 at 15:11 +0200, andriy.shevchenko@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
Vaittinen, Matti Nov. 5, 2019, 2 p.m. UTC | #11
On Tue, 2019-11-05 at 14:36 +0100, Uwe Kleine-König wrote:
> On Tue, Nov 05, 2019 at 01:30:20PM +0000, Vaittinen, Matti wrote:
> > On Tue, 2019-11-05 at 14:10 +0100, Uwe Kleine-König wrote:
> > > Hello,
> > > 
> > > 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:
> > > > > 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(-)
> > > 
> > > I guess he wants to see
> > > 
> > > 	scripts/bloat-o-meter vmlinuz.old vmlinuz
> > > 
> > > . I would expect a 0 there. I didn't look in detail, but in
> > > general I
> > > like the idea to give 0 and 1 a symbolic name.
> > 
> > Thanks Uwe. This far I have only cross-compiled the series for arm
> > which I use for developing the ROHM PMICs. scripts/bloat-o-meter /
> > tools it uses does not seem to recognize the image format (not a
> > big
> > surprize as my host is x86_64).
> 
> It works for me, I guess that's because my binutils support several
> architectures:
> 
> 	$ nm --help
> 	...
> 	nm: supported targets: elf64-x86-64 elf32-i386 elf32-iamcu
> elf32-x86-64
> 	pei-i386 pei-x86-64 elf64-l1om elf64-k1om elf64-little elf64-
> big
> 	elf32-little elf32-big elf64-littleaarch64 elf64-bigaarch64
> 	elf32-littleaarch64 elf32-bigaarch64 elf32-littlearm elf32-
> bigarm
> 	elf64-alpha ecoff-littlealpha elf32-littlearm-fdpic elf32-
> bigarm-fdpic
> 	elf32-hppa-linux elf32-hppa elf64-ia64-little elf64-ia64-big
> pei-ia64
> 	elf32-m32r-linux elf32-m32rle-linux elf32-m68k elf32-
> tradbigmips
> 	elf32-tradlittlemips ecoff-bigmips ecoff-littlemips elf32-
> ntradbigmips
> 	elf64-tradbigmips elf32-ntradlittlemips elf64-tradlittlemips
> 	elf32-powerpc aixcoff-rs6000 elf32-powerpcle ppcboot elf64-
> powerpc
> 	elf64-powerpcle aixcoff64-rs6000 aix5coff64-rs6000 elf64-
> littleriscv
> 	elf32-littleriscv elf32-s390 elf64-s390 elf32-sh-linux elf32-
> shbig-linux
> 	elf32-sh-fdpic elf32-shbig-fdpic elf32-sparc elf64-sparc pe-
> x86-64
> 	pe-bigobj-x86-64 pe-i386 plugin srec symbolsrec verilog tekhex
> binary
> 	ihex
> 
> (added line breaks for easier reading). I got this by installing
> binutils-multiarch (on Debian).

Thanks Uwe! That was kind! I'm on Fedora but I guess I can find the
multiarch binutils :) I'll try that tomorrow when I'm back at the
office. Let's see what kind of results I can get from it.

Unfortunately bunch of the GPIOs depend on x86 - so I need to see what
I can compile in with decent effort. For my compile test I just hacked
the Makefile to force all in and added some dummy macros to fix few
missing functions :| But I guess I can get some results.

> 
> Best regards
> Uwe
>
Andy Shevchenko Nov. 5, 2019, 2:28 p.m. UTC | #12
On Tue, Nov 05, 2019 at 02:40:26PM +0100, Uwe Kleine-König wrote:
> On Tue, Nov 05, 2019 at 03:30:25PM +0200, andriy.shevchenko@linux.intel.com wrote:
> > On Tue, Nov 05, 2019 at 02:10:38PM +0100, Uwe Kleine-König 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:
> > > > > 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(-)
> > > 
> > > I guess he wants to see
> > > 
> > > 	scripts/bloat-o-meter vmlinuz.old vmlinuz
> > 
> > Yes, but be sure you have compiled them all and build them all in.
> > Otherwise you might get wrong result.
> > 
> > > . I would expect a 0 there. I didn't look in detail, but in general I
> > > like the idea to give 0 and 1 a symbolic name.
> > 
> > I'll will be fine with that if and only if maintainers are okay. For now,
> > I don't like the idea to trade bad for worse.
> 
> I don't see you concern.
> 
> 	int somefunction(...)
> 	{
> 		return 1;
> 	}
> 
> is definitively worse than
> 
> 	int somefunction(...)
> 	{
> 		return GPIO_LINE_DIRECTION_IN;
> 	}
> 
> and after cpp had its go on the source the compiler sees the exact same
> thing, so I don't expect any size changes. The only change is that to
> write (or understand) the above code, you have to know that 1
> corresponds to GPIO input (or was it output?) while in the later
> function it is obvious that we're talking about an input.

In almost all cases I'm involved in the original code is different:

	return !!foo();

vs.

	if (foo())
		return A;
	return B;
Andy Shevchenko Nov. 5, 2019, 2:32 p.m. UTC | #13
On Tue, Nov 05, 2019 at 01:54:27PM +0000, Vaittinen, Matti wrote:
> On Tue, 2019-11-05 at 15:11 +0200, andriy.shevchenko@linux.intel.com
> wrote:

> Unfortunately I don't right now have the time to waste arguing over it
> ;)

Neither do I.

P.S. I have already wasted enough on this discussion, as I said before I'm fine
with the change as long as maintainers of the subsystem are fine.

That's my last message on the topic till then.
Uwe Kleine-König Nov. 5, 2019, 2:59 p.m. UTC | #14
Hello Matti,

On Tue, Nov 05, 2019 at 02:00:02PM +0000, Vaittinen, Matti wrote:
> On Tue, 2019-11-05 at 14:36 +0100, Uwe Kleine-König wrote:
> Thanks Uwe! That was kind! I'm on Fedora but I guess I can find the
> multiarch binutils :) I'll try that tomorrow when I'm back at the
> office. Let's see what kind of results I can get from it.
> 
> Unfortunately bunch of the GPIOs depend on x86 - so I need to see what
> I can compile in with decent effort. For my compile test I just hacked
> the Makefile to force all in and added some dummy macros to fix few
> missing functions :| But I guess I can get some results.

Enable CONFIG_COMPILE_TEST and then you should be able to compile most
drivers also on the wrong architecture.

Best regards
Uwe
Vaittinen, Matti Nov. 5, 2019, 3:06 p.m. UTC | #15
On Tue, 2019-11-05 at 15:59 +0100, Uwe Kleine-König wrote:
> Hello Matti,
> 
> On Tue, Nov 05, 2019 at 02:00:02PM +0000, Vaittinen, Matti wrote:
> > On Tue, 2019-11-05 at 14:36 +0100, Uwe Kleine-König wrote:
> > Thanks Uwe! That was kind! I'm on Fedora but I guess I can find the
> > multiarch binutils :) I'll try that tomorrow when I'm back at the
> > office. Let's see what kind of results I can get from it.
> > 
> > Unfortunately bunch of the GPIOs depend on x86 - so I need to see
> > what
> > I can compile in with decent effort. For my compile test I just
> > hacked
> > the Makefile to force all in and added some dummy macros to fix few
> > missing functions :| But I guess I can get some results.
> 
> Enable CONFIG_COMPILE_TEST and then you should be able to compile
> most
> drivers also on the wrong architecture.

Right. That was the first thing I tried out. Unfortunately bunch of the
gpio-drivers won't care about CONFIG_COMPILE_TEST. Thanks anyways :)

Br,
	Matti
Andy Shevchenko Nov. 5, 2019, 3:17 p.m. UTC | #16
On Tue, Nov 05, 2019 at 03:59:46PM +0100, Uwe Kleine-König wrote:
> On Tue, Nov 05, 2019 at 02:00:02PM +0000, Vaittinen, Matti wrote:
> > On Tue, 2019-11-05 at 14:36 +0100, Uwe Kleine-König wrote:
> > Thanks Uwe! That was kind! I'm on Fedora but I guess I can find the
> > multiarch binutils :) I'll try that tomorrow when I'm back at the
> > office. Let's see what kind of results I can get from it.
> > 
> > Unfortunately bunch of the GPIOs depend on x86 - so I need to see what
> > I can compile in with decent effort. For my compile test I just hacked
> > the Makefile to force all in and added some dummy macros to fix few
> > missing functions :| But I guess I can get some results.
> 
> Enable CONFIG_COMPILE_TEST and then you should be able to compile most
> drivers also on the wrong architecture.

*Wrong* is a wrong word here. I guess you misspelled *another* / *not native*.