On Tue, Oct 24, 2023 at 03:47:16PM +0200, Krzysztof Kozlowski wrote: > 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 don't object to having the property, it'd just be good for the commit message to have mentioned that the minimum time may not be sufficient for all configurations. Cheers, Conor. > 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 >
Attachment:
signature.asc
Description: PGP signature
