Re: [PATCH v2 1/2] dt-bindings: iio: light: add as7343

[Jonathan Cameron <jic23@kernel.org>]

On Fri, 18 Sep 2026 18:55:00 -0700
Chang Yu <marcus.yu.56@gmail.com> wrote:

> On Thu, Sep 17, 2026 at 03:48:23AM +0100, Jonathan Cameron wrote:
> > On Tue, 15 Sep 2026 18:11:17 +0100
> > Conor Dooley <conor@kernel.org> wrote:
> >   
> > > On Sun, Sep 13, 2026 at 01:42:03AM +0100, Jonathan Cameron wrote:  
> > > > On Tue, 8 Sep 2026 19:13:55 +0100
> > > > Conor Dooley <conor@kernel.org> wrote:
> > > >     
> > > > > On Mon, Sep 07, 2026 at 02:00:41PM -0700, Chang Yu wrote:    
> > > > > > Add binding for AMS AS7343 which is a 14-channel multi-spectral sensor
> > > > > > with i2c address of 0x39.
> > > > > > 
> > > > > > The GPIO pin is described as a generic GPIO for now. Binding design for the
> > > > > > more advanced measurement/LED synchronization use cases are deferred to
> > > > > > future patches.      
> > > > > 
> > > > > Unfortunately, you can't change what you document, so picking something
> > > > > correct now is needed - even if the driver doesn't use it yet.    
> > > > 
> > > > Definitely needs an outline of how it would be backwards compatible and
> > > > an explanation of why not now.  Sometimes a portion of the binding is
> > > > so uncertain that we do kick it back from initial version but we 'must'
> > > > be sure we can extend the binding to new configurations.  Normally this
> > > > is one of those we are fairly sure, but not entirely sure cases - or
> > > > picking between two options where consensus isn't being reached.
> > > > 
> > > > Chang Yu: This sort of things needs discussion and is one of the reasons to
> > > > go slowly.    
> > > 
> > > Ye, I note that there are 2 more versions of this since I left this
> > > comment, but you seem to be on top of that.
> > >   
> > > > > > Datasheet: https://look.ams-osram.com/m/5f2d27fff9a874d2/original/AS7343-14-Channel-Multi-Spectral-Sensor.pdf
> > > > > > Signed-off-by: Chang Yu <marcus.yu.56@gmail.com>
> > > > > > ---
> > > > > > Changes in v2:
> > > > > >   - Add the LDR, the interrupt pin, and the GPIO pin to the bindings.
> > > > > >   - Fix node name and unit address mismatch.
> > > > > >   - Include MAINTAINERS changes.
> > > > > > 
> > > > > >  .../bindings/iio/light/ams,as7343.yaml        | 69 +++++++++++++++++++
> > > > > >  MAINTAINERS                                   |  6 ++
> > > > > >  2 files changed, 75 insertions(+)
> > > > > >  create mode 100644 Documentation/devicetree/bindings/iio/light/ams,as7343.yaml
> > > > > > 
> > > > > > diff --git a/Documentation/devicetree/bindings/iio/light/ams,as7343.yaml b/Documentation/devicetree/bindings/iio/light/ams,as7343.yaml
> > > > > > new file mode 100644
> > > > > > index 000000000000..b06d445b92b3
> > > > > > --- /dev/null
> > > > > > +++ b/Documentation/devicetree/bindings/iio/light/ams,as7343.yaml
> > > > > > @@ -0,0 +1,69 @@
> > > > > > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> > > > > > +%YAML 1.2
> > > > > > +---
> > > > > > +$id: http://devicetree.org/schemas/iio/light/ams,as7343.yaml#
> > > > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > > > +
> > > > > > +title: AMS AS7343 14-Channel Multi-Spectral Sensor
> > > > > > +
> > > > > > +maintainers:
> > > > > > +  - Chang Yu <marcus.yu.56@gmail.com>
> > > > > > +
> > > > > > +description: |
> > > > > > +  The AMS AS7343 is a 14-channel multi-spectral sensor with i2c address of 0x39.
> > > > > > +  https://look.ams-osram.com/m/5f2d27fff9a874d2/original/AS7343-14-Channel-Multi-Spectral-Sensor.pdf
> > > > > > +
> > > > > > +properties:
> > > > > > +  compatible:
> > > > > > +    enum:
> > > > > > +      - ams,as7343
> > > > > > +
> > > > > > +  reg:
> > > > > > +    description:
> > > > > > +      I2C address of the device (0x39).
> > > > > > +    maxItems: 1
> > > > > > +
> > > > > > +  interrupts:
> > > > > > +    description:
> > > > > > +      Open drain output active low interrupt pin.
> > > > > > +    maxItems: 1
> > > > > > +
> > > > > > +  vdd-supply: true
> > > > > > +
> > > > > > +  ams,led-current-microamp:
> > > > > > +    description:
> > > > > > +      The driver current for the external LED connected to the LDR pin.
> > > > > > +    minimum: 4000
> > > > > > +    maximum: 258000
> > > > > > +    multipleOf: 2000
> > > > > > +    default: 12000      
> > > > > 
> > > > > Rather than a custom property, the tsl2772 uses led-max-microamp:
> > > > > tsl2772.yaml
> > > > > 46:  led-max-microamp:
> > > > > 81:                led-max-microamp = <100000>;
> > > > > 
> > > > > I wonder if the same should be done here, or if there should be an leds
> > > > > subnode? Perhaps the IIO folks can comment on that.    
> > > > 
> > > > I don't think we've ever bothered with a subnode as there only tends
> > > > to be one of them.  Given the enabling etc is all hardware controlled
> > > > I'm not sure a more generic LED binding makes sense. I don't know that
> > > > much about the led bindings though so maybe it is worth doing a subnode
> > > > just to use the leds/common.yaml definition of led-max-microamp?    
> > > 
> > > Could always just put a ref in to leds/common.yaml and not bother with a
> > > child node. I'd stick with the additionalProperties: false, since
> > > there's lots of leds properties that probably don't apply to a device
> > > like this.
> > >   
> > Sounds good to me.
> >   
> > > Cheers,
> > > Conor.  
> >   
> A quick heads up, adding a ref produces the following error when running
> dt_binding_check:
> 
> properties:led-max-microamp: '$ref' should not be valid under {'const': '$ref'}
> 	hint: Standard unit suffix properties don't need a type $ref
> 	from schema $id: http://devicetree.org/meta-schemas/core.yaml
> 
> So it looks like we can just put led-max-microamp, and not add the $ref.

Ah. That's a bit irritating as it is assuming the ref is to get the
type rather than to make the connection to the standard definition
of what it is.  Conor, what is the best way around this? 
Maybe a comment to say where it is documented instead of a $ref?

J