2008-11-12 06:57:22

by Jarkko Nikula

[permalink] [raw]
Subject: [patch 2.6.28-rc4] gpiolib: extend gpio label column width in debugfs file

There are already various drivers having bigger label than 12 bytes. Most
of them fit well under 20 bytes but make column width exact so that
oversized labels don't mess up output alignment.

Signed-off-by: Jarkko Nikula <[email protected]>
---
drivers/gpio/gpiolib.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index faa1cc6..82020ab 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1134,7 +1134,7 @@ static void gpiolib_dbg_show(struct seq_file *s, struct gpio_chip *chip)
continue;

is_out = test_bit(FLAG_IS_OUT, &gdesc->flags);
- seq_printf(s, " gpio-%-3d (%-12s) %s %s",
+ seq_printf(s, " gpio-%-3d (%-20.20s) %s %s",
gpio, gdesc->label,
is_out ? "out" : "in ",
chip->get
--
1.5.6.5


2008-11-14 01:02:57

by Andrew Morton

[permalink] [raw]
Subject: Re: [patch 2.6.28-rc4] gpiolib: extend gpio label column width in debugfs file

On Wed, 12 Nov 2008 08:56:13 +0200
Jarkko Nikula <[email protected]> wrote:

> There are already various drivers having bigger label than 12 bytes. Most
> of them fit well under 20 bytes but make column width exact so that
> oversized labels don't mess up output alignment.

Please provide before-and-after example output for this sort of change
so that we can better understand its effect.

> Signed-off-by: Jarkko Nikula <[email protected]>
> ---
> drivers/gpio/gpiolib.c | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index faa1cc6..82020ab 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -1134,7 +1134,7 @@ static void gpiolib_dbg_show(struct seq_file *s, struct gpio_chip *chip)
> continue;
>
> is_out = test_bit(FLAG_IS_OUT, &gdesc->flags);
> - seq_printf(s, " gpio-%-3d (%-12s) %s %s",
> + seq_printf(s, " gpio-%-3d (%-20.20s) %s %s",
> gpio, gdesc->label,
> is_out ? "out" : "in ",
> chip->get

This is a non-backward-compatible change to a userspace interface. It
looks like a pretty safe one, but this is always a fairly big deal.

If we're going to make this change then we should backport it to
2.6.27.x, 2.6.26.x and to 2.6.25.x to minimise the chance that someone
will write a parser which works on one kernel version and fails on
another.

David? Thoughts?

2008-11-14 01:13:49

by David Brownell

[permalink] [raw]
Subject: Re: [patch 2.6.28-rc4] gpiolib: extend gpio label column width in debugfs file

On Thursday 13 November 2008, Andrew Morton wrote:
> On Wed, 12 Nov 2008 08:56:13 +0200
> Jarkko Nikula <[email protected]> wrote:
>
> > There are already various drivers having bigger label than 12 bytes. Most
> > of them fit well under 20 bytes but make column width exact so that
> > oversized labels don't mess up output alignment.
>
> Please provide before-and-after example output for this sort of change
> so that we can better understand its effect.
>
> > Signed-off-by: Jarkko Nikula <[email protected]>

I meant to send:

Acked-by: David Brownell <[email protected]>

... thanks Jarkko.


> > ---
> > drivers/gpio/gpiolib.c | 2 +-
> > 1 files changed, 1 insertions(+), 1 deletions(-)
> >
> > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> > index faa1cc6..82020ab 100644
> > --- a/drivers/gpio/gpiolib.c
> > +++ b/drivers/gpio/gpiolib.c
> > @@ -1134,7 +1134,7 @@ static void gpiolib_dbg_show(struct seq_file *s, struct gpio_chip *chip)
> > continue;
> >
> > is_out = test_bit(FLAG_IS_OUT, &gdesc->flags);
> > - seq_printf(s, " gpio-%-3d (%-12s) %s %s",
> > + seq_printf(s, " gpio-%-3d (%-20.20s) %s %s",
> > gpio, gdesc->label,
> > is_out ? "out" : "in ",
> > chip->get
>
> This is a non-backward-compatible change to a userspace interface. It
> looks like a pretty safe one, but this is always a fairly big deal.

Not really; this is for debugfs, and formats for debug data
are not ABI-suitable. Developers need the flexibility to
change that stuff ... for example, they can *already* override
the default format to show pullups/pulldowns, drive strength,
polarity inversion, debouncing, and other hardware-specific goodies.


> If we're going to make this change then we should backport it to
> 2.6.27.x, 2.6.26.x and to 2.6.25.x to minimise the chance that someone
> will write a parser which works on one kernel version and fails on
> another.
>
> David? Thoughts?

This is explicitly not supported for ABI-ish purposes.

If they aren't even using those parenthesis to delimit things, I'd
really need to question what happened to their grey cells!

- Dave



2008-11-14 01:18:36

by Andrew Morton

[permalink] [raw]
Subject: Re: [patch 2.6.28-rc4] gpiolib: extend gpio label column width in debugfs file

On Thu, 13 Nov 2008 17:13:38 -0800
David Brownell <[email protected]> wrote:

> On Thursday 13 November 2008, Andrew Morton wrote:
> > On Wed, 12 Nov 2008 08:56:13 +0200
> > Jarkko Nikula <[email protected]> wrote:
> >
> > > There are already various drivers having bigger label than 12 bytes. Most
> > > of them fit well under 20 bytes but make column width exact so that
> > > oversized labels don't mess up output alignment.
> >
> > Please provide before-and-after example output for this sort of change
> > so that we can better understand its effect.
> >
> > > Signed-off-by: Jarkko Nikula <[email protected]>
>
> I meant to send:
>
> Acked-by: David Brownell <[email protected]>
>
> ... thanks Jarkko.
>

OK.

> > > ---
> > > drivers/gpio/gpiolib.c | 2 +-
> > > 1 files changed, 1 insertions(+), 1 deletions(-)
> > >
> > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> > > index faa1cc6..82020ab 100644
> > > --- a/drivers/gpio/gpiolib.c
> > > +++ b/drivers/gpio/gpiolib.c
> > > @@ -1134,7 +1134,7 @@ static void gpiolib_dbg_show(struct seq_file *s, struct gpio_chip *chip)
> > > continue;
> > >
> > > is_out = test_bit(FLAG_IS_OUT, &gdesc->flags);
> > > - seq_printf(s, " gpio-%-3d (%-12s) %s %s",
> > > + seq_printf(s, " gpio-%-3d (%-20.20s) %s %s",
> > > gpio, gdesc->label,
> > > is_out ? "out" : "in ",
> > > chip->get
> >
> > This is a non-backward-compatible change to a userspace interface. It
> > looks like a pretty safe one, but this is always a fairly big deal.
>
> Not really; this is for debugfs, and formats for debug data
> are not ABI-suitable. Developers need the flexibility to
> change that stuff ...

Well that's what we like to say, but it's delusional. If we add stuff
then later change it, other people's stuff will break. Just as with sysfs,
procfs, syscalls, etc.

The only difference is that with debugfs we have this weaselly excuse to
get people off our backs.

> for example, they can *already* override
> the default format to show pullups/pulldowns, drive strength,
> polarity inversion, debouncing, and other hardware-specific goodies.
>
>
> > If we're going to make this change then we should backport it to
> > 2.6.27.x, 2.6.26.x and to 2.6.25.x to minimise the chance that someone
> > will write a parser which works on one kernel version and fails on
> > another.
> >
> > David? Thoughts?
>
> This is explicitly not supported for ABI-ish purposes.
>
> If they aren't even using those parenthesis to delimit things, I'd
> really need to question what happened to their grey cells!

True.