On 4/15/21 1:46 PM, Sean Nyekjaer wrote:
Add basic support for NXP FXLS8962AF/FXLS8964AF Automotive
accelerometers.
It will allow setting up scale/gain and reading x,y,z
axis.
Hi,
Thanks for the patch. This looks very good!
Signed-off-by: Sean Nyekjaer <sean@xxxxxxxxxx>
---
- Interrupt, buffered reads and events support is in the works.
drivers/iio/accel/Kconfig | 22 +
drivers/iio/accel/Makefile | 4 +
drivers/iio/accel/fxls8962af-core.c | 739 ++++++++++++++++++++++++++++
drivers/iio/accel/fxls8962af-i2c.c | 67 +++
drivers/iio/accel/fxls8962af-spi.c | 72 +++
drivers/iio/accel/fxls8962af.h | 21 +
6 files changed, 925 insertions(+)
create mode 100644 drivers/iio/accel/fxls8962af-core.c
create mode 100644 drivers/iio/accel/fxls8962af-i2c.c
create mode 100644 drivers/iio/accel/fxls8962af-spi.c
create mode 100644 drivers/iio/accel/fxls8962af.h
diff --git a/drivers/iio/accel/Kconfig b/drivers/iio/accel/Kconfig
index 2e0c62c39155..c4b42024de42 100644
--- a/drivers/iio/accel/Kconfig
+++ b/drivers/iio/accel/Kconfig
@@ -370,6 +370,28 @@ config MMA8452
To compile this driver as a module, choose M here: the module
will be called mma8452.
+config FXLS8962AF
We try to keep the entries in alphabetical order.
+ tristate "NXP FXLS8962AF and similar Accelerometers Driver"
+ select IIO_BUFFER
+ select IIO_TRIGGERED_BUFFER
The driver does not yet make use of interrupts.
+ select REGMAP
+ select FXLS8962AF_I2C if I2C
+ select FXLS8962AF_SPI if SPI
Usually we do this the other way around. Expose I2C and SPI support as
independent config items and let them select the core. The reason is
because I2C support can be built as a module. To be honest I'm not sure
how exactly this will be have if I2C support is a module, but FXLS8962AF
is built-in. It might actually work fine.
+ 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
+ select REGMAP_I2C
+
+config FXLS8962AF_SPI
+ tristate
+ select REGMAP_SPI
+
config MMA9551_CORE
tristate
diff --git a/drivers/iio/accel/Makefile b/drivers/iio/accel/Makefile
index 4f6c1ebe13b0..3e48a9127863 100644
--- a/drivers/iio/accel/Makefile
+++ b/drivers/iio/accel/Makefile
@@ -40,6 +40,10 @@ obj-$(CONFIG_MMA7660) += mma7660.o
obj-$(CONFIG_MMA8452) += mma8452.o
+obj-$(CONFIG_FXLS8962AF) += fxls8962af-core.o
+obj-$(CONFIG_FXLS8962AF_I2C) += fxls8962af-i2c.o
+obj-$(CONFIG_FXLS8962AF_SPI) += fxls8962af-spi.o
We try to keep the entries in alphabetical order.
+
obj-$(CONFIG_MMA9551_CORE) += mma9551_core.o
obj-$(CONFIG_MMA9551) += mma9551.o
obj-$(CONFIG_MMA9553) += mma9553.o
diff --git a/drivers/iio/accel/fxls8962af-core.c b/drivers/iio/accel/fxls8962af-core.c
new file mode 100644
index 000000000000..daee5b07ca49
--- /dev/null
+++ b/drivers/iio/accel/fxls8962af-core.c
@@ -0,0 +1,739 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright 2021 Connected Cars A/S
+ */
+
+#include <linux/module.h>
+#include <linux/i2c.h>
No I2C in this driver.
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+#include <linux/delay.h>
+#include <linux/of_device.h>
+#include <linux/of_irq.h>
At least of_irq.h is unused
+#include <linux/pm_runtime.h>
+#include <linux/regulator/consumer.h>
+#include <linux/regmap.h>
+
+#include "fxls8962af.h"
[...]
+enum {
+ idx_x,
+ idx_y,
+ idx_z,
+ idx_ts,
Might want to put a prefix on these. idx_x is not that unique of an
identifier.
+};
[...]
+
+static int fxls8962af_get_temp(struct fxls8962af_data *data, int *val)
+{
+ struct device *dev = regmap_get_device(data->regmap);
+ int ret;
+ unsigned int value;
+
+ mutex_lock(&data->lock);
+
+ ret = fxls8962af_drdy(data);
+ if (ret < 0) {
+ mutex_unlock(&data->lock);
+ return ret;
+ }
+
+ ret = fxls8962af_set_power_state(data, true);
+ if (ret) {
+ mutex_unlock(&data->lock);
+ return ret;
+ }
+
+ ret = regmap_read(data->regmap, FXLS8962AF_TEMP_OUT, &value);
+ if (ret < 0) {
+ dev_err(dev, "Error reading reg_temp\n");
+ fxls8962af_set_power_state(data, false);
+ mutex_unlock(&data->lock);
+ return ret;
+ }
+
+ ret = fxls8962af_set_power_state(data, false);
+
+ *val = sign_extend32(value, 7);
Doesn't really matter, but his could also be outside the locked section.
+
+ mutex_unlock(&data->lock);
+ if (ret < 0)
+ return ret;
+
+ return IIO_VAL_INT;
+}
[...]
+static int fxls8962af_write_raw_get_fmt(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ long mask)
+{
+ switch (mask) {
+ case IIO_CHAN_INFO_SCALE:
+ switch (chan->type) {
+ case IIO_ACCEL:
+ return IIO_VAL_INT_PLUS_NANO;
+ default:
+ return IIO_VAL_INT_PLUS_MICRO;
+ }
+ default:
+ return IIO_VAL_INT_PLUS_MICRO;
+ }
Since the driver only supports writing the accelerator scale this could
be simplified to just return IIO_VAL_INT_PLUS_NANO.
+}
+
+static IIO_DEVICE_ATTR(in_accel_scale_available, 0444,
+ fxls8962af_show_scale_avail, NULL, 0);
There is a new API to query the available values through the IIO API.
This can be done by implementing the read_avail callback of the iio_info
struct. This is the recommended method for new drivers rather than
registering a sysfs attribute and do manually formatting of the scale.
[...]
+
+static int fxls8962af_write_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ int val, int val2, long mask)
+{
+ struct fxls8962af_data *data = iio_priv(indio_dev);
+ int ret;
+
+ ret = iio_device_claim_direct_mode(indio_dev);
+ if (ret)
+ return ret;
+
+ switch (mask) {
+ case IIO_CHAN_INFO_SCALE:
Should check that val is 0.
+ ret = fxls8962af_set_full_scale(data, val2);
+ break;
+ default:
+ ret = -EINVAL;
+ break;
+ }
+
+ iio_device_release_direct_mode(indio_dev);
+ return ret;
+}
+
+#define FXLS8962AF_CHANNEL(axis, idx) { \
+ .type = IIO_ACCEL, \
+ .modified = 1, \
+ .channel2 = IIO_MOD_##axis, \
+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
+ BIT(IIO_CHAN_INFO_CALIBBIAS), \
+ .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SAMP_FREQ) | \
+ BIT(IIO_CHAN_INFO_SCALE) | \
+ BIT(IIO_CHAN_INFO_HIGH_PASS_FILTER_3DB_FREQUENCY) | \
+ BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), \
Some of these attributes are not implemented yet.
+ .scan_index = idx, \
+ .scan_type = { \
+ .sign = 's', \
+ .realbits = 12, \
+ .storagebits = 16, \
+ .shift = 4, \
+ .endianness = IIO_BE, \
+ }, \
+}
+
[...]
+int fxls8962af_core_probe(struct device *dev, struct regmap *regmap, int irq,
+ const char *name)
+{
+ struct fxls8962af_data *data;
+ struct iio_dev *indio_dev;
+ unsigned int reg;
+ int ret, i;
+
+ indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
+ if (!indio_dev)
+ return -ENOMEM;
+
+ data = iio_priv(indio_dev);
+ dev_set_drvdata(dev, indio_dev);
+ mutex_init(&data->lock);
+ data->regmap = regmap;
+
+ ret = iio_read_mount_matrix(dev, "mount-matrix", &data->orientation);
+ if (ret)
+ return ret;
+
+ data->vdd_reg = devm_regulator_get_optional(dev, "vdd");
+ if (IS_ERR(data->vdd_reg)) {
+ if ((PTR_ERR(data->vdd_reg) != -ENODEV) && dev->of_node)
Why the check for of_node?
+ return PTR_ERR(data->vdd_reg);
+
+ data->vdd_reg = NULL;
+ } else {
+ ret = regulator_enable(data->vdd_reg);
+ if (ret)
+ return ret;
+ }
+
+ ret = regmap_read(data->regmap, FXLS8962AF_WHO_AM_I, ®);
+ if (ret < 0)
+ goto disable_regulators;
+
+ dev_dbg(dev, "Chip Id %x\n", reg);
+ for (i = 0; i < ARRAY_SIZE(fxls_chip_info_table); i++) {
+ if (fxls_chip_info_table[i].chip_id == reg) {
+ data->chip_info = &fxls_chip_info_table[i];
+ break;
+ }
+ }
+
+ indio_dev->channels = data->chip_info->channels;
+ indio_dev->num_channels = data->chip_info->num_channels;
+ indio_dev->name = name;
Since the driver identifies the chip based on the who-am-i register, I
wonder if the name should also come from the chip_info_table.
+ indio_dev->available_scan_masks = fxls8962af_scan_masks;
+ indio_dev->info = &fxls8962af_info;
+ indio_dev->modes = INDIO_DIRECT_MODE;
+
+ dev_info(dev, "registering %s accelerometer\n", name);
Its a bit of personal taste, but I don't like these 'driver has probed'
messages. It is great for development, but not so great for production.
If every device that gets registered prints such a message that does
slow down boot a fair bit and clutters the boot log.
+
+ ret = fxls8962af_reset(data);
+ if (ret < 0)
+ goto disable_regulators;
+
+ ret = pm_runtime_set_active(dev);
+ if (ret < 0)
+ goto disable_regulators;
+
+ pm_runtime_enable(dev);
+ pm_runtime_set_autosuspend_delay(dev, FXLS8962AF_AUTO_SUSPEND_DELAY_MS);
+ pm_runtime_use_autosuspend(dev);
+
+ ret = iio_device_register(indio_dev);
+ if (ret < 0)
+ goto disable_regulators;
+
+ return 0;
+
+ disable_regulators:
+ if (data->vdd_reg)
+ regulator_disable(data->vdd_reg);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(fxls8962af_core_probe);
+
[...]
+#ifdef CONFIG_PM
[...]
+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;
+
+ if (data->vdd_reg) {
+ ret = regulator_disable(data->vdd_reg);
Should be regulator_enable()?
+ if (ret) {
+ dev_err(dev, "failed to disable VDD regulator\n");
+ return ret;
+ }
+ }
+
+ ret = fxls8962af_active(data);
+ if (ret < 0)
+ goto runtime_resume_failed;
+
+ return 0;
+
+ runtime_resume_failed:
+ if (data->vdd_reg)
+ regulator_disable(data->vdd_reg);
+
+ return ret;
+}
+#endif
[...]
diff --git a/drivers/iio/accel/fxls8962af-i2c.c b/drivers/iio/accel/fxls8962af-i2c.c
new file mode 100644
index 000000000000..4cdbd9793df7
--- /dev/null
+++ b/drivers/iio/accel/fxls8962af-i2c.c
@@ -0,0 +1,67 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright 2021 Connected Cars A/S
+ */
+
+#include <linux/device.h>
+#include <linux/mod_devicetable.h>
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/acpi.h>
No ACPI in this file.
+#include <linux/regmap.h>
+
+#include "fxls8962af.h"
+
+static int fxls8962af_probe(struct i2c_client *client,
+ const struct i2c_device_id *id)
+{
+ struct regmap *regmap;
+ const char *name = NULL;
+
+ regmap = devm_regmap_init_i2c(client, &fxls8962af_regmap_conf);
+ if (IS_ERR(regmap)) {
+ dev_err(&client->dev, "Failed to initialize i2c regmap\n");
+ return PTR_ERR(regmap);
+ }
+
+ if (id)
+ name = id->name;
+
+ return fxls8962af_core_probe(&client->dev, regmap, client->irq, name);
+}
+
+static int fxls8962af_remove(struct i2c_client *client)
+{
+ return fxls8962af_core_remove(&client->dev);
+}
+
+static const struct i2c_device_id fxls8962af_id[] = {
+ {"fxls8962af", fxls8962af},
+ {"fxls8964af", fxls8964af},
+ {}
+};
+MODULE_DEVICE_TABLE(i2c, fxls8962af_id);
+
+static const struct of_device_id fxls8962af_of_match[] = {
+ {.compatible = "nxp,fxls8962af"},
+ {.compatible = "nxp,fxls8964af"},
+ {},
+};
+MODULE_DEVICE_TABLE(of, fxls8962af_of_match);
+
+static struct i2c_driver fxls8962af_driver = {
+ .driver = {
+ .name = "fxls8962af_i2c",
+ .of_match_table = fxls8962af_of_match,
+ .pm = &fxls8962af_pm_ops,
+ },
+ .probe = fxls8962af_probe,
For new i2C drivers use the probe_new callback.
+ .remove = fxls8962af_remove,
+ .id_table = fxls8962af_id,
+};
+
+module_i2c_driver(fxls8962af_driver);
+
+MODULE_AUTHOR("Sean Nyekjaer <sean@xxxxxxxxxx>");
+MODULE_DESCRIPTION("NXP FXLS8962AF/FXLS8964AF accelerometer driver");
+MODULE_LICENSE("GPL v2");
diff --git a/drivers/iio/accel/fxls8962af-spi.c b/drivers/iio/accel/fxls8962af-spi.c
new file mode 100644
index 000000000000..976851863e82
--- /dev/null
+++ b/drivers/iio/accel/fxls8962af-spi.c
@@ -0,0 +1,72 @@
[...]
For the I2C driver you are using a global regmap config, but for the SPI
one a local. Looks to me as if the gobal one can also be used for the
SPI driver.
+static int fxls8962af_probe(struct spi_device *spi)
+{
+ const struct spi_device_id *id = spi_get_device_id(spi);
+ struct regmap *regmap;
+ const char *name = NULL;
+
+ regmap = devm_regmap_init_spi(spi, &fxls8962af_spi_regmap_config);
+ if (IS_ERR(regmap)) {
+ dev_err(&spi->dev, "Failed to register spi regmap %d\n",
+ (int)PTR_ERR(regmap));
%ld and drop the cast.
+ return PTR_ERR(regmap);
+ }
+
+ if (id)
+ name = id->name;
+
+ return fxls8962af_core_probe(&spi->dev, regmap, spi->irq, name);
+}
+
+static int fxls8962af_remove(struct spi_device *spi)
+{
+ return fxls8962af_core_remove(&spi->dev);
+}
