linux-kernel - Re: [PATCH v3 24/36] mtd: st_spi_fsm: Supply the N25Qxxx chip specific configuration call-back

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20131211023323.GP27149@ld-irv-0074.broadcom.com>
Date:	Tue, 10 Dec 2013 18:33:23 -0800
From:	Brian Norris <computersforpeace@...il.com>
To:	Lee Jones <lee.jones@...aro.org>
Cc:	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
	dwmw2@...radead.org, angus.clark@...com, linus.walleij@...aro.org,
	linux-mtd@...ts.infradead.org
Subject: Re: [PATCH v3 24/36] mtd: st_spi_fsm: Supply the N25Qxxx chip
 specific configuration call-back

On Fri, Nov 29, 2013 at 12:19:13PM +0000, Lee Jones wrote:
> In the FSM driver we handle chip differences by providing the possibility
> of calling back into a chip specific initialisation routine. In this patch
> we provide one for the N25Qxxx series, which endeavours to setup things
> like the read, write and erase sequences, as they differ from the
> default. We also configure 32bit support and the amount of dummy cycles to
> use.
> 
> Signed-off-by: Lee Jones <lee.jones@...aro.org>
> ---
>  drivers/mtd/devices/st_spi_fsm.c | 77 ++++++++++++++++++++++++++++++++++++++++
>  drivers/mtd/devices/st_spi_fsm.h |  7 ++--
>  2 files changed, 82 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mtd/devices/st_spi_fsm.c b/drivers/mtd/devices/st_spi_fsm.c
> index 3a6b233..2369da1 100644
> --- a/drivers/mtd/devices/st_spi_fsm.c
> +++ b/drivers/mtd/devices/st_spi_fsm.c
> @@ -98,6 +98,8 @@ static struct seq_rw_config n25q_read4_configs[] = {
>  	{0x00,			0,			0, 0, 0, 0x00, 0, 0},
>  };
>  
> +static struct stfsm_seq stfsm_seq_read; 	/* Dynamically populated */
> +static struct stfsm_seq stfsm_seq_write;	/* Dynamically populated */

These should be fields in struct stfsm, no?

>  static struct stfsm_seq stfsm_seq_en_32bit_addr;/* Dynamically populated */
>  
>  static struct stfsm_seq stfsm_seq_read_jedec = {
> diff --git a/drivers/mtd/devices/st_spi_fsm.h b/drivers/mtd/devices/st_spi_fsm.h
> index 6699174..b5ce07d 100644
> --- a/drivers/mtd/devices/st_spi_fsm.h
> +++ b/drivers/mtd/devices/st_spi_fsm.h
> @@ -304,6 +304,8 @@ struct flash_info {
>  	int		(*config)(struct stfsm *);
>  };
>  
> +static int stfsm_n25q_config(struct stfsm *fsm);
> +

I really dislike putting this static declaration in the header. (And for
now, I don't think you need this header at all, since very little of
your work is shareable yet.) Your table *looks* like it could be shared,
but it won't even compile if this header is included in other files,
since they don't have the definitions of your static functions.

Anyway, I think your table should be reworked if it's going to be useful
to others.

>  static struct flash_info flash_types[] = {
>  	/*
>  	 * ST Microelectronics/Numonyx --
> @@ -347,9 +349,10 @@ static struct flash_info flash_types[] = {
>  		   FLASH_FLAG_WRITE_1_2_2	| \
>  		   FLASH_FLAG_WRITE_1_1_4	| \
>  		   FLASH_FLAG_WRITE_1_4_4)
> -	{ "n25q128", 0x20ba18, 0, 64 * 1024,  256, N25Q_FLAG, 108, NULL },
> +	{ "n25q128", 0x20ba18, 0, 64 * 1024,  256, N25Q_FLAG, 108,
> +	  stfsm_n25q_config },
>  	{ "n25q256", 0x20ba19, 0, 64 * 1024,  512,
> -	  N25Q_FLAG | FLASH_FLAG_32BIT_ADDR, 108, NULL },
> +	  N25Q_FLAG | FLASH_FLAG_32BIT_ADDR, 108, stfsm_n25q_config },
>  
>  	/*
>  	 * Spansion S25FLxxxP

Brian
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/