Re: [PATCH v2 08/14] spi: bcm63xx-hsspi: Handle cs_change correctly

From: William Zhang <william.zhang@broadcom.com>
To: Jonas Gorski <jonas.gorski@gmail.com>,
	Kursad Oney <kursad.oney@broadcom.com>
Cc: Linux SPI List <linux-spi@vger.kernel.org>,
	Broadcom Kernel List <bcm-kernel-feedback-list@broadcom.com>,
	tomer.yacoby@broadcom.com, dregan@mail.com, f.fainelli@gmail.com,
	anand.gore@broadcom.com, dan.beygelman@broadcom.com,
	joel.peshkin@broadcom.com, Mark Brown <broonie@kernel.org>,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 08/14] spi: bcm63xx-hsspi: Handle cs_change correctly
Date: Thu, 26 Jan 2023 19:10:22 -0800	[thread overview]
Message-ID: <f208f86e-06dd-5d46-071d-501968c773bc@broadcom.com> (raw)
In-Reply-To: <CAOiHx=nZBot4xuZ4aG6-4Ch18wNOK0Ud2+z7B2W7+YEc+U3DHg@mail.gmail.com>

[-- Attachment #1: Type: text/plain, Size: 2223 bytes --]


On 01/26/2023 09:33 AM, Jonas Gorski wrote:
> Hi Kursad,
> 
> On Thu, 26 Jan 2023 at 17:22, Kursad Oney <kursad.oney@broadcom.com> wrote:
>>>
>>> While there, you might also want to check the cs_off value(s) as well.
>>
>> Can you explain this please?
> 
> I'm talking about the transfer property cs_off:
> 
> " @cs_off: performs the transfer with chipselect off."
> 
> See how it is handled in the generic spi_transfer_one_message():
> 
>          spi_set_cs(msg->spi, !xfer->cs_off, false);
>          ...
>          list_for_each_entry(xfer, &msg->transfers, transfer_list) {
>                  ...
>                  if (xfer->cs_change) {
>                          if (list_is_last(&xfer->transfer_list,
>                                           &msg->transfers)) {
>                                  keep_cs = true;
>                          } else {
>                                  if (!xfer->cs_off)
>                                          spi_set_cs(msg->spi, false, false);
>                                  _spi_transfer_cs_change_delay(msg, xfer);
>                                  if (!list_next_entry(xfer,
> transfer_list)->cs_off)
>                                          spi_set_cs(msg->spi, true, false);
>                          }
>                  } else if (!list_is_last(&xfer->transfer_list,
> &msg->transfers) &&
>                             xfer->cs_off != list_next_entry(xfer,
> transfer_list)->cs_off) {
>                          spi_set_cs(msg->spi, xfer->cs_off, false);
>                  }
>                  ...
>        }
> 
> if we fix the cs_change handling, we might as well bring it up to state.
> 
We can blindly port this logic over but this cs_off stuff (from the 
spi.h comment @cs_off: performs the transfer with chipselect off) sounds 
weird.  What kind of device do transfer when cs is off? I don't have any 
device like this to test.

> In theory I would suggest to switch to implementing the set_cs() /
> transfer_one() so you could let the core take care of all of that, but
> that wouldn't work with dynamically switching to prepend mode. Might
> be something for v1.1 though.
> 
That is good idea and I can certainly try that.

> 
> Regards
> Jonas
> 

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4212 bytes --]