Re: [PATCH] iio: pressure: hp03: Add Hope RF HP03 sensor support

[Marek Vasut <marex@denx.de>]

On 04/03/2016 10:21 AM, Jonathan Cameron wrote:
> On 01/04/16 16:11, Marek Vasut wrote:
>> Add support for HopeRF pressure and temperature sensor.
>>
>> This device uses two fixed I2C addresses, one for storing
>> calibration coefficients and another for accessing the ADC.
>>
>> Signed-off-by: Marek Vasut <marex@denx.de>
>> Cc: Matt Ranostay <mranostay@gmail.com>
>> Cc: Jonathan Cameron <jic23@kernel.org>
> Only one real comment inline - I'd try to unwind the gpio on an error
> in the i2c read out.  Feels a little more consistent to do that than to
> just leave it in whatever state it is in when the error occurs.

Stupid me, certainly fixed, thanks!

> Jonathan
>> ---
>>  .../devicetree/bindings/iio/pressure/hp03.txt      |  13 +
>>  drivers/iio/pressure/Kconfig                       |  11 +
>>  drivers/iio/pressure/Makefile                      |   1 +
>>  drivers/iio/pressure/hp03.c                        | 290 +++++++++++++++++++++
>>  4 files changed, 315 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/iio/pressure/hp03.txt
>>  create mode 100644 drivers/iio/pressure/hp03.c
>>
>> diff --git a/Documentation/devicetree/bindings/iio/pressure/hp03.txt b/Documentation/devicetree/bindings/iio/pressure/hp03.txt
>> new file mode 100644
>> index 0000000..c9551b2
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/iio/pressure/hp03.txt
>> @@ -0,0 +1,13 @@
>> +HopeRF HP03 digital pressure/temperature sensors
>> +
>> +Required properties:
>> +- compatible: must be "hoperf,hp03"
>> +- xclr-gpio:  must be device tree identifier of the XCLR pin
>> +
>> +Example:
>> +
>> +hp03@0x77 {
>> +	compatible = "hoperf,hp03";
>> +	reg = <0x77>;
>> +	xclr-gpio = <&portc 0 0x0>;
>> +};
>> diff --git a/drivers/iio/pressure/Kconfig b/drivers/iio/pressure/Kconfig
>> index 31c0e1f..3f2b1ed 100644
>> --- a/drivers/iio/pressure/Kconfig
>> +++ b/drivers/iio/pressure/Kconfig
>> @@ -30,6 +30,17 @@ config HID_SENSOR_PRESS
>>  	  To compile this driver as a module, choose M here: the module
>>  	  will be called hid-sensor-press.
>>  
>> +config HP03
>> +	tristate "Hope RF HP03 temperature and pressure sensor driver"
>> +	depends on I2C
>> +	select REGMAP_I2C
>> +	help
>> +	  Say yes here to build support for Hope RF HP03 pressure and
>> +	  temperature sensor.
>> +
>> +	  To compile this driver as a module, choose M here: the module
>> +	  will be called hp03.
>> +
>>  config MPL115
>>  	tristate
>>  
>> diff --git a/drivers/iio/pressure/Makefile b/drivers/iio/pressure/Makefile
>> index d336af1..cb21ab1 100644
>> --- a/drivers/iio/pressure/Makefile
>> +++ b/drivers/iio/pressure/Makefile
>> @@ -5,6 +5,7 @@
>>  # When adding new entries keep the list in alphabetical order
>>  obj-$(CONFIG_BMP280) += bmp280.o
>>  obj-$(CONFIG_HID_SENSOR_PRESS)   += hid-sensor-press.o
>> +obj-$(CONFIG_HP03) += hp03.o
>>  obj-$(CONFIG_MPL115) += mpl115.o
>>  obj-$(CONFIG_MPL115_I2C) += mpl115_i2c.o
>>  obj-$(CONFIG_MPL115_SPI) += mpl115_spi.o
>> diff --git a/drivers/iio/pressure/hp03.c b/drivers/iio/pressure/hp03.c
>> new file mode 100644
>> index 0000000..e8e80f5
>> --- /dev/null
>> +++ b/drivers/iio/pressure/hp03.c
>> @@ -0,0 +1,290 @@
>> +/*
>> + * Copyright (c) 2016 Marek Vasut <marex@denx.de>
>> + *
>> + * Driver for Hope RF HP03 digital temperature and pressure sensor.
>> + *
>> + * 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.
>> + *
> No need for the blank line here <nitpick of the day>

Fixed

>> + */
>> +
>> +#define pr_fmt(fmt) "hp03: " fmt
>> +
>> +#include <linux/module.h>
>> +#include <linux/delay.h>
>> +#include <linux/gpio/consumer.h>
>> +#include <linux/i2c.h>
>> +#include <linux/regmap.h>
>> +#include <linux/iio/iio.h>
>> +#include <linux/iio/sysfs.h>
>> +
>> +/*
>> + * The HP03 sensor occupies two fixed I2C addresses:
>> + *  0x50 ... read-only EEPROM with calibration data
>> + *  0x77 ... read-write ADC for pressure and temperature
>> + */
>> +#define HP03_EEPROM_ADDR		0x50
>> +#define HP03_ADC_ADDR			0x77
>> +
>> +#define HP03_EEPROM_CX_OFFSET		0x10
>> +#define HP03_EEPROM_AB_OFFSET		0x1e
>> +#define HP03_EEPROM_CD_OFFSET		0x20
>> +
>> +#define HP03_ADC_WRITE_REG		0xff
>> +#define HP03_ADC_READ_REG		0xfd
>> +#define HP03_ADC_READ_PRESSURE		0xf0	/* D1 in datasheet */
>> +#define HP03_ADC_READ_TEMP		0xe8	/* D2 in datasheet */
>> +
>> +struct hp03_priv {
>> +	struct i2c_client	*client;
>> +	struct mutex		lock;
>> +	struct gpio_desc	*xclr_gpio;
>> +
>> +	struct i2c_client	*eeprom_client;
>> +	struct regmap		*eeprom_regmap;
>> +
>> +	s32			pressure;	/* kPa */
>> +	s32			temp;		/* Deg. C */
>> +};
>> +
>> +static const struct iio_chan_spec hp03_channels[] = {
>> +	{
>> +		.type = IIO_PRESSURE,
>> +		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
>> +	},
>> +	{
>> +		.type = IIO_TEMP,
>> +		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
>> +	},
>> +};
>> +
>> +static bool hp03_is_writeable_reg(struct device *dev, unsigned int reg)
>> +{
>> +	return false;
>> +}
>> +
>> +static bool hp03_is_volatile_reg(struct device *dev, unsigned int reg)
>> +{
>> +	return false;
>> +}
>> +
>> +static const struct regmap_config hp03_regmap_config = {
>> +	.reg_bits	= 8,
>> +	.val_bits	= 8,
>> +
>> +	.max_register	= HP03_EEPROM_CD_OFFSET + 1,
>> +	.cache_type	= REGCACHE_RBTREE,
>> +
>> +	.writeable_reg	= hp03_is_writeable_reg,
>> +	.volatile_reg	= hp03_is_volatile_reg,
>> +};
>> +
>> +static int hp03_get_temp_pressure(struct hp03_priv *priv, const u8 reg)
>> +{
>> +	int ret;
>> +
>> +	ret = i2c_smbus_write_byte_data(priv->client, HP03_ADC_WRITE_REG, reg);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	msleep(50);	/* Wait for conversion to finish */
>> +
>> +	return i2c_smbus_read_word_data(priv->client, HP03_ADC_READ_REG);
>> +}
>> +
>> +static int hp03_update_temp_pressure(struct hp03_priv *priv)
>> +{
>> +	struct device *dev = &priv->client->dev;
>> +	u8 coefs[18];
>> +	u16 cx_val[7];
>> +	int ab_val, d1_val, d2_val, diff_val, dut, off, sens, x;
>> +	int i, ret;
>> +
>> +	/* Sample coefficients from EEPROM */
>> +	ret = regmap_bulk_read(priv->eeprom_regmap, HP03_EEPROM_CX_OFFSET,
>> +			       coefs, sizeof(coefs));
>> +	if (ret < 0) {
>> +		dev_err(dev, "Failed to read EEPROM (reg=%02x)\n",
>> +			HP03_EEPROM_CX_OFFSET);
>> +		return ret;
>> +	}
>> +
>> +	/* Sample Temperature and Pressure */
>> +	gpiod_set_value_cansleep(priv->xclr_gpio, 1);
>> +
>> +	ret = hp03_get_temp_pressure(priv, HP03_ADC_READ_PRESSURE);
>> +	if (ret < 0) {
>> +		dev_err(dev, "Failed to read pressure\n");
>> +		return ret;
> I'd be tempted to drop the gpio in these error conditions.

Standard failpath which sets the GPIO to 0 added, thanks.

>> +	}
>> +	d1_val = ret;
>> +
>> +	ret = hp03_get_temp_pressure(priv, HP03_ADC_READ_TEMP);
>> +	if (ret < 0) {
>> +		dev_err(dev, "Failed to read temperature\n");
>> +		return ret;
>> +	}
>> +	d2_val = ret;
>> +
>> +	gpiod_set_value_cansleep(priv->xclr_gpio, 0);
>> +
>> +	/* The Cx coefficients and Temp/Pressure values are MSB first. */
>> +	for (i = 0; i < 7; i++)
>> +		cx_val[i] = (coefs[2 * i] << 8) | (coefs[(2 * i) + 1] << 0);
>> +	d1_val = ((d1_val >> 8) & 0xff) | ((d1_val & 0xff) << 8);
>> +	d2_val = ((d2_val >> 8) & 0xff) | ((d2_val & 0xff) << 8);
>> +
>> +	/* Coefficient voodoo from the HP03 datasheet. */
>> +	if (d2_val >= cx_val[4])
>> +		ab_val = coefs[14];	/* A-value */
>> +	else
>> +		ab_val = coefs[15];	/* B-value */
>> +
>> +	diff_val = d2_val - cx_val[4];
>> +	dut = (ab_val * (diff_val >> 7) * (diff_val >> 7)) >> coefs[16];
>> +	dut = diff_val - dut;
>> +
>> +	off = (cx_val[1] + (((cx_val[3] - 1024) * dut) >> 14)) * 4;
>> +	sens = cx_val[0] + ((cx_val[2] * dut) >> 10);
>> +	x = ((sens * (d1_val - 7168)) >> 14) - off;
>> +
>> +	priv->pressure = ((x * 100) >> 5) + (cx_val[6] * 10);
>> +	priv->temp = 250 + ((dut * cx_val[5]) >> 16) - (dut >> coefs[17]);
>> +
>> +	return 0;
>> +}
>> +
>> +static int hp03_read_raw(struct iio_dev *indio_dev,
>> +			 struct iio_chan_spec const *chan,
>> +			 int *val, int *val2, long mask)
>> +{
>> +	struct hp03_priv *priv = iio_priv(indio_dev);
>> +	int ret;
>> +
>> +	mutex_lock(&priv->lock);
>> +	ret = hp03_update_temp_pressure(priv);
>> +	mutex_unlock(&priv->lock);
>> +
>> +	if (ret)
>> +		return ret;
>> +
>> +	switch (mask) {
>> +	case IIO_CHAN_INFO_PROCESSED:
>> +		switch (chan->type) {
>> +		case IIO_PRESSURE:
>> +			*val = priv->pressure / 100;
>> +			*val2 = (priv->pressure % 100) * 10000;
>> +			ret = IIO_VAL_INT_PLUS_MICRO;
>> +			break;
>> +		case IIO_TEMP:
>> +			*val = priv->temp / 100;
>> +			*val2 = (priv->temp % 100) * 10000;
>> +			ret = IIO_VAL_INT_PLUS_MICRO;
>> +			break;
>> +		default:
>> +			ret = -EINVAL;
>> +			break;
>> +		}
>> +		break;
>> +	default:
>> +		ret = -EINVAL;
>> +		break;
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>> +static const struct iio_info hp03_info = {
>> +	.driver_module	= THIS_MODULE,
>> +	.read_raw	= &hp03_read_raw,
>> +};
>> +
>> +static int hp03_probe(struct i2c_client *client,
>> +		      const struct i2c_device_id *id)
>> +{
>> +	struct device *dev = &client->dev;
>> +	struct iio_dev *indio_dev;
>> +	struct hp03_priv *priv;
>> +	int ret;
>> +
>> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*priv));
>> +	if (!indio_dev)
>> +		return -ENOMEM;
>> +
>> +	priv = iio_priv(indio_dev);
>> +	priv->client = client;
>> +	mutex_init(&priv->lock);
>> +
>> +	indio_dev->dev.parent = dev;
>> +	indio_dev->name = id->name;
>> +	indio_dev->channels = hp03_channels;
>> +	indio_dev->num_channels = ARRAY_SIZE(hp03_channels);
>> +	indio_dev->info = &hp03_info;
>> +	indio_dev->modes = INDIO_DIRECT_MODE;
>> +
>> +	priv->eeprom_client = i2c_new_dummy(client->adapter, HP03_EEPROM_ADDR);
>> +	if (!priv->eeprom_client) {
>> +		dev_err(dev, "New EEPROM I2C device failed\n");
>> +		return -ENODEV;
>> +	}
>> +
>> +	priv->eeprom_regmap = devm_regmap_init_i2c(priv->eeprom_client,
>> +						   &hp03_regmap_config);
>> +	if (IS_ERR(priv->eeprom_regmap)) {
>> +		dev_err(dev, "Failed to allocate EEPROM regmap\n");
>> +		ret = PTR_ERR(priv->eeprom_regmap);
>> +		goto err;
>> +	}
>> +
>> +	priv->xclr_gpio = devm_gpiod_get_index(dev, "xclr", 0, GPIOD_OUT_HIGH);
>> +	if (IS_ERR(priv->xclr_gpio)) {
>> +		dev_err(dev, "Failed to claim XCLR GPIO\n");
>> +		ret = PTR_ERR(priv->xclr_gpio);
>> +		goto err;
>> +	}
>> +
>> +	ret = devm_iio_device_register(dev, indio_dev);
>> +	if (ret) {
>> +		dev_err(dev, "Failed to register IIO device\n");
>> +		goto err;
>> +	}
>> +
>> +	i2c_set_clientdata(client, indio_dev);
>> +
>> +	return 0;
>> +
>> +err:
>> +	i2c_unregister_device(priv->eeprom_client);
>> +	return ret;
>> +}
>> +
>> +static int hp03_remove(struct i2c_client *client)
>> +{
>> +	struct iio_dev *indio_dev = i2c_get_clientdata(client);
>> +	struct hp03_priv *priv = iio_priv(indio_dev);
>> +
>> +	i2c_unregister_device(priv->eeprom_client);
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct i2c_device_id hp03_id[] = {
>> +	{ "hp03", 0 },
>> +	{ },
>> +};
>> +MODULE_DEVICE_TABLE(i2c, hp03_id);
>> +
>> +static struct i2c_driver hp03_driver = {
>> +	.driver = {
>> +		.name	= "hp03",
>> +	},
>> +	.probe		= hp03_probe,
>> +	.remove		= hp03_remove,
>> +	.id_table	= hp03_id,
>> +};
>> +module_i2c_driver(hp03_driver);
>> +
>> +MODULE_AUTHOR("Marek Vasut <marex@denx.de>");
>> +MODULE_DESCRIPTION("Driver for Hope RF HP03 pressure and temperature sensor");
>> +MODULE_LICENSE("GPL v2");
>>
> 


-- 
Best regards,
Marek Vasut