Hi Denis,

Thanks for the review. I'm a bit baffled by some of the warnings you are seeing... DOS line endings? Where did those come from... I don't see them here.

Nonetheless, I'll send a quick resubmission that passes checkpatch.pl and we'll take it from there.

Thanks,
Jonas

On 13/03/2019 19:37, Denis Kenzior wrote:
Hi Jonas,

On 03/12/2019 06:09 AM, Jonas Bonn wrote:
This patch adds a call to CGMM into the modem_enable path in order to
establish the specific device model.  From this device model string, a
model-specific capabilities structure can be selected.
---
  plugins/ublox.c | 107 ++++++++++++++++++++++++++++++++++--------------
  1 file changed, 77 insertions(+), 30 deletions(-)


Have you tried running checkpatch.pl on these?  There are too many coding style violations for me to fix.

Here's a small sample:

ERROR: "foo* bar" should be "foo *bar"
#138: FILE: plugins/ublox.c:64:
+    const struct ublox_model* model;

ERROR: DOS line endings
#147: FILE: plugins/ublox.c:158:
+static void query_model_cb(gboolean ok, GAtResult *result, gpointer user_data)^M$

ERROR: DOS line endings
#149: FILE: plugins/ublox.c:160:
+^Istruct ofono_modem* modem = user_data;^M$

ERROR: "foo* bar" should be "foo *bar"
#149: FILE: plugins/ublox.c:160:
+    struct ofono_modem* modem = user_data;



diff --git a/plugins/ublox.c b/plugins/ublox.c
index 32cf1e61..efb0afbc 100644
--- a/plugins/ublox.c
+++ b/plugins/ublox.c
@@ -42,9 +42,10 @@
  #include <ofono/lte.h>
  #include <ofono/voicecall.h>
-#include <drivers/atmodem/atutil.h>
  #include <drivers/atmodem/vendor.h>
+#include <drivers/ubloxmodem/ubloxmodem.h>
+
  static const char *none_prefix[] = { NULL };
  enum supported_models {
@@ -59,6 +60,8 @@ struct ublox_data {
      GAtChat *aux;
      int model_id;
      enum ofono_vendor vendor_family;
+
+    const struct ublox_model* model;
  };
  static void ublox_debug(const char *str, void *user_data)
@@ -152,38 +155,79 @@ static void cfun_enable(gboolean ok, GAtResult *result, gpointer user_data)
      ofono_modem_set_powered(modem, TRUE);
  }
-static int ublox_enable(struct ofono_modem *modem)
+static void query_model_cb(gboolean ok, GAtResult *result, gpointer user_data)
  {
+    struct ofono_modem* modem = user_data;
      struct ublox_data *data = ofono_modem_get_data(modem);
-    const char *model_str = NULL;
+    struct ofono_error error;
+    const char *model;
+    const struct ublox_model* m;
+    const char* model_str;
-    DBG("%p", modem);
+    decode_at_error(&error, g_at_result_final_response(result));
-    model_str = ofono_modem_get_string(modem, "Model");
-    if (model_str == NULL)
-        return -EINVAL;
+    if (!ok)
+        goto fail;
-    /*
-     * Toby L2 devices are more complex and special than previously
-     * supported U-Blox devices. So they need a vendor of their own.
-     */
-    data->model_id = atoi(model_str);
-
-    data->vendor_family = OFONO_VENDOR_UBLOX;
-
-    switch (data->model_id) {
-    case SARA_G270:
-    case TOBYL2_COMPATIBLE_MODE:
-    case TOBYL2_HIGH_THROUGHPUT_MODE:
-        break;
-    case TOBYL2_MEDIUM_THROUGHPUT_MODE:
-        DBG("low/medium throughtput profile unsupported");
-        break;
-    default:
-        DBG("unknown ublox model id %d", data->model_id);
-        return -EINVAL;
+    if (at_util_parse_attr(result, "", &model) == FALSE) {
+        ofono_error("Failed to query modem model");
+        goto fail;
      }
+    m = ublox_model_from_name(model);
+    if (!m->name) {

So you just potentially crashed here

+        ofono_error("Unrecognized model: %s", model);
+        goto fail;
+    }
+
+    data->model = m;
+
+    DBG("Model: %s", data->model->name);
+
+    if (data->model->flags & UBLOX_F_TOBY_L2) {
+        model_str = ofono_modem_get_string(modem, "Model");
+        if (!model_str)
+            goto fail;
+
+        /*
+         * Toby L2 devices are more complex and special than previously
+         * supported U-Blox devices. So they need a vendor of their own.
+         */
+        data->model_id = atoi(model_str);
+

atoi is generally considered harmful.  Use strtoul instead

+        switch (data->model_id) {
+        case TOBYL2_COMPATIBLE_MODE:
+        case TOBYL2_HIGH_THROUGHPUT_MODE:
+            break;
+        case TOBYL2_MEDIUM_THROUGHPUT_MODE:
+            DBG("low/medium throughtput profile unsupported");
+            break;
+        default:
+            DBG("unknown ublox model id %d", data->model_id);
+            goto fail;
+        }
+    } else {
+        data->vendor_family = OFONO_VENDOR_UBLOX;
+    }
+
+    if (g_at_chat_send(data->aux, "AT+CFUN=4", none_prefix,
+                    cfun_enable, modem, NULL))
+        return;
+
+fail:
+    g_at_chat_unref(data->aux);
+    data->aux = NULL;
+    g_at_chat_unref(data->modem);
+    data->modem = NULL;
+    ofono_modem_set_powered(modem, FALSE);
+}
+
+static int ublox_enable(struct ofono_modem *modem)
+{
+    struct ublox_data *data = ofono_modem_get_data(modem);
+
+    DBG("%p", modem);
+
      data->aux = open_device(modem, "Aux", "Aux: ");
      /* If this is a serial modem then the device may be behind
       * the 'Device' attribute instead...
@@ -199,7 +243,6 @@ static int ublox_enable(struct ofono_modem *modem)
          g_at_chat_set_slave(data->modem, data->aux);
          g_at_chat_send(data->modem, "ATE0 +CMEE=1", none_prefix,
                          NULL, NULL, NULL);
-
          g_at_chat_send(data->modem, "AT&C0", NULL, NULL, NULL, NULL);
      }
@@ -209,10 +252,14 @@ static int ublox_enable(struct ofono_modem *modem)
      g_at_chat_send(data->aux, "ATE0 +CMEE=1", none_prefix,
                      NULL, NULL, NULL);
-    g_at_chat_send(data->aux, "AT+CFUN=4", none_prefix,
-                    cfun_enable, modem, NULL);
+    if (g_at_chat_send(data->aux, "AT+CGMM", NULL,
+                query_model_cb, modem, NULL) > 0)
+        return -EINPROGRESS;
-    return -EINPROGRESS;
+    g_at_chat_unref(data->aux);
+    data->aux = NULL;
+
+    return -EINVAL;
  }
  static void cfun_disable(gboolean ok, GAtResult *result, gpointer user_data)


Regards,
-Denis
_______________________________________________
ofono mailing list
[email protected]
https://lists.ofono.org/mailman/listinfo/ofono

Reply via email to