On Fri, 2018-03-30 at 21:17 +0200, Johannes Berg wrote: > On Fri, 2018-03-30 at 15:19 +0800, pkshih@xxxxxxxxxxx wrote: > > > > +static struct rtl_halmac_ops rtl_halmac_operation = { > > You should make this const, if at all possible (it looks like it should > be). > I'll do it. > > + .halmac_init_adapter = rtl_halmac_init_adapter, > > + .halmac_deinit_adapter = rtl_halmac_deinit_adapter, > > + .halmac_init_hal = rtl_halmac_init_hal, > > + .halmac_deinit_hal = rtl_halmac_deinit_hal, > > + .halmac_poweron = rtl_halmac_poweron, > > + .halmac_poweroff = rtl_halmac_poweroff, > > + > > + .halmac_phy_power_switch = rtl_halmac_phy_power_switch, > > + .halmac_set_mac_address = rtl_halmac_set_mac_address, > > + .halmac_set_bssid = rtl_halmac_set_bssid, > > + > > + .halmac_get_physical_efuse_size = rtl_halmac_get_physical_efuse_size, > > + .halmac_read_physical_efuse_map = rtl_halmac_read_physical_efuse_map, > > + .halmac_get_logical_efuse_size = rtl_halmac_get_logical_efuse_size, > > + .halmac_read_logical_efuse_map = rtl_halmac_read_logical_efuse_map, > > + > > + .halmac_set_bandwidth = rtl_halmac_set_bandwidth, > > + > > + .halmac_c2h_handle = rtl_halmac_c2h_handle, > > + > > + .halmac_chk_txdesc = rtl_halmac_chk_txdesc, > > + .halmac_iqk = rtl_halmac_iqk, > > +}; > > + > > +struct rtl_halmac_ops *rtl_halmac_get_ops_pointer(void) > > +{ > > + return &rtl_halmac_operation; > > +} > > +EXPORT_SYMBOL(rtl_halmac_get_ops_pointer); > > This seems like a rather pointless indirection, you can export the > variable. > I use the indirection because another existing module btcoex uses this style. I may also change the existing code. Thanks for your review. PK