Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754865AbcC1PbZ (ORCPT ); Mon, 28 Mar 2016 11:31:25 -0400 Received: from ns.pmeerw.net ([84.19.176.92]:60995 "EHLO pmeerw.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754757AbcC1PbW (ORCPT ); Mon, 28 Mar 2016 11:31:22 -0400 Date: Mon, 28 Mar 2016 17:31:18 +0200 (CEST) From: Peter Meerwald-Stadler To: Crestez Dan Leonard cc: Jonathan Cameron , linux-iio@vger.kernel.org, linux-kernel@vger.kernel.org, Hartmut Knaack , Lars-Peter Clausen , Daniel Baluta Subject: Re: [PATCH v2] hp206c: Initial support for reading sensor values In-Reply-To: Message-ID: References: User-Agent: Alpine 2.02 (DEB 1266 2009-07-14) MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 14007 Lines: 521 > Changes since the first version: > * Use data->client instead of to_i2c_client(indio_dev->dev.parent) > * Adjust i2c_check_functionality bits requested. > > I also fixed gitconfig so it won't create attachments or suppress cc. I've never seen the first version of the patch; anyway, comments below... > Signed-off-by: Crestez Dan Leonard > --- > drivers/iio/pressure/Kconfig | 10 + > drivers/iio/pressure/Makefile | 1 + > drivers/iio/pressure/hp206c.c | 416 ++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 427 insertions(+) > create mode 100644 drivers/iio/pressure/hp206c.c > > diff --git a/drivers/iio/pressure/Kconfig b/drivers/iio/pressure/Kconfig > index 31c0e1f..8de0192 100644 > --- a/drivers/iio/pressure/Kconfig > +++ b/drivers/iio/pressure/Kconfig > @@ -148,4 +148,14 @@ config T5403 > To compile this driver as a module, choose M here: the module > will be called t5403. > > +config HP206C > + tristate "HOPERF HP206C precision barometer and altimeter sensor" > + depends on I2C > + help > + Say yes here to build support for the HOPREF HP206C precision > + barometer and altimeter sensor. > + > + This driver can also be built as a module. If so, the module will > + be called hp206c. > + > endmenu > diff --git a/drivers/iio/pressure/Makefile b/drivers/iio/pressure/Makefile > index d336af1..6e60863 100644 > --- a/drivers/iio/pressure/Makefile > +++ b/drivers/iio/pressure/Makefile > @@ -17,6 +17,7 @@ obj-$(CONFIG_IIO_ST_PRESS) += st_pressure.o > st_pressure-y := st_pressure_core.o > st_pressure-$(CONFIG_IIO_BUFFER) += st_pressure_buffer.o > obj-$(CONFIG_T5403) += t5403.o > +obj-$(CONFIG_HP206C) += hp206c.o > > obj-$(CONFIG_IIO_ST_PRESS_I2C) += st_pressure_i2c.o > obj-$(CONFIG_IIO_ST_PRESS_SPI) += st_pressure_spi.o > diff --git a/drivers/iio/pressure/hp206c.c b/drivers/iio/pressure/hp206c.c > new file mode 100644 > index 0000000..9c2c49d > --- /dev/null > +++ b/drivers/iio/pressure/hp206c.c > @@ -0,0 +1,416 @@ > +/* > + * hp206c.c - HOPERF HP206C precision barometer and altimeter sensor > + * > + * 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 slave address 0x76) > + * > + * Datasheet: > + * http://www.hoperf.com/upload/sensor/HP206C_DataSheet_EN_V2.0.pdf > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +/* I2C commands: */ > +#define HP206C_CMD_SOFT_RST 0x06 > + > +#define HP206C_CMD_ADC_CVT 0x40 > + > +#define HP206C_CMD_ADC_CVT_OSR_4096 0x00 > +#define HP206C_CMD_ADC_CVT_OSR_2048 0x04 > +#define HP206C_CMD_ADC_CVT_OSR_1024 0x08 > +#define HP206C_CMD_ADC_CVT_OSR_512 0x0c > +#define HP206C_CMD_ADC_CVT_OSR_256 0x10 > +#define HP206C_CMD_ADC_CVT_OSR_128 0x14 > + > +#define HP206C_CMD_ADC_CVT_CHNL_PT 0x00 > +#define HP206C_CMD_ADC_CVT_CHNL_T 0x02 > + > +#define HP206C_CMD_READ_P 0x30 > +#define HP206C_CMD_READ_T 0x32 > + > +#define HP206C_CMD_READ_REG 0x80 > +#define HP206C_CMD_WRITE_REG 0xc0 > + > +#define HP206C_REG_INT_EN 0x0b > +#define HP206C_REG_INT_CFG 0x0c > + > +#define HP206C_REG_INT_SRC 0x0d > +#define HP206C_FLAG_DEV_RDY 0x40 > + > +#define HP206C_REG_PARA 0x0f > +#define HP206C_FLAG_CMPS_EN 0x80 > + > +/* Maximum spin for DEV_RDY */ > +#define HP206C_MAX_DEV_RDY_WAIT_COUNT 20 > +#define HP206C_DEV_RDY_WAIT_US 20000 > + > +struct hp206c_data { > + struct mutex mutex; > + struct i2c_client *client; > + u8 temp_osr_index; > + u8 pres_osr_index; I'd rather use type int for the indices > +}; > + > +struct hp206c_osr_setting { > + u8 osr_mask; > + unsigned int temp_conv_time_us; > + unsigned int pres_conv_time_us; > +}; > + > +/* Data from Table 5 in datasheet. */ > +static const struct hp206c_osr_setting hp206c_osr_settings[] = { > + { HP206C_CMD_ADC_CVT_OSR_4096, 65600, 131100}, > + { HP206C_CMD_ADC_CVT_OSR_2048, 32800, 65600}, > + { HP206C_CMD_ADC_CVT_OSR_1024, 16400, 32800}, > + { HP206C_CMD_ADC_CVT_OSR_512, 8200, 16400}, > + { HP206C_CMD_ADC_CVT_OSR_256, 4100, 8200}, > + { HP206C_CMD_ADC_CVT_OSR_128, 2100, 4100}, > +}; maybe whitespace before } > +static const int hp206c_osr_rates[] = { 4096, 2048, 1024, 512, 256, 128 }; > +static const char hp206c_osr_rates_str[] = "4096 2048 1024 512 256 128"; > + > +static inline int hp206c_read_reg(struct i2c_client *client, u8 reg) > +{ > + return i2c_smbus_read_byte_data(client, HP206C_CMD_READ_REG | reg); > +} > + > +static inline int hp206c_write_reg(struct i2c_client *client, u8 reg, u8 val) > +{ > + return i2c_smbus_write_byte_data(client, > + HP206C_CMD_WRITE_REG | reg, val); > +} > + > +static int hp206c_read_20bit(struct i2c_client *client, u8 cmd) > +{ > + int ret; > + uint8_t values[3]; maybe use u8 here as well > + > + ret = i2c_smbus_read_i2c_block_data(client, cmd, 3, values); > + if (ret < 0) > + return ret; > + if (ret != 3) > + return -EIO; > + return ((values[0] & 0xF) << 16) | (values[1] << 8) | (values[2]); > +} > + > +/* Spin for max 160ms until DEV_RDY is 1, or return error. */ > +static int hp206c_wait_dev_rdy(struct iio_dev *indio_dev) > +{ > + int ret; > + int count = 0; > + struct hp206c_data *data = iio_priv(indio_dev); > + struct i2c_client *client = data->client; > + > + while (++count <= HP206C_MAX_DEV_RDY_WAIT_COUNT) { > + ret = hp206c_read_reg(client, HP206C_REG_INT_SRC); > + if (ret < 0) { > + dev_err(&indio_dev->dev, "Failed READ_REG INT_SRC: %d\n", ret); > + return ret; > + } > + if (ret & HP206C_FLAG_DEV_RDY) > + return 0; > + usleep_range(HP206C_DEV_RDY_WAIT_US, HP206C_DEV_RDY_WAIT_US * 3 / 2); > + } > + return -ETIME; most drivers do -ETIMEDOUT (ak8975 and gp2ap020a00f do -ETIME), others do -EIO > +} > + > +static int hp206c_set_compensation(struct i2c_client *client, bool enabled) > +{ > + int val; > + > + val = hp206c_read_reg(client, HP206C_REG_PARA); > + if (val < 0) > + return val; > + if (enabled) > + val |= HP206C_FLAG_CMPS_EN; > + else > + val &= ~HP206C_FLAG_CMPS_EN; > + > + return hp206c_write_reg(client, HP206C_REG_PARA, val); > +} > + > +/* Do a soft reset */ > +static int hp206c_soft_reset(struct iio_dev *indio_dev) > +{ > + int ret; > + struct hp206c_data *data = iio_priv(indio_dev); > + struct i2c_client *client = data->client; > + > + ret = i2c_smbus_write_byte(client, HP206C_CMD_SOFT_RST); > + if (ret) { > + dev_err(&client->dev, "Failed to reset device: %d\n", ret); > + return ret; > + } whitespace here > + usleep_range(400, 600); and here > + ret = hp206c_wait_dev_rdy(indio_dev); > + if (ret) { > + dev_err(&client->dev, "Device not ready after soft reset: %d\n", ret); > + return ret; > + } > + > + ret = hp206c_set_compensation(client, true); > + if (ret) > + dev_err(&client->dev, "Failed to enable compensation: %d\n", ret); > + return ret; > +} > + > +static int hp206c_conv_and_read(struct iio_dev *indio_dev, > + u8 conv_cmd, u8 read_cmd, > + unsigned int sleep_us) > +{ > + int ret; > + struct hp206c_data *data = iio_priv(indio_dev); > + struct i2c_client *client = data->client; > + > + ret = hp206c_wait_dev_rdy(indio_dev); > + if (ret < 0) { > + dev_err(&indio_dev->dev, "Device not ready: %d\n", ret); > + return ret; > + } > + > + ret = i2c_smbus_write_byte(client, conv_cmd); > + if (ret < 0) { > + dev_err(&indio_dev->dev, "Failed convert: %d\n", ret); > + return ret; > + } > + > + usleep_range(sleep_us, sleep_us * 3 / 2); > + > + ret = hp206c_wait_dev_rdy(indio_dev); > + if (ret < 0) { > + dev_err(&indio_dev->dev, "Device not ready: %d\n", ret); > + return ret; > + } > + > + ret = hp206c_read_20bit(client, read_cmd); > + if (ret < 0) { > + dev_err(&indio_dev->dev, "Failed read: %d\n", ret); > + return ret; the last error handling could be similar to _soft_reset() > + } > + > + return ret; > +} > + > +static int hp206c_read_raw(struct iio_dev *indio_dev, > + struct iio_chan_spec const *chan, int *val, > + int *val2, long mask) > +{ > + int ret; > + struct hp206c_data *data = iio_priv(indio_dev); > + const struct hp206c_osr_setting *osr_setting; > + u8 conv_cmd; > + > + mutex_lock(&data->mutex); > + > + switch (mask) { > + case IIO_CHAN_INFO_OVERSAMPLING_RATIO: > + switch (chan->type) { > + case IIO_TEMP: > + *val = hp206c_osr_rates[data->temp_osr_index]; > + ret = IIO_VAL_INT; > + break; > + > + case IIO_PRESSURE: > + *val = hp206c_osr_rates[data->pres_osr_index]; > + ret = IIO_VAL_INT; > + break; > + default: > + ret = -EINVAL; > + } > + break; > + > + case IIO_CHAN_INFO_PROCESSED: > + switch (chan->type) { > + case IIO_TEMP: > + osr_setting = &hp206c_osr_settings[data->temp_osr_index]; > + conv_cmd = HP206C_CMD_ADC_CVT | > + osr_setting->osr_mask | > + HP206C_CMD_ADC_CVT_CHNL_T; > + ret = hp206c_conv_and_read(indio_dev, > + conv_cmd, > + HP206C_CMD_READ_T, > + osr_setting->temp_conv_time_us); > + > + if (ret >= 0) { > + s32 sret = ret; > + /* Sign-extend from bit 20 to 32 */ > + if (sret & 0x80000) > + sret |= 0xFFF00000; use sign_extend32() > + *val = sret / 100; > + *val2 = sret % 100 * 10000; parenthesis around sret % 100 to be clear > + ret = IIO_VAL_INT_PLUS_MICRO; > + } > + break; > + > + case IIO_PRESSURE: > + osr_setting = &hp206c_osr_settings[data->pres_osr_index]; > + conv_cmd = HP206C_CMD_ADC_CVT | > + osr_setting->osr_mask | > + HP206C_CMD_ADC_CVT_CHNL_PT; > + ret = hp206c_conv_and_read(indio_dev, > + conv_cmd, > + HP206C_CMD_READ_P, > + osr_setting->pres_conv_time_us); > + > + if (ret >= 0) { > + *val = ret / 1000; > + *val2 = ret % 1000 * 1000; > + ret = IIO_VAL_INT_PLUS_MICRO; > + } > + break; > + default: > + ret = -EINVAL; > + } > + break; > + > + default: > + ret = -EINVAL; > + } > + > + mutex_unlock(&data->mutex); > + return ret; > +} > + > +static int hp206c_write_raw(struct iio_dev *indio_dev, > + struct iio_chan_spec const *chan, > + int val, int val2, long mask) > +{ > + int ret; > + struct hp206c_data *data = iio_priv(indio_dev); > + > + mutex_lock(&data->mutex); > + switch (mask) { > + case IIO_CHAN_INFO_OVERSAMPLING_RATIO: a simple if (mask != IIO_CHAN_INFO_OVERSAMPLING_RATIO) return -EINVAL; above might be clearer > + switch (chan->type) { > + case IIO_TEMP: > + data->temp_osr_index = find_closest_descending(val, > + hp206c_osr_rates, ARRAY_SIZE(hp206c_osr_rates)); > + ret = 0; initialize ret = 0 above > + break; > + case IIO_PRESSURE: > + data->pres_osr_index = find_closest_descending(val, > + hp206c_osr_rates, ARRAY_SIZE(hp206c_osr_rates)); > + ret = 0; > + break; > + default: > + ret = -EINVAL; > + } > + break; > + default: > + ret = -EINVAL; > + } > + > + mutex_unlock(&data->mutex); > + return ret; > +} > + > +static const struct iio_chan_spec hp206c_channels[] = { > + { > + .type = IIO_TEMP, > + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) | there is no processing going on, use _RAW and express the scaling with _SCALE > + BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), > + }, > + { > + .type = IIO_PRESSURE, > + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) | > + BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), > + } > +}; > + > +static IIO_CONST_ATTR_SAMP_FREQ_AVAIL(hp206c_osr_rates_str); > + > +static struct attribute *hp206c_attributes[] = { > + &iio_const_attr_sampling_frequency_available.dev_attr.attr, > + NULL, > +}; > + > +static const struct attribute_group hp206c_attribute_group = { > + .attrs = hp206c_attributes, > +}; > + > +static const struct iio_info hp206c_info = { > + .attrs = &hp206c_attribute_group, > + .read_raw = hp206c_read_raw, > + .write_raw = hp206c_write_raw, > + .driver_module = THIS_MODULE, > +}; > + > +static int hp206c_probe(struct i2c_client *client, > + const struct i2c_device_id *id) > +{ > + struct iio_dev *indio_dev; > + struct hp206c_data *data; > + int ret; > + > + if (!i2c_check_functionality(client->adapter, > + I2C_FUNC_SMBUS_BYTE | > + I2C_FUNC_SMBUS_BYTE_DATA | > + I2C_FUNC_SMBUS_READ_I2C_BLOCK)) { > + dev_err(&client->dev, "Adapter does not support " > + "all required i2c functionality\n"); > + return -ENODEV; > + } > + > + indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data)); > + if (!indio_dev) > + return -ENOMEM; > + > + data = iio_priv(indio_dev); > + data->client = client; > + mutex_init(&data->mutex); > + > + indio_dev->info = &hp206c_info; > + indio_dev->name = id->name; > + indio_dev->dev.parent = &client->dev; > + indio_dev->modes = INDIO_DIRECT_MODE; > + indio_dev->channels = hp206c_channels; > + indio_dev->num_channels = ARRAY_SIZE(hp206c_channels); > + > + i2c_set_clientdata(client, indio_dev); > + > + /* Do a soft reset on probe */ > + ret = hp206c_soft_reset(indio_dev); > + if (ret) { > + dev_err(&client->dev, "Failed to reset on startup: %d\n", ret); > + /* Maybe it will recover later? */ I suggest to return -ENODEV; > + } > + > + return devm_iio_device_register(&client->dev, indio_dev); > +} > + > +static const struct i2c_device_id hp206c_id[] = { > + {"hp206c"}, > + {} > +}; > + > +static const struct acpi_device_id hp206c_acpi_match[] = { > + {"HOP206C", 0}, > + { }, > +}; > +MODULE_DEVICE_TABLE(acpi, hp206c_acpi_match); > + > +static struct i2c_driver hp206c_driver = { > + .probe = hp206c_probe, > + .id_table = hp206c_id, > + .driver = { > + .name = "hp206c", > + .acpi_match_table = ACPI_PTR(hp206c_acpi_match), > + }, > +}; > + > +module_i2c_driver(hp206c_driver); > + > +MODULE_DESCRIPTION("HOPERF HP206C precision barometer and altimeter sensor"); > +MODULE_AUTHOR("Leonard Crestez "); > +MODULE_LICENSE("GPL v2"); > -- Peter Meerwald-Stadler +43-664-2444418 (mobile)