Add support for magnetometer Yamaha YAS537

Reviewed-by Andy Shevchenko <[email protected]>

---
 drivers/iio/magnetometer/yamaha-yas530.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/iio/magnetometer/yamaha-yas530.c +1 -1

index 9ff7b0e56cf6..b58a9c51e9ff 100644
@@ -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];

Signature · 1 line

2.35.1

Jakob Hauser

author

2022-06-13 01:17 UTC #3 lore raw

PATCH v2 2/7 iio: magnetometer: yas530: Change range of data in volatile register

In function yas5xx_volatile_reg(), the range for measure data should end at "YAS5XX_MEASURE_DATA + 7" instead of "+ 8" as we count from 0 to 7 here.

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

Cc Linus Walleij <[email protected]>

---
 drivers/iio/magnetometer/yamaha-yas530.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/iio/magnetometer/yamaha-yas530.c +1 -1

index b58a9c51e9ff..2d71ae837bf5 100644
@@ -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 + 7);
 }
 
 /* TODO: enable regmap cache, using mark dirty and sync at runtime resume */

Signature · 1 line

2.35.1

Jakob Hauser

author

2022-06-13 01:17 UTC #4 lore raw

PATCH v2 3/7 iio: magnetometer: yas530: Correct scaling of magnetic axes

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

Cc Linus Walleij <[email protected]>

---
 drivers/iio/magnetometer/yamaha-yas530.c | 28 +++++++++++++++---------
 1 file changed, 18 insertions(+), 10 deletions(-)

drivers/iio/magnetometer/yamaha-yas530.c +18 ~~-10~~

index 2d71ae837bf5..40c81c082782 100644
@@ -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,24 @@ 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. Devide by
+			 * 100000000 (10^8) to get Gauss.
+			 */
+			*val = 1;
+			*val2 = 100000000;
+			break;
+		case YAS532_DEVICE_ID:
+			/*
+			 * Raw values of YAS532 are in nanotesla. Devide by
+			 * 100000 (10^5) to get Gauss.
+			 */
+			*val = 1;
+			*val2 = 100000;
+			break;
+		}
 		return IIO_VAL_FRACTIONAL;
 	default:
 		/* Unknown request */

Signature · 1 line

2.35.1

Jakob Hauser

author

2022-06-13 01:17 UTC #5 lore raw

PATCH v2 4/7 iio: magnetometer: yas530: Correct temperature handling

[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

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 yas5xx_get_measure() function.

Cc Linus Walleij <[email protected]>

---
 drivers/iio/magnetometer/yamaha-yas530.c | 93 ++++++++++++++++++------
 1 file changed, 70 insertions(+), 23 deletions(-)

drivers/iio/magnetometer/yamaha-yas530.c +70 ~~-23~~

index 40c81c082782..9bfb3b573907 100644
@@ -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,43 @@ 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;
+	}
+
+	/* 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 +375,34 @@ 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 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 milli degrees 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;
+	}
+
 	/*
 	 * Calibrate [x,y,z] with some formulas like this:
 	 *

@@ -384,6 +435,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 +462,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:
 			/*

@@ -513,7 +560,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,

Signature · 1 line

2.35.1

Jakob Hauser

author

2022-06-13 01:18 UTC #8 lore raw

PATCH v2 5/7 iio: magnetometer: yas530: Change data type of calibration coefficients

[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

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.

Cc Linus Walleij <[email protected]>

---
 drivers/iio/magnetometer/yamaha-yas530.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

drivers/iio/magnetometer/yamaha-yas530.c +4 -2

index 9bfb3b573907..31637a890b7f 100644
@@ -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;
 };

Signature · 1 line

2.35.1

Jakob Hauser

author

2022-06-13 01:18 UTC #9 lore raw

PATCH v2 6/7 iio: magnetometer: yas530: Rename functions and registers

This is a preparation for adding YAS537 variant.

Rename functions used only by YAS530 & YAS532 from yas5xx to yas530_532. Same for the registers.

Cc Linus Walleij <[email protected]>

---
 drivers/iio/magnetometer/yamaha-yas530.c | 106 ++++++++++++-----------
 1 file changed, 55 insertions(+), 51 deletions(-)

drivers/iio/magnetometer/yamaha-yas530.c +55 ~~-51~~

index 31637a890b7f..72958bc0849b 100644
@@ -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
 
+/* Registers used by YAS530 and YAS532 */
+#define YAS530_532_ACTUATE_INIT_COIL	0x81
+#define YAS530_532_MEASURE		0x82
+#define YAS530_532_CONFIG		0x83
+#define YAS530_532_MEASURE_INTERVAL	0x84
+#define YAS530_532_OFFSET_X		0x85 /* [-31 .. 31] */
+#define YAS530_532_OFFSET_Y1		0x86 /* [-31 .. 31] */
+#define YAS530_532_OFFSET_Y2		0x87 /* [-31 .. 31] */
+#define YAS530_532_TEST1		0x88
+#define YAS530_532_TEST2		0x89
+#define YAS530_532_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,7 +184,7 @@ static u16 yas532_extract_axis(u8 *data)
 }
 
 /**
- * yas5xx_measure() - Make a measure from the hardware
+ * yas530_532_measure() - Make a measure from the hardware
  * @yas5xx: The device state
  * @t: the raw temperature measurement
  * @x: the raw x axis measurement

@@ -190,7 +192,8 @@ static u16 yas532_extract_axis(u8 *data)
  * @y2: the y2 axis measurement
  * @return: 0 on success or error code
  */
-static int yas5xx_measure(struct yas5xx *yas5xx, u16 *t, u16 *x, u16 *y1, u16 *y2)
+static int yas530_532_measure(struct yas5xx *yas5xx, u16 *t,
+			      u16 *x, u16 *y1, u16 *y2)
 {
 	unsigned int busy;
 	u8 data[8];

@@ -198,7 +201,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_532_MEASURE, YAS5XX_MEASURE_START);
 	if (ret < 0)
 		goto out_unlock;

@@ -264,7 +267,7 @@ 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)
+static s32 yas530_532_linearize(struct yas5xx *yas5xx, u16 val, int axis)
 {
 	struct yas5xx_calibration *c = &yas5xx->calibration;
 	static const s32 yas532ac_coef[] = {

@@ -306,7 +309,7 @@ static s32 yas5xx_linearize(struct yas5xx *yas5xx, u16 val, int axis)
 }
 
 /**
- * yas5xx_get_measure() - Measure a sample of all axis and process
+ * yas530_532_get_measure() - Measure a sample of all axis and process
  * @yas5xx: The device state
  * @to: Temperature out
  * @xo: X axis out

@@ -314,7 +317,8 @@ static s32 yas5xx_linearize(struct yas5xx *yas5xx, u16 val, int axis)
  * @zo: Z axis out
  * @return: 0 on success or error code
  */
-static int yas5xx_get_measure(struct yas5xx *yas5xx, s32 *to, s32 *xo, s32 *yo, s32 *zo)
+static int yas530_532_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 +327,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_532_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_532_linearize(yas5xx, x, 0);
+	sy1 = yas530_532_linearize(yas5xx, y1, 1);
+	sy2 = yas530_532_linearize(yas5xx, y2, 2);
 
 	/* Set the temperature reference value (unit: counts) */
 	switch (yas5xx->devid) {

@@ -596,11 +600,11 @@ static const struct regmap_config yas5xx_regmap_config = {
 };
 
 /**
- * yas53x_extract_calibration() - extracts the a2-a9 and k calibration
+ * yas530_532_extract_calibration() - extracts the a2-a9 and k calibration
  * @data: the bitfield to use
  * @c: the calibration to populate
  */
-static void yas53x_extract_calibration(u8 *data, struct yas5xx_calibration *c)
+static void yas530_532_extract_calibration(u8 *data, struct yas5xx_calibration *c)
 {
 	u64 val = get_unaligned_be64(data);

@@ -638,12 +642,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_532_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_532_CAL, data, sizeof(data));
 	if (ret)
 		return ret;
 	dev_dbg(yas5xx->dev, "calibration data: %*ph\n", 14, data);

@@ -655,7 +659,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_532_extract_calibration(&data[3], c);
 
 	/*
 	 * Extract linearization:

@@ -686,11 +690,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_532_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_532_CAL, data, sizeof(data));
 	if (ret)
 		return ret;
 	dev_dbg(yas5xx->dev, "calibration data: %*ph\n", 14, data);

@@ -709,7 +713,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_532_extract_calibration(&data[3], c);
 	/*
 	 * Extract linearization:
 	 * Linearization layout in the 32 bits at byte 10:

@@ -732,7 +736,7 @@ static int yas532_get_calibration_data(struct yas5xx *yas5xx)
 	return 0;
 }
 
-static void yas5xx_dump_calibration(struct yas5xx *yas5xx)
+static void yas530_532_dump_calibration(struct yas5xx *yas5xx)
 {
 	struct yas5xx_calibration *c = &yas5xx->calibration;

@@ -755,20 +759,20 @@ 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)
+static int yas530_532_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_532_OFFSET_X, ox);
 	if (ret)
 		return ret;
-	ret = regmap_write(yas5xx->map, YAS5XX_OFFSET_Y1, oy1);
+	ret = regmap_write(yas5xx->map, YAS530_532_OFFSET_Y1, oy1);
 	if (ret)
 		return ret;
-	return regmap_write(yas5xx->map, YAS5XX_OFFSET_Y2, oy2);
+	return regmap_write(yas5xx->map, YAS530_532_OFFSET_Y2, oy2);
 }
 
-static s8 yas5xx_adjust_offset(s8 old, int bit, u16 center, u16 measure)
+static s8 yas530_532_adjust_offset(s8 old, int bit, u16 center, u16 measure)
 {
 	if (measure > center)
 		return old + BIT(bit);

@@ -777,7 +781,7 @@ static s8 yas5xx_adjust_offset(s8 old, int bit, u16 center, u16 measure)
 	return old;
 }
 
-static int yas5xx_meaure_offsets(struct yas5xx *yas5xx)
+static int yas530_532_measure_offsets(struct yas5xx *yas5xx)
 {
 	int ret;
 	u16 center;

@@ -786,7 +790,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_532_ACTUATE_INIT_COIL, 0);
 	if (ret)
 		return ret;

@@ -820,26 +824,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_532_set_offsets(yas5xx, ox, oy1, oy2);
 		if (ret)
 			return ret;
 
-		ret = yas5xx_measure(yas5xx, &t, &x, &y1, &y2);
+		ret = yas530_532_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_532_adjust_offset(ox, i, center, x);
+		oy1 = yas530_532_adjust_offset(oy1, i, center, y1);
+		oy2 = yas530_532_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_532_set_offsets(yas5xx, ox, oy1, oy2);
 	if (ret)
 		return ret;

@@ -848,27 +852,27 @@ static int yas5xx_meaure_offsets(struct yas5xx *yas5xx)
 	return 0;
 }
 
-static int yas5xx_power_on(struct yas5xx *yas5xx)
+static int yas530_532_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_532_TEST1, 0);
 	if (ret)
 		return ret;
-	ret = regmap_write(yas5xx->map, YAS5XX_TEST2, 0);
+	ret = regmap_write(yas5xx->map, YAS530_532_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_532_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_532_MEASURE_INTERVAL, 0);
 }
 
 static int yas5xx_probe(struct i2c_client *i2c,

@@ -950,11 +954,11 @@ static int yas5xx_probe(struct i2c_client *i2c,
 		goto assert_reset;
 	}
 
-	yas5xx_dump_calibration(yas5xx);
-	ret = yas5xx_power_on(yas5xx);
+	yas530_532_dump_calibration(yas5xx);
+	ret = yas530_532_power_on(yas5xx);
 	if (ret)
 		goto assert_reset;
-	ret = yas5xx_meaure_offsets(yas5xx);
+	ret = yas530_532_measure_offsets(yas5xx);
 	if (ret)
 		goto assert_reset;

Signature · 1 line

2.35.1

Jakob Hauser

author

2022-06-13 01:18 UTC #10 lore raw

PATCH v2 7/7 iio: magnetometer: yas530: Add YAS537 variant

[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

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.

Cc Linus Walleij <[email protected]>

In reply to Andy Shevchenko

---
The diff on function yas5xx_probe() is a bit confusing. Maybe better to
understand when comparing before and after code.
before: https://github.com/torvalds/linux/blob/v5.19-rc2/drivers/iio/magnetometer/yamaha-yas530.c#L873-L902
after: https://github.com/Jakko3/linux/blob/yas537_v2/drivers/iio/magnetometer/yamaha-yas530.c#L1413-L1462

 drivers/iio/magnetometer/Kconfig         |   4 +-
 drivers/iio/magnetometer/yamaha-yas530.c | 569 +++++++++++++++++++++--
 2 files changed, 543 insertions(+), 30 deletions(-)

drivers/iio/magnetometer/Kconfig +2 -2

index 07eb619bcfe8..868128cee835 100644
@@ -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. Right now YAS530, YAS532, YAS533 and
+	  YAS537 are fully supported.
 
 	  This driver can also be compiled as a module.
 	  To compile this driver as a module, choose M here: the module

drivers/iio/magnetometer/yamaha-yas530.c +541 ~~-28~~

723 lines not on this page · raw message

Signature · 1 line

2.35.1

Andy Shevchenko

2022-06-13 18:42 UTC #12 lore raw

Re: [PATCH v2 7/7] iio: magnetometer: yas530: Add YAS537 variant

Quoted 1 line · Andy Shevchenko

On Mon, Jun 13, 2022 at 3:18 AM Jakob Hauser <[email protected]> wrote:

Forgot to add that please try to split even more preparatory patches. For example, you may convert existing code to stubs / switch-cases / etc and in the last patch just add the new case or new function / branch.

...

Quoted 2 lines

drivers/iio/magnetometer/yamaha-yas530.c static int yas530_532_get_measure(struct yas5xx *yas5xx, s32 *to, · line 564
Quoted 2 lines
drivers/iio/magnetometer/yamaha-yas530.c +2 -0
+       /*
+        * Raw temperature value t is number of counts. A product description

the number

Quoted 14 lines

Quoted 1 line

+ * of YAS537 mentions a temperature resulution of 0.05 °C/count.

resolution

Quoted 10 lines

drivers/iio/magnetometer/yamaha-yas530.c +10 -0

+        * A readout of the t value at ca. 20 °C returns approx. 8120 counts.
+        *
+        * 8120 counts x 0.05 °C/count corresponds to a range of 406 °C.
+        * 0 counts would be at theoretical -386 °C.
+        *
+        * The formula used for YAS530/532 needs to be adapted to this
+        * theoretical starting temperature, again calculating with 1/10:s
+        * of degrees Celsius and finally multiplying by 100 to get milli
+        * degrees Celsius.
+        */

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Andy Shevchenko

Reviewed-by on the v2 series

2022-06-13 18:44 UTC #17 lore raw

Quoted 7 lines · 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-6 are cleanups and refactoring.
Patch 7 finally adds the YAS537 variant.

Reviewed-by Andy Shevchenko <[email protected]>

for patches 2-6.
Patch 1 is already reviewed, patch 7 needs a bit more work.

Quoted 30 lines

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.
Jakob Hauser (7):
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: Add YAS537 variant
 drivers/iio/magnetometer/Kconfig         |   4 +-
 drivers/iio/magnetometer/yamaha-yas530.c | 790 +++++++++++++++++++----
 2 files changed, 684 insertions(+), 110 deletions(-)
--
2.35.1

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Andy Shevchenko

2022-06-13 18:44 UTC #11 lore raw

Re: [PATCH v2 7/7] iio: magnetometer: yas530: Add YAS537 variant

Quoted 15 lines · 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.

...

Quoted 2 lines

- * YAS537 MS-3T (2015 Samsung Galaxy S6, Note 5, Xiaomi)
+ * YAS537 MS-3T (2015 Samsung Galaxy S6, Note 5, Galaxy S7)

Not sure what happened to Xiaomi. There is nothing in the commit message about this change.

...

Quoted 1 line

+#define YAS537_MAG_AVERAGE_32_MASK GENMASK(6, 4) /* corresponds to 0x70 */

Useless comment.

...

drivers/iio/magnetometer/yamaha-yas530.c line 123

Quoted 3 lines

drivers/iio/magnetometer/yamaha-yas530.c +3 -0

+#define YAS537_MEASURE_TIME_WORST      1500 /* us */
+#define YAS537_DEFAULT_SENSOR_DELAY    50   /* ms */
+#define YAS537_MAG_RCOIL_TIME          65   /* us */

Instead of the comments, use unit suffixes, i.e. _US, _MS, _US.

...

Quoted 1 line

+ /* Read data */

Not sure how useful is this comment.

...

Quoted 1 line

+ *t = ((data[0] << 8) | data[1]);

Looks like get_unaligned_be16().

drivers/iio/magnetometer/yamaha-yas530.c static int yas530_532_measure(struct yas5xx *yas5xx, u16 *t, · line 346

Quoted 3 lines

drivers/iio/magnetometer/yamaha-yas530.c +3 -0

+       xy1y2[0] = ((FIELD_GET(GENMASK(5, 0), data[2]) << 8) | data[3]);
+       xy1y2[1] = ((data[4] << 8) | data[5]);
+       xy1y2[2] = ((data[6] << 8) | data[7]);

Ditto for all above.

...

drivers/iio/magnetometer/yamaha-yas530.c static int yas530_532_measure(struct yas5xx *yas5xx, u16 *t, · line 357

Quoted 2 lines

drivers/iio/magnetometer/yamaha-yas530.c +2 -0

+                       if (h[i] < -8192)
+                               h[i] = -8192;

-BIT(13) ?

drivers/iio/magnetometer/yamaha-yas530.c static int yas530_532_measure(struct yas5xx *yas5xx, u16 *t, · line 359

Quoted 2 lines

drivers/iio/magnetometer/yamaha-yas530.c +2 -0

+                       if (h[i] > 8191)
+                               h[i] = 8191;

Altogether seems like NIH clamp_val() or clamp_t().

...

Quoted 1 line

+ xy1y2[i] = h[i] + 8192;

BIT(13)

...

drivers/iio/magnetometer/yamaha-yas530.c static int yas530_532_get_measure(struct yas5xx *yas5xx, s32 *to, · line 565

Quoted 3 lines

drivers/iio/magnetometer/yamaha-yas530.c +3 -0

+       /*
+        * Raw temperature value t is number of counts. A product description
+        * of YAS537 mentions a temperature resulution of 0.05 °C/count.

resolution

Quoted 10 lines

drivers/iio/magnetometer/yamaha-yas530.c +10 -0

+        * A readout of the t value at ca. 20 °C returns approx. 8120 counts.
+        *
+        * 8120 counts x 0.05 °C/count corresponds to a range of 406 °C.
+        * 0 counts would be at theoretical -386 °C.
+        *
+        * The formula used for YAS530/532 needs to be adapted to this
+        * theoretical starting temperature, again calculating with 1/10:s
+        * of degrees Celsius and finally multiplying by 100 to get milli
+        * degrees Celsius.
+        */

...

drivers/iio/magnetometer/yamaha-yas530.c static int yas5xx_read_raw(struct iio_dev *indio_dev, · line 652

Quoted 5 lines

/*

drivers/iio/magnetometer/yamaha-yas530.c +2 -2

-                        * Raw values of YAS532 are in nanotesla. Devide by
-                        * 100000 (10^5) to get Gauss.
+                        * Raw values of YAS532 and YAS537 are in nanotesla.
+                        * Devide by 100000 (10^5) to get Gauss.

Divide (chance to fix a type while at it)

Quoted 1 line

*/

...

drivers/iio/magnetometer/yamaha-yas530.c static int yas530_get_calibration_data(struct yas5xx *yas5xx) · line 893

Quoted 8 lines

drivers/iio/magnetometer/yamaha-yas530.c +1 -0

@@ -679,6 +887,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;
 }

Not harmful, but a stray change. Ditto for the rest like this. I would rather split them to a separate patch.

...

Quoted 1 line

+ dev_dbg(yas5xx->dev, "calibration data: %*ph\n", 17, data);

Use less stack by "%17ph".

...

drivers/iio/magnetometer/yamaha-yas530.c static int yas532_get_calibration_data(struct yas5xx *yas5xx) · line 970

Quoted 2 lines

drivers/iio/magnetometer/yamaha-yas530.c +2 -0

+       /* Sanity check, is this all zeroes? */
+       if (memchr_inv(data, 0x00, 16) == NULL) {

if (!memchr_inv(...))

Quoted 3 lines

drivers/iio/magnetometer/yamaha-yas530.c +3 -0

+               if (FIELD_GET(GENMASK(5, 0), data[16]) == 0)
+                       dev_warn(yas5xx->dev, "calibration is blank!\n");
+       }

...

Quoted 1 line

+ for (i = 0; i < 17; i++) {

16 and 17 seems like a magic that is used a lot, perhaps define?

...

drivers/iio/magnetometer/yamaha-yas530.c static int yas532_get_calibration_data(struct yas5xx *yas5xx) · line 1086

Quoted 2 lines

drivers/iio/magnetometer/yamaha-yas530.c +2 -0

+               ret = regmap_write(yas5xx->map, YAS537_MTC + 3,
+                                  ((data[3] & GENMASK(7, 5)) | BIT(4)));

Here...

drivers/iio/magnetometer/yamaha-yas530.c static int yas532_get_calibration_data(struct yas5xx *yas5xx) · line 1090

Quoted 4 lines

drivers/iio/magnetometer/yamaha-yas530.c +4 -0

+               if (ret)
+                       return ret;
+               ret = regmap_write(yas5xx->map, YAS537_HCK,
+                                  (FIELD_GET(GENMASK(7, 4), data[15]) << 1));

...and here and in many other places, please drop outer parentheses, they are not needed.

Quoted 2 lines

(quoted diff) +2 -0

+               if (ret)
+                       return ret;

...

Quoted 1 line

+ usleep_range(YAS537_MAG_RCOIL_TIME, YAS537_MAG_RCOIL_TIME+100);

Please, add a comment explaining why this is needed.

...

drivers/iio/magnetometer/yamaha-yas530.c static int yas5xx_probe(struct i2c_client *i2c, · line 1449

Quoted 4 lines

drivers/iio/magnetometer/yamaha-yas530.c +4 -0

+               dev_info(dev, "detected YAS537 MS-3T");
+               /* As the version naming is unknown, provide it for debug only */
+               dev_dbg(yas5xx->dev, "YAS537 version: %s\n",
+                       yas5xx->version ? "1" : "0");

No need to have two prints, just add a version to the above one and drop the bottom one.

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Andy Shevchenko

2022-06-13 18:45 UTC #6 lore raw

Re: [PATCH v2 4/7] iio: magnetometer: yas530: Correct temperature handling

In reply to Andy Shevchenko

Quoted 9 lines · 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].

Quoted 5 lines

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 yas5xx_get_measure() function.

in the

...

Please add default cases into switches.

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Jakob Hauser

author

2022-06-15 21:18 UTC #7 lore raw

Re: [PATCH v2 4/7] iio: magnetometer: yas530: Correct temperature handling

Hi Andy,

Quoted 2 lines · Andy Shevchenko

Please add default cases into switches.

Oh, I forgot these. I'll add them.

Kind regards,
Jakob

Jakob Hauser

author

2022-06-15 21:43 UTC #14 lore raw

Re: [PATCH v2 7/7] iio: magnetometer: yas530: Add YAS537 variant

Hi Andy,

again on the Cc list, sorry: Looks like the part...

"open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS <[email protected]>, Hans de Goede <[email protected]>, Andy Shevchenko <[email protected]>,"

... as a text string is saved in your e-mail clients' address book as the name of the e-mail address <~postmarketos/[email protected]>.

Quoted 8 lines · Andy Shevchenko

Quoted 3 lines · Jakob Hauser
- * YAS537 MS-3T (2015 Samsung Galaxy S6, Note 5, Xiaomi)
+ * YAS537 MS-3T (2015 Samsung Galaxy S6, Note 5, Galaxy S7)
Not sure what happened to Xiaomi. There is nothing in the commit message about this change.

"Xiaomi" is too generic, specific devices should be listed here. E.g. Xiaomi Redmi 2 seems to have YAS537 but I'm fully sure this applies to all its variants [1]. Samsung Galaxy S7 (and S7 edge) is often quoted in conjunction with YAS537, so I took this.

Further down you suggested to move stray changes into a separate patch. I'll move this one too.

[1] https://en.wikipedia.org/wiki/Redmi_2#Variants

Quoted 3 lines

Quoted 1 line
+#define YAS537_MAG_AVERAGE_32_MASK GENMASK(6, 4) /* corresponds to 0x70 */
Useless comment.

The comment is related to the values in the Yamaha Android driver [2]. It depends on the "average" to be chosen [3], "YAS537_MAG_AVERAGE_32" is used for regular operation [4]. As this isn't easily comprehensible on the first sight, I added that comment.

Within the mainline driver, the comment is not necessarily needed. Will remove it.

[2]
https://github.com/msm8916-mainline/android_kernel_qcom_msm8916/blob/GT-I9195I/drivers/iio/magnetometer/yas_mag_drv-yas537.c#L258
[3]
https://github.com/msm8916-mainline/android_kernel_qcom_msm8916/blob/GT-I9195I/drivers/iio/magnetometer/yas_mag_drv-yas537.c#L63-L68
[4]
https://github.com/msm8916-mainline/android_kernel_qcom_msm8916/blob/GT-I9195I/drivers/iio/magnetometer/yas_mag_drv-yas537.c#L748

Quoted 5 lines

drivers/iio/magnetometer/yamaha-yas530.c line 123
Quoted 3 lines
drivers/iio/magnetometer/yamaha-yas530.c +3 -0
+#define YAS537_MEASURE_TIME_WORST      1500 /* us */
+#define YAS537_DEFAULT_SENSOR_DELAY    50   /* ms */
+#define YAS537_MAG_RCOIL_TIME          65   /* us */
Instead of the comments, use unit suffixes, i.e. _US, _MS, _US.

OK, thanks for the hint.

Quoted 3 lines

Quoted 1 line
+ /* Read data */
Not sure how useful is this comment.

It's part of the "process description": Read data, Arrange data, Assign data. Maybe I've overdone the commenting here, I'll remove those three comments.

Quoted 9 lines

Quoted 1 line
+ *t = ((data[0] << 8) | data[1]);
Looks like get_unaligned_be16().
drivers/iio/magnetometer/yamaha-yas530.c static int yas530_532_measure(struct yas5xx *yas5xx, u16 *t, · line 346
Quoted 3 lines
drivers/iio/magnetometer/yamaha-yas530.c +3 -0
+       xy1y2[0] = ((FIELD_GET(GENMASK(5, 0), data[2]) << 8) | data[3]);
+       xy1y2[1] = ((data[4] << 8) | data[5]);
+       xy1y2[2] = ((data[6] << 8) | data[7]);
Ditto for all above.

Ah, I wasn't aware about using this here. Will apply.

Quoted 9 lines

drivers/iio/magnetometer/yamaha-yas530.c static int yas530_532_measure(struct yas5xx *yas5xx, u16 *t, · line 357
Quoted 2 lines
drivers/iio/magnetometer/yamaha-yas530.c +2 -0
+                       if (h[i] < -8192)
+                               h[i] = -8192;
-BIT(13) ?
drivers/iio/magnetometer/yamaha-yas530.c static int yas530_532_measure(struct yas5xx *yas5xx, u16 *t, · line 359
Quoted 2 lines
drivers/iio/magnetometer/yamaha-yas530.c +2 -0
+                       if (h[i] > 8191)
+                               h[i] = 8191;
Altogether seems like NIH clamp_val() or clamp_t().

I didn't know, thanks. I will change...

if (h[i] < -8192)

        h[i] = -8192;
if (h[i] > 8191)
        h[i] = 8191;

... into:

clamp_val(h[i], -BIT(13), BIT(13) - 1);

Quoted 3 lines

Quoted 1 line
+ xy1y2[i] = h[i] + 8192;
BIT(13)

Hm, if doing it here, it probably should be done for the whole paragraph. In that case...

for (i = 0; i < 3; i++)

        s[i] = xy1y2[i] - 8192;
h[0] = (c->k *   (128 * s[0] + c->a2 * s[1] + c->a3 * s[2])) / 8192;
h[1] = (c->k * (c->a4 * s[0] + c->a5 * s[1] + c->a6 * s[2])) / 8192;
h[2] = (c->k * (c->a7 * s[0] + c->a8 * s[1] + c->a9 * s[2])) / 8192;
for (i = 0; i < 3; i++) {
        if (h[i] < -8192)
                h[i] = -8192;
if (h[i] > 8191)
                h[i] = 8191;
        xy1y2[i] = h[i] + 8192;
}

... would become:

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

Further down in function yas537_get_measure(), there is another part...

*xo = (x - 8192) * 300;
*yo = (y1 - y2) * 1732 / 10;
*zo = (-y1 - y2 + 16384) * 300;

... that could be changed accordingly to:

*xo = (x - BIT(13)) * 300;
*yo = (y1 - y2) * 1732 / 10;
*zo = (-y1 - y2 + BIT(14)) * 300;

Overall for me it's harder to read that way. But maybe I'm just not used to it.

Quoted 11 lines

drivers/iio/magnetometer/yamaha-yas530.c static int yas530_get_calibration_data(struct yas5xx *yas5xx) · line 893
Quoted 8 lines
drivers/iio/magnetometer/yamaha-yas530.c +1 -0
@@ -679,6 +887,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;
 }
 
Not harmful, but a stray change. Ditto for the rest like this. I would rather split them to a separate patch.

Makes sense to split. Though the new patch will likely get a quite generic commit title like "minor cleanups".

Quoted 3 lines

Quoted 1 line
+ dev_dbg(yas5xx->dev, "calibration data: %*ph\n", 17, data);
Use less stack by "%17ph".

OK.

There is a similar line in the current driver, will change this as well in the new stray change patch.

In yas530_get_calibration_data() I see a little mistake, it reads 14 fields, should be 16. I'll include this little correction in the stray change patch too.

Quoted 8 lines

drivers/iio/magnetometer/yamaha-yas530.c static int yas532_get_calibration_data(struct yas5xx *yas5xx) · line 970
Quoted 2 lines
drivers/iio/magnetometer/yamaha-yas530.c +2 -0
+       /* Sanity check, is this all zeroes? */
+       if (memchr_inv(data, 0x00, 16) == NULL) {
if (!memchr_inv(...))
Quoted 3 lines
drivers/iio/magnetometer/yamaha-yas530.c +3 -0
+               if (FIELD_GET(GENMASK(5, 0), data[16]) == 0)
+                       dev_warn(yas5xx->dev, "calibration is blank!\n");
+       }

No problem to change for YAS537.

At YAS530/532, there is a similar line that should be changed accordingly. However, there is a patch by Linus that was already added to "fixes-togreg" branch in iio.git quite a while ago [5]. The patch is not included in torvalds/linux v5.19-rc1 or -rc2 and neither in iio.git testing branch. So I'm unsure what I should base the patchset on if I want to change that line. I will probably choose linux-next, as the patch is included there and in Kconfig also patch "iio: magnetometer:
ak8974: Drop dependency on OF" is included (which on the other hand isn't included in "fixes-togreg" branch in iio.git).

[5]
https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git/commit/?h=fixes-togreg&id=bb52d3691db8cf24cea049235223f3599778f264

Quoted 3 lines

Quoted 1 line
+ for (i = 0; i < 17; i++) {
16 and 17 seems like a magic that is used a lot, perhaps define?

17 is the size of the calibration data array in YAS537. Accordingly we work with array fields 0 to 16.

On YAS530, the calibration data array has a size of 16, therefore working with fields 0 to 15. On YAS532 the size is 14, working with fields 0 to 13.

I wouldn't use "define" as it is kind of defined by declaration "u8 data[17];" in YAS537 for example. We could probably make more use of sizeof(data). E.g. the line ...

dev_dbg(yas5xx->dev, "calibration data: %*ph\n", 17, data);

... could be written as...

dev_dbg(yas5xx->dev, "calibration data: %*ph\n", sizeof(data), data);

... but we wanted to change this to %17ph anyway.

Also we could change the line...

        for (i = 0; i < 17; i++) {

... into...

        for (i = 0; i < sizeof(data); i++) {

... however, I wouldn't do that, as it makes readability worse for the next steps,...

                if (i < 12) {
                        ...
                } else if (i < 15) {
                        ...
                } else {
                        ...
                }

... in the current implementation it's easier to see that the last one is the remaining 15 and 16.

Summing up, I wouldn't change anything on the handling of the numbers 17 or 16.

Quoted 5 lines

drivers/iio/magnetometer/yamaha-yas530.c static int yas532_get_calibration_data(struct yas5xx *yas5xx) · line 1086
Quoted 2 lines
drivers/iio/magnetometer/yamaha-yas530.c +2 -0
+               ret = regmap_write(yas5xx->map, YAS537_MTC + 3,
+                                  ((data[3] & GENMASK(7, 5)) | BIT(4)));
Here...

Quoted 7 lines

drivers/iio/magnetometer/yamaha-yas530.c static int yas532_get_calibration_data(struct yas5xx *yas5xx) · line 1090
Quoted 4 lines
drivers/iio/magnetometer/yamaha-yas530.c +4 -0
+               if (ret)
+                       return ret;
+               ret = regmap_write(yas5xx->map, YAS537_HCK,
+                                  (FIELD_GET(GENMASK(7, 4), data[15]) << 1));
..and here and in many other places, please drop outer parentheses, they are not needed.

It looked too dangerous without – but thanks for the hint, I'll gladly drop them.

Quoted 3 lines

Quoted 1 line
+ usleep_range(YAS537_MAG_RCOIL_TIME, YAS537_MAG_RCOIL_TIME+100);
Please, add a comment explaining why this is needed.

OK, I'll add: "Wait until the coil has ramped up".

Quoted 7 lines

drivers/iio/magnetometer/yamaha-yas530.c static int yas5xx_probe(struct i2c_client *i2c, · line 1449
Quoted 4 lines
drivers/iio/magnetometer/yamaha-yas530.c +4 -0
+               dev_info(dev, "detected YAS537 MS-3T");
+               /* As the version naming is unknown, provide it for debug only */
+               dev_dbg(yas5xx->dev, "YAS537 version: %s\n",
+                       yas5xx->version ? "1" : "0");
No need to have two prints, just add a version to the above one and drop the bottom one.

OK.

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

I also spotted a minor mistake in function yas537_get_calibration_data() in the following part:

        /* Get data into these three blocks val1 to val3 */
        val1 = get_unaligned_be32(&data[0]);
        val2 = get_unaligned_be32(&data[3]);
        val3 = get_unaligned_be32(&data[6]);
        val4 = get_unaligned_be32(&data[9]);

The comment isn't up to date, talking about three blocks instead of four. I'll correct this.

Kind regards,
Jakob

Jakob Hauser

author

2022-06-15 21:51 UTC #13 lore raw

Re: [PATCH v2 7/7] iio: magnetometer: yas530: Add YAS537 variant

In reply to Andy Shevchenko

Hi Andy,

Quoted 5 lines · Andy Shevchenko

Forgot to add that please try to split even more preparatory patches. For example, you may convert existing code to stubs / switch-cases / etc and in the last patch just add the new case or new function / branch.

That's a good idea. I had a closer look. But I can't spot something that could be beneficial.

The new functions for YAS537 are similar in structure like their counterparts of YAS530/YAS532. However, in detail they differ too much, which hinders merging them into each other.

yas537_measure() being different to yas530_532_measure():
- regmap_write() has an additional bit YAS5XX_MEASURE_CONT
- regmap_read_poll_timeout() differs in the read location
- calculating the values t, x, y1 and y2 are different

yas537_get_measure() being different to yas530_532_get_measure():
- no linearization
- no temperature compensation
- different way of calculating x, y, z from x, y1, y2

yas537_get_calibration_data() being different to yas530.../yas532_get_calibration_data():
- one regmap_bulk_read() only
- different way of getting or extracting the calibration data

yas537_power_on()
- different procedure and registers

The function yas537_dump_calibration() shares a notable part with yas530_532_dump_calibration() after the changes applied in patchset v2. Although merging them together would need some "if" or "switch" statements because YAS537 version 0 needs to be excluded from that function and YAS537 version 1 would need to be excluded from the first and last part of that function. I would leave it like it is, it's easier readable.

Another approach could be to outsource some parts, which are used by all variants, into separate functions. But again I don't see much beneficial pars here.

In yas537_measure() and yas530_532_measure() it could be:
- regmap_bulk_read()

In yas537_get_calibration_data() and yas530.../yas532_get_calibration_data() it could be:
- add_device_randomness()

I think that's it. Both are too small for being worth outsourcing into a separate function.

Kind regards,
Jakob

Jakob Hauser

author

2022-06-15 22:00 UTC #15 lore raw

Re: [PATCH v2 7/7] iio: magnetometer: yas530: Add YAS537 variant

Quoted 8 lines · Jakob Hauser

Quoted 6 lines · Andy Shevchenko
Quoted 3 lines · Jakob Hauser
- * YAS537 MS-3T (2015 Samsung Galaxy S6, Note 5, Xiaomi)
+ * YAS537 MS-3T (2015 Samsung Galaxy S6, Note 5, Galaxy S7)

Quoted 7 lines

Quoted 2 lines
Not sure what happened to Xiaomi. There is nothing in the commit message about this change.
"Xiaomi" is too generic, specific devices should be listed here. E.g. Xiaomi Redmi 2 seems to have YAS537 but I'm fully sure this applies to all its variants [1]. Samsung Galaxy S7 (and S7 edge) is often quoted in conjunction with YAS537, so I took this.

I forgot a "not" here:

"[...] but I'm *not* fully sure this applies to all its variants [...]"

Jonathan Cameron

maintainer

2022-06-18 14:06 UTC #16 lore raw

Re: [PATCH v2 7/7] iio: magnetometer: yas530: Add YAS537 variant