Hi Joe,
Thanx for you comments. Would you mind if we fix those trivias after
wcn36xx is merged?
On Tue, Oct 8, 2013 at 9:54 PM, Joe Perches <joe@xxxxxxxxxxx> wrote:
> On Tue, 2013-10-08 at 21:25 +0100, Eugene Krasnikov wrote:
>> This is a mac80211 driver for Qualcomm WCN3660/WCN3680 devices. So
>> far WCN3660/WCN3680 is available only on MSM platform.
>
> some trivia:
>
>> diff --git a/drivers/net/wireless/ath/wcn36xx/debug.c b/drivers/net/wireless/ath/wcn36xx/debug.c
> []
>> +static ssize_t read_file_bool_bmps(struct file *file, char __user *user_buf,
>> + size_t count, loff_t *ppos)
>> +{
>> + struct wcn36xx *wcn = file->private_data;
>> + struct wcn36xx_vif *vif_priv = NULL;
>> + struct ieee80211_vif *vif = NULL;
>> + char buf[3];
>> +
>> + list_for_each_entry(vif_priv, &wcn->vif_list, list) {
>> + vif = container_of((void *)vif_priv,
>> + struct ieee80211_vif,
>> + drv_priv);
>> + if (NL80211_IFTYPE_STATION == vif->type) {
>> + if (vif_priv->pw_state == WCN36XX_BMPS)
>> + buf[0] = '1';
>> + else
>> + buf[0] = '0';
>> + break;
>> + }
>
> please indent this properly.
>
> list_for_each...(...) {
> statement...;
> statement...;
> }
>
> []
>
>> +static ssize_t write_file_bool_bmps(struct file *file,
>> + const char __user *user_buf,
>> + size_t count, loff_t *ppos)
>> +{
>> + struct wcn36xx *wcn = file->private_data;
>> + struct wcn36xx_vif *vif_priv = NULL;
>> + struct ieee80211_vif *vif = NULL;
>> +
>> + char buf[32];
>> + int buf_size;
>> +
>> + buf_size = min(count, (sizeof(buf)-1));
>> + if (copy_from_user(buf, user_buf, buf_size))
>> + return -EFAULT;
>> +
>> + switch (buf[0]) {
>> + case 'y':
>> + case 'Y':
>> + case '1':
>> + list_for_each_entry(vif_priv, &wcn->vif_list, list) {
>> + vif = container_of((void *)vif_priv,
>> + struct ieee80211_vif,
>> + drv_priv);
>> + if (NL80211_IFTYPE_STATION == vif->type) {
>
> trivia:
>
> Please use
> if (test == CONSTANT)
>
> Yes, I know the compiler complains if you
> mistakenly leave out one of the = this way.
>
>> +#define ADD_FILE(name, mode, fop, priv_data) \
>> + do { \
>> + struct dentry *d; \
>> + d = debugfs_create_file(__stringify(name), \
>> + mode, dfs->rootdir, \
>> + priv_data, fop); \
>> + dfs->file_##name.dentry = d; \
>> + if (IS_ERR(d)) { \
>> + wcn36xx_warn("Create the debugfs entry failed");\
>
> missing "\n" newline
>
>> + dfs->file_##name.dentry = NULL; \
> []
>
>> diff --git a/drivers/net/wireless/ath/wcn36xx/dxe.c b/drivers/net/wireless/ath/wcn36xx/dxe.c
> []
>> +static int wcn36xx_dxe_init_descs(struct wcn36xx_dxe_ch *wcn_ch)
>> +{
>> + struct wcn36xx_dxe_desc *cur_dxe = NULL;
>> + struct wcn36xx_dxe_desc *prev_dxe = NULL;
>> + struct wcn36xx_dxe_ctl *cur_ctl = NULL;
>> + size_t size;
>> + int i;
>> +
>> + size = wcn_ch->desc_num * sizeof(struct wcn36xx_dxe_desc);
>> + wcn_ch->cpu_addr = dma_alloc_coherent(NULL, size, &wcn_ch->dma_addr,
>> + GFP_KERNEL);
>
> dma_zalloc_coherent
>
> []
>> + memset(wcn_ch->cpu_addr, 0, size);
>
> No longer required.
>
>> + if (0 == i) {
>
> too many styles mixing i == 0 and 0 == i
>
> []
>
>> +int wcn36xx_dxe_allocate_mem_pools(struct wcn36xx *wcn)
>> +{
>> + size_t s;
>
> mixing s and size above, please use one style/name.
>
>> + void *cpu_addr;
>> +
>> + /* Allocate BD headers for MGMT frames */
>> +
>> + /* Where this come from ask QC */
>> + wcn->mgmt_mem_pool.chunk_size = WCN36XX_BD_CHUNK_SIZE +
>> + 16 - (WCN36XX_BD_CHUNK_SIZE % 8);
>> +
>> + s = wcn->mgmt_mem_pool.chunk_size * WCN36XX_DXE_CH_DESC_NUMB_TX_H;
>> + cpu_addr = dma_alloc_coherent(NULL, s, &wcn->mgmt_mem_pool.phy_addr,
>> + GFP_KERNEL);
>
> same use of dma_zalloc_coherent(...)
>
>> + if (!cpu_addr)
>> + goto out_err;
>> +
>> + wcn->mgmt_mem_pool.virt_addr = cpu_addr;
>> + memset(cpu_addr, 0, s);
>
> Now unnecessary memset.
>
> too long, stopped reading.
>
>
>
>
--
Best regards,
Eugene
--
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
