Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Sat, 15 Jan 2022 18:52:03 +0000
From:   Jonathan Cameron <jic23@kernel.org>
To:     Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc:     Dmitry Baryshkov <dmitry.baryshkov@linaro.org>,
        Jonathan Cameron <Jonathan.Cameron@huawei.com>,
        Andreas Kemnade <andreas@kemnade.info>,
        linux-arm-msm@vger.kernel.org, linux-iio@vger.kernel.org,
        linux-kernel@vger.kernel.org, Andy Gross <agross@kernel.org>,
        Bjorn Andersson <bjorn.andersson@linaro.org>,
        Lars-Peter Clausen <lars@metafoo.de>,
        Peter Rosin <peda@axentia.se>
Subject: Re: [PATCH v2 5/5] iio: afe: iio-rescale: Re-use generic struct
 s32_fract
Message-ID: <20220115185203.567780e8@jic23-huawei>
In-Reply-To: <20220110193104.75225-5-andriy.shevchenko@linux.intel.com>
References: <20220110193104.75225-1-andriy.shevchenko@linux.intel.com>
        <20220110193104.75225-5-andriy.shevchenko@linux.intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Mon, 10 Jan 2022 21:31:04 +0200
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> Instead of custom data type re-use generic struct s32_fract.
> No changes intended.
> 
> The new member is put to be the first one to avoid additional
> pointer arithmetic. Besides that one may switch to use fract
> member to perform container_of(), which will be no-op in this
> case, to get struct rescale.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

I'm not totally sold on this series showing there is a strong case for
these macros so interested to hear what others think.
Boiler plate removal is always nice of course...

One trivial comment inline on this one.

> ---
> 
> I found this better in order how code is structurally (re)organized.
> I may rebase this on top of ongoing AFE series.
> 
> Also reveals possibility to switch to rational best approximation.
> But this is another story...

Now that may well justify introducing this shared infrastructure :)

> 
> v2: no changes
> 
>  drivers/iio/afe/iio-rescale.c | 74 +++++++++++++++++------------------
>  1 file changed, 37 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c
> index 774eb3044edd..0368bca8a485 100644
> --- a/drivers/iio/afe/iio-rescale.c
> +++ b/drivers/iio/afe/iio-rescale.c
> @@ -11,6 +11,7 @@
>  #include <linux/gcd.h>
>  #include <linux/iio/consumer.h>
>  #include <linux/iio/iio.h>
> +#include <linux/math.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
>  #include <linux/of_device.h>
> @@ -21,17 +22,16 @@ struct rescale;
>  
>  struct rescale_cfg {
>  	enum iio_chan_type type;
> -	int (*props)(struct device *dev, struct rescale *rescale);
> +	int (*props)(struct device *dev, struct s32_fract *fract);
>  };
>  
>  struct rescale {
> +	struct s32_fract fract;
>  	const struct rescale_cfg *cfg;
>  	struct iio_channel *source;
>  	struct iio_chan_spec chan;
>  	struct iio_chan_spec_ext_info *ext_info;
>  	bool chan_processed;
> -	s32 numerator;
> -	s32 denominator;
>  };
>  
>  static int rescale_read_raw(struct iio_dev *indio_dev,
> @@ -39,6 +39,7 @@ static int rescale_read_raw(struct iio_dev *indio_dev,
>  			    int *val, int *val2, long mask)
>  {
>  	struct rescale *rescale = iio_priv(indio_dev);
> +	struct s32_fract *fract = &rescale->fract;
>  	unsigned long long tmp;
>  	int ret;
>  
> @@ -67,19 +68,19 @@ static int rescale_read_raw(struct iio_dev *indio_dev,
>  		}
>  		switch (ret) {
>  		case IIO_VAL_FRACTIONAL:
> -			*val *= rescale->numerator;
> -			*val2 *= rescale->denominator;
> +			*val *= fract->numerator;
> +			*val2 *= fract->denominator;
>  			return ret;
>  		case IIO_VAL_INT:
> -			*val *= rescale->numerator;
> -			if (rescale->denominator == 1)
> +			*val *= fract->numerator;
> +			if (fract->denominator == 1)
>  				return ret;
> -			*val2 = rescale->denominator;
> +			*val2 = fract->denominator;
>  			return IIO_VAL_FRACTIONAL;
>  		case IIO_VAL_FRACTIONAL_LOG2:
>  			tmp = *val * 1000000000LL;
> -			do_div(tmp, rescale->denominator);
> -			tmp *= rescale->numerator;
> +			do_div(tmp, fract->denominator);
> +			tmp *= fract->numerator;
>  			do_div(tmp, 1000000000LL);
>  			*val = tmp;
>  			return ret;
> @@ -175,7 +176,7 @@ static int rescale_configure_channel(struct device *dev,
>  }
>  
>  static int rescale_current_sense_amplifier_props(struct device *dev,
> -						 struct rescale *rescale)
> +						 struct s32_fract *fract)
>  {
>  	u32 sense;
>  	u32 gain_mult = 1;
> @@ -199,22 +200,22 @@ static int rescale_current_sense_amplifier_props(struct device *dev,
>  	 * numerator/denominator from overflowing.
>  	 */
>  	factor = gcd(sense, 1000000);
> -	rescale->numerator = 1000000 / factor;
> -	rescale->denominator = sense / factor;
> +	fract->numerator = 1000000 / factor;
> +	fract->denominator = sense / factor;
>  
> -	factor = gcd(rescale->numerator, gain_mult);
> -	rescale->numerator /= factor;
> -	rescale->denominator *= gain_mult / factor;
> +	factor = gcd(fract->numerator, gain_mult);
> +	fract->numerator /= factor;
> +	fract->denominator *= gain_mult / factor;
>  
> -	factor = gcd(rescale->denominator, gain_div);
> -	rescale->numerator *= gain_div / factor;
> -	rescale->denominator /= factor;
> +	factor = gcd(fract->denominator, gain_div);
> +	fract->numerator *= gain_div / factor;
> +	fract->denominator /= factor;
>  
>  	return 0;
>  }
>  
>  static int rescale_current_sense_shunt_props(struct device *dev,
> -					     struct rescale *rescale)
> +					     struct s32_fract *fract)
>  {
>  	u32 shunt;
>  	u32 factor;
> @@ -228,35 +229,33 @@ static int rescale_current_sense_shunt_props(struct device *dev,
>  	}
>  
>  	factor = gcd(shunt, 1000000);
> -	rescale->numerator = 1000000 / factor;
> -	rescale->denominator = shunt / factor;
> +	fract->numerator = 1000000 / factor;
> +	fract->denominator = shunt / factor;
>  
>  	return 0;
>  }
>  
>  static int rescale_voltage_divider_props(struct device *dev,
> -					 struct rescale *rescale)
> +					 struct s32_fract *fract)
>  {
>  	int ret;
>  	u32 factor;
>  
> -	ret = device_property_read_u32(dev, "output-ohms",
> -				       &rescale->denominator);
> +	ret = device_property_read_u32(dev, "output-ohms", &fract->denominator);
>  	if (ret) {
>  		dev_err(dev, "failed to read output-ohms: %d\n", ret);
>  		return ret;
>  	}
>  
> -	ret = device_property_read_u32(dev, "full-ohms",
> -				       &rescale->numerator);
> +	ret = device_property_read_u32(dev, "full-ohms", &fract->numerator);
>  	if (ret) {
>  		dev_err(dev, "failed to read full-ohms: %d\n", ret);
>  		return ret;
>  	}
>  
> -	factor = gcd(rescale->numerator, rescale->denominator);
> -	rescale->numerator /= factor;
> -	rescale->denominator /= factor;
> +	factor = gcd(fract->numerator, fract->denominator);
> +	fract->numerator /= factor;
> +	fract->denominator /= factor;
>  
>  	return 0;
>  }
> @@ -299,6 +298,7 @@ static int rescale_probe(struct platform_device *pdev)
>  	struct iio_dev *indio_dev;
>  	struct iio_channel *source;
>  	struct rescale *rescale;
> +	struct s32_fract *fract;
>  	int sizeof_ext_info;
>  	int sizeof_priv;
>  	int i;
> @@ -322,24 +322,24 @@ static int rescale_probe(struct platform_device *pdev)
>  		return -ENOMEM;
>  
>  	rescale = iio_priv(indio_dev);
> -
> +	rescale->source = source;

There seems to be more reorganizing going on in here than is necessary
for the function of this patch. At very least, description should
call it out.  Why move setting source?

>  	rescale->cfg = of_device_get_match_data(dev);
> -	rescale->numerator = 1;
> -	rescale->denominator = 1;
>  
> -	ret = rescale->cfg->props(dev, rescale);
> +	fract = &rescale->fract;
> +	fract->numerator = 1;
> +	fract->denominator = 1;
> +
> +	ret = rescale->cfg->props(dev, fract);
>  	if (ret)
>  		return ret;
>  
> -	if (!rescale->numerator || !rescale->denominator) {
> +	if (!fract->numerator || !fract->denominator) {
>  		dev_err(dev, "invalid scaling factor.\n");
>  		return -EINVAL;
>  	}
>  
>  	platform_set_drvdata(pdev, indio_dev);
>  
> -	rescale->source = source;
> -
>  	indio_dev->name = dev_name(dev);
>  	indio_dev->info = &rescale_info;
>  	indio_dev->modes = INDIO_DIRECT_MODE;