On Mon, Feb 14, 2022 at 10:20:21PM +0800, Yun Zhou wrote:
> I can't see from anywhere that when cs_change is true, we must keep CS
>
> active. If an individual controller needs to keep CS active after the whole
>
> message transmission complete, I think we should set cs_change to false
>
> rather than true, because cs_change means to change CS, not keep CS,
>
> otherwise let us rename cs_change to cs_keep.
*sigh* Please also look back at how this has historically been handled,
this is not new behaviour. I'm not saying that this is the greatest API
ever or that it'd be done this way if it were new but that doesn't mean
we can just randomly change the interface and potentially disrupt users.
Whatever else is going on the current behaviour is intentional.
On 2/14/22 10:36 PM, Mark Brown wrote:
> On Mon, Feb 14, 2022 at 10:20:21PM +0800, Yun Zhou wrote:
>
>> I can't see from anywhere that when cs_change is true, we must keep CS
>> active. If an individual controller needs to keep CS active after the whole
>> message transmission complete, I think we should set cs_change to false
>> rather than true, because cs_change means to change CS, not keep CS,
>> otherwise let us rename cs_change to cs_keep.
>
> *sigh* Please also look back at how this has historically been handled,
> this is not new behaviour.
From the first commit(8ae12a0d85987dc13) for spi subsystem, we can see
that:
- An spi_message is a sequence of of protocol operations, executed
as one atomic sequence. SPI driver controls include:
+ when bidirectional reads and writes start ... by how its
sequence of spi_transfer requests is arranged;
+ optionally defining short delays after transfers ... using
the spi_transfer.delay_usecs setting;
+ whether the chipselect becomes inactive after a transfer and
any delay ... by using the spi_transfer.cs_change flag;
+ hinting whether the next message is likely to go to this same
device ... using the spi_transfer.cs_change flag on the last
transfer in that atomic group, and potentially saving costs
for chip deselect and select operations
When we want chipselect to become inactive after a transfer completes,
should cs_change be true or false? Although it is not stated here, I
think it is obviously true, otherwise we should call it cs_keep not
cs_change.
> I'm not saying that this is the greatest API
> ever or that it'd be done this way if it were new but that doesn't mean
> we can just randomly change the interface and potentially disrupt users.
> Whatever else is going on the current behaviour is intentional.
>
Although the logic dealing with cs_change in spi_transfer_one_message()
has existed a long time and nobody reports issue on it, that doesn't
mean it is correct. I think the main reason is, cs_change is only used
to change chipselect inactive in the middle of message, and nobody set
it at the end of message. Even if the cs_change of the end of message is
set to true, probably there is no impact before
d347b4aaa1a042ea528e385d9070b74c77a14321, since no matter the chipselect
is active or inactive, we will set it to active before a new message.
Even if meet an issue, most of users think it is the incorrect usage
himself, and then the issue can be solved easily by clearing cs_change
of the end of message.
So there are several reasons why we must correct it:
1. We cannot accept an API with the name opposite to its actual performance.
2. The wrong cs_change and the lax last_cs_enable have caused serious bug.
3. My patch only affects the last transfer only when cs_change is true.
I can't think of anyone who will set a flag to complete operations with
opposite semantics.
On Wed, Feb 16, 2022 at 06:41:21PM +0800, Yun Zhou wrote:
> On 2/14/22 10:36 PM, Mark Brown wrote:
> > ever or that it'd be done this way if it were new but that doesn't mean
> > we can just randomly change the interface and potentially disrupt users.
> > Whatever else is going on the current behaviour is intentional.
> Although the logic dealing with cs_change in spi_transfer_one_message() has
> existed a long time and nobody reports issue on it, that doesn't mean it is
> correct. I think the main reason is, cs_change is only used to change
Please read and engage with what what I said above about not disrupting
existing users by just randomly changing this, silently changing how the
parameter operates will break any users that rely on the functionality
which is not going to help anyone and to the extent there is an issue
here it is only those users who would be affected in the first place.
This is not a productive discussion, please stop unless you have
concrete proposals that are considerate of existing users.