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

From: "Michael Walle" <mwalle@kernel.org>
To: "Miquel Raynal" <miquel.raynal@bootlin.com>
Cc: "Pratyush Yadav" <pratyush@kernel.org>,
	"Takahiro Kuwano" <takahiro.kuwano@infineon.com>,
	"Richard Weinberger" <richard@nod.at>,
	"Vignesh Raghavendra" <vigneshr@ti.com>,
	"Nicolas Ferre" <nicolas.ferre@microchip.com>,
	"Alexandre Belloni" <alexandre.belloni@bootlin.com>,
	"Claudiu Beznea" <claudiu.beznea@tuxon.dev>,
	"Steam Lin" <STLin2@winbond.com>,
	"Hsin-Yi Wang" <hsinyi@chromium.org>,
	"Thomas Petazzoni" <thomas.petazzoni@bootlin.com>,
	<linux-mtd@lists.infradead.org>,
	<linux-arm-kernel@lists.infradead.org>,
	<linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 1/5] mtd: spi-nor: Refactor Read Status/Write Status support
Date: Mon, 10 Aug 2026 08:50:13 +0200	[thread overview]
Message-ID: <DKL2BE8MZUQJ.7GKQF4IIQEJN@kernel.org> (raw)
In-Reply-To: <87y0ekek8r.fsf@bootlin.com>

[-- Attachment #1.1: Type: text/plain, Size: 2335 bytes --]

Hi,

On Wed Aug 5, 2026 at 4:04 PM CEST, Miquel Raynal wrote:
> 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

Actually, what I had i mind is that there is no more SR2 and so on.
But just one SR (and the BIT macros will change of course). Some
datasheets are already naming the bits B0 to B15.

> - endianness shall be handled, people always get it wrong (including
>   me), so why bother if an array just makes the whole thing simpler?

Where would you need endianess? If at all, just in the core, which
will split the u32 into two u8.

 sr1 = sr & 0xff;
 sr2 = (sr >> 8) & 0xff;

> - 16-bit accesses get more convoluted, we need extra
>   get_/put_unaligned_le32() calls and pack/unpack bytes in the hot path

There is not really a hot path, is it. Not in a sense that it
matters for performance though.

> - 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.

Because IMHO an array of two u8 (which your qe mask is) is never
self explanatory. While with

#define SR1_QUAD_EN_BIT6 BIT(6)
#define SR2_QUAD_EN_BIT1 BIT(9)
#define SR2_QUAD_EN_BIT7 BIT(15)

you can just use the macros as before.

-michael

> 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

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 297 bytes --]

[-- Attachment #2: Type: text/plain, Size: 144 bytes --]

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