Re: [PATCH net-next v3 2/3] r8152: Add support for the RTL8159 chip

On 05/05/2026 4:30 am, Jakub Kicinski 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.
---
r8152: Add support for the RTL8159 chip

This patch adds support for the RTL8159 chip as RTL_VER_17, reusing the
RTL8157 initialization and USB power management paths. The bulk of the
new code lives in r8157_hw_phy_cfg() to configure the RTL8159 PHY, and
the USB product ID 0x815a is added to the device table.

Is the leading space before ocp_byte_clr_bits here intentional?  The rest
of the switch statement uses a single tab for indentation.  checkpatch.pl
should catch this.

No, will fix in v4. Interesting, that checkpatch does not find this. I 
am very sure I ran b4 prep --check before, it is near impossible to omit 
that step before sending.

Small typo, "nomal" -> "normal".

Will fix.

quoted

+		/* close Sparse NEC, improve connect 5EUU calble performace */

                                                       ^^^^^  ^^^^^^^^^

Two typos here, "calble" -> "cable" and "performace" -> "performance".

Will fix.

Is this "will include in v10" comment a reference to an internal Realtek
source-tree revision?  It does not appear to have meaning in the upstream
tree and could be dropped or rephrased.

I will drop this.

quoted

@@ -8201,6 +8407,11 @@ static void r8156_init(struct r8152 *tp)
  			return;
  	}
  
+	if (tp->version == RTL_VER_17 && r8159_wait_backup_restore(tp)) {
+		dev_err(&tp->intf->dev, "init failed, backup-restore timed out\n");
+		return;
+	}
+

Should this path also set RTL8152_INACCESSIBLE on tp->flags, or should the
init callback be changed to return an error that probe can honor, so that
a half-initialized device is not exposed to userspace?

I'll set RTL8152_INACCESSIBLE in line with what is done already in the 
rest of r8156_init()

quoted

@@ -8274,7 +8485,10 @@ static void r8156_init(struct r8152 *tp)
  	set_bit(GREEN_ETHERNET, &tp->flags);
  
  	/* rx aggregation / 16 bytes Rx descriptor */
-	if (tp->version == RTL_VER_16)
+	if (tp->version == RTL_VER_17)
+		ocp_word_clr_bits(tp, MCU_TYPE_USB, USB_USB_CTRL,
+				  RX_AGG_DISABLE | RX_DESC_16B | BIT(11));

                                                                  ^^^^^^

What does BIT(11) in USB_USB_CTRL correspond to here, and is it
RTL8159-specific?

The commit message says the RTL8159 "reuses hardware features of the
RTL8157 (RTL_VER_16)", but this path clears an extra bit that the
RTL_VER_16 path does not.  Since there is no symbolic name or comment,
it is hard to tell whether RTL_VER_16 is missing this clear, whether
RTL_VER_17 should not be clearing it, or whether it really is an
RTL8159-only quirk.

Could a named define and/or a short comment be added to describe what
this bit does?

This was brought up before, and unfortunately, there is no documentation
whatsoever for this bit. Any guess at a name may turn out to be misleading.
But I will change the comment to read:

/* RX aggregation / 16 bytes RX descriptor
  * BIT(11) is specific to RTL8159, with unknown meaning
  */
if (tp->version == RTL_VER_17)
	ocp_word_clr_bits(tp, MCU_TYPE_USB, USB_USB_CTRL,
			  RX_AGG_DISABLE | RX_DESC_16B | BIT(11));
else if (tp->version == RTL_VER_16)
	ocp_word_clr_bits(tp, MCU_TYPE_USB, USB_USB_CTRL, RX_AGG_DISABLE | 
RX_DESC_16B);
else ...

Birger