| Message ID | 1366967337-5534-6-git-send-email-b32955@freescale.com |
|---|---|
| State | New, archived |
| Headers | show |
> - *busw = 0; > - if (le16_to_cpu(p->features) & 1) > - *busw = NAND_BUSWIDTH_16; > + > + *busw = (onfi_feature(chip) & ONFI_FEATURE_16_BIT_BUS) ? > + NAND_BUSWIDTH_16 : 0; Is this really needed ? you have already checked the 'onfi_version' above in nand_flash_detect_onfi() .. if (!chip->onfi_version) { pr_info("%s: unsupported ONFI version: %d\n", __func__, val); return 0; } with regards, pekon
于 2013年04月30日 18:04, Gupta, Pekon 写道: >> - *busw = 0; >> - if (le16_to_cpu(p->features)& 1) >> - *busw = NAND_BUSWIDTH_16; >> + >> + *busw = (onfi_feature(chip)& ONFI_FEATURE_16_BIT_BUS) ? >> + NAND_BUSWIDTH_16 : 0; > Is this really needed ? you have already checked the 'onfi_version' above in > nand_flash_detect_onfi() .. > if (!chip->onfi_version) { > pr_info("%s: unsupported ONFI version: %d\n", __func__, val); > return 0; > } > > I think checking the onfi_version has no relationship with this patch. :) This patch is just replace the hardcode for 16-bit onfi nand check. thanks Huang Shijie
> >> - *busw = 0; > >> - if (le16_to_cpu(p->features)& 1) > >> - *busw = NAND_BUSWIDTH_16; > >> + > >> + *busw = (onfi_feature(chip)& ONFI_FEATURE_16_BIT_BUS) ? > >> + NAND_BUSWIDTH_16 : 0; > > Is this really needed ? you have already checked the 'onfi_version' > above in > > nand_flash_detect_onfi() .. > > if (!chip->onfi_version) { > > pr_info("%s: unsupported ONFI version: %d\n", > __func__, val); > > return 0; > > } > > > > > I think checking the onfi_version has no relationship with this patch. :) > This patch is just replace the hardcode for 16-bit onfi nand check. > [Pekon]: [Patch 3/9]: add a helper to get the supported features I mean, do you really need this helper function ? +static inline int onfi_feature(struct nand_chip *chip) +{ + return chip->onfi_version ? le16_to_cpu(chip->onfi_params.features) : 0; + } Following change should have been enough.. *busw = (le16_to_cpu(p->features) & ONFI_FEATURE_16_BIT_BUS) ? NAND_BUSWIDTH_16 : 0; with regards, pekon
于 2013年05月02日 13:42, Gupta, Pekon 写道: >>>> - *busw = 0; >>>> - if (le16_to_cpu(p->features)& 1) >>>> - *busw = NAND_BUSWIDTH_16; >>>> + >>>> + *busw = (onfi_feature(chip)& ONFI_FEATURE_16_BIT_BUS) ? >>>> + NAND_BUSWIDTH_16 : 0; >>> Is this really needed ? you have already checked the 'onfi_version' >> above in >>> nand_flash_detect_onfi() .. >>> if (!chip->onfi_version) { >>> pr_info("%s: unsupported ONFI version: %d\n", >> __func__, val); >>> return 0; >>> } >>> >>> >> I think checking the onfi_version has no relationship with this patch. :) >> This patch is just replace the hardcode for 16-bit onfi nand check. >> > [Pekon]: [Patch 3/9]: add a helper to get the supported features > I mean, do you really need this helper function ? > +static inline int onfi_feature(struct nand_chip *chip) > +{ > + return chip->onfi_version ? le16_to_cpu(chip->onfi_params.features) : 0; > + } > > Following change should have been enough.. > *busw = (le16_to_cpu(p->features)& ONFI_FEATURE_16_BIT_BUS) ? > NAND_BUSWIDTH_16 : 0; yes. for this function, it's enough. But we may use the onfi_feature() in other places, such as some drivers. That's reason why i added this helper. thanks Huang Shijie
> 于 2013年05月02日 13:42, Gupta, Pekon 写道: > >>>> - *busw = 0; > >>>> - if (le16_to_cpu(p->features)& 1) > >>>> - *busw = NAND_BUSWIDTH_16; > >>>> + > >>>> + *busw = (onfi_feature(chip)& ONFI_FEATURE_16_BIT_BUS) ? > >>>> + NAND_BUSWIDTH_16 : 0; > >>> Is this really needed ? you have already checked the 'onfi_version' > >> above in > >>> nand_flash_detect_onfi() .. > >>> if (!chip->onfi_version) { > >>> pr_info("%s: unsupported ONFI version: %d\n", > >> __func__, val); > >>> return 0; > >>> } > >>> > >>> > >> I think checking the onfi_version has no relationship with this patch. :) > >> This patch is just replace the hardcode for 16-bit onfi nand check. > >> > > [Pekon]: [Patch 3/9]: add a helper to get the supported features > > I mean, do you really need this helper function ? > > +static inline int onfi_feature(struct nand_chip *chip) > > +{ > > + return chip->onfi_version ? le16_to_cpu(chip- > >onfi_params.features) : 0; > > + } > > > > Following change should have been enough.. > > *busw = (le16_to_cpu(p->features)& ONFI_FEATURE_16_BIT_BUS) ? > > NAND_BUSWIDTH_16 : 0; > yes. for this function, it's enough. > > But we may use the onfi_feature() in other places, such as some drivers. > That's reason why i added this helper. > [Pekon]: onfi_feature() is actually not useful unless someone re-scans the param page again. And once the device is probed following should be used to check if it’s a x16 device. If (chip->options & NAND_BUSWIDTH_16) My view-point was that the code should have less redundancy :-) Anyways, Thanks for your other patch for reading extended_param_page. I too wanted that ecc.codeword and ecc.strength should come from device's parameters. I'll ack them once, I test using my driver. with regards, pekon
于 2013年05月02日 14:17, Gupta, Pekon 写道:
> [Pekon]: onfi_feature() is actually not useful unless someone re-scans the
I do not think so. :)
I think the onfi_feature() is useful in the future.
I only add the two feauture for this helper:
[1] 16-bit and
[2] extended parameter page
But in actually, we may add more feature to this helper, such as
_synchronous_.
For example, some driver may support the synchronous mode for the ONFI nand.
We can use this onfi_feature() in the driver to check if the onfi nand
supports this
synchronous feature.
Add this helper makes the code more readable, though we introduce a
little redandancy.
thanks
Huang Shijie
On Wed, May 1, 2013 at 11:48 PM, Huang Shijie <b32955@freescale.com> wrote: > 于 2013年05月02日 14:17, Gupta, Pekon 写道: > >> [Pekon]: onfi_feature() is actually not useful unless someone re-scans the > > I do not think so. :) > > I think the onfi_feature() is useful in the future. > I only add the two feauture for this helper: > [1] 16-bit and > [2] extended parameter page > > But in actually, we may add more feature to this helper, such as > _synchronous_. > For example, some driver may support the synchronous mode for the ONFI nand. > We can use this onfi_feature() in the driver to check if the onfi nand > supports this > synchronous feature. > > Add this helper makes the code more readable, though we introduce a little > redandancy. I agree with the thoughts here. Readability and reusability are improved a bit by including the ONFI version check. (BTW, I haven't forgotten this series. I had some distractions here. I will fully test and provide my Reviewed-by/Tested-by eventually...) Brian
diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c index abec615..bc3442a 100644 --- a/drivers/mtd/nand/nand_base.c +++ b/drivers/mtd/nand/nand_base.c @@ -2969,9 +2969,9 @@ static int nand_flash_detect_onfi(struct mtd_info *mtd, struct nand_chip *chip, mtd->oobsize = le16_to_cpu(p->spare_bytes_per_page); chip->chipsize = le32_to_cpu(p->blocks_per_lun); chip->chipsize *= (uint64_t)mtd->erasesize * p->lun_count; - *busw = 0; - if (le16_to_cpu(p->features) & 1) - *busw = NAND_BUSWIDTH_16; + + *busw = (onfi_feature(chip) & ONFI_FEATURE_16_BIT_BUS) ? + NAND_BUSWIDTH_16 : 0; if (p->ecc_bits != 0xff) { chip->ecc_strength = p->ecc_bits;
The current code uses the hardcode to detect the 16-bit bus width. Use the onfi_feature() to replace it. Signed-off-by: Huang Shijie <b32955@freescale.com> --- drivers/mtd/nand/nand_base.c | 6 +++--- 1 files changed, 3 insertions(+), 3 deletions(-)