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

Submitted by Huang Shijie on April 26, 2013, 9:08 a.m.

Details

Message ID 1366967337-5534-2-git-send-email-b32955@freescale.com
State New, archived
Headers show

Commit Message

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(-)

Comments

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 hide | download patch | download mbox

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