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

[Birger Koblitz <mail@birger-koblitz.de>]

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.

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

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


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