[PATCH 2/4] mtd: spi-nor: add a new data structrue spi_nor{}
Huang Shijie
b32955 at freescale.com
Tue Nov 26 23:35:28 EST 2013
于 2013年11月26日 19:42, Gupta, Pekon 写道:
>> From: Huang Shijie [mailto:b32955 at 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
More information about the linux-arm-kernel
mailing list