Re: [U-Boot] [PATCH v7 01/10] nds32: add header files support for nds32
Dear Macpaul Lin,
In message BANLkTimhCuOcvMmFrSJGHxYRHyKFXJTc9g@mail.gmail.com you wrote:
Checkpatch complains a lot about "do not add new typedefs".
Indeed, but this seems is special for Linux Kernel,
Not really. This is Linux CodingStyle policy, which we usually adapt.
I've checked some of the "typedefs" from other architecture code in u-boot.
This does not mean much. Yes, there are tons of typedef's in U-Boot, but this is old code that has not been cleaned up yet. At least we now try not to add to that pool.
I did check typedefs one by one by myself in hand and eye checking. If some thing is not suitable for using "typedefs" please let me know.
Please don't add any new typedef's.
+#define PTREGS(reg) [reg]
This also triggers an erro-r from checkpatch, and indeed this is a strange define.
It was strange for me, too. However, it looks like I must use PTREGS define for ptregs for API compatibility. Please refer to the ARM code and MIPS, etc.
I did check. There is not any code anywhere that references anything like "ptreg" or similar.
NAK. Please use a C struct instead.
Other architecture use specific ARM_XXX_R0 or MIPS_XXX _R.
I'm not sure what you might be referring to. The only other file which I can find which has similar code is arch/arm/include/asm/proc-armv/ptrace.h, and this indeed should be cleaned up, too.
Sorry, sometimes such things slip through the reviews.
And I think this part is specific for the API to ptregs.h.
It appears this is something no other architecture needs in U-Boot?
Best regards,
Wolfgang Denk
Hi Wolfgang,
Checkpatch complains a lot about "do not add new typedefs".
Indeed, but this seems is special for Linux Kernel,
Not really. This is Linux CodingStyle policy, which we usually adapt.
I've checked some of the "typedefs" from other architecture code in u-boot.
This does not mean much. Yes, there are tons of typedef's in U-Boot, but this is old code that has not been cleaned up yet. At least we now try not to add to that pool.
I did check typedefs one by one by myself in hand and eye checking. If some thing is not suitable for using "typedefs" please let me know.
Please don't add any new typedef's.
I think we still have to discuss about the typedef's. What does the "new" typedef means?
According to the checkpatch result, "typedef" warning exists in 4 files. arch/nds32/include/asm/posix_types.h arch/nds32/include/asm/types.h arch/nds32/include/asm/global_data.h arch/nds32/include/asm/u-boot.h.
File arch/nds32/include/asm/posix_types.h and arch/nds32/include/asm/types.h come from the Linux kernel. Which is usually used for posix compatibility for Linux Kernel. Which should be "old" features for posix and compatibility. However, you cannot say for a new architecture to support posix and other compatibility as "new" typedef. I've checked the latest kernel (2.6.38.1), arm, mips, avr32, powerpc consist these posix_types.h and types.h with "typedef".
It looks the kernel is not going to fix the "old" typedef for posix_types.h and types.h
I think they say "please do not add any new typedef" might mean to those typedef used in drivers or protocols.
In the other 2 files arch/nds32/include/asm/global_data.h and arch/nds32/include/asm/u-boot.h, typedef was used for #449: FILE: arch/nds32/include/asm/global_data.h:46: +typedef struct global_data { +} gd_t; #1505: FILE: arch/nds32/include/asm/u-boot.h:41: +typedef struct bd_info { +} bd_t;
I don't know if you have any idea of fixing it in u-boot.
If you have an explicit way to fix it, for example, "we must declare bd_t in each function before we use it", I'll very glad to do it for fixing up the coding style. Otherwise I'm afraid of the fixing "typedef" here in these 2 file might lead function or other problem in u-boot build with nds32.
Thanks.
Hi Wolfgang,
I did check typedefs one by one by myself in hand and eye checking. If some thing is not suitable for using "typedefs" please let me know.
Please don't add any new typedef's.
I think we still have to discuss about the typedef's. What does the "new" typedef means?
According to the checkpatch result, "typedef" warning exists in 4 files. arch/nds32/include/asm/posix_types.h arch/nds32/include/asm/types.h arch/nds32/include/asm/global_data.h arch/nds32/include/asm/u-boot.h.
I've found the origin purpose of adding typedef check to checkpatch.pl from the author "apw". Please refer to url "http://lkml.indiana.edu/hypermail/linux/kernel/0801.0/0354.html" I quote his words as below.
Andy Whitcroft (apw said..) "It is checkpatch's role to point out things which are likely to be wrong. There will always be exceptions. Lines which are much more readable if they spill over 80 characters, typedefs which do make sense. atomic_t's for example. This may well be a valid use of them. Note that this is mentioned as a WARNING not an ERROR. As is stated in the patch submission notes, you are meant to be comfortable with everything which checkpatch is still reporting.
checkpatch is a style _guide_, not the be all and end all. It is meant to carry a preferred style to try and maintain some consistency kernel wide."
There is also a section of the coding style which related to "typedef" in Linux Kernel's Documents.
Please also refer to "Chapter 5, typedef" in "Documentation/CodingStyle".
Hope this information is helping other patch submitters. Thanks.
Dear Macpaul Lin,
In message BANLkTi=haZVW17eNWYCmYf3McrGmbm-4Xw@mail.gmail.com you wrote:
I think we still have to discuss about the typedef's. What does the "new" typedef means?
It means adding any new code to U-Boot which includes "typedef"s.
According to the checkpatch result, "typedef" warning exists in 4 files. arch/nds32/include/asm/posix_types.h arch/nds32/include/asm/types.h arch/nds32/include/asm/global_data.h arch/nds32/include/asm/u-boot.h.
File arch/nds32/include/asm/posix_types.h and arch/nds32/include/asm/types.h come from the Linux kernel. Which is usually used for posix compatibility for Linux Kernel.
So these files should be fixed in Linux, too, because it is the Linux checkpatch tool which throws this warning.
I don't think this affects POSIX compatibility. Which typedef's are required by POSIX?
Which should be "old" features for posix and compatibility. However, you cannot say for a new architecture to support posix and other compatibility as "new" typedef. I've checked the latest kernel (2.6.38.1), arm, mips, avr32, powerpc consist these posix_types.h and types.h with "typedef".
Maybe this is old code that was added before checkpatch existed?
It looks the kernel is not going to fix the "old" typedef for posix_types.h and types.h
Eventually they would not add these files as is any more today.
I think they say "please do not add any new typedef" might mean to those typedef used in drivers or protocols.
I mean all of them.
In the other 2 files arch/nds32/include/asm/global_data.h and arch/nds32/include/asm/u-boot.h, typedef was used for #449: FILE: arch/nds32/include/asm/global_data.h:46: +typedef struct global_data { +} gd_t; #1505: FILE: arch/nds32/include/asm/u-boot.h:41: +typedef struct bd_info { +} bd_t;
Ouch. You got me there... :-(
I don't know if you have any idea of fixing it in u-boot.
Well, the fix is basicly straightforward - replace all ocurrences of gd_t and bd_t in the U-Boot code. But this is a bigger issue and more work than I dare to push on you. I'm grinding my teeth, but I will accept these.
Best regards,
Wolfgang Denk
On Sat, Apr 30, 2011 at 7:09 AM, Wolfgang Denk wd@denx.de wrote:
Dear Macpaul Lin,
In message BANLkTi=haZVW17eNWYCmYf3McrGmbm-4Xw@mail.gmail.com you wrote:
I think we still have to discuss about the typedef's. What does the "new" typedef means?
It means adding any new code to U-Boot which includes "typedef"s.
According to the checkpatch result, "typedef" warning exists in 4 files. arch/nds32/include/asm/posix_types.h arch/nds32/include/asm/types.h arch/nds32/include/asm/global_data.h arch/nds32/include/asm/u-boot.h.
File arch/nds32/include/asm/posix_types.h and arch/nds32/include/asm/types.h come from the Linux kernel. Which is usually used for posix compatibility for Linux Kernel.
So these files should be fixed in Linux, too, because it is the Linux checkpatch tool which throws this warning.
I don't think this affects POSIX compatibility. Which typedef's are required by POSIX?
Which should be "old" features for posix and compatibility. However, you cannot say for a new architecture to support posix and other compatibility as "new" typedef. I've checked the latest kernel (2.6.38.1), arm, mips, avr32, powerpc consist these posix_types.h and types.h with "typedef".
Maybe this is old code that was added before checkpatch existed?
In Linux "CodingStyle"
" Chapter 5: Typedefs
Please don't use things like "vps_t".
It's a _mistake_ to use typedef for structures and pointers." [skip] "Lots of people think that typedefs "help readability". Not so. They are useful only for:" [skip] " (b) Clear integer types, where the abstraction _helps_ avoid confusion whether it is "int" or "long".
u8/u16/u32 are perfectly fine typedefs, although they fit into category (d) better than here.
NOTE! Again - there needs to be a _reason_ for this. If something is "unsigned long", then there's no reason to do
typedef unsigned long myflags_t;
but if there is a clear reason for why it under certain circumstances might be an "unsigned int" and under other configurations might be "unsigned long", then by all means go ahead and use a typedef."
I think posix_types may be an example of this case. nds32 have some types different from what has been defined in <asm-generic/posix_types.h>
As for commit time, the latest update for checkpatch.pl is in 2009-01-06 653d4876 (Andy Whitcroft 2007-06-23 17:16:34 -0700 1891) if ($line =~ /\btypedef\s/ && 8054576d (Andy Whitcroft 2009-01-06 14:41:26 -0800 1892) $line !~ /\btypedef\s+$Type\s*(\s**?$Ident\s*)\s*(/ && c45dcabd (Andy Whitcroft 2008-06-05 22:46:01 -0700 1893) $line !~ /\btypedef\s+$Type\s+$Ident\s*(/ && 8ed22cad (Andy Whitcroft 2008-10-15 22:02:32 -0700 1894) $line !~ /\b$typeTypedefs\b/ && 653d4876 (Andy Whitcroft 2007-06-23 17:16:34 -0700 1895) $line !~ /\b__bitwise(?:__|)\b/) { de7d4f0e (Andy Whitcroft 2007-07-15 23:37:22 -0700 1896) WARN("do not add new typedefs\n" . $herecurr); 0a920b5b (Andy Whitcroft 2007-06-01 00:46:48 -0700 1897) }
But Michal Simek's commit, which added posix_types.h is adapted in 2009-03-27. You can reference it by 6d9c3f208580 in Linux repo
It looks the kernel is not going to fix the "old" typedef for posix_types.h and types.h
Eventually they would not add these files as is any more today.
I think they say "please do not add any new typedef" might mean to those typedef used in drivers or protocols.
I mean all of them.
In the other 2 files arch/nds32/include/asm/global_data.h and arch/nds32/include/asm/u-boot.h, typedef was used for #449: FILE: arch/nds32/include/asm/global_data.h:46: +typedef struct global_data { +} gd_t; #1505: FILE: arch/nds32/include/asm/u-boot.h:41: +typedef struct bd_info { +} bd_t;
Ouch. You got me there... :-(
I don't know if you have any idea of fixing it in u-boot.
Well, the fix is basicly straightforward - replace all ocurrences of gd_t and bd_t in the U-Boot code. But this is a bigger issue and more work than I dare to push on you. I'm grinding my teeth, but I will accept these.
Best regards,
Wolfgang Denk
-- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de If ignorance is bliss, why aren't there more happy people? _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Hi Wolfgang,
2011/4/30 Wolfgang Denk wd@denx.de:
Dear Macpaul Lin,
It means adding any new code to U-Boot which includes "typedef"s.
According to the checkpatch result, "typedef" warning exists in 4 files. arch/nds32/include/asm/posix_types.h arch/nds32/include/asm/types.h arch/nds32/include/asm/global_data.h arch/nds32/include/asm/u-boot.h.
File arch/nds32/include/asm/posix_types.h and arch/nds32/include/asm/types.h come from the Linux kernel. Which is usually used for posix compatibility for Linux Kernel.
So these files should be fixed in Linux, too, because it is the Linux checkpatch tool which throws this warning.
I don't think this affects POSIX compatibility. Which typedef's are required by POSIX?
1. posix_types.h I cannot say I'm sure that the posix_types.h is not necessary for u-boot. If this posix_types.h won't be used any more, should we keep it in u-boot?
I listed the required typedef for POSIX compatibility in Linux kernel as follows which are also support by other architectures. typedef unsigned short __kernel_dev_t; typedef unsigned long __kernel_ino_t; typedef unsigned short __kernel_mode_t; typedef unsigned short __kernel_nlink_t; typedef long __kernel_off_t; typedef int __kernel_pid_t; typedef unsigned short __kernel_ipc_pid_t; typedef unsigned short __kernel_uid_t; typedef unsigned short __kernel_gid_t; typedef unsigned int __kernel_size_t; typedef int __kernel_ssize_t; typedef int __kernel_ptrdiff_t; typedef long __kernel_time_t; typedef long __kernel_suseconds_t; typedef long __kernel_clock_t; typedef int __kernel_daddr_t; typedef char *__kernel_caddr_t; typedef unsigned short __kernel_uid16_t; typedef unsigned short __kernel_gid16_t; typedef unsigned int __kernel_uid32_t; typedef unsigned int __kernel_gid32_t; typedef unsigned short __kernel_old_uid_t; typedef unsigned short __kernel_old_gid_t; typedef long long __kernel_loff_t; typedef struct { #if defined(__KERNEL__) || defined(__USE_ALL) int val[2]; #else /* !defined(__KERNEL__) && !defined(__USE_ALL) */ int __val[2]; #endif /* !defined(__KERNEL__) && !defined(__USE_ALL) */ } __kernel_fsid_t;
Maybe this is old code that was added before checkpatch existed?
It looks the kernel is not going to fix the "old" typedef for posix_types.h and types.h
Eventually they would not add these files as is any more today.
Even though kernel would not add these files with "typedef" for implementing POSIX, however, any new architecture want to be commit to Linux kernel must implementing the old "typedef" code for POSIX compatibility support unless they want to rewrite all POSIX interfaces.
2. types.h typedef unsigned short umode_t; typedef unsigned long phys_addr_t; typedef unsigned long phys_size_t; Take phys_addr_t as example, the following code in u-boot will use it. include/common.h:223:phys_size_t initdram (int) will use such typedef. ../include/lmb.h:40:extern phys_addr_t lmb_alloc(struct lmb *lmb, phys_size_t size, ulong align); ../include/addr_map.h:27: phys_size_t size, int idx); ../include/image.h:455:phys_size_t getenv_bootm_size(void);
This file will use umode_t ../fs/ubifs/ubifs.h:115: umode_t i_mode;
I think they say "please do not add any new typedef" might mean to those typedef used in drivers or protocols.
I mean all of them.
In the other 2 files arch/nds32/include/asm/global_data.h and arch/nds32/include/asm/u-boot.h, typedef was used for #449: FILE: arch/nds32/include/asm/global_data.h:46: +typedef struct global_data { +} gd_t; #1505: FILE: arch/nds32/include/asm/u-boot.h:41: +typedef struct bd_info { +} bd_t;
Ouch. You got me there... :-(
I don't know if you have any idea of fixing it in u-boot.
Well, the fix is basicly straightforward - replace all ocurrences of gd_t and bd_t in the U-Boot code. But this is a bigger issue and more work than I dare to push on you. I'm grinding my teeth, but I will accept these.
Okay, well then, according to the list above I think the problem of typedefs in global_data.h and u-boot.h is similar to types.h. I don't know if you agree with it?
If posix_types.h won't be used in u-boot, I'll suggest u-boot remove it. However, I've found that if I have removed posix_types.h in arch/nds32/includes/asm will lead the following compilation fail.
/home/macpaul/u-boot/u-boot-nds32/include/linux/posix_types.h:46:29: asm/posix_types.h: No such file or directory In file included from /home/macpaul/u-boot/u-boot-nds32/include/linux/types.h:8, from /home/macpaul/u-boot/u-boot-nds32/include/common.h:40, from lib/asm-offsets.c:18: /home/macpaul/u-boot/u-boot-nds32/include/linux/posix_types.h:46:29: asm/posix_types.h: No such file or directory /home/macpaul/u-boot/u-boot-nds32/include/linux/types.h:14: error: parse error before "dev_t" /home/macpaul/u-boot/u-boot-nds32/include/linux/types.h:14: warning: type defaults to `int' in declaration of `dev_t' /home/macpaul/u-boot/u-boot-nds32/include/linux/types.h:14: warning: data definition has no type or storage class /home/macpaul/u-boot/u-boot-nds32/include/linux/types.h:15: error: parse error before "ino_t" /home/macpaul/u-boot/u-boot-nds32/include/linux/types.h:15: warning: type defaults to `int' in declaration of `ino_t' /home/macpaul/u-boot/u-boot-nds32/include/linux/types.h:15: warning: data definition has no type or storage class /home/macpaul/u-boot/u-boot-nds32/include/linux/types.h:16: error: parse error before "mode_t" ... and more.
These are indeed related to typedef in posix_types.h
According to Chao's informations, I think the files of these typedef we've send is comfort to Linux's coding styles.
Thanks.
participants (3)
-
Chih-Min Chao -
Macpaul Lin -
Wolfgang Denk