Message ID | 89fb5eec30df734ee8fc58427cf5d94929076514.1702874115.git.alkuor@gmail.com (mailing list archive) |
---|---|
State | Superseded |
Headers | show |
Series | [v2,1/2] dt-bindings: hwmon: (lm75) Add AMS AS6200 temperature sensor | expand |
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
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 + 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>; + }; + };
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(+)