Joe Perches <j...@perches.com> writes:
> On Mon, 2015-03-09 at 14:43 -0400, Jes Sorensen wrote:
>> 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.
>
> 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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
