Date:   Mon, 14 Feb 2022 20:35:33 +0800
From:   Yun Zhou <yun.zhou@...driver.com>
To:     Mark Brown <broonie@...nel.org>
Cc:     "linux-spi@...r.kernel.org" <linux-spi@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "Xue, Ying" <Ying.Xue@...driver.com>
Subject: Re: 回复: [PATCH] spi: disable chipselect after complete transfer

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.

Powered by blists - more mailing lists