Re: [PATCH] iio: temperature: mcp9808: add Microchip MCP9808 temperature sensor
...
>>> +static int mcp9808_read_raw(struct iio_dev *indio_dev,
>>> + struct iio_chan_spec const *channel,
>>> + int *val, int *val2, long mask)
>>> +
>>> +{
>>> + struct mcp9808_data *data = iio_priv(indio_dev);
>>> + int ret;
>>> +
>>> + switch (mask) {
>>> + case IIO_CHAN_INFO_RAW:
>>> + ret = i2c_smbus_read_word_swapped(data->client,
>>> + MCP9808_REG_TAMBIENT);
>>> + if (ret < 0)
>>> + return ret;
>>> + *val = sign_extend32(ret, 12);
>> I just laughed when I saw the pile of BS they have in the datasheet to
>> describe exactly what you have here...
>>
> Well, then you'd find it hilarious the lengths I took to prove that the
> simple sign-extend & scale was correct in response to the data sheets
> bountiful calculations!
:)
>
>>> + return IIO_VAL_INT;
>>> +
>>> + case IIO_CHAN_INFO_SCALE:
>>> + *val = 0;
>>> + *val2 = 62500;
>>> + return IIO_VAL_INT_PLUS_MICRO;
>>> +
>>> + case IIO_CHAN_INFO_INT_TIME:
>>> + *val = 0;
>>> + *val2 = mcp9808_res[data->res_index][0];
>>> + return IIO_VAL_INT_PLUS_MICRO;
>>> +
>>> + default:
>>> + break;
>>> + }
>>> +
>>> + return -EINVAL;
>>> +}
>>> +
>>> +static int mcp9808_write_raw(struct iio_dev *indio_dev,
>>> +struct iio_chan_spec const *channel,
>>> +int val, int val2, long mask)
>>> +{
>>> + struct mcp9808_data *data = iio_priv(indio_dev);
>>> + int ret = -EINVAL;
>>> +
>>> + if (mask == IIO_CHAN_INFO_INT_TIME) {
>>> + if (!val)
>>> + ret = mcp9808_set_resolution(data, val2);
>>
>> Hmm. Is this integration time? I think it is more likely to be either:
>> 1) The number of stages of the ADC. (normally gives a very minor time
>> difference in a SAR ADC or similar as going from say 8 to 10 bits is only
>> a 20% increase).
>> 2) Number of averages samples (oversampling). (almost certainly true here).
>>
>> Integration time doesn't make much sense for a temperature sensor. These
>> tend to be analog parts with a track and hold type ADC that just gets what
>> ever is on the wire when a reading is requested. Integration time is more
>> for things like light sensors where you are looking at counting number
>> of photons (very badly) in a fixed time period.
>>
>> Hmm. So how should it be supported..
>> Could use sampling_frequency as that also reflects sampling period.
>> It's not ideal though. Often we've just not bothered to support anything
>> other than the highest resolution (particularly on fast devices), but here
>> it really does lead to very low speeds... Basis for not supporting it in
>> the past was that mostly the cost was minor to increase the resolution and
>>
>> If we knew it really was just oversampling this would be easy. I suppose it
>> doesn't actually matter what the hardware is doing. That's what the software
>> is effectively seeing .
>>
>> Unfortunately, for oversampling to be used you'd probably also want an
>> indication of the sampling frequency so you'd need that one as well.
>>
>> So I think the best option is oversampling_ratio and sampling_frequency.
>> Control via oversampling_ratio.
>>
>
> I'm wondering if I simply implemented it backwards.
>
> Now I offer a selection resolutions (which I label integration times).
>
> Should I simply flip it to offer a selection of integration times which
> then indicate which resolution driver will set?
I'll admit I've forgotten all about this now and don't have time to
dig back into it... Sorry!
>>> + }
>>> + return ret;
>>> +}
>>> +
Re: [PATCH] iio: temperature: mcp9808: add Microchip MCP9808 temperature sensor
On Sun, Jul 10, 2016 at 02:23:27PM +0100, Jonathan Cameron wrote:
> On 03/07/16 22:04, Alison Schofield wrote:
> > IIO driver, perhaps a reference driver, since this sensor is already
> > supported in hwmon/jc42 driver.
> >
> > Driver supports continuous conversion, resolution changes and
> > suspend/resume power ops.
> >
> > Signed-off-by: Alison Schofield
> > Cc: Daniel Baluta
> Hi Alison,
>
> Thanks for posting this. Given the work was done it's a useful exercise to
> put
> it out for review.
>
> Mostly very good little driver. The complex corner is the multiple resolution
> support. My guess is that this is purely a bit of 'mystified' oversampling
> on the chip and I'd suggest supporting it as such.
>
> Jonathan
Thanks Jonathan! I appreciate you putting eyes on this!
More inline...
alisons
> > ---
> > drivers/iio/temperature/Kconfig | 10 ++
> > drivers/iio/temperature/Makefile | 1 +
> > drivers/iio/temperature/mcp9808.c | 269
> > ++
> > 3 files changed, 280 insertions(+)
> > create mode 100644 drivers/iio/temperature/mcp9808.c
> >
> > diff --git a/drivers/iio/temperature/Kconfig
> > b/drivers/iio/temperature/Kconfig
> > index c4664e5..25915d2 100644
> > --- a/drivers/iio/temperature/Kconfig
> > +++ b/drivers/iio/temperature/Kconfig
> > @@ -3,6 +3,16 @@
> > #
> > menu "Temperature sensors"
> >
> > +config MCP9808
> > + tristate "MCP9808 temperature sensor"
> > + depends on I2C
> > + help
> > + If you say yes here you get support for the Microchip
> > + MCP9808 temperature sensor connected with I2C.
> > +
> > + This driver can also be built as a module. If so, the module will
> > + be called mcp9808.
> > +
> > config MLX90614
> > tristate "MLX90614 contact-less infrared sensor"
> > depends on I2C
> > diff --git a/drivers/iio/temperature/Makefile
> > b/drivers/iio/temperature/Makefile
> > index 02bc79d..92cd1e6 100644
> > --- a/drivers/iio/temperature/Makefile
> > +++ b/drivers/iio/temperature/Makefile
> > @@ -2,6 +2,7 @@
> > # Makefile for industrial I/O temperature drivers
> > #
> >
> > +obj-$(CONFIG_MCP9808) += mcp9808.o
> > obj-$(CONFIG_MLX90614) += mlx90614.o
> > obj-$(CONFIG_TMP006) += tmp006.o
> > obj-$(CONFIG_TSYS01) += tsys01.o
> > diff --git a/drivers/iio/temperature/mcp9808.c
> > b/drivers/iio/temperature/mcp9808.c
> > new file mode 100644
> > index 000..adab708
> > --- /dev/null
> > +++ b/drivers/iio/temperature/mcp9808.c
> > @@ -0,0 +1,269 @@
> > +/*
> > + * mcp9808.c - Support for Microchip MCP9808 Digital Temperature Sensor
> > + *
> > + * Copyright (C) 2016 Alison Schofield
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
> > + *
> > + * Datasheet: http://ww1.microchip.com/downloads/en/DeviceDoc/25095A.pdf
> > + */
> > +
> > +#include
> > +#include
> > +#include
> > +
> > +#include
> > +#include
> > +
> > +#define MCP9808_REG_CONFIG 0x01
> > +#define MCP9808_REG_TAMBIENT 0x05
> > +#define MCP9808_REG_MANUF_ID 0x06
> > +#define MCP9808_REG_DEVICE_ID 0x07
> > +#define MCP9808_REG_RESOLUTION 0x08
> > +
> > +#define MCP9808_CONFIG_DEFAULT 0x00
> There's a lot of other stuff in this register.
> I guess most of it is alert related and that isn't supported here though
> so fair enough. Can be introduced when it becomes relevant.
>
Yes, more options avail. All default to zero now.
> > +#define MCP9808_CONFIG_SHUTDOWN0x0100
> > +
> > +#define MCP9808_RES_DEFAULT62500
> > +
> > +#define MCP9808_MANUF_ID 0x54
> > +#define MCP9808_DEVICE_ID 0x0400
> > +#define MCP9808_DEVICE_ID_MASK 0xff00
> > +
> > +struct mcp9808_data {
> > + struct i2c_client *client;
> > + struct mutex lock;/* protect resolution changes */
> > + intres_index; /* current resolution index*/
> > +};
> > +
> > +/* Resolution, MCP9808_REG_RESOLUTION bits, Conversion Time ms */
> > +static const int mcp9808_res[][3] = {
> > + {50, 0, 30},
> > + {25, 1, 65},
> > + {125000, 2, 130},
> > + { 62500, 3, 250},
> > +};
> > +
> > +static IIO_CONST_ATTR(temp_integration_time_available,
> > + "0.5 0.25 0.125 0.0625");
> > +
> > +static struct attribute *mcp9808_attributes[] = {
> > + &iio_const_attr_temp_integration_time_available.dev_attr.attr,
> > + NULL
> > +};
> > +
> > +static struct attribute_group mcp9808_attribute_group = {
> > + .attrs = mcp9808_attributes,
> > +};
> > +
> > +static int mcp9808_set_resolution(struct mcp9808_data *data, int val2)
> > +{
> > + int i;
> > + int ret = -EINVAL;
> > + int conv_t = mcp9808_res[data->res_index][2];
> > +
> > + for (i = 0; i < ARRAY_SIZE(mcp9808_res); i++) {
> > + if (val2 == mcp9808_res[i][0]) {
> >
Re: [PATCH] iio: temperature: mcp9808: add Microchip MCP9808 temperature sensor
On 03/07/16 22:04, Alison Schofield wrote:
> IIO driver, perhaps a reference driver, since this sensor is already
> supported in hwmon/jc42 driver.
>
> Driver supports continuous conversion, resolution changes and
> suspend/resume power ops.
>
> Signed-off-by: Alison Schofield
> Cc: Daniel Baluta
Hi Alison,
Thanks for posting this. Given the work was done it's a useful exercise to put
it out for review.
Mostly very good little driver. The complex corner is the multiple resolution
support. My guess is that this is purely a bit of 'mystified' oversampling
on the chip and I'd suggest supporting it as such.
Jonathan
> ---
> drivers/iio/temperature/Kconfig | 10 ++
> drivers/iio/temperature/Makefile | 1 +
> drivers/iio/temperature/mcp9808.c | 269
> ++
> 3 files changed, 280 insertions(+)
> create mode 100644 drivers/iio/temperature/mcp9808.c
>
> diff --git a/drivers/iio/temperature/Kconfig b/drivers/iio/temperature/Kconfig
> index c4664e5..25915d2 100644
> --- a/drivers/iio/temperature/Kconfig
> +++ b/drivers/iio/temperature/Kconfig
> @@ -3,6 +3,16 @@
> #
> menu "Temperature sensors"
>
> +config MCP9808
> + tristate "MCP9808 temperature sensor"
> + depends on I2C
> + help
> + If you say yes here you get support for the Microchip
> + MCP9808 temperature sensor connected with I2C.
> +
> + This driver can also be built as a module. If so, the module will
> + be called mcp9808.
> +
> config MLX90614
> tristate "MLX90614 contact-less infrared sensor"
> depends on I2C
> diff --git a/drivers/iio/temperature/Makefile
> b/drivers/iio/temperature/Makefile
> index 02bc79d..92cd1e6 100644
> --- a/drivers/iio/temperature/Makefile
> +++ b/drivers/iio/temperature/Makefile
> @@ -2,6 +2,7 @@
> # Makefile for industrial I/O temperature drivers
> #
>
> +obj-$(CONFIG_MCP9808) += mcp9808.o
> obj-$(CONFIG_MLX90614) += mlx90614.o
> obj-$(CONFIG_TMP006) += tmp006.o
> obj-$(CONFIG_TSYS01) += tsys01.o
> diff --git a/drivers/iio/temperature/mcp9808.c
> b/drivers/iio/temperature/mcp9808.c
> new file mode 100644
> index 000..adab708
> --- /dev/null
> +++ b/drivers/iio/temperature/mcp9808.c
> @@ -0,0 +1,269 @@
> +/*
> + * mcp9808.c - Support for Microchip MCP9808 Digital Temperature Sensor
> + *
> + * Copyright (C) 2016 Alison Schofield
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * Datasheet: http://ww1.microchip.com/downloads/en/DeviceDoc/25095A.pdf
> + */
> +
> +#include
> +#include
> +#include
> +
> +#include
> +#include
> +
> +#define MCP9808_REG_CONFIG 0x01
> +#define MCP9808_REG_TAMBIENT 0x05
> +#define MCP9808_REG_MANUF_ID 0x06
> +#define MCP9808_REG_DEVICE_ID0x07
> +#define MCP9808_REG_RESOLUTION 0x08
> +
> +#define MCP9808_CONFIG_DEFAULT 0x00
There's a lot of other stuff in this register.
I guess most of it is alert related and that isn't supported here though
so fair enough. Can be introduced when it becomes relevant.
> +#define MCP9808_CONFIG_SHUTDOWN 0x0100
> +
> +#define MCP9808_RES_DEFAULT 62500
> +
> +#define MCP9808_MANUF_ID 0x54
> +#define MCP9808_DEVICE_ID0x0400
> +#define MCP9808_DEVICE_ID_MASK 0xff00
> +
> +struct mcp9808_data {
> + struct i2c_client *client;
> + struct mutex lock;/* protect resolution changes */
> + intres_index; /* current resolution index*/
> +};
> +
> +/* Resolution, MCP9808_REG_RESOLUTION bits, Conversion Time ms */
> +static const int mcp9808_res[][3] = {
> + {50, 0, 30},
> + {25, 1, 65},
> + {125000, 2, 130},
> + { 62500, 3, 250},
> +};
> +
> +static IIO_CONST_ATTR(temp_integration_time_available,
> + "0.5 0.25 0.125 0.0625");
> +
> +static struct attribute *mcp9808_attributes[] = {
> + &iio_const_attr_temp_integration_time_available.dev_attr.attr,
> + NULL
> +};
> +
> +static struct attribute_group mcp9808_attribute_group = {
> + .attrs = mcp9808_attributes,
> +};
> +
> +static int mcp9808_set_resolution(struct mcp9808_data *data, int val2)
> +{
> + int i;
> + int ret = -EINVAL;
> + int conv_t = mcp9808_res[data->res_index][2];
> +
> + for (i = 0; i < ARRAY_SIZE(mcp9808_res); i++) {
> + if (val2 == mcp9808_res[i][0]) {
> + mutex_lock(&data->lock);
> + ret = i2c_smbus_write_byte_data(data->client,
> + MCP9808_REG_RESOLUTION,
> + mcp9808_res[i][1]);
> + data->res_index = i;
> + mutex_unlock(&data->lock);
> +
> + /* delay old + new conversion time */
> +
[PATCH] iio: temperature: mcp9808: add Microchip MCP9808 temperature sensor
IIO driver, perhaps a reference driver, since this sensor is already
supported in hwmon/jc42 driver.
Driver supports continuous conversion, resolution changes and
suspend/resume power ops.
Signed-off-by: Alison Schofield
Cc: Daniel Baluta
---
drivers/iio/temperature/Kconfig | 10 ++
drivers/iio/temperature/Makefile | 1 +
drivers/iio/temperature/mcp9808.c | 269 ++
3 files changed, 280 insertions(+)
create mode 100644 drivers/iio/temperature/mcp9808.c
diff --git a/drivers/iio/temperature/Kconfig b/drivers/iio/temperature/Kconfig
index c4664e5..25915d2 100644
--- a/drivers/iio/temperature/Kconfig
+++ b/drivers/iio/temperature/Kconfig
@@ -3,6 +3,16 @@
#
menu "Temperature sensors"
+config MCP9808
+ tristate "MCP9808 temperature sensor"
+ depends on I2C
+ help
+ If you say yes here you get support for the Microchip
+ MCP9808 temperature sensor connected with I2C.
+
+ This driver can also be built as a module. If so, the module will
+ be called mcp9808.
+
config MLX90614
tristate "MLX90614 contact-less infrared sensor"
depends on I2C
diff --git a/drivers/iio/temperature/Makefile b/drivers/iio/temperature/Makefile
index 02bc79d..92cd1e6 100644
--- a/drivers/iio/temperature/Makefile
+++ b/drivers/iio/temperature/Makefile
@@ -2,6 +2,7 @@
# Makefile for industrial I/O temperature drivers
#
+obj-$(CONFIG_MCP9808) += mcp9808.o
obj-$(CONFIG_MLX90614) += mlx90614.o
obj-$(CONFIG_TMP006) += tmp006.o
obj-$(CONFIG_TSYS01) += tsys01.o
diff --git a/drivers/iio/temperature/mcp9808.c
b/drivers/iio/temperature/mcp9808.c
new file mode 100644
index 000..adab708
--- /dev/null
+++ b/drivers/iio/temperature/mcp9808.c
@@ -0,0 +1,269 @@
+/*
+ * mcp9808.c - Support for Microchip MCP9808 Digital Temperature Sensor
+ *
+ * Copyright (C) 2016 Alison Schofield
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * Datasheet: http://ww1.microchip.com/downloads/en/DeviceDoc/25095A.pdf
+ */
+
+#include
+#include
+#include
+
+#include
+#include
+
+#define MCP9808_REG_CONFIG 0x01
+#define MCP9808_REG_TAMBIENT 0x05
+#define MCP9808_REG_MANUF_ID 0x06
+#define MCP9808_REG_DEVICE_ID 0x07
+#define MCP9808_REG_RESOLUTION 0x08
+
+#define MCP9808_CONFIG_DEFAULT 0x00
+#define MCP9808_CONFIG_SHUTDOWN0x0100
+
+#define MCP9808_RES_DEFAULT62500
+
+#define MCP9808_MANUF_ID 0x54
+#define MCP9808_DEVICE_ID 0x0400
+#define MCP9808_DEVICE_ID_MASK 0xff00
+
+struct mcp9808_data {
+ struct i2c_client *client;
+ struct mutex lock;/* protect resolution changes */
+ intres_index; /* current resolution index*/
+};
+
+/* Resolution, MCP9808_REG_RESOLUTION bits, Conversion Time ms */
+static const int mcp9808_res[][3] = {
+ {50, 0, 30},
+ {25, 1, 65},
+ {125000, 2, 130},
+ { 62500, 3, 250},
+};
+
+static IIO_CONST_ATTR(temp_integration_time_available,
+ "0.5 0.25 0.125 0.0625");
+
+static struct attribute *mcp9808_attributes[] = {
+ &iio_const_attr_temp_integration_time_available.dev_attr.attr,
+ NULL
+};
+
+static struct attribute_group mcp9808_attribute_group = {
+ .attrs = mcp9808_attributes,
+};
+
+static int mcp9808_set_resolution(struct mcp9808_data *data, int val2)
+{
+ int i;
+ int ret = -EINVAL;
+ int conv_t = mcp9808_res[data->res_index][2];
+
+ for (i = 0; i < ARRAY_SIZE(mcp9808_res); i++) {
+ if (val2 == mcp9808_res[i][0]) {
+ mutex_lock(&data->lock);
+ ret = i2c_smbus_write_byte_data(data->client,
+ MCP9808_REG_RESOLUTION,
+ mcp9808_res[i][1]);
+ data->res_index = i;
+ mutex_unlock(&data->lock);
+
+ /* delay old + new conversion time */
+ msleep(conv_t + mcp9808_res[i][2]);
+ break;
+ }
+ }
+ return ret;
+}
+
+static int mcp9808_read_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *channel,
+ int *val, int *val2, long mask)
+
+{
+ struct mcp9808_data *data = iio_priv(indio_dev);
+ int ret;
+
+ switch (mask) {
+ case IIO_CHAN_INFO_RAW:
+ ret = i2c_smbus_read_word_swapped(data->client,
+ MCP9808_REG_TAMBIENT);
+ if (ret < 0)
+ return ret;
+ *val = sign_extend32(ret, 12);
+ return IIO_VAL_INT;
+
+ case IIO_CHAN_INFO_
