Received-SPF: pass (google.com: domain of linux-kernel+bounces-187622-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <cef810ee-bb2c-4588-baec-7edfc74daeea@gmail.com>
Date: Thu, 23 May 2024 16:12:37 +0200
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] iio: humidity: hdc3020: fix hysteresis representation
To: Dimitri Fedrau <dima.fedrau@gmail.com>
Cc: Li peiyu <579lpy@gmail.com>, Jonathan Cameron <jic23@kernel.org>,
 Lars-Peter Clausen <lars@metafoo.de>, linux-iio@vger.kernel.org,
 linux-kernel@vger.kernel.org
References: <20240523114336.532428-1-dima.fedrau@gmail.com>
Content-Language: en-US, de-AT
From: Javier Carrasco <javier.carrasco.cruz@gmail.com>
In-Reply-To: <20240523114336.532428-1-dima.fedrau@gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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@gmail.com>
> ---
> 
> 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