linux-kernel - Re: [PATCH] iio: humidity: hdc3020: add threshold events support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <236dc9db-8525-413e-a77b-54df198c86f5@wanadoo.fr>
Date: Sat, 3 Feb 2024 10:58:24 +0100
From: Christophe JAILLET <christophe.jaillet@...adoo.fr>
To: dima.fedrau@...il.com
Cc: 579lpy@...il.com, javier.carrasco.cruz@...il.com, jic23@...nel.org,
 lars@...afoo.de, linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] iio: humidity: hdc3020: add threshold events support

Le 03/02/2024 à 10:05, Dimitri Fedrau a écrit :
> Add threshold events support for temperature and relative humidity. To
> enable them the higher and lower threshold registers must be programmed
> and the higher threshold must be greater then or equal to the lower
> threshold. Otherwise the event is disabled. Invalid hysteresis values
> are ignored by the device. There is no further configuration possible.
> 
> Tested by setting thresholds/hysteresis and turning the heater on/off.
> Used iio_event_monitor in tools/iio to catch events while constantly
> displaying temperature and humidity values.
> Threshold and hysteresis values are cached in the driver, used i2c-tools
> to read the threshold and hysteresis values from the device and make
> sure cached values are consistent to values written to the device.
> 
> Based on Fix:
> a69eeaad093d "iio: humidity: hdc3020: fix temperature offset" in branch
> fixes-togreg
> 
> Signed-off-by: Dimitri Fedrau <dima.fedrau-Re5JQEeQqe8AvxtiuMwx3w@...lic.gmane.org>
> ---
>   drivers/iio/humidity/hdc3020.c | 339 +++++++++++++++++++++++++++++++++
>   1 file changed, 339 insertions(+)
> 
> diff --git a/drivers/iio/humidity/hdc3020.c b/drivers/iio/humidity/hdc3020.c
> index ed70415512f6..1cdff7af4ca8 100644
> --- a/drivers/iio/humidity/hdc3020.c
> +++ b/drivers/iio/humidity/hdc3020.c
> @@ -16,16 +16,27 @@
>   #include <linux/init.h>
>   #include <linux/module.h>
>   #include <linux/mutex.h>
> +#include <linux/interrupt.h>

Nit: alphabetical order could be kept

>   
>   #include <asm/unaligned.h>
>   
>   #include <linux/iio/iio.h>
> +#include <linux/iio/events.h>

Nit: same

..

>   
> +static const u8 HDC3020_R_T_RH_THRESH_LOW[2] = { 0xE1, 0x02 };
> +static const u8 HDC3020_R_R_RH_THRESH_LOW_CLR[2] = { 0xE1, 0x09 };

I don't know what the R and T are for, but shoukld this be 
HDC3020_R_T_RH_THRESH_LOW_CLR to match other adjacent line?

> +static const u8 HDC3020_R_T_RH_THRESH_HIGH_CLR[2] = { 0xE1, 0x14 };
> +static const u8 HDC3020_R_T_RH_THRESH_HIGH[2] = { 0xE1, 0x1F };

..

> +static int hdc3020_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 hdc3020_data *data = iio_priv(indio_dev);
> +	u16 *thresh;
> +	u8 buf[5];
> +	int ret;
> +
> +	/* Supported temperature range is from –40 to 125 degree celsius */
> +	if (val < -45 || val > 125)
> +		return -EINVAL;
> +
> +	/* Select threshold and associated register */
> +	if (info == IIO_EV_INFO_VALUE) {
> +		if (dir == IIO_EV_DIR_RISING) {
> +			thresh = &data->t_rh_thresh_high;
> +			memcpy(buf, HDC3020_S_T_RH_THRESH_HIGH, 2);
> +		} else {
> +			thresh = &data->t_rh_thresh_low;
> +			memcpy(buf, HDC3020_S_T_RH_THRESH_LOW, 2);
> +		}
> +	} else {
> +		if (dir == IIO_EV_DIR_RISING) {
> +			thresh = &data->t_rh_thresh_high_clr;
> +			memcpy(buf, HDC3020_S_T_RH_THRESH_HIGH_CLR, 2);
> +		} else {
> +			thresh = &data->t_rh_thresh_low_clr;
> +			memcpy(buf, HDC3020_S_T_RH_THRESH_LOW_CLR, 2);
> +		}
> +	}
> +
> +	guard(mutex)(&data->lock);
> +	switch (chan->type) {
> +	case IIO_TEMP:
> +		/*
> +		 * Store truncated temperature threshold into 9 LSBs while
> +		 * keeping the old humidity threshold in the 7 MSBs.
> +		 */
> +		val = (((val + 45) * 65535 / 175) >> HDC3020_THRESH_TEMP_SHIFT);

Why 175?
If the span is -40/+120, I guess it should be 160 and if it is -45/+120, 
165. No?

Maybe something like:
   #define MIN_TEMP -45 (or -40)
   #define MAX_TEMP 120
in order to avoid hard coded constant?

> +		val &= HDC3020_THRESH_TEMP_MASK;
> +		val |= (*thresh & HDC3020_THRESH_HUM_MASK);
> +		break;
> +	case IIO_HUMIDITYRELATIVE:
> +		/*
> +		 * Store truncated humidity threshold into 7 MSBs while
> +		 * keeping the old temperature threshold in the 9 LSBs.
> +		 */
> +		val = ((val * 65535 / 100) & HDC3020_THRESH_HUM_MASK);
> +		val |= (*thresh & HDC3020_THRESH_TEMP_MASK);
> +		break;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +
> +	put_unaligned_be16(val, &buf[2]);
> +	buf[4] = crc8(hdc3020_crc8_table, buf + 2, 2, CRC8_INIT_VALUE);
> +	ret = hdc3020_write_bytes(data, buf, 5);
> +	if (ret)
> +		return ret;
> +
> +	/* Update threshold */
> +	*thresh = val;
> +
> +	return 0;
> +}

CJ