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/[email protected]/
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
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 <[email protected]>
---
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/[email protected]/
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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.
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 <[email protected]>
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 <[email protected]>
Signed-off-by: Mark Brown <[email protected]>
---
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
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.
>
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.
On Mon, 16 Jul 2018 22:20:52 -0500
David Lechner <[email protected]> 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 <[email protected]>
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/[email protected]/
>
>
> 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
On 07/21/2018 12:51 PM, Jonathan Cameron wrote:
> On Mon, 16 Jul 2018 22:20:52 -0500
> David Lechner <[email protected]> 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 <[email protected]>
> 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/[email protected]/
>>
>>
>> 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.