Hi Claudiu,
On Mon, 14 May 2018 11:57:24 +0300
Claudiu Beznea <Claudiu.Beznea@xxxxxxxxxxxxx> wrote:
> Hi Ajay,
>
> On 10.05.2018 08:27, Claudiu Beznea wrote:
> >
> >
> > On 09.05.2018 21:42, Ajay Singh wrote:
> >> On Wed, 9 May 2018 16:43:14 +0300
> >> Claudiu Beznea <Claudiu.Beznea@xxxxxxxxxxxxx> wrote:
> >>
> >>> On 07.05.2018 11:43, Ajay Singh wrote:
> >>>> Fix line over 80 characters issue reported by checkpatch in
> >>>> add_network_to_shadow() by using temporary variable.
> >>>
> >>> I, personally, don't like this way of fixing line over 80. From my
> >>> point of view this introduces a new future patch. Maybe, in
> >>> future, somebody will remove this temporary variable stating that
> >>> there is no need for it.
> >>>
> >>
> >> In my opinion, just by removing this temporary variable the patch
> >> might not go through because it will definitely have line over
> >> 80 character issue. As per guideline its recommended to run the
> >> checkpatch before submitting the patch.
> >>
> >> Only using short variables names might help to resolve that issue
> >> but using short variable names will not give clear meaning for the
> >> code. I don't want to shorten the variable name as they don't
> >> convey the complete meaning.
> >>
> >> Do you have any suggestion/code which can help to understand how to
> >> resolve this without using temp/variables name changes.
> >
> > No, for this one I have not. Maybe further refactoring...
> >
>
> Looking over the v2 of this series you send, and over
> wilc_wfi_cfgoperations.c, and remembering your last question on this
> patch, I was thinking that one suggestion for this would be to
> replace last_scanned_shadow with just scanned_shadow or nw_info or
> scanned_nw_info. Just an idea.
>
I avoided use of short name for 'last_scanned_shadow' because it might
not give clear meaning as there are variables like 'last_scanned_cnt',
which also uses same prefix 'last_' and its helpful to know its related
data.
> >>
> >>>>
> >>>> Signed-off-by: Ajay Singh <ajay.kathat@xxxxxxxxxxxxx>
> >>>> ---
> >>>> drivers/staging/wilc1000/wilc_wfi_cfgoperations.c | 52
> >>>> +++++++++++------------ 1 file changed, 25 insertions(+), 27
> >>>> deletions(-)
> >>>>
> >>>> diff --git a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
> >>>> b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c index
> >>>> f1ebaea..0ae2065 100644 ---
> >>>> a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c +++
> >>>> b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c @@ -300,6
> >>>> +300,7 @@ static void add_network_to_shadow(struct network_info
> >>>> *nw_info, int ap_found = is_network_in_shadow(nw_info,
> >>>> user_void); u32 ap_index = 0; u8 rssi_index = 0;
> >>>> + struct network_info *shadow_nw_info;
> >>>>
> >>>> if (last_scanned_cnt >= MAX_NUM_SCANNED_NETWORKS_SHADOW)
> >>>> return;
> >>>> @@ -310,37 +311,34 @@ static void add_network_to_shadow(struct
> >>>> network_info *nw_info, } else {
> >>>> ap_index = ap_found;
> >>>> }
> >>>> - rssi_index =
> >>>> last_scanned_shadow[ap_index].rssi_history.index;
> >>>> -
> >>>> last_scanned_shadow[ap_index].rssi_history.samples[rssi_index++]
> >>>> = nw_info->rssi;
> >>>> + shadow_nw_info = &last_scanned_shadow[ap_index];
> >>>> + rssi_index = shadow_nw_info->rssi_history.index;
> >>>> + shadow_nw_info->rssi_history.samples[rssi_index++] =
> >>>> nw_info->rssi; if (rssi_index == NUM_RSSI) {
> >>>> rssi_index = 0;
> >>>> - last_scanned_shadow[ap_index].rssi_history.full
> >>>> = true;
> >>>> - }
> >>>> - last_scanned_shadow[ap_index].rssi_history.index =
> >>>> rssi_index;
> >>>> - last_scanned_shadow[ap_index].rssi = nw_info->rssi;
> >>>> - last_scanned_shadow[ap_index].cap_info =
> >>>> nw_info->cap_info;
> >>>> - last_scanned_shadow[ap_index].ssid_len =
> >>>> nw_info->ssid_len;
> >>>> - memcpy(last_scanned_shadow[ap_index].ssid,
> >>>> - nw_info->ssid, nw_info->ssid_len);
> >>>> - memcpy(last_scanned_shadow[ap_index].bssid,
> >>>> - nw_info->bssid, ETH_ALEN);
> >>>> - last_scanned_shadow[ap_index].beacon_period =
> >>>> nw_info->beacon_period;
> >>>> - last_scanned_shadow[ap_index].dtim_period =
> >>>> nw_info->dtim_period;
> >>>> - last_scanned_shadow[ap_index].ch = nw_info->ch;
> >>>> - last_scanned_shadow[ap_index].ies_len =
> >>>> nw_info->ies_len;
> >>>> - last_scanned_shadow[ap_index].tsf_hi = nw_info->tsf_hi;
> >>>> + shadow_nw_info->rssi_history.full = true;
> >>>> + }
> >>>> + shadow_nw_info->rssi_history.index = rssi_index;
> >>>> + shadow_nw_info->rssi = nw_info->rssi;
> >>>> + shadow_nw_info->cap_info = nw_info->cap_info;
> >>>> + shadow_nw_info->ssid_len = nw_info->ssid_len;
> >>>> + memcpy(shadow_nw_info->ssid, nw_info->ssid,
> >>>> nw_info->ssid_len);
> >>>> + memcpy(shadow_nw_info->bssid, nw_info->bssid, ETH_ALEN);
> >>>> + shadow_nw_info->beacon_period = nw_info->beacon_period;
> >>>> + shadow_nw_info->dtim_period = nw_info->dtim_period;
> >>>> + shadow_nw_info->ch = nw_info->ch;
> >>>> + shadow_nw_info->ies_len = nw_info->ies_len;
> >>>> + shadow_nw_info->tsf_hi = nw_info->tsf_hi;
> >>>> if (ap_found != -1)
> >>>> - kfree(last_scanned_shadow[ap_index].ies);
> >>>> - last_scanned_shadow[ap_index].ies =
> >>>> kmalloc(nw_info->ies_len,
> >>>> - GFP_KERNEL);
> >>>> - memcpy(last_scanned_shadow[ap_index].ies,
> >>>> - nw_info->ies, nw_info->ies_len);
> >>>> - last_scanned_shadow[ap_index].time_scan = jiffies;
> >>>> - last_scanned_shadow[ap_index].time_scan_cached =
> >>>> jiffies;
> >>>> - last_scanned_shadow[ap_index].found = 1;
> >>>> + kfree(shadow_nw_info->ies);
> >>>> + shadow_nw_info->ies = kmalloc(nw_info->ies_len,
> >>>> GFP_KERNEL);
> >>>> + memcpy(shadow_nw_info->ies, nw_info->ies,
> >>>> nw_info->ies_len);
> >>>> + shadow_nw_info->time_scan = jiffies;
> >>>> + shadow_nw_info->time_scan_cached = jiffies;
> >>>> + shadow_nw_info->found = 1;
> >>>> if (ap_found != -1)
> >>>> -
> >>>> kfree(last_scanned_shadow[ap_index].join_params);
> >>>> - last_scanned_shadow[ap_index].join_params = join_params;
> >>>> + kfree(shadow_nw_info->join_params);
> >>>> + shadow_nw_info->join_params = join_params;
> >>>> }
> >>>>
> >>>> static void cfg_scan_result(enum scan_event scan_event,
> >>>>
> >>
> >>
> >>
> >
