[PATCH] iio: Add driver for Infineon DPS310

[PATCH] iio: Add driver for Infineon DPS310

[Joel Stanley]

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

This driver supports polled measurement of temperature over i2c only.

Signed-off-by: Joel Stanley <joel@jms.id.au>
---
 drivers/iio/pressure/Kconfig  |  11 ++
 drivers/iio/pressure/Makefile |   1 +
 drivers/iio/pressure/dps310.c | 401 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 413 insertions(+)
 create mode 100644 drivers/iio/pressure/dps310.c

diff --git a/drivers/iio/pressure/Kconfig b/drivers/iio/pressure/Kconfig
index bd8d96b96771..50caefc0cd7d 100644
--- a/drivers/iio/pressure/Kconfig
+++ b/drivers/iio/pressure/Kconfig
@@ -42,6 +42,17 @@ config BMP280_SPI
 	depends on SPI_MASTER
 	select REGMAP
 
+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 measures temperature only.
+
+	 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 de3dbc81dc5a..997bbaebfa34 100644
--- a/drivers/iio/pressure/Makefile
+++ b/drivers/iio/pressure/Makefile
@@ -8,6 +8,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_HID_SENSOR_PRESS)   += hid-sensor-press.o
 obj-$(CONFIG_HP03) += hp03.o
 obj-$(CONFIG_MPL115) += mpl115.o
diff --git a/drivers/iio/pressure/dps310.c b/drivers/iio/pressure/dps310.c
new file mode 100644
index 000000000000..5e8f91a5662b
--- /dev/null
+++ b/drivers/iio/pressure/dps310.c
@@ -0,0 +1,401 @@
+/*
+ * Copyright 2017 IBM Corporation
+ *
+ * Joel Stanley <joel@jms.id.au>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version
+ * 2 of the License, or (at your option) any later version.
+ *
+ * The DPS310 is a barometric pressure and temperature sensor.
+ * Currently only reading a single temperature is supported by
+ * this driver.
+ *
+ * TODO:
+ *  - Pressure sensor readings
+ *  - Optionally support the FIFO
+ */
+
+#include <linux/module.h>
+#include <linux/i2c.h>
+#include <linux/regmap.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+
+#define PRS_BASE	0x00
+#define TMP_BASE	0x03
+#define PRS_CFG		0x06
+#define TMP_CFG		0x07
+#define  TMP_RATE_BITS	GENMASK(6, 4)
+#define  TMP_PRC_BITS	GENMASK(3, 0)
+#define  TMP_EXT	BIT(7)
+#define MEAS_CFG	0x08
+#define  MEAS_CTRL_BITS	GENMASK(2, 0)
+#define   PRESSURE_EN	BIT(0)
+#define   TEMP_EN	BIT(1)
+#define   BACKGROUND	BIT(2)
+#define  PRS_RDY	BIT(4)
+#define  TMP_RDY	BIT(5)
+#define  SENSOR_RDY	BIT(6)
+#define  COEF_RDY	BIT(7)
+#define CFG_REG		0x09
+#define  INT_HL		BIT(7)
+#define  TMP_SHIFT_EN	BIT(3)
+#define  PRS_SHIFT_EN	BIT(4)
+#define  FIFO_EN	BIT(5)
+#define  SPI_EN		BIT(6)
+#define RESET		0x0c
+#define  RESET_MAGIC	(BIT(0) | BIT(3))
+#define COEF_BASE	0x10
+
+#define TMP_RATE(_n)	ilog2(_n)
+#define TMP_PRC(_n)	ilog2(_n)
+
+#define MCELSIUS_PER_CELSIUS	1000
+
+const int scale_factor[] = {
+	 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),
+	},
+};
+
+static s32 dps310_twos_compliment(u32 raw, size_t num_bits)
+{
+	s32 out = raw;
+
+	if (raw & BIT(num_bits - 1))
+		out = raw - BIT(num_bits);
+
+	return out;
+}
+
+static int dps310_get_temp_coef(struct dps310_data *data)
+{
+	struct regmap *regmap = data->regmap;
+	uint8_t coef[3] = {0};
+	int ready, r;
+	u32 c0, c1;
+
+	r = regmap_read(regmap, MEAS_CFG, &ready);
+	if (r < 0)
+		return r;
+
+	if (!(ready & COEF_RDY))
+		return -EAGAIN;
+
+	/*
+	 * 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, COEF_BASE, coef, 3);
+	if (r < 0)
+		return r;
+
+	c0 = (coef[0] << 4) | (coef[1] >> 4);
+	data->c0 = dps310_twos_compliment(c0, 12);
+
+	c1 = ((coef[1] & GENMASK(3, 0)) << 8) | coef[2];
+	data->c1 = dps310_twos_compliment(c1, 12);
+
+	return 0;
+}
+
+static int dps310_get_temp_precision(struct dps310_data *data)
+{
+	int val, r;
+
+	r = regmap_read(data->regmap, 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 ? TMP_SHIFT_EN : 0;
+	ret = regmap_write_bits(data->regmap, CFG_REG, TMP_SHIFT_EN, shift_en);
+	if (ret)
+		return ret;
+
+	return regmap_update_bits(data->regmap, TMP_CFG, TMP_PRC_BITS,
+			TMP_PRC(val));
+}
+
+static int dps310_set_temp_samp_freq(struct dps310_data *data, int freq)
+{
+	uint8_t val;
+
+	if (freq < 0 || freq > 128)
+		return -EINVAL;
+
+	val = ilog2(freq) << 4;
+
+	return regmap_update_bits(data->regmap, TMP_CFG, TMP_RATE_BITS, val);
+}
+
+static int dps310_get_temp_samp_freq(struct dps310_data *data)
+{
+	int val, r;
+
+	r = regmap_read(data->regmap, TMP_CFG, &val);
+	if (r < 0)
+		return r;
+
+	return BIT((val & TMP_RATE_BITS) >> 4);
+}
+
+static int dps310_get_temp_k(struct dps310_data *data)
+{
+	int index = ilog2(dps310_get_temp_precision(data));
+
+	return scale_factor[index];
+}
+
+static int dps310_read_temp(struct dps310_data *data)
+{
+	struct device *dev = &data->client->dev;
+	struct regmap *regmap = data->regmap;
+	uint8_t val[3] = {0};
+	int r, ready;
+	int T_raw;
+
+	r = regmap_read(regmap, MEAS_CFG, &ready);
+	if (r < 0)
+		return r;
+	if (!(ready & TMP_RDY)) {
+		dev_dbg(dev, "temperature not ready\n");
+		return -EAGAIN;
+	}
+
+	r = regmap_bulk_read(regmap, TMP_BASE, val, 3);
+	if (r < 0)
+		return r;
+
+	T_raw = (val[0] << 16) | (val[1] << 8) | val[2];
+	data->temp_raw = dps310_twos_compliment(T_raw, 24);
+
+	return 0;
+}
+
+static bool dps310_is_writeable_reg(struct device *dev, unsigned int reg)
+{
+	switch (reg) {
+	case PRS_CFG:
+	case TMP_CFG:
+	case MEAS_CFG:
+	case CFG_REG:
+	case RESET:
+		return true;
+	default:
+		return false;
+	}
+}
+
+static bool dps310_is_volatile_reg(struct device *dev, unsigned int reg)
+{
+	switch (reg) {
+	case PRS_BASE ... (PRS_BASE + 2):
+	case TMP_BASE ... (TMP_BASE + 2):
+	case 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;
+	}
+
+	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;
+
+	/* c0 * 0.5 + c1 * T_raw / kT °C */
+	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:
+		*val = (data->c0 >> 1) * dps310_get_temp_k(data);
+		return IIO_VAL_INT;
+
+	case IIO_CHAN_INFO_SCALE:
+		*val = MCELSIUS_PER_CELSIUS;
+		*val2 = dps310_get_temp_k(data);
+		return IIO_VAL_FRACTIONAL;
+
+	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
+		*val = dps310_get_temp_precision(data);
+		return IIO_VAL_INT;
+
+	default:
+		return -EINVAL;
+	}
+
+	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 = {
+	.driver_module = THIS_MODULE,
+	.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;
+
+	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);
+
+	r = regmap_write(data->regmap, TMP_CFG, TMP_EXT | TMP_PRC(1));
+	if (r < 0)
+		return r;
+	r = regmap_write_bits(data->regmap, CFG_REG, TMP_SHIFT_EN, 0);
+	if (r < 0)
+		return r;
+
+	/* Turn on temperature measurement in the background */
+	r = regmap_write_bits(data->regmap, MEAS_CFG, MEAS_CTRL_BITS,
+			TEMP_EN | BACKGROUND);
+	if (r < 0)
+		return r;
+
+	/*
+	 * Calibration coefficients required for reporting temperature.
+	 * They are availalbe 40ms after the device has started
+	 */
+	r = dps310_get_temp_coef(data);
+	if (r == -EAGAIN)
+		return -EPROBE_DEFER;
+	if (r < 0)
+		return r;
+
+	r = devm_iio_device_register(&client->dev, iio);
+	if (r)
+		return r;
+
+	i2c_set_clientdata(client, iio);
+
+	dev_info(&client->dev, "%s: sensor '%s'\n", dev_name(&iio->dev),
+			client->name);
+
+	return 0;
+}
+
+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,
+	.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");
-- 
2.11.0

Re: [PATCH] iio: Add driver for Infineon DPS310

[Peter Meerwald-Stadler]

[-- Attachment #1: Type: text/plain, Size: 12385 bytes --]

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

comments below
 
> This driver supports polled measurement of temperature over i2c only.
> 
> Signed-off-by: Joel Stanley <joel@jms.id.au>
> ---
>  drivers/iio/pressure/Kconfig  |  11 ++
>  drivers/iio/pressure/Makefile |   1 +
>  drivers/iio/pressure/dps310.c | 401 ++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 413 insertions(+)
>  create mode 100644 drivers/iio/pressure/dps310.c
> 
> diff --git a/drivers/iio/pressure/Kconfig b/drivers/iio/pressure/Kconfig
> index bd8d96b96771..50caefc0cd7d 100644
> --- a/drivers/iio/pressure/Kconfig
> +++ b/drivers/iio/pressure/Kconfig
> @@ -42,6 +42,17 @@ config BMP280_SPI
>  	depends on SPI_MASTER
>  	select REGMAP
>  
> +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 measures temperature only.

does it make sense to propose a driver for it then?
is there intention to add pressure soonish?

link to datasheet would be nice

> +
> +	 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 de3dbc81dc5a..997bbaebfa34 100644
> --- a/drivers/iio/pressure/Makefile
> +++ b/drivers/iio/pressure/Makefile
> @@ -8,6 +8,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_HID_SENSOR_PRESS)   += hid-sensor-press.o
>  obj-$(CONFIG_HP03) += hp03.o
>  obj-$(CONFIG_MPL115) += mpl115.o
> diff --git a/drivers/iio/pressure/dps310.c b/drivers/iio/pressure/dps310.c
> new file mode 100644
> index 000000000000..5e8f91a5662b
> --- /dev/null
> +++ b/drivers/iio/pressure/dps310.c
> @@ -0,0 +1,401 @@
> +/*
> + * Copyright 2017 IBM Corporation
> + *
> + * Joel Stanley <joel@jms.id.au>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version
> + * 2 of the License, or (at your option) any later version.
> + *
> + * The DPS310 is a barometric pressure and temperature sensor.
> + * Currently only reading a single temperature is supported by
> + * this driver.
> + *

maybe add a comment with I2C address

> + * TODO:
> + *  - Pressure sensor readings
> + *  - Optionally support the FIFO
> + */
> +
> +#include <linux/module.h>
> +#include <linux/i2c.h>
> +#include <linux/regmap.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +
> +#define PRS_BASE	0x00

prefix everything with DPS310_ or dps310_

> +#define TMP_BASE	0x03
> +#define PRS_CFG		0x06
> +#define TMP_CFG		0x07
> +#define  TMP_RATE_BITS	GENMASK(6, 4)
> +#define  TMP_PRC_BITS	GENMASK(3, 0)
> +#define  TMP_EXT	BIT(7)
> +#define MEAS_CFG	0x08
> +#define  MEAS_CTRL_BITS	GENMASK(2, 0)
> +#define   PRESSURE_EN	BIT(0)
> +#define   TEMP_EN	BIT(1)
> +#define   BACKGROUND	BIT(2)
> +#define  PRS_RDY	BIT(4)
> +#define  TMP_RDY	BIT(5)
> +#define  SENSOR_RDY	BIT(6)
> +#define  COEF_RDY	BIT(7)
> +#define CFG_REG		0x09
> +#define  INT_HL		BIT(7)
> +#define  TMP_SHIFT_EN	BIT(3)
> +#define  PRS_SHIFT_EN	BIT(4)
> +#define  FIFO_EN	BIT(5)
> +#define  SPI_EN		BIT(6)
> +#define RESET		0x0c
> +#define  RESET_MAGIC	(BIT(0) | BIT(3))
> +#define COEF_BASE	0x10
> +
> +#define TMP_RATE(_n)	ilog2(_n)
> +#define TMP_PRC(_n)	ilog2(_n)
> +
> +#define MCELSIUS_PER_CELSIUS	1000

indeed :)
questionable usefulness

> +
> +const int scale_factor[] = {
> +	 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),
> +	},
> +};
> +
> +static s32 dps310_twos_compliment(u32 raw, size_t num_bits)

use sign_extend32() instead?

> +{
> +	s32 out = raw;
> +
> +	if (raw & BIT(num_bits - 1))
> +		out = raw - BIT(num_bits);
> +
> +	return out;
> +}
> +
> +static int dps310_get_temp_coef(struct dps310_data *data)
> +{
> +	struct regmap *regmap = data->regmap;
> +	uint8_t coef[3] = {0};
> +	int ready, r;
> +	u32 c0, c1;
> +
> +	r = regmap_read(regmap, MEAS_CFG, &ready);
> +	if (r < 0)
> +		return r;
> +
> +	if (!(ready & COEF_RDY))
> +		return -EAGAIN;
> +
> +	/*
> +	 * 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, COEF_BASE, coef, 3);
> +	if (r < 0)
> +		return r;
> +
> +	c0 = (coef[0] << 4) | (coef[1] >> 4);
> +	data->c0 = dps310_twos_compliment(c0, 12);
> +
> +	c1 = ((coef[1] & GENMASK(3, 0)) << 8) | coef[2];
> +	data->c1 = dps310_twos_compliment(c1, 12);
> +
> +	return 0;
> +}
> +
> +static int dps310_get_temp_precision(struct dps310_data *data)
> +{
> +	int val, r;
> +
> +	r = regmap_read(data->regmap, 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 ? TMP_SHIFT_EN : 0;
> +	ret = regmap_write_bits(data->regmap, CFG_REG, TMP_SHIFT_EN, shift_en);
> +	if (ret)
> +		return ret;
> +
> +	return regmap_update_bits(data->regmap, TMP_CFG, TMP_PRC_BITS,
> +			TMP_PRC(val));
> +}
> +
> +static int dps310_set_temp_samp_freq(struct dps310_data *data, int freq)
> +{
> +	uint8_t val;
> +
> +	if (freq < 0 || freq > 128)
> +		return -EINVAL;
> +
> +	val = ilog2(freq) << 4;
> +
> +	return regmap_update_bits(data->regmap, TMP_CFG, TMP_RATE_BITS, val);
> +}
> +
> +static int dps310_get_temp_samp_freq(struct dps310_data *data)
> +{
> +	int val, r;
> +
> +	r = regmap_read(data->regmap, TMP_CFG, &val);
> +	if (r < 0)
> +		return r;
> +
> +	return BIT((val & TMP_RATE_BITS) >> 4);
> +}
> +
> +static int dps310_get_temp_k(struct dps310_data *data)
> +{
> +	int index = ilog2(dps310_get_temp_precision(data));
> +
> +	return scale_factor[index];
> +}
> +
> +static int dps310_read_temp(struct dps310_data *data)
> +{
> +	struct device *dev = &data->client->dev;
> +	struct regmap *regmap = data->regmap;
> +	uint8_t val[3] = {0};
> +	int r, ready;
> +	int T_raw;
> +
> +	r = regmap_read(regmap, MEAS_CFG, &ready);
> +	if (r < 0)
> +		return r;
> +	if (!(ready & TMP_RDY)) {
> +		dev_dbg(dev, "temperature not ready\n");
> +		return -EAGAIN;
> +	}
> +
> +	r = regmap_bulk_read(regmap, TMP_BASE, val, 3);

sizeof(val) maybe

> +	if (r < 0)
> +		return r;
> +
> +	T_raw = (val[0] << 16) | (val[1] << 8) | val[2];
> +	data->temp_raw = dps310_twos_compliment(T_raw, 24);
> +
> +	return 0;
> +}
> +
> +static bool dps310_is_writeable_reg(struct device *dev, unsigned int reg)
> +{
> +	switch (reg) {
> +	case PRS_CFG:
> +	case TMP_CFG:
> +	case MEAS_CFG:
> +	case CFG_REG:
> +	case RESET:
> +		return true;
> +	default:
> +		return false;
> +	}
> +}
> +
> +static bool dps310_is_volatile_reg(struct device *dev, unsigned int reg)
> +{
> +	switch (reg) {
> +	case PRS_BASE ... (PRS_BASE + 2):
> +	case TMP_BASE ... (TMP_BASE + 2):
> +	case 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;
> +	}
> +
> +	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;
> +
> +	/* c0 * 0.5 + c1 * T_raw / kT °C */
> +	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:
> +		*val = (data->c0 >> 1) * dps310_get_temp_k(data);
> +		return IIO_VAL_INT;
> +
> +	case IIO_CHAN_INFO_SCALE:
> +		*val = MCELSIUS_PER_CELSIUS;
> +		*val2 = dps310_get_temp_k(data);
> +		return IIO_VAL_FRACTIONAL;
> +
> +	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> +		*val = dps310_get_temp_precision(data);
> +		return IIO_VAL_INT;
> +
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	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 = {
> +	.driver_module = THIS_MODULE,
> +	.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;
> +
> +	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);
> +
> +	r = regmap_write(data->regmap, TMP_CFG, TMP_EXT | TMP_PRC(1));
> +	if (r < 0)
> +		return r;
> +	r = regmap_write_bits(data->regmap, CFG_REG, TMP_SHIFT_EN, 0);
> +	if (r < 0)
> +		return r;
> +
> +	/* Turn on temperature measurement in the background */
> +	r = regmap_write_bits(data->regmap, MEAS_CFG, MEAS_CTRL_BITS,
> +			TEMP_EN | BACKGROUND);

should be disabled again if _probe fails lateron

> +	if (r < 0)
> +		return r;
> +
> +	/*
> +	 * Calibration coefficients required for reporting temperature.
> +	 * They are availalbe 40ms after the device has started

available

> +	 */
> +	r = dps310_get_temp_coef(data);
> +	if (r == -EAGAIN)
> +		return -EPROBE_DEFER;

wouldn't it make more sense to wait for the data to become ready, maybe 
with a timeout?

> +	if (r < 0)
> +		return r;
> +
> +	r = devm_iio_device_register(&client->dev, iio);
> +	if (r)
> +		return r;
> +
> +	i2c_set_clientdata(client, iio);
> +
> +	dev_info(&client->dev, "%s: sensor '%s'\n", dev_name(&iio->dev),
> +			client->name);

no extra logging, this adds little extra information

> +	return 0;
> +}

turn off measurement in _remove()?

> +
> +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,
> +	.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");
> 

-- 

Peter Meerwald-Stadler
Mobile: +43 664 24 44 418

Re: [PATCH] iio: Add driver for Infineon DPS310

[Jonathan Cameron]

On 04/05/17 08:19, Joel Stanley wrote:
> The DPS310 is a temperature and pressure sensor. It can be accessed over
> i2c and SPI.
> 
> This driver supports polled measurement of temperature over i2c only.
> 
> Signed-off-by: Joel Stanley <joel@jms.id.au>
Few bits inline, but looks pretty good for a v1.
> ---
>  drivers/iio/pressure/Kconfig  |  11 ++
>  drivers/iio/pressure/Makefile |   1 +
>  drivers/iio/pressure/dps310.c | 401 ++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 413 insertions(+)
>  create mode 100644 drivers/iio/pressure/dps310.c
> 
> diff --git a/drivers/iio/pressure/Kconfig b/drivers/iio/pressure/Kconfig
> index bd8d96b96771..50caefc0cd7d 100644
> --- a/drivers/iio/pressure/Kconfig
> +++ b/drivers/iio/pressure/Kconfig
> @@ -42,6 +42,17 @@ config BMP280_SPI
>  	depends on SPI_MASTER
>  	select REGMAP
>  
> +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 measures temperature only.
> +
> +	 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 de3dbc81dc5a..997bbaebfa34 100644
> --- a/drivers/iio/pressure/Makefile
> +++ b/drivers/iio/pressure/Makefile
> @@ -8,6 +8,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_HID_SENSOR_PRESS)   += hid-sensor-press.o
>  obj-$(CONFIG_HP03) += hp03.o
>  obj-$(CONFIG_MPL115) += mpl115.o
> diff --git a/drivers/iio/pressure/dps310.c b/drivers/iio/pressure/dps310.c
> new file mode 100644
> index 000000000000..5e8f91a5662b
> --- /dev/null
> +++ b/drivers/iio/pressure/dps310.c
> @@ -0,0 +1,401 @@
> +/*
> + * Copyright 2017 IBM Corporation
> + *
> + * Joel Stanley <joel@jms.id.au>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version
> + * 2 of the License, or (at your option) any later version.
> + *
> + * The DPS310 is a barometric pressure and temperature sensor.
> + * Currently only reading a single temperature is supported by
> + * this driver.
> + *
> + * TODO:
> + *  - Pressure sensor readings
> + *  - Optionally support the FIFO
> + */
> +
> +#include <linux/module.h>
> +#include <linux/i2c.h>
> +#include <linux/regmap.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
Slight preference for alphabetical order though grouping the iio
ones at the end is fine.
> +
General preference in IIO is to prefix
all defines with something driver appropriate.

DPS310_PRS_BASE etc 

Avoids potential mess down the line with defines
in headers. Lots of these are very generic so
that's not implausible.
> +#define PRS_BASE	0x00
> +#define TMP_BASE	0x03
> +#define PRS_CFG		0x06
> +#define TMP_CFG		0x07
> +#define  TMP_RATE_BITS	GENMASK(6, 4)
> +#define  TMP_PRC_BITS	GENMASK(3, 0)
> +#define  TMP_EXT	BIT(7)
> +#define MEAS_CFG	0x08
> +#define  MEAS_CTRL_BITS	GENMASK(2, 0)
> +#define   PRESSURE_EN	BIT(0)
> +#define   TEMP_EN	BIT(1)
> +#define   BACKGROUND	BIT(2)
> +#define  PRS_RDY	BIT(4)
> +#define  TMP_RDY	BIT(5)
> +#define  SENSOR_RDY	BIT(6)
> +#define  COEF_RDY	BIT(7)
> +#define CFG_REG		0x09
> +#define  INT_HL		BIT(7)
> +#define  TMP_SHIFT_EN	BIT(3)
> +#define  PRS_SHIFT_EN	BIT(4)
> +#define  FIFO_EN	BIT(5)
> +#define  SPI_EN		BIT(6)
> +#define RESET		0x0c
> +#define  RESET_MAGIC	(BIT(0) | BIT(3))
> +#define COEF_BASE	0x10
> +
> +#define TMP_RATE(_n)	ilog2(_n)
You define these but then have it long hand in the code?
> +#define TMP_PRC(_n)	ilog2(_n)
> +
> +#define MCELSIUS_PER_CELSIUS	1000
> +
> +const int scale_factor[] = {
> +	 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),
> +	},
> +};
> +
> +static s32 dps310_twos_compliment(u32 raw, size_t num_bits)
> +{
> +	s32 out = raw;
> +
> +	if (raw & BIT(num_bits - 1))
> +		out = raw - BIT(num_bits);
> +
> +	return out;
> +}
> +
> +static int dps310_get_temp_coef(struct dps310_data *data)
> +{
> +	struct regmap *regmap = data->regmap;
> +	uint8_t coef[3] = {0};
> +	int ready, r;
> +	u32 c0, c1;
> +
> +	r = regmap_read(regmap, MEAS_CFG, &ready);
> +	if (r < 0)
> +		return r;
> +
> +	if (!(ready & COEF_RDY))
> +		return -EAGAIN;
> +
> +	/*
> +	 * 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, COEF_BASE, coef, 3);
> +	if (r < 0)
> +		return r;
> +
> +	c0 = (coef[0] << 4) | (coef[1] >> 4);
> +	data->c0 = dps310_twos_compliment(c0, 12);
> +
> +	c1 = ((coef[1] & GENMASK(3, 0)) << 8) | coef[2];
> +	data->c1 = dps310_twos_compliment(c1, 12);
> +
> +	return 0;
> +}
> +
> +static int dps310_get_temp_precision(struct dps310_data *data)
> +{
> +	int val, r;
> +
> +	r = regmap_read(data->regmap, 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 ? TMP_SHIFT_EN : 0;
> +	ret = regmap_write_bits(data->regmap, CFG_REG, TMP_SHIFT_EN, shift_en);
> +	if (ret)
> +		return ret;
> +
> +	return regmap_update_bits(data->regmap, TMP_CFG, TMP_PRC_BITS,
> +			TMP_PRC(val));
> +}
> +
> +static int dps310_set_temp_samp_freq(struct dps310_data *data, int freq)
> +{
> +	uint8_t val;
> +
> +	if (freq < 0 || freq > 128)
> +		return -EINVAL;
> +
> +	val = ilog2(freq) << 4;
> +
> +	return regmap_update_bits(data->regmap, TMP_CFG, TMP_RATE_BITS, val);
> +}
> +
> +static int dps310_get_temp_samp_freq(struct dps310_data *data)
> +{
> +	int val, r;
> +
> +	r = regmap_read(data->regmap, TMP_CFG, &val);
> +	if (r < 0)
> +		return r;
> +
> +	return BIT((val & TMP_RATE_BITS) >> 4);
> +}
> +
> +static int dps310_get_temp_k(struct dps310_data *data)
> +{
> +	int index = ilog2(dps310_get_temp_precision(data));
> +
> +	return scale_factor[index];
I'd just put the whole thing in one line, but up to you.
> +}
> +
> +static int dps310_read_temp(struct dps310_data *data)
> +{
> +	struct device *dev = &data->client->dev;
> +	struct regmap *regmap = data->regmap;
> +	uint8_t val[3] = {0};
> +	int r, ready;
> +	int T_raw;
> +
> +	r = regmap_read(regmap, MEAS_CFG, &ready);
> +	if (r < 0)
> +		return r;
> +	if (!(ready & TMP_RDY)) {
> +		dev_dbg(dev, "temperature not ready\n");
> +		return -EAGAIN;
> +	}
> +
> +	r = regmap_bulk_read(regmap, TMP_BASE, val, 3);
> +	if (r < 0)
> +		return r;
> +
> +	T_raw = (val[0] << 16) | (val[1] << 8) | val[2];
> +	data->temp_raw = dps310_twos_compliment(T_raw, 24);
> +
> +	return 0;
> +}
> +
> +static bool dps310_is_writeable_reg(struct device *dev, unsigned int reg)
> +{
> +	switch (reg) {
> +	case PRS_CFG:
> +	case TMP_CFG:
> +	case MEAS_CFG:
> +	case CFG_REG:
> +	case RESET:
> +		return true;
> +	default:
> +		return false;
> +	}
> +}
> +
> +static bool dps310_is_volatile_reg(struct device *dev, unsigned int reg)
> +{
> +	switch (reg) {
> +	case PRS_BASE ... (PRS_BASE + 2):
Bit ugly defining this like this.  IS PRS_BASE + 2 something that has
a logical name of its own?
> +	case TMP_BASE ... (TMP_BASE + 2):
> +	case 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;
> +	}
> +
> +	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;
> +
> +	/* c0 * 0.5 + c1 * T_raw / kT °C */
Whilst interesting why is this comment right here?
> +	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:
> +		*val = (data->c0 >> 1) * dps310_get_temp_k(data);
> +		return IIO_VAL_INT;
> +
> +	case IIO_CHAN_INFO_SCALE:
> +		*val = MCELSIUS_PER_CELSIUS;
> +		*val2 = dps310_get_temp_k(data);
> +		return IIO_VAL_FRACTIONAL;
> +
> +	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> +		*val = dps310_get_temp_precision(data);
> +		return IIO_VAL_INT;
> +
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	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 = {
> +	.driver_module = THIS_MODULE,
> +	.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;
> +
> +	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);
> +
Not that obvious what the next two calls are doing, perhaps comments?
(the third one is good)
> +	r = regmap_write(data->regmap, TMP_CFG, TMP_EXT | TMP_PRC(1));
> +	if (r < 0)
> +		return r;
> +	r = regmap_write_bits(data->regmap, CFG_REG, TMP_SHIFT_EN, 0);
> +	if (r < 0)
> +		return r;
> +
> +	/* Turn on temperature measurement in the background */
> +	r = regmap_write_bits(data->regmap, MEAS_CFG, MEAS_CTRL_BITS,
> +			TEMP_EN | BACKGROUND);
> +	if (r < 0)
> +		return r;
> +
> +	/*
> +	 * Calibration coefficients required for reporting temperature.
> +	 * They are availalbe 40ms after the device has started
available
> +	 */
> +	r = dps310_get_temp_coef(data);
> +	if (r == -EAGAIN)
> +		return -EPROBE_DEFER;
Deferred why?  If you need 40ms then sleep for 40ms to be sure it
has woken up. Or even better, loop with a sleep.

Deferred will only result in it being probed if something changes
such as another driver being loaded (which might provide
some resource we are waiting for).
> +	if (r < 0)
> +		return r;
> +
> +	r = devm_iio_device_register(&client->dev, iio);
> +	if (r)
> +		return r;
With the two lines below removed, just
return devm_iio...
> +
> +	i2c_set_clientdata(client, iio);
Used?
> +
> +	dev_info(&client->dev, "%s: sensor '%s'\n", dev_name(&iio->dev),
> +			client->name);
Provides little useful information and just spams the log so
I would prefer this was dropped.
> +
> +	return 0;
> +}
> +
> +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,
> +	.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");
> 

Re: [PATCH] iio: Add driver for Infineon DPS310

[Joel Stanley]

On Sat, May 6, 2017 at 4:15 AM, Jonathan Cameron <jic23@kernel.org> wrote:
> On 04/05/17 08:19, Joel Stanley wrote:
>> The DPS310 is a temperature and pressure sensor. It can be accessed over
>> i2c and SPI.
>>
>> This driver supports polled measurement of temperature over i2c only.
>>
>> Signed-off-by: Joel Stanley <joel@jms.id.au>
> Few bits inline, but looks pretty good for a v1.

Thanks for the reivew.

>> +#include <linux/module.h>
>> +#include <linux/i2c.h>
>> +#include <linux/regmap.h>
>> +#include <linux/iio/iio.h>
>> +#include <linux/iio/sysfs.h>
> Slight preference for alphabetical order though grouping the iio
> ones at the end is fine.

Ok.

>> +
> General preference in IIO is to prefix
> all defines with something driver appropriate.
>
> DPS310_PRS_BASE etc

Sure.

>
> Avoids potential mess down the line with defines
> in headers. Lots of these are very generic so
> that's not implausible.
>> +#define PRS_BASE     0x00
>> +#define TMP_BASE     0x03
>> +#define PRS_CFG              0x06
>> +#define TMP_CFG              0x07
>> +#define  TMP_RATE_BITS       GENMASK(6, 4)
>> +#define  TMP_PRC_BITS        GENMASK(3, 0)
>> +#define  TMP_EXT     BIT(7)
>> +#define MEAS_CFG     0x08
>> +#define  MEAS_CTRL_BITS      GENMASK(2, 0)
>> +#define   PRESSURE_EN        BIT(0)
>> +#define   TEMP_EN    BIT(1)
>> +#define   BACKGROUND BIT(2)
>> +#define  PRS_RDY     BIT(4)
>> +#define  TMP_RDY     BIT(5)
>> +#define  SENSOR_RDY  BIT(6)
>> +#define  COEF_RDY    BIT(7)
>> +#define CFG_REG              0x09
>> +#define  INT_HL              BIT(7)
>> +#define  TMP_SHIFT_EN        BIT(3)
>> +#define  PRS_SHIFT_EN        BIT(4)
>> +#define  FIFO_EN     BIT(5)
>> +#define  SPI_EN              BIT(6)
>> +#define RESET                0x0c
>> +#define  RESET_MAGIC (BIT(0) | BIT(3))
>> +#define COEF_BASE    0x10
>> +
>> +#define TMP_RATE(_n) ilog2(_n)
> You define these but then have it long hand in the code?

I will drop these I think. I've reworked the code a few times and
these were left over.

>> +static int dps310_get_temp_k(struct dps310_data *data)
>> +{
>> +     int index = ilog2(dps310_get_temp_precision(data));
>> +
>> +     return scale_factor[index];
> I'd just put the whole thing in one line, but up to you.

I previously had a check here that the range was not out of range but
I dropped the check as it can't happen.

I'll put it on one line.

>> +static bool dps310_is_volatile_reg(struct device *dev, unsigned int reg)
>> +{
>> +     switch (reg) {
>> +     case PRS_BASE ... (PRS_BASE + 2):
> Bit ugly defining this like this.  IS PRS_BASE + 2 something that has
> a logical name of its own?

Agreed. I used to have PRS_B0, PRS_B1, PRS_B2 as we read out the three
bytes at once they were not required. I'll clean it up.


>> +
>> +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;
>> +
>> +     /* c0 * 0.5 + c1 * T_raw / kT °C */
> Whilst interesting why is this comment right here?

Will move it to the top of the file.

>> +
>> +static int dps310_probe(struct i2c_client *client,
>> +                     const struct i2c_device_id *id)
>> +{
>> +     struct dps310_data *data;
>> +     struct iio_dev *iio;
>> +     int r;
>> +
>> +     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);
>> +
> Not that obvious what the next two calls are doing, perhaps comments?
> (the third one is good)

Ack.

>> +     r = regmap_write(data->regmap, TMP_CFG, TMP_EXT | TMP_PRC(1));
>> +     if (r < 0)
>> +             return r;
>> +     r = regmap_write_bits(data->regmap, CFG_REG, TMP_SHIFT_EN, 0);
>> +     if (r < 0)
>> +             return r;
>> +
>> +     /* Turn on temperature measurement in the background */
>> +     r = regmap_write_bits(data->regmap, MEAS_CFG, MEAS_CTRL_BITS,
>> +                     TEMP_EN | BACKGROUND);
>> +     if (r < 0)
>> +             return r;
>> +
>> +     /*
>> +      * Calibration coefficients required for reporting temperature.
>> +      * They are availalbe 40ms after the device has started
> available
>> +      */
>> +     r = dps310_get_temp_coef(data);
>> +     if (r == -EAGAIN)
>> +             return -EPROBE_DEFER;
> Deferred why?  If you need 40ms then sleep for 40ms to be sure it
> has woken up. Or even better, loop with a sleep.

I didn't want to delay booting the system for the driver to probe.

>
> Deferred will only result in it being probed if something changes
> such as another driver being loaded (which might provide
> some resource we are waiting for).

Ok, I misunderstood how it worked. Thanks for clearing that up.

I will put the loop/sleep in.

>> +     if (r < 0)
>> +             return r;
>> +
>> +     r = devm_iio_device_register(&client->dev, iio);
>> +     if (r)
>> +             return r;
> With the two lines below removed, just
> return devm_iio...
>> +
>> +     i2c_set_clientdata(client, iio);
> Used?

I must admit I was cargo culting. Can you explain under what
circumstances I would want this?

>> +
>> +     dev_info(&client->dev, "%s: sensor '%s'\n", dev_name(&iio->dev),
>> +                     client->name);
> Provides little useful information and just spams the log so
> I would prefer this was dropped.

I would really like the core to do something like this, for both hwmon
and iio devices. I spend a lot of time developing and using systems
that use these drivers, and it's very useful to know the name of the
driver, the bus it's on, and the fact that it was both compiled in and
probed.

Would you take a patch for the core?

Cheers,

Joel

Re: [PATCH] iio: Add driver for Infineon DPS310

[Joel Stanley]

On Fri, May 5, 2017 at 9:13 PM, Peter Meerwald-Stadler
<pmeerw@pmeerw.net> wrote:
>> The DPS310 is a temperature and pressure sensor. It can be accessed over
>> i2c and SPI.
>
> comments below
>
>> This driver supports polled measurement of temperature over i2c only.
>>
>> Signed-off-by: Joel Stanley <joel@jms.id.au>
>> ---
>>  drivers/iio/pressure/Kconfig  |  11 ++
>>  drivers/iio/pressure/Makefile |   1 +
>>  drivers/iio/pressure/dps310.c | 401 ++++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 413 insertions(+)
>>  create mode 100644 drivers/iio/pressure/dps310.c
>>
>> diff --git a/drivers/iio/pressure/Kconfig b/drivers/iio/pressure/Kconfig
>> index bd8d96b96771..50caefc0cd7d 100644
>> --- a/drivers/iio/pressure/Kconfig
>> +++ b/drivers/iio/pressure/Kconfig
>> @@ -42,6 +42,17 @@ config BMP280_SPI
>>       depends on SPI_MASTER
>>       select REGMAP
>>
>> +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 measures temperature only.
>
> does it make sense to propose a driver for it then?

Yes. We are building a system that uses the sensor to measure temperature.

If you would prefer I can put it under drivers/iio/temp?

> is there intention to add pressure soonish?
>
> link to datasheet would be nice

Good idea.

>> +++ b/drivers/iio/pressure/dps310.c
>> @@ -0,0 +1,401 @@
>> +/*
>> + * Copyright 2017 IBM Corporation
>> + *
>> + * Joel Stanley <joel@jms.id.au>
>> + *
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU General Public License
>> + * as published by the Free Software Foundation; either version
>> + * 2 of the License, or (at your option) any later version.
>> + *
>> + * The DPS310 is a barometric pressure and temperature sensor.
>> + * Currently only reading a single temperature is supported by
>> + * this driver.
>> + *
>
> maybe add a comment with I2C address

The addresses are specified at the bottom of the file. Is it
convention to add them to a comment too?

>
>> + * TODO:
>> + *  - Pressure sensor readings
>> + *  - Optionally support the FIFO
>> + */
>> +
>> +#include <linux/module.h>
>> +#include <linux/i2c.h>
>> +#include <linux/regmap.h>
>> +#include <linux/iio/iio.h>
>> +#include <linux/iio/sysfs.h>
>> +
>> +#define PRS_BASE     0x00
>
> prefix everything with DPS310_ or dps310_

Ack.

>
>> +#define TMP_BASE     0x03
>> +#define PRS_CFG              0x06
>> +#define TMP_CFG              0x07
>> +#define  TMP_RATE_BITS       GENMASK(6, 4)
>> +#define  TMP_PRC_BITS        GENMASK(3, 0)
>> +#define  TMP_EXT     BIT(7)
>> +#define MEAS_CFG     0x08
>> +#define  MEAS_CTRL_BITS      GENMASK(2, 0)
>> +#define   PRESSURE_EN        BIT(0)
>> +#define   TEMP_EN    BIT(1)
>> +#define   BACKGROUND BIT(2)
>> +#define  PRS_RDY     BIT(4)
>> +#define  TMP_RDY     BIT(5)
>> +#define  SENSOR_RDY  BIT(6)
>> +#define  COEF_RDY    BIT(7)
>> +#define CFG_REG              0x09
>> +#define  INT_HL              BIT(7)
>> +#define  TMP_SHIFT_EN        BIT(3)
>> +#define  PRS_SHIFT_EN        BIT(4)
>> +#define  FIFO_EN     BIT(5)
>> +#define  SPI_EN              BIT(6)
>> +#define RESET                0x0c
>> +#define  RESET_MAGIC (BIT(0) | BIT(3))
>> +#define COEF_BASE    0x10
>> +
>> +#define TMP_RATE(_n) ilog2(_n)
>> +#define TMP_PRC(_n)  ilog2(_n)
>> +
>> +#define MCELSIUS_PER_CELSIUS 1000
>
> indeed :)
> questionable usefulness

Other drivers do the same. Perhaps it could go in the iio core?

As a driver author it tells me that the drivers is doing this
conversion, and not that the hardware requires a scaling of 1000.

>> +static s32 dps310_twos_compliment(u32 raw, size_t num_bits)
>
> use sign_extend32() instead?

Thanks for pointing this out! I was sure it must exist but I couldn't find it.


>> +
>> +     data->regmap = devm_regmap_init_i2c(client, &dps310_regmap_config);
>> +     if (IS_ERR(data->regmap))
>> +             return PTR_ERR(data->regmap);
>> +
>> +     r = regmap_write(data->regmap, TMP_CFG, TMP_EXT | TMP_PRC(1));
>> +     if (r < 0)
>> +             return r;
>> +     r = regmap_write_bits(data->regmap, CFG_REG, TMP_SHIFT_EN, 0);
>> +     if (r < 0)
>> +             return r;
>> +
>> +     /* Turn on temperature measurement in the background */
>> +     r = regmap_write_bits(data->regmap, MEAS_CFG, MEAS_CTRL_BITS,
>> +                     TEMP_EN | BACKGROUND);
>
> should be disabled again if _probe fails lateron

Ok.

>
>> +     if (r < 0)
>> +             return r;
>> +
>> +     /*
>> +      * Calibration coefficients required for reporting temperature.
>> +      * They are availalbe 40ms after the device has started
>
> available

Thanks.

>
>> +      */
>> +     r = dps310_get_temp_coef(data);
>> +     if (r == -EAGAIN)
>> +             return -EPROBE_DEFER;
>
> wouldn't it make more sense to wait for the data to become ready, maybe
> with a timeout?

Ok.

>
>> +     if (r < 0)
>> +             return r;
>> +
>> +     r = devm_iio_device_register(&client->dev, iio);
>> +     if (r)
>> +             return r;
>> +
>> +     i2c_set_clientdata(client, iio);
>> +
>> +     dev_info(&client->dev, "%s: sensor '%s'\n", dev_name(&iio->dev),
>> +                     client->name);
>
> no extra logging, this adds little extra information

I disagree - lets discuss it in my reply to Jonathan.

>
>> +     return 0;
>> +}
>
> turn off measurement in _remove()?

Ok.

Thanks for the review.

Cheers,

Joel

Re: [PATCH] iio: Add driver for Infineon DPS310

[Jonathan Cameron]

On 12/05/17 07:28, Joel Stanley wrote:
> On Fri, May 5, 2017 at 9:13 PM, Peter Meerwald-Stadler
> <pmeerw@pmeerw.net> wrote:
>>> The DPS310 is a temperature and pressure sensor. It can be accessed over
>>> i2c and SPI.
>>
>> comments below
>>
>>> This driver supports polled measurement of temperature over i2c only.
>>>
>>> Signed-off-by: Joel Stanley <joel@jms.id.au>
>>> ---
>>>   drivers/iio/pressure/Kconfig  |  11 ++
>>>   drivers/iio/pressure/Makefile |   1 +
>>>   drivers/iio/pressure/dps310.c | 401 ++++++++++++++++++++++++++++++++++++++++++
>>>   3 files changed, 413 insertions(+)
>>>   create mode 100644 drivers/iio/pressure/dps310.c
>>>
>>> diff --git a/drivers/iio/pressure/Kconfig b/drivers/iio/pressure/Kconfig
>>> index bd8d96b96771..50caefc0cd7d 100644
>>> --- a/drivers/iio/pressure/Kconfig
>>> +++ b/drivers/iio/pressure/Kconfig
>>> @@ -42,6 +42,17 @@ config BMP280_SPI
>>>        depends on SPI_MASTER
>>>        select REGMAP
>>>
>>> +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 measures temperature only.
>>
>> does it make sense to propose a driver for it then?
> 
> Yes. We are building a system that uses the sensor to measure temperature.
> 
> If you would prefer I can put it under drivers/iio/temp?
No.  You guys are probably the oddity here (it's much easier to make
a temperature chip than a pressure one so primary purpose is normally
going to be pressure.)

No chance at all you could put really basic pressure reading in place
as well?
> 
>> is there intention to add pressure soonish?
>>
>> link to datasheet would be nice
> 
> Good idea.
> 
>>> +++ b/drivers/iio/pressure/dps310.c
>>> @@ -0,0 +1,401 @@
>>> +/*
>>> + * Copyright 2017 IBM Corporation
>>> + *
>>> + * Joel Stanley <joel@jms.id.au>
>>> + *
>>> + * This program is free software; you can redistribute it and/or
>>> + * modify it under the terms of the GNU General Public License
>>> + * as published by the Free Software Foundation; either version
>>> + * 2 of the License, or (at your option) any later version.
>>> + *
>>> + * The DPS310 is a barometric pressure and temperature sensor.
>>> + * Currently only reading a single temperature is supported by
>>> + * this driver.
>>> + *
>>
>> maybe add a comment with I2C address
> 
> The addresses are specified at the bottom of the file. Is it
> convention to add them to a comment too?
> 
>>
>>> + * TODO:
>>> + *  - Pressure sensor readings
>>> + *  - Optionally support the FIFO
>>> + */
>>> +
>>> +#include <linux/module.h>
>>> +#include <linux/i2c.h>
>>> +#include <linux/regmap.h>
>>> +#include <linux/iio/iio.h>
>>> +#include <linux/iio/sysfs.h>
>>> +
>>> +#define PRS_BASE     0x00
>>
>> prefix everything with DPS310_ or dps310_
> 
> Ack.
> 
>>
>>> +#define TMP_BASE     0x03
>>> +#define PRS_CFG              0x06
>>> +#define TMP_CFG              0x07
>>> +#define  TMP_RATE_BITS       GENMASK(6, 4)
>>> +#define  TMP_PRC_BITS        GENMASK(3, 0)
>>> +#define  TMP_EXT     BIT(7)
>>> +#define MEAS_CFG     0x08
>>> +#define  MEAS_CTRL_BITS      GENMASK(2, 0)
>>> +#define   PRESSURE_EN        BIT(0)
>>> +#define   TEMP_EN    BIT(1)
>>> +#define   BACKGROUND BIT(2)
>>> +#define  PRS_RDY     BIT(4)
>>> +#define  TMP_RDY     BIT(5)
>>> +#define  SENSOR_RDY  BIT(6)
>>> +#define  COEF_RDY    BIT(7)
>>> +#define CFG_REG              0x09
>>> +#define  INT_HL              BIT(7)
>>> +#define  TMP_SHIFT_EN        BIT(3)
>>> +#define  PRS_SHIFT_EN        BIT(4)
>>> +#define  FIFO_EN     BIT(5)
>>> +#define  SPI_EN              BIT(6)
>>> +#define RESET                0x0c
>>> +#define  RESET_MAGIC (BIT(0) | BIT(3))
>>> +#define COEF_BASE    0x10
>>> +
>>> +#define TMP_RATE(_n) ilog2(_n)
>>> +#define TMP_PRC(_n)  ilog2(_n)
>>> +
>>> +#define MCELSIUS_PER_CELSIUS 1000
>>
>> indeed :)
>> questionable usefulness
> 
> Other drivers do the same. Perhaps it could go in the iio core?
I think Peter was advocating just using 1000 where needed rather
than having a define.
> 
> As a driver author it tells me that the drivers is doing this
> conversion, and not that the hardware requires a scaling of 1000.
> 
>>> +static s32 dps310_twos_compliment(u32 raw, size_t num_bits)
>>
>> use sign_extend32() instead?
> 
> Thanks for pointing this out! I was sure it must exist but I couldn't find it.
It's surprisingly new ;)
> 
> 
>>> +
>>> +     data->regmap = devm_regmap_init_i2c(client, &dps310_regmap_config);
>>> +     if (IS_ERR(data->regmap))
>>> +             return PTR_ERR(data->regmap);
>>> +
>>> +     r = regmap_write(data->regmap, TMP_CFG, TMP_EXT | TMP_PRC(1));
>>> +     if (r < 0)
>>> +             return r;
>>> +     r = regmap_write_bits(data->regmap, CFG_REG, TMP_SHIFT_EN, 0);
>>> +     if (r < 0)
>>> +             return r;
>>> +
>>> +     /* Turn on temperature measurement in the background */
>>> +     r = regmap_write_bits(data->regmap, MEAS_CFG, MEAS_CTRL_BITS,
>>> +                     TEMP_EN | BACKGROUND);
>>
>> should be disabled again if _probe fails lateron
> 
> Ok.
> 
>>
>>> +     if (r < 0)
>>> +             return r;
>>> +
>>> +     /*
>>> +      * Calibration coefficients required for reporting temperature.
>>> +      * They are availalbe 40ms after the device has started
>>
>> available
> 
> Thanks.
> 
>>
>>> +      */
>>> +     r = dps310_get_temp_coef(data);
>>> +     if (r == -EAGAIN)
>>> +             return -EPROBE_DEFER;
>>
>> wouldn't it make more sense to wait for the data to become ready, maybe
>> with a timeout?
> 
> Ok.
> 
>>
>>> +     if (r < 0)
>>> +             return r;
>>> +
>>> +     r = devm_iio_device_register(&client->dev, iio);
>>> +     if (r)
>>> +             return r;
>>> +
>>> +     i2c_set_clientdata(client, iio);
>>> +
>>> +     dev_info(&client->dev, "%s: sensor '%s'\n", dev_name(&iio->dev),
>>> +                     client->name);
>>
>> no extra logging, this adds little extra information
> 
> I disagree - lets discuss it in my reply to Jonathan.
> 
>>
>>> +     return 0;
>>> +}
>>
>> turn off measurement in _remove()?
> 
> Ok.
> 
> Thanks for the review.
> 
> Cheers,
> 
> Joel
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH] iio: Add driver for Infineon DPS310

[Jonathan Cameron]

On 12/05/17 07:18, Joel Stanley wrote:
> On Sat, May 6, 2017 at 4:15 AM, Jonathan Cameron <jic23@kernel.org> wrote:
>> On 04/05/17 08:19, Joel Stanley wrote:
>>> The DPS310 is a temperature and pressure sensor. It can be accessed over
>>> i2c and SPI.
>>>
>>> This driver supports polled measurement of temperature over i2c only.
>>>
>>> Signed-off-by: Joel Stanley <joel@jms.id.au>
>> Few bits inline, but looks pretty good for a v1.
> 
> Thanks for the reivew.
> 
>>> +#include <linux/module.h>
>>> +#include <linux/i2c.h>
>>> +#include <linux/regmap.h>
>>> +#include <linux/iio/iio.h>
>>> +#include <linux/iio/sysfs.h>
>> Slight preference for alphabetical order though grouping the iio
>> ones at the end is fine.
> 
> Ok.
> 
>>> +
>> General preference in IIO is to prefix
>> all defines with something driver appropriate.
>>
>> DPS310_PRS_BASE etc
> 
> Sure.
> 
>>
>> Avoids potential mess down the line with defines
>> in headers. Lots of these are very generic so
>> that's not implausible.
>>> +#define PRS_BASE     0x00
>>> +#define TMP_BASE     0x03
>>> +#define PRS_CFG              0x06
>>> +#define TMP_CFG              0x07
>>> +#define  TMP_RATE_BITS       GENMASK(6, 4)
>>> +#define  TMP_PRC_BITS        GENMASK(3, 0)
>>> +#define  TMP_EXT     BIT(7)
>>> +#define MEAS_CFG     0x08
>>> +#define  MEAS_CTRL_BITS      GENMASK(2, 0)
>>> +#define   PRESSURE_EN        BIT(0)
>>> +#define   TEMP_EN    BIT(1)
>>> +#define   BACKGROUND BIT(2)
>>> +#define  PRS_RDY     BIT(4)
>>> +#define  TMP_RDY     BIT(5)
>>> +#define  SENSOR_RDY  BIT(6)
>>> +#define  COEF_RDY    BIT(7)
>>> +#define CFG_REG              0x09
>>> +#define  INT_HL              BIT(7)
>>> +#define  TMP_SHIFT_EN        BIT(3)
>>> +#define  PRS_SHIFT_EN        BIT(4)
>>> +#define  FIFO_EN     BIT(5)
>>> +#define  SPI_EN              BIT(6)
>>> +#define RESET                0x0c
>>> +#define  RESET_MAGIC (BIT(0) | BIT(3))
>>> +#define COEF_BASE    0x10
>>> +
>>> +#define TMP_RATE(_n) ilog2(_n)
>> You define these but then have it long hand in the code?
> 
> I will drop these I think. I've reworked the code a few times and
> these were left over.
> 
>>> +static int dps310_get_temp_k(struct dps310_data *data)
>>> +{
>>> +     int index = ilog2(dps310_get_temp_precision(data));
>>> +
>>> +     return scale_factor[index];
>> I'd just put the whole thing in one line, but up to you.
> 
> I previously had a check here that the range was not out of range but
> I dropped the check as it can't happen.
> 
> I'll put it on one line.
> 
>>> +static bool dps310_is_volatile_reg(struct device *dev, unsigned int reg)
>>> +{
>>> +     switch (reg) {
>>> +     case PRS_BASE ... (PRS_BASE + 2):
>> Bit ugly defining this like this.  IS PRS_BASE + 2 something that has
>> a logical name of its own?
> 
> Agreed. I used to have PRS_B0, PRS_B1, PRS_B2 as we read out the three
> bytes at once they were not required. I'll clean it up.
> 
> 
>>> +
>>> +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;
>>> +
>>> +     /* c0 * 0.5 + c1 * T_raw / kT °C */
>> Whilst interesting why is this comment right here?
> 
> Will move it to the top of the file.
> 
>>> +
>>> +static int dps310_probe(struct i2c_client *client,
>>> +                     const struct i2c_device_id *id)
>>> +{
>>> +     struct dps310_data *data;
>>> +     struct iio_dev *iio;
>>> +     int r;
>>> +
>>> +     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);
>>> +
>> Not that obvious what the next two calls are doing, perhaps comments?
>> (the third one is good)
> 
> Ack.
> 
>>> +     r = regmap_write(data->regmap, TMP_CFG, TMP_EXT | TMP_PRC(1));
>>> +     if (r < 0)
>>> +             return r;
>>> +     r = regmap_write_bits(data->regmap, CFG_REG, TMP_SHIFT_EN, 0);
>>> +     if (r < 0)
>>> +             return r;
>>> +
>>> +     /* Turn on temperature measurement in the background */
>>> +     r = regmap_write_bits(data->regmap, MEAS_CFG, MEAS_CTRL_BITS,
>>> +                     TEMP_EN | BACKGROUND);
>>> +     if (r < 0)
>>> +             return r;
>>> +
>>> +     /*
>>> +      * Calibration coefficients required for reporting temperature.
>>> +      * They are availalbe 40ms after the device has started
>> available
>>> +      */
>>> +     r = dps310_get_temp_coef(data);
>>> +     if (r == -EAGAIN)
>>> +             return -EPROBE_DEFER;
>> Deferred why?  If you need 40ms then sleep for 40ms to be sure it
>> has woken up. Or even better, loop with a sleep.
> 
> I didn't want to delay booting the system for the driver to probe.
Hmm. Guessing parallel probing not an option on your system.
Add a comment to that effect and leave this in.
> 
>>
>> Deferred will only result in it being probed if something changes
>> such as another driver being loaded (which might provide
>> some resource we are waiting for).
> 
> Ok, I misunderstood how it worked. Thanks for clearing that up.
> 
> I will put the loop/sleep in.
> 
>>> +     if (r < 0)
>>> +             return r;
>>> +
>>> +     r = devm_iio_device_register(&client->dev, iio);
>>> +     if (r)
>>> +             return r;
>> With the two lines below removed, just
>> return devm_iio...
>>> +
>>> +     i2c_set_clientdata(client, iio);
>> Used?
> 
> I must admit I was cargo culting. Can you explain under what
> circumstances I would want this?
If you had a remove as you'd use i2c_get_clientdata to get back
a reference to the iio structures.
> 
>>> +
>>> +     dev_info(&client->dev, "%s: sensor '%s'\n", dev_name(&iio->dev),
>>> +                     client->name);
>> Provides little useful information and just spams the log so
>> I would prefer this was dropped.
> 
> I would really like the core to do something like this, for both hwmon
> and iio devices. I spend a lot of time developing and using systems
> that use these drivers, and it's very useful to know the name of the
> driver, the bus it's on, and the fact that it was both compiled in and
> probed.
> 
> Would you take a patch for the core?
No. To my mind it's trivial to look in sysfs.  There is a lot of
feeling in the community that people are way to tempted to put
messages in the log which can be easily established without it.

A few lines of script would get you the same information.

This isn't a time critical element either so it's timing in the
log doesn't tell you anything either.
> 
> Cheers,
> 
> Joel
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>