Re: [PATCH v3] qmimodem: support wake on SMS receive

[Tom Nguyen <tomirq@earthlink.net>]

[-- Attachment #1: Type: text/plain, Size: 2162 bytes --]

Hi Denis,

>From: Denis Kenzior <denkenz@gmail.com>
>
>Hi Tom,
>
>>>> Also, it may be clearer if the introduction of raw_read and the
>>>> refactoring of this function are done as a separate commit.
>>>>
>>>
>>> Hmm, looking at the "Quectel EC21" comment again, looks like the original code was done to address a quirk (ie, bug???) with that modem. I'll revert to reading both.
>
>Jonas was the one who introduced that, so he would be the one to ask :)
>
>>>
>>> But, per spec, the 2 message TLVs are mutually exclusive, depending on the route action configuration. I'm referring to setting "new_list->route[0].action =" in get_routes_cb(). So, we shouldn't need to always read both since one is guaranteed to fail.
>>>
>>> Also, the original code, if somehow both "notify" and "message" are NULL, it'll erroneously use a null notify. I'll address this in the revert.
>>>
>> 
>> On 2nd thought, my change is still ok per that "Quectel EC21" comment. I'll keep my changes and add 
>
>Yes, it looked like it would work even for the quirky ec21, but I don't 
>have enough context to say for sure.  Nor do I have a EC21 to test
>
>comment to explain the 2 TLV that are being queried. Also, I introduced 
>raw_read because I need to pull QMI_WMS_RAW_READ out of event_notify to 
>service both event_notify and the message list. So, I'd like to keep 
>this in the same commit.
>
>Yes exactly my point.  You can have the first commit simply perform the 
>'pull QMI_WMS_RAW_READ out of event_notify' transformation, which is 
>more or less self-contained.  And then the second commit would use that 
>new utility function in your message list code.
>
>Smaller commits are easier to review.  When I mentor new contributors I 
>always encourage them to split up their code as much as possible.  In 
>this particular instance I won't insist too hard since it is likely fine 
>either way :)
>
>Regards,
>-Denis

Thank you, thank you, thank you for being lenient with me :)
I can appreciate incremental patches, will keep that in mind for future.
Now, lets see if my 4th update patch will post without problem...

Thanks,
Tom