[1/4,V2] drivers: spi: core: Add optional delay between cs_change transfers
diff mbox series

Message ID 20190717115109.15168-2-alexandru.ardelean@analog.com
State New
Headers show
Series
  • iio: imu: Add support for the ADIS16460 IMU
Related show

Commit Message

Alexandru Ardelean July 17, 2019, 11:51 a.m. UTC
Some devices like the ADIS16460 IMU require a stall period between
transfers, i.e. between when the CS is de-asserted and re-asserted. The
default value of 10us is not enough. This change makes the delay
configurable for when the next CS change goes active.

Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>
Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/spi/spi.c       | 3 ++-
 include/linux/spi/spi.h | 3 +++
 2 files changed, 5 insertions(+), 1 deletion(-)

Comments

Mark Brown July 18, 2019, 12:50 p.m. UTC | #1
On Wed, Jul 17, 2019 at 02:51:06PM +0300, Alexandru Ardelean wrote:
> Some devices like the ADIS16460 IMU require a stall period between
> transfers, i.e. between when the CS is de-asserted and re-asserted. The
> default value of 10us is not enough. This change makes the delay
> configurable for when the next CS change goes active.

To repeat my previous feedback:

| This looks like cs_change_delay.

Please use subject lines matching the style for the subsystem.  This
makes it easier for people to identify relevant patches.

Please don't ignore review comments, people are generally making them
for a reason and are likely to have the same concerns if issues remain
unaddressed.  Having to repeat the same comments can get repetitive and
make people question the value of time spent reviewing.  If you disagree
with the review comments that's fine but you need to reply and discuss
your concerns so that the reviewer can understand your decisions.
Alexandru Ardelean July 18, 2019, 1:36 p.m. UTC | #2
On Thu, 2019-07-18 at 13:50 +0100, Mark Brown wrote:
> On Wed, Jul 17, 2019 at 02:51:06PM +0300, Alexandru Ardelean wrote:
> > Some devices like the ADIS16460 IMU require a stall period between
> > transfers, i.e. between when the CS is de-asserted and re-asserted. The
> > default value of 10us is not enough. This change makes the delay
> > configurable for when the next CS change goes active.
> 
> To repeat my previous feedback:
> 
> > This looks like cs_change_delay.

Ack.
Will use `cs_change_delay`.
I have no strong preference regarding the name.

> 
> Please use subject lines matching the style for the subsystem.  This
> makes it easier for people to identify relevant patches.

Ack.
Will look for SPI subsystem specific subject lines and use them.

> 
> Please don't ignore review comments, people are generally making them
> for a reason and are likely to have the same concerns if issues remain
> unaddressed.  Having to repeat the same comments can get repetitive and
> make people question the value of time spent reviewing.  If you disagree
> with the review comments that's fine but you need to reply and discuss
> your concerns so that the reviewer can understand your decisions.

[ the following part should not be considered in a disrespectful tone ; the intent is nowhere near that, but text-
communication has a design-flaw where a disrespectful tone may be interpreted [where there isn't one] ]

My intent wasn't to ignore the review comment.
Sorry if it came out like that.

I assumed a patch re-spin was preferred vs a verbal discussion.
Some people prefer patch re-spins as a basis for discussion.
Various people have various preferences.

Also, I wasn't sure how soon I'd get a reply back on this, since various people/maintainers have various reply-times.
And I also [sometimes] have longer reply-back-times [which doesn't help either].
And I wasn't sure if `cs_change_delay` was fully intentional/ad-literam, or whether it was a feedback of the sorts
"along-the-lines of `cs_change_delay`".

While looking at `struct spi_transfer` the other "delay" fields seem to be: `word_delay_usecs` & `delay_usecs`, which is
why I assumed `cs_change_delay_usecs` was preferred [though I will admit, it is a long var-name].

And the conclusion [from my side] is: maybe I rushed this a bit and maybe it annoyed you.
Not my intention, and it'll take me a bit to adjust to your style.

Moving forward.

1. I will use `cs_change_delay` as field name 
2. I will use SPI subsystem subject line; I will admit I forget this stuff periodically

Is there anything else I should consider?
Or anything else to discuss?

I'm open to elements I may have forgotten/omitted unintentionally.

Thanks
Alex

Patch
diff mbox series

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 5e75944ad5d1..02fd00bcaace 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -1163,7 +1163,8 @@  static int spi_transfer_one_message(struct spi_controller *ctlr,
 				keep_cs = true;
 			} else {
 				spi_set_cs(msg->spi, false);
-				udelay(10);
+				udelay(xfer->cs_change_delay_usecs ?
+				       xfer->cs_change_delay_usecs : 10);
 				spi_set_cs(msg->spi, true);
 			}
 		}
diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
index 053abd22ad31..c884b3b94841 100644
--- a/include/linux/spi/spi.h
+++ b/include/linux/spi/spi.h
@@ -734,6 +734,8 @@  extern void spi_res_release(struct spi_controller *ctlr,
  *      transfer. If 0 the default (from @spi_device) is used.
  * @bits_per_word: select a bits_per_word other than the device default
  *      for this transfer. If 0 the default (from @spi_device) is used.
+ * @cs_change_delay_usecs: microseconds to delay between cs_change
+ *	transfers.
  * @cs_change: affects chipselect after this transfer completes
  * @delay_usecs: microseconds to delay after this transfer before
  *	(optionally) changing the chipselect status, then starting
@@ -823,6 +825,7 @@  struct spi_transfer {
 #define	SPI_NBITS_QUAD		0x04 /* 4bits transfer */
 	u8		bits_per_word;
 	u8		word_delay_usecs;
+	u8		cs_change_delay_usecs;
 	u16		delay_usecs;
 	u32		speed_hz;
 	u16		word_delay;