From mboxrd@z Thu Jan  1 00:00:00 1970
From: Chin Liang See <clsee@altera.com>
Date: Thu, 26 Nov 2015 08:54:05 +0800
Subject: [U-Boot] [PATCH] mmc: socfpga_dw_mmc: Move drvsel and smplsel
	to dts
In-Reply-To: <201511260150.25454.marex@denx.de>
References: <1448498472-2573-1-git-send-email-clsee@altera.com>
 <201511260150.25454.marex@denx.de>
Message-ID: <1448499245.2097.1.camel@altera.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 Marek,

On Thu, 2015-11-26 at 01:50 +0100, Marek Vasut wrote:
> On Thursday, November 26, 2015 at 01:41:12 AM, clsee wrote:
> > From: Chin Liang See <clsee@altera.com>
> > 
> > socfpga_dw_mmc driver will obtain the drvsel and
> > smplsel value from device tree instead of definition
> > in config header file.
> > 
> > Signed-off-by: Chin Liang See <clsee@altera.com>
> > Cc: Dinh Nguyen <dinguyen@opensource.altera.com>
> > Cc: Dinh Nguyen <dinh.linux@gmail.com>
> > Cc: Pavel Machek <pavel@denx.de>
> > Cc: Marek Vasut <marex@denx.de>
> > Cc: Stefan Roese <sr@denx.de>
> > Cc: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
> > Cc: Simon Glass <sjg@chromium.org>
> > Cc: Jaehoon Chung <jh80.chung@samsung.com>
> > ---
> >  arch/arm/dts/socfpga_cyclone5.dtsi |  2 ++
> >  drivers/mmc/socfpga_dw_mmc.c       | 24 ++++++++++++++++++++++--
> >  include/configs/socfpga_common.h   |  2 --
> >  3 files changed, 24 insertions(+), 4 deletions(-)
> 
> Hi!
> 
> [...]
> 
> > @@ -78,11 +87,19 @@ static int socfpga_dwmci_of_probe(const void
> > *blob, int
> > node, const int idx) return -EINVAL;
> >  	}
> > 
> > +	drvsel = fdtdec_get_uint(blob, node, "drvsel", 0);
> 
> The default value here should be 3, otherwise this won't preserve the
> original
> behavior of the driver in case the nodes in DT are missing.

Nice thinking, will update this.

> 
> > +	smplsel = fdtdec_get_uint(blob, node, "smplsel", 0);
> > +
> >  	/* Allocate the host */
> >  	host = calloc(1, sizeof(*host));
> >  	if (!host)
> >  		return -ENOMEM;
> > 
> > +	/* Allocate the priv */
> > +	priv = calloc(1, sizeof(*priv));
> > +	if (!priv)
> > +		return -ENOMEM;
> 
> If this call fails, you're leaking memory, since you calloc() some
> stuff before.
> 

Oops, yup, this need to be fixed

> > +
> >  	host->name = "SOCFPGA DWMMC";
> >  	host->ioaddr = (void *)reg_base;
> >  	host->buswidth = bus_width;
> > @@ -92,6 +109,9 @@ static int socfpga_dwmci_of_probe(const void
> > *blob, int
> > node, const int idx) host->bus_hz = clk;
> >  	host->fifoth_val = MSIZE(0x2) |
> >  		RX_WMARK(fifo_depth / 2 - 1) | TX_WMARK(fifo_depth
> > / 2);
> > +	priv->drvsel = drvsel;
> > +	priv->smplsel = smplsel;
> > +	host->priv = priv;
> 
> You can move the fdtdec_get_uint() calls here directly, no need to
> introduce
> ad-hoc variable and then just assign it into the private data.

Let me enhance this too

Thanks
Chin Liang

> 
> >  	return add_dwmci(host, host->bus_hz, 400000);
> >  }
> > diff --git a/include/configs/socfpga_common.h
> > b/include/configs/socfpga_common.h index f6808b5..b661cc2 100644
> > --- a/include/configs/socfpga_common.h
> > +++ b/include/configs/socfpga_common.h
> > @@ -153,8 +153,6 @@
> >  #define CONFIG_DWMMC
> >  #define CONFIG_SOCFPGA_DWMMC
> >  #define CONFIG_SOCFPGA_DWMMC_FIFO_DEPTH	1024
> > -#define CONFIG_SOCFPGA_DWMMC_DRVSEL	3
> > -#define CONFIG_SOCFPGA_DWMMC_SMPSEL	0
> >  /* FIXME */
> >  /* using smaller max blk cnt to avoid flooding the limited stack
> > we have
> > */ #define CONFIG_SYS_MMC_MAX_BLK_COUNT	256	/* FIXME 
> > -- SPL only? */