Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Fri, 10 Sep 2021 08:57:30 +0000
From:   Charles Keepax <ckeepax@opensource.cirrus.com>
To:     Lucas Tanure <tanureal@opensource.cirrus.com>
CC:     Mark Brown <broonie@kernel.org>,
        Sanjay R Mehta <sanju.mehta@amd.com>,
        Nehal Bakulchandra Shah <Nehal-Bakulchandra.shah@amd.com>,
        <linux-kernel@vger.kernel.org>, <linux-spi@vger.kernel.org>,
        <patches@opensource.cirrus.com>
Subject: Re: [PATCH 4/4] spi: amd: Check for idle bus before execute opcode
Message-ID: <20210910085730.GX9223@ediswmail.ad.cirrus.com>
References: <20210909111005.304101-1-tanureal@opensource.cirrus.com>
 <20210909111005.304101-4-tanureal@opensource.cirrus.com>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20210909111005.304101-4-tanureal@opensource.cirrus.com>
User-Agent: Mutt/1.5.21 (2010-09-15)
Precedence: bulk

On Thu, Sep 09, 2021 at 12:10:05PM +0100, Lucas Tanure wrote:
> Check if the bus is not in use before starting the
> transfer
> 
> Signed-off-by: Lucas Tanure <tanureal@opensource.cirrus.com>
> ---
>  drivers/spi/spi-amd.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/spi/spi-amd.c b/drivers/spi/spi-amd.c
> index 97838b57871c..99b2b0ccff08 100644
> --- a/drivers/spi/spi-amd.c
> +++ b/drivers/spi/spi-amd.c
> @@ -115,11 +115,18 @@ static int amd_spi_busy_wait(struct amd_spi *amd_spi)
>  	return 0;
>  }
>  
> -static void amd_spi_execute_opcode(struct amd_spi *amd_spi)
> +static int amd_spi_execute_opcode(struct amd_spi *amd_spi)
>  {
> +	int ret;
> +
> +	ret = amd_spi_busy_wait(amd_spi);
> +	if (ret)
> +		return ret;
> +
>  	/* Set ExecuteOpCode bit in the CTRL0 register */
>  	amd_spi_setclear_reg32(amd_spi, AMD_SPI_CTRL0_REG, AMD_SPI_EXEC_CMD, AMD_SPI_EXEC_CMD);
> -	amd_spi_busy_wait(amd_spi);
> +
> +	return 0;
>  }

This feels like the commit message could use come additional
explanation. The message states we are moving the wait, but not
why? Also the original code looks slightly more logical, as
in amd_spi_fifo_xfer we read the receive buffer immediately after
calling this function.

Thanks,
Charles