linux-kernel - Re: [LINUX RFC 1/2] mtd: spi-nor: add dual parallel mode support

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: <20150803160829.GX20873@sirena.org.uk>
Date:	Mon, 3 Aug 2015 17:08:29 +0100
From:	Mark Brown <broonie@...nel.org>
To:	Ranjit Waghmode <ranjit.waghmode@...inx.com>
Cc:	dwmw2@...radead.org, computersforpeace@...il.com,
	michal.simek@...inx.com, soren.brinkmann@...inx.com,
	zajec5@...il.com, ben@...adent.org.uk, marex@...x.de,
	b32955@...escale.com, knut.wohlrab@...bosch.com,
	juhosg@...nwrt.org, beanhuo@...ron.com,
	linux-mtd@...ts.infradead.org, linux-kernel@...r.kernel.org,
	linux-spi@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	harinik@...inx.com, punnaia@...inx.com, ranjitw@...inx.com,
	ran27jit@...il.com
Subject: Re: [LINUX RFC 1/2] mtd: spi-nor: add dual parallel mode support

On Mon, Aug 03, 2015 at 02:35:06PM +0530, Ranjit Waghmode wrote:

>  drivers/mtd/devices/m25p80.c  |  1 +
>  drivers/mtd/spi-nor/spi-nor.c | 92 ++++++++++++++++++++++++++++++++++---------
>  include/linux/mtd/spi-nor.h   |  3 ++
>  include/linux/spi/spi.h       |  2 +
>  4 files changed, 79 insertions(+), 19 deletions(-)

You need to at least split this into two patches, one adding a new SPI
interface and another using it in MTD.  Probably the MTD core and driver
changes need splitting too.  Please see SubmittingPatches for discussion
of splitting things.

> diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
> index d673072..8dec349 100644
> --- a/include/linux/spi/spi.h
> +++ b/include/linux/spi/spi.h
> @@ -355,6 +355,8 @@ struct spi_master {
>  #define SPI_MASTER_NO_TX	BIT(2)		/* can't do buffer write */
>  #define SPI_MASTER_MUST_RX      BIT(3)		/* requires rx */
>  #define SPI_MASTER_MUST_TX      BIT(4)		/* requires tx */
> +#define SPI_MASTER_DATA_STRIPE		BIT(7)		/* support data stripe */
> +#define SPI_MASTER_BOTH_CS		BIT(8)		/* enable both chips */

This is really not adequate description for a new API, I can't tell what
"data stripe" is supposed to mean at all and I've got at best a vague
idea what "both chips" really means.  This means other developers won't
be able to tell how to use or implement these flags either, and it means
I can't really review this.  You need to provide more information here,
both in the code and in the commit message.

I'd also expect some handling in the core for these, for example error
handling if they can't be supported.

Download attachment "signature.asc" of type "application/pgp-signature" (474 bytes)