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