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

[Jes Sorensen <Jes.Sorensen@redhat.com>]

Joe Perches <joe@perches.com> writes:
> 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

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