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

  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(+)
  

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.
  

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
  

On Tue, Dec 19, 2023 at 11:27:29AM -0500, Abdel Alkuor wrote:
> 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.

If multiple devices have the interrupt you should document it for all of
them IMO.
  

On Wed, Dec 20, 2023 at 04:25:15PM +0000, Conor Dooley wrote:
> On Tue, Dec 19, 2023 at 11:27:29AM -0500, Abdel Alkuor wrote:
> > 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:
> > >  
> > No, not all of them support the interrupt. Just tmp101, tmp102, tmp112, and as6200.
> > For now, I'll add the check for ams,as6200.
>
Hi Conor,
> If multiple devices have the interrupt you should document it for all of
> them IMO.

The interrupt hasn't been implemented for tmp101, tmp102 and tmp112 yet.
Should I still add them to the interrupt property? They might be two different
things driver and bindings, but I just wanted to make sure.

Thanks,
Abdel
  

On Wed, Dec 20, 2023 at 12:05:17PM -0500, Abdel Alkuor wrote:
> On Wed, Dec 20, 2023 at 04:25:15PM +0000, Conor Dooley wrote:
> > On Tue, Dec 19, 2023 at 11:27:29AM -0500, Abdel Alkuor wrote:
> > > 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:
> > > >  
> > > No, not all of them support the interrupt. Just tmp101, tmp102, tmp112, and as6200.
> > > For now, I'll add the check for ams,as6200.
> >
> Hi Conor,
> > If multiple devices have the interrupt you should document it for all of
> > them IMO.
> 
> The interrupt hasn't been implemented for tmp101, tmp102 and tmp112 yet.
> Should I still add them to the interrupt property? They might be two different
> things driver and bindings, but I just wanted to make sure.

I don't really care if the driver supports the interrupt on any of the
platforms (including the as6200), if the hardware has an interrupt the
binding should reflect that :)
  

On Wed, Dec 20, 2023 at 05:16:08PM +0000, Conor Dooley wrote:
> On Wed, Dec 20, 2023 at 12:05:17PM -0500, Abdel Alkuor wrote:
> > On Wed, Dec 20, 2023 at 04:25:15PM +0000, Conor Dooley wrote:
> > > On Tue, Dec 19, 2023 at 11:27:29AM -0500, Abdel Alkuor wrote:
> > > > 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:
> > > > >  
> > > > No, not all of them support the interrupt. Just tmp101, tmp102, tmp112, and as6200.
> > > > For now, I'll add the check for ams,as6200.
> > >
> > Hi Conor,
> > > If multiple devices have the interrupt you should document it for all of
> > > them IMO.
> > 
> > The interrupt hasn't been implemented for tmp101, tmp102 and tmp112 yet.
> > Should I still add them to the interrupt property? They might be two different
> > things driver and bindings, but I just wanted to make sure.
> 
> I don't really care if the driver supports the interrupt on any of the
> platforms (including the as6200), if the hardware has an interrupt the
> binding should reflect that :)

Understood. I'll add them in v4.

Abdel