[PATCH] mtd: clean up uclinux.c map driver

[PATCH] mtd: clean up uclinux.c map driver

[gerg]

From: Greg Ungerer <gerg@uclinux.org>

Perform a number of cleanups on the uclinux.c map driver.
No structural or semantic changes, only minor cleanups.

. insert appropriate prefix into printk() calls
. remove redundant "if" checks in the module exit code
. remove unnecessary includes
. make the struct uclinux_ram_map static
. cast the virtual address calculations to keep them sparse clean

Signed-off-by: Greg Ungerer <gerg@uclinux.org>
---
 drivers/mtd/maps/uclinux.c |   34 +++++++++++++---------------------
 1 files changed, 13 insertions(+), 21 deletions(-)

diff --git a/drivers/mtd/maps/uclinux.c b/drivers/mtd/maps/uclinux.c
index 6d43c75..d91b5b4 100644
--- a/drivers/mtd/maps/uclinux.c
+++ b/drivers/mtd/maps/uclinux.c
@@ -12,19 +12,16 @@
 #include <linux/types.h>
 #include <linux/init.h>
 #include <linux/kernel.h>
-#include <linux/fs.h>
 #include <linux/mm.h>
-#include <linux/major.h>
 #include <linux/mtd/mtd.h>
 #include <linux/mtd/map.h>
 #include <linux/mtd/partitions.h>
-#include <asm/io.h>
 
 /****************************************************************************/
 
 extern char _ebss;
 
-struct map_info uclinux_ram_map = {
+static struct map_info uclinux_ram_map = {
 	.name = "RAM",
 	.phys = (unsigned long)&_ebss,
 	.size = 0,
@@ -46,11 +43,11 @@ static int uclinux_point(struct mtd_info *mtd, loff_t from, size_t len,
 	size_t *retlen, void **virt, resource_size_t *phys)
 {
 	struct map_info *map = mtd->priv;
-	*virt = map->virt + from;
+	*virt = (void *) (unsigned long) map->virt + from;
 	if (phys)
 		*phys = map->phys + from;
 	*retlen = len;
-	return(0);
+	return 0;
 }
 
 /****************************************************************************/
@@ -65,22 +62,22 @@ static int __init uclinux_mtd_init(void)
 		mapp->size = PAGE_ALIGN(ntohl(*((unsigned long *)(mapp->phys + 8))));
 	mapp->bankwidth = 4;
 
-	printk("uclinux[mtd]: RAM probe address=0x%x size=0x%x\n",
+	printk(KERN_NOTICE "uclinux[mtd]: RAM probe address=0x%x size=0x%x\n",
 	       	(int) mapp->phys, (int) mapp->size);
 
-	mapp->virt = phys_to_virt(mapp->phys);
+	mapp->virt = (void __iomem *) (unsigned long) phys_to_virt(mapp->phys);
 
-	if (mapp->virt == 0) {
-		printk("uclinux[mtd]: no virtual mapping?\n");
-		return(-EIO);
+	if (mapp->virt == NULL) {
+		printk(KERN_ERR "uclinux[mtd]: no virtual mapping?\n");
+		return -EIO;
 	}
 
 	simple_map_init(mapp);
 
 	mtd = do_map_probe("map_ram", mapp);
 	if (!mtd) {
-		printk("uclinux[mtd]: failed to find a mapping?\n");
-		return(-ENXIO);
+		printk(KERN_ERR "uclinux[mtd]: failed to find a mapping?\n");
+		return -ENXIO;
 	}
 
 	mtd->owner = THIS_MODULE;
@@ -90,20 +87,15 @@ static int __init uclinux_mtd_init(void)
 	uclinux_ram_mtdinfo = mtd;
 	mtd_device_register(mtd, uclinux_romfs, NUM_PARTITIONS);
 
-	return(0);
+	return 0;
 }
 
 /****************************************************************************/
 
 static void __exit uclinux_mtd_cleanup(void)
 {
-	if (uclinux_ram_mtdinfo) {
-		mtd_device_unregister(uclinux_ram_mtdinfo);
-		map_destroy(uclinux_ram_mtdinfo);
-		uclinux_ram_mtdinfo = NULL;
-	}
-	if (uclinux_ram_map.virt)
-		uclinux_ram_map.virt = 0;
+	mtd_device_unregister(uclinux_ram_mtdinfo);
+	map_destroy(uclinux_ram_mtdinfo);
 }
 
 /****************************************************************************/
-- 
1.7.0.4

Re: [uClinux-dev] [PATCH] mtd: clean up uclinux.c map driver

[Greg Ungerer]

On 15/05/12 14:08, gerg@snapgear.com wrote:
[snip]
>   /****************************************************************************/
> @@ -65,22 +62,22 @@ static int __init uclinux_mtd_init(void)
>   		mapp->size = PAGE_ALIGN(ntohl(*((unsigned long *)(mapp->phys + 8))));
>   	mapp->bankwidth = 4;
>
> -	printk("uclinux[mtd]: RAM probe address=0x%x size=0x%x\n",
> +	printk(KERN_NOTICE "uclinux[mtd]: RAM probe address=0x%x size=0x%x\n",
>   	       	(int) mapp->phys, (int) mapp->size);
>
> -	mapp->virt = phys_to_virt(mapp->phys);
> +	mapp->virt = (void __iomem *) (unsigned long) phys_to_virt(mapp->phys);

I am a little un-easy with this casting. It would seem to indicate
some type of abuse of the virt field...

But this same thing has been done in other mtd mapping drivers, for
example drivers/mtd/maps/amd76xrom.c.

Regards
Greg


------------------------------------------------------------------------
Greg Ungerer  --  Principal Engineer        EMAIL:     gerg@snapgear.com
SnapGear Group, McAfee                      PHONE:       +61 7 3435 2888
8 Gardner Close                             FAX:         +61 7 3217 5323
Milton, QLD, 4064, Australia                WEB: http://www.SnapGear.com

Re: [PATCH] mtd: clean up uclinux.c map driver

[Artem Bityutskiy]

[-- Attachment #1: Type: text/plain, Size: 789 bytes --]

On Tue, 2012-05-15 at 14:08 +1000, gerg@snapgear.com wrote:
> From: Greg Ungerer <gerg@uclinux.org>
> 
> Perform a number of cleanups on the uclinux.c map driver.
> No structural or semantic changes, only minor cleanups.
> 
> . insert appropriate prefix into printk() calls
> . remove redundant "if" checks in the module exit code
> . remove unnecessary includes
> . make the struct uclinux_ram_map static
> . cast the virtual address calculations to keep them sparse clean
> 
> Signed-off-by: Greg Ungerer <gerg@uclinux.org>

Thanks, pushed all 3 patches to l2-mtd.git.

Side note: why do you Cc uclinux-dev@uclinux.org if it is closed mailing
list. Why do you need to make people replying you see rejects from the
mailing list?

-- 
Best Regards,
Artem Bityutskiy

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] mtd: clean up uclinux.c map driver

[Greg Ungerer]

On 15/05/12 23:12, Artem Bityutskiy wrote:
> On Tue, 2012-05-15 at 14:08 +1000, gerg@snapgear.com wrote:
>> From: Greg Ungerer<gerg@uclinux.org>
>>
>> Perform a number of cleanups on the uclinux.c map driver.
>> No structural or semantic changes, only minor cleanups.
>>
>> . insert appropriate prefix into printk() calls
>> . remove redundant "if" checks in the module exit code
>> . remove unnecessary includes
>> . make the struct uclinux_ram_map static
>> . cast the virtual address calculations to keep them sparse clean
>>
>> Signed-off-by: Greg Ungerer<gerg@uclinux.org>
>
> Thanks, pushed all 3 patches to l2-mtd.git.

Thanks.


> Side note: why do you Cc uclinux-dev@uclinux.org if it is closed mailing
> list. Why do you need to make people replying you see rejects from the
> mailing list?

Most of the users of the uclinux.c map driver will be on the uclinux
mailing list. It is very annoying that it is a closed list, but at
least there is a chance they know changes are being made to some code
they use.

Regards
Greg



------------------------------------------------------------------------
Greg Ungerer  --  Principal Engineer        EMAIL:     gerg@snapgear.com
SnapGear Group, McAfee                      PHONE:       +61 7 3435 2888
8 Gardner Close                             FAX:         +61 7 3217 5323
Milton, QLD, 4064, Australia                WEB: http://www.SnapGear.com

Re: [uClinux-dev] [PATCH] mtd: clean up uclinux.c map driver

[Mike Frysinger]

On Tue, May 15, 2012 at 12:08 AM,  <gerg@snapgear.com> wrote:
> . make the struct uclinux_ram_map static

NAK: this breaks Blackfin systems.  we specifically don't want this to
be static.

it should probably get a comment added above it saying as much.
-mike

Re: [uClinux-dev] [PATCH] mtd: clean up uclinux.c map driver

[Greg Ungerer]

On 16/05/12 01:57, Mike Frysinger wrote:
> On Tue, May 15, 2012 at 12:08 AM,<gerg@snapgear.com>  wrote:
>> . make the struct uclinux_ram_map static
>
> NAK: this breaks Blackfin systems.  we specifically don't want this to
> be static.
>
> it should probably get a comment added above it saying as much.

A comment won't fix the sparse warning. You need a proper declaration.

Regards
Greg



------------------------------------------------------------------------
Greg Ungerer  --  Principal Engineer        EMAIL:     gerg@snapgear.com
SnapGear Group, McAfee                      PHONE:       +61 7 3435 2888
8 Gardner Close                             FAX:         +61 7 3217 5323
Milton, QLD, 4064, Australia                WEB: http://www.SnapGear.com

Re: [uClinux-dev] [PATCH] mtd: clean up uclinux.c map driver

[Mike Frysinger]

On Tue, May 15, 2012 at 8:45 PM, Greg Ungerer <gerg@snapgear.com> wrote:
> On 16/05/12 01:57, Mike Frysinger wrote:
>> On Tue, May 15, 2012 at 12:08 AM,<gerg@snapgear.com>  wrote:
>>>
>>> . make the struct uclinux_ram_map static
>>
>>
>> NAK: this breaks Blackfin systems.  we specifically don't want this to
>> be static.
>>
>> it should probably get a comment added above it saying as much.
>
> A comment won't fix the sparse warning. You need a proper declaration.

perhaps, but marking it static to fix a warning that people rarely see
whilst simultaneously knowingly breaking an arch doesn't sound like
the correct trade off.
-mike

Re: [uClinux-dev] [PATCH] mtd: clean up uclinux.c map driver

[Greg Ungerer]

On 16/05/12 12:42, Mike Frysinger wrote:
> On Tue, May 15, 2012 at 8:45 PM, Greg Ungerer<gerg@snapgear.com>  wrote:
>> On 16/05/12 01:57, Mike Frysinger wrote:
>>> On Tue, May 15, 2012 at 12:08 AM,<gerg@snapgear.com>  áwrote:
>>>>
>>>> . make the struct uclinux_ram_map static
>>>
>>>
>>> NAK: this breaks Blackfin systems. áwe specifically don't want this to
>>> be static.
>>>
>>> it should probably get a comment added above it saying as much.
>>
>> A comment won't fix the sparse warning. You need a proper declaration.
>
> perhaps, but marking it static to fix a warning that people rarely see
> whilst simultaneously knowingly breaking an arch doesn't sound like
> the correct trade off.

I agree, of course. It wasn't done to knowingly break an arch. But
the sparse warning can be fixed with a proper declaration, that
would avoid you having a local extern for it in
arch/blackfin/kernel/setup.c as well. Cleaner all round.

Regards
Greg



------------------------------------------------------------------------
Greg Ungerer  --  Principal Engineer        EMAIL:     gerg@snapgear.com
SnapGear Group, McAfee                      PHONE:       +61 7 3435 2888
8 Gardner Close                             FAX:         +61 7 3217 5323
Milton, QLD, 4064, Australia                WEB: http://www.SnapGear.com

Re: [uClinux-dev] [PATCH] mtd: clean up uclinux.c map driver

[Mike Frysinger]

On Tue, May 15, 2012 at 10:55 PM, Greg Ungerer <gerg@snapgear.com> wrote:
> On 16/05/12 12:42, Mike Frysinger wrote:
>> On Tue, May 15, 2012 at 8:45 PM, Greg Ungerer<gerg@snapgear.com>  wrote:
>>> On 16/05/12 01:57, Mike Frysinger wrote:
>>>> On Tue, May 15, 2012 at 12:08 AM,<gerg@snapgear.com>  Ã¡wrote:
>>>>> . make the struct uclinux_ram_map static
>>>>
>>>> NAK: this breaks Blackfin systems. áwe specifically don't want this to
>>>> be static.
>>>> it should probably get a comment added above it saying as much.
>>>
>>> A comment won't fix the sparse warning. You need a proper declaration.
>>
>> perhaps, but marking it static to fix a warning that people rarely see
>> whilst simultaneously knowingly breaking an arch doesn't sound like
>> the correct trade off.
>
> I agree, of course. It wasn't done to knowingly break an arch. But
> the sparse warning can be fixed with a proper declaration, that
> would avoid you having a local extern for it in
> arch/blackfin/kernel/setup.c as well. Cleaner all round.

i thought you were going for merging anyways.

where would you suggest adding such a decl ?  there isn't an existing
one i can see that this would fit into.  might have to create a new
one just for this ?
-mike

Re: [uClinux-dev] [PATCH] mtd: clean up uclinux.c map driver

[Greg Ungerer]

On 05/16/2012 03:02 PM, Mike Frysinger wrote:
> On Tue, May 15, 2012 at 10:55 PM, Greg Ungerer<gerg@snapgear.com>  wrote:
>> On 16/05/12 12:42, Mike Frysinger wrote:
>>> On Tue, May 15, 2012 at 8:45 PM, Greg Ungerer<gerg@snapgear.com>  áwrote:
>>>> On 16/05/12 01:57, Mike Frysinger wrote:
>>>>> On Tue, May 15, 2012 at 12:08 AM,<gerg@snapgear.com>  á├íwrote:
>>>>>> . make the struct uclinux_ram_map static
>>>>>
>>>>> NAK: this breaks Blackfin systems. áwe specifically don't want this to
>>>>> be static.
>>>>> it should probably get a comment added above it saying as much.
>>>>
>>>> A comment won't fix the sparse warning. You need a proper declaration.
>>>
>>> perhaps, but marking it static to fix a warning that people rarely see
>>> whilst simultaneously knowingly breaking an arch doesn't sound like
>>> the correct trade off.
>>
>> I agree, of course. It wasn't done to knowingly break an arch. But
>> the sparse warning can be fixed with a proper declaration, that
>> would avoid you having a local extern for it in
>> arch/blackfin/kernel/setup.c as well. Cleaner all round.
>
> i thought you were going for merging anyways.
>
> where would you suggest adding such a decl ?  there isn't an existing
> one i can see that this would fit into.  might have to create a new
> one just for this ?

No I don't see any existing place that makes any sense. I guess it
could be something like a new file include/linux/mtd/uclinux.h.

But it looks like Artem is ok with just reverting it to not be static.
I am happy to leave it that way if you are.

Regards
Greg



------------------------------------------------------------------------
Greg Ungerer  --  Principal Engineer        EMAIL:     gerg@snapgear.com
SnapGear Group, McAfee                      PHONE:       +61 7 3435 2888
8 Gardner Close,                            FAX:         +61 7 3891 3630
Milton, QLD, 4064, Australia                WEB: http://www.SnapGear.com

Re: [uClinux-dev] [PATCH] mtd: clean up uclinux.c map driver

[Mike Frysinger]

On Wed, May 16, 2012 at 7:49 AM, Greg Ungerer <gerg@snapgear.com> wrote:
> On 05/16/2012 03:02 PM, Mike Frysinger wrote:
>>
>> On Tue, May 15, 2012 at 10:55 PM, Greg Ungerer<gerg@snapgear.com>  wrote:
>>>
>>> On 16/05/12 12:42, Mike Frysinger wrote:
>>>>
>>>> On Tue, May 15, 2012 at 8:45 PM, Greg Ungerer<gerg@snapgear.com>
>>>>  Ã¡wrote:
>>>>>
>>>>> On 16/05/12 01:57, Mike Frysinger wrote:
>>>>>>
>>>>>> On Tue, May 15, 2012 at 12:08 AM,<gerg@snapgear.com>  Ã¡â”œÃ­wrote:
>>>>>>
>>>>>>> . make the struct uclinux_ram_map static
>>>>>>
>>>>>>
>>>>>> NAK: this breaks Blackfin systems. áwe specifically don't want
>>>>>> this to
>>>>>>
>>>>>> be static.
>>>>>> it should probably get a comment added above it saying as much.
>>>>>
>>>>>
>>>>> A comment won't fix the sparse warning. You need a proper declaration.
>>>>
>>>>
>>>> perhaps, but marking it static to fix a warning that people rarely see
>>>> whilst simultaneously knowingly breaking an arch doesn't sound like
>>>> the correct trade off.
>>>
>>>
>>> I agree, of course. It wasn't done to knowingly break an arch. But
>>> the sparse warning can be fixed with a proper declaration, that
>>> would avoid you having a local extern for it in
>>> arch/blackfin/kernel/setup.c as well. Cleaner all round.
>>
>>
>> i thought you were going for merging anyways.
>>
>> where would you suggest adding such a decl ?  there isn't an existing
>> one i can see that this would fit into.  might have to create a new
>> one just for this ?
>
>
> No I don't see any existing place that makes any sense. I guess it
> could be something like a new file include/linux/mtd/uclinux.h.
>
> But it looks like Artem is ok with just reverting it to not be static.
> I am happy to leave it that way if you are.

would be good to add a comment so someone doesn't clean this up again.
 i can post a patch for that though.

i know the current struct behavior is ugly, but it's cleaner than
previous iterations ;)
-mike

Re: [uClinux-dev] [PATCH] mtd: clean up uclinux.c map driver

[Artem Bityutskiy]

[-- Attachment #1: Type: text/plain, Size: 859 bytes --]

On Tue, 2012-05-15 at 11:57 -0400, Mike Frysinger wrote:
> On Tue, May 15, 2012 at 12:08 AM,  <gerg@snapgear.com> wrote:
> > . make the struct uclinux_ram_map static
> 
> NAK: this breaks Blackfin systems.  we specifically don't want this to
> be static.
> 
> it should probably get a comment added above it saying as much.

I've just amended that patch and made it non-static as well. But this is
really ugly dependency.

diff --git a/drivers/mtd/maps/uclinux.c b/drivers/mtd/maps/uclinux.c
index ad75346..d8aa6df 100644
--- a/drivers/mtd/maps/uclinux.c
+++ b/drivers/mtd/maps/uclinux.c
@@ -21,7 +21,7 @@
 
 extern char _ebss;
 
-static struct map_info uclinux_ram_map = {
+struct map_info uclinux_ram_map = {
        .name = "RAM",
        .phys = (unsigned long)&_ebss,
        .size = 0,


-- 
Best Regards,
Artem Bityutskiy

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]