Re: [PATCH v13 08/11] iio: afe: rescale: add RTD temperature sensor support

[Peter Rosin]

Hi!

On 2022-01-30 17:10, Liam Beguin wrote:
> An RTD (Resistance Temperature Detector) is a kind of temperature
> sensor used to get a linear voltage to temperature reading within a
> give range (usually 0 to 100 degrees Celsius). Common types of RTDs
> include PT100, PT500, and PT1000.
> 
> Signed-off-by: Liam Beguin <liambeguin@xxxxxxxxx>
> Reviewed-by: Peter Rosin <peda@xxxxxxxxxx>
> ---
>  drivers/iio/afe/iio-rescale.c | 48 +++++++++++++++++++++++++++++++++++
>  1 file changed, 48 insertions(+)
> 
> diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c
> index 27c6664915ff..ae71a545c7e0 100644
> --- a/drivers/iio/afe/iio-rescale.c
> +++ b/drivers/iio/afe/iio-rescale.c
> @@ -395,10 +395,52 @@ static int rescale_voltage_divider_props(struct device *dev,
>  	return 0;
>  }
>  
> +static int rescale_temp_sense_rtd_props(struct device *dev,
> +					struct rescale *rescale)
> +{
> +	u32 factor;
> +	u32 alpha;
> +	u32 iexc;
> +	u32 tmp;
> +	int ret;
> +	u32 r0;
> +
> +	ret = device_property_read_u32(dev, "excitation-current-microamp",
> +				       &iexc);
> +	if (ret) {
> +		dev_err(dev, "failed to read excitation-current-microamp: %d\n",
> +			ret);
> +		return ret;
> +	}
> +
> +	ret = device_property_read_u32(dev, "alpha-ppm-per-celsius", &alpha);
> +	if (ret) {
> +		dev_err(dev, "failed to read alpha-ppm-per-celsius: %d\n",
> +			ret);
> +		return ret;
> +	}
> +
> +	ret = device_property_read_u32(dev, "r-naught-ohms", &r0);
> +	if (ret) {
> +		dev_err(dev, "failed to read r-naught-ohms: %d\n", ret);
> +		return ret;
> +	}
> +
> +	tmp = r0 * iexc * alpha / MEGA;
> +	factor = gcd(tmp, MEGA);
> +	rescale->numerator = MEGA / factor;
> +	rescale->denominator = tmp / factor;
> +
> +	rescale->offset = -1 * ((r0 * iexc) / MEGA * MILLI);

The inner (unneeded) brackets are not helping with clarifying
the precedence. The most "problematic" operation is the last
multiplication inside the outer brackets. Extra brackets are
more useful like this, methinks:

	rescale->offset = -1 * ((r0 * iexc / MEGA) * MILLI);

But, what is more important is that you in v10 had:

	rescale->offset = -1 * ((r0 * iexc) / 1000);

What you tricked yourself into writing when you converted to
these prefix defines is not equivalent. You lose precision.

I.e. dividing by 1000000 and then multiplying by 1000 is not
the same as dividing directly with 1000. And you know this, but
didn't notice perhaps exactly because you got yourself entangled
in prefix macros that blurred the picture?

These macros have wasted quite a bit of review time. I'm not
fully convinced they represent an improvement...

Cheers,
Peter

> +
> +	return 0;
> +}
> +
>  enum rescale_variant {
>  	CURRENT_SENSE_AMPLIFIER,
>  	CURRENT_SENSE_SHUNT,
>  	VOLTAGE_DIVIDER,
> +	TEMP_SENSE_RTD,
>  };
>  
>  static const struct rescale_cfg rescale_cfg[] = {
> @@ -414,6 +456,10 @@ static const struct rescale_cfg rescale_cfg[] = {
>  		.type = IIO_VOLTAGE,
>  		.props = rescale_voltage_divider_props,
>  	},
> +	[TEMP_SENSE_RTD] = {
> +		.type = IIO_TEMP,
> +		.props = rescale_temp_sense_rtd_props,
> +	},
>  };
>  
>  static const struct of_device_id rescale_match[] = {
> @@ -423,6 +469,8 @@ static const struct of_device_id rescale_match[] = {
>  	  .data = &rescale_cfg[CURRENT_SENSE_SHUNT], },
>  	{ .compatible = "voltage-divider",
>  	  .data = &rescale_cfg[VOLTAGE_DIVIDER], },
> +	{ .compatible = "temperature-sense-rtd",
> +	  .data = &rescale_cfg[TEMP_SENSE_RTD], },
>  	{ /* sentinel */ }
>  };
>  MODULE_DEVICE_TABLE(of, rescale_match);