[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <8391fb67-e575-49e6-9857-806cb85e9e59@linaro.org>
Date: Tue, 24 Oct 2023 15:47:16 +0200
From: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To: Nuno Sá <noname.nuno@...il.com>,
Conor Dooley <conor@...nel.org>
Cc: Ramona Gradinariu <ramona.gradinariu@...log.com>, jic23@...nel.org,
nuno.sa@...log.com, robh+dt@...nel.org,
krzysztof.kozlowski+dt@...aro.org, conor+dt@...nel.org,
linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org,
devicetree@...r.kernel.org
Subject: Re: [PATCH v2 3/3] dt-bindings: adis16460: Add
'spi-cs-inactive-delay-ns' property
On 24/10/2023 08:53, Nuno Sá wrote:
> On Mon, 2023-10-23 at 17:06 +0100, Conor Dooley wrote:
>> On Mon, Oct 23, 2023 at 04:27:48PM +0200, Nuno Sá wrote:
>>> On Mon, 2023-10-23 at 17:05 +0300, Ramona Gradinariu wrote:
>>>> The adis16460 device requires a stall time between SPI
>>>> transactions (during which the chip select is inactive),
>>>> with a minimum value equal to 16 microseconds.
>>>> This commit adds 'spi-cs-inactive-delay-ns' property, which should
>>>> indicate the stall time between consecutive SPI transactions.
>>>>
>>>> Signed-off-by: Ramona Gradinariu <ramona.gradinariu@...log.com>
>>>> ---
>>>> changes in v2:
>>>> - added default value
>>>> - updated description
>>>> - updated commit message
>>>> .../devicetree/bindings/iio/imu/adi,adis16460.yaml | 6 ++++++
>>>> 1 file changed, 6 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/iio/imu/adi,adis16460.yaml
>>>> b/Documentation/devicetree/bindings/iio/imu/adi,adis16460.yaml
>>>> index 4e43c80e5119..f10469b86ee0 100644
>>>> --- a/Documentation/devicetree/bindings/iio/imu/adi,adis16460.yaml
>>>> +++ b/Documentation/devicetree/bindings/iio/imu/adi,adis16460.yaml
>>>> @@ -25,6 +25,12 @@ properties:
>>>>
>>>> spi-cpol: true
>>>>
>>>> + spi-cs-inactive-delay-ns:
>>>> + minimum: 16000
>>>> + default: 16000
>>>> + description:
>>>> + Indicates the stall time between consecutive SPI transactions.
>>>> +
>>>
>>> You should drop the description...
>>>
>>> Also, give more time before posting a v2 so others get a chance to review
>>> your
>>> patches. It's also better for you since you can gather more change requests.
>>
>> Further, I don't see an answer to Krzysztof's question of why the stall
>> time would not just be set to 16,000 ns in the driver, based on the
>> compatible.
>
> Hi Conor,
>
> Regarding that, I'm the one to blame since I was the one asking for the property
> during internal review... The reason is that "spi-cs-inactive-delay-ns" is
> already part of spi-peripheral-props.yaml which we already reference. So my
> question would be why not using it?
>
> These devices are a bit sensitive regarding these timings. Not in devices
> supported by this driver but I already experienced having to set timings bigger
> than defined in the datasheet for spi to be reliable. this was true on a RPI but
> might not be in another platform.
>
> Hence having the flexibility to change the time in an already supported property
> does sound good to me. If not set, we still use the default value based on the
> compatible. Now, if you tell me "let's just add this if we really get the need
> for it", I get it but I also don't understand why not add it now...
>
I think it is okay to document specific SPI peripheral constraints in
each device. Just like we document sometimes SPI frequency. The v1 did
not explain this, but I see in this commit msg some rationale.
Best regards,
Krzysztof
Powered by blists - more mailing lists