On Mon, Sep 5, 2016 at 10:43 PM, Peter Meerwald-Stadler <pmeerw@xxxxxxxxxx> 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@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, > > why not IIO_TEMP? Because isn't linear enough. and scaling value wouldn't help... Thoughts? > >> + .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@xxxxxxxxx>"); >> +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@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
