Prev: hwmon: Driver for SMM665 Six-Channel Active DC Output Controller/Monitor
Next: writeback: Creating /sys/kernel/mm/writeback/writeback
From: Jonathan Cameron on 19 Jun 2010 06:40
On 06/19/10 02:58, Christoph Mair wrote:
> This driver adds support for the BMP085 digital pressure sensor
> from Bosch Sensortec. It exposes a sysfs api to userspace where
> pressure and temperature measurement results can be read from the
> pressure0_input and temp0_input file. The chip is able to calculate
> the average of up to eight samples to increase the accuracy.
> This feature can be controlled by writing to the oversampling file.
>
> Signed-off-by: Christoph Mair <christoph.mair(a)gmail.com>

Hi Chrisoph,

Couple of queries inline. One array that looks twice as big
as it needs to be and a few suggestions about passing errors
up to userspace.
> ---
> drivers/misc/Kconfig | 10 ++
> drivers/misc/Makefile | 1 +
> drivers/misc/bmp085.c | 412 +++++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 423 insertions(+), 0 deletions(-)
> create mode 100644 drivers/misc/bmp085.c
>
> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> index 26386a9..9df8d3e 100644
> --- a/drivers/misc/Kconfig
> +++ b/drivers/misc/Kconfig
> @@ -353,6 +353,16 @@ config VMWARE_BALLOON
> To compile this driver as a module, choose M here: the
> module will be called vmware_balloon.
>
> +config BMP085
> + tristate "BMP085 digital pressure sensor"
> + depends on I2C && SYSFS
> + help
> + If you say yes here you get support for the Bosch Sensortec
> + BMP086 digital pressure sensor.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called bmp085.
> +
> source "drivers/misc/c2port/Kconfig"
> source "drivers/misc/eeprom/Kconfig"
> source "drivers/misc/cb710/Kconfig"
> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
> index 6ed06a1..5623c54 100644
> --- a/drivers/misc/Makefile
> +++ b/drivers/misc/Makefile
> @@ -31,3 +31,4 @@ obj-$(CONFIG_IWMC3200TOP) += iwmc3200top/
> obj-y += eeprom/
> obj-y += cb710/
> obj-$(CONFIG_VMWARE_BALLOON) += vmware_balloon.o
> +obj-$(CONFIG_BMP085) += bmp085.o
> diff --git a/drivers/misc/bmp085.c b/drivers/misc/bmp085.c
> new file mode 100644
> index 0000000..e7ee91e
> --- /dev/null
> +++ b/drivers/misc/bmp085.c
> @@ -0,0 +1,412 @@
> +/* Copyright (c) 2010 Christoph Mair <christoph.mair(a)gmail.com>
> +
> + 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.
> +
> + This program is distributed in the hope that it will be useful,
> + but WITHOUT ANY WARRANTY; without even the implied warranty of
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + GNU General Public License for more details.
> +
> + You should have received a copy of the GNU General Public License
> + along with this program; if not, write to the Free Software
> + Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
> +*/
> +
> +
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/i2c.h>
> +#include <linux/slab.h>
> +#include <linux/delay.h>
> +
> +
> +#define BMP085_I2C_ADDRESS 0x77
> +#define BMP085_CHIP_ID 0x55
> +
> +#define BMP085_CALIBRATION_DATA_START 0xAA
> +#define BMP085_CALIBRATION_DATA_LENGTH 22
> +#define BMP085_CHIP_ID_REG 0xD0
> +#define BMP085_VERSION_REG 0xD1
> +#define BMP085_CTRL_REG 0xF4
> +#define BMP085_TEMP_MEASUREMENT 0x2E
> +#define BMP085_PRESSURE_MEASUREMENT 0x34
> +#define BMP085_CONVERSION_REGISTER_MSB 0xF6
> +#define BMP085_CONVERSION_REGISTER_LSB 0xF7
> +#define BMP085_CONVERSION_REGISTER_XLSB 0xF8
> +#define BMP085_TEMP_CONVERSION_TIME 5
> +
> +#define BMP085_MIN_TEMP -400 /* tenth of a degree celsius */
> +#define BMP085_MAX_TEMP 850
> +#define BMP085_MIN_PRESSURE 30000 /* pascal (millibar) */
> +#define BMP085_MAX_PRESSURE 110000
> +#define BMP085_INPUT_POLL_INTERVAL 1000 /* ms */
> +
> +#define BMP085_CLIENT_NAME "bmp085"
> +
> +
> +static const unsigned short normal_i2c[] = { BMP085_I2C_ADDRESS,
> + I2C_CLIENT_END };
> +
> +struct bmp085_calibration_data {
> + s16 AC1, AC2, AC3;
> + u16 AC4, AC5, AC6;
> + s16 B1, B2;
> + s16 MB, MC, MD;
> +};
> +
> +
> +/* Each client has this additional data */
> +struct bmp085_data {
> + struct i2c_client *client;
> + struct mutex lock;
> + struct bmp085_calibration_data calibration;
> + u32 raw_temperature;
> + u32 raw_pressure;
> + unsigned char oversampling_setting;
> + unsigned long last_temp_measurement;
> + s32 b6; /* calculated temperature correction coefficient */
> +};
> +
> +
Why not simply reorder the code to have the init function defined before the probe?

> +static void bmp085_init_client(struct i2c_client *client);
> +
> +
> +static s32 bmp085_read_calibration_data(struct i2c_client *client)
> +{
Why is this so big? You only ever use the first 11 of the 22 elements.
I guess the data type of this changed at some point???

> + u16 tmp[BMP085_CALIBRATION_DATA_LENGTH];
> + struct bmp085_data *data = i2c_get_clientdata(client);
> + struct bmp085_calibration_data *cali = &(data->calibration);
> + s32 status = i2c_smbus_read_i2c_block_data(client,
> + BMP085_CALIBRATION_DATA_START,
> + BMP085_CALIBRATION_DATA_LENGTH, (u8 *)tmp);
I guess an error here probably means all hell has broken loose anyway...
However it would be more conventional to handle any error before trying
to use the data.
> +
> + cali->AC1 = be16_to_cpu(tmp[0]);
> + cali->AC2 = be16_to_cpu(tmp[1]);
> + cali->AC3 = be16_to_cpu(tmp[2]);
> + cali->AC4 = be16_to_cpu(tmp[3]);
> + cali->AC5 = be16_to_cpu(tmp[4]);
> + cali->AC6 = be16_to_cpu(tmp[5]);
> + cali->B1 = be16_to_cpu(tmp[6]);
> + cali->B2 = be16_to_cpu(tmp[7]);
> + cali->MB = be16_to_cpu(tmp[8]);
> + cali->MC = be16_to_cpu(tmp[9]);
> + cali->MD = be16_to_cpu(tmp[10]);
> + return status;
> +}
> +
> +
> +static s32 bmp085_update_raw_temperature(struct bmp085_data *data)
> +{
> + u16 tmp;
> + s32 status;
> +
> + mutex_lock(&data->lock);
> + status = i2c_smbus_write_byte_data(data->client, BMP085_CTRL_REG,
> + BMP085_TEMP_MEASUREMENT);
> + if (status != 0) {
Splitting error messages like this is frowned up as it makes it difficult
to grep the code for where a given message came from. If other reorganizations
don't make checkpatch happy then ignore line length warnings if the alternative
is to do this.
> + dev_err(&data->client->dev, "Error while requesting"
> + " temperature measurement.\n");
> + goto exit;
> + }
> + msleep(BMP085_TEMP_CONVERSION_TIME);
> +
> + status = i2c_smbus_read_i2c_block_data(data->client,
> + BMP085_CONVERSION_REGISTER_MSB, sizeof(tmp), (u8 *)&tmp);
> + if (status != sizeof(tmp)) {
> + dev_err(&data->client->dev, "Error while requesting"
> + " temperature measurement (II): %d\n", status);
> + goto exit;
> + }
> + data->raw_temperature = be16_to_cpu(tmp);
> + data->last_temp_measurement = jiffies;
> +
> +exit:
> + mutex_unlock(&data->lock);
> + return status;
> +}
> +
> +static s32 bmp085_update_raw_pressure(struct bmp085_data *data)
> +{
> + u32 tmp = 0;
> + s32 status;
> +
> + mutex_lock(&data->lock);
> + status = i2c_smbus_write_byte_data(data->client, BMP085_CTRL_REG,
> + BMP085_PRESSURE_MEASUREMENT + (data->oversampling_setting<<6));
> + if (status != 0) {
> + dev_err(&data->client->dev, "Error while requesting"
> + "pressure measurement.\n");
> + goto exit;
> + }
> +
> + /* wait for the end of conversion */
> + msleep(2+(3 << data->oversampling_setting<<1));
> +
> + /* copy data into a u32 (4 bytes), but leave skip the first byte. */
> + status = i2c_smbus_read_i2c_block_data(
> + data->client, 0xF6, 3, ((u8 *)&tmp)+1);
> + if (status != 3) {
> + dev_err(&data->client->dev, "Error while reading"
> + "pressure measurement results: %d\n", status);
> + goto exit;
> + }
> + data->raw_pressure = be32_to_cpu((tmp));
> + data->raw_pressure >>= (8-data->oversampling_setting);
> +
> +exit:
> + mutex_unlock(&data->lock);
> + return status;
> +}
> +
> +
> +/*
> + * This function starts the temperature measurement and returns the value
> + * in tenth of a degree celsius.
> + */
> +static s32 bmp085_get_temperature(struct bmp085_data *data)
> +{
> + struct bmp085_calibration_data *cali = &data->calibration;
> + long x1, x2;
> +
Obviously it will make life more complex, but it would be good to see any
error returned by this function handled properly. If you get an error the
ideal is to pass it all the way up to userspace.
> + bmp085_update_raw_temperature(data);
> +
> + x1 = ((data->raw_temperature - cali->AC6) * cali->AC5) >> 15;
> + x2 = (cali->MC << 11) / (x1 + cali->MD);
> + data->b6 = x1 + x2 - 4000;
> + return (x1+x2+8) >> 4;
> +}
> +
> +/*
> + * This function starts the pressure measurement and returns the value
> + * in millibar. Since the pressure depends on the ambient temperature,
> + * a temperature measurement is executed if the last known value is older
> + * than one second.
> + */
> +static s32 bmp085_get_pressure(struct bmp085_data *data)
> +{
> + struct bmp085_calibration_data *cali = &data->calibration;
> + s32 x1, x2, x3, b3;
> + u32 b4, b7;
> + s32 p;
> +
> + /* alt least every second force an update of the ambient temperature */
> + if (data->last_temp_measurement + 1*HZ < jiffies)
> + bmp085_get_temperature(data);
> +
Same here wrt to error handling.
> + bmp085_update_raw_pressure(data);
> +
> + x1 = (data->b6 * data->b6) >> 12;
> + x1 *= cali->B2;
> + x1 >>= 11;
> +
> + x2 = cali->AC2 * data->b6;
> + x2 >>= 11;
> +
> + x3 = x1 + x2;
> +
> + b3 = (((((s32)cali->AC1) * 4 + x3) << data->oversampling_setting) + 2);
> + b3 >>= 2;
> +
> + x1 = (cali->AC3 * data->b6) >> 13;
> + x2 = (cali->B1 * ((data->b6 * data->b6) >> 12)) >> 16;
> + x3 = (x1 + x2 + 2) >> 2;
> + b4 = (cali->AC4 * (u32)(x3 + 32768)) >> 15;
> +
> + b7 = ((u32)data->raw_pressure - b3) *
> + (50000 >> data->oversampling_setting);
> + p = ((b7 < 0x80000000) ? ((b7 << 1) / b4) : ((b7 / b4) * 2));
> +
> + x1 = p >> 8;
> + x1 *= x1;
> + x1 = (x1 * 3038) >> 16;
> + x2 = (-7357 * p) >> 16;
> + p += (x1 + x2 + 3791) >> 4;
> + return p;
> +}
> +
> +/*
> + * This function sets the chip-internal oversampling. Valid values are 0..3.
> + * The chip will use 2^oversampling samples for interlan averaging.
> + * This influences the measurement time and the accuracy; larger values
> + * increase both. The datasheet gives on overview on how measurement time,
> + * accuracy and noise correlate.
> + */

Given this is only used in the set_oversampling sysfs funciton below, I'd personally
just roll this code in there. Might be preferable to return -EINVAL from the sysfs
function rather than rounding down to a valid value.

> +static void bmp085_set_oversampling(struct bmp085_data *data,
> + unsigned char oversampling)
> +{
> + if (oversampling > 3)
> + oversampling = 3;
> + data->oversampling_setting = oversampling;
> +}
> +
> +/*
> + * Returns the currently selected oversampling. Range: 0..3
> + */
> +static unsigned char bmp085_get_oversampling(struct bmp085_data *data)
> +{
> + return data->oversampling_setting;
> +}
> +
> +/* sysfs callbacks */
> +static ssize_t set_oversampling(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + struct i2c_client *client = to_i2c_client(dev);
> + struct bmp085_data *data = i2c_get_clientdata(client);
> + unsigned long oversampling;
> + int success = strict_strtoul(buf, 10, &oversampling);
> + if (success == 0) {
> + bmp085_set_oversampling(data, oversampling);
> + return count;
> + }
> + return success;
> +}
> +
> +static ssize_t show_oversampling(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct i2c_client *client = to_i2c_client(dev);
> + struct bmp085_data *data = i2c_get_clientdata(client);
> + return sprintf(buf, "%u\n", bmp085_get_oversampling(data));
> +}
> +static DEVICE_ATTR(oversampling, S_IWUSR | S_IRUGO,
> + show_oversampling, set_oversampling);
> +
> +
> +static ssize_t show_temperature(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct i2c_client *client = to_i2c_client(dev);
> + struct bmp085_data *data = i2c_get_clientdata(client);
> + return sprintf(buf, "%d\n", bmp085_get_temperature(data));
> +}
> +static DEVICE_ATTR(temp0_input, S_IRUGO, show_temperature, NULL);
> +
> +
> +static ssize_t show_pressure(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct i2c_client *client = to_i2c_client(dev);
> + struct bmp085_data *data = i2c_get_clientdata(client);
For the error handling comment above, you would want to return the relevant
error here rather than just eating it as currently occurs and returning
garbage.
> + return sprintf(buf, "%d\n", bmp085_get_pressure(data));
> +}
> +static DEVICE_ATTR(pressure0_input, S_IRUGO, show_pressure, NULL);
> +
> +
> +static struct attribute *bmp085_attributes[] = {
> + &dev_attr_temp0_input.attr,
> + &dev_attr_pressure0_input.attr,
> + &dev_attr_oversampling.attr,
> + NULL
> +};
> +
> +static const struct attribute_group bmp085_attr_group = {
> + .attrs = bmp085_attributes,
> +};
> +
> +static int bmp085_detect(struct i2c_client *client, struct i2c_board_info *info)
> +{
> + if (client->addr != BMP085_I2C_ADDRESS)
> + return -ENODEV;
> +
> + if (i2c_smbus_read_byte_data(client, BMP085_CHIP_ID_REG) != BMP085_CHIP_ID)
> + return -ENODEV;
> +
> + return 0;
> +}
> +
> +static int bmp085_probe(struct i2c_client *client,
> + const struct i2c_device_id *id)
> +{
> + struct bmp085_data *data;
> + int err = 0;
> +
> + data = kzalloc(sizeof(struct bmp085_data), GFP_KERNEL);
> + if (!data) {
> + err = -ENOMEM;
> + goto exit;
> + }
> +
> + /* default settings after POR */
> + data->oversampling_setting = 0x00;
> +
> + i2c_set_clientdata(client, data);
> +
> + /* Initialize the BMP085 chip */
> + bmp085_init_client(client);
> +
> + /* Register sysfs hooks */
> + err = sysfs_create_group(&client->dev.kobj, &bmp085_attr_group);
> + if (err)
> + goto exit_free;
> +
> + dev_info(&data->client->dev, "succesfully initialized bmp085!\n");
> + goto exit;
> +
> +exit_free:
> + kfree(data);
> +exit:
> + return err;
> +}
> +
> +static int bmp085_remove(struct i2c_client *client)
> +{
> + sysfs_remove_group(&client->dev.kobj, &bmp085_attr_group);
> + kfree(i2c_get_clientdata(client));
> + return 0;
> +}
> +
> +/* Called when we have found a new HMC5843. */
> +static void bmp085_init_client(struct i2c_client *client)
> +{
> + unsigned char version;
> + struct bmp085_data *data = i2c_get_clientdata(client);
> + data->client = client;
> + bmp085_read_calibration_data(client);
> + version = i2c_smbus_read_byte_data(client, BMP085_VERSION_REG);
> + data->last_temp_measurement = 0;
> + data->oversampling_setting = 3;
> + mutex_init(&data->lock);
> + dev_info(&data->client->dev, "BMP085 ver. %d.%d initialized\n",
> + (version & 0x0F), (version & 0xF0) >> 4);
> +}
> +
> +static const struct i2c_device_id bmp085_id[] = {
> + { "bmp085", 0 },
> + { }
> +};
> +
> +static struct i2c_driver bmp085_driver = {
> + .driver = {
> + .owner = THIS_MODULE,
> + .name = "bmp085"
> + },
> + .id_table = bmp085_id,
> + .probe = bmp085_probe,
> + .remove = bmp085_remove,
> +
> + .detect = bmp085_detect,
> + .address_list = normal_i2c
> +};
> +
> +static int __init bmp085_init(void)
> +{
> + return i2c_add_driver(&bmp085_driver);
> +}
> +
> +static void __exit bmp085_exit(void)
> +{
> + i2c_del_driver(&bmp085_driver);
> +}
> +
> +
> +MODULE_AUTHOR("Christoph Mair <christoph.mair(a)gmail.com");
> +MODULE_DESCRIPTION("BMP085 driver");
> +MODULE_LICENSE("GPL");
> +
> +module_init(bmp085_init);
> +module_exit(bmp085_exit);

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Jonathan Cameron on 20 Jun 2010 07:10
On 06/20/10 01:27, Christoph Mair wrote:
> This driver adds support for the BMP085 digital pressure sensor
> from Bosch Sensortec. It exposes a sysfs api to userspace where
> pressure and temperature measurement results can be read from the
> pressure0_input and temp0_input file. The chip is able to calculate
> the average of up to eight samples to increase the accuracy.
> This feature can be controlled by writing to the oversampling file.
Hi Christoph.

Couple more comments below. One is a confusing comment in the code
that I missed before. The others are again to do with error handling.

With the changes you made here any error results in sysfs reads returning
-EIO. That is correct if not enough bytes are transmitted. However,
if something else goes wrong the i2c core may return an error code
and if it does, it is these error codes that should be passed on up to
user space. (so basically pass on any negative return values).

Fix these last few bits and I'm happy and you can add
Acked-by: Jonathan Cameron <jic23(a)cam.ac.uk>

Nice clean little driver.

>
> Signed-off-by: Christoph Mair <christoph.mair(a)gmail.com>
> ---
> drivers/misc/Kconfig | 10 +
> drivers/misc/Makefile | 1 +
> drivers/misc/bmp085.c | 447 +++++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 458 insertions(+), 0 deletions(-)
> create mode 100644 drivers/misc/bmp085.c
>
> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> index 26386a9..9df8d3e 100644
> --- a/drivers/misc/Kconfig
> +++ b/drivers/misc/Kconfig
> @@ -353,6 +353,16 @@ config VMWARE_BALLOON
> To compile this driver as a module, choose M here: the
> module will be called vmware_balloon.
>
> +config BMP085
> + tristate "BMP085 digital pressure sensor"
> + depends on I2C && SYSFS
> + help
> + If you say yes here you get support for the Bosch Sensortec
> + BMP086 digital pressure sensor.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called bmp085.
> +
> source "drivers/misc/c2port/Kconfig"
> source "drivers/misc/eeprom/Kconfig"
> source "drivers/misc/cb710/Kconfig"
> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
> index 6ed06a1..5623c54 100644
> --- a/drivers/misc/Makefile
> +++ b/drivers/misc/Makefile
> @@ -31,3 +31,4 @@ obj-$(CONFIG_IWMC3200TOP) += iwmc3200top/
> obj-y += eeprom/
> obj-y += cb710/
> obj-$(CONFIG_VMWARE_BALLOON) += vmware_balloon.o
> +obj-$(CONFIG_BMP085) += bmp085.o
> diff --git a/drivers/misc/bmp085.c b/drivers/misc/bmp085.c
> new file mode 100644
> index 0000000..0404a6c
> --- /dev/null
> +++ b/drivers/misc/bmp085.c
> @@ -0,0 +1,447 @@
> +/* Copyright (c) 2010 Christoph Mair <christoph.mair(a)gmail.com>
> +
> + 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.
> +
> + This program is distributed in the hope that it will be useful,
> + but WITHOUT ANY WARRANTY; without even the implied warranty of
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + GNU General Public License for more details.
> +
> + You should have received a copy of the GNU General Public License
> + along with this program; if not, write to the Free Software
> + Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
> +*/
> +
> +
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/i2c.h>
> +#include <linux/slab.h>
> +#include <linux/delay.h>
> +
> +
> +#define BMP085_I2C_ADDRESS 0x77
> +#define BMP085_CHIP_ID 0x55
> +
> +#define BMP085_CALIBRATION_DATA_START 0xAA
> +#define BMP085_CALIBRATION_DATA_LENGTH 11 /* 16 bit values */
> +#define BMP085_CHIP_ID_REG 0xD0
> +#define BMP085_VERSION_REG 0xD1
> +#define BMP085_CTRL_REG 0xF4
> +#define BMP085_TEMP_MEASUREMENT 0x2E
> +#define BMP085_PRESSURE_MEASUREMENT 0x34
> +#define BMP085_CONVERSION_REGISTER_MSB 0xF6
> +#define BMP085_CONVERSION_REGISTER_LSB 0xF7
> +#define BMP085_CONVERSION_REGISTER_XLSB 0xF8
> +#define BMP085_TEMP_CONVERSION_TIME 5
> +
> +#define BMP085_CLIENT_NAME "bmp085"
> +
> +
> +static const unsigned short normal_i2c[] = { BMP085_I2C_ADDRESS,
> + I2C_CLIENT_END };
> +
> +struct bmp085_calibration_data {
> + s16 AC1, AC2, AC3;
> + u16 AC4, AC5, AC6;
> + s16 B1, B2;
> + s16 MB, MC, MD;
> +};
> +
> +
> +/* Each client has this additional data */
> +struct bmp085_data {
> + struct i2c_client *client;
> + struct mutex lock;
> + struct bmp085_calibration_data calibration;
> + u32 raw_temperature;
> + u32 raw_pressure;
> + unsigned char oversampling_setting;
> + unsigned long last_temp_measurement;
> + s32 b6; /* calculated temperature correction coefficient */
> +};
> +
> +
> +static s32 bmp085_read_calibration_data(struct i2c_client *client)
> +{
> + u16 tmp[BMP085_CALIBRATION_DATA_LENGTH];
> + struct bmp085_data *data = i2c_get_clientdata(client);
> + struct bmp085_calibration_data *cali = &(data->calibration);
> + s32 status = i2c_smbus_read_i2c_block_data(client,
> + BMP085_CALIBRATION_DATA_START,
> + BMP085_CALIBRATION_DATA_LENGTH*sizeof(u16),
> + (u8 *)tmp);
> + if (status != BMP085_CALIBRATION_DATA_LENGTH*sizeof(u16))
> + return -EIO;
Conventionally return -EIO only if the length is wrong but no error send). If you get an
error code (i.e. negative then return that directly).
> +
> + cali->AC1 = be16_to_cpu(tmp[0]);
> + cali->AC2 = be16_to_cpu(tmp[1]);
> + cali->AC3 = be16_to_cpu(tmp[2]);
> + cali->AC4 = be16_to_cpu(tmp[3]);
> + cali->AC5 = be16_to_cpu(tmp[4]);
> + cali->AC6 = be16_to_cpu(tmp[5]);
> + cali->B1 = be16_to_cpu(tmp[6]);
> + cali->B2 = be16_to_cpu(tmp[7]);
> + cali->MB = be16_to_cpu(tmp[8]);
> + cali->MC = be16_to_cpu(tmp[9]);
> + cali->MD = be16_to_cpu(tmp[10]);
> + return 0;
> +}
> +
> +
> +static s32 bmp085_update_raw_temperature(struct bmp085_data *data)
> +{
> + u16 tmp;
> + s32 status;
> +
> + mutex_lock(&data->lock);
> + status = i2c_smbus_write_byte_data(data->client, BMP085_CTRL_REG,
> + BMP085_TEMP_MEASUREMENT);
> + if (status != 0) {
> + dev_err(&data->client->dev,
> + "Error while requesting temperature measurement.\n");
> + goto exit;
> + }
> + msleep(BMP085_TEMP_CONVERSION_TIME);
> +
> + status = i2c_smbus_read_i2c_block_data(data->client,
> + BMP085_CONVERSION_REGISTER_MSB, sizeof(tmp), (u8 *)&tmp);
> + if (status != sizeof(tmp)) {
> + dev_err(&data->client->dev,
> + "Error while reading temperature measurement result\n");
Here you probably do want to add a check for a positive but incorrect
value and set status to -EIO if you get one.
> + goto exit;
> + }
> + data->raw_temperature = be16_to_cpu(tmp);
> + data->last_temp_measurement = jiffies;
> + status = 0; /* everything ok, return 0 */
> +
> +exit:
> + mutex_unlock(&data->lock);
> + return status;
> +}
> +
> +static s32 bmp085_update_raw_pressure(struct bmp085_data *data)
> +{
> + u32 tmp = 0;
> + s32 status;
> +
> + mutex_lock(&data->lock);
> + status = i2c_smbus_write_byte_data(data->client, BMP085_CTRL_REG,
> + BMP085_PRESSURE_MEASUREMENT + (data->oversampling_setting<<6));
> + if (status != 0) {
> + dev_err(&data->client->dev,
> + "Error while requesting pressure measurement.\n");
> + status = -EIO;
Again, don't eat errors if they are coming up from the i2c core.
> + goto exit;
> + }
> +
> + /* wait for the end of conversion */
> + msleep(2+(3 << data->oversampling_setting<<1));
> +
> + /* copy data into a u32 (4 bytes), but skip the first byte. */
> + status = i2c_smbus_read_i2c_block_data(data->client,
> + BMP085_CONVERSION_REGISTER_MSB, 3, ((u8 *)&tmp)+1);
> + if (status != 3) {
> + dev_err(&data->client->dev,
> + "Error while reading pressure measurement results\n");
> + status = -EIO;
> + goto exit;
> + }
> + data->raw_pressure = be32_to_cpu((tmp));
> + data->raw_pressure >>= (8-data->oversampling_setting);
> + status = 0; /* everything ok, return 0 */
> +
> +exit:
> + mutex_unlock(&data->lock);
> + return status;
> +}
> +
> +
> +/*
> + * This function starts the temperature measurement and returns the value
> + * in tenth of a degree celsius.
> + */
> +static s32 bmp085_get_temperature(struct bmp085_data *data, int *temperature)
> +{
> + struct bmp085_calibration_data *cali = &data->calibration;
> + long x1, x2;
> + int status;
> +
> + status = bmp085_update_raw_temperature(data);
> + if (status != 0)
> + goto exit;
> +
> + x1 = ((data->raw_temperature - cali->AC6) * cali->AC5) >> 15;
> + x2 = (cali->MC << 11) / (x1 + cali->MD);
> + data->b6 = x1 + x2 - 4000;
> + if (temperature != NULL) /* if NULL just update b6 */
> + *temperature = (x1+x2+8) >> 4; /* used if temp is not required */
The comment above looks reversed to me? Or is it an extension of the one above?
> +
> +exit:
> + return status;;
> +}
> +
> +/*
> + * This function starts the pressure measurement and returns the value
> + * in millibar. Since the pressure depends on the ambient temperature,
> + * a temperature measurement is executed if the last known value is older
> + * than one second.
> + */
> +static s32 bmp085_get_pressure(struct bmp085_data *data, int *pressure)
> +{
> + struct bmp085_calibration_data *cali = &data->calibration;
> + s32 x1, x2, x3, b3;
> + u32 b4, b7;
> + s32 p;
> + int status;
> +
> + /* alt least every second force an update of the ambient temperature */
> + if (data->last_temp_measurement + 1*HZ < jiffies) {
> + status = bmp085_get_temperature(data, NULL);
> + if (status != 0)
> + goto exit;
> + }
> +
> + status = bmp085_update_raw_pressure(data);
> + if (status != 0)
> + goto exit;
> +
> + x1 = (data->b6 * data->b6) >> 12;
> + x1 *= cali->B2;
> + x1 >>= 11;
> +
> + x2 = cali->AC2 * data->b6;
> + x2 >>= 11;
> +
> + x3 = x1 + x2;
> +
> + b3 = (((((s32)cali->AC1) * 4 + x3) << data->oversampling_setting) + 2);
> + b3 >>= 2;
> +
> + x1 = (cali->AC3 * data->b6) >> 13;
> + x2 = (cali->B1 * ((data->b6 * data->b6) >> 12)) >> 16;
> + x3 = (x1 + x2 + 2) >> 2;
> + b4 = (cali->AC4 * (u32)(x3 + 32768)) >> 15;
> +
> + b7 = ((u32)data->raw_pressure - b3) *
> + (50000 >> data->oversampling_setting);
> + p = ((b7 < 0x80000000) ? ((b7 << 1) / b4) : ((b7 / b4) * 2));
> +
> + x1 = p >> 8;
> + x1 *= x1;
> + x1 = (x1 * 3038) >> 16;
> + x2 = (-7357 * p) >> 16;
> + p += (x1 + x2 + 3791) >> 4;
> +
> + *pressure = p;
> +
> +exit:
> + return status;
> +}
> +
> +/*
> + * This function sets the chip-internal oversampling. Valid values are 0..3.
> + * The chip will use 2^oversampling samples for interlan averaging.
> + * This influences the measurement time and the accuracy; larger values
> + * increase both. The datasheet gives on overview on how measurement time,
> + * accuracy and noise correlate.
> + */
> +static void bmp085_set_oversampling(struct bmp085_data *data,
> + unsigned char oversampling)
> +{
> + if (oversampling > 3)
> + oversampling = 3;
> + data->oversampling_setting = oversampling;
> +}
> +
> +/*
> + * Returns the currently selected oversampling. Range: 0..3
> + */
> +static unsigned char bmp085_get_oversampling(struct bmp085_data *data)
> +{
> + return data->oversampling_setting;
> +}
> +
> +/* sysfs callbacks */
> +static ssize_t set_oversampling(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + struct i2c_client *client = to_i2c_client(dev);
> + struct bmp085_data *data = i2c_get_clientdata(client);
> + unsigned long oversampling;
> + int success = strict_strtoul(buf, 10, &oversampling);
> + if (success == 0) {
> + bmp085_set_oversampling(data, oversampling);
> + return count;
> + }
> + return success;
> +}
> +
> +static ssize_t show_oversampling(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct i2c_client *client = to_i2c_client(dev);
> + struct bmp085_data *data = i2c_get_clientdata(client);
> + return sprintf(buf, "%u\n", bmp085_get_oversampling(data));
> +}
> +static DEVICE_ATTR(oversampling, S_IWUSR | S_IRUGO,
> + show_oversampling, set_oversampling);
> +
> +
> +static ssize_t show_temperature(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + int temperature;
> + int status;
> + struct i2c_client *client = to_i2c_client(dev);
> + struct bmp085_data *data = i2c_get_clientdata(client);
> +
> + status = bmp085_get_temperature(data, &temperature);
> + if (status != 0)
> + return status;
> + else
> + return sprintf(buf, "%d\n", temperature);
> +}
> +static DEVICE_ATTR(temp0_input, S_IRUGO, show_temperature, NULL);
> +
> +
> +static ssize_t show_pressure(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + int pressure;
> + int status;
> + struct i2c_client *client = to_i2c_client(dev);
> + struct bmp085_data *data = i2c_get_clientdata(client);
> +
> + status = bmp085_get_pressure(data, &pressure);
> + if (status != 0)
> + return status;
> + else
> + return sprintf(buf, "%d\n", pressure);
> +}
> +static DEVICE_ATTR(pressure0_input, S_IRUGO, show_pressure, NULL);
> +
> +
> +static struct attribute *bmp085_attributes[] = {
> + &dev_attr_temp0_input.attr,
> + &dev_attr_pressure0_input.attr,
> + &dev_attr_oversampling.attr,
> + NULL
> +};
> +
> +static const struct attribute_group bmp085_attr_group = {
> + .attrs = bmp085_attributes,
> +};
> +
> +static int bmp085_detect(struct i2c_client *client, struct i2c_board_info *info)
> +{
> + if (client->addr != BMP085_I2C_ADDRESS)
> + return -ENODEV;
> +
> + if (i2c_smbus_read_byte_data(client, BMP085_CHIP_ID_REG) != BMP085_CHIP_ID)
> + return -ENODEV;
> +
> + return 0;
> +}
> +
> +static int bmp085_init_client(struct i2c_client *client)
> +{
> + unsigned char version;
> + int status;
> + struct bmp085_data *data = i2c_get_clientdata(client);
> + data->client = client;
> + status = bmp085_read_calibration_data(client);
> + if (status != 0)
> + goto exit;
> + version = i2c_smbus_read_byte_data(client, BMP085_VERSION_REG);
> + data->last_temp_measurement = 0;
> + data->oversampling_setting = 3;
> + mutex_init(&data->lock);
> + dev_info(&data->client->dev, "BMP085 ver. %d.%d found.\n",
> + (version & 0x0F), (version & 0xF0) >> 4);
> +exit:
> + return status;
> +}
> +
> +static int bmp085_probe(struct i2c_client *client,
> + const struct i2c_device_id *id)
> +{
> + struct bmp085_data *data;
> + int err = 0;
> +
> + data = kzalloc(sizeof(struct bmp085_data), GFP_KERNEL);
> + if (!data) {
> + err = -ENOMEM;
> + goto exit;
> + }
> +
> + /* default settings after POR */
> + data->oversampling_setting = 0x00;
> +
> + i2c_set_clientdata(client, data);
> +
> + /* Initialize the BMP085 chip */
> + err = bmp085_init_client(client);
> + if (err != 0)
> + goto exit_free;
> +
> + /* Register sysfs hooks */
> + err = sysfs_create_group(&client->dev.kobj, &bmp085_attr_group);
> + if (err)
> + goto exit_free;
> +
> + dev_info(&data->client->dev, "Succesfully initialized bmp085!\n");
> + goto exit;
> +
> +exit_free:
> + kfree(data);
> +exit:
> + return err;
> +}
> +
> +static int bmp085_remove(struct i2c_client *client)
> +{
> + sysfs_remove_group(&client->dev.kobj, &bmp085_attr_group);
> + kfree(i2c_get_clientdata(client));
> + return 0;
> +}
> +
> +static const struct i2c_device_id bmp085_id[] = {
> + { "bmp085", 0 },
> + { }
> +};
> +
> +static struct i2c_driver bmp085_driver = {
> + .driver = {
> + .owner = THIS_MODULE,
> + .name = "bmp085"
> + },
> + .id_table = bmp085_id,
> + .probe = bmp085_probe,
> + .remove = bmp085_remove,
> +
> + .detect = bmp085_detect,
> + .address_list = normal_i2c
> +};
> +
> +static int __init bmp085_init(void)
> +{
> + return i2c_add_driver(&bmp085_driver);
> +}
> +
> +static void __exit bmp085_exit(void)
> +{
> + i2c_del_driver(&bmp085_driver);
> +}
> +
> +
> +MODULE_AUTHOR("Christoph Mair <christoph.mair(a)gmail.com");
> +MODULE_DESCRIPTION("BMP085 driver");
> +MODULE_LICENSE("GPL");
> +
> +module_init(bmp085_init);
> +module_exit(bmp085_exit);

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Christoph Mair on 22 Jun 2010 19:30
Am Montag 21 Juni 2010, 23:49:58 schrieben Sie:
> On Sun, 20 Jun 2010 22:50:58 +0200
>
> Christoph Mair <christoph.mair(a)gmail.com> wrote:
> > This driver adds support for the BMP085 digital pressure sensor
> > from Bosch Sensortec.
>
> If there's a datasheet available, it would be nice to provide a
> reference to that in the driver.
I will send a patch to fix this.

> Please do send a full description of the proposed API so we can review
> that. What are the contents of these files? What are the units of
> those contents, etc?
There is a short description before the methods bmp085_get_temperature() and
bmp085_get_pressure(), but anyway.

Here is a Description of the current sysfs user interface:

The BMP085 digital pressure sensor can measure ambient air pressure and
temperature. Both values can be obtained from sysfs files. The pressure is
measured by reading from pressure0_input. Valid values range from 30000 to
110000 pascal with a resolution of 1 pascal (=0.01 millibar).

temp0_input holds the current temperature in degree celsius, multiplied by 10.
This results in a resolution of a tenth degree celsius. Values range from -400
to 850.

To increase the accuracy, this chip can calculate the average of 1, 2, 4 or 8
samples. This behavior is controlled through the oversampling sysfs file. Two
to the power of the value written to that file specifies how many samples will
be used. Valid values: 0..3.


> Are there any similar drivers in the tree (I don't think so) and if so
> does this new driver offer the same interface?
I did not find one.

> Bear in mind that if new drivers for similar devices _do_ come along
> then we'd prefer that those drivers implement the same interface as
> this one. So is this driver's interface well-designed from that point
> of view?
The pressure0_input and temp0_input should follow the naming convention of
hwmon devices. I think that would be ok for other sensors too. The
oversampling is special to this device and others may not support this.

A more general problem is the obtainable resolution. I work on a humidity
sensor driver for the SHT21. It measures temperatures with a resolution of
0.01 celsius, ten times higher than the bmp085.
Maybe we need an additional sysfs file which contains the current measurement
resolution.

I think it would be nice to have a general sensor api which for example
specifies the sensor type, the units used for this type and the resolution of
each result.
Any thoughts on this?

> Also, we're supposed to docuemnt these things formally in
> Documentation/ABI/.
I will send a patch for this one too.

>
> > ...
> >
> > + * The chip will use 2^oversampling samples for interlan averaging.
>
> I assumed that was supposed to read "internal".
True.

Best Regards,
Christoph
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Jonathan Cameron on 23 Jun 2010 07:40
Hi Christoph
>>> This driver adds support for the BMP085 digital pressure sensor
>>> from Bosch Sensortec.
>>
>> If there's a datasheet available, it would be nice to provide a
>> reference to that in the driver.
> I will send a patch to fix this.
>
>> Please do send a full description of the proposed API so we can review
>> that. What are the contents of these files? What are the units of
>> those contents, etc?
> There is a short description before the methods bmp085_get_temperature() and
> bmp085_get_pressure(), but anyway.
>
> Here is a Description of the current sysfs user interface:
>
> The BMP085 digital pressure sensor can measure ambient air pressure and
> temperature. Both values can be obtained from sysfs files. The pressure is
> measured by reading from pressure0_input. Valid values range from 30000 to
> 110000 pascal with a resolution of 1 pascal (=0.01 millibar).
>
> temp0_input holds the current temperature in degree celsius, multiplied by 10.
> This results in a resolution of a tenth degree celsius. Values range from -400
> to 850.
>
> To increase the accuracy, this chip can calculate the average of 1, 2, 4 or 8
> samples. This behavior is controlled through the oversampling sysfs file. Two
> to the power of the value written to that file specifies how many samples will
> be used. Valid values: 0..3.
>
>
>> Are there any similar drivers in the tree (I don't think so) and if so
>> does this new driver offer the same interface?
> I did not find one.
>
>> Bear in mind that if new drivers for similar devices _do_ come along
>> then we'd prefer that those drivers implement the same interface as
>> this one. So is this driver's interface well-designed from that point
>> of view?
> The pressure0_input and temp0_input should follow the naming convention of
> hwmon devices. I think that would be ok for other sensors too. The
> oversampling is special to this device and others may not support this.
>
> A more general problem is the obtainable resolution. I work on a humidity
> sensor driver for the SHT21. It measures temperatures with a resolution of
> 0.01 celsius, ten times higher than the bmp085.
> Maybe we need an additional sysfs file which contains the current measurement
> resolution.
Follow the hwmon abi where you can. For reference if you haven't seen it, there
is a driver for the sht15 there already. I see sensirion have moved to a sensible
bus which will make life somewhat easier for you! I look forward to seeing the
driver.

The scaling you are talking about is handled in IIO with a series of extensions
to the hwmon abi. (drivers/staging/iio/Documentation/)
>
> I think it would be nice to have a general sensor api which for example
> specifies the sensor type, the units used for this type and the resolution of
> each result.
Covered by IIO though principally for a different reason. There we are interested
enough in speed of capture that we don't want to do simple linear transforms
of raw data in kernel and hence pass the values necessary to do it up to userspace.
Note we still use 'standard' units. The scaling parameters don't have to be integer
so almost any accuracy is possible.

In cases like these sensors, things are generally pretty slow, so just stick to the
conventions of hwmon (and the units specified there) unless you have very specific reasons
not to do so.

When you say sensor type... What do you mean? The current standards make the type
of reading apparent in the naming of the attributes. Do you mean something more detailed
and if so do you have an example of when userspace might care? Likewise on resolution.
Again we have reason to do it in IIO, but that doesn't apply here.
> Any thoughts on this?
>
>> Also, we're supposed to docuemnt these things formally in
>> Documentation/ABI/.
> I will send a patch for this one too.
>
Good :)

Jonathan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
 | 
Pages: 1
Prev: hwmon: Driver for SMM665 Six-Channel Active DC Output Controller/Monitor
Next: writeback: Creating /sys/kernel/mm/writeback/writeback