Patchwork [V4,5/9] mtd: replace the hardcode with the onfi_feature()

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

Comments

Huang Shijie - April 26, 2013, 9:08 a.m.
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(-)
pekon gupta - April 30, 2013, 10:04 a.m.
> -	*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
Huang Shijie - May 2, 2013, 2:14 a.m.
于 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
pekon gupta - May 2, 2013, 5:42 a.m.
> >> -	*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
Huang Shijie - May 2, 2013, 6:01 a.m.
于 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
pekon gupta - May 2, 2013, 6:17 a.m.
> 于 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
Huang Shijie - May 2, 2013, 6:48 a.m.
于 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
Brian Norris - May 2, 2013, 9:24 p.m.
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

Patch

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;