Hi Denis
v2 patch sent with fixes :)
On 10.06.2020 05:11, Denis Kenzior wrote:
> Hi Marius,
>
> On 6/9/20 3:21 PM, Marius Gripsgard wrote:
>> This implements data capability bearer notify to qmi modem.
>> Since this is included in the serving system response this
>> just adds a new data extraction for dc.
>> ---
>> drivers/qmimodem/gprs.c | 29 +++++++++++++++++++++++++++++
>> drivers/qmimodem/nas.c | 36 ++++++++++++++++++++++++++++++++++++
>> drivers/qmimodem/nas.h | 23 +++++++++++++++++++++++
>> 3 files changed, 88 insertions(+)
>>
>
> Looks good, just couple of nitpicks:
>
>> diff --git a/drivers/qmimodem/gprs.c b/drivers/qmimodem/gprs.c
>> index 07adbe9a..0ba24da7 100644
>> --- a/drivers/qmimodem/gprs.c
>> +++ b/drivers/qmimodem/gprs.c
>> @@ -68,6 +68,29 @@ static bool extract_ss_info(struct qmi_result
>> *result, int *status, int *tech)
>> return true;
>> }
>> +static bool extract_dc_info(struct qmi_result *result, int
>> *bearer_tech)
>> +{
>> + const struct qmi_nas_data_capability *dc;
>> + uint16_t len;
>> + int i;
>> +
>> + DBG("");
>> +
>> + dc = qmi_result_get(result,
>> QMI_NAS_RESULT_DATA_CAPABILIT_STATUS, &len);
>
> Should this be CAPABILITY?
Yes :)
>
>> + if (!dc)
>> + return false;
>> +
>> + *bearer_tech = -1;
>> + for (i = 0; i < dc->cap_count; i++) {
>> + DBG("radio tech in use %d", dc->cap[i]);
>> +
>> + *bearer_tech = qmi_nas_cap_to_bearer_tech(dc->cap[i]);
>
> I'm fine with this, but out of curiosity: can there be multiple of
> these? Are they sorted somehow?
There can be more then one sim, but qmimodem currently does not handle
it. This is not the most pretty way of handling it, but i wanted to at
least print all of them but use the last one (as that should be the last
one in use). I also wanted to follow the same style as used above.
We could do dc->cap[cap_count-1], but would be nice to have both
extractions similar.
>
>> + }
>> +
>> + return true;
>> +}
>> +
>> +
>
> No double empty lines please
Wops, didn't see those
>
>> static void get_lte_attach_param_cb(struct qmi_result *result, void
>> *user_data)
>> {
>> struct ofono_gprs *gprs = user_data;
>> @@ -188,6 +211,7 @@ static int handle_ss_info(struct qmi_result
>> *result, struct ofono_gprs *gprs)
>> struct gprs_data *data = ofono_gprs_get_data(gprs);
>> int status;
>> int tech;
>> + int bearer_tech;
>> DBG("");
>> @@ -209,6 +233,11 @@ static int handle_ss_info(struct qmi_result
>> *result, struct ofono_gprs *gprs)
>> data->last_auto_context_id = 0;
>> }
>> + if (!extract_dc_info(result, &bearer_tech))
>> + return -1;
>
> So bearer_tech is pretty much optional, are you sure you want to
> return an error here if the extraction fails? Maybe something like:
>
> if (extract_dc_info(result, &bearer_tech)
> ofono_grps_bearer_notify(gprs, bearer_tech);
>
> would be a bit more conservative.
Yes i agree, we should still return ss return value even if bearer is
not available.
>
>> +
>> + ofono_gprs_bearer_notify(gprs, bearer_tech);
>> +
>> return status;
>> }
>>
>
> Regards,
> -Denis
> _______________________________________________
> ofono mailing list -- [email protected]
> To unsubscribe send an email to [email protected]
_______________________________________________
ofono mailing list -- [email protected]
To unsubscribe send an email to [email protected]
