RE: [PATCH 18/20] wifi: rtw88: Add rtw8821a.{c,h}

[Ping-Ke Shih <pkshih@realtek.com>]

Bitterblue Smith <rtl8821cerfe2@gmail.com> wrote:
> On 23/09/2024 08:47, Ping-Ke Shih wrote:
> > Bitterblue Smith <rtl8821cerfe2@gmail.com> wrote:
> >>>>>>>> +
> >>>>>>>> +const struct rtw_chip_info rtw8812a_hw_spec = {
> >>>>>>>
> >>>>>>> Is it possible moving 8812a to individual file?
> >>>>>>> Since you have rtw8812au.c and rtw8821au.c.
> >>>>>>>
> >>>>>>
> >>>>>> I think it is possible. But most of the code is common to both chips.
> >>>>>> Only the IQ calibration could be moved.
> >>>>>
> >>>>> Yep, depend on how much IQK code echo chip has.
> >>>>>
> >>>>
> >>>> The IQ calibration for RTL8812AU is about 700 lines.
> >>>
> >>> rtw8812au  -----> (a) rtw8812a
> >>>                         |
> >>>                         v
> >>>                   (b) rtw8821a_common  (hard to give a name)
> >>>                         ^
> >>>                         |
> >>> rtw8821au  -----> (c) rtw8821a
> >>>
> >>> Put all common code to (b). IQK code in (a) or (c).
> >>>
> >>> I feel you have thought above picture already. What are problems we will encounter?
> >>> Many export symbols? If so, how about below?
> >>>
> >>> rtw8812au  -----> (1) rtw8812a
> >>>     +---------+
> >>>               +-> (2) rtw8821a_common  (hard to give a name)
> >>>     +---------+
> >>> rtw8821au  -----> (3) rtw8821a
> >>>
> >>> Put rtw8812a_hw_spec and rtw8821a_hw_spec in (2). Only IQK code in (1) and (3)
> >>> respectively, and export IQK entry only. Does it work?
> >>>
> >>>
> >> For the name of the common module, I was thinking rtw88_88xxa.ko.
> >>
> >> I wonder, what is the goal? To put the code in separate kernel
> >> modules, or just separate files?
> >
> > I would like to reduce runtime memory. As I asked, how many IQK code are different
> > from them? If you have separated and compiled them, can you share size by the
> > output of 'size' command?
> >
> 
> I separated the IQK code into two files (still just one module).
> size says:
> 
>    text    data     bss     dec     hex filename
>    7192      32       0    7224    1c38 rtw8821a-iqk.o
>   12319      40       0   12359    3047 rtw8812a-iqk.o
> 
> This is x86_64.
> 
> >>
> >> I think we can have rtw88xxa.c (common code), rtw8821a.c (IQK code,
> >> rtw8821a_hw_spec, bluetooth stuff), and rtw8812a.c (IQK code,
> >> rtw8812a_hw_spec, some efuse stuff, channel switching)... if these
> >> three files compile into a single module, rtw88_88xxa.ko.
> >>
> >> If each file compiles into a module of its own, we have circular
> >> dependencies: rtw8821a_hw_spec -> common code -> IQK code.
> >> If *_hw_spec go in the common module, it still depends on both of
> >> the other two modules, so what use is it?
> >
> > If we have dependency of common code -> IQK code, we can't save runtime
> > memory, because common code reference to both IQK code. So I felt
> > dependency of IQK code would be rtw8812au --> IQK code as above second
> > diagram.
> >
> > But if the work is complicated and save not few runtime memory, we can
> > use simple design as current did.
> >
> >
> 
> The IQK code can be separated into different modules if I duplicate
> rtw8821a_ops and rtw8821a_pwr_track, and rtw8821a_phy_pwrtrack takes
> a pointer to the IQK function. Then your first diagram above can work.

Not sure the "duplicate" you meant. If it only a struct, that would be fine.
Not prefer duplicate of tables.

> 
> The tables also take up a bit of space:
> 
>   text    data     bss     dec     hex filename
>   16832       0       0   16832    41c0 rtw8821a_table.o
>   21552       0       0   21552    5430 rtw8812a_table.o

Good point. 

> 
> I don't know how many kilobytes is enough to make it worth
> creating two more modules.

I think we can list all *.o related to rtw8821a/8812a, and check the
percentage to make decisions. I mean if it occupies 50%, I will prefer
to have separated module. But I don't have an exact number now.