Re: [PATCH v8 1/2] dt-bindings: mfd: Add bindings for MAX5970 and MAX5978

[Naresh Solanki <naresh.solanki@9elements.com>]

Hi Krzysztof,

On 04-11-2022 03:19 am, Krzysztof Kozlowski wrote:
> On 03/11/2022 17:34, Naresh Solanki wrote:
>> From: Marcello Sylvester Bauer <sylv@sylv.io>
>>
> 
> Subject: drop redundant (second) word "bindings"
> 
>> The MAX597x is a hot swap controller with configurable fault protection.
>> It also has 10bit ADC for current & voltage measurements.
>>
>> Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>
>> Signed-off-by: Marcello Sylvester Bauer <sylv@sylv.io>
>> Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com>
> 
> Isn't this third version per day? That's too many. Please keep it one
> per day (usually).
> 
> 
>> ---
>>   .../bindings/mfd/maxim,max5970.yaml           | 170 ++++++++++++++++++
>>   1 file changed, 170 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/mfd/maxim,max5970.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/mfd/maxim,max5970.yaml b/Documentation/devicetree/bindings/mfd/maxim,max5970.yaml
>> new file mode 100644
>> index 000000000000..25079c518f68
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/mfd/maxim,max5970.yaml
>> @@ -0,0 +1,170 @@
>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/mfd/maxim,max5970.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Regulator for MAX5970 smart switch from Maxim Integrated.
>> +
>> +maintainers:
>> +  - Patrick Rudolph <patrick.rudolph@9elements.com>
>> +
>> +description: |
>> +  The smart switch provides no output regulation, but independent fault protection
>> +  and voltage and current sensing.
>> +  Programming is done through I2C bus.
>> +
>> +  Datasheets:
>> +    https://datasheets.maximintegrated.com/en/ds/MAX5970.pdf
>> +    https://datasheets.maximintegrated.com/en/ds/MAX5978.pdf
>> +
>> +properties:
>> +  compatible:
>> +    enum:
>> +      - maxim,max5970
>> +      - maxim,max5978
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  interrupts:
>> +    maxItems: 1
>> +
>> +  leds:
>> +    type: object
>> +    description:
>> +      Properties for single channel.
> 
> This description seems odd. You have here several leds, so what is the
> single channel?
> 
There are four leds. Will update in next revision.
>> +
>> +    properties:
>> +      "#address-cells":
>> +        const: 1
>> +
>> +      "#size-cells":
>> +        const: 0
>> +
>> +    patternProperties:
>> +      "^led@[0-3]$":
>> +        $ref: /schemas/leds/common.yaml#
>> +        type: object
>> +
>> +    additionalProperties: false
>> +
>> +  vss1-supply:
>> +    description: Supply of the first channel.
>> +
>> +  vss2-supply:
>> +    description: Supply of the first channel.
>> +
>> +  "#io-channel-cells":
>> +    const: 1
>> +
>> +  regulators:
>> +    type: object
>> +    description:
>> +      Properties for single channel.
> 
> That's another odd description.
> 
Update. Willbe part of next version.
>> +
>> +    patternProperties:
>> +      "^(SW[0-1])$":
> 
> This should be lowercase. You also do not need ().
> 
Done.
>> +        $ref: /schemas/regulator/regulator.yaml#
>> +        type: object
>> +
>> +      shunt-resistor-micro-ohms:
>> +        description: |
>> +          The value of curent sense resistor in microohms.
>> +          Must be specified for each channel.
> 
> Drop last sentence and instead add "required:" with proper indent.
> 
Fixed. Will be part of next version.
>> +
>> +    additionalProperties: false
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - regulators
>> +  - vss1-supply
>> +
>> +allOf:
>> +  - if:
>> +      properties:
>> +        compatible:
>> +          enum:
>> +            - maxim,max5970
>> +    then:
>> +      properties:
>> +        io-channels:
>> +          items:
>> +            - description: voltage first channel
>> +            - description: current first channel
>> +            - description: voltage second channel
>> +            - description: current second channel
>> +          description: |
>> +            Voltage and current for first and second channel.
> 
> I do not understand the description.
> 
> Also, add it in the existing example.
> 
Have removed them because not being used by the driver.

>> +      required:
>> +        - vss2-supply
>> +    else:
>> +      properties:
>> +        io-channels:
>> +          items:
>> +            - description: voltage first channel
>> +            - description: current first channel
>> +          description: |
>> +            Voltage and current for first channel.
> 
> Same question for the description.
> 
>> +
>> +additionalProperties: false
>> +
> 
> Best regards,
> Krzysztof
>