[RFC,03/15] spi: make `cs_change_delay` the first user of the `spi_delay` logic
diff mbox series

Message ID 20190913114550.956-4-alexandru.ardelean@analog.com
State New
Headers show
Series
  • Unify SPI delays into an `struct spi_delay`
Related show

Commit Message

Alexandru Ardelean Sept. 13, 2019, 11:45 a.m. UTC
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(-)

Comments

Mark Brown Sept. 16, 2019, 12:25 p.m. UTC | #1
On Fri, Sep 13, 2019 at 02:45:38PM +0300, Alexandru Ardelean wrote:

> -	u16		cs_change_delay;
> -	u8		cs_change_delay_unit;
> +	struct spi_delay	cs_change_delay;

This breaks the build as there is a user of this interface.
Alexandru Ardelean Sept. 16, 2019, 12:37 p.m. UTC | #2
On Mon, 2019-09-16 at 13:25 +0100, Mark Brown wrote:
> [External]
> 
> On Fri, Sep 13, 2019 at 02:45:38PM +0300, Alexandru Ardelean wrote:
> 
> > -	u16		cs_change_delay;
> > -	u8		cs_change_delay_unit;
> > +	struct spi_delay	cs_change_delay;
> 
> This breaks the build as there is a user of this interface.


Ack.
Jonathan pointed this out.
There's a V3 that changes both this and it's user (in IIO).

V3:
https://lore.kernel.org/linux-iio/20190916071024.21447-1-alexandru.ardelean@analog.com/T/#t

V2:
https://lore.kernel.org/linux-iio/20190913115549.3823-1-alexandru.ardelean@analog.com/T/#t

[ archive is from the IIO list ]

Well, I'm hoping you are referring to the same user.

On a general note: I apologise for the amount of noise/spam I am doing here. Still adjusting to how to do things/changes
that touch 2 subsystems, especially when trees are not quite in-sync.
Mark Brown Sept. 16, 2019, 12:47 p.m. UTC | #3
On Mon, Sep 16, 2019 at 12:37:12PM +0000, Ardelean, Alexandru wrote:

> > This breaks the build as there is a user of this interface.

> Ack.
> Jonathan pointed this out.
> There's a V3 that changes both this and it's user (in IIO).

That v3 seems to be a small subset of this series?
Alexandru Ardelean Sept. 16, 2019, 1:04 p.m. UTC | #4
On Mon, 2019-09-16 at 13:47 +0100, Mark Brown wrote:
> [External]
> 
> On Mon, Sep 16, 2019 at 12:37:12PM +0000, Ardelean, Alexandru wrote:
> 
> > > This breaks the build as there is a user of this interface.
> > Ack.
> > Jonathan pointed this out.
> > There's a V3 that changes both this and it's user (in IIO).
> 
> That v3 seems to be a small subset of this series?

Ack.
V3 is the first 4 patches from this series.
Well, patches 3 & 4 are squashed.

I am 100% convinced that the entire series is a good idea.
In the sense that a `struct spi_delay` may be a good idea, but at the same time, it may be un-needed.

All I wanted to do, was to add another delay somewhere, and got lost in the rework of current delays.
I thought about proposing just the first 4 patches [on their own], but I thought that showing the current series as-is
now, may be a good idea as well [to gather some feedback].
Mark Brown Sept. 16, 2019, 1:43 p.m. UTC | #5
On Mon, Sep 16, 2019 at 01:04:42PM +0000, Ardelean, Alexandru wrote:
> On Mon, 2019-09-16 at 13:47 +0100, Mark Brown wrote:

> > That v3 seems to be a small subset of this series?

> Ack.
> V3 is the first 4 patches from this series.
> Well, patches 3 & 4 are squashed.

> I am 100% convinced that the entire series is a good idea.
> In the sense that a `struct spi_delay` may be a good idea, but at the same time, it may be un-needed.

> All I wanted to do, was to add another delay somewhere, and got lost in the rework of current delays.
> I thought about proposing just the first 4 patches [on their own], but I thought that showing the current series as-is
> now, may be a good idea as well [to gather some feedback].

I think it makes more sense to review as a whole series rather than only
a part of the conversion, it doesn't really help to only do part of it.

Please fix your mail client to word wrap within paragraphs at something
substantially less than 80 columns.  Doing this makes your messages much
easier to read and reply to.
Alexandru Ardelean Sept. 17, 2019, 6:05 a.m. UTC | #6
On Mon, 2019-09-16 at 14:43 +0100, Mark Brown wrote:
> [External]
> 
> On Mon, Sep 16, 2019 at 01:04:42PM +0000, Ardelean, Alexandru wrote:
> > On Mon, 2019-09-16 at 13:47 +0100, Mark Brown wrote:
> > > That v3 seems to be a small subset of this series?
> > Ack.
> > V3 is the first 4 patches from this series.
> > Well, patches 3 & 4 are squashed.
> > I am 100% convinced that the entire series is a good idea.

Something happened here to the "not" word.
Probably got lost in an alternate dimension  ¯\_(ツ)_/¯ .

Was supposed to be:
"I am not 100% convinced that the entire series is a good idea."


> > In the sense that a `struct spi_delay` may be a good idea, but at the
> > same time, it may be un-needed.
> > All I wanted to do, was to add another delay somewhere, and got lost in
> > the rework of current delays.
> > I thought about proposing just the first 4 patches [on their own], but
> > I thought that showing the current series as-is
> > now, may be a good idea as well [to gather some feedback].
> 
> I think it makes more sense to review as a whole series rather than only
> a part of the conversion, it doesn't really help to only do part of it.
> 
> Please fix your mail client to word wrap within paragraphs at something
> substantially less than 80 columns.  Doing this makes your messages much
> easier to read and reply to.

Ack.
Problem is: I have to re-setup my email client every now-n-then since the
work-email server has some issues with Linux email clients.
And I sometimes forget to configure this.
[ Exchange does not always get along well with non-Outlook clients ]

Patch
diff mbox series

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;