Re: [PATCH 1/2] m25p80, spi-nor: Update id list of Macronix chips

[Brian Norris]

(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@xxxxxxxxx> 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?

But this also suggests that you really need to split your patches more.
You're doing many unrelated things in each patch, and you provide no
rationale for much of it.

> Second, sorry that I directly delete original chip IDs. Some chips are not
> supported by Macronix anymore, so I change the name to a more popular one
> and keep the original device ID. I also add lots of new devices to the table.
> I forgot that the name might still be needed by someone.
> And you're right, the sfdp detection is no need to read IDs. I'll remove these
> part of modifications in the next version. I hope this way can make my patch
> function more simple.
> 
> Summary for next version:
> - Document the patch better and more detail.

Definitely.

> - Remove the part of ID modifications.

Yes. Or at least, don't remove/rename old entries. If new flash with
names are compatible with old names, just use the old name.

If you need to add any information to existing entries, note why. And if
you need to add new entries, make these a separate patch.

> Please let me know if there are any other suggestions.

Big goal: make your patches smaller, so that they encapsulate only a
single thought and can be easily described by you and
understood/reviewed by others.

You might also strip down your patches to just what's necessary to
detect a flash using only SFDP (not binding to a particular name/ID in
the spi_nor_ids[] table). Worrying about the OTP, lock/unlock, etc.,
that you loaded into patch 2 will more likely bog down your review. Or
at least, make these changes into an atomic patch (or patch set) that's
at the end of the series, so we can tackle the more important chunks
first.

Brian
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/