| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <a458a2cbc96a23c0a2ef89327e1f8bcd2e2777e6.camel@siemens.com>
Date: Fri, 16 Aug 2024 01:48:36 +0000
From: "Li, Hua Qian" <HuaQian.Li@...mens.com>
To: "conor@...nel.org" <conor@...nel.org>, "Kiszka, Jan"
<jan.kiszka@...mens.com>
CC: "Zeng, Chao" <chao.zeng@...mens.com>, "linux-iio@...r.kernel.org"
<linux-iio@...r.kernel.org>, "devicetree@...r.kernel.org"
<devicetree@...r.kernel.org>, "robh@...nel.org" <robh@...nel.org>, "Su, Bao
Cheng" <baocheng.su@...mens.com>, "krzk+dt@...nel.org" <krzk+dt@...nel.org>,
"jic23@...nel.org" <jic23@...nel.org>, "conor+dt@...nel.org"
<conor+dt@...nel.org>, "linux-kernel@...r.kernel.org"
<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 2/3] dt-bindings: iio: Add everlight pm16d17 binding
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@...mens.com>
> >
> > Add the binding document for the everlight pm16d17 sensor.
> >
> > Signed-off-by: Chao Zeng <chao.zeng@...mens.com>
> > Co-developed-by: Baocheng Su <baocheng.su@...mens.com>
> > Signed-off-by: Baocheng Su <baocheng.su@...mens.com>
>
> Ditto here Jan.
>
> > ---
> > .../iio/proximity/everlight,pm16d17.yaml | 95
> > +++++++++++++++++++
> > 1 file changed, 95 insertions(+)
> > create mode 100644
> > Documentation/devicetree/bindings/iio/proximity/everlight,pm16d17.y
> > aml
> >
> > 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@...mens.com>
> > +
> > +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
> > +
> > + 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.
>
> > + 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.
>
> > + 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?
>
> > + 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.
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-ms
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
- ...
Documentation/devicetree/bindings/iio/proximity/vishay,vcnl3020.yaml
- vishay,led-current-microamp
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
www.siemens.com
Powered by blists - more mailing lists