On 23/12/2022 18:16, Jonathan Cameron wrote:
> On Fri, 23 Dec 2022 17:13:59 +0100
> Marco Felsch <m.felsch@xxxxxxxxxxxxxx> wrote:
>
>> On 22-12-23, Jonathan Cameron wrote:
>>
>> ...
>>
>>>>>> @@ -118,27 +127,28 @@ static int tmp117_identify(struct i2c_client *client)
>>>>>> int dev_id;
>>>>>>
>>>>>> dev_id = i2c_smbus_read_word_swapped(client, TMP117_REG_DEVICE_ID);
>>>>>> - if (dev_id < 0)
>>>>>
>>>>> Keep this handling of the smbus read returning an error.
>>>>> Otherwise, you end up replacing the error code with -ENODEV rather than
>>>>> returning what actually happened.
>>>>>
>>>>> if (dev_id < 0)
>>>>> return dev_id;
>>>>
>>>> You're right, I will change this thanks.
>>>>
>>>>> switch (dev_id) {
>>>>> ...
>>>>>
>>>>>> + switch (dev_id) {
>>>>>> + case TMP116_DEVICE_ID:
>>>>>> + case TMP117_DEVICE_ID:
>>>>>> return dev_id;
>>>>>> - if (dev_id != TMP117_DEVICE_ID) {
>>>>>> - dev_err(&client->dev, "TMP117 not found\n");
>>>>>> + default:
>>>>>> + dev_err(&client->dev, "TMP116/117 not found\n");
>>>>>> return -ENODEV;
>>>
>>> As per the other branch of this thread. This isn't an error.
>>> If we want fallback compatibles to work in their role of allowing
>>> for newer devices that are actually compatible, the most we should
>>> do here is warn.
>>>
>>> Say a new tmp117b device is released. It's fully backwards compatible
>>> with the exception of an ID - or supports only new features + backwards
>>> compatibility then that would have a fallback to tmp117 and we need
>>> it to work.
>>
>> This isn't part of this patchset and IMHO implementing something which
>> may happen in the future is not the way we should go.
>
> I held a similar view, but the response I got from the DT maintainers was
> that a driver should not reject a DTS that says it is compatible based
> on an unknown ID - because it prevents that case of an old kernel working
> absolutely fine with a completely compatible newer part.
I don't think that there was such generic recommendation. Accepting
unknown devices (unknown register IDs) is a risk - device might behave
correct or not. If it is a critical device, like regulator, misbehave
might damage something.
What's more, how Linux driver behaves on device IDs (not compatibles) is
also a bit outside of DT scope.
If a driver claims it handles compatibles tmp117, then indeed it should
work fine with any DTS node claiming to be compatible with tmp117.
However driver is free to make further checks (if possible) whether the
device (e.g. tmp116 or tmp11X) is really compatible and reject unknown
devices for safety reason.
The same as x86 kernel is fine to reject to work on newest (unknown) x86
processors for safety reasons... which is terrible from user-experience
point of view unless it is real safety case.
Best regards,
Krzysztof
