Re: [PATCH v2 2/2] iio: temperature: add driver support for ti tmp117

[Lars-Peter Clausen <lars@metafoo.de>]

On 4/1/21 11:16 AM, Puranjay Mohan wrote:
> TMP117 is a Digital temperature sensor with integrated NV memory.
>
> Add support for tmp117 driver in iio subsystem.
> Datasheet:-https://www.ti.com/lit/gpn/tmp117
>
> Signed-off-by: Puranjay Mohan <puranjay12@gmail.com>

Nice and clean driver. Just some comments about the CALIBBIAS.

> [...]
> +#define TMP117_RESOLUTION_10UC		78125
Isn't the unit here 100 uC?
> +#define TMP117_DEVICE_ID		0x0117
> +
> +struct tmp117_data {
> +	struct i2c_client *client;
> +};
> +
> +static int tmp117_read_raw(struct iio_dev *indio_dev,
> +		struct iio_chan_spec const *channel, int *val,
> +		int *val2, long mask)
> +{
> [...]
> +	case IIO_CHAN_INFO_CALIBBIAS:
> +		ret = i2c_smbus_read_word_swapped(data->client,
> +					TMP117_REG_TEMP_OFFSET);
> +		if (ret < 0)
> +			return ret;
> +		*val = ((int16_t)ret * (int32_t)TMP117_RESOLUTION_10UC)
> +								/ 10000;
> +		*val2 = ((int16_t)ret * (int32_t)TMP117_RESOLUTION_10UC)
> +								% 10000;

If I understand this right CALBBIAS is written in one unit, but read in 
another unit. E.g. if you do `echo 100 > ..._calibbias` and then `cat 
..._calibbias` you'd read a different value back.

I think that would be quite unexpected behavior. The unit should be the 
same. Here in the read function you can just return the register value. 
Just make sure to properly sign extend like for the RAW property.

> +		return IIO_VAL_INT_PLUS_MICRO;
> [...]
> +}
> +
> +static int tmp117_write_raw(struct iio_dev *indio_dev,
> +		struct iio_chan_spec const *channel, int val,
> +		int val2, long mask)
> +{
> +	struct tmp117_data *data = iio_priv(indio_dev);
> +	s16 off;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_CALIBBIAS:
> +		off = (s16)val;
This should have some input validation to avoid writing bogus values to 
the register when the value is out of range. You can either reject out 
of range values or clamp them into the valid range (using the clamp() 
macro).
> +		return i2c_smbus_write_word_swapped(data->client,
> +						TMP117_REG_TEMP_OFFSET, off);
> +
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
[...]