Patchwork [v2,3/9] mtd: print out the cell information for nand chip

login
register
mail settings
Submitter Huang Shijie
Date Aug. 19, 2013, 2:31 a.m.
Message ID <1376879478-22128-4-git-send-email-b32955@freescale.com>
Download mbox | patch
Permalink /patch/268067/
State New
Headers show

Comments

Huang Shijie - Aug. 19, 2013, 2:31 a.m.
Print out the cell information for nand chip.

Signed-off-by: Huang Shijie <b32955@freescale.com>
---
 drivers/mtd/nand/nand_base.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)
Brian Norris - Aug. 24, 2013, 5:58 a.m.
On Mon, Aug 19, 2013 at 10:31:12AM +0800, Huang Shijie wrote:
> Print out the cell information for nand chip.
> 
> Signed-off-by: Huang Shijie <b32955@freescale.com>
> ---
>  drivers/mtd/nand/nand_base.c |    5 +++--
>  1 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> index 69c4b25..8b487d5 100644
> --- a/drivers/mtd/nand/nand_base.c
> +++ b/drivers/mtd/nand/nand_base.c
> @@ -3454,10 +3454,11 @@ ident_done:
>  		chip->cmdfunc = nand_command_lp;
>  
>  	pr_info("NAND device: Manufacturer ID: 0x%02x, Chip ID: 0x%02x (%s %s),"
> -		" %dMiB, page size: %d, OOB size: %d\n",
> +		" %dMiB, %s, page size: %d, OOB size: %d\n",
>  		*maf_id, *dev_id, nand_manuf_ids[maf_idx].name,
>  		chip->onfi_version ? chip->onfi_params.model : type->name,
> -		(int)(chip->chipsize >> 20), mtd->writesize, mtd->oobsize);
> +		(int)(chip->chipsize >> 20), nand_is_slc(chip) ? "SLC" : "MLC",
> +		mtd->writesize, mtd->oobsize);

This message is getting mighty long (approx. 120 characters when
printed). Are you sure we need all this? Maybe split into two separate
pr_info's sometime. But I'm OK with merging this for now.

>  
>  	return type;
>  }

Brian
Ezequiel Garcia - Aug. 24, 2013, 9:02 p.m.
On Fri, Aug 23, 2013 at 10:58:21PM -0700, Brian Norris wrote:
> On Mon, Aug 19, 2013 at 10:31:12AM +0800, Huang Shijie wrote:
> > Print out the cell information for nand chip.
> > 
> > Signed-off-by: Huang Shijie <b32955@freescale.com>
> > ---
> >  drivers/mtd/nand/nand_base.c |    5 +++--
> >  1 files changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> > index 69c4b25..8b487d5 100644
> > --- a/drivers/mtd/nand/nand_base.c
> > +++ b/drivers/mtd/nand/nand_base.c
> > @@ -3454,10 +3454,11 @@ ident_done:
> >  		chip->cmdfunc = nand_command_lp;
> >  
> >  	pr_info("NAND device: Manufacturer ID: 0x%02x, Chip ID: 0x%02x (%s %s),"
> > -		" %dMiB, page size: %d, OOB size: %d\n",
> > +		" %dMiB, %s, page size: %d, OOB size: %d\n",
> >  		*maf_id, *dev_id, nand_manuf_ids[maf_idx].name,
> >  		chip->onfi_version ? chip->onfi_params.model : type->name,
> > -		(int)(chip->chipsize >> 20), mtd->writesize, mtd->oobsize);
> > +		(int)(chip->chipsize >> 20), nand_is_slc(chip) ? "SLC" : "MLC",
> > +		mtd->writesize, mtd->oobsize);
> 
> This message is getting mighty long (approx. 120 characters when
> printed). Are you sure we need all this? Maybe split into two separate
> pr_info's sometime. But I'm OK with merging this for now.
> 

Hm.. well, although it's still a two-line message, I also don't like
to see this growing.

I think we should try to answer: why would anyone want to see any of
this when the kernel boots?

So maybe we should pr_info only the most important information,
and then either: 1) pr_debug the rest, or 2) show it in some debugfs files.

PS: Can't avoid thinking of Eric Raymond's rule of *silence* :-)
Huang Shijie - Aug. 25, 2013, 4:04 p.m.
On Sat, Aug 24, 2013 at 06:02:16PM -0300, Ezequiel Garcia wrote:
> On Fri, Aug 23, 2013 at 10:58:21PM -0700, Brian Norris wrote:
> > On Mon, Aug 19, 2013 at 10:31:12AM +0800, Huang Shijie wrote:
> > > Print out the cell information for nand chip.
> > > 
> > > Signed-off-by: Huang Shijie <b32955@freescale.com>
> > > ---
> > >  drivers/mtd/nand/nand_base.c |    5 +++--
> > >  1 files changed, 3 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> > > index 69c4b25..8b487d5 100644
> > > --- a/drivers/mtd/nand/nand_base.c
> > > +++ b/drivers/mtd/nand/nand_base.c
> > > @@ -3454,10 +3454,11 @@ ident_done:
> > >  		chip->cmdfunc = nand_command_lp;
> > >  
> > >  	pr_info("NAND device: Manufacturer ID: 0x%02x, Chip ID: 0x%02x (%s %s),"
> > > -		" %dMiB, page size: %d, OOB size: %d\n",
> > > +		" %dMiB, %s, page size: %d, OOB size: %d\n",
> > >  		*maf_id, *dev_id, nand_manuf_ids[maf_idx].name,
> > >  		chip->onfi_version ? chip->onfi_params.model : type->name,
> > > -		(int)(chip->chipsize >> 20), mtd->writesize, mtd->oobsize);
> > > +		(int)(chip->chipsize >> 20), nand_is_slc(chip) ? "SLC" : "MLC",
> > > +		mtd->writesize, mtd->oobsize);
> > 
> > This message is getting mighty long (approx. 120 characters when
> > printed). Are you sure we need all this? Maybe split into two separate
> > pr_info's sometime. But I'm OK with merging this for now.
> > 
> 
> Hm.. well, although it's still a two-line message, I also don't like
> to see this growing.
> 
> I think we should try to answer: why would anyone want to see any of
> this when the kernel boots?
> 
> So maybe we should pr_info only the most important information,

But what information can be regarded as the most important?

if you think the cell type information is not so important, i can
abandon this patch.

thanks
Huang Shijie

Patch

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index 69c4b25..8b487d5 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -3454,10 +3454,11 @@  ident_done:
 		chip->cmdfunc = nand_command_lp;
 
 	pr_info("NAND device: Manufacturer ID: 0x%02x, Chip ID: 0x%02x (%s %s),"
-		" %dMiB, page size: %d, OOB size: %d\n",
+		" %dMiB, %s, page size: %d, OOB size: %d\n",
 		*maf_id, *dev_id, nand_manuf_ids[maf_idx].name,
 		chip->onfi_version ? chip->onfi_params.model : type->name,
-		(int)(chip->chipsize >> 20), mtd->writesize, mtd->oobsize);
+		(int)(chip->chipsize >> 20), nand_is_slc(chip) ? "SLC" : "MLC",
+		mtd->writesize, mtd->oobsize);
 
 	return type;
 }