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

[Conor Dooley <conor@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 2822 bytes --]

On Sun, Dec 17, 2023 at 11:52:27PM -0500, Abdel Alkuor wrote:
> as6200 is a temperature sensor with a range between -40°C to
> 125°C degrees and an accuracy of ±0.4°C degree between 0
> and 65°C and ±1°C for the other ranges.
> 
> Signed-off-by: Abdel Alkuor <alkuor@gmail.com>
> ---
> Changes in v2:
>   - Incorporate as6200 into lm75 bindings
> 
>  .../devicetree/bindings/hwmon/lm75.yaml        | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/hwmon/lm75.yaml b/Documentation/devicetree/bindings/hwmon/lm75.yaml
> index 0b69897f0c63..63b85a83ac18 100644
> --- a/Documentation/devicetree/bindings/hwmon/lm75.yaml
> +++ b/Documentation/devicetree/bindings/hwmon/lm75.yaml
> @@ -14,6 +14,7 @@ properties:
>    compatible:
>      enum:
>        - adi,adt75
> +      - ams,as6200
>        - atmel,at30ts74
>        - dallas,ds1775
>        - dallas,ds75
> @@ -48,6 +49,9 @@ properties:
>    vs-supply:
>      description: phandle to the regulator that provides the +VS supply
>  
> +  interrupts:
> +    maxItems: 1

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
 
 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.

> +
>  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?

Thanks,
Conor.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]