Messages in this thread | | | Subject | Re: [PATCH v2 2/2] iio: temperature: add driver support for ti tmp117 | From | Lars-Peter Clausen <> | Date | Sat, 3 Apr 2021 17:03:17 +0200 |
| |
On 4/3/21 4:58 PM, Puranjay Mohan wrote: > On Fri, Apr 2, 2021 at 1:43 PM Lars-Peter Clausen <lars@metafoo.de> 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@gmail.com> >> 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? Ah, I get it, it is a tenth micro degree, not tens of micro degrees, sorry got confused. > [...] > >> 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. Yea, I think reading and writing in different units would be a bit confusing. >> 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. Casting will truncate the upper bits. So something like 0x12345 gets turned into 0x2345. > >>> + return i2c_smbus_write_word_swapped(data->client, >>> + TMP117_REG_TEMP_OFFSET, off); >>> + >>> + default: >>> + return -EINVAL; >>> + } >>> +} >>> + >> [...] > >
| |