* Re: [PATCH 11/21] drivers: don't use GENMASK() in FIELD_PREP_WM16()
2025-10-25 16:40 ` [PATCH 11/21] drivers: don't use GENMASK() in FIELD_PREP_WM16() Yury Norov (NVIDIA)
@ 2025-10-27 8:49 ` Jani Nikula
2025-10-27 12:21 ` Nicolas Frattaroli
2025-10-27 13:10 ` Mark Brown
2 siblings, 0 replies; 6+ messages in thread
From: Jani Nikula @ 2025-10-27 8:49 UTC (permalink / raw)
To: Yury Norov (NVIDIA), Linus Torvalds, Linus Walleij,
Nicolas Frattaroli, Sandy Huang, Heiko Stübner, Andy Yan,
Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie,
Simona Vetter, Shreeya Patel, Mauro Carvalho Chehab,
Jaehoon Chung, Ulf Hansson, Nicolas Frattaroli, Liam Girdwood,
Mark Brown, Jaroslav Kysela, Takashi Iwai, Yury Norov (NVIDIA),
dri-devel, linux-arm-kernel, linux-rockchip, linux-kernel,
linux-media, kernel, linux-mmc, linux-sound
Cc: Rasmus Villemoes
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 11/21] drivers: don't use GENMASK() in FIELD_PREP_WM16()
2025-10-25 16:40 ` [PATCH 11/21] drivers: don't use GENMASK() in FIELD_PREP_WM16() Yury Norov (NVIDIA)
2025-10-27 8:49 ` Jani Nikula
@ 2025-10-27 12:21 ` Nicolas Frattaroli
2025-10-27 13:10 ` Mark Brown
2 siblings, 0 replies; 6+ messages in thread
From: Nicolas Frattaroli @ 2025-10-27 12:21 UTC (permalink / raw)
To: Linus Torvalds, Linus Walleij, Sandy Huang, Heiko Stübner,
Andy Yan, Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann,
David Airlie, Simona Vetter, Shreeya Patel, Mauro Carvalho Chehab,
Jaehoon Chung, Ulf Hansson, Nicolas Frattaroli, Liam Girdwood,
Mark Brown, Jaroslav Kysela, Takashi Iwai, Yury Norov (NVIDIA),
dri-devel, linux-arm-kernel, linux-rockchip, linux-kernel,
linux-media, kernel, linux-mmc, linux-sound, Yury Norov (NVIDIA)
Cc: Rasmus Villemoes
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 11/21] drivers: don't use GENMASK() in FIELD_PREP_WM16()
2025-10-25 16:40 ` [PATCH 11/21] drivers: don't use GENMASK() in FIELD_PREP_WM16() Yury Norov (NVIDIA)
2025-10-27 8:49 ` Jani Nikula
2025-10-27 12:21 ` Nicolas Frattaroli
@ 2025-10-27 13:10 ` Mark Brown
2 siblings, 0 replies; 6+ messages in thread
From: Mark Brown @ 2025-10-27 13:10 UTC (permalink / raw)
To: Yury Norov (NVIDIA)
Cc: Linus Torvalds, Linus Walleij, Nicolas Frattaroli, Sandy Huang,
Heiko Stübner, Andy Yan, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Simona Vetter, Shreeya Patel,
Mauro Carvalho Chehab, Jaehoon Chung, Ulf Hansson,
Nicolas Frattaroli, Liam Girdwood, Jaroslav Kysela, Takashi Iwai,
dri-devel, linux-arm-kernel, linux-rockchip, linux-kernel,
linux-media, kernel, linux-mmc, linux-sound, Rasmus Villemoes
[-- 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 --]
^ permalink raw reply [flat|nested] 6+ messages in thread