Re: [PATCH 1/3] dt-bindings: iio: pressure: Support Honeywell mpr sensors

[Krzysztof Kozlowski]

On 14/04/2023 09:27, Andreas Klinger wrote:
> Hi Krzysztof,
> 
> Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx> schrieb am Sa, 01. Apr 11:42:
>> On 01/04/2023 11:09, Andreas Klinger wrote:
>>> Honeywell mpr is a pressure sensor family. There are many different
>>> types with different pressure ranges. The range needs to be set up in
>>> the dt. Therefore new properties honeywell,pmin and honeywell,pmax are
>>> introduced.
>>>
>>> Add dt-bindings.
>>>
>>> Signed-off-by: Andreas Klinger <ak@xxxxxxxxxxxxx>
>>> ---
>>>  .../bindings/iio/pressure/honeywell,mpr.yaml  | 74 +++++++++++++++++++
>>>  1 file changed, 74 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/iio/pressure/honeywell,mpr.yaml
>>>
>>> diff --git a/Documentation/devicetree/bindings/iio/pressure/honeywell,mpr.yaml b/Documentation/devicetree/bindings/iio/pressure/honeywell,mpr.yaml
>>> new file mode 100644
>>> index 000000000000..d6fad6f841cf
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/iio/pressure/honeywell,mpr.yaml
>>> @@ -0,0 +1,74 @@
>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/iio/pressure/honeywell,mpr.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: Honeywell mpr pressure sensor
>>> +
>>> +maintainers:
>>> +  - Andreas Klinger <ak@xxxxxxxxxxxxx>
>>> +
>>> +description: |
>>> +  Honeywell pressure sensor of type mpr. This sensor has an I2C and SPI interface. Only the I2C
>>
>> Doesn't look wrapped according to Linux coding style (see Coding style).
>>
>>> +  interface is implemented.
>>> +
>>> +  There are many subtypes with different pressure ranges available. Therefore the minimum and
>>> +  maximum pressure values of the specific sensor needs to be specified in Pascal.
>>> +
>>> +  Specifications about the devices can be found at:
>>> +    https://prod-edam.honeywell.com/content/dam/honeywell-edam/sps/siot/en-us/products/sensors/  \
>>> +      pressure-sensors/board-mount-pressure-sensors/micropressure-mpr-series/documents/          \
>>> +      sps-siot-mpr-series-datasheet-32332628-ciid-172626.pdf
>>
>> Lines are not continued, so drop \
>>
>>> +
>>> +properties:
>>> +  compatible:
>>> +    const: honeywell,mpr
>>
>> You need device specific compatible, not some generic one. Rename also
>> then the filename (should match the compatible).
>>
>>> +
>>> +  reg:
>>> +    maxItems: 1
>>> +
>>> +  interrupts:
>>> +    maxItems: 1
>>> +
>>> +  reset-gpios:
>>> +    description:
>>> +      Optional GPIO for resetting the device. If not present the device is not resetted.
>>
>> Are you sure it is wrapped properly?
>>
>>> +    maxItems: 1
>>> +
>>> +  honeywell,pmin:
>>> +    description:
>>> +      Minimum pressure value the sensor can measure in pascal.
>>
>> Use standard unit suffix:
>> https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/property-units.yaml
> 
> There are only kilopascal as standard unit suffix. But with kilopascal as
> integer the accuracy of the driver is very rough. Therefore I would like to use
> pascal. E. g.:
> 
> honeywell,pmin-pascal
> 
>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>> +
>>> +  honeywell,pmax:
>>> +    description:
>>> +      Maximum pressure value the sensor can measure in pascal.
>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>
>> Same.
>>
>> Why these values are suitable for DT?
> 
> Technically from the software perspective the sensors are identical with the
> only difference of having different pressure ranges, measurement units and
> transfer functions.
> 
> If we omit the pressure values and transfer function we'll need 96 compatibles
> and also 96 I2C ids.

Compatibles are expected to be specific. You used something generic, so
not correct, although I understand the reason behind.

If we go with generic compatible, do you guarantee that all Honeywell
mpr sensors - now and in 50 years - will be 100% compatible with this
set here?

Your description calls it "type mpr", not "model mpr", so I assume you
can have entirely different sensors coming soon which will not be
compatible.

> 
> But there are also custom sensor types. For covering them we'll need another
> compatible and just for this case the pressure values and transfer function.

OK, but isn't this the case in all devices? I could accept family of
identical devices with identical programming model where difference in
supported measurement range is described via this property.

Unfortunately you did not model it that way. Instead it represents all
possible devices even with incompatible programming model.

Best regards,
Krzysztof