[PATCH v2 0/3] iio: Add driver for Infineon DPS310

[PATCH v2 0/3] iio: Add driver for Infineon DPS310

[Eddie James]

The DPS310 is a temperature and pressure sensor. It can be accessed over i2c
and SPI.

The driver supports polled measurement of temperature and pressure over i2c
only.

Changes since v1:
 - Switch to wait for temperature/pressure sensor ready
 - Various cleanup

Christopher Bostic (1):
  iio: dps310: Temperature measurement errata

Eddie James (1):
  iio: dps310: Add pressure sensing capability

Joel Stanley (1):
  iio: Add driver for Infineon DPS310

 MAINTAINERS                   |   6 +
 drivers/iio/pressure/Kconfig  |  10 +
 drivers/iio/pressure/Makefile |   1 +
 drivers/iio/pressure/dps310.c | 761 ++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 778 insertions(+)
 create mode 100644 drivers/iio/pressure/dps310.c

-- 
1.8.3.1

[PATCH v2 1/3] iio: Add driver for Infineon DPS310

[Eddie James]

From: Joel Stanley <joel@jms.id.au>

The DPS310 is a temperature and pressure sensor. It can be accessed over
i2c and SPI.

Signed-off-by: Eddie James <eajames@linux.ibm.com>
---
 MAINTAINERS                   |   6 +
 drivers/iio/pressure/Kconfig  |  10 +
 drivers/iio/pressure/Makefile |   1 +
 drivers/iio/pressure/dps310.c | 429 ++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 446 insertions(+)
 create mode 100644 drivers/iio/pressure/dps310.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 7afaa1a..3d8c3d0 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7737,6 +7737,12 @@ W:	http://industrypack.sourceforge.net
 S:	Maintained
 F:	drivers/ipack/
 
+INFINEON DPS310 Driver
+M:	Eddie James <eajames@linux.ibm.com>
+L:	linux-iio@vger.kernel.org
+F:	drivers/iio/pressure/dps310.c
+S:	Maintained
+
 INFINIBAND SUBSYSTEM
 M:	Doug Ledford <dledford@redhat.com>
 M:	Jason Gunthorpe <jgg@mellanox.com>
diff --git a/drivers/iio/pressure/Kconfig b/drivers/iio/pressure/Kconfig
index efeb89f..681a1cc 100644
--- a/drivers/iio/pressure/Kconfig
+++ b/drivers/iio/pressure/Kconfig
@@ -52,6 +52,16 @@ config IIO_CROS_EC_BARO
 	  To compile this driver as a module, choose M here: the module
 	  will be called cros_ec_baro.
 
+config DPS310
+       tristate "Infineon DPS310 pressure and temperature sensor"
+       depends on I2C
+       select REGMAP_I2C
+       help
+	 Support for the Infineon DPS310 digital barometric pressure sensor.
+
+	 This driver can also be built as a module.  If so, the module will be
+	 called dps310.
+
 config HID_SENSOR_PRESS
 	depends on HID_SENSOR_HUB
 	select IIO_BUFFER
diff --git a/drivers/iio/pressure/Makefile b/drivers/iio/pressure/Makefile
index c2058d7..d8f5ace 100644
--- a/drivers/iio/pressure/Makefile
+++ b/drivers/iio/pressure/Makefile
@@ -9,6 +9,7 @@ obj-$(CONFIG_BMP280) += bmp280.o
 bmp280-objs := bmp280-core.o bmp280-regmap.o
 obj-$(CONFIG_BMP280_I2C) += bmp280-i2c.o
 obj-$(CONFIG_BMP280_SPI) += bmp280-spi.o
+obj-$(CONFIG_DPS310) += dps310.o
 obj-$(CONFIG_IIO_CROS_EC_BARO) += cros_ec_baro.o
 obj-$(CONFIG_HID_SENSOR_PRESS)   += hid-sensor-press.o
 obj-$(CONFIG_HP03) += hp03.o
diff --git a/drivers/iio/pressure/dps310.c b/drivers/iio/pressure/dps310.c
new file mode 100644
index 0000000..7afaa88
--- /dev/null
+++ b/drivers/iio/pressure/dps310.c
@@ -0,0 +1,429 @@
+// SPDX-License-Identifier: GPL-2.0+
+// Copyright IBM Corp 2019
+/*
+ * The DPS310 is a barometric pressure and temperature sensor.
+ * Currently only reading a single temperature is supported by
+ * this driver.
+ *
+ * https://www.infineon.com/dgdl/?fileId=5546d462576f34750157750826c42242
+ *
+ * Temperature calculation:
+ *   c0 * 0.5 + c1 * T_raw / kT °C
+ *
+ * TODO:
+ *  - Pressure sensor readings
+ *  - Optionally support the FIFO
+ */
+
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/regmap.h>
+
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+
+#define DPS310_PRS_B0		0x00
+#define DPS310_PRS_B1		0x01
+#define DPS310_PRS_B2		0x02
+#define DPS310_TMP_B0		0x03
+#define DPS310_TMP_B1		0x04
+#define DPS310_TMP_B2		0x05
+#define DPS310_PRS_CFG		0x06
+#define DPS310_TMP_CFG		0x07
+#define  DPS310_TMP_RATE_BITS	GENMASK(6, 4)
+#define  DPS310_TMP_PRC_BITS	GENMASK(3, 0)
+#define  DPS310_TMP_EXT		BIT(7)
+#define DPS310_MEAS_CFG		0x08
+#define  DPS310_MEAS_CTRL_BITS	GENMASK(2, 0)
+#define   DPS310_PRS_EN		BIT(0)
+#define   DPS310_TEMP_EN	BIT(1)
+#define   DPS310_BACKGROUND	BIT(2)
+#define  DPS310_PRS_RDY		BIT(4)
+#define  DPS310_TMP_RDY		BIT(5)
+#define  DPS310_SENSOR_RDY	BIT(6)
+#define  DPS310_COEF_RDY	BIT(7)
+#define DPS310_CFG_REG		0x09
+#define  DPS310_INT_HL		BIT(7)
+#define  DPS310_TMP_SHIFT_EN	BIT(3)
+#define  DPS310_PRS_SHIFT_EN	BIT(4)
+#define  DPS310_FIFO_EN		BIT(5)
+#define  DPS310_SPI_EN		BIT(6)
+#define DPS310_RESET		0x0c
+#define  DPS310_RESET_MAGIC	(BIT(0) | BIT(3))
+#define DPS310_COEF_BASE	0x10
+
+/* Make sure sleep time is <= 20ms for usleep_range */
+#define DPS310_POLL_SLEEP_US(t)		((t) > 160000 ? 20000 : (t) / 8)
+#define DPS310_POLL_TIMEOUT_US(r)	((r) <= 0 ? 1000000 : 1000000 / (r))
+
+#define DPS310_PRS_BASE		DPS310_PRS_B0
+#define DPS310_TMP_BASE		DPS310_TMP_B0
+
+#define DPS310_CALC_RATE(_n)	ilog2(_n)
+#define DPS310_CALC_PRC(_n)	ilog2(_n)
+
+/*
+ * These values (defined in the spec) indicate how to scale the raw register
+ * values for each level of precision available.
+ */
+static const int scale_factors[] = {
+	 524288,
+	1572864,
+	3670016,
+	7864320,
+	 253952,
+	 516096,
+	1040384,
+	2088960,
+};
+
+struct dps310_data {
+	struct i2c_client *client;
+	struct regmap *regmap;
+
+	s32 c0, c1;
+	s32 temp_raw;
+};
+
+static const struct iio_chan_spec dps310_channels[] = {
+	{
+		.type = IIO_TEMP,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_OFFSET) |
+			BIT(IIO_CHAN_INFO_SCALE) |
+			BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO) |
+			BIT(IIO_CHAN_INFO_SAMP_FREQ) |
+			BIT(IIO_CHAN_INFO_RAW),
+	},
+};
+
+/* To be called after checking the COEF_RDY bit in MEAS_CFG */
+static int dps310_get_temp_coef(struct dps310_data *data)
+{
+	struct regmap *regmap = data->regmap;
+	u8 coef[3];
+	int r;
+	u32 c0, c1;
+
+	/*
+	 * Read temperature calibration coefficients c0 and c1 from the
+	 * COEF register. The numbers are 12-bit 2's compliment numbers
+	 */
+	r = regmap_bulk_read(regmap, DPS310_COEF_BASE, coef, sizeof(coef));
+	if (r < 0)
+		return r;
+
+	c0 = (coef[0] << 4) | (coef[1] >> 4);
+	data->c0 = sign_extend32(c0, 11);
+
+	c1 = ((coef[1] & GENMASK(3, 0)) << 8) | coef[2];
+	data->c1 = sign_extend32(c1, 11);
+
+	return 0;
+}
+
+static int dps310_get_temp_precision(struct dps310_data *data)
+{
+	int val, r;
+
+	r = regmap_read(data->regmap, DPS310_TMP_CFG, &val);
+	if (r < 0)
+		return r;
+
+	/*
+	 * Scale factor is bottom 4 bits of the register, but 1111 is
+	 * reserved so just grab bottom three
+	 */
+	return BIT(val & GENMASK(2, 0));
+}
+
+static int dps310_set_temp_precision(struct dps310_data *data, int val)
+{
+	int ret;
+	u8 shift_en;
+
+	if (val < 0 || val > 128)
+		return -EINVAL;
+
+	shift_en = val >= 16 ? DPS310_TMP_SHIFT_EN : 0;
+	ret = regmap_write_bits(data->regmap, DPS310_CFG_REG,
+				DPS310_TMP_SHIFT_EN, shift_en);
+	if (ret)
+		return ret;
+
+	return regmap_update_bits(data->regmap, DPS310_TMP_CFG,
+				  DPS310_TMP_PRC_BITS, DPS310_CALC_PRC(val));
+}
+
+static int dps310_set_temp_samp_freq(struct dps310_data *data, int freq)
+{
+	u8 val;
+
+	if (freq < 0 || freq > 128)
+		return -EINVAL;
+
+	val = DPS310_CALC_RATE(freq) << 4;
+
+	return regmap_update_bits(data->regmap, DPS310_TMP_CFG,
+				  DPS310_TMP_RATE_BITS, val);
+}
+
+static int dps310_get_temp_samp_freq(struct dps310_data *data)
+{
+	int val, r;
+
+	r = regmap_read(data->regmap, DPS310_TMP_CFG, &val);
+	if (r < 0)
+		return r;
+
+	return BIT((val & DPS310_TMP_RATE_BITS) >> 4);
+}
+
+static int dps310_get_temp_k(struct dps310_data *data)
+{
+	int r = dps310_get_temp_precision(data);
+
+	if (r < 0)
+		return r;
+
+	return scale_factors[DPS310_CALC_PRC(r)];
+}
+
+static int dps310_read_temp(struct dps310_data *data)
+{
+	u8 val[3];
+	s32 raw;
+	int r, ready;
+	int rate = dps310_get_temp_samp_freq(data);
+	int timeout = DPS310_POLL_TIMEOUT_US(rate);
+
+	/* Poll for sensor readiness; base the timeout upon the sample rate. */
+	r = regmap_read_poll_timeout(data->regmap, DPS310_MEAS_CFG, ready,
+				     ready & DPS310_TMP_RDY,
+				     DPS310_POLL_SLEEP_US(timeout), timeout);
+	if (r < 0)
+		return r;
+
+	r = regmap_bulk_read(data->regmap, DPS310_TMP_BASE, val, sizeof(val));
+	if (r < 0)
+		return r;
+
+	raw = (val[0] << 16) | (val[1] << 8) | val[2];
+	data->temp_raw = sign_extend32(raw, 23);
+
+	return 0;
+}
+
+static bool dps310_is_writeable_reg(struct device *dev, unsigned int reg)
+{
+	switch (reg) {
+	case DPS310_PRS_CFG:
+	case DPS310_TMP_CFG:
+	case DPS310_MEAS_CFG:
+	case DPS310_CFG_REG:
+	case DPS310_RESET:
+		return true;
+	default:
+		return false;
+	}
+}
+
+static bool dps310_is_volatile_reg(struct device *dev, unsigned int reg)
+{
+	switch (reg) {
+	case DPS310_PRS_B0:
+	case DPS310_PRS_B1:
+	case DPS310_PRS_B2:
+	case DPS310_TMP_B0:
+	case DPS310_TMP_B1:
+	case DPS310_TMP_B2:
+	case DPS310_MEAS_CFG:
+		return true;
+	default:
+		return false;
+	}
+}
+
+static int dps310_write_raw(struct iio_dev *iio,
+			    struct iio_chan_spec const *chan, int val,
+			    int val2, long mask)
+{
+	struct dps310_data *data = iio_priv(iio);
+
+	if (chan->type != IIO_TEMP)
+		return -EINVAL;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		return dps310_set_temp_samp_freq(data, val);
+	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
+		return dps310_set_temp_precision(data, val);
+	default:
+		return -EINVAL;
+	}
+}
+
+static int dps310_read_raw(struct iio_dev *iio,
+			   struct iio_chan_spec const *chan,
+			   int *val, int *val2, long mask)
+{
+	struct dps310_data *data = iio_priv(iio);
+	int ret;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		*val = dps310_get_temp_samp_freq(data);
+		return IIO_VAL_INT;
+
+	case IIO_CHAN_INFO_RAW:
+		ret = dps310_read_temp(data);
+		if (ret)
+			return ret;
+
+		*val = data->temp_raw * data->c1;
+		return IIO_VAL_INT;
+
+	case IIO_CHAN_INFO_OFFSET:
+		ret = dps310_get_temp_k(data);
+		if (ret < 0)
+			return ret;
+
+		*val = (data->c0 >> 1) * ret;
+		return IIO_VAL_INT;
+
+	case IIO_CHAN_INFO_SCALE:
+		ret = dps310_get_temp_k(data);
+		if (ret < 0)
+			return ret;
+
+		*val = 1000; /* milliCelsius per Celsius */
+		*val2 = ret;
+		return IIO_VAL_FRACTIONAL;
+
+	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
+		*val = dps310_get_temp_precision(data);
+		return IIO_VAL_INT;
+
+	default:
+		return -EINVAL;
+	}
+}
+
+static const struct regmap_config dps310_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+	.writeable_reg = dps310_is_writeable_reg,
+	.volatile_reg = dps310_is_volatile_reg,
+	.cache_type = REGCACHE_RBTREE,
+	.max_register = 0x29,
+};
+
+static const struct iio_info dps310_info = {
+	.read_raw = dps310_read_raw,
+	.write_raw = dps310_write_raw,
+};
+
+static int dps310_probe(struct i2c_client *client,
+			const struct i2c_device_id *id)
+{
+	struct dps310_data *data;
+	struct iio_dev *iio;
+	int r, ready;
+
+	iio = devm_iio_device_alloc(&client->dev,  sizeof(*data));
+	if (!iio)
+		return -ENOMEM;
+
+	data = iio_priv(iio);
+	data->client = client;
+
+	iio->dev.parent = &client->dev;
+	iio->name = id->name;
+	iio->channels = dps310_channels;
+	iio->num_channels = ARRAY_SIZE(dps310_channels);
+	iio->info = &dps310_info;
+	iio->modes = INDIO_DIRECT_MODE;
+
+	data->regmap = devm_regmap_init_i2c(client, &dps310_regmap_config);
+	if (IS_ERR(data->regmap))
+		return PTR_ERR(data->regmap);
+
+	/*
+	 * Set up external (MEMS) temperature sensor in single sample, one
+	 * measurement per second mode
+	 */
+	r = regmap_write(data->regmap, DPS310_TMP_CFG,
+			 DPS310_TMP_EXT | DPS310_CALC_RATE(1) |
+			 DPS310_CALC_PRC(1));
+	if (r < 0)
+		goto err;
+
+	/* Temp shift is disabled when PRC <= 8 */
+	r = regmap_write_bits(data->regmap, DPS310_CFG_REG,
+			      DPS310_TMP_SHIFT_EN, 0);
+	if (r < 0)
+		goto err;
+
+	/* Turn on temperature measurement in the background */
+	r = regmap_write_bits(data->regmap, DPS310_MEAS_CFG,
+			      DPS310_MEAS_CTRL_BITS,
+			      DPS310_TEMP_EN | DPS310_BACKGROUND);
+	if (r < 0)
+		goto err;
+
+	/*
+	 * Calibration coefficients required for reporting temperature.
+	 * They are available 40ms after the device has started
+	 */
+	r = regmap_read_poll_timeout(data->regmap, DPS310_MEAS_CFG, ready,
+				     ready & DPS310_COEF_RDY, 10000, 40000);
+	if (r < 0)
+		goto err;
+
+	r = dps310_get_temp_coef(data);
+	if (r < 0)
+		goto err;
+
+	r = devm_iio_device_register(&client->dev, iio);
+	if (r)
+		goto err;
+
+	i2c_set_clientdata(client, iio);
+
+	return 0;
+
+err:
+	regmap_write(data->regmap, DPS310_RESET, DPS310_RESET_MAGIC);
+	return r;
+}
+
+static int dps310_remove(struct i2c_client *client)
+{
+	struct dps310_data *data = i2c_get_clientdata(client);
+
+	return regmap_write(data->regmap, DPS310_RESET, DPS310_RESET_MAGIC);
+}
+
+static const struct i2c_device_id dps310_id[] = {
+	{ "dps310", 0 },
+	{}
+};
+MODULE_DEVICE_TABLE(i2c, dps310_id);
+
+static const unsigned short normal_i2c[] = {
+	0x77, 0x76, I2C_CLIENT_END
+};
+
+static struct i2c_driver dps310_driver = {
+	.driver = {
+		.name = "dps310",
+	},
+	.probe = dps310_probe,
+	.remove = dps310_remove,
+	.address_list = normal_i2c,
+	.id_table = dps310_id,
+};
+module_i2c_driver(dps310_driver);
+
+MODULE_AUTHOR("Joel Stanley <joel@jms.id.au>");
+MODULE_DESCRIPTION("Infineon DPS310 pressure and temperature sensor");
+MODULE_LICENSE("GPL v2");
-- 
1.8.3.1

Re: [PATCH v2 1/3] iio: Add driver for Infineon DPS310

[Jonathan Cameron]

On Wed,  8 May 2019 14:35:26 -0500
Eddie James <eajames@linux.ibm.com> wrote:

> From: Joel Stanley <joel@jms.id.au>
> 
> The DPS310 is a temperature and pressure sensor. It can be accessed over
> i2c and SPI.
> 
> Signed-off-by: Eddie James <eajames@linux.ibm.com>
Hi Eddie,

Ideally we'll get a sign off form Joel as well on this.

A few comments inline.

I 'think' this is probably fine without any locking to prevent simultaneous reads
and /or writes to the registers because the few functions that do multiple reads
and writes look fine.  Please do take another look at that though to confirm there
are no corner cases.

Otherwise there is a race in the remove path that needs fixing.
Various minor bits and bobs inline.

thanks,

Jonathan



> ---
>  MAINTAINERS                   |   6 +
>  drivers/iio/pressure/Kconfig  |  10 +
>  drivers/iio/pressure/Makefile |   1 +
>  drivers/iio/pressure/dps310.c | 429 ++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 446 insertions(+)
>  create mode 100644 drivers/iio/pressure/dps310.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 7afaa1a..3d8c3d0 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -7737,6 +7737,12 @@ W:	http://industrypack.sourceforge.net
>  S:	Maintained
>  F:	drivers/ipack/
>  
> +INFINEON DPS310 Driver
> +M:	Eddie James <eajames@linux.ibm.com>
> +L:	linux-iio@vger.kernel.org
> +F:	drivers/iio/pressure/dps310.c
> +S:	Maintained
> +
>  INFINIBAND SUBSYSTEM
>  M:	Doug Ledford <dledford@redhat.com>
>  M:	Jason Gunthorpe <jgg@mellanox.com>
> diff --git a/drivers/iio/pressure/Kconfig b/drivers/iio/pressure/Kconfig
> index efeb89f..681a1cc 100644
> --- a/drivers/iio/pressure/Kconfig
> +++ b/drivers/iio/pressure/Kconfig
> @@ -52,6 +52,16 @@ config IIO_CROS_EC_BARO
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called cros_ec_baro.
>  
> +config DPS310
> +       tristate "Infineon DPS310 pressure and temperature sensor"
> +       depends on I2C
> +       select REGMAP_I2C
> +       help
> +	 Support for the Infineon DPS310 digital barometric pressure sensor.
> +
> +	 This driver can also be built as a module.  If so, the module will be
> +	 called dps310.
> +
>  config HID_SENSOR_PRESS
>  	depends on HID_SENSOR_HUB
>  	select IIO_BUFFER
> diff --git a/drivers/iio/pressure/Makefile b/drivers/iio/pressure/Makefile
> index c2058d7..d8f5ace 100644
> --- a/drivers/iio/pressure/Makefile
> +++ b/drivers/iio/pressure/Makefile
> @@ -9,6 +9,7 @@ obj-$(CONFIG_BMP280) += bmp280.o
>  bmp280-objs := bmp280-core.o bmp280-regmap.o
>  obj-$(CONFIG_BMP280_I2C) += bmp280-i2c.o
>  obj-$(CONFIG_BMP280_SPI) += bmp280-spi.o
> +obj-$(CONFIG_DPS310) += dps310.o
>  obj-$(CONFIG_IIO_CROS_EC_BARO) += cros_ec_baro.o
>  obj-$(CONFIG_HID_SENSOR_PRESS)   += hid-sensor-press.o
>  obj-$(CONFIG_HP03) += hp03.o
> diff --git a/drivers/iio/pressure/dps310.c b/drivers/iio/pressure/dps310.c
> new file mode 100644
> index 0000000..7afaa88
> --- /dev/null
> +++ b/drivers/iio/pressure/dps310.c
> @@ -0,0 +1,429 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +// Copyright IBM Corp 2019
> +/*
> + * The DPS310 is a barometric pressure and temperature sensor.
> + * Currently only reading a single temperature is supported by
> + * this driver.
> + *
> + * https://www.infineon.com/dgdl/?fileId=5546d462576f34750157750826c42242
> + *
> + * Temperature calculation:
> + *   c0 * 0.5 + c1 * T_raw / kT °C
> + *
> + * TODO:
> + *  - Pressure sensor readings
> + *  - Optionally support the FIFO
> + */
> +
> +#include <linux/i2c.h>
> +#include <linux/module.h>
> +#include <linux/regmap.h>
> +
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +
> +#define DPS310_PRS_B0		0x00
> +#define DPS310_PRS_B1		0x01
> +#define DPS310_PRS_B2		0x02
> +#define DPS310_TMP_B0		0x03
> +#define DPS310_TMP_B1		0x04
> +#define DPS310_TMP_B2		0x05
> +#define DPS310_PRS_CFG		0x06
> +#define DPS310_TMP_CFG		0x07
> +#define  DPS310_TMP_RATE_BITS	GENMASK(6, 4)
> +#define  DPS310_TMP_PRC_BITS	GENMASK(3, 0)
> +#define  DPS310_TMP_EXT		BIT(7)
> +#define DPS310_MEAS_CFG		0x08
> +#define  DPS310_MEAS_CTRL_BITS	GENMASK(2, 0)
> +#define   DPS310_PRS_EN		BIT(0)
> +#define   DPS310_TEMP_EN	BIT(1)
> +#define   DPS310_BACKGROUND	BIT(2)
> +#define  DPS310_PRS_RDY		BIT(4)
> +#define  DPS310_TMP_RDY		BIT(5)
> +#define  DPS310_SENSOR_RDY	BIT(6)
> +#define  DPS310_COEF_RDY	BIT(7)
> +#define DPS310_CFG_REG		0x09
> +#define  DPS310_INT_HL		BIT(7)
> +#define  DPS310_TMP_SHIFT_EN	BIT(3)
> +#define  DPS310_PRS_SHIFT_EN	BIT(4)
> +#define  DPS310_FIFO_EN		BIT(5)
> +#define  DPS310_SPI_EN		BIT(6)
> +#define DPS310_RESET		0x0c
> +#define  DPS310_RESET_MAGIC	(BIT(0) | BIT(3))
Is this expressed as bits in the datasheet?  Superficially I would say this
would be more readable as a simple number.

> +#define DPS310_COEF_BASE	0x10
> +
> +/* Make sure sleep time is <= 20ms for usleep_range */
> +#define DPS310_POLL_SLEEP_US(t)		((t) > 160000 ? 20000 : (t) / 8)
min(20000, (t)/8) is perhaps more readable?

> +#define DPS310_POLL_TIMEOUT_US(r)	((r) <= 0 ? 1000000 : 1000000 / (r))
I'm curious.  How does r ever end up as less than 0?

> +
> +#define DPS310_PRS_BASE		DPS310_PRS_B0
> +#define DPS310_TMP_BASE		DPS310_TMP_B0
> +
> +#define DPS310_CALC_RATE(_n)	ilog2(_n)
> +#define DPS310_CALC_PRC(_n)	ilog2(_n)
Why? I'm not convinced these defines help readability over just using ilog2 inline.

> +
> +/*
> + * These values (defined in the spec) indicate how to scale the raw register
> + * values for each level of precision available.
> + */
> +static const int scale_factors[] = {
> +	 524288,
> +	1572864,
> +	3670016,
> +	7864320,
> +	 253952,
> +	 516096,
> +	1040384,
> +	2088960,
> +};
> +
> +struct dps310_data {
> +	struct i2c_client *client;
> +	struct regmap *regmap;
> +
> +	s32 c0, c1;
> +	s32 temp_raw;
> +};
> +
> +static const struct iio_chan_spec dps310_channels[] = {
> +	{
> +		.type = IIO_TEMP,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_OFFSET) |
> +			BIT(IIO_CHAN_INFO_SCALE) |
> +			BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO) |
> +			BIT(IIO_CHAN_INFO_SAMP_FREQ) |
> +			BIT(IIO_CHAN_INFO_RAW),
> +	},
> +};
> +
> +/* To be called after checking the COEF_RDY bit in MEAS_CFG */
> +static int dps310_get_temp_coef(struct dps310_data *data)
> +{
> +	struct regmap *regmap = data->regmap;
> +	u8 coef[3];
> +	int r;
> +	u32 c0, c1;
> +
> +	/*
> +	 * Read temperature calibration coefficients c0 and c1 from the
> +	 * COEF register. The numbers are 12-bit 2's compliment numbers
> +	 */
> +	r = regmap_bulk_read(regmap, DPS310_COEF_BASE, coef, sizeof(coef));
> +	if (r < 0)
> +		return r;
> +
> +	c0 = (coef[0] << 4) | (coef[1] >> 4);
> +	data->c0 = sign_extend32(c0, 11);
> +
> +	c1 = ((coef[1] & GENMASK(3, 0)) << 8) | coef[2];
> +	data->c1 = sign_extend32(c1, 11);
> +
> +	return 0;
> +}
> +
> +static int dps310_get_temp_precision(struct dps310_data *data)
> +{
> +	int val, r;
> +
> +	r = regmap_read(data->regmap, DPS310_TMP_CFG, &val);
> +	if (r < 0)
> +		return r;
> +
> +	/*
> +	 * Scale factor is bottom 4 bits of the register, but 1111 is
> +	 * reserved so just grab bottom three
> +	 */
> +	return BIT(val & GENMASK(2, 0));
> +}
> +
> +static int dps310_set_temp_precision(struct dps310_data *data, int val)
> +{
> +	int ret;
> +	u8 shift_en;
> +
> +	if (val < 0 || val > 128)
> +		return -EINVAL;
> +
> +	shift_en = val >= 16 ? DPS310_TMP_SHIFT_EN : 0;
> +	ret = regmap_write_bits(data->regmap, DPS310_CFG_REG,
> +				DPS310_TMP_SHIFT_EN, shift_en);
> +	if (ret)
> +		return ret;

There isn't any locking around these.  Is there any potential problem if
a read occurs between these two calls?  It may be fine - I haven't checked
but this looks risky.

> +
> +	return regmap_update_bits(data->regmap, DPS310_TMP_CFG,
> +				  DPS310_TMP_PRC_BITS, DPS310_CALC_PRC(val));
> +}
> +
> +static int dps310_set_temp_samp_freq(struct dps310_data *data, int freq)
> +{
> +	u8 val;
> +
> +	if (freq < 0 || freq > 128)
> +		return -EINVAL;
> +
> +	val = DPS310_CALC_RATE(freq) << 4;
> +
> +	return regmap_update_bits(data->regmap, DPS310_TMP_CFG,
> +				  DPS310_TMP_RATE_BITS, val);
> +}
> +
> +static int dps310_get_temp_samp_freq(struct dps310_data *data)
> +{
> +	int val, r;
> +
> +	r = regmap_read(data->regmap, DPS310_TMP_CFG, &val);
> +	if (r < 0)
> +		return r;
> +
> +	return BIT((val & DPS310_TMP_RATE_BITS) >> 4);
> +}
> +
> +static int dps310_get_temp_k(struct dps310_data *data)
> +{
> +	int r = dps310_get_temp_precision(data);
> +
> +	if (r < 0)
> +		return r;
> +
> +	return scale_factors[DPS310_CALC_PRC(r)];
> +}
> +
> +static int dps310_read_temp(struct dps310_data *data)
> +{
> +	u8 val[3];
> +	s32 raw;
> +	int r, ready;
> +	int rate = dps310_get_temp_samp_freq(data);
> +	int timeout = DPS310_POLL_TIMEOUT_US(rate);

Are there any potential races in these functions?  Note that there is nothing
stopping mulitple calls to read_raw occurring at the same time.

> +
> +	/* Poll for sensor readiness; base the timeout upon the sample rate. */
> +	r = regmap_read_poll_timeout(data->regmap, DPS310_MEAS_CFG, ready,
> +				     ready & DPS310_TMP_RDY,
> +				     DPS310_POLL_SLEEP_US(timeout), timeout);
> +	if (r < 0)
> +		return r;
> +
> +	r = regmap_bulk_read(data->regmap, DPS310_TMP_BASE, val, sizeof(val));
> +	if (r < 0)
> +		return r;
> +
> +	raw = (val[0] << 16) | (val[1] << 8) | val[2];
> +	data->temp_raw = sign_extend32(raw, 23);
> +
> +	return 0;
> +}
> +
> +static bool dps310_is_writeable_reg(struct device *dev, unsigned int reg)
> +{
> +	switch (reg) {
> +	case DPS310_PRS_CFG:
> +	case DPS310_TMP_CFG:
> +	case DPS310_MEAS_CFG:
> +	case DPS310_CFG_REG:
> +	case DPS310_RESET:
> +		return true;
> +	default:
> +		return false;
> +	}
> +}
> +
> +static bool dps310_is_volatile_reg(struct device *dev, unsigned int reg)
> +{
> +	switch (reg) {
> +	case DPS310_PRS_B0:
> +	case DPS310_PRS_B1:
> +	case DPS310_PRS_B2:
> +	case DPS310_TMP_B0:
> +	case DPS310_TMP_B1:
> +	case DPS310_TMP_B2:
> +	case DPS310_MEAS_CFG:
> +		return true;
> +	default:
> +		return false;
> +	}
> +}
> +
> +static int dps310_write_raw(struct iio_dev *iio,
> +			    struct iio_chan_spec const *chan, int val,
> +			    int val2, long mask)
> +{
> +	struct dps310_data *data = iio_priv(iio);
> +
> +	if (chan->type != IIO_TEMP)
> +		return -EINVAL;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		return dps310_set_temp_samp_freq(data, val);
> +	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> +		return dps310_set_temp_precision(data, val);
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int dps310_read_raw(struct iio_dev *iio,
> +			   struct iio_chan_spec const *chan,
> +			   int *val, int *val2, long mask)
> +{
> +	struct dps310_data *data = iio_priv(iio);
> +	int ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		*val = dps310_get_temp_samp_freq(data);
> +		return IIO_VAL_INT;
> +
> +	case IIO_CHAN_INFO_RAW:
> +		ret = dps310_read_temp(data);
> +		if (ret)
> +			return ret;
> +
> +		*val = data->temp_raw * data->c1;
> +		return IIO_VAL_INT;
> +
> +	case IIO_CHAN_INFO_OFFSET:
> +		ret = dps310_get_temp_k(data);
> +		if (ret < 0)
> +			return ret;
> +
> +		*val = (data->c0 >> 1) * ret;
> +		return IIO_VAL_INT;
> +
> +	case IIO_CHAN_INFO_SCALE:
> +		ret = dps310_get_temp_k(data);
> +		if (ret < 0)
> +			return ret;
> +
> +		*val = 1000; /* milliCelsius per Celsius */
> +		*val2 = ret;
> +		return IIO_VAL_FRACTIONAL;
> +
> +	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> +		*val = dps310_get_temp_precision(data);
> +		return IIO_VAL_INT;
> +
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static const struct regmap_config dps310_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +	.writeable_reg = dps310_is_writeable_reg,
> +	.volatile_reg = dps310_is_volatile_reg,
> +	.cache_type = REGCACHE_RBTREE,
> +	.max_register = 0x29,
Normally a define relating to the last register name would
be used here.

> +};
> +
> +static const struct iio_info dps310_info = {
> +	.read_raw = dps310_read_raw,
> +	.write_raw = dps310_write_raw,
> +};
> +
> +static int dps310_probe(struct i2c_client *client,
> +			const struct i2c_device_id *id)
> +{
> +	struct dps310_data *data;
> +	struct iio_dev *iio;
> +	int r, ready;
> +
> +	iio = devm_iio_device_alloc(&client->dev,  sizeof(*data));
> +	if (!iio)
> +		return -ENOMEM;
> +
> +	data = iio_priv(iio);
> +	data->client = client;
> +
> +	iio->dev.parent = &client->dev;
> +	iio->name = id->name;
> +	iio->channels = dps310_channels;
> +	iio->num_channels = ARRAY_SIZE(dps310_channels);
> +	iio->info = &dps310_info;
> +	iio->modes = INDIO_DIRECT_MODE;
> +
> +	data->regmap = devm_regmap_init_i2c(client, &dps310_regmap_config);
> +	if (IS_ERR(data->regmap))
> +		return PTR_ERR(data->regmap);
> +
> +	/*
> +	 * Set up external (MEMS) temperature sensor in single sample, one
> +	 * measurement per second mode
> +	 */
> +	r = regmap_write(data->regmap, DPS310_TMP_CFG,
> +			 DPS310_TMP_EXT | DPS310_CALC_RATE(1) |
> +			 DPS310_CALC_PRC(1));
> +	if (r < 0)
> +		goto err;
> +
> +	/* Temp shift is disabled when PRC <= 8 */
> +	r = regmap_write_bits(data->regmap, DPS310_CFG_REG,
> +			      DPS310_TMP_SHIFT_EN, 0);
> +	if (r < 0)
> +		goto err;
> +
> +	/* Turn on temperature measurement in the background */
> +	r = regmap_write_bits(data->regmap, DPS310_MEAS_CFG,
> +			      DPS310_MEAS_CTRL_BITS,
> +			      DPS310_TEMP_EN | DPS310_BACKGROUND);
> +	if (r < 0)
> +		goto err;
> +
> +	/*
> +	 * Calibration coefficients required for reporting temperature.
> +	 * They are available 40ms after the device has started
> +	 */
> +	r = regmap_read_poll_timeout(data->regmap, DPS310_MEAS_CFG, ready,
> +				     ready & DPS310_COEF_RDY, 10000, 40000);
> +	if (r < 0)
> +		goto err;
> +
> +	r = dps310_get_temp_coef(data);
> +	if (r < 0)
> +		goto err;
> +
> +	r = devm_iio_device_register(&client->dev, iio);
> +	if (r)
> +		goto err;
> +
> +	i2c_set_clientdata(client, iio);
> +
> +	return 0;
> +
> +err:
> +	regmap_write(data->regmap, DPS310_RESET, DPS310_RESET_MAGIC);
> +	return r;
> +}
> +
> +static int dps310_remove(struct i2c_client *client)
> +{
> +	struct dps310_data *data = i2c_get_clientdata(client);
> +
> +	return regmap_write(data->regmap, DPS310_RESET, DPS310_RESET_MAGIC);

This could do with a comment.  I'm going to assume that a reset
puts the device into a low power state?

You could use a devm_add_action_or_reset call to remove the need for the
error path in probe and for the remove function to exist at all.

As it stands there is a race on remove as you reset the device before
removing the userspace and inkernel interfaces.  It is almost never
valid to mix devm_ calls with other calls (devm_add_action_or_reset
is the nicest solution to this by making anything a managed call).

> +}
> +
> +static const struct i2c_device_id dps310_id[] = {
> +	{ "dps310", 0 },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(i2c, dps310_id);
> +
> +static const unsigned short normal_i2c[] = {
> +	0x77, 0x76, I2C_CLIENT_END
> +};
> +
> +static struct i2c_driver dps310_driver = {
> +	.driver = {
> +		.name = "dps310",
> +	},
> +	.probe = dps310_probe,
> +	.remove = dps310_remove,
> +	.address_list = normal_i2c,
I'm fairly sure the address list is only used along with the detection
infrastructure.  As such it doesn't actually provide any value unless
you have a detect callback.  Please remove.

I would like to see a DT and/or ACPI binding though as that is the
means most people will use to find the device.

> +	.id_table = dps310_id,
> +};
> +module_i2c_driver(dps310_driver);
> +
> +MODULE_AUTHOR("Joel Stanley <joel@jms.id.au>");
> +MODULE_DESCRIPTION("Infineon DPS310 pressure and temperature sensor");
> +MODULE_LICENSE("GPL v2");

Re: [PATCH v2 1/3] iio: Add driver for Infineon DPS310

[Eddie James]

On 5/11/19 4:22 AM, Jonathan Cameron wrote:
> On Wed,  8 May 2019 14:35:26 -0500
> Eddie James <eajames@linux.ibm.com> wrote:
>
>> From: Joel Stanley <joel@jms.id.au>
>>
>> The DPS310 is a temperature and pressure sensor. It can be accessed over
>> i2c and SPI.
>>
>> Signed-off-by: Eddie James <eajames@linux.ibm.com>
> Hi Eddie,
>
> Ideally we'll get a sign off form Joel as well on this.
>
> A few comments inline.
>
> I 'think' this is probably fine without any locking to prevent simultaneous reads
> and /or writes to the registers because the few functions that do multiple reads
> and writes look fine.  Please do take another look at that though to confirm there
> are no corner cases.
>
> Otherwise there is a race in the remove path that needs fixing.
> Various minor bits and bobs inline.
>
> thanks,
>
> Jonathan
>
>
>
>> ---
>>   MAINTAINERS                   |   6 +
>>   drivers/iio/pressure/Kconfig  |  10 +
>>   drivers/iio/pressure/Makefile |   1 +
>>   drivers/iio/pressure/dps310.c | 429 ++++++++++++++++++++++++++++++++++++++++++
>>   4 files changed, 446 insertions(+)
>>   create mode 100644 drivers/iio/pressure/dps310.c
>>

>> +};
>> +MODULE_DEVICE_TABLE(i2c, dps310_id);
>> +
>> +static const unsigned short normal_i2c[] = {
>> +	0x77, 0x76, I2C_CLIENT_END
>> +};
>> +
>> +static struct i2c_driver dps310_driver = {
>> +	.driver = {
>> +		.name = "dps310",
>> +	},
>> +	.probe = dps310_probe,
>> +	.remove = dps310_remove,
>> +	.address_list = normal_i2c,
> I'm fairly sure the address list is only used along with the detection
> infrastructure.  As such it doesn't actually provide any value unless
> you have a detect callback.  Please remove.
>
> I would like to see a DT and/or ACPI binding though as that is the
> means most people will use to find the device.


Somehow the device is already present in the witherspoon device tree 
where it's currently being used, so I don't have anything to add. 
arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts

Thanks,

Eddie


>
>> +	.id_table = dps310_id,
>> +};
>> +module_i2c_driver(dps310_driver);
>> +
>> +MODULE_AUTHOR("Joel Stanley <joel@jms.id.au>");
>> +MODULE_DESCRIPTION("Infineon DPS310 pressure and temperature sensor");
>> +MODULE_LICENSE("GPL v2");

Re: [PATCH v2 1/3] iio: Add driver for Infineon DPS310

[Jonathan Cameron]

On Wed, 15 May 2019 13:46:46 -0500
Eddie James <eajames@linux.ibm.com> wrote:

> On 5/11/19 4:22 AM, Jonathan Cameron wrote:
> > On Wed,  8 May 2019 14:35:26 -0500
> > Eddie James <eajames@linux.ibm.com> wrote:
> >  
> >> From: Joel Stanley <joel@jms.id.au>
> >>
> >> The DPS310 is a temperature and pressure sensor. It can be accessed over
> >> i2c and SPI.
> >>
> >> Signed-off-by: Eddie James <eajames@linux.ibm.com>  
> > Hi Eddie,
> >
> > Ideally we'll get a sign off form Joel as well on this.
> >
> > A few comments inline.
> >
> > I 'think' this is probably fine without any locking to prevent simultaneous reads
> > and /or writes to the registers because the few functions that do multiple reads
> > and writes look fine.  Please do take another look at that though to confirm there
> > are no corner cases.
> >
> > Otherwise there is a race in the remove path that needs fixing.
> > Various minor bits and bobs inline.
> >
> > thanks,
> >
> > Jonathan
> >
> >
> >  
> >> ---
> >>   MAINTAINERS                   |   6 +
> >>   drivers/iio/pressure/Kconfig  |  10 +
> >>   drivers/iio/pressure/Makefile |   1 +
> >>   drivers/iio/pressure/dps310.c | 429 ++++++++++++++++++++++++++++++++++++++++++
> >>   4 files changed, 446 insertions(+)
> >>   create mode 100644 drivers/iio/pressure/dps310.c
> >>  
> 
> >> +};
> >> +MODULE_DEVICE_TABLE(i2c, dps310_id);
> >> +
> >> +static const unsigned short normal_i2c[] = {
> >> +	0x77, 0x76, I2C_CLIENT_END
> >> +};
> >> +
> >> +static struct i2c_driver dps310_driver = {
> >> +	.driver = {
> >> +		.name = "dps310",
> >> +	},
> >> +	.probe = dps310_probe,
> >> +	.remove = dps310_remove,
> >> +	.address_list = normal_i2c,  
> > I'm fairly sure the address list is only used along with the detection
> > infrastructure.  As such it doesn't actually provide any value unless
> > you have a detect callback.  Please remove.
> >
> > I would like to see a DT and/or ACPI binding though as that is the
> > means most people will use to find the device.  
> 
> 
> Somehow the device is already present in the witherspoon device tree 
> where it's currently being used, so I don't have anything to add. 
> arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts
Yes, there is a fallback path (which in theory will be removed one day)
in which the vendor is stripped off the dts entry and the rest matched
against the registered i2c device table.

However, it's less than ideal as it'll lead to a potential false match
if some other company uses dps310 as a part number.

Hence it is preferred to always provide and explicit dt binding which is
checked before this fallback path.

As a side note, this is a classic thing for people to pick up on and send
follow up patches for.  Makes everyone's life easier to do it early!

Thanks,

Jonathan

> 
> Thanks,
> 
> Eddie
> 
> 
> >  
> >> +	.id_table = dps310_id,
> >> +};
> >> +module_i2c_driver(dps310_driver);
> >> +
> >> +MODULE_AUTHOR("Joel Stanley <joel@jms.id.au>");
> >> +MODULE_DESCRIPTION("Infineon DPS310 pressure and temperature sensor");
> >> +MODULE_LICENSE("GPL v2");  
> 

[PATCH v2 2/3] iio: dps310: Temperature measurement errata

[Eddie James]

From: Christopher Bostic <cbostic@linux.vnet.ibm.com>

Add a manufacturer's suggested workaround to deal with early revisions
of chip that don't indicate correct temperature. Readings can be in the
~60C range when they should be in the ~20's.

Signed-off-by: Christopher Bostic <cbostic@linux.vnet.ibm.com>
Signed-off-by: Joel Stanley <joel@jms.id.au>
Signed-off-by: Eddie James <eajames@linux.ibm.com>
---
 drivers/iio/pressure/dps310.c | 51 ++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 50 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/pressure/dps310.c b/drivers/iio/pressure/dps310.c
index 7afaa88..c42808e 100644
--- a/drivers/iio/pressure/dps310.c
+++ b/drivers/iio/pressure/dps310.c
@@ -221,6 +221,9 @@ static bool dps310_is_writeable_reg(struct device *dev, unsigned int reg)
 	case DPS310_MEAS_CFG:
 	case DPS310_CFG_REG:
 	case DPS310_RESET:
+	case 0x0e:
+	case 0x0f:
+	case 0x62:
 		return true;
 	default:
 		return false;
@@ -237,6 +240,7 @@ static bool dps310_is_volatile_reg(struct device *dev, unsigned int reg)
 	case DPS310_TMP_B1:
 	case DPS310_TMP_B2:
 	case DPS310_MEAS_CFG:
+	case 0x32:
 		return true;
 	default:
 		return false;
@@ -314,7 +318,7 @@ static int dps310_read_raw(struct iio_dev *iio,
 	.writeable_reg = dps310_is_writeable_reg,
 	.volatile_reg = dps310_is_volatile_reg,
 	.cache_type = REGCACHE_RBTREE,
-	.max_register = 0x29,
+	.max_register = 0x62,
 };
 
 static const struct iio_info dps310_info = {
@@ -322,6 +326,47 @@ static int dps310_read_raw(struct iio_dev *iio,
 	.write_raw = dps310_write_raw,
 };
 
+/*
+ * Some verions of chip will read temperatures in the ~60C range when
+ * its actually ~20C. This is the manufacturer recommended workaround
+ * to correct the issue.
+ */
+static int dps310_temp_workaround(struct dps310_data *data)
+{
+	int r, reg;
+
+	r = regmap_read(data->regmap, 0x32, &reg);
+	if (r < 0)
+		return r;
+
+	/*
+	 * If bit 1 is set then the device is okay, and the workaround does not
+	 * need to be applied
+	 */
+	if (reg & BIT(1))
+		return 0;
+
+	r = regmap_write(data->regmap, 0x0e, 0xA5);
+	if (r < 0)
+		return r;
+
+	r = regmap_write(data->regmap, 0x0f, 0x96);
+	if (r < 0)
+		return r;
+
+	r = regmap_write(data->regmap, 0x62, 0x02);
+	if (r < 0)
+		return r;
+
+	r = regmap_write(data->regmap, 0x0e, 0x00);
+	if (r < 0)
+		return r;
+
+	r = regmap_write(data->regmap, 0x0f, 0x00);
+
+	return r;
+}
+
 static int dps310_probe(struct i2c_client *client,
 			const struct i2c_device_id *id)
 {
@@ -383,6 +428,10 @@ static int dps310_probe(struct i2c_client *client,
 	if (r < 0)
 		goto err;
 
+	r = dps310_temp_workaround(data);
+	if (r < 0)
+		return r;
+
 	r = devm_iio_device_register(&client->dev, iio);
 	if (r)
 		goto err;
-- 
1.8.3.1

Re: [PATCH v2 2/3] iio: dps310: Temperature measurement errata

[Matt Ranostay]

On Thu, May 9, 2019 at 3:36 AM Eddie James <eajames@linux.ibm.com> wrote:
>
> From: Christopher Bostic <cbostic@linux.vnet.ibm.com>
>
> Add a manufacturer's suggested workaround to deal with early revisions
> of chip that don't indicate correct temperature. Readings can be in the
> ~60C range when they should be in the ~20's.
>
> Signed-off-by: Christopher Bostic <cbostic@linux.vnet.ibm.com>
> Signed-off-by: Joel Stanley <joel@jms.id.au>
> Signed-off-by: Eddie James <eajames@linux.ibm.com>
> ---
>  drivers/iio/pressure/dps310.c | 51 ++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 50 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/iio/pressure/dps310.c b/drivers/iio/pressure/dps310.c
> index 7afaa88..c42808e 100644
> --- a/drivers/iio/pressure/dps310.c
> +++ b/drivers/iio/pressure/dps310.c
> @@ -221,6 +221,9 @@ static bool dps310_is_writeable_reg(struct device *dev, unsigned int reg)
>         case DPS310_MEAS_CFG:
>         case DPS310_CFG_REG:
>         case DPS310_RESET:
> +       case 0x0e:
> +       case 0x0f:
> +       case 0x62:

What is with the magic values? Are they not documented to what they
are, and hence not defining enum values for them?

- Matt

>                 return true;
>         default:
>                 return false;
> @@ -237,6 +240,7 @@ static bool dps310_is_volatile_reg(struct device *dev, unsigned int reg)
>         case DPS310_TMP_B1:
>         case DPS310_TMP_B2:
>         case DPS310_MEAS_CFG:
> +       case 0x32:
>                 return true;
>         default:
>                 return false;
> @@ -314,7 +318,7 @@ static int dps310_read_raw(struct iio_dev *iio,
>         .writeable_reg = dps310_is_writeable_reg,
>         .volatile_reg = dps310_is_volatile_reg,
>         .cache_type = REGCACHE_RBTREE,
> -       .max_register = 0x29,
> +       .max_register = 0x62,
>  };
>
>  static const struct iio_info dps310_info = {
> @@ -322,6 +326,47 @@ static int dps310_read_raw(struct iio_dev *iio,
>         .write_raw = dps310_write_raw,
>  };
>
> +/*
> + * Some verions of chip will read temperatures in the ~60C range when
> + * its actually ~20C. This is the manufacturer recommended workaround
> + * to correct the issue.
> + */
> +static int dps310_temp_workaround(struct dps310_data *data)
> +{
> +       int r, reg;
> +
> +       r = regmap_read(data->regmap, 0x32, &reg);
> +       if (r < 0)
> +               return r;
> +
> +       /*
> +        * If bit 1 is set then the device is okay, and the workaround does not
> +        * need to be applied
> +        */
> +       if (reg & BIT(1))
> +               return 0;
> +
> +       r = regmap_write(data->regmap, 0x0e, 0xA5);
> +       if (r < 0)
> +               return r;
> +
> +       r = regmap_write(data->regmap, 0x0f, 0x96);
> +       if (r < 0)
> +               return r;
> +
> +       r = regmap_write(data->regmap, 0x62, 0x02);
> +       if (r < 0)
> +               return r;
> +
> +       r = regmap_write(data->regmap, 0x0e, 0x00);
> +       if (r < 0)
> +               return r;
> +
> +       r = regmap_write(data->regmap, 0x0f, 0x00);
> +
> +       return r;
> +}
> +
>  static int dps310_probe(struct i2c_client *client,
>                         const struct i2c_device_id *id)
>  {
> @@ -383,6 +428,10 @@ static int dps310_probe(struct i2c_client *client,
>         if (r < 0)
>                 goto err;
>
> +       r = dps310_temp_workaround(data);
> +       if (r < 0)
> +               return r;
> +
>         r = devm_iio_device_register(&client->dev, iio);
>         if (r)
>                 goto err;
> --
> 1.8.3.1
>

Re: [PATCH v2 2/3] iio: dps310: Temperature measurement errata

[Eddie James]

On 5/8/19 10:09 PM, Matt Ranostay wrote:
> On Thu, May 9, 2019 at 3:36 AM Eddie James <eajames@linux.ibm.com> wrote:
>> From: Christopher Bostic <cbostic@linux.vnet.ibm.com>
>>
>> Add a manufacturer's suggested workaround to deal with early revisions
>> of chip that don't indicate correct temperature. Readings can be in the
>> ~60C range when they should be in the ~20's.
>>
>> Signed-off-by: Christopher Bostic <cbostic@linux.vnet.ibm.com>
>> Signed-off-by: Joel Stanley <joel@jms.id.au>
>> Signed-off-by: Eddie James <eajames@linux.ibm.com>
>> ---
>>   drivers/iio/pressure/dps310.c | 51 ++++++++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 50 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/iio/pressure/dps310.c b/drivers/iio/pressure/dps310.c
>> index 7afaa88..c42808e 100644
>> --- a/drivers/iio/pressure/dps310.c
>> +++ b/drivers/iio/pressure/dps310.c
>> @@ -221,6 +221,9 @@ static bool dps310_is_writeable_reg(struct device *dev, unsigned int reg)
>>          case DPS310_MEAS_CFG:
>>          case DPS310_CFG_REG:
>>          case DPS310_RESET:
>> +       case 0x0e:
>> +       case 0x0f:
>> +       case 0x62:
> What is with the magic values? Are they not documented to what they
> are, and hence not defining enum values for them?
>
> - Matt


Thats correct. These don't show up in the data sheet so I left them as 
raw values. Chris, do you know what the source for these values was?

Thanks,

Eddie


>
>>                  return true;
>>          default:
>>                  return false;
>> @@ -237,6 +240,7 @@ static bool dps310_is_volatile_reg(struct device *dev, unsigned int reg)
>>          case DPS310_TMP_B1:
>>          case DPS310_TMP_B2:
>>          case DPS310_MEAS_CFG:
>> +       case 0x32:
>>                  return true;
>>          default:
>>                  return false;
>> @@ -314,7 +318,7 @@ static int dps310_read_raw(struct iio_dev *iio,
>>          .writeable_reg = dps310_is_writeable_reg,
>>          .volatile_reg = dps310_is_volatile_reg,
>>          .cache_type = REGCACHE_RBTREE,
>> -       .max_register = 0x29,
>> +       .max_register = 0x62,
>>   };
>>
>>   static const struct iio_info dps310_info = {
>> @@ -322,6 +326,47 @@ static int dps310_read_raw(struct iio_dev *iio,
>>          .write_raw = dps310_write_raw,
>>   };
>>
>> +/*
>> + * Some verions of chip will read temperatures in the ~60C range when
>> + * its actually ~20C. This is the manufacturer recommended workaround
>> + * to correct the issue.
>> + */
>> +static int dps310_temp_workaround(struct dps310_data *data)
>> +{
>> +       int r, reg;
>> +
>> +       r = regmap_read(data->regmap, 0x32, &reg);
>> +       if (r < 0)
>> +               return r;
>> +
>> +       /*
>> +        * If bit 1 is set then the device is okay, and the workaround does not
>> +        * need to be applied
>> +        */
>> +       if (reg & BIT(1))
>> +               return 0;
>> +
>> +       r = regmap_write(data->regmap, 0x0e, 0xA5);
>> +       if (r < 0)
>> +               return r;
>> +
>> +       r = regmap_write(data->regmap, 0x0f, 0x96);
>> +       if (r < 0)
>> +               return r;
>> +
>> +       r = regmap_write(data->regmap, 0x62, 0x02);
>> +       if (r < 0)
>> +               return r;
>> +
>> +       r = regmap_write(data->regmap, 0x0e, 0x00);
>> +       if (r < 0)
>> +               return r;
>> +
>> +       r = regmap_write(data->regmap, 0x0f, 0x00);
>> +
>> +       return r;
>> +}
>> +
>>   static int dps310_probe(struct i2c_client *client,
>>                          const struct i2c_device_id *id)
>>   {
>> @@ -383,6 +428,10 @@ static int dps310_probe(struct i2c_client *client,
>>          if (r < 0)
>>                  goto err;
>>
>> +       r = dps310_temp_workaround(data);
>> +       if (r < 0)
>> +               return r;
>> +
>>          r = devm_iio_device_register(&client->dev, iio);
>>          if (r)
>>                  goto err;
>> --
>> 1.8.3.1
>>

Re: [PATCH v2 2/3] iio: dps310: Temperature measurement errata

[Matt Ranostay]

On Thu, May 9, 2019 at 11:17 PM Eddie James <eajames@linux.vnet.ibm.com> wrote:
>
>
> On 5/8/19 10:09 PM, Matt Ranostay wrote:
> > On Thu, May 9, 2019 at 3:36 AM Eddie James <eajames@linux.ibm.com> wrote:
> >> From: Christopher Bostic <cbostic@linux.vnet.ibm.com>
> >>
> >> Add a manufacturer's suggested workaround to deal with early revisions
> >> of chip that don't indicate correct temperature. Readings can be in the
> >> ~60C range when they should be in the ~20's.
> >>
> >> Signed-off-by: Christopher Bostic <cbostic@linux.vnet.ibm.com>
> >> Signed-off-by: Joel Stanley <joel@jms.id.au>
> >> Signed-off-by: Eddie James <eajames@linux.ibm.com>
> >> ---
> >>   drivers/iio/pressure/dps310.c | 51 ++++++++++++++++++++++++++++++++++++++++++-
> >>   1 file changed, 50 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/iio/pressure/dps310.c b/drivers/iio/pressure/dps310.c
> >> index 7afaa88..c42808e 100644
> >> --- a/drivers/iio/pressure/dps310.c
> >> +++ b/drivers/iio/pressure/dps310.c
> >> @@ -221,6 +221,9 @@ static bool dps310_is_writeable_reg(struct device *dev, unsigned int reg)
> >>          case DPS310_MEAS_CFG:
> >>          case DPS310_CFG_REG:
> >>          case DPS310_RESET:
> >> +       case 0x0e:
> >> +       case 0x0f:
> >> +       case 0x62:
> > What is with the magic values? Are they not documented to what they
> > are, and hence not defining enum values for them?
> >
> > - Matt
>
>
> Thats correct. These don't show up in the data sheet so I left them as
> raw values. Chris, do you know what the source for these values was?

Please at least make a comment in the code stating as much.

- Matt
>
> Thanks,
>
> Eddie
>
>
> >
> >>                  return true;
> >>          default:
> >>                  return false;
> >> @@ -237,6 +240,7 @@ static bool dps310_is_volatile_reg(struct device *dev, unsigned int reg)
> >>          case DPS310_TMP_B1:
> >>          case DPS310_TMP_B2:
> >>          case DPS310_MEAS_CFG:
> >> +       case 0x32:
> >>                  return true;
> >>          default:
> >>                  return false;
> >> @@ -314,7 +318,7 @@ static int dps310_read_raw(struct iio_dev *iio,
> >>          .writeable_reg = dps310_is_writeable_reg,
> >>          .volatile_reg = dps310_is_volatile_reg,
> >>          .cache_type = REGCACHE_RBTREE,
> >> -       .max_register = 0x29,
> >> +       .max_register = 0x62,
> >>   };
> >>
> >>   static const struct iio_info dps310_info = {
> >> @@ -322,6 +326,47 @@ static int dps310_read_raw(struct iio_dev *iio,
> >>          .write_raw = dps310_write_raw,
> >>   };
> >>
> >> +/*
> >> + * Some verions of chip will read temperatures in the ~60C range when
> >> + * its actually ~20C. This is the manufacturer recommended workaround
> >> + * to correct the issue.
> >> + */
> >> +static int dps310_temp_workaround(struct dps310_data *data)
> >> +{
> >> +       int r, reg;
> >> +
> >> +       r = regmap_read(data->regmap, 0x32, &reg);
> >> +       if (r < 0)
> >> +               return r;
> >> +
> >> +       /*
> >> +        * If bit 1 is set then the device is okay, and the workaround does not
> >> +        * need to be applied
> >> +        */
> >> +       if (reg & BIT(1))
> >> +               return 0;
> >> +
> >> +       r = regmap_write(data->regmap, 0x0e, 0xA5);
> >> +       if (r < 0)
> >> +               return r;
> >> +
> >> +       r = regmap_write(data->regmap, 0x0f, 0x96);
> >> +       if (r < 0)
> >> +               return r;
> >> +
> >> +       r = regmap_write(data->regmap, 0x62, 0x02);
> >> +       if (r < 0)
> >> +               return r;
> >> +
> >> +       r = regmap_write(data->regmap, 0x0e, 0x00);
> >> +       if (r < 0)
> >> +               return r;
> >> +
> >> +       r = regmap_write(data->regmap, 0x0f, 0x00);
> >> +
> >> +       return r;
> >> +}
> >> +
> >>   static int dps310_probe(struct i2c_client *client,
> >>                          const struct i2c_device_id *id)
> >>   {
> >> @@ -383,6 +428,10 @@ static int dps310_probe(struct i2c_client *client,
> >>          if (r < 0)
> >>                  goto err;
> >>
> >> +       r = dps310_temp_workaround(data);
> >> +       if (r < 0)
> >> +               return r;
> >> +
> >>          r = devm_iio_device_register(&client->dev, iio);
> >>          if (r)
> >>                  goto err;
> >> --
> >> 1.8.3.1
> >>
>

[PATCH v2 3/3] iio: dps310: Add pressure sensing capability

[Eddie James]

The DPS310 supports measurement of pressure, so support that in the
driver. Use background measurement like the temperature sensing and
default to lowest precision and lowest measurement rate.

Signed-off-by: Eddie James <eajames@linux.ibm.com>
---
 drivers/iio/pressure/dps310.c | 331 +++++++++++++++++++++++++++++++++++++++---
 1 file changed, 307 insertions(+), 24 deletions(-)

diff --git a/drivers/iio/pressure/dps310.c b/drivers/iio/pressure/dps310.c
index c42808e..a7ee28c 100644
--- a/drivers/iio/pressure/dps310.c
+++ b/drivers/iio/pressure/dps310.c
@@ -11,11 +11,11 @@
  *   c0 * 0.5 + c1 * T_raw / kT °C
  *
  * TODO:
- *  - Pressure sensor readings
  *  - Optionally support the FIFO
  */
 
 #include <linux/i2c.h>
+#include <linux/math64.h>
 #include <linux/module.h>
 #include <linux/regmap.h>
 
@@ -29,6 +29,8 @@
 #define DPS310_TMP_B1		0x04
 #define DPS310_TMP_B2		0x05
 #define DPS310_PRS_CFG		0x06
+#define  DPS310_PRS_RATE_BITS	GENMASK(6, 4)
+#define  DPS310_PRS_PRC_BITS	GENMASK(3, 0)
 #define DPS310_TMP_CFG		0x07
 #define  DPS310_TMP_RATE_BITS	GENMASK(6, 4)
 #define  DPS310_TMP_PRC_BITS	GENMASK(3, 0)
@@ -82,6 +84,8 @@ struct dps310_data {
 	struct regmap *regmap;
 
 	s32 c0, c1;
+	s32 c00, c10, c20, c30, c01, c11, c21;
+	s32 pressure_raw;
 	s32 temp_raw;
 };
 
@@ -94,33 +98,79 @@ struct dps310_data {
 			BIT(IIO_CHAN_INFO_SAMP_FREQ) |
 			BIT(IIO_CHAN_INFO_RAW),
 	},
+	{
+		.type = IIO_PRESSURE,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_SCALE) |
+			BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO) |
+			BIT(IIO_CHAN_INFO_SAMP_FREQ) |
+			BIT(IIO_CHAN_INFO_RAW),
+	},
 };
 
 /* To be called after checking the COEF_RDY bit in MEAS_CFG */
-static int dps310_get_temp_coef(struct dps310_data *data)
+static int dps310_get_coefs(struct dps310_data *data)
 {
 	struct regmap *regmap = data->regmap;
-	u8 coef[3];
 	int r;
+	u8 coef[18];
 	u32 c0, c1;
+	u32 c00, c10, c20, c30, c01, c11, c21;
 
-	/*
-	 * Read temperature calibration coefficients c0 and c1 from the
-	 * COEF register. The numbers are 12-bit 2's compliment numbers
-	 */
+	/* Read all sensor calibration coefficients from the COEF registers. */
 	r = regmap_bulk_read(regmap, DPS310_COEF_BASE, coef, sizeof(coef));
 	if (r < 0)
 		return r;
 
+	/*
+	 * Calculate temperature calibration coefficients c0 and c1. The numbers
+	 * are 12-bit 2's complement numbers.
+	 */
 	c0 = (coef[0] << 4) | (coef[1] >> 4);
 	data->c0 = sign_extend32(c0, 11);
 
 	c1 = ((coef[1] & GENMASK(3, 0)) << 8) | coef[2];
 	data->c1 = sign_extend32(c1, 11);
 
+	/*
+	 * Calculate pressure calibration coefficients. c00 and c10 are 20 bit
+	 * 2's complement numbers, while the rest are 16 bit 2's complement
+	 * numbers.
+	 */
+	c00 = (coef[3] << 12) | (coef[4] << 4) | (coef[5] >> 4);
+	data->c00 = sign_extend32(c00, 19);
+
+	c10 = ((coef[5] & GENMASK(3, 0)) << 16) | (coef[6] << 8) | coef[7];
+	data->c10 = sign_extend32(c10, 19);
+
+	c01 = (coef[8] << 8) | coef[9];
+	data->c01 = sign_extend32(c01, 15);
+
+	c11 = (coef[10] << 8) | coef[11];
+	data->c11 = sign_extend32(c11, 15);
+
+	c20 = (coef[12] << 8) | coef[13];
+	data->c20 = sign_extend32(c20, 15);
+
+	c21 = (coef[14] << 8) | coef[15];
+	data->c21 = sign_extend32(c21, 15);
+
+	c30 = (coef[16] << 8) | coef[17];
+	data->c30 = sign_extend32(c30, 15);
+
 	return 0;
 }
 
+static int dps310_get_pres_precision(struct dps310_data *data)
+{
+	int val, r;
+
+	r = regmap_read(data->regmap, DPS310_PRS_CFG, &val);
+	if (r < 0)
+		return r;
+
+	return BIT(val & GENMASK(2, 0));
+}
+
 static int dps310_get_temp_precision(struct dps310_data *data)
 {
 	int val, r;
@@ -136,6 +186,24 @@ static int dps310_get_temp_precision(struct dps310_data *data)
 	return BIT(val & GENMASK(2, 0));
 }
 
+static int dps310_set_pres_precision(struct dps310_data *data, int val)
+{
+	int ret;
+	u8 shift_en;
+
+	if (val < 0 || val > 128)
+		return -EINVAL;
+
+	shift_en = val >= 16 ? DPS310_PRS_SHIFT_EN : 0;
+	ret = regmap_write_bits(data->regmap, DPS310_CFG_REG,
+				DPS310_PRS_SHIFT_EN, shift_en);
+	if (ret)
+		return ret;
+
+	return regmap_update_bits(data->regmap, DPS310_PRS_CFG,
+				  DPS310_PRS_PRC_BITS, DPS310_CALC_PRC(val));
+}
+
 static int dps310_set_temp_precision(struct dps310_data *data, int val)
 {
 	int ret;
@@ -154,6 +222,19 @@ static int dps310_set_temp_precision(struct dps310_data *data, int val)
 				  DPS310_TMP_PRC_BITS, DPS310_CALC_PRC(val));
 }
 
+static int dps310_set_pres_samp_freq(struct dps310_data *data, int freq)
+{
+	u8 val;
+
+	if (freq < 0 || freq > 128)
+		return -EINVAL;
+
+	val = DPS310_CALC_RATE(freq) << 4;
+
+	return regmap_update_bits(data->regmap, DPS310_PRS_CFG,
+				  DPS310_PRS_RATE_BITS, val);
+}
+
 static int dps310_set_temp_samp_freq(struct dps310_data *data, int freq)
 {
 	u8 val;
@@ -167,6 +248,17 @@ static int dps310_set_temp_samp_freq(struct dps310_data *data, int freq)
 				  DPS310_TMP_RATE_BITS, val);
 }
 
+static int dps310_get_pres_samp_freq(struct dps310_data *data)
+{
+	int val, r;
+
+	r = regmap_read(data->regmap, DPS310_PRS_CFG, &val);
+	if (r < 0)
+		return r;
+
+	return BIT((val & DPS310_PRS_RATE_BITS) >> 4);
+}
+
 static int dps310_get_temp_samp_freq(struct dps310_data *data)
 {
 	int val, r;
@@ -178,6 +270,16 @@ static int dps310_get_temp_samp_freq(struct dps310_data *data)
 	return BIT((val & DPS310_TMP_RATE_BITS) >> 4);
 }
 
+static int dps310_get_pres_k(struct dps310_data *data)
+{
+	int r = dps310_get_pres_precision(data);
+
+	if (r < 0)
+		return r;
+
+	return scale_factors[DPS310_CALC_PRC(r)];
+}
+
 static int dps310_get_temp_k(struct dps310_data *data)
 {
 	int r = dps310_get_temp_precision(data);
@@ -188,21 +290,37 @@ static int dps310_get_temp_k(struct dps310_data *data)
 	return scale_factors[DPS310_CALC_PRC(r)];
 }
 
-static int dps310_read_temp(struct dps310_data *data)
+static int dps310_read_pres_raw(struct dps310_data *data)
 {
+	int r, ready;
 	u8 val[3];
 	s32 raw;
-	int r, ready;
-	int rate = dps310_get_temp_samp_freq(data);
+	int rate = dps310_get_pres_samp_freq(data);
 	int timeout = DPS310_POLL_TIMEOUT_US(rate);
 
 	/* Poll for sensor readiness; base the timeout upon the sample rate. */
 	r = regmap_read_poll_timeout(data->regmap, DPS310_MEAS_CFG, ready,
-				     ready & DPS310_TMP_RDY,
+				     ready & DPS310_PRS_RDY,
 				     DPS310_POLL_SLEEP_US(timeout), timeout);
+	if (r)
+		return r;
+
+	r = regmap_bulk_read(data->regmap, DPS310_PRS_BASE, val, sizeof(val));
 	if (r < 0)
 		return r;
 
+	raw = (val[0] << 16) | (val[1] << 8) | val[2];
+	data->pressure_raw = sign_extend32(raw, 23);
+
+	return 0;
+}
+
+static int dps310_read_temp_ready(struct dps310_data *data)
+{
+	int r;
+	u8 val[3];
+	s32 raw;
+
 	r = regmap_bulk_read(data->regmap, DPS310_TMP_BASE, val, sizeof(val));
 	if (r < 0)
 		return r;
@@ -213,6 +331,22 @@ static int dps310_read_temp(struct dps310_data *data)
 	return 0;
 }
 
+static int dps310_read_temp_raw(struct dps310_data *data)
+{
+	int r, ready;
+	int rate = dps310_get_temp_samp_freq(data);
+	int timeout = DPS310_POLL_TIMEOUT_US(rate);
+
+	/* Poll for sensor readiness; base the timeout upon the sample rate. */
+	r = regmap_read_poll_timeout(data->regmap, DPS310_MEAS_CFG, ready,
+				     ready & DPS310_TMP_RDY,
+				     DPS310_POLL_SLEEP_US(timeout), timeout);
+	if (r < 0)
+		return r;
+
+	return dps310_read_temp_ready(data);
+}
+
 static bool dps310_is_writeable_reg(struct device *dev, unsigned int reg)
 {
 	switch (reg) {
@@ -253,24 +387,141 @@ static int dps310_write_raw(struct iio_dev *iio,
 {
 	struct dps310_data *data = iio_priv(iio);
 
-	if (chan->type != IIO_TEMP)
+	switch (mask) {
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		switch (chan->type) {
+		case IIO_PRESSURE:
+			return dps310_set_pres_samp_freq(data, val);
+
+		case IIO_TEMP:
+			return dps310_set_temp_samp_freq(data, val);
+
+		default:
+			return -EINVAL;
+		}
+
+	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
+		switch (chan->type) {
+		case IIO_PRESSURE:
+			return dps310_set_pres_precision(data, val);
+
+		case IIO_TEMP:
+			return dps310_set_temp_precision(data, val);
+
+		default:
+			return -EINVAL;
+		}
+
+	default:
 		return -EINVAL;
+	}
+}
+
+static int dps310_calculate_pressure(struct dps310_data *data)
+{
+	int i, r, t_ready;
+	int kpi = dps310_get_pres_k(data);
+	int kti = dps310_get_temp_k(data);
+	s64 rem = 0ULL;
+	s64 pressure = 0ULL;
+	s64 p;
+	s64 t;
+	s64 denoms[7];
+	s64 nums[7];
+	s64 rems[7];
+	s64 kp;
+	s64 kt;
+
+	if (kpi < 0)
+		return kpi;
+
+	if (kti < 0)
+		return kti;
+
+	kp = (s64)kpi;
+	kt = (s64)kti;
+
+	/* Refresh temp if it's ready, otherwise just use the latest value */
+	r = regmap_read(data->regmap, DPS310_MEAS_CFG, &t_ready);
+	if (r >= 0 && t_ready & DPS310_TMP_RDY)
+		dps310_read_temp_ready(data);
+
+	p = (s64)data->pressure_raw;
+	t = (s64)data->temp_raw;
+
+	/* Section 4.9.1 of the DPS310 spec; algebra'd to avoid underflow */
+	nums[0] = (s64)data->c00;
+	denoms[0] = 1LL;
+	nums[1] = p * (s64)data->c10;
+	denoms[1] = kp;
+	nums[2] = p * p * (s64)data->c20;
+	denoms[2] = kp * kp;
+	nums[3] = p * p * p * (s64)data->c30;
+	denoms[3] = kp * kp * kp;
+	nums[4] = t * (s64)data->c01;
+	denoms[4] = kt;
+	nums[5] = t * p * (s64)data->c11;
+	denoms[5] = kp * kt;
+	nums[6] = t * p * p * (s64)data->c21;
+	denoms[6] = kp * kp * kt;
+
+	/* Kernel lacks a div64_s64_rem function; denoms are all positive */
+	for (i = 0; i < 7; ++i) {
+		u64 rem;
+
+		if (nums[i] < 0LL) {
+			pressure -= div64_u64_rem(-nums[i], denoms[i], &rem);
+			rems[i] = -rem;
+		} else {
+			pressure += div64_u64_rem(nums[i], denoms[i], &rem);
+			rems[i] = (s64)rem;
+		}
+	}
+
+	/* Increase precision and calculate the remainder sum */
+	for (i = 0; i < 7; ++i)
+		rem += div64_s64((s64)rems[i] * 1000000000LL, denoms[i]);
+
+	pressure += div_s64(rem, 1000000000LL);
+
+	return (int)pressure;
+}
+
+static int dps310_read_pressure(struct dps310_data *data, int *val, int *val2,
+				long mask)
+{
+	int ret;
 
 	switch (mask) {
 	case IIO_CHAN_INFO_SAMP_FREQ:
-		return dps310_set_temp_samp_freq(data, val);
+		*val = dps310_get_pres_samp_freq(data);
+		return IIO_VAL_INT;
+
+	case IIO_CHAN_INFO_RAW:
+		ret = dps310_read_pres_raw(data);
+		if (ret)
+			return ret;
+
+		*val = dps310_calculate_pressure(data);
+		return IIO_VAL_INT;
+
+	case IIO_CHAN_INFO_SCALE:
+		*val = 1;
+		*val2 = 1000; /* Convert Pa to KPa per IIO ABI */
+		return IIO_VAL_FRACTIONAL;
+
 	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
-		return dps310_set_temp_precision(data, val);
+		*val = dps310_get_pres_precision(data);
+		return IIO_VAL_INT;
+
 	default:
 		return -EINVAL;
 	}
 }
 
-static int dps310_read_raw(struct iio_dev *iio,
-			   struct iio_chan_spec const *chan,
-			   int *val, int *val2, long mask)
+static int dps310_read_temp(struct dps310_data *data, int *val, int *val2,
+			    long mask)
 {
-	struct dps310_data *data = iio_priv(iio);
 	int ret;
 
 	switch (mask) {
@@ -279,7 +530,7 @@ static int dps310_read_raw(struct iio_dev *iio,
 		return IIO_VAL_INT;
 
 	case IIO_CHAN_INFO_RAW:
-		ret = dps310_read_temp(data);
+		ret = dps310_read_temp_raw(data);
 		if (ret)
 			return ret;
 
@@ -312,6 +563,24 @@ static int dps310_read_raw(struct iio_dev *iio,
 	}
 }
 
+static int dps310_read_raw(struct iio_dev *iio,
+			   struct iio_chan_spec const *chan,
+			   int *val, int *val2, long mask)
+{
+	struct dps310_data *data = iio_priv(iio);
+
+	switch (chan->type) {
+	case IIO_PRESSURE:
+		return dps310_read_pressure(data, val, val2, mask);
+
+	case IIO_TEMP:
+		return dps310_read_temp(data, val, val2, mask);
+
+	default:
+		return -EINVAL;
+	}
+}
+
 static const struct regmap_config dps310_regmap_config = {
 	.reg_bits = 8,
 	.val_bits = 8,
@@ -393,6 +662,13 @@ static int dps310_probe(struct i2c_client *client,
 		return PTR_ERR(data->regmap);
 
 	/*
+	 * Set up pressure sensor in single sample, one measurement per second
+	 * mode
+	 */
+	r = regmap_write(data->regmap, DPS310_PRS_CFG,
+			 DPS310_CALC_RATE(1) | DPS310_CALC_PRC(1));
+
+	/*
 	 * Set up external (MEMS) temperature sensor in single sample, one
 	 * measurement per second mode
 	 */
@@ -402,16 +678,23 @@ static int dps310_probe(struct i2c_client *client,
 	if (r < 0)
 		goto err;
 
-	/* Temp shift is disabled when PRC <= 8 */
+	/* Temp and pressure shifts are disabled when PRC <= 8 */
 	r = regmap_write_bits(data->regmap, DPS310_CFG_REG,
-			      DPS310_TMP_SHIFT_EN, 0);
+			      DPS310_TMP_SHIFT_EN | DPS310_PRS_SHIFT_EN, 0);
+	if (r < 0)
+		goto err;
+
+	/* MEAS_CFG doesn't seem to update unless first written with 0 */
+	r = regmap_write_bits(data->regmap, DPS310_MEAS_CFG,
+			      DPS310_MEAS_CTRL_BITS, 0);
 	if (r < 0)
 		goto err;
 
-	/* Turn on temperature measurement in the background */
+	/* Turn on temperature and pressure measurement in the background */
 	r = regmap_write_bits(data->regmap, DPS310_MEAS_CFG,
 			      DPS310_MEAS_CTRL_BITS,
-			      DPS310_TEMP_EN | DPS310_BACKGROUND);
+			      DPS310_PRS_EN | DPS310_TEMP_EN |
+			      DPS310_BACKGROUND);
 	if (r < 0)
 		goto err;
 
@@ -424,7 +707,7 @@ static int dps310_probe(struct i2c_client *client,
 	if (r < 0)
 		goto err;
 
-	r = dps310_get_temp_coef(data);
+	r = dps310_get_coefs(data);
 	if (r < 0)
 		goto err;
 
-- 
1.8.3.1

Re: [PATCH v2 3/3] iio: dps310: Add pressure sensing capability

[Jonathan Cameron]

On Wed,  8 May 2019 14:35:28 -0500
Eddie James <eajames@linux.ibm.com> wrote:

> The DPS310 supports measurement of pressure, so support that in the
> driver. Use background measurement like the temperature sensing and
> default to lowest precision and lowest measurement rate.
> 
> Signed-off-by: Eddie James <eajames@linux.ibm.com>
Hi Eddie,

A few comments inline.  One is around how you might look at adding
fifo support (pushing to an IIO buffer) in the future...  The IIO 
data model isn't as flexible as this device can be, so we may need
to put some restrictions on combinations of options.

Jonathan
> ---
>  drivers/iio/pressure/dps310.c | 331 +++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 307 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/iio/pressure/dps310.c b/drivers/iio/pressure/dps310.c
> index c42808e..a7ee28c 100644
> --- a/drivers/iio/pressure/dps310.c
> +++ b/drivers/iio/pressure/dps310.c
> @@ -11,11 +11,11 @@
>   *   c0 * 0.5 + c1 * T_raw / kT °C
>   *
>   * TODO:
> - *  - Pressure sensor readings
>   *  - Optionally support the FIFO
>   */
>  
>  #include <linux/i2c.h>
> +#include <linux/math64.h>
>  #include <linux/module.h>
>  #include <linux/regmap.h>
>  
> @@ -29,6 +29,8 @@
>  #define DPS310_TMP_B1		0x04
>  #define DPS310_TMP_B2		0x05
>  #define DPS310_PRS_CFG		0x06
> +#define  DPS310_PRS_RATE_BITS	GENMASK(6, 4)
> +#define  DPS310_PRS_PRC_BITS	GENMASK(3, 0)
>  #define DPS310_TMP_CFG		0x07
>  #define  DPS310_TMP_RATE_BITS	GENMASK(6, 4)
>  #define  DPS310_TMP_PRC_BITS	GENMASK(3, 0)
> @@ -82,6 +84,8 @@ struct dps310_data {
>  	struct regmap *regmap;
>  
>  	s32 c0, c1;
> +	s32 c00, c10, c20, c30, c01, c11, c21;
> +	s32 pressure_raw;
>  	s32 temp_raw;
>  };
>  
> @@ -94,33 +98,79 @@ struct dps310_data {
>  			BIT(IIO_CHAN_INFO_SAMP_FREQ) |
>  			BIT(IIO_CHAN_INFO_RAW),
>  	},
> +	{
> +		.type = IIO_PRESSURE,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_SCALE) |
> +			BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO) |
> +			BIT(IIO_CHAN_INFO_SAMP_FREQ) |
> +			BIT(IIO_CHAN_INFO_RAW),
> +	},
>  };
>  
>  /* To be called after checking the COEF_RDY bit in MEAS_CFG */
> -static int dps310_get_temp_coef(struct dps310_data *data)
> +static int dps310_get_coefs(struct dps310_data *data)
>  {
>  	struct regmap *regmap = data->regmap;
> -	u8 coef[3];
>  	int r;
> +	u8 coef[18];
>  	u32 c0, c1;
> +	u32 c00, c10, c20, c30, c01, c11, c21;
>  
> -	/*
> -	 * Read temperature calibration coefficients c0 and c1 from the
> -	 * COEF register. The numbers are 12-bit 2's compliment numbers
> -	 */
> +	/* Read all sensor calibration coefficients from the COEF registers. */
>  	r = regmap_bulk_read(regmap, DPS310_COEF_BASE, coef, sizeof(coef));
>  	if (r < 0)
>  		return r;
>  
> +	/*
> +	 * Calculate temperature calibration coefficients c0 and c1. The numbers
> +	 * are 12-bit 2's complement numbers.
> +	 */
>  	c0 = (coef[0] << 4) | (coef[1] >> 4);
>  	data->c0 = sign_extend32(c0, 11);
>  
>  	c1 = ((coef[1] & GENMASK(3, 0)) << 8) | coef[2];
>  	data->c1 = sign_extend32(c1, 11);
>  
> +	/*
> +	 * Calculate pressure calibration coefficients. c00 and c10 are 20 bit
> +	 * 2's complement numbers, while the rest are 16 bit 2's complement
> +	 * numbers.
> +	 */
> +	c00 = (coef[3] << 12) | (coef[4] << 4) | (coef[5] >> 4);
> +	data->c00 = sign_extend32(c00, 19);
> +
> +	c10 = ((coef[5] & GENMASK(3, 0)) << 16) | (coef[6] << 8) | coef[7];
> +	data->c10 = sign_extend32(c10, 19);
> +
> +	c01 = (coef[8] << 8) | coef[9];
> +	data->c01 = sign_extend32(c01, 15);
> +
> +	c11 = (coef[10] << 8) | coef[11];
> +	data->c11 = sign_extend32(c11, 15);
> +
> +	c20 = (coef[12] << 8) | coef[13];
> +	data->c20 = sign_extend32(c20, 15);
> +
> +	c21 = (coef[14] << 8) | coef[15];
> +	data->c21 = sign_extend32(c21, 15);
> +
> +	c30 = (coef[16] << 8) | coef[17];
> +	data->c30 = sign_extend32(c30, 15);
> +
>  	return 0;
>  }
>  
> +static int dps310_get_pres_precision(struct dps310_data *data)
> +{
> +	int val, r;
> +
> +	r = regmap_read(data->regmap, DPS310_PRS_CFG, &val);
> +	if (r < 0)
> +		return r;
> +
> +	return BIT(val & GENMASK(2, 0));
> +}
> +
>  static int dps310_get_temp_precision(struct dps310_data *data)
>  {
>  	int val, r;
> @@ -136,6 +186,24 @@ static int dps310_get_temp_precision(struct dps310_data *data)
>  	return BIT(val & GENMASK(2, 0));
>  }
>  
> +static int dps310_set_pres_precision(struct dps310_data *data, int val)
> +{
> +	int ret;
> +	u8 shift_en;
> +
> +	if (val < 0 || val > 128)
> +		return -EINVAL;
> +
> +	shift_en = val >= 16 ? DPS310_PRS_SHIFT_EN : 0;
> +	ret = regmap_write_bits(data->regmap, DPS310_CFG_REG,
> +				DPS310_PRS_SHIFT_EN, shift_en);
> +	if (ret)
> +		return ret;
> +
> +	return regmap_update_bits(data->regmap, DPS310_PRS_CFG,
> +				  DPS310_PRS_PRC_BITS, DPS310_CALC_PRC(val));
> +}
> +
>  static int dps310_set_temp_precision(struct dps310_data *data, int val)
>  {
>  	int ret;
> @@ -154,6 +222,19 @@ static int dps310_set_temp_precision(struct dps310_data *data, int val)
>  				  DPS310_TMP_PRC_BITS, DPS310_CALC_PRC(val));
>  }
>  
> +static int dps310_set_pres_samp_freq(struct dps310_data *data, int freq)
> +{
> +	u8 val;
> +
> +	if (freq < 0 || freq > 128)
> +		return -EINVAL;
> +
> +	val = DPS310_CALC_RATE(freq) << 4;
> +
> +	return regmap_update_bits(data->regmap, DPS310_PRS_CFG,
> +				  DPS310_PRS_RATE_BITS, val);
> +}
> +
>  static int dps310_set_temp_samp_freq(struct dps310_data *data, int freq)
>  {
>  	u8 val;
> @@ -167,6 +248,17 @@ static int dps310_set_temp_samp_freq(struct dps310_data *data, int freq)
>  				  DPS310_TMP_RATE_BITS, val);
>  }
>  
> +static int dps310_get_pres_samp_freq(struct dps310_data *data)
> +{
> +	int val, r;
> +
> +	r = regmap_read(data->regmap, DPS310_PRS_CFG, &val);
> +	if (r < 0)
> +		return r;
> +
> +	return BIT((val & DPS310_PRS_RATE_BITS) >> 4);
> +}
> +
>  static int dps310_get_temp_samp_freq(struct dps310_data *data)
>  {
>  	int val, r;
> @@ -178,6 +270,16 @@ static int dps310_get_temp_samp_freq(struct dps310_data *data)
>  	return BIT((val & DPS310_TMP_RATE_BITS) >> 4);
>  }
>  
> +static int dps310_get_pres_k(struct dps310_data *data)
> +{
> +	int r = dps310_get_pres_precision(data);
> +
> +	if (r < 0)
> +		return r;
> +
> +	return scale_factors[DPS310_CALC_PRC(r)];
> +}
> +
>  static int dps310_get_temp_k(struct dps310_data *data)
>  {
>  	int r = dps310_get_temp_precision(data);
> @@ -188,21 +290,37 @@ static int dps310_get_temp_k(struct dps310_data *data)
>  	return scale_factors[DPS310_CALC_PRC(r)];
>  }
>  
> -static int dps310_read_temp(struct dps310_data *data)
> +static int dps310_read_pres_raw(struct dps310_data *data)
>  {
> +	int r, ready;
>  	u8 val[3];
>  	s32 raw;
> -	int r, ready;
> -	int rate = dps310_get_temp_samp_freq(data);
> +	int rate = dps310_get_pres_samp_freq(data);
>  	int timeout = DPS310_POLL_TIMEOUT_US(rate);
>  
>  	/* Poll for sensor readiness; base the timeout upon the sample rate. */
>  	r = regmap_read_poll_timeout(data->regmap, DPS310_MEAS_CFG, ready,
> -				     ready & DPS310_TMP_RDY,
> +				     ready & DPS310_PRS_RDY,
>  				     DPS310_POLL_SLEEP_US(timeout), timeout);
> +	if (r)
> +		return r;
> +
> +	r = regmap_bulk_read(data->regmap, DPS310_PRS_BASE, val, sizeof(val));
>  	if (r < 0)
>  		return r;
>  
> +	raw = (val[0] << 16) | (val[1] << 8) | val[2];
> +	data->pressure_raw = sign_extend32(raw, 23);
> +
> +	return 0;
> +}
> +
> +static int dps310_read_temp_ready(struct dps310_data *data)
> +{
> +	int r;
> +	u8 val[3];
> +	s32 raw;
> +
>  	r = regmap_bulk_read(data->regmap, DPS310_TMP_BASE, val, sizeof(val));
>  	if (r < 0)
>  		return r;
> @@ -213,6 +331,22 @@ static int dps310_read_temp(struct dps310_data *data)
>  	return 0;
>  }
>  
> +static int dps310_read_temp_raw(struct dps310_data *data)
> +{
> +	int r, ready;
> +	int rate = dps310_get_temp_samp_freq(data);
> +	int timeout = DPS310_POLL_TIMEOUT_US(rate);
> +
> +	/* Poll for sensor readiness; base the timeout upon the sample rate. */
> +	r = regmap_read_poll_timeout(data->regmap, DPS310_MEAS_CFG, ready,
> +				     ready & DPS310_TMP_RDY,
> +				     DPS310_POLL_SLEEP_US(timeout), timeout);
> +	if (r < 0)
> +		return r;
> +
> +	return dps310_read_temp_ready(data);
> +}
> +
>  static bool dps310_is_writeable_reg(struct device *dev, unsigned int reg)
>  {
>  	switch (reg) {
> @@ -253,24 +387,141 @@ static int dps310_write_raw(struct iio_dev *iio,
>  {
>  	struct dps310_data *data = iio_priv(iio);
>  
> -	if (chan->type != IIO_TEMP)
> +	switch (mask) {
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		switch (chan->type) {
> +		case IIO_PRESSURE:
> +			return dps310_set_pres_samp_freq(data, val);
> +
> +		case IIO_TEMP:
> +			return dps310_set_temp_samp_freq(data, val);

This may need a bit of thought if there is any chance we will later support
the fifo.

The IIO model is that of scans that read all channels at each 'trigger'. In
devices like this which allow for different sampling rates for different sensor
channels there are two options.

1) Don't support it.
2) Deal with registering two separate IIO devices and do the demux in the
driver to the relevant one.

All depends on whether there is a substantial usecase for different sampling
rates or not.  Here I suspect the answer is not.

The complexity is that, you then need to work out how to 'upgrade' the
interface when buffered support is added. Obvious options are:

1) Refuse to move to buffered mode if the sampling frequencies are different.
2) Force the slower channel to be sampled faster if that is possible.
3) Change to only having one exposed sampling frequency at all - the problem
with this last one is that it changes the ABI for existing users.

It may be no one ever cares about the fifo mode though as high speed pressure
measurement is 'unusual' ;)

> +
> +		default:
> +			return -EINVAL;
> +		}
> +
> +	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> +		switch (chan->type) {
> +		case IIO_PRESSURE:
> +			return dps310_set_pres_precision(data, val);
> +
> +		case IIO_TEMP:
> +			return dps310_set_temp_precision(data, val);
> +
> +		default:
> +			return -EINVAL;
> +		}
> +
> +	default:
>  		return -EINVAL;
> +	}
> +}
> +
> +static int dps310_calculate_pressure(struct dps310_data *data)
> +{
> +	int i, r, t_ready;
> +	int kpi = dps310_get_pres_k(data);
> +	int kti = dps310_get_temp_k(data);
> +	s64 rem = 0ULL;
> +	s64 pressure = 0ULL;
> +	s64 p;
> +	s64 t;
> +	s64 denoms[7];
> +	s64 nums[7];
> +	s64 rems[7];
> +	s64 kp;
> +	s64 kt;
> +
> +	if (kpi < 0)
> +		return kpi;
> +
> +	if (kti < 0)
> +		return kti;
> +
> +	kp = (s64)kpi;
> +	kt = (s64)kti;
> +
> +	/* Refresh temp if it's ready, otherwise just use the latest value */
> +	r = regmap_read(data->regmap, DPS310_MEAS_CFG, &t_ready);
> +	if (r >= 0 && t_ready & DPS310_TMP_RDY)
> +		dps310_read_temp_ready(data);
> +
> +	p = (s64)data->pressure_raw;
> +	t = (s64)data->temp_raw;
> +
> +	/* Section 4.9.1 of the DPS310 spec; algebra'd to avoid underflow */
> +	nums[0] = (s64)data->c00;
> +	denoms[0] = 1LL;
> +	nums[1] = p * (s64)data->c10;
> +	denoms[1] = kp;
> +	nums[2] = p * p * (s64)data->c20;
> +	denoms[2] = kp * kp;
> +	nums[3] = p * p * p * (s64)data->c30;
> +	denoms[3] = kp * kp * kp;
> +	nums[4] = t * (s64)data->c01;
> +	denoms[4] = kt;
> +	nums[5] = t * p * (s64)data->c11;
> +	denoms[5] = kp * kt;
> +	nums[6] = t * p * p * (s64)data->c21;
> +	denoms[6] = kp * kp * kt;
> +
> +	/* Kernel lacks a div64_s64_rem function; denoms are all positive */
> +	for (i = 0; i < 7; ++i) {
> +		u64 rem;
> +
> +		if (nums[i] < 0LL) {
> +			pressure -= div64_u64_rem(-nums[i], denoms[i], &rem);
> +			rems[i] = -rem;
> +		} else {
> +			pressure += div64_u64_rem(nums[i], denoms[i], &rem);
> +			rems[i] = (s64)rem;
> +		}
> +	}
> +
> +	/* Increase precision and calculate the remainder sum */
> +	for (i = 0; i < 7; ++i)
> +		rem += div64_s64((s64)rems[i] * 1000000000LL, denoms[i]);
> +
> +	pressure += div_s64(rem, 1000000000LL);
> +
> +	return (int)pressure;
> +}
> +
> +static int dps310_read_pressure(struct dps310_data *data, int *val, int *val2,
> +				long mask)
> +{
> +	int ret;
>  
>  	switch (mask) {
>  	case IIO_CHAN_INFO_SAMP_FREQ:
> -		return dps310_set_temp_samp_freq(data, val);
> +		*val = dps310_get_pres_samp_freq(data);
> +		return IIO_VAL_INT;
> +
> +	case IIO_CHAN_INFO_RAW:
> +		ret = dps310_read_pres_raw(data);
> +		if (ret)
> +			return ret;
> +
> +		*val = dps310_calculate_pressure(data);
This is rather far from raw :)  It might be better at this point to just
go for PROCESSED and apply the scale in here.

> +		return IIO_VAL_INT;
> +
> +	case IIO_CHAN_INFO_SCALE:
> +		*val = 1;
> +		*val2 = 1000; /* Convert Pa to KPa per IIO ABI */
> +		return IIO_VAL_FRACTIONAL;
> +
>  	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> -		return dps310_set_temp_precision(data, val);
> +		*val = dps310_get_pres_precision(data);
> +		return IIO_VAL_INT;
> +
>  	default:
>  		return -EINVAL;
>  	}
>  }
>  
> -static int dps310_read_raw(struct iio_dev *iio,
> -			   struct iio_chan_spec const *chan,
> -			   int *val, int *val2, long mask)
> +static int dps310_read_temp(struct dps310_data *data, int *val, int *val2,
> +			    long mask)
>  {
> -	struct dps310_data *data = iio_priv(iio);
>  	int ret;
>  
>  	switch (mask) {
> @@ -279,7 +530,7 @@ static int dps310_read_raw(struct iio_dev *iio,
>  		return IIO_VAL_INT;
>  
>  	case IIO_CHAN_INFO_RAW:
> -		ret = dps310_read_temp(data);
> +		ret = dps310_read_temp_raw(data);
>  		if (ret)
>  			return ret;
>  
> @@ -312,6 +563,24 @@ static int dps310_read_raw(struct iio_dev *iio,
>  	}
>  }
>  
> +static int dps310_read_raw(struct iio_dev *iio,
> +			   struct iio_chan_spec const *chan,
> +			   int *val, int *val2, long mask)
> +{
> +	struct dps310_data *data = iio_priv(iio);
> +
> +	switch (chan->type) {
> +	case IIO_PRESSURE:
> +		return dps310_read_pressure(data, val, val2, mask);
> +
> +	case IIO_TEMP:
> +		return dps310_read_temp(data, val, val2, mask);
> +
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
>  static const struct regmap_config dps310_regmap_config = {
>  	.reg_bits = 8,
>  	.val_bits = 8,
> @@ -393,6 +662,13 @@ static int dps310_probe(struct i2c_client *client,
>  		return PTR_ERR(data->regmap);
>  
>  	/*
> +	 * Set up pressure sensor in single sample, one measurement per second
> +	 * mode
> +	 */
> +	r = regmap_write(data->regmap, DPS310_PRS_CFG,
> +			 DPS310_CALC_RATE(1) | DPS310_CALC_PRC(1));
> +
> +	/*
>  	 * Set up external (MEMS) temperature sensor in single sample, one
>  	 * measurement per second mode
>  	 */
> @@ -402,16 +678,23 @@ static int dps310_probe(struct i2c_client *client,
>  	if (r < 0)
>  		goto err;
>  
> -	/* Temp shift is disabled when PRC <= 8 */
> +	/* Temp and pressure shifts are disabled when PRC <= 8 */
>  	r = regmap_write_bits(data->regmap, DPS310_CFG_REG,
> -			      DPS310_TMP_SHIFT_EN, 0);
> +			      DPS310_TMP_SHIFT_EN | DPS310_PRS_SHIFT_EN, 0);
> +	if (r < 0)
> +		goto err;
> +
> +	/* MEAS_CFG doesn't seem to update unless first written with 0 */
> +	r = regmap_write_bits(data->regmap, DPS310_MEAS_CFG,
> +			      DPS310_MEAS_CTRL_BITS, 0);
>  	if (r < 0)
>  		goto err;
>  
> -	/* Turn on temperature measurement in the background */
> +	/* Turn on temperature and pressure measurement in the background */
>  	r = regmap_write_bits(data->regmap, DPS310_MEAS_CFG,
>  			      DPS310_MEAS_CTRL_BITS,
> -			      DPS310_TEMP_EN | DPS310_BACKGROUND);
> +			      DPS310_PRS_EN | DPS310_TEMP_EN |
> +			      DPS310_BACKGROUND);
>  	if (r < 0)
>  		goto err;
>  
> @@ -424,7 +707,7 @@ static int dps310_probe(struct i2c_client *client,
>  	if (r < 0)
>  		goto err;
>  
> -	r = dps310_get_temp_coef(data);
> +	r = dps310_get_coefs(data);
>  	if (r < 0)
>  		goto err;
>  

Re: [PATCH v2 3/3] iio: dps310: Add pressure sensing capability

[Eddie James]

On 5/11/19 4:48 AM, Jonathan Cameron wrote:
> On Wed,  8 May 2019 14:35:28 -0500
> Eddie James <eajames@linux.ibm.com> wrote:
>
>> The DPS310 supports measurement of pressure, so support that in the
>> driver. Use background measurement like the temperature sensing and
>> default to lowest precision and lowest measurement rate.
>>
>> Signed-off-by: Eddie James <eajames@linux.ibm.com>
> Hi Eddie,
>
> A few comments inline.  One is around how you might look at adding
> fifo support (pushing to an IIO buffer) in the future...  The IIO
> data model isn't as flexible as this device can be, so we may need
> to put some restrictions on combinations of options.
>
> Jonathan
>> ---
>>   drivers/iio/pressure/dps310.c | 331 +++++++++++++++++++++++++++++++++++++++---
>>   1 file changed, 307 insertions(+), 24 deletions(-)
>>
>> diff --git a/drivers/iio/pressure/dps310.c b/drivers/iio/pressure/dps310.c
>> index c42808e..a7ee28c 100644
>> --- a/drivers/iio/pressure/dps310.c
>> +++ b/drivers/iio/pressure/dps310.c
>> @@ -11,11 +11,11 @@
>>    *   c0 * 0.5 + c1 * T_raw / kT °C
>>    *
>>    * TODO:
>> - *  - Pressure sensor readings
>>    *  - Optionally support the FIFO
>>    */
>>   
>>   #include <linux/i2c.h>
>> +#include <linux/math64.h>
>>   #include <linux/module.h>
>>   #include <linux/regmap.h>
>>   
>> @@ -29,6 +29,8 @@
>>   #define DPS310_TMP_B1		0x04
>>   #define DPS310_TMP_B2		0x05
>>   #define DPS310_PRS_CFG		0x06
>> +#define  DPS310_PRS_RATE_BITS	GENMASK(6, 4)
>> +#define  DPS310_PRS_PRC_BITS	GENMASK(3, 0)
>>   #define DPS310_TMP_CFG		0x07
>>   #define  DPS310_TMP_RATE_BITS	GENMASK(6, 4)
>>   #define  DPS310_TMP_PRC_BITS	GENMASK(3, 0)
>> @@ -82,6 +84,8 @@ struct dps310_data {
>>   	struct regmap *regmap;
>>   

>> +
>>   static bool dps310_is_writeable_reg(struct device *dev, unsigned int reg)
>>   {
>>   	switch (reg) {
>> @@ -253,24 +387,141 @@ static int dps310_write_raw(struct iio_dev *iio,
>>   {
>>   	struct dps310_data *data = iio_priv(iio);
>>   
>> -	if (chan->type != IIO_TEMP)
>> +	switch (mask) {
>> +	case IIO_CHAN_INFO_SAMP_FREQ:
>> +		switch (chan->type) {
>> +		case IIO_PRESSURE:
>> +			return dps310_set_pres_samp_freq(data, val);
>> +
>> +		case IIO_TEMP:
>> +			return dps310_set_temp_samp_freq(data, val);
> This may need a bit of thought if there is any chance we will later support
> the fifo.
>
> The IIO model is that of scans that read all channels at each 'trigger'. In
> devices like this which allow for different sampling rates for different sensor
> channels there are two options.
>
> 1) Don't support it.
> 2) Deal with registering two separate IIO devices and do the demux in the
> driver to the relevant one.
>
> All depends on whether there is a substantial usecase for different sampling
> rates or not.  Here I suspect the answer is not.
>
> The complexity is that, you then need to work out how to 'upgrade' the
> interface when buffered support is added. Obvious options are:
>
> 1) Refuse to move to buffered mode if the sampling frequencies are different.
> 2) Force the slower channel to be sampled faster if that is possible.
> 3) Change to only having one exposed sampling frequency at all - the problem
> with this last one is that it changes the ABI for existing users.
>
> It may be no one ever cares about the fifo mode though as high speed pressure
> measurement is 'unusual' ;)


Thanks for the comments Jonathan. I will follow your suggestions 
throughout the driver.

The sampling rates are a bit confusing for me, and I haven't looked into 
the buffered mode at all. Are you saying that in the current form, it 
won't work to use different sampling frequencies for the two sensors 
(without buffered mode I mean)? I haven't noticed any problems in my 
tests. I'm inclined force the slower channel to be sampled faster if 
necessary when buffered mode is implemented.

Thanks,

Eddie


>
>> +
>> +		default:
>> +			return -EINVAL;
>> +		}
>> +
>> +	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
>> +		switch (chan->type) {
>> +		case IIO_PRESSURE:
>> +			return dps310_set_pres_precision(data, val);
>> +
>> +		case IIO_TEMP:
>> +			return dps310_set_temp_precision(data, val);
>> +
>> +		default:
>> +			return -EINVAL;
>> +		}
>> +
>> +	default:
>>   		return -EINVAL;
>> +	}
>> +}
>> +
>> +static int dps310_calculate_pressure(struct dps310_data *data)
>> +{
>> +	int i, r, t_ready;
>> +	int kpi = dps310_get_pres_k(data);
>> +	int kti = dps310_get_temp_k(data);
>> +	s64 rem = 0ULL;
>> +	s64 pressure = 0ULL;
>> +	s64 p;
>> +	s64 t;
>> +	s64 denoms[7];
>> +	s64 nums[7];
>> +	s64 rems[7];
>> +	s64 kp;
>> +	s64 kt;
>> +
>> +	if (kpi < 0)
>> +		return kpi;
>> +
>> +	if (kti < 0)
>> +		return kti;
>> +
>> +	kp = (s64)kpi;
>> +	kt = (s64)kti;
>> +
>> +	/* Refresh temp if it's ready, otherwise just use the latest value */
>> +	r = regmap_read(data->regmap, DPS310_MEAS_CFG, &t_ready);
>> +	if (r >= 0 && t_ready & DPS310_TMP_RDY)
>> +		dps310_read_temp_ready(data);
>> +
>> +	p = (s64)data->pressure_raw;
>> +	t = (s64)data->temp_raw;
>> +
>> +	/* Section 4.9.1 of the DPS310 spec; algebra'd to avoid underflow */
>> +	nums[0] = (s64)data->c00;
>> +	denoms[0] = 1LL;
>> +	nums[1] = p * (s64)data->c10;
>> +	denoms[1] = kp;
>> +	nums[2] = p * p * (s64)data->c20;
>> +	denoms[2] = kp * kp;
>> +	nums[3] = p * p * p * (s64)data->c30;
>> +	denoms[3] = kp * kp * kp;
>> +	nums[4] = t * (s64)data->c01;
>> +	denoms[4] = kt;
>> +	nums[5] = t * p * (s64)data->c11;
>> +	denoms[5] = kp * kt;
>> +	nums[6] = t * p * p * (s64)data->c21;
>> +	denoms[6] = kp * kp * kt;
>> +
>> +	/* Kernel lacks a div64_s64_rem function; denoms are all positive */
>> +	for (i = 0; i < 7; ++i) {
>> +		u64 rem;
>> +
>> +		if (nums[i] < 0LL) {
>> +			pressure -= div64_u64_rem(-nums[i], denoms[i], &rem);
>> +			rems[i] = -rem;
>> +		} else {
>> +			pressure += div64_u64_rem(nums[i], denoms[i], &rem);
>> +			rems[i] = (s64)rem;
>> +		}
>> +	}
>> +
>> +	/* Increase precision and calculate the remainder sum */
>> +	for (i = 0; i < 7; ++i)
>> +		rem += div64_s64((s64)rems[i] * 1000000000LL, denoms[i]);
>> +
>> +	pressure += div_s64(rem, 1000000000LL);
>> +
>> +	return (int)pressure;
>> +}
>> +
>> +static int dps310_read_pressure(struct dps310_data *data, int *val, int *val2,
>> +				long mask)
>> +{
>> +	int ret;
>>   
>>   	switch (mask) {
>>   	case IIO_CHAN_INFO_SAMP_FREQ:
>> -		return dps310_set_temp_samp_freq(data, val);
>> +		*val = dps310_get_pres_samp_freq(data);
>> +		return IIO_VAL_INT;
>> +
>> +	case IIO_CHAN_INFO_RAW:
>> +		ret = dps310_read_pres_raw(data);
>> +		if (ret)
>> +			return ret;
>> +
>> +		*val = dps310_calculate_pressure(data);
> This is rather far from raw :)  It might be better at this point to just
> go for PROCESSED and apply the scale in here.
>
>> +		return IIO_VAL_INT;
>> +
>> +	case IIO_CHAN_INFO_SCALE:
>> +		*val = 1;
>> +		*val2 = 1000; /* Convert Pa to KPa per IIO ABI */
>> +		return IIO_VAL_FRACTIONAL;
>> +
>>   	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
>> -		return dps310_set_temp_precision(data, val);
>> +		*val = dps310_get_pres_precision(data);
>> +		return IIO_VAL_INT;
>> +
>>   	default:
>>   		return -EINVAL;
>>   	}
>>   }
>>   
>> -static int dps310_read_raw(struct iio_dev *iio,
>> -			   struct iio_chan_spec const *chan,
>> -			   int *val, int *val2, long mask)
>> +static int dps310_read_temp(struct dps310_data *data, int *val, int *val2,
>> +			    long mask)
>>   {
>> -	struct dps310_data *data = iio_priv(iio);
>>   	int ret;
>>   
>>   	switch (mask) {
>> @@ -279,7 +530,7 @@ static int dps310_read_raw(struct iio_dev *iio,
>>   		return IIO_VAL_INT;
>>   
>>   	case IIO_CHAN_INFO_RAW:
>> -		ret = dps310_read_temp(data);
>> +		ret = dps310_read_temp_raw(data);
>>   		if (ret)
>>   			return ret;
>>   
>> @@ -312,6 +563,24 @@ static int dps310_read_raw(struct iio_dev *iio,
>>   	}
>>   }
>>   
>> +static int dps310_read_raw(struct iio_dev *iio,
>> +			   struct iio_chan_spec const *chan,
>> +			   int *val, int *val2, long mask)
>> +{
>> +	struct dps310_data *data = iio_priv(iio);
>> +
>> +	switch (chan->type) {
>> +	case IIO_PRESSURE:
>> +		return dps310_read_pressure(data, val, val2, mask);
>> +
>> +	case IIO_TEMP:
>> +		return dps310_read_temp(data, val, val2, mask);
>> +
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +}
>> +
>>   static const struct regmap_config dps310_regmap_config = {
>>   	.reg_bits = 8,
>>   	.val_bits = 8,
>> @@ -393,6 +662,13 @@ static int dps310_probe(struct i2c_client *client,
>>   		return PTR_ERR(data->regmap);
>>   
>>   	/*
>> +	 * Set up pressure sensor in single sample, one measurement per second
>> +	 * mode
>> +	 */
>> +	r = regmap_write(data->regmap, DPS310_PRS_CFG,
>> +			 DPS310_CALC_RATE(1) | DPS310_CALC_PRC(1));
>> +
>> +	/*
>>   	 * Set up external (MEMS) temperature sensor in single sample, one
>>   	 * measurement per second mode
>>   	 */
>> @@ -402,16 +678,23 @@ static int dps310_probe(struct i2c_client *client,
>>   	if (r < 0)
>>   		goto err;
>>   
>> -	/* Temp shift is disabled when PRC <= 8 */
>> +	/* Temp and pressure shifts are disabled when PRC <= 8 */
>>   	r = regmap_write_bits(data->regmap, DPS310_CFG_REG,
>> -			      DPS310_TMP_SHIFT_EN, 0);
>> +			      DPS310_TMP_SHIFT_EN | DPS310_PRS_SHIFT_EN, 0);
>> +	if (r < 0)
>> +		goto err;
>> +
>> +	/* MEAS_CFG doesn't seem to update unless first written with 0 */
>> +	r = regmap_write_bits(data->regmap, DPS310_MEAS_CFG,
>> +			      DPS310_MEAS_CTRL_BITS, 0);
>>   	if (r < 0)
>>   		goto err;
>>   
>> -	/* Turn on temperature measurement in the background */
>> +	/* Turn on temperature and pressure measurement in the background */
>>   	r = regmap_write_bits(data->regmap, DPS310_MEAS_CFG,
>>   			      DPS310_MEAS_CTRL_BITS,
>> -			      DPS310_TEMP_EN | DPS310_BACKGROUND);
>> +			      DPS310_PRS_EN | DPS310_TEMP_EN |
>> +			      DPS310_BACKGROUND);
>>   	if (r < 0)
>>   		goto err;
>>   
>> @@ -424,7 +707,7 @@ static int dps310_probe(struct i2c_client *client,
>>   	if (r < 0)
>>   		goto err;
>>   
>> -	r = dps310_get_temp_coef(data);
>> +	r = dps310_get_coefs(data);
>>   	if (r < 0)
>>   		goto err;
>>   

Re: [PATCH v2 3/3] iio: dps310: Add pressure sensing capability

[Jonathan Cameron]

On Tue, 14 May 2019 15:25:55 -0500
Eddie James <eajames@linux.vnet.ibm.com> wrote:

> On 5/11/19 4:48 AM, Jonathan Cameron wrote:
> > On Wed,  8 May 2019 14:35:28 -0500
> > Eddie James <eajames@linux.ibm.com> wrote:
> >  
> >> The DPS310 supports measurement of pressure, so support that in the
> >> driver. Use background measurement like the temperature sensing and
> >> default to lowest precision and lowest measurement rate.
> >>
> >> Signed-off-by: Eddie James <eajames@linux.ibm.com>  
> > Hi Eddie,
> >
> > A few comments inline.  One is around how you might look at adding
> > fifo support (pushing to an IIO buffer) in the future...  The IIO
> > data model isn't as flexible as this device can be, so we may need
> > to put some restrictions on combinations of options.
> >
> > Jonathan  
> >> ---
> >>   drivers/iio/pressure/dps310.c | 331 +++++++++++++++++++++++++++++++++++++++---
> >>   1 file changed, 307 insertions(+), 24 deletions(-)
> >>
> >> diff --git a/drivers/iio/pressure/dps310.c b/drivers/iio/pressure/dps310.c
> >> index c42808e..a7ee28c 100644
> >> --- a/drivers/iio/pressure/dps310.c
> >> +++ b/drivers/iio/pressure/dps310.c
> >> @@ -11,11 +11,11 @@
> >>    *   c0 * 0.5 + c1 * T_raw / kT °C
> >>    *
> >>    * TODO:
> >> - *  - Pressure sensor readings
> >>    *  - Optionally support the FIFO
> >>    */
> >>   
> >>   #include <linux/i2c.h>
> >> +#include <linux/math64.h>
> >>   #include <linux/module.h>
> >>   #include <linux/regmap.h>
> >>   
> >> @@ -29,6 +29,8 @@
> >>   #define DPS310_TMP_B1		0x04
> >>   #define DPS310_TMP_B2		0x05
> >>   #define DPS310_PRS_CFG		0x06
> >> +#define  DPS310_PRS_RATE_BITS	GENMASK(6, 4)
> >> +#define  DPS310_PRS_PRC_BITS	GENMASK(3, 0)
> >>   #define DPS310_TMP_CFG		0x07
> >>   #define  DPS310_TMP_RATE_BITS	GENMASK(6, 4)
> >>   #define  DPS310_TMP_PRC_BITS	GENMASK(3, 0)
> >> @@ -82,6 +84,8 @@ struct dps310_data {
> >>   	struct regmap *regmap;
> >>     
> 
> >> +
> >>   static bool dps310_is_writeable_reg(struct device *dev, unsigned int reg)
> >>   {
> >>   	switch (reg) {
> >> @@ -253,24 +387,141 @@ static int dps310_write_raw(struct iio_dev *iio,
> >>   {
> >>   	struct dps310_data *data = iio_priv(iio);
> >>   
> >> -	if (chan->type != IIO_TEMP)
> >> +	switch (mask) {
> >> +	case IIO_CHAN_INFO_SAMP_FREQ:
> >> +		switch (chan->type) {
> >> +		case IIO_PRESSURE:
> >> +			return dps310_set_pres_samp_freq(data, val);
> >> +
> >> +		case IIO_TEMP:
> >> +			return dps310_set_temp_samp_freq(data, val);  
> > This may need a bit of thought if there is any chance we will later support
> > the fifo.
> >
> > The IIO model is that of scans that read all channels at each 'trigger'. In
> > devices like this which allow for different sampling rates for different sensor
> > channels there are two options.
> >
> > 1) Don't support it.
> > 2) Deal with registering two separate IIO devices and do the demux in the
> > driver to the relevant one.
> >
> > All depends on whether there is a substantial usecase for different sampling
> > rates or not.  Here I suspect the answer is not.
> >
> > The complexity is that, you then need to work out how to 'upgrade' the
> > interface when buffered support is added. Obvious options are:
> >
> > 1) Refuse to move to buffered mode if the sampling frequencies are different.
> > 2) Force the slower channel to be sampled faster if that is possible.
> > 3) Change to only having one exposed sampling frequency at all - the problem
> > with this last one is that it changes the ABI for existing users.
> >
> > It may be no one ever cares about the fifo mode though as high speed pressure
> > measurement is 'unusual' ;)  
> 
> 
> Thanks for the comments Jonathan. I will follow your suggestions 
> throughout the driver.
> 
> The sampling rates are a bit confusing for me, and I haven't looked into 
> the buffered mode at all. Are you saying that in the current form, it 
> won't work to use different sampling frequencies for the two sensors 
> (without buffered mode I mean)? I haven't noticed any problems in my 
> tests. I'm inclined force the slower channel to be sampled faster if 
> necessary when buffered mode is implemented.
That would be a 'slightly' interesting interpretation of the ABI, but
I'd 'probably' let it go as any correct userspace should be fine
receiving data at a higher rate than it asked for.

Jonathan
> 
> Thanks,
> 
> Eddie
> 
> 
> >  
> >> +
> >> +		default:
> >> +			return -EINVAL;
> >> +		}
> >> +
> >> +	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> >> +		switch (chan->type) {
> >> +		case IIO_PRESSURE:
> >> +			return dps310_set_pres_precision(data, val);
> >> +
> >> +		case IIO_TEMP:
> >> +			return dps310_set_temp_precision(data, val);
> >> +
> >> +		default:
> >> +			return -EINVAL;
> >> +		}
> >> +
> >> +	default:
> >>   		return -EINVAL;
> >> +	}
> >> +}
> >> +
> >> +static int dps310_calculate_pressure(struct dps310_data *data)
> >> +{
> >> +	int i, r, t_ready;
> >> +	int kpi = dps310_get_pres_k(data);
> >> +	int kti = dps310_get_temp_k(data);
> >> +	s64 rem = 0ULL;
> >> +	s64 pressure = 0ULL;
> >> +	s64 p;
> >> +	s64 t;
> >> +	s64 denoms[7];
> >> +	s64 nums[7];
> >> +	s64 rems[7];
> >> +	s64 kp;
> >> +	s64 kt;
> >> +
> >> +	if (kpi < 0)
> >> +		return kpi;
> >> +
> >> +	if (kti < 0)
> >> +		return kti;
> >> +
> >> +	kp = (s64)kpi;
> >> +	kt = (s64)kti;
> >> +
> >> +	/* Refresh temp if it's ready, otherwise just use the latest value */
> >> +	r = regmap_read(data->regmap, DPS310_MEAS_CFG, &t_ready);
> >> +	if (r >= 0 && t_ready & DPS310_TMP_RDY)
> >> +		dps310_read_temp_ready(data);
> >> +
> >> +	p = (s64)data->pressure_raw;
> >> +	t = (s64)data->temp_raw;
> >> +
> >> +	/* Section 4.9.1 of the DPS310 spec; algebra'd to avoid underflow */
> >> +	nums[0] = (s64)data->c00;
> >> +	denoms[0] = 1LL;
> >> +	nums[1] = p * (s64)data->c10;
> >> +	denoms[1] = kp;
> >> +	nums[2] = p * p * (s64)data->c20;
> >> +	denoms[2] = kp * kp;
> >> +	nums[3] = p * p * p * (s64)data->c30;
> >> +	denoms[3] = kp * kp * kp;
> >> +	nums[4] = t * (s64)data->c01;
> >> +	denoms[4] = kt;
> >> +	nums[5] = t * p * (s64)data->c11;
> >> +	denoms[5] = kp * kt;
> >> +	nums[6] = t * p * p * (s64)data->c21;
> >> +	denoms[6] = kp * kp * kt;
> >> +
> >> +	/* Kernel lacks a div64_s64_rem function; denoms are all positive */
> >> +	for (i = 0; i < 7; ++i) {
> >> +		u64 rem;
> >> +
> >> +		if (nums[i] < 0LL) {
> >> +			pressure -= div64_u64_rem(-nums[i], denoms[i], &rem);
> >> +			rems[i] = -rem;
> >> +		} else {
> >> +			pressure += div64_u64_rem(nums[i], denoms[i], &rem);
> >> +			rems[i] = (s64)rem;
> >> +		}
> >> +	}
> >> +
> >> +	/* Increase precision and calculate the remainder sum */
> >> +	for (i = 0; i < 7; ++i)
> >> +		rem += div64_s64((s64)rems[i] * 1000000000LL, denoms[i]);
> >> +
> >> +	pressure += div_s64(rem, 1000000000LL);
> >> +
> >> +	return (int)pressure;
> >> +}
> >> +
> >> +static int dps310_read_pressure(struct dps310_data *data, int *val, int *val2,
> >> +				long mask)
> >> +{
> >> +	int ret;
> >>   
> >>   	switch (mask) {
> >>   	case IIO_CHAN_INFO_SAMP_FREQ:
> >> -		return dps310_set_temp_samp_freq(data, val);
> >> +		*val = dps310_get_pres_samp_freq(data);
> >> +		return IIO_VAL_INT;
> >> +
> >> +	case IIO_CHAN_INFO_RAW:
> >> +		ret = dps310_read_pres_raw(data);
> >> +		if (ret)
> >> +			return ret;
> >> +
> >> +		*val = dps310_calculate_pressure(data);  
> > This is rather far from raw :)  It might be better at this point to just
> > go for PROCESSED and apply the scale in here.
> >  
> >> +		return IIO_VAL_INT;
> >> +
> >> +	case IIO_CHAN_INFO_SCALE:
> >> +		*val = 1;
> >> +		*val2 = 1000; /* Convert Pa to KPa per IIO ABI */
> >> +		return IIO_VAL_FRACTIONAL;
> >> +
> >>   	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> >> -		return dps310_set_temp_precision(data, val);
> >> +		*val = dps310_get_pres_precision(data);
> >> +		return IIO_VAL_INT;
> >> +
> >>   	default:
> >>   		return -EINVAL;
> >>   	}
> >>   }
> >>   
> >> -static int dps310_read_raw(struct iio_dev *iio,
> >> -			   struct iio_chan_spec const *chan,
> >> -			   int *val, int *val2, long mask)
> >> +static int dps310_read_temp(struct dps310_data *data, int *val, int *val2,
> >> +			    long mask)
> >>   {
> >> -	struct dps310_data *data = iio_priv(iio);
> >>   	int ret;
> >>   
> >>   	switch (mask) {
> >> @@ -279,7 +530,7 @@ static int dps310_read_raw(struct iio_dev *iio,
> >>   		return IIO_VAL_INT;
> >>   
> >>   	case IIO_CHAN_INFO_RAW:
> >> -		ret = dps310_read_temp(data);
> >> +		ret = dps310_read_temp_raw(data);
> >>   		if (ret)
> >>   			return ret;
> >>   
> >> @@ -312,6 +563,24 @@ static int dps310_read_raw(struct iio_dev *iio,
> >>   	}
> >>   }
> >>   
> >> +static int dps310_read_raw(struct iio_dev *iio,
> >> +			   struct iio_chan_spec const *chan,
> >> +			   int *val, int *val2, long mask)
> >> +{
> >> +	struct dps310_data *data = iio_priv(iio);
> >> +
> >> +	switch (chan->type) {
> >> +	case IIO_PRESSURE:
> >> +		return dps310_read_pressure(data, val, val2, mask);
> >> +
> >> +	case IIO_TEMP:
> >> +		return dps310_read_temp(data, val, val2, mask);
> >> +
> >> +	default:
> >> +		return -EINVAL;
> >> +	}
> >> +}
> >> +
> >>   static const struct regmap_config dps310_regmap_config = {
> >>   	.reg_bits = 8,
> >>   	.val_bits = 8,
> >> @@ -393,6 +662,13 @@ static int dps310_probe(struct i2c_client *client,
> >>   		return PTR_ERR(data->regmap);
> >>   
> >>   	/*
> >> +	 * Set up pressure sensor in single sample, one measurement per second
> >> +	 * mode
> >> +	 */
> >> +	r = regmap_write(data->regmap, DPS310_PRS_CFG,
> >> +			 DPS310_CALC_RATE(1) | DPS310_CALC_PRC(1));
> >> +
> >> +	/*
> >>   	 * Set up external (MEMS) temperature sensor in single sample, one
> >>   	 * measurement per second mode
> >>   	 */
> >> @@ -402,16 +678,23 @@ static int dps310_probe(struct i2c_client *client,
> >>   	if (r < 0)
> >>   		goto err;
> >>   
> >> -	/* Temp shift is disabled when PRC <= 8 */
> >> +	/* Temp and pressure shifts are disabled when PRC <= 8 */
> >>   	r = regmap_write_bits(data->regmap, DPS310_CFG_REG,
> >> -			      DPS310_TMP_SHIFT_EN, 0);
> >> +			      DPS310_TMP_SHIFT_EN | DPS310_PRS_SHIFT_EN, 0);
> >> +	if (r < 0)
> >> +		goto err;
> >> +
> >> +	/* MEAS_CFG doesn't seem to update unless first written with 0 */
> >> +	r = regmap_write_bits(data->regmap, DPS310_MEAS_CFG,
> >> +			      DPS310_MEAS_CTRL_BITS, 0);
> >>   	if (r < 0)
> >>   		goto err;
> >>   
> >> -	/* Turn on temperature measurement in the background */
> >> +	/* Turn on temperature and pressure measurement in the background */
> >>   	r = regmap_write_bits(data->regmap, DPS310_MEAS_CFG,
> >>   			      DPS310_MEAS_CTRL_BITS,
> >> -			      DPS310_TEMP_EN | DPS310_BACKGROUND);
> >> +			      DPS310_PRS_EN | DPS310_TEMP_EN |
> >> +			      DPS310_BACKGROUND);
> >>   	if (r < 0)
> >>   		goto err;
> >>   
> >> @@ -424,7 +707,7 @@ static int dps310_probe(struct i2c_client *client,
> >>   	if (r < 0)
> >>   		goto err;
> >>   
> >> -	r = dps310_get_temp_coef(data);
> >> +	r = dps310_get_coefs(data);
> >>   	if (r < 0)
> >>   		goto err;
> >>     
>