linux-kernel - Re: [PATCH 2/2] iio: humditiy: hdc3020: fix units for thresholds and hysteresis

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <DCANVO3ZBHUN.A8E9WABNLHG4@gmail.com>
Date: Sun, 24 Aug 2025 20:36:58 +0800
From: "Javier Carrasco" <javier.carrasco.cruz@...il.com>
To: <dimitri.fedrau@...bherr.com>, "Li peiyu" <579lpy@...il.com>, "Jonathan
 Cameron" <jic23@...nel.org>, "David Lechner" <dlechner@...libre.com>,
 Nuno Sá <nuno.sa@...log.com>, "Andy Shevchenko"
 <andy@...nel.org>, "Dimitri Fedrau" <dima.fedrau@...il.com>
Cc: "Jonathan Cameron" <Jonathan.Cameron@...wei.com>,
 <linux-iio@...r.kernel.org>, <linux-kernel@...r.kernel.org>, "Chris Lesiak"
 <chris.lesiak@...orbio.com>
Subject: Re: [PATCH 2/2] iio: humditiy: hdc3020: fix units for thresholds
 and hysteresis

Hello Dimitri, thank you for your patch. A few comments inline:

> From: Dimitri Fedrau <dimitri.fedrau@...bherr.com>
>
> According to the ABI the units after application of scale and offset are
> milli degree celsius for temperature thresholds and milli percent for
> relative humidity thresholds. Change scale factor to fix this issue.

I miss some explanation of what is going on (i.e. wrong) at the moment,
the scale factor that is being used and what results are being obtained.

> @@ -379,12 +379,12 @@ static int hdc3020_thresh_get_temp(u16 thresh)
>  	 * 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

Having used 65535 back then without explaining why, or at least without using
a #define was not the best idea. It's difficult to understand why it is used
without getting into details.

13107 is even less obvious. I believe you divided 65535 by 5 everywhere in the
code, but it's not clear why.

I'd suggest a clear definition at the beginning of the code because it
is used in different parts of the code, after having explained why it
is necessary in the commit message as I mentioned before.

> -	 * datasheet. Result is degree celsius scaled by 65535.
> +	 * datasheet. Result is degree celsius scaled by 13107.
>  	 */
>  	temp = FIELD_GET(HDC3020_THRESH_TEMP_MASK, thresh) <<
>  	       HDC3020_THRESH_TEMP_TRUNC_SHIFT;
>  

Again, it's difficult to understand why everything is divided by 5.

> -	return -2949075 + (175 * temp);
> +	return -589815 + (35 * temp);
>  }
>  
>  static int hdc3020_thresh_get_hum(u16 thresh)
> @@ -395,12 +395,12 @@ static int hdc3020_thresh_get_hum(u16 thresh)
>  	 * 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.
> +	 * percent scaled by 13107.
>  	 */
>  	hum = FIELD_GET(HDC3020_THRESH_HUM_MASK, thresh) <<
>  	      HDC3020_THRESH_HUM_TRUNC_SHIFT;
>  

Similarly, multiplying by 20 (100/5) looks weird for a percentage.

> -	return hum * 100;
> +	return hum * 20;
>  }

...

> @@ -630,7 +630,7 @@ static int hdc3020_read_thresh(struct iio_dev *indio_dev,
>  		thresh = hdc3020_thresh_get_temp(ret);
>  		switch (info) {
>  		case IIO_EV_INFO_VALUE:

MILLI, as suggested for [1/2]? The same would apply to the following
diffs.

> -			*val = thresh;
> +			*val = thresh * 1000;
>  			break;
>  		case IIO_EV_INFO_HYSTERESIS:
>  			ret = hdc3020_read_be16(data, reg_clr);
> @@ -638,18 +638,18 @@ static int hdc3020_read_thresh(struct iio_dev *indio_dev,
>  				return ret;
>  
>  			clr = hdc3020_thresh_get_temp(ret);

Thanks and best regards,
Javier Carrasco