Re: [PATCH 2/2 v4] iio: magnetometer: Add driver for Yamaha YAS530

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

On Sun, Nov 29, 2020 at 11:57 PM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> This adds an IIO magnetometer driver for the Yamaha
> YAS530 family of magnetometer/compass chips YAS530,
> YAS532 and YAS533.
>
> A quick survey of the source code released by different
> vendors reveal that we have these variants in the family
> with some deployments listed:
>
>  * YAS529 MS-3C (2005 Samsung Aries)
>  * YAS530 MS-3E (2011 Samsung Galaxy S Advance)
>  * YAS532 MS-3R (2011 Samsung Galaxy S4)
>  * YAS533 MS-3F (Vivo 1633, 1707, V3, Y21L)
>  * (YAS534 is a magnetic switch)
>  * YAS535 MS-6C
>  * YAS536 MS-3W
>  * YAS537 MS-3T (2015 Samsung Galaxy S6, Note 5)
>  * YAS539 MS-3S (2018 Samsung Galaxy A7 SM-A750FN)
>
> The YAS529 is so significantly different from the
> YAS53x variants that it will require its own driver.
> The YAS537 and YAS539 have slightly different register
> sets but have strong similarities so a common driver
> patching this one will probably be reasonable.
>
> The source code for Samsung Galaxy A7's YAS539 is not
> that significantly different from the YAS530 in the

that is significantly

> Galaxy S Advance, so I believe we will only need this
> one driver with quirks to handle all of them.
>
> The YAS539 is actively announced on Yamaha's devices
> site:
> https://device.yamaha.com/en/lsi/products/e_compass/
>
> This is a driver written from scratch using buffered
> IIO and runtime PM handling regulators and reset.

May I comment? :-) Okay my (mostly minor)  comments below.

...

> +#include <linux/module.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/i2c.h>
> +#include <linux/err.h>
> +#include <linux/mutex.h>
> +#include <linux/delay.h>
> +#include <linux/bitops.h>
> +#include <linux/random.h>
> +#include <linux/regmap.h>
> +#include <linux/unaligned/be_byteshift.h>
> +#include <linux/bitfield.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/pm_runtime.h>

Perhaps sorted?

> +#include <linux/iio/iio.h>
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/trigger_consumer.h>
> +#include <linux/iio/triggered_buffer.h>

Ditto?

...

> +#define YAS5XX_OFFSET_X                        0x85 /* +/-31 */
> +#define YAS5XX_OFFSET_Y1               0x86 /* +/-31 */
> +#define YAS5XX_OFFSET_Y2               0x87 /* +/-31 */

What comments mean? Range of valid values? Perhaps [-31..31]
I explain why I'm commenting on this. My confusion becomes from my
first thought was that this is about register offsets. But then I have
read the name and assumed above. And still not sure that it is a right
assumption.

...

> +#define YAS530_DATA_CENTER             2048
> +#define YAS530_DATA_OVERFLOW           4095

Here...

> +#define YAS532_DATA_CENTER             4096
> +#define YAS532_DATA_OVERFLOW           8191

...and here sound like values are depend to the register bits, so, I
would perhaps right them in a such way (or make a comment)

...

> +/* Turn off device regulators etc after 5 seconds of inactivity */
> +#define YAS5XX_AUTOSUSPEND_DELAY       5000

_MS

...

> +
> +

One blank line is usually enough.

...

> +       val = get_unaligned_be16(&data[0]);

> +       val = get_unaligned_be16(&data[0]);

Briefly looking I don't see other calls, but here no need to use
get_unaligned_*(), rather be16_to_cpu{p}/cpu_to_be16{p}.

...

> +static int yas5xx_busy_wait(struct yas5xx *yas5xx)
> +{
> +       int maxloops = 1000;
> +       unsigned int val;
> +       int ret;
> +
> +       while (maxloops) {
> +               ret = regmap_read(yas5xx->map, YAS5XX_MEASURE_DATA, &val);
> +               if (ret)
> +                       return ret;
> +
> +               if (!(val & YAS5XX_MEASURE_DATA_BUSY))
> +                       return 0;
> +
> +               maxloops--;
> +       }
> +
> +       return -ETIMEDOUT;
> +}

Reimplementation of regmap_read_poll_timeout() ?

...

> +static const struct iio_chan_spec_ext_info yas5xx_ext_info[] = {
> +       IIO_MOUNT_MATRIX(IIO_SHARED_BY_DIR, yas5xx_get_mount_matrix),

> +       { },

No need for comma.

> +};

...

> +#define YAS5XX_AXIS_CHANNEL(axis, index)                               \
> +       {                                                               \
> +               .type = IIO_MAGN,                                       \
> +               .modified = 1,                                          \
> +               .channel2 = IIO_MOD_##axis,                             \
> +               .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |          \
> +                       BIT(IIO_CHAN_INFO_SCALE),                       \
> +               .ext_info = yas5xx_ext_info,                            \
> +               .address = index,                                       \
> +               .scan_index = index,                                    \
> +               .scan_type = {                                          \
> +                       .sign = 's',                                    \
> +                       .realbits = 32,                                 \
> +                       .storagebits = 32,                              \

> +                       .endianness = IIO_CPU                           \

...but here I would rather leave it.

> +               },                                                      \
> +       }

...

> +                       .endianness = IIO_CPU

Ditto.

...

> +static const unsigned long yas5xx_scan_masks[] = { 0xf, 0 };

GENMASK()? Or is it plain value?

...

> +static const struct regmap_config yas5xx_regmap_config = {
> +       .reg_bits = 8,
> +       .val_bits = 8,
> +       .max_register = 0xff,
> +       .volatile_reg = yas5xx_volatile_reg,
> +};

You have mutex in the code and another lock by regmap core. Do you need both?

...

> +       /* Sanity check */
> +       for (i = 0; i < 13; i++) {
> +               if (data[i] != 0)
> +                       break;
> +       }

memchr_inv()

...

> +       if (i == 13 && !(data[13] & BIT(7)))
> +               dev_err(yas5xx->dev, "calibration is blank!\n");

And we continue... Shouldn't we bail out or decrease level to warning?

...

> +       yas5xx->version = data[13] & 0x01;

BIT(0) ?

...

> +       indio_dev = devm_iio_device_alloc(dev, sizeof(*yas5xx));
> +       if (indio_dev == NULL)

!indio_dev ?

> +               return -ENOMEM;

...

> +       if (yas5xx->reset)

If it's optional we don't need this check, otherwise we don't need it
either. Or I missed something?

> +               gpiod_set_value_cansleep(yas5xx->reset, 1);

...

> +       if (yas5xx->reset)

Ditto.

> +               gpiod_set_value_cansleep(yas5xx->reset, 1);

...

> +       if (yas5xx->reset)
> +               gpiod_set_value_cansleep(yas5xx->reset, 0);

Ditto.

...

> +       if (yas5xx->reset)
> +               gpiod_set_value_cansleep(yas5xx->reset, 1);

Ditto.

-- 
With Best Regards,
Andy Shevchenko