RE: [PATCH net-next v6 01/10] r8152: split r8156_init per chip and factor out wait_autoload_done

[Chih Kai Hsu <hsu.chih.kai@realtek.com>]

Hi Birger,
Thanks for the review.

On Thursday, October 8, 2026, 8:18 PM, Birger wrote:
> Hi Chih Kai,
> 
> On 08/10/2026 10:15 am, Chih Kai Hsu wrote:
> > RTL8156, RTL8156B, RTL8157 and RTL8159 shared a single r8156_init(),
> > gated by a dense set of tp->version range checks. That made the init
> > sequence hard to follow.
> >
> > Split r8156_init() into one function per chip family:
> > - r8156_init  (VER_10/11)
> > - r8156b_init (VER_12/13/15)
> > - r8157_init  (VER_16)
> > - r8159_init  (VER_17)
> 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.

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

> 
> [...]
> >
> > No functional change intended.
> >
> > Signed-off-by: Chih Kai Hsu <hsu.chih.kai@realtek.com>
> Best,
>    Birger
Best,
Chih-Kai