Message-ID: <20240622194407.4ee2a342@jic23-huawei>
Date: Sat, 22 Jun 2024 19:44:07 +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 v3 2/2] iio: adc: ti-ads1119: Add driver

On Mon, 17 Jun 2024 20:39:05 +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.
> 
> 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>

Hi. My suggested changes are minor, so I just made them whilst applying.
Please check the result!

Applied with these tweaks a few more whitespace things checkpatch --strict
pointed out to the togreg branch of iio.git and pushed out as testing for
0-day to see what we missed.

Thanks,

Jonathan

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

> +
> +#define ADS1119_V_CHAN(_chan, _chan2, _addr) {			\
Why pass in the parameters as you use this in one place and they
are all 0?

Just put this definition inline where you use it as

	const struct iio_chan_spec ads1119_channel =
		(const struct iio_chan_spec) {
			.type = IIO_VOLTAGE, 
	etc.
(or something along these lines.

		};
> +	.type = IIO_VOLTAGE,					\
> +	.indexed = 1,						\
> +	.address = _addr,					\
> +	.channel = _chan,					\
> +	.channel2 = _chan2,					\
> +	.info_mask_separate =					\
> +		BIT(IIO_CHAN_INFO_RAW) |			\
> +		BIT(IIO_CHAN_INFO_SCALE) |			\
> +		BIT(IIO_CHAN_INFO_OFFSET) |			\
> +		BIT(IIO_CHAN_INFO_SAMP_FREQ),			\
> +	.info_mask_shared_by_all_available =			\
> +		BIT(IIO_CHAN_INFO_SCALE) |			\
> +		BIT(IIO_CHAN_INFO_SAMP_FREQ),			\
> +	.scan_index = _addr,					\
> +	.scan_type = {						\
> +		.sign = 's',					\
> +		.realbits = 16,					\
> +		.storagebits = 16,				\
> +		.endianness = IIO_CPU,				\
> +	},							\
> +}
> +
> +struct ads1119_channel_config {
> +	int gain;
> +	int datarate;
> +	int mux;
> +};
> +
> +struct ads1119_state {
> +	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_channels_cfg;
> +	unsigned int cached_config;
> +	int vref_uV;
> +};
> +
> +static const char * const ads1119_power_supplies[] = {
> +	"avdd", "dvdd"
> +};
> +
> +static const int ads1119_available_datarates[] = {
> +	20, 90, 330, 1000,
> +};
> +
> +static const int ads1119_available_gains[] = {
> +	1, 1,
> +	1, 4,
> +};
> +
> +static int ads1119_upd_cfg_reg(struct ads1119_state *st, unsigned int fields,
> +			       unsigned int val)
> +{
> +	unsigned int config = st->cached_config;
> +	int ret;
> +
> +	config &= ~fields;
> +	config |= val;
> +
> +	ret = i2c_smbus_write_byte_data(st->client, ADS1119_CMD_WREG, config);
> +	if (ret)
> +		return ret;
> +
> +	st->cached_config = config;
> +
> +	return 0;
> +}
> +
> +static bool ads1119_data_ready(struct ads1119_state *st)
> +{
> +	int status;
> +
> +	status = i2c_smbus_read_byte_data(st->client, ADS1119_CMD_RREG_STATUS);
> +	if (status < 0)
> +		return false;
> +
> +	return !!FIELD_GET(ADS1119_STATUS_DRDY_FIELD, status);

The !! doesn't add anything as the cast to bool effectively does the same thing


> +}

> +
> +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;
Slightly neater to do this at declaration fo variables.

	int mux = st->...
etc.

> +
> +	if (calib_offset)
> +		mux = ADS1119_MUX_SHORTED;
> +
> +	ret = pm_runtime_resume_and_get(dev);
> +	if (ret)
> +		goto pdown;
> +
> +	ret = ads1119_configure_channel(st, mux, gain, datarate);
> +	if (ret)
> +		goto pdown;
> +
> +	ret = i2c_smbus_write_byte(st->client, 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);
> +	return ret;
> +}

> +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;
> +
> +	/* Poll 5 times more than the data rate */
> +	wait_time = DIV_ROUND_CLOSEST(MICRO, 5 * datarate);
> +
> +	return read_poll_timeout(ads1119_data_ready, data_ready,
> +				 data_ready == true, wait_time,

checkpatch --strict pointed out that you might as well just use dataready
as the conditional, rather than dataready == true.

> +				 ADS1119_MAX_DRDY_TIMEOUT, false, st);
> +}

> +
> +static int ads1119_alloc_and_config_channels(struct iio_dev *indio_dev)
> +{
> +	const struct iio_chan_spec ads1119_channel = ADS1119_V_CHAN(0, 0, 0);
> +	const struct iio_chan_spec ads1119_ts = IIO_CHAN_SOFT_TIMESTAMP(0);
> +	struct ads1119_state *st = iio_priv(indio_dev);
> +	struct iio_chan_spec *iio_channels, *chan;
> +	struct device *dev = &st->client->dev;
> +	unsigned int num_channels, i;
> +	bool differential;
> +	u32 ain[2];
> +	int ret;
> +
> +	st->num_channels_cfg = device_get_child_node_count(dev);
> +	if (st->num_channels_cfg > ADS1119_MAX_CHANNELS)
> +		return dev_err_probe(dev, -EINVAL,
> +				     "Too many channels %d, max is %d\n",
> +				     st->num_channels_cfg,
> +				     ADS1119_MAX_CHANNELS);
> +
> +	st->channels_cfg = devm_kcalloc(dev, st->num_channels_cfg,
> +					sizeof(*st->channels_cfg), GFP_KERNEL);
> +	if (!st->channels_cfg)
> +		return -ENOMEM;
> +
> +	/* Allocate one more iio channel for the timestamp */
> +	num_channels = st->num_channels_cfg + 1;
> +	iio_channels = devm_kcalloc(dev, num_channels,
> +				    sizeof(*iio_channels),
> +				    GFP_KERNEL);
Can wrap that on one fewer line.
> +	if (!iio_channels)
> +		return -ENOMEM;
> +
> +	i = 0;
> +
>
>
> +static int ads1119_probe(struct i2c_client *client)
> +{
...

> +	if (client->irq > 0) {
> +		ret = devm_request_threaded_irq(dev,
> +						client->irq,
Can combine some of these lines under 80 chars.
> +						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");
> +	}
> +
> +	ret = ads1119_init(st, vref_external);
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "Failed to initialize device\n");
> +
> +	pm_runtime_set_autosuspend_delay(dev, ADS1119_SUSPEND_DELAY);
> +	pm_runtime_use_autosuspend(dev);
> +	pm_runtime_mark_last_busy(dev);
> +	pm_runtime_set_active(dev);
> +
> +	ret = devm_pm_runtime_enable(dev);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to enable pm runtime\n");
> +
> +	ret = devm_add_action_or_reset(dev, ads1119_powerdown, st);
Hmm. This naturally (I think anyway) pairs with the reset in ads1119_init()
so it's a little odd to find it down here. 

I assume the reasoning was that the runtime PM might result in it being on?
However I think the worst that can happen is that runtime pm powers down
the powered down device just before it is stopped by the devm_pm_runtime
enable cleanup. So this ordering doesn't matter.

Hence I'll leave it alone, but it did make me think for a few minutes.


> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "Failed to add powerdown action\n");
> +
> +	return devm_iio_device_register(dev, indio_dev);
> +}

> +/*
> + * 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);

Trivial but can rewrap those lines to save 2 lines without significant loss
of readability.

Powered by blists - more mailing lists