Re: [PATH] NAND Flash support for Intel IXP4xx platform

["Ruslan V. Sushko" <rsushko@ru.mvista.com>]

Thomas,
please see my comments below

Thomas Gleixner wrote:
> On Sat, 2007-04-28 at 15:41 +0400, Ruslan V. Sushko wrote:
>   
>> +static void ixp_write_buf(struct mtd_info *mtd, const u_char * buf, int 
>> len)
>> +{
>> +       int i;
>> +       struct nand_chip *this = mtd->priv;
>> +
>> +       for (i = 0; i < len; i++)
>> +               writeb(buf[i], this->IO_ADDR_W);
>> +}
>>     
>
> How excatly is this functionally different from the generic write_buf
> function in nand_base.c ?
>
> static void nand_write_buf(struct mtd_info *mtd, const uint8_t *buf, int len)
> {
>         int i;
>         struct nand_chip *chip = mtd->priv;
>
>         for (i = 0; i < len; i++)
>                 writeb(buf[i], chip->IO_ADDR_W);
> }
>
>   
This function should be removed. It was different for originally. ALE 
and CLE signal assertion was herre, but I decide this functionality is 
not necessary for data writing, so I've remove that, but forget to check 
remainder with generic code.
>> +static void ixp4xx_hwcontrol(struct mtd_info *mtd, int cmd, unsigned 
>> int ctrl)
>> +{
>> +       struct nand_chip *this = mtd->priv;
>> +       struct ixp4xx_faddr_info_t *addr_info = this->priv;
>>     
>
> Newline between variables and code !
>   
will fixed.
>   
>> +       if (ctrl & NAND_CTRL_CHANGE) {
>> +               addr_info->offset = (ctrl & NAND_CLE) ? IXP4XX_NAND_CLE : 0;
>> +               addr_info->offset |= (ctrl & NAND_ALE) ? IXP4XX_NAND_ALE 
>> : 0;
>>     
>
> Your patch is word wrapped !
>   
will fixed
>   
>> +               if (addr_info->chip_select)
>> +                       addr_info->chip_select(ctrl);
>> +       }
>> +
>> +       if (cmd != NAND_CMD_NONE)
>> +               writeb(cmd, this->IO_ADDR_W + addr_info->offset);
>> +}
>>     
>
> Aside of that I agree with Lennert, that we really need to get around
> and make this real platform code.
>   
Sorry don't understand
Do you propose to move all these (especially hw_ctrl function) 
functionality to platform/arch code?


Best regards,
Ruslan
> 	tglx
>
>
>