> Use iio: ms5637: Add Measurement Specialties MS5637 support instead of
> iio: (ms5637) Add Measurement Specialties MS5637 support for subject.
>
> If available please add a link to the datasheet here.
some more comments below...
> On Thu, Nov 6, 2014 at 11:46 AM, Markezana, William
> <William.Markezana@xxxxxxxxxxxxx> wrote:
> > Signed-off-by: Ludovic <ludovic.tancerel@xxxxxxxxxxxxxxxxx>
> > ---
> > drivers/iio/pressure/Kconfig | 10 +
> > drivers/iio/pressure/Makefile | 1 +
> > drivers/iio/pressure/ms5637.c | 488 +++++++++++++++++++++
> > drivers/staging/iio/Documentation/pressure/ms5637 | 25 ++
don't add documentation to staging
> > 4 files changed, 524 insertions(+)
> > create mode 100644 drivers/iio/pressure/ms5637.c
> > create mode 100644 drivers/staging/iio/Documentation/pressure/ms5637
> >
> > diff --git a/drivers/iio/pressure/Kconfig b/drivers/iio/pressure/Kconfig
> > index ffac8ac..4ba9bd5 100644
> > --- a/drivers/iio/pressure/Kconfig
> > +++ b/drivers/iio/pressure/Kconfig
> > @@ -41,6 +41,16 @@ config MPL3115
> > To compile this driver as a module, choose M here: the module
> > will be called mpl3115.
> >
> > +config MS5637
> > + tristate "MS5637 pressure & temperature sensor"
> > + depends on I2C
> > + help
> > + If you say yes here you get support for the Measurement Specialties
> > + MS5637 pressure and temperature sensor.
> > +
> > + This driver can also be built as a module. If so, the module will
> > + be called ms5637.
> > +
> > config IIO_ST_PRESS
> > tristate "STMicroelectronics pressure sensor Driver"
> > depends on (I2C || SPI_MASTER) && SYSFS
> > diff --git a/drivers/iio/pressure/Makefile b/drivers/iio/pressure/Makefile
> > index c53d250..50fe66e 100644
> > --- a/drivers/iio/pressure/Makefile
> > +++ b/drivers/iio/pressure/Makefile
> > @@ -6,6 +6,7 @@
> > obj-$(CONFIG_HID_SENSOR_PRESS) += hid-sensor-press.o
> > obj-$(CONFIG_MPL115) += mpl115.o
> > obj-$(CONFIG_MPL3115) += mpl3115.o
> > +obj-$(CONFIG_MS5637) += ms5637.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
> > diff --git a/drivers/iio/pressure/ms5637.c b/drivers/iio/pressure/ms5637.c
> > new file mode 100644
> > index 0000000..ea74636
> > --- /dev/null
> > +++ b/drivers/iio/pressure/ms5637.c
> > @@ -0,0 +1,488 @@
> > +/*
> > + * ms5637.c - Support for Measurement Specialties MS5637 temperature sensor
pressure & temperature
> > + *
> > + * Copyright (c) 2014 Measurement Specialties
> > + *
> > + * Licensed under the GPL-2.
> > + *
> > + * (7-bit I2C slave address 0x77)
> > + *
> > + */
> > +
> > +#include <linux/init.h>
> > +#include <linux/device.h>
> > +#include <linux/kernel.h>
> > +#include <linux/stat.h>
> > +#include <linux/module.h>
> > +#include <linux/i2c.h>
> > +#include <linux/iio/iio.h>
> > +#include <linux/iio/sysfs.h>
> > +#include <linux/jiffies.h>
> > +#include <linux/delay.h>
> > +#include <linux/mutex.h>
> > +
> > +/* MS5637 Commands */
> > +#define MS5637_RESET (0x1E)
> > +#define MS5637_TEMPERATURE_CONVERSION_START (0x50)
> > +#define MS5637_PRESSURE_CONVERSION_START (0x40)
> > +#define MS5637_ADC_READ (0x00)
> > +#define MS5637_PROM_READ (0xA0)
> > +#define MS5637_PROM_ELEMENTS_NB (7)
>
> I would prefer to remove "()" around hexa/decimal values.
>
> > +
> > +#define ATTR_RESET 0
> > +#define ATTR_RESOLUTION 1
MS5637_ prefix please
> > +
> > +static const u16 conversion_time[] = { 1000, 2000, 3000, 5000, 9000, 17000 };
ms5637_ prefix please
> > +
> > +struct ms5637_dev {
> > + struct i2c_client *client;
> > + struct mutex lock; /* mutex protecting this data structure */
> > + u16 calibration_coeffs[MS5637_PROM_ELEMENTS_NB];
> > + bool got_calibration_words;
> > + unsigned long last_update;
> > + bool valid;
> > + int temperature;
> > + unsigned int pressure;
> > + u8 resolution_index;
> > +};
> > +
> > +static int ms5637_get_calibration_coeffs(struct ms5637_dev *dev_data,
> > + unsigned char index, u16 *word)
> > +{
> > + int ret = 0;
> > +
> > + ret = i2c_smbus_read_word_swapped(dev_data->client,
> > + MS5637_PROM_READ + (index << 1));
> > + if (ret < 0)
> > + return ret;
> > + *word = (u16) ret & 0xFFFF;
> > + return 0;
> > +}
> > +
> > +static bool ms5637_crc_check(u16 *n_prom, u8 crc)
> > +{
> > + u8 cnt, n_bit;
cnt should be unsigned int, it's just a counter should have the CPUs
native size
> > + u16 n_rem, crc_read;
> > +
> > + n_rem = 0x00;
should be 0 or 0x0000 as n_rem is u16
> > + crc_read = n_prom[0];
> > + n_prom[MS5637_PROM_ELEMENTS_NB] = 0;
> > + n_prom[0] = (0x0FFF & (n_prom[0])); /* Clear the CRC byte */
remove extra parenthesis
> > +
> > + for (cnt = 0; cnt < (MS5637_PROM_ELEMENTS_NB + 1) * 2; cnt++) {
> > + if (cnt % 2 == 1)
> > + n_rem ^= n_prom[cnt >> 1] & 0x00FF;
> > + else
> > + n_rem ^= n_prom[cnt >> 1] >> 8;
> > +
> > + for (n_bit = 8; n_bit > 0; n_bit--) {
> > + if (n_rem & 0x8000)
> > + n_rem = (n_rem << 1) ^ 0x3000;
> > + else
> > + n_rem <<= 1;
> > + }
> > + }
> > + n_rem >>= 12;
> > + n_prom[0] = crc_read;
> > + return (n_rem == crc);
> > +}
> > +
> > +static int ms5637_fill_calibration_coeffs(struct ms5637_dev *dev_data)
> > +{
> > + int i, ret = 0;
> > +
> > + for (i = 0; i < MS5637_PROM_ELEMENTS_NB; i++) {
> > + ret = ms5637_get_calibration_coeffs(dev_data, i,
> > + &dev_data->calibration_coeffs
> > + [i]);
linebreak looks weird before [i]
> > +
> > + dev_dbg(&dev_data->client->dev, "Coeff %d : %d", i,
> > + dev_data->calibration_coeffs[i]);
debug output should be after validity check
> > +
> > + if (ret < 0) {
> > + dev_err(&dev_data->client->dev,
> > + "unable to get calibration coefficients at address %d\n",
Unable probably (uppercase all all other messages
> > + i + 1);
> > + return ret;
> > + }
> > + }
> > +
> > + if (!ms5637_crc_check(dev_data->calibration_coeffs,
> > + (dev_data->
> > + calibration_coeffs[0] & 0xF000) >> 12)) {
> > + dev_err(&dev_data->client->dev,
> > + "Calibration coefficients crc check error\n");
> > + return -1;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int ms5637_read_adc_value(struct ms5637_dev *dev_data, u32 *adc_value)
> > +{
> > + int ret = 0;
no need to initialize ret
> > + u8 buf[3];
> > +
> > + ret =
> > + i2c_smbus_read_i2c_block_data(dev_data->client, MS5637_ADC_READ, 3,
> > + buf);
> > + if (ret < 0)
> > + return ret;
> > + dev_dbg(&dev_data->client->dev, "ADC raw value : %x %x %x\n", buf[0],
> > + buf[1], buf[2]);
> > + *adc_value = (buf[0] << 16) + (buf[1] << 8) + buf[0];
> > + return 0;
> > +}
> > +
> > +static int ms5637_conversion_and_read_adc(struct ms5637_dev *dev_data,
> > + u32 *t_adc, u32 *p_adc)
> > +{
> > + int ret;
> > +
> > + /* Trigger Temperature conversion */
> > + ret = i2c_smbus_write_byte(dev_data->client,
> > + MS5637_TEMPERATURE_CONVERSION_START
> > + + dev_data->resolution_index * 2);
> > + if (ret < 0)
> > + return ret;
> > + usleep_range(conversion_time[dev_data->resolution_index],
> > + conversion_time[dev_data->resolution_index]+3000);
> > + /* Retrieve ADC value */
> > + ret = ms5637_read_adc_value(dev_data, t_adc);
> > +
> No check for ret here? If this is not needed then you can remove
> the above assignment.
>
> > + /* Trigger Pressure conversion */
> > + ret = i2c_smbus_write_byte(dev_data->client,
> > + MS5637_PRESSURE_CONVERSION_START
> > + + dev_data->resolution_index * 2);
> > + if (ret < 0)
> > + return ret;
> > + usleep_range(conversion_time[dev_data->resolution_index],
> > + conversion_time[dev_data->resolution_index]+3000);
> > + /* Retrieve ADC value */
> > + ret = ms5637_read_adc_value(dev_data, p_adc);
> > +
> No new line here.
> > + if (ret < 0)
> > + return ret;
> > + return 0;
> > +}
> > +
> > +static int ms5637_read_temperature_and_pressure(struct ms5637_dev *dev_data)
> > +{
> > + int ret = 0;
> > + u32 t_adc, p_adc;
> > + s32 dt, temp;
> > + s64 off, sens, t2, off2, sens2;
> > +
> > + mutex_lock(&dev_data->lock);
> > +
> > + if (time_after(jiffies, dev_data->last_update + HZ / 2) ||
> > + !dev_data->valid) {
> > + if (!dev_data->got_calibration_words) {
> > + ret = ms5637_fill_calibration_coeffs(dev_data);
> > + if (ret < 0)
> > + goto out;
> > +
> > + dev_data->got_calibration_words = true;
> > + }
> > +
> > + ret = ms5637_conversion_and_read_adc(dev_data, &t_adc, &p_adc);
> > +
remove blank line
> > + if (ret < 0) {
> > + dev_data->got_calibration_words = false;
> > + dev_err(&dev_data->client->dev,
> > + "unable to make sensor adc conversion\n");
> > + goto out;
> > + }
> > +
> > + dt = (s32) t_adc - (dev_data->calibration_coeffs[5] << 8);
> > +
> > + /* Actual temperature = 2000 + dT * TEMPSENS */
> > + temp =
> > + 2000 + (((s64) dt * dev_data->calibration_coeffs[6]) >> 23);
> > +
> > + /* Second order temperature compensation */
> > + if (temp < 2000) {
> > + s64 tmp = (s64) temp - 2000;
> > +
> > + t2 = (3 * ((s64) dt * (s64) dt)) >> 33;
> > + off2 = (61 * tmp * tmp) >> 4;
> > + sens2 = (29 * tmp * tmp) >> 4;
> > +
> > + if (temp < -1500) {
> > + s64 tmp = (s64) temp + 1500;
> > +
> > + off2 += 17 * tmp * tmp;
> > + sens2 += 9 * tmp * tmp;
> > + }
> > + } else {
> > + t2 = (5 * ((s64) dt * (s64) dt)) >> 38;
> > + off2 = 0;
> > + sens2 = 0;
> > + }
> > +
> > + /* OFF = OFF_T1 + TCO * dT */
> > + off = (((s64) dev_data->calibration_coeffs[2]) << 17)
> > + +
> > + ((((s64) dev_data->calibration_coeffs[4]) * (s64) dt) >> 6);
> > + off -= off2;
> > +
> > + /* Sensitivity at actual temperature = SENS_T1 + TCS * dT */
> > + sens = (((s64) dev_data->calibration_coeffs[1]) << 16)
> > + + (((s64) dev_data->calibration_coeffs[3] * dt) >> 7);
> > + sens -= sens2;
> > +
> > + /* Temperature compensated pressure = D1 * SENS - OFF */
> > + dev_data->temperature = (temp - t2) * 10;
> > + dev_data->pressure = (u32) (((((s64) p_adc * sens) >> 21)
> > + - off) >> 15) / 100;
> > +
> > + dev_data->last_update = jiffies;
> > + dev_data->valid = true;
> > + }
> > + out:
> > + mutex_unlock(&dev_data->lock);
> > +
> > + return ret >= 0 ? 0 : ret;
> > +}
> > +
> > +static int ms5637_read_raw(struct iio_dev *indio_dev,
> > + struct iio_chan_spec const *channel, int *val,
> > + int *val2, long mask)
> > +{
> > + int ret;
> > + struct ms5637_dev *dev_data = iio_priv(indio_dev);
> > +
> > + switch (mask) {
> > + case IIO_CHAN_INFO_PROCESSED:
> > + switch (channel->type) {
> > + case IIO_TEMP: /* in 0.001 °C */
> > + ret = ms5637_read_temperature_and_pressure(dev_data);
> > + if (ret)
> > + goto t_err;
> > + *val = dev_data->temperature;
> > + break;
> > + case IIO_PRESSURE: /* in mB */
IIO wants kilopoascal
> > + ret = ms5637_read_temperature_and_pressure(dev_data);
> > + if (ret)
> > + goto p_err;
> > + *val = dev_data->pressure;
> > + break;
> > + default:
> > + return -EINVAL;
> > + }
> > + ret = IIO_VAL_INT;
> > + break;
> > + default:
> > + return -EINVAL;
> > + }
> > +
> > + return ret;
should simplify control flow a bit, just return instead of breaking,
jumping
is it really necesary to have two different error messages?
could just code the call to ms5637_read_temperature_and_pressure() once
> > + t_err:
> > + dev_err(&indio_dev->dev, "Temperature read error\n");
> > + return ret;
> > + p_err:
> > + dev_err(&indio_dev->dev, "Pressure read error\n");
> > + return ret;
> > +}
> > +
> > +static const struct iio_chan_spec ms5637_channels[] = {
> > + {
> > + .type = IIO_TEMP,
> > + .output = 1,
> > + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
> > + .scan_index = 0,
> > + },
> > + {
> > + .type = IIO_PRESSURE,
> > + .output = 1,
output is for output channels, I guess the ms5637 cannot change the
weather :)
> > + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
> > + .scan_index = 0,
no need set the scan_index
> > + }
> > +};
> > +
> > +static ssize_t ms5637_read_attr(struct device *dev,
> > + struct device_attribute *attr, char *buf)
> > +{
> > + struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> > + struct ms5637_dev *dev_data = iio_priv(indio_dev);
> > + struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
> > + int ret = 0;
> > +
> > + switch (this_attr->address) {
> > + case ATTR_RESET:
> > + ret = sprintf(buf, "0\n");
> > + break;
> > + case ATTR_RESOLUTION:
uhuh, private ABI
maybe this could be modelled by IIO's integration time?
> > + ret = sprintf(buf, "%d\n", dev_data->resolution_index + 8);
> > + break;
> > + default:
> > + return -EINVAL;
> > + }
> > +
> > + return ret;
> > +}
> > +
> > +static ssize_t ms5637_write_attr(struct device *dev,
> > + struct device_attribute *attr,
> > + const char *buf, size_t len)
> > +{
> > + struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> > + struct ms5637_dev *dev_data = iio_priv(indio_dev);
> > + struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
> > + u8 val;
> > + int ret;
> > +
> > + ret = kstrtou8(buf, 10, &val);
> > + if (ret)
> > + return ret;
> > +
> > + mutex_lock(&dev_data->lock);
> > + switch (this_attr->address) {
> > + case ATTR_RESET:
> > + if (val != 1)
> > + goto einval;
> > +
> > + ret = i2c_smbus_write_byte(dev_data->client, MS5637_RESET);
> > + if (ret < 0)
> > + goto out;
> > + usleep_range(3000, 6000);
> > +
> > + break;
> > + case ATTR_RESOLUTION:
> > + if ((val < 8) || (val > 13))
> > + goto einval;
> > + dev_data->resolution_index = val - 8;
> > + break;
> > + default:
> > + goto einval;
> > + }
> > +
> > + dev_data->valid = false;
> > + goto out;
> > + einval:
> > + ret = -EINVAL;
> > + out:
> > + mutex_unlock(&dev_data->lock);
> > +
> > + return ret ? ret : len;
> > +}
> > +
> > +static IIO_DEVICE_ATTR(in_reset,
> > + S_IRUGO | S_IWUSR,
> > + ms5637_read_attr, ms5637_write_attr, ATTR_RESET);
> > +static IIO_DEVICE_ATTR(in_resolution,
> > + S_IRUGO | S_IWUSR,
> > + ms5637_read_attr, ms5637_write_attr, ATTR_RESOLUTION);
> > +static IIO_CONST_ATTR(resolution_available, "8 9 10 11 12 13");
> > +
> > +static struct attribute *ms5637_attributes[] = {
> > + &iio_dev_attr_in_reset.dev_attr.attr,
> > + &iio_dev_attr_in_resolution.dev_attr.attr,
> > + &iio_const_attr_resolution_available.dev_attr.attr,
> > + NULL,
> > +};
> > +
> > +static const struct attribute_group ms5637_attribute_group = {
> > + .attrs = ms5637_attributes,
> > +};
> > +
> > +static const struct iio_info ms5637_info = {
> > + .read_raw = ms5637_read_raw,
> > + .attrs = &ms5637_attribute_group,
> > + .driver_module = THIS_MODULE,
> > +};
> > +
> > +static int ms5637_probe(struct i2c_client *client,
> > + const struct i2c_device_id *id)
> > +{
> > + struct ms5637_dev *dev_data;
> > + struct iio_dev *indio_dev;
> > + int ret;
> > +
> > + if (!i2c_check_functionality(client->adapter,
> > + I2C_FUNC_SMBUS_READ_WORD_DATA)) {
> > + dev_err(&client->dev,
> > + "adapter does not support SMBus read word transactions\n");
> > + return -ENODEV;
> > + }
> > +
> > + if (!i2c_check_functionality(client->adapter,
> > + I2C_FUNC_SMBUS_WRITE_BYTE)) {
> > + dev_err(&client->dev,
> > + "adapter does not support SMBus write byte transactions\n");
> > + return -ENODEV;
> > + }
> > +
> > + if (!i2c_check_functionality(client->adapter,
> > + I2C_FUNC_SMBUS_READ_I2C_BLOCK)) {
> > + dev_err(&client->dev,
> > + "adapter does not support SMBus read block transactions\n");
> > + return -ENODEV;
> > + }
> > +
> > + indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*dev_data));
> > + if (!indio_dev)
> > + return -ENOMEM;
> > +
> > + dev_data = iio_priv(indio_dev);
> > + dev_data->client = client;
> > + mutex_init(&dev_data->lock);
> > +
> > + indio_dev->info = &ms5637_info;
> > + indio_dev->name = id->name;
> > + indio_dev->dev.parent = &client->dev;
> > + indio_dev->modes = INDIO_DIRECT_MODE;
> > + indio_dev->channels = ms5637_channels;
> > + indio_dev->num_channels = ARRAY_SIZE(ms5637_channels);
> > +
> > + i2c_set_clientdata(client, indio_dev);
> > + ret = iio_device_register(indio_dev);
> > + if (ret < 0)
> > + return ret;
> > +
> > + ret = i2c_smbus_write_byte(client, MS5637_RESET);
I'd rather do this before iio_device_register() -- what happens when it
fails? there is no unregister()
> > + if (ret < 0)
> > + return ret;
> > + usleep_range(3000, 6000);
> > +
> > + ret = ms5637_fill_calibration_coeffs(dev_data);
> > + if (ret == 0)
> > + dev_data->got_calibration_words = true;
> > +
> > + if (ret < 0)
> > + return ret;
> > +
> > + dev_dbg(&client->dev, "Driver initialization done");
> > + return 0;
> > +}
> > +
> > +static int ms5637_remove(struct i2c_client *client)
> > +{
> > + struct iio_dev *indio_dev = i2c_get_clientdata(client);
> > +
> > + iio_device_unregister(indio_dev);
> > +
> > + return 0;
> > +}
> > +
> > +static const struct i2c_device_id ms5637_id[] = {
> > + {"ms5637", 0},
> > + {}
> > +};
> > +
> > +static struct i2c_driver ms5637_driver = {
> > + .probe = ms5637_probe,
> > + .remove = ms5637_remove,
> > + .id_table = ms5637_id,
> > + .driver = {
> > + .name = "ms5637",
> > + .owner = THIS_MODULE,
> > + },
> > +};
> > +
> > +module_i2c_driver(ms5637_driver);
> > +
> > +MODULE_LICENSE("GPL");
> > +MODULE_DESCRIPTION("Measurement Specialties MS5637 temperature driver");
> > +MODULE_AUTHOR("William Markezana <william.markezana@xxxxxxxxxxxxx>");
> > +MODULE_AUTHOR("Ludovic Tancerel <ludovic.tancerel@xxxxxxxxxxxxxxxxx>");
> > diff --git a/drivers/staging/iio/Documentation/pressure/ms5637 b/drivers/staging/iio/Documentation/pressure/ms5637
> > new file mode 100644
> > index 0000000..d0d64f3
> > --- /dev/null
> > +++ b/drivers/staging/iio/Documentation/pressure/ms5637
> > @@ -0,0 +1,25 @@
> > +Kernel driver ms5637
> > +====================
> > +
> > +Supported chips:
> > + * Measurement Specialties MS5637
> > + Prefix: 'ms5637'
> > + Addresses scanned: I2C 0x76
0x77 is claimed at the top of ms5637.c
> > + Datasheet: Available for download on meas-spec.com
> > +
> > +Authors:
> > + William Markezana (Meas-Spec) <william.markezana@xxxxxxxxxxxxx>
> > + Ludovic Tancerel <ludovic.tancerel@xxxxxxxxxxxxxxxxx>
> > +
> > +Description
> > +-----------
> > +
> > +The MS5637 is a single chip pressure & temperature sensor.
> > +The driver returns a milli-bar pressure value and a milli-degre celius value using the iio framework
degree; the information so far is redundant
> > +
> > +Via the iio sysfs interface, there are several attributes available.
> > +in_reset - Reset ms5637 device by writing a 1 in it.
> > +in_resolution - Set the number of bits for ADC resolution.
> > +out_pressure_input - Current pressure from ms5637 sensor (milli-bar)
> > +out_temp_input - Current temperature from htu21 sensor (milli-°C value)
> > +resolution_available - List of resolutions supported
in_, not out_
regular IIO channels (integration time?) could/should be used
I don't know about reset, needed?
private ABI should go to documentation/ABI/testing/sysfs-bus-iio-*
> > --
> > 1.7.9.5
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > the body of a message to majordomo@xxxxxxxxxxxxxxx
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at http://www.tux.org/lkml/
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
--
Peter Meerwald
+43-664-2444418 (mobile)
