Re: [PATCH 1/4][V2] drivers: spi: core: Add optional delay between cs_change transfers
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
Re: [PATCH 1/4][V2] drivers: spi: core: Add optional delay between cs_change transfers
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. signature.asc Description: PGP signature
[PATCH 1/4][V2] drivers: spi: core: Add optional delay between cs_change transfers
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
Signed-off-by: Alexandru Ardelean
---
drivers/spi/spi.c | 3 ++-
include/linux/spi/spi.h | 3 +++
2 files changed, 5 insertions(+), 1 deletion(-)
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 {
#defineSPI_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;
--
2.20.1
