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

[Andy Shevchenko <andy.shevchenko@gmail.com>]

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

...

> - * 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.

...

> +#define YAS537_MAG_AVERAGE_32_MASK     GENMASK(6, 4) /* corresponds to 0x70 */

Useless comment.

...

> +#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.

...

> +       /* Read data */

Not sure how useful is this comment.

...

> +       *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.

...

> +                       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().

...

> +                       xy1y2[i] = h[i] + 8192;

BIT(13)

...

> +       /*
> +        * Raw temperature value t is number of counts. A product description
> +        * of YAS537 mentions a temperature resulution of 0.05 °C/count.

resolution

> +        * A readout of the t value at ca. 20 °C returns approx. 8120 counts.
> +        *
> +        * 8120 counts x 0.05 °C/count corresponds to a range of 406 °C.
> +        * 0 counts would be at theoretical -386 °C.
> +        *
> +        * The formula used for YAS530/532 needs to be adapted to this
> +        * theoretical starting temperature, again calculating with 1/10:s
> +        * of degrees Celsius and finally multiplying by 100 to get milli
> +        * degrees Celsius.
> +        */

...

>                         /*
> -                        * Raw values of YAS532 are in nanotesla. Devide by
> -                        * 100000 (10^5) to get Gauss.
> +                        * Raw values of YAS532 and YAS537 are in nanotesla.
> +                        * Devide by 100000 (10^5) to get Gauss.

Divide (chance to fix a type while at 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.

...

> +       dev_dbg(yas5xx->dev, "calibration data: %*ph\n", 17, data);

Use less stack by "%17ph".

...

> +       /* 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");
> +       }

...

> +               for (i = 0; i < 17; i++) {

16 and 17 seems like a magic that is used a lot, perhaps define?

...

> +               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.

> +               if (ret)
> +                       return ret;

...

> +       usleep_range(YAS537_MAG_RCOIL_TIME, YAS537_MAG_RCOIL_TIME+100);

Please, add a comment explaining why this is needed.

...

> +               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.


-- 
With Best Regards,
Andy Shevchenko