| Message ID | 20231024074332.462741-1-acelan.kao@canonical.com |
|---|---|
| State | Superseded |
| Delegated to: | Ambarus Tudor |
| Headers | show |
| Series | [v2] mtd: spi-nor: Improve reporting for software reset failures | expand |
[+Mika] > 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. > > v2. only lower the priority for the not supported failure > > Signed-off-by: Chia-Lin Kao (AceLan) <acelan.kao@canonical.com> > --- > drivers/mtd/spi-nor/core.c | 10 ++++++++-- > 1 file changed, 8 insertions(+), 2 deletions(-) > > diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c > index 1b0c6770c14e..76920dbc568b 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 == -ENOTSUPP) 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/ > + dev_info(nor->dev, "Software reset enable command doesn't support: > %d\n", ret); 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. > + else > + dev_warn(nor->dev, "Software reset failed: %d\n", ret); > return; > } > > @@ -3262,7 +3265,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 == -ENOTSUPP) > + dev_info(nor->dev, "Software reset command doesn't support: %d\n", > ret); 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(). -michael > + else > + dev_warn(nor->dev, "Software reset failed: %d\n", ret); > return; > }
Hi, On Tue, Oct 24, 2023 at 10:59:50AM +0200, Michael Walle wrote: > [+Mika] > > > > 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. > > > > v2. only lower the priority for the not supported failure > > > > Signed-off-by: Chia-Lin Kao (AceLan) <acelan.kao@canonical.com> > > --- > > drivers/mtd/spi-nor/core.c | 10 ++++++++-- > > 1 file changed, 8 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c > > index 1b0c6770c14e..76920dbc568b 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 == -ENOTSUPP) > > 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/ Makes sense. > > + dev_info(nor->dev, "Software reset enable command doesn't support: > > %d\n", ret); > > 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. Yes, that's right.
diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c index 1b0c6770c14e..76920dbc568b 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 == -ENOTSUPP) + 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; } @@ -3262,7 +3265,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 == -ENOTSUPP) + dev_info(nor->dev, "Software reset command doesn't support: %d\n", ret); + else + dev_warn(nor->dev, "Software reset failed: %d\n", ret); return; }