[U-Boot] [PATCH 1/3] cfi_flash: use specific length types for cword

[Ryan Harkin]

Hi Stefan,

On 21 October 2015 at 11:24, Stefan Roese <sr at denx.de> wrote:
> Hi Ryan,
>
> On 21.10.2015 11:54, Ryan Harkin wrote:
>>
>> cword.l is an unsigned long int, but it is intended to be 32 bits long.
>>
>> Unfortunately, it's 64-bits long on a 64-bit system, meaning that a
>> 64-bit system cannot use 32-bit wide CFI flash parts.
>>
>> Similar problems also exist with the other cword sizes.
>>
>> Also, this patch introduced compiler warnings:
>>
>> drivers/mtd/cfi_flash.c: In function 'flash_write_cmd':
>> drivers/mtd/cfi_flash.c:348:3: warning: format '%lx' expects argument of
>> type 'long unsigned int', but argument 4 has type '__u32' [-Wformat=]
>>     debug ("fwc addr %p cmd %x %8.8lx 32bit x %d bit\n", addr,
>>     ^
>> drivers/mtd/cfi_flash.c: In function 'flash_isequal':
>> drivers/mtd/cfi_flash.c:404:3: warning: format '%lx' expects argument of
>> type 'long unsigned int', but argument 3 has type '__u32' [-Wformat=]
>>     debug ("is= %8.8x %8.8lx\n", flash_read32(addr), cword.l);
>>     ^
>>
>> I masked these warnings in this patch, however, I am quite sure this is
>> the wrong approach.
>
>
> Why do you think this is the wrong approach?

That comment was more referring to the change from %lx to %x because I
moved from "unsigned long" to "u32".

> Changing from "long" to
> "u32" seems correct to me.

Ok, good.

Perhaps also the "c", "w", "l" and "ll" in "cword" should be renamed
to "u8", "u16", "u32" and "u64"?  We aren't actually looking for a
"char" in this file, but an 8 bit value, for example.

>
>
>> Signed-off-by: Ryan Harkin <ryan.harkin at linaro.org>
>> ---
>>   drivers/mtd/cfi_flash.c | 4 ++--
>>   include/mtd/cfi_flash.h | 8 ++++----
>>   2 files changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/mtd/cfi_flash.c b/drivers/mtd/cfi_flash.c
>> index 50983b8..cee85a9 100644
>> --- a/drivers/mtd/cfi_flash.c
>> +++ b/drivers/mtd/cfi_flash.c
>> @@ -345,7 +345,7 @@ void flash_write_cmd (flash_info_t * info,
>> flash_sect_t sect,
>>                 flash_write16(cword.w, addr);
>>                 break;
>>         case FLASH_CFI_32BIT:
>> -               debug ("fwc addr %p cmd %x %8.8lx 32bit x %d bit\n", addr,
>> +               debug ("fwc addr %p cmd %x %8.8x 32bit x %d bit\n", addr,
>>                        cmd, cword.l,
>>                        info->chipwidth << CFI_FLASH_SHIFT_WIDTH);
>>                 flash_write32(cword.l, addr);
>> @@ -401,7 +401,7 @@ static int flash_isequal (flash_info_t * info,
>> flash_sect_t sect,
>>                 retval = (flash_read16(addr) == cword.w);
>>                 break;
>>         case FLASH_CFI_32BIT:
>> -               debug ("is= %8.8x %8.8lx\n", flash_read32(addr), cword.l);
>> +               debug ("is= %8.8x %8.8x\n", flash_read32(addr), cword.l);
>>                 retval = (flash_read32(addr) == cword.l);
>>                 break;
>>         case FLASH_CFI_64BIT:
>> diff --git a/include/mtd/cfi_flash.h b/include/mtd/cfi_flash.h
>> index 048b477..cd30619 100644
>> --- a/include/mtd/cfi_flash.h
>> +++ b/include/mtd/cfi_flash.h
>> @@ -105,10 +105,10 @@
>>   #define NUM_ERASE_REGIONS     4 /* max. number of erase regions */
>>
>>   typedef union {
>> -       unsigned char c;
>> -       unsigned short w;
>> -       unsigned long l;
>> -       unsigned long long ll;
>> +       __u8 c;
>> +       __u16 w;
>> +       __u32 l;
>> +       __u64 ll;
>>   } cfiword_t;
>
>
> Please use the "uX" types and not the "__uX" types here. Those are
> already widely used in this header.

Yes, I'll make that change for v2.

Thanks,
Ryan.


>
> Thanks,
> Stefan
>