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;
Subject: Re: [PATCH v2 2/2] iio: temperature: add driver support for ti tmp117
To:     Puranjay Mohan <puranjay12@gmail.com>,
        alexandru.ardelean@analog.com, jic23@kernel.org,
        devicetree@vger.kernel.org, knaack.h@gmx.de,
        linux-iio@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20210401091648.87421-1-puranjay12@gmail.com>
 <20210401091648.87421-3-puranjay12@gmail.com>
From:   Lars-Peter Clausen <lars@metafoo.de>
Message-ID: <7a9097bf-9f8d-0fe7-7b5e-84643bcff760@metafoo.de>
Date:   Fri, 2 Apr 2021 10:13:25 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.9.0
MIME-Version: 1.0
In-Reply-To: <20210401091648.87421-3-puranjay12@gmail.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 7bit
Content-Language: en-US
Precedence: bulk

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

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. 
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).
> +		return i2c_smbus_write_word_swapped(data->client,
> +						TMP117_REG_TEMP_OFFSET, off);
> +
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
[...]