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 <amsfield22@xxxxxxxxx> > > Cc: Daniel Baluta <daniel.baluta@xxxxxxxxx> > 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 0000000..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 <amsfield22@xxxxxxxxx> > > + * > > + * 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 <linux/delay.h> > > +#include <linux/i2c.h> > > +#include <linux/module.h> > > + > > +#include <linux/iio/iio.h> > > +#include <linux/iio/sysfs.h> > > + > > +#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_SHUTDOWN 0x0100 > > + > > +#define MCP9808_RES_DEFAULT 62500 > > + > > +#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 */ > > + int res_index; /* current resolution index */ > > +}; > > + > > +/* Resolution, MCP9808_REG_RESOLUTION bits, Conversion Time ms */ > > +static const int mcp9808_res[][3] = { > > + {500000, 0, 30}, > > + {250000, 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); > 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? > > + } > > + return ret; > > +} > > + > > +static const struct iio_chan_spec mcp9808_channels[] = { > > + { > > + .type = IIO_TEMP, > > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | > > + BIT(IIO_CHAN_INFO_SCALE) | > > + BIT(IIO_CHAN_INFO_INT_TIME) > > + } > > +}; > > + > > +static const struct iio_info mcp9808_info = { > > + .read_raw = mcp9808_read_raw, > > + .write_raw = mcp9808_write_raw, > > + .attrs = &mcp9808_attribute_group, > > + .driver_module = THIS_MODULE, > > +}; > > + > > +static bool mcp9808_check_id(struct i2c_client *client) > > +{ > > + int mid, did; > > + > > + mid = i2c_smbus_read_word_swapped(client, MCP9808_REG_MANUF_ID); > > + if (mid < 0) > > + return false; > You are potentially eating more informative errors here. I'd have preferred > a function of the form > static int mcp9808_check_id(struct i2c_client *client) that simply > returned -ENODEV if the reads were good but the id wrong. In other error > cases it could return the result of the i2c calls. > > These are the first i2c calls, so if there is something going wrong > with the i2c controller or similar, we definitely don't want it to > simply indicate that the wrong device was present. got it! > > + > > + did = i2c_smbus_read_word_swapped(client, MCP9808_REG_DEVICE_ID); > > + if (did < 0) > > + return false; > > + > > + return ((mid == MCP9808_MANUF_ID) && > > + ((did & MCP9808_DEVICE_ID_MASK) == MCP9808_DEVICE_ID)); > > +} > > + > > +static int mcp9808_probe(struct i2c_client *client, > > + const struct i2c_device_id *id) > > +{ > > + struct iio_dev *indio_dev; > > + struct mcp9808_data *data; > > + int ret; > > + > > + if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_WORD_DATA > > + | I2C_FUNC_SMBUS_WRITE_BYTE_DATA)) > > + return -EOPNOTSUPP; > > + > > + if (!mcp9808_check_id(client)) { > > + dev_err(&client->dev, "no MCP9808 sensor\n"); > > + return -ENODEV; > > + } > > + > > + indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data)); > > + if (!indio_dev) > > + return -ENOMEM; > > + > > + data = iio_priv(indio_dev); > > + i2c_set_clientdata(client, indio_dev); > > + data->client = client; > > + mutex_init(&data->lock); > > + > > + indio_dev->dev.parent = &client->dev; > > + indio_dev->name = id->name; > > + indio_dev->info = &mcp9808_info; > > + indio_dev->modes = INDIO_DIRECT_MODE; > > + indio_dev->channels = mcp9808_channels; > > + indio_dev->num_channels = ARRAY_SIZE(mcp9808_channels); > > + > > + /* set config register to power-on default */ > > + ret = i2c_smbus_write_word_swapped(data->client, MCP9808_REG_CONFIG, > > + MCP9808_CONFIG_DEFAULT); > > + if (ret < 0) > > + return ret; > > + > > + /* set resolution register to power-on default */ > > + ret = mcp9808_set_resolution(data, MCP9808_RES_DEFAULT); > > + if (ret < 0) > > + return ret; > > + > > + return iio_device_register(indio_dev); > > +} > > + > > +static int mcp9808_shutdown(struct mcp9808_data *data) > > +{ > > + return i2c_smbus_write_word_swapped(data->client, MCP9808_REG_CONFIG, > > + MCP9808_CONFIG_SHUTDOWN); > > +} > > + > > +static int mcp9808_remove(struct i2c_client *client) > > +{ > > + struct iio_dev *indio_dev = i2c_get_clientdata(client); > > + > > + iio_device_unregister(indio_dev); > > + > > + return mcp9808_shutdown(iio_priv(indio_dev)); > > +} > > + > > +#ifdef CONFIG_PM_SLEEP > > +static int mcp9808_suspend(struct device *dev) > > +{ > > + struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev)); > > + > > + return mcp9808_shutdown(iio_priv(indio_dev)); > > +} > > + > > +static int mcp9808_resume(struct device *dev) > > +{ > > + struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev)); > > + struct mcp9808_data *data = iio_priv(indio_dev); > > + int ret; > > + > > + ret = mcp9808_set_resolution(data, mcp9808_res[data->res_index][0]); > > + if (ret < 0) > > + return ret; > > + > > + return i2c_smbus_write_word_swapped(data->client, MCP9808_REG_CONFIG, > > + MCP9808_CONFIG_DEFAULT); > > +} > > +#endif > > + > > +static SIMPLE_DEV_PM_OPS(mcp9808_pm_ops, mcp9808_suspend, mcp9808_resume); > > + > > +static const struct i2c_device_id mcp9808_id[] = { > > + { "mcp9808", 0 }, > > + { } > > +}; > > +MODULE_DEVICE_TABLE(i2c, mcp9808_id); > > + > > +static struct i2c_driver mcp9808_driver = { > > + .driver = { > > + .name = "mcp9808", > > + .pm = &mcp9808_pm_ops, > > + }, > > + .probe = mcp9808_probe, > > + .remove = mcp9808_remove, > > + .id_table = mcp9808_id, > > +}; > > +module_i2c_driver(mcp9808_driver); > > + > > +MODULE_AUTHOR("Alison Schofield <amsfield22@xxxxxxxxx>"); > > +MODULE_DESCRIPTION("MCP9808 Temperature Sensor Driver"); > > +MODULE_LICENSE("GPL v2"); > > > -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
