Review comments 20130804

[Eugene Krasnikov]

Fixed the firs part of comments
https://github.com/KrasnikovEugene/wcn36xx/pull/100
Now will focus on implementing platform interface.

2013/8/5 Eugene Krasnikov <k.eugene.e at gmail.com>:
> Hi Kalle,
>
> Thank you very much for review. Let me first fix the most easiest one;)
>
> 2013/8/4 Kalle Valo <kvalo at qca.qualcomm.com>:
>> 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
>>
>> _______________________________________________
>> wcn36xx mailing list
>> wcn36xx at lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/wcn36xx
>
>
>
> --
> Best regards,
> Eugene



-- 
Best regards,
Eugene