From mboxrd@z Thu Jan  1 00:00:00 1970
From: Thomas Chou <thomas@wytron.com.tw>
Date: Fri, 9 Oct 2015 21:31:59 +0800
Subject: [U-Boot] [PATCH] dm: implement a cfi flash uclass
In-Reply-To: <CAPnjgZ20oJhdOvzYfAZRPDkxDKPhYO7xjB9dLsKBjG+Ku5P66g@mail.gmail.com>
References: <1444289667-23775-1-git-send-email-thomas@wytron.com.tw>
 <CAPnjgZ20oJhdOvzYfAZRPDkxDKPhYO7xjB9dLsKBjG+Ku5P66g@mail.gmail.com>
Message-ID: <5617C1CF.4020408@wytron.com.tw>
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

Hi Simon,

On 10/09/2015 05:36 PM, Simon Glass wrote:
> Can you create a sandbox driver for this so you can add a test?

Yes. I will add a sandbox driver and test later.

>> @@ -348,6 +349,8 @@ static int initr_flash(void)
>>          /* update start of FLASH memory    */
>>   #ifdef CONFIG_SYS_FLASH_BASE
>>          bd->bi_flashstart = CONFIG_SYS_FLASH_BASE;
>> +#else
>> +       bd->bi_flashstart = cfi_flash_bank_addr(0);
>>   #endif
>
> Can we make this dynamic - i.e. only probe the device when it is used?
> Then we could remove initr_flash() in the DM case.

I will add this to the TODO list. Though the flash is probed to find the 
size and print message here in initr_flash. It is almost the same time 
for use.

>> +menu "CFI Flash Support"
>> +
>> +config DM_CFI_FLASH
>> +       bool "Enable Driver Model for CFI Flash drivers"
>> +       depends on DM
>> +       help
>> +         Enable driver model for CFI flash access. It uses the same API as
>> +         drivers/mtd/cfi_flash.c. But now implemented by the uclass.
>
> In the help can you explain what CFI is and what it is for?

Yes, I will do.

>> +UCLASS_DRIVER(cfi_flash) = {
>> +       .id             = UCLASS_CFI_FLASH,
>> +       .name           = "cfi_flash",
>> +       .per_device_auto_alloc_size = sizeof(struct cfi_flash_dev_priv),
>> +};
>> +
>> +struct cfi_flash_platdata {
>> +       phys_addr_t base;
>> +};
>
> Can you put this declaration at the top of the file?

I will put it to the top, as I did it at the very early version.

> Is there a binding somewhere for this?

A dts binding of cfi-flash will be added.

>> +#ifdef CONFIG_DM_CFI_FLASH
>> +phys_addr_t cfi_flash_bank_addr(int i)
>> +{
>> +       struct udevice *dev;
>> +       int ret;
>> +
>> +       ret = uclass_get_device(UCLASS_CFI_FLASH, i, &dev);
>> +       if (ret)
>> +               return ret;
>> +       if (!dev)
>> +               return -ENODEV;
>
> That function will never return a NULL dev, unless it returns an
> error. It is different from uclass_first_device(). Also are you sure
> you want uclass_get_device() and not uclass_get_device_by_seq()?

Yes, I should use by_seq. Thanks for reminding.

Best regards,
Thomas