[2.6 patch] make drivers/mtd/cmdlinepart.c:mtdpart_setup() static

[2.6 patch] make drivers/mtd/cmdlinepart.c:mtdpart_setup() static

[Adrian Bunk]

This patch makes the needlessly global mtdpart_setup() static.

Signed-off-by: Adrian Bunk <bunk@stusta.de>

--- linux-2.6.17-mm2-full/drivers/mtd/cmdlinepart.c.old	2006-06-26 23:18:39.000000000 +0200
+++ linux-2.6.17-mm2-full/drivers/mtd/cmdlinepart.c	2006-06-26 23:18:51.000000000 +0200
@@ -346,7 +346,7 @@
  *
  * This function needs to be visible for bootloaders.
  */
-int mtdpart_setup(char *s)
+static int mtdpart_setup(char *s)
 {
 	cmdline = s;
 	return 1;

Re: [2.6 patch] make drivers/mtd/cmdlinepart.c:mtdpart_setup() static

[David Woodhouse]

On Tue, 2006-06-27 at 00:02 +0200, Adrian Bunk wrote:
> This patch makes the needlessly global mtdpart_setup() static.
> 
> Signed-off-by: Adrian Bunk <bunk@stusta.de>
> 
> --- linux-2.6.17-mm2-full/drivers/mtd/cmdlinepart.c.old 2006-06-26 23:18:39.000000000 +0200
> +++ linux-2.6.17-mm2-full/drivers/mtd/cmdlinepart.c     2006-06-26 23:18:51.000000000 +0200
> @@ -346,7 +346,7 @@
>   *
>   * This function needs to be visible for bootloaders.
>   */
> -int mtdpart_setup(char *s)
> +static int mtdpart_setup(char *s) 

Patch lacks sufficient explanation. Explain the relevance of the comment
immediately above the function declaration, in the context of your
patch. Explain your decision to change the behaviour, but not change the
comment itself.

Think. Or you will be replaced with a small shell script.

-- 
dwmw2

Re: [2.6 patch] make drivers/mtd/cmdlinepart.c:mtdpart_setup() static

[Adrian Bunk]

On Tue, Jun 27, 2006 at 02:49:00PM +0100, David Woodhouse wrote:
> On Tue, 2006-06-27 at 00:02 +0200, Adrian Bunk wrote:
> > This patch makes the needlessly global mtdpart_setup() static.
> > 
> > Signed-off-by: Adrian Bunk <bunk@stusta.de>
> > 
> > --- linux-2.6.17-mm2-full/drivers/mtd/cmdlinepart.c.old 2006-06-26 23:18:39.000000000 +0200
> > +++ linux-2.6.17-mm2-full/drivers/mtd/cmdlinepart.c     2006-06-26 23:18:51.000000000 +0200
> > @@ -346,7 +346,7 @@
> >   *
> >   * This function needs to be visible for bootloaders.
> >   */
> > -int mtdpart_setup(char *s)
> > +static int mtdpart_setup(char *s) 
> 
> Patch lacks sufficient explanation. Explain the relevance of the comment
> immediately above the function declaration, in the context of your
> patch. Explain your decision to change the behaviour, but not change the
> comment itself.

My explanation regarding the relevance of the comment is that it seems 
to be nonsense.

Do I miss something, or why and how should a bootloader access 
in-kernel functions?

> Think. Or you will be replaced with a small shell script.

No problem, I'm waiting for your submission of Adrian 1.0 ;-)

> dwmw2

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: [2.6 patch] make drivers/mtd/cmdlinepart.c:mtdpart_setup() static

[David Woodhouse]

On Thu, 2006-06-29 at 19:32 +0200, Adrian Bunk wrote:
> On Tue, Jun 27, 2006 at 02:49:00PM +0100, David Woodhouse wrote:
> > On Tue, 2006-06-27 at 00:02 +0200, Adrian Bunk wrote:
> > > This patch makes the needlessly global mtdpart_setup() static.
> > > 
> > > Signed-off-by: Adrian Bunk <bunk@stusta.de>
> > > 
> > > --- linux-2.6.17-mm2-full/drivers/mtd/cmdlinepart.c.old 2006-06-26 23:18:39.000000000 +0200
> > > +++ linux-2.6.17-mm2-full/drivers/mtd/cmdlinepart.c     2006-06-26 23:18:51.000000000 +0200
> > > @@ -346,7 +346,7 @@
> > >   *
> > >   * This function needs to be visible for bootloaders.
> > >   */
> > > -int mtdpart_setup(char *s)
> > > +static int mtdpart_setup(char *s) 
> > 
> > Patch lacks sufficient explanation. Explain the relevance of the comment
> > immediately above the function declaration, in the context of your
> > patch. Explain your decision to change the behaviour, but not change the
> > comment itself.
> 
> My explanation regarding the relevance of the comment is that it seems 
> to be nonsense.
> 
> Do I miss something, or why and how should a bootloader access 
> in-kernel functions? 

I'm not entirely sure, but allegedly it does -- Juha, can you elaborate?

-- 
dwmw2

Re: [2.6 patch] make drivers/mtd/cmdlinepart.c:mtdpart_setup() static

[Juha Yrjölä]

David Woodhouse wrote:

>>>> --- linux-2.6.17-mm2-full/drivers/mtd/cmdlinepart.c.old 2006-06-26 23:18:39.000000000 +0200
>>>> +++ linux-2.6.17-mm2-full/drivers/mtd/cmdlinepart.c     2006-06-26 23:18:51.000000000 +0200
>>>> @@ -346,7 +346,7 @@
>>>>   *
>>>>   * This function needs to be visible for bootloaders.
>>>>   */
>>>> -int mtdpart_setup(char *s)
>>>> +static int mtdpart_setup(char *s) 
>>> Patch lacks sufficient explanation. Explain the relevance of the comment
>>> immediately above the function declaration, in the context of your
>>> patch. Explain your decision to change the behaviour, but not change the
>>> comment itself.
>> My explanation regarding the relevance of the comment is that it seems 
>> to be nonsense.
>>
>> Do I miss something, or why and how should a bootloader access 
>> in-kernel functions? 
> 
> I'm not entirely sure, but allegedly it does -- Juha, can you elaborate?

Our bootloader doesn't access in-kernel functions, for obvious reasons. 
  The comment in cmdlinepart.c is unfortunately inaccurate.  The 
bootloader does need a mechanism for passing the partition table to the 
kernel, though.  Our partition table is generated on-the-fly by the 
bootloader to guarantee that each partition gets a certain number of 
non-bad NAND blocks.

We used to do this by passing the partition table in a string compatible 
with cmdlinepart syntax.  The kernel NAND driver then just passed the 
string received from the bootloader to mtdpart_setup().

Nowadays there is a better way of doing this, so as far as we are 
concerned, mtdpart_setup() can be made static again.  We'll migrate our 
MTD drivers to use the platform_data mechanism instead.

Cheers,
Juha

Re: [2.6 patch] make drivers/mtd/cmdlinepart.c:mtdpart_setup() static

[Sergei Shtylyov]

Hello.

David Woodhouse wrote:

>>>>This patch makes the needlessly global mtdpart_setup() static.

>>>>Signed-off-by: Adrian Bunk <bunk@stusta.de>
>>>>
>>>>--- linux-2.6.17-mm2-full/drivers/mtd/cmdlinepart.c.old 2006-06-26 23:18:39.000000000 +0200
>>>>+++ linux-2.6.17-mm2-full/drivers/mtd/cmdlinepart.c     2006-06-26 23:18:51.000000000 +0200
>>>>@@ -346,7 +346,7 @@
>>>>  *
>>>>  * This function needs to be visible for bootloaders.
>>>>  */
>>>>-int mtdpart_setup(char *s)
>>>>+static int mtdpart_setup(char *s) 

>>>Patch lacks sufficient explanation. Explain the relevance of the comment
>>>immediately above the function declaration, in the context of your
>>>patch. Explain your decision to change the behaviour, but not change the
>>>comment itself.

>>My explanation regarding the relevance of the comment is that it seems 
>>to be nonsense.

>>Do I miss something, or why and how should a bootloader access 
>>in-kernel functions? 

> I'm not entirely sure, but allegedly it does -- Juha, can you elaborate?

    In addition, this function might be needed to support parsing of the 
partition info extracted from the OF device tree (if this way of storing it 
there will be accepted)...

WBR, Sergei

Re: [2.6 patch] make drivers/mtd/cmdlinepart.c:mtdpart_setup() static

[David Woodhouse]

On Mon, 2006-07-10 at 14:56 +0400, Sergei Shtylyov wrote:
>     In addition, this function might be needed to support parsing of the 
> partition info extracted from the OF device tree (if this way of storing it 
> there will be accepted)... 

Yes, we should extend physmap to handle of_devices and partition
information represented as a string. It won't be mtdpart_setup() which
we use for that though.

-- 
dwmw2

[2.6 patch] make drivers/mtd/cmdlinepart.c:mtdpart_setup() static

[Adrian Bunk]

This patch makes the needlessly global mtdpart_setup() static.

Signed-off-by: Adrian Bunk <bunk@stusta.de>

---

This patch was already sent on:
- 27 Jun 2006

--- linux-2.6.17-mm2-full/drivers/mtd/cmdlinepart.c.old	2006-06-26 23:18:39.000000000 +0200
+++ linux-2.6.17-mm2-full/drivers/mtd/cmdlinepart.c	2006-06-26 23:18:51.000000000 +0200
@@ -346,7 +346,7 @@
  *
  * This function needs to be visible for bootloaders.
  */
-int mtdpart_setup(char *s)
+static int mtdpart_setup(char *s)
 {
 	cmdline = s;
 	return 1;

Re: [2.6 patch] make drivers/mtd/cmdlinepart.c:mtdpart_setup() static

[David Woodhouse]

On Sat, 2006-11-25 at 20:15 +0100, Adrian Bunk wrote:
> This patch makes the needlessly global mtdpart_setup() static.
> 
> Signed-off-by: Adrian Bunk <bunk@stusta.de> 

Applied; thanks.

-- 
dwmw2

Re: [2.6 patch] make drivers/mtd/cmdlinepart.c:mtdpart_setup() static

[Andy Isaacson]

On Sat, Nov 25, 2006 at 08:15:41PM +0100, Adrian Bunk wrote:
> This patch makes the needlessly global mtdpart_setup() static.
> 
> @@ -346,7 +346,7 @@
>   *
>   * This function needs to be visible for bootloaders.
>   */
> -int mtdpart_setup(char *s)
> +static int mtdpart_setup(char *s)
>  {
>  	cmdline = s;
>  	return 1;

Jarkko,

You're recorded as submitting the original patch to make this
non-static:
http://linux.bkbits.net:8080/linux-2.6/diffs/drivers/mtd/cmdlinepart.c@1.11?nav=index.html|src/|src/drivers|src/drivers/mtd|hist/drivers/mtd/cmdlinepart.c

Is this change correct?

If so, it should also delete the "needs to be visible" comment.

-andy

Re: [2.6 patch] make drivers/mtd/cmdlinepart.c:mtdpart_setup() static

[Jarkko Lavinen]

Our bootloader once used mtdpart_setup() for passing mtd partitions to 
kernel. We do not do that any more and instead use the kernel command line 
for passing the information.

Jarkko Lavinen

On Thu, Nov 30, 2006 at 10:45:56AM -0800, ext Andy Isaacson wrote:
> On Sat, Nov 25, 2006 at 08:15:41PM +0100, Adrian Bunk wrote:
> > This patch makes the needlessly global mtdpart_setup() static.
> > 
> > @@ -346,7 +346,7 @@
> >   *
> >   * This function needs to be visible for bootloaders.
> >   */
> > -int mtdpart_setup(char *s)
> > +static int mtdpart_setup(char *s)
> >  {
> >  	cmdline = s;
> >  	return 1;
> 
> Jarkko,
> 
> You're recorded as submitting the original patch to make this
> non-static:
> http://linux.bkbits.net:8080/linux-2.6/diffs/drivers/mtd/cmdlinepart.c@1.11?nav=index.html|src/|src/drivers|src/drivers/mtd|hist/drivers/mtd/cmdlinepart.c
> 
> Is this change correct?
> 
> If so, it should also delete the "needs to be visible" comment.
> 
> -andy