[PATCH qmi 0/5] QMI patches

[PATCH qmi 0/5] QMI patches

[Jonas Bonn]

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

This is a series of cleanups related to QMI modems.

You might want to consider patch 5 as an object for review more than
submission.  It's what I'm currently using with my UC21.

Jonas Bonn (5):
  qmimodem: fix typo
  qmimodem: add WDA service string
  gobi: query presence of WDA service
  gobi: take LLP-matching code from qmimodem
  udevng: add qmiwwan modem driver

 drivers/qmimodem/gprs-context.c | 74 +--------------------------------------
 drivers/qmimodem/qmi.c          |  4 ++-
 plugins/gobi.c                  | 77 +++++++++++++++++++++++++++++++++++++++++
 plugins/udevng.c                | 41 ++++++++++++++++++++++
 4 files changed, 122 insertions(+), 74 deletions(-)

-- 
2.9.3

[PATCH qmi 1/5] qmimodem: fix typo

[Jonas Bonn]

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

---
 drivers/qmimodem/qmi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/qmimodem/qmi.c b/drivers/qmimodem/qmi.c
index 8a9a88d..e50da19 100644
--- a/drivers/qmimodem/qmi.c
+++ b/drivers/qmimodem/qmi.c
@@ -330,7 +330,7 @@ static const char *__service_type_to_string(uint8_t type)
 	case QMI_SERVICE_TS:
 		return "TS";
 	case QMI_SERVICE_TMD:
-		return "TMS";
+		return "TMD";
 	case QMI_SERVICE_PDC:
 		return "PDC";
 	case QMI_SERVICE_CAT_OLD:
-- 
2.9.3

[PATCH qmi 2/5] qmimodem: add WDA service string

[Jonas Bonn]

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

---
 drivers/qmimodem/qmi.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/qmimodem/qmi.c b/drivers/qmimodem/qmi.c
index e50da19..39fbb19 100644
--- a/drivers/qmimodem/qmi.c
+++ b/drivers/qmimodem/qmi.c
@@ -331,6 +331,8 @@ static const char *__service_type_to_string(uint8_t type)
 		return "TS";
 	case QMI_SERVICE_TMD:
 		return "TMD";
+	case QMI_SERVICE_WDA:
+		return "WDA";
 	case QMI_SERVICE_PDC:
 		return "PDC";
 	case QMI_SERVICE_CAT_OLD:
-- 
2.9.3

[PATCH qmi 3/5] gobi: query presence of WDA service

[Jonas Bonn]

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

---
 plugins/gobi.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/plugins/gobi.c b/plugins/gobi.c
index 06f906d..df35f94 100644
--- a/plugins/gobi.c
+++ b/plugins/gobi.c
@@ -48,6 +48,7 @@
 
 #include <drivers/qmimodem/qmi.h>
 #include <drivers/qmimodem/dms.h>
+#include <drivers/qmimodem/wda.h>
 #include <drivers/qmimodem/util.h>
 
 #define GOBI_DMS	(1 << 0)
@@ -60,6 +61,7 @@
 #define GOBI_CAT	(1 << 7)
 #define GOBI_CAT_OLD	(1 << 8)
 #define GOBI_VOICE	(1 << 9)
+#define GOBI_WDA	(1 << 10)
 
 struct gobi_data {
 	struct qmi_device *device;
@@ -275,6 +277,9 @@ static void discover_cb(uint8_t count, const struct qmi_version *list,
 		case QMI_SERVICE_WDS:
 			data->features |= GOBI_WDS;
 			break;
+		case QMI_SERVICE_WDA:
+			data->features |= GOBI_WDA;
+			break;
 		case QMI_SERVICE_PDS:
 			data->features |= GOBI_PDS;
 			break;
-- 
2.9.3

[PATCH qmi 4/5] gobi: take LLP-matching code from qmimodem

[Jonas Bonn]

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

The gobi plugin is a top-level construct that steers the creation
of other constructs based on modem capabilities.  The existence of
the WDA QMI service should thus be used to decide whether to attempt
to set the link-layer protocol based on the device configuration;
this code therefore belongs in the gobi plugin instead of in the
gprs-context code which is otherwise agnostic of any link-layer
settings.

This patch, therefore, moves the code for querying the WDA service
and setting the link-layer protocol into the gobi driver.  The only
small change made with respect the original is a de-coupling of
service creation and setting of the link layer.
---
 drivers/qmimodem/gprs-context.c | 74 +----------------------------------------
 plugins/gobi.c                  | 72 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 73 insertions(+), 73 deletions(-)

diff --git a/drivers/qmimodem/gprs-context.c b/drivers/qmimodem/gprs-context.c
index da2be24..ab9f005 100644
--- a/drivers/qmimodem/gprs-context.c
+++ b/drivers/qmimodem/gprs-context.c
@@ -30,15 +30,12 @@
 #include <ofono/gprs-context.h>
 
 #include "qmi.h"
-#include "wda.h"
 #include "wds.h"
 
 #include "qmimodem.h"
 
 struct gprs_context_data {
 	struct qmi_service *wds;
-	struct qmi_service *wda;
-	struct qmi_device *dev;
 	unsigned int active_context;
 	uint32_t pkt_handle;
 };
@@ -290,69 +287,6 @@ static void create_wds_cb(struct qmi_service *service, void *user_data)
 					pkt_status_notify, gc, NULL);
 }
 
-static void get_data_format_cb(struct qmi_result *result, void *user_data)
-{
-	struct ofono_gprs_context *gc = user_data;
-	struct gprs_context_data *data = ofono_gprs_context_get_data(gc);
-	uint32_t llproto;
-	enum qmi_device_expected_data_format expected_llproto;
-
-	DBG("");
-
-	if (qmi_result_set_error(result, NULL))
-		goto done;
-
-	if (!qmi_result_get_uint32(result, QMI_WDA_LL_PROTOCOL, &llproto))
-		goto done;
-
-	expected_llproto = qmi_device_get_expected_data_format(data->dev);
-
-	if ((llproto == QMI_WDA_DATA_LINK_PROTOCOL_802_3) &&
-			(expected_llproto ==
-				QMI_DEVICE_EXPECTED_DATA_FORMAT_RAW_IP)) {
-		if (!qmi_device_set_expected_data_format(data->dev,
-					QMI_DEVICE_EXPECTED_DATA_FORMAT_802_3))
-			DBG("Fail to set expected data to 802.3");
-		else
-			DBG("expected data set to 802.3");
-	} else if ((llproto == QMI_WDA_DATA_LINK_PROTOCOL_RAW_IP) &&
-			(expected_llproto ==
-				QMI_DEVICE_EXPECTED_DATA_FORMAT_802_3)) {
-		if (!qmi_device_set_expected_data_format(data->dev,
-					QMI_DEVICE_EXPECTED_DATA_FORMAT_RAW_IP))
-			DBG("Fail to set expected data to raw-ip");
-		else
-			DBG("expected data set to raw-ip");
-	}
-
-done:
-	qmi_service_create(data->dev, QMI_SERVICE_WDS, create_wds_cb, gc,
-									NULL);
-}
-
-static void create_wda_cb(struct qmi_service *service, void *user_data)
-{
-	struct ofono_gprs_context *gc = user_data;
-	struct gprs_context_data *data = ofono_gprs_context_get_data(gc);
-
-	DBG("");
-
-	if (!service) {
-		DBG("Failed to request WDA service, continue initialization");
-		goto error;
-	}
-
-	data->wda = qmi_service_ref(service);
-
-	if (qmi_service_send(data->wda, QMI_WDA_GET_DATA_FORMAT, NULL,
-					get_data_format_cb, gc, NULL) > 0)
-		return;
-
-error:
-	qmi_service_create(data->dev, QMI_SERVICE_WDS, create_wds_cb, gc,
-									NULL);
-}
-
 static int qmi_gprs_context_probe(struct ofono_gprs_context *gc,
 					unsigned int vendor, void *user_data)
 {
@@ -364,9 +298,8 @@ static int qmi_gprs_context_probe(struct ofono_gprs_context *gc,
 	data = g_new0(struct gprs_context_data, 1);
 
 	ofono_gprs_context_set_data(gc, data);
-	data->dev = device;
 
-	qmi_service_create(device, QMI_SERVICE_WDA, create_wda_cb, gc, NULL);
+	qmi_service_create(device, QMI_SERVICE_WDS, create_wds_cb, gc, NULL);
 
 	return 0;
 }
@@ -384,11 +317,6 @@ static void qmi_gprs_context_remove(struct ofono_gprs_context *gc)
 		qmi_service_unref(data->wds);
 	}
 
-	if (data->wda) {
-		qmi_service_unregister_all(data->wda);
-		qmi_service_unref(data->wda);
-	}
-
 	g_free(data);
 }
 
diff --git a/plugins/gobi.c b/plugins/gobi.c
index df35f94..d160d58 100644
--- a/plugins/gobi.c
+++ b/plugins/gobi.c
@@ -66,6 +66,7 @@
 struct gobi_data {
 	struct qmi_device *device;
 	struct qmi_service *dms;
+	struct qmi_service *wda;
 	unsigned long features;
 	unsigned int discover_attempts;
 	uint8_t oper_mode;
@@ -252,6 +253,21 @@ error:
 	shutdown_device(modem);
 }
 
+static void create_wda_cb(struct qmi_service *service, void *user_data)
+{
+	struct ofono_modem *modem = user_data;
+	struct gobi_data *data = ofono_modem_get_data(modem);
+
+	DBG("");
+
+	if (!service) {
+		DBG("Failed to request WDA service, continue initialization");
+		return;
+	}
+
+	data->wda = qmi_service_ref(service);
+}
+
 static void discover_cb(uint8_t count, const struct qmi_version *list,
 							void *user_data)
 {
@@ -313,6 +329,10 @@ static void discover_cb(uint8_t count, const struct qmi_version *list,
 		return;
 	}
 
+	if (data->features & GOBI_WDA)
+		qmi_service_create(data->device, QMI_SERVICE_WDA,
+						create_wda_cb, modem, NULL);
+
 	qmi_service_create_shared(data->device, QMI_SERVICE_DMS,
 						create_dms_cb, modem, NULL);
 }
@@ -368,6 +388,11 @@ static int gobi_disable(struct ofono_modem *modem)
 	qmi_service_cancel_all(data->dms);
 	qmi_service_unregister_all(data->dms);
 
+	if (data->wda) {
+		qmi_service_unregister_all(data->wda);
+		qmi_service_unref(data->wda);
+	}
+
 	/*
 	 * Telit QMI modem must remain online. If powered down, it also
 	 * powers down the sim card, and QMI interface has no way to bring
@@ -484,14 +509,61 @@ static void gobi_post_sim(struct ofono_modem *modem)
 		ofono_sms_create(modem, 0, "qmimodem", data->device);
 }
 
+static void get_data_format_cb(struct qmi_result *result, void *user_data)
+{
+	struct ofono_modem* modem = user_data;
+	struct gobi_data *data = ofono_modem_get_data(modem);
+	uint32_t llproto;
+	enum qmi_device_expected_data_format expected_llproto;
+
+	DBG("");
+
+	if (qmi_result_set_error(result, NULL))
+		return;
+
+	if (!qmi_result_get_uint32(result, QMI_WDA_LL_PROTOCOL, &llproto))
+		return;
+
+	expected_llproto = qmi_device_get_expected_data_format(data->device);
+
+	if ((llproto == QMI_WDA_DATA_LINK_PROTOCOL_802_3) &&
+			(expected_llproto ==
+				QMI_DEVICE_EXPECTED_DATA_FORMAT_RAW_IP)) {
+		if (!qmi_device_set_expected_data_format(data->device,
+					QMI_DEVICE_EXPECTED_DATA_FORMAT_802_3))
+			DBG("Fail to set expected data to 802.3");
+		else
+			DBG("expected data set to 802.3");
+	} else if ((llproto == QMI_WDA_DATA_LINK_PROTOCOL_RAW_IP) &&
+			(expected_llproto ==
+				QMI_DEVICE_EXPECTED_DATA_FORMAT_802_3)) {
+		if (!qmi_device_set_expected_data_format(data->device,
+					QMI_DEVICE_EXPECTED_DATA_FORMAT_RAW_IP))
+			DBG("Fail to set expected data to raw-ip");
+		else
+			DBG("expected data set to raw-ip");
+	}
+}
+
 static void gobi_post_online(struct ofono_modem *modem)
 {
 	struct gobi_data *data = ofono_modem_get_data(modem);
 	struct ofono_gprs *gprs;
 	struct ofono_gprs_context *gc;
+	int err;
 
 	DBG("%p", modem);
 
+	if (data->features & GOBI_WDA) {
+		/* Query device link-layer protocol */
+		err = qmi_service_send(data->wda,
+					QMI_WDA_GET_DATA_FORMAT, NULL,
+					get_data_format_cb, modem, NULL);
+		if (err) {
+			DBG("Failed to query link layer protocol");
+		}
+	}
+
 	if (data->features & GOBI_NAS)
 		ofono_netreg_create(modem, 0, "qmimodem", data->device);
 
-- 
2.9.3

[PATCH qmi 5/5] udevng: add qmiwwan modem driver

[Jonas Bonn]

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

This can hopefully be a generic QMI modem setup function.  The channels
need to be labelled by udev.
---
 plugins/udevng.c | 41 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 41 insertions(+)

diff --git a/plugins/udevng.c b/plugins/udevng.c
index 2279bbe..0b9c341 100644
--- a/plugins/udevng.c
+++ b/plugins/udevng.c
@@ -177,6 +177,46 @@ static gboolean setup_hso(struct modem_info *modem)
 	return TRUE;
 }
 
+static gboolean setup_qmiwwan(struct modem_info *modem)
+{
+	const char *qmi = NULL, *net = NULL;
+	const char *gps = NULL;
+	GSList *list;
+
+	DBG("%s", modem->syspath);
+
+	for (list = modem->devices; list; list = list->next) {
+		struct device_info *info = list->data;
+
+		DBG("%s %s %s %s %s", info->devnode, info->interface,
+				info->number, info->label, info->subsystem);
+
+		if (!g_strcmp0(info->label, "qmi")) {
+			if (!g_strcmp0(info->subsystem, "usbmisc"))
+					qmi = info->devnode;
+			else if (!g_strcmp0(info->subsystem, "net"))
+					net = info->devnode;
+		} else if (!g_strcmp0(info->label, "gps")) {
+			gps = info->devnode;
+		}
+	}
+
+	if (qmi == NULL || net == NULL)
+		return FALSE;
+
+	DBG("qmi=%s net=%s gps=%s", qmi, net, gps);
+
+	ofono_modem_set_driver(modem->modem, "gobi");
+
+	ofono_modem_set_string(modem->modem, "Device", qmi);
+	ofono_modem_set_string(modem->modem, "NetworkInterface", net);
+	if (gps)
+		ofono_modem_set_string(modem->modem, "GPS", gps);
+
+	return TRUE;
+}
+
+
 static gboolean setup_gobi(struct modem_info *modem)
 {
 	const char *qmi = NULL, *mdm = NULL, *net = NULL;
@@ -941,6 +981,7 @@ static struct {
 	{ "isiusb",	setup_isi,	"type"			},
 	{ "mbm",	setup_mbm,	"device/interface"	},
 	{ "hso",	setup_hso,	"hsotype"		},
+	{ "qmiwwan",	setup_qmiwwan	},
 	{ "gobi",	setup_gobi	},
 	{ "sierra",	setup_sierra	},
 	{ "option",	setup_option	},
-- 
2.9.3

Re: [PATCH qmi 4/5] gobi: take LLP-matching code from qmimodem

[Denis Kenzior]

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

Hi Jonas,

On 03/25/2017 11:57 AM, Jonas Bonn wrote:
> The gobi plugin is a top-level construct that steers the creation
> of other constructs based on modem capabilities.  The existence of
> the WDA QMI service should thus be used to decide whether to attempt
> to set the link-layer protocol based on the device configuration;
> this code therefore belongs in the gobi plugin instead of in the
> gprs-context code which is otherwise agnostic of any link-layer
> settings.
>
> This patch, therefore, moves the code for querying the WDA service
> and setting the link-layer protocol into the gobi driver.  The only
> small change made with respect the original is a de-coupling of
> service creation and setting of the link layer.

Christophe added this recently, so he might want to comment...

> ---
>  drivers/qmimodem/gprs-context.c | 74 +----------------------------------------
>  plugins/gobi.c                  | 72 +++++++++++++++++++++++++++++++++++++++
>  2 files changed, 73 insertions(+), 73 deletions(-)
>
> diff --git a/drivers/qmimodem/gprs-context.c b/drivers/qmimodem/gprs-context.c
> index da2be24..ab9f005 100644
> --- a/drivers/qmimodem/gprs-context.c
> +++ b/drivers/qmimodem/gprs-context.c
> @@ -30,15 +30,12 @@
>  #include <ofono/gprs-context.h>
>
>  #include "qmi.h"
> -#include "wda.h"
>  #include "wds.h"
>
>  #include "qmimodem.h"
>
>  struct gprs_context_data {
>  	struct qmi_service *wds;
> -	struct qmi_service *wda;
> -	struct qmi_device *dev;
>  	unsigned int active_context;
>  	uint32_t pkt_handle;
>  };
> @@ -290,69 +287,6 @@ static void create_wds_cb(struct qmi_service *service, void *user_data)
>  					pkt_status_notify, gc, NULL);
>  }
>
> -static void get_data_format_cb(struct qmi_result *result, void *user_data)
> -{
> -	struct ofono_gprs_context *gc = user_data;
> -	struct gprs_context_data *data = ofono_gprs_context_get_data(gc);
> -	uint32_t llproto;
> -	enum qmi_device_expected_data_format expected_llproto;
> -
> -	DBG("");
> -
> -	if (qmi_result_set_error(result, NULL))
> -		goto done;
> -
> -	if (!qmi_result_get_uint32(result, QMI_WDA_LL_PROTOCOL, &llproto))
> -		goto done;
> -
> -	expected_llproto = qmi_device_get_expected_data_format(data->dev);
> -
> -	if ((llproto == QMI_WDA_DATA_LINK_PROTOCOL_802_3) &&
> -			(expected_llproto ==
> -				QMI_DEVICE_EXPECTED_DATA_FORMAT_RAW_IP)) {
> -		if (!qmi_device_set_expected_data_format(data->dev,
> -					QMI_DEVICE_EXPECTED_DATA_FORMAT_802_3))
> -			DBG("Fail to set expected data to 802.3");
> -		else
> -			DBG("expected data set to 802.3");
> -	} else if ((llproto == QMI_WDA_DATA_LINK_PROTOCOL_RAW_IP) &&
> -			(expected_llproto ==
> -				QMI_DEVICE_EXPECTED_DATA_FORMAT_802_3)) {
> -		if (!qmi_device_set_expected_data_format(data->dev,
> -					QMI_DEVICE_EXPECTED_DATA_FORMAT_RAW_IP))
> -			DBG("Fail to set expected data to raw-ip");
> -		else
> -			DBG("expected data set to raw-ip");
> -	}
> -
> -done:
> -	qmi_service_create(data->dev, QMI_SERVICE_WDS, create_wds_cb, gc,
> -									NULL);
> -}
> -
> -static void create_wda_cb(struct qmi_service *service, void *user_data)
> -{
> -	struct ofono_gprs_context *gc = user_data;
> -	struct gprs_context_data *data = ofono_gprs_context_get_data(gc);
> -
> -	DBG("");
> -
> -	if (!service) {
> -		DBG("Failed to request WDA service, continue initialization");
> -		goto error;
> -	}
> -
> -	data->wda = qmi_service_ref(service);
> -
> -	if (qmi_service_send(data->wda, QMI_WDA_GET_DATA_FORMAT, NULL,
> -					get_data_format_cb, gc, NULL) > 0)
> -		return;
> -
> -error:
> -	qmi_service_create(data->dev, QMI_SERVICE_WDS, create_wds_cb, gc,
> -									NULL);
> -}
> -
>  static int qmi_gprs_context_probe(struct ofono_gprs_context *gc,
>  					unsigned int vendor, void *user_data)
>  {
> @@ -364,9 +298,8 @@ static int qmi_gprs_context_probe(struct ofono_gprs_context *gc,
>  	data = g_new0(struct gprs_context_data, 1);
>
>  	ofono_gprs_context_set_data(gc, data);
> -	data->dev = device;
>
> -	qmi_service_create(device, QMI_SERVICE_WDA, create_wda_cb, gc, NULL);
> +	qmi_service_create(device, QMI_SERVICE_WDS, create_wds_cb, gc, NULL);
>
>  	return 0;
>  }
> @@ -384,11 +317,6 @@ static void qmi_gprs_context_remove(struct ofono_gprs_context *gc)
>  		qmi_service_unref(data->wds);
>  	}
>
> -	if (data->wda) {
> -		qmi_service_unregister_all(data->wda);
> -		qmi_service_unref(data->wda);
> -	}
> -
>  	g_free(data);
>  }
>
> diff --git a/plugins/gobi.c b/plugins/gobi.c
> index df35f94..d160d58 100644
> --- a/plugins/gobi.c
> +++ b/plugins/gobi.c
> @@ -66,6 +66,7 @@
>  struct gobi_data {
>  	struct qmi_device *device;
>  	struct qmi_service *dms;
> +	struct qmi_service *wda;
>  	unsigned long features;
>  	unsigned int discover_attempts;
>  	uint8_t oper_mode;
> @@ -252,6 +253,21 @@ error:
>  	shutdown_device(modem);
>  }
>
> +static void create_wda_cb(struct qmi_service *service, void *user_data)
> +{
> +	struct ofono_modem *modem = user_data;
> +	struct gobi_data *data = ofono_modem_get_data(modem);
> +
> +	DBG("");
> +
> +	if (!service) {
> +		DBG("Failed to request WDA service, continue initialization");
> +		return;
> +	}
> +
> +	data->wda = qmi_service_ref(service);
> +}
> +
>  static void discover_cb(uint8_t count, const struct qmi_version *list,
>  							void *user_data)
>  {
> @@ -313,6 +329,10 @@ static void discover_cb(uint8_t count, const struct qmi_version *list,
>  		return;
>  	}
>
> +	if (data->features & GOBI_WDA)
> +		qmi_service_create(data->device, QMI_SERVICE_WDA,
> +						create_wda_cb, modem, NULL);
> +
>  	qmi_service_create_shared(data->device, QMI_SERVICE_DMS,
>  						create_dms_cb, modem, NULL);
>  }
> @@ -368,6 +388,11 @@ static int gobi_disable(struct ofono_modem *modem)
>  	qmi_service_cancel_all(data->dms);
>  	qmi_service_unregister_all(data->dms);
>
> +	if (data->wda) {
> +		qmi_service_unregister_all(data->wda);
> +		qmi_service_unref(data->wda);
> +	}
> +
>  	/*
>  	 * Telit QMI modem must remain online. If powered down, it also
>  	 * powers down the sim card, and QMI interface has no way to bring
> @@ -484,14 +509,61 @@ static void gobi_post_sim(struct ofono_modem *modem)
>  		ofono_sms_create(modem, 0, "qmimodem", data->device);
>  }
>
> +static void get_data_format_cb(struct qmi_result *result, void *user_data)
> +{
> +	struct ofono_modem* modem = user_data;
> +	struct gobi_data *data = ofono_modem_get_data(modem);
> +	uint32_t llproto;
> +	enum qmi_device_expected_data_format expected_llproto;
> +
> +	DBG("");
> +
> +	if (qmi_result_set_error(result, NULL))
> +		return;
> +
> +	if (!qmi_result_get_uint32(result, QMI_WDA_LL_PROTOCOL, &llproto))
> +		return;
> +
> +	expected_llproto = qmi_device_get_expected_data_format(data->device);
> +
> +	if ((llproto == QMI_WDA_DATA_LINK_PROTOCOL_802_3) &&
> +			(expected_llproto ==
> +				QMI_DEVICE_EXPECTED_DATA_FORMAT_RAW_IP)) {
> +		if (!qmi_device_set_expected_data_format(data->device,
> +					QMI_DEVICE_EXPECTED_DATA_FORMAT_802_3))
> +			DBG("Fail to set expected data to 802.3");
> +		else
> +			DBG("expected data set to 802.3");
> +	} else if ((llproto == QMI_WDA_DATA_LINK_PROTOCOL_RAW_IP) &&
> +			(expected_llproto ==
> +				QMI_DEVICE_EXPECTED_DATA_FORMAT_802_3)) {
> +		if (!qmi_device_set_expected_data_format(data->device,
> +					QMI_DEVICE_EXPECTED_DATA_FORMAT_RAW_IP))
> +			DBG("Fail to set expected data to raw-ip");
> +		else
> +			DBG("expected data set to raw-ip");
> +	}
> +}
> +
>  static void gobi_post_online(struct ofono_modem *modem)
>  {
>  	struct gobi_data *data = ofono_modem_get_data(modem);
>  	struct ofono_gprs *gprs;
>  	struct ofono_gprs_context *gc;
> +	int err;
>
>  	DBG("%p", modem);
>
> +	if (data->features & GOBI_WDA) {
> +		/* Query device link-layer protocol */
> +		err = qmi_service_send(data->wda,
> +					QMI_WDA_GET_DATA_FORMAT, NULL,
> +					get_data_format_cb, modem, NULL);
> +		if (err) {
> +			DBG("Failed to query link layer protocol");
> +		}

Not our style, no need for {}

Also, it is really frowned upon to call initialization functions from 
inside pre_sim, post_sim and post_online.  They're really only for atom 
creation.

Is WDA_GET_DATA_FORMAT available right after we power the device on, 
e.g. by using SET_OPER_MODE?

Alternatively, this might need to be done as a part of the .set_online 
transaction.

> +	}
> +
>  	if (data->features & GOBI_NAS)
>  		ofono_netreg_create(modem, 0, "qmimodem", data->device);
>
>

Regards,
-Denis

Re: [PATCH qmi 0/5] QMI patches

[Denis Kenzior]

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

Hi Jonas,

On 03/25/2017 11:57 AM, Jonas Bonn wrote:
> This is a series of cleanups related to QMI modems.
>
> You might want to consider patch 5 as an object for review more than
> submission.  It's what I'm currently using with my UC21.
>
> Jonas Bonn (5):
>   qmimodem: fix typo
>   qmimodem: add WDA service string
>   gobi: query presence of WDA service

I went ahead and applied the first three.

Regards,
-Denis

Re: [PATCH qmi 4/5] gobi: take LLP-matching code from qmimodem

[Christophe Ronco]

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

Hello Jonas, hello Denis,

On 03/25/2017 10:11 PM, Denis Kenzior wrote:
> Hi Jonas,
>
> On 03/25/2017 11:57 AM, Jonas Bonn wrote:
>> The gobi plugin is a top-level construct that steers the creation
>> of other constructs based on modem capabilities.  The existence of
>> the WDA QMI service should thus be used to decide whether to attempt
>> to set the link-layer protocol based on the device configuration;
>> this code therefore belongs in the gobi plugin instead of in the
>> gprs-context code which is otherwise agnostic of any link-layer
>> settings.
>>
>> This patch, therefore, moves the code for querying the WDA service
>> and setting the link-layer protocol into the gobi driver.  The only
>> small change made with respect the original is a de-coupling of
>> service creation and setting of the link layer.
>
> Christophe added this recently, so he might want to comment...
First, I tried this patch on my MC7430 modem and it is still working.

I really don't know if this code should be in QMI driver or in Gobi 
plugin. It can be used by all QMI devices, that's why I put it in the 
driver but if Gobi plugin is the only entry point for QMI devices, 
that's also a great place for this code.

Creating service during enable is the best place to do it. I would have 
done service creation sequentially, not WDA and DMS at the same time but 
I assume it won't change anything and WDA service creation will always 
be shorter than DMS service creation and all the commands sent after.

As Denis proposed, I would have done the change of LLP inside the 
set_online callback rather than in gobi_post_online to be sure this is 
finished before the gprs_context is created (and maybe activated).

Best Regards,

Christophe

>
>> ---
>>  drivers/qmimodem/gprs-context.c | 74 
>> +----------------------------------------
>>  plugins/gobi.c                  | 72 
>> +++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 73 insertions(+), 73 deletions(-)
>>
>> diff --git a/drivers/qmimodem/gprs-context.c 
>> b/drivers/qmimodem/gprs-context.c
>> index da2be24..ab9f005 100644
>> --- a/drivers/qmimodem/gprs-context.c
>> +++ b/drivers/qmimodem/gprs-context.c
>> @@ -30,15 +30,12 @@
>>  #include <ofono/gprs-context.h>
>>
>>  #include "qmi.h"
>> -#include "wda.h"
>>  #include "wds.h"
>>
>>  #include "qmimodem.h"
>>
>>  struct gprs_context_data {
>>      struct qmi_service *wds;
>> -    struct qmi_service *wda;
>> -    struct qmi_device *dev;
>>      unsigned int active_context;
>>      uint32_t pkt_handle;
>>  };
>> @@ -290,69 +287,6 @@ static void create_wds_cb(struct qmi_service 
>> *service, void *user_data)
>>                      pkt_status_notify, gc, NULL);
>>  }
>>
>> -static void get_data_format_cb(struct qmi_result *result, void 
>> *user_data)
>> -{
>> -    struct ofono_gprs_context *gc = user_data;
>> -    struct gprs_context_data *data = ofono_gprs_context_get_data(gc);
>> -    uint32_t llproto;
>> -    enum qmi_device_expected_data_format expected_llproto;
>> -
>> -    DBG("");
>> -
>> -    if (qmi_result_set_error(result, NULL))
>> -        goto done;
>> -
>> -    if (!qmi_result_get_uint32(result, QMI_WDA_LL_PROTOCOL, &llproto))
>> -        goto done;
>> -
>> -    expected_llproto = qmi_device_get_expected_data_format(data->dev);
>> -
>> -    if ((llproto == QMI_WDA_DATA_LINK_PROTOCOL_802_3) &&
>> -            (expected_llproto ==
>> -                QMI_DEVICE_EXPECTED_DATA_FORMAT_RAW_IP)) {
>> -        if (!qmi_device_set_expected_data_format(data->dev,
>> -                    QMI_DEVICE_EXPECTED_DATA_FORMAT_802_3))
>> -            DBG("Fail to set expected data to 802.3");
>> -        else
>> -            DBG("expected data set to 802.3");
>> -    } else if ((llproto == QMI_WDA_DATA_LINK_PROTOCOL_RAW_IP) &&
>> -            (expected_llproto ==
>> -                QMI_DEVICE_EXPECTED_DATA_FORMAT_802_3)) {
>> -        if (!qmi_device_set_expected_data_format(data->dev,
>> -                    QMI_DEVICE_EXPECTED_DATA_FORMAT_RAW_IP))
>> -            DBG("Fail to set expected data to raw-ip");
>> -        else
>> -            DBG("expected data set to raw-ip");
>> -    }
>> -
>> -done:
>> -    qmi_service_create(data->dev, QMI_SERVICE_WDS, create_wds_cb, gc,
>> -                                    NULL);
>> -}
>> -
>> -static void create_wda_cb(struct qmi_service *service, void *user_data)
>> -{
>> -    struct ofono_gprs_context *gc = user_data;
>> -    struct gprs_context_data *data = ofono_gprs_context_get_data(gc);
>> -
>> -    DBG("");
>> -
>> -    if (!service) {
>> -        DBG("Failed to request WDA service, continue initialization");
>> -        goto error;
>> -    }
>> -
>> -    data->wda = qmi_service_ref(service);
>> -
>> -    if (qmi_service_send(data->wda, QMI_WDA_GET_DATA_FORMAT, NULL,
>> -                    get_data_format_cb, gc, NULL) > 0)
>> -        return;
>> -
>> -error:
>> -    qmi_service_create(data->dev, QMI_SERVICE_WDS, create_wds_cb, gc,
>> -                                    NULL);
>> -}
>> -
>>  static int qmi_gprs_context_probe(struct ofono_gprs_context *gc,
>>                      unsigned int vendor, void *user_data)
>>  {
>> @@ -364,9 +298,8 @@ static int qmi_gprs_context_probe(struct 
>> ofono_gprs_context *gc,
>>      data = g_new0(struct gprs_context_data, 1);
>>
>>      ofono_gprs_context_set_data(gc, data);
>> -    data->dev = device;
>>
>> -    qmi_service_create(device, QMI_SERVICE_WDA, create_wda_cb, gc, 
>> NULL);
>> +    qmi_service_create(device, QMI_SERVICE_WDS, create_wds_cb, gc, 
>> NULL);
>>
>>      return 0;
>>  }
>> @@ -384,11 +317,6 @@ static void qmi_gprs_context_remove(struct 
>> ofono_gprs_context *gc)
>>          qmi_service_unref(data->wds);
>>      }
>>
>> -    if (data->wda) {
>> -        qmi_service_unregister_all(data->wda);
>> -        qmi_service_unref(data->wda);
>> -    }
>> -
>>      g_free(data);
>>  }
>>
>> diff --git a/plugins/gobi.c b/plugins/gobi.c
>> index df35f94..d160d58 100644
>> --- a/plugins/gobi.c
>> +++ b/plugins/gobi.c
>> @@ -66,6 +66,7 @@
>>  struct gobi_data {
>>      struct qmi_device *device;
>>      struct qmi_service *dms;
>> +    struct qmi_service *wda;
>>      unsigned long features;
>>      unsigned int discover_attempts;
>>      uint8_t oper_mode;
>> @@ -252,6 +253,21 @@ error:
>>      shutdown_device(modem);
>>  }
>>
>> +static void create_wda_cb(struct qmi_service *service, void *user_data)
>> +{
>> +    struct ofono_modem *modem = user_data;
>> +    struct gobi_data *data = ofono_modem_get_data(modem);
>> +
>> +    DBG("");
>> +
>> +    if (!service) {
>> +        DBG("Failed to request WDA service, continue initialization");
>> +        return;
>> +    }
>> +
>> +    data->wda = qmi_service_ref(service);
>> +}
>> +
>>  static void discover_cb(uint8_t count, const struct qmi_version *list,
>>                              void *user_data)
>>  {
>> @@ -313,6 +329,10 @@ static void discover_cb(uint8_t count, const 
>> struct qmi_version *list,
>>          return;
>>      }
>>
>> +    if (data->features & GOBI_WDA)
>> +        qmi_service_create(data->device, QMI_SERVICE_WDA,
>> +                        create_wda_cb, modem, NULL);
>> +
>>      qmi_service_create_shared(data->device, QMI_SERVICE_DMS,
>>                          create_dms_cb, modem, NULL);
>>  }
>> @@ -368,6 +388,11 @@ static int gobi_disable(struct ofono_modem *modem)
>>      qmi_service_cancel_all(data->dms);
>>      qmi_service_unregister_all(data->dms);
>>
>> +    if (data->wda) {
>> +        qmi_service_unregister_all(data->wda);
>> +        qmi_service_unref(data->wda);
>> +    }
>> +
>>      /*
>>       * Telit QMI modem must remain online. If powered down, it also
>>       * powers down the sim card, and QMI interface has no way to bring
>> @@ -484,14 +509,61 @@ static void gobi_post_sim(struct ofono_modem 
>> *modem)
>>          ofono_sms_create(modem, 0, "qmimodem", data->device);
>>  }
>>
>> +static void get_data_format_cb(struct qmi_result *result, void 
>> *user_data)
>> +{
>> +    struct ofono_modem* modem = user_data;
>> +    struct gobi_data *data = ofono_modem_get_data(modem);
>> +    uint32_t llproto;
>> +    enum qmi_device_expected_data_format expected_llproto;
>> +
>> +    DBG("");
>> +
>> +    if (qmi_result_set_error(result, NULL))
>> +        return;
>> +
>> +    if (!qmi_result_get_uint32(result, QMI_WDA_LL_PROTOCOL, &llproto))
>> +        return;
>> +
>> +    expected_llproto = 
>> qmi_device_get_expected_data_format(data->device);
>> +
>> +    if ((llproto == QMI_WDA_DATA_LINK_PROTOCOL_802_3) &&
>> +            (expected_llproto ==
>> +                QMI_DEVICE_EXPECTED_DATA_FORMAT_RAW_IP)) {
>> +        if (!qmi_device_set_expected_data_format(data->device,
>> +                    QMI_DEVICE_EXPECTED_DATA_FORMAT_802_3))
>> +            DBG("Fail to set expected data to 802.3");
>> +        else
>> +            DBG("expected data set to 802.3");
>> +    } else if ((llproto == QMI_WDA_DATA_LINK_PROTOCOL_RAW_IP) &&
>> +            (expected_llproto ==
>> +                QMI_DEVICE_EXPECTED_DATA_FORMAT_802_3)) {
>> +        if (!qmi_device_set_expected_data_format(data->device,
>> +                    QMI_DEVICE_EXPECTED_DATA_FORMAT_RAW_IP))
>> +            DBG("Fail to set expected data to raw-ip");
>> +        else
>> +            DBG("expected data set to raw-ip");
>> +    }
>> +}
>> +
>>  static void gobi_post_online(struct ofono_modem *modem)
>>  {
>>      struct gobi_data *data = ofono_modem_get_data(modem);
>>      struct ofono_gprs *gprs;
>>      struct ofono_gprs_context *gc;
>> +    int err;
>>
>>      DBG("%p", modem);
>>
>> +    if (data->features & GOBI_WDA) {
>> +        /* Query device link-layer protocol */
>> +        err = qmi_service_send(data->wda,
>> +                    QMI_WDA_GET_DATA_FORMAT, NULL,
>> +                    get_data_format_cb, modem, NULL);
>> +        if (err) {
>> +            DBG("Failed to query link layer protocol");
>> +        }
>
> Not our style, no need for {}
>
> Also, it is really frowned upon to call initialization functions from 
> inside pre_sim, post_sim and post_online.  They're really only for 
> atom creation.
>
> Is WDA_GET_DATA_FORMAT available right after we power the device on, 
> e.g. by using SET_OPER_MODE?
>
> Alternatively, this might need to be done as a part of the .set_online 
> transaction.
>
>> +    }
>> +
>>      if (data->features & GOBI_NAS)
>>          ofono_netreg_create(modem, 0, "qmimodem", data->device);
>>
>>
>
> Regards,
> -Denis
>