Re: [PATCH v5] iio: potentiostat: add LMP91000 support

[Jonathan Cameron <jic23@kernel.org>]

On 16/09/16 07:34, Matt Ranostay wrote:
> On Mon, Sep 12, 2016 at 1:01 PM, Jonathan Cameron <jic23@kernel.org> wrote:
>> On 12/09/16 01:54, Matt Ranostay wrote:
>>> On Sat, Sep 10, 2016 at 4:50 PM, Matt Ranostay <mranostay@gmail.com> wrote:
>>>> On Sat, Sep 10, 2016 at 8:54 AM, Jonathan Cameron <jic23@kernel.org> wrote:
>>>>> On 07/09/16 03:22, Matt Ranostay wrote:
>>>>>> On Tue, Sep 6, 2016 at 12:31 PM, Matt Ranostay <mranostay@gmail.com> wrote:
>>>>>>> On Tue, Sep 6, 2016 at 12:14 AM, Matt Ranostay <mranostay@gmail.com> wrote:
>>>>>>>> On Mon, Sep 5, 2016 at 10:43 PM, Peter Meerwald-Stadler
>>>>>>>> <pmeerw@pmeerw.net> wrote:
>>>>>>>>>
>>>>>>>>>> Add support for the LMP91000 potentiostat which is used for chemical
>>>>>>>>>> sensing applications.
>>>>>>>>>
>>>>>>>>> minor comments below
>>>>>>>>>
>>>>>>>>>> Signed-off-by: Matt Ranostay <matt@ranostay.consulting>
>>>>>>>>>> ---
>>>>>>>>>>
>>>>>>>>>> Changes from v1:
>>>>>>>>>>  * Rename ti-adc1x1s.c driver to ti-adc161s626.c
>>>>>>>>>>  * Switch from iio_channel_read() to using the industrialio-buffer-cb
>>>>>>>>>>    interface
>>>>>>>>>>  * Process data in ADC trigger handler for buffer callbacks
>>>>>>>>>>  * Set the mux trigger to the ADC referenced by io-channels by default
>>>>>>>>>>
>>>>>>>>>> Changes from v2:
>>>>>>>>>>  * Fix configuration index shifting values
>>>>>>>>>>  * Switch from wait_for_completion_interruptible to wait_for_completion_timeout
>>>>>>>>>>
>>>>>>>>>> Changes from v3:
>>>>>>>>>>  * Fix order of probe function error rollback
>>>>>>>>>>
>>>>>>>>>> Changes from v4:
>>>>>>>>>>  * add iio_trigger_set_immutable() call
>>>>>>>>>>  * move iio_channel_*_all_cb calls to *_buffer_preenable/postenable
>>>>>>>>>>
>>>>>>>>>> .../bindings/iio/potentiostat/lmp91000.txt         |  30 ++
>>>>>>>>>>  drivers/iio/Kconfig                                |   1 +
>>>>>>>>>>  drivers/iio/Makefile                               |   1 +
>>>>>>>>>>  drivers/iio/potentiostat/Kconfig                   |  22 ++
>>>>>>>>>>  drivers/iio/potentiostat/Makefile                  |   6 +
>>>>>>>>>>  drivers/iio/potentiostat/lmp91000.c                | 397 +++++++++++++++++++++
>>>>>>>>>>  6 files changed, 457 insertions(+)
>>>>>>>>>>  create mode 100644 Documentation/devicetree/bindings/iio/potentiostat/lmp91000.txt
>>>>>>>>>>  create mode 100644 drivers/iio/potentiostat/Kconfig
>>>>>>>>>>  create mode 100644 drivers/iio/potentiostat/Makefile
>>>>>>>>>>  create mode 100644 drivers/iio/potentiostat/lmp91000.c
>>>>>>>>>>
>>>>>>>>>> diff --git a/Documentation/devicetree/bindings/iio/potentiostat/lmp91000.txt b/Documentation/devicetree/bindings/iio/potentiostat/lmp91000.txt
>>>>>>>>>> new file mode 100644
>>>>>>>>>> index 000000000000..b9b621e94cd7
>>>>>>>>>> --- /dev/null
>>>>>>>>>> +++ b/Documentation/devicetree/bindings/iio/potentiostat/lmp91000.txt
>>>>>>>>>> @@ -0,0 +1,30 @@
>>>>>>>>>> +* Texas Instruments LMP91000 potentiostat
>>>>>>>>>> +
>>>>>>>>>> +http://www.ti.com/lit/ds/symlink/lmp91000.pdf
>>>>>>>>>> +
>>>>>>>>>> +Required properties:
>>>>>>>>>> +
>>>>>>>>>> +  - compatible: should be "ti,lmp91000"
>>>>>>>>>> +  - reg: the I2C address of the device
>>>>>>>>>> +  - io-channels: the phandle of the iio provider
>>>>>>>>>> +
>>>>>>>>>> +  - ti,external-tia-resistor: if the property ti,tia-gain-ohm is not defined this
>>>>>>>>>> +    needs to be set to signal that an external resistor value is being used.
>>>>>>>>>> +
>>>>>>>>>> +Optional properties:
>>>>>>>>>> +
>>>>>>>>>> +  - ti,tia-gain-ohm: ohm value of the internal resistor for the transimpedance
>>>>>>>>>> +    amplifier. Must be 2750, 3500, 7000, 14000, 35000, 120000, or 350000 ohms.
>>>>>>>>>> +
>>>>>>>>>> +  - ti,rload-ohm: ohm value of the internal resistor load applied to the gas
>>>>>>>>>> +    sensor. Must be 10, 33, 50, or 100 (default) ohms.
>>>>>>>>>> +
>>>>>>>>>> +Example:
>>>>>>>>>> +
>>>>>>>>>> +lmp91000@48 {
>>>>>>>>>> +     compatible = "ti,lmp91000";
>>>>>>>>>> +     reg = <0x48>;
>>>>>>>>>> +     ti,tia-gain-ohm = <7500>;
>>>>>>>>>> +     ti,rload = <100>;
>>>>>>>>>> +     io-channels = <&adc>;
>>>>>>>>>> +};
>>>>>>>>>> diff --git a/drivers/iio/Kconfig b/drivers/iio/Kconfig
>>>>>>>>>> index 505e921f0b19..ad5bbaefc18e 100644
>>>>>>>>>> --- a/drivers/iio/Kconfig
>>>>>>>>>> +++ b/drivers/iio/Kconfig
>>>>>>>>>> @@ -79,6 +79,7 @@ if IIO_TRIGGER
>>>>>>>>>>     source "drivers/iio/trigger/Kconfig"
>>>>>>>>>>  endif #IIO_TRIGGER
>>>>>>>>>>  source "drivers/iio/potentiometer/Kconfig"
>>>>>>>>>> +source "drivers/iio/potentiostat/Kconfig"
>>>>>>>>>>  source "drivers/iio/pressure/Kconfig"
>>>>>>>>>>  source "drivers/iio/proximity/Kconfig"
>>>>>>>>>>  source "drivers/iio/temperature/Kconfig"
>>>>>>>>>> diff --git a/drivers/iio/Makefile b/drivers/iio/Makefile
>>>>>>>>>> index 20f649073462..f7369b77ed94 100644
>>>>>>>>>> --- a/drivers/iio/Makefile
>>>>>>>>>> +++ b/drivers/iio/Makefile
>>>>>>>>>> @@ -28,6 +28,7 @@ obj-y += light/
>>>>>>>>>>  obj-y += magnetometer/
>>>>>>>>>>  obj-y += orientation/
>>>>>>>>>>  obj-y += potentiometer/
>>>>>>>>>> +obj-y += potentiostat/
>>>>>>>>>>  obj-y += pressure/
>>>>>>>>>>  obj-y += proximity/
>>>>>>>>>>  obj-y += temperature/
>>>>>>>>>> diff --git a/drivers/iio/potentiostat/Kconfig b/drivers/iio/potentiostat/Kconfig
>>>>>>>>>> new file mode 100644
>>>>>>>>>> index 000000000000..1e3baf2cc97d
>>>>>>>>>> --- /dev/null
>>>>>>>>>> +++ b/drivers/iio/potentiostat/Kconfig
>>>>>>>>>> @@ -0,0 +1,22 @@
>>>>>>>>>> +#
>>>>>>>>>> +# Potentiostat drivers
>>>>>>>>>> +#
>>>>>>>>>> +# When adding new entries keep the list in alphabetical order
>>>>>>>>>> +
>>>>>>>>>> +menu "Digital potentiostats"
>>>>>>>>>> +
>>>>>>>>>> +config LMP91000
>>>>>>>>>> +     tristate "Texas Instruments LMP91000 potentiostat driver"
>>>>>>>>>> +     depends on I2C
>>>>>>>>>> +     select REGMAP_I2C
>>>>>>>>>> +     select IIO_BUFFER
>>>>>>>>>> +     select IIO_BUFFER_CB
>>>>>>>>>> +     select IIO_TRIGGERED_BUFFER
>>>>>>>>>> +     help
>>>>>>>>>> +       Say yes here to build support for the Texas Instruments
>>>>>>>>>> +       LMP91000 digital potentiostat chip.
>>>>>>>>>> +
>>>>>>>>>> +       To compile this driver as a module, choose M here: the
>>>>>>>>>> +       module will be called lmp91000
>>>>>>>>>> +
>>>>>>>>>> +endmenu
>>>>>>>>>> diff --git a/drivers/iio/potentiostat/Makefile b/drivers/iio/potentiostat/Makefile
>>>>>>>>>> new file mode 100644
>>>>>>>>>> index 000000000000..64d315ef4449
>>>>>>>>>> --- /dev/null
>>>>>>>>>> +++ b/drivers/iio/potentiostat/Makefile
>>>>>>>>>> @@ -0,0 +1,6 @@
>>>>>>>>>> +#
>>>>>>>>>> +# Makefile for industrial I/O potentiostat drivers
>>>>>>>>>> +#
>>>>>>>>>> +
>>>>>>>>>> +# When adding new entries keep the list in alphabetical order
>>>>>>>>>> +obj-$(CONFIG_LMP91000) += lmp91000.o
>>>>>>>>>> diff --git a/drivers/iio/potentiostat/lmp91000.c b/drivers/iio/potentiostat/lmp91000.c
>>>>>>>>>> new file mode 100644
>>>>>>>>>> index 000000000000..ec4394581039
>>>>>>>>>> --- /dev/null
>>>>>>>>>> +++ b/drivers/iio/potentiostat/lmp91000.c
>>>>>>>>>> @@ -0,0 +1,397 @@
>>>>>>>>>> +/*
>>>>>>>>>> + * lmp91000.c - Support for Texas Instruments digital potentiostats
>>>>>>>>>> + *
>>>>>>>>>> + * Copyright (C) 2016 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.
>>>>>>>>>> + *
>>>>>>>>>> + * TODO: bias voltage + polarity control, and multiple chip support
>>>>>>>>>> + */
>>>>>>>>>> +
>>>>>>>>>> +#include <linux/module.h>
>>>>>>>>>> +#include <linux/i2c.h>
>>>>>>>>>> +#include <linux/delay.h>
>>>>>>>>>> +#include <linux/of.h>
>>>>>>>>>> +#include <linux/regmap.h>
>>>>>>>>>> +#include <linux/iio/iio.h>
>>>>>>>>>> +#include <linux/iio/buffer.h>
>>>>>>>>>> +#include <linux/iio/consumer.h>
>>>>>>>>>> +#include <linux/iio/trigger.h>
>>>>>>>>>> +#include <linux/iio/trigger_consumer.h>
>>>>>>>>>> +#include <linux/iio/triggered_buffer.h>
>>>>>>>>>> +
>>>>>>>>>> +#define LMP91000_REG_LOCK            0x01
>>>>>>>>>> +#define LMP91000_REG_TIACN           0x10
>>>>>>>>>> +#define LMP91000_REG_TIACN_GAIN_SHIFT        2
>>>>>>>>>> +
>>>>>>>>>> +#define LMP91000_REG_REFCN           0x11
>>>>>>>>>> +#define LMP91000_REG_REFCN_EXT_REF   0x20
>>>>>>>>>> +#define LMP91000_REG_REFCN_50_ZERO   0x80
>>>>>>>>>> +
>>>>>>>>>> +#define LMP91000_REG_MODECN          0x12
>>>>>>>>>> +#define LMP91000_REG_MODECN_3LEAD    0x03
>>>>>>>>>> +#define LMP91000_REG_MODECN_TEMP     0x07
>>>>>>>>>> +
>>>>>>>>>> +#define LMP91000_DRV_NAME    "lmp91000"
>>>>>>>>>> +
>>>>>>>>>> +static const int lmp91000_tia_gain[] = { 0, 2750, 3500, 7000, 14000, 35000,
>>>>>>>>>> +                                      120000, 350000 };
>>>>>>>>>> +
>>>>>>>>>> +static const int lmp91000_rload[] = { 10, 33, 50, 100 };
>>>>>>>>>> +
>>>>>>>>>> +static const struct regmap_config lmp91000_regmap_config = {
>>>>>>>>>> +     .reg_bits = 8,
>>>>>>>>>> +     .val_bits = 8,
>>>>>>>>>> +};
>>>>>>>>>> +
>>>>>>>>>> +struct lmp91000_data {
>>>>>>>>>> +     struct regmap *regmap;
>>>>>>>>>> +     struct device *dev;
>>>>>>>>>> +
>>>>>>>>>> +     struct iio_trigger *trig;
>>>>>>>>>> +     struct iio_cb_buffer *cb_buffer;
>>>>>>>>>> +
>>>>>>>>>> +     struct completion completion;
>>>>>>>>>> +     u8 chan_select;
>>>>>>>>>> +
>>>>>>>>>> +     u32 buffer[4]; /* 64-bit data + 64-bit timestamp */
>>>>>>>>>> +};
>>>>>>>>>> +
>>>>>>>>>> +static const struct iio_chan_spec lmp91000_channels[] = {
>>>>>>>>>> +     { /* chemical channel mV */
>>>>>>>>>> +             .type = IIO_VOLTAGE,
>>>>>>>>>> +             .channel = 0,
>>>>>>>>>> +             .indexed = 1,
>>>>>>>>>> +             .address = LMP91000_REG_MODECN_3LEAD,
>>>>>>>>>> +             .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
>>>>>>>>>> +             .scan_index = 0,
>>>>>>>>>> +             .scan_type = {
>>>>>>>>>> +                     .sign = 's',
>>>>>>>>>> +                     .realbits = 32,
>>>>>>>>>> +                     .storagebits = 32,
>>>>>>>>>> +             },
>>>>>>>>>> +     },
>>>>>>>>>> +     IIO_CHAN_SOFT_TIMESTAMP(1),
>>>>>>>>>> +     { /* temperature channel mV */
>>>>>>>>>> +             .type = IIO_VOLTAGE,
>>>>>>>>>
>>>>>>>>> why not IIO_TEMP?
>>>>>>>>
>>>>>>>> Because isn't linear enough. and scaling value wouldn't help... Thoughts?
>>>>>>>
>>>>>>> Actually second look in the datasheet it is mostly linear.. probably
>>>>>>> can add an offset and scaling value.
>>>>>>>
>>>>>>
>>>>>> Digging deeper into the datasheet ->
>>>>>> http://www.ti.com/lit/ds/symlink/lmp91000.pdf
>>>>>>
>>>>>> "Although the temperature sensor is very linear, its response does
>>>>>> have a slight downward parabolic shape. This shape is very accurately
>>>>>> reflected in Table 1. "
>>>>>>
>>>>>> So don't think we can really convert to IIO_TEMP since it isn't
>>>>>> completely linear scaling... Any thoughts on this?
>>>>> You aren't exposing it in the buffer so do it as IIO_CHAN_INFO_PROCESSED
>>>>> and do the magic in kernel.  That's one of the main reasons we
>>>>> have that option there.
>>>>
>>>> Ok makes sense. Do really want the most accurate readings since
>>>> electrochemical sensors are highly sensitive to temperature changes.
>>>
>>> Also probably makes more sense to have a lookup table of the mV to
>>> temp, rather than to implement that transfer function in the
>>> datasheet, correct?
>> Up to you ;)  Given that's how it's presented in the datasheet,
>> a lookup table and local linear interpolation looks to make sense.
> 
> Just noticed we need to figure a way to get the offset + scaling from
> the ADC for processing in our callback buffer. Have any idea how we
> could do this sanely?
The iio_cb_buffer structure has a pointer to the channels. 
That is opaque to the consumer however - so we'll need a function to
access the struct iio_channel pointers.  Once we have those
then the normal iio_read_channel_scale and similar should do the job.

You'd almost of thought we hadn't really used this infrastructure
before this driver of yours :)
> 
>>
>> Jonathan
>>>
>>>>
>>>>>
>>>>> We do this all the time in light sensors with nasty non linear
>>>>> (sometimes fusing multiple sensors) mappings to illuminance from
>>>>> the ADC readings.
>>>>>
>>>>> Jonathan
>>>>>>
>>>>>>> Thanks,
>>>>>>>
>>>>>>> Matt
>>>>>>>
>>>>>>>>
>>>>>>>>>
>>>>>>>>>> +             .channel = 1,
>>>>>>>>>> +             .indexed = 1,
>>>>>>>>>> +             .address = LMP91000_REG_MODECN_TEMP,
>>>>>>>>>> +             .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
>>>>>>>>>> +             .scan_index = -1,
>>>>>>>>>> +     },
>>>>>>>>>> +};
>>>>>>>>>> +
>>>>>>>>>> +static int lmp91000_read(struct lmp91000_data *data, int channel, int *val)
>>>>>>>>>> +{
>>>>>>>>>> +     int state, ret;
>>>>>>>>>> +
>>>>>>>>>> +     ret = regmap_read(data->regmap, LMP91000_REG_MODECN, &state);
>>>>>>>>>> +     if (ret)
>>>>>>>>>> +             return -EINVAL;
>>>>>>>>>
>>>>>>>>> simply return ret?
>>>>>>>>
>>>>>>>> No real reason.
>>>>>>>>>
>>>>>>>>>> +
>>>>>>>>>> +     ret = regmap_write(data->regmap, LMP91000_REG_MODECN, channel);
>>>>>>>>>> +     if (ret)
>>>>>>>>>> +             return -EINVAL;
>>>>>>>>>> +
>>>>>>>>>> +     /* delay till first temperature reading is complete */
>>>>>>>>>> +     if ((state != channel) && (channel == LMP91000_REG_MODECN_TEMP))
>>>>>>>>>> +             usleep_range(3000, 4000);
>>>>>>>>>> +
>>>>>>>>>> +     data->chan_select = channel != LMP91000_REG_MODECN_3LEAD;
>>>>>>>>>> +
>>>>>>>>>> +     iio_trigger_poll_chained(data->trig);
>>>>>>>>>> +
>>>>>>>>>> +     ret = wait_for_completion_timeout(&data->completion, HZ);
>>>>>>>>>> +     reinit_completion(&data->completion);
>>>>>>>>>> +
>>>>>>>>>> +     if (!ret)
>>>>>>>>>> +             return -ETIMEDOUT;
>>>>>>>>>> +
>>>>>>>>>> +     *val = data->buffer[data->chan_select];
>>>>>>>>>> +
>>>>>>>>>> +     return 0;
>>>>>>>>>> +}
>>>>>>>>>> +
>>>>>>>>>> +static irqreturn_t lmp91000_buffer_handler(int irq, void *private)
>>>>>>>>>> +{
>>>>>>>>>> +     struct iio_poll_func *pf = private;
>>>>>>>>>> +     struct iio_dev *indio_dev = pf->indio_dev;
>>>>>>>>>> +     struct lmp91000_data *data = iio_priv(indio_dev);
>>>>>>>>>> +     int ret, val;
>>>>>>>>>> +
>>>>>>>>>> +     memset(data->buffer, 0, sizeof(data->buffer));
>>>>>>>>>> +
>>>>>>>>>> +     ret = lmp91000_read(data, LMP91000_REG_MODECN_3LEAD, &val);
>>>>>>>>>> +     if (!ret) {
>>>>>>>>>> +             data->buffer[0] = val;
>>>>>>>>>> +             iio_push_to_buffers_with_timestamp(indio_dev, data->buffer,
>>>>>>>>>> +                                                iio_get_time_ns(indio_dev));
>>>>>>>>>> +     }
>>>>>>>>>> +
>>>>>>>>>> +     iio_trigger_notify_done(indio_dev->trig);
>>>>>>>>>> +
>>>>>>>>>> +     return IRQ_HANDLED;
>>>>>>>>>> +}
>>>>>>>>>> +
>>>>>>>>>> +static int lmp91000_read_raw(struct iio_dev *indio_dev,
>>>>>>>>>> +                          struct iio_chan_spec const *chan,
>>>>>>>>>> +                          int *val, int *val2, long mask)
>>>>>>>>>> +{
>>>>>>>>>> +     struct lmp91000_data *data = iio_priv(indio_dev);
>>>>>>>>>> +     int ret;
>>>>>>>>>> +
>>>>>>>>>> +     if (mask != IIO_CHAN_INFO_RAW)
>>>>>>>>>> +             return -EINVAL;
>>>>>>>>>> +
>>>>>>>>>> +     ret = iio_channel_start_all_cb(data->cb_buffer);
>>>>>>>>>> +     if (ret)
>>>>>>>>>> +             return ret;
>>>>>>>>>> +
>>>>>>>>>> +     ret = lmp91000_read(data, chan->address, val);
>>>>>>>>>> +
>>>>>>>>>> +     iio_channel_stop_all_cb(data->cb_buffer);
>>>>>>>>>> +
>>>>>>>>>> +     if (!ret)
>>>>>>>>>> +             return IIO_VAL_INT;
>>>>>>>>>> +
>>>>>>>>>> +     return ret;
>>>>>>>>>> +}
>>>>>>>>>> +
>>>>>>>>>> +static const struct iio_info lmp91000_info = {
>>>>>>>>>> +     .driver_module = THIS_MODULE,
>>>>>>>>>> +     .read_raw = lmp91000_read_raw,
>>>>>>>>>> +};
>>>>>>>>>> +
>>>>>>>>>> +static int lmp91000_read_config(struct lmp91000_data *data)
>>>>>>>>>> +{
>>>>>>>>>> +     struct device *dev = data->dev;
>>>>>>>>>> +     struct device_node *np = dev->of_node;
>>>>>>>>>> +     unsigned int reg, val;
>>>>>>>>>> +     int i, ret;
>>>>>>>>>> +
>>>>>>>>>> +     ret = of_property_read_u32(np, "ti,tia-gain-ohm", &val);
>>>>>>>>>> +     if (ret) {
>>>>>>>>>> +             if (of_property_read_bool(np, "ti,external-tia-resistor"))
>>>>>>>>>> +                     val = 0;
>>>>>>>>>> +             else {
>>>>>>>>>> +                     dev_err(dev, "no ti,tia-gain-ohm defined");
>>>>>>>>>> +                     return ret;
>>>>>>>>>> +             }
>>>>>>>>>> +     }
>>>>>>>>>> +
>>>>>>>>>> +     ret = -EINVAL;
>>>>>>>>>
>>>>>>>>> instead of fiddling with ret, one could check i >=
>>>>>>>>> ARRAY_SIZE(lmp91000_tia_gain) after the loop, matter of taste
>>>>>>>>
>>>>>>>> No issue with this.
>>>>>>>>>
>>>>>>>>>> +     for (i = 0; i < ARRAY_SIZE(lmp91000_tia_gain); i++) {
>>>>>>>>>> +             if (lmp91000_tia_gain[i] == val) {
>>>>>>>>>> +                     reg = i << LMP91000_REG_TIACN_GAIN_SHIFT;
>>>>>>>>>> +                     ret = 0;
>>>>>>>>>> +                     break;
>>>>>>>>>> +             }
>>>>>>>>>> +     }
>>>>>>>>>> +
>>>>>>>>>> +     if (ret) {
>>>>>>>>>> +             dev_err(dev, "invalid ti,tia-gain-ohm %d\n", val);
>>>>>>>>>> +             return ret;
>>>>>>>>>> +     }
>>>>>>>>>> +
>>>>>>>>>> +     ret = of_property_read_u32(np, "ti,rload-ohm", &val);
>>>>>>>>>> +     if (ret) {
>>>>>>>>>> +             val = 100;
>>>>>>>>>> +             dev_info(dev, "no ti,rload-ohm defined, default to %d\n", val);
>>>>>>>>>> +     }
>>>>>>>>>> +
>>>>>>>>>> +     ret = -EINVAL;
>>>>>>>>>> +     for (i = 0; i < ARRAY_SIZE(lmp91000_rload); i++) {
>>>>>>>>>> +             if (lmp91000_rload[i] == val) {
>>>>>>>>>> +                     reg |= i;
>>>>>>>>>> +                     ret = 0;
>>>>>>>>>> +                     break;
>>>>>>>>>> +             }
>>>>>>>>>> +     }
>>>>>>>>>> +
>>>>>>>>>> +     if (ret) {
>>>>>>>>>> +             dev_err(dev, "invalid ti,rload-ohm %d\n", val);
>>>>>>>>>> +             return ret;
>>>>>>>>>> +     }
>>>>>>>>>> +
>>>>>>>>>> +     regmap_write(data->regmap, LMP91000_REG_LOCK, 0);
>>>>>>>>>> +     regmap_write(data->regmap, LMP91000_REG_TIACN, reg);
>>>>>>>>>> +     regmap_write(data->regmap, LMP91000_REG_REFCN, LMP91000_REG_REFCN_EXT_REF
>>>>>>>>>> +                                     | LMP91000_REG_REFCN_50_ZERO);
>>>>>>>>>> +     regmap_write(data->regmap, LMP91000_REG_LOCK, 1);
>>>>>>>>>
>>>>>>>>> no retval checking
>>>>>>>>
>>>>>>>> Have no issues  with this as well!
>>>>>>>>>
>>>>>>>>>> +
>>>>>>>>>> +     return 0;
>>>>>>>>>> +}
>>>>>>>>>> +
>>>>>>>>>> +static int lmp91000_buffer_cb(const void *val, void *private)
>>>>>>>>>> +{
>>>>>>>>>> +     struct iio_dev *indio_dev = private;
>>>>>>>>>> +     struct lmp91000_data *data = iio_priv(indio_dev);
>>>>>>>>>> +
>>>>>>>>>> +     data->buffer[data->chan_select] = *((int *)val);
>>>>>>>>>> +     complete_all(&data->completion);
>>>>>>>>>> +
>>>>>>>>>> +     return 0;
>>>>>>>>>> +}
>>>>>>>>>> +
>>>>>>>>>> +static const struct iio_trigger_ops lmp91000_trigger_ops = {
>>>>>>>>>> +     .owner = THIS_MODULE,
>>>>>>>>>> +};
>>>>>>>>>> +
>>>>>>>>>
>>>>>>>>> drop newline
>>>>>>>>>
>>>>>>>>>> +
>>>>>>>>>> +static int lmp91000_buffer_preenable(struct iio_dev *indio_dev)
>>>>>>>>>> +{
>>>>>>>>>> +     struct lmp91000_data *data = iio_priv(indio_dev);
>>>>>>>>>> +
>>>>>>>>>> +     return iio_channel_start_all_cb(data->cb_buffer);
>>>>>>>>>> +}
>>>>>>>>>> +
>>>>>>>>>> +static int lmp91000_buffer_predisable(struct iio_dev *indio_dev)
>>>>>>>>>> +{
>>>>>>>>>> +     struct lmp91000_data *data = iio_priv(indio_dev);
>>>>>>>>>> +
>>>>>>>>>> +     iio_channel_stop_all_cb(data->cb_buffer);
>>>>>>>>>> +
>>>>>>>>>> +     return 0;
>>>>>>>>>> +}
>>>>>>>>>> +
>>>>>>>>>> +static const struct iio_buffer_setup_ops lmp91000_buffer_setup_ops = {
>>>>>>>>>> +     .preenable = lmp91000_buffer_preenable,
>>>>>>>>>> +     .postenable = iio_triggered_buffer_postenable,
>>>>>>>>>> +     .predisable = lmp91000_buffer_predisable,
>>>>>>>>>> +};
>>>>>>>>>> +
>>>>>>>>>> +static int lmp91000_probe(struct i2c_client *client,
>>>>>>>>>> +                       const struct i2c_device_id *id)
>>>>>>>>>> +{
>>>>>>>>>> +     struct device *dev = &client->dev;
>>>>>>>>>> +     struct lmp91000_data *data;
>>>>>>>>>> +     struct iio_dev *indio_dev;
>>>>>>>>>> +     int ret;
>>>>>>>>>> +
>>>>>>>>>> +     indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
>>>>>>>>>> +     if (!indio_dev)
>>>>>>>>>> +             return -ENOMEM;
>>>>>>>>>> +
>>>>>>>>>> +     indio_dev->info = &lmp91000_info;
>>>>>>>>>> +     indio_dev->channels = lmp91000_channels;
>>>>>>>>>> +     indio_dev->num_channels = ARRAY_SIZE(lmp91000_channels);
>>>>>>>>>> +     indio_dev->name = LMP91000_DRV_NAME;
>>>>>>>>>> +     indio_dev->modes = INDIO_DIRECT_MODE;
>>>>>>>>>> +     i2c_set_clientdata(client, indio_dev);
>>>>>>>>>> +
>>>>>>>>>> +     data = iio_priv(indio_dev);
>>>>>>>>>> +     data->dev = dev;
>>>>>>>>>> +     data->regmap = devm_regmap_init_i2c(client, &lmp91000_regmap_config);
>>>>>>>>>> +     if (IS_ERR(data->regmap)) {
>>>>>>>>>> +             dev_err(dev, "regmap initialization failed.\n");
>>>>>>>>>> +             return PTR_ERR(data->regmap);
>>>>>>>>>> +     }
>>>>>>>>>> +
>>>>>>>>>> +     data->trig = devm_iio_trigger_alloc(data->dev, "%s-mux%d",
>>>>>>>>>> +                                         indio_dev->name, indio_dev->id);
>>>>>>>>>> +     if (!data->trig) {
>>>>>>>>>> +             dev_err(dev, "cannot allocate iio trigger.\n");
>>>>>>>>>> +             return -ENOMEM;
>>>>>>>>>> +     }
>>>>>>>>>> +
>>>>>>>>>> +     data->trig->ops = &lmp91000_trigger_ops;
>>>>>>>>>> +     data->trig->dev.parent = dev;
>>>>>>>>>> +     init_completion(&data->completion);
>>>>>>>>>> +
>>>>>>>>>> +     ret = lmp91000_read_config(data);
>>>>>>>>>> +     if (ret)
>>>>>>>>>> +             return ret;
>>>>>>>>>> +
>>>>>>>>>> +     ret = iio_triggered_buffer_setup(indio_dev, NULL,
>>>>>>>>>> +                                      &lmp91000_buffer_handler,
>>>>>>>>>> +                                      &lmp91000_buffer_setup_ops);
>>>>>>>>>> +     if (ret)
>>>>>>>>>> +             return ret;
>>>>>>>>>> +
>>>>>>>>>> +     ret = iio_device_register(indio_dev);
>>>>>>>>>> +     if (ret)
>>>>>>>>>> +             goto error_unreg_buffer;
>>>>>>>>>> +
>>>>>>>>>> +     data->cb_buffer = iio_channel_get_all_cb(dev, &lmp91000_buffer_cb,
>>>>>>>>>> +                                              indio_dev);
>>>>>>>>>> +     if (IS_ERR(data->cb_buffer)) {
>>>>>>>>>> +             if (PTR_ERR(data->cb_buffer) == -ENODEV)
>>>>>>>>>> +                     ret = -EPROBE_DEFER;
>>>>>>>>>> +             else
>>>>>>>>>> +                     ret = PTR_ERR(data->cb_buffer);
>>>>>>>>>> +
>>>>>>>>>> +             goto error_unreg_device;
>>>>>>>>>> +     }
>>>>>>>>>> +
>>>>>>>>>> +     ret = iio_trigger_register(data->trig);
>>>>>>>>>> +     if (ret) {
>>>>>>>>>> +             dev_err(dev, "cannot register iio trigger.\n");
>>>>>>>>>> +             goto error_unreg_cb_buffer;
>>>>>>>>>> +     }
>>>>>>>>>> +
>>>>>>>>>> +     return iio_trigger_set_immutable(iio_channel_cb_get_iio_dev(data->cb_buffer),
>>>>>>>>>> +                                      data->trig);
>>>>>>>>>> +
>>>>>>>>>> +error_unreg_cb_buffer:
>>>>>>>>>> +     iio_channel_release_all_cb(data->cb_buffer);
>>>>>>>>>> +
>>>>>>>>>> +error_unreg_device:
>>>>>>>>>> +     iio_device_unregister(indio_dev);
>>>>>>>>>> +
>>>>>>>>>> +error_unreg_buffer:
>>>>>>>>>> +     iio_triggered_buffer_cleanup(indio_dev);
>>>>>>>>>> +
>>>>>>>>>> +     return ret;
>>>>>>>>>> +}
>>>>>>>>>> +
>>>>>>>>>> +static int lmp91000_remove(struct i2c_client *client)
>>>>>>>>>> +{
>>>>>>>>>> +     struct iio_dev *indio_dev = i2c_get_clientdata(client);
>>>>>>>>>> +     struct lmp91000_data *data = iio_priv(indio_dev);
>>>>>>>>>> +
>>>>>>>>>> +     iio_device_unregister(indio_dev);
>>>>>>>>>> +
>>>>>>>>>> +     iio_channel_stop_all_cb(data->cb_buffer);
>>>>>>>>>> +     iio_channel_release_all_cb(data->cb_buffer);
>>>>>>>>>> +
>>>>>>>>>> +     iio_triggered_buffer_cleanup(indio_dev);
>>>>>>>>>> +     iio_trigger_unregister(data->trig);
>>>>>>>>>> +
>>>>>>>>>> +     return 0;
>>>>>>>>>> +}
>>>>>>>>>> +
>>>>>>>>>> +static const struct of_device_id lmp91000_of_match[] = {
>>>>>>>>>> +     { .compatible = "ti,lmp91000", },
>>>>>>>>>> +     { },
>>>>>>>>>> +};
>>>>>>>>>> +MODULE_DEVICE_TABLE(of, lmp91000_of_match);
>>>>>>>>>> +
>>>>>>>>>> +static const struct i2c_device_id lmp91000_id[] = {
>>>>>>>>>> +     { "lmp91000", 0 },
>>>>>>>>>> +     {}
>>>>>>>>>> +};
>>>>>>>>>> +MODULE_DEVICE_TABLE(i2c, lmp91000_id);
>>>>>>>>>> +
>>>>>>>>>> +static struct i2c_driver lmp91000_driver = {
>>>>>>>>>> +     .driver = {
>>>>>>>>>> +             .name = LMP91000_DRV_NAME,
>>>>>>>>>> +             .of_match_table = of_match_ptr(lmp91000_of_match),
>>>>>>>>>> +     },
>>>>>>>>>> +     .probe = lmp91000_probe,
>>>>>>>>>> +     .remove = lmp91000_remove,
>>>>>>>>>> +     .id_table = lmp91000_id,
>>>>>>>>>> +};
>>>>>>>>>> +module_i2c_driver(lmp91000_driver);
>>>>>>>>>> +
>>>>>>>>>> +MODULE_AUTHOR("Matt Ranostay <mranostay@gmail.com>");
>>>>>>>>>> +MODULE_DESCRIPTION("LMP91000 digital potentiostat");
>>>>>>>>>> +MODULE_LICENSE("GPL");
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> --
>>>>>>>>>
>>>>>>>>> Peter Meerwald-Stadler
>>>>>>>>> +43-664-2444418 (mobile)
>>>>>
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>