| Message ID | a77a6bb8b3abf1c127014c78f71533c24afacd93.1534423916.git.baolin.wang@linaro.org (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | eeaceb8b7d1fb64b6030249ca0dd1d902ef3069e |
| Headers | show |
| Series | [v2,1/3] spi: Introduce one new field to set word delay | expand |
On čtvrtek 16. srpna 2018 14:54:49 CEST, Baolin Wang wrote: > + * @word_delay: clock cycles to inter word delay after each word size > + * (set by bits_per_word) transmission. I need a similar functionality for talking to a SPI device from userspace -- see my attempt for implementing this in spi-orion.c at [1]. The device's datasheet says that I should wait, e.g., 3µs between each two words. I therefore like this patch :). The description can be improved because it left me wondering what "clock this is about. I suppose it's about the SPI clock cycles and not CPU clock cycles, right? I'll be hapy to patch this once Baolin confirms that that is the intended meaning. It seems that this is only implemented in one newly added driver. I'm interested in supporting this in spi-orion.c, but that sounds like driver-specific work for something which is pretty generic. How should this be implemented? Given that drivers for SPI masters can implement a function which transfers several words at once, there are not that many better possibilities than adding udelay()s, though. Thoughts? What is your plan to do with drivers which do not implement this (yet)? If a spi_transfer gets queued which asks for a word_delay delay, it is silently ignored now, AFAIU. What about userspace support, spidev and spi_ioc_transfer (that's my target, actually)? Is it OK to s/pad/word_delay/ in the spidev code and pass that to the generated struct spi_transfer? In my opinion, once we support specifying this from userspace, one has to definitely check that the SPI controller is ready to honor this request. Do we want a new bit in spi_controller.flags for this? With kind regards, Jan [1] https://patchwork.kernel.org/patch/10221397/
On Wed, Aug 29, 2018 at 01:33:24PM +0200, Jan Kundrát wrote: > On čtvrtek 16. srpna 2018 14:54:49 CEST, Baolin Wang wrote: > > + * @word_delay: clock cycles to inter word delay after each word size > > + * (set by bits_per_word) transmission. > The description can be improved because it left me wondering what "clock > this is about. I suppose it's about the SPI clock cycles and not CPU clock > cycles, right? I'll be hapy to patch this once Baolin confirms that that is > the intended meaning. That's certainly how I read it. > It seems that this is only implemented in one newly added driver. I'm > interested in supporting this in spi-orion.c, but that sounds like > driver-specific work for something which is pretty generic. How should this > be implemented? Given that drivers for SPI masters can implement a function > which transfers several words at once, there are not that many better > possibilities than adding udelay()s, though. Thoughts? Yeah, you'd need to split the transfer into words and then add a delay between which would be rather expensive but it's about as good as we can get I think. > What is your plan to do with drivers which do not implement this (yet)? If a > spi_transfer gets queued which asks for a word_delay delay, it is silently > ignored now, AFAIU. Yes. A generic handler would be best. > What about userspace support, spidev and spi_ioc_transfer (that's my target, > actually)? Is it OK to s/pad/word_delay/ in the spidev code and pass that to > the generated struct spi_transfer? In my opinion, once we support specifying > this from userspace, one has to definitely check that the SPI controller is > ready to honor this request. Do we want a new bit in spi_controller.flags > for this? Not seeing pad in the spidev code? A feature flag would make sense along with a generic implementation.
On 29 August 2018 at 19:33, Jan Kundrát <jan.kundrat@cesnet.cz> wrote: > On čtvrtek 16. srpna 2018 14:54:49 CEST, Baolin Wang wrote: >> >> + * @word_delay: clock cycles to inter word delay after each word size >> + * (set by bits_per_word) transmission. > > > I need a similar functionality for talking to a SPI device from userspace > -- see my attempt for implementing this in spi-orion.c at [1]. The device's > datasheet says that I should wait, e.g., 3µs between each two words. I > therefore like this patch :). > > The description can be improved because it left me wondering what "clock > this is about. I suppose it's about the SPI clock cycles and not CPU clock > cycles, right? I'll be hapy to patch this once Baolin confirms that that is > the intended meaning. Sorry for confusing. Since our SPI word delay unit is clock cycle of the SPI clock, so my intention of the word_delay's unit is SPI clock cycles according to our previous discussion. > > It seems that this is only implemented in one newly added driver. I'm > interested in supporting this in spi-orion.c, but that sounds like > driver-specific work for something which is pretty generic. How should this > be implemented? Given that drivers for SPI masters can implement a function > which transfers several words at once, there are not that many better > possibilities than adding udelay()s, though. Thoughts? > > What is your plan to do with drivers which do not implement this (yet)? If a > spi_transfer gets queued which asks for a word_delay delay, it is silently > ignored now, AFAIU. > > What about userspace support, spidev and spi_ioc_transfer (that's my target, > actually)? Is it OK to s/pad/word_delay/ in the spidev code and pass that to > the generated struct spi_transfer? In my opinion, once we support specifying > this from userspace, one has to definitely check that the SPI controller is > ready to honor this request. Do we want a new bit in spi_controller.flags > for this? > > With kind regards, > Jan > > [1] https://patchwork.kernel.org/patch/10221397/
diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h index a64235e..d698f9d 100644 --- a/include/linux/spi/spi.h +++ b/include/linux/spi/spi.h @@ -711,6 +711,8 @@ extern void spi_res_release(struct spi_controller *ctlr, * @delay_usecs: microseconds to delay after this transfer before * (optionally) changing the chipselect status, then starting * the next transfer or completing this @spi_message. + * @word_delay: clock cycles to inter word delay after each word size + * (set by bits_per_word) transmission. * @transfer_list: transfers are sequenced through @spi_message.transfers * @tx_sg: Scatterlist for transmit, currently not for client use * @rx_sg: Scatterlist for receive, currently not for client use @@ -793,6 +795,7 @@ struct spi_transfer { u8 bits_per_word; u16 delay_usecs; u32 speed_hz; + u16 word_delay; struct list_head transfer_list; };
For some SPI controllers, after each word size (specified by bits_per_word) transimission, the hardware need some delay to make sure the slave has enough time to receive the whole data. So introducing one new 'word_delay' field of struct spi_tansfer for slave devices to set this inter word delay time. Signed-off-by: Baolin Wang <baolin.wang@linaro.org> --- Changes from v1: - New patch in v2. --- include/linux/spi/spi.h | 3 +++ 1 file changed, 3 insertions(+)