[PATCH 0/2] spi: spi-qpic-snand: extend bitmasks usage

[PATCH 0/2] spi: spi-qpic-snand: extend bitmasks usage

[Gabor Juhos]

The two patches in the series are extending the usage of FIELD_PREP()
macro and predefined bitmasks usage in the driver.

Signed-off-by: Gabor Juhos <j4g8y7@gmail.com>
---
Gabor Juhos (2):
      spi: spi-qpic-snand: use CW_PER_PAGE_MASK bitmask
      spi: spi-qpic-snand: extend FIELD_PREP() macro usage

 drivers/spi/spi-qpic-snand.c | 45 ++++++++++++++++++++++----------------------
 1 file changed, 23 insertions(+), 22 deletions(-)
---
base-commit: 4614fd6342ab69feebb067d5db84a9bfb9aada9f
change-id: 20250515-qpic-snand-use-bitmasks-2030c19bbeb9

Best regards,
-- 
Gabor Juhos <j4g8y7@gmail.com>

[PATCH 1/2] spi: spi-qpic-snand: use CW_PER_PAGE_MASK bitmask

[Gabor Juhos]

Change the code to use the already defined CW_PER_PAGE_MASK
bitmask along with the FIELD_PREP() macro instead of using
magic values.

This makes the code more readable. It also syncs the affected
codes with their counterparts in the 'qcom_nandc' driver, so it
makes it easier to spot the differences between the two
implementations.

No functional changes intended.

Signed-off-by: Gabor Juhos <j4g8y7@gmail.com>
---
 drivers/spi/spi-qpic-snand.c | 31 ++++++++++++++++---------------
 1 file changed, 16 insertions(+), 15 deletions(-)

diff --git a/drivers/spi/spi-qpic-snand.c b/drivers/spi/spi-qpic-snand.c
index 7207bbb57802ce53dfab4d9689113e7f9ba8f131..bc45b834fadc5456eda1fe778e5ca8b16177465e 100644
--- a/drivers/spi/spi-qpic-snand.c
+++ b/drivers/spi/spi-qpic-snand.c
@@ -483,7 +483,8 @@ static int qcom_spi_block_erase(struct qcom_nand_controller *snandc)
 	snandc->regs->cmd = snandc->qspi->cmd;
 	snandc->regs->addr0 = snandc->qspi->addr1;
 	snandc->regs->addr1 = snandc->qspi->addr2;
-	snandc->regs->cfg0 = cpu_to_le32(ecc_cfg->cfg0_raw & ~(7 << CW_PER_PAGE));
+	snandc->regs->cfg0 = cpu_to_le32((ecc_cfg->cfg0_raw & ~CW_PER_PAGE_MASK) |
+					 FIELD_PREP(CW_PER_PAGE_MASK, 0));
 	snandc->regs->cfg1 = cpu_to_le32(ecc_cfg->cfg1_raw);
 	snandc->regs->exec = cpu_to_le32(1);
 
@@ -544,8 +545,8 @@ static int qcom_spi_read_last_cw(struct qcom_nand_controller *snandc,
 	snandc->regs->addr0 = (snandc->qspi->addr1 | cpu_to_le32(col));
 	snandc->regs->addr1 = snandc->qspi->addr2;
 
-	cfg0 = (ecc_cfg->cfg0_raw & ~(7U << CW_PER_PAGE)) |
-		0 << CW_PER_PAGE;
+	cfg0 = (ecc_cfg->cfg0_raw & ~CW_PER_PAGE_MASK) |
+	       FIELD_PREP(CW_PER_PAGE_MASK, 0);
 	cfg1 = ecc_cfg->cfg1_raw;
 	ecc_bch_cfg = ECC_CFG_ECC_DISABLE;
 
@@ -687,8 +688,8 @@ static int qcom_spi_read_cw_raw(struct qcom_nand_controller *snandc, u8 *data_bu
 	qcom_clear_bam_transaction(snandc);
 	raw_cw = num_cw - 1;
 
-	cfg0 = (ecc_cfg->cfg0_raw & ~(7U << CW_PER_PAGE)) |
-				0 << CW_PER_PAGE;
+	cfg0 = (ecc_cfg->cfg0_raw & ~CW_PER_PAGE_MASK) |
+	       FIELD_PREP(CW_PER_PAGE_MASK, 0);
 	cfg1 = ecc_cfg->cfg1_raw;
 	ecc_bch_cfg = ECC_CFG_ECC_DISABLE;
 
@@ -808,8 +809,8 @@ static int qcom_spi_read_page_ecc(struct qcom_nand_controller *snandc,
 	snandc->buf_start = 0;
 	qcom_clear_read_regs(snandc);
 
-	cfg0 = (ecc_cfg->cfg0 & ~(7U << CW_PER_PAGE)) |
-				(num_cw - 1) << CW_PER_PAGE;
+	cfg0 = (ecc_cfg->cfg0 & ~CW_PER_PAGE_MASK) |
+	       FIELD_PREP(CW_PER_PAGE_MASK, num_cw - 1);
 	cfg1 = ecc_cfg->cfg1;
 	ecc_bch_cfg = ecc_cfg->ecc_bch_cfg;
 
@@ -904,8 +905,8 @@ static int qcom_spi_read_page_oob(struct qcom_nand_controller *snandc,
 	qcom_clear_read_regs(snandc);
 	qcom_clear_bam_transaction(snandc);
 
-	cfg0 = (ecc_cfg->cfg0 & ~(7U << CW_PER_PAGE)) |
-				(num_cw - 1) << CW_PER_PAGE;
+	cfg0 = (ecc_cfg->cfg0 & ~CW_PER_PAGE_MASK) |
+	       FIELD_PREP(CW_PER_PAGE_MASK, num_cw - 1);
 	cfg1 = ecc_cfg->cfg1;
 	ecc_bch_cfg = ecc_cfg->ecc_bch_cfg;
 
@@ -1015,8 +1016,8 @@ static int qcom_spi_program_raw(struct qcom_nand_controller *snandc,
 	int num_cw = snandc->qspi->num_cw;
 	u32 cfg0, cfg1, ecc_bch_cfg;
 
-	cfg0 = (ecc_cfg->cfg0_raw & ~(7U << CW_PER_PAGE)) |
-			(num_cw - 1) << CW_PER_PAGE;
+	cfg0 = (ecc_cfg->cfg0_raw & ~CW_PER_PAGE_MASK) |
+	       FIELD_PREP(CW_PER_PAGE_MASK, num_cw - 1);
 	cfg1 = ecc_cfg->cfg1_raw;
 	ecc_bch_cfg = ECC_CFG_ECC_DISABLE;
 
@@ -1098,8 +1099,8 @@ static int qcom_spi_program_ecc(struct qcom_nand_controller *snandc,
 	int num_cw = snandc->qspi->num_cw;
 	u32 cfg0, cfg1, ecc_bch_cfg, ecc_buf_cfg;
 
-	cfg0 = (ecc_cfg->cfg0 & ~(7U << CW_PER_PAGE)) |
-				(num_cw - 1) << CW_PER_PAGE;
+	cfg0 = (ecc_cfg->cfg0 & ~CW_PER_PAGE_MASK) |
+	       FIELD_PREP(CW_PER_PAGE_MASK, num_cw - 1);
 	cfg1 = ecc_cfg->cfg1;
 	ecc_bch_cfg = ecc_cfg->ecc_bch_cfg;
 	ecc_buf_cfg = ecc_cfg->ecc_buf_cfg;
@@ -1175,8 +1176,8 @@ static int qcom_spi_program_oob(struct qcom_nand_controller *snandc,
 	int num_cw = snandc->qspi->num_cw;
 	u32 cfg0, cfg1, ecc_bch_cfg, ecc_buf_cfg;
 
-	cfg0 = (ecc_cfg->cfg0 & ~(7U << CW_PER_PAGE)) |
-				(num_cw - 1) << CW_PER_PAGE;
+	cfg0 = (ecc_cfg->cfg0 & ~CW_PER_PAGE_MASK) |
+	       FIELD_PREP(CW_PER_PAGE_MASK, num_cw - 1);
 	cfg1 = ecc_cfg->cfg1;
 	ecc_bch_cfg = ecc_cfg->ecc_bch_cfg;
 	ecc_buf_cfg = ecc_cfg->ecc_buf_cfg;

-- 
2.49.0

[PATCH 2/2] spi: spi-qpic-snand: extend FIELD_PREP() macro usage

[Gabor Juhos]

Large part of the code uses the FIELD_PREP() macro already to construct
values to be written to hardware registers. Change the code to use also
the macro for more registers of which the corresponding bitmasks are
defined already.

This makes the code more readable. It also syncs the affected
codes with their counterparts in the 'qcom_nandc' driver, so it
makes it easier to spot the differences between the two
implementations.

No functional changes intended.

Signed-off-by: Gabor Juhos <j4g8y7@gmail.com>
---
 drivers/spi/spi-qpic-snand.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/spi/spi-qpic-snand.c b/drivers/spi/spi-qpic-snand.c
index bc45b834fadc5456eda1fe778e5ca8b16177465e..ad01bb456a88b54f0ffc801dd14eb3fa2708ec2e 100644
--- a/drivers/spi/spi-qpic-snand.c
+++ b/drivers/spi/spi-qpic-snand.c
@@ -130,9 +130,9 @@ static void qcom_spi_set_read_loc_first(struct qcom_nand_controller *snandc,
 					int is_last_read_loc)
 {
 	__le32 locreg_val;
-	u32 val = (((cw_offset) << READ_LOCATION_OFFSET) |
-		  ((read_size) << READ_LOCATION_SIZE) | ((is_last_read_loc)
-		  << READ_LOCATION_LAST));
+	u32 val = FIELD_PREP(READ_LOCATION_OFFSET_MASK, cw_offset) |
+		  FIELD_PREP(READ_LOCATION_SIZE_MASK, read_size) |
+		  FIELD_PREP(READ_LOCATION_LAST_MASK, is_last_read_loc);
 
 	locreg_val = cpu_to_le32(val);
 
@@ -151,9 +151,9 @@ static void qcom_spi_set_read_loc_last(struct qcom_nand_controller *snandc,
 				       int is_last_read_loc)
 {
 	__le32 locreg_val;
-	u32 val = (((cw_offset) << READ_LOCATION_OFFSET) |
-		  ((read_size) << READ_LOCATION_SIZE) | ((is_last_read_loc)
-		  << READ_LOCATION_LAST));
+	u32 val = FIELD_PREP(READ_LOCATION_OFFSET_MASK, cw_offset) |
+		  FIELD_PREP(READ_LOCATION_SIZE_MASK, read_size) |
+		  FIELD_PREP(READ_LOCATION_LAST_MASK, is_last_read_loc);
 
 	locreg_val = cpu_to_le32(val);
 
@@ -352,7 +352,7 @@ static int qcom_spi_ecc_init_ctx_pipelined(struct nand_device *nand)
 			       FIELD_PREP(ECC_MODE_MASK, 0) |
 			       FIELD_PREP(ECC_PARITY_SIZE_BYTES_BCH_MASK, ecc_cfg->ecc_bytes_hw);
 
-	ecc_cfg->ecc_buf_cfg = 0x203 << NUM_STEPS;
+	ecc_cfg->ecc_buf_cfg = FIELD_PREP(NUM_STEPS_MASK, 0x203);
 	ecc_cfg->clrflashstatus = FS_READY_BSY_N;
 	ecc_cfg->clrreadstatus = 0xc0;
 

-- 
2.49.0

Re: [PATCH 1/2] spi: spi-qpic-snand: use CW_PER_PAGE_MASK bitmask

[Md Sadre Alam]

On 5/16/2025 12:28 AM, Gabor Juhos wrote:
> Change the code to use the already defined CW_PER_PAGE_MASK
> bitmask along with the FIELD_PREP() macro instead of using
> magic values.
> 
> This makes the code more readable. It also syncs the affected
> codes with their counterparts in the 'qcom_nandc' driver, so it
> makes it easier to spot the differences between the two
> implementations.
> 
> No functional changes intended.
> 
> Signed-off-by: Gabor Juhos <j4g8y7@gmail.com>
> ---
>   drivers/spi/spi-qpic-snand.c | 31 ++++++++++++++++---------------
>   1 file changed, 16 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/spi/spi-qpic-snand.c b/drivers/spi/spi-qpic-snand.c
> index 7207bbb57802ce53dfab4d9689113e7f9ba8f131..bc45b834fadc5456eda1fe778e5ca8b16177465e 100644
> --- a/drivers/spi/spi-qpic-snand.c
> +++ b/drivers/spi/spi-qpic-snand.c
> @@ -483,7 +483,8 @@ static int qcom_spi_block_erase(struct qcom_nand_controller *snandc)
>   	snandc->regs->cmd = snandc->qspi->cmd;
>   	snandc->regs->addr0 = snandc->qspi->addr1;
>   	snandc->regs->addr1 = snandc->qspi->addr2;
> -	snandc->regs->cfg0 = cpu_to_le32(ecc_cfg->cfg0_raw & ~(7 << CW_PER_PAGE));
> +	snandc->regs->cfg0 = cpu_to_le32((ecc_cfg->cfg0_raw & ~CW_PER_PAGE_MASK) |
> +					 FIELD_PREP(CW_PER_PAGE_MASK, 0));
>   	snandc->regs->cfg1 = cpu_to_le32(ecc_cfg->cfg1_raw);
>   	snandc->regs->exec = cpu_to_le32(1);
>   
> @@ -544,8 +545,8 @@ static int qcom_spi_read_last_cw(struct qcom_nand_controller *snandc,
>   	snandc->regs->addr0 = (snandc->qspi->addr1 | cpu_to_le32(col));
>   	snandc->regs->addr1 = snandc->qspi->addr2;
>   
> -	cfg0 = (ecc_cfg->cfg0_raw & ~(7U << CW_PER_PAGE)) |
> -		0 << CW_PER_PAGE;
> +	cfg0 = (ecc_cfg->cfg0_raw & ~CW_PER_PAGE_MASK) |
> +	       FIELD_PREP(CW_PER_PAGE_MASK, 0);
>   	cfg1 = ecc_cfg->cfg1_raw;
>   	ecc_bch_cfg = ECC_CFG_ECC_DISABLE;
>   
> @@ -687,8 +688,8 @@ static int qcom_spi_read_cw_raw(struct qcom_nand_controller *snandc, u8 *data_bu
>   	qcom_clear_bam_transaction(snandc);
>   	raw_cw = num_cw - 1;
>   
> -	cfg0 = (ecc_cfg->cfg0_raw & ~(7U << CW_PER_PAGE)) |
> -				0 << CW_PER_PAGE;
> +	cfg0 = (ecc_cfg->cfg0_raw & ~CW_PER_PAGE_MASK) |
> +	       FIELD_PREP(CW_PER_PAGE_MASK, 0);
>   	cfg1 = ecc_cfg->cfg1_raw;
>   	ecc_bch_cfg = ECC_CFG_ECC_DISABLE;
>   
> @@ -808,8 +809,8 @@ static int qcom_spi_read_page_ecc(struct qcom_nand_controller *snandc,
>   	snandc->buf_start = 0;
>   	qcom_clear_read_regs(snandc);
>   
> -	cfg0 = (ecc_cfg->cfg0 & ~(7U << CW_PER_PAGE)) |
> -				(num_cw - 1) << CW_PER_PAGE;
> +	cfg0 = (ecc_cfg->cfg0 & ~CW_PER_PAGE_MASK) |
> +	       FIELD_PREP(CW_PER_PAGE_MASK, num_cw - 1);
>   	cfg1 = ecc_cfg->cfg1;
>   	ecc_bch_cfg = ecc_cfg->ecc_bch_cfg;
>   
> @@ -904,8 +905,8 @@ static int qcom_spi_read_page_oob(struct qcom_nand_controller *snandc,
>   	qcom_clear_read_regs(snandc);
>   	qcom_clear_bam_transaction(snandc);
>   
> -	cfg0 = (ecc_cfg->cfg0 & ~(7U << CW_PER_PAGE)) |
> -				(num_cw - 1) << CW_PER_PAGE;
> +	cfg0 = (ecc_cfg->cfg0 & ~CW_PER_PAGE_MASK) |
> +	       FIELD_PREP(CW_PER_PAGE_MASK, num_cw - 1);
>   	cfg1 = ecc_cfg->cfg1;
>   	ecc_bch_cfg = ecc_cfg->ecc_bch_cfg;
>   
> @@ -1015,8 +1016,8 @@ static int qcom_spi_program_raw(struct qcom_nand_controller *snandc,
>   	int num_cw = snandc->qspi->num_cw;
>   	u32 cfg0, cfg1, ecc_bch_cfg;
>   
> -	cfg0 = (ecc_cfg->cfg0_raw & ~(7U << CW_PER_PAGE)) |
> -			(num_cw - 1) << CW_PER_PAGE;
> +	cfg0 = (ecc_cfg->cfg0_raw & ~CW_PER_PAGE_MASK) |
> +	       FIELD_PREP(CW_PER_PAGE_MASK, num_cw - 1);
>   	cfg1 = ecc_cfg->cfg1_raw;
>   	ecc_bch_cfg = ECC_CFG_ECC_DISABLE;
>   
> @@ -1098,8 +1099,8 @@ static int qcom_spi_program_ecc(struct qcom_nand_controller *snandc,
>   	int num_cw = snandc->qspi->num_cw;
>   	u32 cfg0, cfg1, ecc_bch_cfg, ecc_buf_cfg;
>   
> -	cfg0 = (ecc_cfg->cfg0 & ~(7U << CW_PER_PAGE)) |
> -				(num_cw - 1) << CW_PER_PAGE;
> +	cfg0 = (ecc_cfg->cfg0 & ~CW_PER_PAGE_MASK) |
> +	       FIELD_PREP(CW_PER_PAGE_MASK, num_cw - 1);
>   	cfg1 = ecc_cfg->cfg1;
>   	ecc_bch_cfg = ecc_cfg->ecc_bch_cfg;
>   	ecc_buf_cfg = ecc_cfg->ecc_buf_cfg;
> @@ -1175,8 +1176,8 @@ static int qcom_spi_program_oob(struct qcom_nand_controller *snandc,
>   	int num_cw = snandc->qspi->num_cw;
>   	u32 cfg0, cfg1, ecc_bch_cfg, ecc_buf_cfg;
>   
> -	cfg0 = (ecc_cfg->cfg0 & ~(7U << CW_PER_PAGE)) |
> -				(num_cw - 1) << CW_PER_PAGE;
> +	cfg0 = (ecc_cfg->cfg0 & ~CW_PER_PAGE_MASK) |
> +	       FIELD_PREP(CW_PER_PAGE_MASK, num_cw - 1);
>   	cfg1 = ecc_cfg->cfg1;
>   	ecc_bch_cfg = ecc_cfg->ecc_bch_cfg;
>   	ecc_buf_cfg = ecc_cfg->ecc_buf_cfg;
>

Reviewed-by: Md Sadre Alam <quic_mdalam@quicinc.com>

Re: [PATCH 2/2] spi: spi-qpic-snand: extend FIELD_PREP() macro usage

[Md Sadre Alam]

On 5/16/2025 12:28 AM, Gabor Juhos wrote:
> Large part of the code uses the FIELD_PREP() macro already to construct
> values to be written to hardware registers. Change the code to use also
> the macro for more registers of which the corresponding bitmasks are
> defined already.
> 
> This makes the code more readable. It also syncs the affected
> codes with their counterparts in the 'qcom_nandc' driver, so it
> makes it easier to spot the differences between the two
> implementations.
> 
> No functional changes intended.
> 
> Signed-off-by: Gabor Juhos <j4g8y7@gmail.com>
> ---
>   drivers/spi/spi-qpic-snand.c | 14 +++++++-------
>   1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/spi/spi-qpic-snand.c b/drivers/spi/spi-qpic-snand.c
> index bc45b834fadc5456eda1fe778e5ca8b16177465e..ad01bb456a88b54f0ffc801dd14eb3fa2708ec2e 100644
> --- a/drivers/spi/spi-qpic-snand.c
> +++ b/drivers/spi/spi-qpic-snand.c
> @@ -130,9 +130,9 @@ static void qcom_spi_set_read_loc_first(struct qcom_nand_controller *snandc,
>   					int is_last_read_loc)
>   {
>   	__le32 locreg_val;
> -	u32 val = (((cw_offset) << READ_LOCATION_OFFSET) |
> -		  ((read_size) << READ_LOCATION_SIZE) | ((is_last_read_loc)
> -		  << READ_LOCATION_LAST));
> +	u32 val = FIELD_PREP(READ_LOCATION_OFFSET_MASK, cw_offset) |
> +		  FIELD_PREP(READ_LOCATION_SIZE_MASK, read_size) |
> +		  FIELD_PREP(READ_LOCATION_LAST_MASK, is_last_read_loc);
>   
>   	locreg_val = cpu_to_le32(val);
>   
> @@ -151,9 +151,9 @@ static void qcom_spi_set_read_loc_last(struct qcom_nand_controller *snandc,
>   				       int is_last_read_loc)
>   {
>   	__le32 locreg_val;
> -	u32 val = (((cw_offset) << READ_LOCATION_OFFSET) |
> -		  ((read_size) << READ_LOCATION_SIZE) | ((is_last_read_loc)
> -		  << READ_LOCATION_LAST));
> +	u32 val = FIELD_PREP(READ_LOCATION_OFFSET_MASK, cw_offset) |
> +		  FIELD_PREP(READ_LOCATION_SIZE_MASK, read_size) |
> +		  FIELD_PREP(READ_LOCATION_LAST_MASK, is_last_read_loc);
>   
>   	locreg_val = cpu_to_le32(val);
>   
> @@ -352,7 +352,7 @@ static int qcom_spi_ecc_init_ctx_pipelined(struct nand_device *nand)
>   			       FIELD_PREP(ECC_MODE_MASK, 0) |
>   			       FIELD_PREP(ECC_PARITY_SIZE_BYTES_BCH_MASK, ecc_cfg->ecc_bytes_hw);
>   
> -	ecc_cfg->ecc_buf_cfg = 0x203 << NUM_STEPS;
> +	ecc_cfg->ecc_buf_cfg = FIELD_PREP(NUM_STEPS_MASK, 0x203);
>   	ecc_cfg->clrflashstatus = FS_READY_BSY_N;
>   	ecc_cfg->clrreadstatus = 0xc0;
>   
> 

Reviewed-by: Md Sadre Alam <quic_mdalam@quicinc.com>