Message-ID: <20240817144215.1b2e9db3@jic23-huawei>
Date: Sat, 17 Aug 2024 14:42:15 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: "Li, Hua Qian" <HuaQian.Li@...mens.com>
Cc: "conor@...nel.org" <conor@...nel.org>, "Kiszka, Jan"
 <jan.kiszka@...mens.com>, "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>,
 "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 Fri, 16 Aug 2024 01:48:36 +0000
"Li, Hua Qian" <HuaQian.Li@...mens.com> 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@...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.

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.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




> 
> 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
> > >   
> 

Powered by blists - more mailing lists