diff mbox series

[v2,1/2] dt-bindings: iio: pressure: add honeywell,hsc030

Message ID 20231117192305.17612-1-petre.rodan@subdimension.ro (mailing list archive)
State Changes Requested
Headers show
Series [v2,1/2] dt-bindings: iio: pressure: add honeywell,hsc030 | expand

Commit Message

Petre Rodan Nov. 17, 2023, 7:22 p.m. UTC
Adds binding for digital Honeywell TruStability HSC and SSC series pressure 
and temperature sensors.

Datasheet:
 [HSC] https://prod-edam.honeywell.com/content/dam/honeywell-edam/sps/siot/en-us/products/sensors/pressure-sensors/board-mount-pressure-sensors/trustability-hsc-series/documents/sps-siot-trustability-hsc-series-high-accuracy-board-mount-pressure-sensors-50099148-a-en-ciid-151133.pdf
 [SSC] https://prod-edam.honeywell.com/content/dam/honeywell-edam/sps/siot/en-us/products/sensors/pressure-sensors/board-mount-pressure-sensors/trustability-ssc-series/documents/sps-siot-trustability-ssc-series-standard-accuracy-board-mount-pressure-sensors-50099533-a-en-ciid-151134.pdf

Signed-off-by: Petre Rodan <petre.rodan@subdimension.ro>
---

Changes for v2:
- Removed redundant quotations reported by robh's bot
- Fixed yamllint warnings

I'm failing to run 'make DT_CHECKER_FLAGS=-m dt_binding_check' due to
python errors and exceptions
---
 .../iio/pressure/honeywell,hsc030pa.yaml      | 156 ++++++++++++++++++
 1 file changed, 156 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml

Comments

Rob Herring Nov. 17, 2023, 8:13 p.m. UTC | #1
On Fri, 17 Nov 2023 21:22:57 +0200, Petre Rodan wrote:
> Adds binding for digital Honeywell TruStability HSC and SSC series pressure
> and temperature sensors.
> 
> Datasheet:
>  [HSC] https://prod-edam.honeywell.com/content/dam/honeywell-edam/sps/siot/en-us/products/sensors/pressure-sensors/board-mount-pressure-sensors/trustability-hsc-series/documents/sps-siot-trustability-hsc-series-high-accuracy-board-mount-pressure-sensors-50099148-a-en-ciid-151133.pdf
>  [SSC] https://prod-edam.honeywell.com/content/dam/honeywell-edam/sps/siot/en-us/products/sensors/pressure-sensors/board-mount-pressure-sensors/trustability-ssc-series/documents/sps-siot-trustability-ssc-series-standard-accuracy-board-mount-pressure-sensors-50099533-a-en-ciid-151134.pdf
> 
> Signed-off-by: Petre Rodan <petre.rodan@subdimension.ro>
> ---
> 
> Changes for v2:
> - Removed redundant quotations reported by robh's bot
> - Fixed yamllint warnings
> 
> I'm failing to run 'make DT_CHECKER_FLAGS=-m dt_binding_check' due to
> python errors and exceptions
> ---
>  .../iio/pressure/honeywell,hsc030pa.yaml      | 156 ++++++++++++++++++
>  1 file changed, 156 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
Error: Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.example.dts:36.15-16 syntax error
FATAL ERROR: Unable to parse input tree
make[2]: *** [scripts/Makefile.lib:419: Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.example.dtb] Error 1
make[2]: *** Waiting for unfinished jobs....
make[1]: *** [/builds/robherring/dt-review-ci/linux/Makefile:1424: dt_binding_check] Error 2
make: *** [Makefile:234: __sub-make] Error 2

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20231117192305.17612-1-petre.rodan@subdimension.ro

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.
Rob Herring Nov. 19, 2023, 1:49 p.m. UTC | #2
On Fri, Nov 17, 2023 at 09:22:57PM +0200, Petre Rodan wrote:
> Adds binding for digital Honeywell TruStability HSC and SSC series pressure 
> and temperature sensors.
> 
> Datasheet:
>  [HSC] https://prod-edam.honeywell.com/content/dam/honeywell-edam/sps/siot/en-us/products/sensors/pressure-sensors/board-mount-pressure-sensors/trustability-hsc-series/documents/sps-siot-trustability-hsc-series-high-accuracy-board-mount-pressure-sensors-50099148-a-en-ciid-151133.pdf
>  [SSC] https://prod-edam.honeywell.com/content/dam/honeywell-edam/sps/siot/en-us/products/sensors/pressure-sensors/board-mount-pressure-sensors/trustability-ssc-series/documents/sps-siot-trustability-ssc-series-standard-accuracy-board-mount-pressure-sensors-50099533-a-en-ciid-151134.pdf
> 
> Signed-off-by: Petre Rodan <petre.rodan@subdimension.ro>
> ---
> 
> Changes for v2:
> - Removed redundant quotations reported by robh's bot
> - Fixed yamllint warnings
> 
> I'm failing to run 'make DT_CHECKER_FLAGS=-m dt_binding_check' due to
> python errors and exceptions

What exceptions?
Petre Rodan Nov. 19, 2023, 8:14 p.m. UTC | #3
Good morning!

On Sun, Nov 19, 2023 at 07:49:39AM -0600, Rob Herring wrote:
> On Fri, Nov 17, 2023 at 09:22:57PM +0200, Petre Rodan wrote:
> > Adds binding for digital Honeywell TruStability HSC and SSC series pressure 
> > and temperature sensors.
> > 
[..]
> > Changes for v2:
> > - Removed redundant quotations reported by robh's bot
> > - Fixed yamllint warnings
> > 
> > I'm failing to run 'make DT_CHECKER_FLAGS=-m dt_binding_check' due to
> > python errors and exceptions
> 
> What exceptions?

thanks for asking.

first off, installed packages. the first 4 are not part of the official Gentoo repo, so I might have prepared them with missing options if any where not included by default.
I know nothing about python.

$ equery l dtschema pylibfdt ruamel-yaml yamllint jsonschema python 
[I-O] [  ] dev-python/dtschema-2023.9:0
[I-O] [  ] dev-python/pylibfdt-1.7.0_p1:0
[I-O] [  ] dev-python/ruamel-yaml-0.18.5:0
[I-O] [  ] dev-python/yamllint-1.33.0:0
[IP-] [  ] dev-python/jsonschema-4.19.1:0
[IP-] [  ] dev-lang/python-2.7.18_p16-r1:2.7
[IP-] [  ] dev-lang/python-3.10.13:3.10
[IP-] [  ] dev-lang/python-3.11.5:3.11
prodan@sunspire /usr/src/linux-upstream $ python --version
Python 3.11.5

# binding check
prodan@sunspire /usr/src/linux-upstream $ make DT_SCHEMA_FILES=Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml  DT_CHECKER_FLAGS=-m dt_binding_check
Traceback (most recent call last):
  File "/usr/lib/python3.11/site-packages/jsonschema/validators.py", line 1152, in resolve_fragment
    document = document[part]
               ~~~~~~~~^^^^^^
KeyError: 'definitions'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/usr/lib/python-exec/python3.11/dt-doc-validate", line 64, in <module>
    ret |= check_doc(f)
           ^^^^^^^^^^^^
  File "/usr/lib/python-exec/python3.11/dt-doc-validate", line 32, in check_doc
    for error in sorted(dtsch.iter_errors(), key=lambda e: e.linecol):
                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.11/site-packages/dtschema/schema.py", line 123, in iter_errors
    for error in self.validator.iter_errors(self):
  File "/usr/lib/python3.11/site-packages/jsonschema/validators.py", line 368, in iter_errors
    for error in errors:
  File "/usr/lib/python3.11/site-packages/jsonschema/_keywords.py", line 335, in allOf
    yield from validator.descend(instance, subschema, schema_path=index)
  File "/usr/lib/python3.11/site-packages/jsonschema/validators.py", line 416, in descend
    for error in errors:
  File "/usr/lib/python3.11/site-packages/jsonschema/_keywords.py", line 284, in ref
    yield from validator._validate_reference(ref=ref, instance=instance)
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.11/site-packages/jsonschema/validators.py", line 465, in _validate_reference
    return list(self.descend(instance, resolved))
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.11/site-packages/jsonschema/validators.py", line 416, in descend
    for error in errors:
  File "/usr/lib/python3.11/site-packages/jsonschema/_keywords.py", line 335, in allOf
    yield from validator.descend(instance, subschema, schema_path=index)
  File "/usr/lib/python3.11/site-packages/jsonschema/validators.py", line 416, in descend
    for error in errors:
  File "/usr/lib/python3.11/site-packages/jsonschema/_keywords.py", line 284, in ref
    yield from validator._validate_reference(ref=ref, instance=instance)
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.11/site-packages/jsonschema/validators.py", line 465, in _validate_reference
    return list(self.descend(instance, resolved))
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.11/site-packages/jsonschema/validators.py", line 416, in descend
    for error in errors:
  File "/usr/lib/python3.11/site-packages/jsonschema/_keywords.py", line 305, in properties
    yield from validator.descend(
  File "/usr/lib/python3.11/site-packages/jsonschema/validators.py", line 416, in descend
    for error in errors:
  File "/usr/lib/python3.11/site-packages/jsonschema/_keywords.py", line 34, in propertyNames
    yield from validator.descend(instance=property, schema=propertyNames)
  File "/usr/lib/python3.11/site-packages/jsonschema/validators.py", line 416, in descend
    for error in errors:
  File "/usr/lib/python3.11/site-packages/jsonschema/_keywords.py", line 335, in allOf
    yield from validator.descend(instance, subschema, schema_path=index)
  File "/usr/lib/python3.11/site-packages/jsonschema/validators.py", line 416, in descend
    for error in errors:
  File "/usr/lib/python3.11/site-packages/jsonschema/_keywords.py", line 378, in not_
    if validator.evolve(schema=not_schema).is_valid(instance):
       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.11/site-packages/jsonschema/validators.py", line 483, in is_valid
    error = next(self.iter_errors(instance), None)
            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.11/site-packages/jsonschema/validators.py", line 368, in iter_errors
    for error in errors:
  File "/usr/lib/python3.11/site-packages/jsonschema/_keywords.py", line 284, in ref
    yield from validator._validate_reference(ref=ref, instance=instance)
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.11/site-packages/jsonschema/validators.py", line 461, in _validate_reference
    scope, resolved = resolve(ref)
                      ^^^^^^^^^^^^
  File "/usr/lib/python3.11/site-packages/jsonschema/validators.py", line 1086, in resolve
    return url, self._remote_cache(url)
                ^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.11/site-packages/jsonschema/validators.py", line 1104, in resolve_from_url
    return self.resolve_fragment(document, fragment)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.11/site-packages/jsonschema/validators.py", line 1154, in resolve_fragment
    raise exceptions._RefResolutionError(
jsonschema.exceptions._RefResolutionError: Unresolvable JSON pointer: 'definitions/json-schema-prop-names'
Error: Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.example.dts:36.15-16 syntax error
FATAL ERROR: Unable to parse input tree
make[2]: *** [scripts/Makefile.lib:419: Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.example.dtb] Error 1
make[1]: *** [/usr/src/linux-upstream/Makefile:1424: dt_binding_check] Error 2
make: *** [Makefile:234: __sub-make] Error 2

best regards,
peter
Krzysztof Kozlowski Nov. 20, 2023, 10:15 a.m. UTC | #4
On 19/11/2023 21:14, Petre Rodan wrote:
> 
> Good morning!
> 
> On Sun, Nov 19, 2023 at 07:49:39AM -0600, Rob Herring wrote:
>> On Fri, Nov 17, 2023 at 09:22:57PM +0200, Petre Rodan wrote:
>>> Adds binding for digital Honeywell TruStability HSC and SSC series pressure 
>>> and temperature sensors.
>>>
> [..]
>>> Changes for v2:
>>> - Removed redundant quotations reported by robh's bot
>>> - Fixed yamllint warnings
>>>
>>> I'm failing to run 'make DT_CHECKER_FLAGS=-m dt_binding_check' due to
>>> python errors and exceptions
>>
>> What exceptions?
> 
> thanks for asking.
> 
> first off, installed packages. the first 4 are not part of the official Gentoo repo, so I might have prepared them with missing options if any where not included by default.
> I know nothing about python.

The easiest is to install them with pip. You might get old versions from
your distro. Although in your case, it looks you got most recent.

> 
> $ equery l dtschema pylibfdt ruamel-yaml yamllint jsonschema python 
> [I-O] [  ] dev-python/dtschema-2023.9:0
> [I-O] [  ] dev-python/pylibfdt-1.7.0_p1:0
> [I-O] [  ] dev-python/ruamel-yaml-0.18.5:0
> [I-O] [  ] dev-python/yamllint-1.33.0:0
> [IP-] [  ] dev-python/jsonschema-4.19.1:0

I guess this is the problem. The dtschema explicitly asks for:
    "jsonschema>=4.1.2,<4.18",
because newer jsonschema does not work well for us.

Best regards,
Krzysztof
Krzysztof Kozlowski Nov. 20, 2023, 10:21 a.m. UTC | #5
On 17/11/2023 20:22, Petre Rodan wrote:
> Adds binding for digital Honeywell TruStability HSC and SSC series pressure 
> and temperature sensors.
> 
> Datasheet:
>  [HSC] https://prod-edam.honeywell.com/content/dam/honeywell-edam/sps/siot/en-us/products/sensors/pressure-sensors/board-mount-pressure-sensors/trustability-hsc-series/documents/sps-siot-trustability-hsc-series-high-accuracy-board-mount-pressure-sensors-50099148-a-en-ciid-151133.pdf
>  [SSC] https://prod-edam.honeywell.com/content/dam/honeywell-edam/sps/siot/en-us/products/sensors/pressure-sensors/board-mount-pressure-sensors/trustability-ssc-series/documents/sps-siot-trustability-ssc-series-standard-accuracy-board-mount-pressure-sensors-50099533-a-en-ciid-151134.pdf
> 
> Signed-off-by: Petre Rodan <petre.rodan@subdimension.ro>
> ---
> 
> Changes for v2:
> - Removed redundant quotations reported by robh's bot
> - Fixed yamllint warnings
> 
> I'm failing to run 'make DT_CHECKER_FLAGS=-m dt_binding_check' due to
> python errors and exceptions
> ---
>  .../iio/pressure/honeywell,hsc030pa.yaml      | 156 ++++++++++++++++++
>  1 file changed, 156 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml
> 
> diff --git a/Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml b/Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml
> new file mode 100644
> index 000000000000..c7e5d3bd5ef4
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml
> @@ -0,0 +1,156 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/pressure/honeywell,hsc030pa.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Honeywell TruStability HSC and SSC pressure sensor families
> +
> +description: |
> +  support for Honeywell TruStability HSC and SSC digital pressure sensor
> +  families.
> +
> +  These sensors have either an I2C, an SPI or an analog interface. Only the
> +  digital versions are supported by this driver.
> +
> +  There are 118 models with different pressure ranges available in each family.
> +  The vendor calls them "HSC series" and "SSC series". All of them have an
> +  identical programming model but differ in pressure range, unit and transfer
> +  function.
> +
> +  To support different models one need to specify the pressure range as well as
> +  the transfer function. Pressure range can either be provided via range_str or
> +  in case it's a custom chip via numerical range limits converted to pascals.
> +
> +  The transfer function defines the ranges of raw conversion values delivered
> +  by the sensor. pmin-pascal and pmax-pascal corespond to the minimum and
> +  maximum pressure that can be measured.
> +
> +  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/trustability-hsc-series/documents/sps-siot-trustability-hsc-series-high-accuracy-board-mount-pressure-sensors-50099148-a-en-ciid-151133.pdf
> +  https://prod-edam.honeywell.com/content/dam/honeywell-edam/sps/siot/en-us/products/sensors/pressure-sensors/board-mount-pressure-sensors/trustability-ssc-series/documents/sps-siot-trustability-ssc-series-standard-accuracy-board-mount-pressure-sensors-50099533-a-en-ciid-151134.pdf
> +
> +maintainers:
> +  - Petre Rodan <petre.rodan@subdimension.ro>
> +
> +properties:
> +  compatible:
> +    enum:
> +      - honeywell,hsc

Way too generic

> +      - honeywell,ssc

Way too generic

> +
> +  reg:
> +    maxItems: 1
> +
> +  honeywell,transfer-function:
> +    description: |
> +      Transfer function which defines the range of valid values delivered by
> +      the sensor.
> +      0 - A, 10% to 90% of 2^14
> +      1 - B, 5% to 95% of 2^14
> +      2 - C, 5% to 85% of 2^14
> +      3 - F, 4% to 94% of 2^14
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +
> +  honeywell,range_str:

No underscores in property names.

"str" is redundant. Instead say what is it, because "range" is way too
vague.

> +    description: |
> +      Five character string that defines "pressure range, unit and type"
> +      as part of the device nomenclature. In the unlikely case of a custom
> +      chip, set to "NA" and provide honeywell,pmin-pascal honeywell,pmax-pascal
> +    enum: [001BA, 1.6BA, 2.5BA, 004BA, 006BA, 010BA, 1.6MD, 2.5MD, 004MD,
> +           006MD, 010MD, 016MD, 025MD, 040MD, 060MD, 100MD, 160MD, 250MD,
> +           400MD, 600MD, 001BD, 1.6BD, 2.5BD, 004BD, 2.5MG, 004MG, 006MG,
> +           010MG, 016MG, 025MG, 040MG, 060MG, 100MG, 160MG, 250MG, 400MG,
> +           600MG, 001BG, 1.6BG, 2.5BG, 004BG, 006BG, 010BG, 100KA, 160KA,
> +           250KA, 400KA, 600KA, 001GA, 160LD, 250LD, 400LD, 600LD, 001KD,
> +           1.6KD, 2.5KD, 004KD, 006KD, 010KD, 016KD, 025KD, 040KD, 060KD,
> +           100KD, 160KD, 250KD, 400KD, 250LG, 400LG, 600LG, 001KG, 1.6KG,
> +           2.5KG, 004KG, 006KG, 010KG, 016KG, 025KG, 040KG, 060KG, 100KG,
> +           160KG, 250KG, 400KG, 600KG, 001GG, 015PA, 030PA, 060PA, 100PA,
> +           150PA, 0.5ND, 001ND, 002ND, 004ND, 005ND, 010ND, 020ND, 030ND,
> +           001PD, 005PD, 015PD, 030PD, 060PD, 001NG, 002NG, 004NG, 005NG,
> +           010NG, 020NG, 030NG, 001PG, 005PG, 015PG, 030PG, 060PG, 100PG,
> +           150PG, NA]
> +    $ref: /schemas/types.yaml#/definitions/string
> +
> +  honeywell,pmin-pascal:
> +    description: |
> +      Minimum pressure value the sensor can measure in pascal.
> +      To be specified only if honeywell,range_str is set to "NA".
> +    $ref: /schemas/types.yaml#/definitions/int32

That's uint32. Why do you need negative values?

> +
> +  honeywell,pmax-pascal:
> +    description: |
> +      Maximum pressure value the sensor can measure in pascal.
> +      To be specified only if honeywell,range_str is set to "NA".
> +    $ref: /schemas/types.yaml#/definitions/int32

Ditto

> +
> +  vdd-supply:
> +    description: |
> +      Provide VDD power to the sensor (either 3.3V or 5V depending on the chip).
> +      Optional, activate only if required by the target board.

Drop the last sentence. The supplies are required not by target board
but by hardware. I also do not understand what "activate" means in terms
of bindings and DTS.

> +
> +  spi-max-frequency:
> +    description: SPI clock to be kept between 50 and 800kHz

Drop description, add minimum/maximum constraints if worth.

> +
> +  clock-frequency:
> +    description: i2c clock to be kept between 100 and 400kHz

Drop, that's not really an I2C device property. Your driver must use
common clock framework.

> +
> +required:
> +  - compatible
> +  - reg
> +  - honeywell,transfer-function
> +  - honeywell,range_str
> +  - clock-frequency

Why?

> +  - spi-max-frequency
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +    i2c {
> +        status = "okay";

?!? Drop

> +        clock-frequency = <400000>;

Drop

> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        HSCMRNN030PA2A3@28 {

Node names should be generic. See also an explanation and list of
examples (not exhaustive) in DT specification:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation

Plus, upper case is not allowed...

> +          status = "okay";

Drop. BTW status never comes first!

> +          compatible = "honeywell,hsc";
> +          reg = <0x28>;
> +
> +          honeywell,transfer-function = <0>;
> +          honeywell,range_str = "030PA";
> +        };
> +    };
> +
> +    spi {
> +        # note that MOSI is not required by this sensor

This should be then part of description, not example.

> +        status = "okay";

Drop

> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        HSCMLNN100PASA3@0 {

Eh...

> +          status = "okay";

Drop

> +          compatible = "honeywell,hsc";
> +          reg = <0>;
> +          spi-max-frequency = <800000>;
> +
> +          honeywell,transfer-function = <0>;
> +          honeywell,range_str = "100PA";
> +        };
> +
> +        HSC_CUSTOM_CHIP@0 {

Drop, not needed. One example is enough.

> +          status = "okay";
> +          compatible = "honeywell,hsc";
> +          reg = <1>;
> +          spi-max-frequency = <800000>;

Also, your indentation is broken.

Use 4 spaces for example indentation.

> +
> +          honeywell,transfer-function = <0>;
> +          honeywell,range_str = "NA";
> +          honeywell,pmin-pascal = <0>;
> +          honeywell,pmax-pascal = <206850>;
> +        };
> +

No stray blank lines.

Best regards,
Krzysztof
Petre Rodan Nov. 20, 2023, 1:42 p.m. UTC | #6
Hello Krzysztof,

thanks for the pointer regarding the version requirement for jsonschema.
installing an older version fixed all python exceptions.


On Mon, Nov 20, 2023 at 11:21:42AM +0100, Krzysztof Kozlowski wrote:
> On 17/11/2023 20:22, Petre Rodan wrote:
> > Adds binding for digital Honeywell TruStability HSC and SSC series pressure 
> > and temperature sensors.
> > 
> > Datasheet:
> >  [HSC] https://prod-edam.honeywell.com/content/dam/honeywell-edam/sps/siot/en-us/products/sensors/pressure-sensors/board-mount-pressure-sensors/trustability-hsc-series/documents/sps-siot-trustability-hsc-series-high-accuracy-board-mount-pressure-sensors-50099148-a-en-ciid-151133.pdf
> >  [SSC] https://prod-edam.honeywell.com/content/dam/honeywell-edam/sps/siot/en-us/products/sensors/pressure-sensors/board-mount-pressure-sensors/trustability-ssc-series/documents/sps-siot-trustability-ssc-series-standard-accuracy-board-mount-pressure-sensors-50099533-a-en-ciid-151134.pdf

> > +properties:
> > +  compatible:
> > +    enum:
> > +      - honeywell,hsc
> 
> Way too generic

I'm new to this, please excuse my ignorance.
my driver covers all Honeywell pressure sensors under the "TruStability board mount HSC/SSC" moniker.
that is why my intention was to provide a rather generic name for the driver itself.
are you afraid that they will come up with a different device that they will call "hsc" in the future?
in this case honeywell,trustability-hsc would be fine?

as I see you prefer to target a particular chip, but I am a bit afraid that the end-user will be confused by needing to set up something like

pressure@28 {
	compatible = "honeywell,hsc030pa";
	reg = <0x28>;
	honeywell,transfer-function = <0>;
	honeywell,pressure-range = "250MD";
};

ie. specifying "hsc030pa" as driver while his chip is not in the 030PA range, but 250MD.

so do you prefer
 honeywell,trustability-hsc  OR
 honeywell,hsc030pa

> > +  honeywell,range_str:
> 
> No underscores in property names.
> 
> "str" is redundant. Instead say what is it, because "range" is way too
> vague.

will rename to honeywell,pressure-range if that is ok with you.

> > +    description: |
> > +      Five character string that defines "pressure range, unit and type"
> > +      as part of the device nomenclature. In the unlikely case of a custom
> > +      chip, set to "NA" and provide honeywell,pmin-pascal honeywell,pmax-pascal
> > +    enum: [001BA, 1.6BA, 2.5BA, 004BA, 006BA, 010BA, 1.6MD, 2.5MD, 004MD,
> > +           006MD, 010MD, 016MD, 025MD, 040MD, 060MD, 100MD, 160MD, 250MD,
> > +           400MD, 600MD, 001BD, 1.6BD, 2.5BD, 004BD, 2.5MG, 004MG, 006MG,
> > +           010MG, 016MG, 025MG, 040MG, 060MG, 100MG, 160MG, 250MG, 400MG,
> > +           600MG, 001BG, 1.6BG, 2.5BG, 004BG, 006BG, 010BG, 100KA, 160KA,
> > +           250KA, 400KA, 600KA, 001GA, 160LD, 250LD, 400LD, 600LD, 001KD,
> > +           1.6KD, 2.5KD, 004KD, 006KD, 010KD, 016KD, 025KD, 040KD, 060KD,
> > +           100KD, 160KD, 250KD, 400KD, 250LG, 400LG, 600LG, 001KG, 1.6KG,
> > +           2.5KG, 004KG, 006KG, 010KG, 016KG, 025KG, 040KG, 060KG, 100KG,
> > +           160KG, 250KG, 400KG, 600KG, 001GG, 015PA, 030PA, 060PA, 100PA,
> > +           150PA, 0.5ND, 001ND, 002ND, 004ND, 005ND, 010ND, 020ND, 030ND,
> > +           001PD, 005PD, 015PD, 030PD, 060PD, 001NG, 002NG, 004NG, 005NG,
> > +           010NG, 020NG, 030NG, 001PG, 005PG, 015PG, 030PG, 060PG, 100PG,
> > +           150PG, NA]
> > +    $ref: /schemas/types.yaml#/definitions/string
> > +
> > +  honeywell,pmin-pascal:
> > +    description: |
> > +      Minimum pressure value the sensor can measure in pascal.
> > +      To be specified only if honeywell,range_str is set to "NA".
> > +    $ref: /schemas/types.yaml#/definitions/int32
> 
> That's uint32. Why do you need negative values?

signed int32 is intentional. some chips have two physical input ports and measure a pressure differential in which case pmin is negative.
see either of the pdfs at page 14, table 8, column 2, row 7+

> > +  honeywell,pmax-pascal:
> > +    description: |
> > +      Maximum pressure value the sensor can measure in pascal.
> > +      To be specified only if honeywell,range_str is set to "NA".
> > +    $ref: /schemas/types.yaml#/definitions/int32
> 
> Ditto

well, since we saw pmin needs to be signed should we have pmax unsigned?

> > +  vdd-supply:
> > +    description: |
> > +      Provide VDD power to the sensor (either 3.3V or 5V depending on the chip).
> > +      Optional, activate only if required by the target board.
> 
> Drop the last sentence. The supplies are required not by target board
> but by hardware. I also do not understand what "activate" means in terms
> of bindings and DTS.

ok, ignore rambling.

> > +
> > +  spi-max-frequency:
> > +    description: SPI clock to be kept between 50 and 800kHz
> 
> Drop description, add minimum/maximum constraints if worth.

will replace block with

  spi-max-frequency:
    maximum: 800000

as I saw in other yaml files
 
> > +  clock-frequency:
> > +    description: i2c clock to be kept between 100 and 400kHz
> 
> Drop, that's not really an I2C device property. Your driver must use
> common clock framework.

ack

> > +required:
> > +  - compatible
> > +  - reg
> > +  - honeywell,transfer-function
> > +  - honeywell,range_str
> > +  - clock-frequency
> 
> Why?

dropped clock-frequency

everything else below will be as you asked.

I will provide a new set of patches after I get your inpyt.

my very best regards,
peter

> > +  - spi-max-frequency
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/gpio/gpio.h>
> > +    i2c {
> > +        status = "okay";
> 
> ?!? Drop
> 
> > +        clock-frequency = <400000>;
> 
> Drop
> 
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +
> > +        HSCMRNN030PA2A3@28 {
> 
> Node names should be generic. See also an explanation and list of
> examples (not exhaustive) in DT specification:
> https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
> 
> Plus, upper case is not allowed...
> 
> > +          status = "okay";
> 
> Drop. BTW status never comes first!
> 
> > +          compatible = "honeywell,hsc";
> > +          reg = <0x28>;
> > +
> > +          honeywell,transfer-function = <0>;
> > +          honeywell,range_str = "030PA";
> > +        };
> > +    };
> > +
> > +    spi {
> > +        # note that MOSI is not required by this sensor
> 
> This should be then part of description, not example.
> 
> > +        status = "okay";
> 
> Drop
> 
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +
> > +        HSCMLNN100PASA3@0 {
> 
> Eh...
> 
> > +          status = "okay";
> 
> Drop
> 
> > +          compatible = "honeywell,hsc";
> > +          reg = <0>;
> > +          spi-max-frequency = <800000>;
> > +
> > +          honeywell,transfer-function = <0>;
> > +          honeywell,range_str = "100PA";
> > +        };
> > +
> > +        HSC_CUSTOM_CHIP@0 {
> 
> Drop, not needed. One example is enough.
> 
> > +          status = "okay";
> > +          compatible = "honeywell,hsc";
> > +          reg = <1>;
> > +          spi-max-frequency = <800000>;
> 
> Also, your indentation is broken.
> 
> Use 4 spaces for example indentation.
> 
> > +
> > +          honeywell,transfer-function = <0>;
> > +          honeywell,range_str = "NA";
> > +          honeywell,pmin-pascal = <0>;
> > +          honeywell,pmax-pascal = <206850>;
> > +        };
> > +
> 
> No stray blank lines.
> 
> Best regards,
> Krzysztof
> 
>
Krzysztof Kozlowski Nov. 20, 2023, 2:04 p.m. UTC | #7
On 20/11/2023 14:42, Petre Rodan wrote:

>>> +properties:
>>> +  compatible:
>>> +    enum:
>>> +      - honeywell,hsc
>>
>> Way too generic
> 
> I'm new to this, please excuse my ignorance.
> my driver covers all Honeywell pressure sensors under the "TruStability board mount HSC/SSC" moniker.

We talk here about bindings, not driver. For the driver you can use
whatever name is approved by reviewers of your driver.

> that is why my intention was to provide a rather generic name for the driver itself.
> are you afraid that they will come up with a different device that they will call "hsc" in the future?
> in this case honeywell,trustability-hsc would be fine?
> 
> as I see you prefer to target a particular chip, but I am a bit afraid that the end-user will be confused by needing to set up something like
> 
> pressure@28 {
> 	compatible = "honeywell,hsc030pa";

The compatible should be specific, thus for example match exact model
number.

If you can guarantee that all devices from given family are the same in
respect of programming model and hardware requirements (e.g. supplies),
then you could go with family name. However such guarantees are rarely
given. Therefore for mprls0025pa I agreed for using one specific model
for entire family.

https://lore.kernel.org/all/d577bc44-780f-f25d-29c6-ed1d353b540c@linaro.org/


> 	reg = <0x28>;
> 	honeywell,transfer-function = <0>;
> 	honeywell,pressure-range = "250MD";
> };
> 
> ie. specifying "hsc030pa" as driver while his chip is not in the 030PA range, but 250MD.
> 
> so do you prefer
>  honeywell,trustability-hsc  OR
>  honeywell,hsc030pa

I think the latter, just like we did for mprls0025pa. How many devices
do you have there?


> 
>>> +  honeywell,range_str:
>>
>> No underscores in property names.
>>
>> "str" is redundant. Instead say what is it, because "range" is way too
>> vague.
> 
> will rename to honeywell,pressure-range if that is ok with you.

Yes

> 
>>> +    description: |
>>> +      Five character string that defines "pressure range, unit and type"
>>> +      as part of the device nomenclature. In the unlikely case of a custom
>>> +      chip, set to "NA" and provide honeywell,pmin-pascal honeywell,pmax-pascal
>>> +    enum: [001BA, 1.6BA, 2.5BA, 004BA, 006BA, 010BA, 1.6MD, 2.5MD, 004MD,
>>> +           006MD, 010MD, 016MD, 025MD, 040MD, 060MD, 100MD, 160MD, 250MD,
>>> +           400MD, 600MD, 001BD, 1.6BD, 2.5BD, 004BD, 2.5MG, 004MG, 006MG,
>>> +           010MG, 016MG, 025MG, 040MG, 060MG, 100MG, 160MG, 250MG, 400MG,
>>> +           600MG, 001BG, 1.6BG, 2.5BG, 004BG, 006BG, 010BG, 100KA, 160KA,
>>> +           250KA, 400KA, 600KA, 001GA, 160LD, 250LD, 400LD, 600LD, 001KD,
>>> +           1.6KD, 2.5KD, 004KD, 006KD, 010KD, 016KD, 025KD, 040KD, 060KD,
>>> +           100KD, 160KD, 250KD, 400KD, 250LG, 400LG, 600LG, 001KG, 1.6KG,
>>> +           2.5KG, 004KG, 006KG, 010KG, 016KG, 025KG, 040KG, 060KG, 100KG,
>>> +           160KG, 250KG, 400KG, 600KG, 001GG, 015PA, 030PA, 060PA, 100PA,
>>> +           150PA, 0.5ND, 001ND, 002ND, 004ND, 005ND, 010ND, 020ND, 030ND,
>>> +           001PD, 005PD, 015PD, 030PD, 060PD, 001NG, 002NG, 004NG, 005NG,
>>> +           010NG, 020NG, 030NG, 001PG, 005PG, 015PG, 030PG, 060PG, 100PG,
>>> +           150PG, NA]
>>> +    $ref: /schemas/types.yaml#/definitions/string
>>> +
>>> +  honeywell,pmin-pascal:
>>> +    description: |
>>> +      Minimum pressure value the sensor can measure in pascal.
>>> +      To be specified only if honeywell,range_str is set to "NA".
>>> +    $ref: /schemas/types.yaml#/definitions/int32
>>
>> That's uint32. Why do you need negative values?
> 
> signed int32 is intentional. some chips have two physical input ports and measure a pressure differential in which case pmin is negative.
> see either of the pdfs at page 14, table 8, column 2, row 7+

Then the best would be to change the type in other bindings to have
int32 everywhere... but dtschema also requires unt32:
https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/property-units.yaml

I think pressure can be negative (e.g. when device measures pressure in
relation to atmospheric pressure), thus maybe we need to fix dtschema first.

> 
>>> +  honeywell,pmax-pascal:
>>> +    description: |
>>> +      Maximum pressure value the sensor can measure in pascal.
>>> +      To be specified only if honeywell,range_str is set to "NA".
>>> +    $ref: /schemas/types.yaml#/definitions/int32
>>
>> Ditto
> 
> well, since we saw pmin needs to be signed should we have pmax unsigned?

I guess this could stay uint32, although it depends on final units for
pascal.

Best regards,
Krzysztof
Petre Rodan Nov. 20, 2023, 2:40 p.m. UTC | #8
Hello!

On Mon, Nov 20, 2023 at 03:04:07PM +0100, Krzysztof Kozlowski wrote:
> On 20/11/2023 14:42, Petre Rodan wrote:
> 
> >>> +properties:
> >>> +  compatible:
> >>> +    enum:
> >>> +      - honeywell,hsc
> >>
> >> Way too generic
> > 
> > I'm new to this, please excuse my ignorance.
> > my driver covers all Honeywell pressure sensors under the "TruStability board mount HSC/SSC" moniker.
> 
> We talk here about bindings, not driver. For the driver you can use
> whatever name is approved by reviewers of your driver.
> 
> > that is why my intention was to provide a rather generic name for the driver itself.
> > are you afraid that they will come up with a different device that they will call "hsc" in the future?
> > in this case honeywell,trustability-hsc would be fine?
> > 
> > as I see you prefer to target a particular chip, but I am a bit afraid that the end-user will be confused by needing to set up something like
> > 
> > pressure@28 {
> > 	compatible = "honeywell,hsc030pa";
> 
> The compatible should be specific, thus for example match exact model
> number.

there are an infinite number of combinations of 4 transfer functions and 118 ranges + one custom range, so providing an array with all specific chips that could end up as compatible is out of the question.
I was aiming at providing a generic name for the binding and get the transfer function and the pressure range as required parameters.

> If you can guarantee that all devices from given family are the same in
> respect of programming model and hardware requirements (e.g. supplies),
> then you could go with family name. However such guarantees are rarely
> given.

I see your point.

> Therefore for mprls0025pa I agreed for using one specific model
> for entire family.
> 
> https://lore.kernel.org/all/d577bc44-780f-f25d-29c6-ed1d353b540c@linaro.org/
> 
> 
> > 	reg = <0x28>;
> > 	honeywell,transfer-function = <0>;
> > 	honeywell,pressure-range = "250MD";
> > };
> > 
> > ie. specifying "hsc030pa" as driver while his chip is not in the 030PA range, but 250MD.
> > 
> > so do you prefer
> >  honeywell,trustability-hsc  OR
> >  honeywell,hsc030pa
> 
> I think the latter, just like we did for mprls0025pa. How many devices
> do you have there?

both hsc and ssc have 118 ranges, 4 transfer functions and both can be requested from the manufacturer with custom measurement ranges.

ok,I will rename hsc->hsc030pa in the code as you requested.

> >>> +  honeywell,pmin-pascal:
> >>> +    description: |
> >>> +      Minimum pressure value the sensor can measure in pascal.
> >>> +      To be specified only if honeywell,range_str is set to "NA".
> >>> +    $ref: /schemas/types.yaml#/definitions/int32
> >>
> >> That's uint32. Why do you need negative values?
> > 
> > signed int32 is intentional. some chips have two physical input ports and measure a pressure differential in which case pmin is negative.
> > see either of the pdfs at page 14, table 8, column 2, row 7+
> 
> Then the best would be to change the type in other bindings to have
> int32 everywhere... but dtschema also requires unt32:
> https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/property-units.yaml

yeah, and also '-kpascal' might be too coarse of a unit when we talk about sensors that have a -125 .. 125 pascal measurement range as '0.5ND' has.

cheers,
peter
Rob Herring Nov. 20, 2023, 5:19 p.m. UTC | #9
On Sun, Nov 19, 2023 at 10:14:58PM +0200, Petre Rodan wrote:
> 
> Good morning!
> 
> On Sun, Nov 19, 2023 at 07:49:39AM -0600, Rob Herring wrote:
> > On Fri, Nov 17, 2023 at 09:22:57PM +0200, Petre Rodan wrote:
> > > Adds binding for digital Honeywell TruStability HSC and SSC series pressure 
> > > and temperature sensors.
> > > 
> [..]
> > > Changes for v2:
> > > - Removed redundant quotations reported by robh's bot
> > > - Fixed yamllint warnings
> > > 
> > > I'm failing to run 'make DT_CHECKER_FLAGS=-m dt_binding_check' due to
> > > python errors and exceptions
> > 
> > What exceptions?
> 
> thanks for asking.
> 
> first off, installed packages. the first 4 are not part of the 
> official Gentoo repo, so I might have prepared them with missing 
> options if any where not included by default.
> I know nothing about python.
> 
> $ equery l dtschema pylibfdt ruamel-yaml yamllint jsonschema python 
> [I-O] [  ] dev-python/dtschema-2023.9:0
> [I-O] [  ] dev-python/pylibfdt-1.7.0_p1:0
> [I-O] [  ] dev-python/ruamel-yaml-0.18.5:0
> [I-O] [  ] dev-python/yamllint-1.33.0:0
> [IP-] [  ] dev-python/jsonschema-4.19.1:0

4.18 and later are not supported.

Apparently behavior we relied on in pre-4.18 was "wrong" usage... 4.18 
also makes rust a hard dependency. That's a problem for any arch without 
LLVM support.

Installing via pip will check this dependency.

Rob
Jonathan Cameron Nov. 20, 2023, 5:39 p.m. UTC | #10
On Mon, 20 Nov 2023 16:40:51 +0200
Petre Rodan <petre.rodan@subdimension.ro> wrote:

> Hello!
> 
> On Mon, Nov 20, 2023 at 03:04:07PM +0100, Krzysztof Kozlowski wrote:
> > On 20/11/2023 14:42, Petre Rodan wrote:
> >   
> > >>> +properties:
> > >>> +  compatible:
> > >>> +    enum:
> > >>> +      - honeywell,hsc  
> > >>
> > >> Way too generic  
> > > 
> > > I'm new to this, please excuse my ignorance.
> > > my driver covers all Honeywell pressure sensors under the "TruStability board mount HSC/SSC" moniker.  
> > 
> > We talk here about bindings, not driver. For the driver you can use
> > whatever name is approved by reviewers of your driver.
> >   
> > > that is why my intention was to provide a rather generic name for the driver itself.
> > > are you afraid that they will come up with a different device that they will call "hsc" in the future?
> > > in this case honeywell,trustability-hsc would be fine?
> > > 
> > > as I see you prefer to target a particular chip, but I am a bit afraid that the end-user will be confused by needing to set up something like
> > > 
> > > pressure@28 {
> > > 	compatible = "honeywell,hsc030pa";  
> > 
> > The compatible should be specific, thus for example match exact model
> > number.  
> 
> there are an infinite number of combinations of 4 transfer functions and 118 ranges + one custom range, so providing an array with all specific chips that could end up as compatible is out of the question.
> I was aiming at providing a generic name for the binding and get the transfer function and the pressure range as required parameters.
> 
> > If you can guarantee that all devices from given family are the same in
> > respect of programming model and hardware requirements (e.g. supplies),
> > then you could go with family name. However such guarantees are rarely
> > given.  
> 
> I see your point.
> 
> > Therefore for mprls0025pa I agreed for using one specific model
> > for entire family.
> > 
> > https://lore.kernel.org/all/d577bc44-780f-f25d-29c6-ed1d353b540c@linaro.org/
> > 
> >   
> > > 	reg = <0x28>;
> > > 	honeywell,transfer-function = <0>;
> > > 	honeywell,pressure-range = "250MD";
> > > };
> > > 
> > > ie. specifying "hsc030pa" as driver while his chip is not in the 030PA range, but 250MD.
> > > 
> > > so do you prefer
> > >  honeywell,trustability-hsc  OR
> > >  honeywell,hsc030pa  
> > 
> > I think the latter, just like we did for mprls0025pa. How many devices
> > do you have there?  
> 
> both hsc and ssc have 118 ranges, 4 transfer functions and both can be requested from the manufacturer with custom measurement ranges.
> 
> ok,I will rename hsc->hsc030pa in the code as you requested.

Where does pa come from? 

If we are going generic, feels like trustability-ssc etc are more representative
and matches the datasheet cover page.


> 
> > >>> +  honeywell,pmin-pascal:
> > >>> +    description: |
> > >>> +      Minimum pressure value the sensor can measure in pascal.
> > >>> +      To be specified only if honeywell,range_str is set to "NA".
> > >>> +    $ref: /schemas/types.yaml#/definitions/int32  
> > >>
> > >> That's uint32. Why do you need negative values?  
> > > 
> > > signed int32 is intentional. some chips have two physical input ports and measure a pressure differential in which case pmin is negative.
> > > see either of the pdfs at page 14, table 8, column 2, row 7+  
> > 
> > Then the best would be to change the type in other bindings to have
> > int32 everywhere... but dtschema also requires unt32:
> > https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/property-units.yaml  
> 
> yeah, and also '-kpascal' might be too coarse of a unit when we talk about sensors that have a -125 .. 125 pascal measurement range as '0.5ND' has.
> 
> cheers,
> peter
>
Petre Rodan Nov. 20, 2023, 6:09 p.m. UTC | #11
hello!

On Mon, Nov 20, 2023 at 10:19:03AM -0700, Rob Herring wrote:
> > first off, installed packages. the first 4 are not part of the 
> > official Gentoo repo, so I might have prepared them with missing 
> > options if any where not included by default.
> > I know nothing about python.
> > 
> > $ equery l dtschema pylibfdt ruamel-yaml yamllint jsonschema python 
[..]
> > [IP-] [  ] dev-python/jsonschema-4.19.1:0
> 
> 4.18 and later are not supported.
> 
> Apparently behavior we relied on in pre-4.18 was "wrong" usage... 4.18 
> also makes rust a hard dependency. That's a problem for any arch without 
> LLVM support.
> 
> Installing via pip will check this dependency.

I confirm that installing ver 4.17 of jsonschema fixed all the exceptions.

thanks.
peter
Petre Rodan Nov. 20, 2023, 6:25 p.m. UTC | #12
Hello!

On Mon, Nov 20, 2023 at 05:39:29PM +0000, Jonathan Cameron wrote:
> On Mon, 20 Nov 2023 16:40:51 +0200
> Petre Rodan <petre.rodan@subdimension.ro> wrote:
> 
> > Hello!
> > 
> > On Mon, Nov 20, 2023 at 03:04:07PM +0100, Krzysztof Kozlowski wrote:
> > > On 20/11/2023 14:42, Petre Rodan wrote:
> > >   
> > > >>> +properties:
> > > >>> +  compatible:
> > > >>> +    enum:
> > > >>> +      - honeywell,hsc  
> > > >>
> > > >> Way too generic  
> > > > 
> > > > I'm new to this, please excuse my ignorance.
> > > > my driver covers all Honeywell pressure sensors under the "TruStability board mount HSC/SSC" moniker.  
> > > 
> > > We talk here about bindings, not driver. For the driver you can use
> > > whatever name is approved by reviewers of your driver.
> > >   
> > > > that is why my intention was to provide a rather generic name for the driver itself.
> > > > are you afraid that they will come up with a different device that they will call "hsc" in the future?
> > > > in this case honeywell,trustability-hsc would be fine?
> > > > 
> > > > as I see you prefer to target a particular chip, but I am a bit afraid that the end-user will be confused by needing to set up something like
> > > > 
> > > > pressure@28 {
> > > > 	compatible = "honeywell,hsc030pa";  
> > > 
> > > The compatible should be specific, thus for example match exact model
> > > number.  
> > 
> > there are an infinite number of combinations of 4 transfer functions and 118 ranges + one custom range, so providing an array with all specific chips that could end up as compatible is out of the question.
> > I was aiming at providing a generic name for the binding and get the transfer function and the pressure range as required parameters.
> > 
> > > If you can guarantee that all devices from given family are the same in
> > > respect of programming model and hardware requirements (e.g. supplies),
> > > then you could go with family name. However such guarantees are rarely
> > > given.  
> > 
> > I see your point.
> > 
> > > Therefore for mprls0025pa I agreed for using one specific model
> > > for entire family.
> > > 
> > > https://lore.kernel.org/all/d577bc44-780f-f25d-29c6-ed1d353b540c@linaro.org/
> > > 
> > >   
> > > > 	reg = <0x28>;
> > > > 	honeywell,transfer-function = <0>;
> > > > 	honeywell,pressure-range = "250MD";
> > > > };
> > > > 
> > > > ie. specifying "hsc030pa" as driver while his chip is not in the 030PA range, but 250MD.
> > > > 
> > > > so do you prefer
> > > >  honeywell,trustability-hsc  OR
> > > >  honeywell,hsc030pa  
> > > 
> > > I think the latter, just like we did for mprls0025pa. How many devices
> > > do you have there?  
> > 
> > both hsc and ssc have 118 ranges, 4 transfer functions and both can be requested from the manufacturer with custom measurement ranges.
> > 
> > ok,I will rename hsc->hsc030pa in the code as you requested.
> 
> Where does pa come from? 

honeywell,hsc030pa was provided as an equivalent to honeywell,mprls0025pa (which is already in the repo).

'030PA' and '0025PA' define the pressure range (0-30, 0-25), the unit of measure (Psi) and the measurement type (Absolute) for a particular chip in the honeywell catalog. (please ignore the psi part, we convert everything to pascals).
but both my driver and Andreas Klinger's mprls0025pa actually provide a generic abstraction layer for entire series of sensors.

> If we are going generic, feels like trustability-ssc etc are more representative
> and matches the datasheet cover page.

Krzysztof voted for non-generic, honeywell,mprls0025pa is already set up non-generic, my intent was to go generic.

I'll rewrite the code to whatever you guys feel is best.

peter
Jonathan Cameron Nov. 25, 2023, 7:19 p.m. UTC | #13
On Fri, 17 Nov 2023 21:22:57 +0200
Petre Rodan <petre.rodan@subdimension.ro> wrote:

> Adds binding for digital Honeywell TruStability HSC and SSC series pressure 
> and temperature sensors.
> 
> Datasheet:
>  [HSC] https://prod-edam.honeywell.com/content/dam/honeywell-edam/sps/siot/en-us/products/sensors/pressure-sensors/board-mount-pressure-sensors/trustability-hsc-series/documents/sps-siot-trustability-hsc-series-high-accuracy-board-mount-pressure-sensors-50099148-a-en-ciid-151133.pdf
>  [SSC] https://prod-edam.honeywell.com/content/dam/honeywell-edam/sps/siot/en-us/products/sensors/pressure-sensors/board-mount-pressure-sensors/trustability-ssc-series/documents/sps-siot-trustability-ssc-series-standard-accuracy-board-mount-pressure-sensors-50099533-a-en-ciid-151134.pdf
> 
> Signed-off-by: Petre Rodan <petre.rodan@subdimension.ro>

Hi Petre,

Please resend whole series when you do a new version.  I know that some
areas of the kernel do minor tweaks by reply to an earlier version but
in IIO we rely heavily on patchwork for tracking and it makes it very
hard to find the email.

Also, don't make that a reply to earlier version. The nesting of
remotely complex threads makes that impossible track once we have
a few versions posted.

Jonathan
Jonathan Cameron Nov. 25, 2023, 7:21 p.m. UTC | #14
On Mon, 20 Nov 2023 20:25:08 +0200
Petre Rodan <petre.rodan@subdimension.ro> wrote:

> Hello!
> 
> On Mon, Nov 20, 2023 at 05:39:29PM +0000, Jonathan Cameron wrote:
> > On Mon, 20 Nov 2023 16:40:51 +0200
> > Petre Rodan <petre.rodan@subdimension.ro> wrote:
> >   
> > > Hello!
> > > 
> > > On Mon, Nov 20, 2023 at 03:04:07PM +0100, Krzysztof Kozlowski wrote:  
> > > > On 20/11/2023 14:42, Petre Rodan wrote:
> > > >     
> > > > >>> +properties:
> > > > >>> +  compatible:
> > > > >>> +    enum:
> > > > >>> +      - honeywell,hsc    
> > > > >>
> > > > >> Way too generic    
> > > > > 
> > > > > I'm new to this, please excuse my ignorance.
> > > > > my driver covers all Honeywell pressure sensors under the "TruStability board mount HSC/SSC" moniker.    
> > > > 
> > > > We talk here about bindings, not driver. For the driver you can use
> > > > whatever name is approved by reviewers of your driver.
> > > >     
> > > > > that is why my intention was to provide a rather generic name for the driver itself.
> > > > > are you afraid that they will come up with a different device that they will call "hsc" in the future?
> > > > > in this case honeywell,trustability-hsc would be fine?
> > > > > 
> > > > > as I see you prefer to target a particular chip, but I am a bit afraid that the end-user will be confused by needing to set up something like
> > > > > 
> > > > > pressure@28 {
> > > > > 	compatible = "honeywell,hsc030pa";    
> > > > 
> > > > The compatible should be specific, thus for example match exact model
> > > > number.    
> > > 
> > > there are an infinite number of combinations of 4 transfer functions and 118 ranges + one custom range, so providing an array with all specific chips that could end up as compatible is out of the question.
> > > I was aiming at providing a generic name for the binding and get the transfer function and the pressure range as required parameters.
> > >   
> > > > If you can guarantee that all devices from given family are the same in
> > > > respect of programming model and hardware requirements (e.g. supplies),
> > > > then you could go with family name. However such guarantees are rarely
> > > > given.    
> > > 
> > > I see your point.
> > >   
> > > > Therefore for mprls0025pa I agreed for using one specific model
> > > > for entire family.
> > > > 
> > > > https://lore.kernel.org/all/d577bc44-780f-f25d-29c6-ed1d353b540c@linaro.org/
> > > > 
> > > >     
> > > > > 	reg = <0x28>;
> > > > > 	honeywell,transfer-function = <0>;
> > > > > 	honeywell,pressure-range = "250MD";
> > > > > };
> > > > > 
> > > > > ie. specifying "hsc030pa" as driver while his chip is not in the 030PA range, but 250MD.
> > > > > 
> > > > > so do you prefer
> > > > >  honeywell,trustability-hsc  OR
> > > > >  honeywell,hsc030pa    
> > > > 
> > > > I think the latter, just like we did for mprls0025pa. How many devices
> > > > do you have there?    
> > > 
> > > both hsc and ssc have 118 ranges, 4 transfer functions and both can be requested from the manufacturer with custom measurement ranges.
> > > 
> > > ok,I will rename hsc->hsc030pa in the code as you requested.  
> > 
> > Where does pa come from?   
> 
> honeywell,hsc030pa was provided as an equivalent to honeywell,mprls0025pa (which is already in the repo).
> 
> '030PA' and '0025PA' define the pressure range (0-30, 0-25), the unit of measure (Psi) and the measurement type (Absolute) for a particular chip in the honeywell catalog. (please ignore the psi part, we convert everything to pascals).
> but both my driver and Andreas Klinger's mprls0025pa actually provide a generic abstraction layer for entire series of sensors.

ah ok. That's fine then - searching the datasheet I found didn't include that particular
string, so I was rather confused.

I'm fine with specific now you've explained where it came from!

Jonathan

> 
> > If we are going generic, feels like trustability-ssc etc are more representative
> > and matches the datasheet cover page.  
> 
> Krzysztof voted for non-generic, honeywell,mprls0025pa is already set up non-generic, my intent was to go generic.
> 
> I'll rewrite the code to whatever you guys feel is best.
> 
> peter
> 
>
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml b/Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml
new file mode 100644
index 000000000000..c7e5d3bd5ef4
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml
@@ -0,0 +1,156 @@ 
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/pressure/honeywell,hsc030pa.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Honeywell TruStability HSC and SSC pressure sensor families
+
+description: |
+  support for Honeywell TruStability HSC and SSC digital pressure sensor
+  families.
+
+  These sensors have either an I2C, an SPI or an analog interface. Only the
+  digital versions are supported by this driver.
+
+  There are 118 models with different pressure ranges available in each family.
+  The vendor calls them "HSC series" and "SSC series". All of them have an
+  identical programming model but differ in pressure range, unit and transfer
+  function.
+
+  To support different models one need to specify the pressure range as well as
+  the transfer function. Pressure range can either be provided via range_str or
+  in case it's a custom chip via numerical range limits converted to pascals.
+
+  The transfer function defines the ranges of raw conversion values delivered
+  by the sensor. pmin-pascal and pmax-pascal corespond to the minimum and
+  maximum pressure that can be measured.
+
+  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/trustability-hsc-series/documents/sps-siot-trustability-hsc-series-high-accuracy-board-mount-pressure-sensors-50099148-a-en-ciid-151133.pdf
+  https://prod-edam.honeywell.com/content/dam/honeywell-edam/sps/siot/en-us/products/sensors/pressure-sensors/board-mount-pressure-sensors/trustability-ssc-series/documents/sps-siot-trustability-ssc-series-standard-accuracy-board-mount-pressure-sensors-50099533-a-en-ciid-151134.pdf
+
+maintainers:
+  - Petre Rodan <petre.rodan@subdimension.ro>
+
+properties:
+  compatible:
+    enum:
+      - honeywell,hsc
+      - honeywell,ssc
+
+  reg:
+    maxItems: 1
+
+  honeywell,transfer-function:
+    description: |
+      Transfer function which defines the range of valid values delivered by
+      the sensor.
+      0 - A, 10% to 90% of 2^14
+      1 - B, 5% to 95% of 2^14
+      2 - C, 5% to 85% of 2^14
+      3 - F, 4% to 94% of 2^14
+    $ref: /schemas/types.yaml#/definitions/uint32
+
+  honeywell,range_str:
+    description: |
+      Five character string that defines "pressure range, unit and type"
+      as part of the device nomenclature. In the unlikely case of a custom
+      chip, set to "NA" and provide honeywell,pmin-pascal honeywell,pmax-pascal
+    enum: [001BA, 1.6BA, 2.5BA, 004BA, 006BA, 010BA, 1.6MD, 2.5MD, 004MD,
+           006MD, 010MD, 016MD, 025MD, 040MD, 060MD, 100MD, 160MD, 250MD,
+           400MD, 600MD, 001BD, 1.6BD, 2.5BD, 004BD, 2.5MG, 004MG, 006MG,
+           010MG, 016MG, 025MG, 040MG, 060MG, 100MG, 160MG, 250MG, 400MG,
+           600MG, 001BG, 1.6BG, 2.5BG, 004BG, 006BG, 010BG, 100KA, 160KA,
+           250KA, 400KA, 600KA, 001GA, 160LD, 250LD, 400LD, 600LD, 001KD,
+           1.6KD, 2.5KD, 004KD, 006KD, 010KD, 016KD, 025KD, 040KD, 060KD,
+           100KD, 160KD, 250KD, 400KD, 250LG, 400LG, 600LG, 001KG, 1.6KG,
+           2.5KG, 004KG, 006KG, 010KG, 016KG, 025KG, 040KG, 060KG, 100KG,
+           160KG, 250KG, 400KG, 600KG, 001GG, 015PA, 030PA, 060PA, 100PA,
+           150PA, 0.5ND, 001ND, 002ND, 004ND, 005ND, 010ND, 020ND, 030ND,
+           001PD, 005PD, 015PD, 030PD, 060PD, 001NG, 002NG, 004NG, 005NG,
+           010NG, 020NG, 030NG, 001PG, 005PG, 015PG, 030PG, 060PG, 100PG,
+           150PG, NA]
+    $ref: /schemas/types.yaml#/definitions/string
+
+  honeywell,pmin-pascal:
+    description: |
+      Minimum pressure value the sensor can measure in pascal.
+      To be specified only if honeywell,range_str is set to "NA".
+    $ref: /schemas/types.yaml#/definitions/int32
+
+  honeywell,pmax-pascal:
+    description: |
+      Maximum pressure value the sensor can measure in pascal.
+      To be specified only if honeywell,range_str is set to "NA".
+    $ref: /schemas/types.yaml#/definitions/int32
+
+  vdd-supply:
+    description: |
+      Provide VDD power to the sensor (either 3.3V or 5V depending on the chip).
+      Optional, activate only if required by the target board.
+
+  spi-max-frequency:
+    description: SPI clock to be kept between 50 and 800kHz
+
+  clock-frequency:
+    description: i2c clock to be kept between 100 and 400kHz
+
+required:
+  - compatible
+  - reg
+  - honeywell,transfer-function
+  - honeywell,range_str
+  - clock-frequency
+  - spi-max-frequency
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+    i2c {
+        status = "okay";
+        clock-frequency = <400000>;
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        HSCMRNN030PA2A3@28 {
+          status = "okay";
+          compatible = "honeywell,hsc";
+          reg = <0x28>;
+
+          honeywell,transfer-function = <0>;
+          honeywell,range_str = "030PA";
+        };
+    };
+
+    spi {
+        # note that MOSI is not required by this sensor
+        status = "okay";
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        HSCMLNN100PASA3@0 {
+          status = "okay";
+          compatible = "honeywell,hsc";
+          reg = <0>;
+          spi-max-frequency = <800000>;
+
+          honeywell,transfer-function = <0>;
+          honeywell,range_str = "100PA";
+        };
+
+        HSC_CUSTOM_CHIP@0 {
+          status = "okay";
+          compatible = "honeywell,hsc";
+          reg = <1>;
+          spi-max-frequency = <800000>;
+
+          honeywell,transfer-function = <0>;
+          honeywell,range_str = "NA";
+          honeywell,pmin-pascal = <0>;
+          honeywell,pmax-pascal = <206850>;
+        };
+
+    };