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

[Liam Beguin]

Hi Peter,
On Wed, Feb 02, 2022 at 05:58:25PM +0100, Peter Rosin wrote:
> 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>
> > ---

-- snip --

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

Apologies for this oversight. Your observation is correct, I looked at
the prefix changes and failed to catch this mistake.

Would you be okay with the following:

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

This would keep things consistent with what I said here[1].

[1] https://lore.kernel.org/linux-iio/YfmJ3P1gYaEkVjlY@shaak/

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

Sorry for the wasted cycles here.

Cheers,
Liam

> 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);