linux4sam / at91bootstrap

Second level bootloader for Microchip SoC (aka AT91)
https://www.linux4sam.org/linux4sam/bin/view/Linux4SAM/AT91Bootstrap4
112 stars 232 forks source link

Unselecting an LED causes compile errors #139

Closed Saiberion closed 3 years ago

Saiberion commented 3 years ago

For our SAMA5D27 project that is based on SAMA5D27SOM1EK I wanted to unselct the use of the blue LED as we only have 2 LEDs connected but that causes compile errors in driver/led.c

CC driver/led.c driver/led.c: In function ‘at91_leds_init’: driver/led.c:53:22: error: ‘LED_B_PIO’ undeclared (first use in this function); did you mean ‘LED_G_PIO’? 53 | pio_set_gpio_output(LED_B_PIO, CONFIG_LED_B_VALUE); | ^~~~~ | LED_G_PIO driver/led.c:53:22: note: each undeclared identifier is reported only once for each function it appears in driver/led.c:53:33: error: ‘CONFIG_LED_B_VALUE’ undeclared (first use in this function); did you mean ‘CONFIG_LED_G_VALUE’? 53 | pio_set_gpio_output(LED_B_PIO, CONFIG_LED_B_VALUE); | ^~~~~~ | CONFIG_LED_G_VALUE

I found out that #ifndef for the macros does not work as selecting "No LED on board" defines LED_x_NONE with 'y' which means the macros are always true.

I fixed in on my end by doing a different check. Patch file attached. at91bootstrap3-v4.0.0-fix-skipping-unselected-leds.patch.txt

ehristev commented 3 years ago

Hi,

You are correct, there is a bug. But I think your fix is not correct. Here is what I think it's better:

diff --git a/driver/led.c b/driver/led.c
index 59dffc23..767614ad 100644
--- a/driver/led.c
+++ b/driver/led.c
@@ -43,13 +43,13 @@

 void at91_leds_init(void)
 {
-#ifndef LED_R_NONE
+#ifndef CONFIG_LED_R_NONE
        pio_set_gpio_output(LED_R_PIO, CONFIG_LED_R_VALUE);
 #endif
-#ifndef LED_G_NONE
+#ifndef CONFIG_LED_G_NONE
        pio_set_gpio_output(LED_G_PIO, CONFIG_LED_G_VALUE);
 #endif
-#ifndef LED_B_NONE
+#ifndef CONFIG_LED_B_NONE
        pio_set_gpio_output(LED_B_PIO, CONFIG_LED_B_VALUE);
 #endif
 }

Does this solve your problem ?

If you wish I can add you to the commit with your sign-off , if you provide me your email/name, in the form :

Reported-by: First name last name email@..

Saiberion commented 3 years ago

checking for CONFIG_LED_x_NONE seems to have the same issues than LED_B_NONE. That is also defined when the LED is set to "No LED on board".

Checking for the existance of LED_x_PIO still feels right as those are only defined if any of the CONFIG_LED_x_ON_PIOn is selected.

ehristev commented 3 years ago

You do make sense. I thought more about it, and if we are using PIO calls, makes sense to check for PIO config. Do you wish to add your name to your patch ?

Saiberion commented 3 years ago

Yes, you can add my name for the patch. Do I need to do something else?

ehristev commented 3 years ago

Yes, you can add my name for the patch. Do I need to do something else?

Then give me your tag : Signed-off-by: Name <email>

Saiberion commented 3 years ago

I tried to create a pull-request but have trouble to get it working.

My tag is Signed-off-by: Matthias Wieloch matthias.wieloch@few-bauer.de

ehristev commented 3 years ago

Patched applied and pushed out. Thanks !