Re: [PATCH v4 1/2] dt-bindings: iio: light: Add APDS9160 binding

[Mikael Gonella-Bolduc <mgonellabolduc@dimonoff.com>]

On Sat, Jan 18, 2025 at 11:48:46AM +0000, Jonathan Cameron wrote:
> On Mon, 13 Jan 2025 09:07:07 -0500
> Mikael Gonella-Bolduc <mgonellabolduc@dimonoff.com> wrote:
> 
> > On Sun, Jan 12, 2025 at 11:10:59AM +0000, Jonathan Cameron wrote:
> > > On Mon, 06 Jan 2025 17:23:01 -0500
> > > Mikael Gonella-Bolduc via B4 Relay <devnull+mgonellabolduc.dimonoff.com@kernel.org> wrote:
> > >   
> > > > From: Mikael Gonella-Bolduc <mgonellabolduc@dimonoff.com>
> > > > 
> > > > Add device tree bindings for APDS9160
> > > > Note: Using alternate email for maintainer
> > > > 
> > > > Signed-off-by: Mikael Gonella-Bolduc <mgonellabolduc@dimonoff.com>
> > > > ---
> > > >  .../bindings/iio/light/brcm,apds9160.yaml          | 86 ++++++++++++++++++++++
> > > >  1 file changed, 86 insertions(+)
> > > > 
> > > > diff --git a/Documentation/devicetree/bindings/iio/light/brcm,apds9160.yaml b/Documentation/devicetree/bindings/iio/light/brcm,apds9160.yaml
> > > > new file mode 100644
> > > > index 0000000000000000000000000000000000000000..756d46c2edb171da840ee49a7339cb781fe84ad2
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/iio/light/brcm,apds9160.yaml
> > > > @@ -0,0 +1,86 @@
> > > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > > > +%YAML 1.2
> > > > +---
> > > > +$id: http://devicetree.org/schemas/iio/light/brcm,apds9160.yaml#
> > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > +
> > > > +title: Broadcom Combined Proximity & Ambient light sensor
> > > > +
> > > > +maintainers:
> > > > +  - Mikael Gonella-Bolduc <m.gonella.bolduc@gmail.com>
> > > > +
> > > > +description: |
> > > > +  Datasheet: https://docs.broadcom.com/docs/APDS-9160-003-DS
> > > > +
> > > > +properties:
> > > > +  compatible:
> > > > +    enum:
> > > > +      - brcm,apds9160
> > > > +
> > > > +  reg:
> > > > +    maxItems: 1
> > > > +
> > > > +  interrupts:
> > > > +    maxItems: 1
> > > > +
> > > > +  vdd-supply: true
> > > > +
> > > > +  ps-cancellation-duration:
> > > > +    $ref: /schemas/types.yaml#/definitions/uint32
> > > > +    description: |
> > > > +      Proximity sensor cancellation pulse duration in half clock cycles.
> > > > +      This parameter determines a cancellation pulse duration.
> > > > +      The cancellation is applied in the integration phase to cancel out
> > > > +      unwanted reflected light from very near objects such as tempered glass
> > > > +      in front of the sensor.
> > > > +    minimum: 0
> > > > +    maximum: 63
> > > > +    default: 0
> > > > +
> > > > +  ps-cancellation-current-coarse:  
> > > 
> > > I've lost track on what we've discussed previously but I'm curious as to whether
> > > we can end up with a cleaner binding for this.  We may well see other identical
> > > controls in future, so nice to have something more 'generic'.  I'm not suggesting
> > > we don't keep it vendor specific though as not sure it will generalize beyond
> > > different broadcomm parts.
> > > 
> > > It is a multiple of nA, so can we just express the combination of
> > > this and ps-cancellation-current-fine as a single parameter, probably in pA
> > > 
> > > The tricky bit being there seem to be holes, so the allowed list would be complex.
> > > 
> > > Even if we can't do that can we express it as two nA values rather than indexes?  
> > 
> > Hi Jonathan,
> > 
> > These holes just have me puzzled on what to do. There's many of them, and the range in value is very large.
> > I thought about just having a single more generic parameter in pA but like you said but I found it was confusing to 
> > describe the allowed values and confusing as well to just round up or down since the holes are so large.
> > 
> > Having two values as a multiplier is more straightfoward for this chip since it's just based on what's described in the datasheet.
> 
> I would like them in common standard units even if we go this way.
> 
> > 
> > If you prefer to keep a more generic parameter, I'm open to the idea of going back to just a single one in pA and
> > log a warning in the driver if the value provided ends up in a hole and round to the nearest allowed value.
> 
> That makes sense to me as the cleanest option.
> Just rely on descriptive text to tell writer of DT binding what values are allowed.
> 
> > 
> > Are you confortable with this plan?
> > 
> > If so, there's another problem with the above. I don't see any picoamp suffix in the dt bindings property-units.yaml.
> > How should I handle this?
> 
> Add it.  They tend to get added on first requirement.  Here
> nothing above picoamp works for us.
> 
> Jonathan.
> 
> > 
> > Best regards,
> > Mikael
> > 
> > 
> > 
> 

Hi Jonathan,

Changes implemented in v5, please take a look.
I also opened a PR for the dt-schema property units change.

Best regards,
Mikael