[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:43 PM, Kalle Valo <kvalo at qca.qualcomm.com> wrote:
> 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.
>

FYI

<6>[  244.598779] wcn36xx: firmware WLAN version 'WCN v2.0 RadioPhy
vIris_TSMC_2.0 with 48MHz XO' and CRM version '1240107'
<6>[  244.611536] wcn36xx: firmware API 1.4.1.2, 41 stations, 2 bssids

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