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

["Javier Carrasco" <javier.carrasco.cruz@gmail.com>]

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

> From: Dimitri Fedrau <dimitri.fedrau@liebherr.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