Re: [PATCH v2 2/2] mtd: spi-nor: issi: Add support for is25wx01g

From: "Nuno Sá" <nuno.sa@analog.com>
To: Michael Walle <mwalle@kernel.org>
Cc: linux-mtd@lists.infradead.org, linux-kernel@vger.kernel.org,
	Pratyush Yadav <pratyush@kernel.org>,
	Takahiro Kuwano <takahiro.kuwano@infineon.com>,
	Miquel Raynal <miquel.raynal@bootlin.com>,
	Richard Weinberger <richard@nod.at>,
	Vignesh Raghavendra <vigneshr@ti.com>
Subject: Re: [PATCH v2 2/2] mtd: spi-nor: issi: Add support for is25wx01g
Date: Mon, 14 Sep 2026 16:31:57 +0100	[thread overview]
Message-ID: <aqgRsP3cxzcLok5C@nsa> (raw)
In-Reply-To: <DLF3H41RCZJF.37WOY30CYJ4AY@kernel.org>

On Mon, Sep 14, 2026 at 04:04:40PM +0200, Michael Walle wrote:
> Hi,
> 
> On Mon Sep 14, 2026 at 3:42 PM CEST, Nuno Sá wrote:
> > (*): I should note that the command actually failed with -EIO but it
> > actually unlocked the chip! And the reason is because the flash as the same
> > FSR register than the micron-st flash. So WEL is set to 1 but can only
> > be cleared when clearing the FSR register.
> 
> Why doesn't this affect only the locking operation? WEL polling is
> used also during write and erase.

Not sure if I fully understand. But AFAICT, the reason why erase and
write is silent is because the default spi_nor_sr_ready() only looks at
SR_WIP [1]:

OTOH, on the unlock path we do spi_nor_write_sr1_and_sr2_and_check() and
give no special handling to WEL so I imagine that we try to set it as 0
but read it as 1 (given that it clears only with FSR) and hence I got
the -EIO in [2].

[1]: https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git/tree/drivers/mtd/spi-nor/core.c?h=spi-nor/next#n558
[2]: https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git/tree/drivers/mtd/spi-nor/core.c?h=spi-nor/next#n970

> 
> > AFAICT, we should do something similar as micron so the writing to an
> > actual protected region fails rather than being silently discarded with
> > that status bit set. The question would be how to do it? The code is
> > pretty much identical to [1]. The masks, the opcoded... So should we
> > somehow handle this in the core (by having some common helper) that
> > could be set in .late_init() under a common MFR_FSR flag? Or just keep
> > both implementations separate for now?
> 
> I'd like to keep that out of the core.c, but also like to avoid any
> code duplication esp. because there is already handling for the
> intel spi controller in there. So maybe move it it into a new
> common.c.

Also don't like the dup tbh. Could that be a follow up or should it be
v3. From the top of my head I could think on a mfr_common.c kind of thing.
Don't thing this FSR register is standard?

Thx!
- Nuno Sá

> 
> Apart from that, this patch set looks good to go.
> 
> -michael
> 
> > [1]: https://elixir.bootlin.com/linux/v7.2.5/source/drivers/mtd/spi-nor/micron-st.c#L585

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