[PATCH v1 0/3] spi: CS code, variables and comments clarification

[PATCH v1 0/3] spi: CS code, variables and comments clarification

[Andy Shevchenko]

There are a few duplicated code pieces, inconsitent types and comments
regarding to Chip Select pins. Refactor and clarify that.

Andy Shevchenko (3):
  spi: Exctract spi_set_all_cs_unused() helper
  spi: Exctract spi_dev_check_cs() helper
  spi: Fix multiple issues with Chip Select variables and comments

 drivers/spi/spi.c       | 173 +++++++++++++++++-----------------------
 include/linux/spi/spi.h |  15 ++--
 2 files changed, 82 insertions(+), 106 deletions(-)

-- 
2.43.0.rc1.1.gbec44491f096

[PATCH v1 1/3] spi: Exctract spi_set_all_cs_unused() helper

[Andy Shevchenko]

It seems a few functions implement the similar for-loop to mark all
chip select pins unused. Let's deduplicate that code in order to have
a single place of that for better maintenance.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/spi/spi.c | 74 +++++++++++++++--------------------------------
 1 file changed, 24 insertions(+), 50 deletions(-)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index ba4d3fde2054..5e530fa790b0 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -745,6 +745,23 @@ int spi_add_device(struct spi_device *spi)
 }
 EXPORT_SYMBOL_GPL(spi_add_device);
 
+static void spi_set_all_cs_unused(struct spi_device *spi)
+{
+	u8 idx;
+
+	/*
+	 * Zero(0) is a valid physical CS value and can be located at any
+	 * logical CS in the spi->chip_select[]. If all the physical CS
+	 * are initialized to 0 then It would be difficult to differentiate
+	 * between a valid physical CS 0 & an unused logical CS whose physical
+	 * CS can be 0. As a solution to this issue initialize all the CS to 0xFF.
+	 * Now all the unused logical CS will have 0xFF physical CS value & can be
+	 * ignore while performing physical CS validity checks.
+	 */
+	for (idx = 0; idx < SPI_CS_CNT_MAX; idx++)
+		spi_set_chipselect(spi, idx, 0xFF);
+}
+
 /**
  * spi_new_device - instantiate one new SPI device
  * @ctlr: Controller to which device is connected
@@ -764,7 +781,6 @@ struct spi_device *spi_new_device(struct spi_controller *ctlr,
 {
 	struct spi_device	*proxy;
 	int			status;
-	u8                      idx;
 
 	/*
 	 * NOTE:  caller did any chip->bus_num checks necessary.
@@ -780,19 +796,10 @@ struct spi_device *spi_new_device(struct spi_controller *ctlr,
 
 	WARN_ON(strlen(chip->modalias) >= sizeof(proxy->modalias));
 
-	/*
-	 * Zero(0) is a valid physical CS value and can be located at any
-	 * logical CS in the spi->chip_select[]. If all the physical CS
-	 * are initialized to 0 then It would be difficult to differentiate
-	 * between a valid physical CS 0 & an unused logical CS whose physical
-	 * CS can be 0. As a solution to this issue initialize all the CS to 0xFF.
-	 * Now all the unused logical CS will have 0xFF physical CS value & can be
-	 * ignore while performing physical CS validity checks.
-	 */
-	for (idx = 0; idx < SPI_CS_CNT_MAX; idx++)
-		spi_set_chipselect(proxy, idx, 0xFF);
-
+	/* Use provided chip-select for proxy device */
+	spi_set_all_cs_unused(proxy);
 	spi_set_chipselect(proxy, 0, chip->chip_select);
+
 	proxy->max_speed_hz = chip->max_speed_hz;
 	proxy->mode = chip->mode;
 	proxy->irq = chip->irq;
@@ -2418,17 +2425,7 @@ static int of_spi_parse_dt(struct spi_controller *ctlr, struct spi_device *spi,
 		return -EINVAL;
 	}
 
-	/*
-	 * Zero(0) is a valid physical CS value and can be located at any
-	 * logical CS in the spi->chip_select[]. If all the physical CS
-	 * are initialized to 0 then It would be difficult to differentiate
-	 * between a valid physical CS 0 & an unused logical CS whose physical
-	 * CS can be 0. As a solution to this issue initialize all the CS to 0xFF.
-	 * Now all the unused logical CS will have 0xFF physical CS value & can be
-	 * ignore while performing physical CS validity checks.
-	 */
-	for (idx = 0; idx < SPI_CS_CNT_MAX; idx++)
-		spi_set_chipselect(spi, idx, 0xFF);
+	spi_set_all_cs_unused(spi);
 
 	/* Device address */
 	rc = of_property_read_variable_u32_array(nc, "reg", &cs[0], 1,
@@ -2565,7 +2562,6 @@ struct spi_device *spi_new_ancillary_device(struct spi_device *spi,
 	struct spi_controller *ctlr = spi->controller;
 	struct spi_device *ancillary;
 	int rc = 0;
-	u8 idx;
 
 	/* Alloc an spi_device */
 	ancillary = spi_alloc_device(ctlr);
@@ -2576,19 +2572,8 @@ struct spi_device *spi_new_ancillary_device(struct spi_device *spi,
 
 	strscpy(ancillary->modalias, "dummy", sizeof(ancillary->modalias));
 
-	/*
-	 * Zero(0) is a valid physical CS value and can be located at any
-	 * logical CS in the spi->chip_select[]. If all the physical CS
-	 * are initialized to 0 then It would be difficult to differentiate
-	 * between a valid physical CS 0 & an unused logical CS whose physical
-	 * CS can be 0. As a solution to this issue initialize all the CS to 0xFF.
-	 * Now all the unused logical CS will have 0xFF physical CS value & can be
-	 * ignore while performing physical CS validity checks.
-	 */
-	for (idx = 0; idx < SPI_CS_CNT_MAX; idx++)
-		spi_set_chipselect(ancillary, idx, 0xFF);
-
 	/* Use provided chip-select for ancillary device */
+	spi_set_all_cs_unused(ancillary);
 	spi_set_chipselect(ancillary, 0, chip_select);
 
 	/* Take over SPI mode/speed from SPI main device */
@@ -2805,7 +2790,6 @@ struct spi_device *acpi_spi_device_alloc(struct spi_controller *ctlr,
 	struct acpi_spi_lookup lookup = {};
 	struct spi_device *spi;
 	int ret;
-	u8 idx;
 
 	if (!ctlr && index == -1)
 		return ERR_PTR(-EINVAL);
@@ -2841,24 +2825,14 @@ struct spi_device *acpi_spi_device_alloc(struct spi_controller *ctlr,
 		return ERR_PTR(-ENOMEM);
 	}
 
-	/*
-	 * Zero(0) is a valid physical CS value and can be located at any
-	 * logical CS in the spi->chip_select[]. If all the physical CS
-	 * are initialized to 0 then It would be difficult to differentiate
-	 * between a valid physical CS 0 & an unused logical CS whose physical
-	 * CS can be 0. As a solution to this issue initialize all the CS to 0xFF.
-	 * Now all the unused logical CS will have 0xFF physical CS value & can be
-	 * ignore while performing physical CS validity checks.
-	 */
-	for (idx = 0; idx < SPI_CS_CNT_MAX; idx++)
-		spi_set_chipselect(spi, idx, 0xFF);
+	spi_set_all_cs_unused(spi);
+	spi_set_chipselect(spi, 0, lookup.chip_select);
 
 	ACPI_COMPANION_SET(&spi->dev, adev);
 	spi->max_speed_hz	= lookup.max_speed_hz;
 	spi->mode		|= lookup.mode;
 	spi->irq		= lookup.irq;
 	spi->bits_per_word	= lookup.bits_per_word;
-	spi_set_chipselect(spi, 0, lookup.chip_select);
 	/*
 	 * spi->chip_select[i] gives the corresponding physical CS for logical CS i
 	 * logical CS number is represented by setting the ith bit in spi->cs_index_mask
-- 
2.43.0.rc1.1.gbec44491f096

[PATCH v1 2/3] spi: Exctract spi_dev_check_cs() helper

[Andy Shevchenko]

It seems a few functions implement the similar for-loop to validate
chip select pins for uniqueness. Let's deduplicate that code in order
to have a single place of that for better maintenance.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/spi/spi.c | 47 +++++++++++++++++++++++++++--------------------
 1 file changed, 27 insertions(+), 20 deletions(-)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 5e530fa790b0..eb7e3ddf909b 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -608,23 +608,35 @@ static void spi_dev_set_name(struct spi_device *spi)
 		     spi_get_chipselect(spi, 0));
 }
 
+static inline int spi_dev_check_cs(struct device *dev,
+				   struct spi_device *spi, u8 idx,
+				   struct spi_device *new_spi, u8 new_idx)
+{
+	u8 cs, cs_new;
+	u8 idx_new;
+
+	cs = spi_get_chipselect(spi, idx);
+	for (idx_new = new_idx; idx_new < SPI_CS_CNT_MAX; idx_new++) {
+		cs_new = spi_get_chipselect(new_spi, idx_new);
+		if (cs != 0xFF && cs_new != 0xFF && cs == cs_new) {
+			dev_err(dev, "chipselect %u already in use\n", cs_new);
+			return -EBUSY;
+		}
+	}
+	return 0;
+}
+
 static int spi_dev_check(struct device *dev, void *data)
 {
 	struct spi_device *spi = to_spi_device(dev);
 	struct spi_device *new_spi = data;
-	int idx, nw_idx;
-	u8 cs, cs_nw;
+	int status, idx;
 
 	if (spi->controller == new_spi->controller) {
 		for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) {
-			cs = spi_get_chipselect(spi, idx);
-			for (nw_idx = 0; nw_idx < SPI_CS_CNT_MAX; nw_idx++) {
-				cs_nw = spi_get_chipselect(new_spi, nw_idx);
-				if (cs != 0xFF && cs_nw != 0xFF && cs == cs_nw) {
-					dev_err(dev, "chipselect %d already in use\n", cs_nw);
-					return -EBUSY;
-				}
-			}
+			status = spi_dev_check_cs(dev, spi, idx, new_spi, 0);
+			if (status)
+				return status;
 		}
 	}
 	return 0;
@@ -640,8 +652,8 @@ static int __spi_add_device(struct spi_device *spi)
 {
 	struct spi_controller *ctlr = spi->controller;
 	struct device *dev = ctlr->dev.parent;
-	int status, idx, nw_idx;
-	u8 cs, nw_cs;
+	int status, idx;
+	u8 cs;
 
 	for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) {
 		/* Chipselects are numbered 0..max; validate. */
@@ -658,14 +670,9 @@ static int __spi_add_device(struct spi_device *spi)
 	 * For example, spi->chip_select[0] != spi->chip_select[1] and so on.
 	 */
 	for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) {
-		cs = spi_get_chipselect(spi, idx);
-		for (nw_idx = idx + 1; nw_idx < SPI_CS_CNT_MAX; nw_idx++) {
-			nw_cs = spi_get_chipselect(spi, nw_idx);
-			if (cs != 0xFF && nw_cs != 0xFF && cs == nw_cs) {
-				dev_err(dev, "chipselect %d already in use\n", nw_cs);
-				return -EBUSY;
-			}
-		}
+		status = spi_dev_check_cs(dev, spi, idx, spi, idx + 1);
+		if (status)
+			return status;
 	}
 
 	/* Set the bus ID string */
-- 
2.43.0.rc1.1.gbec44491f096

[PATCH v1 3/3] spi: Fix multiple issues with Chip Select variables and comments

[Andy Shevchenko]

There are the following issues with the current code:
- inconsistent use of 0xFF and -1 for invalid chip select pin
- inconsistent plain or BIT() use
- wrong types used for last_cs_* fields
- wrong multi-line comment style
- misleading or hard-to-understand comments

Fix all of these here.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/spi/spi.c       | 74 +++++++++++++++++++----------------------
 include/linux/spi/spi.h | 15 +++++----
 2 files changed, 42 insertions(+), 47 deletions(-)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index eb7e3ddf909b..f18738ae95f8 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -608,6 +608,22 @@ static void spi_dev_set_name(struct spi_device *spi)
 		     spi_get_chipselect(spi, 0));
 }
 
+/*
+ * Zero(0) is a valid physical CS value and can be located at any
+ * logical CS in the spi->chip_select[]. If all the physical CS
+ * are initialized to 0 then It would be difficult to differentiate
+ * between a valid physical CS 0 & an unused logical CS whose physical
+ * CS can be 0. As a solution to this issue initialize all the CS to -1.
+ * Now all the unused logical CS will have -1 physical CS value & can be
+ * ignored while performing physical CS validity checks.
+ */
+#define SPI_INVALID_CS		((s8)-1)
+
+static inline bool is_valid_cs(s8 chip_select)
+{
+	return chip_select != SPI_INVALID_CS;
+}
+
 static inline int spi_dev_check_cs(struct device *dev,
 				   struct spi_device *spi, u8 idx,
 				   struct spi_device *new_spi, u8 new_idx)
@@ -618,7 +634,7 @@ static inline int spi_dev_check_cs(struct device *dev,
 	cs = spi_get_chipselect(spi, idx);
 	for (idx_new = new_idx; idx_new < SPI_CS_CNT_MAX; idx_new++) {
 		cs_new = spi_get_chipselect(new_spi, idx_new);
-		if (cs != 0xFF && cs_new != 0xFF && cs == cs_new) {
+		if (is_valid_cs(cs) && is_valid_cs(cs_new) && cs == cs_new) {
 			dev_err(dev, "chipselect %u already in use\n", cs_new);
 			return -EBUSY;
 		}
@@ -658,7 +674,7 @@ static int __spi_add_device(struct spi_device *spi)
 	for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) {
 		/* Chipselects are numbered 0..max; validate. */
 		cs = spi_get_chipselect(spi, idx);
-		if (cs != 0xFF && cs >= ctlr->num_chipselect) {
+		if (is_valid_cs(cs) && cs >= ctlr->num_chipselect) {
 			dev_err(dev, "cs%d >= max %d\n", spi_get_chipselect(spi, idx),
 				ctlr->num_chipselect);
 			return -EINVAL;
@@ -698,7 +714,7 @@ static int __spi_add_device(struct spi_device *spi)
 
 		for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) {
 			cs = spi_get_chipselect(spi, idx);
-			if (cs != 0xFF)
+			if (is_valid_cs(cs))
 				spi_set_csgpiod(spi, idx, ctlr->cs_gpiods[cs]);
 		}
 	}
@@ -756,17 +772,8 @@ static void spi_set_all_cs_unused(struct spi_device *spi)
 {
 	u8 idx;
 
-	/*
-	 * Zero(0) is a valid physical CS value and can be located at any
-	 * logical CS in the spi->chip_select[]. If all the physical CS
-	 * are initialized to 0 then It would be difficult to differentiate
-	 * between a valid physical CS 0 & an unused logical CS whose physical
-	 * CS can be 0. As a solution to this issue initialize all the CS to 0xFF.
-	 * Now all the unused logical CS will have 0xFF physical CS value & can be
-	 * ignore while performing physical CS validity checks.
-	 */
 	for (idx = 0; idx < SPI_CS_CNT_MAX; idx++)
-		spi_set_chipselect(spi, idx, 0xFF);
+		spi_set_chipselect(spi, idx, SPI_INVALID_CS);
 }
 
 /**
@@ -1021,7 +1028,7 @@ static inline bool spi_is_last_cs(struct spi_device *spi)
 	bool last = false;
 
 	for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) {
-		if ((spi->cs_index_mask >> idx) & 0x01) {
+		if (spi->cs_index_mask & BIT(idx)) {
 			if (spi->controller->last_cs[idx] == spi_get_chipselect(spi, idx))
 				last = true;
 		}
@@ -1050,7 +1057,7 @@ static void spi_set_cs(struct spi_device *spi, bool enable, bool force)
 
 	spi->controller->last_cs_index_mask = spi->cs_index_mask;
 	for (idx = 0; idx < SPI_CS_CNT_MAX; idx++)
-		spi->controller->last_cs[idx] = enable ? spi_get_chipselect(spi, 0) : -1;
+		spi->controller->last_cs[idx] = enable ? spi_get_chipselect(spi, 0) : SPI_INVALID_CS;
 	spi->controller->last_cs_mode_high = spi->mode & SPI_CS_HIGH;
 
 	if (spi->mode & SPI_CS_HIGH)
@@ -1072,8 +1079,7 @@ static void spi_set_cs(struct spi_device *spi, bool enable, bool force)
 			 * into account.
 			 */
 			for (idx = 0; idx < SPI_CS_CNT_MAX; idx++) {
-				if (((spi->cs_index_mask >> idx) & 0x01) &&
-				    spi_get_csgpiod(spi, idx)) {
+				if ((spi->cs_index_mask & BIT(idx)) && spi_get_csgpiod(spi, idx)) {
 					if (has_acpi_companion(&spi->dev))
 						gpiod_set_value_cansleep(spi_get_csgpiod(spi, idx),
 									 !enable);
@@ -2456,14 +2462,10 @@ static int of_spi_parse_dt(struct spi_controller *ctlr, struct spi_device *spi,
 		spi_set_chipselect(spi, idx, cs[idx]);
 
 	/*
-	 * spi->chip_select[i] gives the corresponding physical CS for logical CS i
-	 * logical CS number is represented by setting the ith bit in spi->cs_index_mask
-	 * So, for example, if spi->cs_index_mask = 0x01 then logical CS number is 0 and
-	 * spi->chip_select[0] will give the physical CS.
-	 * By default spi->chip_select[0] will hold the physical CS number so, set
-	 * spi->cs_index_mask as 0x01.
+	 * By default spi->chip_select[0] will hold the physical CS number,
+	 * so set bit 0 in spi->cs_index_mask.
 	 */
-	spi->cs_index_mask = 0x01;
+	spi->cs_index_mask = BIT(0);
 
 	/* Device speed */
 	if (!of_property_read_u32(nc, "spi-max-frequency", &value))
@@ -2587,14 +2589,10 @@ struct spi_device *spi_new_ancillary_device(struct spi_device *spi,
 	ancillary->max_speed_hz = spi->max_speed_hz;
 	ancillary->mode = spi->mode;
 	/*
-	 * spi->chip_select[i] gives the corresponding physical CS for logical CS i
-	 * logical CS number is represented by setting the ith bit in spi->cs_index_mask
-	 * So, for example, if spi->cs_index_mask = 0x01 then logical CS number is 0 and
-	 * spi->chip_select[0] will give the physical CS.
-	 * By default spi->chip_select[0] will hold the physical CS number so, set
-	 * spi->cs_index_mask as 0x01.
+	 * By default spi->chip_select[0] will hold the physical CS number,
+	 * so set bit 0 in spi->cs_index_mask.
 	 */
-	ancillary->cs_index_mask = 0x01;
+	ancillary->cs_index_mask = BIT(0);
 
 	WARN_ON(!mutex_is_locked(&ctlr->add_lock));
 
@@ -2841,14 +2839,10 @@ struct spi_device *acpi_spi_device_alloc(struct spi_controller *ctlr,
 	spi->irq		= lookup.irq;
 	spi->bits_per_word	= lookup.bits_per_word;
 	/*
-	 * spi->chip_select[i] gives the corresponding physical CS for logical CS i
-	 * logical CS number is represented by setting the ith bit in spi->cs_index_mask
-	 * So, for example, if spi->cs_index_mask = 0x01 then logical CS number is 0 and
-	 * spi->chip_select[0] will give the physical CS.
-	 * By default spi->chip_select[0] will hold the physical CS number so, set
-	 * spi->cs_index_mask as 0x01.
+	 * By default spi->chip_select[0] will hold the physical CS number,
+	 * so set bit 0 in spi->cs_index_mask.
 	 */
-	spi->cs_index_mask	= 0x01;
+	spi->cs_index_mask	= BIT(0);
 
 	return spi;
 }
@@ -3346,9 +3340,9 @@ int spi_register_controller(struct spi_controller *ctlr)
 		goto free_bus_id;
 	}
 
-	/* Setting last_cs to -1 means no chip selected */
+	/* Setting last_cs to SPI_INVALID_CS means no chip selected */
 	for (idx = 0; idx < SPI_CS_CNT_MAX; idx++)
-		ctlr->last_cs[idx] = -1;
+		ctlr->last_cs[idx] = SPI_INVALID_CS;
 
 	status = device_add(&ctlr->dev);
 	if (status < 0)
diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
index e400d454b3f0..81ca62e608c1 100644
--- a/include/linux/spi/spi.h
+++ b/include/linux/spi/spi.h
@@ -226,7 +226,8 @@ struct spi_device {
 	/* The statistics */
 	struct spi_statistics __percpu	*pcpu_statistics;
 
-	/* Bit mask of the chipselect(s) that the driver need to use from
+	/*
+	 * Bit mask of the chipselect(s) that the driver need to use from
 	 * the chipselect array.When the controller is capable to handle
 	 * multiple chip selects & memories are connected in parallel
 	 * then more than one bit need to be set in cs_index_mask.
@@ -448,9 +449,11 @@ extern struct spi_device *spi_new_ancillary_device(struct spi_device *spi, u8 ch
  *	the @cur_msg_completion. This flag is used to signal the context that
  *	is running spi_finalize_current_message() that it needs to complete()
  * @cur_msg_mapped: message has been mapped for DMA
+ * @fallback: fallback to PIO if DMA transfer return failure with
+ *	SPI_TRANS_FAIL_NO_START.
+ * @last_cs_mode_high: was (mode & SPI_CS_HIGH) true on the last call to set_cs.
  * @last_cs: the last chip_select that is recorded by set_cs, -1 on non chip
  *           selected
- * @last_cs_mode_high: was (mode & SPI_CS_HIGH) true on the last call to set_cs.
  * @xfer_completion: used by core transfer_one_message()
  * @busy: message pump is busy
  * @running: message pump is running
@@ -527,8 +530,6 @@ extern struct spi_device *spi_new_ancillary_device(struct spi_device *spi, u8 ch
  *	If the driver does not set this, the SPI core takes the snapshot as
  *	close to the driver hand-over as possible.
  * @irq_flags: Interrupt enable state during PTP system timestamping
- * @fallback: fallback to PIO if DMA transfer return failure with
- *	SPI_TRANS_FAIL_NO_START.
  * @queue_empty: signal green light for opportunistically skipping the queue
  *	for spi_sync transfers.
  * @must_async: disable all fast paths in the core
@@ -708,10 +709,10 @@ struct spi_controller {
 	bool				rt;
 	bool				auto_runtime_pm;
 	bool				cur_msg_mapped;
-	char				last_cs[SPI_CS_CNT_MAX];
-	char				last_cs_index_mask;
-	bool				last_cs_mode_high;
 	bool                            fallback;
+	bool				last_cs_mode_high;
+	s8				last_cs[SPI_CS_CNT_MAX];
+	u32				last_cs_index_mask : SPI_CS_CNT_MAX;
 	struct completion               xfer_completion;
 	size_t				max_dma_len;
 
-- 
2.43.0.rc1.1.gbec44491f096

Re: [PATCH v1 3/3] spi: Fix multiple issues with Chip Select variables and comments

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 518 bytes --]

On Wed, Mar 06, 2024 at 05:59:42PM +0200, Andy Shevchenko wrote:
> There are the following issues with the current code:
> - inconsistent use of 0xFF and -1 for invalid chip select pin
> - inconsistent plain or BIT() use
> - wrong types used for last_cs_* fields
> - wrong multi-line comment style
> - misleading or hard-to-understand comments
> 
> Fix all of these here.

Please don't do this, as covered in submitting-patches.rst submit one
change per patch.  This makes it much easier to review things.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v1 3/3] spi: Fix multiple issues with Chip Select variables and comments

[Andy Shevchenko]

On Wed, Mar 06, 2024 at 06:08:43PM +0000, Mark Brown wrote:
> On Wed, Mar 06, 2024 at 05:59:42PM +0200, Andy Shevchenko wrote:
> > There are the following issues with the current code:
> > - inconsistent use of 0xFF and -1 for invalid chip select pin
> > - inconsistent plain or BIT() use
> > - wrong types used for last_cs_* fields
> > - wrong multi-line comment style
> > - misleading or hard-to-understand comments
> > 
> > Fix all of these here.
> 
> Please don't do this, as covered in submitting-patches.rst submit one
> change per patch.  This makes it much easier to review things.

Fine by me, consider this patch as RFC to understand if we want to have this
or not in general. I will rework it, if the idea is acceptable.

If you are fine on the first two, perhaps they can be applied first.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 3/3] spi: Fix multiple issues with Chip Select variables and comments

[Andy Shevchenko]

On Wed, Mar 06, 2024 at 10:12:06PM +0200, Andy Shevchenko wrote:
> On Wed, Mar 06, 2024 at 06:08:43PM +0000, Mark Brown wrote:
> > On Wed, Mar 06, 2024 at 05:59:42PM +0200, Andy Shevchenko wrote:
> > > There are the following issues with the current code:
> > > - inconsistent use of 0xFF and -1 for invalid chip select pin
> > > - inconsistent plain or BIT() use
> > > - wrong types used for last_cs_* fields
> > > - wrong multi-line comment style
> > > - misleading or hard-to-understand comments
> > > 
> > > Fix all of these here.
> > 
> > Please don't do this, as covered in submitting-patches.rst submit one
> > change per patch.  This makes it much easier to review things.
> 
> Fine by me, consider this patch as RFC to understand if we want to have this
> or not in general. I will rework it, if the idea is acceptable.

I have sent a new series where I split this to three patches (and excluded
the rest for now).


-- 
With Best Regards,
Andy Shevchenko

Re: (subset) [PATCH v1 0/3] spi: CS code, variables and comments clarification

[Mark Brown]

On Wed, 06 Mar 2024 17:59:39 +0200, Andy Shevchenko wrote:
> There are a few duplicated code pieces, inconsitent types and comments
> regarding to Chip Select pins. Refactor and clarify that.
> 
> Andy Shevchenko (3):
>   spi: Exctract spi_set_all_cs_unused() helper
>   spi: Exctract spi_dev_check_cs() helper
>   spi: Fix multiple issues with Chip Select variables and comments
> 
> [...]

Applied to

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

Thanks!

[1/3] spi: Exctract spi_set_all_cs_unused() helper
      commit: 5ee91605ad9ad363766a7ed13dc7d47f5102982a
[2/3] spi: Exctract spi_dev_check_cs() helper
      commit: 9086d0f23b7c292f162a828967975e29e97c0680

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