Review comments 20130804

[Kalle Valo]

Hi,

I did a quick review of wcn36xx from upstream point of view. In this
round I only focused on architectural issues and coding style, will look
at functionality on the second round.

Here are my comments:

     static struct ieee80211_hw *private_hw;

need to handle this through platform struct or something like that


     if (!(wcn->fw_major == 1 &&
        wcn->fw_minor == 2 &&
        wcn->fw_version == 2 &&
        wcn->fw_revision == 24)) {


an inline function for checking firmware versions would be nice

   static int wcn36xx_read_mac_addresses(struct wcn36xx *wcn)

mac address needs to come from the platform struct

    /* Configuring supported rates */
    wcn->supported_rates.op_rate_mode = STA_11n;
    wcn->supported_rates.dsss_rates[0] = 0x82;
    wcn->supported_rates.dsss_rates[1] = 0x84;
    wcn->supported_rates.dsss_rates[2] = 0x8b;
    wcn->supported_rates.dsss_rates[3] = 0x96;

    wcn->supported_rates.ofdm_rates[0] = 0x0C;
    wcn->supported_rates.ofdm_rates[1] = 0x12;
    wcn->supported_rates.ofdm_rates[2] = 0x18;
    wcn->supported_rates.ofdm_rates[3] = 0x24;
    wcn->supported_rates.ofdm_rates[4] = 0x30;
    wcn->supported_rates.ofdm_rates[5] = 0x48;
    wcn->supported_rates.ofdm_rates[6] = 0x60;
    wcn->supported_rates.ofdm_rates[7] = 0x6C;

    wcn->supported_rates.supported_mcs_set[0] = 0xFF;

would be nice to document these

      if (cap < 0 || cap > 127) {
         wcn36xx_warn("error cap idx %d", cap);
                             return -EINVAL;
                             } else {

unnecessary else branch

            if (cap < 0 || cap > 127) {
               wcn36xx_warn("error cap idx %d", cap);
               } else {

with return else branch would not be needed

     if (wcn36xx_smd_rsp_status_check(buf, len))
        wcn36xx_warn("error response for caps exchange");
        return 0;

should return error?

       ADD_FILE(debug_mask, S_IRUSR | S_IWUSR,
                                  &fops_wcn36xx_debug_mask, wcn);

there's also /sys/module/wcn36xx/parameters/debug_mask, why do we need
this?

        #include <linux/wcnss_wlan.h>
        wcn->dev = wcnss_wlan_get_device();
        wcnss_memory = wcnss_wlan_get_memory_map(wcn->dev);
        wcn->tx_irq = wcnss_wlan_get_dxe_tx_irq(wcn->dev);
        wcn->rx_irq = wcnss_wlan_get_dxe_rx_irq(wcn->dev);

These are not available in upstream kernels, so this
information needs to come from the platform struct

            /* TODO make a conf file where to read this information from
            */
            /* TODO read this from config */

Upstream drivers cannot use a conffile, most likely that should come
from mac80211 or something.

     if (wait_for_completion_timeout(&wcn->smd_compl,
        msecs_to_jiffies(SMD_MSG_TIMEOUT)) <= 0) {

A temporary variable would be good for readibility

  int wcn36xx_smd_send_beacon(struct wcn36xx *wcn, struct sk_buff
  *skb_beacon,
                    u16 tim_off, u16 p2p_off) {
                        ...
                        };

Unnecessary semicolon. Also after wcn36xx_smd_update_proberesp_tmpl().

            /* // TODO need to find out why this is needed? */
            /* msg_body.beacon_length = skb_beacon->len + 6; */

No C++ style comments. Code commented out should be removed, better to
describe the problem verbally.

         wmb();

         wcn36xx_dbg(WCN36XX_DBG_DXE,
                    "wcn36xx_dxe_write_register: addr=%x, data=%x",
                                                     addr, data);

Should we have the debug message before the wmb()?

       wcn36xx_dbg(WCN36XX_DBG_DXE,
            "wcn36xx_dxe_read_register: addr=%x, data=%x",
                                            addr, *data);

                                            rmb();

And the rmb() before the debug message?

    #define RSSI0(x) (100 - ((x->phy_stat0 >> 24) & 0xff))

Using an inline function gives type checking.

-- 
Kalle Valo