From mboxrd@z Thu Jan  1 00:00:00 1970
From: Simon Glass <sjg@chromium.org>
Date: Thu, 21 Apr 2016 08:11:55 -0600
Subject: [U-Boot] [PATCH 4/9] drivers: nand: implement a NAND uclass
In-Reply-To: <57186B5C.1050506@ti.com>
References: <1459510190-26306-1-git-send-email-mugunthanvnm@ti.com>
 <1459510190-26306-5-git-send-email-mugunthanvnm@ti.com>
 <CAPnjgZ05z7LdsXwFpNbvqCrsGPSwbxPehJmGzUoS6ty6kQUYoA@mail.gmail.com>
 <57186B5C.1050506@ti.com>
Message-ID: <CAPnjgZ1jG+g0t+LTH=mzhtoTofBTfjO_GVik6GPWa7xjQ7z=fg@mail.gmail.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

Hi Mugunthan,

On 20 April 2016 at 23:55, Mugunthan V N <mugunthanvnm@ti.com> wrote:
> On Wednesday 20 April 2016 08:09 PM, Simon Glass wrote:
>> Hi Mugunthan,
>>
>> On 1 April 2016 at 05:29, Mugunthan V N <mugunthanvnm@ti.com> wrote:
>>> Implement a NAND uclass so that the NAND devices can be
>>> accessed via the DM framework.
>>>
>>> Signed-off-by: Mugunthan V N <mugunthanvnm@ti.com>
>>> ---
>>>  drivers/mtd/nand/Kconfig       | 10 +++++++
>>>  drivers/mtd/nand/Makefile      |  2 ++
>>>  drivers/mtd/nand/nand-uclass.c | 62 ++++++++++++++++++++++++++++++++++++++++++
>>>  drivers/mtd/nand/nand.c        |  6 +++-
>>>  include/dm/uclass-id.h         |  1 +
>>>  include/nand.h                 |  5 ++++
>>>  6 files changed, 85 insertions(+), 1 deletion(-)
>>>  create mode 100644 drivers/mtd/nand/nand-uclass.c
>>>
>>> diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig
>>> index 7787505..53b57b8 100644
>>> --- a/drivers/mtd/nand/Kconfig
>>> +++ b/drivers/mtd/nand/Kconfig
>>> @@ -8,6 +8,16 @@ menuconfig NAND
>>>
>>>  if NAND
>>>
>>> +config DM_NAND
>>> +       bool "Enable driver model for NAND"
>>> +       depends on NAND && DM
>>> +       help
>>> +         Enable driver model for NAND. The NAND interface is then
>>> +         implemented by the NAND uclass. Multiple NAND devices can
>>> +         be attached and used. The 'nand' command works as normal.
>>> +
>>> +         If the NAND drivers doesn't support DM, say N.
>>> +
>>>  config SYS_NAND_SELF_INIT
>>>         bool
>>>         help
>>> diff --git a/drivers/mtd/nand/Makefile b/drivers/mtd/nand/Makefile
>>> index 6fb3718..7745705 100644
>>> --- a/drivers/mtd/nand/Makefile
>>> +++ b/drivers/mtd/nand/Makefile
>>> @@ -39,6 +39,8 @@ endif # not spl
>>>
>>>  ifdef NORMAL_DRIVERS
>>>
>>> +obj-$(CONFIG_DM_NAND) += nand-uclass.o
>>> +
>>>  obj-$(CONFIG_NAND_ECC_BCH) += nand_bch.o
>>>
>>>  obj-$(CONFIG_NAND_ATMEL) += atmel_nand.o
>>> diff --git a/drivers/mtd/nand/nand-uclass.c b/drivers/mtd/nand/nand-uclass.c
>>> new file mode 100644
>>> index 0000000..d68d357
>>> --- /dev/null
>>> +++ b/drivers/mtd/nand/nand-uclass.c
>>> @@ -0,0 +1,62 @@
>>> +/*
>>> + * NAND uclass driver for NAND bus.
>>> + *
>>> + * (C) Copyright 2012-2016
>>> + *     Texas Instruments Incorporated, <www.ti.com>
>>> + *
>>> + * SPDX-License-Identifier:     GPL-2.0+
>>> + */
>>> +
>>> +#include <common.h>
>>> +#include <dm.h>
>>> +#include <errno.h>
>>> +#include <nand.h>
>>> +
>>> +DECLARE_GLOBAL_DATA_PTR;
>>> +
>>> +#ifdef CONFIG_DM_NAND
>>> +
>>> +nand_info_t *get_nand_dev_by_index(int idx)
>>> +{
>>> +       nand_info_t *nand;
>>> +       struct udevice *dev;
>>> +       int ret;
>>> +
>>> +       ret = uclass_get_device(UCLASS_NAND, idx, &dev);
>>> +       if (ret) {
>>> +               error("NAND device (%d) not found\n", idx);
>>
>> This should return -ENODEV. Also please avoid printf() in drivers. The
>> caller can report the error.
>>
>
> Return type is pointer so returned NULL and NULL denotes no dev.
> Will change this error to debug in v2.

OK, I see that this is the existing API.

>
>>> +               return NULL;
>>> +       }
>>> +
>>> +       nand = (nand_info_t *)dev_get_uclass_priv(dev);
>>> +       if (!nand) {
>>> +               error("Nand device not ready\n");
>>> +               return NULL;
>>> +       }
>>
>> But if the device has been probed ((with uclass_get_device()) then
>> this cannot be NULL.
>
> This check is just a failsafe. ideally it should not fail here.

I cannot get to probe the device without this data, since this would
be a bug in DM. So the check has no value.

[snip]

Regards,
Simon