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

Ruslan V. Sushko
2007-04-28T13:42:02Z

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
>
>
>