[U-Boot] [PATCH] drivers: SPI: sunxi SPL: fix warning
Somehow an int returning function without a return statement sneaked in. Fix it.
Signed-off-by: Andre Przywara andre.przywara@arm.com --- drivers/mtd/spi/sunxi_spi_spl.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/mtd/spi/sunxi_spi_spl.c b/drivers/mtd/spi/sunxi_spi_spl.c index 67c7edd..7502314 100644 --- a/drivers/mtd/spi/sunxi_spi_spl.c +++ b/drivers/mtd/spi/sunxi_spi_spl.c @@ -158,9 +158,10 @@ static void spi0_disable_clock(void) (1 << AHB_RESET_SPI0_SHIFT)); }
-static int spi0_init(void) +static void spi0_init(void) { unsigned int pin_function = SUNXI_GPC_SPI0; + if (IS_ENABLED(CONFIG_MACH_SUN50I)) pin_function = SUN50I_GPC_SPI0;
Hi,
On 03-11-16 01:58, Andre Przywara wrote:
Somehow an int returning function without a return statement sneaked in. Fix it.
Signed-off-by: Andre Przywara andre.przywara@arm.com
LGTM:
Reviewed-by: Hans de Goede hdegoede@redhat.com
Regards,
Hans
drivers/mtd/spi/sunxi_spi_spl.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/mtd/spi/sunxi_spi_spl.c b/drivers/mtd/spi/sunxi_spi_spl.c index 67c7edd..7502314 100644 --- a/drivers/mtd/spi/sunxi_spi_spl.c +++ b/drivers/mtd/spi/sunxi_spi_spl.c @@ -158,9 +158,10 @@ static void spi0_disable_clock(void) (1 << AHB_RESET_SPI0_SHIFT)); }
-static int spi0_init(void) +static void spi0_init(void) { unsigned int pin_function = SUNXI_GPC_SPI0;
- if (IS_ENABLED(CONFIG_MACH_SUN50I)) pin_function = SUN50I_GPC_SPI0;
On Thu, Nov 3, 2016 at 6:28 AM, Andre Przywara andre.przywara@arm.com wrote:
Somehow an int returning function without a return statement sneaked in. Fix it.
Signed-off-by: Andre Przywara andre.przywara@arm.com
drivers/mtd/spi/sunxi_spi_spl.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/mtd/spi/sunxi_spi_spl.c b/drivers/mtd/spi/sunxi_spi_spl.c index 67c7edd..7502314 100644 --- a/drivers/mtd/spi/sunxi_spi_spl.c +++ b/drivers/mtd/spi/sunxi_spi_spl.c @@ -158,9 +158,10 @@ static void spi0_disable_clock(void) (1 << AHB_RESET_SPI0_SHIFT)); }
-static int spi0_init(void) +static void spi0_init(void) { unsigned int pin_function = SUNXI_GPC_SPI0;
Space not needed or unrelated, please remove this.
thanks!
Hi,
On 14/11/16 16:30, Jagan Teki wrote:
On Thu, Nov 3, 2016 at 6:28 AM, Andre Przywara andre.przywara@arm.com wrote:
Somehow an int returning function without a return statement sneaked in. Fix it.
Signed-off-by: Andre Przywara andre.przywara@arm.com
drivers/mtd/spi/sunxi_spi_spl.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/mtd/spi/sunxi_spi_spl.c b/drivers/mtd/spi/sunxi_spi_spl.c index 67c7edd..7502314 100644 --- a/drivers/mtd/spi/sunxi_spi_spl.c +++ b/drivers/mtd/spi/sunxi_spi_spl.c @@ -158,9 +158,10 @@ static void spi0_disable_clock(void) (1 << AHB_RESET_SPI0_SHIFT)); }
-static int spi0_init(void) +static void spi0_init(void) { unsigned int pin_function = SUNXI_GPC_SPI0;
Space not needed or unrelated, please remove this.
This is Linux coding style, which U-Boot adheres to. "WARNING: Missing a blank line after declarations"
I thought I should fix this since this is was in the context of this very simple patch and it improves readability. If this is too much, then please remove the line before committing.
Thanks! Andre.
On Mon, Nov 14, 2016 at 04:47:26PM +0000, Andre Przywara wrote:
Hi,
On 14/11/16 16:30, Jagan Teki wrote:
On Thu, Nov 3, 2016 at 6:28 AM, Andre Przywara andre.przywara@arm.com wrote:
Somehow an int returning function without a return statement sneaked in. Fix it.
Signed-off-by: Andre Przywara andre.przywara@arm.com
drivers/mtd/spi/sunxi_spi_spl.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/mtd/spi/sunxi_spi_spl.c b/drivers/mtd/spi/sunxi_spi_spl.c index 67c7edd..7502314 100644 --- a/drivers/mtd/spi/sunxi_spi_spl.c +++ b/drivers/mtd/spi/sunxi_spi_spl.c @@ -158,9 +158,10 @@ static void spi0_disable_clock(void) (1 << AHB_RESET_SPI0_SHIFT)); }
-static int spi0_init(void) +static void spi0_init(void) { unsigned int pin_function = SUNXI_GPC_SPI0;
Space not needed or unrelated, please remove this.
This is Linux coding style, which U-Boot adheres to. "WARNING: Missing a blank line after declarations"
I thought I should fix this since this is was in the context of this very simple patch and it improves readability. If this is too much, then please remove the line before committing.
Making things checkpatch clean is good, in the future please also mention that in commit messages. Thanks!
On Mon, 14 Nov 2016 11:56:50 -0500 Tom Rini trini@konsulko.com wrote:
On Mon, Nov 14, 2016 at 04:47:26PM +0000, Andre Przywara wrote:
Hi,
On 14/11/16 16:30, Jagan Teki wrote:
On Thu, Nov 3, 2016 at 6:28 AM, Andre Przywara andre.przywara@arm.com wrote:
Somehow an int returning function without a return statement sneaked in. Fix it.
Signed-off-by: Andre Przywara andre.przywara@arm.com
drivers/mtd/spi/sunxi_spi_spl.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/mtd/spi/sunxi_spi_spl.c b/drivers/mtd/spi/sunxi_spi_spl.c index 67c7edd..7502314 100644 --- a/drivers/mtd/spi/sunxi_spi_spl.c +++ b/drivers/mtd/spi/sunxi_spi_spl.c @@ -158,9 +158,10 @@ static void spi0_disable_clock(void) (1 << AHB_RESET_SPI0_SHIFT)); }
-static int spi0_init(void) +static void spi0_init(void) { unsigned int pin_function = SUNXI_GPC_SPI0;
Space not needed or unrelated, please remove this.
This is Linux coding style, which U-Boot adheres to. "WARNING: Missing a blank line after declarations"
I thought I should fix this since this is was in the context of this very simple patch and it improves readability. If this is too much, then please remove the line before committing.
Making things checkpatch clean is good, in the future please also mention that in commit messages. Thanks!
How can I get this checkpatch warning?
$ scripts/checkpatch.pl 0001-sunxi-Support-booting-from-SPI-flash.patch total: 0 errors, 0 warnings, 0 checks, 361 lines checked
NOTE: Ignored message types: COMPLEX_MACRO CONSIDER_KSTRTO MINMAX MULTISTATEMENT_MACRO_USE_DO_WHILE NETWORKING_BLOCK_COMMENT_STYLE PREFER_ETHER_ADDR_COPY USLEEP_RANGE
0001-sunxi-Support-booting-from-SPI-flash.patch has no obvious style problems and is ready for submission.
On Thu, 3 Nov 2016 00:58:12 +0000 Andre Przywara andre.przywara@arm.com wrote:
Somehow an int returning function without a return statement sneaked in. Fix it.
Signed-off-by: Andre Przywara andre.przywara@arm.com
drivers/mtd/spi/sunxi_spi_spl.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/mtd/spi/sunxi_spi_spl.c b/drivers/mtd/spi/sunxi_spi_spl.c index 67c7edd..7502314 100644 --- a/drivers/mtd/spi/sunxi_spi_spl.c +++ b/drivers/mtd/spi/sunxi_spi_spl.c @@ -158,9 +158,10 @@ static void spi0_disable_clock(void) (1 << AHB_RESET_SPI0_SHIFT)); }
-static int spi0_init(void) +static void spi0_init(void) { unsigned int pin_function = SUNXI_GPC_SPI0;
- if (IS_ENABLED(CONFIG_MACH_SUN50I)) pin_function = SUN50I_GPC_SPI0;
Thanks for spotting and fixing this compilation warning.
This was a last minute change. Originally there was also a check for the pins state and the function returned an error code in the case if the pins are already configured for NAND. But I removed this code before sending the patch.
The idea is that probing the SPI flash may be useful in the future even if booting from some other media. We may store some board-specific configuration in the on-board SPI flash (for example, the DRAM and CPU speed grade information). But this functionality definitely belongs to a separate patch and can be always contributed later. There is also the SPL size concern and we don't want to unnecessarily increase the code size.
On 14/11/16 18:32, Siarhei Siamashka wrote:
On Thu, 3 Nov 2016 00:58:12 +0000 Andre Przywara andre.przywara@arm.com wrote:
Somehow an int returning function without a return statement sneaked in. Fix it.
Signed-off-by: Andre Przywara andre.przywara@arm.com
drivers/mtd/spi/sunxi_spi_spl.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/mtd/spi/sunxi_spi_spl.c b/drivers/mtd/spi/sunxi_spi_spl.c index 67c7edd..7502314 100644 --- a/drivers/mtd/spi/sunxi_spi_spl.c +++ b/drivers/mtd/spi/sunxi_spi_spl.c @@ -158,9 +158,10 @@ static void spi0_disable_clock(void) (1 << AHB_RESET_SPI0_SHIFT)); }
-static int spi0_init(void) +static void spi0_init(void) { unsigned int pin_function = SUNXI_GPC_SPI0;
- if (IS_ENABLED(CONFIG_MACH_SUN50I)) pin_function = SUN50I_GPC_SPI0;
Hi Siarhei,
Thanks for spotting and fixing this compilation warning.
This was a last minute change. Originally there was also a check for the pins state and the function returned an error code in the case if the pins are already configured for NAND. But I removed this code before sending the patch.
please, no need to apologize or explain, those things happen, especially when shuffling around patches.
The only learning that we should take from it that at least those compilation warnings should be spotted on a more automated base. I think the problem here is that the feature is not enabled in any defconfig atm, but I think I will enable it in my Opi PC 2 series.
On a related matter, I ran buildman on HEAD for armv8 today and GCC 6.2 found quite some issues (will send out the fixes ASAP). So is there some buildbot somewhere that runs buildman? If yes, with what compilers?
The idea is that probing the SPI flash may be useful in the future even if booting from some other media. We may store some board-specific configuration in the on-board SPI flash (for example, the DRAM and CPU speed grade information). But this functionality definitely belongs to a separate patch and can be always contributed later. There is also the SPL size concern and we don't want to unnecessarily increase the code size.
Totally agree, and as I said: No worries. Actually thanks a lot for this series, as booting from NOR flash is really a cool feature.
Cheers, Andre.
On Mon, Nov 14, 2016 at 07:43:40PM +0000, André Przywara wrote:
[snip]
On a related matter, I ran buildman on HEAD for armv8 today and GCC 6.2 found quite some issues (will send out the fixes ASAP). So is there some buildbot somewhere that runs buildman? If yes, with what compilers?
What gcc-6.2 compilers are you using? The travis-ci setup is using the Ubuntu 'Trusty' compilers, which are not that recent. I also have been running debian/unstable in a chroot, but it's old enough of a copy to be using gcc-5.3.1 currently. I'm going to kick off a new debootstrap now'ish to see what toolchain versions I can get there. Do you know if these gcc-6.2 problems you see would also show up for rpi3 in 64bit mode? That's my current easiest test platform for aarch64. Thanks!
On 14/11/16 20:12, Tom Rini wrote:
On Mon, Nov 14, 2016 at 07:43:40PM +0000, André Przywara wrote:
[snip]
On a related matter, I ran buildman on HEAD for armv8 today and GCC 6.2 found quite some issues (will send out the fixes ASAP). So is there some buildbot somewhere that runs buildman? If yes, with what compilers?
What gcc-6.2 compilers are you using?
I just built my own, in fact for Trusty. I have some easy build scripts here [1], though the README needs some update. With this I create .deb packages for binutils and gcc-stage1 of (almost) any vanilla GCC version you throw at it.
I don't know of any newer GCC in some proper repo, historically not many people cared and either built their own or used a Linaro drop.
The travis-ci setup is using the Ubuntu 'Trusty' compilers, which are not that recent. I also have been running debian/unstable in a chroot, but it's old enough of a copy to be using gcc-5.3.1 currently. I'm going to kick off a new debootstrap now'ish to see what toolchain versions I can get there. Do you know if these gcc-6.2 problems you see would also show up for rpi3 in 64bit mode?
I didn't observe any runtime issues (though at least someone else did in one occasion). The compiler warnings are mostly due to the new GCC6 option -Wmisleading-indentation, which seems to be enabled by default. Some of them are really just wrong indentation, so cosmetic, but others are genuine bugs. I will send patches later today ($work here atm).
That's my current easiest test platform for aarch64. Thanks!
As said this was observed only by running buildman, so you shouldn't need any actual hardware to see this.
Cheers, Andre.
participants (6)
-
Andre Przywara -
André Przywara
-
Hans de Goede -
Jagan Teki -
Siarhei Siamashka -
Tom Rini