Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:2 as permitted sender) client-ip=2620:137:e000::3:2;
MIME-Version: 1.0
References: <20231026012017.518610-1-acelan.kao@canonical.com> <F56F4D7D-1264-41E1-9CE7-5DA410A22D73@walle.cc>
In-Reply-To: <F56F4D7D-1264-41E1-9CE7-5DA410A22D73@walle.cc>
From:   AceLan Kao <acelan.kao@canonical.com>
Date:   Thu, 26 Oct 2023 18:08:54 +0800
Message-ID: <CAFv23QmHvvib=E3RPtGXN-jAT9dXuZ6_-62xy0OjBYGRmrFPwg@mail.gmail.com>
Subject: Re: [PATCH v4] mtd: spi-nor: Improve reporting for software reset failures
To:     Michael Walle <michael@walle.cc>
Cc:     Tudor Ambarus <tudor.ambarus@linaro.org>,
        Pratyush Yadav <pratyush@kernel.org>,
        Miquel Raynal <miquel.raynal@bootlin.com>,
        Richard Weinberger <richard@nod.at>,
        Vignesh Raghavendra <vigneshr@ti.com>,
        Mika Westerberg <mika.westerberg@linux.intel.com>,
        linux-mtd@lists.infradead.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Michael Walle <michael@walle.cc> =E6=96=BC 2023=E5=B9=B410=E6=9C=8826=E6=97=
=A5 =E9=80=B1=E5=9B=9B =E4=B8=8B=E5=8D=882:28=E5=AF=AB=E9=81=93=EF=BC=9A
>
> Am 26. Oktober 2023 04:20:17 OESZ schrieb AceLan Kao <acelan.kao@canonica=
l.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@mi=
crochip.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@canonica=
l.com/T/#m7e5e7872151a913d5fe274fc20b7981bd10dd09f
2. https://lore.kernel.org/lkml/20231025030501.490355-1-acelan.kao@canonica=
l.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 *no=
r)
> >
> >       ret =3D spi_mem_exec_op(nor->spimem, &op);
> >       if (ret) {
> >-              dev_warn(nor->dev, "Software reset failed: %d\n", ret);
> >+              if (ret =3D=3D -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 struc=
t 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(m=
em->spi, 0)) {
> >               ret =3D spi_mem_access_start(mem);
>