On Fri, Apr 2, 2021 at 1:43 PM Lars-Peter Clausen <lars@xxxxxxxxxx> wrote: > > 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? it is 7.8125 milli degrees_C so 78125 x 10^-4 milli degrees_C which is 78125 x 10^-4 x 10^3 micro degrees_C so it becomes 78125 x 10^-1 micro degrees_C = 78125 10_microdegrees_C. Did it in detail so I remember it in the future. I guess you thought it as 0.78125 millidegrees_C? > > +#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. > yes, reading it will return the value in milli degrees_C. but to write it should be in the register format according to the datasheet. > 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. Okay, if you feel that would be right then I will do it. > 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). the maximum value which this register takes is 0xffff, so it should get clamped automatically when casting to s16? I might be wrong here. > > + return i2c_smbus_write_word_swapped(data->client, > > + TMP117_REG_TEMP_OFFSET, off); > > + > > + default: > > + return -EINVAL; > > + } > > +} > > + > [...] -- Thanks and Regards Yours Truly, Puranjay Mohan
