Re: [PATCH v2 10/17] mtd: spi-nor: winbond: Prepare the introduction of W35TxxNW parts

From: Miquel Raynal <miquel.raynal@bootlin.com>
To: sashiko-bot@kernel.org
Cc: "Vignesh Raghavendra" <vigneshr@ti.com>,
	 "Michael Walle" <mwalle@kernel.org>,
	 "Pratyush Yadav" <pratyush@kernel.org>,
	 "Richard Weinberger" <richard@nod.at>,
	 "Takahiro Kuwano" <takahiro.kuwano@infineon.com>,
	 sashiko-reviews@lists.linux.dev,
	 "Steam Lin" <STLin2@winbond.com>,
	 linux-kernel@vger.kernel.org, linux-mtd@lists.infradead.org,
	 "Thomas Petazzoni" <thomas.petazzoni@bootlin.com>
Subject: Re: [PATCH v2 10/17] mtd: spi-nor: winbond: Prepare the introduction of W35TxxNW parts
Date: Tue, 13 Oct 2026 18:22:40 +0200	[thread overview]
Message-ID: <87qzhtvacv.fsf@bootlin.com> (raw)
In-Reply-To: <20261004214530.837371F000FF@smtp.kernel.org> (sashiko-bot@kernel.org's message of "Sun, 04 Oct 2026 21:45:30 +0000")

On 04/10/2026 at 21:45:30 GMT, sashiko-bot@kernel.org wrote:

> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] mtd: spi-nor: winbond: missing Flag Register polling ignores P/E errors
> --
>
> commit eef1bdae97bf8ec6c5ee8f47f2358cc4351e258b
> Author: Miquel Raynal <miquel.raynal@bootlin.com>
>
> mtd: spi-nor: winbond: Prepare the introduction of W35TxxNW parts
>
> This commit prepares for a new octal DTR family by adding callbacks for octal
> DTR modes and dummy cycles configuration. It also mentions these chips feature
> SECDEC and lack SR2, but they do have a flag register.
>
>> diff --git a/drivers/mtd/spi-nor/winbond.c b/drivers/mtd/spi-nor/winbond.c
>> index 51f6bcd3f8e5b..058a2951c184e 100644
>> --- a/drivers/mtd/spi-nor/winbond.c
>> +++ b/drivers/mtd/spi-nor/winbond.c
>> @@ -11,6 +11,13 @@
>>  #define WINBOND_NOR_OP_RDEAR	0xc8	/* Read Extended Address Register */
>>  #define WINBOND_NOR_OP_WREAR	0xc5	/* Write Extended Address Register */
>>  #define WINBOND_NOR_OP_SELDIE	0xc2	/* Select active die */
>> +#define WINBOND_NOR_OP_WR_VCR	0x81	/* Write VCR registers */
>> +#define   WINBOND_NOR_VCR_IO_MODE_CFG_REG 0x00 /* IO mode configuration address (VCR) */
>> +#define     WINBOND_NOR_VCR_IO_MODE_SSDR 0xFF
>> +#define     WINBOND_NOR_VCR_IO_MODE_ODDR 0xE7
>> +#define   WINBOND_NOR_VCR_DUMMY_CFG_REG 0x01 /* Dummy cycle configuration address (VCR) */
>> +#define WINBOND_NOR_OP_RD_FR	0x70	/* Read Flag registers */
>> +#define WINBOND_NOR_OP_CL_FR	0x50	/* Clear Flag registers */
>
> [Severity: High]
> Are these flag register opcodes omitted from the driver logic?

I'll drop these.

>> +static int winbond_w35txxnw_nor_late_init(struct spi_nor *nor)
>> +{
>> +	nor->params->set_octal_dtr = winbond_w35txxnw_nor_set_octal_dtr;
>> +	nor->params->set_dummy = winbond_w35txxnw_nor_set_dummy;
>> +	nor->params->opcodes.read_sr2 = 0;
>> +	nor->params->opcodes.write_sr1_and_sr2 = 0;
>
> [Severity: High]
> Should a custom ready callback be assigned here to poll the Flag Register?
>
> Since no custom callback is assigned, the driver falls back to the default
> spi_nor_sr_ready, which only checks the SR1 WIP bit and does not check the 
> Flag Register for errors.

The ready callback is not here for that. It shall just return once the
operation is done, regardless of its success.

What I have in mind is to maybe, if I continue the ECC route, to open
the way for new callbacks allowing to check the validity of the data
(SECDEC) after reads, erases and writes, report corrected bitflips when
possible, but also why not make full memory ranges checks and fully use
the Flag Register (or let that to be done by ubihealthd, I don't know
yet).

But for now let's focus on basic support.

Miquèl

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