[PATCHV2]Remove the PNO verion bit from the update scan param response to get the correcly response status

[YanBo]

On Fri, May 24, 2013 at 3:08 PM, Kalle Valo <kvalo at qca.qualcomm.com> wrote:
> YanBo <dreamfly281 at gmail.com> writes:
>
>> The update scan param status always report error cause it contain the PNO mark,
>> remove it to get the correctly response status
>
> As I don't see this on my old mako firmware I take it that this happens
> only on newer firmware versions, right? It would be good to mention in
> the commit log the firmware version. I suspect that in the future we
> have quite a lot of work to do to handle different versions of firmware
> interfaces :/
>

The firmware I used for this test is 120158, it is possible this issue
only existed on new firmware version.
what the firmware version you used?

>> +int wcn36xx_smd_rsp_update_scan_param_status_check(void *buf, size_t len)
>> +{
>> +       struct wcn36xx_fw_msg_status_rsp * rsp;
>> +       if (len < sizeof(struct wcn36xx_hal_msg_header) +
>> +               sizeof(struct wcn36xx_fw_msg_status_rsp))
>> +               return -EIO;
>> +       rsp = (struct wcn36xx_fw_msg_status_rsp *)
>> +               (buf + sizeof(struct wcn36xx_hal_msg_header));
>> +
>> +       /* Remove the PNO version bit */
>> +       rsp->status &= (~(WCN36XX_FW_MSG_PNO_VERSION_MASK));
>> +
>> +       if (WCN36XX_FW_MSG_RESULT_SUCCESS != rsp->status) {
>> +               return -EIO;
>> +       }
>> +       return 0;
>> +}
>
> Like Eugene I don't understand why we need the new function.
>

The reason delete the wcn36xx_smd_update_scan_params  but use the
wcn36xx_smd_rsp_update_scan_param_status_check is that,
the currently wcn36xx_smd_update_scan_params_rsp do nothing but just
check the status, the name
wcn36xx_smd_rsp_update_scan_param_status_check
seem more match what it actually do and consistent the existed
function name "wcn36xx_smd_rsp_status_check", another reason is the
function has been implemented long ago
in my repo before the wcn36xx_smd_update_scan_params has been created,
and I just regenerated it with newest repo then keep the old name.

If that really make you guys confuse, it is OK to included in the new function.


>>  int wcn36xx_smd_load_nv(struct wcn36xx *wcn)
>>  {
>>         struct nv_data *nv_d;
>> @@ -298,18 +316,6 @@ int wcn36xx_smd_update_scan_params(struct wcn36xx *wcn){
>>
>>         return wcn36xx_smd_send_and_wait(wcn, msg_body.header.len);
>>  }
>> -static int wcn36xx_smd_update_scan_params_rsp(void *buf, size_t len)
>> -{
>> -       struct  wcn36xx_hal_update_scan_params_resp * rsp;
>> -
>> -       rsp = (struct wcn36xx_hal_update_scan_params_resp *)buf;
>> -
>> -       wcn36xx_dbg(WCN36XX_DBG_HAL,
>> -                   "hal rsp update scan params status %d",
>> -                   rsp->status);
>> -
>> -       return 0;
>> -}
>
> You could do the same just in this function.
>
>> @@ -901,7 +907,10 @@ static void wcn36xx_smd_rsp_process(struct
>> wcn36xx *wcn, void *buf, si
>>                                  ze_t len)
>>                 wcn36xx_smd_join_rsp(buf, len);
>>                 break;
>>         case WCN36XX_HAL_UPDATE_SCAN_PARAM_RSP:
>> -               wcn36xx_smd_update_scan_params_rsp(buf, len);
>> +               if(wcn36xx_smd_rsp_update_scan_param_status_check(buf, len)) {
>> +                       wcn36xx_warn("error response from hal request %d",
>> +                                    msg_header->msg_type);
>> +               }
>>                 break;
>
> That way we can keep this part simple.
>
>>         case WCN36XX_HAL_CH_SWITCH_RSP:
>>                 wcn36xx_smd_switch_channel_rsp(buf,len);
>> diff --git a/smd.h b/smd.h
>> index 47a6645..9703e19 100644
>> --- a/smd.h
>> +++ b/smd.h
>> @@ -27,6 +27,9 @@
>>  #define SMD_MSG_TIMEOUT 200
>>  #define WCN36XX_SMSM_WLAN_TX_ENABLE                    0x00000400
>>  #define WCN36XX_SMSM_WLAN_TX_RINGS_EMPTY               0x00000200
>> +/* The PNO version info be contained in the rsp msg */
>> +#define WCN36XX_FW_MSG_PNO_VERSION_MASK                        0X8000
>
> "0x8000" (lower case)
Yes that should be more consistent
>
>> +
>>
>>  enum wcn36xx_fw_msg_result {
>>         WCN36XX_FW_MSG_RESULT_SUCCESS                   = 0,
>> @@ -54,7 +57,6 @@ int wcn36xx_smd_init_scan(struct wcn36xx *wcn);
>>  int wcn36xx_smd_start_scan(struct wcn36xx *wcn, int ch);
>>  int wcn36xx_smd_end_scan(struct wcn36xx *wcn, int ch);
>>  int wcn36xx_smd_finish_scan(struct wcn36xx *wcn);
>> -int wcn36xx_smd_update_scan_params(struct wcn36xx *wcn);
>
> Why this?
>
Good catch, OOH, the is a misoperation when I regenerated the patch on
the new repo, thanks.

BR /Yanbo