On Mon, Jun 13, 2022 at 3:18 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.
...
> - * 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
