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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 24 Oct 2023 16:11:35 +0100
From:   Conor Dooley <conor@...nel.org>
To:     Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
Cc:     Nuno Sá <noname.nuno@...il.com>,
        Ramona Gradinariu <ramona.gradinariu@...log.com>,
        jic23@...nel.org, nuno.sa@...log.com, robh+dt@...nel.org,
        krzysztof.kozlowski+dt@...aro.org, conor+dt@...nel.org,
        linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org,
        devicetree@...r.kernel.org
Subject: Re: [PATCH v2 3/3] dt-bindings: adis16460: Add
 'spi-cs-inactive-delay-ns' property

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@...log.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
> 

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)