From mboxrd@z Thu Jan  1 00:00:00 1970
From: Scott Wood <scottwood@freescale.com>
Date: Thu, 5 Jul 2012 20:28:26 -0500
Subject: [U-Boot] [PATCH v3 6/7] tegra: nand: Add Tegra NAND driver
In-Reply-To: <4B9C9637D5087840A465BDCB251780E9E2D6EDA3FA@HKMAIL02.nvidia.com>
References: <1334688614-4977-1-git-send-email-sjg@chromium.org>
	<1334688614-4977-7-git-send-email-sjg@chromium.org>
	<4F9877DC.8050605@freescale.com>
	<4B9C9637D5087840A465BDCB251780E9E2D6EDA3FA@HKMAIL02.nvidia.com>
Message-ID: <4FF63F3A.7060402@freescale.com>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

On 07/04/2012 02:46 AM, Jim Lin wrote:
>> -----Original Message-----
>> From: Scott Wood [mailto:scottwood at freescale.com] 
>> Sent: Thursday, April 26, 2012 6:17 AM
>> To: Simon Glass
>> Cc: U-Boot Mailing List; Tom Warren; Stephen Warren; Jim Lin; Stephen Warren
>> Subject: Re: [PATCH v3 6/7] tegra: nand: Add Tegra NAND driver
>>
>> On 04/17/2012 01:50 PM, Simon Glass wrote:
>>> +static void write_buf(struct mtd_info *mtd, const uint8_t *buf, int len)
>>> +{
>>> +	int i, j, l;
>>> +	struct nand_chip *chip = mtd->priv;
>>> +	struct nand_drv *info;
>>> +
>>> +	info = (struct nand_drv *)chip->priv;
>>> +
>>> +	for (i = 0; i < len / 4; i++) {
>>> +		l = ((int *)buf)[i];
>>
>> If you're assuming the buffer is 32-bit aligned, comment it.  Ideally
>> these assumptions should be stated in the interface itself...
> This doesn't mean that buf needs to be 32-bit aligned.
> It only says each write is 32-bit.

OK, didn't realize modern ARM could deal with unaligned accesses.

>> Should also comment that there's an endian dependency here.
> What do you mean? Could you explain more or have an example?

You load a value using host endianness, and store it using a little
endian accessor.  This would be fine if it represented a real 32-bit
integer, but it's really a sequence of bytes that should not be swapped.

It's not a big deal if you don't see the driver ever being used with a
big endian host, but a comment would be nice.

>>> +/**
>>> + * Board-specific NAND initialization
>>> + *
>>> + * @param nand	nand chip info structure
>>> + * @return 0, after initialized, -1 on error
>>> + */
>>> +int board_nand_init(struct nand_chip *nand)
>>
>> Please consider using CONFIG_SYS_NAND_SELF_INIT.
> So far I don't see the demand.

I'd like to see the old way go away eventually.

> -----------------------------------------------------------------------------------
> This email message is for the sole use of the intended recipient(s) and may contain
> confidential information.  Any unauthorized review, use, disclosure or distribution
> is prohibited.  If you are not the intended recipient, please contact the sender by
> reply email and destroy all copies of the original message.
> -----------------------------------------------------------------------------------

Please try to get rid of this.  This is a public mailing list.

-Scott