On Fri, Apr 16, 2021 at 1:28 PM Sean Nyekjaer <sean@xxxxxxxxxx> 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@xxxxxxxxxx>
...
> +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, ®);
> + 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, ®);
> + 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
