Re: [PATCH v3 8/8] iio: magnetometer: yas530: Add YAS537 variant

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Input]     [Linux Kernel]     [Linux SCSI]     [X.org]

  Powered by Linux