iio: adc: ti-ads7950: allow simultaneous use of buffer and direct mode

PATCH Superseded David Lechner 2 participants 2 messages Jul 16, 2018 – Jul 21, 2018 IIO

Share & tools

David Lechner

author

2018-07-16 23:36 UTC #0 lore raw

PATCH 1/1 iio: adc: ti-ads7950: allow simultaneous use of buffer and direct mode

This modifies the TI ADS7950 A/DC driver to allow the simultaneous use of both the triggered buffer and reading channels directly (via in- kernel API or sysfs).

The use case for this is on LEGO MINDSTORMS EV3. Two of the voltage channels are used in-kernel by a power supply driver, which reads the values using iio_read_channel_processed(). These channels are only read at a slow rate (<= 1Hz). However, we want to be able to read 12 other channels at the same time using the triggered buffer at a high rate (>= 100Hz) without breaking the power supply driver.

Signed-off-by David Lechner <[email protected]>

---
 drivers/iio/adc/ti-ads7950.c | 29 +++++++++++++++++------------
 1 file changed, 17 insertions(+), 12 deletions(-)

drivers/iio/adc/ti-ads7950.c +17 ~~-12~~

index 0225c1b333ab..ba7e5a027490 100644
@@ -58,6 +58,7 @@
 	(((val) >> (dec)) & ((1 << (bits)) - 1))
 
 struct ti_ads7950_state {
+	struct iio_dev		*indio_dev;
 	struct spi_device	*spi;
 	struct spi_transfer	ring_xfer[TI_ADS7950_MAX_CHAN + 2];
 	struct spi_transfer	scan_single_xfer[3];

@@ -68,6 +69,8 @@ struct ti_ads7950_state {
 	unsigned int		vref_mv;
 
 	unsigned int		settings;
+	__be16			single_tx;
+	__be16			single_rx;
 
 	/*
 	 * DMA (thus cache coherency maintenance) requires the

@@ -299,14 +302,21 @@ static int ti_ads7950_scan_direct(struct ti_ads7950_state *st, unsigned int ch)
 {
 	int ret, cmd;
 
+	mutex_lock(&st->indio_dev->mlock);
+
 	cmd = TI_ADS7950_CR_WRITE | TI_ADS7950_CR_CHAN(ch) | st->settings;
-	st->tx_buf[0] = cpu_to_be16(cmd);
+	st->single_tx = cpu_to_be16(cmd);
 
 	ret = spi_sync(st->spi, &st->scan_single_msg);
 	if (ret)
-		return ret;
+		goto out;
+
+	ret = be16_to_cpu(st->single_rx);
+
+out:
+	mutex_unlock(&st->indio_dev->mlock);
 
-	return be16_to_cpu(st->rx_buf[0]);
+		return ret;
 }
 
 static int ti_ads7950_get_range(struct ti_ads7950_state *st)

@@ -338,13 +348,7 @@ static int ti_ads7950_read_raw(struct iio_dev *indio_dev,
 
 	switch (m) {
 	case IIO_CHAN_INFO_RAW:
-
-		ret = iio_device_claim_direct_mode(indio_dev);
-		if (ret < 0)
-			return ret;
-
 		ret = ti_ads7950_scan_direct(st, chan->address);
-		iio_device_release_direct_mode(indio_dev);
 		if (ret < 0)
 			return ret;

@@ -389,6 +393,7 @@ static int ti_ads7950_probe(struct spi_device *spi)
 
 	spi_set_drvdata(spi, indio_dev);
 
+	st->indio_dev = indio_dev;
 	st->spi = spi;
 	st->settings = TI_ADS7950_CR_MANUAL | TI_ADS7950_CR_RANGE_5V;

@@ -410,13 +415,13 @@ static int ti_ads7950_probe(struct spi_device *spi)
 	 * was read at the end of the first transfer.
 	 */
 
-	st->scan_single_xfer[0].tx_buf = &st->tx_buf[0];
+	st->scan_single_xfer[0].tx_buf = &st->single_tx;
 	st->scan_single_xfer[0].len = 2;
 	st->scan_single_xfer[0].cs_change = 1;
-	st->scan_single_xfer[1].tx_buf = &st->tx_buf[0];
+	st->scan_single_xfer[1].tx_buf = &st->single_tx;
 	st->scan_single_xfer[1].len = 2;
 	st->scan_single_xfer[1].cs_change = 1;
-	st->scan_single_xfer[2].rx_buf = &st->rx_buf[0];
+	st->scan_single_xfer[2].rx_buf = &st->single_rx;
 	st->scan_single_xfer[2].len = 2;
 
 	spi_message_init_with_transfers(&st->scan_single_msg,

Signature · 1 line

2.17.1

Jonathan Cameron

maintainer

2018-07-21 17:58 UTC #1 lore raw

Quoted 4 lines · On Mon, 16 Jul 2018 18:35:50 -0500 David Lechner <[email protected]> wrote:

This modifies the TI ADS7950 A/DC driver to allow the simultaneous use of both the triggered buffer and reading channels directly (via in- kernel API or sysfs).

Quoted 8 lines

The use case for this is on LEGO MINDSTORMS EV3. Two of the voltage channels are used in-kernel by a power supply driver, which reads the values using iio_read_channel_processed(). These channels are only read at a slow rate (<= 1Hz). However, we want to be able to read 12 other channels at the same time using the triggered buffer at a high rate (>= 100Hz) without breaking the power supply driver.
Signed-off-by David Lechner <[email protected]>

Hmm. I've wondered for a long time what the 'right' way to handle these dual speed / method situations is.

There is a more involved solution in the iio core that would work. When we have another in kernel user request a particular set of channels, we could have the core setup a buffer consumer that basically caches them. The trick is to then make that optional so that it only gets enabled if someone else is using the buffered interface. Getting that detail right was the bit that always left me stuck.

Even then there is the potential for a really slow buffered read to block a fast sysfs read. Hence it's less than ideal.

So let's go with what you have here and perhaps revisit in future if it keeps turning up.

A few comments line.

Jonathan

drivers/iio/adc/ti-ads7950.c line 61

Quoted 13 lines

---
 drivers/iio/adc/ti-ads7950.c | 29 +++++++++++++++++------------
 1 file changed, 17 insertions(+), 12 deletions(-)

drivers/iio/adc/ti-ads7950.c +1 -0

index 0225c1b333ab..ba7e5a027490 100644
@@ -58,6 +58,7 @@
 	(((val) >> (dec)) & ((1 << (bits)) - 1))
 
 struct ti_ads7950_state {
+	struct iio_dev		*indio_dev;

The moment I see this, it rings alarm bells, suggesting some less than ideal architecture.

Quoted 13 lines

drivers/iio/adc/ti-ads7950.c +2 -0

 	struct spi_device	*spi;
 	struct spi_transfer	ring_xfer[TI_ADS7950_MAX_CHAN + 2];
 	struct spi_transfer	scan_single_xfer[3];

@@ -68,6 +69,8 @@ struct ti_ads7950_state {
 	unsigned int		vref_mv;
 
 	unsigned int		settings;
+	__be16			single_tx;
+	__be16			single_rx;
 
 	/*
 	 * DMA (thus cache coherency maintenance) requires the

drivers/iio/adc/ti-ads7950.c static int ti_ads7950_scan_direct(struct ti_ads7950_state *st, unsigned int ch) · line 305

Quoted 4 lines

drivers/iio/adc/ti-ads7950.c +1 -0

@@ -299,14 +302,21 @@ static int ti_ads7950_scan_direct(struct ti_ads7950_state *st, unsigned int ch)
 {
 	int ret, cmd;
 
+	mutex_lock(&st->indio_dev->mlock);

Change it so that scan_direct is passed the indio_dev rather than st.

Quoted 7 lines

drivers/iio/adc/ti-ads7950.c +2 -1

+
 	cmd = TI_ADS7950_CR_WRITE | TI_ADS7950_CR_CHAN(ch) | st->settings;
-	st->tx_buf[0] = cpu_to_be16(cmd);
+	st->single_tx = cpu_to_be16(cmd);
 
 	ret = spi_sync(st->spi, &st->scan_single_msg);
 	if (ret)

drivers/iio/adc/ti-ads7950.c static int ti_ads7950_scan_direct(struct ti_ads7950_state *st, unsigned int ch) · line 319

Quoted 10 lines

drivers/iio/adc/ti-ads7950.c +7 -2

-		return ret;
+		goto out;
+
+	ret = be16_to_cpu(st->single_rx);
+
+out:
+	mutex_unlock(&st->indio_dev->mlock);
 
-	return be16_to_cpu(st->rx_buf[0]);
+		return ret;

Indenting seems unlikely to be right!

Quoted 42 lines

drivers/iio/adc/ti-ads7950.c +4 -9

 }
 
 static int ti_ads7950_get_range(struct ti_ads7950_state *st)

@@ -338,13 +348,7 @@ static int ti_ads7950_read_raw(struct iio_dev *indio_dev,
 
 	switch (m) {
 	case IIO_CHAN_INFO_RAW:
-
-		ret = iio_device_claim_direct_mode(indio_dev);
-		if (ret < 0)
-			return ret;
-
 		ret = ti_ads7950_scan_direct(st, chan->address);
-		iio_device_release_direct_mode(indio_dev);
 		if (ret < 0)
 			return ret;

@@ -389,6 +393,7 @@ static int ti_ads7950_probe(struct spi_device *spi)
 
 	spi_set_drvdata(spi, indio_dev);
 
+	st->indio_dev = indio_dev;
 	st->spi = spi;
 	st->settings = TI_ADS7950_CR_MANUAL | TI_ADS7950_CR_RANGE_5V;

@@ -410,13 +415,13 @@ static int ti_ads7950_probe(struct spi_device *spi)
 	 * was read at the end of the first transfer.
 	 */
 
-	st->scan_single_xfer[0].tx_buf = &st->tx_buf[0];
+	st->scan_single_xfer[0].tx_buf = &st->single_tx;
 	st->scan_single_xfer[0].len = 2;
 	st->scan_single_xfer[0].cs_change = 1;
-	st->scan_single_xfer[1].tx_buf = &st->tx_buf[0];
+	st->scan_single_xfer[1].tx_buf = &st->single_tx;
 	st->scan_single_xfer[1].len = 2;
 	st->scan_single_xfer[1].cs_change = 1;
-	st->scan_single_xfer[2].rx_buf = &st->rx_buf[0];
+	st->scan_single_xfer[2].rx_buf = &st->single_rx;
 	st->scan_single_xfer[2].len = 2;
 
 	spi_message_init_with_transfers(&st->scan_single_msg,