On Sat, May 15, 2021 at 10:15:16AM +0300, Alexandru Ardelean wrote: > On Fri, May 14, 2021 at 5:10 PM József Horváth <info@xxxxxxxxxxx> wrote: > > > > Hi Alexandru, > > > > On Fri, May 14, 2021 at 11:50:00AM +0300, Alexandru Ardelean wrote: > > > 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 > > > > > > ... > > > > +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. > > > > I don't like returning with pointer, but I can live with it. I'll do that. > > Personally, I don't have a strong opinion on the matter. > But kernel code tends to try to avoid double pointers [as parameters] > where possible. > > Even the IS_ERR() / PTR_ERR() constructs suggest that. This is an important info, thank you. I made the changes already. My personal preference is to separate the reason of why the pointer is NULL, and the "returned" pointer value. This is why I don't like return with pointers. > > > > > > > > > > > > > +{ > > > > + 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_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; > > > > Ok, I'll fix it here and below. > > > > > > > > > + 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 > > > > Ok, I'll remove it here and below. > > > > > > > > > + } > > > > + 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; > > > > > > > I've rethought this condition, and returning anything else than -EINVAL, is wrong. > > > > Story: > > 1. this get called first from triggered_buffer_preenable hook > > 2. the alert_ch == 0 if no thresh_{rising/falling} enabled on any channel > > 3. returning 0 from here will cause that, the user waits for nothing > > > > > > + > > > > +seq_start: > > > > + ret = ti_ads7142_sequence_start(indio_dev); > > > > + priv->monitor_pending = !ret; > > > > + > > > > + 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; > > > > > > > > > > You are right, I'll do that. > > > > > > + 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; > > > > > > > Looks better, thank you. > > > > > > + 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. > > > > > > > I'll remove these statements. > > > > > > +} > > > > + > > > > +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_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; > > > > > > > Ok, I'll do that. > > > > > > > > > + 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 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; > > > > > > > Ok, I'll do that. > > > > > > + > > > > + iio_trigger_notify_done(indio_dev->trig); > > > > + > > > > + return IRQ_HANDLED; > > > > +} > > > > + > > ... > > > > + > > > > +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; > > > > > > > Ok, I'll remove it. > > > > > > +}; > > > > + > > > > +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. > > > > This device have more features, like high precision mode(16 bits of data) and pseudo-differential mode. > > I would implement these features in the next weeks/months. > > So I would keep this dynamic initialization, and after the features above has implemented, > > I'll change it to static, if it will optimal. > > So, you can always choose to allocate iio_channels later. > > Or you can do it now, and a global-static "iio_channel[s]" object as template. > See this example: > https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git/tree/drivers/iio/adc/ad7124.c#n794 > It's essentially a memcpy() done via C compiler, where a lot of the > base information from the channel is stored in a global-const object > and copied over. > Then if something else needs tuning, it is applied from the DT,. I felt this kind of initialization less readable, but ok. > > > > > > > > > > + } > > > > + > > > > + 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); > > > } > > > > > > > Thank you for sharing your experience, I will do this > > > > > 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. > > > > The devm_regulator_get() can return NULL, if the CONFIG_REGULATOR is not selected. > > Oh right. > My bad here. [1] > > > I didnt saw any system without CONFIG_REGULATOR, but who knows... > > See: include/linux/regulator/consumer.h line:325 > > I guess the thing is that the usual construct is: > priv->avdd = devm_regulator_get(&client->dev, "avdd"); > if (IS_ERR(priv->avdd)) > return PTR(priv->avdd); > Maybe there was a broader discussion about regulators being NULL, but > from the code in other drivers, this doesn't seem to be the case. You are right, I missed the error handling here. Returned NULL value can be ok (no CONFIG_REGULATOR), but IS_ERR is exacly an error. > > > > > > > > > > > > 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. > > > > Ok, I'll do that. > > > > > > > > > + 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;' > > > > Yes, you are right, this is not required, I'll remove it. > > > > > > > > > > > > + 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. > > > > > > > It is a short block and readable. > > > > > > + 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 > > > > > > > > > > > > Thank you for review and suggestions. > > > > Best regards > > József > > > Thank you. Best regards József
