Re: [PATCH 2/3] dt-bindings: iio: Add everlight pm16d17 binding

[Conor Dooley <conor@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 6892 bytes --]

On Fri, Aug 16, 2024 at 01:48:36AM +0000, Li, Hua Qian 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@siemens.com>
> > > 
> > > Add the binding document for the everlight pm16d17 sensor.
> > > 
> > > Signed-off-by: Chao Zeng <chao.zeng@siemens.com>
> > > Co-developed-by: Baocheng Su <baocheng.su@siemens.com>
> > > Signed-off-by: Baocheng Su <baocheng.su@siemens.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@siemens.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.

"Other people did it" is not sufficient justification, you need to
independently justify the properties you add. It appears however, that
Jonathan (who understands these devices better than I, and had a
functioning datasheet link), is also of the opinion that these would be
better suited as userspace controls or require improved explanations.
I suggest you read and reply to his mail in an itemised manner.

Cheers,
Conor.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]