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

[Simon Glass]

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.

Regards,
Simon