Re: [PATCH] dt-bindings: max310x: convert to YAML

[Krzysztof Kozlowski]

On 12/10/2023 15:59, Hugo Villeneuve wrote:
>>> +  clock-frequency:
>>> +    description:
>>> +      When there is no clock provider visible to the platform, this
>>> +      is the source crystal frequency for the IC in Hz.
>>> +    minimum: 1000000
>>> +    maximum: 4000000
>>
>> This wasn't in original binding. Explain this in the commit msg.
> 
> I will add the corresponding explanation in V2. 
> 
> The 'clock-frequency' property is already supported
> by the driver but was not documented in the original txt binding.
> 
> This is related to the commit d4d6f03c4fb3: serial: max310x: Try to
> get crystal clock rate from property (Author: Andy Shevchenko):
>     
>     In some configurations, mainly ACPI-based, the clock frequency of
>     the device is supplied by very well established 'clock-frequency'
>     property. Hence, try to get it from the property at last if no other
>     providers are available.

This does not justify adding it to bindings. ACPI stuff stays with ACPI.
Drop it.

> 
> 
>>
>>> +
>>> +  clock-names:
>>> +    enum:
>>> +      - xtal # External crystal
>>> +      - osc  # External clock source
>>
>> clock-names follow immediately clocks.
> 
> Will fix for V2.
> 
> 
>>
>>> +
>>> +  gpio-controller: true
>>> +
>>> +  "#gpio-cells":
>>> +    const: 2
>>> +
>>> +  gpio-line-names:
>>> +    minItems: 1
>>> +    maxItems: 16
>>> +
>>> +allOf:
>>
>> allOf: block goes after required: block.
> 
> Will fix for V2.
> 
> 
>>
>>> +  - $ref: /schemas/spi/spi-peripheral-props.yaml#
>>> +  - $ref: /schemas/serial/serial.yaml#
>>> +  - $ref: /schemas/serial/rs485.yaml#
>>> +
>>> +required:
>>> +  - compatible
>>> +  - reg
>>> +  - interrupts
>>> +
>>> +oneOf:
>>> +  - required:
>>> +      - clocks
>>> +      - clock-names
>>> +  - required:
>>> +      - clock-frequency
>>
>> That's also something new as well. The original binding required clocks.
>> Why are you changing this?
> 
> See explanation above about clock-frequency.
> 
> If clocks is not provided, than at least 'clock-frequency' must be
> provided.
> 
> 
>>> +
>>> +unevaluatedProperties: false
>>> +
>>> +examples:
>>> +  - |
>>> +    #include <dt-bindings/interrupt-controller/irq.h>
>>> +    i2c {
>>> +        #address-cells = <1>;
>>> +        #size-cells = <0>;
>>> +
>>> +        serial@2c {
>>> +            compatible = "maxim,max3107";
>>> +            reg = <0x2c>;
>>> +            clocks = <&xtal4m>;
>>> +            clock-names = "xtal";
>>> +            interrupt-parent = <&gpio3>;
>>> +            interrupts = <7 IRQ_TYPE_LEVEL_LOW>;
>>> +            gpio-controller;
>>> +            #gpio-cells = <2>;
>>> +        };
>>> +
>>
>> One example is enuogh. All other are the same.
> 
> Not really, clock-names is different for example 2 (osc), and example 3
> shows that 'clock-frequency' can be used if no clock is provided?

Difference by one property does not really justify new example. The
third case - clock frequency - is okay, but the property is going away.

Best regards,
Krzysztof