Patchwork [V4,1/9] mtd: add more comment for ecc_strength/ecc_size

login
register
mail settings
Submitter Huang Shijie
Date April 26, 2013, 9:08 a.m.
Message ID <1366967337-5534-2-git-send-email-b32955@freescale.com>
Download mbox | patch
Permalink /patch/239774/
State New
Headers show

Comments

Huang Shijie - April 26, 2013, 9:08 a.m.
Add more commit for ecc_strength and ecc_size fields.
We can treat the comment as the initial semantics for the two fields.

Signed-off-by: Huang Shijie <b32955@freescale.com>
---
 include/linux/mtd/nand.h |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)
Artem Bityutskiy - May 15, 2013, 7:27 a.m.
On Fri, 2013-04-26 at 17:08 +0800, Huang Shijie wrote:
> Add more commit for ecc_strength and ecc_size fields.
> We can treat the comment as the initial semantics for the two fields.
> 
> Signed-off-by: Huang Shijie <b32955@freescale.com>

Huang, let me drop the 3 patches I already merged. Please, re-send them
in v5. I think this is better because I see you start applying patches
on top of them, which is a bit confusing.

>   * @cellinfo:		[INTERN] MLC/multichip data from chip ident
>   * @ecc_strength:	[INTERN] ECC correctability from the datasheet.
> + *			The minimum number of bits correctability, if known;
> + *			if unknown, set to 0.

I find this confusing still. How about this comment.

ECC correctability from the datasheet. Minumum amount of bit errors per
@ecc_size guaranteed to be correctable). If unknown, set to zero.


>   * @ecc_size:		[INTERN] ECC size required by the @ecc_strength,
> - *                      also from the datasheet.
> + *                      also from the datasheet. It is the recommended ECC step
> + *			size, if known; if unknown, set to 0.

Silly question, why you call this one "ecc_size", and not "ecc_step"?
Huang Shijie - May 15, 2013, 7:38 a.m.
于 2013年05月15日 15:27, Artem Bityutskiy 写道:
> On Fri, 2013-04-26 at 17:08 +0800, Huang Shijie wrote:
>> Add more commit for ecc_strength and ecc_size fields.
>> We can treat the comment as the initial semantics for the two fields.
>>
>> Signed-off-by: Huang Shijie<b32955@freescale.com>
> Huang, let me drop the 3 patches I already merged. Please, re-send them
> in v5. I think this is better because I see you start applying patches
> on top of them, which is a bit confusing.
>
Ok, Please drop the 3 patches.

>>    * @cellinfo:		[INTERN] MLC/multichip data from chip ident
>>    * @ecc_strength:	[INTERN] ECC correctability from the datasheet.
>> + *			The minimum number of bits correctability, if known;
>> + *			if unknown, set to 0.
> I find this confusing still. How about this comment.
>
> ECC correctability from the datasheet. Minumum amount of bit errors per
> @ecc_size guaranteed to be correctable). If unknown, set to zero.
>
>
it's okay to me.
>>    * @ecc_size:		[INTERN] ECC size required by the @ecc_strength,
>> - *                      also from the datasheet.
>> + *                      also from the datasheet. It is the recommended ECC step
>> + *			size, if known; if unknown, set to 0.
> Silly question, why you call this one "ecc_size", and not "ecc_step"?
>
In nand_ecc_ctrl{}, the ecc step is named to @size.

Personally, i perfer to ecc_step.


thanks
Huang Shijie
Artem Bityutskiy - May 15, 2013, 7:42 a.m.
On Wed, 2013-05-15 at 15:38 +0800, Huang Shijie wrote:
> 于 2013年05月15日 15:27, Artem Bityutskiy 写道:
> > On Fri, 2013-04-26 at 17:08 +0800, Huang Shijie wrote:
> >> Add more commit for ecc_strength and ecc_size fields.
> >> We can treat the comment as the initial semantics for the two fields.
> >>
> >> Signed-off-by: Huang Shijie<b32955@freescale.com>
> > Huang, let me drop the 3 patches I already merged. Please, re-send them
> > in v5. I think this is better because I see you start applying patches
> > on top of them, which is a bit confusing.
> >
> Ok, Please drop the 3 patches.
> 
> >>    * @cellinfo:		[INTERN] MLC/multichip data from chip ident
> >>    * @ecc_strength:	[INTERN] ECC correctability from the datasheet.
> >> + *			The minimum number of bits correctability, if known;
> >> + *			if unknown, set to 0.
> > I find this confusing still. How about this comment.
> >
> > ECC correctability from the datasheet. Minumum amount of bit errors per
> > @ecc_size guaranteed to be correctable). If unknown, set to zero.
> >
> >
> it's okay to me.
> >>    * @ecc_size:		[INTERN] ECC size required by the @ecc_strength,
> >> - *                      also from the datasheet.
> >> + *                      also from the datasheet. It is the recommended ECC step
> >> + *			size, if known; if unknown, set to 0.
> > Silly question, why you call this one "ecc_size", and not "ecc_step"?
> >
> In nand_ecc_ctrl{}, the ecc step is named to @size.
> 
> Personally, i perfer to ecc_step.

You could harmonize the naming. Rename all the names to ecc_step, which
is a lot easier to understand.

You did not send v4 thus far, is this because you are busy or you did
not have any requests to address?

Thanks!
Huang Shijie - May 15, 2013, 8:02 a.m.
于 2013年05月15日 15:42, Artem Bityutskiy 写道:
> You did not send v4 thus far, is this because you are busy or you did
> not have any requests to address?
>
I am not busy.
I just thought the v4 is enough.

but now i will send out the v5.

thanks
Huang Shijie
Artem Bityutskiy - May 15, 2013, 10:01 a.m.
On Wed, 2013-05-15 at 16:02 +0800, Huang Shijie wrote:
> 于 2013年05月15日 15:42, Artem Bityutskiy 写道:
> > You did not send v4 thus far, is this because you are busy or you did
> > not have any requests to address?
> >
> I am not busy.
> I just thought the v4 is enough.
> 
> but now i will send out the v5.

To me it looked like Brian is not really satisfied with v4.

Patch

diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
index 5458021..a0c486b 100644
--- a/include/linux/mtd/nand.h
+++ b/include/linux/mtd/nand.h
@@ -435,8 +435,11 @@  struct nand_buffers {
  *			not bad when badblockbits == 7
  * @cellinfo:		[INTERN] MLC/multichip data from chip ident
  * @ecc_strength:	[INTERN] ECC correctability from the datasheet.
+ *			The minimum number of bits correctability, if known;
+ *			if unknown, set to 0.
  * @ecc_size:		[INTERN] ECC size required by the @ecc_strength,
- *                      also from the datasheet.
+ *                      also from the datasheet. It is the recommended ECC step
+ *			size, if known; if unknown, set to 0.
  * @numchips:		[INTERN] number of physical chips
  * @chipsize:		[INTERN] the size of one chip for multichip arrays
  * @pagemask:		[INTERN] page number mask = number of (pages / chip) - 1