Date: Sun, 9 Jun 2024 11:52:34 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Francesco Dolcini <francesco@...cini.it>
Cc: Lars-Peter Clausen <lars@...afoo.de>, João Paulo
 Gonçalves <jpaulo.silvagoncalves@...il.com>, Liam
 Girdwood <lgirdwood@...il.com>, Mark Brown <broonie@...nel.org>,
 João Paulo Gonçalves
 <joao.goncalves@...adex.com>, linux-kernel@...r.kernel.org,
 linux-iio@...r.kernel.org, Francesco Dolcini
 <francesco.dolcini@...adex.com>
Subject: Re: [PATCH v2 2/2] iio: adc: ti-ads1119: Add driver

On Thu,  6 Jun 2024 18:35:29 +0200
Francesco Dolcini <francesco@...cini.it> wrote:

> From: João Paulo Gonçalves <joao.goncalves@...adex.com>
> 
> The ADS1119 is a precision, 16-bit, analog-to-digital converter (ADC)
> that features two differential or four single-ended inputs through a
> flexible input multiplexer (MUX), rail-to-rail input
> buffers, a programmable gain stage, a voltage reference, and an
> oscillator.
> 
> Apart from normal single conversion, the driver also supports
> continuous conversion mode using a triggered buffer. However, in this
> mode only one channel can be scanned at a time, and it only uses the data
> ready interrupt as a trigger. This is because the device channels are
> multiplexed, and using its own data ready interrupt as a trigger guarantees
> the signal sampling frequency.
> 
> Datasheet: https://www.ti.com/lit/gpn/ads1119
> Signed-off-by: João Paulo Gonçalves <joao.goncalves@...adex.com>
> Signed-off-by: Francesco Dolcini <francesco.dolcini@...adex.com>

A few more comments inline. Some of these I missed in the first
versions - sorry about that.  Takes a few passes to pick up
on everything unfortunately.

> diff --git a/drivers/iio/adc/ti-ads1119.c b/drivers/iio/adc/ti-ads1119.c
> new file mode 100644
> index 000000000000..ea0573f07279
> --- /dev/null
> +++ b/drivers/iio/adc/ti-ads1119.c
> @@ -0,0 +1,850 @@

> +
> +struct ads1119_state {
> +	unsigned long scan_masks[ADS1119_MAX_CHANNELS + 1];

Could you just use the onehot support in the IIO core?
iio_validate_scan_mask_onehot() 

That combined with the channels not being available should
provide the required restrictions I think.

> +	u8 scan_buf[ADS1119_SCAN_BUFFER_SIZE];
> +	struct completion completion;
> +	struct i2c_client *client;
> +	struct gpio_desc *reset_gpio;
> +	struct iio_trigger *trig;
> +	struct ads1119_channel_config *channels_cfg;
> +	unsigned int num_bytes_sample;
> +	unsigned int num_channels_cfg;
> +	int vref_uV;
> +};

> +
> +static int ads1119_cmd(struct ads1119_state *st, unsigned int cmd)
> +{
> +	dev_dbg(&st->client->dev, "cmd: %#x\n", cmd);
> +
> +	return i2c_smbus_write_byte(st->client, cmd);
I'm not a fan of tiny wrappers to add debug info.
The i2c core has trace points that let you get to the relevant data. Better
to use those for debug and flatten this code so we
see the actual bus accesses inline.

> +}
> +
> +static int ads1119_cmd_wreg(struct ads1119_state *st, unsigned int val)
> +{
> +	dev_dbg(&st->client->dev, "wreg: %#x\n", val);
> +
> +	return i2c_smbus_write_byte_data(st->client, ADS1119_CMD_WREG, val);
> +}
> +
> +
> +static int ads1119_get_hw_datarate(int datarate)
> +{
> +	switch (datarate) {
> +	case 90:
> +		datarate = ADS1119_DR_90_SPS;
		return ADS1...
etc

> +		break;
> +	case 330:
> +		datarate = ADS1119_DR_330_SPS;
> +		break;
> +	case 1000:
> +		datarate = ADS1119_DR_1000_SPS;
> +		break;
> +	case 20:
> +	default:
> +		datarate = ADS1119_DR_20_SPS;
> +		break;
> +	}
> +
> +	return datarate;
and this can go as unused.
> +}
> +

> +
> +static int ads1119_poll_data_ready(struct ads1119_state *st,
> +				   struct iio_chan_spec const *chan)
> +{
> +	unsigned int datarate = st->channels_cfg[chan->address].datarate;
> +	unsigned long wait_time;
> +	bool data_ready;
> +
> +	/* Sleep between poll half of the data rate */
> +	wait_time = DIV_ROUND_CLOSEST(1000000, 2 * datarate);
> +
> +	return read_poll_timeout(ads1119_data_ready,
> +				 data_ready,
> +				 (data_ready == true),
> +				 wait_time,
> +				 ADS1119_MAX_DRDY_TIMEOUT,
> +				 false,
> +				 st);
Over wrapped. Aim for closer to 80 chars.

> +}
> +
> +static int ads1119_read_data(struct ads1119_state *st,
> +			     struct iio_chan_spec const *chan,
> +			     unsigned int *val)
> +{
> +	unsigned int timeout;
> +	int ret = 0;
> +
> +	timeout = msecs_to_jiffies(ADS1119_MAX_DRDY_TIMEOUT);
> +
> +	if (!st->client->irq)
> +		ret = ads1119_poll_data_ready(st, chan);
		if (ret)
			return ret;
	} else if () {
> +	else if (!wait_for_completion_timeout(&st->completion, timeout))
> +		ret = -ETIMEDOUT;
		return -ETIMEDOUT;
}

Is probably clearer to the reader than poking a value into ret in the
timeout path just to share this error check.

> +
> +	if (ret)
> +		return ret;
> +
> +	return ads1119_cmd_rdata(st, val);
> +}
> +
> +static int ads1119_single_conversion(struct ads1119_state *st,
> +				     struct iio_chan_spec const *chan,
> +				     int *val,
> +				     bool calib_offset)
> +{
> +	struct device *dev = &st->client->dev;
> +	int mux, gain, datarate;
> +	unsigned int sample;
> +	int ret;
> +
> +	mux = st->channels_cfg[chan->address].mux;
> +	gain = st->channels_cfg[chan->address].gain;
> +	datarate = st->channels_cfg[chan->address].datarate;
> +
> +	if (calib_offset)
> +		mux = ADS1119_MUX_SHORTED;
> +
> +	ret = pm_runtime_resume_and_get(dev);
> +	if (ret)
> +		goto end;
> +
> +	ret = ads1119_configure_channel(st, mux, gain, datarate);
> +	if (ret)
> +		goto pdown;
> +
> +	ret = ads1119_cmd(st, ADS1119_CMD_START_SYNC);
> +	if (ret)
> +		goto pdown;
> +
> +	ret = ads1119_read_data(st, chan, &sample);
> +	if (ret)
> +		goto pdown;
> +
> +	*val = sign_extend32(sample, chan->scan_type.realbits - 1);
> +	ret = IIO_VAL_INT;
> +pdown:
> +	pm_runtime_mark_last_busy(dev);
> +	pm_runtime_put_autosuspend(dev);
> +end:

Just return above rather than having this label.

> +	return ret;
> +}
> +

> +
> +static int ads1119_validate_gain(struct ads1119_state *st, int scale, int uscale)
> +{
> +	int gain = 1000000 / ((scale * 1000000) + uscale);
> +
> +	switch (gain) {
> +	case 1:
> +	case 4:
> +		return gain;
Odd to calculate it if we don't need it
		return MICRO / (scale * MICRO + uscale); 
use constants as it's easy to drop a 0 in these without anyone noticing.

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

> +static int ads1119_read_raw(struct iio_dev *indio_dev,
> +			    struct iio_chan_spec const *chan, int *val,
> +			    int *val2, long mask)
> +{
> +	struct ads1119_state *st = iio_priv(indio_dev);
> +	unsigned int index = chan->address;
> +
> +	if (index >= st->num_channels_cfg)
> +		return -EINVAL;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		iio_device_claim_direct_scoped(return -EBUSY, indio_dev)
> +			return ads1119_single_conversion(st, chan, val, false);
> +		unreachable();
> +	case IIO_CHAN_INFO_OFFSET:
> +		iio_device_claim_direct_scoped(return -EBUSY, indio_dev)
> +			return ads1119_single_conversion(st, chan, val, true);
> +		unreachable();
> +	case IIO_CHAN_INFO_SCALE:
> +		*val = st->vref_uV / 1000;
> +		*val /= st->channels_cfg[index].gain;
> +		*val2 = chan->scan_type.realbits - 1;
> +		return IIO_VAL_FRACTIONAL_LOG2;
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		*val = st->channels_cfg[index].datarate;
> +		return IIO_VAL_INT;
> +	default:
> +		return -EINVAL;
> +	}
> +
Can't get here so drop this. 

> +	return 0;
> +}
> +
> +static int ads1119_write_raw(struct iio_dev *indio_dev,
> +			     struct iio_chan_spec const *chan, int val,
> +			     int val2, long mask)
> +{
> +	struct ads1119_state *st = iio_priv(indio_dev);
> +	unsigned int index = chan->address;
> +	int ret;
> +
> +	if (index >= st->num_channels_cfg)
> +		return -EINVAL;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_SCALE:
> +		ret = ads1119_validate_gain(st, val, val2);
> +		if (ret < 0)
> +			return ret;
> +
> +		st->channels_cfg[index].gain = ret;
> +		break;

		return 0; etc
No point in a reader having to go look for the what happens after
the switch statement if nothing does.


> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		ret = ads1119_validate_datarate(st, val);
> +		if (ret < 0)
> +			return ret;
> +
> +		st->channels_cfg[index].datarate = ret;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}

> +static int ads1119_set_trigger_state(struct iio_trigger *trig, bool state)
> +{
> +	struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
> +	struct ads1119_state *st = iio_priv(indio_dev);
> +	struct device *dev = &st->client->dev;
> +	unsigned int index;
> +	int ret;
> +
> +	index = find_first_bit(indio_dev->active_scan_mask,
> +			       indio_dev->masklength);
> +
> +	if (index >= st->num_channels_cfg)
> +		return -EINVAL;
> +
> +	st->num_bytes_sample =
> +		indio_dev->channels[index].scan_type.storagebits / 8;
> +
> +	ret = ads1119_set_conv_mode(st, state);
> +	if (ret)
> +		return ret;
> +
> +	if (!state) {
> +		pm_runtime_mark_last_busy(dev);
> +		return pm_runtime_put_autosuspend(dev);
> +	}

When a trigger is tightly coupled to a device, the boundaries between
what makes sense in trigger_ops and what makes sense in the buffer ones
gets blurry.

To make it easier to relax need for the trigger (typical for some boards
to not wire the interrupt), in which case we set the device going and read
at a known lower rate using a hrtimer trigger or similar, I'd suggest
moving at least some of this.

I'd expect the interrupt source enabling if it's independent of enabling
capture in here and stuff more related to channel configuration either
in update_scan_modes, or in preenable for the buffer.

So almost everything in here belongs in the preenable for the buffer and
postdisable I think...



> +
> +	ret = ads1119_configure_channel(st,
> +					st->channels_cfg[index].mux,
> +					st->channels_cfg[index].gain,
> +					st->channels_cfg[index].datarate);
> +	if (ret)
> +		return ret;
> +
> +	ret = pm_runtime_resume_and_get(dev);
> +	if (ret)
> +		return ret;
> +
> +	return ads1119_cmd(st, ADS1119_CMD_START_SYNC);
> +}


> +static int ads1119_init(struct ads1119_state *st, bool vref_external)
> +{
> +	int ret;
> +
> +	ret = ads1119_reset(st);
> +	if (ret)
> +		return ret;
> +
> +	if (vref_external)
> +		return ads1119_update_config_reg(st,
> +						 ADS1119_CONFIG_VREF_FIELD,
> +						 FIELD_PREP(ADS1119_CONFIG_VREF_FIELD,
> +							    ADS1119_VREF_EXTERNAL));
> +	return 0;
> +}
> +

Single blank line

> +
> +static int ads1119_map_analog_inputs_mux(int ain_pos, int ain_neg, bool differential)
...

> +
> +static int ads1119_probe(struct i2c_client *client)
> +{
> +	struct iio_dev *indio_dev;
> +	struct ads1119_state *st;
> +	struct device *dev = &client->dev;
> +	bool vref_external = true;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
> +	if (!indio_dev)
> +		return dev_err_probe(dev, -ENOMEM,
> +				     "Failed to allocate IIO device\n");
> +
> +	st = iio_priv(indio_dev);
> +	st->client = client;
> +
> +	indio_dev->name = "ads1119";
> +	indio_dev->info = &ads1119_info;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +
> +	i2c_set_clientdata(client, indio_dev);
> +
> +	st->vref_uV = devm_regulator_get_enable_read_voltage(dev, "vref");
> +	if (st->vref_uV == -ENODEV) {
> +		vref_external = false;
> +		st->vref_uV = ADS1119_VREF_INTERNAL_VAL;
> +	} else if (st->vref_uV < 0) {
> +		return dev_err_probe(dev, st->vref_uV, "Failed to get vref\n");
> +	}
> +
> +	st->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
> +	if (IS_ERR(st->reset_gpio))
> +		return dev_err_probe(dev, PTR_ERR(st->reset_gpio),
> +				     "Failed to get reset gpio\n");
> +
> +	ret = ads1119_alloc_and_config_channels(indio_dev);
> +	if (ret)
> +		return ret;
> +
> +	init_completion(&st->completion);
> +
> +	if (client->irq > 0) {
> +		ret = devm_request_threaded_irq(dev,
> +						client->irq,
> +						ads1119_irq_handler,
> +						NULL,
> +						IRQF_TRIGGER_FALLING,
> +						"ads1119",
> +						indio_dev);
> +		if (ret)
> +			return dev_err_probe(dev, ret, "Failed to allocate irq\n");
> +
> +		st->trig = devm_iio_trigger_alloc(dev, "%s-dev%d",
> +						  indio_dev->name,
> +						  iio_device_id(indio_dev));
> +		if (!st->trig)
> +			return dev_err_probe(dev, -ENOMEM,
> +					     "Failed to allocate IIO trigger\n");
> +
> +		st->trig->ops = &ads1119_trigger_ops;
> +		iio_trigger_set_drvdata(st->trig, indio_dev);
> +
> +		ret = devm_iio_trigger_register(dev, st->trig);
> +		if (ret)
> +			return dev_err_probe(dev, ret,
> +					     "Failed to register IIO trigger\n");
> +
> +		indio_dev->trig = iio_trigger_get(st->trig);
> +
> +		ret = devm_iio_triggered_buffer_setup(dev, indio_dev, NULL,
> +						      ads1119_trigger_handler,
> +						      NULL);

Is the device unable to use other triggers? You have the trigger validating
it is being used with the device, but nothing stopping the device being
disconnected from it's own trigger and connected to another one.
The validate_trigger callback IIRC.
We also have iio_trigger_set_immutable() to stop the unbind.

If it's not too hard to make it work with other triggers that can be
a very useful feature.


> +		if (ret)
> +			return dev_err_probe(dev, ret,
> +					     "Failed to setup IIO buffer\n");
> +	}

...

> +
> +static int ads1119_runtime_suspend(struct device *dev)
> +{
> +	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
> +	struct ads1119_state *st = iio_priv(indio_dev);
> +
> +	return ads1119_cmd(st, ADS1119_CMD_POWERDOWN);

I'd like an additional call of this power down command in the driver removal path
via a devm_add_action_or_reset() as we always want to leave a device
powered down if we rip out the userspace intefaces!

In many cases it might be powered down anyway due to runtimepm, but
it might not by coincidence of timing, or because runtime_pm isn't built
or enabled for some reason.


> +}
> +
> +/*
> + * The ADS1119 does not require a resume function because it automatically powers
> + * on after a reset.
> + * After a power down command, the ADS1119 can still communicate but turns off its
> + * analog parts. To resume from power down, the device will power up again
> + * upon receiving a start/sync command.
> + */
> +static DEFINE_RUNTIME_DEV_PM_OPS(ads1119_pm_ops,
> +				 ads1119_runtime_suspend,
> +				 NULL,
> +				 NULL);
> +
Single blank line here, not 2.
> +
> +static const struct of_device_id __maybe_unused ads1119_of_match[] = {

Powered by blists - more mailing lists