Re: [PATCH v1 1/2] iio: adc: driver for texas instruments ads7142

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, May 14, 2021 at 1:52 AM Jozsef Horvath <info@xxxxxxxxxxx> wrote:
>
> This is an iio driver for
>  Texas Instruments ADS7142 dual-channel, programmable sensor monitor.
>

Hey,

Looks good overall.
Some comments inline.

> All of the data buffer modes, supported by the device are selectable
>  over sysfs:
>   /sys/bus/iio/devices/iio:deviceX/buffer_mode
>
> Data buffer operation modes:
>   - start_burst:
>     In triggered buffer preenable hook: not relevant.
>     In trigger handler: the driver selects the autonomous monitoring
>       (see chapter 7.4.3 in datasheet) operation mode,
>           configures the channels in sequencer as specified by
>       sysfs(scan_elements/in_voltageY_en),
>           sets the data buffer mode to "Start Burst Mode",
>       then starts the conversion.
>     In irq handler: the driver pushes the conversion results received
>       from the device to the buffer,
>       then restarts the conversion like in trigger handler.
>     Both IRQ and trigger are required to support this mode.
>     See chapter 7.4.3.2.1 "Autonomous Mode with Start Burst"
>     in datasheet.
>   - stop_burst:
>     In triggered buffer preenable hook: the driver selects the
>       autonomous monitoring (see chapter 7.4.3 in datasheet)
>       operation mode,
>       configures the channels in sequencer as
>       specified by sysfs(scan_elements/in_voltageY_en),
>           sets the data buffer mode to "Stop Burst Mode",
>       then starts the conversion.
>     In trigger handler: the driver pushes the conversion results received
>       from the device to the buffer,
>           then restarts the conversion like in preenable hook.
>     In irq handler: not relevant.
>     Trigger is required to support this mode.
>     See chapter 7.4.3.2.2 "Autonomous Mode with Stop Burst"
>     in datasheet.
>   - pre_alert:
>     In triggered buffer preenable hook: the driver selects the autonomous
>       monitoring (see chapter 7.4.3 in datasheet) operation mode,
>       configures the channels in sequencer
>         as specified by sysfs(scan_elements/in_voltageY_en),
>       configures the digital window comparator and alert flags,
>           sets the data buffer mode to "Pre Alert Data Mode",
>       then starts the conversion.
>     In trigger handler: not relevant.
>     In irq handler: the driver pushes the conversion results received
>       from the device to the buffer,
>       then restarts the conversion like in preenable hook.
>     IRQ is required to support this mode.
>     See chapter 7.4.3.1.1 "Autonomous Mode with Pre Alert Data"
>     in datasheet
>   - post_alert:
>     The operations are same like in pre_alert mode,
>       except the data buffer mode selection, the selected mode is
>       "Post Alert Data Mode".
>     See chapter 7.4.3.1.2 "Autonomous Mode with Post Alert Data"
>     in datasheet
>
> The in_voltageY_raw can be used, if the buffering mode is not enabled
>  in sysfs(buffer/enable).
>  The driver initiates a single conversion in the device for each
>  read request(in_voltageY_raw).
>  This is a one-shot conversion.
>  See chapter 7.4.2.2 "Manual Mode with AUTO Sequence" in datasheet.
>
> Datasheet: https://www.ti.com/lit/ds/symlink/ads7142.pdf
>
> Signed-off-by: Jozsef Horvath <info@xxxxxxxxxxx>
> ---
>
> changes v1
>   - All of the buffer opertaion modes
>     (pre_alert, post_alert, start_burst, stop_burst)
>         are added
>   - Added triggered buffer
>   - Added buffer operation mode selection sysfs support
>   - Redundant parameters (ti,threshold-rising, etc.)
>     are removed
>   - Supply name changed(vref -> avdd)
>   - Added dvdd supply
>   - Added device sampling rate calculation
>   - Use device-managed functions for regulator, iio device register
>     and triggered buffer
> ---
>  .../ABI/testing/sysfs-bus-iio-adc-ti-ads7142  |   11 +
>  MAINTAINERS                                   |    7 +
>  drivers/iio/adc/Kconfig                       |   13 +
>  drivers/iio/adc/Makefile                      |    1 +
>  drivers/iio/adc/ti-ads7142.c                  | 1469 +++++++++++++++++
>  5 files changed, 1501 insertions(+)
>  create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-adc-ti-ads7142
>  create mode 100644 drivers/iio/adc/ti-ads7142.c
>
> diff --git a/Documentation/ABI/testing/sysfs-bus-iio-adc-ti-ads7142 b/Documentation/ABI/testing/sysfs-bus-iio-adc-ti-ads7142
> new file mode 100644
> index 000000000000..7de69a4819e5
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-bus-iio-adc-ti-ads7142
> @@ -0,0 +1,11 @@
> +What:          /sys/bus/iio/devices/iio:deviceX/buffer_mode_available
> +Date:          Maa 2021
> +KernelVersion: 5.13
> +Contact:       info@xxxxxxxxxxx
> +Description:   List all available buffer_mode settings.
> +
> +What:          /sys/bus/iio/devices/iio:deviceX/buffer_mode
> +Date:          Maa 2021
> +KernelVersion: 5.13
> +Contact:       info@xxxxxxxxxxx
> +Description:   Sets up the device data buffer mode.
> diff --git a/MAINTAINERS b/MAINTAINERS
> index ba0cc0a67b32..9acc64ed54e1 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -18084,6 +18084,13 @@ 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@xxxxxxxxxxx>
> +S:     Maintained
> +F:     Documentation/ABI/testing/sysfs-bus-iio-adc-ti-ads7142
> +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@xxxxxxxxx>
>  L:     alsa-devel@xxxxxxxxxxxxxxxx (moderated for non-subscribers)
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index db0c8fb60515..6c34ede355e4 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -1111,6 +1111,19 @@ 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
> +       select IIO_BUFFER
> +       select IIO_TRIGGERED_BUFFER
> +       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 f70d877c555a..486ca7f4037e 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..ab49531933e1
> --- /dev/null
> +++ b/drivers/iio/adc/ti-ads7142.c
> @@ -0,0 +1,1469 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2021 Jozsef Horvath <info@xxxxxxxxxxx>
> + *
> + */
> +#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>
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/trigger_consumer.h>
> +#include <linux/iio/triggered_buffer.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
> +#define TI_ADS7142_NCLK_SEL_MSK                                0xFF
> +/* 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
> +
> +#define TI_ADS7142_HSO_FREQ                            20000000
> +#define TI_ADS7142_HSO_NCLK_MIN                                21
> +#define TI_ADS7142_HSO_NCLK_MAX                                255
> +#define TI_ADS7142_HSO_MIN_SS                          78431
> +#define TI_ADS7142_HSO_MAX_SS                          952380
> +#define TI_ADS7142_LPO_FREQ                            10504
> +#define TI_ADS7142_LPO_NCLK_MIN                                18
> +#define TI_ADS7142_LPO_NCLK_MAX                                255
> +#define TI_ADS7142_LPO_MIN_SS                          41
> +#define TI_ADS7142_LPO_MAX_SS                          583
> +
> +#define TI_ADS7142_CHANNEL_COUNT                       2
> +
> +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;
> +       u32 channel;
> +};
> +
> +struct ti_ads7142_config {
> +       bool osc_sel;
> +       u32 n_clk;
> +       int buffer_mode;
> +};
> +
> +struct ti_ads7142_priv {
> +       struct mutex lock; /* For syncing access to device */
> +       struct regulator *avdd;
> +       struct regulator *dvdd;
> +       struct ti_ads7142_config config;
> +       struct ti_ads7142_channel *channels;
> +       u16 *scan_data;
> +       bool irq_present;
> +       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 const char * const ti_ads7142_ain_names[] = {
> +       "AIN0",
> +       "AIN1",
> +};
> +
> +#define TI_ADS7142_BUFFM_NONE                          0
> +#define TI_ADS7142_BUFFM_STOP_BURST                    1
> +#define TI_ADS7142_BUFFM_START_BURST                   2
> +#define TI_ADS7142_BUFFM_PRE_ALERT                     3
> +#define TI_ADS7142_BUFFM_POST_ALERT                    4
> +static const char * const ti_ads7142_buffer_modes[] = {
> +       [TI_ADS7142_BUFFM_NONE]         = "none",
> +       [TI_ADS7142_BUFFM_STOP_BURST]   = "stop_burst",
> +       [TI_ADS7142_BUFFM_START_BURST]  = "start_burst",
> +       [TI_ADS7142_BUFFM_PRE_ALERT]    = "pre_alert",
> +       [TI_ADS7142_BUFFM_POST_ALERT]   = "post_alert"
> +};
> +
> +static int ti_ads7142_reg_write(const struct i2c_client *client, u8 reg,
> +                               u8 data)
> +{
> +       u8 buf[3] = { TI_ADS7142_OC_SINGLE_REG_WRITE, reg, data };
> +       int ret;
> +
> +       ret = i2c_master_send(client, buf, sizeof(buf));
> +       if (ret >= 0 && ret != sizeof(buf))
> +               ret = -EIO;
> +
> +       return ret == sizeof(buf) ? 0 : ret;
> +}
> +
> +static int ti_ads7142_reg_read(const struct i2c_client *client, u8 reg,
> +                              u8 *data)
> +{
> +       u8 buf[2] = { TI_ADS7142_OC_SINGLE_REG_READ, reg };
> +       struct i2c_msg msg[2] = {
> +               {
> +                       .addr = client->addr,
> +                       .len = sizeof(buf),
> +                       .buf = buf,
> +               },
> +               {
> +                       .addr = client->addr,
> +                       .flags = I2C_M_RD,
> +                       .len = 1,
> +                       .buf = data,
> +               }
> +       };
> +       int ret;
> +
> +       ret = i2c_transfer(client->adapter, msg, 2);
> +       if (ret >= 0 && ret != ARRAY_SIZE(msg))
> +               ret = -EIO;
> +
> +       return ret == ARRAY_SIZE(msg) ? 0 : ret;
> +}
> +
> +static int ti_ads7142_data_buffer_read(const struct i2c_client *client,
> +                                      int length, void *data)
> +{
> +       int ret;
> +
> +       ret = i2c_master_recv(client, data, length);
> +       if (ret >= 0 && ret != length)
> +               ret = -EIO;
> +
> +       return ret == length ? 0 : ret;
> +}
> +
> +static int ti_ads7142_soft_reset(const struct i2c_client *client)
> +{
> +       u8 buf[2] = { TI_ADS7142_OC_GENERAL, 0x06 };
> +       int ret;
> +
> +       ret = i2c_master_send(client, buf, sizeof(buf));
> +       if (ret >= 0 && ret != sizeof(buf))
> +               ret = -EIO;
> +
> +       return ret == sizeof(buf) ? 0 : ret;
> +}
> +
> +static int ti_ads7142_address2channel(struct iio_dev *indio_dev,
> +                                     int address,
> +                                     struct ti_ads7142_channel **channel)

This could be simplified a bit to return "const struct
ti_ads7142_channel *" and then when NULL is returned, the caller
assumes -ENODEV.
Though, in  ti_ads7142_ist()  care should be taken to assign  ret =
-ENODEV; but that's only one place.


> +{
> +       struct ti_ads7142_priv *priv = iio_priv(indio_dev);
> +       int i;
> +
> +       for (i = 0; i < indio_dev->num_channels; 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_osc_calc_set(struct iio_dev *indio_dev, int request)
> +{
> +       struct ti_ads7142_priv *priv = iio_priv(indio_dev);
> +       bool osc_sel;
> +       u32 osc_freq;
> +
> +       if (request <= TI_ADS7142_LPO_MAX_SS) {
> +               osc_sel = true;
> +               if (request < TI_ADS7142_LPO_MIN_SS)
> +                       request = TI_ADS7142_LPO_MIN_SS;
> +               osc_freq = TI_ADS7142_LPO_FREQ;
> +       } else {
> +               osc_sel = false;
> +               if (request < TI_ADS7142_HSO_MIN_SS)
> +                       request = TI_ADS7142_HSO_MIN_SS;
> +               if (request > TI_ADS7142_HSO_MAX_SS)
> +                       request = TI_ADS7142_HSO_MAX_SS;
> +               osc_freq = TI_ADS7142_HSO_FREQ;
> +       }
> +
> +       priv->config.osc_sel = osc_sel;
> +       priv->config.n_clk = (osc_freq / request) & TI_ADS7142_NCLK_SEL_MSK;
> +       return 0;
> +}
> +
> +static int ti_ads7142_osc_calc_get(struct iio_dev *indio_dev, int *result)
> +{
> +       struct ti_ads7142_priv *priv = iio_priv(indio_dev);
> +       u32 n_clk;
> +       u32 osc_freq;
> +
> +       if (priv->config.osc_sel) {
> +               // Low power oscillator
> +               n_clk = priv->config.n_clk;
> +               if (n_clk < TI_ADS7142_LPO_NCLK_MIN)
> +                       n_clk = TI_ADS7142_LPO_NCLK_MIN;
> +               if (n_clk > TI_ADS7142_LPO_NCLK_MAX)
> +                       n_clk = TI_ADS7142_LPO_NCLK_MAX;
> +               osc_freq = TI_ADS7142_LPO_FREQ;
> +       } else {
> +               // High speed oscillator
> +               n_clk = priv->config.n_clk;
> +               if (n_clk < TI_ADS7142_HSO_NCLK_MIN)
> +                       n_clk = TI_ADS7142_HSO_NCLK_MIN;
> +               if (n_clk > TI_ADS7142_HSO_NCLK_MAX)
> +                       n_clk = TI_ADS7142_HSO_NCLK_MAX;
> +               osc_freq = TI_ADS7142_HSO_FREQ;
> +       }
> +       *result = osc_freq / n_clk;
> +       return 0;
> +}
> +
> +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_buffered_setup_and_start(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);
> +       struct ti_ads7142_channel *channel;
> +       int i;
> +       u8 alert_ch = 0;
> +       u8 buffer_op_mode;
> +       u8 seq_channels = 0;
> +       int ret;
> +
> +       if (!priv->config.buffer_mode)
> +               return 0;
> +
> +       priv->monitor_pending = false;
> +
> +       ret = ti_ads7142_sequence_abort(indio_dev);
> +       if (ret)
> +               return ret;
> +
> +       ret = ti_ads7142_osc_set(indio_dev);
> +       if (ret)
> +               return ret;
> +
> +       ret = ti_ads7142_input_cfg_set(indio_dev);
> +       if (ret)
> +               return ret;
> +
> +       ret = ti_ads7142_dout_format_set(indio_dev);
> +       if (ret)
> +               return ret;
> +
> +       switch (priv->config.buffer_mode) {
> +       case TI_ADS7142_BUFFM_STOP_BURST:
> +               buffer_op_mode = TI_ADS7142_DATA_BUFFER_OPMODE_STOP_BURST;
> +       break;
> +       case TI_ADS7142_BUFFM_START_BURST:
> +               buffer_op_mode = TI_ADS7142_DATA_BUFFER_OPMODE_START_BURST;
> +       break;
> +       case TI_ADS7142_BUFFM_PRE_ALERT:
> +               buffer_op_mode = TI_ADS7142_DATA_BUFFER_OPMODE_PRE_ALERT;
> +       break;

the indentation of the break; statements is a bit odd;
IIRC, they usually go aligned with the case block;
though i could be wrong;

> +       case TI_ADS7142_BUFFM_POST_ALERT:
> +               buffer_op_mode = TI_ADS7142_DATA_BUFFER_OPMODE_POST_ALERT;
> +       break;
> +       default:
> +               return -EINVAL;
> +       break;

also an unreachable statement

> +       }
> +       ret = ti_ads7142_reg_write(client, TI_ADS7142_DATA_BUFFER_OPMODE,
> +                                  buffer_op_mode);
> +       if (ret)
> +               return ret;
> +
> +       ret = ti_ads7142_reg_write(client, TI_ADS7142_OPMODE_SEL,
> +                                  TI_ADS7142_OPMODE_SEL_MONITORING);
> +       if (ret)
> +               return ret;
> +
> +       for_each_set_bit(i, indio_dev->active_scan_mask,
> +                        indio_dev->masklength) {
> +               seq_channels |= 1 << i;
> +       }
> +
> +       ret = ti_ads7142_reg_write(client, TI_ADS7142_AUTO_SEQ_CHEN,
> +                                  seq_channels);
> +       if (ret)
> +               return ret;
> +
> +       if (priv->config.buffer_mode < TI_ADS7142_BUFFM_PRE_ALERT)
> +               goto seq_start;
> +
> +       /*
> +        * Pre and post alert settings
> +        */
> +       ret = ti_ads7142_reg_write(client, TI_ADS7142_PRE_ALT_EVT_CNT, 0);
> +       if (ret)
> +               return ret;
> +
> +       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)
> +               return ret;
> +
> +       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)
> +               return ret;
> +
> +       for_each_set_bit(i, indio_dev->active_scan_mask,
> +                        indio_dev->masklength) {
> +               ret = ti_ads7142_address2channel(indio_dev, i,
> +                                                &channel);
> +               if (ret)
> +                       return ret;
> +
> +               ret = ti_ads7142_hth_set(indio_dev, channel->channel,
> +                                        channel->config.high_threshold);
> +               if (ret)
> +                       return ret;
> +
> +               ret = ti_ads7142_lth_set(indio_dev, channel->channel,
> +                                        channel->config.low_threshold);
> +               if (ret)
> +                       return ret;
> +
> +               ret = ti_ads7142_hys_set(indio_dev, channel->channel,
> +                                        channel->config.hysteresis);
> +               if (ret)
> +                       return ret;
> +
> +               if (channel->config.alert_low ||
> +                   channel->config.alert_high) {
> +                       alert_ch |= 1 << channel->channel;
> +               }
> +       }
> +
> +       ret = ti_ads7142_reg_write(client, TI_ADS7142_ALERT_DWC_EN,
> +                                  alert_ch ? TI_ADS7142_ALERT_DWC_EN_BLOCK_EN : 0);
> +       if (ret)
> +               return ret;
> +
> +       ret = ti_ads7142_reg_write(client, TI_ADS7142_ALERT_CHEN,
> +                                  alert_ch);
> +       if (ret)
> +               return ret;
> +
> +       if (!alert_ch)
> +               return ret;

this looks the same as
          if (!alert_ch)
              return 0;
for a while, this looked like 'ret' was not initialized;

> +
> +seq_start:
> +       ret = ti_ads7142_sequence_start(indio_dev);
> +       priv->monitor_pending = !ret;
> +
> +       return ret;
> +}
> +
> +static int ti_ads7142_buffered_collect(struct iio_dev *indio_dev,
> +                                      int *channel_collected)
> +{
> +       struct ti_ads7142_priv *priv = iio_priv(indio_dev);
> +       struct i2c_client *client = to_i2c_client(indio_dev->dev.parent);
> +       int scan_channel_count;
> +       int have_valid_data;
> +       int data_valid;
> +       u16 data_buffer;
> +       u16 buffer[TI_ADS7142_CHANNEL_COUNT];
> +       u8 seq_channels = 0;
> +       int channel_address;
> +       int value;
> +       int i, j;
> +       int ret;
> +
> +       if (!priv->scan_data)
> +               return -EINVAL;
> +
> +       scan_channel_count = bitmap_weight(indio_dev->active_scan_mask,
> +                                          indio_dev->masklength);
> +       if (!scan_channel_count)
> +               return -EINVAL;
> +
> +       for_each_set_bit(i, indio_dev->active_scan_mask,
> +                        indio_dev->masklength) {
> +               seq_channels |= 1 << i;
> +       }
> +
> +       do {
> +               memset(priv->scan_data, 0x00, indio_dev->scan_bytes);
> +               have_valid_data = 0;
> +               for (i = 0; i < scan_channel_count; i++) {
> +                       ret = ti_ads7142_data_buffer_read(client,
> +                                                         sizeof(data_buffer),
> +                                                         &data_buffer);
> +                       if (ret)
> +                               return ret;
> +                       data_buffer = be16_to_cpu(data_buffer);
> +                       data_valid = data_buffer & 1;
> +                       if (!data_valid) {
> +                               ret = -ENOENT;
> +                               break;
> +                       }
> +
> +                       channel_address = (data_buffer >> 1) & 0x7;
> +                       if (!(seq_channels & 1 << channel_address)) {
> +                               dev_err(indio_dev->dev.parent,
> +                                       "%s: invalid channel address(%d)",
> +                                       __func__, channel_address);
> +                               return -EIO;
> +                       }
> +
> +                       value = data_buffer >> 4;
> +                       buffer[channel_address] = value;
> +                       have_valid_data = 1;
> +                       if (channel_collected)
> +                               *channel_collected |= 1 << channel_address;
> +               }
> +
> +               if (!have_valid_data)
> +                       continue;
> +
> +               j = 0;
> +               for_each_set_bit(i, indio_dev->active_scan_mask,
> +                                indio_dev->masklength) {
> +                       priv->scan_data[j] = buffer[i];
> +                       j++;
> +               }
> +               iio_push_to_buffers_with_timestamp(indio_dev, priv->scan_data,
> +                                                  iio_get_time_ns(indio_dev));
> +       } while (data_valid);
> +
> +       return ret;
> +}
> +
> +static int ti_ads7142_buffered_abort(struct iio_dev *indio_dev)
> +{
> +       struct ti_ads7142_priv *priv = iio_priv(indio_dev);
> +       int ret;
> +
> +       ret = ti_ads7142_sequence_abort(indio_dev);
> +       if (!ret)
> +               priv->monitor_pending = false;
> +
> +       return ret;
> +}
> +
> +static int ti_ads7142_manual_read(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;
> +
> +       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 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.buffer_mode || !priv->monitor_pending) {
> +               mutex_unlock(&priv->lock);
> +               return IRQ_NONE;
> +       }
> +
> +       /*
> +        * BUSY/PDY fires when the sequence stopped in
> +        * trigger handler(ti_ads7142_trigger_handler),
> +        * if buffer mode is stop_burst, all the required
> +        * operations are in trigger handler, so irq handler
> +        * simple returns at this point.
> +        */
> +       if (priv->config.buffer_mode == TI_ADS7142_BUFFM_STOP_BURST) {
> +               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;
> +       }
> +
> +       ret = ti_ads7142_sequence_abort(indio_dev);
> +       if (ret)
> +               goto final;
> +
> +       priv->monitor_pending = false;
> +
> +       channel_collected = 0;
> +       ret = ti_ads7142_buffered_collect(indio_dev, &channel_collected);
> +       if (ret && ret != -ENOENT) {
> +               dev_err(indio_dev->dev.parent,
> +                       "%s: error(%d) when collecting result for %s mode",
> +                       __func__, ret,
> +                       ti_ads7142_buffer_modes[priv->config.buffer_mode]);
> +               goto final;
> +       }
> +
> +       if (ret == -ENOENT)
> +               ret = 0;
> +
> +       if (!channel_collected)
> +               goto final;
> +
> +       if (priv->config.buffer_mode < TI_ADS7142_BUFFM_PRE_ALERT)
> +               goto final;
> +
> +       for_each_set_bit(i, indio_dev->active_scan_mask,
> +                        indio_dev->masklength) {
> +               ret = ti_ads7142_address2channel(indio_dev, i,
> +                                                &channel);
> +               if (ret)
> +                       goto final;
> +
> +               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,
> +                                                           i,
> +                                                           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,
> +                                                           i,
> +                                                           IIO_EV_TYPE_THRESH,
> +                                                           IIO_EV_DIR_RISING),
> +                                      timestamp);
> +               }
> +       }
> +
> +final:

This final label looks a bit busy.
Maybe an idea would be to have 2 labels, and depdending on 'ret' you
do "goto {err,out}_unlock":

out_unlock:
      if (priv->config.buffer_mode >= TI_ADS7142_BUFFM_PRE_ALERT) {
              ret = ti_ads7142_buffered_setup_and_start(indio_dev);
               if (ret) {
                      dev_err(indio_dev->dev.parent,
                              "%s: error(%d) when starting %s mode",
                              __func__, ret,

ti_ads7142_buffer_modes[priv->config.buffer_mode]);
                      goto err_unlock;
              }
      }
      mutex_unlock(&priv->lock);
      return IRQ_HANDLED;

err_unlock:
       mutex_unlock(&priv->lock);
       return IRQ_NONE;


> +       if (!ret && priv->config.buffer_mode >= TI_ADS7142_BUFFM_PRE_ALERT) {
> +               ret = ti_ads7142_buffered_setup_and_start(indio_dev);
> +               if (ret) {
> +                       dev_err(indio_dev->dev.parent,
> +                               "%s: error(%d) when starting %s mode",
> +                               __func__, ret,
> +                               ti_ads7142_buffer_modes[priv->config.buffer_mode]);
> +               }
> +       }
> +       mutex_unlock(&priv->lock);
> +       if (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:
> +               if (iio_buffer_enabled(indio_dev))
> +                       return -EBUSY;
> +               ret = iio_device_claim_direct_mode(indio_dev);
> +               if (ret)
> +                       return ret;
> +               ret = ti_ads7142_manual_read(indio_dev, chan->address,
> +                                            val);
> +               if (!ret)
> +                       ret = IIO_VAL_INT;
> +               iio_device_release_direct_mode(indio_dev);
> +               return ret;
> +       case IIO_CHAN_INFO_SAMP_FREQ:
> +               ti_ads7142_osc_calc_get(indio_dev, val);
> +               return IIO_VAL_INT;
> +       case IIO_CHAN_INFO_SCALE:
> +               if (IS_ERR(priv->avdd)) {
> +                       ret = -EINVAL;
> +               } else {
> +                       *val = regulator_get_voltage(priv->avdd) / 1000;
> +                       *val2 = chan->scan_type.realbits;
> +                       ret = IIO_VAL_FRACTIONAL_LOG2;
> +               }

purely stylistic change; but this could also be:

              if (IS_ERR(priv->avdd))
                  return -EINVAL

              *val = regulator_get_voltage(priv->avdd) / 1000;
              *val2 = chan->scan_type.realbits;
              return IIO_VAL_FRACTIONAL_LOG2;

> +               return ret;
> +       default:
> +               return -EINVAL;
> +       }
> +       return 0;

This "return 0;" looks like an unreachable statement.
Some GCC versions complain about it. Not sure which.

> +}
> +
> +static int ti_ads7142_write_raw(struct iio_dev *indio_dev,
> +                               struct iio_chan_spec const *chan,
> +                               int val, int val2, long mask)
> +{
> +       switch (mask) {
> +       case IIO_CHAN_INFO_SAMP_FREQ:
> +               return ti_ads7142_osc_calc_set(indio_dev, val);
> +       default:
> +               return -EINVAL;
> +       }
> +
> +       return 0;

Also, an unreachale statement.

> +}
> +
> +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_channel *channel;
> +       int ret;
> +
> +       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;
> +               return IIO_VAL_INT;
> +       case IIO_EV_INFO_HYSTERESIS:
> +               *val = channel->config.hysteresis;
> +               return IIO_VAL_INT;
> +       default:
> +               return -EINVAL;
> +       }
> +       return 0;

Also an unreachable statement.

> +}
> +
> +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_channel *channel;
> +       int ret;
> +
> +       ret = ti_ads7142_address2channel(indio_dev, chan->address,
> +                                        &channel);
> +       if (ret)
> +               return ret;
> +
> +       switch (info) {
> +       case IIO_EV_INFO_VALUE:
> +               if (val < 0 || val > TI_ADS7142_THRESHOLD_MSK) {
> +                       ret = -EINVAL;
> +               } else {
> +                       if (dir == IIO_EV_DIR_RISING)
> +                               channel->config.high_threshold = val;
> +                       else
> +                               channel->config.low_threshold = val;
> +               }
> +       break;
> +       case IIO_EV_INFO_HYSTERESIS:
> +               if (val < 0 || val > TI_ADS7142_HYSTERESIS_MSK)
> +                       ret = -EINVAL;
> +               else
> +                       channel->config.hysteresis = val;
> +       break;
> +       default:
> +               ret = -EINVAL;
> +       break;
> +       }
> +
> +       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_channel *channel;
> +       int ret;
> +
> +       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_channel *channel;
> +       int ret;
> +
> +       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)
> +               channel->config.alert_high = state;
> +       else
> +               channel->config.alert_low = state;
> +
> +       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_triggered_buffer_preenable(struct iio_dev *indio_dev)
> +{
> +       struct ti_ads7142_priv *priv = iio_priv(indio_dev);
> +       int ret;
> +
> +       if (!priv->config.buffer_mode)
> +               return -EINVAL;
> +
> +       /*
> +        * Start/stop burst buffer modes requires trigger
> +        */
> +       if (priv->config.buffer_mode <= TI_ADS7142_BUFFM_START_BURST &&
> +           !indio_dev->trig) {
> +               dev_err(indio_dev->dev.parent,
> +                       "%s: Start/stop burst buffer modes requires trigger",
> +                       __func__);
> +               return -EINVAL;
> +       }
> +
> +       /*
> +        * Start burst and pre/post alert modes requires irq
> +        */
> +       if (priv->config.buffer_mode >= TI_ADS7142_BUFFM_START_BURST &&
> +           !priv->irq_present) {
> +               dev_err(indio_dev->dev.parent,
> +                       "%s: Start burst and pre/post alert modes requires irq",
> +                       __func__);
> +               return -EINVAL;
> +       }
> +
> +       priv->scan_data = devm_krealloc(indio_dev->dev.parent,
> +                                       priv->scan_data,
> +                                       indio_dev->scan_bytes, GFP_KERNEL);

the scan_data doesn't look like it would be awefully big;
so, it could be static buffer on 'priv';
it should be marked with ____cacheline_aligned  though  [if moved on
the priv object].
though, i don't feel too strongly about moving it;


> +       if (!priv->scan_data)
> +               return -ENOMEM;
> +
> +       mutex_lock(&priv->lock);
> +       /*
> +        * Start burst mode started in trigger handler.
> +        * Sequencer aborted here, just for safe.
> +        */
> +       if (priv->config.buffer_mode == TI_ADS7142_BUFFM_START_BURST)
> +               ret = ti_ads7142_buffered_abort(indio_dev);
> +       else
> +               ret = ti_ads7142_buffered_setup_and_start(indio_dev);
> +       mutex_unlock(&priv->lock);
> +
> +       return ret;
> +}
> +
> +static int ti_ads7142_triggered_buffer_predisable(struct iio_dev *indio_dev)
> +{
> +       struct ti_ads7142_priv *priv = iio_priv(indio_dev);
> +       int ret = 0;
> +
> +       mutex_lock(&priv->lock);
> +       ti_ads7142_buffered_abort(indio_dev);
> +       mutex_unlock(&priv->lock);
> +
> +       return ret;
> +}
> +
> +static const struct iio_buffer_setup_ops ti_ads7142_triggered_buffer_ops = {
> +       .preenable = &ti_ads7142_triggered_buffer_preenable,
> +       .predisable = &ti_ads7142_triggered_buffer_predisable,
> +};
> +
> +static irqreturn_t ti_ads7142_trigger_handler(int irq, void *p)
> +{
> +       struct iio_poll_func *pf = p;
> +       struct iio_dev *indio_dev = pf->indio_dev;
> +       struct ti_ads7142_priv *priv = iio_priv(indio_dev);
> +       int ret = 0;
> +
> +       mutex_lock(&priv->lock);
> +       if (priv->config.buffer_mode == TI_ADS7142_BUFFM_STOP_BURST) {
> +               ret = ti_ads7142_buffered_abort(indio_dev);
> +               if (ret) {
> +                       dev_err(indio_dev->dev.parent,
> +                               "%s: error(%d) when aborting in %s mode",
> +                               __func__, ret,
> +                               ti_ads7142_buffer_modes[priv->config.buffer_mode]);
> +               }
> +
> +               ret = ti_ads7142_buffered_collect(indio_dev, NULL);
> +               if (ret && ret != -ENOENT) {
> +                       dev_err(indio_dev->dev.parent,
> +                               "%s: error(%d) when collecting result for %s mode",
> +                               __func__, ret,
> +                               ti_ads7142_buffer_modes[priv->config.buffer_mode]);
> +               }
> +
> +               if (ret == -ENOENT)
> +                       ret = 0;
> +       }
> +       if (!ret &&
> +           (priv->config.buffer_mode == TI_ADS7142_BUFFM_START_BURST ||
> +            priv->config.buffer_mode == TI_ADS7142_BUFFM_STOP_BURST)) {
> +               ret = ti_ads7142_buffered_setup_and_start(indio_dev);
> +               if (ret) {
> +                       dev_err(indio_dev->dev.parent,
> +                               "%s: error(%d) when starting %s mode",
> +                               __func__, ret,
> +                               ti_ads7142_buffer_modes[priv->config.buffer_mode]);
> +               }
> +       }
> +
> +       mutex_unlock(&priv->lock);
> +
> +       if (ret)
> +               return IRQ_NONE;

maybe doing 2 labels here (with goto err_unlock/out_unlock) would be
an idea here as well;

> +
> +       iio_trigger_notify_done(indio_dev->trig);
> +
> +       return IRQ_HANDLED;
> +}
> +
> +static int ti_ads7142_get_buffer_mode(struct iio_dev *indio_dev,
> +                                     const struct iio_chan_spec *chan)
> +{
> +       struct ti_ads7142_priv *priv = iio_priv(indio_dev);
> +
> +       return priv->config.buffer_mode;
> +}
> +
> +static int ti_ads7142_set_buffer_mode(struct iio_dev *indio_dev,
> +                                     const struct iio_chan_spec *chan,
> +                                     unsigned int mode)
> +{
> +       struct ti_ads7142_priv *priv = iio_priv(indio_dev);
> +       int ret;
> +
> +       if (mode > TI_ADS7142_BUFFM_STOP_BURST && !priv->irq_present) {
> +               dev_err(indio_dev->dev.parent,
> +                       "%s: no irq(BUSY/RDY) specified, mode %s is not supported",
> +                       __func__, ti_ads7142_buffer_modes[mode]);
> +               return -EINVAL;
> +       }
> +
> +       ret = iio_device_claim_direct_mode(indio_dev);
> +       if (ret)
> +               return ret;
> +
> +       priv->config.buffer_mode = mode;
> +       iio_device_release_direct_mode(indio_dev);
> +       return 0;
> +}
> +
> +static const struct iio_enum ti_ads7142_buffer_modes_enum = {
> +       .items = ti_ads7142_buffer_modes,
> +       .num_items = ARRAY_SIZE(ti_ads7142_buffer_modes),
> +       .get = ti_ads7142_get_buffer_mode,
> +       .set = ti_ads7142_set_buffer_mode,
> +};
> +
> +static const struct iio_chan_spec_ext_info ti_ads7142_ext_info[] = {
> +       IIO_ENUM("buffer_mode", IIO_SHARED_BY_ALL,
> +                &ti_ads7142_buffer_modes_enum),
> +       {
> +               .name = "buffer_mode_available",
> +               .shared = IIO_SHARED_BY_ALL,
> +               .read = iio_enum_available_read,
> +               .private = (uintptr_t)&ti_ads7142_buffer_modes_enum,
> +       },
> +       { },

the comma can be removed here;
since it's a null terminator;

> +};
> +
> +static int ti_ads7142_parse_channel_config(struct device *dev,
> +                                          struct iio_dev *indio_dev)
> +{
> +       struct ti_ads7142_priv *priv = iio_priv(indio_dev);
> +       struct fwnode_handle *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 channel_count;
> +       int ret;
> +
> +       channel_count = device_get_child_node_count(dev);
> +       if (!channel_count) {
> +               dev_err(dev, "dt: there is no channel definition");
> +               return -ENODEV;
> +       }
> +
> +       if (channel_count > TI_ADS7142_CHANNEL_COUNT) {
> +               dev_err(dev, "dt: invalid number of channel definitions");
> +               return -ENODEV;
> +       }
> +
> +       priv->channels = devm_kcalloc(dev, channel_count,
> +                                     sizeof(*priv->channels),
> +                                     GFP_KERNEL);
> +       if (!priv->channels)
> +               return -ENOMEM;

It looks to me that this device has up-to 2 channels.
For such a low channel count, it may make sense to just keep a static
table of iio_channels pointers [1].

> +
> +       indio_dev->num_channels = channel_count;
> +       iio_channels = devm_kcalloc(dev, channel_count, sizeof(*iio_channels),
> +                                   GFP_KERNEL);
> +       if (!iio_channels)
> +               return -ENOMEM;
> +
> +       indio_dev->channels = iio_channels;
> +
> +       device_for_each_child_node(dev, channel_node) {
> +               ads_channel = &priv->channels[channel_index];
> +
> +               ret = fwnode_property_read_u32(channel_node, "reg",
> +                                              &ads_channel->channel);
> +               if (ret) {
> +                       fwnode_handle_put(channel_node);
> +                       return ret;
> +               }
> +
> +               iio_channel = &iio_channels[channel_index];
> +               iio_channel->datasheet_name = ti_ads7142_ain_names[ads_channel->channel];
> +               iio_channel->type = IIO_VOLTAGE;
> +               iio_channel->indexed = 1;
> +               iio_channel->info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
> +               if (!IS_ERR_OR_NULL(priv->avdd))
> +                       iio_channel->info_mask_separate |= BIT(IIO_CHAN_INFO_SCALE);
> +               iio_channel->info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ);
> +               iio_channel->scan_type.sign = 'u';
> +               iio_channel->scan_type.realbits = 12;
> +               iio_channel->scan_type.storagebits = 16;
> +               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;
> +               iio_channel->event_spec = ti_ads7142_events;
> +               iio_channel->num_event_specs = ARRAY_SIZE(ti_ads7142_events);
> +               iio_channel->ext_info = ti_ads7142_ext_info;
> +
> +               ads_channel->config.high_threshold = TI_ADS7142_THRESHOLD_MSK;
> +               channel_index++;

Also, here I think it may make sense to keep a global const-static
table of iio_channel object-parameters.
Then it can be assigned to the iio_channels [1] pointer-list and then
to indio_dev->channels/num_channels.

> +       }
> +
> +       return 0;
> +}
> +
> +static int ti_ads7142_parse_config(struct device *dev,
> +                                  struct iio_dev *indio_dev)
> +{
> +       return ti_ads7142_parse_channel_config(dev, indio_dev);
> +}
> +
> +static void ti_ads7142_regulators_disable(void *data)
> +{
> +       struct ti_ads7142_priv *priv = data;
> +
> +       if (!IS_ERR_OR_NULL(priv->avdd))
> +               regulator_disable(priv->avdd);
> +       if (!IS_ERR_OR_NULL(priv->dvdd))
> +               regulator_disable(priv->dvdd);
> +}

So, I've also learned this the hard way, but the general rule of these
callbacks is one per resource.
Something like:

static void ti_ads7142_regulators_disable(void *reg)
{
        regulator_disable(reg);
}

Then some calls are needed for each regulator.
Also, IS_ERR() can be used instead of IS_ERR_OR_NULL()
{devm_}regulator_get() always returns non-NULL.


        priv->avdd = devm_regulator_get(&client->dev, "avdd");
        if (!IS_ERR(priv->avdd)) {
                ret = regulator_enable(priv->avdd);
                if (ret)
                        return ret;

                ret = devm_add_action_or_reset(&client->dev,
                                               ti_ads7142_regulators_disable,
                                               priv->avdd);
                if (ret)
                        return ret;
        }

        priv->dvdd = devm_regulator_get(&client->dev, "dvdd");
        if (!IS_ERR(priv->dvdd)) {
                ret = regulator_enable(priv->dvdd);
                if (ret)
                        return ret;

                ret = devm_add_action_or_reset(&client->dev,
                                               ti_ads7142_regulators_disable,
                                               priv->dvdd);
                if (ret)
                        return ret;
        }


> +
> +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;
> +
> +       indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*priv));
> +       if (!indio_dev)
> +               return -ENOMEM;
> +
> +       i2c_set_clientdata(client, indio_dev);
> +       priv = iio_priv(indio_dev);
> +
> +       /**
> +        * starting from v5.9-rc1 iio_device_alloc
> +        *  sets indio_dev->dev.parent, but older versions not :(
> +        **/
> +       if (!indio_dev->dev.parent)
> +               indio_dev->dev.parent = &client->dev;
> +       indio_dev->name = TI_ADS7142_NAME;
> +       indio_dev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_SOFTWARE;
> +       indio_dev->info = &ti_ads7142_iio_info;
> +
> +       priv->avdd = devm_regulator_get(&client->dev, "avdd");
> +       if (!IS_ERR_OR_NULL(priv->avdd)) {
> +               ret = regulator_enable(priv->avdd);
> +               if (ret)
> +                       return ret;
> +       }
> +
> +       priv->dvdd = devm_regulator_get(&client->dev, "dvdd");
> +       if (!IS_ERR_OR_NULL(priv->dvdd)) {
> +               ret = regulator_enable(priv->dvdd);
> +               if (ret)
> +                       goto final;
> +       }
> +
> +       ret = devm_add_action_or_reset(&client->dev,
> +                                      ti_ads7142_regulators_disable,
> +                                      priv);
> +       if (ret)
> +               goto final;
> +
> +       ret = ti_ads7142_soft_reset(client);
> +       if (ret)
> +               goto final;
> +
> +       ret = ti_ads7142_parse_config(&client->dev, indio_dev);
> +       if (ret)
> +               goto final;
> +
> +       mutex_init(&priv->lock);
> +
> +       if (client->irq) {
> +               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 final;
> +               }
> +               priv->irq_present = true;
> +       }
> +
> +       ret = devm_iio_triggered_buffer_setup(&client->dev,
> +                                             indio_dev,
> +                                             &iio_pollfunc_store_time,
> +                                             &ti_ads7142_trigger_handler,
> +                                             &ti_ads7142_triggered_buffer_ops);
> +       if (ret)
> +               goto final;
> +
> +       ret = devm_iio_device_register(&client->dev, indio_dev);

Usually "return devm_iio_device_register(&client->dev, indio_dev);" is
sufficient.
The rest is syslog spam.

> +       if (ret) {
> +               dev_err(&client->dev, "Failed to register iio device");
> +               goto final;
> +       }
> +
> +       dev_info(&client->dev, "%s is a %s device at address 0x%X",
> +                dev_name(&indio_dev->dev), indio_dev->name,
> +                client->addr);
> +final:

This 'final' label looks odd.
It looks like all 'goto final' calls in this function could be
replaced with 'return ret'
And the return below could be 'return 0;'


> +       return ret;
> +}
> +
> +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);
> +
> +       mutex_lock(&priv->lock);
> +       do {
> +               /*
> +                * Keep all regulators on when the device in autonomous
> +                *  monitoring mode.
> +                * The device can wake up the system with ALERT pin
> +                */
> +               if (priv->monitor_pending &&
> +                   (priv->config.buffer_mode == TI_ADS7142_BUFFM_PRE_ALERT ||
> +                    priv->config.buffer_mode == TI_ADS7142_BUFFM_POST_ALERT))
> +                       continue;
> +
> +               if (!IS_ERR_OR_NULL(priv->avdd))
> +                       regulator_disable(priv->avdd);
> +               if (!IS_ERR_OR_NULL(priv->dvdd))
> +                       regulator_disable(priv->dvdd);
> +       } while (0);
> +       mutex_unlock(&priv->lock);
> +
> +       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 = 0;
> +
> +       mutex_lock(&priv->lock);
> +       do {
> +               /*
> +                * Nothing to do when the device in autonomous monitoring mode.
> +                */
> +               if (priv->monitor_pending &&
> +                   (priv->config.buffer_mode == TI_ADS7142_BUFFM_PRE_ALERT ||
> +                    priv->config.buffer_mode == TI_ADS7142_BUFFM_POST_ALERT))
> +                       continue;
> +
> +               if (!IS_ERR_OR_NULL(priv->avdd)) {
> +                       ret = regulator_enable(priv->avdd);
> +                       if (ret)
> +                               continue;
> +               }
> +               if (!IS_ERR_OR_NULL(priv->dvdd)) {
> +                       ret = regulator_enable(priv->dvdd);
> +                       if (ret)
> +                               continue;
> +               }
> +       } while (0);

This while(0) loop looks a bit funky.
I understand why it's here; but it still looks a bit weird.

> +       mutex_unlock(&priv->lock);
> +
> +       return ret;
> +}
> +
> +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,
> +       .id_table       = ti_ads7142_id,
> +};
> +
> +module_i2c_driver(ti_ads7142_driver);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Jozsef Horvath <info@xxxxxxxxxxx>");
> +MODULE_DESCRIPTION("Texas Instruments ADS7142 ADC driver");
> --
> 2.17.1
>
>



[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Input]     [Linux Kernel]     [Linux SCSI]     [X.org]

  Powered by Linux