Message ID | 1385447575-23773-3-git-send-email-b32955@freescale.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
> From: Huang Shijie [mailto:b32955@freescale.com] [...] > > +#define MAX_CMD_SIZE 6 > + > +enum read_type { > + M25P80_NORMAL = 0, > + M25P80_FAST, > + M25P80_QUAD, > +}; Sorry. no 'M25P80' suffix here this is spi-nor.h :-) > + > +struct spi_nor { > + struct mutex lock; > + struct mtd_info mtd; > mtd_info should not be present here. Rather it should be other way round 'mtd_info->priv = (struct spi_nor *) spi_nor; > + struct device *dev; Again, spi_nor would be a MTD device, not a new type of device on its own. Thus you should use, mtd_info->dev. > + u16 page_size; > + u16 addr_width; > + u8 erase_opcode; > + u8 read_opcode; s/read_opcode/read_flash_opcode How about '+ u8 read_reg_opcode' ?? > + u8 program_opcode; + How about '+ u8 write_reg_opcode' ?? > + u8 command[MAX_CMD_SIZE]; > + enum read_type flash_read; s/read_type/read_mode (agree .. there is nothing in the name, but it matches SPI generic framework) Other missing fields I can think of are.. + u8 read_dummy_cycles; + u8 write_dummy_cycles; > + bool sst_write_second; > + > + /* > + * Read the register: > + * Read `len` length data from the register specified by the `opcode`, > + * and store the data to the `buf`. > + */ > + int (*read_reg)(struct spi_nor *flash, u8 opcode, u8 *buf, int len); > Do you need 'opcode' passed in argument here ? Can you add it as 'read_reg_opcode' field in struct spi_nor ? 'read_reg_opcode' should be fixed for a device like a 'read_flash_opcode'. > + > + /* > + * Write the register: > + * Write the `cmd_len` length data stored in the @command to the > NOR, > + * the command[0] stores the write opcode. `offset` is only used for > + * erase operation, it should set to zero for other NOR commands. > + */ > + int (*write_reg)(struct spi_nor *flash, int cmd_len, u32 offset); > Instead of having actual 'command[]' array in struct spi_nor, and pass its valid length here.. shouldn't you pass the command as u8[] here.. int (*write_reg)(struct spi_nor *flash, u8 *cmd, u32 cmd_len); where cmd[0] == command_opcode cmd[1] == command argument 1 (like offset for erase) cmd[2] == command argument 2 ... > + > + /* write data */ > + void (*write)(struct spi_nor *flash, loff_t to, size_t len, > + size_t *retlen, const u_char *buf); > + /* read data */ > + int (*read)(struct spi_nor *flash, loff_t from, size_t len, > + size_t *retlen, u_char *buf); > +}; > #endif > -- > 1.7.2.rc3 > Sorry for bit intrusive feedback. But I think it would help you to refine it better. Also some reference below http://lists.infradead.org/pipermail/linux-mtd/2013-October/049307.html with regards, pekon
? 2013?11?26? 19:42, Gupta, Pekon ??: >> From: Huang Shijie [mailto:b32955@freescale.com] > [...] >> +#define MAX_CMD_SIZE 6 >> + >> +enum read_type { >> + M25P80_NORMAL = 0, >> + M25P80_FAST, >> + M25P80_QUAD, >> +}; > Sorry. no 'M25P80' suffix here this is spi-nor.h :-) > > ok. thanks. :) >> + >> +struct spi_nor { >> + struct mutex lock; >> + struct mtd_info mtd; >> > mtd_info should not be present here. Rather it should be other way round > 'mtd_info->priv = (struct spi_nor *) spi_nor; > > put the mtd here can make code simple, do David/Brian have any comment about this? If all object to put the mtd here, i will change it. >> + struct device *dev; > Again, spi_nor would be a MTD device, not a new type of device on its own. > Thus you should use, mtd_info->dev. > > this dev pointer is not from the mtd_info->dev, it from the spi_device or other spi nor device . >> + u16 page_size; >> + u16 addr_width; >> + u8 erase_opcode; >> + u8 read_opcode; > s/read_opcode/read_flash_opcode why not keep the legacy name? should we also rename the erase_opcode to erase_flash_opcode? :) > How about '+ u8 read_reg_opcode' ?? > > >> + u8 program_opcode; > + How about '+ u8 write_reg_opcode' ?? > > >> + u8 command[MAX_CMD_SIZE]; >> + enum read_type flash_read; > s/read_type/read_mode > (agree .. there is nothing in the name, but it matches SPI generic framework) > > Other missing fields I can think of are.. > + u8 read_dummy_cycles; I was wondering if other SPI NOR driver needs this field. current dummy is 8 clock cycles for quad-read/fast-read, but for DDR QUAD read, the dummy is not 8 clock cycles. so i did not add this field to spi-nor{}. I do not know how the m25p80 handle the non-8 clock cycles cases... But it's okay to me to add this field to spi-nor{}. > + u8 write_dummy_cycles; > > do the write need a dummy? I check the s25fl128s's quad page program, it does not need a dummy. >> + bool sst_write_second; >> + >> + /* >> + * Read the register: >> + * Read `len` length data from the register specified by the `opcode`, >> + * and store the data to the `buf`. >> + */ >> + int (*read_reg)(struct spi_nor *flash, u8 opcode, u8 *buf, int len); >> > Do you need 'opcode' passed in argument here ? > Can you add it as 'read_reg_opcode' field in struct spi_nor ? > 'read_reg_opcode' should be fixed for a device like a 'read_flash_opcode'. > > the @read_reg can be used to read id, read status and so on. so the opcode here is not a fixed value. but i do not object to add the read_reg_opcode/write_reg_opcode to spi_nor{}. >> + >> + /* >> + * Write the register: >> + * Write the `cmd_len` length data stored in the @command to the >> NOR, >> + * the command[0] stores the write opcode. `offset` is only used for >> + * erase operation, it should set to zero for other NOR commands. >> + */ >> + int (*write_reg)(struct spi_nor *flash, int cmd_len, u32 offset); >> > Instead of having actual 'command[]' array in struct spi_nor, and pass its > valid length here.. shouldn't you pass the command as u8[] here.. > int (*write_reg)(struct spi_nor *flash, u8 *cmd, u32 cmd_len); > where > cmd[0] == command_opcode > cmd[1] == command argument 1 (like offset for erase) > cmd[2] == command argument 2 > ... > is there any benefit of your code? you will use a local array in the stack. why not use the spi_nor->command which has used for a long time. >> + >> + /* write data */ >> + void (*write)(struct spi_nor *flash, loff_t to, size_t len, >> + size_t *retlen, const u_char *buf); >> + /* read data */ >> + int (*read)(struct spi_nor *flash, loff_t from, size_t len, >> + size_t *retlen, u_char *buf); >> +}; >> #endif >> -- >> 1.7.2.rc3 >> > Sorry for bit intrusive feedback. But I think it would help you to > refine it better. Also some reference below > http://lists.infradead.org/pipermail/linux-mtd/2013-October/049307.html > thanks. I referenced it when i wrote this patch. thanks Huang Shijie
Dear Huang Shijie, > ? 2013?11?26? 19:42, Gupta, Pekon ??: > >> From: Huang Shijie [mailto:b32955@freescale.com] > > > > [...] > > > >> +#define MAX_CMD_SIZE 6 > >> + > >> +enum read_type { > >> + M25P80_NORMAL = 0, > >> + M25P80_FAST, > >> + M25P80_QUAD, > >> +}; > > > > Sorry. no 'M25P80' suffix here this is spi-nor.h :-) > > ok. thanks. :) > > >> + > >> +struct spi_nor { > >> + struct mutex lock; > >> + struct mtd_info mtd; > > > > mtd_info should not be present here. Rather it should be other way round > > 'mtd_info->priv = (struct spi_nor *) spi_nor; > > put the mtd here can make code simple, The MTD API functions will pass you the struct mtd_info anyway, so you will need to implement mtdinfo_to_yourdriverdata() function, no need for duplication. > do David/Brian have any comment about this? > If all object to put the mtd here, i will change it. > > >> + struct device *dev; > > > > Again, spi_nor would be a MTD device, not a new type of device on its > > own. Thus you should use, mtd_info->dev. > > this dev pointer is not from the mtd_info->dev, it from the spi_device > or other spi nor device . So this is your own device pointer or ... what kind of device pointer? [...] Best regards, Marek Vasut
? 2013?11?27? 17:32, Marek Vasut ??:
> So this is your own device pointer or ... what kind of device pointer?
the pointer from spi_device{}
Huang Shijie
Dear Huang Shijie, > ? 2013?11?27? 17:32, Marek Vasut ??: > > So this is your own device pointer or ... what kind of device pointer? > > the pointer from spi_device{} OK, I am 200% positive you need to add comments into the structure about what exactly does each member of it mean AND it would be nice if you could rename them to have less generic names (like spi_dev instead of dev). Thanks ! :) Best regards, Marek Vasut
diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c index 4703aa4..13d9864 100644 --- a/drivers/mtd/devices/m25p80.c +++ b/drivers/mtd/devices/m25p80.c @@ -44,12 +44,6 @@ /****************************************************************************/ -enum read_type { - M25P80_NORMAL = 0, - M25P80_FAST, - M25P80_QUAD, -}; - struct m25p { struct spi_device *spi; struct mutex lock; diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h index ab2ea1e..8da1f69 100644 --- a/include/linux/mtd/spi-nor.h +++ b/include/linux/mtd/spi-nor.h @@ -50,4 +50,47 @@ /* Configuration Register bits. */ #define CR_QUAD_EN_SPAN 0x2 /* Spansion Quad I/O */ +#define MAX_CMD_SIZE 6 + +enum read_type { + M25P80_NORMAL = 0, + M25P80_FAST, + M25P80_QUAD, +}; + +struct spi_nor { + struct mutex lock; + struct mtd_info mtd; + struct device *dev; + u16 page_size; + u16 addr_width; + u8 erase_opcode; + u8 read_opcode; + u8 program_opcode; + u8 command[MAX_CMD_SIZE]; + enum read_type flash_read; + bool sst_write_second; + + /* + * Read the register: + * Read `len` length data from the register specified by the `opcode`, + * and store the data to the `buf`. + */ + int (*read_reg)(struct spi_nor *flash, u8 opcode, u8 *buf, int len); + + /* + * Write the register: + * Write the `cmd_len` length data stored in the @command to the NOR, + * the command[0] stores the write opcode. `offset` is only used for + * erase operation, it should set to zero for other NOR commands. + */ + int (*write_reg)(struct spi_nor *flash, int cmd_len, u32 offset); + + /* write data */ + void (*write)(struct spi_nor *flash, loff_t to, size_t len, + size_t *retlen, const u_char *buf); + /* read data */ + int (*read)(struct spi_nor *flash, loff_t from, size_t len, + size_t *retlen, u_char *buf); +}; #endif
The spi_nor{} is cloned from the m25p{}. The spi_nor{} can be used by both the m25p80 and spi-nor controller. 1) Add four hooks: @read_reg: used to read the registers, such as read status register, read ID, read configure register. @write_reg: used to write the registers, such as write enable, erase sector. @read: use the proper read opcode to read out the data from the NOR. @write: use the proper write opcode to write data to the NOR. 2) Add a new field sst_write_second for the SST NOR write. 3) change the @command field from pointer to array which makes the code more simple. Signed-off-by: Huang Shijie <b32955@freescale.com> --- drivers/mtd/devices/m25p80.c | 6 ----- include/linux/mtd/spi-nor.h | 43 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 43 insertions(+), 6 deletions(-)