Re: 回复: [PATCH] spi: disable chipselect after complete transfer

From: Yun Zhou
Date: Mon Feb 14 2022 - 07:36:02 EST

Hi Brown,

The focus of our differences is what the role of cs_change is.
I think the direct effect of cs_change is to change CS to inactive.
Here are some comments from spi.h:
1. Line 573, comments of 'transfer' operation in struct spi_controller:
 572      * + Chipselect stays active during the entire message
 573      *   (unless modified by spi_transfer.cs_change != 0).
It means that if cs_change != 0, chipselect will be changed to inactive.

2. Line 820, comments for struct spi_transfer:
 820  * @cs_change: affects chipselect after this transfer completes
 821  * @cs_change_delay: delay between cs deassert and assert when
 822  *      @cs_change is set and @spi_transfer is not the last in @spi_message
It says that the chipselect will be changed after this transfer completes.

3. Line 885, also the comments for struct spi_transfer:
 883  * All SPI transfers start with the relevant chipselect active.  Normally
 884  * it stays selected until after the last transfer in a message.  Drivers
 885  * can affect the chipselect signal using cs_change.
It means that we can use cs_change to make chipselect not active.

In the whole SPI subsystem, cs_change is written in several places, but only read
in one place: in function spi_transfer_one_message(), which is the only place
where cs_change comes into play.
1426     list_for_each_entry(xfer, &msg->transfers, transfer_list) {
...
1441             ret = ctlr->transfer_one(ctlr, msg->spi, xfer);
...
1484         if (xfer->cs_change) {
1485             if (list_is_last(&xfer->transfer_list,
1486                      &msg->transfers)) {
1487                 keep_cs = true;
1488             } else {
1489                 spi_set_cs(msg->spi, false, false);
1490                 _spi_transfer_cs_change_delay(msg, xfer);
1491                 spi_set_cs(msg->spi, true, false);
1492             }
1493         }
1495         msg->actual_length += xfer->len;
1496     }
spi_transfer_one_message() will process one or more transfers in the message
one by one. During this period, if cs_change is true, chipselect will be changed
to inactive and then be changed back to active(for next transfer), except the
last transfer as we no need to change chipselect back to active after the last
transfer. In other words, if cs_change is true, we will make an independent
transmission for each transfer.

I also investigated the role of cs_change in several drivers, the result is
similar, e.g. spi-mpc52xx-psc.c, spi-fsl-spi.c, spi-bcm63xx.c, spi-mpc52xx.c, etc.

To sum up, there is no indication and no requirement that when cs_change is
true, we need to keep chipselect active.

I hope you can seriously consider my analysis. If what I said is wrong, please
correct it.

Regards,

Yun

On 2/11/22 1:14 AM, Mark Brown wrote:
On Fri, Feb 11, 2022 at 01:01:20AM +0800, Yun Zhou wrote:

If there are multiple messages, and each message only has one xfer,
and the cs_change of each xfer is 1, during the transmission of the
messages, the CS will keep active even until at the end. This must be
unreasonable.
This is not something that most drivers are expected to use, cs_change
should only be being used at all for very unusual hardware and it should
be used even less frequently for the last transfer in a message. It is
fragile and anyone using it really needs to know what they're doing but
the feature is there.
Maybe it's not normal to set "cs_change" in the last xfer. However, in
most cases, SPI messages come from user space, and these messages may
I would question your use of "most" here...

come from multiple different applications. We can't make the whole
controller fail to work normally due to an inappropriate message of one
application.
This is one of the many hazards of using spidev, it is not an especially
safe or robust interface. To the extent that there's an issue here it's
something that should be addressed at the spidev level, though I expect
that there will be some users who want this facility and would want a
way to disable any access controls. I recommend writing device drivers
in kernel.

The feature predates me working on the SPI stack, the obvious examples
would be a device that doesn't actually use chip select where you want
to avoid all chip select changes or if you need to do some other actions
in the middle of a SPI transaction for some reason (which would need a
bunch of system level considerations to actually be safe/sensible like
making sure you're not sharing the SPI bus).
At present, if "cs_change" is not set, CS will be changed back to inactive
after the transmission is completed. If "cs_change" is set, CS will not
be changed. This obviously violates the definition of "cs_change".
No, it is exactly the specified behaviour of cs_change. Please see
spi.h.