Re: [PATCH 1/2] mtd: spi-nor: introduce SNOR_ID3()

From: Michael Walle <michael@walle.cc>
To: Tudor.Ambarus@microchip.com
Cc: p.yadav@ti.com, miquel.raynal@bootlin.com, richard@nod.at,
	vigneshr@ti.com, linux-mtd@lists.infradead.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/2] mtd: spi-nor: introduce SNOR_ID3()
Date: Tue, 19 Jul 2022 09:07:10 +0200	[thread overview]
Message-ID: <02125e047cfbf347d81811224be3141e@walle.cc> (raw)
In-Reply-To: <160ddac1-6d0a-c922-1380-09e7823a285e@microchip.com>

Am 2022-07-19 07:57, schrieb Tudor.Ambarus@microchip.com:
> On 5/10/22 17:02, Michael Walle wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know 
>> the content is safe
>> 
>> Up until now, flashes were defined by specifying the JEDEC ID, the
>> sector size and the number of sectors. This can be read by parsing the
>> SFDP, we don't have to specify it. Thus provide a new macro SNOR_ID3()
>> which just takes the JEDEC ID and implicitly set .parse_sfdp = true. 
>> All
>> new flashes which have SFDP should use this macro.
> 
> I like the idea, but you need to refine it a bit.
> Your assumptions are true only for flashes that are compliant with SFDP 
> revB or
> later because params->page_size is initialized by querying BFPT DWORD
> 11. I think it would be good to specify this in the comment section.

Sure.

> Also, I think you introduce
> a bug in spi_nor_select_erase() when CONFIG_MTD_SPI_NOR_USE_4K_SECTORS 
> is not
> selected. wanted_size will be zero, will you select an invalid erase 
> type?

You mean to squeeze [1] into this one? If so, sure.

-michael

[1] 
https://lore.kernel.org/linux-mtd/20220716000643.3541839-1-quic_jaehyoo@quicinc.com/