dt-bindings: iio: pressure: Support Honeywell mpr sensors
Subsystems (2)
PATCH 1/3 dt-bindings: iio: pressure: Support Honeywell mpr sensors
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.
--- .../bindings/iio/pressure/honeywell,mpr.yaml | 74 +++++++++++++++++++ 1 file changed, 74 insertions(+) create mode 100644 Documentation/devicetree/bindings/iio/pressure/honeywell,mpr.yaml
Documentation/devicetree/bindings/iio/pressure/honeywell,mpr.yaml new file +74 -0
@@ -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 <[email protected]> + +description: | + Honeywell pressure sensor of type mpr. This sensor has an I2C and SPI interface. Only the I2C + 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 + +properties: + compatible: + const: honeywell,mpr + + reg: + maxItems: 1 + + interrupts: + maxItems: 1 + + reset-gpios: + description: + Optional GPIO for resetting the device. If not present the device is not resetted. + maxItems: 1 + + honeywell,pmin: + description: + Minimum pressure value the sensor can measure in 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 + +required: + - compatible + - reg + - honeywell,pmin + - honeywell,pmax + +additionalProperties: false + +examples: + - | + #include <dt-bindings/gpio/gpio.h> + #include <dt-bindings/interrupt-controller/irq.h> + i2c { + #address-cells = <1>; + #size-cells = <0>; + + pressure@18 { + compatible = "honeywell,mpr"; + reg = <0x18>; + reset-gpios = <&gpio3 19 GPIO_ACTIVE_HIGH>; + interrupt-parent = <&gpio3>; + interrupts = <21 IRQ_TYPE_EDGE_FALLING>; + honeywell,pmin = <0>; + honeywell,pmax = <172369>; + }; + };
Signature · 1 line
2.30.2
Quoted 22 lines · Andreas Klinger
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 <[email protected]>--- .../bindings/iio/pressure/honeywell,mpr.yaml | 74 +++++++++++++++++++ 1 file changed, 74 insertions(+) create mode 100644 Documentation/devicetree/bindings/iio/pressure/honeywell,mpr.yamlDocumentation/devicetree/bindings/iio/pressure/honeywell,mpr.yaml new file +3
-0@@ -0,0 +1,74 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +---
Documentation/devicetree/bindings/iio/pressure/honeywell,mpr.yaml line 13
Quoted 10 lines
Documentation/devicetree/bindings/iio/pressure/honeywell,mpr.yaml new file +10
-0+$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 <[email protected]> + +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).
Documentation/devicetree/bindings/iio/pressure/honeywell,mpr.yaml line 22
Quoted 9 lines
Documentation/devicetree/bindings/iio/pressure/honeywell,mpr.yaml new file +9
-0+ 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 \
Documentation/devicetree/bindings/iio/pressure/honeywell,mpr.yaml line 26
Quoted 4 lines
Documentation/devicetree/bindings/iio/pressure/honeywell,mpr.yaml new file +4
-0+ +properties: + compatible: + const: honeywell,mpr
You need device specific compatible, not some generic one. Rename also then the filename (should match the compatible).
Documentation/devicetree/bindings/iio/pressure/honeywell,mpr.yaml line 36
Quoted 10 lines
Documentation/devicetree/bindings/iio/pressure/honeywell,mpr.yaml new file +10
-0+ + 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?
Documentation/devicetree/bindings/iio/pressure/honeywell,mpr.yaml line 41
Quoted 5 lines
Documentation/devicetree/bindings/iio/pressure/honeywell,mpr.yaml new file +5
-0+ 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
Documentation/devicetree/bindings/iio/pressure/honeywell,mpr.yaml line 47
Quoted 6 lines
Documentation/devicetree/bindings/iio/pressure/honeywell,mpr.yaml new file +6
-0+ $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? Does it depend on type of sensor (thus it is implied from compatible) or on system setup?
Documentation/devicetree/bindings/iio/pressure/honeywell,mpr.yaml line 54
Quoted 7 lines
Documentation/devicetree/bindings/iio/pressure/honeywell,mpr.yaml new file +7
-0+ +required: + - compatible + - reg + - honeywell,pmin + - honeywell,pmax +
Best regards,
Krzysztof
Jonathan Cameron
maintainerQuoted 85 lines · On Sat, 1 Apr 2023 11:42:15 +0200 Krzysztof Kozlowski…
Quoted 22 lines · Andreas Klinger
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 <[email protected]>--- .../bindings/iio/pressure/honeywell,mpr.yaml | 74 +++++++++++++++++++ 1 file changed, 74 insertions(+) create mode 100644 Documentation/devicetree/bindings/iio/pressure/honeywell,mpr.yamlDocumentation/devicetree/bindings/iio/pressure/honeywell,mpr.yaml new file +3
-0@@ -0,0 +1,74 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +---Documentation/devicetree/bindings/iio/pressure/honeywell,mpr.yaml line 13
Quoted 10 lines
Documentation/devicetree/bindings/iio/pressure/honeywell,mpr.yaml new file +10
-0+$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 <[email protected]> + +description: | + Honeywell pressure sensor of type mpr. This sensor has an I2C and SPI interface. Only the I2CDoesn't look wrapped according to Linux coding style (see Coding style).
Documentation/devicetree/bindings/iio/pressure/honeywell,mpr.yaml line 22
Quoted 9 lines
Documentation/devicetree/bindings/iio/pressure/honeywell,mpr.yaml new file +9
-0+ 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.pdfLines are not continued, so drop \
Documentation/devicetree/bindings/iio/pressure/honeywell,mpr.yaml line 26
Quoted 4 lines
Documentation/devicetree/bindings/iio/pressure/honeywell,mpr.yaml new file +4
-0+ +properties: + compatible: + const: honeywell,mprYou need device specific compatible, not some generic one. Rename also then the filename (should match the compatible).
Documentation/devicetree/bindings/iio/pressure/honeywell,mpr.yaml line 36
Quoted 10 lines
Documentation/devicetree/bindings/iio/pressure/honeywell,mpr.yaml new file +10
-0+ + 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?
Documentation/devicetree/bindings/iio/pressure/honeywell,mpr.yaml line 41
Quoted 5 lines
Documentation/devicetree/bindings/iio/pressure/honeywell,mpr.yaml new file +5
-0+ 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.yamlDocumentation/devicetree/bindings/iio/pressure/honeywell,mpr.yaml line 47
Quoted 6 lines
Documentation/devicetree/bindings/iio/pressure/honeywell,mpr.yaml new file +6
-0+ $ref: /schemas/types.yaml#/definitions/uint32 + + honeywell,pmax: + description: + Maximum pressure value the sensor can measure in pascal. + $ref: /schemas/types.yaml#/definitions/uint32
Quoted 4 lines
Same.
Why these values are suitable for DT? Does it depend on type of sensor (thus it is implied from compatible) or on system setup?
I think we'll end up with a lot of compatibles, but that's still better than free form description. May still need these as well though given the datasheet helpfully adds a foot note.
1. Custom pressure ranges are available.
Might not be worth including all the details though but unhelpfully the bits we care about are after details like is the gel food grade or the port long. Definitely can ignore the encoding of i2c address / spi in the last few bits but may need the transfer function.
mpr-0025GA-A maybe as a form?
Quoted 13 lines
Documentation/devicetree/bindings/iio/pressure/honeywell,mpr.yaml line 54
Quoted 8 lines
Documentation/devicetree/bindings/iio/pressure/honeywell,mpr.yaml new file +7
-0+ +required: + - compatible + - reg + - honeywell,pmin + - honeywell,pmax +Best regards,
Krzysztof
Hi,
thanks to Krzysztof, Lars-Peter and Jonathan for the review and suggestions. I have one thing to clarify. See below.
Jonathan Cameron <[email protected]> schrieb am Sa, 01. Apr 16:27:
Quoted 4 lines
Quoted 1 line · On Sat, 1 Apr 2023 11:42:15 +0200 Krzysztof Kozlowski…
On 01/04/2023 11:09, Andreas Klinger wrote:
[...]
Quoted 25 lines
Quoted 14 lines
Documentation/devicetree/bindings/iio/pressure/honeywell,mpr.yaml line 41
Quoted 3 lines
Documentation/devicetree/bindings/iio/pressure/honeywell,mpr.yaml new file +3
-0+ 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.yamlDocumentation/devicetree/bindings/iio/pressure/honeywell,mpr.yaml line 47
Quoted 6 lines
Documentation/devicetree/bindings/iio/pressure/honeywell,mpr.yaml new file +6
-0+ $ref: /schemas/types.yaml#/definitions/uint32 + + honeywell,pmax: + description: + Maximum pressure value the sensor can measure in pascal. + $ref: /schemas/types.yaml#/definitions/uint32Quoted 4 lines
Same.
Why these values are suitable for DT? Does it depend on type of sensor (thus it is implied from compatible) or on system setup?
I think we'll end up with a lot of compatibles, but that's still better than free form description. May still need these as well though given the datasheet helpfully adds a foot note.
1. Custom pressure ranges are available.
Quoted 7 lines
Might not be worth including all the details though but unhelpfully the bits we care about are after details like is the gel food grade or the port long. Definitely can ignore the encoding of i2c address / spi in the last few bits but may need the transfer function.
mpr-0025GA-A maybe as a form?
Just to clarify: There are 32 different pressure ranges and 3 transfer functions which means we'll end up with 96 compatibles and 96 I2C ids.
Would it be an option to have only one dt compatible and to add the pressure range as dt property?
e. g.: honeywell,range = "0025PA";
But because of "Custom pressure ranges" we still need the DT properties. In this case there's another "mpr-custom" compatible, right?
Andreas
Quoted 42 lines · Andreas Klinger
Hi,
thanks to Krzysztof, Lars-Peter and Jonathan for the review and suggestions. I have one thing to clarify. See below.
Jonathan Cameron <[email protected]> schrieb am Sa, 01. Apr 16:27:
Quoted 4 lines
Quoted 1 line · On Sat, 1 Apr 2023 11:42:15 +0200 Krzysztof Kozlowski…
On 01/04/2023 11:09, Andreas Klinger wrote:
[...]
Quoted 31 lines
Quoted 14 lines
Documentation/devicetree/bindings/iio/pressure/honeywell,mpr.yaml line 41
Quoted 3 lines
Documentation/devicetree/bindings/iio/pressure/honeywell,mpr.yaml new file +3
-0+ 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.yamlDocumentation/devicetree/bindings/iio/pressure/honeywell,mpr.yaml line 47
Quoted 6 lines
Documentation/devicetree/bindings/iio/pressure/honeywell,mpr.yaml new file +6
-0+ $ref: /schemas/types.yaml#/definitions/uint32 + + honeywell,pmax: + description: + Maximum pressure value the sensor can measure in pascal. + $ref: /schemas/types.yaml#/definitions/uint32Quoted 4 lines
Same.
Why these values are suitable for DT? Does it depend on type of sensor (thus it is implied from compatible) or on system setup?
I think we'll end up with a lot of compatibles, but that's still better than free form description. May still need these as well though given the datasheet helpfully adds a foot note.
1. Custom pressure ranges are available.
Might not be worth including all the details though but unhelpfully the bits we care about are after details like is the gel food grade or the port long. Definitely can ignore the encoding of i2c address / spi in the last few bits but may need the transfer function.
Quoted 4 lines
Quoted 1 line
mpr-0025GA-A maybe as a form?
Just to clarify: There are 32 different pressure ranges and 3 transfer functions which means we'll end up with 96 compatibles and 96 I2C ids.
You anyway need compatibles per devices, don't you?
https://elixir.bootlin.com/linux/v6.1-rc1/source/Documentation/devicetree/bindings/writing-bindings.rst#L42
I already commented on this.
Quoted 4 lines
Would it be an option to have only one dt compatible and to add the pressure range as dt property?
e. g.: honeywell,range = "0025PA";
Did you just decided to ignore my comment?
Quoted 3 lines
But because of "Custom pressure ranges" we still need the DT properties. In this case there's another "mpr-custom" compatible, right?
Please go to my email and respond to comments.
Best regards,
Krzysztof
Hi Krzysztof,
Krzysztof Kozlowski <[email protected]> schrieb am Sa, 01. Apr 11:42:
Quoted 69 lines · Andreas Klinger
Quoted 22 lines
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 <[email protected]>--- .../bindings/iio/pressure/honeywell,mpr.yaml | 74 +++++++++++++++++++ 1 file changed, 74 insertions(+) create mode 100644 Documentation/devicetree/bindings/iio/pressure/honeywell,mpr.yamlDocumentation/devicetree/bindings/iio/pressure/honeywell,mpr.yaml new file +3
-0@@ -0,0 +1,74 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +---Documentation/devicetree/bindings/iio/pressure/honeywell,mpr.yaml line 13
Quoted 10 lines
Documentation/devicetree/bindings/iio/pressure/honeywell,mpr.yaml new file +10
-0+$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 <[email protected]> + +description: | + Honeywell pressure sensor of type mpr. This sensor has an I2C and SPI interface. Only the I2CDoesn't look wrapped according to Linux coding style (see Coding style).
Documentation/devicetree/bindings/iio/pressure/honeywell,mpr.yaml line 22
Quoted 9 lines
Documentation/devicetree/bindings/iio/pressure/honeywell,mpr.yaml new file +9
-0+ 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.pdfLines are not continued, so drop \
Documentation/devicetree/bindings/iio/pressure/honeywell,mpr.yaml line 26
Quoted 4 lines
Documentation/devicetree/bindings/iio/pressure/honeywell,mpr.yaml new file +4
-0+ +properties: + compatible: + const: honeywell,mprYou need device specific compatible, not some generic one. Rename also then the filename (should match the compatible).
Documentation/devicetree/bindings/iio/pressure/honeywell,mpr.yaml line 36
Quoted 10 lines
Documentation/devicetree/bindings/iio/pressure/honeywell,mpr.yaml new file +10
-0+ + 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?
Quoted 8 lines
Documentation/devicetree/bindings/iio/pressure/honeywell,mpr.yaml line 41
Quoted 5 lines
Documentation/devicetree/bindings/iio/pressure/honeywell,mpr.yaml new file +5
-0+ 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
Quoted 10 lines
Documentation/devicetree/bindings/iio/pressure/honeywell,mpr.yaml line 47
Quoted 6 lines
Documentation/devicetree/bindings/iio/pressure/honeywell,mpr.yaml new file +6
-0+ $ref: /schemas/types.yaml#/definitions/uint32 + + honeywell,pmax: + description: + Maximum pressure value the sensor can measure in pascal. + $ref: /schemas/types.yaml#/definitions/uint32Same.
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.
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.
Quoted 2 lines
Does it depend on type of sensor (thus it is implied from compatible) or on system setup?
For the standard types it can be derived from the type of sensor but for the custom types it's not possible.
So sum up it'll look like this:
standard types:
96 compatibles, e. g. "honeywell,mpr-0025pa-a"
custom types:
1 compatible: "honeywell,mpr-custom"
honeywell,pmin-pascal
honeywell,pmax-pascal
honeywell,transfer-function
Best regards,
Andreas
Quoted 96 lines · Andreas Klinger
Hi Krzysztof,
Krzysztof Kozlowski <[email protected]> schrieb am Sa, 01. Apr 11:42:
Quoted 69 lines · Andreas Klinger
Quoted 22 lines
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 <[email protected]>--- .../bindings/iio/pressure/honeywell,mpr.yaml | 74 +++++++++++++++++++ 1 file changed, 74 insertions(+) create mode 100644 Documentation/devicetree/bindings/iio/pressure/honeywell,mpr.yamlDocumentation/devicetree/bindings/iio/pressure/honeywell,mpr.yaml new file +3
-0@@ -0,0 +1,74 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +---Documentation/devicetree/bindings/iio/pressure/honeywell,mpr.yaml line 13
Quoted 10 lines
Documentation/devicetree/bindings/iio/pressure/honeywell,mpr.yaml new file +10
-0+$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 <[email protected]> + +description: | + Honeywell pressure sensor of type mpr. This sensor has an I2C and SPI interface. Only the I2CDoesn't look wrapped according to Linux coding style (see Coding style).
Documentation/devicetree/bindings/iio/pressure/honeywell,mpr.yaml line 22
Quoted 9 lines
Documentation/devicetree/bindings/iio/pressure/honeywell,mpr.yaml new file +9
-0+ 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.pdfLines are not continued, so drop \
Documentation/devicetree/bindings/iio/pressure/honeywell,mpr.yaml line 26
Quoted 4 lines
Documentation/devicetree/bindings/iio/pressure/honeywell,mpr.yaml new file +4
-0+ +properties: + compatible: + const: honeywell,mprYou need device specific compatible, not some generic one. Rename also then the filename (should match the compatible).
Documentation/devicetree/bindings/iio/pressure/honeywell,mpr.yaml line 36
Quoted 10 lines
Documentation/devicetree/bindings/iio/pressure/honeywell,mpr.yaml new file +10
-0+ + 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?
Quoted 8 lines
Documentation/devicetree/bindings/iio/pressure/honeywell,mpr.yaml line 41
Quoted 5 lines
Documentation/devicetree/bindings/iio/pressure/honeywell,mpr.yaml new file +5
-0+ 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.yamlThere 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
Quoted 9 lines
Documentation/devicetree/bindings/iio/pressure/honeywell,mpr.yaml line 47
Quoted 6 lines
Documentation/devicetree/bindings/iio/pressure/honeywell,mpr.yaml new file +6
-0+ $ref: /schemas/types.yaml#/definitions/uint32 + + honeywell,pmax: + description: + Maximum pressure value the sensor can measure in pascal. + $ref: /schemas/types.yaml#/definitions/uint32Same.
Quoted 8 lines
Quoted 1 line
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.
Quoted 3 lines
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
Jonathan Cameron
maintainerQuoted 31 lines · On Sat, 1 Apr 2023 11:09:41 +0200 Andreas Klinger <[email protected]> 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 <[email protected]>--- .../bindings/iio/pressure/honeywell,mpr.yaml | 74 +++++++++++++++++++ 1 file changed, 74 insertions(+) create mode 100644 Documentation/devicetree/bindings/iio/pressure/honeywell,mpr.yamlDocumentation/devicetree/bindings/iio/pressure/honeywell,mpr.yaml new file +12
-0@@ -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 <[email protected]> + +description: |
Documentation/devicetree/bindings/iio/pressure/honeywell,mpr.yaml line 22
Quoted 10 lines
Documentation/devicetree/bindings/iio/pressure/honeywell,mpr.yaml new file +10
-0+ Honeywell pressure sensor of type mpr. This sensor has an I2C and SPI interface. Only the I2C + 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
Ouch. Anyone want to suggest to honeywell that they add some short paths for this stuff!
Please add regulator for VDD. May well be a fixed reg on the boards you care about, but it's still good document it's existence.
Quoted 5 lines
Documentation/devicetree/bindings/iio/pressure/honeywell,mpr.yaml new file +5
-0+ +properties: + compatible: + const: honeywell,mpr +
PATCH 2/3 iio: pressure: Honeywell mpr pressure sensor
Honeywell mpr is a familiy of pressure sensors.
Add initial I2C support.
Note:
- Buffered mode is supported
- SPI mode is not supported
--- drivers/iio/pressure/Kconfig | 12 ++ drivers/iio/pressure/Makefile | 1 + drivers/iio/pressure/mpr.c | 307 ++++++++++++++++++++++++++++++++++ 3 files changed, 320 insertions(+) create mode 100644 drivers/iio/pressure/mpr.c
drivers/iio/pressure/Kconfig +12 -0
@@ -148,6 +148,18 @@ config MPL3115 To compile this driver as a module, choose M here: the module will be called mpl3115. +config MPR + tristate "Honeywell MPR (MicroPressure sensors)" + depends on I2C + select IIO_BUFFER + select IIO_TRIGGERED_BUFFER + help + Say Y here to build support for the Honeywell MicroPressure pressure sensors MPR. + There are many different types with different pressure range. These ranges can be set up + in the device tree. + + To compile this driver as a module, choose M here: the module will be called mpr. + config MS5611 tristate "Measurement Specialties MS5611 pressure sensor driver" select IIO_BUFFER
drivers/iio/pressure/Makefile +1 -0
@@ -19,6 +19,7 @@ obj-$(CONFIG_MPL115) += mpl115.o obj-$(CONFIG_MPL115_I2C) += mpl115_i2c.o obj-$(CONFIG_MPL115_SPI) += mpl115_spi.o obj-$(CONFIG_MPL3115) += mpl3115.o +obj-$(CONFIG_MPR) += mpr.o obj-$(CONFIG_MS5611) += ms5611_core.o obj-$(CONFIG_MS5611_I2C) += ms5611_i2c.o obj-$(CONFIG_MS5611_SPI) += ms5611_spi.o
drivers/iio/pressure/mpr.c new file +307 -0
307 lines not on this page · raw message
Signature · 1 line
2.30.2
kernel test robot
build botRe: [PATCH 2/3] iio: pressure: Honeywell mpr pressure sensor
I love your patch! Yet something to improve:
Hi Andreas,
I love your patch! Yet something to improve:
[auto build test ERROR on jic23-iio/togreg]
[If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Andreas-Klinger/dt-bindings-iio-pressure-Support-Honeywell-mpr-sensors/20230401-171226 base: https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg patch link: https://lore.kernel.org/r/ZCf085W4XL2PtQf6%40arbad patch subject: [PATCH 2/3] iio: pressure: Honeywell mpr pressure sensor config: m68k-randconfig-r023-20230401 (https://download.01.org/0day-ci/archive/20230401/[email protected]/config) compiler: m68k-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/6a49dae45811d8a644c56dc18b6cdbc6ea67df98
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Andreas-Klinger/dt-bindings-iio-pressure-Support-Honeywell-mpr-sensors/20230401-171226 git checkout 6a49dae45811d8a644c56dc18b6cdbc6ea67df98
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=m68k olddefconfig COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=m68k SHELL=/bin/bash
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
| Link: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All errors (new ones prefixed by >>, old ones prefixed by <<):
>> ERROR: modpost: "__divdi3" [drivers/iio/pressure/mpr.ko] undefined!
Signature · 2 lines
0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests
Jonathan Cameron
maintainerRe: [PATCH 2/3] iio: pressure: Honeywell mpr pressure sensor
In reply to Andreas KlingerOn Sat, 1 Apr 2023 11:10:11 +0200
Andreas Klinger <[email protected]> wrote:
Hi Andreas,
Various comments inline.
Quoted 1 line
Honeywell mpr is a familiy of pressure sensors.
Spell check.
Quoted 59 lines
Add initial I2C support.
Note:
- Buffered mode is supported
- SPI mode is not supportedSigned-off-by Andreas Klinger <[email protected]>--- drivers/iio/pressure/Kconfig | 12 ++ drivers/iio/pressure/Makefile | 1 + drivers/iio/pressure/mpr.c | 307 ++++++++++++++++++++++++++++++++++ 3 files changed, 320 insertions(+) create mode 100644 drivers/iio/pressure/mpr.cdrivers/iio/pressure/Kconfig +12
-0@@ -148,6 +148,18 @@ config MPL3115 To compile this driver as a module, choose M here: the module will be called mpl3115. +config MPR + tristate "Honeywell MPR (MicroPressure sensors)" + depends on I2C + select IIO_BUFFER + select IIO_TRIGGERED_BUFFER + help + Say Y here to build support for the Honeywell MicroPressure pressure sensors MPR. + There are many different types with different pressure range. These ranges can be set up + in the device tree. + + To compile this driver as a module, choose M here: the module will be called mpr. + config MS5611 tristate "Measurement Specialties MS5611 pressure sensor driver" select IIO_BUFFERdrivers/iio/pressure/Makefile +1
-0@@ -19,6 +19,7 @@ obj-$(CONFIG_MPL115) += mpl115.o obj-$(CONFIG_MPL115_I2C) += mpl115_i2c.o obj-$(CONFIG_MPL115_SPI) += mpl115_spi.o obj-$(CONFIG_MPL3115) += mpl3115.o +obj-$(CONFIG_MPR) += mpr.o obj-$(CONFIG_MS5611) += ms5611_core.o obj-$(CONFIG_MS5611_I2C) += ms5611_i2c.o obj-$(CONFIG_MS5611_SPI) += ms5611_spi.odrivers/iio/pressure/mpr.c new file +3
-0@@ -0,0 +1,307 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* +* MPR - MicroPressure pressure sensor driver
drivers/iio/pressure/mpr.c line 13
Quoted 10 lines
drivers/iio/pressure/mpr.c new file +10
-0+* +* Copyright (c) Andreas Klinger <[email protected]> +* +* Data sheet: +* 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 +* +* 7-bit I2C default slave address: 0x18 +*
Trivial but this blank line doesn't add anything useful to my eye
drivers/iio/pressure/mpr.c line 16
Quoted 3 lines
drivers/iio/pressure/mpr.c new file +3
-0+*/ + +#include <linux/of.h>
As below, should use stuff in linux/property.h not of.h
drivers/iio/pressure/mpr.c line 28
Quoted 12 lines
drivers/iio/pressure/mpr.c new file +12
-0+#include <linux/delay.h> +#include <linux/i2c.h> +#include <linux/module.h> +#include <linux/mod_devicetable.h> + +#include <linux/device.h> +#include <linux/gpio/consumer.h> + +#include <asm/unaligned.h> + +#include <linux/iio/iio.h> +#include <linux/iio/sysfs.h>
Not seeing any custom attributes (which is pretty much only reason you'd need iio/sysfs.h)
drivers/iio/pressure/mpr.c line 40
Quoted 12 lines
drivers/iio/pressure/mpr.c new file +12
-0+#include <linux/iio/buffer.h> +#include <linux/iio/triggered_buffer.h> +#include <linux/iio/trigger_consumer.h> + +/* bits in i2c status byte */ +#define MPR_I2C_POWER BIT(6)/* device is powered */ +#define MPR_I2C_BUSY BIT(5)/* device is busy */ +#define MPR_I2C_MEMORY BIT(2)/* integrity test passed */ +#define MPR_I2C_MATH BIT(0)/* internal math saturation */ + +struct mpr_data { +struct device *dev;
Given it's easy to get this dev from client->dev, don't bother holding both in here.
drivers/iio/pressure/mpr.c line 42
Quoted 2 lines
drivers/iio/pressure/mpr.c new file +2
-0+void *client; +struct mutex lock;
All locks should have a comment saying what exactly they protect.
drivers/iio/pressure/mpr.c line 48
Quoted 6 lines
drivers/iio/pressure/mpr.c new file +6
-0+s32 pmin; +s32 pmax; +struct gpio_desc *gpiod_reset; +int irq; +struct completion completion; +s64 channel[2 ]__aligned(8) ;
Why? Good to call out the explicit purpose of this. Usually easiest option is to use a structure where you can call out the two elements with names that tell reader what is going on.
drivers/iio/pressure/mpr.c line 52
Quoted 4 lines
drivers/iio/pressure/mpr.c new file +4
-0+}; + +static int mpr_read_raw (struct iio_dev *indio_dev,struct iio_chan_spec const *chan,int *val, +int *val2,long mask);
Should be possible to reorder code so this isn't needed.
Quoted 18 lines
drivers/iio/pressure/mpr.c new file +17
-0+ +static const struct iio_chan_spec mpr_channels[] = { + { + .type =IIO_PRESSURE , + .info_mask_separate =BIT (IIO_CHAN_INFO_PROCESSED ), + .scan_index =0 , + .scan_type = { + .sign ='s' , + .realbits =64 , + .storagebits =64 , + .endianness =IIO_CPU , + }, + }, +IIO_CHAN_SOFT_TIMESTAMP (1 ), +}; + +static const struct iio_info mpr_info = {
drivers/iio/pressure/mpr.c line 79
Quoted 10 lines
drivers/iio/pressure/mpr.c new file +10
-0+ .read_raw = &mpr_read_raw, +}; + +static void mpr_reset (struct mpr_data *data) +{ +if (data->gpiod_reset) { +gpiod_set_value (data->gpiod_reset,0 ); +udelay (10 ); +gpiod_set_value (data->gpiod_reset,1 ); + }
If there isn't a reset signal, I'd like to see an attempt at least to write all configuration registers to a known value (same as the one you'd get after reset).
Quoted 21 lines
drivers/iio/pressure/mpr.c new file +21
-0+} + +static int mpr_read_pressure (struct mpr_data *data,s64 *press) +{ +int ret, i; +u8 wdata[] = {0xAA ,0x00 ,0x00 }; +s32 status; +int nloops =10 ; +char buf[5 ]; +s64 press_cnt; +s64 outputmin =1677722 ; +s64 outputmax =15099494 ; + +reinit_completion (&data->completion); + + ret =i2c_master_send (data->client, wdata,sizeof (wdata)); +if (ret <0 ) { +dev_err (data->dev,"error while writing ret: %d\n" , ret); +return ret; + } +
drivers/iio/pressure/mpr.c line 110
Quoted 10 lines
drivers/iio/pressure/mpr.c new file +10
-0+if (data->irq >0 ) { + ret =wait_for_completion_timeout (&data->completion,HZ ); +if (!ret) { +dev_err (data->dev,"timeout while waiting for eoc interrupt\n" ); +return -ETIMEDOUT ; + } + }else { +/* wait until status indicates data is ready */ +for (i =0 ; i < nloops; i++) { +usleep_range (5000 ,10000 );
Add a comment on why this particular timing makes sense (reference to datasheet probably)
Quoted 17 lines
drivers/iio/pressure/mpr.c new file +17
-0+ status =i2c_smbus_read_byte (data->client); +if (status <0 ) { +dev_err (data->dev,"error while reading, status: %d\n" , status); +return status; + } +if (!(status &MPR_I2C_BUSY )) +break ; + } +if (i == nloops) { +dev_err (data->dev,"timeout while reading\n" ); +return -ETIMEDOUT ; + } + } + + ret =i2c_master_recv (data->client, buf,sizeof (buf)); +if (ret <0 ) { +dev_err (data->dev,"error in i2c_master_recv ret: %d\n" , ret);
drivers/iio/pressure/mpr.c line 137
Quoted 10 lines
drivers/iio/pressure/mpr.c new file +10
-0+return ret; + } + +if (buf[0 ] &MPR_I2C_BUSY ) { +/* it should never be the case that status still indicates business */ +dev_err (data->dev,"data still not ready: %08x\n" , buf[0 ]); +return -ETIMEDOUT ; + } + + press_cnt = buf[3 ] + buf[2 ] *256 + buf[1 ] *65536 ;
Looks like a get_unaligned_le24() open coded? Use the standard function for this as that makes it easier to see what is going on.
drivers/iio/pressure/mpr.c line 140
Quoted 3 lines
drivers/iio/pressure/mpr.c new file +3
-0+ + *press = ((press_cnt - outputmin) * (s64)(data->pmax - data->pmin)) + / (outputmax - outputmin) + (s64 )data->pmin;
Some defines for the value of outputmin and outputmax would be better than using local variables that happen to be constant.
Looks to me like this could be broken into
(raw + offset) * scale in which case you could use a _RAW interface with _OFFSET and _SCALE allow you to make this into a 24 bit (well 32 bit) slot of a buffer should you add buffered support later.
Quoted 36 lines
drivers/iio/pressure/mpr.c new file +36
-0+ +dev_dbg (data->dev,"received: %*ph cnt: %lld press: %lld\n" , ret, buf, press_cnt, *press); + +return 0 ; +} + +static irqreturn_t mpr_eoc_handler (int irq,void *p) +{ +struct mpr_data *data = (struct mpr_data *)p; + +complete (&data->completion); + +return IRQ_HANDLED ; +} + +static irqreturn_t mpr_trigger_handler (int irq,void *p) +{ +int ret; +struct iio_poll_func *pf = p; +struct iio_dev *indio_dev = pf->indio_dev; +struct mpr_data *data =iio_priv (indio_dev); + +mutex_lock (&data->lock); + ret =mpr_read_pressure (data, &data->channel[0 ]); +if (ret <0 ) { +mutex_unlock (&data->lock); + goto err; + } + +iio_push_to_buffers_with_timestamp (indio_dev, data->channel,iio_get_time_ns (indio_dev)); +mutex_unlock (&data->lock); + +err: +iio_trigger_notify_done (indio_dev->trig); + +return IRQ_HANDLED ;
drivers/iio/pressure/mpr.c line 186
Quoted 10 lines
drivers/iio/pressure/mpr.c new file +10
-0+} + +static int mpr_read_raw (struct iio_dev *indio_dev,struct iio_chan_spec const *chan,int *val, +int *val2,long mask) +{ +int ret; +s64 pressure; +struct mpr_data *data =iio_priv (indio_dev); + +if (mask ==IIO_CHAN_INFO_PROCESSED ) {
Given you only support one option neater to error out early if it's not a match even if you have two do that twice.
if (mask != IIO_CHAN_INFO_PROCESSED) return -EINVAL;
drivers/iio/pressure/mpr.c line 193
Quoted 7 lines
drivers/iio/pressure/mpr.c new file +7
-0+mutex_lock (&data->lock); + ret =mpr_read_pressure (data, &pressure); +mutex_unlock (&data->lock); +if (ret <0 ) +return ret; + +if (chan->type ==IIO_PRESSURE ) {
Check that earlier. Not a lot of point in reading the data if channel type is wrong.
drivers/iio/pressure/mpr.c line 194
Quoted 1 line
drivers/iio/pressure/mpr.c new file +1
-0+ *val = (s32 )clamp (pressure,0LL ,2147483648LL );
What is special about that constant? If it's a device specific value then given it a name.
Quoted 4 lines
drivers/iio/pressure/mpr.c new file +4
-0+return IIO_VAL_INT ; + } + } +
drivers/iio/pressure/mpr.c line 208
Quoted 10 lines
drivers/iio/pressure/mpr.c new file +10
-0+return -EINVAL ; +} + +static int mpr_probe (struct i2c_client *client,const struct i2c_device_id *id) +{ +int ret; +struct mpr_data *data; +struct iio_dev *indio_dev; +struct device *dev = &client->dev; +struct device_node *np = dev->of_node;
As below, switch to non of specific firmware handlign.
drivers/iio/pressure/mpr.c line 219
Quoted 11 lines
drivers/iio/pressure/mpr.c new file +11
-0+ +if (!i2c_check_functionality (client->adapter,I2C_FUNC_SMBUS_READ_BYTE )) +return -EOPNOTSUPP ; + + indio_dev =devm_iio_device_alloc (dev,sizeof (*data)); +if (!indio_dev) +return -ENOMEM ; + + data =iio_priv (indio_dev); + data->dev = &client->dev; +i2c_set_clientdata (client, indio_dev);
I can't see where this is used.
drivers/iio/pressure/mpr.c line 226
Quoted 7 lines
drivers/iio/pressure/mpr.c new file +7
-0+ data->client = client; + data->irq = client->irq; + +mutex_init (&data->lock); +init_completion (&data->completion); + + indio_dev->name = client->name;
Should be part number. As a string provided here (or more likely in a device type specific structure that you are going to add).
drivers/iio/pressure/mpr.c line 232
Quoted 6 lines
drivers/iio/pressure/mpr.c new file +6
-0+ indio_dev->info = &mpr_info; + indio_dev->channels = mpr_channels; + indio_dev->num_channels =ARRAY_SIZE (mpr_channels); + indio_dev->modes =INDIO_DIRECT_MODE ; + +if (np) {
Don't use of specific interfaces. Use the ones from linux/property.h instead. Lets other firmware types work 'for free' so we are trying to avoid introducing any of specific handling into new IIO drivers (and slowly scrubbing the old ones for this)·
if (dev_fwnode()) etc
drivers/iio/pressure/mpr.c line 233
Quoted 1 line
drivers/iio/pressure/mpr.c new file +1
-0+if (of_property_read_s32 (np,"honeywell,pmin" , &data->pmin)) {
all these need to converting to the device_property_xxx equivalents.
drivers/iio/pressure/mpr.c line 242
Quoted 9 lines
drivers/iio/pressure/mpr.c new file +9
-0+dev_err (dev,"honeywell,pmin missing in DT\n" ); +return -EINVAL ; + } +if (of_property_read_s32 (np,"honeywell,pmax" , &data->pmax)) { +dev_err (dev,"honeywell,pmax missing in DT\n" ); +return -EINVAL ; + } + + data->gpiod_reset =devm_gpiod_get (dev,"reset" ,GPIOD_OUT_HIGH );
Whilst these are sort of fw node dependent they don't need the protection. So drop them out of this if (np) block.
devm_gpiod_get_optional() given you support not having it.
drivers/iio/pressure/mpr.c line 245
Quoted 3 lines
drivers/iio/pressure/mpr.c new file +3
-0+if (IS_ERR (data->gpiod_reset)) { +dev_err (dev,"failed to get reset-gpios: err=%pe\n" , + data->gpiod_reset);
Not an error so dev_dbg() probably right thing to report.
Quoted 4 lines
drivers/iio/pressure/mpr.c new file +4
-0+ data->gpiod_reset =NULL ; + } + +if (data->irq >0 ) {
drivers/iio/pressure/mpr.c line 259
Quoted 10 lines
drivers/iio/pressure/mpr.c new file +10
-0+ ret =devm_request_irq (dev, data->irq, mpr_eoc_handler, +IRQF_TRIGGER_RISING , client->name, data); +if (ret) { +dev_err (dev,"request irq %d failed\n" , data->irq); +return ret; + } + } + }else { +/* when loaded as i2c device we need to use default values */ +dev_warn (dev,"no dt node found; using defaults\n" );
A strong argument in favour of not handling all devices via one 'compatible' (or the the i2c_device_id version of that anyway).
drivers/iio/pressure/mpr.c line 261
Quoted 2 lines
drivers/iio/pressure/mpr.c new file +2
-0+ data->pmin =0 ; + data->pmax =172369 ;/* 25 psi */
Perhaps worth defining the maths for that conversion so that you can just use
psi_to_milibar(25); or whatever makes sense.
drivers/iio/pressure/mpr.c
Quoted 1 line
drivers/iio/pressure/mpr.c new file +1
-0+ data->gpiod_reset =NULL ;
Should be no need to set that as it will be NULL anyway in this path.
drivers/iio/pressure/mpr.c line 275
Quoted 13 lines
drivers/iio/pressure/mpr.c new file +13
-0+ } + +mpr_reset (data); + + ret =devm_iio_triggered_buffer_setup (dev, indio_dev,NULL , mpr_trigger_handler,NULL ); +if (ret <0 ) { +dev_err (dev,"iio triggered buffer setup failed\n" ); +return ret; + } + + ret =devm_iio_device_register (dev, indio_dev); +if (ret <0 ) { +dev_err (dev,"unable to register iio device\n" );
In general use
return dev_err_probe(dev, ret, ...)
for all errors that only occur on probe. That wraps up some stuff around deferred probing but is in general a good thing to do even if a given call can't defer.
drivers/iio/pressure/mpr.c line 283
Quoted 8 lines
drivers/iio/pressure/mpr.c new file +8
-0+return ret; + } + +return 0 ; +} + +static const struct of_device_id mpr_matches[] = { + { .compatible ="honeywell,mpr" , .data =0 },
No point in filling data either here or in the i2c_device_id entries until something useful is done with it.
When you do (which is likely after request to use compatibles
to indicate a lot of the per device type variability) then you should look for having the data as a pointer to a const structure that describes what those per device type details.
Quoted 14 lines
drivers/iio/pressure/mpr.c new file +14
-0+ { } +}; +MODULE_DEVICE_TABLE (of, mpr_matches); + +static const struct i2c_device_id mpr_id[] = { + {"honeywell,mpr" ,0 }, + { } +}; +MODULE_DEVICE_TABLE (i2c, mpr_id); + +static struct i2c_driver mpr_driver = { + .probe = mpr_probe, + .id_table = mpr_id, + .driver = {
drivers/iio/pressure/mpr.c line 307
Quoted 10 lines
drivers/iio/pressure/mpr.c new file +10
-0+ .name = "mpr", + .of_match_table = mpr_matches, + }, +}; +module_i2c_driver (mpr_driver); + +MODULE_AUTHOR ("Andreas Klinger <[email protected]>" ); +MODULE_DESCRIPTION ("MPR I2C driver" ); +MODULE_LICENSE ("GPL" ); +MODULE_IMPORT_NS (IIO_MPR );
Why? You aren't exporting any symbols in that namespace that I can see.
Re: [PATCH 2/3] iio: pressure: Honeywell mpr pressure sensor
Hi Jonathan,
thanks for the extensive review. Most of it is clear but one questions remain. See below.
Jonathan Cameron <[email protected]> schrieb am Sa, 01. Apr 18:57:
Quoted 11 lines
drivers/iio/pressure/mpr.c line 79
Quoted 7 lines
drivers/iio/pressure/mpr.c new file +7
-0+static void mpr_reset (struct mpr_data *data) +{ +if (data->gpiod_reset) { +gpiod_set_value (data->gpiod_reset,0 ); +udelay (10 ); +gpiod_set_value (data->gpiod_reset,1 ); + }If there isn't a reset signal, I'd like to see an attempt at least to write all configuration registers to a known value (same as the one you'd get after reset).
There is no configuration register in the sensor I could write to. But maybe I didn't comprehend your point.
Andreas
Jonathan Cameron
maintainerRe: [PATCH 2/3] iio: pressure: Honeywell mpr pressure sensor
Quoted 18 lines · On Thu, 6 Apr 2023 21:43:57 +0200 Andreas Klinger <[email protected]> wrote:
Hi Jonathan,
thanks for the extensive review. Most of it is clear but one questions remain. See below.
Jonathan Cameron <[email protected]> schrieb am Sa, 01. Apr 18:57:
Quoted 11 lines
drivers/iio/pressure/mpr.c line 79
Quoted 7 lines
drivers/iio/pressure/mpr.c new file +7
-0+static void mpr_reset (struct mpr_data *data) +{ +if (data->gpiod_reset) { +gpiod_set_value (data->gpiod_reset,0 ); +udelay (10 ); +gpiod_set_value (data->gpiod_reset,1 ); + }If there isn't a reset signal, I'd like to see an attempt at least to write all configuration registers to a known value (same as the one you'd get after reset).
Quoted 2 lines
There is no configuration register in the sensor I could write to. But maybe I didn't comprehend your point.
Ah. Devices is even simpler than I was anticipating. Which makes me wonder. What does the reset actually do?
I checked the datasheet and reason to bother with this is about powersupplies that don't come up fast enough. Fair enough. If someone hasn't wired the reset I guess they are happy that the power on reset will work. (4.0 Power support requirement)
Jonathan
Quoted 3 lines
Andreas
Re: [PATCH 2/3] iio: pressure: Honeywell mpr pressure sensor
In reply to Andreas KlingerHi,
Looks pretty good. Jonathan already covered most of it, a few additional comments.
drivers/iio/pressure/mpr.c line 41
Quoted 4 lines · Andreas Klinger
[...]
drivers/iio/pressure/mpr.c new file +3
-0+struct mpr_data { +struct device *dev; +void *client;
Any reason not to use struct i2c_client for the type?
Quoted 7 lines
drivers/iio/pressure/mpr.c new file +7
-0+struct mutex lock; +s32 pmin; +s32 pmax; +struct gpio_desc *gpiod_reset; +int irq; +struct completion completion; +s64 channel[2 ]__aligned(8) ;
drivers/iio/pressure/mpr.c line 88
Quoted 10 lines
drivers/iio/pressure/mpr.c new file +9
-0+}; + +static int mpr_read_pressure (struct mpr_data *data,s64 *press) +{ +int ret, i; +u8 wdata[] = {0xAA ,0x00 ,0x00 }; +s32 status; +int nloops =10 ; +char buf[5 ];
The tx buffer is u8, the rx buffer is char. This should be consistent.
drivers/iio/pressure/mpr.c line 95
Quoted 7 lines
drivers/iio/pressure/mpr.c new file +7
-0+s64 press_cnt; +s64 outputmin =1677722 ; +s64 outputmax =15099494 ; + +reinit_completion (&data->completion); + + ret =i2c_master_send (data->client, wdata,sizeof (wdata));
The i2c family of transfer functions returns the number of bytes transferred, this can be less than what you expect if you get an early NACK. Its always good to check that all the data was transferred. E.g.
if (ret >= 0 && ret != sizeof(wdata))
ret = -EIO;
Same for the receive later on.
[...]
kernel test robot
build botRe: [PATCH 2/3] iio: pressure: Honeywell mpr pressure sensor
In reply to Andreas KlingerI love your patch! Yet something to improve:
Hi Andreas,
I love your patch! Yet something to improve:
[auto build test ERROR on jic23-iio/togreg]
[If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Andreas-Klinger/dt-bindings-iio-pressure-Support-Honeywell-mpr-sensors/20230401-171226 base: https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg patch link: https://lore.kernel.org/r/ZCf085W4XL2PtQf6%40arbad patch subject: [PATCH 2/3] iio: pressure: Honeywell mpr pressure sensor config: arm-buildonly-randconfig-r004-20230401 (https://download.01.org/0day-ci/archive/20230402/[email protected]/config) compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project 67409911353323ca5edf2049ef0df54132fa1ca7)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross
# install arm cross compiling tool for clang build
# apt-get install binutils-arm-linux-gnueabi
# https://github.com/intel-lab-lkp/linux/commit/6a49dae45811d8a644c56dc18b6cdbc6ea67df98
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Andreas-Klinger/dt-bindings-iio-pressure-Support-Honeywell-mpr-sensors/20230401-171226 git checkout 6a49dae45811d8a644c56dc18b6cdbc6ea67df98
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm olddefconfig COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm SHELL=/bin/bash
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
| Link: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All errors (new ones prefixed by >>, old ones prefixed by <<):
>> ERROR: modpost: "__aeabi_ldivmod" [drivers/iio/pressure/mpr.ko] undefined!
Signature · 2 lines
0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests