Re: [PATCH 1/1] New driver: rtl8723au (mac80211)

[Joe Perches <joe@perches.com>]

On Mon, 2015-03-09 at 13:00 -0400, Jes.Sorensen@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.Sorensen@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

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)

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 ?

> +{
[]
> +	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;
> +}

[]

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

> +
> +	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:

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