Re: [PATCH v2 17/25] mtd: spi-nor: winbond: Add support for W25Q33PW-Q/N

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] spi-nor: winbond: commit message claims ECC support for W25Q33PW-Q/N but code excludes it
--

commit a0e0dc16aaef35a04a300d228b7e07949d22b31b
Author: Miquel Raynal <miquel.raynal@xxxxxxxxxxx>

mtd: spi-nor: winbond: Add support for W25Q33PW-Q/N
    
There is an ID collision with chips of same density from the JW family
(which it self conflicted with yet another family). Chips are very
similar in practice, it is mostly a matter of electrical differences
(mostly power consumption being lower) as well as the addition of inline
ECC capability.

>     There is an ID collision with chips of same density from the JW family
>     (which it self conflicted with yet another family). Chips are very
>     similar in practice, it is mostly a matter of electrical differences
>     (mostly power consumption being lower) as well as the addition of inline
>     ECC capability.
>     
>     16 bytes chunks are protected against single errors. In case a
>     non-aligned write happens, ECC is locally disabled until the next erase.

[Severity: Medium]
Does the W25Q33PW-Q/N actually have ECC support?

The commit message claims it does, but this patch maps the W25Q33PW-Q/N to
ID 0xef 0x60 0x16. There is existing logic that explicitly excludes this
density from ECC configuration:

drivers/mtd/spi-nor/winbond.c:winbond_pw_with_ecc_match() {
	...
	/* W25QxxPW densities <= 32 Mbit (id[2] <= 0x16) do not have built-in ECC support */
	return id->len == 3 && id->bytes[2] >= 0x17;
}

Because id->bytes[2] is 0x16, this function will return false and ECC won't
be enabled.

Could the commit message be inaccurate, or does the ECC exclusion logic need
to be updated to support the W25Q33PW-Q/N?

> diff --git a/drivers/mtd/spi-nor/winbond.c b/drivers/mtd/spi-nor/winbond.c
> index a2dea77dfc2ce..6934f3e10ec3f 100644
> --- a/drivers/mtd/spi-nor/winbond.c
> +++ b/drivers/mtd/spi-nor/winbond.c
> @@ -373,7 +373,7 @@ static const struct flash_info winbond_nor_parts[] = {
>  		.flags = SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB,
>  		.no_sfdp_flags = SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ,
>  	}, {
> -		/* W25Q32DW-P/G, W25Q32FW-G/Q, W25Q32JW-Q/N */
> +		/* W25Q32DW-P/G, W25Q32FW-G/Q, W25Q32JW-Q/N, W25Q33PW-Q/N */
>  		.id = SNOR_ID(0xef, 0x60, 0x16),
>  		.name = "w25q32dw",
>  		.size = SZ_4M,

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260920-winbond-master-spi-nor-jw-cleanup-pw-addition-v2-0-ab9c9688d071@xxxxxxxxxxx?part=17