Re: [PATCH V2] Nand driver for Nomadik 8815 SoC (on NHK8815 board)

[Alessandro Rubini <rubini-list@gnudd.com>]

Thanks JC for your comments.

>> +		.name	= "MemInit(NAND)",
>> +		.offset	= MTDPART_OFS_APPEND,
>> +		.size	= SZ_256K,
> MemInit?
> why do you need this?

It's mandated by the boot rom. I added a comment.

>> +	}, {
>> +		.name	= "BootLoader(NAND)",
>> +		.offset	= MTDPART_OFS_APPEND,
>> +		.size	= SZ_2M,
>> +	}, {
> 2M for U-Boot? it's quite big

I agree, but I prefer to keep compatibility with what's shipped in the
evaluation kit.

>>  static struct platform_device *nhk8815_platform_devices[] __initdata = {
>> -	/* currently empty, will add keypad, touchscreen etc */
>> +	&nhk8815_nand_device,
>> +	/* will add keypad, touchscreen etc */
>>  };
> it will be better to create a devices file as done for at91 to simplify board
> adding

There is only one board, currently. And, I confess, I don't like the at91
approach: in two projects I had to change such generic file to account for
a different in the specific board. So I've left this as is.

>> +/* control and timing registers for CS0..CS3 */
>> +#define FSMC_BCR0	((void __iomem *)(NOMADIK_FSMC_VA + 0x00))
>> +#define FSMC_BTR0	((void __iomem *)(NOMADIK_FSMC_VA + 0x04))
>> +#define FSMC_BCR1	((void __iomem *)(NOMADIK_FSMC_VA + 0x08))
>> +#define FSMC_BTR1	((void __iomem *)(NOMADIK_FSMC_VA + 0x0c))

> why not this
> 
> /* control and timing registers for CS0..CS3 */
> #define FSMC_BCR(x)	(NOMADIK_FSMC_VA + (x << 3))
> #define FSMC_BTR(x)	(NOMADIK_FSMC_VA + (x << 3) + 0x04)
> [...]

Ok, I've done it.

> by the a description of each register will be nice

I think anyone with the board will have the manual. But I added a one-liner
for each of them, as you ask.

>> +#define NOMADIK_MTU0_VA		IO_ADDRESS(NOMADIK_MTU0_BASE)
>> +#define NOMADIK_MTU1_VA		IO_ADDRESS(NOMADIK_MTU1_BASE)
> NOMADIK_MTU_VA(x) will be better

No, not here. These are physical addresses within the register area,
just random 4k addresses, with no useful ordering nor similarities
to be factorized out.

>> +static inline int parity(int b) /* uses low 8 bits: returns 0 or all-1 */
>> +{
>> +	b = b ^ (b >> 4);
>> +	b = b ^ (b >> 2);
>> +	return (b ^ (b >> 1)) & 1
>> +		? ~0 : 0;
>> +}
> in U-Boot you chose to use the asembly implementation it will we nice if you
> can keep it sync

After Scott Wood asked to re-check if asm is needed, I found that the
generated code is the same, so I finally got this exact code in U-Boot.

/alessandro