| Message ID | 45912ba25c34a63b8098f471c3c8ebf8857a4716.1584292056.git.mirq-linux@rere.qmqm.pl (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | spi: fix cs_change for last transfer | expand |
On Sun, Mar 15, 2020 at 06:08:53PM +0100, Michał Mirosław wrote: > Generic spi_transfer_one_message() implementation introduced in > commit b158935f70b9 has a bug in cs_change handling: it keeps CS > asserted when cs_change is set. Fix it. > struct spi_transfer *xfer; > - bool keep_cs = false; > + bool keep_cs = true; > int ret = 0; > struct spi_statistics *statm = &ctlr->statistics; > struct spi_statistics *stats = &msg->spi->statistics; > @@ -1268,7 +1268,7 @@ static int spi_transfer_one_message(struct spi_controller *ctlr, > if (xfer->cs_change) { > if (list_is_last(&xfer->transfer_list, > &msg->transfers)) { > - keep_cs = true; > + keep_cs = false; > } else { This is the opposite of the intended behaviour - we want to deassert chip select at the end of the message unless cs_change is set on the last transfer. If this were broken I would expect to see widespread problems being reported.
On Mon, Mar 16, 2020 at 12:17:50PM +0000, Mark Brown wrote: > On Sun, Mar 15, 2020 at 06:08:53PM +0100, Michał Mirosław wrote: > > Generic spi_transfer_one_message() implementation introduced in > > commit b158935f70b9 has a bug in cs_change handling: it keeps CS > > asserted when cs_change is set. Fix it. > > > struct spi_transfer *xfer; > > - bool keep_cs = false; > > + bool keep_cs = true; > > int ret = 0; > > struct spi_statistics *statm = &ctlr->statistics; > > struct spi_statistics *stats = &msg->spi->statistics; > > @@ -1268,7 +1268,7 @@ static int spi_transfer_one_message(struct spi_controller *ctlr, > > if (xfer->cs_change) { > > if (list_is_last(&xfer->transfer_list, > > &msg->transfers)) { > > - keep_cs = true; > > + keep_cs = false; > > } else { > > This is the opposite of the intended behaviour - we want to deassert > chip select at the end of the message unless cs_change is set on the > last transfer. If this were broken I would expect to see widespread > problems being reported. This is unfortunate naming I suppose. I reread the spi.h comments a few more times and it seems indeed, that .cs_change == 1 on last transfer means to a driver: "you may leave CS unchanged" - quite the reverse compared to non-last transfers. Please drop this patch then. Best Regards Michał Mirosław
On Mon, Mar 16, 2020 at 03:34:55PM +0100, Michał Mirosław wrote: > On Mon, Mar 16, 2020 at 12:17:50PM +0000, Mark Brown wrote: > > This is the opposite of the intended behaviour - we want to deassert > > chip select at the end of the message unless cs_change is set on the > > last transfer. If this were broken I would expect to see widespread > > problems being reported. > This is unfortunate naming I suppose. I reread the spi.h comments > a few more times and it seems indeed, that .cs_change == 1 on last > transfer means to a driver: "you may leave CS unchanged" - quite the > reverse compared to non-last transfers. cs_change also means that we should add an extra chip select transition on transfers other than the last. > Please drop this patch then. OK.
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c index 8994545367a2..5012eabde468 100644 --- a/drivers/spi/spi.c +++ b/drivers/spi/spi.c @@ -1206,7 +1206,7 @@ static int spi_transfer_one_message(struct spi_controller *ctlr, struct spi_message *msg) { struct spi_transfer *xfer; - bool keep_cs = false; + bool keep_cs = true; int ret = 0; struct spi_statistics *statm = &ctlr->statistics; struct spi_statistics *stats = &msg->spi->statistics; @@ -1268,7 +1268,7 @@ static int spi_transfer_one_message(struct spi_controller *ctlr, if (xfer->cs_change) { if (list_is_last(&xfer->transfer_list, &msg->transfers)) { - keep_cs = true; + keep_cs = false; } else { spi_set_cs(msg->spi, false); _spi_transfer_cs_change_delay(msg, xfer);
Generic spi_transfer_one_message() implementation introduced in commit b158935f70b9 has a bug in cs_change handling: it keeps CS asserted when cs_change is set. Fix it. Cc: stable@vger.kernel.org Fixes: b158935f70b9 ("spi: Provide common spi_message processing loop") Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl> --- drivers/spi/spi.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)