MIME-Version: 1.0
In-Reply-To: <1381265688.23020.11.camel@joe-AO722>
References: <CAB=NE6Xs=ebxpQapB4xXu7edWfMEkgZu8hTyO98zfam56og2Hg@mail.gmail.com>
	<1381263958-14846-1-git-send-email-k.eugene.e@gmail.com>
	<1381265688.23020.11.camel@joe-AO722>
Date: Wed, 9 Oct 2013 16:59:03 +0100
Message-ID: <CAFSJ42Y_QE9_ep6iArNDkUffmi-kMkU_Y8AeEN8tJXyuAZ1y4Q@mail.gmail.com> (sfid-20131009_175909_489929_BF7CEB92)
Subject: Re: [PATCH] wcn36xx: mac80211 driver for Qualcomm WCN3660/WCN3680 hardware
From: Eugene Krasnikov <k.eugene.e@gmail.com>
To: Joe Perches <joe@perches.com>
Cc: John Linville <linville@tuxdriver.com>,
	linux-wireless <linux-wireless@vger.kernel.org>,
	wcn36xx@lists.infradead.org
Content-Type: text/plain; charset=ISO-8859-1
Sender: linux-wireless-owner@vger.kernel.org

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@perches.com> 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