linux-kernel - Re: [PATCH v2 1/2] dt-bindings: hwmon: (lm75) Add AMS AS6200 temperature sensor

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: Tue, 19 Dec 2023 11:27:29 -0500
From: Abdel Alkuor <alkuor@...il.com>
To: Conor Dooley <conor@...nel.org>
Cc: Jean Delvare <jdelvare@...e.com>, Guenter Roeck <linux@...ck-us.net>,
	Rob Herring <robh+dt@...nel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Jonathan Corbet <corbet@....net>, linux-hwmon@...r.kernel.org,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-doc@...r.kernel.org
Subject: Re: [PATCH v2 1/2] dt-bindings: hwmon: (lm75) Add AMS AS6200
 temperature sensor

On Tue, Dec 19, 2023 at 03:18:24PM +0000, Conor Dooley wrote:
> On Sun, Dec 17, 2023 at 11:52:27PM -0500, Abdel Alkuor wrote:
> 
> Do the other devices here have interrupts? If not, you just allowed
> interrupts for them. You, at the very least, need to add something like:
> diff --git a/Documentation/devicetree/bindings/hwmon/lm75.yaml b/Documentation/devicetree/bindings/hwmon/lm75.yaml
> index 63b85a83ac18..d7ce96606400 100644
> --- a/Documentation/devicetree/bindings/hwmon/lm75.yaml
> +++ b/Documentation/devicetree/bindings/hwmon/lm75.yaml
> @@ -56,6 +56,17 @@ required:
>    - compatible
>    - reg
>  
> +allOf:
> +  - if:
> +      not:
> +        properties:
> +          compatible:
> +            contains:
> +              const: ams,as6200
> +    then:
> +      properties:
> +        interrupts: false
> +
>  additionalProperties: false
>  
No, not all of them support the interrupt. Just tmp101, tmp102, tmp112, and as6200.
For now, I'll add the check for ams,as6200.
>  examples:
> 
> I had a brief look at the driver though, but I could not immediately
> tell if the interrupt was required on the ams,as6200 or if the driver
> continued on without that functionality. It seemed like an additional
> feature that the interrupt was required for, but if not you should make
> the interrupt required for the as6200.
> 
It is an additional feature. The interrupt basically notifies the user space when the
alarm state changes through temp1_alarm sysfs using poll on the file for example. That
being said, we should still be able to read the alarm state for as6200 without the
interrupt present.
> > +
> >  required:
> >    - compatible
> >    - reg
> > @@ -66,3 +70,17 @@ examples:
> >          vs-supply = <&vs>;
> >        };
> >      };
> > +  - |
> > +    #include <dt-bindings/interrupt-controller/irq.h>
> > +    i2c {
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +
> > +        temperature-sensor@48 {
> > +            compatible = "ams,as6200";
> > +            reg = <0x48>;
> > +            vs-supply = <&vs>;
> > +            interrupt-parent = <&gpio1>;
> > +            interrupts = <17 IRQ_TYPE_EDGE_BOTH>;
> > +        };
> > +    };
> 
> Can you make the indent here match that in the other example in this
> file please?
Sure. 

Thanks,
Abdel