| Message ID | 1362415349-7107-13-git-send-email-dedekind1@gmail.com |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, 2013-03-04 at 18:42 +0200, Artem Bityutskiy wrote: > /** > * struct nand_flash_dev - NAND Flash Device ID Structure > * @name: Identify the device type > - * @dev_id: device ID code > + * @mfr_id: manufecturer ID part of the full chip ID array (byte 0) This should be 'manufacturer'. > + * @dev_id: device ID part of the full chip ID array (byte 1)
On Mon, Mar 4, 2013 at 8:42 AM, Artem Bityutskiy <dedekind1@gmail.com> wrote: > From: Artem Bityutskiy <artem.bityutskiy@linux.intel.com> > > Signed-off-by: Artem Bityutskiy <artem.bityutskiy@linux.intel.com> > --- > include/linux/mtd/nand.h | 12 ++++++++++-- > 1 file changed, 10 insertions(+), 2 deletions(-) > > diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h > index 7af6600..951ea0d 100644 > --- a/include/linux/mtd/nand.h > +++ b/include/linux/mtd/nand.h > @@ -559,7 +559,9 @@ struct nand_chip { > /** > * struct nand_flash_dev - NAND Flash Device ID Structure > * @name: Identify the device type > - * @dev_id: device ID code > + * @mfr_id: manufecturer ID part of the full chip ID array (byte 0) > + * @dev_id: device ID part of the full chip ID array (byte 1) > + * @id: full device ID array > * @pagesize: Pagesize in bytes. Either 256 or 512 or 0 > * If the pagesize is 0, then the real pagesize > * and the eraseize are determined from the > @@ -570,7 +572,13 @@ struct nand_chip { > */ > struct nand_flash_dev { > char *name; > - int dev_id; > + union { > + struct { > + uint8_t mfr_id; > + uint8_t dev_id; > + }; > + uint8_t id[8]; > + }; I'm sorry that I was unable to fully review Huang's code while it was being discussed earlier. But from what I read, it seemed there was some disagreement on some of the format of the tables (e.g., Artem wondered why the extra zeroes). Now, it seems like instead, we're including duplicate fields. The "mfr_id" and "dev_id" are always the first two bytes of the "id[8]" field. Does it make sense to repeat them? And I actually don't ever see where "mfr_id" would be used. There's also the (yet unanswered?) question of whether we reasonably would handle in this table both those chips which have their full information listed in conjunction with their 8-byte ID string and those chips with minimal information in the table, and the rest must be decoded from extended ID. Perhaps the answer to this last question was already assumed to be "yes." But how would we differentiate the two types of table entries? Simply by seeing whether they filled out "dev_id" vs. "id[8]"? This patch doesn't clearly show how the new fields could be used. Hopefully my comments make some sense. If they don't, then that probably means we need some more discussion before including this last patch. If they do make sense, then that probably still means this last patch needs to be rewritten and/or included only along with a series that can reasonably make use of these fields (like Huang's series). > unsigned long pagesize; > unsigned long chipsize; > unsigned long erasesize; Thanks, Brian
On Tue, Mar 5, 2013 at 12:42 AM, Artem Bityutskiy <dedekind1@gmail.com> wrote: > From: Artem Bityutskiy <artem.bityutskiy@linux.intel.com> > > Signed-off-by: Artem Bityutskiy <artem.bityutskiy@linux.intel.com> > --- > include/linux/mtd/nand.h | 12 ++++++++++-- > 1 file changed, 10 insertions(+), 2 deletions(-) > > diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h > index 7af6600..951ea0d 100644 > --- a/include/linux/mtd/nand.h > +++ b/include/linux/mtd/nand.h > @@ -559,7 +559,9 @@ struct nand_chip { > /** > * struct nand_flash_dev - NAND Flash Device ID Structure > * @name: Identify the device type > - * @dev_id: device ID code > + * @mfr_id: manufecturer ID part of the full chip ID array (byte 0) > + * @dev_id: device ID part of the full chip ID array (byte 1) > + * @id: full device ID array > * @pagesize: Pagesize in bytes. Either 256 or 512 or 0 > * If the pagesize is 0, then the real pagesize > * and the eraseize are determined from the > @@ -570,7 +572,13 @@ struct nand_chip { > */ > struct nand_flash_dev { > char *name; > - int dev_id; > + union { > + struct { > + uint8_t mfr_id; > + uint8_t dev_id; > + }; > + uint8_t id[8]; > + }; > We can add a new helper like compare_nand_with_devid() to remove the mfr_id and dev_id, only left the id[8]. The compare_nand_with_devid() can be used in places where your patch 11 does. For example: .............................................................. if (id == nand_flash_ids[i].dev_id) { changed to: if (compare_nand_with_devid(id)){ ............................................................... By this way, we can remove the mfr_id and dev_id. thanks Huang Shijie
On Mon, 2013-03-04 at 11:45 -0800, Brian Norris wrote: > > struct nand_flash_dev { > > char *name; > > - int dev_id; > > + union { > > + struct { > > + uint8_t mfr_id; > > + uint8_t dev_id; > > + }; > > + uint8_t id[8]; > > + }; > > I'm sorry that I was unable to fully review Huang's code while it was > being discussed earlier. But from what I read, it seemed there was > some disagreement on some of the format of the tables (e.g., Artem > wondered why the extra zeroes). Now, it seems like instead, we're > including duplicate fields. The "mfr_id" and "dev_id" are always the > first two bytes of the "id[8]" field. Does it make sense to repeat > them? And I actually don't ever see where "mfr_id" would be used. This is not repeating. Because of the union {}, mfr_id's memory address is identical to id[0]'s address, and dev_id's memory address is identical to id[1]'s address. You can consider 'mfr_id' and 'dev_id' to be alias names for id[0] and id[1]. If one needs to have an alias name for other ID bytes, they can be added. My idea was that type->mfr_id and type->dev_id is more readable than type->id[0] and type->id[1]. > There's also the (yet unanswered?) question of whether we reasonably > would handle in this table both those chips which have their full > information listed in conjunction with their 8-byte ID string and > those chips with minimal information in the table, and the rest must > be decoded from extended ID. I was thinking that we have a single table which contains both, and we make sure that all full ID records go first, and device ID-only records go last. This would mean that we first match by full ID, and then by device IDs. > Perhaps the answer to this last question was already assumed to be > "yes." But how would we differentiate the two types of table entries? > Simply by seeing whether they filled out "dev_id" vs. "id[8]"? This > patch doesn't clearly show how the new fields could be used. I was thinking about if (type->mfr_id == 0) /* this is device-only ID */ else /* this is full ID */ or static inline bool is_full_nand_id(type) { return type->mfr_id; } > Hopefully my comments make some sense. If they don't, then that > probably means we need some more discussion before including this last > patch. If they do make sense, then that probably still means this last > patch needs to be rewritten and/or included only along with a series > that can reasonably make use of these fields (like Huang's series). Sure, this is why you was in "To:" - you are the main NAND janitor nowadays, because others mostly care about their little drivers, while you demonstrated skills and will to make the general layer sane.
On Tue, 2013-03-05 at 14:08 +0800, Huang Shijie wrote: > > struct nand_flash_dev { > > char *name; > > - int dev_id; > > + union { > > + struct { > > + uint8_t mfr_id; > > + uint8_t dev_id; > > + }; > > + uint8_t id[8]; > > + }; > > > We can add a new helper like compare_nand_with_devid() to remove the > mfr_id and dev_id, only left the id[8]. Huang, did you miss the "union { ... }" part or you think using 'id[N]' directly is readable enough?
On Tue, Mar 5, 2013 at 6:42 PM, Artem Bityutskiy <dedekind1@gmail.com> wrote: > On Tue, 2013-03-05 at 14:08 +0800, Huang Shijie wrote: >> > struct nand_flash_dev { >> > char *name; >> > - int dev_id; >> > + union { >> > + struct { >> > + uint8_t mfr_id; >> > + uint8_t dev_id; >> > + }; >> > + uint8_t id[8]; >> > + }; >> > >> We can add a new helper like compare_nand_with_devid() to remove the >> mfr_id and dev_id, only left the id[8]. > > Huang, did you miss the "union { ... }" part or you think using 'id[N]' > directly is readable enough? ｉ ｔｈｉｎｋ ｕｓｉｎｇ ｔｈｅ ｉｄ［］ ｉｓ ｅｎｏｕｇｈ． ｔｈｅ ｕｎｉｏｎ ｉｓ ｒｅｄｕｎｄａｎｔ． ｔｈａｎｋｓ Ｈｕａｎｇ Ｓｈｉｊｉｅ
On Tue, 2013-03-05 at 22:36 +0800, Huang Shijie wrote: > >> We can add a new helper like compare_nand_with_devid() to remove the > >> mfr_id and dev_id, only left the id[8]. > > > > Huang, did you miss the "union { ... }" part or you think using 'id[N]' > > directly is readable enough? > ｉ ｔｈｉｎｋ ｕｓｉｎｇ ｔｈｅ ｉｄ［］ ｉｓ ｅｎｏｕｇｈ． > ｔｈｅ ｕｎｉｏｎ ｉｓ ｒｅｄｕｎｄａｎｔ． I just find 'type->dev_id' more readable than 'type->id[1]'...
于 2013年03月05日 22:55, Artem Bityutskiy 写道: > On Tue, 2013-03-05 at 22:36 +0800, Huang Shijie wrote: >>>> We can add a new helper like compare_nand_with_devid() to remove the >>>> mfr_id and dev_id, only left the id[8]. >>> Huang, did you miss the "union { ... }" part or you think using 'id[N]' >>> directly is readable enough? >> ｉ ｔｈｉｎｋ ｕｓｉｎｇ ｔｈｅ ｉｄ［］ ｉｓ ｅｎｏｕｇｈ． >> ｔｈｅ ｕｎｉｏｎ ｉｓ ｒｅｄｕｎｄａｎｔ． > I just find 'type->dev_id' more readable than 'type->id[1]'... Just a suggestion, add more comments to the code will also make it readable. But it's ok to me if you keep the union. thanks Huang Shijie
于 2013年03月05日 18:37, Artem Bityutskiy 写道: > I was thinking about > > if (type->mfr_id == 0) > /* this is device-only ID */ > else > /* this is full ID */ > > or > > static inline bool is_full_nand_id(type) { > return type->mfr_id; > } okay. If you think we should not add another table for the full-id nand. I will follow your advice. thanks Huang Shijie
diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h index 7af6600..951ea0d 100644 --- a/include/linux/mtd/nand.h +++ b/include/linux/mtd/nand.h @@ -559,7 +559,9 @@ struct nand_chip { /** * struct nand_flash_dev - NAND Flash Device ID Structure * @name: Identify the device type - * @dev_id: device ID code + * @mfr_id: manufecturer ID part of the full chip ID array (byte 0) + * @dev_id: device ID part of the full chip ID array (byte 1) + * @id: full device ID array * @pagesize: Pagesize in bytes. Either 256 or 512 or 0 * If the pagesize is 0, then the real pagesize * and the eraseize are determined from the @@ -570,7 +572,13 @@ struct nand_chip { */ struct nand_flash_dev { char *name; - int dev_id; + union { + struct { + uint8_t mfr_id; + uint8_t dev_id; + }; + uint8_t id[8]; + }; unsigned long pagesize; unsigned long chipsize; unsigned long erasesize;