[U-Boot] [PATCH 1/7] mtd: Fix function description in part_validate comment
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
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
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 */
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.
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)
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)); }
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
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(®s->gpio_psr) >> gpio) & 0x01; + direction = readl(®s->gpio_dir);
- return val; + if ((direction >> gpio) & 0x01) + return (readl(®s->gpio_dr) >> gpio) & 0x01; /* output mode */ + else + return (readl(®s->gpio_psr) >> gpio) & 0x01; /* input mode */ }
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(®s->gpio_psr) >> gpio) & 0x01;
direction = readl(®s->gpio_dir);
return val;
if ((direction >> gpio) & 0x01)return (readl(®s->gpio_dr) >> gpio) & 0x01; /* output mode */elsereturn (readl(®s->gpio_psr) >> gpio) & 0x01; /* input mode */}
This did the trick!
I dropped this patch.
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
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
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.
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.
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
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)
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
participants (4)
-
Benoît Thébaudeau -
Eric Bénard -
Fabio Estevam -
Otavio Salvador