Re: [PATCH v2 3/4] iio: temperature: tmp117: add TI TMP116 support

[Jonathan Cameron <jic23@kernel.org>]

On Tue, 27 Dec 2022 09:30:08 +0100
Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:

> On 23/12/2022 18:16, Jonathan Cameron wrote:
> > On Fri, 23 Dec 2022 17:13:59 +0100
> > Marco Felsch <m.felsch@pengutronix.de> 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
>