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

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

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