Re: [PATCH 3/3] NAND: add support for reading ONFI parameters from NAND device

[Florian Fainelli <ffainelli@freebox.fr>]

Hi,

On Friday 13 August 2010 01:06:47 Brian Norris wrote:
> Hi,
> 
> On 08/12/2010 12:47 PM, Florian Fainelli wrote:
> >> Also, previously, you said:
> >>> +   if (!is_power_of_2(val) || val == 1 || val > (1 << 4)) {
> >>> the !is_power_of_2 check does not work for ONFI version 2.1 (3), so I
> >>> would only keep the two other checks."
> >> 
> >> So why is it now:
> >>> +   if (is_power_of_2(val) || val == 1 || val > (1 << 4)) {
> >> 
> >> Is that a typo? Perhaps it's better to throw that test out altogether.
> > 
> > That was not a typo, I actually misread the ONFI specification and
> > confused bit is set, with the actual value. So this is the correct
> > check, sorry about that.

I forgot to answer that, yes, that was a typo, the original code was right 
(that is: !is_power_of_2()).

> 
> Again, I might be mistaken, but it seems that the "is_power_of_2" would
> weed out ONFI 1.0, since val would be simply 1 << 1, i.e., val == 2.
> Really, it seems that the ONFI version number may or may not be a power
> of two. If so, then we should just check individual bits as performed in
> the other tests (and perhaps include a test to be sure "val & 1 == 0")

2 is still a power of 2, even though the exponent is 1 and is_power_of_2(2) 
returns true.

From my understanding of the ONFI specification, the corresponding bit is set, 
when the ONFI version is supported. What is not clear to me and this is where 
the is_power_of_2() check will return false, is in the case we have a device 
supporting, say ONFI 2.0, we might have bits 1, 2, 3, 4 being set (30), which 
is not a power of 2. I agree with you on that. So let's just check the 
invidual bits instead starting with the higher one.

> 
> >> I "fixed" the changes I mentioned as well as a few coding style, logic
> >> cleanups, etc. (e.g. too many levels of logic, creating lines > 80
> >> chars). Here's a new patch. I didn't change over the crc function to
> >> the library function because that would require configuring the Kbuild
> >> options and setting a dependency, which I'm not familiar with. I'm
> >> certainly not an expert on most of this, so take my patch with a grain
> >> of salt!
> > 
> > It is usually as simple as doing the proper select FOO in the related
> > Kconfig.
> 
> Thanks for the tip. It worked for me.

I will respin the patch with Matthieu's comments and yours.
--
Florian