> -----Original Message----- > From: Jonathan Cameron [mailto:jic23@xxxxxxxxxx] > Sent: Saturday, April 16, 2016 1:53 PM > To: Breana, Tiberiu A <tiberiu.a.breana@xxxxxxxxx>; linux- > iio@xxxxxxxxxxxxxxx > Subject: Re: [PATCH 1/2] iio: temperature: Add support for AM2315 > > On 15/04/16 16:36, Tiberiu Breana wrote: > > Add basic support for the Aosong AM2315 ambient temperature and > > humidity sensor. > > Includes support for raw readings and ACPI detection. > > > > Datasheet: > > http://www.aosong.com/asp_bin/Products/en/AM2315.pdf > > > > Signed-off-by: Tiberiu Breana <tiberiu.a.breana@xxxxxxxxx> > I think I'd prefer this in drivers/iio/humidty/. If people just want a > temperature sensor they'll not use the much more complex device needed > to measure humidity, whereas to measure humidity you have to know what > the temperature is. Hence I'd argue that here the humidity side of the > device in the primary one. > 'Interestingly' temperature and humidity seems to sound better to me in > English than humidity and temperature... hohum. > > I suppose we could roll these two directories together if people think that is > sensible? Sounds logical. I'll move it to iio/humidity. > > Various little bits inline.. > > Jonathan > > --- > > drivers/iio/temperature/Kconfig | 10 ++ > > drivers/iio/temperature/Makefile | 1 + > > drivers/iio/temperature/am2315.c | 216 > > +++++++++++++++++++++++++++++++++++++++ > > 3 files changed, 227 insertions(+) > > create mode 100644 drivers/iio/temperature/am2315.c > > > > diff --git a/drivers/iio/temperature/Kconfig > > b/drivers/iio/temperature/Kconfig index c4664e5..79e4409 100644 > > --- a/drivers/iio/temperature/Kconfig > > +++ b/drivers/iio/temperature/Kconfig > > @@ -3,6 +3,16 @@ > > # > > menu "Temperature sensors" > > > > +config AM2315 > > + tristate "Aosong AM2315 temperature and humidity sensor" > > + depends on I2C > > + help > > + If you say yes here you get support for the Aosong AM2315 > > + ambient temperature and humidity sensor. > > + > > + This driver can also be built as a module. If so, the module will > > + be called am2315. > > + > > 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..2e7521b 100644 > > --- a/drivers/iio/temperature/Makefile > > +++ b/drivers/iio/temperature/Makefile > > @@ -2,6 +2,7 @@ > > # Makefile for industrial I/O temperature drivers # > > > > +obj-$(CONFIG_AM2315) += am2315.o > > obj-$(CONFIG_MLX90614) += mlx90614.o > > obj-$(CONFIG_TMP006) += tmp006.o > > obj-$(CONFIG_TSYS01) += tsys01.o > > diff --git a/drivers/iio/temperature/am2315.c > > b/drivers/iio/temperature/am2315.c > > new file mode 100644 > > index 0000000..2642bae > > --- /dev/null > > +++ b/drivers/iio/temperature/am2315.c > > @@ -0,0 +1,216 @@ > > +/** > > + * Aosong AM2315 ambient temperature & humidity sensor driver > > + * > > + * Copyright (c) 2016, Intel Corporation. > > + * > > + * This file is subject to the terms and conditions of version 2 of > > + * the GNU General Public License. See the file COPYING in the main > > + * directory of this archive for more details. > > + * > > + * 7-bit I2C address: 0x5C. > > + */ > > + > > +#include <linux/acpi.h> > > +#include <linux/delay.h> > > +#include <linux/i2c.h> > > +#include <linux/kernel.h> > > +#include <linux/module.h> > > +#include <linux/iio/iio.h> > > +#include <linux/iio/sysfs.h> > > + > > +#define AM2315_REG_HUM_MSB 0x00 > > +#define AM2315_REG_HUM_LSB 0x01 > > +#define AM2315_REG_TEMP_MSB 0x02 > > +#define AM2315_REG_TEMP_LSB 0x03 > > + > > +#define AM2315_FUNCTION_READ 0x03 > > +#define AM2315_HUM_OFFSET 0 > > +#define AM2315_TEMP_OFFSET 2 > > + > > +#define AM2315_DRIVER_NAME "am2315" > > + > > +struct am2315_data { > > + struct i2c_client *client; > > +}; > > + > > +static const struct iio_chan_spec am2315_channels[] = { > > + { > > + .type = IIO_HUMIDITYRELATIVE, > > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | > > + BIT(IIO_CHAN_INFO_SCALE) > > + }, > > + { > > + .type = IIO_TEMP, > > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | > > + BIT(IIO_CHAN_INFO_SCALE) > > + }, > > +}; > > + > > +/* CRC calculation algorithm, as specified in the datasheet (page > > +13). */ static u16 am2315_crc(u8 *data, u8 nr_bytes) > > I think this 'might' be reverse CRC16-IBM according to wikipedia... > Which is obscure enough that it probably doesn't have a generic > implementation already in the kernel... > > There is one other user of this polynomial that I can fine, not enough to > justify a common implementation at this point I guess. > > Note that 16bit polynomials tend to get implemented as lookup tables if > generalized. Speed isn't that important here though I guess. Given the already slow nature of the device - see sleep time before data retrieval, recommended 2s wait time before measurements, indeed, speed is not very important here. > > > +{ > > + int i; > > + u16 crc = 0xffff; > > + > > + while (nr_bytes--) { > > + crc ^= *data++; > > + for (i = 0; i < 8; i++) { > > + if (crc & 0x01) { > > + crc >>= 1; > > + crc ^= 0xA001; > > + } else { > > + crc >>= 1; > > + } > > + } > > + } > > + > > + return crc; > > +} > > + > > +/* Simple function that sends a few bytes to the device to wake it > > +up. */ > We always like novel hardware corners <groans> > > +static inline void am2315_ping(struct i2c_client *client) { > > + i2c_smbus_read_byte_data(client, AM2315_REG_HUM_MSB); } > > + > > +static int am2315_read_data(struct i2c_client *client, u8 *rx_buf) { > > + int ret; > > + /* tx_buf format: <function code> <start addr> <nr of regs to read> > */ > > + u8 tx_buf[3] = { AM2315_FUNCTION_READ, > AM2315_REG_HUM_MSB, 4 }; > > + u16 crc; > > + > > + /* First wake up the device. */ > > + am2315_ping(client); > > + > > + ret = i2c_master_send(client, tx_buf, sizeof(tx_buf)); > > + if (ret < 0) { > > + dev_err(&client->dev, "failed to send read request\n"); > > + return ret; > > + } > > + /* Wait 2-3 ms, then read back the data sent by the device. */ > > + usleep_range(2000, 3000); > > + > > + ret = i2c_master_recv(client, rx_buf, sizeof(rx_buf)); > > + if (ret < 0) { > > + dev_err(&client->dev, "failed to read sensor data\n"); > > + return ret; > > + } > > + /* > > + * Do a CRC check on the data and compare it to the value > > + * calculated by the device. > > + */ > > + crc = am2315_crc(rx_buf, sizeof(rx_buf) - 2); > > + if ((crc & 0xff) != rx_buf[6] || (crc >> 8) != rx_buf[7]) { > > + dev_err(&client->dev, "failed to verify sensor data\n"); > > + return -1; > Provide a more informative error code... -EIO maybe... Will do this in v2. > > + } > > + > > + return ret; > > +} > > + > > +static int am2315_read_raw(struct iio_dev *indio_dev, > > + struct iio_chan_spec const *chan, > > + int *val, int *val2, long mask) { > > + int ret; > > + s16 value; > > + u8 rx_buf[8]; > > + u8 offset; > > + struct am2315_data *data = iio_priv(indio_dev); > > + struct i2c_client *client = data->client; > > + > > + switch (mask) { > > + case IIO_CHAN_INFO_RAW: > > + offset = (chan->type == IIO_HUMIDITYRELATIVE) ? > > + AM2315_HUM_OFFSET : > AM2315_TEMP_OFFSET; > > + /* Do a bulk data read, then pick out what we need. */ > > + ret = am2315_read_data(client, rx_buf); > > + if (ret < 0) > > + return ret; > > + /* > > + * rx_buf format: > > + * <function code> <number of registers read> > > + * <humidity MSB> <humidity LSB> <temp MSB> <temp LSB> > > + * <CRC LSB> <CRC MSB> > > + */ > > + value = (rx_buf[offset + 2] << 8) | rx_buf[offset + 3]; > > + *val = value / 10; > > + *val2 = (value % 10) * 100000; > > So for temperature this is outputing a value in degrees? Give you are > outputting as raw, don't divide by 10 and just return IIO_VAL_INT; Leave the > maths to userspace. Ok, will output raw values in v2. > > > + return IIO_VAL_INT_PLUS_MICRO; > > + case IIO_CHAN_INFO_SCALE: > > + *val = 0; > > + *val2 = 100000; > For temp to be presented in standard base units (milli degrees C) this should > be > *val2 = 1000 I think... > > Same for humidity - obviously this will change if you don't do the divide by > 10 in the raw read out. Actually if we're aiming for milli units, the scale should be 100. > > > > + return IIO_VAL_INT_PLUS_MICRO; > > + } > > + > > + return -EINVAL; > > +} > > + > > +static const struct iio_info am2315_info = { > > + .driver_module = THIS_MODULE, > > + .read_raw = am2315_read_raw, > > +}; > > + > > +static int am2315_probe(struct i2c_client *client, > > + const struct i2c_device_id *id) > > +{ > > + struct iio_dev *indio_dev; > > + struct am2315_data *data; > > + > > + indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data)); > > + if (!indio_dev) { > > + dev_err(&client->dev, "iio allocation failed!\n"); > > + return -ENOMEM; > > + } > > + > > + data = iio_priv(indio_dev); > > + data->client = client; > > + i2c_set_clientdata(client, indio_dev); > > + > > + indio_dev->dev.parent = &client->dev; > > + indio_dev->info = &am2315_info; > > + indio_dev->name = AM2315_DRIVER_NAME; > > + indio_dev->modes = INDIO_DIRECT_MODE; > > + indio_dev->channels = am2315_channels; > > + indio_dev->num_channels = ARRAY_SIZE(am2315_channels); > > + > > + return iio_device_register(indio_dev); } > > + > > +static int am2315_remove(struct i2c_client *client) { > > + struct iio_dev *indio_dev = i2c_get_clientdata(client); > > + > > If all you have in remove is the iio_device_unregister, you are safe to use > devm_iio_device_unregister in probe and drop the remove entirely. (It's > relatively rare that there is nothing to do in remove, but it is to simiplify that > rare case that we have the devm_iio_device_register > call) I initially used the devm variant, but to reduce patch 2 diff, I've decided to add the .remove function here and in v2 just add a iio_triggered_buffer_cleanup call. If you don't agree with this, I can change it. > > + iio_device_unregister(indio_dev); > > + > > + return 0; > > +} > > + > > +static const struct i2c_device_id am2315_i2c_id[] = { > > + {"am2315", 0}, > > + {} > > +}; > > + > > +static const struct acpi_device_id am2315_acpi_id[] = { > > + {"AOS2315", 0}, > > + {} > > +}; > > + > > +MODULE_DEVICE_TABLE(acpi, am2315_acpi_id); > > + > > +static struct i2c_driver am2315_driver = { > > + .driver = { > > + .name = "am2315", > > + .acpi_match_table = ACPI_PTR(am2315_acpi_id), > > + }, > > + .probe = am2315_probe, > > + .remove = am2315_remove, > > + .id_table = am2315_i2c_id, > > +}; > > + > > +module_i2c_driver(am2315_driver); > > + > > +MODULE_AUTHOR("Tiberiu Breana <tiberiu.a.breana@xxxxxxxxx>"); > > +MODULE_DESCRIPTION("Aosong AM2315 ambient temperature & > humidity > > +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
