| Message ID | 20220130161101.1067691-9-liambeguin@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | iio: afe: add temperature rescaling support | expand |
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@gmail.com> > Reviewed-by: Peter Rosin <peda@axentia.se> > --- > 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);
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@gmail.com> > > Reviewed-by: Peter Rosin <peda@axentia.se> > > --- -- 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);
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); + + 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);