Re: [PATCH v4 3/3] drivers: iio: adc: add support for ad777x family

[Jonathan Cameron <jic23@kernel.org>]

On Wed, 24 Jul 2024 18:54:41 +0300
Ramona Alexandra Nechita <ramona.nechita@analog.com> wrote:

> Add support for AD7770, AD7771, AD7779 ADCs. The device is capable of
> sending out data both on DOUT lines interface,as on the SDO line.
> The driver currently implements only theSDO data streaming mode. SPI
> communication is used alternatively foraccessingregisters and streaming
> data. Register access are protected by crc8.
> 
> Signed-off-by: Ramona Alexandra Nechita <ramona.nechita@analog.com>
Hi Ramona,

Various comments inline.  Key one though is make sure you read your
own code before posting as there is a bunch of left over stuff
from updates still in the code.

Jonathan

> diff --git a/drivers/iio/adc/ad7779.c b/drivers/iio/adc/ad7779.c
> new file mode 100644
> index 000000000000..7a83977fd00c
> --- /dev/null
> +++ b/drivers/iio/adc/ad7779.c

> +
> +static int ad7779_spi_write(struct ad7779_state *st, u8 reg, u8 val)
> +{
> +	int length = 3;
> +	struct spi_transfer reg_write_tr[] = {
> +		{
> +			.tx_buf = st->reg_tx_buf,
> +		},
> +	};
> +
> +	if (reg == AD7779_REG_GEN_ERR_REG_1_EN)
> +		length = 2;
> +	reg_write_tr[0].len = length;
> +
> +	st->reg_tx_buf[0] = FIELD_GET(AD7779_REG_MSK, reg);
> +	st->reg_tx_buf[1] = val;
> +	st->reg_tx_buf[2] = crc8(ad7779_crc8_table, st->reg_tx_buf, 2, 0);
> +
> +	return spi_sync_transfer(st->spi, reg_write_tr, ARRAY_SIZE(reg_write_tr));

spi_write()?

> +}


> +static int ad7779_set_sampling_frequency(struct ad7779_state *st,
> +					 unsigned int sampling_freq)
> +{
...

> +
> +	if (st->power_mode == AD7779_LOW_POWER)

At the moment we only end up in low power mode via suspend I think.
So this logic and indeed the tracking of power mode isn't yet needed.

> +		div = AD7779_LOWPOWER_DIV;
> +	else
> +		div = AD7779_HIGHPOWER_DIV;
> +


> +
> +static int ad7779_get_calibscale(struct ad7779_state *st, int channel)
> +{
> +	int ret;
> +	u8 calibscale[3];
> +	// u8 low, mid, high;
> +	ret = ad7779_spi_read(st, AD7779_REG_CH_GAIN_LOWER_BYTE(channel), &calibscale[0]);
> +	if (ret)
> +		return ret;
> +
> +	ret = ad7779_spi_read(st, AD7779_REG_CH_GAIN_MID_BYTE(channel), &calibscale[1]);
> +	if (ret)
> +		return ret;
> +
> +	ret = ad7779_spi_read(st, AD7779_REG_CH_GAIN_UPPER_BYTE(channel), &calibscale[2]);
> +	if (ret)
> +		return ret;
> +
> +	// return FIELD_PREP(AD7779_UPPER, high) | FIELD_PREP(AD7779_MID, mid) |
> +	//        FIELD_PREP(AD7779_LOWER, low);

Remember to cleanup before posting.  Always read through your own patch for this
sort of leftover stuff or get someone else to do it before posting (as can be hard
to spot in your own code).


> +	return get_unaligned_be24(calibscale);
> +}

> +
> +static int ad7779_set_calibbias(struct ad7779_state *st, int channel, int val)
> +{
> +	int ret;
> +	u8 calibbias[3];
> +	u8 msb, mid, lsb;
> +
> +	msb = FIELD_GET(AD7779_UPPER, val);
> +	mid = FIELD_GET(AD7779_MID, val);
> +	lsb = FIELD_GET(AD7779_LOWER, val);
? left over code?
> +	put_unaligned_be24(val, calibbias);
> +	ret = ad7779_spi_write(st,
> +			       AD7779_REG_CH_OFFSET_UPPER_BYTE(channel),
> +			       calibbias[0]);
> +	if (ret)
> +		return ret;
> +
> +	ret = ad7779_spi_write(st,
> +			       AD7779_REG_CH_OFFSET_MID_BYTE(channel),
> +			       calibbias[1]);
> +	if (ret)
> +		return ret;
> +
> +	return ad7779_spi_write(st,
> +				AD7779_REG_CH_OFFSET_LOWER_BYTE(channel),
> +				calibbias[2]);
> +}
> +
> +static int ad7779_read_raw(struct iio_dev *indio_dev,
> +			   struct iio_chan_spec const *chan,
> +			   int *val,
> +			   int *val2,
> +			   long mask)
> +{
> +	struct ad7779_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
> +		switch (mask) {
> +		case IIO_CHAN_INFO_CALIBSCALE:
> +			*val = ad7779_get_calibscale(st, chan->channel);
> +			iio_device_release_direct_mode(indio_dev);
> +			if (*val < 0)
> +				return -EINVAL;
> +			*val2 = GAIN_REL;
> +			return IIO_VAL_FRACTIONAL;
> +		case IIO_CHAN_INFO_CALIBBIAS:
> +			*val = ad7779_get_calibbias(st, chan->channel);
> +			iio_device_release_direct_mode(indio_dev);
> +			if (*val < 0)
> +				return -EINVAL;
> +			return IIO_VAL_INT;
> +		case IIO_CHAN_INFO_SAMP_FREQ:
> +			*val = st->sampling_freq;
> +			iio_device_release_direct_mode(indio_dev);
> +			if (*val < 0)
> +				return -EINVAL;
> +			return IIO_VAL_INT;
> +		}
> +		return -EINVAL;
> +	}
> +	unreachable();
> +}
> +
> +static int ad7779_write_raw(struct iio_dev *indio_dev,
> +			    struct iio_chan_spec const *chan,
> +			    int val,
> +			    int val2,
No need to wrap so much.  Stick a few of these on one line.

> +			    long mask)
> +{
> +	struct ad7779_state *st = iio_priv(indio_dev);
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_CALIBSCALE:
> +		return ad7779_set_calibscale(st, chan->channel, val2);

We have to take the direct_mode lock to read them but not write?
That seems backwards. Should always be able to read, but write
should be stopped if buffered capture in flight.


> +	case IIO_CHAN_INFO_CALIBBIAS:
> +		return ad7779_set_calibbias(st, chan->channel, val);
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		return ad7779_set_sampling_frequency(st, val);
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int ad7779_buffer_preenable(struct iio_dev *indio_dev)
> +{
> +	int ret;
> +	struct ad7779_state *st = iio_priv(indio_dev);
> +
> +	ret = ad7779_spi_write_mask(st,
> +				    AD7779_REG_GENERAL_USER_CONFIG_3,
> +				    AD7779_MOD_SPI_EN_MSK,
> +				    FIELD_PREP(AD7779_MOD_SPI_EN_MSK, 1));
> +	if (ret)
> +		return ret;
> + 
> +	/* DRDY output cannot be disabled at device level

Comment syntax needs fixing.

> +	 * therefore we mask the irq at host end.
> +	 */
> +	enable_irq(st->spi->irq);
> +
> +	return 0;
> +}
> +
> +static int ad7779_buffer_postdisable(struct iio_dev *indio_dev)
> +{
> +	struct ad7779_state *st = iio_priv(indio_dev);
> +
> +	disable_irq(st->spi->irq);
> +
> +	return ad7779_spi_write(st, AD7779_REG_GENERAL_USER_CONFIG_3,
> +			       AD7779_DISABLE_SD);
> +}
> +
> +static irqreturn_t ad7779_trigger_handler(int irq, void *p)
> +{
> +	struct iio_poll_func *pf = p;
> +	struct iio_dev *indio_dev = pf->indio_dev;
> +	struct ad7779_state *st = iio_priv(indio_dev);
> +	int ret;
> +	int bit;
> +	int k = 0;
> +	/* Each channel shifts out HEADER + 24 bits of data

Wrong comment syntax, and also early than necessary line wrap.

> +	 * therefore 8 * u32 for the data and 64 bits for
> +	 * the timestamp
> +	 */
> +	u32 tmp[10]; 
> +
> +	struct spi_transfer sd_readback_tr[] = {
> +		{
> +			.rx_buf = st->spidata_rx,
> +			.tx_buf = st->spidata_tx,
> +			.len = 32,

Why 32?  Good to add some maths or a comment on the sizing.

> +		}
> +	};
> +
> +	if (!iio_buffer_enabled(indio_dev)){
> +		iio_trigger_notify_done(indio_dev->trig);
> +		return IRQ_HANDLED;

use a goto.

> +	}
> +
> +	st->spidata_tx[0] = AD7779_SPI_READ_CMD;
> +	ret = spi_sync_transfer(st->spi, sd_readback_tr,
> +				ARRAY_SIZE(sd_readback_tr));
> +	if (ret) {
> +		dev_err(&st->spi->dev,
> +			"spi transfer error in irq handler");
goto.

> +		iio_trigger_notify_done(indio_dev->trig);
> +		return IRQ_HANDLED;
> +	}
> +
> +	for_each_set_bit(bit, indio_dev->active_scan_mask, AD7779_NUM_CHANNELS - 1)
> +		tmp[k++] = st->spidata_rx[bit];

Update this to use Nuno's new macros for iterating over the scan mask.

> +
> +	iio_push_to_buffers_with_timestamp(indio_dev, &tmp[0], pf->timestamp);
> +
> +	iio_trigger_notify_done(indio_dev->trig);
> +
> +	return IRQ_HANDLED;
> +}

> +static int ad7779_powerup(struct ad7779_state *st, struct gpio_desc *start_gpio)
> +{
> +	int ret;
> +	
> +	ret = ad7779_spi_write_mask(st, AD7779_REG_GEN_ERR_REG_1_EN,
> +				    AD7779_SPI_CRC_EN_MSK,
> +				    FIELD_PREP(AD7779_SPI_CRC_EN_MSK, 1));
> +	if (ret)
> +		return ret;
> +
> +	ret = ad7779_spi_write_mask(st, AD7779_REG_GENERAL_USER_CONFIG_1,
> +				    AD7779_MOD_POWERMODE_MSK,
> +				    FIELD_PREP(AD7779_MOD_POWERMODE_MSK, 1));

Do these need to be done separately?  Or can a single write be used to write
both fields?

> +	if (ret)
> +		return ret;
> +
> +	ret = ad7779_spi_write_mask(st, AD7779_REG_GENERAL_USER_CONFIG_1,
> +				    AD7779_MOD_PDB_REFOUT_MSK,
> +				    FIELD_PREP(AD7779_MOD_PDB_REFOUT_MSK, 1));
> +	if (ret)
> +		return ret;
> +
> +	ret = ad7779_spi_write_mask(st, AD7779_REG_DOUT_FORMAT,
> +				    AD7779_DCLK_CLK_DIV_MSK,
> +				    FIELD_PREP(AD7779_DCLK_CLK_DIV_MSK, 1));
> +	if (ret)
> +		return ret;
> +
> +	ret = ad7779_spi_write_mask(st, AD7779_REG_ADC_MUX_CONFIG,
> +				    AD7779_REFMUX_CTRL_MSK,
> +				    FIELD_PREP(AD7779_REFMUX_CTRL_MSK, 1));
> +	if (ret)
> +		return ret;
> +
> +
> +	st->power_mode = AD7779_HIGH_POWER;
> +	ret = ad7779_set_sampling_frequency(st, AD7779_DEFAULT_SAMPLING_FREQ);
> +	if (ret)
> +		return ret;
> +
> +	gpiod_set_value(start_gpio, 0);
> +	/* Start setup time */
> +	fsleep(15);
> +	gpiod_set_value(start_gpio, 1);
> +	fsleep(15);
> +	gpiod_set_value(start_gpio, 0);
> +	fsleep(15);
> +
> +	return 0;
> +}
> +
> +static int ad7779_probe(struct spi_device *spi)
> +{

...

> +	indio_dev->name = st->chip_info->name;
> +	indio_dev->info = &ad7779_info;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->channels = st->chip_info->channels;
> +	indio_dev->num_channels = ARRAY_SIZE(ad7779_channels);
> +
> +	st->trig = devm_iio_trigger_alloc(&spi->dev, "%s-dev%d",
> +					  		indio_dev->name, iio_device_id(indio_dev));

As has already been commented on. Fix all this alignment stuff.

> +	if (!st->trig)
> +		return -ENOMEM;
> +
> +	st->trig->ops= &ad7779_trigger_ops;
> +	st->trig->dev.parent = &spi->dev;

devm_iio_trigger_alloc does this for you.
https://elixir.bootlin.com/linux/v6.10/source/drivers/iio/industrialio-trigger.c#L563

> +
> +	iio_trigger_set_drvdata(st->trig, st);
> +
> +	ret = devm_request_irq(&spi->dev, spi->irq,
> +						   iio_trigger_generic_data_rdy_poll,
> +						   IRQF_ONESHOT | IRQF_NO_AUTOEN,
> +						   indio_dev->name, st->trig);
> +	if (ret)
> +		return dev_err_probe(&spi->dev, ret, "request irq %d failed\n",
> +				     st->spi->irq);

...

> +	return devm_iio_device_register(&spi->dev, indio_dev);
> +}
> +
> +static int __maybe_unused ad7779_suspend(struct device *dev)
> +{
> +	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> +	struct ad7779_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	ret = ad7779_spi_write_mask(st, AD7779_REG_GENERAL_USER_CONFIG_1,
> +				    AD7779_MOD_POWERMODE_MSK,
> +				    FIELD_PREP(AD7779_MOD_POWERMODE_MSK,
> +					       AD7779_LOW_POWER));
> +	if (ret)
> +		return ret;
> +
> +	st->power_mode = AD7779_LOW_POWER;
As above. There should be no need to track this as you only change it
in suspend / resume.

> +	return 0;
> +}
> +
> +static int __maybe_unused ad7779_resume(struct device *dev)

Don't need __maybe_unused as DEFINE_SIMPLE_DEV_PM_OPS and pm_sleep_ptr() ensure
these functions are visible to the compiler, but that it them removes them
if they aren't in use.

> +{
> +	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> +	struct ad7779_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	ret = ad7779_spi_write_mask(st, AD7779_REG_GENERAL_USER_CONFIG_1,
> +				    AD7779_MOD_POWERMODE_MSK,
> +				    FIELD_PREP(AD7779_MOD_POWERMODE_MSK,
> +					       AD7779_HIGH_POWER));
> +	if (ret)
> +		return ret;
> +
> +	st->power_mode = AD7779_HIGH_POWER;
> +
> +	return 0;
> +}