Messages in this thread | | | From | AceLan Kao <> | Date | Thu, 26 Oct 2023 18:08:54 +0800 | Subject | Re: [PATCH v4] mtd: spi-nor: Improve reporting for software reset failures |
| |
Michael Walle <michael@walle.cc> 於 2023年10月26日 週四 下午2:28寫道: > > Am 26. Oktober 2023 04:20:17 OESZ schrieb AceLan Kao <acelan.kao@canonical.com>: > >From: "Chia-Lin Kao (AceLan)" <acelan.kao@canonical.com> > > > >When the software reset command isn't supported, we now report it > >as an informational message(dev_info) instead of a warning(dev_warn). > >This adjustment helps avoid unnecessary alarm and confusion regarding > >software reset capabilities. > > > >Signed-off-by: Chia-Lin Kao (AceLan) <acelan.kao@canonical.com> > > NAK. You surely missed my comments on the previous version.
Hi Michael,
It's strange that I didn't receive your first reply, and I just checked it from web archive[1]. I quote your opinions and reply them below.
> It bothers me that we use ENOTSUPP here. We should really use > > EOPNOTSUPP. > The core uses EOPNOTSUPP everywhere except for the intel things. > > Please have a look at changing that to EOPNOTSUPP. See also: > https://lore.kernel.org/linux-mtd/85f9c462-c155-dc17-dc97-3254acfa55d2@microchip.com/ Yes, this has been done in v3
> I'm not sure this is helpful. It's only the intel SPI controller which > does magic things (instead of just issuing our commands). Mika, do you > know wether your controller will do a reset on it's own? I presume so, > because AFAIR you have some kind of high level controller which also > does > SFDP parsing and read opcode handling on their own. Mika's replied you, and I think even if intel SPI controller do the magic things, the error message is still annoying.
> I'd leave that as is, because how are the chances that the first one is > supported and the second command, isn't? > When working with the intel controller, we'll return early after the > first spi_mem_exec_op(). Yes, this has been done in v3.
And then I checked again and found you have replied[2] the v3 patch and I still didn't recevie that one, too. Now I got your point, will revise my patch and submit v5 soon. Thanks.
1. https://lore.kernel.org/lkml/20231024074332.462741-1-acelan.kao@canonical.com/T/#m7e5e7872151a913d5fe274fc20b7981bd10dd09f 2. https://lore.kernel.org/lkml/20231025030501.490355-1-acelan.kao@canonical.com/T/#u > > -michael > > >--- > >v2. only lower the priority for the not supported failure > >v3. replace ENOTSUPP with EOPNOTSUPP and check the first command only > >v4. move the version information below the '---' line > >--- > > drivers/mtd/spi-nor/core.c | 5 ++++- > > drivers/spi/spi-mem.c | 2 +- > > 2 files changed, 5 insertions(+), 2 deletions(-) > > > >diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c > >index 1b0c6770c14e..42e52af76289 100644 > >--- a/drivers/mtd/spi-nor/core.c > >+++ b/drivers/mtd/spi-nor/core.c > >@@ -3252,7 +3252,10 @@ static void spi_nor_soft_reset(struct spi_nor *nor) > > > > ret = spi_mem_exec_op(nor->spimem, &op); > > if (ret) { > >- dev_warn(nor->dev, "Software reset failed: %d\n", ret); > >+ if (ret == -EOPNOTSUPP) > >+ dev_info(nor->dev, "Software reset enable command doesn't support: %d\n", ret); > >+ else > >+ dev_warn(nor->dev, "Software reset failed: %d\n", ret); > > return; > > } > > > >diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c > >index edd7430d4c05..93b77ac0b798 100644 > >--- a/drivers/spi/spi-mem.c > >+++ b/drivers/spi/spi-mem.c > >@@ -323,7 +323,7 @@ int spi_mem_exec_op(struct spi_mem *mem, const struct spi_mem_op *op) > > return ret; > > > > if (!spi_mem_internal_supports_op(mem, op)) > >- return -ENOTSUPP; > >+ return -EOPNOTSUPP; > > > > if (ctlr->mem_ops && ctlr->mem_ops->exec_op && !spi_get_csgpiod(mem->spi, 0)) { > > ret = spi_mem_access_start(mem); >
| |