Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Date:   Mon, 14 Feb 2022 08:23:04 -0700
From:   Nathan Chancellor <nathan@kernel.org>
To:     Li-hao Kuo <lhjeff911@gmail.com>
Cc:     broonie@kernel.org, linux-spi@vger.kernel.org,
        linux-kernel@vger.kernel.org, wells.lu@sunplus.com,
        lh.kuo@sunplus.com, trix@redhat.com
Subject: Re: [PATCH] spi: Fix warning for Clang build and simplify code
Message-ID: <Ygpz2M9sH+vAKqNL@dev-arch.archlinux-ax161>
References: <7d91e6ce29f9a8df2c53a47b4b977664020e237a.1644805060.git.lhjeff911@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <7d91e6ce29f9a8df2c53a47b4b977664020e237a.1644805060.git.lhjeff911@gmail.com>
Precedence: bulk

On Mon, Feb 14, 2022 at 10:20:11AM +0800, Li-hao Kuo wrote:
> Clang build fails with
> spi-sunplus-sp7021.c:405:2: error: variable 'ret' is used
>   uninitialized whenever switch default is taken
>         default:
> 
> simplify code
> 
> Restore initializing ret. and add return error at default

This part of the commit message does not really match what the change
does anymore and I think that the "simplify code" part of the message
could be flushed out a little bit more, maybe something like:

"Hoist the calls to sp7021_spi_slave_tx() and sp7021_spi_slave_rx() into
the if statments and rewrite the if statements to explicitly allow only
unidirectional transfers."

?

> Fixes: 47e8fe57a66f ("spi: Modify irq request position and modify parameters")
> Reported-by: Tom Rix <trix@redhat.com>
> Reported-by: kernel test robot <lkp@intel.com>
> Reported-by: Nathan Chancellor <nathan@kernel.org>
> Reported-by: Mark Brown <broonie@kernel.org>
> Signed-off-by: Li-hao Kuo <lhjeff911@gmail.com>

Regardless, the actual change itself looks fine. Feel free to carry
this tag on future revisions:

Reviewed-by: Nathan Chancellor <nathan@kernel.org>

> ---
>  drivers/spi/spi-sunplus-sp7021.c | 38 +++++++++-----------------------------
>  1 file changed, 9 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/spi/spi-sunplus-sp7021.c b/drivers/spi/spi-sunplus-sp7021.c
> index ba5ed9f..ade7a0f 100644
> --- a/drivers/spi/spi-sunplus-sp7021.c
> +++ b/drivers/spi/spi-sunplus-sp7021.c
> @@ -69,12 +69,6 @@
>  #define SP7021_SPI_DATA_SIZE		(255)
>  #define SP7021_FIFO_DATA_LEN		(16)
>  
> -enum SP_SPI_MODE {
> -	SP7021_SLAVE_READ = 0,
> -	SP7021_SLAVE_WRITE = 1,
> -	SP7021_SPI_IDLE = 2,
> -};
> -
>  enum {
>  	SP7021_MASTER_MODE = 0,
>  	SP7021_SLAVE_MODE = 1,
> @@ -375,40 +369,26 @@ static int sp7021_spi_slave_transfer_one(struct spi_controller *ctlr, struct spi
>  {
>  	struct sp7021_spi_ctlr *pspim = spi_master_get_devdata(ctlr);
>  	struct device *dev = pspim->dev;
> -	int mode, ret;
> +	int ret;
>  
> -	mode = SP7021_SPI_IDLE;
> -	if (xfer->tx_buf && xfer->rx_buf) {
> -		dev_dbg(&ctlr->dev, "%s() wrong command\n", __func__);
> -		return -EINVAL;
> -	} else if (xfer->tx_buf) {
> +	if (xfer->tx_buf && !xfer->rx_buf) {
>  		xfer->tx_dma = dma_map_single(dev, (void *)xfer->tx_buf,
>  					      xfer->len, DMA_TO_DEVICE);
>  		if (dma_mapping_error(dev, xfer->tx_dma))
>  			return -ENOMEM;
> -		mode = SP7021_SLAVE_WRITE;
> -	} else if (xfer->rx_buf) {
> +		 ret = sp7021_spi_slave_tx(spi, xfer);
> +		 dma_unmap_single(dev, xfer->tx_dma, xfer->len, DMA_TO_DEVICE);
> +	} else if (xfer->rx_buf && !xfer->tx_buf) {
>  		xfer->rx_dma = dma_map_single(dev, xfer->rx_buf, xfer->len,
>  					      DMA_FROM_DEVICE);
>  		if (dma_mapping_error(dev, xfer->rx_dma))
>  			return -ENOMEM;
> -		mode = SP7021_SLAVE_READ;
> -	}
> -
> -	switch (mode) {
> -	case SP7021_SLAVE_WRITE:
> -		ret = sp7021_spi_slave_tx(spi, xfer);
> -		break;
> -	case SP7021_SLAVE_READ:
>  		ret = sp7021_spi_slave_rx(spi, xfer);
> -		break;
> -	default:
> -		break;
> -	}
> -	if (xfer->tx_buf)
> -		dma_unmap_single(dev, xfer->tx_dma, xfer->len, DMA_TO_DEVICE);
> -	if (xfer->rx_buf)
>  		dma_unmap_single(dev, xfer->rx_dma, xfer->len, DMA_FROM_DEVICE);
> +	} else {
> +		dev_dbg(&ctlr->dev, "%s() wrong command\n", __func__);
> +		return -EINVAL;
> +	}
>  
>  	spi_finalize_current_transfer(ctlr);
>  	return ret;
> -- 
> 2.7.4
>