[U-Boot] [PATCH] FAT: make FAT compile without VFAT

[U-Boot] [PATCH] FAT: make FAT compile without VFAT

[Richard Genoud]

Signed-off-by: Richard Genoud <richard.genoud@gmail.com>
---
 fs/fat/fat.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/fs/fat/fat.c b/fs/fat/fat.c
index 393c378..defdd74 100644
--- a/fs/fat/fat.c
+++ b/fs/fat/fat.c
@@ -589,7 +589,9 @@ static dir_entry *get_dentfromdir(fsdata *mydata, int startsect,
 				  char *filename, dir_entry *retdent,
 				  int dols)
 {
+#ifdef CONFIG_SUPPORT_VFAT
 	__u16 prevcksum = 0xffff;
+#endif
 	__u32 curclust = START(retdent);
 	int files = 0, dirs = 0;
 
@@ -828,7 +830,9 @@ do_fat_read_at(const char *filename, unsigned long pos, void *buffer,
 	fsdata datablock;
 	fsdata *mydata = &datablock;
 	dir_entry *dentptr = NULL;
+#ifdef CONFIG_SUPPORT_VFAT
 	__u16 prevcksum = 0xffff;
+#endif
 	char *subname = "";
 	__u32 cursect;
 	int idx, isdir = 0;
@@ -944,7 +948,9 @@ do_fat_read_at(const char *filename, unsigned long pos, void *buffer,
 
 		for (i = 0; i < DIRENTSPERBLOCK; i++) {
 			char s_name[14], l_name[VFAT_MAXLEN_BYTES];
+#ifdef CONFIG_SUPPORT_VFAT
 			__u8 csum;
+#endif
 
 			l_name[0] = '\0';
 			if (dentptr->name[0] == DELETED_FLAG) {
@@ -952,7 +958,9 @@ do_fat_read_at(const char *filename, unsigned long pos, void *buffer,
 				continue;
 			}
 
+#ifdef CONFIG_SUPPORT_VFAT
 			csum = mkcksum(dentptr->name, dentptr->ext);
+#endif
 			if (dentptr->attr & ATTR_VOLUME) {
 #ifdef CONFIG_SUPPORT_VFAT
 				if ((dentptr->attr & ATTR_VFAT) == ATTR_VFAT &&
-- 
1.7.2.5

[U-Boot] [PATCH] FAT: make FAT compile without VFAT

[Marek Vasut]

Dear Richard Genoud,

Please provide a reasonable commit message. Why is this needed?

I really dislike the amount of added #ifdefs :(
[...]

Best regards,
Marek Vasut

[U-Boot] [PATCH] FAT: make FAT compile without VFAT

[Richard Genoud]

2012/12/4 Marek Vasut <marex@denx.de>:
> Dear Richard Genoud,
>
> Please provide a reasonable commit message. Why is this needed?
hi,
You're right, my commit message is nearly inexistent.. sorry.

This is needed in the case where we don't want the VFAT support (for
whatever reason).
in this case, we #undef CONFIG_SUPPORT_VFAT (in fat.h)
and it breaks at :
csum = mkcksum(dentptr->name, dentptr->ext);


> I really dislike the amount of added #ifdefs :(
> [...]
yes, I wasn't very comfortable with that either, I'll come with
something cleaner.

Best regards,

Richard.



>
> Best regards,
> Marek Vasut



-- 
for me, ck means con kolivas and not calvin klein... does it mean I'm a geek ?

[U-Boot] [PATCH] FAT: make FAT compile without VFAT

[Stefano Babic]

On 04/12/2012 14:04, Richard Genoud wrote:
> Signed-off-by: Richard Genoud <richard.genoud@gmail.com>
> ---
>  fs/fat/fat.c |    8 ++++++++
>  1 files changed, 8 insertions(+), 0 deletions(-)
> 
> diff --git a/fs/fat/fat.c b/fs/fat/fat.c
> index 393c378..defdd74 100644
> --- a/fs/fat/fat.c
> +++ b/fs/fat/fat.c
> @@ -589,7 +589,9 @@ static dir_entry *get_dentfromdir(fsdata *mydata, int startsect,
>  				  char *filename, dir_entry *retdent,
>  				  int dols)
>  {
> +#ifdef CONFIG_SUPPORT_VFAT
>  	__u16 prevcksum = 0xffff;
> +#endif

You can declare the variable __maybe_unused without adding the #ifdef

>  	__u32 curclust = START(retdent);
>  	int files = 0, dirs = 0;
>  
> @@ -828,7 +830,9 @@ do_fat_read_at(const char *filename, unsigned long pos, void *buffer,
>  	fsdata datablock;
>  	fsdata *mydata = &datablock;
>  	dir_entry *dentptr = NULL;
> +#ifdef CONFIG_SUPPORT_VFAT
>  	__u16 prevcksum = 0xffff;
> +#endif

Ditto

> +#ifdef CONFIG_SUPPORT_VFAT
>  			csum = mkcksum(dentptr->name, dentptr->ext);
> +#endif

I think this is the only place where #ifdef is necessary

Best regards,
Stefano Babic

-- 
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================

[U-Boot] [PATCH 0/2] clean some FAT code

[Richard Genoud]

Hi,

Instead of adding some more ugly #ifdefs, I came with another approach.
It makes the code easier to read, and correct the compilation error
when VFAT wasn't enabled.

Best regards,

Richard Genoud (2):
  FAT: remove ifdefs to make the code more readable
  FAT: use toupper/tolower instead of recoding them

 fs/fat/fat.c       |   58 +++++++++++++++++++++++++++------------------------
 fs/fat/fat_write.c |   14 ++++--------
 include/fat.h      |    3 --
 3 files changed, 36 insertions(+), 39 deletions(-)

-- 
1.7.2.5

[U-Boot] [PATCH 1/2] FAT: remove ifdefs to make the code more readable

[Richard Genoud]

ifdefs in the code are making it harder to read.
The use of simple if(VFAT_ENABLED) makes no more code and is cleaner.
(the code is discarded by the compiler and linker instead of the
preprocessor.)

and bonus, now the code compiles even if CONFIG_SUPPORT_VFAT is not
defined.

Signed-off-by: Richard Genoud <richard.genoud@gmail.com>
---
 fs/fat/fat.c       |   55 +++++++++++++++++++++++++++------------------------
 fs/fat/fat_write.c |   11 ++-------
 2 files changed, 32 insertions(+), 34 deletions(-)

diff --git a/fs/fat/fat.c b/fs/fat/fat.c
index 393c378..c79e3e3 100644
--- a/fs/fat/fat.c
+++ b/fs/fat/fat.c
@@ -34,6 +34,12 @@
 #include <malloc.h>
 #include <linux/compiler.h>
 
+#ifdef CONFIG_SUPPORT_VFAT
+#define VFAT_ENABLED 1
+#else
+#define VFAT_ENABLED 0
+#endif
+
 /*
  * Convert a string to lowercase.
  */
@@ -441,7 +447,6 @@ getit:
 	} while (1);
 }
 
-#ifdef CONFIG_SUPPORT_VFAT
 /*
  * Extract the file name information from 'slotptr' into 'l_name',
  * starting at l_name[*idx].
@@ -576,7 +581,6 @@ static __u8 mkcksum(const char name[8], const char ext[3])
 
 	return ret;
 }
-#endif	/* CONFIG_SUPPORT_VFAT */
 
 /*
  * Get the directory entry associated with 'filename' from the directory
@@ -617,8 +621,8 @@ static dir_entry *get_dentfromdir(fsdata *mydata, int startsect,
 				continue;
 			}
 			if ((dentptr->attr & ATTR_VOLUME)) {
-#ifdef CONFIG_SUPPORT_VFAT
-				if ((dentptr->attr & ATTR_VFAT) == ATTR_VFAT &&
+				if (VFAT_ENABLED &&
+				    (dentptr->attr & ATTR_VFAT) == ATTR_VFAT &&
 				    (dentptr->name[0] & LAST_LONG_ENTRY_MASK)) {
 					prevcksum = ((dir_slot *)dentptr)->alias_checksum;
 					get_vfatname(mydata, curclust,
@@ -658,9 +662,7 @@ static dir_entry *get_dentfromdir(fsdata *mydata, int startsect,
 						continue;
 					}
 					debug("vfatname: |%s|\n", l_name);
-				} else
-#endif
-				{
+				} else {
 					/* Volume label or VFAT entry */
 					dentptr++;
 					continue;
@@ -674,14 +676,15 @@ static dir_entry *get_dentfromdir(fsdata *mydata, int startsect,
 				debug("Dentname == NULL - %d\n", i);
 				return NULL;
 			}
-#ifdef CONFIG_SUPPORT_VFAT
-			__u8 csum = mkcksum(dentptr->name, dentptr->ext);
-			if (dols && csum == prevcksum) {
-				prevcksum = 0xffff;
-				dentptr++;
-				continue;
+			if (VFAT_ENABLED) {
+				__u8 csum = mkcksum(dentptr->name, dentptr->ext);
+				if (dols && csum == prevcksum) {
+					prevcksum = 0xffff;
+					dentptr++;
+					continue;
+				}
 			}
-#endif
+
 			get_name(dentptr, s_name);
 			if (dols) {
 				int isdir = (dentptr->attr & ATTR_DIR);
@@ -884,9 +887,9 @@ do_fat_read_at(const char *filename, unsigned long pos, void *buffer,
 		return -1;
 	}
 
-#ifdef CONFIG_SUPPORT_VFAT
-	debug("VFAT Support enabled\n");
-#endif
+	if (VFAT_ENABLED)
+		debug("VFAT Support enabled\n");
+
 	debug("FAT%d, fat_sect: %d, fatlength: %d\n",
 	       mydata->fatsize, mydata->fat_sect, mydata->fatlength);
 	debug("Rootdir begins at cluster: %d, sector: %d, offset: %x\n"
@@ -952,10 +955,12 @@ do_fat_read_at(const char *filename, unsigned long pos, void *buffer,
 				continue;
 			}
 
-			csum = mkcksum(dentptr->name, dentptr->ext);
+			if (VFAT_ENABLED)
+				csum = mkcksum(dentptr->name, dentptr->ext);
+
 			if (dentptr->attr & ATTR_VOLUME) {
-#ifdef CONFIG_SUPPORT_VFAT
-				if ((dentptr->attr & ATTR_VFAT) == ATTR_VFAT &&
+				if (VFAT_ENABLED &&
+				    (dentptr->attr & ATTR_VFAT) == ATTR_VFAT &&
 				    (dentptr->name[0] & LAST_LONG_ENTRY_MASK)) {
 					prevcksum =
 						((dir_slot *)dentptr)->alias_checksum;
@@ -999,9 +1004,7 @@ do_fat_read_at(const char *filename, unsigned long pos, void *buffer,
 					}
 					debug("Rootvfatname: |%s|\n",
 					       l_name);
-				} else
-#endif
-				{
+				} else {
 					/* Volume label or VFAT entry */
 					dentptr++;
 					continue;
@@ -1015,13 +1018,13 @@ do_fat_read_at(const char *filename, unsigned long pos, void *buffer,
 				}
 				goto exit;
 			}
-#ifdef CONFIG_SUPPORT_VFAT
-			else if (dols == LS_ROOT && csum == prevcksum) {
+			else if (VFAT_ENABLED &&
+				 dols == LS_ROOT && csum == prevcksum) {
 				prevcksum = 0xffff;
 				dentptr++;
 				continue;
 			}
-#endif
+
 			get_name(dentptr, s_name);
 
 			if (dols == LS_ROOT) {
diff --git a/fs/fat/fat_write.c b/fs/fat/fat_write.c
index 4a1bda0..ed3eaa0 100644
--- a/fs/fat/fat_write.c
+++ b/fs/fat/fat_write.c
@@ -248,7 +248,6 @@ static __u32 get_fatent_value(fsdata *mydata, __u32 entry)
 	return ret;
 }
 
-#ifdef CONFIG_SUPPORT_VFAT
 /*
  * Set the file name information from 'name' into 'slotptr',
  */
@@ -468,8 +467,6 @@ get_long_file_name(fsdata *mydata, int curclust, __u8 *cluster,
 	return 0;
 }
 
-#endif
-
 /*
  * Set the entry at index 'entry' in a FAT (16/32) table.
  */
@@ -853,16 +850,14 @@ static dir_entry *find_directory_entry(fsdata *mydata, int startsect,
 				continue;
 			}
 			if ((dentptr->attr & ATTR_VOLUME)) {
-#ifdef CONFIG_SUPPORT_VFAT
-				if ((dentptr->attr & ATTR_VFAT) &&
+				if (VFAT_ENABLED &&
+				    (dentptr->attr & ATTR_VFAT) &&
 				    (dentptr->name[0] & LAST_LONG_ENTRY_MASK)) {
 					get_long_file_name(mydata, curclust,
 						     get_dentfromdir_block,
 						     &dentptr, l_name);
 					debug("vfatname: |%s|\n", l_name);
-				} else
-#endif
-				{
+				} else {
 					/* Volume label or VFAT entry */
 					dentptr++;
 					if (is_next_clust(mydata, dentptr))
-- 
1.7.2.5

[U-Boot] [PATCH 2/2] FAT: use toupper/tolower instead of recoding them

[Richard Genoud]

toupper/tolower function are already declared, so use them.

Signed-off-by: Richard Genoud <richard.genoud@gmail.com>
---
 fs/fat/fat.c       |    3 ++-
 fs/fat/fat_write.c |    3 ++-
 include/fat.h      |    3 ---
 3 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/fs/fat/fat.c b/fs/fat/fat.c
index c79e3e3..cff0526 100644
--- a/fs/fat/fat.c
+++ b/fs/fat/fat.c
@@ -33,6 +33,7 @@
 #include <part.h>
 #include <malloc.h>
 #include <linux/compiler.h>
+#include <linux/ctype.h>
 
 #ifdef CONFIG_SUPPORT_VFAT
 #define VFAT_ENABLED 1
@@ -46,7 +47,7 @@
 static void downcase(char *str)
 {
 	while (*str != '\0') {
-		TOLOWER(*str);
+		*str = tolower(*str);
 		str++;
 	}
 }
diff --git a/fs/fat/fat_write.c b/fs/fat/fat_write.c
index ed3eaa0..13c4de1 100644
--- a/fs/fat/fat_write.c
+++ b/fs/fat/fat_write.c
@@ -28,6 +28,7 @@
 #include <fat.h>
 #include <asm/byteorder.h>
 #include <part.h>
+#include <linux/ctype.h>
 #include "fat.c"
 
 static void uppercase(char *str, int len)
@@ -35,7 +36,7 @@ static void uppercase(char *str, int len)
 	int i;
 
 	for (i = 0; i < len; i++) {
-		TOUPPER(*str);
+		*str = toupper(*str);
 		str++;
 	}
 }
diff --git a/include/fat.h b/include/fat.h
index 706cd7a..b28c3fd 100644
--- a/include/fat.h
+++ b/include/fat.h
@@ -98,9 +98,6 @@
 #endif
 #endif
 
-#define TOLOWER(c)	if((c) >= 'A' && (c) <= 'Z'){(c)+=('a' - 'A');}
-#define TOUPPER(c)	if ((c) >= 'a' && (c) <= 'z') \
-				(c) -= ('a' - 'A');
 #define START(dent)	(FAT2CPU16((dent)->start) \
 			+ (mydata->fatsize != 32 ? 0 : \
 			  (FAT2CPU16((dent)->starthi) << 16)))
-- 
1.7.2.5

[U-Boot] [PATCH 0/2] clean some FAT code

[Marek Vasut]

Dear Richard Genoud,

> Hi,
> 
> Instead of adding some more ugly #ifdefs, I came with another approach.
> It makes the code easier to read, and correct the compilation error
> when VFAT wasn't enabled.

I feel bad for the code when you say it's FAT so explicitly ... it certainly 
wishes it was SLIM, but it can't ;-)

Best regards,
Marek Vasut

[U-Boot] [PATCH 1/2] FAT: remove ifdefs to make the code more readable

[Marek Vasut]

Dear Richard Genoud,

> ifdefs in the code are making it harder to read.
> The use of simple if(VFAT_ENABLED) makes no more code and is cleaner.
> (the code is discarded by the compiler and linker instead of the
> preprocessor.)
> 
> and bonus, now the code compiles even if CONFIG_SUPPORT_VFAT is not
> defined.
> 
> Signed-off-by: Richard Genoud <richard.genoud@gmail.com>
> ---
>  fs/fat/fat.c       |   55
> +++++++++++++++++++++++++++------------------------ fs/fat/fat_write.c |  
> 11 ++-------
>  2 files changed, 32 insertions(+), 34 deletions(-)
> 
> diff --git a/fs/fat/fat.c b/fs/fat/fat.c
> index 393c378..c79e3e3 100644
> --- a/fs/fat/fat.c
> +++ b/fs/fat/fat.c
> @@ -34,6 +34,12 @@
>  #include <malloc.h>
>  #include <linux/compiler.h>
> 
> +#ifdef CONFIG_SUPPORT_VFAT
> +#define VFAT_ENABLED 1
> +#else
> +#define VFAT_ENABLED 0
> +#endif
[...]

Make it static const int maybe ?

Best regards,
Marek Vasut

[U-Boot] [PATCH 2/2] FAT: use toupper/tolower instead of recoding them

[Marek Vasut]

Dear Richard Genoud,

> toupper/tolower function are already declared, so use them.
> 
> Signed-off-by: Richard Genoud <richard.genoud@gmail.com>
> ---
>  fs/fat/fat.c       |    3 ++-
>  fs/fat/fat_write.c |    3 ++-
>  include/fat.h      |    3 ---
>  3 files changed, 4 insertions(+), 5 deletions(-)

Acked-by: Marek Vasut <marex@denx.de>

Best regards,
Marek Vasut

[U-Boot] [PATCH 0/2] clean some FAT code

[Richard Genoud]

2012/12/13 Marek Vasut <marex@denx.de>:
> Dear Richard Genoud,
>
> I feel bad for the code when you say it's FAT so explicitly ... it certainly
> wishes it was SLIM, but it can't ;-)
>
:) I didn't write that on purpose... But it's definitely a good war cry !

[U-Boot] [PATCH 1/2] FAT: remove ifdefs to make the code more readable

[Richard Genoud]

2012/12/13 Marek Vasut <marex@denx.de>:
> Dear Richard Genoud,
>
>> ifdefs in the code are making it harder to read.
>> The use of simple if(VFAT_ENABLED) makes no more code and is cleaner.
>> (the code is discarded by the compiler and linker instead of the
>> preprocessor.)
>>
>> and bonus, now the code compiles even if CONFIG_SUPPORT_VFAT is not
>> defined.
>>
>> Signed-off-by: Richard Genoud <richard.genoud@gmail.com>
>> ---
>>  fs/fat/fat.c       |   55
>> +++++++++++++++++++++++++++------------------------ fs/fat/fat_write.c |
>> 11 ++-------
>>  2 files changed, 32 insertions(+), 34 deletions(-)
>>
>> diff --git a/fs/fat/fat.c b/fs/fat/fat.c
>> index 393c378..c79e3e3 100644
>> --- a/fs/fat/fat.c
>> +++ b/fs/fat/fat.c
>> @@ -34,6 +34,12 @@
>>  #include <malloc.h>
>>  #include <linux/compiler.h>
>>
>> +#ifdef CONFIG_SUPPORT_VFAT
>> +#define VFAT_ENABLED 1
>> +#else
>> +#define VFAT_ENABLED 0
>> +#endif
> [...]
>
> Make it static const int maybe ?
I hesitate to make them static const, I can't figure why it's better
to use static const variable instead of defines.
Could you enlighten me ?

Best regards,
Richard.

[U-Boot] [PATCH 1/2] FAT: remove ifdefs to make the code more readable

[Tom Rini]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 12/13/12 05:47, Richard Genoud wrote:
> ifdefs in the code are making it harder to read. The use of simple
> if(VFAT_ENABLED) makes no more code and is cleaner. (the code is
> discarded by the compiler and linker instead of the preprocessor.)
> 
> and bonus, now the code compiles even if CONFIG_SUPPORT_VFAT is
> not defined.
> 
> Signed-off-by: Richard Genoud <richard.genoud@gmail.com> --- 
> fs/fat/fat.c       |   55
> +++++++++++++++++++++++++++------------------------ 
> fs/fat/fat_write.c |   11 ++------- 2 files changed, 32
> insertions(+), 34 deletions(-)
> 
> diff --git a/fs/fat/fat.c b/fs/fat/fat.c index 393c378..c79e3e3
> 100644 --- a/fs/fat/fat.c +++ b/fs/fat/fat.c @@ -34,6 +34,12 @@ 
> #include <malloc.h> #include <linux/compiler.h>
> 
> +#ifdef CONFIG_SUPPORT_VFAT +#define VFAT_ENABLED 1 +#else +#define
> VFAT_ENABLED 0 +#endif + /* * Convert a string to lowercase. */ @@
> -441,7 +447,6 @@ getit: } while (1); }
> 
> -#ifdef CONFIG_SUPPORT_VFAT /* * Extract the file name information
> from 'slotptr' into 'l_name', * starting at l_name[*idx]. @@ -576,7
> +581,6 @@ static __u8 mkcksum(const char name[8], const char
> ext[3])
> 
> return ret; } -#endif	/* CONFIG_SUPPORT_VFAT */
> 
> /* * Get the directory entry associated with 'filename' from the
> directory @@ -617,8 +621,8 @@ static dir_entry
> *get_dentfromdir(fsdata *mydata, int startsect, continue; } if
> ((dentptr->attr & ATTR_VOLUME)) { -#ifdef CONFIG_SUPPORT_VFAT -
> if ((dentptr->attr & ATTR_VFAT) == ATTR_VFAT && +				if
> (VFAT_ENABLED && +				    (dentptr->attr & ATTR_VFAT) == ATTR_VFAT
> && (dentptr->name[0] & LAST_LONG_ENTRY_MASK)) { prevcksum =
> ((dir_slot *)dentptr)->alias_checksum; get_vfatname(mydata,
> curclust, @@ -658,9 +662,7 @@ static dir_entry
> *get_dentfromdir(fsdata *mydata, int startsect, continue; } 
> debug("vfatname: |%s|\n", l_name); -				} else -#endif -				{ +
> } else { /* Volume label or VFAT entry */ dentptr++; continue; @@
> -674,14 +676,15 @@ static dir_entry *get_dentfromdir(fsdata
> *mydata, int startsect, debug("Dentname == NULL - %d\n", i); return
> NULL; } -#ifdef CONFIG_SUPPORT_VFAT -			__u8 csum =
> mkcksum(dentptr->name, dentptr->ext); -			if (dols && csum ==
> prevcksum) { -				prevcksum = 0xffff; -				dentptr++; -
> continue; +			if (VFAT_ENABLED) { +				__u8 csum =
> mkcksum(dentptr->name, dentptr->ext); +				if (dols && csum ==
> prevcksum) { +					prevcksum = 0xffff; +					dentptr++; +
> continue; +				} } -#endif + get_name(dentptr, s_name); if (dols)
> { int isdir = (dentptr->attr & ATTR_DIR); @@ -884,9 +887,9 @@
> do_fat_read_at(const char *filename, unsigned long pos, void
> *buffer, return -1; }
> 
> -#ifdef CONFIG_SUPPORT_VFAT -	debug("VFAT Support enabled\n"); 
> -#endif +	if (VFAT_ENABLED) +		debug("VFAT Support enabled\n"); + 
> debug("FAT%d, fat_sect: %d, fatlength: %d\n", mydata->fatsize,
> mydata->fat_sect, mydata->fatlength); debug("Rootdir begins at
> cluster: %d, sector: %d, offset: %x\n" @@ -952,10 +955,12 @@
> do_fat_read_at(const char *filename, unsigned long pos, void
> *buffer, continue; }
> 
> -			csum = mkcksum(dentptr->name, dentptr->ext); +			if
> (VFAT_ENABLED) +				csum = mkcksum(dentptr->name, dentptr->ext); + 
> if (dentptr->attr & ATTR_VOLUME) { -#ifdef CONFIG_SUPPORT_VFAT -
> if ((dentptr->attr & ATTR_VFAT) == ATTR_VFAT && +				if
> (VFAT_ENABLED && +				    (dentptr->attr & ATTR_VFAT) == ATTR_VFAT
> && (dentptr->name[0] & LAST_LONG_ENTRY_MASK)) { prevcksum = 
> ((dir_slot *)dentptr)->alias_checksum; @@ -999,9 +1004,7 @@
> do_fat_read_at(const char *filename, unsigned long pos, void
> *buffer, } debug("Rootvfatname: |%s|\n", l_name); -				} else 
> -#endif -				{ +				} else { /* Volume label or VFAT entry */ 
> dentptr++; continue; @@ -1015,13 +1018,13 @@ do_fat_read_at(const
> char *filename, unsigned long pos, void *buffer, } goto exit; } 
> -#ifdef CONFIG_SUPPORT_VFAT -			else if (dols == LS_ROOT && csum ==
> prevcksum) { +			else if (VFAT_ENABLED && +				 dols == LS_ROOT &&
> csum == prevcksum) { prevcksum = 0xffff; dentptr++; continue; } 
> -#endif + get_name(dentptr, s_name);
> 
> if (dols == LS_ROOT) { diff --git a/fs/fat/fat_write.c
> b/fs/fat/fat_write.c index 4a1bda0..ed3eaa0 100644 ---
> a/fs/fat/fat_write.c +++ b/fs/fat/fat_write.c @@ -248,7 +248,6 @@
> static __u32 get_fatent_value(fsdata *mydata, __u32 entry) return
> ret; }
> 
> -#ifdef CONFIG_SUPPORT_VFAT /* * Set the file name information from
> 'name' into 'slotptr', */ @@ -468,8 +467,6 @@
> get_long_file_name(fsdata *mydata, int curclust, __u8 *cluster, 
> return 0; }
> 
> -#endif

Note that we don't use --gc-sections on all archs so I'm not sure we
discard the unused VFAT functions on say ARM.

- -- 
Tom
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://www.enigmail.net/

iQIcBAEBAgAGBQJQydOVAAoJENk4IS6UOR1WDYQP/2P6ywBeeE/qnx9C13qJnCcr
wXG1IJWEpjgAEOViQZ77kE5Knn9ltiuXKj6EYc7e3oY4S2mBREkG/sHNmG+e+9/0
WHuZJoaiLPaia9ztL/d8BCyVPkSbJ88iyA20TgefR94j7SqITiVT+3yDA4XEB3bW
C8rXZdmYzGjLzMBPI458uxNmIKSeHVfUZnDdWdA0rIOK8t6QN+OKLJRMezOYwx8e
jM8Td4hDmJ20EGfFTpcO2spdUMCqi6FfO6bXJHeFHeUqGXqz482xyIxiZGpETut9
tFOVegHIpbKf0eBoMvs+3ajHprAsMGFlAqv0YSxh4w04ZIXUBT+DibjGiQtf/Xjl
HDvAkBGEbQjljZaQDYlaAfNWFctU7lkapujiMuOKjv04DvClp0AgKEvOjAUW/P2N
ERIQzc67dUdomhmdl0sl+yFYJDB4zjT1VnlYXGx/yz4KLIiD5t+gaQAyq3pGwYjH
jZ3q5gAprYNR0VKl6QM8gQFdAjUKT0owJqUb0iZ2YqblIr+IHLIFHddhwOHB76cg
ViLKtbJ7Y43aShdQUE2t8Ibth+ywba3EDBaieqkz4Q928k//kScGSal6nxqE4mhR
AZ4ca5euEpADicacHZmKNSbtgeBYvhsC/c4ly27hdLPQjpAGOT/axPoaIfUxIe9H
PKIJx4VC14A2u4tS1A6Q
=1PBB
-----END PGP SIGNATURE-----

[U-Boot] [PATCH 1/2] FAT: remove ifdefs to make the code more readable

[Marek Vasut]

Dear Richard Genoud,

> 2012/12/13 Marek Vasut <marex@denx.de>:
> > Dear Richard Genoud,
> > 
> >> ifdefs in the code are making it harder to read.
> >> The use of simple if(VFAT_ENABLED) makes no more code and is cleaner.
> >> (the code is discarded by the compiler and linker instead of the
> >> preprocessor.)
> >> 
> >> and bonus, now the code compiles even if CONFIG_SUPPORT_VFAT is not
> >> defined.
> >> 
> >> Signed-off-by: Richard Genoud <richard.genoud@gmail.com>
> >> ---
> >> 
> >>  fs/fat/fat.c       |   55
> >> 
> >> +++++++++++++++++++++++++++------------------------ fs/fat/fat_write.c |
> >> 11 ++-------
> >> 
> >>  2 files changed, 32 insertions(+), 34 deletions(-)
> >> 
> >> diff --git a/fs/fat/fat.c b/fs/fat/fat.c
> >> index 393c378..c79e3e3 100644
> >> --- a/fs/fat/fat.c
> >> +++ b/fs/fat/fat.c
> >> @@ -34,6 +34,12 @@
> >> 
> >>  #include <malloc.h>
> >>  #include <linux/compiler.h>
> >> 
> >> +#ifdef CONFIG_SUPPORT_VFAT
> >> +#define VFAT_ENABLED 1
> >> +#else
> >> +#define VFAT_ENABLED 0
> >> +#endif
> > 
> > [...]
> > 
> > Make it static const int maybe ?
> 
> I hesitate to make them static const, I can't figure why it's better
> to use static const variable instead of defines.
> Could you enlighten me ?

Because you get the type-checking. Preprocessor is evil ;-)

Best regards,
Marek Vasut

[U-Boot] [PATCH 1/2] FAT: remove ifdefs to make the code more readable

[Richard Genoud]

2012/12/13 Tom Rini <trini@ti.com>:
> Note that we don't use --gc-sections on all archs so I'm not sure we
> discard the unused VFAT functions on say ARM.
>
I tested it on at91sam9x5ek, and the vfat functions are not present in
the System.map.

The problem is in my commit message: the linker doesn"t have anything
to do with that because all discarded functions are static.

So I think it's ok on all archs (but maybe not with -O0)

Richard

[U-Boot] [PATCH 1/2] FAT: remove ifdefs to make the code more readable

[Richard Genoud]

2012/12/13 Marek Vasut <marex@denx.de>:
> Dear Richard Genoud,
>
>> 2012/12/13 Marek Vasut <marex@denx.de>:
>> > Dear Richard Genoud,
>> >
>> >> ifdefs in the code are making it harder to read.
>> >> The use of simple if(VFAT_ENABLED) makes no more code and is cleaner.
>> >> (the code is discarded by the compiler and linker instead of the
>> >> preprocessor.)
>> >>
>> >> and bonus, now the code compiles even if CONFIG_SUPPORT_VFAT is not
>> >> defined.
>> >>
>> >> Signed-off-by: Richard Genoud <richard.genoud@gmail.com>
>> >> ---
>> >>
>> >>  fs/fat/fat.c       |   55
>> >>
>> >> +++++++++++++++++++++++++++------------------------ fs/fat/fat_write.c |
>> >> 11 ++-------
>> >>
>> >>  2 files changed, 32 insertions(+), 34 deletions(-)
>> >>
>> >> diff --git a/fs/fat/fat.c b/fs/fat/fat.c
>> >> index 393c378..c79e3e3 100644
>> >> --- a/fs/fat/fat.c
>> >> +++ b/fs/fat/fat.c
>> >> @@ -34,6 +34,12 @@
>> >>
>> >>  #include <malloc.h>
>> >>  #include <linux/compiler.h>
>> >>
>> >> +#ifdef CONFIG_SUPPORT_VFAT
>> >> +#define VFAT_ENABLED 1
>> >> +#else
>> >> +#define VFAT_ENABLED 0
>> >> +#endif
>> >
>> > [...]
>> >
>> > Make it static const int maybe ?
>>
>> I hesitate to make them static const, I can't figure why it's better
>> to use static const variable instead of defines.
>> Could you enlighten me ?
>
> Because you get the type-checking. Preprocessor is evil ;-)
>
> Best regards,
> Marek Vasut

Ok ! Thanks, I'll resend it with the change.


Richard.

[U-Boot] [PATCH 1/2] FAT: remove ifdefs to make the code more readable

[Marek Vasut]

Dear Richard Genoud,

> 2012/12/13 Marek Vasut <marex@denx.de>:
> > Dear Richard Genoud,
> > 
> >> 2012/12/13 Marek Vasut <marex@denx.de>:
> >> > Dear Richard Genoud,
> >> > 
> >> >> ifdefs in the code are making it harder to read.
> >> >> The use of simple if(VFAT_ENABLED) makes no more code and is cleaner.
> >> >> (the code is discarded by the compiler and linker instead of the
> >> >> preprocessor.)
> >> >> 
> >> >> and bonus, now the code compiles even if CONFIG_SUPPORT_VFAT is not
> >> >> defined.
> >> >> 
> >> >> Signed-off-by: Richard Genoud <richard.genoud@gmail.com>
> >> >> ---
> >> >> 
> >> >>  fs/fat/fat.c       |   55
> >> >> 
> >> >> +++++++++++++++++++++++++++------------------------
> >> >> fs/fat/fat_write.c | 11 ++-------
> >> >> 
> >> >>  2 files changed, 32 insertions(+), 34 deletions(-)
> >> >> 
> >> >> diff --git a/fs/fat/fat.c b/fs/fat/fat.c
> >> >> index 393c378..c79e3e3 100644
> >> >> --- a/fs/fat/fat.c
> >> >> +++ b/fs/fat/fat.c
> >> >> @@ -34,6 +34,12 @@
> >> >> 
> >> >>  #include <malloc.h>
> >> >>  #include <linux/compiler.h>
> >> >> 
> >> >> +#ifdef CONFIG_SUPPORT_VFAT
> >> >> +#define VFAT_ENABLED 1
> >> >> +#else
> >> >> +#define VFAT_ENABLED 0
> >> >> +#endif
> >> > 
> >> > [...]
> >> > 
> >> > Make it static const int maybe ?
> >> 
> >> I hesitate to make them static const, I can't figure why it's better
> >> to use static const variable instead of defines.
> >> Could you enlighten me ?
> > 
> > Because you get the type-checking. Preprocessor is evil ;-)
> > 
> > Best regards,
> > Marek Vasut
> 
> Ok ! Thanks, I'll resend it with the change.

Thanks!

Yes, preprocessor eats kittens too ;-)

Best regards,
Marek Vasut

[U-Boot] [PATCHv2] FAT: remove ifdefs to make the code more readable

[Richard Genoud]

ifdefs in the code are making it harder to read.
The use of simple if(vfat_enabled) makes no more code and is cleaner.
(the code is discarded by the compiler instead of the preprocessor.)
NB: if -O0 is used, the code won't be discarded

and bonus, now the code compiles even if CONFIG_SUPPORT_VFAT is not
defined.

Signed-off-by: Richard Genoud <richard.genoud@gmail.com>
---
 fs/fat/fat.c       |   55 +++++++++++++++++++++++++++------------------------
 fs/fat/fat_write.c |   11 ++-------
 2 files changed, 32 insertions(+), 34 deletions(-)

diff --git a/fs/fat/fat.c b/fs/fat/fat.c
index 393c378..7c23b67 100644
--- a/fs/fat/fat.c
+++ b/fs/fat/fat.c
@@ -34,6 +34,12 @@
 #include <malloc.h>
 #include <linux/compiler.h>
 
+#ifdef CONFIG_SUPPORT_VFAT
+static const int vfat_enabled = 1;
+#else
+static const int vfat_enabled = 0;
+#endif
+
 /*
  * Convert a string to lowercase.
  */
@@ -441,7 +447,6 @@ getit:
 	} while (1);
 }
 
-#ifdef CONFIG_SUPPORT_VFAT
 /*
  * Extract the file name information from 'slotptr' into 'l_name',
  * starting at l_name[*idx].
@@ -576,7 +581,6 @@ static __u8 mkcksum(const char name[8], const char ext[3])
 
 	return ret;
 }
-#endif	/* CONFIG_SUPPORT_VFAT */
 
 /*
  * Get the directory entry associated with 'filename' from the directory
@@ -617,8 +621,8 @@ static dir_entry *get_dentfromdir(fsdata *mydata, int startsect,
 				continue;
 			}
 			if ((dentptr->attr & ATTR_VOLUME)) {
-#ifdef CONFIG_SUPPORT_VFAT
-				if ((dentptr->attr & ATTR_VFAT) == ATTR_VFAT &&
+				if (vfat_enabled &&
+				    (dentptr->attr & ATTR_VFAT) == ATTR_VFAT &&
 				    (dentptr->name[0] & LAST_LONG_ENTRY_MASK)) {
 					prevcksum = ((dir_slot *)dentptr)->alias_checksum;
 					get_vfatname(mydata, curclust,
@@ -658,9 +662,7 @@ static dir_entry *get_dentfromdir(fsdata *mydata, int startsect,
 						continue;
 					}
 					debug("vfatname: |%s|\n", l_name);
-				} else
-#endif
-				{
+				} else {
 					/* Volume label or VFAT entry */
 					dentptr++;
 					continue;
@@ -674,14 +676,15 @@ static dir_entry *get_dentfromdir(fsdata *mydata, int startsect,
 				debug("Dentname == NULL - %d\n", i);
 				return NULL;
 			}
-#ifdef CONFIG_SUPPORT_VFAT
-			__u8 csum = mkcksum(dentptr->name, dentptr->ext);
-			if (dols && csum == prevcksum) {
-				prevcksum = 0xffff;
-				dentptr++;
-				continue;
+			if (vfat_enabled) {
+				__u8 csum = mkcksum(dentptr->name, dentptr->ext);
+				if (dols && csum == prevcksum) {
+					prevcksum = 0xffff;
+					dentptr++;
+					continue;
+				}
 			}
-#endif
+
 			get_name(dentptr, s_name);
 			if (dols) {
 				int isdir = (dentptr->attr & ATTR_DIR);
@@ -884,9 +887,9 @@ do_fat_read_at(const char *filename, unsigned long pos, void *buffer,
 		return -1;
 	}
 
-#ifdef CONFIG_SUPPORT_VFAT
-	debug("VFAT Support enabled\n");
-#endif
+	if (vfat_enabled)
+		debug("VFAT Support enabled\n");
+
 	debug("FAT%d, fat_sect: %d, fatlength: %d\n",
 	       mydata->fatsize, mydata->fat_sect, mydata->fatlength);
 	debug("Rootdir begins at cluster: %d, sector: %d, offset: %x\n"
@@ -952,10 +955,12 @@ do_fat_read_at(const char *filename, unsigned long pos, void *buffer,
 				continue;
 			}
 
-			csum = mkcksum(dentptr->name, dentptr->ext);
+			if (vfat_enabled)
+				csum = mkcksum(dentptr->name, dentptr->ext);
+
 			if (dentptr->attr & ATTR_VOLUME) {
-#ifdef CONFIG_SUPPORT_VFAT
-				if ((dentptr->attr & ATTR_VFAT) == ATTR_VFAT &&
+				if (vfat_enabled &&
+				    (dentptr->attr & ATTR_VFAT) == ATTR_VFAT &&
 				    (dentptr->name[0] & LAST_LONG_ENTRY_MASK)) {
 					prevcksum =
 						((dir_slot *)dentptr)->alias_checksum;
@@ -999,9 +1004,7 @@ do_fat_read_at(const char *filename, unsigned long pos, void *buffer,
 					}
 					debug("Rootvfatname: |%s|\n",
 					       l_name);
-				} else
-#endif
-				{
+				} else {
 					/* Volume label or VFAT entry */
 					dentptr++;
 					continue;
@@ -1015,13 +1018,13 @@ do_fat_read_at(const char *filename, unsigned long pos, void *buffer,
 				}
 				goto exit;
 			}
-#ifdef CONFIG_SUPPORT_VFAT
-			else if (dols == LS_ROOT && csum == prevcksum) {
+			else if (vfat_enabled &&
+				 dols == LS_ROOT && csum == prevcksum) {
 				prevcksum = 0xffff;
 				dentptr++;
 				continue;
 			}
-#endif
+
 			get_name(dentptr, s_name);
 
 			if (dols == LS_ROOT) {
diff --git a/fs/fat/fat_write.c b/fs/fat/fat_write.c
index 4a1bda0..e7cda17 100644
--- a/fs/fat/fat_write.c
+++ b/fs/fat/fat_write.c
@@ -248,7 +248,6 @@ static __u32 get_fatent_value(fsdata *mydata, __u32 entry)
 	return ret;
 }
 
-#ifdef CONFIG_SUPPORT_VFAT
 /*
  * Set the file name information from 'name' into 'slotptr',
  */
@@ -468,8 +467,6 @@ get_long_file_name(fsdata *mydata, int curclust, __u8 *cluster,
 	return 0;
 }
 
-#endif
-
 /*
  * Set the entry at index 'entry' in a FAT (16/32) table.
  */
@@ -853,16 +850,14 @@ static dir_entry *find_directory_entry(fsdata *mydata, int startsect,
 				continue;
 			}
 			if ((dentptr->attr & ATTR_VOLUME)) {
-#ifdef CONFIG_SUPPORT_VFAT
-				if ((dentptr->attr & ATTR_VFAT) &&
+				if (vfat_enabled &&
+				    (dentptr->attr & ATTR_VFAT) &&
 				    (dentptr->name[0] & LAST_LONG_ENTRY_MASK)) {
 					get_long_file_name(mydata, curclust,
 						     get_dentfromdir_block,
 						     &dentptr, l_name);
 					debug("vfatname: |%s|\n", l_name);
-				} else
-#endif
-				{
+				} else {
 					/* Volume label or VFAT entry */
 					dentptr++;
 					if (is_next_clust(mydata, dentptr))
-- 
1.7.2.5

[U-Boot] [PATCH 1/2] FAT: remove ifdefs to make the code more readable

[Marek Vasut]

Dear Tom Rini,

[...]

> Note that we don't use --gc-sections on all archs so I'm not sure we
> discard the unused VFAT functions on say ARM.

Valid point, Albert?

Best regards,
Marek Vasut

[U-Boot] [PATCH 1/2] FAT: remove ifdefs to make the code more readable

[Richard Genoud]

2012/12/13 Marek Vasut <marex@denx.de>:
> Dear Tom Rini,
>
> [...]
>
>> Note that we don't use --gc-sections on all archs so I'm not sure we
>> discard the unused VFAT functions on say ARM.
>
> Valid point, Albert?
>
> Best regards,
> Marek Vasut

I check and the code is discarded (on ARM at91sam9x5ek).
This is the case because the function to discard are all in the same
file (fat.c or fat_write.c).
The compiler will discard unused function with -Os. (but not with -O0)

[U-Boot] [PATCH 1/2] FAT: remove ifdefs to make the code more readable

[Tom Rini]

On Thu, Dec 13, 2012 at 03:24:08PM +0100, Richard Genoud wrote:
> 2012/12/13 Marek Vasut <marex@denx.de>:
> > Dear Tom Rini,
> >
> > [...]
> >
> >> Note that we don't use --gc-sections on all archs so I'm not sure we
> >> discard the unused VFAT functions on say ARM.
> >
> > Valid point, Albert?
> >
> > Best regards,
> > Marek Vasut
> 
> I check and the code is discarded (on ARM at91sam9x5ek).
> This is the case because the function to discard are all in the same
> file (fat.c or fat_write.c).
> The compiler will discard unused function with -Os. (but not with -O0)

Thanks for checking.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20121213/79a23308/attachment.pgp>

[U-Boot] [PATCH 2/2] FAT: use toupper/tolower instead of recoding them

[Stefano Babic]

On 13/12/2012 11:47, Richard Genoud wrote:
> toupper/tolower function are already declared, so use them.
> 
> Signed-off-by: Richard Genoud <richard.genoud@gmail.com>
> ---
>  fs/fat/fat.c       |    3 ++-
>  fs/fat/fat_write.c |    3 ++-
>  include/fat.h      |    3 ---
>  3 files changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/fat/fat.c b/fs/fat/fat.c
> index c79e3e3..cff0526 100644
> --- a/fs/fat/fat.c
> +++ b/fs/fat/fat.c
> @@ -33,6 +33,7 @@
>  #include <part.h>
>  #include <malloc.h>
>  #include <linux/compiler.h>
> +#include <linux/ctype.h>
>  
>  #ifdef CONFIG_SUPPORT_VFAT
>  #define VFAT_ENABLED 1
> @@ -46,7 +47,7 @@
>  static void downcase(char *str)
>  {
>  	while (*str != '\0') {
> -		TOLOWER(*str);
> +		*str = tolower(*str);
>  		str++;
>  	}
>  }
> diff --git a/fs/fat/fat_write.c b/fs/fat/fat_write.c
> index ed3eaa0..13c4de1 100644
> --- a/fs/fat/fat_write.c
> +++ b/fs/fat/fat_write.c
> @@ -28,6 +28,7 @@
>  #include <fat.h>
>  #include <asm/byteorder.h>
>  #include <part.h>
> +#include <linux/ctype.h>
>  #include "fat.c"
>  
>  static void uppercase(char *str, int len)
> @@ -35,7 +36,7 @@ static void uppercase(char *str, int len)
>  	int i;
>  
>  	for (i = 0; i < len; i++) {
> -		TOUPPER(*str);
> +		*str = toupper(*str);
>  		str++;
>  	}
>  }
> diff --git a/include/fat.h b/include/fat.h
> index 706cd7a..b28c3fd 100644
> --- a/include/fat.h
> +++ b/include/fat.h
> @@ -98,9 +98,6 @@
>  #endif
>  #endif
>  
> -#define TOLOWER(c)	if((c) >= 'A' && (c) <= 'Z'){(c)+=('a' - 'A');}
> -#define TOUPPER(c)	if ((c) >= 'a' && (c) <= 'z') \
> -				(c) -= ('a' - 'A');
>  #define START(dent)	(FAT2CPU16((dent)->start) \
>  			+ (mydata->fatsize != 32 ? 0 : \
>  			  (FAT2CPU16((dent)->starthi) << 16)))
> 

Acked-by: Stefano Babic <sbabic@denx.de>

Best regards,
Stefano Babic

-- 
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================

[U-Boot] [U-Boot, 2/2] FAT: use toupper/tolower instead of recoding them

[Tom Rini]

On Thu, Dec 13, 2012 at 12:47:36AM -0000, Richard Genoud wrote:

> toupper/tolower function are already declared, so use them.
> 
> Signed-off-by: Richard Genoud <richard.genoud@gmail.com>
> Acked-by: Marek Vasut <marex@denx.de>
> Acked-by: Stefano Babic <sbabic@denx.de>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20130204/4caeb4b1/attachment.pgp>

[U-Boot] [U-Boot, PATCHv2] FAT: remove ifdefs to make the code more readable

[Tom Rini]

On Thu, Dec 13, 2012 at 03:30:10AM -0000, Richard Genoud wrote:

> ifdefs in the code are making it harder to read.
> The use of simple if(vfat_enabled) makes no more code and is cleaner.
> (the code is discarded by the compiler instead of the preprocessor.)
> NB: if -O0 is used, the code won't be discarded
> 
> and bonus, now the code compiles even if CONFIG_SUPPORT_VFAT is not
> defined.
> 
> Signed-off-by: Richard Genoud <richard.genoud@gmail.com>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20130204/17ccf778/attachment.pgp>