[PATCH v1 0/4] iio: afe: rescale: A few cleanups

[PATCH v1 0/4] iio: afe: rescale: A few cleanups

[Andy Shevchenko]

There are cleanups to the code, most of the + LoCs due to missed inclusions.

The patch 3 is kinda interesting as in long term the idea to add multiplication
and division macros over the fract structures, which will simplify code that
uses rational numbers. Hence the mentioned patch is just a preparatory for making
next step easier.

Andy Shevchenko (4):
  iio: afe: rescale: Don't use ^ for booleans
  iio: afe: rescale: Don't use ULL(1) << x instead of BIT(x)
  iio: afe: rescale: Re-use generic struct s32_fract
  iio: afe: rescale: Don't use "proxy" headers

 drivers/iio/afe/iio-rescale.c   | 88 ++++++++++++++++++---------------
 include/linux/iio/afe/rescale.h |  5 +-
 2 files changed, 51 insertions(+), 42 deletions(-)

-- 
2.43.0.rc1.1336.g36b5255a03ac

[PATCH v1 1/4] iio: afe: rescale: Don't use ^ for booleans

[Andy Shevchenko]

There are two (non-critical) issues with the code. First of all,
the eXclusive OR is not defined for booleans, so boolean to integer
promotion is required, Second, the u32 variable is used to keep
boolean value, so boolean is converted implicitly to the integer.

All these are not needed when variable is defined as boolean to begin
with and operations are replaced by simple != and ||.

Fixes: 701ee14da95d ("iio: afe: rescale: add INT_PLUS_{MICRO,NANO} support")
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/iio/afe/iio-rescale.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c
index b6a46036d5ea..470dd7d41b2a 100644
--- a/drivers/iio/afe/iio-rescale.c
+++ b/drivers/iio/afe/iio-rescale.c
@@ -26,7 +26,7 @@ int rescale_process_scale(struct rescale *rescale, int scale_type,
 	int _val, _val2;
 	s32 rem, rem2;
 	u32 mult;
-	u32 neg;
+	bool neg;
 
 	switch (scale_type) {
 	case IIO_VAL_INT:
@@ -95,7 +95,7 @@ int rescale_process_scale(struct rescale *rescale, int scale_type,
 		 * If only one of the rescaler elements or the schan scale is
 		 * negative, the combined scale is negative.
 		 */
-		if (neg ^ ((rescale->numerator < 0) ^ (rescale->denominator < 0))) {
+		if (neg != (rescale->numerator < 0 || rescale->denominator < 0)) {
 			if (*val)
 				*val = -*val;
 			else
-- 
2.43.0.rc1.1336.g36b5255a03ac

[PATCH v1 2/4] iio: afe: rescale: Don't use ULL(1) << x instead of BIT(x)

[Andy Shevchenko]

ULL(1) << x is just an open-coded implementation of BIT_ULL().
Replace the former by the latter.

Note, the rest of the code properly uses BIT()/BIT_ULL() already.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/iio/afe/iio-rescale.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c
index 470dd7d41b2a..9d33e7aabe4d 100644
--- a/drivers/iio/afe/iio-rescale.c
+++ b/drivers/iio/afe/iio-rescale.c
@@ -8,6 +8,7 @@
  * Author: Peter Rosin <peda@axentia.se>
  */
 
+#include <linux/bits.h>
 #include <linux/err.h>
 #include <linux/gcd.h>
 #include <linux/mod_devicetable.h>
@@ -62,7 +63,7 @@ int rescale_process_scale(struct rescale *rescale, int scale_type,
 		if (scale_type == IIO_VAL_FRACTIONAL)
 			tmp = *val2;
 		else
-			tmp = ULL(1) << *val2;
+			tmp = BIT_ULL(*val2);
 
 		rem2 = *val % (int)tmp;
 		*val = *val / (int)tmp;
-- 
2.43.0.rc1.1336.g36b5255a03ac

[PATCH v1 3/4] iio: afe: rescale: Re-use generic struct s32_fract

[Andy Shevchenko]

Instead of custom data type re-use generic struct s32_fract.
No changes intended.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/iio/afe/iio-rescale.c   | 79 +++++++++++++++++----------------
 include/linux/iio/afe/rescale.h |  5 ++-
 2 files changed, 44 insertions(+), 40 deletions(-)

diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c
index 9d33e7aabe4d..bbc73798082d 100644
--- a/drivers/iio/afe/iio-rescale.c
+++ b/drivers/iio/afe/iio-rescale.c
@@ -23,6 +23,7 @@
 int rescale_process_scale(struct rescale *rescale, int scale_type,
 			  int *val, int *val2)
 {
+	struct s32_fract *fract = &rescale->fract;
 	s64 tmp;
 	int _val, _val2;
 	s32 rem, rem2;
@@ -31,10 +32,10 @@ int rescale_process_scale(struct rescale *rescale, int scale_type,
 
 	switch (scale_type) {
 	case IIO_VAL_INT:
-		*val *= rescale->numerator;
-		if (rescale->denominator == 1)
+		*val *= fract->numerator;
+		if (fract->denominator == 1)
 			return scale_type;
-		*val2 = rescale->denominator;
+		*val2 = fract->denominator;
 		return IIO_VAL_FRACTIONAL;
 	case IIO_VAL_FRACTIONAL:
 		/*
@@ -42,8 +43,8 @@ int rescale_process_scale(struct rescale *rescale, int scale_type,
 		 * potential accuracy loss (for in kernel consumers) by
 		 * keeping a fractional representation.
 		 */
-		if (!check_mul_overflow(*val, rescale->numerator, &_val) &&
-		    !check_mul_overflow(*val2, rescale->denominator, &_val2)) {
+		if (!check_mul_overflow(*val, fract->numerator, &_val) &&
+		    !check_mul_overflow(*val2, fract->denominator, &_val2)) {
 			*val = _val;
 			*val2 = _val2;
 			return IIO_VAL_FRACTIONAL;
@@ -51,8 +52,8 @@ int rescale_process_scale(struct rescale *rescale, int scale_type,
 		fallthrough;
 	case IIO_VAL_FRACTIONAL_LOG2:
 		tmp = (s64)*val * 1000000000LL;
-		tmp = div_s64(tmp, rescale->denominator);
-		tmp *= rescale->numerator;
+		tmp = div_s64(tmp, fract->denominator);
+		tmp *= fract->numerator;
 
 		tmp = div_s64_rem(tmp, 1000000000LL, &rem);
 		*val = tmp;
@@ -84,11 +85,11 @@ int rescale_process_scale(struct rescale *rescale, int scale_type,
 		 */
 		neg = *val < 0 || *val2 < 0;
 
-		tmp = (s64)abs(*val) * abs(rescale->numerator);
-		*val = div_s64_rem(tmp, abs(rescale->denominator), &rem);
+		tmp = (s64)abs(*val) * abs(fract->numerator);
+		*val = div_s64_rem(tmp, abs(fract->denominator), &rem);
 
-		tmp = (s64)rem * mult + (s64)abs(*val2) * abs(rescale->numerator);
-		tmp = div_s64(tmp, abs(rescale->denominator));
+		tmp = (s64)rem * mult + (s64)abs(*val2) * abs(fract->numerator);
+		tmp = div_s64(tmp, abs(fract->denominator));
 
 		*val += div_s64_rem(tmp, mult, val2);
 
@@ -96,7 +97,7 @@ int rescale_process_scale(struct rescale *rescale, int scale_type,
 		 * If only one of the rescaler elements or the schan scale is
 		 * negative, the combined scale is negative.
 		 */
-		if (neg != (rescale->numerator < 0 || rescale->denominator < 0)) {
+		if (neg != (fract->numerator < 0 || fract->denominator < 0)) {
 			if (*val)
 				*val = -*val;
 			else
@@ -323,6 +324,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 = &rescale->fract;
 	u32 sense;
 	u32 gain_mult = 1;
 	u32 gain_div = 1;
@@ -345,16 +347,16 @@ 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;
 }
@@ -362,6 +364,7 @@ static int rescale_current_sense_amplifier_props(struct device *dev,
 static int rescale_current_sense_shunt_props(struct device *dev,
 					     struct rescale *rescale)
 {
+	struct s32_fract *fract = &rescale->fract;
 	u32 shunt;
 	u32 factor;
 	int ret;
@@ -374,8 +377,8 @@ 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;
 }
@@ -383,26 +386,25 @@ static int rescale_current_sense_shunt_props(struct device *dev,
 static int rescale_voltage_divider_props(struct device *dev,
 					 struct rescale *rescale)
 {
+	struct s32_fract *fract = &rescale->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;
 }
@@ -410,6 +412,7 @@ static int rescale_voltage_divider_props(struct device *dev,
 static int rescale_temp_sense_rtd_props(struct device *dev,
 					struct rescale *rescale)
 {
+	struct s32_fract *fract = &rescale->fract;
 	u32 factor;
 	u32 alpha;
 	u32 iexc;
@@ -440,8 +443,8 @@ static int rescale_temp_sense_rtd_props(struct device *dev,
 
 	tmp = r0 * iexc * alpha / 1000000;
 	factor = gcd(tmp, 1000000);
-	rescale->numerator = 1000000 / factor;
-	rescale->denominator = tmp / factor;
+	fract->numerator = 1000000 / factor;
+	fract->denominator = tmp / factor;
 
 	rescale->offset = -1 * ((r0 * iexc) / 1000);
 
@@ -451,6 +454,7 @@ static int rescale_temp_sense_rtd_props(struct device *dev,
 static int rescale_temp_transducer_props(struct device *dev,
 					 struct rescale *rescale)
 {
+	struct s32_fract *fract = &rescale->fract;
 	s32 offset = 0;
 	s32 sense = 1;
 	s32 alpha;
@@ -464,11 +468,10 @@ static int rescale_temp_transducer_props(struct device *dev,
 		return ret;
 	}
 
-	rescale->numerator = 1000000;
-	rescale->denominator = alpha * sense;
+	fract->numerator = 1000000;
+	fract->denominator = alpha * sense;
 
-	rescale->offset = div_s64((s64)offset * rescale->denominator,
-				  rescale->numerator);
+	rescale->offset = div_s64((s64)offset * fract->denominator, fract->numerator);
 
 	return 0;
 }
@@ -550,15 +553,15 @@ static int rescale_probe(struct platform_device *pdev)
 	rescale = iio_priv(indio_dev);
 
 	rescale->cfg = device_get_match_data(dev);
-	rescale->numerator = 1;
-	rescale->denominator = 1;
+	rescale->fract.numerator = 1;
+	rescale->fract.denominator = 1;
 	rescale->offset = 0;
 
 	ret = rescale->cfg->props(dev, rescale);
 	if (ret)
 		return ret;
 
-	if (!rescale->numerator || !rescale->denominator) {
+	if (!rescale->fract.numerator || !rescale->fract.denominator) {
 		dev_err(dev, "invalid scaling factor.\n");
 		return -EINVAL;
 	}
diff --git a/include/linux/iio/afe/rescale.h b/include/linux/iio/afe/rescale.h
index 6eecb435488f..d6998806b4f5 100644
--- a/include/linux/iio/afe/rescale.h
+++ b/include/linux/iio/afe/rescale.h
@@ -6,7 +6,9 @@
 #ifndef __IIO_RESCALE_H__
 #define __IIO_RESCALE_H__
 
+#include <linux/math.h>
 #include <linux/types.h>
+
 #include <linux/iio/iio.h>
 
 struct device;
@@ -19,12 +21,11 @@ struct rescale_cfg {
 
 struct rescale {
 	const struct rescale_cfg *cfg;
+	struct s32_fract fract;
 	struct iio_channel *source;
 	struct iio_chan_spec chan;
 	struct iio_chan_spec_ext_info *ext_info;
 	bool chan_processed;
-	s32 numerator;
-	s32 denominator;
 	s32 offset;
 };
 
-- 
2.43.0.rc1.1336.g36b5255a03ac

[PATCH v1 4/4] iio: afe: rescale: Don't use "proxy" headers

[Andy Shevchenko]

Update header inclusions to follow IWYU (Include What You Use)
principle.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/iio/afe/iio-rescale.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c
index bbc73798082d..067823a356d9 100644
--- a/drivers/iio/afe/iio-rescale.c
+++ b/drivers/iio/afe/iio-rescale.c
@@ -9,12 +9,16 @@
  */
 
 #include <linux/bits.h>
+#include <linux/dev_printk.h>
 #include <linux/err.h>
 #include <linux/gcd.h>
+#include <linux/math64.h>
 #include <linux/mod_devicetable.h>
 #include <linux/module.h>
+#include <linux/overflow.h>
 #include <linux/platform_device.h>
 #include <linux/property.h>
+#include <linux/types.h>
 
 #include <linux/iio/afe/rescale.h>
 #include <linux/iio/consumer.h>
-- 
2.43.0.rc1.1336.g36b5255a03ac

Re: [PATCH v1 3/4] iio: afe: rescale: Re-use generic struct s32_fract

[kernel test robot]

Hi Andy,

kernel test robot noticed the following build errors:

[auto build test ERROR on jic23-iio/togreg]
[also build test ERROR on linus/master v6.13-rc1 next-20241203]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Andy-Shevchenko/iio-afe-rescale-Don-t-use-for-booleans/20241204-124353
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
patch link:    https://lore.kernel.org/r/20241204013620.862943-4-andriy.shevchenko%40linux.intel.com
patch subject: [PATCH v1 3/4] iio: afe: rescale: Re-use generic struct s32_fract
config: arm64-randconfig-001 (https://download.01.org/0day-ci/archive/20241204/202412041825.tYQkmq7d-lkp@intel.com/config)
compiler: aarch64-linux-gcc (GCC) 14.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241204/202412041825.tYQkmq7d-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202412041825.tYQkmq7d-lkp@intel.com/

All errors (new ones prefixed by >>):

   drivers/iio/test/iio-test-rescale.c: In function 'iio_rescale_test_scale':
>> drivers/iio/test/iio-test-rescale.c:655:16: error: 'struct rescale' has no member named 'numerator'
     655 |         rescale.numerator = t->numerator;
         |                ^
>> drivers/iio/test/iio-test-rescale.c:656:16: error: 'struct rescale' has no member named 'denominator'
     656 |         rescale.denominator = t->denominator;
         |                ^
   drivers/iio/test/iio-test-rescale.c: In function 'iio_rescale_test_offset':
   drivers/iio/test/iio-test-rescale.c:684:16: error: 'struct rescale' has no member named 'numerator'
     684 |         rescale.numerator = t->numerator;
         |                ^
   drivers/iio/test/iio-test-rescale.c:685:16: error: 'struct rescale' has no member named 'denominator'
     685 |         rescale.denominator = t->denominator;
         |                ^


vim +655 drivers/iio/test/iio-test-rescale.c

8e74a48d17d509b Liam Beguin 2022-02-12  645  
8e74a48d17d509b Liam Beguin 2022-02-12  646  static void iio_rescale_test_scale(struct kunit *test)
8e74a48d17d509b Liam Beguin 2022-02-12  647  {
8e74a48d17d509b Liam Beguin 2022-02-12  648  	struct rescale_tc_data *t = (struct rescale_tc_data *)test->param_value;
8e74a48d17d509b Liam Beguin 2022-02-12  649  	char *buff = kunit_kmalloc(test, PAGE_SIZE, GFP_KERNEL);
8e74a48d17d509b Liam Beguin 2022-02-12  650  	struct rescale rescale;
8e74a48d17d509b Liam Beguin 2022-02-12  651  	int values[2];
8e74a48d17d509b Liam Beguin 2022-02-12  652  	int rel_ppm;
8e74a48d17d509b Liam Beguin 2022-02-12  653  	int ret;
8e74a48d17d509b Liam Beguin 2022-02-12  654  
8e74a48d17d509b Liam Beguin 2022-02-12 @655  	rescale.numerator = t->numerator;
8e74a48d17d509b Liam Beguin 2022-02-12 @656  	rescale.denominator = t->denominator;
8e74a48d17d509b Liam Beguin 2022-02-12  657  	rescale.offset = t->offset;
8e74a48d17d509b Liam Beguin 2022-02-12  658  	values[0] = t->schan_val;
8e74a48d17d509b Liam Beguin 2022-02-12  659  	values[1] = t->schan_val2;
8e74a48d17d509b Liam Beguin 2022-02-12  660  
8e74a48d17d509b Liam Beguin 2022-02-12  661  	ret = rescale_process_scale(&rescale, t->schan_scale_type,
8e74a48d17d509b Liam Beguin 2022-02-12  662  				    &values[0], &values[1]);
8e74a48d17d509b Liam Beguin 2022-02-12  663  
8e74a48d17d509b Liam Beguin 2022-02-12  664  	ret = iio_format_value(buff, ret, 2, values);
8e74a48d17d509b Liam Beguin 2022-02-12  665  	KUNIT_EXPECT_EQ(test, (int)strlen(buff), ret);
8e74a48d17d509b Liam Beguin 2022-02-12  666  
8e74a48d17d509b Liam Beguin 2022-02-12  667  	rel_ppm = iio_test_relative_error_ppm(buff, t->expected);
8e74a48d17d509b Liam Beguin 2022-02-12  668  	KUNIT_EXPECT_GE_MSG(test, rel_ppm, 0, "failed to compute ppm\n");
8e74a48d17d509b Liam Beguin 2022-02-12  669  
8e74a48d17d509b Liam Beguin 2022-02-12  670  	KUNIT_EXPECT_EQ_MSG(test, rel_ppm, 0,
8e74a48d17d509b Liam Beguin 2022-02-12  671  			    "\t    real=%s"
8e74a48d17d509b Liam Beguin 2022-02-12  672  			    "\texpected=%s\n",
8e74a48d17d509b Liam Beguin 2022-02-12  673  			    buff, t->expected);
8e74a48d17d509b Liam Beguin 2022-02-12  674  }
8e74a48d17d509b Liam Beguin 2022-02-12  675  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v1 3/4] iio: afe: rescale: Re-use generic struct s32_fract

[kernel test robot]

Hi Andy,

kernel test robot noticed the following build errors:

[auto build test ERROR on jic23-iio/togreg]
[also build test ERROR on linus/master v6.13-rc1 next-20241203]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Andy-Shevchenko/iio-afe-rescale-Don-t-use-for-booleans/20241204-124353
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
patch link:    https://lore.kernel.org/r/20241204013620.862943-4-andriy.shevchenko%40linux.intel.com
patch subject: [PATCH v1 3/4] iio: afe: rescale: Re-use generic struct s32_fract
config: arm-randconfig-003 (https://download.01.org/0day-ci/archive/20241204/202412041908.UaZf89I0-lkp@intel.com/config)
compiler: clang version 20.0.0git (https://github.com/llvm/llvm-project 592c0fe55f6d9a811028b5f3507be91458ab2713)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241204/202412041908.UaZf89I0-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202412041908.UaZf89I0-lkp@intel.com/

All errors (new ones prefixed by >>):

>> drivers/iio/test/iio-test-rescale.c:655:10: error: no member named 'numerator' in 'struct rescale'
     655 |         rescale.numerator = t->numerator;
         |         ~~~~~~~ ^
>> drivers/iio/test/iio-test-rescale.c:656:10: error: no member named 'denominator' in 'struct rescale'
     656 |         rescale.denominator = t->denominator;
         |         ~~~~~~~ ^
   drivers/iio/test/iio-test-rescale.c:684:10: error: no member named 'numerator' in 'struct rescale'
     684 |         rescale.numerator = t->numerator;
         |         ~~~~~~~ ^
   drivers/iio/test/iio-test-rescale.c:685:10: error: no member named 'denominator' in 'struct rescale'
     685 |         rescale.denominator = t->denominator;
         |         ~~~~~~~ ^
   4 errors generated.


vim +655 drivers/iio/test/iio-test-rescale.c

8e74a48d17d509 Liam Beguin 2022-02-12  645  
8e74a48d17d509 Liam Beguin 2022-02-12  646  static void iio_rescale_test_scale(struct kunit *test)
8e74a48d17d509 Liam Beguin 2022-02-12  647  {
8e74a48d17d509 Liam Beguin 2022-02-12  648  	struct rescale_tc_data *t = (struct rescale_tc_data *)test->param_value;
8e74a48d17d509 Liam Beguin 2022-02-12  649  	char *buff = kunit_kmalloc(test, PAGE_SIZE, GFP_KERNEL);
8e74a48d17d509 Liam Beguin 2022-02-12  650  	struct rescale rescale;
8e74a48d17d509 Liam Beguin 2022-02-12  651  	int values[2];
8e74a48d17d509 Liam Beguin 2022-02-12  652  	int rel_ppm;
8e74a48d17d509 Liam Beguin 2022-02-12  653  	int ret;
8e74a48d17d509 Liam Beguin 2022-02-12  654  
8e74a48d17d509 Liam Beguin 2022-02-12 @655  	rescale.numerator = t->numerator;
8e74a48d17d509 Liam Beguin 2022-02-12 @656  	rescale.denominator = t->denominator;
8e74a48d17d509 Liam Beguin 2022-02-12  657  	rescale.offset = t->offset;
8e74a48d17d509 Liam Beguin 2022-02-12  658  	values[0] = t->schan_val;
8e74a48d17d509 Liam Beguin 2022-02-12  659  	values[1] = t->schan_val2;
8e74a48d17d509 Liam Beguin 2022-02-12  660  
8e74a48d17d509 Liam Beguin 2022-02-12  661  	ret = rescale_process_scale(&rescale, t->schan_scale_type,
8e74a48d17d509 Liam Beguin 2022-02-12  662  				    &values[0], &values[1]);
8e74a48d17d509 Liam Beguin 2022-02-12  663  
8e74a48d17d509 Liam Beguin 2022-02-12  664  	ret = iio_format_value(buff, ret, 2, values);
8e74a48d17d509 Liam Beguin 2022-02-12  665  	KUNIT_EXPECT_EQ(test, (int)strlen(buff), ret);
8e74a48d17d509 Liam Beguin 2022-02-12  666  
8e74a48d17d509 Liam Beguin 2022-02-12  667  	rel_ppm = iio_test_relative_error_ppm(buff, t->expected);
8e74a48d17d509 Liam Beguin 2022-02-12  668  	KUNIT_EXPECT_GE_MSG(test, rel_ppm, 0, "failed to compute ppm\n");
8e74a48d17d509 Liam Beguin 2022-02-12  669  
8e74a48d17d509 Liam Beguin 2022-02-12  670  	KUNIT_EXPECT_EQ_MSG(test, rel_ppm, 0,
8e74a48d17d509 Liam Beguin 2022-02-12  671  			    "\t    real=%s"
8e74a48d17d509 Liam Beguin 2022-02-12  672  			    "\texpected=%s\n",
8e74a48d17d509 Liam Beguin 2022-02-12  673  			    buff, t->expected);
8e74a48d17d509 Liam Beguin 2022-02-12  674  }
8e74a48d17d509 Liam Beguin 2022-02-12  675  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

RE: [PATCH v1 1/4] iio: afe: rescale: Don't use ^ for booleans

[David Laight]

From: Andy Shevchenko
> Sent: 04 December 2024 01:33
> 
> There are two (non-critical) issues with the code. First of all,
> the eXclusive OR is not defined for booleans, so boolean to integer
> promotion is required, Second, the u32 variable is used to keep
> boolean value, so boolean is converted implicitly to the integer.

Except there is no such thing as 'boolean' they are all integers.
And the compiler has to have some set of rules to handle the cases
where the memory that hold the 'boolean' doesn't have the value 0 or 1.

> 
> All these are not needed when variable is defined as boolean to begin
> with and operations are replaced by simple != and ||.
> 
> Fixes: 701ee14da95d ("iio: afe: rescale: add INT_PLUS_{MICRO,NANO} support")
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/iio/afe/iio-rescale.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c
> index b6a46036d5ea..470dd7d41b2a 100644
> --- a/drivers/iio/afe/iio-rescale.c
> +++ b/drivers/iio/afe/iio-rescale.c
> @@ -26,7 +26,7 @@ int rescale_process_scale(struct rescale *rescale, int scale_type,
>  	int _val, _val2;
>  	s32 rem, rem2;
>  	u32 mult;
> -	u32 neg;
> +	bool neg;
> 
>  	switch (scale_type) {
>  	case IIO_VAL_INT:
> @@ -95,7 +95,7 @@ int rescale_process_scale(struct rescale *rescale, int scale_type,
>  		 * If only one of the rescaler elements or the schan scale is
>  		 * negative, the combined scale is negative.
>  		 */
> -		if (neg ^ ((rescale->numerator < 0) ^ (rescale->denominator < 0))) {
> +		if (neg != (rescale->numerator < 0 || rescale->denominator < 0)) {

That is wrong, the || would also need to be !=.
Which will all generate real pile of horrid code.
(I think the x86 version will stun you.)

I'm guessing that somewhere there is a:
	neg = value < 0;

Provided all the values are the same size (eg int/s32), in which case:
	neg = value;
...
	if ((neg ^ rescale->numerator ^ rescale->denominator) < 0)
will be the desired test.

	David

>  			if (*val)
>  				*val = -*val;
>  			else
> --
> 2.43.0.rc1.1336.g36b5255a03ac
> 

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH v1 1/4] iio: afe: rescale: Don't use ^ for booleans

['Andy Shevchenko']

On Fri, Dec 06, 2024 at 01:24:09PM +0000, David Laight wrote:
> From: Andy Shevchenko
> > Sent: 04 December 2024 01:33
> > 
> > There are two (non-critical) issues with the code. First of all,
> > the eXclusive OR is not defined for booleans, so boolean to integer
> > promotion is required, Second, the u32 variable is used to keep
> > boolean value, so boolean is converted implicitly to the integer.
> 
> Except there is no such thing as 'boolean' they are all integers.

I believe this is an exercise in linguistics as I'm not native speaker
but I am very well aware of the promotions to the integer values.

> And the compiler has to have some set of rules to handle the cases
> where the memory that hold the 'boolean' doesn't have the value 0 or 1.

No doubts.

...

> >  		 * If only one of the rescaler elements or the schan scale is
> >  		 * negative, the combined scale is negative.
> >  		 */
> > -		if (neg ^ ((rescale->numerator < 0) ^ (rescale->denominator < 0))) {
> > +		if (neg != (rescale->numerator < 0 || rescale->denominator < 0)) {
> 
> That is wrong, the || would also need to be !=.

Why do you think so? Maybe it's comment(s) that is(are) wrong?

> Which will all generate real pile of horrid code.
> (I think the x86 version will stun you.)

I think your remark is based on something, can you show the output to elaborate
what exactly becomes horrible in this case?

> I'm guessing that somewhere there is a:
> 	neg = value < 0;

Nope.

> Provided all the values are the same size (eg int/s32), in which case:
> 	neg = value;
> ...
> 	if ((neg ^ rescale->numerator ^ rescale->denominator) < 0)
> will be the desired test.

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH v1 1/4] iio: afe: rescale: Don't use ^ for booleans

[David Laight]

From: 'Andy Shevchenko'
> Sent: 06 December 2024 15:20
...
> ...
> 
> > >  		 * If only one of the rescaler elements or the schan scale is
> > >  		 * negative, the combined scale is negative.
> > >  		 */
> > > -		if (neg ^ ((rescale->numerator < 0) ^ (rescale->denominator < 0))) {
> > > +		if (neg != (rescale->numerator < 0 || rescale->denominator < 0)) {
> >
> > That is wrong, the || would also need to be !=.
> 
> Why do you think so? Maybe it's comment(s) that is(are) wrong?

The old code certainly negates for each of them - so you can get the double
and triple negative cases.
So believe the code not the comment.
> 
> > Which will all generate real pile of horrid code.
> > (I think the x86 version will stun you.)
> 
> I think your remark is based on something, can you show the output to elaborate
> what exactly becomes horrible in this case?

Ok it isn't quite as bad as I thought because all the compilers will use xor
for the equality test on sign bits. See https://www.godbolt.org/z/qxz5KPcTh
So changing ^ to != makes no difference at all.

But f3() shows the sort of thing that can happen.
Sometimes made worse because the x86 SETcc instruction only set 8bit registers.
(Odd since they got added for the 386)

	David

> 
> > I'm guessing that somewhere there is a:
> > 	neg = value < 0;
> 
> Nope.
> 
> > Provided all the values are the same size (eg int/s32), in which case:
> > 	neg = value;
> > ...
> > 	if ((neg ^ rescale->numerator ^ rescale->denominator) < 0)
> > will be the desired test.
> 
> --
> With Best Regards,
> Andy Shevchenko
> 

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH v1 1/4] iio: afe: rescale: Don't use ^ for booleans

[Peter Rosin]

Hi!

2024-12-06 at 21:13, David Laight wrote:
> From: 'Andy Shevchenko'
>> Sent: 06 December 2024 15:20
> ...
>> ...
>>
>>>>  		 * If only one of the rescaler elements or the schan scale is
>>>>  		 * negative, the combined scale is negative.
>>>>  		 */
>>>> -		if (neg ^ ((rescale->numerator < 0) ^ (rescale->denominator < 0))) {
>>>> +		if (neg != (rescale->numerator < 0 || rescale->denominator < 0)) {
>>>
>>> That is wrong, the || would also need to be !=.
>>
>> Why do you think so? Maybe it's comment(s) that is(are) wrong?
> 
> The old code certainly negates for each of them - so you can get the double
> and triple negative cases.

Indeed. And for me, xor is the natural operator for getting to such
"oddness" when three or more values needs to be considered. So, I
prefer to keep the code as is since a ^ b ^ c is nice and succinct,
while anything you try to write using != is going to be convoluted
when you have three or more values.

> So believe the code not the comment.

I claim that the comment is correct. Or at least not incorrect. It might
not be complete, but what it does state holds. It does not spell out that
the combined scale is positive if both of the rescaler elements and the
schan scale are positive. It does not spell out that the combined scale
is negative if all three are negative. What it does give you though, is
a hint that the whole if () is all about the sign of the combined scale.

But yes, the comment could be improved.

I expect a fail from this test in iio-test-rescale.c with the new code

	{
		.name = "negative IIO_VAL_INT_PLUS_NANO, 3 negative",
		.numerator = -1000000,
		.denominator = -8060,
		.schan_scale_type = IIO_VAL_INT_PLUS_NANO,
		.schan_val = -10,
		.schan_val2 = 123456,
		.expected = "-1240.710106203",
	},

but the 0-day has been silent. I wonder why? Does it not actually
run the tests?

There could also be some more tests to try make sure the logic doesn't
regress. The first of these should also fail with this patch, while
the second should be ok.

	{
		.name = "positive IIO_VAL_INT_PLUS_MICRO, 2 negative",
		.numerator = -1,
		.denominator = -2,
		.schan_scale_type = IIO_VAL_INT_PLUS_MICRO,
		.schan_val = 5,
		.schan_val2 = 1234,
		.expected = "2.500617",
	},
	{
		.name = "positive IIO_VAL_INT_PLUS_MICRO, 2 negative",
		.numerator = 1,
		.denominator = -2,
		.schan_scale_type = IIO_VAL_INT_PLUS_MICRO,
		.schan_val = -5,
		.schan_val2 = 1234,
		.expected = "2.500617",
	},

Cheers,
Peter