Re: [PATCH] iio: Add t5403 barometric pressure sensor driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 25/05/14 22:26, Hartmut Knaack wrote:
Peter Meerwald schrieb:
Hello Hartmut,

16-bit pressure and temperature sensor

the chip can do I2C and SPI, only the I2C interface is supported
by the driver at the moment

datasheet: http://www.epcos.com/inf/57/ds/T5400.pdf
application note: http://www.epcos.com/blob/993154/download/1/t5403-applicationnote.pdf

an out-of-tree driver targetting the input subsystem is at
https://github.com/unixphere/t5400, it was rejected here:
http://comments.gmane.org/gmane.linux.kernel.input/28107
thanks for the comments!

replies inline

regards, p.

---
  drivers/iio/pressure/Kconfig  |  10 ++
  drivers/iio/pressure/Makefile |   1 +
  drivers/iio/pressure/t5403.c  | 268 ++++++++++++++++++++++++++++++++++++++++++
  3 files changed, 279 insertions(+)
  create mode 100644 drivers/iio/pressure/t5403.c

diff --git a/drivers/iio/pressure/Kconfig b/drivers/iio/pressure/Kconfig
index ffac8ac..15afbc9 100644
--- a/drivers/iio/pressure/Kconfig
+++ b/drivers/iio/pressure/Kconfig
@@ -70,4 +70,14 @@ config IIO_ST_PRESS_SPI
  	depends on IIO_ST_PRESS
  	depends on IIO_ST_SENSORS_SPI

+config T5403
+	tristate "EPCOS T5403 digital barometric pressure sensor driver"
+	depends on I2C
+	help
+	  Say yes here to build support for the EPCOS T5403 pressure sensor
+	  connected via I2C.
+
+          To compile this driver as a module, choose M here: the module
+          will be called t5403.
+
  endmenu
diff --git a/drivers/iio/pressure/Makefile b/drivers/iio/pressure/Makefile
index c53d250..90a37e8 100644
--- a/drivers/iio/pressure/Makefile
+++ b/drivers/iio/pressure/Makefile
@@ -9,6 +9,7 @@ obj-$(CONFIG_MPL3115) += mpl3115.o
  obj-$(CONFIG_IIO_ST_PRESS) += st_pressure.o
  st_pressure-y := st_pressure_core.o
  st_pressure-$(CONFIG_IIO_BUFFER) += st_pressure_buffer.o
+obj-$(CONFIG_T5403) += t5403.o

  obj-$(CONFIG_IIO_ST_PRESS_I2C) += st_pressure_i2c.o
  obj-$(CONFIG_IIO_ST_PRESS_SPI) += st_pressure_spi.o
diff --git a/drivers/iio/pressure/t5403.c b/drivers/iio/pressure/t5403.c
new file mode 100644
index 0000000..cb01d6d
--- /dev/null
+++ b/drivers/iio/pressure/t5403.c
@@ -0,0 +1,268 @@
+/*
+ * t5403.c - Support for EPCOS T5403 pressure/temperature sensor
+ *
+ * Copyright (c) 2014 Peter Meerwald <pmeerw@xxxxxxxxxx>
+ *
+ * This file is subject to the terms and conditions of version 2 of
+ * the GNU General Public License.  See the file COPYING in the main
+ * directory of this archive for more details.
+ *
+ * (7-bit I2C slave address 0x77)
+ *
+ * TODO: end-of-conversion irq
+ */
+
+#include <linux/module.h>
+#include <linux/i2c.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+#include <linux/delay.h>
+
+#define T5403_DATA 0xf5 /* data, LSB first, 16 bit */
+#define T5403_CALIB_DATA 0x8e /* 10 calibration coeff., LSB first, 16 bit */
+#define T5403_SLAVE_ADDR 0x88 /* I2C slave address, 0x77 */
+#define T5403_COMMAND 0xf1
+
+/* command bits */
+#define T5403_MODE_SHIFT 3 /* conversion time: 2, 8, 16, 66 ms */
+#define T5403_PT BIT(1) /* 0 .. pressure, 1 .. temperature measurement */
+#define T5403_SCO BIT(0) /* start conversion */
+
+#define T5403_MODE_LOW 0
+#define T5403_MODE_STANDARD 1
+#define T5403_MODE_HIGH 1
^That needs to be 2.
obviously :), thanks!

+#define T5403_MODE_ULTRA_HIGH 3
+
+#define T5403_I2C_MASK (~BIT(7))
+#define T5403_I2C_ADDR 0x77
+
+static const int t5403_pressure_conv_ms[] = {2, 8, 16, 66};
+
+struct t5403_data {
+	struct i2c_client *client;
+	struct mutex lock;
+	int mode;
+	__le16 c[10];
+};
+
+#define T5403_C_U16(i) le16_to_cpu(data->c[(i) - 1])
+#define T5403_C(i) sign_extend32(T5403_C_U16(i), 15)
+
+static int t5403_read(struct t5403_data *data, bool pressure)
+{
+	int wait_time = 3;  /* wakeup time in ms */
+
+	int ret = i2c_smbus_write_byte_data(data->client, T5403_COMMAND,
+		(pressure ? (data->mode << T5403_MODE_SHIFT) : T5403_PT) |
+		T5403_SCO);
This is a bit confusing. Although not causing any harm, I would keep sending data->mode unchanged at all time and just set/clear the pt bits as needed.
according to the datasheet/appnote, mode is not relevant for temperature
measurement, so I rather not send it (also following the appnote example)
Fair enough.

+	if (ret < 0)
+		return ret;
+
+	wait_time += pressure ? t5403_pressure_conv_ms[data->mode] : 2;
+
+	msleep(wait_time);
+
+	return i2c_smbus_read_word_data(data->client, T5403_DATA);
+}
+
+static int t5403_comp_pressure(struct t5403_data *data, int *val, int *val2)
+{
+	int ret;
+	s16 t_r;
+	u16 p_r;
+	s32 S, O, X;
+
+	mutex_lock(&data->lock);
+
+	ret = t5403_read(data, false);
+	if (ret < 0)
+		goto done;
+	t_r = ret;
+
+	ret = t5403_read(data, true);
+	if (ret < 0)
+		goto done;
+	p_r = ret;
+
+	/* see EPCOS application note */
What about simplifying those equations, they look to me like this:
S = (c9 * t_r^3)/2^46 + (c5 * t_r^2)/2^34 + (c4 * t_r)/2^17 + c3
O = (c9 * t_r^3)/2^31 + (c8 * t_r^2)/2^19 + (c7 * t_r)/2^3 + c_6 * 2^14
That should barely fit into s64.
I think there is some merit to use the equations as given in the
appnote; I'd rather not 'simplify' using s64
It seems to me, that they distribute their 2^x divisions over the t_r multiplications to avoid an integer overflow. As I pointed out, it seems to be a third order polynomial equation, thus the biggest value could not exceed 3*15 bit for t_r plus 16 bits for c9, plus one sign bit, so 57 bits maximum. I would guess, the people writing the application note mainly had MCUs in mind, which have have very low amounts of RAM compared to the amount of flash/eeprom for storing instruction code - so that wasting 8 bytes on a variable would hurt much more than wasting several bytes on instructions. Yet, machines running Linux have at least several megabytes of RAM, and even hold all instruction code in RAM. Just my 2 cents, I leave it up to you.
I'd go with matching the app note as it doesn't really matter but that
is is easier for any later readers of the code to check against.

+	S = T5403_C_U16(3) + (s32) T5403_C_U16(4) * t_r / 0x20000 +
+		T5403_C(5) * t_r / 0x8000 * t_r / 0x80000 +
+		T5403_C(9) * t_r / 0x8000 * t_r / 0x8000 * t_r / 0x10000;
+
+	O = T5403_C(6) * 0x4000 + T5403_C(7) * t_r / 8 +
+		T5403_C(8) * t_r / 0x8000 * t_r / 16 +
+		T5403_C(9) * t_r / 0x8000 * t_r / 0x10000 * t_r;
+
+	X = (S * p_r + O) / 0x4000;
+
+	X += ((X - 75000) * (X - 75000) / 0x10000 - 9537) *
+	    T5403_C(10) / 0x10000;
+
+	*val = X / 1000;
+	*val2 = (X % 1000) * 1000;
+
+done:
+	mutex_unlock(&data->lock);
+	return ret;
+}
+
+static int t5403_comp_temp(struct t5403_data *data, int *val)
+{
+	int ret;
+	s16 t_r;
+
+	mutex_lock(&data->lock);
+	ret = t5403_read(data, false);
+	if (ret < 0)
+		goto done;
+	t_r = ret;
+
+	/* see EPCOS application note */
+	*val = ((s32) T5403_C_U16(1) * t_r / 0x100 +
+		(s32) T5403_C_U16(2) * 0x40) * 1000 / 0x10000;
+
+done:
+	mutex_unlock(&data->lock);
+	return ret;
+}
+
+static int t5403_read_raw(struct iio_dev *indio_dev,
+			  struct iio_chan_spec const *chan,
+			  int *val, int *val2, long mask)
+{
+	struct t5403_data *data = iio_priv(indio_dev);
+	int ret;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_PROCESSED:
+		switch (chan->type) {
+		case IIO_PRESSURE:
+			ret = t5403_comp_pressure(data, val, val2);
+			if (ret < 0)
+				return ret;
+			return IIO_VAL_INT_PLUS_MICRO;
+		case IIO_TEMP:
+			ret = t5403_comp_temp(data, val);
+			if (ret < 0)
+				return ret;
+			return IIO_VAL_INT;
+		default:
+			return -EINVAL;
+	    }
+	case IIO_CHAN_INFO_INT_TIME:
+		*val = 0;
+		*val2 = t5403_pressure_conv_ms[data->mode] * 1000;
+		return IIO_VAL_INT_PLUS_MICRO;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int t5403_write_raw(struct iio_dev *indio_dev,
+			   struct iio_chan_spec const *chan,
+			   int val, int val2, long mask)
+{
+	struct t5403_data *data = iio_priv(indio_dev);
+	int i;
+
Check for the right mask (IIO_CHAN_INFO_INT_TIME).
there is only one writable mask?
That is supposed to be the case. Yet, the drivers I reviewed so far were always checking the mask in their write_raw-function, and returned -EINVAL on mismatch.

Technically the core doesn't in anyway prevent writing to any info_mask elements.
There is no way of it knowing that only this one is
writable.  Hence the need to check that we have the only one we actually want to be writable.
Short of having twice as many info_mask bitmaps there is no particularly easy way of implementing
this such a check in the core.
(I'm open to suggestions as it is rather ugly as things currently stand!)


+	for (i = 0; i < ARRAY_SIZE(t5403_pressure_conv_ms); i++)
+		if (val == 0 && val2 == t5403_pressure_conv_ms[i] * 1000) {
+			mutex_lock(&data->lock);
+			data->mode = i;
+			mutex_unlock(&data->lock);
+			return 0;
+		}
+	return -EINVAL;
+}
+
+static const struct iio_chan_spec t5403_channels[] = {
+	{
+		.type = IIO_PRESSURE,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) |
+		    BIT(IIO_CHAN_INFO_INT_TIME),
+	},
+	{
+		.type = IIO_TEMP,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
+	},
+};
+
+static IIO_CONST_ATTR_INT_TIME_AVAIL("0.002 0.008 0.016 0.066");
+
+static struct attribute *t5403_attributes[] = {
+	&iio_const_attr_integration_time_available.dev_attr.attr,
+	NULL
+};
+
+static const struct attribute_group t5403_attribute_group = {
+	.attrs = t5403_attributes,
+};
+
+static const struct iio_info t5403_info = {
+	.read_raw = &t5403_read_raw,
+	.write_raw = &t5403_write_raw,
+	.attrs = &t5403_attribute_group,
+	.driver_module = THIS_MODULE,
+};
+
+static int t5403_probe(struct i2c_client *client,
+			 const struct i2c_device_id *id)
+{
+	struct t5403_data *data;
+	struct iio_dev *indio_dev;
+	int ret;
+
+	if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_WORD_DATA |
+	    I2C_FUNC_SMBUS_I2C_BLOCK))
+		return -ENODEV;
+
+	ret = i2c_smbus_read_byte_data(client, T5403_SLAVE_ADDR);
+	if (ret < 0)
+		return ret;
+	if ((ret & T5403_I2C_MASK) != T5403_I2C_ADDR)
+		return -ENODEV;
+
+	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	data = iio_priv(indio_dev);
+	data->client = client;
+	mutex_init(&data->lock);
+
+	i2c_set_clientdata(client, indio_dev);
+	indio_dev->info = &t5403_info;
+	indio_dev->name = id->name;
+	indio_dev->dev.parent = &client->dev;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->channels = t5403_channels;
+	indio_dev->num_channels = ARRAY_SIZE(t5403_channels);
+
+	data->mode = T5403_MODE_STANDARD;
+
+	ret = i2c_smbus_read_i2c_block_data(data->client, T5403_CALIB_DATA,
+	    sizeof(data->c), (u8 *) data->c);
+	if (ret < 0)
+		return ret;
+
+	return devm_iio_device_register(&client->dev, indio_dev);
+}
+
+static const struct i2c_device_id t5403_id[] = {
+	{ "t5403", 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, t5403_id);
+
+static struct i2c_driver t5403_driver = {
+	.driver = {
+		.name	= "t5403",
+	},
+	.probe = t5403_probe,
+	.id_table = t5403_id,
+};
+module_i2c_driver(t5403_driver);
+
+MODULE_AUTHOR("Peter Meerwald <pmeerw@xxxxxxxxxx>");
+MODULE_DESCRIPTION("EPCOS T5403 pressure/temperature sensor driver");
+MODULE_LICENSE("GPL");
--
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


--
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


--
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




[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Input]     [Linux Kernel]     [Linux SCSI]     [X.org]

  Powered by Linux