Joe Perches <j...@perches.com> writes:
> On Mon, 2015-03-09 at 13:00 -0400, jes.soren...@redhat.com wrote:
>> This is an alternate driver for the Realtek 8723AU (rtl8723au) written
>> from scratch utilizing the mac80211 stack.
>
> Mostly trivial comments:
>
>> diff --git a/MAINTAINERS b/MAINTAINERS
> []
>> +RTL8XXXU WIRELESS DRIVER (rtl8xxxu)
>> +M: Jes Sorensen <jes.soren...@redhat.com>
>> +L: linux-wireless@vger.kernel.org
>> +W: http://intellinuxwireless.org
>> +T: git git://git.Mkernel.org/pub/scm/linux/kernel/git/jes/linux.git
>> + git branch rtl8723au-mac80211
>
> please keep this on one line
Lines are 80 characters, and it won't fit.
> T: git git://git.kernel.org/pub/scm/linux/kernel/git/jes/linux.git
> rtl8723au-mac80211
>
>> diff --git a/drivers/net/wireless/rtl8xxxu.c
>> b/drivers/net/wireless/rtl8xxxu.c
>
> []
>
>> +static struct usb_device_id dev_table[] = {
>
> Please use const where appropriate.
>
>
>> +static struct ieee80211_rate rtl8xxxu_rates[] = {
> []
>> +static struct ieee80211_channel rtl8xxxu_channels_2g[] = {
> []
>> +static struct ieee80211_supported_band rtl8xxxu_supported_band = {
> []
>> +static struct rtl8xxxu_reg8val rtl8723a_mac_init_table[] = {
> []
>> +static struct rtl8xxxu_reg32val rtl8723a_phy_1t_init_table[] = {
> []
>> +static struct rtl8xxxu_reg32val rtl8723a_agc_1t_init_table[] = {
> []
>> +static struct rtl8xxxu_rfregval rtl8723au_radioa_rf6052_1t_init_table[] = {
>
>> +static u8 rtl8723au_read8(struct rtl8xxxu_priv *priv, u16 addr)
>
>> + if (rtl8xxxu_debug & RTL8XXXU_DEBUG_REG_READ)
>> + dev_info(&udev->dev, "%s(%04x) = 0x%02x, len %i\n",
>> + __func__, addr, data, len);
>
> Probably better to use a local rtl_dbg which use
> whatever flag you want (or no flag at all)
>
> #define rtl_dbg(dev, flag, fmt, ...)
> do {
> if (rtl8xxxu_debug & RTL8XXX_DEBUG_##flag)
> dev_dbg(dev, fmt, ##__VA_ARGS__);
> } while (0)
Ewww, absolutely not! This is way worse than how I wrote it!
> so this becomes
>
> rtl_dbg(&udev->dev, REG_REG, "%s(%04x) = 0x%02x, len %i\n",
> __func__, addr, data, len);
>
> though you could remove the __func__ here as
> dynamic_debug can add it for you.
>
>
>> +static bool rtl8xxxu_simularity_compare(struct rtl8xxxu_priv *priv,
>> + int result[][8], int c1, int c2)
>
> similarity ?
Simularity came from the vendor driver, I assume it references comparing
the radio signal quality. Unless you have evidence to the contrary, I am
not changing it.
>> +{
> []
>> + if (simubitmap == 0) {
>> + for (i = 0; i < (bound / 4); i++) {
>> + if (candidate[i] >= 0) {
>> + for (j = i * 4; j < (i + 1) * 4 - 2; j++)
>> + result[3][j] = result[candidate[i]][j];
>> + retval = false;
>> + }
>> + }
>> + return retval;
>> + } else if (!(simubitmap & 0x0f)) {
>
> Maybe remove the else
>
>> + /* path A OK */
>> + for (i = 0; i < 4; i++)
>> + result[3][i] = result[c1][i];
>> + } else if (!(simubitmap & 0xf0) && is_2t) {
>> + /* path B OK */
>> + for (i = 4; i < 8; i++)
>> + result[3][i] = result[c1][i];
>> + }
>> +
>> + return false;
>> +}
These are part of the same comparison chain, removing the else would be
inconsistent.
>> +static int rtl8xxxu_iqk_path_a(struct rtl8xxxu_priv *priv, bool configpathb)
>
> It might be helpful to describe the return
> flags here as it's not negative error
>
>> + if (!(reg_eac & BIT(28)) &&
>> + ((reg_e94 & 0x03ff0000) != 0x01420000) &&
>> + ((reg_e9c & 0x03ff0000) != 0x00420000))
>> + result |= 0x01;
>> + else /* If TX not OK, ignore RX */
>> + goto out;
>> +
>> + /* If TX is OK, check whether RX is OK */
>> + if (!(reg_eac & BIT(27)) &&
>> + ((reg_ea4 & 0x03ff0000) != 0x01320000) &&
>> + ((reg_eac & 0x03ff0000) != 0x00360000))
>> + result |= 0x02;
>> + else
>> + dev_warn(&priv->udev->dev, "%s: Path A RX IQK failed!\n",
>> + __func__);
>> +out:
>> + return result;
>> +}
>> +
>> +static void rtl8xxxu_phy_iqcalibrate(struct rtl8xxxu_priv *priv,
>> + int result[][8], int t, bool is_2t)
>> +{
>> + struct device *dev = &priv->udev->dev;
>> + u32 i, val32;
>> + int path_a_ok /*, path_b_ok */;
>> + int retry = 2;
>> +
>> + u32 ADDA_REG[RTL8XXXU_ADDA_REGS] = {
>> + REG_FPGA0_XCD_SWITCH_CTRL, REG_BLUETOOTH,
>> + REG_RX_WAIT_CCA, REG_TX_CCK_RFON,
>> + REG_TX_CCK_BBON, REG_TX_OFDM_RFON,
>> + REG_TX_OFDM_BBON, REG_TX_TO_RX,
>> + REG_TX_TO_TX, REG_RX_CCK,
>> + REG_RX_OFDM, REG_RX_WAIT_RIFS,
>> + REG_RX_TO_RX, REG_STANDBY,
>> + REG_SLEEP, REG_PMPD_ANAEN
>> + };
>
> static const is generally better.
It's irrelevant
>> +
>> + u32 IQK_MAC_REG[RTL8XXXU_MAC_REGS] = {
>> + REG_TXPAUSE, REG_BEACON_CTRL,
>> + REG_BEACON_CTRL_1, REG_GPIO_MUXCFG
>> + };
>> +
>> + u32 IQK_BB_REG_92C[RTL8XXXU_BB_REGS] = {
>> + REG_OFDM0_TRX_PATH_ENABLE, REG_OFDM0_TR_MUX_PAR,
>> + REG_FPGA0_XCD_RF_SW_CTRL, REG_CONFIG_ANT_A, REG_CONFIG_ANT_B,
>> + REG_FPGA0_XAB_RF_SW_CTRL, REG_FPGA0_XA_RF_INT_OE,
>> + REG_FPGA0_XB_RF_INT_OE, REG_FPGA0_RF_MODE
>> + };
>
> []
>
>> +static void rtl8723a_phy_iq_calibrate(struct rtl8xxxu_priv *priv,
>> bool recovery)
>> +{
> []
>> + u32 iqk_bb_reg_92c[RTL8XXXU_BB_REGS] = {
>> + REG_OFDM0_XA_RX_IQ_IMBALANCE, REG_OFDM0_XB_RX_IQ_IMBALANCE,
>> + REG_OFDM0_ENERGY_CCA_THRES, REG_OFDM0_AGCR_SSI_TABLE,
>> + REG_OFDM0_XA_TX_IQ_IMBALANCE, REG_OFDM0_XB_TX_IQ_IMBALANCE,
>> + REG_OFDM0_XC_TX_AFE, REG_OFDM0_XD_TX_AFE,
>> + REG_OFDM0_RX_IQ_EXT_ANTA
>> + };
>
> More static const?
>
>> +static int rtl8xxxu_set_bssid(struct rtl8xxxu_priv *priv, const u8 *bssid)
>> +{
>> + int i;
>> + u16 reg;
>> +
>> + dev_dbg(&priv->udev->dev,
>> + "%s: (%02x:%02x:%02x:%02x:%02x:%02x)\n",
>
> %pM
>
>> __func__,
>> + bssid[0], bssid[1], bssid[2], bssid[3], bssid[4], bssid[5]);
>
> []
>
>> +static int rtl8xxxu_init_device(struct ieee80211_hw *hw)
>> +{
>
>> + ret = rtl8xxxu_power_on(priv);
>> + if (ret < 0) {
>> + dev_warn(dev, "%s: Failed power on\n", __func__);
>
> Do you want to use _func__ on every logging message?
> If so, I suggest using custom macros to add them
> instead of adding it at every use. Something like:
I use them when I find it appropriate, your suggestion for custom macros
is ugly, and will not go in here!
> #define rtl_err(dev, fmt, ...) \
> dev_err(dev, "%s: " fmt, __func__, ##__VA_ARGS__)
> #define rtl_notice(dev, fmt, ...) \
> dev_notice(dev, "%s: " fmt, __func__, ##__VA_ARGS__)
> #define rtl_warn(dev, fmt, ...) \
> dev_warn(dev, "%s: " fmt, __func__, ##__VA_ARGS__)
> #define rtl_info(dev, fmt, ...) \
> dev_info(dev, "%s: " fmt, __func__, ##__VA_ARGS__)
>
>
>> +static void rtl8xxxu_int_complete(struct urb *urb)
>> +{
>> + struct rtl8xxxu_priv *priv = (struct rtl8xxxu_priv *)urb->context;
>> + struct device *dev = &priv->udev->dev;
>> + int i, ret;
>> +
>> + dev_dbg(dev, "%s: status %i\n", __func__, urb->status);
>> + if (urb->status == 0) {
>> + for (i = 0; i < USB_INTR_CONTENT_LENGTH; i++) {
>> + printk("%02x ", priv->int_buf[i]);
>> + if ((i & 0x0f) == 0x0f)
>> + printk("\n");
>> + }
>
> print_hex_dump();
>
>
>
>> +static int rtl8xxxu_probe(struct usb_interface *interface,
>> + const struct usb_device_id *id)
>> +{
> []
>> + dev_info(&udev->dev, "RTL8723au MAC %02x:%02x:%02x:%02x:%02x:%02x\n",
>> + priv->efuse_wifi.efuse.mac_addr[0],
>> + priv->efuse_wifi.efuse.mac_addr[1],
>> + priv->efuse_wifi.efuse.mac_addr[2],
>> + priv->efuse_wifi.efuse.mac_addr[3],
>> + priv->efuse_wifi.efuse.mac_addr[4],
>> + priv->efuse_wifi.efuse.mac_addr[5]);
>
> %pM
>
> []
>
>> +struct rtl8xxxu_priv {
> []
>> + u32 has_wifi:1;
>> + u32 has_bluetooth:1;
>> + u32 enable_bluetooth:1;
>> + u32 has_gps:1;
>> + u32 vendor_umc:1;
>> + u32 has_polarity_ctrl:1;
>> + u32 has_eeprom:1;
>> + u32 boot_eeprom:1;
>> + u32 ep_tx_high_queue:1;
>> + u32 ep_tx_normal_queue:1;
>> + u32 ep_tx_low_queue:1;
>> + u32 path_a_hi_power:1;
>
> These might be better as bool instead of packed bitfields.
bool wastes 4 bytes in the struct, so no that would be worse.
If you have any *bugs* to report, I welcome those comments very much.
However if you are just looking to nag, please do that somewhere else.
Jes
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
