| Message ID | 20231224143500.10940-3-petre.rodan@subdimension.ro (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | changes to mprls0025pa | expand |
On 24/12/2023 15:34, Petre Rodan wrote: > Change order of properties in order for the end user to de-prioritize > pmin-pascal and pmax-pascal which are superseded by pressure-triplet. > > Add pressure-triplet property which automatically initializes > pmin-pascal and pmax-pascal inside the driver > > Rework honeywell,pmXX-pascal requirements based on feedback from > Jonathan and Conor. > > Signed-off-by: Petre Rodan <petre.rodan@subdimension.ro> > Signed-off-by: Andreas Klinger <ak@it-klinger.de> > --- > .../iio/pressure/honeywell,mprls0025pa.yaml | 64 ++++++++++++++----- > 1 file changed, 47 insertions(+), 17 deletions(-) > > diff --git a/Documentation/devicetree/bindings/iio/pressure/honeywell,mprls0025pa.yaml b/Documentation/devicetree/bindings/iio/pressure/honeywell,mprls0025pa.yaml > index 84ced4e5a7da..e4021306d187 100644 > --- a/Documentation/devicetree/bindings/iio/pressure/honeywell,mprls0025pa.yaml > +++ b/Documentation/devicetree/bindings/iio/pressure/honeywell,mprls0025pa.yaml > @@ -19,14 +19,17 @@ description: | > calls them "mpr series". All of them have the identical programming model and > differ in the 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 needs to be converted from its unit to > + To support different models one need to specify its pressure triplet as well > + as the transfer function. > + > + For custom models the pressure values can alternatively be specified manually. > + The minimal range value stands for the minimum pressure and the maximum value > + also for the maximum pressure with linear relation inside the range. > + Pressure range needs to be converted from the datasheet specified unit to > pascal. > > The transfer function defines the ranges of numerical values delivered by the > - sensor. The minimal range value stands for the minimum pressure and the > - maximum value also for the maximum pressure with linear relation inside the > - range. > + sensor. > > Specifications about the devices can be found at: > https://prod-edam.honeywell.com/content/dam/honeywell-edam/sps/siot/en-us/ > @@ -54,14 +57,6 @@ properties: > If not present the device is not reset during the probe. > maxItems: 1 > > - honeywell,pmin-pascal: > - description: > - Minimum pressure value the sensor can measure in pascal. > - > - honeywell,pmax-pascal: > - description: > - Maximum pressure value the sensor can measure in pascal. > - > honeywell,transfer-function: > description: | > Transfer function which defines the range of valid values delivered by the > @@ -72,17 +67,52 @@ properties: > enum: [1, 2, 3] > $ref: /schemas/types.yaml#/definitions/uint32 > > + honeywell,pressure-triplet: Why not putting it just before existing properties? > + description: | > + Case-sensitive five character string that defines pressure range, unit > + and type as part of the device nomenclature. In the unlikely case of a > + custom chip, unset and provide pmin-pascal and pmax-pascal instead. > + enum: [0001BA, 01.6BA, 02.5BA, 0060MG, 0100MG, 0160MG, 0250MG, 0400MG, > + 0600MG, 0001BG, 01.6BG, 02.5BG, 0100KA, 0160KA, 0250KA, 0006KG, > + 0010KG, 0016KG, 0025KG, 0040KG, 0060KG, 0100KG, 0160KG, 0250KG, > + 0015PA, 0025PA, 0030PA, 0001PG, 0005PG, 0015PG, 0030PG, 0300YG] > + $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,pressure-triplet is not set. The last sentence is redundant - schema should enforce that. > + > + honeywell,pmax-pascal: > + description: > + Maximum pressure value the sensor can measure in pascal. > + To be specified only if honeywell,pressure-triplet is not set. > + > vdd-supply: > description: provide VDD power to the sensor. > > required: > - compatible > - reg > - - honeywell,pmin-pascal > - - honeywell,pmax-pascal > - honeywell,transfer-function > - vdd-supply > > +oneOf: > + - required: > + - honeywell,pmin-pascal > + - honeywell,pmax-pascal > + - required: > + - honeywell,pressure-triplet > + > +allOf: > + - if: > + required: > + - honeywell,pressure-triplet > + then: > + properties: > + honeywell,pmin-pascal: false > + honeywell,pmax-pascal: false This allOf is not needed. Best regards, Krzysztof
hello Krzysztof, On Mon, Dec 25, 2023 at 01:57:39PM +0100, Krzysztof Kozlowski wrote: > On 24/12/2023 15:34, Petre Rodan wrote: > > @@ -54,14 +57,6 @@ properties: > > If not present the device is not reset during the probe. > > maxItems: 1 > > > > - honeywell,pmin-pascal: > > - description: > > - Minimum pressure value the sensor can measure in pascal. > > - > > - honeywell,pmax-pascal: > > - description: > > - Maximum pressure value the sensor can measure in pascal. > > - > > honeywell,transfer-function: > > description: | > > Transfer function which defines the range of valid values delivered by the > > @@ -72,17 +67,52 @@ properties: > > enum: [1, 2, 3] > > $ref: /schemas/types.yaml#/definitions/uint32 > > > > + honeywell,pressure-triplet: > > Why not putting it just before existing properties? I'd like to have pmin-pascal, pmax-pascal as the last two honeywell specific properties, since they are not to be used unless someone has custom silicon. so we will still have a block moved just like above. the most logic order is the one I proposed above: honeywell,transfer-function: [..] honeywell,pressure-triplet: [..] honeywell,pmin-pascal: [..] honeywell,pmax-pascal: [..] since the last 3 are tied together as we will see below. is there any reason you want this order to change? > > + honeywell,pmin-pascal: > > + description: > > + Minimum pressure value the sensor can measure in pascal. > > + To be specified only if honeywell,pressure-triplet is not set. > > The last sentence is redundant - schema should enforce that. when someone generates the dtbo files via cpp -nostdinc -I include -I ${LINUX_SRC}/include/ -I arch -undef -x assembler-with-cpp ${file}.dts "${BUILD_DIR}/${file}.dts.preprocessed" dtc -@ -I dts -O dtb -o "${BUILD_DIR}/${file}.dtbo" "${BUILD_DIR}/${file}.dts.preprocessed" the schema is not checked in any way. so unless people can be bothered to understand the yaml intricacies in the bindings file, I feel they need to see that redundant information there, see below. > > +oneOf: > > + - required: > > + - honeywell,pmin-pascal > > + - honeywell,pmax-pascal > > + - required: > > + - honeywell,pressure-triplet > > + > > +allOf: > > + - if: > > + required: > > + - honeywell,pressure-triplet > > + then: > > + properties: > > + honeywell,pmin-pascal: false > > + honeywell,pmax-pascal: false > > This allOf is not needed. speaking for intricacies, if the allOf is removed, then a binding containing honeywell,pmax-pascal = <840000>; honeywell,pressure-triplet = "0015PA"; would be considered to be correct by the schema, but that would be the incorrect result. so afaict allOf needs to stay, and so does the redundant text. best regards, peter
On 25/12/2023 14:23, Petre Rodan wrote: > > hello Krzysztof, > > On Mon, Dec 25, 2023 at 01:57:39PM +0100, Krzysztof Kozlowski wrote: >> On 24/12/2023 15:34, Petre Rodan wrote: >>> @@ -54,14 +57,6 @@ properties: >>> If not present the device is not reset during the probe. >>> maxItems: 1 >>> >>> - honeywell,pmin-pascal: >>> - description: >>> - Minimum pressure value the sensor can measure in pascal. >>> - >>> - honeywell,pmax-pascal: >>> - description: >>> - Maximum pressure value the sensor can measure in pascal. >>> - >>> honeywell,transfer-function: >>> description: | >>> Transfer function which defines the range of valid values delivered by the >>> @@ -72,17 +67,52 @@ properties: >>> enum: [1, 2, 3] >>> $ref: /schemas/types.yaml#/definitions/uint32 >>> >>> + honeywell,pressure-triplet: >> >> Why not putting it just before existing properties? > > I'd like to have pmin-pascal, pmax-pascal as the last two honeywell specific > properties, since they are not to be used unless someone has custom silicon. > so we will still have a block moved just like above. > the most logic order is the one I proposed above: > > honeywell,transfer-function: > [..] > honeywell,pressure-triplet: > [..] > honeywell,pmin-pascal: > [..] > honeywell,pmax-pascal: > [..] > > since the last 3 are tied together as we will see below. > is there any reason you want this order to change? I just don't get why moving the code instead of adding new property next to them. The order is often alphabetical. > >>> + honeywell,pmin-pascal: >>> + description: >>> + Minimum pressure value the sensor can measure in pascal. >>> + To be specified only if honeywell,pressure-triplet is not set. >> >> The last sentence is redundant - schema should enforce that. > > when someone generates the dtbo files via > > cpp -nostdinc -I include -I ${LINUX_SRC}/include/ -I arch -undef -x assembler-with-cpp ${file}.dts "${BUILD_DIR}/${file}.dts.preprocessed" > dtc -@ -I dts -O dtb -o "${BUILD_DIR}/${file}.dtbo" "${BUILD_DIR}/${file}.dts.preprocessed" And how this command matters? DT overlays are checked, so error is printed. > > the schema is not checked in any way. When I run `make` the schema is also not checked, so is it an argument to add anything to the binding? No. Drop redundant text. > so unless people can be bothered to understand the yaml intricacies in the > bindings file, I feel they need to see that redundant information there, see below. > >>> +oneOf: >>> + - required: >>> + - honeywell,pmin-pascal >>> + - honeywell,pmax-pascal >>> + - required: >>> + - honeywell,pressure-triplet >>> + >>> +allOf: >>> + - if: >>> + required: >>> + - honeywell,pressure-triplet >>> + then: >>> + properties: >>> + honeywell,pmin-pascal: false >>> + honeywell,pmax-pascal: false >> >> This allOf is not needed. > > speaking for intricacies, if the allOf is removed, then a binding containing > > honeywell,pmax-pascal = <840000>; > honeywell,pressure-triplet = "0015PA"; > > would be considered to be correct by the schema, but that would be the incorrect > result. so afaict allOf needs to stay, and so does the redundant text. Really? Did you test it? Best regards, Krzysztof
On 25/12/2023 14:34, Krzysztof Kozlowski wrote: >>>> +oneOf: >>>> + - required: >>>> + - honeywell,pmin-pascal >>>> + - honeywell,pmax-pascal >>>> + - required: >>>> + - honeywell,pressure-triplet >>>> + >>>> +allOf: >>>> + - if: >>>> + required: >>>> + - honeywell,pressure-triplet >>>> + then: >>>> + properties: >>>> + honeywell,pmin-pascal: false >>>> + honeywell,pmax-pascal: false >>> >>> This allOf is not needed. >> >> speaking for intricacies, if the allOf is removed, then a binding containing >> >> honeywell,pmax-pascal = <840000>; >> honeywell,pressure-triplet = "0015PA"; >> >> would be considered to be correct by the schema, but that would be the incorrect >> result. so afaict allOf needs to stay, and so does the redundant text. > > Really? Did you test it? Hm, indeed, on pmin/pmax would not trigger first required case. OK, then this part make sense. Best regards, Krzysztof
hello, On Mon, Dec 25, 2023 at 02:34:04PM +0100, Krzysztof Kozlowski wrote: > On 25/12/2023 14:23, Petre Rodan wrote: > > honeywell,transfer-function: > > [..] > > honeywell,pressure-triplet: > > [..] > > honeywell,pmin-pascal: > > [..] > > honeywell,pmax-pascal: > > [..] > > > > since the last 3 are tied together as we will see below. > > is there any reason you want this order to change? > > I just don't get why moving the code instead of adding new property next > to them. as I also said in the comments and in my last reply I want the user to not feel in any way obliged to fill in pmin-pascal, pmax-pascal. and since a user reads this file from top to bottom, the order in which these properties are shown to him is important, and it is the one above. > The order is often alphabetical. can we please make an exception? > >>> + honeywell,pmin-pascal: > >>> + description: > >>> + Minimum pressure value the sensor can measure in pascal. > >>> + To be specified only if honeywell,pressure-triplet is not set. > >> > >> The last sentence is redundant - schema should enforce that. > > > > when someone generates the dtbo files via > > > > cpp -nostdinc -I include -I ${LINUX_SRC}/include/ -I arch -undef -x assembler-with-cpp ${file}.dts "${BUILD_DIR}/${file}.dts.preprocessed" > > dtc -@ -I dts -O dtb -o "${BUILD_DIR}/${file}.dtbo" "${BUILD_DIR}/${file}.dts.preprocessed" > > And how this command matters? DT overlays are checked, so error is printed. > > > > > the schema is not checked in any way. > > When I run `make` the schema is also not checked, so is it an argument > to add anything to the binding? No. Drop redundant text. > > > so unless people can be bothered to understand the yaml intricacies in the > > bindings file, I feel they need to see that redundant information there, see below. > > > > > > >>> +oneOf: > >>> + - required: > >>> + - honeywell,pmin-pascal > >>> + - honeywell,pmax-pascal > >>> + - required: > >>> + - honeywell,pressure-triplet > >>> + > >>> +allOf: > >>> + - if: > >>> + required: > >>> + - honeywell,pressure-triplet > >>> + then: > >>> + properties: > >>> + honeywell,pmin-pascal: false > >>> + honeywell,pmax-pascal: false > >> > >> This allOf is not needed. > > > > speaking for intricacies, if the allOf is removed, then a binding containing > > > > honeywell,pmax-pascal = <840000>; > > honeywell,pressure-triplet = "0015PA"; > > > > would be considered to be correct by the schema, but that would be the incorrect > > result. so afaict allOf needs to stay, and so does the redundant text. > > Really? Did you test it? for more hours than I would have liked. the allOf was provided with kindness by Conor in my first revision. testing it: 1. invalid yaml with both honeywell,pmax-pascal and honeywell,pressure-triplet defined passes the schema check if the allOf is removed: # make DT_SCHEMA_FILES=Documentation/devicetree/bindings/iio/pressure/honeywell,mprls0025pa.yaml DT_CHECKER_FLAGS=-m dt_binding_check # echo $? 0 2. the same invalid yaml but with the allOf not removed issues this output: [..]/Documentation/devicetree/bindings/iio/pressure/honeywell,mprls0025pa.example.dtb: pressure@18: honeywell,pmax-pascal: False schema does not allow [[84000]] from schema $id: http://devicetree.org/schemas/iio/pressure/honeywell,mprls0025pa.yaml# which is the expected behaviour. so AFAICT the allOf block is required, as well as the redundant text for the humans that read the human-readable parts of the bindings file. invalid yaml example used above: #include <dt-bindings/gpio/gpio.h> #include <dt-bindings/interrupt-controller/irq.h> i2c { #address-cells = <1>; #size-cells = <0>; pressure@18 { compatible = "honeywell,mprls0025pa"; reg = <0x18>; reset-gpios = <&gpio3 19 GPIO_ACTIVE_HIGH>; interrupt-parent = <&gpio3>; interrupts = <21 IRQ_TYPE_EDGE_RISING>; honeywell,pmax-pascal = <84000>; honeywell,pressure-triplet = "0025PA"; honeywell,transfer-function = <1>; vdd-supply = <&vcc_3v3>; }; }; best regards, peter
diff --git a/Documentation/devicetree/bindings/iio/pressure/honeywell,mprls0025pa.yaml b/Documentation/devicetree/bindings/iio/pressure/honeywell,mprls0025pa.yaml index 84ced4e5a7da..e4021306d187 100644 --- a/Documentation/devicetree/bindings/iio/pressure/honeywell,mprls0025pa.yaml +++ b/Documentation/devicetree/bindings/iio/pressure/honeywell,mprls0025pa.yaml @@ -19,14 +19,17 @@ description: | calls them "mpr series". All of them have the identical programming model and differ in the 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 needs to be converted from its unit to + To support different models one need to specify its pressure triplet as well + as the transfer function. + + For custom models the pressure values can alternatively be specified manually. + The minimal range value stands for the minimum pressure and the maximum value + also for the maximum pressure with linear relation inside the range. + Pressure range needs to be converted from the datasheet specified unit to pascal. The transfer function defines the ranges of numerical values delivered by the - sensor. The minimal range value stands for the minimum pressure and the - maximum value also for the maximum pressure with linear relation inside the - range. + sensor. Specifications about the devices can be found at: https://prod-edam.honeywell.com/content/dam/honeywell-edam/sps/siot/en-us/ @@ -54,14 +57,6 @@ properties: If not present the device is not reset during the probe. maxItems: 1 - honeywell,pmin-pascal: - description: - Minimum pressure value the sensor can measure in pascal. - - honeywell,pmax-pascal: - description: - Maximum pressure value the sensor can measure in pascal. - honeywell,transfer-function: description: | Transfer function which defines the range of valid values delivered by the @@ -72,17 +67,52 @@ properties: enum: [1, 2, 3] $ref: /schemas/types.yaml#/definitions/uint32 + honeywell,pressure-triplet: + description: | + Case-sensitive five character string that defines pressure range, unit + and type as part of the device nomenclature. In the unlikely case of a + custom chip, unset and provide pmin-pascal and pmax-pascal instead. + enum: [0001BA, 01.6BA, 02.5BA, 0060MG, 0100MG, 0160MG, 0250MG, 0400MG, + 0600MG, 0001BG, 01.6BG, 02.5BG, 0100KA, 0160KA, 0250KA, 0006KG, + 0010KG, 0016KG, 0025KG, 0040KG, 0060KG, 0100KG, 0160KG, 0250KG, + 0015PA, 0025PA, 0030PA, 0001PG, 0005PG, 0015PG, 0030PG, 0300YG] + $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,pressure-triplet is not set. + + honeywell,pmax-pascal: + description: + Maximum pressure value the sensor can measure in pascal. + To be specified only if honeywell,pressure-triplet is not set. + vdd-supply: description: provide VDD power to the sensor. required: - compatible - reg - - honeywell,pmin-pascal - - honeywell,pmax-pascal - honeywell,transfer-function - vdd-supply +oneOf: + - required: + - honeywell,pmin-pascal + - honeywell,pmax-pascal + - required: + - honeywell,pressure-triplet + +allOf: + - if: + required: + - honeywell,pressure-triplet + then: + properties: + honeywell,pmin-pascal: false + honeywell,pmax-pascal: false + additionalProperties: false examples: @@ -99,8 +129,8 @@ examples: reset-gpios = <&gpio3 19 GPIO_ACTIVE_HIGH>; interrupt-parent = <&gpio3>; interrupts = <21 IRQ_TYPE_EDGE_RISING>; - honeywell,pmin-pascal = <0>; - honeywell,pmax-pascal = <172369>; + + honeywell,pressure-triplet = "0025PA"; honeywell,transfer-function = <1>; vdd-supply = <&vcc_3v3>; };