[PATCH v2 1/3] spi: Deduplicate spi_match_id() in __spi_register_driver()

[PATCH v2 1/3] spi: Deduplicate spi_match_id() in __spi_register_driver()

[Andy Shevchenko]

The same logic is used in spi_match_id() and in the __spi_register_driver().
By switching the former from taking struct spi_device * to const char * as
the second parameter we may deduplicate the code.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
v2: no changes
 drivers/spi/spi.c | 17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index fdd530b150a7..9d19d9bae253 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -315,11 +315,10 @@ static void spi_statistics_add_transfer_stats(struct spi_statistics *stats,
  * and the sysfs version makes coldplug work too.
  */
 
-static const struct spi_device_id *spi_match_id(const struct spi_device_id *id,
-						const struct spi_device *sdev)
+static const struct spi_device_id *spi_match_id(const struct spi_device_id *id, const char *name)
 {
 	while (id->name[0]) {
-		if (!strcmp(sdev->modalias, id->name))
+		if (!strcmp(name, id->name))
 			return id;
 		id++;
 	}
@@ -330,7 +329,7 @@ const struct spi_device_id *spi_get_device_id(const struct spi_device *sdev)
 {
 	const struct spi_driver *sdrv = to_spi_driver(sdev->dev.driver);
 
-	return spi_match_id(sdrv->id_table, sdev);
+	return spi_match_id(sdrv->id_table, sdev->modalias);
 }
 EXPORT_SYMBOL_GPL(spi_get_device_id);
 
@@ -352,7 +351,7 @@ static int spi_match_device(struct device *dev, struct device_driver *drv)
 		return 1;
 
 	if (sdrv->id_table)
-		return !!spi_match_id(sdrv->id_table, spi);
+		return !!spi_match_id(sdrv->id_table, spi->modalias);
 
 	return strcmp(spi->modalias, drv->name) == 0;
 }
@@ -474,12 +473,8 @@ int __spi_register_driver(struct module *owner, struct spi_driver *sdrv)
 			if (sdrv->id_table) {
 				const struct spi_device_id *spi_id;
 
-				for (spi_id = sdrv->id_table; spi_id->name[0];
-				     spi_id++)
-					if (strcmp(spi_id->name, of_name) == 0)
-						break;
-
-				if (spi_id->name[0])
+				spi_id = spi_match_id(sdrv->id_table, of_name);
+				if (!spi_id)
 					continue;
 			} else {
 				if (strcmp(sdrv->driver.name, of_name) == 0)
-- 
2.33.0

[PATCH v2 2/3] spi: Replace memset() with __GFP_ZERO

[Andy Shevchenko]

krealloc() as any other kernel memory allocation calls accepts GFP flags,
one of which is __GFP_ZERO. Hence, no need to call memset() explicitly on
the reallocated buffer.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
v2: new patch
 drivers/spi/spi.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 9d19d9bae253..3b9010a67397 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -1222,11 +1222,10 @@ static int spi_map_msg(struct spi_controller *ctlr, struct spi_message *msg)
 
 		if (max_tx) {
 			tmp = krealloc(ctlr->dummy_tx, max_tx,
-				       GFP_KERNEL | GFP_DMA);
+				       GFP_KERNEL | GFP_DMA | __GFP_ZERO);
 			if (!tmp)
 				return -ENOMEM;
 			ctlr->dummy_tx = tmp;
-			memset(tmp, 0, max_tx);
 		}
 
 		if (max_rx) {
-- 
2.33.0

[PATCH v2 3/3] spi: Fix multi-line comment style

[Andy Shevchenko]

  /*
   * Fix multi-line comment style as in this short example. Pay attention
   * to the capitalization, period and starting line of the text.
   */

While at it, split the (supposedly short) description of couple of functions
to summary (short description) and (long) description.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
v2: rebased on top of spi/for-next (Mark)
 drivers/spi/spi.c | 160 ++++++++++++++++++++++++++--------------------
 1 file changed, 89 insertions(+), 71 deletions(-)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 3b9010a67397..5bf680fcb170 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -311,10 +311,10 @@ static void spi_statistics_add_transfer_stats(struct spi_statistics *stats,
 	spin_unlock_irqrestore(&stats->lock, flags);
 }
 
-/* modalias support makes "modprobe $MODALIAS" new-style hotplug work,
+/*
+ * modalias support makes "modprobe $MODALIAS" new-style hotplug work,
  * and the sysfs version makes coldplug work too.
  */
-
 static const struct spi_device_id *spi_match_id(const struct spi_device_id *id, const char *name)
 {
 	while (id->name[0]) {
@@ -492,7 +492,8 @@ EXPORT_SYMBOL_GPL(__spi_register_driver);
 
 /*-------------------------------------------------------------------------*/
 
-/* SPI devices should normally not be created by SPI device drivers; that
+/*
+ * SPI devices should normally not be created by SPI device drivers; that
  * would make them board-specific.  Similarly with SPI controller drivers.
  * Device registration normally goes into like arch/.../mach.../board-YYY.c
  * with other readonly (flashable) information about mainboard devices.
@@ -508,8 +509,8 @@ static LIST_HEAD(spi_controller_list);
 
 /*
  * Used to protect add/del operation for board_info list and
- * spi_controller list, and their matching process
- * also used to protect object of type struct idr
+ * spi_controller list, and their matching process also used
+ * to protect object of type struct idr.
  */
 static DEFINE_MUTEX(board_lock);
 
@@ -616,7 +617,8 @@ static int __spi_add_device(struct spi_device *spi)
 	else if (ctlr->cs_gpios)
 		spi->cs_gpio = ctlr->cs_gpios[spi->chip_select];
 
-	/* Drivers may modify this initial i/o setup, but will
+	/*
+	 * Drivers may modify this initial i/o setup, but will
 	 * normally rely on the device being setup.  Devices
 	 * using SPI_CS_HIGH can't coexist well otherwise...
 	 */
@@ -710,7 +712,8 @@ struct spi_device *spi_new_device(struct spi_controller *ctlr,
 	struct spi_device	*proxy;
 	int			status;
 
-	/* NOTE:  caller did any chip->bus_num checks necessary.
+	/*
+	 * NOTE:  caller did any chip->bus_num checks necessary.
 	 *
 	 * Also, unless we change the return value convention to use
 	 * error-or-pointer (not NULL-or-pointer), troubleshootability
@@ -878,7 +881,6 @@ static void *spi_res_alloc(struct spi_device *spi, spi_res_release_t release,
 /**
  * spi_res_free - free an spi resource
  * @res: pointer to the custom data of a resource
- *
  */
 static void spi_res_free(void *res)
 {
@@ -973,7 +975,7 @@ static void spi_set_cs(struct spi_device *spi, bool enable, bool force)
 					gpiod_set_value_cansleep(spi->cs_gpiod, activate);
 			} else {
 				/*
-				 * invert the enable line, as active low is
+				 * Invert the enable line, as active low is
 				 * default for SPI.
 				 */
 				gpio_set_value_cansleep(spi->cs_gpio, !enable);
@@ -1711,16 +1713,7 @@ static void spi_pump_messages(struct kthread_work *work)
 }
 
 /**
- * spi_take_timestamp_pre - helper for drivers to collect the beginning of the
- *			    TX timestamp for the requested byte from the SPI
- *			    transfer. The frequency with which this function
- *			    must be called (once per word, once for the whole
- *			    transfer, once per batch of words etc) is arbitrary
- *			    as long as the @tx buffer offset is greater than or
- *			    equal to the requested byte at the time of the
- *			    call. The timestamp is only taken once, at the
- *			    first such call. It is assumed that the driver
- *			    advances its @tx buffer pointer monotonically.
+ * spi_take_timestamp_pre - helper to collect the beginning of the TX timestamp
  * @ctlr: Pointer to the spi_controller structure of the driver
  * @xfer: Pointer to the transfer being timestamped
  * @progress: How many words (not bytes) have been transferred so far
@@ -1730,6 +1723,14 @@ static void spi_pump_messages(struct kthread_work *work)
  *	      spi_take_timestamp_post or otherwise system will crash.
  *	      WARNING: for fully predictable results, the CPU frequency must
  *	      also be under control (governor).
+ *
+ * This is a helper for drivers to collect the beginning of the TX timestamp
+ * for the requested byte from the SPI transfer. The frequency with which this
+ * function must be called (once per word, once for the whole transfer, once
+ * per batch of words etc) is arbitrary as long as the @tx buffer offset is
+ * greater than or equal to the requested byte at the time of the call. The
+ * timestamp is only taken once, at the first such call. It is assumed that
+ * the driver advances its @tx buffer pointer monotonically.
  */
 void spi_take_timestamp_pre(struct spi_controller *ctlr,
 			    struct spi_transfer *xfer,
@@ -1757,16 +1758,16 @@ void spi_take_timestamp_pre(struct spi_controller *ctlr,
 EXPORT_SYMBOL_GPL(spi_take_timestamp_pre);
 
 /**
- * spi_take_timestamp_post - helper for drivers to collect the end of the
- *			     TX timestamp for the requested byte from the SPI
- *			     transfer. Can be called with an arbitrary
- *			     frequency: only the first call where @tx exceeds
- *			     or is equal to the requested word will be
- *			     timestamped.
+ * spi_take_timestamp_post - helper to collect the end of the TX timestamp
  * @ctlr: Pointer to the spi_controller structure of the driver
  * @xfer: Pointer to the transfer being timestamped
  * @progress: How many words (not bytes) have been transferred so far
  * @irqs_off: If true, will re-enable IRQs and preemption for the local CPU.
+ *
+ * This is a helper for drivers to collect the end of the TX timestamp for
+ * the requested byte from the SPI transfer. Can be called with an arbitrary
+ * frequency: only the first call where @tx exceeds or is equal to the
+ * requested word will be timestamped.
  */
 void spi_take_timestamp_post(struct spi_controller *ctlr,
 			     struct spi_transfer *xfer,
@@ -1899,10 +1900,12 @@ 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 splited transfers here since spi_map_msg is done on the
-	 * splited transfers.
+	/*
+	 * 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);
 
@@ -2944,8 +2947,9 @@ int spi_register_controller(struct spi_controller *ctlr)
 	if (!ctlr->max_dma_len)
 		ctlr->max_dma_len = INT_MAX;
 
-	/* register the device, then userspace will see it.
-	 * registration fails if the bus ID is in use.
+	/*
+	 * Register the device, then userspace will see it.
+	 * Registration fails if the bus ID is in use.
 	 */
 	dev_set_name(&ctlr->dev, "spi%u", ctlr->bus_num);
 
@@ -3211,16 +3215,18 @@ static struct spi_replaced_transfers *spi_replace_transfers(
 	/* init the replaced_transfers list */
 	INIT_LIST_HEAD(&rxfer->replaced_transfers);
 
-	/* assign the list_entry after which we should reinsert
+	/*
+	 * Assign the list_entry after which we should reinsert
 	 * the @replaced_transfers - it may be spi_message.messages!
 	 */
 	rxfer->replaced_after = xfer_first->transfer_list.prev;
 
 	/* remove the requested number of transfers */
 	for (i = 0; i < remove; i++) {
-		/* if the entry after replaced_after it is msg->transfers
+		/*
+		 * If the entry after replaced_after it is msg->transfers
 		 * then we have been requested to remove more transfers
-		 * than are in the list
+		 * than are in the list.
 		 */
 		if (rxfer->replaced_after->next == &msg->transfers) {
 			dev_err(&msg->spi->dev,
@@ -3236,15 +3242,17 @@ static struct spi_replaced_transfers *spi_replace_transfers(
 			return ERR_PTR(-EINVAL);
 		}
 
-		/* remove the entry after replaced_after from list of
-		 * transfers and add it to list of replaced_transfers
+		/*
+		 * Remove the entry after replaced_after from list of
+		 * transfers and add it to list of replaced_transfers.
 		 */
 		list_move_tail(rxfer->replaced_after->next,
 			       &rxfer->replaced_transfers);
 	}
 
-	/* create copy of the given xfer with identical settings
-	 * based on the first transfer to get removed
+	/*
+	 * Create copy of the given xfer with identical settings
+	 * based on the first transfer to get removed.
 	 */
 	for (i = 0; i < insert; i++) {
 		/* we need to run in reverse order */
@@ -3292,18 +3300,20 @@ static int __spi_split_transfer_maxsize(struct spi_controller *ctlr,
 		return PTR_ERR(srt);
 	xfers = srt->inserted_transfers;
 
-	/* now handle each of those newly inserted spi_transfers
-	 * note that the replacements spi_transfers all are preset
+	/*
+	 * Now handle each of those newly inserted spi_transfers.
+	 * Note that the replacements spi_transfers all are preset
 	 * to the same values as *xferp, so tx_buf, rx_buf and len
 	 * are all identical (as well as most others)
 	 * so we just have to fix up len and the pointers.
 	 *
-	 * this also includes support for the depreciated
-	 * spi_message.is_dma_mapped interface
+	 * This also includes support for the depreciated
+	 * spi_message.is_dma_mapped interface.
 	 */
 
-	/* the first transfer just needs the length modified, so we
-	 * run it outside the loop
+	/*
+	 * The first transfer just needs the length modified, so we
+	 * run it outside the loop.
 	 */
 	xfers[0].len = min_t(size_t, maxsize, xfer[0].len);
 
@@ -3323,8 +3333,9 @@ static int __spi_split_transfer_maxsize(struct spi_controller *ctlr,
 		xfers[i].len = min(maxsize, xfers[i].len - offset);
 	}
 
-	/* we set up xferp to the last entry we have inserted,
-	 * so that we skip those already split transfers
+	/*
+	 * We set up xferp to the last entry we have inserted,
+	 * so that we skip those already split transfers.
 	 */
 	*xferp = &xfers[count - 1];
 
@@ -3356,11 +3367,12 @@ int spi_split_transfers_maxsize(struct spi_controller *ctlr,
 	struct spi_transfer *xfer;
 	int ret;
 
-	/* iterate over the transfer_list,
+	/*
+	 * Iterate over the transfer_list,
 	 * but note that xfer is advanced to the last transfer inserted
 	 * to avoid checking sizes again unnecessarily (also xfer does
-	 * potentiall belong to a different list by the time the
-	 * replacement has happened
+	 * potentially belong to a different list by the time the
+	 * replacement has happened).
 	 */
 	list_for_each_entry(xfer, &msg->transfers, transfer_list) {
 		if (xfer->len > maxsize) {
@@ -3421,8 +3433,8 @@ int spi_setup(struct spi_device *spi)
 	int		status;
 
 	/*
-	 * check mode to prevent that any two of DUAL, QUAD and NO_MOSI/MISO
-	 * are set at the same time
+	 * Check mode to prevent that any two of DUAL, QUAD and NO_MOSI/MISO
+	 * are set at the same time.
 	 */
 	if ((hweight_long(spi->mode &
 		(SPI_TX_DUAL | SPI_TX_QUAD | SPI_NO_TX)) > 1) ||
@@ -3432,20 +3444,21 @@ int spi_setup(struct spi_device *spi)
 		"setup: can not select any two of dual, quad and no-rx/tx at the same time\n");
 		return -EINVAL;
 	}
-	/* if it is SPI_3WIRE mode, DUAL and QUAD should be forbidden
-	 */
+	/* If it is SPI_3WIRE mode, DUAL and QUAD should be forbidden */
 	if ((spi->mode & SPI_3WIRE) && (spi->mode &
 		(SPI_TX_DUAL | SPI_TX_QUAD | SPI_TX_OCTAL |
 		 SPI_RX_DUAL | SPI_RX_QUAD | SPI_RX_OCTAL)))
 		return -EINVAL;
-	/* help drivers fail *cleanly* when they need options
-	 * that aren't supported with their current controller
+	/*
+	 * Help drivers fail *cleanly* when they need options
+	 * that aren't supported with their current controller.
 	 * SPI_CS_WORD has a fallback software implementation,
 	 * so it is ignored here.
 	 */
 	bad_bits = spi->mode & ~(spi->controller->mode_bits | SPI_CS_WORD |
 				 SPI_NO_TX | SPI_NO_RX);
-	/* nothing prevents from working with active-high CS in case if it
+	/*
+	 * Nothing prevents from working with active-high CS in case if it
 	 * is driven by GPIO.
 	 */
 	if (gpio_is_valid(spi->cs_gpio))
@@ -3567,7 +3580,8 @@ static int __spi_validate(struct spi_device *spi, struct spi_message *message)
 	if (list_empty(&message->transfers))
 		return -EINVAL;
 
-	/* If an SPI controller does not support toggling the CS line on each
+	/*
+	 * 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
@@ -3597,7 +3611,8 @@ static int __spi_validate(struct spi_device *spi, struct spi_message *message)
 		}
 	}
 
-	/* Half-duplex links include original MicroWire, and ones with
+	/*
+	 * Half-duplex links include original MicroWire, and ones with
 	 * only one data pin like SPI_3WIRE (switches direction) or where
 	 * either MOSI or MISO is missing.  They can also be caused by
 	 * software limitations.
@@ -3616,7 +3631,7 @@ static int __spi_validate(struct spi_device *spi, struct spi_message *message)
 		}
 	}
 
-	/**
+	/*
 	 * Set transfer bits_per_word and max speed as spi device default if
 	 * it is not set for this transfer.
 	 * Set transfer tx_nbits and rx_nbits as single transfer default
@@ -3642,7 +3657,7 @@ static int __spi_validate(struct spi_device *spi, struct spi_message *message)
 
 		/*
 		 * SPI transfer length should be multiple of SPI word size
-		 * where SPI word size should be power-of-two multiple
+		 * where SPI word size should be power-of-two multiple.
 		 */
 		if (xfer->bits_per_word <= 8)
 			w_size = 1;
@@ -3663,7 +3678,8 @@ static int __spi_validate(struct spi_device *spi, struct spi_message *message)
 			xfer->tx_nbits = SPI_NBITS_SINGLE;
 		if (xfer->rx_buf && !xfer->rx_nbits)
 			xfer->rx_nbits = SPI_NBITS_SINGLE;
-		/* check transfer tx/rx_nbits:
+		/*
+		 * Check transfer tx/rx_nbits:
 		 * 1. check the value matches one of single, dual and quad
 		 * 2. check tx/rx_nbits match the mode in spi_device
 		 */
@@ -3842,7 +3858,8 @@ static int spi_async_locked(struct spi_device *spi, struct spi_message *message)
 
 /*-------------------------------------------------------------------------*/
 
-/* Utility methods for SPI protocol drivers, layered on
+/*
+ * Utility methods for SPI protocol drivers, layered on
  * top of the core.  Some other utility methods are defined as
  * inline functions.
  */
@@ -3870,7 +3887,8 @@ static int __spi_sync(struct spi_device *spi, struct spi_message *message)
 	SPI_STATISTICS_INCREMENT_FIELD(&ctlr->statistics, spi_sync);
 	SPI_STATISTICS_INCREMENT_FIELD(&spi->statistics, spi_sync);
 
-	/* If we're not using the legacy transfer method then we will
+	/*
+	 * If we're not using the legacy transfer method then we will
 	 * try to transfer in the calling context so special case.
 	 * This code would be less tricky if we could remove the
 	 * support for driver implemented message queues.
@@ -3888,9 +3906,7 @@ static int __spi_sync(struct spi_device *spi, struct spi_message *message)
 	}
 
 	if (status == 0) {
-		/* Push out the messages in the calling context if we
-		 * can.
-		 */
+		/* Push out the messages in the calling context if we can */
 		if (ctlr->transfer == spi_queued_transfer) {
 			SPI_STATISTICS_INCREMENT_FIELD(&ctlr->statistics,
 						       spi_sync_immediate);
@@ -4051,7 +4067,8 @@ int spi_write_then_read(struct spi_device *spi,
 	struct spi_transfer	x[2];
 	u8			*local_buf;
 
-	/* Use preallocated DMA-safe buffer if we can.  We can't avoid
+	/*
+	 * Use preallocated DMA-safe buffer if we can. We can't avoid
 	 * copying here, (as a pure convenience thing), but we can
 	 * keep heap costs out of the hot path unless someone else is
 	 * using the pre-allocated buffer or the transfer is too large.
@@ -4287,11 +4304,12 @@ static int __init spi_init(void)
 	return status;
 }
 
-/* board_info is normally registered in arch_initcall(),
- * but even essential drivers wait till later
+/*
+ * A board_info is normally registered in arch_initcall(),
+ * but even essential drivers wait till later.
  *
- * REVISIT only boardinfo really needs static linking. the rest (device and
- * driver registration) _could_ be dynamically linked (modular) ... costs
+ * REVISIT only boardinfo really needs static linking. The rest (device and
+ * driver registration) _could_ be dynamically linked (modular) ... Costs
  * include needing to have boardinfo data structures be much more public.
  */
 postcore_initcall(spi_init);
-- 
2.33.0

Re: [PATCH v2 1/3] spi: Deduplicate spi_match_id() in __spi_register_driver()

[Mark Brown]

On Mon, 22 Nov 2021 19:17:19 +0200, Andy Shevchenko wrote:
> The same logic is used in spi_match_id() and in the __spi_register_driver().
> By switching the former from taking struct spi_device * to const char * as
> the second parameter we may deduplicate the code.
> 
> 

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next

Thanks!

[1/3] spi: Deduplicate spi_match_id() in __spi_register_driver()
      (no commit info)
[2/3] spi: Replace memset() with __GFP_ZERO
      commit: b00bab9d48bbb6446a5cf366f5f8e501a16031a1
[3/3] spi: Fix multi-line comment style
      commit: 350de7ce26caba5c7ec0dd4ef1802c9a50a5d85d

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark