[PATCH v3 0/8] Add support for magnetometer Yamaha YAS537

[PATCH v3 0/8] 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-7 are cleanups and refactoring.
Patch 8 finally adds the YAS537 variant.

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.

Jakob Hauser (8):
  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: Apply minor cleanups
  iio: magnetometer: yas530: Add YAS537 variant

 drivers/iio/magnetometer/Kconfig         |   4 +-
 drivers/iio/magnetometer/yamaha-yas530.c | 797 +++++++++++++++++++----
 2 files changed, 688 insertions(+), 113 deletions(-)

-- 
2.35.1

[PATCH v3 1/8] 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")
Cc: Linus Walleij <linus.walleij@linaro.org>
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

Re: [PATCH v3 1/8] iio: magnetometer: yas530: Change data type of hard_offsets to signed

[Jonathan Cameron]

On Sat, 18 Jun 2022 02:13:09 +0200
Jakob Hauser <jahau@rocketmail.com> wrote:

> 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")
> Cc: Linus Walleij <linus.walleij@linaro.org>

Trivial but it's nice to clean out CC if you also have a tag from the person.
All the automation will generally send emails to anyone who has given a tag
so the CC doesn't add anything at this point

I try to drop cases of this when applying patches, but if it's done by
the submitter it makes my life a little easier!

Thanks,

Jonathan


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

Re: [PATCH v3 1/8] iio: magnetometer: yas530: Change data type of hard_offsets to signed

[Jakob Hauser]

Hi Jonathan,

On 18.06.22 16:18, Jonathan Cameron wrote:
>
> On Sat, 18 Jun 2022 02:13:09 +0200
> Jakob Hauser <jahau@rocketmail.com> wrote:
> 
>> 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")
>> Cc: Linus Walleij <linus.walleij@linaro.org>
> 
> Trivial but it's nice to clean out CC if you also have a tag from the person.
> All the automation will generally send emails to anyone who has given a tag
> so the CC doesn't add anything at this point
> 
> I try to drop cases of this when applying patches, but if it's done by
> the submitter it makes my life a little easier!

I didn't know, thanks for explaining. I'll remove the redundant Cc's.

Kind regards,
Jakob

...

[PATCH v3 2/8] iio: magnetometer: yas530: Change range of data in volatile register

[Jakob Hauser]

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 <linus.walleij@linaro.org>
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..2799ae0784fd 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 + 7);
 }
 
 /* TODO: enable regmap cache, using mark dirty and sync at runtime resume */
-- 
2.35.1

Re: [PATCH v3 2/8] iio: magnetometer: yas530: Change range of data in volatile register

[Jonathan Cameron]

On Sat, 18 Jun 2022 02:13:10 +0200
Jakob Hauser <jahau@rocketmail.com> wrote:

> 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 <linus.walleij@linaro.org>

Drop the Cc now you have an RB from Linus.

> 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..2799ae0784fd 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 + 7);
trivial, but I'd prefer it as
reg < RAS5XX_MEASURE_DATA + 8
because we have 8 registers and do bulk reads of sizeof(u8[8]) so it is more natural
to use 8 as the constant.


>  }
>  
>  /* TODO: enable regmap cache, using mark dirty and sync at runtime resume */

Re: [PATCH v3 2/8] iio: magnetometer: yas530: Change range of data in volatile register

[Jakob Hauser]

Hi Jonathan,

On 18.06.22 16:21, Jonathan Cameron wrote:
>
> On Sat, 18 Jun 2022 02:13:10 +0200
> Jakob Hauser <jahau@rocketmail.com> wrote:
...
>>  	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);
> trivial, but I'd prefer it as
> reg < RAS5XX_MEASURE_DATA + 8
> because we have 8 registers and do bulk reads of sizeof(u8[8]) so it is more natural
> to use 8 as the constant.

OK, I'll change that. Need to slightly change the commit message then,
I'll probably add a sentence.

Kind regards,
Jakob

[PATCH v3 3/8] iio: magnetometer: yas530: Correct scaling of magnetic axes

[Jakob Hauser]

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

Cc: Linus Walleij <linus.walleij@linaro.org>
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 | 28 +++++++++++++++---------
 1 file changed, 18 insertions(+), 10 deletions(-)

diff --git a/drivers/iio/magnetometer/yamaha-yas530.c b/drivers/iio/magnetometer/yamaha-yas530.c
index 2799ae0784fd..bd43b2555b73 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,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. 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;
+		}
 		return IIO_VAL_FRACTIONAL;
 	default:
 		/* Unknown request */
-- 
2.35.1

[PATCH v3 4/8] 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

Cc: Linus Walleij <linus.walleij@linaro.org>
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 bd43b2555b73..9b45a550f31e 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 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;
+	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:
 			/*
@@ -513,7 +566,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

Re: [PATCH v3 4/8] iio: magnetometer: yas530: Correct temperature handling

[Jonathan Cameron]

On Sat, 18 Jun 2022 02:13:12 +0200
Jakob Hauser <jahau@rocketmail.com> wrote:

> 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
> 
> Cc: Linus Walleij <linus.walleij@linaro.org>
> 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 bd43b2555b73..9b45a550f31e 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)

Hmm. I'm not a great fun of big hydra functions to handle differences
between devices.  This could easily all be one code flow with some
lookups into chip specific constant data (as btw could a lot of
the other switch statements in the existing driver).


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

One thought to simplify the divergent flow below.

		t_ref2 = 0;
> +		break;
> +	case YAS532_DEVICE_ID:
> +		t_ref = YAS532_20DEGREES;
		if (yas5xx->version == YAS532_VERSION_AC)
			t_ref2 = YAS432_20DEGREES;
		else
			t_ref2 = 0;

Possibly with moving some of the comments below up here.
As mentioned below, this stuff would be even better if
in a chip type specific const structure rather than as code.
That is have one switch statement in probe that picks from
an array of 

struct yas5xx_chip_info {
	/* COMMENTS ON WHAT these are.. *
	u16 tref;
	u16 tref2;
	int ref_temp_celsius;
	int min_temp_celsuis;
};
static const struct yas5xx_chip_info[] = {
	[ENUM value we will use to pick right on in probe] = {
		...

etc


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

Use t_ref2 here and then you could drop the two paths.

> +	} 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).
Roll this info into the maths and only have the constants set in the switch
statement.  Even better if you just move them into chip specific data so
look them up directly rather than via a switch of devid.  The whole driver
would benefit from moving this stuff to const data rather than switch
statements all over the place.

	int min_temp_x10 = yas5xx->chip_info.min_temp_x10;
	const int ref_temp_x10 = 200;

	*to = (min_temp_x10 + ((ref_temp_x10 - min_temp_x10) * t / t_ref)) * 100;

That should make the code self explanatory and remove need for the comments.

> +		 *
> +		 * 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;
> +	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:
>  			/*
> @@ -513,7 +566,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,

Re: [PATCH v3 4/8] iio: magnetometer: yas530: Correct temperature handling

[Jakob Hauser]

Hi Jonathan,

On 18.06.22 16:53, Jonathan Cameron wrote:
>
> On Sat, 18 Jun 2022 02:13:12 +0200
> Jakob Hauser <jahau@rocketmail.com> wrote:
...
>>  /* 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)
> 
> Hmm. I'm not a great fun of big hydra functions to handle differences
> between devices.  This could easily all be one code flow with some
> lookups into chip specific constant data (as btw could a lot of
> the other switch statements in the existing driver).

I'll try to implement the chip_info approach. This should become a
separate patch.

Concerning the patchset, I would prefer to introduce the chip_info
approach rather late. That would mean to leave this patch unchanged and
introduce your suggestions later within the patchset. I think it's
easier to follow the changes along the patchset.

However, you probably would prefer to place the chip_info patch rather
early in the patchset?

>>  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;
> 
> One thought to simplify the divergent flow below.
> 
> 		t_ref2 = 0;
>> +		break;
>> +	case YAS532_DEVICE_ID:
>> +		t_ref = YAS532_20DEGREES;
> 		if (yas5xx->version == YAS532_VERSION_AC)
> 			t_ref2 = YAS432_20DEGREES;
> 		else
> 			t_ref2 = 0;

The t_ref2 approach looks confusing to me. Because for the most version
it's "t_ref2 = 0", only one version out of four needs this.

Another approach: I would rather introduce t_comp (for compensation). In
the chip_info, for the most version it would be...

        .t_comp = t,

... and for the one variant it would be:

        .t_comp = t - t_ref,

A problem: I would include the YAS variants like YAS530, YAS532 etc. in
the chip_info. The versions like "AB" and "AC", on the other hand, I
wouldn't include into the chip_info, instead I would handle these in the
functions. In that case the, "t_comp" thing would need to be done in the
function using an if statement, similar to what you suggested up here.
I'll have a closer look when setting up patchset v4.

> Possibly with moving some of the comments below up here.
> As mentioned below, this stuff would be even better if
> in a chip type specific const structure rather than as code.
> That is have one switch statement in probe that picks from
> an array of 
> 
> struct yas5xx_chip_info {
> 	/* COMMENTS ON WHAT these are.. *
> 	u16 tref;
> 	u16 tref2;
> 	int ref_temp_celsius;
> 	int min_temp_celsuis;
> };
> static const struct yas5xx_chip_info[] = {
> 	[ENUM value we will use to pick right on in probe] = {
> 		...
> 
> etc

Thanks for writing down what it's supposed to look, that's helpful to
compare with other examples.

...

>> @@ -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).
> Roll this info into the maths and only have the constants set in the switch
> statement.  Even better if you just move them into chip specific data so
> look them up directly rather than via a switch of devid.  The whole driver
> would benefit from moving this stuff to const data rather than switch
> statements all over the place.
> 
> 	int min_temp_x10 = yas5xx->chip_info.min_temp_x10;
> 	const int ref_temp_x10 = 200;
> 
> 	*to = (min_temp_x10 + ((ref_temp_x10 - min_temp_x10) * t / t_ref)) * 100;
> 
> That should make the code self explanatory and remove need for the comments.

I'll have a look and will try to implement this.

...

Kind regards,
Jakob

Re: [PATCH v3 4/8] iio: magnetometer: yas530: Correct temperature handling

[Jonathan Cameron]

On Tue, 21 Jun 2022 02:48:46 +0200
Jakob Hauser <jahau@rocketmail.com> wrote:

> Hi Jonathan,
> 
> On 18.06.22 16:53, Jonathan Cameron wrote:
> >
> > On Sat, 18 Jun 2022 02:13:12 +0200
> > Jakob Hauser <jahau@rocketmail.com> wrote:  
> ...
> >>  /* 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)  
> > 
> > Hmm. I'm not a great fun of big hydra functions to handle differences
> > between devices.  This could easily all be one code flow with some
> > lookups into chip specific constant data (as btw could a lot of
> > the other switch statements in the existing driver).  
> 
> I'll try to implement the chip_info approach. This should become a
> separate patch.
> 
> Concerning the patchset, I would prefer to introduce the chip_info
> approach rather late. That would mean to leave this patch unchanged and
> introduce your suggestions later within the patchset. I think it's
> easier to follow the changes along the patchset.
> 
> However, you probably would prefer to place the chip_info patch rather
> early in the patchset?

Whilst I'd prefer it earlier, if it's a real pain, just put a note on
that in the cover letter and I'll cope :)

> 
> >>  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;  
> > 
> > One thought to simplify the divergent flow below.
> > 
> > 		t_ref2 = 0;  
> >> +		break;
> >> +	case YAS532_DEVICE_ID:
> >> +		t_ref = YAS532_20DEGREES;  
> > 		if (yas5xx->version == YAS532_VERSION_AC)
> > 			t_ref2 = YAS432_20DEGREES;
> > 		else
> > 			t_ref2 = 0;  
> 
> The t_ref2 approach looks confusing to me. Because for the most version
> it's "t_ref2 = 0", only one version out of four needs this.
> 
> Another approach: I would rather introduce t_comp (for compensation). In
> the chip_info, for the most version it would be...
> 
>         .t_comp = t,
> 
> ... and for the one variant it would be:
> 
>         .t_comp = t - t_ref,

That looks sensible to me.
> 
> A problem: I would include the YAS variants like YAS530, YAS532 etc. in
> the chip_info. The versions like "AB" and "AC", on the other hand, I
> wouldn't include into the chip_info, instead I would handle these in the
> functions. In that case the, "t_comp" thing would need to be done in the
> function using an if statement, similar to what you suggested up here.

I'd assume there won't be too many different versions that need separate
support and have a chipinfo for each of those versions.  So you
select the chipinfo based on both the device part number and version.

>
Thanks,

J

[PATCH v3 5/8] 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

Cc: Linus Walleij <linus.walleij@linaro.org>
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 9b45a550f31e..c6f5f25793c4 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

Re: [PATCH v3 5/8] iio: magnetometer: yas530: Change data type of calibration coefficients

[Jonathan Cameron]

On Sat, 18 Jun 2022 02:13:13 +0200
Jakob Hauser <jahau@rocketmail.com> wrote:

> 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.
Ok. If this is harmless to existing drivers fair enough, though my personal
inclination would have been to take the easier approach of making the
new variant sign extend on variable load (sign_extend_32() and similar)
just so we didn't need to check the older parts weren't affected.

Thanks,

Jonathan

> 
> [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
> 
> Cc: Linus Walleij <linus.walleij@linaro.org>
> 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 9b45a550f31e..c6f5f25793c4 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;
>  };

Re: [PATCH v3 5/8] iio: magnetometer: yas530: Change data type of calibration coefficients

[Jakob Hauser]

Hi Jonathan,

On 18.06.22 16:56, Jonathan Cameron wrote:
> On Sat, 18 Jun 2022 02:13:13 +0200
> Jakob Hauser <jahau@rocketmail.com> wrote:
> 
>> 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.
> Ok. If this is harmless to existing drivers fair enough, though my personal
> inclination would have been to take the easier approach of making the
> new variant sign extend on variable load (sign_extend_32() and similar)
> just so we didn't need to check the older parts weren't affected.

I didn't know that operation :) Let's take this.

Not sure how to handle the "Reviewed-by:" tags. Even though it's a small
patch, it gets modified a lot. Therefore I'd remove the tags of Linus
and Andy.

Kind regards,
Jakob

Re: [PATCH v3 5/8] iio: magnetometer: yas530: Change data type of calibration coefficients

[Linus Walleij]

On Tue, Jun 21, 2022 at 2:51 AM Jakob Hauser <jahau@rocketmail.com> wrote:
> On 18.06.22 16:56, Jonathan Cameron wrote:
> > On Sat, 18 Jun 2022 02:13:13 +0200
> > Jakob Hauser <jahau@rocketmail.com> wrote:

> > Ok. If this is harmless to existing drivers fair enough, though my personal
> > inclination would have been to take the easier approach of making the
> > new variant sign extend on variable load (sign_extend_32() and similar)
> > just so we didn't need to check the older parts weren't affected.
>
> I didn't know that operation :) Let's take this.
>
> Not sure how to handle the "Reviewed-by:" tags. Even though it's a small
> patch, it gets modified a lot. Therefore I'd remove the tags of Linus
> and Andy.

Keep mine.
I very seldom disagree with Jonathan (or Andy) so the default to any comment
from them is to keep my ACKs/Reviews.

Yours,
Linus Walleij

Re: [PATCH v3 5/8] iio: magnetometer: yas530: Change data type of calibration coefficients

[Jakob Hauser]

Hi Jonathan,

On 21.06.22 02:51, Jakob Hauser wrote:
>
> 
> On 18.06.22 16:56, Jonathan Cameron wrote:
>
>> On Sat, 18 Jun 2022 02:13:13 +0200
>> Jakob Hauser <jahau@rocketmail.com> wrote:
>>
>>> 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.
>>
>> Ok. If this is harmless to existing drivers fair enough, though my personal
>> inclination would have been to take the easier approach of making the
>> new variant sign extend on variable load (sign_extend_32() and similar)
>> just so we didn't need to check the older parts weren't affected.
> 
> I didn't know that operation :) Let's take this.

While working on patchset v4, I just realized that sign_extend32() can't
be used at the variable declaration but instead needs to be applied at
"variable load", as you wrote.

I wasn't aware of this until now. In that case, I'd  prefer to leave the
patch unchanged. Overall the resulting code looks simpler that way.
Applying sign_extend32() at all locations where we extract calibration
coefficients makes it more dizzy.

Kind regards,
Jakob

[PATCH v3 6/8] iio: magnetometer: yas530: Rename functions and registers

[Jakob Hauser]

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 <linus.walleij@linaro.org>
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 | 106 ++++++++++++-----------
 1 file changed, 55 insertions(+), 51 deletions(-)

diff --git a/drivers/iio/magnetometer/yamaha-yas530.c b/drivers/iio/magnetometer/yamaha-yas530.c
index c6f5f25793c4..98c8d365fab7 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
 
+/* 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) {
@@ -602,11 +606,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);
 
@@ -644,12 +648,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);
@@ -661,7 +665,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:
@@ -692,11 +696,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);
@@ -715,7 +719,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:
@@ -738,7 +742,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;
 
@@ -761,20 +765,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);
@@ -783,7 +787,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;
@@ -792,7 +796,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;
 
@@ -826,26 +830,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;
 
@@ -854,27 +858,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,
@@ -956,11 +960,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;
 
-- 
2.35.1

Re: [PATCH v3 6/8] iio: magnetometer: yas530: Rename functions and registers

[Jonathan Cameron]

On Sat, 18 Jun 2022 02:13:14 +0200
Jakob Hauser <jahau@rocketmail.com> wrote:

> This is a preparation for adding YAS537 variant.
> 
> Rename functions used only by YAS530 & YAS532 from yas5xx to yas530_532.

We've been bitten in the past by naming choices like this, so please
use yas530 only and rely on comments or code that makes it obvious that
it applies to the yas532 as well.

> Same for the registers.
> 
> Cc: Linus Walleij <linus.walleij@linaro.org>
> 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 | 106 ++++++++++++-----------
>  1 file changed, 55 insertions(+), 51 deletions(-)
> 
> diff --git a/drivers/iio/magnetometer/yamaha-yas530.c b/drivers/iio/magnetometer/yamaha-yas530.c
> index c6f5f25793c4..98c8d365fab7 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
>  
> +/* Registers used by YAS530 and YAS532 */
> +#define YAS530_532_ACTUATE_INIT_COIL	0x81
As below, don't start listing parts. Just go with the first one
and a comment to say it applies to other devices.  This approach
doesn't age well as a driver gains more supported parts.

#define YAS530_MEASURE is a better choice.

> +#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,

This naming convention often causes us problems long run because
people feel they need to keep listing new devices that look similar.

Generally much better to just pick the first part that uses it and
name the function after that.  So here it would be simply
yas530_measure()

> +			      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) {
> @@ -602,11 +606,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);
>  
> @@ -644,12 +648,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);
> @@ -661,7 +665,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:
> @@ -692,11 +696,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);
> @@ -715,7 +719,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:
> @@ -738,7 +742,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;
>  
> @@ -761,20 +765,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);
> @@ -783,7 +787,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;
> @@ -792,7 +796,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;
>  
> @@ -826,26 +830,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;
>  
> @@ -854,27 +858,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,
> @@ -956,11 +960,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;
>  

Re: [PATCH v3 6/8] iio: magnetometer: yas530: Rename functions and registers

[Jakob Hauser]

Hi Jonathan,

On 18.06.22 17:00, Jonathan Cameron wrote:
>
> On Sat, 18 Jun 2022 02:13:14 +0200
> Jakob Hauser <jahau@rocketmail.com> wrote:
> 
>> This is a preparation for adding YAS537 variant.
>>
>> Rename functions used only by YAS530 & YAS532 from yas5xx to yas530_532.
> 
> We've been bitten in the past by naming choices like this, so please
> use yas530 only and rely on comments or code that makes it obvious that
> it applies to the yas532 as well.

Hm, ok. It's harder to keep the overview. But I can imagine adding up
names can get out of control. I'll change it.

For functions and registers used by all variants, I'd keep yas5xx or
YAS5XX respectively. I hope that's ok.

Again I'm not sure on the "Reviewed-by:" tags. Again the patch gets
modified a lot, therefore I would remove the tags.

...

Kind regards,
Jakob

Re: [PATCH v3 6/8] iio: magnetometer: yas530: Rename functions and registers

[Andy Shevchenko]

On Tue, Jun 21, 2022 at 2:54 AM Jakob Hauser <jahau@rocketmail.com> wrote:
> On 18.06.22 17:00, Jonathan Cameron wrote:

...

> Again I'm not sure on the "Reviewed-by:" tags. Again the patch gets
> modified a lot, therefore I would remove the tags.

Yes, that's right course of actions.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 6/8] iio: magnetometer: yas530: Rename functions and registers

[Jonathan Cameron]

On Tue, 21 Jun 2022 02:53:58 +0200
Jakob Hauser <jahau@rocketmail.com> wrote:

> Hi Jonathan,
> 
> On 18.06.22 17:00, Jonathan Cameron wrote:
> >
> > On Sat, 18 Jun 2022 02:13:14 +0200
> > Jakob Hauser <jahau@rocketmail.com> wrote:
> >   
> >> This is a preparation for adding YAS537 variant.
> >>
> >> Rename functions used only by YAS530 & YAS532 from yas5xx to yas530_532.  
> > 
> > We've been bitten in the past by naming choices like this, so please
> > use yas530 only and rely on comments or code that makes it obvious that
> > it applies to the yas532 as well.  
> 
> Hm, ok. It's harder to keep the overview. But I can imagine adding up
> names can get out of control. I'll change it.
> 
> For functions and registers used by all variants, I'd keep yas5xx or
> YAS5XX respectively. I hope that's ok.

I reserve the right to laugh at you if the next variant to come along
fits the wild card but not the registers that have been shared until then :)

Otherwise, I'm fine with keeping the naming for those cases.

Jonathan

> 
> Again I'm not sure on the "Reviewed-by:" tags. Again the patch gets
> modified a lot, therefore I would remove the tags.
> 
> ...
> 
> Kind regards,
> Jakob

Re: [PATCH v3 6/8] iio: magnetometer: yas530: Rename functions and registers

[Jakob Hauser]

Hi Jonathan,

On 25.06.22 16:16, Jonathan Cameron wrote:
>
> On Tue, 21 Jun 2022 02:53:58 +0200
> Jakob Hauser <jahau@rocketmail.com> wrote:
> 
>> On 18.06.22 17:00, Jonathan Cameron wrote:
>>>
>>> We've been bitten in the past by naming choices like this, so please
>>> use yas530 only and rely on comments or code that makes it obvious that
>>> it applies to the yas532 as well.  
>>
>> Hm, ok. It's harder to keep the overview. But I can imagine adding up
>> names can get out of control. I'll change it.
>>
>> For functions and registers used by all variants, I'd keep yas5xx or
>> YAS5XX respectively. I hope that's ok.
> 
> I reserve the right to laugh at you if the next variant to come along
> fits the wild card but not the registers that have been shared until then :)
> 
> Otherwise, I'm fine with keeping the naming for those cases.

Thanks for your reply. Reserving that right is fine :) It could turn out
that way.

For the time being and as long as possible, keeping the generic ya5xx
naming for those cases makes it more distinguishable what's generally
applicable and what's variant-specific.

Kind regards,
Jakob

[PATCH v3 7/8] iio: magnetometer: yas530: Apply minor cleanups

[Jakob Hauser]

This commit gathers minor cleanups in the current driver.

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.

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.
---
 drivers/iio/magnetometer/yamaha-yas530.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/iio/magnetometer/yamaha-yas530.c b/drivers/iio/magnetometer/yamaha-yas530.c
index 98c8d365fab7..72a75dc57e11 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
@@ -322,7 +322,7 @@ static int yas530_532_get_measure(struct yas5xx *yas5xx, s32 *to,
 {
 	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;
 
@@ -656,9 +656,12 @@ static int yas530_get_calibration_data(struct yas5xx *yas5xx)
 	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);
+	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 */
@@ -685,6 +688,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;
 }
 
@@ -703,15 +707,17 @@ static int yas532_get_calibration_data(struct yas5xx *yas5xx)
 	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);
+	dev_dbg(yas5xx->dev, "calibration data: %14ph\n", data);
 
 	/* Sanity check, is this all zeroes? */
-	if (memchr_inv(data, 0x00, 13) == NULL) {
+	if (!memchr_inv(data, 0x00, 13)) {
 		if (!(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);
 
@@ -720,6 +726,7 @@ static int yas532_get_calibration_data(struct yas5xx *yas5xx)
 	c->Cy1 = data[1] * 10 - 1280;
 	c->Cy2 = data[2] * 10 - 1280;
 	yas530_532_extract_calibration(&data[3], c);
+
 	/*
 	 * Extract linearization:
 	 * Linearization layout in the 32 bits at byte 10:
-- 
2.35.1

Re: [PATCH v3 7/8] iio: magnetometer: yas530: Apply minor cleanups

[Andy Shevchenko]

On Sat, Jun 18, 2022 at 2:15 AM Jakob Hauser <jahau@rocketmail.com> wrote:
>
> This commit gathers minor cleanups in the current driver.
>
> 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.
>
> 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.

We do not accept or even consider contributions w/o SoB tag, sorry.

P.S. Reading briefly, split this to two patches:
1) moving %*ph parameters out from stack;
2) documentation and indentation fixes.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 7/8] iio: magnetometer: yas530: Apply minor cleanups

[Jakob Hauser]

Hi Andy,

On 18.06.22 11:53, Andy Shevchenko wrote:
>
> On Sat, Jun 18, 2022 at 2:15 AM Jakob Hauser <jahau@rocketmail.com> wrote:
>>
>> This commit gathers minor cleanups in the current driver.
>>
>> 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.
>>
>> 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.
> 
> We do not accept or even consider contributions w/o SoB tag, sorry.

I'm not used to the routines yet, sorry...

> P.S. Reading briefly, split this to two patches:
> 1) moving %*ph parameters out from stack;
> 2) documentation and indentation fixes.

There is also the change on memchr_inv(). Additionally on that line, I
would also add the "if (a && b)" that you suggested in patch 8.

So I'd split this patch into two, as recommended by you, but the second
one would be "documentation and style fixes" including the changes on
memchr_inv().

Kind regards,
Jakob

[PATCH v3 8/8] 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

Cc: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Jakob Hauser <jahau@rocketmail.com>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
---
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_v3/drivers/iio/magnetometer/yamaha-yas530.c#L1416-L1463

 drivers/iio/magnetometer/Kconfig         |   4 +-
 drivers/iio/magnetometer/yamaha-yas530.c | 553 +++++++++++++++++++++--
 2 files changed, 529 insertions(+), 28 deletions(-)

diff --git a/drivers/iio/magnetometer/Kconfig b/drivers/iio/magnetometer/Kconfig
index 07eb619bcfe8..868128cee835 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. 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
diff --git a/drivers/iio/magnetometer/yamaha-yas530.c b/drivers/iio/magnetometer/yamaha-yas530.c
index 72a75dc57e11..e6123d1d9383 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>
@@ -56,6 +59,23 @@
 #define YAS530_532_TEST2		0x89
 #define YAS530_532_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 +87,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)
@@ -93,8 +114,15 @@
 #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 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_20DEGREES		8120 /* Counts starting at -386 °C */
+
 #define YAS539_DEVICE_ID		0x08 /* YAS539 (MS-3S) */
 
 /* Turn off device regulators etc after 5 seconds of inactivity */
@@ -267,6 +295,78 @@ static int yas530_532_measure(struct yas5xx *yas5xx, u16 *t,
 	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;
+}
+
 static s32 yas530_532_linearize(struct yas5xx *yas5xx, u16 val, int axis)
 {
 	struct yas5xx_calibration *c = &yas5xx->calibration;
@@ -437,6 +537,58 @@ static int yas530_532_get_measure(struct yas5xx *yas5xx, s32 *to,
 	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_ref, 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;
+
+	/* Set the temperature reference value (unit: counts) */
+	t_ref = YAS537_20DEGREES;
+
+	/*
+	 * Raw temperature value t is the number of counts. YAS537 product
+	 * description No. PBAS537A-000-01-e mentions a temperature resolution
+	 * of 0.05 °C/count. 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.
+	 */
+	*to = ((4060 * t / t_ref) - 3860) * 100;
+
+	/*
+	 * 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,
@@ -450,7 +602,18 @@ 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);
+		switch (yas5xx->devid) {
+		case YAS530_DEVICE_ID:
+		case YAS532_DEVICE_ID:
+			ret = yas530_532_get_measure(yas5xx, &t, &x, &y, &z);
+			break;
+		case YAS537_DEVICE_ID:
+			ret = yas537_get_measure(yas5xx, &t, &x, &y, &z);
+			break;
+		default:
+			dev_err(yas5xx->dev, "unknown device type\n");
+			return -EINVAL;
+		}
 		pm_runtime_mark_last_busy(yas5xx->dev);
 		pm_runtime_put_autosuspend(yas5xx->dev);
 		if (ret)
@@ -484,9 +647,10 @@ static int yas5xx_read_raw(struct iio_dev *indio_dev,
 			*val2 = 100000000;
 			break;
 		case YAS532_DEVICE_ID:
+		case YAS537_DEVICE_ID:
 			/*
-			 * Raw values of YAS532 are in nanotesla. Divide by
-			 * 100000 (10^5) to get Gauss.
+			 * Raw values of YAS532 and YAS537 are in nanotesla.
+			 * Divide by 100000 (10^5) to get Gauss.
 			 */
 			*val = 1;
 			*val2 = 100000;
@@ -506,7 +670,18 @@ 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);
+	switch (yas5xx->devid) {
+	case YAS530_DEVICE_ID:
+	case YAS532_DEVICE_ID:
+		ret = yas530_532_get_measure(yas5xx, &t, &x, &y, &z);
+		break;
+	case YAS537_DEVICE_ID:
+		ret = yas537_get_measure(yas5xx, &t, &x, &y, &z);
+		break;
+	default:
+		dev_err(yas5xx->dev, "unknown device type\n");
+		return;
+	}
 	pm_runtime_mark_last_busy(yas5xx->dev);
 	pm_runtime_put_autosuspend(yas5xx->dev);
 	if (ret) {
@@ -592,9 +767,34 @@ 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 ||
-		(reg >= YAS5XX_MEASURE_DATA && reg <= YAS5XX_MEASURE_DATA + 7);
+	struct iio_dev *indio_dev = dev_get_drvdata(dev);
+	struct yas5xx *yas5xx = iio_priv(indio_dev);
+
+	if (reg >= YAS5XX_MEASURE_DATA && reg <= YAS5XX_MEASURE_DATA + 7)
+		return true;
+
+	/*
+	 * YAS versions share different registers on the same address,
+	 * need to differentiate.
+	 */
+	switch (yas5xx->devid) {
+	case YAS530_DEVICE_ID:
+	case YAS532_DEVICE_ID:
+		if (reg == YAS530_532_ACTUATE_INIT_COIL ||
+		    reg == YAS530_532_MEASURE)
+			return true;
+		break;
+	case YAS537_DEVICE_ID:
+		if (reg == YAS537_MEASURE)
+			return true;
+		break;
+	default:
+		dev_err(yas5xx->dev, "unknown device type\n");
+		break;
+		/* fall through */
+	}
+
+	return false;
 }
 
 /* TODO: enable regmap cache, using mark dirty and sync at runtime resume */
@@ -749,6 +949,216 @@ 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, the exact meaning is unknown */
+	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)) {
+		if (FIELD_GET(GENMASK(5, 0), data[16]) == 0)
+			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 < 17; i++) {
+			if (i < 12) {
+				ret = regmap_write(yas5xx->map,
+						   YAS537_MTC + i,
+						   data[i]);
+				if (ret)
+					return ret;
+			} else if (i < 15) {
+				ret = regmap_write(yas5xx->map,
+						   YAS537_OFFSET_X + i - 12,
+						   data[i]);
+				if (ret)
+					return ret;
+				yas5xx->hard_offsets[i - 12] = data[i];
+			} else {
+				ret = regmap_write(yas5xx->map,
+						   YAS537_HCK + i - 15,
+						   data[i]);
+				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;
+			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,
+				   (data[3] & GENMASK(7, 5)) | BIT(4));
+		if (ret)
+			return ret;
+		ret = regmap_write(yas5xx->map, YAS537_HCK,
+				   FIELD_GET(GENMASK(7, 4), data[15]) << 1);
+		if (ret)
+			return ret;
+		ret = regmap_write(yas5xx->map, YAS537_LCK,
+				   FIELD_GET(GENMASK(3, 0), data[15]) << 1);
+		if (ret)
+			return ret;
+		ret = regmap_write(yas5xx->map, YAS537_OC,
+				   FIELD_GET(GENMASK(5, 0), 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
+		 */
+
+		/* Get data into these four blocks val1 to val4 */
+		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;
+}
+
 static void yas530_532_dump_calibration(struct yas5xx *yas5xx)
 {
 	struct yas5xx_calibration *c = &yas5xx->calibration;
@@ -772,6 +1182,26 @@ static void yas530_532_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);
+	}
+}
+
 static int yas530_532_set_offsets(struct yas5xx *yas5xx, s8 ox, s8 oy1, s8 oy2)
 {
 	int ret;
@@ -888,6 +1318,45 @@ static int yas530_532_power_on(struct yas5xx *yas5xx)
 	return regmap_write(yas5xx->map, YAS530_532_MEASURE_INTERVAL, 0);
 }
 
+static int yas537_power_on(struct yas5xx *yas5xx)
+{
+	int ret;
+	u8 intrvl;
+
+	/* Write registers according to Android driver */
+	ret = regmap_write(yas5xx->map, YAS537_ADCCAL, GENMASK(1, 0));
+	if (ret)
+		return ret;
+	ret = regmap_write(yas5xx->map, YAS537_ADCCAL+1, GENMASK(7, 3));
+	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 * 1000
+		 - 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 int yas5xx_probe(struct i2c_client *i2c,
 			const struct i2c_device_id *id)
 {
@@ -946,35 +1415,53 @@ static int yas5xx_probe(struct i2c_client *i2c,
 
 	switch (yas5xx->devid) {
 	case YAS530_DEVICE_ID:
-		ret = yas530_get_calibration_data(yas5xx);
+	case YAS532_DEVICE_ID:
+
+		if (yas5xx->devid == 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));
+		} else {
+			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));
+		}
+
+		yas530_532_dump_calibration(yas5xx);
+		ret = yas530_532_power_on(yas5xx);
+		if (ret)
+			goto assert_reset;
+		ret = yas530_532_measure_offsets(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);
+
+	case YAS537_DEVICE_ID:
+		ret = yas537_get_calibration_data(yas5xx);
+		if (ret)
+			goto assert_reset;
+		dev_info(dev, "detected YAS537 MS-3T version %s",
+			 yas5xx->version ? "1" : "0");
+		strncpy(yas5xx->name, "yas537", sizeof(yas5xx->name));
+
+		yas537_dump_calibration(yas5xx);
+		ret = yas537_power_on(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);
 		goto assert_reset;
 	}
 
-	yas530_532_dump_calibration(yas5xx);
-	ret = yas530_532_power_on(yas5xx);
-	if (ret)
-		goto assert_reset;
-	ret = yas530_532_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;
@@ -1070,7 +1557,19 @@ 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);
+	switch (yas5xx->devid) {
+	case YAS530_DEVICE_ID:
+	case YAS532_DEVICE_ID:
+		ret = yas530_532_power_on(yas5xx);
+		break;
+	case YAS537_DEVICE_ID:
+		ret = yas537_power_on(yas5xx);
+		break;
+	default:
+		dev_err(yas5xx->dev, "unknown device type\n");
+		ret = -EINVAL;
+		break;
+	}
 	if (ret) {
 		dev_err(dev, "cannot power on\n");
 		goto out_reset;
@@ -1096,6 +1595,7 @@ static const struct i2c_device_id yas5xx_id[] = {
 	{"yas530", },
 	{"yas532", },
 	{"yas533", },
+	{"yas537", },
 	{}
 };
 MODULE_DEVICE_TABLE(i2c, yas5xx_id);
@@ -1104,6 +1604,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 v3 8/8] iio: magnetometer: yas530: Add YAS537 variant

[Andy Shevchenko]

On Sat, Jun 18, 2022 at 2:15 AM Jakob Hauser <jahau@rocketmail.com> wrote:
>
> 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.

...

> -/* These variant IDs are known from code dumps */

Not sure why this change
a) is here (means maybe you need to move the comment somewhere else?);
b) done at all (means maybe this comment is still actual?).

...

>         case IIO_CHAN_INFO_RAW:
>                 pm_runtime_get_sync(yas5xx->dev);
> -               ret = yas5xx_get_measure(yas5xx, &t, &x, &y, &z);
> +               switch (yas5xx->devid) {
> +               case YAS530_DEVICE_ID:
> +               case YAS532_DEVICE_ID:
> +                       ret = yas530_532_get_measure(yas5xx, &t, &x, &y, &z);
> +                       break;
> +               case YAS537_DEVICE_ID:
> +                       ret = yas537_get_measure(yas5xx, &t, &x, &y, &z);
> +                       break;
> +               default:
> +                       dev_err(yas5xx->dev, "unknown device type\n");

> +                       return -EINVAL;

Can't return here, because you leave the PM counters imbalanced.

> +               }
>                 pm_runtime_mark_last_busy(yas5xx->dev);
>                 pm_runtime_put_autosuspend(yas5xx->dev);

...

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

Be consistent (you used milli degress above, I would suggest to use
millidegrees to have it aligned with this comment spelling style).

...

> +       switch (yas5xx->devid) {
> +       case YAS530_DEVICE_ID:
> +       case YAS532_DEVICE_ID:
> +               ret = yas530_532_get_measure(yas5xx, &t, &x, &y, &z);
> +               break;
> +       case YAS537_DEVICE_ID:
> +               ret = yas537_get_measure(yas5xx, &t, &x, &y, &z);
> +               break;
> +       default:
> +               dev_err(yas5xx->dev, "unknown device type\n");

> +               return;

Same issue with PM counters.

> +       }
>         pm_runtime_mark_last_busy(yas5xx->dev);
>         pm_runtime_put_autosuspend(yas5xx->dev);

...

> +       switch (yas5xx->devid) {
> +       case YAS530_DEVICE_ID:
> +       case YAS532_DEVICE_ID:
> +               if (reg == YAS530_532_ACTUATE_INIT_COIL ||
> +                   reg == YAS530_532_MEASURE)
> +                       return true;
> +               break;
> +       case YAS537_DEVICE_ID:
> +               if (reg == YAS537_MEASURE)
> +                       return true;
> +               break;
> +       default:
> +               dev_err(yas5xx->dev, "unknown device type\n");
> +               break;

> +               /* fall through */

What does this mean?

It seems here you may actually to return directly.

> +       }
> +
> +       return false;

...

> +       /* Writing SRST register, the exact meaning is unknown */

Wild guess: Software ReSeT (SRST). Would explain why it should be done
before calibration.

> +       /* Sanity check, is this all zeroes? */
> +       if (!memchr_inv(data, 0x00, 16)) {
> +               if (FIELD_GET(GENMASK(5, 0), data[16]) == 0)
> +                       dev_warn(yas5xx->dev, "calibration is blank!\n");
> +       }

  if (a) {
    if (b) {
      ...
    }
  }

===>

  if (a && b) {
    ...
  }

...

> +       case YAS537_VERSION_0:

> +

Redundant blank line.

> +               /*
> +                * 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
> +                */

> +

Ditto.

> +               for (i = 0; i < 17; i++) {
> +                       if (i < 12) {
> +                               ret = regmap_write(yas5xx->map,
> +                                                  YAS537_MTC + i,
> +                                                  data[i]);
> +                               if (ret)
> +                                       return ret;
> +                       } else if (i < 15) {
> +                               ret = regmap_write(yas5xx->map,
> +                                                  YAS537_OFFSET_X + i - 12,
> +                                                  data[i]);
> +                               if (ret)
> +                                       return ret;
> +                               yas5xx->hard_offsets[i - 12] = data[i];
> +                       } else {
> +                               ret = regmap_write(yas5xx->map,
> +                                                  YAS537_HCK + i - 15,
> +                                                  data[i]);
> +                               if (ret)
> +                                       return ret;
> +                       }
> +               }
> +
> +               break;

> +

Ditto.

...

> +       case YAS537_VERSION_1:

As per above.

...

> +       case YAS532_DEVICE_ID:

> +

Redundant blank line.

> +               if (yas5xx->devid == 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));

strscpy()

> +               } else {
> +                       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));

strscpy()

> +               }

...

> -               strncpy(yas5xx->name, "yas530", sizeof(yas5xx->name));

Okay, strncpy() -> strscpy() perhaps in the separate patch.

> +       case YAS537_DEVICE_ID:
> +               ret = yas537_get_calibration_data(yas5xx);
> +               if (ret)
> +                       goto assert_reset;
> +               dev_info(dev, "detected YAS537 MS-3T version %s",
> +                        yas5xx->version ? "1" : "0");
> +               strncpy(yas5xx->name, "yas537", sizeof(yas5xx->name));

strscpy()

...

>                 break;
> +
>         default:

Unneeded change.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 8/8] iio: magnetometer: yas530: Add YAS537 variant

[Jakob Hauser]

Hi Andy,

On 18.06.22 12:57, Andy Shevchenko wrote:
>
> On Sat, Jun 18, 2022 at 2:15 AM Jakob Hauser <jahau@rocketmail.com> wrote:
>>
>> -/* These variant IDs are known from code dumps */
> 
> Not sure why this change
> a) is here (means maybe you need to move the comment somewhere else?);
> b) done at all (means maybe this comment is still actual?).

The device IDs of YAS537 and YAS539 were placed there as placeholders
for future additions. The comment kind of explained why they are there
without being used within the driver. Now, with YAS537 being added, it
becomes clear that there is only YAS539 left to complete the family, the
comment is not necessary.

...

>> +       switch (yas5xx->devid) {
>> +       case YAS530_DEVICE_ID:
>> +       case YAS532_DEVICE_ID:
>> +               if (reg == YAS530_532_ACTUATE_INIT_COIL ||
>> +                   reg == YAS530_532_MEASURE)
>> +                       return true;
>> +               break;
>> +       case YAS537_DEVICE_ID:
>> +               if (reg == YAS537_MEASURE)
>> +                       return true;
>> +               break;
>> +       default:
>> +               dev_err(yas5xx->dev, "unknown device type\n");
>> +               break;
> 
>> +               /* fall through */
> 
> What does this mean?
> 
> It seems here you may actually to return directly.

I wanted to avoid:

        default:
                dev_err(yas5xx->dev, "unknown device type\n");
                return false;
        }

        return false;

I'll just remove the "fall through" comment and keep it like this:

        default:
                dev_err(yas5xx->dev, "unknown device type\n");
                break;
        }

        return false;

...

>> +       /* Writing SRST register, the exact meaning is unknown */
> 
> Wild guess: Software ReSeT (SRST). Would explain why it should be done
> before calibration.

I'll remove the "the exact meaning is unknown" part.

...

>> +       case YAS537_VERSION_0:
> 
>> +
> 
> Redundant blank line.

Within switch statements no empty lines for visual structuring allowed
at all?

...

>> -               strncpy(yas5xx->name, "yas530", sizeof(yas5xx->name));
> 
> Okay, strncpy() -> strscpy() perhaps in the separate patch.

Yes, this needs to go into a separate patch, it's implemented like this
in the current driver.

...

Kind regards,
Jakob

Re: [PATCH v3 8/8] iio: magnetometer: yas530: Add YAS537 variant

[Jonathan Cameron]

On Sat, 18 Jun 2022 02:13:16 +0200
Jakob Hauser <jahau@rocketmail.com> wrote:

> 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
> 
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Signed-off-by: Jakob Hauser <jahau@rocketmail.com>
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Various comments inline, but mostly following through from earlier feedback
that you would end up with much simpler and more readable code by using
a nice const chip_info structure the address of which you assign in a single
place in _probe()

It's a common pattern and it almost always ends up simpler for drivers that end
up supporting more than one or two different device variants.

Thanks,

Jonathan

> ---
> 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_v3/drivers/iio/magnetometer/yamaha-yas530.c#L1416-L1463
> 
>  drivers/iio/magnetometer/Kconfig         |   4 +-
>  drivers/iio/magnetometer/yamaha-yas530.c | 553 +++++++++++++++++++++--
>  2 files changed, 529 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/iio/magnetometer/Kconfig b/drivers/iio/magnetometer/Kconfig
> index 07eb619bcfe8..868128cee835 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. Right now YAS530, YAS532, YAS533 and
> +	  YAS537 are fully supported.
Whilst here I'd reduce this to

	  3-Axis Magnetometers. YAS530, YAS532, YAS533 and YAS537
	  are supported.

"Right now" provides no information - we are hardly likely to be talking
about code at some stage in the past or future.
"fully" isn't all that well defined and doesn't add anything over 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 72a75dc57e11..e6123d1d9383 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>
> @@ -56,6 +59,23 @@
>  #define YAS530_532_TEST2		0x89
>  #define YAS530_532_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 +87,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)
> @@ -93,8 +114,15 @@
>  #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 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_20DEGREES		8120 /* Counts starting at -386 °C */
> +
>  #define YAS539_DEVICE_ID		0x08 /* YAS539 (MS-3S) */
>  
>  /* Turn off device regulators etc after 5 seconds of inactivity */
> @@ -267,6 +295,78 @@ static int yas530_532_measure(struct yas5xx *yas5xx, u16 *t,
>  	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;
> +}
> +
>  static s32 yas530_532_linearize(struct yas5xx *yas5xx, u16 val, int axis)
>  {
>  	struct yas5xx_calibration *c = &yas5xx->calibration;
> @@ -437,6 +537,58 @@ static int yas530_532_get_measure(struct yas5xx *yas5xx, s32 *to,
>  	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_ref, 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;
> +
> +	/* Set the temperature reference value (unit: counts) */
> +	t_ref = YAS537_20DEGREES;
> +
> +	/*
> +	 * Raw temperature value t is the number of counts. YAS537 product
> +	 * description No. PBAS537A-000-01-e mentions a temperature resolution
> +	 * of 0.05 °C/count. 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.
> +	 */
> +	*to = ((4060 * t / t_ref) - 3860) * 100;
> +
> +	/*
> +	 * 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,
> @@ -450,7 +602,18 @@ 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);
> +		switch (yas5xx->devid) {
> +		case YAS530_DEVICE_ID:
> +		case YAS532_DEVICE_ID:
> +			ret = yas530_532_get_measure(yas5xx, &t, &x, &y, &z);

As below, use function pointers in a chip_info struct to avoid switch
statements like this and give shorter + more readable code.

> +			break;
> +		case YAS537_DEVICE_ID:
> +			ret = yas537_get_measure(yas5xx, &t, &x, &y, &z);
> +			break;
> +		default:
> +			dev_err(yas5xx->dev, "unknown device type\n");
> +			return -EINVAL;
> +		}
>  		pm_runtime_mark_last_busy(yas5xx->dev);
>  		pm_runtime_put_autosuspend(yas5xx->dev);
>  		if (ret)
> @@ -484,9 +647,10 @@ static int yas5xx_read_raw(struct iio_dev *indio_dev,
>  			*val2 = 100000000;
>  			break;
>  		case YAS532_DEVICE_ID:
> +		case YAS537_DEVICE_ID:
>  			/*
> -			 * Raw values of YAS532 are in nanotesla. Divide by
> -			 * 100000 (10^5) to get Gauss.
> +			 * Raw values of YAS532 and YAS537 are in nanotesla.
> +			 * Divide by 100000 (10^5) to get Gauss.
>  			 */
>  			*val = 1;
>  			*val2 = 100000;
> @@ -506,7 +670,18 @@ 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);

yas5xx->chip_info.get_measure()

> +	switch (yas5xx->devid) {
> +	case YAS530_DEVICE_ID:
> +	case YAS532_DEVICE_ID:
> +		ret = yas530_532_get_measure(yas5xx, &t, &x, &y, &z);
> +		break;
> +	case YAS537_DEVICE_ID:
> +		ret = yas537_get_measure(yas5xx, &t, &x, &y, &z);
> +		break;
> +	default:
> +		dev_err(yas5xx->dev, "unknown device type\n");
> +		return;
> +	}
>  	pm_runtime_mark_last_busy(yas5xx->dev);
>  	pm_runtime_put_autosuspend(yas5xx->dev);
>  	if (ret) {
> @@ -592,9 +767,34 @@ 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 ||
> -		(reg >= YAS5XX_MEASURE_DATA && reg <= YAS5XX_MEASURE_DATA + 7);
> +	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> +	struct yas5xx *yas5xx = iio_priv(indio_dev);
> +
> +	if (reg >= YAS5XX_MEASURE_DATA && reg <= YAS5XX_MEASURE_DATA + 7)
> +		return true;
> +
> +	/*
> +	 * YAS versions share different registers on the same address,
> +	 * need to differentiate.
Better as separate functions via the chip_info structure I'm suggesting
you add.  That will end up more readable as additional devices are added
at the cost of a slightly more code in this case.

> +	 */
> +	switch (yas5xx->devid) {
> +	case YAS530_DEVICE_ID:
> +	case YAS532_DEVICE_ID:
> +		if (reg == YAS530_532_ACTUATE_INIT_COIL ||
> +		    reg == YAS530_532_MEASURE)
> +			return true;
> +		break;
> +	case YAS537_DEVICE_ID:
> +		if (reg == YAS537_MEASURE)
> +			return true;
> +		break;
> +	default:
> +		dev_err(yas5xx->dev, "unknown device type\n");
> +		break;
> +		/* fall through */
> +	}
> +
> +	return false;
>  }
>  
>  /* TODO: enable regmap cache, using mark dirty and sync at runtime resume */
> @@ -749,6 +949,216 @@ 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, the exact meaning is unknown */
> +	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)) {
> +		if (FIELD_GET(GENMASK(5, 0), data[16]) == 0)
> +			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:
Seems like we might need more chip_info variants, though perhaps in this case
it is worth handling it as a switch statement as hopefully the number of
way s this is done for a given part number won't grow any further.

> +
> +		/*
> +		 * 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 < 17; i++) {

Reduce indent by doing this as multiple loops.
Though even better if you can use bulk writes.

		int j = 0;
		for (i = 0; i < 12; i++) {
			ret = regmap_write(yas5xx->map, YAS537_MTC + i,
					   data[j++])
			if (ret)
				return ret;
		}

		for (i = 0; i < 4; i++) {
			ret = regmap_write(yas5xx->map, YAS573_OFFSET_X + i,
					   data[j++]);
			if (ret)
				return ret;
		} 	

> +			if (i < 12) {
> +				ret = regmap_write(yas5xx->map,
> +						   YAS537_MTC + i,
> +						   data[i]);
> +				if (ret)
> +					return ret;
> +			} else if (i < 15) {
> +				ret = regmap_write(yas5xx->map,
> +						   YAS537_OFFSET_X + i - 12,
> +						   data[i]);
> +				if (ret)
> +					return ret;
> +				yas5xx->hard_offsets[i - 12] = data[i];
> +			} else {
> +				ret = regmap_write(yas5xx->map,
> +						   YAS537_HCK + i - 15,
> +						   data[i]);
> +				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;
> +			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,
> +				   (data[3] & GENMASK(7, 5)) | BIT(4));

If we can give the masks meaningful names that would be great then
use FIELD_GET and FIELD_PREP with those defines (even if it puts it back
in the same place like here).  Let the compiler optimise out anything
unnecessary in the way of masks and shifts.

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

FIELD_PREP() with suitable mask defined to make it clear what field is being written.

> +		if (ret)
> +			return ret;
> +		ret = regmap_write(yas5xx->map, YAS537_LCK,
> +				   FIELD_GET(GENMASK(3, 0), data[15]) << 1);
> +		if (ret)
> +			return ret;
> +		ret = regmap_write(yas5xx->map, YAS537_OC,
> +				   FIELD_GET(GENMASK(5, 0), 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
> +		 */
> +
> +		/* Get data into these four blocks val1 to val4 */
> +		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;
> +}
> +
>  static void yas530_532_dump_calibration(struct yas5xx *yas5xx)
>  {
>  	struct yas5xx_calibration *c = &yas5xx->calibration;
> @@ -772,6 +1182,26 @@ static void yas530_532_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);
> +	}
> +}
> +
>  static int yas530_532_set_offsets(struct yas5xx *yas5xx, s8 ox, s8 oy1, s8 oy2)
>  {
>  	int ret;
> @@ -888,6 +1318,45 @@ static int yas530_532_power_on(struct yas5xx *yas5xx)
>  	return regmap_write(yas5xx->map, YAS530_532_MEASURE_INTERVAL, 0);
>  }
>  
> +static int yas537_power_on(struct yas5xx *yas5xx)
> +{
> +	int ret;
> +	u8 intrvl;
> +
> +	/* Write registers according to Android driver */
> +	ret = regmap_write(yas5xx->map, YAS537_ADCCAL, GENMASK(1, 0));
> +	if (ret)
> +		return ret;
> +	ret = regmap_write(yas5xx->map, YAS537_ADCCAL+1, GENMASK(7, 3));
Spaces around the + 

> +	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 * 1000
> +		 - 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 int yas5xx_probe(struct i2c_client *i2c,
>  			const struct i2c_device_id *id)
>  {
Hopefully I conveyed the point in earlier review, but I'd expcte
a simple switch statement in here based on id and version
to set yas5xx->chip_info to appropriate static const structure
which includes both data and function pointers to flatten
all the device specific switches into simple assignments or
calls of function pointers.
 
> @@ -946,35 +1415,53 @@ static int yas5xx_probe(struct i2c_client *i2c,
>  

Below becomes something like
	ret = yas5xx->chip_info.get_calib_data(yas5xx);
	if (ret)
		goto assert_reset;

	yas5xx->chip_info.dump_calibration(yas5xx);
	yas5xx->chip_info.power_on(yas5xx)
	if (yas5xx->chip_info.measure_offsets) {
		ret = yas5xx->chip_info.measure_offsets(yas5xx);
		if (ret)
			got asert_reset;
	}
	Which is a lot more readable and less code at the expense
	of static const data (a good trade off in most cases).

>  	switch (yas5xx->devid) {
>  	case YAS530_DEVICE_ID:
> -		ret = yas530_get_calibration_data(yas5xx);
> +	case YAS532_DEVICE_ID:
> +
> +		if (yas5xx->devid == 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));

Indirection as suggested below and this is already part of
yas5xx->chip_info

> +		} else {
> +			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));
> +		}
> +
> +		yas530_532_dump_calibration(yas5xx);
> +		ret = yas530_532_power_on(yas5xx);
> +		if (ret)
> +			goto assert_reset;
> +		ret = yas530_532_measure_offsets(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);
> +
> +	case YAS537_DEVICE_ID:
> +		ret = yas537_get_calibration_data(yas5xx);
> +		if (ret)
> +			goto assert_reset;
> +		dev_info(dev, "detected YAS537 MS-3T version %s",
> +			 yas5xx->version ? "1" : "0");
> +		strncpy(yas5xx->name, "yas537", sizeof(yas5xx->name));
> +
> +		yas537_dump_calibration(yas5xx);
> +		ret = yas537_power_on(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);
>  		goto assert_reset;
>  	}
>  
> -	yas530_532_dump_calibration(yas5xx);
> -	ret = yas530_532_power_on(yas5xx);
> -	if (ret)
> -		goto assert_reset;
> -	ret = yas530_532_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;
> @@ -1070,7 +1557,19 @@ 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);
> +	switch (yas5xx->devid) {
> +	case YAS530_DEVICE_ID:
> +	case YAS532_DEVICE_ID:
> +		ret = yas530_532_power_on(yas5xx);

After you've added that device type specific structure I keep talking about
it can have a function pointer and this switch statement becomes as simple
bit of indirection to a constant data.


	yas5xx->chip_info.power_on(yas5xx); 

> +		break;
> +	case YAS537_DEVICE_ID:
> +		ret = yas537_power_on(yas5xx);
> +		break;
> +	default:
> +		dev_err(yas5xx->dev, "unknown device type\n");
> +		ret = -EINVAL;
> +		break;
> +	}
>  	if (ret) {
>  		dev_err(dev, "cannot power on\n");
>  		goto out_reset;

Re: [PATCH v3 8/8] iio: magnetometer: yas530: Add YAS537 variant

[Andy Shevchenko]

On Sat, Jun 18, 2022 at 5:19 PM Jonathan Cameron <jic23@kernel.org> wrote:
> On Sat, 18 Jun 2022 02:13:16 +0200
> Jakob Hauser <jahau@rocketmail.com> wrote:

...

> > +             for (i = 0; i < 17; i++) {
>
> Reduce indent by doing this as multiple loops.
> Though even better if you can use bulk writes.
>
>                 int j = 0;
>                 for (i = 0; i < 12; i++) {
>                         ret = regmap_write(yas5xx->map, YAS537_MTC + i,
>                                            data[j++])
>                         if (ret)
>                                 return ret;
>                 }
>
>                 for (i = 0; i < 4; i++) {
>                         ret = regmap_write(yas5xx->map, YAS573_OFFSET_X + i,
>                                            data[j++]);
>                         if (ret)
>                                 return ret;
>                 }

I guess you are referring to _noinc variants of regmap bulk operations.

> > +                     if (i < 12) {
> > +                             ret = regmap_write(yas5xx->map,
> > +                                                YAS537_MTC + i,
> > +                                                data[i]);
> > +                             if (ret)
> > +                                     return ret;
> > +                     } else if (i < 15) {
> > +                             ret = regmap_write(yas5xx->map,
> > +                                                YAS537_OFFSET_X + i - 12,
> > +                                                data[i]);
> > +                             if (ret)
> > +                                     return ret;
> > +                             yas5xx->hard_offsets[i - 12] = data[i];
> > +                     } else {
> > +                             ret = regmap_write(yas5xx->map,
> > +                                                YAS537_HCK + i - 15,
> > +                                                data[i]);
> > +                             if (ret)
> > +                                     return ret;
> > +                     }
> > +             }

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 8/8] iio: magnetometer: yas530: Add YAS537 variant

[Jonathan Cameron]

On Sun, 19 Jun 2022 12:32:56 +0200
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Sat, Jun 18, 2022 at 5:19 PM Jonathan Cameron <jic23@kernel.org> wrote:
> > On Sat, 18 Jun 2022 02:13:16 +0200
> > Jakob Hauser <jahau@rocketmail.com> wrote:  
> 
> ...
> 
> > > +             for (i = 0; i < 17; i++) {  
> >
> > Reduce indent by doing this as multiple loops.
> > Though even better if you can use bulk writes.
> >
> >                 int j = 0;
> >                 for (i = 0; i < 12; i++) {
> >                         ret = regmap_write(yas5xx->map, YAS537_MTC + i,
> >                                            data[j++])
> >                         if (ret)
> >                                 return ret;
> >                 }
> >
> >                 for (i = 0; i < 4; i++) {
> >                         ret = regmap_write(yas5xx->map, YAS573_OFFSET_X + i,
> >                                            data[j++]);
> >                         if (ret)
> >                                 return ret;
> >                 }  
> 
> I guess you are referring to _noinc variants of regmap bulk operations.
Hi Andy,

No. These seem to be incrementing the register address, so you don't want the _noinc
variant as far as I can see.

> 
> > > +                     if (i < 12) {
> > > +                             ret = regmap_write(yas5xx->map,
> > > +                                                YAS537_MTC + i,
> > > +                                                data[i]);
> > > +                             if (ret)
> > > +                                     return ret;
> > > +                     } else if (i < 15) {
> > > +                             ret = regmap_write(yas5xx->map,
> > > +                                                YAS537_OFFSET_X + i - 12,
> > > +                                                data[i]);
> > > +                             if (ret)
> > > +                                     return ret;
> > > +                             yas5xx->hard_offsets[i - 12] = data[i];
> > > +                     } else {
> > > +                             ret = regmap_write(yas5xx->map,
> > > +                                                YAS537_HCK + i - 15,
> > > +                                                data[i]);
> > > +                             if (ret)
> > > +                                     return ret;
> > > +                     }
> > > +             }  
> 

Re: [PATCH v3 8/8] iio: magnetometer: yas530: Add YAS537 variant

[Jakob Hauser]

Hi Jonathan,

On 18.06.22 17:28, Jonathan Cameron wrote:
>
> On Sat, 18 Jun 2022 02:13:16 +0200
> Jakob Hauser <jahau@rocketmail.com> wrote:
...
>> diff --git a/drivers/iio/magnetometer/Kconfig b/drivers/iio/magnetometer/Kconfig
>> index 07eb619bcfe8..868128cee835 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. Right now YAS530, YAS532, YAS533 and
>> +	  YAS537 are fully supported.
> Whilst here I'd reduce this to
> 
> 	  3-Axis Magnetometers. YAS530, YAS532, YAS533 and YAS537
> 	  are supported.
> 
> "Right now" provides no information - we are hardly likely to be talking
> about code at some stage in the past or future.
> "fully" isn't all that well defined and doesn't add anything over supported.

OK, will change that.

...

>>  static int yas5xx_read_raw(struct iio_dev *indio_dev,
>>  			   struct iio_chan_spec const *chan,
>>  			   int *val, int *val2,
>> @@ -450,7 +602,18 @@ 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);
>> +		switch (yas5xx->devid) {
>> +		case YAS530_DEVICE_ID:
>> +		case YAS532_DEVICE_ID:
>> +			ret = yas530_532_get_measure(yas5xx, &t, &x, &y, &z);
> 
> As below, use function pointers in a chip_info struct to avoid switch
> statements like this and give shorter + more readable code.

I'll try to implement this.

...

>> +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, the exact meaning is unknown */
>> +	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)) {
>> +		if (FIELD_GET(GENMASK(5, 0), data[16]) == 0)
>> +			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:
> Seems like we might need more chip_info variants, though perhaps in this case
> it is worth handling it as a switch statement as hopefully the number of
> way s this is done for a given part number won't grow any further.

Yes, I think I'll introduce chip_info for the variants like YAS530,
YAS532, etc. but keep handling the versions (like 0 and 1 in this case)
within the functions. I'll have a look again when preparing patchset v4.

>> +
>> +		/*
>> +		 * 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 < 17; i++) {
> 
> Reduce indent by doing this as multiple loops.
> Though even better if you can use bulk writes.
> 
> 		int j = 0;
> 		for (i = 0; i < 12; i++) {
> 			ret = regmap_write(yas5xx->map, YAS537_MTC + i,
> 					   data[j++])
> 			if (ret)
> 				return ret;
> 		}
> 
> 		for (i = 0; i < 4; i++) {
> 			ret = regmap_write(yas5xx->map, YAS573_OFFSET_X + i,
> 					   data[j++]);
> 			if (ret)
> 				return ret;
> 		} 	

I'll change that.

It would also work without adding "int j = 0;" when using data[i + 12]
in the second loop. But I guess you added integer j on purpose, I'll
apply it.

>> +			if (i < 12) {
>> +				ret = regmap_write(yas5xx->map,
>> +						   YAS537_MTC + i,
>> +						   data[i]);
>> +				if (ret)
>> +					return ret;
>> +			} else if (i < 15) {
>> +				ret = regmap_write(yas5xx->map,
>> +						   YAS537_OFFSET_X + i - 12,
>> +						   data[i]);
>> +				if (ret)
>> +					return ret;
>> +				yas5xx->hard_offsets[i - 12] = data[i];
>> +			} else {
>> +				ret = regmap_write(yas5xx->map,
>> +						   YAS537_HCK + i - 15,
>> +						   data[i]);
>> +				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;
>> +			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,
>> +				   (data[3] & GENMASK(7, 5)) | BIT(4));
> 
> If we can give the masks meaningful names that would be great then
> use FIELD_GET and FIELD_PREP with those defines (even if it puts it back
> in the same place like here).  Let the compiler optimise out anything
> unnecessary in the way of masks and shifts.

I don't know what these abbreviations stand for. We could do:

#define YAS537_MTC3_MASK_PREP ...
#define YAS537_MTC3_MASK_GET ...
#define YAS537_HCK_MASK_PREP ...
#define YAS537_HCK_MASK_GET ...
etc.

We need both FIELD_GET and FIELD_PREP, right? The first to retrieve the
data and the second to place it at the right position.

Doesn't that get strange-looking like:

        ret = regmap_write(yas5xx->map, YAS537_HCK,
                           FIELD_PREP(YAS537_HCK_MASK_PREP,
                                      FIELD_GET(YAS537_HCK_MASK_GET,
                                                data[15])));

Or maybe different indentation, would that be acceptable?

        ret = regmap_write(yas5xx->map, YAS537_HCK,
                           FIELD_PREP(YAS537_HCK_MASK_PREP,
                           FIELD_GET(YAS537_HCK_MASK_GET, data[15])));

On the one above your comment, the "YAS537_MTC + 3", we would still need
the "| BIT(4)" to place that "1" there. Or is there some other trick to
do this?

>> +		if (ret)
>> +			return ret;
>> +		ret = regmap_write(yas5xx->map, YAS537_HCK,
>> +				   FIELD_GET(GENMASK(7, 4), data[15]) << 1);
> 
> FIELD_PREP() with suitable mask defined to make it clear what field is being written.
> 
>> +		if (ret)
>> +			return ret;
>> +		ret = regmap_write(yas5xx->map, YAS537_LCK,
>> +				   FIELD_GET(GENMASK(3, 0), data[15]) << 1);
>> +		if (ret)
>> +			return ret;
>> +		ret = regmap_write(yas5xx->map, YAS537_OC,
>> +				   FIELD_GET(GENMASK(5, 0), data[16]));
>> +		if (ret)
>> +			return ret;
>> +

...

>> @@ -946,35 +1415,53 @@ static int yas5xx_probe(struct i2c_client *i2c,
>>  
> 
> Below becomes something like
> 	ret = yas5xx->chip_info.get_calib_data(yas5xx);
> 	if (ret)
> 		goto assert_reset;
> 
> 	yas5xx->chip_info.dump_calibration(yas5xx);
> 	yas5xx->chip_info.power_on(yas5xx)
> 	if (yas5xx->chip_info.measure_offsets) {
> 		ret = yas5xx->chip_info.measure_offsets(yas5xx);
> 		if (ret)
> 			got asert_reset;
> 	}
> 	Which is a lot more readable and less code at the expense
> 	of static const data (a good trade off in most cases).

Thanks for the example, that's helpful.

If YAS537 doesn't have a .measure_offsets function pointer in chip_info,
it skips that if statement cleanly?

...

Kind regards,
Jakob

Re: [PATCH v3 8/8] iio: magnetometer: yas530: Add YAS537 variant

[Jonathan Cameron]

> 
> >> +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, the exact meaning is unknown */
> >> +	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)) {
> >> +		if (FIELD_GET(GENMASK(5, 0), data[16]) == 0)
> >> +			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:  
> > Seems like we might need more chip_info variants, though perhaps in this case
> > it is worth handling it as a switch statement as hopefully the number of
> > way s this is done for a given part number won't grow any further.  
> 
> Yes, I think I'll introduce chip_info for the variants like YAS530,
> YAS532, etc. but keep handling the versions (like 0 and 1 in this case)
> within the functions. I'll have a look again when preparing patchset v4.
> 

Up to you, though I'm guessing it'll end up better with just having a bit
more duplicate data to handle the versions as separate device types.


> >> +
> >> +		/*
> >> +		 * 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,
> >> +				   (data[3] & GENMASK(7, 5)) | BIT(4));  
> > 
> > If we can give the masks meaningful names that would be great then
> > use FIELD_GET and FIELD_PREP with those defines (even if it puts it back
> > in the same place like here).  Let the compiler optimise out anything
> > unnecessary in the way of masks and shifts.  
> 
> I don't know what these abbreviations stand for. We could do:
> 
> #define YAS537_MTC3_MASK_PREP ...
> #define YAS537_MTC3_MASK_GET ...
> #define YAS537_HCK_MASK_PREP ...
> #define YAS537_HCK_MASK_GET ...
> etc.
> 
> We need both FIELD_GET and FIELD_PREP, right? The first to retrieve the
> data and the second to place it at the right position.
> 
> Doesn't that get strange-looking like:
> 

yup. you get this sort of oddity sometimes, but it still keeps
it clear which field you are working with.

>         ret = regmap_write(yas5xx->map, YAS537_HCK,
>                            FIELD_PREP(YAS537_HCK_MASK_PREP,
>                                       FIELD_GET(YAS537_HCK_MASK_GET,
>                                                 data[15])));
> 
> Or maybe different indentation, would that be acceptable?
> 
>         ret = regmap_write(yas5xx->map, YAS537_HCK,
>                            FIELD_PREP(YAS537_HCK_MASK_PREP,
>                            FIELD_GET(YAS537_HCK_MASK_GET, data[15])));
Definitely nicer this way.

> 
> On the one above your comment, the "YAS537_MTC + 3", we would still need
> the "| BIT(4)" to place that "1" there. Or is there some other trick to
> do this?

Give it a define and FIELD_PREP() that one as well so we have some
info on what it is via the code.  Obviously it's longer code, but
generally easier to maintain as puts the register definitions in one place
to check against any docs.


> 
> >> +		if (ret)
> >> +			return ret;
> >> +		ret = regmap_write(yas5xx->map, YAS537_HCK,
> >> +				   FIELD_GET(GENMASK(7, 4), data[15]) << 1);  
> > 
> > FIELD_PREP() with suitable mask defined to make it clear what field is being written.
> >   
> >> +		if (ret)
> >> +			return ret;
> >> +		ret = regmap_write(yas5xx->map, YAS537_LCK,
> >> +				   FIELD_GET(GENMASK(3, 0), data[15]) << 1);
> >> +		if (ret)
> >> +			return ret;
> >> +		ret = regmap_write(yas5xx->map, YAS537_OC,
> >> +				   FIELD_GET(GENMASK(5, 0), data[16]));
> >> +		if (ret)
> >> +			return ret;
> >> +  
> 
>
...

> 
> If YAS537 doesn't have a .measure_offsets function pointer in chip_info,
> it skips that if statement cleanly?

Two options - either add a stub that doesn't do anything or
(I prefer this one as makes it obvious the function might not do anything) check 
if (chip_info->measure_offsets)
	chip_info->measure_offsets()...

> 
> ...
> 
> Kind regards,
> Jakob