[U-Boot] [PATCH v2 10/15] drivers: nand: implement a NAND uclass

[Grygorii Strashko]

On 02/10/2017 10:22 AM, Simon Glass wrote:
> Hi,
>
> On 6 February 2017 at 11:39, Grygorii Strashko <grygorii.strashko at ti.com> wrote:
>>
>>
>> On 02/06/2017 09:34 AM, Simon Glass wrote:
>>>
>>> Hi,
>>>
>>> On 31 January 2017 at 13:37, Grygorii Strashko <grygorii.strashko at ti.com>
>>> wrote:
>>>>
>>>> From: Mugunthan V N <mugunthanvnm at ti.com>
>>>>
>>>> Implement a NAND uclass so that the NAND devices can be
>>>> accessed via the DM framework.
>>>>
>>>> Signed-off-by: Mugunthan V N <mugunthanvnm at ti.com>
>>>> Signed-off-by: Grygorii Strashko <grygorii.strashko at ti.com>
>>>> ---
>>>>  drivers/mtd/nand/Kconfig       | 10 ++++++++++
>>>>  drivers/mtd/nand/Makefile      |  2 ++
>>>>  drivers/mtd/nand/nand-uclass.c | 38
>>>> ++++++++++++++++++++++++++++++++++++++
>>>>  drivers/mtd/nand/nand.c        | 17 +++++++++++++++--
>>>>  include/dm/uclass-id.h         |  1 +
>>>>  5 files changed, 66 insertions(+), 2 deletions(-)
>>>>  create mode 100644 drivers/mtd/nand/nand-uclass.c
>>>>
>>
>> [...]
>>
>>>> +
>>>> +DECLARE_GLOBAL_DATA_PTR;
>>>> +
>>>> +struct mtd_info *get_nand_dev_by_index(int idx)
>>>> +{
>>>> +       struct nand_chip *chip;
>>>> +       struct udevice *dev;
>>>> +       int ret;
>>>> +
>>>> +       ret = uclass_get_device(UCLASS_NAND, idx, &dev);
>>>> +       if (ret) {
>>>> +               debug("NAND device (%d) not found\n", idx);
>>>> +               return NULL;
>>>> +       }
>>>> +
>>>> +       chip = (struct nand_chip *)dev_get_priv(dev);
>>>> +
>>>> +       return nand_to_mtd(chip);
>>>
>>>
>>> Can you add some comments to the nand.h header file (perhaps within
>>> #ifdef CONFIG_DM_NAND) to explain that drivers must have struct
>>> nand_chip as the first part of their private data? Assuming I have
>>> this right...
>>>
>>
>> Will below work for you?
>>
>> diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
>> index d55807b..567f286 100644
>> --- a/include/linux/mtd/nand.h
>> +++ b/include/linux/mtd/nand.h
>> @@ -684,6 +684,16 @@ struct nand_buffers {
>>   *                     correctable).
>>   * @write_page:                [REPLACEABLE] High-level page write function
>>   */
>> +#ifdef CONFIG_DM_NAND
>> +/**
>> + * DM compatible nand drivers must have struct nand_chip as
>> + * the first part of their private data:
>> + * U_BOOT_DRIVER(...) = {
>> + *     ...
>> + *             .priv_auto_alloc_size = sizeof(struct nand_chip),
>> + * };
>> + */
>> +#endif
>
> Seems good
>
>>
>>  struct nand_chip {
>>         struct mtd_info mtd;
>>
>>
>>>> +}
>>>> +
>>>> +UCLASS_DRIVER(nand) = {
>>>> +       .id                             = UCLASS_NAND,
>>>> +       .name                           = "nand",
>>>> +       .flags                          = DM_UC_FLAG_SEQ_ALIAS,
>>>> +};
>>>
>>>
>>> Also can we have a sandbox NAND driver and some tests.
>>>
>>
>> Sry, for the stupid question, but what's "sandbox NAND driver"? Any ref?
>
> I mean you should create a NAND driver for sandbox. It should pretend
> to be a NAND chip and support operation as such. There are various
> sandbox drivers for other uclasses that do this.
>
> Do you have any operations in your uclass? See for example struct
> dm_mmc_ops. I added the MMC uclass without the operations, and it was
> painful to add them later.
>
>>
>> Also does it really required to be done as part of this series?
>
> Yes - any new uclass needs a test in test/dm. It can be a simple test, though.
>

ok. i understood your requests, but, unfortunately, I'm not sure
that i'll be able to satisfy them now :( - i'm pretty limited in time.

For now I'll resend preparation patches get_nand_dev_by_index() and will 
try to get back here as soon as i can.

-- 
regards,
-grygorii