Re: [U-Boot] [PATCH v8 2/4] gpio: Replace ARM gpio.h with the common API in include/asm-generic

2011-11-07 Thread Mike Frysinger 
On Monday 07 November 2011 12:02:16 Stephen Warren wrote:
> Mike Frysinger wrote at Monday, November 07, 2011 10:40 AM:
> > On Monday 07 November 2011 11:35:33 Stephen Warren wrote:
> > > Joe Hershberger wrote at Friday, November 04, 2011 8:25 PM:
> > > > -void gpio_free(int gp)
> > > > +int gpio_free(unsigned gpio)
> > > >  {
> > > > +   return 0;
> > > >  }
> > > 
> > > If you're doing a cleanup pass on this driver, you may as well make
> > > gpio_free() do something; it should probably clear
> > > gpio_names[gpio].name and perhaps set the pin back to SFIO - in other
> > > words, undo gpio_reqeust().
> > 
> > i think the decision made in Linux was that freeing a GPIO should not
> > cause it to change tristate or anything.  all it should do is mark it as
> > "available" so something else can request it.
> 
> OK, I'll buy that, but presumably gpio_names[gpio].name should still be
> cleared to indicate the pin is free?

if the tegra code is using the gpio_names[] array to determine whether a pin 
is allocated, then gpio_free() should take care of clearing it
-mike


signature.asc
Description: This is a digitally signed message part.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH v8 2/4] gpio: Replace ARM gpio.h with the common API in include/asm-generic

2011-11-07 Thread Joe Hershberger 
On Mon, Nov 7, 2011 at 11:02 AM, Stephen Warren  wrote:
> Mike Frysinger wrote at Monday, November 07, 2011 10:40 AM:
>> * PGP Signed by an unknown key
>>
>> On Monday 07 November 2011 11:35:33 Stephen Warren wrote:
>> > Joe Hershberger wrote at Friday, November 04, 2011 8:25 PM:
>> > > -void gpio_free(int gp)
>> > > +int gpio_free(unsigned gpio)
>> > >  {
>> > > + return 0;
>> > >  }
>> >
>> > If you're doing a cleanup pass on this driver, you may as well make
>> > gpio_free() do something; it should probably clear gpio_names[gpio].name
>> > and perhaps set the pin back to SFIO - in other words, undo gpio_reqeust().
>>
>> i think the decision made in Linux was that freeing a GPIO should not cause 
>> it
>> to change tristate or anything.  all it should do is mark it as "available" 
>> so
>> something else can request it.
>> -mike
>
> OK, I'll buy that, but presumably gpio_names[gpio].name should still be 
> cleared
> to indicate the pin is free?

Besides, that would directly undo this patch:
http://patchwork.ozlabs.org/patch/119277/

I'll clear the string.

Thanks,
-Joe
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH v8 2/4] gpio: Replace ARM gpio.h with the common API in include/asm-generic

2011-11-07 Thread Stephen Warren 
Mike Frysinger wrote at Monday, November 07, 2011 10:40 AM:
> * PGP Signed by an unknown key
> 
> On Monday 07 November 2011 11:35:33 Stephen Warren wrote:
> > Joe Hershberger wrote at Friday, November 04, 2011 8:25 PM:
> > > -void gpio_free(int gp)
> > > +int gpio_free(unsigned gpio)
> > >  {
> > > + return 0;
> > >  }
> >
> > If you're doing a cleanup pass on this driver, you may as well make
> > gpio_free() do something; it should probably clear gpio_names[gpio].name
> > and perhaps set the pin back to SFIO - in other words, undo gpio_reqeust().
> 
> i think the decision made in Linux was that freeing a GPIO should not cause it
> to change tristate or anything.  all it should do is mark it as "available" so
> something else can request it.
> -mike

OK, I'll buy that, but presumably gpio_names[gpio].name should still be cleared
to indicate the pin is free?

-- 
nvpublic

___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH v8 2/4] gpio: Replace ARM gpio.h with the common API in include/asm-generic

2011-11-07 Thread Mike Frysinger 
On Monday 07 November 2011 11:35:33 Stephen Warren wrote:
> Joe Hershberger wrote at Friday, November 04, 2011 8:25 PM:
> > -void gpio_free(int gp)
> > +int gpio_free(unsigned gpio)
> >  {
> > +   return 0;
> >  }
> 
> If you're doing a cleanup pass on this driver, you may as well make
> gpio_free() do something; it should probably clear gpio_names[gpio].name
> and perhaps set the pin back to SFIO - in other words, undo gpio_reqeust().

i think the decision made in Linux was that freeing a GPIO should not cause it 
to change tristate or anything.  all it should do is mark it as "available" so 
something else can request it.
-mike


signature.asc
Description: This is a digitally signed message part.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH v8 2/4] gpio: Replace ARM gpio.h with the common API in include/asm-generic

2011-11-07 Thread Stephen Warren 
Joe Hershberger wrote at Friday, November 04, 2011 8:25 PM:
> ARM boards should use the generic GPIO API
> Update the existing gpio implementations to conform the the generic API

The Tegra changes look mostly OK, but:

a) They are almost entirely unrelated to this patch description; most of
the diff is variable renaming and nothing to do with conforming to the API.

b) I've CC'd the main Tegra maintainers to give final comment.

c) Some comments below.

> diff --git a/drivers/gpio/tegra2_gpio.c b/drivers/gpio/tegra2_gpio.c
...
> -void gpio_free(int gp)
> +int gpio_free(unsigned gpio)
>  {
> + return 0;
>  }

If you're doing a cleanup pass on this driver, you may as well make
gpio_free() do something; it should probably clear gpio_names[gpio].name
and perhaps set the pin back to SFIO - in other words, undo gpio_reqeust().

> -void gpio_toggle_value(int gp)
> -{
> - gpio_set_value(gp, !gpio_get_output_value(gp));

You should probably at least mention this function being removed in the
commit description. I hope nothing is using it.

-- 
nvpublic

___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

[U-Boot] [PATCH v8 2/4] gpio: Replace ARM gpio.h with the common API in include/asm-generic

2011-11-04 Thread Joe Hershberger 
ARM boards should use the generic GPIO API
Update the existing gpio implementations to conform the the generic API

Signed-off-by: Joe Hershberger 
Cc: Joe Hershberger 
Cc: Kim Phillips 
---
Changes for v4:
 - Split out of patch 1/2
Changes for v5:
 - Moved asm/arch/gpio.h include to asm/gpio.h
Changes for v6:
Changes for v7:
Changes for v8:
 - Include omap-common/gpio.c in retrofit

 arch/arm/cpu/armv7/omap-common/gpio.c |   23 +++--
 arch/arm/include/asm/gpio.h   |   38 +
 drivers/gpio/da8xx_gpio.c |   73 +++
 drivers/gpio/tegra2_gpio.c|  160 
 4 files changed, 128 insertions(+), 166 deletions(-)

diff --git a/arch/arm/cpu/armv7/omap-common/gpio.c 
b/arch/arm/cpu/armv7/omap-common/gpio.c
index 75a02da..fc62acc 100644
--- a/arch/arm/cpu/armv7/omap-common/gpio.c
+++ b/arch/arm/cpu/armv7/omap-common/gpio.c
@@ -36,7 +36,7 @@
  * published by the Free Software Foundation.
  */
 #include 
-#include 
+#include 
 #include 
 #include 
 
@@ -142,20 +142,22 @@ static void _set_gpio_dataout(const struct gpio_bank 
*bank, int gpio,
 /**
  * Set value of the specified gpio
  */
-void gpio_set_value(int gpio, int value)
+int gpio_set_value(unsigned gpio, int value)
 {
const struct gpio_bank *bank;
 
if (check_gpio(gpio) < 0)
-   return;
+   return -1;
bank = get_gpio_bank(gpio);
_set_gpio_dataout(bank, get_gpio_index(gpio), value);
+
+   return 0;
 }
 
 /**
  * Get value of the specified gpio
  */
-int gpio_get_value(int gpio)
+int gpio_get_value(unsigned gpio)
 {
const struct gpio_bank *bank;
void *reg;
@@ -176,11 +178,11 @@ int gpio_get_value(int gpio)
reg += OMAP_GPIO_DATAOUT;
break;
default:
-   return -EINVAL;
+   return -1;
}
break;
default:
-   return -EINVAL;
+   return -1;
}
return (__raw_readl(reg)
& (1 << get_gpio_index(gpio))) != 0;
@@ -194,7 +196,7 @@ int gpio_direction_input(unsigned gpio)
const struct gpio_bank *bank;
 
if (check_gpio(gpio) < 0)
-   return -EINVAL;
+   return -1;
 
bank = get_gpio_bank(gpio);
_set_gpio_direction(bank, get_gpio_index(gpio), 1);
@@ -210,7 +212,7 @@ int gpio_direction_output(unsigned gpio, int value)
const struct gpio_bank *bank;
 
if (check_gpio(gpio) < 0)
-   return -EINVAL;
+   return -1;
 
bank = get_gpio_bank(gpio);
_set_gpio_dataout(bank, get_gpio_index(gpio), value);
@@ -224,7 +226,7 @@ int gpio_direction_output(unsigned gpio, int value)
  *
  * NOTE: Argument 'label' is unused.
  */
-int gpio_request(int gpio, const char *label)
+int gpio_request(unsigned gpio, const char *label)
 {
if (check_gpio(gpio) < 0)
return -EINVAL;
@@ -235,6 +237,7 @@ int gpio_request(int gpio, const char *label)
 /**
  * Reset and free the gpio after using it.
  */
-void gpio_free(unsigned gpio)
+int gpio_free(unsigned gpio)
 {
+   return 0;
 }
diff --git a/arch/arm/include/asm/gpio.h b/arch/arm/include/asm/gpio.h
index eb071d1..d49ad08 100644
--- a/arch/arm/include/asm/gpio.h
+++ b/arch/arm/include/asm/gpio.h
@@ -1,38 +1,2 @@
-/*
- * Copyright (c) 2011, NVIDIA Corp. All rights reserved.
- * See file CREDITS for list of people who contributed to this
- * project.
- *
- * This program is free software; you can redistribute it and/or
- * modify it under the terms of the GNU General Public License as
- * published by the Free Software Foundation; either version 2 of
- * the License, or (at your option) any later version.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU General Public License for more details.
- *
- * You should have received a copy of the GNU General Public License
- * along with this program; if not, write to the Free Software
- * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
- * MA 02111-1307 USA
- */
-
-#ifndef _GPIO_H_
-#define _GPIO_H_
-
 #include 
-/*
- * Generic GPIO API
- */
-
-int gpio_request(int gp, const char *label);
-void gpio_free(int gp);
-void gpio_toggle_value(int gp);
-int gpio_direction_input(int gp);
-int gpio_direction_output(int gp, int value);
-int gpio_get_value(int gp);
-void gpio_set_value(int gp, int value);
-
-#endif /* _GPIO_H_ */
+#include 
diff --git a/drivers/gpio/da8xx_gpio.c b/drivers/gpio/da8xx_gpio.c
index 7a15614..9c2df44 100644
--- a/drivers/gpio/da8xx_gpio.c
+++ b/drivers/gpio/da8xx_gpio.c
@@ -181,90 +181,85 @@ static const struct pinmux_config gpio_pinmux[] = {
{ pinmux(18), 8, 2 },
 };
 
-int gpio_request(int gp, const char *label)
+int gpio_request(unsigned g