Message ID | 1366707297-31309-7-git-send-email-b32955@freescale.com |
---|---|
State | New, archived |
Headers | show |
Last nitpicks for v3 :) On Tue, Apr 23, 2013 at 1:54 AM, Huang Shijie <b32955@freescale.com> wrote: > Add the @ecc_info in the nand_flash_dev{}. > The lower 16 bits are used to store the ECC bits, while the upper 16 bits > are used to store the ECC data chunk size. A bit late on this one, but is there a good reason this wasn't just 2 separate 16-bit fields? We already have a few, and I don't see why this couldn't be the same. > Signed-off-by: Huang Shijie <b32955@freescale.com> > --- > include/linux/mtd/nand.h | 10 ++++++++++ > 1 files changed, 10 insertions(+), 0 deletions(-) > > diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h > index 063e517..f4c7777 100644 > --- a/include/linux/mtd/nand.h > +++ b/include/linux/mtd/nand.h > @@ -624,6 +624,10 @@ struct nand_chip { > { .name = (nm), {{ .dev_id = (devid) }}, .chipsize = (chipsz), \ > .options = (opts) } > > +#define NAND_ECC_INFO(strength, size) (((strength) << 16) | (size)) We could redefine this: #define NAND_ECC_INFO(strength, size) .ecc_strength = (strength), .ecc_size = (size) > +#define NAND_ECC_STRENGTH(x) (((x) >> 16) & 0xffff) > +#define NAND_ECC_SIZE(x) ((x) & 0xffff) Then we could just drop these unpacking macros. > /** > * struct nand_flash_dev - NAND Flash Device ID Structure > * @name: a human-readable name of the NAND chip > @@ -641,6 +645,11 @@ struct nand_chip { > * @options: stores various chip bit options > * @id_len: The valid length of the @id. > * @oobsize: OOB size > + * @ecc_info: The ECC information. > + * lower 16 bits: store the ECC bits. > + * upper 16 bits: store the ECC data chunk size. > + * For example, the "4bit ECC for each 512Byte" can be set with > + * NAND_ECC_INFO(4, 512) macro. > */ > struct nand_flash_dev { > char *name; > @@ -657,6 +666,7 @@ struct nand_flash_dev { > unsigned int options; > uint16_t id_len; > uint16_t oobsize; > + uint32_t ecc_info; This could be split to: uint16_t ecc_strength; uint16_t ecc_size; > }; > > /** Brian
于 2013年04月25日 14:57, Brian Norris 写道: > > A bit late on this one, but is there a good reason this wasn't just 2 > separate 16-bit fields? We already have a few, and I don't see why > this couldn't be the same. > I just want to make the ecc_strength/ecc_size more coupled for the nand_flash_dev{}. If we spilit to two fields. It makes the nand_flash_ids[] less readable. >> Signed-off-by: Huang Shijie<b32955@freescale.com> >> --- >> include/linux/mtd/nand.h | 10 ++++++++++ >> 1 files changed, 10 insertions(+), 0 deletions(-) >> >> diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h >> index 063e517..f4c7777 100644 >> --- a/include/linux/mtd/nand.h >> +++ b/include/linux/mtd/nand.h >> @@ -624,6 +624,10 @@ struct nand_chip { >> { .name = (nm), {{ .dev_id = (devid) }}, .chipsize = (chipsz), \ >> .options = (opts) } >> >> +#define NAND_ECC_INFO(strength, size) (((strength)<< 16) | (size)) > We could redefine this: > > #define NAND_ECC_INFO(strength, size) .ecc_strength = (strength), > .ecc_size = (size) Do this type of macro could be accepted by the kernel? I think your macro needs a pair of bracket, such as: #define NAND_ECC_INFO(strength, size) (.ecc_strength = (strength), .ecc_size = (size)) But if we add the brackets, we will meet a compiler error. thanks Huang Shijie
On Thu, Apr 25, 2013 at 1:56 AM, Huang Shijie <b32955@freescale.com> wrote: > 于 2013年04月25日 14:57, Brian Norris 写道: > >> >> A bit late on this one, but is there a good reason this wasn't just 2 >> separate 16-bit fields? We already have a few, and I don't see why >> this couldn't be the same. >> > I just want to make the ecc_strength/ecc_size more coupled for the > nand_flash_dev{}. > If we spilit to two fields. It makes the nand_flash_ids[] less readable. Sure, that's a decent reason. But how about as an instance of an anonymous struct instead? See my suggestion below. >>> Signed-off-by: Huang Shijie<b32955@freescale.com> >>> --- >>> include/linux/mtd/nand.h | 10 ++++++++++ >>> 1 files changed, 10 insertions(+), 0 deletions(-) >>> >>> diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h >>> index 063e517..f4c7777 100644 >>> --- a/include/linux/mtd/nand.h >>> +++ b/include/linux/mtd/nand.h >>> @@ -624,6 +624,10 @@ struct nand_chip { >>> { .name = (nm), {{ .dev_id = (devid) }}, .chipsize = (chipsz), \ >>> .options = (opts) } >>> >>> +#define NAND_ECC_INFO(strength, size) (((strength)<< 16) | (size)) >> >> We could redefine this: >> >> #define NAND_ECC_INFO(strength, size) .ecc_strength = (strength), >> .ecc_size = (size) > > Do this type of macro could be accepted by the kernel? Something like this should be acceptable, I think, if I can straighten it out so that it compiles right (!) > I think your macro needs a pair of bracket, such as: > #define NAND_ECC_INFO(strength, size) (.ecc_strength = (strength), .ecc_size > = (size)) > > But if we add the brackets, we will meet a compiler error. Here's my modification (note the underscores, since we can't have the field .strength in the same macro as the "parameter" strength): #define ECC_INFO(_strength, _size) { .strength = (_strength), .size = (_size) } #define ECC_STRENGTH(type) ((type)->ecc.strength) #define ECC_SIZE(type) ((type)->ecc.strength) And replace the field in nand_flash_dev with: struct { uint16_t strength; uint16_t size; } ecc; How does that look? It's actually quite similar to the construct Artem used with mfr_id and dev_id, except that we give the struct a name. And this time, it actually compiles! Brian
于 2013年04月26日 14:15, Brian Norris 写道: > How does that look? It's actually quite similar to the construct Artem > used with mfr_id and dev_id, except that we give the struct a name. > And this time, it actually compiles! > It's ok to me. :) I will use it in the next version. thanks Huang Shijie
diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h index 063e517..f4c7777 100644 --- a/include/linux/mtd/nand.h +++ b/include/linux/mtd/nand.h @@ -624,6 +624,10 @@ struct nand_chip { { .name = (nm), {{ .dev_id = (devid) }}, .chipsize = (chipsz), \ .options = (opts) } +#define NAND_ECC_INFO(strength, size) (((strength) << 16) | (size)) +#define NAND_ECC_STRENGTH(x) (((x) >> 16) & 0xffff) +#define NAND_ECC_SIZE(x) ((x) & 0xffff) + /** * struct nand_flash_dev - NAND Flash Device ID Structure * @name: a human-readable name of the NAND chip @@ -641,6 +645,11 @@ struct nand_chip { * @options: stores various chip bit options * @id_len: The valid length of the @id. * @oobsize: OOB size + * @ecc_info: The ECC information. + * lower 16 bits: store the ECC bits. + * upper 16 bits: store the ECC data chunk size. + * For example, the "4bit ECC for each 512Byte" can be set with + * NAND_ECC_INFO(4, 512) macro. */ struct nand_flash_dev { char *name; @@ -657,6 +666,7 @@ struct nand_flash_dev { unsigned int options; uint16_t id_len; uint16_t oobsize; + uint32_t ecc_info; }; /**
Add the @ecc_info in the nand_flash_dev{}. The lower 16 bits are used to store the ECC bits, while the upper 16 bits are used to store the ECC data chunk size. Signed-off-by: Huang Shijie <b32955@freescale.com> --- include/linux/mtd/nand.h | 10 ++++++++++ 1 files changed, 10 insertions(+), 0 deletions(-)