On Mon, Jul 4, 2022 at 12:05 AM Jakob Hauser <jahau@xxxxxxxxxxxxxx> wrote: > > This adds support for the magnetometer Yamaha YAS537. The additions are based > on comparison of Yamaha Android kernel drivers for YAS532 [1] and YAS537 [2]. > > In the Yamaha YAS537 Android driver, there is an overflow/underflow control > implemented. For regular usage, this seems not necessary. A similar overflow/ > underflow control of Yamaha YAS530/532 Android driver isn't integrated in the > mainline driver. It is therefore skipped for YAS537 in mainline too. > > Also in the Yamaha YAS537 Android driver, at the end of the reset_yas537() > function, a measurement is saved in "last_after_rcoil". Later on, this is > compared to current measurements. If the difference gets too big, a new > reset is initialized. The difference in measurements needs to be quite big, > it's hard to say if this is necessary for regular operation. Therefore this > isn't integrated in the mainline driver either. > > [1] https://github.com/msm8916-mainline/android_kernel_qcom_msm8916/blob/GT-I9195I/drivers/iio/magnetometer/yas_mag_drv-yas532.c > [2] https://github.com/msm8916-mainline/android_kernel_qcom_msm8916/blob/GT-I9195I/drivers/iio/magnetometer/yas_mag_drv-yas537.c Much better, my comments below. ... > yas530, > yas532, > yas533, > + yas537, > }; > > static const char yas5xx_product_name[][13] = { > "YAS530 MS-3E", > "YAS532 MS-3R", > - "YAS533 MS-3F" > + "YAS533 MS-3F", This is exactly the point why it's good practice to add comma for non-terminator entries. > + "YAS537 MS-3T" ...here. > }; > > static const char yas5xx_version_name[][2][3] = { > { "A", "B" }, > { "AB", "AC" }, > - { "AB", "AC" } > + { "AB", "AC" }, Ditto. > + { "v0", "v1" } > }; ... > + /* Write registers according to Android driver */ Would be nice to elaborate in the comment what exactly the flow is here, like "a) setting value of X; b) ...". ... > + ret = regmap_write(yas5xx->map, YAS537_ADCCAL, GENMASK(1, 0)); > + if (ret) > + return ret; > + ret = regmap_write(yas5xx->map, YAS537_ADCCAL + 1, GENMASK(7, 3)); > + if (ret) > + return ret; Can bulk write be used here? ... > + /* The interval value is static in regular operation */ > + intrvl = (YAS537_DEFAULT_SENSOR_DELAY_MS * 1000 MILLI ? > + - YAS537_MEASURE_TIME_WORST_US) / 4100; ... > + }, > } And this is for chip_info and comma for non-terminator entries (see above). ... > - ret = yas5xx->chip_info->measure_offsets(yas5xx); > - if (ret) > - goto assert_reset; > + if (yas5xx->chip_info->measure_offsets) { This can be done when you introduce this callback in the chip_info structure, so this patch won't need to shuffle code again. I.o.w. we can reduce ping-pong development in this series. > + ret = yas5xx->chip_info->measure_offsets(yas5xx); > + if (ret) > + goto assert_reset; > + } -- With Best Regards, Andy Shevchenko
