Add support for magnetometer Yamaha YAS537

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.

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

+static int yas530_measure(struct yas5xx *yas5xx, u16 *t, u16 *x, u16 *y1, u16 *y2)

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

+static int yas530_get_measure(struct yas5xx *yas5xx, s32 *to, s32 *xo, s32 *yo, s32 *zo)

+static void yas530_extract_calibration(u8 *data, struct yas5xx_calibration *c)

Quoted 1 line · Jakob Hauser

Quoted 6 lines

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

drivers/iio/magnetometer/yamaha-yas530.c static u16 yas532_extract_axis(u8 *data) · line 195

Quoted 6 lines

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

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

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.

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 <[email protected]>

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

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

index 8358d4b3d1e2..4e2f460a4efd 100644
@@ -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

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 <[email protected]>

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

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

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

+};

+};

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

+       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 = yas5xx->chip_info->volatile_reg_qty;

drivers/iio/magnetometer/yamaha-yas530.c +24 -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] = {

+};

                goto assert_reset;
        }

Quoted 5 lines · 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"),

Device Tree

Quoted 2 lines

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.

Quoted 1 line · Jakob Hauser

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.

drivers/iio/magnetometer/yamaha-yas530.c

Quoted 3 lines

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

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

Quoted 10 lines

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

+enum chip_ids {
+       yas530,
+       yas532,
+       yas533,
+};
+
+static const char yas5xx_product_name[][13] = {
+       "YAS530 MS-3E",
+       "YAS532 MS-3R",
+       "YAS533 MS-3F"

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

Quoted 6 lines

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

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

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

Quoted 3 lines

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

+static const int yas530_volatile_reg[] = {
+       YAS530_ACTUATE_INIT_COIL,
+       YAS530_MEASURE

+ Comma.

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

Quoted 5 lines

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

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

drivers/iio/magnetometer/yamaha-yas530.c struct yas5xx_calibration { · line 158

Quoted 6 lines

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

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

Quoted 1 line

+ int i, j;

j can have a proper name.

Quoted 1 line

+ j = yas5xx->chip_info->volatile_reg_qty;

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

Quoted 2 lines

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

-       int ret;
+       int id_check, ret;

Don't add variables with different semantics on the same line.

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

Quoted 5 lines

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

+       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() ?

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

Quoted 2 lines

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

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

Quoted 1 line · Andy Shevchenko

On Mon, Jul 4, 2022 at 12:04 AM Jakob Hauser <[email protected]> wrote:

Quoted 4 lines

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.

Quoted 5 lines

drivers/iio/magnetometer/yamaha-yas530.c

Quoted 3 lines

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

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

Quoted 6 lines

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

Quoted 4 lines

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

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

Quoted 1 line

Also, use * instead of one dimension of array.

Sorry, probably I lack the basic knowledge here. Can you explain how to do that?

Quoted 7 lines

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

Quoted 5 lines

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

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

Quoted 9 lines

drivers/iio/magnetometer/yamaha-yas530.c struct yas5xx_calibration { · line 158

Quoted 6 lines

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

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

Quoted 7 lines

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

Quoted 5 lines

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

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

Quoted 4 lines

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

Quoted 2 lines

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

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

Quoted 2 lines · Jakob Hauser

Quoted 1 line · Andy Shevchenko

On Mon, Jul 4, 2022 at 12:04 AM Jakob Hauser <[email protected]> wrote:

..

Quoted 7 lines

Quoted 4 lines

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.

Quoted 38 lines

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

Quoted 2 lines

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.

Quoted 8 lines

Quoted 5 lines

drivers/iio/magnetometer/yamaha-yas530.c

Quoted 3 lines

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

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

..

Quoted 4 lines

Quoted 1 line

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[][] = {
  };

?

Quoted 28 lines

Quoted 7 lines

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

Quoted 5 lines

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

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

Quoted 7 lines

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

Quoted 61 lines

Quoted 9 lines

drivers/iio/magnetometer/yamaha-yas530.c struct yas5xx_calibration { · line 158

Quoted 6 lines

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

+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] = {

                        ...

Quoted 4 lines

                },
        };

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.

Quoted 40 lines

Quoted 7 lines

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

Quoted 5 lines

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

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

Quoted 2 lines

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, ...);

Quoted 67 lines

Quoted 4 lines

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

Quoted 2 lines

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

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

Quoted 2 lines

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?

Quoted 4 lines

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.

Quoted 1 line · Andy Shevchenko

On Wed, Jul 27, 2022 at 12:01 AM Jakob Hauser <[email protected]> wrote:

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

Quoted 4 lines

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:

        dev_info(dev, "detected %s %s\n",
                 yas5xx->chip_info->product_name,
                 yas5xx->chip_info->version_name[yas5xx->version]);

On Fri, Jul 29, 2022 at 1:06 AM Jakob Hauser <[email protected]> wrote:

Quoted 2 lines

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.

On 27.07.22 19:39, Andy Shevchenko wrote:

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

                },
        };

On Fri, Jul 29, 2022 at 1:06 AM Jakob Hauser <[email protected]> wrote:

..

Just a couple of remarks.

Quoted 8 lines

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",
        };

Quoted 1 line

        static const char * const yas5xx_version_name[][2] = {

yas5xx_version_names (note S at the end)

Quoted 15 lines

                [yas530] = { "A", "B" },
                [yas532] = { "AB", "AC" },
                [yas533] = { "AB", "AC" },
        };

        ...

        struct yas5xx_chip_info {
                ...
                const char *product_name;
                const char * const *version_name;
                ...
        };

Quoted 5 lines

        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)

Quoted 4 lines

                },
        };

Quoted 1 line · Andy Shevchenko

On Fri, Jul 29, 2022 at 1:06 AM Jakob Hauser <[email protected]> wrote:

Quoted 3 lines

Quoted 1 line

        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

Quoted 4 lines

Quoted 4 lines

                [yas530] = { "A", "B" },
                [yas532] = { "AB", "AC" },
                [yas533] = { "AB", "AC" },
        };

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

+ "YAS537 MS-3T"

+ { "v0", "v1" }
};

+ /* Write registers according to Android driver */

+ - YAS537_MEASURE_TIME_WORST_US) / 4100;

+ },

        }

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

+               ret = yas5xx->chip_info->measure_offsets(yas5xx);
+               if (ret)
+                       goto assert_reset;
+       }

Quoted 1 line · Jakob Hauser

drivers/iio/magnetometer/yamaha-yas530.c enum chip_ids { · line 144

Quoted 11 lines

        yas530,
        yas532,
        yas533,

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

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

Quoted 1 line

+ "YAS537 MS-3T"

..here.

Quoted 1 line

};

Quoted 1 line

+ /* 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) ...".

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

Quoted 6 lines

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

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

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

Quoted 2 lines

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

+       /* The interval value is static in regular operation */
+       intrvl = (YAS537_DEFAULT_SENSOR_DELAY_MS * 1000

MILLI ?

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

Quoted 5 lines

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

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

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

Quoted 4 lines

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

+               ret = yas5xx->chip_info->measure_offsets(yas5xx);
+               if (ret)
+                       goto assert_reset;
+       }

Quoted 1 line · Andy Shevchenko

On Mon, Jul 4, 2022 at 12:05 AM Jakob Hauser <[email protected]> wrote:

Quoted 4 lines

Quoted 1 line

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

Quoted 7 lines

Quoted 6 lines

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

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

Quoted 1 line

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.

Quoted 4 lines

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

Quoted 2 lines

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

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

Quoted 9 lines

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

Quoted 5 lines

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

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

Quoted 4 lines

Quoted 4 lines

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

+               ret = yas5xx->chip_info->measure_offsets(yas5xx);
+               if (ret)
+                       goto assert_reset;
+       }

Quoted 2 lines · Jakob Hauser

Quoted 1 line · Andy Shevchenko

On Mon, Jul 4, 2022 at 12:05 AM Jakob Hauser <[email protected]> wrote:

..

Quoted 12 lines

Quoted 4 lines

Quoted 1 line

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

Quoted 2 lines

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.

Quoted 7 lines

Quoted 7 lines

Quoted 6 lines

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

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

Quoted 8 lines

Quoted 1 line

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.

Quoted 5 lines

Quoted 4 lines

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

Quoted 2 lines

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

+       /* The interval value is static in regular operation */
+       intrvl = (YAS537_DEFAULT_SENSOR_DELAY_MS * 1000

MILLI ?

Quoted 3 lines

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.

Quoted 10 lines

Quoted 9 lines

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

Quoted 5 lines

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

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

Quoted 5 lines

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.

On 27.07.22 19:46, Andy Shevchenko wrote:

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

Quoted 4 lines

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.

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

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

Quoted 1 line · Jakob Hauser

On 27.07.22 19:46, Andy Shevchenko wrote:

..

Quoted 3 lines

/*
* Write registers according to Android driver, the exact meaning
* is unknown

With a period at the end :-)

Quoted 6 lines

*/

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

Quoted 3 lines

Accordingly, I would choose the following comment here:

/* Writing ADCCAL and TRM registers */

Fine with me!

..

Quoted 5 lines

Quoted 4 lines

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.

Quoted 2 lines

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.

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.

Quoted 7 lines

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

Quoted 4 lines

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

Quoted 2 lines · Andy Shevchenko

Quoted 1 line · Jakob Hauser

On 27.07.22 19:46, Andy Shevchenko wrote:

Quoted 6 lines

Quoted 2 lines

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;

Quoted 4 lines

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.

Quoted 10 lines

Quoted 10 lines

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:

Quoted 4 lines

Quoted 1 line

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

Quoted 2 lines · Jakob Hauser

Quoted 1 line · Andy Shevchenko

On Fri, Jul 29, 2022 at 1:13 AM Jakob Hauser <[email protected]> wrote:

..

Quoted 11 lines

Quoted 6 lines

Quoted 2 lines

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:

Quoted 1 line

        __be16 buf = cpu_to_be16(GENMASK(9, 3));

Looks like that, yes.

Quoted 1 line

        ret = regmap_bulk_write(yas5xx->map, YAS537_ADCCAL, &buf, 2);

sizeof(buf)

Quoted 5 lines

        if (ret)
                return ret;

The whole block would then look like:

Quoted 2 lines

        /* 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(...);

Quoted 6 lines

        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;

..

Quoted 5 lines

Quoted 4 lines

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.

Quoted 1 line

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

..

Quoted 15 lines

Quoted 14 lines

Quoted 11 lines

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.

Quoted 2 lines

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

Quoted 3 lines · Andy Shevchenko

Quoted 2 lines · Jakob Hauser

Quoted 1 line · Andy Shevchenko

On Fri, Jul 29, 2022 at 1:13 AM Jakob Hauser <[email protected]> wrote:

Quoted 9 lines

Quoted 6 lines

Quoted 4 lines

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.

I think the define and the formula both look strange.

Quoted 3 lines · Jakob Hauser

Quoted 2 lines · Andy Shevchenko

Quoted 1 line · Jakob Hauser

On 29.07.22 19:24, Andy Shevchenko wrote:

..

Quoted 16 lines

Quoted 9 lines

Quoted 6 lines

Quoted 4 lines

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.

Quoted 2 lines

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.

..

Quoted 1 line

I think the define and the formula both look strange.

Definition is available in units.h, for most of the SI prefixes.

Quoted 1 line

I think the define and the formula both look strange.

Definition is available in units.h, for most of the SI prefixes.

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.

Quoted 6 lines · Jakob Hauser

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

Patch 1 is a fix on the current driver.
Patches 2-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