| Message ID | 1416264471-774-2-git-send-email-b.galvani@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Nov 17, 2014 at 11:47:48PM +0100, Beniamino Galvani wrote: > + list_for_each_entry(xfer, &master->cur_msg->transfers, transfer_list) { > + xfer->last = list_is_last(&xfer->transfer_list, > + &master->cur_msg->transfers); > + } It's incredibly sad to iterate through the entire list in order to find the last entry, especially given that it's a doubly linked list and this is a bit of a hot path. We should look at the previous entry for the list head instead, or perhaps better yet by doing this as part of spi_validate() which already itereates over the entire list and is where we do other similar fixups. Though looking at this I'm not sure that a flag is the best approach at all - why not just have the driver call list_is_last() in the transfer function or ideally provide an inline function that does that so that we can change the implementation later?
On Tue, Nov 18, 2014 at 02:06:58PM +0000, Mark Brown wrote: > It's incredibly sad to iterate through the entire list in order to find > the last entry, especially given that it's a doubly linked list and this > is a bit of a hot path. We should look at the previous entry for the > list head instead, or perhaps better yet by doing this as part of > spi_validate() which already itereates over the entire list and is where > we do other similar fixups. > > Though looking at this I'm not sure that a flag is the best approach at > all - why not just have the driver call list_is_last() in the transfer > function or ideally provide an inline function that does that so that we > can change the implementation later? I didn't realize that the master structure passed to transfer_one() has a reference to the current message and thus to the transfer list. Then yes, the additional flag in the transfer structure probably doesn't make much sense. Would it be better to introduce something like: static inline bool spi_transfer_is_last(struct spi_master *master, struct spi_transfer *xfer) { return list_is_last(&xfer->transfer_list, &master->cur_msg->transfers); } or open code it in the driver? Beniamino -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Nov 18, 2014 at 11:55:46PM +0100, Beniamino Galvani wrote: > Would it be better to introduce something like: > static inline bool > spi_transfer_is_last(struct spi_master *master, struct spi_transfer *xfer) > { > return list_is_last(&xfer->transfer_list, &master->cur_msg->transfers); > } > or open code it in the driver? Adding the helper function seems better, it means we can change the implementation later.
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c index ebcb33d..fc7f02d 100644 --- a/drivers/spi/spi.c +++ b/drivers/spi/spi.c @@ -887,6 +887,7 @@ static void spi_pump_messages(struct kthread_work *work) { struct spi_master *master = container_of(work, struct spi_master, pump_messages); + struct spi_transfer *xfer; unsigned long flags; bool was_busy = false; int ret; @@ -941,6 +942,11 @@ static void spi_pump_messages(struct kthread_work *work) } } + list_for_each_entry(xfer, &master->cur_msg->transfers, transfer_list) { + xfer->last = list_is_last(&xfer->transfer_list, + &master->cur_msg->transfers); + } + if (!was_busy) trace_spi_master_busy(master); diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h index 46d188a..37f055a 100644 --- a/include/linux/spi/spi.h +++ b/include/linux/spi/spi.h @@ -535,6 +535,7 @@ extern struct spi_master *spi_busnum_to_master(u16 busnum); * (SPI_NBITS_SINGLE) is used. * @rx_nbits: number of bits used for reading. If 0 the default * (SPI_NBITS_SINGLE) is used. + * @last: whether the transfer is the last in the message. * @len: size of rx and tx buffers (in bytes) * @speed_hz: Select a speed other than the device default for this * transfer. If 0 the default (from @spi_device) is used. @@ -620,6 +621,7 @@ struct spi_transfer { unsigned cs_change:1; unsigned tx_nbits:3; unsigned rx_nbits:3; + unsigned last:1; #define SPI_NBITS_SINGLE 0x01 /* 1bit transfer */ #define SPI_NBITS_DUAL 0x02 /* 2bits transfer */ #define SPI_NBITS_QUAD 0x04 /* 4bits transfer */
Some drivers need to know whether the current transfer is the last in the message in order to properly handle CS. A common way to achieve this is to reimplement transfer_one_message() but this leads to undesirable code duplication. This patch adds a 'last' field to the spi_transfer structure and populates it before passing the structure to the driver. Signed-off-by: Beniamino Galvani <b.galvani@gmail.com> --- drivers/spi/spi.c | 6 ++++++ include/linux/spi/spi.h | 2 ++ 2 files changed, 8 insertions(+)