Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:5 as permitted sender) client-ip=2620:137:e000::3:5;
MIME-Version: 1.0
Date:   Tue, 24 Oct 2023 10:59:50 +0200
From:   Michael Walle <michael@walle.cc>
To:     AceLan Kao <acelan.kao@canonical.com>
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>,
        linux-mtd@lists.infradead.org, linux-kernel@vger.kernel.org,
        mika.westerberg@linux.intel.com
Subject: Re: [PATCH v2] mtd: spi-nor: Improve reporting for software reset
 failures
In-Reply-To: <20231024074332.462741-1-acelan.kao@canonical.com>
References: <20231024074332.462741-1-acelan.kao@canonical.com>
Message-ID: <4c11d06931f9b4e29c0e8feafc18e763@walle.cc>
Content-Type: text/plain; charset=US-ASCII;
 format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

[+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;
>  	}