Re: [RFC v4 3/3] iio: potentiostat: add LMP91000 support

[Jonathan Cameron <jic23@kernel.org>]

On 06/09/16 02:58, Matt Ranostay wrote:
> On Sat, Sep 3, 2016 at 9:56 PM, Matt Ranostay <mranostay@gmail.com> wrote:
>> On Sat, Sep 3, 2016 at 7:59 AM, Jonathan Cameron <jic23@kernel.org> wrote:
>>> On 30/08/16 07:01, Matt Ranostay wrote:
>>>> On Mon, Aug 29, 2016 at 10:38 AM, Jonathan Cameron <jic23@kernel.org> wrote:
>>>>> On 26/08/16 04:09, Matt Ranostay wrote:
>>>>>> On Sun, Aug 21, 2016 at 3:46 AM, Jonathan Cameron <jic23@kernel.org> wrote:
>>>>>>> On 20/08/16 04:17, Matt Ranostay wrote:
>>>>>>>> Add support for the LMP91000 potentiostat which is used for chemical
>>>>>>>> sensing applications.
>>>>>>>>
>>>>>>>> Signed-off-by: Matt Ranostay <mranostay@gmail.com>
>>>>>>> As this is 'unusual' I'd appreciate more docs in general of what is
>>>>>>> going on.
>>>>>>>
>>>>>>> Mostly looking good but few bits and bobs that I think need tweaking inline.
>>>>>>>
>>>>>>> I'd be inclined to factor out the relevant code in industrialio-trigger.c
>>>>>>> (iio_trigger_write_current) to do validation etc as well on this.
>>>>>>> It's not strickly necessary but might be nice. Also you can then put
>>>>>>> a 'exclusive' mode check in there to prevent sysfs writes changing the
>>>>>>> trigger on you.
>>>>>>>
>>>>>>>> ---
>>>>>>>>  .../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                | 377 +++++++++++++++++++++
>>>>>>>>  6 files changed, 437 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 6743b18194fb..a31a8cf2c330 100644
>>>>>>>> --- a/drivers/iio/Kconfig
>>>>>>>> +++ b/drivers/iio/Kconfig
>>>>>>>> @@ -87,6 +87,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 87e4c4369e2f..2b6e2762a886 100644
>>>>>>>> --- a/drivers/iio/Makefile
>>>>>>>> +++ b/drivers/iio/Makefile
>>>>>>>> @@ -29,6 +29,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..5046d2d3da98
>>>>>>>> --- /dev/null
>>>>>>>> +++ b/drivers/iio/potentiostat/lmp91000.c
>>>>>>>> @@ -0,0 +1,377 @@
>>>>>>>> +/*
>>>>>>>> + * 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,
>>>>>>>> +             .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;
>>>>>>>> +
>>>>>>>> +     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;
>>>>>>>> +
>>>>>>> I'd like a comment here to explain that this is causing the sample to
>>>>>>> be taken.
>>>>>>>
>>>>>>> To get it clear in my head:
>>>>>>> 1) Calls handle_nested_irq (should rename this function in general as my
>>>>>>> understanding of the meaning of chained interrupts was actually wrong when
>>>>>>> I came up with this name).
>>>>>>
>>>>>> Yeah the naming kinda doesn't make sense, and to be honest wasn't that
>>>>>> clear to me initially.
>>>>> Hmm. If you fancy fixing that feel free ;)
>>>>>>
>>>>>>> 2) poll func of the ADC is called (bottom half only, but that's fine).
>>>>>>> 3) This calls iio_push_to_buffer.
>>>>>>> 4) The demux plucks on the relevant channel (only one enabled so that's easy
>>>>>>> :) and passes it on to the callback buffer.
>>>>>>> 5) The completion below finishes, data is copied out and pushed into the
>>>>>>> buffer of this device with a timestamp added etc. Job done.
>>>>>>
>>>>>> Correct!
>>>>>>
>>>>>>>
>>>>>>>> +     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 = iio_channel_start_all_cb(data->cb_buffer);
>>>>>>>> +     if (ret)
>>>>>>>> +             goto error_out;
>>>>>>> Why the need for the explicit start each time?  You have control of the
>>>>>>> trigger so it shouldn't capture otherwise.  This is a fairly heavy weight
>>>>>>> operation so best avoided if we can keep it running across multiple scans.
>>>>>>
>>>>>> There is a few issues with locking in the code doing that...
>>>>>> indio_dev->mlock is already held before iio_channel_start_all_cb. So
>>>>>> we need to really rework a few things or have this hack for now.
>>>>> I'm not sure I follow.
>>>>>
>>>>> Talk me through the locking issue with doing the start in the preenable for
>>>>> the buffer and the stop in the post disable?
>>>>>>
>>>>
>>>> Yeah you get this doing that.... seems multiple attempts to get mlock
>>>> mutex with the update_buffers...
>>>
>>> Hmm. Let me try and get my head around this. I'm going to call the
>>> front end device the outer, and the adc the inner device..
>>>
>>> We call enable on the outer buffer. (iio_buffer_store_enable).
>>> This takes the outer mlock and calls
>>> __iio_update_buffers which calls iio_enable_buffers.
>>> This calls indio_dev->setup_ops->preenable (for the outer device).
>>>
>>> This preenable will call the iio_channel_start_all_cb which looks
>>> dodgy as it'll call a nested iio_update_buffers which takes the
>>> lock.  However, it should be calling it on the inner device whose
>>> mlock we haven't taken yet.
>>>
>>> So why is it going wrong?  I can't immediately see...
>>>
>>> If I get a chance later I'll mock this up and see if I can track down
>>> why it's a problem.  May well not get back to it this weekend though.
>>>
>>
>> I am really perplexed as well since it should be different indio_dev->mlock's
>>
>>> If you get a chance to dig some more that would be great!
> 
> Hmm is this a possible lockdep issue? Seen a few fixes go into in the
> stable branch..
Honestly, I have no idea!

Jonathan
> 
>>>
>>> Jonathan
>>>
>>>>
>>>>
>>>>    77.959336] =============================================
>>>> [   77.963451] [ INFO: possible recursive locking detected ]
>>>> [   77.967575] 4.7.0-rc4-00044-g0e4912e9bd3b-dirty #15 Not tainted
>>>> [   77.972212] ---------------------------------------------
>>>> [   77.976335] sh/321 is trying to acquire lock:
>>>> [   77.979406]  (&dev->mlock){+.+.+.}, at: [<c0630ce0>]
>>>> iio_update_buffers+0x38/0xd4
>>>> [   77.985700]
>>>> but task is already holding lock:
>>>> [   77.988940]  (&dev->mlock){+.+.+.}, at: [<c0630db0>]
>>>> iio_buffer_store_enable+0x34/0x98
>>>> [   77.995632]
>>>> other info that might help us debug this:
>>>> [   77.999572]  Possible unsafe locking scenario:
>>>> [   78.002899]        CPU0
>>>> [   78.004045]        ----
>>>> [   78.005190]   lock(&dev->mlock);
>>>> [   78.007142]   lock(&dev->mlock);
>>>> [   78.009095]
>>>>  *** DEADLOCK ***
>>>> [   78.011115]  May be due to missing lock nesting notation
>>>> [   78.015317] 5 locks held by sh/321:
>>>> [   78.017511]  #0:  (sb_writers#4){.+.+.+}, at: [<c02967d0>]
>>>> __sb_start_write+0x8c/0xb0
>>>> [   78.024143]  #1:  (&of->mutex){+.+.+.}, at: [<c0314184>]
>>>> kernfs_fop_write+0x88/0x1f0
>>>> [   78.030677]  #2:  (s_active#80){.+.+.+}, at: [<c031418c>]
>>>> kernfs_fop_write+0x90/0x1f0
>>>> [   78.037300]  #3:  (&dev->mlock){+.+.+.}, at: [<c0630db0>]
>>>> iio_buffer_store_enable+0x34/0x98
>>>> [   78.044435]  #4:  (&dev->info_exist_lock){+.+...}, at: [<c0630cd4>]
>>>> iio_update_buffers+0x2c/0xd4
>>>> [   78.052006]
>>>> stack backtrace:
>>>> [   78.053773] PU: 0 PID: 321 Comm: sh Not tainted
>>>> 4.7.0-rc4-00044-g0e4912e9bd3b-dirty #15
>>>> [   78.060591] Hardware name: Generic AM33XX (Flattened Device Tree)
>>>> [   78.065428] [<c0110060>] (unwind_backtrace) from [<c010c1f8>]
>>>> (show_stack+0x10/0x14)
>>>> [   78.071919] [<c010c1f8>] (show_stack) from [<c0485904>]
>>>> (dump_stack+0xac/0xe0)
>>>> [   78.077884] [<c0485904>] (dump_stack) from [<c0190d38>]
>>>> (__lock_acquire+0xf0c/0x1880)
>>>> [   78.084450] [<c0190d38>] (__lock_acquire) from [<c0191ad4>]
>>>> (lock_acquire+0xcc/0x238)
>>>> [   78.091027] [<c0191ad4>] (lock_acquire) from [<c0751118>]
>>>> (mutex_lock_nested+0x3c/0x3d4)
>>>> [   78.097856] [<c0751118>] (mutex_lock_nested) from [<c0630ce0>]
>>>> (iio_update_buffers+0x38/0xd4)
>>>> [   78.105120] [<c0630ce0>] (iio_update_buffers) from [<c0630c50>]
>>>> (__iio_update_buffers+0x440/0x498)
>>>> [   78.112820] [<c0630c50>] (__iio_update_buffers) from [<c0630de4>]
>>>> (iio_buffer_store_enable+0x68/0x98)
>>>> [   78.120784] [<c0630de4>] (iio_buffer_store_enable) from
>>>> [<c03141c0>] (kernfs_fop_write+0xc4/0x1f0)
>>>> [   78.128486] [<c03141c0>] (kernfs_fop_write) from [<c02927b8>]
>>>> (__vfs_write+0x1c/0x114)
>>>> [   78.135136] [<c02927b8>] (__vfs_write) from [<c02936f8>]
>>>> (vfs_write+0xa0/0x180)
>>>> [   78.141176] [<c02936f8>] (vfs_write) from [<c0294450>] (SyS_write+0x3c/0x90)
>>>> [   78.146965] [<c0294450>] (SyS_write) from [<c0107840>]
>>>> (ret_fast_syscall+0x0/0x1c)
>>>>
>>>>>>>
>>>>>>> Would have thought you can just do this in the postenable callback. Then
>>>>>>> disable in the predisable.
>>>>>>>> +
>>>>>>>> +     ret = lmp91000_read(data, LMP91000_REG_MODECN_3LEAD, &val);
>>>>>>>> +     iio_channel_stop_all_cb(data->cb_buffer);
>>>>>>>> +
>>>>>>>> +     if (!ret) {
>>>>>>>> +             data->buffer[0] = val;
>>>>>>>> +             iio_push_to_buffers_with_timestamp(indio_dev, data->buffer,
>>>>>>>> +                                                iio_get_time_ns(indio_dev));
>>>>>>>> +     }
>>>>>>>> +
>>>>>>>> +error_out:
>>>>>>>> +     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;
>>>>>>>> +
>>>>>>> I'd protect this with claim_direct.  We don't want this occuring
>>>>>>> mid way through a buffered read - or two of these happening at
>>>>>>> once.
>>>>>>>
>>>>>>>> +     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;
>>>>>>>> +     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);
>>>>>>>> +
>>>>>>>> +     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,
>>>>>>>> +};
>>>>>>>> +
>>>>>>>> +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;
>>>>>>>> +
>>>>>>>> +     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)
>>>>>>>> +                     return -EPROBE_DEFER;
>>>>>>>> +             return PTR_ERR(data->cb_buffer);
>>>>>>>> +     }
>>>>>>>> +
>>>>>>>> +     ret = iio_triggered_buffer_setup(indio_dev, NULL,
>>>>>>>> +                                      &lmp91000_buffer_handler, NULL);
>>>>>>>> +     if (ret)
>>>>>>>> +             goto error_unreg_cb_buffer;
>>>>>>>> +
>>>>>>>> +     ret = iio_trigger_register(data->trig);
>>>>>>>> +     if (ret) {
>>>>>>>> +             dev_err(dev, "cannot register iio trigger.\n");
>>>>>>>> +             goto error_unreg_cb_buffer;
>>>>>>>> +     }
>>>>>>>> +
>>>>>>>> +     ret = iio_device_register(indio_dev);
>>>>>>>> +     if (ret)
>>>>>>>> +             goto error_unreg_buffer;
>>>>>>>> +
>>>>>>>
>>>>>>>> +     iio_channel_cb_get_iio_dev(data->cb_buffer)->trig =
>>>>>>>> +                                     iio_trigger_get(data->trig);
>>>>>>>
>>>>>>> Hmm. Is this really all there is to it?
>>>>>>>
>>>>>>> * What prevents the trigger from being changed?  The buffer is only enabled
>>>>>>>   currently during each reading so there is plenty of time to 'steal' it.
>>>>>>>   We need some for of 'exclusive' lock on this. As we are explicitly clocking
>>>>>>>   the ADC capture from here I can't see where a non exclusive mode would work
>>>>>>>   without some very very fiddly handling.
>>>>>>>
>>>>>>
>>>>>> Ok yeah this is a bit ugly... and you are correct the trigger could be changed.
>>>>>> So do we need to have a flag in the iio_dev struct that signals the
>>>>>> trigger isn't changeable?
>>>>> Yes, something along those lines I think.
>>>>>>
>>>>>>> * I'd prefer this happening through a utility function as it's more
>>>>>>>   that possible it'll be used in drivers that don't know about struct iio_dev.
>>>>>>>   So keep that opaque.
>>>>>>>
>>>>>>> It's racey with the iio_device_register... So probably needs to be
>>>>>>> prior to that.  I'd even be inclined to work out how to do the cb_buffer
>>>>>>> start in the probe and stop in the remove.  I'm not 100% sure all the
>>>>>>> infrastructure is in place before the iio_device_register though. If it's
>>>>>>> not we may need ot think about adding another stage for this usecase.
>>>>>>> Might just push races into iio_device_register itself.
>>>>>>>
>>>>>>
>>>>>> See locking issues noted above :).
>>>>> We need to fix those...  Should not be the case.
>>>>>>
>>>>>>>> +
>>>>>>>> +     return 0;
>>>>>>>> +
>>>>>>>> +error_unreg_buffer:
>>>>>>>> +     iio_triggered_buffer_cleanup(indio_dev);
>>>>>>>> +     iio_trigger_unregister(data->trig);
>>>>>>>> +
>>>>>>>> +error_unreg_cb_buffer:
>>>>>>>> +     iio_channel_release_all_cb(data->cb_buffer);
>>>>>>>> +
>>>>>>>> +     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");
>>>>>>>>
>>>>>>>
>>>>>> --
>>>>>> 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
>>>>>>
>>>>>
>>>