Re: [v2][PATCH] spi: use specific last_cs instead of last_cs_enable

[Yun Zhou <yun.zhou@windriver.com>]

Hi Mark,

Do you have any comments on the new patch? It just fixes the
regression introduced by d40f0b6f2e21, and it no longer affect cs_change.

Regards,
Yun
On 2/17/22 10:12 PM, Yun Zhou wrote:
> Commit d40f0b6f2e21 instroduced last_cs_enable to avoid setting
> chipselect if it's not necessary, but it also introduces a bug. The
> chipselect may not be set correctly on multi-device SPI busses. The
> reason is that we can't judge the chipselect by bool last_cs_enable,
> since chipselect may be modified after other devices were accessed.
> 
> So we should record the specific state of chipselect in case of
> confusion.
> 
> Signed-off-by: Yun Zhou <yun.zhou@windriver.com>
> ---
>   drivers/spi/spi.c       | 8 ++++++--
>   include/linux/spi/spi.h | 5 +++--
>   2 files changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> index 4599b121d744..d054229ffdda 100644
> --- a/drivers/spi/spi.c
> +++ b/drivers/spi/spi.c
> @@ -936,13 +936,14 @@ static void spi_set_cs(struct spi_device *spi, bool enable, bool force)
>   	 * Avoid calling into the driver (or doing delays) if the chip select
>   	 * isn't actually changing from the last time this was called.
>   	 */
> -	if (!force && (spi->controller->last_cs_enable == enable) &&
> +	if (!force && ((enable && spi->controller->last_cs == spi->chip_select) ||
> +				(!enable && spi->controller->last_cs != spi->chip_select)) &&
>   	    (spi->controller->last_cs_mode_high == (spi->mode & SPI_CS_HIGH)))
>   		return;
>   
>   	trace_spi_set_cs(spi, activate);
>   
> -	spi->controller->last_cs_enable = enable;
> +	spi->controller->last_cs = enable ? spi->chip_select : -1;
>   	spi->controller->last_cs_mode_high = spi->mode & SPI_CS_HIGH;
>   
>   	if ((spi->cs_gpiod || gpio_is_valid(spi->cs_gpio) ||
> @@ -2980,6 +2981,9 @@ int spi_register_controller(struct spi_controller *ctlr)
>   		goto free_bus_id;
>   	}
>   
> +	/* setting last_cs to -1 means no chip selected */
> +	ctlr->last_cs = -1;
> +
>   	status = device_add(&ctlr->dev);
>   	if (status < 0)
>   		goto free_bus_id;
> diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
> index 7ab3fed7b804..5a54ea354053 100644
> --- a/include/linux/spi/spi.h
> +++ b/include/linux/spi/spi.h
> @@ -373,7 +373,8 @@ extern struct spi_device *spi_new_ancillary_device(struct spi_device *spi, u8 ch
>    * @cur_msg_prepared: spi_prepare_message was called for the currently
>    *                    in-flight message
>    * @cur_msg_mapped: message has been mapped for DMA
> - * @last_cs_enable: was enable true on the last call to set_cs.
> + * @last_cs: the last chip_select that is recorded by set_cs, -1 on non chip
> + *           selected
>    * @last_cs_mode_high: was (mode & SPI_CS_HIGH) true on the last call to set_cs.
>    * @xfer_completion: used by core transfer_one_message()
>    * @busy: message pump is busy
> @@ -611,7 +612,7 @@ struct spi_controller {
>   	bool				auto_runtime_pm;
>   	bool                            cur_msg_prepared;
>   	bool				cur_msg_mapped;
> -	bool				last_cs_enable;
> +	char				last_cs;
>   	bool				last_cs_mode_high;
>   	bool                            fallback;
>   	struct completion               xfer_completion;
>