[U-Boot-Users] [PATCH] Fill in remaining MTD driver data for OneNAND (take #2)

[U-Boot-Users] [PATCH] Fill in remaining MTD driver data for OneNAND (take #2)

[Fathi BOUDRA]

Fill in remaining MTD driver data for OneNAND.
Review onenand_print_device_info():
 - Return device info to fill mtd device name.
 - Remove verbose parameter as it become useless.

Since last comments:
 - Include malloc.h
 - Initialize dev_info pointer.

Signed-off-by: Fathi Boudra <fabo@debian.org>
---
diff --git a/common/cmd_onenand.c b/common/cmd_onenand.c
index ce99a38..c9a0627 100644
--- a/common/cmd_onenand.c
+++ b/common/cmd_onenand.c
@@ -38,7 +38,7 @@ int do_onenand(cmd_tbl_t * cmdtp, int flag, int argc, char 
*argv[])
 			onenand_init();
 			return 0;
 		}
-		onenand_print_device_info(onenand_chip.device_id, 1);
+		printf("%s\n", onenand_mtd.name);
 		return 0;
 
 	default:
diff --git a/drivers/mtd/onenand/onenand_base.c 
b/drivers/mtd/onenand/onenand_base.c
index d32e382..8d76724 100644
--- a/drivers/mtd/onenand/onenand_base.c
+++ b/drivers/mtd/onenand/onenand_base.c
@@ -19,6 +19,7 @@
 
 #include <asm/io.h>
 #include <asm/errno.h>
+#include <malloc.h>
 
 /* It should access 16-bit instead of 8-bit */
 static inline void *memcpy_16(void *dst, const void *src, unsigned int len)
@@ -1112,21 +1113,21 @@ int onenand_unlock(struct mtd_info *mtd, loff_t ofs, 
size_t len)
  *
  * Print device ID
  */
-void onenand_print_device_info(int device, int verbose)
+char * onenand_print_device_info(int device)
 {
 	int vcc, demuxed, ddp, density;
-
-	if (!verbose)
-		return;
+	char *dev_info = (char *)malloc(80);
 
 	vcc = device & ONENAND_DEVICE_VCC_MASK;
 	demuxed = device & ONENAND_DEVICE_IS_DEMUX;
 	ddp = device & ONENAND_DEVICE_IS_DDP;
 	density = device >> ONENAND_DEVICE_DENSITY_SHIFT;
-	printk(KERN_INFO "%sOneNAND%s %dMB %sV 16-bit (0x%02x)\n",
+	sprintf(dev_info, "%sOneNAND%s %dMB %sV 16-bit (0x%02x)",
 	       demuxed ? "" : "Muxed ",
 	       ddp ? "(DDP)" : "",
 	       (16 << density), vcc ? "2.65/3.3" : "1.8", device);
+
+	return dev_info;
 }
 
 static const struct onenand_manufacturers onenand_manuf_ids[] = {
@@ -1205,7 +1206,7 @@ static int onenand_probe(struct mtd_info *mtd)
 	}
 
 	/* Flash device information */
-	onenand_print_device_info(dev_id, 0);
+	mtd->name = onenand_print_device_info(dev_id);
 	this->device_id = dev_id;
 
 	density = dev_id >> ONENAND_DEVICE_DENSITY_SHIFT;
@@ -1241,6 +1242,18 @@ static int onenand_probe(struct mtd_info *mtd)
 		this->options |= ONENAND_CONT_LOCK;
 	}
 
+	/* Fill in remaining MTD driver data */
+	mtd->erase = onenand_erase;
+	mtd->read = onenand_read;
+	mtd->write = onenand_write;
+	mtd->read_ecc = onenand_read_ecc;
+	mtd->write_ecc = onenand_write_ecc;
+	mtd->read_oob = onenand_read_oob;
+	mtd->write_oob = onenand_write_oob;
+	mtd->sync = onenand_sync;
+	mtd->block_isbad = onenand_block_isbad;
+	mtd->block_markbad = onenand_block_markbad;
+
 	return 0;
 }
 
diff --git a/include/onenand_uboot.h b/include/onenand_uboot.h
index 4449f98..cb7914d 100644
--- a/include/onenand_uboot.h
+++ b/include/onenand_uboot.h
@@ -39,6 +39,6 @@ extern int onenand_erase(struct mtd_info *mtd, struct 
erase_info *instr);
 
 extern int onenand_unlock(struct mtd_info *mtd, loff_t ofs, size_t len);
 
-extern void onenand_print_device_info(int device, int verbose);
+extern char * onenand_print_device_info(int device);
 
 #endif /* __UBOOT_ONENAND_H */

[U-Boot-Users] [PATCH] Fill in remaining MTD driver data for OneNAND (take #2)

[Scott Wood]

On Wed, Aug 06, 2008 at 10:06:20AM +0200, Fathi BOUDRA wrote:
> -void onenand_print_device_info(int device, int verbose)
> +char * onenand_print_device_info(int device)

No space after unary '*' (here and elsewhere).

>  {
>  	int vcc, demuxed, ddp, density;
> -
> -	if (!verbose)
> -		return;
> +	char *dev_info = (char *)malloc(80);

Don't cast the return of malloc.

Why not just declare a static array?

> -	printk(KERN_INFO "%sOneNAND%s %dMB %sV 16-bit (0x%02x)\n",
> +	sprintf(dev_info, "%sOneNAND%s %dMB %sV 16-bit (0x%02x)",
>  	       demuxed ? "" : "Muxed ",
>  	       ddp ? "(DDP)" : "",
>  	       (16 << density), vcc ? "2.65/3.3" : "1.8", device);

It'd be better to use snprintf, even if you're pretty sure it won't
overflow.

-Scott

[U-Boot-Users] [PATCH] Fill in remaining MTD driver data for OneNAND (take #2)

[Wolfgang Denk]

In message <20080806160947.GA2640@ld0162-tx32.am.freescale.net> you wrote:
> 
> > -	printk(KERN_INFO "%sOneNAND%s %dMB %sV 16-bit (0x%02x)\n",
> > +	sprintf(dev_info, "%sOneNAND%s %dMB %sV 16-bit (0x%02x)",
> >  	       demuxed ? "" : "Muxed ",
> >  	       ddp ? "(DDP)" : "",
> >  	       (16 << density), vcc ? "2.65/3.3" : "1.8", device);
> 
> It'd be better to use snprintf, even if you're pretty sure it won't
> overflow.

This in turn requires that you test the return code for overflowing
the buffer - otherwise there is the risk of using an unterminated
string.

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 at denx.de
A good aphorism is too hard for the tooth of time, and  is  not  worn
away  by  all  the  centuries,  although  it serves as food for every
epoch.                                  - Friedrich Wilhelm Nietzsche
                          _Miscellaneous Maxims and Opinions_ no. 168

[U-Boot-Users] [PATCH] Fill in remaining MTD driver data for OneNAND (take #2)

[Fathi BOUDRA]

> Don't cast the return of malloc.

ok.

> Why not just declare a static array?

I tried with a static array but it doesn't give the expected result (a quick 
test with onenand info command returns an empty mtd name), so I used a 
pointer.

> It'd be better to use snprintf, even if you're pretty sure it won't
> overflow.

I tried it too :) U-Boot doesn't have snprintf or asprintf or I missed 
something.

Do I need to re-submit the patch only for cast ?

cheers,

Fathi

[U-Boot-Users] [PATCH] Fill in remaining MTD driver data for OneNAND (take #2)

[Scott Wood]

Fathi BOUDRA wrote:
>> Why not just declare a static array?
> 
> I tried with a static array but it doesn't give the expected result (a quick 
> test with onenand info command returns an empty mtd name), so I used a 
> pointer.

Odd... Maybe a relocation issue?

>> It'd be better to use snprintf, even if you're pretty sure it won't
>> overflow.
> 
> I tried it too :) U-Boot doesn't have snprintf or asprintf or I missed 
> something.

Hmm, so it doesn't.  That sucks.

> Do I need to re-submit the patch only for cast ?

No, I'll fix it and apply to nand-flash/testing.

-Scott