diff mbox series

[4/4] dt-bindings: hwmon: Add Amphenol ChipCap 2

Message ID 20231020-topic-chipcap2-v1-4-087e21d4b1ed@gmail.com
State Superseded
Headers show
Series hwmon: Add support for Amphenol ChipCap 2 | expand

Checks

Context Check Description
robh/checkpatch success
robh/patch-applied success
robh/dtbs-check warning build log
robh/dt-meta-schema success

Commit Message

Javier Carrasco Nov. 8, 2023, 12:29 p.m. UTC
Add device tree bindings and an example for the ChipCap 2 humidity
and temperature sensor.

Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
---
 .../bindings/hwmon/amphenol,chipcap2.yaml          | 72 ++++++++++++++++++++++
 1 file changed, 72 insertions(+)

Comments

Krzysztof Kozlowski Nov. 8, 2023, 12:34 p.m. UTC | #1
On 08/11/2023 13:29, Javier Carrasco wrote:
> Add device tree bindings and an example for the ChipCap 2 humidity
> and temperature sensor.
> 
> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
> ---

...

> +maintainers:
> +  - Javier Carrasco <javier.carrasco.cruz@gmail.com>
> +
> +description: |
> +  Relative humidity and temperature sensor on I2C bus.
> +
> +  Datasheets:
> +    https://www.amphenol-sensors.com/en/telaire/humidity/527-humidity-sensors/3095-chipcap-2
> +
> +properties:
> +  compatible:
> +    enum:
> +      - amphenol,cc2dxx
> +      - amphenol,cc2dxxs

What does xx stand for? Wildcard? I do not see cc2dxx in part numbers.
We expect specific compatibles, not generic. What are the differences
between all parts?

> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 3
> +    description: |
> +      The device provides three optional interrupts. READY indicates that
> +      a measurement was finished. LOW indicates a low humidity alarm and
> +      HIGH a high humidity alarm.
> +      All interrupts must be IRQ_TYPE_RISING_EDGE.

Instead use items: with description: for each item.

> +
> +  interrupt-names:
> +    items:
> +      - const: READY
> +      - const: LOW
> +      - const: HIGH

Lowercase names

> +
> +  vdd-supply:
> +    description:
> +      Dedicated, controllable supply-regulator to reset the device and
> +      enter in command mode. If defined, it must provide a GPIO for its
> +      control.

I don't understand what GPIO has anything to do with power supply.

> +      If not defined, no alarms will be available.
> +
> +

Only one blank line.

> +required:
> +  - compatible
> +  - reg
> +
> +additionalProperties: false

Best regards,
Krzysztof
Javier Carrasco Nov. 8, 2023, 12:44 p.m. UTC | #2
Hello,

On 08.11.23 13:34, Krzysztof Kozlowski wrote:
> On 08/11/2023 13:29, Javier Carrasco wrote:
>> Add device tree bindings and an example for the ChipCap 2 humidity
>> and temperature sensor.
>>
>> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
>> ---
> 
> ...
> 
>> +maintainers:
>> +  - Javier Carrasco <javier.carrasco.cruz@gmail.com>
>> +
>> +description: |
>> +  Relative humidity and temperature sensor on I2C bus.
>> +
>> +  Datasheets:
>> +    https://www.amphenol-sensors.com/en/telaire/humidity/527-humidity-sensors/3095-chipcap-2
>> +
>> +properties:
>> +  compatible:
>> +    enum:
>> +      - amphenol,cc2dxx
>> +      - amphenol,cc2dxxs
> 
> What does xx stand for? Wildcard? I do not see cc2dxx in part numbers.
> We expect specific compatibles, not generic. What are the differences
> between all parts?
> 
There are two device families: cc2dxx and cc2dxxs, where xx indicates
the voltage and the accuracy. That does not change how the devices works
and it is not relevant for the driver. The 's' indicates that it is a
sleep device, and that modifies how it works.
I listed the supported part numbers in the hwmon documentation, where
they are also divided into these two families.
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  interrupts:
>> +    maxItems: 3
>> +    description: |
>> +      The device provides three optional interrupts. READY indicates that
>> +      a measurement was finished. LOW indicates a low humidity alarm and
>> +      HIGH a high humidity alarm.
>> +      All interrupts must be IRQ_TYPE_RISING_EDGE.
> 
> Instead use items: with description: for each item.
> 
>> +
>> +  interrupt-names:
>> +    items:
>> +      - const: READY
>> +      - const: LOW
>> +      - const: HIGH
> 
> Lowercase names
> 
>> +
>> +  vdd-supply:
>> +    description:
>> +      Dedicated, controllable supply-regulator to reset the device and
>> +      enter in command mode. If defined, it must provide a GPIO for its
>> +      control.
> 
> I don't understand what GPIO has anything to do with power supply.
> 
>> +      If not defined, no alarms will be available.
>> +
>> +
> 
> Only one blank line.
> 
>> +required:
>> +  - compatible
>> +  - reg
>> +
>> +additionalProperties: false
> 
> Best regards,
> Krzysztof
> 
Thanks for your review.

Best regards,
Javier Carrasco
Krzysztof Kozlowski Nov. 9, 2023, 8:41 a.m. UTC | #3
On 08/11/2023 13:44, Javier Carrasco wrote:

>>> +properties:
>>> +  compatible:
>>> +    enum:
>>> +      - amphenol,cc2dxx
>>> +      - amphenol,cc2dxxs
>>
>> What does xx stand for? Wildcard? I do not see cc2dxx in part numbers.
>> We expect specific compatibles, not generic. What are the differences
>> between all parts?
>>
> There are two device families: cc2dxx and cc2dxxs, where xx indicates
> the voltage and the accuracy. That does not change how the devices works
> and it is not relevant for the driver. The 's' indicates that it is a
> sleep device, and that modifies how it works.
> I listed the supported part numbers in the hwmon documentation, where
> they are also divided into these two families.

If the number of devices is relatively small, list them all. Otherwise
choose one device model and use it. No family models. No wildcards.

>>> +


Best regards,
Krzysztof
Guenter Roeck Nov. 9, 2023, 2:47 p.m. UTC | #4
On 11/9/23 00:41, Krzysztof Kozlowski wrote:
> On 08/11/2023 13:44, Javier Carrasco wrote:
> 
>>>> +properties:
>>>> +  compatible:
>>>> +    enum:
>>>> +      - amphenol,cc2dxx
>>>> +      - amphenol,cc2dxxs
>>>
>>> What does xx stand for? Wildcard? I do not see cc2dxx in part numbers.
>>> We expect specific compatibles, not generic. What are the differences
>>> between all parts?
>>>
>> There are two device families: cc2dxx and cc2dxxs, where xx indicates
>> the voltage and the accuracy. That does not change how the devices works
>> and it is not relevant for the driver. The 's' indicates that it is a
>> sleep device, and that modifies how it works.
>> I listed the supported part numbers in the hwmon documentation, where
>> they are also divided into these two families.
> 
> If the number of devices is relatively small, list them all. Otherwise
> choose one device model and use it. No family models. No wildcards.
> 

Agreed. There is no guarantee that CC2D[00..99][X] will be the same
devices.

Guenter
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/hwmon/amphenol,chipcap2.yaml b/Documentation/devicetree/bindings/hwmon/amphenol,chipcap2.yaml
new file mode 100644
index 000000000000..f0ab20eb418a
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/amphenol,chipcap2.yaml
@@ -0,0 +1,72 @@ 
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/hwmon/amphenol,chipcap2.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: ChipCap 2 humidity and temperature iio sensor
+
+maintainers:
+  - Javier Carrasco <javier.carrasco.cruz@gmail.com>
+
+description: |
+  Relative humidity and temperature sensor on I2C bus.
+
+  Datasheets:
+    https://www.amphenol-sensors.com/en/telaire/humidity/527-humidity-sensors/3095-chipcap-2
+
+properties:
+  compatible:
+    enum:
+      - amphenol,cc2dxx
+      - amphenol,cc2dxxs
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 3
+    description: |
+      The device provides three optional interrupts. READY indicates that
+      a measurement was finished. LOW indicates a low humidity alarm and
+      HIGH a high humidity alarm.
+      All interrupts must be IRQ_TYPE_RISING_EDGE.
+
+  interrupt-names:
+    items:
+      - const: READY
+      - const: LOW
+      - const: HIGH
+
+  vdd-supply:
+    description:
+      Dedicated, controllable supply-regulator to reset the device and
+      enter in command mode. If defined, it must provide a GPIO for its
+      control.
+      If not defined, no alarms will be available.
+
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        humidity@28 {
+            compatible = "amphenol,cc2dxxs";
+            reg = <0x28>;
+            interrupt-parent = <&gpio>;
+            interrupts = <4 IRQ_TYPE_EDGE_RISING>,
+                         <5 IRQ_TYPE_EDGE_RISING>,
+                         <6 IRQ_TYPE_EDGE_RISING>;
+            interrupt-names = "READY", "LOW", "HIGH";
+            vdd-supply = <&reg_vdd>;
+        };
+    };