[PATCH v5 00/14] Add support for magnetometer Yamaha YAS537

[PATCH v5 00/14] Add support for magnetometer Yamaha YAS537

[Jakob Hauser]

This patchset adds YAS537 variant to the already existing driver for
Yamaha YAS magnetometers.

Patch 1 is a fix on the current driver.
Patches 2-13 are cleanups and refactoring.
Patch 14 finally adds the YAS537 variant.

Changes in v5:
 - Rebased to torvalds/linux v5.19.
 - Patch 6: Moved 3x comment "Used by YAS530, YAS532 and YAS533" into
   kernel doc.
 - Patch 6: Corrected missing renaming of 2x "yas530_get_measure",
   1x "yas530_power_on", 1x "YAS530_ACTUATE_INIT_COIL", 1x "YAS530_MEASURE".
 - Patch 7: Added "Suggested-by:" tag.
 - Split patch 9 v4 "Introduce 'chip_info' structure" into five patches 9-13.
 - Patch 9: In commit message changed wording "Device Tree".
 - Patch 9: Added commas to non-terminating arrays of "yas5xx_product_name",
   "yas5xx_version_name", and "yas5xx_chip_info".
 - Patch 9: Added "product_name" and "version_name" to "chip_info" struct.
 - Patch 9: Added "s" to array name "yas5xx_version_names".
 - Patch 9: Added indices to array "yas5xx_version_names".
 - Patch 9: For strings in arrays "yas5xx_product_name" and
   "yas5xx_version_names", applied char * instead of char [].
 - Patch 9: Initialize struct "chip_info" as const.
 - Patch 9: In function yas5xx_probe(), moved declaration of "id_check"
   to a new line.
 - Patch 9: In function yas5xx_probe(), after "if (id_check !=
   yas5xx->chip_info->devid)" applied dev_err_probe().
 - Patch 10: In function yas5xx_volatile_reg(), renamed integer "j" into
   "reg_qty".
 - Patch 12: Improved comments on arrays "t_ref_counts" and
   "min_temp_celsius_x10".
 - Patch 12: Changed arrays "t_ref_counts" and "min_temp_celsius_x10"
   to static.
 - Patch 12: Corrected wrong spelling of "celcius" with "c" in array
   "min_temp_celsius_x10"
 - Patch 13: In function yas5xx_probe(), added the conditional
   "if (yas5xx->chip_info->measure_offsets)" as a preparatory step for YAS537.
 - Patch 14: In function yas537_power_on(), replaced comment "Write registers
   according to Android driver" by "Writing ADCCAL and TRM registers".
 - Patch 14: In function yas537_power_on(), write register ADCCAL as a
   bulk write.
 - Patch 14: In function yas537_power_on(), in formula for "intrvl" replaced
   value "1000" by MILLI. Added "linux/units.h" to includes.

Changes in v4:
 - Rebased to torvalds/linux v5.19-rc4, as this now includes Linus' patch
   "Fix memchr_inv() misuse" on driver yamaha-yas530.
 - Removed redundant Cc tags.
 - Patch 2: Replaced "<= ... + 7" by "< ... + 8" and adapted commit message.
 - Patch 3: Added default for switch statement, I forgot to add this.
 - Patch 4: In function yas5xx_get_measure(), changed wording "milli degrees"
   to "millidegrees".
 - Patch 6: Changed the renaming of function from "yas530_532_" to "yas530_".
   Same for registers. Added additional comments where appropriate.
 - Patch 6: Removed "Reviewed-by:" tag of Andy.
 - Split patch 7 v3 into two patches -> patch 7 v4 and patch 8 v4.
 - Patch 8: Applied "if (a && b)" suggestion at memchr_inv() by Andy in
   function yas532_get_calibration_data().
 - Patch 8: Removed defines for device IDs YAS537 and YAS539 and accordingly
   the comment "These variant IDs are known from code dumps".
 - Patch 9: New patch to introduce the "chip_info" approach.
 - Patch 10: In function yas537_get_calibration_data(), removed "the exact
   meaning is unknown" from comment "Writing SRST register".
 - Patch 10: Also applied "if (a && b)" suggestion at memchr_inv() by Andy
   in function yas537_get_calibration_data(). Additionally changed the second
   condition from "== 0" to "!".
 - Patch 10: In function yas537_get_calibration_data(), removed empty lines
   within switch statement. In that context, removed comment "Get data into
   these four blocks val1 to val4".
 - Patch 10: In Kconfig, simplified wording.
 - Patch 10: In function yas537_get_calibration_data() "case YAS537_VERSION_0",
   reduced indent of "for" loop by splitting it into multiple loops. I didn't
   use integer j, as it was suggested by Jonathan, because only using integer i
   is more consistant with the loop in "case YAS537_VERSION_1".
 - Accordingly, split the "for" loop in "case YAS537_VERSION_1" into two loops
   as well. Technically this isn't neccessary but it improves readability.
 - Patch 10: Added new defines of masks for YAS537 and applied these by
   FIELD_PREP and FIELD_GET in function yas537_get_calibration_data()
   within "case YAS537_VERSION_1".
 - Patch 10: In function yas537_power_on(), added spaced at "YAS537_ADCCAL + 1".

Changes in v3:
 - In patch 3 fixed 2x typo "Divide".
 - In commit message of patch 4 fixed wording "in the yas5xx_get_measure()
   function".
 - In patch 4 in the comment for the temperature calculation fixed wording
   "is the number of counts".
 - In patch 4 added defaults to switch statements.
 - Splitted stray changes into new patch 7 v3. "Add YAS537 variant" is now
   patch 8 v3. I haven't added "Reviewed-by:" tag of Linus to patch 7 v3
   because as a separate patch these changes appear in a different context.
 - In new patch 7 v3, changed printk format specifiers in the function
   yas530_get_calibration_data() to "%16ph" and in the function
   yas532_get_calibration_data() to "%14ph". The first one is also a minor
   correction in behaviour, as the calibration data array size of YAS530
   is 16 (the dev_dbg printed 14 before).
 - Rebased to linux-next to include patch bb52d3691db8 "iio: magnetometer:
   yas530: Fix memchr_inv() misuse".
 - In patch 7 v3, changed memchr_inv() line for YAS532.
 - In patch 8 v3 in the function yas537_get_calibration_data(), changed
   memchr_inv() line for YAS537.
 - Removed comment "corresponds to 0x70" at define YAS537_MAG_AVERAGE_32_MASK.
 - Added suffixes _US and _MS in defines for YAS537.
 - In the function yas537_measure(), removed comments "Read data", "Arrange
   data", "Assign data".
 - In the function yas537_measure(), replaced bitwise shift by
   get_unaligned_be16().
 - Replaced "if (h[i] < -8192)" etc. by clamp_val().
 - In the functions yas537_measure() and yas537_get_measure(), replaced 8192
   by BIT(13) and 16384 by BIT(14).
 - Fixed typo "resolution" in the function yas5xx_read_raw().
 - Fixed typo "Divide" in patch 8 v3 in the function yas5xx_read_raw().
 - In patch 8 v3 in the yas537_get_calibration_data(), changed printk format
   specifier to "%17ph"
 - In the functions yas537_measure() and yas537_get_calibration_data(), drop
   some parentheses in regmap_write().
 - In the function yas537_power_on(), added comment "Wait until the coil has
   ramped up".
 - In the function yas5xx_probe(), put YAS537 variant and version printings
   into one print.
 - In the function yas537_get_measure(), fixed wording "is the number of
   counts" in the comment for the temperature calculation.
 - In the function yas537_get_measure(), added product description document No.
   into the comment for the temperature calculation (as I first thought the
   review comment "the number" is related to this).
 - In the function yas537_get_calibration_data(), corrected comment "Get data
   into these four blocks val1 to val4".

Changes in v2:
 - Reordered the patchset by moving patch 4 v1 to patch 1 v2.
 - Removed patch 6 v1 ("Remove redundant defaults on switch devid")
 - Accordingly, added "default:" to each switch statement in patch 7.
 - Moved renamings in patch 7 v1 into a separate new patch 6 v2. I added
   the "Reviewed-by:" tag of Linus to both patches, hope that's ok, else
   feel free to comment.
 - Removed regmap reads and related debug dumps in patch 7 in function
   yas537_dump_calibration(). As this function now applies to version 1
   only, replaced switch statement by if clause.
 - Also removed "hard_offsets" debug dumps in that function.
 - Fixed typo "initialized" in commit message of patch 7.

v4: https://lore.kernel.org/linux-iio/cover.1656883851.git.jahau@rocketmail.com/T/#t
v3: https://lore.kernel.org/linux-iio/cover.1655509425.git.jahau@rocketmail.com/T/#t
v2: https://lore.kernel.org/linux-iio/cover.1655081082.git.jahau@rocketmail.com/T/#t
v1: https://lore.kernel.org/linux-iio/cover.1654727058.git.jahau@rocketmail.com/T/#t

Jakob Hauser (14):
  iio: magnetometer: yas530: Change data type of hard_offsets to signed
  iio: magnetometer: yas530: Change range of data in volatile register
  iio: magnetometer: yas530: Correct scaling of magnetic axes
  iio: magnetometer: yas530: Correct temperature handling
  iio: magnetometer: yas530: Change data type of calibration
    coefficients
  iio: magnetometer: yas530: Rename functions and registers
  iio: magnetometer: yas530: Move printk %*ph parameters out from stack
  iio: magnetometer: yas530: Apply documentation and style fixes
  iio: magnetometer: yas530: Introduce "chip_info" structure
  iio: magnetometer: yas530: Add volatile registers to "chip_info"
  iio: magnetometer: yas530: Add IIO scaling to "chip_info"
  iio: magnetometer: yas530: Add temperature calculation to "chip_info"
  iio: magnetometer: yas530: Add function pointers to "chip_info"
  iio: magnetometer: yas530: Add YAS537 variant

 drivers/iio/magnetometer/Kconfig         |   4 +-
 drivers/iio/magnetometer/yamaha-yas530.c | 854 +++++++++++++++++++----
 2 files changed, 727 insertions(+), 131 deletions(-)

-- 
2.35.1

[PATCH v5 01/14] iio: magnetometer: yas530: Change data type of hard_offsets to signed

[Jakob Hauser]

The "hard_offsets" are currently unsigned u8 but they should be signed as they
can get negative. They are signed in function yas5xx_meaure_offsets() and in the
Yamaha drivers [1][2].

[1] https://github.com/NovaFusion/android_kernel_samsung_golden/blob/cm-12.1/drivers/sensor/compass/yas.h#L156
[2] https://github.com/msm8916-mainline/android_kernel_qcom_msm8916/blob/GT-I9195I/drivers/iio/magnetometer/yas_mag_drv-yas532.c#L91

Fixes: de8860b1ed47 ("iio: magnetometer: Add driver for Yamaha YAS530")
Signed-off-by: Jakob Hauser <jahau@rocketmail.com>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
---
 drivers/iio/magnetometer/yamaha-yas530.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/magnetometer/yamaha-yas530.c b/drivers/iio/magnetometer/yamaha-yas530.c
index b2bc637150bf..40192aa46b04 100644
--- a/drivers/iio/magnetometer/yamaha-yas530.c
+++ b/drivers/iio/magnetometer/yamaha-yas530.c
@@ -132,7 +132,7 @@ struct yas5xx {
 	unsigned int version;
 	char name[16];
 	struct yas5xx_calibration calibration;
-	u8 hard_offsets[3];
+	s8 hard_offsets[3];
 	struct iio_mount_matrix orientation;
 	struct regmap *map;
 	struct regulator_bulk_data regs[2];
-- 
2.35.1

[PATCH v5 02/14] iio: magnetometer: yas530: Change range of data in volatile register

[Jakob Hauser]

In function yas5xx_volatile_reg(), register "YAS5XX_MEASURE_DATA + 8" shouldn't
be volatile as we count from 0 to 7 here.

Instead of lowering the number from 8 to 7, the operator "<=" is replaced by
"<". The size of the measure data array is 8, therefore it's more natural to
use 8 as a constant.

This change is of low importance as the "+ 8" register isn't called.

Signed-off-by: Jakob Hauser <jahau@rocketmail.com>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
---
 drivers/iio/magnetometer/yamaha-yas530.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/magnetometer/yamaha-yas530.c b/drivers/iio/magnetometer/yamaha-yas530.c
index 40192aa46b04..6fa29b96c013 100644
--- a/drivers/iio/magnetometer/yamaha-yas530.c
+++ b/drivers/iio/magnetometer/yamaha-yas530.c
@@ -527,7 +527,7 @@ static bool yas5xx_volatile_reg(struct device *dev, unsigned int reg)
 {
 	return reg == YAS5XX_ACTUATE_INIT_COIL ||
 		reg == YAS5XX_MEASURE ||
-		(reg >= YAS5XX_MEASURE_DATA && reg <= YAS5XX_MEASURE_DATA + 8);
+		(reg >= YAS5XX_MEASURE_DATA && reg < YAS5XX_MEASURE_DATA + 8);
 }
 
 /* TODO: enable regmap cache, using mark dirty and sync at runtime resume */
-- 
2.35.1

[PATCH v5 03/14] iio: magnetometer: yas530: Correct scaling of magnetic axes

[Jakob Hauser]

Looks like YAS530 raw values return picotesla and YAS532 nanotesla. Adapt
comments and scaling.

Signed-off-by: Jakob Hauser <jahau@rocketmail.com>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
---
 drivers/iio/magnetometer/yamaha-yas530.c | 31 ++++++++++++++++--------
 1 file changed, 21 insertions(+), 10 deletions(-)

diff --git a/drivers/iio/magnetometer/yamaha-yas530.c b/drivers/iio/magnetometer/yamaha-yas530.c
index 6fa29b96c013..8cad724fb328 100644
--- a/drivers/iio/magnetometer/yamaha-yas530.c
+++ b/drivers/iio/magnetometer/yamaha-yas530.c
@@ -310,8 +310,6 @@ static s32 yas5xx_linearize(struct yas5xx *yas5xx, u16 val, int axis)
  * @yo: Y axis out
  * @zo: Z axis out
  * @return: 0 on success or error code
- *
- * Returned values are in nanotesla according to some code.
  */
 static int yas5xx_get_measure(struct yas5xx *yas5xx, s32 *to, s32 *xo, s32 *yo, s32 *zo)
 {
@@ -417,14 +415,27 @@ static int yas5xx_read_raw(struct iio_dev *indio_dev,
 			*val = 1;
 			return IIO_VAL_INT;
 		}
-		/*
-		 * The axis values are in nanotesla according to the vendor
-		 * drivers, but is clearly in microtesla according to
-		 * experiments. Since 1 uT = 0.01 Gauss, we need to divide
-		 * by 100000000 (10^8) to get to Gauss from the raw value.
-		 */
-		*val = 1;
-		*val2 = 100000000;
+		switch (yas5xx->devid) {
+		case YAS530_DEVICE_ID:
+			/*
+			 * Raw values of YAS530 are in picotesla. Divide by
+			 * 100000000 (10^8) to get Gauss.
+			 */
+			*val = 1;
+			*val2 = 100000000;
+			break;
+		case YAS532_DEVICE_ID:
+			/*
+			 * Raw values of YAS532 are in nanotesla. Divide by
+			 * 100000 (10^5) to get Gauss.
+			 */
+			*val = 1;
+			*val2 = 100000;
+			break;
+		default:
+			dev_err(yas5xx->dev, "unknown device type\n");
+			return -EINVAL;
+		}
 		return IIO_VAL_FRACTIONAL;
 	default:
 		/* Unknown request */
-- 
2.35.1

[PATCH v5 04/14] iio: magnetometer: yas530: Correct temperature handling

[Jakob Hauser]

The raw temperature value is a number of counts from a certain starting
point. The resolution of the temperature counts is different for the YAS
variants.

Temperature compensation for YAS532 version AC seems to be handled differently.
It uses the deviation from 20 degree Celsius [1] whereas YAS530 and older
versions of YAS532 apply solely the t value as a multiplier [2][3].

In funtion yas5xx_read_raw(), add case IIO_CHAN_INFO_PROCESSED. Remove scale
of temperature as this isn't applied.

Additionally correct sign of temperature channel in iio_chan_spec. It's already
defined that way in the yas5xx_get_measure() function.

[1] https://github.com/msm8916-mainline/android_kernel_qcom_msm8916/blob/GT-I9195I/drivers/iio/magnetometer/yas_mag_drv-yas532.c#L442
[2] https://github.com/NovaFusion/android_kernel_samsung_golden/blob/cm-12.1/drivers/sensor/compass/yas_mag_driver-yas530.c#L881-L883
[3] https://github.com/LineageOS/android_kernel_samsung_msm8930-common/blob/lineage-18.1/drivers/sensors/geomagnetic/yas_mag_driver-yas53x.c#L856-L858

Signed-off-by: Jakob Hauser <jahau@rocketmail.com>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
---
 drivers/iio/magnetometer/yamaha-yas530.c | 99 ++++++++++++++++++------
 1 file changed, 76 insertions(+), 23 deletions(-)

diff --git a/drivers/iio/magnetometer/yamaha-yas530.c b/drivers/iio/magnetometer/yamaha-yas530.c
index 8cad724fb328..c6889a30a1b5 100644
--- a/drivers/iio/magnetometer/yamaha-yas530.c
+++ b/drivers/iio/magnetometer/yamaha-yas530.c
@@ -77,6 +77,7 @@
 #define YAS530_DATA_BITS		12
 #define YAS530_DATA_CENTER		BIT(YAS530_DATA_BITS - 1)
 #define YAS530_DATA_OVERFLOW		(BIT(YAS530_DATA_BITS) - 1)
+#define YAS530_20DEGREES		182 /* Counts starting at -62 °C */
 
 #define YAS532_DEVICE_ID		0x02 /* YAS532/YAS533 (MS-3R/F) */
 #define YAS532_VERSION_AB		0 /* YAS532/533 AB (MS-3R/F AB) */
@@ -88,7 +89,7 @@
 #define YAS532_DATA_BITS		13
 #define YAS532_DATA_CENTER		BIT(YAS532_DATA_BITS - 1)
 #define YAS532_DATA_OVERFLOW		(BIT(YAS532_DATA_BITS) - 1)
-#define YAS532_20DEGREES		390 /* Looks like Kelvin */
+#define YAS532_20DEGREES		390 /* Counts starting at -50 °C */
 
 /* These variant IDs are known from code dumps */
 #define YAS537_DEVICE_ID		0x07 /* YAS537 (MS-3T) */
@@ -314,7 +315,7 @@ static s32 yas5xx_linearize(struct yas5xx *yas5xx, u16 val, int axis)
 static int yas5xx_get_measure(struct yas5xx *yas5xx, s32 *to, s32 *xo, s32 *yo, s32 *zo)
 {
 	struct yas5xx_calibration *c = &yas5xx->calibration;
-	u16 t, x, y1, y2;
+	u16 t_ref, t, x, y1, y2;
 	/* These are "signed x, signed y1 etc */
 	s32 sx, sy1, sy2, sy, sz;
 	int ret;
@@ -329,16 +330,46 @@ static int yas5xx_get_measure(struct yas5xx *yas5xx, s32 *to, s32 *xo, s32 *yo,
 	sy1 = yas5xx_linearize(yas5xx, y1, 1);
 	sy2 = yas5xx_linearize(yas5xx, y2, 2);
 
-	/*
-	 * Temperature compensation for x, y1, y2 respectively:
-	 *
-	 *          Cx * t
-	 * x' = x - ------
-	 *           100
-	 */
-	sx = sx - (c->Cx * t) / 100;
-	sy1 = sy1 - (c->Cy1 * t) / 100;
-	sy2 = sy2 - (c->Cy2 * t) / 100;
+	/* Set the temperature reference value (unit: counts) */
+	switch (yas5xx->devid) {
+	case YAS530_DEVICE_ID:
+		t_ref = YAS530_20DEGREES;
+		break;
+	case YAS532_DEVICE_ID:
+		t_ref = YAS532_20DEGREES;
+		break;
+	default:
+		dev_err(yas5xx->dev, "unknown device type\n");
+		return -EINVAL;
+	}
+
+	/* Temperature compensation for x, y1, y2 respectively */
+	if (yas5xx->devid == YAS532_DEVICE_ID &&
+	    yas5xx->version == YAS532_VERSION_AC) {
+		/*
+		 * YAS532 version AC uses the temperature deviation as a
+		 * multiplier.
+		 *
+		 *          Cx * (t - t_ref)
+		 * x' = x - ----------------
+		 *                100
+		 */
+		sx = sx - (c->Cx * (t - t_ref)) / 100;
+		sy1 = sy1 - (c->Cy1 * (t - t_ref)) / 100;
+		sy2 = sy2 - (c->Cy2 * (t - t_ref)) / 100;
+	} else {
+		/*
+		 * YAS530 and YAS532 version AB use solely the t value as a
+		 * multiplier.
+		 *
+		 *          Cx * t
+		 * x' = x - ------
+		 *           100
+		 */
+		sx = sx - (c->Cx * t) / 100;
+		sy1 = sy1 - (c->Cy1 * t) / 100;
+		sy2 = sy2 - (c->Cy2 * t) / 100;
+	}
 
 	/*
 	 * Break y1 and y2 into y and z, y1 and y2 are apparently encoding
@@ -347,11 +378,37 @@ static int yas5xx_get_measure(struct yas5xx *yas5xx, s32 *to, s32 *xo, s32 *yo,
 	sy = sy1 - sy2;
 	sz = -sy1 - sy2;
 
-	/*
-	 * FIXME: convert to Celsius? Just guessing this is given
-	 * as 1/10:s of degrees so multiply by 100 to get millicentigrades.
-	 */
-	*to = t * 100;
+	/* Process temperature readout */
+	switch (yas5xx->devid) {
+	case YAS530_DEVICE_ID:
+		/*
+		 * Raw temperature value t is the number of counts starting
+		 * at -62 °C. Reference value t_ref is the number of counts
+		 * between -62 °C and 20 °C (82 °C range).
+		 *
+		 * Temperature in °C would be (82 / t_ref * t) - 62.
+		 *
+		 * Contrary to this, perform multiplication first and division
+		 * second due to calculating with integers.
+		 *
+		 * To get a nicer result, calculate with 1/10:s degrees Celsius
+		 * and finally multiply by 100 to return millidegrees Celsius.
+		 */
+		*to = ((820 * t / t_ref) - 620) * 100;
+		break;
+	case YAS532_DEVICE_ID:
+		/*
+		 * Actually same procedure for YAS532 but the starting point is
+		 * at -50 °C. Reference value t_ref is the number of counts
+		 * between -50 °C and 20 °C (70 °C range).
+		 */
+		*to = ((700 * t / t_ref) - 500) * 100;
+		break;
+	default:
+		dev_err(yas5xx->dev, "unknown device type\n");
+		return -EINVAL;
+	}
+
 	/*
 	 * Calibrate [x,y,z] with some formulas like this:
 	 *
@@ -384,6 +441,7 @@ static int yas5xx_read_raw(struct iio_dev *indio_dev,
 	int ret;
 
 	switch (mask) {
+	case IIO_CHAN_INFO_PROCESSED:
 	case IIO_CHAN_INFO_RAW:
 		pm_runtime_get_sync(yas5xx->dev);
 		ret = yas5xx_get_measure(yas5xx, &t, &x, &y, &z);
@@ -410,11 +468,6 @@ static int yas5xx_read_raw(struct iio_dev *indio_dev,
 		}
 		return IIO_VAL_INT;
 	case IIO_CHAN_INFO_SCALE:
-		if (chan->address == 0) {
-			/* Temperature is unscaled */
-			*val = 1;
-			return IIO_VAL_INT;
-		}
 		switch (yas5xx->devid) {
 		case YAS530_DEVICE_ID:
 			/*
@@ -516,7 +569,7 @@ static const struct iio_chan_spec yas5xx_channels[] = {
 		.address = 0,
 		.scan_index = 0,
 		.scan_type = {
-			.sign = 'u',
+			.sign = 's',
 			.realbits = 32,
 			.storagebits = 32,
 			.endianness = IIO_CPU,
-- 
2.35.1

[PATCH v5 05/14] iio: magnetometer: yas530: Change data type of calibration coefficients

[Jakob Hauser]

This is a preparation for adding YAS537 variant.

YAS537 uses other data types on the calibration coefficients [1] than YAS530 [2]
and YAS532 [3].

On YAS537, at least for a4 and a7 this could matter because 8-bit unsigned data
from the register gets stored into a signed data type, therefore this should be
8-bit as well.

For YAS530/532, on the other hand, it doesn't seem to matter. The size of a2-a9
and k is smaller than 8-bit at extraction, also the applied math is low. And
Cx/Cy1/Cy2, now being defined as signed 16-bit, are extracted as unsigned 8-bit
and undergo only minor math.

[1] https://github.com/msm8916-mainline/android_kernel_qcom_msm8916/blob/GT-I9195I/drivers/iio/magnetometer/yas_mag_drv-yas537.c#L76-L78
[2] https://github.com/NovaFusion/android_kernel_samsung_golden/blob/cm-12.1/drivers/sensor/compass/yas_mag_driver-yas530.c#L526-L527
[3] https://github.com/msm8916-mainline/android_kernel_qcom_msm8916/blob/GT-I9195I/drivers/iio/magnetometer/yas_mag_drv-yas532.c#L76-L77

Signed-off-by: Jakob Hauser <jahau@rocketmail.com>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
---
 drivers/iio/magnetometer/yamaha-yas530.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/magnetometer/yamaha-yas530.c b/drivers/iio/magnetometer/yamaha-yas530.c
index c6889a30a1b5..839baeca673a 100644
--- a/drivers/iio/magnetometer/yamaha-yas530.c
+++ b/drivers/iio/magnetometer/yamaha-yas530.c
@@ -103,9 +103,11 @@ struct yas5xx_calibration {
 	s32 r[3];
 	u32 f[3];
 	/* Temperature compensation calibration */
-	s32 Cx, Cy1, Cy2;
+	s16 Cx, Cy1, Cy2;
 	/* Misc calibration coefficients */
-	s32 a2, a3, a4, a5, a6, a7, a8, a9, k;
+	s8  a2, a3, a4, a6, a7, a8;
+	s16 a5, a9;
+	u8  k;
 	/* clock divider */
 	u8 dck;
 };
-- 
2.35.1

[PATCH v5 06/14] iio: magnetometer: yas530: Rename functions and registers

[Jakob Hauser]

This is a preparation for adding YAS537 variant.

Functions that are used only by YAS530, YAS532 and YAS533 are renamed from
yas5xx to yas530. Same for the registers.

To avoid part listing in function and registers names, the name of the first
variant is used. Where appropriate, comments were added that these functions
are used by more than one variant.

Functions that will be used by all variants including YAS537 remain in the
naming scheme yas5xx. Or YAS5XX for registers, respectively.

Signed-off-by: Jakob Hauser <jahau@rocketmail.com>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/iio/magnetometer/yamaha-yas530.c | 126 +++++++++++++----------
 1 file changed, 70 insertions(+), 56 deletions(-)

diff --git a/drivers/iio/magnetometer/yamaha-yas530.c b/drivers/iio/magnetometer/yamaha-yas530.c
index 839baeca673a..b27cc2b432ee 100644
--- a/drivers/iio/magnetometer/yamaha-yas530.c
+++ b/drivers/iio/magnetometer/yamaha-yas530.c
@@ -40,20 +40,22 @@
 
 #include <asm/unaligned.h>
 
-/* This register map covers YAS530 and YAS532 but differs in YAS 537 and YAS539 */
+/* Commonly used registers */
 #define YAS5XX_DEVICE_ID		0x80
-#define YAS5XX_ACTUATE_INIT_COIL	0x81
-#define YAS5XX_MEASURE			0x82
-#define YAS5XX_CONFIG			0x83
-#define YAS5XX_MEASURE_INTERVAL		0x84
-#define YAS5XX_OFFSET_X			0x85 /* [-31 .. 31] */
-#define YAS5XX_OFFSET_Y1		0x86 /* [-31 .. 31] */
-#define YAS5XX_OFFSET_Y2		0x87 /* [-31 .. 31] */
-#define YAS5XX_TEST1			0x88
-#define YAS5XX_TEST2			0x89
-#define YAS5XX_CAL			0x90
 #define YAS5XX_MEASURE_DATA		0xB0
 
+/* These registers are used by YAS530, YAS532 and YAS533 */
+#define YAS530_ACTUATE_INIT_COIL	0x81
+#define YAS530_MEASURE			0x82
+#define YAS530_CONFIG			0x83
+#define YAS530_MEASURE_INTERVAL		0x84
+#define YAS530_OFFSET_X			0x85 /* [-31 .. 31] */
+#define YAS530_OFFSET_Y1		0x86 /* [-31 .. 31] */
+#define YAS530_OFFSET_Y2		0x87 /* [-31 .. 31] */
+#define YAS530_TEST1			0x88
+#define YAS530_TEST2			0x89
+#define YAS530_CAL			0x90
+
 /* Bits in the YAS5xx config register */
 #define YAS5XX_CONFIG_INTON		BIT(0) /* Interrupt on? */
 #define YAS5XX_CONFIG_INTHACT		BIT(1) /* Interrupt active high? */
@@ -182,15 +184,17 @@ static u16 yas532_extract_axis(u8 *data)
 }
 
 /**
- * yas5xx_measure() - Make a measure from the hardware
+ * yas530_measure() - Make a measure from the hardware
  * @yas5xx: The device state
  * @t: the raw temperature measurement
  * @x: the raw x axis measurement
  * @y1: the y1 axis measurement
  * @y2: the y2 axis measurement
  * @return: 0 on success or error code
+ *
+ * Used by YAS530, YAS532 and YAS533
  */
-static int yas5xx_measure(struct yas5xx *yas5xx, u16 *t, u16 *x, u16 *y1, u16 *y2)
+static int yas530_measure(struct yas5xx *yas5xx, u16 *t, u16 *x, u16 *y1, u16 *y2)
 {
 	unsigned int busy;
 	u8 data[8];
@@ -198,7 +202,7 @@ static int yas5xx_measure(struct yas5xx *yas5xx, u16 *t, u16 *x, u16 *y1, u16 *y
 	u16 val;
 
 	mutex_lock(&yas5xx->lock);
-	ret = regmap_write(yas5xx->map, YAS5XX_MEASURE, YAS5XX_MEASURE_START);
+	ret = regmap_write(yas5xx->map, YAS530_MEASURE, YAS5XX_MEASURE_START);
 	if (ret < 0)
 		goto out_unlock;
 
@@ -264,7 +268,8 @@ static int yas5xx_measure(struct yas5xx *yas5xx, u16 *t, u16 *x, u16 *y1, u16 *y
 	return ret;
 }
 
-static s32 yas5xx_linearize(struct yas5xx *yas5xx, u16 val, int axis)
+/* Used by YAS530, YAS532 and YAS533 */
+static s32 yas530_linearize(struct yas5xx *yas5xx, u16 val, int axis)
 {
 	struct yas5xx_calibration *c = &yas5xx->calibration;
 	static const s32 yas532ac_coef[] = {
@@ -306,15 +311,17 @@ static s32 yas5xx_linearize(struct yas5xx *yas5xx, u16 val, int axis)
 }
 
 /**
- * yas5xx_get_measure() - Measure a sample of all axis and process
+ * yas530_get_measure() - Measure a sample of all axis and process
  * @yas5xx: The device state
  * @to: Temperature out
  * @xo: X axis out
  * @yo: Y axis out
  * @zo: Z axis out
  * @return: 0 on success or error code
+ *
+ * Used by YAS530, YAS532 and YAS533
  */
-static int yas5xx_get_measure(struct yas5xx *yas5xx, s32 *to, s32 *xo, s32 *yo, s32 *zo)
+static int yas530_get_measure(struct yas5xx *yas5xx, s32 *to, s32 *xo, s32 *yo, s32 *zo)
 {
 	struct yas5xx_calibration *c = &yas5xx->calibration;
 	u16 t_ref, t, x, y1, y2;
@@ -323,14 +330,14 @@ static int yas5xx_get_measure(struct yas5xx *yas5xx, s32 *to, s32 *xo, s32 *yo,
 	int ret;
 
 	/* We first get raw data that needs to be translated to [x,y,z] */
-	ret = yas5xx_measure(yas5xx, &t, &x, &y1, &y2);
+	ret = yas530_measure(yas5xx, &t, &x, &y1, &y2);
 	if (ret)
 		return ret;
 
 	/* Do some linearization if available */
-	sx = yas5xx_linearize(yas5xx, x, 0);
-	sy1 = yas5xx_linearize(yas5xx, y1, 1);
-	sy2 = yas5xx_linearize(yas5xx, y2, 2);
+	sx = yas530_linearize(yas5xx, x, 0);
+	sy1 = yas530_linearize(yas5xx, y1, 1);
+	sy2 = yas530_linearize(yas5xx, y2, 2);
 
 	/* Set the temperature reference value (unit: counts) */
 	switch (yas5xx->devid) {
@@ -446,7 +453,7 @@ static int yas5xx_read_raw(struct iio_dev *indio_dev,
 	case IIO_CHAN_INFO_PROCESSED:
 	case IIO_CHAN_INFO_RAW:
 		pm_runtime_get_sync(yas5xx->dev);
-		ret = yas5xx_get_measure(yas5xx, &t, &x, &y, &z);
+		ret = yas530_get_measure(yas5xx, &t, &x, &y, &z);
 		pm_runtime_mark_last_busy(yas5xx->dev);
 		pm_runtime_put_autosuspend(yas5xx->dev);
 		if (ret)
@@ -505,7 +512,7 @@ static void yas5xx_fill_buffer(struct iio_dev *indio_dev)
 	int ret;
 
 	pm_runtime_get_sync(yas5xx->dev);
-	ret = yas5xx_get_measure(yas5xx, &t, &x, &y, &z);
+	ret = yas530_get_measure(yas5xx, &t, &x, &y, &z);
 	pm_runtime_mark_last_busy(yas5xx->dev);
 	pm_runtime_put_autosuspend(yas5xx->dev);
 	if (ret) {
@@ -591,8 +598,8 @@ static const struct iio_info yas5xx_info = {
 
 static bool yas5xx_volatile_reg(struct device *dev, unsigned int reg)
 {
-	return reg == YAS5XX_ACTUATE_INIT_COIL ||
-		reg == YAS5XX_MEASURE ||
+	return reg == YAS530_ACTUATE_INIT_COIL ||
+		reg == YAS530_MEASURE ||
 		(reg >= YAS5XX_MEASURE_DATA && reg < YAS5XX_MEASURE_DATA + 8);
 }
 
@@ -605,11 +612,13 @@ static const struct regmap_config yas5xx_regmap_config = {
 };
 
 /**
- * yas53x_extract_calibration() - extracts the a2-a9 and k calibration
+ * yas530_extract_calibration() - extracts the a2-a9 and k calibration
  * @data: the bitfield to use
  * @c: the calibration to populate
+ *
+ * Used by YAS530, YAS532 and YAS533
  */
-static void yas53x_extract_calibration(u8 *data, struct yas5xx_calibration *c)
+static void yas530_extract_calibration(u8 *data, struct yas5xx_calibration *c)
 {
 	u64 val = get_unaligned_be64(data);
 
@@ -647,12 +656,12 @@ static int yas530_get_calibration_data(struct yas5xx *yas5xx)
 	int ret;
 
 	/* Dummy read, first read is ALWAYS wrong */
-	ret = regmap_bulk_read(yas5xx->map, YAS5XX_CAL, data, sizeof(data));
+	ret = regmap_bulk_read(yas5xx->map, YAS530_CAL, data, sizeof(data));
 	if (ret)
 		return ret;
 
 	/* Actual calibration readout */
-	ret = regmap_bulk_read(yas5xx->map, YAS5XX_CAL, data, sizeof(data));
+	ret = regmap_bulk_read(yas5xx->map, YAS530_CAL, data, sizeof(data));
 	if (ret)
 		return ret;
 	dev_dbg(yas5xx->dev, "calibration data: %*ph\n", 14, data);
@@ -664,7 +673,7 @@ static int yas530_get_calibration_data(struct yas5xx *yas5xx)
 	c->Cx = data[0] * 6 - 768;
 	c->Cy1 = data[1] * 6 - 768;
 	c->Cy2 = data[2] * 6 - 768;
-	yas53x_extract_calibration(&data[3], c);
+	yas530_extract_calibration(&data[3], c);
 
 	/*
 	 * Extract linearization:
@@ -695,11 +704,11 @@ static int yas532_get_calibration_data(struct yas5xx *yas5xx)
 	int ret;
 
 	/* Dummy read, first read is ALWAYS wrong */
-	ret = regmap_bulk_read(yas5xx->map, YAS5XX_CAL, data, sizeof(data));
+	ret = regmap_bulk_read(yas5xx->map, YAS530_CAL, data, sizeof(data));
 	if (ret)
 		return ret;
 	/* Actual calibration readout */
-	ret = regmap_bulk_read(yas5xx->map, YAS5XX_CAL, data, sizeof(data));
+	ret = regmap_bulk_read(yas5xx->map, YAS530_CAL, data, sizeof(data));
 	if (ret)
 		return ret;
 	dev_dbg(yas5xx->dev, "calibration data: %*ph\n", 14, data);
@@ -718,7 +727,7 @@ static int yas532_get_calibration_data(struct yas5xx *yas5xx)
 	c->Cx = data[0] * 10 - 1280;
 	c->Cy1 = data[1] * 10 - 1280;
 	c->Cy2 = data[2] * 10 - 1280;
-	yas53x_extract_calibration(&data[3], c);
+	yas530_extract_calibration(&data[3], c);
 	/*
 	 * Extract linearization:
 	 * Linearization layout in the 32 bits at byte 10:
@@ -741,7 +750,8 @@ static int yas532_get_calibration_data(struct yas5xx *yas5xx)
 	return 0;
 }
 
-static void yas5xx_dump_calibration(struct yas5xx *yas5xx)
+/* Used by YAS530, YAS532 and YAS533 */
+static void yas530_dump_calibration(struct yas5xx *yas5xx)
 {
 	struct yas5xx_calibration *c = &yas5xx->calibration;
 
@@ -764,20 +774,22 @@ static void yas5xx_dump_calibration(struct yas5xx *yas5xx)
 	dev_dbg(yas5xx->dev, "dck = %d\n", c->dck);
 }
 
-static int yas5xx_set_offsets(struct yas5xx *yas5xx, s8 ox, s8 oy1, s8 oy2)
+/* Used by YAS530, YAS532 and YAS533 */
+static int yas530_set_offsets(struct yas5xx *yas5xx, s8 ox, s8 oy1, s8 oy2)
 {
 	int ret;
 
-	ret = regmap_write(yas5xx->map, YAS5XX_OFFSET_X, ox);
+	ret = regmap_write(yas5xx->map, YAS530_OFFSET_X, ox);
 	if (ret)
 		return ret;
-	ret = regmap_write(yas5xx->map, YAS5XX_OFFSET_Y1, oy1);
+	ret = regmap_write(yas5xx->map, YAS530_OFFSET_Y1, oy1);
 	if (ret)
 		return ret;
-	return regmap_write(yas5xx->map, YAS5XX_OFFSET_Y2, oy2);
+	return regmap_write(yas5xx->map, YAS530_OFFSET_Y2, oy2);
 }
 
-static s8 yas5xx_adjust_offset(s8 old, int bit, u16 center, u16 measure)
+/* Used by YAS530, YAS532 and YAS533 */
+static s8 yas530_adjust_offset(s8 old, int bit, u16 center, u16 measure)
 {
 	if (measure > center)
 		return old + BIT(bit);
@@ -786,7 +798,8 @@ static s8 yas5xx_adjust_offset(s8 old, int bit, u16 center, u16 measure)
 	return old;
 }
 
-static int yas5xx_meaure_offsets(struct yas5xx *yas5xx)
+/* Used by YAS530, YAS532 and YAS533 */
+static int yas530_measure_offsets(struct yas5xx *yas5xx)
 {
 	int ret;
 	u16 center;
@@ -795,7 +808,7 @@ static int yas5xx_meaure_offsets(struct yas5xx *yas5xx)
 	int i;
 
 	/* Actuate the init coil and measure offsets */
-	ret = regmap_write(yas5xx->map, YAS5XX_ACTUATE_INIT_COIL, 0);
+	ret = regmap_write(yas5xx->map, YAS530_ACTUATE_INIT_COIL, 0);
 	if (ret)
 		return ret;
 
@@ -829,26 +842,26 @@ static int yas5xx_meaure_offsets(struct yas5xx *yas5xx)
 	oy2 = 0;
 
 	for (i = 4; i >= 0; i--) {
-		ret = yas5xx_set_offsets(yas5xx, ox, oy1, oy2);
+		ret = yas530_set_offsets(yas5xx, ox, oy1, oy2);
 		if (ret)
 			return ret;
 
-		ret = yas5xx_measure(yas5xx, &t, &x, &y1, &y2);
+		ret = yas530_measure(yas5xx, &t, &x, &y1, &y2);
 		if (ret)
 			return ret;
 		dev_dbg(yas5xx->dev, "measurement %d: x=%d, y1=%d, y2=%d\n",
 			5-i, x, y1, y2);
 
-		ox = yas5xx_adjust_offset(ox, i, center, x);
-		oy1 = yas5xx_adjust_offset(oy1, i, center, y1);
-		oy2 = yas5xx_adjust_offset(oy2, i, center, y2);
+		ox = yas530_adjust_offset(ox, i, center, x);
+		oy1 = yas530_adjust_offset(oy1, i, center, y1);
+		oy2 = yas530_adjust_offset(oy2, i, center, y2);
 	}
 
 	/* Needed for calibration algorithm */
 	yas5xx->hard_offsets[0] = ox;
 	yas5xx->hard_offsets[1] = oy1;
 	yas5xx->hard_offsets[2] = oy2;
-	ret = yas5xx_set_offsets(yas5xx, ox, oy1, oy2);
+	ret = yas530_set_offsets(yas5xx, ox, oy1, oy2);
 	if (ret)
 		return ret;
 
@@ -857,27 +870,28 @@ static int yas5xx_meaure_offsets(struct yas5xx *yas5xx)
 	return 0;
 }
 
-static int yas5xx_power_on(struct yas5xx *yas5xx)
+/* Used by YAS530, YAS532 and YAS533 */
+static int yas530_power_on(struct yas5xx *yas5xx)
 {
 	unsigned int val;
 	int ret;
 
 	/* Zero the test registers */
-	ret = regmap_write(yas5xx->map, YAS5XX_TEST1, 0);
+	ret = regmap_write(yas5xx->map, YAS530_TEST1, 0);
 	if (ret)
 		return ret;
-	ret = regmap_write(yas5xx->map, YAS5XX_TEST2, 0);
+	ret = regmap_write(yas5xx->map, YAS530_TEST2, 0);
 	if (ret)
 		return ret;
 
 	/* Set up for no interrupts, calibrated clock divider */
 	val = FIELD_PREP(YAS5XX_CONFIG_CCK_MASK, yas5xx->calibration.dck);
-	ret = regmap_write(yas5xx->map, YAS5XX_CONFIG, val);
+	ret = regmap_write(yas5xx->map, YAS530_CONFIG, val);
 	if (ret)
 		return ret;
 
 	/* Measure interval 0 (back-to-back?)  */
-	return regmap_write(yas5xx->map, YAS5XX_MEASURE_INTERVAL, 0);
+	return regmap_write(yas5xx->map, YAS530_MEASURE_INTERVAL, 0);
 }
 
 static int yas5xx_probe(struct i2c_client *i2c,
@@ -959,11 +973,11 @@ static int yas5xx_probe(struct i2c_client *i2c,
 		goto assert_reset;
 	}
 
-	yas5xx_dump_calibration(yas5xx);
-	ret = yas5xx_power_on(yas5xx);
+	yas530_dump_calibration(yas5xx);
+	ret = yas530_power_on(yas5xx);
 	if (ret)
 		goto assert_reset;
-	ret = yas5xx_meaure_offsets(yas5xx);
+	ret = yas530_measure_offsets(yas5xx);
 	if (ret)
 		goto assert_reset;
 
@@ -1062,7 +1076,7 @@ static int __maybe_unused yas5xx_runtime_resume(struct device *dev)
 	usleep_range(31000, 40000);
 	gpiod_set_value_cansleep(yas5xx->reset, 0);
 
-	ret = yas5xx_power_on(yas5xx);
+	ret = yas530_power_on(yas5xx);
 	if (ret) {
 		dev_err(dev, "cannot power on\n");
 		goto out_reset;
-- 
2.35.1

Re: [PATCH v5 06/14] iio: magnetometer: yas530: Rename functions and registers

[Andy Shevchenko]

On Mon, Aug 8, 2022 at 1:03 AM Jakob Hauser <jahau@rocketmail.com> wrote:
>
> This is a preparation for adding YAS537 variant.

the YAS537

> Functions that are used only by YAS530, YAS532 and YAS533 are renamed from
> yas5xx to yas530. Same for the registers.
>
> To avoid part listing in function and registers names, the name of the first
> variant is used. Where appropriate, comments were added that these functions
> are used by more than one variant.
>
> Functions that will be used by all variants including YAS537 remain in the
> naming scheme yas5xx. Or YAS5XX for registers, respectively.

...

>  /**
> - * yas5xx_get_measure() - Measure a sample of all axis and process
> + * yas530_get_measure() - Measure a sample of all axis and process
>   * @yas5xx: The device state
>   * @to: Temperature out
>   * @xo: X axis out
>   * @yo: Y axis out
>   * @zo: Z axis out
>   * @return: 0 on success or error code

> + * Used by YAS530, YAS532 and YAS533

In this case (multi-line comment) the period should be added to the
sentence. Ditto for other similar cases.

>   */

Otherwise the change looks good. So, if you address these,
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 06/14] iio: magnetometer: yas530: Rename functions and registers

[Jakob Hauser]

Hi Andy,

On 08.08.22 13:08, Andy Shevchenko wrote:
> On Mon, Aug 8, 2022 at 1:03 AM Jakob Hauser <jahau@rocketmail.com> wrote:
>>
>> This is a preparation for adding YAS537 variant.
> 
> the YAS537

OK

>> Functions that are used only by YAS530, YAS532 and YAS533 are renamed from
>> yas5xx to yas530. Same for the registers.
>>
>> To avoid part listing in function and registers names, the name of the first
>> variant is used. Where appropriate, comments were added that these functions
>> are used by more than one variant.
>>
>> Functions that will be used by all variants including YAS537 remain in the
>> naming scheme yas5xx. Or YAS5XX for registers, respectively.
> 
> ..
> 
>>  /**
>> - * yas5xx_get_measure() - Measure a sample of all axis and process
>> + * yas530_get_measure() - Measure a sample of all axis and process
>>   * @yas5xx: The device state
>>   * @to: Temperature out
>>   * @xo: X axis out
>>   * @yo: Y axis out
>>   * @zo: Z axis out
>>   * @return: 0 on success or error code
> 
>> + * Used by YAS530, YAS532 and YAS533
> 
> In this case (multi-line comment) the period should be added to the
> sentence. Ditto for other similar cases.

OK

>>   */

...

Kind regards,
Jakob

[PATCH v5 07/14] iio: magnetometer: yas530: Move printk %*ph parameters out from stack

[Jakob Hauser]

Use less stack by modifying %*ph parameters.

Additionally, in the function yas530_get_calibration_data(), the debug dump was
extended to 16 elements as this is the size of the calibration data array of
YAS530.

Signed-off-by: Jakob Hauser <jahau@rocketmail.com>
Suggested-by: Andy Shevchenko <andy.shevchenko@gmail.com>
---
 drivers/iio/magnetometer/yamaha-yas530.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/magnetometer/yamaha-yas530.c b/drivers/iio/magnetometer/yamaha-yas530.c
index b27cc2b432ee..48995176fa39 100644
--- a/drivers/iio/magnetometer/yamaha-yas530.c
+++ b/drivers/iio/magnetometer/yamaha-yas530.c
@@ -664,7 +664,7 @@ static int yas530_get_calibration_data(struct yas5xx *yas5xx)
 	ret = regmap_bulk_read(yas5xx->map, YAS530_CAL, data, sizeof(data));
 	if (ret)
 		return ret;
-	dev_dbg(yas5xx->dev, "calibration data: %*ph\n", 14, data);
+	dev_dbg(yas5xx->dev, "calibration data: %16ph\n", data);
 
 	add_device_randomness(data, sizeof(data));
 	yas5xx->version = data[15] & GENMASK(1, 0);
@@ -711,7 +711,7 @@ static int yas532_get_calibration_data(struct yas5xx *yas5xx)
 	ret = regmap_bulk_read(yas5xx->map, YAS530_CAL, data, sizeof(data));
 	if (ret)
 		return ret;
-	dev_dbg(yas5xx->dev, "calibration data: %*ph\n", 14, data);
+	dev_dbg(yas5xx->dev, "calibration data: %14ph\n", data);
 
 	/* Sanity check, is this all zeroes? */
 	if (memchr_inv(data, 0x00, 13) == NULL) {
-- 
2.35.1

Re: [PATCH v5 07/14] iio: magnetometer: yas530: Move printk %*ph parameters out from stack

[Andy Shevchenko]

On Mon, Aug 8, 2022 at 1:07 AM Jakob Hauser <jahau@rocketmail.com> wrote:
>
> Use less stack by modifying %*ph parameters.
>
> Additionally, in the function yas530_get_calibration_data(), the debug dump was

Additionally --> While at it

(The difference is that "additionally" means you need to split to two
changes, which makes a little sense in this case)

> extended to 16 elements as this is the size of the calibration data array of
> YAS530.

Otherwise looks good,
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

> Signed-off-by: Jakob Hauser <jahau@rocketmail.com>
> Suggested-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> ---
>  drivers/iio/magnetometer/yamaha-yas530.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/iio/magnetometer/yamaha-yas530.c b/drivers/iio/magnetometer/yamaha-yas530.c
> index b27cc2b432ee..48995176fa39 100644
> --- a/drivers/iio/magnetometer/yamaha-yas530.c
> +++ b/drivers/iio/magnetometer/yamaha-yas530.c
> @@ -664,7 +664,7 @@ static int yas530_get_calibration_data(struct yas5xx *yas5xx)
>         ret = regmap_bulk_read(yas5xx->map, YAS530_CAL, data, sizeof(data));
>         if (ret)
>                 return ret;
> -       dev_dbg(yas5xx->dev, "calibration data: %*ph\n", 14, data);
> +       dev_dbg(yas5xx->dev, "calibration data: %16ph\n", data);
>
>         add_device_randomness(data, sizeof(data));
>         yas5xx->version = data[15] & GENMASK(1, 0);
> @@ -711,7 +711,7 @@ static int yas532_get_calibration_data(struct yas5xx *yas5xx)
>         ret = regmap_bulk_read(yas5xx->map, YAS530_CAL, data, sizeof(data));
>         if (ret)
>                 return ret;
> -       dev_dbg(yas5xx->dev, "calibration data: %*ph\n", 14, data);
> +       dev_dbg(yas5xx->dev, "calibration data: %14ph\n", data);
>
>         /* Sanity check, is this all zeroes? */
>         if (memchr_inv(data, 0x00, 13) == NULL) {

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 07/14] iio: magnetometer: yas530: Move printk %*ph parameters out from stack

[Jakob Hauser]

Hi Andy,

On 08.08.22 13:10, Andy Shevchenko wrote:
> On Mon, Aug 8, 2022 at 1:07 AM Jakob Hauser <jahau@rocketmail.com> wrote:
>>
>> Use less stack by modifying %*ph parameters.
>>
>> Additionally, in the function yas530_get_calibration_data(), the debug dump was
> 
> Additionally --> While at it
> 
> (The difference is that "additionally" means you need to split to two
> changes, which makes a little sense in this case)

OK

>> extended to 16 elements as this is the size of the calibration data array of
>> YAS530.

...

Kind regards,
Jakob

[PATCH v5 08/14] iio: magnetometer: yas530: Apply documentation and style fixes

[Jakob Hauser]

This commit gathers several minor changes.

In the device examples, "Xiaomi" is too generic, specific devices should be
listed here. E.g. Xiaomi Redmi 2 seems to have YAS537 but it's not fully clear
if this applies to all its variants. Samsung Galaxy S7 is often quoted in
conjunction with YAS537.

Removed defines for device IDs of YAS537 and YAS539, they are not needed so far.

Signed-off-by: Jakob Hauser <jahau@rocketmail.com>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/iio/magnetometer/yamaha-yas530.c | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/drivers/iio/magnetometer/yamaha-yas530.c b/drivers/iio/magnetometer/yamaha-yas530.c
index 48995176fa39..bf0aa64ac1a2 100644
--- a/drivers/iio/magnetometer/yamaha-yas530.c
+++ b/drivers/iio/magnetometer/yamaha-yas530.c
@@ -10,7 +10,7 @@
  * (YAS534 is a magnetic switch, not handled)
  * YAS535 MS-6C
  * YAS536 MS-3W
- * YAS537 MS-3T (2015 Samsung Galaxy S6, Note 5, Xiaomi)
+ * YAS537 MS-3T (2015 Samsung Galaxy S6, Note 5, Galaxy S7)
  * YAS539 MS-3S (2018 Samsung Galaxy A7 SM-A750FN)
  *
  * Code functions found in the MPU3050 YAS530 and YAS532 drivers
@@ -93,10 +93,6 @@
 #define YAS532_DATA_OVERFLOW		(BIT(YAS532_DATA_BITS) - 1)
 #define YAS532_20DEGREES		390 /* Counts starting at -50 °C */
 
-/* These variant IDs are known from code dumps */
-#define YAS537_DEVICE_ID		0x07 /* YAS537 (MS-3T) */
-#define YAS539_DEVICE_ID		0x08 /* YAS539 (MS-3S) */
-
 /* Turn off device regulators etc after 5 seconds of inactivity */
 #define YAS5XX_AUTOSUSPEND_DELAY_MS	5000
 
@@ -325,7 +321,7 @@ static int yas530_get_measure(struct yas5xx *yas5xx, s32 *to, s32 *xo, s32 *yo,
 {
 	struct yas5xx_calibration *c = &yas5xx->calibration;
 	u16 t_ref, t, x, y1, y2;
-	/* These are "signed x, signed y1 etc */
+	/* These are signed x, signed y1 etc */
 	s32 sx, sy1, sy2, sy, sz;
 	int ret;
 
@@ -666,7 +662,10 @@ static int yas530_get_calibration_data(struct yas5xx *yas5xx)
 		return ret;
 	dev_dbg(yas5xx->dev, "calibration data: %16ph\n", data);
 
+	/* Contribute calibration data to the input pool for kernel entropy */
 	add_device_randomness(data, sizeof(data));
+
+	/* Extract version */
 	yas5xx->version = data[15] & GENMASK(1, 0);
 
 	/* Extract the calibration from the bitfield */
@@ -693,6 +692,7 @@ static int yas530_get_calibration_data(struct yas5xx *yas5xx)
 	c->r[0] = sign_extend32(FIELD_GET(GENMASK(28, 23), val), 5);
 	c->r[1] = sign_extend32(FIELD_GET(GENMASK(20, 15), val), 5);
 	c->r[2] = sign_extend32(FIELD_GET(GENMASK(12, 7), val), 5);
+
 	return 0;
 }
 
@@ -714,12 +714,12 @@ static int yas532_get_calibration_data(struct yas5xx *yas5xx)
 	dev_dbg(yas5xx->dev, "calibration data: %14ph\n", data);
 
 	/* Sanity check, is this all zeroes? */
-	if (memchr_inv(data, 0x00, 13) == NULL) {
-		if (!(data[13] & BIT(7)))
-			dev_warn(yas5xx->dev, "calibration is blank!\n");
-	}
+	if (!memchr_inv(data, 0x00, 13) && !(data[13] & BIT(7)))
+		dev_warn(yas5xx->dev, "calibration is blank!\n");
 
+	/* Contribute calibration data to the input pool for kernel entropy */
 	add_device_randomness(data, sizeof(data));
+
 	/* Only one bit of version info reserved here as far as we know */
 	yas5xx->version = data[13] & BIT(0);
 
@@ -728,6 +728,7 @@ static int yas532_get_calibration_data(struct yas5xx *yas5xx)
 	c->Cy1 = data[1] * 10 - 1280;
 	c->Cy2 = data[2] * 10 - 1280;
 	yas530_extract_calibration(&data[3], c);
+
 	/*
 	 * Extract linearization:
 	 * Linearization layout in the 32 bits at byte 10:
-- 
2.35.1

[PATCH v5 09/14] iio: magnetometer: yas530: Introduce "chip_info" structure

[Jakob Hauser]

This commit introduces the "chip_info" structure approach for better variant
handling.

The variant to be used is now chosen by the Device Tree (enum "chip_ids"),
not by the chip ID in the register. However, there is a check to make sure
they match (using integer "id_check").

Signed-off-by: Jakob Hauser <jahau@rocketmail.com>
---
 drivers/iio/magnetometer/yamaha-yas530.c | 107 ++++++++++++++++++-----
 1 file changed, 83 insertions(+), 24 deletions(-)

diff --git a/drivers/iio/magnetometer/yamaha-yas530.c b/drivers/iio/magnetometer/yamaha-yas530.c
index bf0aa64ac1a2..ecc2b61a5c4f 100644
--- a/drivers/iio/magnetometer/yamaha-yas530.c
+++ b/drivers/iio/magnetometer/yamaha-yas530.c
@@ -96,6 +96,24 @@
 /* Turn off device regulators etc after 5 seconds of inactivity */
 #define YAS5XX_AUTOSUSPEND_DELAY_MS	5000
 
+enum chip_ids {
+	yas530,
+	yas532,
+	yas533,
+};
+
+static const char * const yas5xx_product_name[] = {
+	"YAS530 MS-3E",
+	"YAS532 MS-3R",
+	"YAS533 MS-3F",
+};
+
+static const char * const yas5xx_version_names[][2] = {
+	[yas530] = { "A", "B" },
+	[yas532] = { "AB", "AC" },
+	[yas533] = { "AB", "AC" },
+};
+
 struct yas5xx_calibration {
 	/* Linearization calibration x, y1, y2 */
 	s32 r[3];
@@ -110,12 +128,26 @@ struct yas5xx_calibration {
 	u8 dck;
 };
 
+struct yas5xx;
+
+/**
+ * struct yas5xx_chip_info - device-specific data and function pointers
+ * @devid: device ID number
+ * @product_name: product name of the YAS variant
+ * @version_name: version letter or naming
+ */
+struct yas5xx_chip_info {
+	unsigned int devid;
+	const char *product_name;
+	const char * const *version_name;
+};
+
 /**
  * struct yas5xx - state container for the YAS5xx driver
  * @dev: parent device pointer
- * @devid: device ID number
+ * @chip: enumeration of the device variant
+ * @chip_info: device-specific data
  * @version: device version
- * @name: device name
  * @calibration: calibration settings from the OTP storage
  * @hard_offsets: offsets for each axis measured with initcoil actuated
  * @orientation: mounting matrix, flipped axis etc
@@ -129,9 +161,9 @@ struct yas5xx_calibration {
  */
 struct yas5xx {
 	struct device *dev;
-	unsigned int devid;
+	enum chip_ids chip;
+	const struct yas5xx_chip_info *chip_info;
 	unsigned int version;
-	char name[16];
 	struct yas5xx_calibration calibration;
 	s8 hard_offsets[3];
 	struct iio_mount_matrix orientation;
@@ -222,7 +254,7 @@ static int yas530_measure(struct yas5xx *yas5xx, u16 *t, u16 *x, u16 *y1, u16 *y
 
 	mutex_unlock(&yas5xx->lock);
 
-	switch (yas5xx->devid) {
+	switch (yas5xx->chip_info->devid) {
 	case YAS530_DEVICE_ID:
 		/*
 		 * The t value is 9 bits in big endian format
@@ -276,7 +308,7 @@ static s32 yas530_linearize(struct yas5xx *yas5xx, u16 val, int axis)
 	s32 coef;
 
 	/* Select coefficients */
-	switch (yas5xx->devid) {
+	switch (yas5xx->chip_info->devid) {
 	case YAS530_DEVICE_ID:
 		if (yas5xx->version == YAS530_VERSION_A)
 			coef = YAS530_VERSION_A_COEF;
@@ -336,7 +368,7 @@ static int yas530_get_measure(struct yas5xx *yas5xx, s32 *to, s32 *xo, s32 *yo,
 	sy2 = yas530_linearize(yas5xx, y2, 2);
 
 	/* Set the temperature reference value (unit: counts) */
-	switch (yas5xx->devid) {
+	switch (yas5xx->chip_info->devid) {
 	case YAS530_DEVICE_ID:
 		t_ref = YAS530_20DEGREES;
 		break;
@@ -349,7 +381,7 @@ static int yas530_get_measure(struct yas5xx *yas5xx, s32 *to, s32 *xo, s32 *yo,
 	}
 
 	/* Temperature compensation for x, y1, y2 respectively */
-	if (yas5xx->devid == YAS532_DEVICE_ID &&
+	if (yas5xx->chip_info->devid == YAS532_DEVICE_ID &&
 	    yas5xx->version == YAS532_VERSION_AC) {
 		/*
 		 * YAS532 version AC uses the temperature deviation as a
@@ -384,7 +416,7 @@ static int yas530_get_measure(struct yas5xx *yas5xx, s32 *to, s32 *xo, s32 *yo,
 	sz = -sy1 - sy2;
 
 	/* Process temperature readout */
-	switch (yas5xx->devid) {
+	switch (yas5xx->chip_info->devid) {
 	case YAS530_DEVICE_ID:
 		/*
 		 * Raw temperature value t is the number of counts starting
@@ -473,7 +505,7 @@ static int yas5xx_read_raw(struct iio_dev *indio_dev,
 		}
 		return IIO_VAL_INT;
 	case IIO_CHAN_INFO_SCALE:
-		switch (yas5xx->devid) {
+		switch (yas5xx->chip_info->devid) {
 		case YAS530_DEVICE_ID:
 			/*
 			 * Raw values of YAS530 are in picotesla. Divide by
@@ -814,7 +846,7 @@ static int yas530_measure_offsets(struct yas5xx *yas5xx)
 		return ret;
 
 	/* When the initcoil is active this should be around the center */
-	switch (yas5xx->devid) {
+	switch (yas5xx->chip_info->devid) {
 	case YAS530_DEVICE_ID:
 		center = YAS530_DATA_CENTER;
 		break;
@@ -895,12 +927,31 @@ static int yas530_power_on(struct yas5xx *yas5xx)
 	return regmap_write(yas5xx->map, YAS530_MEASURE_INTERVAL, 0);
 }
 
+static const struct yas5xx_chip_info yas5xx_chip_info_tbl[] = {
+	[yas530] = {
+		.devid = YAS530_DEVICE_ID,
+		.product_name = yas5xx_product_name[yas530],
+		.version_name = yas5xx_version_names[yas530],
+	},
+	[yas532] = {
+		.devid = YAS532_DEVICE_ID,
+		.product_name = yas5xx_product_name[yas532],
+		.version_name = yas5xx_version_names[yas532],
+	},
+	[yas533] = {
+		.devid = YAS532_DEVICE_ID,
+		.product_name = yas5xx_product_name[yas533],
+		.version_name = yas5xx_version_names[yas533],
+	},
+};
+
 static int yas5xx_probe(struct i2c_client *i2c,
 			const struct i2c_device_id *id)
 {
 	struct iio_dev *indio_dev;
 	struct device *dev = &i2c->dev;
 	struct yas5xx *yas5xx;
+	int id_check;
 	int ret;
 
 	indio_dev = devm_iio_device_alloc(dev, sizeof(*yas5xx));
@@ -947,33 +998,41 @@ static int yas5xx_probe(struct i2c_client *i2c,
 		goto assert_reset;
 	}
 
-	ret = regmap_read(yas5xx->map, YAS5XX_DEVICE_ID, &yas5xx->devid);
+	yas5xx->chip = id->driver_data;
+	yas5xx->chip_info = &yas5xx_chip_info_tbl[yas5xx->chip];
+
+	ret = regmap_read(yas5xx->map, YAS5XX_DEVICE_ID, &id_check);
 	if (ret)
 		goto assert_reset;
 
-	switch (yas5xx->devid) {
+	if (id_check != yas5xx->chip_info->devid) {
+		ret = dev_err_probe(dev, -ENODEV,
+				    "device ID %02x doesn't match %s\n",
+				    id_check, id->name);
+		goto assert_reset;
+	}
+
+	switch (yas5xx->chip_info->devid) {
 	case YAS530_DEVICE_ID:
 		ret = yas530_get_calibration_data(yas5xx);
 		if (ret)
 			goto assert_reset;
-		dev_info(dev, "detected YAS530 MS-3E %s",
-			 yas5xx->version ? "B" : "A");
-		strncpy(yas5xx->name, "yas530", sizeof(yas5xx->name));
 		break;
 	case YAS532_DEVICE_ID:
 		ret = yas532_get_calibration_data(yas5xx);
 		if (ret)
 			goto assert_reset;
-		dev_info(dev, "detected YAS532/YAS533 MS-3R/F %s",
-			 yas5xx->version ? "AC" : "AB");
-		strncpy(yas5xx->name, "yas532", sizeof(yas5xx->name));
 		break;
 	default:
 		ret = -ENODEV;
-		dev_err(dev, "unhandled device ID %02x\n", yas5xx->devid);
+		dev_err(dev, "unhandled device ID %02x\n",
+			yas5xx->chip_info->devid);
 		goto assert_reset;
 	}
 
+	dev_info(dev, "detected %s %s\n", yas5xx->chip_info->product_name,
+		 yas5xx->chip_info->version_name[yas5xx->version]);
+
 	yas530_dump_calibration(yas5xx);
 	ret = yas530_power_on(yas5xx);
 	if (ret)
@@ -985,7 +1044,7 @@ static int yas5xx_probe(struct i2c_client *i2c,
 	indio_dev->info = &yas5xx_info;
 	indio_dev->available_scan_masks = yas5xx_scan_masks;
 	indio_dev->modes = INDIO_DIRECT_MODE;
-	indio_dev->name = yas5xx->name;
+	indio_dev->name = id->name;
 	indio_dev->channels = yas5xx_channels;
 	indio_dev->num_channels = ARRAY_SIZE(yas5xx_channels);
 
@@ -1100,9 +1159,9 @@ static const struct dev_pm_ops yas5xx_dev_pm_ops = {
 };
 
 static const struct i2c_device_id yas5xx_id[] = {
-	{"yas530", },
-	{"yas532", },
-	{"yas533", },
+	{"yas530", yas530 },
+	{"yas532", yas532 },
+	{"yas533", yas533 },
 	{}
 };
 MODULE_DEVICE_TABLE(i2c, yas5xx_id);
-- 
2.35.1

Re: [PATCH v5 09/14] iio: magnetometer: yas530: Introduce "chip_info" structure

[kernel test robot]

Hi Jakob,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on v5.19]
[also build test ERROR on next-20220805]
[cannot apply to jic23-iio/togreg linus/master]
[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/Jakob-Hauser/iio-magnetometer-yas530-Change-data-type-of-hard_offsets-to-signed/20220808-080209
base:    3d7cb6b04c3f3115719235cc6866b10326de34cd
config: hexagon-randconfig-r045-20220807 (https://download.01.org/0day-ci/archive/20220808/202208081346.EWHUWCSa-lkp@intel.com/config)
compiler: clang version 16.0.0 (https://github.com/llvm/llvm-project 5f1c7e2cc5a3c07cbc2412e851a7283c1841f520)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/2e5a660a127b0fa7ca71e3e30356dc2254ec13eb
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Jakob-Hauser/iio-magnetometer-yas530-Change-data-type-of-hard_offsets-to-signed/20220808-080209
        git checkout 2e5a660a127b0fa7ca71e3e30356dc2254ec13eb
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon SHELL=/bin/bash drivers/iio/magnetometer/

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> drivers/iio/magnetometer/yamaha-yas530.c:933:19: error: initializer element is not a compile-time constant
                   .product_name = yas5xx_product_name[yas530],
                                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~
   1 error generated.


vim +933 drivers/iio/magnetometer/yamaha-yas530.c

   929	
   930	static const struct yas5xx_chip_info yas5xx_chip_info_tbl[] = {
   931		[yas530] = {
   932			.devid = YAS530_DEVICE_ID,
 > 933			.product_name = yas5xx_product_name[yas530],
   934			.version_name = yas5xx_version_names[yas530],
   935		},
   936		[yas532] = {
   937			.devid = YAS532_DEVICE_ID,
   938			.product_name = yas5xx_product_name[yas532],
   939			.version_name = yas5xx_version_names[yas532],
   940		},
   941		[yas533] = {
   942			.devid = YAS532_DEVICE_ID,
   943			.product_name = yas5xx_product_name[yas533],
   944			.version_name = yas5xx_version_names[yas533],
   945		},
   946	};
   947	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

Re: [PATCH v5 09/14] iio: magnetometer: yas530: Introduce "chip_info" structure

[Andy Shevchenko]

On Mon, Aug 8, 2022 at 7:40 AM kernel test robot <lkp@intel.com> wrote:

...

> All errors (new ones prefixed by >>):
>
> >> drivers/iio/magnetometer/yamaha-yas530.c:933:19: error: initializer element is not a compile-time constant
>                    .product_name = yas5xx_product_name[yas530],
>                                    ^~~~~~~~~~~~~~~~~~~~~~~~~~~
>    1 error generated.

What?!

The yas530 is a part of the enum, how come that compiler can't see
this? Looks like a Clang bug.

>    930  static const struct yas5xx_chip_info yas5xx_chip_info_tbl[] = {
>    931          [yas530] = {
>    932                  .devid = YAS530_DEVICE_ID,
>  > 933                  .product_name = yas5xx_product_name[yas530],
>    934                  .version_name = yas5xx_version_names[yas530],
>    935          },
>    936          [yas532] = {
>    937                  .devid = YAS532_DEVICE_ID,
>    938                  .product_name = yas5xx_product_name[yas532],
>    939                  .version_name = yas5xx_version_names[yas532],
>    940          },
>    941          [yas533] = {
>    942                  .devid = YAS532_DEVICE_ID,
>    943                  .product_name = yas5xx_product_name[yas533],
>    944                  .version_name = yas5xx_version_names[yas533],
>    945          },
>    946  };

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 09/14] iio: magnetometer: yas530: Introduce "chip_info" structure

[Andy Shevchenko]

+Cc: clang people

On Mon, Aug 8, 2022 at 1:18 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Mon, Aug 8, 2022 at 7:40 AM kernel test robot <lkp@intel.com> wrote:
>
> ...
>
> > All errors (new ones prefixed by >>):
> >
> > >> drivers/iio/magnetometer/yamaha-yas530.c:933:19: error: initializer element is not a compile-time constant
> >                    .product_name = yas5xx_product_name[yas530],
> >                                    ^~~~~~~~~~~~~~~~~~~~~~~~~~~
> >    1 error generated.
>
> What?!
>
> The yas530 is a part of the enum, how come that compiler can't see
> this? Looks like a Clang bug.
>
> >    930  static const struct yas5xx_chip_info yas5xx_chip_info_tbl[] = {
> >    931          [yas530] = {
> >    932                  .devid = YAS530_DEVICE_ID,
> >  > 933                  .product_name = yas5xx_product_name[yas530],
> >    934                  .version_name = yas5xx_version_names[yas530],
> >    935          },
> >    936          [yas532] = {
> >    937                  .devid = YAS532_DEVICE_ID,
> >    938                  .product_name = yas5xx_product_name[yas532],
> >    939                  .version_name = yas5xx_version_names[yas532],
> >    940          },
> >    941          [yas533] = {
> >    942                  .devid = YAS532_DEVICE_ID,
> >    943                  .product_name = yas5xx_product_name[yas533],
> >    944                  .version_name = yas5xx_version_names[yas533],
> >    945          },
> >    946  };

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 09/14] iio: magnetometer: yas530: Introduce "chip_info" structure

[Nathan Chancellor]

Hi Andy,

On Mon, Aug 08, 2022 at 01:18:06PM, +0200, Andy Shevchenko wrote:
> On Mon, Aug 8, 2022 at 7:40 AM kernel test robot <lkp@intel.com> wrote:
> 
> ...
> 
> > All errors (new ones prefixed by >>):
> >
> > >> drivers/iio/magnetometer/yamaha-yas530.c:933:19: error: initializer element is not a compile-time constant
> >                    .product_name = yas5xx_product_name[yas530],
> >                                    ^~~~~~~~~~~~~~~~~~~~~~~~~~~
> >    1 error generated.
> 
> What?!
> 
> The yas530 is a part of the enum, how come that compiler can't see
> this? Looks like a Clang bug.

That is not what clang is complaining about here, you'll see the same
error even if you used '0', '1', or '2' here:

  drivers/iio/magnetometer/yamaha-yas530.c:933:19: error: initializer element is not a compile-time constant
                  .product_name = yas5xx_product_name[0],
                                  ^~~~~~~~~~~~~~~~~~~~~~
  1 error generated.

It is complaining that the initializer element
('yas5xx_product_name[yas530]', rather than just 'yas530') is not
constant, which is a true complaint if I am reading C11 standard 6.6.7
correctly.

GCC 8+ has chosen to accept const structures as constant expressions in
designated initializers, which it is allowed to do per 6.6.10. Nick did
have a patch to try and match this behavior in clang but the work that
was requested doesn't seem to be trivial so it was never finalized:
https://reviews.llvm.org/D76096

You'll see the same error with GCC 7:

  drivers/iio/magnetometer/yamaha-yas530.c:933:19: error: initializer element is not constant
     .product_name = yas5xx_product_name[yas530],
                     ^~~~~~~~~~~~~~~~~~~
  drivers/iio/magnetometer/yamaha-yas530.c:933:19: note: (near initialization for ‘yas5xx_chip_info_tbl[0].product_name’)
  drivers/iio/magnetometer/yamaha-yas530.c:938:19: error: initializer element is not constant
     .product_name = yas5xx_product_name[yas532],
                     ^~~~~~~~~~~~~~~~~~~
  drivers/iio/magnetometer/yamaha-yas530.c:938:19: note: (near initialization for ‘yas5xx_chip_info_tbl[1].product_name’)
  drivers/iio/magnetometer/yamaha-yas530.c:943:19: error: initializer element is not constant
     .product_name = yas5xx_product_name[yas533],
                     ^~~~~~~~~~~~~~~~~~~
  drivers/iio/magnetometer/yamaha-yas530.c:943:19: note: (near initialization for ‘yas5xx_chip_info_tbl[2].product_name’)

Cheers,
Nathan

> >    930  static const struct yas5xx_chip_info yas5xx_chip_info_tbl[] = {
> >    931          [yas530] = {
> >    932                  .devid = YAS530_DEVICE_ID,
> >  > 933                  .product_name = yas5xx_product_name[yas530],
> >    934                  .version_name = yas5xx_version_names[yas530],
> >    935          },
> >    936          [yas532] = {
> >    937                  .devid = YAS532_DEVICE_ID,
> >    938                  .product_name = yas5xx_product_name[yas532],
> >    939                  .version_name = yas5xx_version_names[yas532],
> >    940          },
> >    941          [yas533] = {
> >    942                  .devid = YAS532_DEVICE_ID,
> >    943                  .product_name = yas5xx_product_name[yas533],
> >    944                  .version_name = yas5xx_version_names[yas533],
> >    945          },
> >    946  };
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 

Re: [PATCH v5 09/14] iio: magnetometer: yas530: Introduce "chip_info" structure

[Andy Shevchenko]

On Mon, Aug 8, 2022 at 5:59 PM Nathan Chancellor <nathan@kernel.org> wrote:
> On Mon, Aug 08, 2022 at 01:18:06PM, +0200, Andy Shevchenko wrote:
> > On Mon, Aug 8, 2022 at 7:40 AM kernel test robot <lkp@intel.com> wrote:
> >
> > ...
> >
> > > All errors (new ones prefixed by >>):
> > >
> > > >> drivers/iio/magnetometer/yamaha-yas530.c:933:19: error: initializer element is not a compile-time constant
> > >                    .product_name = yas5xx_product_name[yas530],
> > >                                    ^~~~~~~~~~~~~~~~~~~~~~~~~~~
> > >    1 error generated.
> >
> > What?!
> >
> > The yas530 is a part of the enum, how come that compiler can't see
> > this? Looks like a Clang bug.
>
> That is not what clang is complaining about here, you'll see the same
> error even if you used '0', '1', or '2' here:
>
>   drivers/iio/magnetometer/yamaha-yas530.c:933:19: error: initializer element is not a compile-time constant
>                   .product_name = yas5xx_product_name[0],
>                                   ^~~~~~~~~~~~~~~~~~~~~~
>   1 error generated.
>
> It is complaining that the initializer element
> ('yas5xx_product_name[yas530]', rather than just 'yas530') is not
> constant, which is a true complaint if I am reading C11 standard 6.6.7
> correctly.
>
> GCC 8+ has chosen to accept const structures as constant expressions in
> designated initializers, which it is allowed to do per 6.6.10. Nick did
> have a patch to try and match this behavior in clang but the work that
> was requested doesn't seem to be trivial so it was never finalized:
> https://reviews.llvm.org/D76096
>
> You'll see the same error with GCC 7:
>
>   drivers/iio/magnetometer/yamaha-yas530.c:933:19: error: initializer element is not constant
>      .product_name = yas5xx_product_name[yas530],
>                      ^~~~~~~~~~~~~~~~~~~
>   drivers/iio/magnetometer/yamaha-yas530.c:933:19: note: (near initialization for ‘yas5xx_chip_info_tbl[0].product_name’)
>   drivers/iio/magnetometer/yamaha-yas530.c:938:19: error: initializer element is not constant
>      .product_name = yas5xx_product_name[yas532],
>                      ^~~~~~~~~~~~~~~~~~~
>   drivers/iio/magnetometer/yamaha-yas530.c:938:19: note: (near initialization for ‘yas5xx_chip_info_tbl[1].product_name’)
>   drivers/iio/magnetometer/yamaha-yas530.c:943:19: error: initializer element is not constant
>      .product_name = yas5xx_product_name[yas533],
>                      ^~~~~~~~~~~~~~~~~~~
>   drivers/iio/magnetometer/yamaha-yas530.c:943:19: note: (near initialization for ‘yas5xx_chip_info_tbl[2].product_name’)

> > >    930  static const struct yas5xx_chip_info yas5xx_chip_info_tbl[] = {
> > >    931          [yas530] = {
> > >    932                  .devid = YAS530_DEVICE_ID,
> > >  > 933                  .product_name = yas5xx_product_name[yas530],
> > >    934                  .version_name = yas5xx_version_names[yas530],

Would then

  .product_name = "YAS530 MS-3E",
  .version_names = { "A", "B" },

work?

Jakob, note 's' in the field name as well.

> > >    935          },
> > >    936          [yas532] = {
> > >    937                  .devid = YAS532_DEVICE_ID,
> > >    938                  .product_name = yas5xx_product_name[yas532],
> > >    939                  .version_name = yas5xx_version_names[yas532],
> > >    940          },
> > >    941          [yas533] = {
> > >    942                  .devid = YAS532_DEVICE_ID,
> > >    943                  .product_name = yas5xx_product_name[yas533],
> > >    944                  .version_name = yas5xx_version_names[yas533],
> > >    945          },
> > >    946  };

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 09/14] iio: magnetometer: yas530: Introduce "chip_info" structure

[Nathan Chancellor]

On Mon, Aug 08, 2022 at 08:04:20PM +0200, Andy Shevchenko wrote:
> On Mon, Aug 8, 2022 at 5:59 PM Nathan Chancellor <nathan@kernel.org> wrote:
> > On Mon, Aug 08, 2022 at 01:18:06PM, +0200, Andy Shevchenko wrote:
> > > On Mon, Aug 8, 2022 at 7:40 AM kernel test robot <lkp@intel.com> wrote:
> > >
> > > ...
> > >
> > > > All errors (new ones prefixed by >>):
> > > >
> > > > >> drivers/iio/magnetometer/yamaha-yas530.c:933:19: error: initializer element is not a compile-time constant
> > > >                    .product_name = yas5xx_product_name[yas530],
> > > >                                    ^~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > >    1 error generated.
> > >
> > > What?!
> > >
> > > The yas530 is a part of the enum, how come that compiler can't see
> > > this? Looks like a Clang bug.
> >
> > That is not what clang is complaining about here, you'll see the same
> > error even if you used '0', '1', or '2' here:
> >
> >   drivers/iio/magnetometer/yamaha-yas530.c:933:19: error: initializer element is not a compile-time constant
> >                   .product_name = yas5xx_product_name[0],
> >                                   ^~~~~~~~~~~~~~~~~~~~~~
> >   1 error generated.
> >
> > It is complaining that the initializer element
> > ('yas5xx_product_name[yas530]', rather than just 'yas530') is not
> > constant, which is a true complaint if I am reading C11 standard 6.6.7
> > correctly.
> >
> > GCC 8+ has chosen to accept const structures as constant expressions in
> > designated initializers, which it is allowed to do per 6.6.10. Nick did
> > have a patch to try and match this behavior in clang but the work that
> > was requested doesn't seem to be trivial so it was never finalized:
> > https://reviews.llvm.org/D76096
> >
> > You'll see the same error with GCC 7:
> >
> >   drivers/iio/magnetometer/yamaha-yas530.c:933:19: error: initializer element is not constant
> >      .product_name = yas5xx_product_name[yas530],
> >                      ^~~~~~~~~~~~~~~~~~~
> >   drivers/iio/magnetometer/yamaha-yas530.c:933:19: note: (near initialization for ‘yas5xx_chip_info_tbl[0].product_name’)
> >   drivers/iio/magnetometer/yamaha-yas530.c:938:19: error: initializer element is not constant
> >      .product_name = yas5xx_product_name[yas532],
> >                      ^~~~~~~~~~~~~~~~~~~
> >   drivers/iio/magnetometer/yamaha-yas530.c:938:19: note: (near initialization for ‘yas5xx_chip_info_tbl[1].product_name’)
> >   drivers/iio/magnetometer/yamaha-yas530.c:943:19: error: initializer element is not constant
> >      .product_name = yas5xx_product_name[yas533],
> >                      ^~~~~~~~~~~~~~~~~~~
> >   drivers/iio/magnetometer/yamaha-yas530.c:943:19: note: (near initialization for ‘yas5xx_chip_info_tbl[2].product_name’)
> 
> > > >    930  static const struct yas5xx_chip_info yas5xx_chip_info_tbl[] = {
> > > >    931          [yas530] = {
> > > >    932                  .devid = YAS530_DEVICE_ID,
> > > >  > 933                  .product_name = yas5xx_product_name[yas530],
> > > >    934                  .version_name = yas5xx_version_names[yas530],
> 
> Would then
> 
>   .product_name = "YAS530 MS-3E",
>   .version_names = { "A", "B" },
> 
> work?

I haven't tested it but there is no reason that shouldn't work.

> Jakob, note 's' in the field name as well.
> 
> > > >    935          },
> > > >    936          [yas532] = {
> > > >    937                  .devid = YAS532_DEVICE_ID,
> > > >    938                  .product_name = yas5xx_product_name[yas532],
> > > >    939                  .version_name = yas5xx_version_names[yas532],
> > > >    940          },
> > > >    941          [yas533] = {
> > > >    942                  .devid = YAS532_DEVICE_ID,
> > > >    943                  .product_name = yas5xx_product_name[yas533],
> > > >    944                  .version_name = yas5xx_version_names[yas533],
> > > >    945          },
> > > >    946  };
> 
> -- 
> With Best Regards,
> Andy Shevchenko

Re: [PATCH v5 09/14] iio: magnetometer: yas530: Introduce "chip_info" structure

[Jakob Hauser]

Hi Andy,

On 08.08.22 20:04, Andy Shevchenko wrote:
> On Mon, Aug 8, 2022 at 5:59 PM Nathan Chancellor <nathan@kernel.org> wrote:
>> On Mon, Aug 08, 2022 at 01:18:06PM, +0200, Andy Shevchenko wrote:
>>> On Mon, Aug 8, 2022 at 7:40 AM kernel test robot <lkp@intel.com> wrote:
>>>
>>> ...
>>>
>>>> All errors (new ones prefixed by >>):
>>>>
>>>>>> drivers/iio/magnetometer/yamaha-yas530.c:933:19: error: initializer element is not a compile-time constant
>>>>                    .product_name = yas5xx_product_name[yas530],
>>>>                                    ^~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>>    1 error generated.
>>>
>>> What?!
>>>
>>> The yas530 is a part of the enum, how come that compiler can't see
>>> this? Looks like a Clang bug.
>>
>> That is not what clang is complaining about here, you'll see the same
>> error even if you used '0', '1', or '2' here:
>>
>>   drivers/iio/magnetometer/yamaha-yas530.c:933:19: error: initializer element is not a compile-time constant
>>                   .product_name = yas5xx_product_name[0],
>>                                   ^~~~~~~~~~~~~~~~~~~~~~
>>   1 error generated.
>>
>> It is complaining that the initializer element
>> ('yas5xx_product_name[yas530]', rather than just 'yas530') is not
>> constant, which is a true complaint if I am reading C11 standard 6.6.7
>> correctly.
>>
>> GCC 8+ has chosen to accept const structures as constant expressions in
>> designated initializers, which it is allowed to do per 6.6.10. Nick did
>> have a patch to try and match this behavior in clang but the work that
>> was requested doesn't seem to be trivial so it was never finalized:
>> https://reviews.llvm.org/D76096
>>
>> You'll see the same error with GCC 7:
>>
>>   drivers/iio/magnetometer/yamaha-yas530.c:933:19: error: initializer element is not constant
>>      .product_name = yas5xx_product_name[yas530],
>>                      ^~~~~~~~~~~~~~~~~~~
>>   drivers/iio/magnetometer/yamaha-yas530.c:933:19: note: (near initialization for ‘yas5xx_chip_info_tbl[0].product_name’)
>>   drivers/iio/magnetometer/yamaha-yas530.c:938:19: error: initializer element is not constant
>>      .product_name = yas5xx_product_name[yas532],
>>                      ^~~~~~~~~~~~~~~~~~~
>>   drivers/iio/magnetometer/yamaha-yas530.c:938:19: note: (near initialization for ‘yas5xx_chip_info_tbl[1].product_name’)
>>   drivers/iio/magnetometer/yamaha-yas530.c:943:19: error: initializer element is not constant
>>      .product_name = yas5xx_product_name[yas533],
>>                      ^~~~~~~~~~~~~~~~~~~
>>   drivers/iio/magnetometer/yamaha-yas530.c:943:19: note: (near initialization for ‘yas5xx_chip_info_tbl[2].product_name’)
> 
>>>>    930  static const struct yas5xx_chip_info yas5xx_chip_info_tbl[] = {
>>>>    931          [yas530] = {
>>>>    932                  .devid = YAS530_DEVICE_ID,
>>>>  > 933                  .product_name = yas5xx_product_name[yas530],
>>>>    934                  .version_name = yas5xx_version_names[yas530],
> 
> Would then
> 
>   .product_name = "YAS530 MS-3E",
>   .version_names = { "A", "B" },
> 
> work?
> 
> Jakob, note 's' in the field name as well.

Thanks for clarifying. I'll change it that way.

>>>>    935          },
>>>>    936          [yas532] = {
>>>>    937                  .devid = YAS532_DEVICE_ID,
>>>>    938                  .product_name = yas5xx_product_name[yas532],
>>>>    939                  .version_name = yas5xx_version_names[yas532],
>>>>    940          },
>>>>    941          [yas533] = {
>>>>    942                  .devid = YAS532_DEVICE_ID,
>>>>    943                  .product_name = yas5xx_product_name[yas533],
>>>>    944                  .version_name = yas5xx_version_names[yas533],
>>>>    945          },
>>>>    946  };
> 

Kind regards,
Jakob

Re: [PATCH v5 09/14] iio: magnetometer: yas530: Introduce "chip_info" structure

[Andy Shevchenko]

On Mon, Aug 8, 2022 at 1:07 AM Jakob Hauser <jahau@rocketmail.com> wrote:
>
> This commit introduces the "chip_info" structure approach for better variant
> handling.

Read "Submitting Patches" in the chapter which mentions "This patch"
pattern and fix the above accordingly.

> The variant to be used is now chosen by the Device Tree (enum "chip_ids"),
> not by the chip ID in the register. However, there is a check to make sure
> they match (using integer "id_check").

...

> +enum chip_ids {
> +       yas530,
> +       yas532,
> +       yas533,
> +};

So, it's an error from Clang... Workaround can be simply to use set of
#define:s instead.

...

> +       if (id_check != yas5xx->chip_info->devid) {

> +       switch (yas5xx->chip_info->devid) {

You can make these kind of lines shorter by introducing a temporary variable:

   struct ... *ci = yaas5xx->chip_info;

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 09/14] iio: magnetometer: yas530: Introduce "chip_info" structure

[Jakob Hauser]

Hi Andy,

On 08.08.22 13:22, Andy Shevchenko wrote:
> On Mon, Aug 8, 2022 at 1:07 AM Jakob Hauser <jahau@rocketmail.com> wrote:
>>
>> This commit introduces the "chip_info" structure approach for better variant
>> handling.
> 
> Read "Submitting Patches" in the chapter which mentions "This patch"
> pattern and fix the above accordingly.

I'll change the sentence to imperative mood.

>> The variant to be used is now chosen by the Device Tree (enum "chip_ids"),
>> not by the chip ID in the register. However, there is a check to make sure
>> they match (using integer "id_check").

...

>> +       if (id_check != yas5xx->chip_info->devid) {
> 
>> +       switch (yas5xx->chip_info->devid) {
> 
> You can make these kind of lines shorter by introducing a temporary variable:
> 
>    struct ... *ci = yaas5xx->chip_info;
> 

Everywhere? OK. There will be many changes, this also affects the
following patches.

I hope "ci" for chip_info and "c" for calibration is not too confusing.
Though I guess it's ok.

Kind regards,
Jakob

Re: [PATCH v5 09/14] iio: magnetometer: yas530: Introduce "chip_info" structure

[Andy Shevchenko]

On Mon, Aug 8, 2022 at 1:07 AM Jakob Hauser <jahau@rocketmail.com> wrote:

...

> +       yas5xx->chip = id->driver_data;
> +       yas5xx->chip_info = &yas5xx_chip_info_tbl[yas5xx->chip];

I don't see how ->chip is being used, I would expect it is the part of
chip_info, if it's really needed. That said, please make it directly a
pointer, so the above becomes:

  ... ->chip_info = (const struct ...)id->driver_data;

...

> +       {"yas530", yas530 },
> +       {"yas532", yas532 },
> +       {"yas533", yas533 },

Read above and here:

  yas53... ==> (kernel_ulong_t)&...[yas53...]

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 09/14] iio: magnetometer: yas530: Introduce "chip_info" structure

[Jakob Hauser]

Hi Andy,

On 08.08.22 13:32, Andy Shevchenko wrote:
> On Mon, Aug 8, 2022 at 1:07 AM Jakob Hauser <jahau@rocketmail.com> wrote:
> 
> ..
> 
>> +       yas5xx->chip = id->driver_data;
>> +       yas5xx->chip_info = &yas5xx_chip_info_tbl[yas5xx->chip];
> 
> I don't see how ->chip is being used, I would expect it is the part of
> chip_info, if it's really needed. That said, please make it directly a
> pointer, so the above becomes:
> 
>   ... ->chip_info = (const struct ...)id->driver_data;
> 
> ..
> 
>> +       {"yas530", yas530 },
>> +       {"yas532", yas532 },
>> +       {"yas533", yas533 },
> 
> Read above and here:
> 
>   yas53... ==> (kernel_ulong_t)&...[yas53...]
> 

Generally on this part, I'm quite confused about the ...

        enum chip_ids {
                yas530,
                yas532,
                yas533,
        };

... at the beginning of the driver.


In my naive beginners approach I think that I have to initialize this
enum. I did this by:

        struct yas5xx {
                ...
                enum chip_ids chip;
                ...
        };

        ...

        yas5xx->chip = id->driver_data;

The i2c_device_id at the end of the driver initially only contained the
fist part, looking like this:

        static const struct i2c_device_id yas5xx_id[] = {
                {"yas530", },
                {"yas532", },
                {"yas533", },
                {}
        };

This first part is "char name[I2C_NAME_SIZE]" according to [1]. I didn't
manage to initialize the enum with "id->name"...

        yas5xx->chip = id->name;

... this resulted in a compiler error stating "incompatible types when
assigning to type 'enum chip_ids' from type 'const char *'".

This made me introduce the second part of i2c_device_id...

        static const struct i2c_device_id yas5xx_id[] = {
                {"yas530", yas530 },
                {"yas532", yas532 },
                {"yas533", yas533 },
                {}
        };

... which is "kernel_ulong_t driver_data;". Initializing the enum by
"id->driver_data" did work:

        yas5xx->chip = id->driver_data;

[1]
https://github.com/torvalds/linux/blob/v5.19/include/linux/mod_devicetable.h#L465

--------------------

I think in other drivers I've seen enums not being initialized. I don't
understand how this works. Unfortunately I can't recall specific examples.

--------------------

I now had a try with following changes...

        struct yas5xx {
                struct device *dev;
-               enum chip_ids chip;
                const struct yas5xx_chip_info *chip_info;
                ...
        };

        ...

-       yas5xx->chip = id->driver_data;
-       yas5xx->chip_info = &yas5xx_chip_info_tbl[yas5xx->chip];
+       yas5xx->chip_info = &yas5xx_chip_info_tbl[id->driver_data];


... or summarized as:

        enum chip_ids {
                yas530,
                yas532,
                yas533,
        };

        ...

        struct yas5xx {
                ...
        };

        ...

        yas5xx->chip_info = &yas5xx_chip_info_tbl[id->driver_data];

        ...

        static const struct i2c_device_id yas5xx_id[] = {
                {"yas530", yas530 },
                {"yas532", yas532 },
                {"yas533", yas533 },
                {}
        };

This seems to work. Therefore I would it implement it that way. I hope
it works reliably, as I don't see a connection between the enum and the
chip_info.

--------------------

What I still can't manage is getting rid of the "id->driver_data" part.
When trying the above with "id->name"...

        yas5xx->chip_info = &yas5xx_chip_info_tbl[id->name];

... the compiler complains about "array subscript is not an integer".
When trying to add quotation marks to the enum chip_ids content, the
compiler complains about this by "expected identifier before string
constant".

Kind regards,
Jakob

[PATCH v5 10/14] iio: magnetometer: yas530: Add volatile registers to "chip_info"

[Jakob Hauser]

Add volatile registers to the "chip_info" structure to ease the handling of
different YAS variants.

Signed-off-by: Jakob Hauser <jahau@rocketmail.com>
---
 drivers/iio/magnetometer/yamaha-yas530.c | 37 ++++++++++++++++++++++--
 1 file changed, 34 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/magnetometer/yamaha-yas530.c b/drivers/iio/magnetometer/yamaha-yas530.c
index ecc2b61a5c4f..914f7f0a243e 100644
--- a/drivers/iio/magnetometer/yamaha-yas530.c
+++ b/drivers/iio/magnetometer/yamaha-yas530.c
@@ -114,6 +114,11 @@ static const char * const yas5xx_version_names[][2] = {
 	[yas533] = { "AB", "AC" },
 };
 
+static const int yas530_volatile_reg[] = {
+	YAS530_ACTUATE_INIT_COIL,
+	YAS530_MEASURE,
+};
+
 struct yas5xx_calibration {
 	/* Linearization calibration x, y1, y2 */
 	s32 r[3];
@@ -135,11 +140,15 @@ struct yas5xx;
  * @devid: device ID number
  * @product_name: product name of the YAS variant
  * @version_name: version letter or naming
+ * @volatile_reg: device-specific volatile registers
+ * @volatile_reg_qty: quantity of device-specific volatile registers
  */
 struct yas5xx_chip_info {
 	unsigned int devid;
 	const char *product_name;
 	const char * const *version_name;
+	const int *volatile_reg;
+	int volatile_reg_qty;
 };
 
 /**
@@ -626,9 +635,25 @@ static const struct iio_info yas5xx_info = {
 
 static bool yas5xx_volatile_reg(struct device *dev, unsigned int reg)
 {
-	return reg == YAS530_ACTUATE_INIT_COIL ||
-		reg == YAS530_MEASURE ||
-		(reg >= YAS5XX_MEASURE_DATA && reg < YAS5XX_MEASURE_DATA + 8);
+	struct iio_dev *indio_dev = dev_get_drvdata(dev);
+	struct yas5xx *yas5xx = iio_priv(indio_dev);
+	int reg_qty;
+	int i;
+
+	if (reg >= YAS5XX_MEASURE_DATA && reg < YAS5XX_MEASURE_DATA + 8)
+		return true;
+
+	/*
+	 * YAS versions share different registers on the same address,
+	 * need to differentiate.
+	 */
+	reg_qty = yas5xx->chip_info->volatile_reg_qty;
+	for (i = 0; i < reg_qty; i++) {
+		if (reg == yas5xx->chip_info->volatile_reg[i])
+			return true;
+	}
+
+	return false;
 }
 
 /* TODO: enable regmap cache, using mark dirty and sync at runtime resume */
@@ -932,16 +957,22 @@ static const struct yas5xx_chip_info yas5xx_chip_info_tbl[] = {
 		.devid = YAS530_DEVICE_ID,
 		.product_name = yas5xx_product_name[yas530],
 		.version_name = yas5xx_version_names[yas530],
+		.volatile_reg = yas530_volatile_reg,
+		.volatile_reg_qty = ARRAY_SIZE(yas530_volatile_reg),
 	},
 	[yas532] = {
 		.devid = YAS532_DEVICE_ID,
 		.product_name = yas5xx_product_name[yas532],
 		.version_name = yas5xx_version_names[yas532],
+		.volatile_reg = yas530_volatile_reg,
+		.volatile_reg_qty = ARRAY_SIZE(yas530_volatile_reg),
 	},
 	[yas533] = {
 		.devid = YAS532_DEVICE_ID,
 		.product_name = yas5xx_product_name[yas533],
 		.version_name = yas5xx_version_names[yas533],
+		.volatile_reg = yas530_volatile_reg,
+		.volatile_reg_qty = ARRAY_SIZE(yas530_volatile_reg),
 	},
 };
 
-- 
2.35.1

Re: [PATCH v5 10/14] iio: magnetometer: yas530: Add volatile registers to "chip_info"

[Andy Shevchenko]

On Mon, Aug 8, 2022 at 1:07 AM Jakob Hauser <jahau@rocketmail.com> wrote:
>
> Add volatile registers to the "chip_info" structure to ease the handling of
> different YAS variants.

Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

> Signed-off-by: Jakob Hauser <jahau@rocketmail.com>
> ---
>  drivers/iio/magnetometer/yamaha-yas530.c | 37 ++++++++++++++++++++++--
>  1 file changed, 34 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/iio/magnetometer/yamaha-yas530.c b/drivers/iio/magnetometer/yamaha-yas530.c
> index ecc2b61a5c4f..914f7f0a243e 100644
> --- a/drivers/iio/magnetometer/yamaha-yas530.c
> +++ b/drivers/iio/magnetometer/yamaha-yas530.c
> @@ -114,6 +114,11 @@ static const char * const yas5xx_version_names[][2] = {
>         [yas533] = { "AB", "AC" },
>  };
>
> +static const int yas530_volatile_reg[] = {
> +       YAS530_ACTUATE_INIT_COIL,
> +       YAS530_MEASURE,
> +};
> +
>  struct yas5xx_calibration {
>         /* Linearization calibration x, y1, y2 */
>         s32 r[3];
> @@ -135,11 +140,15 @@ struct yas5xx;
>   * @devid: device ID number
>   * @product_name: product name of the YAS variant
>   * @version_name: version letter or naming
> + * @volatile_reg: device-specific volatile registers
> + * @volatile_reg_qty: quantity of device-specific volatile registers
>   */
>  struct yas5xx_chip_info {
>         unsigned int devid;
>         const char *product_name;
>         const char * const *version_name;
> +       const int *volatile_reg;
> +       int volatile_reg_qty;
>  };
>
>  /**
> @@ -626,9 +635,25 @@ static const struct iio_info yas5xx_info = {
>
>  static bool yas5xx_volatile_reg(struct device *dev, unsigned int reg)
>  {
> -       return reg == YAS530_ACTUATE_INIT_COIL ||
> -               reg == YAS530_MEASURE ||
> -               (reg >= YAS5XX_MEASURE_DATA && reg < YAS5XX_MEASURE_DATA + 8);
> +       struct iio_dev *indio_dev = dev_get_drvdata(dev);
> +       struct yas5xx *yas5xx = iio_priv(indio_dev);
> +       int reg_qty;
> +       int i;
> +
> +       if (reg >= YAS5XX_MEASURE_DATA && reg < YAS5XX_MEASURE_DATA + 8)
> +               return true;
> +
> +       /*
> +        * YAS versions share different registers on the same address,
> +        * need to differentiate.
> +        */
> +       reg_qty = yas5xx->chip_info->volatile_reg_qty;
> +       for (i = 0; i < reg_qty; i++) {
> +               if (reg == yas5xx->chip_info->volatile_reg[i])
> +                       return true;
> +       }
> +
> +       return false;
>  }
>
>  /* TODO: enable regmap cache, using mark dirty and sync at runtime resume */
> @@ -932,16 +957,22 @@ static const struct yas5xx_chip_info yas5xx_chip_info_tbl[] = {
>                 .devid = YAS530_DEVICE_ID,
>                 .product_name = yas5xx_product_name[yas530],
>                 .version_name = yas5xx_version_names[yas530],
> +               .volatile_reg = yas530_volatile_reg,
> +               .volatile_reg_qty = ARRAY_SIZE(yas530_volatile_reg),
>         },
>         [yas532] = {
>                 .devid = YAS532_DEVICE_ID,
>                 .product_name = yas5xx_product_name[yas532],
>                 .version_name = yas5xx_version_names[yas532],
> +               .volatile_reg = yas530_volatile_reg,
> +               .volatile_reg_qty = ARRAY_SIZE(yas530_volatile_reg),
>         },
>         [yas533] = {
>                 .devid = YAS532_DEVICE_ID,
>                 .product_name = yas5xx_product_name[yas533],
>                 .version_name = yas5xx_version_names[yas533],
> +               .volatile_reg = yas530_volatile_reg,
> +               .volatile_reg_qty = ARRAY_SIZE(yas530_volatile_reg),
>         },
>  };
>
> --
> 2.35.1
>


-- 
With Best Regards,
Andy Shevchenko

[PATCH v5 11/14] iio: magnetometer: yas530: Add IIO scaling to "chip_info"

[Jakob Hauser]

Add IIO scaling to the "chip_info" structure to ease the handling to
different YAS variants.

Signed-off-by: Jakob Hauser <jahau@rocketmail.com>
---
 drivers/iio/magnetometer/yamaha-yas530.c | 28 ++++++------------------
 1 file changed, 7 insertions(+), 21 deletions(-)

diff --git a/drivers/iio/magnetometer/yamaha-yas530.c b/drivers/iio/magnetometer/yamaha-yas530.c
index 914f7f0a243e..262a36c31616 100644
--- a/drivers/iio/magnetometer/yamaha-yas530.c
+++ b/drivers/iio/magnetometer/yamaha-yas530.c
@@ -142,6 +142,7 @@ struct yas5xx;
  * @version_name: version letter or naming
  * @volatile_reg: device-specific volatile registers
  * @volatile_reg_qty: quantity of device-specific volatile registers
+ * @scaling_val2: scaling value for IIO_CHAN_INFO_SCALE
  */
 struct yas5xx_chip_info {
 	unsigned int devid;
@@ -149,6 +150,7 @@ struct yas5xx_chip_info {
 	const char * const *version_name;
 	const int *volatile_reg;
 	int volatile_reg_qty;
+	u32 scaling_val2;
 };
 
 /**
@@ -514,27 +516,8 @@ static int yas5xx_read_raw(struct iio_dev *indio_dev,
 		}
 		return IIO_VAL_INT;
 	case IIO_CHAN_INFO_SCALE:
-		switch (yas5xx->chip_info->devid) {
-		case YAS530_DEVICE_ID:
-			/*
-			 * Raw values of YAS530 are in picotesla. Divide by
-			 * 100000000 (10^8) to get Gauss.
-			 */
-			*val = 1;
-			*val2 = 100000000;
-			break;
-		case YAS532_DEVICE_ID:
-			/*
-			 * Raw values of YAS532 are in nanotesla. Divide by
-			 * 100000 (10^5) to get Gauss.
-			 */
-			*val = 1;
-			*val2 = 100000;
-			break;
-		default:
-			dev_err(yas5xx->dev, "unknown device type\n");
-			return -EINVAL;
-		}
+		*val = 1;
+		*val2 = yas5xx->chip_info->scaling_val2;
 		return IIO_VAL_FRACTIONAL;
 	default:
 		/* Unknown request */
@@ -959,6 +942,7 @@ static const struct yas5xx_chip_info yas5xx_chip_info_tbl[] = {
 		.version_name = yas5xx_version_names[yas530],
 		.volatile_reg = yas530_volatile_reg,
 		.volatile_reg_qty = ARRAY_SIZE(yas530_volatile_reg),
+		.scaling_val2 = 100000000, /* picotesla to Gauss */
 	},
 	[yas532] = {
 		.devid = YAS532_DEVICE_ID,
@@ -966,6 +950,7 @@ static const struct yas5xx_chip_info yas5xx_chip_info_tbl[] = {
 		.version_name = yas5xx_version_names[yas532],
 		.volatile_reg = yas530_volatile_reg,
 		.volatile_reg_qty = ARRAY_SIZE(yas530_volatile_reg),
+		.scaling_val2 = 100000, /* nanotesla to Gauss */
 	},
 	[yas533] = {
 		.devid = YAS532_DEVICE_ID,
@@ -973,6 +958,7 @@ static const struct yas5xx_chip_info yas5xx_chip_info_tbl[] = {
 		.version_name = yas5xx_version_names[yas533],
 		.volatile_reg = yas530_volatile_reg,
 		.volatile_reg_qty = ARRAY_SIZE(yas530_volatile_reg),
+		.scaling_val2 = 100000, /* nanotesla to Gauss */
 	},
 };
 
-- 
2.35.1

Re: [PATCH v5 11/14] iio: magnetometer: yas530: Add IIO scaling to "chip_info"

[Andy Shevchenko]

On Mon, Aug 8, 2022 at 1:07 AM Jakob Hauser <jahau@rocketmail.com> wrote:
>
> Add IIO scaling to the "chip_info" structure to ease the handling to
> different YAS variants.

Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

> Signed-off-by: Jakob Hauser <jahau@rocketmail.com>
> ---
>  drivers/iio/magnetometer/yamaha-yas530.c | 28 ++++++------------------
>  1 file changed, 7 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/iio/magnetometer/yamaha-yas530.c b/drivers/iio/magnetometer/yamaha-yas530.c
> index 914f7f0a243e..262a36c31616 100644
> --- a/drivers/iio/magnetometer/yamaha-yas530.c
> +++ b/drivers/iio/magnetometer/yamaha-yas530.c
> @@ -142,6 +142,7 @@ struct yas5xx;
>   * @version_name: version letter or naming
>   * @volatile_reg: device-specific volatile registers
>   * @volatile_reg_qty: quantity of device-specific volatile registers
> + * @scaling_val2: scaling value for IIO_CHAN_INFO_SCALE
>   */
>  struct yas5xx_chip_info {
>         unsigned int devid;
> @@ -149,6 +150,7 @@ struct yas5xx_chip_info {
>         const char * const *version_name;
>         const int *volatile_reg;
>         int volatile_reg_qty;
> +       u32 scaling_val2;
>  };
>
>  /**
> @@ -514,27 +516,8 @@ static int yas5xx_read_raw(struct iio_dev *indio_dev,
>                 }
>                 return IIO_VAL_INT;
>         case IIO_CHAN_INFO_SCALE:
> -               switch (yas5xx->chip_info->devid) {
> -               case YAS530_DEVICE_ID:
> -                       /*
> -                        * Raw values of YAS530 are in picotesla. Divide by
> -                        * 100000000 (10^8) to get Gauss.
> -                        */
> -                       *val = 1;
> -                       *val2 = 100000000;
> -                       break;
> -               case YAS532_DEVICE_ID:
> -                       /*
> -                        * Raw values of YAS532 are in nanotesla. Divide by
> -                        * 100000 (10^5) to get Gauss.
> -                        */
> -                       *val = 1;
> -                       *val2 = 100000;
> -                       break;
> -               default:
> -                       dev_err(yas5xx->dev, "unknown device type\n");
> -                       return -EINVAL;
> -               }
> +               *val = 1;
> +               *val2 = yas5xx->chip_info->scaling_val2;
>                 return IIO_VAL_FRACTIONAL;
>         default:
>                 /* Unknown request */
> @@ -959,6 +942,7 @@ static const struct yas5xx_chip_info yas5xx_chip_info_tbl[] = {
>                 .version_name = yas5xx_version_names[yas530],
>                 .volatile_reg = yas530_volatile_reg,
>                 .volatile_reg_qty = ARRAY_SIZE(yas530_volatile_reg),
> +               .scaling_val2 = 100000000, /* picotesla to Gauss */
>         },
>         [yas532] = {
>                 .devid = YAS532_DEVICE_ID,
> @@ -966,6 +950,7 @@ static const struct yas5xx_chip_info yas5xx_chip_info_tbl[] = {
>                 .version_name = yas5xx_version_names[yas532],
>                 .volatile_reg = yas530_volatile_reg,
>                 .volatile_reg_qty = ARRAY_SIZE(yas530_volatile_reg),
> +               .scaling_val2 = 100000, /* nanotesla to Gauss */
>         },
>         [yas533] = {
>                 .devid = YAS532_DEVICE_ID,
> @@ -973,6 +958,7 @@ static const struct yas5xx_chip_info yas5xx_chip_info_tbl[] = {
>                 .version_name = yas5xx_version_names[yas533],
>                 .volatile_reg = yas530_volatile_reg,
>                 .volatile_reg_qty = ARRAY_SIZE(yas530_volatile_reg),
> +               .scaling_val2 = 100000, /* nanotesla to Gauss */
>         },
>  };
>
> --
> 2.35.1
>


-- 
With Best Regards,
Andy Shevchenko

[PATCH v5 12/14] iio: magnetometer: yas530: Add temperature calculation to "chip_info"

[Jakob Hauser]

Add temperature calculation to the "chip_info" structure to ease the handling
of different YAS variants.

Signed-off-by: Jakob Hauser <jahau@rocketmail.com>
---
 drivers/iio/magnetometer/yamaha-yas530.c | 140 ++++++++++++-----------
 1 file changed, 71 insertions(+), 69 deletions(-)

diff --git a/drivers/iio/magnetometer/yamaha-yas530.c b/drivers/iio/magnetometer/yamaha-yas530.c
index 262a36c31616..780c7f4d1eff 100644
--- a/drivers/iio/magnetometer/yamaha-yas530.c
+++ b/drivers/iio/magnetometer/yamaha-yas530.c
@@ -79,7 +79,6 @@
 #define YAS530_DATA_BITS		12
 #define YAS530_DATA_CENTER		BIT(YAS530_DATA_BITS - 1)
 #define YAS530_DATA_OVERFLOW		(BIT(YAS530_DATA_BITS) - 1)
-#define YAS530_20DEGREES		182 /* Counts starting at -62 °C */
 
 #define YAS532_DEVICE_ID		0x02 /* YAS532/YAS533 (MS-3R/F) */
 #define YAS532_VERSION_AB		0 /* YAS532/533 AB (MS-3R/F AB) */
@@ -91,7 +90,6 @@
 #define YAS532_DATA_BITS		13
 #define YAS532_DATA_CENTER		BIT(YAS532_DATA_BITS - 1)
 #define YAS532_DATA_OVERFLOW		(BIT(YAS532_DATA_BITS) - 1)
-#define YAS532_20DEGREES		390 /* Counts starting at -50 °C */
 
 /* Turn off device regulators etc after 5 seconds of inactivity */
 #define YAS5XX_AUTOSUSPEND_DELAY_MS	5000
@@ -119,6 +117,31 @@ static const int yas530_volatile_reg[] = {
 	YAS530_MEASURE,
 };
 
+/*
+ * t_ref_counts is the number of counts at reference temperature.
+ *
+ * The temperature value at YAS magnetometers is a number of counts. The
+ * values in t_ref_counts[] are the counts at the reference temperature
+ * of 20 °C.
+ *
+ * For YAS532/533, this value is known from the Android driver. For YAS530,
+ * it was approximately measured.
+ */
+static const u16 t_ref_counts[] = { 182, 390, 390 };
+
+/*
+ * min_temp_celsius_x10 is the starting point of temperature counting
+ * in 1/10:s degrees Celsius.
+ *
+ * The array min_temp_celsius_x10[] contains the temperatures where the
+ * temperature value count is 0. The values are in 1/10:s degrees Celsius
+ * to ease the further temperature calculation.
+ *
+ * These temperatures are derived from the temperature resolutions given
+ * in the data sheets.
+ */
+static const s16 min_temp_celsius_x10[] = { -620, -500, -500 };
+
 struct yas5xx_calibration {
 	/* Linearization calibration x, y1, y2 */
 	s32 r[3];
@@ -143,6 +166,8 @@ struct yas5xx;
  * @volatile_reg: device-specific volatile registers
  * @volatile_reg_qty: quantity of device-specific volatile registers
  * @scaling_val2: scaling value for IIO_CHAN_INFO_SCALE
+ * @t_ref: number of counts at reference temperature
+ * @min_temp_x10: starting point of temperature counting in 1/10:s degrees Celsius
  */
 struct yas5xx_chip_info {
 	unsigned int devid;
@@ -151,6 +176,8 @@ struct yas5xx_chip_info {
 	const int *volatile_reg;
 	int volatile_reg_qty;
 	u32 scaling_val2;
+	u16 t_ref;
+	s16 min_temp_x10;
 };
 
 /**
@@ -349,6 +376,20 @@ static s32 yas530_linearize(struct yas5xx *yas5xx, u16 val, int axis)
 		(yas5xx->hard_offsets[axis] - c->r[axis]) * coef;
 }
 
+static s32 yas5xx_calc_temperature(struct yas5xx *yas5xx, u16 t)
+{
+	s32 to;
+	u16 t_ref;
+	int min_temp_x10, ref_temp_x10;
+
+	t_ref = yas5xx->chip_info->t_ref;
+	min_temp_x10 = yas5xx->chip_info->min_temp_x10;
+	ref_temp_x10 = 200;
+
+	to = (min_temp_x10 + ((ref_temp_x10 - min_temp_x10) * t / t_ref)) * 100;
+	return to;
+}
+
 /**
  * yas530_get_measure() - Measure a sample of all axis and process
  * @yas5xx: The device state
@@ -363,7 +404,7 @@ static s32 yas530_linearize(struct yas5xx *yas5xx, u16 val, int axis)
 static int yas530_get_measure(struct yas5xx *yas5xx, s32 *to, s32 *xo, s32 *yo, s32 *zo)
 {
 	struct yas5xx_calibration *c = &yas5xx->calibration;
-	u16 t_ref, t, x, y1, y2;
+	u16 t_ref, t_comp, t, x, y1, y2;
 	/* These are signed x, signed y1 etc */
 	s32 sx, sy1, sy2, sy, sz;
 	int ret;
@@ -378,47 +419,30 @@ static int yas530_get_measure(struct yas5xx *yas5xx, s32 *to, s32 *xo, s32 *yo,
 	sy1 = yas530_linearize(yas5xx, y1, 1);
 	sy2 = yas530_linearize(yas5xx, y2, 2);
 
-	/* Set the temperature reference value (unit: counts) */
-	switch (yas5xx->chip_info->devid) {
-	case YAS530_DEVICE_ID:
-		t_ref = YAS530_20DEGREES;
-		break;
-	case YAS532_DEVICE_ID:
-		t_ref = YAS532_20DEGREES;
-		break;
-	default:
-		dev_err(yas5xx->dev, "unknown device type\n");
-		return -EINVAL;
-	}
-
-	/* Temperature compensation for x, y1, y2 respectively */
+	/*
+	 * Set the temperature for compensation (unit: counts):
+	 * YAS532/YAS533 version AC uses the temperature deviation as a
+	 * multiplier. YAS530 and YAS532 version AB use solely the t value.
+	 */
+	t_ref = yas5xx->chip_info->t_ref;
 	if (yas5xx->chip_info->devid == YAS532_DEVICE_ID &&
 	    yas5xx->version == YAS532_VERSION_AC) {
-		/*
-		 * YAS532 version AC uses the temperature deviation as a
-		 * multiplier.
-		 *
-		 *          Cx * (t - t_ref)
-		 * x' = x - ----------------
-		 *                100
-		 */
-		sx = sx - (c->Cx * (t - t_ref)) / 100;
-		sy1 = sy1 - (c->Cy1 * (t - t_ref)) / 100;
-		sy2 = sy2 - (c->Cy2 * (t - t_ref)) / 100;
+		t_comp = t - t_ref;
 	} else {
-		/*
-		 * YAS530 and YAS532 version AB use solely the t value as a
-		 * multiplier.
-		 *
-		 *          Cx * t
-		 * x' = x - ------
-		 *           100
-		 */
-		sx = sx - (c->Cx * t) / 100;
-		sy1 = sy1 - (c->Cy1 * t) / 100;
-		sy2 = sy2 - (c->Cy2 * t) / 100;
+		t_comp = t;
 	}
 
+	/*
+	 * Temperature compensation for x, y1, y2 respectively:
+	 *
+	 *          Cx * t_comp
+	 * x' = x - -----------
+	 *              100
+	 */
+	sx = sx - (c->Cx * t_comp) / 100;
+	sy1 = sy1 - (c->Cy1 * t_comp) / 100;
+	sy2 = sy2 - (c->Cy2 * t_comp) / 100;
+
 	/*
 	 * Break y1 and y2 into y and z, y1 and y2 are apparently encoding
 	 * y and z.
@@ -426,36 +450,8 @@ static int yas530_get_measure(struct yas5xx *yas5xx, s32 *to, s32 *xo, s32 *yo,
 	sy = sy1 - sy2;
 	sz = -sy1 - sy2;
 
-	/* Process temperature readout */
-	switch (yas5xx->chip_info->devid) {
-	case YAS530_DEVICE_ID:
-		/*
-		 * Raw temperature value t is the number of counts starting
-		 * at -62 °C. Reference value t_ref is the number of counts
-		 * between -62 °C and 20 °C (82 °C range).
-		 *
-		 * Temperature in °C would be (82 / t_ref * t) - 62.
-		 *
-		 * Contrary to this, perform multiplication first and division
-		 * second due to calculating with integers.
-		 *
-		 * To get a nicer result, calculate with 1/10:s degrees Celsius
-		 * and finally multiply by 100 to return millidegrees Celsius.
-		 */
-		*to = ((820 * t / t_ref) - 620) * 100;
-		break;
-	case YAS532_DEVICE_ID:
-		/*
-		 * Actually same procedure for YAS532 but the starting point is
-		 * at -50 °C. Reference value t_ref is the number of counts
-		 * between -50 °C and 20 °C (70 °C range).
-		 */
-		*to = ((700 * t / t_ref) - 500) * 100;
-		break;
-	default:
-		dev_err(yas5xx->dev, "unknown device type\n");
-		return -EINVAL;
-	}
+	/* Calculate temperature readout */
+	*to = yas5xx_calc_temperature(yas5xx, t);
 
 	/*
 	 * Calibrate [x,y,z] with some formulas like this:
@@ -943,6 +939,8 @@ static const struct yas5xx_chip_info yas5xx_chip_info_tbl[] = {
 		.volatile_reg = yas530_volatile_reg,
 		.volatile_reg_qty = ARRAY_SIZE(yas530_volatile_reg),
 		.scaling_val2 = 100000000, /* picotesla to Gauss */
+		.t_ref = t_ref_counts[yas530],
+		.min_temp_x10 = min_temp_celsius_x10[yas530],
 	},
 	[yas532] = {
 		.devid = YAS532_DEVICE_ID,
@@ -951,6 +949,8 @@ static const struct yas5xx_chip_info yas5xx_chip_info_tbl[] = {
 		.volatile_reg = yas530_volatile_reg,
 		.volatile_reg_qty = ARRAY_SIZE(yas530_volatile_reg),
 		.scaling_val2 = 100000, /* nanotesla to Gauss */
+		.t_ref = t_ref_counts[yas532],
+		.min_temp_x10 = min_temp_celsius_x10[yas532],
 	},
 	[yas533] = {
 		.devid = YAS532_DEVICE_ID,
@@ -959,6 +959,8 @@ static const struct yas5xx_chip_info yas5xx_chip_info_tbl[] = {
 		.volatile_reg = yas530_volatile_reg,
 		.volatile_reg_qty = ARRAY_SIZE(yas530_volatile_reg),
 		.scaling_val2 = 100000, /* nanotesla to Gauss */
+		.t_ref = t_ref_counts[yas533],
+		.min_temp_x10 = min_temp_celsius_x10[yas533],
 	},
 };
 
-- 
2.35.1

Re: [PATCH v5 12/14] iio: magnetometer: yas530: Add temperature calculation to "chip_info"

[Andy Shevchenko]

On Mon, Aug 8, 2022 at 1:07 AM Jakob Hauser <jahau@rocketmail.com> wrote:
>
> Add temperature calculation to the "chip_info" structure to ease the handling
> of different YAS variants.

Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

> Signed-off-by: Jakob Hauser <jahau@rocketmail.com>
> ---
>  drivers/iio/magnetometer/yamaha-yas530.c | 140 ++++++++++++-----------
>  1 file changed, 71 insertions(+), 69 deletions(-)
>
> diff --git a/drivers/iio/magnetometer/yamaha-yas530.c b/drivers/iio/magnetometer/yamaha-yas530.c
> index 262a36c31616..780c7f4d1eff 100644
> --- a/drivers/iio/magnetometer/yamaha-yas530.c
> +++ b/drivers/iio/magnetometer/yamaha-yas530.c
> @@ -79,7 +79,6 @@
>  #define YAS530_DATA_BITS               12
>  #define YAS530_DATA_CENTER             BIT(YAS530_DATA_BITS - 1)
>  #define YAS530_DATA_OVERFLOW           (BIT(YAS530_DATA_BITS) - 1)
> -#define YAS530_20DEGREES               182 /* Counts starting at -62 °C */
>
>  #define YAS532_DEVICE_ID               0x02 /* YAS532/YAS533 (MS-3R/F) */
>  #define YAS532_VERSION_AB              0 /* YAS532/533 AB (MS-3R/F AB) */
> @@ -91,7 +90,6 @@
>  #define YAS532_DATA_BITS               13
>  #define YAS532_DATA_CENTER             BIT(YAS532_DATA_BITS - 1)
>  #define YAS532_DATA_OVERFLOW           (BIT(YAS532_DATA_BITS) - 1)
> -#define YAS532_20DEGREES               390 /* Counts starting at -50 °C */
>
>  /* Turn off device regulators etc after 5 seconds of inactivity */
>  #define YAS5XX_AUTOSUSPEND_DELAY_MS    5000
> @@ -119,6 +117,31 @@ static const int yas530_volatile_reg[] = {
>         YAS530_MEASURE,
>  };
>
> +/*
> + * t_ref_counts is the number of counts at reference temperature.
> + *
> + * The temperature value at YAS magnetometers is a number of counts. The
> + * values in t_ref_counts[] are the counts at the reference temperature
> + * of 20 °C.
> + *
> + * For YAS532/533, this value is known from the Android driver. For YAS530,
> + * it was approximately measured.
> + */
> +static const u16 t_ref_counts[] = { 182, 390, 390 };
> +
> +/*
> + * min_temp_celsius_x10 is the starting point of temperature counting
> + * in 1/10:s degrees Celsius.
> + *
> + * The array min_temp_celsius_x10[] contains the temperatures where the
> + * temperature value count is 0. The values are in 1/10:s degrees Celsius
> + * to ease the further temperature calculation.
> + *
> + * These temperatures are derived from the temperature resolutions given
> + * in the data sheets.
> + */
> +static const s16 min_temp_celsius_x10[] = { -620, -500, -500 };
> +
>  struct yas5xx_calibration {
>         /* Linearization calibration x, y1, y2 */
>         s32 r[3];
> @@ -143,6 +166,8 @@ struct yas5xx;
>   * @volatile_reg: device-specific volatile registers
>   * @volatile_reg_qty: quantity of device-specific volatile registers
>   * @scaling_val2: scaling value for IIO_CHAN_INFO_SCALE
> + * @t_ref: number of counts at reference temperature
> + * @min_temp_x10: starting point of temperature counting in 1/10:s degrees Celsius
>   */
>  struct yas5xx_chip_info {
>         unsigned int devid;
> @@ -151,6 +176,8 @@ struct yas5xx_chip_info {
>         const int *volatile_reg;
>         int volatile_reg_qty;
>         u32 scaling_val2;
> +       u16 t_ref;
> +       s16 min_temp_x10;
>  };
>
>  /**
> @@ -349,6 +376,20 @@ static s32 yas530_linearize(struct yas5xx *yas5xx, u16 val, int axis)
>                 (yas5xx->hard_offsets[axis] - c->r[axis]) * coef;
>  }
>
> +static s32 yas5xx_calc_temperature(struct yas5xx *yas5xx, u16 t)
> +{
> +       s32 to;
> +       u16 t_ref;
> +       int min_temp_x10, ref_temp_x10;
> +
> +       t_ref = yas5xx->chip_info->t_ref;
> +       min_temp_x10 = yas5xx->chip_info->min_temp_x10;
> +       ref_temp_x10 = 200;
> +
> +       to = (min_temp_x10 + ((ref_temp_x10 - min_temp_x10) * t / t_ref)) * 100;
> +       return to;
> +}
> +
>  /**
>   * yas530_get_measure() - Measure a sample of all axis and process
>   * @yas5xx: The device state
> @@ -363,7 +404,7 @@ static s32 yas530_linearize(struct yas5xx *yas5xx, u16 val, int axis)
>  static int yas530_get_measure(struct yas5xx *yas5xx, s32 *to, s32 *xo, s32 *yo, s32 *zo)
>  {
>         struct yas5xx_calibration *c = &yas5xx->calibration;
> -       u16 t_ref, t, x, y1, y2;
> +       u16 t_ref, t_comp, t, x, y1, y2;
>         /* These are signed x, signed y1 etc */
>         s32 sx, sy1, sy2, sy, sz;
>         int ret;
> @@ -378,47 +419,30 @@ static int yas530_get_measure(struct yas5xx *yas5xx, s32 *to, s32 *xo, s32 *yo,
>         sy1 = yas530_linearize(yas5xx, y1, 1);
>         sy2 = yas530_linearize(yas5xx, y2, 2);
>
> -       /* Set the temperature reference value (unit: counts) */
> -       switch (yas5xx->chip_info->devid) {
> -       case YAS530_DEVICE_ID:
> -               t_ref = YAS530_20DEGREES;
> -               break;
> -       case YAS532_DEVICE_ID:
> -               t_ref = YAS532_20DEGREES;
> -               break;
> -       default:
> -               dev_err(yas5xx->dev, "unknown device type\n");
> -               return -EINVAL;
> -       }
> -
> -       /* Temperature compensation for x, y1, y2 respectively */
> +       /*
> +        * Set the temperature for compensation (unit: counts):
> +        * YAS532/YAS533 version AC uses the temperature deviation as a
> +        * multiplier. YAS530 and YAS532 version AB use solely the t value.
> +        */
> +       t_ref = yas5xx->chip_info->t_ref;
>         if (yas5xx->chip_info->devid == YAS532_DEVICE_ID &&
>             yas5xx->version == YAS532_VERSION_AC) {
> -               /*
> -                * YAS532 version AC uses the temperature deviation as a
> -                * multiplier.
> -                *
> -                *          Cx * (t - t_ref)
> -                * x' = x - ----------------
> -                *                100
> -                */
> -               sx = sx - (c->Cx * (t - t_ref)) / 100;
> -               sy1 = sy1 - (c->Cy1 * (t - t_ref)) / 100;
> -               sy2 = sy2 - (c->Cy2 * (t - t_ref)) / 100;
> +               t_comp = t - t_ref;
>         } else {
> -               /*
> -                * YAS530 and YAS532 version AB use solely the t value as a
> -                * multiplier.
> -                *
> -                *          Cx * t
> -                * x' = x - ------
> -                *           100
> -                */
> -               sx = sx - (c->Cx * t) / 100;
> -               sy1 = sy1 - (c->Cy1 * t) / 100;
> -               sy2 = sy2 - (c->Cy2 * t) / 100;
> +               t_comp = t;
>         }
>
> +       /*
> +        * Temperature compensation for x, y1, y2 respectively:
> +        *
> +        *          Cx * t_comp
> +        * x' = x - -----------
> +        *              100
> +        */
> +       sx = sx - (c->Cx * t_comp) / 100;
> +       sy1 = sy1 - (c->Cy1 * t_comp) / 100;
> +       sy2 = sy2 - (c->Cy2 * t_comp) / 100;
> +
>         /*
>          * Break y1 and y2 into y and z, y1 and y2 are apparently encoding
>          * y and z.
> @@ -426,36 +450,8 @@ static int yas530_get_measure(struct yas5xx *yas5xx, s32 *to, s32 *xo, s32 *yo,
>         sy = sy1 - sy2;
>         sz = -sy1 - sy2;
>
> -       /* Process temperature readout */
> -       switch (yas5xx->chip_info->devid) {
> -       case YAS530_DEVICE_ID:
> -               /*
> -                * Raw temperature value t is the number of counts starting
> -                * at -62 °C. Reference value t_ref is the number of counts
> -                * between -62 °C and 20 °C (82 °C range).
> -                *
> -                * Temperature in °C would be (82 / t_ref * t) - 62.
> -                *
> -                * Contrary to this, perform multiplication first and division
> -                * second due to calculating with integers.
> -                *
> -                * To get a nicer result, calculate with 1/10:s degrees Celsius
> -                * and finally multiply by 100 to return millidegrees Celsius.
> -                */
> -               *to = ((820 * t / t_ref) - 620) * 100;
> -               break;
> -       case YAS532_DEVICE_ID:
> -               /*
> -                * Actually same procedure for YAS532 but the starting point is
> -                * at -50 °C. Reference value t_ref is the number of counts
> -                * between -50 °C and 20 °C (70 °C range).
> -                */
> -               *to = ((700 * t / t_ref) - 500) * 100;
> -               break;
> -       default:
> -               dev_err(yas5xx->dev, "unknown device type\n");
> -               return -EINVAL;
> -       }
> +       /* Calculate temperature readout */
> +       *to = yas5xx_calc_temperature(yas5xx, t);
>
>         /*
>          * Calibrate [x,y,z] with some formulas like this:
> @@ -943,6 +939,8 @@ static const struct yas5xx_chip_info yas5xx_chip_info_tbl[] = {
>                 .volatile_reg = yas530_volatile_reg,
>                 .volatile_reg_qty = ARRAY_SIZE(yas530_volatile_reg),
>                 .scaling_val2 = 100000000, /* picotesla to Gauss */
> +               .t_ref = t_ref_counts[yas530],
> +               .min_temp_x10 = min_temp_celsius_x10[yas530],
>         },
>         [yas532] = {
>                 .devid = YAS532_DEVICE_ID,
> @@ -951,6 +949,8 @@ static const struct yas5xx_chip_info yas5xx_chip_info_tbl[] = {
>                 .volatile_reg = yas530_volatile_reg,
>                 .volatile_reg_qty = ARRAY_SIZE(yas530_volatile_reg),
>                 .scaling_val2 = 100000, /* nanotesla to Gauss */
> +               .t_ref = t_ref_counts[yas532],
> +               .min_temp_x10 = min_temp_celsius_x10[yas532],
>         },
>         [yas533] = {
>                 .devid = YAS532_DEVICE_ID,
> @@ -959,6 +959,8 @@ static const struct yas5xx_chip_info yas5xx_chip_info_tbl[] = {
>                 .volatile_reg = yas530_volatile_reg,
>                 .volatile_reg_qty = ARRAY_SIZE(yas530_volatile_reg),
>                 .scaling_val2 = 100000, /* nanotesla to Gauss */
> +               .t_ref = t_ref_counts[yas533],
> +               .min_temp_x10 = min_temp_celsius_x10[yas533],
>         },
>  };
>
> --
> 2.35.1
>


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 12/14] iio: magnetometer: yas530: Add temperature calculation to "chip_info"

[Jakob Hauser]

Hi Andy,

On 08.08.22 13:36, Andy Shevchenko wrote:
> On Mon, Aug 8, 2022 at 1:07 AM Jakob Hauser <jahau@rocketmail.com> wrote:
>>
>> Add temperature calculation to the "chip_info" structure to ease the handling
>> of different YAS variants.
> 
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

...

Due to C11 standard 6.6.7 considering array calls not as constant
expression (discussion on patch 9), the temperature values need to be
moved directly into the chip_info table as well.

I would move the comments in a reduced way into the kernel doc. It would
look like this:

/**
 * struct yas5xx_chip_info - device-specific data and function pointers
   ...
 * @t_ref: number of counts at reference temperature 20 °C
 * @min_temp_x10: starting point of temperature counting in 1/10:s
 * degrees Celsius
   ...
 *
 * The "t_ref" value for YAS532/533 is known from the Android driver.
 * For YAS530 it was approximately measured.
 *
 * The temperatures "min_temp_x10" are derived from the temperature
 * resolutions given in the data sheets.
 */
struct yas5xx_chip_info {
        ...
        u16 t_ref;
        s16 min_temp_x10;
        ...
};

static const struct yas5xx_chip_info yas5xx_chip_info_tbl[] = {
        [yas530] = {
                ...
                .t_ref = 182, /* counts */
                .min_temp_x10 = -620, /* 1/10:s degrees Celsius */
                ...
        },
        [yas532] = {
                ...
                .t_ref = 390, /* counts */
                .min_temp_x10 = -500, /* 1/10:s degrees Celsius */
                ...
        },
        [yas533] = {
                ...
                .t_ref = 390, /* counts */
                .min_temp_x10 = -500, /* 1/10:s degrees Celsius */
                ...
        },
};

As this is quite some change on that patch, I'd skip your "Reviewed-by:"
tag and you would need to review it again in v6.

Kind regards,
Jakob

[PATCH v5 13/14] iio: magnetometer: yas530: Add function pointers to "chip_info"

[Jakob Hauser]

Add funtion pointers to the "chip_info" structure to ease the handling of
different YAS variants.

In the function yas5xx_probe(), the function call for "measure_offsets" was
added as a conditional "if (yas5xx->chip_info->measure_offsets)". This is a
preparatory step for YAS537, as this variant doesn't need an offset
measurement.

Signed-off-by: Jakob Hauser <jahau@rocketmail.com>
---
 drivers/iio/magnetometer/yamaha-yas530.c | 65 +++++++++++++++---------
 1 file changed, 40 insertions(+), 25 deletions(-)

diff --git a/drivers/iio/magnetometer/yamaha-yas530.c b/drivers/iio/magnetometer/yamaha-yas530.c
index 780c7f4d1eff..62d5f23d8e08 100644
--- a/drivers/iio/magnetometer/yamaha-yas530.c
+++ b/drivers/iio/magnetometer/yamaha-yas530.c
@@ -168,6 +168,11 @@ struct yas5xx;
  * @scaling_val2: scaling value for IIO_CHAN_INFO_SCALE
  * @t_ref: number of counts at reference temperature
  * @min_temp_x10: starting point of temperature counting in 1/10:s degrees Celsius
+ * @get_measure: function pointer to get a measurement
+ * @get_calibration_data: function pointer to get calibration data
+ * @dump_calibration: function pointer to dump calibration for debugging
+ * @measure_offsets: function pointer to measure the offsets
+ * @power_on: function pointer to power-on procedure
  */
 struct yas5xx_chip_info {
 	unsigned int devid;
@@ -178,13 +183,18 @@ struct yas5xx_chip_info {
 	u32 scaling_val2;
 	u16 t_ref;
 	s16 min_temp_x10;
+	int (*get_measure)(struct yas5xx *yas5xx, s32 *to, s32 *xo, s32 *yo, s32 *zo);
+	int (*get_calibration_data)(struct yas5xx *yas5xx);
+	void (*dump_calibration)(struct yas5xx *yas5xx);
+	int (*measure_offsets)(struct yas5xx *yas5xx);
+	int (*power_on)(struct yas5xx *yas5xx);
 };
 
 /**
  * struct yas5xx - state container for the YAS5xx driver
  * @dev: parent device pointer
  * @chip: enumeration of the device variant
- * @chip_info: device-specific data
+ * @chip_info: device-specific data and function pointers
  * @version: device version
  * @calibration: calibration settings from the OTP storage
  * @hard_offsets: offsets for each axis measured with initcoil actuated
@@ -488,7 +498,7 @@ static int yas5xx_read_raw(struct iio_dev *indio_dev,
 	case IIO_CHAN_INFO_PROCESSED:
 	case IIO_CHAN_INFO_RAW:
 		pm_runtime_get_sync(yas5xx->dev);
-		ret = yas530_get_measure(yas5xx, &t, &x, &y, &z);
+		ret = yas5xx->chip_info->get_measure(yas5xx, &t, &x, &y, &z);
 		pm_runtime_mark_last_busy(yas5xx->dev);
 		pm_runtime_put_autosuspend(yas5xx->dev);
 		if (ret)
@@ -528,7 +538,7 @@ static void yas5xx_fill_buffer(struct iio_dev *indio_dev)
 	int ret;
 
 	pm_runtime_get_sync(yas5xx->dev);
-	ret = yas530_get_measure(yas5xx, &t, &x, &y, &z);
+	ret = yas5xx->chip_info->get_measure(yas5xx, &t, &x, &y, &z);
 	pm_runtime_mark_last_busy(yas5xx->dev);
 	pm_runtime_put_autosuspend(yas5xx->dev);
 	if (ret) {
@@ -941,6 +951,11 @@ static const struct yas5xx_chip_info yas5xx_chip_info_tbl[] = {
 		.scaling_val2 = 100000000, /* picotesla to Gauss */
 		.t_ref = t_ref_counts[yas530],
 		.min_temp_x10 = min_temp_celsius_x10[yas530],
+		.get_measure = yas530_get_measure,
+		.get_calibration_data = yas530_get_calibration_data,
+		.dump_calibration = yas530_dump_calibration,
+		.measure_offsets = yas530_measure_offsets,
+		.power_on = yas530_power_on,
 	},
 	[yas532] = {
 		.devid = YAS532_DEVICE_ID,
@@ -951,6 +966,11 @@ static const struct yas5xx_chip_info yas5xx_chip_info_tbl[] = {
 		.scaling_val2 = 100000, /* nanotesla to Gauss */
 		.t_ref = t_ref_counts[yas532],
 		.min_temp_x10 = min_temp_celsius_x10[yas532],
+		.get_measure = yas530_get_measure,
+		.get_calibration_data = yas532_get_calibration_data,
+		.dump_calibration = yas530_dump_calibration,
+		.measure_offsets = yas530_measure_offsets,
+		.power_on = yas530_power_on,
 	},
 	[yas533] = {
 		.devid = YAS532_DEVICE_ID,
@@ -961,6 +981,11 @@ static const struct yas5xx_chip_info yas5xx_chip_info_tbl[] = {
 		.scaling_val2 = 100000, /* nanotesla to Gauss */
 		.t_ref = t_ref_counts[yas533],
 		.min_temp_x10 = min_temp_celsius_x10[yas533],
+		.get_measure = yas530_get_measure,
+		.get_calibration_data = yas532_get_calibration_data,
+		.dump_calibration = yas530_dump_calibration,
+		.measure_offsets = yas530_measure_offsets,
+		.power_on = yas530_power_on,
 	},
 };
 
@@ -1031,35 +1056,25 @@ static int yas5xx_probe(struct i2c_client *i2c,
 		goto assert_reset;
 	}
 
-	switch (yas5xx->chip_info->devid) {
-	case YAS530_DEVICE_ID:
-		ret = yas530_get_calibration_data(yas5xx);
-		if (ret)
-			goto assert_reset;
-		break;
-	case YAS532_DEVICE_ID:
-		ret = yas532_get_calibration_data(yas5xx);
-		if (ret)
-			goto assert_reset;
-		break;
-	default:
-		ret = -ENODEV;
-		dev_err(dev, "unhandled device ID %02x\n",
-			yas5xx->chip_info->devid);
+	ret = yas5xx->chip_info->get_calibration_data(yas5xx);
+	if (ret)
 		goto assert_reset;
-	}
 
 	dev_info(dev, "detected %s %s\n", yas5xx->chip_info->product_name,
 		 yas5xx->chip_info->version_name[yas5xx->version]);
 
-	yas530_dump_calibration(yas5xx);
-	ret = yas530_power_on(yas5xx);
-	if (ret)
-		goto assert_reset;
-	ret = yas530_measure_offsets(yas5xx);
+	yas5xx->chip_info->dump_calibration(yas5xx);
+
+	ret = yas5xx->chip_info->power_on(yas5xx);
 	if (ret)
 		goto assert_reset;
 
+	if (yas5xx->chip_info->measure_offsets) {
+		ret = yas5xx->chip_info->measure_offsets(yas5xx);
+		if (ret)
+			goto assert_reset;
+	}
+
 	indio_dev->info = &yas5xx_info;
 	indio_dev->available_scan_masks = yas5xx_scan_masks;
 	indio_dev->modes = INDIO_DIRECT_MODE;
@@ -1155,7 +1170,7 @@ static int __maybe_unused yas5xx_runtime_resume(struct device *dev)
 	usleep_range(31000, 40000);
 	gpiod_set_value_cansleep(yas5xx->reset, 0);
 
-	ret = yas530_power_on(yas5xx);
+	ret = yas5xx->chip_info->power_on(yas5xx);
 	if (ret) {
 		dev_err(dev, "cannot power on\n");
 		goto out_reset;
-- 
2.35.1

Re: [PATCH v5 13/14] iio: magnetometer: yas530: Add function pointers to "chip_info"

[Andy Shevchenko]

On Mon, Aug 8, 2022 at 1:07 AM Jakob Hauser <jahau@rocketmail.com> wrote:
>
> Add funtion pointers to the "chip_info" structure to ease the handling of

function

> different YAS variants.
>
> In the function yas5xx_probe(), the function call for "measure_offsets" was
> added as a conditional "if (yas5xx->chip_info->measure_offsets)". This is a
> preparatory step for YAS537, as this variant doesn't need an offset
> measurement.

Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

> Signed-off-by: Jakob Hauser <jahau@rocketmail.com>
> ---
>  drivers/iio/magnetometer/yamaha-yas530.c | 65 +++++++++++++++---------
>  1 file changed, 40 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/iio/magnetometer/yamaha-yas530.c b/drivers/iio/magnetometer/yamaha-yas530.c
> index 780c7f4d1eff..62d5f23d8e08 100644
> --- a/drivers/iio/magnetometer/yamaha-yas530.c
> +++ b/drivers/iio/magnetometer/yamaha-yas530.c
> @@ -168,6 +168,11 @@ struct yas5xx;
>   * @scaling_val2: scaling value for IIO_CHAN_INFO_SCALE
>   * @t_ref: number of counts at reference temperature
>   * @min_temp_x10: starting point of temperature counting in 1/10:s degrees Celsius
> + * @get_measure: function pointer to get a measurement
> + * @get_calibration_data: function pointer to get calibration data
> + * @dump_calibration: function pointer to dump calibration for debugging
> + * @measure_offsets: function pointer to measure the offsets
> + * @power_on: function pointer to power-on procedure
>   */
>  struct yas5xx_chip_info {
>         unsigned int devid;
> @@ -178,13 +183,18 @@ struct yas5xx_chip_info {
>         u32 scaling_val2;
>         u16 t_ref;
>         s16 min_temp_x10;
> +       int (*get_measure)(struct yas5xx *yas5xx, s32 *to, s32 *xo, s32 *yo, s32 *zo);
> +       int (*get_calibration_data)(struct yas5xx *yas5xx);
> +       void (*dump_calibration)(struct yas5xx *yas5xx);
> +       int (*measure_offsets)(struct yas5xx *yas5xx);
> +       int (*power_on)(struct yas5xx *yas5xx);
>  };
>
>  /**
>   * struct yas5xx - state container for the YAS5xx driver
>   * @dev: parent device pointer
>   * @chip: enumeration of the device variant
> - * @chip_info: device-specific data
> + * @chip_info: device-specific data and function pointers
>   * @version: device version
>   * @calibration: calibration settings from the OTP storage
>   * @hard_offsets: offsets for each axis measured with initcoil actuated
> @@ -488,7 +498,7 @@ static int yas5xx_read_raw(struct iio_dev *indio_dev,
>         case IIO_CHAN_INFO_PROCESSED:
>         case IIO_CHAN_INFO_RAW:
>                 pm_runtime_get_sync(yas5xx->dev);
> -               ret = yas530_get_measure(yas5xx, &t, &x, &y, &z);
> +               ret = yas5xx->chip_info->get_measure(yas5xx, &t, &x, &y, &z);
>                 pm_runtime_mark_last_busy(yas5xx->dev);
>                 pm_runtime_put_autosuspend(yas5xx->dev);
>                 if (ret)
> @@ -528,7 +538,7 @@ static void yas5xx_fill_buffer(struct iio_dev *indio_dev)
>         int ret;
>
>         pm_runtime_get_sync(yas5xx->dev);
> -       ret = yas530_get_measure(yas5xx, &t, &x, &y, &z);
> +       ret = yas5xx->chip_info->get_measure(yas5xx, &t, &x, &y, &z);
>         pm_runtime_mark_last_busy(yas5xx->dev);
>         pm_runtime_put_autosuspend(yas5xx->dev);
>         if (ret) {
> @@ -941,6 +951,11 @@ static const struct yas5xx_chip_info yas5xx_chip_info_tbl[] = {
>                 .scaling_val2 = 100000000, /* picotesla to Gauss */
>                 .t_ref = t_ref_counts[yas530],
>                 .min_temp_x10 = min_temp_celsius_x10[yas530],
> +               .get_measure = yas530_get_measure,
> +               .get_calibration_data = yas530_get_calibration_data,
> +               .dump_calibration = yas530_dump_calibration,
> +               .measure_offsets = yas530_measure_offsets,
> +               .power_on = yas530_power_on,
>         },
>         [yas532] = {
>                 .devid = YAS532_DEVICE_ID,
> @@ -951,6 +966,11 @@ static const struct yas5xx_chip_info yas5xx_chip_info_tbl[] = {
>                 .scaling_val2 = 100000, /* nanotesla to Gauss */
>                 .t_ref = t_ref_counts[yas532],
>                 .min_temp_x10 = min_temp_celsius_x10[yas532],
> +               .get_measure = yas530_get_measure,
> +               .get_calibration_data = yas532_get_calibration_data,
> +               .dump_calibration = yas530_dump_calibration,
> +               .measure_offsets = yas530_measure_offsets,
> +               .power_on = yas530_power_on,
>         },
>         [yas533] = {
>                 .devid = YAS532_DEVICE_ID,
> @@ -961,6 +981,11 @@ static const struct yas5xx_chip_info yas5xx_chip_info_tbl[] = {
>                 .scaling_val2 = 100000, /* nanotesla to Gauss */
>                 .t_ref = t_ref_counts[yas533],
>                 .min_temp_x10 = min_temp_celsius_x10[yas533],
> +               .get_measure = yas530_get_measure,
> +               .get_calibration_data = yas532_get_calibration_data,
> +               .dump_calibration = yas530_dump_calibration,
> +               .measure_offsets = yas530_measure_offsets,
> +               .power_on = yas530_power_on,
>         },
>  };
>
> @@ -1031,35 +1056,25 @@ static int yas5xx_probe(struct i2c_client *i2c,
>                 goto assert_reset;
>         }
>
> -       switch (yas5xx->chip_info->devid) {
> -       case YAS530_DEVICE_ID:
> -               ret = yas530_get_calibration_data(yas5xx);
> -               if (ret)
> -                       goto assert_reset;
> -               break;
> -       case YAS532_DEVICE_ID:
> -               ret = yas532_get_calibration_data(yas5xx);
> -               if (ret)
> -                       goto assert_reset;
> -               break;
> -       default:
> -               ret = -ENODEV;
> -               dev_err(dev, "unhandled device ID %02x\n",
> -                       yas5xx->chip_info->devid);
> +       ret = yas5xx->chip_info->get_calibration_data(yas5xx);
> +       if (ret)
>                 goto assert_reset;
> -       }
>
>         dev_info(dev, "detected %s %s\n", yas5xx->chip_info->product_name,
>                  yas5xx->chip_info->version_name[yas5xx->version]);
>
> -       yas530_dump_calibration(yas5xx);
> -       ret = yas530_power_on(yas5xx);
> -       if (ret)
> -               goto assert_reset;
> -       ret = yas530_measure_offsets(yas5xx);
> +       yas5xx->chip_info->dump_calibration(yas5xx);
> +
> +       ret = yas5xx->chip_info->power_on(yas5xx);
>         if (ret)
>                 goto assert_reset;
>
> +       if (yas5xx->chip_info->measure_offsets) {
> +               ret = yas5xx->chip_info->measure_offsets(yas5xx);
> +               if (ret)
> +                       goto assert_reset;
> +       }
> +
>         indio_dev->info = &yas5xx_info;
>         indio_dev->available_scan_masks = yas5xx_scan_masks;
>         indio_dev->modes = INDIO_DIRECT_MODE;
> @@ -1155,7 +1170,7 @@ static int __maybe_unused yas5xx_runtime_resume(struct device *dev)
>         usleep_range(31000, 40000);
>         gpiod_set_value_cansleep(yas5xx->reset, 0);
>
> -       ret = yas530_power_on(yas5xx);
> +       ret = yas5xx->chip_info->power_on(yas5xx);
>         if (ret) {
>                 dev_err(dev, "cannot power on\n");
>                 goto out_reset;
> --
> 2.35.1
>


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 13/14] iio: magnetometer: yas530: Add function pointers to "chip_info"

[Jakob Hauser]

Hi Andy,

On 08.08.22 13:37, Andy Shevchenko wrote:
> On Mon, Aug 8, 2022 at 1:07 AM Jakob Hauser <jahau@rocketmail.com> wrote:
>>
>> Add funtion pointers to the "chip_info" structure to ease the handling of
> 
> function

Oops. Will correct this.

>> different YAS variants.
>>

...

Kind regards,
Jakob

[PATCH v5 14/14] iio: magnetometer: yas530: Add YAS537 variant

[Jakob Hauser]

This adds support for the magnetometer Yamaha YAS537. The additions are based
on comparison of Yamaha Android kernel drivers for YAS532 [1] and YAS537 [2].

In the Yamaha YAS537 Android driver, there is an overflow/underflow control
implemented. For regular usage, this seems not necessary. A similar overflow/
underflow control of Yamaha YAS530/532 Android driver isn't integrated in the
mainline driver. It is therefore skipped for YAS537 in mainline too.

Also in the Yamaha YAS537 Android driver, at the end of the reset_yas537()
function, a measurement is saved in "last_after_rcoil". Later on, this is
compared to current measurements. If the difference gets too big, a new
reset is initialized. The difference in measurements needs to be quite big,
it's hard to say if this is necessary for regular operation. Therefore this
isn't integrated in the mainline driver either.

[1] https://github.com/msm8916-mainline/android_kernel_qcom_msm8916/blob/GT-I9195I/drivers/iio/magnetometer/yas_mag_drv-yas532.c
[2] https://github.com/msm8916-mainline/android_kernel_qcom_msm8916/blob/GT-I9195I/drivers/iio/magnetometer/yas_mag_drv-yas537.c

Signed-off-by: Jakob Hauser <jahau@rocketmail.com>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
---
Result can be seen at:
https://github.com/Jakko3/linux/blob/yas537_v5/drivers/iio/magnetometer/yamaha-yas530.c

 drivers/iio/magnetometer/Kconfig         |   4 +-
 drivers/iio/magnetometer/yamaha-yas530.c | 436 ++++++++++++++++++++++-
 2 files changed, 431 insertions(+), 9 deletions(-)

diff --git a/drivers/iio/magnetometer/Kconfig b/drivers/iio/magnetometer/Kconfig
index 07eb619bcfe8..b91fc5e6a26e 100644
--- a/drivers/iio/magnetometer/Kconfig
+++ b/drivers/iio/magnetometer/Kconfig
@@ -216,8 +216,8 @@ config YAMAHA_YAS530
 	select IIO_TRIGGERED_BUFFER
 	help
 	  Say Y here to add support for the Yamaha YAS530 series of
-	  3-Axis Magnetometers. Right now YAS530, YAS532 and YAS533 are
-	  fully supported.
+	  3-Axis Magnetometers. YAS530, YAS532, YAS533 and YAS537 are
+	  supported.
 
 	  This driver can also be compiled as a module.
 	  To compile this driver as a module, choose M here: the module
diff --git a/drivers/iio/magnetometer/yamaha-yas530.c b/drivers/iio/magnetometer/yamaha-yas530.c
index 62d5f23d8e08..004bd6fcc6f3 100644
--- a/drivers/iio/magnetometer/yamaha-yas530.c
+++ b/drivers/iio/magnetometer/yamaha-yas530.c
@@ -17,6 +17,9 @@
  * named "inv_compass" in the Tegra Android kernel tree.
  * Copyright (C) 2012 InvenSense Corporation
  *
+ * Code functions for YAS537 based on Yamaha Android kernel driver.
+ * Copyright (c) 2014 Yamaha Corporation
+ *
  * Author: Linus Walleij <linus.walleij@linaro.org>
  */
 #include <linux/bitfield.h>
@@ -32,6 +35,7 @@
 #include <linux/regmap.h>
 #include <linux/regulator/consumer.h>
 #include <linux/random.h>
+#include <linux/units.h>
 
 #include <linux/iio/buffer.h>
 #include <linux/iio/iio.h>
@@ -56,6 +60,23 @@
 #define YAS530_TEST2			0x89
 #define YAS530_CAL			0x90
 
+/* Registers used by YAS537 */
+#define YAS537_MEASURE			0x81 /* Originally YAS537_REG_CMDR */
+#define YAS537_CONFIG			0x82 /* Originally YAS537_REG_CONFR */
+#define YAS537_MEASURE_INTERVAL		0x83 /* Originally YAS537_REG_INTRVLR */
+#define YAS537_OFFSET_X			0x84 /* Originally YAS537_REG_OXR */
+#define YAS537_OFFSET_Y1		0x85 /* Originally YAS537_REG_OY1R */
+#define YAS537_OFFSET_Y2		0x86 /* Originally YAS537_REG_OY2R */
+#define YAS537_AVR			0x87
+#define YAS537_HCK			0x88
+#define YAS537_LCK			0x89
+#define YAS537_SRST			0x90
+#define YAS537_ADCCAL			0x91
+#define YAS537_MTC			0x93
+#define YAS537_OC			0x9E
+#define YAS537_TRM			0x9F
+#define YAS537_CAL			0xC0
+
 /* Bits in the YAS5xx config register */
 #define YAS5XX_CONFIG_INTON		BIT(0) /* Interrupt on? */
 #define YAS5XX_CONFIG_INTHACT		BIT(1) /* Interrupt active high? */
@@ -67,6 +88,7 @@
 #define YAS5XX_MEASURE_LDTC		BIT(1)
 #define YAS5XX_MEASURE_FORS		BIT(2)
 #define YAS5XX_MEASURE_DLYMES		BIT(4)
+#define YAS5XX_MEASURE_CONT		BIT(5)
 
 /* Bits in the measure data register */
 #define YAS5XX_MEASURE_DATA_BUSY	BIT(7)
@@ -91,6 +113,22 @@
 #define YAS532_DATA_CENTER		BIT(YAS532_DATA_BITS - 1)
 #define YAS532_DATA_OVERFLOW		(BIT(YAS532_DATA_BITS) - 1)
 
+#define YAS537_DEVICE_ID		0x07 /* YAS537 (MS-3T) */
+#define YAS537_VERSION_0		0 /* Version naming unknown */
+#define YAS537_VERSION_1		1 /* Version naming unknown */
+#define YAS537_MAG_AVERAGE_32_MASK	GENMASK(6, 4)
+#define YAS537_MEASURE_TIME_WORST_US	1500
+#define YAS537_DEFAULT_SENSOR_DELAY_MS	50
+#define YAS537_MAG_RCOIL_TIME_US	65
+#define YAS537_MTC3_MASK_PREP		GENMASK(7, 0)
+#define YAS537_MTC3_MASK_GET		GENMASK(7, 5)
+#define YAS537_MTC3_ADD_BIT		BIT(4)
+#define YAS537_HCK_MASK_PREP		GENMASK(4, 0)
+#define YAS537_HCK_MASK_GET		GENMASK(7, 4)
+#define YAS537_LCK_MASK_PREP		GENMASK(4, 0)
+#define YAS537_LCK_MASK_GET		GENMASK(3, 0)
+#define YAS537_OC_MASK_GET		GENMASK(5, 0)
+
 /* Turn off device regulators etc after 5 seconds of inactivity */
 #define YAS5XX_AUTOSUSPEND_DELAY_MS	5000
 
@@ -98,18 +136,21 @@ enum chip_ids {
 	yas530,
 	yas532,
 	yas533,
+	yas537,
 };
 
 static const char * const yas5xx_product_name[] = {
 	"YAS530 MS-3E",
 	"YAS532 MS-3R",
 	"YAS533 MS-3F",
+	"YAS537 MS-3T",
 };
 
 static const char * const yas5xx_version_names[][2] = {
 	[yas530] = { "A", "B" },
 	[yas532] = { "AB", "AC" },
 	[yas533] = { "AB", "AC" },
+	[yas537] = { "v0", "v1" },
 };
 
 static const int yas530_volatile_reg[] = {
@@ -117,6 +158,10 @@ static const int yas530_volatile_reg[] = {
 	YAS530_MEASURE,
 };
 
+static const int yas537_volatile_reg[] = {
+	YAS537_MEASURE,
+};
+
 /*
  * t_ref_counts is the number of counts at reference temperature.
  *
@@ -124,23 +169,23 @@ static const int yas530_volatile_reg[] = {
  * values in t_ref_counts[] are the counts at the reference temperature
  * of 20 °C.
  *
- * For YAS532/533, this value is known from the Android driver. For YAS530,
- * it was approximately measured.
+ * For YAS532/533, this value is known from the Android driver. For YAS530
+ * and YAS537, it was approximately measured.
  */
-static const u16 t_ref_counts[] = { 182, 390, 390 };
+static const u16 t_ref_counts[] = { 182, 390, 390, 8120 };
 
 /*
  * min_temp_celsius_x10 is the starting point of temperature counting
  * in 1/10:s degrees Celsius.
  *
- * The array min_temp_celsius_x10[] contains the temperatures where the
- * temperature value count is 0. The values are in 1/10:s degrees Celsius
- * to ease the further temperature calculation.
+ * The array min_temp_celsius_x10[] contains the theoretical temperatures
+ * where the temperature value count is 0. The values are in 1/10:s degrees
+ * Celsius to ease the further temperature calculation.
  *
  * These temperatures are derived from the temperature resolutions given
  * in the data sheets.
  */
-static const s16 min_temp_celsius_x10[] = { -620, -500, -500 };
+static const s16 min_temp_celsius_x10[] = { -620, -500, -500, -3860 };
 
 struct yas5xx_calibration {
 	/* Linearization calibration x, y1, y2 */
@@ -344,6 +389,77 @@ static int yas530_measure(struct yas5xx *yas5xx, u16 *t, u16 *x, u16 *y1, u16 *y
 	return ret;
 }
 
+/**
+ * yas537_measure() - Make a measure from the hardware
+ * @yas5xx: The device state
+ * @t: the raw temperature measurement
+ * @x: the raw x axis measurement
+ * @y1: the y1 axis measurement
+ * @y2: the y2 axis measurement
+ * @return: 0 on success or error code
+ */
+static int yas537_measure(struct yas5xx *yas5xx, u16 *t, u16 *x, u16 *y1, u16 *y2)
+{
+	struct yas5xx_calibration *c = &yas5xx->calibration;
+	unsigned int busy;
+	u8 data[8];
+	u16 xy1y2[3];
+	s32 h[3], s[3];
+	int i, ret;
+
+	mutex_lock(&yas5xx->lock);
+
+	/* Contrary to YAS530/532, also a "cont" bit is set, meaning unknown */
+	ret = regmap_write(yas5xx->map, YAS537_MEASURE, YAS5XX_MEASURE_START |
+			   YAS5XX_MEASURE_CONT);
+	if (ret < 0)
+		goto out_unlock;
+
+	/* Use same timeout like YAS530/532 but the bit is in data row 2 */
+	ret = regmap_read_poll_timeout(yas5xx->map, YAS5XX_MEASURE_DATA + 2, busy,
+				       !(busy & YAS5XX_MEASURE_DATA_BUSY),
+				       500, 20000);
+	if (ret) {
+		dev_err(yas5xx->dev, "timeout waiting for measurement\n");
+		goto out_unlock;
+	}
+
+	ret = regmap_bulk_read(yas5xx->map, YAS5XX_MEASURE_DATA,
+			       data, sizeof(data));
+	if (ret)
+		goto out_unlock;
+
+	mutex_unlock(&yas5xx->lock);
+
+	*t = get_unaligned_be16(&data[0]);
+	xy1y2[0] = FIELD_GET(GENMASK(13, 0), get_unaligned_be16(&data[2]));
+	xy1y2[1] = get_unaligned_be16(&data[4]);
+	xy1y2[2] = get_unaligned_be16(&data[6]);
+
+	/* The second version of YAS537 needs to include calibration coefficients */
+	if (yas5xx->version == YAS537_VERSION_1) {
+		for (i = 0; i < 3; i++)
+			s[i] = xy1y2[i] - BIT(13);
+		h[0] = (c->k *   (128 * s[0] + c->a2 * s[1] + c->a3 * s[2])) / BIT(13);
+		h[1] = (c->k * (c->a4 * s[0] + c->a5 * s[1] + c->a6 * s[2])) / BIT(13);
+		h[2] = (c->k * (c->a7 * s[0] + c->a8 * s[1] + c->a9 * s[2])) / BIT(13);
+		for (i = 0; i < 3; i++) {
+			clamp_val(h[i], -BIT(13), BIT(13) - 1);
+			xy1y2[i] = h[i] + BIT(13);
+		}
+	}
+
+	*x = xy1y2[0];
+	*y1 = xy1y2[1];
+	*y2 = xy1y2[2];
+
+	return 0;
+
+out_unlock:
+	mutex_unlock(&yas5xx->lock);
+	return ret;
+}
+
 /* Used by YAS530, YAS532 and YAS533 */
 static s32 yas530_linearize(struct yas5xx *yas5xx, u16 val, int axis)
 {
@@ -485,6 +601,41 @@ static int yas530_get_measure(struct yas5xx *yas5xx, s32 *to, s32 *xo, s32 *yo,
 	return 0;
 }
 
+/**
+ * yas537_get_measure() - Measure a sample of all axis and process
+ * @yas5xx: The device state
+ * @to: Temperature out
+ * @xo: X axis out
+ * @yo: Y axis out
+ * @zo: Z axis out
+ * @return: 0 on success or error code
+ */
+static int yas537_get_measure(struct yas5xx *yas5xx, s32 *to, s32 *xo, s32 *yo, s32 *zo)
+{
+	u16 t, x, y1, y2;
+	int ret;
+
+	/* We first get raw data that needs to be translated to [x,y,z] */
+	ret = yas537_measure(yas5xx, &t, &x, &y1, &y2);
+	if (ret)
+		return ret;
+
+	/* Calculate temperature readout */
+	*to = yas5xx_calc_temperature(yas5xx, t);
+
+	/*
+	 * Unfortunately, no linearization or temperature compensation formulas
+	 * are known for YAS537.
+	 */
+
+	/* Calculate x, y, z from x, y1, y2 */
+	*xo = (x - BIT(13)) * 300;
+	*yo = (y1 - y2) * 1732 / 10;
+	*zo = (-y1 - y2 + BIT(14)) * 300;
+
+	return 0;
+}
+
 static int yas5xx_read_raw(struct iio_dev *indio_dev,
 			   struct iio_chan_spec const *chan,
 			   int *val, int *val2,
@@ -797,6 +948,202 @@ static int yas532_get_calibration_data(struct yas5xx *yas5xx)
 	return 0;
 }
 
+static int yas537_get_calibration_data(struct yas5xx *yas5xx)
+{
+	struct yas5xx_calibration *c = &yas5xx->calibration;
+	u8 data[17];
+	u32 val1, val2, val3, val4;
+	int i, ret;
+
+	/* Writing SRST register */
+	ret = regmap_write(yas5xx->map, YAS537_SRST, BIT(1));
+	if (ret)
+		return ret;
+
+	/* Calibration readout, YAS537 needs one readout only */
+	ret = regmap_bulk_read(yas5xx->map, YAS537_CAL, data, sizeof(data));
+	if (ret)
+		return ret;
+	dev_dbg(yas5xx->dev, "calibration data: %17ph\n", data);
+
+	/* Sanity check, is this all zeroes? */
+	if (!memchr_inv(data, 0x00, 16) && !FIELD_GET(GENMASK(5, 0), data[16]))
+		dev_warn(yas5xx->dev, "calibration is blank!\n");
+
+	/* Contribute calibration data to the input pool for kernel entropy */
+	add_device_randomness(data, sizeof(data));
+
+	/* Extract version information */
+	yas5xx->version = FIELD_GET(GENMASK(7, 6), data[16]);
+
+	/* There are two versions of YAS537 behaving differently */
+	switch (yas5xx->version) {
+	case YAS537_VERSION_0:
+		/*
+		 * The first version simply writes data back into registers:
+		 *
+		 * data[0]  YAS537_MTC		0x93
+		 * data[1]			0x94
+		 * data[2]			0x95
+		 * data[3]			0x96
+		 * data[4]			0x97
+		 * data[5]			0x98
+		 * data[6]			0x99
+		 * data[7]			0x9a
+		 * data[8]			0x9b
+		 * data[9]			0x9c
+		 * data[10]			0x9d
+		 * data[11] YAS537_OC		0x9e
+		 *
+		 * data[12] YAS537_OFFSET_X	0x84
+		 * data[13] YAS537_OFFSET_Y1	0x85
+		 * data[14] YAS537_OFFSET_Y2	0x86
+		 *
+		 * data[15] YAS537_HCK		0x88
+		 * data[16] YAS537_LCK		0x89
+		 */
+		for (i = 0; i < 12; i++) {
+			ret = regmap_write(yas5xx->map, YAS537_MTC + i,
+					   data[i]);
+			if (ret)
+				return ret;
+		}
+		for (i = 0; i < 3; i++) {
+			ret = regmap_write(yas5xx->map, YAS537_OFFSET_X + i,
+					   data[i + 12]);
+			if (ret)
+				return ret;
+			yas5xx->hard_offsets[i] = data[i + 12];
+		}
+		for (i = 0; i < 2; i++) {
+			ret = regmap_write(yas5xx->map, YAS537_HCK + i,
+					   data[i + 15]);
+			if (ret)
+				return ret;
+		}
+		break;
+	case YAS537_VERSION_1:
+		/*
+		 * The second version writes some data into registers but also
+		 * extracts calibration coefficients.
+		 *
+		 * Registers being written:
+		 *
+		 * data[0]  YAS537_MTC			0x93
+		 * data[1]  YAS537_MTC+1		0x94
+		 * data[2]  YAS537_MTC+2		0x95
+		 * data[3]  YAS537_MTC+3 (partially)	0x96
+		 *
+		 * data[12] YAS537_OFFSET_X		0x84
+		 * data[13] YAS537_OFFSET_Y1		0x85
+		 * data[14] YAS537_OFFSET_Y2		0x86
+		 *
+		 * data[15] YAS537_HCK (partially)	0x88
+		 *          YAS537_LCK (partially)	0x89
+		 * data[16] YAS537_OC  (partially)	0x9e
+		 */
+		for (i = 0; i < 3; i++) {
+			ret = regmap_write(yas5xx->map, YAS537_MTC + i,
+					   data[i]);
+			if (ret)
+				return ret;
+		}
+		for (i = 0; i < 3; i++) {
+			ret = regmap_write(yas5xx->map, YAS537_OFFSET_X + i,
+					   data[i + 12]);
+			if (ret)
+				return ret;
+			yas5xx->hard_offsets[i] = data[i + 12];
+		}
+		/*
+		 * Visualization of partially taken data:
+		 *
+		 * data[3]       n 7 6 5 4 3 2 1 0
+		 * YAS537_MTC+3    x x x 1 0 0 0 0
+		 *
+		 * data[15]      n 7 6 5 4 3 2 1 0
+		 * YAS537_HCK      x x x x 0
+		 *
+		 * data[15]      n 7 6 5 4 3 2 1 0
+		 * YAS537_LCK              x x x x 0
+		 *
+		 * data[16]      n 7 6 5 4 3 2 1 0
+		 * YAS537_OC           x x x x x x
+		 */
+		ret = regmap_write(yas5xx->map, YAS537_MTC + 3,
+				   FIELD_PREP(YAS537_MTC3_MASK_PREP,
+				   FIELD_GET(YAS537_MTC3_MASK_GET, data[3])) |
+				   YAS537_MTC3_ADD_BIT);
+		if (ret)
+			return ret;
+		ret = regmap_write(yas5xx->map, YAS537_HCK,
+				   FIELD_PREP(YAS537_HCK_MASK_PREP,
+				   FIELD_GET(YAS537_HCK_MASK_GET, data[15])));
+		if (ret)
+			return ret;
+		ret = regmap_write(yas5xx->map, YAS537_LCK,
+				   FIELD_PREP(YAS537_LCK_MASK_PREP,
+				   FIELD_GET(YAS537_LCK_MASK_GET, data[15])));
+		if (ret)
+			return ret;
+		ret = regmap_write(yas5xx->map, YAS537_OC,
+				   FIELD_GET(YAS537_OC_MASK_GET, data[16]));
+		if (ret)
+			return ret;
+		/*
+		 * For data extraction, build some blocks. Four 32-bit blocks
+		 * look appropriate.
+		 *
+		 *            n    7  6  5  4  3  2  1  0
+		 *  data[0]   0 [ Cx Cx Cx Cx Cx Cx Cx Cx ] bits 31 .. 24
+		 *  data[1]   1 [ Cx C1 C1 C1 C1 C1 C1 C1 ] bits 23 .. 16
+		 *  data[2]   2 [ C1 C1 C2 C2 C2 C2 C2 C2 ] bits 15 .. 8
+		 *  data[3]   3 [ C2 C2 C2                ] bits  7 .. 0
+		 *
+		 *            n    7  6  5  4  3  2  1  0
+		 *  data[3]   0 [          a2 a2 a2 a2 a2 ] bits 31 .. 24
+		 *  data[4]   1 [ a2 a2 a3 a3 a3 a3 a3 a3 ] bits 23 .. 16
+		 *  data[5]   2 [ a3 a4 a4 a4 a4 a4 a4 a4 ] bits 15 .. 8
+		 *  data[6]   3 [ a4                      ] bits  7 .. 0
+		 *
+		 *            n    7  6  5  4  3  2  1  0
+		 *  data[6]   0 [    a5 a5 a5 a5 a5 a5 a5 ] bits 31 .. 24
+		 *  data[7]   1 [ a5 a5 a6 a6 a6 a6 a6 a6 ] bits 23 .. 16
+		 *  data[8]   2 [ a6 a7 a7 a7 a7 a7 a7 a7 ] bits 15 .. 8
+		 *  data[9]   3 [ a7                      ] bits  7 .. 0
+		 *
+		 *            n    7  6  5  4  3  2  1  0
+		 *  data[9]   0 [    a8 a8 a8 a8 a8 a8 a8 ] bits 31 .. 24
+		 *  data[10]  1 [ a9 a9 a9 a9 a9 a9 a9 a9 ] bits 23 .. 16
+		 *  data[11]  2 [ a9  k  k  k  k  k  k  k ] bits 15 .. 8
+		 *  data[12]  3 [                         ] bits  7 .. 0
+		 */
+		val1 = get_unaligned_be32(&data[0]);
+		val2 = get_unaligned_be32(&data[3]);
+		val3 = get_unaligned_be32(&data[6]);
+		val4 = get_unaligned_be32(&data[9]);
+		/* Extract calibration coefficients and modify */
+		c->Cx  = FIELD_GET(GENMASK(31, 23), val1) - 256;
+		c->Cy1 = FIELD_GET(GENMASK(22, 14), val1) - 256;
+		c->Cy2 = FIELD_GET(GENMASK(13,  5), val1) - 256;
+		c->a2  = FIELD_GET(GENMASK(28, 22), val2) -  64;
+		c->a3  = FIELD_GET(GENMASK(21, 15), val2) -  64;
+		c->a4  = FIELD_GET(GENMASK(14,  7), val2) - 128;
+		c->a5  = FIELD_GET(GENMASK(30, 22), val3) - 112;
+		c->a6  = FIELD_GET(GENMASK(21, 15), val3) -  64;
+		c->a7  = FIELD_GET(GENMASK(14,  7), val3) - 128;
+		c->a8  = FIELD_GET(GENMASK(30, 24), val4) -  64;
+		c->a9  = FIELD_GET(GENMASK(23, 15), val4) - 112;
+		c->k   = FIELD_GET(GENMASK(14,  8), val4);
+		break;
+	default:
+		dev_err(yas5xx->dev, "unknown version of YAS537\n");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 /* Used by YAS530, YAS532 and YAS533 */
 static void yas530_dump_calibration(struct yas5xx *yas5xx)
 {
@@ -821,6 +1168,26 @@ static void yas530_dump_calibration(struct yas5xx *yas5xx)
 	dev_dbg(yas5xx->dev, "dck = %d\n", c->dck);
 }
 
+static void yas537_dump_calibration(struct yas5xx *yas5xx)
+{
+	struct yas5xx_calibration *c = &yas5xx->calibration;
+
+	if (yas5xx->version == YAS537_VERSION_1) {
+		dev_dbg(yas5xx->dev, "Cx = %d\n", c->Cx);
+		dev_dbg(yas5xx->dev, "Cy1 = %d\n", c->Cy1);
+		dev_dbg(yas5xx->dev, "Cy2 = %d\n", c->Cy2);
+		dev_dbg(yas5xx->dev, "a2 = %d\n", c->a2);
+		dev_dbg(yas5xx->dev, "a3 = %d\n", c->a3);
+		dev_dbg(yas5xx->dev, "a4 = %d\n", c->a4);
+		dev_dbg(yas5xx->dev, "a5 = %d\n", c->a5);
+		dev_dbg(yas5xx->dev, "a6 = %d\n", c->a6);
+		dev_dbg(yas5xx->dev, "a7 = %d\n", c->a7);
+		dev_dbg(yas5xx->dev, "a8 = %d\n", c->a8);
+		dev_dbg(yas5xx->dev, "a9 = %d\n", c->a9);
+		dev_dbg(yas5xx->dev, "k = %d\n", c->k);
+	}
+}
+
 /* Used by YAS530, YAS532 and YAS533 */
 static int yas530_set_offsets(struct yas5xx *yas5xx, s8 ox, s8 oy1, s8 oy2)
 {
@@ -941,6 +1308,44 @@ static int yas530_power_on(struct yas5xx *yas5xx)
 	return regmap_write(yas5xx->map, YAS530_MEASURE_INTERVAL, 0);
 }
 
+static int yas537_power_on(struct yas5xx *yas5xx)
+{
+	__be16 buf;
+	int ret;
+	u8 intrvl;
+
+	/* Writing ADCCAL and TRM registers */
+	buf = cpu_to_be16(GENMASK(9, 3));
+	ret = regmap_bulk_write(yas5xx->map, YAS537_ADCCAL, &buf, sizeof(buf));
+	if (ret)
+		return ret;
+	ret = regmap_write(yas5xx->map, YAS537_TRM, GENMASK(7, 0));
+	if (ret)
+		return ret;
+
+	/* The interval value is static in regular operation */
+	intrvl = (YAS537_DEFAULT_SENSOR_DELAY_MS * MILLI
+		 - YAS537_MEASURE_TIME_WORST_US) / 4100;
+	ret = regmap_write(yas5xx->map, YAS537_MEASURE_INTERVAL, intrvl);
+	if (ret)
+		return ret;
+
+	/* The average value is also static in regular operation */
+	ret = regmap_write(yas5xx->map, YAS537_AVR, YAS537_MAG_AVERAGE_32_MASK);
+	if (ret)
+		return ret;
+
+	/* Perform the "rcoil" part but skip the "last_after_rcoil" read */
+	ret = regmap_write(yas5xx->map, YAS537_CONFIG, BIT(3));
+	if (ret)
+		return ret;
+
+	/* Wait until the coil has ramped up */
+	usleep_range(YAS537_MAG_RCOIL_TIME_US, YAS537_MAG_RCOIL_TIME_US + 100);
+
+	return 0;
+}
+
 static const struct yas5xx_chip_info yas5xx_chip_info_tbl[] = {
 	[yas530] = {
 		.devid = YAS530_DEVICE_ID,
@@ -987,6 +1392,21 @@ static const struct yas5xx_chip_info yas5xx_chip_info_tbl[] = {
 		.measure_offsets = yas530_measure_offsets,
 		.power_on = yas530_power_on,
 	},
+	[yas537] = {
+		.devid = YAS537_DEVICE_ID,
+		.product_name = yas5xx_product_name[yas537],
+		.version_name = yas5xx_version_names[yas537],
+		.volatile_reg = yas537_volatile_reg,
+		.volatile_reg_qty = ARRAY_SIZE(yas537_volatile_reg),
+		.scaling_val2 = 100000, /* nanotesla to Gauss */
+		.t_ref = t_ref_counts[yas537],
+		.min_temp_x10 = min_temp_celsius_x10[yas537],
+		.get_measure = yas537_get_measure,
+		.get_calibration_data = yas537_get_calibration_data,
+		.dump_calibration = yas537_dump_calibration,
+		/* .measure_offets is not needed for yas537 */
+		.power_on = yas537_power_on,
+	},
 };
 
 static int yas5xx_probe(struct i2c_client *i2c,
@@ -1196,6 +1616,7 @@ static const struct i2c_device_id yas5xx_id[] = {
 	{"yas530", yas530 },
 	{"yas532", yas532 },
 	{"yas533", yas533 },
+	{"yas537", yas537 },
 	{}
 };
 MODULE_DEVICE_TABLE(i2c, yas5xx_id);
@@ -1204,6 +1625,7 @@ static const struct of_device_id yas5xx_of_match[] = {
 	{ .compatible = "yamaha,yas530", },
 	{ .compatible = "yamaha,yas532", },
 	{ .compatible = "yamaha,yas533", },
+	{ .compatible = "yamaha,yas537", },
 	{}
 };
 MODULE_DEVICE_TABLE(of, yas5xx_of_match);
-- 
2.35.1

Re: [PATCH v5 14/14] iio: magnetometer: yas530: Add YAS537 variant

[Andy Shevchenko]

On Mon, Aug 8, 2022 at 1:12 AM Jakob Hauser <jahau@rocketmail.com> wrote:
>
> This adds support for the magnetometer Yamaha YAS537. The additions are based

Add support

> on comparison of Yamaha Android kernel drivers for YAS532 [1] and YAS537 [2].
>
> In the Yamaha YAS537 Android driver, there is an overflow/underflow control
> implemented. For regular usage, this seems not necessary. A similar overflow/
> underflow control of Yamaha YAS530/532 Android driver isn't integrated in the
> mainline driver. It is therefore skipped for YAS537 in mainline too.

the mainline

> Also in the Yamaha YAS537 Android driver, at the end of the reset_yas537()
> function, a measurement is saved in "last_after_rcoil". Later on, this is
> compared to current measurements. If the difference gets too big, a new
> reset is initialized. The difference in measurements needs to be quite big,
> it's hard to say if this is necessary for regular operation. Therefore this
> isn't integrated in the mainline driver either.

...

>         help
>           Say Y here to add support for the Yamaha YAS530 series of
> -         3-Axis Magnetometers. Right now YAS530, YAS532 and YAS533 are
> -         fully supported.
> +         3-Axis Magnetometers. YAS530, YAS532, YAS533 and YAS537 are
> +         supported.

So, after this change the rest become partially supported?

Perhaps you want to leave the original and add a new sentence like:

  "The YAS537 is partially supported."

?

...

> - * For YAS532/533, this value is known from the Android driver. For YAS530,

It seems this comma is unneeded in the original comment.

> - * it was approximately measured.
> + * For YAS532/533, this value is known from the Android driver. For YAS530
> + * and YAS537, it was approximately measured.

P.S. Do you see now how your series and the end result become better?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 14/14] iio: magnetometer: yas530: Add YAS537 variant

[Jakob Hauser]

Hi Andy,

On 08.08.22 13:47, Andy Shevchenko wrote:
> On Mon, Aug 8, 2022 at 1:12 AM Jakob Hauser <jahau@rocketmail.com> wrote:
>>
>> This adds support for the magnetometer Yamaha YAS537. The additions are based
> 
> Add support

OK

>> on comparison of Yamaha Android kernel drivers for YAS532 [1] and YAS537 [2].
>>
>> In the Yamaha YAS537 Android driver, there is an overflow/underflow control
>> implemented. For regular usage, this seems not necessary. A similar overflow/
>> underflow control of Yamaha YAS530/532 Android driver isn't integrated in the
>> mainline driver. It is therefore skipped for YAS537 in mainline too.
> 
> the mainline

OK

>> Also in the Yamaha YAS537 Android driver, at the end of the reset_yas537()
>> function, a measurement is saved in "last_after_rcoil". Later on, this is
>> compared to current measurements. If the difference gets too big, a new
>> reset is initialized. The difference in measurements needs to be quite big,
>> it's hard to say if this is necessary for regular operation. Therefore this
>> isn't integrated in the mainline driver either.
> 
> ..
> 
>>         help
>>           Say Y here to add support for the Yamaha YAS530 series of
>> -         3-Axis Magnetometers. Right now YAS530, YAS532 and YAS533 are
>> -         fully supported.
>> +         3-Axis Magnetometers. YAS530, YAS532, YAS533 and YAS537 are
>> +         supported.
> 
> So, after this change the rest become partially supported?
> 
> Perhaps you want to leave the original and add a new sentence like:
> 
>   "The YAS537 is partially supported."
> 
> ?

All four variants are fully supported. I changed the sentence according
to Jonathan's suggestion in v3:
https://lore.kernel.org/linux-iio/cover.1655509425.git.jahau@rocketmail.com/T/#m6ba6b576cdf6f86f550f7598fcf7c408ac5f2d46

> 
> ..
> 
>> - * For YAS532/533, this value is known from the Android driver. For YAS530,
> 
> It seems this comma is unneeded in the original comment.

I'll remove the comma here and in patch 12, where it was introduced.

>> - * it was approximately measured.
>> + * For YAS532/533, this value is known from the Android driver. For YAS530
>> + * and YAS537, it was approximately measured.
> 
> P.S. Do you see now how your series and the end result become better?

The driver improves. Though we kind of get lost in details, I have the
impression we could go on like this forever.

Kind regards,
Jakob

Re: [PATCH v5 14/14] iio: magnetometer: yas530: Add YAS537 variant

[Linus Walleij]

On Wed, Aug 10, 2022 at 1:41 AM Jakob Hauser <jahau@rocketmail.com> wrote:
> On 08.08.22 13:47, Andy Shevchenko wrote:

> > P.S. Do you see now how your series and the end result become better?
>
> The driver improves. Though we kind of get lost in details, I have the
> impression we could go on like this forever.

I think at one point Jonathan mentioned the phenomenon "perfect is the
enemy of good", which even has its own Wikipedia article:
https://en.wikipedia.org/wiki/Perfect_is_the_enemy_of_good

When I feel (for some intuitive definition of "feeling") that a patch
series to my subsystem is getting over-reviewed, I usually just
merge it and tell everyone involved to write and/or request
additional patches if they are troubled by the result. It's a fine line,
admittedly, it's not like I can define the trigger point.

I wished some core parts of the kernel get as much attention
and review as some IIO drivers get.

Yours,
Linus Walleij

Re: [PATCH v5 14/14] iio: magnetometer: yas530: Add YAS537 variant

[Andy Shevchenko]

On Sat, Aug 13, 2022 at 12:43 AM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> On Wed, Aug 10, 2022 at 1:41 AM Jakob Hauser <jahau@rocketmail.com> wrote:
> > On 08.08.22 13:47, Andy Shevchenko wrote:
>
> > > P.S. Do you see now how your series and the end result become better?
> >
> > The driver improves. Though we kind of get lost in details, I have the
> > impression we could go on like this forever.
>
> I think at one point Jonathan mentioned the phenomenon "perfect is the
> enemy of good", which even has its own Wikipedia article:
> https://en.wikipedia.org/wiki/Perfect_is_the_enemy_of_good
>
> When I feel (for some intuitive definition of "feeling") that a patch
> series to my subsystem is getting over-reviewed, I usually just
> merge it and tell everyone involved to write and/or request
> additional patches if they are troubled by the result. It's a fine line,
> admittedly, it's not like I can define the trigger point.

The problem here is that a new round of review was triggered by a
build bot that can't compile it with a Clang which follows C standard,
but doesn't have sugar to it. Otherwise series was ready to be merged
and can be improved later on.


-- 
With Best Regards,
Andy Shevchenko