| Message ID | 20231023140534.704312-4-ramona.gradinariu@analog.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: imu: adis: Use spi cs inactive delay | expand |
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@analog.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. - Nuno Sá
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@analog.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.
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@analog.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... Thanks! - Nuno Sá
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@analog.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
On 23/10/2023 16:05, 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@analog.com> > --- > changes in v2: > - added default value > - updated description You can drop the description from the property. > - updated commit message Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> Best regards, Krzysztof
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@analog.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 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 >
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@analog.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 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á
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. + interrupts: maxItems: 1
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@analog.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(+) -- 2.34.1