diff mbox

[v3,2/7] mtd: spi-nor: add the basic data structures

Message ID 1387184330-14448-3-git-send-email-b32955@freescale.com (mailing list archive)
State New, archived
Headers show

Commit Message

Huang Shijie Dec. 16, 2013, 8:58 a.m. UTC
The spi_nor{} is cloned from the m25p{}.
The spi_nor{} can be used by both the m25p80 and spi-nor controller.

We also add the spi_nor_xfer_cfg{} which can be used by the two
fundamental primitives: read_xfer/write_xfer.

 1) the hooks for spi_nor{}:
    @prepare/unpreare: used to do some work before or after the
             read/write/erase/lock/unlock.
    @read_xfer/write_xfer: We can use these two hooks to code all
             the following hooks if the driver tries to implement them
             by itself.
    @read_reg: used to read the registers, such as read status register,
             read configure register.
    @write_reg: used to write the registers, such as write enable,
             erase sector.
    @read_id: read out the ID info.
    @wait_till_ready: wait till the NOR becomes ready.
    @read: read out the data from the NOR.
    @write: write data to the NOR.
    @erase: erase a sector of the NOR.

 2) Add a new field sst_write_second for the SST NOR write.

Signed-off-by: Huang Shijie <b32955@freescale.com>
---
 include/linux/mtd/spi-nor.h |  108 +++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 108 insertions(+), 0 deletions(-)

Comments

Marek Vasut Dec. 17, 2013, 1:05 p.m. UTC | #1
On Monday, December 16, 2013 at 09:58:45 AM, Huang Shijie wrote:
> The spi_nor{} is cloned from the m25p{}.
> The spi_nor{} can be used by both the m25p80 and spi-nor controller.
> 
> We also add the spi_nor_xfer_cfg{} which can be used by the two
> fundamental primitives: read_xfer/write_xfer.
> 
>  1) the hooks for spi_nor{}:
>     @prepare/unpreare: used to do some work before or after the
>              read/write/erase/lock/unlock.
>     @read_xfer/write_xfer: We can use these two hooks to code all
>              the following hooks if the driver tries to implement them
>              by itself.
>     @read_reg: used to read the registers, such as read status register,
>              read configure register.
>     @write_reg: used to write the registers, such as write enable,
>              erase sector.
>     @read_id: read out the ID info.
>     @wait_till_ready: wait till the NOR becomes ready.
>     @read: read out the data from the NOR.
>     @write: write data to the NOR.
>     @erase: erase a sector of the NOR.
> 
>  2) Add a new field sst_write_second for the SST NOR write.
> 
> Signed-off-by: Huang Shijie <b32955@freescale.com>
> ---
>  include/linux/mtd/spi-nor.h |  108
> +++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 108
> insertions(+), 0 deletions(-)
> 
> diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
> index ab2ea1e..83ca63d 100644
> --- a/include/linux/mtd/spi-nor.h
> +++ b/include/linux/mtd/spi-nor.h
> @@ -50,4 +50,112 @@
>  /* Configuration Register bits. */
>  #define CR_QUAD_EN_SPAN		0x2     /* Spansion Quad I/O */
> 
> +enum read_mode {
> +	SPI_NOR_NORMAL = 0,
> +	SPI_NOR_FAST,
> +	SPI_NOR_QUAD,
> +};
> +
> +/*
> + * struct spi_nor_xfer_cfg - Structure for defining a Serial Flash
> transfer + * @wren:		command for "Write Enable", or 0x00 for not 
required
> + * @cmd:		command for operation
> + * @cmd_pins:		number of pins to send @cmd (1, 2, 4)
> + * @addr:		address for operation
> + * @addr_pins:		number of pins to send @addr (1, 2, 4)
> + * @addr_width: 	number of address bytes (3,4, or 0 for address not
> required) + * @mode:		mode data
> + * @mode_pins:		number of pins to send @mode (1, 2, 4)
> + * @mode_cycles:	number of mode cycles (0 for mode not required)
> + * @dummy_cycles:	number of dummy cycles (0 for dummy not required)
> + */
> +struct spi_nor_xfer_cfg {
> +	u8		wren;
> +	u8		cmd;
> +	u8		cmd_pins;
> +	u32		addr;
> +	u8		addr_pins;
> +	u8		addr_width;
> +	u8		mode;
> +	u8		mode_pins;
> +	u8		mode_cycles;
> +	u8		dummy_cycles;
> +};
> +
> +#define	SPI_NOR_MAX_CMD_SIZE	8
> +enum spi_nor_ops {
> +	SPI_NOR_OPS_READ = 0,
> +	SPI_NOR_OPS_WRITE,
> +	SPI_NOR_OPS_ERASE,
> +	SPI_NOR_OPS_LOCK,
> +	SPI_NOR_OPS_UNLOCK,
> +};
> +
> +struct spi_nor {
> +	struct mtd_info		*mtd;
> +	struct mutex		lock;
> +
> +	/* pointer to a spi device */
> +	struct device		*dev;
> +	u32			page_size;
> +	u8			addr_width;
> +	u8			erase_opcode;
> +	u8			read_opcode;
> +	u8			read_dummy;
> +	u8			program_opcode;
> +	enum read_mode		flash_read;
> +	bool			sst_write_second;
> +	struct spi_nor_xfer_cfg	cfg;

You do want to split the function pointers below and the device configuration 
above into separate structures.

> +	/* for write_reg */
> +	u8			cmd_buf[SPI_NOR_MAX_CMD_SIZE];
> +
> +	/*
> +	 * Do some work before or after we run these operations:
> +	 *   read/write/erese/lock/unlock

Proper kernel-doc style comments for this structure would be nice.

> +	 */
> +	int (*prepare)(struct spi_nor *nor, enum spi_nor_ops ops);
> +	void (*unprepare)(struct spi_nor *nor, enum spi_nor_ops ops);
> +
> +	/*
> +	 * The two fundamental primitives, you can use them to implement
> +	 * all the other hooks, except the prepare/unprepare.
> +	 */
> +	int (*read_xfer)(struct spi_nor *nor, struct spi_nor_xfer_cfg *cfg,
> +			 u8 *buf, size_t len);
> +	int (*write_xfer)(struct spi_nor *nor, struct spi_nor_xfer_cfg *cfg,
> +			  u8 *buf, size_t len);
> +
> +	/*
> +	 * The two hooks are used to read/write SPI NOR register, such as
> +	 * read status register, write status register.
> +         */

The format of the comment is messed up, please fix globally.

> +	int (*read_reg)(struct spi_nor *nor, u8 opcode, u8 *buf, int len);
> +	int (*write_reg)(struct spi_nor *nor, u8 opcode, u8 *buf, int len,
> +			int write_enable);
> +
> +	/*
> +	 * The hook for reading out the ID, the spi-nor controller drivers
> +         * can fill it with its own implementation if the default
> +	 * could not meet its requirement.
> +	 */
> +	const struct spi_device_id *(*read_id)(struct spi_nor *nor);
> +
> +	/*
> +	 * The hook for "Wait till ready", some spi-nor controller drivers
> +	 * may fill it with its own implementation.
> +	 */
> +	int (*wait_till_ready)(struct spi_nor *nor);
> +
> +	/* write */

write ... what ? I get it, but a proper documentation for new API would be 
_nice_ . Besides, I do not understand the parameters at all. Neither do I 
understand how to implement driver based on this API.

> +	void (*write)(struct spi_nor *nor, loff_t to,
> +			size_t len, size_t *retlen, const u_char *buf);
> +	/* read */
> +	int (*read)(struct spi_nor *nor, loff_t from,
> +			size_t len, size_t *retlen, u_char *buf);
> +	/* erase a sector(4K/64K, etc..) */

How do you select the erase size here (this is not documented, I dont understand 
it at all)?

> +	int (*erase)(struct spi_nor *nor, loff_t offs);
> +
> +	void *priv;
> +};
>  #endif
Huang Shijie Dec. 17, 2013, 1:49 p.m. UTC | #2
On Tue, Dec 17, 2013 at 02:05:55PM +0100, Marek Vasut wrote:
> On Monday, December 16, 2013 at 09:58:45 AM, Huang Shijie wrote:
> > +struct spi_nor { > > +	struct mtd_info		*mtd;
> > +	struct mutex		lock;
> > +
> > +	/* pointer to a spi device */
> > +	struct device		*dev;
> > +	u32			page_size;
> > +	u8			addr_width;
> > +	u8			erase_opcode;
> > +	u8			read_opcode;
> > +	u8			read_dummy;
> > +	u8			program_opcode;
> > +	enum read_mode		flash_read;
> > +	bool			sst_write_second;
> > +	struct spi_nor_xfer_cfg	cfg;
> 
> You do want to split the function pointers below and the device configuration 
> above into separate structures.

sorry, i prefer to keep them in one data structrue, just like the
nand_chip{} does.

> 
> > +	/* for write_reg */
> > +	u8			cmd_buf[SPI_NOR_MAX_CMD_SIZE];
> > +
> > +	/*
> > +	 * Do some work before or after we run these operations:
> > +	 *   read/write/erese/lock/unlock
> 
> Proper kernel-doc style comments for this structure would be nice.

Do you mean the style used by nand_chip{}?

thanks
Huang Shijie
Marek Vasut Dec. 17, 2013, 3:16 p.m. UTC | #3
On Tuesday, December 17, 2013 at 02:49:25 PM, Huang Shijie wrote:
> On Tue, Dec 17, 2013 at 02:05:55PM +0100, Marek Vasut wrote:
> > On Monday, December 16, 2013 at 09:58:45 AM, Huang Shijie wrote:
> > > +struct spi_nor { > > +	struct mtd_info		*mtd;
> > > +	struct mutex		lock;
> > > +
> > > +	/* pointer to a spi device */
> > > +	struct device		*dev;
> > > +	u32			page_size;
> > > +	u8			addr_width;
> > > +	u8			erase_opcode;
> > > +	u8			read_opcode;
> > > +	u8			read_dummy;
> > > +	u8			program_opcode;
> > > +	enum read_mode		flash_read;
> > > +	bool			sst_write_second;
> > > +	struct spi_nor_xfer_cfg	cfg;
> > 
> > You do want to split the function pointers below and the device
> > configuration above into separate structures.
> 
> sorry, i prefer to keep them in one data structrue, just like the
> nand_chip{} does.

Can you explain why?

> > > +	/* for write_reg */
> > > +	u8			cmd_buf[SPI_NOR_MAX_CMD_SIZE];
> > > +
> > > +	/*
> > > +	 * Do some work before or after we run these operations:
> > > +	 *   read/write/erese/lock/unlock
> > 
> > Proper kernel-doc style comments for this structure would be nice.
> 
> Do you mean the style used by nand_chip{}?

Yes. See Documentation/kernel-doc-nano-HOWTO.txt

Best regards,
Marek Vasut
Huang Shijie Dec. 17, 2013, 4 p.m. UTC | #4
On Tue, Dec 17, 2013 at 04:16:56PM +0100, Marek Vasut wrote:
> On Tuesday, December 17, 2013 at 02:49:25 PM, Huang Shijie wrote:
> > On Tue, Dec 17, 2013 at 02:05:55PM +0100, Marek Vasut wrote:
> > > On Monday, December 16, 2013 at 09:58:45 AM, Huang Shijie wrote:
> > > > +struct spi_nor { > > +	struct mtd_info		*mtd;
> > > > +	struct mutex		lock;
> > > > +
> > > > +	/* pointer to a spi device */
> > > > +	struct device		*dev;
> > > > +	u32			page_size;
> > > > +	u8			addr_width;
> > > > +	u8			erase_opcode;
> > > > +	u8			read_opcode;
> > > > +	u8			read_dummy;
> > > > +	u8			program_opcode;
> > > > +	enum read_mode		flash_read;
> > > > +	bool			sst_write_second;
> > > > +	struct spi_nor_xfer_cfg	cfg;
> > > 
> > > You do want to split the function pointers below and the device
> > > configuration above into separate structures.
> > 
> > sorry, i prefer to keep them in one data structrue, just like the
> > nand_chip{} does.
> 

Take @read_reg for example, currently, we call it with nor->read_reg();
If we add a new data structrue for these hooks, we wil call it with
nor->ops->read_reg().

So just like the nand_chip{}, make the code more simple.


thanks
Huang Shijie
diff mbox

Patch

diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
index ab2ea1e..83ca63d 100644
--- a/include/linux/mtd/spi-nor.h
+++ b/include/linux/mtd/spi-nor.h
@@ -50,4 +50,112 @@ 
 /* Configuration Register bits. */
 #define CR_QUAD_EN_SPAN		0x2     /* Spansion Quad I/O */
 
+enum read_mode {
+	SPI_NOR_NORMAL = 0,
+	SPI_NOR_FAST,
+	SPI_NOR_QUAD,
+};
+
+/*
+ * struct spi_nor_xfer_cfg - Structure for defining a Serial Flash transfer
+ * @wren:		command for "Write Enable", or 0x00 for not required
+ * @cmd:		command for operation
+ * @cmd_pins:		number of pins to send @cmd (1, 2, 4)
+ * @addr:		address for operation
+ * @addr_pins:		number of pins to send @addr (1, 2, 4)
+ * @addr_width: 	number of address bytes (3,4, or 0 for address not required)
+ * @mode:		mode data
+ * @mode_pins:		number of pins to send @mode (1, 2, 4)
+ * @mode_cycles:	number of mode cycles (0 for mode not required)
+ * @dummy_cycles:	number of dummy cycles (0 for dummy not required)
+ */
+struct spi_nor_xfer_cfg {
+	u8		wren;
+	u8		cmd;
+	u8		cmd_pins;
+	u32		addr;
+	u8		addr_pins;
+	u8		addr_width;
+	u8		mode;
+	u8		mode_pins;
+	u8		mode_cycles;
+	u8		dummy_cycles;
+};
+
+#define	SPI_NOR_MAX_CMD_SIZE	8
+enum spi_nor_ops {
+	SPI_NOR_OPS_READ = 0,
+	SPI_NOR_OPS_WRITE,
+	SPI_NOR_OPS_ERASE,
+	SPI_NOR_OPS_LOCK,
+	SPI_NOR_OPS_UNLOCK,
+};
+
+struct spi_nor {
+	struct mtd_info		*mtd;
+	struct mutex		lock;
+
+	/* pointer to a spi device */
+	struct device		*dev;
+	u32			page_size;
+	u8			addr_width;
+	u8			erase_opcode;
+	u8			read_opcode;
+	u8			read_dummy;
+	u8			program_opcode;
+	enum read_mode		flash_read;
+	bool			sst_write_second;
+	struct spi_nor_xfer_cfg	cfg;
+
+	/* for write_reg */
+	u8			cmd_buf[SPI_NOR_MAX_CMD_SIZE];
+
+	/*
+	 * Do some work before or after we run these operations:
+	 *   read/write/erese/lock/unlock
+	 */
+	int (*prepare)(struct spi_nor *nor, enum spi_nor_ops ops);
+	void (*unprepare)(struct spi_nor *nor, enum spi_nor_ops ops);
+
+	/*
+	 * The two fundamental primitives, you can use them to implement
+	 * all the other hooks, except the prepare/unprepare.
+	 */
+	int (*read_xfer)(struct spi_nor *nor, struct spi_nor_xfer_cfg *cfg,
+			 u8 *buf, size_t len);
+	int (*write_xfer)(struct spi_nor *nor, struct spi_nor_xfer_cfg *cfg,
+			  u8 *buf, size_t len);
+
+	/*
+	 * The two hooks are used to read/write SPI NOR register, such as
+	 * read status register, write status register.
+         */
+	int (*read_reg)(struct spi_nor *nor, u8 opcode, u8 *buf, int len);
+	int (*write_reg)(struct spi_nor *nor, u8 opcode, u8 *buf, int len,
+			int write_enable);
+
+	/*
+	 * The hook for reading out the ID, the spi-nor controller drivers
+         * can fill it with its own implementation if the default
+	 * could not meet its requirement.
+	 */
+	const struct spi_device_id *(*read_id)(struct spi_nor *nor);
+
+	/*
+	 * The hook for "Wait till ready", some spi-nor controller drivers
+	 * may fill it with its own implementation.
+	 */
+	int (*wait_till_ready)(struct spi_nor *nor);
+
+	/* write */
+	void (*write)(struct spi_nor *nor, loff_t to,
+			size_t len, size_t *retlen, const u_char *buf);
+	/* read */
+	int (*read)(struct spi_nor *nor, loff_t from,
+			size_t len, size_t *retlen, u_char *buf);
+	/* erase a sector(4K/64K, etc..) */
+	int (*erase)(struct spi_nor *nor, loff_t offs);
+
+	void *priv;
+};
 #endif