Re: [PATCH 02/13] spi: spi-fsl-lpspi: Set correct chip-select polarity bit
From: James Clark
Date: Tue Aug 19 2025 - 04:22:56 EST
On 18/08/2025 4:19 pm, Frank Li wrote:
On Mon, Aug 18, 2025 at 02:05:16PM +0100, James Clark wrote:
On 14/08/2025 5:49 pm, Frank Li wrote:
On Thu, Aug 14, 2025 at 05:06:42PM +0100, James Clark wrote:
From: Larisa Grigore <larisa.grigore@xxxxxxx>
The driver currently supports multiple chip-selects, but only sets the
polarity for the first one (CS 0). Fix it by setting the PCSPOL bit for
the desired chip-select.
Fixes: 5314987de5e5 ("spi: imx: add lpspi bus driver")
Signed-off-by: Larisa Grigore <larisa.grigore@xxxxxxx>
Signed-off-by: James Clark <james.clark@xxxxxxxxxx>
---
drivers/spi/spi-fsl-lpspi.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/spi/spi-fsl-lpspi.c b/drivers/spi/spi-fsl-lpspi.c
index d44a23f7d6c1..c65eb6d31ee7 100644
--- a/drivers/spi/spi-fsl-lpspi.c
+++ b/drivers/spi/spi-fsl-lpspi.c
@@ -70,7 +70,7 @@
#define DER_TDDE BIT(0)
#define CFGR1_PCSCFG BIT(27)
#define CFGR1_PINCFG (BIT(24)|BIT(25))
-#define CFGR1_PCSPOL BIT(8)
+#define CFGR1_PCSPOL_MASK GENMASK(11, 8)
#define CFGR1_NOSTALL BIT(3)
#define CFGR1_HOST BIT(0)
#define FSR_TXCOUNT (0xFF)
@@ -425,7 +425,9 @@ static int fsl_lpspi_config(struct fsl_lpspi_data *fsl_lpspi)
else
temp = CFGR1_PINCFG;
if (fsl_lpspi->config.mode & SPI_CS_HIGH)
- temp |= CFGR1_PCSPOL;
+ temp |= FIELD_PREP(CFGR1_PCSPOL_MASK,
+ BIT(fsl_lpspi->config.chip_select));
+
Feel like FILED_PREP(..., BIT()) is stranged.
I suggest #define CFGR1_PCSPOL(x) BIT((x) + 8)
Frank
It's using an existing macro that everyone knows though and I found 65
instances of exactly this. It can be read as "set bit X and put it into the
PCSPOL field without any further investigation.
Where have such pattern in kernel?
Frank
Grep "FIELD_PREP\(.*,\n?.*BIT\("
65 results, e.g:
return FIELD_PREP(RTL8366RB_LED_0_X_CTRL_MASK, BIT(port));
James
If we make a new macro, first the reader will have to jump to it, then it
still doesn't immediately explain what the "+ 8" part is. Using FIELD_PREP()
also has the potential to use autogenerated field masks from a machine
readable version of the reference manual. You can't statically check your
macro to see if + 8 is correct or not, and it also doesn't catch overflow
errors like FIELD_PREP() does.
There might be an argument to add a new global macro like FIELD_BIT(mask,
bit). But it's not very flexible (can't set multiple bits) and you can
already accomplish the same thing by adding BIT() to the existing one.
Thanks
James
writel(temp, fsl_lpspi->base + IMX7ULP_CFGR1);
temp = readl(fsl_lpspi->base + IMX7ULP_CR);
--
2.34.1