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

[Chang Yu <marcus.yu.56@gmail.com>]

On Sun, Sep 20, 2026 at 03:14:30PM +0200, Conor wrote:
> 
> 
> On 20 September 2026 02:04:48 GMT+02:00, Jonathan Cameron <jic23@kernel.org> wrote:
> >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?
> 
> From the error, it looks like this was done incorrectly, but impossible to tell for sure without the snippet.
> The ref I was talking about would be at the  top level
> 
> >
> >J
> >
I was doing something like:
    led-max-microamp:
      $ref: /schemas/leds/common.yaml#/properties/led-max-microamp
      minimum: 4000
which gave me the error.

IIUC, Conor was suggesting something like:
    allOf:
      - $ref: /schemas/leds/common.yaml#

From my limited understanding of dt bindings, the above should not be
used together with additionalProperties: false ? So do we want something
like:

    allOf:
      - $ref: /schemas/leds/common.yaml#

    unevaluatedProperties: false

Or just stick with what's in v5?