On Fri, Dec 11, 2015 at 4:41 AM, Adriana Reus <adriana.reus@xxxxxxxxx> wrote: > > > On 11.12.2015 06:50, Matt Ranostay wrote: >> >> On Thu, Dec 10, 2015 at 2:24 AM, Adriana Reus <adriana.reus@xxxxxxxxx> >> wrote: >>> >>> A couple of notes inline... >>> >>> >>> On 10.12.2015 10:10, 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 <mranostay@xxxxxxxxx> >>>> --- >>>> .../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 | 385 >>>> +++++++++++++++++++++ >>>> 4 files changed, 421 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..a060fd1 >>>> --- /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..753eab6 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_KFIFO_BUF >>>> + 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..6c6f261 >>>> --- /dev/null >>>> +++ b/drivers/iio/chemical/atlas-ph-sensor.c >>>> @@ -0,0 +1,385 @@ >>>> +/* >>>> + * atlas-ph-sensor.c - Support for Atlas Scientific OEM pH-SM sensor >>>> + * >>>> + * Copyright (C) 2015 Matt Ranostay <mranostay@xxxxxxxxx> >>>> + * >>>> + * 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 <linux/module.h> >>>> +#include <linux/init.h> >>>> +#include <linux/interrupt.h> >>>> +#include <linux/delay.h> >>>> +#include <linux/mutex.h> >>>> +#include <linux/err.h> >>>> +#include <linux/irq.h> >>>> +#include <linux/gpio.h> >>>> +#include <linux/i2c.h> >>>> +#include <linux/regmap.h> >>>> +#include <linux/iio/iio.h> >>>> +#include <linux/iio/buffer.h> >>>> +#include <linux/iio/kfifo_buf.h> >>>> +#include <linux/pm_runtime.h> >>>> + >>>> +#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 regmap *regmap; >>>> + >>>> + __be32 buffer[3]; /* 32-bit pH data + 64-bit timestamp */ >>>> +}; >>>> + >>>> +static bool max30100_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 = max30100_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 >>>> + }, >>>> +}; >>>> + >>>> +#ifdef CONFIG_PM >>>> +static int atlas_set_powermode(struct atlas_data *data, int on) >>>> +{ >>>> + return regmap_write(data->regmap, ATLAS_REG_PWR_CONTROL, on); >>>> +} >>>> +#else >>>> +static int atlas_set_powermode(struct atlas_data *data, int on) >>>> +{ >>>> + return 0; >>>> +} >>>> +#endif >>> >>> >>> ^ Why does this need to depend on CONFIG_PM? You may want to use it >>> if CONFIG_PM is not defined also (See my comment in the remove >>> funcion). >>> >>>> + >>>> +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); >>>> + >>>> + 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_buffer_setup_ops atlas_buffer_setup_ops = { >>>> + .postenable = atlas_buffer_postenable, >>>> + .predisable = atlas_buffer_predisable, >>>> +}; >>>> + >>>> +static irqreturn_t atlas_interrupt_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) >>>> + iio_push_to_buffers_with_timestamp(indio_dev, >>>> data->buffer, >>>> + iio_get_time_ns()); >>>> + >>>> + /* 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; >>>> + >>>> + 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); >>>> + >>>> + return ret; >>>> +} >>>> + >>>> +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 = -EINVAL; >>>> + goto error_busy; >>>> + } >>>> + >>>> + switch (chan->type) { >>>> + case IIO_TEMP: >>>> + ret = regmap_bulk_read(data->regmap, >>>> chan->address, >>>> + (u8 *) ®, sizeof(reg)); >>>> + 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_buffer *buffer; >>>> + struct iio_dev *indio_dev; >>>> + int ret; >>>> + >>>> + indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data)); >>>> + if (!indio_dev) >>>> + return -ENOMEM; >>>> + >>>> + buffer = devm_iio_kfifo_allocate(&client->dev); >>>> + if (!buffer) >>>> + return -ENOMEM; >>>> + >>>> + iio_device_attach_buffer(indio_dev, buffer); >>>> + >>>> + 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; >>>> + >>>> + data = iio_priv(indio_dev); >>>> + data->client = client; >>>> + >>>> + 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 = devm_request_threaded_irq(&client->dev, client->irq, >>>> + NULL, atlas_interrupt_handler, >>>> + IRQF_TRIGGER_FALLING | >>>> IRQF_ONESHOT, >>>> + "atlas_event", >>>> + indio_dev); >>>> + if (ret) { >>>> + dev_err(&client->dev, "request irq (%d) failed\n", >>>> client->irq); >>>> + return ret; >>>> + } >>>> + >>>> + pm_runtime_enable(&client->dev); >>>> + pm_runtime_set_autosuspend_delay(&client->dev, 2500); >>>> + pm_runtime_use_autosuspend(&client->dev); >>>> + >>>> + ret = atlas_set_powermode(data, 0); >>>> + if (ret) >>>> + return ret; >>> >>> >>> Unless I am missing something I don't think you need to explicitly >>> set power off here ^. Runtime pm will call your suspend callback in >>> some seconds anyway, so it seems redundant. >> >> Ah true. Would pm_runtime_mark_last_busy be required to be called here as >> well? > > I don't think it is. You're setting a positive delay and instructing it > to use autosuspend, so by quickly glancing at the implementation in > runtime.c it would seem that runtime pm will eventually call > rpm_suspend (rpm_idle -> rpm_suspend) - which in turn calls your > registered suspend function. Don't take my word for it though, > you may want to check and experiment. (Errata at my above comment: > I now think it's right away, not in "some seconds") > >> >> Although doing more thought on this.. that it should be enabling power >> up in case CONFIG_PM/runtime pm isn't enabled... since .remove() could >> power off the device on a reset. > > You don't need to power up either.. if CONFIG_PM is not defined, your > rpm callbacks will not be acknowledged and won't be ever called. So > you'll start with the chip enabled, the pm stuff will have no effect > and the chip will still be enabled when you exit probe. At remove > you should power it off regardless of CONFIG_PM (Note my previous > comment in the .remove() function). > But if CONFIG_PM is disabled wouldn't the corner case of loading the module, unloading, and re-loading cause a device that is powered off? >>>> >>>> + >>>> + return iio_device_register(indio_dev); >>>> +} >>>> + >>>> +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); >>>> + >>>> + pm_runtime_disable(&client->dev); >>>> + pm_runtime_set_suspended(&client->dev); >>>> + >>>> + return atlas_set_powermode(data, 0); >>> >>> >>> If you don't have CONFIG_PM defined, set_powermode will be no-op, >>> I don't think that's what you want here. >>> >>>> +} >>>> + >>>> +#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 <mranostay@xxxxxxxxx>"); >>>> +MODULE_DESCRIPTION("Atlas Scientific pH-SM sensor"); >>>> +MODULE_LICENSE("GPL"); >>>> >>> > -- 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
