linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 0/4] spi: introduce `struct spi_delay` data-type
@ 2019-09-13 11:55 Alexandru Ardelean
  2019-09-13 11:55 ` [PATCH v2 1/4] spi: move `cs_change_delay` backwards compat logic outside switch Alexandru Ardelean
                   ` (3 more replies)
  0 siblings, 4 replies; 7+ messages in thread
From: Alexandru Ardelean @ 2019-09-13 11:55 UTC (permalink / raw)
  To: linux-iio, linux-spi, linux-kernel; +Cc: jic23, broonie, Alexandru Ardelean

Discussion reference:
  https://lore.kernel.org/lkml/20190913114550.956-1-alexandru.ardelean@analog.com/

This changeset introduces an `spi_delay` struct/data-type and makes the
IIO ADIS driver library the first user of this.

The patchset base is Jonathan's `iio/togreg` branch, but it also applies on
Mark's `spi/for-next` branch.

The reasons for choosing `cs_change_delay`, is:
1. this change introduces delay units
2. it is not yet widely used, meaning it can still be changed 

Some delays like `delay` from `spi_transfer` would require a longer
update-time change & discussion.

Alexandru Ardelean (4):
  spi: move `cs_change_delay` backwards compat logic outside switch
  spi: introduce spi_delay struct as "value + unit" &  spi_delay_exec()
  spi: make `cs_change_delay` the first user of the `spi_delay` logic
  iio: imu: adis: convert cs_change_delay to spi_delay struct

 drivers/iio/imu/adis.c  | 24 +++++++--------
 drivers/spi/spi.c       | 68 +++++++++++++++++++++++++++++++----------
 include/linux/spi/spi.h | 22 +++++++++----
 3 files changed, 80 insertions(+), 34 deletions(-)

-- 

Changelog v1 -> v2:
* split away from the RFC patchset, which aims to be a broader explanation
  for this changeset; parts of v1 are not 100% defined yet, and may require
  some discussion and refinement.

2.20.1


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH v2 1/4] spi: move `cs_change_delay` backwards compat logic outside switch
  2019-09-13 11:55 [PATCH v2 0/4] spi: introduce `struct spi_delay` data-type Alexandru Ardelean
@ 2019-09-13 11:55 ` Alexandru Ardelean
  2019-09-13 11:55 ` [PATCH v2 2/4] spi: introduce spi_delay struct as "value + unit" & spi_delay_exec() Alexandru Ardelean
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 7+ messages in thread
From: Alexandru Ardelean @ 2019-09-13 11:55 UTC (permalink / raw)
  To: linux-iio, linux-spi, linux-kernel; +Cc: jic23, broonie, Alexandru Ardelean

The `cs_change_delay` backwards compatibility value could be moved outside
of the switch statement.
The only reason to do it, is to make the next patches easier to diff.

Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/spi/spi.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 75ac046cae52..c90e02e6d62f 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -1114,16 +1114,15 @@ static void _spi_transfer_cs_change_delay(struct spi_message *msg,
 	u32 hz;
 
 	/* return early on "fast" mode - for everything but USECS */
-	if (!delay && unit != SPI_DELAY_UNIT_USECS)
+	if (!delay) {
+		if (unit == SPI_DELAY_UNIT_USECS)
+			_spi_transfer_delay_ns(10000);
 		return;
+	}
 
 	switch (unit) {
 	case SPI_DELAY_UNIT_USECS:
-		/* for compatibility use default of 10us */
-		if (!delay)
-			delay = 10000;
-		else
-			delay *= 1000;
+		delay *= 1000;
 		break;
 	case SPI_DELAY_UNIT_NSECS: /* nothing to do here */
 		break;
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH v2 2/4] spi: introduce spi_delay struct as "value + unit" &  spi_delay_exec()
  2019-09-13 11:55 [PATCH v2 0/4] spi: introduce `struct spi_delay` data-type Alexandru Ardelean
  2019-09-13 11:55 ` [PATCH v2 1/4] spi: move `cs_change_delay` backwards compat logic outside switch Alexandru Ardelean
@ 2019-09-13 11:55 ` Alexandru Ardelean
  2019-09-13 11:55 ` [PATCH v2 3/4] spi: make `cs_change_delay` the first user of the `spi_delay` logic Alexandru Ardelean
  2019-09-13 11:55 ` [PATCH v2 4/4] iio: imu: adis: convert cs_change_delay to spi_delay struct Alexandru Ardelean
  3 siblings, 0 replies; 7+ messages in thread
From: Alexandru Ardelean @ 2019-09-13 11:55 UTC (permalink / raw)
  To: linux-iio, linux-spi, linux-kernel; +Cc: jic23, broonie, Alexandru Ardelean

There are plenty of delays that have been introduced in SPI core. Most of
them are in micro-seconds, some need to be in nano-seconds, and some in
clock-cycles.

For some of these delays (related to transfers & CS timing) it may make
sense to have a `spi_delay` struct that abstracts these a bit.

The important element of these delays [for unification] seems to be the
`unit` of the delay.
It looks like micro-seconds is good enough for most people, but every-once
in a while, some delays seem to require other units of measurement.

This change adds the `spi_delay` struct & a `spi_delay_exec()` function
that processes a `spi_delay` object/struct to execute the delay.
It's a copy of the `cs_change_delay` mechanism, but without the default
for 10 uS.

The clock-cycle delay unit is a bit special, as it needs to be bound to an
`spi_transfer` object to execute.

Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/spi/spi.c       | 51 +++++++++++++++++++++++++++++++++++++++++
 include/linux/spi/spi.h | 18 ++++++++++++---
 2 files changed, 66 insertions(+), 3 deletions(-)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index c90e02e6d62f..1883de8ffa82 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -1106,6 +1106,57 @@ static void _spi_transfer_delay_ns(u32 ns)
 	}
 }
 
+static int _spi_delay_to_ns(struct spi_delay *_delay, struct spi_transfer *xfer)
+{
+	u32 delay = _delay->value;
+	u32 unit = _delay->unit;
+	u32 hz;
+
+	if (!delay)
+		return 0;
+
+	switch (unit) {
+	case SPI_DELAY_UNIT_USECS:
+		delay *= 1000;
+		break;
+	case SPI_DELAY_UNIT_NSECS: /* nothing to do here */
+		break;
+	case SPI_DELAY_UNIT_SCK:
+		/* clock cycles need to be obtained from spi_transfer */
+		if (!xfer)
+			return -EINVAL;
+		/* if there is no effective speed know, then approximate
+		 * by underestimating with half the requested hz
+		 */
+		hz = xfer->effective_speed_hz ?: xfer->speed_hz / 2;
+		if (!hz)
+			return -EINVAL;
+		delay *= DIV_ROUND_UP(1000000000, hz);
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return delay;
+}
+
+int spi_delay_exec(struct spi_delay *_delay, struct spi_transfer *xfer)
+{
+	int delay;
+
+	if (!_delay)
+		return -EINVAL;
+
+	delay = _spi_delay_to_ns(_delay, xfer);
+	if (delay < 0)
+		return delay;
+
+	_spi_transfer_delay_ns(delay);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(spi_delay_exec);
+
 static void _spi_transfer_cs_change_delay(struct spi_message *msg,
 					  struct spi_transfer *xfer)
 {
diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
index af4f265d0f67..c18cfa7cda35 100644
--- a/include/linux/spi/spi.h
+++ b/include/linux/spi/spi.h
@@ -89,6 +89,21 @@ void spi_statistics_add_transfer_stats(struct spi_statistics *stats,
 #define SPI_STATISTICS_INCREMENT_FIELD(stats, field)	\
 	SPI_STATISTICS_ADD_TO_FIELD(stats, field, 1)
 
+/**
+ * struct spi_delay - SPI delay information
+ * @value: Value for the delay
+ * @unit: Unit for the delay
+ */
+struct spi_delay {
+#define SPI_DELAY_UNIT_USECS	0
+#define SPI_DELAY_UNIT_NSECS	1
+#define SPI_DELAY_UNIT_SCK	2
+	u16	value;
+	u8	unit;
+};
+
+extern int spi_delay_exec(struct spi_delay *_delay, struct spi_transfer *xfer);
+
 /**
  * struct spi_device - Controller side proxy for an SPI slave device
  * @dev: Driver model representation of the device.
@@ -834,9 +849,6 @@ struct spi_transfer {
 	u16		delay_usecs;
 	u16		cs_change_delay;
 	u8		cs_change_delay_unit;
-#define SPI_DELAY_UNIT_USECS	0
-#define SPI_DELAY_UNIT_NSECS	1
-#define SPI_DELAY_UNIT_SCK	2
 	u32		speed_hz;
 	u16		word_delay;
 
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH v2 3/4] spi: make `cs_change_delay` the first user of the `spi_delay` logic
  2019-09-13 11:55 [PATCH v2 0/4] spi: introduce `struct spi_delay` data-type Alexandru Ardelean
  2019-09-13 11:55 ` [PATCH v2 1/4] spi: move `cs_change_delay` backwards compat logic outside switch Alexandru Ardelean
  2019-09-13 11:55 ` [PATCH v2 2/4] spi: introduce spi_delay struct as "value + unit" & spi_delay_exec() Alexandru Ardelean
@ 2019-09-13 11:55 ` Alexandru Ardelean
  2019-09-13 11:55 ` [PATCH v2 4/4] iio: imu: adis: convert cs_change_delay to spi_delay struct Alexandru Ardelean
  3 siblings, 0 replies; 7+ messages in thread
From: Alexandru Ardelean @ 2019-09-13 11:55 UTC (permalink / raw)
  To: linux-iio, linux-spi, linux-kernel; +Cc: jic23, broonie, Alexandru Ardelean

Since the logic for `spi_delay` struct + `spi_delay_exec()` has been copied
from the `cs_change_delay` logic, it's natural to make this delay, the
first user.

The `cs_change_delay` logic requires that the default remain 10 uS, in case
it is unspecified/unconfigured. So, there is some special handling needed
to do that.

Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/spi/spi.c       | 28 +++++++---------------------
 include/linux/spi/spi.h |  4 +---
 2 files changed, 8 insertions(+), 24 deletions(-)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 1883de8ffa82..d0bf0ffca042 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -1160,9 +1160,9 @@ EXPORT_SYMBOL_GPL(spi_delay_exec);
 static void _spi_transfer_cs_change_delay(struct spi_message *msg,
 					  struct spi_transfer *xfer)
 {
-	u32 delay = xfer->cs_change_delay;
-	u32 unit = xfer->cs_change_delay_unit;
-	u32 hz;
+	u32 delay = xfer->cs_change_delay.value;
+	u32 unit = xfer->cs_change_delay.unit;
+	int ret;
 
 	/* return early on "fast" mode - for everything but USECS */
 	if (!delay) {
@@ -1171,27 +1171,13 @@ static void _spi_transfer_cs_change_delay(struct spi_message *msg,
 		return;
 	}
 
-	switch (unit) {
-	case SPI_DELAY_UNIT_USECS:
-		delay *= 1000;
-		break;
-	case SPI_DELAY_UNIT_NSECS: /* nothing to do here */
-		break;
-	case SPI_DELAY_UNIT_SCK:
-		/* if there is no effective speed know, then approximate
-		 * by underestimating with half the requested hz
-		 */
-		hz = xfer->effective_speed_hz ?: xfer->speed_hz / 2;
-		delay *= DIV_ROUND_UP(1000000000, hz);
-		break;
-	default:
+	ret = spi_delay_exec(&xfer->cs_change_delay, xfer);
+	if (ret) {
 		dev_err_once(&msg->spi->dev,
 			     "Use of unsupported delay unit %i, using default of 10us\n",
-			     xfer->cs_change_delay_unit);
-		delay = 10000;
+			     unit);
+		_spi_transfer_delay_ns(10000);
 	}
-	/* now sleep for the requested amount of time */
-	_spi_transfer_delay_ns(delay);
 }
 
 /*
diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
index c18cfa7cda35..9ded3f44d58e 100644
--- a/include/linux/spi/spi.h
+++ b/include/linux/spi/spi.h
@@ -754,7 +754,6 @@ extern void spi_res_release(struct spi_controller *ctlr,
  * @cs_change: affects chipselect after this transfer completes
  * @cs_change_delay: delay between cs deassert and assert when
  *      @cs_change is set and @spi_transfer is not the last in @spi_message
- * @cs_change_delay_unit: unit of cs_change_delay
  * @delay_usecs: microseconds to delay after this transfer before
  *	(optionally) changing the chipselect status, then starting
  *	the next transfer or completing this @spi_message.
@@ -847,8 +846,7 @@ struct spi_transfer {
 	u8		bits_per_word;
 	u8		word_delay_usecs;
 	u16		delay_usecs;
-	u16		cs_change_delay;
-	u8		cs_change_delay_unit;
+	struct spi_delay	cs_change_delay;
 	u32		speed_hz;
 	u16		word_delay;
 
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH v2 4/4] iio: imu: adis: convert cs_change_delay to spi_delay struct
  2019-09-13 11:55 [PATCH v2 0/4] spi: introduce `struct spi_delay` data-type Alexandru Ardelean
                   ` (2 preceding siblings ...)
  2019-09-13 11:55 ` [PATCH v2 3/4] spi: make `cs_change_delay` the first user of the `spi_delay` logic Alexandru Ardelean
@ 2019-09-13 11:55 ` Alexandru Ardelean
  2019-09-15 10:14   ` Jonathan Cameron
  3 siblings, 1 reply; 7+ messages in thread
From: Alexandru Ardelean @ 2019-09-13 11:55 UTC (permalink / raw)
  To: linux-iio, linux-spi, linux-kernel; +Cc: jic23, broonie, Alexandru Ardelean

The ADIS library is one of the few users of the new `cs_change_delay`
parameter for an spi_transfer.

The introduction of the `spi_delay` struct, requires that the users of of
`cs_change_delay` get an update. This change updates the ADIS library.

Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/iio/imu/adis.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/iio/imu/adis.c b/drivers/iio/imu/adis.c
index 1631c255deab..2cd2cc2316c6 100644
--- a/drivers/iio/imu/adis.c
+++ b/drivers/iio/imu/adis.c
@@ -39,24 +39,24 @@ int adis_write_reg(struct adis *adis, unsigned int reg,
 			.len = 2,
 			.cs_change = 1,
 			.delay_usecs = adis->data->write_delay,
-			.cs_change_delay = adis->data->cs_change_delay,
-			.cs_change_delay_unit = SPI_DELAY_UNIT_USECS,
+			.cs_change_delay.value = adis->data->cs_change_delay,
+			.cs_change_delay.unit = SPI_DELAY_UNIT_USECS,
 		}, {
 			.tx_buf = adis->tx + 2,
 			.bits_per_word = 8,
 			.len = 2,
 			.cs_change = 1,
 			.delay_usecs = adis->data->write_delay,
-			.cs_change_delay = adis->data->cs_change_delay,
-			.cs_change_delay_unit = SPI_DELAY_UNIT_USECS,
+			.cs_change_delay.value = adis->data->cs_change_delay,
+			.cs_change_delay.unit = SPI_DELAY_UNIT_USECS,
 		}, {
 			.tx_buf = adis->tx + 4,
 			.bits_per_word = 8,
 			.len = 2,
 			.cs_change = 1,
 			.delay_usecs = adis->data->write_delay,
-			.cs_change_delay = adis->data->cs_change_delay,
-			.cs_change_delay_unit = SPI_DELAY_UNIT_USECS,
+			.cs_change_delay.value = adis->data->cs_change_delay,
+			.cs_change_delay.unit = SPI_DELAY_UNIT_USECS,
 		}, {
 			.tx_buf = adis->tx + 6,
 			.bits_per_word = 8,
@@ -139,16 +139,16 @@ int adis_read_reg(struct adis *adis, unsigned int reg,
 			.len = 2,
 			.cs_change = 1,
 			.delay_usecs = adis->data->write_delay,
-			.cs_change_delay = adis->data->cs_change_delay,
-			.cs_change_delay_unit = SPI_DELAY_UNIT_USECS,
+			.cs_change_delay.value = adis->data->cs_change_delay,
+			.cs_change_delay.unit = SPI_DELAY_UNIT_USECS,
 		}, {
 			.tx_buf = adis->tx + 2,
 			.bits_per_word = 8,
 			.len = 2,
 			.cs_change = 1,
 			.delay_usecs = adis->data->read_delay,
-			.cs_change_delay = adis->data->cs_change_delay,
-			.cs_change_delay_unit = SPI_DELAY_UNIT_USECS,
+			.cs_change_delay.value = adis->data->cs_change_delay,
+			.cs_change_delay.unit = SPI_DELAY_UNIT_USECS,
 		}, {
 			.tx_buf = adis->tx + 4,
 			.rx_buf = adis->rx,
@@ -156,8 +156,8 @@ int adis_read_reg(struct adis *adis, unsigned int reg,
 			.len = 2,
 			.cs_change = 1,
 			.delay_usecs = adis->data->read_delay,
-			.cs_change_delay = adis->data->cs_change_delay,
-			.cs_change_delay_unit = SPI_DELAY_UNIT_USECS,
+			.cs_change_delay.value = adis->data->cs_change_delay,
+			.cs_change_delay.unit = SPI_DELAY_UNIT_USECS,
 		}, {
 			.rx_buf = adis->rx + 2,
 			.bits_per_word = 8,
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH v2 4/4] iio: imu: adis: convert cs_change_delay to spi_delay struct
  2019-09-13 11:55 ` [PATCH v2 4/4] iio: imu: adis: convert cs_change_delay to spi_delay struct Alexandru Ardelean
@ 2019-09-15 10:14   ` Jonathan Cameron
  2019-09-16  6:58     ` Ardelean, Alexandru
  0 siblings, 1 reply; 7+ messages in thread
From: Jonathan Cameron @ 2019-09-15 10:14 UTC (permalink / raw)
  To: Alexandru Ardelean; +Cc: linux-iio, linux-spi, linux-kernel, broonie

On Fri, 13 Sep 2019 14:55:49 +0300
Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:

> The ADIS library is one of the few users of the new `cs_change_delay`
> parameter for an spi_transfer.
> 
> The introduction of the `spi_delay` struct, requires that the users of of
> `cs_change_delay` get an update. This change updates the ADIS library.
> 
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>

Looks to me like the build is broken between patches 3 and 4.
Don't do that as it breaks bisectability.

If you are changing an interface like this it has to occur in one patch,
of you have to have intermediate code that deals with the smooth transition.

Otherwise, looks like a sensible bit of rework to me.

Jonathan

> ---
>  drivers/iio/imu/adis.c | 24 ++++++++++++------------
>  1 file changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/iio/imu/adis.c b/drivers/iio/imu/adis.c
> index 1631c255deab..2cd2cc2316c6 100644
> --- a/drivers/iio/imu/adis.c
> +++ b/drivers/iio/imu/adis.c
> @@ -39,24 +39,24 @@ int adis_write_reg(struct adis *adis, unsigned int reg,
>  			.len = 2,
>  			.cs_change = 1,
>  			.delay_usecs = adis->data->write_delay,
> -			.cs_change_delay = adis->data->cs_change_delay,
> -			.cs_change_delay_unit = SPI_DELAY_UNIT_USECS,
> +			.cs_change_delay.value = adis->data->cs_change_delay,
> +			.cs_change_delay.unit = SPI_DELAY_UNIT_USECS,
>  		}, {
>  			.tx_buf = adis->tx + 2,
>  			.bits_per_word = 8,
>  			.len = 2,
>  			.cs_change = 1,
>  			.delay_usecs = adis->data->write_delay,
> -			.cs_change_delay = adis->data->cs_change_delay,
> -			.cs_change_delay_unit = SPI_DELAY_UNIT_USECS,
> +			.cs_change_delay.value = adis->data->cs_change_delay,
> +			.cs_change_delay.unit = SPI_DELAY_UNIT_USECS,
>  		}, {
>  			.tx_buf = adis->tx + 4,
>  			.bits_per_word = 8,
>  			.len = 2,
>  			.cs_change = 1,
>  			.delay_usecs = adis->data->write_delay,
> -			.cs_change_delay = adis->data->cs_change_delay,
> -			.cs_change_delay_unit = SPI_DELAY_UNIT_USECS,
> +			.cs_change_delay.value = adis->data->cs_change_delay,
> +			.cs_change_delay.unit = SPI_DELAY_UNIT_USECS,
>  		}, {
>  			.tx_buf = adis->tx + 6,
>  			.bits_per_word = 8,
> @@ -139,16 +139,16 @@ int adis_read_reg(struct adis *adis, unsigned int reg,
>  			.len = 2,
>  			.cs_change = 1,
>  			.delay_usecs = adis->data->write_delay,
> -			.cs_change_delay = adis->data->cs_change_delay,
> -			.cs_change_delay_unit = SPI_DELAY_UNIT_USECS,
> +			.cs_change_delay.value = adis->data->cs_change_delay,
> +			.cs_change_delay.unit = SPI_DELAY_UNIT_USECS,
>  		}, {
>  			.tx_buf = adis->tx + 2,
>  			.bits_per_word = 8,
>  			.len = 2,
>  			.cs_change = 1,
>  			.delay_usecs = adis->data->read_delay,
> -			.cs_change_delay = adis->data->cs_change_delay,
> -			.cs_change_delay_unit = SPI_DELAY_UNIT_USECS,
> +			.cs_change_delay.value = adis->data->cs_change_delay,
> +			.cs_change_delay.unit = SPI_DELAY_UNIT_USECS,
>  		}, {
>  			.tx_buf = adis->tx + 4,
>  			.rx_buf = adis->rx,
> @@ -156,8 +156,8 @@ int adis_read_reg(struct adis *adis, unsigned int reg,
>  			.len = 2,
>  			.cs_change = 1,
>  			.delay_usecs = adis->data->read_delay,
> -			.cs_change_delay = adis->data->cs_change_delay,
> -			.cs_change_delay_unit = SPI_DELAY_UNIT_USECS,
> +			.cs_change_delay.value = adis->data->cs_change_delay,
> +			.cs_change_delay.unit = SPI_DELAY_UNIT_USECS,
>  		}, {
>  			.rx_buf = adis->rx + 2,
>  			.bits_per_word = 8,


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v2 4/4] iio: imu: adis: convert cs_change_delay to spi_delay struct
  2019-09-15 10:14   ` Jonathan Cameron
@ 2019-09-16  6:58     ` Ardelean, Alexandru
  0 siblings, 0 replies; 7+ messages in thread
From: Ardelean, Alexandru @ 2019-09-16  6:58 UTC (permalink / raw)
  To: jic23; +Cc: linux-spi, broonie, linux-kernel, linux-iio

On Sun, 2019-09-15 at 11:14 +0100, Jonathan Cameron wrote:
> [External]
> 
> On Fri, 13 Sep 2019 14:55:49 +0300
> Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:
> 
> > The ADIS library is one of the few users of the new `cs_change_delay`
> > parameter for an spi_transfer.
> > 
> > The introduction of the `spi_delay` struct, requires that the users of of
> > `cs_change_delay` get an update. This change updates the ADIS library.
> > 
> > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> 
> Looks to me like the build is broken between patches 3 and 4.
> Don't do that as it breaks bisectability.
> 
> If you are changing an interface like this it has to occur in one patch,
> of you have to have intermediate code that deals with the smooth transition.
> 
> Otherwise, looks like a sensible bit of rework to me.

I thought about it, but wasn't sure.
Will create a v3 with patches 3 & 4 squashed.

Thanks
Alex

> 
> Jonathan
> 
> > ---
> >  drivers/iio/imu/adis.c | 24 ++++++++++++------------
> >  1 file changed, 12 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/iio/imu/adis.c b/drivers/iio/imu/adis.c
> > index 1631c255deab..2cd2cc2316c6 100644
> > --- a/drivers/iio/imu/adis.c
> > +++ b/drivers/iio/imu/adis.c
> > @@ -39,24 +39,24 @@ int adis_write_reg(struct adis *adis, unsigned int reg,
> >  			.len = 2,
> >  			.cs_change = 1,
> >  			.delay_usecs = adis->data->write_delay,
> > -			.cs_change_delay = adis->data->cs_change_delay,
> > -			.cs_change_delay_unit = SPI_DELAY_UNIT_USECS,
> > +			.cs_change_delay.value = adis->data->cs_change_delay,
> > +			.cs_change_delay.unit = SPI_DELAY_UNIT_USECS,
> >  		}, {
> >  			.tx_buf = adis->tx + 2,
> >  			.bits_per_word = 8,
> >  			.len = 2,
> >  			.cs_change = 1,
> >  			.delay_usecs = adis->data->write_delay,
> > -			.cs_change_delay = adis->data->cs_change_delay,
> > -			.cs_change_delay_unit = SPI_DELAY_UNIT_USECS,
> > +			.cs_change_delay.value = adis->data->cs_change_delay,
> > +			.cs_change_delay.unit = SPI_DELAY_UNIT_USECS,
> >  		}, {
> >  			.tx_buf = adis->tx + 4,
> >  			.bits_per_word = 8,
> >  			.len = 2,
> >  			.cs_change = 1,
> >  			.delay_usecs = adis->data->write_delay,
> > -			.cs_change_delay = adis->data->cs_change_delay,
> > -			.cs_change_delay_unit = SPI_DELAY_UNIT_USECS,
> > +			.cs_change_delay.value = adis->data->cs_change_delay,
> > +			.cs_change_delay.unit = SPI_DELAY_UNIT_USECS,
> >  		}, {
> >  			.tx_buf = adis->tx + 6,
> >  			.bits_per_word = 8,
> > @@ -139,16 +139,16 @@ int adis_read_reg(struct adis *adis, unsigned int reg,
> >  			.len = 2,
> >  			.cs_change = 1,
> >  			.delay_usecs = adis->data->write_delay,
> > -			.cs_change_delay = adis->data->cs_change_delay,
> > -			.cs_change_delay_unit = SPI_DELAY_UNIT_USECS,
> > +			.cs_change_delay.value = adis->data->cs_change_delay,
> > +			.cs_change_delay.unit = SPI_DELAY_UNIT_USECS,
> >  		}, {
> >  			.tx_buf = adis->tx + 2,
> >  			.bits_per_word = 8,
> >  			.len = 2,
> >  			.cs_change = 1,
> >  			.delay_usecs = adis->data->read_delay,
> > -			.cs_change_delay = adis->data->cs_change_delay,
> > -			.cs_change_delay_unit = SPI_DELAY_UNIT_USECS,
> > +			.cs_change_delay.value = adis->data->cs_change_delay,
> > +			.cs_change_delay.unit = SPI_DELAY_UNIT_USECS,
> >  		}, {
> >  			.tx_buf = adis->tx + 4,
> >  			.rx_buf = adis->rx,
> > @@ -156,8 +156,8 @@ int adis_read_reg(struct adis *adis, unsigned int reg,
> >  			.len = 2,
> >  			.cs_change = 1,
> >  			.delay_usecs = adis->data->read_delay,
> > -			.cs_change_delay = adis->data->cs_change_delay,
> > -			.cs_change_delay_unit = SPI_DELAY_UNIT_USECS,
> > +			.cs_change_delay.value = adis->data->cs_change_delay,
> > +			.cs_change_delay.unit = SPI_DELAY_UNIT_USECS,
> >  		}, {
> >  			.rx_buf = adis->rx + 2,
> >  			.bits_per_word = 8,

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2019-09-16  6:58 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-13 11:55 [PATCH v2 0/4] spi: introduce `struct spi_delay` data-type Alexandru Ardelean
2019-09-13 11:55 ` [PATCH v2 1/4] spi: move `cs_change_delay` backwards compat logic outside switch Alexandru Ardelean
2019-09-13 11:55 ` [PATCH v2 2/4] spi: introduce spi_delay struct as "value + unit" & spi_delay_exec() Alexandru Ardelean
2019-09-13 11:55 ` [PATCH v2 3/4] spi: make `cs_change_delay` the first user of the `spi_delay` logic Alexandru Ardelean
2019-09-13 11:55 ` [PATCH v2 4/4] iio: imu: adis: convert cs_change_delay to spi_delay struct Alexandru Ardelean
2019-09-15 10:14   ` Jonathan Cameron
2019-09-16  6:58     ` Ardelean, Alexandru

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).