Patchwork [U-Boot,v3,1/2] MX25: tx25: Avoid the usage of extern in C file

login
register
mail settings
Submitter Fabio Estevam
Date Sept. 2, 2011, 2:02 p.m.
Message ID <1314972139-20068-1-git-send-email-fabio.estevam@freescale.com>
Download mbox | patch
Permalink /patch/113143/
State Superseded
Delegated to: Stefano Babic
Headers show

Comments

Fabio Estevam - Sept. 2, 2011, 2:02 p.m.
Avoid the usage of extern in C file as pointed out by checkpatch.

Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
---
Changes since v2:
- Place the extern in the MX25 imx-regs.h instead of inside the MX27 imx-regs.h
Changes since v1:
- No changes
 arch/arm/include/asm/arch-mx25/imx-regs.h |    1 +
 board/karo/tx25/tx25.c                    |    1 -
 2 files changed, 1 insertions(+), 1 deletions(-)
Stefano Babic - Sept. 5, 2011, 10:43 a.m.
On 09/02/2011 04:02 PM, Fabio Estevam wrote:
> Avoid the usage of extern in C file as pointed out by checkpatch.
> 
> Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>

Hi Fabio,

> diff --git a/arch/arm/include/asm/arch-mx25/imx-regs.h b/arch/arm/include/asm/arch-mx25/imx-regs.h
> index 9e30f7c..7e34050 100644
> --- a/arch/arm/include/asm/arch-mx25/imx-regs.h
> +++ b/arch/arm/include/asm/arch-mx25/imx-regs.h
> @@ -37,6 +37,7 @@
>  #ifdef CONFIG_FEC_MXC
>  extern void mx25_fec_init_pins(void);
>  extern void imx_get_mac_from_fuse(unsigned char *mac);
> +extern void mx25_uart1_init_pins(void);

As i can see, in the imx-regs.h for the different processors there is
(as the name says) only definitions and structures for the internal
registers of each processor. No function prototypes.

What about to move the prototype ? For MX5/MX35 there is a sys_proto.h
(as it is done for other SOCs), in this case, well, it seems too much to
add a file for a single line. However, we could move it in clock.h and
creating a sys_proto.h if the number of exported functions will increase.

Best regards,
Stefano Babic
Fabio Estevam - Sept. 5, 2011, 11:37 a.m.
On Mon, Sep 5, 2011 at 7:43 AM, Stefano Babic <sbabic@denx.de> wrote:
...
> What about to move the prototype ? For MX5/MX35 there is a sys_proto.h
> (as it is done for other SOCs), in this case, well, it seems too much to
> add a file for a single line. However, we could move it in clock.h and
> creating a sys_proto.h if the number of exported functions will increase.

Ok, I can create a sys_proto.h for MX25 and put the following extern´s there:

extern void mx25_fec_init_pins(void);
extern void imx_get_mac_from_fuse(unsigned char *mac);
extern void mx25_uart1_init_pins(void);

Regards,

Fabio Estevam
Marek Vasut - Sept. 5, 2011, 1:05 p.m.
On Monday, September 05, 2011 01:37:17 PM Fabio Estevam wrote:
> On Mon, Sep 5, 2011 at 7:43 AM, Stefano Babic <sbabic@denx.de> wrote:
> ...
> 
> > What about to move the prototype ? For MX5/MX35 there is a sys_proto.h
> > (as it is done for other SOCs), in this case, well, it seems too much to
> > add a file for a single line. However, we could move it in clock.h and
> > creating a sys_proto.h if the number of exported functions will increase.
> 
> Ok, I can create a sys_proto.h for MX25 and put the following extern´s
> there:
> 
> extern void mx25_fec_init_pins(void);
> extern void imx_get_mac_from_fuse(unsigned char *mac);
> extern void mx25_uart1_init_pins(void);

Ok, this might be a stupid one, but ...

why use externs in header files ?

> 
> Regards,
> 
> Fabio Estevam
> _______________________________________________
> U-Boot mailing list
> U-Boot@lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
Fabio Estevam - Sept. 5, 2011, 1:15 p.m.
On Mon, Sep 5, 2011 at 10:05 AM, Marek Vasut <marek.vasut@gmail.com> wrote:
...
>> extern void mx25_fec_init_pins(void);
>> extern void imx_get_mac_from_fuse(unsigned char *mac);
>> extern void mx25_uart1_init_pins(void);
>
> Ok, this might be a stupid one, but ...
>
> why use externs in header files ?

This will make checkpatch happy :-)

Regards,

Fabio Estevam
Marek Vasut - Sept. 5, 2011, 3:05 p.m.
On Monday, September 05, 2011 03:15:44 PM Fabio Estevam wrote:
> On Mon, Sep 5, 2011 at 10:05 AM, Marek Vasut <marek.vasut@gmail.com> wrote:
> ...
> 
> >> extern void mx25_fec_init_pins(void);
> >> extern void imx_get_mac_from_fuse(unsigned char *mac);
> >> extern void mx25_uart1_init_pins(void);
> > 
> > Ok, this might be a stupid one, but ...
> > 
> > why use externs in header files ?
> 
> This will make checkpatch happy :-)
> 
I'm not quite sure I understand ... ?

> Regards,
> 
> Fabio Estevam
Fabio Estevam - Sept. 5, 2011, 3:38 p.m.
On Mon, Sep 5, 2011 at 12:05 PM, Marek Vasut <marek.vasut@gmail.com> wrote:
> On Monday, September 05, 2011 03:15:44 PM Fabio Estevam wrote:
>> On Mon, Sep 5, 2011 at 10:05 AM, Marek Vasut <marek.vasut@gmail.com> wrote:
>> ...
>>
>> >> extern void mx25_fec_init_pins(void);
>> >> extern void imx_get_mac_from_fuse(unsigned char *mac);
>> >> extern void mx25_uart1_init_pins(void);
>> >
>> > Ok, this might be a stupid one, but ...
>> >
>> > why use externs in header files ?
>>
>> This will make checkpatch happy :-)
>>
> I'm not quite sure I understand ... ?

./scripts/checkpatch.pl -F u-boot/board/karo/tx25/tx25.c
....
WARNING: externs should be avoided in .c files
#144: FILE: home/fabio/denx/u-boot/board/karo/tx25/tx25.c:144:
+	extern void mx25_uart1_init_pins(void);
Marek Vasut - Sept. 5, 2011, 3:40 p.m.
On Monday, September 05, 2011 05:38:26 PM Fabio Estevam wrote:
> On Mon, Sep 5, 2011 at 12:05 PM, Marek Vasut <marek.vasut@gmail.com> wrote:
> > On Monday, September 05, 2011 03:15:44 PM Fabio Estevam wrote:
> >> On Mon, Sep 5, 2011 at 10:05 AM, Marek Vasut <marek.vasut@gmail.com>
> >> wrote: ...
> >> 
> >> >> extern void mx25_fec_init_pins(void);
> >> >> extern void imx_get_mac_from_fuse(unsigned char *mac);
> >> >> extern void mx25_uart1_init_pins(void);
> >> > 
> >> > Ok, this might be a stupid one, but ...
> >> > 
> >> > why use externs in header files ?
> >> 
> >> This will make checkpatch happy :-)
> > 
> > I'm not quite sure I understand ... ?
> 
> ./scripts/checkpatch.pl -F u-boot/board/karo/tx25/tx25.c
> ....
> WARNING: externs should be avoided in .c files
> #144: FILE: home/fabio/denx/u-boot/board/karo/tx25/tx25.c:144:
> +	extern void mx25_uart1_init_pins(void);

But you're using extern in _header_ (.h) file ... so ... why ?
Fabio Estevam - Sept. 5, 2011, 3:48 p.m.
On Mon, Sep 5, 2011 at 12:40 PM, Marek Vasut <marek.vasut@gmail.com> wrote:
...

>> ./scripts/checkpatch.pl -F u-boot/board/karo/tx25/tx25.c
>> ....
>> WARNING: externs should be avoided in .c files
>> #144: FILE: home/fabio/denx/u-boot/board/karo/tx25/tx25.c:144:
>> +     extern void mx25_uart1_init_pins(void);
>
> But you're using extern in _header_ (.h) file ... so ... why ?
>

My patch removes the extern from the C file and put it on a header
file with other extern's.

The checkpatch warning I showed happens with the original file, and
not after my patch is applied.
Wolfgang Denk - Sept. 5, 2011, 4:02 p.m.
Dear Fabio Estevam,

In message <CAOMZO5A1v5jUkXTF9nKnZh8VNewzkoMB1tMmE9cRJCJDp+y2ug@mail.gmail.com> you wrote:
>
> > But you're using extern in _header_ (.h) file ... so ... why ?
> >
> 
> My patch removes the extern from the C file and put it on a header
> file with other extern's.

What Marek probably wants to tell yet (in a bit a complicated way) is
that you should omit the "extern" in the prototype declaration. Just
remove all "extern" from the header file.

Best regards,

Wolfgang Denk
Marek Vasut - Sept. 5, 2011, 4:12 p.m.
On Monday, September 05, 2011 06:02:21 PM Wolfgang Denk wrote:
> Dear Fabio Estevam,
> 
> In message 
<CAOMZO5A1v5jUkXTF9nKnZh8VNewzkoMB1tMmE9cRJCJDp+y2ug@mail.gmail.com> you wrote:
> > > But you're using extern in _header_ (.h) file ... so ... why ?
> > 
> > My patch removes the extern from the C file and put it on a header
> > file with other extern's.
> 
> What Marek probably wants to tell yet (in a bit a complicated way) is
> that you should omit the "extern" in the prototype declaration. Just
> remove all "extern" from the header file.

No, I was asking if there's some special reason for this or it's plain wrong. I 
see the later's the case.

Thanks, cheers!
> 
> Best regards,
> 
> Wolfgang Denk
Stefano Babic - Sept. 5, 2011, 4:49 p.m.
On 09/05/2011 05:48 PM, Fabio Estevam wrote:
> On Mon, Sep 5, 2011 at 12:40 PM, Marek Vasut <marek.vasut@gmail.com> wrote:
> ...
>

Hi Fabio,

>>> ./scripts/checkpatch.pl -F u-boot/board/karo/tx25/tx25.c
>>> ....
>>> WARNING: externs should be avoided in .c files
>>> #144: FILE: home/fabio/denx/u-boot/board/karo/tx25/tx25.c:144:
>>> +     extern void mx25_uart1_init_pins(void);
>>
>> But you're using extern in _header_ (.h) file ... so ... why ?
>>
> 
> My patch removes the extern from the C file and put it on a header
> file with other extern's.
> 
> The checkpatch warning I showed happens with the original file, and
> not after my patch is applied.
> 

I think checkpatch warnings because the original file has already some
extern. In the most of u-boot code we do not mark the prototypes with
extern.
I made this simple test - I create a new file (from sys_proto.h) and I
run checkpatch on it:

+#ifndef _SYS_PROTO_H_
+#define _SYS_PROTO_H_
+
+u32 get_cpu_rev(void);
+#define is_soc_rev(rev)	((get_cpu_rev() & 0xFF) - rev)
+void sdelay(unsigned long);
+void set_chipselect_size(int const);
+#endif

Patch

diff --git a/arch/arm/include/asm/arch-mx25/imx-regs.h b/arch/arm/include/asm/arch-mx25/imx-regs.h
index 9e30f7c..7e34050 100644
--- a/arch/arm/include/asm/arch-mx25/imx-regs.h
+++ b/arch/arm/include/asm/arch-mx25/imx-regs.h
@@ -37,6 +37,7 @@ 
 #ifdef CONFIG_FEC_MXC
 extern void mx25_fec_init_pins(void);
 extern void imx_get_mac_from_fuse(unsigned char *mac);
+extern void mx25_uart1_init_pins(void);
 #endif
 
 /* Clock Control Module (CCM) registers */
diff --git a/board/karo/tx25/tx25.c b/board/karo/tx25/tx25.c
index 25b99e8..2787715 100644
--- a/board/karo/tx25/tx25.c
+++ b/board/karo/tx25/tx25.c
@@ -141,7 +141,6 @@  void tx25_fec_init(void)
 int board_init()
 {
 #ifdef CONFIG_MXC_UART
-	extern void mx25_uart1_init_pins(void);
 
 	mx25_uart1_init_pins();
 #endif