[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 4:38 PM, Eugene Krasnikov <k.eugene.e at gmail.com> wrote:
> Hi YanBo,
>
> Could you please resend this patch with correct function naming and
> git send-email?
>

Sure, will sent it soon

BR /Yanbo
> 2013/5/24 Kalle Valo <kvalo at qca.qualcomm.com>:
>> 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
>>
>> _______________________________________________
>> wcn36xx mailing list
>> wcn36xx at lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/wcn36xx
>
>
>
> --
> Best regards,
> Eugene