linux-kernel - Re: [PATCH v5 1/2] dt-bindings: hwmon: Add nct7802 bindings

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Sat, 9 Oct 2021 23:06:33 -0400
From:   Oskar Senft <osk@...gle.com>
To:     Guenter Roeck <linux@...ck-us.net>
Cc:     Jean Delvare <jdelvare@...e.com>, Rob Herring <robh+dt@...nel.org>,
        linux-hwmon@...r.kernel.org, linux-kernel@...r.kernel.org,
        devicetree@...r.kernel.org
Subject: Re: [PATCH v5 1/2] dt-bindings: hwmon: Add nct7802 bindings

Hi Guenter

Thanks again for your review!

> > Signed-off-by: Oskar Senft <osk@...gle.com>
> > ---
>
> change log goes here.
This might be a silly question: I'm using "git send-email", but I
don't think there's a way to edit the e-mail before it goes out. Do I
just add "---\n[Change log]" manually in the commit description?

> > +description: |
> > +  The NCT7802Y is a hardware monitor IC which supports one on-die and up to
> > +  5 remote temperature sensors with SMBus interface.
> > +
>
> Just noticed: 5 remote temperature sensors ? Shouldn't that be 3 ?
This includes 2 temperature sensors that are queried via PECI (i.e.
SMBus). I generated the description from the "general description"
section in the datasheet. I think the driver doesn't implement the 2
PECI sensors at this time, but the statement about the HW is still
true.

> > +      sensor-type:
> > +        items:
> > +          - enum:
> > +              - temperature
> > +              - voltage
> > +      temperature-mode:
> > +        items:
> > +          - enum:
> > +              - thermistor
> > +              - thermal-diode
> > +    required:
> > +      - reg
> > +      - sensor-type
>
> If I understand correctly, "temperature-mode" is implemented as mandatory
> for channels 1 and 2 if sensor-type is "temperature" (which makes sense).
> No idea though if it is possible to express that in yaml.
> If not, can it be mentioned as comment ?

After doing a bit more searching, I found the amazing "if: then:
else:" construct that allows to express this properly and eliminates
the code duplication. I'll follow up in PATCH v6.

Thanks
Oskar.



>
> > +
> > +  channel@2:
> > +    type: object
> > +    description: Remote Temperature Sensor or Voltage Sensor ("RTD2")
> > +    properties:
> > +      reg:
> > +        const: 2
> > +      sensor-type:
> > +        items:
> > +          - enum:
> > +              - temperature
> > +              - voltage
> > +      temperature-mode:
> > +        items:
> > +          - enum:
> > +              - thermistor
> > +              - thermal-diode
> > +    required:
> > +      - reg
> > +      - sensor-type
> > +
> > +  channel@3:
> > +    type: object
> > +    description: Remote Temperature Sensor or Voltage Sensor ("RTD3")
> > +    properties:
> > +      reg:
> > +        const: 3
> > +      sensor-type:
> > +        items:
> > +          - enum:
> > +              - temperature
> > +              - voltage
> > +    required:
> > +      - reg
> > +      - sensor-type
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    i2c {
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +
> > +        nct7802@28 {
> > +            compatible = "nuvoton,nct7802";
> > +            reg = <0x28>;
> > +
> > +            #address-cells = <1>;
> > +            #size-cells = <0>;
> > +
> > +            channel@0 { /* LTD */
> > +              reg = <0>;
> > +              status = "okay";
> > +            };
> > +
> > +            channel@1 { /* RTD1 */
> > +              reg = <1>;
> > +              status = "okay";
> > +              sensor-type = "temperature";
> > +              temperature-mode = "thermistor";
> > +            };
> > +
> > +            channel@2 { /* RTD2 */
> > +              reg = <2>;
> > +              status = "okay";
> > +              sensor-type = "temperature";
> > +              temperature-mode = "thermal-diode";
> > +            };
> > +
> > +            channel@3 { /* RTD3 */
> > +              reg = <3>;
> > +              status = "okay";
> > +              sensor-type = "voltage";
> > +            };
> > +        };
> > +    };
> >
>