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 <dima.fedrau@xxxxxxxxx>
> ---
>
> 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
