Re: [PATCH v4 3/3] mtd: spi-nor: Add support for IS25LX256 operating in 1S-8S-8S octal read mode

From: Michael Walle <michael@walle.cc>
To: Tudor Ambarus <tudor.ambarus@linaro.org>
Cc: Nathan Barrett-Morrison <nathan.morrison@timesys.com>,
	greg.malysa@timesys.com,
	Tudor Ambarus <tudor.ambarus@microchip.com>,
	Pratyush Yadav <pratyush@kernel.org>,
	Miquel Raynal <miquel.raynal@bootlin.com>,
	Richard Weinberger <richard@nod.at>,
	Vignesh Raghavendra <vigneshr@ti.com>,
	"open list:SPI NOR SUBSYSTEM" <linux-mtd@lists.infradead.org>,
	open list <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v4 3/3] mtd: spi-nor: Add support for IS25LX256 operating in 1S-8S-8S octal read mode
Date: Tue, 27 Dec 2022 13:37:43 +0100	[thread overview]
Message-ID: <14b5fc24d0e9391807768c595a4277ee@walle.cc> (raw)
In-Reply-To: <eafebda5-7342-dfa9-7745-543d5f7d6f3d@linaro.org>

Am 2022-12-26 09:17, schrieb Tudor Ambarus:
> On 26.12.2022 10:04, Tudor Ambarus wrote:
>> Hi, Nathan,
>> 
>> The series is starting to look good, but I'll need another version,
>> please.
>> 
>> On 02.12.2022 15:55, Nathan Barrett-Morrison wrote:
>>> This adds the IS25LX256 chip into the ISSI flash_info parts table
>> 
>> Describe your changes in imperative mood, e.g. "Add support for
>> S25LX256" instead of "This adds ..."
>> 
>> It may worth to re-read
>> https://www.kernel.org/doc/html/latest/process/submitting-patches.html
>> once in a while.
>> 
>>> 
>>> Signed-off-by: Nathan Barrett-Morrison <nathan.morrison@timesys.com>
>>> ---
>>>   drivers/mtd/spi-nor/issi.c | 32 ++++++++++++++++++++++++++++++++
>>>   1 file changed, 32 insertions(+)
>>> 
>>> diff --git a/drivers/mtd/spi-nor/issi.c b/drivers/mtd/spi-nor/issi.c
>>> index 89a66a19d754..362bc3603d8f 100644
>>> --- a/drivers/mtd/spi-nor/issi.c
>>> +++ b/drivers/mtd/spi-nor/issi.c
>>> @@ -29,6 +29,35 @@ static const struct spi_nor_fixups 
>>> is25lp256_fixups = {
>>>       .post_bfpt = is25lp256_post_bfpt_fixups,
>>>   };
>>> +static int
>>> +is25lx256_post_bfpt_fixups(struct spi_nor *nor,
>>> +               const struct sfdp_parameter_header *bfpt_header,
>>> +               const struct sfdp_bfpt *bfpt)
>>> +{
>>> +    /*
>>> +     * IS25LX256 supports both 1S-1S-8S and 1S-8S-8S.
>>> +     * However, the BFPT does not contain any information denoting 
>>> this
>>> +     * functionality, so the proper fast read opcodes are never 
>>> setup.
>>> +     * We're correcting this issue via the fixup below.  Page 
>>> program
>>> +     * commands are detected and setup properly via the 4BAIT 
>>> lookup.
>>> +     */
> 
> Why don't you set the READ support when parsing the 4bait table?

That would deviate from the read handling of all the other modes.

> We need to see the SFDP dump to determine how we treat this. I'm not
> sure a post_bfpt hook is the right thing to do for this flash.

See:
https://lore.kernel.org/linux-mtd/e07105d41ec62a6ee47ca0295ca347dc@walle.cc/

But yes, I missed that you should include the sfdp dump and some other
commands in the comment section of the patch which adds support for that
flash (see Tudors reply).

Tudor, what is the status of your documentation patch? I'd really like
to refer to the kernel docs instead of having write the same over and
over again ;)

-michael