[PATCH 0/4] spi: introduce SPI_CS_WORD mode flag

[PATCH 0/4] spi: introduce SPI_CS_WORD mode flag

[David Lechner]

This series introduces a new SPI mode flag, SPI_CS_WORD, that indicates that
the chip select line should be toggled after each word sent. This series
includes examples of how this can be implemented for both an SPI controller
and an SPI device.

The motivation here is to take advantage of DMA transfers with an analog/digital
convert chip. This chip requires that the chip select line be toggled after
each word send in order to drive the internal circuitry of the chip.

The way we were accomplishing this was, for example, to read 16 channels, we
would create 16 SPI _transfer_s in one SPI _message_. Although this works, it
uses quite a bit of CPU because we have to do work at the end of each transfer
to get the next transfer ready. When you are polling the chip at 100Hz, this
CPU usage adds up.

The SPI controller being used has DMA support, but only on the _transfer_ level
and not on the _message_ level. So, to take advantage of DMA, we need to read
all of the A/DC channels in a single _transfer_. The SPI controller is capable
automatically toggling the chip select line during a DMA transfer, so we are
adding a new SPI flag in order to take advantage of this.

Dependency: the iio patch applies on top of "iio: adc: ti-ads7950: allow
simultaneous use of buffer and direct mode"[1]

[1]: https://lore.kernel.org/lkml/20180716233550.6449-1-david@lechnology.com/

David Lechner (4):
  spi: spi-bitbang: change flags from u8 to u16
  spi: add new SPI_CS_WORD flag
  spi: spi-davinci: Add support for SPI_CS_WORD
  iio: adc: ti-ads7950: use SPI_CS_WORD to reduce CPU usage

 drivers/iio/adc/ti-ads7950.c    | 53 +++++++++++++++++++--------------
 drivers/spi/spi-davinci.c       | 11 +++++--
 include/linux/spi/spi.h         |  2 +-
 include/linux/spi/spi_bitbang.h |  2 +-
 4 files changed, 41 insertions(+), 27 deletions(-)

-- 
2.17.1

[PATCH 1/4] spi: spi-bitbang: change flags from u8 to u16

[David Lechner]

This changes the data type of the flags field in struct spi_bitbang from
u8 to u16. This matches the size of the mode field of struct spi_device
where these flags are also used.

This is done in preparation of adding a new SPI mode flag that will be
used with this field that would otherwise not fit in 8 bits.

Signed-off-by: David Lechner <david@lechnology.com>
---
 include/linux/spi/spi_bitbang.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/spi/spi_bitbang.h b/include/linux/spi/spi_bitbang.h
index 51d8c060e513..c1e61641a7f1 100644
--- a/include/linux/spi/spi_bitbang.h
+++ b/include/linux/spi/spi_bitbang.h
@@ -8,7 +8,7 @@ struct spi_bitbang {
 	struct mutex		lock;
 	u8			busy;
 	u8			use_dma;
-	u8			flags;		/* extra spi->mode support */
+	u16			flags;		/* extra spi->mode support */
 
 	struct spi_master	*master;
 
-- 
2.17.1

[PATCH 2/4] spi: add new SPI_CS_WORD flag

[David Lechner]

This adds a new SPI mode flag, SPI_CS_WORD, that is used to indicate
that a SPI device requires the chip select to be toggled after each
word that is transferred.

Signed-off-by: David Lechner <david@lechnology.com>
---
 include/linux/spi/spi.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
index a64235e05321..7cc1466111f5 100644
--- a/include/linux/spi/spi.h
+++ b/include/linux/spi/spi.h
@@ -163,6 +163,7 @@ struct spi_device {
 #define	SPI_TX_QUAD	0x200			/* transmit with 4 wires */
 #define	SPI_RX_DUAL	0x400			/* receive with 2 wires */
 #define	SPI_RX_QUAD	0x800			/* receive with 4 wires */
+#define SPI_CS_WORD	0x1000			/* toggle cs after each word */
 	int			irq;
 	void			*controller_state;
 	void			*controller_data;
@@ -177,7 +178,6 @@ struct spi_device {
 	 * the controller talks to each chip, like:
 	 *  - memory packing (12 bit samples into low bits, others zeroed)
 	 *  - priority
-	 *  - drop chipselect after each word
 	 *  - chipselect delays
 	 *  - ...
 	 */
-- 
2.17.1

[PATCH 3/4] spi: spi-davinci: Add support for SPI_CS_WORD

[David Lechner]

This adds support for the SPI_CS_WORD flag to the TI DaVinci SPI
driver. This mode can be used as long as we are using the hardware
chip select and not a GPIO chip select.

Signed-off-by: David Lechner <david@lechnology.com>
---
 drivers/spi/spi-davinci.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/spi/spi-davinci.c b/drivers/spi/spi-davinci.c
index 577084bb911b..8e4b869b50fd 100644
--- a/drivers/spi/spi-davinci.c
+++ b/drivers/spi/spi-davinci.c
@@ -232,7 +232,8 @@ static void davinci_spi_chipselect(struct spi_device *spi, int value)
 				!(spi->mode & SPI_CS_HIGH));
 	} else {
 		if (value == BITBANG_CS_ACTIVE) {
-			spidat1 |= SPIDAT1_CSHOLD_MASK;
+			if (!(spi->mode & SPI_CS_WORD))
+				spidat1 |= SPIDAT1_CSHOLD_MASK;
 			spidat1 &= ~(0x1 << chip_sel);
 		}
 	}
@@ -449,8 +450,12 @@ static int davinci_spi_setup(struct spi_device *spi)
 			return retval;
 		}
 
-		if (internal_cs)
+		if (internal_cs) {
 			set_io_bits(dspi->base + SPIPC0, 1 << spi->chip_select);
+		} else if (spi->mode & SPI_CS_WORD) {
+			dev_err(&spi->dev, "SPI_CS_WORD can't be use with GPIO CS\n");
+			return -EINVAL;
+		}
 	}
 
 	if (spi->mode & SPI_READY)
@@ -985,7 +990,7 @@ static int davinci_spi_probe(struct platform_device *pdev)
 	dspi->prescaler_limit = pdata->prescaler_limit;
 	dspi->version = pdata->version;
 
-	dspi->bitbang.flags = SPI_NO_CS | SPI_LSB_FIRST | SPI_LOOP;
+	dspi->bitbang.flags = SPI_NO_CS | SPI_LSB_FIRST | SPI_LOOP | SPI_CS_WORD;
 	if (dspi->version == SPI_VERSION_2)
 		dspi->bitbang.flags |= SPI_READY;
 
-- 
2.17.1

[PATCH 4/4] iio: adc: ti-ads7950: use SPI_CS_WORD to reduce CPU usage

[David Lechner]

This changes how the SPI message for the triggered buffer is setup in
the TI ADS7950 A/DC driver. By using the SPI_CS_WORD flag, we can read
multiple samples in a single SPI transfer. If the SPI controller
supports DMA transfers, we can see a significant reduction in CPU usage.

For example, on an ARM9 system running at 456MHz reading just 4 channels
at 100Hz: before this change, top shows the CPU usage of the IRQ thread
of this driver to be ~7.7%. After this change, the CPU usage drops to
~3.8%.

Signed-off-by: David Lechner <david@lechnology.com>
---

Dependency: this patch applies on top of "iio: adc: ti-ads7950: allow
simultaneous use of buffer and direct mode"[1]

[1]: https://lore.kernel.org/lkml/20180716233550.6449-1-david@lechnology.com/


 drivers/iio/adc/ti-ads7950.c | 53 +++++++++++++++++++++---------------
 1 file changed, 31 insertions(+), 22 deletions(-)

diff --git a/drivers/iio/adc/ti-ads7950.c b/drivers/iio/adc/ti-ads7950.c
index ba7e5a027490..60de4cbbd5fc 100644
--- a/drivers/iio/adc/ti-ads7950.c
+++ b/drivers/iio/adc/ti-ads7950.c
@@ -60,7 +60,7 @@
 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	ring_xfer;
 	struct spi_transfer	scan_single_xfer[3];
 	struct spi_message	ring_msg;
 	struct spi_message	scan_single_msg;
@@ -69,16 +69,16 @@ struct ti_ads7950_state {
 	unsigned int		vref_mv;
 
 	unsigned int		settings;
-	__be16			single_tx;
-	__be16			single_rx;
+	u16			single_tx;
+	u16			single_rx;
 
 	/*
 	 * DMA (thus cache coherency maintenance) requires the
 	 * transfer buffers to live in their own cache lines.
 	 */
-	__be16	rx_buf[TI_ADS7950_MAX_CHAN + TI_ADS7950_TIMESTAMP_SIZE]
+	u16 rx_buf[TI_ADS7950_MAX_CHAN + 2 + TI_ADS7950_TIMESTAMP_SIZE]
 							____cacheline_aligned;
-	__be16	tx_buf[TI_ADS7950_MAX_CHAN];
+	u16 tx_buf[TI_ADS7950_MAX_CHAN + 2];
 };
 
 struct ti_ads7950_chip_info {
@@ -116,7 +116,7 @@ enum ti_ads7950_id {
 		.realbits = bits,				\
 		.storagebits = 16,				\
 		.shift = 12 - (bits),				\
-		.endianness = IIO_BE,				\
+		.endianness = IIO_CPU,				\
 	},							\
 }
 
@@ -257,23 +257,14 @@ static int ti_ads7950_update_scan_mode(struct iio_dev *indio_dev,
 	len = 0;
 	for_each_set_bit(i, active_scan_mask, indio_dev->num_channels) {
 		cmd = TI_ADS7950_CR_WRITE | TI_ADS7950_CR_CHAN(i) | st->settings;
-		st->tx_buf[len++] = cpu_to_be16(cmd);
+		st->tx_buf[len++] = cmd;
 	}
 
 	/* Data for the 1st channel is not returned until the 3rd transfer */
-	len += 2;
-	for (i = 0; i < len; i++) {
-		if ((i + 2) < len)
-			st->ring_xfer[i].tx_buf = &st->tx_buf[i];
-		if (i >= 2)
-			st->ring_xfer[i].rx_buf = &st->rx_buf[i - 2];
-		st->ring_xfer[i].len = 2;
-		st->ring_xfer[i].cs_change = 1;
-	}
-	/* make sure last transfer's cs_change is not set */
-	st->ring_xfer[len - 1].cs_change = 0;
+	st->tx_buf[len++] = 0;
+	st->tx_buf[len++] = 0;
 
-	spi_message_init_with_transfers(&st->ring_msg, st->ring_xfer, len);
+	st->ring_xfer.len = len * 2;
 
 	return 0;
 }
@@ -289,7 +280,7 @@ static irqreturn_t ti_ads7950_trigger_handler(int irq, void *p)
 	if (ret < 0)
 		goto out;
 
-	iio_push_to_buffers_with_timestamp(indio_dev, st->rx_buf,
+	iio_push_to_buffers_with_timestamp(indio_dev, &st->rx_buf[2],
 					   iio_get_time_ns(indio_dev));
 
 out:
@@ -305,13 +296,13 @@ static int ti_ads7950_scan_direct(struct ti_ads7950_state *st, unsigned int ch)
 	mutex_lock(&st->indio_dev->mlock);
 
 	cmd = TI_ADS7950_CR_WRITE | TI_ADS7950_CR_CHAN(ch) | st->settings;
-	st->single_tx = cpu_to_be16(cmd);
+	st->single_tx = cmd;
 
 	ret = spi_sync(st->spi, &st->scan_single_msg);
 	if (ret)
 		goto out;
 
-	ret = be16_to_cpu(st->single_rx);
+	ret = st->single_rx;
 
 out:
 	mutex_unlock(&st->indio_dev->mlock);
@@ -385,6 +376,14 @@ static int ti_ads7950_probe(struct spi_device *spi)
 	const struct ti_ads7950_chip_info *info;
 	int ret;
 
+	spi->bits_per_word = 16;
+	spi->mode |= SPI_CS_WORD;
+	ret = spi_setup(spi);
+	if (ret < 0) {
+		dev_err(&spi->dev, "Error in spi setup\n");
+		return ret;
+	}
+
 	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
 	if (!indio_dev)
 		return -ENOMEM;
@@ -406,6 +405,16 @@ static int ti_ads7950_probe(struct spi_device *spi)
 	indio_dev->num_channels = info->num_channels;
 	indio_dev->info = &ti_ads7950_info;
 
+	/* build spi ring message */
+	spi_message_init(&st->ring_msg);
+
+	st->ring_xfer.tx_buf = &st->tx_buf[0];
+	st->ring_xfer.rx_buf = &st->rx_buf[0];
+	/* len will be set later */
+	st->ring_xfer.cs_change = true;
+
+	spi_message_add_tail(&st->ring_xfer, &st->ring_msg);
+
 	/*
 	 * Setup default message. The sample is read at the end of the first
 	 * transfer, then it takes one full cycle to convert the sample and one
-- 
2.17.1

Re: [PATCH 2/4] spi: add new SPI_CS_WORD flag

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 978 bytes --]

On Mon, Jul 16, 2018 at 10:20:50PM -0500, David Lechner wrote:
> This adds a new SPI mode flag, SPI_CS_WORD, that is used to indicate
> that a SPI device requires the chip select to be toggled after each
> word that is transferred.

This feels like it should have a soft implementation if it is going to
be truly usable, the vast majority of SPI controllers don't do this and
I can only think of a few that have the hardware feature.  I'd also
expect to see some validation added to the core spi_setup() since at
present a client driver could set the mode option but then have it
ignored by the controller which would presumably break things, we
currently only have checks for specific modes and nothing that'd catch
an unknown flag like this.

Ideally we'd also have some ability to use this as an optimization where
possible with longer sequences (I can see a regmap cache sync being able
to take advantage of this for example) but that might be more trouble
than it's worth.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Applied "spi: spi-bitbang: change flags from u8 to u16" to the spi tree

[Mark Brown]

The patch

   spi: spi-bitbang: change flags from u8 to u16

has been applied to the spi tree at

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git 

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

From 7c201ead1bfd19935f689fca69100c335028c047 Mon Sep 17 00:00:00 2001
From: David Lechner <david@lechnology.com>
Date: Mon, 16 Jul 2018 22:20:49 -0500
Subject: [PATCH] spi: spi-bitbang: change flags from u8 to u16

This changes the data type of the flags field in struct spi_bitbang from
u8 to u16. This matches the size of the mode field of struct spi_device
where these flags are also used.

This is done in preparation of adding a new SPI mode flag that will be
used with this field that would otherwise not fit in 8 bits.

Signed-off-by: David Lechner <david@lechnology.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 include/linux/spi/spi_bitbang.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/spi/spi_bitbang.h b/include/linux/spi/spi_bitbang.h
index 51d8c060e513..c1e61641a7f1 100644
--- a/include/linux/spi/spi_bitbang.h
+++ b/include/linux/spi/spi_bitbang.h
@@ -8,7 +8,7 @@ struct spi_bitbang {
 	struct mutex		lock;
 	u8			busy;
 	u8			use_dma;
-	u8			flags;		/* extra spi->mode support */
+	u16			flags;		/* extra spi->mode support */
 
 	struct spi_master	*master;
 
-- 
2.18.0

Applied "spi: spi-bitbang: change flags from u8 to u16" to the spi tree

[Mark Brown]

The patch

   spi: spi-bitbang: change flags from u8 to u16

has been applied to the spi tree at

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git 

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

>From 7c201ead1bfd19935f689fca69100c335028c047 Mon Sep 17 00:00:00 2001
From: David Lechner <david@lechnology.com>
Date: Mon, 16 Jul 2018 22:20:49 -0500
Subject: [PATCH] spi: spi-bitbang: change flags from u8 to u16

This changes the data type of the flags field in struct spi_bitbang from
u8 to u16. This matches the size of the mode field of struct spi_device
where these flags are also used.

This is done in preparation of adding a new SPI mode flag that will be
used with this field that would otherwise not fit in 8 bits.

Signed-off-by: David Lechner <david@lechnology.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 include/linux/spi/spi_bitbang.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/spi/spi_bitbang.h b/include/linux/spi/spi_bitbang.h
index 51d8c060e513..c1e61641a7f1 100644
--- a/include/linux/spi/spi_bitbang.h
+++ b/include/linux/spi/spi_bitbang.h
@@ -8,7 +8,7 @@ struct spi_bitbang {
 	struct mutex		lock;
 	u8			busy;
 	u8			use_dma;
-	u8			flags;		/* extra spi->mode support */
+	u16			flags;		/* extra spi->mode support */
 
 	struct spi_master	*master;
 
-- 
2.18.0

Re: [PATCH 2/4] spi: add new SPI_CS_WORD flag

[David Lechner]

On 7/18/18 10:04 AM, Mark Brown wrote:
> On Mon, Jul 16, 2018 at 10:20:50PM -0500, David Lechner wrote:
>> This adds a new SPI mode flag, SPI_CS_WORD, that is used to indicate
>> that a SPI device requires the chip select to be toggled after each
>> word that is transferred.
> 
> This feels like it should have a soft implementation if it is going to
> be truly usable, the vast majority of SPI controllers don't do this and

This occurred to me as well. Another possibility, though, would be to 
leave it up to the client device drivers to support both cases, e.g.:

	if (master has SPI_CS_WORD support)
		setup message as single transfer
	else
		setup message as multiple one-word transfers

This seems like that would be more efficient than having a generic 
implementation for masters that says:

	if (master does not have SPI_CS_WORD support)
		allocate enough transfers for each word of each
			each transfer of the message
		allocate and setup a new message for these transfers
		loop through the original transfers of the original
			message and copy them to the new transfers
		send the new message
		free allocated message and transfers

> I can only think of a few that have the hardware feature.  I'd also
> expect to see some validation added to the core spi_setup() since at
> present a client driver could set the mode option but then have it
> ignored by the controller which would presumably break things, we
> currently only have checks for specific modes and nothing that'd catch
> an unknown flag like this.

There is already a generic mode flags check in spi_setup() that will 
catch this and return an error if the device has the SPI_CS_WORD flag 
set and the controller does not. (I know this works because I ran into 
it during development.)

> 
> Ideally we'd also have some ability to use this as an optimization where
> possible with longer sequences (I can see a regmap cache sync being able
> to take advantage of this for example) but that might be more trouble
> than it's worth.
> 

Re: [PATCH 2/4] spi: add new SPI_CS_WORD flag

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 2125 bytes --]

On Wed, Jul 18, 2018 at 11:47:30AM -0500, David Lechner wrote:
> On 7/18/18 10:04 AM, Mark Brown wrote:

> > This feels like it should have a soft implementation if it is going to
> > be truly usable, the vast majority of SPI controllers don't do this and

> This occurred to me as well. Another possibility, though, would be to leave
> it up to the client device drivers to support both cases, e.g.:

> 	if (master has SPI_CS_WORD support)
> 		setup message as single transfer
> 	else
> 		setup message as multiple one-word transfers

> This seems like that would be more efficient than having a generic
> implementation for masters that says:

That then requires every single user to open code this which immediately
suggests that there should be a helper which is going to look a lot like
any generic implementation.

> 	if (master does not have SPI_CS_WORD support)
> 		allocate enough transfers for each word of each
> 			each transfer of the message
> 		allocate and setup a new message for these transfers
> 		loop through the original transfers of the original
> 			message and copy them to the new transfers
> 		send the new message
> 		free allocated message and transfers

I'd imagine that the much bigger problem is that you end up with
enormous numbers of operations for any non-trivial transfers which is
going to happen anyway.  It's really only the copying bit that's at all
an overhead here.

> > I can only think of a few that have the hardware feature.  I'd also
> > expect to see some validation added to the core spi_setup() since at
> > present a client driver could set the mode option but then have it
> > ignored by the controller which would presumably break things, we
> > currently only have checks for specific modes and nothing that'd catch
> > an unknown flag like this.

> There is already a generic mode flags check in spi_setup() that will catch
> this and return an error if the device has the SPI_CS_WORD flag set and the
> controller does not. (I know this works because I ran into it during
> development.)

Ah, good - I'd forgotten it was there and didn't spot it when I went to
check.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 4/4] iio: adc: ti-ads7950: use SPI_CS_WORD to reduce CPU usage

[Jonathan Cameron]

On Mon, 16 Jul 2018 22:20:52 -0500
David Lechner <david@lechnology.com> wrote:

> This changes how the SPI message for the triggered buffer is setup in
> the TI ADS7950 A/DC driver. By using the SPI_CS_WORD flag, we can read
> multiple samples in a single SPI transfer. If the SPI controller
> supports DMA transfers, we can see a significant reduction in CPU usage.
> 
> For example, on an ARM9 system running at 456MHz reading just 4 channels
> at 100Hz: before this change, top shows the CPU usage of the IRQ thread
> of this driver to be ~7.7%. After this change, the CPU usage drops to
> ~3.8%.
> 
> Signed-off-by: David Lechner <david@lechnology.com>
Hmm. There is a userspace ABI change in here, though it shouldn't matter
as long as people are using the full ABI rather than running some
scripts that make assumptions.  

It's quite nice if we have all the relevant emulation in the SPI core
that this doesn't break things on any spi controllers.

Jonathan

> ---
> 
> Dependency: this patch applies on top of "iio: adc: ti-ads7950: allow
> simultaneous use of buffer and direct mode"[1]
> 
> [1]: https://lore.kernel.org/lkml/20180716233550.6449-1-david@lechnology.com/
> 
> 
>  drivers/iio/adc/ti-ads7950.c | 53 +++++++++++++++++++++---------------
>  1 file changed, 31 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/iio/adc/ti-ads7950.c b/drivers/iio/adc/ti-ads7950.c
> index ba7e5a027490..60de4cbbd5fc 100644
> --- a/drivers/iio/adc/ti-ads7950.c
> +++ b/drivers/iio/adc/ti-ads7950.c
> @@ -60,7 +60,7 @@
>  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	ring_xfer;
>  	struct spi_transfer	scan_single_xfer[3];
>  	struct spi_message	ring_msg;
>  	struct spi_message	scan_single_msg;
> @@ -69,16 +69,16 @@ struct ti_ads7950_state {
>  	unsigned int		vref_mv;
>  
>  	unsigned int		settings;
> -	__be16			single_tx;
> -	__be16			single_rx;
> +	u16			single_tx;
> +	u16			single_rx;
>  
>  	/*
>  	 * DMA (thus cache coherency maintenance) requires the
>  	 * transfer buffers to live in their own cache lines.
>  	 */
> -	__be16	rx_buf[TI_ADS7950_MAX_CHAN + TI_ADS7950_TIMESTAMP_SIZE]
> +	u16 rx_buf[TI_ADS7950_MAX_CHAN + 2 + TI_ADS7950_TIMESTAMP_SIZE]
>  							____cacheline_aligned;
> -	__be16	tx_buf[TI_ADS7950_MAX_CHAN];
> +	u16 tx_buf[TI_ADS7950_MAX_CHAN + 2];
>  };
>  
>  struct ti_ads7950_chip_info {
> @@ -116,7 +116,7 @@ enum ti_ads7950_id {
>  		.realbits = bits,				\
>  		.storagebits = 16,				\
>  		.shift = 12 - (bits),				\
> -		.endianness = IIO_BE,				\
> +		.endianness = IIO_CPU,				\

Hmm. I'm getting a little dubious.  This is a userspace ABI change - it 'might'
break someone.  We'd have to cross our fingers it doesn't.

>  	},							\
>  }
>  
> @@ -257,23 +257,14 @@ static int ti_ads7950_update_scan_mode(struct iio_dev *indio_dev,
>  	len = 0;
>  	for_each_set_bit(i, active_scan_mask, indio_dev->num_channels) {
>  		cmd = TI_ADS7950_CR_WRITE | TI_ADS7950_CR_CHAN(i) | st->settings;
> -		st->tx_buf[len++] = cpu_to_be16(cmd);
> +		st->tx_buf[len++] = cmd;
>  	}
>  
>  	/* Data for the 1st channel is not returned until the 3rd transfer */
> -	len += 2;
> -	for (i = 0; i < len; i++) {
> -		if ((i + 2) < len)
> -			st->ring_xfer[i].tx_buf = &st->tx_buf[i];
> -		if (i >= 2)
> -			st->ring_xfer[i].rx_buf = &st->rx_buf[i - 2];
> -		st->ring_xfer[i].len = 2;
> -		st->ring_xfer[i].cs_change = 1;
> -	}
> -	/* make sure last transfer's cs_change is not set */
> -	st->ring_xfer[len - 1].cs_change = 0;
> +	st->tx_buf[len++] = 0;
> +	st->tx_buf[len++] = 0;
>  
> -	spi_message_init_with_transfers(&st->ring_msg, st->ring_xfer, len);
> +	st->ring_xfer.len = len * 2;
>  
>  	return 0;
>  }
> @@ -289,7 +280,7 @@ static irqreturn_t ti_ads7950_trigger_handler(int irq, void *p)
>  	if (ret < 0)
>  		goto out;
>  
> -	iio_push_to_buffers_with_timestamp(indio_dev, st->rx_buf,
> +	iio_push_to_buffers_with_timestamp(indio_dev, &st->rx_buf[2],
>  					   iio_get_time_ns(indio_dev));
>  
>  out:
> @@ -305,13 +296,13 @@ static int ti_ads7950_scan_direct(struct ti_ads7950_state *st, unsigned int ch)
>  	mutex_lock(&st->indio_dev->mlock);
>  
>  	cmd = TI_ADS7950_CR_WRITE | TI_ADS7950_CR_CHAN(ch) | st->settings;
> -	st->single_tx = cpu_to_be16(cmd);
> +	st->single_tx = cmd;
>  
>  	ret = spi_sync(st->spi, &st->scan_single_msg);
>  	if (ret)
>  		goto out;
>  
> -	ret = be16_to_cpu(st->single_rx);
> +	ret = st->single_rx;
>  
>  out:
>  	mutex_unlock(&st->indio_dev->mlock);
> @@ -385,6 +376,14 @@ static int ti_ads7950_probe(struct spi_device *spi)
>  	const struct ti_ads7950_chip_info *info;
>  	int ret;
>  
> +	spi->bits_per_word = 16;
> +	spi->mode |= SPI_CS_WORD;
> +	ret = spi_setup(spi);
> +	if (ret < 0) {
> +		dev_err(&spi->dev, "Error in spi setup\n");
> +		return ret;
> +	}
> +
>  	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
>  	if (!indio_dev)
>  		return -ENOMEM;
> @@ -406,6 +405,16 @@ static int ti_ads7950_probe(struct spi_device *spi)
>  	indio_dev->num_channels = info->num_channels;
>  	indio_dev->info = &ti_ads7950_info;
>  
> +	/* build spi ring message */
> +	spi_message_init(&st->ring_msg);
> +
> +	st->ring_xfer.tx_buf = &st->tx_buf[0];
> +	st->ring_xfer.rx_buf = &st->rx_buf[0];
> +	/* len will be set later */
> +	st->ring_xfer.cs_change = true;
> +
> +	spi_message_add_tail(&st->ring_xfer, &st->ring_msg);
> +
>  	/*
>  	 * Setup default message. The sample is read at the end of the first
>  	 * transfer, then it takes one full cycle to convert the sample and one

Re: [PATCH 4/4] iio: adc: ti-ads7950: use SPI_CS_WORD to reduce CPU usage

[David Lechner]

On 07/21/2018 12:51 PM, Jonathan Cameron wrote:
> On Mon, 16 Jul 2018 22:20:52 -0500
> David Lechner <david@lechnology.com> wrote:
> 
>> This changes how the SPI message for the triggered buffer is setup in
>> the TI ADS7950 A/DC driver. By using the SPI_CS_WORD flag, we can read
>> multiple samples in a single SPI transfer. If the SPI controller
>> supports DMA transfers, we can see a significant reduction in CPU usage.
>>
>> For example, on an ARM9 system running at 456MHz reading just 4 channels
>> at 100Hz: before this change, top shows the CPU usage of the IRQ thread
>> of this driver to be ~7.7%. After this change, the CPU usage drops to
>> ~3.8%.
>>
>> Signed-off-by: David Lechner <david@lechnology.com>
> Hmm. There is a userspace ABI change in here, though it shouldn't matter
> as long as people are using the full ABI rather than running some
> scripts that make assumptions.
> 
> It's quite nice if we have all the relevant emulation in the SPI core
> that this doesn't break things on any spi controllers.
> 
> Jonathan
> 
>> ---
>>
>> Dependency: this patch applies on top of "iio: adc: ti-ads7950: allow
>> simultaneous use of buffer and direct mode"[1]
>>
>> [1]: https://lore.kernel.org/lkml/20180716233550.6449-1-david@lechnology.com/
>>
>>
>>   drivers/iio/adc/ti-ads7950.c | 53 +++++++++++++++++++++---------------
>>   1 file changed, 31 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/iio/adc/ti-ads7950.c b/drivers/iio/adc/ti-ads7950.c
>> index ba7e5a027490..60de4cbbd5fc 100644
>> --- a/drivers/iio/adc/ti-ads7950.c
>> +++ b/drivers/iio/adc/ti-ads7950.c
>> @@ -60,7 +60,7 @@
>>   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	ring_xfer;
>>   	struct spi_transfer	scan_single_xfer[3];
>>   	struct spi_message	ring_msg;
>>   	struct spi_message	scan_single_msg;
>> @@ -69,16 +69,16 @@ struct ti_ads7950_state {
>>   	unsigned int		vref_mv;
>>   
>>   	unsigned int		settings;
>> -	__be16			single_tx;
>> -	__be16			single_rx;
>> +	u16			single_tx;
>> +	u16			single_rx;
>>   
>>   	/*
>>   	 * DMA (thus cache coherency maintenance) requires the
>>   	 * transfer buffers to live in their own cache lines.
>>   	 */
>> -	__be16	rx_buf[TI_ADS7950_MAX_CHAN + TI_ADS7950_TIMESTAMP_SIZE]
>> +	u16 rx_buf[TI_ADS7950_MAX_CHAN + 2 + TI_ADS7950_TIMESTAMP_SIZE]
>>   							____cacheline_aligned;
>> -	__be16	tx_buf[TI_ADS7950_MAX_CHAN];
>> +	u16 tx_buf[TI_ADS7950_MAX_CHAN + 2];
>>   };
>>   
>>   struct ti_ads7950_chip_info {
>> @@ -116,7 +116,7 @@ enum ti_ads7950_id {
>>   		.realbits = bits,				\
>>   		.storagebits = 16,				\
>>   		.shift = 12 - (bits),				\
>> -		.endianness = IIO_BE,				\
>> +		.endianness = IIO_CPU,				\
> 
> Hmm. I'm getting a little dubious.  This is a userspace ABI change - it 'might'
> break someone.  We'd have to cross our fingers it doesn't.

Dubious is a good word for this. ;-)

I was hoping that we could try to get away with this anyway. If someone
complains, we can always change it back, right? And if no one complains, then
did we really break anything?

I'll have to play around with the a default SPI_CS_WORD implementation first
to make sure it won't influence this either.