Hi Andy, On 27.07.22 19:46, Andy Shevchenko wrote: > On Wed, Jul 27, 2022 at 12:13 AM Jakob Hauser <jahau@xxxxxxxxxxxxxx> wrote: >> On 04.07.22 21:47, Andy Shevchenko wrote: >>> On Mon, Jul 4, 2022 at 12:05 AM Jakob Hauser <jahau@xxxxxxxxxxxxxx> 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
