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

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

[Yun Zhou]

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

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

[Yun Zhou]

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

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 967 bytes --]

On Fri, Feb 25, 2022 at 04:22:01PM +0800, Yun Zhou wrote:

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

Please don't send content free pings and please allow a reasonable time
for review.  People get busy, go on holiday, attend conferences and so 
on so unless there is some reason for urgency (like critical bug fixes)
please allow at least a couple of weeks for review.  If there have been
review comments then people may be waiting for those to be addressed.

Sending content free pings adds to the mail volume (if they are seen at
all) which is often the problem and since they can't be reviewed
directly if something has gone wrong you'll have to resend the patches
anyway, so sending again is generally a better approach though there are
some other maintainers who like them - if in doubt look at how patches
for the subsystem are normally handled.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Mark Brown]

On Thu, 17 Feb 2022 22:12:34 +0800, 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.
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next

Thanks!

[1/1] spi: use specific last_cs instead of last_cs_enable
      commit: 6bb477df04366e0f69dd2f49e1ae1099069326bc

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark