[U-Boot] [PATCH] drivers: SPI: sunxi SPL: fix warning

[U-Boot] [PATCH] drivers: SPI: sunxi SPL: fix warning

[Andre Przywara]

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;
 
-- 
2.8.2

[U-Boot] [PATCH] drivers: SPI: sunxi SPL: fix warning

[Hans de Goede]

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;
>
>

[U-Boot] [PATCH] drivers: SPI: sunxi SPL: fix warning

[Jagan Teki]

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!
-- 
Jagan Teki
Free Software Engineer | www.openedev.com
U-Boot, Linux | Upstream Maintainer
Hyderabad, India.

[U-Boot] [PATCH] drivers: SPI: sunxi SPL: fix warning

[Andre Przywara]

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.

[U-Boot] [PATCH] drivers: SPI: sunxi SPL: fix warning

[Tom Rini]

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!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20161114/57c0e803/attachment.sig>

[U-Boot] [PATCH] drivers: SPI: sunxi SPL: fix warning

[Siarhei Siamashka]

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.


-- 
Best regards,
Siarhei Siamashka

[U-Boot] [PATCH] drivers: SPI: sunxi SPL: fix warning

[Siarhei Siamashka]

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.

-- 
Best regards,
Siarhei Siamashka

[U-Boot] [PATCH] drivers: SPI: sunxi SPL: fix warning

[André Przywara]

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.

[U-Boot] [PATCH] drivers: SPI: sunxi SPL: fix warning

[Tom Rini]

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!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20161114/d9cf91c9/attachment.sig>

[U-Boot] [PATCH] drivers: SPI: sunxi SPL: fix warning

[Andre Przywara]

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.

[1] https://github.com/apritzel/cross