Received-SPF: pass (google.com: domain of linux-kernel+bounces-202921-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <5dcd9701-2725-4aff-9e73-d8f2e038be75@baylibre.com>
Date: Wed, 5 Jun 2024 11:37:48 -0500
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v3 1/6] spi: Add SPI mode bit for MOSI idle state
 configuration
To: Mark Brown <broonie@kernel.org>,
 Marcelo Schmitt <marcelo.schmitt@analog.com>
Cc: lars@metafoo.de, Michael.Hennerich@analog.com, jic23@kernel.org,
 robh+dt@kernel.org, krzysztof.kozlowski+dt@linaro.org, conor+dt@kernel.org,
 nuno.sa@analog.com, marcelo.schmitt1@gmail.com, linux-iio@vger.kernel.org,
 devicetree@vger.kernel.org, linux-spi@vger.kernel.org,
 linux-kernel@vger.kernel.org
References: <cover.1717539384.git.marcelo.schmitt@analog.com>
 <e1d5d57f7a7481c84f64a764f9898122e278739b.1717539384.git.marcelo.schmitt@analog.com>
 <0a716b10-0ae0-425f-919a-ea5d8b7975b6@sirena.org.uk>
Content-Language: en-US
From: David Lechner <dlechner@baylibre.com>
In-Reply-To: <0a716b10-0ae0-425f-919a-ea5d8b7975b6@sirena.org.uk>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit

On 6/5/24 7:24 AM, Mark Brown wrote:
> On Tue, Jun 04, 2024 at 07:41:47PM -0300, Marcelo Schmitt wrote:
> 
>> The behavior of an SPI controller data output line (SDO or MOSI or COPI
>> (Controller Output Peripheral Input) for disambiguation) is not specified
>> when the controller is not clocking out data on SCLK edges. However, there
>> exist SPI peripherals that require specific COPI line state when data is
>> not being clocked out of the controller.

I think this description is missing a key detail that the tx data line
needs to be high just before and also during the CS assertion at the start
of each message.

And it would be helpful to have this more detailed description in the
source code somewhere and not just in the commit message.

> 
> This is an optimisation for accelerating devices that need a specific
> value, really if these devices need a value they should send it.
> 
>>  #define SPI_MOSI_IDLE_LOW	_BITUL(17)	/* leave mosi line low when idle */
>> +#define SPI_MOSI_IDLE_HIGH	_BITUL(18)	/* leave mosi line high when idle */
> 
> Realistically we'll have a large set of drivers that are expecting the
> line to be held low so I'm not sure we need that option.  I would also
> expect to have an implementation of these options in the core which
> supplies buffers with the relevant data for use with controllers that
> don't have the feature (similar to how _MUST_TX and _MUST_RX are done).
> Even without that we'd need feature detection so that drivers that try
> to use this aren't just buggy when used with a controller that doesn't
> implement it, but once you're detecting you may as well just make things
> work.

I could see something like this working for controllers that leave the
tx data line in the state of the last bit of a write transfer. I.e. do a
write transfer of 0xff (using the smallest number of bits per word
supported by the controller) with CS not asserted, then assert CS, then
do the rest of actual the transfers requested by the peripheral.

But it doesn't seem like it would work for controllers that always
return the tx data line to a low state after a write since this would
mean that the data line would still be low during the CS assertion
which is what we need to prevent.