linux-kernel - Re: [PATCH v2 1/2] dt-bindings: mfd: Add sony,cronos-cpld

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]

Message-ID: <b6a18d66-6022-4947-9616-53cfbb17e759@linaro.org>
Date:   Fri, 1 Dec 2023 09:04:35 +0100
From:   Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To:     Shawn Anastasio <sanastasio@...torengineering.com>,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        Rob Herring <robh+dt@...nel.org>,
        Conor Dooley <conor+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Lee Jones <lee@...nel.org>,
        Georgy Yakovlev <Georgy.Yakovlev@...y.com>
Cc:     Timothy Pearson <tpearson@...torengineering.com>
Subject: Re: [PATCH v2 1/2] dt-bindings: mfd: Add sony,cronos-cpld

On 01/12/2023 00:03, Shawn Anastasio wrote:
>>> +properties:
>>> +  compatible:
>>> +    const: sony,cronos-cpld
>>> +
>>> +  reg:
>>> +    maxItems: 1
>>> +
>>> +  leds:
>>> +    type: object
>>> +    description: Cronos Platform Status LEDs
>>
>> Missing ref to LEDs common bindings.
>>
> 
> Will fix.
> 
>>> +
>>> +    properties:
>>> +      compatible:
>>> +        const: sony,cronos-leds
>>> +
>>> +      sony,led-mask:
>>> +        $ref: /schemas/types.yaml#/definitions/uint32
>>
>> Why aren't you using LEDs bindings? A node for one property is otherwise
>> quite useless. I already commented on this last time.
>>
> 
> Our driver as-is doesn't support any of the properties in the LEDs
> common bindings, but it doesn't seem like there's anything that would
> preclude support in hardware, so this can be fixed.

Driver does not matter. We talk here about bindings, so about hardware,
not driver.

You must describe here hardware fully, not the driver.

> 
> Will use the LED bindings in v3.
> 
>>> +        minimum: 0x0
>>> +        maximum: 0x7fff
>>> +        description: |
>>> +          A bitmask that specifies which LEDs are present and can be controlled
>>> +          by the Cronos CPLD. Bits 0-5 correspond to platform Status LEDs, bits
>>> +          6-10 correspond to Link LEDs, and bits 11-14 correspond to the Power
>>> +          State LEDs. All other bits are unused. The default value is 0x7fff
>>> +          (all possible LEDs enabled).
>>> +
>>> +    additionalProperties: false
>>> +
>>> +  watchdog:
>>> +    type: object
>>> +    description: Cronos Platform Watchdog Timer
>>
>>
>>> +
>>> +    properties:
>>> +      compatible:
>>> +        const: sony,cronos-watchdog
>>> +
>>> +      sony,default-timeout:
>>
>> No, you must use existing bindings. Missing ref to watchdog and drop all
>> duplicated properties like this one.
>>
> 
> In this case the existing watchdog binding allows for arbitrary timeout
> values to be set, but the hardware only tolerates one of a few fixed
> values, enumerated below, which is why I felt it was appropriate to use
> a vendor-specific binding that documents the supported values.

You can narrow the accepted values.

> 
> Would you still prefer we ref to watchdog and just handle unsupported
> values in the driver by e.g. rounding or rejecting unsupported values?

It's not preference, it's a must.

Best regards,
Krzysztof