On 3/19/21 9:30 PM, 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 <redacted>

Hi,

Thanks for the patch, this looks really good. I have a couple of small 
comments inline.

And one general comment. What you have implemented as 
IIO_CHAN_INFO_OFFSET should be IIO_CHAN_INFO_CALIBBIAS, since it is an 
offset for calibration. The IIO_CHAN_INFO_OFFSET property on the other 
hand is used to describe how to convert from raw data to SI units.

interrupt.h is for consumers, irq.h is for providers. Since this driver 
is only a interrupt consumer you only need irq.h.

I believe bitops.h and delay.h are also not needed by this driver.

+
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+#include <linux/iio/events.h>
+
+#define TMP117_REG_TEMP			0x0
+#define TMP117_REG_CFGR			0x1
+#define TMP117_REG_HIGH_LIM		0x2
+#define TMP117_REG_LOW_LIM		0x3
+#define TMP117_REG_EEPROM_UL		0x4
+#define TMP117_REG_EEPROM1		0x5
+#define TMP117_REG_EEPROM2		0x6
+#define TMP117_REG_EEPROM3		0x7
+#define TMP117_REG_TEMP_OFFSET		0x7

You got register 0x7 twice here.

+#define TMP117_REG_EEPROM4		0x8
+#define TMP117_REG_DEVICE_ID		0xF
+
+#define TMP117_RESOLUTION		78125       /* in tens of uCelsius*/
+#define TMP117_RESOLUTION_DIV		10000000
+
+#define TMP117_DEVICE_ID		0x0117
+
+struct tmp117_data {
+	struct i2c_client *client;
+	struct mutex lock;
+};
+
+static int tmp117_read_reg(struct tmp117_data *data, u8 reg)
+{
+

Being pedantic: the newline at the top here and also in write_reg are 
not needed.

+	return i2c_smbus_read_word_swapped(data->client, reg);
+}
+
+static int tmp117_write_reg(struct tmp117_data *data, u8 reg, int val)
+{
+
+	return i2c_smbus_write_word_swapped(data->client, reg, val);
+}
+
+static int tmp117_read_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);
+	u16 tmp, off;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_PROCESSED:
+		tmp = tmp117_read_reg(data, TMP117_REG_TEMP);
+		*val = ((int16_t)tmp * (int32_t)TMP117_RESOLUTION) / 10000000;
+		*val2 = ((int16_t)tmp * (int32_t)TMP117_RESOLUTION) % 10000000;

Since this is a linear transform it would be better to report RAW 
measurements and then provide the scale attribute for conversion,.

+		return IIO_VAL_INT_PLUS_MICRO;
+
+	case IIO_CHAN_INFO_OFFSET:
+		off = tmp117_read_reg(data, TMP117_REG_TEMP_OFFSET);
+		*val = ((int16_t)off * (int32_t)TMP117_RESOLUTION) / 10000000;
+		*val2 = ((int16_t)off * (int32_t)TMP117_RESOLUTION) % 10000000;
+		return IIO_VAL_INT_PLUS_MICRO;
+
+	default:
+		return -EINVAL;
+	}
+}
+
+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);
+	u16 off;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_OFFSET:
+		off = ((val * 10000000) + (val2 * 10))
+						/ (int32_t)TMP117_RESOLUTION;
+		return tmp117_write_reg(data, TMP117_REG_TEMP_OFFSET, off);
+
+	default:
+		return -EINVAL;
+	}
+}
+
+static const struct iio_chan_spec tmp117_channels[] = {
+	{
+		.type = IIO_TEMP,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) |
+				BIT(IIO_CHAN_INFO_OFFSET),
+	},
+};
+
+static const struct iio_info tmp117_info = {
+	.read_raw = tmp117_read_raw,
+	.write_raw = tmp117_write_raw,
+};
+
+static bool 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)
+		return false;
+
+	return (dev_id == TMP117_DEVICE_ID);
+}
+
+static int tmp117_probe(struct i2c_client *client,
+			const struct i2c_device_id *tmp117_id)
+{
+	struct tmp117_data *data;
+	struct iio_dev *indio_dev;
+
+	if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_WORD_DATA))
+		return -EOPNOTSUPP;
+
+	if (!tmp117_identify(client)) {
+		dev_err(&client->dev, "TMP117 not found\n");
+		return -ENODEV;
+	}
+
+	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	data = iio_priv(indio_dev);
+	i2c_set_clientdata(client, indio_dev);

If you remove the remove() callback the i2c_set_clientdata() here can 
also be removed.

+	data->client = client;
+	mutex_init(&data->lock);
+
+	indio_dev->dev.parent = &client->dev;

The parent assignment is no longer needed in the latest upstream kernel 
since it happens in devm_iio_device_alloc().

+	indio_dev->name = "tmp117";
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->info = &tmp117_info;
+
+	indio_dev->channels = tmp117_channels;
+	indio_dev->num_channels = ARRAY_SIZE(tmp117_channels);
+
+	return iio_device_register(indio_dev);

devm_iio_device_register(...) and then you can remove the the remove() 
callback.

+}
+
[...]