Re: [PATCH 1/2] iio: accel: add support for FXLS8962AF/FXLS8964AF accelerometers

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

On Fri, Apr 16, 2021 at 1:28 PM Sean Nyekjaer <sean@geanix.com> wrote:
>
> Add basic support for NXP FXLS8962AF/FXLS8964AF Automotive
> accelerometers.
> It will allow setting up scale/gain and reading x,y,z
> axis.

Do you have a link to the data sheet? Can you add it as Datasheet: tag?

> Signed-off-by: Sean Nyekjaer <sean@geanix.com>

...

> +config FXLS8962AF
> +       tristate "NXP FXLS8962AF and similar Accelerometers Driver"

I guess this can be hidden.

> +       select REGMAP
> +       help
> +         Say yes here to build support for the NXP 3-axis automotive
> +         accelerometer FXLS8962AF/FXLS8964AF.
> +
> +         To compile this driver as a module, choose M here: the module
> +         will be called fxls8962af.
> +
> +config FXLS8962AF_I2C
> +       tristate "NXP FXLS8962AF I2C transport"

> +       depends on FXLS8962AF

Why not select?

> +       depends on I2C

> +       default FXLS8962AF

And drop this.

> +       select REGMAP_I2C
> +       help
> +         Say yes here to enable the NXP FXLS8962AF accelerometer
> +         I2C transport channel.

Missed module name.

> +config FXLS8962AF_SPI
> +       tristate "NXP FXLS8962AF SPI transport"
> +       depends on FXLS8962AF
> +       depends on SPI
> +       default FXLS8962AF
> +       select REGMAP_SPI
> +       help
> +         Say yes here to enable the NXP FXLS8962AF accelerometer
> +         SPI transport channel.

Three comments as per I2C case.

...

> +/*
> + * Copyright 2021 Connected Cars A/S
> + */

One line

...

> +#include <linux/module.h>


Missed bits.h

> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>

Can you split it to a separate group to go...

> +#include <linux/pm_runtime.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/regmap.h>

...here?

> +#include "fxls8962af.h"

...

> +#define FXLS8962AF_FSR_MASK                    (BIT(2) | BIT(1))

GENMASK() ?

...

> +#define FXLS8962AF_TEMP_CENTER_VAL             25

Is it decimal one or bit based? I mean the maximum is 50 or is it a
bit field which represents raw value?

...

> +struct fxls8962af_chip_info {
> +       u8 chip_id;

You may save some memory if you move this at the end of the structure.

> +       const char *name;
> +       const struct iio_chan_spec *channels;
> +       int num_channels;
> +};

...

> +struct fxls8962af_data {
> +       struct regmap *regmap;
> +       struct mutex lock;

So, you are  using the regmap lock, what is this for? The golden rule
one _must_ describe locks (their purposes).

> +       const struct fxls8962af_chip_info *chip_info;
> +       struct regulator *vdd_reg;
> +       struct iio_mount_matrix orientation;
> +};

...

> +static int fxls8962af_drdy(struct fxls8962af_data *data)
> +{
> +       int tries = 150, ret;
> +       unsigned int reg;
> +       struct device *dev = regmap_get_device(data->regmap);

Try to keep reversed xmas tree order in definition block(s).

> +       while (tries-- > 0) {
> +               ret = regmap_read(data->regmap, FXLS8962AF_INT_STATUS, &reg);
> +               if (ret < 0)
> +                       return ret;
> +
> +               if ((reg & FXLS8962AF_INT_STATUS_SRC_DRDY) ==
> +                   FXLS8962AF_INT_STATUS_SRC_DRDY)
> +                       return 0;
> +
> +               msleep(20);
> +       }

Entire loop is repetition of regmap_read_poll_timeout().

> +       dev_err(dev, "data not ready\n");
> +
> +       return -EIO;
> +}
> +
> +static int fxls8962af_set_power_state(struct fxls8962af_data *data, bool on)
> +{

> +#ifdef CONFIG_PM

Why?

> +       struct device *dev = regmap_get_device(data->regmap);
> +       int ret;
> +
> +       if (on) {
> +               ret = pm_runtime_get_sync(dev);
> +       } else {
> +               pm_runtime_mark_last_busy(dev);
> +               ret = pm_runtime_put_autosuspend(dev);
> +       }
> +
> +       if (ret < 0) {
> +               dev_err(dev, "failed to change power state to %d\n", on);
> +               if (on)
> +                       pm_runtime_put_noidle(dev);
> +
> +               return ret;
> +       }

Untangle this. You may do on and off cases separately for better understanding.

> +#endif
> +
> +       return 0;
> +}

...

> +       if (ret < 0) {
> +               mutex_unlock(&data->lock);
> +               return ret;
> +       }

goto variant in all similar places will save you a lot of lines

goto out_unlock;

...

out_unlock: // or err_unlock if it's only for error path.
  mutex_unlock(...);
  return ret;

...

> +       *val = sign_extend32(le16_to_cpu(raw_val),
> +                            chan->scan_type.realbits - 1);

One line?

...

> +       mutex_lock(&data->lock);
> +
> +       is_active = fxls8962af_is_active(data);
> +       if (is_active < 0) {
> +               ret = is_active;
> +               goto fail;
> +       }

Why not

ret = ...;
if (ret < 0)
  goto fail;

... = ret;

?

Also note inconsistency here with goto style and there where you open
coded error paths.

...

> + fail:

err_unlock:

Labels should be descriptive.

> +       mutex_unlock(&data->lock);

...

> +               switch (chan->type) {
> +               case IIO_TEMP:
> +                       ret = fxls8962af_get_temp(data, val);
> +                       break;
> +               case IIO_ACCEL:
> +                       ret = fxls8962af_get_axis(data, chan, val);
> +                       break;
> +               default:
> +                       ret = -EINVAL;

break is missed.

> +               }

...

> +       case IIO_CHAN_INFO_OFFSET:
> +               if (chan->type == IIO_TEMP) {
> +                       *val = FXLS8962AF_TEMP_CENTER_VAL;
> +                       return IIO_VAL_INT;


> +               } else {

Redundant 'else'

> +                       return -EINVAL;
> +               }

You may rather refactor this to be like

if (type != ...)
  return -EINVAL;

...


> +               ret = fxls8962af_read_full_scale(data, val2);
> +               return ret;

Why not return directly?

> +       default:
> +               ret = -EINVAL;
> +               break;

Ditto.

> +       }

> +       return -EINVAL;

Isn't it a dead code?

...

> +static const struct fxls8962af_chip_info fxls_chip_info_table[] = {
> +       [fxls8962af] = {
> +               .chip_id = FXLS8962AF_DEVICE_ID,
> +               .name = "fxls8962af",
> +               .channels = fxls8962af_channels,
> +               .num_channels = ARRAY_SIZE(fxls8962af_channels),
> +       },
> +       [fxls8964af] = {
> +               .chip_id = FXLS8964AF_DEVICE_ID,
> +               .name = "fxls8964af",
> +               .channels = fxls8962af_channels,
> +               .num_channels = ARRAY_SIZE(fxls8962af_channels),


> +       }

Leave the comma here as well.

> +};

...

> +       for (i = 0; i < 10; i++) {
> +               usleep_range(100, 200);
> +               ret = regmap_read(data->regmap, FXLS8962AF_SENS_CONFIG1, &reg);
> +               if (ret == -EIO)
> +                       continue;       /* I2C comm reset */
> +               if (ret < 0)
> +                       return ret;
> +               if (!(reg & FXLS8962AF_SENS_CONFIG1_RST))
> +                       return 0;
> +       }
> +
> +       return -ETIMEDOUT;

Consider to adapt to regmap_read_poll_timeout().

> +}

...

> +       ret = devm_add_action_or_reset(dev, fxls8962af_pm_disable, dev);
> +       if (ret)
> +               return ret;
> +
> +

Too many blank lines.

> +       return devm_iio_device_register(dev, indio_dev);
> +

Ditto.

...

> +#ifdef CONFIG_PM

No, please use __maybe_unused attribute.

> +static int fxls8962af_runtime_suspend(struct device *dev)
> +{
> +       struct iio_dev *indio_dev = dev_get_drvdata(dev);
> +       struct fxls8962af_data *data = iio_priv(indio_dev);
> +       int ret;
> +
> +       mutex_lock(&data->lock);
> +       ret = fxls8962af_standby(data);
> +       mutex_unlock(&data->lock);
> +       if (ret < 0) {
> +               dev_err(dev, "powering off device failed\n");

> +               return -EAGAIN;

Why error code shadowing?

> +       }
> +
> +       return 0;
> +}
> +
> +static int fxls8962af_runtime_resume(struct device *dev)
> +{
> +       struct iio_dev *indio_dev = dev_get_drvdata(dev);
> +       struct fxls8962af_data *data = iio_priv(indio_dev);
> +       int ret;
> +
> +       ret = fxls8962af_active(data);
> +       if (ret < 0)
> +               return ret;
> +
> +       return 0;
> +}
> +#endif

...

> +/*
> + * Copyright 2021 Connected Cars A/S
> + */

One line.

...

> +#include <linux/device.h>

Shouldn't this be dev_printk.h?

> +#include <linux/mod_devicetable.h>
> +#include <linux/i2c.h>
> +#include <linux/module.h>
> +#include <linux/regmap.h>

...

> +/*
> + * Copyright 2021 Connected Cars A/S
> + */

One line?

...

> +#include <linux/kernel.h>

What for?

...

> +static const struct of_device_id fxls8962af_spi_of_match[] = {
> +       {.compatible = "nxp,fxls8962af"},
> +       {.compatible = "nxp,fxls8964af"},

> +       {},

Comma is not needed for terminator lines.

> +};

...

> +static const struct spi_device_id fxls8962af_spi_id_table[] = {
> +       {"fxls8962af", fxls8962af},
> +       {"fxls8964af", fxls8964af},
> +       {},

Ditto.

> +};

...

> +/*
> + * Copyright 2021 Connected Cars A/S
> + */

One line.

...

> +struct regmap;

struct device;

is missed.

-- 
With Best Regards,
Andy Shevchenko