diff mbox series

[v4,10/10] iio: magnetometer: yas530: Add YAS537 variant

Message ID 69a512cf5b62b34415d5983a6406c3d5ba438a1d.1656883851.git.jahau@rocketmail.com (mailing list archive)
State Changes Requested
Headers show
Series Add support for magnetometer Yamaha YAS537 | expand

Commit Message

Jakob Hauser July 3, 2022, 10:05 p.m. UTC
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(-)

Comments

Andy Shevchenko July 4, 2022, 7:47 p.m. UTC | #1
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;
> +       }
Jakob Hauser July 26, 2022, 10:13 p.m. UTC | #2
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
Andy Shevchenko July 27, 2022, 5:46 p.m. UTC | #3
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;
> >> +       }
Jakob Hauser July 28, 2022, 11:13 p.m. UTC | #4
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
Andy Shevchenko July 29, 2022, 5:24 p.m. UTC | #5
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.
Jakob Hauser July 29, 2022, 11:10 p.m. UTC | #6
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
Andy Shevchenko July 30, 2022, 11:32 a.m. UTC | #7
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).
Jakob Hauser July 30, 2022, 1:31 p.m. UTC | #8
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
Andy Shevchenko July 30, 2022, 4:36 p.m. UTC | #9
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.
Jakob Hauser July 31, 2022, 5:53 p.m. UTC | #10
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
Linus Walleij Aug. 3, 2022, 6:27 p.m. UTC | #11
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
diff mbox series

Patch

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