Re: [PATCH 00/20] BYEWORD_UPDATE: unifying (most) HIWORD_UPDATE macros

Yury Norov
2025-06-12T19:45:04Z

On Thu, Jun 12, 2025 at 08:56:02PM +0200, Nicolas Frattaroli wrote:
> This series was spawned by [1], where I was asked to move every instance
> of HIWORD_UPDATE et al that I could find to a common macro in the same
> series that I am introducing said common macro.

And it means, at least for patch #1:

Suggested-by: Yury Norov [NVIDIA] <yury.norov@gmail.com>

> The first patch of the series introduces the two new macros in
> bitfield.h, called HWORD_UPDATE and HWORD_UPDATE_CONST. The latter can
> be used in initializers.
>
> This macro definition checks that the mask fits, and that the value fits
> in the mask. Like FIELD_PREP, it also shifts the value up to the mask,
> so turning off a bit does not require using the mask as a value. Masks
> are also required to be contiguous, like with FIELD_PREP.
>
> For each definition of such a macro, the driver(s) that used it were
> evaluated for three different treatments:
> - full conversion to the new macro, for cases where replacing the
> implementation of the old macro wouldn't have worked, or where the
> conversion was trivial. These are the most complex patches in this
> series, as they sometimes have to pull apart definitions of masks
> and values due to the new semantics, which require a contiguous
> mask and shift the value for us.
> - replacing the implementation of the old macro with an instance of the
> new macro, done where I felt it made the patch much easier to review
> because I didn't want to drop a big diff on people.
> - skipping conversion entirely, usually because the mask is
> non-constant and it's not trivial to make it constant. Sometimes an
> added complication is that said non-constant mask is either used in a
> path where runtime overhead may not be desirable, or in an
> initializer.
>
> Left out of conversion:
> - drivers/mmc/host/sdhci-of-arasan.c: mask is non-constant.
> - drivers/phy/rockchip/phy-rockchip-inno-csidphy.c: mask is
> non-constant likely by way of runtime pointer dereferencing, even if
> struct and members are made const.
> - drivers/clk/rockchip/clk.h: way too many clock drivers use non-const
> masks in the context of an initializer.
>
> I will not be addressing these 3 remaining users in this series, as
> implementing a runtime checked version on top of this and verifying that
> it doesn't cause undue overhead just for 3 stragglers is a bit outside
> the scope of wanting to get my RK3576 PWM series unblocked. Please have
> mercy.
>
> In total, I count 19 different occurrences of such a macro fixed out of
> 22 I found. The vast majority of these patches have either undergone
> static testing to ensure the values end up the same during development,
> or have been verified to not break the device the driver is for at
> runtime. Only a handful are just compile-tested, and the individual
> patches remark which ones those are.
>
> This took a lot of manual work as this wasn't really something that
> could be automated: code had to be refactored to ensure masks were
> contiguous, made sense to how the hardware actually works and to human
> readers, were constant, and that the code uses unshifted values.
>
> https://lore.kernel.org/all/aD8hB-qJ4Qm6IFuS@yury/ [1]
>
> Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> ---
> Nicolas Frattaroli (20):
> bitfield: introduce HWORD_UPDATE bitfield macros
> mmc: dw_mmc-rockchip: switch to HWORD_UPDATE macro
> soc: rockchip: grf: switch to HWORD_UPDATE_CONST macro
> media: synopsys: hdmirx: replace macros with bitfield variants
> drm/rockchip: lvds: switch to HWORD_UPDATE macro
> phy: rockchip-emmc: switch to HWORD_UPDATE macro
> drm/rockchip: dsi: switch to HWORD_UPDATE* macros
> drm/rockchip: vop2: switch to HWORD_UPDATE macro
> phy: rockchip-samsung-dcphy: switch to HWORD_UPDATE macro
> drm/rockchip: dw_hdmi_qp: switch to HWORD_UPDATE macro
> drm/rockchip: inno-hdmi: switch to HWORD_UPDATE macro
> phy: rockchip-usb: switch to HWORD_UPDATE macro
> drm/rockchip: dw_hdmi: switch to HWORD_UPDATE* macros
> ASoC: rockchip: i2s-tdm: switch to HWORD_UPDATE_CONST macro
> net: stmmac: dwmac-rk: switch to HWORD_UPDATE macro
> PCI: rockchip: switch to HWORD_UPDATE* macros
> PCI: dw-rockchip: switch to HWORD_UPDATE macro
> PM / devfreq: rockchip-dfi: switch to HWORD_UPDATE macro
> clk: sp7021: switch to HWORD_UPDATE macro
> phy: rockchip-pcie: switch to HWORD_UPDATE macro
>
> drivers/clk/clk-sp7021.c | 21 +--
> drivers/devfreq/event/rockchip-dfi.c | 26 ++--
> drivers/gpu/drm/rockchip/dw-mipi-dsi-rockchip.c | 142 ++++++++++-----------
> drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c | 80 ++++++------
> drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c | 68 +++++-----
> drivers/gpu/drm/rockchip/inno_hdmi.c | 11 +-
> drivers/gpu/drm/rockchip/rockchip_drm_vop2.h | 1 -
> drivers/gpu/drm/rockchip/rockchip_lvds.h | 21 +--
> drivers/gpu/drm/rockchip/rockchip_vop2_reg.c | 14 +-
> .../media/platform/synopsys/hdmirx/snps_hdmirx.h | 5 +-
> drivers/mmc/host/dw_mmc-rockchip.c | 7 +-
> drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c | 3 +-
> drivers/pci/controller/dwc/pcie-dw-rockchip.c | 39 +++---
> drivers/pci/controller/pcie-rockchip.h | 35 ++---
> drivers/phy/rockchip/phy-rockchip-emmc.c | 3 +-
> drivers/phy/rockchip/phy-rockchip-pcie.c | 72 +++--------
> drivers/phy/rockchip/phy-rockchip-samsung-dcphy.c | 10 +-
> drivers/phy/rockchip/phy-rockchip-usb.c | 51 +++-----
> drivers/soc/rockchip/grf.c | 35 +++--
> include/linux/bitfield.h | 47 +++++++
> sound/soc/rockchip/rockchip_i2s_tdm.h | 4 +-
> 21 files changed, 342 insertions(+), 353 deletions(-)
> ---
> base-commit: d9946fe286439c2aeaa7953b8c316efe5b83d515
> change-id: 20250610-byeword-update-445c0eea771d
>
> Best regards,
> --
> Nicolas Frattaroli <nicolas.frattaroli@collabora.com>