On Sat, 2024-08-17 at 14:42 +0100, Jonathan Cameron wrote: > On Fri, 16 Aug 2024 01:48:36 +0000 > "Li, Hua Qian" <HuaQian.Li@xxxxxxxxxxx> wrote: > > > On Tue, 2024-08-13 at 16:52 +0100, Conor Dooley wrote: > > > On Tue, Aug 13, 2024 at 07:40:41AM +0200, Jan Kiszka 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> > > > > > > Ditto here Jan. > > > > > > > --- > > > > .../iio/proximity/everlight,pm16d17.yaml | 95 > > > > +++++++++++++++++++ > > > > 1 file changed, 95 insertions(+) > > > > create mode 100644 > > > > Documentation/devicetree/bindings/iio/proximity/everlight,pm16d > > > > 17.y > > > > aml > > > > > > > > diff --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 > > > > .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. > > > > > > Bindings should be complete, even if software doesn't use the > > > interrupts. Can you document them please. > > > > > > > + 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. > > > > > > > + > > > > +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 > > > > + How about using `in_proximity0_hw_gain` as a userspace interface here? > > > > + 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? > > > > > > > + 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? > > > > > > > + 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. > > > How about using `in_proximity0_wait_time` as a userspace interface here? > > > > + 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 > > > > > > 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? > > > > + 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. > > > How about using `in_proximity0_offset_cancel` as a userspace interface here? > > > 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. > > 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. > > > > > > 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 | > > +-----------------------+-------+------+------+------+-----+------- > > ---+ > > */ > > 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). > > > > > > 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). > > > Documentation/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 > 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. > > > - ... > > Documentation/devicetree/bindings/iio/proximity/vishay,vcnl3020.yam > > l > > - 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 > 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 > > > > > > > This is why we are leveraging the hardware properties. > > > > Thanks, > > Hua Qian > > > > > > > > > + $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>; > > > > + }; > > > > + }; > > > > -- > > > > 2.43.0 > > > > > > > -- Hua Qian Li Siemens AG http://www.siemens.com/
