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

[Abdel Alkuor]

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