On Tue, 27 Dec 2022 09:30:08 +0100
Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx> wrote:
> 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.
Agreed - I didn't express that there are limits to such a requirement.
Indeed not a good idea with regulators etc! However, for input devices
like this one things are a little simpler - in theory they could be used
for something that ends up damaging hardware if done wrong, but it's much
less likely.
>
> 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.
Ok. For input devices at least in IIO we went around this a few times and
ended up with deciding that a dev_info() type message was the best balance.
We will need to be more careful for output devices.
>
> 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.
Hopefully that never happens :)
Jonathan
>
> Best regards,
> Krzysztof
>
