Re: [PATCH net-next v3 1/2] r8152: add helper functions for PLA/USB OCP registers

[Simon Horman]

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.

> 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

[ ... ]

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