Re: [PATCH 2/2] devicetree: hwmon: shtc1: Add sensirion,shtc1.yaml

[Guenter Roeck]

On 7/4/20 5:30 PM, Chris Ruehl wrote:
> Hi Guenter,
> 
> On 3/7/2020 1:49 pm, Guenter Roeck wrote:
>> On 7/2/20 8:48 PM, Chris Ruehl wrote:
>>> Add documentation for the newly added DTS support in the shtc1 driver.
>>>
>>> Signed-off-by: Chris Ruehl <chris.ruehl@xxxxxxxxxxxx>
>>> ---
>>> Â .../bindings/hwmon/sensirion,shtc1.yamlÂÂÂÂÂÂ | 53 +++++++++++++++++++
>>> Â 1 file changed, 53 insertions(+)
>>> Â create mode 100644 Documentation/devicetree/bindings/hwmon/sensirion,shtc1.yaml
>>>
>>> diff --git a/Documentation/devicetree/bindings/hwmon/sensirion,shtc1.yaml b/Documentation/devicetree/bindings/hwmon/sensirion,shtc1.yaml
>>> new file mode 100644
>>> index 000000000000..e3e292bc6d7d
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/hwmon/sensirion,shtc1.yaml
>>> @@ -0,0 +1,53 @@
>>> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/hwmon/sensirion,shtc1.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: Sensirion SHTC1 Humidity and Temperature Sensor IC
>>> +
>>> +maintainers:
>>> +Â - jdelvare@xxxxxxxx
>>> +
>>> +description: |
>>> +Â The SHTC1, SHTW1 and SHTC3 are digital humidity and temperature sensor
>>> +Â designed especially for battery-driven high-volume consumer electronics
>>> +Â applications.
>>> +Â For further information refere to Documentation/hwmon/shtc1.rst
>>> +
>>> +Â This binding document describes the binding for the hardware monitor
>>> +Â portion of the driver.
>>> +
>>> +properties:
>>> +Â compatible:
>>> +ÂÂÂ enum:
>>> +ÂÂÂÂÂ - sensirion,shtc1
>>> +ÂÂÂÂÂ - sensirion,shtw1
>>> +ÂÂÂÂÂ - sensirion,shtc3
>>> +
>>> +Â reg: I2C address 0x70
>>> +
>>> +Optional properties:
>>> +Â sensirion,blocking_io: |
>>> +ÂÂÂ u8, if > 0 the i2c bus hold until measure finished (default 0)
>>> +Â sensirion,high_precision: |
>>> +ÂÂÂ u8, if > 0 aquire data with high precision (default 1)
>>> +
>> Why u8 and not boolean ?
>>
>> Guenter
> The author of the driver make high_precision default (recommend) in the code,
> if I use boolean, then the device tree _must_ have have the sensirion,high_precision set
> or I need to do the opposite and define sensirion,low_precision.
> (blocking_io = false default, high_precision = true default)
> 
> that's the reason I was thinking use a u8 and test with of_property_read_bool to check
> the presence of it and set it if value > 0.
> 

Devicetree properties are supposed to be independent from actual implementations.
Declaring "we must do so because of an existing implementation" would set a really
bad precedence - everyone could use that later on to push through arbitrary sets
of devicetree properties. On top of that, this is supposed to be a new set of
bindings, with no one using it today. Any argument along the line of "must have"
seems irrelevant, since there is no real concern about devicetree backwards
compatibility. And on top of all that, I find the currently suggested code
really awkward and convoluted.

With that in mind, I personally would neither accept your argument nor your code.
If you object to defining sensirion,high_precision as boolean, and at the same
time object to defining sensirion,low_precision as well, I'd say, fine, then
let's stick with what we have today.

Anyway, I'll follow Rob's guidance.

Guenter