From mboxrd@z Thu Jan  1 00:00:00 1970
From: Scott Wood <oss@buserror.net>
Date: Fri, 01 Apr 2016 18:25:39 -0500
Subject: [U-Boot] [PATCH 4/9] drivers: nand: implement a NAND uclass
In-Reply-To: <1459510190-26306-5-git-send-email-mugunthanvnm@ti.com>
References: <1459510190-26306-1-git-send-email-mugunthanvnm@ti.com>
 <1459510190-26306-5-git-send-email-mugunthanvnm@ti.com>
Message-ID: <1459553139.32510.49.camel@buserror.net>
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

On Fri, 2016-04-01 at 16:59 +0530, Mugunthan V N wrote:
> +static int nand_child_pre_probe(struct udevice *dev)
> +{
> +	nand_info_t *nand = dev_get_uclass_priv(dev);
> +	void *priv = dev_get_priv(dev);
> +
> +	/*
> +	 * Store nand device priv pointer in nand_info so that
> +	 * it can be used by nand command
> +	 */
> +	nand->priv = priv;

Wouldn't it make more sense to have a pointer to the device in the NAND
struct, and let the driver manage both privs as it chooses?

> +
> +	return 0;
> +}
> +
> +UCLASS_DRIVER(nand) = {
> +	.id				= UCLASS_NAND,
> +	.name				= "nand",
> +	.flags				= DM_UC_FLAG_SEQ_ALIAS,
> +	.post_probe		= nand_child_pre_probe,
> +	.per_device_auto_alloc_size	= sizeof(nand_info_t),
> +};

Post probe = pre probe?

> @@ -63,8 +63,10 @@ int nand_register(int devnum)
>  static void nand_init_chip(int i)
>  {
>  	struct mtd_info *mtd;
> +#ifndef CONFIG_DM_NAND
>  	struct nand_chip *nand = &nand_chip[i];
>  	ulong base_addr = base_address[i];
> +#endif
>  	int maxchips = CONFIG_SYS_NAND_MAX_CHIPS;
>  
>  	if (maxchips < 1)
> @@ -74,11 +76,13 @@ static void nand_init_chip(int i)
>  	if (!mtd)
>  		return;
>  
> +#ifndef CONFIG_DM_NAND
>  	mtd->priv = nand;
>  	nand->IO_ADDR_R = nand->IO_ADDR_W = (void  __iomem *)base_addr;
>  
>  	if (board_nand_init(nand))
>  		return;
> +#endif
>  
>  	if (nand_scan(mtd, maxchips))
>  		return;

Please do not use this code for DM.  Have drivers' probe functions call
nand_scan (split into ident and tail if necessary) as is done with
 CONFIG_SYS_NAND_SELF_INIT.

> diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h
> index 37c4176..6a88d39 100644
> --- a/include/dm/uclass-id.h
> +++ b/include/dm/uclass-id.h
> @@ -49,6 +49,7 @@ enum uclass_id {
>  	UCLASS_MMC,		/* SD / MMC card or chip */
>  	UCLASS_MOD_EXP,		/* RSA Mod Exp device */
>  	UCLASS_MTD,		/* Memory Technology Device (MTD) device
> */
> +	UCLASS_NAND,		/* NAND bus */

NAND is not a bus.

> +#ifndef CONFIG_DM_NAND
>  static inline nand_info_t *get_nand_dev_by_index(int dev)
>  {
>  	if (dev < 0 || dev >= CONFIG_SYS_MAX_NAND_DEVICE ||
> @@ -154,5 +156,8 @@ static inline nand_info_t *get_nand_dev_by_index(int
> dev)
>  
>  	return &nand_info[dev];
>  }
> +#else
> +nand_info_t *get_nand_dev_by_index(int idx);
> +#endif

Please use "if X/else", not "if !X/else".

-Scott