On 06/09/16 02:58, Matt Ranostay wrote: > On Sat, Sep 3, 2016 at 9:56 PM, Matt Ranostay <mranostay@xxxxxxxxx> wrote: >> On Sat, Sep 3, 2016 at 7:59 AM, Jonathan Cameron <jic23@xxxxxxxxxx> wrote: >>> On 30/08/16 07:01, Matt Ranostay wrote: >>>> On Mon, Aug 29, 2016 at 10:38 AM, Jonathan Cameron <jic23@xxxxxxxxxx> wrote: >>>>> On 26/08/16 04:09, Matt Ranostay wrote: >>>>>> On Sun, Aug 21, 2016 at 3:46 AM, Jonathan Cameron <jic23@xxxxxxxxxx> 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@xxxxxxxxx> >>>>>>> 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@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. >>>>>>>> + * >>>>>>>> + * 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@xxxxxxxxx>"); >>>>>>>> +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@xxxxxxxxxxxxxxx >>>>>> More majordomo info at http://vger.kernel.org/majordomo-info.html >>>>>> >>>>> >>> -- 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
