Re: [RFC v1 3/3] iio: chemical: add Atlas pH-SM sensor support

[Adriana Reus <adriana.reus@intel.com>]

On 11.12.2015 06:50, Matt Ranostay wrote:
> On Thu, Dec 10, 2015 at 2:24 AM, Adriana Reus <adriana.reus@intel.com> 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@gmail.com>
>>> ---
>>>    .../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@gmail.com>
>>> + *
>>> + * 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 *) &reg, sizeof(reg));
>>> +                       break;
>>> +               case IIO_PH:
>>> +                       ret = atlas_read_ph_measurement(data, &reg);
>>> +                       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, &reg, 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).
>>>
>>> +
>>> +       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@gmail.com>");
>>> +MODULE_DESCRIPTION("Atlas Scientific pH-SM sensor");
>>> +MODULE_LICENSE("GPL");
>>>
>>