Re: [PATCH V5 1/2] mtd: nand: move layout structure into nand_ecc_ctrl

Re: [PATCH V5 1/2] mtd: nand: move layout structure into nand_ecc_ctrl

[David Brownell]

On Thursday 16 July 2009, Troy Kisky wrote:
>                 case 2048:
> -                       nand_chip->ecc.layout = &atmel_oobinfo_large;
> +                       memcpy(&nand_chip->ecc.layout, &atmel_oobinfo_large,
> +                                       sizeof(nand_chip->ecc.layout));
>                         ecc_writel(host->ecc, MR, ATMEL_ECC_PAGESIZE_2112);
>                         break;

Surely using structure assignment

			nand_chip->ecc.layout = atmel_oobinfo_large;

would be better, everywhere?

Re: [PATCH V5 1/2] mtd: nand: move layout structure into nand_ecc_ctrl

[Troy Kisky]

David Brownell wrote:
> On Thursday 16 July 2009, Troy Kisky wrote:
>>                 case 2048:
>> -                       nand_chip->ecc.layout = &atmel_oobinfo_large;
>> +                       memcpy(&nand_chip->ecc.layout, &atmel_oobinfo_large,
>> +                                       sizeof(nand_chip->ecc.layout));
>>                         ecc_writel(host->ecc, MR, ATMEL_ECC_PAGESIZE_2112);
>>                         break;
> 
> Surely using structure assignment
> 
> 			nand_chip->ecc.layout = atmel_oobinfo_large;
> 
> would be better, everywhere?
> 
> 
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/
> 
Certainly would be easier to understand, but I thought it would generate
larger code, no?


No problem to change, I will next round, unless someone objects.

Thanks for the review.
Troy

Re: [PATCH V5 1/2] mtd: nand: move layout structure into nand_ecc_ctrl

[David Brownell]

On Friday 17 July 2009, Troy Kisky wrote:
> 
> > Surely using structure assignment
> > 
> >                       nand_chip->ecc.layout = atmel_oobinfo_large;
> > 
> > would be better, everywhere?
>
> Certainly would be easier to understand, but I thought it would generate
> larger code, no?

If it did, that would be a bug...

Re: [PATCH V5 2/2] mtd: nand: Calculate better default ecc layout

[Troy Kisky]

Troy Kisky wrote:
> Have nand_base calculate a good ecc layout
> default. A benefit of this is allowing
> a software ecc other than 3 bytes long. For example,
> 4 bit software ecc correction.
> 
> This patch should be safe because it adds new
> routines to be used. As board maintainers verify
> that the new routines work for them, they should
> send in a patch changing
> 
>    nand_scan -> nand_scan_finish
>    nand_scan_tail -> nand_scan_finish_tail
> 
> This patch will also add all oob bytes that
> are not reserved for ecc or bad block markers
> to the end of the free list. I don't think this
> can cause problems, but I would like an authority
> to say, "Yes, it's safe."
> 
I have a few checkpatch issues to deal with if this
approach is approved.

Sorry, it slipped my mind.

Troy

Re: [PATCH V5 2/2] mtd: nand: Calculate better default ecc layout

[Troy Kisky]

Troy Kisky wrote:
> Have nand_base calculate a good ecc layout
> default. A benefit of this is allowing
> a software ecc other than 3 bytes long. For example,
> 4 bit software ecc correction.
> 
> This patch should be safe because it adds new
> routines to be used. As board maintainers verify
> that the new routines work for them, they should
> send in a patch changing
> 
>    nand_scan -> nand_scan_finish
>    nand_scan_tail -> nand_scan_finish_tail

Maybe nand_probe_common, and nand_probe_common_tail are better names ?

> 
> This patch will also add all oob bytes that
> are not reserved for ecc or bad block markers
> to the end of the free list. I don't think this
> can cause problems, but I would like an authority
> to say, "Yes, it's safe."

What do you think of leaving nand_base totally alone
and add a file nand_probe.h which contain the new
routines marked as inline, or mark it __devinit or
__init ? I don't think anyone but probe calls these
functions.

Re: [PATCH V5 2/2] mtd: nand: Calculate better default ecc layout

[David Brownell]

On Thursday 23 July 2009, Troy Kisky wrote:
> What do you think of leaving nand_base totally alone
> and add a file nand_probe.h which contain the new
> routines marked as inline, or mark it __devinit or
> __init ? I don't think anyone but probe calls these
> functions.

You don't want those nand-core functions to be removed.
Drivers can be modular, and they need to call them.

Re: [PATCH V5 2/2] mtd: nand: Calculate better default ecc layout

[Troy Kisky]

David Brownell wrote:
> On Thursday 23 July 2009, Troy Kisky wrote:
>> What do you think of leaving nand_base totally alone
>> and add a file nand_probe.h which contain the new
>> routines marked as inline, or mark it __devinit or
>> __init ? I don't think anyone but probe calls these
>> functions.
> 
> You don't want those nand-core functions to be removed.
> Drivers can be modular, and they need to call them.
> 
> 
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/
> 

You misunderstand. I'm suggesting that each driver
includes nand_probe.h

That way, the routines can be freed when initialization
is complete.