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

Christophe JAILLET
2024-02-03T10:07:10Z

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@public.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