Joe Perches <joe@xxxxxxxxxxx> writes: > On Mon, 2015-03-09 at 14:43 -0400, Jes Sorensen wrote: >> Joe Perches <joe@xxxxxxxxxxx> writes: >> > On Mon, 2015-03-09 at 13:00 -0400, Jes.Sorensen@xxxxxxxxxx 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@xxxxxxxxxx> >> >> +L: linux-wireless@xxxxxxxxxxxxxxx >> >> +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. > > For code yes, for MAINTAINERS no. > There are many lines > 80 chars there. > Please keep: > <TYPE>: entry > on single lines. Longer than 80 characters is ugly and you haven't provided any justification for putting it on one line. >> >> +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 > > Not really, it requires an initialization on entry > and would make the object code smaller to use const. As I said, it's irrelevant. The gain from this is microscopic, sure it can be done, but it's not a priority. >> >> +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. > > It's a used-once struct. 4 bytes, wow. Once per device - but it all comes down to a matter of style and taste of the developer in the end. You seem to be obsessed with imposing your ideas on everybody, rather than actually looking for real bugs. >> 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. > > Imperious much? So far you have reported one real bug (the endian issue) and a couple of trailing whitespace and messed up indentation issues. If you find more *real* issues to report, I would welcome those reports very much. However you are clearly on a mission to have the last word on every patch posted to the kernel - how about looking at real bugs or writing code instead, that would be a lot more constructive for the kernel as a whole! Jes -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
