Re: [PATCH] iio: (ms5637) Add Measurement Specialties MS5637 support

[Jonathan Cameron]

On 06/11/14 16:54, Peter Meerwald wrote:
>> 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...
And a few more from me...
>  
>> 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;
Proper error codes.
>>> +       }
>>> +
>>> +       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];
Looks suspect... Using buf[0] twice?
>>> +       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?
Or sampling_frequency (not always the same ;)  
> 
>>> +               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;

Not nice.  Set ret when the error occurs and have the good path
drop straight through.

Also check the request is valid before taking the lock and your flow
becomes quite a bit simpler.

>>> + 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");
This one is always interesting.

Do any applications care?
Would we be better off just providing one case?
Does it effect anything else? (data rate?)
If so are we better off using the sampling_frequency
interface to expose it?  Most of the time that is the
knob that userspace is going to want to turn.


>>> +
>>> +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()

Just to reinforce this.  When iio_device_register is called, the
device is exposed to userspace.  Hence it must be ready to respond
coherently to any interactions.  That's why it is almost always the
last thing called in the probe funciton.
> 
>>> +       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);
As all you have is iio_device_unregister in here you have
a rare case where we could use devm_iio_device_register
and drop the remove functional altogether.
(which is why we have a devm version of that function)
>>> +
>>> +       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
Also doesn't scan.  Address that is valid is fair enough though if correct
> 
>>> +    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
Line is way to long as well ;)
> 
>>> +
>>> +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?
Explicit userspace reset is unusual.  Normally either the driver
can detect a problem (device not responding etc) and reset or
it is done on driver load to get the device into a sane state in the
first place.

Tends to be useful during driver debugging, but adds nothing
after the driver is written... 
 
> 
> private ABI should go to documentation/ABI/testing/sysfs-bus-iio-*
Absolutely.
> 
>>> --
>>> 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/
>>
> 

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