diff mbox series

[v2,3/3] dt-bindings: adis16460: Add 'spi-cs-inactive-delay-ns' property

Message ID 20231023140534.704312-4-ramona.gradinariu@analog.com
State New
Headers
Series iio: imu: adis: Use spi cs inactive delay |

Commit Message

Ramona Gradinariu Oct. 23, 2023, 2:05 p.m. UTC 
  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

Comments

Nuno Sá Oct. 23, 2023, 2:27 p.m. UTC | #1 
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á
Conor Dooley Oct. 23, 2023, 4:06 p.m. UTC | #2 
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.
Nuno Sá Oct. 24, 2023, 6:53 a.m. UTC | #3 
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á
Krzysztof Kozlowski Oct. 24, 2023, 1:47 p.m. UTC | #4 
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
Krzysztof Kozlowski Oct. 24, 2023, 1:48 p.m. UTC | #5 
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
Conor Dooley Oct. 24, 2023, 3:11 p.m. UTC | #6 
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
>
Nuno Sá Oct. 25, 2023, 7:41 a.m. UTC | #7 
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 mbox series

Patch

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