Re: [PATCH] iio: Add driver for Infineon DPS310
From: Jonathan Cameron
Date: Sun May 14 2017 - 10:52:08 EST
On 12/05/17 07:28, Joel Stanley wrote:
On Fri, May 5, 2017 at 9:13 PM, Peter Meerwald-Stadler
<pmeerw@xxxxxxxxxx> 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@xxxxxxxxx>
---
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@xxxxxxxxx>
+ *
+ * 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html