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

[Krzysztof Kozlowski]

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.

> +
> +    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.

> +        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.

> +        $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