On 04/02/2016 09:41 PM, Matt Ranostay wrote: Dear Mr. Ranostay, > Overall seems fine. But few comments in line Thank you for your kind input. > On Fri, Apr 1, 2016 at 8:11 AM, Marek Vasut <marex@xxxxxxx> 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@xxxxxxx> >> Cc: Matt Ranostay <mranostay@xxxxxxxxx> >> Cc: Jonathan Cameron <jic23@xxxxxxxxxx> >> --- >> .../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 > > Can you explain a little more what this XCLR pin is in the documentation? According to documentation [1], page 2, the XCLR resets the on-die ADC. On page 5 bottom, section "Remark", the datasheet states that XCLR pin must be pulled HI before starting the conversion. In fact, the XCLR pin seems to gate the ADC altogether and if the XCLR is pulled low, the ADC registers are not accessible at all. On page 3 middle, the "Note" states that in all other times, the XCLR must be held low, otherwise readout of the calibration coefficients from the EEPROM might be influenced. I did not observe this problem. The driver behaves according to the datasheet and toggles the XCLR only before performing the ADC readout, after which the XCLR is set to LO again. btw this design is braindead, it is not possible to easily synchronize GPIO toggling and I2C register access, since both the GPIO and I2C can be on totally different interconnects in the system, but at these low speeds, it probably doesn't pose a problem. [1] http://www.hoperf.com/upload/sensor/HP03S.pdf >> + >> +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@xxxxxxx> >> + * >> + * 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. >> + * >> + */ >> + >> +#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; >> + } >> + 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; > Better to use IIO_CHAN_INFO_RAW, and have the respective IIO_CHAN_INFO_SCALE. OK, this sounds indeed better. >> + 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; >> + > > Can you document in a comment why it needs to access another i2c > device. Know this is where the calibration data is stored, but that > isn't obvious at first. Comment is OK ? >> + 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@xxxxxxx>"); >> +MODULE_DESCRIPTION("Driver for Hope RF HP03 pressure and temperature sensor"); >> +MODULE_LICENSE("GPL v2"); >> -- >> 2.7.0 >> -- Best regards, Marek Vasut -- 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
