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 <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?
>
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
