Re: [PATCH 2/2] iio: temperature: tmp117: add support for HIGH and LOW alerts

[Marcelo Schmitt <marcelo.schmitt1@gmail.com>]

On 10/05, Thomas Bonnefille wrote:
> The TMP117/116/119 chips allow the user to set an alert if the
> temperature reaches a HIGH or LOW limit.
> 
> Add support for setting the high and low alert limits.
> An IIO event is generated when the temperature crosses either limit
> thanks to an interrupt on the Alert pin.
> 
> Signed-off-by: Thomas Bonnefille <thomas.bonnefille@bootlin.com>
> ---
...
> +static int tmp117_read_thresh(struct iio_dev *indio_dev,
> +			      const struct iio_chan_spec *chan,
> +			      enum iio_event_type type,
> +			      enum iio_event_direction dir,
> +			      enum iio_event_info info,
> +			      int *val, int *val2)
> +{
> +	struct tmp117_data *data = iio_priv(indio_dev);
> +	s32 ret;
> +	u8 reg;
> +
> +	switch (dir) {
> +	case IIO_EV_DIR_RISING:
> +		reg = TMP117_REG_HIGH_LIM;
> +		break;
> +	case IIO_EV_DIR_FALLING:
> +		reg = TMP117_REG_LOW_LIM;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	if (info != IIO_EV_INFO_VALUE || type != IIO_EV_TYPE_THRESH)
> +		return -EINVAL;
> +	ret = i2c_smbus_read_word_swapped(data->client, reg);
> +	if (ret < 0)
> +		return ret;
> +
> +	/*
> +	 * Report the threshold in millidegrees Celsius as an exact fraction:
> +	 * value = raw * TMP117_LIMIT_LSB_NUM / TMP117_LIMIT_LSB_DEN.
> +	 */
The IIO temperature channel provides single-read data in (raw) device units
(i.e. not scaled to mili degrees Celsius). Now, in contrast with the single-read
interface, the event configuration being proposed reads/writes data in milli
degrees Celsius. And, since the event ABI doesn't yet provide clear indication
about IIO event data unit types, users might get confused when using the
different interfaces.

There are two main options for avoiding/mitigating the potential interface mismatch:
(1) provide/get raw device units in tmp117_read/write_thresh().

(2) add IIO_CHAN_INFO_PROCESSED to the IIO channel mask and implement the
handling for the channel _input interface to provide single-read data in
milli degrees Celsius units. Though, this option will also need an updated
IIO event interface. Probably something like [1] or [2]. I'll respin on that
next week. The event specs would be updated to something like


+static const struct iio_event_spec tmp11x_obj_event[] = {
+	{
+		.type = IIO_EV_TYPE_THRESH,
+		.dir = IIO_EV_DIR_RISING,
+		.unit = IIO_EV_UNIT_PROCESSED,
+		.mask_separate = BIT(IIO_EV_INFO_VALUE),
+	},
+	{
+		.type = IIO_EV_TYPE_THRESH,
+		.dir = IIO_EV_DIR_FALLING,
+		.unit = IIO_EV_UNIT_PROCESSED,
+		.mask_separate = BIT(IIO_EV_INFO_VALUE),
+	},
+};
+


[1]: https://lore.kernel.org/linux-iio/cover.1768759292.git.marcelo.schmitt1@gmail.com/
[2]: https://lore.kernel.org/linux-iio/e359fa003ebc61c8b0e37ba9810a04c70e5a300e.1791437050.git.marcelo.schmitt@analog.com/

With best regards,
Marcelo

> +	*val = sign_extend32(ret, 15) * TMP117_LIMIT_LSB_NUM;
> +	*val2 = TMP117_LIMIT_LSB_DEN;
> +
> +	return IIO_VAL_FRACTIONAL;
> +}
> +
> +static int tmp117_write_thresh(struct iio_dev *indio_dev,
> +			       const struct iio_chan_spec *chan,
> +			       enum iio_event_type type,
> +			       enum iio_event_direction dir,
> +			       enum iio_event_info info,
> +			       int val, int val2)
> +{
> +	struct tmp117_data *data = iio_priv(indio_dev);
> +	s64 tmp;
> +	u8 reg;
> +
> +	switch (dir) {
> +	case IIO_EV_DIR_RISING:
> +		reg = TMP117_REG_HIGH_LIM;
> +		break;
> +	case IIO_EV_DIR_FALLING:
> +		reg = TMP117_REG_LOW_LIM;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	if (info != IIO_EV_INFO_VALUE || type != IIO_EV_TYPE_THRESH)
> +		return -EINVAL;
> +
> +	/*
> +	 * The threshold is passed in millidegrees Celsius as
> +	 * value = val + val2 / 1000000 (val2 holds the fractional part, with
> +	 * the sign taken from val). Convert it back with the same LSB ratio:
> +	 * raw = value / (TMP117_LIMIT_LSB_NUM / TMP117_LIMIT_LSB_DEN).
> +	 */
> +	tmp = (val < 0) ? (s64)val * 1000000 - val2
> +			: (s64)val * 1000000 + val2;
> +	tmp = DIV_ROUND_CLOSEST(tmp * TMP117_LIMIT_LSB_DEN,
> +				(s64)TMP117_LIMIT_LSB_NUM * 1000000);
> +	tmp = clamp_t(s64, tmp, S16_MIN, S16_MAX);
> +
> +	return i2c_smbus_write_word_swapped(data->client,
> +					    reg, tmp);
> +}
> +