On Tue, 2023-10-24 at 16:11 +0100, Conor Dooley wrote: > 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. > Fair enough... Thanks! - Nuno Sá
