Message-ID: <20240914180648.592cd69e@jic23-huawei>
Date: Sat, 14 Sep 2024 18:06:48 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Ramona Alexandra Nechita <ramona.nechita@...log.com>
Cc: Lars-Peter Clausen <lars@...afoo.de>, Cosmin Tanislav
 <cosmin.tanislav@...log.com>, Michael Hennerich
 <Michael.Hennerich@...log.com>, Rob Herring <robh@...nel.org>, "Krzysztof
 Kozlowski" <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, Nuno
 Sa <nuno.sa@...log.com>, Andy Shevchenko <andy@...nel.org>, David Lechner
 <dlechner@...libre.com>, Marcelo Schmitt <marcelo.schmitt@...log.com>,
 Olivier Moysan <olivier.moysan@...s.st.com>, Dumitru Ceclan
 <mitrutzceclan@...il.com>, Matteo Martelli <matteomartelli3@...il.com>,
 AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>,
 Alisa-Dariana Roman <alisadariana@...il.com>, Ivan Mikhaylov
 <fr0st61te@...il.com>, Mike Looijmans <mike.looijmans@...ic.nl>,
 <linux-iio@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
 <devicetree@...r.kernel.org>
Subject: Re: [PATCH v5 3/3] drivers: iio: adc: add support for ad777x family

On Thu, 12 Sep 2024 15:15:47 +0300
Ramona Alexandra Nechita <ramona.nechita@...log.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 the SDO data streaming mode. SPI
> communication is used alternatively for accessing registers and streaming
> data. Register access are protected by crc8.
> 
> Signed-off-by: Ramona Alexandra Nechita <ramona.nechita@...log.com>
Hi Ramona,

A few additional comments inline,

Jonathan
> diff --git a/drivers/iio/adc/ad7779.c b/drivers/iio/adc/ad7779.c
> new file mode 100644
> index 000000000000..05ae72257c9e
> --- /dev/null
> +++ b/drivers/iio/adc/ad7779.c
> @@ -0,0 +1,917 @@

> +static int ad7779_set_calibbias(struct ad7779_state *st, int channel, int val)
> +{
> +	int ret;
> +	u8 calibbias[3];
> +
> +	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),
Wrap closer to 80 chars.
	return ad7779_spi_write(st, AD7779_REG_CH_OFFSET_LOWER_BYTE(channel),
				calibbias[2]);

etc as it'll shorted the code a little bit for no significant loss
of readability.  Do the same for all other such cases.
> +}

> +
> +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 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 = AD7779_NUM_CHANNELS * AD7779_CHAN_DATA_SIZE,
> +		}
> +	};
> +
> +	if (!iio_buffer_enabled(indio_dev))
> +		goto exit_handler;

If buffers aren't enabled, the push to buffers won't do anything. So this race
shouldn't matter.  If it does, what happens?
I'm curious because I'd expect any races that cause trouble in this case
to be pretty universal across drivers.


> +
> +	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 exit_handler;
> +	}
> +
> +	for_each_set_bit(bit, indio_dev->active_scan_mask, AD7779_NUM_CHANNELS - 1)
> +		tmp[k++] = st->spidata_rx[bit];

If you can't use the core demux handling + available_scan_masks, please add
a comment here to say why.  That would allow you to do the SPI transfer directly
into the buffer you then push below. The IIO core will figure out how to
pull data out of that if the user wants a subset of channels.


> +
> +	iio_push_to_buffers_with_timestamp(indio_dev, &tmp[0], pf->timestamp);
> +
> +exit_handler:
> +	iio_trigger_notify_done(indio_dev->trig);
> +	return IRQ_HANDLED;
> +}
> +
> +static int ad7779_reset(struct iio_dev *indio_dev, struct gpio_desc *reset_gpio)
> +{
> +	struct ad7779_state *st = iio_priv(indio_dev);
> +	int ret;
> +	struct spi_transfer reg_read_tr[] = {
> +		{
> +			.tx_buf = st->reset_buf,
> +			.len = 8,
> +		},
> +	};
> +
> +	memset(st->reset_buf, 0xff, sizeof(st->reset_buf));
> +
> +	if (reset_gpio) {
> +		/* Delay for reset to occur is 225 microseconds*/
> +		gpiod_set_value(reset_gpio, 1);
> +		fsleep(230);
> +		return 0;

 +	}
	if (reset_gpio) {
		/* Delay for reset to occur is 225 microseconds*/
		gpiod_set_value(reset_gpio, 1);

	} else {
		struct spi_transfer reg_read_tr[] = {
			{
				.tx_buf = st->reset_buf,
				.len = 8,
			},
		};
		int ret;

		memset(st->reset_buf, 0xff, sizeof(st->reset_buf));
		ret = spi_sync_transfer(st->spi, reg_read_tr,
					ARRAY_SIZE(reg_read_tr));
		if (ret)
			return ret;
	}
	fsleep(230);
	return 0;

or something along those lines.  Shares the sleep so showing the wait
is the same in both types of reset and doesn't do a memset that I think
is otherwise irrelevant if there is a gpio.
> +
> +	ret = spi_sync_transfer(st->spi, reg_read_tr,
> +				ARRAY_SIZE(reg_read_tr));
> +	if (ret)
> +		return ret;
> +
> +	/* Delay for reset to occur is 225 microseconds*/
> +	fsleep(230);
> +
> +	return 0;
> +}

> +static int ad7779_probe(struct spi_device *spi)
> +{
> +	struct iio_dev *indio_dev;
> +	struct ad7779_state *st;
> +	struct gpio_desc *reset_gpio, *start_gpio;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	st = iio_priv(indio_dev);
> +
> +	st->mclk = devm_clk_get_enabled(&spi->dev, "mclk");
> +	if (IS_ERR(st->mclk))
> +		return PTR_ERR(st->mclk);
> +
> +	if (!spi->irq)
> +		return dev_err_probe(&spi->dev, ret,
> +				     "DRDY irq not present\n");
> +
> +	reset_gpio = devm_gpiod_get_optional(&spi->dev, "reset", GPIOD_OUT_LOW);
> +	if (IS_ERR(reset_gpio))
> +		return PTR_ERR(reset_gpio);
> +
> +	start_gpio = devm_gpiod_get(&spi->dev, "start", GPIOD_OUT_HIGH);
> +	if (IS_ERR(start_gpio))
> +		return PTR_ERR(start_gpio);
> +
> +	crc8_populate_msb(ad7779_crc8_table, AD7779_CRC8_POLY);
> +	st->spi = spi;
> +
> +	st->chip_info = spi_get_device_match_data(spi);
> +	if (!st->chip_info)
> +		return -ENODEV;
> +
> +	ret = ad7779_reset(indio_dev, reset_gpio);
> +	if (ret)
> +		return ret;
> +
> +	ad7779_powerup(st, start_gpio);
> +	if (ret)
> +		return ret;
What powers the device down again if we hit an error?

Probably need a devm_add_action_or_reset() or if it self powers down
may a comment on that.

> +
> +	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));
> +	if (!st->trig)
> +		return -ENOMEM;
> +
> +	st->trig->ops = &ad7779_trigger_ops;
> +
> +	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);
> +
> +	ret = devm_iio_trigger_register(&spi->dev, st->trig);
> +	if (ret)
> +		return ret;
> +
> +	indio_dev->trig = iio_trigger_get(st->trig);
> +
> +	init_completion(&st->completion);
> +
> +	ret = devm_iio_triggered_buffer_setup(&spi->dev, indio_dev,
> +					      &iio_pollfunc_store_time,
> +					      &ad7779_trigger_handler,
> +					      &ad7779_buffer_setup_ops);
> +	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, 7));
> +	if (ret)
> +		return ret;
> +
> +	return devm_iio_device_register(&spi->dev, indio_dev);
> +}
> +
> +static int 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;
As below.  Given if !ret, ret == 0 so same as just returning it unconditionally.


> +
> +	return 0;
> +}
> +
> +static int ad7779_resume(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_HIGH_POWER));
> +	if (ret)
> +		return ret;
	return ad7779_spi_write_mask...

> +
> +	return 0;
> +}

Powered by blists - more mailing lists