public inbox for u-boot@lists.denx.de
 help / color / mirror / Atom feed
* [U-Boot] [PATCH 1/7] mtd: Fix function description in part_validate comment
@ 2013-09-28  3:24 Otavio Salvador
  2013-09-28  3:24 ` [U-Boot] [PATCH 2/7] doc: Fix a typo in the description in doc/README.JFFS2_NAND Otavio Salvador
                   ` (6 more replies)
  0 siblings, 7 replies; 16+ messages in thread
From: Otavio Salvador @ 2013-09-28  3:24 UTC (permalink / raw)
  To: u-boot

The part_validate comment had a wrong description of the actions it
does and referenced to non-existent functions while in fact it calls
'part_validate_eraseblock()'.

Signed-off-by: Otavio Salvador <otavio@ossystems.com.br>
---
 common/cmd_mtdparts.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/common/cmd_mtdparts.c b/common/cmd_mtdparts.c
index 3023479..0104285 100644
--- a/common/cmd_mtdparts.c
+++ b/common/cmd_mtdparts.c
@@ -381,10 +381,10 @@ static int part_validate_eraseblock(struct mtdids *id, struct part_info *part)
 
 
 /**
- * Performs sanity check for supplied partition. Offset and size are verified
- * to be within valid range. Partition type is checked and either
- * parts_validate_nor() or parts_validate_nand() is called with the argument
- * of part.
+ * Performs sanity check for supplied partition. Offset and size are
+ * verified to be within valid range. Partition type is checked and
+ * either part_validate_eraseblock() is called with the argument of
+ * part.
  *
  * @param id of the parent device
  * @param part partition to validate
-- 
1.8.4.rc3

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [U-Boot] [PATCH 2/7] doc: Fix a typo in the description in doc/README.JFFS2_NAND
  2013-09-28  3:24 [U-Boot] [PATCH 1/7] mtd: Fix function description in part_validate comment Otavio Salvador
@ 2013-09-28  3:24 ` Otavio Salvador
  2013-09-28  3:24 ` [U-Boot] [PATCH 3/7] include/linux/fb.h: Add a missing include for 'list.h' Otavio Salvador
                   ` (5 subsequent siblings)
  6 siblings, 0 replies; 16+ messages in thread
From: Otavio Salvador @ 2013-09-28  3:24 UTC (permalink / raw)
  To: u-boot

Signed-off-by: Otavio Salvador <otavio@ossystems.com.br>
---
 doc/README.JFFS2_NAND | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/doc/README.JFFS2_NAND b/doc/README.JFFS2_NAND
index 5018ae8..09788d5 100644
--- a/doc/README.JFFS2_NAND
+++ b/doc/README.JFFS2_NAND
@@ -1,6 +1,6 @@
 JFFS2 NAND support:
 
-To ebable, use the following #define in the board configuration file:
+To enable, use the following #define in the board configuration file:
 
 #define CONFIG_JFFS2_NAND 1
 
-- 
1.8.4.rc3

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [U-Boot] [PATCH 3/7] include/linux/fb.h: Add a missing include for 'list.h'
  2013-09-28  3:24 [U-Boot] [PATCH 1/7] mtd: Fix function description in part_validate comment Otavio Salvador
  2013-09-28  3:24 ` [U-Boot] [PATCH 2/7] doc: Fix a typo in the description in doc/README.JFFS2_NAND Otavio Salvador
@ 2013-09-28  3:24 ` Otavio Salvador
  2013-09-28 16:46   ` Fabio Estevam
  2013-09-28  3:24 ` [U-Boot] [PATCH 4/7] gpio-led: Use __led_set in __led_init code Otavio Salvador
                   ` (4 subsequent siblings)
  6 siblings, 1 reply; 16+ messages in thread
From: Otavio Salvador @ 2013-09-28  3:24 UTC (permalink / raw)
  To: u-boot

Signed-off-by: Otavio Salvador <otavio@ossystems.com.br>
---
 include/linux/fb.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/linux/fb.h b/include/linux/fb.h
index 3858f8f..111372c 100644
--- a/include/linux/fb.h
+++ b/include/linux/fb.h
@@ -2,6 +2,7 @@
 #define _LINUX_FB_H
 
 #include <linux/types.h>
+#include <linux/list.h>
 
 /* Definitions of frame buffers						*/
 
-- 
1.8.4.rc3

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [U-Boot] [PATCH 4/7] gpio-led: Use __led_set in __led_init code
  2013-09-28  3:24 [U-Boot] [PATCH 1/7] mtd: Fix function description in part_validate comment Otavio Salvador
  2013-09-28  3:24 ` [U-Boot] [PATCH 2/7] doc: Fix a typo in the description in doc/README.JFFS2_NAND Otavio Salvador
  2013-09-28  3:24 ` [U-Boot] [PATCH 3/7] include/linux/fb.h: Add a missing include for 'list.h' Otavio Salvador
@ 2013-09-28  3:24 ` Otavio Salvador
  2013-09-28  3:24 ` [U-Boot] [PATCH 5/7] gpio-led: Fix __led_toggle support to first set GPIO as input Otavio Salvador
                   ` (3 subsequent siblings)
  6 siblings, 0 replies; 16+ messages in thread
From: Otavio Salvador @ 2013-09-28  3:24 UTC (permalink / raw)
  To: u-boot

This avoid logic code duplication and is need to fix __led_toggle
later.

Signed-off-by: Otavio Salvador <otavio@ossystems.com.br>
---
 drivers/misc/gpio_led.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/misc/gpio_led.c b/drivers/misc/gpio_led.c
index 3fedddc..6afb986 100644
--- a/drivers/misc/gpio_led.c
+++ b/drivers/misc/gpio_led.c
@@ -12,12 +12,12 @@
 void __led_init(led_id_t mask, int state)
 {
 	gpio_request(mask, "gpio_led");
-	gpio_direction_output(mask, state == STATUS_LED_ON);
+	__led_set(mask, state);
 }
 
 void __led_set(led_id_t mask, int state)
 {
-	gpio_set_value(mask, state == STATUS_LED_ON);
+	gpio_direction_output(mask, state == STATUS_LED_ON);
 }
 
 void __led_toggle(led_id_t mask)
-- 
1.8.4.rc3

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [U-Boot] [PATCH 5/7] gpio-led: Fix __led_toggle support to first set GPIO as input
  2013-09-28  3:24 [U-Boot] [PATCH 1/7] mtd: Fix function description in part_validate comment Otavio Salvador
                   ` (2 preceding siblings ...)
  2013-09-28  3:24 ` [U-Boot] [PATCH 4/7] gpio-led: Use __led_set in __led_init code Otavio Salvador
@ 2013-09-28  3:24 ` Otavio Salvador
  2013-09-28 13:12   ` Eric Bénard
  2013-09-28  3:24 ` [U-Boot] [PATCH 6/7] status_led: Add support for inverted LEDs Otavio Salvador
                   ` (2 subsequent siblings)
  6 siblings, 1 reply; 16+ messages in thread
From: Otavio Salvador @ 2013-09-28  3:24 UTC (permalink / raw)
  To: u-boot

The GPIO need to be set as input before reading its current value and
set back to output for setting it; this fixes the non-working
'led <id> toggle' for GPIO based LEDs.

Signed-off-by: Otavio Salvador <otavio@ossystems.com.br>
---
 drivers/misc/gpio_led.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/misc/gpio_led.c b/drivers/misc/gpio_led.c
index 6afb986..1882751 100644
--- a/drivers/misc/gpio_led.c
+++ b/drivers/misc/gpio_led.c
@@ -22,5 +22,6 @@ void __led_set(led_id_t mask, int state)
 
 void __led_toggle(led_id_t mask)
 {
-	gpio_set_value(mask, !gpio_get_value(mask));
+	gpio_direction_input(mask);
+	__led_set(mask, !gpio_get_value(mask));
 }
-- 
1.8.4.rc3

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [U-Boot] [PATCH 6/7] status_led: Add support for inverted LEDs
  2013-09-28  3:24 [U-Boot] [PATCH 1/7] mtd: Fix function description in part_validate comment Otavio Salvador
                   ` (3 preceding siblings ...)
  2013-09-28  3:24 ` [U-Boot] [PATCH 5/7] gpio-led: Fix __led_toggle support to first set GPIO as input Otavio Salvador
@ 2013-09-28  3:24 ` Otavio Salvador
  2013-09-28 14:17   ` Benoît Thébaudeau
  2013-09-28  3:24 ` [U-Boot] [PATCH 7/7] cmd_led: Add support for inverted BIT leds Otavio Salvador
  2013-09-28 13:06 ` [U-Boot] [PATCH 1/7] mtd: Fix function description in part_validate comment Eric Bénard
  6 siblings, 1 reply; 16+ messages in thread
From: Otavio Salvador @ 2013-09-28  3:24 UTC (permalink / raw)
  To: u-boot

There're cases we want to use active-low LEDs and the 'inverted' logic
needs to be added. This includes it using the STATUS_LED_INVERT macro.

Signed-off-by: Otavio Salvador <otavio@ossystems.com.br>
---
 doc/README.LED            |  2 ++
 drivers/misc/status_led.c | 21 ++++++++++++++++++---
 include/status_led.h      | 14 ++++++++++++++
 3 files changed, 34 insertions(+), 3 deletions(-)

diff --git a/doc/README.LED b/doc/README.LED
index c3bcb3a..c14555a 100644
--- a/doc/README.LED
+++ b/doc/README.LED
@@ -43,6 +43,8 @@ STATUS_LED_RED is the red LED.  It is used signal errors. This must be a valid
 STATUS_LED_BIT value. Other similar color LED's are STATUS_LED_YELLOW and
 STATUS_LED_BLUE.
 
+STATUS_LED_INVERT and STATUS_LED_INVERT<n> to use active-low LEDs.
+
 These board must define these functions
 
 __led_init is called once to initialize the LED to STATUS_LED_STATE. One time
diff --git a/drivers/misc/status_led.c b/drivers/misc/status_led.c
index 6b71ad4..5878d15 100644
--- a/drivers/misc/status_led.c
+++ b/drivers/misc/status_led.c
@@ -23,19 +23,22 @@ typedef struct {
 	led_id_t mask;
 	int state;
 	int period;
+	int invert;
 	int cnt;
 } led_dev_t;
 
-led_dev_t led_dev[] = {
+static led_dev_t led_dev[] = {
     {	STATUS_LED_BIT,
 	STATUS_LED_STATE,
 	STATUS_LED_PERIOD,
+	STATUS_LED_INVERT,
 	0,
     },
 #if defined(STATUS_LED_BIT1)
     {	STATUS_LED_BIT1,
 	STATUS_LED_STATE1,
 	STATUS_LED_PERIOD1,
+	STATUS_LED_INVERT1,
 	0,
     },
 #endif
@@ -43,6 +46,7 @@ led_dev_t led_dev[] = {
     {	STATUS_LED_BIT2,
 	STATUS_LED_STATE2,
 	STATUS_LED_PERIOD2,
+	STATUS_LED_INVERT2,
 	0,
     },
 #endif
@@ -50,6 +54,7 @@ led_dev_t led_dev[] = {
     {	STATUS_LED_BIT3,
 	STATUS_LED_STATE3,
 	STATUS_LED_PERIOD3,
+	STATUS_LED_INVERT3,
 	0,
     },
 #endif
@@ -64,9 +69,11 @@ static void status_led_init (void)
 	led_dev_t *ld;
 	int i;
 
-	for (i = 0, ld = led_dev; i < MAX_LED_DEV; i++, ld++)
-		__led_init (ld->mask, ld->state);
+	/* Avoid join in a call loop */
 	status_led_init_done = 1;
+
+	for (i = 0, ld = led_dev; i < MAX_LED_DEV; i++, ld++)
+		status_led_set (i, ld->state);
 }
 
 void status_led_tick (ulong timestamp)
@@ -107,5 +114,13 @@ void status_led_set (int led, int state)
 		ld->cnt = 0;		/* always start with full period    */
 		state = STATUS_LED_ON;	/* always start with LED _ON_       */
 	}
+
+	if (ld->invert) {
+		if (state == STATUS_LED_ON)
+			state = STATUS_LED_OFF;
+		else
+			state = STATUS_LED_ON;
+	}
+
 	__led_set (ld->mask, state);
 }
diff --git a/include/status_led.h b/include/status_led.h
index ecff60d..0da3fda 100644
--- a/include/status_led.h
+++ b/include/status_led.h
@@ -288,6 +288,20 @@ extern void __led_set (led_id_t mask, int state);
 #else
 # error Status LED configuration missing
 #endif
+
+#ifndef STATUS_LED_INVERT
+#define STATUS_LED_INVERT 0
+#endif
+#ifndef STATUS_LED_INVERT1
+#define STATUS_LED_INVERT1 0
+#endif
+#ifndef STATUS_LED_INVERT2
+#define STATUS_LED_INVERT2 0
+#endif
+#ifndef STATUS_LED_INVERT3
+#define STATUS_LED_INVERT3 0
+#endif
+
 /************************************************************************/
 
 #ifndef CONFIG_BOARD_SPECIFIC_LED
-- 
1.8.4.rc3

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [U-Boot] [PATCH 7/7] cmd_led: Add support for inverted BIT leds
  2013-09-28  3:24 [U-Boot] [PATCH 1/7] mtd: Fix function description in part_validate comment Otavio Salvador
                   ` (4 preceding siblings ...)
  2013-09-28  3:24 ` [U-Boot] [PATCH 6/7] status_led: Add support for inverted LEDs Otavio Salvador
@ 2013-09-28  3:24 ` Otavio Salvador
  2013-09-28 13:06 ` [U-Boot] [PATCH 1/7] mtd: Fix function description in part_validate comment Eric Bénard
  6 siblings, 0 replies; 16+ messages in thread
From: Otavio Salvador @ 2013-09-28  3:24 UTC (permalink / raw)
  To: u-boot

Signed-off-by: Otavio Salvador <otavio@ossystems.com.br>
---
 common/cmd_led.c | 27 ++++++++++++++++-----------
 1 file changed, 16 insertions(+), 11 deletions(-)

diff --git a/common/cmd_led.c b/common/cmd_led.c
index c48603c..d541f2f 100644
--- a/common/cmd_led.c
+++ b/common/cmd_led.c
@@ -18,6 +18,7 @@
 struct led_tbl_s {
 	char		*string;	/* String for use in the command */
 	led_id_t	mask;		/* Mask used for calling __led_set() */
+	int		invert;	/* Is the LED inverted? */
 	void		(*off)(void);	/* Optional function for turning LED off */
 	void		(*on)(void);	/* Optional function for turning LED on */
 	void		(*toggle)(void);/* Optional function for toggling LED */
@@ -28,31 +29,31 @@ typedef struct led_tbl_s led_tbl_t;
 static const led_tbl_t led_commands[] = {
 #ifdef CONFIG_BOARD_SPECIFIC_LED
 #ifdef STATUS_LED_BIT
-	{ "0", STATUS_LED_BIT, NULL, NULL, NULL },
+	{ "0", STATUS_LED_BIT, STATUS_LED_INVERT, NULL, NULL, NULL },
 #endif
 #ifdef STATUS_LED_BIT1
-	{ "1", STATUS_LED_BIT1, NULL, NULL, NULL },
+	{ "1", STATUS_LED_BIT1, STATUS_LED_INVERT1, NULL, NULL, NULL },
 #endif
 #ifdef STATUS_LED_BIT2
-	{ "2", STATUS_LED_BIT2, NULL, NULL, NULL },
+	{ "2", STATUS_LED_BIT2, STATUS_LED_INVERT2, NULL, NULL, NULL },
 #endif
 #ifdef STATUS_LED_BIT3
-	{ "3", STATUS_LED_BIT3, NULL, NULL, NULL },
+	{ "3", STATUS_LED_BIT3, STATUS_LED_INVERT3, NULL, NULL, NULL },
 #endif
 #endif
 #ifdef STATUS_LED_GREEN
-	{ "green", STATUS_LED_GREEN, green_led_off, green_led_on, NULL },
+	{ "green", STATUS_LED_GREEN, 0, green_led_off, green_led_on, NULL },
 #endif
 #ifdef STATUS_LED_YELLOW
-	{ "yellow", STATUS_LED_YELLOW, yellow_led_off, yellow_led_on, NULL },
+	{ "yellow", STATUS_LED_YELLOW, 0, yellow_led_off, yellow_led_on, NULL },
 #endif
 #ifdef STATUS_LED_RED
-	{ "red", STATUS_LED_RED, red_led_off, red_led_on, NULL },
+	{ "red", STATUS_LED_RED, 0, red_led_off, red_led_on, NULL },
 #endif
 #ifdef STATUS_LED_BLUE
-	{ "blue", STATUS_LED_BLUE, blue_led_off, blue_led_on, NULL },
+	{ "blue", STATUS_LED_BLUE, 0, blue_led_off, blue_led_on, NULL },
 #endif
-	{ NULL, 0, NULL, NULL, NULL }
+	{ NULL, 0, 0, NULL, NULL, NULL }
 };
 
 enum led_cmd { LED_ON, LED_OFF, LED_TOGGLE };
@@ -95,14 +96,18 @@ int do_led (cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 					led_commands[i].on();
 				else
 					__led_set(led_commands[i].mask,
-							  STATUS_LED_ON);
+							  (led_commands[i].invert
+							   ? STATUS_LED_OFF
+							   : STATUS_LED_ON));
 				break;
 			case LED_OFF:
 				if (led_commands[i].off)
 					led_commands[i].off();
 				else
 					__led_set(led_commands[i].mask,
-							  STATUS_LED_OFF);
+							  (led_commands[i].invert
+							   ? STATUS_LED_ON
+							   : STATUS_LED_OFF));
 				break;
 			case LED_TOGGLE:
 				if (led_commands[i].toggle)
-- 
1.8.4.rc3

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [U-Boot] [PATCH 1/7] mtd: Fix function description in part_validate comment
  2013-09-28  3:24 [U-Boot] [PATCH 1/7] mtd: Fix function description in part_validate comment Otavio Salvador
                   ` (5 preceding siblings ...)
  2013-09-28  3:24 ` [U-Boot] [PATCH 7/7] cmd_led: Add support for inverted BIT leds Otavio Salvador
@ 2013-09-28 13:06 ` Eric Bénard
  6 siblings, 0 replies; 16+ messages in thread
From: Eric Bénard @ 2013-09-28 13:06 UTC (permalink / raw)
  To: u-boot

Hi Otavio,

Le Sat, 28 Sep 2013 00:24:12 -0300,
Otavio Salvador <otavio@ossystems.com.br> a ?crit :

> The part_validate comment had a wrong description of the actions it
> does and referenced to non-existent functions while in fact it calls
> 'part_validate_eraseblock()'.
> 
> Signed-off-by: Otavio Salvador <otavio@ossystems.com.br>
> ---
>  common/cmd_mtdparts.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/common/cmd_mtdparts.c b/common/cmd_mtdparts.c
> index 3023479..0104285 100644
> --- a/common/cmd_mtdparts.c
> +++ b/common/cmd_mtdparts.c
> @@ -381,10 +381,10 @@ static int part_validate_eraseblock(struct mtdids *id, struct part_info *part)
>  
>  
>  /**
> - * Performs sanity check for supplied partition. Offset and size are verified
> - * to be within valid range. Partition type is checked and either
> - * parts_validate_nor() or parts_validate_nand() is called with the argument
> - * of part.
> + * Performs sanity check for supplied partition. Offset and size are
> + * verified to be within valid range. Partition type is checked and
> + * either part_validate_eraseblock() is called with the argument of

and now you can remove either ;-)

Eric

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [U-Boot] [PATCH 5/7] gpio-led: Fix __led_toggle support to first set GPIO as input
  2013-09-28  3:24 ` [U-Boot] [PATCH 5/7] gpio-led: Fix __led_toggle support to first set GPIO as input Otavio Salvador
@ 2013-09-28 13:12   ` Eric Bénard
  2013-09-28 16:35     ` Fabio Estevam
  0 siblings, 1 reply; 16+ messages in thread
From: Eric Bénard @ 2013-09-28 13:12 UTC (permalink / raw)
  To: u-boot

Hi Otavio,

Le Sat, 28 Sep 2013 00:24:16 -0300,
Otavio Salvador <otavio@ossystems.com.br> a ?crit :

> The GPIO need to be set as input before reading its current value and
> set back to output for setting it; this fixes the non-working
> 'led <id> toggle' for GPIO based LEDs.
> 
> Signed-off-by: Otavio Salvador <otavio@ossystems.com.br>
> ---
>  drivers/misc/gpio_led.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/misc/gpio_led.c b/drivers/misc/gpio_led.c
> index 6afb986..1882751 100644
> --- a/drivers/misc/gpio_led.c
> +++ b/drivers/misc/gpio_led.c
> @@ -22,5 +22,6 @@ void __led_set(led_id_t mask, int state)
>  
>  void __led_toggle(led_id_t mask)
>  {
> -	gpio_set_value(mask, !gpio_get_value(mask));
> +	gpio_direction_input(mask);
> +	__led_set(mask, !gpio_get_value(mask));
>  }

How can that work on a hardware point of view ?
If you configure the gpio as an input it isn't any more driven as an
output so the value you read depends on the way the led is wired (and
maybe also on internal pull up/down) and not on the way it was
previously driven when the gpio was an output.

Maybe the real fix would be to check why gpio_get_value doesn't return
the level of the gpio when it's configured as an output.

Eric

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [U-Boot] [PATCH 6/7] status_led: Add support for inverted LEDs
  2013-09-28  3:24 ` [U-Boot] [PATCH 6/7] status_led: Add support for inverted LEDs Otavio Salvador
@ 2013-09-28 14:17   ` Benoît Thébaudeau
  2013-09-28 16:49     ` Fabio Estevam
  0 siblings, 1 reply; 16+ messages in thread
From: Benoît Thébaudeau @ 2013-09-28 14:17 UTC (permalink / raw)
  To: u-boot

Dear Otavio Salvador,

On Saturday, September 28, 2013 5:24:17 AM, Otavio Salvador wrote:
> There're cases we want to use active-low LEDs and the 'inverted' logic
> needs to be added. This includes it using the STATUS_LED_INVERT macro.

There is already a STATUS_LED_ACTIVE definition (though not one per LED) in
include/status_led.h for some platforms. Wouldn't it be worth keeping the same
naming here for consistency (i.e. STATUS_LED_ACTIVEn, which would also imply
exchanging 0/1 values)?

[...]

Best regards,
Beno?t

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [U-Boot] [PATCH 5/7] gpio-led: Fix __led_toggle support to first set GPIO as input
  2013-09-28 13:12   ` Eric Bénard
@ 2013-09-28 16:35     ` Fabio Estevam
  2013-09-28 19:05       ` Otavio Salvador
  0 siblings, 1 reply; 16+ messages in thread
From: Fabio Estevam @ 2013-09-28 16:35 UTC (permalink / raw)
  To: u-boot

On Sat, Sep 28, 2013 at 10:12 AM, Eric B?nard <eric@eukrea.com> wrote:

> Maybe the real fix would be to check why gpio_get_value doesn't return
> the level of the gpio when it's configured as an output.

I agree with Eric.

Could you please test the patch below?

--- a/drivers/gpio/mxc_gpio.c
+++ b/drivers/gpio/mxc_gpio.c
@@ -93,7 +93,7 @@ int gpio_get_value(unsigned gpio)
 {
        unsigned int port = GPIO_TO_PORT(gpio);
        struct gpio_regs *regs;
-       u32 val;
+       u32 direction;

        if (port >= ARRAY_SIZE(gpio_ports))
                return -1;
@@ -102,9 +102,12 @@ int gpio_get_value(unsigned gpio)

        regs = (struct gpio_regs *)gpio_ports[port];

-       val = (readl(&regs->gpio_psr) >> gpio) & 0x01;
+       direction = readl(&regs->gpio_dir);

-       return val;
+       if ((direction >> gpio) & 0x01)
+               return (readl(&regs->gpio_dr) >> gpio) & 0x01; /* output mode */
+       else
+               return (readl(&regs->gpio_psr) >> gpio) & 0x01; /* input mode */
 }

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [U-Boot] [PATCH 3/7] include/linux/fb.h: Add a missing include for 'list.h'
  2013-09-28  3:24 ` [U-Boot] [PATCH 3/7] include/linux/fb.h: Add a missing include for 'list.h' Otavio Salvador
@ 2013-09-28 16:46   ` Fabio Estevam
  0 siblings, 0 replies; 16+ messages in thread
From: Fabio Estevam @ 2013-09-28 16:46 UTC (permalink / raw)
  To: u-boot

On Sat, Sep 28, 2013 at 12:24 AM, Otavio Salvador
<otavio@ossystems.com.br> wrote:
> Signed-off-by: Otavio Salvador <otavio@ossystems.com.br>

Please provide a commit log and explain which function from list.h is
needed and/or what is the build error you get  without this header.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [U-Boot] [PATCH 6/7] status_led: Add support for inverted LEDs
  2013-09-28 14:17   ` Benoît Thébaudeau
@ 2013-09-28 16:49     ` Fabio Estevam
  2013-09-28 19:08       ` Otavio Salvador
  0 siblings, 1 reply; 16+ messages in thread
From: Fabio Estevam @ 2013-09-28 16:49 UTC (permalink / raw)
  To: u-boot

On Sat, Sep 28, 2013 at 11:17 AM, Beno?t Th?baudeau
<benoit.thebaudeau@advansee.com> wrote:
> Dear Otavio Salvador,
>
> On Saturday, September 28, 2013 5:24:17 AM, Otavio Salvador wrote:
>> There're cases we want to use active-low LEDs and the 'inverted' logic
>> needs to be added. This includes it using the STATUS_LED_INVERT macro.
>
> There is already a STATUS_LED_ACTIVE definition (though not one per LED) in
> include/status_led.h for some platforms. Wouldn't it be worth keeping the same
> naming here for consistency (i.e. STATUS_LED_ACTIVEn, which would also imply
> exchanging 0/1 values)?

I agree. "INVERT" is confusing, because we don't know what is the normal state.

Doing like Beno?t suggests would be clearer: STATUS_LED_ACTIVE0 or
STATUS_LED_ACTIVE1.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [U-Boot] [PATCH 5/7] gpio-led: Fix __led_toggle support to first set GPIO as input
  2013-09-28 16:35     ` Fabio Estevam
@ 2013-09-28 19:05       ` Otavio Salvador
  0 siblings, 0 replies; 16+ messages in thread
From: Otavio Salvador @ 2013-09-28 19:05 UTC (permalink / raw)
  To: u-boot

On Sat, Sep 28, 2013 at 1:35 PM, Fabio Estevam <festevam@gmail.com> wrote:
> On Sat, Sep 28, 2013 at 10:12 AM, Eric B?nard <eric@eukrea.com> wrote:
>
>> Maybe the real fix would be to check why gpio_get_value doesn't return
>> the level of the gpio when it's configured as an output.
>
> I agree with Eric.
>
> Could you please test the patch below?
>
> --- a/drivers/gpio/mxc_gpio.c
> +++ b/drivers/gpio/mxc_gpio.c
> @@ -93,7 +93,7 @@ int gpio_get_value(unsigned gpio)
>  {
>         unsigned int port = GPIO_TO_PORT(gpio);
>         struct gpio_regs *regs;
> -       u32 val;
> +       u32 direction;
>
>         if (port >= ARRAY_SIZE(gpio_ports))
>                 return -1;
> @@ -102,9 +102,12 @@ int gpio_get_value(unsigned gpio)
>
>         regs = (struct gpio_regs *)gpio_ports[port];
>
> -       val = (readl(&regs->gpio_psr) >> gpio) & 0x01;
> +       direction = readl(&regs->gpio_dir);
>
> -       return val;
> +       if ((direction >> gpio) & 0x01)
> +               return (readl(&regs->gpio_dr) >> gpio) & 0x01; /* output mode */
> +       else
> +               return (readl(&regs->gpio_psr) >> gpio) & 0x01; /* input mode */
>  }

This did the trick!

I dropped this patch.

-- 
Otavio Salvador                             O.S. Systems
http://www.ossystems.com.br        http://code.ossystems.com.br
Mobile: +55 (53) 9981-7854            Mobile: +1 (347) 903-9750

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [U-Boot] [PATCH 6/7] status_led: Add support for inverted LEDs
  2013-09-28 16:49     ` Fabio Estevam
@ 2013-09-28 19:08       ` Otavio Salvador
  2013-09-29 13:40         ` Benoît Thébaudeau
  0 siblings, 1 reply; 16+ messages in thread
From: Otavio Salvador @ 2013-09-28 19:08 UTC (permalink / raw)
  To: u-boot

On Sat, Sep 28, 2013 at 1:49 PM, Fabio Estevam <festevam@gmail.com> wrote:
> On Sat, Sep 28, 2013 at 11:17 AM, Beno?t Th?baudeau
> <benoit.thebaudeau@advansee.com> wrote:
>> Dear Otavio Salvador,
>>
>> On Saturday, September 28, 2013 5:24:17 AM, Otavio Salvador wrote:
>>> There're cases we want to use active-low LEDs and the 'inverted' logic
>>> needs to be added. This includes it using the STATUS_LED_INVERT macro.
>>
>> There is already a STATUS_LED_ACTIVE definition (though not one per LED) in
>> include/status_led.h for some platforms. Wouldn't it be worth keeping the same
>> naming here for consistency (i.e. STATUS_LED_ACTIVEn, which would also imply
>> exchanging 0/1 values)?
>
> I agree. "INVERT" is confusing, because we don't know what is the normal state.
>
> Doing like Beno?t suggests would be clearer: STATUS_LED_ACTIVE0 or
> STATUS_LED_ACTIVE1.

The problem here is that the BIT LEDs are used in the cmd_led and it
does not have the 'active' knowledge but a ON OFF concept. So what we
do there is to change the intended status. The STATUS_LED_ACTIVE is
for the STATUS_LED_BOOT and not for a 'specific' bit.

-- 
Otavio Salvador                             O.S. Systems
http://www.ossystems.com.br        http://code.ossystems.com.br
Mobile: +55 (53) 9981-7854            Mobile: +1 (347) 903-9750

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [U-Boot] [PATCH 6/7] status_led: Add support for inverted LEDs
  2013-09-28 19:08       ` Otavio Salvador
@ 2013-09-29 13:40         ` Benoît Thébaudeau
  0 siblings, 0 replies; 16+ messages in thread
From: Benoît Thébaudeau @ 2013-09-29 13:40 UTC (permalink / raw)
  To: u-boot

Hi Otavio,

On Saturday, September 28, 2013 9:08:48 PM, Otavio Salvador wrote:
> On Sat, Sep 28, 2013 at 1:49 PM, Fabio Estevam <festevam@gmail.com> wrote:
> > On Sat, Sep 28, 2013 at 11:17 AM, Beno?t Th?baudeau
> > <benoit.thebaudeau@advansee.com> wrote:
> >> Dear Otavio Salvador,
> >>
> >> On Saturday, September 28, 2013 5:24:17 AM, Otavio Salvador wrote:
> >>> There're cases we want to use active-low LEDs and the 'inverted' logic
> >>> needs to be added. This includes it using the STATUS_LED_INVERT macro.
> >>
> >> There is already a STATUS_LED_ACTIVE definition (though not one per LED)
> >> in
> >> include/status_led.h for some platforms. Wouldn't it be worth keeping the
> >> same
> >> naming here for consistency (i.e. STATUS_LED_ACTIVEn, which would also
> >> imply
> >> exchanging 0/1 values)?
> >
> > I agree. "INVERT" is confusing, because we don't know what is the normal
> > state.
> >
> > Doing like Beno?t suggests would be clearer: STATUS_LED_ACTIVE0 or
> > STATUS_LED_ACTIVE1.
> 
> The problem here is that the BIT LEDs are used in the cmd_led and it
> does not have the 'active' knowledge but a ON OFF concept. So what we
> do there is to change the intended status. The STATUS_LED_ACTIVE is
> for the STATUS_LED_BOOT and not for a 'specific' bit.

I meant that the current use of STATUS_LED_ACTIVE could be extended to what you
are trying to do here:
+#ifndef STATUS_LED_ACTIVE
+#define STATUS_LED_ACTIVE 1
+#endif
+#ifndef STATUS_LED_ACTIVE1
+#define STATUS_LED_ACTIVE1 1
+#endif
+#ifndef STATUS_LED_ACTIVE2
+#define STATUS_LED_ACTIVE2 1
+#endif
+#ifndef STATUS_LED_ACTIVE3
+#define STATUS_LED_ACTIVE3 1
+#endif

But then, maybe it's not the call to __led_set() that should be changed, but
rather how the passed value is used in the underlying layer, e.g. in
drivers/misc/gpio_led.c. However, since the status LED API takes binary states,
it better fits in drivers/misc/status_led.c as you did.

That means that __led_set() would no longer take STATUS_LED_ON/OFF, but rather
something like STATUS_LED_HI/LO, and led_state_value() would convert
STATUS_LED_ON/OFF to STATUS_LED_HI/LO according to STATUS_LED_ACTIVEn.

Best regards,
Beno?t

^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2013-09-29 13:40 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-09-28  3:24 [U-Boot] [PATCH 1/7] mtd: Fix function description in part_validate comment Otavio Salvador
2013-09-28  3:24 ` [U-Boot] [PATCH 2/7] doc: Fix a typo in the description in doc/README.JFFS2_NAND Otavio Salvador
2013-09-28  3:24 ` [U-Boot] [PATCH 3/7] include/linux/fb.h: Add a missing include for 'list.h' Otavio Salvador
2013-09-28 16:46   ` Fabio Estevam
2013-09-28  3:24 ` [U-Boot] [PATCH 4/7] gpio-led: Use __led_set in __led_init code Otavio Salvador
2013-09-28  3:24 ` [U-Boot] [PATCH 5/7] gpio-led: Fix __led_toggle support to first set GPIO as input Otavio Salvador
2013-09-28 13:12   ` Eric Bénard
2013-09-28 16:35     ` Fabio Estevam
2013-09-28 19:05       ` Otavio Salvador
2013-09-28  3:24 ` [U-Boot] [PATCH 6/7] status_led: Add support for inverted LEDs Otavio Salvador
2013-09-28 14:17   ` Benoît Thébaudeau
2013-09-28 16:49     ` Fabio Estevam
2013-09-28 19:08       ` Otavio Salvador
2013-09-29 13:40         ` Benoît Thébaudeau
2013-09-28  3:24 ` [U-Boot] [PATCH 7/7] cmd_led: Add support for inverted BIT leds Otavio Salvador
2013-09-28 13:06 ` [U-Boot] [PATCH 1/7] mtd: Fix function description in part_validate comment Eric Bénard

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox