On Fri, 23 Dec 2022 17:10:51 +0100 Marco Felsch <m.felsch@xxxxxxxxxxxxxx> wrote: > On 22-12-23, Jonathan Cameron wrote: > > On Fri, 23 Dec 2022 16:03:38 +0100 > > Marco Felsch <m.felsch@xxxxxxxxxxxxxx> wrote: > > > > > On 22-12-23, Jonathan Cameron wrote: > > > > On Wed, 21 Dec 2022 10:27:59 +0100 > > > > Marco Felsch <m.felsch@xxxxxxxxxxxxxx> wrote: > > > > > > > > > The TMP116 is the predecessor of the TMP117. > > > > > > > > > > Signed-off-by: Marco Felsch <m.felsch@xxxxxxxxxxxxxx> > > > > I'm not sure this is introducing a valid fallback. The driver changes > > > > imply some things the tmp117 driver supports, that this device > > > > does not. A fallback compatible would mean that a new DT > > > > with an old kernel would load the tmp117 against a tmp116 and > > > > expect it to fully work. > > > > > > Since driver does all the detection an update of the bindings isn't > > > really necessary. It is just to have a compatible already in place in > > > case there a things we can't detected during runtime. This flow is > > > common for a lot of SoC drivers. The fallback will be used as long as > > > possible and once a specific feature can't be detected only via the > > > binding, the driver adds the new binding to it of_compatible. > > > > That's true going forwards and for drivers that introduce a shared > > generic compatible alongside the initial binding. It can't be easily > > retrofit. > > > > Fallback compatible is also to allow this to work with old kernels > > What this small series does is adding the support for the chip. So the > support starts with the kernel version which includes these patches. Why > do you assume that one expect to have a proper support with an older > kernel? I fully get the point that driver needs to deal with older > device-tree's but having using a newer device-tree's (fw) on older > kernels and expecting that older kernels does support the chip is a bit > odd to me. Probably need the DT maintainers to offer the opinion on this as we disagree on how fallback compatibles are supposed to work. I'll accept whatever they say on this point (I've been persuaded into a more relaxed stance in the past on this). Allowing a new DTS file to work with old kernels is one of the advantages of fallback compatibles. On many devices there is no coupling between the two and there shouldn't need to be one. So in this case the device is not compatible and we should not imply that it is. > > > - which it doesn't because the kernel driver > > a) rejects non tmp117 ids (we should fix that by just warning instead > > if the ID isn't what we expect. It would be good to factor that out > > as a separate patch that we can backport) > > > > b) assumes the tmp116 (after above fixed) supports things it doesn't. > > > > So it's not a valid use of a fallback compatible. > > I added the compatible sring only to have it in place for the future. I > really didn't have to do that at all since the detection can be done > during runtime and the only compatible would be tmp117. So sorry but I > don't get your point here. Fallback compatibles have multiple purposes. One of them, which should work is to allow a kernel that only support tmp117 to work with a DTS that says it is a tmp116 with fallback to tmp117. That's not true here, so there should not be a fallback. > > > A driver can't rely on matching device IDs it didn't previously know > > about. It sees tmp116 compatible and thinks it knows how to handle the > > device, which it doesn't. This might lead to some very grumpy people > > not understanding why their old kernel doesn't work. > > Why do you think that people think that the driver have to support a > device based on the compatible? I saw device-trees with nodes not > matching any kernel driver since the device-tree is just the firmware > descibing the hardware. If I got Krzysztof correctly people have to > check the driver implementation for the according support since the > bindings can be used by other projects as well and are not limited to > the linux-kernel. My understanding is that supporting new compatible parts on old kernels is one of the reasons fall back compatibles exist (as well as the case you mention of not modifying a driver until we hit something that requires it) If that doesn't work, then the driver is broken - which is why I suggest we fix that. However in this case the part is not compatible so the fallback should not be there. You should just add the ID to the driver given you are modifying it to support the part anyway. Jonathan > > Regards, > Marco > > > Jonathan > > > > > > > > Regards, > > > Marco > > > > > > > An example is calibbias which you've dropped from the channels > > > > array entry. > > > > > > > > Jonathan > > > > > > > > > > > > > --- > > > > > v2: > > > > > - drop items from single enum > > > > > > > > > > .../bindings/iio/temperature/ti,tmp117.yaml | 14 ++++++++++---- > > > > > 1 file changed, 10 insertions(+), 4 deletions(-) > > > > > > > > > > diff --git a/Documentation/devicetree/bindings/iio/temperature/ti,tmp117.yaml b/Documentation/devicetree/bindings/iio/temperature/ti,tmp117.yaml > > > > > index 8d1ec4d39b28..9b78357d6a79 100644 > > > > > --- a/Documentation/devicetree/bindings/iio/temperature/ti,tmp117.yaml > > > > > +++ b/Documentation/devicetree/bindings/iio/temperature/ti,tmp117.yaml > > > > > @@ -7,8 +7,9 @@ $schema: "http://devicetree.org/meta-schemas/core.yaml#"; > > > > > title: "TI TMP117 - Digital temperature sensor with integrated NV memory" > > > > > > > > > > description: | > > > > > - TI TMP117 - Digital temperature sensor with integrated NV memory that supports > > > > > - I2C interface. > > > > > + TI TMP116/117 - Digital temperature sensor with integrated NV memory that > > > > > + supports I2C interface. > > > > > + https://www.ti.com/lit/gpn/tmp116 > > > > > https://www.ti.com/lit/gpn/tmp117 > > > > > > > > > > maintainers: > > > > > @@ -16,8 +17,13 @@ maintainers: > > > > > > > > > > properties: > > > > > compatible: > > > > > - enum: > > > > > - - ti,tmp117 > > > > > + oneOf: > > > > > + - enum: > > > > > + - ti,tmp117 > > > > > + - items: > > > > > + - enum: > > > > > + - ti,tmp116 > > > > > + - const: ti,tmp117 > > > > > > > > > > reg: > > > > > maxItems: 1 > > > > > > > > > > > >
