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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxx>

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;
+	}
+}
+
[...]



[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Input]     [Linux Kernel]     [Linux SCSI]     [X.org]

  Powered by Linux