[U-Boot] [PATCH 1/2] GPIO: Extend the cmd_gpio API by gpio_{from, to}_string()

[U-Boot] [PATCH 1/2] GPIO: Extend the cmd_gpio API by gpio_{from, to}_string()

[Marek Vasut]

The gpio_from_string() call shall parse the incoming GPIO name taken
from the command line and return the GPIO number used within U-Boot or
return -1 on error.

The gpio_to_string() on the other hand allows nicer reporting of GPIO
name in the output of cmd_gpio.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Detlev Zundel <dzu@denx.de>
Cc: Mike Frysinger <vapier@gentoo.org>
Cc: Stefano Babic <sbabic@denx.de>
---
 common/cmd_gpio.c |   27 +++++++++++++++++++++++----
 1 file changed, 23 insertions(+), 4 deletions(-)

diff --git a/common/cmd_gpio.c b/common/cmd_gpio.c
index 47eee89..12870ef 100644
--- a/common/cmd_gpio.c
+++ b/common/cmd_gpio.c
@@ -15,6 +15,22 @@
 #define name_to_gpio(name) simple_strtoul(name, NULL, 10)
 #endif
 
+int __gpio_from_string(const char *name)
+{
+	return name_to_gpio(name);
+}
+
+int gpio_from_string(const char *name)
+	__attribute__((weak, alias("__gpio_from_string")));
+
+int __gpio_to_string(int gpio, char *buf, int buflen)
+{
+	return 0;
+}
+
+int gpio_to_string(int gpio, char *buf, int buflen)
+	__attribute__((weak, alias("__gpio_from_string")));
+
 enum gpio_cmd {
 	GPIO_INPUT,
 	GPIO_SET,
@@ -27,7 +43,9 @@ static int do_gpio(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 	int gpio;
 	enum gpio_cmd sub_cmd;
 	ulong value;
-	const char *str_cmd, *str_gpio;
+	const char *str_cmd;
+	char str_gpio[16] = { 0 };
+	int ret;
 
 #ifdef gpio_status
 	if (argc == 2 && !strcmp(argv[1], "status")) {
@@ -40,7 +58,6 @@ static int do_gpio(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
  show_usage:
 		return CMD_RET_USAGE;
 	str_cmd = argv[1];
-	str_gpio = argv[2];
 
 	/* parse the behavior */
 	switch (*str_cmd) {
@@ -52,7 +69,7 @@ static int do_gpio(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 	}
 
 	/* turn the gpio name into a gpio number */
-	gpio = name_to_gpio(str_gpio);
+	gpio = gpio_from_string(argv[2]);
 	if (gpio < 0)
 		goto show_usage;
 
@@ -75,8 +92,10 @@ static int do_gpio(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 		}
 		gpio_direction_output(gpio, value);
 	}
+
+	ret = gpio_to_string(gpio, str_gpio, sizeof(str_gpio));
 	printf("gpio: pin %s (gpio %i) value is %lu\n",
-		str_gpio, gpio, value);
+		ret ? str_gpio : argv[2], gpio, value);
 
 	gpio_free(gpio);
 
-- 
1.7.10

[U-Boot] [PATCH 2/2] GPIO: Implement gpio_{from, to}_string on M28EVK

[Marek Vasut]

Implement the above two functions. The format of input parameters is
"x_y", where x is the bank number and y is the pin number in that bank.

Example:
  => gpio i 3_13
  gpio: pin 3_13 (gpio 107) value is 0

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Detlev Zundel <dzu@denx.de>
Cc: Mike Frysinger <vapier@gentoo.org>
Cc: Stefano Babic <sbabic@denx.de>
---
 drivers/gpio/mxs_gpio.c |   32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/drivers/gpio/mxs_gpio.c b/drivers/gpio/mxs_gpio.c
index 38dbc81..988b105 100644
--- a/drivers/gpio/mxs_gpio.c
+++ b/drivers/gpio/mxs_gpio.c
@@ -130,3 +130,35 @@ int gpio_free(unsigned gpio)
 {
 	return 0;
 }
+
+int gpio_from_string(const char *cname)
+{
+	char *name = (char *)cname;
+	char *bank, *pin, *endp;
+	int b, p;
+
+	bank = strtok(name, "_");
+	if (!bank)
+		return -1;
+
+	pin = strtok(NULL, "_");
+	if (!pin)
+		return -1;
+
+	b = simple_strtol(bank, &endp, 10);
+	if (*endp != '\0')
+		return -1;
+
+	p = simple_strtol(pin, &endp, 10);
+	if (*endp != '\0')
+		return -1;
+
+	return MXS_IOMUX_PAD(b, p, 0, 0, 0, 0);
+}
+
+int gpio_to_string(int gpio, char *buf, int buflen)
+{
+	int ret;
+	ret = snprintf(buf, buflen, "%i_%i", PAD_BANK(gpio), PAD_PIN(gpio));
+	return ret < buflen;
+}
-- 
1.7.10

[U-Boot] [PATCH 1/2] GPIO: Extend the cmd_gpio API by gpio_{from, to}_string()

[Marek Vasut]

Hi,

> The gpio_from_string() call shall parse the incoming GPIO name taken
> from the command line and return the GPIO number used within U-Boot or
> return -1 on error.
> 
> The gpio_to_string() on the other hand allows nicer reporting of GPIO
> name in the output of cmd_gpio.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Detlev Zundel <dzu@denx.de>
> Cc: Mike Frysinger <vapier@gentoo.org>
> Cc: Stefano Babic <sbabic@denx.de>

Bump on these two patches please?

Best regards,
Marek Vasut

[U-Boot] [PATCH 1/2] GPIO: Extend the cmd_gpio API by gpio_{from, to}_string()

[Mike Frysinger]

On Tuesday 01 May 2012 16:50:14 Marek Vasut wrote:
> The gpio_from_string() call shall parse the incoming GPIO name taken
> from the command line and return the GPIO number used within U-Boot or
> return -1 on error.

i.e. the already existing name_to_gpio() func

> The gpio_to_string() on the other hand allows nicer reporting of GPIO
> name in the output of cmd_gpio.

i don't see the value in this.  we already have the string name from the user, 
so all you've implemented is:
	gpio_to_string(gpio_from_string(argv[]))
at which point, argv[] works fine.
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20120514/d8528d81/attachment.pgp>

[U-Boot] [PATCH 1/2] GPIO: Extend the cmd_gpio API by gpio_{from, to}_string()

[Marek Vasut]

Dear Mike Frysinger,

> On Tuesday 01 May 2012 16:50:14 Marek Vasut wrote:
> > The gpio_from_string() call shall parse the incoming GPIO name taken
> > from the command line and return the GPIO number used within U-Boot or
> > return -1 on error.
> 
> i.e. the already existing name_to_gpio() func

You mean blackfin specific macro crap? Let's actually drop that and switch over 
to this one. That was my intention from start.

> > The gpio_to_string() on the other hand allows nicer reporting of GPIO
> > name in the output of cmd_gpio.
> 
> i don't see the value in this.  we already have the string name from the
> user, so all you've implemented is:
> 	gpio_to_string(gpio_from_string(argv[]))
> at which point, argv[] works fine.

This allows for taking multiple different inputs, while reporting one unified 
output.

> -mike

Best regards,
Marek Vasut

[U-Boot] [PATCH 1/2] GPIO: Extend the cmd_gpio API by gpio_{from, to}_string()

[Mike Frysinger]

On Monday 14 May 2012 07:31:35 Marek Vasut wrote:
> Dear Mike Frysinger,
> > On Tuesday 01 May 2012 16:50:14 Marek Vasut wrote:
> > > The gpio_from_string() call shall parse the incoming GPIO name taken
> > > from the command line and return the GPIO number used within U-Boot or
> > > return -1 on error.
> > 
> > i.e. the already existing name_to_gpio() func
> 
> You mean blackfin specific macro crap? Let's actually drop that and switch
> over to this one. That was my intention from start.

don't spout this crap.  try reading the actual history on the lists for the 
background on the naming, and then try actually grepping the tree to see that 
multiple targets implement it.

> > > The gpio_to_string() on the other hand allows nicer reporting of GPIO
> > > name in the output of cmd_gpio.
> > 
> > i don't see the value in this.  we already have the string name from the
> > 
> > user, so all you've implemented is:
> > 	gpio_to_string(gpio_from_string(argv[]))
> > 
> > at which point, argv[] works fine.
> 
> This allows for taking multiple different inputs, while reporting one
> unified output.

sounds like unnecessary bloat
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20120514/d8140d68/attachment.pgp>

[U-Boot] [PATCH 1/2] GPIO: Extend the cmd_gpio API by gpio_{from, to}_string()

[Marek Vasut]

Dear Mike Frysinger,

> On Monday 14 May 2012 07:31:35 Marek Vasut wrote:
> > Dear Mike Frysinger,
> > 
> > > On Tuesday 01 May 2012 16:50:14 Marek Vasut wrote:
> > > > The gpio_from_string() call shall parse the incoming GPIO name taken
> > > > from the command line and return the GPIO number used within U-Boot
> > > > or return -1 on error.
> > > 
> > > i.e. the already existing name_to_gpio() func
> > 
> > You mean blackfin specific macro crap? Let's actually drop that and
> > switch over to this one. That was my intention from start.
> 
> don't spout this crap.  try reading the actual history on the lists for the
> background on the naming, and then try actually grepping the tree to see
> that multiple targets implement it.

Can you point me to something? This is the result of git grep, so I odn't see it 
used at all.

arch/blackfin/include/asm/gpio.h:static inline int name_to_gpio(const char 
*name)
arch/blackfin/include/asm/gpio.h:#define name_to_gpio(n) name_to_gpio(n)
common/cmd_gpio.c:#ifndef name_to_gpio
common/cmd_gpio.c:#define name_to_gpio(name) simple_strtoul(name, NULL, 10)
common/cmd_gpio.c:      return name_to_gpio(name);

> > > > The gpio_to_string() on the other hand allows nicer reporting of GPIO
> > > > name in the output of cmd_gpio.
> > > 
> > > i don't see the value in this.  we already have the string name from
> > > the
> > > 
> > > user, so all you've implemented is:
> > > 	gpio_to_string(gpio_from_string(argv[]))
> > > 
> > > at which point, argv[] works fine.
> > 
> > This allows for taking multiple different inputs, while reporting one
> > unified output.
> 
> sounds like unnecessary bloat

Now you're spouting crap, it makes sense to be able to report the name 
consistently.

> -mike

Best regards,
Marek Vasut

[U-Boot] [PATCH 1/2] GPIO: Extend the cmd_gpio API by gpio_{from, to}_string()

[Mike Frysinger]

On Monday 14 May 2012 14:25:14 Marek Vasut wrote:
> Dear Mike Frysinger,
> > On Monday 14 May 2012 07:31:35 Marek Vasut wrote:
> > > Dear Mike Frysinger,
> > > > On Tuesday 01 May 2012 16:50:14 Marek Vasut wrote:
> > > > > The gpio_from_string() call shall parse the incoming GPIO name
> > > > > taken from the command line and return the GPIO number used within
> > > > > U-Boot or return -1 on error.
> > > > 
> > > > i.e. the already existing name_to_gpio() func
> > > 
> > > You mean blackfin specific macro crap? Let's actually drop that and
> > > switch over to this one. That was my intention from start.
> > 
> > don't spout this crap.  try reading the actual history on the lists for
> > the background on the naming, and then try actually grepping the tree to
> > see that multiple targets implement it.
> 
> Can you point me to something? This is the result of git grep, so I odn't
> see it used at all.
> 
> arch/blackfin/include/asm/gpio.h:static inline int name_to_gpio(const char
> *name)
> arch/blackfin/include/asm/gpio.h:#define name_to_gpio(n) name_to_gpio(n)
> common/cmd_gpio.c:#ifndef name_to_gpio
> common/cmd_gpio.c:#define name_to_gpio(name) simple_strtoul(name, NULL, 10)
> common/cmd_gpio.c:      return name_to_gpio(name);

seems like the patches posted and i've been through haven't been merged.  
gpio_status() is the only one to gain wider support.

the naming style however has been discussed on list and wasn't pulled out of 
nowhere.  it follows the Linux GPIO API conventions whereas your proposed 
naming does not.

> > > > > The gpio_to_string() on the other hand allows nicer reporting of
> > > > > GPIO name in the output of cmd_gpio.
> > > > 
> > > > i don't see the value in this.  we already have the string name from
> > > > the
> > > > 
> > > > user, so all you've implemented is:
> > > > 	gpio_to_string(gpio_from_string(argv[]))
> > > > 
> > > > at which point, argv[] works fine.
> > > 
> > > This allows for taking multiple different inputs, while reporting one
> > > unified output.
> > 
> > sounds like unnecessary bloat
> 
> Now you're spouting crap, it makes sense to be able to report the name
> consistently.

except the output is for user's eyes only and doesn't make a lick of a 
difference to runtime behavior.  either they know the GPIO # (because they 
looked it up), or they're using the friendly name (which came from a 
datasheet).  in either case, things continue to work regardless of the input.  
the "value add" here doesn't justify the overhead imo.
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20120515/b8c872a8/attachment.pgp>

[U-Boot] [PATCH 1/2] GPIO: Extend the cmd_gpio API by gpio_{from, to}_string()

[Marek Vasut]

Dear Mike Frysinger,

> On Monday 14 May 2012 14:25:14 Marek Vasut wrote:
> > Dear Mike Frysinger,
> > 
> > > On Monday 14 May 2012 07:31:35 Marek Vasut wrote:
> > > > Dear Mike Frysinger,
> > > > 
> > > > > On Tuesday 01 May 2012 16:50:14 Marek Vasut wrote:
> > > > > > The gpio_from_string() call shall parse the incoming GPIO name
> > > > > > taken from the command line and return the GPIO number used
> > > > > > within U-Boot or return -1 on error.
> > > > > 
> > > > > i.e. the already existing name_to_gpio() func
> > > > 
> > > > You mean blackfin specific macro crap? Let's actually drop that and
> > > > switch over to this one. That was my intention from start.
> > > 
> > > don't spout this crap.  try reading the actual history on the lists for
> > > the background on the naming, and then try actually grepping the tree
> > > to see that multiple targets implement it.
> > 
> > Can you point me to something? This is the result of git grep, so I odn't
> > see it used at all.
> > 
> > arch/blackfin/include/asm/gpio.h:static inline int name_to_gpio(const
> > char *name)
> > arch/blackfin/include/asm/gpio.h:#define name_to_gpio(n) name_to_gpio(n)
> > common/cmd_gpio.c:#ifndef name_to_gpio
> > common/cmd_gpio.c:#define name_to_gpio(name) simple_strtoul(name, NULL,
> > 10) common/cmd_gpio.c:      return name_to_gpio(name);
> 
> seems like the patches posted and i've been through haven't been merged.

Well ... that sucks. So there was some improvement in that direction already? 
Why werent those patches merged, can you repost?

> gpio_status() is the only one to gain wider support.

What gpio_status()?

> the naming style however has been discussed on list and wasn't pulled out
> of nowhere.  it follows the Linux GPIO API conventions whereas your
> proposed naming does not.

Fine with me either way.

> > > > > > The gpio_to_string() on the other hand allows nicer reporting of
> > > > > > GPIO name in the output of cmd_gpio.
> > > > > 
> > > > > i don't see the value in this.  we already have the string name
> > > > > from the
> > > > > 
> > > > > user, so all you've implemented is:
> > > > > 	gpio_to_string(gpio_from_string(argv[]))
> > > > > 
> > > > > at which point, argv[] works fine.
> > > > 
> > > > This allows for taking multiple different inputs, while reporting one
> > > > unified output.
> > > 
> > > sounds like unnecessary bloat
> > 
> > Now you're spouting crap, it makes sense to be able to report the name
> > consistently.
> 
> except the output is for user's eyes only and doesn't make a lick of a
> difference to runtime behavior.  either they know the GPIO # (because they
> looked it up), or they're using the friendly name (which came from a
> datasheet).  in either case, things continue to work regardless of the
> input. the "value add" here doesn't justify the overhead imo.

Well how often do you use cmd_gpio in a timing-critical aplication?

> -mike

Best regards,
Marek Vasut

[U-Boot] [PATCH 1/2] GPIO: Extend the cmd_gpio API by gpio_{from, to}_string()

[Mike Frysinger]

On Tuesday 15 May 2012 01:53:10 Marek Vasut wrote:
> Dear Mike Frysinger,
> > On Monday 14 May 2012 14:25:14 Marek Vasut wrote:
> > > Dear Mike Frysinger,
> > > > On Monday 14 May 2012 07:31:35 Marek Vasut wrote:
> > > > > Dear Mike Frysinger,
> > > > > > On Tuesday 01 May 2012 16:50:14 Marek Vasut wrote:
> > > > > > > The gpio_from_string() call shall parse the incoming GPIO name
> > > > > > > taken from the command line and return the GPIO number used
> > > > > > > within U-Boot or return -1 on error.
> > > > > > 
> > > > > > i.e. the already existing name_to_gpio() func
> > > > > 
> > > > > You mean blackfin specific macro crap? Let's actually drop that and
> > > > > switch over to this one. That was my intention from start.
> > > > 
> > > > don't spout this crap.  try reading the actual history on the lists
> > > > for the background on the naming, and then try actually grepping the
> > > > tree to see that multiple targets implement it.
> > > 
> > > Can you point me to something? This is the result of git grep, so I
> > > odn't see it used at all.
> > > 
> > > arch/blackfin/include/asm/gpio.h:static inline int name_to_gpio(const
> > > char *name)
> > > arch/blackfin/include/asm/gpio.h:#define name_to_gpio(n)
> > > name_to_gpio(n) common/cmd_gpio.c:#ifndef name_to_gpio
> > > common/cmd_gpio.c:#define name_to_gpio(name) simple_strtoul(name, NULL,
> > > 10) common/cmd_gpio.c:      return name_to_gpio(name);
> > 
> > seems like the patches posted and i've been through haven't been merged.
> 
> Well ... that sucks. So there was some improvement in that direction
> already? Why werent those patches merged, can you repost?

i wasn't the one posting them, just helping review :)

> > gpio_status() is the only one to gain wider support.
> 
> What gpio_status()?

grep the tree for it :p

it let's you do "gpio status" and see what gpios have been allocated by 
different devices.  just a debugging tool, similar to like /proc/iomem.

> > > > > > > The gpio_to_string() on the other hand allows nicer reporting
> > > > > > > of GPIO name in the output of cmd_gpio.
> > > > > > 
> > > > > > i don't see the value in this.  we already have the string name
> > > > > > from the
> > > > > > 
> > > > > > user, so all you've implemented is:
> > > > > > 	gpio_to_string(gpio_from_string(argv[]))
> > > > > > 
> > > > > > at which point, argv[] works fine.
> > > > > 
> > > > > This allows for taking multiple different inputs, while reporting
> > > > > one unified output.
> > > > 
> > > > sounds like unnecessary bloat
> > > 
> > > Now you're spouting crap, it makes sense to be able to report the name
> > > consistently.
> > 
> > except the output is for user's eyes only and doesn't make a lick of a
> > difference to runtime behavior.  either they know the GPIO # (because
> > they looked it up), or they're using the friendly name (which came from
> > a datasheet).  in either case, things continue to work regardless of the
> > input. the "value add" here doesn't justify the overhead imo.
> 
> Well how often do you use cmd_gpio in a timing-critical aplication?

not sure how that's relevant.  my complaint is with code size expansion for 
what is, imo, not terribly useful.  maybe Blackfin parts are different, but the 
GPIO use has been fairly straight forward for people.

if you were to implement gpio_to_name like the existing name_to_gpio defines, i 
wouldn't complain as anyone choosing to not implement wouldn't see a code size 
difference.
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20120517/9f1b28b3/attachment.pgp>

[U-Boot] [PATCH 1/2] GPIO: Extend the cmd_gpio API by gpio_{from, to}_string()

[Marek Vasut]

Dear Mike Frysinger,

> On Monday 14 May 2012 07:31:35 Marek Vasut wrote:
> > Dear Mike Frysinger,
> > 
> > > On Tuesday 01 May 2012 16:50:14 Marek Vasut wrote:
> > > > The gpio_from_string() call shall parse the incoming GPIO name taken
> > > > from the command line and return the GPIO number used within U-Boot
> > > > or return -1 on error.
> > > 
> > > i.e. the already existing name_to_gpio() func
> > 
> > You mean blackfin specific macro crap? Let's actually drop that and
> > switch over to this one. That was my intention from start.
> 
> don't spout this crap.  try reading the actual history on the lists for the
> background on the naming, and then try actually grepping the tree to see
> that multiple targets implement it.

So ... it was chosen to be compatible with linux gpio api, that's what I 
understood ... so I grepped the kernel source:

linux-2.6# git grep name_to_gpio drivers/gpio/
linux-2.6# git grep name_to_gpio include/
linux-2.6# git grep gpio_to_name include/
linux-2.6# git grep gpio_to_name drivers/gpio/

Nothing.

[...]

Best regards,
Marek Vasut

[U-Boot] [PATCH 1/2] GPIO: Extend the cmd_gpio API by gpio_{from, to}_string()

[Mike Frysinger]

On Thursday 02 August 2012 06:05:14 Marek Vasut wrote:
> Dear Mike Frysinger,
> 
> > On Monday 14 May 2012 07:31:35 Marek Vasut wrote:
> > > Dear Mike Frysinger,
> > > 
> > > > On Tuesday 01 May 2012 16:50:14 Marek Vasut wrote:
> > > > > The gpio_from_string() call shall parse the incoming GPIO name
> > > > > taken from the command line and return the GPIO number used within
> > > > > U-Boot or return -1 on error.
> > > > 
> > > > i.e. the already existing name_to_gpio() func
> > > 
> > > You mean blackfin specific macro crap? Let's actually drop that and
> > > switch over to this one. That was my intention from start.
> > 
> > don't spout this crap.  try reading the actual history on the lists for
> > the background on the naming, and then try actually grepping the tree to
> > see that multiple targets implement it.
> 
> So ... it was chosen to be compatible with linux gpio api, that's what I
> understood ... so I grepped the kernel source:
> 
> linux-2.6# git grep name_to_gpio drivers/gpio/
> linux-2.6# git grep name_to_gpio include/
> linux-2.6# git grep gpio_to_name include/
> linux-2.6# git grep gpio_to_name drivers/gpio/
> 
> Nothing.

the naming convention is inline with the linux naming convention.  there is no 
concept in linux of stringifying a GPIO number.
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20120802/eceb27da/attachment.pgp>