On Tue, 2018-03-27 at 10:32 +0300, Kalle Valo wrote:
> <pkshih@xxxxxxxxxxx> writes:
>
> > From: Ping-Ke Shih <pkshih@xxxxxxxxxxx>
> >
> > There are two or three physical antenna in 8822be WiFi modules, so btcoex
> > introduce two coex files to handle these two cases.
> >
> > Signed-off-by: Ping-Ke Shih <pkshih@xxxxxxxxxxx>
> > ---
> > .../realtek/rtlwifi/btcoexist/halbtc8822b1ant.c | 5327 +++++++++++++++++++
> > .../realtek/rtlwifi/btcoexist/halbtc8822b1ant.h | 413 ++
> > .../realtek/rtlwifi/btcoexist/halbtc8822b2ant.c | 5370 ++++++++++++++++++++
> > .../realtek/rtlwifi/btcoexist/halbtc8822b2ant.h | 434 ++
> > 4 files changed, 11544 insertions(+)
>
> Huge patches like this are pain to review. I'm going to split this into
> two sets, patches 1-11 and patches 12-15.
>
Do I need to split the four files into four patches?
> > +/* ************************************************************
> > + * Description:
> > + *
> > + * This file is for RTL8822B Co-exist mechanism
> > + *
> > + * History
> > + * 2012/11/15 Cosa first check in.
>
> The history feels useless to me.
>
I'll delete it.
> > + *
> > + * *************************************************************/
> > +
> > +/* ************************************************************
> > + * include files
> > + * *************************************************************/
> > +/*only for rf4ce*/
> > +#include "halbt_precomp.h"
> > +
> > +/* ************************************************************
> > + * Global variables, these are static variables
> > + * *************************************************************/
>
> Also extensive use of "******" lines is not really upstream style.
>
They're not meaningful, so I'll delete them.
> > +static struct coex_dm_8822b_1ant glcoex_dm_8822b_1ant;
> > +static struct coex_dm_8822b_1ant *coex_dm = &glcoex_dm_8822b_1ant;
> > +static struct coex_sta_8822b_1ant glcoex_sta_8822b_1ant;
> > +static struct coex_sta_8822b_1ant *coex_sta = &glcoex_sta_8822b_1ant;
> > +static struct rfe_type_8822b_1ant gl_rfe_type_8822b_1ant;
> > +static struct rfe_type_8822b_1ant *rfe_type = &gl_rfe_type_8822b_1ant;
> > +
> > +static const char *const glbt_info_src_8822b_1ant[] = {
> > + "BT Info[wifi fw]",
> > + "BT Info[bt rsp]",
> > + "BT Info[bt auto report]",
> > +};
> > +
> > +static u32 glcoex_ver_date_8822b_1ant = 20180112;
> > +static u32 glcoex_ver_8822b_1ant = 0x59;
> > +static u32 glcoex_ver_btdesired_8822b_1ant = 0x56;
>
> Having static variables like this means that this will not work if there
> are two or more device per host, right? IIRC we discussed this before,
> so what's the plan to solve that?
>
> In upstream drivers there should not be artificial limitations like one
> device per host. Is that even checked anywhere or will it just be buggy
> if there are more than one device?
>
The variables coex_dm/coex_sta/rfe_type should move to struct btcoexist, but
other btcoex files also use this style. So, my plan is to keep static variables
in this patch, and use another patch to remove all of them. Since this takes
a little time to discuss with our btcoex guys, could I send patches 12-15 first?
The version related variables are used to display in debug message, so they
work on multiple devices.
Thanks
PK
