| Message ID | 20220217141234.72737-1-yun.zhou@windriver.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 6bb477df04366e0f69dd2f49e1ae1099069326bc |
| Headers | show |
| Series | [v2] spi: use specific last_cs instead of last_cs_enable | expand |
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; >
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.
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
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;
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(-)