iio: Add Everlight PM16D17 proximity sensor
Subsystems (2)
cover iio: Add Everlight PM16D17 proximity sensor
The Everlight PM16D17 is attached via I2C and can measure the proximity of objects optically. Model it as IIO driver and include it into the device tree of the IOT2050-SM where it is available on the main board.
Note that at the time of writing, the datasheet link is still work in progress at Everlight (product listed, link still broken). We are in contact with them.
Jan
Baocheng Su (1):
dt-bindings: vendor-prefixes: Add EVERLIGHT
Chao Zeng (2):
dt-bindings: iio: Add everlight pm16d17 binding
iio: proximity: Add support for everlight pmd16d17 sensor
.../iio/proximity/everlight,pm16d17.yaml | 95 +++++ .../devicetree/bindings/vendor-prefixes.yaml | 2 + drivers/iio/proximity/Kconfig | 11 + drivers/iio/proximity/Makefile | 1 + drivers/iio/proximity/pm16d17.c | 324 ++++++++++++++++++ 5 files changed, 433 insertions(+) create mode 100644 Documentation/devicetree/bindings/iio/proximity/everlight,pm16d17.yaml create mode 100644 drivers/iio/proximity/pm16d17.c
Signature · 1 line
2.43.0
PATCH 1/3 dt-bindings: vendor-prefixes: Add EVERLIGHT
From: Baocheng Su <[email protected]>
Add vendor prefix for EVERLIGHT Electronics Co., Ltd.
--- Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++ 1 file changed, 2 insertions(+)
Documentation/devicetree/bindings/vendor-prefixes.yaml +2 -0
@@ -486,6 +486,8 @@ patternProperties: description: Eukréa Electromatique "^everest,.*": description: Everest Semiconductor Co. Ltd. + "^everlight,.*": + description: EVERLIGHT Electronics Co., Ltd. "^everspin,.*": description: Everspin Technologies, Inc. "^evervision,.*":
Signature · 1 line
2.43.0
Re: [PATCH 1/3] dt-bindings: vendor-prefixes: Add EVERLIGHT
Quoted 5 lines · Jan Kiszka
From: Baocheng Su <[email protected]>
Add vendor prefix for EVERLIGHT Electronics Co., Ltd.
Signed-off-by Baocheng Su <[email protected]>
This is missing your signoff Jan. With it,
Quoted 20 lines
--- Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++ 1 file changed, 2 insertions(+)Documentation/devicetree/bindings/vendor-prefixes.yaml +2
-0@@ -486,6 +486,8 @@ patternProperties: description: Eukréa Electromatique "^everest,.*": description: Everest Semiconductor Co. Ltd. + "^everlight,.*": + description: EVERLIGHT Electronics Co., Ltd. "^everspin,.*": description: Everspin Technologies, Inc. "^evervision,.*":Signature · 1 line
2.43.0
Re: [PATCH 1/3] dt-bindings: vendor-prefixes: Add EVERLIGHT
Quoted 9 lines · Conor Dooley
Quoted 5 lines · Jan Kiszka
From: Baocheng Su <[email protected]>
Add vendor prefix for EVERLIGHT Electronics Co., Ltd.
Signed-off-by Baocheng Su <[email protected]>This is missing your signoff Jan. With it,
Acked-by Conor Dooley <[email protected]>
The next one as well...
Best regards,
Krzysztof
Re: [PATCH 1/3] dt-bindings: vendor-prefixes: Add EVERLIGHT
Quoted 4 lines · Krzysztof Kozlowski
Quoted 3 lines · Conor Dooley
Quoted 2 lines · Jan Kiszka
From: Baocheng Su <[email protected]>
Quoted 8 lines
Quoted 6 lines
Quoted 3 lines
Add vendor prefix for EVERLIGHT Electronics Co., Ltd.
Signed-off-by Baocheng Su <[email protected]>This is missing your signoff Jan. With it,
Acked-by Conor Dooley <[email protected]>The next one as well...
I had a longer reply for the other one ;)
PATCH 2/3 dt-bindings: iio: Add everlight pm16d17 binding
In reply to Jan KiszkaFrom: Chao Zeng <[email protected]>
Add the binding document for the everlight pm16d17 sensor.
--- .../iio/proximity/everlight,pm16d17.yaml | 95 +++++++++++++++++++ 1 file changed, 95 insertions(+) create mode 100644 Documentation/devicetree/bindings/iio/proximity/everlight,pm16d17.yaml
Documentation/devicetree/bindings/iio/proximity/everlight,pm16d17.yaml new file +95 -0
@@ -0,0 +1,95 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/iio/proximity/everlight,pm16d17.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Everlight PM-16D17 Ambient Light & Proximity Sensor + +maintainers: + - Chao Zeng <[email protected]> + +description: | + This sensor uses standard I2C interface. Interrupt function is not covered. + Datasheet: https://en.everlight.com/sensor/category-proximity_sensor/digital_proximity_sensor/ + +properties: + compatible: + enum: + - everlight,pm16d17 + + reg: + maxItems: 1 + + ps-gain: + description: Receiver gain of proximity sensor + $ref: /schemas/types.yaml#/definitions/uint32 + enum: [1, 2, 4, 8] + default: 1 + + ps-itime: + description: Conversion time for proximity sensor [ms] + $ref: /schemas/types.yaml#/definitions/string + enum: + - "0.4" + - "0.8" + - "1.6" + - "3.2" + - "6.3" + - "12.6" + - "25.2" + default: "0.4" + + ps-wtime: + description: Waiting time for proximity sensor [ms] + $ref: /schemas/types.yaml#/definitions/string + enum: + - "12.5" + - "25" + - "50" + - "100" + - "200" + - "400" + - "800" + - "1600" + default: "12.5" + + ps-ir-led-pulse-count: + description: IR LED drive pulse count + $ref: /schemas/types.yaml#/definitions/uint32 + minimum: 1 + maximum: 256 + default: 1 + + ps-offset-cancel: + description: | + When PS offset cancel function is enabled, the result of subtracting any + value specified by the PS offset cancel register from the internal PS + output data is written to the PS output data register. + $ref: /schemas/types.yaml#/definitions/uint32 + default: 0 + maximum: 65535 + +required: + - compatible + - reg + +unevaluatedProperties: false + +examples: + - | + i2c { + #address-cells = <1>; + #size-cells = <0>; + + lightsensor: pm16d17@44 { + compatible = "everlight,pm16d17"; + reg = <0x44>; + + ps-gain = <1>; + ps-itime = "0.4"; + ps-wtime = "12.5"; + ps-ir-led-pulse-count = <1>; + ps-offset-cancel = <280>; + }; + };
Signature · 1 line
2.43.0
Re: [PATCH 2/3] dt-bindings: iio: Add everlight pm16d17 binding
Quoted 7 lines · Jan Kiszka
From: Chao Zeng <[email protected]>
Add the binding document for the everlight pm16d17 sensor.
Signed-off-by Chao Zeng <[email protected]>Co-developed-by Baocheng Su <[email protected]>Signed-off-by Baocheng Su <[email protected]>
Ditto here Jan.
Quoted 14 lines
--- .../iio/proximity/everlight,pm16d17.yaml | 95 +++++++++++++++++++ 1 file changed, 95 insertions(+) create mode 100644 Documentation/devicetree/bindings/iio/proximity/everlight,pm16d17.yamlDocumentation/devicetree/bindings/iio/proximity/everlight,pm16d17.yaml new file +3
-0@@ -0,0 +1,95 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +%YAML 1.2 +---
Documentation/devicetree/bindings/iio/proximity/everlight,pm16d17.yaml line 13
Quoted 10 lines
Documentation/devicetree/bindings/iio/proximity/everlight,pm16d17.yaml new file +10
-0+$id: http://devicetree.org/schemas/iio/proximity/everlight,pm16d17.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Everlight PM-16D17 Ambient Light & Proximity Sensor + +maintainers: + - Chao Zeng <[email protected]> + +description: | + This sensor uses standard I2C interface. Interrupt function is not covered.
Bindings should be complete, even if software doesn't use the interrupts. Can you document them please.
Documentation/devicetree/bindings/iio/proximity/everlight,pm16d17.yaml line 14
Quoted 1 line
Documentation/devicetree/bindings/iio/proximity/everlight,pm16d17.yaml new file +1
-0+ Datasheet: https://en.everlight.com/sensor/category-proximity_sensor/digital_proximity_sensor/
Do you have a link to a datasheet? The link to the pm16d17 here 404s for me.
Quoted 6 lines
Documentation/devicetree/bindings/iio/proximity/everlight,pm16d17.yaml new file +6
-0+ +properties: + compatible: + enum: + - everlight,pm16d17 +
Documentation/devicetree/bindings/iio/proximity/everlight,pm16d17.yaml line 30
Quoted 10 lines
Documentation/devicetree/bindings/iio/proximity/everlight,pm16d17.yaml new file +10
-0+ reg: + maxItems: 1 + + ps-gain: + description: Receiver gain of proximity sensor + $ref: /schemas/types.yaml#/definitions/uint32 + enum: [1, 2, 4, 8] + default: 1 + + ps-itime:
How did you get itime from conversion time? To the layman (like me!) conversion-time would make more sense...
Also, "ps"? The whole thing is a proxy sensor, so why have that prefix on properties. What is missing however is a vendor prefix.
Documentation/devicetree/bindings/iio/proximity/everlight,pm16d17.yaml line 32
Quoted 2 lines
Documentation/devicetree/bindings/iio/proximity/everlight,pm16d17.yaml new file +2
-0+ description: Conversion time for proximity sensor [ms] + $ref: /schemas/types.yaml#/definitions/string
Instead of a string, please use the -us suffix, and put this in microseconds instead.
In total, that would be s/ps-itime/everlight,conversion-time-us/.
I would, however, like to know why this is a property of the hardware. What factors do you have to consider when determining what value to put in here?
Documentation/devicetree/bindings/iio/proximity/everlight,pm16d17.yaml line 45
Quoted 13 lines
Documentation/devicetree/bindings/iio/proximity/everlight,pm16d17.yaml new file +13
-0+ enum: + - "0.4" + - "0.8" + - "1.6" + - "3.2" + - "6.3" + - "12.6" + - "25.2" + default: "0.4" + + ps-wtime: + description: Waiting time for proximity sensor [ms] + $ref: /schemas/types.yaml#/definitions/string
All of the same comments apply here. E.g. why "wtime" isntead of "waiting-time" and so on.
I would really like to know why these things are properties of the hardware, rather than something that software should control.
Quoted 4 lines
Documentation/devicetree/bindings/iio/proximity/everlight,pm16d17.yaml new file +4
-0+ enum: + - "12.5" + - "25" + - "50"
Documentation/devicetree/bindings/iio/proximity/everlight,pm16d17.yaml line 59
Quoted 10 lines
Documentation/devicetree/bindings/iio/proximity/everlight,pm16d17.yaml new file +10
-0+ - "100" + - "200" + - "400" + - "800" + - "1600" + default: "12.5" + + ps-ir-led-pulse-count: + description: IR LED drive pulse count + $ref: /schemas/types.yaml#/definitions/uint32
All custom properties require a vendor prefix, not "ps". Again, what makes this a property of the hardware, rather than something that software should control?
Documentation/devicetree/bindings/iio/proximity/everlight,pm16d17.yaml line 68
Quoted 9 lines
Documentation/devicetree/bindings/iio/proximity/everlight,pm16d17.yaml new file +9
-0+ minimum: 1 + maximum: 256 + default: 1 + + ps-offset-cancel: + description: | + When PS offset cancel function is enabled, the result of subtracting any + value specified by the PS offset cancel register from the internal PS + output data is written to the PS output data register.
Again, what makes this a property of the hardware? What hardware related factors determine that value that you put in here?
Thanks,
Conor.
Quoted 30 lines
Documentation/devicetree/bindings/iio/proximity/everlight,pm16d17.yaml new file +27
-0+ $ref: /schemas/types.yaml#/definitions/uint32 + default: 0 + maximum: 65535 + +required: + - compatible + - reg + +unevaluatedProperties: false + +examples: + - | + i2c { + #address-cells = <1>; + #size-cells = <0>; + + lightsensor: pm16d17@44 { + compatible = "everlight,pm16d17"; + reg = <0x44>; + + ps-gain = <1>; + ps-itime = "0.4"; + ps-wtime = "12.5"; + ps-ir-led-pulse-count = <1>; + ps-offset-cancel = <280>; + }; + };Signature · 1 line
2.43.0
Re: [PATCH 2/3] dt-bindings: iio: Add everlight pm16d17 binding
Quoted 145 lines · Conor Dooley
Quoted 7 lines · Jan Kiszka
From: Chao Zeng <[email protected]>
Add the binding document for the everlight pm16d17 sensor.
Signed-off-by Chao Zeng <[email protected]>Co-developed-by Baocheng Su <[email protected]>Signed-off-by Baocheng Su <[email protected]>Ditto here Jan.
Quoted 34 lines
---
.../iio/proximity/everlight,pm16d17.yaml | 95
+++++++++++++++++++1 file changed, 95 insertions(+)create mode 100644
Documentation/devicetree/bindings/iio/proximity/everlight,pm16d17.yamldiff --git a/Documentation/devicetree/bindings/iio/proximity/everlight,pm16d17 .yaml b/Documentation/devicetree/bindings/iio/proximity/everlight,pm16d17 .yaml
new file mode 100644
index 000000000000..fadc3075181a
---/dev/null
+++
b/Documentation/devicetree/bindings/iio/proximity/everlight,pm16d17 .yamlDocumentation/devicetree/bindings/iio/proximity/everlight,pm16d17.yaml new file +4
-0@@ -0,0 +1,95 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +%YAML 1.2 +--- +$id:http://devicetree.org/schemas/iio/proximity/everlight,pm16d17.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#Documentation/devicetree/bindings/iio/proximity/everlight,pm16d17.yaml new file +8
-0+ +title: Everlight PM-16D17 Ambient Light & Proximity Sensor + +maintainers: + - Chao Zeng <[email protected]> + +description: | + This sensor uses standard I2C interface. Interrupt function isQuoted 1 line
not covered.
Bindings should be complete, even if software doesn't use the interrupts. Can you document them please.
Quoted 2 lines
+ Datasheet:
https://en.everlight.com/sensor/category-proximity_sensor/digital_proximity_sensor/Do you have a link to a datasheet? The link to the pm16d17 here 404s for
me.Quoted 6 lines
Documentation/devicetree/bindings/iio/proximity/everlight,pm16d17.yaml new file +6
-0+ +properties: + compatible: + enum: + - everlight,pm16d17 +Documentation/devicetree/bindings/iio/proximity/everlight,pm16d17.yaml line 30
Quoted 10 lines
Documentation/devicetree/bindings/iio/proximity/everlight,pm16d17.yaml new file +10
-0+ reg: + maxItems: 1 + + ps-gain: + description: Receiver gain of proximity sensor + $ref: /schemas/types.yaml#/definitions/uint32 + enum: [1, 2, 4, 8] + default: 1 + + ps-itime:How did you get itime from conversion time? To the layman (like me!) conversion-time would make more sense...
Also, "ps"? The whole thing is a proxy sensor, so why have that prefix
on properties. What is missing however is a vendor prefix.Quoted 2 lines
+ description: Conversion time for proximity sensor [ms] + $ref: /schemas/types.yaml#/definitions/string
Instead of a string, please use the -us suffix, and put this in microseconds instead.
In total, that would be s/ps-itime/everlight,conversion-time-us/.
I would, however, like to know why this is a property of the hardware.
What factors do you have to consider when determining what value to put
in here?Quoted 5 lines
+ enum:
+ - "0.4"
+ - "0.8"
+ - "1.6"
+ - "3.2"Quoted 8 lines
+ - "6.3"
+ - "12.6"
+ - "25.2"
+ default: "0.4"
+
+ ps-wtime:
+ description: Waiting time for proximity sensor [ms] + $ref: /schemas/types.yaml#/definitions/stringAll of the same comments apply here. E.g. why "wtime" isntead of "waiting-time" and so on.
I would really like to know why these things are properties of the hardware, rather than something that software should control.Quoted 6 lines
+ enum:
+ - "12.5"
+ - "25"
+ - "50"
+ - "100"
+ - "200"Quoted 8 lines
+ - "400"
+ - "800"
+ - "1600"
+ default: "12.5"
+
+ ps-ir-led-pulse-count:
+ description: IR LED drive pulse count + $ref: /schemas/types.yaml#/definitions/uint32All custom properties require a vendor prefix, not "ps". Again, what makes this a property of the hardware, rather than something that software should control?
Quoted 11 lines
+ minimum: 1
+ maximum: 256
+ default: 1
+
+ ps-offset-cancel:
+ description: |
+ When PS offset cancel function is enabled, the result of subtracting any
+ value specified by the PS offset cancel register from the internal PS
+ output data is written to the PS output data register.
Quoted 6 lines
Again, what makes this a property of the hardware? What hardware related
factors determine that value that you put in here?Thanks,
Conor.
Certain parameters such as conversion time, wait time, or sampling rate are directly tied to the physical characteristics and capabilities of the sensor. These parameters are typically determined by the sensor specifications, and the datasheet usually provides recommended values for these parameters. Below is an excerpt from the datasheet:
/*
+-----------------------+-------+------+------+------+-----+----------+ | Parameter | Symbol| Min | Typ | Max | Unit| Condition| +-----------------------+-------+------+------+------+-----+----------+ | PS A/D conversion time| TPS | 21.4 | 25.2 | 28.9 | ms | PS A/DC=16bit | | PS wait time setting | TPSWAIT| 10.6| 12.5 | 14.3 | ms | 12.5ms setting | +-----------------------+-------+------+------+------+-----+----------+
*/
However, there are some similar cases in the kernel, as follows:
Documentation/devicetree/bindings/iio/proximity/devantech-srf04.yaml
- startup-time-msDocumentation/devicetree/bindings/iio/proximity/semtech,sx9310.yaml Documentation/devicetree/bindings/iio/proximity/semtech,sx9324.yaml Documentation/devicetree/bindings/iio/proximity/semtech,sx9360.yaml
- semtech,avg-pos-strength
- semtech,ph01-resolution
- semtech,input-analog-gain
- ...Documentation/devicetree/bindings/iio/proximity/vishay,vcnl3020.yaml
- vishay,led-current-microamp
This is why we are leveraging the hardware properties.
Thanks,
Hua Qian
Quoted 31 lines
Quoted 31 lines
+ $ref: /schemas/types.yaml#/definitions/uint32 + default: 0
+ maximum: 65535Documentation/devicetree/bindings/iio/proximity/everlight,pm16d17.yaml new file +24
-0+ +required: + - compatible + - reg + +unevaluatedProperties: false + +examples: + - | + i2c { + #address-cells = <1>; + #size-cells = <0>; + + lightsensor: pm16d17@44 { + compatible = "everlight,pm16d17"; + reg = <0x44>; + + ps-gain = <1>; + ps-itime = "0.4"; + ps-wtime = "12.5"; + ps-ir-led-pulse-count = <1>; + ps-offset-cancel = <280>; + }; + };Signature · 1 line
2.43.0
Signature · 3 lines
Hua Qian Li Siemens AG www.siemens.com
Re: [PATCH 2/3] dt-bindings: iio: Add everlight pm16d17 binding
Quoted 185 lines · Hua Qian
Quoted 145 lines · Conor Dooley
Quoted 7 lines · Jan Kiszka
From: Chao Zeng <[email protected]>
Add the binding document for the everlight pm16d17 sensor.
Signed-off-by Chao Zeng <[email protected]>Co-developed-by Baocheng Su <[email protected]>Signed-off-by Baocheng Su <[email protected]>Ditto here Jan.
Quoted 34 lines
---
.../iio/proximity/everlight,pm16d17.yaml | 95
+++++++++++++++++++1 file changed, 95 insertions(+)create mode 100644
Documentation/devicetree/bindings/iio/proximity/everlight,pm16d17.yamldiff --git a/Documentation/devicetree/bindings/iio/proximity/everlight,pm16d17 .yaml b/Documentation/devicetree/bindings/iio/proximity/everlight,pm16d17 .yaml
new file mode 100644
index 000000000000..fadc3075181a
---/dev/null
+++
b/Documentation/devicetree/bindings/iio/proximity/everlight,pm16d17 .yamlDocumentation/devicetree/bindings/iio/proximity/everlight,pm16d17.yaml new file +4
-0@@ -0,0 +1,95 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +%YAML 1.2 +--- +$id:http://devicetree.org/schemas/iio/proximity/everlight,pm16d17.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#Documentation/devicetree/bindings/iio/proximity/everlight,pm16d17.yaml new file +8
-0+ +title: Everlight PM-16D17 Ambient Light & Proximity Sensor + +maintainers: + - Chao Zeng <[email protected]> + +description: | + This sensor uses standard I2C interface. Interrupt function isQuoted 1 line
not covered.
Bindings should be complete, even if software doesn't use the interrupts. Can you document them please.
Quoted 2 lines
+ Datasheet:
https://en.everlight.com/sensor/category-proximity_sensor/digital_proximity_sensor/Do you have a link to a datasheet? The link to the pm16d17 here 404s for
me.Quoted 6 lines
Documentation/devicetree/bindings/iio/proximity/everlight,pm16d17.yaml new file +6
-0+ +properties: + compatible: + enum: + - everlight,pm16d17 +Documentation/devicetree/bindings/iio/proximity/everlight,pm16d17.yaml line 30
Quoted 10 lines
Documentation/devicetree/bindings/iio/proximity/everlight,pm16d17.yaml new file +10
-0+ reg: + maxItems: 1 + + ps-gain: + description: Receiver gain of proximity sensor + $ref: /schemas/types.yaml#/definitions/uint32 + enum: [1, 2, 4, 8] + default: 1 + + ps-itime:How did you get itime from conversion time? To the layman (like me!) conversion-time would make more sense...
Also, "ps"? The whole thing is a proxy sensor, so why have that prefix
on properties. What is missing however is a vendor prefix.Quoted 2 lines
+ description: Conversion time for proximity sensor [ms] + $ref: /schemas/types.yaml#/definitions/string
Instead of a string, please use the -us suffix, and put this in microseconds instead.
In total, that would be s/ps-itime/everlight,conversion-time-us/.
I would, however, like to know why this is a property of the hardware.
What factors do you have to consider when determining what value to put
in here?Quoted 5 lines
+ enum:
+ - "0.4"
+ - "0.8"
+ - "1.6"
+ - "3.2"Quoted 8 lines
+ - "6.3"
+ - "12.6"
+ - "25.2"
+ default: "0.4"
+
+ ps-wtime:
+ description: Waiting time for proximity sensor [ms] + $ref: /schemas/types.yaml#/definitions/stringAll of the same comments apply here. E.g. why "wtime" isntead of "waiting-time" and so on.
I would really like to know why these things are properties of the hardware, rather than something that software should control.Quoted 6 lines
+ enum:
+ - "12.5"
+ - "25"
+ - "50"
+ - "100"
+ - "200"Quoted 8 lines
+ - "400"
+ - "800"
+ - "1600"
+ default: "12.5"
+
+ ps-ir-led-pulse-count:
+ description: IR LED drive pulse count + $ref: /schemas/types.yaml#/definitions/uint32All custom properties require a vendor prefix, not "ps". Again, what makes this a property of the hardware, rather than something that software should control?
Quoted 11 lines
+ minimum: 1
+ maximum: 256
+ default: 1
+
+ ps-offset-cancel:
+ description: |
+ When PS offset cancel function is enabled, the result of subtracting any
+ value specified by the PS offset cancel register from the internal PS
+ output data is written to the PS output data register.Quoted 6 lines
Again, what makes this a property of the hardware? What hardware related
factors determine that value that you put in here?Thanks,
Conor.Certain parameters such as conversion time, wait time, or sampling rate are directly tied to the physical characteristics and capabilities of the sensor. These parameters are typically determined by the sensor specifications, and the datasheet usually provides recommended values for these parameters. Below is an excerpt from the datasheet:
/*
+-----------------------+-------+------+------+------+-----+----------+ | Parameter | Symbol| Min | Typ | Max | Unit| Condition| +-----------------------+-------+------+------+------+-----+----------+ | PS A/D conversion time| TPS | 21.4 | 25.2 | 28.9 | ms | PS A/DC=16bit | | PS wait time setting | TPSWAIT| 10.6| 12.5 | 14.3 | ms | 12.5ms setting | +-----------------------+-------+------+------+------+-----+----------+*/
However, there are some similar cases in the kernel, as follows:
Documentation/devicetree/bindings/iio/proximity/devantech-srf04.yaml
- startup-time-msDocumentation/devicetree/bindings/iio/proximity/semtech,sx9310.yamlDocumentation/devicetree/bindings/iio/proximity/semtech,sx9324.yamlDocumentation/devicetree/bindings/iio/proximity/semtech,sx9360.yaml
- semtech,avg-pos-strength
- semtech,ph01-resolution
- semtech,input-analog-gain
- ...Documentation/devicetree/bindings/iio/proximity/vishay,vcnl3020.yaml
- vishay,led-current-microamp
Quoted 1 line
This is why we are leveraging the hardware properties.
"Other people did it" is not sufficient justification, you need to independently justify the properties you add. It appears however, that Jonathan (who understands these devices better than I, and had a functioning datasheet link), is also of the opinion that these would be better suited as userspace controls or require improved explanations. I suggest you read and reply to his mail in an itemised manner.
Cheers,
Conor.
Jonathan Cameron
maintainerRe: [PATCH 2/3] dt-bindings: iio: Add everlight pm16d17 binding
In reply to Li, Hua QianQuoted 150 lines · Hua Qian
Quoted 149 lines · Conor Dooley
Quoted 7 lines · Jan Kiszka
From: Chao Zeng <[email protected]>
Add the binding document for the everlight pm16d17 sensor.
Signed-off-by Chao Zeng <[email protected]>Co-developed-by Baocheng Su <[email protected]>Signed-off-by Baocheng Su <[email protected]>Ditto here Jan.
Quoted 34 lines
---
.../iio/proximity/everlight,pm16d17.yaml | 95
+++++++++++++++++++1 file changed, 95 insertions(+)create mode 100644
Documentation/devicetree/bindings/iio/proximity/everlight,pm16d17.yamldiff --git a/Documentation/devicetree/bindings/iio/proximity/everlight,pm16d17 .yaml b/Documentation/devicetree/bindings/iio/proximity/everlight,pm16d17 .yaml
new file mode 100644
index 000000000000..fadc3075181a
---/dev/null
+++
b/Documentation/devicetree/bindings/iio/proximity/everlight,pm16d17 .yamlDocumentation/devicetree/bindings/iio/proximity/everlight,pm16d17.yaml new file +4
-0@@ -0,0 +1,95 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +%YAML 1.2 +--- +$id:http://devicetree.org/schemas/iio/proximity/everlight,pm16d17.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#Documentation/devicetree/bindings/iio/proximity/everlight,pm16d17.yaml new file +8
-0+ +title: Everlight PM-16D17 Ambient Light & Proximity Sensor + +maintainers: + - Chao Zeng <[email protected]> + +description: | + This sensor uses standard I2C interface. Interrupt function isQuoted 1 line
not covered.
Bindings should be complete, even if software doesn't use the interrupts. Can you document them please.
Quoted 2 lines
+ Datasheet:
https://en.everlight.com/sensor/category-proximity_sensor/digital_proximity_sensor/Do you have a link to a datasheet? The link to the pm16d17 here 404s for
me.Quoted 6 lines
Documentation/devicetree/bindings/iio/proximity/everlight,pm16d17.yaml new file +6
-0+ +properties: + compatible: + enum: + - everlight,pm16d17 +Documentation/devicetree/bindings/iio/proximity/everlight,pm16d17.yaml line 30
Quoted 10 lines
Documentation/devicetree/bindings/iio/proximity/everlight,pm16d17.yaml new file +10
-0+ reg: + maxItems: 1 + + ps-gain: + description: Receiver gain of proximity sensor + $ref: /schemas/types.yaml#/definitions/uint32 + enum: [1, 2, 4, 8] + default: 1 + + ps-itime:How did you get itime from conversion time? To the layman (like me!) conversion-time would make more sense...
Also, "ps"? The whole thing is a proxy sensor, so why have that prefix
on properties. What is missing however is a vendor prefix.Quoted 2 lines
+ description: Conversion time for proximity sensor [ms] + $ref: /schemas/types.yaml#/definitions/string
Instead of a string, please use the -us suffix, and put this in microseconds instead.
In total, that would be s/ps-itime/everlight,conversion-time-us/.
I would, however, like to know why this is a property of the hardware.
What factors do you have to consider when determining what value to put
in here?Quoted 5 lines
+ enum:
+ - "0.4"
+ - "0.8"
+ - "1.6"
+ - "3.2"Quoted 8 lines
+ - "6.3"
+ - "12.6"
+ - "25.2"
+ default: "0.4"
+
+ ps-wtime:
+ description: Waiting time for proximity sensor [ms] + $ref: /schemas/types.yaml#/definitions/stringAll of the same comments apply here. E.g. why "wtime" isntead of "waiting-time" and so on.
I would really like to know why these things are properties of the hardware, rather than something that software should control.Quoted 6 lines
+ enum:
+ - "12.5"
+ - "25"
+ - "50"
+ - "100"
+ - "200"Quoted 8 lines
+ - "400"
+ - "800"
+ - "1600"
+ default: "12.5"
+
+ ps-ir-led-pulse-count:
+ description: IR LED drive pulse count + $ref: /schemas/types.yaml#/definitions/uint32All custom properties require a vendor prefix, not "ps". Again, what makes this a property of the hardware, rather than something that software should control?
Quoted 11 lines
+ minimum: 1
+ maximum: 256
+ default: 1
+
+ ps-offset-cancel:
+ description: |
+ When PS offset cancel function is enabled, the result of subtracting any
+ value specified by the PS offset cancel register from the internal PS
+ output data is written to the PS output data register.Again, what makes this a property of the hardware? What hardware related
factors determine that value that you put in here?
Quoted 6 lines
Quoted 2 lines
Thanks,
Conor.Certain parameters such as conversion time, wait time, or sampling rate are directly tied to the physical characteristics and capabilities of the sensor.
Ah. I think I'd missed this uses an external LED (rather than it being in package). In that case, the characteristics that 'work' for proximity sensing are somewhat dependent on the system design (simplifying heavily, led output for a given current, optical filter
on that LED etc).
For the sensor specific side, it should be just from the compatible, but when another part is involved, DT binding based tuning may make sense as long as it is 'per consumer device / board' not per specific instance.
Quoted 13 lines
These parameters are typically determined by the sensor specifications, and the datasheet usually provides recommended values for these parameters. Below is an excerpt from the datasheet:
/*
+-----------------------+-------+------+------+------+-----+----------+ | Parameter | Symbol| Min | Typ | Max | Unit| Condition| +-----------------------+-------+------+------+------+-----+----------+ | PS A/D conversion time| TPS | 21.4 | 25.2 | 28.9 | ms | PS A/DC=16bit | | PS wait time setting | TPSWAIT| 10.6| 12.5 | 14.3 | ms | 12.5ms setting | +-----------------------+-------+------+------+------+-----+----------+
Quoted 1 line
*/
Doesn't apply to everything you have here though. wtime / wait time is about how often you get a reading not the physical device. How is that affected by the physical device?
I 'think' the table above is giving precision of the value for a particular control setting. If you set wtime to 12.5msec (value 0 in register) then it will actually be between 10.6 and 14.3 msec, not that you should set it to 12.5msec.
There are 3 controls related to gain that you could argue for defaults for in DT (maybe) but given proximity sensing is also about the target, not just the measurement device, there won't be a right answer unless your proximity sensor is being used for a fixed purpose (e.g. WIFI signal strength limiting or a button type control).
Quoted 6 lines
However, there are some similar cases in the kernel, as follows:
Documentation/devicetree/bindings/iio/proximity/devantech-srf04.yaml
- startup-time-ms
That's after a resume and I think depends one exactly what the circuitry is (in this case the device is more of a reference design than a single device).
Quoted 6 lines
Documentation/devicetree/bindings/iio/proximity/semtech,sx9310.yamlDocumentation/devicetree/bindings/iio/proximity/semtech,sx9324.yamlDocumentation/devicetree/bindings/iio/proximity/semtech,sx9360.yaml
- semtech,avg-pos-strength
- semtech,ph01-resolution
- semtech,input-analog-gain
These are SAR sensors I think, so the sensor element is external to the device. In theory we could have described the sensing element and used that to work out the right values of these, but in practice it was easier to just provide the parameters from some 'per design' tuning.
Quoted 3 lines
- ...
Documentation/devicetree/bindings/iio/proximity/vishay,vcnl3020.yaml
- vishay,led-current-microamp
I think this is about whether you can burn the external LED out or not. On the datasheet I'm looking at for this device, only value 000 is specified in this 3bit field so I guess it's not controllable?
Pulse counts are less likely to be relevant to the LED burning out, but maybe(?)
Anyhow, it's not entirely obvious to me that it makes sense to control so much in DT for this device. Better to put it in userspace control and rely on udev etc setting things right for a given device + application.
Jonathan
Quoted 38 lines
This is why we are leveraging the hardware properties.
Thanks,
Hua QianQuoted 31 lines
Quoted 31 lines
+ $ref: /schemas/types.yaml#/definitions/uint32 + default: 0
+ maximum: 65535Documentation/devicetree/bindings/iio/proximity/everlight,pm16d17.yaml new file +24
-0+ +required: + - compatible + - reg + +unevaluatedProperties: false + +examples: + - | + i2c { + #address-cells = <1>; + #size-cells = <0>; + + lightsensor: pm16d17@44 { + compatible = "everlight,pm16d17"; + reg = <0x44>; + + ps-gain = <1>; + ps-itime = "0.4"; + ps-wtime = "12.5"; + ps-ir-led-pulse-count = <1>; + ps-offset-cancel = <280>; + }; + };Signature · 1 line
2.43.0
Re: [PATCH 2/3] dt-bindings: iio: Add everlight pm16d17 binding
Quoted 69 lines · Jonathan Cameron
Quoted 66 lines · Hua Qian
Quoted 65 lines · Conor Dooley
Quoted 7 lines · Jan Kiszka
From: Chao Zeng <[email protected]>
Add the binding document for the everlight pm16d17 sensor.
Signed-off-by Chao Zeng <[email protected]>Co-developed-by Baocheng Su <[email protected]>Signed-off-by Baocheng Su <[email protected]>Ditto here Jan.
Quoted 39 lines
--- .../iio/proximity/everlight,pm16d17.yaml | 95 +++++++++++++++++++ 1 file changed, 95 insertions(+) create mode 100644
Documentation/devicetree/bindings/iio/proximity/everlight,pm16d17.y
amldiff --git a/Documentation/devicetree/bindings/iio/proximity/everlight,pm1 6d17
.yaml b/Documentation/devicetree/bindings/iio/proximity/everlight,pm1 6d17
.yaml
new file mode 100644
index 000000000000..fadc3075181a
---/dev/null
+++
b/Documentation/devicetree/bindings/iio/proximity/everlight,pm1 6d17
.yamlDocumentation/devicetree/bindings/iio/proximity/everlight,pm16d17.yaml new file +4
-0@@ -0,0 +1,95 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +%YAML 1.2 +--- +$id:http://devicetree.org/schemas/iio/proximity/everlight,pm16d17.yaml#
+$schema:
http://devicetree.org/meta-schemas/core.yaml#Documentation/devicetree/bindings/iio/proximity/everlight,pm16d17.yaml new file +8
-0+ +title: Everlight PM-16D17 Ambient Light & Proximity Sensor + +maintainers: + - Chao Zeng <[email protected]> + +description: | + This sensor uses standard I2C interface. Interrupt functionQuoted 2 lines
is
not covered.Bindings should be complete, even if software doesn't use the interrupts. Can you document them please.
Quoted 3 lines
+ Datasheet:
https://en.everlight.com/sensor/category-proximity_sensor/digital_proximity_sensor/Do you have a link to a datasheet? The link to the pm16d17 here 404s
for
me.
Quoted 15 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Documentation/devicetree/bindings/iio/proximity/everlight,pm16d17.yaml new file +7
-0+ +properties: + compatible: + enum: + - everlight,pm16d17 + + reg:Quoted 8 lines
Quoted 8 lines
Documentation/devicetree/bindings/iio/proximity/everlight,pm16d17.yaml line 29
Quoted 8 lines
Documentation/devicetree/bindings/iio/proximity/everlight,pm16d17.yaml new file +8
-0+ maxItems: 1 + + ps-gain: + description: Receiver gain of proximity sensor + $ref: /schemas/types.yaml#/definitions/uint32 + enum: [1, 2, 4, 8] + default: 1 +
How about using in_proximity0_hw_gain as a userspace interface here?
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 1 line
+ ps-itime:
How did you get itime from conversion time? To the layman (like me!)
conversion-time would make more sense...Also, "ps"? The whole thing is a proxy sensor, so why have that prefix
on properties. What is missing however is a vendor prefix.
How about using in_proximity0_conversion_time as a userspace interface here?
Quoted 30 lines
Quoted 30 lines
Quoted 30 lines
Review
Quoted 3 lines
(quoted diff) +2
-0+ description: Conversion time for proximity sensor [ms] + $ref: /schemas/types.yaml#/definitions/stringInstead of a string, please use the -us suffix, and put this in microseconds instead.
In total, that would be s/ps-itime/everlight,conversion-time-us/.
I would, however, like to know why this is a property of the hardware.
What factors do you have to consider when determining what value to
put
in here?Quoted 13 lines
Documentation/devicetree/bindings/iio/proximity/everlight,pm16d17.yaml new file +13
-0+ enum: + - "0.4" + - "0.8" + - "1.6" + - "3.2" + - "6.3" + - "12.6" + - "25.2" + default: "0.4" + + ps-wtime: + description: Waiting time for proximity sensor [ms] + $ref: /schemas/types.yaml#/definitions/string
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
All of the same comments apply here. E.g. why "wtime" isntead of "waiting-time" and so on.
I would really like to know why these things are properties of the
hardware, rather than something that software should control.
How about using in_proximity0_wait_time as a userspace interface here?
Quoted 15 lines
Quoted 15 lines
Quoted 15 lines
Quoted 14 lines
Documentation/devicetree/bindings/iio/proximity/everlight,pm16d17.yaml new file +14
-0+ enum: + - "12.5" + - "25" + - "50" + - "100" + - "200" + - "400" + - "800" + - "1600" + default: "12.5" + + ps-ir-led-pulse-count: + description: IR LED drive pulse count + $ref: /schemas/types.yaml#/definitions/uint32
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
All custom properties require a vendor prefix, not "ps". Again, what
makes this a property of the hardware, rather than something that software should control?
How about using in_proximity0_pulse_count as a userspace interface here?
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Documentation/devicetree/bindings/iio/proximity/everlight,pm16d17.yaml new file +5
-0+ minimum: 1 + maximum: 256 + default: 1 + + ps-offset-cancel:
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 7 lines
(quoted diff) +2
-0+ description: | + When PS offset cancel function is enabled, the result ofsubtracting any
+ value specified by the PS offset cancel register from
the
internal PS
+ output data is written to the PS output data register.
How about using in_proximity0_offset_cancel as a userspace interface here?
Quoted 106 lines
Quoted 4 lines
Quoted 4 lines
Again, what makes this a property of the hardware? What hardware related
factors determine that value that you put in here?Quoted 8 lines
Quoted 2 lines
Thanks,
Conor.Certain parameters such as conversion time, wait time, or sampling rate
are directly tied to the physical characteristics and capabilities of
the sensor.Ah. I think I'd missed this uses an external LED (rather than it being
in package). In that case, the characteristics that 'work' for proximity sensing are somewhat dependent on the system design (simplifying heavily, led output for a given current, optical filter
on that LED etc).For the sensor specific side, it should be just from the compatible, but
when another part is involved, DT binding based tuning may make sense as long as it is 'per consumer device / board' not per specific instance.Quoted 12 lines
These parameters are typically determined by the sensor specifications, and the datasheet usually provides recommended values
for these parameters. Below is an excerpt from the datasheet:/*
+-----------------------+-------+------+------+------+-----+-------
---+Quoted 2 lines
Parameter | Symbol| Min | Typ | Max | Unit|
Condition|+-----------------------+-------+------+------+------+-----+-------
---+Quoted 7 lines
Quoted 1 line
PS A/D conversion time| TPS | 21.4 | 25.2 | 28.9 | ms | PS
A/DC=16bit |
Quoted 1 line
PS wait time setting | TPSWAIT| 10.6| 12.5 | 14.3 | ms | 12.5ms
setting |
+-----------------------+-------+------+------+------+-----+-------
---+
*/Doesn't apply to everything you have here though. wtime / wait time is about how often you get a reading not the physical device. How is that affected by the physical device?
I 'think' the table above is giving precision of the value for a particular
control setting. If you set wtime to 12.5msec (value 0 in register) then it will actually be between 10.6 and 14.3 msec, not that you should
set it to 12.5msec.There are 3 controls related to gain that you could argue for defaults
for in DT (maybe) but given proximity sensing is also about the target, not just the measurement device, there won't be a right answer
unless your proximity sensor is being used for a fixed purpose (e.g. WIFI signal strength limiting or a button type control).Quoted 7 lines
However, there are some similar cases in the kernel, as follows:
Documentation/devicetree/bindings/iio/proximity/devantech-srf04.yaml
- startup-time-msThat's after a resume and I think depends one exactly what the circuitry
is (in this case the device is more of a reference design than a single
device).Quoted 6 lines
Documentation/devicetree/bindings/iio/proximity/semtech,sx9310.yamlDocumentation/devicetree/bindings/iio/proximity/semtech,sx9324.yamlDocumentation/devicetree/bindings/iio/proximity/semtech,sx9360.yaml
- semtech,avg-pos-strength
- semtech,ph01-resolution
- semtech,input-analog-gainThese are SAR sensors I think, so the sensor element is external to the device. In theory we could have described the sensing element and used that to work out the right values of these, but in practice it was easier to just provide the parameters from some 'per design' tuning.
Quoted 4 lines
- ...
Documentation/devicetree/bindings/iio/proximity/vishay,vcnl3020.yaml
- vishay,led-current-microampI think this is about whether you can burn the external LED out or not.
On the datasheet I'm looking at for this device, only value 000 is specified in this 3bit field so I guess it's not controllable?Pulse counts are less likely to be relevant to the LED burning out, but
maybe(?)
Quoted 8 lines
Anyhow, it's not entirely obvious to me that it makes sense to control
so much in DT for this device. Better to put it in userspace control and rely on udev etc setting things right for a given device + application.Jonathan
I agree with your comments, we'll modify the DT to allow userspace control as introduced above. Consequently, all the dt properites will be removed.
Thanks,
Hua Qian
Quoted 33 lines
Quoted 33 lines
This is why we are leveraging the hardware properties.
Thanks,
Hua QianQuoted 24 lines
Quoted 24 lines
Documentation/devicetree/bindings/iio/proximity/everlight,pm16d17.yaml new file +23
-0+ $ref: /schemas/types.yaml#/definitions/uint32 + default: 0 + maximum: 65535 + +required: + - compatible + - reg + +unevaluatedProperties: false + +examples: + - | + i2c { + #address-cells = <1>; + #size-cells = <0>; + + lightsensor: pm16d17@44 { + compatible = "everlight,pm16d17"; + reg = <0x44>; + + ps-gain = <1>; + ps-itime = "0.4"; + ps-wtime = "12.5";
Quoted 9 lines
Quoted 8 lines
Quoted 7 lines
Quoted 7 lines
Documentation/devicetree/bindings/iio/proximity/everlight,pm16d17.yaml new file +4
-1+ ps-ir-led-pulse-count = <1>; + ps-offset-cancel = <280>; + }; + }; --2.43.0
--
Hua Qian Li
Siemens AG
http://www.siemens.com/
Jonathan Cameron
maintainerRe: [PATCH 2/3] dt-bindings: iio: Add everlight pm16d17 binding
Quoted 70 lines · Hua Qian
Quoted 69 lines · Jonathan Cameron
Quoted 66 lines · Hua Qian
Quoted 65 lines · Conor Dooley
Quoted 7 lines · Jan Kiszka
From: Chao Zeng <[email protected]>
Add the binding document for the everlight pm16d17 sensor.
Signed-off-by Chao Zeng <[email protected]>Co-developed-by Baocheng Su <[email protected]>Signed-off-by Baocheng Su <[email protected]>Ditto here Jan.
Quoted 39 lines
--- .../iio/proximity/everlight,pm16d17.yaml | 95 +++++++++++++++++++ 1 file changed, 95 insertions(+) create mode 100644
Documentation/devicetree/bindings/iio/proximity/everlight,pm16d17.y
amldiff --git a/Documentation/devicetree/bindings/iio/proximity/everlight,pm1 6d17
.yaml b/Documentation/devicetree/bindings/iio/proximity/everlight,pm1 6d17
.yaml
new file mode 100644
index 000000000000..fadc3075181a
---/dev/null
+++
b/Documentation/devicetree/bindings/iio/proximity/everlight,pm1 6d17
.yamlDocumentation/devicetree/bindings/iio/proximity/everlight,pm16d17.yaml new file +4
-0@@ -0,0 +1,95 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +%YAML 1.2 +--- +$id:http://devicetree.org/schemas/iio/proximity/everlight,pm16d17.yaml#
+$schema:
http://devicetree.org/meta-schemas/core.yaml#Documentation/devicetree/bindings/iio/proximity/everlight,pm16d17.yaml new file +8
-0+ +title: Everlight PM-16D17 Ambient Light & Proximity Sensor + +maintainers: + - Chao Zeng <[email protected]> + +description: | + This sensor uses standard I2C interface. Interrupt functionQuoted 2 lines
is
not covered.Bindings should be complete, even if software doesn't use the interrupts. Can you document them please.
Quoted 3 lines
+ Datasheet:
https://en.everlight.com/sensor/category-proximity_sensor/digital_proximity_sensor/Do you have a link to a datasheet? The link to the pm16d17 here 404s
for
me.
Quoted 16 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Documentation/devicetree/bindings/iio/proximity/everlight,pm16d17.yaml new file +7
-0+ +properties: + compatible: + enum: + - everlight,pm16d17 + + reg:Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Documentation/devicetree/bindings/iio/proximity/everlight,pm16d17.yaml line 29
Quoted 8 lines
Documentation/devicetree/bindings/iio/proximity/everlight,pm16d17.yaml new file +8
-0+ maxItems: 1 + + ps-gain: + description: Receiver gain of proximity sensor + $ref: /schemas/types.yaml#/definitions/uint32 + enum: [1, 2, 4, 8] + default: 1 +How about using
in_proximity0_hw_gainas a userspace interface here?
hwgain is used for cases where the gain does not affect the singal reported (so things like brightness on a time of flight sensor). If this directly affects the output values for proximity then it should be scale. If there are multiple things affecting the scale it is up to the driver to figure out the 'right' combination to satisfy the user request.
Quoted 11 lines
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 1 line
+ ps-itime:
How did you get itime from conversion time? To the layman (like me!)
conversion-time would make more sense...Also, "ps"? The whole thing is a proxy sensor, so why have that prefix
on properties. What is missing however is a vendor prefix.How about using
in_proximity0_conversion_timeas a userspace interface here?
See if you can use standard ABI. New ABI is effectively ABI that is unused by all existing software and unlikely to be adopted by anything generic in future.
We have various controls this might map to. integration_time for the amount of exposure time for a given sensor, sampling_frequency covers the rate at which the overall process occurs (1/sampling period). That covers the majority of cases. If it's something else that can't be mapped to these interfaces I want to fully understand why userspace would choose to change it.
Quoted 30 lines
Quoted 30 lines
Quoted 30 lines
Quoted 30 lines
Review
Quoted 3 lines
(quoted diff) +2
-0+ description: Conversion time for proximity sensor [ms] + $ref: /schemas/types.yaml#/definitions/stringInstead of a string, please use the -us suffix, and put this in microseconds instead.
In total, that would be s/ps-itime/everlight,conversion-time-us/.
I would, however, like to know why this is a property of the hardware.
What factors do you have to consider when determining what value to
put
in here?Quoted 13 lines
Documentation/devicetree/bindings/iio/proximity/everlight,pm16d17.yaml new file +13
-0+ enum: + - "0.4" + - "0.8" + - "1.6" + - "3.2" + - "6.3" + - "12.6" + - "25.2" + default: "0.4" + + ps-wtime: + description: Waiting time for proximity sensor [ms] + $ref: /schemas/types.yaml#/definitions/string
Quoted 8 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
All of the same comments apply here. E.g. why "wtime" isntead of "waiting-time" and so on.
I would really like to know why these things are properties of the
hardware, rather than something that software should control.How about using
in_proximity0_wait_timeas a userspace interface here?
Again, if you add new ABI, it will be unused.
So basically do not do so. Work out how to map to existing ABI.
Superficially seems like sampling_frequency = 1/(wait_time + conversion_time) And 'maybe' conversion time is close at least to 'integration_time'.
It is sometimes a pain to map to standard ABI for hardware that chooses different control schemes, but it is necessary to do so if you want to be able to use normal software / scripts etc.
If there is something we can't represent and a strong usecase for why userspace will want to directly tune it (rather than it being one element of the hardware controls that back a single userspace control) then it is fine to propose new userspace ABI along with documentation. We'll consider it, but it will be much more challenging to get accepted than using standard ABI.
Quoted 15 lines
Quoted 15 lines
Quoted 15 lines
Quoted 15 lines
Quoted 14 lines
Documentation/devicetree/bindings/iio/proximity/everlight,pm16d17.yaml new file +14
-0+ enum: + - "12.5" + - "25" + - "50" + - "100" + - "200" + - "400" + - "800" + - "1600" + default: "12.5" + + ps-ir-led-pulse-count: + description: IR LED drive pulse count + $ref: /schemas/types.yaml#/definitions/uint32
Quoted 7 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
All custom properties require a vendor prefix, not "ps". Again, what
makes this a property of the hardware, rather than something that software should control?How about using
in_proximity0_pulse_countas a userspace interface here?
This one is unsual, so I want to understand what it actually means for a user. Why would they control pulse count? What does it do?
- Increase gain / sensitivity?
- Reduce noise?
Quoted 13 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Documentation/devicetree/bindings/iio/proximity/everlight,pm16d17.yaml new file +5
-0+ minimum: 1 + maximum: 256 + default: 1 + + ps-offset-cancel:Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 7 lines
(quoted diff) +2
-0+ description: | + When PS offset cancel function is enabled, the result ofsubtracting any
+ value specified by the PS offset cancel register from
the
internal PS
+ output data is written to the PS output data register.
Quoted 2 lines
How about using
in_proximity0_offset_cancelas a userspace interface here?
This seems likely to be calibbias or offset. The cancel or not is just whether the offset value is 0 or not.
Quoted 112 lines
Quoted 112 lines
Quoted 4 lines
Quoted 4 lines
Again, what makes this a property of the hardware? What hardware related
factors determine that value that you put in here?Quoted 8 lines
Quoted 2 lines
Thanks,
Conor.Certain parameters such as conversion time, wait time, or sampling rate
are directly tied to the physical characteristics and capabilities of
the sensor.Ah. I think I'd missed this uses an external LED (rather than it being
in package). In that case, the characteristics that 'work' for proximity sensing are somewhat dependent on the system design (simplifying heavily, led output for a given current, optical filter
on that LED etc).For the sensor specific side, it should be just from the compatible, but
when another part is involved, DT binding based tuning may make sense as long as it is 'per consumer device / board' not per specific instance.Quoted 17 lines
These parameters are typically determined by the sensor specifications, and the datasheet usually provides recommended values
for these parameters. Below is an excerpt from the datasheet:/*
+-----------------------+-------+------+------+------+-----+------- ---+ > Parameter | Symbol| Min | Typ | Max | Unit| > Condition| +-----------------------+-------+------+------+------+-----+------- ---+ > PS A/D conversion time| TPS | 21.4 | 25.2 | 28.9 | ms | PS A/DC=16bit | > PS wait time setting | TPSWAIT| 10.6| 12.5 | 14.3 | ms | 12.5ms setting | +-----------------------+-------+------+------+------+-----+-------Quoted 2 lines
---+
*/Doesn't apply to everything you have here though. wtime / wait time is about how often you get a reading not the physical device. How is that affected by the physical device?
I 'think' the table above is giving precision of the value for a particular
control setting. If you set wtime to 12.5msec (value 0 in register) then it will actually be between 10.6 and 14.3 msec, not that you should
set it to 12.5msec.There are 3 controls related to gain that you could argue for defaults
for in DT (maybe) but given proximity sensing is also about the target, not just the measurement device, there won't be a right answer
unless your proximity sensor is being used for a fixed purpose (e.g. WIFI signal strength limiting or a button type control).Quoted 7 lines
However, there are some similar cases in the kernel, as follows:
Documentation/devicetree/bindings/iio/proximity/devantech-srf04.yaml
- startup-time-msThat's after a resume and I think depends one exactly what the circuitry
is (in this case the device is more of a reference design than a single
device).Quoted 6 lines
Documentation/devicetree/bindings/iio/proximity/semtech,sx9310.yamlDocumentation/devicetree/bindings/iio/proximity/semtech,sx9324.yamlDocumentation/devicetree/bindings/iio/proximity/semtech,sx9360.yaml
- semtech,avg-pos-strength
- semtech,ph01-resolution
- semtech,input-analog-gainThese are SAR sensors I think, so the sensor element is external to the device. In theory we could have described the sensing element and used that to work out the right values of these, but in practice it was easier to just provide the parameters from some 'per design' tuning.
Quoted 4 lines
- ...
Documentation/devicetree/bindings/iio/proximity/vishay,vcnl3020.yaml
- vishay,led-current-microampI think this is about whether you can burn the external LED out or not.
On the datasheet I'm looking at for this device, only value 000 is specified in this 3bit field so I guess it's not controllable?Pulse counts are less likely to be relevant to the LED burning out, but
maybe(?)Anyhow, it's not entirely obvious to me that it makes sense to control
so much in DT for this device. Better to put it in userspace control and rely on udev etc setting things right for a given device + application.
Quoted 5 lines
Quoted 2 lines
Jonathan
I agree with your comments, we'll modify the DT to allow userspace control as introduced above. Consequently, all the dt properites will be removed.
That's good, but do focus on how to fit existing ABI as that's what existing userspace is aware of.
Jonathan
Quoted 51 lines
Thanks,
Hua QianQuoted 33 lines
Quoted 33 lines
This is why we are leveraging the hardware properties.
Thanks,
Hua QianQuoted 24 lines
Quoted 24 lines
Documentation/devicetree/bindings/iio/proximity/everlight,pm16d17.yaml new file +23
-0+ $ref: /schemas/types.yaml#/definitions/uint32 + default: 0 + maximum: 65535 + +required: + - compatible + - reg + +unevaluatedProperties: false + +examples: + - | + i2c { + #address-cells = <1>; + #size-cells = <0>; + + lightsensor: pm16d17@44 { + compatible = "everlight,pm16d17"; + reg = <0x44>; + + ps-gain = <1>; + ps-itime = "0.4"; + ps-wtime = "12.5";Quoted 9 lines
Quoted 8 lines
Quoted 7 lines
Quoted 7 lines
Documentation/devicetree/bindings/iio/proximity/everlight,pm16d17.yaml new file +4
-1+ ps-ir-led-pulse-count = <1>; + ps-offset-cancel = <280>; + }; + }; --2.43.0
--
Hua Qian Li
Siemens AG
http://www.siemens.com/
Jonathan Cameron
maintainerRe: [PATCH 2/3] dt-bindings: iio: Add everlight pm16d17 binding
In reply to Jan KiszkaQuoted 7 lines · On Tue, 13 Aug 2024 07:40:41 +0200 Jan Kiszka <[email protected]> wrote:
From: Chao Zeng <[email protected]>
Add the binding document for the everlight pm16d17 sensor.
Signed-off-by Chao Zeng <[email protected]>Co-developed-by Baocheng Su <[email protected]>Signed-off-by Baocheng Su <[email protected]>
hi Jan,
From a first read at least, almost everything in here is stuff we should be controlling from the driver, not providing as fixed values from firmware.
Specific comments inline.
Jonathan
Quoted 29 lines
--- .../iio/proximity/everlight,pm16d17.yaml | 95 +++++++++++++++++++ 1 file changed, 95 insertions(+) create mode 100644 Documentation/devicetree/bindings/iio/proximity/everlight,pm16d17.yamlDocumentation/devicetree/bindings/iio/proximity/everlight,pm16d17.yaml new file +18
-0@@ -0,0 +1,95 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/iio/proximity/everlight,pm16d17.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Everlight PM-16D17 Ambient Light & Proximity Sensor + +maintainers: + - Chao Zeng <[email protected]> + +description: | + This sensor uses standard I2C interface. Interrupt function is not covered. + Datasheet: https://en.everlight.com/sensor/category-proximity_sensor/digital_proximity_sensor/ + +properties: + compatible: + enum:
Documentation/devicetree/bindings/iio/proximity/everlight,pm16d17.yaml line 28
Quoted 10 lines
Documentation/devicetree/bindings/iio/proximity/everlight,pm16d17.yaml new file +10
-0+ - everlight,pm16d17 + + reg: + maxItems: 1 + + ps-gain: + description: Receiver gain of proximity sensor + $ref: /schemas/types.yaml#/definitions/uint32 + enum: [1, 2, 4, 8] + default: 1
This should I think be a userspace control. Given it's not related to proximity as such, probably in_proximity0_hardwaregain
Documentation/devicetree/bindings/iio/proximity/everlight,pm16d17.yaml line 41
Quoted 13 lines
Documentation/devicetree/bindings/iio/proximity/everlight,pm16d17.yaml new file +13
-0+ + ps-itime: + description: Conversion time for proximity sensor [ms] + $ref: /schemas/types.yaml#/definitions/string + enum: + - "0.4" + - "0.8" + - "1.6" + - "3.2" + - "6.3" + - "12.6" + - "25.2" + default: "0.4"
Definitely a userspace control. Is this actually integration time which we'd expect to affect the hardwaregain or is it just 1/ frequency of sampling (with fixed integration time). Looking at datasheet it's coupled to resolution which may make this oversampling related. Hard to tell.
Documentation/devicetree/bindings/iio/proximity/everlight,pm16d17.yaml line 44
Quoted 3 lines
Documentation/devicetree/bindings/iio/proximity/everlight,pm16d17.yaml new file +3
-0+ + ps-wtime: + description: Waiting time for proximity sensor [ms]
I guess the above was the integration time and this sets the sampling_frequency. In that case definitely a userspace thing, doesn't belong in DT.
Quoted 4 lines
Documentation/devicetree/bindings/iio/proximity/everlight,pm16d17.yaml new file +4
-0+ $ref: /schemas/types.yaml#/definitions/string + enum: + - "12.5" + - "25"
Documentation/devicetree/bindings/iio/proximity/everlight,pm16d17.yaml line 58
Quoted 10 lines
Documentation/devicetree/bindings/iio/proximity/everlight,pm16d17.yaml new file +10
-0+ - "50" + - "100" + - "200" + - "400" + - "800" + - "1600" + default: "12.5" + + ps-ir-led-pulse-count: + description: IR LED drive pulse count
This needs more information. Why would this be changed? Seems from datasheet that this is effectively a different form of gain. Why would we choose one rather than the other? Or are they both ways of increasing the overall sensitivity?
Documentation/devicetree/bindings/iio/proximity/everlight,pm16d17.yaml line 68
Quoted 10 lines
Documentation/devicetree/bindings/iio/proximity/everlight,pm16d17.yaml new file +10
-0+ $ref: /schemas/types.yaml#/definitions/uint32 + minimum: 1 + maximum: 256 + default: 1 + + ps-offset-cancel: + description: | + When PS offset cancel function is enabled, the result of subtracting any + value specified by the PS offset cancel register from the internal PS + output data is written to the PS output data register.
That sounds like a calibbias userspace control, but more info needed.
Documentation/devicetree/bindings/iio/proximity/everlight,pm16d17.yaml line 72
Quoted 4 lines
Documentation/devicetree/bindings/iio/proximity/everlight,pm16d17.yaml new file +4
-0+ $ref: /schemas/types.yaml#/definitions/uint32 + default: 0 + maximum: 65535 +
As Conor mentioned, need to describe the hardware as fully as possible so interrupts and power supplies (even if they are always on for your particular board)
Quoted 23 lines
Documentation/devicetree/bindings/iio/proximity/everlight,pm16d17.yaml new file +23
-0+required: + - compatible + - reg + +unevaluatedProperties: false + +examples: + - | + i2c { + #address-cells = <1>; + #size-cells = <0>; + + lightsensor: pm16d17@44 { + compatible = "everlight,pm16d17"; + reg = <0x44>; + + ps-gain = <1>; + ps-itime = "0.4"; + ps-wtime = "12.5"; + ps-ir-led-pulse-count = <1>; + ps-offset-cancel = <280>; + }; + };
Re: [PATCH 2/3] dt-bindings: iio: Add everlight pm16d17 binding
Quoted 10 lines · Jonathan Cameron
Quoted 7 lines · On Tue, 13 Aug 2024 07:40:41 +0200 Jan Kiszka <[email protected]> wrote:
From: Chao Zeng <[email protected]>
Add the binding document for the everlight pm16d17 sensor.
Signed-off-by Chao Zeng <[email protected]>Co-developed-by Baocheng Su <[email protected]>Signed-off-by Baocheng Su <[email protected]>
Quoted 8 lines
hi Jan,
From a first read at least, almost everything in here is stuff we should be controlling from the driver, not providing as fixed values from firmware.
Specific comments inline.
Thanks for the feedback. I'm looping in my colleague Hua Quian who will try to answer your valid questions ASAP.
Good news: The datasheet link is now working, see
https://www.everlight.com.cn/wp-content/plugins/ItemRelationship/product_files/pdf/PM-16D17-2010-DF6-TR8_datasheet_V1.pdf
We just hope that we don't need to expand the IIO subsystem for this device. ;)
Jan
Quoted 135 lines
Jonathan
Quoted 29 lines
--- .../iio/proximity/everlight,pm16d17.yaml | 95 +++++++++++++++++++ 1 file changed, 95 insertions(+) create mode 100644 Documentation/devicetree/bindings/iio/proximity/everlight,pm16d17.yamlDocumentation/devicetree/bindings/iio/proximity/everlight,pm16d17.yaml new file +18
-0@@ -0,0 +1,95 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/iio/proximity/everlight,pm16d17.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Everlight PM-16D17 Ambient Light & Proximity Sensor + +maintainers: + - Chao Zeng <[email protected]> + +description: | + This sensor uses standard I2C interface. Interrupt function is not covered. + Datasheet: https://en.everlight.com/sensor/category-proximity_sensor/digital_proximity_sensor/ + +properties: + compatible: + enum:Documentation/devicetree/bindings/iio/proximity/everlight,pm16d17.yaml line 28
Quoted 10 lines
Documentation/devicetree/bindings/iio/proximity/everlight,pm16d17.yaml new file +10
-0+ - everlight,pm16d17 + + reg: + maxItems: 1 + + ps-gain: + description: Receiver gain of proximity sensor + $ref: /schemas/types.yaml#/definitions/uint32 + enum: [1, 2, 4, 8] + default: 1This should I think be a userspace control. Given it's not related to proximity as such, probably
in_proximity0_hardwaregainDocumentation/devicetree/bindings/iio/proximity/everlight,pm16d17.yaml line 41
Quoted 13 lines
Documentation/devicetree/bindings/iio/proximity/everlight,pm16d17.yaml new file +13
-0+ + ps-itime: + description: Conversion time for proximity sensor [ms] + $ref: /schemas/types.yaml#/definitions/string + enum: + - "0.4" + - "0.8" + - "1.6" + - "3.2" + - "6.3" + - "12.6" + - "25.2" + default: "0.4"Definitely a userspace control. Is this actually integration time which we'd expect to affect the hardwaregain or is it just 1/ frequency of sampling (with fixed integration time). Looking at datasheet it's coupled to resolution which may make this oversampling related. Hard to tell.
Documentation/devicetree/bindings/iio/proximity/everlight,pm16d17.yaml line 44
Quoted 3 lines
Documentation/devicetree/bindings/iio/proximity/everlight,pm16d17.yaml new file +3
-0+ + ps-wtime: + description: Waiting time for proximity sensor [ms]I guess the above was the integration time and this sets the
sampling_frequency. In that case definitely a userspace thing, doesn't belong in DT.Quoted 4 lines
Documentation/devicetree/bindings/iio/proximity/everlight,pm16d17.yaml new file +4
-0+ $ref: /schemas/types.yaml#/definitions/string + enum: + - "12.5" + - "25"Documentation/devicetree/bindings/iio/proximity/everlight,pm16d17.yaml line 58
Quoted 10 lines
Documentation/devicetree/bindings/iio/proximity/everlight,pm16d17.yaml new file +10
-0+ - "50" + - "100" + - "200" + - "400" + - "800" + - "1600" + default: "12.5" + + ps-ir-led-pulse-count: + description: IR LED drive pulse countThis needs more information. Why would this be changed? Seems from datasheet that this is effectively a different form of gain. Why would we choose one rather than the other? Or are they both ways of increasing the overall sensitivity?
Documentation/devicetree/bindings/iio/proximity/everlight,pm16d17.yaml line 68
Quoted 10 lines
Documentation/devicetree/bindings/iio/proximity/everlight,pm16d17.yaml new file +10
-0+ $ref: /schemas/types.yaml#/definitions/uint32 + minimum: 1 + maximum: 256 + default: 1 + + ps-offset-cancel: + description: | + When PS offset cancel function is enabled, the result of subtracting any + value specified by the PS offset cancel register from the internal PS + output data is written to the PS output data register.That sounds like a calibbias userspace control, but more info needed.
Documentation/devicetree/bindings/iio/proximity/everlight,pm16d17.yaml line 72
Quoted 4 lines
Documentation/devicetree/bindings/iio/proximity/everlight,pm16d17.yaml new file +4
-0+ $ref: /schemas/types.yaml#/definitions/uint32 + default: 0 + maximum: 65535 +As Conor mentioned, need to describe the hardware as fully as possible so interrupts and power supplies (even if they are always on for your particular board)
Quoted 23 lines
Documentation/devicetree/bindings/iio/proximity/everlight,pm16d17.yaml new file +23
-0+required: + - compatible + - reg + +unevaluatedProperties: false + +examples: + - | + i2c { + #address-cells = <1>; + #size-cells = <0>; + + lightsensor: pm16d17@44 { + compatible = "everlight,pm16d17"; + reg = <0x44>; + + ps-gain = <1>; + ps-itime = "0.4"; + ps-wtime = "12.5"; + ps-ir-led-pulse-count = <1>; + ps-offset-cancel = <280>; + }; + };
Signature · 2 lines
Siemens AG, Technology Linux Expert Center
PATCH 3/3 iio: proximity: Add support for everlight pmd16d17 sensor
In reply to Jan KiszkaFrom: Chao Zeng <[email protected]>
Add initial support for everlight pm16d17 proximity sensor.
--- drivers/iio/proximity/Kconfig | 11 ++ drivers/iio/proximity/Makefile | 1 + drivers/iio/proximity/pm16d17.c | 324 ++++++++++++++++++++++++++++++++ 3 files changed, 336 insertions(+) create mode 100644 drivers/iio/proximity/pm16d17.c
drivers/iio/proximity/Kconfig +11 -0
@@ -96,6 +96,17 @@ config PING To compile this driver as a module, choose M here: the module will be called ping. +config PM16D17 + tristate "PM16D17 proximity sensor" + select REGMAP_I2C + depends on I2C + help + Say Y here to build a driver for the Everlight Devices + PM16D17 proximity sensor. + + To compile this driver as a module, choose M here: the + module will be called pm16d17. + config RFD77402 tristate "RFD77402 ToF sensor" depends on I2C
drivers/iio/proximity/Makefile +1 -0
@@ -11,6 +11,7 @@ obj-$(CONFIG_ISL29501) += isl29501.o obj-$(CONFIG_LIDAR_LITE_V2) += pulsedlight-lidar-lite-v2.o obj-$(CONFIG_MB1232) += mb1232.o obj-$(CONFIG_PING) += ping.o +obj-$(CONFIG_PM16D17) += pm16d17.o obj-$(CONFIG_RFD77402) += rfd77402.o obj-$(CONFIG_SRF04) += srf04.o obj-$(CONFIG_SRF08) += srf08.o
drivers/iio/proximity/pm16d17.c new file +324 -0
324 lines not on this page · raw message
Signature · 1 line
2.43.0
kernel test robot
build botRe: [PATCH 3/3] iio: proximity: Add support for everlight pmd16d17 sensor
kernel test robot noticed the following build warnings:
Hi Jan,
kernel test robot noticed the following build warnings:
[auto build test WARNING on jic23-iio/togreg]
[also build test WARNING on robh/for-next linus/master v6.11-rc3 next-20240814] [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/Jan-Kiszka/dt-bindings-vendor-prefixes-Add-EVERLIGHT/20240814-234801 base: https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg patch link: https://lore.kernel.org/r/abb0c1c0724be733138276f638e43e98784bd191.1723527641.git.jan.kiszka%40siemens.com patch subject: [PATCH 3/3] iio: proximity: Add support for everlight pmd16d17 sensor config: x86_64-allyesconfig (https://download.01.org/0day-ci/archive/20240815/[email protected]/config) compiler: clang version 18.1.5 (https://github.com/llvm/llvm-project 617a15a9eac96088ae5e9134248d8236e34b91b1) reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240815/[email protected]/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All warnings (new ones prefixed by >>):
>> drivers/iio/proximity/pm16d17.c:142:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
142 | default:
| ^
drivers/iio/proximity/pm16d17.c:142:2: note: insert 'break;' to avoid fall-through
142 | default:
| ^
| break;
>> drivers/iio/proximity/pm16d17.c:187:3: warning: variable 'op_mode_setting_val' is uninitialized when used here [-Wuninitialized]
187 | op_mode_setting_val |= (ilog2(pgain) << 6) & PS_GAIN_MASK;
| ^~~~~~~~~~~~~~~~~~~
drivers/iio/proximity/pm16d17.c:159:29: note: initialize the variable 'op_mode_setting_val' to silence this warning
159 | uint8_t op_mode_setting_val;
| ^
| = '\0'2 warnings generated.
vim +142 drivers/iio/proximity/pm16d17.c
109
110 static int pm16d17_read_raw(struct iio_dev *indio_dev,
111 struct iio_chan_spec const *chan,
112 int *val, int *val2, long mask)
113 {
114 struct pm16d17_data *data = iio_priv(indio_dev);
115 unsigned int ps_data_l;
116 unsigned int ps_data_h;
117 uint16_t ps_data;
118 int ret = -EINVAL;
119
120 switch (mask) {
121 case IIO_CHAN_INFO_RAW:
122 switch (chan->type) {
123 case IIO_PROXIMITY:
124 ret = pm16d17_read_reg(data, PM16D17_PS_DATA_L, &ps_data_l);
125 if (ret < 0)
126 return ret;
127
128 ret = pm16d17_read_reg(data, PM16D17_PS_DATA_H, &ps_data_h);
129 if (ret < 0)
130 return ret;
131
132 ps_data = (ps_data_h << 8) | ps_data_l;
133
134 dev_dbg(&data->client->dev, "PS data: %x\n", ps_data);
135
136 *val = ps_data;
137 ret = IIO_VAL_INT;
138 break;
139 default:
140 break;
141 }
> 142 default:
143 break;
144 }
145
146 return ret;
147 }
148
149 static const struct iio_info pm16d17_info = {
150 .read_raw = pm16d17_read_raw,
151 };
152
153 static int pm16d17_chip_init(struct pm16d17_data *data)
154 {
155 struct i2c_client *client = data->client;
156 struct device_node *np = client->dev.of_node;
157 const char *conv_time = NULL;
158 const char *wait_time = NULL;
159 uint8_t op_mode_setting_val;
160 uint32_t ps_offset_cancel;
161 uint8_t offset_lsb;
162 uint8_t offset_msb;
163 uint32_t pulse_count;
164 uint32_t pgain;
165 unsigned int val;
166 int ret;
167
168 ret = pm16d17_read_reg(data, PM16D17_DEV_ID, &val);
169
170 if (ret < 0 || (val != DEVICE_ID)) {
171 dev_err(&client->dev, "Invalid chip id 0x%04x\n", val);
172 return -ENODEV;
173 }
174
175 dev_dbg(&client->dev, "Detected PM16D17 with chip id: 0x%04x\n", val);
176
177 ret = pm16d17_write_reg(data, PM16D17_OP_MODE, ENABLE_PS_FUNCTION);
178 if (ret < 0)
179 return ret;
180
181 of_property_read_u32(np, "ps-gain", &pgain);
182 switch (pgain) {
183 case 1:
184 case 2:
185 case 4:
186 case 8:
> 187 op_mode_setting_val |= (ilog2(pgain) << 6) & PS_GAIN_MASK; 188 break;
189 default:
190 break;
191 }
192
193 of_property_read_string(np, "ps-itime", &conv_time);
194 if (strcmp(conv_time, "0.4") == 0)
195 op_mode_setting_val |= PITIME_0_POINT_4_MS & PS_ITIME_MASK;
196 else if (strcmp(conv_time, "0.8") == 0)
197 op_mode_setting_val |= PITIME_0_POINT_8_MS & PS_ITIME_MASK;
198 else if (strcmp(conv_time, "1.6") == 0)
199 op_mode_setting_val |= PITIME_1_POINT_6_MS & PS_ITIME_MASK;
200 else if (strcmp(conv_time, "3.2") == 0)
201 op_mode_setting_val |= PITIME_3_POINT_2_MS & PS_ITIME_MASK;
202 else if (strcmp(conv_time, "6.3") == 0)
203 op_mode_setting_val |= PITIME_6_POINT_3_MS & PS_ITIME_MASK;
204 else if (strcmp(conv_time, "12.6") == 0)
205 op_mode_setting_val |= PITIME_12_POINT_6_MS & PS_ITIME_MASK;
206 else if (strcmp(conv_time, "25.2") == 0)
207 op_mode_setting_val |= PITIME_25_POINT_2_MS & PS_ITIME_MASK;
208 else {
209 dev_info(&client->dev, "Using default ps itime value\n");
210 op_mode_setting_val |= PITIME_0_POINT_4_MS & PS_ITIME_MASK;
211 }
212
213 of_property_read_string(np, "ps-wtime", &wait_time);
214 if (strcmp(wait_time, "12.5") == 0)
215 op_mode_setting_val |= PWTIME_12_POINT_5_MS & PS_WTIME_MASK;
216 else if (strcmp(wait_time, "25") == 0)
217 op_mode_setting_val |= PWTIME_25_MS & PS_WTIME_MASK;
218 else if (strcmp(wait_time, "50") == 0)
219 op_mode_setting_val |= PWTIME_50_MS & PS_WTIME_MASK;
220 else if (strcmp(wait_time, "100") == 0)
221 op_mode_setting_val |= PWTIME_100_MS & PS_WTIME_MASK;
222 else if (strcmp(wait_time, "200") == 0)
223 op_mode_setting_val |= PWTIME_200_MS & PS_WTIME_MASK;
224 else if (strcmp(wait_time, "400") == 0)
225 op_mode_setting_val |= PWTIME_400_MS & PS_WTIME_MASK;
226 else if (strcmp(wait_time, "800") == 0)
227 op_mode_setting_val |= PWTIME_800_MS & PS_WTIME_MASK;
228 else if (strcmp(wait_time, "1600") == 0)
229 op_mode_setting_val |= PWTIME_1600_MS & PS_WTIME_MASK;
230 else {
231 dev_info(&client->dev, "Using default ps wtime value\n");
232 op_mode_setting_val |= PWTIME_12_POINT_5_MS & PS_WTIME_MASK;
233 }
234
235 ret = pm16d17_write_reg(data, PM16D17_PS_SETTING, op_mode_setting_val);
236 if (ret < 0)
237 return ret;
238
239 of_property_read_u32(np, "ps-ir-led-pulse-count", &pulse_count);
240 if (pulse_count > 256)
241 pulse_count = 256;
242 ret = pm16d17_write_reg(data, PM16D17_VCSEL_DRIVE_PULSE, pulse_count - 1);
243 if (ret < 0)
244 return ret;
245
246 of_property_read_u32(np, "ps-offset-cancel", &ps_offset_cancel);
247 if (ps_offset_cancel != 0) {
248 ret = pm16d17_write_reg(data, PM16D17_PS_SETTING2, OFFSET_CANCEL_ENABLE);
249 if (ret < 0)
250 return ret;
251
252 offset_lsb = ps_offset_cancel & PS_OFFSET_CANCEL_LSB_MASK;
253 offset_msb = (ps_offset_cancel & PS_OFFSET_CANCEL_MSB_MASK) >> 8;
254
255 ret = pm16d17_write_reg(data, PM16D17_PS_OFFSET_CANCEL_L, offset_lsb);
256 if (ret < 0)
257 return ret;
258
259 ret = pm16d17_write_reg(data, PM16D17_PS_OFFSET_CANCEL_H, offset_msb);
260 if (ret < 0)
261 return ret;
262 }
263
264 return 0;
265 }
266
Signature · 2 lines
0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests/wiki
kernel test robot
build botRe: [PATCH 3/3] iio: proximity: Add support for everlight pmd16d17 sensor
In reply to Jan Kiszkakernel test robot noticed the following build warnings:
Hi Jan,
kernel test robot noticed the following build warnings:
[auto build test WARNING on jic23-iio/togreg]
[also build test WARNING on robh/for-next linus/master v6.11-rc3 next-20240815] [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/Jan-Kiszka/dt-bindings-vendor-prefixes-Add-EVERLIGHT/20240814-234801 base: https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg patch link: https://lore.kernel.org/r/abb0c1c0724be733138276f638e43e98784bd191.1723527641.git.jan.kiszka%40siemens.com patch subject: [PATCH 3/3] iio: proximity: Add support for everlight pmd16d17 sensor config: x86_64-randconfig-101-20240816 (https://download.01.org/0day-ci/archive/20240816/[email protected]/config) compiler: gcc-12 (Debian 12.2.0-14) 12.2.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240816/[email protected]/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All warnings (new ones prefixed by >>):
drivers/iio/proximity/pm16d17.c: In function 'pm16d17_chip_init':
>> drivers/iio/proximity/pm16d17.c:194:13: warning: argument 1 null where non-null expected [-Wnonnull]
194 | if (strcmp(conv_time, "0.4") == 0)
| ^~~~~~~~~~~~~~~~~~~~~~~~
In file included from include/linux/bitmap.h:13,
from include/linux/cpumask.h:12,
from arch/x86/include/asm/tlbbatch.h:5,
from include/linux/mm_types_task.h:16,
from include/linux/sched.h:38,
from include/linux/delay.h:23,
from drivers/iio/proximity/pm16d17.c:11:
include/linux/string.h:156:12: note: in a call to function 'strcmp' declared 'nonnull'
156 | extern int strcmp(const char *,const char *);
| ^~~~~~
drivers/iio/proximity/pm16d17.c:196:18: warning: argument 1 null where non-null expected [-Wnonnull]
196 | else if (strcmp(conv_time, "0.8") == 0)
| ^~~~~~~~~~~~~~~~~~~~~~~~
include/linux/string.h:156:12: note: in a call to function 'strcmp' declared 'nonnull'
156 | extern int strcmp(const char *,const char *);
| ^~~~~~
drivers/iio/proximity/pm16d17.c:198:18: warning: argument 1 null where non-null expected [-Wnonnull]
198 | else if (strcmp(conv_time, "1.6") == 0)
| ^~~~~~~~~~~~~~~~~~~~~~~~
include/linux/string.h:156:12: note: in a call to function 'strcmp' declared 'nonnull'
156 | extern int strcmp(const char *,const char *);
| ^~~~~~
drivers/iio/proximity/pm16d17.c:200:18: warning: argument 1 null where non-null expected [-Wnonnull]
200 | else if (strcmp(conv_time, "3.2") == 0)
| ^~~~~~~~~~~~~~~~~~~~~~~~
include/linux/string.h:156:12: note: in a call to function 'strcmp' declared 'nonnull'
156 | extern int strcmp(const char *,const char *);
| ^~~~~~
drivers/iio/proximity/pm16d17.c:202:18: warning: argument 1 null where non-null expected [-Wnonnull]
202 | else if (strcmp(conv_time, "6.3") == 0)
| ^~~~~~~~~~~~~~~~~~~~~~~~
include/linux/string.h:156:12: note: in a call to function 'strcmp' declared 'nonnull'
156 | extern int strcmp(const char *,const char *);
| ^~~~~~
drivers/iio/proximity/pm16d17.c:204:18: warning: argument 1 null where non-null expected [-Wnonnull]
204 | else if (strcmp(conv_time, "12.6") == 0)
| ^~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/string.h:156:12: note: in a call to function 'strcmp' declared 'nonnull'
156 | extern int strcmp(const char *,const char *);
| ^~~~~~
drivers/iio/proximity/pm16d17.c:206:18: warning: argument 1 null where non-null expected [-Wnonnull]
206 | else if (strcmp(conv_time, "25.2") == 0)
| ^~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/string.h:156:12: note: in a call to function 'strcmp' declared 'nonnull'
156 | extern int strcmp(const char *,const char *);
| ^~~~~~
drivers/iio/proximity/pm16d17.c:214:13: warning: argument 1 null where non-null expected [-Wnonnull]
214 | if (strcmp(wait_time, "12.5") == 0)
| ^~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/string.h:156:12: note: in a call to function 'strcmp' declared 'nonnull'
156 | extern int strcmp(const char *,const char *);
| ^~~~~~
drivers/iio/proximity/pm16d17.c:216:18: warning: argument 1 null where non-null expected [-Wnonnull]
216 | else if (strcmp(wait_time, "25") == 0)
| ^~~~~~~~~~~~~~~~~~~~~~~
include/linux/string.h:156:12: note: in a call to function 'strcmp' declared 'nonnull'
156 | extern int strcmp(const char *,const char *);
| ^~~~~~
drivers/iio/proximity/pm16d17.c:218:18: warning: argument 1 null where non-null expected [-Wnonnull]
218 | else if (strcmp(wait_time, "50") == 0)
| ^~~~~~~~~~~~~~~~~~~~~~~
include/linux/string.h:156:12: note: in a call to function 'strcmp' declared 'nonnull'
156 | extern int strcmp(const char *,const char *);
| ^~~~~~
drivers/iio/proximity/pm16d17.c:220:18: warning: argument 1 null where non-null expected [-Wnonnull]
220 | else if (strcmp(wait_time, "100") == 0)
| ^~~~~~~~~~~~~~~~~~~~~~~~
include/linux/string.h:156:12: note: in a call to function 'strcmp' declared 'nonnull'
156 | extern int strcmp(const char *,const char *);
| ^~~~~~
drivers/iio/proximity/pm16d17.c:222:18: warning: argument 1 null where non-null expected [-Wnonnull]
222 | else if (strcmp(wait_time, "200") == 0)
| ^~~~~~~~~~~~~~~~~~~~~~~~
include/linux/string.h:156:12: note: in a call to function 'strcmp' declared 'nonnull'
156 | extern int strcmp(const char *,const char *);
| ^~~~~~
drivers/iio/proximity/pm16d17.c:224:18: warning: argument 1 null where non-null expected [-Wnonnull]
224 | else if (strcmp(wait_time, "400") == 0)
| ^~~~~~~~~~~~~~~~~~~~~~~~
include/linux/string.h:156:12: note: in a call to function 'strcmp' declared 'nonnull'
156 | extern int strcmp(const char *,const char *);
| ^~~~~~
drivers/iio/proximity/pm16d17.c:226:18: warning: argument 1 null where non-null expected [-Wnonnull]
226 | else if (strcmp(wait_time, "800") == 0)
| ^~~~~~~~~~~~~~~~~~~~~~~~
include/linux/string.h:156:12: note: in a call to function 'strcmp' declared 'nonnull'
156 | extern int strcmp(const char *,const char *);
| ^~~~~~
drivers/iio/proximity/pm16d17.c:228:18: warning: argument 1 null where non-null expected [-Wnonnull]
228 | else if (strcmp(wait_time, "1600") == 0)
| ^~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/string.h:156:12: note: in a call to function 'strcmp' declared 'nonnull'
156 | extern int strcmp(const char *,const char *);
| ^~~~~~vim +194 drivers/iio/proximity/pm16d17.c
152
153 static int pm16d17_chip_init(struct pm16d17_data *data)
154 {
155 struct i2c_client *client = data->client;
156 struct device_node *np = client->dev.of_node;
157 const char *conv_time = NULL;
158 const char *wait_time = NULL;
159 uint8_t op_mode_setting_val;
160 uint32_t ps_offset_cancel;
161 uint8_t offset_lsb;
162 uint8_t offset_msb;
163 uint32_t pulse_count;
164 uint32_t pgain;
165 unsigned int val;
166 int ret;
167
168 ret = pm16d17_read_reg(data, PM16D17_DEV_ID, &val);
169
170 if (ret < 0 || (val != DEVICE_ID)) {
171 dev_err(&client->dev, "Invalid chip id 0x%04x\n", val);
172 return -ENODEV;
173 }
174
175 dev_dbg(&client->dev, "Detected PM16D17 with chip id: 0x%04x\n", val);
176
177 ret = pm16d17_write_reg(data, PM16D17_OP_MODE, ENABLE_PS_FUNCTION);
178 if (ret < 0)
179 return ret;
180
181 of_property_read_u32(np, "ps-gain", &pgain);
182 switch (pgain) {
183 case 1:
184 case 2:
185 case 4:
186 case 8:
187 op_mode_setting_val |= (ilog2(pgain) << 6) & PS_GAIN_MASK;
188 break;
189 default:
190 break;
191 }
192
193 of_property_read_string(np, "ps-itime", &conv_time);
> 194 if (strcmp(conv_time, "0.4") == 0)
195 op_mode_setting_val |= PITIME_0_POINT_4_MS & PS_ITIME_MASK;
196 else if (strcmp(conv_time, "0.8") == 0)
197 op_mode_setting_val |= PITIME_0_POINT_8_MS & PS_ITIME_MASK;
198 else if (strcmp(conv_time, "1.6") == 0)
199 op_mode_setting_val |= PITIME_1_POINT_6_MS & PS_ITIME_MASK;
200 else if (strcmp(conv_time, "3.2") == 0)
201 op_mode_setting_val |= PITIME_3_POINT_2_MS & PS_ITIME_MASK;
202 else if (strcmp(conv_time, "6.3") == 0)
203 op_mode_setting_val |= PITIME_6_POINT_3_MS & PS_ITIME_MASK;
204 else if (strcmp(conv_time, "12.6") == 0)
205 op_mode_setting_val |= PITIME_12_POINT_6_MS & PS_ITIME_MASK;
206 else if (strcmp(conv_time, "25.2") == 0)
207 op_mode_setting_val |= PITIME_25_POINT_2_MS & PS_ITIME_MASK;
208 else {
209 dev_info(&client->dev, "Using default ps itime value\n");
210 op_mode_setting_val |= PITIME_0_POINT_4_MS & PS_ITIME_MASK;
211 }
212
213 of_property_read_string(np, "ps-wtime", &wait_time);
214 if (strcmp(wait_time, "12.5") == 0)
215 op_mode_setting_val |= PWTIME_12_POINT_5_MS & PS_WTIME_MASK;
216 else if (strcmp(wait_time, "25") == 0)
217 op_mode_setting_val |= PWTIME_25_MS & PS_WTIME_MASK;
218 else if (strcmp(wait_time, "50") == 0)
219 op_mode_setting_val |= PWTIME_50_MS & PS_WTIME_MASK;
220 else if (strcmp(wait_time, "100") == 0)
221 op_mode_setting_val |= PWTIME_100_MS & PS_WTIME_MASK;
222 else if (strcmp(wait_time, "200") == 0)
223 op_mode_setting_val |= PWTIME_200_MS & PS_WTIME_MASK;
224 else if (strcmp(wait_time, "400") == 0)
225 op_mode_setting_val |= PWTIME_400_MS & PS_WTIME_MASK;
226 else if (strcmp(wait_time, "800") == 0)
227 op_mode_setting_val |= PWTIME_800_MS & PS_WTIME_MASK;
228 else if (strcmp(wait_time, "1600") == 0)
229 op_mode_setting_val |= PWTIME_1600_MS & PS_WTIME_MASK;
230 else {
231 dev_info(&client->dev, "Using default ps wtime value\n");
232 op_mode_setting_val |= PWTIME_12_POINT_5_MS & PS_WTIME_MASK;
233 }
234
235 ret = pm16d17_write_reg(data, PM16D17_PS_SETTING, op_mode_setting_val);
236 if (ret < 0)
237 return ret;
238
239 of_property_read_u32(np, "ps-ir-led-pulse-count", &pulse_count);
240 if (pulse_count > 256)
241 pulse_count = 256;
242 ret = pm16d17_write_reg(data, PM16D17_VCSEL_DRIVE_PULSE, pulse_count - 1);
243 if (ret < 0)
244 return ret;
245
246 of_property_read_u32(np, "ps-offset-cancel", &ps_offset_cancel);
247 if (ps_offset_cancel != 0) {
248 ret = pm16d17_write_reg(data, PM16D17_PS_SETTING2, OFFSET_CANCEL_ENABLE);
249 if (ret < 0)
250 return ret;
251
252 offset_lsb = ps_offset_cancel & PS_OFFSET_CANCEL_LSB_MASK;
253 offset_msb = (ps_offset_cancel & PS_OFFSET_CANCEL_MSB_MASK) >> 8;
254
255 ret = pm16d17_write_reg(data, PM16D17_PS_OFFSET_CANCEL_L, offset_lsb);
256 if (ret < 0)
257 return ret;
258
259 ret = pm16d17_write_reg(data, PM16D17_PS_OFFSET_CANCEL_H, offset_msb);
260 if (ret < 0)
261 return ret;
262 }
263
264 return 0;
265 }
266
Signature · 2 lines
0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests/wiki
Jonathan Cameron
maintainerRe: [PATCH 3/3] iio: proximity: Add support for everlight pmd16d17 sensor
In reply to Jan KiszkaQuoted 8 lines · On Tue, 13 Aug 2024 07:40:42 +0200 Jan Kiszka <[email protected]> wrote:
From: Chao Zeng <[email protected]>
Add initial support for everlight pm16d17 proximity sensor.
Signed-off-by Chao Zeng <[email protected]>Co-developed-by Baocheng Su <[email protected]>Signed-off-by Baocheng Su <[email protected]>Signed-off-by Jan Kiszka <[email protected]>
Hi Jan,
Various comments inline.
Thanks,
Jonathan
Quoted 10 lines
drivers/iio/proximity/pm16d17.c new file +4
-0@@ -0,0 +1,324 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* +* Copyright (c) Siemens AG, 2023-2024 +*
drivers/iio/proximity/pm16d17.c line 14
Quoted 10 lines
drivers/iio/proximity/pm16d17.c new file +10
-0+* Driver for Everlight PM-16d17 proximity sensor +* +* Author: Chao Zeng <[email protected]> +*/ + +#include <linux/bitfield.h> +#include <linux/delay.h> +#include <linux/err.h> +#include <linux/i2c.h> +#include <linux/init.h>
Unlikely you need this one...
drivers/iio/proximity/pm16d17.c line 15
Quoted 1 line
drivers/iio/proximity/pm16d17.c new file +1
-0+#include <linux/interrupt.h>
Not in use yet?
drivers/iio/proximity/pm16d17.c line 17
Quoted 2 lines
drivers/iio/proximity/pm16d17.c new file +2
-0+#include <linux/irq.h> +#include <linux/module.h>
mod_devicetable.hproperty.h
Also check these again as you should as currently written have had of.h and don't, so may be other things missing.
drivers/iio/proximity/pm16d17.c line 20
Quoted 3 lines
drivers/iio/proximity/pm16d17.c new file +3
-0+#include <linux/regmap.h> +#include <linux/iio/iio.h> +#include <linux/iio/sysfs.h>
Only needed if you are doing custom attrs. So drop.
drivers/iio/proximity/pm16d17.c line 21
Quoted 1 line
drivers/iio/proximity/pm16d17.c new file +1
-0+#include <linux/iio/events.h>
Only needed for events. So drop for now.
drivers/iio/proximity/pm16d17.c line 24
Quoted 3 lines
drivers/iio/proximity/pm16d17.c new file +3
-0+ +#define PM16D17_DRV_NAME "pm16d17" +#define PM16D17_REGMAP_NAME "pm16d17_regmap"
Don't have defines for these two. See later for more comments on that.
drivers/iio/proximity/pm16d17.c line 26
Quoted 2 lines
drivers/iio/proximity/pm16d17.c new file +2
-0+ +/* Registers Address */
To keep clear distinction between fields and registers I'd add _REG to end of these.
Quoted 7 lines
drivers/iio/proximity/pm16d17.c new file +7
-0+#define PM16D17_OP_MODE 0x00 +#define PM16D17_INTERRUPT_FLAG 0x01 +#define PM16D17_PS_SETTING 0x0A +#define PM16D17_VCSEL_DRIVE_CURRENT 0x0B +#define PM16D17_VCSEL_DRIVE_PULSE 0x0C +#define PM16D17_PS_INTUPT_LTHD_L 0x0D +#define PM16D17_PS_INTUPT_LTHD_H 0x0E
drivers/iio/proximity/pm16d17.c line 43
Quoted 10 lines
drivers/iio/proximity/pm16d17.c new file +10
-0+#define PM16D17_PS_INTUPT_HTHD_L 0x0F +#define PM16D17_PS_INTUPT_HTHD_H 0x10 +#define PM16D17_PS_DATA_L 0x11 +#define PM16D17_PS_DATA_H 0x12 +#define PM16D17_PS_SETTING2 0x13 +#define PM16D17_PS_OFFSET_CANCEL_L 0x14 +#define PM16D17_PS_OFFSET_CANCEL_H 0x15 +#define PM16D17_DEV_ID 0x18 + +#define DEVICE_ID 0x11
Needs to be prefixed as DEVICE_ID is a very generic define! PM16D17_DEV_ID_PM16D17
or something like that to indicate which register and value means what.
Quoted 7 lines
drivers/iio/proximity/pm16d17.c new file +7
-0+ +#define ENABLE_PS_FUNCTION BIT(3) +#define PS_GAIN_MASK GENMASK(7, 6) +#define PS_ITIME_MASK GENMASK(5, 3) +#define PS_WTIME_MASK GENMASK(2, 0) +#define OFFSET_CANCEL_ENABLE BIT(7) +#define PS_OFFSET_CANCEL_LSB_MASK GENMASK(7, 0)
drivers/iio/proximity/pm16d17.c line 60
Quoted 10 lines
drivers/iio/proximity/pm16d17.c new file +10
-0+#define PS_OFFSET_CANCEL_MSB_MASK GENMASK(15, 8) + +enum { +PITIME_0_POINT_4_MS = (0 <<3 ), +PITIME_0_POINT_8_MS = (1 <<3 ), +PITIME_1_POINT_6_MS = (2 <<3 ), +PITIME_3_POINT_2_MS = (3 <<3 ), +PITIME_6_POINT_3_MS = (4 <<3 ), +PITIME_12_POINT_6_MS = (5 <<3 ), +PITIME_25_POINT_2_MS = (6 <<3 ),
Don't shift these values.
Use a suitable mask and FIELD_PREP() to shift them at the point where you are writing them.
Quoted 5 lines
drivers/iio/proximity/pm16d17.c new file +5
-0+}; + +enum { +PWTIME_12_POINT_5_MS =0 , +PWTIME_25_MS ,
drivers/iio/proximity/pm16d17.c line 75
Quoted 10 lines
drivers/iio/proximity/pm16d17.c new file +10
-0+ PWTIME_50_MS, +PWTIME_100_MS , +PWTIME_200_MS , +PWTIME_400_MS , +PWTIME_800_MS , + PWTIME_1600_MS, +}; + +struct pm16d17_data { +struct i2c_client *client;
As below. More useful to store &client->dev
Quoted 6 lines
drivers/iio/proximity/pm16d17.c new file +6
-0+struct regmap *regmap; +}; + +static const struct regmap_config pm16d17_regmap_config = { + .name =PM16D17_REGMAP_NAME , + .reg_bits =8 ,
drivers/iio/proximity/pm16d17.c line 91
Quoted 10 lines
drivers/iio/proximity/pm16d17.c new file +10
-0+ .val_bits = 8, + .cache_type = REGCACHE_NONE, +}; + +static const struct iio_chan_spec pm16d17_channels[] = { + { + .type =IIO_PROXIMITY , + .indexed =1 , + .channel =0 , + .scan_index =-1 ,
Don't need to set this unless you are registering the buffered interfaces. So don't set it for now.
drivers/iio/proximity/pm16d17.c line 100
Quoted 9 lines
drivers/iio/proximity/pm16d17.c new file +9
-0+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), + } +}; + +static inline int pm16d17_write_reg (struct pm16d17_data *data, +unsigned int reg, +unsigned int value) +{ +return regmap_write (data->regmap, reg, value);
Get rid of these wrappers and call regmap_write / read directly inline. These just make the code a little harder to read for no benefit.
Quoted 22 lines
drivers/iio/proximity/pm16d17.c new file +22
-0+} + +static inline unsigned int pm16d17_read_reg (struct pm16d17_data *data, +unsigned int reg, +unsigned int *reg_val) +{ +return regmap_read (data->regmap, reg, reg_val); +} + +static int pm16d17_read_raw (struct iio_dev *indio_dev, +struct iio_chan_spec const *chan, +int *val,int *val2,long mask) +{ +struct pm16d17_data *data =iio_priv (indio_dev); +unsigned int ps_data_l; +unsigned int ps_data_h; +uint16_t ps_data; +int ret = -EINVAL ; + +switch (mask) { +case IIO_CHAN_INFO_RAW : +switch (chan->type) {
drivers/iio/proximity/pm16d17.c line 132
Quoted 10 lines
drivers/iio/proximity/pm16d17.c new file +10
-0+case IIO_PROXIMITY : + ret =pm16d17_read_reg (data,PM16D17_PS_DATA_L , &ps_data_l); +if (ret <0 ) +return ret; + + ret =pm16d17_read_reg (data,PM16D17_PS_DATA_H , &ps_data_h); +if (ret <0 ) +return ret; + + ps_data = (ps_data_h <<8 ) | ps_data_l;
read both values into a u8 [2] and use get_unaligned_be16() or similar.
Actually you should be able to do a bulk read (which is obvious once you stop wrapping regmap calls with function s that suggest something more is going on).
drivers/iio/proximity/pm16d17.c line 138
Quoted 6 lines
drivers/iio/proximity/pm16d17.c new file +6
-0+ +dev_dbg (&data->client->dev,"PS data: %x\n" , ps_data); + + *val = ps_data; + ret =IIO_VAL_INT ; +break ;
return IIO_VAL_INT;
drivers/iio/proximity/pm16d17.c line 140
Quoted 2 lines
drivers/iio/proximity/pm16d17.c new file +2
-0+default : +break ;
return an error
drivers/iio/proximity/pm16d17.c line 140
Quoted 3 lines
drivers/iio/proximity/pm16d17.c new file +3
-0+ } +default : +break ;
return an error.
drivers/iio/proximity/pm16d17.c line 145
Quoted 2 lines
drivers/iio/proximity/pm16d17.c new file +2
-0+ } +
Should not be able to get here so drop this (with above changes to return early)
Quoted 14 lines
drivers/iio/proximity/pm16d17.c new file +14
-0+return ret; +} + +static const struct iio_info pm16d17_info = { + .read_raw = pm16d17_read_raw, +}; + +static int pm16d17_chip_init (struct pm16d17_data *data) +{ +struct i2c_client *client = data->client; +struct device_node *np = client->dev.of_node; +const char *conv_time =NULL ; +const char *wait_time =NULL ; +uint8_t op_mode_setting_val;
drivers/iio/proximity/pm16d17.c line 169
Quoted 10 lines
drivers/iio/proximity/pm16d17.c new file +10
-0+uint32_t ps_offset_cancel; +uint8_t offset_lsb; +uint8_t offset_msb; +uint32_t pulse_count; +uint32_t pgain; +unsigned int val; +int ret; + + ret =pm16d17_read_reg (data,PM16D17_DEV_ID , &val); +
No blank line between a call and the check of it's error. Good keep them logically associated.
drivers/iio/proximity/pm16d17.c line 171
Quoted 2 lines
drivers/iio/proximity/pm16d17.c new file +2
-0+if (ret <0 || (val !=DEVICE_ID )) { +dev_err (&client->dev,"Invalid chip id 0x%04x\n" , val);
if (ret < 0)
return dev_err_probe()
if (val != DEVICE_ID)
dev_info(&client->dev, "Unexpected chip id ..."
and carry on.
The reason for this is to enable use in future of fallback DT compatibles so if a new device is released that is backwards compatible it can be supported by existing distro kernels etc without change.
We used to get this wrong in IIO and haven't yet fixed all drivers so you will see lots of examples similar to what you have here.
Quoted 9 lines
drivers/iio/proximity/pm16d17.c new file +9
-0+return -ENODEV ; + } + +dev_dbg (&client->dev,"Detected PM16D17 with chip id: 0x%04x\n" , val); + + ret =pm16d17_write_reg (data,PM16D17_OP_MODE ,ENABLE_PS_FUNCTION ); +if (ret <0 ) +return ret; +
drivers/iio/proximity/pm16d17.c line 190
Quoted 10 lines
drivers/iio/proximity/pm16d17.c new file +10
-0+of_property_read_u32 (np,"ps-gain" , &pgain); +switch (pgain) { +case 1 : +case 2 : +case 4 : +case 8 : + op_mode_setting_val |= (ilog2 (pgain) <<6 ) &PS_GAIN_MASK ; +break ; +default : +break ;
Not an error?
drivers/iio/proximity/pm16d17.c line 193
Quoted 3 lines
drivers/iio/proximity/pm16d17.c new file +3
-0+ } + +of_property_read_string (np,"ps-itime" , &conv_time);
Is strcmp safe for NULL? Seems unlikely.
Make the time in msecs / usecs of appropriate so you can handle it as an integer.
device_property_read_string()
Use the generic firmware stuff in linux/property.h instead of of calls thoughout. That enables use of things like ACPI bindings via PRP0001 _HID
Quoted 6 lines
drivers/iio/proximity/pm16d17.c new file +6
-0+if (strcmp (conv_time,"0.4" ) ==0 ) + op_mode_setting_val |=PITIME_0_POINT_4_MS &PS_ITIME_MASK ; +else if (strcmp (conv_time,"0.8" ) ==0 ) + op_mode_setting_val |=PITIME_0_POINT_8_MS &PS_ITIME_MASK ; +else if (strcmp (conv_time,"1.6" ) ==0 ) + op_mode_setting_val |=PITIME_1_POINT_6_MS &PS_ITIME_MASK ;
drivers/iio/proximity/pm16d17.c line 209
Quoted 10 lines
drivers/iio/proximity/pm16d17.c new file +10
-0+else if (strcmp (conv_time,"3.2" ) ==0 ) + op_mode_setting_val |=PITIME_3_POINT_2_MS &PS_ITIME_MASK ; +else if (strcmp (conv_time,"6.3" ) ==0 ) + op_mode_setting_val |=PITIME_6_POINT_3_MS &PS_ITIME_MASK ; +else if (strcmp (conv_time,"12.6" ) ==0 ) + op_mode_setting_val |=PITIME_12_POINT_6_MS &PS_ITIME_MASK ; +else if (strcmp (conv_time,"25.2" ) ==0 ) + op_mode_setting_val |=PITIME_25_POINT_2_MS &PS_ITIME_MASK ; +else { +dev_info (&client->dev,"Using default ps itime value\n" );
If a property was there and invalid value error out, otherwise just use default here without printing anything.
drivers/iio/proximity/pm16d17.c line 213
Quoted 4 lines
drivers/iio/proximity/pm16d17.c new file +4
-0+ op_mode_setting_val |=PITIME_0_POINT_4_MS &PS_ITIME_MASK ; + } + +of_property_read_string (np,"ps-wtime" , &wait_time);
Again, pick units so it's an integer. Other comments as above.
Quoted 18 lines
drivers/iio/proximity/pm16d17.c new file +18
-0+if (strcmp (wait_time,"12.5" ) ==0 ) + op_mode_setting_val |=PWTIME_12_POINT_5_MS &PS_WTIME_MASK ; +else if (strcmp (wait_time,"25" ) ==0 ) + op_mode_setting_val |=PWTIME_25_MS &PS_WTIME_MASK ; +else if (strcmp (wait_time,"50" ) ==0 ) + op_mode_setting_val |=PWTIME_50_MS &PS_WTIME_MASK ; +else if (strcmp (wait_time,"100" ) ==0 ) + op_mode_setting_val |=PWTIME_100_MS &PS_WTIME_MASK ; +else if (strcmp (wait_time,"200" ) ==0 ) + op_mode_setting_val |=PWTIME_200_MS &PS_WTIME_MASK ; +else if (strcmp (wait_time,"400" ) ==0 ) + op_mode_setting_val |=PWTIME_400_MS &PS_WTIME_MASK ; +else if (strcmp (wait_time,"800" ) ==0 ) + op_mode_setting_val |=PWTIME_800_MS &PS_WTIME_MASK ; +else if (strcmp (wait_time,"1600" ) ==0 ) + op_mode_setting_val |=PWTIME_1600_MS &PS_WTIME_MASK ; +else { +dev_info (&client->dev,"Using default ps wtime value\n" );
drivers/iio/proximity/pm16d17.c line 241
Quoted 10 lines
drivers/iio/proximity/pm16d17.c new file +10
-0+ op_mode_setting_val |=PWTIME_12_POINT_5_MS &PS_WTIME_MASK ; + } + + ret =pm16d17_write_reg (data,PM16D17_PS_SETTING , op_mode_setting_val); +if (ret <0 ) +return ret; + +of_property_read_u32 (np,"ps-ir-led-pulse-count" , &pulse_count); +if (pulse_count >256 ) + pulse_count =256 ;
pulse_count = min(256, pulse_count);
Quoted 26 lines
drivers/iio/proximity/pm16d17.c new file +26
-0+ ret =pm16d17_write_reg (data,PM16D17_VCSEL_DRIVE_PULSE , pulse_count -1 ); +if (ret <0 ) +return ret; + +of_property_read_u32 (np,"ps-offset-cancel" , &ps_offset_cancel); +if (ps_offset_cancel !=0 ) { + ret =pm16d17_write_reg (data,PM16D17_PS_SETTING2 ,OFFSET_CANCEL_ENABLE ); +if (ret <0 ) +return ret; + + offset_lsb = ps_offset_cancel &PS_OFFSET_CANCEL_LSB_MASK ; + offset_msb = (ps_offset_cancel &PS_OFFSET_CANCEL_MSB_MASK ) >>8 ; + + ret =pm16d17_write_reg (data,PM16D17_PS_OFFSET_CANCEL_L , offset_lsb); +if (ret <0 ) +return ret; + + ret =pm16d17_write_reg (data,PM16D17_PS_OFFSET_CANCEL_H , offset_msb); +if (ret <0 ) +return ret; + } + +return 0 ; +} + +static int pm16d17_probe (struct i2c_client *client)
drivers/iio/proximity/pm16d17.c line 277
Quoted 10 lines
drivers/iio/proximity/pm16d17.c new file +10
-0+{ +struct pm16d17_data *data; +struct iio_dev *indio_dev; +int ret; + + indio_dev =devm_iio_device_alloc (&client->dev,sizeof (*data)); +if (!indio_dev) +return -ENOMEM ; + + indio_dev->dev.parent = &client->dev;
No need to set that - devm_iio_device_alloc() does it for you.
drivers/iio/proximity/pm16d17.c line 279
Quoted 2 lines
drivers/iio/proximity/pm16d17.c new file +2
-0+ indio_dev->info = &pm16d17_info; + indio_dev->name =PM16D17_DRV_NAME ;
There is no particularly reason why the name here and the driver name should match. As such, I'd much rather just see the explicit string listed here directly and that define being dropped.
drivers/iio/proximity/pm16d17.c line 285
Quoted 6 lines
drivers/iio/proximity/pm16d17.c new file +6
-0+ indio_dev->channels = pm16d17_channels; + indio_dev->num_channels =ARRAY_SIZE (pm16d17_channels); + indio_dev->modes =INDIO_DIRECT_MODE ; + + data =iio_priv (indio_dev); + data->client = client;
You only ever use this to get to client->dev. Just have a struct device in there instead. Or you can retrieve it via regmap_get_device() if you prefer not to store it explicitly.
drivers/iio/proximity/pm16d17.c line 290
Quoted 5 lines
drivers/iio/proximity/pm16d17.c new file +5
-0+ + data->regmap =devm_regmap_init_i2c (client, &pm16d17_regmap_config); +if (IS_ERR (data->regmap)) { +dev_err (&client->dev,"regmap initialization failed.\n" ); +return PTR_ERR (data->regmap);
For error prints in probe or functions just called from probe use
return dev_err_probe(&client->dev, PTR_ERR(data->regmap), ....);
Quoted 14 lines
drivers/iio/proximity/pm16d17.c new file +14
-0+ } + + ret =pm16d17_chip_init (data); +if (ret) +return ret; + +return devm_iio_device_register (&client->dev, indio_dev); +} + +static const struct i2c_device_id pm16d17_id[] = { + {"pm16d17" ,0 }, + {} +}; +MODULE_DEVICE_TABLE (i2c, pm16d17_id);
drivers/iio/proximity/pm16d17.c line 314
Quoted 10 lines
drivers/iio/proximity/pm16d17.c new file +10
-0+ +static const struct of_device_id pm16d17_of_match[] = { + { .compatible ="everlight,pm16d17" }, + {} +}; +MODULE_DEVICE_TABLE (of, pm16d17_of_match); + +static struct i2c_driver pm16d17_driver = { + .driver = { + .name =PM16D17_DRV_NAME ,
Put the string in directly here
Quoted 10 lines
drivers/iio/proximity/pm16d17.c new file +10
-0+ .of_match_table = pm16d17_of_match, + }, + .probe = pm16d17_probe, + .id_table = pm16d17_id, +}; +module_i2c_driver (pm16d17_driver); + +MODULE_AUTHOR ("Chao Zeng <[email protected]>" ); +MODULE_DESCRIPTION ("PM16D17 proximity sensor" ); +MODULE_LICENSE ("GPL" );