[U-Boot] [PATCH] dm: implement a cfi flash uclass

Thomas Chou
2015-10-09T13:31:59Z

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