Re: [U-Boot] [PATCH 1/2 V2] SF: Add driver for Gigabyte device GD25LQ and GD25Q64B

Hi,

I think this reviewed already, but have a very few comments.

On Wed, Jan 23, 2013 at 12:00 PM, Rajeshwari Shinde rajeshwari.s@samsung.com wrote:

This patch adds driver for the gigabyte devices GD25LQ and GD25Q64B required for Snow Board.

Signed-off-by: Rajeshwari Shinde rajeshwari.s@samsung.com

Changes in V2: - Added U-Boot copyright header to gigadevice.c - Removed unnecessary blank lines. drivers/mtd/spi/Makefile | 1 + drivers/mtd/spi/gigadevice.c | 81 ++++++++++++++++++++++++++++++++++ drivers/mtd/spi/spi_flash.c | 3 + drivers/mtd/spi/spi_flash_internal.h | 1 + 4 files changed, 86 insertions(+), 0 deletions(-) create mode 100644 drivers/mtd/spi/gigadevice.c

diff --git a/drivers/mtd/spi/Makefile b/drivers/mtd/spi/Makefile index 90f8392..ecbb210 100644 --- a/drivers/mtd/spi/Makefile +++ b/drivers/mtd/spi/Makefile @@ -32,6 +32,7 @@ endif COBJS-$(CONFIG_SPI_FLASH) += spi_flash.o COBJS-$(CONFIG_SPI_FLASH_ATMEL) += atmel.o COBJS-$(CONFIG_SPI_FLASH_EON) += eon.o +COBJS-$(CONFIG_SPI_FLASH_GIGADEVICE) += gigadevice.o COBJS-$(CONFIG_SPI_FLASH_MACRONIX) += macronix.o COBJS-$(CONFIG_SPI_FLASH_SPANSION) += spansion.o COBJS-$(CONFIG_SPI_FLASH_SST) += sst.o diff --git a/drivers/mtd/spi/gigadevice.c b/drivers/mtd/spi/gigadevice.c new file mode 100644 index 0000000..b5e1ebe --- /dev/null +++ b/drivers/mtd/spi/gigadevice.c @@ -0,0 +1,81 @@ +/*

• • Gigadevice SPI flash driver
• • Copyright 2013, Samsung Electronics Co., Ltd.
• • Author: Banajit Goswami banajit.g@samsung.com
• • See file CREDITS for list of people who contributed to this
• • project.
• • This 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.
• • This 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, write to the Free Software
• • Foundation, Inc., 59 Temple Place, Suite 330, Boston,
• • MA 02111-1307 USA
• */

+#include <common.h> +#include <malloc.h> +#include <spi_flash.h>

+#include "spi_flash_internal.h"

+struct gigadevice_spi_flash_params {

•   uint16_t        id;
  

•   uint16_t        nr_blocks;
  

I think it's better to use u16 instead of uint16_t, uin16_t will get back to arch include from include/linux which does u16 for directly for first time.

•   const char      *name;
  

+};

+static const struct gigadevice_spi_flash_params gigadevice_spi_flash_table[] = {

•   {
  

•           .id                     = 0x6016,
  

•           .nr_blocks              = 64,
  

•           .name                   = "GD25LQ",
  

•   },
  

•   {
  

•           .id                     = 0x4017,
  

•           .nr_blocks              = 128,
  

•           .name                   = "GD25Q64B",
  

•   },
  

Better to use clean code shape like.. { .id = 0x60, .nr_blocks = 64, .name = "GD25LQ", }

+};

+struct spi_flash *spi_flash_probe_gigadevice(struct spi_slave *spi, u8 *idcode) +{

•   const struct gigadevice_spi_flash_params *params;
  

•   struct spi_flash *flash;
  

•   unsigned int i;
  

•   for (i = 0; i < ARRAY_SIZE(gigadevice_spi_flash_table); i++) {
  

•           params = &gigadevice_spi_flash_table[i];
  

•           if (params->id == ((idcode[1] << 8) | idcode[2]))
  

•                   break;
  

•   }
  

•   if (i == ARRAY_SIZE(gigadevice_spi_flash_table)) {
  

•           debug("SF: Unsupported Gigadevice ID %02x%02x\n",
  

•                           idcode[1], idcode[2]);
  

•           return NULL;
  

•   }
  

•   flash = spi_flash_alloc_base(spi, params->name);
  

•   if (!flash) {
  

•           debug("SF: Failed to allocate memory\n");
  

•           return NULL;
  

•   }
  

better to add a space here

•   /* page_size */
  

•   flash->page_size = 256;
  

•   /* sector_size = page_size * pages_per_sector */
  

•   flash->sector_size = flash->page_size * 16;
  

•   /* size = sector_size * sector_per_block * number of blocks */
  

•   flash->size = flash->sector_size * 16 * params->nr_blocks;
  

comments on above size calculations are good, but not that much important i guess.

And also please provide a stand' link for flash part data sheet on commit message, if possible. I thought it's a good to refers don't no this sounds valid..

Thanks, Jagan.