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
