On Sat, Jun 18, 2022 at 2:15 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. ... > -/* These variant IDs are known from code dumps */ Not sure why this change a) is here (means maybe you need to move the comment somewhere else?); b) done at all (means maybe this comment is still actual?). ... > case IIO_CHAN_INFO_RAW: > pm_runtime_get_sync(yas5xx->dev); > - ret = yas5xx_get_measure(yas5xx, &t, &x, &y, &z); > + switch (yas5xx->devid) { > + case YAS530_DEVICE_ID: > + case YAS532_DEVICE_ID: > + ret = yas530_532_get_measure(yas5xx, &t, &x, &y, &z); > + break; > + case YAS537_DEVICE_ID: > + ret = yas537_get_measure(yas5xx, &t, &x, &y, &z); > + break; > + default: > + dev_err(yas5xx->dev, "unknown device type\n"); > + return -EINVAL; Can't return here, because you leave the PM counters imbalanced. > + } > pm_runtime_mark_last_busy(yas5xx->dev); > pm_runtime_put_autosuspend(yas5xx->dev); ... > /* > - * Raw values of YAS532 are in nanotesla. Divide by > - * 100000 (10^5) to get Gauss. > + * Raw values of YAS532 and YAS537 are in nanotesla. > + * Divide by 100000 (10^5) to get Gauss. > */ Be consistent (you used milli degress above, I would suggest to use millidegrees to have it aligned with this comment spelling style). ... > + switch (yas5xx->devid) { > + case YAS530_DEVICE_ID: > + case YAS532_DEVICE_ID: > + ret = yas530_532_get_measure(yas5xx, &t, &x, &y, &z); > + break; > + case YAS537_DEVICE_ID: > + ret = yas537_get_measure(yas5xx, &t, &x, &y, &z); > + break; > + default: > + dev_err(yas5xx->dev, "unknown device type\n"); > + return; Same issue with PM counters. > + } > pm_runtime_mark_last_busy(yas5xx->dev); > pm_runtime_put_autosuspend(yas5xx->dev); ... > + switch (yas5xx->devid) { > + case YAS530_DEVICE_ID: > + case YAS532_DEVICE_ID: > + if (reg == YAS530_532_ACTUATE_INIT_COIL || > + reg == YAS530_532_MEASURE) > + return true; > + break; > + case YAS537_DEVICE_ID: > + if (reg == YAS537_MEASURE) > + return true; > + break; > + default: > + dev_err(yas5xx->dev, "unknown device type\n"); > + break; > + /* fall through */ What does this mean? It seems here you may actually to return directly. > + } > + > + return false; ... > + /* Writing SRST register, the exact meaning is unknown */ Wild guess: Software ReSeT (SRST). Would explain why it should be done before calibration. > + /* Sanity check, is this all zeroes? */ > + if (!memchr_inv(data, 0x00, 16)) { > + if (FIELD_GET(GENMASK(5, 0), data[16]) == 0) > + dev_warn(yas5xx->dev, "calibration is blank!\n"); > + } if (a) { if (b) { ... } } ===> if (a && b) { ... } ... > + case YAS537_VERSION_0: > + Redundant blank line. > + /* > + * 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 > + */ > + Ditto. > + for (i = 0; i < 17; i++) { > + if (i < 12) { > + ret = regmap_write(yas5xx->map, > + YAS537_MTC + i, > + data[i]); > + if (ret) > + return ret; > + } else if (i < 15) { > + ret = regmap_write(yas5xx->map, > + YAS537_OFFSET_X + i - 12, > + data[i]); > + if (ret) > + return ret; > + yas5xx->hard_offsets[i - 12] = data[i]; > + } else { > + ret = regmap_write(yas5xx->map, > + YAS537_HCK + i - 15, > + data[i]); > + if (ret) > + return ret; > + } > + } > + > + break; > + Ditto. ... > + case YAS537_VERSION_1: As per above. ... > + case YAS532_DEVICE_ID: > + Redundant blank line. > + if (yas5xx->devid == YAS530_DEVICE_ID) { > + ret = yas530_get_calibration_data(yas5xx); > + if (ret) > + goto assert_reset; > + dev_info(dev, "detected YAS530 MS-3E %s", > + yas5xx->version ? "B" : "A"); > + strncpy(yas5xx->name, "yas530", sizeof(yas5xx->name)); strscpy() > + } else { > + ret = yas532_get_calibration_data(yas5xx); > + if (ret) > + goto assert_reset; > + dev_info(dev, "detected YAS532/YAS533 MS-3R/F %s", > + yas5xx->version ? "AC" : "AB"); > + strncpy(yas5xx->name, "yas532", sizeof(yas5xx->name)); strscpy() > + } ... > - strncpy(yas5xx->name, "yas530", sizeof(yas5xx->name)); Okay, strncpy() -> strscpy() perhaps in the separate patch. > + case YAS537_DEVICE_ID: > + ret = yas537_get_calibration_data(yas5xx); > + if (ret) > + goto assert_reset; > + dev_info(dev, "detected YAS537 MS-3T version %s", > + yas5xx->version ? "1" : "0"); > + strncpy(yas5xx->name, "yas537", sizeof(yas5xx->name)); strscpy() ... > break; > + > default: Unneeded change. -- With Best Regards, Andy Shevchenko