Discussion:
[uClinux-dev] [PATCH] mtd: clean up uclinux.c map driver
gerg
2012-05-15 04:08:48 UTC
Permalink
From: Greg Ungerer <gerg at 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 at 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
Greg Ungerer
2012-05-15 04:17:27 UTC
Permalink
On 15/05/12 14:08, gerg at snapgear.com wrote:
[snip]
Post by gerg
/****************************************************************************/
@@ -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 at 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
Mike Frysinger
2012-05-15 15:57:43 UTC
Permalink
Post by gerg
. 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
Greg Ungerer
2012-05-16 00:45:29 UTC
Permalink
Post by Mike Frysinger
Post by gerg
. 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 at 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
Mike Frysinger
2012-05-16 02:42:59 UTC
Permalink
Post by Greg Ungerer
Post by gerg
. 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
Greg Ungerer
2012-05-16 02:55:07 UTC
Permalink
Post by Mike Frysinger
Post by Greg Ungerer
Post by gerg
. 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 at 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
Mike Frysinger
2012-05-16 05:02:39 UTC
Permalink
Post by Greg Ungerer
Post by Mike Frysinger
Post by Greg Ungerer
Post by gerg
. 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
Greg Ungerer
2012-05-16 11:49:42 UTC
Permalink
Post by Mike Frysinger
Post by Greg Ungerer
Post by Mike Frysinger
Post by Greg Ungerer
Post by gerg
. 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 at 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
Mike Frysinger
2012-05-16 16:23:44 UTC
Permalink
Post by Greg Ungerer
Post by Mike Frysinger
Post by Greg Ungerer
On Tue, May 15, 2012 at 8:45 PM, Greg Ungerer<gerg at snapgear.com>
Post by Greg Ungerer
Post by gerg
. 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

Artem Bityutskiy
2012-05-16 09:28:52 UTC
Permalink
Post by Mike Frysinger
Post by gerg
. 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part
URL: <http://mailman.uclinux.org/pipermail/uclinux-dev/attachments/20120516/cdb052ab/attachment.sig>
Loading...