| Message ID | cover.1656883851.git.jahau@rocketmail.com |
|---|---|
| Headers | show |
| Series | Add support for magnetometer Yamaha YAS537 | expand |
On Mon, Jul 4, 2022 at 12:03 AM Jakob Hauser <jahau@rocketmail.com> wrote: > > Use less stack by modifying %*ph parameters. > > Additionally, in the function yas530_get_calibration_data(), the debug dump was > extended to 16 elements as this is the size of the calibration data array of > YAS530. Suggested-by?
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; > + }
On Mon, Jul 4, 2022 at 12:03 AM Jakob Hauser <jahau@rocketmail.com> wrote: > This patchset adds YAS537 variant to the already existing driver for > Yamaha YAS magnetometers. > > Patch 1 is a fix on the current driver. > Patches 2-9 are cleanups and refactoring. > Patch 10 finally adds the YAS537 variant. This patch set is really nice and getting nicer. Maybe Jonathan could apply patches 1-5 so you don't have to resend so much code and get more focus on the top 5 patches? They are anyway nice in their own right. Yours, Linus Walleij
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
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; > >> + }
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
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
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).
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
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.
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
This patchset adds YAS537 variant to the already existing driver for Yamaha YAS magnetometers. Patch 1 is a fix on the current driver. Patches 2-9 are cleanups and refactoring. Patch 10 finally adds the YAS537 variant. Patch 9 adds the "chip_info" approach. As this change is quite extensive, it might need some more discussion and refinement. Also the patchset as such. However, applying that patch late in the patchset allows a good traceability how the other changes were introduced. By adding patch 9 (chip_info), some of the review comments on the last patchset became obsolete: - Patch 8 v3: Comment "Can't return here, because you leave the PM counters imbalanced." - Patch 8 v3: Comment on strncpy() vs. strscpy(), - Patch 8 v3: "fall through" comment at function yas5xx_volatile_reg(). - Patch 8 v3: Empty line between "break" and "default" within switch statement of function yas5xx_probe(). Changes in v4: - Rebased to torvalds/linux v5.19-rc4, as this now includes Linus' patch "Fix memchr_inv() misuse" on driver yamaha-yas530. - Removed redundant Cc tags. - Patch 2: Replaced "<= ... + 7" by "< ... + 8" and adapted commit message. - Patch 3: Added default for switch statement, I forgot to add this. - Patch 4: In function yas5xx_get_measure(), changed wording "milli degrees" to "millidegrees". - Patch 6: Changed the renaming of function from "yas530_532_" to "yas530_". Same for registers. Added additional comments where appropriate. - Patch 6: Removed "Reviewed-by:" tag of Andy. - Split patch 7 v3 into two patches -> patch 7 v4 and patch 8 v4. - Patch 8: Applied "if (a && b)" suggestion at memchr_inv() by Andy in function yas532_get_calibration_data(). - Patch 8: Removed defines for device IDs YAS537 and YAS539 and accordingly the comment "These variant IDs are known from code dumps". - Patch 9: New patch to introduce the "chip_info" approach. - Patch 10: In function yas537_get_calibration_data(), removed "the exact meaning is unknown" from comment "Writing SRST register". - Patch 10: Also applied "if (a && b)" suggestion at memchr_inv() by Andy in function yas537_get_calibration_data(). Additionally changed the second condition from "== 0" to "!". - Patch 10: In function yas537_get_calibration_data(), removed empty lines within switch statement. In that context, removed comment "Get data into these four blocks val1 to val4". - Patch 10: In Kconfig, simplified wording. - Patch 10: In function yas537_get_calibration_data() "case YAS537_VERSION_0", reduced indent of "for" loop by splitting it into multiple loops. I didn't use integer j, as it was suggested by Jonathan, because only using integer i is more consistant with the loop in "case YAS537_VERSION_1". - Accordingly, split the "for" loop in "case YAS537_VERSION_1" into two loops as well. Technically this isn't neccessary but it improves readability. - Patch 10: Added new defines of masks for YAS537 and applied these by FIELD_PREP and FIELD_GET in function yas537_get_calibration_data() within "case YAS537_VERSION_1". - Patch 10: In function yas537_power_on(), added spaced at "YAS537_ADCCAL + 1". Changes in v3: - In patch 3 fixed 2x typo "Divide". - In commit message of patch 4 fixed wording "in the yas5xx_get_measure() function". - In patch 4 in the comment for the temperature calculation fixed wording "is the number of counts". - In patch 4 added defaults to switch statements. - Splitted stray changes into new patch 7 v3. "Add YAS537 variant" is now patch 8 v3. I haven't added "Reviewed-by:" tag of Linus to patch 7 v3 because as a separate patch these changes appear in a different context. - In new patch 7 v3, changed printk format specifiers in the function yas530_get_calibration_data() to "%16ph" and in the function yas532_get_calibration_data() to "%14ph". The first one is also a minor correction in behaviour, as the calibration data array size of YAS530 is 16 (the dev_dbg printed 14 before). - Rebased to linux-next to include patch bb52d3691db8 "iio: magnetometer: yas530: Fix memchr_inv() misuse". - In patch 7 v3, changed memchr_inv() line for YAS532. - In patch 8 v3 in the function yas537_get_calibration_data(), changed memchr_inv() line for YAS537. - Removed comment "corresponds to 0x70" at define YAS537_MAG_AVERAGE_32_MASK. - Added suffixes _US and _MS in defines for YAS537. - In the function yas537_measure(), removed comments "Read data", "Arrange data", "Assign data". - In the function yas537_measure(), replaced bitwise shift by get_unaligned_be16(). - Replaced "if (h[i] < -8192)" etc. by clamp_val(). - In the functions yas537_measure() and yas537_get_measure(), replaced 8192 by BIT(13) and 16384 by BIT(14). - Fixed typo "resolution" in the function yas5xx_read_raw(). - Fixed typo "Divide" in patch 8 v3 in the function yas5xx_read_raw(). - In patch 8 v3 in the yas537_get_calibration_data(), changed printk format specifier to "%17ph" - In the functions yas537_measure() and yas537_get_calibration_data(), drop some parentheses in regmap_write(). - In the function yas537_power_on(), added comment "Wait until the coil has ramped up". - In the function yas5xx_probe(), put YAS537 variant and version printings into one print. - In the function yas537_get_measure(), fixed wording "is the number of counts" in the comment for the temperature calculation. - In the function yas537_get_measure(), added product description document No. into the comment for the temperature calculation (as I first thought the review comment "the number" is related to this). - In the function yas537_get_calibration_data(), corrected comment "Get data into these four blocks val1 to val4". Changes in v2: - Reordered the patchset by moving patch 4 v1 to patch 1 v2. - Removed patch 6 v1 ("Remove redundant defaults on switch devid") - Accordingly, added "default:" to each switch statement in patch 7. - Moved renamings in patch 7 v1 into a separate new patch 6 v2. I added the "Reviewed-by:" tag of Linus to both patches, hope that's ok, else feel free to comment. - Removed regmap reads and related debug dumps in patch 7 in function yas537_dump_calibration(). As this function now applies to version 1 only, replaced switch statement by if clause. - Also removed "hard_offsets" debug dumps in that function. - Fixed typo "initialized" in commit message of patch 7. Jakob Hauser (10): iio: magnetometer: yas530: Change data type of hard_offsets to signed iio: magnetometer: yas530: Change range of data in volatile register iio: magnetometer: yas530: Correct scaling of magnetic axes iio: magnetometer: yas530: Correct temperature handling iio: magnetometer: yas530: Change data type of calibration coefficients iio: magnetometer: yas530: Rename functions and registers iio: magnetometer: yas530: Move printk %*ph parameters out from stack iio: magnetometer: yas530: Apply documentation and style fixes iio: magnetometer: yas530: Introduce "chip_info" structure iio: magnetometer: yas530: Add YAS537 variant drivers/iio/magnetometer/Kconfig | 4 +- drivers/iio/magnetometer/yamaha-yas530.c | 808 +++++++++++++++++++---- 2 files changed, 680 insertions(+), 132 deletions(-)