* [PATCH 1/2] iio: adc: driver for texas instruments ads7142
@ 2021-05-01 18:24 Jozsef Horvath
2021-05-01 18:25 ` [PATCH 2/2] dt-bindings: iio: adc: devicetree bindings for texas instruments ads7142 iio driver Jozsef Horvath
2021-05-02 17:14 ` [PATCH 1/2] iio: adc: driver for texas instruments ads7142 Jonathan Cameron
0 siblings, 2 replies; 10+ messages in thread
From: Jozsef Horvath @ 2021-05-01 18:24 UTC (permalink / raw)
To: Jozsef Horvath, Jonathan Cameron, Lars-Peter Clausen,
Peter Meerwald-Stadler, Rob Herring, Liam Girdwood, Mark Brown,
Alexandru Ardelean, Heiko Stuebner, Andy Shevchenko, Alex Dewar,
Gene Chen, Saravanan Sekar, Lee Jones, linux-iio, devicetree,
linux-kernel
This is an iio driver for
Texas Instruments ADS7142 dual-channel, programmable sensor monitor.
Operation modes supportedby the driver:
When the 'ti,monitoring-mode' property is not present
in the devicetree node definition, the driver initiates a single
conversion in the device for each read request
(/sys/bus/iio/devices/iio:deviceX/in_voltageY_raw).
This is a one-shot conversion, and it is called
"Manual Mode" in the datasheet.
When the 'ti,monitoring-mode' property is present
in the devicetree node definition, the driver configures
the device's digital window comparator and sets the device's
data buffer operation mode to pre alert data mode.
The driver reads the conversion result when the BUSY/RDY interrupt
fires, and keeps the value until the next BUSY/RDY interrupt
or the first read request
(/sys/bus/iio/devices/iio:deviceX/in_voltageY_raw).
The digital window comparator and hysteresis parameters
can be controlled by:
- the devicetree definition of channel node
- iio sysfs interfaces
This is event driven conversion, and is called
"Autonomous Mode with Pre Alert Data" in the datasheet.
This mode can be used to wake up the system with the ALERT pin,
in case when the monitored voltage level is out of the configured range.
Datasheet: https://www.ti.com/lit/ds/symlink/ads7142.pdf
Signed-off-by: Jozsef Horvath <info@ministro.hu>
---
---
MAINTAINERS | 6 +
drivers/iio/adc/Kconfig | 10 +
drivers/iio/adc/Makefile | 1 +
drivers/iio/adc/ti-ads7142.c | 1215 ++++++++++++++++++++++++++++++++++
4 files changed, 1232 insertions(+)
create mode 100644 drivers/iio/adc/ti-ads7142.c
diff --git a/MAINTAINERS b/MAINTAINERS
index 9450e052f1b1..954cbabde801 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -17694,6 +17694,12 @@ T: git git://github.com/czankel/xtensa-linux.git
F: arch/xtensa/
F: drivers/irqchip/irq-xtensa-*
+TEXAS INSTRUMENTS ADS7142 ADC DRIVER
+M: Jozsef Horvath <info@ministro.hu>
+S: Maintained
+F: Documentation/devicetree/bindings/iio/adc/ti,ads7142.yaml
+F: drivers/iio/adc/ti-ads7142.c
+
TEXAS INSTRUMENTS ASoC DRIVERS
M: Peter Ujfalusi <peter.ujfalusi@gmail.com>
L: alsa-devel@alsa-project.org (moderated for non-subscribers)
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index e0667c4b3c08..69d672298719 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -1111,6 +1111,16 @@ config TI_ADS1015
This driver can also be built as a module. If so, the module will be
called ti-ads1015.
+config TI_ADS7142
+ tristate "Texas Instruments ADS7142 ADC driver"
+ depends on I2C
+ help
+ This driver is for Texas Instruments ADS7142 Nanopower, Dual-Channel, Programmable Sensor Monitor.
+ Say 'Y' here if you wish to use it.
+
+ To compile this driver as a module, choose M here: the
+ module will be called ti-ads7142.
+
config TI_ADS7950
tristate "Texas Instruments ADS7950 ADC driver"
depends on SPI && GPIOLIB
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index 5fca90ada0ec..83d2a72ab758 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -99,6 +99,7 @@ obj-$(CONFIG_TI_ADC108S102) += ti-adc108s102.o
obj-$(CONFIG_TI_ADC128S052) += ti-adc128s052.o
obj-$(CONFIG_TI_ADC161S626) += ti-adc161s626.o
obj-$(CONFIG_TI_ADS1015) += ti-ads1015.o
+obj-$(CONFIG_TI_ADS7142) += ti-ads7142.o
obj-$(CONFIG_TI_ADS7950) += ti-ads7950.o
obj-$(CONFIG_TI_ADS8344) += ti-ads8344.o
obj-$(CONFIG_TI_ADS8688) += ti-ads8688.o
diff --git a/drivers/iio/adc/ti-ads7142.c b/drivers/iio/adc/ti-ads7142.c
new file mode 100644
index 000000000000..6e42efe5f1e8
--- /dev/null
+++ b/drivers/iio/adc/ti-ads7142.c
@@ -0,0 +1,1215 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2020 Jozsef Horvath <info@ministro.hu>
+ *
+ */
+#include <linux/i2c.h>
+#include <linux/device.h>
+#include <linux/interrupt.h>
+#include <linux/err.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/regulator/consumer.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/events.h>
+
+#define TI_ADS7142_NAME "ads7142"
+
+#define TI_ADS7142_DATA_VALID_TIMEOUT 100
+
+/* Opcodes for commands */
+/* General */
+#define TI_ADS7142_OC_GENERAL 0x00
+/* Single Register Read */
+#define TI_ADS7142_OC_SINGLE_REG_READ 0x10
+/* Single Register Write */
+#define TI_ADS7142_OC_SINGLE_REG_WRITE 0x08
+/* Single Bit Set */
+#define TI_ADS7142_OC_SET_BIT 0x18
+/* Single Bit Clear */
+#define TI_ADS7142_OC_CLEAR_BIT 0x20
+/* Block Register Read */
+#define TI_ADS7142_OC_BLOCK_READ 0x30
+/* Block Register Write */
+#define TI_ADS7142_OC_BLOCK_WRITE 0x28
+
+/* Registers */
+/* Reset registers */
+#define TI_ADS7142_WKEY 0x17
+#define TI_ADS7142_DEVICE_RESET 0x14
+/* Functional mode select registers */
+#define TI_ADS7142_OFFSET_CAL 0x15
+#define TI_ADS7142_OPMODE_SEL 0x1C
+#define TI_ADS7142_OPMODE_SEL_MANUALCH0 (0)
+#define TI_ADS7142_OPMODE_SEL_MANUALSEQ (4)
+#define TI_ADS7142_OPMODE_SEL_MONITORING (6)
+#define TI_ADS7142_OPMODE_SEL_HIGHPREC (7)
+#define TI_ADS7142_OPMODE_STATUS 0x00
+#define TI_ADS7142_OPMODE_STATUS_OPMODE_MSK (3)
+#define TI_ADS7142_OPMODE_STATUS_OPMODE_MANUAL (0)
+#define TI_ADS7142_OPMODE_STATUS_OPMODE_AUTO (2)
+#define TI_ADS7142_OPMODE_STATUS_OPMODE_HIGHPREC (3)
+#define TI_ADS7142_OPMODE_STATUS_HS_MODE BIT(2)
+
+/* Input config register */
+#define TI_ADS7142_CH_INPUT_CFG 0x24
+#define TI_ADS7142_CH_INPUT_CFG_TCSE (0)
+#define TI_ADS7142_CH_INPUT_CFG_SCSE (1)
+#define TI_ADS7142_CH_INPUT_CFG_SCPD (2)
+/* Analog mux and sequencer registers */
+#define TI_ADS7142_AUTO_SEQ_CHEN 0x20
+#define TI_ADS7142_AUTO_SEQ_CHEN_CH0 BIT(0)
+#define TI_ADS7142_AUTO_SEQ_CHEN_CH1 BIT(1)
+#define TI_ADS7142_START_SEQUENCE 0x1E
+#define TI_ADS7142_START_SEQUENCE_SEQ_START BIT(0)
+#define TI_ADS7142_ABORT_SEQUENCE 0x1F
+#define TI_ADS7142_ABORT_SEQUENCE_SEQ_ABORT BIT(0)
+#define TI_ADS7142_SEQUENCE_STATUS 0x04
+#define TI_ADS7142_SEQUENCE_STATUS_SEQ_ERR_ST_MSK (0x06)
+#define TI_ADS7142_SEQUENCE_STATUS_SEQ_DISABLED (0x00)
+#define TI_ADS7142_SEQUENCE_STATUS_SEQ_ENABLED (0x02)
+#define TI_ADS7142_SEQUENCE_STATUS_SEQ_ERROR (0x06)
+/* Oscillator and timing control registers */
+#define TI_ADS7142_OSC_SEL 0x18
+#define TI_ADS7142_OSC_SEL_HSZ_LP BIT(0)
+#define TI_ADS7142_NCLK_SEL 0x19
+/* Data buffer control register */
+#define TI_ADS7142_DATA_BUFFER_OPMODE 0x2C
+#define TI_ADS7142_DATA_BUFFER_OPMODE_STOP_BURST (0)
+#define TI_ADS7142_DATA_BUFFER_OPMODE_START_BURST (1)
+#define TI_ADS7142_DATA_BUFFER_OPMODE_PRE_ALERT (4)
+#define TI_ADS7142_DATA_BUFFER_OPMODE_POST_ALERT (6)
+#define TI_ADS7142_DOUT_FORMAT_CFG 0x28
+#define TI_ADS7142_DOUT_FORMAT_CFG_12B (0)
+#define TI_ADS7142_DOUT_FORMAT_CFG_12BCH (1)
+#define TI_ADS7142_DOUT_FORMAT_CFG_12BCHDV (2)
+#define TI_ADS7142_DATA_BUFFER_STATUS 0x01
+/* Accumulator control register */
+#define TI_ADS7142_ACC_EN 0x30
+#define TI_ADS7142_ACC_CH0_LSB 0x08
+#define TI_ADS7142_ACC_CH0_MSB 0x09
+#define TI_ADS7142_ACC_CH1_LSB 0x0A
+#define TI_ADS7142_ACC_CH1_MSB 0x0B
+#define TI_ADS7142_ACC_STATUS 0x02
+/* Digital window comparator registers */
+#define TI_ADS7142_ALERT_DWC_EN 0x37
+#define TI_ADS7142_ALERT_DWC_EN_BLOCK_EN BIT(0)
+#define TI_ADS7142_ALERT_CHEN 0x34
+#define TI_ADS7142_DWC_HTH_CH0_LSB 0x38
+#define TI_ADS7142_DWC_HTH_CH0_MSB 0x39
+#define TI_ADS7142_DWC_LTH_CH0_LSB 0x3A
+#define TI_ADS7142_DWC_LTH_CH0_MSB 0x3B
+#define TI_ADS7142_DWC_HYS_CH0 0x40
+#define TI_ADS7142_DWC_HTH_CH1_LSB 0x3C
+#define TI_ADS7142_DWC_HTH_CH1_MSB 0x3D
+#define TI_ADS7142_DWC_LTH_CH1_LSB 0x3E
+#define TI_ADS7142_DWC_LTH_CH1_MSB 0x3F
+#define TI_ADS7142_DWC_HYS_CH1 0x41
+#define TI_ADS7142_PRE_ALT_EVT_CNT 0x36
+#define TI_ADS7142_ALT_TRIG_CHID 0x03
+#define TI_ADS7142_ALT_LOW_FLAGS 0x0C
+#define TI_ADS7142_ALT_LOW_FLAGS_CH0 BIT(0)
+#define TI_ADS7142_ALT_LOW_FLAGS_CH1 BIT(1)
+#define TI_ADS7142_ALT_HIGH_FLAGS 0x0E
+#define TI_ADS7142_ALT_HIGH_FLAGS_CH0 BIT(0)
+#define TI_ADS7142_ALT_HIGH_FLAGS_CH1 BIT(1)
+
+#define TI_ADS7142_THRESHOLD_MSK 0xFFF
+#define TI_ADS7142_HYSTERESIS_MSK 0x3F
+
+struct ti_ads7142_channel_data {
+ int status;
+ int value;
+};
+
+struct ti_ads7142_channel_config {
+ bool alert_low;
+ bool alert_high;
+ int high_threshold;
+ int low_threshold;
+ int hysteresis;
+};
+
+struct ti_ads7142_channel {
+ struct ti_ads7142_channel_config config;
+ struct ti_ads7142_channel_data data;
+ u32 channel;
+};
+
+struct ti_ads7142_config {
+ bool osc_sel;
+ u32 n_clk;
+ bool monitoring_mode;
+};
+
+struct ti_ads7142_priv {
+ struct mutex lock; /* For syncing access to device */
+ struct regulator *vref;
+ struct regulator *power;
+ struct ti_ads7142_config config;
+ int channel_count;
+ struct ti_ads7142_channel *channels;
+ bool monitor_pending;
+};
+
+static const struct iio_event_spec ti_ads7142_events[] = {
+ {
+ .type = IIO_EV_TYPE_THRESH,
+ .dir = IIO_EV_DIR_RISING,
+ .mask_separate = BIT(IIO_EV_INFO_VALUE)
+ | BIT(IIO_EV_INFO_ENABLE),
+ }, {
+ .type = IIO_EV_TYPE_THRESH,
+ .dir = IIO_EV_DIR_FALLING,
+ .mask_separate = BIT(IIO_EV_INFO_VALUE)
+ | BIT(IIO_EV_INFO_ENABLE),
+ }, {
+ .type = IIO_EV_TYPE_THRESH,
+ .dir = IIO_EV_DIR_EITHER,
+ .mask_separate = BIT(IIO_EV_INFO_HYSTERESIS),
+ },
+};
+
+static int ti_ads7142_reg_write(const struct i2c_client *client, u8 reg,
+ u8 data)
+{
+ struct i2c_msg msg;
+ u8 buf[3];
+ int ret;
+
+ buf[0] = TI_ADS7142_OC_SINGLE_REG_WRITE;
+ buf[1] = reg;
+ buf[2] = data;
+
+ msg.addr = client->addr;
+ msg.flags = 0;
+ msg.len = 3;
+ msg.buf = buf;
+
+ ret = i2c_transfer(client->adapter, &msg, 1);
+
+ return ret >= 0 ? 0 : ret;
+}
+
+static int ti_ads7142_reg_read(const struct i2c_client *client, u8 reg,
+ u8 *data)
+{
+ struct i2c_msg msg[2];
+ u8 buf[2];
+ int ret;
+
+ buf[0] = TI_ADS7142_OC_SINGLE_REG_READ;
+ buf[1] = reg;
+
+ msg[0].addr = client->addr;
+ msg[0].flags = 0;
+ msg[0].len = 2;
+ msg[0].buf = buf;
+
+ msg[1].addr = client->addr;
+ msg[1].flags = I2C_M_RD;
+ msg[1].len = 1;
+ msg[1].buf = data;
+
+ ret = i2c_transfer(client->adapter, msg, 2);
+
+ return ret >= 0 ? 0 : ret;
+}
+
+static int ti_ads7142_data_buffer_read(const struct i2c_client *client,
+ int length, void *data)
+{
+ struct i2c_msg msg;
+ int ret;
+
+ msg.addr = client->addr;
+ msg.flags = I2C_M_RD;
+ msg.len = length;
+ msg.buf = data;
+
+ ret = i2c_transfer(client->adapter, &msg, 1);
+
+ return ret >= 0 ? 0 : ret;
+}
+
+static int ti_ads7142_soft_reset(const struct i2c_client *client)
+{
+ struct i2c_msg msg;
+ u8 buf[2];
+ int ret;
+
+ buf[0] = TI_ADS7142_OC_GENERAL;
+ buf[1] = 0x06;
+
+ msg.addr = client->addr;
+ msg.flags = 0;
+ msg.len = 2;
+ msg.buf = buf;
+
+ ret = i2c_transfer(client->adapter, &msg, 1);
+
+ return ret >= 0 ? 0 : ret;
+}
+
+static int ti_ads7142_address2channel(struct iio_dev *indio_dev,
+ int address,
+ struct ti_ads7142_channel **channel)
+{
+ struct ti_ads7142_priv *priv = iio_priv(indio_dev);
+ int i;
+
+ for (i = 0; i < priv->channel_count; i++) {
+ if (address == priv->channels[i].channel) {
+ *channel = &priv->channels[i];
+ return 0;
+ }
+ }
+ return -ENODEV;
+}
+
+static int ti_ads7142_sequence_start(struct iio_dev *indio_dev)
+{
+ struct i2c_client *client = to_i2c_client(indio_dev->dev.parent);
+
+ return ti_ads7142_reg_write(client, TI_ADS7142_START_SEQUENCE,
+ TI_ADS7142_START_SEQUENCE_SEQ_START);
+}
+
+static int ti_ads7142_sequence_abort(struct iio_dev *indio_dev)
+{
+ struct i2c_client *client = to_i2c_client(indio_dev->dev.parent);
+
+ return ti_ads7142_reg_write(client, TI_ADS7142_ABORT_SEQUENCE,
+ TI_ADS7142_ABORT_SEQUENCE_SEQ_ABORT);
+}
+
+static int ti_ads7142_osc_set(struct iio_dev *indio_dev)
+{
+ struct ti_ads7142_priv *priv = iio_priv(indio_dev);
+ struct i2c_client *client = to_i2c_client(indio_dev->dev.parent);
+ int ret;
+
+ ret = ti_ads7142_reg_write(client, TI_ADS7142_OSC_SEL,
+ priv->config.osc_sel ? TI_ADS7142_OSC_SEL_HSZ_LP : 0);
+ if (ret)
+ return ret;
+
+ return ti_ads7142_reg_write(client, TI_ADS7142_NCLK_SEL,
+ priv->config.n_clk);
+}
+
+static int ti_ads7142_input_cfg_set(struct iio_dev *indio_dev)
+{
+ struct i2c_client *client = to_i2c_client(indio_dev->dev.parent);
+
+ return ti_ads7142_reg_write(client, TI_ADS7142_CH_INPUT_CFG,
+ TI_ADS7142_CH_INPUT_CFG_TCSE);
+}
+
+static int ti_ads7142_dout_format_set(struct iio_dev *indio_dev)
+{
+ struct i2c_client *client = to_i2c_client(indio_dev->dev.parent);
+
+ return ti_ads7142_reg_write(client, TI_ADS7142_DOUT_FORMAT_CFG,
+ TI_ADS7142_DOUT_FORMAT_CFG_12BCHDV);
+}
+
+static int ti_ads7142_hth_set(struct iio_dev *indio_dev, int channel,
+ int threshold)
+{
+ struct i2c_client *client = to_i2c_client(indio_dev->dev.parent);
+ int ret;
+
+ if (threshold < 0 || threshold > TI_ADS7142_THRESHOLD_MSK)
+ return -EINVAL;
+
+ ret = ti_ads7142_reg_write(client,
+ TI_ADS7142_DWC_HTH_CH0_LSB + channel * 4,
+ threshold & 0xFF);
+ if (ret)
+ return ret;
+
+ ret = ti_ads7142_reg_write(client,
+ TI_ADS7142_DWC_HTH_CH0_MSB + channel * 4,
+ (threshold >> 8) & 0xF);
+ return ret;
+}
+
+static int ti_ads7142_lth_set(struct iio_dev *indio_dev, int channel,
+ int threshold)
+{
+ struct i2c_client *client = to_i2c_client(indio_dev->dev.parent);
+ int ret;
+
+ if (threshold < 0 || threshold > TI_ADS7142_THRESHOLD_MSK)
+ return -EINVAL;
+
+ ret = ti_ads7142_reg_write(client,
+ TI_ADS7142_DWC_LTH_CH0_LSB + channel * 4,
+ threshold & 0xFF);
+ if (ret)
+ return ret;
+
+ ret = ti_ads7142_reg_write(client,
+ TI_ADS7142_DWC_LTH_CH0_MSB + channel * 4,
+ (threshold >> 8) & 0xF);
+ return ret;
+}
+
+static int ti_ads7142_hys_set(struct iio_dev *indio_dev, int channel,
+ int hysteresis)
+{
+ struct i2c_client *client = to_i2c_client(indio_dev->dev.parent);
+ int ret;
+
+ if (hysteresis < 0 || hysteresis > TI_ADS7142_HYSTERESIS_MSK)
+ return -EINVAL;
+
+ ret = ti_ads7142_reg_write(client, TI_ADS7142_DWC_HYS_CH0 + channel,
+ hysteresis & TI_ADS7142_HYSTERESIS_MSK);
+ return ret;
+}
+
+static int ti_ads7142_collect_channel_data(struct iio_dev *indio_dev,
+ int *channel_collected)
+{
+ struct i2c_client *client = to_i2c_client(indio_dev->dev.parent);
+ struct ti_ads7142_channel *channel;
+ u16 data_buffer;
+ u8 data_buffer_status;
+ int data_valid;
+ int channel_address;
+ int value;
+ int ret;
+
+ ret = ti_ads7142_reg_read(client, TI_ADS7142_DATA_BUFFER_STATUS,
+ &data_buffer_status);
+ if (ret)
+ return ret;
+
+ data_buffer_status &= 0x1F;
+
+ do {
+ ret = ti_ads7142_data_buffer_read(client, sizeof(data_buffer),
+ &data_buffer);
+ if (ret)
+ break;
+ data_buffer = be16_to_cpu(data_buffer);
+ data_valid = data_buffer & 1;
+ if (data_valid) {
+ channel_address = (data_buffer >> 1) & 0x7;
+ value = data_buffer >> 4;
+ ret = ti_ads7142_address2channel(indio_dev,
+ channel_address,
+ &channel);
+ if (!ret) {
+ channel->data.status = data_valid;
+ channel->data.value = value;
+ *channel_collected |= 1 << channel_address;
+ }
+ }
+ } while (--data_buffer_status);
+
+ return ret;
+}
+
+static int ti_ads7142_do_work(struct iio_dev *indio_dev)
+{
+ struct ti_ads7142_priv *priv = iio_priv(indio_dev);
+ struct i2c_client *client = to_i2c_client(indio_dev->dev.parent);
+ int i;
+ int alert_ch = 0;
+ int ret;
+
+ if (!priv->config.monitoring_mode)
+ return 0;
+
+ mutex_lock(&priv->lock);
+ priv->monitor_pending = false;
+
+ ret = ti_ads7142_sequence_abort(indio_dev);
+ if (ret)
+ goto final;
+
+ ret = ti_ads7142_osc_set(indio_dev);
+ if (ret)
+ goto final;
+
+ ret = ti_ads7142_input_cfg_set(indio_dev);
+ if (ret)
+ goto final;
+
+ ret = ti_ads7142_dout_format_set(indio_dev);
+ if (ret)
+ goto final;
+
+ ret = ti_ads7142_reg_write(client, TI_ADS7142_DATA_BUFFER_OPMODE,
+ TI_ADS7142_DATA_BUFFER_OPMODE_PRE_ALERT);
+ if (ret)
+ goto final;
+
+ ret = ti_ads7142_reg_write(client, TI_ADS7142_OPMODE_SEL,
+ TI_ADS7142_OPMODE_SEL_MONITORING);
+ if (ret)
+ goto final;
+
+ ret = ti_ads7142_reg_write(client, TI_ADS7142_AUTO_SEQ_CHEN,
+ TI_ADS7142_AUTO_SEQ_CHEN_CH0
+ | TI_ADS7142_AUTO_SEQ_CHEN_CH1);
+ if (ret)
+ goto final;
+
+ ret = ti_ads7142_reg_write(client, TI_ADS7142_PRE_ALT_EVT_CNT, 0);
+ if (ret)
+ goto final;
+
+ ret = ti_ads7142_reg_write(client, TI_ADS7142_ALT_LOW_FLAGS,
+ TI_ADS7142_ALT_LOW_FLAGS_CH0
+ | TI_ADS7142_ALT_LOW_FLAGS_CH1);
+ if (ret)
+ goto final;
+
+ ret = ti_ads7142_reg_write(client, TI_ADS7142_ALT_HIGH_FLAGS,
+ TI_ADS7142_ALT_HIGH_FLAGS_CH0
+ | TI_ADS7142_ALT_HIGH_FLAGS_CH1);
+ if (ret)
+ goto final;
+
+ for (i = 0; i < priv->channel_count; i++) {
+ ret = ti_ads7142_hth_set(indio_dev, priv->channels[i].channel,
+ priv->channels[i].config.high_threshold);
+ if (ret)
+ goto final;
+
+ ret = ti_ads7142_lth_set(indio_dev, priv->channels[i].channel,
+ priv->channels[i].config.low_threshold);
+ if (ret)
+ goto final;
+
+ ret = ti_ads7142_hys_set(indio_dev, priv->channels[i].channel,
+ priv->channels[i].config.hysteresis);
+ if (ret)
+ goto final;
+
+ if (priv->channels[i].config.alert_low ||
+ priv->channels[i].config.alert_high) {
+ alert_ch |= 1 << priv->channels[i].channel;
+ }
+ }
+
+ ret = ti_ads7142_reg_write(client, TI_ADS7142_ALERT_DWC_EN,
+ alert_ch ? TI_ADS7142_ALERT_DWC_EN_BLOCK_EN : 0);
+ if (ret)
+ goto final;
+
+ ret = ti_ads7142_reg_write(client, TI_ADS7142_ALERT_CHEN,
+ alert_ch);
+ if (ret)
+ goto final;
+
+ if (alert_ch) {
+ ret = ti_ads7142_sequence_start(indio_dev);
+ priv->monitor_pending = !ret;
+ }
+final:
+ mutex_unlock(&priv->lock);
+ return ret;
+}
+
+static int ti_ads7142_read_channel_manual(struct iio_dev *indio_dev,
+ int address, int *val)
+{
+ struct ti_ads7142_priv *priv = iio_priv(indio_dev);
+ struct i2c_client *client = to_i2c_client(indio_dev->dev.parent);
+ u16 data_buffer;
+ int ret;
+
+ if (address < 0 || address > 1)
+ return -EINVAL;
+
+ mutex_lock(&priv->lock);
+ ret = ti_ads7142_sequence_abort(indio_dev);
+ if (ret)
+ goto final;
+
+ ret = ti_ads7142_osc_set(indio_dev);
+ if (ret)
+ goto final;
+
+ ret = ti_ads7142_input_cfg_set(indio_dev);
+ if (ret)
+ goto final;
+
+ ret = ti_ads7142_dout_format_set(indio_dev);
+ if (ret)
+ goto final;
+
+ ret = ti_ads7142_reg_write(client, TI_ADS7142_OPMODE_SEL,
+ TI_ADS7142_OPMODE_SEL_MANUALSEQ);
+ if (ret)
+ goto final;
+
+ ret = ti_ads7142_reg_write(client, TI_ADS7142_AUTO_SEQ_CHEN,
+ 1 << address);
+ if (ret)
+ goto final;
+
+ ret = ti_ads7142_sequence_start(indio_dev);
+ if (ret)
+ goto final;
+
+ ret = ti_ads7142_data_buffer_read(client, sizeof(data_buffer),
+ &data_buffer);
+ if (ret)
+ goto abort;
+
+ *val = (be16_to_cpu(data_buffer) >> 4);
+
+abort:
+ ret = ti_ads7142_sequence_abort(indio_dev);
+final:
+ mutex_unlock(&priv->lock);
+ return ret;
+}
+
+static int ti_ads7142_read_channel_monitor(struct iio_dev *indio_dev,
+ int address, int *val)
+{
+ struct ti_ads7142_priv *priv = iio_priv(indio_dev);
+ struct ti_ads7142_channel *channel;
+ int ret;
+
+ if (address < 0 || address > 1)
+ return -EINVAL;
+
+ ret = ti_ads7142_address2channel(indio_dev, address, &channel);
+ if (ret)
+ return ret;
+
+ mutex_lock(&priv->lock);
+ if (!channel->data.status) {
+ ret = -EAGAIN;
+ } else {
+ *val = channel->data.value;
+ channel->data.status = 0;
+ ret = 0;
+ }
+ mutex_unlock(&priv->lock);
+ return ret;
+}
+
+static int ti_ads7142_read_channel(struct iio_dev *indio_dev,
+ int address, int *val)
+{
+ struct ti_ads7142_priv *priv = iio_priv(indio_dev);
+
+ if (priv->config.monitoring_mode)
+ return ti_ads7142_read_channel_monitor(indio_dev, address,
+ val);
+ return ti_ads7142_read_channel_manual(indio_dev, address, val);
+}
+
+static irqreturn_t ti_ads7142_ist(int irq, void *dev_id)
+{
+ struct iio_dev *indio_dev = dev_id;
+ struct i2c_client *client = to_i2c_client(indio_dev->dev.parent);
+ struct ti_ads7142_priv *priv = iio_priv(indio_dev);
+ struct ti_ads7142_channel *channel;
+ u8 low_flags;
+ u8 high_flags;
+ u8 seq_st;
+ int i;
+ int ret;
+ int channel_collected;
+ s64 timestamp = iio_get_time_ns(indio_dev);
+
+ mutex_lock(&priv->lock);
+ if (!priv->config.monitoring_mode || !priv->monitor_pending) {
+ mutex_unlock(&priv->lock);
+ return IRQ_NONE;
+ }
+
+ ret = ti_ads7142_reg_read(client, TI_ADS7142_SEQUENCE_STATUS, &seq_st);
+ if (ret) {
+ dev_err(indio_dev->dev.parent,
+ "%s: SEQUENCE_STATUS reg read error(%i)",
+ __func__, ret);
+ goto final;
+ }
+
+ if ((seq_st & TI_ADS7142_SEQUENCE_STATUS_SEQ_ERR_ST_MSK)
+ != TI_ADS7142_SEQUENCE_STATUS_SEQ_ENABLED) {
+ dev_err(indio_dev->dev.parent,
+ "%s: SEQUENCE_STATUS error(%i)",
+ __func__, seq_st);
+ goto final;
+ }
+
+ ret = ti_ads7142_reg_read(client, TI_ADS7142_ALT_LOW_FLAGS,
+ &low_flags);
+ if (ret) {
+ dev_err(indio_dev->dev.parent,
+ "%s: ALT_LOW_FLAGS reg read error(%i)",
+ __func__, ret);
+ goto final;
+ }
+
+ ret = ti_ads7142_reg_read(client, TI_ADS7142_ALT_HIGH_FLAGS,
+ &high_flags);
+ if (ret) {
+ dev_err(indio_dev->dev.parent,
+ "%s: ALT_HIGH_FLAGS reg read error(%i)",
+ __func__, ret);
+ goto final;
+ }
+
+ channel_collected = 0;
+ ret = ti_ads7142_collect_channel_data(indio_dev, &channel_collected);
+ if (ret)
+ goto final;
+
+ if (!channel_collected)
+ goto final;
+
+ for (i = 0; i < priv->channel_count; i++) {
+ channel = &priv->channels[i];
+ if (!(channel_collected & (1 << channel->channel)))
+ continue;
+ if (channel->config.alert_low &&
+ (low_flags & (1 << channel->channel))) {
+ iio_push_event(indio_dev,
+ IIO_UNMOD_EVENT_CODE(IIO_VOLTAGE,
+ channel->channel,
+ IIO_EV_TYPE_THRESH,
+ IIO_EV_DIR_FALLING),
+ timestamp);
+ }
+
+ if (channel->config.alert_high &&
+ (high_flags & (1 << channel->channel))) {
+ iio_push_event(indio_dev,
+ IIO_UNMOD_EVENT_CODE(IIO_VOLTAGE,
+ channel->channel,
+ IIO_EV_TYPE_THRESH,
+ IIO_EV_DIR_RISING),
+ timestamp);
+ }
+ }
+
+final:
+ mutex_unlock(&priv->lock);
+
+ ret = ti_ads7142_do_work(indio_dev);
+ if (ret) {
+ dev_err(indio_dev->dev.parent,
+ "%s: start monitoring error(%i)",
+ __func__, ret);
+ return IRQ_NONE;
+ }
+ return IRQ_HANDLED;
+}
+
+static int ti_ads7142_read_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ int *val, int *val2, long info)
+{
+ struct ti_ads7142_priv *priv = iio_priv(indio_dev);
+ int ret;
+
+ switch (info) {
+ case IIO_CHAN_INFO_RAW:
+ ret = ti_ads7142_read_channel(indio_dev, chan->address, val);
+ if (!ret)
+ ret = IIO_VAL_INT;
+ break;
+ case IIO_CHAN_INFO_SAMP_FREQ:
+ *val = priv->config.n_clk;
+ ret = IIO_VAL_INT;
+ break;
+ case IIO_CHAN_INFO_SCALE:
+ if (IS_ERR(priv->vref)) {
+ ret = -EINVAL;
+ } else {
+ *val = regulator_get_voltage(priv->vref) / 1000;
+ *val2 = chan->scan_type.realbits;
+ ret = IIO_VAL_FRACTIONAL_LOG2;
+ }
+ break;
+ default:
+ ret = -EINVAL;
+ }
+ return ret;
+}
+
+static int ti_ads7142_write_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ int val, int val2, long mask)
+{
+ struct ti_ads7142_priv *priv = iio_priv(indio_dev);
+ int ret;
+
+ switch (mask) {
+ case IIO_CHAN_INFO_SAMP_FREQ:
+ priv->config.n_clk = val;
+ if (priv->config.monitoring_mode)
+ ret = ti_ads7142_do_work(indio_dev);
+ else
+ ret = 0;
+ break;
+ default:
+ ret = -EINVAL;
+ break;
+ }
+
+ return ret;
+}
+
+static int ti_ads7142_read_event_value(struct iio_dev *indio_dev,
+ const struct iio_chan_spec *chan,
+ enum iio_event_type type,
+ enum iio_event_direction dir,
+ enum iio_event_info info,
+ int *val, int *val2)
+{
+ struct ti_ads7142_priv *priv = iio_priv(indio_dev);
+ struct ti_ads7142_channel *channel;
+ int ret;
+
+ if (!priv->config.monitoring_mode)
+ return -EINVAL;
+
+ ret = ti_ads7142_address2channel(indio_dev, chan->address,
+ &channel);
+ if (ret)
+ return ret;
+
+ switch (info) {
+ case IIO_EV_INFO_VALUE:
+ if (dir == IIO_EV_DIR_RISING)
+ *val = channel->config.high_threshold;
+ else
+ *val = channel->config.low_threshold;
+ ret = IIO_VAL_INT;
+ break;
+ case IIO_EV_INFO_HYSTERESIS:
+ *val = channel->config.hysteresis;
+ ret = IIO_VAL_INT;
+ break;
+ default:
+ ret = -EINVAL;
+ break;
+ }
+ return ret;
+}
+
+static int ti_ads7142_write_event_value(struct iio_dev *indio_dev,
+ const struct iio_chan_spec *chan,
+ enum iio_event_type type,
+ enum iio_event_direction dir,
+ enum iio_event_info info,
+ int val, int val2)
+{
+ struct ti_ads7142_priv *priv = iio_priv(indio_dev);
+ struct ti_ads7142_channel *channel;
+ bool have_to_do = false;
+ int ret;
+
+ if (!priv->config.monitoring_mode)
+ return -EINVAL;
+
+ ret = ti_ads7142_address2channel(indio_dev, chan->address,
+ &channel);
+ if (ret)
+ return ret;
+
+ mutex_lock(&priv->lock);
+ switch (info) {
+ case IIO_EV_INFO_VALUE:
+ if (val < 0 || val > TI_ADS7142_THRESHOLD_MSK) {
+ ret = -EINVAL;
+ } else {
+ if (dir == IIO_EV_DIR_RISING) {
+ if (val != channel->config.high_threshold) {
+ channel->config.high_threshold = val;
+ have_to_do = true;
+ }
+ } else {
+ if (val != channel->config.low_threshold) {
+ channel->config.low_threshold = val;
+ have_to_do = true;
+ }
+ }
+ }
+ break;
+ case IIO_EV_INFO_HYSTERESIS:
+ if (val < 0 || val > TI_ADS7142_HYSTERESIS_MSK) {
+ ret = -EINVAL;
+ } else {
+ if (val != channel->config.hysteresis) {
+ channel->config.hysteresis = val;
+ have_to_do = true;
+ }
+ }
+ break;
+ default:
+ ret = -EINVAL;
+ break;
+ }
+ mutex_unlock(&priv->lock);
+ if (!ret && have_to_do)
+ ret = ti_ads7142_do_work(indio_dev);
+ return ret;
+}
+
+static int ti_ads7142_read_event_config(struct iio_dev *indio_dev,
+ const struct iio_chan_spec *chan,
+ enum iio_event_type type,
+ enum iio_event_direction dir)
+{
+ struct ti_ads7142_priv *priv = iio_priv(indio_dev);
+ struct ti_ads7142_channel *channel;
+ int ret;
+
+ if (!priv->config.monitoring_mode)
+ return -EINVAL;
+
+ if (type != IIO_EV_TYPE_THRESH)
+ return -EINVAL;
+
+ ret = ti_ads7142_address2channel(indio_dev, chan->address,
+ &channel);
+ if (ret)
+ return ret;
+
+ if (dir == IIO_EV_DIR_RISING)
+ ret = channel->config.alert_high ? 1 : 0;
+ else
+ ret = channel->config.alert_low ? 1 : 0;
+
+ return ret;
+}
+
+static int ti_ads7142_write_event_config(struct iio_dev *indio_dev,
+ const struct iio_chan_spec *chan,
+ enum iio_event_type type,
+ enum iio_event_direction dir,
+ int state)
+{
+ struct ti_ads7142_priv *priv = iio_priv(indio_dev);
+ struct ti_ads7142_channel *channel;
+ bool have_to_do = false;
+ int ret;
+
+ if (!priv->config.monitoring_mode)
+ return -EINVAL;
+
+ if (type != IIO_EV_TYPE_THRESH)
+ return -EINVAL;
+
+ ret = ti_ads7142_address2channel(indio_dev, chan->address,
+ &channel);
+ if (ret)
+ return ret;
+
+ mutex_lock(&priv->lock);
+ if (dir == IIO_EV_DIR_RISING) {
+ if (channel->config.alert_high != state) {
+ channel->config.alert_high = state;
+ have_to_do = true;
+ }
+ } else {
+ if (channel->config.alert_low != state) {
+ channel->config.alert_low = state;
+ have_to_do = true;
+ }
+ }
+ mutex_unlock(&priv->lock);
+
+ if (have_to_do)
+ ret = ti_ads7142_do_work(indio_dev);
+
+ return ret;
+}
+
+static const struct iio_info ti_ads7142_iio_info = {
+ .read_raw = ti_ads7142_read_raw,
+ .write_raw = ti_ads7142_write_raw,
+ .read_event_value = ti_ads7142_read_event_value,
+ .write_event_value = ti_ads7142_write_event_value,
+ .read_event_config = ti_ads7142_read_event_config,
+ .write_event_config = ti_ads7142_write_event_config,
+};
+
+static int ti_ads7142_parse_channel_config_of(struct device *dev,
+ struct iio_dev *indio_dev)
+{
+ struct ti_ads7142_priv *priv = iio_priv(indio_dev);
+ struct device_node *channel_node;
+ struct iio_chan_spec *iio_channels;
+ struct iio_chan_spec *iio_channel;
+ struct ti_ads7142_channel *ads_channel;
+ int channel_index = 0;
+ int ret;
+
+ priv->channel_count = of_get_available_child_count(dev->of_node);
+ if (!priv->channel_count) {
+ dev_err(dev, "dt: there is no channel definition");
+ return -ENODEV;
+ }
+
+ priv->channels = devm_kcalloc(dev, priv->channel_count,
+ sizeof(*priv->channels),
+ GFP_KERNEL);
+ if (!priv->channels)
+ return -ENOMEM;
+
+ indio_dev->num_channels = priv->channel_count;
+ iio_channels = devm_kcalloc(dev, priv->channel_count,
+ sizeof(*iio_channels),
+ GFP_KERNEL);
+ if (!iio_channels)
+ return -ENOMEM;
+
+ indio_dev->channels = iio_channels;
+
+ for_each_available_child_of_node(dev->of_node, channel_node) {
+ ads_channel = &priv->channels[channel_index];
+
+ ret = of_property_read_u32(channel_node, "reg",
+ &ads_channel->channel);
+ if (ret)
+ goto err;
+
+ iio_channel = &iio_channels[channel_index];
+ iio_channel->type = IIO_VOLTAGE;
+ iio_channel->indexed = 1;
+ iio_channel->info_mask_separate = BIT(IIO_CHAN_INFO_RAW)
+ | BIT(IIO_CHAN_INFO_SAMP_FREQ);
+ if (!IS_ERR(priv->vref))
+ iio_channel->info_mask_separate |= BIT(IIO_CHAN_INFO_SCALE);
+ iio_channel->scan_type.sign = 'u';
+ iio_channel->scan_type.realbits = 12;
+ iio_channel->scan_type.storagebits = 16;
+ iio_channel->scan_type.shift = 0;
+ iio_channel->scan_type.endianness = IIO_CPU;
+ iio_channel->address = ads_channel->channel;
+ iio_channel->scan_index = ads_channel->channel;
+ iio_channel->channel = ads_channel->channel;
+ if (priv->config.monitoring_mode) {
+ iio_channel->event_spec = ti_ads7142_events;
+ iio_channel->num_event_specs = ARRAY_SIZE(ti_ads7142_events);
+ }
+
+ ads_channel->config.high_threshold = TI_ADS7142_THRESHOLD_MSK;
+ ret = of_property_read_u32(channel_node, "ti,threshold-rising",
+ &ads_channel->config.high_threshold);
+ ads_channel->config.alert_high = !ret;
+ ret = of_property_read_u32(channel_node, "ti,threshold-falling",
+ &ads_channel->config.low_threshold);
+ ads_channel->config.alert_low = !ret;
+ ret = of_property_read_u32(channel_node, "ti,hysteresis",
+ &ads_channel->config.hysteresis);
+ channel_index++;
+ }
+
+ return 0;
+err:
+ of_node_put(channel_node);
+ return ret;
+}
+
+static int ti_ads7142_parse_config_of(struct device *dev,
+ struct iio_dev *indio_dev)
+{
+ struct ti_ads7142_priv *priv = iio_priv(indio_dev);
+
+ priv->config.osc_sel = of_property_read_bool(dev->of_node,
+ "ti,osc-sel");
+ of_property_read_u32(dev->of_node, "ti,n-clk", &priv->config.n_clk);
+ priv->config.monitoring_mode = of_property_read_bool(dev->of_node,
+ "ti,monitoring-mode");
+
+ return ti_ads7142_parse_channel_config_of(dev, indio_dev);
+}
+
+static int ti_ads7142_probe(struct i2c_client *client,
+ const struct i2c_device_id *id)
+{
+ struct iio_dev *indio_dev;
+ struct ti_ads7142_priv *priv;
+ int ret;
+
+ ret = ti_ads7142_soft_reset(client);
+ if (ret)
+ return ret;
+
+ indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*priv));
+ if (!indio_dev)
+ return -ENOMEM;
+
+ priv = iio_priv(indio_dev);
+ i2c_set_clientdata(client, indio_dev);
+
+ indio_dev->dev.parent = &client->dev;
+ indio_dev->dev.of_node = client->dev.of_node;
+ indio_dev->name = TI_ADS7142_NAME;
+ indio_dev->modes = INDIO_DIRECT_MODE;
+ indio_dev->info = &ti_ads7142_iio_info;
+
+ mutex_init(&priv->lock);
+
+ priv->vref = devm_regulator_get_optional(&client->dev, "vref");
+ if (!IS_ERR(priv->vref)) {
+ ret = regulator_enable(priv->vref);
+ if (ret)
+ goto err;
+ }
+
+ priv->power = devm_regulator_get_optional(&client->dev, "power");
+ if (!IS_ERR(priv->power)) {
+ ret = regulator_enable(priv->power);
+ if (ret)
+ goto err_regulator;
+ }
+
+ ret = ti_ads7142_parse_config_of(&client->dev, indio_dev);
+ if (ret)
+ goto err_regulator;
+
+ if (!client->irq && priv->config.monitoring_mode) {
+ ret = -EINVAL;
+ dev_err(&client->dev, "Interrupt not specified\n");
+ goto err_regulator;
+ }
+ if (client->irq && priv->config.monitoring_mode) {
+ ret = devm_request_threaded_irq(&client->dev, client->irq,
+ NULL, ti_ads7142_ist,
+ IRQF_ONESHOT | IRQF_SHARED,
+ dev_name(&client->dev),
+ indio_dev);
+ if (ret) {
+ dev_err(&client->dev, "Unable to request IRQ %i",
+ client->irq);
+ goto err_regulator;
+ }
+ }
+
+ ret = iio_device_register(indio_dev);
+ if (ret) {
+ dev_err(&client->dev, "Failed to register iio device");
+ goto err_regulator;
+ }
+
+ ret = ti_ads7142_do_work(indio_dev);
+ if (!ret) {
+ dev_info(&client->dev, "%s is a %s device at address 0x%X",
+ dev_name(&indio_dev->dev), indio_dev->name,
+ client->addr);
+ return ret;
+ }
+
+ iio_device_unregister(indio_dev);
+
+err_regulator:
+ if (!IS_ERR(priv->vref))
+ regulator_disable(priv->vref);
+ if (!IS_ERR(priv->power))
+ regulator_disable(priv->power);
+err:
+ mutex_destroy(&priv->lock);
+
+ return ret;
+}
+
+static int ti_ads7142_remove(struct i2c_client *client)
+{
+ struct iio_dev *indio_dev = i2c_get_clientdata(client);
+ struct ti_ads7142_priv *priv = iio_priv(indio_dev);
+
+ if (!IS_ERR(priv->vref))
+ regulator_disable(priv->vref);
+ if (!IS_ERR(priv->power))
+ regulator_disable(priv->power);
+ mutex_destroy(&priv->lock);
+ iio_device_unregister(indio_dev);
+
+ return 0;
+}
+
+static int __maybe_unused ti_ads7142_suspend(struct device *dev)
+{
+ struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
+ struct ti_ads7142_priv *priv = iio_priv(indio_dev);
+
+ /**
+ * Keep all regulators on when the device in autonomous
+ * monitoring mode.
+ * The device can wake up the system with ALERT pin
+ **/
+ if (priv->config.monitoring_mode && priv->monitor_pending)
+ return 0;
+
+ if (!IS_ERR(priv->vref))
+ regulator_disable(priv->vref);
+ if (!IS_ERR(priv->power))
+ regulator_disable(priv->power);
+
+ return 0;
+}
+
+static int __maybe_unused ti_ads7142_resume(struct device *dev)
+{
+ struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
+ struct ti_ads7142_priv *priv = iio_priv(indio_dev);
+ int ret;
+
+ /**
+ * Nothing to do when the device in autonomous monitoring mode.
+ **/
+ if (priv->config.monitoring_mode && priv->monitor_pending)
+ return 0;
+
+ if (!IS_ERR(priv->vref)) {
+ ret = regulator_enable(priv->vref);
+ if (ret)
+ return ret;
+ }
+ if (!IS_ERR(priv->power)) {
+ ret = regulator_enable(priv->power);
+ if (ret)
+ return ret;
+ }
+ return 0;
+}
+
+static SIMPLE_DEV_PM_OPS(ti_ads7142_pm_ops, ti_ads7142_suspend,
+ ti_ads7142_resume);
+
+static const struct i2c_device_id ti_ads7142_id[] = {
+ { TI_ADS7142_NAME, 0 },
+ { }
+};
+MODULE_DEVICE_TABLE(i2c, ti_ads7142_id);
+
+static const struct of_device_id ti_ads7142_of_match[] = {
+ { .compatible = "ti,ads7142" },
+ {}
+};
+MODULE_DEVICE_TABLE(of, ti_ads7142_of_match);
+
+static struct i2c_driver ti_ads7142_driver = {
+ .driver = {
+ .name = TI_ADS7142_NAME,
+ .of_match_table = ti_ads7142_of_match,
+ .pm = &ti_ads7142_pm_ops,
+ },
+ .probe = ti_ads7142_probe,
+ .remove = ti_ads7142_remove,
+ .id_table = ti_ads7142_id,
+};
+
+module_i2c_driver(ti_ads7142_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Jozsef Horvath <info@ministro.hu>");
+MODULE_DESCRIPTION("Texas Instruments TI_ADS7142 ADC driver");
--
2.17.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 2/2] dt-bindings: iio: adc: devicetree bindings for texas instruments ads7142 iio driver
2021-05-01 18:24 [PATCH 1/2] iio: adc: driver for texas instruments ads7142 Jozsef Horvath
@ 2021-05-01 18:25 ` Jozsef Horvath
2021-05-02 17:22 ` Jonathan Cameron
2021-05-02 17:14 ` [PATCH 1/2] iio: adc: driver for texas instruments ads7142 Jonathan Cameron
1 sibling, 1 reply; 10+ messages in thread
From: Jozsef Horvath @ 2021-05-01 18:25 UTC (permalink / raw)
To: Jozsef Horvath, Jonathan Cameron, Lars-Peter Clausen,
Peter Meerwald-Stadler, Rob Herring, Liam Girdwood, Mark Brown,
Alexandru Ardelean, Heiko Stuebner, Andy Shevchenko, Alex Dewar,
Gene Chen, Saravanan Sekar, Lee Jones, linux-iio, devicetree,
linux-kernel
This is a device tree schema for iio driver for
Texas Instruments ADS7142 dual-channel, programmable sensor monitor.
Datasheet: https://www.ti.com/lit/ds/symlink/ads7142.pdf
Signed-off-by: Jozsef Horvath <info@ministro.hu>
---
---
.../bindings/iio/adc/ti,ads7142.yaml | 198 ++++++++++++++++++
1 file changed, 198 insertions(+)
create mode 100644 Documentation/devicetree/bindings/iio/adc/ti,ads7142.yaml
diff --git a/Documentation/devicetree/bindings/iio/adc/ti,ads7142.yaml b/Documentation/devicetree/bindings/iio/adc/ti,ads7142.yaml
new file mode 100644
index 000000000000..b4e752160156
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/ti,ads7142.yaml
@@ -0,0 +1,198 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: "http://devicetree.org/schemas/iio/adc/ti,ads7142.yaml#"
+$schema: "http://devicetree.org/meta-schemas/core.yaml#"
+
+title: Texas Instruments ADS7142 adc driver device tree bindings
+
+maintainers:
+ - József Horváth <info@ministro.hu>
+
+description: |
+ This document is for describing the required device tree parameters
+ for ads7142 adc driver.
+ The required parameters for proper operation are described below.
+
+ Datasheet: https://www.ti.com/lit/ds/symlink/ads7142.pdf
+
+ Operation modes supportedby the driver:
+ When the 'ti,monitoring-mode' property is not present
+ in the devicetree node definition, the driver initiates a single
+ conversion in the device for each read request
+ (/sys/bus/iio/devices/iio:deviceX/in_voltageY_raw).
+ This is a one-shot conversion, and it is called
+ "Manual Mode" in the datasheet.
+
+ When the 'ti,monitoring-mode' property is present
+ in the devicetree node definition, the driver configures
+ the device's digital window comparator and sets the device's
+ data buffer operation mode to pre alert data mode.
+ The driver reads the conversion result when the BUSY/RDY interrupt
+ fires, and keeps the value until the next BUSY/RDY interrupt
+ or the first read request
+ (/sys/bus/iio/devices/iio:deviceX/in_voltageY_raw).
+ The digital window comparator and hysteresis parameters
+ can be controlled by:
+ - the devicetree definition of channel node
+ - iio sysfs interfaces
+ This is event driven conversion, and is called
+ "Autonomous Mode with Pre Alert Data" in the datasheet.
+ This mode can be used to wake up the system with the ALERT pin,
+ in case when the monitored voltage level is out of the configured range.
+
+properties:
+ compatible:
+ const: ti,ads7142
+
+ reg:
+ maxItems: 1
+
+ interrupts:
+ description: |
+ The BUSY/PDY pin is used as interrupt line in autonomous monitoring mode.
+ maxItems: 1
+
+ vref-supply:
+ description: Regulator for the reference voltage
+
+ power-supply: true
+
+ "#address-cells":
+ const: 1
+
+ "#size-cells":
+ const: 0
+
+ "#io-channel-cells":
+ const: 1
+
+ ti,osc-sel:
+ description: |
+ If present, the driver selects the high speed oscillator.
+ See chapter 7.3.5 Oscillator and Timing Control in datasheet.
+ type: boolean
+
+ ti,n-clk:
+ description: |
+ nCLK is number of clocks in one conversion cycle.
+ See chapter 7.3.5 Oscillator and Timing Control in datasheet.
+ $ref: /schemas/types.yaml#/definitions/uint32
+ maximum: 255
+ minimum: 0
+
+ ti,monitoring-mode:
+ description: |
+ If present, the driver selects the autonomous monitoring mode with pre alert data.
+ See chapter 7.4 Device Functional Modes in datasheet.
+ type: boolean
+
+patternProperties:
+ "^channel@[0-1]$":
+ $ref: "adc.yaml"
+ type: object
+ description: |
+ Represents the external channels which are connected to the ADC.
+ properties:
+ reg:
+ description: |
+ The channel number.
+ items:
+ minimum: 0
+ maximum: 1
+ "ti,threshold-falling":
+ description: The low threshold for channel
+ $ref: /schemas/types.yaml#/definitions/uint32
+ maximum: 4095
+ minimum: 0
+ "ti,threshold-rising":
+ description: The high threshold for channel
+ $ref: /schemas/types.yaml#/definitions/uint32
+ maximum: 4095
+ minimum: 0
+ "ti,hysteresis":
+ description: The hysteresis for both comparators for channel
+ $ref: /schemas/types.yaml#/definitions/uint32
+ maximum: 63
+ minimum: 0
+
+ required:
+ - reg
+
+ additionalProperties: false
+
+allOf:
+ - if:
+ required:
+ - ti,monitoring-mode
+ then:
+ required:
+ - interrupts
+
+required:
+ - compatible
+ - "#io-channel-cells"
+
+additionalProperties: false
+
+examples:
+ - |
+ i2c {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ adc@18 {
+ compatible = "ti,ads7142";
+ reg = <0x18>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+ #io-channel-cells = <1>;
+
+ vref-supply = <&vdd_3v3_reg>;
+ power-supply = <&vdd_1v8_reg>;
+
+ channel@0 {
+ reg = <0>;
+ };
+
+ channel@1 {
+ reg = <1>;
+ };
+ };
+ };
+ - |
+ i2c {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ adc@1f {
+ compatible = "ti,ads7142";
+ reg = <0x1f>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ #io-channel-cells = <1>;
+
+ vref-supply = <&vdd_3v3_reg>;
+ power-supply = <&vdd_1v8_reg>;
+
+ interrupt-parent = <&gpio>;
+ interrupts = <7 2>;
+
+ ti,monitoring-mode;
+
+ channel@0 {
+ reg = <0>;
+ ti,threshold-falling = <1000>;
+ ti,threshold-rising = <2000>;
+ ti,hysteresis = <20>;
+ };
+
+ channel@1 {
+ reg = <1>;
+ ti,threshold-falling = <100>;
+ ti,threshold-rising = <2500>;
+ ti,hysteresis = <0>;
+ };
+ };
+ };
+...
+
--
2.17.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] iio: adc: driver for texas instruments ads7142
2021-05-01 18:24 [PATCH 1/2] iio: adc: driver for texas instruments ads7142 Jozsef Horvath
2021-05-01 18:25 ` [PATCH 2/2] dt-bindings: iio: adc: devicetree bindings for texas instruments ads7142 iio driver Jozsef Horvath
@ 2021-05-02 17:14 ` Jonathan Cameron
2021-05-02 20:48 ` József Horváth
1 sibling, 1 reply; 10+ messages in thread
From: Jonathan Cameron @ 2021-05-02 17:14 UTC (permalink / raw)
To: Jozsef Horvath
Cc: Lars-Peter Clausen, Peter Meerwald-Stadler, Rob Herring,
Liam Girdwood, Mark Brown, Alexandru Ardelean, Heiko Stuebner,
Andy Shevchenko, Alex Dewar, Gene Chen, Saravanan Sekar,
Lee Jones, linux-iio, devicetree, linux-kernel
On Sat, 1 May 2021 18:24:28 +0000
Jozsef Horvath <info@ministro.hu> wrote:
> This is an iio driver for
> Texas Instruments ADS7142 dual-channel, programmable sensor monitor.
>
> Operation modes supportedby the driver:
> When the 'ti,monitoring-mode' property is not present
> in the devicetree node definition, the driver initiates a single
> conversion in the device for each read request
> (/sys/bus/iio/devices/iio:deviceX/in_voltageY_raw).
> This is a one-shot conversion, and it is called
> "Manual Mode" in the datasheet.
>
> When the 'ti,monitoring-mode' property is present
> in the devicetree node definition, the driver configures
> the device's digital window comparator and sets the device's
> data buffer operation mode to pre alert data mode.
> The driver reads the conversion result when the BUSY/RDY interrupt
> fires, and keeps the value until the next BUSY/RDY interrupt
> or the first read request
> (/sys/bus/iio/devices/iio:deviceX/in_voltageY_raw).
Hi Jozsef.
Interesting device - somewhat like an impact sensor, but on a general
purpose ADC.
Hmm. This sounds rather unintuitive and also very much like a policy
decision rather than anything to do with the hardware. Hence it
should almost certainly be in control of userspace and no via
dt parameters.
The interrupt driven nature of the device implies that a polled interface
such as sysfs is not appropriate to support this mode.
Based on the description you have given here and a quick look
at the flow charts in the datasheet I would suggest.
1) Enable sysfs reads as manual mode only.
2) Implement the buffered part of an IIO driver. This is what we use
for data where autonomous clocking is going on.
3) Add triggers to represent the different autonomous modes. In some
sense all the modes present can be considered be a series of
'capture now' signals that are being generated by the hardware in
response to some event'.
So you'd have a pre_alert_tigger, post_alert_trigger
Stop_burst and start_burst are more interesting to handle because you
will need something to actually start/stop them. These could be done
via a sysfs attribute for the trigger, or more complex schemes exist
such as triggering them off another trigger... one or two of the SoC
ADCs do that sort of thing.
> The digital window comparator and hysteresis parameters
> can be controlled by:
> - the devicetree definition of channel node
> - iio sysfs interfaces
> This is event driven conversion, and is called
> "Autonomous Mode with Pre Alert Data" in the datasheet.
> This mode can be used to wake up the system with the ALERT pin,
> in case when the monitored voltage level is out of the configured range.
Whilst it's fine to only enable the modes you want, we should think about how
to ensure other modes can be supported.
>
> Datasheet: https://www.ti.com/lit/ds/symlink/ads7142.pdf
>
> Signed-off-by: Jozsef Horvath <info@ministro.hu>
> ---
Should only be one ---
comments inline.
> ---
> MAINTAINERS | 6 +
> drivers/iio/adc/Kconfig | 10 +
> drivers/iio/adc/Makefile | 1 +
> drivers/iio/adc/ti-ads7142.c | 1215 ++++++++++++++++++++++++++++++++++
> 4 files changed, 1232 insertions(+)
> create mode 100644 drivers/iio/adc/ti-ads7142.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 9450e052f1b1..954cbabde801 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -17694,6 +17694,12 @@ T: git git://github.com/czankel/xtensa-linux.git
> F: arch/xtensa/
> F: drivers/irqchip/irq-xtensa-*
>
> +TEXAS INSTRUMENTS ADS7142 ADC DRIVER
> +M: Jozsef Horvath <info@ministro.hu>
> +S: Maintained
> +F: Documentation/devicetree/bindings/iio/adc/ti,ads7142.yaml
> +F: drivers/iio/adc/ti-ads7142.c
> +
> TEXAS INSTRUMENTS ASoC DRIVERS
> M: Peter Ujfalusi <peter.ujfalusi@gmail.com>
> L: alsa-devel@alsa-project.org (moderated for non-subscribers)
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index e0667c4b3c08..69d672298719 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -1111,6 +1111,16 @@ config TI_ADS1015
> This driver can also be built as a module. If so, the module will be
> called ti-ads1015.
>
> +config TI_ADS7142
> + tristate "Texas Instruments ADS7142 ADC driver"
> + depends on I2C
> + help
> + This driver is for Texas Instruments ADS7142 Nanopower, Dual-Channel, Programmable Sensor Monitor.
Please keep to shorter lines in Kconfig files < 80 chars preferred.
> + Say 'Y' here if you wish to use it.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called ti-ads7142.
> +
> config TI_ADS7950
> tristate "Texas Instruments ADS7950 ADC driver"
> depends on SPI && GPIOLIB
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index 5fca90ada0ec..83d2a72ab758 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -99,6 +99,7 @@ obj-$(CONFIG_TI_ADC108S102) += ti-adc108s102.o
> obj-$(CONFIG_TI_ADC128S052) += ti-adc128s052.o
> obj-$(CONFIG_TI_ADC161S626) += ti-adc161s626.o
> obj-$(CONFIG_TI_ADS1015) += ti-ads1015.o
> +obj-$(CONFIG_TI_ADS7142) += ti-ads7142.o
> obj-$(CONFIG_TI_ADS7950) += ti-ads7950.o
> obj-$(CONFIG_TI_ADS8344) += ti-ads8344.o
> obj-$(CONFIG_TI_ADS8688) += ti-ads8688.o
> diff --git a/drivers/iio/adc/ti-ads7142.c b/drivers/iio/adc/ti-ads7142.c
> new file mode 100644
> index 000000000000..6e42efe5f1e8
> --- /dev/null
> +++ b/drivers/iio/adc/ti-ads7142.c
> @@ -0,0 +1,1215 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2020 Jozsef Horvath <info@ministro.hu>
> + *
blank line not needed, plus maybe 2021 given when you are posting it?
> + */
> +#include <linux/i2c.h>
> +#include <linux/device.h>
> +#include <linux/interrupt.h>
> +#include <linux/err.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/events.h>
> +
> +#define TI_ADS7142_NAME "ads7142"
> +
> +#define TI_ADS7142_DATA_VALID_TIMEOUT 100
> +
> +/* Opcodes for commands */
> +/* General */
> +#define TI_ADS7142_OC_GENERAL 0x00
> +/* Single Register Read */
> +#define TI_ADS7142_OC_SINGLE_REG_READ 0x10
> +/* Single Register Write */
> +#define TI_ADS7142_OC_SINGLE_REG_WRITE 0x08
> +/* Single Bit Set */
> +#define TI_ADS7142_OC_SET_BIT 0x18
> +/* Single Bit Clear */
> +#define TI_ADS7142_OC_CLEAR_BIT 0x20
> +/* Block Register Read */
> +#define TI_ADS7142_OC_BLOCK_READ 0x30
> +/* Block Register Write */
> +#define TI_ADS7142_OC_BLOCK_WRITE 0x28
> +
> +/* Registers */
> +/* Reset registers */
> +#define TI_ADS7142_WKEY 0x17
> +#define TI_ADS7142_DEVICE_RESET 0x14
> +/* Functional mode select registers */
> +#define TI_ADS7142_OFFSET_CAL 0x15
> +#define TI_ADS7142_OPMODE_SEL 0x1C
> +#define TI_ADS7142_OPMODE_SEL_MANUALCH0 (0)
> +#define TI_ADS7142_OPMODE_SEL_MANUALSEQ (4)
> +#define TI_ADS7142_OPMODE_SEL_MONITORING (6)
> +#define TI_ADS7142_OPMODE_SEL_HIGHPREC (7)
> +#define TI_ADS7142_OPMODE_STATUS 0x00
> +#define TI_ADS7142_OPMODE_STATUS_OPMODE_MSK (3)
> +#define TI_ADS7142_OPMODE_STATUS_OPMODE_MANUAL (0)
> +#define TI_ADS7142_OPMODE_STATUS_OPMODE_AUTO (2)
> +#define TI_ADS7142_OPMODE_STATUS_OPMODE_HIGHPREC (3)
> +#define TI_ADS7142_OPMODE_STATUS_HS_MODE BIT(2)
> +
> +/* Input config register */
> +#define TI_ADS7142_CH_INPUT_CFG 0x24
> +#define TI_ADS7142_CH_INPUT_CFG_TCSE (0)
> +#define TI_ADS7142_CH_INPUT_CFG_SCSE (1)
> +#define TI_ADS7142_CH_INPUT_CFG_SCPD (2)
> +/* Analog mux and sequencer registers */
> +#define TI_ADS7142_AUTO_SEQ_CHEN 0x20
> +#define TI_ADS7142_AUTO_SEQ_CHEN_CH0 BIT(0)
> +#define TI_ADS7142_AUTO_SEQ_CHEN_CH1 BIT(1)
> +#define TI_ADS7142_START_SEQUENCE 0x1E
> +#define TI_ADS7142_START_SEQUENCE_SEQ_START BIT(0)
> +#define TI_ADS7142_ABORT_SEQUENCE 0x1F
> +#define TI_ADS7142_ABORT_SEQUENCE_SEQ_ABORT BIT(0)
> +#define TI_ADS7142_SEQUENCE_STATUS 0x04
> +#define TI_ADS7142_SEQUENCE_STATUS_SEQ_ERR_ST_MSK (0x06)
> +#define TI_ADS7142_SEQUENCE_STATUS_SEQ_DISABLED (0x00)
> +#define TI_ADS7142_SEQUENCE_STATUS_SEQ_ENABLED (0x02)
> +#define TI_ADS7142_SEQUENCE_STATUS_SEQ_ERROR (0x06)
> +/* Oscillator and timing control registers */
> +#define TI_ADS7142_OSC_SEL 0x18
> +#define TI_ADS7142_OSC_SEL_HSZ_LP BIT(0)
> +#define TI_ADS7142_NCLK_SEL 0x19
> +/* Data buffer control register */
> +#define TI_ADS7142_DATA_BUFFER_OPMODE 0x2C
> +#define TI_ADS7142_DATA_BUFFER_OPMODE_STOP_BURST (0)
> +#define TI_ADS7142_DATA_BUFFER_OPMODE_START_BURST (1)
> +#define TI_ADS7142_DATA_BUFFER_OPMODE_PRE_ALERT (4)
> +#define TI_ADS7142_DATA_BUFFER_OPMODE_POST_ALERT (6)
> +#define TI_ADS7142_DOUT_FORMAT_CFG 0x28
> +#define TI_ADS7142_DOUT_FORMAT_CFG_12B (0)
> +#define TI_ADS7142_DOUT_FORMAT_CFG_12BCH (1)
> +#define TI_ADS7142_DOUT_FORMAT_CFG_12BCHDV (2)
> +#define TI_ADS7142_DATA_BUFFER_STATUS 0x01
> +/* Accumulator control register */
> +#define TI_ADS7142_ACC_EN 0x30
> +#define TI_ADS7142_ACC_CH0_LSB 0x08
> +#define TI_ADS7142_ACC_CH0_MSB 0x09
> +#define TI_ADS7142_ACC_CH1_LSB 0x0A
> +#define TI_ADS7142_ACC_CH1_MSB 0x0B
> +#define TI_ADS7142_ACC_STATUS 0x02
> +/* Digital window comparator registers */
> +#define TI_ADS7142_ALERT_DWC_EN 0x37
> +#define TI_ADS7142_ALERT_DWC_EN_BLOCK_EN BIT(0)
> +#define TI_ADS7142_ALERT_CHEN 0x34
> +#define TI_ADS7142_DWC_HTH_CH0_LSB 0x38
> +#define TI_ADS7142_DWC_HTH_CH0_MSB 0x39
> +#define TI_ADS7142_DWC_LTH_CH0_LSB 0x3A
> +#define TI_ADS7142_DWC_LTH_CH0_MSB 0x3B
> +#define TI_ADS7142_DWC_HYS_CH0 0x40
> +#define TI_ADS7142_DWC_HTH_CH1_LSB 0x3C
> +#define TI_ADS7142_DWC_HTH_CH1_MSB 0x3D
> +#define TI_ADS7142_DWC_LTH_CH1_LSB 0x3E
> +#define TI_ADS7142_DWC_LTH_CH1_MSB 0x3F
> +#define TI_ADS7142_DWC_HYS_CH1 0x41
> +#define TI_ADS7142_PRE_ALT_EVT_CNT 0x36
> +#define TI_ADS7142_ALT_TRIG_CHID 0x03
> +#define TI_ADS7142_ALT_LOW_FLAGS 0x0C
> +#define TI_ADS7142_ALT_LOW_FLAGS_CH0 BIT(0)
> +#define TI_ADS7142_ALT_LOW_FLAGS_CH1 BIT(1)
> +#define TI_ADS7142_ALT_HIGH_FLAGS 0x0E
> +#define TI_ADS7142_ALT_HIGH_FLAGS_CH0 BIT(0)
> +#define TI_ADS7142_ALT_HIGH_FLAGS_CH1 BIT(1)
> +
> +#define TI_ADS7142_THRESHOLD_MSK 0xFFF
> +#define TI_ADS7142_HYSTERESIS_MSK 0x3F
> +
> +struct ti_ads7142_channel_data {
> + int status;
> + int value;
> +};
> +
> +struct ti_ads7142_channel_config {
> + bool alert_low;
> + bool alert_high;
> + int high_threshold;
> + int low_threshold;
> + int hysteresis;
> +};
> +
> +struct ti_ads7142_channel {
> + struct ti_ads7142_channel_config config;
> + struct ti_ads7142_channel_data data;
> + u32 channel;
> +};
> +
> +struct ti_ads7142_config {
> + bool osc_sel;
> + u32 n_clk;
> + bool monitoring_mode;
> +};
> +
> +struct ti_ads7142_priv {
> + struct mutex lock; /* For syncing access to device */
> + struct regulator *vref;
> + struct regulator *power;
> + struct ti_ads7142_config config;
> + int channel_count;
> + struct ti_ads7142_channel *channels;
> + bool monitor_pending;
> +};
> +
> +static const struct iio_event_spec ti_ads7142_events[] = {
> + {
> + .type = IIO_EV_TYPE_THRESH,
> + .dir = IIO_EV_DIR_RISING,
> + .mask_separate = BIT(IIO_EV_INFO_VALUE)
> + | BIT(IIO_EV_INFO_ENABLE),
> + }, {
> + .type = IIO_EV_TYPE_THRESH,
> + .dir = IIO_EV_DIR_FALLING,
> + .mask_separate = BIT(IIO_EV_INFO_VALUE)
> + | BIT(IIO_EV_INFO_ENABLE),
> + }, {
> + .type = IIO_EV_TYPE_THRESH,
> + .dir = IIO_EV_DIR_EITHER,
> + .mask_separate = BIT(IIO_EV_INFO_HYSTERESIS),
> + },
> +};
> +
> +static int ti_ads7142_reg_write(const struct i2c_client *client, u8 reg,
> + u8 data)
> +{
> + struct i2c_msg msg;
> + u8 buf[3];
> + int ret;
> +
> + buf[0] = TI_ADS7142_OC_SINGLE_REG_WRITE;
> + buf[1] = reg;
> + buf[2] = data;
> +
> + msg.addr = client->addr;
> + msg.flags = 0;
> + msg.len = 3;
> + msg.buf = buf;
> +
> + ret = i2c_transfer(client->adapter, &msg, 1);
> +
> + return ret >= 0 ? 0 : ret;
> +}
> +
> +static int ti_ads7142_reg_read(const struct i2c_client *client, u8 reg,
> + u8 *data)
> +{
> + struct i2c_msg msg[2];
Use c99 assignment to do this as something like.
struct i2c_msg msg[2] = {
{
.addr = client->addr,
.len = 2,
.buf = buf,
}, {
.addr = client->addr,
.flags = I2C_M_RD,
.len = 1,
.buf = data,
}
} ;
> + u8 buf[2];
u8 buf[2] = { TI_..., reg };
> + int ret;
> +
> + buf[0] = TI_ADS7142_OC_SINGLE_REG_READ;
> + buf[1] = reg;
> +
> + msg[0].addr = client->addr;
> + msg[0].flags = 0;
> + msg[0].len = 2;
> + msg[0].buf = buf;
> +
> + msg[1].addr = client->addr;
> + msg[1].flags = I2C_M_RD;
> + msg[1].len = 1;
> + msg[1].buf = data;
> +
> + ret = i2c_transfer(client->adapter, msg, 2);
> +
> + return ret >= 0 ? 0 : ret;
> +}
> +
> +static int ti_ads7142_data_buffer_read(const struct i2c_client *client,
> + int length, void *data)
> +{
> + struct i2c_msg msg;
> + int ret;
> +
> + msg.addr = client->addr;
> + msg.flags = I2C_M_RD;
> + msg.len = length;
> + msg.buf = data;
> +
> + ret = i2c_transfer(client->adapter, &msg, 1);
Looks very similar to an i2c_smbus_read_block_data call though I suppose it
is a little odd to use mixture of smbus and non in one driver.
> +
> + return ret >= 0 ? 0 : ret;
> +}
> +
> +static int ti_ads7142_soft_reset(const struct i2c_client *client)
> +{
> + struct i2c_msg msg;
> + u8 buf[2];
u8 buf[2] = { TI_ADS7142_OC_GENERAL, 0x06 }; is more compact for no loss
of readability.
> + int ret;
> +
> + buf[0] = TI_ADS7142_OC_GENERAL;
> + buf[1] = 0x06;
> +
> + msg.addr = client->addr;
> + msg.flags = 0;
> + msg.len = 2;
> + msg.buf = buf;
> +
> + ret = i2c_transfer(client->adapter, &msg, 1);
i2c_master_send() or even better isn't this just an open coded
i2c_smbus_write_byte_data()
> +
> + return ret >= 0 ? 0 : ret;
if ret == 0 then something went wrong and we should report that.
> +}
> +
> +static int ti_ads7142_address2channel(struct iio_dev *indio_dev,
> + int address,
> + struct ti_ads7142_channel **channel)
> +{
> + struct ti_ads7142_priv *priv = iio_priv(indio_dev);
> + int i;
> +
> + for (i = 0; i < priv->channel_count; i++) {
> + if (address == priv->channels[i].channel) {
> + *channel = &priv->channels[i];
> + return 0;
> + }
> + }
> + return -ENODEV;
> +}
> +
> +static int ti_ads7142_sequence_start(struct iio_dev *indio_dev)
> +{
> + struct i2c_client *client = to_i2c_client(indio_dev->dev.parent);
> +
> + return ti_ads7142_reg_write(client, TI_ADS7142_START_SEQUENCE,
> + TI_ADS7142_START_SEQUENCE_SEQ_START);
> +}
> +
> +static int ti_ads7142_sequence_abort(struct iio_dev *indio_dev)
> +{
> + struct i2c_client *client = to_i2c_client(indio_dev->dev.parent);
> +
> + return ti_ads7142_reg_write(client, TI_ADS7142_ABORT_SEQUENCE,
> + TI_ADS7142_ABORT_SEQUENCE_SEQ_ABORT);
> +}
> +
> +static int ti_ads7142_osc_set(struct iio_dev *indio_dev)
> +{
> + struct ti_ads7142_priv *priv = iio_priv(indio_dev);
> + struct i2c_client *client = to_i2c_client(indio_dev->dev.parent);
> + int ret;
> +
> + ret = ti_ads7142_reg_write(client, TI_ADS7142_OSC_SEL,
> + priv->config.osc_sel ? TI_ADS7142_OSC_SEL_HSZ_LP : 0);
> + if (ret)
> + return ret;
> +
> + return ti_ads7142_reg_write(client, TI_ADS7142_NCLK_SEL,
> + priv->config.n_clk);
> +}
> +
> +static int ti_ads7142_input_cfg_set(struct iio_dev *indio_dev)
> +{
> + struct i2c_client *client = to_i2c_client(indio_dev->dev.parent);
> +
> + return ti_ads7142_reg_write(client, TI_ADS7142_CH_INPUT_CFG,
> + TI_ADS7142_CH_INPUT_CFG_TCSE);
> +}
> +
> +static int ti_ads7142_dout_format_set(struct iio_dev *indio_dev)
> +{
> + struct i2c_client *client = to_i2c_client(indio_dev->dev.parent);
> +
> + return ti_ads7142_reg_write(client, TI_ADS7142_DOUT_FORMAT_CFG,
> + TI_ADS7142_DOUT_FORMAT_CFG_12BCHDV);
> +}
> +
> +static int ti_ads7142_hth_set(struct iio_dev *indio_dev, int channel,
> + int threshold)
> +{
> + struct i2c_client *client = to_i2c_client(indio_dev->dev.parent);
> + int ret;
> +
> + if (threshold < 0 || threshold > TI_ADS7142_THRESHOLD_MSK)
> + return -EINVAL;
> +
> + ret = ti_ads7142_reg_write(client,
> + TI_ADS7142_DWC_HTH_CH0_LSB + channel * 4,
> + threshold & 0xFF);
> + if (ret)
> + return ret;
> +
> + ret = ti_ads7142_reg_write(client,
> + TI_ADS7142_DWC_HTH_CH0_MSB + channel * 4,
> + (threshold >> 8) & 0xF);
> + return ret;
> +}
> +
> +static int ti_ads7142_lth_set(struct iio_dev *indio_dev, int channel,
> + int threshold)
> +{
> + struct i2c_client *client = to_i2c_client(indio_dev->dev.parent);
> + int ret;
> +
> + if (threshold < 0 || threshold > TI_ADS7142_THRESHOLD_MSK)
> + return -EINVAL;
> +
> + ret = ti_ads7142_reg_write(client,
> + TI_ADS7142_DWC_LTH_CH0_LSB + channel * 4,
> + threshold & 0xFF);
> + if (ret)
> + return ret;
> +
> + ret = ti_ads7142_reg_write(client,
> + TI_ADS7142_DWC_LTH_CH0_MSB + channel * 4,
> + (threshold >> 8) & 0xF);
> + return ret;
> +}
> +
> +static int ti_ads7142_hys_set(struct iio_dev *indio_dev, int channel,
> + int hysteresis)
> +{
> + struct i2c_client *client = to_i2c_client(indio_dev->dev.parent);
> + int ret;
> +
> + if (hysteresis < 0 || hysteresis > TI_ADS7142_HYSTERESIS_MSK)
> + return -EINVAL;
> +
> + ret = ti_ads7142_reg_write(client, TI_ADS7142_DWC_HYS_CH0 + channel,
> + hysteresis & TI_ADS7142_HYSTERESIS_MSK);
> + return ret;
> +}
> +
> +static int ti_ads7142_collect_channel_data(struct iio_dev *indio_dev,
> + int *channel_collected)
> +{
> + struct i2c_client *client = to_i2c_client(indio_dev->dev.parent);
> + struct ti_ads7142_channel *channel;
> + u16 data_buffer;
> + u8 data_buffer_status;
> + int data_valid;
> + int channel_address;
> + int value;
> + int ret;
> +
> + ret = ti_ads7142_reg_read(client, TI_ADS7142_DATA_BUFFER_STATUS,
> + &data_buffer_status);
> + if (ret)
> + return ret;
> +
> + data_buffer_status &= 0x1F;
> +
> + do {
> + ret = ti_ads7142_data_buffer_read(client, sizeof(data_buffer),
> + &data_buffer);
> + if (ret)
> + break;
> + data_buffer = be16_to_cpu(data_buffer);
> + data_valid = data_buffer & 1;
> + if (data_valid) {
> + channel_address = (data_buffer >> 1) & 0x7;
> + value = data_buffer >> 4;
> + ret = ti_ads7142_address2channel(indio_dev,
> + channel_address,
> + &channel);
> + if (!ret) {
> + channel->data.status = data_valid;
> + channel->data.value = value;
> + *channel_collected |= 1 << channel_address;
> + }
> + }
> + } while (--data_buffer_status);
> +
> + return ret;
> +}
> +
> +static int ti_ads7142_do_work(struct iio_dev *indio_dev)
As mentioned below, these function needs a more informative name.
> +{
> + struct ti_ads7142_priv *priv = iio_priv(indio_dev);
> + struct i2c_client *client = to_i2c_client(indio_dev->dev.parent);
> + int i;
> + int alert_ch = 0;
> + int ret;
> +
> + if (!priv->config.monitoring_mode)
> + return 0;
> +
> + mutex_lock(&priv->lock);
> + priv->monitor_pending = false;
> +
> + ret = ti_ads7142_sequence_abort(indio_dev);
> + if (ret)
> + goto final;
> +
> + ret = ti_ads7142_osc_set(indio_dev);
> + if (ret)
> + goto final;
> +
> + ret = ti_ads7142_input_cfg_set(indio_dev);
> + if (ret)
> + goto final;
> +
> + ret = ti_ads7142_dout_format_set(indio_dev);
> + if (ret)
> + goto final;
> +
> + ret = ti_ads7142_reg_write(client, TI_ADS7142_DATA_BUFFER_OPMODE,
> + TI_ADS7142_DATA_BUFFER_OPMODE_PRE_ALERT);
> + if (ret)
> + goto final;
> +
> + ret = ti_ads7142_reg_write(client, TI_ADS7142_OPMODE_SEL,
> + TI_ADS7142_OPMODE_SEL_MONITORING);
> + if (ret)
> + goto final;
> +
> + ret = ti_ads7142_reg_write(client, TI_ADS7142_AUTO_SEQ_CHEN,
> + TI_ADS7142_AUTO_SEQ_CHEN_CH0
> + | TI_ADS7142_AUTO_SEQ_CHEN_CH1);
> + if (ret)
> + goto final;
> +
> + ret = ti_ads7142_reg_write(client, TI_ADS7142_PRE_ALT_EVT_CNT, 0);
> + if (ret)
> + goto final;
> +
> + ret = ti_ads7142_reg_write(client, TI_ADS7142_ALT_LOW_FLAGS,
> + TI_ADS7142_ALT_LOW_FLAGS_CH0
> + | TI_ADS7142_ALT_LOW_FLAGS_CH1);
> + if (ret)
> + goto final;
> +
> + ret = ti_ads7142_reg_write(client, TI_ADS7142_ALT_HIGH_FLAGS,
> + TI_ADS7142_ALT_HIGH_FLAGS_CH0
> + | TI_ADS7142_ALT_HIGH_FLAGS_CH1);
> + if (ret)
> + goto final;
> +
> + for (i = 0; i < priv->channel_count; i++) {
> + ret = ti_ads7142_hth_set(indio_dev, priv->channels[i].channel,
> + priv->channels[i].config.high_threshold);
> + if (ret)
> + goto final;
> +
> + ret = ti_ads7142_lth_set(indio_dev, priv->channels[i].channel,
> + priv->channels[i].config.low_threshold);
> + if (ret)
> + goto final;
> +
> + ret = ti_ads7142_hys_set(indio_dev, priv->channels[i].channel,
> + priv->channels[i].config.hysteresis);
> + if (ret)
> + goto final;
> +
> + if (priv->channels[i].config.alert_low ||
> + priv->channels[i].config.alert_high) {
> + alert_ch |= 1 << priv->channels[i].channel;
> + }
> + }
> +
> + ret = ti_ads7142_reg_write(client, TI_ADS7142_ALERT_DWC_EN,
> + alert_ch ? TI_ADS7142_ALERT_DWC_EN_BLOCK_EN : 0);
> + if (ret)
> + goto final;
> +
> + ret = ti_ads7142_reg_write(client, TI_ADS7142_ALERT_CHEN,
> + alert_ch);
> + if (ret)
> + goto final;
> +
> + if (alert_ch) {
> + ret = ti_ads7142_sequence_start(indio_dev);
> + priv->monitor_pending = !ret;
> + }
> +final:
> + mutex_unlock(&priv->lock);
> + return ret;
> +}
> +
> +static int ti_ads7142_read_channel_manual(struct iio_dev *indio_dev,
> + int address, int *val)
> +{
> + struct ti_ads7142_priv *priv = iio_priv(indio_dev);
> + struct i2c_client *client = to_i2c_client(indio_dev->dev.parent);
> + u16 data_buffer;
> + int ret;
> +
> + if (address < 0 || address > 1)
I'm assuming there is no way we could get here with this not being true?
If so drop it. If it is possible, then add a comment as it seems like
an odd thing to need to check.
> + return -EINVAL;
> +
> + mutex_lock(&priv->lock);
> + ret = ti_ads7142_sequence_abort(indio_dev);
> + if (ret)
> + goto final;
> +
> + ret = ti_ads7142_osc_set(indio_dev);
> + if (ret)
> + goto final;
> +
> + ret = ti_ads7142_input_cfg_set(indio_dev);
> + if (ret)
> + goto final;
> +
> + ret = ti_ads7142_dout_format_set(indio_dev);
> + if (ret)
> + goto final;
> +
> + ret = ti_ads7142_reg_write(client, TI_ADS7142_OPMODE_SEL,
> + TI_ADS7142_OPMODE_SEL_MANUALSEQ);
> + if (ret)
> + goto final;
> +
> + ret = ti_ads7142_reg_write(client, TI_ADS7142_AUTO_SEQ_CHEN,
> + 1 << address);
> + if (ret)
> + goto final;
> +
> + ret = ti_ads7142_sequence_start(indio_dev);
> + if (ret)
> + goto final;
> +
> + ret = ti_ads7142_data_buffer_read(client, sizeof(data_buffer),
> + &data_buffer);
> + if (ret)
> + goto abort;
> +
> + *val = (be16_to_cpu(data_buffer) >> 4);
> +
> +abort:
> + ret = ti_ads7142_sequence_abort(indio_dev);
> +final:
> + mutex_unlock(&priv->lock);
> + return ret;
> +}
> +
> +static int ti_ads7142_read_channel_monitor(struct iio_dev *indio_dev,
> + int address, int *val)
> +{
> + struct ti_ads7142_priv *priv = iio_priv(indio_dev);
> + struct ti_ads7142_channel *channel;
> + int ret;
> +
> + if (address < 0 || address > 1)
> + return -EINVAL;
> +
> + ret = ti_ads7142_address2channel(indio_dev, address, &channel);
> + if (ret)
> + return ret;
> +
> + mutex_lock(&priv->lock);
> + if (!channel->data.status) {
> + ret = -EAGAIN;
> + } else {
> + *val = channel->data.value;
> + channel->data.status = 0;
> + ret = 0;
ret already is 0 so no need to set it.
> + }
> + mutex_unlock(&priv->lock);
> + return ret;
> +}
> +
> +static int ti_ads7142_read_channel(struct iio_dev *indio_dev,
> + int address, int *val)
> +{
> + struct ti_ads7142_priv *priv = iio_priv(indio_dev);
> +
> + if (priv->config.monitoring_mode)
> + return ti_ads7142_read_channel_monitor(indio_dev, address,
> + val);
> + return ti_ads7142_read_channel_manual(indio_dev, address, val);
> +}
> +
> +static irqreturn_t ti_ads7142_ist(int irq, void *dev_id)
> +{
> + struct iio_dev *indio_dev = dev_id;
> + struct i2c_client *client = to_i2c_client(indio_dev->dev.parent);
> + struct ti_ads7142_priv *priv = iio_priv(indio_dev);
> + struct ti_ads7142_channel *channel;
> + u8 low_flags;
> + u8 high_flags;
> + u8 seq_st;
> + int i;
> + int ret;
> + int channel_collected;
> + s64 timestamp = iio_get_time_ns(indio_dev);
> +
> + mutex_lock(&priv->lock);
> + if (!priv->config.monitoring_mode || !priv->monitor_pending) {
> + mutex_unlock(&priv->lock);
> + return IRQ_NONE;
> + }
> +
> + ret = ti_ads7142_reg_read(client, TI_ADS7142_SEQUENCE_STATUS, &seq_st);
> + if (ret) {
> + dev_err(indio_dev->dev.parent,
> + "%s: SEQUENCE_STATUS reg read error(%i)",
> + __func__, ret);
> + goto final;
> + }
> +
> + if ((seq_st & TI_ADS7142_SEQUENCE_STATUS_SEQ_ERR_ST_MSK)
> + != TI_ADS7142_SEQUENCE_STATUS_SEQ_ENABLED) {
> + dev_err(indio_dev->dev.parent,
> + "%s: SEQUENCE_STATUS error(%i)",
> + __func__, seq_st);
> + goto final;
> + }
> +
> + ret = ti_ads7142_reg_read(client, TI_ADS7142_ALT_LOW_FLAGS,
> + &low_flags);
> + if (ret) {
> + dev_err(indio_dev->dev.parent,
> + "%s: ALT_LOW_FLAGS reg read error(%i)",
> + __func__, ret);
> + goto final;
> + }
> +
> + ret = ti_ads7142_reg_read(client, TI_ADS7142_ALT_HIGH_FLAGS,
> + &high_flags);
> + if (ret) {
> + dev_err(indio_dev->dev.parent,
> + "%s: ALT_HIGH_FLAGS reg read error(%i)",
> + __func__, ret);
> + goto final;
> + }
> +
> + channel_collected = 0;
> + ret = ti_ads7142_collect_channel_data(indio_dev, &channel_collected);
> + if (ret)
> + goto final;
> +
> + if (!channel_collected)
> + goto final;
> +
> + for (i = 0; i < priv->channel_count; i++) {
> + channel = &priv->channels[i];
> + if (!(channel_collected & (1 << channel->channel)))
> + continue;
Perhaps use a for_each_bit_set(i, channel_collected) to simplify this.
> + if (channel->config.alert_low &&
> + (low_flags & (1 << channel->channel))) {
> + iio_push_event(indio_dev,
> + IIO_UNMOD_EVENT_CODE(IIO_VOLTAGE,
> + channel->channel,
> + IIO_EV_TYPE_THRESH,
> + IIO_EV_DIR_FALLING),
> + timestamp);
> + }
> +
> + if (channel->config.alert_high &&
> + (high_flags & (1 << channel->channel))) {
> + iio_push_event(indio_dev,
> + IIO_UNMOD_EVENT_CODE(IIO_VOLTAGE,
> + channel->channel,
> + IIO_EV_TYPE_THRESH,
> + IIO_EV_DIR_RISING),
> + timestamp);
> + }
> + }
> +
> +final:
> + mutex_unlock(&priv->lock);
> +
> + ret = ti_ads7142_do_work(indio_dev);
> + if (ret) {
> + dev_err(indio_dev->dev.parent,
> + "%s: start monitoring error(%i)",
> + __func__, ret);
> + return IRQ_NONE;
> + }
> + return IRQ_HANDLED;
> +}
> +
> +static int ti_ads7142_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int *val, int *val2, long info)
> +{
> + struct ti_ads7142_priv *priv = iio_priv(indio_dev);
> + int ret;
> +
> + switch (info) {
> + case IIO_CHAN_INFO_RAW:
> + ret = ti_ads7142_read_channel(indio_dev, chan->address, val);
> + if (!ret)
if (ret)
return ret;
return IIO_VAL_INT;
Always have error cases out of line. That consistency makes
it easier to review.
> + ret = IIO_VAL_INT;
> + break;
> + case IIO_CHAN_INFO_SAMP_FREQ:
> + *val = priv->config.n_clk;
> + ret = IIO_VAL_INT;
> + break;
> + case IIO_CHAN_INFO_SCALE:
> + if (IS_ERR(priv->vref)) {
> + ret = -EINVAL;
> + } else {
> + *val = regulator_get_voltage(priv->vref) / 1000;
> + *val2 = chan->scan_type.realbits;
> + ret = IIO_VAL_FRACTIONAL_LOG2;
> + }
> + break;
> + default:
> + ret = -EINVAL;
> + }
> + return ret;
> +}
> +
> +static int ti_ads7142_write_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int val, int val2, long mask)
> +{
> + struct ti_ads7142_priv *priv = iio_priv(indio_dev);
> + int ret;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_SAMP_FREQ:
> + priv->config.n_clk = val;
> + if (priv->config.monitoring_mode)
> + ret = ti_ads7142_do_work(indio_dev);
return ti_...
Early returns almost always easier to read.
Note applies to lots of stuff above.
> + else
> + ret = 0;
> + break;
> + default:
> + ret = -EINVAL;
> + break;
> + }
> +
> + return ret;
> +}
> +
> +static int ti_ads7142_read_event_value(struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan,
> + enum iio_event_type type,
> + enum iio_event_direction dir,
> + enum iio_event_info info,
> + int *val, int *val2)
> +{
> + struct ti_ads7142_priv *priv = iio_priv(indio_dev);
> + struct ti_ads7142_channel *channel;
> + int ret;
> +
> + if (!priv->config.monitoring_mode)
> + return -EINVAL;
> +
> + ret = ti_ads7142_address2channel(indio_dev, chan->address,
> + &channel);
> + if (ret)
> + return ret;
> +
> + switch (info) {
> + case IIO_EV_INFO_VALUE:
> + if (dir == IIO_EV_DIR_RISING)
> + *val = channel->config.high_threshold;
> + else
> + *val = channel->config.low_threshold;
> + ret = IIO_VAL_INT;
> + break;
> + case IIO_EV_INFO_HYSTERESIS:
> + *val = channel->config.hysteresis;
> + ret = IIO_VAL_INT;
return IIO_VAL_INT; etc
> + break;
> + default:
> + ret = -EINVAL;
return -EINVAL;
> + break;
> + }
> + return ret;
> +}
> +
> +static int ti_ads7142_write_event_value(struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan,
> + enum iio_event_type type,
> + enum iio_event_direction dir,
> + enum iio_event_info info,
> + int val, int val2)
> +{
> + struct ti_ads7142_priv *priv = iio_priv(indio_dev);
> + struct ti_ads7142_channel *channel;
> + bool have_to_do = false;
> + int ret;
> +
> + if (!priv->config.monitoring_mode)
> + return -EINVAL;
> +
> + ret = ti_ads7142_address2channel(indio_dev, chan->address,
> + &channel);
> + if (ret)
> + return ret;
> +
> + mutex_lock(&priv->lock);
> + switch (info) {
> + case IIO_EV_INFO_VALUE:
> + if (val < 0 || val > TI_ADS7142_THRESHOLD_MSK) {
> + ret = -EINVAL;
> + } else {
> + if (dir == IIO_EV_DIR_RISING) {
> + if (val != channel->config.high_threshold) {
> + channel->config.high_threshold = val;
> + have_to_do = true;
> + }
> + } else {
> + if (val != channel->config.low_threshold) {
> + channel->config.low_threshold = val;
> + have_to_do = true;
> + }
> + }
> + }
> + break;
> + case IIO_EV_INFO_HYSTERESIS:
> + if (val < 0 || val > TI_ADS7142_HYSTERESIS_MSK) {
> + ret = -EINVAL;
> + } else {
> + if (val != channel->config.hysteresis) {
> + channel->config.hysteresis = val;
> + have_to_do = true;
> + }
> + }
> + break;
> + default:
> + ret = -EINVAL;
> + break;
> + }
> + mutex_unlock(&priv->lock);
> + if (!ret && have_to_do)
> + ret = ti_ads7142_do_work(indio_dev);
> + return ret;
> +}
> +
> +static int ti_ads7142_read_event_config(struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan,
> + enum iio_event_type type,
> + enum iio_event_direction dir)
> +{
> + struct ti_ads7142_priv *priv = iio_priv(indio_dev);
> + struct ti_ads7142_channel *channel;
> + int ret;
> +
> + if (!priv->config.monitoring_mode)
> + return -EINVAL;
> +
> + if (type != IIO_EV_TYPE_THRESH)
> + return -EINVAL;
> +
> + ret = ti_ads7142_address2channel(indio_dev, chan->address,
> + &channel);
> + if (ret)
> + return ret;
> +
> + if (dir == IIO_EV_DIR_RISING)
> + ret = channel->config.alert_high ? 1 : 0;
Not fine using ret = channel->config.alert_high; directly?
> + else
> + ret = channel->config.alert_low ? 1 : 0;
> +
> + return ret;
> +}
> +
> +static int ti_ads7142_write_event_config(struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan,
> + enum iio_event_type type,
> + enum iio_event_direction dir,
> + int state)
> +{
> + struct ti_ads7142_priv *priv = iio_priv(indio_dev);
> + struct ti_ads7142_channel *channel;
> + bool have_to_do = false;
> + int ret;
> +
> + if (!priv->config.monitoring_mode)
> + return -EINVAL;
> +
> + if (type != IIO_EV_TYPE_THRESH)
> + return -EINVAL;
> +
> + ret = ti_ads7142_address2channel(indio_dev, chan->address,
> + &channel);
> + if (ret)
> + return ret;
> +
> + mutex_lock(&priv->lock);
> + if (dir == IIO_EV_DIR_RISING) {
> + if (channel->config.alert_high != state) {
> + channel->config.alert_high = state;
> + have_to_do = true;
> + }
> + } else {
> + if (channel->config.alert_low != state) {
> + channel->config.alert_low = state;
> + have_to_do = true;
> + }
> + }
> + mutex_unlock(&priv->lock);
> +
> + if (have_to_do)
> + ret = ti_ads7142_do_work(indio_dev);
that's going to need a better name as I have no idea what _do_work implies.
> +
> + return ret;
> +}
> +
> +static const struct iio_info ti_ads7142_iio_info = {
If interrupt is going to be optional, pick between two versions of struct iio_info
so that we don't provide callbacks for the case where we have no interrupts.
> + .read_raw = ti_ads7142_read_raw,
> + .write_raw = ti_ads7142_write_raw,
> + .read_event_value = ti_ads7142_read_event_value,
> + .write_event_value = ti_ads7142_write_event_value,
> + .read_event_config = ti_ads7142_read_event_config,
> + .write_event_config = ti_ads7142_write_event_config,
> +};
> +
> +static int ti_ads7142_parse_channel_config_of(struct device *dev,
> + struct iio_dev *indio_dev)
> +{
> + struct ti_ads7142_priv *priv = iio_priv(indio_dev);
> + struct device_node *channel_node;
> + struct iio_chan_spec *iio_channels;
> + struct iio_chan_spec *iio_channel;
> + struct ti_ads7142_channel *ads_channel;
> + int channel_index = 0;
> + int ret;
> +
> + priv->channel_count = of_get_available_child_count(dev->of_node);
> + if (!priv->channel_count) {
> + dev_err(dev, "dt: there is no channel definition");
> + return -ENODEV;
> + }
> +
> + priv->channels = devm_kcalloc(dev, priv->channel_count,
> + sizeof(*priv->channels),
> + GFP_KERNEL);
> + if (!priv->channels)
> + return -ENOMEM;
> +
> + indio_dev->num_channels = priv->channel_count;
Why do you need that in two places?
> + iio_channels = devm_kcalloc(dev, priv->channel_count,
> + sizeof(*iio_channels),
> + GFP_KERNEL);
> + if (!iio_channels)
> + return -ENOMEM;
> +
> + indio_dev->channels = iio_channels;
> +
> + for_each_available_child_of_node(dev->of_node, channel_node) {
> + ads_channel = &priv->channels[channel_index];
> +
> + ret = of_property_read_u32(channel_node, "reg",
> + &ads_channel->channel);
> + if (ret)
> + goto err;
> +
> + iio_channel = &iio_channels[channel_index];
> + iio_channel->type = IIO_VOLTAGE;
> + iio_channel->indexed = 1;
> + iio_channel->info_mask_separate = BIT(IIO_CHAN_INFO_RAW)
> + | BIT(IIO_CHAN_INFO_SAMP_FREQ);
> + if (!IS_ERR(priv->vref))
Ah. This somewhat explains the optional regulator.
We have done this in a few old drivers because they initially were missing vref support
and we couldn't break existing device tree bindings. I'm not keen to see it
done for a new driver. Just make the vref-supply required.
If it's a fixed voltage that is always on, then the device tree additions are
trivial anyway.
> + iio_channel->info_mask_separate |= BIT(IIO_CHAN_INFO_SCALE);
> + iio_channel->scan_type.sign = 'u';
> + iio_channel->scan_type.realbits = 12;
> + iio_channel->scan_type.storagebits = 16;
> + iio_channel->scan_type.shift = 0;
No need to specify obvious default shift of 0. Rely on the zeroed allocation.
> + iio_channel->scan_type.endianness = IIO_CPU;
> + iio_channel->address = ads_channel->channel;
> + iio_channel->scan_index = ads_channel->channel;
> + iio_channel->channel = ads_channel->channel;
> + if (priv->config.monitoring_mode) {
> + iio_channel->event_spec = ti_ads7142_events;
> + iio_channel->num_event_specs = ARRAY_SIZE(ti_ads7142_events);
> + }
> +
> + ads_channel->config.high_threshold = TI_ADS7142_THRESHOLD_MSK;
> + ret = of_property_read_u32(channel_node, "ti,threshold-rising",
These are usually considered a matter of userspace policy only. Will need a strong
argument for them to be in DT.
> + &ads_channel->config.high_threshold);
> + ads_channel->config.alert_high = !ret;
> + ret = of_property_read_u32(channel_node, "ti,threshold-falling",
> + &ads_channel->config.low_threshold);
> + ads_channel->config.alert_low = !ret;
> + ret = of_property_read_u32(channel_node, "ti,hysteresis",
> + &ads_channel->config.hysteresis);
> + channel_index++;
> + }
> +
> + return 0;
> +err:
> + of_node_put(channel_node);
> + return ret;
> +}
> +
> +static int ti_ads7142_parse_config_of(struct device *dev,
> + struct iio_dev *indio_dev)
> +{
> + struct ti_ads7142_priv *priv = iio_priv(indio_dev);
> +
> + priv->config.osc_sel = of_property_read_bool(dev->of_node,
> + "ti,osc-sel");
Please use generic device property access functions where possible.
That basically gives us support on non OF based platforms for free.
> + of_property_read_u32(dev->of_node, "ti,n-clk", &priv->config.n_clk);
> + priv->config.monitoring_mode = of_property_read_bool(dev->of_node,
> + "ti,monitoring-mode");
> +
> + return ti_ads7142_parse_channel_config_of(dev, indio_dev);
> +}
> +
> +static int ti_ads7142_probe(struct i2c_client *client,
> + const struct i2c_device_id *id)
> +{
> + struct iio_dev *indio_dev;
> + struct ti_ads7142_priv *priv;
> + int ret;
> +
> + ret = ti_ads7142_soft_reset(client);
> + if (ret)
> + return ret;
> +
> + indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*priv));
> + if (!indio_dev)
> + return -ENOMEM;
> +
> + priv = iio_priv(indio_dev);
> + i2c_set_clientdata(client, indio_dev);
> +
> + indio_dev->dev.parent = &client->dev;
> + indio_dev->dev.of_node = client->dev.of_node;
Both of the above are set by the iio core, so no need to set her.
> + indio_dev->name = TI_ADS7142_NAME;
> + indio_dev->modes = INDIO_DIRECT_MODE;
> + indio_dev->info = &ti_ads7142_iio_info;
> +
> + mutex_init(&priv->lock);
> +
> + priv->vref = devm_regulator_get_optional(&client->dev, "vref");
There isn't a vref pin on the device. Vref is used in the datasheet
but the pinout makes it clear it is actually avdd which is definitely
not optional.
> + if (!IS_ERR(priv->vref)) {
> + ret = regulator_enable(priv->vref);
> + if (ret)
> + goto err;
As you have simple handling of power in here, I would definitely look
to use managed calls to disable the regulators.
devm_add_action_or_reset() is usual way of doing this.
If nothing else it will ensure that the unwind on removal is the mirror
image of the setup on probe() and hence it is much easier to avoid any
subtle race conditions.
> + }
> +
> + priv->power = devm_regulator_get_optional(&client->dev, "power");
dvdd? Not sure what else power could be. Note that you should only
use _get_optional() for regulators that really optional and may not be
connected. For cases where you simply can't control them and they aren't
specified in the dts then devm_regulator_get() will supply a stub regulator
that is always on.
> + if (!IS_ERR(priv->power)) {
> + ret = regulator_enable(priv->power);
> + if (ret)
> + goto err_regulator;
> + }
> +
> + ret = ti_ads7142_parse_config_of(&client->dev, indio_dev);
> + if (ret)
> + goto err_regulator;
> +
> + if (!client->irq && priv->config.monitoring_mode) {
Given you can check this before you have to do any unwinding, move it
earlier.
> + ret = -EINVAL;
> + dev_err(&client->dev, "Interrupt not specified\n");
> + goto err_regulator;
> + }
> + if (client->irq && priv->config.monitoring_mode) {
> + ret = devm_request_threaded_irq(&client->dev, client->irq,
> + NULL, ti_ads7142_ist,
> + IRQF_ONESHOT | IRQF_SHARED,
> + dev_name(&client->dev),
> + indio_dev);
> + if (ret) {
> + dev_err(&client->dev, "Unable to request IRQ %i",
> + client->irq);
> + goto err_regulator;
> + }
> + }
> +
> + ret = iio_device_register(indio_dev);
> + if (ret) {
> + dev_err(&client->dev, "Failed to register iio device");
> + goto err_regulator;
> + }
> +
> + ret = ti_ads7142_do_work(indio_dev);
As iio_device_register() is responsible for exposing userspace interfaces it
is very rarely a good idea to do anything after it in probe()
(some exceptions for runtime pm autosuspend setup as that can kick in later)
> + if (!ret) {
> + dev_info(&client->dev, "%s is a %s device at address 0x%X",
> + dev_name(&indio_dev->dev), indio_dev->name,
> + client->addr);
> + return ret;
Always have the error path out of line as it is much more consistent
with what reviewers are expecting
if (ret)
goto error_unregister()
return 0;
> + }
> +
> + iio_device_unregister(indio_dev);
> +
> +err_regulator:
> + if (!IS_ERR(priv->vref))
> + regulator_disable(priv->vref);
> + if (!IS_ERR(priv->power))
> + regulator_disable(priv->power);
> +err:
> + mutex_destroy(&priv->lock);
We rarely bother with mutex_destroy() in paths where the driver is being
removed as it is only useful for debugging use of mutexes after they have
been removed. That doesn't happen in these paths so it tends to just be
noise to have a mutex_destroy(). This function is useful if you have a
mutex inside something with a different lifespan from the device and
like to enable mutex debugging.
> +
> + return ret;
> +}
> +
> +static int ti_ads7142_remove(struct i2c_client *client)
> +{
> + struct iio_dev *indio_dev = i2c_get_clientdata(client);
> + struct ti_ads7142_priv *priv = iio_priv(indio_dev);
> +
> + if (!IS_ERR(priv->vref))
> + regulator_disable(priv->vref);
If you'd not used _optional() above you could do these without worrying
about whether the regulator was there or not.
> + if (!IS_ERR(priv->power))
> + regulator_disable(priv->power);
> + mutex_destroy(&priv->lock);
> + iio_device_unregister(indio_dev);
_remove should always look like the unwinding of probe - so
I should be able to match items in reverse order.
> +
> + return 0;
> +}
> +
> +static int __maybe_unused ti_ads7142_suspend(struct device *dev)
> +{
> + struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
> + struct ti_ads7142_priv *priv = iio_priv(indio_dev);
> +
> + /**
> + * Keep all regulators on when the device in autonomous
> + * monitoring mode.
> + * The device can wake up the system with ALERT pin
> + **/
> + if (priv->config.monitoring_mode && priv->monitor_pending)
> + return 0;
> +
> + if (!IS_ERR(priv->vref))
> + regulator_disable(priv->vref);
> + if (!IS_ERR(priv->power))
> + regulator_disable(priv->power);
> +
> + return 0;
> +}
> +
> +static int __maybe_unused ti_ads7142_resume(struct device *dev)
> +{
> + struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
> + struct ti_ads7142_priv *priv = iio_priv(indio_dev);
> + int ret;
> +
> + /**
> + * Nothing to do when the device in autonomous monitoring mode.
> + **/
> + if (priv->config.monitoring_mode && priv->monitor_pending)
> + return 0;
> +
> + if (!IS_ERR(priv->vref)) {
> + ret = regulator_enable(priv->vref);
> + if (ret)
> + return ret;
> + }
> + if (!IS_ERR(priv->power)) {
> + ret = regulator_enable(priv->power);
> + if (ret)
> + return ret;
> + }
> + return 0;
> +}
> +
> +static SIMPLE_DEV_PM_OPS(ti_ads7142_pm_ops, ti_ads7142_suspend,
> + ti_ads7142_resume);
> +
> +static const struct i2c_device_id ti_ads7142_id[] = {
> + { TI_ADS7142_NAME, 0 },
> + { }
> +};
> +MODULE_DEVICE_TABLE(i2c, ti_ads7142_id);
> +
> +static const struct of_device_id ti_ads7142_of_match[] = {
> + { .compatible = "ti,ads7142" },
> + {}
> +};
> +MODULE_DEVICE_TABLE(of, ti_ads7142_of_match);
> +
> +static struct i2c_driver ti_ads7142_driver = {
> + .driver = {
> + .name = TI_ADS7142_NAME,
> + .of_match_table = ti_ads7142_of_match,
> + .pm = &ti_ads7142_pm_ops,
> + },
> + .probe = ti_ads7142_probe,
> + .remove = ti_ads7142_remove,
> + .id_table = ti_ads7142_id,
> +};
> +
> +module_i2c_driver(ti_ads7142_driver);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Jozsef Horvath <info@ministro.hu>");
> +MODULE_DESCRIPTION("Texas Instruments TI_ADS7142 ADC driver");
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] dt-bindings: iio: adc: devicetree bindings for texas instruments ads7142 iio driver
2021-05-01 18:25 ` [PATCH 2/2] dt-bindings: iio: adc: devicetree bindings for texas instruments ads7142 iio driver Jozsef Horvath
@ 2021-05-02 17:22 ` Jonathan Cameron
2021-05-02 21:10 ` József Horváth
0 siblings, 1 reply; 10+ messages in thread
From: Jonathan Cameron @ 2021-05-02 17:22 UTC (permalink / raw)
To: Jozsef Horvath
Cc: Lars-Peter Clausen, Peter Meerwald-Stadler, Rob Herring,
Liam Girdwood, Mark Brown, Alexandru Ardelean, Heiko Stuebner,
Andy Shevchenko, Alex Dewar, Gene Chen, Saravanan Sekar,
Lee Jones, linux-iio, devicetree, linux-kernel
On Sat, 1 May 2021 18:25:18 +0000
Jozsef Horvath <info@ministro.hu> wrote:
> This is a device tree schema for iio driver for
> Texas Instruments ADS7142 dual-channel, programmable sensor monitor.
>
> Datasheet: https://www.ti.com/lit/ds/symlink/ads7142.pdf
>
> Signed-off-by: Jozsef Horvath <info@ministro.hu>
> ---
> ---
> .../bindings/iio/adc/ti,ads7142.yaml | 198 ++++++++++++++++++
> 1 file changed, 198 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/iio/adc/ti,ads7142.yaml
>
> diff --git a/Documentation/devicetree/bindings/iio/adc/ti,ads7142.yaml b/Documentation/devicetree/bindings/iio/adc/ti,ads7142.yaml
> new file mode 100644
> index 000000000000..b4e752160156
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/ti,ads7142.yaml
> @@ -0,0 +1,198 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: "http://devicetree.org/schemas/iio/adc/ti,ads7142.yaml#"
> +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
> +
> +title: Texas Instruments ADS7142 adc driver device tree bindings
> +
> +maintainers:
> + - József Horváth <info@ministro.hu>
> +
> +description: |
> + This document is for describing the required device tree parameters
> + for ads7142 adc driver.
Document describes hardware, not a particular driver. So just refer
to the device here. There may well be other drives in future using
the same binding (e.g. in an RTOS).
> + The required parameters for proper operation are described below.
> +
> + Datasheet: https://www.ti.com/lit/ds/symlink/ads7142.pdf
> +
> + Operation modes supportedby the driver:
> + When the 'ti,monitoring-mode' property is not present
> + in the devicetree node definition, the driver initiates a single
> + conversion in the device for each read request
> + (/sys/bus/iio/devices/iio:deviceX/in_voltageY_raw).
> + This is a one-shot conversion, and it is called
> + "Manual Mode" in the datasheet.
> +
> + When the 'ti,monitoring-mode' property is present
> + in the devicetree node definition, the driver configures
> + the device's digital window comparator and sets the device's
> + data buffer operation mode to pre alert data mode.
> + The driver reads the conversion result when the BUSY/RDY interrupt
> + fires, and keeps the value until the next BUSY/RDY interrupt
> + or the first read request
> + (/sys/bus/iio/devices/iio:deviceX/in_voltageY_raw).
> + The digital window comparator and hysteresis parameters
> + can be controlled by:
> + - the devicetree definition of channel node
> + - iio sysfs interfaces
> + This is event driven conversion, and is called
> + "Autonomous Mode with Pre Alert Data" in the datasheet.
> + This mode can be used to wake up the system with the ALERT pin,
> + in case when the monitored voltage level is out of the configured range.
I talked about these in the driver review so look there for comments.
Short summary is this is something userspace should have control off (assuming irq
is wired up) not dt.
> +
> +properties:
> + compatible:
> + const: ti,ads7142
> +
> + reg:
> + maxItems: 1
> +
> + interrupts:
> + description: |
> + The BUSY/PDY pin is used as interrupt line in autonomous monitoring mode.
> + maxItems: 1
> +
> + vref-supply:
> + description: Regulator for the reference voltage
> +
> + power-supply: true
These don't match the naming on the pin diagram.
> +
> + "#address-cells":
> + const: 1
> +
> + "#size-cells":
> + const: 0
> +
> + "#io-channel-cells":
> + const: 1
> +
> + ti,osc-sel:
> + description: |
> + If present, the driver selects the high speed oscillator.
> + See chapter 7.3.5 Oscillator and Timing Control in datasheet.
> + type: boolean
This looks connected to the possible sampling frequencies when in various autonomous modes.
Should it be controlled by userspace?
> +
> + ti,n-clk:
> + description: |
> + nCLK is number of clocks in one conversion cycle.
> + See chapter 7.3.5 Oscillator and Timing Control in datasheet.
Sounds like a policy decision for userspace.
> + $ref: /schemas/types.yaml#/definitions/uint32
> + maximum: 255
> + minimum: 0
> +
> + ti,monitoring-mode:
> + description: |
> + If present, the driver selects the autonomous monitoring mode with pre alert data.
> + See chapter 7.4 Device Functional Modes in datasheet.
As mentioned in the driver review, this looks like something we should control from userspace
not dt to me.
> + type: boolean
> +
> +patternProperties:
> + "^channel@[0-1]$":
> + $ref: "adc.yaml"
> + type: object
> + description: |
> + Represents the external channels which are connected to the ADC.
> + properties:
> + reg:
> + description: |
> + The channel number.
> + items:
> + minimum: 0
> + maximum: 1
> + "ti,threshold-falling":
> + description: The low threshold for channel
For these, we need a strong argument presented in this doc for why they are not
a question of policy (and hence why they should be in dt at all).
> + $ref: /schemas/types.yaml#/definitions/uint32
> + maximum: 4095
> + minimum: 0
> + "ti,threshold-rising":
> + description: The high threshold for channel
> + $ref: /schemas/types.yaml#/definitions/uint32
> + maximum: 4095
> + minimum: 0
> + "ti,hysteresis":
> + description: The hysteresis for both comparators for channel
> + $ref: /schemas/types.yaml#/definitions/uint32
> + maximum: 63
> + minimum: 0
> +
> + required:
> + - reg
> +
> + additionalProperties: false
> +
> +allOf:
> + - if:
> + required:
> + - ti,monitoring-mode
> + then:
> + required:
> + - interrupts
> +
> +required:
> + - compatible
> + - "#io-channel-cells"
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + i2c {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + adc@18 {
I would not bother having two examples. The second one covers more things afterall
and the binding makes it clear what is required.
> + compatible = "ti,ads7142";
> + reg = <0x18>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> + #io-channel-cells = <1>;
> +
> + vref-supply = <&vdd_3v3_reg>;
> + power-supply = <&vdd_1v8_reg>;
> +
> + channel@0 {
> + reg = <0>;
> + };
> +
> + channel@1 {
> + reg = <1>;
> + };
> + };
> + };
> + - |
> + i2c {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + adc@1f {
> + compatible = "ti,ads7142";
> + reg = <0x1f>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + #io-channel-cells = <1>;
> +
> + vref-supply = <&vdd_3v3_reg>;
> + power-supply = <&vdd_1v8_reg>;
> +
> + interrupt-parent = <&gpio>;
> + interrupts = <7 2>;
> +
> + ti,monitoring-mode;
> +
> + channel@0 {
> + reg = <0>;
> + ti,threshold-falling = <1000>;
> + ti,threshold-rising = <2000>;
> + ti,hysteresis = <20>;
> + };
> +
> + channel@1 {
> + reg = <1>;
> + ti,threshold-falling = <100>;
> + ti,threshold-rising = <2500>;
> + ti,hysteresis = <0>;
> + };
> + };
> + };
> +...
> +
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] iio: adc: driver for texas instruments ads7142
2021-05-02 17:14 ` [PATCH 1/2] iio: adc: driver for texas instruments ads7142 Jonathan Cameron
@ 2021-05-02 20:48 ` József Horváth
2021-05-03 10:26 ` Jonathan Cameron
0 siblings, 1 reply; 10+ messages in thread
From: József Horváth @ 2021-05-02 20:48 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Lars-Peter Clausen, Peter Meerwald-Stadler, Rob Herring,
Liam Girdwood, Mark Brown, Alexandru Ardelean, Heiko Stuebner,
Andy Shevchenko, Alex Dewar, Gene Chen, Saravanan Sekar,
Lee Jones, linux-iio, devicetree, linux-kernel
On Sun, May 02, 2021 at 06:14:23PM +0100, Jonathan Cameron wrote:
> On Sat, 1 May 2021 18:24:28 +0000
> Jozsef Horvath <info@ministro.hu> wrote:
>
> > This is an iio driver for
> > Texas Instruments ADS7142 dual-channel, programmable sensor monitor.
> >
> > Operation modes supportedby the driver:
> > When the 'ti,monitoring-mode' property is not present
> > in the devicetree node definition, the driver initiates a single
> > conversion in the device for each read request
> > (/sys/bus/iio/devices/iio:deviceX/in_voltageY_raw).
> > This is a one-shot conversion, and it is called
> > "Manual Mode" in the datasheet.
> >
> > When the 'ti,monitoring-mode' property is present
> > in the devicetree node definition, the driver configures
> > the device's digital window comparator and sets the device's
> > data buffer operation mode to pre alert data mode.
> > The driver reads the conversion result when the BUSY/RDY interrupt
> > fires, and keeps the value until the next BUSY/RDY interrupt
> > or the first read request
> > (/sys/bus/iio/devices/iio:deviceX/in_voltageY_raw).
>
> Hi Jozsef.
>
> Interesting device - somewhat like an impact sensor, but on a general
> purpose ADC.
Yes, but now I'm using as an ADC in my project.
In my point of view this is a general purpose ADC with monitoring features.
>
> Hmm. This sounds rather unintuitive and also very much like a policy
> decision rather than anything to do with the hardware. Hence it
> should almost certainly be in control of userspace and no via
> dt parameters.
>
I think that, this operation modes are not generic enough to bring it to sysfs.
> The interrupt driven nature of the device implies that a polled interface
> such as sysfs is not appropriate to support this mode.
>
> Based on the description you have given here and a quick look
> at the flow charts in the datasheet I would suggest.
> 1) Enable sysfs reads as manual mode only.
> 2) Implement the buffered part of an IIO driver. This is what we use
> for data where autonomous clocking is going on.
I'll check the buffered api.
> 3) Add triggers to represent the different autonomous modes. In some
> sense all the modes present can be considered be a series of
> 'capture now' signals that are being generated by the hardware in
> response to some event'.
>
> So you'd have a pre_alert_tigger, post_alert_trigger
> Stop_burst and start_burst are more interesting to handle because you
> will need something to actually start/stop them. These could be done
> via a sysfs attribute for the trigger, or more complex schemes exist
> such as triggering them off another trigger... one or two of the SoC
> ADCs do that sort of thing.
>
>
> > The digital window comparator and hysteresis parameters
> > can be controlled by:
> > - the devicetree definition of channel node
> > - iio sysfs interfaces
> > This is event driven conversion, and is called
> > "Autonomous Mode with Pre Alert Data" in the datasheet.
> > This mode can be used to wake up the system with the ALERT pin,
> > in case when the monitored voltage level is out of the configured range.
>
> Whilst it's fine to only enable the modes you want, we should think about how
> to ensure other modes can be supported.
>
As I described above, I would keep the operation modes in dt, and
'ti,monitoring-mode' can be an enum.
> >
> > Datasheet: https://www.ti.com/lit/ds/symlink/ads7142.pdf
> >
> > Signed-off-by: Jozsef Horvath <info@ministro.hu>
> > ---
> Should only be one ---
>
> comments inline.
> > ---
> > MAINTAINERS | 6 +
> > drivers/iio/adc/Kconfig | 10 +
> > drivers/iio/adc/Makefile | 1 +
> >
> > +config TI_ADS7142
> > + tristate "Texas Instruments ADS7142 ADC driver"
> > + depends on I2C
> > + help
> > + This driver is for Texas Instruments ADS7142 Nanopower, Dual-Channel, Programmable Sensor Monitor.
>
> Please keep to shorter lines in Kconfig files < 80 chars preferred.
Ok, you are right.
>
> > + Say 'Y' here if you wish to use it.
> > +
> > + To compile this driver as a module, choose M here: the
> > + module will be called ti-ads7142.
> > + * Copyright (C) 2020 Jozsef Horvath <info@ministro.hu>
> > + *
> blank line not needed, plus maybe 2021 given when you are posting it?
>
> > +}
> > +
> > +static int ti_ads7142_reg_read(const struct i2c_client *client, u8 reg,
> > + u8 *data)
> > +{
> > + struct i2c_msg msg[2];
> Use c99 assignment to do this as something like.
>
> struct i2c_msg msg[2] = {
> {
> .addr = client->addr,
> .len = 2,
> .buf = buf,
> }, {
> .addr = client->addr,
> .flags = I2C_M_RD,
> .len = 1,
> .buf = data,
> }
> } ;
> > + u8 buf[2];
>
> u8 buf[2] = { TI_..., reg };
>
Ok, you are right
> > +static int ti_ads7142_data_buffer_read(const struct i2c_client *client,
> > + int length, void *data)
> > +{
> > + struct i2c_msg msg;
> > + int ret;
> > +
> > + msg.addr = client->addr;
> > + msg.flags = I2C_M_RD;
> > + msg.len = length;
> > + msg.buf = data;
> > +
> > + ret = i2c_transfer(client->adapter, &msg, 1);
>
> Looks very similar to an i2c_smbus_read_block_data call though I suppose it
> is a little odd to use mixture of smbus and non in one driver.
>
I would use i2c_transfer, or i2c_master_recv could better.
> > +static int ti_ads7142_soft_reset(const struct i2c_client *client)
> > +{
> > + struct i2c_msg msg;
> > + u8 buf[2];
> u8 buf[2] = { TI_ADS7142_OC_GENERAL, 0x06 }; is more compact for no loss
> of readability.
>
Ok, I'll do that
> > + int ret;
> > +
> > + buf[0] = TI_ADS7142_OC_GENERAL;
> > + buf[1] = 0x06;
> > +
> > + msg.addr = client->addr;
> > + msg.flags = 0;
> > + msg.len = 2;
> > + msg.buf = buf;
> > +
> > + ret = i2c_transfer(client->adapter, &msg, 1);
>
> i2c_master_send() or even better isn't this just an open coded
> i2c_smbus_write_byte_data()
You are right, I'll change to i2c_master_send
>
>
> > +
> > + return ret >= 0 ? 0 : ret;
>
> if ret == 0 then something went wrong and we should report that.
You are right
> > + channel->data.value = value;
> > + *channel_collected |= 1 << channel_address;
> > + }
> > + }
> > + } while (--data_buffer_status);
> > +
> > + return ret;
> > +}
> > +
> > +static int ti_ads7142_do_work(struct iio_dev *indio_dev)
>
> As mentioned below, these function needs a more informative name.
I'll change it to ..._do_monitoring_work, and create something like
start_pre_alert_monitoring, start_post_alert_monitoring, etc
> > +static int ti_ads7142_read_channel_manual(struct iio_dev *indio_dev,
> > + int address, int *val)
> > +{
> > + struct ti_ads7142_priv *priv = iio_priv(indio_dev);
> > + struct i2c_client *client = to_i2c_client(indio_dev->dev.parent);
> > + u16 data_buffer;
> > + int ret;
> > +
> > + if (address < 0 || address > 1)
>
> I'm assuming there is no way we could get here with this not being true?
> If so drop it. If it is possible, then add a comment as it seems like
> an odd thing to need to check.
>
I'ts get called by iio_info.read_raw, so if it's not require to check, I'll remove it.
> > +static int ti_ads7142_read_channel_monitor(struct iio_dev *indio_dev,
> > + int address, int *val)
> > +{
> > + struct ti_ads7142_priv *priv = iio_priv(indio_dev);
> > + struct ti_ads7142_channel *channel;
> > + int ret;
> > +
> > + if (address < 0 || address > 1)
> > + return -EINVAL;
> > +
> > + ret = ti_ads7142_address2channel(indio_dev, address, &channel);
> > + if (ret)
> > + return ret;
> > +
> > + mutex_lock(&priv->lock);
> > + if (!channel->data.status) {
> > + ret = -EAGAIN;
> > + } else {
> > + *val = channel->data.value;
> > + channel->data.status = 0;
> > + ret = 0;
>
> ret already is 0 so no need to set it.
You are right.
> > +static irqreturn_t ti_ads7142_ist(int irq, void *dev_id)
> > +{
> > + struct iio_dev *indio_dev = dev_id;
> > + struct i2c_client *client = to_i2c_client(indio_dev->dev.parent);
> > + struct ti_ads7142_priv *priv = iio_priv(indio_dev);
> > + struct ti_ads7142_channel *channel;
> > + u8 low_flags;
> > + u8 high_flags;
> > + u8 seq_st;
> > + int i;
> > + int ret;
> > + int channel_collected;
> > + s64 timestamp = iio_get_time_ns(indio_dev);
> > +
> > + mutex_lock(&priv->lock);
> > + if (!priv->config.monitoring_mode || !priv->monitor_pending) {
> > + mutex_unlock(&priv->lock);
> > + return IRQ_NONE;
> > + }
> > +
> > + ret = ti_ads7142_reg_read(client, TI_ADS7142_SEQUENCE_STATUS, &seq_st);
> > + if (ret) {
> > + dev_err(indio_dev->dev.parent,
> > + "%s: SEQUENCE_STATUS reg read error(%i)",
> > + __func__, ret);
> > + goto final;
> > + }
> > +
> > + if ((seq_st & TI_ADS7142_SEQUENCE_STATUS_SEQ_ERR_ST_MSK)
> > + != TI_ADS7142_SEQUENCE_STATUS_SEQ_ENABLED) {
> > + dev_err(indio_dev->dev.parent,
> > + "%s: SEQUENCE_STATUS error(%i)",
> > + __func__, seq_st);
> > + goto final;
> > + }
> > +
> > + ret = ti_ads7142_reg_read(client, TI_ADS7142_ALT_LOW_FLAGS,
> > + &low_flags);
> > + if (ret) {
> > + dev_err(indio_dev->dev.parent,
> > + "%s: ALT_LOW_FLAGS reg read error(%i)",
> > + __func__, ret);
> > + goto final;
> > + }
> > +
> > + ret = ti_ads7142_reg_read(client, TI_ADS7142_ALT_HIGH_FLAGS,
> > + &high_flags);
> > + if (ret) {
> > + dev_err(indio_dev->dev.parent,
> > + "%s: ALT_HIGH_FLAGS reg read error(%i)",
> > + __func__, ret);
> > + goto final;
> > + }
> > +
> > + channel_collected = 0;
> > + ret = ti_ads7142_collect_channel_data(indio_dev, &channel_collected);
> > + if (ret)
> > + goto final;
> > +
> > + if (!channel_collected)
> > + goto final;
> > +
> > + for (i = 0; i < priv->channel_count; i++) {
> > + channel = &priv->channels[i];
> > + if (!(channel_collected & (1 << channel->channel)))
> > + continue;
>
> Perhaps use a for_each_bit_set(i, channel_collected) to simplify this.
>
I'll check it. Thank you.
> > +static int ti_ads7142_read_raw(struct iio_dev *indio_dev,
> > + struct iio_chan_spec const *chan,
> > + int *val, int *val2, long info)
> > +{
> > + struct ti_ads7142_priv *priv = iio_priv(indio_dev);
> > + int ret;
> > +
> > + switch (info) {
> > + case IIO_CHAN_INFO_RAW:
> > + ret = ti_ads7142_read_channel(indio_dev, chan->address, val);
> > + if (!ret)
>
> if (ret)
> return ret;
>
> return IIO_VAL_INT;
>
> Always have error cases out of line. That consistency makes
> it easier to review.
>
I dont like 'return' from 'case', but I can live with this. I'll change it.
> > +static int ti_ads7142_write_raw(struct iio_dev *indio_dev,
> > + struct iio_chan_spec const *chan,
> > + int val, int val2, long mask)
> > +{
> > + struct ti_ads7142_priv *priv = iio_priv(indio_dev);
> > + int ret;
> > +
> > + switch (mask) {
> > + case IIO_CHAN_INFO_SAMP_FREQ:
> > + priv->config.n_clk = val;
> > + if (priv->config.monitoring_mode)
> > + ret = ti_ads7142_do_work(indio_dev);
> return ti_...
>
> Early returns almost always easier to read.
> Note applies to lots of stuff above.
>
Ok, I'll fix it all
> > +static int ti_ads7142_read_event_config(struct iio_dev *indio_dev,
> > + const struct iio_chan_spec *chan,
> > + enum iio_event_type type,
> > + enum iio_event_direction dir)
> > +{
> > + struct ti_ads7142_priv *priv = iio_priv(indio_dev);
> > + struct ti_ads7142_channel *channel;
> > + int ret;
> > +
> > + if (!priv->config.monitoring_mode)
> > + return -EINVAL;
> > +
> > + if (type != IIO_EV_TYPE_THRESH)
> > + return -EINVAL;
> > +
> > + ret = ti_ads7142_address2channel(indio_dev, chan->address,
> > + &channel);
> > + if (ret)
> > + return ret;
> > +
> > + if (dir == IIO_EV_DIR_RISING)
> > + ret = channel->config.alert_high ? 1 : 0;
>
> Not fine using ret = channel->config.alert_high; directly?
>
alert_high is bool, ret is int.
I know, the 'true' value is 1, and its autmatically casted,
but who knows the future...I would keep this, if possible.
> > +static int ti_ads7142_write_event_config(struct iio_dev *indio_dev,
> > + const struct iio_chan_spec *chan,
> > + enum iio_event_type type,
> > + enum iio_event_direction dir,
> > + int state)
> > +{
> > + struct ti_ads7142_priv *priv = iio_priv(indio_dev);
> > + struct ti_ads7142_channel *channel;
> > + bool have_to_do = false;
> > + int ret;
> > +
> > + if (!priv->config.monitoring_mode)
> > + return -EINVAL;
> > +
> > + if (type != IIO_EV_TYPE_THRESH)
> > + return -EINVAL;
> > +
> > + ret = ti_ads7142_address2channel(indio_dev, chan->address,
> > + &channel);
> > + if (ret)
> > + return ret;
> > +
> > + mutex_lock(&priv->lock);
> > + if (dir == IIO_EV_DIR_RISING) {
> > + if (channel->config.alert_high != state) {
> > + channel->config.alert_high = state;
> > + have_to_do = true;
> > + }
> > + } else {
> > + if (channel->config.alert_low != state) {
> > + channel->config.alert_low = state;
> > + have_to_do = true;
> > + }
> > + }
> > + mutex_unlock(&priv->lock);
> > +
> > + if (have_to_do)
> > + ret = ti_ads7142_do_work(indio_dev);
> that's going to need a better name as I have no idea what _do_work implies.
>
> > +
> > + return ret;
> > +}
> > +
> > +static const struct iio_info ti_ads7142_iio_info = {
>
> If interrupt is going to be optional, pick between two versions of struct iio_info
> so that we don't provide callbacks for the case where we have no interrupts.
>
Ok, I'll do that.
> > + .read_raw = ti_ads7142_read_raw,
> > + .write_raw = ti_ads7142_write_raw,
> > + .read_event_value = ti_ads7142_read_event_value,
> > + .write_event_value = ti_ads7142_write_event_value,
> > + .read_event_config = ti_ads7142_read_event_config,
> > + .write_event_config = ti_ads7142_write_event_config,
> > +};
> > +
> > +static int ti_ads7142_parse_channel_config_of(struct device *dev,
> > + struct iio_dev *indio_dev)
> > +{
> > + struct ti_ads7142_priv *priv = iio_priv(indio_dev);
> > + struct device_node *channel_node;
> > + struct iio_chan_spec *iio_channels;
> > + struct iio_chan_spec *iio_channel;
> > + struct ti_ads7142_channel *ads_channel;
> > + int channel_index = 0;
> > + int ret;
> > +
> > + priv->channel_count = of_get_available_child_count(dev->of_node);
> > + if (!priv->channel_count) {
> > + dev_err(dev, "dt: there is no channel definition");
> > + return -ENODEV;
> > + }
> > +
> > + priv->channels = devm_kcalloc(dev, priv->channel_count,
> > + sizeof(*priv->channels),
> > + GFP_KERNEL);
> > + if (!priv->channels)
> > + return -ENOMEM;
> > +
> > + indio_dev->num_channels = priv->channel_count;
>
> Why do you need that in two places?
>
You are right this is redundancy.
> > + iio_channels = devm_kcalloc(dev, priv->channel_count,
> > + sizeof(*iio_channels),
> > + GFP_KERNEL);
> > + if (!iio_channels)
> > + return -ENOMEM;
> > +
> > + indio_dev->channels = iio_channels;
> > +
> > + for_each_available_child_of_node(dev->of_node, channel_node) {
> > + ads_channel = &priv->channels[channel_index];
> > +
> > + ret = of_property_read_u32(channel_node, "reg",
> > + &ads_channel->channel);
> > + if (ret)
> > + goto err;
> > +
> > + iio_channel = &iio_channels[channel_index];
> > + iio_channel->type = IIO_VOLTAGE;
> > + iio_channel->indexed = 1;
> > + iio_channel->info_mask_separate = BIT(IIO_CHAN_INFO_RAW)
> > + | BIT(IIO_CHAN_INFO_SAMP_FREQ);
> > + if (!IS_ERR(priv->vref))
> Ah. This somewhat explains the optional regulator.
> We have done this in a few old drivers because they initially were missing vref support
> and we couldn't break existing device tree bindings. I'm not keen to see it
> done for a new driver. Just make the vref-supply required.
> If it's a fixed voltage that is always on, then the device tree additions are
> trivial anyway.
>
Ok, I'll do that.
> > + iio_channel->info_mask_separate |= BIT(IIO_CHAN_INFO_SCALE);
> > + iio_channel->scan_type.sign = 'u';
> > + iio_channel->scan_type.realbits = 12;
> > + iio_channel->scan_type.storagebits = 16;
> > + iio_channel->scan_type.shift = 0;
>
> No need to specify obvious default shift of 0. Rely on the zeroed allocation.
>
> > + iio_channel->scan_type.endianness = IIO_CPU;
> > + iio_channel->address = ads_channel->channel;
> > + iio_channel->scan_index = ads_channel->channel;
> > + iio_channel->channel = ads_channel->channel;
> > + if (priv->config.monitoring_mode) {
> > + iio_channel->event_spec = ti_ads7142_events;
> > + iio_channel->num_event_specs = ARRAY_SIZE(ti_ads7142_events);
> > + }
> > +
> > + ads_channel->config.high_threshold = TI_ADS7142_THRESHOLD_MSK;
> > + ret = of_property_read_u32(channel_node, "ti,threshold-rising",
>
> These are usually considered a matter of userspace policy only. Will need a strong
> argument for them to be in DT.
Ok, I'll remove this from dt.
>
> > + &ads_channel->config.high_threshold);
> > + ads_channel->config.alert_high = !ret;
> > + ret = of_property_read_u32(channel_node, "ti,threshold-falling",
> > + &ads_channel->config.low_threshold);
> > + ads_channel->config.alert_low = !ret;
> > + ret = of_property_read_u32(channel_node, "ti,hysteresis",
> > + &ads_channel->config.hysteresis);
> > + channel_index++;
> > + }
> > +
> > + return 0;
> > +err:
> > + of_node_put(channel_node);
> > + return ret;
> > +}
> > +
> > +static int ti_ads7142_parse_config_of(struct device *dev,
> > + struct iio_dev *indio_dev)
> > +{
> > + struct ti_ads7142_priv *priv = iio_priv(indio_dev);
> > +
> > + priv->config.osc_sel = of_property_read_bool(dev->of_node,
> > + "ti,osc-sel");
>
> Please use generic device property access functions where possible.
> That basically gives us support on non OF based platforms for free.
Could you please explain this, I dont understand.
>
> > + of_property_read_u32(dev->of_node, "ti,n-clk", &priv->config.n_clk);
> > + priv->config.monitoring_mode = of_property_read_bool(dev->of_node,
> > + "ti,monitoring-mode");
> > +
> > + return ti_ads7142_parse_channel_config_of(dev, indio_dev);
> > +}
> > +
> > +static int ti_ads7142_probe(struct i2c_client *client,
> > + const struct i2c_device_id *id)
> > +{
> > + struct iio_dev *indio_dev;
> > + struct ti_ads7142_priv *priv;
> > + int ret;
> > +
> > + ret = ti_ads7142_soft_reset(client);
> > + if (ret)
> > + return ret;
> > +
> > + indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*priv));
> > + if (!indio_dev)
> > + return -ENOMEM;
> > +
> > + priv = iio_priv(indio_dev);
> > + i2c_set_clientdata(client, indio_dev);
> > +
> > + indio_dev->dev.parent = &client->dev;
> > + indio_dev->dev.of_node = client->dev.of_node;
>
> Both of the above are set by the iio core, so no need to set her.
Ok, thank you, I'll remove it.
>
> > + indio_dev->name = TI_ADS7142_NAME;
> > + indio_dev->modes = INDIO_DIRECT_MODE;
> > + indio_dev->info = &ti_ads7142_iio_info;
> > +
> > + mutex_init(&priv->lock);
> > +
> > + priv->vref = devm_regulator_get_optional(&client->dev, "vref");
>
> There isn't a vref pin on the device. Vref is used in the datasheet
> but the pinout makes it clear it is actually avdd which is definitely
> not optional.
You are right, avdd is a better name.
>
> > + if (!IS_ERR(priv->vref)) {
> > + ret = regulator_enable(priv->vref);
> > + if (ret)
> > + goto err;
> As you have simple handling of power in here, I would definitely look
> to use managed calls to disable the regulators.
>
> devm_add_action_or_reset() is usual way of doing this.
>
> If nothing else it will ensure that the unwind on removal is the mirror
> image of the setup on probe() and hence it is much easier to avoid any
> subtle race conditions.
Very good suggestion, thank you.
>
> > + }
> > +
> > + priv->power = devm_regulator_get_optional(&client->dev, "power");
>
> dvdd? Not sure what else power could be. Note that you should only
> use _get_optional() for regulators that really optional and may not be
> connected. For cases where you simply can't control them and they aren't
> specified in the dts then devm_regulator_get() will supply a stub regulator
> that is always on.
>
Ok, I'll change it to dvdd and required property
>
> > + if (!IS_ERR(priv->power)) {
> > + ret = regulator_enable(priv->power);
> > + if (ret)
> > + goto err_regulator;
> > + }
> > +
> > + ret = ti_ads7142_parse_config_of(&client->dev, indio_dev);
> > + if (ret)
> > + goto err_regulator;
> > +
> > + if (!client->irq && priv->config.monitoring_mode) {
>
> Given you can check this before you have to do any unwinding, move it
> earlier.
Ok, I'll do this.
>
> > + ret = -EINVAL;
> > + dev_err(&client->dev, "Interrupt not specified\n");
> > + goto err_regulator;
> > + }
> > + if (client->irq && priv->config.monitoring_mode) {
> > + ret = devm_request_threaded_irq(&client->dev, client->irq,
> > + NULL, ti_ads7142_ist,
> > + IRQF_ONESHOT | IRQF_SHARED,
> > + dev_name(&client->dev),
> > + indio_dev);
> > + if (ret) {
> > + dev_err(&client->dev, "Unable to request IRQ %i",
> > + client->irq);
> > + goto err_regulator;
> > + }
> > + }
> > +
> > + ret = iio_device_register(indio_dev);
> > + if (ret) {
> > + dev_err(&client->dev, "Failed to register iio device");
> > + goto err_regulator;
> > + }
> > +
> > + ret = ti_ads7142_do_work(indio_dev);
>
> As iio_device_register() is responsible for exposing userspace interfaces it
> is very rarely a good idea to do anything after it in probe()
> (some exceptions for runtime pm autosuspend setup as that can kick in later)
>
Ok, I'll move it earlier.
> > + if (!ret) {
> > + dev_info(&client->dev, "%s is a %s device at address 0x%X",
> > + dev_name(&indio_dev->dev), indio_dev->name,
> > + client->addr);
> > + return ret;
> Always have the error path out of line as it is much more consistent
> with what reviewers are expecting
>
> if (ret)
> goto error_unregister()
>
> return 0;
>
> > + }
> > +
> > + iio_device_unregister(indio_dev);
>
>
>
> > +
> > +err_regulator:
> > + if (!IS_ERR(priv->vref))
> > + regulator_disable(priv->vref);
> > + if (!IS_ERR(priv->power))
> > + regulator_disable(priv->power);
> > +err:
> > + mutex_destroy(&priv->lock);
> We rarely bother with mutex_destroy() in paths where the driver is being
> removed as it is only useful for debugging use of mutexes after they have
> been removed. That doesn't happen in these paths so it tends to just be
> noise to have a mutex_destroy(). This function is useful if you have a
> mutex inside something with a different lifespan from the device and
> like to enable mutex debugging.
>
Ok, I'll remove it, thank you for the explanation.
> > +
> > + return ret;
> > +}
> > +
> > +static int ti_ads7142_remove(struct i2c_client *client)
> > +{
> > + struct iio_dev *indio_dev = i2c_get_clientdata(client);
> > + struct ti_ads7142_priv *priv = iio_priv(indio_dev);
> > +
> > + if (!IS_ERR(priv->vref))
> > + regulator_disable(priv->vref);
> If you'd not used _optional() above you could do these without worrying
> about whether the regulator was there or not.
>
> > + if (!IS_ERR(priv->power))
> > + regulator_disable(priv->power);
> > + mutex_destroy(&priv->lock);
> > + iio_device_unregister(indio_dev);
>
> _remove should always look like the unwinding of probe - so
> I should be able to match items in reverse order.
Ok, I check it.
Thank you for the review and suggestions.
Best regards
Jozsef
>
> > +
> > + return 0;
> > +}
> > +
> > +module_i2c_driver(ti_ads7142_driver);
> > +
> > +MODULE_LICENSE("GPL");
> > +MODULE_AUTHOR("Jozsef Horvath <info@ministro.hu>");
> > +MODULE_DESCRIPTION("Texas Instruments TI_ADS7142 ADC driver");
>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] dt-bindings: iio: adc: devicetree bindings for texas instruments ads7142 iio driver
2021-05-02 17:22 ` Jonathan Cameron
@ 2021-05-02 21:10 ` József Horváth
2021-05-03 10:30 ` Jonathan Cameron
0 siblings, 1 reply; 10+ messages in thread
From: József Horváth @ 2021-05-02 21:10 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Lars-Peter Clausen, Peter Meerwald-Stadler, Rob Herring,
Liam Girdwood, Mark Brown, Heiko Stuebner, Andy Shevchenko,
Alex Dewar, Gene Chen, Saravanan Sekar, Lee Jones, linux-iio,
devicetree, linux-kernel
On Sun, May 02, 2021 at 06:22:55PM +0100, Jonathan Cameron wrote:
> On Sat, 1 May 2021 18:25:18 +0000
> Jozsef Horvath <info@ministro.hu> wrote:
>
> > This is a device tree schema for iio driver for
> > Texas Instruments ADS7142 dual-channel, programmable sensor monitor.
> >
> > Datasheet: https://www.ti.com/lit/ds/symlink/ads7142.pdf
> >
> > Signed-off-by: Jozsef Horvath <info@ministro.hu>
> > ---
> > ---
> > .../bindings/iio/adc/ti,ads7142.yaml | 198 ++++++++++++++++++
> > 1 file changed, 198 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/iio/adc/ti,ads7142.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/iio/adc/ti,ads7142.yaml b/Documentation/devicetree/bindings/iio/adc/ti,ads7142.yaml
> > new file mode 100644
> > index 000000000000..b4e752160156
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/adc/ti,ads7142.yaml
> > @@ -0,0 +1,198 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: "http://devicetree.org/schemas/iio/adc/ti,ads7142.yaml#"
> > +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
> > +
> > +title: Texas Instruments ADS7142 adc driver device tree bindings
> > +
> > +maintainers:
> > + - József Horváth <info@ministro.hu>
> > +
> > +description: |
> > + This document is for describing the required device tree parameters
> > + for ads7142 adc driver.
>
> Document describes hardware, not a particular driver. So just refer
> to the device here. There may well be other drives in future using
> the same binding (e.g. in an RTOS).
>
> > + The required parameters for proper operation are described below.
> > +
> > + Datasheet: https://www.ti.com/lit/ds/symlink/ads7142.pdf
> > +
> > + Operation modes supportedby the driver:
> > + When the 'ti,monitoring-mode' property is not present
> > + in the devicetree node definition, the driver initiates a single
> > + conversion in the device for each read request
> > + (/sys/bus/iio/devices/iio:deviceX/in_voltageY_raw).
> > + This is a one-shot conversion, and it is called
> > + "Manual Mode" in the datasheet.
> > +
> > + When the 'ti,monitoring-mode' property is present
> > + in the devicetree node definition, the driver configures
> > + the device's digital window comparator and sets the device's
> > + data buffer operation mode to pre alert data mode.
> > + The driver reads the conversion result when the BUSY/RDY interrupt
> > + fires, and keeps the value until the next BUSY/RDY interrupt
> > + or the first read request
> > + (/sys/bus/iio/devices/iio:deviceX/in_voltageY_raw).
> > + The digital window comparator and hysteresis parameters
> > + can be controlled by:
> > + - the devicetree definition of channel node
> > + - iio sysfs interfaces
> > + This is event driven conversion, and is called
> > + "Autonomous Mode with Pre Alert Data" in the datasheet.
> > + This mode can be used to wake up the system with the ALERT pin,
> > + in case when the monitored voltage level is out of the configured range.
>
> I talked about these in the driver review so look there for comments.
> Short summary is this is something userspace should have control off (assuming irq
> is wired up) not dt.
>
> > +
> > +properties:
> > + compatible:
> > + const: ti,ads7142
> > +
> > + reg:
> > + maxItems: 1
> > +
> > + interrupts:
> > + description: |
> > + The BUSY/PDY pin is used as interrupt line in autonomous monitoring mode.
> > + maxItems: 1
> > +
> > + vref-supply:
> > + description: Regulator for the reference voltage
> > +
> > + power-supply: true
>
> These don't match the naming on the pin diagram.
I'll cange it to dvdd, and vref to avdd.
>
> > +
> > + "#address-cells":
> > + const: 1
> > +
> > + "#size-cells":
> > + const: 0
> > +
> > + "#io-channel-cells":
> > + const: 1
> > +
> > + ti,osc-sel:
> > + description: |
> > + If present, the driver selects the high speed oscillator.
> > + See chapter 7.3.5 Oscillator and Timing Control in datasheet.
> > + type: boolean
>
> This looks connected to the possible sampling frequencies when in various autonomous modes.
> Should it be controlled by userspace?
The sampling frequency is controlled with the osc-sel and n-clk.
I'll remove n-clk from sysfs.
>
> > +
> > + ti,n-clk:
> > + description: |
> > + nCLK is number of clocks in one conversion cycle.
> > + See chapter 7.3.5 Oscillator and Timing Control in datasheet.
>
> Sounds like a policy decision for userspace.
>
> > + $ref: /schemas/types.yaml#/definitions/uint32
> > + maximum: 255
> > + minimum: 0
> > +
> > + ti,monitoring-mode:
> > + description: |
> > + If present, the driver selects the autonomous monitoring mode with pre alert data.
> > + See chapter 7.4 Device Functional Modes in datasheet.
>
> As mentioned in the driver review, this looks like something we should control from userspace
> not dt to me.
>
I would keep this here, but it will be an enum.
> > + type: boolean
> > +
> > +patternProperties:
> > + "^channel@[0-1]$":
> > + $ref: "adc.yaml"
> > + type: object
> > + description: |
> > + Represents the external channels which are connected to the ADC.
> > + properties:
> > + reg:
> > + description: |
> > + The channel number.
> > + items:
> > + minimum: 0
> > + maximum: 1
> > + "ti,threshold-falling":
> > + description: The low threshold for channel
>
> For these, we need a strong argument presented in this doc for why they are not
> a question of policy (and hence why they should be in dt at all).
I'll remove all threshold and hysteresys from dt.
>
> > + $ref: /schemas/types.yaml#/definitions/uint32
> > + maximum: 4095
> > + minimum: 0
> > + "ti,threshold-rising":
> > + description: The high threshold for channel
> > + $ref: /schemas/types.yaml#/definitions/uint32
> > + maximum: 4095
> > + minimum: 0
> > + "ti,hysteresis":
> > + description: The hysteresis for both comparators for channel
> > + $ref: /schemas/types.yaml#/definitions/uint32
> > + maximum: 63
> > + minimum: 0
> > +
> > + required:
> > + - reg
> > +
> > + additionalProperties: false
> > +
> > +allOf:
> > + - if:
> > + required:
> > + - ti,monitoring-mode
> > + then:
> > + required:
> > + - interrupts
> > +
> > +required:
> > + - compatible
> > + - "#io-channel-cells"
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > + - |
> > + i2c {
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > + adc@18 {
>
> I would not bother having two examples. The second one covers more things afterall
> and the binding makes it clear what is required.
>
I do this because of the conditional requirement of interrupts.
> > + compatible = "ti,ads7142";
> > + reg = <0x18>;
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > + #io-channel-cells = <1>;
> > +
> > + vref-supply = <&vdd_3v3_reg>;
> > + power-supply = <&vdd_1v8_reg>;
> > +
> > + channel@0 {
> > + reg = <0>;
> > + };
> > +
> > + channel@1 {
> > + reg = <1>;
> > + };
> > + };
> > + };
> > + - |
> > + i2c {
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > + adc@1f {
> > + compatible = "ti,ads7142";
> > + reg = <0x1f>;
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > +
> > + #io-channel-cells = <1>;
> > +
> > + vref-supply = <&vdd_3v3_reg>;
> > + power-supply = <&vdd_1v8_reg>;
> > +
> > + interrupt-parent = <&gpio>;
> > + interrupts = <7 2>;
> > +
> > + ti,monitoring-mode;
> > +
> > + channel@0 {
> > + reg = <0>;
> > + ti,threshold-falling = <1000>;
> > + ti,threshold-rising = <2000>;
> > + ti,hysteresis = <20>;
> > + };
> > +
> > + channel@1 {
> > + reg = <1>;
> > + ti,threshold-falling = <100>;
> > + ti,threshold-rising = <2500>;
> > + ti,hysteresis = <0>;
> > + };
> > + };
> > + };
> > +...
> > +
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] iio: adc: driver for texas instruments ads7142
2021-05-02 20:48 ` József Horváth
@ 2021-05-03 10:26 ` Jonathan Cameron
2021-05-03 19:33 ` József Horváth
0 siblings, 1 reply; 10+ messages in thread
From: Jonathan Cameron @ 2021-05-03 10:26 UTC (permalink / raw)
To: József Horváth
Cc: Lars-Peter Clausen, Peter Meerwald-Stadler, Rob Herring,
Liam Girdwood, Mark Brown, Alexandru Ardelean, Heiko Stuebner,
Andy Shevchenko, Alex Dewar, Gene Chen, Saravanan Sekar,
Lee Jones, linux-iio, devicetree, linux-kernel
On Sun, 2 May 2021 20:48:47 +0000
József Horváth <info@ministro.hu> wrote:
> On Sun, May 02, 2021 at 06:14:23PM +0100, Jonathan Cameron wrote:
> > On Sat, 1 May 2021 18:24:28 +0000
> > Jozsef Horvath <info@ministro.hu> wrote:
> >
> > > This is an iio driver for
> > > Texas Instruments ADS7142 dual-channel, programmable sensor monitor.
> > >
> > > Operation modes supportedby the driver:
> > > When the 'ti,monitoring-mode' property is not present
> > > in the devicetree node definition, the driver initiates a single
> > > conversion in the device for each read request
> > > (/sys/bus/iio/devices/iio:deviceX/in_voltageY_raw).
> > > This is a one-shot conversion, and it is called
> > > "Manual Mode" in the datasheet.
> > >
> > > When the 'ti,monitoring-mode' property is present
> > > in the devicetree node definition, the driver configures
> > > the device's digital window comparator and sets the device's
> > > data buffer operation mode to pre alert data mode.
> > > The driver reads the conversion result when the BUSY/RDY interrupt
> > > fires, and keeps the value until the next BUSY/RDY interrupt
> > > or the first read request
> > > (/sys/bus/iio/devices/iio:deviceX/in_voltageY_raw).
> >
> > Hi Jozsef.
> >
> > Interesting device - somewhat like an impact sensor, but on a general
> > purpose ADC.
>
> Yes, but now I'm using as an ADC in my project.
> In my point of view this is a general purpose ADC with monitoring features.
>
> >
> > Hmm. This sounds rather unintuitive and also very much like a policy
> > decision rather than anything to do with the hardware. Hence it
> > should almost certainly be in control of userspace and no via
> > dt parameters.
> >
>
> I think that, this operation modes are not generic enough to bring it to sysfs.
That just means you need to figure out how to do it, not make it a
boot time, or board flash time based control.
>
> > The interrupt driven nature of the device implies that a polled interface
> > such as sysfs is not appropriate to support this mode.
> >
> > Based on the description you have given here and a quick look
> > at the flow charts in the datasheet I would suggest.
> > 1) Enable sysfs reads as manual mode only.
> > 2) Implement the buffered part of an IIO driver. This is what we use
> > for data where autonomous clocking is going on.
>
> I'll check the buffered api.
>
> > 3) Add triggers to represent the different autonomous modes. In some
> > sense all the modes present can be considered be a series of
> > 'capture now' signals that are being generated by the hardware in
> > response to some event'.
> >
> > So you'd have a pre_alert_tigger, post_alert_trigger
> > Stop_burst and start_burst are more interesting to handle because you
> > will need something to actually start/stop them. These could be done
> > via a sysfs attribute for the trigger, or more complex schemes exist
> > such as triggering them off another trigger... one or two of the SoC
> > ADCs do that sort of thing.
> >
> >
> > > The digital window comparator and hysteresis parameters
> > > can be controlled by:
> > > - the devicetree definition of channel node
> > > - iio sysfs interfaces
> > > This is event driven conversion, and is called
> > > "Autonomous Mode with Pre Alert Data" in the datasheet.
> > > This mode can be used to wake up the system with the ALERT pin,
> > > in case when the monitored voltage level is out of the configured range.
> >
> > Whilst it's fine to only enable the modes you want, we should think about how
> > to ensure other modes can be supported.
> >
>
> As I described above, I would keep the operation modes in dt, and
> 'ti,monitoring-mode' can be an enum.
Sorry but no. Unless you can make a 'very' strong argument of the
fact that this a characteristic of the hardware setup (wiring etc) then
it needs to be userspace controlled.
>
> > >
> > > Datasheet: https://www.ti.com/lit/ds/symlink/ads7142.pdf
> > >
> > > Signed-off-by: Jozsef Horvath <info@ministro.hu>
> > > ---
...
>
> >
> >
> > > +
> > > + return ret >= 0 ? 0 : ret;
> >
> > if ret == 0 then something went wrong and we should report that.
>
> You are right
>
> > > + channel->data.value = value;
> > > + *channel_collected |= 1 << channel_address;
> > > + }
> > > + }
> > > + } while (--data_buffer_status);
> > > +
> > > + return ret;
> > > +}
> > > +
> > > +static int ti_ads7142_do_work(struct iio_dev *indio_dev)
> >
> > As mentioned below, these function needs a more informative name.
>
> I'll change it to ..._do_monitoring_work, and create something like
> start_pre_alert_monitoring, start_post_alert_monitoring, etc
Maybe, but I'd expect 'work' to imply it was the function called on
each cycle of monitoring. This is more monitoring_setup()
...
>
> > > +static int ti_ads7142_read_event_config(struct iio_dev *indio_dev,
> > > + const struct iio_chan_spec *chan,
> > > + enum iio_event_type type,
> > > + enum iio_event_direction dir)
> > > +{
> > > + struct ti_ads7142_priv *priv = iio_priv(indio_dev);
> > > + struct ti_ads7142_channel *channel;
> > > + int ret;
> > > +
> > > + if (!priv->config.monitoring_mode)
> > > + return -EINVAL;
> > > +
> > > + if (type != IIO_EV_TYPE_THRESH)
> > > + return -EINVAL;
> > > +
> > > + ret = ti_ads7142_address2channel(indio_dev, chan->address,
> > > + &channel);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + if (dir == IIO_EV_DIR_RISING)
> > > + ret = channel->config.alert_high ? 1 : 0;
> >
> > Not fine using ret = channel->config.alert_high; directly?
> >
>
> alert_high is bool, ret is int.
> I know, the 'true' value is 1, and its autmatically casted,
> but who knows the future...I would keep this, if possible.
Ok. Assuming the c spec will change to make this invalid is a bit implausible,
but it's not too bad I guess.
>
...
>
> >
> > > + &ads_channel->config.high_threshold);
> > > + ads_channel->config.alert_high = !ret;
> > > + ret = of_property_read_u32(channel_node, "ti,threshold-falling",
> > > + &ads_channel->config.low_threshold);
> > > + ads_channel->config.alert_low = !ret;
> > > + ret = of_property_read_u32(channel_node, "ti,hysteresis",
> > > + &ads_channel->config.hysteresis);
> > > + channel_index++;
> > > + }
> > > +
> > > + return 0;
> > > +err:
> > > + of_node_put(channel_node);
> > > + return ret;
> > > +}
> > > +
> > > +static int ti_ads7142_parse_config_of(struct device *dev,
> > > + struct iio_dev *indio_dev)
> > > +{
> > > + struct ti_ads7142_priv *priv = iio_priv(indio_dev);
> > > +
> > > + priv->config.osc_sel = of_property_read_bool(dev->of_node,
> > > + "ti,osc-sel");
> >
> > Please use generic device property access functions where possible.
> > That basically gives us support on non OF based platforms for free.
>
> Could you please explain this, I dont understand.
See include/linux/property.h
That provides firmware type agnostic property accessors - so they will work
whether the property is provided via ACPI or via DT (or a number of other
options though those are less common).
ACPI DSDT tables can use a special device type PRP0001 which basically is a
wrapper for device tree properties. So the properties are all the same
(including compatible) but you need to use the generic accessors to be able to
read them.
>
> >
> > > + of_property_read_u32(dev->of_node, "ti,n-clk", &priv->config.n_clk);
> > > + priv->config.monitoring_mode = of_property_read_bool(dev->of_node,
> > > + "ti,monitoring-mode");
> > > +
> > > + return ti_ads7142_parse_channel_config_of(dev, indio_dev);
> > > +}
>
...
> Thank you for the review and suggestions.
You are welcome.
Thanks,
Jonathan
>
> Best regards
> Jozsef
>
> >
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +module_i2c_driver(ti_ads7142_driver);
> > > +
> > > +MODULE_LICENSE("GPL");
> > > +MODULE_AUTHOR("Jozsef Horvath <info@ministro.hu>");
> > > +MODULE_DESCRIPTION("Texas Instruments TI_ADS7142 ADC driver");
> >
> >
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] dt-bindings: iio: adc: devicetree bindings for texas instruments ads7142 iio driver
2021-05-02 21:10 ` József Horváth
@ 2021-05-03 10:30 ` Jonathan Cameron
2021-05-03 19:42 ` József Horváth
0 siblings, 1 reply; 10+ messages in thread
From: Jonathan Cameron @ 2021-05-03 10:30 UTC (permalink / raw)
To: József Horváth
Cc: Lars-Peter Clausen, Peter Meerwald-Stadler, Rob Herring,
Liam Girdwood, Mark Brown, Heiko Stuebner, Andy Shevchenko,
Alex Dewar, Gene Chen, Saravanan Sekar, Lee Jones, linux-iio,
devicetree, linux-kernel
On Sun, 2 May 2021 21:10:20 +0000
József Horváth <info@ministro.hu> wrote:
Hi József,
> >
> > > +
> > > + "#address-cells":
> > > + const: 1
> > > +
> > > + "#size-cells":
> > > + const: 0
> > > +
> > > + "#io-channel-cells":
> > > + const: 1
> > > +
> > > + ti,osc-sel:
> > > + description: |
> > > + If present, the driver selects the high speed oscillator.
> > > + See chapter 7.3.5 Oscillator and Timing Control in datasheet.
> > > + type: boolean
> >
> > This looks connected to the possible sampling frequencies when in various autonomous modes.
> > Should it be controlled by userspace?
>
> The sampling frequency is controlled with the osc-sel and n-clk.
> I'll remove n-clk from sysfs.
Not sure I follow that. I think we should only have these controlled via
sysfs. It will be a bit complex as two related controls, but that is a common situation
and there is usually a sensible combination of options that makes sense.
For example, if we can meet the sampling frequency requested with the lower
power oscillator we go for that.
>
> >
> > > +
> > > + ti,n-clk:
> > > + description: |
> > > + nCLK is number of clocks in one conversion cycle.
> > > + See chapter 7.3.5 Oscillator and Timing Control in datasheet.
> >
> > Sounds like a policy decision for userspace.
> >
> > > + $ref: /schemas/types.yaml#/definitions/uint32
> > > + maximum: 255
> > > + minimum: 0
> > > +
> > > + ti,monitoring-mode:
> > > + description: |
> > > + If present, the driver selects the autonomous monitoring mode with pre alert data.
> > > + See chapter 7.4 Device Functional Modes in datasheet.
> >
> > As mentioned in the driver review, this looks like something we should control from userspace
> > not dt to me.
> >
>
> I would keep this here, but it will be an enum.
Sorry, but no. As mentioned in the driver thread, this doesn't sound like
a characteristic of the hardware (board etc) so it doesn't belong in DT.
It may be challenging to implement an interface that makes sense, but
that doesn't mean we can avoid doing it.
>
> > > + type: boolean
> > > +
> > > +patternProperties:
> > > + "^channel@[0-1]$":
> > > + $ref: "adc.yaml"
> > > + type: object
> > > + description: |
> > > + Represents the external channels which are connected to the ADC.
> > > + properties:
> > > + reg:
> > > + description: |
> > > + The channel number.
> > > + items:
> > > + minimum: 0
> > > + maximum: 1
> > > + "ti,threshold-falling":
> > > + description: The low threshold for channel
> >
> > For these, we need a strong argument presented in this doc for why they are not
> > a question of policy (and hence why they should be in dt at all).
>
> I'll remove all threshold and hysteresys from dt.
>
> >
> > > + $ref: /schemas/types.yaml#/definitions/uint32
> > > + maximum: 4095
> > > + minimum: 0
> > > + "ti,threshold-rising":
> > > + description: The high threshold for channel
> > > + $ref: /schemas/types.yaml#/definitions/uint32
> > > + maximum: 4095
> > > + minimum: 0
> > > + "ti,hysteresis":
> > > + description: The hysteresis for both comparators for channel
> > > + $ref: /schemas/types.yaml#/definitions/uint32
> > > + maximum: 63
> > > + minimum: 0
> > > +
> > > + required:
> > > + - reg
> > > +
> > > + additionalProperties: false
> > > +
> > > +allOf:
> > > + - if:
> > > + required:
> > > + - ti,monitoring-mode
> > > + then:
> > > + required:
> > > + - interrupts
> > > +
> > > +required:
> > > + - compatible
> > > + - "#io-channel-cells"
> > > +
> > > +additionalProperties: false
> > > +
> > > +examples:
> > > + - |
> > > + i2c {
> > > + #address-cells = <1>;
> > > + #size-cells = <0>;
> > > + adc@18 {
> >
> > I would not bother having two examples. The second one covers more things afterall
> > and the binding makes it clear what is required.
> >
>
> I do this because of the conditional requirement of interrupts.
Understood, but that is clearly expressed by the 'required' above.
It's easy enough to understand how to not have parts that aren't
required without an example.
>
> > > + compatible = "ti,ads7142";
> > > + reg = <0x18>;
> > > + #address-cells = <1>;
> > > + #size-cells = <0>;
> > > + #io-channel-cells = <1>;
> > > +
> > > + vref-supply = <&vdd_3v3_reg>;
> > > + power-supply = <&vdd_1v8_reg>;
> > > +
> > > + channel@0 {
> > > + reg = <0>;
> > > + };
> > > +
> > > + channel@1 {
> > > + reg = <1>;
> > > + };
> > > + };
> > > + };
> > > + - |
> > > + i2c {
> > > + #address-cells = <1>;
> > > + #size-cells = <0>;
> > > + adc@1f {
> > > + compatible = "ti,ads7142";
> > > + reg = <0x1f>;
> > > + #address-cells = <1>;
> > > + #size-cells = <0>;
> > > +
> > > + #io-channel-cells = <1>;
> > > +
> > > + vref-supply = <&vdd_3v3_reg>;
> > > + power-supply = <&vdd_1v8_reg>;
> > > +
> > > + interrupt-parent = <&gpio>;
> > > + interrupts = <7 2>;
> > > +
> > > + ti,monitoring-mode;
> > > +
> > > + channel@0 {
> > > + reg = <0>;
> > > + ti,threshold-falling = <1000>;
> > > + ti,threshold-rising = <2000>;
> > > + ti,hysteresis = <20>;
> > > + };
> > > +
> > > + channel@1 {
> > > + reg = <1>;
> > > + ti,threshold-falling = <100>;
> > > + ti,threshold-rising = <2500>;
> > > + ti,hysteresis = <0>;
> > > + };
> > > + };
> > > + };
> > > +...
> > > +
> >
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] iio: adc: driver for texas instruments ads7142
2021-05-03 10:26 ` Jonathan Cameron
@ 2021-05-03 19:33 ` József Horváth
0 siblings, 0 replies; 10+ messages in thread
From: József Horváth @ 2021-05-03 19:33 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Lars-Peter Clausen, Peter Meerwald-Stadler, Rob Herring,
Liam Girdwood, Mark Brown, Heiko Stuebner, Andy Shevchenko,
Alex Dewar, Gene Chen, Saravanan Sekar, Lee Jones, linux-iio,
devicetree, linux-kernel
On Mon, May 03, 2021 at 11:26:00AM +0100, Jonathan Cameron wrote:
> On Sun, 2 May 2021 20:48:47 +0000
> József Horváth <info@ministro.hu> wrote:
>
> > On Sun, May 02, 2021 at 06:14:23PM +0100, Jonathan Cameron wrote:
> > > On Sat, 1 May 2021 18:24:28 +0000
> > > Jozsef Horvath <info@ministro.hu> wrote:
> > >
> > > > This is an iio driver for
> > > > Texas Instruments ADS7142 dual-channel, programmable sensor monitor.
> > > >
> > > > Operation modes supportedby the driver:
> > > > When the 'ti,monitoring-mode' property is not present
> > > > in the devicetree node definition, the driver initiates a single
> > > > conversion in the device for each read request
> > > > (/sys/bus/iio/devices/iio:deviceX/in_voltageY_raw).
> > > > This is a one-shot conversion, and it is called
> > > > "Manual Mode" in the datasheet.
> > > >
> > > > When the 'ti,monitoring-mode' property is present
> > > > in the devicetree node definition, the driver configures
> > > > the device's digital window comparator and sets the device's
> > > > data buffer operation mode to pre alert data mode.
> > > > The driver reads the conversion result when the BUSY/RDY interrupt
> > > > fires, and keeps the value until the next BUSY/RDY interrupt
> > > > or the first read request
> > > > (/sys/bus/iio/devices/iio:deviceX/in_voltageY_raw).
> > >
> > > Hi Jozsef.
> > >
> > > Interesting device - somewhat like an impact sensor, but on a general
> > > purpose ADC.
> >
> > Yes, but now I'm using as an ADC in my project.
> > In my point of view this is a general purpose ADC with monitoring features.
> >
> > >
> > > Hmm. This sounds rather unintuitive and also very much like a policy
> > > decision rather than anything to do with the hardware. Hence it
> > > should almost certainly be in control of userspace and no via
> > > dt parameters.
> > >
> >
> > I think that, this operation modes are not generic enough to bring it to sysfs.
>
> That just means you need to figure out how to do it, not make it a
> boot time, or board flash time based control.
I will try to bring it into sysfs.
>
> >
> > > The interrupt driven nature of the device implies that a polled interface
> > > such as sysfs is not appropriate to support this mode.
> > >
> > > Based on the description you have given here and a quick look
> > > at the flow charts in the datasheet I would suggest.
> > > 1) Enable sysfs reads as manual mode only.
> > > 2) Implement the buffered part of an IIO driver. This is what we use
> > > for data where autonomous clocking is going on.
> >
> > I'll check the buffered api.
> >
> > > 3) Add triggers to represent the different autonomous modes. In some
> > > sense all the modes present can be considered be a series of
> > > 'capture now' signals that are being generated by the hardware in
> > > response to some event'.
> > >
> > > So you'd have a pre_alert_tigger, post_alert_trigger
> > > Stop_burst and start_burst are more interesting to handle because you
> > > will need something to actually start/stop them. These could be done
> > > via a sysfs attribute for the trigger, or more complex schemes exist
> > > such as triggering them off another trigger... one or two of the SoC
> > > ADCs do that sort of thing.
> > >
> > >
> > > > The digital window comparator and hysteresis parameters
> > > > can be controlled by:
> > > > - the devicetree definition of channel node
> > > > - iio sysfs interfaces
> > > > This is event driven conversion, and is called
> > > > "Autonomous Mode with Pre Alert Data" in the datasheet.
> > > > This mode can be used to wake up the system with the ALERT pin,
> > > > in case when the monitored voltage level is out of the configured range.
> > >
> > > Whilst it's fine to only enable the modes you want, we should think about how
> > > to ensure other modes can be supported.
> > >
> >
> > As I described above, I would keep the operation modes in dt, and
> > 'ti,monitoring-mode' can be an enum.
>
> Sorry but no. Unless you can make a 'very' strong argument of the
> fact that this a characteristic of the hardware setup (wiring etc) then
> it needs to be userspace controlled.
>
Ok, I'll come back a possible solution soon.
> >
> > > >
> > > > Datasheet: https://www.ti.com/lit/ds/symlink/ads7142.pdf
> > > >
> > > > Signed-off-by: Jozsef Horvath <info@ministro.hu>
> > > > ---
>
> ...
>
> >
> > >
> > >
> > > > +
> > > > + return ret >= 0 ? 0 : ret;
> > >
> > > if ret == 0 then something went wrong and we should report that.
> >
> > You are right
> >
> > > > + channel->data.value = value;
> > > > + *channel_collected |= 1 << channel_address;
> > > > + }
> > > > + }
> > > > + } while (--data_buffer_status);
> > > > +
> > > > + return ret;
> > > > +}
> > > > +
> > > > +static int ti_ads7142_do_work(struct iio_dev *indio_dev)
> > >
> > > As mentioned below, these function needs a more informative name.
> >
> > I'll change it to ..._do_monitoring_work, and create something like
> > start_pre_alert_monitoring, start_post_alert_monitoring, etc
>
> Maybe, but I'd expect 'work' to imply it was the function called on
> each cycle of monitoring. This is more monitoring_setup()
>
'..._setup' is ok for me.
>
> ...
>
> >
> > > > +static int ti_ads7142_read_event_config(struct iio_dev *indio_dev,
> > > > + const struct iio_chan_spec *chan,
> > > > + enum iio_event_type type,
> > > > + enum iio_event_direction dir)
> > > > +{
> > > > + struct ti_ads7142_priv *priv = iio_priv(indio_dev);
> > > > + struct ti_ads7142_channel *channel;
> > > > + int ret;
> > > > +
> > > > + if (!priv->config.monitoring_mode)
> > > > + return -EINVAL;
> > > > +
> > > > + if (type != IIO_EV_TYPE_THRESH)
> > > > + return -EINVAL;
> > > > +
> > > > + ret = ti_ads7142_address2channel(indio_dev, chan->address,
> > > > + &channel);
> > > > + if (ret)
> > > > + return ret;
> > > > +
> > > > + if (dir == IIO_EV_DIR_RISING)
> > > > + ret = channel->config.alert_high ? 1 : 0;
> > >
> > > Not fine using ret = channel->config.alert_high; directly?
> > >
> >
> > alert_high is bool, ret is int.
> > I know, the 'true' value is 1, and its autmatically casted,
> > but who knows the future...I would keep this, if possible.
>
> Ok. Assuming the c spec will change to make this invalid is a bit implausible,
> but it's not too bad I guess.
>
> >
> ...
>
> >
> > >
> > > > + &ads_channel->config.high_threshold);
> > > > + ads_channel->config.alert_high = !ret;
> > > > + ret = of_property_read_u32(channel_node, "ti,threshold-falling",
> > > > + &ads_channel->config.low_threshold);
> > > > + ads_channel->config.alert_low = !ret;
> > > > + ret = of_property_read_u32(channel_node, "ti,hysteresis",
> > > > + &ads_channel->config.hysteresis);
> > > > + channel_index++;
> > > > + }
> > > > +
> > > > + return 0;
> > > > +err:
> > > > + of_node_put(channel_node);
> > > > + return ret;
> > > > +}
> > > > +
> > > > +static int ti_ads7142_parse_config_of(struct device *dev,
> > > > + struct iio_dev *indio_dev)
> > > > +{
> > > > + struct ti_ads7142_priv *priv = iio_priv(indio_dev);
> > > > +
> > > > + priv->config.osc_sel = of_property_read_bool(dev->of_node,
> > > > + "ti,osc-sel");
> > >
> > > Please use generic device property access functions where possible.
> > > That basically gives us support on non OF based platforms for free.
> >
> > Could you please explain this, I dont understand.
>
> See include/linux/property.h
>
> That provides firmware type agnostic property accessors - so they will work
> whether the property is provided via ACPI or via DT (or a number of other
> options though those are less common).
>
> ACPI DSDT tables can use a special device type PRP0001 which basically is a
> wrapper for device tree properties. So the properties are all the same
> (including compatible) but you need to use the generic accessors to be able to
> read them.
>
I'ts clear now, include/linux/property.h was a good starting point. Thank you.
>
> >
> > >
> > > > + of_property_read_u32(dev->of_node, "ti,n-clk", &priv->config.n_clk);
> > > > + priv->config.monitoring_mode = of_property_read_bool(dev->of_node,
> > > > + "ti,monitoring-mode");
> > > > +
> > > > + return ti_ads7142_parse_channel_config_of(dev, indio_dev);
> > > > +}
> >
> ...
> > Thank you for the review and suggestions.
>
> You are welcome.
>
> Thanks,
>
> Jonathan
>
> >
> > Best regards
> > Jozsef
>
> >
> > >
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +module_i2c_driver(ti_ads7142_driver);
> > > > +
> > > > +MODULE_LICENSE("GPL");
> > > > +MODULE_AUTHOR("Jozsef Horvath <info@ministro.hu>");
> > > > +MODULE_DESCRIPTION("Texas Instruments TI_ADS7142 ADC driver");
> > >
> > >
> >
>
Best regards
Jozsef
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] dt-bindings: iio: adc: devicetree bindings for texas instruments ads7142 iio driver
2021-05-03 10:30 ` Jonathan Cameron
@ 2021-05-03 19:42 ` József Horváth
0 siblings, 0 replies; 10+ messages in thread
From: József Horváth @ 2021-05-03 19:42 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Lars-Peter Clausen, Peter Meerwald-Stadler, Rob Herring,
Liam Girdwood, Mark Brown, Heiko Stuebner, Andy Shevchenko,
Alex Dewar, Gene Chen, Saravanan Sekar, Lee Jones, linux-iio,
devicetree, linux-kernel
On Mon, May 03, 2021 at 11:30:08AM +0100, Jonathan Cameron wrote:
Hi Jonathan,
> On Sun, 2 May 2021 21:10:20 +0000
> József Horváth <info@ministro.hu> wrote:
>
> Hi József,
>
> > >
> > > > +
> > > > + "#address-cells":
> > > > + const: 1
> > > > +
> > > > + "#size-cells":
> > > > + const: 0
> > > > +
> > > > + "#io-channel-cells":
> > > > + const: 1
> > > > +
> > > > + ti,osc-sel:
> > > > + description: |
> > > > + If present, the driver selects the high speed oscillator.
> > > > + See chapter 7.3.5 Oscillator and Timing Control in datasheet.
> > > > + type: boolean
> > >
> > > This looks connected to the possible sampling frequencies when in various autonomous modes.
> > > Should it be controlled by userspace?
> >
> > The sampling frequency is controlled with the osc-sel and n-clk.
> > I'll remove n-clk from sysfs.
>
> Not sure I follow that. I think we should only have these controlled via
> sysfs. It will be a bit complex as two related controls, but that is a common situation
> and there is usually a sensible combination of options that makes sense.
>
> For example, if we can meet the sampling frequency requested with the lower
> power oscillator we go for that.
>
Ok, I'll do that.
> >
> > >
> > > > +
> > > > + ti,n-clk:
> > > > + description: |
> > > > + nCLK is number of clocks in one conversion cycle.
> > > > + See chapter 7.3.5 Oscillator and Timing Control in datasheet.
> > >
> > > Sounds like a policy decision for userspace.
> > >
> > > > + $ref: /schemas/types.yaml#/definitions/uint32
> > > > + maximum: 255
> > > > + minimum: 0
> > > > +
> > > > + ti,monitoring-mode:
> > > > + description: |
> > > > + If present, the driver selects the autonomous monitoring mode with pre alert data.
> > > > + See chapter 7.4 Device Functional Modes in datasheet.
> > >
> > > As mentioned in the driver review, this looks like something we should control from userspace
> > > not dt to me.
> > >
> >
> > I would keep this here, but it will be an enum.
>
> Sorry, but no. As mentioned in the driver thread, this doesn't sound like
> a characteristic of the hardware (board etc) so it doesn't belong in DT.
> It may be challenging to implement an interface that makes sense, but
> that doesn't mean we can avoid doing it.
Ok, I'll bring it to sysfs.
>
> >
> > > > + type: boolean
> > > > +
> > > > +patternProperties:
> > > > + "^channel@[0-1]$":
> > > > + $ref: "adc.yaml"
> > > > + type: object
> > > > + description: |
> > > > + Represents the external channels which are connected to the ADC.
> > > > + properties:
> > > > + reg:
> > > > + description: |
> > > > + The channel number.
> > > > + items:
> > > > + minimum: 0
> > > > + maximum: 1
> > > > + "ti,threshold-falling":
> > > > + description: The low threshold for channel
> > >
> > > For these, we need a strong argument presented in this doc for why they are not
> > > a question of policy (and hence why they should be in dt at all).
> >
> > I'll remove all threshold and hysteresys from dt.
> >
> > >
> > > > + $ref: /schemas/types.yaml#/definitions/uint32
> > > > + maximum: 4095
> > > > + minimum: 0
> > > > + "ti,threshold-rising":
> > > > + description: The high threshold for channel
> > > > + $ref: /schemas/types.yaml#/definitions/uint32
> > > > + maximum: 4095
> > > > + minimum: 0
> > > > + "ti,hysteresis":
> > > > + description: The hysteresis for both comparators for channel
> > > > + $ref: /schemas/types.yaml#/definitions/uint32
> > > > + maximum: 63
> > > > + minimum: 0
> > > > +
> > > > + required:
> > > > + - reg
> > > > +
> > > > + additionalProperties: false
> > > > +
> > > > +allOf:
> > > > + - if:
> > > > + required:
> > > > + - ti,monitoring-mode
> > > > + then:
> > > > + required:
> > > > + - interrupts
> > > > +
> > > > +required:
> > > > + - compatible
> > > > + - "#io-channel-cells"
> > > > +
> > > > +additionalProperties: false
> > > > +
> > > > +examples:
> > > > + - |
> > > > + i2c {
> > > > + #address-cells = <1>;
> > > > + #size-cells = <0>;
> > > > + adc@18 {
> > >
> > > I would not bother having two examples. The second one covers more things afterall
> > > and the binding makes it clear what is required.
> > >
> >
> > I do this because of the conditional requirement of interrupts.
>
> Understood, but that is clearly expressed by the 'required' above.
> It's easy enough to understand how to not have parts that aren't
> required without an example.
I'll bring the monitoring mode to sysfs, and in that case, the conditional requirement
will be pointless, and interrupt property will be optional, so you are right, one example
will be enough.
>
> >
> > > > + compatible = "ti,ads7142";
> > > > + reg = <0x18>;
> > > > + #address-cells = <1>;
> > > > + #size-cells = <0>;
> > > > + #io-channel-cells = <1>;
> > > > +
> > > > + vref-supply = <&vdd_3v3_reg>;
> > > > + power-supply = <&vdd_1v8_reg>;
> > > > +
> > > > + channel@0 {
> > > > + reg = <0>;
> > > > + };
> > > > +
> > > > + channel@1 {
> > > > + reg = <1>;
> > > > + };
> > > > + };
> > > > + };
> > > > + - |
> > > > + i2c {
> > > > + #address-cells = <1>;
> > > > + #size-cells = <0>;
> > > > + adc@1f {
> > > > + compatible = "ti,ads7142";
> > > > + reg = <0x1f>;
> > > > + #address-cells = <1>;
> > > > + #size-cells = <0>;
> > > > +
> > > > + #io-channel-cells = <1>;
> > > > +
> > > > + vref-supply = <&vdd_3v3_reg>;
> > > > + power-supply = <&vdd_1v8_reg>;
> > > > +
> > > > + interrupt-parent = <&gpio>;
> > > > + interrupts = <7 2>;
> > > > +
> > > > + ti,monitoring-mode;
> > > > +
> > > > + channel@0 {
> > > > + reg = <0>;
> > > > + ti,threshold-falling = <1000>;
> > > > + ti,threshold-rising = <2000>;
> > > > + ti,hysteresis = <20>;
> > > > + };
> > > > +
> > > > + channel@1 {
> > > > + reg = <1>;
> > > > + ti,threshold-falling = <100>;
> > > > + ti,threshold-rising = <2500>;
> > > > + ti,hysteresis = <0>;
> > > > + };
> > > > + };
> > > > + };
> > > > +...
> > > > +
> > >
>
Best regards
Jozsef
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2021-05-03 19:43 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-01 18:24 [PATCH 1/2] iio: adc: driver for texas instruments ads7142 Jozsef Horvath
2021-05-01 18:25 ` [PATCH 2/2] dt-bindings: iio: adc: devicetree bindings for texas instruments ads7142 iio driver Jozsef Horvath
2021-05-02 17:22 ` Jonathan Cameron
2021-05-02 21:10 ` József Horváth
2021-05-03 10:30 ` Jonathan Cameron
2021-05-03 19:42 ` József Horváth
2021-05-02 17:14 ` [PATCH 1/2] iio: adc: driver for texas instruments ads7142 Jonathan Cameron
2021-05-02 20:48 ` József Horváth
2021-05-03 10:26 ` Jonathan Cameron
2021-05-03 19:33 ` József Horváth
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).