Re: [PATCH 1/2] dt-bindings: iio: light: Add AMS TCS3472 and TCS3400

[Conor Dooley <conor@kernel.org>]

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

On Sat, Oct 17, 2026 at 01:36:00AM +0100, Jonathan Cameron wrote:
> On Fri, 16 Oct 2026 16:43:42 +0200
> Krzysztof Kozlowski <krzk@kernel.org> wrote:
> 
> > On 16/10/2026 10:15, HuskerMinion via B4 Relay wrote:
> > > From: HuskerMinion <mocker-mug-unwoven@duck.com>
> > > 
> > > The tcs3472 driver has no binding yet. It matches DT nodes only through
> > > its I2C device ID.
> > > 
> > > Document the TCS3472 family and the AMS TCS3400 family, which the next
> > > patch adds to the driver. Both are I2C color sensors with one threshold
> > > interrupt and a VDD supply.
> > > 
> > > Signed-off-by: HuskerMinion <mocker-mug-unwoven@duck.com>  
> > 
> > Please read submitting patches how to properly attribute the work with
> > your real and known identity
> > 
> > 
> > > ---
> > >  .../devicetree/bindings/iio/light/ams,tcs3472.yaml | 57 ++++++++++++++++++++++
> > >  1 file changed, 57 insertions(+)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/iio/light/ams,tcs3472.yaml b/Documentation/devicetree/bindings/iio/light/ams,tcs3472.yaml
> > > new file mode 100644
> > > index 000000000..cd4a0516e
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/iio/light/ams,tcs3472.yaml
> > > @@ -0,0 +1,57 @@
> > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/iio/light/ams,tcs3472.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: AMS TCS3472 and TCS3400 color light-to-digital converters
> > > +
> > > +maintainers:
> > > +  - HuskerMinion <mocker-mug-unwoven@duck.com>
> > > +
> > > +description: |
> > > +  Color light sensors with 16-bit clear, red, green and blue channels, an
> > > +  I2C interface and a threshold interrupt.
> > > +
> > > +  The TCS3472 family covers the TCS34721, TCS34723, TCS34725 and TCS34727.
> > > +  The TCS3400 family covers the TCS34001, TCS34003, TCS34005 and TCS34007.  
> > 
> > We do not take families, but actual devices. TCS3400 seems to be a real
> > device, so I don't get why you call it a family.
> > 
> > 
> > > +  The variants differ in I2C address and interface voltage.
> > > +
> > > +properties:
> > > +  compatible:
> > > +    enum:
> > > +      - ams,tcs3400
> > > +      - ams,tcs3472  
> > 
> > Your driver cod suggests these are compatible, so it should be expressed.
> 
> That's only true because this driver hard rejects a whoami mismatch
> which is something we shouldn't really be doing. If we do a look
> up in driver and fail to match we should be using the data associated with
> the ID that the fallback compatible would have had.

Agreed. At most, the driver can emit at info level that the unknown ID is
being treated as xyz device instead.

> Our old fun of what should we do about fallback compatibles and
> devices with ID registers.  Some of which (in this driver) are actually
> compatible with each other and some of which are not (in the sense
> that the match the WHOAMI and do different things depending on that
> matching).
> 
> I still prefer that we never claim devices that need different
> handling are compatible (even if detectable) because of that prevents
> new drop in parts being used (as they normally have different whoami
> values even as supposedly drop in replacements).

I think I've said this a few times, but I have no recollection if I said
it to you or elsewhere, but IMO how detection works is as follows:

If you can detect capabilities at runtime, then having some sort of
common compatible is fine, because you can figure out how to handle the
device based on what the capability discovery interface reports and this
works fine for new devices. Ideally the common compatible is a fallback in
case some issue with the integration crops up or to constrain things like
input clocks etc. The former is more of a problem for things like i2c
controller IPs and the latter probably isn't super relevant either to
different devices in a family of ADCs that likely all share the same
package. If there's some new bit, you have to change the driver to deal
with that feature, but you'd have to do that for any new feature no
matter the scheme used, so that's not a big deal. The designware SPI
controller works like this, for example. I don't know if there are any
IIO devices supported that have something like this.

On the other hand, detecting the device ID at runtime is no better in any
way than having a specific compatible. You effectively end up with match
data gated on a whoami value, which is basically the same thing as
gating it on a specific compatible except for how this blows up in your
face when someone comes along with drop-in replacements (which maybe are
made by another company) that, as you point out, can have different whoami
values meaning you need to modify the driver for every new device
despite having the ability to detect what device is being used.

Devices being declared compatible that need different handling, because
you can use whoami values to account for the behavioural differences I
would advise avoiding since it's equally easy to use match data to get
the same result and it's immediately obvious when looking at a
potentially 1000s of line driver spread across multiple files that
things are different when you can jump to the of/spi/i2c device id
array and see what the differences between devices actually are.

IOW, I am recommending removing decision making based on whoami values,
removing warnings about whoami mismatches, adding specific compatibles
for all devices to the bindings, using fallbacks to devices with
compatible programming models where applying identical handling to both
devices in the driver is functional and adding match data in place of
whoami-based decision making in drivers. The end result of that being
that your drop in replacement devices, be that from a different vendor
or the same one after a product refresh/manufacturing change etc can
work without any changes being needed to the code.

Personally, I think it's okay for an 8 channel device to fall back to a
4 channel one, if the extra 4 channels won't work since technically the
fallback is an exact subset of the 8 channel device - but I do
understand that people don't want to do that because 4 channels not
working is not really the same thing as a device where using the
fallback means a reduction in performance, e.g. using a fallback on an
ethernet controller that allows networking in U-Boot to function without
driver changes, but at a reduced bandwidth compared to using some new
feature only available in the new device. So I tend to ack stuff where
people say that an 8 channel and a 4 channel device aren't compatible,
even though technically a driver written for the 4 channel device would
work perfectly fine on the first 4 channels of the 8 channel device.

Hope that helps?

Conor.

> I don't mind enough to argue much about it however.
> Note that before this series all the supported parts were actually
> compatible and the only difference was a print on the id match. So
> if this patch hadn't turned up an I'd noticed, I have simply
> deleted the error return.

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