[U-Boot] [PATCH 2/4] Armada100: SPI Flash support for Marvell gplugD

[Wolfgang Denk]

Dear Ajay Bhargav,

In message <1310106168-17166-2-git-send-email-ajay.bhargav at einfochips.com> you wrote:
> SPI Flash support added for Marvell GuruPlug-Display. Environment
> variables are now stored in flash.
...
> --- a/arch/arm/include/asm/arch-armada100/gpio.h
> +++ b/arch/arm/include/asm/arch-armada100/gpio.h
> @@ -25,7 +25,7 @@
>  #define __PXA_GPIO_H
>  
>  
> -#define GPIO_REGS_VIRT (0xD4019000)
> +#define GPIO_REGS_VIRT	(0xD4019000)

No parens areound plain constants values, please.

Please fix globally.

...
> --- /dev/null
> +++ b/arch/arm/include/asm/arch-armada100/regs-ssp.h
> @@ -0,0 +1,165 @@
> +/**************************************************************************
> + *
> + * Copyright (c) 2009, 2010 Marvell International Ltd.

Incorrect multiline comment style.  Please fix globally.

> diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
> index a9b1ca4..9626772 100644
> --- a/drivers/spi/Makefile
> +++ b/drivers/spi/Makefile
> @@ -40,6 +40,7 @@ COBJS-$(CONFIG_OMAP3_SPI) += omap3_spi.o
>  COBJS-$(CONFIG_SOFT_SPI) += soft_spi.o
>  COBJS-$(CONFIG_SH_SPI) += sh_spi.o
>  COBJS-$(CONFIG_FSL_ESPI) += fsl_espi.o
> +COBJS-$(CONFIG_PXA3XX_SPI) += pxa3xx_spi.o

Please keep / make list sorted.

...
> diff --git a/drivers/spi/pxa3xx_spi.c b/drivers/spi/pxa3xx_spi.c
> new file mode 100644
> index 0000000..8d05f90
> --- /dev/null
> +++ b/drivers/spi/pxa3xx_spi.c
> @@ -0,0 +1,256 @@
> +/**************************************************************************
> + *
> + * Copyright (c) 2009, 2010 Marvell International Ltd.
> + *
> + * This file is part of GNU program.
> + *
> + * GNU program is free software: you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License as published by the
> + * Free Software Foundation, either version 2 of the License, or (at your
> + * option) any later version.
> + *
> + * GNU program is distributed in the hope that it will be useful, but
> + * WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU General
> + * Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License along
> + * with this program.
> + *
> + * If not, see http://www.gnu.org/licenses/old-licenses/gpl-2.0.html
> + *
> + *************************************************************************/
> +
> +/*
> + * Driver for simulating ssp as spi device for pxa
> + *
> + * Copyright (c) 2009 Marvell Inc.
> + * Lei Wen <leiwen at marvell.com>
> + *
> + * Licensed under the GPL-2 or later.
> + */

Please remove the redunant parts of the Copyright/License headers.
Please fix globally.


> +#define DEFINE_SSP_REG(reg, off) \
> +	static inline u32 read_##reg(void) \
> +{ return __raw_readl(CONFIG_SYS_SSP_BASE + (off)); } \

We don't allow accesses through base address plus offset; please use
proper C structs instead.

> +DEFINE_SSP_REG(SSCR0, 0x00)
> +DEFINE_SSP_REG(SSCR1, 0x04)
> +DEFINE_SSP_REG(SSSR, 0x08)
> +DEFINE_SSP_REG(SSITR, 0x0c)
> +DEFINE_SSP_REG(SSDR, 0x10)
> +DEFINE_SSP_REG(SSTO, 0x28)
> +DEFINE_SSP_REG(SSPSP, 0x2c)

NAK.  Please use C struct.

> +static void null_writer(struct pxa_spi_slave *pss)
> +{
> +	while (!(read_SSSR() & SSSR_TNF))

We don't allow camel-case identifiers.  Please fix globally.

> +		continue;

Please use a plain ';' instead of this 'continue'.  Please fix
globally.


> @@ -96,10 +121,5 @@
>  
>  #define CONFIG_SYS_NS16550_COM1 ARMD1_UART3_BASE
>  
> -/*
> - * Environment variables configurations
> - */
> -#define CONFIG_ENV_IS_NOWHERE	1	/* if env in SDRAM */
> -#define CONFIG_ENV_SIZE	0x20000		/* 64k */
> -
>  #endif	/* __CONFIG_GPLUGD_H */
> +
> -- 

Please do not add trailing empty lines.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
The main thing is the play itself. I swear that greed for  money  has
nothing  to  do with it, although heaven knows I am sorely in need of
money.                                           - Feodor Dostoyevsky