Re: [PATCH v2 7/7] iio: magnetometer: yas530: Add YAS537 variant

[Jakob Hauser <jahau@rocketmail.com>]

Hi Andy,

again on the Cc list, sorry: Looks like the part...

"open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS
<devicetree@vger.kernel.org>, Hans de Goede <hdegoede@redhat.com>, Andy
Shevchenko <andy.shevchenko@gmail.com>,"

... as a text string is saved in your e-mail clients' address book as
the name of the e-mail address <~postmarketos/upstreaming@lists.sr.ht>.

On 13.06.22 17:20, Andy Shevchenko wrote:
>
> On Mon, Jun 13, 2022 at 3:18 AM Jakob Hauser <jahau@rocketmail.com> 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