Hi Andy, again on the Cc list, sorry: Looks like the part... "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS <devicetree@xxxxxxxxxxxxxxx>, Hans de Goede <hdegoede@xxxxxxxxxx>, Andy Shevchenko <andy.shevchenko@xxxxxxxxx>," ... as a text string is saved in your e-mail clients' address book as the name of the e-mail address <~postmarketos/upstreaming@xxxxxxxxxxx>. On 13.06.22 17:20, Andy Shevchenko wrote: > > On Mon, Jun 13, 2022 at 3:18 AM Jakob Hauser <jahau@xxxxxxxxxxxxxx> wrote: >> >> - * YAS537 MS-3T (2015 Samsung Galaxy S6, Note 5, Xiaomi) >> + * YAS537 MS-3T (2015 Samsung Galaxy S6, Note 5, Galaxy S7) > > Not sure what happened to Xiaomi. There is nothing in the commit > message about this change. "Xiaomi" is too generic, specific devices should be listed here. E.g. Xiaomi Redmi 2 seems to have YAS537 but I'm fully sure this applies to all its variants [1]. Samsung Galaxy S7 (and S7 edge) is often quoted in conjunction with YAS537, so I took this. Further down you suggested to move stray changes into a separate patch. I'll move this one too. [1] https://en.wikipedia.org/wiki/Redmi_2#Variants >> +#define YAS537_MAG_AVERAGE_32_MASK GENMASK(6, 4) /* corresponds to 0x70 */ > > Useless comment. The comment is related to the values in the Yamaha Android driver [2]. It depends on the "average" to be chosen [3], "YAS537_MAG_AVERAGE_32" is used for regular operation [4]. As this isn't easily comprehensible on the first sight, I added that comment. Within the mainline driver, the comment is not necessarily needed. Will remove it. [2] https://github.com/msm8916-mainline/android_kernel_qcom_msm8916/blob/GT-I9195I/drivers/iio/magnetometer/yas_mag_drv-yas537.c#L258 [3] https://github.com/msm8916-mainline/android_kernel_qcom_msm8916/blob/GT-I9195I/drivers/iio/magnetometer/yas_mag_drv-yas537.c#L63-L68 [4] https://github.com/msm8916-mainline/android_kernel_qcom_msm8916/blob/GT-I9195I/drivers/iio/magnetometer/yas_mag_drv-yas537.c#L748 >> +#define YAS537_MEASURE_TIME_WORST 1500 /* us */ >> +#define YAS537_DEFAULT_SENSOR_DELAY 50 /* ms */ >> +#define YAS537_MAG_RCOIL_TIME 65 /* us */ > > Instead of the comments, use unit suffixes, i.e. _US, _MS, _US. OK, thanks for the hint. >> + /* Read data */ > > Not sure how useful is this comment. It's part of the "process description": Read data, Arrange data, Assign data. Maybe I've overdone the commenting here, I'll remove those three comments. >> + *t = ((data[0] << 8) | data[1]); > > Looks like get_unaligned_be16(). > >> + xy1y2[0] = ((FIELD_GET(GENMASK(5, 0), data[2]) << 8) | data[3]); >> + xy1y2[1] = ((data[4] << 8) | data[5]); >> + xy1y2[2] = ((data[6] << 8) | data[7]); > > Ditto for all above. Ah, I wasn't aware about using this here. Will apply. >> + if (h[i] < -8192) >> + h[i] = -8192; > > -BIT(13) ? > >> + if (h[i] > 8191) >> + h[i] = 8191; > > Altogether seems like NIH clamp_val() or clamp_t(). I didn't know, thanks. I will change... if (h[i] < -8192) h[i] = -8192; if (h[i] > 8191) h[i] = 8191; ... into: clamp_val(h[i], -BIT(13), BIT(13) - 1); >> + xy1y2[i] = h[i] + 8192; > > BIT(13) Hm, if doing it here, it probably should be done for the whole paragraph. In that case... for (i = 0; i < 3; i++) s[i] = xy1y2[i] - 8192; h[0] = (c->k * (128 * s[0] + c->a2 * s[1] + c->a3 * s[2])) / 8192; h[1] = (c->k * (c->a4 * s[0] + c->a5 * s[1] + c->a6 * s[2])) / 8192; h[2] = (c->k * (c->a7 * s[0] + c->a8 * s[1] + c->a9 * s[2])) / 8192; for (i = 0; i < 3; i++) { if (h[i] < -8192) h[i] = -8192; if (h[i] > 8191) h[i] = 8191; xy1y2[i] = h[i] + 8192; } ... would become: for (i = 0; i < 3; i++) s[i] = xy1y2[i] - BIT(13); h[0] = (c->k * (128 * s[0] + c->a2 * s[1] + c->a3 * s[2])) / BIT(13); h[1] = (c->k * (c->a4 * s[0] + c->a5 * s[1] + c->a6 * s[2])) / BIT(13); h[2] = (c->k * (c->a7 * s[0] + c->a8 * s[1] + c->a9 * s[2])) / BIT(13); for (i = 0; i < 3; i++) { clamp_val(h[i], -BIT(13), BIT(13) - 1); xy1y2[i] = h[i] + BIT(13); } Further down in function yas537_get_measure(), there is another part... *xo = (x - 8192) * 300; *yo = (y1 - y2) * 1732 / 10; *zo = (-y1 - y2 + 16384) * 300; ... that could be changed accordingly to: *xo = (x - BIT(13)) * 300; *yo = (y1 - y2) * 1732 / 10; *zo = (-y1 - y2 + BIT(14)) * 300; Overall for me it's harder to read that way. But maybe I'm just not used to it. >> @@ -679,6 +887,7 @@ static int yas530_get_calibration_data(struct yas5xx *yas5xx) >> c->r[0] = sign_extend32(FIELD_GET(GENMASK(28, 23), val), 5); >> c->r[1] = sign_extend32(FIELD_GET(GENMASK(20, 15), val), 5); >> c->r[2] = sign_extend32(FIELD_GET(GENMASK(12, 7), val), 5); >> + >> return 0; >> } >> > > Not harmful, but a stray change. Ditto for the rest like this. I would > rather split them to a separate patch. Makes sense to split. Though the new patch will likely get a quite generic commit title like "minor cleanups". >> + dev_dbg(yas5xx->dev, "calibration data: %*ph\n", 17, data); > > Use less stack by "%17ph". OK. There is a similar line in the current driver, will change this as well in the new stray change patch. In yas530_get_calibration_data() I see a little mistake, it reads 14 fields, should be 16. I'll include this little correction in the stray change patch too. >> + /* Sanity check, is this all zeroes? */ >> + if (memchr_inv(data, 0x00, 16) == NULL) { > > if (!memchr_inv(...)) > >> + if (FIELD_GET(GENMASK(5, 0), data[16]) == 0) >> + dev_warn(yas5xx->dev, "calibration is blank!\n"); >> + } No problem to change for YAS537. At YAS530/532, there is a similar line that should be changed accordingly. However, there is a patch by Linus that was already added to "fixes-togreg" branch in iio.git quite a while ago [5]. The patch is not included in torvalds/linux v5.19-rc1 or -rc2 and neither in iio.git testing branch. So I'm unsure what I should base the patchset on if I want to change that line. I will probably choose linux-next, as the patch is included there and in Kconfig also patch "iio: magnetometer: ak8974: Drop dependency on OF" is included (which on the other hand isn't included in "fixes-togreg" branch in iio.git). [5] https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git/commit/?h=fixes-togreg&id=bb52d3691db8cf24cea049235223f3599778f264 >> + for (i = 0; i < 17; i++) { > > 16 and 17 seems like a magic that is used a lot, perhaps define? 17 is the size of the calibration data array in YAS537. Accordingly we work with array fields 0 to 16. On YAS530, the calibration data array has a size of 16, therefore working with fields 0 to 15. On YAS532 the size is 14, working with fields 0 to 13. I wouldn't use "define" as it is kind of defined by declaration "u8 data[17];" in YAS537 for example. We could probably make more use of sizeof(data). E.g. the line ... dev_dbg(yas5xx->dev, "calibration data: %*ph\n", 17, data); ... could be written as... dev_dbg(yas5xx->dev, "calibration data: %*ph\n", sizeof(data), data); ... but we wanted to change this to %17ph anyway. Also we could change the line... for (i = 0; i < 17; i++) { ... into... for (i = 0; i < sizeof(data); i++) { ... however, I wouldn't do that, as it makes readability worse for the next steps,... if (i < 12) { ... } else if (i < 15) { ... } else { ... } ... in the current implementation it's easier to see that the last one is the remaining 15 and 16. Summing up, I wouldn't change anything on the handling of the numbers 17 or 16. >> + ret = regmap_write(yas5xx->map, YAS537_MTC + 3, >> + ((data[3] & GENMASK(7, 5)) | BIT(4))); > > Here... > >> + if (ret) >> + return ret; >> + ret = regmap_write(yas5xx->map, YAS537_HCK, >> + (FIELD_GET(GENMASK(7, 4), data[15]) << 1)); > > ..and here and in many other places, please drop outer parentheses, > they are not needed. It looked too dangerous without – but thanks for the hint, I'll gladly drop them. >> + usleep_range(YAS537_MAG_RCOIL_TIME, YAS537_MAG_RCOIL_TIME+100); > > Please, add a comment explaining why this is needed. OK, I'll add: "Wait until the coil has ramped up". >> + dev_info(dev, "detected YAS537 MS-3T"); >> + /* As the version naming is unknown, provide it for debug only */ >> + dev_dbg(yas5xx->dev, "YAS537 version: %s\n", >> + yas5xx->version ? "1" : "0"); > > No need to have two prints, just add a version to the above one and > drop the bottom one. OK. ---------------------- I also spotted a minor mistake in function yas537_get_calibration_data() in the following part: /* Get data into these three blocks val1 to val3 */ val1 = get_unaligned_be32(&data[0]); val2 = get_unaligned_be32(&data[3]); val3 = get_unaligned_be32(&data[6]); val4 = get_unaligned_be32(&data[9]); The comment isn't up to date, talking about three blocks instead of four. I'll correct this. Kind regards, Jakob
