[PATCH 00/21] lib: add alternatives for GENMASK()

[PATCH 00/21] lib: add alternatives for GENMASK()

[Yury Norov (NVIDIA)]

GENMASK(high, low) notation reflects a common pattern to describe
hardware registers. However, out of drivers context it's confusing and
error-prone. 

This series introduces alternatives for GENMASK():

 - BITS(low, high);
 - FIRST_BITS(nbits);
 - LAST_BITS(start);

Patches 1-6 and 8 rename existing local BITS(). Patches 7 and 9 introduce
new generic macros. Patches 10-18 switch GENMASK() usage in core files to
better alternatives. Patch 19 adds Documentation section describing
preferred parameters ordering.

The series is inspired by:

https://lore.kernel.org/all/CAHk-=whoOUsqPKb7OQwhQf9H_3=5sXGPJrDbfQfwLB3Bi13tcQ@mail.gmail.com/

Yury Norov (NVIDIA) (21):
  arc: disasm: rename BITS() for FIELD()
  iwlwifi: drop unused BITS()
  select: rename BITS() to FDS_BITS()
  ALSA: rename BITS to R_BITS
  zlib: BITS() to LOWBITS()
  mfd: prepare to generalize BITS() macro
  bits: Add BITS() macro
  mfd: drop local BITS() macro
  bits: generalize BITMAP_{FIRST,LAST}_WORD_MASK
  i2c: nomadik: don't use GENMASK()
  drivers: don't use GENMASK() in FIELD_PREP_WM16()
  bitmap: don't use GENMASK()
  trace: don't use GENMASK()
  lib: 842: don't use GENMASK_ULL()
  bpf: don't use GENMASK()
  kcsan: don't use GENMASK()
  mm: don't use GENMASK()
  fprobe: don't use GENMASK()
  fs: don't use GENMASK()
  fortify-string: don't use GENMASK()
  Docs: add Functions parameters order section

 Documentation/process/coding-style.rst        | 48 ++++++++++++++
 arch/arc/include/asm/disasm.h                 | 62 +++++++++----------
 arch/arc/kernel/disasm.c                      | 46 +++++++-------
 drivers/gpu/drm/rockchip/rockchip_lvds.h      |  2 +-
 drivers/gpu/drm/rockchip/rockchip_vop2_reg.c  |  4 +-
 drivers/i2c/busses/i2c-nomadik.c              | 44 ++++++-------
 .../platform/synopsys/hdmirx/snps_hdmirx.h    |  4 +-
 drivers/mfd/db8500-prcmu-regs.h               |  2 -
 drivers/mmc/host/dw_mmc-rockchip.c            |  4 +-
 .../net/wireless/intel/iwlwifi/fw/api/coex.h  |  2 -
 drivers/soc/rockchip/grf.c                    |  4 +-
 fs/erofs/internal.h                           |  2 +-
 fs/f2fs/data.c                                |  2 +-
 fs/f2fs/inode.c                               |  2 +-
 fs/f2fs/segment.c                             |  2 +-
 fs/f2fs/super.c                               |  2 +-
 fs/proc/task_mmu.c                            |  2 +-
 fs/resctrl/pseudo_lock.c                      |  2 +-
 fs/select.c                                   |  6 +-
 include/asm-generic/fprobe.h                  |  7 +--
 include/linux/bitmap.h                        | 11 ++--
 include/linux/bits.h                          | 11 ++++
 include/linux/f2fs_fs.h                       |  2 +-
 include/linux/find.h                          | 34 +++++-----
 include/linux/fortify-string.h                |  4 +-
 kernel/bpf/verifier.c                         |  4 +-
 kernel/kcsan/encoding.h                       |  4 +-
 kernel/trace/fgraph.c                         | 10 +--
 kernel/trace/trace_probe.h                    |  2 +-
 lib/842/842_compress.c                        |  2 +-
 lib/842/842_decompress.c                      |  2 +-
 lib/bitmap.c                                  |  2 +-
 lib/test_bitmap.c                             | 14 ++---
 lib/zlib_inflate/inflate.c                    | 48 +++++++-------
 mm/debug_vm_pgtable.c                         |  2 +-
 sound/core/oss/rate.c                         | 12 ++--
 sound/soc/rockchip/rockchip_i2s_tdm.h         |  2 +-
 37 files changed, 235 insertions(+), 180 deletions(-)

-- 
2.43.0

[PATCH 01/21] arc: disasm: rename BITS() for FIELD()

[Yury Norov (NVIDIA)]

In preparation for adding generic BITS() macro, rename the local one.

Signed-off-by: Yury Norov (NVIDIA) <yury.norov@gmail.com>
---
 arch/arc/include/asm/disasm.h | 62 +++++++++++++++++------------------
 arch/arc/kernel/disasm.c      | 46 +++++++++++++-------------
 2 files changed, 54 insertions(+), 54 deletions(-)

diff --git a/arch/arc/include/asm/disasm.h b/arch/arc/include/asm/disasm.h
index 61fb4d7affa7..33a0e88e1e8c 100644
--- a/arch/arc/include/asm/disasm.h
+++ b/arch/arc/include/asm/disasm.h
@@ -29,44 +29,44 @@ enum flow {
 };
 
 #define IS_BIT(word, n)		((word) & (1<<n))
-#define BITS(word, s, e)	(((word) >> (s)) & (~((-2) << ((e) - (s)))))
+#define FIELD(word, s, e)	(((word) >> (s)) & (~((-2) << ((e) - (s)))))
 
-#define MAJOR_OPCODE(word)	(BITS((word), 27, 31))
-#define MINOR_OPCODE(word)	(BITS((word), 16, 21))
-#define FIELD_A(word)		(BITS((word), 0, 5))
-#define FIELD_B(word)		((BITS((word), 12, 14)<<3) | \
-				(BITS((word), 24, 26)))
-#define FIELD_C(word)		(BITS((word), 6, 11))
+#define MAJOR_OPCODE(word)	(FIELD((word), 27, 31))
+#define MINOR_OPCODE(word)	(FIELD((word), 16, 21))
+#define FIELD_A(word)		(FIELD((word), 0, 5))
+#define FIELD_B(word)		((FIELD((word), 12, 14)<<3) | \
+				(FIELD((word), 24, 26)))
+#define FIELD_C(word)		(FIELD((word), 6, 11))
 #define FIELD_u6(word)		FIELDC(word)
-#define FIELD_s12(word)		sign_extend(((BITS((word), 0, 5) << 6) | \
-					BITS((word), 6, 11)), 12)
+#define FIELD_s12(word)		sign_extend(((FIELD((word), 0, 5) << 6) | \
+					FIELD((word), 6, 11)), 12)
 
 /* note that for BL/BRcc these two macro's need another AND statement to mask
  * out bit 1 (make the result a multiple of 4) */
-#define FIELD_s9(word)		sign_extend(((BITS(word, 15, 15) << 8) | \
-					BITS(word, 16, 23)), 9)
-#define FIELD_s21(word)		sign_extend(((BITS(word, 6, 15) << 11) | \
-					(BITS(word, 17, 26) << 1)), 12)
-#define FIELD_s25(word)		sign_extend(((BITS(word, 0, 3) << 21) | \
-					(BITS(word, 6, 15) << 11) | \
-					(BITS(word, 17, 26) << 1)), 12)
+#define FIELD_s9(word)		sign_extend(((FIELD(word, 15, 15) << 8) | \
+					FIELD(word, 16, 23)), 9)
+#define FIELD_s21(word)		sign_extend(((FIELD(word, 6, 15) << 11) | \
+					(FIELD(word, 17, 26) << 1)), 12)
+#define FIELD_s25(word)		sign_extend(((FIELD(word, 0, 3) << 21) | \
+					(FIELD(word, 6, 15) << 11) | \
+					(FIELD(word, 17, 26) << 1)), 12)
 
 /* note: these operate on 16 bits! */
-#define FIELD_S_A(word)		((BITS((word), 2, 2)<<3) | BITS((word), 0, 2))
-#define FIELD_S_B(word)		((BITS((word), 10, 10)<<3) | \
-				BITS((word), 8, 10))
-#define FIELD_S_C(word)		((BITS((word), 7, 7)<<3) | BITS((word), 5, 7))
-#define FIELD_S_H(word)		((BITS((word), 0, 2)<<3) | BITS((word), 5, 8))
-#define FIELD_S_u5(word)	(BITS((word), 0, 4))
-#define FIELD_S_u6(word)	(BITS((word), 0, 4) << 1)
-#define FIELD_S_u7(word)	(BITS((word), 0, 4) << 2)
-#define FIELD_S_u10(word)	(BITS((word), 0, 7) << 2)
-#define FIELD_S_s7(word)	sign_extend(BITS((word), 0, 5) << 1, 9)
-#define FIELD_S_s8(word)	sign_extend(BITS((word), 0, 7) << 1, 9)
-#define FIELD_S_s9(word)	sign_extend(BITS((word), 0, 8), 9)
-#define FIELD_S_s10(word)	sign_extend(BITS((word), 0, 8) << 1, 10)
-#define FIELD_S_s11(word)	sign_extend(BITS((word), 0, 8) << 2, 11)
-#define FIELD_S_s13(word)	sign_extend(BITS((word), 0, 10) << 2, 13)
+#define FIELD_S_A(word)		((FIELD((word), 2, 2)<<3) | FIELD((word), 0, 2))
+#define FIELD_S_B(word)		((FIELD((word), 10, 10)<<3) | \
+				FIELD((word), 8, 10))
+#define FIELD_S_C(word)		((FIELD((word), 7, 7)<<3) | FIELD((word), 5, 7))
+#define FIELD_S_H(word)		((FIELD((word), 0, 2)<<3) | FIELD((word), 5, 8))
+#define FIELD_S_u5(word)	(FIELD((word), 0, 4))
+#define FIELD_S_u6(word)	(FIELD((word), 0, 4) << 1)
+#define FIELD_S_u7(word)	(FIELD((word), 0, 4) << 2)
+#define FIELD_S_u10(word)	(FIELD((word), 0, 7) << 2)
+#define FIELD_S_s7(word)	sign_extend(FIELD((word), 0, 5) << 1, 9)
+#define FIELD_S_s8(word)	sign_extend(FIELD((word), 0, 7) << 1, 9)
+#define FIELD_S_s9(word)	sign_extend(FIELD((word), 0, 8), 9)
+#define FIELD_S_s10(word)	sign_extend(FIELD((word), 0, 8) << 1, 10)
+#define FIELD_S_s11(word)	sign_extend(FIELD((word), 0, 8) << 2, 11)
+#define FIELD_S_s13(word)	sign_extend(FIELD((word), 0, 10) << 2, 13)
 
 #define STATUS32_L		0x00000100
 #define REG_LIMM		62
diff --git a/arch/arc/kernel/disasm.c b/arch/arc/kernel/disasm.c
index ccc7e8c39eb3..926b67c40346 100644
--- a/arch/arc/kernel/disasm.c
+++ b/arch/arc/kernel/disasm.c
@@ -100,12 +100,12 @@ void __kprobes disasm_instr(unsigned long addr, struct disasm_state *state,
 
 	case op_LD:  /* LD<zz> a,[b,s9] */
 		state->write = 0;
-		state->di = BITS(state->words[0], 11, 11);
+		state->di = FIELD(state->words[0], 11, 11);
 		if (state->di)
 			break;
-		state->x = BITS(state->words[0], 6, 6);
-		state->zz = BITS(state->words[0], 7, 8);
-		state->aa = BITS(state->words[0], 9, 10);
+		state->x = FIELD(state->words[0], 6, 6);
+		state->zz = FIELD(state->words[0], 7, 8);
+		state->aa = FIELD(state->words[0], 9, 10);
 		state->wb_reg = FIELD_B(state->words[0]);
 		if (state->wb_reg == REG_LIMM) {
 			state->instr_len += 4;
@@ -121,11 +121,11 @@ void __kprobes disasm_instr(unsigned long addr, struct disasm_state *state,
 
 	case op_ST:
 		state->write = 1;
-		state->di = BITS(state->words[0], 5, 5);
+		state->di = FIELD(state->words[0], 5, 5);
 		if (state->di)
 			break;
-		state->aa = BITS(state->words[0], 3, 4);
-		state->zz = BITS(state->words[0], 1, 2);
+		state->aa = FIELD(state->words[0], 3, 4);
+		state->zz = FIELD(state->words[0], 1, 2);
 		state->src1 = FIELD_C(state->words[0]);
 		if (state->src1 == REG_LIMM) {
 			state->instr_len += 4;
@@ -160,7 +160,7 @@ void __kprobes disasm_instr(unsigned long addr, struct disasm_state *state,
 				is_linked = 1;
 
 			fieldCisReg = 0;
-			op_format = BITS(state->words[0], 22, 23);
+			op_format = FIELD(state->words[0], 22, 23);
 			if (op_format == 0 || ((op_format == 3) &&
 				(!IS_BIT(state->words[0], 5)))) {
 				fieldC = FIELD_C(state->words[0]);
@@ -192,7 +192,7 @@ void __kprobes disasm_instr(unsigned long addr, struct disasm_state *state,
 			break;
 
 		case 40:	/* LPcc */
-			if (BITS(state->words[0], 22, 23) == 3) {
+			if (FIELD(state->words[0], 22, 23) == 3) {
 				/* Conditional LPcc u7 */
 				fieldC = FIELD_C(state->words[0]);
 
@@ -207,12 +207,12 @@ void __kprobes disasm_instr(unsigned long addr, struct disasm_state *state,
 			break;
 
 		case 48 ... 55:	/* LD a,[b,c] */
-			state->di = BITS(state->words[0], 15, 15);
+			state->di = FIELD(state->words[0], 15, 15);
 			if (state->di)
 				break;
-			state->x = BITS(state->words[0], 16, 16);
-			state->zz = BITS(state->words[0], 17, 18);
-			state->aa = BITS(state->words[0], 22, 23);
+			state->x = FIELD(state->words[0], 16, 16);
+			state->zz = FIELD(state->words[0], 17, 18);
+			state->aa = FIELD(state->words[0], 22, 23);
 			state->wb_reg = FIELD_B(state->words[0]);
 			if (state->wb_reg == REG_LIMM) {
 				state->instr_len += 4;
@@ -237,7 +237,7 @@ void __kprobes disasm_instr(unsigned long addr, struct disasm_state *state,
 		case 10:	/* MOV */
 			/* still need to check for limm to extract instr len */
 			/* MOV is special case because it only takes 2 args */
-			switch (BITS(state->words[0], 22, 23)) {
+			switch (FIELD(state->words[0], 22, 23)) {
 			case 0: /* OP a,b,c */
 				if (FIELD_C(state->words[0]) == REG_LIMM)
 					state->instr_len += 4;
@@ -258,7 +258,7 @@ void __kprobes disasm_instr(unsigned long addr, struct disasm_state *state,
 		default:
 			/* Not a Load, Jump or Loop instruction */
 			/* still need to check for limm to extract instr len */
-			switch (BITS(state->words[0], 22, 23)) {
+			switch (FIELD(state->words[0], 22, 23)) {
 			case 0: /* OP a,b,c */
 				if ((FIELD_B(state->words[0]) == REG_LIMM) ||
 				    (FIELD_C(state->words[0]) == REG_LIMM))
@@ -281,7 +281,7 @@ void __kprobes disasm_instr(unsigned long addr, struct disasm_state *state,
 
 	/* 16 Bit Instructions */
 	case op_LD_ADD: /* LD_S|LDB_S|LDW_S a,[b,c] */
-		state->zz = BITS(state->words[0], 3, 4);
+		state->zz = FIELD(state->words[0], 3, 4);
 		state->src1 = get_reg(FIELD_S_B(state->words[0]), regs, cregs);
 		state->src2 = get_reg(FIELD_S_C(state->words[0]), regs, cregs);
 		state->dest = FIELD_S_A(state->words[0]);
@@ -289,13 +289,13 @@ void __kprobes disasm_instr(unsigned long addr, struct disasm_state *state,
 
 	case op_ADD_MOV_CMP:
 		/* check for limm, ignore mov_s h,b (== mov_s 0,b) */
-		if ((BITS(state->words[0], 3, 4) < 3) &&
+		if ((FIELD(state->words[0], 3, 4) < 3) &&
 		    (FIELD_S_H(state->words[0]) == REG_LIMM))
 			state->instr_len += 4;
 		break;
 
 	case op_S:
-		subopcode = BITS(state->words[0], 5, 7);
+		subopcode = FIELD(state->words[0], 5, 7);
 		switch (subopcode) {
 		case 0:	/* j_s */
 		case 1:	/* j_s.d */
@@ -308,7 +308,7 @@ void __kprobes disasm_instr(unsigned long addr, struct disasm_state *state,
 				direct_call : indirect_jump;
 			break;
 		case 7:
-			switch (BITS(state->words[0], 8, 10)) {
+			switch (FIELD(state->words[0], 8, 10)) {
 			case 4:	/* jeq_s [blink] */
 			case 5:	/* jne_s [blink] */
 			case 6:	/* j_s [blink] */
@@ -367,8 +367,8 @@ void __kprobes disasm_instr(unsigned long addr, struct disasm_state *state,
 		/* note: we are ignoring possibility of:
 		 * ADD_S, SUB_S, PUSH_S, POP_S as these should not
 		 * cause unaligned exception anyway */
-		state->write = BITS(state->words[0], 6, 6);
-		state->zz = BITS(state->words[0], 5, 5);
+		state->write = FIELD(state->words[0], 6, 6);
+		state->zz = FIELD(state->words[0], 5, 5);
 		if (state->zz)
 			break;	/* byte accesses should not come here */
 		if (!state->write) {
@@ -385,7 +385,7 @@ void __kprobes disasm_instr(unsigned long addr, struct disasm_state *state,
 
 	case op_GP:	/* LD_S|LDB_S|LDW_S r0,[gp,s11/s9/s10] */
 		/* note: ADD_S r0, gp, s11 is ignored */
-		state->zz = BITS(state->words[0], 9, 10);
+		state->zz = FIELD(state->words[0], 9, 10);
 		state->src1 = get_reg(26, regs, cregs);
 		state->src2 = state->zz ? FIELD_S_s10(state->words[0]) :
 			FIELD_S_s11(state->words[0]);
@@ -405,7 +405,7 @@ void __kprobes disasm_instr(unsigned long addr, struct disasm_state *state,
 		break;
 
 	case op_B_S:
-		fieldA = (BITS(state->words[0], 9, 10) == 3) ?
+		fieldA = (FIELD(state->words[0], 9, 10) == 3) ?
 			FIELD_S_s7(state->words[0]) :
 			FIELD_S_s10(state->words[0]);
 		state->target = fieldA + (addr & ~0x03);
-- 
2.43.0

[PATCH 02/21] iwlwifi: drop unused BITS()

[Yury Norov (NVIDIA)]

BITS() is unused in iwlwifi. Drop it.

Signed-off-by: Yury Norov (NVIDIA) <yury.norov@gmail.com>
---
 drivers/net/wireless/intel/iwlwifi/fw/api/coex.h | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/net/wireless/intel/iwlwifi/fw/api/coex.h b/drivers/net/wireless/intel/iwlwifi/fw/api/coex.h
index ddc84430d895..b0d68b4c504b 100644
--- a/drivers/net/wireless/intel/iwlwifi/fw/api/coex.h
+++ b/drivers/net/wireless/intel/iwlwifi/fw/api/coex.h
@@ -11,8 +11,6 @@
 #include <linux/types.h>
 #include <linux/bitops.h>
 
-#define BITS(nb) (BIT(nb) - 1)
-
 enum iwl_bt_coex_lut_type {
 	BT_COEX_TIGHT_LUT = 0,
 	BT_COEX_LOOSE_LUT,
-- 
2.43.0

[PATCH 03/21] select: rename BITS() to FDS_BITS()

[Yury Norov (NVIDIA)]

In preparation for adding generic BITS() macro, rename the local one.

Signed-off-by: Yury Norov (NVIDIA) <yury.norov@gmail.com>
---
 fs/select.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/select.c b/fs/select.c
index 082cf60c7e23..ad5bfb4907ea 100644
--- a/fs/select.c
+++ b/fs/select.c
@@ -412,7 +412,7 @@ void zero_fd_set(unsigned long nr, unsigned long *fdset)
 #define FDS_OUT(fds, n)		(fds->out + n)
 #define FDS_EX(fds, n)		(fds->ex + n)
 
-#define BITS(fds, n)	(*FDS_IN(fds, n)|*FDS_OUT(fds, n)|*FDS_EX(fds, n))
+#define FDS_BITS(fds, n)	(*FDS_IN(fds, n)|*FDS_OUT(fds, n)|*FDS_EX(fds, n))
 
 static int max_select_fd(unsigned long n, fd_set_bits *fds)
 {
@@ -428,7 +428,7 @@ static int max_select_fd(unsigned long n, fd_set_bits *fds)
 	open_fds = fdt->open_fds + n;
 	max = 0;
 	if (set) {
-		set &= BITS(fds, n);
+		set &= FDS_BITS(fds, n);
 		if (set) {
 			if (!(set & ~*open_fds))
 				goto get_max;
@@ -438,7 +438,7 @@ static int max_select_fd(unsigned long n, fd_set_bits *fds)
 	while (n) {
 		open_fds--;
 		n--;
-		set = BITS(fds, n);
+		set = FDS_BITS(fds, n);
 		if (!set)
 			continue;
 		if (set & ~*open_fds)
-- 
2.43.0

Re: [PATCH 03/21] select: rename BITS() to FDS_BITS()

[Jan Kara]

On Sat 25-10-25 12:40:02, Yury Norov (NVIDIA) wrote:
> In preparation for adding generic BITS() macro, rename the local one.
> 
> Signed-off-by: Yury Norov (NVIDIA) <yury.norov@gmail.com>

Looks fine. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/select.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/select.c b/fs/select.c
> index 082cf60c7e23..ad5bfb4907ea 100644
> --- a/fs/select.c
> +++ b/fs/select.c
> @@ -412,7 +412,7 @@ void zero_fd_set(unsigned long nr, unsigned long *fdset)
>  #define FDS_OUT(fds, n)		(fds->out + n)
>  #define FDS_EX(fds, n)		(fds->ex + n)
>  
> -#define BITS(fds, n)	(*FDS_IN(fds, n)|*FDS_OUT(fds, n)|*FDS_EX(fds, n))
> +#define FDS_BITS(fds, n)	(*FDS_IN(fds, n)|*FDS_OUT(fds, n)|*FDS_EX(fds, n))
>  
>  static int max_select_fd(unsigned long n, fd_set_bits *fds)
>  {
> @@ -428,7 +428,7 @@ static int max_select_fd(unsigned long n, fd_set_bits *fds)
>  	open_fds = fdt->open_fds + n;
>  	max = 0;
>  	if (set) {
> -		set &= BITS(fds, n);
> +		set &= FDS_BITS(fds, n);
>  		if (set) {
>  			if (!(set & ~*open_fds))
>  				goto get_max;
> @@ -438,7 +438,7 @@ static int max_select_fd(unsigned long n, fd_set_bits *fds)
>  	while (n) {
>  		open_fds--;
>  		n--;
> -		set = BITS(fds, n);
> +		set = FDS_BITS(fds, n);
>  		if (!set)
>  			continue;
>  		if (set & ~*open_fds)
> -- 
> 2.43.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH 04/21] ALSA: rename BITS to R_BITS

[Yury Norov (NVIDIA)]

In preparation for adding generic BITS() macro, rename the local one.

Signed-off-by: Yury Norov (NVIDIA) <yury.norov@gmail.com>
---
 sound/core/oss/rate.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/sound/core/oss/rate.c b/sound/core/oss/rate.c
index b56eeda5e30e..90a40221e4ce 100644
--- a/sound/core/oss/rate.c
+++ b/sound/core/oss/rate.c
@@ -25,8 +25,8 @@
 #include "pcm_plugin.h"
 
 #define SHIFT	11
-#define BITS	(1<<SHIFT)
-#define R_MASK	(BITS-1)
+#define R_BITS	(1<<SHIFT)
+#define R_MASK	(R_BITS-1)
 
 /*
  *  Basic rate conversion plugin
@@ -104,7 +104,7 @@ static void resample_expand(struct snd_pcm_plugin *plugin,
 					src += src_step;
 				}
 			}
-			val = S1 + ((S2 - S1) * (signed int)pos) / BITS;
+			val = S1 + ((S2 - S1) * (signed int)pos) / R_BITS;
 			if (val < -32768)
 				val = -32768;
 			else if (val > 32767)
@@ -162,7 +162,7 @@ static void resample_shrink(struct snd_pcm_plugin *plugin,
 			}
 			if (pos & ~R_MASK) {
 				pos &= R_MASK;
-				val = S1 + ((S2 - S1) * (signed int)pos) / BITS;
+				val = S1 + ((S2 - S1) * (signed int)pos) / R_BITS;
 				if (val < -32768)
 					val = -32768;
 				else if (val > 32767)
@@ -191,7 +191,7 @@ static snd_pcm_sframes_t rate_src_frames(struct snd_pcm_plugin *plugin, snd_pcm_
 		return 0;
 	data = (struct rate_priv *)plugin->extra_data;
 	if (plugin->src_format.rate < plugin->dst_format.rate) {
-		res = (((frames * data->pitch) + (BITS/2)) >> SHIFT);
+		res = (((frames * data->pitch) + (R_BITS/2)) >> SHIFT);
 	} else {
 		res = DIV_ROUND_CLOSEST(frames << SHIFT, data->pitch);
 	}
@@ -226,7 +226,7 @@ static snd_pcm_sframes_t rate_dst_frames(struct snd_pcm_plugin *plugin, snd_pcm_
 	if (plugin->src_format.rate < plugin->dst_format.rate) {
 		res = DIV_ROUND_CLOSEST(frames << SHIFT, data->pitch);
 	} else {
-		res = (((frames * data->pitch) + (BITS/2)) >> SHIFT);
+		res = (((frames * data->pitch) + (R_BITS/2)) >> SHIFT);
 	}
 	if (data->old_dst_frames > 0) {
 		snd_pcm_sframes_t frames1 = frames, res1 = data->old_src_frames;
-- 
2.43.0

[PATCH 05/21] zlib: rename BITS() to LOWBITS()

[Yury Norov (NVIDIA)]

In preparation from adding generic BITS() macro, rename the local one.

Signed-off-by: Yury Norov (NVIDIA) <yury.norov@gmail.com>
---
 lib/zlib_inflate/inflate.c | 48 +++++++++++++++++++-------------------
 1 file changed, 24 insertions(+), 24 deletions(-)

diff --git a/lib/zlib_inflate/inflate.c b/lib/zlib_inflate/inflate.c
index d1efad69f02b..3102137b51c2 100644
--- a/lib/zlib_inflate/inflate.c
+++ b/lib/zlib_inflate/inflate.c
@@ -229,7 +229,7 @@ static int zlib_inflateSyncPacket(z_streamp strm)
     } while (0)
 
 /* Return the low n bits of the bit accumulator (n < 16) */
-#define BITS(n) \
+#define LOWBITS(n) \
     ((unsigned)hold & ((1U << (n)) - 1))
 
 /* Remove n bits from the bit accumulator */
@@ -270,16 +270,16 @@ static int zlib_inflateSyncPacket(z_streamp strm)
    is:
 
         NEEDBITS(n);
-        ... do something with BITS(n) ...
+        ... do something with LOWBITS(n) ...
         DROPBITS(n);
 
    where NEEDBITS(n) either returns from inflate() if there isn't enough
-   input left to load n bits into the accumulator, or it continues.  BITS(n)
+   input left to load n bits into the accumulator, or it continues. LOWBITS(n)
    gives the low n bits in the accumulator.  When done, DROPBITS(n) drops
    the low n bits off the accumulator.  INITBITS() clears the accumulator
    and sets the number of available bits to zero.  BYTEBITS() discards just
    enough bits to put the accumulator on a byte boundary.  After BYTEBITS()
-   and a NEEDBITS(8), then BITS(8) would return the next byte in the stream.
+   and a NEEDBITS(8), then LOWBITS(8) would return the next byte in the stream.
 
    NEEDBITS(n) uses PULLBYTE() to get an available byte of input, or to return
    if there is no input available.  The decoding of variable length codes uses
@@ -295,7 +295,7 @@ static int zlib_inflateSyncPacket(z_streamp strm)
     case STATEw:
         while (want < need) {
             NEEDBITS(n);
-            keep[want++] = BITS(n);
+            keep[want++] = LOWBITS(n);
             DROPBITS(n);
         }
         state = STATEx;
@@ -369,18 +369,18 @@ int zlib_inflate(z_streamp strm, int flush)
             }
             NEEDBITS(16);
             if (
-                ((BITS(8) << 8) + (hold >> 8)) % 31) {
+                ((LOWBITS(8) << 8) + (hold >> 8)) % 31) {
                 strm->msg = (char *)"incorrect header check";
                 state->mode = BAD;
                 break;
             }
-            if (BITS(4) != Z_DEFLATED) {
+            if (LOWBITS(4) != Z_DEFLATED) {
                 strm->msg = (char *)"unknown compression method";
                 state->mode = BAD;
                 break;
             }
             DROPBITS(4);
-            len = BITS(4) + 8;
+            len = LOWBITS(4) + 8;
             if (len > state->wbits) {
                 strm->msg = (char *)"invalid window size";
                 state->mode = BAD;
@@ -416,9 +416,9 @@ int zlib_inflate(z_streamp strm, int flush)
                 break;
             }
             NEEDBITS(3);
-            state->last = BITS(1);
+            state->last = LOWBITS(1);
             DROPBITS(1);
-            switch (BITS(2)) {
+            switch (LOWBITS(2)) {
             case 0:                             /* stored block */
                 state->mode = STORED;
                 break;
@@ -465,11 +465,11 @@ int zlib_inflate(z_streamp strm, int flush)
             break;
         case TABLE:
             NEEDBITS(14);
-            state->nlen = BITS(5) + 257;
+            state->nlen = LOWBITS(5) + 257;
             DROPBITS(5);
-            state->ndist = BITS(5) + 1;
+            state->ndist = LOWBITS(5) + 1;
             DROPBITS(5);
-            state->ncode = BITS(4) + 4;
+            state->ncode = LOWBITS(4) + 4;
             DROPBITS(4);
 #ifndef PKZIP_BUG_WORKAROUND
             if (state->nlen > 286 || state->ndist > 30) {
@@ -484,7 +484,7 @@ int zlib_inflate(z_streamp strm, int flush)
         case LENLENS:
             while (state->have < state->ncode) {
                 NEEDBITS(3);
-                state->lens[order[state->have++]] = (unsigned short)BITS(3);
+                state->lens[order[state->have++]] = (unsigned short)LOWBITS(3);
                 DROPBITS(3);
             }
             while (state->have < 19)
@@ -505,7 +505,7 @@ int zlib_inflate(z_streamp strm, int flush)
         case CODELENS:
             while (state->have < state->nlen + state->ndist) {
                 for (;;) {
-                    this = state->lencode[BITS(state->lenbits)];
+                    this = state->lencode[LOWBITS(state->lenbits)];
                     if ((unsigned)(this.bits) <= bits) break;
                     PULLBYTE();
                 }
@@ -524,21 +524,21 @@ int zlib_inflate(z_streamp strm, int flush)
                             break;
                         }
                         len = state->lens[state->have - 1];
-                        copy = 3 + BITS(2);
+                        copy = 3 + LOWBITS(2);
                         DROPBITS(2);
                     }
                     else if (this.val == 17) {
                         NEEDBITS(this.bits + 3);
                         DROPBITS(this.bits);
                         len = 0;
-                        copy = 3 + BITS(3);
+                        copy = 3 + LOWBITS(3);
                         DROPBITS(3);
                     }
                     else {
                         NEEDBITS(this.bits + 7);
                         DROPBITS(this.bits);
                         len = 0;
-                        copy = 11 + BITS(7);
+                        copy = 11 + LOWBITS(7);
                         DROPBITS(7);
                     }
                     if (state->have + copy > state->nlen + state->ndist) {
@@ -584,7 +584,7 @@ int zlib_inflate(z_streamp strm, int flush)
                 break;
             }
             for (;;) {
-                this = state->lencode[BITS(state->lenbits)];
+                this = state->lencode[LOWBITS(state->lenbits)];
                 if ((unsigned)(this.bits) <= bits) break;
                 PULLBYTE();
             }
@@ -592,7 +592,7 @@ int zlib_inflate(z_streamp strm, int flush)
                 last = this;
                 for (;;) {
                     this = state->lencode[last.val +
-                            (BITS(last.bits + last.op) >> last.bits)];
+                            (LOWBITS(last.bits + last.op) >> last.bits)];
                     if ((unsigned)(last.bits + this.bits) <= bits) break;
                     PULLBYTE();
                 }
@@ -619,14 +619,14 @@ int zlib_inflate(z_streamp strm, int flush)
         case LENEXT:
             if (state->extra) {
                 NEEDBITS(state->extra);
-                state->length += BITS(state->extra);
+                state->length += LOWBITS(state->extra);
                 DROPBITS(state->extra);
             }
             state->mode = DIST;
 	    fallthrough;
         case DIST:
             for (;;) {
-                this = state->distcode[BITS(state->distbits)];
+                this = state->distcode[LOWBITS(state->distbits)];
                 if ((unsigned)(this.bits) <= bits) break;
                 PULLBYTE();
             }
@@ -634,7 +634,7 @@ int zlib_inflate(z_streamp strm, int flush)
                 last = this;
                 for (;;) {
                     this = state->distcode[last.val +
-                            (BITS(last.bits + last.op) >> last.bits)];
+                            (LOWBITS(last.bits + last.op) >> last.bits)];
                     if ((unsigned)(last.bits + this.bits) <= bits) break;
                     PULLBYTE();
                 }
@@ -653,7 +653,7 @@ int zlib_inflate(z_streamp strm, int flush)
         case DISTEXT:
             if (state->extra) {
                 NEEDBITS(state->extra);
-                state->offset += BITS(state->extra);
+                state->offset += LOWBITS(state->extra);
                 DROPBITS(state->extra);
             }
 #ifdef INFLATE_STRICT
-- 
2.43.0

[PATCH 06/21] mfd: prepare to generalize BITS() macro

[Yury Norov (NVIDIA)]

In preparation for adding generic BITS() macro, add an #undef directive
for the existing BITS(). The following patches will drop it entirely.

Signed-off-by: Yury Norov (NVIDIA) <yury.norov@gmail.com>
---
 drivers/mfd/db8500-prcmu-regs.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/mfd/db8500-prcmu-regs.h b/drivers/mfd/db8500-prcmu-regs.h
index 75fd1069372c..25d2d5966211 100644
--- a/drivers/mfd/db8500-prcmu-regs.h
+++ b/drivers/mfd/db8500-prcmu-regs.h
@@ -12,6 +12,7 @@
 #ifndef __DB8500_PRCMU_REGS_H
 #define __DB8500_PRCMU_REGS_H
 
+#undef BITS
 #define BITS(_start, _end) ((BIT(_end) - BIT(_start)) + BIT(_end))
 
 #define PRCM_ACLK_MGT		(0x004)
-- 
2.43.0

[PATCH 07/21] bits: Add BITS() macro

[Yury Norov (NVIDIA)]

The BITS(low, high) macro is preferable over a similar GENMASK(high, low)
because (low, high) parameters order is more natural. The (high, low)
order is confusing and has a record of misuse.

To enforce unintuitive parameters order, GENMASK() is enforced with
compile time checks. In addition, fixed-width versions of GENMASK() had
been developed. They make sense in describing hardware registers.

In generic code, using standard ordering (low to high) is more preferable,
and fixed-width features are not that useful.

In non-driver code, BITS() must be taken over GENMASK(). In drivers code,
BITS() is preferable over GENMASK().

The following pattern of using GENMASK() is highly unfavorable:

  /* Status register (SR) */
  #define I2C_SR_OP               GENMASK(1, 0)   /* Operation */
  #define I2C_SR_STATUS           GENMASK(3, 2)   /* controller status */
  #define I2C_SR_CAUSE            GENMASK(6, 4)   /* Abort cause */
  #define I2C_SR_TYPE             GENMASK(8, 7)   /* Receive type */
  #define I2C_SR_LENGTH           GENMASK(19, 9)  /* Transfer length */

Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
Link: https://lore.kernel.org/all/CAHk-=whoOUsqPKb7OQwhQf9H_3=5sXGPJrDbfQfwLB3Bi13tcQ@mail.gmail.com/
Signed-off-by: Yury Norov (NVIDIA) <yury.norov@gmail.com>
---
 include/linux/bits.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/linux/bits.h b/include/linux/bits.h
index a40cc861b3a7..c7c587e90e2d 100644
--- a/include/linux/bits.h
+++ b/include/linux/bits.h
@@ -57,6 +57,9 @@
 #define GENMASK_U64(h, l)	GENMASK_TYPE(u64, h, l)
 #define GENMASK_U128(h, l)	GENMASK_TYPE(u128, h, l)
 
+#define BITS(l, h)		GENMASK(h, l)
+#define BITS_ULL(l, h)		GENMASK_ULL(h, l)
+
 /*
  * Fixed-type variants of BIT(), with additional checks like GENMASK_TYPE(). The
  * following examples generate compiler warnings due to -Wshift-count-overflow:
-- 
2.43.0

[PATCH 08/21] mfd: drop local BITS() macro

[Yury Norov (NVIDIA)]

Now that generic BITS() is introduced, drop the local one.

Signed-off-by: Yury Norov (NVIDIA) <yury.norov@gmail.com>
---
 drivers/mfd/db8500-prcmu-regs.h | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/mfd/db8500-prcmu-regs.h b/drivers/mfd/db8500-prcmu-regs.h
index 25d2d5966211..c2bbf325efb9 100644
--- a/drivers/mfd/db8500-prcmu-regs.h
+++ b/drivers/mfd/db8500-prcmu-regs.h
@@ -12,9 +12,6 @@
 #ifndef __DB8500_PRCMU_REGS_H
 #define __DB8500_PRCMU_REGS_H
 
-#undef BITS
-#define BITS(_start, _end) ((BIT(_end) - BIT(_start)) + BIT(_end))
-
 #define PRCM_ACLK_MGT		(0x004)
 #define PRCM_SVAMMCSPCLK_MGT	(0x008)
 #define PRCM_SIAMMDSPCLK_MGT	(0x00C)
-- 
2.43.0

[PATCH 09/21] bits: generalize BITMAP_{FIRST,LAST}_WORD_MASK

[Yury Norov (NVIDIA)]

The macros are helpful in many non-bitmap places too. Move them from
bitmap.h to bits.h.

Signed-off-by: Yury Norov (NVIDIA) <yury.norov@gmail.com>
---
 include/linux/bitmap.h | 7 +++++--
 include/linux/bits.h   | 8 ++++++++
 2 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/include/linux/bitmap.h b/include/linux/bitmap.h
index 595217b7a6e7..fbe2d12bceab 100644
--- a/include/linux/bitmap.h
+++ b/include/linux/bitmap.h
@@ -223,8 +223,11 @@ void bitmap_onto(unsigned long *dst, const unsigned long *orig,
 void bitmap_fold(unsigned long *dst, const unsigned long *orig,
 		unsigned int sz, unsigned int nbits);
 
-#define BITMAP_FIRST_WORD_MASK(start) (~0UL << ((start) & (BITS_PER_LONG - 1)))
-#define BITMAP_LAST_WORD_MASK(nbits) (~0UL >> (-(nbits) & (BITS_PER_LONG - 1)))
+#define BITMAP_FIRST_WORD_MASK(start) LAST_BITS(start)
+#define BITMAP_LAST_WORD_MASK(nbits) FIRST_BITS(nbits)
+
+#define BITMAP_FIRST_WORD_MASK_ULL(start) LAST_BITS(start)
+#define BITMAP_LAST_WORD_MASK_ULL(nbits) FIRST_BITS(nbits)
 
 #define bitmap_size(nbits)	(ALIGN(nbits, BITS_PER_LONG) / BITS_PER_BYTE)
 
diff --git a/include/linux/bits.h b/include/linux/bits.h
index c7c587e90e2d..0d2950b80a3b 100644
--- a/include/linux/bits.h
+++ b/include/linux/bits.h
@@ -5,6 +5,14 @@
 #include <vdso/bits.h>
 #include <uapi/linux/bits.h>
 
+/* Mask with first nbist set */
+#define FIRST_BITS(nbits) (~0UL >> (-(nbits) & (BITS_PER_LONG - 1)))
+#define FIRST_BITS_ULL(nbits) (~0ULL >> (-(nbits) & (BITS_PER_LONG_LONG - 1)))
+
+/* Mask with all bits before start unset */
+#define LAST_BITS(start) (~0UL << ((start) & (BITS_PER_LONG - 1)))
+#define LAST_BITS_ULL(start) (~0ULL << ((start) & (BITS_PER_LONG_LONG - 1)))
+
 #define BIT_MASK(nr)		(UL(1) << ((nr) % BITS_PER_LONG))
 #define BIT_WORD(nr)		((nr) / BITS_PER_LONG)
 #define BIT_ULL_MASK(nr)	(ULL(1) << ((nr) % BITS_PER_LONG_LONG))
-- 
2.43.0

[PATCH 10/21] i2c: nomadik: don't use GENMASK()

[Yury Norov (NVIDIA)]

GENMASK(high, low) notation is confusing. Switch to BITS() or FIRST_BITS()
where appropriate.

Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
Link: https://lore.kernel.org/all/CAHk-=whoOUsqPKb7OQwhQf9H_3=5sXGPJrDbfQfwLB3Bi13tcQ@mail.gmail.com/
Signed-off-by: Yury Norov (NVIDIA) <yury.norov@gmail.com>
---
 drivers/i2c/busses/i2c-nomadik.c | 44 ++++++++++++++++----------------
 1 file changed, 22 insertions(+), 22 deletions(-)

diff --git a/drivers/i2c/busses/i2c-nomadik.c b/drivers/i2c/busses/i2c-nomadik.c
index 19b648fc094d..4c79ada5e1d4 100644
--- a/drivers/i2c/busses/i2c-nomadik.c
+++ b/drivers/i2c/busses/i2c-nomadik.c
@@ -53,9 +53,9 @@
 
 /* Control registers */
 #define I2C_CR_PE		BIT(0)		/* Peripheral Enable */
-#define I2C_CR_OM		GENMASK(2, 1)	/* Operating mode */
+#define I2C_CR_OM		BITS(1, 2)	/* Operating mode */
 #define I2C_CR_SAM		BIT(3)		/* Slave addressing mode */
-#define I2C_CR_SM		GENMASK(5, 4)	/* Speed mode */
+#define I2C_CR_SM		BITS(4, 5)	/* Speed mode */
 #define I2C_CR_SGCM		BIT(6)		/* Slave general call mode */
 #define I2C_CR_FTX		BIT(7)		/* Flush Transmit */
 #define I2C_CR_FRX		BIT(8)		/* Flush Receive */
@@ -63,31 +63,31 @@
 #define I2C_CR_DMA_RX_EN	BIT(10)		/* DMA Rx Enable */
 #define I2C_CR_DMA_SLE		BIT(11)		/* DMA sync. logic enable */
 #define I2C_CR_LM		BIT(12)		/* Loopback mode */
-#define I2C_CR_FON		GENMASK(14, 13)	/* Filtering on */
-#define I2C_CR_FS		GENMASK(16, 15)	/* Force stop enable */
+#define I2C_CR_FON		BITS(13, 14)	/* Filtering on */
+#define I2C_CR_FS		BITS(15, 16)	/* Force stop enable */
 
 /* Slave control register (SCR) */
-#define I2C_SCR_SLSU		GENMASK(31, 16)	/* Slave data setup time */
+#define I2C_SCR_SLSU		BITS(16, 31)	/* Slave data setup time */
 
 /* Master controller (MCR) register */
 #define I2C_MCR_OP		BIT(0)		/* Operation */
-#define I2C_MCR_A7		GENMASK(7, 1)	/* 7-bit address */
-#define I2C_MCR_EA10		GENMASK(10, 8)	/* 10-bit Extended address */
+#define I2C_MCR_A7		BITS(1, 7)	/* 7-bit address */
+#define I2C_MCR_EA10		BITS(8, 10)	/* 10-bit Extended address */
 #define I2C_MCR_SB		BIT(11)		/* Extended address */
-#define I2C_MCR_AM		GENMASK(13, 12)	/* Address type */
+#define I2C_MCR_AM		BITS(12, 13)	/* Address type */
 #define I2C_MCR_STOP		BIT(14)		/* Stop condition */
-#define I2C_MCR_LENGTH		GENMASK(25, 15)	/* Transaction length */
+#define I2C_MCR_LENGTH		BITS(15, 25)	/* Transaction length */
 
 /* Status register (SR) */
-#define I2C_SR_OP		GENMASK(1, 0)	/* Operation */
-#define I2C_SR_STATUS		GENMASK(3, 2)	/* controller status */
-#define I2C_SR_CAUSE		GENMASK(6, 4)	/* Abort cause */
-#define I2C_SR_TYPE		GENMASK(8, 7)	/* Receive type */
-#define I2C_SR_LENGTH		GENMASK(19, 9)	/* Transfer length */
+#define I2C_SR_OP		BITS(0, 1)	/* Operation */
+#define I2C_SR_STATUS		BITS(2, 3)	/* controller status */
+#define I2C_SR_CAUSE		BITS(4, 6)	/* Abort cause */
+#define I2C_SR_TYPE		BITS(7, 8)	/* Receive type */
+#define I2C_SR_LENGTH		BITS(9, 19)	/* Transfer length */
 
 /* Baud-rate counter register (BRCR) */
-#define I2C_BRCR_BRCNT1		GENMASK(31, 16)	/* Baud-rate counter 1 */
-#define I2C_BRCR_BRCNT2		GENMASK(15, 0)	/* Baud-rate counter 2 */
+#define I2C_BRCR_BRCNT2		FIRST_BITS(16)	/* Baud-rate counter 2 */
+#define I2C_BRCR_BRCNT1		BITS(16, 31)	/* Baud-rate counter 1 */
 
 /* Interrupt mask set/clear (IMSCR) bits */
 #define I2C_IT_TXFE		BIT(0)
@@ -339,7 +339,7 @@ static int init_hw(struct nmk_i2c_dev *priv)
 #define DEFAULT_I2C_REG_CR	(FIELD_PREP(I2C_CR_OM, I2C_OM_MASTER) | I2C_CR_PE)
 
 /* grab top three bits from extended I2C addresses */
-#define ADR_3MSB_BITS		GENMASK(9, 7)
+#define ADR_3MSB_BITS		BITS(7, 9)
 
 /**
  * load_i2c_mcr_reg() - load the MCR register
@@ -1028,11 +1028,11 @@ static void nmk_i2c_of_probe(struct device_node *np,
 }
 
 static const unsigned int nmk_i2c_eyeq5_masks[] = {
-	GENMASK(5, 4),
-	GENMASK(7, 6),
-	GENMASK(9, 8),
-	GENMASK(11, 10),
-	GENMASK(13, 12),
+	BITS(4, 5),
+	BITS(6, 7),
+	BITS(8, 9),
+	BITS(10, 11),
+	BITS(12, 13),
 };
 
 static int nmk_i2c_eyeq5_probe(struct nmk_i2c_dev *priv)
-- 
2.43.0

[PATCH 11/21] drivers: don't use GENMASK() in FIELD_PREP_WM16()

[Yury Norov (NVIDIA)]

Recently added FIELD_PREP_WM16() in a few places uses GENMASK. It's
confusing and may mislead readers. Switch to BITS() or FIRST_BITS()
as appropriate.

Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
Link: https://lore.kernel.org/all/CAHk-=whoOUsqPKb7OQwhQf9H_3=5sXGPJrDbfQfwLB3Bi13tcQ@mail.gmail.com/
Signed-off-by: Yury Norov (NVIDIA) <yury.norov@gmail.com>
---
 drivers/gpu/drm/rockchip/rockchip_lvds.h             | 2 +-
 drivers/gpu/drm/rockchip/rockchip_vop2_reg.c         | 4 ++--
 drivers/media/platform/synopsys/hdmirx/snps_hdmirx.h | 4 ++--
 drivers/mmc/host/dw_mmc-rockchip.c                   | 4 ++--
 drivers/soc/rockchip/grf.c                           | 4 ++--
 sound/soc/rockchip/rockchip_i2s_tdm.h                | 2 +-
 6 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_lvds.h b/drivers/gpu/drm/rockchip/rockchip_lvds.h
index 2d92447d819b..e79e6031be59 100644
--- a/drivers/gpu/drm/rockchip/rockchip_lvds.h
+++ b/drivers/gpu/drm/rockchip/rockchip_lvds.h
@@ -115,7 +115,7 @@
 #define   PX30_LVDS_INVERT_DCLK(val)		FIELD_PREP_WM16(BIT(5), (val))
 
 #define PX30_LVDS_GRF_PD_VO_CON1		0x438
-#define   PX30_LVDS_FORMAT(val)			FIELD_PREP_WM16(GENMASK(14, 13), (val))
+#define   PX30_LVDS_FORMAT(val)			FIELD_PREP_WM16(BITS(13, 14), (val))
 #define   PX30_LVDS_MODE_EN(val)		FIELD_PREP_WM16(BIT(12), (val))
 #define   PX30_LVDS_MSBSEL(val)			FIELD_PREP_WM16(BIT(11), (val))
 #define   PX30_LVDS_P2S_EN(val)			FIELD_PREP_WM16(BIT(6), (val))
diff --git a/drivers/gpu/drm/rockchip/rockchip_vop2_reg.c b/drivers/gpu/drm/rockchip/rockchip_vop2_reg.c
index 38c49030c7ab..438fea5f6f6d 100644
--- a/drivers/gpu/drm/rockchip/rockchip_vop2_reg.c
+++ b/drivers/gpu/drm/rockchip/rockchip_vop2_reg.c
@@ -1698,7 +1698,7 @@ static unsigned long rk3588_set_intf_mux(struct vop2_video_port *vp, int id, u32
 		val = rk3588_get_hdmi_pol(polflags);
 		regmap_write(vop2->vop_grf, RK3588_GRF_VOP_CON2, FIELD_PREP_WM16(BIT(1), 1));
 		regmap_write(vop2->vo1_grf, RK3588_GRF_VO1_CON0,
-			     FIELD_PREP_WM16(GENMASK(6, 5), val));
+			     FIELD_PREP_WM16(BITS(5, 6), val));
 		break;
 	case ROCKCHIP_VOP2_EP_HDMI1:
 		div &= ~RK3588_DSP_IF_EDP_HDMI1_DCLK_DIV;
@@ -1711,7 +1711,7 @@ static unsigned long rk3588_set_intf_mux(struct vop2_video_port *vp, int id, u32
 		val = rk3588_get_hdmi_pol(polflags);
 		regmap_write(vop2->vop_grf, RK3588_GRF_VOP_CON2, FIELD_PREP_WM16(BIT(4), 1));
 		regmap_write(vop2->vo1_grf, RK3588_GRF_VO1_CON0,
-			     FIELD_PREP_WM16(GENMASK(8, 7), val));
+			     FIELD_PREP_WM16(BITS(7, 8), val));
 		break;
 	case ROCKCHIP_VOP2_EP_EDP0:
 		div &= ~RK3588_DSP_IF_EDP_HDMI0_DCLK_DIV;
diff --git a/drivers/media/platform/synopsys/hdmirx/snps_hdmirx.h b/drivers/media/platform/synopsys/hdmirx/snps_hdmirx.h
index b13f58e31944..14df3f53ff8f 100644
--- a/drivers/media/platform/synopsys/hdmirx/snps_hdmirx.h
+++ b/drivers/media/platform/synopsys/hdmirx/snps_hdmirx.h
@@ -12,8 +12,8 @@
 #include <linux/bitops.h>
 #include <linux/hw_bitfield.h>
 
-#define UPDATE(x, h, l)		FIELD_PREP(GENMASK((h), (l)), (x))
-#define HIWORD_UPDATE(v, h, l)	FIELD_PREP_WM16(GENMASK((h), (l)), (v))
+#define UPDATE(x, h, l)		FIELD_PREP(BITS((l), (h)), (x))
+#define HIWORD_UPDATE(v, h, l)	FIELD_PREP_WM16(BITS((l), (h)), (v))
 
 /* SYS_GRF */
 #define SYS_GRF_SOC_CON1			0x0304
diff --git a/drivers/mmc/host/dw_mmc-rockchip.c b/drivers/mmc/host/dw_mmc-rockchip.c
index 82dd906bb002..7fac1a7281bf 100644
--- a/drivers/mmc/host/dw_mmc-rockchip.c
+++ b/drivers/mmc/host/dw_mmc-rockchip.c
@@ -148,10 +148,10 @@ static int rockchip_mmc_set_internal_phase(struct dw_mci *host, bool sample, int
 
 	if (sample)
 		mci_writel(host, TIMING_CON1,
-			   FIELD_PREP_WM16(GENMASK(11, 1), raw_value));
+			   FIELD_PREP_WM16(BITS(1, 11), raw_value));
 	else
 		mci_writel(host, TIMING_CON0,
-			   FIELD_PREP_WM16(GENMASK(11, 1), raw_value));
+			   FIELD_PREP_WM16(BITS(1, 11), raw_value));
 
 	dev_dbg(host->dev, "set %s_phase(%d) delay_nums=%u actual_degrees=%d\n",
 		sample ? "sample" : "drv", degrees, delay_num,
diff --git a/drivers/soc/rockchip/grf.c b/drivers/soc/rockchip/grf.c
index 344870da7675..89fd4a4c69eb 100644
--- a/drivers/soc/rockchip/grf.c
+++ b/drivers/soc/rockchip/grf.c
@@ -125,8 +125,8 @@ static const struct rockchip_grf_info rk3566_pipegrf __initconst = {
 #define RK3576_SYSGRF_SOC_CON1		0x0004
 
 static const struct rockchip_grf_value rk3576_defaults_sys_grf[] __initconst = {
-	{ "i3c0 weakpull", RK3576_SYSGRF_SOC_CON1, FIELD_PREP_WM16_CONST(GENMASK(7, 6), 3) },
-	{ "i3c1 weakpull", RK3576_SYSGRF_SOC_CON1, FIELD_PREP_WM16_CONST(GENMASK(9, 8), 3) },
+	{ "i3c0 weakpull", RK3576_SYSGRF_SOC_CON1, FIELD_PREP_WM16_CONST(BITS(6, 7), 3) },
+	{ "i3c1 weakpull", RK3576_SYSGRF_SOC_CON1, FIELD_PREP_WM16_CONST(BITS(8, 9), 3) },
 };
 
 static const struct rockchip_grf_info rk3576_sysgrf __initconst = {
diff --git a/sound/soc/rockchip/rockchip_i2s_tdm.h b/sound/soc/rockchip/rockchip_i2s_tdm.h
index 0171e05ee886..eee6db372ee7 100644
--- a/sound/soc/rockchip/rockchip_i2s_tdm.h
+++ b/sound/soc/rockchip/rockchip_i2s_tdm.h
@@ -287,7 +287,7 @@ enum {
 #define I2S_TDM_RXCR	(0x0034)
 #define I2S_CLKDIV	(0x0038)
 
-#define HIWORD_UPDATE(v, h, l)	(FIELD_PREP_WM16_CONST(GENMASK((h), (l)), (v)))
+#define HIWORD_UPDATE(v, h, l)	(FIELD_PREP_WM16_CONST(BITS((l), (h)), (v)))
 
 /* PX30 GRF CONFIGS */
 #define PX30_I2S0_CLK_IN_SRC_FROM_TX		HIWORD_UPDATE(1, 13, 12)
-- 
2.43.0

Re: [PATCH 11/21] drivers: don't use GENMASK() in FIELD_PREP_WM16()

[Jani Nikula]

On Sat, 25 Oct 2025, "Yury Norov (NVIDIA)" <yury.norov@gmail.com> wrote:
> Recently added FIELD_PREP_WM16() in a few places uses GENMASK. It's
> confusing and may mislead readers. Switch to BITS() or FIRST_BITS()
> as appropriate.

GENMASK argument order matches how most specs I've ever looked at define
bits. It's high:low. I, for one, think it's silly to add another set of
macros purely to swap the argument order.

> Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
> Link: https://lore.kernel.org/all/CAHk-=whoOUsqPKb7OQwhQf9H_3=5sXGPJrDbfQfwLB3Bi13tcQ@mail.gmail.com/
> Signed-off-by: Yury Norov (NVIDIA) <yury.norov@gmail.com>
> ---
>  drivers/gpu/drm/rockchip/rockchip_lvds.h             | 2 +-
>  drivers/gpu/drm/rockchip/rockchip_vop2_reg.c         | 4 ++--
>  drivers/media/platform/synopsys/hdmirx/snps_hdmirx.h | 4 ++--
>  drivers/mmc/host/dw_mmc-rockchip.c                   | 4 ++--
>  drivers/soc/rockchip/grf.c                           | 4 ++--
>  sound/soc/rockchip/rockchip_i2s_tdm.h                | 2 +-
>  6 files changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/rockchip/rockchip_lvds.h b/drivers/gpu/drm/rockchip/rockchip_lvds.h
> index 2d92447d819b..e79e6031be59 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_lvds.h
> +++ b/drivers/gpu/drm/rockchip/rockchip_lvds.h
> @@ -115,7 +115,7 @@
>  #define   PX30_LVDS_INVERT_DCLK(val)		FIELD_PREP_WM16(BIT(5), (val))
>  
>  #define PX30_LVDS_GRF_PD_VO_CON1		0x438
> -#define   PX30_LVDS_FORMAT(val)			FIELD_PREP_WM16(GENMASK(14, 13), (val))
> +#define   PX30_LVDS_FORMAT(val)			FIELD_PREP_WM16(BITS(13, 14), (val))

IMO this change would make more sense in defining register contents:

+#define   PX30_LVDS_FORMAT_MASK			GENMASK(14, 13)
-#define   PX30_LVDS_FORMAT(val)			FIELD_PREP_WM16(GENMASK(14, 13), (val))
+#define   PX30_LVDS_FORMAT(val)			FIELD_PREP_WM16(PX30_LVDS_FORMAT_MASK, (val))


BR,
Jani.


>  #define   PX30_LVDS_MODE_EN(val)		FIELD_PREP_WM16(BIT(12), (val))
>  #define   PX30_LVDS_MSBSEL(val)			FIELD_PREP_WM16(BIT(11), (val))
>  #define   PX30_LVDS_P2S_EN(val)			FIELD_PREP_WM16(BIT(6), (val))
> diff --git a/drivers/gpu/drm/rockchip/rockchip_vop2_reg.c b/drivers/gpu/drm/rockchip/rockchip_vop2_reg.c
> index 38c49030c7ab..438fea5f6f6d 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_vop2_reg.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_vop2_reg.c
> @@ -1698,7 +1698,7 @@ static unsigned long rk3588_set_intf_mux(struct vop2_video_port *vp, int id, u32
>  		val = rk3588_get_hdmi_pol(polflags);
>  		regmap_write(vop2->vop_grf, RK3588_GRF_VOP_CON2, FIELD_PREP_WM16(BIT(1), 1));
>  		regmap_write(vop2->vo1_grf, RK3588_GRF_VO1_CON0,
> -			     FIELD_PREP_WM16(GENMASK(6, 5), val));
> +			     FIELD_PREP_WM16(BITS(5, 6), val));
>  		break;
>  	case ROCKCHIP_VOP2_EP_HDMI1:
>  		div &= ~RK3588_DSP_IF_EDP_HDMI1_DCLK_DIV;
> @@ -1711,7 +1711,7 @@ static unsigned long rk3588_set_intf_mux(struct vop2_video_port *vp, int id, u32
>  		val = rk3588_get_hdmi_pol(polflags);
>  		regmap_write(vop2->vop_grf, RK3588_GRF_VOP_CON2, FIELD_PREP_WM16(BIT(4), 1));
>  		regmap_write(vop2->vo1_grf, RK3588_GRF_VO1_CON0,
> -			     FIELD_PREP_WM16(GENMASK(8, 7), val));
> +			     FIELD_PREP_WM16(BITS(7, 8), val));
>  		break;
>  	case ROCKCHIP_VOP2_EP_EDP0:
>  		div &= ~RK3588_DSP_IF_EDP_HDMI0_DCLK_DIV;
> diff --git a/drivers/media/platform/synopsys/hdmirx/snps_hdmirx.h b/drivers/media/platform/synopsys/hdmirx/snps_hdmirx.h
> index b13f58e31944..14df3f53ff8f 100644
> --- a/drivers/media/platform/synopsys/hdmirx/snps_hdmirx.h
> +++ b/drivers/media/platform/synopsys/hdmirx/snps_hdmirx.h
> @@ -12,8 +12,8 @@
>  #include <linux/bitops.h>
>  #include <linux/hw_bitfield.h>
>  
> -#define UPDATE(x, h, l)		FIELD_PREP(GENMASK((h), (l)), (x))
> -#define HIWORD_UPDATE(v, h, l)	FIELD_PREP_WM16(GENMASK((h), (l)), (v))
> +#define UPDATE(x, h, l)		FIELD_PREP(BITS((l), (h)), (x))
> +#define HIWORD_UPDATE(v, h, l)	FIELD_PREP_WM16(BITS((l), (h)), (v))
>  
>  /* SYS_GRF */
>  #define SYS_GRF_SOC_CON1			0x0304
> diff --git a/drivers/mmc/host/dw_mmc-rockchip.c b/drivers/mmc/host/dw_mmc-rockchip.c
> index 82dd906bb002..7fac1a7281bf 100644
> --- a/drivers/mmc/host/dw_mmc-rockchip.c
> +++ b/drivers/mmc/host/dw_mmc-rockchip.c
> @@ -148,10 +148,10 @@ static int rockchip_mmc_set_internal_phase(struct dw_mci *host, bool sample, int
>  
>  	if (sample)
>  		mci_writel(host, TIMING_CON1,
> -			   FIELD_PREP_WM16(GENMASK(11, 1), raw_value));
> +			   FIELD_PREP_WM16(BITS(1, 11), raw_value));
>  	else
>  		mci_writel(host, TIMING_CON0,
> -			   FIELD_PREP_WM16(GENMASK(11, 1), raw_value));
> +			   FIELD_PREP_WM16(BITS(1, 11), raw_value));
>  
>  	dev_dbg(host->dev, "set %s_phase(%d) delay_nums=%u actual_degrees=%d\n",
>  		sample ? "sample" : "drv", degrees, delay_num,
> diff --git a/drivers/soc/rockchip/grf.c b/drivers/soc/rockchip/grf.c
> index 344870da7675..89fd4a4c69eb 100644
> --- a/drivers/soc/rockchip/grf.c
> +++ b/drivers/soc/rockchip/grf.c
> @@ -125,8 +125,8 @@ static const struct rockchip_grf_info rk3566_pipegrf __initconst = {
>  #define RK3576_SYSGRF_SOC_CON1		0x0004
>  
>  static const struct rockchip_grf_value rk3576_defaults_sys_grf[] __initconst = {
> -	{ "i3c0 weakpull", RK3576_SYSGRF_SOC_CON1, FIELD_PREP_WM16_CONST(GENMASK(7, 6), 3) },
> -	{ "i3c1 weakpull", RK3576_SYSGRF_SOC_CON1, FIELD_PREP_WM16_CONST(GENMASK(9, 8), 3) },
> +	{ "i3c0 weakpull", RK3576_SYSGRF_SOC_CON1, FIELD_PREP_WM16_CONST(BITS(6, 7), 3) },
> +	{ "i3c1 weakpull", RK3576_SYSGRF_SOC_CON1, FIELD_PREP_WM16_CONST(BITS(8, 9), 3) },
>  };
>  
>  static const struct rockchip_grf_info rk3576_sysgrf __initconst = {
> diff --git a/sound/soc/rockchip/rockchip_i2s_tdm.h b/sound/soc/rockchip/rockchip_i2s_tdm.h
> index 0171e05ee886..eee6db372ee7 100644
> --- a/sound/soc/rockchip/rockchip_i2s_tdm.h
> +++ b/sound/soc/rockchip/rockchip_i2s_tdm.h
> @@ -287,7 +287,7 @@ enum {
>  #define I2S_TDM_RXCR	(0x0034)
>  #define I2S_CLKDIV	(0x0038)
>  
> -#define HIWORD_UPDATE(v, h, l)	(FIELD_PREP_WM16_CONST(GENMASK((h), (l)), (v)))
> +#define HIWORD_UPDATE(v, h, l)	(FIELD_PREP_WM16_CONST(BITS((l), (h)), (v)))
>  
>  /* PX30 GRF CONFIGS */
>  #define PX30_I2S0_CLK_IN_SRC_FROM_TX		HIWORD_UPDATE(1, 13, 12)

-- 
Jani Nikula, Intel

Re: [PATCH 11/21] drivers: don't use GENMASK() in FIELD_PREP_WM16()

[Nicolas Frattaroli]

On Saturday, 25 October 2025 18:40:10 Central European Standard Time Yury Norov (NVIDIA) wrote:
> Recently added FIELD_PREP_WM16() in a few places uses GENMASK. It's
> confusing and may mislead readers. Switch to BITS() or FIRST_BITS()
> as appropriate.
> 
> Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
> Link: https://lore.kernel.org/all/CAHk-=whoOUsqPKb7OQwhQf9H_3=5sXGPJrDbfQfwLB3Bi13tcQ@mail.gmail.com/
> Signed-off-by: Yury Norov (NVIDIA) <yury.norov@gmail.com>
> ---
>  drivers/gpu/drm/rockchip/rockchip_lvds.h             | 2 +-
>  drivers/gpu/drm/rockchip/rockchip_vop2_reg.c         | 4 ++--
>  drivers/media/platform/synopsys/hdmirx/snps_hdmirx.h | 4 ++--
>  drivers/mmc/host/dw_mmc-rockchip.c                   | 4 ++--
>  drivers/soc/rockchip/grf.c                           | 4 ++--
>  sound/soc/rockchip/rockchip_i2s_tdm.h                | 2 +-
>  6 files changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rockchip/rockchip_lvds.h b/drivers/gpu/drm/rockchip/rockchip_lvds.h
> index 2d92447d819b..e79e6031be59 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_lvds.h
> +++ b/drivers/gpu/drm/rockchip/rockchip_lvds.h
> @@ -115,7 +115,7 @@
>  #define   PX30_LVDS_INVERT_DCLK(val)		FIELD_PREP_WM16(BIT(5), (val))
>  
>  #define PX30_LVDS_GRF_PD_VO_CON1		0x438
> -#define   PX30_LVDS_FORMAT(val)			FIELD_PREP_WM16(GENMASK(14, 13), (val))
> +#define   PX30_LVDS_FORMAT(val)			FIELD_PREP_WM16(BITS(13, 14), (val))
>  #define   PX30_LVDS_MODE_EN(val)		FIELD_PREP_WM16(BIT(12), (val))
>  #define   PX30_LVDS_MSBSEL(val)			FIELD_PREP_WM16(BIT(11), (val))
>  #define   PX30_LVDS_P2S_EN(val)			FIELD_PREP_WM16(BIT(6), (val))
> diff --git a/drivers/gpu/drm/rockchip/rockchip_vop2_reg.c b/drivers/gpu/drm/rockchip/rockchip_vop2_reg.c
> index 38c49030c7ab..438fea5f6f6d 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_vop2_reg.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_vop2_reg.c
> @@ -1698,7 +1698,7 @@ static unsigned long rk3588_set_intf_mux(struct vop2_video_port *vp, int id, u32
>  		val = rk3588_get_hdmi_pol(polflags);
>  		regmap_write(vop2->vop_grf, RK3588_GRF_VOP_CON2, FIELD_PREP_WM16(BIT(1), 1));
>  		regmap_write(vop2->vo1_grf, RK3588_GRF_VO1_CON0,
> -			     FIELD_PREP_WM16(GENMASK(6, 5), val));
> +			     FIELD_PREP_WM16(BITS(5, 6), val));
>  		break;
>  	case ROCKCHIP_VOP2_EP_HDMI1:
>  		div &= ~RK3588_DSP_IF_EDP_HDMI1_DCLK_DIV;
> @@ -1711,7 +1711,7 @@ static unsigned long rk3588_set_intf_mux(struct vop2_video_port *vp, int id, u32
>  		val = rk3588_get_hdmi_pol(polflags);
>  		regmap_write(vop2->vop_grf, RK3588_GRF_VOP_CON2, FIELD_PREP_WM16(BIT(4), 1));
>  		regmap_write(vop2->vo1_grf, RK3588_GRF_VO1_CON0,
> -			     FIELD_PREP_WM16(GENMASK(8, 7), val));
> +			     FIELD_PREP_WM16(BITS(7, 8), val));
>  		break;
>  	case ROCKCHIP_VOP2_EP_EDP0:
>  		div &= ~RK3588_DSP_IF_EDP_HDMI0_DCLK_DIV;
> diff --git a/drivers/media/platform/synopsys/hdmirx/snps_hdmirx.h b/drivers/media/platform/synopsys/hdmirx/snps_hdmirx.h
> index b13f58e31944..14df3f53ff8f 100644
> --- a/drivers/media/platform/synopsys/hdmirx/snps_hdmirx.h
> +++ b/drivers/media/platform/synopsys/hdmirx/snps_hdmirx.h
> @@ -12,8 +12,8 @@
>  #include <linux/bitops.h>
>  #include <linux/hw_bitfield.h>
>  
> -#define UPDATE(x, h, l)		FIELD_PREP(GENMASK((h), (l)), (x))
> -#define HIWORD_UPDATE(v, h, l)	FIELD_PREP_WM16(GENMASK((h), (l)), (v))
> +#define UPDATE(x, h, l)		FIELD_PREP(BITS((l), (h)), (x))
> +#define HIWORD_UPDATE(v, h, l)	FIELD_PREP_WM16(BITS((l), (h)), (v))
>  
>  /* SYS_GRF */
>  #define SYS_GRF_SOC_CON1			0x0304
> diff --git a/drivers/mmc/host/dw_mmc-rockchip.c b/drivers/mmc/host/dw_mmc-rockchip.c
> index 82dd906bb002..7fac1a7281bf 100644
> --- a/drivers/mmc/host/dw_mmc-rockchip.c
> +++ b/drivers/mmc/host/dw_mmc-rockchip.c
> @@ -148,10 +148,10 @@ static int rockchip_mmc_set_internal_phase(struct dw_mci *host, bool sample, int
>  
>  	if (sample)
>  		mci_writel(host, TIMING_CON1,
> -			   FIELD_PREP_WM16(GENMASK(11, 1), raw_value));
> +			   FIELD_PREP_WM16(BITS(1, 11), raw_value));
>  	else
>  		mci_writel(host, TIMING_CON0,
> -			   FIELD_PREP_WM16(GENMASK(11, 1), raw_value));
> +			   FIELD_PREP_WM16(BITS(1, 11), raw_value));
>  
>  	dev_dbg(host->dev, "set %s_phase(%d) delay_nums=%u actual_degrees=%d\n",
>  		sample ? "sample" : "drv", degrees, delay_num,
> diff --git a/drivers/soc/rockchip/grf.c b/drivers/soc/rockchip/grf.c
> index 344870da7675..89fd4a4c69eb 100644
> --- a/drivers/soc/rockchip/grf.c
> +++ b/drivers/soc/rockchip/grf.c
> @@ -125,8 +125,8 @@ static const struct rockchip_grf_info rk3566_pipegrf __initconst = {
>  #define RK3576_SYSGRF_SOC_CON1		0x0004
>  
>  static const struct rockchip_grf_value rk3576_defaults_sys_grf[] __initconst = {
> -	{ "i3c0 weakpull", RK3576_SYSGRF_SOC_CON1, FIELD_PREP_WM16_CONST(GENMASK(7, 6), 3) },
> -	{ "i3c1 weakpull", RK3576_SYSGRF_SOC_CON1, FIELD_PREP_WM16_CONST(GENMASK(9, 8), 3) },
> +	{ "i3c0 weakpull", RK3576_SYSGRF_SOC_CON1, FIELD_PREP_WM16_CONST(BITS(6, 7), 3) },
> +	{ "i3c1 weakpull", RK3576_SYSGRF_SOC_CON1, FIELD_PREP_WM16_CONST(BITS(8, 9), 3) },
>  };
>  
>  static const struct rockchip_grf_info rk3576_sysgrf __initconst = {
> diff --git a/sound/soc/rockchip/rockchip_i2s_tdm.h b/sound/soc/rockchip/rockchip_i2s_tdm.h
> index 0171e05ee886..eee6db372ee7 100644
> --- a/sound/soc/rockchip/rockchip_i2s_tdm.h
> +++ b/sound/soc/rockchip/rockchip_i2s_tdm.h
> @@ -287,7 +287,7 @@ enum {
>  #define I2S_TDM_RXCR	(0x0034)
>  #define I2S_CLKDIV	(0x0038)
>  
> -#define HIWORD_UPDATE(v, h, l)	(FIELD_PREP_WM16_CONST(GENMASK((h), (l)), (v)))
> +#define HIWORD_UPDATE(v, h, l)	(FIELD_PREP_WM16_CONST(BITS((l), (h)), (v)))
>  
>  /* PX30 GRF CONFIGS */
>  #define PX30_I2S0_CLK_IN_SRC_FROM_TX		HIWORD_UPDATE(1, 13, 12)
> 

Reviewed-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>

I made sure there were no accidental changes introduced by the
swapping of values, so in terms of correctness, this appears
all good to me.

Kind regards,
Nicolas Frattaroli

Re: [PATCH 11/21] drivers: don't use GENMASK() in FIELD_PREP_WM16()

[Mark Brown]

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

On Sat, Oct 25, 2025 at 12:40:10PM -0400, Yury Norov (NVIDIA) wrote:

> Recently added FIELD_PREP_WM16() in a few places uses GENMASK. It's
> confusing and may mislead readers. Switch to BITS() or FIRST_BITS()
> as appropriate.

This doesn't seem to line up with the actual change, you're not altering
FIELD_PREP_WM16() at all but rather altering things that use it.

As mentioned in submitting-patches.rst when submitting a patch series
you should supply a cover letter for that patch series which describes
the overall content of the series.  This helps people understand what
they are looking at and how things fit together.

> ---
>  drivers/gpu/drm/rockchip/rockchip_lvds.h             | 2 +-
>  drivers/gpu/drm/rockchip/rockchip_vop2_reg.c         | 4 ++--
>  drivers/media/platform/synopsys/hdmirx/snps_hdmirx.h | 4 ++--
>  drivers/mmc/host/dw_mmc-rockchip.c                   | 4 ++--
>  drivers/soc/rockchip/grf.c                           | 4 ++--
>  sound/soc/rockchip/rockchip_i2s_tdm.h                | 2 +-
>  6 files changed, 10 insertions(+), 10 deletions(-)

It doesn't help to send changes to unrelated subsystems in one patch
either...

> --- a/sound/soc/rockchip/rockchip_i2s_tdm.h
> +++ b/sound/soc/rockchip/rockchip_i2s_tdm.h
> @@ -287,7 +287,7 @@ enum {
>  #define I2S_TDM_RXCR	(0x0034)
>  #define I2S_CLKDIV	(0x0038)
>  
> -#define HIWORD_UPDATE(v, h, l)	(FIELD_PREP_WM16_CONST(GENMASK((h), (l)), (v)))
> +#define HIWORD_UPDATE(v, h, l)	(FIELD_PREP_WM16_CONST(BITS((l), (h)), (v)))

This is adjusting the implementation of something that takes in h, l to
use l, h which if anything seems likely to introduce confusion, and
definitely feels well into bikeshed territory.  I don't *super* care but
I'm having a hard time seeing the benefit.

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

[PATCH 12/21] bitmap: don't use GENMASK()

[Yury Norov (NVIDIA)]

GENMASK(high, low) notation is confusing. Switch to a more natural
BITS(low, high) mask generator, or FIRST/LAST_BITS() as appropriate.

Signed-off-by: Yury Norov (NVIDIA) <yury.norov@gmail.com>
---
 include/linux/bitmap.h |  4 ++--
 include/linux/find.h   | 34 +++++++++++++++++-----------------
 lib/bitmap.c           |  2 +-
 lib/test_bitmap.c      | 14 +++++++-------
 4 files changed, 27 insertions(+), 27 deletions(-)

diff --git a/include/linux/bitmap.h b/include/linux/bitmap.h
index fbe2d12bceab..3bed29f1780b 100644
--- a/include/linux/bitmap.h
+++ b/include/linux/bitmap.h
@@ -473,7 +473,7 @@ void bitmap_set(unsigned long *map, unsigned int start, unsigned int nbits)
 	if (__builtin_constant_p(nbits) && nbits == 1)
 		__set_bit(start, map);
 	else if (small_const_nbits(start + nbits))
-		*map |= GENMASK(start + nbits - 1, start);
+		*map |= BITS(start, start + nbits - 1);
 	else if (__builtin_constant_p(start & BITMAP_MEM_MASK) &&
 		 IS_ALIGNED(start, BITMAP_MEM_ALIGNMENT) &&
 		 __builtin_constant_p(nbits & BITMAP_MEM_MASK) &&
@@ -489,7 +489,7 @@ void bitmap_clear(unsigned long *map, unsigned int start, unsigned int nbits)
 	if (__builtin_constant_p(nbits) && nbits == 1)
 		__clear_bit(start, map);
 	else if (small_const_nbits(start + nbits))
-		*map &= ~GENMASK(start + nbits - 1, start);
+		*map &= ~BITS(start, start + nbits - 1);
 	else if (__builtin_constant_p(start & BITMAP_MEM_MASK) &&
 		 IS_ALIGNED(start, BITMAP_MEM_ALIGNMENT) &&
 		 __builtin_constant_p(nbits & BITMAP_MEM_MASK) &&
diff --git a/include/linux/find.h b/include/linux/find.h
index 9d720ad92bc1..24d7266fd02b 100644
--- a/include/linux/find.h
+++ b/include/linux/find.h
@@ -66,7 +66,7 @@ unsigned long find_next_bit(const unsigned long *addr, unsigned long size,
 		if (unlikely(offset >= size))
 			return size;
 
-		val = *addr & GENMASK(size - 1, offset);
+		val = *addr & BITS(offset, size - 1);
 		return val ? __ffs(val) : size;
 	}
 
@@ -96,7 +96,7 @@ unsigned long find_next_and_bit(const unsigned long *addr1,
 		if (unlikely(offset >= size))
 			return size;
 
-		val = *addr1 & *addr2 & GENMASK(size - 1, offset);
+		val = *addr1 & *addr2 & BITS(offset, size - 1);
 		return val ? __ffs(val) : size;
 	}
 
@@ -127,7 +127,7 @@ unsigned long find_next_andnot_bit(const unsigned long *addr1,
 		if (unlikely(offset >= size))
 			return size;
 
-		val = *addr1 & ~*addr2 & GENMASK(size - 1, offset);
+		val = *addr1 & ~*addr2 & BITS(offset, size - 1);
 		return val ? __ffs(val) : size;
 	}
 
@@ -157,7 +157,7 @@ unsigned long find_next_or_bit(const unsigned long *addr1,
 		if (unlikely(offset >= size))
 			return size;
 
-		val = (*addr1 | *addr2) & GENMASK(size - 1, offset);
+		val = (*addr1 | *addr2) & BITS(offset, size - 1);
 		return val ? __ffs(val) : size;
 	}
 
@@ -185,7 +185,7 @@ unsigned long find_next_zero_bit(const unsigned long *addr, unsigned long size,
 		if (unlikely(offset >= size))
 			return size;
 
-		val = *addr | ~GENMASK(size - 1, offset);
+		val = *addr | ~BITS(offset, size - 1);
 		return val == ~0UL ? size : ffz(val);
 	}
 
@@ -206,7 +206,7 @@ static __always_inline
 unsigned long find_first_bit(const unsigned long *addr, unsigned long size)
 {
 	if (small_const_nbits(size)) {
-		unsigned long val = *addr & GENMASK(size - 1, 0);
+		unsigned long val = *addr & FIRST_BITS(size);
 
 		return val ? __ffs(val) : size;
 	}
@@ -235,7 +235,7 @@ unsigned long find_nth_bit(const unsigned long *addr, unsigned long size, unsign
 		return size;
 
 	if (small_const_nbits(size)) {
-		unsigned long val =  *addr & GENMASK(size - 1, 0);
+		unsigned long val =  *addr & FIRST_BITS(size);
 
 		return val ? fns(val, n) : size;
 	}
@@ -261,7 +261,7 @@ unsigned long find_nth_and_bit(const unsigned long *addr1, const unsigned long *
 		return size;
 
 	if (small_const_nbits(size)) {
-		unsigned long val =  *addr1 & *addr2 & GENMASK(size - 1, 0);
+		unsigned long val =  *addr1 & *addr2 & FIRST_BITS(size);
 
 		return val ? fns(val, n) : size;
 	}
@@ -291,7 +291,7 @@ unsigned long find_nth_and_andnot_bit(const unsigned long *addr1,
 		return size;
 
 	if (small_const_nbits(size)) {
-		unsigned long val =  *addr1 & *addr2 & (~*addr3) & GENMASK(size - 1, 0);
+		unsigned long val =  *addr1 & *addr2 & (~*addr3) & FIRST_BITS(size);
 
 		return val ? fns(val, n) : size;
 	}
@@ -315,7 +315,7 @@ unsigned long find_first_and_bit(const unsigned long *addr1,
 				 unsigned long size)
 {
 	if (small_const_nbits(size)) {
-		unsigned long val = *addr1 & *addr2 & GENMASK(size - 1, 0);
+		unsigned long val = *addr1 & *addr2 & FIRST_BITS(size);
 
 		return val ? __ffs(val) : size;
 	}
@@ -339,7 +339,7 @@ unsigned long find_first_andnot_bit(const unsigned long *addr1,
 				 unsigned long size)
 {
 	if (small_const_nbits(size)) {
-		unsigned long val = *addr1 & (~*addr2) & GENMASK(size - 1, 0);
+		unsigned long val = *addr1 & (~*addr2) & FIRST_BITS(size);
 
 		return val ? __ffs(val) : size;
 	}
@@ -364,7 +364,7 @@ unsigned long find_first_and_and_bit(const unsigned long *addr1,
 				     unsigned long size)
 {
 	if (small_const_nbits(size)) {
-		unsigned long val = *addr1 & *addr2 & *addr3 & GENMASK(size - 1, 0);
+		unsigned long val = *addr1 & *addr2 & *addr3 & FIRST_BITS(size);
 
 		return val ? __ffs(val) : size;
 	}
@@ -385,7 +385,7 @@ static __always_inline
 unsigned long find_first_zero_bit(const unsigned long *addr, unsigned long size)
 {
 	if (small_const_nbits(size)) {
-		unsigned long val = *addr | ~GENMASK(size - 1, 0);
+		unsigned long val = *addr | ~FIRST_BITS(size);
 
 		return val == ~0UL ? size : ffz(val);
 	}
@@ -406,7 +406,7 @@ static __always_inline
 unsigned long find_last_bit(const unsigned long *addr, unsigned long size)
 {
 	if (small_const_nbits(size)) {
-		unsigned long val = *addr & GENMASK(size - 1, 0);
+		unsigned long val = *addr & FIRST_BITS(size);
 
 		return val ? __fls(val) : size;
 	}
@@ -537,7 +537,7 @@ unsigned long find_next_zero_bit_le(const void *addr, unsigned
 		if (unlikely(offset >= size))
 			return size;
 
-		val = swab(val) | ~GENMASK(size - 1, offset);
+		val = swab(val) | ~BITS(offset, size - 1);
 		return val == ~0UL ? size : ffz(val);
 	}
 
@@ -550,7 +550,7 @@ static __always_inline
 unsigned long find_first_zero_bit_le(const void *addr, unsigned long size)
 {
 	if (small_const_nbits(size)) {
-		unsigned long val = swab(*(const unsigned long *)addr) | ~GENMASK(size - 1, 0);
+		unsigned long val = swab(*(const unsigned long *)addr) | ~FIRST_BITS(size);
 
 		return val == ~0UL ? size : ffz(val);
 	}
@@ -570,7 +570,7 @@ unsigned long find_next_bit_le(const void *addr, unsigned
 		if (unlikely(offset >= size))
 			return size;
 
-		val = swab(val) & GENMASK(size - 1, offset);
+		val = swab(val) & BITS(offset, size - 1);
 		return val ? __ffs(val) : size;
 	}
 
diff --git a/lib/bitmap.c b/lib/bitmap.c
index b97692854966..ec11cc36624e 100644
--- a/lib/bitmap.c
+++ b/lib/bitmap.c
@@ -876,7 +876,7 @@ void bitmap_to_arr64(u64 *buf, const unsigned long *bitmap, unsigned int nbits)
 
 	/* Clear tail bits in the last element of array beyond nbits. */
 	if (nbits % 64)
-		buf[-1] &= GENMASK_ULL((nbits - 1) % 64, 0);
+		buf[-1] &= FIRST_BITS_ULL(nbits);
 }
 EXPORT_SYMBOL(bitmap_to_arr64);
 #endif
diff --git a/lib/test_bitmap.c b/lib/test_bitmap.c
index c83829ef557f..c198fc7a66d2 100644
--- a/lib/test_bitmap.c
+++ b/lib/test_bitmap.c
@@ -692,10 +692,10 @@ static void __init test_bitmap_arr64(void)
 		}
 
 		if ((nbits % 64) &&
-		    (arr[(nbits - 1) / 64] & ~GENMASK_ULL((nbits - 1) % 64, 0))) {
+		    (arr[(nbits - 1) / 64] & ~FIRST_BITS_ULL(nbits))) {
 			pr_err("bitmap_to_arr64(nbits == %d): tail is not safely cleared: 0x%016llx (must be 0x%016llx)\n",
 			       nbits, arr[(nbits - 1) / 64],
-			       GENMASK_ULL((nbits - 1) % 64, 0));
+			       FIRST_BITS_ULL(nbits));
 			failed_tests++;
 		}
 
@@ -1217,7 +1217,7 @@ static void __init test_bitmap_const_eval(void)
 	 * in runtime.
 	 */
 
-	/* Equals to `unsigned long bitmap[1] = { GENMASK(6, 5), }` */
+	/* Equals to `unsigned long bitmap[1] = { BITS(5, 6), }` */
 	bitmap_clear(bitmap, 0, BITS_PER_LONG);
 	if (!test_bit(7, bitmap))
 		bitmap_set(bitmap, 5, 2);
@@ -1229,9 +1229,9 @@ static void __init test_bitmap_const_eval(void)
 	/* Equals to `unsigned long var = BIT(25)` */
 	var |= BIT(25);
 	if (var & BIT(0))
-		var ^= GENMASK(9, 6);
+		var ^= BITS(6, 9);
 
-	/* __const_hweight<32|64>(GENMASK(6, 5)) == 2 */
+	/* __const_hweight<32|64>(BITS(5, 6)) == 2 */
 	res = bitmap_weight(bitmap, 20);
 	BUILD_BUG_ON(!__builtin_constant_p(res));
 	BUILD_BUG_ON(res != 2);
@@ -1241,8 +1241,8 @@ static void __init test_bitmap_const_eval(void)
 	BUILD_BUG_ON(!__builtin_constant_p(res));
 	BUILD_BUG_ON(!res);
 
-	/* BIT(2) & GENMASK(14, 8) == 0 */
-	res = initvar & GENMASK(14, 8);
+	/* BIT(2) & BITS(8, 14) == 0 */
+	res = initvar & BITS(8, 14);
 	BUILD_BUG_ON(!__builtin_constant_p(res));
 	BUILD_BUG_ON(res);
 
-- 
2.43.0

[PATCH 13/21] trace: don't use GENMASK()

[Yury Norov (NVIDIA)]

GENMASK(high, low) notation is confusing. FIRST_BITS() is more
appropriate.

Signed-off-by: Yury Norov (NVIDIA) <yury.norov@gmail.com>
---
 kernel/trace/fgraph.c      | 10 +++++-----
 kernel/trace/trace_probe.h |  2 +-
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/kernel/trace/fgraph.c b/kernel/trace/fgraph.c
index 484ad7a18463..4f21bd837055 100644
--- a/kernel/trace/fgraph.c
+++ b/kernel/trace/fgraph.c
@@ -106,10 +106,10 @@
  *     (RESERVED or BITMAP)
  */
 #define FGRAPH_FRAME_OFFSET_BITS	10
-#define FGRAPH_FRAME_OFFSET_MASK	GENMASK(FGRAPH_FRAME_OFFSET_BITS - 1, 0)
+#define FGRAPH_FRAME_OFFSET_MASK	FIRST_BITS(FGRAPH_FRAME_OFFSET_BITS)
 
 #define FGRAPH_TYPE_BITS	2
-#define FGRAPH_TYPE_MASK	GENMASK(FGRAPH_TYPE_BITS - 1, 0)
+#define FGRAPH_TYPE_MASK	FIRST_BITS(FGRAPH_TYPE_BITS)
 #define FGRAPH_TYPE_SHIFT	FGRAPH_FRAME_OFFSET_BITS
 
 enum {
@@ -123,7 +123,7 @@ enum {
  *   FGRAPH_INDEX (12-27) bits holding the gops index wanting return callback called
  */
 #define FGRAPH_INDEX_BITS	16
-#define FGRAPH_INDEX_MASK	GENMASK(FGRAPH_INDEX_BITS - 1, 0)
+#define FGRAPH_INDEX_MASK	FIRST_BITS(FGRAPH_INDEX_BITS)
 #define FGRAPH_INDEX_SHIFT	(FGRAPH_TYPE_SHIFT + FGRAPH_TYPE_BITS)
 
 /*
@@ -135,12 +135,12 @@ enum {
  *  data_size == 0 means 1 word, and 31 (=2^5 - 1) means 32 words.
  */
 #define FGRAPH_DATA_BITS	5
-#define FGRAPH_DATA_MASK	GENMASK(FGRAPH_DATA_BITS - 1, 0)
+#define FGRAPH_DATA_MASK	FIRST_BITS(FGRAPH_DATA_BITS)
 #define FGRAPH_DATA_SHIFT	(FGRAPH_TYPE_SHIFT + FGRAPH_TYPE_BITS)
 #define FGRAPH_MAX_DATA_SIZE (sizeof(long) * (1 << FGRAPH_DATA_BITS))
 
 #define FGRAPH_DATA_INDEX_BITS	4
-#define FGRAPH_DATA_INDEX_MASK	GENMASK(FGRAPH_DATA_INDEX_BITS - 1, 0)
+#define FGRAPH_DATA_INDEX_MASK	FIRST_BITS(FGRAPH_DATA_INDEX_BITS)
 #define FGRAPH_DATA_INDEX_SHIFT	(FGRAPH_DATA_SHIFT + FGRAPH_DATA_BITS)
 
 #define FGRAPH_MAX_INDEX	\
diff --git a/kernel/trace/trace_probe.h b/kernel/trace/trace_probe.h
index 08b5bda24da2..88de129dcde0 100644
--- a/kernel/trace/trace_probe.h
+++ b/kernel/trace/trace_probe.h
@@ -401,7 +401,7 @@ static inline int traceprobe_get_entry_data_size(struct trace_probe *tp)
 #define TPARG_FL_USER   BIT(4)
 #define TPARG_FL_FPROBE BIT(5)
 #define TPARG_FL_TPOINT BIT(6)
-#define TPARG_FL_LOC_MASK	GENMASK(4, 0)
+#define TPARG_FL_LOC_MASK	FIRST_BITS(5)
 
 static inline bool tparg_is_function_entry(unsigned int flags)
 {
-- 
2.43.0

[PATCH 14/21] lib: 842: don't use GENMASK_ULL()

[Yury Norov (NVIDIA)]

GENMASK_ULL(high, low) notation is confusing. FIRST_BITS_ULL() is more
appropriate.

Signed-off-by: Yury Norov (NVIDIA) <yury.norov@gmail.com>
---
 lib/842/842_compress.c   | 2 +-
 lib/842/842_decompress.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/842/842_compress.c b/lib/842/842_compress.c
index 055356508d97..83b68c85904f 100644
--- a/lib/842/842_compress.c
+++ b/lib/842/842_compress.c
@@ -161,7 +161,7 @@ static int __split_add_bits(struct sw842_param *p, u64 d, u8 n, u8 s)
 	ret = add_bits(p, d >> s, n - s);
 	if (ret)
 		return ret;
-	return add_bits(p, d & GENMASK_ULL(s - 1, 0), s);
+	return add_bits(p, d & FIRST_BITS_ULL(s), s);
 }
 
 static int add_bits(struct sw842_param *p, u64 d, u8 n)
diff --git a/lib/842/842_decompress.c b/lib/842/842_decompress.c
index 582085ef8b49..0520f20f4121 100644
--- a/lib/842/842_decompress.c
+++ b/lib/842/842_decompress.c
@@ -115,7 +115,7 @@ static int next_bits(struct sw842_param *p, u64 *d, u8 n)
 	else
 		*d = be64_to_cpu(get_unaligned((__be64 *)in)) >> (64 - bits);
 
-	*d &= GENMASK_ULL(n - 1, 0);
+	*d &= FIRST_BITS_ULL(n);
 
 	p->bit += n;
 
-- 
2.43.0

[PATCH 15/21] bpf: don't use GENMASK()

[Yury Norov (NVIDIA)]

GENMASK(high, low) notation is confusing. BITS(low, high) is more
appropriate.

Signed-off-by: Yury Norov (NVIDIA) <yury.norov@gmail.com>
---
 kernel/bpf/verifier.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index ff40e5e65c43..a9d690d3a507 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -17676,7 +17676,7 @@ static void mark_fastcall_pattern_for_call(struct bpf_verifier_env *env,
 	 * - includes R1-R5 if corresponding parameter has is described
 	 *   in the function prototype.
 	 */
-	clobbered_regs_mask = GENMASK(cs.num_params, cs.is_void ? 1 : 0);
+	clobbered_regs_mask = BITS(cs.is_void ? 1 : 0, cs.num_params);
 	/* e.g. if helper call clobbers r{0,1}, expect r{2,3,4,5} in the pattern */
 	expected_regs_mask = ~clobbered_regs_mask & ALL_CALLER_SAVED_REGS;
 
@@ -24210,7 +24210,7 @@ static void compute_insn_live_regs(struct bpf_verifier_env *env,
 			def = ALL_CALLER_SAVED_REGS;
 			use = def & ~BIT(BPF_REG_0);
 			if (get_call_summary(env, insn, &cs))
-				use = GENMASK(cs.num_params, 1);
+				use = BITS(1, cs.num_params);
 			break;
 		default:
 			def = 0;
-- 
2.43.0

[PATCH 16/21] kcsan: don't use GENMASK()

[Yury Norov (NVIDIA)]

GENMASK(high, low) notation is confusing. Use BITS(low, high) and
FIRST_BITS() where appropriate.

Signed-off-by: Yury Norov (NVIDIA) <yury.norov@gmail.com>
---
 kernel/kcsan/encoding.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/kcsan/encoding.h b/kernel/kcsan/encoding.h
index 170a2bb22f53..3a4cb7b354e3 100644
--- a/kernel/kcsan/encoding.h
+++ b/kernel/kcsan/encoding.h
@@ -44,8 +44,8 @@
 
 /* Bitmasks for the encoded watchpoint access information. */
 #define WATCHPOINT_WRITE_MASK	BIT(BITS_PER_LONG-1)
-#define WATCHPOINT_SIZE_MASK	GENMASK(BITS_PER_LONG-2, WATCHPOINT_ADDR_BITS)
-#define WATCHPOINT_ADDR_MASK	GENMASK(WATCHPOINT_ADDR_BITS-1, 0)
+#define WATCHPOINT_ADDR_MASK	FIRST_BITS(WATCHPOINT_ADDR_BITS)
+#define WATCHPOINT_SIZE_MASK	BITS(WATCHPOINT_ADDR_BITS, BITS_PER_LONG-2)
 static_assert(WATCHPOINT_ADDR_MASK == (1UL << WATCHPOINT_ADDR_BITS) - 1);
 static_assert((WATCHPOINT_WRITE_MASK ^ WATCHPOINT_SIZE_MASK ^ WATCHPOINT_ADDR_MASK) == ~0UL);
 
-- 
2.43.0

[PATCH 17/21] mm: don't use GENMASK()

[Yury Norov (NVIDIA)]

GENMASK(high, low) notation is confusing. FIRST_BITS() is more
appropriate.

Signed-off-by: Yury Norov (NVIDIA) <yury.norov@gmail.com>
---
 mm/debug_vm_pgtable.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/debug_vm_pgtable.c b/mm/debug_vm_pgtable.c
index 830107b6dd08..b722dc3c091b 100644
--- a/mm/debug_vm_pgtable.c
+++ b/mm/debug_vm_pgtable.c
@@ -40,7 +40,7 @@
  * expectations that are being validated here. All future changes in here
  * or the documentation need to be in sync.
  */
-#define RANDOM_NZVALUE	GENMASK(7, 0)
+#define RANDOM_NZVALUE	FIRST_BITS(8)
 
 struct pgtable_debug_args {
 	struct mm_struct	*mm;
-- 
2.43.0

[PATCH 18/21] fprobe: don't use GENMASK()

[Yury Norov (NVIDIA)]

GENMASK(high, low) notation is confusing. FIRST/LAST_BITS() are
more appropriate.

Signed-off-by: Yury Norov (NVIDIA) <yury.norov@gmail.com>
---
 include/asm-generic/fprobe.h | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/include/asm-generic/fprobe.h b/include/asm-generic/fprobe.h
index 8659a4dc6eb6..bf2523078661 100644
--- a/include/asm-generic/fprobe.h
+++ b/include/asm-generic/fprobe.h
@@ -16,17 +16,14 @@
 #define ARCH_DEFINE_ENCODE_FPROBE_HEADER
 
 #define FPROBE_HEADER_MSB_SIZE_SHIFT (BITS_PER_LONG - FPROBE_DATA_SIZE_BITS)
-#define FPROBE_HEADER_MSB_MASK					\
-	GENMASK(FPROBE_HEADER_MSB_SIZE_SHIFT - 1, 0)
+#define FPROBE_HEADER_MSB_MASK	FIRST_BITS(FPROBE_HEADER_MSB_SIZE_SHIFT)
 
 /*
  * By default, this expects the MSBs in the address of kprobe is 0xf.
  * If any arch needs another fixed pattern (e.g. s390 is zero filled),
  * override this.
  */
-#define FPROBE_HEADER_MSB_PATTERN				\
-	GENMASK(BITS_PER_LONG - 1, FPROBE_HEADER_MSB_SIZE_SHIFT)
-
+#define FPROBE_HEADER_MSB_PATTERN	LAST_BITS(FPROBE_HEADER_MSB_SIZE_SHIFT)
 #define arch_fprobe_header_encodable(fp)			\
 	(((unsigned long)(fp) & ~FPROBE_HEADER_MSB_MASK) ==	\
 	 FPROBE_HEADER_MSB_PATTERN)
-- 
2.43.0

[PATCH 19/21] fs: don't use GENMASK()

[Yury Norov (NVIDIA)]

GENMASK(high, low) notation is confusing. FIRST/LAST_BITS() are
more appropriate.

Signed-off-by: Yury Norov (NVIDIA) <yury.norov@gmail.com>
---
 fs/erofs/internal.h      | 2 +-
 fs/f2fs/data.c           | 2 +-
 fs/f2fs/inode.c          | 2 +-
 fs/f2fs/segment.c        | 2 +-
 fs/f2fs/super.c          | 2 +-
 fs/proc/task_mmu.c       | 2 +-
 fs/resctrl/pseudo_lock.c | 2 +-
 include/linux/f2fs_fs.h  | 2 +-
 8 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/fs/erofs/internal.h b/fs/erofs/internal.h
index f7f622836198..6e0f03092c52 100644
--- a/fs/erofs/internal.h
+++ b/fs/erofs/internal.h
@@ -250,7 +250,7 @@ static inline u64 erofs_nid_to_ino64(struct erofs_sb_info *sbi, erofs_nid_t nid)
 	 * Note: on-disk NIDs remain unchanged as they are primarily used for
 	 * compatibility with non-LFS 32-bit applications.
 	 */
-	return ((nid << 1) & GENMASK_ULL(63, 32)) | (nid & GENMASK(30, 0)) |
+	return ((nid << 1) & LAST_BITS_ULL(32)) | (nid & FIRST_BITS(31)) |
 		((nid >> EROFS_DIRENT_NID_METABOX_BIT) << 31);
 }
 
diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index 775aa4f63aa3..ef08464e003f 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -416,7 +416,7 @@ int f2fs_target_device_index(struct f2fs_sb_info *sbi, block_t blkaddr)
 
 static blk_opf_t f2fs_io_flags(struct f2fs_io_info *fio)
 {
-	unsigned int temp_mask = GENMASK(NR_TEMP_TYPE - 1, 0);
+	unsigned int temp_mask = FIRST_BITS(NR_TEMP_TYPE);
 	unsigned int fua_flag, meta_flag, io_flag;
 	blk_opf_t op_flags = 0;
 
diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
index 8c4eafe9ffac..42a43f558136 100644
--- a/fs/f2fs/inode.c
+++ b/fs/f2fs/inode.c
@@ -524,7 +524,7 @@ static int do_read_inode(struct inode *inode)
 			fi->i_compress_level = compress_flag >>
 						COMPRESS_LEVEL_OFFSET;
 			fi->i_compress_flag = compress_flag &
-					GENMASK(COMPRESS_LEVEL_OFFSET - 1, 0);
+						FIRST_BITS(COMPRESS_LEVEL_OFFSET);
 			fi->i_cluster_size = BIT(fi->i_log_cluster_size);
 			set_inode_flag(inode, FI_COMPRESSED_FILE);
 		}
diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
index b45eace879d7..64433d3b67d4 100644
--- a/fs/f2fs/segment.c
+++ b/fs/f2fs/segment.c
@@ -5425,7 +5425,7 @@ static int do_fix_curseg_write_pointer(struct f2fs_sb_info *sbi, int type)
 		wp_block = zbd->start_blk + (zone.wp >> log_sectors_per_block);
 		wp_segno = GET_SEGNO(sbi, wp_block);
 		wp_blkoff = wp_block - START_BLOCK(sbi, wp_segno);
-		wp_sector_off = zone.wp & GENMASK(log_sectors_per_block - 1, 0);
+		wp_sector_off = zone.wp & FIRST_BITS(log_sectors_per_block);
 
 		if (cs->segno == wp_segno && cs->next_blkoff == wp_blkoff &&
 				wp_sector_off == 0)
diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index db7afb806411..96621fd45cdc 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -4501,7 +4501,7 @@ static void save_stop_reason(struct f2fs_sb_info *sbi, unsigned char reason)
 	unsigned long flags;
 
 	spin_lock_irqsave(&sbi->error_lock, flags);
-	if (sbi->stop_reason[reason] < GENMASK(BITS_PER_BYTE - 1, 0))
+	if (sbi->stop_reason[reason] < FIRST_BITS(BITS_PER_BYTE))
 		sbi->stop_reason[reason]++;
 	spin_unlock_irqrestore(&sbi->error_lock, flags);
 }
diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index fc35a0543f01..71de487b244c 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -1845,7 +1845,7 @@ struct pagemapread {
 
 #define PM_ENTRY_BYTES		sizeof(pagemap_entry_t)
 #define PM_PFRAME_BITS		55
-#define PM_PFRAME_MASK		GENMASK_ULL(PM_PFRAME_BITS - 1, 0)
+#define PM_PFRAME_MASK		FIRST_BITS_ULL(PM_PFRAME_BITS)
 #define PM_SOFT_DIRTY		BIT_ULL(55)
 #define PM_MMAP_EXCLUSIVE	BIT_ULL(56)
 #define PM_UFFD_WP		BIT_ULL(57)
diff --git a/fs/resctrl/pseudo_lock.c b/fs/resctrl/pseudo_lock.c
index 87bbc2605de1..45703bbd3bca 100644
--- a/fs/resctrl/pseudo_lock.c
+++ b/fs/resctrl/pseudo_lock.c
@@ -30,7 +30,7 @@
  */
 static unsigned int pseudo_lock_major;
 
-static unsigned long pseudo_lock_minor_avail = GENMASK(MINORBITS, 0);
+static unsigned long pseudo_lock_minor_avail = FIRST_BITS(MINORBITS + 1);
 
 static char *pseudo_lock_devnode(const struct device *dev, umode_t *mode)
 {
diff --git a/include/linux/f2fs_fs.h b/include/linux/f2fs_fs.h
index 6afb4a13b81d..9996356b79e0 100644
--- a/include/linux/f2fs_fs.h
+++ b/include/linux/f2fs_fs.h
@@ -356,7 +356,7 @@ enum {
 	OFFSET_BIT_SHIFT
 };
 
-#define OFFSET_BIT_MASK		GENMASK(OFFSET_BIT_SHIFT - 1, 0)
+#define OFFSET_BIT_MASK		FIRST_BITS(OFFSET_BIT_SHIFT)
 
 struct f2fs_node {
 	/* can be one of three types: inode, direct, and indirect types */
-- 
2.43.0

[PATCH 20/21] fortify-string: don't use GENMASK()

[Yury Norov (NVIDIA)]

GENMASK(high, low) notation is confusing. BITS(low, high) is more
appropriate.

Signed-off-by: Yury Norov (NVIDIA) <yury.norov@gmail.com>
---
 include/linux/fortify-string.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/linux/fortify-string.h b/include/linux/fortify-string.h
index b3b53f8c1b28..0c95cdcca736 100644
--- a/include/linux/fortify-string.h
+++ b/include/linux/fortify-string.h
@@ -11,9 +11,9 @@
 #define __RENAME(x) __asm__(#x)
 
 #define FORTIFY_REASON_DIR(r)		FIELD_GET(BIT(0), r)
-#define FORTIFY_REASON_FUNC(r)		FIELD_GET(GENMASK(7, 1), r)
+#define FORTIFY_REASON_FUNC(r)		FIELD_GET(BITS(1, 7), r)
 #define FORTIFY_REASON(func, write)	(FIELD_PREP(BIT(0), write) | \
-					 FIELD_PREP(GENMASK(7, 1), func))
+					 FIELD_PREP(BITS(1, 7), func))
 
 /* Overridden by KUnit tests. */
 #ifndef fortify_panic
-- 
2.43.0

[PATCH 21/21] Docs: add Functions parameters order section

[Yury Norov (NVIDIA)]

Standardize parameters ordering in some typical cases to minimize
confusion.

Signed-off-by: Yury Norov (NVIDIA) <yury.norov@gmail.com>
---
 Documentation/process/coding-style.rst | 48 ++++++++++++++++++++++++++
 1 file changed, 48 insertions(+)

diff --git a/Documentation/process/coding-style.rst b/Documentation/process/coding-style.rst
index d1a8e5465ed9..dde24148305c 100644
--- a/Documentation/process/coding-style.rst
+++ b/Documentation/process/coding-style.rst
@@ -523,6 +523,54 @@ below, compared to the **declaration** example above)::
 	...
  }
 
+6.2) Function parameters order
+------------------------------
+
+The order of parameters is important both for code generation and readability.
+Passing parameters in an unusual order is a common source of bugs. Listing
+them in standard widely adopted order helps to avoid confusion.
+
+Many ABIs put first function parameter and return value in R0. If your
+function returns one of its parameters, passing it at the very beginning
+would lead to a better code generation. For example::
+
+        void *memset64(uint64_t *s, uint64_t v, size_t count);
+        void *memcpy(void *dest, const void *src, size_t count);
+
+If your function doesn't propagate a parameter, but has a meaning of copying
+and/or processing data, the best practice is following the traditional order:
+destination, source, options, flags.
+
+for_each()-like iterators should take an enumerator the first. For example::
+
+        for_each_set_bit(bit, mask, nbits);
+                do_something(bit);
+
+        list_for_each_entry(pos, head, member);
+                do_something(pos);
+
+If function operates on a range or ranges of data, corresponding parameters
+may be described as ``start - end`` or ``start - size`` pairs. In both cases,
+the parameters should follow each other. For example::
+
+        int
+        check_range(unsigned long vstart, unsigned long vend,
+                    unsigned long kstart, unsigned long kend);
+
+        static inline void flush_icache_range(unsigned long start, unsigned long end);
+
+        static inline void flush_icache_user_page(struct vm_area_struct *vma,
+                                            struct page *page,
+                                            unsigned long addr, int len);
+
+Both ``start`` and ``end`` of the interval are inclusive.
+
+Describing intervals in order ``end - start`` is unfavorable. One notable
+example is the ``GENMASK(high, low)`` macro. While such a notation is popular
+in hardware context, particularly to describe registers structure, in context
+of software development it looks counter intuitive and confusing. Please switch
+to an equivalent ``BITS(low, high)`` version.
+
 7) Centralized exiting of functions
 -----------------------------------
 
-- 
2.43.0

Re: [PATCH 00/21] lib: add alternatives for GENMASK()

[Linus Torvalds]

On Sat, 25 Oct 2025 at 09:40, Yury Norov (NVIDIA) <yury.norov@gmail.com> wrote:
>
> GENMASK(high, low) notation reflects a common pattern to describe
> hardware registers. However, out of drivers context it's confusing and
> error-prone.

So I obviously approve of the BITS() syntax, and am not a huge fan of
the odd "GENMASK()" argument ordering.

That said, I'm not convinced about adding the other helpers. I don't
think "FIRST_BITS(8)" is any more readable than "BITS(0,7)", and I
think there's a real danger to having lots of specialized macros that
people then have to know what they mean.

I have an absolutely *disgusting* trick to use the syntax

    BITS(3 ... 25)

to do this all, but it's so disgusting and limited that I don't think
it's actually reasonable.

In case somebody can come up with a cleaner model, here's the trick:

   #define LOWRANGE_0 0,
   #define LOWRANGE_1 1,
   #define LOWRANGE_2 2,
   #define LOWRANGE_3 3,
   #define LOWRANGE_4 4,
   #define LOWRANGE_5 5,
   // ..and so on

   #define _HIGH_VAL(x) (sizeof((char[]){[x]=1})-1)
   #define __FIRST(x, ...) x
   #define ___LOW_VAL(x) __FIRST(x)
   #define __LOW_VAL(x) ___LOW_VAL(LOWRANGE_ ##x)
   #define _LOW_VAL(x) __LOW_VAL(x)

   #define BITS(x) GENMASK(_HIGH_VAL(x), _LOW_VAL(x))

   #define TESTVAL 5
   unsigned long val1 = BITS(3 ... 25);
   unsigned long val2 = BITS(4);
   unsigned long val3 = BITS(TESTVAL ... 31);

and that syntax with either "3 ... 25" or just a plain "4" does
actually work. But only with fairly simple numbers.

It doesn't work with more complex expressions (due to the nasty
preprocessor pasting hack), and I couldn't figure out a way to make it
do so.

I also suspect that we shouldn't do the core code conversions without
having acks from maintainers. Some people may prefer the odd
"high,low" ordering due to it matching some equally odd hardware
documentation.

               Linus

Re: [PATCH 00/21] lib: add alternatives for GENMASK()

[Rasmus Villemoes]

On Sat, Oct 25 2025, Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Sat, 25 Oct 2025 at 09:40, Yury Norov (NVIDIA) <yury.norov@gmail.com> wrote:
>>
>> GENMASK(high, low) notation reflects a common pattern to describe
>> hardware registers. However, out of drivers context it's confusing and
>> error-prone.
>
> So I obviously approve of the BITS() syntax, and am not a huge fan of
> the odd "GENMASK()" argument ordering.
>
> That said, I'm not convinced about adding the other helpers. I don't
> think "FIRST_BITS(8)" is any more readable than "BITS(0,7)", and I
> think there's a real danger to having lots of specialized macros that
> people then have to know what they mean.
>
> I have an absolutely *disgusting* trick to use the syntax
>
>     BITS(3 ... 25)
>
> to do this all, but it's so disgusting and limited that I don't think
> it's actually reasonable.
>
> In case somebody can come up with a cleaner model, here's the trick:
>
>    #define LOWRANGE_0 0,
>    #define LOWRANGE_1 1,
>    #define LOWRANGE_2 2,
>    #define LOWRANGE_3 3,
>    #define LOWRANGE_4 4,
>    #define LOWRANGE_5 5,
>    // ..and so on
>
>    #define _HIGH_VAL(x) (sizeof((char[]){[x]=1})-1)
>    #define __FIRST(x, ...) x
>    #define ___LOW_VAL(x) __FIRST(x)
>    #define __LOW_VAL(x) ___LOW_VAL(LOWRANGE_ ##x)
>    #define _LOW_VAL(x) __LOW_VAL(x)
>
>    #define BITS(x) GENMASK(_HIGH_VAL(x), _LOW_VAL(x))
>
>    #define TESTVAL 5
>    unsigned long val1 = BITS(3 ... 25);
>    unsigned long val2 = BITS(4);
>    unsigned long val3 = BITS(TESTVAL ... 31);
>
> and that syntax with either "3 ... 25" or just a plain "4" does
> actually work. But only with fairly simple numbers.
>
> It doesn't work with more complex expressions (due to the nasty
> preprocessor pasting hack), and I couldn't figure out a way to make it
> do so.

It's cute, but no, I also don't know how to make it work without the
preprocessor concatenation stuff.

There is, however, an alternative that resembles the syntax in data
sheets even more closely:

#define BITS(low_high) GENMASK((0 ? low_high), (1 ? low_high))

That'll work for

unsigned long val1 = BITS(3:25);
unsigned long val3 = BITS(TESTVAL:31);

for most of the things TESTVAL might expand to - I'm not sure what would
happen if one of the lo/hi values contains ternaries and isn't properly
parenthesized, but nobody writes stuff like

#define RESET_BIT(rev) rev == 0xaa ? 7 : 9

It doesn't work for the single-bit case, but I don't think it's so bad
to have to say

unsigned long val2 = BIT(4);

and obviously BITS(4:4) would work as well.

It also doesn't do anything to prevent the hi-lo 11:7 order.

Rasmus

Re: [PATCH 00/21] lib: add alternatives for GENMASK()

[david laight]

On Wed, 26 Nov 2025 10:07:34 +0100
Rasmus Villemoes <ravi@prevas.dk> wrote:

> On Sat, Oct 25 2025, Linus Torvalds <torvalds@linux-foundation.org> wrote:
> 
> > On Sat, 25 Oct 2025 at 09:40, Yury Norov (NVIDIA) <yury.norov@gmail.com> wrote:  
> >>
> >> GENMASK(high, low) notation reflects a common pattern to describe
> >> hardware registers. However, out of drivers context it's confusing and
> >> error-prone.  
> >
> > So I obviously approve of the BITS() syntax, and am not a huge fan of
> > the odd "GENMASK()" argument ordering.
> >
> > That said, I'm not convinced about adding the other helpers. I don't
> > think "FIRST_BITS(8)" is any more readable than "BITS(0,7)", and I
> > think there's a real danger to having lots of specialized macros that
> > people then have to know what they mean.
> >
> > I have an absolutely *disgusting* trick to use the syntax
> >
> >     BITS(3 ... 25)
> >
> > to do this all, but it's so disgusting and limited that I don't think
> > it's actually reasonable.
> >
> > In case somebody can come up with a cleaner model, here's the trick:
> >
> >    #define LOWRANGE_0 0,
> >    #define LOWRANGE_1 1,
> >    #define LOWRANGE_2 2,
> >    #define LOWRANGE_3 3,
> >    #define LOWRANGE_4 4,
> >    #define LOWRANGE_5 5,
> >    // ..and so on
> >
> >    #define _HIGH_VAL(x) (sizeof((char[]){[x]=1})-1)
> >    #define __FIRST(x, ...) x
> >    #define ___LOW_VAL(x) __FIRST(x)
> >    #define __LOW_VAL(x) ___LOW_VAL(LOWRANGE_ ##x)
> >    #define _LOW_VAL(x) __LOW_VAL(x)
> >
> >    #define BITS(x) GENMASK(_HIGH_VAL(x), _LOW_VAL(x))
> >
> >    #define TESTVAL 5
> >    unsigned long val1 = BITS(3 ... 25);
> >    unsigned long val2 = BITS(4);
> >    unsigned long val3 = BITS(TESTVAL ... 31);
> >
> > and that syntax with either "3 ... 25" or just a plain "4" does
> > actually work. But only with fairly simple numbers.
> >
> > It doesn't work with more complex expressions (due to the nasty
> > preprocessor pasting hack), and I couldn't figure out a way to make it
> > do so.  
> 
> It's cute, but no, I also don't know how to make it work without the
> preprocessor concatenation stuff.
> 
> There is, however, an alternative that resembles the syntax in data
> sheets even more closely:
> 
> #define BITS(low_high) GENMASK((0 ? low_high), (1 ? low_high))

That is both beautiful and horrid...
 
> 
> That'll work for
> 
> unsigned long val1 = BITS(3:25);
> unsigned long val3 = BITS(TESTVAL:31);
> 
> for most of the things TESTVAL might expand to - I'm not sure what would
> happen if one of the lo/hi values contains ternaries and isn't properly
> parenthesized, but nobody writes stuff like
> 
> #define RESET_BIT(rev) rev == 0xaa ? 7 : 9
> 
> It doesn't work for the single-bit case, but I don't think it's so bad
> to have to say
> 
> unsigned long val2 = BIT(4);
> 
> and obviously BITS(4:4) would work as well.
> 
> It also doesn't do anything to prevent the hi-lo 11:7 order.

For constants that gets trapped.
If the correct expression in used in GENMASK() the compiler will reject
the negative shift - so doesn't need an explicit test.

Not directly related...

GENMASK() and the FIELD_GET() family need to go on massive diets.
Nest them and the pre-processor generates multi-kb lines.

Somewhere there is a (probably pointless) _Generic() for all the integer types.
(Especially since FIELD_GET() is always u64.)

FIELD_GET(mask, val) is just (val & mask) / (mask & -mask), but even that
really needs mask assigned to a local (just to reduce the line length).
For variable 'mask' you might worry about the cost of the 'divide by power of 2'
but for constants it doesn't matter.

FIELD_PREP(mask, val) is (val * (mask & -mask)) & mask

'Bloat city' comes from:
#define MASK GENMASK(9, 0);
	x = FIELD_GET(MASK, MASK);

I've just looked at bitfield.h - no wonder it bloats the world.
95% of it needs deleting :-)

	David

> 
> Rasmus
> 

Re: [PATCH 00/21] lib: add alternatives for GENMASK()

[Linus Torvalds]

On Wed, 26 Nov 2025 at 01:07, Rasmus Villemoes <ravi@prevas.dk> wrote:
>
> There is, however, an alternative that resembles the syntax in data
> sheets even more closely:
>
> #define BITS(low_high) GENMASK((0 ? low_high), (1 ? low_high))

Oh, me likey. That's a much better idea than my crazy syntax.

            Linus

Re: [PATCH 00/21] lib: add alternatives for GENMASK()

[david laight]

On Wed, 26 Nov 2025 11:44:59 -0800
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Wed, 26 Nov 2025 at 01:07, Rasmus Villemoes <ravi@prevas.dk> wrote:
> >
> > There is, however, an alternative that resembles the syntax in data
> > sheets even more closely:
> >
> > #define BITS(low_high) GENMASK((0 ? low_high), (1 ? low_high))  
> 
> Oh, me likey. That's a much better idea than my crazy syntax.

Mark B. will accuse you of abusing ?: :-)

I've just looked at a .i file.
GENMASK() currently expands to 855 chars plus four copies of each argument.
Both FIELD_PREP/GET(GENMASK(), v) are about 18k plus three copies of v.
FIELD_GET(mask,v) has 18 expansions of 'mask'.

HWEIHT32(GENMASK(15,0)) is about 30k - don't ask why anyone would do it.

They all look excessive.

	David

Re: [PATCH 00/21] lib: add alternatives for GENMASK()

[Linus Torvalds]

On Wed, 26 Nov 2025 at 14:17, david laight <david.laight@runbox.com> wrote:
>
> Mark B. will accuse you of abusing ?: :-)

Compared to '__is_constexpr()', this is child's play.  Not *that* is
abusing the ternary op.

> I've just looked at a .i file.
> GENMASK() currently expands to 855 chars plus four copies of each argument.

Yeah, I don't love it. It's a horrid macro. But because of the odd
order of arguments, it needs all that crazy checking.

That said, I do think it could be simplified. Particularly if we just
make the rule be that GENMASK _has_ to take just constant values.

Right now, I think 99.9% of all users are constants, and we spend a
lot of effort on the 0.1% that isn't.

               Linus

Re: [PATCH 00/21] lib: add alternatives for GENMASK()

[david laight]

On Wed, 26 Nov 2025 15:47:28 -0800
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Wed, 26 Nov 2025 at 14:17, david laight <david.laight@runbox.com> wrote:
> >
> > Mark B. will accuse you of abusing ?: :-)  
> 
> Compared to '__is_constexpr()', this is child's play.  Not *that* is
> abusing the ternary op.
> 
> > I've just looked at a .i file.
> > GENMASK() currently expands to 855 chars plus four copies of each argument.  
> 
> Yeah, I don't love it. It's a horrid macro. But because of the odd
> order of arguments, it needs all that crazy checking.
> 
> That said, I do think it could be simplified. Particularly if we just
> make the rule be that GENMASK _has_ to take just constant values.
> 
> Right now, I think 99.9% of all users are constants, and we spend a
> lot of effort on the 0.1% that isn't.

There is the other 0.1% that actually need an 'integer constant expression'.
They stop you using ({ ...}).
I doubt there are many (if any) for FIELD_PREP() and FIELD_GET() and using
	auto _mask = mask;
would shrink those massively.
(If there are any I suspect you are the only person who can fix them
in one release cycle.)

I've not looked closely at the expansions, but as well as some is_constexpr()
(that could probably be 'statically_true()') there is the _Generic() that
generates u8/u16/u32/u64 based on 'something'.
That is mostly entirely pointless, you just need to pick u32 or u64, the u8/u16
get promoted to 'int' as soon as they are used - so it only makes a difference
to code that looks at the type of the variable/result.

In spite of the faffing, the type of FIELD_GET() is u64.

	David

> 
>                Linus
> 

Re: [PATCH 00/21] lib: add alternatives for GENMASK()

[Nicolas Frattaroli]

On Thursday, 27 November 2025 00:47:28 Central European Standard Time Linus Torvalds wrote:
> On Wed, 26 Nov 2025 at 14:17, david laight <david.laight@runbox.com> wrote:
> >
> > Mark B. will accuse you of abusing ?: :-)
> 
> Compared to '__is_constexpr()', this is child's play.  Not *that* is
> abusing the ternary op.
> 
> > I've just looked at a .i file.
> > GENMASK() currently expands to 855 chars plus four copies of each argument.
> 
> Yeah, I don't love it. It's a horrid macro. But because of the odd
> order of arguments, it needs all that crazy checking.
> 
> That said, I do think it could be simplified. Particularly if we just
> make the rule be that GENMASK _has_ to take just constant values.
> 
> Right now, I think 99.9% of all users are constants, and we spend a
> lot of effort on the 0.1% that isn't.
> 
>                Linus
> 

As a datapoint from my hw_bitfield.h series, all but 3 drivers were
trivial to convert to using constant bit masks, which is required by
both variants of the FIELD_PREP_WM16* macro.

One of the big outliers was the Rockchip clock driver, which needed
non-const things all over the place, and wouldn't have been that
easy to refactor.

Another one worth calling out is phy-rockchip-inno-csidphy.c, where
the non-constness of the mask was inherited from a pointer dereference
of what is a const value, but the indirection makes it non-const,
and I didn't want to have to deal with that 20 patches in without a
good way to test it. Entirely possible I was a doofus and didn't try
making the pointer itself const as well.

Occasionally, masks in code are non-constant for frivolous reasons,
like using function arguments as the mask because they're assumed
to be always called with one of a set of known values, but that set
is never made obvious unless you check all the call sites. The
spicier variant of that is masks read from hardware registers or
firmware, because at that point you have no view into the
possibilities, but I've yet to come across this.

I think enforcing GENMASK to take constant values is a good idea in
light of that, because people who really need a non-const version
should make this explicit so that reviewers can catch it as a
potential code smell, in addition to the less horrid preprocessor
output.

Kind regards,
Nicolas Frattaroli

Re: [PATCH 00/21] lib: add alternatives for GENMASK()

[Linus Torvalds]

On Thu, 27 Nov 2025 at 06:12, Nicolas Frattaroli
<nicolas.frattaroli@collabora.com> wrote:
>
> I think enforcing GENMASK to take constant values is a good idea in
> light of that, because people who really need a non-const version
> should make this explicit so that reviewers can catch it as a
> potential code smell, in addition to the less horrid preprocessor
> output.

So I did some very preliminary checking, and we do end up having more
GENMASK() users with non-constant arguments than I thought.

They are still a small minority - about 3% of the users - which is why
my first grep made me think "it's all just constants".

It's just not quite as tiny a minority as I thought from a quick visual grep.

IOW, there's about a thousand cases of GENMASK() with things that
aren't obvious constants.

So trying to get rid of them would be somewhat painful. Fairly easily
automated, but still..

             Linus

Re: [PATCH 00/21] lib: add alternatives for GENMASK()

[david laight]

On Wed, 26 Nov 2025 22:17:18 +0000
david laight <david.laight@runbox.com> wrote:

...
> Both FIELD_PREP/GET(GENMASK(), v) are about 18k plus three copies of v.
> FIELD_GET(mask,v) has 18 expansions of 'mask'.

All the FIELD_XXX() are already statement expressions.
I think that means they should 'just work' if the parameters are
copied to locals - annoyingly there are a few bitfields...

Even after that a shed-load (or two) of bloat comes from __BF_FIELD_CHECK().
Not helped by expanding pointless checks because the same define is
used lots of times.

I'm not really sure the expensive test that uses __bf_cast_unsigned()
is actually worth doing - unless a trivial equivalent can be found.
Actually the 'mask' side can just be 'mask + 0u + 0ul + 0ull' which
zero-extends 'mask';
the other just '~0ull >> (64 - sizeof(_reg) * 8)'.

I see a patch lurking...

	David

Re: [PATCH 00/21] lib: add alternatives for GENMASK()

[david laight]

On Wed, 26 Nov 2025 11:44:59 -0800
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Wed, 26 Nov 2025 at 01:07, Rasmus Villemoes <ravi@prevas.dk> wrote:
> >
> > There is, however, an alternative that resembles the syntax in data
> > sheets even more closely:
> >
> > #define BITS(low_high) GENMASK((0 ? low_high), (1 ? low_high))  
> 
> Oh, me likey. That's a much better idea than my crazy syntax.
> 
>             Linus
> 

I've had a couple of further thoughts...

1) GENMASK() could just swap the two arguments if they are constant and
in the wrong order, so GENMASK(2, 4) and GENMASK(4, 2) are equivalent.
Easily done without too much bloat in a statement expression.

2) It a lot of cases the result from GENMASK() is only ever passed to FIELD_XXX().
The latter really don't want the mask - they want the two bit numbers.
(In particular the low bit number.)

So you could change the headers to have (eg):
#define REGISTER_BITS_FOR_XXX (5:7)

Then the expected calling 'convention' would be FIELD_XXX((lo:hi), val).
All done with:
#define _BIT_LO(lo_hi) (1 ? lo_hi)
#define _BIT_HI(lo_hi) (0 ? lo_hi)
#define FIELD_XXX(lo_hi, ...) ({
	u32 _lo = _BIT_LO lo_hi;
	u32 _hi = _BIT_HI lo_hi;
	...

Any attempted use without the 'magic macros' will be a nice syntax error.

	David

[PATCH 00/21] lib: add alternatives for GENMASK()

[Yury Norov (NVIDIA)]

GENMASK(high, low) notation reflects a common pattern to describe
hardware registers. However, out of drivers context it's confusing and
error-prone. 

This series introduces alternatives for GENMASK():

 - BITS(low, high);
 - FIRST_BITS(nbits);
 - LAST_BITS(start);

Patches 1-6 and 8 rename existing local BITS(). Patches 7 and 9 introduce
new generic macros. Patches 10-18 switch GENMASK() usage in core files to
better alternatives. Patch 19 adds Documentation section describing
preferred parameters ordering.

The series is inspired by:

https://lore.kernel.org/all/CAHk-=whoOUsqPKb7OQwhQf9H_3=5sXGPJrDbfQfwLB3Bi13tcQ@mail.gmail.com/

Yury Norov (NVIDIA) (21):
  arc: disasm: rename BITS() for FIELD()
  iwlwifi: drop unused BITS()
  select: rename BITS() to FDS_BITS()
  ALSA: rename BITS to R_BITS
  zlib: BITS() to LOWBITS()
  mfd: prepare to generalize BITS() macro
  bits: Add BITS() macro
  mfd: drop local BITS() macro
  bits: generalize BITMAP_{FIRST,LAST}_WORD_MASK
  i2c: nomadik: don't use GENMASK()
  drivers: don't use GENMASK() in FIELD_PREP_WM16()
  bitmap: don't use GENMASK()
  trace: don't use GENMASK()
  lib: 842: don't use GENMASK_ULL()
  bpf: don't use GENMASK()
  kcsan: don't use GENMASK()
  mm: don't use GENMASK()
  fprobe: don't use GENMASK()
  fs: don't use GENMASK()
  fortify-string: don't use GENMASK()
  Docs: add Functions parameters order section

 Documentation/process/coding-style.rst        | 48 ++++++++++++++
 arch/arc/include/asm/disasm.h                 | 62 +++++++++----------
 arch/arc/kernel/disasm.c                      | 46 +++++++-------
 drivers/gpu/drm/rockchip/rockchip_lvds.h      |  2 +-
 drivers/gpu/drm/rockchip/rockchip_vop2_reg.c  |  4 +-
 drivers/i2c/busses/i2c-nomadik.c              | 44 ++++++-------
 .../platform/synopsys/hdmirx/snps_hdmirx.h    |  4 +-
 drivers/mfd/db8500-prcmu-regs.h               |  2 -
 drivers/mmc/host/dw_mmc-rockchip.c            |  4 +-
 .../net/wireless/intel/iwlwifi/fw/api/coex.h  |  2 -
 drivers/soc/rockchip/grf.c                    |  4 +-
 fs/erofs/internal.h                           |  2 +-
 fs/f2fs/data.c                                |  2 +-
 fs/f2fs/inode.c                               |  2 +-
 fs/f2fs/segment.c                             |  2 +-
 fs/f2fs/super.c                               |  2 +-
 fs/proc/task_mmu.c                            |  2 +-
 fs/resctrl/pseudo_lock.c                      |  2 +-
 fs/select.c                                   |  6 +-
 include/asm-generic/fprobe.h                  |  7 +--
 include/linux/bitmap.h                        | 11 ++--
 include/linux/bits.h                          | 11 ++++
 include/linux/f2fs_fs.h                       |  2 +-
 include/linux/find.h                          | 34 +++++-----
 include/linux/fortify-string.h                |  4 +-
 kernel/bpf/verifier.c                         |  4 +-
 kernel/kcsan/encoding.h                       |  4 +-
 kernel/trace/fgraph.c                         | 10 +--
 kernel/trace/trace_probe.h                    |  2 +-
 lib/842/842_compress.c                        |  2 +-
 lib/842/842_decompress.c                      |  2 +-
 lib/bitmap.c                                  |  2 +-
 lib/test_bitmap.c                             | 14 ++---
 lib/zlib_inflate/inflate.c                    | 48 +++++++-------
 mm/debug_vm_pgtable.c                         |  2 +-
 sound/core/oss/rate.c                         | 12 ++--
 sound/soc/rockchip/rockchip_i2s_tdm.h         |  2 +-
 37 files changed, 235 insertions(+), 180 deletions(-)

-- 
2.43.0

[PATCH 11/21] drivers: don't use GENMASK() in FIELD_PREP_WM16()

[Yury Norov (NVIDIA)]

Recently added FIELD_PREP_WM16() in a few places uses GENMASK. It's
confusing and may mislead readers. Switch to BITS() or FIRST_BITS()
as appropriate.

Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
Link: https://lore.kernel.org/all/CAHk-=whoOUsqPKb7OQwhQf9H_3=5sXGPJrDbfQfwLB3Bi13tcQ@mail.gmail.com/
Signed-off-by: Yury Norov (NVIDIA) <yury.norov@gmail.com>
---
 drivers/gpu/drm/rockchip/rockchip_lvds.h             | 2 +-
 drivers/gpu/drm/rockchip/rockchip_vop2_reg.c         | 4 ++--
 drivers/media/platform/synopsys/hdmirx/snps_hdmirx.h | 4 ++--
 drivers/mmc/host/dw_mmc-rockchip.c                   | 4 ++--
 drivers/soc/rockchip/grf.c                           | 4 ++--
 sound/soc/rockchip/rockchip_i2s_tdm.h                | 2 +-
 6 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_lvds.h b/drivers/gpu/drm/rockchip/rockchip_lvds.h
index 2d92447d819b..e79e6031be59 100644
--- a/drivers/gpu/drm/rockchip/rockchip_lvds.h
+++ b/drivers/gpu/drm/rockchip/rockchip_lvds.h
@@ -115,7 +115,7 @@
 #define   PX30_LVDS_INVERT_DCLK(val)		FIELD_PREP_WM16(BIT(5), (val))
 
 #define PX30_LVDS_GRF_PD_VO_CON1		0x438
-#define   PX30_LVDS_FORMAT(val)			FIELD_PREP_WM16(GENMASK(14, 13), (val))
+#define   PX30_LVDS_FORMAT(val)			FIELD_PREP_WM16(BITS(13, 14), (val))
 #define   PX30_LVDS_MODE_EN(val)		FIELD_PREP_WM16(BIT(12), (val))
 #define   PX30_LVDS_MSBSEL(val)			FIELD_PREP_WM16(BIT(11), (val))
 #define   PX30_LVDS_P2S_EN(val)			FIELD_PREP_WM16(BIT(6), (val))
diff --git a/drivers/gpu/drm/rockchip/rockchip_vop2_reg.c b/drivers/gpu/drm/rockchip/rockchip_vop2_reg.c
index 38c49030c7ab..438fea5f6f6d 100644
--- a/drivers/gpu/drm/rockchip/rockchip_vop2_reg.c
+++ b/drivers/gpu/drm/rockchip/rockchip_vop2_reg.c
@@ -1698,7 +1698,7 @@ static unsigned long rk3588_set_intf_mux(struct vop2_video_port *vp, int id, u32
 		val = rk3588_get_hdmi_pol(polflags);
 		regmap_write(vop2->vop_grf, RK3588_GRF_VOP_CON2, FIELD_PREP_WM16(BIT(1), 1));
 		regmap_write(vop2->vo1_grf, RK3588_GRF_VO1_CON0,
-			     FIELD_PREP_WM16(GENMASK(6, 5), val));
+			     FIELD_PREP_WM16(BITS(5, 6), val));
 		break;
 	case ROCKCHIP_VOP2_EP_HDMI1:
 		div &= ~RK3588_DSP_IF_EDP_HDMI1_DCLK_DIV;
@@ -1711,7 +1711,7 @@ static unsigned long rk3588_set_intf_mux(struct vop2_video_port *vp, int id, u32
 		val = rk3588_get_hdmi_pol(polflags);
 		regmap_write(vop2->vop_grf, RK3588_GRF_VOP_CON2, FIELD_PREP_WM16(BIT(4), 1));
 		regmap_write(vop2->vo1_grf, RK3588_GRF_VO1_CON0,
-			     FIELD_PREP_WM16(GENMASK(8, 7), val));
+			     FIELD_PREP_WM16(BITS(7, 8), val));
 		break;
 	case ROCKCHIP_VOP2_EP_EDP0:
 		div &= ~RK3588_DSP_IF_EDP_HDMI0_DCLK_DIV;
diff --git a/drivers/media/platform/synopsys/hdmirx/snps_hdmirx.h b/drivers/media/platform/synopsys/hdmirx/snps_hdmirx.h
index b13f58e31944..14df3f53ff8f 100644
--- a/drivers/media/platform/synopsys/hdmirx/snps_hdmirx.h
+++ b/drivers/media/platform/synopsys/hdmirx/snps_hdmirx.h
@@ -12,8 +12,8 @@
 #include <linux/bitops.h>
 #include <linux/hw_bitfield.h>
 
-#define UPDATE(x, h, l)		FIELD_PREP(GENMASK((h), (l)), (x))
-#define HIWORD_UPDATE(v, h, l)	FIELD_PREP_WM16(GENMASK((h), (l)), (v))
+#define UPDATE(x, h, l)		FIELD_PREP(BITS((l), (h)), (x))
+#define HIWORD_UPDATE(v, h, l)	FIELD_PREP_WM16(BITS((l), (h)), (v))
 
 /* SYS_GRF */
 #define SYS_GRF_SOC_CON1			0x0304
diff --git a/drivers/mmc/host/dw_mmc-rockchip.c b/drivers/mmc/host/dw_mmc-rockchip.c
index 82dd906bb002..7fac1a7281bf 100644
--- a/drivers/mmc/host/dw_mmc-rockchip.c
+++ b/drivers/mmc/host/dw_mmc-rockchip.c
@@ -148,10 +148,10 @@ static int rockchip_mmc_set_internal_phase(struct dw_mci *host, bool sample, int
 
 	if (sample)
 		mci_writel(host, TIMING_CON1,
-			   FIELD_PREP_WM16(GENMASK(11, 1), raw_value));
+			   FIELD_PREP_WM16(BITS(1, 11), raw_value));
 	else
 		mci_writel(host, TIMING_CON0,
-			   FIELD_PREP_WM16(GENMASK(11, 1), raw_value));
+			   FIELD_PREP_WM16(BITS(1, 11), raw_value));
 
 	dev_dbg(host->dev, "set %s_phase(%d) delay_nums=%u actual_degrees=%d\n",
 		sample ? "sample" : "drv", degrees, delay_num,
diff --git a/drivers/soc/rockchip/grf.c b/drivers/soc/rockchip/grf.c
index 344870da7675..89fd4a4c69eb 100644
--- a/drivers/soc/rockchip/grf.c
+++ b/drivers/soc/rockchip/grf.c
@@ -125,8 +125,8 @@ static const struct rockchip_grf_info rk3566_pipegrf __initconst = {
 #define RK3576_SYSGRF_SOC_CON1		0x0004
 
 static const struct rockchip_grf_value rk3576_defaults_sys_grf[] __initconst = {
-	{ "i3c0 weakpull", RK3576_SYSGRF_SOC_CON1, FIELD_PREP_WM16_CONST(GENMASK(7, 6), 3) },
-	{ "i3c1 weakpull", RK3576_SYSGRF_SOC_CON1, FIELD_PREP_WM16_CONST(GENMASK(9, 8), 3) },
+	{ "i3c0 weakpull", RK3576_SYSGRF_SOC_CON1, FIELD_PREP_WM16_CONST(BITS(6, 7), 3) },
+	{ "i3c1 weakpull", RK3576_SYSGRF_SOC_CON1, FIELD_PREP_WM16_CONST(BITS(8, 9), 3) },
 };
 
 static const struct rockchip_grf_info rk3576_sysgrf __initconst = {
diff --git a/sound/soc/rockchip/rockchip_i2s_tdm.h b/sound/soc/rockchip/rockchip_i2s_tdm.h
index 0171e05ee886..eee6db372ee7 100644
--- a/sound/soc/rockchip/rockchip_i2s_tdm.h
+++ b/sound/soc/rockchip/rockchip_i2s_tdm.h
@@ -287,7 +287,7 @@ enum {
 #define I2S_TDM_RXCR	(0x0034)
 #define I2S_CLKDIV	(0x0038)
 
-#define HIWORD_UPDATE(v, h, l)	(FIELD_PREP_WM16_CONST(GENMASK((h), (l)), (v)))
+#define HIWORD_UPDATE(v, h, l)	(FIELD_PREP_WM16_CONST(BITS((l), (h)), (v)))
 
 /* PX30 GRF CONFIGS */
 #define PX30_I2S0_CLK_IN_SRC_FROM_TX		HIWORD_UPDATE(1, 13, 12)
-- 
2.43.0