According to the ABI docs hysteresis values are represented as offsets to
threshold values. Current implementation represents hysteresis values as
absolute values which is wrong. Nevertheless the device stores them as
absolute values and the datasheet refers to them as clear thresholds. Fix
the reading and writing of hysteresis values by including thresholds into
calculations.
Fixes: 3ad0e7e5f0cb ("iio: humidity: hdc3020: add threshold events support")
Signed-off-by: Dimitri Fedrau <[email protected]>
---
Since absolute values are used on the device, the hysteresis values are
influenced by setting thresholds. Is this behavior in line with the ABI docs ?
It can be fixed by readjusting the threshold clear value whenever setting
thresholds to have the same hysteresis value as before. See some example below:
# echo 25 > /sys/bus/iio/devices/iio\:device0/events/in_temp_thresh_rising_value
# cat /sys/bus/iio/devices/iio\:device0/events/in_temp_thresh_rising_value
24.727626459
# echo 5 > /sys/bus/iio/devices/iio\:device0/events/in_temp_thresh_rising_hysteresis
# cat /sys/bus/iio/devices/iio\:device0/events/in_temp_thresh_rising_hysteresis
5.127031357
# echo 35 > /sys/bus/iio/devices/iio\:device0/events/in_temp_thresh_rising_value
# cat /sys/bus/iio/devices/iio\:device0/events/in_temp_thresh_rising_hysteresis
15.381094071
Below are some corner cases tested by setting threshold and hysteresis values.
To check that the threshold clear values are correct, registers are read out by
using i2ctransfer and the corresponding temperature and relative humidity
thresholds are calculated using the formulas in the datasheet.
# echo 125 > in_temp_thresh_rising_value
# cat in_temp_thresh_rising_value
124.875638971
# echo 165 > in_temp_thresh_rising_hysteresis
# cat in_temp_thresh_rising_hysteresis
164.748607614
# echo 100 > in_humidityrelative_thresh_rising_value
# cat in_humidityrelative_thresh_rising_value
99.220263981
# echo 100 > in_humidityrelative_thresh_rising_hysteresis
# cat in_humidityrelative_thresh_rising_hysteresis
99.220263981
threshold high, temperature = 124,875638972 C, humidity = 99.220263981
# i2ctransfer -f -y 4 w2@0x44 0xe1 0x1f r3
0xff 0xf1 0xb3
threshold high clear, temperature = -39.872968643 C, humidity = 0
# i2ctransfer -f -y 4 w2@0x44 0xe1 0x14 r3
0x00 0x0f 0xaf
# echo -40 > in_temp_thresh_falling_value
# cat in_temp_thresh_falling_value
-39.872968642
# echo 165 > in_temp_thresh_falling_hysteresis
# cat in_temp_thresh_falling_hysteresis
164.406805523
# echo 0 > in_humidityrelative_thresh_falling_value
# cat in_humidityrelative_thresh_falling_value
0.000000000
# echo 100 > in_humidityrelative_thresh_falling_hysteresis
# cat in_humidityrelative_thresh_falling_hysteresis
99.220263981
threshold low, temperature = -39.872968643 C, humidity = 0
# i2ctransfer -f -y 4 w2@0x44 0xe1 0x02 r3
0x00 0x0f 0xaf
threshold low clear, temperature = 124,533836881 C, humidity = 99,220263981
# i2ctransfer -f -y 4 w2@0x44 0xe1 0x09 r3
0xff 0xf0 0x82
---
drivers/iio/humidity/hdc3020.c | 292 +++++++++++++++++++++++++--------
1 file changed, 221 insertions(+), 71 deletions(-)
diff --git a/drivers/iio/humidity/hdc3020.c b/drivers/iio/humidity/hdc3020.c
index cdc4789213ba..d41713ff1deb 100644
--- a/drivers/iio/humidity/hdc3020.c
+++ b/drivers/iio/humidity/hdc3020.c
@@ -19,6 +19,8 @@
#include <linux/i2c.h>
#include <linux/init.h>
#include <linux/interrupt.h>
+#include <linux/math.h>
+#include <linux/math64.h>
#include <linux/module.h>
#include <linux/mutex.h>
#include <linux/pm.h>
@@ -66,8 +68,10 @@
#define HDC3020_CRC8_POLYNOMIAL 0x31
-#define HDC3020_MIN_TEMP -40
-#define HDC3020_MAX_TEMP 125
+#define HDC3020_MIN_TEMP_MICRO -39872968
+#define HDC3020_MAX_TEMP_MICRO 124875639
+#define HDC3020_MAX_TEMP_HYST_MICRO 164748607
+#define HDC3020_MAX_HUM_MICRO 99220264
struct hdc3020_data {
struct i2c_client *client;
@@ -368,6 +372,75 @@ static int hdc3020_write_raw(struct iio_dev *indio_dev,
return -EINVAL;
}
+static int hdc3020_tresh_get_temp(u16 thresh)
+{
+ int temp;
+
+ /*
+ * Get the temperature threshold from 9 LSBs, shift them to get
+ * the truncated temperature threshold representation and
+ * calculate the threshold according to the formula in the
+ * datasheet. Result is degree celsius scaled by 65535.
+ */
+ temp = FIELD_GET(HDC3020_THRESH_TEMP_MASK, thresh) <<
+ HDC3020_THRESH_TEMP_TRUNC_SHIFT;
+
+ return -2949075 + (175 * temp);
+}
+
+static int hdc3020_tresh_get_hum(u16 thresh)
+{
+ int hum;
+
+ /*
+ * Get the humidity threshold from 7 MSBs, shift them to get the
+ * truncated humidity threshold representation and calculate the
+ * threshold according to the formula in the datasheet. Result is
+ * percent scaled by 65535.
+ */
+ hum = FIELD_GET(HDC3020_THRESH_HUM_MASK, thresh) <<
+ HDC3020_THRESH_HUM_TRUNC_SHIFT;
+
+ return hum * 100;
+}
+
+static u16 hdc3020_thresh_set_temp(int s_temp, u16 curr_thresh)
+{
+ u64 temp;
+ u16 thresh;
+
+ /*
+ * Calculate temperature threshold, shift it down to get the
+ * truncated threshold representation in the 9LSBs while keeping
+ * the current humidity threshold in the 7 MSBs.
+ */
+ temp = (u64)(s_temp + 45000000) * 65535ULL;
+ temp = div_u64(temp, 1000000 * 175) >> HDC3020_THRESH_TEMP_TRUNC_SHIFT;
+ thresh = FIELD_PREP(HDC3020_THRESH_TEMP_MASK, temp);
+ thresh |= (FIELD_GET(HDC3020_THRESH_HUM_MASK, curr_thresh) <<
+ HDC3020_THRESH_HUM_TRUNC_SHIFT);
+
+ return thresh;
+}
+
+static u16 hdc3020_thresh_set_hum(int s_hum, u16 curr_thresh)
+{
+ u64 hum;
+ u16 thresh;
+
+ /*
+ * Calculate humidity threshold, shift it down and up to get the
+ * truncated threshold representation in the 7MSBs while keeping
+ * the current temperature threshold in the 9 LSBs.
+ */
+ hum = (u64)(s_hum) * 65535ULL;
+ hum = div_u64(hum, 1000000 * 100) >> HDC3020_THRESH_HUM_TRUNC_SHIFT;
+ thresh = FIELD_PREP(HDC3020_THRESH_HUM_MASK, hum);
+ thresh |= FIELD_GET(HDC3020_THRESH_TEMP_MASK, curr_thresh);
+
+ return thresh;
+}
+
static int hdc3020_write_thresh(struct iio_dev *indio_dev,
const struct iio_chan_spec *chan,
enum iio_event_type type,
@@ -376,65 +449,130 @@ static int hdc3020_write_thresh(struct iio_dev *indio_dev,
int val, int val2)
{
struct hdc3020_data *data = iio_priv(indio_dev);
+ u16 reg, reg_val, reg_thresh_rd, reg_clr_rd, reg_thresh_wr, reg_clr_wr;
+ s64 s_thresh, s_hyst, s_clr;
+ int s_val, ret;
u8 buf[5];
- u64 tmp;
- u16 reg;
- int ret;
-
- /* Supported temperature range is from –40 to 125 degree celsius */
- if (val < HDC3020_MIN_TEMP || val > HDC3020_MAX_TEMP)
- return -EINVAL;
- /* Select threshold register */
- if (info == IIO_EV_INFO_VALUE) {
- if (dir == IIO_EV_DIR_RISING)
- reg = HDC3020_S_T_RH_THRESH_HIGH;
- else
- reg = HDC3020_S_T_RH_THRESH_LOW;
+ /* Select threshold registers */
+ if (dir == IIO_EV_DIR_RISING) {
+ reg_thresh_rd = HDC3020_R_T_RH_THRESH_HIGH;
+ reg_thresh_wr = HDC3020_S_T_RH_THRESH_HIGH;
+ reg_clr_rd = HDC3020_R_T_RH_THRESH_HIGH_CLR;
+ reg_clr_wr = HDC3020_S_T_RH_THRESH_HIGH_CLR;
} else {
- if (dir == IIO_EV_DIR_RISING)
- reg = HDC3020_S_T_RH_THRESH_HIGH_CLR;
- else
- reg = HDC3020_S_T_RH_THRESH_LOW_CLR;
+ reg_thresh_rd = HDC3020_R_T_RH_THRESH_LOW;
+ reg_thresh_wr = HDC3020_S_T_RH_THRESH_LOW;
+ reg_clr_rd = HDC3020_R_T_RH_THRESH_LOW_CLR;
+ reg_clr_wr = HDC3020_S_T_RH_THRESH_LOW_CLR;
}
guard(mutex)(&data->lock);
- ret = hdc3020_read_be16(data, reg);
+ ret = hdc3020_read_be16(data, reg_thresh_rd);
if (ret < 0)
return ret;
+ /* Scale value to include decimal part into calculations */
+ s_val = (val < 0) ? (val * 1000000 - val2) : (val * 1000000 + val2);
+
switch (chan->type) {
case IIO_TEMP:
- /*
- * Calculate temperature threshold, shift it down to get the
- * truncated threshold representation in the 9LSBs while keeping
- * the current humidity threshold in the 7 MSBs.
- */
- tmp = ((u64)(((val + 45) * MICRO) + val2)) * 65535ULL;
- tmp = div_u64(tmp, MICRO * 175);
- val = tmp >> HDC3020_THRESH_TEMP_TRUNC_SHIFT;
- val = FIELD_PREP(HDC3020_THRESH_TEMP_MASK, val);
- val |= (FIELD_GET(HDC3020_THRESH_HUM_MASK, ret) <<
- HDC3020_THRESH_HUM_TRUNC_SHIFT);
+ switch (info) {
+ case IIO_EV_INFO_VALUE:
+ /* Range is from –40 to 125 degree celsius */
+ s_val = max(s_val, HDC3020_MIN_TEMP_MICRO);
+ s_val = min(s_val, HDC3020_MAX_TEMP_MICRO);
+
+ reg = reg_thresh_wr;
+ reg_val = hdc3020_thresh_set_temp(s_val, ret);
+ break;
+ case IIO_EV_INFO_HYSTERESIS:
+ /*
+ * Function hdc3020_tresh_get_temp returns temperature
+ * in degree celsius scaled by 65535. Scale by 1000000
+ * to be able to subtract scaled hysteresis value.
+ */
+ s_thresh = (s64)hdc3020_tresh_get_temp(ret) * 1000000;
+ /*
+ * Units of s_val are in micro degree celsius, scale by
+ * 65535 to get same units as s_thresh.
+ */
+ s_val = min(abs(s_val), HDC3020_MAX_TEMP_HYST_MICRO);
+ s_hyst = (s64)s_val * 65535;
+ /*
+ * Include directions when calculation the clear value,
+ * since hysteresis is unsigned by definition and the
+ * clear value is an absolute value which is signed.
+ */
+ if (dir == IIO_EV_DIR_RISING)
+ s_clr = s_thresh - s_hyst;
+ else
+ s_clr = s_thresh + s_hyst;
+
+ /* Devide by 65535 to get units of micro degree celsius */
+ s_val = div_s64(s_clr, 65535);
+ ret = hdc3020_read_be16(data, reg_clr_rd);
+ if (ret < 0)
+ return ret;
+
+ reg = reg_clr_wr;
+ reg_val = hdc3020_thresh_set_temp(s_val, ret);
+ break;
+ default:
+ return -EOPNOTSUPP;
+ }
break;
case IIO_HUMIDITYRELATIVE:
- /*
- * Calculate humidity threshold, shift it down and up to get the
- * truncated threshold representation in the 7MSBs while keeping
- * the current temperature threshold in the 9 LSBs.
- */
- tmp = ((u64)((val * MICRO) + val2)) * 65535ULL;
- tmp = div_u64(tmp, MICRO * 100);
- val = tmp >> HDC3020_THRESH_HUM_TRUNC_SHIFT;
- val = FIELD_PREP(HDC3020_THRESH_HUM_MASK, val);
- val |= FIELD_GET(HDC3020_THRESH_TEMP_MASK, ret);
+ switch (info) {
+ case IIO_EV_INFO_VALUE:
+ /* Range is from 0 to 100 percent */
+ s_val = min(abs(s_val), HDC3020_MAX_HUM_MICRO);
+
+ reg = reg_thresh_wr;
+ reg_val = hdc3020_thresh_set_hum(s_val, ret);
+ break;
+ case IIO_EV_INFO_HYSTERESIS:
+ /*
+ * Function hdc3020_tresh_get_hum returns relative
+ * humidity in percent scaled by 65535. Scale by 1000000
+ * to be able to subtract scaled hysteresis value.
+ */
+ s_thresh = (s64)hdc3020_tresh_get_hum(ret) * 1000000;
+ /*
+ * Units of s_val are in micro percent, scale by 65535
+ * to get same units as s_thresh.
+ */
+ s_val = min(abs(s_val), HDC3020_MAX_HUM_MICRO);
+ s_hyst = (s64)s_val * 65535;
+ /*
+ * Include directions when calculation the clear value,
+ * since hysteresis is unsigned by definition and the
+ * clear value is an absolute value which is signed.
+ */
+ if (dir == IIO_EV_DIR_RISING)
+ s_clr = s_thresh - s_hyst;
+ else
+ s_clr = s_thresh + s_hyst;
+
+ /* Devide by 65535 to get units of micro degree percent */
+ s_val = div_s64(s_clr, 65535);
+ ret = hdc3020_read_be16(data, reg_clr_rd);
+ if (ret < 0)
+ return ret;
+
+ reg = reg_clr_wr;
+ reg_val = hdc3020_thresh_set_hum(s_val, ret);
+ break;
+ default:
+ return -EOPNOTSUPP;
+ }
break;
default:
return -EOPNOTSUPP;
}
put_unaligned_be16(reg, buf);
- put_unaligned_be16(val, buf + 2);
+ put_unaligned_be16(reg_val, buf + 2);
buf[4] = crc8(hdc3020_crc8_table, buf + 2, 2, CRC8_INIT_VALUE);
return hdc3020_write_bytes(data, buf, 5);
}
@@ -447,48 +585,60 @@ static int hdc3020_read_thresh(struct iio_dev *indio_dev,
int *val, int *val2)
{
struct hdc3020_data *data = iio_priv(indio_dev);
- u16 reg;
- int ret;
+ u16 reg_thresh, reg_clr;
+ int thresh, clr, ret;
- /* Select threshold register */
- if (info == IIO_EV_INFO_VALUE) {
- if (dir == IIO_EV_DIR_RISING)
- reg = HDC3020_R_T_RH_THRESH_HIGH;
- else
- reg = HDC3020_R_T_RH_THRESH_LOW;
+ /* Select threshold registers */
+ if (dir == IIO_EV_DIR_RISING) {
+ reg_thresh = HDC3020_R_T_RH_THRESH_HIGH;
+ reg_clr = HDC3020_R_T_RH_THRESH_HIGH_CLR;
} else {
- if (dir == IIO_EV_DIR_RISING)
- reg = HDC3020_R_T_RH_THRESH_HIGH_CLR;
- else
- reg = HDC3020_R_T_RH_THRESH_LOW_CLR;
+ reg_thresh = HDC3020_R_T_RH_THRESH_LOW;
+ reg_clr = HDC3020_R_T_RH_THRESH_LOW_CLR;
}
guard(mutex)(&data->lock);
- ret = hdc3020_read_be16(data, reg);
+ ret = hdc3020_read_be16(data, reg_thresh);
if (ret < 0)
return ret;
switch (chan->type) {
case IIO_TEMP:
- /*
- * Get the temperature threshold from 9 LSBs, shift them to get
- * the truncated temperature threshold representation and
- * calculate the threshold according to the formula in the
- * datasheet.
- */
- *val = FIELD_GET(HDC3020_THRESH_TEMP_MASK, ret);
- *val = *val << HDC3020_THRESH_TEMP_TRUNC_SHIFT;
- *val = -2949075 + (175 * (*val));
+ thresh = hdc3020_tresh_get_temp(ret);
+ switch (info) {
+ case IIO_EV_INFO_VALUE:
+ *val = thresh;
+ break;
+ case IIO_EV_INFO_HYSTERESIS:
+ ret = hdc3020_read_be16(data, reg_clr);
+ if (ret < 0)
+ return ret;
+
+ clr = hdc3020_tresh_get_temp(ret);
+ *val = abs(thresh - clr);
+ break;
+ default:
+ return -EOPNOTSUPP;
+ }
*val2 = 65535;
return IIO_VAL_FRACTIONAL;
case IIO_HUMIDITYRELATIVE:
- /*
- * Get the humidity threshold from 7 MSBs, shift them to get the
- * truncated humidity threshold representation and calculate the
- * threshold according to the formula in the datasheet.
- */
- *val = FIELD_GET(HDC3020_THRESH_HUM_MASK, ret);
- *val = (*val << HDC3020_THRESH_HUM_TRUNC_SHIFT) * 100;
+ thresh = hdc3020_tresh_get_hum(ret);
+ switch (info) {
+ case IIO_EV_INFO_VALUE:
+ *val = thresh;
+ break;
+ case IIO_EV_INFO_HYSTERESIS:
+ ret = hdc3020_read_be16(data, reg_clr);
+ if (ret < 0)
+ return ret;
+
+ clr = hdc3020_tresh_get_hum(ret);
+ *val = abs(thresh - clr);
+ break;
+ default:
+ return -EOPNOTSUPP;
+ }
*val2 = 65535;
return IIO_VAL_FRACTIONAL;
default:
--
2.39.2
Hi Dimitri, a few comments inline.
On 23/05/2024 13:43, Dimitri Fedrau wrote:
> According to the ABI docs hysteresis values are represented as offsets to
> threshold values. Current implementation represents hysteresis values as
> absolute values which is wrong. Nevertheless the device stores them as
> absolute values and the datasheet refers to them as clear thresholds. Fix
> the reading and writing of hysteresis values by including thresholds into
> calculations.
>
> Fixes: 3ad0e7e5f0cb ("iio: humidity: hdc3020: add threshold events support")
> Signed-off-by: Dimitri Fedrau <[email protected]>
> ---
>
> Since absolute values are used on the device, the hysteresis values are
> influenced by setting thresholds. Is this behavior in line with the ABI docs ?
> It can be fixed by readjusting the threshold clear value whenever setting
> thresholds to have the same hysteresis value as before. See some example below:
>
> # echo 25 > /sys/bus/iio/devices/iio\:device0/events/in_temp_thresh_rising_value
> # cat /sys/bus/iio/devices/iio\:device0/events/in_temp_thresh_rising_value
> 24.727626459
> # echo 5 > /sys/bus/iio/devices/iio\:device0/events/in_temp_thresh_rising_hysteresis
> # cat /sys/bus/iio/devices/iio\:device0/events/in_temp_thresh_rising_hysteresis
> 5.127031357
> # echo 35 > /sys/bus/iio/devices/iio\:device0/events/in_temp_thresh_rising_value
> # cat /sys/bus/iio/devices/iio\:device0/events/in_temp_thresh_rising_hysteresis
> 15.381094071
>
> Below are some corner cases tested by setting threshold and hysteresis values.
> To check that the threshold clear values are correct, registers are read out by
> using i2ctransfer and the corresponding temperature and relative humidity
> thresholds are calculated using the formulas in the datasheet.
>
> # echo 125 > in_temp_thresh_rising_value
> # cat in_temp_thresh_rising_value
> 124.875638971
>
> # echo 165 > in_temp_thresh_rising_hysteresis
> # cat in_temp_thresh_rising_hysteresis
> 164.748607614
>
> # echo 100 > in_humidityrelative_thresh_rising_value
> # cat in_humidityrelative_thresh_rising_value
> 99.220263981
>
> # echo 100 > in_humidityrelative_thresh_rising_hysteresis
> # cat in_humidityrelative_thresh_rising_hysteresis
> 99.220263981
>
> threshold high, temperature = 124,875638972 C, humidity = 99.220263981
> # i2ctransfer -f -y 4 w2@0x44 0xe1 0x1f r3
> 0xff 0xf1 0xb3
>
> threshold high clear, temperature = -39.872968643 C, humidity = 0
> # i2ctransfer -f -y 4 w2@0x44 0xe1 0x14 r3
> 0x00 0x0f 0xaf
>
>
> # echo -40 > in_temp_thresh_falling_value
> # cat in_temp_thresh_falling_value
> -39.872968642
>
> # echo 165 > in_temp_thresh_falling_hysteresis
> # cat in_temp_thresh_falling_hysteresis
> 164.406805523
>
> # echo 0 > in_humidityrelative_thresh_falling_value
> # cat in_humidityrelative_thresh_falling_value
> 0.000000000
>
> # echo 100 > in_humidityrelative_thresh_falling_hysteresis
> # cat in_humidityrelative_thresh_falling_hysteresis
> 99.220263981
>
> threshold low, temperature = -39.872968643 C, humidity = 0
> # i2ctransfer -f -y 4 w2@0x44 0xe1 0x02 r3
> 0x00 0x0f 0xaf
>
> threshold low clear, temperature = 124,533836881 C, humidity = 99,220263981
> # i2ctransfer -f -y 4 w2@0x44 0xe1 0x09 r3
> 0xff 0xf0 0x82
>
> ---
> drivers/iio/humidity/hdc3020.c | 292 +++++++++++++++++++++++++--------
> 1 file changed, 221 insertions(+), 71 deletions(-)
>
> diff --git a/drivers/iio/humidity/hdc3020.c b/drivers/iio/humidity/hdc3020.c
> index cdc4789213ba..d41713ff1deb 100644
> --- a/drivers/iio/humidity/hdc3020.c
> +++ b/drivers/iio/humidity/hdc3020.c
> @@ -19,6 +19,8 @@
> #include <linux/i2c.h>
> #include <linux/init.h>
> #include <linux/interrupt.h>
> +#include <linux/math.h>
Is math.h not included in math64.h?
> +#include <linux/math64.h>
> #include <linux/module.h>
> #include <linux/mutex.h>
> #include <linux/pm.h>
> @@ -66,8 +68,10 @@
>
> #define HDC3020_CRC8_POLYNOMIAL 0x31
>
> -#define HDC3020_MIN_TEMP -40
> -#define HDC3020_MAX_TEMP 125
> +#define HDC3020_MIN_TEMP_MICRO -39872968
> +#define HDC3020_MAX_TEMP_MICRO 124875639
> +#define HDC3020_MAX_TEMP_HYST_MICRO 164748607
> +#define HDC3020_MAX_HUM_MICRO 99220264
>
> struct hdc3020_data {
> struct i2c_client *client;
> @@ -368,6 +372,75 @@ static int hdc3020_write_raw(struct iio_dev *indio_dev,
> return -EINVAL;
> }
>
> +static int hdc3020_tresh_get_temp(u16 thresh)
> +{
> + int temp;
> +
> + /*
> + * Get the temperature threshold from 9 LSBs, shift them to get
> + * the truncated temperature threshold representation and
> + * calculate the threshold according to the formula in the
> + * datasheet. Result is degree celsius scaled by 65535.
> + */
> + temp = FIELD_GET(HDC3020_THRESH_TEMP_MASK, thresh) <<
> + HDC3020_THRESH_TEMP_TRUNC_SHIFT;
> +
> + return -2949075 + (175 * temp);
> +}
> +
> +static int hdc3020_tresh_get_hum(u16 thresh)
> +{
> + int hum;
> +
> + /*
> + * Get the humidity threshold from 7 MSBs, shift them to get the
> + * truncated humidity threshold representation and calculate the
> + * threshold according to the formula in the datasheet. Result is
> + * percent scaled by 65535.
> + */
> + hum = FIELD_GET(HDC3020_THRESH_HUM_MASK, thresh) <<
> + HDC3020_THRESH_HUM_TRUNC_SHIFT;
> +
> + return hum * 100;
> +}
> +
> +static u16 hdc3020_thresh_set_temp(int s_temp, u16 curr_thresh)
> +{
> + u64 temp;
> + u16 thresh;
> +
> + /*
> + * Calculate temperature threshold, shift it down to get the
> + * truncated threshold representation in the 9LSBs while keeping
> + * the current humidity threshold in the 7 MSBs.
> + */
> + temp = (u64)(s_temp + 45000000) * 65535ULL;
> + temp = div_u64(temp, 1000000 * 175) >> HDC3020_THRESH_TEMP_TRUNC_SHIFT;
> + thresh = FIELD_PREP(HDC3020_THRESH_TEMP_MASK, temp);
> + thresh |= (FIELD_GET(HDC3020_THRESH_HUM_MASK, curr_thresh) <<
> + HDC3020_THRESH_HUM_TRUNC_SHIFT);
> +
> + return thresh;
> +}
> +
> +static u16 hdc3020_thresh_set_hum(int s_hum, u16 curr_thresh)
> +{
> + u64 hum;
> + u16 thresh;
> +
> + /*
> + * Calculate humidity threshold, shift it down and up to get the
> + * truncated threshold representation in the 7MSBs while keeping
> + * the current temperature threshold in the 9 LSBs.
> + */
> + hum = (u64)(s_hum) * 65535ULL;
> + hum = div_u64(hum, 1000000 * 100) >> HDC3020_THRESH_HUM_TRUNC_SHIFT;
> + thresh = FIELD_PREP(HDC3020_THRESH_HUM_MASK, hum);
> + thresh |= FIELD_GET(HDC3020_THRESH_TEMP_MASK, curr_thresh);
> +
> + return thresh;
> +}
> +
> static int hdc3020_write_thresh(struct iio_dev *indio_dev,
> const struct iio_chan_spec *chan,
> enum iio_event_type type,
> @@ -376,65 +449,130 @@ static int hdc3020_write_thresh(struct iio_dev *indio_dev,
> int val, int val2)
> {
> struct hdc3020_data *data = iio_priv(indio_dev);
> + u16 reg, reg_val, reg_thresh_rd, reg_clr_rd, reg_thresh_wr, reg_clr_wr;
> + s64 s_thresh, s_hyst, s_clr;
> + int s_val, ret;
> u8 buf[5];
> - u64 tmp;
> - u16 reg;
> - int ret;
> -
> - /* Supported temperature range is from –40 to 125 degree celsius */
> - if (val < HDC3020_MIN_TEMP || val > HDC3020_MAX_TEMP)
> - return -EINVAL;
>
> - /* Select threshold register */
> - if (info == IIO_EV_INFO_VALUE) {
> - if (dir == IIO_EV_DIR_RISING)
> - reg = HDC3020_S_T_RH_THRESH_HIGH;
> - else
> - reg = HDC3020_S_T_RH_THRESH_LOW;
> + /* Select threshold registers */
> + if (dir == IIO_EV_DIR_RISING) {
> + reg_thresh_rd = HDC3020_R_T_RH_THRESH_HIGH;
> + reg_thresh_wr = HDC3020_S_T_RH_THRESH_HIGH;
Do we always need to set reg_clr_rd and reg_clr_wr? It seems that they
are only required for the IIO_EV_INFO_HYSTERESIS case, where the EV_DIR
is checked again Maybe we could even get rid of those auxiliary
variables, or have a single check for EV_DIR with the sign for the
operations.
> + reg_clr_rd = HDC3020_R_T_RH_THRESH_HIGH_CLR;
> + reg_clr_wr = HDC3020_S_T_RH_THRESH_HIGH_CLR;
> } else {
> - if (dir == IIO_EV_DIR_RISING)
> - reg = HDC3020_S_T_RH_THRESH_HIGH_CLR;
> - else
> - reg = HDC3020_S_T_RH_THRESH_LOW_CLR;
> + reg_thresh_rd = HDC3020_R_T_RH_THRESH_LOW;
> + reg_thresh_wr = HDC3020_S_T_RH_THRESH_LOW;> + reg_clr_rd = HDC3020_R_T_RH_THRESH_LOW_CLR;
> + reg_clr_wr = HDC3020_S_T_RH_THRESH_LOW_CLR;
> }
>
> guard(mutex)(&data->lock);
> - ret = hdc3020_read_be16(data, reg);
> + ret = hdc3020_read_be16(data, reg_thresh_rd);
> if (ret < 0)
> return ret;
>
> + /* Scale value to include decimal part into calculations */
> + s_val = (val < 0) ? (val * 1000000 - val2) : (val * 1000000 + val2);
> +
> switch (chan->type) {
> case IIO_TEMP:
> - /*
> - * Calculate temperature threshold, shift it down to get the
> - * truncated threshold representation in the 9LSBs while keeping
> - * the current humidity threshold in the 7 MSBs.
> - */
> - tmp = ((u64)(((val + 45) * MICRO) + val2)) * 65535ULL;
> - tmp = div_u64(tmp, MICRO * 175);
> - val = tmp >> HDC3020_THRESH_TEMP_TRUNC_SHIFT;
> - val = FIELD_PREP(HDC3020_THRESH_TEMP_MASK, val);
> - val |= (FIELD_GET(HDC3020_THRESH_HUM_MASK, ret) <<
> - HDC3020_THRESH_HUM_TRUNC_SHIFT);
> + switch (info) {
> + case IIO_EV_INFO_VALUE:
The comment could be dropped. The range is obvious from the constants
and the values don't mach anymore now that you use MICRO.
> + /* Range is from –40 to 125 degree celsius */
> + s_val = max(s_val, HDC3020_MIN_TEMP_MICRO);
> + s_val = min(s_val, HDC3020_MAX_TEMP_MICRO);
> +
> + reg = reg_thresh_wr;
> + reg_val = hdc3020_thresh_set_temp(s_val, ret);
> + break;
> + case IIO_EV_INFO_HYSTERESIS:
> + /*
> + * Function hdc3020_tresh_get_temp returns temperature
> + * in degree celsius scaled by 65535. Scale by 1000000
> + * to be able to subtract scaled hysteresis value.
> + */
> + s_thresh = (s64)hdc3020_tresh_get_temp(ret) * 1000000;
> + /*
> + * Units of s_val are in micro degree celsius, scale by
> + * 65535 to get same units as s_thresh.
> + */
> + s_val = min(abs(s_val), HDC3020_MAX_TEMP_HYST_MICRO);
> + s_hyst = (s64)s_val * 65535;
> + /*
> + * Include directions when calculation the clear value,
> + * since hysteresis is unsigned by definition and the
> + * clear value is an absolute value which is signed.
> + */
> + if (dir == IIO_EV_DIR_RISING)
> + s_clr = s_thresh - s_hyst;
> + else
> + s_clr = s_thresh + s_hyst;
> +
Nit: "Divide". You can avoid such typos by using checkpactch.pl with the
--codespell option.
> + /* Devide by 65535 to get units of micro degree celsius */
> + s_val = div_s64(s_clr, 65535);
> + ret = hdc3020_read_be16(data, reg_clr_rd);
> + if (ret < 0)
> + return ret;
> +
> + reg = reg_clr_wr;
> + reg_val = hdc3020_thresh_set_temp(s_val, ret);
> + break;
> + default:
> + return -EOPNOTSUPP;
> + }
> break;
> case IIO_HUMIDITYRELATIVE:
> - /*
> - * Calculate humidity threshold, shift it down and up to get the
> - * truncated threshold representation in the 7MSBs while keeping
> - * the current temperature threshold in the 9 LSBs.
> - */
> - tmp = ((u64)((val * MICRO) + val2)) * 65535ULL;
> - tmp = div_u64(tmp, MICRO * 100);
> - val = tmp >> HDC3020_THRESH_HUM_TRUNC_SHIFT;
> - val = FIELD_PREP(HDC3020_THRESH_HUM_MASK, val);
> - val |= FIELD_GET(HDC3020_THRESH_TEMP_MASK, ret);
> + switch (info) {
> + case IIO_EV_INFO_VALUE:
The 100% value does not match the max val anymore. Could be dropped too.
> + /* Range is from 0 to 100 percent */
> + s_val = min(abs(s_val), HDC3020_MAX_HUM_MICRO);
> +
> + reg = reg_thresh_wr;
> + reg_val = hdc3020_thresh_set_hum(s_val, ret);
> + break;
> + case IIO_EV_INFO_HYSTERESIS:
> + /*
> + * Function hdc3020_tresh_get_hum returns relative
> + * humidity in percent scaled by 65535. Scale by 1000000
> + * to be able to subtract scaled hysteresis value.
> + */
> + s_thresh = (s64)hdc3020_tresh_get_hum(ret) * 1000000;
> + /*
> + * Units of s_val are in micro percent, scale by 65535
> + * to get same units as s_thresh.
> + */
> + s_val = min(abs(s_val), HDC3020_MAX_HUM_MICRO);
> + s_hyst = (s64)s_val * 65535;
> + /*
> + * Include directions when calculation the clear value,
> + * since hysteresis is unsigned by definition and the
> + * clear value is an absolute value which is signed.
> + */
> + if (dir == IIO_EV_DIR_RISING)
> + s_clr = s_thresh - s_hyst;
> + else
> + s_clr = s_thresh + s_hyst;
> +
Nit: "Divide".
> + /* Devide by 65535 to get units of micro degree percent */
> + s_val = div_s64(s_clr, 65535);
> + ret = hdc3020_read_be16(data, reg_clr_rd);
> + if (ret < 0)
> + return ret;
> +
> + reg = reg_clr_wr;
> + reg_val = hdc3020_thresh_set_hum(s_val, ret);
> + break;
> + default:
> + return -EOPNOTSUPP;
> + }
> break;
> default:
> return -EOPNOTSUPP;
> }
>
> put_unaligned_be16(reg, buf);
> - put_unaligned_be16(val, buf + 2);
> + put_unaligned_be16(reg_val, buf + 2);
> buf[4] = crc8(hdc3020_crc8_table, buf + 2, 2, CRC8_INIT_VALUE);
Now that you are working on this function, maybe you could add the
missing empty line before the return to keep format consistency.
> return hdc3020_write_bytes(data, buf, 5);
> }
> @@ -447,48 +585,60 @@ static int hdc3020_read_thresh(struct iio_dev *indio_dev,
> int *val, int *val2)
> {
> struct hdc3020_data *data = iio_priv(indio_dev);
> - u16 reg;
> - int ret;
> + u16 reg_thresh, reg_clr;
> + int thresh, clr, ret;
>
> - /* Select threshold register */
> - if (info == IIO_EV_INFO_VALUE) {
> - if (dir == IIO_EV_DIR_RISING)
> - reg = HDC3020_R_T_RH_THRESH_HIGH;
> - else
> - reg = HDC3020_R_T_RH_THRESH_LOW;
> + /* Select threshold registers */
> + if (dir == IIO_EV_DIR_RISING) {
> + reg_thresh = HDC3020_R_T_RH_THRESH_HIGH;
> + reg_clr = HDC3020_R_T_RH_THRESH_HIGH_CLR;
> } else {
> - if (dir == IIO_EV_DIR_RISING)
> - reg = HDC3020_R_T_RH_THRESH_HIGH_CLR;
> - else
> - reg = HDC3020_R_T_RH_THRESH_LOW_CLR;
> + reg_thresh = HDC3020_R_T_RH_THRESH_LOW;
> + reg_clr = HDC3020_R_T_RH_THRESH_LOW_CLR;
> }
>
> guard(mutex)(&data->lock);
> - ret = hdc3020_read_be16(data, reg);
> + ret = hdc3020_read_be16(data, reg_thresh);
> if (ret < 0)
> return ret;
>
> switch (chan->type) {
> case IIO_TEMP:
> - /*
> - * Get the temperature threshold from 9 LSBs, shift them to get
> - * the truncated temperature threshold representation and
> - * calculate the threshold according to the formula in the
> - * datasheet.
> - */
> - *val = FIELD_GET(HDC3020_THRESH_TEMP_MASK, ret);
> - *val = *val << HDC3020_THRESH_TEMP_TRUNC_SHIFT;
> - *val = -2949075 + (175 * (*val));
> + thresh = hdc3020_tresh_get_temp(ret);
> + switch (info) {
> + case IIO_EV_INFO_VALUE:
> + *val = thresh;
> + break;
> + case IIO_EV_INFO_HYSTERESIS:
> + ret = hdc3020_read_be16(data, reg_clr);
> + if (ret < 0)
> + return ret;
> +
> + clr = hdc3020_tresh_get_temp(ret);
> + *val = abs(thresh - clr);
> + break;
> + default:
> + return -EOPNOTSUPP;
> + }
> *val2 = 65535;
> return IIO_VAL_FRACTIONAL;
> case IIO_HUMIDITYRELATIVE:
> - /*
> - * Get the humidity threshold from 7 MSBs, shift them to get the
> - * truncated humidity threshold representation and calculate the
> - * threshold according to the formula in the datasheet.
> - */
> - *val = FIELD_GET(HDC3020_THRESH_HUM_MASK, ret);
> - *val = (*val << HDC3020_THRESH_HUM_TRUNC_SHIFT) * 100;
> + thresh = hdc3020_tresh_get_hum(ret);
> + switch (info) {
> + case IIO_EV_INFO_VALUE:
> + *val = thresh;
> + break;
> + case IIO_EV_INFO_HYSTERESIS:
> + ret = hdc3020_read_be16(data, reg_clr);
> + if (ret < 0)
> + return ret;
> +
> + clr = hdc3020_tresh_get_hum(ret);
> + *val = abs(thresh - clr);
> + break;
> + default:
> + return -EOPNOTSUPP;
> + }
> *val2 = 65535;
> return IIO_VAL_FRACTIONAL;
> default:
Thank you for your patch and best regards,
Javier Carrasco
Am Thu, May 23, 2024 at 04:12:37PM +0200 schrieb Javier Carrasco:
> Hi Dimitri, a few comments inline.
>
> On 23/05/2024 13:43, Dimitri Fedrau wrote:
> > According to the ABI docs hysteresis values are represented as offsets to
> > threshold values. Current implementation represents hysteresis values as
> > absolute values which is wrong. Nevertheless the device stores them as
> > absolute values and the datasheet refers to them as clear thresholds. Fix
> > the reading and writing of hysteresis values by including thresholds into
> > calculations.
> >
> > Fixes: 3ad0e7e5f0cb ("iio: humidity: hdc3020: add threshold events support")
> > Signed-off-by: Dimitri Fedrau <[email protected]>
> > ---
> >
> > Since absolute values are used on the device, the hysteresis values are
> > influenced by setting thresholds. Is this behavior in line with the ABI docs ?
> > It can be fixed by readjusting the threshold clear value whenever setting
> > thresholds to have the same hysteresis value as before. See some example below:
> >
> > # echo 25 > /sys/bus/iio/devices/iio\:device0/events/in_temp_thresh_rising_value
> > # cat /sys/bus/iio/devices/iio\:device0/events/in_temp_thresh_rising_value
> > 24.727626459
> > # echo 5 > /sys/bus/iio/devices/iio\:device0/events/in_temp_thresh_rising_hysteresis
> > # cat /sys/bus/iio/devices/iio\:device0/events/in_temp_thresh_rising_hysteresis
> > 5.127031357
> > # echo 35 > /sys/bus/iio/devices/iio\:device0/events/in_temp_thresh_rising_value
> > # cat /sys/bus/iio/devices/iio\:device0/events/in_temp_thresh_rising_hysteresis
> > 15.381094071
> >
> > Below are some corner cases tested by setting threshold and hysteresis values.
> > To check that the threshold clear values are correct, registers are read out by
> > using i2ctransfer and the corresponding temperature and relative humidity
> > thresholds are calculated using the formulas in the datasheet.
> >
> > # echo 125 > in_temp_thresh_rising_value
> > # cat in_temp_thresh_rising_value
> > 124.875638971
> >
> > # echo 165 > in_temp_thresh_rising_hysteresis
> > # cat in_temp_thresh_rising_hysteresis
> > 164.748607614
> >
> > # echo 100 > in_humidityrelative_thresh_rising_value
> > # cat in_humidityrelative_thresh_rising_value
> > 99.220263981
> >
> > # echo 100 > in_humidityrelative_thresh_rising_hysteresis
> > # cat in_humidityrelative_thresh_rising_hysteresis
> > 99.220263981
> >
> > threshold high, temperature = 124,875638972 C, humidity = 99.220263981
> > # i2ctransfer -f -y 4 w2@0x44 0xe1 0x1f r3
> > 0xff 0xf1 0xb3
> >
> > threshold high clear, temperature = -39.872968643 C, humidity = 0
> > # i2ctransfer -f -y 4 w2@0x44 0xe1 0x14 r3
> > 0x00 0x0f 0xaf
> >
> >
> > # echo -40 > in_temp_thresh_falling_value
> > # cat in_temp_thresh_falling_value
> > -39.872968642
> >
> > # echo 165 > in_temp_thresh_falling_hysteresis
> > # cat in_temp_thresh_falling_hysteresis
> > 164.406805523
> >
> > # echo 0 > in_humidityrelative_thresh_falling_value
> > # cat in_humidityrelative_thresh_falling_value
> > 0.000000000
> >
> > # echo 100 > in_humidityrelative_thresh_falling_hysteresis
> > # cat in_humidityrelative_thresh_falling_hysteresis
> > 99.220263981
> >
> > threshold low, temperature = -39.872968643 C, humidity = 0
> > # i2ctransfer -f -y 4 w2@0x44 0xe1 0x02 r3
> > 0x00 0x0f 0xaf
> >
> > threshold low clear, temperature = 124,533836881 C, humidity = 99,220263981
> > # i2ctransfer -f -y 4 w2@0x44 0xe1 0x09 r3
> > 0xff 0xf0 0x82
> >
> > ---
> > drivers/iio/humidity/hdc3020.c | 292 +++++++++++++++++++++++++--------
> > 1 file changed, 221 insertions(+), 71 deletions(-)
> >
> > diff --git a/drivers/iio/humidity/hdc3020.c b/drivers/iio/humidity/hdc3020.c
> > index cdc4789213ba..d41713ff1deb 100644
> > --- a/drivers/iio/humidity/hdc3020.c
> > +++ b/drivers/iio/humidity/hdc3020.c
> > @@ -19,6 +19,8 @@
> > #include <linux/i2c.h>
> > #include <linux/init.h>
> > #include <linux/interrupt.h>
> > +#include <linux/math.h>
> Is math.h not included in math64.h?
>
It is. Will fix it.
> > +#include <linux/math64.h>
> > #include <linux/module.h>
> > #include <linux/mutex.h>
> > #include <linux/pm.h>
> > @@ -66,8 +68,10 @@
> >
> > #define HDC3020_CRC8_POLYNOMIAL 0x31
> >
> > -#define HDC3020_MIN_TEMP -40
> > -#define HDC3020_MAX_TEMP 125
> > +#define HDC3020_MIN_TEMP_MICRO -39872968
> > +#define HDC3020_MAX_TEMP_MICRO 124875639
> > +#define HDC3020_MAX_TEMP_HYST_MICRO 164748607
> > +#define HDC3020_MAX_HUM_MICRO 99220264
> >
> > struct hdc3020_data {
> > struct i2c_client *client;
> > @@ -368,6 +372,75 @@ static int hdc3020_write_raw(struct iio_dev *indio_dev,
> > return -EINVAL;
> > }
> >
> > +static int hdc3020_tresh_get_temp(u16 thresh)
> > +{
> > + int temp;
> > +
> > + /*
> > + * Get the temperature threshold from 9 LSBs, shift them to get
> > + * the truncated temperature threshold representation and
> > + * calculate the threshold according to the formula in the
> > + * datasheet. Result is degree celsius scaled by 65535.
> > + */
> > + temp = FIELD_GET(HDC3020_THRESH_TEMP_MASK, thresh) <<
> > + HDC3020_THRESH_TEMP_TRUNC_SHIFT;
> > +
> > + return -2949075 + (175 * temp);
> > +}
> > +
> > +static int hdc3020_tresh_get_hum(u16 thresh)
> > +{
> > + int hum;
> > +
> > + /*
> > + * Get the humidity threshold from 7 MSBs, shift them to get the
> > + * truncated humidity threshold representation and calculate the
> > + * threshold according to the formula in the datasheet. Result is
> > + * percent scaled by 65535.
> > + */
> > + hum = FIELD_GET(HDC3020_THRESH_HUM_MASK, thresh) <<
> > + HDC3020_THRESH_HUM_TRUNC_SHIFT;
> > +
> > + return hum * 100;
> > +}
> > +
> > +static u16 hdc3020_thresh_set_temp(int s_temp, u16 curr_thresh)
> > +{
> > + u64 temp;
> > + u16 thresh;
> > +
> > + /*
> > + * Calculate temperature threshold, shift it down to get the
> > + * truncated threshold representation in the 9LSBs while keeping
> > + * the current humidity threshold in the 7 MSBs.
> > + */
> > + temp = (u64)(s_temp + 45000000) * 65535ULL;
> > + temp = div_u64(temp, 1000000 * 175) >> HDC3020_THRESH_TEMP_TRUNC_SHIFT;
> > + thresh = FIELD_PREP(HDC3020_THRESH_TEMP_MASK, temp);
> > + thresh |= (FIELD_GET(HDC3020_THRESH_HUM_MASK, curr_thresh) <<
> > + HDC3020_THRESH_HUM_TRUNC_SHIFT);
> > +
> > + return thresh;
> > +}
> > +
> > +static u16 hdc3020_thresh_set_hum(int s_hum, u16 curr_thresh)
> > +{
> > + u64 hum;
> > + u16 thresh;
> > +
> > + /*
> > + * Calculate humidity threshold, shift it down and up to get the
> > + * truncated threshold representation in the 7MSBs while keeping
> > + * the current temperature threshold in the 9 LSBs.
> > + */
> > + hum = (u64)(s_hum) * 65535ULL;
> > + hum = div_u64(hum, 1000000 * 100) >> HDC3020_THRESH_HUM_TRUNC_SHIFT;
> > + thresh = FIELD_PREP(HDC3020_THRESH_HUM_MASK, hum);
> > + thresh |= FIELD_GET(HDC3020_THRESH_TEMP_MASK, curr_thresh);
> > +
> > + return thresh;
> > +}
> > +
> > static int hdc3020_write_thresh(struct iio_dev *indio_dev,
> > const struct iio_chan_spec *chan,
> > enum iio_event_type type,
> > @@ -376,65 +449,130 @@ static int hdc3020_write_thresh(struct iio_dev *indio_dev,
> > int val, int val2)
> > {
> > struct hdc3020_data *data = iio_priv(indio_dev);
> > + u16 reg, reg_val, reg_thresh_rd, reg_clr_rd, reg_thresh_wr, reg_clr_wr;
> > + s64 s_thresh, s_hyst, s_clr;
> > + int s_val, ret;
> > u8 buf[5];
> > - u64 tmp;
> > - u16 reg;
> > - int ret;
> > -
> > - /* Supported temperature range is from –40 to 125 degree celsius */
> > - if (val < HDC3020_MIN_TEMP || val > HDC3020_MAX_TEMP)
> > - return -EINVAL;
> >
> > - /* Select threshold register */
> > - if (info == IIO_EV_INFO_VALUE) {
> > - if (dir == IIO_EV_DIR_RISING)
> > - reg = HDC3020_S_T_RH_THRESH_HIGH;
> > - else
> > - reg = HDC3020_S_T_RH_THRESH_LOW;
> > + /* Select threshold registers */
> > + if (dir == IIO_EV_DIR_RISING) {
> > + reg_thresh_rd = HDC3020_R_T_RH_THRESH_HIGH;
> > + reg_thresh_wr = HDC3020_S_T_RH_THRESH_HIGH;
>
> Do we always need to set reg_clr_rd and reg_clr_wr? It seems that they
> are only required for the IIO_EV_INFO_HYSTERESIS case, where the EV_DIR
> is checked again Maybe we could even get rid of those auxiliary
> variables, or have a single check for EV_DIR with the sign for the
> operations.
>
Yes, you are right.
> > + reg_clr_rd = HDC3020_R_T_RH_THRESH_HIGH_CLR;
> > + reg_clr_wr = HDC3020_S_T_RH_THRESH_HIGH_CLR;
> > } else {
> > - if (dir == IIO_EV_DIR_RISING)
> > - reg = HDC3020_S_T_RH_THRESH_HIGH_CLR;
> > - else
> > - reg = HDC3020_S_T_RH_THRESH_LOW_CLR;
> > + reg_thresh_rd = HDC3020_R_T_RH_THRESH_LOW;
> > + reg_thresh_wr = HDC3020_S_T_RH_THRESH_LOW;> + reg_clr_rd = HDC3020_R_T_RH_THRESH_LOW_CLR;
> > + reg_clr_wr = HDC3020_S_T_RH_THRESH_LOW_CLR;
> > }
> >
> > guard(mutex)(&data->lock);
> > - ret = hdc3020_read_be16(data, reg);
> > + ret = hdc3020_read_be16(data, reg_thresh_rd);
> > if (ret < 0)
> > return ret;
> >
> > + /* Scale value to include decimal part into calculations */
> > + s_val = (val < 0) ? (val * 1000000 - val2) : (val * 1000000 + val2);
> > +
> > switch (chan->type) {
> > case IIO_TEMP:
> > - /*
> > - * Calculate temperature threshold, shift it down to get the
> > - * truncated threshold representation in the 9LSBs while keeping
> > - * the current humidity threshold in the 7 MSBs.
> > - */
> > - tmp = ((u64)(((val + 45) * MICRO) + val2)) * 65535ULL;
> > - tmp = div_u64(tmp, MICRO * 175);
> > - val = tmp >> HDC3020_THRESH_TEMP_TRUNC_SHIFT;
> > - val = FIELD_PREP(HDC3020_THRESH_TEMP_MASK, val);
> > - val |= (FIELD_GET(HDC3020_THRESH_HUM_MASK, ret) <<
> > - HDC3020_THRESH_HUM_TRUNC_SHIFT);
> > + switch (info) {
> > + case IIO_EV_INFO_VALUE:
>
> The comment could be dropped. The range is obvious from the constants
> and the values don't mach anymore now that you use MICRO.
>
Yes.
> > + /* Range is from –40 to 125 degree celsius */
> > + s_val = max(s_val, HDC3020_MIN_TEMP_MICRO);
> > + s_val = min(s_val, HDC3020_MAX_TEMP_MICRO);
> > +
> > + reg = reg_thresh_wr;
> > + reg_val = hdc3020_thresh_set_temp(s_val, ret);
> > + break;
> > + case IIO_EV_INFO_HYSTERESIS:
> > + /*
> > + * Function hdc3020_tresh_get_temp returns temperature
> > + * in degree celsius scaled by 65535. Scale by 1000000
> > + * to be able to subtract scaled hysteresis value.
> > + */
> > + s_thresh = (s64)hdc3020_tresh_get_temp(ret) * 1000000;
> > + /*
> > + * Units of s_val are in micro degree celsius, scale by
> > + * 65535 to get same units as s_thresh.
> > + */
> > + s_val = min(abs(s_val), HDC3020_MAX_TEMP_HYST_MICRO);
> > + s_hyst = (s64)s_val * 65535;
> > + /*
> > + * Include directions when calculation the clear value,
> > + * since hysteresis is unsigned by definition and the
> > + * clear value is an absolute value which is signed.
> > + */
> > + if (dir == IIO_EV_DIR_RISING)
> > + s_clr = s_thresh - s_hyst;
> > + else
> > + s_clr = s_thresh + s_hyst;
> > +
>
> Nit: "Divide". You can avoid such typos by using checkpactch.pl with the
> --codespell option.
>
Thanks, didn't know that. Will use it for future patches.
> > + /* Devide by 65535 to get units of micro degree celsius */
> > + s_val = div_s64(s_clr, 65535);
> > + ret = hdc3020_read_be16(data, reg_clr_rd);
> > + if (ret < 0)
> > + return ret;
> > +
> > + reg = reg_clr_wr;
> > + reg_val = hdc3020_thresh_set_temp(s_val, ret);
> > + break;
> > + default:
> > + return -EOPNOTSUPP;
> > + }
> > break;
> > case IIO_HUMIDITYRELATIVE:
> > - /*
> > - * Calculate humidity threshold, shift it down and up to get the
> > - * truncated threshold representation in the 7MSBs while keeping
> > - * the current temperature threshold in the 9 LSBs.
> > - */
> > - tmp = ((u64)((val * MICRO) + val2)) * 65535ULL;
> > - tmp = div_u64(tmp, MICRO * 100);
> > - val = tmp >> HDC3020_THRESH_HUM_TRUNC_SHIFT;
> > - val = FIELD_PREP(HDC3020_THRESH_HUM_MASK, val);
> > - val |= FIELD_GET(HDC3020_THRESH_TEMP_MASK, ret);
> > + switch (info) {
> > + case IIO_EV_INFO_VALUE:
>
> The 100% value does not match the max val anymore. Could be dropped too.
>
Ok.
> > + /* Range is from 0 to 100 percent */
> > + s_val = min(abs(s_val), HDC3020_MAX_HUM_MICRO);
> > +
> > + reg = reg_thresh_wr;
> > + reg_val = hdc3020_thresh_set_hum(s_val, ret);
> > + break;
> > + case IIO_EV_INFO_HYSTERESIS:
> > + /*
> > + * Function hdc3020_tresh_get_hum returns relative
> > + * humidity in percent scaled by 65535. Scale by 1000000
> > + * to be able to subtract scaled hysteresis value.
> > + */
> > + s_thresh = (s64)hdc3020_tresh_get_hum(ret) * 1000000;
> > + /*
> > + * Units of s_val are in micro percent, scale by 65535
> > + * to get same units as s_thresh.
> > + */
> > + s_val = min(abs(s_val), HDC3020_MAX_HUM_MICRO);
> > + s_hyst = (s64)s_val * 65535;
> > + /*
> > + * Include directions when calculation the clear value,
> > + * since hysteresis is unsigned by definition and the
> > + * clear value is an absolute value which is signed.
> > + */
> > + if (dir == IIO_EV_DIR_RISING)
> > + s_clr = s_thresh - s_hyst;
> > + else
> > + s_clr = s_thresh + s_hyst;
> > +
>
> Nit: "Divide".
>
Ok.
> > + /* Devide by 65535 to get units of micro degree percent */
> > + s_val = div_s64(s_clr, 65535);
> > + ret = hdc3020_read_be16(data, reg_clr_rd);
> > + if (ret < 0)
> > + return ret;
> > +
> > + reg = reg_clr_wr;
> > + reg_val = hdc3020_thresh_set_hum(s_val, ret);
> > + break;
> > + default:
> > + return -EOPNOTSUPP;
> > + }
> > break;
> > default:
> > return -EOPNOTSUPP;
> > }
> >
> > put_unaligned_be16(reg, buf);
> > - put_unaligned_be16(val, buf + 2);
> > + put_unaligned_be16(reg_val, buf + 2);
> > buf[4] = crc8(hdc3020_crc8_table, buf + 2, 2, CRC8_INIT_VALUE);
>
> Now that you are working on this function, maybe you could add the
> missing empty line before the return to keep format consistency.
>
Ok.
> > return hdc3020_write_bytes(data, buf, 5);
> > }
> > @@ -447,48 +585,60 @@ static int hdc3020_read_thresh(struct iio_dev *indio_dev,
> > int *val, int *val2)
> > {
> > struct hdc3020_data *data = iio_priv(indio_dev);
> > - u16 reg;
> > - int ret;
> > + u16 reg_thresh, reg_clr;
> > + int thresh, clr, ret;
> >
> > - /* Select threshold register */
> > - if (info == IIO_EV_INFO_VALUE) {
> > - if (dir == IIO_EV_DIR_RISING)
> > - reg = HDC3020_R_T_RH_THRESH_HIGH;
> > - else
> > - reg = HDC3020_R_T_RH_THRESH_LOW;
> > + /* Select threshold registers */
> > + if (dir == IIO_EV_DIR_RISING) {
> > + reg_thresh = HDC3020_R_T_RH_THRESH_HIGH;
> > + reg_clr = HDC3020_R_T_RH_THRESH_HIGH_CLR;
> > } else {
> > - if (dir == IIO_EV_DIR_RISING)
> > - reg = HDC3020_R_T_RH_THRESH_HIGH_CLR;
> > - else
> > - reg = HDC3020_R_T_RH_THRESH_LOW_CLR;
> > + reg_thresh = HDC3020_R_T_RH_THRESH_LOW;
> > + reg_clr = HDC3020_R_T_RH_THRESH_LOW_CLR;
> > }
> >
> > guard(mutex)(&data->lock);
> > - ret = hdc3020_read_be16(data, reg);
> > + ret = hdc3020_read_be16(data, reg_thresh);
> > if (ret < 0)
> > return ret;
> >
> > switch (chan->type) {
> > case IIO_TEMP:
> > - /*
> > - * Get the temperature threshold from 9 LSBs, shift them to get
> > - * the truncated temperature threshold representation and
> > - * calculate the threshold according to the formula in the
> > - * datasheet.
> > - */
> > - *val = FIELD_GET(HDC3020_THRESH_TEMP_MASK, ret);
> > - *val = *val << HDC3020_THRESH_TEMP_TRUNC_SHIFT;
> > - *val = -2949075 + (175 * (*val));
> > + thresh = hdc3020_tresh_get_temp(ret);
> > + switch (info) {
> > + case IIO_EV_INFO_VALUE:
> > + *val = thresh;
> > + break;
> > + case IIO_EV_INFO_HYSTERESIS:
> > + ret = hdc3020_read_be16(data, reg_clr);
> > + if (ret < 0)
> > + return ret;
> > +
> > + clr = hdc3020_tresh_get_temp(ret);
> > + *val = abs(thresh - clr);
> > + break;
> > + default:
> > + return -EOPNOTSUPP;
> > + }
> > *val2 = 65535;
> > return IIO_VAL_FRACTIONAL;
> > case IIO_HUMIDITYRELATIVE:
> > - /*
> > - * Get the humidity threshold from 7 MSBs, shift them to get the
> > - * truncated humidity threshold representation and calculate the
> > - * threshold according to the formula in the datasheet.
> > - */
> > - *val = FIELD_GET(HDC3020_THRESH_HUM_MASK, ret);
> > - *val = (*val << HDC3020_THRESH_HUM_TRUNC_SHIFT) * 100;
> > + thresh = hdc3020_tresh_get_hum(ret);
> > + switch (info) {
> > + case IIO_EV_INFO_VALUE:
> > + *val = thresh;
> > + break;
> > + case IIO_EV_INFO_HYSTERESIS:
> > + ret = hdc3020_read_be16(data, reg_clr);
> > + if (ret < 0)
> > + return ret;
> > +
> > + clr = hdc3020_tresh_get_hum(ret);
> > + *val = abs(thresh - clr);
> > + break;
> > + default:
> > + return -EOPNOTSUPP;
> > + }
> > *val2 = 65535;
> > return IIO_VAL_FRACTIONAL;
> > default:
>
>
> Thank you for your patch and best regards,
> Javier Carrasco
Hi Javier,
thanks for reviewing so quickly. Do you think I should correct the clear
threshold values once I changed the threshold. I have an example
provided where I set the threshold and hysteresis. After setting the
threshold again the hysteresis value also changes.
Dimitri
On 23/05/2024 16:28, Dimitri Fedrau wrote:
> Am Thu, May 23, 2024 at 04:12:37PM +0200 schrieb Javier Carrasco:
>> Hi Dimitri, a few comments inline.
>>
>> On 23/05/2024 13:43, Dimitri Fedrau wrote:
>>> According to the ABI docs hysteresis values are represented as offsets to
>>> threshold values. Current implementation represents hysteresis values as
>>> absolute values which is wrong. Nevertheless the device stores them as
>>> absolute values and the datasheet refers to them as clear thresholds. Fix
>>> the reading and writing of hysteresis values by including thresholds into
>>> calculations.
>>>
>>> Fixes: 3ad0e7e5f0cb ("iio: humidity: hdc3020: add threshold events support")
>>> Signed-off-by: Dimitri Fedrau <[email protected]>
>>> ---
>>>
>>> Since absolute values are used on the device, the hysteresis values are
>>> influenced by setting thresholds. Is this behavior in line with the ABI docs ?
>>> It can be fixed by readjusting the threshold clear value whenever setting
>>> thresholds to have the same hysteresis value as before. See some example below:
>>>
>>> # echo 25 > /sys/bus/iio/devices/iio\:device0/events/in_temp_thresh_rising_value
>>> # cat /sys/bus/iio/devices/iio\:device0/events/in_temp_thresh_rising_value
>>> 24.727626459
>>> # echo 5 > /sys/bus/iio/devices/iio\:device0/events/in_temp_thresh_rising_hysteresis
>>> # cat /sys/bus/iio/devices/iio\:device0/events/in_temp_thresh_rising_hysteresis
>>> 5.127031357
>>> # echo 35 > /sys/bus/iio/devices/iio\:device0/events/in_temp_thresh_rising_value
>>> # cat /sys/bus/iio/devices/iio\:device0/events/in_temp_thresh_rising_hysteresis
>>> 15.381094071
>>>
> Hi Javier,
>
> thanks for reviewing so quickly. Do you think I should correct the clear
> threshold values once I changed the threshold. I have an example
> provided where I set the threshold and hysteresis. After setting the
> threshold again the hysteresis value also changes.
>
> Dimitri
I am not sure if I got that right. Did the rising hysteresis change its
value automatically from ~5 to ~15 by just adding 10 to the threshold?
If we are treating the hysteresis as an offset, then it should not
change its value i.e. it should keep the configured value.
Best regards,
Javier Carrasco
Am Thu, May 23, 2024 at 05:45:34PM +0200 schrieb Javier Carrasco:
> On 23/05/2024 16:28, Dimitri Fedrau wrote:
> > Am Thu, May 23, 2024 at 04:12:37PM +0200 schrieb Javier Carrasco:
> >> Hi Dimitri, a few comments inline.
> >>
> >> On 23/05/2024 13:43, Dimitri Fedrau wrote:
> >>> According to the ABI docs hysteresis values are represented as offsets to
> >>> threshold values. Current implementation represents hysteresis values as
> >>> absolute values which is wrong. Nevertheless the device stores them as
> >>> absolute values and the datasheet refers to them as clear thresholds. Fix
> >>> the reading and writing of hysteresis values by including thresholds into
> >>> calculations.
> >>>
> >>> Fixes: 3ad0e7e5f0cb ("iio: humidity: hdc3020: add threshold events support")
> >>> Signed-off-by: Dimitri Fedrau <[email protected]>
> >>> ---
> >>>
> >>> Since absolute values are used on the device, the hysteresis values are
> >>> influenced by setting thresholds. Is this behavior in line with the ABI docs ?
> >>> It can be fixed by readjusting the threshold clear value whenever setting
> >>> thresholds to have the same hysteresis value as before. See some example below:
> >>>
> >>> # echo 25 > /sys/bus/iio/devices/iio\:device0/events/in_temp_thresh_rising_value
> >>> # cat /sys/bus/iio/devices/iio\:device0/events/in_temp_thresh_rising_value
> >>> 24.727626459
> >>> # echo 5 > /sys/bus/iio/devices/iio\:device0/events/in_temp_thresh_rising_hysteresis
> >>> # cat /sys/bus/iio/devices/iio\:device0/events/in_temp_thresh_rising_hysteresis
> >>> 5.127031357
> >>> # echo 35 > /sys/bus/iio/devices/iio\:device0/events/in_temp_thresh_rising_value
> >>> # cat /sys/bus/iio/devices/iio\:device0/events/in_temp_thresh_rising_hysteresis
> >>> 15.381094071
> >>>
>
> > Hi Javier,
> >
> > thanks for reviewing so quickly. Do you think I should correct the clear
> > threshold values once I changed the threshold. I have an example
> > provided where I set the threshold and hysteresis. After setting the
> > threshold again the hysteresis value also changes.
> >
> > Dimitri
>
> I am not sure if I got that right. Did the rising hysteresis change its
> value automatically from ~5 to ~15 by just adding 10 to the threshold?
> If we are treating the hysteresis as an offset, then it should not
> change its value i.e. it should keep the configured value.
>
> Best regards,
> Javier Carrasco
You got it right and I also think that the hysteresis value should keep
the configured value. The doc states that the hysteresis is represented
as offset to the threshold, but I didn't find anything related to the
changing hysteresis value. Propably because most devices implement the
hysteresis value as offset !? Would change this behavior in the next
version of the patch if Jonathan agrees.
Thanks again for taking time to review.
Dimitri