| Message ID | 20190913114550.956-4-alexandru.ardelean@analog.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Unify SPI delays into an `struct spi_delay` | expand |
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.
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.
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?
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].
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.
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 ]
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;
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(-)