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?

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

+       }
+
+       return true;
+}
+
+

No double empty lines please

  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.

+
+       ofono_gprs_bearer_notify(gprs, bearer_tech);
+
        return status;
  }

Regards,
-Denis
_______________________________________________
ofono mailing list -- ofono@ofono.org
To unsubscribe send an email to ofono-le...@ofono.org

Reply via email to