Re: [PATCH v2 2/3] dt-bindings: hwmon: Add binding for max6639

From: Guenter Roeck
Date: Tue Oct 11 2022 - 12:55:01 EST


On 10/11/22 09:28, Krzysztof Kozlowski wrote:
On 11/10/2022 06:47, Naresh Solanki wrote:
From: Marcello Sylvester Bauer <sylv@xxxxxxx>

Add Devicetree binding documentation for Maxim MAX6639 temperature
monitor with PWM fan-speed controller.

Signed-off-by: Marcello Sylvester Bauer <sylv@xxxxxxx>
Signed-off-by: Naresh Solanki <Naresh.Solanki@xxxxxxxxxxxxx>
---
.../bindings/hwmon/maxim,max6639.yaml | 116 ++++++++++++++++++
1 file changed, 116 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..bbefb0a57ab3
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/maxim,max6639.yaml
@@ -0,0 +1,116 @@
+# 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
+
+maintainers:
+ - Roland Stigge <stigge@xxxxxxxxx>
+
+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
+
+required:
+ - compatible
+ - reg
+
+patternProperties:
+ "^ot[0-1]_indication$":

No underscores in names.

+ type: boolean
+ default: false
+ description:
+ If true then enable OT pin indication.

Description copies the name of property. Not really useful. Describe
that it does.

Why this has 0 and 1 numbers? Isn't it connected with fan?


I had to look up the suggested code to understand what it means.
All the _indication properties actually configure the chip
to enable or disable various alarm output pins (ALERT, OT,
THERM, and FANFAIL). I for my part find the therm "indication"
quite confusing.

Guenter

+
+ "^therm[0-1]_indication$":
+ type: boolean
+ default: false
+ description:
+ If true then enable THERM pin indication.

Ditto

+
+ "^fan@[0-1]$":

[01]
The same in other cases.

+ type: object
+ description: |
+ Represents the two fans and their specific configuration.
+
+ $ref: fan-common.yaml#
+
+ properties:
+ reg:
+ description: |
+ The fan number.
+ items:
+ minimum: 0
+ maximum: 1
+
+ maxim,fan-spin-up:
+ type: boolean
+ description:
+ If true then whnever the fan starts up from zero drive, it

whenever
run spell-check

+ is driven with 100% duty cycle for 2s to ensure that it
+ starts.
+
+ maxim,full-speed-on-therm:
+ type: boolean
+ description:
+ If true then force fan to full speed if THERM pin goes low.
+
+ maxim,fanfail_indication:

No underscores

+ type: boolean
+ description:
+ If true then enable fanfail pin indication.

Missing blank line

+ required:
+ - reg
+
+additionalProperties: false
+
+examples:
+ - |
+ i2c {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ max6639@10 {

Node names should be generic.
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation

+ compatible = "maxim,max6639";
+ reg = <0x10>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ fan@0 {
+ reg = <0x0>;
+ pulses-per-revolution = <2>;
+ max-rpm = <4000>;
+ pwm-frequency = <25000>;
+ };
+
+ fan@1 {
+ reg = <0x1>;
+ pulses-per-revolution = <2>;
+ max-rpm = <32000>;
+ pwm-frequency = <25000>;
+ };
+ };
+ };
+...

Best regards,
Krzysztof