* [PATCH v4 00/10] Add support for magnetometer Yamaha YAS537
[not found] <cover.1656883851.git.jahau.ref@rocketmail.com>
@ 2022-07-03 22:02 ` Jakob Hauser
2022-07-03 22:02 ` [PATCH v4 01/10] iio: magnetometer: yas530: Change data type of hard_offsets to signed Jakob Hauser
` (10 more replies)
0 siblings, 11 replies; 39+ messages in thread
From: Jakob Hauser @ 2022-07-03 22:02 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Lars-Peter Clausen, Linus Walleij, Andy Shevchenko,
Hans de Goede, linux-iio, devicetree, phone-devel,
~postmarketos/upstreaming, 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-9 are cleanups and refactoring.
Patch 10 finally adds the YAS537 variant.
Patch 9 adds the "chip_info" approach. As this change is quite extensive, it
might need some more discussion and refinement. Also the patchset as such.
However, applying that patch late in the patchset allows a good traceability
how the other changes were introduced.
By adding patch 9 (chip_info), some of the review comments on the last patchset
became obsolete:
- Patch 8 v3: Comment "Can't return here, because you leave the PM counters
imbalanced."
- Patch 8 v3: Comment on strncpy() vs. strscpy(),
- Patch 8 v3: "fall through" comment at function yas5xx_volatile_reg().
- Patch 8 v3: Empty line between "break" and "default" within switch statement
of function yas5xx_probe().
Changes in v4:
- Rebased to torvalds/linux v5.19-rc4, as this now includes Linus' patch
"Fix memchr_inv() misuse" on driver yamaha-yas530.
- Removed redundant Cc tags.
- Patch 2: Replaced "<= ... + 7" by "< ... + 8" and adapted commit message.
- Patch 3: Added default for switch statement, I forgot to add this.
- Patch 4: In function yas5xx_get_measure(), changed wording "milli degrees"
to "millidegrees".
- Patch 6: Changed the renaming of function from "yas530_532_" to "yas530_".
Same for registers. Added additional comments where appropriate.
- Patch 6: Removed "Reviewed-by:" tag of Andy.
- Split patch 7 v3 into two patches -> patch 7 v4 and patch 8 v4.
- Patch 8: Applied "if (a && b)" suggestion at memchr_inv() by Andy in
function yas532_get_calibration_data().
- Patch 8: Removed defines for device IDs YAS537 and YAS539 and accordingly
the comment "These variant IDs are known from code dumps".
- Patch 9: New patch to introduce the "chip_info" approach.
- Patch 10: In function yas537_get_calibration_data(), removed "the exact
meaning is unknown" from comment "Writing SRST register".
- Patch 10: Also applied "if (a && b)" suggestion at memchr_inv() by Andy
in function yas537_get_calibration_data(). Additionally changed the second
condition from "== 0" to "!".
- Patch 10: In function yas537_get_calibration_data(), removed empty lines
within switch statement. In that context, removed comment "Get data into
these four blocks val1 to val4".
- Patch 10: In Kconfig, simplified wording.
- Patch 10: In function yas537_get_calibration_data() "case YAS537_VERSION_0",
reduced indent of "for" loop by splitting it into multiple loops. I didn't
use integer j, as it was suggested by Jonathan, because only using integer i
is more consistant with the loop in "case YAS537_VERSION_1".
- Accordingly, split the "for" loop in "case YAS537_VERSION_1" into two loops
as well. Technically this isn't neccessary but it improves readability.
- Patch 10: Added new defines of masks for YAS537 and applied these by
FIELD_PREP and FIELD_GET in function yas537_get_calibration_data()
within "case YAS537_VERSION_1".
- Patch 10: In function yas537_power_on(), added spaced at "YAS537_ADCCAL + 1".
Changes in v3:
- In patch 3 fixed 2x typo "Divide".
- In commit message of patch 4 fixed wording "in the yas5xx_get_measure()
function".
- In patch 4 in the comment for the temperature calculation fixed wording
"is the number of counts".
- In patch 4 added defaults to switch statements.
- Splitted stray changes into new patch 7 v3. "Add YAS537 variant" is now
patch 8 v3. I haven't added "Reviewed-by:" tag of Linus to patch 7 v3
because as a separate patch these changes appear in a different context.
- In new patch 7 v3, changed printk format specifiers in the function
yas530_get_calibration_data() to "%16ph" and in the function
yas532_get_calibration_data() to "%14ph". The first one is also a minor
correction in behaviour, as the calibration data array size of YAS530
is 16 (the dev_dbg printed 14 before).
- Rebased to linux-next to include patch bb52d3691db8 "iio: magnetometer:
yas530: Fix memchr_inv() misuse".
- In patch 7 v3, changed memchr_inv() line for YAS532.
- In patch 8 v3 in the function yas537_get_calibration_data(), changed
memchr_inv() line for YAS537.
- Removed comment "corresponds to 0x70" at define YAS537_MAG_AVERAGE_32_MASK.
- Added suffixes _US and _MS in defines for YAS537.
- In the function yas537_measure(), removed comments "Read data", "Arrange
data", "Assign data".
- In the function yas537_measure(), replaced bitwise shift by
get_unaligned_be16().
- Replaced "if (h[i] < -8192)" etc. by clamp_val().
- In the functions yas537_measure() and yas537_get_measure(), replaced 8192
by BIT(13) and 16384 by BIT(14).
- Fixed typo "resolution" in the function yas5xx_read_raw().
- Fixed typo "Divide" in patch 8 v3 in the function yas5xx_read_raw().
- In patch 8 v3 in the yas537_get_calibration_data(), changed printk format
specifier to "%17ph"
- In the functions yas537_measure() and yas537_get_calibration_data(), drop
some parentheses in regmap_write().
- In the function yas537_power_on(), added comment "Wait until the coil has
ramped up".
- In the function yas5xx_probe(), put YAS537 variant and version printings
into one print.
- In the function yas537_get_measure(), fixed wording "is the number of
counts" in the comment for the temperature calculation.
- In the function yas537_get_measure(), added product description document No.
into the comment for the temperature calculation (as I first thought the
review comment "the number" is related to this).
- In the function yas537_get_calibration_data(), corrected comment "Get data
into these four blocks val1 to val4".
Changes in v2:
- Reordered the patchset by moving patch 4 v1 to patch 1 v2.
- Removed patch 6 v1 ("Remove redundant defaults on switch devid")
- Accordingly, added "default:" to each switch statement in patch 7.
- Moved renamings in patch 7 v1 into a separate new patch 6 v2. I added
the "Reviewed-by:" tag of Linus to both patches, hope that's ok, else
feel free to comment.
- Removed regmap reads and related debug dumps in patch 7 in function
yas537_dump_calibration(). As this function now applies to version 1
only, replaced switch statement by if clause.
- Also removed "hard_offsets" debug dumps in that function.
- Fixed typo "initialized" in commit message of patch 7.
Jakob Hauser (10):
iio: magnetometer: yas530: Change data type of hard_offsets to signed
iio: magnetometer: yas530: Change range of data in volatile register
iio: magnetometer: yas530: Correct scaling of magnetic axes
iio: magnetometer: yas530: Correct temperature handling
iio: magnetometer: yas530: Change data type of calibration
coefficients
iio: magnetometer: yas530: Rename functions and registers
iio: magnetometer: yas530: Move printk %*ph parameters out from stack
iio: magnetometer: yas530: Apply documentation and style fixes
iio: magnetometer: yas530: Introduce "chip_info" structure
iio: magnetometer: yas530: Add YAS537 variant
drivers/iio/magnetometer/Kconfig | 4 +-
drivers/iio/magnetometer/yamaha-yas530.c | 808 +++++++++++++++++++----
2 files changed, 680 insertions(+), 132 deletions(-)
--
2.35.1
^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH v4 01/10] iio: magnetometer: yas530: Change data type of hard_offsets to signed
2022-07-03 22:02 ` [PATCH v4 00/10] Add support for magnetometer Yamaha YAS537 Jakob Hauser
@ 2022-07-03 22:02 ` Jakob Hauser
2022-07-03 22:02 ` [PATCH v4 02/10] iio: magnetometer: yas530: Change range of data in volatile register Jakob Hauser
` (9 subsequent siblings)
10 siblings, 0 replies; 39+ messages in thread
From: Jakob Hauser @ 2022-07-03 22:02 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Lars-Peter Clausen, Linus Walleij, Andy Shevchenko,
Hans de Goede, linux-iio, devicetree, phone-devel,
~postmarketos/upstreaming, Jakob Hauser
The "hard_offsets" are currently unsigned u8 but they should be signed as they
can get negative. They are signed in function yas5xx_meaure_offsets() and in the
Yamaha drivers [1][2].
[1] https://github.com/NovaFusion/android_kernel_samsung_golden/blob/cm-12.1/drivers/sensor/compass/yas.h#L156
[2] https://github.com/msm8916-mainline/android_kernel_qcom_msm8916/blob/GT-I9195I/drivers/iio/magnetometer/yas_mag_drv-yas532.c#L91
Fixes: de8860b1ed47 ("iio: magnetometer: Add driver for Yamaha YAS530")
Signed-off-by: Jakob Hauser <jahau@rocketmail.com>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
---
drivers/iio/magnetometer/yamaha-yas530.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/iio/magnetometer/yamaha-yas530.c b/drivers/iio/magnetometer/yamaha-yas530.c
index b2bc637150bf..40192aa46b04 100644
--- a/drivers/iio/magnetometer/yamaha-yas530.c
+++ b/drivers/iio/magnetometer/yamaha-yas530.c
@@ -132,7 +132,7 @@ struct yas5xx {
unsigned int version;
char name[16];
struct yas5xx_calibration calibration;
- u8 hard_offsets[3];
+ s8 hard_offsets[3];
struct iio_mount_matrix orientation;
struct regmap *map;
struct regulator_bulk_data regs[2];
--
2.35.1
^ permalink raw reply related [flat|nested] 39+ messages in thread
* [PATCH v4 02/10] iio: magnetometer: yas530: Change range of data in volatile register
2022-07-03 22:02 ` [PATCH v4 00/10] Add support for magnetometer Yamaha YAS537 Jakob Hauser
2022-07-03 22:02 ` [PATCH v4 01/10] iio: magnetometer: yas530: Change data type of hard_offsets to signed Jakob Hauser
@ 2022-07-03 22:02 ` Jakob Hauser
2022-07-03 22:02 ` [PATCH v4 03/10] iio: magnetometer: yas530: Correct scaling of magnetic axes Jakob Hauser
` (8 subsequent siblings)
10 siblings, 0 replies; 39+ messages in thread
From: Jakob Hauser @ 2022-07-03 22:02 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Lars-Peter Clausen, Linus Walleij, Andy Shevchenko,
Hans de Goede, linux-iio, devicetree, phone-devel,
~postmarketos/upstreaming, Jakob Hauser
In function yas5xx_volatile_reg(), register "YAS5XX_MEASURE_DATA + 8" shouldn't
be volatile as we count from 0 to 7 here.
Instead of lowering the number from 8 to 7, the operator "<=" is replaced by
"<". The size of the measure data array is 8, therefore it's more natural to
use 8 as a constant.
This change is of low importance as the "+ 8" register isn't called.
Signed-off-by: Jakob Hauser <jahau@rocketmail.com>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
---
drivers/iio/magnetometer/yamaha-yas530.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/iio/magnetometer/yamaha-yas530.c b/drivers/iio/magnetometer/yamaha-yas530.c
index 40192aa46b04..6fa29b96c013 100644
--- a/drivers/iio/magnetometer/yamaha-yas530.c
+++ b/drivers/iio/magnetometer/yamaha-yas530.c
@@ -527,7 +527,7 @@ static bool yas5xx_volatile_reg(struct device *dev, unsigned int reg)
{
return reg == YAS5XX_ACTUATE_INIT_COIL ||
reg == YAS5XX_MEASURE ||
- (reg >= YAS5XX_MEASURE_DATA && reg <= YAS5XX_MEASURE_DATA + 8);
+ (reg >= YAS5XX_MEASURE_DATA && reg < YAS5XX_MEASURE_DATA + 8);
}
/* TODO: enable regmap cache, using mark dirty and sync at runtime resume */
--
2.35.1
^ permalink raw reply related [flat|nested] 39+ messages in thread
* [PATCH v4 03/10] iio: magnetometer: yas530: Correct scaling of magnetic axes
2022-07-03 22:02 ` [PATCH v4 00/10] Add support for magnetometer Yamaha YAS537 Jakob Hauser
2022-07-03 22:02 ` [PATCH v4 01/10] iio: magnetometer: yas530: Change data type of hard_offsets to signed Jakob Hauser
2022-07-03 22:02 ` [PATCH v4 02/10] iio: magnetometer: yas530: Change range of data in volatile register Jakob Hauser
@ 2022-07-03 22:02 ` Jakob Hauser
2022-07-03 22:02 ` [PATCH v4 04/10] iio: magnetometer: yas530: Correct temperature handling Jakob Hauser
` (7 subsequent siblings)
10 siblings, 0 replies; 39+ messages in thread
From: Jakob Hauser @ 2022-07-03 22:02 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Lars-Peter Clausen, Linus Walleij, Andy Shevchenko,
Hans de Goede, linux-iio, devicetree, phone-devel,
~postmarketos/upstreaming, Jakob Hauser
Looks like YAS530 raw values return picotesla and YAS532 nanotesla. Adapt
comments and scaling.
Signed-off-by: Jakob Hauser <jahau@rocketmail.com>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
---
drivers/iio/magnetometer/yamaha-yas530.c | 31 ++++++++++++++++--------
1 file changed, 21 insertions(+), 10 deletions(-)
diff --git a/drivers/iio/magnetometer/yamaha-yas530.c b/drivers/iio/magnetometer/yamaha-yas530.c
index 6fa29b96c013..8cad724fb328 100644
--- a/drivers/iio/magnetometer/yamaha-yas530.c
+++ b/drivers/iio/magnetometer/yamaha-yas530.c
@@ -310,8 +310,6 @@ static s32 yas5xx_linearize(struct yas5xx *yas5xx, u16 val, int axis)
* @yo: Y axis out
* @zo: Z axis out
* @return: 0 on success or error code
- *
- * Returned values are in nanotesla according to some code.
*/
static int yas5xx_get_measure(struct yas5xx *yas5xx, s32 *to, s32 *xo, s32 *yo, s32 *zo)
{
@@ -417,14 +415,27 @@ static int yas5xx_read_raw(struct iio_dev *indio_dev,
*val = 1;
return IIO_VAL_INT;
}
- /*
- * The axis values are in nanotesla according to the vendor
- * drivers, but is clearly in microtesla according to
- * experiments. Since 1 uT = 0.01 Gauss, we need to divide
- * by 100000000 (10^8) to get to Gauss from the raw value.
- */
- *val = 1;
- *val2 = 100000000;
+ switch (yas5xx->devid) {
+ case YAS530_DEVICE_ID:
+ /*
+ * Raw values of YAS530 are in picotesla. Divide by
+ * 100000000 (10^8) to get Gauss.
+ */
+ *val = 1;
+ *val2 = 100000000;
+ break;
+ case YAS532_DEVICE_ID:
+ /*
+ * Raw values of YAS532 are in nanotesla. Divide by
+ * 100000 (10^5) to get Gauss.
+ */
+ *val = 1;
+ *val2 = 100000;
+ break;
+ default:
+ dev_err(yas5xx->dev, "unknown device type\n");
+ return -EINVAL;
+ }
return IIO_VAL_FRACTIONAL;
default:
/* Unknown request */
--
2.35.1
^ permalink raw reply related [flat|nested] 39+ messages in thread
* [PATCH v4 04/10] iio: magnetometer: yas530: Correct temperature handling
2022-07-03 22:02 ` [PATCH v4 00/10] Add support for magnetometer Yamaha YAS537 Jakob Hauser
` (2 preceding siblings ...)
2022-07-03 22:02 ` [PATCH v4 03/10] iio: magnetometer: yas530: Correct scaling of magnetic axes Jakob Hauser
@ 2022-07-03 22:02 ` Jakob Hauser
2022-07-03 22:02 ` [PATCH v4 05/10] iio: magnetometer: yas530: Change data type of calibration coefficients Jakob Hauser
` (6 subsequent siblings)
10 siblings, 0 replies; 39+ messages in thread
From: Jakob Hauser @ 2022-07-03 22:02 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Lars-Peter Clausen, Linus Walleij, Andy Shevchenko,
Hans de Goede, linux-iio, devicetree, phone-devel,
~postmarketos/upstreaming, Jakob Hauser
The raw temperature value is a number of counts from a certain starting
point. The resolution of the temperature counts is different for the YAS
variants.
Temperature compensation for YAS532 version AC seems to be handled differently.
It uses the deviation from 20 degree Celsius [1] whereas YAS530 and older
versions of YAS532 apply solely the t value as a multiplier [2][3].
In funtion yas5xx_read_raw(), add case IIO_CHAN_INFO_PROCESSED. Remove scale
of temperature as this isn't applied.
Additionally correct sign of temperature channel in iio_chan_spec. It's already
defined that way in the yas5xx_get_measure() function.
[1] https://github.com/msm8916-mainline/android_kernel_qcom_msm8916/blob/GT-I9195I/drivers/iio/magnetometer/yas_mag_drv-yas532.c#L442
[2] https://github.com/NovaFusion/android_kernel_samsung_golden/blob/cm-12.1/drivers/sensor/compass/yas_mag_driver-yas530.c#L881-L883
[3] https://github.com/LineageOS/android_kernel_samsung_msm8930-common/blob/lineage-18.1/drivers/sensors/geomagnetic/yas_mag_driver-yas53x.c#L856-L858
Signed-off-by: Jakob Hauser <jahau@rocketmail.com>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
---
drivers/iio/magnetometer/yamaha-yas530.c | 99 ++++++++++++++++++------
1 file changed, 76 insertions(+), 23 deletions(-)
diff --git a/drivers/iio/magnetometer/yamaha-yas530.c b/drivers/iio/magnetometer/yamaha-yas530.c
index 8cad724fb328..c6889a30a1b5 100644
--- a/drivers/iio/magnetometer/yamaha-yas530.c
+++ b/drivers/iio/magnetometer/yamaha-yas530.c
@@ -77,6 +77,7 @@
#define YAS530_DATA_BITS 12
#define YAS530_DATA_CENTER BIT(YAS530_DATA_BITS - 1)
#define YAS530_DATA_OVERFLOW (BIT(YAS530_DATA_BITS) - 1)
+#define YAS530_20DEGREES 182 /* Counts starting at -62 °C */
#define YAS532_DEVICE_ID 0x02 /* YAS532/YAS533 (MS-3R/F) */
#define YAS532_VERSION_AB 0 /* YAS532/533 AB (MS-3R/F AB) */
@@ -88,7 +89,7 @@
#define YAS532_DATA_BITS 13
#define YAS532_DATA_CENTER BIT(YAS532_DATA_BITS - 1)
#define YAS532_DATA_OVERFLOW (BIT(YAS532_DATA_BITS) - 1)
-#define YAS532_20DEGREES 390 /* Looks like Kelvin */
+#define YAS532_20DEGREES 390 /* Counts starting at -50 °C */
/* These variant IDs are known from code dumps */
#define YAS537_DEVICE_ID 0x07 /* YAS537 (MS-3T) */
@@ -314,7 +315,7 @@ static s32 yas5xx_linearize(struct yas5xx *yas5xx, u16 val, int axis)
static int yas5xx_get_measure(struct yas5xx *yas5xx, s32 *to, s32 *xo, s32 *yo, s32 *zo)
{
struct yas5xx_calibration *c = &yas5xx->calibration;
- u16 t, x, y1, y2;
+ u16 t_ref, t, x, y1, y2;
/* These are "signed x, signed y1 etc */
s32 sx, sy1, sy2, sy, sz;
int ret;
@@ -329,16 +330,46 @@ static int yas5xx_get_measure(struct yas5xx *yas5xx, s32 *to, s32 *xo, s32 *yo,
sy1 = yas5xx_linearize(yas5xx, y1, 1);
sy2 = yas5xx_linearize(yas5xx, y2, 2);
- /*
- * Temperature compensation for x, y1, y2 respectively:
- *
- * Cx * t
- * x' = x - ------
- * 100
- */
- sx = sx - (c->Cx * t) / 100;
- sy1 = sy1 - (c->Cy1 * t) / 100;
- sy2 = sy2 - (c->Cy2 * t) / 100;
+ /* Set the temperature reference value (unit: counts) */
+ switch (yas5xx->devid) {
+ case YAS530_DEVICE_ID:
+ t_ref = YAS530_20DEGREES;
+ break;
+ case YAS532_DEVICE_ID:
+ t_ref = YAS532_20DEGREES;
+ break;
+ default:
+ dev_err(yas5xx->dev, "unknown device type\n");
+ return -EINVAL;
+ }
+
+ /* Temperature compensation for x, y1, y2 respectively */
+ if (yas5xx->devid == YAS532_DEVICE_ID &&
+ yas5xx->version == YAS532_VERSION_AC) {
+ /*
+ * YAS532 version AC uses the temperature deviation as a
+ * multiplier.
+ *
+ * Cx * (t - t_ref)
+ * x' = x - ----------------
+ * 100
+ */
+ sx = sx - (c->Cx * (t - t_ref)) / 100;
+ sy1 = sy1 - (c->Cy1 * (t - t_ref)) / 100;
+ sy2 = sy2 - (c->Cy2 * (t - t_ref)) / 100;
+ } else {
+ /*
+ * YAS530 and YAS532 version AB use solely the t value as a
+ * multiplier.
+ *
+ * Cx * t
+ * x' = x - ------
+ * 100
+ */
+ sx = sx - (c->Cx * t) / 100;
+ sy1 = sy1 - (c->Cy1 * t) / 100;
+ sy2 = sy2 - (c->Cy2 * t) / 100;
+ }
/*
* Break y1 and y2 into y and z, y1 and y2 are apparently encoding
@@ -347,11 +378,37 @@ static int yas5xx_get_measure(struct yas5xx *yas5xx, s32 *to, s32 *xo, s32 *yo,
sy = sy1 - sy2;
sz = -sy1 - sy2;
- /*
- * FIXME: convert to Celsius? Just guessing this is given
- * as 1/10:s of degrees so multiply by 100 to get millicentigrades.
- */
- *to = t * 100;
+ /* Process temperature readout */
+ switch (yas5xx->devid) {
+ case YAS530_DEVICE_ID:
+ /*
+ * Raw temperature value t is the number of counts starting
+ * at -62 °C. Reference value t_ref is the number of counts
+ * between -62 °C and 20 °C (82 °C range).
+ *
+ * Temperature in °C would be (82 / t_ref * t) - 62.
+ *
+ * Contrary to this, perform multiplication first and division
+ * second due to calculating with integers.
+ *
+ * To get a nicer result, calculate with 1/10:s degrees Celsius
+ * and finally multiply by 100 to return millidegrees Celsius.
+ */
+ *to = ((820 * t / t_ref) - 620) * 100;
+ break;
+ case YAS532_DEVICE_ID:
+ /*
+ * Actually same procedure for YAS532 but the starting point is
+ * at -50 °C. Reference value t_ref is the number of counts
+ * between -50 °C and 20 °C (70 °C range).
+ */
+ *to = ((700 * t / t_ref) - 500) * 100;
+ break;
+ default:
+ dev_err(yas5xx->dev, "unknown device type\n");
+ return -EINVAL;
+ }
+
/*
* Calibrate [x,y,z] with some formulas like this:
*
@@ -384,6 +441,7 @@ static int yas5xx_read_raw(struct iio_dev *indio_dev,
int ret;
switch (mask) {
+ case IIO_CHAN_INFO_PROCESSED:
case IIO_CHAN_INFO_RAW:
pm_runtime_get_sync(yas5xx->dev);
ret = yas5xx_get_measure(yas5xx, &t, &x, &y, &z);
@@ -410,11 +468,6 @@ static int yas5xx_read_raw(struct iio_dev *indio_dev,
}
return IIO_VAL_INT;
case IIO_CHAN_INFO_SCALE:
- if (chan->address == 0) {
- /* Temperature is unscaled */
- *val = 1;
- return IIO_VAL_INT;
- }
switch (yas5xx->devid) {
case YAS530_DEVICE_ID:
/*
@@ -516,7 +569,7 @@ static const struct iio_chan_spec yas5xx_channels[] = {
.address = 0,
.scan_index = 0,
.scan_type = {
- .sign = 'u',
+ .sign = 's',
.realbits = 32,
.storagebits = 32,
.endianness = IIO_CPU,
--
2.35.1
^ permalink raw reply related [flat|nested] 39+ messages in thread
* [PATCH v4 05/10] iio: magnetometer: yas530: Change data type of calibration coefficients
2022-07-03 22:02 ` [PATCH v4 00/10] Add support for magnetometer Yamaha YAS537 Jakob Hauser
` (3 preceding siblings ...)
2022-07-03 22:02 ` [PATCH v4 04/10] iio: magnetometer: yas530: Correct temperature handling Jakob Hauser
@ 2022-07-03 22:02 ` Jakob Hauser
2022-07-03 22:02 ` [PATCH v4 06/10] iio: magnetometer: yas530: Rename functions and registers Jakob Hauser
` (5 subsequent siblings)
10 siblings, 0 replies; 39+ messages in thread
From: Jakob Hauser @ 2022-07-03 22:02 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Lars-Peter Clausen, Linus Walleij, Andy Shevchenko,
Hans de Goede, linux-iio, devicetree, phone-devel,
~postmarketos/upstreaming, Jakob Hauser
This is a preparation for adding YAS537 variant.
YAS537 uses other data types on the calibration coefficients [1] than YAS530 [2]
and YAS532 [3].
On YAS537, at least for a4 and a7 this could matter because 8-bit unsigned data
from the register gets stored into a signed data type, therefore this should be
8-bit as well.
For YAS530/532, on the other hand, it doesn't seem to matter. The size of a2-a9
and k is smaller than 8-bit at extraction, also the applied math is low. And
Cx/Cy1/Cy2, now being defined as signed 16-bit, are extracted as unsigned 8-bit
and undergo only minor math.
[1] https://github.com/msm8916-mainline/android_kernel_qcom_msm8916/blob/GT-I9195I/drivers/iio/magnetometer/yas_mag_drv-yas537.c#L76-L78
[2] https://github.com/NovaFusion/android_kernel_samsung_golden/blob/cm-12.1/drivers/sensor/compass/yas_mag_driver-yas530.c#L526-L527
[3] https://github.com/msm8916-mainline/android_kernel_qcom_msm8916/blob/GT-I9195I/drivers/iio/magnetometer/yas_mag_drv-yas532.c#L76-L77
Signed-off-by: Jakob Hauser <jahau@rocketmail.com>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
---
drivers/iio/magnetometer/yamaha-yas530.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/iio/magnetometer/yamaha-yas530.c b/drivers/iio/magnetometer/yamaha-yas530.c
index c6889a30a1b5..839baeca673a 100644
--- a/drivers/iio/magnetometer/yamaha-yas530.c
+++ b/drivers/iio/magnetometer/yamaha-yas530.c
@@ -103,9 +103,11 @@ struct yas5xx_calibration {
s32 r[3];
u32 f[3];
/* Temperature compensation calibration */
- s32 Cx, Cy1, Cy2;
+ s16 Cx, Cy1, Cy2;
/* Misc calibration coefficients */
- s32 a2, a3, a4, a5, a6, a7, a8, a9, k;
+ s8 a2, a3, a4, a6, a7, a8;
+ s16 a5, a9;
+ u8 k;
/* clock divider */
u8 dck;
};
--
2.35.1
^ permalink raw reply related [flat|nested] 39+ messages in thread
* [PATCH v4 06/10] iio: magnetometer: yas530: Rename functions and registers
2022-07-03 22:02 ` [PATCH v4 00/10] Add support for magnetometer Yamaha YAS537 Jakob Hauser
` (4 preceding siblings ...)
2022-07-03 22:02 ` [PATCH v4 05/10] iio: magnetometer: yas530: Change data type of calibration coefficients Jakob Hauser
@ 2022-07-03 22:02 ` Jakob Hauser
2022-07-04 18:04 ` Andy Shevchenko
2022-07-03 22:02 ` [PATCH v4 07/10] iio: magnetometer: yas530: Move printk %*ph parameters out from stack Jakob Hauser
` (4 subsequent siblings)
10 siblings, 1 reply; 39+ messages in thread
From: Jakob Hauser @ 2022-07-03 22:02 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Lars-Peter Clausen, Linus Walleij, Andy Shevchenko,
Hans de Goede, linux-iio, devicetree, phone-devel,
~postmarketos/upstreaming, Jakob Hauser
This is a preparation for adding YAS537 variant.
Functions that are used only by YAS530, YAS532 and YAS533 are renamed from
yas5xx to yas530. Same for the registers.
To avoid part listing in function and registers names, the name of the first
variant is used. Where appropriate, comments were added that these functions
are used by more than one variant.
Functions that will be used by all variants including YAS537 remain in the
naming scheme yas5xx. Or YAS5XX for registers, respectively.
Signed-off-by: Jakob Hauser <jahau@rocketmail.com>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
---
drivers/iio/magnetometer/yamaha-yas530.c | 113 +++++++++++++----------
1 file changed, 62 insertions(+), 51 deletions(-)
diff --git a/drivers/iio/magnetometer/yamaha-yas530.c b/drivers/iio/magnetometer/yamaha-yas530.c
index 839baeca673a..c250909df8f4 100644
--- a/drivers/iio/magnetometer/yamaha-yas530.c
+++ b/drivers/iio/magnetometer/yamaha-yas530.c
@@ -40,20 +40,22 @@
#include <asm/unaligned.h>
-/* This register map covers YAS530 and YAS532 but differs in YAS 537 and YAS539 */
+/* Commonly used registers */
#define YAS5XX_DEVICE_ID 0x80
-#define YAS5XX_ACTUATE_INIT_COIL 0x81
-#define YAS5XX_MEASURE 0x82
-#define YAS5XX_CONFIG 0x83
-#define YAS5XX_MEASURE_INTERVAL 0x84
-#define YAS5XX_OFFSET_X 0x85 /* [-31 .. 31] */
-#define YAS5XX_OFFSET_Y1 0x86 /* [-31 .. 31] */
-#define YAS5XX_OFFSET_Y2 0x87 /* [-31 .. 31] */
-#define YAS5XX_TEST1 0x88
-#define YAS5XX_TEST2 0x89
-#define YAS5XX_CAL 0x90
#define YAS5XX_MEASURE_DATA 0xB0
+/* These registers are used by YAS530, YAS532 and YAS533 */
+#define YAS530_ACTUATE_INIT_COIL 0x81
+#define YAS530_MEASURE 0x82
+#define YAS530_CONFIG 0x83
+#define YAS530_MEASURE_INTERVAL 0x84
+#define YAS530_OFFSET_X 0x85 /* [-31 .. 31] */
+#define YAS530_OFFSET_Y1 0x86 /* [-31 .. 31] */
+#define YAS530_OFFSET_Y2 0x87 /* [-31 .. 31] */
+#define YAS530_TEST1 0x88
+#define YAS530_TEST2 0x89
+#define YAS530_CAL 0x90
+
/* Bits in the YAS5xx config register */
#define YAS5XX_CONFIG_INTON BIT(0) /* Interrupt on? */
#define YAS5XX_CONFIG_INTHACT BIT(1) /* Interrupt active high? */
@@ -182,7 +184,7 @@ static u16 yas532_extract_axis(u8 *data)
}
/**
- * yas5xx_measure() - Make a measure from the hardware
+ * yas530_measure() - Make a measure from the hardware
* @yas5xx: The device state
* @t: the raw temperature measurement
* @x: the raw x axis measurement
@@ -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)
+/* Used by YAS530, YAS532 and YAS533 */
+static int yas530_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_MEASURE, YAS5XX_MEASURE_START);
if (ret < 0)
goto out_unlock;
@@ -264,7 +267,8 @@ static int yas5xx_measure(struct yas5xx *yas5xx, u16 *t, u16 *x, u16 *y1, u16 *y
return ret;
}
-static s32 yas5xx_linearize(struct yas5xx *yas5xx, u16 val, int axis)
+/* Used by YAS530, YAS532 and YAS533 */
+static s32 yas530_linearize(struct yas5xx *yas5xx, u16 val, int axis)
{
struct yas5xx_calibration *c = &yas5xx->calibration;
static const s32 yas532ac_coef[] = {
@@ -306,7 +310,7 @@ static s32 yas5xx_linearize(struct yas5xx *yas5xx, u16 val, int axis)
}
/**
- * yas5xx_get_measure() - Measure a sample of all axis and process
+ * yas530_get_measure() - Measure a sample of all axis and process
* @yas5xx: The device state
* @to: Temperature out
* @xo: X axis out
@@ -314,7 +318,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)
+/* Used by YAS530, YAS532 and YAS533 */
+static int yas530_get_measure(struct yas5xx *yas5xx, s32 *to, s32 *xo, s32 *yo, s32 *zo)
{
struct yas5xx_calibration *c = &yas5xx->calibration;
u16 t_ref, t, x, y1, y2;
@@ -323,14 +328,14 @@ static int yas5xx_get_measure(struct yas5xx *yas5xx, s32 *to, s32 *xo, s32 *yo,
int ret;
/* We first get raw data that needs to be translated to [x,y,z] */
- ret = yas5xx_measure(yas5xx, &t, &x, &y1, &y2);
+ ret = yas530_measure(yas5xx, &t, &x, &y1, &y2);
if (ret)
return ret;
/* Do some linearization if available */
- sx = yas5xx_linearize(yas5xx, x, 0);
- sy1 = yas5xx_linearize(yas5xx, y1, 1);
- sy2 = yas5xx_linearize(yas5xx, y2, 2);
+ sx = yas530_linearize(yas5xx, x, 0);
+ sy1 = yas530_linearize(yas5xx, y1, 1);
+ sy2 = yas530_linearize(yas5xx, y2, 2);
/* Set the temperature reference value (unit: counts) */
switch (yas5xx->devid) {
@@ -605,11 +610,12 @@ static const struct regmap_config yas5xx_regmap_config = {
};
/**
- * yas53x_extract_calibration() - extracts the a2-a9 and k calibration
+ * yas530_extract_calibration() - extracts the a2-a9 and k calibration
* @data: the bitfield to use
* @c: the calibration to populate
*/
-static void yas53x_extract_calibration(u8 *data, struct yas5xx_calibration *c)
+/* Used by YAS530, YAS532 and YAS533 */
+static void yas530_extract_calibration(u8 *data, struct yas5xx_calibration *c)
{
u64 val = get_unaligned_be64(data);
@@ -647,12 +653,12 @@ static int yas530_get_calibration_data(struct yas5xx *yas5xx)
int ret;
/* Dummy read, first read is ALWAYS wrong */
- ret = regmap_bulk_read(yas5xx->map, YAS5XX_CAL, data, sizeof(data));
+ ret = regmap_bulk_read(yas5xx->map, YAS530_CAL, data, sizeof(data));
if (ret)
return ret;
/* Actual calibration readout */
- ret = regmap_bulk_read(yas5xx->map, YAS5XX_CAL, data, sizeof(data));
+ ret = regmap_bulk_read(yas5xx->map, YAS530_CAL, data, sizeof(data));
if (ret)
return ret;
dev_dbg(yas5xx->dev, "calibration data: %*ph\n", 14, data);
@@ -664,7 +670,7 @@ static int yas530_get_calibration_data(struct yas5xx *yas5xx)
c->Cx = data[0] * 6 - 768;
c->Cy1 = data[1] * 6 - 768;
c->Cy2 = data[2] * 6 - 768;
- yas53x_extract_calibration(&data[3], c);
+ yas530_extract_calibration(&data[3], c);
/*
* Extract linearization:
@@ -695,11 +701,11 @@ static int yas532_get_calibration_data(struct yas5xx *yas5xx)
int ret;
/* Dummy read, first read is ALWAYS wrong */
- ret = regmap_bulk_read(yas5xx->map, YAS5XX_CAL, data, sizeof(data));
+ ret = regmap_bulk_read(yas5xx->map, YAS530_CAL, data, sizeof(data));
if (ret)
return ret;
/* Actual calibration readout */
- ret = regmap_bulk_read(yas5xx->map, YAS5XX_CAL, data, sizeof(data));
+ ret = regmap_bulk_read(yas5xx->map, YAS530_CAL, data, sizeof(data));
if (ret)
return ret;
dev_dbg(yas5xx->dev, "calibration data: %*ph\n", 14, data);
@@ -718,7 +724,7 @@ static int yas532_get_calibration_data(struct yas5xx *yas5xx)
c->Cx = data[0] * 10 - 1280;
c->Cy1 = data[1] * 10 - 1280;
c->Cy2 = data[2] * 10 - 1280;
- yas53x_extract_calibration(&data[3], c);
+ yas530_extract_calibration(&data[3], c);
/*
* Extract linearization:
* Linearization layout in the 32 bits at byte 10:
@@ -741,7 +747,8 @@ static int yas532_get_calibration_data(struct yas5xx *yas5xx)
return 0;
}
-static void yas5xx_dump_calibration(struct yas5xx *yas5xx)
+/* Used by YAS530, YAS532 and YAS533 */
+static void yas530_dump_calibration(struct yas5xx *yas5xx)
{
struct yas5xx_calibration *c = &yas5xx->calibration;
@@ -764,20 +771,22 @@ static void yas5xx_dump_calibration(struct yas5xx *yas5xx)
dev_dbg(yas5xx->dev, "dck = %d\n", c->dck);
}
-static int yas5xx_set_offsets(struct yas5xx *yas5xx, s8 ox, s8 oy1, s8 oy2)
+/* Used by YAS530, YAS532 and YAS533 */
+static int yas530_set_offsets(struct yas5xx *yas5xx, s8 ox, s8 oy1, s8 oy2)
{
int ret;
- ret = regmap_write(yas5xx->map, YAS5XX_OFFSET_X, ox);
+ ret = regmap_write(yas5xx->map, YAS530_OFFSET_X, ox);
if (ret)
return ret;
- ret = regmap_write(yas5xx->map, YAS5XX_OFFSET_Y1, oy1);
+ ret = regmap_write(yas5xx->map, YAS530_OFFSET_Y1, oy1);
if (ret)
return ret;
- return regmap_write(yas5xx->map, YAS5XX_OFFSET_Y2, oy2);
+ return regmap_write(yas5xx->map, YAS530_OFFSET_Y2, oy2);
}
-static s8 yas5xx_adjust_offset(s8 old, int bit, u16 center, u16 measure)
+/* Used by YAS530, YAS532 and YAS533 */
+static s8 yas530_adjust_offset(s8 old, int bit, u16 center, u16 measure)
{
if (measure > center)
return old + BIT(bit);
@@ -786,7 +795,8 @@ static s8 yas5xx_adjust_offset(s8 old, int bit, u16 center, u16 measure)
return old;
}
-static int yas5xx_meaure_offsets(struct yas5xx *yas5xx)
+/* Used by YAS530, YAS532 and YAS533 */
+static int yas530_measure_offsets(struct yas5xx *yas5xx)
{
int ret;
u16 center;
@@ -795,7 +805,7 @@ static int yas5xx_meaure_offsets(struct yas5xx *yas5xx)
int i;
/* Actuate the init coil and measure offsets */
- ret = regmap_write(yas5xx->map, YAS5XX_ACTUATE_INIT_COIL, 0);
+ ret = regmap_write(yas5xx->map, YAS530_ACTUATE_INIT_COIL, 0);
if (ret)
return ret;
@@ -829,26 +839,26 @@ static int yas5xx_meaure_offsets(struct yas5xx *yas5xx)
oy2 = 0;
for (i = 4; i >= 0; i--) {
- ret = yas5xx_set_offsets(yas5xx, ox, oy1, oy2);
+ ret = yas530_set_offsets(yas5xx, ox, oy1, oy2);
if (ret)
return ret;
- ret = yas5xx_measure(yas5xx, &t, &x, &y1, &y2);
+ ret = yas530_measure(yas5xx, &t, &x, &y1, &y2);
if (ret)
return ret;
dev_dbg(yas5xx->dev, "measurement %d: x=%d, y1=%d, y2=%d\n",
5-i, x, y1, y2);
- ox = yas5xx_adjust_offset(ox, i, center, x);
- oy1 = yas5xx_adjust_offset(oy1, i, center, y1);
- oy2 = yas5xx_adjust_offset(oy2, i, center, y2);
+ ox = yas530_adjust_offset(ox, i, center, x);
+ oy1 = yas530_adjust_offset(oy1, i, center, y1);
+ oy2 = yas530_adjust_offset(oy2, i, center, y2);
}
/* Needed for calibration algorithm */
yas5xx->hard_offsets[0] = ox;
yas5xx->hard_offsets[1] = oy1;
yas5xx->hard_offsets[2] = oy2;
- ret = yas5xx_set_offsets(yas5xx, ox, oy1, oy2);
+ ret = yas530_set_offsets(yas5xx, ox, oy1, oy2);
if (ret)
return ret;
@@ -857,27 +867,28 @@ static int yas5xx_meaure_offsets(struct yas5xx *yas5xx)
return 0;
}
-static int yas5xx_power_on(struct yas5xx *yas5xx)
+/* Used by YAS530, YAS532 and YAS533 */
+static int yas530_power_on(struct yas5xx *yas5xx)
{
unsigned int val;
int ret;
/* Zero the test registers */
- ret = regmap_write(yas5xx->map, YAS5XX_TEST1, 0);
+ ret = regmap_write(yas5xx->map, YAS530_TEST1, 0);
if (ret)
return ret;
- ret = regmap_write(yas5xx->map, YAS5XX_TEST2, 0);
+ ret = regmap_write(yas5xx->map, YAS530_TEST2, 0);
if (ret)
return ret;
/* Set up for no interrupts, calibrated clock divider */
val = FIELD_PREP(YAS5XX_CONFIG_CCK_MASK, yas5xx->calibration.dck);
- ret = regmap_write(yas5xx->map, YAS5XX_CONFIG, val);
+ ret = regmap_write(yas5xx->map, YAS530_CONFIG, val);
if (ret)
return ret;
/* Measure interval 0 (back-to-back?) */
- return regmap_write(yas5xx->map, YAS5XX_MEASURE_INTERVAL, 0);
+ return regmap_write(yas5xx->map, YAS530_MEASURE_INTERVAL, 0);
}
static int yas5xx_probe(struct i2c_client *i2c,
@@ -959,11 +970,11 @@ static int yas5xx_probe(struct i2c_client *i2c,
goto assert_reset;
}
- yas5xx_dump_calibration(yas5xx);
- ret = yas5xx_power_on(yas5xx);
+ yas530_dump_calibration(yas5xx);
+ ret = yas530_power_on(yas5xx);
if (ret)
goto assert_reset;
- ret = yas5xx_meaure_offsets(yas5xx);
+ ret = yas530_measure_offsets(yas5xx);
if (ret)
goto assert_reset;
--
2.35.1
^ permalink raw reply related [flat|nested] 39+ messages in thread
* [PATCH v4 07/10] iio: magnetometer: yas530: Move printk %*ph parameters out from stack
2022-07-03 22:02 ` [PATCH v4 00/10] Add support for magnetometer Yamaha YAS537 Jakob Hauser
` (5 preceding siblings ...)
2022-07-03 22:02 ` [PATCH v4 06/10] iio: magnetometer: yas530: Rename functions and registers Jakob Hauser
@ 2022-07-03 22:02 ` Jakob Hauser
2022-07-04 18:06 ` Andy Shevchenko
2022-07-03 22:02 ` [PATCH v4 08/10] iio: magnetometer: yas530: Apply documentation and style fixes Jakob Hauser
` (3 subsequent siblings)
10 siblings, 1 reply; 39+ messages in thread
From: Jakob Hauser @ 2022-07-03 22:02 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Lars-Peter Clausen, Linus Walleij, Andy Shevchenko,
Hans de Goede, linux-iio, devicetree, phone-devel,
~postmarketos/upstreaming, Jakob Hauser
Use less stack by modifying %*ph parameters.
Additionally, in the function yas530_get_calibration_data(), the debug dump was
extended to 16 elements as this is the size of the calibration data array of
YAS530.
Signed-off-by: Jakob Hauser <jahau@rocketmail.com>
---
drivers/iio/magnetometer/yamaha-yas530.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/iio/magnetometer/yamaha-yas530.c b/drivers/iio/magnetometer/yamaha-yas530.c
index c250909df8f4..8358d4b3d1e2 100644
--- a/drivers/iio/magnetometer/yamaha-yas530.c
+++ b/drivers/iio/magnetometer/yamaha-yas530.c
@@ -661,7 +661,7 @@ static int yas530_get_calibration_data(struct yas5xx *yas5xx)
ret = regmap_bulk_read(yas5xx->map, YAS530_CAL, data, sizeof(data));
if (ret)
return ret;
- dev_dbg(yas5xx->dev, "calibration data: %*ph\n", 14, data);
+ dev_dbg(yas5xx->dev, "calibration data: %16ph\n", data);
add_device_randomness(data, sizeof(data));
yas5xx->version = data[15] & GENMASK(1, 0);
@@ -708,7 +708,7 @@ static int yas532_get_calibration_data(struct yas5xx *yas5xx)
ret = regmap_bulk_read(yas5xx->map, YAS530_CAL, data, sizeof(data));
if (ret)
return ret;
- dev_dbg(yas5xx->dev, "calibration data: %*ph\n", 14, data);
+ dev_dbg(yas5xx->dev, "calibration data: %14ph\n", data);
/* Sanity check, is this all zeroes? */
if (memchr_inv(data, 0x00, 13) == NULL) {
--
2.35.1
^ permalink raw reply related [flat|nested] 39+ messages in thread
* [PATCH v4 08/10] iio: magnetometer: yas530: Apply documentation and style fixes
2022-07-03 22:02 ` [PATCH v4 00/10] Add support for magnetometer Yamaha YAS537 Jakob Hauser
` (6 preceding siblings ...)
2022-07-03 22:02 ` [PATCH v4 07/10] iio: magnetometer: yas530: Move printk %*ph parameters out from stack Jakob Hauser
@ 2022-07-03 22:02 ` Jakob Hauser
2022-07-04 18:07 ` Andy Shevchenko
2022-07-04 23:29 ` Linus Walleij
2022-07-03 22:04 ` [PATCH v4 09/10] iio: magnetometer: yas530: Introduce "chip_info" structure Jakob Hauser
` (2 subsequent siblings)
10 siblings, 2 replies; 39+ messages in thread
From: Jakob Hauser @ 2022-07-03 22:02 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Lars-Peter Clausen, Linus Walleij, Andy Shevchenko,
Hans de Goede, linux-iio, devicetree, phone-devel,
~postmarketos/upstreaming, Jakob Hauser
This commit gathers several minor changes.
In the device examples, "Xiaomi" is too generic, specific devices should be
listed here. E.g. Xiaomi Redmi 2 seems to have YAS537 but it's not fully clear
if this applies to all its variants. Samsung Galaxy S7 is often quoted in
conjunction with YAS537.
Removed defines for device IDs of YAS537 and YAS539, they are not needed so far.
Signed-off-by: Jakob Hauser <jahau@rocketmail.com>
---
drivers/iio/magnetometer/yamaha-yas530.c | 21 +++++++++++----------
1 file changed, 11 insertions(+), 10 deletions(-)
diff --git a/drivers/iio/magnetometer/yamaha-yas530.c b/drivers/iio/magnetometer/yamaha-yas530.c
index 8358d4b3d1e2..4e2f460a4efd 100644
--- a/drivers/iio/magnetometer/yamaha-yas530.c
+++ b/drivers/iio/magnetometer/yamaha-yas530.c
@@ -10,7 +10,7 @@
* (YAS534 is a magnetic switch, not handled)
* YAS535 MS-6C
* YAS536 MS-3W
- * YAS537 MS-3T (2015 Samsung Galaxy S6, Note 5, Xiaomi)
+ * YAS537 MS-3T (2015 Samsung Galaxy S6, Note 5, Galaxy S7)
* YAS539 MS-3S (2018 Samsung Galaxy A7 SM-A750FN)
*
* Code functions found in the MPU3050 YAS530 and YAS532 drivers
@@ -93,10 +93,6 @@
#define YAS532_DATA_OVERFLOW (BIT(YAS532_DATA_BITS) - 1)
#define YAS532_20DEGREES 390 /* Counts starting at -50 °C */
-/* These variant IDs are known from code dumps */
-#define YAS537_DEVICE_ID 0x07 /* YAS537 (MS-3T) */
-#define YAS539_DEVICE_ID 0x08 /* YAS539 (MS-3S) */
-
/* Turn off device regulators etc after 5 seconds of inactivity */
#define YAS5XX_AUTOSUSPEND_DELAY_MS 5000
@@ -323,7 +319,7 @@ static int yas530_get_measure(struct yas5xx *yas5xx, s32 *to, s32 *xo, s32 *yo,
{
struct yas5xx_calibration *c = &yas5xx->calibration;
u16 t_ref, t, x, y1, y2;
- /* These are "signed x, signed y1 etc */
+ /* These are signed x, signed y1 etc */
s32 sx, sy1, sy2, sy, sz;
int ret;
@@ -663,7 +659,10 @@ static int yas530_get_calibration_data(struct yas5xx *yas5xx)
return ret;
dev_dbg(yas5xx->dev, "calibration data: %16ph\n", data);
+ /* Contribute calibration data to the input pool for kernel entropy */
add_device_randomness(data, sizeof(data));
+
+ /* Extract version */
yas5xx->version = data[15] & GENMASK(1, 0);
/* Extract the calibration from the bitfield */
@@ -690,6 +689,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;
}
@@ -711,12 +711,12 @@ static int yas532_get_calibration_data(struct yas5xx *yas5xx)
dev_dbg(yas5xx->dev, "calibration data: %14ph\n", data);
/* Sanity check, is this all zeroes? */
- if (memchr_inv(data, 0x00, 13) == NULL) {
- if (!(data[13] & BIT(7)))
- dev_warn(yas5xx->dev, "calibration is blank!\n");
- }
+ if (!memchr_inv(data, 0x00, 13) && !(data[13] & BIT(7)))
+ dev_warn(yas5xx->dev, "calibration is blank!\n");
+ /* Contribute calibration data to the input pool for kernel entropy */
add_device_randomness(data, sizeof(data));
+
/* Only one bit of version info reserved here as far as we know */
yas5xx->version = data[13] & BIT(0);
@@ -725,6 +725,7 @@ static int yas532_get_calibration_data(struct yas5xx *yas5xx)
c->Cy1 = data[1] * 10 - 1280;
c->Cy2 = data[2] * 10 - 1280;
yas530_extract_calibration(&data[3], c);
+
/*
* Extract linearization:
* Linearization layout in the 32 bits at byte 10:
--
2.35.1
^ permalink raw reply related [flat|nested] 39+ messages in thread
* [PATCH v4 09/10] iio: magnetometer: yas530: Introduce "chip_info" structure
2022-07-03 22:02 ` [PATCH v4 00/10] Add support for magnetometer Yamaha YAS537 Jakob Hauser
` (7 preceding siblings ...)
2022-07-03 22:02 ` [PATCH v4 08/10] iio: magnetometer: yas530: Apply documentation and style fixes Jakob Hauser
@ 2022-07-03 22:04 ` Jakob Hauser
2022-07-04 19:37 ` Andy Shevchenko
2022-07-03 22:05 ` [PATCH v4 10/10] iio: magnetometer: yas530: Add YAS537 variant Jakob Hauser
2022-07-04 23:31 ` [PATCH v4 00/10] Add support for magnetometer Yamaha YAS537 Linus Walleij
10 siblings, 1 reply; 39+ messages in thread
From: Jakob Hauser @ 2022-07-03 22:04 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Lars-Peter Clausen, Linus Walleij, Andy Shevchenko,
Hans de Goede, linux-iio, devicetree, phone-devel,
~postmarketos/upstreaming, Jakob Hauser
This commit introduces the "chip_info" structure approach for better variant
handling.
The variant to be used is now chosen by the devicetree (enum "chip_ids"),
not by the chip ID in the register. However, there is a check to make sure
they match (using integer "id_check").
Signed-off-by: Jakob Hauser <jahau@rocketmail.com>
---
Result of the patch can be seen at:
https://github.com/Jakko3/linux/blob/28a2a9ec27c6fb4073149b897415475a8f04e3f7/drivers/iio/magnetometer/yamaha-yas530.c
drivers/iio/magnetometer/yamaha-yas530.c | 314 +++++++++++++----------
1 file changed, 182 insertions(+), 132 deletions(-)
diff --git a/drivers/iio/magnetometer/yamaha-yas530.c b/drivers/iio/magnetometer/yamaha-yas530.c
index 4e2f460a4efd..ce9c1077c121 100644
--- a/drivers/iio/magnetometer/yamaha-yas530.c
+++ b/drivers/iio/magnetometer/yamaha-yas530.c
@@ -79,7 +79,6 @@
#define YAS530_DATA_BITS 12
#define YAS530_DATA_CENTER BIT(YAS530_DATA_BITS - 1)
#define YAS530_DATA_OVERFLOW (BIT(YAS530_DATA_BITS) - 1)
-#define YAS530_20DEGREES 182 /* Counts starting at -62 °C */
#define YAS532_DEVICE_ID 0x02 /* YAS532/YAS533 (MS-3R/F) */
#define YAS532_VERSION_AB 0 /* YAS532/533 AB (MS-3R/F AB) */
@@ -91,11 +90,39 @@
#define YAS532_DATA_BITS 13
#define YAS532_DATA_CENTER BIT(YAS532_DATA_BITS - 1)
#define YAS532_DATA_OVERFLOW (BIT(YAS532_DATA_BITS) - 1)
-#define YAS532_20DEGREES 390 /* Counts starting at -50 °C */
/* Turn off device regulators etc after 5 seconds of inactivity */
#define YAS5XX_AUTOSUSPEND_DELAY_MS 5000
+enum chip_ids {
+ yas530,
+ yas532,
+ yas533,
+};
+
+static const char yas5xx_product_name[][13] = {
+ "YAS530 MS-3E",
+ "YAS532 MS-3R",
+ "YAS533 MS-3F"
+};
+
+static const char yas5xx_version_name[][2][3] = {
+ { "A", "B" },
+ { "AB", "AC" },
+ { "AB", "AC" }
+};
+
+static const int yas530_volatile_reg[] = {
+ YAS530_ACTUATE_INIT_COIL,
+ YAS530_MEASURE
+};
+
+/* Number of counts between minimum and reference temperature */
+const u16 t_ref_counts[] = { 182, 390, 390 };
+
+/* Starting point of temperature counting in 1/10:s degrees Celsius */
+const s16 min_temp_celcius_x10[] = { -620, -500, -500 };
+
struct yas5xx_calibration {
/* Linearization calibration x, y1, y2 */
s32 r[3];
@@ -110,12 +137,38 @@ struct yas5xx_calibration {
u8 dck;
};
+struct yas5xx;
+
+/**
+ * struct yas5xx_chip_info - device-specific data and function pointers
+ * @devid: device ID number
+ * @volatile_reg: device-specific volatile registers
+ * @volatile_reg_qty: quantity of device-specific volatile registers
+ * @scaling_val2: scaling value for IIO_CHAN_INFO_SCALE
+ * @get_measure: function pointer to get a measurement
+ * @get_calibration_data: function pointer to get calibration data
+ * @dump_calibration: function pointer to dump calibration for debugging
+ * @measure_offsets: function pointer to measure the offsets
+ * @power_on: function pointer to power-on procedure
+ */
+struct yas5xx_chip_info {
+ unsigned int devid;
+ const int *volatile_reg;
+ const int volatile_reg_qty;
+ const u32 scaling_val2;
+ int (*get_measure)(struct yas5xx *yas5xx, s32 *to, s32 *xo, s32 *yo, s32 *zo);
+ int (*get_calibration_data)(struct yas5xx *yas5xx);
+ void (*dump_calibration)(struct yas5xx *yas5xx);
+ int (*measure_offsets)(struct yas5xx *yas5xx);
+ int (*power_on)(struct yas5xx *yas5xx);
+};
+
/**
* struct yas5xx - state container for the YAS5xx driver
* @dev: parent device pointer
- * @devid: device ID number
+ * @chip: enumeration of the device variant
+ * @chip_info: device-specific data and function pointers
* @version: device version
- * @name: device name
* @calibration: calibration settings from the OTP storage
* @hard_offsets: offsets for each axis measured with initcoil actuated
* @orientation: mounting matrix, flipped axis etc
@@ -129,9 +182,9 @@ struct yas5xx_calibration {
*/
struct yas5xx {
struct device *dev;
- unsigned int devid;
+ enum chip_ids chip;
+ struct yas5xx_chip_info *chip_info;
unsigned int version;
- char name[16];
struct yas5xx_calibration calibration;
s8 hard_offsets[3];
struct iio_mount_matrix orientation;
@@ -221,7 +274,7 @@ static int yas530_measure(struct yas5xx *yas5xx, u16 *t, u16 *x, u16 *y1, u16 *y
mutex_unlock(&yas5xx->lock);
- switch (yas5xx->devid) {
+ switch (yas5xx->chip_info->devid) {
case YAS530_DEVICE_ID:
/*
* The t value is 9 bits in big endian format
@@ -275,7 +328,7 @@ static s32 yas530_linearize(struct yas5xx *yas5xx, u16 val, int axis)
s32 coef;
/* Select coefficients */
- switch (yas5xx->devid) {
+ switch (yas5xx->chip_info->devid) {
case YAS530_DEVICE_ID:
if (yas5xx->version == YAS530_VERSION_A)
coef = YAS530_VERSION_A_COEF;
@@ -305,6 +358,20 @@ static s32 yas530_linearize(struct yas5xx *yas5xx, u16 val, int axis)
(yas5xx->hard_offsets[axis] - c->r[axis]) * coef;
}
+static s32 yas5xx_calc_temperature(struct yas5xx *yas5xx, u16 t)
+{
+ s32 to;
+ u16 t_ref;
+ int min_temp_x10, ref_temp_x10;
+
+ t_ref = t_ref_counts[yas5xx->chip];
+ min_temp_x10 = min_temp_celcius_x10[yas5xx->chip];
+ ref_temp_x10 = 200;
+
+ to = (min_temp_x10 + ((ref_temp_x10 - min_temp_x10) * t / t_ref)) * 100;
+ return to;
+}
+
/**
* yas530_get_measure() - Measure a sample of all axis and process
* @yas5xx: The device state
@@ -318,7 +385,7 @@ static s32 yas530_linearize(struct yas5xx *yas5xx, u16 val, int axis)
static int yas530_get_measure(struct yas5xx *yas5xx, s32 *to, s32 *xo, s32 *yo, s32 *zo)
{
struct yas5xx_calibration *c = &yas5xx->calibration;
- u16 t_ref, t, x, y1, y2;
+ u16 t_ref, t_comp, t, x, y1, y2;
/* These are signed x, signed y1 etc */
s32 sx, sy1, sy2, sy, sz;
int ret;
@@ -333,47 +400,30 @@ static int yas530_get_measure(struct yas5xx *yas5xx, s32 *to, s32 *xo, s32 *yo,
sy1 = yas530_linearize(yas5xx, y1, 1);
sy2 = yas530_linearize(yas5xx, y2, 2);
- /* Set the temperature reference value (unit: counts) */
- switch (yas5xx->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 &&
+ /*
+ * Set the temperature for compensation (unit: counts):
+ * YAS532/YAS533 version AC uses the temperature deviation as a
+ * multiplier. YAS530 and YAS532 version AB use solely the t value.
+ */
+ t_ref = t_ref_counts[yas5xx->chip];
+ if (yas5xx->chip_info->devid == YAS532_DEVICE_ID &&
yas5xx->version == YAS532_VERSION_AC) {
- /*
- * YAS532 version AC uses the temperature deviation as a
- * multiplier.
- *
- * Cx * (t - t_ref)
- * x' = x - ----------------
- * 100
- */
- sx = sx - (c->Cx * (t - t_ref)) / 100;
- sy1 = sy1 - (c->Cy1 * (t - t_ref)) / 100;
- sy2 = sy2 - (c->Cy2 * (t - t_ref)) / 100;
+ t_comp = t - t_ref;
} else {
- /*
- * YAS530 and YAS532 version AB use solely the t value as a
- * multiplier.
- *
- * Cx * t
- * x' = x - ------
- * 100
- */
- sx = sx - (c->Cx * t) / 100;
- sy1 = sy1 - (c->Cy1 * t) / 100;
- sy2 = sy2 - (c->Cy2 * t) / 100;
+ t_comp = t;
}
+ /*
+ * Temperature compensation for x, y1, y2 respectively:
+ *
+ * Cx * t_comp
+ * x' = x - -----------
+ * 100
+ */
+ sx = sx - (c->Cx * t_comp) / 100;
+ sy1 = sy1 - (c->Cy1 * t_comp) / 100;
+ sy2 = sy2 - (c->Cy2 * t_comp) / 100;
+
/*
* Break y1 and y2 into y and z, y1 and y2 are apparently encoding
* y and z.
@@ -381,36 +431,8 @@ static int yas530_get_measure(struct yas5xx *yas5xx, s32 *to, s32 *xo, s32 *yo,
sy = sy1 - sy2;
sz = -sy1 - sy2;
- /* Process temperature readout */
- switch (yas5xx->devid) {
- case YAS530_DEVICE_ID:
- /*
- * Raw temperature value t is the number of counts starting
- * at -62 °C. Reference value t_ref is the number of counts
- * between -62 °C and 20 °C (82 °C range).
- *
- * Temperature in °C would be (82 / t_ref * t) - 62.
- *
- * Contrary to this, perform multiplication first and division
- * second due to calculating with integers.
- *
- * To get a nicer result, calculate with 1/10:s degrees Celsius
- * and finally multiply by 100 to return millidegrees Celsius.
- */
- *to = ((820 * t / t_ref) - 620) * 100;
- break;
- case YAS532_DEVICE_ID:
- /*
- * Actually same procedure for YAS532 but the starting point is
- * at -50 °C. Reference value t_ref is the number of counts
- * between -50 °C and 20 °C (70 °C range).
- */
- *to = ((700 * t / t_ref) - 500) * 100;
- break;
- default:
- dev_err(yas5xx->dev, "unknown device type\n");
- return -EINVAL;
- }
+ /* Calculate temperature readout */
+ *to = yas5xx_calc_temperature(yas5xx, t);
/*
* Calibrate [x,y,z] with some formulas like this:
@@ -447,7 +469,7 @@ static int yas5xx_read_raw(struct iio_dev *indio_dev,
case IIO_CHAN_INFO_PROCESSED:
case IIO_CHAN_INFO_RAW:
pm_runtime_get_sync(yas5xx->dev);
- ret = yas5xx_get_measure(yas5xx, &t, &x, &y, &z);
+ ret = yas5xx->chip_info->get_measure(yas5xx, &t, &x, &y, &z);
pm_runtime_mark_last_busy(yas5xx->dev);
pm_runtime_put_autosuspend(yas5xx->dev);
if (ret)
@@ -471,27 +493,8 @@ static int yas5xx_read_raw(struct iio_dev *indio_dev,
}
return IIO_VAL_INT;
case IIO_CHAN_INFO_SCALE:
- switch (yas5xx->devid) {
- case YAS530_DEVICE_ID:
- /*
- * Raw values of YAS530 are in picotesla. Divide by
- * 100000000 (10^8) to get Gauss.
- */
- *val = 1;
- *val2 = 100000000;
- break;
- case YAS532_DEVICE_ID:
- /*
- * Raw values of YAS532 are in nanotesla. Divide by
- * 100000 (10^5) to get Gauss.
- */
- *val = 1;
- *val2 = 100000;
- break;
- default:
- dev_err(yas5xx->dev, "unknown device type\n");
- return -EINVAL;
- }
+ *val = 1;
+ *val2 = yas5xx->chip_info->scaling_val2;
return IIO_VAL_FRACTIONAL;
default:
/* Unknown request */
@@ -506,7 +509,7 @@ static void yas5xx_fill_buffer(struct iio_dev *indio_dev)
int ret;
pm_runtime_get_sync(yas5xx->dev);
- ret = yas5xx_get_measure(yas5xx, &t, &x, &y, &z);
+ ret = yas5xx->chip_info->get_measure(yas5xx, &t, &x, &y, &z);
pm_runtime_mark_last_busy(yas5xx->dev);
pm_runtime_put_autosuspend(yas5xx->dev);
if (ret) {
@@ -592,9 +595,24 @@ 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 + 8);
+ struct iio_dev *indio_dev = dev_get_drvdata(dev);
+ struct yas5xx *yas5xx = iio_priv(indio_dev);
+ int i, j;
+
+ if (reg >= YAS5XX_MEASURE_DATA && reg < YAS5XX_MEASURE_DATA + 8)
+ return true;
+
+ /*
+ * YAS versions share different registers on the same address,
+ * need to differentiate.
+ */
+ j = yas5xx->chip_info->volatile_reg_qty;
+ for (i = 0; i < j; i++) {
+ if (reg == yas5xx->chip_info->volatile_reg[i])
+ return true;
+ }
+
+ return false;
}
/* TODO: enable regmap cache, using mark dirty and sync at runtime resume */
@@ -811,7 +829,7 @@ static int yas530_measure_offsets(struct yas5xx *yas5xx)
return ret;
/* When the initcoil is active this should be around the center */
- switch (yas5xx->devid) {
+ switch (yas5xx->chip_info->devid) {
case YAS530_DEVICE_ID:
center = YAS530_DATA_CENTER;
break;
@@ -892,13 +910,49 @@ static int yas530_power_on(struct yas5xx *yas5xx)
return regmap_write(yas5xx->map, YAS530_MEASURE_INTERVAL, 0);
}
+static struct yas5xx_chip_info yas5xx_chip_info_tbl[] = {
+ [yas530] = {
+ .devid = YAS530_DEVICE_ID,
+ .volatile_reg = yas530_volatile_reg,
+ .volatile_reg_qty = ARRAY_SIZE(yas530_volatile_reg),
+ .scaling_val2 = 100000000, /* picotesla to Gauss */
+ .get_measure = yas530_get_measure,
+ .get_calibration_data = yas530_get_calibration_data,
+ .dump_calibration = yas530_dump_calibration,
+ .measure_offsets = yas530_measure_offsets,
+ .power_on = yas530_power_on,
+ },
+ [yas532] = {
+ .devid = YAS532_DEVICE_ID,
+ .volatile_reg = yas530_volatile_reg,
+ .volatile_reg_qty = ARRAY_SIZE(yas530_volatile_reg),
+ .scaling_val2 = 100000, /* nanotesla to Gauss */
+ .get_measure = yas530_get_measure,
+ .get_calibration_data = yas532_get_calibration_data,
+ .dump_calibration = yas530_dump_calibration,
+ .measure_offsets = yas530_measure_offsets,
+ .power_on = yas530_power_on,
+ },
+ [yas533] = {
+ .devid = YAS532_DEVICE_ID,
+ .volatile_reg = yas530_volatile_reg,
+ .volatile_reg_qty = ARRAY_SIZE(yas530_volatile_reg),
+ .scaling_val2 = 100000, /* nanotesla to Gauss */
+ .get_measure = yas530_get_measure,
+ .get_calibration_data = yas532_get_calibration_data,
+ .dump_calibration = yas530_dump_calibration,
+ .measure_offsets = yas530_measure_offsets,
+ .power_on = yas530_power_on,
+ }
+};
+
static int yas5xx_probe(struct i2c_client *i2c,
const struct i2c_device_id *id)
{
struct iio_dev *indio_dev;
struct device *dev = &i2c->dev;
struct yas5xx *yas5xx;
- int ret;
+ int id_check, ret;
indio_dev = devm_iio_device_alloc(dev, sizeof(*yas5xx));
if (!indio_dev)
@@ -944,45 +998,41 @@ static int yas5xx_probe(struct i2c_client *i2c,
goto assert_reset;
}
- ret = regmap_read(yas5xx->map, YAS5XX_DEVICE_ID, &yas5xx->devid);
+ yas5xx->chip = id->driver_data;
+ yas5xx->chip_info = &yas5xx_chip_info_tbl[yas5xx->chip];
+
+ ret = regmap_read(yas5xx->map, YAS5XX_DEVICE_ID, &id_check);
if (ret)
goto assert_reset;
- switch (yas5xx->devid) {
- case YAS530_DEVICE_ID:
- ret = yas530_get_calibration_data(yas5xx);
- if (ret)
- goto assert_reset;
- dev_info(dev, "detected YAS530 MS-3E %s",
- yas5xx->version ? "B" : "A");
- strncpy(yas5xx->name, "yas530", sizeof(yas5xx->name));
- break;
- case YAS532_DEVICE_ID:
- ret = yas532_get_calibration_data(yas5xx);
- if (ret)
- goto assert_reset;
- dev_info(dev, "detected YAS532/YAS533 MS-3R/F %s",
- yas5xx->version ? "AC" : "AB");
- strncpy(yas5xx->name, "yas532", sizeof(yas5xx->name));
- break;
- default:
+ if (id_check != yas5xx->chip_info->devid) {
ret = -ENODEV;
- dev_err(dev, "unhandled device ID %02x\n", yas5xx->devid);
+ dev_err(dev, "device ID %02x doesn't match %s\n",
+ id_check, id->name);
goto assert_reset;
}
- yas530_dump_calibration(yas5xx);
- ret = yas530_power_on(yas5xx);
+ ret = yas5xx->chip_info->get_calibration_data(yas5xx);
if (ret)
goto assert_reset;
- ret = yas530_measure_offsets(yas5xx);
+
+ dev_info(dev, "detected %s %s\n", yas5xx_product_name[yas5xx->chip],
+ yas5xx_version_name[yas5xx->chip][yas5xx->version]);
+
+ yas5xx->chip_info->dump_calibration(yas5xx);
+
+ ret = yas5xx->chip_info->power_on(yas5xx);
+ if (ret)
+ goto assert_reset;
+
+ ret = yas5xx->chip_info->measure_offsets(yas5xx);
if (ret)
goto assert_reset;
indio_dev->info = &yas5xx_info;
indio_dev->available_scan_masks = yas5xx_scan_masks;
indio_dev->modes = INDIO_DIRECT_MODE;
- indio_dev->name = yas5xx->name;
+ indio_dev->name = id->name;
indio_dev->channels = yas5xx_channels;
indio_dev->num_channels = ARRAY_SIZE(yas5xx_channels);
@@ -1074,7 +1124,7 @@ static int __maybe_unused yas5xx_runtime_resume(struct device *dev)
usleep_range(31000, 40000);
gpiod_set_value_cansleep(yas5xx->reset, 0);
- ret = yas5xx_power_on(yas5xx);
+ ret = yas5xx->chip_info->power_on(yas5xx);
if (ret) {
dev_err(dev, "cannot power on\n");
goto out_reset;
@@ -1097,9 +1147,9 @@ static const struct dev_pm_ops yas5xx_dev_pm_ops = {
};
static const struct i2c_device_id yas5xx_id[] = {
- {"yas530", },
- {"yas532", },
- {"yas533", },
+ {"yas530", yas530 },
+ {"yas532", yas532 },
+ {"yas533", yas533 },
{}
};
MODULE_DEVICE_TABLE(i2c, yas5xx_id);
--
2.35.1
^ permalink raw reply related [flat|nested] 39+ messages in thread
* [PATCH v4 10/10] iio: magnetometer: yas530: Add YAS537 variant
2022-07-03 22:02 ` [PATCH v4 00/10] Add support for magnetometer Yamaha YAS537 Jakob Hauser
` (8 preceding siblings ...)
2022-07-03 22:04 ` [PATCH v4 09/10] iio: magnetometer: yas530: Introduce "chip_info" structure Jakob Hauser
@ 2022-07-03 22:05 ` Jakob Hauser
2022-07-04 19:47 ` Andy Shevchenko
2022-07-04 23:31 ` [PATCH v4 00/10] Add support for magnetometer Yamaha YAS537 Linus Walleij
10 siblings, 1 reply; 39+ messages in thread
From: Jakob Hauser @ 2022-07-03 22:05 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Lars-Peter Clausen, Linus Walleij, Andy Shevchenko,
Hans de Goede, linux-iio, devicetree, phone-devel,
~postmarketos/upstreaming, Jakob Hauser
This adds support for the magnetometer Yamaha YAS537. The additions are based
on comparison of Yamaha Android kernel drivers for YAS532 [1] and YAS537 [2].
In the Yamaha YAS537 Android driver, there is an overflow/underflow control
implemented. For regular usage, this seems not necessary. A similar overflow/
underflow control of Yamaha YAS530/532 Android driver isn't integrated in the
mainline driver. It is therefore skipped for YAS537 in mainline too.
Also in the Yamaha YAS537 Android driver, at the end of the reset_yas537()
function, a measurement is saved in "last_after_rcoil". Later on, this is
compared to current measurements. If the difference gets too big, a new
reset is initialized. The difference in measurements needs to be quite big,
it's hard to say if this is necessary for regular operation. Therefore this
isn't integrated in the mainline driver either.
[1] https://github.com/msm8916-mainline/android_kernel_qcom_msm8916/blob/GT-I9195I/drivers/iio/magnetometer/yas_mag_drv-yas532.c
[2] https://github.com/msm8916-mainline/android_kernel_qcom_msm8916/blob/GT-I9195I/drivers/iio/magnetometer/yas_mag_drv-yas537.c
Signed-off-by: Jakob Hauser <jahau@rocketmail.com>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
---
Result of the patch can be seen at:
https://github.com/Jakko3/linux/blob/yas537_v4/drivers/iio/magnetometer/yamaha-yas530.c
drivers/iio/magnetometer/Kconfig | 4 +-
drivers/iio/magnetometer/yamaha-yas530.c | 434 ++++++++++++++++++++++-
2 files changed, 429 insertions(+), 9 deletions(-)
diff --git a/drivers/iio/magnetometer/Kconfig b/drivers/iio/magnetometer/Kconfig
index 07eb619bcfe8..b91fc5e6a26e 100644
--- a/drivers/iio/magnetometer/Kconfig
+++ b/drivers/iio/magnetometer/Kconfig
@@ -216,8 +216,8 @@ config YAMAHA_YAS530
select IIO_TRIGGERED_BUFFER
help
Say Y here to add support for the Yamaha YAS530 series of
- 3-Axis Magnetometers. Right now YAS530, YAS532 and YAS533 are
- fully supported.
+ 3-Axis Magnetometers. YAS530, YAS532, YAS533 and YAS537 are
+ supported.
This driver can also be compiled as a module.
To compile this driver as a module, choose M here: the module
diff --git a/drivers/iio/magnetometer/yamaha-yas530.c b/drivers/iio/magnetometer/yamaha-yas530.c
index ce9c1077c121..4692e8bd4de3 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_TEST2 0x89
#define YAS530_CAL 0x90
+/* Registers used by YAS537 */
+#define YAS537_MEASURE 0x81 /* Originally YAS537_REG_CMDR */
+#define YAS537_CONFIG 0x82 /* Originally YAS537_REG_CONFR */
+#define YAS537_MEASURE_INTERVAL 0x83 /* Originally YAS537_REG_INTRVLR */
+#define YAS537_OFFSET_X 0x84 /* Originally YAS537_REG_OXR */
+#define YAS537_OFFSET_Y1 0x85 /* Originally YAS537_REG_OY1R */
+#define YAS537_OFFSET_Y2 0x86 /* Originally YAS537_REG_OY2R */
+#define YAS537_AVR 0x87
+#define YAS537_HCK 0x88
+#define YAS537_LCK 0x89
+#define YAS537_SRST 0x90
+#define YAS537_ADCCAL 0x91
+#define YAS537_MTC 0x93
+#define YAS537_OC 0x9E
+#define YAS537_TRM 0x9F
+#define YAS537_CAL 0xC0
+
/* Bits in the YAS5xx config register */
#define YAS5XX_CONFIG_INTON BIT(0) /* Interrupt on? */
#define YAS5XX_CONFIG_INTHACT BIT(1) /* Interrupt active high? */
@@ -67,6 +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)
@@ -91,6 +112,22 @@
#define YAS532_DATA_CENTER BIT(YAS532_DATA_BITS - 1)
#define YAS532_DATA_OVERFLOW (BIT(YAS532_DATA_BITS) - 1)
+#define YAS537_DEVICE_ID 0x07 /* YAS537 (MS-3T) */
+#define YAS537_VERSION_0 0 /* Version naming unknown */
+#define YAS537_VERSION_1 1 /* Version naming unknown */
+#define YAS537_MAG_AVERAGE_32_MASK GENMASK(6, 4)
+#define YAS537_MEASURE_TIME_WORST_US 1500
+#define YAS537_DEFAULT_SENSOR_DELAY_MS 50
+#define YAS537_MAG_RCOIL_TIME_US 65
+#define YAS537_MTC3_MASK_PREP GENMASK(7, 0)
+#define YAS537_MTC3_MASK_GET GENMASK(7, 5)
+#define YAS537_MTC3_ADD_BIT BIT(4)
+#define YAS537_HCK_MASK_PREP GENMASK(4, 0)
+#define YAS537_HCK_MASK_GET GENMASK(7, 4)
+#define YAS537_LCK_MASK_PREP GENMASK(4, 0)
+#define YAS537_LCK_MASK_GET GENMASK(3, 0)
+#define YAS537_OC_MASK_GET GENMASK(5, 0)
+
/* Turn off device regulators etc after 5 seconds of inactivity */
#define YAS5XX_AUTOSUSPEND_DELAY_MS 5000
@@ -98,18 +135,21 @@ enum chip_ids {
yas530,
yas532,
yas533,
+ yas537,
};
static const char yas5xx_product_name[][13] = {
"YAS530 MS-3E",
"YAS532 MS-3R",
- "YAS533 MS-3F"
+ "YAS533 MS-3F",
+ "YAS537 MS-3T"
};
static const char yas5xx_version_name[][2][3] = {
{ "A", "B" },
{ "AB", "AC" },
- { "AB", "AC" }
+ { "AB", "AC" },
+ { "v0", "v1" }
};
static const int yas530_volatile_reg[] = {
@@ -117,11 +157,15 @@ static const int yas530_volatile_reg[] = {
YAS530_MEASURE
};
+static const int yas537_volatile_reg[] = {
+ YAS537_MEASURE
+};
+
/* Number of counts between minimum and reference temperature */
-const u16 t_ref_counts[] = { 182, 390, 390 };
+const u16 t_ref_counts[] = { 182, 390, 390, 8120 };
/* Starting point of temperature counting in 1/10:s degrees Celsius */
-const s16 min_temp_celcius_x10[] = { -620, -500, -500 };
+const s16 min_temp_celcius_x10[] = { -620, -500, -500, -3860 };
struct yas5xx_calibration {
/* Linearization calibration x, y1, y2 */
@@ -316,6 +360,77 @@ static int yas530_measure(struct yas5xx *yas5xx, u16 *t, u16 *x, u16 *y1, u16 *y
return ret;
}
+/**
+ * yas537_measure() - Make a measure from the hardware
+ * @yas5xx: The device state
+ * @t: the raw temperature measurement
+ * @x: the raw x axis measurement
+ * @y1: the y1 axis measurement
+ * @y2: the y2 axis measurement
+ * @return: 0 on success or error code
+ */
+static int yas537_measure(struct yas5xx *yas5xx, u16 *t, u16 *x, u16 *y1, u16 *y2)
+{
+ struct yas5xx_calibration *c = &yas5xx->calibration;
+ unsigned int busy;
+ u8 data[8];
+ u16 xy1y2[3];
+ s32 h[3], s[3];
+ int i, ret;
+
+ mutex_lock(&yas5xx->lock);
+
+ /* Contrary to YAS530/532, also a "cont" bit is set, meaning unknown */
+ ret = regmap_write(yas5xx->map, YAS537_MEASURE, YAS5XX_MEASURE_START |
+ YAS5XX_MEASURE_CONT);
+ if (ret < 0)
+ goto out_unlock;
+
+ /* Use same timeout like YAS530/532 but the bit is in data row 2 */
+ ret = regmap_read_poll_timeout(yas5xx->map, YAS5XX_MEASURE_DATA + 2, busy,
+ !(busy & YAS5XX_MEASURE_DATA_BUSY),
+ 500, 20000);
+ if (ret) {
+ dev_err(yas5xx->dev, "timeout waiting for measurement\n");
+ goto out_unlock;
+ }
+
+ ret = regmap_bulk_read(yas5xx->map, YAS5XX_MEASURE_DATA,
+ data, sizeof(data));
+ if (ret)
+ goto out_unlock;
+
+ mutex_unlock(&yas5xx->lock);
+
+ *t = get_unaligned_be16(&data[0]);
+ xy1y2[0] = FIELD_GET(GENMASK(13, 0), get_unaligned_be16(&data[2]));
+ xy1y2[1] = get_unaligned_be16(&data[4]);
+ xy1y2[2] = get_unaligned_be16(&data[6]);
+
+ /* The second version of YAS537 needs to include calibration coefficients */
+ if (yas5xx->version == YAS537_VERSION_1) {
+ for (i = 0; i < 3; i++)
+ s[i] = xy1y2[i] - BIT(13);
+ h[0] = (c->k * (128 * s[0] + c->a2 * s[1] + c->a3 * s[2])) / BIT(13);
+ h[1] = (c->k * (c->a4 * s[0] + c->a5 * s[1] + c->a6 * s[2])) / BIT(13);
+ h[2] = (c->k * (c->a7 * s[0] + c->a8 * s[1] + c->a9 * s[2])) / BIT(13);
+ for (i = 0; i < 3; i++) {
+ clamp_val(h[i], -BIT(13), BIT(13) - 1);
+ xy1y2[i] = h[i] + BIT(13);
+ }
+ }
+
+ *x = xy1y2[0];
+ *y1 = xy1y2[1];
+ *y2 = xy1y2[2];
+
+ return 0;
+
+out_unlock:
+ mutex_unlock(&yas5xx->lock);
+ return ret;
+}
+
/* Used by YAS530, YAS532 and YAS533 */
static s32 yas530_linearize(struct yas5xx *yas5xx, u16 val, int axis)
{
@@ -456,6 +571,41 @@ static int yas530_get_measure(struct yas5xx *yas5xx, s32 *to, s32 *xo, s32 *yo,
return 0;
}
+/**
+ * yas537_get_measure() - Measure a sample of all axis and process
+ * @yas5xx: The device state
+ * @to: Temperature out
+ * @xo: X axis out
+ * @yo: Y axis out
+ * @zo: Z axis out
+ * @return: 0 on success or error code
+ */
+static int yas537_get_measure(struct yas5xx *yas5xx, s32 *to, s32 *xo, s32 *yo, s32 *zo)
+{
+ u16 t, x, y1, y2;
+ int ret;
+
+ /* We first get raw data that needs to be translated to [x,y,z] */
+ ret = yas537_measure(yas5xx, &t, &x, &y1, &y2);
+ if (ret)
+ return ret;
+
+ /* Calculate temperature readout */
+ *to = yas5xx_calc_temperature(yas5xx, t);
+
+ /*
+ * Unfortunately, no linearization or temperature compensation formulas
+ * are known for YAS537.
+ */
+
+ /* Calculate x, y, z from x, y1, y2 */
+ *xo = (x - BIT(13)) * 300;
+ *yo = (y1 - y2) * 1732 / 10;
+ *zo = (-y1 - y2 + BIT(14)) * 300;
+
+ return 0;
+}
+
static int yas5xx_read_raw(struct iio_dev *indio_dev,
struct iio_chan_spec const *chan,
int *val, int *val2,
@@ -766,6 +916,202 @@ static int yas532_get_calibration_data(struct yas5xx *yas5xx)
return 0;
}
+static int yas537_get_calibration_data(struct yas5xx *yas5xx)
+{
+ struct yas5xx_calibration *c = &yas5xx->calibration;
+ u8 data[17];
+ u32 val1, val2, val3, val4;
+ int i, ret;
+
+ /* Writing SRST register */
+ ret = regmap_write(yas5xx->map, YAS537_SRST, BIT(1));
+ if (ret)
+ return ret;
+
+ /* Calibration readout, YAS537 needs one readout only */
+ ret = regmap_bulk_read(yas5xx->map, YAS537_CAL, data, sizeof(data));
+ if (ret)
+ return ret;
+ dev_dbg(yas5xx->dev, "calibration data: %17ph\n", data);
+
+ /* Sanity check, is this all zeroes? */
+ if (!memchr_inv(data, 0x00, 16) && !FIELD_GET(GENMASK(5, 0), data[16]))
+ dev_warn(yas5xx->dev, "calibration is blank!\n");
+
+ /* Contribute calibration data to the input pool for kernel entropy */
+ add_device_randomness(data, sizeof(data));
+
+ /* Extract version information */
+ yas5xx->version = FIELD_GET(GENMASK(7, 6), data[16]);
+
+ /* There are two versions of YAS537 behaving differently */
+ switch (yas5xx->version) {
+ case YAS537_VERSION_0:
+ /*
+ * The first version simply writes data back into registers:
+ *
+ * data[0] YAS537_MTC 0x93
+ * data[1] 0x94
+ * data[2] 0x95
+ * data[3] 0x96
+ * data[4] 0x97
+ * data[5] 0x98
+ * data[6] 0x99
+ * data[7] 0x9a
+ * data[8] 0x9b
+ * data[9] 0x9c
+ * data[10] 0x9d
+ * data[11] YAS537_OC 0x9e
+ *
+ * data[12] YAS537_OFFSET_X 0x84
+ * data[13] YAS537_OFFSET_Y1 0x85
+ * data[14] YAS537_OFFSET_Y2 0x86
+ *
+ * data[15] YAS537_HCK 0x88
+ * data[16] YAS537_LCK 0x89
+ */
+ for (i = 0; i < 12; i++) {
+ ret = regmap_write(yas5xx->map, YAS537_MTC + i,
+ data[i]);
+ if (ret)
+ return ret;
+ }
+ for (i = 0; i < 3; i++) {
+ ret = regmap_write(yas5xx->map, YAS537_OFFSET_X + i,
+ data[i + 12]);
+ if (ret)
+ return ret;
+ yas5xx->hard_offsets[i] = data[i + 12];
+ }
+ for (i = 0; i < 2; i++) {
+ ret = regmap_write(yas5xx->map, YAS537_HCK + i,
+ data[i + 15]);
+ if (ret)
+ return ret;
+ }
+ break;
+ case YAS537_VERSION_1:
+ /*
+ * The second version writes some data into registers but also
+ * extracts calibration coefficients.
+ *
+ * Registers being written:
+ *
+ * data[0] YAS537_MTC 0x93
+ * data[1] YAS537_MTC+1 0x94
+ * data[2] YAS537_MTC+2 0x95
+ * data[3] YAS537_MTC+3 (partially) 0x96
+ *
+ * data[12] YAS537_OFFSET_X 0x84
+ * data[13] YAS537_OFFSET_Y1 0x85
+ * data[14] YAS537_OFFSET_Y2 0x86
+ *
+ * data[15] YAS537_HCK (partially) 0x88
+ * YAS537_LCK (partially) 0x89
+ * data[16] YAS537_OC (partially) 0x9e
+ */
+ for (i = 0; i < 3; i++) {
+ ret = regmap_write(yas5xx->map, YAS537_MTC + i,
+ data[i]);
+ if (ret)
+ return ret;
+ }
+ for (i = 0; i < 3; i++) {
+ ret = regmap_write(yas5xx->map, YAS537_OFFSET_X + i,
+ data[i + 12]);
+ if (ret)
+ return ret;
+ yas5xx->hard_offsets[i] = data[i + 12];
+ }
+ /*
+ * Visualization of partially taken data:
+ *
+ * data[3] n 7 6 5 4 3 2 1 0
+ * YAS537_MTC+3 x x x 1 0 0 0 0
+ *
+ * data[15] n 7 6 5 4 3 2 1 0
+ * YAS537_HCK x x x x 0
+ *
+ * data[15] n 7 6 5 4 3 2 1 0
+ * YAS537_LCK x x x x 0
+ *
+ * data[16] n 7 6 5 4 3 2 1 0
+ * YAS537_OC x x x x x x
+ */
+ ret = regmap_write(yas5xx->map, YAS537_MTC + 3,
+ FIELD_PREP(YAS537_MTC3_MASK_PREP,
+ FIELD_GET(YAS537_MTC3_MASK_GET, data[3])) |
+ YAS537_MTC3_ADD_BIT);
+ if (ret)
+ return ret;
+ ret = regmap_write(yas5xx->map, YAS537_HCK,
+ FIELD_PREP(YAS537_HCK_MASK_PREP,
+ FIELD_GET(YAS537_HCK_MASK_GET, data[15])));
+ if (ret)
+ return ret;
+ ret = regmap_write(yas5xx->map, YAS537_LCK,
+ FIELD_PREP(YAS537_LCK_MASK_PREP,
+ FIELD_GET(YAS537_LCK_MASK_GET, data[15])));
+ if (ret)
+ return ret;
+ ret = regmap_write(yas5xx->map, YAS537_OC,
+ FIELD_GET(YAS537_OC_MASK_GET, data[16]));
+ if (ret)
+ return ret;
+ /*
+ * For data extraction, build some blocks. Four 32-bit blocks
+ * look appropriate.
+ *
+ * n 7 6 5 4 3 2 1 0
+ * data[0] 0 [ Cx Cx Cx Cx Cx Cx Cx Cx ] bits 31 .. 24
+ * data[1] 1 [ Cx C1 C1 C1 C1 C1 C1 C1 ] bits 23 .. 16
+ * data[2] 2 [ C1 C1 C2 C2 C2 C2 C2 C2 ] bits 15 .. 8
+ * data[3] 3 [ C2 C2 C2 ] bits 7 .. 0
+ *
+ * n 7 6 5 4 3 2 1 0
+ * data[3] 0 [ a2 a2 a2 a2 a2 ] bits 31 .. 24
+ * data[4] 1 [ a2 a2 a3 a3 a3 a3 a3 a3 ] bits 23 .. 16
+ * data[5] 2 [ a3 a4 a4 a4 a4 a4 a4 a4 ] bits 15 .. 8
+ * data[6] 3 [ a4 ] bits 7 .. 0
+ *
+ * n 7 6 5 4 3 2 1 0
+ * data[6] 0 [ a5 a5 a5 a5 a5 a5 a5 ] bits 31 .. 24
+ * data[7] 1 [ a5 a5 a6 a6 a6 a6 a6 a6 ] bits 23 .. 16
+ * data[8] 2 [ a6 a7 a7 a7 a7 a7 a7 a7 ] bits 15 .. 8
+ * data[9] 3 [ a7 ] bits 7 .. 0
+ *
+ * n 7 6 5 4 3 2 1 0
+ * data[9] 0 [ a8 a8 a8 a8 a8 a8 a8 ] bits 31 .. 24
+ * data[10] 1 [ a9 a9 a9 a9 a9 a9 a9 a9 ] bits 23 .. 16
+ * data[11] 2 [ a9 k k k k k k k ] bits 15 .. 8
+ * data[12] 3 [ ] bits 7 .. 0
+ */
+ val1 = get_unaligned_be32(&data[0]);
+ val2 = get_unaligned_be32(&data[3]);
+ val3 = get_unaligned_be32(&data[6]);
+ val4 = get_unaligned_be32(&data[9]);
+ /* Extract calibration coefficients and modify */
+ c->Cx = FIELD_GET(GENMASK(31, 23), val1) - 256;
+ c->Cy1 = FIELD_GET(GENMASK(22, 14), val1) - 256;
+ c->Cy2 = FIELD_GET(GENMASK(13, 5), val1) - 256;
+ c->a2 = FIELD_GET(GENMASK(28, 22), val2) - 64;
+ c->a3 = FIELD_GET(GENMASK(21, 15), val2) - 64;
+ c->a4 = FIELD_GET(GENMASK(14, 7), val2) - 128;
+ c->a5 = FIELD_GET(GENMASK(30, 22), val3) - 112;
+ c->a6 = FIELD_GET(GENMASK(21, 15), val3) - 64;
+ c->a7 = FIELD_GET(GENMASK(14, 7), val3) - 128;
+ c->a8 = FIELD_GET(GENMASK(30, 24), val4) - 64;
+ c->a9 = FIELD_GET(GENMASK(23, 15), val4) - 112;
+ c->k = FIELD_GET(GENMASK(14, 8), val4);
+ break;
+ default:
+ dev_err(yas5xx->dev, "unknown version of YAS537\n");
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
/* Used by YAS530, YAS532 and YAS533 */
static void yas530_dump_calibration(struct yas5xx *yas5xx)
{
@@ -790,6 +1136,26 @@ static void yas530_dump_calibration(struct yas5xx *yas5xx)
dev_dbg(yas5xx->dev, "dck = %d\n", c->dck);
}
+static void yas537_dump_calibration(struct yas5xx *yas5xx)
+{
+ struct yas5xx_calibration *c = &yas5xx->calibration;
+
+ if (yas5xx->version == YAS537_VERSION_1) {
+ dev_dbg(yas5xx->dev, "Cx = %d\n", c->Cx);
+ dev_dbg(yas5xx->dev, "Cy1 = %d\n", c->Cy1);
+ dev_dbg(yas5xx->dev, "Cy2 = %d\n", c->Cy2);
+ dev_dbg(yas5xx->dev, "a2 = %d\n", c->a2);
+ dev_dbg(yas5xx->dev, "a3 = %d\n", c->a3);
+ dev_dbg(yas5xx->dev, "a4 = %d\n", c->a4);
+ dev_dbg(yas5xx->dev, "a5 = %d\n", c->a5);
+ dev_dbg(yas5xx->dev, "a6 = %d\n", c->a6);
+ dev_dbg(yas5xx->dev, "a7 = %d\n", c->a7);
+ dev_dbg(yas5xx->dev, "a8 = %d\n", c->a8);
+ dev_dbg(yas5xx->dev, "a9 = %d\n", c->a9);
+ dev_dbg(yas5xx->dev, "k = %d\n", c->k);
+ }
+}
+
/* Used by YAS530, YAS532 and YAS533 */
static int yas530_set_offsets(struct yas5xx *yas5xx, s8 ox, s8 oy1, s8 oy2)
{
@@ -910,6 +1276,45 @@ static int yas530_power_on(struct yas5xx *yas5xx)
return regmap_write(yas5xx->map, YAS530_MEASURE_INTERVAL, 0);
}
+static int yas537_power_on(struct yas5xx *yas5xx)
+{
+ 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 struct yas5xx_chip_info yas5xx_chip_info_tbl[] = {
[yas530] = {
.devid = YAS530_DEVICE_ID,
@@ -943,6 +1348,17 @@ static struct yas5xx_chip_info yas5xx_chip_info_tbl[] = {
.dump_calibration = yas530_dump_calibration,
.measure_offsets = yas530_measure_offsets,
.power_on = yas530_power_on,
+ },
+ [yas537] = {
+ .devid = YAS537_DEVICE_ID,
+ .volatile_reg = yas537_volatile_reg,
+ .volatile_reg_qty = ARRAY_SIZE(yas537_volatile_reg),
+ .scaling_val2 = 100000, /* nanotesla to Gauss */
+ .get_measure = yas537_get_measure,
+ .get_calibration_data = yas537_get_calibration_data,
+ .dump_calibration = yas537_dump_calibration,
+ /* .measure_offets is not needed for yas537 */
+ .power_on = yas537_power_on,
}
};
@@ -1025,9 +1441,11 @@ static int yas5xx_probe(struct i2c_client *i2c,
if (ret)
goto assert_reset;
- ret = yas5xx->chip_info->measure_offsets(yas5xx);
- if (ret)
- goto assert_reset;
+ if (yas5xx->chip_info->measure_offsets) {
+ ret = yas5xx->chip_info->measure_offsets(yas5xx);
+ if (ret)
+ goto assert_reset;
+ }
indio_dev->info = &yas5xx_info;
indio_dev->available_scan_masks = yas5xx_scan_masks;
@@ -1150,6 +1568,7 @@ static const struct i2c_device_id yas5xx_id[] = {
{"yas530", yas530 },
{"yas532", yas532 },
{"yas533", yas533 },
+ {"yas537", yas537 },
{}
};
MODULE_DEVICE_TABLE(i2c, yas5xx_id);
@@ -1158,6 +1577,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
^ permalink raw reply related [flat|nested] 39+ messages in thread
* Re: [PATCH v4 06/10] iio: magnetometer: yas530: Rename functions and registers
2022-07-03 22:02 ` [PATCH v4 06/10] iio: magnetometer: yas530: Rename functions and registers Jakob Hauser
@ 2022-07-04 18:04 ` Andy Shevchenko
2022-07-26 21:40 ` Jakob Hauser
0 siblings, 1 reply; 39+ messages in thread
From: Andy Shevchenko @ 2022-07-04 18:04 UTC (permalink / raw)
To: Jakob Hauser
Cc: Jonathan Cameron, Lars-Peter Clausen, Linus Walleij,
Hans de Goede, linux-iio, devicetree, phone-devel,
~postmarketos/upstreaming
On Mon, Jul 4, 2022 at 12:03 AM Jakob Hauser <jahau@rocketmail.com> wrote:
>
> This is a preparation for adding YAS537 variant.
>
> Functions that are used only by YAS530, YAS532 and YAS533 are renamed from
> yas5xx to yas530. Same for the registers.
>
> To avoid part listing in function and registers names, the name of the first
> variant is used. Where appropriate, comments were added that these functions
> are used by more than one variant.
>
> Functions that will be used by all variants including YAS537 remain in the
> naming scheme yas5xx. Or YAS5XX for registers, respectively.
...
> /**
> - * yas5xx_measure() - Make a measure from the hardware
> + * yas530_measure() - Make a measure from the hardware
> * @yas5xx: The device state
> * @t: the raw temperature measurement
> * @x: the raw x axis measurement
> @@ -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)
> +/* Used by YAS530, YAS532 and YAS533 */
Why a separate comment and not embedded into the kernel doc above?
> +static int yas530_measure(struct yas5xx *yas5xx, u16 *t, u16 *x, u16 *y1, u16 *y2)
...
> /**
> - * yas5xx_get_measure() - Measure a sample of all axis and process
> + * yas530_get_measure() - Measure a sample of all axis and process
> * @yas5xx: The device state
> * @to: Temperature out
> * @xo: X axis out
> @@ -314,7 +318,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)
> +/* Used by YAS530, YAS532 and YAS533 */
Ditto.
> +static int yas530_get_measure(struct yas5xx *yas5xx, s32 *to, s32 *xo, s32 *yo, s32 *zo)
...
> /**
> - * yas53x_extract_calibration() - extracts the a2-a9 and k calibration
> + * yas530_extract_calibration() - extracts the a2-a9 and k calibration
> * @data: the bitfield to use
> * @c: the calibration to populate
> */
> -static void yas53x_extract_calibration(u8 *data, struct yas5xx_calibration *c)
> +/* Used by YAS530, YAS532 and YAS533 */
Ditto.
> +static void yas530_extract_calibration(u8 *data, struct yas5xx_calibration *c)
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v4 07/10] iio: magnetometer: yas530: Move printk %*ph parameters out from stack
2022-07-03 22:02 ` [PATCH v4 07/10] iio: magnetometer: yas530: Move printk %*ph parameters out from stack Jakob Hauser
@ 2022-07-04 18:06 ` Andy Shevchenko
0 siblings, 0 replies; 39+ messages in thread
From: Andy Shevchenko @ 2022-07-04 18:06 UTC (permalink / raw)
To: Jakob Hauser
Cc: Jonathan Cameron, Lars-Peter Clausen, Linus Walleij,
Hans de Goede, linux-iio, devicetree, phone-devel,
~postmarketos/upstreaming
On Mon, Jul 4, 2022 at 12:03 AM Jakob Hauser <jahau@rocketmail.com> wrote:
>
> Use less stack by modifying %*ph parameters.
>
> Additionally, in the function yas530_get_calibration_data(), the debug dump was
> extended to 16 elements as this is the size of the calibration data array of
> YAS530.
Suggested-by?
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v4 08/10] iio: magnetometer: yas530: Apply documentation and style fixes
2022-07-03 22:02 ` [PATCH v4 08/10] iio: magnetometer: yas530: Apply documentation and style fixes Jakob Hauser
@ 2022-07-04 18:07 ` Andy Shevchenko
2022-07-04 23:29 ` Linus Walleij
1 sibling, 0 replies; 39+ messages in thread
From: Andy Shevchenko @ 2022-07-04 18:07 UTC (permalink / raw)
To: Jakob Hauser
Cc: Jonathan Cameron, Lars-Peter Clausen, Linus Walleij,
Hans de Goede, linux-iio, devicetree, phone-devel,
~postmarketos/upstreaming
On Mon, Jul 4, 2022 at 12:03 AM Jakob Hauser <jahau@rocketmail.com> wrote:
>
> This commit gathers several minor changes.
>
> In the device examples, "Xiaomi" is too generic, specific devices should be
> listed here. E.g. Xiaomi Redmi 2 seems to have YAS537 but it's not fully clear
> if this applies to all its variants. Samsung Galaxy S7 is often quoted in
> conjunction with YAS537.
>
> Removed defines for device IDs of YAS537 and YAS539, they are not needed so far.
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> Signed-off-by: Jakob Hauser <jahau@rocketmail.com>
> ---
> drivers/iio/magnetometer/yamaha-yas530.c | 21 +++++++++++----------
> 1 file changed, 11 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/iio/magnetometer/yamaha-yas530.c b/drivers/iio/magnetometer/yamaha-yas530.c
> index 8358d4b3d1e2..4e2f460a4efd 100644
> --- a/drivers/iio/magnetometer/yamaha-yas530.c
> +++ b/drivers/iio/magnetometer/yamaha-yas530.c
> @@ -10,7 +10,7 @@
> * (YAS534 is a magnetic switch, not handled)
> * YAS535 MS-6C
> * YAS536 MS-3W
> - * YAS537 MS-3T (2015 Samsung Galaxy S6, Note 5, Xiaomi)
> + * YAS537 MS-3T (2015 Samsung Galaxy S6, Note 5, Galaxy S7)
> * YAS539 MS-3S (2018 Samsung Galaxy A7 SM-A750FN)
> *
> * Code functions found in the MPU3050 YAS530 and YAS532 drivers
> @@ -93,10 +93,6 @@
> #define YAS532_DATA_OVERFLOW (BIT(YAS532_DATA_BITS) - 1)
> #define YAS532_20DEGREES 390 /* Counts starting at -50 °C */
>
> -/* These variant IDs are known from code dumps */
> -#define YAS537_DEVICE_ID 0x07 /* YAS537 (MS-3T) */
> -#define YAS539_DEVICE_ID 0x08 /* YAS539 (MS-3S) */
> -
> /* Turn off device regulators etc after 5 seconds of inactivity */
> #define YAS5XX_AUTOSUSPEND_DELAY_MS 5000
>
> @@ -323,7 +319,7 @@ static int yas530_get_measure(struct yas5xx *yas5xx, s32 *to, s32 *xo, s32 *yo,
> {
> struct yas5xx_calibration *c = &yas5xx->calibration;
> u16 t_ref, t, x, y1, y2;
> - /* These are "signed x, signed y1 etc */
> + /* These are signed x, signed y1 etc */
> s32 sx, sy1, sy2, sy, sz;
> int ret;
>
> @@ -663,7 +659,10 @@ static int yas530_get_calibration_data(struct yas5xx *yas5xx)
> return ret;
> dev_dbg(yas5xx->dev, "calibration data: %16ph\n", data);
>
> + /* Contribute calibration data to the input pool for kernel entropy */
> add_device_randomness(data, sizeof(data));
> +
> + /* Extract version */
> yas5xx->version = data[15] & GENMASK(1, 0);
>
> /* Extract the calibration from the bitfield */
> @@ -690,6 +689,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;
> }
>
> @@ -711,12 +711,12 @@ static int yas532_get_calibration_data(struct yas5xx *yas5xx)
> dev_dbg(yas5xx->dev, "calibration data: %14ph\n", data);
>
> /* Sanity check, is this all zeroes? */
> - if (memchr_inv(data, 0x00, 13) == NULL) {
> - if (!(data[13] & BIT(7)))
> - dev_warn(yas5xx->dev, "calibration is blank!\n");
> - }
> + if (!memchr_inv(data, 0x00, 13) && !(data[13] & BIT(7)))
> + dev_warn(yas5xx->dev, "calibration is blank!\n");
>
> + /* Contribute calibration data to the input pool for kernel entropy */
> add_device_randomness(data, sizeof(data));
> +
> /* Only one bit of version info reserved here as far as we know */
> yas5xx->version = data[13] & BIT(0);
>
> @@ -725,6 +725,7 @@ static int yas532_get_calibration_data(struct yas5xx *yas5xx)
> c->Cy1 = data[1] * 10 - 1280;
> c->Cy2 = data[2] * 10 - 1280;
> yas530_extract_calibration(&data[3], c);
> +
> /*
> * Extract linearization:
> * Linearization layout in the 32 bits at byte 10:
> --
> 2.35.1
>
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v4 09/10] iio: magnetometer: yas530: Introduce "chip_info" structure
2022-07-03 22:04 ` [PATCH v4 09/10] iio: magnetometer: yas530: Introduce "chip_info" structure Jakob Hauser
@ 2022-07-04 19:37 ` Andy Shevchenko
2022-07-16 17:10 ` Jonathan Cameron
2022-07-26 22:01 ` Jakob Hauser
0 siblings, 2 replies; 39+ messages in thread
From: Andy Shevchenko @ 2022-07-04 19:37 UTC (permalink / raw)
To: Jakob Hauser
Cc: Jonathan Cameron, Lars-Peter Clausen, Linus Walleij,
Hans de Goede, linux-iio, devicetree, phone-devel,
~postmarketos/upstreaming
On Mon, Jul 4, 2022 at 12:04 AM Jakob Hauser <jahau@rocketmail.com> wrote:
>
> This commit introduces the "chip_info" structure approach for better variant
> handling.
>
> The variant to be used is now chosen by the devicetree (enum "chip_ids"),
Device Tree
> not by the chip ID in the register. However, there is a check to make sure
> they match (using integer "id_check").
...
Thanks for a new version, it's getting better. My comments below.
But first of all, can you split this to at least two patches, i.e.
1) split out functions without introducing chip->info yet;
2) adding chip_info.
Possible alternative would be more steps in 2), i.e. introducing
chip_info for the callback only, then add field (or semantically
unified fields) by field with corresponding changes in the code. In
this case it would be easier to review.
I leave this exercise to you if Jonathan thinks it worth it.
...
> -#define YAS530_20DEGREES 182 /* Counts starting at -62 °C */
> -#define YAS532_20DEGREES 390 /* Counts starting at -50 °C */
The comments suddenly disappear from the file. See below.
...
> +enum chip_ids {
> + yas530,
> + yas532,
> + yas533,
> +};
> +
> +static const char yas5xx_product_name[][13] = {
> + "YAS530 MS-3E",
> + "YAS532 MS-3R",
> + "YAS533 MS-3F"
> +};
> +
> +static const char yas5xx_version_name[][2][3] = {
> + { "A", "B" },
> + { "AB", "AC" },
> + { "AB", "AC" }
Shan't we put indices here?
Also, use * instead of one dimension of array.
> +};
...
> +static const int yas530_volatile_reg[] = {
> + YAS530_ACTUATE_INIT_COIL,
> + YAS530_MEASURE
+ Comma.
> +};
...
> +/* Number of counts between minimum and reference temperature */
> +const u16 t_ref_counts[] = { 182, 390, 390 };
> +
> +/* Starting point of temperature counting in 1/10:s degrees Celsius */
> +const s16 min_temp_celcius_x10[] = { -620, -500, -500 };
See above.
...
> +struct yas5xx_chip_info {
> + unsigned int devid;
> + const int *volatile_reg;
> + const int volatile_reg_qty;
> + const u32 scaling_val2;
Why const here?
I assume entire structure is const, no?
> + int (*get_measure)(struct yas5xx *yas5xx, s32 *to, s32 *xo, s32 *yo, s32 *zo);
> + int (*get_calibration_data)(struct yas5xx *yas5xx);
> + void (*dump_calibration)(struct yas5xx *yas5xx);
> + int (*measure_offsets)(struct yas5xx *yas5xx);
> + int (*power_on)(struct yas5xx *yas5xx);
> +};
...
> + int i, j;
j can have a proper name.
> + j = yas5xx->chip_info->volatile_reg_qty;
...
> +static struct yas5xx_chip_info yas5xx_chip_info_tbl[] = {
> + [yas530] = {
> + .devid = YAS530_DEVICE_ID,
> + .volatile_reg = yas530_volatile_reg,
> + .volatile_reg_qty = ARRAY_SIZE(yas530_volatile_reg),
> + .scaling_val2 = 100000000, /* picotesla to Gauss */
> + .get_measure = yas530_get_measure,
> + .get_calibration_data = yas530_get_calibration_data,
> + .dump_calibration = yas530_dump_calibration,
> + .measure_offsets = yas530_measure_offsets,
> + .power_on = yas530_power_on,
> + },
> + [yas532] = {
> + .devid = YAS532_DEVICE_ID,
> + .volatile_reg = yas530_volatile_reg,
> + .volatile_reg_qty = ARRAY_SIZE(yas530_volatile_reg),
> + .scaling_val2 = 100000, /* nanotesla to Gauss */
> + .get_measure = yas530_get_measure,
> + .get_calibration_data = yas532_get_calibration_data,
> + .dump_calibration = yas530_dump_calibration,
> + .measure_offsets = yas530_measure_offsets,
> + .power_on = yas530_power_on,
> + },
> + [yas533] = {
> + .devid = YAS532_DEVICE_ID,
> + .volatile_reg = yas530_volatile_reg,
> + .volatile_reg_qty = ARRAY_SIZE(yas530_volatile_reg),
> + .scaling_val2 = 100000, /* nanotesla to Gauss */
> + .get_measure = yas530_get_measure,
> + .get_calibration_data = yas532_get_calibration_data,
> + .dump_calibration = yas530_dump_calibration,
> + .measure_offsets = yas530_measure_offsets,
> + .power_on = yas530_power_on,
> + }
Keep comma here.
> +};
...
> - int ret;
> + int id_check, ret;
Don't add variables with different semantics on the same line.
...
> + if (id_check != yas5xx->chip_info->devid) {
> ret = -ENODEV;
> - dev_err(dev, "unhandled device ID %02x\n", yas5xx->devid);
> + dev_err(dev, "device ID %02x doesn't match %s\n",
> + id_check, id->name);
ret = dev_err_probe() ?
> goto assert_reset;
> }
...
> + dev_info(dev, "detected %s %s\n", yas5xx_product_name[yas5xx->chip],
> + yas5xx_version_name[yas5xx->chip][yas5xx->version]);
I'm wondering if these arrays can be actually embedded into chip_info?
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v4 10/10] iio: magnetometer: yas530: Add YAS537 variant
2022-07-03 22:05 ` [PATCH v4 10/10] iio: magnetometer: yas530: Add YAS537 variant Jakob Hauser
@ 2022-07-04 19:47 ` Andy Shevchenko
2022-07-26 22:13 ` Jakob Hauser
0 siblings, 1 reply; 39+ messages in thread
From: Andy Shevchenko @ 2022-07-04 19:47 UTC (permalink / raw)
To: Jakob Hauser
Cc: Jonathan Cameron, Lars-Peter Clausen, Linus Walleij,
Hans de Goede, linux-iio, devicetree, phone-devel,
~postmarketos/upstreaming
On Mon, Jul 4, 2022 at 12:05 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.
>
> [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
Much better, my comments below.
...
> yas530,
> yas532,
> yas533,
> + yas537,
> };
>
> static const char yas5xx_product_name[][13] = {
> "YAS530 MS-3E",
> "YAS532 MS-3R",
> - "YAS533 MS-3F"
> + "YAS533 MS-3F",
This is exactly the point why it's good practice to add comma for
non-terminator entries.
> + "YAS537 MS-3T"
...here.
> };
>
> static const char yas5xx_version_name[][2][3] = {
> { "A", "B" },
> { "AB", "AC" },
> - { "AB", "AC" }
> + { "AB", "AC" },
Ditto.
> + { "v0", "v1" }
> };
...
> + /* Write registers according to Android driver */
Would be nice to elaborate in the comment what exactly the flow is
here, like "a) setting value of X; b) ...".
...
> + 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;
Can bulk write be used here?
...
> + /* The interval value is static in regular operation */
> + intrvl = (YAS537_DEFAULT_SENSOR_DELAY_MS * 1000
MILLI ?
> + - YAS537_MEASURE_TIME_WORST_US) / 4100;
...
> + },
> }
And this is for chip_info and comma for non-terminator entries (see above).
...
> - ret = yas5xx->chip_info->measure_offsets(yas5xx);
> - if (ret)
> - goto assert_reset;
> + if (yas5xx->chip_info->measure_offsets) {
This can be done when you introduce this callback in the chip_info
structure, so this patch won't need to shuffle code again. I.o.w. we
can reduce ping-pong development in this series.
> + ret = yas5xx->chip_info->measure_offsets(yas5xx);
> + if (ret)
> + goto assert_reset;
> + }
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v4 08/10] iio: magnetometer: yas530: Apply documentation and style fixes
2022-07-03 22:02 ` [PATCH v4 08/10] iio: magnetometer: yas530: Apply documentation and style fixes Jakob Hauser
2022-07-04 18:07 ` Andy Shevchenko
@ 2022-07-04 23:29 ` Linus Walleij
1 sibling, 0 replies; 39+ messages in thread
From: Linus Walleij @ 2022-07-04 23:29 UTC (permalink / raw)
To: Jakob Hauser
Cc: Jonathan Cameron, Lars-Peter Clausen, Andy Shevchenko,
Hans de Goede, linux-iio, devicetree, phone-devel,
~postmarketos/upstreaming
On Mon, Jul 4, 2022 at 12:03 AM Jakob Hauser <jahau@rocketmail.com> wrote:
> This commit gathers several minor changes.
>
> In the device examples, "Xiaomi" is too generic, specific devices should be
> listed here. E.g. Xiaomi Redmi 2 seems to have YAS537 but it's not fully clear
> if this applies to all its variants. Samsung Galaxy S7 is often quoted in
> conjunction with YAS537.
>
> Removed defines for device IDs of YAS537 and YAS539, they are not needed so far.
>
> Signed-off-by: Jakob Hauser <jahau@rocketmail.com>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v4 00/10] Add support for magnetometer Yamaha YAS537
2022-07-03 22:02 ` [PATCH v4 00/10] Add support for magnetometer Yamaha YAS537 Jakob Hauser
` (9 preceding siblings ...)
2022-07-03 22:05 ` [PATCH v4 10/10] iio: magnetometer: yas530: Add YAS537 variant Jakob Hauser
@ 2022-07-04 23:31 ` Linus Walleij
2022-07-16 17:05 ` Jonathan Cameron
10 siblings, 1 reply; 39+ messages in thread
From: Linus Walleij @ 2022-07-04 23:31 UTC (permalink / raw)
To: Jakob Hauser
Cc: Jonathan Cameron, Lars-Peter Clausen, Andy Shevchenko,
Hans de Goede, linux-iio, devicetree, phone-devel,
~postmarketos/upstreaming
On Mon, Jul 4, 2022 at 12:03 AM Jakob Hauser <jahau@rocketmail.com> wrote:
> 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-9 are cleanups and refactoring.
> Patch 10 finally adds the YAS537 variant.
This patch set is really nice and getting nicer.
Maybe Jonathan could apply patches 1-5 so you don't have to
resend so much code and get more focus on the top 5 patches?
They are anyway nice in their own right.
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v4 00/10] Add support for magnetometer Yamaha YAS537
2022-07-04 23:31 ` [PATCH v4 00/10] Add support for magnetometer Yamaha YAS537 Linus Walleij
@ 2022-07-16 17:05 ` Jonathan Cameron
0 siblings, 0 replies; 39+ messages in thread
From: Jonathan Cameron @ 2022-07-16 17:05 UTC (permalink / raw)
To: Linus Walleij
Cc: Jakob Hauser, Lars-Peter Clausen, Andy Shevchenko, Hans de Goede,
linux-iio, devicetree, phone-devel, ~postmarketos/upstreaming
On Tue, 5 Jul 2022 01:31:48 +0200
Linus Walleij <linus.walleij@linaro.org> wrote:
> On Mon, Jul 4, 2022 at 12:03 AM Jakob Hauser <jahau@rocketmail.com> wrote:
>
> > 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-9 are cleanups and refactoring.
> > Patch 10 finally adds the YAS537 variant.
>
> This patch set is really nice and getting nicer.
>
> Maybe Jonathan could apply patches 1-5 so you don't have to
> resend so much code and get more focus on the top 5 patches?
> They are anyway nice in their own right.
>
Given I'm running way behind (at least I'm in the right month now ;)
and my tree isn't stable currently (I'll rebase after rc1) and only
pushed out as testing as a result that isn't a good idea right now.
It would be at any other timing though.
So, please keep all the patches for v5.
Thanks,
Jonathan
> Yours,
> Linus Walleij
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v4 09/10] iio: magnetometer: yas530: Introduce "chip_info" structure
2022-07-04 19:37 ` Andy Shevchenko
@ 2022-07-16 17:10 ` Jonathan Cameron
2022-07-26 22:01 ` Jakob Hauser
1 sibling, 0 replies; 39+ messages in thread
From: Jonathan Cameron @ 2022-07-16 17:10 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Jakob Hauser, Lars-Peter Clausen, Linus Walleij, Hans de Goede,
linux-iio, devicetree, phone-devel, ~postmarketos/upstreaming
On Mon, 4 Jul 2022 21:37:30 +0200
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> On Mon, Jul 4, 2022 at 12:04 AM Jakob Hauser <jahau@rocketmail.com> wrote:
> >
> > This commit introduces the "chip_info" structure approach for better variant
> > handling.
> >
> > The variant to be used is now chosen by the devicetree (enum "chip_ids"),
>
> Device Tree
>
> > not by the chip ID in the register. However, there is a check to make sure
> > they match (using integer "id_check").
>
> ...
>
> Thanks for a new version, it's getting better. My comments below.
>
> But first of all, can you split this to at least two patches, i.e.
> 1) split out functions without introducing chip->info yet;
> 2) adding chip_info.
>
> Possible alternative would be more steps in 2), i.e. introducing
> chip_info for the callback only, then add field (or semantically
> unified fields) by field with corresponding changes in the code. In
> this case it would be easier to review.
>
> I leave this exercise to you if Jonathan thinks it worth it.
You are of course correct that it would be nicer to have it split, but
I'm not going to be fussy about it this time ;)
Other than addressing Andy's eagle eyed review comments, this series looks good to me.
Thanks,
Jonathan
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v4 06/10] iio: magnetometer: yas530: Rename functions and registers
2022-07-04 18:04 ` Andy Shevchenko
@ 2022-07-26 21:40 ` Jakob Hauser
0 siblings, 0 replies; 39+ messages in thread
From: Jakob Hauser @ 2022-07-26 21:40 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Jonathan Cameron, Lars-Peter Clausen, Linus Walleij,
Hans de Goede, linux-iio, devicetree, phone-devel,
~postmarketos/upstreaming
Hi Andy,
On 04.07.22 20:04, Andy Shevchenko wrote:
> On Mon, Jul 4, 2022 at 12:03 AM Jakob Hauser <jahau@rocketmail.com> wrote:
>>
...
>> /**
>> - * yas5xx_measure() - Make a measure from the hardware
>> + * yas530_measure() - Make a measure from the hardware
>> * @yas5xx: The device state
>> * @t: the raw temperature measurement
>> * @x: the raw x axis measurement
>> @@ -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)
>> +/* Used by YAS530, YAS532 and YAS533 */
>
> Why a separate comment and not embedded into the kernel doc above?
>
I thought of it as an internal comment or hint. I'm not familiar with
kernel doc and didn't dare to add it there.
I'll move it to the bottom of the kernel doc part.
Kind regards,
Jakob
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v4 09/10] iio: magnetometer: yas530: Introduce "chip_info" structure
2022-07-04 19:37 ` Andy Shevchenko
2022-07-16 17:10 ` Jonathan Cameron
@ 2022-07-26 22:01 ` Jakob Hauser
2022-07-27 17:39 ` Andy Shevchenko
1 sibling, 1 reply; 39+ messages in thread
From: Jakob Hauser @ 2022-07-26 22:01 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Jonathan Cameron, Lars-Peter Clausen, Linus Walleij,
Hans de Goede, linux-iio, devicetree, phone-devel,
~postmarketos/upstreaming
Hi Andy,
On 04.07.22 21:37, Andy Shevchenko wrote:
> On Mon, Jul 4, 2022 at 12:04 AM Jakob Hauser <jahau@rocketmail.com> wrote:
>>
...
> Thanks for a new version, it's getting better. My comments below.
>
> But first of all, can you split this to at least two patches, i.e.
> 1) split out functions without introducing chip->info yet;
> 2) adding chip_info.
The only function that's split out newly is yas5xx_calc_temperature().
However, it uses "yas5xx->chip" to look up the right values in the array
for "t_ref" and "min_temp_x10". So it cannot be easily split from the
introduction to the chip_info approach.
> Possible alternative would be more steps in 2), i.e. introducing
> chip_info for the callback only, then add field (or semantically
> unified fields) by field with corresponding changes in the code. In
> this case it would be easier to review.
What do you mean by "introducing chip_info for the callback only"? I
guess you mean the function pointers?
According to this, the patch could be split into:
1) introduce function pointers
2) introduce arrays to look up values
However, what's missing there is the introduction of the chip_info
approach as such. Admittedly difficult to follow are the changes in
yas5xx_probe():
- yas5xx->devid gets removed, it was the main switch decider before
- (for usage on other switches it is moved to yas5xx->chip_info->devid)
- yas5xx->chip as an enumeration is newly introduced
- yas5xx->chip_info as a structure gets initiated
- As the chip_info now chooses according to the i2c_device_id (via
enum chip_ids, thus yas5xx->chip), a new check is introduced to
make sure it matches the device ID read from the register. This
is done by "if (id_check != yas5xx->chip_info->devid)".
- The way of reporting product name and version name was changed
because introducing chip_info required to do this in one place
for all versions.
- As a consequence of this, yas5xx->name became obsolete.
This would have to be done before introducing function pointers and
arrays, like:
1) introduce chip_info structural changes
2) add function pointers
3) add arrays to look up values
But it can't be easily split that way. I can't establish an "empty"
chip_info approach as a fist step without removing or changing many
other things. The structural changes are related to other changes in
that patch.
I'm thinking about first introducing chip_info and thereafter
establishing the function pointers. In between I could try to split out
the new temperature function:
1) introduce chip_info structural changes incl. arrays to look up values
2) split out new yas5xx_calc_temperature() function
3) add function pointers
I'm not yet sure it can be split that way because things are entangled.
But I will try to this in v5.
Btw, looking through this, I realized that in patch 6 "Rename functions
and registers" I unfortunately missed to rename instances of
"yas5xx_get_measure", "yas5xx_power_on", "YAS5XX_ACTUATE_INIT_COIL" and
"YAS5XX_MEASURE". I'll correct this in v5.
...
>> -#define YAS530_20DEGREES 182 /* Counts starting at -62 °C */
>
>> -#define YAS532_20DEGREES 390 /* Counts starting at -50 °C */
>
> The comments suddenly disappear from the file. See below.
The comment's didn't actually disappear but rather got restructured by
introducing new chip_info approach. See below.
>> +enum chip_ids {
>> + yas530,
>> + yas532,
>> + yas533,
>> +};
>> +
>> +static const char yas5xx_product_name[][13] = {
>> + "YAS530 MS-3E",
>> + "YAS532 MS-3R",
>> + "YAS533 MS-3F"
>> +};
>> +
>> +static const char yas5xx_version_name[][2][3] = {
>> + { "A", "B" },
>> + { "AB", "AC" },
>> + { "AB", "AC" }
>
> Shan't we put indices here?
Do you mean:
static const char yas5xx_version_name[][2][3] = {
[yas530] = { "A", "B" },
[yas532] = { "AB", "AC" },
[yas533] = { "AB", "AC" },
};
I can add this.
> Also, use * instead of one dimension of array.
Sorry, probably I lack the basic knowledge here. Can you explain how to
do that?
...
>> +static const int yas530_volatile_reg[] = {
>> + YAS530_ACTUATE_INIT_COIL,
>> + YAS530_MEASURE
>
> + Comma.
OK, I didn't know this is allowed. I'll add it here and at the other
locations.
>> +/* Number of counts between minimum and reference temperature */
>> +const u16 t_ref_counts[] = { 182, 390, 390 };
>> +
>> +/* Starting point of temperature counting in 1/10:s degrees Celsius */
>> +const s16 min_temp_celcius_x10[] = { -620, -500, -500 };
>
> See above.
The former comments...
182 /* Counts starting at -62 °C */
390 /* Counts starting at -50 °C */
... and the two new comments above the arrays actually should say the
same thing. At least that was my intention.
The first value is a number of counts. And the counting starts at a
certain temperature, which is the second value. Both the old and the new
comments are intended to describe this.
In the introduction of this temperature handling (patch 4), there is a
lot more description on these values in function yas5xx_get_measure().
When creating the new "chip_info" patch 9, I was thinking about moving
some of that description up here to these arrays. However, instead I
tried to following Jonathan's suggestion in v3 to keep the describing
text low and rather let the formulas speak for themselves. These values
are applied at a formula in function yas5xx_calc_temperature() which is
supposed to by kind of self explanatory.
What may lead to confusion is the equivalent usage of "starting" and
"minimum" here. In the initial patchset I used "starting" related to the
counts, Jonathan suggested "minimum" or actually "min_temp" related to
the temperature. The comments here above are bit of a mixture of both. I
still think it's good enough to understand. The sentence "Starting point
of temperature ..." describes the value min_temp_celcius_x10. Using a
term like "starting temperature" would probably be more confusing.
>> +struct yas5xx_chip_info {
>> + unsigned int devid;
>
>> + const int *volatile_reg;
>> + const int volatile_reg_qty;
>> + const u32 scaling_val2;
>
> Why const here?
> I assume entire structure is const, no?
I'm rather new to C language, not having a good grasp of "const" in
structures yet. I would have guessed it doesn't work well with the
function pointers.
However, having some compile tests, there are no complaints about the
function pointers.
To change the whole chip_info structure to "const", I would:
- within the "struct yas5xx" change to "const struct
yas5xx_chip_info *chip_info;"
- change following typedef to "static const struct
yas5xx_chip_info yas5xx_chip_info_tbl[] = ..."
Then, within the "struct yas5xx_chip_info", I can remove "const" from:
- int volatile_reg_qty;
- u32 scaling_val2;
However, I have to keep "const" at the following, otherwise the complier
complains that "initialization discards 'const' qualifier from pointer
target type":
- const int *volatile_reg;
Summarizing, after the changes it would look like the following (snippets):
struct yas5xx_chip_info {
unsigned int devid;
const int *volatile_reg;
int volatile_reg_qty;
u32 scaling_val2;
int (*get_measure)(struct yas5xx *yas5xx, s32 *to,
s32 *xo, s32 *yo, s32 *zo);
int (*get_calibration_data)(struct yas5xx *yas5xx);
void (*dump_calibration)(struct yas5xx *yas5xx);
int (*measure_offsets)(struct yas5xx *yas5xx);
int (*power_on)(struct yas5xx *yas5xx);
};
struct yas5xx {
struct device *dev;
enum chip_ids chip;
const struct yas5xx_chip_info *chip_info;
...
};
static const struct yas5xx_chip_info yas5xx_chip_info_tbl[] = {
[yas530] = {
...
},
};
If that's reasonable, I'll change it that way.
...
>> + int i, j;
>
> j can have a proper name.
Ok, makes sense. I'll probably name it "reg_qty".
>> + j = yas5xx->chip_info->volatile_reg_qty;
>
...
>> - int ret;
>> + int id_check, ret;
>
> Don't add variables with different semantics on the same line.
Ok. I wasn't aware of putting different semantics on different lines,
thanks for the hint, makes sense.
>> + if (id_check != yas5xx->chip_info->devid) {
>> ret = -ENODEV;
>> - dev_err(dev, "unhandled device ID %02x\n", yas5xx->devid);
>> + dev_err(dev, "device ID %02x doesn't match %s\n",
>> + id_check, id->name);
>
> ret = dev_err_probe() ?
Makes sense, will change that.
Though I have difficulties to implement this nicely. dev_err_probe()
requires an "error value to test". The current "if (id_check !=
yas5xx->chip_info->devid)" doesn't offer such a value by itself.
Do you think the following would be appropriate, nesting the check
within the dev_err_probe()? It doesn't look too good to me.
ret = dev_err_probe(dev, id_check != yas5xx->chip_info->devid,
"device ID %02x doesn't match %s\n",
id_check, id->name);
if (ret)
goto assert_reset;
If there are no better ideas, I would implement it that way.
Additionally adding a comment and putting it into a block with the
regmap_read before:
/*
* Check if the device ID from the register matches the one set
* in the Device Tree.
*/
ret = regmap_read(yas5xx->map, YAS5XX_DEVICE_ID, &id_check);
if (ret)
goto assert_reset;
ret = dev_err_probe(dev, id_check != yas5xx->chip_info->devid,
"device ID %02x doesn't match %s\n",
id_check, id->name);
if (ret)
goto assert_reset;
Hm, I think it's a bit hard to read. Suggestions for improvement are
welcome. Otherwise I'd add it that way.
...
>> + dev_info(dev, "detected %s %s\n", yas5xx_product_name[yas5xx->chip],
>> + yas5xx_version_name[yas5xx->chip][yas5xx->version]);
>
> I'm wondering if these arrays can be actually embedded into chip_info?
While the variants (like "YAS530") are listed in the chip_info, the
versions (like "A") are not. Yet, including the versions in chip_info
would double the amount, making it visually more unclear, with only
minor benefit.
The first part of this call, the "product name", applies to the
variants. Going the detour via chip_info, the array element to call
could just be hard-coded in the chip_info table, like:
static struct yas5xx_chip_info yas5xx_chip_info_tbl[] = {
[yas530] = {
...
.product_name = yas5xx_product_name[0],
...
The second part, the "version name", is currently in the
three-dimensional array yas5xx_version_name[]. The first dimension is
the variant, this would need to be hard-coded in the chip_info table,
similar to the example above. The second dimension is the version, this
would need to come from within the yas5xx_probe() the function. I
couldn't find a way how to assign one dimension in one place and another
dimension in another place.
To include the second part in the chip_info, I would split the
three-dimensional array yas5xx_version_name[] into three separate
two-dimensional arrays, one per variant. It would look like this (snippets):
static const char yas530_version_name[][3] = { "A", "B" };
static const char yas532_version_name[][3] = { "AB", "AC" };
static const char yas533_version_name[][3] = { "AB", "AC" };
...
struct yas5xx_chip_info {
...
const char *product_name;
const char (*version_name)[3];
...
...
static struct yas5xx_chip_info yas5xx_chip_info_tbl[] = {
[yas530] = {
...
.product_name = yas5xx_product_name[0],
.version_name = yas530_version_name,
...
...
dev_info(dev, "detected %s %s\n",
yas5xx->chip_info->product_name,
yas5xx->chip_info->version_name[yas5xx->version]);
I'm not fully sure this is the best way. Also note that I had to set the
size of the second dimension of yas530_version_name to 3.
Do you think I should do it this way to include "product_name" and
"version_name" in the chip_info?
If yes, should I probably do a similar thing for the values
"t_ref_counts" and "min_temp_celcius_x10"? Here as well, the array
elements are directly called in function yas5xx_calc_temperature()
without using the chip_info structure.
Also I noticed that I should add "static" to the typedef of arrays
"t_ref_counts" and "min_temp_celcius_x10". Darn, and I have to correct
the spelling of "celcius" into "celsius" in "min_temp_celcius_x10".
Kind regards,
Jakob
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v4 10/10] iio: magnetometer: yas530: Add YAS537 variant
2022-07-04 19:47 ` Andy Shevchenko
@ 2022-07-26 22:13 ` Jakob Hauser
2022-07-27 17:46 ` Andy Shevchenko
0 siblings, 1 reply; 39+ messages in thread
From: Jakob Hauser @ 2022-07-26 22:13 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Jonathan Cameron, Lars-Peter Clausen, Linus Walleij,
Hans de Goede, linux-iio, devicetree, phone-devel,
~postmarketos/upstreaming
Hi Andy,
On 04.07.22 21:47, Andy Shevchenko wrote:
> On Mon, Jul 4, 2022 at 12:05 AM Jakob Hauser <jahau@rocketmail.com> wrote:
>>
...
>> yas530,
>> yas532,
>> yas533,
>> + yas537,
>> };
>>
>> static const char yas5xx_product_name[][13] = {
>> "YAS530 MS-3E",
>> "YAS532 MS-3R",
>> - "YAS533 MS-3F"
>> + "YAS533 MS-3F",
>
> This is exactly the point why it's good practice to add comma for
> non-terminator entries.
>
>> + "YAS537 MS-3T"
>
> ..here.
>
>> };
Yes, makes sense.
...
>> + /* Write registers according to Android driver */
>
> Would be nice to elaborate in the comment what exactly the flow is
> here, like "a) setting value of X; b) ...".
Unfortunately, I don't know more than what the code already says. In the
Yamaha Android driver, this part looks like this:
https://github.com/msm8916-mainline/android_kernel_qcom_msm8916/blob/GT-I9195I/drivers/iio/magnetometer/yas_mag_drv-yas537.c#L345-L350
The comment "Write registers according to Android driver" actually says
"I don't know what I'm doing here, this is copy-paste from Android".
I can remove the comment if you find it inappropriate. Though from my
point of view the comment is ok.
...
>> + 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;
>
> Can bulk write be used here?
Technically yes. But in this case I strongly would like to keep the
single regmap_write as we go through different registers step by step
and write them. That way it's much better readable. And it's just these
two that are neighboring each other. As this happens in
yas537_power_on(), it isn't done very often, thus doesn't cost much
resources.
...
>> + /* The interval value is static in regular operation */
>> + intrvl = (YAS537_DEFAULT_SENSOR_DELAY_MS * 1000
>
> MILLI ?
What do you mean by this comment?
The suffixes _MS and _US were proposed by you in v2. I think they are fine.
...
>> - ret = yas5xx->chip_info->measure_offsets(yas5xx);
>> - if (ret)
>> - goto assert_reset;
>
>> + if (yas5xx->chip_info->measure_offsets) {
>
> This can be done when you introduce this callback in the chip_info
> structure, so this patch won't need to shuffle code again. I.o.w. we
> can reduce ping-pong development in this series.
I did this change in this patch on purpose because it's the introduction
of YAS537 variant that's causing this change. YAS537 is the first
variant that doesn't use yas5xx->chip_info->measure_offsets.
Shall I move it to patch 9 nonetheless?
>> + ret = yas5xx->chip_info->measure_offsets(yas5xx);
>> + if (ret)
>> + goto assert_reset;
>> + }
>
Kind regards,
Jakob
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v4 09/10] iio: magnetometer: yas530: Introduce "chip_info" structure
2022-07-26 22:01 ` Jakob Hauser
@ 2022-07-27 17:39 ` Andy Shevchenko
2022-07-28 23:05 ` Jakob Hauser
0 siblings, 1 reply; 39+ messages in thread
From: Andy Shevchenko @ 2022-07-27 17:39 UTC (permalink / raw)
To: Jakob Hauser
Cc: Jonathan Cameron, Lars-Peter Clausen, Linus Walleij,
Hans de Goede, linux-iio, devicetree, phone-devel,
~postmarketos/upstreaming
On Wed, Jul 27, 2022 at 12:01 AM Jakob Hauser <jahau@rocketmail.com> wrote:
> On 04.07.22 21:37, Andy Shevchenko wrote:
> > On Mon, Jul 4, 2022 at 12:04 AM Jakob Hauser <jahau@rocketmail.com> wrote:
...
> > Possible alternative would be more steps in 2), i.e. introducing
> > chip_info for the callback only, then add field (or semantically
> > unified fields) by field with corresponding changes in the code. In
> > this case it would be easier to review.
>
> What do you mean by "introducing chip_info for the callback only"? I
> guess you mean the function pointers?
Yes.
> According to this, the patch could be split into:
> 1) introduce function pointers
> 2) introduce arrays to look up values
>
> However, what's missing there is the introduction of the chip_info
> approach as such. Admittedly difficult to follow are the changes in
> yas5xx_probe():
> - yas5xx->devid gets removed, it was the main switch decider before
> - (for usage on other switches it is moved to yas5xx->chip_info->devid)
> - yas5xx->chip as an enumeration is newly introduced
> - yas5xx->chip_info as a structure gets initiated
> - As the chip_info now chooses according to the i2c_device_id (via
> enum chip_ids, thus yas5xx->chip), a new check is introduced to
> make sure it matches the device ID read from the register. This
> is done by "if (id_check != yas5xx->chip_info->devid)".
> - The way of reporting product name and version name was changed
> because introducing chip_info required to do this in one place
> for all versions.
> - As a consequence of this, yas5xx->name became obsolete.
>
> This would have to be done before introducing function pointers and
> arrays, like:
> 1) introduce chip_info structural changes
> 2) add function pointers
> 3) add arrays to look up values
>
> But it can't be easily split that way. I can't establish an "empty"
> chip_info approach as a fist step without removing or changing many
> other things. The structural changes are related to other changes in
> that patch.
>
> I'm thinking about first introducing chip_info and thereafter
> establishing the function pointers. In between I could try to split out
> the new temperature function:
> 1) introduce chip_info structural changes incl. arrays to look up values
> 2) split out new yas5xx_calc_temperature() function
> 3) add function pointers
>
> I'm not yet sure it can be split that way because things are entangled.
> But I will try to this in v5.
Yes, my main point is to try to see under different angles on how you
can split the series. We have plenty of time.
> Btw, looking through this, I realized that in patch 6 "Rename functions
> and registers" I unfortunately missed to rename instances of
> "yas5xx_get_measure", "yas5xx_power_on", "YAS5XX_ACTUATE_INIT_COIL" and
> "YAS5XX_MEASURE". I'll correct this in v5.
Also my point why smaller changes are better.
...
> >> -#define YAS530_20DEGREES 182 /* Counts starting at -62 °C */
> >
> >> -#define YAS532_20DEGREES 390 /* Counts starting at -50 °C */
> >
> > The comments suddenly disappear from the file. See below.
>
> The comment's didn't actually disappear but rather got restructured by
> introducing new chip_info approach. See below.
See below.
...
> >> +static const char yas5xx_version_name[][2][3] = {
> >> + { "A", "B" },
> >> + { "AB", "AC" },
> >> + { "AB", "AC" }
> >
> > Shan't we put indices here?
>
> Do you mean:
>
> static const char yas5xx_version_name[][2][3] = {
> [yas530] = { "A", "B" },
> [yas532] = { "AB", "AC" },
> [yas533] = { "AB", "AC" },
> };
>
> I can add this.
Yes.
> > Also, use * instead of one dimension of array.
>
> Sorry, probably I lack the basic knowledge here. Can you explain how to
> do that?
static const char *_name[][] = {
};
?
...
> >> +/* Number of counts between minimum and reference temperature */
> >> +const u16 t_ref_counts[] = { 182, 390, 390 };
> >> +
> >> +/* Starting point of temperature counting in 1/10:s degrees Celsius */
> >> +const s16 min_temp_celcius_x10[] = { -620, -500, -500 };
> >
> > See above.
>
> The former comments...
> 182 /* Counts starting at -62 °C */
> 390 /* Counts starting at -50 °C */
>
> ... and the two new comments above the arrays actually should say the
> same thing. At least that was my intention.
>
> The first value is a number of counts. And the counting starts at a
> certain temperature, which is the second value. Both the old and the new
> comments are intended to describe this.
>
> In the introduction of this temperature handling (patch 4), there is a
> lot more description on these values in function yas5xx_get_measure().
> When creating the new "chip_info" patch 9, I was thinking about moving
> some of that description up here to these arrays. However, instead I
> tried to following Jonathan's suggestion in v3 to keep the describing
> text low and rather let the formulas speak for themselves. These values
> are applied at a formula in function yas5xx_calc_temperature() which is
> supposed to by kind of self explanatory.
>
> What may lead to confusion is the equivalent usage of "starting" and
> "minimum" here. In the initial patchset I used "starting" related to the
> counts, Jonathan suggested "minimum" or actually "min_temp" related to
> the temperature. The comments here above are bit of a mixture of both. I
> still think it's good enough to understand. The sentence "Starting point
> of temperature ..." describes the value min_temp_celcius_x10. Using a
> term like "starting temperature" would probably be more confusing.
Confusing to me, who doesn't know the specifics of the chip, it is
easy to read '-62 °C' vs. "read comment, look into an array, calculate
in your brain".
...
> >> +struct yas5xx_chip_info {
> >> + unsigned int devid;
> >
> >> + const int *volatile_reg;
> >> + const int volatile_reg_qty;
> >> + const u32 scaling_val2;
> >
> > Why const here?
> > I assume entire structure is const, no?
>
> I'm rather new to C language, not having a good grasp of "const" in
> structures yet. I would have guessed it doesn't work well with the
> function pointers.
>
> However, having some compile tests, there are no complaints about the
> function pointers.
>
> To change the whole chip_info structure to "const", I would:
> - within the "struct yas5xx" change to "const struct
> yas5xx_chip_info *chip_info;"
> - change following typedef to "static const struct
> yas5xx_chip_info yas5xx_chip_info_tbl[] = ..."
>
> Then, within the "struct yas5xx_chip_info", I can remove "const" from:
> - int volatile_reg_qty;
> - u32 scaling_val2;
>
> However, I have to keep "const" at the following, otherwise the complier
> complains that "initialization discards 'const' qualifier from pointer
> target type":
> - const int *volatile_reg;
>
> Summarizing, after the changes it would look like the following (snippets):
>
> struct yas5xx_chip_info {
> unsigned int devid;
> const int *volatile_reg;
> int volatile_reg_qty;
> u32 scaling_val2;
> int (*get_measure)(struct yas5xx *yas5xx, s32 *to,
> s32 *xo, s32 *yo, s32 *zo);
> int (*get_calibration_data)(struct yas5xx *yas5xx);
> void (*dump_calibration)(struct yas5xx *yas5xx);
> int (*measure_offsets)(struct yas5xx *yas5xx);
> int (*power_on)(struct yas5xx *yas5xx);
> };
>
> struct yas5xx {
> struct device *dev;
> enum chip_ids chip;
> const struct yas5xx_chip_info *chip_info;
>
> ...
>
> };
>
> static const struct yas5xx_chip_info yas5xx_chip_info_tbl[] = {
> [yas530] = {
>
> ...
>
> },
> };
>
> If that's reasonable, I'll change it that way.
Yes, chip_info should be immutable, right? Otherwise it's something we
may not rely on.
> >> + if (id_check != yas5xx->chip_info->devid) {
> >> ret = -ENODEV;
> >> - dev_err(dev, "unhandled device ID %02x\n", yas5xx->devid);
> >> + dev_err(dev, "device ID %02x doesn't match %s\n",
> >> + id_check, id->name);
> >
> > ret = dev_err_probe() ?
>
> Makes sense, will change that.
>
> Though I have difficulties to implement this nicely. dev_err_probe()
> requires an "error value to test". The current "if (id_check !=
> yas5xx->chip_info->devid)" doesn't offer such a value by itself.
>
> Do you think the following would be appropriate, nesting the check
> within the dev_err_probe()? It doesn't look too good to me.
>
> ret = dev_err_probe(dev, id_check != yas5xx->chip_info->devid,
> "device ID %02x doesn't match %s\n",
> id_check, id->name);
> if (ret)
> goto assert_reset;
>
> If there are no better ideas, I would implement it that way.
> Additionally adding a comment and putting it into a block with the
> regmap_read before:
>
> /*
> * Check if the device ID from the register matches the one set
> * in the Device Tree.
> */
> ret = regmap_read(yas5xx->map, YAS5XX_DEVICE_ID, &id_check);
> if (ret)
> goto assert_reset;
> ret = dev_err_probe(dev, id_check != yas5xx->chip_info->devid,
> "device ID %02x doesn't match %s\n",
> id_check, id->name);
> if (ret)
> goto assert_reset;
>
> Hm, I think it's a bit hard to read. Suggestions for improvement are
> welcome. Otherwise I'd add it that way.
It should be like:
if (id_check != ...)
return dev_err_probe(dev, -ENODEV, ...);
...
> >> + dev_info(dev, "detected %s %s\n", yas5xx_product_name[yas5xx->chip],
> >> + yas5xx_version_name[yas5xx->chip][yas5xx->version]);
> >
> > I'm wondering if these arrays can be actually embedded into chip_info?
>
> While the variants (like "YAS530") are listed in the chip_info, the
> versions (like "A") are not. Yet, including the versions in chip_info
> would double the amount, making it visually more unclear, with only
> minor benefit.
>
> The first part of this call, the "product name", applies to the
> variants. Going the detour via chip_info, the array element to call
> could just be hard-coded in the chip_info table, like:
>
> static struct yas5xx_chip_info yas5xx_chip_info_tbl[] = {
> [yas530] = {
> ...
> .product_name = yas5xx_product_name[0],
> ...
>
> The second part, the "version name", is currently in the
> three-dimensional array yas5xx_version_name[]. The first dimension is
> the variant, this would need to be hard-coded in the chip_info table,
> similar to the example above. The second dimension is the version, this
> would need to come from within the yas5xx_probe() the function. I
> couldn't find a way how to assign one dimension in one place and another
> dimension in another place.
>
> To include the second part in the chip_info, I would split the
> three-dimensional array yas5xx_version_name[] into three separate
> two-dimensional arrays, one per variant. It would look like this (snippets):
>
>
> static const char yas530_version_name[][3] = { "A", "B" };
>
> static const char yas532_version_name[][3] = { "AB", "AC" };
>
> static const char yas533_version_name[][3] = { "AB", "AC" };
>
> ...
>
> struct yas5xx_chip_info {
> ...
> const char *product_name;
> const char (*version_name)[3];
> ...
>
> ...
>
> static struct yas5xx_chip_info yas5xx_chip_info_tbl[] = {
> [yas530] = {
> ...
> .product_name = yas5xx_product_name[0],
> .version_name = yas530_version_name,
> ...
>
> ...
>
>
> dev_info(dev, "detected %s %s\n",
> yas5xx->chip_info->product_name,
> yas5xx->chip_info->version_name[yas5xx->version]);
>
>
> I'm not fully sure this is the best way. Also note that I had to set the
> size of the second dimension of yas530_version_name to 3.
>
> Do you think I should do it this way to include "product_name" and
> "version_name" in the chip_info?
Again, you are the author and my point is just to make you look at
this from different angles. If you see no better way, go with the
current approach, but just spend a half an hour and perhaps we may
have a cleaner solution?
> If yes, should I probably do a similar thing for the values
> "t_ref_counts" and "min_temp_celcius_x10"? Here as well, the array
> elements are directly called in function yas5xx_calc_temperature()
> without using the chip_info structure.
>
> Also I noticed that I should add "static" to the typedef of arrays
> "t_ref_counts" and "min_temp_celcius_x10". Darn, and I have to correct
> the spelling of "celcius" into "celsius" in "min_temp_celcius_x10".
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v4 10/10] iio: magnetometer: yas530: Add YAS537 variant
2022-07-26 22:13 ` Jakob Hauser
@ 2022-07-27 17:46 ` Andy Shevchenko
2022-07-28 23:13 ` Jakob Hauser
0 siblings, 1 reply; 39+ messages in thread
From: Andy Shevchenko @ 2022-07-27 17:46 UTC (permalink / raw)
To: Jakob Hauser
Cc: Jonathan Cameron, Lars-Peter Clausen, Linus Walleij,
Hans de Goede, linux-iio, devicetree, phone-devel,
~postmarketos/upstreaming
On Wed, Jul 27, 2022 at 12:13 AM Jakob Hauser <jahau@rocketmail.com> wrote:
> On 04.07.22 21:47, Andy Shevchenko wrote:
> > On Mon, Jul 4, 2022 at 12:05 AM Jakob Hauser <jahau@rocketmail.com> wrote:
...
> >> + /* Write registers according to Android driver */
> >
> > Would be nice to elaborate in the comment what exactly the flow is
> > here, like "a) setting value of X; b) ...".
>
> Unfortunately, I don't know more than what the code already says. In the
> Yamaha Android driver, this part looks like this:
> https://github.com/msm8916-mainline/android_kernel_qcom_msm8916/blob/GT-I9195I/drivers/iio/magnetometer/yas_mag_drv-yas537.c#L345-L350
>
> The comment "Write registers according to Android driver" actually says
> "I don't know what I'm doing here, this is copy-paste from Android".
>
> I can remove the comment if you find it inappropriate. Though from my
> point of view the comment is ok.
The comment is okay for you, but for me it needs elaboration. So,
something like above in compressed format (couple of short sentences
to explain that nobody knows what the heck is that) will be
appreciated.
...
> >> + 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;
> >
> > Can bulk write be used here?
>
> Technically yes. But in this case I strongly would like to keep the
> single regmap_write as we go through different registers step by step
> and write them. That way it's much better readable. And it's just these
> two that are neighboring each other. As this happens in
> yas537_power_on(), it isn't done very often, thus doesn't cost much
> resources.
You seems program the 16-bit register with a single value, I don't
think it's a good idea to split a such. When it's a bulk write and
value defined with __be16 / __le16 it makes much more clear what
hardware is and what it expects.
...
> >> + /* The interval value is static in regular operation */
> >> + intrvl = (YAS537_DEFAULT_SENSOR_DELAY_MS * 1000
> >
> > MILLI ?
>
> What do you mean by this comment?
>
> The suffixes _MS and _US were proposed by you in v2. I think they are fine.
1000 --> MILLI ?
10^-3 sec * 10^-3 = 10^-6 sec.
...
> >> - ret = yas5xx->chip_info->measure_offsets(yas5xx);
> >> - if (ret)
> >> - goto assert_reset;
> >
> >> + if (yas5xx->chip_info->measure_offsets) {
> >
> > This can be done when you introduce this callback in the chip_info
> > structure, so this patch won't need to shuffle code again. I.o.w. we
> > can reduce ping-pong development in this series.
>
> I did this change in this patch on purpose because it's the introduction
> of YAS537 variant that's causing this change. YAS537 is the first
> variant that doesn't use yas5xx->chip_info->measure_offsets.
>
> Shall I move it to patch 9 nonetheless?
It's a bit hard to answer yes or no, I think after you try to resplit,
we will see what is the best for this part.
> >> + ret = yas5xx->chip_info->measure_offsets(yas5xx);
> >> + if (ret)
> >> + goto assert_reset;
> >> + }
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v4 09/10] iio: magnetometer: yas530: Introduce "chip_info" structure
2022-07-27 17:39 ` Andy Shevchenko
@ 2022-07-28 23:05 ` Jakob Hauser
2022-07-29 16:08 ` Andy Shevchenko
2022-07-29 16:13 ` Andy Shevchenko
0 siblings, 2 replies; 39+ messages in thread
From: Jakob Hauser @ 2022-07-28 23:05 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Jonathan Cameron, Lars-Peter Clausen, Linus Walleij,
Hans de Goede, linux-iio, devicetree, phone-devel,
~postmarketos/upstreaming
Hi Andy,
On 27.07.22 19:39, Andy Shevchenko wrote:
> On Wed, Jul 27, 2022 at 12:01 AM Jakob Hauser <jahau@rocketmail.com> wrote:
>> On 04.07.22 21:37, Andy Shevchenko wrote:
>>> On Mon, Jul 4, 2022 at 12:04 AM Jakob Hauser <jahau@rocketmail.com> wrote:
>
> ..
>
>>> Possible alternative would be more steps in 2), i.e. introducing
>>> chip_info for the callback only, then add field (or semantically
>>> unified fields) by field with corresponding changes in the code. In
>>> this case it would be easier to review.
>>
>> What do you mean by "introducing chip_info for the callback only"? I
>> guess you mean the function pointers?
>
> Yes.
>
>> According to this, the patch could be split into:
>> 1) introduce function pointers
>> 2) introduce arrays to look up values
>>
>> However, what's missing there is the introduction of the chip_info
>> approach as such. Admittedly difficult to follow are the changes in
>> yas5xx_probe():
>> - yas5xx->devid gets removed, it was the main switch decider before
>> - (for usage on other switches it is moved to yas5xx->chip_info->devid)
>> - yas5xx->chip as an enumeration is newly introduced
>> - yas5xx->chip_info as a structure gets initiated
>> - As the chip_info now chooses according to the i2c_device_id (via
>> enum chip_ids, thus yas5xx->chip), a new check is introduced to
>> make sure it matches the device ID read from the register. This
>> is done by "if (id_check != yas5xx->chip_info->devid)".
>> - The way of reporting product name and version name was changed
>> because introducing chip_info required to do this in one place
>> for all versions.
>> - As a consequence of this, yas5xx->name became obsolete.
>>
>> This would have to be done before introducing function pointers and
>> arrays, like:
>> 1) introduce chip_info structural changes
>> 2) add function pointers
>> 3) add arrays to look up values
>>
>> But it can't be easily split that way. I can't establish an "empty"
>> chip_info approach as a fist step without removing or changing many
>> other things. The structural changes are related to other changes in
>> that patch.
>>
>> I'm thinking about first introducing chip_info and thereafter
>> establishing the function pointers. In between I could try to split out
>> the new temperature function:
>> 1) introduce chip_info structural changes incl. arrays to look up values
>> 2) split out new yas5xx_calc_temperature() function
>> 3) add function pointers
>>
>> I'm not yet sure it can be split that way because things are entangled.
>> But I will try to this in v5.
>
> Yes, my main point is to try to see under different angles on how you
> can split the series. We have plenty of time.
OK, I'll try it like this in v5.
...
>>>> -#define YAS530_20DEGREES 182 /* Counts starting at -62 °C */
>>>
>>>> -#define YAS532_20DEGREES 390 /* Counts starting at -50 °C */
>>>
>>> The comments suddenly disappear from the file. See below.
>>
>> The comment's didn't actually disappear but rather got restructured by
>> introducing new chip_info approach. See below.
>
> See below.
>
> ..
>
...
>>> Also, use * instead of one dimension of array.
>>
>> Sorry, probably I lack the basic knowledge here. Can you explain how to
>> do that?
>
> static const char *_name[][] = {
> };
>
> ?
Indeed I was missing that basic knowledge. Thanks for showing!
After some trying and reading, I think it should be:
static const char * const _name[][] = {
};
>>>> +/* Number of counts between minimum and reference temperature */
>>>> +const u16 t_ref_counts[] = { 182, 390, 390 };
>>>> +
>>>> +/* Starting point of temperature counting in 1/10:s degrees Celsius */
>>>> +const s16 min_temp_celcius_x10[] = { -620, -500, -500 };
>>>
>>> See above.
>>
>> The former comments...
>> 182 /* Counts starting at -62 °C */
>> 390 /* Counts starting at -50 °C */
>>
>> ... and the two new comments above the arrays actually should say the
>> same thing. At least that was my intention.
>>
>> The first value is a number of counts. And the counting starts at a
>> certain temperature, which is the second value. Both the old and the new
>> comments are intended to describe this.
>>
>> In the introduction of this temperature handling (patch 4), there is a
>> lot more description on these values in function yas5xx_get_measure().
>> When creating the new "chip_info" patch 9, I was thinking about moving
>> some of that description up here to these arrays. However, instead I
>> tried to following Jonathan's suggestion in v3 to keep the describing
>> text low and rather let the formulas speak for themselves. These values
>> are applied at a formula in function yas5xx_calc_temperature() which is
>> supposed to by kind of self explanatory.
>>
>> What may lead to confusion is the equivalent usage of "starting" and
>> "minimum" here. In the initial patchset I used "starting" related to the
>> counts, Jonathan suggested "minimum" or actually "min_temp" related to
>> the temperature. The comments here above are bit of a mixture of both. I
>> still think it's good enough to understand. The sentence "Starting point
>> of temperature ..." describes the value min_temp_celcius_x10. Using a
>> term like "starting temperature" would probably be more confusing.
>
> Confusing to me, who doesn't know the specifics of the chip, it is
> easy to read '-62 °C' vs. "read comment, look into an array, calculate
> in your brain".
I don't have a good idea how to fulfill your requirement in a brief way
when the values are stored in an array altogether.
Instead I end up at a longer comment again.
Though this also offers the chance to add some additional information
where the values were taken from.
Is it appropriate to include this to kernel doc? Generally I'm unsure on
kernel doc but I guess yes...
It would look like:
/**
* const u16 t_ref_counts[] - Number of counts at reference temperature
*
* The temperature value at YAS magnetometers is a number of counts. The
* values in t_ref_counts[] are the counts at the reference temperature
* of 20 °C.
*
* For YAS532/533, this value is known from the Android driver. For
* YAS530, it was approximately measured.
* /
static const u16 t_ref_counts[] = { 182, 390, 390, };
/**
* const s16 min_temp_celsius_x10[] - Starting point of temperature
* counting in 1/10:s degrees Celsius
*
* The array min_temp_celsius_x10[] contains the temperatures where the
* temperature value count is 0. The values are in 1/10:s degrees
* Celsius to ease the further temperature calculation.
*
* These temperatures are derived from the temperature resolutions given
* in the data sheets.
*/
static const s16 min_temp_celsius_x10[] = { -620, -500, -500, };
Please note that I have to touch and extend these comments in the last
patch "Add YAS537 variant". In the first comment I'll add that the value
for YAS537 was approximately measured too. And in the second command I
have to add the word "theoretical" temperatures because on YAS537 it is
at -386 °C, which is below absolute zero Kelvin, thus a mere theoretical
temperature.
>>>> +struct yas5xx_chip_info {
>>>> + unsigned int devid;
>>>
>>>> + const int *volatile_reg;
>>>> + const int volatile_reg_qty;
>>>> + const u32 scaling_val2;
>>>
>>> Why const here?
>>> I assume entire structure is const, no?
>>
>> I'm rather new to C language, not having a good grasp of "const" in
>> structures yet. I would have guessed it doesn't work well with the
>> function pointers.
>>
>> However, having some compile tests, there are no complaints about the
>> function pointers.
>>
>> To change the whole chip_info structure to "const", I would:
>> - within the "struct yas5xx" change to "const struct
>> yas5xx_chip_info *chip_info;"
>> - change following typedef to "static const struct
>> yas5xx_chip_info yas5xx_chip_info_tbl[] = ..."
>>
>> Then, within the "struct yas5xx_chip_info", I can remove "const" from:
>> - int volatile_reg_qty;
>> - u32 scaling_val2;
>>
>> However, I have to keep "const" at the following, otherwise the complier
>> complains that "initialization discards 'const' qualifier from pointer
>> target type":
>> - const int *volatile_reg;
>>
>> Summarizing, after the changes it would look like the following (snippets):
>>
>> struct yas5xx_chip_info {
>> unsigned int devid;
>> const int *volatile_reg;
>> int volatile_reg_qty;
>> u32 scaling_val2;
>> int (*get_measure)(struct yas5xx *yas5xx, s32 *to,
>> s32 *xo, s32 *yo, s32 *zo);
>> int (*get_calibration_data)(struct yas5xx *yas5xx);
>> void (*dump_calibration)(struct yas5xx *yas5xx);
>> int (*measure_offsets)(struct yas5xx *yas5xx);
>> int (*power_on)(struct yas5xx *yas5xx);
>> };
>>
>> struct yas5xx {
>> struct device *dev;
>> enum chip_ids chip;
>> const struct yas5xx_chip_info *chip_info;
>>
>> ...
>>
>> };
>>
>> static const struct yas5xx_chip_info yas5xx_chip_info_tbl[] = {
>> [yas530] = {
>>
>> ...
>>
>> },
>> };
>>
>> If that's reasonable, I'll change it that way.
>
> Yes, chip_info should be immutable, right? Otherwise it's something we
> may not rely on.
OK
>>>> + if (id_check != yas5xx->chip_info->devid) {
>>>> ret = -ENODEV;
>>>> - dev_err(dev, "unhandled device ID %02x\n", yas5xx->devid);
>>>> + dev_err(dev, "device ID %02x doesn't match %s\n",
>>>> + id_check, id->name);
>>>
>>> ret = dev_err_probe() ?
>>
>> Makes sense, will change that.
>>
>> Though I have difficulties to implement this nicely. dev_err_probe()
>> requires an "error value to test". The current "if (id_check !=
>> yas5xx->chip_info->devid)" doesn't offer such a value by itself.
>>
>> Do you think the following would be appropriate, nesting the check
>> within the dev_err_probe()? It doesn't look too good to me.
>>
>> ret = dev_err_probe(dev, id_check != yas5xx->chip_info->devid,
>> "device ID %02x doesn't match %s\n",
>> id_check, id->name);
>> if (ret)
>> goto assert_reset;
>>
>> If there are no better ideas, I would implement it that way.
>> Additionally adding a comment and putting it into a block with the
>> regmap_read before:
>>
>> /*
>> * Check if the device ID from the register matches the one set
>> * in the Device Tree.
>> */
>> ret = regmap_read(yas5xx->map, YAS5XX_DEVICE_ID, &id_check);
>> if (ret)
>> goto assert_reset;
>> ret = dev_err_probe(dev, id_check != yas5xx->chip_info->devid,
>> "device ID %02x doesn't match %s\n",
>> id_check, id->name);
>> if (ret)
>> goto assert_reset;
>>
>> Hm, I think it's a bit hard to read. Suggestions for improvement are
>> welcome. Otherwise I'd add it that way.
>
> It should be like:
>
> if (id_check != ...)
> return dev_err_probe(dev, -ENODEV, ...);
Ah! Thanks :)
>>>> + dev_info(dev, "detected %s %s\n", yas5xx_product_name[yas5xx->chip],
>>>> + yas5xx_version_name[yas5xx->chip][yas5xx->version]);
>>>
>>> I'm wondering if these arrays can be actually embedded into chip_info?
>>
>> While the variants (like "YAS530") are listed in the chip_info, the
>> versions (like "A") are not. Yet, including the versions in chip_info
>> would double the amount, making it visually more unclear, with only
>> minor benefit.
>>
>> The first part of this call, the "product name", applies to the
>> variants. Going the detour via chip_info, the array element to call
>> could just be hard-coded in the chip_info table, like:
>>
>> static struct yas5xx_chip_info yas5xx_chip_info_tbl[] = {
>> [yas530] = {
>> ...
>> .product_name = yas5xx_product_name[0],
>> ...
>>
>> The second part, the "version name", is currently in the
>> three-dimensional array yas5xx_version_name[]. The first dimension is
>> the variant, this would need to be hard-coded in the chip_info table,
>> similar to the example above. The second dimension is the version, this
>> would need to come from within the yas5xx_probe() the function. I
>> couldn't find a way how to assign one dimension in one place and another
>> dimension in another place.
>>
>> To include the second part in the chip_info, I would split the
>> three-dimensional array yas5xx_version_name[] into three separate
>> two-dimensional arrays, one per variant. It would look like this (snippets):
>>
>>
>> static const char yas530_version_name[][3] = { "A", "B" };
>>
>> static const char yas532_version_name[][3] = { "AB", "AC" };
>>
>> static const char yas533_version_name[][3] = { "AB", "AC" };
>>
>> ...
>>
>> struct yas5xx_chip_info {
>> ...
>> const char *product_name;
>> const char (*version_name)[3];
>> ...
>>
>> ...
>>
>> static struct yas5xx_chip_info yas5xx_chip_info_tbl[] = {
>> [yas530] = {
>> ...
>> .product_name = yas5xx_product_name[0],
>> .version_name = yas530_version_name,
>> ...
>>
>> ...
>>
>>
>> dev_info(dev, "detected %s %s\n",
>> yas5xx->chip_info->product_name,
>> yas5xx->chip_info->version_name[yas5xx->version]);
>>
>>
>> I'm not fully sure this is the best way. Also note that I had to set the
>> size of the second dimension of yas530_version_name to 3.
>>
>> Do you think I should do it this way to include "product_name" and
>> "version_name" in the chip_info?
>
> Again, you are the author and my point is just to make you look at
> this from different angles. If you see no better way, go with the
> current approach, but just spend a half an hour and perhaps we may
> have a cleaner solution?
Indeed, to my own surprise I found a solution with the 2D array:
static const char * const yas5xx_product_name[] = {
"YAS530 MS-3E",
"YAS532 MS-3R",
"YAS533 MS-3F",
};
static const char * const yas5xx_version_name[][2] = {
[yas530] = { "A", "B" },
[yas532] = { "AB", "AC" },
[yas533] = { "AB", "AC" },
};
...
struct yas5xx_chip_info {
...
const char *product_name;
const char * const *version_name;
...
};
static const struct yas5xx_chip_info yas5xx_chip_info_tbl[] = {
[yas530] = {
...
.product_name = yas5xx_product_name[0],
.version_name = yas5xx_version_name[0],
...
},
};
...
dev_info(dev, "detected %s %s\n",
yas5xx->chip_info->product_name,
yas5xx->chip_info->version_name[yas5xx->version]);
>> If yes, should I probably do a similar thing for the values
>> "t_ref_counts" and "min_temp_celcius_x10"? Here as well, the array
>> elements are directly called in function yas5xx_calc_temperature()
>> without using the chip_info structure.
I then will include "product_name" and "version_name" as well as
"t_ref_counts" and "min_temp_celsius_x10" to the chip_info struct to
have everything collected.
...
Kind regards,
Jakob
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v4 10/10] iio: magnetometer: yas530: Add YAS537 variant
2022-07-27 17:46 ` Andy Shevchenko
@ 2022-07-28 23:13 ` Jakob Hauser
2022-07-29 17:24 ` Andy Shevchenko
0 siblings, 1 reply; 39+ messages in thread
From: Jakob Hauser @ 2022-07-28 23:13 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Jonathan Cameron, Lars-Peter Clausen, Linus Walleij,
Hans de Goede, linux-iio, devicetree, phone-devel,
~postmarketos/upstreaming
Hi Andy,
On 27.07.22 19:46, Andy Shevchenko wrote:
> On Wed, Jul 27, 2022 at 12:13 AM Jakob Hauser <jahau@rocketmail.com> wrote:
>> On 04.07.22 21:47, Andy Shevchenko wrote:
>>> On Mon, Jul 4, 2022 at 12:05 AM Jakob Hauser <jahau@rocketmail.com> wrote:
>
> ..
>
>>>> + /* Write registers according to Android driver */
>>>
>>> Would be nice to elaborate in the comment what exactly the flow is
>>> here, like "a) setting value of X; b) ...".
>>
>> Unfortunately, I don't know more than what the code already says. In the
>> Yamaha Android driver, this part looks like this:
>> https://github.com/msm8916-mainline/android_kernel_qcom_msm8916/blob/GT-I9195I/drivers/iio/magnetometer/yas_mag_drv-yas537.c#L345-L350
>>
>> The comment "Write registers according to Android driver" actually says
>> "I don't know what I'm doing here, this is copy-paste from Android".
>>
>> I can remove the comment if you find it inappropriate. Though from my
>> point of view the comment is ok.
>
> The comment is okay for you, but for me it needs elaboration. So,
> something like above in compressed format (couple of short sentences
> to explain that nobody knows what the heck is that) will be
> appreciated.
I was thinking about:
/*
* Write registers according to Android driver, the exact meaning
* is unknown
*/
This reminded me of another location where I first had a comment
"Writing SRST register, the exact meaning is unknown". There you
criticized the part "the exact meaning is unknown", so I changed it to
simply "Writing SRST register".
Accordingly, I would choose the following comment here:
/* Writing ADCCAL and TRM registers */
>>>> + 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;
>>>
>>> Can bulk write be used here?
>>
>> Technically yes. But in this case I strongly would like to keep the
>> single regmap_write as we go through different registers step by step
>> and write them. That way it's much better readable. And it's just these
>> two that are neighboring each other. As this happens in
>> yas537_power_on(), it isn't done very often, thus doesn't cost much
>> resources.
>
> You seems program the 16-bit register with a single value, I don't
> think it's a good idea to split a such. When it's a bulk write and
> value defined with __be16 / __le16 it makes much more clear what
> hardware is and what it expects.
We don't know for sure whether it is a 16-bit register or an incomplete
register naming.
Not all the registers are properly named in the original driver. E.g.
there is a register called "YAS537_REG_MTCR" [1] with no names for the
following registers. Further down, this and the following 11 registers
are written by just counting up the register number [2].
It looks similar to the situation at register "YAS537_REG_ADCCALR",
where the numerically following register (0x92) doesn't have a name [3].
It could be because of a 16-bit register, as you say, but it could also
be a naming thing.
At the location in discussion, the original driver uses two single
writes [4]. I'd stick to that.
[1]
https://github.com/msm8916-mainline/android_kernel_qcom_msm8916/blob/GT-I9195I/drivers/iio/magnetometer/yas_mag_drv-yas537.c#L38
[2]
https://github.com/msm8916-mainline/android_kernel_qcom_msm8916/blob/GT-I9195I/drivers/iio/magnetometer/yas_mag_drv-yas537.c#L277-L279
[3]
https://github.com/msm8916-mainline/android_kernel_qcom_msm8916/blob/GT-I9195I/drivers/iio/magnetometer/yas_mag_drv-yas537.c#L37
[4]
https://github.com/msm8916-mainline/android_kernel_qcom_msm8916/blob/GT-I9195I/drivers/iio/magnetometer/yas_mag_drv-yas537.c#L345-L348
>>>> + /* The interval value is static in regular operation */
>>>> + intrvl = (YAS537_DEFAULT_SENSOR_DELAY_MS * 1000
>>>
>>> MILLI ?
>>
>> What do you mean by this comment?
>>
>> The suffixes _MS and _US were proposed by you in v2. I think they are fine.
>
> 1000 --> MILLI ?
>
> 10^-3 sec * 10^-3 = 10^-6 sec.
Ah, well, the full formula is ...
intrvl = (YAS537_DEFAULT_SENSOR_DELAY_MS * 1000
- YAS537_MEASURE_TIME_WORST_US) / 4100;
... with the fixed defined values:
#define YAS537_DEFAULT_SENSOR_DELAY_MS 50
#define YAS537_MEASURE_TIME_WORST_US 1500
So it means ...
intrvl = (50 milliseconds * 1000 - 1500 microseconds) / 4100;
... which is:
intrvl = (50000 microseconds - 1500 microseconds) / 4100;
The units of "4100" and "intrvl" are unclear. I don't know if "intrvl"
is a time or some abstract value.
Still I didn't get your comment. Is your intention to change the "50
milliseconds * 1000" to "50000 microseconds" in the define?
It would look like ...
#define YAS537_DEFAULT_SENSOR_DELAY_US 50000
... though I would prefer to keep current define, as it is implemented
now and stated above:
#define YAS537_DEFAULT_SENSOR_DELAY_MS 50
>>>> - ret = yas5xx->chip_info->measure_offsets(yas5xx);
>>>> - if (ret)
>>>> - goto assert_reset;
>>>
>>>> + if (yas5xx->chip_info->measure_offsets) {
>>>
>>> This can be done when you introduce this callback in the chip_info
>>> structure, so this patch won't need to shuffle code again. I.o.w. we
>>> can reduce ping-pong development in this series.
>>
>> I did this change in this patch on purpose because it's the introduction
>> of YAS537 variant that's causing this change. YAS537 is the first
>> variant that doesn't use yas5xx->chip_info->measure_offsets.
>>
>> Shall I move it to patch 9 nonetheless?
>
> It's a bit hard to answer yes or no, I think after you try to resplit,
> we will see what is the best for this part.
Hm... to avoid discussions and shorten iterations, I'll move it to the
newly "add function pointers" patch in v5. I'll add a comment on this in
the commit message of that patch.
...
Kind regards,
Jakob
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v4 09/10] iio: magnetometer: yas530: Introduce "chip_info" structure
2022-07-28 23:05 ` Jakob Hauser
@ 2022-07-29 16:08 ` Andy Shevchenko
2022-07-29 22:52 ` Jakob Hauser
2022-07-29 16:13 ` Andy Shevchenko
1 sibling, 1 reply; 39+ messages in thread
From: Andy Shevchenko @ 2022-07-29 16:08 UTC (permalink / raw)
To: Jakob Hauser
Cc: Jonathan Cameron, Lars-Peter Clausen, Linus Walleij,
Hans de Goede, linux-iio, devicetree, phone-devel,
~postmarketos/upstreaming
On Fri, Jul 29, 2022 at 1:06 AM Jakob Hauser <jahau@rocketmail.com> wrote:
> On 27.07.22 19:39, Andy Shevchenko wrote:
> > On Wed, Jul 27, 2022 at 12:01 AM Jakob Hauser <jahau@rocketmail.com> wrote:
...
> Instead I end up at a longer comment again.
It's fine!
> Though this also offers the chance to add some additional information
> where the values were taken from.
>
> Is it appropriate to include this to kernel doc? Generally I'm unsure on
> kernel doc but I guess yes...
I'm unsure if it's appropriate for static (integer) arrays, you may
run kernel doc script yourself and check man, html and pdf formats to
see if it's rendered correctly.
...
> > Again, you are the author and my point is just to make you look at
> > this from different angles. If you see no better way, go with the
> > current approach, but just spend a half an hour and perhaps we may
> > have a cleaner solution?
>
> Indeed, to my own surprise I found a solution with the 2D array:
It looks better!
...
> dev_info(dev, "detected %s %s\n",
> yas5xx->chip_info->product_name,
> yas5xx->chip_info->version_name[yas5xx->version]);
Very good!
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v4 09/10] iio: magnetometer: yas530: Introduce "chip_info" structure
2022-07-28 23:05 ` Jakob Hauser
2022-07-29 16:08 ` Andy Shevchenko
@ 2022-07-29 16:13 ` Andy Shevchenko
2022-07-29 22:56 ` Jakob Hauser
1 sibling, 1 reply; 39+ messages in thread
From: Andy Shevchenko @ 2022-07-29 16:13 UTC (permalink / raw)
To: Jakob Hauser
Cc: Jonathan Cameron, Lars-Peter Clausen, Linus Walleij,
Hans de Goede, linux-iio, devicetree, phone-devel,
~postmarketos/upstreaming
On Fri, Jul 29, 2022 at 1:06 AM Jakob Hauser <jahau@rocketmail.com> wrote:
> On 27.07.22 19:39, Andy Shevchenko wrote:
...
Just a couple of remarks.
> Indeed, to my own surprise I found a solution with the 2D array:
>
> static const char * const yas5xx_product_name[] = {
> "YAS530 MS-3E",
> "YAS532 MS-3R",
> "YAS533 MS-3F",
> };
>
> static const char * const yas5xx_version_name[][2] = {
yas5xx_version_names (note S at the end)
> [yas530] = { "A", "B" },
> [yas532] = { "AB", "AC" },
> [yas533] = { "AB", "AC" },
> };
>
> ...
>
> struct yas5xx_chip_info {
> ...
> const char *product_name;
> const char * const *version_name;
> ...
> };
>
> static const struct yas5xx_chip_info yas5xx_chip_info_tbl[] = {
> [yas530] = {
> ...
> .product_name = yas5xx_product_name[0],
> .version_name = yas5xx_version_name[0],
Also 0 --> yas530 (use enum:ed indices)
> },
> };
>
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v4 10/10] iio: magnetometer: yas530: Add YAS537 variant
2022-07-28 23:13 ` Jakob Hauser
@ 2022-07-29 17:24 ` Andy Shevchenko
2022-07-29 23:10 ` Jakob Hauser
0 siblings, 1 reply; 39+ messages in thread
From: Andy Shevchenko @ 2022-07-29 17:24 UTC (permalink / raw)
To: Jakob Hauser
Cc: Jonathan Cameron, Lars-Peter Clausen, Linus Walleij,
Hans de Goede, linux-iio, devicetree, phone-devel,
~postmarketos/upstreaming
On Fri, Jul 29, 2022 at 1:13 AM Jakob Hauser <jahau@rocketmail.com> wrote:
> On 27.07.22 19:46, Andy Shevchenko wrote:
...
> /*
> * Write registers according to Android driver, the exact meaning
> * is unknown
With a period at the end :-)
> */
> This reminded me of another location where I first had a comment
> "Writing SRST register, the exact meaning is unknown". There you
> criticized the part "the exact meaning is unknown", so I changed it to
> simply "Writing SRST register".
Yeah, but that is different, SRST seems like easy to deduce to "soft
reset" (taking into account where it's programmed in the run flow).
> Accordingly, I would choose the following comment here:
>
> /* Writing ADCCAL and TRM registers */
Fine with me!
...
> > You seem to program the 16-bit register with a single value, I don't
> > think it's a good idea to split a such. When it's a bulk write and
> > value defined with __be16 / __le16 it makes much more clear what
> > hardware is and what it expects.
>
> We don't know for sure whether it is a 16-bit register or an incomplete
> register naming.
By the values you write into it seems to be a __be16 calibration
register. The value to write is 0x3f8 which might ring a bell to you
if you know what other values related to ADC.
> Not all the registers are properly named in the original driver. E.g.
> there is a register called "YAS537_REG_MTCR" [1] with no names for the
> following registers. Further down, this and the following 11 registers
> are written by just counting up the register number [2].
12 (8-bit) registers, which may suggest 6 __be16, in any case looks
like a mount matrix or so, with X,Y,Z values programmed.
This is interesting as well...
https://software-dl.ti.com/simplelink/esd/plugins/sail/1.10.00.06/exports/docs/sail/doxygen/html/structyas537__t.html
(Btw, it also suggest that ADC calibration is 16-bit register)
> It looks similar to the situation at register "YAS537_REG_ADCCALR",
> where the numerically following register (0x92) doesn't have a name [3].
> It could be because of a 16-bit register, as you say, but it could also
> be a naming thing.
>
> At the location in discussion, the original driver uses two single
> writes [4]. I'd stick to that.
>
> [1]
> https://github.com/msm8916-mainline/android_kernel_qcom_msm8916/blob/GT-I9195I/drivers/iio/magnetometer/yas_mag_drv-yas537.c#L38
> [2]
> https://github.com/msm8916-mainline/android_kernel_qcom_msm8916/blob/GT-I9195I/drivers/iio/magnetometer/yas_mag_drv-yas537.c#L277-L279
> [3]
> https://github.com/msm8916-mainline/android_kernel_qcom_msm8916/blob/GT-I9195I/drivers/iio/magnetometer/yas_mag_drv-yas537.c#L37
> [4]
> https://github.com/msm8916-mainline/android_kernel_qcom_msm8916/blob/GT-I9195I/drivers/iio/magnetometer/yas_mag_drv-yas537.c#L345-L348
It's all the joy of reverse engineering...
To the 4100 denominator:
https://github.com/XPerience-AOSP-Lollipop/android_kernel_wingtech_msm8916/blob/xpe-11.1/drivers/input/misc/yas_mag_drv-yas537.c#L235,
seems you can find a lot by browsing someone's code and perhaps a Git
history.
...
> Still I didn't get your comment. Is your intention to change the "50
> milliseconds * 1000" to "50000 microseconds" in the define?
>
> It would look like ...
>
> #define YAS537_DEFAULT_SENSOR_DELAY_US 50000
>
> ... though I would prefer to keep current define, as it is implemented
> now and stated above:
>
> #define YAS537_DEFAULT_SENSOR_DELAY_MS 50
No, just to show in the actual calculation that you convert MS to US
using MILLI.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v4 09/10] iio: magnetometer: yas530: Introduce "chip_info" structure
2022-07-29 16:08 ` Andy Shevchenko
@ 2022-07-29 22:52 ` Jakob Hauser
0 siblings, 0 replies; 39+ messages in thread
From: Jakob Hauser @ 2022-07-29 22:52 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Jonathan Cameron, Lars-Peter Clausen, Linus Walleij,
Hans de Goede, linux-iio, devicetree, phone-devel,
~postmarketos/upstreaming
Hi Andy,
On 29.07.22 18:08, Andy Shevchenko wrote:
> On Fri, Jul 29, 2022 at 1:06 AM Jakob Hauser <jahau@rocketmail.com> wrote:
...
>> Is it appropriate to include this to kernel doc? Generally I'm unsure on
>> kernel doc but I guess yes...
>
> I'm unsure if it's appropriate for static (integer) arrays, you may
> run kernel doc script yourself and check man, html and pdf formats to
> see if it's rendered correctly.
Nope, doesn't work...
It was a beginner's mistake on my side. I did check
Documentation/doc-guide/kernel-doc.rst before and followed the "typedef"
part, which is quite short. I thought "typedef" is a more general
designation – but after reading more on "typedef" I now know what it is :/
https://github.com/torvalds/linux/blob/v5.18/Documentation/doc-guide/kernel-doc.rst#typedef-documentation
So, as far as I can see, kernel doc applies to functions, structures,
unions, enumerations and typedefs only.
I would then switch the comments to regular comments:
/*
* t_ref_counts[] is the number of counts at reference temperature.
*
* The temperature value at YAS magnetometers is a number of counts. The
* values in t_ref_counts[] are the counts at the reference temperature
* of 20 °C.
*
* For YAS532/533, this value is known from the Android driver. For
* YAS530, it was approximately measured.
*/
static const u16 t_ref_counts[] = { 182, 390, 390, };
/*
* min_temp_celsius_x10[] is the starting point of temperature counting
* in 1/10:s degrees Celsius.
*
* The array min_temp_celsius_x10[] contains the temperatures where the
* temperature value count is 0. The values are in 1/10:s degrees
* Celsius to ease the further temperature calculation.
*
* These temperatures are derived from the temperature resolutions given
* in the data sheets.
*/
static const s16 min_temp_celsius_x10[] = { -620, -500, -500, };
...
Kind regards,
Jakob
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v4 09/10] iio: magnetometer: yas530: Introduce "chip_info" structure
2022-07-29 16:13 ` Andy Shevchenko
@ 2022-07-29 22:56 ` Jakob Hauser
2022-07-30 0:53 ` Andy Shevchenko
0 siblings, 1 reply; 39+ messages in thread
From: Jakob Hauser @ 2022-07-29 22:56 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Jonathan Cameron, Lars-Peter Clausen, Linus Walleij,
Hans de Goede, linux-iio, devicetree, phone-devel,
~postmarketos/upstreaming
Hi Andy,
On 29.07.22 18:13, Andy Shevchenko wrote:
> On Fri, Jul 29, 2022 at 1:06 AM Jakob Hauser <jahau@rocketmail.com> wrote:
>
> ..
>
> Just a couple of remarks.
>
>> Indeed, to my own surprise I found a solution with the 2D array:
>>
>> static const char * const yas5xx_product_name[] = {
>> "YAS530 MS-3E",
>> "YAS532 MS-3R",
>> "YAS533 MS-3F",
>> };
>>
>> static const char * const yas5xx_version_name[][2] = {
>
> yas5xx_version_names (note S at the end)
As I understand you, it's to be applied on "yas5xx_version_names" only.
In the chip_info table, it would then look like:
.product_name = yas5xx_product_name[yas530],
.version_name = yas5xx_version_names[yas530],
^
S
>
>> [yas530] = { "A", "B" },
>> [yas532] = { "AB", "AC" },
>> [yas533] = { "AB", "AC" },
>> };
>>
>> ...
>>
>> struct yas5xx_chip_info {
>> ...
>> const char *product_name;
>> const char * const *version_name;
>> ...
>> };
>>
>> static const struct yas5xx_chip_info yas5xx_chip_info_tbl[] = {
>> [yas530] = {
>> ...
>> .product_name = yas5xx_product_name[0],
>> .version_name = yas5xx_version_name[0],
>
> Also 0 --> yas530 (use enum:ed indices)
OK
>
>> },
>> };
>>
>
Kind regards,
Jakob
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v4 10/10] iio: magnetometer: yas530: Add YAS537 variant
2022-07-29 17:24 ` Andy Shevchenko
@ 2022-07-29 23:10 ` Jakob Hauser
2022-07-30 11:32 ` Andy Shevchenko
0 siblings, 1 reply; 39+ messages in thread
From: Jakob Hauser @ 2022-07-29 23:10 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Jonathan Cameron, Lars-Peter Clausen, Linus Walleij,
Hans de Goede, linux-iio, devicetree, phone-devel,
~postmarketos/upstreaming
Hi Andy,
On 29.07.22 19:24, Andy Shevchenko wrote:
> On Fri, Jul 29, 2022 at 1:13 AM Jakob Hauser <jahau@rocketmail.com> wrote:
>> On 27.07.22 19:46, Andy Shevchenko wrote:
>
> ..
>
>> /*
>> * Write registers according to Android driver, the exact meaning
>> * is unknown
>
> With a period at the end :-)
>
>> */
>
>> This reminded me of another location where I first had a comment
>> "Writing SRST register, the exact meaning is unknown". There you
>> criticized the part "the exact meaning is unknown", so I changed it to
>> simply "Writing SRST register".
>
> Yeah, but that is different, SRST seems like easy to deduce to "soft
> reset" (taking into account where it's programmed in the run flow).
>
>> Accordingly, I would choose the following comment here:
>>
>> /* Writing ADCCAL and TRM registers */
>
> Fine with me!
OK, I'll apply the comment "Writing ADCCAL and TRM registers".
>
> ..
>
>>> You seem to program the 16-bit register with a single value, I don't
>>> think it's a good idea to split a such. When it's a bulk write and
>>> value defined with __be16 / __le16 it makes much more clear what
>>> hardware is and what it expects.
>>
>> We don't know for sure whether it is a 16-bit register or an incomplete
>> register naming.
>
> By the values you write into it seems to be a __be16 calibration
> register. The value to write is 0x3f8 which might ring a bell to you
> if you know what other values related to ADC.
Sigh, ok, I'll apply bulk write.
How to do it correctly? I guess:
__be16 buf = cpu_to_be16(GENMASK(9, 3));
ret = regmap_bulk_write(yas5xx->map, YAS537_ADCCAL, &buf, 2);
if (ret)
return ret;
The whole block would then look like:
/* Writing ADCCAL and TRM registers */
__be16 buf = cpu_to_be16(GENMASK(9, 3));
ret = regmap_bulk_write(yas5xx->map, YAS537_ADCCAL, &buf, 2);
if (ret)
return ret;
ret = regmap_write(yas5xx->map, YAS537_TRM, GENMASK(7, 0));
if (ret)
return ret;
...
> To the 4100 denominator:
> https://github.com/XPerience-AOSP-Lollipop/android_kernel_wingtech_msm8916/blob/xpe-11.1/drivers/input/misc/yas_mag_drv-yas537.c#L235,
> seems you can find a lot by browsing someone's code and perhaps a Git
> history.
I've seen that comment before but I don't understand its meaning.
>> Still I didn't get your comment. Is your intention to change the "50
>> milliseconds * 1000" to "50000 microseconds" in the define?
>>
>> It would look like ...
>>
>> #define YAS537_DEFAULT_SENSOR_DELAY_US 50000
>>
>> ... though I would prefer to keep current define, as it is implemented
>> now and stated above:
>>
>> #define YAS537_DEFAULT_SENSOR_DELAY_MS 50
>
> No, just to show in the actual calculation that you convert MS to US
> using MILLI.
Sorry, I still don't get what you want me to do. What do you mean by
"using MILLI", can you elaborate?
Kind regards,
Jakob
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v4 09/10] iio: magnetometer: yas530: Introduce "chip_info" structure
2022-07-29 22:56 ` Jakob Hauser
@ 2022-07-30 0:53 ` Andy Shevchenko
0 siblings, 0 replies; 39+ messages in thread
From: Andy Shevchenko @ 2022-07-30 0:53 UTC (permalink / raw)
To: Jakob Hauser
Cc: Jonathan Cameron, Lars-Peter Clausen, Linus Walleij,
Hans de Goede, linux-iio, devicetree, phone-devel,
~postmarketos/upstreaming
On Sat, Jul 30, 2022 at 12:56 AM Jakob Hauser <jahau@rocketmail.com> wrote:
> On 29.07.22 18:13, Andy Shevchenko wrote:
> > On Fri, Jul 29, 2022 at 1:06 AM Jakob Hauser <jahau@rocketmail.com> wrote:
...
> >> static const char * const yas5xx_version_name[][2] = {
> >
> > yas5xx_version_names (note S at the end)
>
> As I understand you, it's to be applied on "yas5xx_version_names" only.
> In the chip_info table, it would then look like:
>
> .product_name = yas5xx_product_name[yas530],
> .version_name = yas5xx_version_names[yas530],
> ^
> S
Yes.
> >> [yas530] = { "A", "B" },
> >> [yas532] = { "AB", "AC" },
> >> [yas533] = { "AB", "AC" },
> >> };
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v4 10/10] iio: magnetometer: yas530: Add YAS537 variant
2022-07-29 23:10 ` Jakob Hauser
@ 2022-07-30 11:32 ` Andy Shevchenko
2022-07-30 13:31 ` Jakob Hauser
0 siblings, 1 reply; 39+ messages in thread
From: Andy Shevchenko @ 2022-07-30 11:32 UTC (permalink / raw)
To: Jakob Hauser
Cc: Jonathan Cameron, Lars-Peter Clausen, Linus Walleij,
Hans de Goede, linux-iio, devicetree, phone-devel,
~postmarketos/upstreaming
On Sat, Jul 30, 2022 at 1:10 AM Jakob Hauser <jahau@rocketmail.com> wrote:
> On 29.07.22 19:24, Andy Shevchenko wrote:
> > On Fri, Jul 29, 2022 at 1:13 AM Jakob Hauser <jahau@rocketmail.com> wrote:
> >> On 27.07.22 19:46, Andy Shevchenko wrote:
...
> >> We don't know for sure whether it is a 16-bit register or an incomplete
> >> register naming.
> >
> > By the values you write into it seems to be a __be16 calibration
> > register. The value to write is 0x3f8 which might ring a bell to you
> > if you know what other values related to ADC.
>
> Sigh, ok, I'll apply bulk write.
>
> How to do it correctly? I guess:
>
> __be16 buf = cpu_to_be16(GENMASK(9, 3));
Looks like that, yes.
> ret = regmap_bulk_write(yas5xx->map, YAS537_ADCCAL, &buf, 2);
sizeof(buf)
> if (ret)
> return ret;
>
> The whole block would then look like:
>
> /* Writing ADCCAL and TRM registers */
> __be16 buf = cpu_to_be16(GENMASK(9, 3));
(Taking into account that definitions are at the top of the function it would be
__be16 buf;
...
buf = cpu_to_be16(...);
> ret = regmap_bulk_write(yas5xx->map, YAS537_ADCCAL, &buf, 2);
> if (ret)
> return ret;
> ret = regmap_write(yas5xx->map, YAS537_TRM, GENMASK(7, 0));
> if (ret)
> return ret;
...
> > To the 4100 denominator:
> > https://github.com/XPerience-AOSP-Lollipop/android_kernel_wingtech_msm8916/blob/xpe-11.1/drivers/input/misc/yas_mag_drv-yas537.c#L235,
> > seems you can find a lot by browsing someone's code and perhaps a Git
> > history.
>
> I've seen that comment before but I don't understand its meaning.
It points out that there is a SMPLTIM, which I decode as Sample Time,
which is in 4.1 msec steps (up to 255 steps).
...
> >> Still I didn't get your comment. Is your intention to change the "50
> >> milliseconds * 1000" to "50000 microseconds" in the define?
> >>
> >> It would look like ...
> >>
> >> #define YAS537_DEFAULT_SENSOR_DELAY_US 50000
> >>
> >> ... though I would prefer to keep current define, as it is implemented
> >> now and stated above:
> >>
> >> #define YAS537_DEFAULT_SENSOR_DELAY_MS 50
> >
> > No, just to show in the actual calculation that you convert MS to US
> > using MILLI.
>
> Sorry, I still don't get what you want me to do. What do you mean by
> "using MILLI", can you elaborate?
You use formula x * 1000 to convert milliseconds to microseconds. My
suggestion is to replace 1000 with MILLI which adds information about
exponent sign, i.e. 10^-3 (which may be important to the reader).
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v4 10/10] iio: magnetometer: yas530: Add YAS537 variant
2022-07-30 11:32 ` Andy Shevchenko
@ 2022-07-30 13:31 ` Jakob Hauser
2022-07-30 16:36 ` Andy Shevchenko
0 siblings, 1 reply; 39+ messages in thread
From: Jakob Hauser @ 2022-07-30 13:31 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Jonathan Cameron, Lars-Peter Clausen, Linus Walleij,
Hans de Goede, linux-iio, devicetree, phone-devel,
~postmarketos/upstreaming
Hi Andy,
On 30.07.22 13:32, Andy Shevchenko wrote:
> On Sat, Jul 30, 2022 at 1:10 AM Jakob Hauser <jahau@rocketmail.com> wrote:
>> On 29.07.22 19:24, Andy Shevchenko wrote:
>>> On Fri, Jul 29, 2022 at 1:13 AM Jakob Hauser <jahau@rocketmail.com> wrote:
>
> ..
>
>>>> We don't know for sure whether it is a 16-bit register or an incomplete
>>>> register naming.
>>>
>>> By the values you write into it seems to be a __be16 calibration
>>> register. The value to write is 0x3f8 which might ring a bell to you
>>> if you know what other values related to ADC.
>>
>> Sigh, ok, I'll apply bulk write.
>>
>> How to do it correctly? I guess:
>>
>> __be16 buf = cpu_to_be16(GENMASK(9, 3));
>
> Looks like that, yes.
>
>> ret = regmap_bulk_write(yas5xx->map, YAS537_ADCCAL, &buf, 2);
>
> sizeof(buf)
OK
>> if (ret)
>> return ret;
>>
>> The whole block would then look like:
>>
>> /* Writing ADCCAL and TRM registers */
>> __be16 buf = cpu_to_be16(GENMASK(9, 3));
>
> (Taking into account that definitions are at the top of the function it would be
>
> __be16 buf;
> ...
> buf = cpu_to_be16(...);
Thanks for the details, I'll implement it like this.
>> ret = regmap_bulk_write(yas5xx->map, YAS537_ADCCAL, &buf, 2);
>> if (ret)
>> return ret;
>> ret = regmap_write(yas5xx->map, YAS537_TRM, GENMASK(7, 0));
>> if (ret)
>> return ret;
>
> ..
>
>>> To the 4100 denominator:
>>> https://github.com/XPerience-AOSP-Lollipop/android_kernel_wingtech_msm8916/blob/xpe-11.1/drivers/input/misc/yas_mag_drv-yas537.c#L235,
>>> seems you can find a lot by browsing someone's code and perhaps a Git
>>> history.
>>
>> I've seen that comment before but I don't understand its meaning.
>
> It points out that there is a SMPLTIM, which I decode as Sample Time,
> which is in 4.1 msec steps (up to 255 steps).
Also thanks for this interpretation, that makes sense. Then the
denominator consists of factor 1000 to convert microseconds back to
milliseconds and a factor of 4.1 milliseconds per step. The value
"intrvl", which is written into the YAS537_MEASURE_INTERVAL register,
would then be the number of steps of the sample time.
However, I wouldn't add anything of this into the driver as a comment or
as a name, because we're just guessing.
>
> ..
>
>>>> Still I didn't get your comment. Is your intention to change the "50
>>>> milliseconds * 1000" to "50000 microseconds" in the define?
>>>>
>>>> It would look like ...
>>>>
>>>> #define YAS537_DEFAULT_SENSOR_DELAY_US 50000
>>>>
>>>> ... though I would prefer to keep current define, as it is implemented
>>>> now and stated above:
>>>>
>>>> #define YAS537_DEFAULT_SENSOR_DELAY_MS 50
>>>
>>> No, just to show in the actual calculation that you convert MS to US
>>> using MILLI.
>>
>> Sorry, I still don't get what you want me to do. What do you mean by
>> "using MILLI", can you elaborate?
>
> You use formula x * 1000 to convert milliseconds to microseconds. My
> suggestion is to replace 1000 with MILLI which adds information about
> exponent sign, i.e. 10^-3 (which may be important to the reader).
Hm, ok, but the MILLI has to be defined. Or is it predefined somewhere?
I couldn't find any examples.
To my interpretation, it would look like this (upper part showing the
location of the define, lower part the formula):
...
#define YAS537_LCK_MASK_GET GENMASK(3, 0)
#define YAS537_OC_MASK_GET GENMASK(5, 0)
#define MILLI 1000
/* Turn off device regulators etc after 5 seconds of inactivity */
#define YAS5XX_AUTOSUSPEND_DELAY_MS 5000
enum chip_ids {
...
};
...
intrvl = (YAS537_DEFAULT_SENSOR_DELAY_MS * MILLI
- YAS537_MEASURE_TIME_WORST_US) / 4100;
I think the define and the formula both look strange.
Kind regards,
Jakob
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v4 10/10] iio: magnetometer: yas530: Add YAS537 variant
2022-07-30 13:31 ` Jakob Hauser
@ 2022-07-30 16:36 ` Andy Shevchenko
2022-07-31 17:53 ` Jakob Hauser
2022-08-03 18:27 ` Linus Walleij
0 siblings, 2 replies; 39+ messages in thread
From: Andy Shevchenko @ 2022-07-30 16:36 UTC (permalink / raw)
To: Jakob Hauser
Cc: Jonathan Cameron, Lars-Peter Clausen, Linus Walleij,
Hans de Goede, linux-iio, devicetree, phone-devel,
~postmarketos/upstreaming
On Sat, Jul 30, 2022 at 3:32 PM Jakob Hauser <jahau@rocketmail.com> wrote:
> On 30.07.22 13:32, Andy Shevchenko wrote:
> > On Sat, Jul 30, 2022 at 1:10 AM Jakob Hauser <jahau@rocketmail.com> wrote:
> >> On 29.07.22 19:24, Andy Shevchenko wrote:
> >>> On Fri, Jul 29, 2022 at 1:13 AM Jakob Hauser <jahau@rocketmail.com> wrote:
...
> >>> To the 4100 denominator:
> >>> https://github.com/XPerience-AOSP-Lollipop/android_kernel_wingtech_msm8916/blob/xpe-11.1/drivers/input/misc/yas_mag_drv-yas537.c#L235,
> >>> seems you can find a lot by browsing someone's code and perhaps a Git
> >>> history.
> >>
> >> I've seen that comment before but I don't understand its meaning.
> >
> > It points out that there is a SMPLTIM, which I decode as Sample Time,
> > which is in 4.1 msec steps (up to 255 steps).
>
> Also thanks for this interpretation, that makes sense. Then the
> denominator consists of factor 1000 to convert microseconds back to
> milliseconds and a factor of 4.1 milliseconds per step. The value
> "intrvl", which is written into the YAS537_MEASURE_INTERVAL register,
> would then be the number of steps of the sample time.
>
> However, I wouldn't add anything of this into the driver as a comment or
> as a name, because we're just guessing.
Or we can precisely tell that this is guesswork. Up to you.
...
> I think the define and the formula both look strange.
Definition is available in units.h, for most of the SI prefixes.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v4 10/10] iio: magnetometer: yas530: Add YAS537 variant
2022-07-30 16:36 ` Andy Shevchenko
@ 2022-07-31 17:53 ` Jakob Hauser
2022-08-03 18:27 ` Linus Walleij
1 sibling, 0 replies; 39+ messages in thread
From: Jakob Hauser @ 2022-07-31 17:53 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Jonathan Cameron, Lars-Peter Clausen, Linus Walleij,
Hans de Goede, linux-iio, devicetree, phone-devel,
~postmarketos/upstreaming
Hi Andy,
On 30.07.22 18:36, Andy Shevchenko wrote:
> On Sat, Jul 30, 2022 at 3:32 PM Jakob Hauser <jahau@rocketmail.com> wrote:
>> On 30.07.22 13:32, Andy Shevchenko wrote:
>>> On Sat, Jul 30, 2022 at 1:10 AM Jakob Hauser <jahau@rocketmail.com> wrote:
>>>> On 29.07.22 19:24, Andy Shevchenko wrote:
>
> ..
>
>>>>> To the 4100 denominator:
>>>>> https://github.com/XPerience-AOSP-Lollipop/android_kernel_wingtech_msm8916/blob/xpe-11.1/drivers/input/misc/yas_mag_drv-yas537.c#L235,
>>>>> seems you can find a lot by browsing someone's code and perhaps a Git
>>>>> history.
>>>>
>>>> I've seen that comment before but I don't understand its meaning.
>>>
>>> It points out that there is a SMPLTIM, which I decode as Sample Time,
>>> which is in 4.1 msec steps (up to 255 steps).
>>
>> Also thanks for this interpretation, that makes sense. Then the
>> denominator consists of factor 1000 to convert microseconds back to
>> milliseconds and a factor of 4.1 milliseconds per step. The value
>> "intrvl", which is written into the YAS537_MEASURE_INTERVAL register,
>> would then be the number of steps of the sample time.
>>
>> However, I wouldn't add anything of this into the driver as a comment or
>> as a name, because we're just guessing.
>
> Or we can precisely tell that this is guesswork. Up to you.
I would keep it as it is. It has no direct relevance.
>
> ..
>
>> I think the define and the formula both look strange.
>
> Definition is available in units.h, for most of the SI prefixes.
>
Ah, thanks, I didn't find that myself. Sorry for my incomprehension.
OK, everything clarified. I'll prepare v5 within the next days.
Kind regards,
Jakob
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v4 10/10] iio: magnetometer: yas530: Add YAS537 variant
2022-07-30 16:36 ` Andy Shevchenko
2022-07-31 17:53 ` Jakob Hauser
@ 2022-08-03 18:27 ` Linus Walleij
1 sibling, 0 replies; 39+ messages in thread
From: Linus Walleij @ 2022-08-03 18:27 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Jakob Hauser, Jonathan Cameron, Lars-Peter Clausen,
Hans de Goede, linux-iio, devicetree, phone-devel,
~postmarketos/upstreaming
On Sat, Jul 30, 2022 at 6:36 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> > I think the define and the formula both look strange.
>
> Definition is available in units.h, for most of the SI prefixes.
WHoa news2me, I never saw that file before! Learn something new every day....
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 39+ messages in thread
end of thread, other threads:[~2022-08-03 18:27 UTC | newest]
Thread overview: 39+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <cover.1656883851.git.jahau.ref@rocketmail.com>
2022-07-03 22:02 ` [PATCH v4 00/10] Add support for magnetometer Yamaha YAS537 Jakob Hauser
2022-07-03 22:02 ` [PATCH v4 01/10] iio: magnetometer: yas530: Change data type of hard_offsets to signed Jakob Hauser
2022-07-03 22:02 ` [PATCH v4 02/10] iio: magnetometer: yas530: Change range of data in volatile register Jakob Hauser
2022-07-03 22:02 ` [PATCH v4 03/10] iio: magnetometer: yas530: Correct scaling of magnetic axes Jakob Hauser
2022-07-03 22:02 ` [PATCH v4 04/10] iio: magnetometer: yas530: Correct temperature handling Jakob Hauser
2022-07-03 22:02 ` [PATCH v4 05/10] iio: magnetometer: yas530: Change data type of calibration coefficients Jakob Hauser
2022-07-03 22:02 ` [PATCH v4 06/10] iio: magnetometer: yas530: Rename functions and registers Jakob Hauser
2022-07-04 18:04 ` Andy Shevchenko
2022-07-26 21:40 ` Jakob Hauser
2022-07-03 22:02 ` [PATCH v4 07/10] iio: magnetometer: yas530: Move printk %*ph parameters out from stack Jakob Hauser
2022-07-04 18:06 ` Andy Shevchenko
2022-07-03 22:02 ` [PATCH v4 08/10] iio: magnetometer: yas530: Apply documentation and style fixes Jakob Hauser
2022-07-04 18:07 ` Andy Shevchenko
2022-07-04 23:29 ` Linus Walleij
2022-07-03 22:04 ` [PATCH v4 09/10] iio: magnetometer: yas530: Introduce "chip_info" structure Jakob Hauser
2022-07-04 19:37 ` Andy Shevchenko
2022-07-16 17:10 ` Jonathan Cameron
2022-07-26 22:01 ` Jakob Hauser
2022-07-27 17:39 ` Andy Shevchenko
2022-07-28 23:05 ` Jakob Hauser
2022-07-29 16:08 ` Andy Shevchenko
2022-07-29 22:52 ` Jakob Hauser
2022-07-29 16:13 ` Andy Shevchenko
2022-07-29 22:56 ` Jakob Hauser
2022-07-30 0:53 ` Andy Shevchenko
2022-07-03 22:05 ` [PATCH v4 10/10] iio: magnetometer: yas530: Add YAS537 variant Jakob Hauser
2022-07-04 19:47 ` Andy Shevchenko
2022-07-26 22:13 ` Jakob Hauser
2022-07-27 17:46 ` Andy Shevchenko
2022-07-28 23:13 ` Jakob Hauser
2022-07-29 17:24 ` Andy Shevchenko
2022-07-29 23:10 ` Jakob Hauser
2022-07-30 11:32 ` Andy Shevchenko
2022-07-30 13:31 ` Jakob Hauser
2022-07-30 16:36 ` Andy Shevchenko
2022-07-31 17:53 ` Jakob Hauser
2022-08-03 18:27 ` Linus Walleij
2022-07-04 23:31 ` [PATCH v4 00/10] Add support for magnetometer Yamaha YAS537 Linus Walleij
2022-07-16 17:05 ` Jonathan Cameron
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).