Message-ID: <18528634e39eec1307777a0fed5eda7324e78575.camel@siemens.com>
Date: Mon, 26 Aug 2024 07:12:53 +0000
From: "Li, Hua Qian" <HuaQian.Li@...mens.com>
To: "jic23@...nel.org" <jic23@...nel.org>
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>,
	"conor@...nel.org" <conor@...nel.org>, "Su, Bao Cheng"
	<baocheng.su@...mens.com>, "krzk+dt@...nel.org" <krzk+dt@...nel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, "Kiszka, Jan"
	<jan.kiszka@...mens.com>, "conor+dt@...nel.org" <conor+dt@...nel.org>, "Lopes
 Ivo, Diogo Miguel" <diogo.ivo@...mens.com>
Subject: Re: [PATCH 2/3] dt-bindings: iio: Add everlight pm16d17 binding

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@...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,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@...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
> > > > +
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/

Powered by blists - more mailing lists