Re: [PATCH net-next v3 1/2] r8152: add helper functions for PLA/USB OCP registers
From: Simon Horman <horms@kernel.org>
Date: 2026-03-24 18:05:43
Also in:
linux-usb, lkml
This is an AI-generated review of your patch. The human sending this email has considered the AI review valid, or at least plausible. For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html --- r8152: add helper functions for PLA/USB OCP registers This commit introduces new helper functions (ocp_dword_w0w1, ocp_word_w0w1, ocp_byte_w0w1, and their _clr_bits/_set_bits variants) to simplify bitwise operations on PLA/USB OCP registers throughout the r8152 driver.
quoted hunk ↗ jump to hunk
diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c index 3b6d4252d34c..ba6dc4dbfd80 100644 --- a/drivers/net/usb/r8152.c +++ b/drivers/net/usb/r8152.c
[ ... ]
quoted hunk ↗ jump to hunk
@@ -3875,35 +3873,30 @@ static void r8153c_ups_en(struct r8152 *tp, bool enable) static void r8156_ups_en(struct r8152 *tp, bool enable) { - u32 ocp_data = ocp_read_byte(tp, MCU_TYPE_USB, USB_POWER_CUT); - if (enable) { r8156_ups_flags(tp); - ocp_data |= UPS_EN | USP_PREWAKE | PHASE2_EN; - ocp_write_byte(tp, MCU_TYPE_USB, USB_POWER_CUT, ocp_data); + ocp_byte_set_bits(tp, MCU_TYPE_USB, USB_POWER_CUT, + UPS_EN | USP_PREWAKE | PHASE2_EN); - ocp_data = ocp_read_byte(tp, MCU_TYPE_USB, USB_MISC_2); - ocp_data |= UPS_FORCE_PWR_DOWN; - ocp_write_byte(tp, MCU_TYPE_USB, USB_MISC_2, ocp_data); + ocp_byte_set_bits(tp, MCU_TYPE_USB, USB_MISC_2, + UPS_FORCE_PWR_DOWN); switch (tp->version) { case RTL_VER_13: case RTL_VER_15: - ocp_data = ocp_read_word(tp, MCU_TYPE_USB, USB_UPHY_XTAL); - ocp_data &= ~OOBS_POLLING; - ocp_write_byte(tp, MCU_TYPE_USB, USB_UPHY_XTAL, ocp_data); + ocp_word_clr_bits(tp, MCU_TYPE_USB, USB_UPHY_XTAL, + OOBS_POLLING);
^^^^ Does this actually change the hardware behavior for RTL_VER_13 and RTL_VER_15? The old code read a 16-bit word with ocp_read_word() to get OOBS_POLLING (BIT(8)), then cleared BIT(8) in the local variable, but wrote it back using ocp_write_byte(). Since ocp_write_byte() masks the data with 0xff, the write operation would only touch the low 8 bits and never actually clear BIT(8) in the hardware register. This made the old code effectively a no-op for clearing OOBS_POLLING. The new code uses ocp_word_clr_bits() which calls ocp_word_w0w1(), and that performs a proper 16-bit read-modify-write cycle with ocp_write_word(). This means BIT(8) will now actually be cleared in the hardware. While this looks like it fixes a latent bug where OOBS_POLLING was never being cleared, the commit message describes this as pure refactoring to simplify the code with no mention of a behavioral change. Can you confirm whether this behavioral change is intentional? Should the commit message document that OOBS_POLLING will now actually be cleared for RTL_VER_13/15 devices during UPS enable, when it was not being cleared before?
break;
default:
break;
}
} else {
- ocp_data &= ~(UPS_EN | USP_PREWAKE);
- ocp_write_byte(tp, MCU_TYPE_USB, USB_POWER_CUT, ocp_data);
+ ocp_byte_clr_bits(tp, MCU_TYPE_USB, USB_POWER_CUT,
+ UPS_EN | USP_PREWAKE);
- ocp_data = ocp_read_byte(tp, MCU_TYPE_USB, USB_MISC_2);
- ocp_data &= ~UPS_FORCE_PWR_DOWN;
- ocp_write_byte(tp, MCU_TYPE_USB, USB_MISC_2, ocp_data);
+ ocp_byte_clr_bits(tp, MCU_TYPE_USB, USB_MISC_2,
+ UPS_FORCE_PWR_DOWN);
if (ocp_read_word(tp, MCU_TYPE_USB, USB_MISC_0) & PCUT_STATUS) {
tp->rtl_ops.hw_phy_cfg(tp);