From: Huang Shijie <b32955@freescale.com>
To: "Gupta, Pekon" <pekon@ti.com>
Cc: "marex@denx.de" <marex@denx.de>,
"broonie@linaro.org" <broonie@linaro.org>,
"linux-mtd@lists.infradead.org" <linux-mtd@lists.infradead.org>,
"Poddar, Sourav" <sourav.poddar@ti.com>,
"computersforpeace@gmail.com" <computersforpeace@gmail.com>,
"dwmw2@infradead.org" <dwmw2@infradead.org>,
"linux-arm-kernel@lists.infradead.org"
<linux-arm-kernel@lists.infradead.org>
Subject: Re: [PATCH 2/4] mtd: spi-nor: add a new data structrue spi_nor{}
Date: Wed, 27 Nov 2013 12:35:28 +0800 [thread overview]
Message-ID: <52957690.60102@freescale.com> (raw)
In-Reply-To: <20980858CB6D3A4BAE95CA194937D5E73EA4EEF0@DBDE04.ent.ti.com>
于 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
next prev parent reply other threads:[~2013-11-27 4:35 UTC|newest]
Thread overview: 64+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-11-26 6:32 [PATCH 0/4] mtd: spi-nor: add a new framework for SPI NOR Huang Shijie
2013-11-26 6:32 ` [PATCH 1/4] mtd: spi-nor: move the SPI NOR commands to a new header file Huang Shijie
2013-11-26 7:42 ` Gupta, Pekon
2013-11-26 8:53 ` Huang Shijie
2013-11-26 14:48 ` Angus Clark
2013-11-26 6:32 ` [PATCH 2/4] mtd: spi-nor: add a new data structrue spi_nor{} Huang Shijie
2013-11-26 11:42 ` Gupta, Pekon
2013-11-27 4:35 ` Huang Shijie [this message]
2013-11-27 9:32 ` Marek Vasut
2013-11-27 10:24 ` Huang Shijie
2013-11-27 10:27 ` Marek Vasut
2013-11-26 6:32 ` [PATCH 3/4] mtd: spi-nor: add the framework for SPI NOR Huang Shijie
2013-11-26 10:03 ` Gupta, Pekon
2013-11-27 9:39 ` Marek Vasut
2013-11-26 6:32 ` [PATCH 4/4] mtd: m25p80: use the new spi-nor APIs Huang Shijie
2013-11-26 12:57 ` [PATCH 0/4] mtd: spi-nor: add a new framework for SPI NOR Angus Clark
2013-11-27 4:32 ` Brian Norris
2013-11-27 4:39 ` Huang Shijie
2013-11-29 14:52 ` Angus Clark
2013-12-02 10:06 ` Huang Shijie
2013-12-02 11:01 ` Gupta, Pekon
2013-12-02 11:19 ` Angus Clark
2013-12-03 6:20 ` Huang Shijie
2013-12-03 8:23 ` Lee Jones
2013-12-10 8:25 ` Brian Norris
2013-12-10 10:00 ` Lee Jones
2013-12-03 0:32 ` Marek Vasut
2013-12-03 10:36 ` Huang Shijie
2013-12-03 14:51 ` David Woodhouse
2013-12-04 18:44 ` Brian Norris
2013-12-05 7:12 ` Huang Shijie
2013-12-05 8:11 ` Brian Norris
2013-12-05 7:59 ` Huang Shijie
2013-12-05 9:20 ` Brian Norris
2013-12-06 3:07 ` Huang Shijie
2013-12-05 14:35 ` Angus Clark
2013-12-06 8:18 ` Huang Shijie
2013-12-10 9:08 ` Brian Norris
2013-12-04 2:46 ` Huang Shijie
2013-12-04 6:58 ` Angus Clark
2013-12-04 7:19 ` Gupta, Pekon
2013-12-04 8:21 ` Angus Clark
2013-12-04 15:36 ` Marek Vasut
2013-12-05 2:42 ` Huang Shijie
2013-12-05 5:43 ` Gupta, Pekon
2013-12-05 7:33 ` Huang Shijie
2013-11-27 9:27 ` Marek Vasut
2013-11-27 9:47 ` Sourav Poddar
2013-11-27 10:06 ` Marek Vasut
2013-11-27 10:56 ` Sourav Poddar
2013-12-02 23:59 ` Marek Vasut
2013-12-03 10:01 ` Sourav Poddar
2013-12-03 13:42 ` Marek Vasut
2013-12-03 13:50 ` Sourav Poddar
2013-12-03 14:19 ` Marek Vasut
2013-12-03 14:31 ` Sourav Poddar
2013-12-03 15:09 ` Marek Vasut
2013-12-03 15:16 ` Sourav Poddar
2013-12-03 15:35 ` Marek Vasut
2013-12-03 15:23 ` David Woodhouse
2013-12-03 18:28 ` Brian Norris
2013-12-03 23:41 ` Marek Vasut
2013-11-27 10:19 ` Huang Shijie
2013-12-03 0:00 ` Marek Vasut
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=52957690.60102@freescale.com \
--to=b32955@freescale.com \
--cc=broonie@linaro.org \
--cc=computersforpeace@gmail.com \
--cc=dwmw2@infradead.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-mtd@lists.infradead.org \
--cc=marex@denx.de \
--cc=pekon@ti.com \
--cc=sourav.poddar@ti.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).