[PATCH v2 1/5] math.h: Introduce data types for fractional numbers

[PATCH v2 1/5] math.h: Introduce data types for fractional numbers

[Andy Shevchenko]

Introduce a macro to produce data types like

	struct TYPE_fract {
		__TYPE numerator;
		__TYPE denominator;
	};

to be used in the code wherever it's needed.

In the following changes convert some users to it.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---

May be pulled via IIO tree.

v2: no changes

 include/linux/math.h | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/include/linux/math.h b/include/linux/math.h
index 53674a327e39..439b8f0b9ebd 100644
--- a/include/linux/math.h
+++ b/include/linux/math.h
@@ -2,6 +2,7 @@
 #ifndef _LINUX_MATH_H
 #define _LINUX_MATH_H
 
+#include <linux/types.h>
 #include <asm/div64.h>
 #include <uapi/linux/kernel.h>
 
@@ -106,6 +107,17 @@
 }							\
 )
 
+#define __STRUCT_FRACT(type)				\
+struct type##_fract {					\
+	__##type numerator;				\
+	__##type denominator;				\
+};
+__STRUCT_FRACT(s16)
+__STRUCT_FRACT(u16)
+__STRUCT_FRACT(s32)
+__STRUCT_FRACT(u32)
+#undef __STRUCT_FRACT
+
 /*
  * Multiplies an integer by a fraction, while avoiding unnecessary
  * overflow or loss of precision.
-- 
2.34.1

[PATCH v2 2/5] iio: adc: rn5t618: Re-use generic struct u16_fract

[Andy Shevchenko]

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

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---

v2: no changes

 drivers/iio/adc/rn5t618-adc.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/iio/adc/rn5t618-adc.c b/drivers/iio/adc/rn5t618-adc.c
index 7d891b4ea461..6bf32907f01d 100644
--- a/drivers/iio/adc/rn5t618-adc.c
+++ b/drivers/iio/adc/rn5t618-adc.c
@@ -42,11 +42,6 @@ struct rn5t618_adc_data {
 	int irq;
 };
 
-struct rn5t618_channel_ratios {
-	u16 numerator;
-	u16 denominator;
-};
-
 enum rn5t618_channels {
 	LIMMON = 0,
 	VBAT,
@@ -58,7 +53,7 @@ enum rn5t618_channels {
 	AIN0
 };
 
-static const struct rn5t618_channel_ratios rn5t618_ratios[8] = {
+static const struct u16_fract rn5t618_ratios[8] = {
 	[LIMMON] = {50, 32}, /* measured across 20mOhm, amplified by 32 */
 	[VBAT] = {2, 1},
 	[VADP] = {3, 1},
-- 
2.34.1

[PATCH v2 3/5] iio: adc: twl4030-madc: Re-use generic struct s16_fract

[Andy Shevchenko]

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

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---

v2: no changes

 drivers/iio/adc/twl4030-madc.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/drivers/iio/adc/twl4030-madc.c b/drivers/iio/adc/twl4030-madc.c
index 6ce40cc4568a..f8f8aea15612 100644
--- a/drivers/iio/adc/twl4030-madc.c
+++ b/drivers/iio/adc/twl4030-madc.c
@@ -231,13 +231,7 @@ static const struct iio_chan_spec twl4030_madc_iio_channels[] = {
 
 static struct twl4030_madc_data *twl4030_madc;
 
-struct twl4030_prescale_divider_ratios {
-	s16 numerator;
-	s16 denominator;
-};
-
-static const struct twl4030_prescale_divider_ratios
-twl4030_divider_ratios[16] = {
+static const struct s16_fract twl4030_divider_ratios[16] = {
 	{1, 1},		/* CHANNEL 0 No Prescaler */
 	{1, 1},		/* CHANNEL 1 No Prescaler */
 	{6, 10},	/* CHANNEL 2 */
@@ -256,7 +250,6 @@ twl4030_divider_ratios[16] = {
 	{5, 11},	/* CHANNEL 15 */
 };
 
-
 /* Conversion table from -3 to 55 degrees Celcius */
 static int twl4030_therm_tbl[] = {
 	30800,	29500,	28300,	27100,
-- 
2.34.1

[PATCH v2 4/5] iio: adc: qcom-vadc-common: Re-use generic struct u32_fract

[Andy Shevchenko]

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

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---

v2: comile-tested, i.o.w. fixed compilation errors

 drivers/iio/adc/qcom-pm8xxx-xoadc.c      | 15 ++--
 drivers/iio/adc/qcom-spmi-vadc.c         | 24 +++----
 drivers/iio/adc/qcom-vadc-common.c       | 92 ++++++++++++------------
 include/linux/iio/adc/qcom-vadc-common.h | 15 +---
 4 files changed, 69 insertions(+), 77 deletions(-)

diff --git a/drivers/iio/adc/qcom-pm8xxx-xoadc.c b/drivers/iio/adc/qcom-pm8xxx-xoadc.c
index 21d7eff645c3..5e9e56821075 100644
--- a/drivers/iio/adc/qcom-pm8xxx-xoadc.c
+++ b/drivers/iio/adc/qcom-pm8xxx-xoadc.c
@@ -175,7 +175,7 @@ struct xoadc_channel {
 	const char *datasheet_name;
 	u8 pre_scale_mux:2;
 	u8 amux_channel:4;
-	const struct vadc_prescale_ratio prescale;
+	const struct u32_fract prescale;
 	enum iio_chan_type type;
 	enum vadc_scale_fn_type scale_fn_type;
 	u8 amux_ip_rsv:3;
@@ -218,7 +218,9 @@ struct xoadc_variant {
 		.datasheet_name = __stringify(_dname),			\
 		.pre_scale_mux = _presmux,				\
 		.amux_channel = _amux,					\
-		.prescale = { .num = _prenum, .den = _preden },		\
+		.prescale = {						\
+			.numerator = _prenum, .denominator = _preden,	\
+		},							\
 		.type = _type,						\
 		.scale_fn_type = _scale,				\
 		.amux_ip_rsv = _amip,					\
@@ -809,12 +811,11 @@ static int pm8xxx_xoadc_parse_channel(struct device *dev,
 		BIT(IIO_CHAN_INFO_PROCESSED);
 	iio_chan->indexed = 1;
 
-	dev_dbg(dev, "channel [PRESCALE/MUX: %02x AMUX: %02x] \"%s\" "
-		"ref voltage: %d, decimation %d "
-		"prescale %d/%d, scale function %d\n",
+	dev_dbg(dev,
+		"channel [PRESCALE/MUX: %02x AMUX: %02x] \"%s\" ref voltage: %d, decimation %d prescale %d/%d, scale function %d\n",
 		hwchan->pre_scale_mux, hwchan->amux_channel, ch->name,
-		ch->amux_ip_rsv, ch->decimation, hwchan->prescale.num,
-		hwchan->prescale.den, hwchan->scale_fn_type);
+		ch->amux_ip_rsv, ch->decimation, hwchan->prescale.numerator,
+		hwchan->prescale.denominator, hwchan->scale_fn_type);
 
 	return 0;
 }
diff --git a/drivers/iio/adc/qcom-spmi-vadc.c b/drivers/iio/adc/qcom-spmi-vadc.c
index 07b1a99381d9..34202ba52469 100644
--- a/drivers/iio/adc/qcom-spmi-vadc.c
+++ b/drivers/iio/adc/qcom-spmi-vadc.c
@@ -122,15 +122,15 @@ struct vadc_priv {
 	struct mutex		 lock;
 };
 
-static const struct vadc_prescale_ratio vadc_prescale_ratios[] = {
-	{.num =  1, .den =  1},
-	{.num =  1, .den =  3},
-	{.num =  1, .den =  4},
-	{.num =  1, .den =  6},
-	{.num =  1, .den = 20},
-	{.num =  1, .den =  8},
-	{.num = 10, .den = 81},
-	{.num =  1, .den = 10}
+static const struct u32_fract vadc_prescale_ratios[] = {
+	{ .numerator =  1, .denominator =  1 },
+	{ .numerator =  1, .denominator =  3 },
+	{ .numerator =  1, .denominator =  4 },
+	{ .numerator =  1, .denominator =  6 },
+	{ .numerator =  1, .denominator = 20 },
+	{ .numerator =  1, .denominator =  8 },
+	{ .numerator = 10, .denominator = 81 },
+	{ .numerator =  1, .denominator = 10 },
 };
 
 static int vadc_read(struct vadc_priv *vadc, u16 offset, u8 *data)
@@ -404,13 +404,13 @@ static int vadc_measure_ref_points(struct vadc_priv *vadc)
 	return ret;
 }
 
-static int vadc_prescaling_from_dt(u32 num, u32 den)
+static int vadc_prescaling_from_dt(u32 numerator, u32 denominator)
 {
 	unsigned int pre;
 
 	for (pre = 0; pre < ARRAY_SIZE(vadc_prescale_ratios); pre++)
-		if (vadc_prescale_ratios[pre].num == num &&
-		    vadc_prescale_ratios[pre].den == den)
+		if (vadc_prescale_ratios[pre].numerator == numerator &&
+		    vadc_prescale_ratios[pre].denominator == denominator)
 			break;
 
 	if (pre == ARRAY_SIZE(vadc_prescale_ratios))
diff --git a/drivers/iio/adc/qcom-vadc-common.c b/drivers/iio/adc/qcom-vadc-common.c
index 14723896aab2..6c6aec848f98 100644
--- a/drivers/iio/adc/qcom-vadc-common.c
+++ b/drivers/iio/adc/qcom-vadc-common.c
@@ -289,44 +289,44 @@ static const struct vadc_map_pt adcmap7_100k[] = {
 	{ 2420, 130048 }
 };
 
-static const struct vadc_prescale_ratio adc5_prescale_ratios[] = {
-	{.num =  1, .den =  1},
-	{.num =  1, .den =  3},
-	{.num =  1, .den =  4},
-	{.num =  1, .den =  6},
-	{.num =  1, .den = 20},
-	{.num =  1, .den =  8},
-	{.num = 10, .den = 81},
-	{.num =  1, .den = 10},
-	{.num =  1, .den = 16}
+static const struct u32_fract adc5_prescale_ratios[] = {
+	{ .numerator =  1, .denominator =  1 },
+	{ .numerator =  1, .denominator =  3 },
+	{ .numerator =  1, .denominator =  4 },
+	{ .numerator =  1, .denominator =  6 },
+	{ .numerator =  1, .denominator = 20 },
+	{ .numerator =  1, .denominator =  8 },
+	{ .numerator = 10, .denominator = 81 },
+	{ .numerator =  1, .denominator = 10 },
+	{ .numerator =  1, .denominator = 16 },
 };
 
 static int qcom_vadc_scale_hw_calib_volt(
-				const struct vadc_prescale_ratio *prescale,
+				const struct u32_fract *prescale,
 				const struct adc5_data *data,
 				u16 adc_code, int *result_uv);
 static int qcom_vadc_scale_hw_calib_therm(
-				const struct vadc_prescale_ratio *prescale,
+				const struct u32_fract *prescale,
 				const struct adc5_data *data,
 				u16 adc_code, int *result_mdec);
 static int qcom_vadc7_scale_hw_calib_therm(
-				const struct vadc_prescale_ratio *prescale,
+				const struct u32_fract *prescale,
 				const struct adc5_data *data,
 				u16 adc_code, int *result_mdec);
 static int qcom_vadc_scale_hw_smb_temp(
-				const struct vadc_prescale_ratio *prescale,
+				const struct u32_fract *prescale,
 				const struct adc5_data *data,
 				u16 adc_code, int *result_mdec);
 static int qcom_vadc_scale_hw_chg5_temp(
-				const struct vadc_prescale_ratio *prescale,
+				const struct u32_fract *prescale,
 				const struct adc5_data *data,
 				u16 adc_code, int *result_mdec);
 static int qcom_vadc_scale_hw_calib_die_temp(
-				const struct vadc_prescale_ratio *prescale,
+				const struct u32_fract *prescale,
 				const struct adc5_data *data,
 				u16 adc_code, int *result_mdec);
 static int qcom_vadc7_scale_hw_calib_die_temp(
-				const struct vadc_prescale_ratio *prescale,
+				const struct u32_fract *prescale,
 				const struct adc5_data *data,
 				u16 adc_code, int *result_mdec);
 
@@ -406,7 +406,7 @@ static void qcom_vadc_scale_calib(const struct vadc_linear_graph *calib_graph,
 }
 
 static int qcom_vadc_scale_volt(const struct vadc_linear_graph *calib_graph,
-				const struct vadc_prescale_ratio *prescale,
+				const struct u32_fract *prescale,
 				bool absolute, u16 adc_code,
 				int *result_uv)
 {
@@ -414,15 +414,15 @@ static int qcom_vadc_scale_volt(const struct vadc_linear_graph *calib_graph,
 
 	qcom_vadc_scale_calib(calib_graph, adc_code, absolute, &voltage);
 
-	voltage = voltage * prescale->den;
-	result = div64_s64(voltage, prescale->num);
+	voltage *= prescale->denominator;
+	result = div64_s64(voltage, prescale->numerator);
 	*result_uv = result;
 
 	return 0;
 }
 
 static int qcom_vadc_scale_therm(const struct vadc_linear_graph *calib_graph,
-				 const struct vadc_prescale_ratio *prescale,
+				 const struct u32_fract *prescale,
 				 bool absolute, u16 adc_code,
 				 int *result_mdec)
 {
@@ -444,7 +444,7 @@ static int qcom_vadc_scale_therm(const struct vadc_linear_graph *calib_graph,
 }
 
 static int qcom_vadc_scale_die_temp(const struct vadc_linear_graph *calib_graph,
-				    const struct vadc_prescale_ratio *prescale,
+				    const struct u32_fract *prescale,
 				    bool absolute,
 				    u16 adc_code, int *result_mdec)
 {
@@ -454,8 +454,8 @@ static int qcom_vadc_scale_die_temp(const struct vadc_linear_graph *calib_graph,
 	qcom_vadc_scale_calib(calib_graph, adc_code, absolute, &voltage);
 
 	if (voltage > 0) {
-		temp = voltage * prescale->den;
-		do_div(temp, prescale->num * 2);
+		temp = voltage * prescale->denominator;
+		do_div(temp, prescale->numerator * 2);
 		voltage = temp;
 	} else {
 		voltage = 0;
@@ -467,7 +467,7 @@ static int qcom_vadc_scale_die_temp(const struct vadc_linear_graph *calib_graph,
 }
 
 static int qcom_vadc_scale_chg_temp(const struct vadc_linear_graph *calib_graph,
-				    const struct vadc_prescale_ratio *prescale,
+				    const struct u32_fract *prescale,
 				    bool absolute,
 				    u16 adc_code, int *result_mdec)
 {
@@ -475,8 +475,8 @@ static int qcom_vadc_scale_chg_temp(const struct vadc_linear_graph *calib_graph,
 
 	qcom_vadc_scale_calib(calib_graph, adc_code, absolute, &voltage);
 
-	voltage = voltage * prescale->den;
-	voltage = div64_s64(voltage, prescale->num);
+	voltage *= prescale->denominator;
+	voltage = div64_s64(voltage, prescale->numerator);
 	voltage = ((PMI_CHG_SCALE_1) * (voltage * 2));
 	voltage = (voltage + PMI_CHG_SCALE_2);
 	result =  div64_s64(voltage, 1000000);
@@ -487,21 +487,21 @@ static int qcom_vadc_scale_chg_temp(const struct vadc_linear_graph *calib_graph,
 
 /* convert voltage to ADC code, using 1.875V reference */
 static u16 qcom_vadc_scale_voltage_code(s32 voltage,
-					const struct vadc_prescale_ratio *prescale,
+					const struct u32_fract *prescale,
 					const u32 full_scale_code_volt,
 					unsigned int factor)
 {
 	s64 volt = voltage;
 	s64 adc_vdd_ref_mv = 1875; /* reference voltage */
 
-	volt *= prescale->num * factor * full_scale_code_volt;
-	volt = div64_s64(volt, (s64)prescale->den * adc_vdd_ref_mv * 1000);
+	volt *= prescale->numerator * factor * full_scale_code_volt;
+	volt = div64_s64(volt, (s64)prescale->denominator * adc_vdd_ref_mv * 1000);
 
 	return volt;
 }
 
 static int qcom_vadc_scale_code_voltage_factor(u16 adc_code,
-				const struct vadc_prescale_ratio *prescale,
+				const struct u32_fract *prescale,
 				const struct adc5_data *data,
 				unsigned int factor)
 {
@@ -520,8 +520,8 @@ static int qcom_vadc_scale_code_voltage_factor(u16 adc_code,
 	voltage = (s64) adc_code * adc_vdd_ref_mv * 1000;
 	voltage = div64_s64(voltage, data->full_scale_code_volt);
 	if (voltage > 0) {
-		voltage *= prescale->den;
-		temp = prescale->num * factor;
+		voltage *= prescale->denominator;
+		temp = prescale->numerator * factor;
 		voltage = div64_s64(voltage, temp);
 	} else {
 		voltage = 0;
@@ -531,7 +531,7 @@ static int qcom_vadc_scale_code_voltage_factor(u16 adc_code,
 }
 
 static int qcom_vadc7_scale_hw_calib_therm(
-				const struct vadc_prescale_ratio *prescale,
+				const struct u32_fract *prescale,
 				const struct adc5_data *data,
 				u16 adc_code, int *result_mdec)
 {
@@ -557,7 +557,7 @@ static int qcom_vadc7_scale_hw_calib_therm(
 }
 
 static int qcom_vadc_scale_hw_calib_volt(
-				const struct vadc_prescale_ratio *prescale,
+				const struct u32_fract *prescale,
 				const struct adc5_data *data,
 				u16 adc_code, int *result_uv)
 {
@@ -568,7 +568,7 @@ static int qcom_vadc_scale_hw_calib_volt(
 }
 
 static int qcom_vadc_scale_hw_calib_therm(
-				const struct vadc_prescale_ratio *prescale,
+				const struct u32_fract *prescale,
 				const struct adc5_data *data,
 				u16 adc_code, int *result_mdec)
 {
@@ -584,7 +584,7 @@ static int qcom_vadc_scale_hw_calib_therm(
 }
 
 static int qcom_vadc_scale_hw_calib_die_temp(
-				const struct vadc_prescale_ratio *prescale,
+				const struct u32_fract *prescale,
 				const struct adc5_data *data,
 				u16 adc_code, int *result_mdec)
 {
@@ -596,7 +596,7 @@ static int qcom_vadc_scale_hw_calib_die_temp(
 }
 
 static int qcom_vadc7_scale_hw_calib_die_temp(
-				const struct vadc_prescale_ratio *prescale,
+				const struct u32_fract *prescale,
 				const struct adc5_data *data,
 				u16 adc_code, int *result_mdec)
 {
@@ -611,7 +611,7 @@ static int qcom_vadc7_scale_hw_calib_die_temp(
 }
 
 static int qcom_vadc_scale_hw_smb_temp(
-				const struct vadc_prescale_ratio *prescale,
+				const struct u32_fract *prescale,
 				const struct adc5_data *data,
 				u16 adc_code, int *result_mdec)
 {
@@ -623,7 +623,7 @@ static int qcom_vadc_scale_hw_smb_temp(
 }
 
 static int qcom_vadc_scale_hw_chg5_temp(
-				const struct vadc_prescale_ratio *prescale,
+				const struct u32_fract *prescale,
 				const struct adc5_data *data,
 				u16 adc_code, int *result_mdec)
 {
@@ -636,7 +636,7 @@ static int qcom_vadc_scale_hw_chg5_temp(
 
 int qcom_vadc_scale(enum vadc_scale_fn_type scaletype,
 		    const struct vadc_linear_graph *calib_graph,
-		    const struct vadc_prescale_ratio *prescale,
+		    const struct u32_fract *prescale,
 		    bool absolute,
 		    u16 adc_code, int *result)
 {
@@ -667,7 +667,7 @@ EXPORT_SYMBOL(qcom_vadc_scale);
 u16 qcom_adc_tm5_temp_volt_scale(unsigned int prescale_ratio,
 				 u32 full_scale_code_volt, int temp)
 {
-	const struct vadc_prescale_ratio *prescale = &adc5_prescale_ratios[prescale_ratio];
+	const struct u32_fract *prescale = &adc5_prescale_ratios[prescale_ratio];
 	s32 voltage;
 
 	voltage = qcom_vadc_map_temp_voltage(adcmap_100k_104ef_104fb_1875_vref,
@@ -682,7 +682,7 @@ int qcom_adc5_hw_scale(enum vadc_scale_fn_type scaletype,
 		    const struct adc5_data *data,
 		    u16 adc_code, int *result)
 {
-	const struct vadc_prescale_ratio *prescale = &adc5_prescale_ratios[prescale_ratio];
+	const struct u32_fract *prescale = &adc5_prescale_ratios[prescale_ratio];
 
 	if (!(scaletype >= SCALE_HW_CALIB_DEFAULT &&
 		scaletype < SCALE_HW_CALIB_INVALID)) {
@@ -695,13 +695,13 @@ int qcom_adc5_hw_scale(enum vadc_scale_fn_type scaletype,
 }
 EXPORT_SYMBOL(qcom_adc5_hw_scale);
 
-int qcom_adc5_prescaling_from_dt(u32 num, u32 den)
+int qcom_adc5_prescaling_from_dt(u32 numerator, u32 denominator)
 {
 	unsigned int pre;
 
 	for (pre = 0; pre < ARRAY_SIZE(adc5_prescale_ratios); pre++)
-		if (adc5_prescale_ratios[pre].num == num &&
-		    adc5_prescale_ratios[pre].den == den)
+		if (adc5_prescale_ratios[pre].numerator == numerator &&
+		    adc5_prescale_ratios[pre].denominator == denominator)
 			break;
 
 	if (pre == ARRAY_SIZE(adc5_prescale_ratios))
diff --git a/include/linux/iio/adc/qcom-vadc-common.h b/include/linux/iio/adc/qcom-vadc-common.h
index 33f60f43e1aa..ce78d4804994 100644
--- a/include/linux/iio/adc/qcom-vadc-common.h
+++ b/include/linux/iio/adc/qcom-vadc-common.h
@@ -6,6 +6,7 @@
 #ifndef QCOM_VADC_COMMON_H
 #define QCOM_VADC_COMMON_H
 
+#include <linux/math.h>
 #include <linux/types.h>
 
 #define VADC_CONV_TIME_MIN_US			2000
@@ -79,16 +80,6 @@ struct vadc_linear_graph {
 	s32 gnd;
 };
 
-/**
- * struct vadc_prescale_ratio - Represent scaling ratio for ADC input.
- * @num: the inverse numerator of the gain applied to the input channel.
- * @den: the inverse denominator of the gain applied to the input channel.
- */
-struct vadc_prescale_ratio {
-	u32 num;
-	u32 den;
-};
-
 /**
  * enum vadc_scale_fn_type - Scaling function to convert ADC code to
  *				physical scaled units for the channel.
@@ -144,12 +135,12 @@ struct adc5_data {
 
 int qcom_vadc_scale(enum vadc_scale_fn_type scaletype,
 		    const struct vadc_linear_graph *calib_graph,
-		    const struct vadc_prescale_ratio *prescale,
+		    const struct u32_fract *prescale,
 		    bool absolute,
 		    u16 adc_code, int *result_mdec);
 
 struct qcom_adc5_scale_type {
-	int (*scale_fn)(const struct vadc_prescale_ratio *prescale,
+	int (*scale_fn)(const struct u32_fract *prescale,
 		const struct adc5_data *data, u16 adc_code, int *result);
 };
 
-- 
2.34.1

[PATCH v2 5/5] iio: afe: iio-rescale: Re-use generic struct s32_fract

[Andy Shevchenko]

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 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...

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

Re: [PATCH v2 5/5] iio: afe: iio-rescale: Re-use generic struct s32_fract

[Jonathan Cameron]

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;

Re: [PATCH v2 5/5] iio: afe: iio-rescale: Re-use generic struct s32_fract

[Andy Shevchenko]

On Sat, Jan 15, 2022 at 06:52:03PM +0000, Jonathan Cameron wrote:
> 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.

So far no news :-)

> Boiler plate removal is always nice of course...

That's what I considered nice as well.

...

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

We also have mult_frac() macro which can be extended by mult_fract() for
these structures.

...

> >  	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?

Yeah, I agree that this may be in a separate change before of after the series.
I will split.

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

> > -	rescale->source = source;
> > -

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 5/5] iio: afe: iio-rescale: Re-use generic struct s32_fract

[Andy Shevchenko]

On Mon, Jan 24, 2022 at 05:18:33PM +0200, Andy Shevchenko wrote:
> On Sat, Jan 15, 2022 at 06:52:03PM +0000, Jonathan Cameron wrote:
> > On Mon, 10 Jan 2022 21:31:04 +0200
> > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> > I'm not totally sold on this series showing there is a strong case for
> > these macros so interested to hear what others think.
> 
> So far no news :-)
> 
> > Boiler plate removal is always nice of course...
> 
> That's what I considered nice as well.

Ah, and thanks for review!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 5/5] iio: afe: iio-rescale: Re-use generic struct s32_fract

[Liam Beguin]

Hi Andy,

On Mon, Jan 24, 2022 at 05:18:32PM +0200, Andy Shevchenko wrote:
> On Sat, Jan 15, 2022 at 06:52:03PM +0000, Jonathan Cameron wrote:
> > 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.
> 
> So far no news :-)

Like I mentioned briefly in the other thread[1], I don't really see the
advantage for the AFE driver given that it's almost just like renaming
the parameters.

For the other drivers affected by the change, it drops the definition of
the structure which is nice. So overall, it's a plus IMO :-)

[1] https://lore.kernel.org/linux-iio/20220108205319.2046348-1-liambeguin@gmail.com/

Cheers,
Liam

> > Boiler plate removal is always nice of course...
> 
> That's what I considered nice as well.
> 
> ...
> 
> > > 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 :)
> 
> We also have mult_frac() macro which can be extended by mult_fract() for
> these structures.
> 
> ...
> 
> > >  	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?
> 
> Yeah, I agree that this may be in a separate change before of after the series.
> I will split.
> 
> > >  	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;
> 
> > > -	rescale->source = source;
> > > -
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 

Re: [PATCH v2 5/5] iio: afe: iio-rescale: Re-use generic struct s32_fract

[Andy Shevchenko]

On Mon, Jan 24, 2022 at 04:28:09PM -0500, Liam Beguin wrote:
> On Mon, Jan 24, 2022 at 05:18:32PM +0200, Andy Shevchenko wrote:
> > On Sat, Jan 15, 2022 at 06:52:03PM +0000, Jonathan Cameron wrote:
> > > 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.
> > 
> > So far no news :-)
> 
> Like I mentioned briefly in the other thread[1], I don't really see the
> advantage for the AFE driver given that it's almost just like renaming
> the parameters.

I tend to disagree, perhaps I wasn't so clear in my points.

The change reveals that the layering can be improved. In OOP
the object A which is inherited (or encapsulated as we see here)
allows to clearly get what kind of data the methods are operating
with / on. As you may see the two functions and the method
declaration appears to have interest only in the fraction data
for rescaling. The cleanup I consider helpful in the terms
of layering and OOP.

> For the other drivers affected by the change, it drops the definition of
> the structure which is nice. So overall, it's a plus IMO :-)

Thanks!

> [1] https://lore.kernel.org/linux-iio/20220108205319.2046348-1-liambeguin@gmail.com/

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 5/5] iio: afe: iio-rescale: Re-use generic struct s32_fract

[Peter Rosin]

On 2022-01-25 14:17, Andy Shevchenko wrote:
> On Mon, Jan 24, 2022 at 04:28:09PM -0500, Liam Beguin wrote:
>> On Mon, Jan 24, 2022 at 05:18:32PM +0200, Andy Shevchenko wrote:
>>> On Sat, Jan 15, 2022 at 06:52:03PM +0000, Jonathan Cameron wrote:
>>>> 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.
>>>
>>> So far no news :-)
>>
>> Like I mentioned briefly in the other thread[1], I don't really see the
>> advantage for the AFE driver given that it's almost just like renaming
>> the parameters.
> 
> I tend to disagree, perhaps I wasn't so clear in my points.
> 
> The change reveals that the layering can be improved. In OOP
> the object A which is inherited (or encapsulated as we see here)
> allows to clearly get what kind of data the methods are operating
> with / on. As you may see the two functions and the method
> declaration appears to have interest only in the fraction data
> for rescaling. The cleanup I consider helpful in the terms
> of layering and OOP.

Hi!

[Sorry for the delay, I have been far too busy for far too long]

While this is all true for the current set of front-ends, it is not
all that far-fetched to imagine some kind of future front-end that
requires some other parameter, such that the rescaling fraction is no
longer the only thing of interest. So, I have the feeling that changing
the type of the 2nd argument of the ->props functions to just the
fraction instead of the bigger object is conceptually wrong and
something that will later turn out to have been a bad idea.

Regarding the new xyz_fract types, I have no strong opinion. But as
long as there are no helper functions for the new types, I see little
value in them. To me, they look mostly like something that newcomers
(and others) will not know about or expect, and it will just be
another thing that you have to look out for during review, to keep new
numerators/denominators from appearing and causing extra rounds of
review for something that is mostly a bikeshed issue.

My guess is that many times where fractions are used, they are used
since fp math is not available. And that means that there will be a
lot of special handling and shortcuts done since various things about
accuracy and precision can be assumed. I think it will be hard to do
all that centrally and in a comprehensible way. But if it is done it
will of course also be possible to eliminate bugs since people may
have taken too many shortcuts. But simply changing to xyz_fract and
then not take it further than that will not change much.

Also, there is already a v4l2_fract which is exposed in UAPI (maybe
there are others?). I don't see how we bring that one in line with this
new struct xyz_fract scheme?

Cheers,
Peter

Re: [PATCH v2 5/5] iio: afe: iio-rescale: Re-use generic struct s32_fract

[Andy Shevchenko]

On Tue, Jan 25, 2022 at 03:54:07PM +0100, Peter Rosin wrote:
> On 2022-01-25 14:17, Andy Shevchenko wrote:
> > On Mon, Jan 24, 2022 at 04:28:09PM -0500, Liam Beguin wrote:
> >> On Mon, Jan 24, 2022 at 05:18:32PM +0200, Andy Shevchenko wrote:
> >>> On Sat, Jan 15, 2022 at 06:52:03PM +0000, Jonathan Cameron wrote:
> >>>> On Mon, 10 Jan 2022 21:31:04 +0200
> >>>> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

...

> >>>> I'm not totally sold on this series showing there is a strong case for
> >>>> these macros so interested to hear what others think.
> >>>
> >>> So far no news :-)
> >>
> >> Like I mentioned briefly in the other thread[1], I don't really see the
> >> advantage for the AFE driver given that it's almost just like renaming
> >> the parameters.
> > 
> > I tend to disagree, perhaps I wasn't so clear in my points.
> > 
> > The change reveals that the layering can be improved. In OOP
> > the object A which is inherited (or encapsulated as we see here)
> > allows to clearly get what kind of data the methods are operating
> > with / on. As you may see the two functions and the method
> > declaration appears to have interest only in the fraction data
> > for rescaling. The cleanup I consider helpful in the terms
> > of layering and OOP.

> [Sorry for the delay, I have been far too busy for far too long]

Anyway, thanks for review! My answers below.

> While this is all true for the current set of front-ends, it is not
> all that far-fetched to imagine some kind of future front-end that
> requires some other parameter, such that the rescaling fraction is no
> longer the only thing of interest. So, I have the feeling that changing
> the type of the 2nd argument of the ->props functions to just the
> fraction instead of the bigger object is conceptually wrong and
> something that will later turn out to have been a bad idea.

How? Technically as I mentioned currently it's the case to use
only the date from fraction. The bigger object would be needed
in case of using data that is not fraction. Either way it would
be easy to add a container_of() than supply unneeded data to
the method.

TL;DR: It makes possible not to mix bananas with wooden boxes.

> Regarding the new xyz_fract types, I have no strong opinion. But as
> long as there are no helper functions for the new types, I see little
> value in them. To me, they look mostly like something that newcomers
> (and others) will not know about or expect, and it will just be
> another thing that you have to look out for during review, to keep new
> numerators/denominators from appearing and causing extra rounds of
> review for something that is mostly a bikeshed issue.
> 
> My guess is that many times where fractions are used, they are used
> since fp math is not available. And that means that there will be a
> lot of special handling and shortcuts done since various things about
> accuracy and precision can be assumed. I think it will be hard to do
> all that centrally and in a comprehensible way. But if it is done it
> will of course also be possible to eliminate bugs since people may
> have taken too many shortcuts. But simply changing to xyz_fract and
> then not take it further than that will not change much.

I understand your point. I will provide a mult_fract() macro and
apply it to AFE to show the usability of this. Would it work better?

I haven't checked the other possible improvements based on new type, but
I truly believe that the types themselves are good to have.

> Also, there is already a v4l2_fract which is exposed in UAPI (maybe
> there are others?). I don't see how we bring that one in line with this
> new struct xyz_fract scheme?

UAPI is another story. It might be possible to extend, but I would like
to avoid expanding the proposed types to UAPI (we don't have many users
and I believe they more or less unique, so v4l2 is rather exception than
usual).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 5/5] iio: afe: iio-rescale: Re-use generic struct s32_fract

[Peter Rosin]

On 2022-01-25 19:17, Andy Shevchenko wrote:
> On Tue, Jan 25, 2022 at 03:54:07PM +0100, Peter Rosin wrote:
>> On 2022-01-25 14:17, Andy Shevchenko wrote:
>>> On Mon, Jan 24, 2022 at 04:28:09PM -0500, Liam Beguin wrote:
>>>> On Mon, Jan 24, 2022 at 05:18:32PM +0200, Andy Shevchenko wrote:
>>>>> On Sat, Jan 15, 2022 at 06:52:03PM +0000, Jonathan Cameron wrote:
>>>>>> On Mon, 10 Jan 2022 21:31:04 +0200
>>>>>> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> 
> ...
> 
>>>>>> I'm not totally sold on this series showing there is a strong case for
>>>>>> these macros so interested to hear what others think.
>>>>>
>>>>> So far no news :-)
>>>>
>>>> Like I mentioned briefly in the other thread[1], I don't really see the
>>>> advantage for the AFE driver given that it's almost just like renaming
>>>> the parameters.
>>>
>>> I tend to disagree, perhaps I wasn't so clear in my points.
>>>
>>> The change reveals that the layering can be improved. In OOP
>>> the object A which is inherited (or encapsulated as we see here)
>>> allows to clearly get what kind of data the methods are operating
>>> with / on. As you may see the two functions and the method
>>> declaration appears to have interest only in the fraction data
>>> for rescaling. The cleanup I consider helpful in the terms
>>> of layering and OOP.
> 
>> [Sorry for the delay, I have been far too busy for far too long]
> 
> Anyway, thanks for review! My answers below.
> 
>> While this is all true for the current set of front-ends, it is not
>> all that far-fetched to imagine some kind of future front-end that
>> requires some other parameter, such that the rescaling fraction is no
>> longer the only thing of interest. So, I have the feeling that changing
>> the type of the 2nd argument of the ->props functions to just the
>> fraction instead of the bigger object is conceptually wrong and
>> something that will later turn out to have been a bad idea.
> 
> How? Technically as I mentioned currently it's the case to use
> only the date from fraction. The bigger object would be needed
> in case of using data that is not fraction. Either way it would
> be easy to add a container_of() than supply unneeded data to
> the method.

It's easy to both remove and to add back "the bigger object". I just
don't see the point of the churn. Technically you can probably rearrange
stuff in probe and remove the 2nd argument to ->props() altogether and
chase pointers from the dev object instead. I don't see the point of
that either. It doesn't really make things simpler, it doesn't really
make things easier to read. To me, it's just pointless churn.

> 
> TL;DR: It makes possible not to mix bananas with wooden boxes.

Which is all good until you need to ship an apple in the box with the
bananas. (e.g. if you for some reason need the bananas to get ripe real
quick, apples produce ethylene)

> 
>> Regarding the new xyz_fract types, I have no strong opinion. But as
>> long as there are no helper functions for the new types, I see little
>> value in them. To me, they look mostly like something that newcomers
>> (and others) will not know about or expect, and it will just be
>> another thing that you have to look out for during review, to keep new
>> numerators/denominators from appearing and causing extra rounds of
>> review for something that is mostly a bikeshed issue.
>>
>> My guess is that many times where fractions are used, they are used
>> since fp math is not available. And that means that there will be a
>> lot of special handling and shortcuts done since various things about
>> accuracy and precision can be assumed. I think it will be hard to do
>> all that centrally and in a comprehensible way. But if it is done it
>> will of course also be possible to eliminate bugs since people may
>> have taken too many shortcuts. But simply changing to xyz_fract and
>> then not take it further than that will not change much.
> 
> I understand your point. I will provide a mult_fract() macro and
> apply it to AFE to show the usability of this. Would it work better?

In my experience, burying stuff in macros will make it harder to follow
what is really happening. Something that has proven hard as-is in this
driver. While reviewing the changes from Liam, I have repeatedly looked
up the various division variants to find out what they are actually
doing. Here, it's not the individual steps that are difficult. I feel
that if we start adding macros for fractions they will soon multiply and
I'm not really looking forward to also have a set of similar fraction
macros to juggle.

But sure, feel free to suggest something. But please hold until the
current work from Liam is merged. That series is clearly more
important, and I'm not really interested in neither adding more work for
him nor a cleanup of the current code without those pending changes.

Cheers,
Peter

Re: [PATCH v2 5/5] iio: afe: iio-rescale: Re-use generic struct s32_fract

[Andy Shevchenko]

On Wed, Jan 26, 2022 at 11:26:50AM +0100, Peter Rosin wrote:
> On 2022-01-25 19:17, Andy Shevchenko wrote:
> > On Tue, Jan 25, 2022 at 03:54:07PM +0100, Peter Rosin wrote:
> >> On 2022-01-25 14:17, Andy Shevchenko wrote:
> >>> On Mon, Jan 24, 2022 at 04:28:09PM -0500, Liam Beguin wrote:
> >>>> On Mon, Jan 24, 2022 at 05:18:32PM +0200, Andy Shevchenko wrote:
> >>>>> On Sat, Jan 15, 2022 at 06:52:03PM +0000, Jonathan Cameron wrote:
> >>>>>> On Mon, 10 Jan 2022 21:31:04 +0200
> >>>>>> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

...

> >>>>>> I'm not totally sold on this series showing there is a strong case for
> >>>>>> these macros so interested to hear what others think.
> >>>>>
> >>>>> So far no news :-)
> >>>>
> >>>> Like I mentioned briefly in the other thread[1], I don't really see the
> >>>> advantage for the AFE driver given that it's almost just like renaming
> >>>> the parameters.
> >>>
> >>> I tend to disagree, perhaps I wasn't so clear in my points.
> >>>
> >>> The change reveals that the layering can be improved. In OOP
> >>> the object A which is inherited (or encapsulated as we see here)
> >>> allows to clearly get what kind of data the methods are operating
> >>> with / on. As you may see the two functions and the method
> >>> declaration appears to have interest only in the fraction data
> >>> for rescaling. The cleanup I consider helpful in the terms
> >>> of layering and OOP.
> > 
> >> [Sorry for the delay, I have been far too busy for far too long]
> > 
> > Anyway, thanks for review! My answers below.
> > 
> >> While this is all true for the current set of front-ends, it is not
> >> all that far-fetched to imagine some kind of future front-end that
> >> requires some other parameter, such that the rescaling fraction is no
> >> longer the only thing of interest. So, I have the feeling that changing
> >> the type of the 2nd argument of the ->props functions to just the
> >> fraction instead of the bigger object is conceptually wrong and
> >> something that will later turn out to have been a bad idea.
> > 
> > How? Technically as I mentioned currently it's the case to use
> > only the date from fraction. The bigger object would be needed
> > in case of using data that is not fraction. Either way it would
> > be easy to add a container_of() than supply unneeded data to
> > the method.
> 
> It's easy to both remove and to add back "the bigger object". I just
> don't see the point of the churn. Technically you can probably rearrange
> stuff in probe and remove the 2nd argument to ->props() altogether and
> chase pointers from the dev object instead. I don't see the point of
> that either. It doesn't really make things simpler, it doesn't really
> make things easier to read. To me, it's just pointless churn.

Since you still haven't got a point the conclusions are wrong.
The point is (I dunno how more clear to make it) is to have proper
layering from the (current) design perspective.

If we go to the road full of "if it will come XYZ then this sucks".
The future is uncertain and neither of us may prove the current
change good or bad in terms of the future (unknown and uncertain)
changes.

Preventing this patch to land is to tell "Oh, my design is bad,
but I will keep it, because in the future everything may change".
So, why don't you make this future to be now?

> > TL;DR: It makes possible not to mix bananas with wooden boxes.
> 
> Which is all good until you need to ship an apple in the box with the
> bananas. (e.g. if you for some reason need the bananas to get ripe real
> quick, apples produce ethylene)

Really. arguments about the future changes are weak. If you have
patches in mind, send them, We will see in practice what you meant.

> >> Regarding the new xyz_fract types, I have no strong opinion. But as
> >> long as there are no helper functions for the new types, I see little
> >> value in them. To me, they look mostly like something that newcomers
> >> (and others) will not know about or expect, and it will just be
> >> another thing that you have to look out for during review, to keep new
> >> numerators/denominators from appearing and causing extra rounds of
> >> review for something that is mostly a bikeshed issue.
> >>
> >> My guess is that many times where fractions are used, they are used
> >> since fp math is not available. And that means that there will be a
> >> lot of special handling and shortcuts done since various things about
> >> accuracy and precision can be assumed. I think it will be hard to do
> >> all that centrally and in a comprehensible way. But if it is done it
> >> will of course also be possible to eliminate bugs since people may
> >> have taken too many shortcuts. But simply changing to xyz_fract and
> >> then not take it further than that will not change much.
> > 
> > I understand your point. I will provide a mult_fract() macro and
> > apply it to AFE to show the usability of this. Would it work better?
> 
> In my experience, burying stuff in macros will make it harder to follow
> what is really happening. Something that has proven hard as-is in this
> driver. While reviewing the changes from Liam, I have repeatedly looked
> up the various division variants to find out what they are actually
> doing. Here, it's not the individual steps that are difficult. I feel
> that if we start adding macros for fractions they will soon multiply and
> I'm not really looking forward to also have a set of similar fraction
> macros to juggle.

The problem here is that every driver would like to do this differently
and since it's related to the calculation we will have all possible error
prone implementations which do miscalculations (yes, one may not notice
traditional off-by-one until it becomes a huge issue by using additional
conversion formulas or so).

> But sure, feel free to suggest something. But please hold until the
> current work from Liam is merged.
> That series is clearly more
> important, and I'm not really interested in neither adding more work for
> him nor a cleanup of the current code without those pending changes.

I'm very well fine with that. As I mentioned from the beginning, I may rebase
this on top of the Liam's work.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 5/5] iio: afe: iio-rescale: Re-use generic struct s32_fract

[Peter Rosin]

On 2022-01-26 13:04, Andy Shevchenko wrote:
> On Wed, Jan 26, 2022 at 11:26:50AM +0100, Peter Rosin wrote:
>> It's easy to both remove and to add back "the bigger object". I just
>> don't see the point of the churn. Technically you can probably rearrange
>> stuff in probe and remove the 2nd argument to ->props() altogether and
>> chase pointers from the dev object instead. I don't see the point of
>> that either. It doesn't really make things simpler, it doesn't really
>> make things easier to read. To me, it's just pointless churn.
> 
> Since you still haven't got a point the conclusions are wrong.
> The point is (I dunno how more clear to make it) is to have proper
> layering from the (current) design perspective.

I think got the gist of it. I simply do not agree with your conclusion
about what the "proper layering" should be.

Cheers,
Peter

Re: [PATCH v2 5/5] iio: afe: iio-rescale: Re-use generic struct s32_fract

[Andy Shevchenko]

On Wed, Jan 26, 2022 at 01:35:09PM +0100, Peter Rosin wrote:
> On 2022-01-26 13:04, Andy Shevchenko wrote:
> > On Wed, Jan 26, 2022 at 11:26:50AM +0100, Peter Rosin wrote:
> >> It's easy to both remove and to add back "the bigger object". I just
> >> don't see the point of the churn. Technically you can probably rearrange
> >> stuff in probe and remove the 2nd argument to ->props() altogether and
> >> chase pointers from the dev object instead. I don't see the point of
> >> that either. It doesn't really make things simpler, it doesn't really
> >> make things easier to read. To me, it's just pointless churn.
> > 
> > Since you still haven't got a point the conclusions are wrong.
> > The point is (I dunno how more clear to make it) is to have proper
> > layering from the (current) design perspective.
> 
> I think got the gist of it. I simply do not agree with your conclusion
> about what the "proper layering" should be.

And I see no real argument against it. With the patch applied I see
a better structure of the code and exactly necessary data to be passed
to the method. Which makes me think that current implementation is
either a leftover or was something like "let's take a bigger object
_just in case_", which I can't take as a proper layering.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 5/5] iio: afe: iio-rescale: Re-use generic struct s32_fract

[Liam Beguin]

Hi Andy,
On Wed, Jan 26, 2022 at 02:04:53PM +0200, Andy Shevchenko wrote:
> On Wed, Jan 26, 2022 at 11:26:50AM +0100, Peter Rosin wrote:
> > On 2022-01-25 19:17, Andy Shevchenko wrote:
> > > On Tue, Jan 25, 2022 at 03:54:07PM +0100, Peter Rosin wrote:
> > >> On 2022-01-25 14:17, Andy Shevchenko wrote:
> > >>> On Mon, Jan 24, 2022 at 04:28:09PM -0500, Liam Beguin wrote:
> > >>>> On Mon, Jan 24, 2022 at 05:18:32PM +0200, Andy Shevchenko wrote:
> > >>>>> On Sat, Jan 15, 2022 at 06:52:03PM +0000, Jonathan Cameron wrote:
> > >>>>>> On Mon, 10 Jan 2022 21:31:04 +0200
> > >>>>>> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> 
> ...

...

> The problem here is that every driver would like to do this differently
> and since it's related to the calculation we will have all possible error
> prone implementations which do miscalculations (yes, one may not notice
> traditional off-by-one until it becomes a huge issue by using additional
> conversion formulas or so).
> 
> > But sure, feel free to suggest something. But please hold until the
> > current work from Liam is merged.
> > That series is clearly more
> > important, and I'm not really interested in neither adding more work for
> > him nor a cleanup of the current code without those pending changes.
> 
> I'm very well fine with that. As I mentioned from the beginning, I may rebase
> this on top of the Liam's work.

I appreciate that! I'll make time to wrap things up so I don't hold you
up.

Cheers,
Liam

> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 

Re: [PATCH v2 5/5] iio: afe: iio-rescale: Re-use generic struct s32_fract

[Peter Rosin]

On 2022-01-26 14:01, Andy Shevchenko wrote:
> On Wed, Jan 26, 2022 at 01:35:09PM +0100, Peter Rosin wrote:
>> On 2022-01-26 13:04, Andy Shevchenko wrote:
>>> On Wed, Jan 26, 2022 at 11:26:50AM +0100, Peter Rosin wrote:
>>>> It's easy to both remove and to add back "the bigger object". I just
>>>> don't see the point of the churn. Technically you can probably rearrange
>>>> stuff in probe and remove the 2nd argument to ->props() altogether and
>>>> chase pointers from the dev object instead. I don't see the point of
>>>> that either. It doesn't really make things simpler, it doesn't really
>>>> make things easier to read. To me, it's just pointless churn.
>>>
>>> Since you still haven't got a point the conclusions are wrong.
>>> The point is (I dunno how more clear to make it) is to have proper
>>> layering from the (current) design perspective.
>>
>> I think got the gist of it. I simply do not agree with your conclusion
>> about what the "proper layering" should be.
> 
> And I see no real argument against it. With the patch applied I see
> a better structure of the code and exactly necessary data to be passed
> to the method. Which makes me think that current implementation is
> either a leftover or was something like "let's take a bigger object
> _just in case_", which I can't take as a proper layering.

The bigger object, or the one and only object as the current code is
written, is given to ->props() by design.

BTW, you don't seem to understand the ->props() functions. There is no
data "passed to" the ->props() functions. These functions instead fill
in properties. Currently this boils down to the scaling fraction, but I
can imagine other properties.

On 2022-01-25 19:17, Andy Shevchenko wrote:
>                              The bigger object would be needed
> in case of using data that is not fraction. Either way it would
> be easy to add a container_of() than supply unneeded data to
> the method.

You argued that it is easy to "break out" to the bigger object in case
it's needed. Which in my book is a sign of poor layering.
It's way easier to *not* change things, it's perfectly fine as-is.

The argument against making the change you propose is that it does not
make this small driver any easier to understand. It would still just
change things for the sake of changing them, and I do not see the point
of erasing the existing future-proofing when it has no cost.

To sum up, I'm ok with introducing fract_s32 in this driver, but I
don't agree with the signature change of ->props().

Cheers,
Peter

Re: [PATCH v2 5/5] iio: afe: iio-rescale: Re-use generic struct s32_fract

[Peter Rosin]

On 2022-01-26 13:04, Andy Shevchenko wrote:
> On Wed, Jan 26, 2022 at 11:26:50AM +0100, Peter Rosin wrote:
>> It's easy to both remove and to add back "the bigger object". I just
>> don't see the point of the churn. Technically you can probably rearrange
>> stuff in probe and remove the 2nd argument to ->props() altogether and
>> chase pointers from the dev object instead. I don't see the point of
>> that either. It doesn't really make things simpler, it doesn't really
>> make things easier to read. To me, it's just pointless churn.
> 
> Since you still haven't got a point the conclusions are wrong.
> The point is (I dunno how more clear to make it) is to have proper
> layering from the (current) design perspective.
> 
> If we go to the road full of "if it will come XYZ then this sucks".
> The future is uncertain and neither of us may prove the current
> change good or bad in terms of the future (unknown and uncertain)
> changes.
> 
> Preventing this patch to land is to tell "Oh, my design is bad,
> but I will keep it, because in the future everything may change".
> So, why don't you make this future to be now?
> 
>>> TL;DR: It makes possible not to mix bananas with wooden boxes.
>>
>> Which is all good until you need to ship an apple in the box with the
>> bananas. (e.g. if you for some reason need the bananas to get ripe real
>> quick, apples produce ethylene)
> 
> Really. arguments about the future changes are weak. If you have
> patches in mind, send them, We will see in practice what you meant.

I can do one better - here are links to patches from 7-8 months ago.

https://lore.kernel.org/lkml/20210530005917.20953-1-liambeguin@gmail.com/
https://lore.kernel.org/lkml/20210530005917.20953-6-liambeguin@gmail.com/

Or, if you prefer, the latest revisions.

https://lore.kernel.org/lkml/20220108205319.2046348-9-liambeguin@gmail.com/
https://lore.kernel.org/lkml/20220108205319.2046348-14-liambeguin@gmail.com/

You have made review comments on that series.

My previous arguments were based on gut feel, and I'm sorry for not
thinking of the offset in the referred series before.

Cheers,
Peter

Re: [PATCH v2 5/5] iio: afe: iio-rescale: Re-use generic struct s32_fract

[Andy Shevchenko]

On Thu, Jan 27, 2022 at 12:03:49PM +0100, Peter Rosin wrote:
> On 2022-01-26 14:01, Andy Shevchenko wrote:
> > On Wed, Jan 26, 2022 at 01:35:09PM +0100, Peter Rosin wrote:
> >> On 2022-01-26 13:04, Andy Shevchenko wrote:
> >>> On Wed, Jan 26, 2022 at 11:26:50AM +0100, Peter Rosin wrote:
> >>>> It's easy to both remove and to add back "the bigger object". I just
> >>>> don't see the point of the churn. Technically you can probably rearrange
> >>>> stuff in probe and remove the 2nd argument to ->props() altogether and
> >>>> chase pointers from the dev object instead. I don't see the point of
> >>>> that either. It doesn't really make things simpler, it doesn't really
> >>>> make things easier to read. To me, it's just pointless churn.
> >>>
> >>> Since you still haven't got a point the conclusions are wrong.
> >>> The point is (I dunno how more clear to make it) is to have proper
> >>> layering from the (current) design perspective.
> >>
> >> I think got the gist of it. I simply do not agree with your conclusion
> >> about what the "proper layering" should be.
> > 
> > And I see no real argument against it. With the patch applied I see
> > a better structure of the code and exactly necessary data to be passed
> > to the method. Which makes me think that current implementation is
> > either a leftover or was something like "let's take a bigger object
> > _just in case_", which I can't take as a proper layering.
> 
> The bigger object, or the one and only object as the current code is
> written, is given to ->props() by design.
> 
> BTW, you don't seem to understand the ->props() functions. There is no
> data "passed to" the ->props() functions. These functions instead fill
> in properties. Currently this boils down to the scaling fraction, but I
> can imagine other properties.

Currently the object of the properties is the same as struct __Txx_fract.
In the future it may indeed be expanded. In such case I see that the layering
might look like

struct ..._props {
	struct __Txx_fract fract;
	...
};

Of course it depends on the properties themselves, but at least that's
how I believe the OOP paradigm works. Am I mistaken?

> On 2022-01-25 19:17, Andy Shevchenko wrote:
> >                              The bigger object would be needed
> > in case of using data that is not fraction. Either way it would
> > be easy to add a container_of() than supply unneeded data to
> > the method.
> 
> You argued that it is easy to "break out" to the bigger object in case
> it's needed. Which in my book is a sign of poor layering.
> It's way easier to *not* change things, it's perfectly fine as-is.
> 
> The argument against making the change you propose is that it does not
> make this small driver any easier to understand. It would still just
> change things for the sake of changing them, and I do not see the point
> of erasing the existing future-proofing when it has no cost.
> 
> To sum up, I'm ok with introducing fract_s32 in this driver, but I
> don't agree with the signature change of ->props().

Thanks for valuable comments!

I postponed the change in any case, let Liam to finish his part first,
which we agreed is more important.


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 5/5] iio: afe: iio-rescale: Re-use generic struct s32_fract

[Andy Shevchenko]

On Thu, Jan 27, 2022 at 01:11:14PM +0100, Peter Rosin wrote:
> On 2022-01-26 13:04, Andy Shevchenko wrote:
> > On Wed, Jan 26, 2022 at 11:26:50AM +0100, Peter Rosin wrote:
> >> It's easy to both remove and to add back "the bigger object". I just
> >> don't see the point of the churn. Technically you can probably rearrange
> >> stuff in probe and remove the 2nd argument to ->props() altogether and
> >> chase pointers from the dev object instead. I don't see the point of
> >> that either. It doesn't really make things simpler, it doesn't really
> >> make things easier to read. To me, it's just pointless churn.
> > 
> > Since you still haven't got a point the conclusions are wrong.
> > The point is (I dunno how more clear to make it) is to have proper
> > layering from the (current) design perspective.
> > 
> > If we go to the road full of "if it will come XYZ then this sucks".
> > The future is uncertain and neither of us may prove the current
> > change good or bad in terms of the future (unknown and uncertain)
> > changes.
> > 
> > Preventing this patch to land is to tell "Oh, my design is bad,
> > but I will keep it, because in the future everything may change".
> > So, why don't you make this future to be now?
> > 
> >>> TL;DR: It makes possible not to mix bananas with wooden boxes.
> >>
> >> Which is all good until you need to ship an apple in the box with the
> >> bananas. (e.g. if you for some reason need the bananas to get ripe real
> >> quick, apples produce ethylene)
> > 
> > Really. arguments about the future changes are weak. If you have
> > patches in mind, send them, We will see in practice what you meant.
> 
> I can do one better - here are links to patches from 7-8 months ago.
> 
> https://lore.kernel.org/lkml/20210530005917.20953-1-liambeguin@gmail.com/
> https://lore.kernel.org/lkml/20210530005917.20953-6-liambeguin@gmail.com/
> 
> Or, if you prefer, the latest revisions.
> 
> https://lore.kernel.org/lkml/20220108205319.2046348-9-liambeguin@gmail.com/
> https://lore.kernel.org/lkml/20220108205319.2046348-14-liambeguin@gmail.com/
> 
> You have made review comments on that series.
> 
> My previous arguments were based on gut feel, and I'm sorry for not
> thinking of the offset in the referred series before.

No problem and thanks for your comments!

-- 
With Best Regards,
Andy Shevchenko