On Thu, Apr 1, 2021 at 12:19 PM Puranjay Mohan <puranjay12@xxxxxxxxx> 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 <puranjay12@xxxxxxxxx> ... > +/* > + * 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 <puranjay12@xxxxxxxxx> > + * > + * 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
