On Thu, Dec 9, 2021 at 7:18 PM Andre Przywara <[email protected]> wrote: Hi Andre,
> > On Thu, 9 Dec 2021 18:50:06 +0330 > Javad Rahimi <[email protected]> wrote: > > Hi Javad, > > thanks for the patch! > > > When enabling the LED support for Cubieboard2 (SUN7I) > > the uboot freezes in status_led_init() function. > > It uses a static global variable, while the .BSS is defined in DRAM area > > while that function is called before DRAM > > So I'd say that the commit message and the subject is misleading, it's not > a real fix, because nothing was really broken before. That fact that sunxi > puts the BSS into DRAM, even when this is not available at the very > beginning, is more a nasty workaround than a feature. > > So can you please reword the commit message, to focus on what this patch > actually does, and mention that this allow to get rid of a global > variable. And then just briefly mention that it helps sunxi to enable LEDs > early, as a motivation. > > Some things below .... > Sure, I will change the patch title and will present what exactly this patch is doing and why it should be done. > > Signed-off-by: Javad Rahimi <[email protected]> > > --- > > > > drivers/misc/status_led.c | 28 +++++++++++++++++++--------- > > 1 file changed, 19 insertions(+), 9 deletions(-) > > > > diff --git a/drivers/misc/status_led.c b/drivers/misc/status_led.c > > index a6e9c03a02..7d30355423 100644 > > --- a/drivers/misc/status_led.c > > +++ b/drivers/misc/status_led.c > > @@ -23,6 +23,7 @@ typedef struct { > > int state; > > int period; > > int cnt; > > + int initialized:1; > > This bitfield doesn't really help here, since the compiler will pad it > with 31 more bits. So just make it a "bool", and let the compiler figure > out what's best here. > Or can you check whether you can fold this bit into the "state" variable? > By introducing something like an "uninitialised" state? Or is there is > another way to detect initialisation (period != 0)? > Exactly, I will try to fold it into the "state" variable. Because only one bit of this variable is used. Other bit fields are free to be used. > > } led_dev_t; > > > > led_dev_t led_dev[] = { > > @@ -30,12 +31,14 @@ led_dev_t led_dev[] = { > > CONFIG_LED_STATUS_STATE, > > LED_STATUS_PERIOD, > > 0, > > + 0, > > }, > > #if defined(CONFIG_LED_STATUS1) > > { CONFIG_LED_STATUS_BIT1, > > CONFIG_LED_STATUS_STATE1, > > LED_STATUS_PERIOD1, > > 0, > > + 0, > > }, > > #endif > > #if defined(CONFIG_LED_STATUS2) > > @@ -43,6 +46,7 @@ led_dev_t led_dev[] = { > > CONFIG_LED_STATUS_STATE2, > > LED_STATUS_PERIOD2, > > 0, > > + 0, > > }, > > #endif > > #if defined(CONFIG_LED_STATUS3) > > @@ -50,6 +54,7 @@ led_dev_t led_dev[] = { > > CONFIG_LED_STATUS_STATE3, > > LED_STATUS_PERIOD3, > > 0, > > + 0, > > }, > > #endif > > #if defined(CONFIG_LED_STATUS4) > > @@ -57,6 +62,7 @@ led_dev_t led_dev[] = { > > CONFIG_LED_STATUS_STATE4, > > LED_STATUS_PERIOD4, > > 0, > > + 0, > > }, > > #endif > > #if defined(CONFIG_LED_STATUS5) > > @@ -64,22 +70,26 @@ led_dev_t led_dev[] = { > > CONFIG_LED_STATUS_STATE5, > > LED_STATUS_PERIOD5, > > 0, > > + 0, > > }, > > #endif > > }; > > > > #define MAX_LED_DEV (sizeof(led_dev)/sizeof(led_dev_t)) > > > > -static int status_led_init_done = 0; > > > > void status_led_init(void) > > { > > led_dev_t *ld; > > int i; > > > > - for (i = 0, ld = led_dev; i < MAX_LED_DEV; i++, ld++) > > + for (i = 0, ld = led_dev; i < MAX_LED_DEV; i++, ld++) { > > __led_init (ld->mask, ld->state); > > - status_led_init_done = 1; > > + ld->initialized = 1; > > + } > > + > > + return 0; > > + > > } > > > > void status_led_tick(ulong timestamp) > > @@ -87,11 +97,11 @@ void status_led_tick(ulong timestamp) > > led_dev_t *ld; > > int i; > > > > - if (!status_led_init_done) > > - status_led_init(); > > - > > for (i = 0, ld = led_dev; i < MAX_LED_DEV; i++, ld++) { > > > > + if (!ld->initialized) > > + continue; > > But this is not what the current code does, is it? At the moment a call to > status_led_tick() would initialise all LEDs, now you just skip over (all > of) them? > Right, I missed calling "status_led_init" before for loop. Because I thought "status_led_init" is called before any LED I/O manipulation. If these functions are called without calling "status_led_init" the initialization should be done, However, I think it may be a bad practice to call any I/O handling function without doing some initialization. > > + > > if (ld->state != CONFIG_LED_STATUS_BLINKING) > > continue; > > > > @@ -110,11 +120,11 @@ void status_led_set(int led, int state) > > if (led < 0 || led >= MAX_LED_DEV) > > return; > > > > - if (!status_led_init_done) > > - status_led_init(); > > - > > ld = &led_dev[led]; > > > > + if (!ld->initialized) > > + return; > > Same here, I think? Same as above :) > > Cheers, > Andre > > > ld->state = state; > > if (state == CONFIG_LED_STATUS_BLINKING) { > > ld->cnt = 0; /* always start with full period */ >
