[PATCH v2 0/3] iio: imu: adis: Use spi cs inactive delay

[PATCH v2 0/3] iio: imu: adis: Use spi cs inactive delay

[Ramona Gradinariu]

A delay is needed each time the chip selected becomes inactive,
even after burst data readings are performed.
Currently, there is no delay added after a burst reading
and in case a new SPI transfer is performed before
the needed delay, the adis device becomes unresponsive until
reset.

First patch adds the spi cs inactive delay in case it is not set and removes
the additional chip select change delay present in adis APIs (to avoid a
double delay).
Second and third patch updates dt-bindings for the drivers which are affected
by the first patch.

Ramona Gradinariu (3):
  iio: imu: adis: Use spi cs inactive delay
  dt-bindings: adis16475: Add 'spi-cs-inactive-delay-ns' property
  dt-bindings: adis16460: Add 'spi-cs-inactive-delay-ns' property

 .../bindings/iio/imu/adi,adis16460.yaml        |  6 ++++++
 .../bindings/iio/imu/adi,adis16475.yaml        |  6 ++++++
 drivers/iio/imu/adis.c                         | 18 ++++++------------
 3 files changed, 18 insertions(+), 12 deletions(-)

--
2.34.1

[PATCH v2 1/3] iio: imu: adis: Use spi cs inactive delay

[Ramona Gradinariu]

A delay is needed each time the chip selected becomes inactive,
even after burst data readings are performed.
Currently, there is no delay added after a burst reading
and in case a new SPI transfer is performed before
the needed delay, the adis device becomes unresponsive until
reset.

This commit is adding the needed delay directly to the spi driver,
using the cs_inactive parameter, in case it is not set and is
removing the additional chip select change delay present in adis
APIs to remove the double delay.

Signed-off-by: Ramona Gradinariu <ramona.gradinariu@analog.com>
---
 drivers/iio/imu/adis.c | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/drivers/iio/imu/adis.c b/drivers/iio/imu/adis.c
index bc40240b29e2..495caf4ce87a 100644
--- a/drivers/iio/imu/adis.c
+++ b/drivers/iio/imu/adis.c
@@ -44,8 +44,6 @@ int __adis_write_reg(struct adis *adis, unsigned int reg, unsigned int value,
 			.cs_change = 1,
 			.delay.value = adis->data->write_delay,
 			.delay.unit = SPI_DELAY_UNIT_USECS,
-			.cs_change_delay.value = adis->data->cs_change_delay,
-			.cs_change_delay.unit = SPI_DELAY_UNIT_USECS,
 		}, {
 			.tx_buf = adis->tx + 2,
 			.bits_per_word = 8,
@@ -53,8 +51,6 @@ int __adis_write_reg(struct adis *adis, unsigned int reg, unsigned int value,
 			.cs_change = 1,
 			.delay.value = adis->data->write_delay,
 			.delay.unit = SPI_DELAY_UNIT_USECS,
-			.cs_change_delay.value = adis->data->cs_change_delay,
-			.cs_change_delay.unit = SPI_DELAY_UNIT_USECS,
 		}, {
 			.tx_buf = adis->tx + 4,
 			.bits_per_word = 8,
@@ -62,8 +58,6 @@ int __adis_write_reg(struct adis *adis, unsigned int reg, unsigned int value,
 			.cs_change = 1,
 			.delay.value = adis->data->write_delay,
 			.delay.unit = SPI_DELAY_UNIT_USECS,
-			.cs_change_delay.value = adis->data->cs_change_delay,
-			.cs_change_delay.unit = SPI_DELAY_UNIT_USECS,
 		}, {
 			.tx_buf = adis->tx + 6,
 			.bits_per_word = 8,
@@ -144,8 +138,6 @@ int __adis_read_reg(struct adis *adis, unsigned int reg, unsigned int *val,
 			.cs_change = 1,
 			.delay.value = adis->data->write_delay,
 			.delay.unit = SPI_DELAY_UNIT_USECS,
-			.cs_change_delay.value = adis->data->cs_change_delay,
-			.cs_change_delay.unit = SPI_DELAY_UNIT_USECS,
 		}, {
 			.tx_buf = adis->tx + 2,
 			.bits_per_word = 8,
@@ -153,8 +145,6 @@ int __adis_read_reg(struct adis *adis, unsigned int reg, unsigned int *val,
 			.cs_change = 1,
 			.delay.value = adis->data->read_delay,
 			.delay.unit = SPI_DELAY_UNIT_USECS,
-			.cs_change_delay.value = adis->data->cs_change_delay,
-			.cs_change_delay.unit = SPI_DELAY_UNIT_USECS,
 		}, {
 			.tx_buf = adis->tx + 4,
 			.rx_buf = adis->rx,
@@ -163,8 +153,6 @@ int __adis_read_reg(struct adis *adis, unsigned int reg, unsigned int *val,
 			.cs_change = 1,
 			.delay.value = adis->data->read_delay,
 			.delay.unit = SPI_DELAY_UNIT_USECS,
-			.cs_change_delay.value = adis->data->cs_change_delay,
-			.cs_change_delay.unit = SPI_DELAY_UNIT_USECS,
 		}, {
 			.rx_buf = adis->rx + 2,
 			.bits_per_word = 8,
@@ -524,6 +512,12 @@ int adis_init(struct adis *adis, struct iio_dev *indio_dev,
 	}
 
 	mutex_init(&adis->state_lock);
+
+	if (!spi->cs_inactive.value) {
+		spi->cs_inactive.value = data->cs_change_delay;
+		spi->cs_inactive.unit = SPI_DELAY_UNIT_USECS;
+	}
+
 	adis->spi = spi;
 	adis->data = data;
 	iio_device_set_drvdata(indio_dev, adis);
-- 
2.34.1

[PATCH v2 2/3] dt-bindings: adis16475: Add 'spi-cs-inactive-delay-ns' property

[Ramona Gradinariu]

The devices supported by adis16475 driver require a stall period
between SPI transactions (during which the chip select is
inactive), with a minimum value equal to 16 microseconds, thus
adding '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,adis16475.yaml          | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/iio/imu/adi,adis16475.yaml b/Documentation/devicetree/bindings/iio/imu/adi,adis16475.yaml
index c73533c54588..135ccdd5c392 100644
--- a/Documentation/devicetree/bindings/iio/imu/adi,adis16475.yaml
+++ b/Documentation/devicetree/bindings/iio/imu/adi,adis16475.yaml
@@ -47,6 +47,12 @@ properties:
   spi-max-frequency:
     maximum: 2000000

+  spi-cs-inactive-delay-ns:
+    minimum: 16000
+    default: 16000
+    description:
+      Indicates the stall time between consecutive SPI transactions.
+
   interrupts:
     maxItems: 1

--
2.34.1

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

[Ramona Gradinariu]

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.
+
   interrupts:
     maxItems: 1

--
2.34.1

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

[Nuno Sá]

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á

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

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1708 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Nuno Sá]

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á

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

[Krzysztof Kozlowski]

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

Re: [PATCH v2 2/3] dt-bindings: adis16475: Add 'spi-cs-inactive-delay-ns' property

[Krzysztof Kozlowski]

On 23/10/2023 16:05, Ramona Gradinariu wrote:
> The devices supported by adis16475 driver require a stall period
> between SPI transactions (during which the chip select is
> inactive), with a minimum value equal to 16 microseconds, thus
> adding '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,adis16475.yaml          | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/iio/imu/adi,adis16475.yaml b/Documentation/devicetree/bindings/iio/imu/adi,adis16475.yaml
> index c73533c54588..135ccdd5c392 100644
> --- a/Documentation/devicetree/bindings/iio/imu/adi,adis16475.yaml
> +++ b/Documentation/devicetree/bindings/iio/imu/adi,adis16475.yaml
> @@ -47,6 +47,12 @@ properties:
>    spi-max-frequency:
>      maximum: 2000000
> 
> +  spi-cs-inactive-delay-ns:
> +    minimum: 16000
> +    default: 16000
> +    description:
> +      Indicates the stall time between consecutive SPI transactions.

You can skip the description entirely, it is coming from
spi-peripheral-props.

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

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

[Krzysztof Kozlowski]

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

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

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 3402 bytes --]

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
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Nuno Sá]

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á