[2/4] mtd: spi-nor: add a new data structrue spi_nor{}

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(-)

> 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