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;
Date:   Sun, 11 Apr 2021 15:54:20 +0100
From:   Jonathan Cameron <jic23@kernel.org>
To:     Puranjay Mohan <puranjay12@gmail.com>
Cc:     alexandru.ardelean@analog.com, devicetree@vger.kernel.org,
        knaack.h@gmx.de, linux-iio@vger.kernel.org,
        linux-kernel@vger.kernel.org, lars@metafoo.de,
        andy.shevchenko@gmail.com
Subject: Re: [PATCH v4 2/2] iio: temperature: add driver support for ti
 tmp117
Message-ID: <20210411155420.318e866e@jic23-huawei>
In-Reply-To: <20210407182147.77221-3-puranjay12@gmail.com>
References: <20210407182147.77221-1-puranjay12@gmail.com>
        <20210407182147.77221-3-puranjay12@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Wed,  7 Apr 2021 23:51:47 +0530
Puranjay Mohan <puranjay12@gmail.com> wrote:

> TMP117 is a Digital temperature sensor with integrated Non-Volatile 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>
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

A few really small things in here that I tidied up whilst applying and
running local build tests.

Note that as IIO is effectively closed for the coming merge window,
I'm queuing this up for the next cycle and it will be in linux-next
after the merge window closes in about 3 weeks time.

Thanks,

Jonathan


> +static int tmp117_read_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);
> +	s32 ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		ret = i2c_smbus_read_word_swapped(data->client,
> +						TMP117_REG_TEMP);
> +		if (ret < 0)
> +			return ret;
> +		*val = sign_extend32(ret, 15);
> +		return IIO_VAL_INT;
> +
> +	case IIO_CHAN_INFO_CALIBBIAS:
> +		ret = i2c_smbus_read_word_swapped(data->client,
> +					TMP117_REG_TEMP_OFFSET);
> +		if (ret < 0)
> +			return ret;
> +		*val = sign_extend32(ret, 15);
> +		return IIO_VAL_INT;
> +
> +	case IIO_CHAN_INFO_SCALE:
> +		/* Conversion from 10s of uC to mC
Totally trivial so I'll fix it whilst applying, but comment convention used
in IIO is

/*
 * Conversion

> +		 * as IIO reports temperature in mC
> +		 */
> +		*val = TMP117_RESOLUTION_10UC / MICRODEGREE_PER_10MILLIDEGREE;
> +		*val2 = (TMP117_RESOLUTION_10UC %
> +					MICRODEGREE_PER_10MILLIDEGREE) * 100;
> +
> +		return IIO_VAL_INT_PLUS_MICRO;
> +
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +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 = clamp(val, S16_MIN, S16_MAX);

With a C=1 W=1 build (sparse an lots of warnings) this causes problems because
the S16_MIN and S16_MAX are as you might imagine s16 values whereas val is
an int.  I've added casts to force S16_MIN and S16_MAX to ints as well.

> +		if (off == data->calibbias)
> +			return 0;
> +		data->calibbias = off;
> +		return i2c_smbus_write_word_swapped(data->client,
> +						TMP117_REG_TEMP_OFFSET, off);
> +
> +	default:
> +		return -EINVAL;
> +	}
> +}
...