From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga02.intel.com ([134.134.136.20]:62758 "EHLO mga02.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S964841AbbLOLDq (ORCPT ); Tue, 15 Dec 2015 06:03:46 -0500 Subject: Re: [RFC v2 3/3] iio: chemical: add Atlas pH-SM sensor support To: Matt Ranostay , linux-iio@vger.kernel.org, jic23@kernel.org References: <1450171574-3283-1-git-send-email-mranostay@gmail.com> <1450171574-3283-4-git-send-email-mranostay@gmail.com> From: Adriana Reus Message-ID: <566FF36E.4090501@intel.com> Date: Tue, 15 Dec 2015 13:03:10 +0200 MIME-Version: 1.0 In-Reply-To: <1450171574-3283-4-git-send-email-mranostay@gmail.com> Content-Type: text/plain; charset=windows-1252; format=flowed Sender: linux-iio-owner@vger.kernel.org List-Id: linux-iio@vger.kernel.org On 15.12.2015 11:26, Matt Ranostay wrote: > Add support for the Atlas Scientific pH-SM chemical sensor that can > detect pH levels of solutions in the range of 0-14. > > Signed-off-by: Matt Ranostay > --- > .../bindings/iio/chemical/atlas,ph-sm.txt | 22 ++ > drivers/iio/chemical/Kconfig | 13 + > drivers/iio/chemical/Makefile | 1 + > drivers/iio/chemical/atlas-ph-sensor.c | 431 +++++++++++++++++++++ > 4 files changed, 467 insertions(+) > create mode 100644 Documentation/devicetree/bindings/iio/chemical/atlas,ph-sm.txt > create mode 100644 drivers/iio/chemical/atlas-ph-sensor.c > > diff --git a/Documentation/devicetree/bindings/iio/chemical/atlas,ph-sm.txt b/Documentation/devicetree/bindings/iio/chemical/atlas,ph-sm.txt > new file mode 100644 > index 0000000..cffa190 > --- /dev/null > +++ b/Documentation/devicetree/bindings/iio/chemical/atlas,ph-sm.txt > @@ -0,0 +1,22 @@ > +* Atlas Scientific pH-SM OEM sensor > + > +http://www.atlas-scientific.com/_files/_datasheets/_oem/pH_oem_datasheet.pdf > + > +Required properties: > + > + - compatible: must be "atlas,ph-sm" > + - reg: the I2C address of the sensor > + - interrupt-parent: should be the phandle for the interrupt controller > + - interrupts: the sole interrupt generated by the device > + > + Refer to interrupt-controller/interrupts.txt for generic interrupt client > + node bindings. > + > +Example: > + > +atlas@65 { > + compatible = "atlas,ph-sm"; > + reg = <0x65>; > + interrupt-parent = <&gpio1>; > + interrupts = <16 2>; > +}; > diff --git a/drivers/iio/chemical/Kconfig b/drivers/iio/chemical/Kconfig > index 3061b72..a358e4b 100644 > --- a/drivers/iio/chemical/Kconfig > +++ b/drivers/iio/chemical/Kconfig > @@ -4,6 +4,19 @@ > > menu "Chemical Sensors" > > +config ATLAS_PH_SENSOR > + tristate "Atlas Scientific OEM pH-SM sensor" > + depends on I2C > + select REGMAP_I2C > + select IIO_BUFFER > + select IIO_TRIGGERED_BUFFER > + help > + Say Y here to build I2C interface support for the Atlas > + Scientific OEM pH-SM sensor. > + > + To compile this driver as module, choose M here: the > + module will be called atlas-ph-sensor. > + > config VZ89X > tristate "SGX Sensortech MiCS VZ89X VOC sensor" > depends on I2C > diff --git a/drivers/iio/chemical/Makefile b/drivers/iio/chemical/Makefile > index 7292f2d..e9eb852 100644 > --- a/drivers/iio/chemical/Makefile > +++ b/drivers/iio/chemical/Makefile > @@ -3,4 +3,5 @@ > # > > # When adding new entries keep the list in alphabetical order > +obj-$(CONFIG_ATLAS_PH_SENSOR) += atlas-ph-sensor.o > obj-$(CONFIG_VZ89X) += vz89x.o > diff --git a/drivers/iio/chemical/atlas-ph-sensor.c b/drivers/iio/chemical/atlas-ph-sensor.c > new file mode 100644 > index 0000000..63da26b > --- /dev/null > +++ b/drivers/iio/chemical/atlas-ph-sensor.c > @@ -0,0 +1,431 @@ > +/* > + * atlas-ph-sensor.c - Support for Atlas Scientific OEM pH-SM sensor > + * > + * Copyright (C) 2015 Matt Ranostay > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation; either version 2 of the License, or > + * (at your option) any later version. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#define ATLAS_REGMAP_NAME "atlas_ph_regmap" > +#define ATLAS_DRV_NAME "atlas_ph" > + > +#define ATLAS_REG_DEV_TYPE 0x00 > +#define ATLAS_REG_DEV_VERSION 0x01 > + > +#define ATLAS_REG_INT_CONTROL 0x04 > +#define ATLAS_REG_INT_CONTROL_EN BIT(2) > + > +#define ATLAS_REG_PWR_CONTROL 0x06 > + > +#define ATLAS_REG_TEMP_DATA 0x0e > +#define ATLAS_REG_PH_DATA 0x16 > + > +#define ATLAS_PH_INT_TIME_IN_US 450000 > + > +struct atlas_data { > + struct i2c_client *client; > + struct iio_trigger *trig; > + struct regmap *regmap; > + > + __be32 buffer[4]; /* 32-bit pH data + 32-bit pad + 64-bit timestamp */ > +}; > + > +static bool atlas_is_volatile_reg(struct device *dev, unsigned int reg) > +{ > + if (reg == ATLAS_REG_INT_CONTROL) > + return true; > + > + return false; > +} > + > +static const struct regmap_config atlas_regmap_config = { > + .name = ATLAS_REGMAP_NAME, > + > + .reg_bits = 8, > + .val_bits = 8, > + > + .volatile_reg = atlas_is_volatile_reg, > + .max_register = ATLAS_REG_PH_DATA + 4, > + .cache_type = REGCACHE_FLAT, > + > +}; > + > +static const struct iio_chan_spec atlas_channels[] = { > + { > + .type = IIO_PH, > + .info_mask_separate = > + BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE), > + .scan_index = 0, > + .scan_type = { > + .sign = 'u', > + .realbits = 32, > + .storagebits = 32, > + .endianness = IIO_BE, > + }, > + }, > + { > + .type = IIO_TEMP, > + .address = ATLAS_REG_TEMP_DATA, > + .info_mask_separate = > + BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE), > + .output = 1, > + .scan_index = -1 > + }, > +}; > + > +static int atlas_set_powermode(struct atlas_data *data, int on) > +{ > + return regmap_write(data->regmap, ATLAS_REG_PWR_CONTROL, on); > +} > + > +static int atlas_set_interrupt(struct atlas_data *data, bool state) > +{ > + return regmap_update_bits(data->regmap, ATLAS_REG_INT_CONTROL, > + ATLAS_REG_INT_CONTROL_EN, > + state ? ATLAS_REG_INT_CONTROL_EN : 0); > +} > + > +static int atlas_buffer_postenable(struct iio_dev *indio_dev) > +{ > + struct atlas_data *data = iio_priv(indio_dev); > + > + pm_runtime_get_sync(&data->client->dev); ^ pm_runtime_get_sync may return errors > + > + return atlas_set_interrupt(data, true); > +} > + > +static int atlas_buffer_predisable(struct iio_dev *indio_dev) > +{ > + struct atlas_data *data = iio_priv(indio_dev); > + int ret; > + > + ret = atlas_set_interrupt(data, false); > + if (ret) > + return ret; > + > + pm_runtime_mark_last_busy(&data->client->dev); > + return pm_runtime_put_autosuspend(&data->client->dev); > +} > + > +static const struct iio_trigger_ops atlas_interrupt_trigger_ops = { > + .owner = THIS_MODULE, > +}; > + > +static const struct iio_buffer_setup_ops atlas_buffer_setup_ops = { > + .postenable = atlas_buffer_postenable, > + .predisable = atlas_buffer_predisable, > +}; > + > +static irqreturn_t atlas_trigger_handler(int irq, void *private) > +{ > + struct iio_dev *indio_dev = private; > + struct atlas_data *data = iio_priv(indio_dev); > + int ret; > + > + ret = i2c_smbus_read_i2c_block_data(data->client, ATLAS_REG_PH_DATA, > + sizeof(data->buffer[0]), (u8 *) &data->buffer); > + > + if (ret > 0) > + iio_push_to_buffers_with_timestamp(indio_dev, data->buffer, > + iio_get_time_ns()); > + > + iio_trigger_notify_done(indio_dev->trig); > + > + return IRQ_HANDLED; > +} > + > +static irqreturn_t atlas_interrupt_handler(int irq, void *private) > +{ > + struct iio_dev *indio_dev = private; > + struct atlas_data *data = iio_priv(indio_dev); > + > + /* re-enable interrupt */ > + atlas_set_interrupt(data, true); > + > + return IRQ_HANDLED; > +} > + > +static int atlas_read_ph_measurement(struct atlas_data *data, __be32 *val) > +{ > + struct device *dev = &data->client->dev; > + int suspended = pm_runtime_suspended(dev); > + int ret; > + > + ret = pm_runtime_get_sync(dev); > + if (ret) > + return ret; ^ pm_runtime_get_sync can also return 1 (if the device is already active).. in which case you definitely don't want to return here. Also if this does return an error you may want to call put_noidle because get_sync increments the device usage_count before it tries to resume and you can end up with an unbalanced count there. > + > + if (suspended) > + usleep_range(ATLAS_PH_INT_TIME_IN_US, > + ATLAS_PH_INT_TIME_IN_US + 100000); > + > + ret = i2c_smbus_read_i2c_block_data(data->client, ATLAS_REG_PH_DATA, > + sizeof(*val), (u8 *) val); > + > + pm_runtime_mark_last_busy(dev); > + pm_runtime_put_autosuspend(dev); ^ also may return err code > + > + return ret == sizeof(*val) ? 0 : -EINVAL; > +} > + > +static int atlas_read_raw(struct iio_dev *indio_dev, > + struct iio_chan_spec const *chan, > + int *val, int *val2, long mask) > +{ > + struct atlas_data *data = iio_priv(indio_dev); > + > + switch (mask) { > + case IIO_CHAN_INFO_RAW: { > + int ret; > + __be32 reg; > + > + mutex_lock(&indio_dev->mlock); > + > + if (iio_buffer_enabled(indio_dev)) { > + ret = -EBUSY; > + goto error_busy; > + } > + > + switch (chan->type) { > + case IIO_TEMP: > + ret = regmap_bulk_read(data->regmap, chan->address, > + (u8 *) ®, sizeof(reg)); ^ what would happen if temp is read while the chip is suspended runtime-pm wise? Unless I am missing something the chip may be powered off at this point. > + break; > + case IIO_PH: > + ret = atlas_read_ph_measurement(data, ®); > + break; > + default: > + ret = -EINVAL; > + } > + > + if (!ret) { > + *val = be32_to_cpu(reg); > + ret = IIO_VAL_INT; > + } > +error_busy: > + mutex_unlock(&indio_dev->mlock); > + return ret; > + } > + case IIO_CHAN_INFO_SCALE: > + *val = 0; /* 0.001 */ > + *val2 = 1000; > + return IIO_VAL_INT_PLUS_MICRO; > + } > + > + return -EINVAL; > +} > + > +static int atlas_write_raw(struct iio_dev *indio_dev, > + struct iio_chan_spec const *chan, > + int val, int val2, long mask) > +{ > + struct atlas_data *data = iio_priv(indio_dev); > + __be32 reg = cpu_to_be32(val); > + > + if (val2 != 0) > + return -EINVAL; > + > + if (mask != IIO_CHAN_INFO_RAW || chan->type != IIO_TEMP) > + return -EINVAL; > + > + return regmap_bulk_write(data->regmap, chan->address, ®, 4); > +} > + > +static const struct iio_info atlas_info = { > + .driver_module = THIS_MODULE, > + .read_raw = atlas_read_raw, > + .write_raw = atlas_write_raw, > +}; > + > +static int atlas_probe(struct i2c_client *client, > + const struct i2c_device_id *id) > +{ > + struct atlas_data *data; > + struct iio_trigger *trig; > + struct iio_dev *indio_dev; > + int ret; > + > + indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data)); > + if (!indio_dev) > + return -ENOMEM; > + > + indio_dev->info = &atlas_info; > + indio_dev->name = ATLAS_DRV_NAME; > + indio_dev->channels = atlas_channels; > + indio_dev->modes = INDIO_BUFFER_SOFTWARE | INDIO_DIRECT_MODE; > + indio_dev->setup_ops = &atlas_buffer_setup_ops; > + indio_dev->dev.parent = &client->dev; > + > + trig = devm_iio_trigger_alloc(&client->dev, "%s-dev%d", > + indio_dev->name, indio_dev->id); > + > + if (!trig) > + return -ENOMEM; > + > + data = iio_priv(indio_dev); > + data->client = client; > + data->trig = trig; > + trig->dev.parent = indio_dev->dev.parent; > + trig->ops = &atlas_interrupt_trigger_ops; > + iio_trigger_set_drvdata(trig, indio_dev); > + > + i2c_set_clientdata(client, indio_dev); > + > + data->regmap = devm_regmap_init_i2c(client, &atlas_regmap_config); > + if (IS_ERR(data->regmap)) { > + dev_err(&client->dev, "regmap initialization failed\n"); > + return PTR_ERR(data->regmap); > + } > + > + ret = pm_runtime_set_active(&client->dev); > + if (ret) > + return ret; > + > + if (client->irq <= 0) { > + dev_err(&client->dev, "no valid irq defined\n"); > + return -EINVAL; > + } > + > + ret = iio_trigger_register(trig); > + if (ret) { > + dev_err(&client->dev, "failed to register trigger\n"); > + return ret; > + } > + > + ret = iio_triggered_buffer_setup(indio_dev, NULL, > + &atlas_trigger_handler, NULL); > + > + if (ret) { > + dev_err(&client->dev, "cannot setup iio trigger\n"); > + goto unregister_trigger; > + } > + > + ret = devm_request_threaded_irq(&client->dev, client->irq, > + NULL, atlas_interrupt_handler, > + IRQF_TRIGGER_FALLING | IRQF_ONESHOT, > + "atlas_irq", > + indio_dev); > + if (ret) { > + dev_err(&client->dev, "request irq (%d) failed\n", client->irq); > + goto unregister_buffer; > + } > + > + ret = atlas_set_powermode(data, 1); > + if (ret) { > + dev_err(&client->dev, "cannot power device on"); > + goto unregister_buffer; > + } > + > + pm_runtime_enable(&client->dev); > + pm_runtime_set_autosuspend_delay(&client->dev, 2500); > + pm_runtime_use_autosuspend(&client->dev); > + > + ret = iio_device_register(indio_dev); > + if (ret) { > + dev_err(&client->dev, "unable to register device\n"); > + goto unregister_buffer; > + } > + > + return 0; > + > +unregister_buffer: > + iio_triggered_buffer_cleanup(indio_dev); > + > +unregister_trigger: > + iio_trigger_unregister(data->trig); > + > + return ret; > +} > + > +static int atlas_remove(struct i2c_client *client) > +{ > + struct iio_dev *indio_dev = i2c_get_clientdata(client); > + struct atlas_data *data = iio_priv(indio_dev); > + > + iio_device_unregister(indio_dev); > + iio_triggered_buffer_cleanup(indio_dev); > + iio_trigger_unregister(data->trig); > + > + pm_runtime_disable(&client->dev); > + pm_runtime_set_suspended(&client->dev); > + pm_runtime_put_noidle(&client->dev); > + > + return atlas_set_powermode(data, 0); > +} > + > +#ifdef CONFIG_PM > +static int atlas_runtime_suspend(struct device *dev) > +{ > + struct atlas_data *data = > + iio_priv(i2c_get_clientdata(to_i2c_client(dev))); > + > + return atlas_set_powermode(data, 0); > +} > + > +static int atlas_runtime_resume(struct device *dev) > +{ > + struct atlas_data *data = > + iio_priv(i2c_get_clientdata(to_i2c_client(dev))); > + > + return atlas_set_powermode(data, 1); > +} > +#endif > + > +static const struct dev_pm_ops atlas_pm_ops = { > + SET_RUNTIME_PM_OPS(atlas_runtime_suspend, > + atlas_runtime_resume, NULL) > +}; > + > +static const struct i2c_device_id atlas_id[] = { > + { "atlas-ph-sm", 0 }, > + {} > +}; > +MODULE_DEVICE_TABLE(i2c, atlas_id); > + > +static const struct of_device_id atlas_dt_ids[] = { > + { .compatible = "atlas,ph-sm" }, > + { } > +}; > +MODULE_DEVICE_TABLE(of, atlas_dt_ids); > + > +static struct i2c_driver atlas_driver = { > + .driver = { > + .name = ATLAS_DRV_NAME, > + .of_match_table = of_match_ptr(atlas_dt_ids), > + .pm = &atlas_pm_ops, > + }, > + .probe = atlas_probe, > + .remove = atlas_remove, > + .id_table = atlas_id, > +}; > +module_i2c_driver(atlas_driver); > + > +MODULE_AUTHOR("Matt Ranostay "); > +MODULE_DESCRIPTION("Atlas Scientific pH-SM sensor"); > +MODULE_LICENSE("GPL"); >