Re: [PATCH 3/3] dt-bindings: pse-pd: ti,tps23881: Add TPS23881B

[Conor Dooley <conor@kernel.org>]

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

On Thu, Oct 09, 2025 at 11:43:04PM +0200, Andrew Lunn wrote:
> > When adapting the driver, I also considered an auto-detection mechanism.
> > However, it felt safer to rely on the devicetree information than reading
> > a silicon revision register, which has a totally different meaning on
> > some other device. I have therefore decided to make the driver behaviour
> > solely dependent on the devicetree information and to use the silicon
> > revision only as a sanity check (as already implemented in the driver).
> 
> So if the silicon and the DT disagree, you get -ENODEV or similar?
> That is what i would recommend, so that broken DT blobs get found by
> the developer.

I'm personally not a big fan of this kind of thing, as it prevents using
fallbacks for new devices when done strictly. I only really like it
being done this way if the driver does not produce errors for unknown
part numbers, only if (using this case as an example) a b device is
labeled as a non-b, or vice-versa. IOW, if the driver doesn't recognise
the ID, believe what's in DT.

> > Is there any best practice when to use auto-detection with I2C devices?
> 
> Not really. There are devices/drivers where the compatible is just
> used to indicate where to find the ID register in the hardware,
> nothing else. The ID register is then used by the driver to do the
> right thing, we trust the silicon to describe itself. But things like
> PHY devices have the ID in a well known location, so we actually don't
> require a compatible, but if one is given, we use that instead of the
> ID found in the silicon. So the exact opposite.
> 
> > Regardless of whether the driver queries the silicon revision, the B
> > device declaration would look somehow strange to me with a driver having
> > one single compatible, i.e. compatible = "ti,tps23881b", "ti,tps23881".
> > The first one specifically names the hardware, the fallback is actually
> > the name of its predecessor, which is strictly speaking not 100%
> > compatible but required to have the driver loaded.
> 
> If it is not compatible, a fallback will not actually work, don't list
> a fallback.

Yeah, seconded. I think my original mail about this was maybe a bit
confusingly worded, where I was envisaging a world where a driver that
encountered a b device could load the firmware for the non-b device, and
it would just be a redundant operation. A fallback would be suitable,
but obviously not ideal then. Since that isn't permitted, using a
fallback here does not make sense.

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