Review comments 20130804
Eugene Krasnikov
k.eugene.e at gmail.com
Mon Aug 5 04:00:15 EDT 2013
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
More information about the wcn36xx
mailing list