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

[Shawn Anastasio]

On 11/29/23 3:23 AM, Krzysztof Kozlowski wrote:
> On 28/11/2023 22:00, Shawn Anastasio wrote:
>> The Sony Cronos Platform Controller CPLD is a multi-purpose platform
>> controller that provides both a watchdog timer and an LED controller for
>> the Sony Interactive Entertainment Cronos x86 server platform. As both
>> functions are provided by the same CPLD, a multi-function device is
>> exposed as the parent of both functions.
>>
>> Add a DT binding for this device.
>>
>> Signed-off-by: Shawn Anastasio <sanastasio@xxxxxxxxxxxxxxxxxxxxx>
>> ---
>> Changes in v2:
>>   - Change SIE to Sony to use the already-established prefix.
>>   - Clarify that Cronos is an x86 server platform in description
>>   - Drop #address-cells/#size-cells
>>   - Add missing additionalProperties to leds/watchdog objects
>>   - Add sony,led-mask property to leds object
>>   - Add sony,default-timeout property to watchdog object
>>   - Update example
>>
>>  .../bindings/mfd/sony,cronos-cpld.yaml        | 92 +++++++++++++++++++
>>  1 file changed, 92 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/mfd/sony,cronos-cpld.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/mfd/sony,cronos-cpld.yaml b/Documentation/devicetree/bindings/mfd/sony,cronos-cpld.yaml
>> new file mode 100644
>> index 000000000000..df2c2e83ccb4
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/mfd/sony,cronos-cpld.yaml
>> @@ -0,0 +1,92 @@
>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>> +# Copyright 2023 Raptor Engineering, LLC
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/mfd/sony,cronos-cpld.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Sony Cronos Platform Controller CPLD multi-function device
>> +
>> +maintainers:
>> +  - Timothy Pearson <tpearson@xxxxxxxxxxxxxxxxxxxxx>
>> +
>> +description: |
>> +  The Sony Cronos Platform Controller CPLD is a multi-purpose platform
>> +  controller that provides both a watchdog timer and an LED controller for the
>> +  Sony Interactive Entertainment Cronos x86 server platform. As both functions
>> +  are provided by the same CPLD, a multi-function device is exposed as the
>> +  parent of both functions.
>> +
>> +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.

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.

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

>> +        $ref: /schemas/types.yaml#/definitions/uint32
>> +        description: |
>> +          The default timeout with which the watchdog timer is initialized, in
>> +          seconds. Supported values are: 10, 20, 30, 40, 50, 60, 70, 80. All
>> +          other values will be rounded down to the nearest supported value.  The
>> +          default value is 80.
>> +
> 
> 
> 
> Best regards,
> Krzysztof
> 

Thanks,
Shawn