Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 2 Feb 2022 15:56:28 -0500
From:   Liam Beguin <liambeguin@gmail.com>
To:     Peter Rosin <peda@axentia.se>
Cc:     jic23@kernel.org, andy.shevchenko@gmail.com, lars@metafoo.de,
        linux-kernel@vger.kernel.org, linux-iio@vger.kernel.org,
        devicetree@vger.kernel.org, robh+dt@kernel.org
Subject: Re: [PATCH v13 08/11] iio: afe: rescale: add RTD temperature sensor
 support
Message-ID: <Yfrv/K8ohqTINEED@shaak>
References: <20220130161101.1067691-1-liambeguin@gmail.com>
 <20220130161101.1067691-9-liambeguin@gmail.com>
 <cb48ec7c-da91-cadd-ee7f-02691671e7be@axentia.se>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <cb48ec7c-da91-cadd-ee7f-02691671e7be@axentia.se>
Precedence: bulk

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