On Thu, Apr 1, 2021 at 12:19 PM Puranjay Mohan [off-list ref] wrote:

TMP117 is a Digital temperature sensor with integrated NV memory.

Add support for tmp117 driver in iio subsystem.

+ blank line

Datasheet:-https://www.ti.com/lit/gpn/tmp117

Make it a tag, i.e. remove the following blank line and use a space after colon.

Signed-off-by: Puranjay Mohan <redacted>

...

+/*
+ * tmp117.c - Digital temperature sensor with integrated NV memory

It's useless and provokes an unneeded churn when having a file name
inside the file.
Please, drop it for good.

+ *
+ * Copyright (c) 2021 Puranjay Mohan [off-list ref]
+ *
+ * Driver for the Texas Instruments TMP117 Temperature Sensor

+ *

Redundant blank line.

+ * (7-bit I2C slave address (0x48 - 0x4B), changeable via ADD pins)
+ *
+ * Note: This driver assumes that the sensor has been calibrated beforehand.
+ */

...

+#include <linux/err.h>
+#include <linux/i2c.h>
+#include <linux/module.h>

Missed:
  bitops.h //sign_extend32()
  types.h // s32

+
+#include <linux/iio/iio.h>

...

+struct tmp117_data {
+       struct i2c_client *client;
+};

Doesn't make any sense to have a separate structure for just one
pointer member. Use that pointer directly.

...

+       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;

One line

+               *val2 = ((int16_t)ret * (int32_t)TMP117_RESOLUTION_10UC)
+                                                               % 10000;

One line.

I'll be honest, I do not like these explicit castings at all. Can you
revisit and try to refactor that you won't need them?
For example, I can't understand how ret can be higher than 16 bit
since we checked on negative values beforehand.

+               return IIO_VAL_INT_PLUS_MICRO;
+
+       case IIO_CHAN_INFO_SCALE:
+               /* Conversion from 10s of uC to mC
+                * as IIO reports temperature in mC
+                */
+               *val = TMP117_RESOLUTION_10UC / 10000;
+               *val2 = (TMP117_RESOLUTION_10UC % 10000) * 100;
+               return IIO_VAL_INT_PLUS_MICRO;

You use 10000 many times, can you give it an appropriate name (via #define)?

...

+       s16 off;

+       case IIO_CHAN_INFO_CALIBBIAS:

+               off = (s16)val;

Redundant explicit casting.

+               return i2c_smbus_write_word_swapped(data->client,
+                                               TMP117_REG_TEMP_OFFSET, off);

...

+static const struct of_device_id tmp117_of_match[] = {
+       { .compatible = "ti,tmp117", },

+       { },

No need to comma in terminator line(s).

+};

-- 
With Best Regards,
Andy Shevchenko