On 25/03/26 2:06 am, Simon Horman wrote:

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

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

@@ -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);

quoted

-
      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);

quoted

+             ocp_byte_set_bits(tp, MCU_TYPE_USB, USB_POWER_CUT,
+                               UPS_EN | USP_PREWAKE |

PHASE2_EN);

quoted

-             ocp_data = ocp_read_byte(tp, MCU_TYPE_USB,

USB_MISC_2);

quoted

-             ocp_data |= UPS_FORCE_PWR_DOWN;
-             ocp_write_byte(tp, MCU_TYPE_USB, USB_MISC_2,

ocp_data);

quoted

+             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);

quoted

-                     ocp_data &= ~OOBS_POLLING;
-                     ocp_write_byte(tp, MCU_TYPE_USB,

USB_UPHY_XTAL, ocp_data);

quoted

+                     ocp_word_clr_bits(tp, MCU_TYPE_USB,

USB_UPHY_XTAL,

quoted

+                                       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.

The OOBS_POLLING (BIT(8)) must be cleared in the USB_UPHY_XTAL register.
The old code using ocp_write_byte() is wrong.

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.

I will mention this fix in the v4 commit message.

Thanks for your review.

Best Regards,
Chih-Kai