Honeywell mpr is a familiy of pressure sensors.
Add initial I2C support.
Note:
- Buffered mode is supported
- SPI mode is not supported
Signed-off-by: Andreas Klinger <[email protected]>
---
drivers/iio/pressure/Kconfig | 12 ++
drivers/iio/pressure/Makefile | 1 +
drivers/iio/pressure/mpr.c | 307 ++++++++++++++++++++++++++++++++++
3 files changed, 320 insertions(+)
create mode 100644 drivers/iio/pressure/mpr.c
diff --git a/drivers/iio/pressure/Kconfig b/drivers/iio/pressure/Kconfig
index c9453389e4f7..e3ec94036e3c 100644
--- a/drivers/iio/pressure/Kconfig
+++ b/drivers/iio/pressure/Kconfig
@@ -148,6 +148,18 @@ config MPL3115
To compile this driver as a module, choose M here: the module
will be called mpl3115.
+config MPR
+ tristate "Honeywell MPR (MicroPressure sensors)"
+ depends on I2C
+ select IIO_BUFFER
+ select IIO_TRIGGERED_BUFFER
+ help
+ Say Y here to build support for the Honeywell MicroPressure pressure sensors MPR.
+ There are many different types with different pressure range. These ranges can be set up
+ in the device tree.
+
+ To compile this driver as a module, choose M here: the module will be called mpr.
+
config MS5611
tristate "Measurement Specialties MS5611 pressure sensor driver"
select IIO_BUFFER
diff --git a/drivers/iio/pressure/Makefile b/drivers/iio/pressure/Makefile
index 083ae87ff48f..b701d9c7187d 100644
--- a/drivers/iio/pressure/Makefile
+++ b/drivers/iio/pressure/Makefile
@@ -19,6 +19,7 @@ obj-$(CONFIG_MPL115) += mpl115.o
obj-$(CONFIG_MPL115_I2C) += mpl115_i2c.o
obj-$(CONFIG_MPL115_SPI) += mpl115_spi.o
obj-$(CONFIG_MPL3115) += mpl3115.o
+obj-$(CONFIG_MPR) += mpr.o
obj-$(CONFIG_MS5611) += ms5611_core.o
obj-$(CONFIG_MS5611_I2C) += ms5611_i2c.o
obj-$(CONFIG_MS5611_SPI) += ms5611_spi.o
diff --git a/drivers/iio/pressure/mpr.c b/drivers/iio/pressure/mpr.c
new file mode 100644
index 000000000000..1728b42bee7e
--- /dev/null
+++ b/drivers/iio/pressure/mpr.c
@@ -0,0 +1,307 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * MPR - MicroPressure pressure sensor driver
+ *
+ * Copyright (c) Andreas Klinger <[email protected]>
+ *
+ * Data sheet:
+ * https://prod-edam.honeywell.com/content/dam/honeywell-edam/sps/siot/en-us/products/sensors/ \
+ * pressure-sensors/board-mount-pressure-sensors/micropressure-mpr-series/documents/ \
+ * sps-siot-mpr-series-datasheet-32332628-ciid-172626.pdf
+ *
+ * 7-bit I2C default slave address: 0x18
+ *
+ */
+
+#include <linux/of.h>
+#include <linux/delay.h>
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/mod_devicetable.h>
+
+#include <linux/device.h>
+#include <linux/gpio/consumer.h>
+
+#include <asm/unaligned.h>
+
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+#include <linux/iio/buffer.h>
+#include <linux/iio/triggered_buffer.h>
+#include <linux/iio/trigger_consumer.h>
+
+/* bits in i2c status byte */
+#define MPR_I2C_POWER BIT(6) /* device is powered */
+#define MPR_I2C_BUSY BIT(5) /* device is busy */
+#define MPR_I2C_MEMORY BIT(2) /* integrity test passed */
+#define MPR_I2C_MATH BIT(0) /* internal math saturation */
+
+struct mpr_data {
+ struct device *dev;
+ void *client;
+ struct mutex lock;
+ s32 pmin;
+ s32 pmax;
+ struct gpio_desc *gpiod_reset;
+ int irq;
+ struct completion completion;
+ s64 channel[2] __aligned(8);
+};
+
+static int mpr_read_raw(struct iio_dev *indio_dev, struct iio_chan_spec const *chan, int *val,
+ int *val2, long mask);
+
+static const struct iio_chan_spec mpr_channels[] = {
+ {
+ .type = IIO_PRESSURE,
+ .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
+ .scan_index = 0,
+ .scan_type = {
+ .sign = 's',
+ .realbits = 64,
+ .storagebits = 64,
+ .endianness = IIO_CPU,
+ },
+ },
+ IIO_CHAN_SOFT_TIMESTAMP(1),
+};
+
+static const struct iio_info mpr_info = {
+ .read_raw = &mpr_read_raw,
+};
+
+static void mpr_reset(struct mpr_data *data)
+{
+ if (data->gpiod_reset) {
+ gpiod_set_value(data->gpiod_reset, 0);
+ udelay(10);
+ gpiod_set_value(data->gpiod_reset, 1);
+ }
+}
+
+static int mpr_read_pressure(struct mpr_data *data, s64 *press)
+{
+ int ret, i;
+ u8 wdata[] = {0xAA, 0x00, 0x00};
+ s32 status;
+ int nloops = 10;
+ char buf[5];
+ s64 press_cnt;
+ s64 outputmin = 1677722;
+ s64 outputmax = 15099494;
+
+ reinit_completion(&data->completion);
+
+ ret = i2c_master_send(data->client, wdata, sizeof(wdata));
+ if (ret < 0) {
+ dev_err(data->dev, "error while writing ret: %d\n", ret);
+ return ret;
+ }
+
+ if (data->irq > 0) {
+ ret = wait_for_completion_timeout(&data->completion, HZ);
+ if (!ret) {
+ dev_err(data->dev, "timeout while waiting for eoc interrupt\n");
+ return -ETIMEDOUT;
+ }
+ } else {
+ /* wait until status indicates data is ready */
+ for (i = 0; i < nloops; i++) {
+ usleep_range(5000, 10000);
+ status = i2c_smbus_read_byte(data->client);
+ if (status < 0) {
+ dev_err(data->dev, "error while reading, status: %d\n", status);
+ return status;
+ }
+ if (!(status & MPR_I2C_BUSY))
+ break;
+ }
+ if (i == nloops) {
+ dev_err(data->dev, "timeout while reading\n");
+ return -ETIMEDOUT;
+ }
+ }
+
+ ret = i2c_master_recv(data->client, buf, sizeof(buf));
+ if (ret < 0) {
+ dev_err(data->dev, "error in i2c_master_recv ret: %d\n", ret);
+ return ret;
+ }
+
+ if (buf[0] & MPR_I2C_BUSY) {
+ /* it should never be the case that status still indicates business */
+ dev_err(data->dev, "data still not ready: %08x\n", buf[0]);
+ return -ETIMEDOUT;
+ }
+
+ press_cnt = buf[3] + buf[2] * 256 + buf[1] * 65536;
+
+ *press = ((press_cnt - outputmin) * (s64)(data->pmax - data->pmin))
+ / (outputmax - outputmin) + (s64)data->pmin;
+
+ dev_dbg(data->dev, "received: %*ph cnt: %lld press: %lld\n", ret, buf, press_cnt, *press);
+
+ return 0;
+}
+
+static irqreturn_t mpr_eoc_handler(int irq, void *p)
+{
+ struct mpr_data *data = (struct mpr_data *)p;
+
+ complete(&data->completion);
+
+ return IRQ_HANDLED;
+}
+
+static irqreturn_t mpr_trigger_handler(int irq, void *p)
+{
+ int ret;
+ struct iio_poll_func *pf = p;
+ struct iio_dev *indio_dev = pf->indio_dev;
+ struct mpr_data *data = iio_priv(indio_dev);
+
+ mutex_lock(&data->lock);
+ ret = mpr_read_pressure(data, &data->channel[0]);
+ if (ret < 0) {
+ mutex_unlock(&data->lock);
+ goto err;
+ }
+
+ iio_push_to_buffers_with_timestamp(indio_dev, data->channel, iio_get_time_ns(indio_dev));
+ mutex_unlock(&data->lock);
+
+err:
+ iio_trigger_notify_done(indio_dev->trig);
+
+ return IRQ_HANDLED;
+}
+
+static int mpr_read_raw(struct iio_dev *indio_dev, struct iio_chan_spec const *chan, int *val,
+ int *val2, long mask)
+{
+ int ret;
+ s64 pressure;
+ struct mpr_data *data = iio_priv(indio_dev);
+
+ if (mask == IIO_CHAN_INFO_PROCESSED) {
+ mutex_lock(&data->lock);
+ ret = mpr_read_pressure(data, &pressure);
+ mutex_unlock(&data->lock);
+ if (ret < 0)
+ return ret;
+
+ if (chan->type == IIO_PRESSURE) {
+ *val = (s32)clamp(pressure, 0LL, 2147483648LL);
+ return IIO_VAL_INT;
+ }
+ }
+
+ return -EINVAL;
+}
+
+static int mpr_probe(struct i2c_client *client, const struct i2c_device_id *id)
+{
+ int ret;
+ struct mpr_data *data;
+ struct iio_dev *indio_dev;
+ struct device *dev = &client->dev;
+ struct device_node *np = dev->of_node;
+
+ if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_READ_BYTE))
+ return -EOPNOTSUPP;
+
+ indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
+ if (!indio_dev)
+ return -ENOMEM;
+
+ data = iio_priv(indio_dev);
+ data->dev = &client->dev;
+ i2c_set_clientdata(client, indio_dev);
+ data->client = client;
+ data->irq = client->irq;
+
+ mutex_init(&data->lock);
+ init_completion(&data->completion);
+
+ indio_dev->name = client->name;
+ indio_dev->info = &mpr_info;
+ indio_dev->channels = mpr_channels;
+ indio_dev->num_channels = ARRAY_SIZE(mpr_channels);
+ indio_dev->modes = INDIO_DIRECT_MODE;
+
+ if (np) {
+ if (of_property_read_s32(np, "honeywell,pmin", &data->pmin)) {
+ dev_err(dev, "honeywell,pmin missing in DT\n");
+ return -EINVAL;
+ }
+ if (of_property_read_s32(np, "honeywell,pmax", &data->pmax)) {
+ dev_err(dev, "honeywell,pmax missing in DT\n");
+ return -EINVAL;
+ }
+
+ data->gpiod_reset = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
+ if (IS_ERR(data->gpiod_reset)) {
+ dev_err(dev, "failed to get reset-gpios: err=%pe\n",
+ data->gpiod_reset);
+ data->gpiod_reset = NULL;
+ }
+
+ if (data->irq > 0) {
+ ret = devm_request_irq(dev, data->irq, mpr_eoc_handler,
+ IRQF_TRIGGER_RISING, client->name, data);
+ if (ret) {
+ dev_err(dev, "request irq %d failed\n", data->irq);
+ return ret;
+ }
+ }
+ } else {
+ /* when loaded as i2c device we need to use default values */
+ dev_warn(dev, "no dt node found; using defaults\n");
+ data->pmin = 0;
+ data->pmax = 172369; /* 25 psi */
+ data->gpiod_reset = NULL;
+ }
+
+ mpr_reset(data);
+
+ ret = devm_iio_triggered_buffer_setup(dev, indio_dev, NULL, mpr_trigger_handler, NULL);
+ if (ret < 0) {
+ dev_err(dev, "iio triggered buffer setup failed\n");
+ return ret;
+ }
+
+ ret = devm_iio_device_register(dev, indio_dev);
+ if (ret < 0) {
+ dev_err(dev, "unable to register iio device\n");
+ return ret;
+ }
+
+ return 0;
+}
+
+static const struct of_device_id mpr_matches[] = {
+ { .compatible = "honeywell,mpr", .data = 0 },
+ { }
+};
+MODULE_DEVICE_TABLE(of, mpr_matches);
+
+static const struct i2c_device_id mpr_id[] = {
+ { "honeywell,mpr", 0 },
+ { }
+};
+MODULE_DEVICE_TABLE(i2c, mpr_id);
+
+static struct i2c_driver mpr_driver = {
+ .probe = mpr_probe,
+ .id_table = mpr_id,
+ .driver = {
+ .name = "mpr",
+ .of_match_table = mpr_matches,
+ },
+};
+module_i2c_driver(mpr_driver);
+
+MODULE_AUTHOR("Andreas Klinger <[email protected]>");
+MODULE_DESCRIPTION("MPR I2C driver");
+MODULE_LICENSE("GPL");
+MODULE_IMPORT_NS(IIO_MPR);
--
2.30.2
Hi Andreas,
I love your patch! Yet something to improve:
[auto build test ERROR on jic23-iio/togreg]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Andreas-Klinger/dt-bindings-iio-pressure-Support-Honeywell-mpr-sensors/20230401-171226
base: https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
patch link: https://lore.kernel.org/r/ZCf085W4XL2PtQf6%40arbad
patch subject: [PATCH 2/3] iio: pressure: Honeywell mpr pressure sensor
config: m68k-randconfig-r023-20230401 (https://download.01.org/0day-ci/archive/20230401/[email protected]/config)
compiler: m68k-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/6a49dae45811d8a644c56dc18b6cdbc6ea67df98
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Andreas-Klinger/dt-bindings-iio-pressure-Support-Honeywell-mpr-sensors/20230401-171226
git checkout 6a49dae45811d8a644c56dc18b6cdbc6ea67df98
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=m68k olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=m68k SHELL=/bin/bash
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
| Link: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All errors (new ones prefixed by >>, old ones prefixed by <<):
>> ERROR: modpost: "__divdi3" [drivers/iio/pressure/mpr.ko] undefined!
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests
On Sat, 1 Apr 2023 11:10:11 +0200
Andreas Klinger <[email protected]> wrote:
Hi Andreas,
Various comments inline.
> Honeywell mpr is a familiy of pressure sensors.
Spell check.
>
> Add initial I2C support.
>
> Note:
> - Buffered mode is supported
> - SPI mode is not supported
>
> Signed-off-by: Andreas Klinger <[email protected]>
> ---
> drivers/iio/pressure/Kconfig | 12 ++
> drivers/iio/pressure/Makefile | 1 +
> drivers/iio/pressure/mpr.c | 307 ++++++++++++++++++++++++++++++++++
> 3 files changed, 320 insertions(+)
> create mode 100644 drivers/iio/pressure/mpr.c
>
> diff --git a/drivers/iio/pressure/Kconfig b/drivers/iio/pressure/Kconfig
> index c9453389e4f7..e3ec94036e3c 100644
> --- a/drivers/iio/pressure/Kconfig
> +++ b/drivers/iio/pressure/Kconfig
> @@ -148,6 +148,18 @@ config MPL3115
> To compile this driver as a module, choose M here: the module
> will be called mpl3115.
>
> +config MPR
> + tristate "Honeywell MPR (MicroPressure sensors)"
> + depends on I2C
> + select IIO_BUFFER
> + select IIO_TRIGGERED_BUFFER
> + help
> + Say Y here to build support for the Honeywell MicroPressure pressure sensors MPR.
> + There are many different types with different pressure range. These ranges can be set up
> + in the device tree.
> +
> + To compile this driver as a module, choose M here: the module will be called mpr.
> +
> config MS5611
> tristate "Measurement Specialties MS5611 pressure sensor driver"
> select IIO_BUFFER
> diff --git a/drivers/iio/pressure/Makefile b/drivers/iio/pressure/Makefile
> index 083ae87ff48f..b701d9c7187d 100644
> --- a/drivers/iio/pressure/Makefile
> +++ b/drivers/iio/pressure/Makefile
> @@ -19,6 +19,7 @@ obj-$(CONFIG_MPL115) += mpl115.o
> obj-$(CONFIG_MPL115_I2C) += mpl115_i2c.o
> obj-$(CONFIG_MPL115_SPI) += mpl115_spi.o
> obj-$(CONFIG_MPL3115) += mpl3115.o
> +obj-$(CONFIG_MPR) += mpr.o
> obj-$(CONFIG_MS5611) += ms5611_core.o
> obj-$(CONFIG_MS5611_I2C) += ms5611_i2c.o
> obj-$(CONFIG_MS5611_SPI) += ms5611_spi.o
> diff --git a/drivers/iio/pressure/mpr.c b/drivers/iio/pressure/mpr.c
> new file mode 100644
> index 000000000000..1728b42bee7e
> --- /dev/null
> +++ b/drivers/iio/pressure/mpr.c
> @@ -0,0 +1,307 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * MPR - MicroPressure pressure sensor driver
> + *
> + * Copyright (c) Andreas Klinger <[email protected]>
> + *
> + * Data sheet:
> + * https://prod-edam.honeywell.com/content/dam/honeywell-edam/sps/siot/en-us/products/sensors/ \
> + * pressure-sensors/board-mount-pressure-sensors/micropressure-mpr-series/documents/ \
> + * sps-siot-mpr-series-datasheet-32332628-ciid-172626.pdf
> + *
> + * 7-bit I2C default slave address: 0x18
> + *
Trivial but this blank line doesn't add anything useful to my eye
> + */
> +
> +#include <linux/of.h>
As below, should use stuff in linux/property.h not of.h
> +#include <linux/delay.h>
> +#include <linux/i2c.h>
> +#include <linux/module.h>
> +#include <linux/mod_devicetable.h>
> +
> +#include <linux/device.h>
> +#include <linux/gpio/consumer.h>
> +
> +#include <asm/unaligned.h>
> +
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
Not seeing any custom attributes (which is pretty much only reason you'd
need iio/sysfs.h)
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/triggered_buffer.h>
> +#include <linux/iio/trigger_consumer.h>
> +
> +/* bits in i2c status byte */
> +#define MPR_I2C_POWER BIT(6) /* device is powered */
> +#define MPR_I2C_BUSY BIT(5) /* device is busy */
> +#define MPR_I2C_MEMORY BIT(2) /* integrity test passed */
> +#define MPR_I2C_MATH BIT(0) /* internal math saturation */
> +
> +struct mpr_data {
> + struct device *dev;
Given it's easy to get this dev from client->dev, don't bother holding
both in here.
> + void *client;
> + struct mutex lock;
All locks should have a comment saying what exactly they protect.
> + s32 pmin;
> + s32 pmax;
> + struct gpio_desc *gpiod_reset;
> + int irq;
> + struct completion completion;
> + s64 channel[2] __aligned(8);
Why? Good to call out the explicit purpose of this. Usually easiest
option is to use a structure where you can call out the two elements
with names that tell reader what is going on.
> +};
> +
> +static int mpr_read_raw(struct iio_dev *indio_dev, struct iio_chan_spec const *chan, int *val,
> + int *val2, long mask);
Should be possible to reorder code so this isn't needed.
> +
> +static const struct iio_chan_spec mpr_channels[] = {
> + {
> + .type = IIO_PRESSURE,
> + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
> + .scan_index = 0,
> + .scan_type = {
> + .sign = 's',
> + .realbits = 64,
> + .storagebits = 64,
> + .endianness = IIO_CPU,
> + },
> + },
> + IIO_CHAN_SOFT_TIMESTAMP(1),
> +};
> +
> +static const struct iio_info mpr_info = {
> + .read_raw = &mpr_read_raw,
> +};
> +
> +static void mpr_reset(struct mpr_data *data)
> +{
> + if (data->gpiod_reset) {
> + gpiod_set_value(data->gpiod_reset, 0);
> + udelay(10);
> + gpiod_set_value(data->gpiod_reset, 1);
> + }
If there isn't a reset signal, I'd like to see an attempt at least to write
all configuration registers to a known value (same as the one you'd
get after reset).
> +}
> +
> +static int mpr_read_pressure(struct mpr_data *data, s64 *press)
> +{
> + int ret, i;
> + u8 wdata[] = {0xAA, 0x00, 0x00};
> + s32 status;
> + int nloops = 10;
> + char buf[5];
> + s64 press_cnt;
> + s64 outputmin = 1677722;
> + s64 outputmax = 15099494;
> +
> + reinit_completion(&data->completion);
> +
> + ret = i2c_master_send(data->client, wdata, sizeof(wdata));
> + if (ret < 0) {
> + dev_err(data->dev, "error while writing ret: %d\n", ret);
> + return ret;
> + }
> +
> + if (data->irq > 0) {
> + ret = wait_for_completion_timeout(&data->completion, HZ);
> + if (!ret) {
> + dev_err(data->dev, "timeout while waiting for eoc interrupt\n");
> + return -ETIMEDOUT;
> + }
> + } else {
> + /* wait until status indicates data is ready */
> + for (i = 0; i < nloops; i++) {
> + usleep_range(5000, 10000);
Add a comment on why this particular timing makes sense (reference to datasheet probably)
> + status = i2c_smbus_read_byte(data->client);
> + if (status < 0) {
> + dev_err(data->dev, "error while reading, status: %d\n", status);
> + return status;
> + }
> + if (!(status & MPR_I2C_BUSY))
> + break;
> + }
> + if (i == nloops) {
> + dev_err(data->dev, "timeout while reading\n");
> + return -ETIMEDOUT;
> + }
> + }
> +
> + ret = i2c_master_recv(data->client, buf, sizeof(buf));
> + if (ret < 0) {
> + dev_err(data->dev, "error in i2c_master_recv ret: %d\n", ret);
> + return ret;
> + }
> +
> + if (buf[0] & MPR_I2C_BUSY) {
> + /* it should never be the case that status still indicates business */
> + dev_err(data->dev, "data still not ready: %08x\n", buf[0]);
> + return -ETIMEDOUT;
> + }
> +
> + press_cnt = buf[3] + buf[2] * 256 + buf[1] * 65536;
Looks like a get_unaligned_le24() open coded? Use the standard function for
this as that makes it easier to see what is going on.
> +
> + *press = ((press_cnt - outputmin) * (s64)(data->pmax - data->pmin))
> + / (outputmax - outputmin) + (s64)data->pmin;
Some defines for the value of outputmin and outputmax would be better than
using local variables that happen to be constant.
Looks to me like this could be broken into
(raw + offset) * scale in which case you could use a _RAW interface with _OFFSET and
_SCALE allow you to make this into a 24 bit (well 32 bit) slot of a buffer should
you add buffered support later.
> +
> + dev_dbg(data->dev, "received: %*ph cnt: %lld press: %lld\n", ret, buf, press_cnt, *press);
> +
> + return 0;
> +}
> +
> +static irqreturn_t mpr_eoc_handler(int irq, void *p)
> +{
> + struct mpr_data *data = (struct mpr_data *)p;
> +
> + complete(&data->completion);
> +
> + return IRQ_HANDLED;
> +}
> +
> +static irqreturn_t mpr_trigger_handler(int irq, void *p)
> +{
> + int ret;
> + struct iio_poll_func *pf = p;
> + struct iio_dev *indio_dev = pf->indio_dev;
> + struct mpr_data *data = iio_priv(indio_dev);
> +
> + mutex_lock(&data->lock);
> + ret = mpr_read_pressure(data, &data->channel[0]);
> + if (ret < 0) {
> + mutex_unlock(&data->lock);
> + goto err;
> + }
> +
> + iio_push_to_buffers_with_timestamp(indio_dev, data->channel, iio_get_time_ns(indio_dev));
> + mutex_unlock(&data->lock);
> +
> +err:
> + iio_trigger_notify_done(indio_dev->trig);
> +
> + return IRQ_HANDLED;
> +}
> +
> +static int mpr_read_raw(struct iio_dev *indio_dev, struct iio_chan_spec const *chan, int *val,
> + int *val2, long mask)
> +{
> + int ret;
> + s64 pressure;
> + struct mpr_data *data = iio_priv(indio_dev);
> +
> + if (mask == IIO_CHAN_INFO_PROCESSED) {
Given you only support one option neater to error out early if it's not a match even if you
have two do that twice.
if (mask != IIO_CHAN_INFO_PROCESSED)
return -EINVAL;
> + mutex_lock(&data->lock);
> + ret = mpr_read_pressure(data, &pressure);
> + mutex_unlock(&data->lock);
> + if (ret < 0)
> + return ret;
> +
> + if (chan->type == IIO_PRESSURE) {
Check that earlier. Not a lot of point in reading the data if channel type
is wrong.
> + *val = (s32)clamp(pressure, 0LL, 2147483648LL);
What is special about that constant? If it's a device specific value then
given it a name.
> + return IIO_VAL_INT;
> + }
> + }
> +
> + return -EINVAL;
> +}
> +
> +static int mpr_probe(struct i2c_client *client, const struct i2c_device_id *id)
> +{
> + int ret;
> + struct mpr_data *data;
> + struct iio_dev *indio_dev;
> + struct device *dev = &client->dev;
> + struct device_node *np = dev->of_node;
As below, switch to non of specific firmware handlign.
> +
> + if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_READ_BYTE))
> + return -EOPNOTSUPP;
> +
> + indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> + if (!indio_dev)
> + return -ENOMEM;
> +
> + data = iio_priv(indio_dev);
> + data->dev = &client->dev;
> + i2c_set_clientdata(client, indio_dev);
I can't see where this is used.
> + data->client = client;
> + data->irq = client->irq;
> +
> + mutex_init(&data->lock);
> + init_completion(&data->completion);
> +
> + indio_dev->name = client->name;
Should be part number. As a string provided here (or more likely in a
device type specific structure that you are going to add).
> + indio_dev->info = &mpr_info;
> + indio_dev->channels = mpr_channels;
> + indio_dev->num_channels = ARRAY_SIZE(mpr_channels);
> + indio_dev->modes = INDIO_DIRECT_MODE;
> +
> + if (np) {
Don't use of specific interfaces. Use the ones from linux/property.h instead.
Lets other firmware types work 'for free' so we are trying to avoid introducing
any of specific handling into new IIO drivers (and slowly scrubbing the old ones
for this)·
if (dev_fwnode()) etc
> + if (of_property_read_s32(np, "honeywell,pmin", &data->pmin)) {
all these need to converting to the device_property_xxx equivalents.
> + dev_err(dev, "honeywell,pmin missing in DT\n");
> + return -EINVAL;
> + }
> + if (of_property_read_s32(np, "honeywell,pmax", &data->pmax)) {
> + dev_err(dev, "honeywell,pmax missing in DT\n");
> + return -EINVAL;
> + }
> +
> + data->gpiod_reset = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
Whilst these are sort of fw node dependent they don't need the protection.
So drop them out of this if (np) block.
devm_gpiod_get_optional() given you support not having it.
> + if (IS_ERR(data->gpiod_reset)) {
> + dev_err(dev, "failed to get reset-gpios: err=%pe\n",
> + data->gpiod_reset);
Not an error so dev_dbg() probably right thing to report.
> + data->gpiod_reset = NULL;
> + }
> +
> + if (data->irq > 0) {
> + ret = devm_request_irq(dev, data->irq, mpr_eoc_handler,
> + IRQF_TRIGGER_RISING, client->name, data);
> + if (ret) {
> + dev_err(dev, "request irq %d failed\n", data->irq);
> + return ret;
> + }
> + }
> + } else {
> + /* when loaded as i2c device we need to use default values */
> + dev_warn(dev, "no dt node found; using defaults\n");
A strong argument in favour of not handling all devices via one 'compatible'
(or the the i2c_device_id version of that anyway).
> + data->pmin = 0;
> + data->pmax = 172369; /* 25 psi */
Perhaps worth defining the maths for that conversion so that you can just use
psi_to_milibar(25); or whatever makes sense.
> + data->gpiod_reset = NULL;
Should be no need to set that as it will be NULL anyway in this path.
> + }
> +
> + mpr_reset(data);
> +
> + ret = devm_iio_triggered_buffer_setup(dev, indio_dev, NULL, mpr_trigger_handler, NULL);
> + if (ret < 0) {
> + dev_err(dev, "iio triggered buffer setup failed\n");
> + return ret;
> + }
> +
> + ret = devm_iio_device_register(dev, indio_dev);
> + if (ret < 0) {
> + dev_err(dev, "unable to register iio device\n");
In general use
return dev_err_probe(dev, ret, ...)
for all errors that only occur on probe. That wraps up some stuff around
deferred probing but is in general a good thing to do even if a given
call can't defer.
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static const struct of_device_id mpr_matches[] = {
> + { .compatible = "honeywell,mpr", .data = 0 },
No point in filling data either here or in the i2c_device_id
entries until something useful is done with it.
When you do (which is likely after request to use compatibles
to indicate a lot of the per device type variability) then you should look
for having the data as a pointer to a const structure that describes what those
per device type details.
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, mpr_matches);
> +
> +static const struct i2c_device_id mpr_id[] = {
> + { "honeywell,mpr", 0 },
> + { }
> +};
> +MODULE_DEVICE_TABLE(i2c, mpr_id);
> +
> +static struct i2c_driver mpr_driver = {
> + .probe = mpr_probe,
> + .id_table = mpr_id,
> + .driver = {
> + .name = "mpr",
> + .of_match_table = mpr_matches,
> + },
> +};
> +module_i2c_driver(mpr_driver);
> +
> +MODULE_AUTHOR("Andreas Klinger <[email protected]>");
> +MODULE_DESCRIPTION("MPR I2C driver");
> +MODULE_LICENSE("GPL");
> +MODULE_IMPORT_NS(IIO_MPR);
Why? You aren't exporting any symbols in that namespace that I can see.
Hi,
Looks pretty good. Jonathan already covered most of it, a few additional
comments.
On 4/1/23 02:10, Andreas Klinger wrote:
> [...]
> +struct mpr_data {
> + struct device *dev;
> + void *client;
Any reason not to use `struct i2c_client` for the type?
> + struct mutex lock;
> + s32 pmin;
> + s32 pmax;
> + struct gpio_desc *gpiod_reset;
> + int irq;
> + struct completion completion;
> + s64 channel[2] __aligned(8);
> +};
> +
> [...]
> +static int mpr_read_pressure(struct mpr_data *data, s64 *press)
> +{
> + int ret, i;
> + u8 wdata[] = {0xAA, 0x00, 0x00};
> + s32 status;
> + int nloops = 10;
> + char buf[5];
The tx buffer is `u8`, the rx buffer is `char`. This should be consistent.
> + s64 press_cnt;
> + s64 outputmin = 1677722;
> + s64 outputmax = 15099494;
> +
> + reinit_completion(&data->completion);
> +
> + ret = i2c_master_send(data->client, wdata, sizeof(wdata));
The i2c family of transfer functions returns the number of bytes
transferred, this can be less than what you expect if you get an early
NACK. Its always good to check that all the data was transferred. E.g.
if (ret >= 0 && ret != sizeof(wdata))
ret = -EIO;
Same for the receive later on.
[...]
Hi Andreas,
I love your patch! Yet something to improve:
[auto build test ERROR on jic23-iio/togreg]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Andreas-Klinger/dt-bindings-iio-pressure-Support-Honeywell-mpr-sensors/20230401-171226
base: https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
patch link: https://lore.kernel.org/r/ZCf085W4XL2PtQf6%40arbad
patch subject: [PATCH 2/3] iio: pressure: Honeywell mpr pressure sensor
config: arm-buildonly-randconfig-r004-20230401 (https://download.01.org/0day-ci/archive/20230402/[email protected]/config)
compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project 67409911353323ca5edf2049ef0df54132fa1ca7)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install arm cross compiling tool for clang build
# apt-get install binutils-arm-linux-gnueabi
# https://github.com/intel-lab-lkp/linux/commit/6a49dae45811d8a644c56dc18b6cdbc6ea67df98
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Andreas-Klinger/dt-bindings-iio-pressure-Support-Honeywell-mpr-sensors/20230401-171226
git checkout 6a49dae45811d8a644c56dc18b6cdbc6ea67df98
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm SHELL=/bin/bash
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
| Link: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All errors (new ones prefixed by >>, old ones prefixed by <<):
>> ERROR: modpost: "__aeabi_ldivmod" [drivers/iio/pressure/mpr.ko] undefined!
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests
Hi Jonathan,
thanks for the extensive review. Most of it is clear but one questions remain.
See below.
Jonathan Cameron <[email protected]> schrieb am Sa, 01. Apr 18:57:
> > +static void mpr_reset(struct mpr_data *data)
> > +{
> > + if (data->gpiod_reset) {
> > + gpiod_set_value(data->gpiod_reset, 0);
> > + udelay(10);
> > + gpiod_set_value(data->gpiod_reset, 1);
> > + }
>
> If there isn't a reset signal, I'd like to see an attempt at least to write
> all configuration registers to a known value (same as the one you'd
> get after reset).
There is no configuration register in the sensor I could write to. But maybe I
didn't comprehend your point.
Andreas
On Thu, 6 Apr 2023 21:43:57 +0200
Andreas Klinger <[email protected]> wrote:
> Hi Jonathan,
>
> thanks for the extensive review. Most of it is clear but one questions remain.
> See below.
>
> Jonathan Cameron <[email protected]> schrieb am Sa, 01. Apr 18:57:
> > > +static void mpr_reset(struct mpr_data *data)
> > > +{
> > > + if (data->gpiod_reset) {
> > > + gpiod_set_value(data->gpiod_reset, 0);
> > > + udelay(10);
> > > + gpiod_set_value(data->gpiod_reset, 1);
> > > + }
> >
> > If there isn't a reset signal, I'd like to see an attempt at least to write
> > all configuration registers to a known value (same as the one you'd
> > get after reset).
>
> There is no configuration register in the sensor I could write to. But maybe I
> didn't comprehend your point.
Ah. Devices is even simpler than I was anticipating. Which makes me wonder.
What does the reset actually do?
I checked the datasheet and reason to bother with this is about powersupplies
that don't come up fast enough. Fair enough. If someone hasn't wired
the reset I guess they are happy that the power on reset will work.
(4.0 Power support requirement)
Jonathan
>
> Andreas
>