Message-ID: <e7a2438a-f6a3-439e-8058-937248dd5b3f@baylibre.com>
Date: Wed, 19 Jun 2024 15:36:41 -0500
From: David Lechner <dlechner@...libre.com>
To: Marcelo Schmitt <marcelo.schmitt1@...il.com>
Cc: Marcelo Schmitt <marcelo.schmitt@...log.com>, broonie@...nel.org,
 lars@...afoo.de, Michael.Hennerich@...log.com, jic23@...nel.org,
 robh+dt@...nel.org, krzysztof.kozlowski+dt@...aro.org, conor+dt@...nel.org,
 nuno.sa@...log.com, linux-iio@...r.kernel.org, devicetree@...r.kernel.org,
 linux-spi@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 1/6] spi: Enable controllers to extend the SPI protocol
 with MOSI idle configuration

On 6/19/24 1:58 PM, Marcelo Schmitt wrote:
> On 06/19, David Lechner wrote:
>> On 6/18/24 6:10 PM, Marcelo Schmitt wrote:
>>
>>

...

>>
>>> +the peripheral and also when CS is inactive.
>>
>> As I mentioned in a previous review, I think the key detail here is that the
>> MOSI line has to be in the required state during the CS line assertion
>> (falling edge). I didn't really get that from the current wording. The current
>> wording makes it sound like MOSI needs to be high indefinitely longer.
> 
> It may be that we only need MOSI high just before bringing CS low. Though,
> I don't see that info in the datasheets. How much time would MOSI be required
> to be high prior to bringing CS low? The timing diagrams for register access and
> ADC sampling in "3-wire" mode all start and end with MOSI at logical 1 (high).
> I think reg access work if MOSI is brought low after CS gets low, but sample
> read definitely don't work.
> 
> From the info available in datasheets, it looks like MOSI is indeed expected 
> to be high indefinitely amount of time. Except when the controller is clocking
> out data to the peripheral.
> 
> Even if find out the amount of time MOSI would be required high prior to CS low,
> then we would need some sort of MOSI high/low state set with a delay prior to
> active CS. That might be enough to support the AD4000 series of devices but,
> would it be worth the added complexity?
> 

It needs to happen at the same time as setting CPOL for the SCLK line for the
device that is about to have the CS asserted. So I don't think we are breaking
new ground here. Typically, in most datasheets I've seen they tend to say
something like 2 ns before the CS change. So in most cases, I don't think
anyone bothers adding a delay. But if a longer delay was really needed for
a specific peripheral, we could add a SPI xfer with no read/write that has
cs_off=1 and a delay to get the correct state of both MOSI and SCLK a longer
time before the CS change.

>>
>>> index e8e1e798924f..8e50a8559225 100644
>>> --- a/include/linux/spi/spi.h
>>> +++ b/include/linux/spi/spi.h
>>> @@ -599,6 +599,12 @@ struct spi_controller {
>>>  	 * assert/de-assert more than one chip select at once.
>>>  	 */
>>>  #define SPI_CONTROLLER_MULTI_CS		BIT(7)
>>> +	/*
>>> +	 * The spi-controller is capable of keeping the MOSI line low or high
>>> +	 * when not clocking out data.
>>> +	 */
>>> +#define SPI_CONTROLLER_MOSI_IDLE_LOW    BIT(8)  /* Can idle MOSI low */
>>> +#define SPI_CONTROLLER_MOSI_IDLE_HIGH   BIT(9)  /* Can idle MOSI high */
>>
>> I don't see where these are used anywhere else in the series. They
>> seem redundant with SPI_MOSI_IDLE_LOW and SPI_MOSI_IDLE_HIGH.
>>
> Good point.
> They are currently not being used.
> Comparing with what we have for SPI_CONTROLLER_MULTI_CS, I'm thinking it may be
> handy to have dt properties to indicate controller MOSI idle capabilities.
> Does that sound reasonable?

I don't see any properties in spi-controller.yaml related to
SPI_CONTROLLER_MULTI_CS. So I don't see how a property for
the idle capabilities would be useful there.

Powered by blists - more mailing lists