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