Re: [PATCH 1/5] mtd: spi-nor: Refactor Read Status/Write Status support

[Miquel Raynal <miquel.raynal@bootlin.com>]

Hello Michael,

>>> +int spi_nor_read_srs(struct spi_nor *nor, u8 *sr1, u8 *sr2)
>>
>> nitpick, i think this is a weird name. But maybe it's just me.
>>
>> What if we'd just make the one status register as u32? I mean in the
>> winbond datasheet it's called S7-S0 and S15-S8. That way we also
>> don't need a two byte qe_mask. To optimize the standard usecase to
>> poll the WIP bit, we could add a byte mask.

I experimented a bit the u32 status register, I don't think it is a good
idea.

My target of improving the readability is completely defeated by the
fact that:
- all the callers need to add extra maths to explain what SR
  they want
- endianness shall be handled, people always get it wrong (including
  me), so why bother if an array just makes the whole thing simpler?
- 16-bit accesses get more convoluted, we need extra
  get_/put_unaligned_le32() calls and pack/unpack bytes in the hot path
- 8-bit accesses imply an extensive use of FIELD_GET(GENMASK(),) macros,
  which make the whole fonction totally non obvious anymore.

Whereas, a simple:

        if (sr2)
                *sr2 = foo;

is self explanatory. I also do not really get the wish for a QE mask
instead of a two bytes array.

So I will rename the "srs" naming that you dislike, I will group the
opcodes in a big structure, change the baseline default to match your
suggestion and follow-up with the other minor comments, but I believe
I'm going to avoid the u32 switch.

Thanks,
Miquèl

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/