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@xxxxxxxxxx> >>>> --- >>>> 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
