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

[Kalle Valo]

YanBo <dreamfly281 at gmail.com> writes:

> 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

Can you send the full output from wcn36xx, please? I want to see also
the firmware API version.

> it is possible this issue only existed on new firmware version. what
> the firmware version you used?

This is what I'm using on my mako:

wcn36xx: firmware WLAN version 'WCN v2.0 IRIS v2.0 with 48MHz XO' and CRM version 'AAAAAANAAW122023'
wcn36xx: firmware API 1.3.1.0, 12 stations, 2 bssids

>>> +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"

I'm sorry but IMHO the new function you add isn't consistent with rest
of the file. We have handlers with certain name scheme
(wcn36xx_smd_<cmd>_rsp) for all response handlers, your function doesn't
fit to that. 

It doesn't matter what the response handler actually does, it can only
check a status variable or do something more complicated. Most important
is that there's a consistent naming scheme for the functions so that
developers can always find the correct function easily. That's why it's
important to follow the style of the file.

> 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.

Sure, I understand that. But it doesn't take that long to change your
patch to use the old function again and changing your patch is much
better in the long.

-- 
Kalle Valo