Messages in this thread | | | Date | Tue, 10 Feb 2015 11:58:33 +0800 | From | Huang Shijie <> | Subject | Re: [PATCH 1/2] m25p80, spi-nor: Update id list of Macronix chips |
| |
On Fri, Feb 06, 2015 at 11:09:50AM -0800, Brian Norris wrote: > (fixed Huang's current email) > > Hi, > > On Fri, Feb 06, 2015 at 10:33:29PM +0800, Jim-Ting Kuo wrote: > > On Fri, Feb 6, 2015 at 9:18 AM, Brian Norris > > <computersforpeace@gmail.com> wrote: > > > On Fri, Feb 06, 2015 at 02:44:04AM +0800, Jim Kuo wrote: > > >> --- a/drivers/mtd/devices/m25p80.c > > >> +++ b/drivers/mtd/devices/m25p80.c > > >> @@ -170,6 +170,74 @@ static int m25p80_erase(struct spi_nor *nor, loff_t offset) > > >> return 0; > > >> } > > >> > > >> +static int m25p80_write_xfer(struct spi_nor *nor, > > >> + struct spi_nor_xfer_cfg *cfg, u8 *buf, size_t len) > > >> +{ > ... > > >> +} > > >> + > > >> +static int m25p80_read_xfer(struct spi_nor *nor, > > >> + struct spi_nor_xfer_cfg *cfg, u8 *buf, size_t len) > > >> +{ > ... > > >> +} > > >> + > > > > > > All of the above is pointless. The write_xfer/read_xfer functions are > > > not even used. (They should probably just be dropped, since they're > > > doing nothing as-is.) > > > > > >> /* > > >> * board specific setup should have ensured the SPI clock used here > > >> * matches what the READ command supports, at least until this driver > > >> @@ -199,6 +267,8 @@ static int m25p_probe(struct spi_device *spi) > > >> nor->erase = m25p80_erase; > > >> nor->write_reg = m25p80_write_reg; > > >> nor->read_reg = m25p80_read_reg; > > >> + nor->write_xfer = m25p80_write_xfer; > > >> + nor->read_xfer = m25p80_read_xfer; > > >> > > >> nor->dev = &spi->dev; > > >> nor->mtd = &flash->mtd; > > >> @@ -261,10 +331,15 @@ static const struct spi_device_id m25p_ids[] = { > > >> {"mr25h256"}, {"mr25h10"}, > > >> {"gd25q32"}, {"gd25q64"}, > > >> {"160s33b"}, {"320s33b"}, {"640s33b"}, > > >> - {"mx25l2005a"}, {"mx25l4005a"}, {"mx25l8005"}, {"mx25l1606e"}, > > >> - {"mx25l3205d"}, {"mx25l3255e"}, {"mx25l6405d"}, {"mx25l12805d"}, > > >> - {"mx25l12855e"},{"mx25l25635e"},{"mx25l25655e"},{"mx66l51235l"}, > > >> - {"mx66l1g55g"}, > > >> + {"mx25l512e"}, {"mx25l5121e"}, {"mx25l1006e"}, {"mx25l1021e"}, > > >> + {"mx25l2006e"}, {"mx25l4006e"}, {"mx25u4035"}, {"mx25v4035"}, > > >> + {"mx25l8006e"}, {"mx25u8035"}, {"mx25v8035"}, {"mx25l1606e"}, > > >> + {"mx25l1633e"}, {"mx25l1635e"}, {"mx25u1635e"}, {"mx25l3206e"}, > > >> + {"mx25l3239e"}, {"mx25l3225d"}, {"mx25l3255e"}, {"mx25l6406e"}, > > >> + {"mx25l6439e"}, {"mx25l12839f"}, {"mx25l12855e"}, > > >> + {"mx25u12835f"}, {"mx25l25635e"}, {"mx25l25655e"}, > > >> + {"mx25u25635f"}, {"mx66l51235f"}, {"mx66u51235f"}, > > >> + {"mx66l1g45g"}, {"mx66l1g55g"}, > > >> {"n25q064"}, {"n25q128a11"}, {"n25q128a13"}, {"n25q256a"}, > > >> {"n25q512a"}, {"n25q512ax3"}, {"n25q00"}, > > >> {"pm25lv512"}, {"pm25lv010"}, {"pm25lq032"}, > > >> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c > > >> index 0f8ec3c..a6c7337 100644 > > >> --- a/drivers/mtd/spi-nor/spi-nor.c > > >> +++ b/drivers/mtd/spi-nor/spi-nor.c > > >> @@ -545,19 +545,37 @@ static const struct spi_device_id spi_nor_ids[] = { > > >> { "640s33b", INFO(0x898913, 0, 64 * 1024, 128, 0) }, > > >> > > >> /* Macronix */ > > >> - { "mx25l2005a", INFO(0xc22012, 0, 64 * 1024, 4, SECT_4K) }, > > > > > > Nak. > > > > > >> - { "mx25l4005a", INFO(0xc22013, 0, 64 * 1024, 8, SECT_4K) }, > > > > > > Nak. > > > > > >> - { "mx25l8005", INFO(0xc22014, 0, 64 * 1024, 16, 0) }, > > > > > > Nak. > > > > > > (you get the picture) > > > > [...] > > > > > > You need to do a lot better job of documenting your patches (i.e., > > > describe why you're doing these things) if you want me to take them. > > > > > > What's more, the SFDP support you're trying to add should be done in a > > > way where you *don't* need to muck with the ID table every time. With > > > SFDP you can get most/all this information anyway, and devices should > > > just be able to bind to this driver using a generic name like > > > "spi-nor,sfdp" or something like that, instead of having to expand this > > > table. > > > > The write_xfer/read_xfer functions actually are used in patch 2. > > - read_xfer: for macronix_block_lock function. > > - write_xfer: for sfdp_read, macronix_read_lock_status function. > > Original read/write fcuntions can only support one type transfer I/O (such as > > FAST_READ, DUAL_READ or QUAD_READ which was detected at begin), > > and they also only support fixed cycle of dummy (bind with transfer command). > > So the commands not related to data transfer, such as sfdp read, lock/unlock > > are hard to use original read/write. That's why I built these two of functions. > > I thought they are suitable for this condition. > > OK, I suppose that's a little more reasonable. But I want to make sure > this is actually necessary. I suppose we can't get this functionality > with the existing read_reg()/write_reg() ops, can we? > > I'm not actually sure the exact purpose of the read_xfer()/write_xfer() > functions as originally defined. They obviously weren't used yet. > Perhaps Huang can comment here? The read_xfer()/write_xfer() are designed for the strange spi-nor controllers which has special optimizations for some operations, such as combine several operations into one operation.
I am not familiar with SFDP, but I think it will be okay if the SFDP implements the read_xfer()/write_xfer() if it really can not be implemented by the read/write/read_reg/write_reg.
please see: http://lists.infradead.org/pipermail/linux-mtd/2013-November/050492.html
Please split these patch into small patches, and document it well. :)
thanks Huang Shijie
| |