Re: [PATCH] spi: Fix warning for Clang build and simplify code

From: Nathan Chancellor
Date: Mon Feb 14 2022 - 10:23:14 EST


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@xxxxxxxxxx>
> Reported-by: kernel test robot <lkp@xxxxxxxxx>
> Reported-by: Nathan Chancellor <nathan@xxxxxxxxxx>
> Reported-by: Mark Brown <broonie@xxxxxxxxxx>
> Signed-off-by: Li-hao Kuo <lhjeff911@xxxxxxxxx>

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

Reviewed-by: Nathan Chancellor <nathan@xxxxxxxxxx>

> ---
> 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
>