Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210401091648.87421-1-puranjay12@gmail.com> <20210401091648.87421-3-puranjay12@gmail.com>
 <7a9097bf-9f8d-0fe7-7b5e-84643bcff760@metafoo.de>
In-Reply-To: <7a9097bf-9f8d-0fe7-7b5e-84643bcff760@metafoo.de>
From:   Puranjay Mohan <puranjay12@gmail.com>
Date:   Sat, 3 Apr 2021 20:28:25 +0530
Message-ID: <CANk7y0gwSZgxzmxtOZayyy_TrUP+rbjtN4WXZAsZE7E46UmUDg@mail.gmail.com>
Subject: Re: [PATCH v2 2/2] iio: temperature: add driver support for ti tmp117
To:     Lars-Peter Clausen <lars@metafoo.de>
Cc:     alexandru.ardelean@analog.com, Jonathan Cameron <jic23@kernel.org>,
        devicetree@vger.kernel.org, knaack.h@gmx.de,
        linux-iio <linux-iio@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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?

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