Re: [PATCH 2/2] iio: adc: ti-ads112c14: add continuous mode support

[Jonathan Cameron <jic23@kernel.org>]

On Fri, 24 Jul 2026 15:13:11 -0500
"David Lechner (TI)" <dlechner@baylibre.com> wrote:

> Add support for continuous mode in the TI ADS112C14 ADC driver. In this
> mode the ADC itself is starting each conversion, so we add a trigger
> based on the DRDY interrupt to read each sample. This mode is also
> limited in that only one channel can be enabled at a time since the
> chip does not have a sequencer or simultaneous sampling capability.
> Continuous mode will only be used when this new trigger is the current
> trigger.
> 
> Signed-off-by: David Lechner (TI) <dlechner@baylibre.com>
Hi David,

A few comments inline.

Thanks,

Jonathan

> ---
>  drivers/iio/adc/ti-ads112c14.c | 200 ++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 178 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/iio/adc/ti-ads112c14.c b/drivers/iio/adc/ti-ads112c14.c
> index 57e301c15314..f3a2eba55362 100644
> --- a/drivers/iio/adc/ti-ads112c14.c
> +++ b/drivers/iio/adc/ti-ads112c14.c

> @@ -601,6 +625,32 @@ static int ads112c14_prepare_sys_mon_channel(struct ads112c14_data *data,
>  	return 0;
>  }
>  
> +static int ads112c14_prepare_channel(struct ads112c14_data *data,
> +				     const struct iio_chan_spec *chan)
> +{
> +	if (chan->channel < ADS112C14_SYS_MON_CHANNEL_BASE)
> +		return ads112c14_prepare_measurement_channel(data, chan);
> +
> +	return ads112c14_prepare_sys_mon_channel(data, chan);
> +}

Left these visible as referred to below.

> +
> +static int ads112c14_scan_read(struct ads112c14_data *data, u8 *buf)
> +{
> +	struct i2c_client *client = to_i2c_client(regmap_get_device(data->regmap));
> +	int ret;
> +	u8 len;
> +
> +	len = BITS_TO_BYTES(data->chip_info->resolution_bits);
> +	if (data->i2c_crc_enabled)
> +		len += 1;
> +
> +	ret = i2c_smbus_read_i2c_block_data(client, ADS112C14_CMD_RDATA, len, buf);
> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;
> +}
> +
>  static int ads112c14_single_conversion(struct ads112c14_data *data,
>  				       const struct iio_chan_spec *chan,
>  				       u8 *buf, bool for_scan)
> @@ -611,15 +661,9 @@ static int ads112c14_single_conversion(struct ads112c14_data *data,
>  
>  	guard(mutex)(&data->lock);
>  
> -	if (chan->channel < ADS112C14_SYS_MON_CHANNEL_BASE) {
> -		ret = ads112c14_prepare_measurement_channel(data, chan);
> -		if (ret)
> -			return ret;
> -	} else {
> -		ret = ads112c14_prepare_sys_mon_channel(data, chan);
> -		if (ret)
> -			return ret;
> -	}
> +	ret = ads112c14_prepare_channel(data, chan);

Minor obviously but maybe have these factoring out changes (to enable reuse
in the new code) as a precursor step?

> +	if (ret)
> +		return ret;
>  
>  	if (data->drdy_irq) {
>  		reinit_completion(&data->drdy_completion);
> @@ -655,17 +699,8 @@ static int ads112c14_single_conversion(struct ads112c14_data *data,
>  	 * with CRC errors, but rather leave it to userspace to decide what to
>  	 * do.
>  	 */
> -	if (for_scan) {
> -		u8 len = BITS_TO_BYTES(data->chip_info->resolution_bits) +
> -			 (data->i2c_crc_enabled ? 1 : 0);
> -
> -		ret = i2c_smbus_read_i2c_block_data(client, ADS112C14_CMD_RDATA,
> -						    len, buf);
> -		if (ret < 0)
> -			return ret;
> -
> -		return 0;
> -	}
> +	if (for_scan)
> +		return ads112c14_scan_read(data, buf);

Similar on perhaps factoring this out prior to the main change here

> +static bool ads112c14_validate_scan_mask(struct iio_dev *indio_dev,
> +					 const unsigned long *mask)
> +{
> +	if (!ads112c14_using_drdy_trigger(indio_dev))
> +		return true;
> +
> +	return bitmap_weight(mask, iio_get_masklength(indio_dev)) == 1;

Maybe use iio_validate_mask_onehot() as a kind of really obvious documentation
of this final line?

> +}
> +
> +static int ads112c14_buffer_postenable(struct iio_dev *indio_dev)
> +{
> +	struct ads112c14_data *data = iio_priv(indio_dev);
> +	unsigned int scan_mask_len = iio_get_masklength(indio_dev);
> +	unsigned int i;
> +	const struct iio_chan_spec *chan;
> +	int ret;

Might as well target reverse xmas tree where you can.

> +
> +	if (!ads112c14_using_drdy_trigger(indio_dev))
> +		return 0;
> +
> +	i = find_first_bit(indio_dev->active_scan_mask, scan_mask_len);
> +	if (i >= scan_mask_len)
> +		return -EINVAL;

> +
> +static int ads112c14_buffer_predisable(struct iio_dev *indio_dev)
> +{
> +	struct ads112c14_data *data = iio_priv(indio_dev);
> +	int ret, ret2;
> +
> +	if (!ads112c14_using_drdy_trigger(indio_dev))
> +		return 0;
> +
> +	guard(mutex)(&data->lock);
> +
> +	ret = regmap_write(data->regmap, ADS112C14_REG_CONVERSION_CTRL,
> +			   ADS112C14_CONVERSION_CTRL_STOP);
> +	ret2 = regmap_assign_bits(data->regmap, ADS112C14_REG_DEVICE_CFG,
> +				  ADS112C14_DEVICE_CFG_CONV_MODE,
> +				  ADS112C14_DEVICE_CFG_CONV_MODE_SINGLE_SHOT);

This is odd looking code. Needs comments on why we should prefer returning
the error for the second call over that for the first.

> +	if (ret2)
> +		return ret2;
> +
> +	return ret;
> +}


>  static int ads112c14_populate_idac_mag(u32 current_nA, u8 *idac_mag)
>  {
>  	u32 current_uA = current_nA / (NANO / MICRO);
> @@ -1458,6 +1598,21 @@ static int ads112c14_probe(struct i2c_client *client)
>  				       IRQF_NO_AUTOEN, dev_name(dev), indio_dev);
>  		if (ret)
>  			return ret;
> +
> +		data->drdy_trig = devm_iio_trigger_alloc(dev, "%s-dev%d-drdy",
> +							 info->name,
> +							 iio_device_id(indio_dev));
> +		if (!data->drdy_trig)
> +			return -ENOMEM;
> +
> +		data->drdy_trig->ops = &ads112c14_trigger_ops;
> +		iio_trigger_set_drvdata(data->drdy_trig, indio_dev);
> +
> +		ret = devm_iio_trigger_register(dev, data->drdy_trig);
> +		if (ret)
> +			return ret;
> +
> +		indio_dev->trig = iio_trigger_get(data->drdy_trig);

Given you are intending to let the trigger choice select behaviour, I'd
not have any default trigger. It's not obvious that the devices own trigger
is the right 'default' given the limitations around its use!

Jonathan

>  	}