diff mbox series

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

Message ID 20240306160114.3471398-4-andriy.shevchenko@linux.intel.com (mailing list archive)
State New
Headers show
Series spi: CS code, variables and comments clarification | expand

Commit Message

Andy Shevchenko March 6, 2024, 3:59 p.m. UTC 
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(-)

Comments

Mark Brown March 6, 2024, 6:08 p.m. UTC | #1 
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.
Andy Shevchenko March 6, 2024, 8:12 p.m. UTC | #2 
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.
Andy Shevchenko March 7, 2024, 3:08 p.m. UTC | #3 
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).
diff mbox series

Patch

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;