diff mbox series

[v2,2/5] spi: move splitting transfers to spi_optimize_message()

Message ID 20240219-mainline-spi-precook-message-v2-2-4a762c6701b9@baylibre.com (mailing list archive)
State Accepted
Commit fab53fea21a909e4e0656764a8ee7c356fe89d6f
Headers show
Series spi: add support for pre-cooking messages | expand

Commit Message

David Lechner Feb. 19, 2024, 10:33 p.m. UTC
Splitting transfers is an expensive operation so we can potentially
optimize it by doing it only once per optimization of the message
instead of repeating each time the message is transferred.

The transfer splitting functions are currently the only user of
spi_res_alloc() so spi_res_release() can be safely moved at this time
from spi_finalize_current_message() to spi_unoptimize_message().

The doc comments of the public functions for splitting transfers are
also updated so that callers will know when it is safe to call them
to ensure proper resource management.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Signed-off-by: David Lechner <dlechner@baylibre.com>
---

v2 changes:
- Changed line break for multiline if condition
- Removed kernel doc inclusion (/** -> /*) from static members
- Picked up Jonathan's Reviewed-by

 drivers/spi/spi.c | 110 +++++++++++++++++++++++++++++++++---------------------
 1 file changed, 68 insertions(+), 42 deletions(-)

Comments

Nuno Sá Feb. 20, 2024, 10:52 a.m. UTC | #1
On Mon, 2024-02-19 at 16:33 -0600, David Lechner wrote:
> Splitting transfers is an expensive operation so we can potentially
> optimize it by doing it only once per optimization of the message
> instead of repeating each time the message is transferred.
> 
> The transfer splitting functions are currently the only user of
> spi_res_alloc() so spi_res_release() can be safely moved at this time
> from spi_finalize_current_message() to spi_unoptimize_message().
> 
> The doc comments of the public functions for splitting transfers are
> also updated so that callers will know when it is safe to call them
> to ensure proper resource management.
> 
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Signed-off-by: David Lechner <dlechner@baylibre.com>
> ---

Acked-by: Nuno Sa <nuno.sa@analog.com>

> 
> v2 changes:
> - Changed line break for multiline if condition
> - Removed kernel doc inclusion (/** -> /*) from static members
> - Picked up Jonathan's Reviewed-by
> 
>  drivers/spi/spi.c | 110 +++++++++++++++++++++++++++++++++---------------------
>  1 file changed, 68 insertions(+), 42 deletions(-)
> 
> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> index f68d92b57543..ba4d3fde2054 100644
> --- a/drivers/spi/spi.c
> +++ b/drivers/spi/spi.c
> @@ -1747,38 +1747,6 @@ static int __spi_pump_transfer_message(struct spi_controller
> *ctlr,
>  
>  	trace_spi_message_start(msg);
>  
> -	/*
> -	 * If an SPI controller does not support toggling the CS line on each
> -	 * transfer (indicated by the SPI_CS_WORD flag) or we are using a GPIO
> -	 * for the CS line, we can emulate the CS-per-word hardware function by
> -	 * splitting transfers into one-word transfers and ensuring that
> -	 * cs_change is set for each transfer.
> -	 */
> -	if ((msg->spi->mode & SPI_CS_WORD) && (!(ctlr->mode_bits & SPI_CS_WORD) ||
> -					       spi_is_csgpiod(msg->spi))) {
> -		ret = spi_split_transfers_maxwords(ctlr, msg, 1);
> -		if (ret) {
> -			msg->status = ret;
> -			spi_finalize_current_message(ctlr);
> -			return ret;
> -		}
> -
> -		list_for_each_entry(xfer, &msg->transfers, transfer_list) {
> -			/* Don't change cs_change on the last entry in the list */
> -			if (list_is_last(&xfer->transfer_list, &msg->transfers))
> -				break;
> -			xfer->cs_change = 1;
> -		}
> -	} else {
> -		ret = spi_split_transfers_maxsize(ctlr, msg,
> -						  spi_max_transfer_size(msg-
> >spi));
> -		if (ret) {
> -			msg->status = ret;
> -			spi_finalize_current_message(ctlr);
> -			return ret;
> -		}
> -	}
> -
>  	if (ctlr->prepare_message) {
>  		ret = ctlr->prepare_message(ctlr, msg);
>  		if (ret) {
> @@ -2124,6 +2092,8 @@ static void __spi_unoptimize_message(struct spi_message *msg)
>  	if (ctlr->unoptimize_message)
>  		ctlr->unoptimize_message(msg);
>  
> +	spi_res_release(ctlr, msg);
> +
>  	msg->optimized = false;
>  	msg->opt_state = NULL;
>  }
> @@ -2169,15 +2139,6 @@ void spi_finalize_current_message(struct spi_controller
> *ctlr)
>  
>  	spi_unmap_msg(ctlr, mesg);
>  
> -	/*
> -	 * In the prepare_messages callback the SPI bus has the opportunity
> -	 * to split a transfer to smaller chunks.
> -	 *
> -	 * Release the split transfers here since spi_map_msg() is done on
> -	 * the split transfers.
> -	 */
> -	spi_res_release(ctlr, mesg);
> -
>  	if (mesg->prepared && ctlr->unprepare_message) {
>  		ret = ctlr->unprepare_message(ctlr, mesg);
>  		if (ret) {
> @@ -3819,6 +3780,10 @@ static int __spi_split_transfer_maxsize(struct
> spi_controller *ctlr,
>   * @msg:   the @spi_message to transform
>   * @maxsize:  the maximum when to apply this
>   *
> + * This function allocates resources that are automatically freed during the
> + * spi message unoptimize phase so this function should only be called from
> + * optimize_message callbacks.
> + *
>   * Return: status of transformation
>   */
>  int spi_split_transfers_maxsize(struct spi_controller *ctlr,
> @@ -3857,6 +3822,10 @@ EXPORT_SYMBOL_GPL(spi_split_transfers_maxsize);
>   * @msg:      the @spi_message to transform
>   * @maxwords: the number of words to limit each transfer to
>   *
> + * This function allocates resources that are automatically freed during the
> + * spi message unoptimize phase so this function should only be called from
> + * optimize_message callbacks.
> + *
>   * Return: status of transformation
>   */
>  int spi_split_transfers_maxwords(struct spi_controller *ctlr,
> @@ -4231,6 +4200,57 @@ static int __spi_validate(struct spi_device *spi, struct
> spi_message *message)
>  	return 0;
>  }
>  
> +/*
> + * spi_split_transfers - generic handling of transfer splitting
> + * @msg: the message to split
> + *
> + * Under certain conditions, a SPI controller may not support arbitrary
> + * transfer sizes or other features required by a peripheral. This function
> + * will split the transfers in the message into smaller transfers that are
> + * supported by the controller.
> + *
> + * Controllers with special requirements not covered here can also split
> + * transfers in the optimize_message() callback.
> + *
> + * Context: can sleep
> + * Return: zero on success, else a negative error code
> + */
> +static int spi_split_transfers(struct spi_message *msg)
> +{
> +	struct spi_controller *ctlr = msg->spi->controller;
> +	struct spi_transfer *xfer;
> +	int ret;
> +
> +	/*
> +	 * If an SPI controller does not support toggling the CS line on each
> +	 * transfer (indicated by the SPI_CS_WORD flag) or we are using a GPIO
> +	 * for the CS line, we can emulate the CS-per-word hardware function by
> +	 * splitting transfers into one-word transfers and ensuring that
> +	 * cs_change is set for each transfer.
> +	 */
> +	if ((msg->spi->mode & SPI_CS_WORD) &&
> +	    (!(ctlr->mode_bits & SPI_CS_WORD) || spi_is_csgpiod(msg->spi))) {
> +		ret = spi_split_transfers_maxwords(ctlr, msg, 1);
> +		if (ret)
> +			return ret;
> +
> +		list_for_each_entry(xfer, &msg->transfers, transfer_list) {
> +			/* Don't change cs_change on the last entry in the list */
> +			if (list_is_last(&xfer->transfer_list, &msg->transfers))
> +				break;
> +
> +			xfer->cs_change = 1;
> +		}
> +	} else {
> +		ret = spi_split_transfers_maxsize(ctlr, msg,
> +						  spi_max_transfer_size(msg-
> >spi));
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
>  /*
>   * __spi_optimize_message - shared implementation for spi_optimize_message()
>   *                          and spi_maybe_optimize_message()
> @@ -4254,10 +4274,16 @@ static int __spi_optimize_message(struct spi_device *spi,
>  	if (ret)
>  		return ret;
>  
> +	ret = spi_split_transfers(msg);
> +	if (ret)
> +		return ret;
> +
>  	if (ctlr->optimize_message) {
>  		ret = ctlr->optimize_message(msg);
> -		if (ret)
> +		if (ret) {
> +			spi_res_release(ctlr, msg);
>  			return ret;
> +		}
>  	}
>  
>  	msg->optimized = true;
>
diff mbox series

Patch

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index f68d92b57543..ba4d3fde2054 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -1747,38 +1747,6 @@  static int __spi_pump_transfer_message(struct spi_controller *ctlr,
 
 	trace_spi_message_start(msg);
 
-	/*
-	 * If an SPI controller does not support toggling the CS line on each
-	 * transfer (indicated by the SPI_CS_WORD flag) or we are using a GPIO
-	 * for the CS line, we can emulate the CS-per-word hardware function by
-	 * splitting transfers into one-word transfers and ensuring that
-	 * cs_change is set for each transfer.
-	 */
-	if ((msg->spi->mode & SPI_CS_WORD) && (!(ctlr->mode_bits & SPI_CS_WORD) ||
-					       spi_is_csgpiod(msg->spi))) {
-		ret = spi_split_transfers_maxwords(ctlr, msg, 1);
-		if (ret) {
-			msg->status = ret;
-			spi_finalize_current_message(ctlr);
-			return ret;
-		}
-
-		list_for_each_entry(xfer, &msg->transfers, transfer_list) {
-			/* Don't change cs_change on the last entry in the list */
-			if (list_is_last(&xfer->transfer_list, &msg->transfers))
-				break;
-			xfer->cs_change = 1;
-		}
-	} else {
-		ret = spi_split_transfers_maxsize(ctlr, msg,
-						  spi_max_transfer_size(msg->spi));
-		if (ret) {
-			msg->status = ret;
-			spi_finalize_current_message(ctlr);
-			return ret;
-		}
-	}
-
 	if (ctlr->prepare_message) {
 		ret = ctlr->prepare_message(ctlr, msg);
 		if (ret) {
@@ -2124,6 +2092,8 @@  static void __spi_unoptimize_message(struct spi_message *msg)
 	if (ctlr->unoptimize_message)
 		ctlr->unoptimize_message(msg);
 
+	spi_res_release(ctlr, msg);
+
 	msg->optimized = false;
 	msg->opt_state = NULL;
 }
@@ -2169,15 +2139,6 @@  void spi_finalize_current_message(struct spi_controller *ctlr)
 
 	spi_unmap_msg(ctlr, mesg);
 
-	/*
-	 * In the prepare_messages callback the SPI bus has the opportunity
-	 * to split a transfer to smaller chunks.
-	 *
-	 * Release the split transfers here since spi_map_msg() is done on
-	 * the split transfers.
-	 */
-	spi_res_release(ctlr, mesg);
-
 	if (mesg->prepared && ctlr->unprepare_message) {
 		ret = ctlr->unprepare_message(ctlr, mesg);
 		if (ret) {
@@ -3819,6 +3780,10 @@  static int __spi_split_transfer_maxsize(struct spi_controller *ctlr,
  * @msg:   the @spi_message to transform
  * @maxsize:  the maximum when to apply this
  *
+ * This function allocates resources that are automatically freed during the
+ * spi message unoptimize phase so this function should only be called from
+ * optimize_message callbacks.
+ *
  * Return: status of transformation
  */
 int spi_split_transfers_maxsize(struct spi_controller *ctlr,
@@ -3857,6 +3822,10 @@  EXPORT_SYMBOL_GPL(spi_split_transfers_maxsize);
  * @msg:      the @spi_message to transform
  * @maxwords: the number of words to limit each transfer to
  *
+ * This function allocates resources that are automatically freed during the
+ * spi message unoptimize phase so this function should only be called from
+ * optimize_message callbacks.
+ *
  * Return: status of transformation
  */
 int spi_split_transfers_maxwords(struct spi_controller *ctlr,
@@ -4231,6 +4200,57 @@  static int __spi_validate(struct spi_device *spi, struct spi_message *message)
 	return 0;
 }
 
+/*
+ * spi_split_transfers - generic handling of transfer splitting
+ * @msg: the message to split
+ *
+ * Under certain conditions, a SPI controller may not support arbitrary
+ * transfer sizes or other features required by a peripheral. This function
+ * will split the transfers in the message into smaller transfers that are
+ * supported by the controller.
+ *
+ * Controllers with special requirements not covered here can also split
+ * transfers in the optimize_message() callback.
+ *
+ * Context: can sleep
+ * Return: zero on success, else a negative error code
+ */
+static int spi_split_transfers(struct spi_message *msg)
+{
+	struct spi_controller *ctlr = msg->spi->controller;
+	struct spi_transfer *xfer;
+	int ret;
+
+	/*
+	 * If an SPI controller does not support toggling the CS line on each
+	 * transfer (indicated by the SPI_CS_WORD flag) or we are using a GPIO
+	 * for the CS line, we can emulate the CS-per-word hardware function by
+	 * splitting transfers into one-word transfers and ensuring that
+	 * cs_change is set for each transfer.
+	 */
+	if ((msg->spi->mode & SPI_CS_WORD) &&
+	    (!(ctlr->mode_bits & SPI_CS_WORD) || spi_is_csgpiod(msg->spi))) {
+		ret = spi_split_transfers_maxwords(ctlr, msg, 1);
+		if (ret)
+			return ret;
+
+		list_for_each_entry(xfer, &msg->transfers, transfer_list) {
+			/* Don't change cs_change on the last entry in the list */
+			if (list_is_last(&xfer->transfer_list, &msg->transfers))
+				break;
+
+			xfer->cs_change = 1;
+		}
+	} else {
+		ret = spi_split_transfers_maxsize(ctlr, msg,
+						  spi_max_transfer_size(msg->spi));
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
 /*
  * __spi_optimize_message - shared implementation for spi_optimize_message()
  *                          and spi_maybe_optimize_message()
@@ -4254,10 +4274,16 @@  static int __spi_optimize_message(struct spi_device *spi,
 	if (ret)
 		return ret;
 
+	ret = spi_split_transfers(msg);
+	if (ret)
+		return ret;
+
 	if (ctlr->optimize_message) {
 		ret = ctlr->optimize_message(msg);
-		if (ret)
+		if (ret) {
+			spi_res_release(ctlr, msg);
 			return ret;
+		}
 	}
 
 	msg->optimized = true;