Re: [PATCH v2] mtd: cfi_cmdset_0002: Use chip_ready() for write on S29GL064N

[Miquel Raynal <miquel.raynal@bootlin.com>]

Hello,

vigneshr@ti.com wrote on Tue, 15 Mar 2022 11:41:52 +0530:

> Hi Miquel,
> 
> On 06/03/22 9:02 pm, Tokunori Ikegami wrote:
> > The regression issue has been caused on S29GL064N and reported it.
> > Also the change mentioned is to use chip_good() for buffered write.
> > So disable the change on S29GL064N and use chip_ready() as before.
> > 
> > Fixes: dfeae1073583("mtd: cfi_cmdset_0002: Change write buffer to check correct value")
> > Signed-off-by: Tokunori Ikegami <ikegami.t@gmail.com>
> > Cc: Miquel Raynal <miquel.raynal@bootlin.com>
> > Cc: Richard Weinberger <richard@nod.at>
> > Cc: Vignesh Raghavendra <vigneshr@ti.com>
> > Cc: linux-mtd@lists.infradead.org
> > Cc: stable@vger.kernel.org
> > Link: https://lore.kernel.org/r/b687c259-6413-26c9-d4c9-b3afa69ea124@pengutronix.de/  
> 
> Could you please pull this in for v5.18-rc1 as this is a regression
> being tracked for a while?

I was about to apply the patch but reviewing it quickly shown a number
of issues. I can still pull the patch for -rc1 if all this is
corrected rather quickly but otherwise it will go through Fixes later
on.

- I just don't understand the commit log, please rephrase it entirely.
  You can include the link to the bug report as well inside the commit
  text, not as part of a Link tag, because the Link tag is here to
  refer to the mailing list discussion and is added automatically.
  Something like:

	As pointed out by this bug report [1], <what> is now broken on
	S29GL064N. The reason is that <explain the problem>. One way to
	solve the issue is to <explain why I should choose this
	solution>.

	[1] <the link here>

- The patch itself should be split into at least three parts:
  * The ID definition (should not be part of the fix).
  * The chip_check/chip_ready/chip_good_for_write code moves/renaming
    independently of any functinal change as a preparation patch.
  * The actual fix within one of the above functions.

Aside from these, I don't quite get what the __xipram keyword is
supposed to carry. We return a boolean expression stored in an int
provided by the map_word_equal() macro which does return a simple int
declared on the stack. It's not stored anywhere else than regular RAM,
or am I missing something?

> Acked-by: Vignesh Raghavendra <vigneshr@ti.com>
> 
> [...]
> 
> Regards
> Vignesh
> 

Thanks,
Miquèl

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