Re: [PATCH 06/11] ubloxmodem: add Toby L2 gprs context driver

[Denis Kenzior <denkenz@gmail.com>]

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

Hi Dragos,

 >>
>> Since this is a two line function, it would be easier to read the code if it is moved into cgact_enable_cb
>>
>> In fact, just sending CGCONTRDP inside cgact_enable_cb would be the simplest.
>
> The next patches will need this as a standalone function. Another line for reading the router mode ip config will be introduced. And the function will be called from 3 places:
> * directly in pri_context_activate (this patch)
> * after authentication command (authentication support patch)
> * from read_settings function (automatic context activation patch)
>

Okay, go ahead with your proposal.

<snip>

>>> +    gcd->cb = cb;
>>> +    gcd->cb_data = data;
>>> +    memcpy(gcd->apn, ctx->apn, sizeof(ctx->apn));
>>
>> Why do we memcpy the APN into this structure?  If you don't want to make this function too long, then just pass the apn as a parameter to ublox_activate_ctx.
>>
> The APN will be required by another patch that sends authentication command and then sends CGDCONT.
>

Okay, but then logically it should be part of that patch :)

We prefer that data structure members are added on as needed basis.  So 
it becomes much easier to understand what is used how when reviewing 
large patch sets.

This is also useful in e.g. git blame and git show when reviewing what 
was done previously and for what reason.

Regards,
-Denis