Re: [PATCH v2 1/2] dt-bindings: hwmon: Add max6639

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

Hi Krzysztof,


On 21-04-2023 01:07 pm, Krzysztof Kozlowski wrote:
> On 20/04/2023 13:17, Naresh Solanki wrote:
>> From: Marcello Sylvester Bauer <sylv@sylv.io>
>>
>> Add Devicetree binding documentation for Maxim MAX6639 temperature
>> monitor with PWM fan-speed controller.
>>
>> Signed-off-by: Marcello Sylvester Bauer <sylv@sylv.io>
>> Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com>
>> ...
> 
> Wrong syntax. '...' do not mean anything, it's ---
Ack
> 
>> Changes in V2:
>> - Update subject
>> - Drop blank lines
>> ---
>>   .../bindings/hwmon/maxim,max6639.yaml         | 52 +++++++++++++++++++
>>   1 file changed, 52 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/hwmon/maxim,max6639.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/hwmon/maxim,max6639.yaml b/Documentation/devicetree/bindings/hwmon/maxim,max6639.yaml
>> new file mode 100644
>> index 000000000000..1aaedfd7cee0
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/hwmon/maxim,max6639.yaml
>> @@ -0,0 +1,52 @@
>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/hwmon/maxim,max6639.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Maxim max6639
> 
> What is this device? fan controller?
Yes Fan controller.
Do you want me to update the title here as:
"Maxim MAC6639 2 channel fan controller & temperature monitor" ?

> 
>> +
>> +maintainers:
>> +  - Naresh Solanki <Naresh.Solanki@9elements.com>
>> +
>> +description: |
>> +  The MAX6639 is a 2-channel temperature monitor with dual, automatic, PWM
>> +  fan-speed controller.  It monitors its own temperature and one external
>> +  diode-connected transistor or the temperatures of two external diode-connected
>> +  transistors, typically available in CPUs, FPGAs, or GPUs.
>> +
>> +  Datasheets:
>> +    https://datasheets.maximintegrated.com/en/ds/MAX6639-MAX6639F.pdf
>> +
>> +properties:
>> +  compatible:
>> +    enum:
>> +      - maxim,max6639
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  '#address-cells':
>> +    const: 1
>> +
>> +  '#size-cells':
>> +    const: 0
> 
> Why do you need these two properties?
Ack. Will remove them.
> 
> Anyway, the binding looks incomplete. Where are the supplies? Interrupts?
This patch just adds basic support to the existing platform driver.
Intention is to be able to call the driver from DT with basic 
initialization from driver the existing driver.
> 
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    i2c {
>> +      #address-cells = <1>;
>> +      #size-cells = <0>;
>> +
>> +      fan-controller@10 {
>> +        compatible = "maxim,max6639";
>> +        reg = <0x10>;
>> +      };
>> +    };
>> +...
>>
>> base-commit: b8610e0fe08f1476895f141be60f8dd842f0adac
> 
> Best regards,
> Krzysztof
> 
Regards,
Naresh