Re: [PATCH net-next v6 01/10] r8152: split r8156_init per chip and factor out wait_autoload_done
From: Birger Koblitz
Date: Mon Oct 12 2026 - 06:08:25 EST
Hi Chi-Kai,
On 12/10/2026 11:24 am, Chih Kai Hsu wrote:
Hi Birger,Thanks a lot for looking into this! I think we agree on that maintainability of the code is a very important quality. Where you believe that using per-chip init functions is necessary for that, please follow that road, striking a balance between as many as necessary and as few as possible. Maybe look int the different generations of chips to see whether there are better ways to combine or split the init_functions() than is the case now.
I'm sorry for making the previous reply hard to read, so I resent this.
On Thursday, October 8, 2026, 8:18 PM, Birger wrote:
Hi Chih Kai,
I have a hard time to understand why you want to separate out one init
function into 4 different ones, for each chip one. Especially, if, as you say no
functional changes are intended, which might justify the re-factoring in this
manner. By multiplying the functions, it will become more difficult to maintain
the code: if an issue is found in the init-sequence, it may now be necessary to
fix it 4-times and future developments touching the init-sequence will also
need to be replicated multiple times. In total, there would afterwards be 8(!)
different init sequences including the other ones for the RTL8152 and different
versions of the RTL8153. The same pattern repeats with _up(), _down() and
other configuration functions.
The split is meant to make each function readable on its own. In a single shared
function, every chip difference is a tp->version range check, and the function is
hard to follow. Your concern is valid: if a fix needs to be applied to the four chips,
it will need to be applied four times. But if a chip gets chip-specific fixes, the init
or other functions will be more complex with several tp->version checks.
On the other hand, using a single shared function will make the function very
complex when new ICs are added.
That sounds like a good plan, it would be great to reduce the number of these little helper functions being used, where suitable. It would greatly improve readability.
Would it not make more sense to try to further combine the init sequences or
try to disentangle the code by establishing common library routines put into a
_lib.c file and possibly different .c files for the different generations of the chips?
For example, it appears that the
rtl8157/9 belong to one generation of chips, based on the rtl8156 design. On
the other hand, rtl8152/3 appear to be a different generation with various
family members. That would allow to build a basis for future development like
to rely on using phylink to handle the PHYs integrated in the chips, which all
appear to be already supported by the Realtek-PHY driver.
I agree that chips with the same generation or very similar sequences should be combined.
I prefer to combine the callbacks, including init, up, down, and enable.
- For init, I can merge r8156b_init into r8156_init and r8159_init into r8157_init.
- For up/down, I can move the sequence shared by rtl8156_up/rtl8157_up and by the down
functions into common helpers, as rtl8152b/3 do with exit_oob/enter_oob.
- For enable, I can combine rtl8156_enable and rtl8156b_enable.
Best,
Birger