Re: [PATCH 2/5] speedup: Check for supported modem capabilities first

Re: [PATCH 2/5] speedup: Check for supported modem capabilities first

[Denis Kenzior]

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

Hi Guillaume,

On 01/09/2012 07:10 AM, Guillaume Zajac wrote:
> ---
>  plugins/speedup.c |   40 ++++++++++++++++++++++++++++++++++++++--
>  1 files changed, 38 insertions(+), 2 deletions(-)
> 
> diff --git a/plugins/speedup.c b/plugins/speedup.c
> index a90dfe3..f03f8f3 100644
> --- a/plugins/speedup.c
> +++ b/plugins/speedup.c
> @@ -25,6 +25,7 @@
>  
>  #include <errno.h>
>  #include <stdlib.h>
> +#include <string.h>
>  
>  #include <glib.h>
>  #include <gatchat.h>
> @@ -47,11 +48,15 @@
>  #include <drivers/atmodem/atutil.h>
>  #include <drivers/atmodem/vendor.h>
>  
> +static const char *gcap_prefix[] = { "+GCAP:", NULL };
> +
>  struct speedup_data {
>  	GAtChat *modem;
>  	GAtChat *aux;
>  	gboolean have_sim;
>  	struct at_util_sim_state_query *sim_state_query;
> +	gboolean have_gsm;
> +	gboolean have_cdma;

It might be a good idea to use a single gboolean, or better yet an enum
here instead.  There's no point to waste 8 bytes when a single byte can do.

>  };
>  
>  static int speedup_probe(struct ofono_modem *modem)
> @@ -165,6 +170,36 @@ static void cfun_enable(gboolean ok, GAtResult *result, gpointer user_data)
>  						2, 20, sim_state_cb, modem);
>  }
>  
> +static void gcap_support(gboolean ok, GAtResult *result, gpointer user_data)
> +{
> +	struct ofono_modem *modem = user_data;
> +	struct speedup_data *data = ofono_modem_get_data(modem);
> +	GAtResultIter iter;
> +	const char *gcap;
> +
> +	if (!ok)
> +		goto done;
> +
> +	g_at_result_iter_init(&iter, result);
> +
> +	if (!g_at_result_iter_next(&iter, "+GCAP:"))
> +		goto done;
> +
> +	while (g_at_result_iter_next_unquoted_string(&iter, &gcap)) {
> +		if (*gcap == '\0')
> +			break;
> +
> +		if (!strcmp(gcap, "+CGSM"))
> +			data->have_gsm = TRUE;
> +		else if (!strcmp(gcap, "+CIS707-A"))
> +			data->have_cdma = TRUE;
> +	}

First of all, this really belongs in atutil as a convenience function.

Second, why don't we fail on the obvious condition that neither CDMA nor
GSM is detected?

> +
> +done:
> +	g_at_chat_send(data->aux, "AT+CFUN=1", NULL,
> +					cfun_enable, modem, NULL);
> +}
> +
>  static int speedup_enable(struct ofono_modem *modem)
>  {
>  	struct speedup_data *data = ofono_modem_get_data(modem);
> @@ -185,8 +220,9 @@ static int speedup_enable(struct ofono_modem *modem)
>  	g_at_chat_send(data->modem, "ATE0 +CMEE=1", NULL, NULL, NULL, NULL);
>  	g_at_chat_send(data->aux, "ATE0 +CMEE=1", NULL, NULL, NULL, NULL);
>  
> -	g_at_chat_send(data->aux, "AT+CFUN=1", NULL,
> -					cfun_enable, modem, NULL);
> +	/* Check for GSM/CDMA capabilities */
> +	g_at_chat_send(data->aux, "ATI", gcap_prefix,
> +					gcap_support, modem, NULL);
>  
>  	return -EINPROGRESS;
>  }

Regards,
-Denis

Re: [PATCH 1/5] udevng: Remove huaweicdma label from driver_list

[Denis Kenzior]

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

Hi Guillaume,

On 01/09/2012 07:10 AM, Guillaume Zajac wrote:
> ---
>  plugins/udevng.c |    1 -
>  1 files changed, 0 insertions(+), 1 deletions(-)

Applied, thanks.

Regards,
-Denis

Re: [PATCH 2/5] speedup: Check for supported modem capabilities first

[Denis Kenzior]

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

Hi Marcel,

>>>  struct speedup_data {
>>>  	GAtChat *modem;
>>>  	GAtChat *aux;
>>>  	gboolean have_sim;
>>>  	struct at_util_sim_state_query *sim_state_query;
>>> +	gboolean have_gsm;
>>> +	gboolean have_cdma;
>>
>> It might be a good idea to use a single gboolean, or better yet an enum
>> here instead.  There's no point to waste 8 bytes when a single byte can do.
> 
> I did this on purpose for the Huawei driver. We can unify this later on,
> but at this moment I rather see what is actually happening.
> 
> Since we keep parsing all capabilities, I wanna avoid that a later one
> overwrites a previous one. That said, just using some flags would be
> better anyway. Especially since we also always have have_sim as well.
> 
> However that can be done as a further optimization in the Speedup and
> Huawei drivers.
> 

A flagged enum is indeed what I had in mind when I wrote this.  But fair
enough on your reasoning.

Regards,
-Denis

Re: [PATCH 2/5] speedup: Check for supported modem capabilities first

[Denis Kenzior]

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

Hi Guillaume,

On 01/10/2012 07:43 AM, Guillaume Zajac wrote:
> Hi Marcel and Denis,
> 
>>>>>   struct speedup_data {
>>>>>       GAtChat *modem;
>>>>>       GAtChat *aux;
>>>>>       gboolean have_sim;
>>>>>       struct at_util_sim_state_query *sim_state_query;
>>>>> +    gboolean have_gsm;
>>>>> +    gboolean have_cdma;
>>>> It might be a good idea to use a single gboolean, or better yet an enum
>>>> here instead.  There's no point to waste 8 bytes when a single byte
>>>> can do.
>>> I did this on purpose for the Huawei driver. We can unify this later on,
>>> but at this moment I rather see what is actually happening.
>>>
>>> Since we keep parsing all capabilities, I wanna avoid that a later one
>>> overwrites a previous one. That said, just using some flags would be
>>> better anyway. Especially since we also always have have_sim as well.
>>>
>>> However that can be done as a further optimization in the Speedup and
>>> Huawei drivers.
>>>
>> A flagged enum is indeed what I had in mind when I wrote this.  But fair
>> enough on your reasoning
> 
> As we are starting thinking about tweaking SIM atom to make it work with
> CDMA modem,
> maybe having quickly the flagged enum would be helpful to distinguish if
> it is a CDMA or GSM modem that is asking for the SIM atom creation.
> 
> Then where should this enum modem_type take place?
> Should we tweak the already existing one into modem.h in replacing:
> 
> OFONO_MODEM_TYPE_HARDWARE = 0,
> OFONO_MODEM_TYPE_HFP,
> OFONO_MODEM_TYPE_SAP,
> 
> by
> 
> OFONO_MODEM_TYPE_UNKNOWN_HARDWARE = 0,
> OFONO_MODEM_TYPE_GSM_HARDWARE = 1,
> OFONO_MODEM_TYPE_CDMA_HARDWARE = 2,
> OFONO_MODEM_TYPE_HFP = 4,
> OFONO_MODEM_TYPE_SAP = 8,
> 
> Then we have just to specify the modem_type in creating the SIM atom to
> use its limited functionnalities when we have CDMA modem
> e.g. manage only PIN protection.

No, the type information is a kludge that will hamper us in the end, so
I am against any such changes.  Even the current modem_type enum took
some convincing and it is meant to be used as a hint for external
applications, not used internally by oFono core in any way.

Regards,
-Denis

Re: [PATCH 2/5] speedup: Check for supported modem capabilities first

[Denis Kenzior]

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

Hi Guillaume,

> We can have a private enum modem type into plugin, but we won't be able
> to use it into any atoms.
> Maybe by deduction we can add an enum sim_type into sim.h store the type
> into the plugin data and pass it at SIM atom creation.
> What do you think?

Inside the plugin you can do whatever you want for determining the cdma
vs gsm modem type.  I'm not quite sure I follow your comment about sim.h...

Regards,
-Denis

[PATCH 0/5] Unify SpeedUp plugin

[Guillaume Zajac]

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

Hi,

Patch 1 is deleting huaweicdma label in udevng.
I submitted it again into this set of patch to avoid conflicts.
The 4 next patches are doing same job as for Huawei plugin on SpeedUp plugin.

Kind regards,
Guillaume

Guillaume Zajac (5):
  udevng: Remove huaweicdma label from driver_list
  speedup: Check for supported modem capabilities first
  speedup: Create GSM/UMTS atoms only if support has been detected
  speedup: Create CDMA atoms if support has been detected
  udevng: Simplify vendor and driver list for SpeedUp

 plugins/speedup.c |   99 ++++++++++++++++++++++++++++++++++++++++------------
 plugins/udevng.c  |    3 --
 2 files changed, 76 insertions(+), 26 deletions(-)

[PATCH 1/5] udevng: Remove huaweicdma label from driver_list

[Guillaume Zajac]

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

---
 plugins/udevng.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/plugins/udevng.c b/plugins/udevng.c
index 7d81416..315b8af 100644
--- a/plugins/udevng.c
+++ b/plugins/udevng.c
@@ -606,7 +606,6 @@ static struct {
 	{ "gobi",	setup_gobi,	},
 	{ "sierra",	setup_sierra	},
 	{ "huawei",	setup_huawei	},
-	{ "huaweicdma",	setup_huawei	},
 	{ "speedupcdma",setup_speedup	},
 	{ "speedup",	setup_speedup	},
 	{ "linktop",	setup_linktop	},
-- 
1.7.1

[PATCH 2/5] speedup: Check for supported modem capabilities first

[Guillaume Zajac]

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

---
 plugins/speedup.c |   40 ++++++++++++++++++++++++++++++++++++++--
 1 files changed, 38 insertions(+), 2 deletions(-)

diff --git a/plugins/speedup.c b/plugins/speedup.c
index a90dfe3..f03f8f3 100644
--- a/plugins/speedup.c
+++ b/plugins/speedup.c
@@ -25,6 +25,7 @@
 
 #include <errno.h>
 #include <stdlib.h>
+#include <string.h>
 
 #include <glib.h>
 #include <gatchat.h>
@@ -47,11 +48,15 @@
 #include <drivers/atmodem/atutil.h>
 #include <drivers/atmodem/vendor.h>
 
+static const char *gcap_prefix[] = { "+GCAP:", NULL };
+
 struct speedup_data {
 	GAtChat *modem;
 	GAtChat *aux;
 	gboolean have_sim;
 	struct at_util_sim_state_query *sim_state_query;
+	gboolean have_gsm;
+	gboolean have_cdma;
 };
 
 static int speedup_probe(struct ofono_modem *modem)
@@ -165,6 +170,36 @@ static void cfun_enable(gboolean ok, GAtResult *result, gpointer user_data)
 						2, 20, sim_state_cb, modem);
 }
 
+static void gcap_support(gboolean ok, GAtResult *result, gpointer user_data)
+{
+	struct ofono_modem *modem = user_data;
+	struct speedup_data *data = ofono_modem_get_data(modem);
+	GAtResultIter iter;
+	const char *gcap;
+
+	if (!ok)
+		goto done;
+
+	g_at_result_iter_init(&iter, result);
+
+	if (!g_at_result_iter_next(&iter, "+GCAP:"))
+		goto done;
+
+	while (g_at_result_iter_next_unquoted_string(&iter, &gcap)) {
+		if (*gcap == '\0')
+			break;
+
+		if (!strcmp(gcap, "+CGSM"))
+			data->have_gsm = TRUE;
+		else if (!strcmp(gcap, "+CIS707-A"))
+			data->have_cdma = TRUE;
+	}
+
+done:
+	g_at_chat_send(data->aux, "AT+CFUN=1", NULL,
+					cfun_enable, modem, NULL);
+}
+
 static int speedup_enable(struct ofono_modem *modem)
 {
 	struct speedup_data *data = ofono_modem_get_data(modem);
@@ -185,8 +220,9 @@ static int speedup_enable(struct ofono_modem *modem)
 	g_at_chat_send(data->modem, "ATE0 +CMEE=1", NULL, NULL, NULL, NULL);
 	g_at_chat_send(data->aux, "ATE0 +CMEE=1", NULL, NULL, NULL, NULL);
 
-	g_at_chat_send(data->aux, "AT+CFUN=1", NULL,
-					cfun_enable, modem, NULL);
+	/* Check for GSM/CDMA capabilities */
+	g_at_chat_send(data->aux, "ATI", gcap_prefix,
+					gcap_support, modem, NULL);
 
 	return -EINPROGRESS;
 }
-- 
1.7.1

[PATCH 3/5] speedup: Create GSM/UMTS atoms only if support has been detected

[Guillaume Zajac]

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

---
 plugins/speedup.c |   50 +++++++++++++++++++++++++++++---------------------
 1 files changed, 29 insertions(+), 21 deletions(-)

diff --git a/plugins/speedup.c b/plugins/speedup.c
index f03f8f3..02b6f61 100644
--- a/plugins/speedup.c
+++ b/plugins/speedup.c
@@ -265,49 +265,57 @@ static int speedup_disable(struct ofono_modem *modem)
 static void speedup_pre_sim(struct ofono_modem *modem)
 {
 	struct speedup_data *data = ofono_modem_get_data(modem);
-	struct ofono_sim *sim;
 
 	DBG("%p", modem);
 
-	ofono_devinfo_create(modem, 0, "atmodem", data->aux);
-	sim = ofono_sim_create(modem, OFONO_VENDOR_SPEEDUP,
-						"atmodem", data->aux);
+	if (data->have_gsm == TRUE) {
+		struct ofono_sim *sim;
+		ofono_devinfo_create(modem, 0, "atmodem", data->aux);
+		sim = ofono_sim_create(modem, OFONO_VENDOR_SPEEDUP,
+							"atmodem", data->aux);
 
-	if (sim && data->have_sim == TRUE)
-		ofono_sim_inserted_notify(sim, TRUE);
+		if (sim && data->have_sim == TRUE)
+			ofono_sim_inserted_notify(sim, TRUE);
+	}
 }
 
 static void speedup_post_sim(struct ofono_modem *modem)
 {
 	struct speedup_data *data = ofono_modem_get_data(modem);
-	struct ofono_gprs *gprs;
-	struct ofono_gprs_context *gc;
 
 	DBG("%p", modem);
 
-	ofono_phonebook_create(modem, 0, "atmodem", data->aux);
+	if (data->have_gsm == TRUE) {
+		struct ofono_gprs *gprs;
+		struct ofono_gprs_context *gc;
+		ofono_phonebook_create(modem, 0, "atmodem", data->aux);
 
-	ofono_sms_create(modem, OFONO_VENDOR_QUALCOMM_MSM,
-						"atmodem", data->aux);
+		ofono_sms_create(modem, OFONO_VENDOR_QUALCOMM_MSM,
+							"atmodem", data->aux);
 
-	gprs = ofono_gprs_create(modem, OFONO_VENDOR_SPEEDUP,
-						"atmodem", data->aux);
-	gc = ofono_gprs_context_create(modem, 0, "atmodem", data->modem);
+		gprs = ofono_gprs_create(modem, OFONO_VENDOR_SPEEDUP,
+							"atmodem", data->aux);
+		gc = ofono_gprs_context_create(modem, 0, "atmodem",
+							data->modem);
 
-	if (gprs && gc)
-		ofono_gprs_add_context(gprs, gc);
+		if (gprs && gc)
+			ofono_gprs_add_context(gprs, gc);
+	}
 }
 
 static void speedup_post_online(struct ofono_modem *modem)
 {
 	struct speedup_data *data = ofono_modem_get_data(modem);
 
-	ofono_netreg_create(modem, OFONO_VENDOR_SPEEDUP, "atmodem", data->aux);
+	if (data->have_gsm == TRUE) {
+		ofono_netreg_create(modem, OFONO_VENDOR_SPEEDUP, "atmodem",
+							data->aux);
 
-	ofono_cbs_create(modem, OFONO_VENDOR_QUALCOMM_MSM,
-						"atmodem", data->aux);
-	ofono_ussd_create(modem, OFONO_VENDOR_QUALCOMM_MSM,
-						"atmodem", data->aux);
+		ofono_cbs_create(modem, OFONO_VENDOR_QUALCOMM_MSM,
+							"atmodem", data->aux);
+		ofono_ussd_create(modem, OFONO_VENDOR_QUALCOMM_MSM,
+							"atmodem", data->aux);
+	}
 }
 
 static struct ofono_modem_driver speedup_driver = {
-- 
1.7.1

[PATCH 4/5] speedup: Create CDMA atoms if support has been detected

[Guillaume Zajac]

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

---
 plugins/speedup.c |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/plugins/speedup.c b/plugins/speedup.c
index 02b6f61..7c475a1 100644
--- a/plugins/speedup.c
+++ b/plugins/speedup.c
@@ -43,6 +43,8 @@
 #include <ofono/gprs.h>
 #include <ofono/gprs-context.h>
 #include <ofono/phonebook.h>
+#include <ofono/cdma-netreg.h>
+#include <ofono/cdma-connman.h>
 #include <ofono/log.h>
 
 #include <drivers/atmodem/atutil.h>
@@ -276,6 +278,8 @@ static void speedup_pre_sim(struct ofono_modem *modem)
 
 		if (sim && data->have_sim == TRUE)
 			ofono_sim_inserted_notify(sim, TRUE);
+	} else if (data->have_cdma == TRUE) {
+		ofono_devinfo_create(modem, 0, "cdmamodem", data->aux);
 	}
 }
 
@@ -315,6 +319,11 @@ static void speedup_post_online(struct ofono_modem *modem)
 							"atmodem", data->aux);
 		ofono_ussd_create(modem, OFONO_VENDOR_QUALCOMM_MSM,
 							"atmodem", data->aux);
+	} else if (data->have_cdma == TRUE) {
+		ofono_cdma_netreg_create(modem, 0, "huaweimodem", data->aux);
+
+		ofono_cdma_connman_create(modem, OFONO_VENDOR_HUAWEI,
+						"cdmamodem", data->modem);
 	}
 }
 
-- 
1.7.1

[PATCH 5/5] udevng: Simplify vendor and driver list for SpeedUp

[Guillaume Zajac]

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

---
 plugins/udevng.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/plugins/udevng.c b/plugins/udevng.c
index 315b8af..3dd48d5 100644
--- a/plugins/udevng.c
+++ b/plugins/udevng.c
@@ -606,7 +606,6 @@ static struct {
 	{ "gobi",	setup_gobi,	},
 	{ "sierra",	setup_sierra	},
 	{ "huawei",	setup_huawei	},
-	{ "speedupcdma",setup_speedup	},
 	{ "speedup",	setup_speedup	},
 	{ "linktop",	setup_linktop	},
 	{ "alcatel",	setup_alcatel	},
@@ -795,7 +794,6 @@ static struct {
 	{ "huawei",	"option",	"201e"		},
 	{ "huawei",	"cdc_ether",	"12d1"		},
 	{ "huawei",	"option",	"12d1"		},
-	{ "speedupcdma","option",	"1c9e", "9e00"	},
 	{ "speedup",	"option",	"1c9e"		},
 	{ "speedup",	"option",	"2020"		},
 	{ "alcatel",	"option",	"1bbb", "0017"	},
-- 
1.7.1

[PATCH] speedupcdma: Delete unused plugin

[Guillaume Zajac]

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

---
 Makefile.am           |    3 -
 plugins/speedupcdma.c |  247 -------------------------------------------------
 2 files changed, 0 insertions(+), 250 deletions(-)
 delete mode 100644 plugins/speedupcdma.c

diff --git a/Makefile.am b/Makefile.am
index 9763d00..8addb39 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -340,9 +340,6 @@ builtin_sources += plugins/alcatel.c
 builtin_modules += speedup
 builtin_sources += plugins/speedup.c
 
-builtin_modules += speedupcdma
-builtin_sources += plugins/speedupcdma.c
-
 builtin_modules += samsung
 builtin_sources += plugins/samsung.c
 
diff --git a/plugins/speedupcdma.c b/plugins/speedupcdma.c
deleted file mode 100644
index 8e5f324..0000000
--- a/plugins/speedupcdma.c
+++ /dev/null
@@ -1,247 +0,0 @@
-/*
- *
- *  oFono - Open Source Telephony
- *
- *  Copyright (C) 2008-2011  Intel Corporation. All rights reserved.
- *
- *  This program is free software; you can redistribute it and/or modify
- *  it under the terms of the GNU General Public License version 2 as
- *  published by the Free Software Foundation.
- *
- *  This program is distributed in the hope that it will be useful,
- *  but WITHOUT ANY WARRANTY; without even the implied warranty of
- *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- *  GNU General Public License for more details.
- *
- *  You should have received a copy of the GNU General Public License
- *  along with this program; if not, write to the Free Software
- *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
- *
- */
-
-#ifdef HAVE_CONFIG_H
-#include <config.h>
-#endif
-
-#include <errno.h>
-#include <stdlib.h>
-
-#include <glib.h>
-#include <gatchat.h>
-#include <gattty.h>
-
-#define OFONO_API_SUBJECT_TO_CHANGE
-#include <ofono/plugin.h>
-#include <ofono/modem.h>
-#include <ofono/devinfo.h>
-#include <ofono/cdma-netreg.h>
-#include <ofono/cdma-connman.h>
-#include <ofono/log.h>
-
-#include "drivers/atmodem/vendor.h"
-
-struct speedupcdma_data {
-	GAtChat *modem;
-	GAtChat *aux;
-};
-
-static void speedupcdma_debug(const char *str, void *data)
-{
-	const char *prefix = data;
-
-	ofono_info("%s%s", prefix, str);
-}
-
-static int speedupcdma_probe(struct ofono_modem *modem)
-{
-	struct speedupcdma_data *data;
-
-	DBG("%p", modem);
-
-	data = g_try_new0(struct speedupcdma_data, 1);
-	if (data == NULL)
-		return -ENOMEM;
-
-	ofono_modem_set_data(modem, data);
-
-	return 0;
-}
-
-static void speedupcdma_remove(struct ofono_modem *modem)
-{
-	struct speedupcdma_data *data = ofono_modem_get_data(modem);
-
-	DBG("%p", modem);
-
-	ofono_modem_set_data(modem, NULL);
-
-	/* Cleanup after hot-unplug */
-	g_at_chat_unref(data->aux);
-
-	g_free(data);
-}
-
-static void cfun_enable(gboolean ok, GAtResult *result, gpointer user_data)
-{
-	struct ofono_modem *modem = user_data;
-	struct speedupcdma_data *data = ofono_modem_get_data(modem);
-
-	DBG("");
-
-	if (!ok) {
-		g_at_chat_unref(data->modem);
-		data->modem = NULL;
-
-		g_at_chat_unref(data->aux);
-		data->aux = NULL;
-	}
-
-	ofono_modem_set_powered(modem, ok);
-}
-
-static GAtChat *open_device(struct ofono_modem *modem,
-				const char *key, char *debug)
-{
-	const char *device;
-	GIOChannel *channel;
-	GAtSyntax *syntax;
-	GAtChat *chat;
-
-	device = ofono_modem_get_string(modem, key);
-	if (device == NULL)
-		return NULL;
-
-	DBG("%s %s", key, device);
-
-	channel = g_at_tty_open(device, NULL);
-	if (channel == NULL)
-		return NULL;
-
-	syntax = g_at_syntax_new_gsm_permissive();
-	chat = g_at_chat_new(channel, syntax);
-	g_at_syntax_unref(syntax);
-
-	g_io_channel_unref(channel);
-
-	if (chat == NULL)
-		return NULL;
-
-	if (getenv("OFONO_AT_DEBUG"))
-		g_at_chat_set_debug(chat, speedupcdma_debug, debug);
-
-	return chat;
-}
-
-static int speedupcdma_enable(struct ofono_modem *modem)
-{
-	struct speedupcdma_data *data = ofono_modem_get_data(modem);
-
-	DBG("");
-
-	data->modem = open_device(modem, "Modem", "Modem: ");
-	if (data->modem == NULL)
-		return -EINVAL;
-
-	data->aux = open_device(modem, "Aux", "Aux: ");
-	if (data->aux == NULL) {
-		g_at_chat_unref(data->modem);
-		data->modem = NULL;
-		return -EIO;
-	}
-
-	g_at_chat_set_slave(data->modem, data->aux);
-
-	g_at_chat_send(data->modem, "ATE0 &C0 +CMEE=1", NULL, NULL, NULL, NULL);
-	g_at_chat_send(data->aux, "ATE0 &C0 +CMEE=1", NULL, NULL, NULL, NULL);
-
-	g_at_chat_send(data->aux, "AT+CFUN=1", NULL,
-					cfun_enable, modem, NULL);
-
-	return -EINPROGRESS;
-}
-
-static void cfun_disable(gboolean ok, GAtResult *result, gpointer user_data)
-{
-	struct ofono_modem *modem = user_data;
-	struct speedupcdma_data *data = ofono_modem_get_data(modem);
-
-	DBG("");
-
-	g_at_chat_unref(data->aux);
-	data->aux = NULL;
-
-	if (ok)
-		ofono_modem_set_powered(modem, FALSE);
-}
-
-static int speedupcdma_disable(struct ofono_modem *modem)
-{
-	struct speedupcdma_data *data = ofono_modem_get_data(modem);
-
-	DBG("%p", modem);
-
-	g_at_chat_cancel_all(data->modem);
-	g_at_chat_unregister_all(data->modem);
-
-	g_at_chat_unref(data->modem);
-	data->modem = NULL;
-
-	g_at_chat_cancel_all(data->aux);
-	g_at_chat_unregister_all(data->aux);
-
-	g_at_chat_send(data->aux, "AT+CFUN=0", NULL,
-					cfun_disable, modem, NULL);
-
-	return -EINPROGRESS;
-}
-
-static void speedupcdma_pre_sim(struct ofono_modem *modem)
-{
-	struct speedupcdma_data *data = ofono_modem_get_data(modem);
-
-	DBG("%p", modem);
-
-	ofono_devinfo_create(modem, 0, "cdmamodem", data->aux);
-}
-
-static void speedupcdma_post_sim(struct ofono_modem *modem)
-{
-	DBG("%p", modem);
-}
-
-static void speedupcdma_post_online(struct ofono_modem *modem)
-{
-	struct speedupcdma_data *data = ofono_modem_get_data(modem);
-
-	DBG("%p", modem);
-
-	ofono_cdma_netreg_create(modem, 0, "huaweicdmamodem", data->aux);
-
-	ofono_cdma_connman_create(modem, OFONO_VENDOR_HUAWEI, "cdmamodem",
-					data->modem);
-}
-
-static struct ofono_modem_driver speedupcdma_driver = {
-	.name		= "speedupcdma",
-	.probe		= speedupcdma_probe,
-	.remove		= speedupcdma_remove,
-	.enable		= speedupcdma_enable,
-	.disable	= speedupcdma_disable,
-	.pre_sim	= speedupcdma_pre_sim,
-	.post_sim	= speedupcdma_post_sim,
-	.post_online	= speedupcdma_post_online,
-};
-
-static int speedupcdma_init(void)
-{
-	return ofono_modem_driver_register(&speedupcdma_driver);
-}
-
-static void speedupcdma_exit(void)
-{
-	ofono_modem_driver_unregister(&speedupcdma_driver);
-}
-
-OFONO_PLUGIN_DEFINE(speedupcdma, "Speed Up CDMA modem driver", VERSION,
-				OFONO_PLUGIN_PRIORITY_DEFAULT,
-				speedupcdma_init, speedupcdma_exit)
-- 
1.7.1

Re: [PATCH 2/5] speedup: Check for supported modem capabilities first

[Marcel Holtmann]

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

Hi Denis,

> >  plugins/speedup.c |   40 ++++++++++++++++++++++++++++++++++++++--
> >  1 files changed, 38 insertions(+), 2 deletions(-)
> > 
> > diff --git a/plugins/speedup.c b/plugins/speedup.c
> > index a90dfe3..f03f8f3 100644
> > --- a/plugins/speedup.c
> > +++ b/plugins/speedup.c
> > @@ -25,6 +25,7 @@
> >  
> >  #include <errno.h>
> >  #include <stdlib.h>
> > +#include <string.h>
> >  
> >  #include <glib.h>
> >  #include <gatchat.h>
> > @@ -47,11 +48,15 @@
> >  #include <drivers/atmodem/atutil.h>
> >  #include <drivers/atmodem/vendor.h>
> >  
> > +static const char *gcap_prefix[] = { "+GCAP:", NULL };
> > +
> >  struct speedup_data {
> >  	GAtChat *modem;
> >  	GAtChat *aux;
> >  	gboolean have_sim;
> >  	struct at_util_sim_state_query *sim_state_query;
> > +	gboolean have_gsm;
> > +	gboolean have_cdma;
> 
> It might be a good idea to use a single gboolean, or better yet an enum
> here instead.  There's no point to waste 8 bytes when a single byte can do.

I did this on purpose for the Huawei driver. We can unify this later on,
but at this moment I rather see what is actually happening.

Since we keep parsing all capabilities, I wanna avoid that a later one
overwrites a previous one. That said, just using some flags would be
better anyway. Especially since we also always have have_sim as well.

However that can be done as a further optimization in the Speedup and
Huawei drivers.

Regards

Marcel

Re: [PATCH 2/5] speedup: Check for supported modem capabilities first

[Guillaume Zajac]

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

Hi Marcel and Denis,

>>>>   struct speedup_data {
>>>>   	GAtChat *modem;
>>>>   	GAtChat *aux;
>>>>   	gboolean have_sim;
>>>>   	struct at_util_sim_state_query *sim_state_query;
>>>> +	gboolean have_gsm;
>>>> +	gboolean have_cdma;
>>> It might be a good idea to use a single gboolean, or better yet an enum
>>> here instead.  There's no point to waste 8 bytes when a single byte can do.
>> I did this on purpose for the Huawei driver. We can unify this later on,
>> but at this moment I rather see what is actually happening.
>>
>> Since we keep parsing all capabilities, I wanna avoid that a later one
>> overwrites a previous one. That said, just using some flags would be
>> better anyway. Especially since we also always have have_sim as well.
>>
>> However that can be done as a further optimization in the Speedup and
>> Huawei drivers.
>>
> A flagged enum is indeed what I had in mind when I wrote this.  But fair
> enough on your reasoning

As we are starting thinking about tweaking SIM atom to make it work with 
CDMA modem,
maybe having quickly the flagged enum would be helpful to distinguish if 
it is a CDMA or GSM modem that is asking for the SIM atom creation.

Then where should this enum modem_type take place?
Should we tweak the already existing one into modem.h in replacing:

OFONO_MODEM_TYPE_HARDWARE = 0,
OFONO_MODEM_TYPE_HFP,
OFONO_MODEM_TYPE_SAP,

by

OFONO_MODEM_TYPE_UNKNOWN_HARDWARE = 0,
OFONO_MODEM_TYPE_GSM_HARDWARE = 1,
OFONO_MODEM_TYPE_CDMA_HARDWARE = 2,
OFONO_MODEM_TYPE_HFP = 4,
OFONO_MODEM_TYPE_SAP = 8,

Then we have just to specify the modem_type in creating the SIM atom to 
use its limited functionnalities when we have CDMA modem
e.g. manage only PIN protection.

Kind regards,
Guillaume

Re: [PATCH 2/5] speedup: Check for supported modem capabilities first

[Guillaume Zajac]

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

Hi Denis,

>>>>>>    struct speedup_data {
>>>>>>        GAtChat *modem;
>>>>>>        GAtChat *aux;
>>>>>>        gboolean have_sim;
>>>>>>        struct at_util_sim_state_query *sim_state_query;
>>>>>> +    gboolean have_gsm;
>>>>>> +    gboolean have_cdma;
>>>>> It might be a good idea to use a single gboolean, or better yet an enum
>>>>> here instead.  There's no point to waste 8 bytes when a single byte
>>>>> can do.
>>>> I did this on purpose for the Huawei driver. We can unify this later on,
>>>> but at this moment I rather see what is actually happening.
>>>>
>>>> Since we keep parsing all capabilities, I wanna avoid that a later one
>>>> overwrites a previous one. That said, just using some flags would be
>>>> better anyway. Especially since we also always have have_sim as well.
>>>>
>>>> However that can be done as a further optimization in the Speedup and
>>>> Huawei drivers.
>>>>
>>> A flagged enum is indeed what I had in mind when I wrote this.  But fair
>>> enough on your reasoning
>> As we are starting thinking about tweaking SIM atom to make it work with
>> CDMA modem,
>> maybe having quickly the flagged enum would be helpful to distinguish if
>> it is a CDMA or GSM modem that is asking for the SIM atom creation.
>>
>> Then where should this enum modem_type take place?
>> Should we tweak the already existing one into modem.h in replacing:
>>
>> OFONO_MODEM_TYPE_HARDWARE = 0,
>> OFONO_MODEM_TYPE_HFP,
>> OFONO_MODEM_TYPE_SAP,
>>
>> by
>>
>> OFONO_MODEM_TYPE_UNKNOWN_HARDWARE = 0,
>> OFONO_MODEM_TYPE_GSM_HARDWARE = 1,
>> OFONO_MODEM_TYPE_CDMA_HARDWARE = 2,
>> OFONO_MODEM_TYPE_HFP = 4,
>> OFONO_MODEM_TYPE_SAP = 8,
>>
>> Then we have just to specify the modem_type in creating the SIM atom to
>> use its limited functionnalities when we have CDMA modem
>> e.g. manage only PIN protection.
> No, the type information is a kludge that will hamper us in the end, so
> I am against any such changes.  Even the current modem_type enum took
> some convincing and it is meant to be used as a hint for external
> applications, not used internally by oFono core in any way

We can have a private enum modem type into plugin, but we won't be able 
to use it into any atoms.
Maybe by deduction we can add an enum sim_type into sim.h store the type 
into the plugin data and pass it at SIM atom creation.
What do you think?

Kind regards,
Guillaume

Re: [PATCH 2/5] speedup: Check for supported modem capabilities first

[Guillaume Zajac]

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

Hi Denis,

>> We can have a private enum modem type into plugin, but we won't be able
>> to use it into any atoms.
>> Maybe by deduction we can add an enum sim_type into sim.h store the type
>> into the plugin data and pass it at SIM atom creation.
>> What do you think?
> Inside the plugin you can do whatever you want for determining the cdma
> vs gsm modem type.  I'm not quite sure I follow your comment about sim.h...

To use only PIN management and retrieving IMSI from current sim atom, 
sim type (R-UIM or SIM) has to be specified at atom creation.
The sim type can is determined into Huawei plugin at the same time as 
modem type:
     - If modem type if GSM, sim type SIM.
     - If modem type is CDMA, sim is R-UIM (sim atom is not created in 
case of UIM/ROMSIM)

I was proposing to have those 2 types into include/sim.h.
Thus, sim atom can be flagged as SIM or R-UIM to access or not to the 
file system and use PIN management only e.g. in:
sim_initialize() and sim_initialize_after_pin() we just get the imsi we 
don't try to read the sim file system.

Kind regards,
Guillaume

Re: [PATCH 2/5] speedup: Check for supported modem capabilities first

[Denis Kenzior]

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

Hi Guillaume,

On 01/11/2012 04:48 AM, Guillaume Zajac wrote:
> Hi Denis,
> 
>>> We can have a private enum modem type into plugin, but we won't be able
>>> to use it into any atoms.
>>> Maybe by deduction we can add an enum sim_type into sim.h store the type
>>> into the plugin data and pass it at SIM atom creation.
>>> What do you think?
>> Inside the plugin you can do whatever you want for determining the cdma
>> vs gsm modem type.  I'm not quite sure I follow your comment about
>> sim.h...
> 
> To use only PIN management and retrieving IMSI from current sim atom,
> sim type (R-UIM or SIM) has to be specified at atom creation.
> The sim type can is determined into Huawei plugin at the same time as
> modem type:
>     - If modem type if GSM, sim type SIM.
>     - If modem type is CDMA, sim is R-UIM (sim atom is not created in
> case of UIM/ROMSIM)

Yes, I understand that part ;)

> 
> I was proposing to have those 2 types into include/sim.h.
> Thus, sim atom can be flagged as SIM or R-UIM to access or not to the
> file system and use PIN management only e.g. in:
> sim_initialize() and sim_initialize_after_pin() we just get the imsi we
> don't try to read the sim file system.

And no, we're not going to do it this way.  In the end we must be able
to read EFs from CDMA modems as well, since many of the EFs exist on
RUIMs and the logic is exactly the same.  You must find another way.

Regards,
-Denis

Re: [PATCH 2/5] speedup: Check for supported modem capabilities first

[Guillaume Zajac]

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

Hi Denis,

>>>> We can have a private enum modem type into plugin, but we won't be able
>>>> to use it into any atoms.
>>>> Maybe by deduction we can add an enum sim_type into sim.h store the type
>>>> into the plugin data and pass it at SIM atom creation.
>>>> What do you think?
>>> Inside the plugin you can do whatever you want for determining the cdma
>>> vs gsm modem type.  I'm not quite sure I follow your comment about
>>> sim.h...
>> To use only PIN management and retrieving IMSI from current sim atom,
>> sim type (R-UIM or SIM) has to be specified at atom creation.
>> The sim type can is determined into Huawei plugin at the same time as
>> modem type:
>>      - If modem type if GSM, sim type SIM.
>>      - If modem type is CDMA, sim is R-UIM (sim atom is not created in
>> case of UIM/ROMSIM)
> Yes, I understand that part ;)
>
>> I was proposing to have those 2 types into include/sim.h.
>> Thus, sim atom can be flagged as SIM or R-UIM to access or not to the
>> file system and use PIN management only e.g. in:
>> sim_initialize() and sim_initialize_after_pin() we just get the imsi we
>> don't try to read the sim file system.
> And no, we're not going to do it this way.  In the end we must be able
> to read EFs from CDMA modems as well, since many of the EFs exist on
> RUIMs and the logic is exactly the same.  You must find another way

I knew it...
That 's why I have created a new thread that summarize all the concerns 
and ideas we have :)

Kind regards,
Guillaume

Re: [PATCH 0/5] Unify SpeedUp plugin

[Guillaume Zajac]

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

Hi Denis,

On 09/01/2012 14:10, Guillaume Zajac wrote:
> Hi,
>
> Patch 1 is deleting huaweicdma label in udevng.
> I submitted it again into this set of patch to avoid conflicts.
> The 4 next patches are doing same job as for Huawei plugin on SpeedUp plugin.
>
> Kind regards,
> Guillaume
>
> Guillaume Zajac (5):
>    udevng: Remove huaweicdma label from driver_list
>    speedup: Check for supported modem capabilities first
>    speedup: Create GSM/UMTS atoms only if support has been detected
>    speedup: Create CDMA atoms if support has been detected
>    udevng: Simplify vendor and driver list for SpeedUp
>
>   plugins/speedup.c |   99 ++++++++++++++++++++++++++++++++++++++++------------
>   plugins/udevng.c  |    3 --
>   2 files changed, 76 insertions(+), 26 deletions(-)
>
> _______________________________________________
> ofono mailing list
> ofono(a)ofono.org
> http://lists.ofono.org/listinfo/ofono
>

As Marcel gave you some explanations concerning implementation are those 
patches ready to be applied or does it need some other changes?

Kind regards,
Guillaume