linux-kernel - Re: [PATCH v3 09/36] mtd: st_spi_fsm: Provide device look-up table

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <20131210220338.GF27149@ld-irv-0074.broadcom.com>
Date:	Tue, 10 Dec 2013 14:03:38 -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 09/36] mtd: st_spi_fsm: Provide device look-up table

On Fri, Nov 29, 2013 at 12:18:58PM +0000, Lee Jones wrote:
> --- a/drivers/mtd/devices/st_spi_fsm.h
> +++ b/drivers/mtd/devices/st_spi_fsm.h
> @@ -253,4 +253,141 @@ struct stfsm_seq {
>  } __attribute__((__packed__, aligned(4)));
>  #define STFSM_SEQ_SIZE sizeof(struct stfsm_seq)
>  
> +/* SPI Flash Device Table */
> +struct flash_info {
> +	char		*name;
> +	/*
> +	 * JEDEC id zero means "no ID" (most older chips); otherwise it has
> +	 * a high byte of zero plus three data bytes: the manufacturer id,
> +	 * then a two byte device id.
> +	 */
> +	u32		jedec_id;
> +	u16             ext_id;

Will 5 bytes of ID be enough? I think we're running into a need for 6
bytes of ID in m25p80.c right about now. Might make sense to start with
the right number of bytes.

> +	/*
> +	 * The size listed here is what works with FLASH_CMD_SE, which isn't
> +	 * necessarily called a "sector" by the vendor.
> +	 */
> +	unsigned	sector_size;
> +	u16		n_sectors;
> +	u32		flags;
> +	/*
> +	 * Note, where FAST_READ is supported, freq_max specifies the
> +	 * FAST_READ frequency, not the READ frequency.
> +	 */
> +	u32		max_freq;
> +	int		(*config)(struct stfsm *);

Do you *really* need a configuration callback? Can the callback be
represented as simply a flag for the special configuration behavior that
is needed, then your driver calls the appropriate "callback" when it
sees the flag?

BTW, your later patches have to introduce static declarations in this
header, which seems very ugly to me; you are entwining data with your
driver's implementation.

So this callback field can only be used by your driver, and it is one of
the main reasons that your table structure can't be used in other
drivers.

> +};
> +
> +static struct flash_info flash_types[] = {
> +	/*
> +	 * ST Microelectronics/Numonyx --
> +	 * (newer production versions may have feature updates
> +	 * (eg faster operating frequency)
> +	 */
> +#define M25P_FLAG (FLASH_FLAG_READ_WRITE | FLASH_FLAG_READ_FAST)

Please don't define macros in the middle of your table like this. (You
have several of these.)

> +	{ "m25p40",  0x202013, 0,  64 * 1024,   8, M25P_FLAG, 25, NULL },
> +	{ "m25p80",  0x202014, 0,  64 * 1024,  16, M25P_FLAG, 25, NULL },
> +	{ "m25p16",  0x202015, 0,  64 * 1024,  32, M25P_FLAG, 25, NULL },
> +	{ "m25p32",  0x202016, 0,  64 * 1024,  64, M25P_FLAG, 50, NULL },
> +	{ "m25p64",  0x202017, 0,  64 * 1024, 128, M25P_FLAG, 50, NULL },
> +	{ "m25p128", 0x202018, 0, 256 * 1024,  64, M25P_FLAG, 50, NULL },

[snip]

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/