On 14.08.24 21:10, Jonathan Cameron wrote: > On Tue, 13 Aug 2024 07:40:41 +0200 > Jan Kiszka <jan.kiszka@xxxxxxxxxxx> wrote: > >> From: Chao Zeng <chao.zeng@xxxxxxxxxxx> >> >> Add the binding document for the everlight pm16d17 sensor. >> >> Signed-off-by: Chao Zeng <chao.zeng@xxxxxxxxxxx> >> Co-developed-by: Baocheng Su <baocheng.su@xxxxxxxxxxx> >> Signed-off-by: Baocheng Su <baocheng.su@xxxxxxxxxxx> > 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 > Jonathan > >> --- >> .../iio/proximity/everlight,pm16d17.yaml | 95 +++++++++++++++++++ >> 1 file changed, 95 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/iio/proximity/everlight,pm16d17.yaml >> >> diff --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.yaml >> @@ -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 <chao.zeng@xxxxxxxxxxx> >> + >> +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 > > This should I think be a userspace control. > Given it's not related to proximity as such, probably > in_proximity0_hardwaregain > >> + >> + 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. > >> + >> + 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. > >> + $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 > > 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? > >> + $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. > >> + $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) > >> +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>; >> + }; >> + }; > -- Siemens AG, Technology Linux Expert Center
