[PATCH v2 1/3] Add basic Telit UC864-G support:

[PATCH v2 1/3] Add basic Telit UC864-G support:

[Bernhard.Guillon]

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

From: Bernhard Guillon <Bernhard.Guillon@hale.at>

*add a basic plugin based on different ofono plugins
*use Telit specific QSS for SIM-state
*add Telit to atmodem/vendors
*update Makefile

Co-authored-by: Christopher Vogl <Christopher.Vogl@hale.at>
---
 Makefile.am              |    3 +
 drivers/atmodem/vendor.h |    1 +
 plugins/telit.c          |  384 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 388 insertions(+), 0 deletions(-)
 create mode 100644 plugins/telit.c

diff --git a/Makefile.am b/Makefile.am
index a413a47..6197cf6 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -326,6 +326,9 @@ builtin_sources += plugins/nokiacdma.c
 builtin_modules += linktop
 builtin_sources += plugins/linktop.c
 
+builtin_modules += telit
+builtin_sources += plugins/telit.c
+
 if BLUETOOTH
 builtin_modules += bluetooth
 builtin_sources += plugins/bluetooth.c plugins/bluetooth.h
diff --git a/drivers/atmodem/vendor.h b/drivers/atmodem/vendor.h
index 3898fa8..412bc76 100644
--- a/drivers/atmodem/vendor.h
+++ b/drivers/atmodem/vendor.h
@@ -35,4 +35,5 @@ enum ofono_vendor {
 	OFONO_VENDOR_WAVECOM,
 	OFONO_VENDOR_NOKIA,
 	OFONO_VENDOR_PHONESIM,
+	OFONO_VENDOR_TELIT,
 };
diff --git a/plugins/telit.c b/plugins/telit.c
new file mode 100644
index 0000000..80cca14
--- /dev/null
+++ b/plugins/telit.c
@@ -0,0 +1,384 @@
+/*
+ *
+ *  oFono - Open Source Telephony
+ *
+ *  Copyright (C) 2008-2010  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 <stdio.h>
+#include <unistd.h>
+
+#include <glib.h>
+#include <gatchat.h>
+#include <gattty.h>
+
+#define OFONO_API_SUBJECT_TO_CHANGE
+#include <ofono/plugin.h>
+#include <ofono/log.h>
+#include <ofono/modem.h>
+#include <ofono/call-barring.h>
+#include <ofono/call-forwarding.h>
+#include <ofono/call-meter.h>
+#include <ofono/call-settings.h>
+#include <ofono/devinfo.h>
+#include <ofono/message-waiting.h>
+#include <ofono/netreg.h>
+#include <ofono/phonebook.h>
+#include <ofono/sim.h>
+#include <ofono/gprs.h>
+#include <ofono/gprs-context.h>
+#include <ofono/sms.h>
+#include <ofono/ussd.h>
+#include <ofono/voicecall.h>
+
+#include <drivers/atmodem/vendor.h>
+#include <drivers/atmodem/atutil.h>
+
+static const char *none_prefix[] = { NULL };
+static const char *qss_prefix[] = { "#QSS:", NULL };
+
+struct telit_data {
+	GAtChat *chat;
+	struct ofono_sim *sim;
+};
+
+static void telit_debug(const char *str, void *user_data)
+{
+	const char *prefix = user_data;
+
+	ofono_info("%s%s", prefix, str);
+}
+
+static int telit_probe(struct ofono_modem *modem)
+{
+	struct telit_data *data;
+
+	DBG("%p", modem);
+
+	data = g_try_new0(struct telit_data, 1);
+	if (data == NULL)
+		return -ENOMEM;
+
+	ofono_modem_set_data(modem, data);
+
+	return 0;
+}
+
+static void telit_remove(struct ofono_modem *modem)
+{
+	struct telit_data *data = ofono_modem_get_data(modem);
+
+	DBG("%p", modem);
+
+	ofono_modem_set_data(modem, NULL);
+
+	if (data->chat)
+		g_at_chat_unref(data->chat);
+
+	g_free(data);
+}
+
+static void telit_qss_notify(GAtResult *result, gpointer user_data)
+{
+	struct ofono_modem *modem = user_data;
+	struct telit_data *data = ofono_modem_get_data(modem);
+	int status;
+	GAtResultIter iter;
+	g_at_result_iter_init(&iter, result);
+
+	if (!g_at_result_iter_next(&iter, "#QSS:"))
+		return;
+
+	g_at_result_iter_next_number(&iter, &status);
+
+	switch(status) {
+	case 0:
+		DBG("SIM not inserted");
+		ofono_sim_inserted_notify(data->sim,FALSE);
+		break;
+	case 1:
+		DBG("SIM inserted");
+		/* We need to sleep a bit */
+		sleep(1);
+		ofono_sim_inserted_notify(data->sim,TRUE);
+		break;
+	case 2:
+		DBG("SIM inserted and PIN unlocked");
+		break;
+	case 3:
+		DBG("SIM inserted and ready");
+		break;
+	default:
+		return;
+	}
+	return;
+}
+
+static void telit_qss_cb(gboolean ok, GAtResult *result, gpointer user_data)
+{
+	struct ofono_modem *modem = user_data;
+	struct telit_data *data = ofono_modem_get_data(modem);
+	int mode;
+	int status;
+	GAtResultIter iter;
+	g_at_result_iter_init(&iter, result);
+
+	if (!g_at_result_iter_next(&iter, "#QSS:"))
+		return;
+
+	g_at_result_iter_next_number(&iter, &mode);
+	g_at_result_iter_next_number(&iter, &status);
+
+	switch(status) {
+	case 0:
+		DBG("SIM not inserted");
+		break;
+	case 1:
+		DBG("SIM inserted");
+		ofono_sim_inserted_notify(data->sim,TRUE);
+		break;
+	case 2:
+		DBG("SIM inserted and PIN unlocked");
+		break;
+	case 3:
+		DBG("SIM inserted and ready");
+		break;
+	default:
+		return;
+	}
+}
+
+static void cfun_enable_cb(gboolean ok, GAtResult *result, gpointer user_data)
+{
+	struct ofono_modem *modem = user_data;
+	struct telit_data *data = ofono_modem_get_data(modem);
+
+	DBG("");
+
+	if (ok)
+		ofono_modem_set_powered(modem, TRUE);
+
+	/* Enable sim state notification */
+	g_at_chat_send(data->chat, "AT#QSS=1", none_prefix, NULL, NULL, NULL);
+
+	/* Follow sim state */
+	g_at_chat_register(data->chat, "#QSS:", telit_qss_notify,
+				FALSE, modem, NULL);
+
+	/* Query current sim state */
+	g_at_chat_send(data->chat, "AT#QSS?", qss_prefix,
+				telit_qss_cb, modem, NULL);
+}
+
+static void cfun_disable_cb(gboolean ok, GAtResult *result, gpointer user_data)
+{
+	struct ofono_modem *modem = user_data;
+	struct telit_data *data = ofono_modem_get_data(modem);
+
+	DBG("");
+
+	g_at_chat_unref(data->chat);
+	data->chat = NULL;
+
+	if (ok)
+		ofono_modem_set_powered(modem, FALSE);
+}
+
+static GAtChat *open_device(struct ofono_modem *modem,
+				const char *key, char *debug)
+{
+	const char *device;
+	GAtSyntax *syntax;
+	GIOChannel *channel;
+	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_gsmv1();
+	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, telit_debug, debug);
+
+	return chat;
+}
+
+static int telit_enable(struct ofono_modem *modem)
+{
+	struct telit_data *data = ofono_modem_get_data(modem);
+
+	DBG("%p", modem);
+
+	data->chat = open_device(modem, "Modem", "Modem: ");
+	if (data->chat == NULL)
+		return -EINVAL;
+
+	/*
+	 * Disable command echo and
+	 * enable the Extended Error Result Codes
+	 */
+	g_at_chat_send(data->chat, "ATE0 +CMEE=1", none_prefix,
+				NULL, NULL, NULL);
+
+	/* Set phone functionality */
+	g_at_chat_send(data->chat, "AT+CFUN=4", none_prefix,
+				cfun_enable_cb, modem, NULL);
+
+	return -EINPROGRESS;
+}
+
+static int telit_disable(struct ofono_modem *modem)
+{
+	struct telit_data *data = ofono_modem_get_data(modem);
+	DBG("%p", modem);
+
+	if (data->chat == NULL)
+		return 0;
+
+	g_at_chat_cancel_all(data->chat);
+	g_at_chat_unregister_all(data->chat);
+
+	/* Power down modem */
+	g_at_chat_send(data->chat, "AT+CFUN=0", none_prefix,
+				cfun_disable_cb, modem, NULL);
+
+	return -EINPROGRESS;
+}
+
+static void set_online_cb(gboolean ok, GAtResult *result, gpointer user_data)
+{
+	struct cb_data *cbd = user_data;
+	ofono_modem_online_cb_t cb = cbd->cb;
+
+	if (ok)
+		CALLBACK_WITH_SUCCESS(cb, cbd->data);
+	else
+		CALLBACK_WITH_FAILURE(cb, cbd->data);
+}
+
+static void telit_set_online(struct ofono_modem *modem, ofono_bool_t online,
+				ofono_modem_online_cb_t cb, void *user_data)
+{
+	struct telit_data *data = ofono_modem_get_data(modem);
+	struct cb_data *cbd = cb_data_new(cb, user_data);
+	char const *command = online ? "AT+CFUN=1" : "AT+CFUN=4";
+
+	DBG("modem %p %s", modem, online ? "online" : "offline");
+
+	if (data->chat == NULL)
+		goto error;
+
+	if (g_at_chat_send(data->chat, command, none_prefix,
+			set_online_cb, cbd, g_free))
+		return;
+
+error:
+	g_free(cbd);
+
+	CALLBACK_WITH_FAILURE(cb, cbd->data);
+}
+
+static void telit_pre_sim(struct ofono_modem *modem)
+{
+	struct telit_data *data = ofono_modem_get_data(modem);
+
+	DBG("%p", modem);
+
+	ofono_devinfo_create(modem, 0, "atmodem", data->chat);
+	data->sim = ofono_sim_create(modem, 0, "atmodem", data->chat);
+	ofono_voicecall_create(modem, 0, "atmodem", data->chat);
+}
+
+static void telit_post_sim(struct ofono_modem *modem)
+{
+	struct telit_data *data = ofono_modem_get_data(modem);
+
+	DBG("%p", modem);
+
+	ofono_sms_create(modem, 0, "atmodem", data->chat);
+}
+
+static void telit_post_online(struct ofono_modem *modem)
+{
+	struct telit_data *data = ofono_modem_get_data(modem);
+	struct ofono_message_waiting *mw;
+	struct ofono_gprs *gprs;
+	struct ofono_gprs_context *gc;
+
+	DBG("%p", modem);
+
+	ofono_ussd_create(modem, 0, "atmodem", data->chat);
+	ofono_call_forwarding_create(modem, 0, "atmodem", data->chat);
+	ofono_call_settings_create(modem, 0, "atmodem", data->chat);
+	ofono_netreg_create(modem, OFONO_VENDOR_TELIT, "atmodem", data->chat);
+	ofono_call_meter_create(modem, 0, "atmodem", data->chat);
+	ofono_call_barring_create(modem, 0, "atmodem", data->chat);
+
+	gprs = ofono_gprs_create(modem, 0, "atmodem", data->chat);
+	gc = ofono_gprs_context_create(modem, 0, "atmodem", data->chat);
+
+	if (gprs && gc)
+		ofono_gprs_add_context(gprs, gc);
+
+	mw = ofono_message_waiting_create(modem);
+	if (mw)
+		ofono_message_waiting_register(mw);
+}
+
+static struct ofono_modem_driver telit_driver = {
+	.name			= "telit",
+	.probe			= telit_probe,
+	.remove			= telit_remove,
+	.enable			= telit_enable,
+	.disable		= telit_disable,
+	.set_online		= telit_set_online,
+	.pre_sim		= telit_pre_sim,
+	.post_sim		= telit_post_sim,
+	.post_online	= telit_post_online,
+};
+
+static int telit_init(void)
+{
+	return ofono_modem_driver_register(&telit_driver);
+}
+
+static void telit_exit(void)
+{
+	ofono_modem_driver_unregister(&telit_driver);
+}
+
+OFONO_PLUGIN_DEFINE(telit, "telit driver", VERSION,
+		OFONO_PLUGIN_PRIORITY_DEFAULT, telit_init, telit_exit)
-- 
1.7.0.4



--
Scanned by MailScanner.

[PATCH v2 2/3] network-registration.c: implement CIND forTelit UC864-G

[Bernhard.Guillon]

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

From: Bernhard Guillon <Bernhard.Guillon@hale.at>

*UC864 has an incompatible CIND
*add a Telit specific check for "not measurable" strength

Co-authored-by: Christopher Vogl <Christopher.Vogl@hale.at>
---
 drivers/atmodem/network-registration.c |   40 +++++++++++++++++++++++++++++--
 1 files changed, 37 insertions(+), 3 deletions(-)

diff --git a/drivers/atmodem/network-registration.c b/drivers/atmodem/network-registration.c
index b3aa511..984e85e 100644
--- a/drivers/atmodem/network-registration.c
+++ b/drivers/atmodem/network-registration.c
@@ -666,7 +666,20 @@ static void ciev_notify(GAtResult *result, gpointer user_data)
 	if (!g_at_result_iter_next_number(&iter, &strength))
 		return;
 
-	strength = (strength * 100) / (nd->signal_max - nd->signal_min);
+	switch (nd->vendor) {
+	case OFONO_VENDOR_TELIT:
+		/* Check for "signal not measurable" */
+		if (strength == 99)
+			strength = 0;
+		else
+			strength = (strength * 100) / (nd->signal_max -
+						nd->signal_min);
+		break;
+	default:
+		strength = (strength * 100) / (nd->signal_max - nd->signal_min);
+		break;
+	}
+
 	ofono_netreg_strength_notify(netreg, strength);
 }
 
@@ -798,8 +811,19 @@ static void cind_cb(gboolean ok, GAtResult *result, gpointer user_data)
 
 	g_at_result_iter_next_number(&iter, &strength);
 
-	strength = (strength * 100) / (nd->signal_max - nd->signal_min);
-
+	switch (nd->vendor) {
+	case OFONO_VENDOR_TELIT:
+		/* Check for "signal not measurable" */
+		if (strength == 99)
+			strength = 0;
+		else
+			strength = (strength * 100) / (nd->signal_max -
+						nd->signal_min);
+		break;
+	default:
+		strength = (strength * 100) / (nd->signal_max - nd->signal_min);
+		break;
+	}
 	cb(&error, strength, cbd->data);
 }
 
@@ -1302,6 +1326,16 @@ static void at_creg_set_cb(gboolean ok, GAtResult *result, gpointer user_data)
 	case OFONO_VENDOR_NOKIA:
 		/* Signal strength reporting via CIND is not supported */
 		break;
+	case OFONO_VENDOR_TELIT:
+		/* Use RSSI instead of signal.*/
+		nd->signal_index = 9;
+		nd->signal_min = 0;
+		nd->signal_max = 5;
+		g_at_chat_send(nd->chat, "AT+CIND=1", cind_prefix,
+						NULL, NULL, NULL);
+		g_at_chat_register(nd->chat, "+CIEV:", ciev_notify,
+					FALSE, netreg, NULL);
+		break;
 	default:
 		g_at_chat_send(nd->chat, "AT+CIND=?", cind_prefix,
 				cind_support_cb, netreg, NULL);
-- 
1.7.0.4



--
Scanned by MailScanner.

Re: [PATCH v2 2/3] network-registration.c: implement CIND forTelit UC864-G

[Marcel Holtmann]

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

Hi Bernhard,

> *UC864 has an incompatible CIND
> *add a Telit specific check for "not measurable" strength
> 
> Co-authored-by: Christopher Vogl <Christopher.Vogl@hale.at>
> ---
>  drivers/atmodem/network-registration.c |   40 +++++++++++++++++++++++++++++--
>  1 files changed, 37 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/atmodem/network-registration.c b/drivers/atmodem/network-registration.c
> index b3aa511..984e85e 100644
> --- a/drivers/atmodem/network-registration.c
> +++ b/drivers/atmodem/network-registration.c
> @@ -666,7 +666,20 @@ static void ciev_notify(GAtResult *result, gpointer user_data)
>  	if (!g_at_result_iter_next_number(&iter, &strength))
>  		return;
>  
> -	strength = (strength * 100) / (nd->signal_max - nd->signal_min);
> +	switch (nd->vendor) {
> +	case OFONO_VENDOR_TELIT:
> +		/* Check for "signal not measurable" */
> +		if (strength == 99)
> +			strength = 0;
> +		else
> +			strength = (strength * 100) / (nd->signal_max -
> +						nd->signal_min);
> +		break;
> +	default:
> +		strength = (strength * 100) / (nd->signal_max - nd->signal_min);
> +		break;
> +	}
> +

this is something that does not look like vendor quirk. The 99 is always
a special value and we should just do this for all modems.

Denis, any thoughts?

>  	ofono_netreg_strength_notify(netreg, strength);
>  }
>  
> @@ -798,8 +811,19 @@ static void cind_cb(gboolean ok, GAtResult *result, gpointer user_data)
>  
>  	g_at_result_iter_next_number(&iter, &strength);
>  
> -	strength = (strength * 100) / (nd->signal_max - nd->signal_min);
> -
> +	switch (nd->vendor) {
> +	case OFONO_VENDOR_TELIT:
> +		/* Check for "signal not measurable" */
> +		if (strength == 99)
> +			strength = 0;
> +		else
> +			strength = (strength * 100) / (nd->signal_max -
> +						nd->signal_min);
> +		break;
> +	default:
> +		strength = (strength * 100) / (nd->signal_max - nd->signal_min);
> +		break;
> +	}
>  	cb(&error, strength, cbd->data);
>  }

If we really need quirks, then this code should be extracted into its
own function. However right now I would say to just treat 99 special for
all modems.

> @@ -1302,6 +1326,16 @@ static void at_creg_set_cb(gboolean ok, GAtResult *result, gpointer user_data)
>  	case OFONO_VENDOR_NOKIA:
>  		/* Signal strength reporting via CIND is not supported */
>  		break;
> +	case OFONO_VENDOR_TELIT:
> +		/* Use RSSI instead of signal.*/
> +		nd->signal_index = 9;
> +		nd->signal_min = 0;
> +		nd->signal_max = 5;
> +		g_at_chat_send(nd->chat, "AT+CIND=1", cind_prefix,
> +						NULL, NULL, NULL);
> +		g_at_chat_register(nd->chat, "+CIEV:", ciev_notify,
> +					FALSE, netreg, NULL);
> +		break;
>  	default:
>  		g_at_chat_send(nd->chat, "AT+CIND=?", cind_prefix,
>  				cind_support_cb, netreg, NULL);

Regards

Marcel

Re: [PATCH v2 2/3] network-registration.c: implement CIND forTelit UC864-G

[Denis Kenzior]

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

Hi Marcel,

On 05/25/2011 02:08 PM, Marcel Holtmann wrote:
> Hi Bernhard,
> 
>> *UC864 has an incompatible CIND
>> *add a Telit specific check for "not measurable" strength
>>
>> Co-authored-by: Christopher Vogl <Christopher.Vogl@hale.at>
>> ---
>>  drivers/atmodem/network-registration.c |   40 +++++++++++++++++++++++++++++--
>>  1 files changed, 37 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/atmodem/network-registration.c b/drivers/atmodem/network-registration.c
>> index b3aa511..984e85e 100644
>> --- a/drivers/atmodem/network-registration.c
>> +++ b/drivers/atmodem/network-registration.c
>> @@ -666,7 +666,20 @@ static void ciev_notify(GAtResult *result, gpointer user_data)
>>  	if (!g_at_result_iter_next_number(&iter, &strength))
>>  		return;
>>  
>> -	strength = (strength * 100) / (nd->signal_max - nd->signal_min);
>> +	switch (nd->vendor) {
>> +	case OFONO_VENDOR_TELIT:
>> +		/* Check for "signal not measurable" */
>> +		if (strength == 99)
>> +			strength = 0;
>> +		else
>> +			strength = (strength * 100) / (nd->signal_max -
>> +						nd->signal_min);
>> +		break;
>> +	default:
>> +		strength = (strength * 100) / (nd->signal_max - nd->signal_min);
>> +		break;
>> +	}
>> +
> 
> this is something that does not look like vendor quirk. The 99 is always
> a special value and we should just do this for all modems.
> 
> Denis, any thoughts?
> 

99 is used by +CSQ and certain vendor extensions to CSQ to denote an
unmeasurable / unknown RSSI value.  This is already handled by
at_util_convert_signal_strength.

Some modems / protocols (e.g. mbm, hfp) use CIND style indicators for
signal strength instead, however the indicator values are contiguous
(e.g. 0-5) and oFono's logic assumes this behavior.

It seems that the Telit modem uses a contiguous range (0-5) and the
special value 99 to denote unmeasurable value.  The Telit modem is the
first one I have seen that uses this behavior.  From that standpoint I
really have no preference here, but it seems that generalizing the logic
to handle 99 should be easy enough.

However, I really don't like this patch hardcoding the signal
index/min/max inside at_creg_set_cb.  Telit is a nicely compliant modem,
we should be able to trust what it reports.

Bernhard, a question for you: what does the UC864-G return when issuing:
'AT+CIND=?'

Regards,
-Denis

Re: [PATCH v2 2/3] network-registration.c: implement CIND forTelitUC864-G

[Bernhard Guillon]

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



On Tue, 24 May 2011, Denis Kenzior wrote:

> However, I really don't like this patch hardcoding the signal
> index/min/max inside at_creg_set_cb.  Telit is a nicely compliant modem,
> we should be able to trust what it reports.
>
> Bernhard, a question for you: what does the UC864-G return when issuing:
> 'AT+CIND=?'

Hi,

AT+CIND=?
+CIND: 
(("battchg",(0-5,99)),("signal",(0-7,99)),("service",(0,1)),("sounder",(0,1)),("message",(0,1)),("call",(0,1)),("roam",(0,1)),("smsfull",(0,1)),("rssi",(0-5,99)))

I reworked the network-registration part and made it more general. Patch 
follows...

Best regards,
Bernahrd


--
Scanned by MailScanner.

[PATCH] network-registration.c: enhance CIND=? support

[Bernhard.Guillon]

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

From: Bernhard Guillon <Bernhard.Guillon@hale.at>

*add support for CIND=? tokens like ("signal",(0-7,99))
*add support for token encapsulation e.g.
    (("one",(0-7,99)),("two",(0-7,99)))
---
 drivers/atmodem/network-registration.c |   51 ++++++++++++++++++++++++++++---
 1 files changed, 46 insertions(+), 5 deletions(-)

diff --git a/drivers/atmodem/network-registration.c b/drivers/atmodem/network-registration.c
index b3aa511..18f2299 100644
--- a/drivers/atmodem/network-registration.c
+++ b/drivers/atmodem/network-registration.c
@@ -56,6 +56,7 @@ struct netreg_data {
 	int signal_index; /* If strength is reported via CIND */
 	int signal_min; /* min strength reported via CIND */
 	int signal_max; /* max strength reported via CIND */
+	int signal_invalid; /* invalid strength reported via CIND */
 	int tech;
 	struct ofono_network_time time;
 	guint nitz_timeout;
@@ -666,7 +667,10 @@ static void ciev_notify(GAtResult *result, gpointer user_data)
 	if (!g_at_result_iter_next_number(&iter, &strength))
 		return;
 
-	strength = (strength * 100) / (nd->signal_max - nd->signal_min);
+	if (strength == nd->signal_invalid)
+		strength = -1;
+	else
+		strength = (strength * 100) / (nd->signal_max - nd->signal_min);
 	ofono_netreg_strength_notify(netreg, strength);
 }
 
@@ -798,7 +802,10 @@ static void cind_cb(gboolean ok, GAtResult *result, gpointer user_data)
 
 	g_at_result_iter_next_number(&iter, &strength);
 
-	strength = (strength * 100) / (nd->signal_max - nd->signal_min);
+	if (strength == nd->signal_invalid)
+		strength = -1;
+	else
+		strength = (strength * 100) / (nd->signal_max - nd->signal_min);
 
 	cb(&error, strength, cbd->data);
 }
@@ -1133,7 +1140,9 @@ static void cind_support_cb(gboolean ok, GAtResult *result, gpointer user_data)
 	GAtResultIter iter;
 	const char *str;
 	int index;
-	int min, max;
+	int min = 0;
+	int max = 0;
+	int tmp_min, tmp_max, invalid;
 
 	if (!ok)
 		goto error;
@@ -1145,14 +1154,45 @@ static void cind_support_cb(gboolean ok, GAtResult *result, gpointer user_data)
 	index = 1;
 
 	while (g_at_result_iter_open_list(&iter)) {
+		/*
+		 * Some modems encapsulate the result list in braces
+		 * So we just skip the opening brace in this case.
+		 * We do not need to care about the closing one.
+		 */
+		g_at_result_iter_open_list(&iter);
+
+		/* Reset invalid default value for every token*/
+		invalid = 99;
+
 		if (!g_at_result_iter_next_string(&iter, &str))
 			goto error;
 
 		if (!g_at_result_iter_open_list(&iter))
 			goto error;
 
-		while (g_at_result_iter_next_range(&iter, &min, &max))
-			;
+		while (g_at_result_iter_next_range(&iter, &tmp_min, &tmp_max)) {
+			/*
+			 * This part of code makes havy use of the implementation
+			 * details of g_at_result_iter_next_range()
+			 * We currently support this token type ("signal",(0-5))
+			 * While tokens like this ("call",(0,1))
+			 * will be parsed to the end to get to the next token.
+			 * With the same idea we can support tokens like
+			 * ("signal",(0-7,99)) it is unlikely that a valid
+			 * token is "signal",(0-0) so if both values are the
+			 * same we know we are at the 99 case of (0-7,99)
+			 * This implementation only works because of how the 
+			 * g_at_result_iter_next_range() function is implemented
+			 * if it changes this code will break.
+			 */
+			if(tmp_min!=tmp_max) {
+				min=tmp_min;
+				max=tmp_max;
+			}
+			else {
+				invalid=tmp_min;
+			}
+		}
 
 		if (!g_at_result_iter_close_list(&iter))
 			goto error;
@@ -1164,6 +1204,7 @@ static void cind_support_cb(gboolean ok, GAtResult *result, gpointer user_data)
 			nd->signal_index = index;
 			nd->signal_min = min;
 			nd->signal_max = max;
+			nd->signal_invalid = invalid;
 		}
 
 		index += 1;
-- 
1.7.0.4



--
Scanned by MailScanner.

Re: [PATCH] network-registration.c: enhance CIND=? support

[Denis Kenzior]

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

Hi Bernhard,

On 06/07/2011 07:04 AM, Bernhard.Guillon(a)hale.at wrote:
> From: Bernhard Guillon <Bernhard.Guillon@hale.at>
> 
> *add support for CIND=? tokens like ("signal",(0-7,99))
> *add support for token encapsulation e.g.
>     (("one",(0-7,99)),("two",(0-7,99)))

Err, ok, so we can forget about what I said of Telit being a nicely
compliant modem ;)  There are like 5 examples of this in 27.007 and they
still manage to get this wrong.

> ---
>  drivers/atmodem/network-registration.c |   51 ++++++++++++++++++++++++++++---
>  1 files changed, 46 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/atmodem/network-registration.c b/drivers/atmodem/network-registration.c
> index b3aa511..18f2299 100644
> --- a/drivers/atmodem/network-registration.c
> +++ b/drivers/atmodem/network-registration.c
> @@ -56,6 +56,7 @@ struct netreg_data {
>  	int signal_index; /* If strength is reported via CIND */
>  	int signal_min; /* min strength reported via CIND */
>  	int signal_max; /* max strength reported via CIND */
> +	int signal_invalid; /* invalid strength reported via CIND */
>  	int tech;
>  	struct ofono_network_time time;
>  	guint nitz_timeout;
> @@ -666,7 +667,10 @@ static void ciev_notify(GAtResult *result, gpointer user_data)
>  	if (!g_at_result_iter_next_number(&iter, &strength))
>  		return;
>  
> -	strength = (strength * 100) / (nd->signal_max - nd->signal_min);
> +	if (strength == nd->signal_invalid)
> +		strength = -1;
> +	else
> +		strength = (strength * 100) / (nd->signal_max - nd->signal_min);
>  	ofono_netreg_strength_notify(netreg, strength);

I'm fine with this change, but please respect our coding style, in
particular item M1 of doc/coding-style.txt

>  }
>  
> @@ -798,7 +802,10 @@ static void cind_cb(gboolean ok, GAtResult *result, gpointer user_data)
>  
>  	g_at_result_iter_next_number(&iter, &strength);
>  
> -	strength = (strength * 100) / (nd->signal_max - nd->signal_min);
> +	if (strength == nd->signal_invalid)
> +		strength = -1;
> +	else
> +		strength = (strength * 100) / (nd->signal_max - nd->signal_min);
>  
>  	cb(&error, strength, cbd->data);
>  }
> @@ -1133,7 +1140,9 @@ static void cind_support_cb(gboolean ok, GAtResult *result, gpointer user_data)
>  	GAtResultIter iter;
>  	const char *str;
>  	int index;
> -	int min, max;
> +	int min = 0;
> +	int max = 0;
> +	int tmp_min, tmp_max, invalid;
>  
>  	if (!ok)
>  		goto error;
> @@ -1145,14 +1154,45 @@ static void cind_support_cb(gboolean ok, GAtResult *result, gpointer user_data)
>  	index = 1;
>  
>  	while (g_at_result_iter_open_list(&iter)) {
> +		/*
> +		 * Some modems encapsulate the result list in braces
> +		 * So we just skip the opening brace in this case.
> +		 * We do not need to care about the closing one.
> +		 */
> +		g_at_result_iter_open_list(&iter);
> +

This looks wrong.  This behavior is peculiar to the Telit modem, so you
should open the list outside the while loop and quirk it.

e.g.

if (vendor == OFONO_VENDOR_TELIT)
	g_at_result_iter_open_list(&iter);

while (g_at_result_iter_open_list(&iter)
	...

if (vendor == OFONO_VENDOR_TELIT)
	g_at_result_iter_close_list(&iter);

> +		/* Reset invalid default value for every token*/
> +		invalid = 99;
> +
>  		if (!g_at_result_iter_next_string(&iter, &str))
>  			goto error;
>  
>  		if (!g_at_result_iter_open_list(&iter))
>  			goto error;
>  
> -		while (g_at_result_iter_next_range(&iter, &min, &max))
> -			;
> +		while (g_at_result_iter_next_range(&iter, &tmp_min, &tmp_max)) {
> +			/*
> +			 * This part of code makes havy use of the implementation
> +			 * details of g_at_result_iter_next_range()
> +			 * We currently support this token type ("signal",(0-5))
> +			 * While tokens like this ("call",(0,1))
> +			 * will be parsed to the end to get to the next token.
> +			 * With the same idea we can support tokens like
> +			 * ("signal",(0-7,99)) it is unlikely that a valid
> +			 * token is "signal",(0-0) so if both values are the
> +			 * same we know we are at the 99 case of (0-7,99)
> +			 * This implementation only works because of how the 
> +			 * g_at_result_iter_next_range() function is implemented
> +			 * if it changes this code will break.
> +			 */

This comment is not really needed, there are no peculiarities of
implementation.  The current implementation simply does not consider the
possibility of signal being anything but (0-n)

> +			if(tmp_min!=tmp_max) {
> +				min=tmp_min;
> +				max=tmp_max;
> +			}
> +			else {
> +				invalid=tmp_min;
> +			}
> +		}

The coding style here is completely wrong, please review
doc/coding-style.txt and fix this accordingly. e.g. spaces after if,
spaces before & after arithmetic operations and assignments, else on the
same line as the closing brace, etc, etc, etc.

>  
>  		if (!g_at_result_iter_close_list(&iter))
>  			goto error;
> @@ -1164,6 +1204,7 @@ static void cind_support_cb(gboolean ok, GAtResult *result, gpointer user_data)
>  			nd->signal_index = index;
>  			nd->signal_min = min;
>  			nd->signal_max = max;
> +			nd->signal_invalid = invalid;
>  		}
>  
>  		index += 1;

Regards,
-Denis

Re: [PATCH] network-registration.c: enhance CIND=? support

[Bernhard Guillon]

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



On Tue, 7 Jun 2011, Bernhard.Guillon(a)hale.at wrote:

> From: Bernhard Guillon <Bernhard.Guillon@hale.at>
>
> *add support for CIND=? tokens like ("signal",(0-7,99))
> *add support for token encapsulation e.g.
>    (("one",(0-7,99)),("two",(0-7,99)))
> ---
> drivers/atmodem/network-registration.c |   51 ++++++++++++++++++++++++++++---
> 1 files changed, 46 insertions(+), 5 deletions(-)


Tested with:

Telit:
AT+CIND=?
+CIND: (("battchg",(0-5,99)),("signal",(0-7,99)),("service",(0,1)),("sounder",(0,1)),("message",(0,1)),("call",(0,1)),("roam",(0,1)),("smsfull",(0,1)),("rssi",(0-5,99)))

and some kind of Motorola from wikipedia [1]:

+CIND: ("Voice Mail",(0,1)),("service",(0,1)),("call",(0,1)),("Roam",(0-2)),("signal",(0-5)),("callsetup",(0-3)),("smsfull",(0,1))

Best regards,
Bernhard

1 http://en.wikipedia.org/wiki/Motorola_phone_AT_commands


--
Scanned by MailScanner.

[PATCH v2 3/3] udev: add Telit UC864-G and update udev rules

[Bernhard.Guillon]

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

From: Bernhard Guillon <Bernhard.Guillon@hale.at>

*for now only check for Modem, add GPS later

Co-authored-by: Christopher Vogl <Christopher.Vogl@hale.at>
---
 plugins/ofono.rules |    8 ++++++++
 plugins/udev.c      |   31 +++++++++++++++++++++++++++++++
 2 files changed, 39 insertions(+), 0 deletions(-)

diff --git a/plugins/ofono.rules b/plugins/ofono.rules
index 5a36380..87ef065 100644
--- a/plugins/ofono.rules
+++ b/plugins/ofono.rules
@@ -344,6 +344,14 @@ ATTRS{idVendor}=="12d1", ATTRS{idProduct}=="1485", ENV{OFONO_IFACE_NUM}=="02", E
 ATTRS{idVendor}=="12d1", ATTRS{idProduct}=="1486", ENV{OFONO_IFACE_NUM}=="00", ENV{OFONO_HUAWEI_TYPE}="Modem"
 ATTRS{idVendor}=="12d1", ATTRS{idProduct}=="1486", ENV{OFONO_IFACE_NUM}=="02", ENV{OFONO_HUAWEI_TYPE}="Pcui"
 
+#Telit UC864-G
+ATTRS{idVendor}=="1bc7", ATTRS{idProduct}=="1004", ENV{OFONO_IFACE_NUM}=="00", ENV{OFONO_TELIT_TYPE}="Modem"
+ATTRS{idVendor}=="1bc7", ATTRS{idProduct}=="1004", ENV{OFONO_IFACE_NUM}=="01", ENV{OFONO_TELIT_TYPE}="Aux"
+ATTRS{idVendor}=="1bc7", ATTRS{idProduct}=="1004", ENV{OFONO_IFACE_NUM}=="02", ENV{OFONO_TELIT_TYPE}="GPS"
+ATTRS{idVendor}=="1bc7", ATTRS{idProduct}=="1004", ENV{OFONO_IFACE_NUM}=="03", ENV{OFONO_TELIT_TYPE}="UART"
+
+ATTRS{idVendor}=="1bc7", ENV{OFONO_DRIVER}="telit"
+
 LABEL="ofono_tty_end"
 
 # ISI/Phonet drivers
diff --git a/plugins/udev.c b/plugins/udev.c
index cbb596d..28fff72 100644
--- a/plugins/udev.c
+++ b/plugins/udev.c
@@ -588,6 +588,35 @@ static void add_linktop(struct ofono_modem *modem,
 	}
 }
 
+static void add_telit(struct ofono_modem *modem,
+					struct udev_device *udev_device)
+{
+	struct udev_list_entry *entry;
+	const char *devnode;
+	gboolean found = FALSE;
+
+	DBG("modem %p", modem);
+
+	entry = udev_device_get_properties_list_entry(udev_device);
+	while (entry) {
+		const char *name = udev_list_entry_get_name(entry);
+		const char *value = udev_list_entry_get_value(entry);
+
+		if (g_str_equal(name, "OFONO_TELIT_TYPE") == TRUE &&
+					g_str_equal(value, "Modem") == TRUE) {
+			found = TRUE;
+		}
+		entry = udev_list_entry_get_next(entry);
+	}
+
+	if (found == FALSE)
+		return;
+
+	devnode = udev_device_get_devnode(udev_device);
+	ofono_modem_set_string(modem, "Modem", devnode);
+	ofono_modem_register(modem);
+}
+
 static void add_modem(struct udev_device *udev_device)
 {
 	struct ofono_modem *modem;
@@ -682,6 +711,8 @@ done:
 		add_calypso(modem, udev_device);
 	else if (g_strcmp0(driver, "tc65") == 0)
 		add_tc65(modem, udev_device);
+	else if (g_strcmp0(driver, "telit") == 0)
+		add_telit(modem, udev_device);
 	else if (g_strcmp0(driver, "nokiacdma") == 0)
 		add_nokiacdma(modem, udev_device);
         else if (g_strcmp0(driver, "linktop") == 0)
-- 
1.7.0.4



--
Scanned by MailScanner.

Re: [PATCH v2 3/3] udev: add Telit UC864-G and update udev rules

[Marcel Holtmann]

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

Hi Bernard,

> *for now only check for Modem, add GPS later
> 
> Co-authored-by: Christopher Vogl <Christopher.Vogl@hale.at>
> ---
>  plugins/ofono.rules |    8 ++++++++
>  plugins/udev.c      |   31 +++++++++++++++++++++++++++++++
>  2 files changed, 39 insertions(+), 0 deletions(-)
> 
> diff --git a/plugins/ofono.rules b/plugins/ofono.rules
> index 5a36380..87ef065 100644
> --- a/plugins/ofono.rules
> +++ b/plugins/ofono.rules
> @@ -344,6 +344,14 @@ ATTRS{idVendor}=="12d1", ATTRS{idProduct}=="1485", ENV{OFONO_IFACE_NUM}=="02", E
>  ATTRS{idVendor}=="12d1", ATTRS{idProduct}=="1486", ENV{OFONO_IFACE_NUM}=="00", ENV{OFONO_HUAWEI_TYPE}="Modem"
>  ATTRS{idVendor}=="12d1", ATTRS{idProduct}=="1486", ENV{OFONO_IFACE_NUM}=="02", ENV{OFONO_HUAWEI_TYPE}="Pcui"
>  
> +#Telit UC864-G
> +ATTRS{idVendor}=="1bc7", ATTRS{idProduct}=="1004", ENV{OFONO_IFACE_NUM}=="00", ENV{OFONO_TELIT_TYPE}="Modem"
> +ATTRS{idVendor}=="1bc7", ATTRS{idProduct}=="1004", ENV{OFONO_IFACE_NUM}=="01", ENV{OFONO_TELIT_TYPE}="Aux"
> +ATTRS{idVendor}=="1bc7", ATTRS{idProduct}=="1004", ENV{OFONO_IFACE_NUM}=="02", ENV{OFONO_TELIT_TYPE}="GPS"
> +ATTRS{idVendor}=="1bc7", ATTRS{idProduct}=="1004", ENV{OFONO_IFACE_NUM}=="03", ENV{OFONO_TELIT_TYPE}="UART"
> +

I can actually look this up, but which ones are you planning to use. I
can see the Modem, Aux and GPS, but what is the UART for?

> +ATTRS{idVendor}=="1bc7", ENV{OFONO_DRIVER}="telit"
> +
>  LABEL="ofono_tty_end"
>  
>  # ISI/Phonet drivers
> diff --git a/plugins/udev.c b/plugins/udev.c
> index cbb596d..28fff72 100644
> --- a/plugins/udev.c
> +++ b/plugins/udev.c
> @@ -588,6 +588,35 @@ static void add_linktop(struct ofono_modem *modem,
>  	}
>  }
>  
> +static void add_telit(struct ofono_modem *modem,
> +					struct udev_device *udev_device)
> +{
> +	struct udev_list_entry *entry;
> +	const char *devnode;
> +	gboolean found = FALSE;
> +
> +	DBG("modem %p", modem);
> +
> +	entry = udev_device_get_properties_list_entry(udev_device);
> +	while (entry) {
> +		const char *name = udev_list_entry_get_name(entry);
> +		const char *value = udev_list_entry_get_value(entry);
> +
> +		if (g_str_equal(name, "OFONO_TELIT_TYPE") == TRUE &&
> +					g_str_equal(value, "Modem") == TRUE) {
> +			found = TRUE;
> +		}
> +		entry = udev_list_entry_get_next(entry);
> +	}
> +
> +	if (found == FALSE)
> +		return;
> +
> +	devnode = udev_device_get_devnode(udev_device);
> +	ofono_modem_set_string(modem, "Modem", devnode);
> +	ofono_modem_register(modem);
> +}
> +

You can not really do it like this. You have to wait to register the
modem until all mandatory TTYs are present. Especially in case you
require two or more TTYs, which it seems you will.

Check the other ones like Novatel for examples.

>  static void add_modem(struct udev_device *udev_device)
>  {
>  	struct ofono_modem *modem;
> @@ -682,6 +711,8 @@ done:
>  		add_calypso(modem, udev_device);
>  	else if (g_strcmp0(driver, "tc65") == 0)
>  		add_tc65(modem, udev_device);
> +	else if (g_strcmp0(driver, "telit") == 0)
> +		add_telit(modem, udev_device);
>  	else if (g_strcmp0(driver, "nokiacdma") == 0)
>  		add_nokiacdma(modem, udev_device);
>          else if (g_strcmp0(driver, "linktop") == 0)

Regards

Marcel

Re: [PATCH v2 1/3] Add basic Telit UC864-G support:

[Marcel Holtmann]

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

Hi Bernhard,

> *add a basic plugin based on different ofono plugins
> *use Telit specific QSS for SIM-state
> *add Telit to atmodem/vendors
> *update Makefile
> 
> Co-authored-by: Christopher Vogl <Christopher.Vogl@hale.at>

I know this is not really satisfying, but we normally do not do it like
this. Patches are split in one author per patch.

> ---
>  Makefile.am              |    3 +
>  drivers/atmodem/vendor.h |    1 +
>  plugins/telit.c          |  384 ++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 388 insertions(+), 0 deletions(-)
>  create mode 100644 plugins/telit.c

Please send a separate patch for adding OFONO_VENDOR_TELIT first. We do
not really wanna intermix plugins/ and drivers/ patches.

> diff --git a/Makefile.am b/Makefile.am
> index a413a47..6197cf6 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -326,6 +326,9 @@ builtin_sources += plugins/nokiacdma.c
>  builtin_modules += linktop
>  builtin_sources += plugins/linktop.c
>  
> +builtin_modules += telit
> +builtin_sources += plugins/telit.c
> +
>  if BLUETOOTH
>  builtin_modules += bluetooth
>  builtin_sources += plugins/bluetooth.c plugins/bluetooth.h
> diff --git a/drivers/atmodem/vendor.h b/drivers/atmodem/vendor.h
> index 3898fa8..412bc76 100644
> --- a/drivers/atmodem/vendor.h
> +++ b/drivers/atmodem/vendor.h
> @@ -35,4 +35,5 @@ enum ofono_vendor {
>  	OFONO_VENDOR_WAVECOM,
>  	OFONO_VENDOR_NOKIA,
>  	OFONO_VENDOR_PHONESIM,
> +	OFONO_VENDOR_TELIT,
>  };
> diff --git a/plugins/telit.c b/plugins/telit.c
> new file mode 100644
> index 0000000..80cca14
> --- /dev/null
> +++ b/plugins/telit.c
> @@ -0,0 +1,384 @@
> +/*
> + *
> + *  oFono - Open Source Telephony
> + *
> + *  Copyright (C) 2008-2010  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 <stdio.h>
> +#include <unistd.h>
> +
> +#include <glib.h>
> +#include <gatchat.h>
> +#include <gattty.h>
> +
> +#define OFONO_API_SUBJECT_TO_CHANGE
> +#include <ofono/plugin.h>
> +#include <ofono/log.h>
> +#include <ofono/modem.h>
> +#include <ofono/call-barring.h>
> +#include <ofono/call-forwarding.h>
> +#include <ofono/call-meter.h>
> +#include <ofono/call-settings.h>
> +#include <ofono/devinfo.h>
> +#include <ofono/message-waiting.h>
> +#include <ofono/netreg.h>
> +#include <ofono/phonebook.h>
> +#include <ofono/sim.h>
> +#include <ofono/gprs.h>
> +#include <ofono/gprs-context.h>
> +#include <ofono/sms.h>
> +#include <ofono/ussd.h>
> +#include <ofono/voicecall.h>
> +
> +#include <drivers/atmodem/vendor.h>
> +#include <drivers/atmodem/atutil.h>
> +
> +static const char *none_prefix[] = { NULL };
> +static const char *qss_prefix[] = { "#QSS:", NULL };
> +
> +struct telit_data {
> +	GAtChat *chat;
> +	struct ofono_sim *sim;
> +};
> +
> +static void telit_debug(const char *str, void *user_data)
> +{
> +	const char *prefix = user_data;
> +
> +	ofono_info("%s%s", prefix, str);
> +}
> +
> +static int telit_probe(struct ofono_modem *modem)
> +{
> +	struct telit_data *data;
> +
> +	DBG("%p", modem);
> +
> +	data = g_try_new0(struct telit_data, 1);
> +	if (data == NULL)
> +		return -ENOMEM;
> +
> +	ofono_modem_set_data(modem, data);
> +
> +	return 0;
> +}
> +
> +static void telit_remove(struct ofono_modem *modem)
> +{
> +	struct telit_data *data = ofono_modem_get_data(modem);
> +
> +	DBG("%p", modem);
> +
> +	ofono_modem_set_data(modem, NULL);
> +
> +	if (data->chat)
> +		g_at_chat_unref(data->chat);

You should not need this. The chat should be unreferenced in
telit_disable.

And oFono core will ensure that the modem gets disabled before remove
callback is called.

> +	g_free(data);
> +}
> +
> +static void telit_qss_notify(GAtResult *result, gpointer user_data)
> +{
> +	struct ofono_modem *modem = user_data;
> +	struct telit_data *data = ofono_modem_get_data(modem);
> +	int status;
> +	GAtResultIter iter;

Variable declarations and code should be separate by an empty line. So
please add an extra empty line here.

> +	g_at_result_iter_init(&iter, result);
> +
> +	if (!g_at_result_iter_next(&iter, "#QSS:"))
> +		return;
> +
> +	g_at_result_iter_next_number(&iter, &status);
> +
> +	switch(status) {

Style is switch (status) here.

> +	case 0:
> +		DBG("SIM not inserted");
> +		ofono_sim_inserted_notify(data->sim,FALSE);
> +		break;
> +	case 1:
> +		DBG("SIM inserted");
> +		/* We need to sleep a bit */
> +		sleep(1);

This is not good at all. You are sleeping in a single-threaded event
driven daemon for a second.

If the modem is broken and is not ready right away after this
notification comes in, then you need to trigger this via
g_timeout_add_seconds().

> +		ofono_sim_inserted_notify(data->sim,TRUE);
> +		break;
> +	case 2:
> +		DBG("SIM inserted and PIN unlocked");
> +		break;
> +	case 3:
> +		DBG("SIM inserted and ready");
> +		break;
> +	default:
> +		return;
> +	}
> +	return;

We do not use empty return statements. Just remove this one.

> +}
> +
> +static void telit_qss_cb(gboolean ok, GAtResult *result, gpointer user_data)
> +{
> +	struct ofono_modem *modem = user_data;
> +	struct telit_data *data = ofono_modem_get_data(modem);
> +	int mode;
> +	int status;
> +	GAtResultIter iter;
> +	g_at_result_iter_init(&iter, result);
> +
> +	if (!g_at_result_iter_next(&iter, "#QSS:"))
> +		return;
> +
> +	g_at_result_iter_next_number(&iter, &mode);
> +	g_at_result_iter_next_number(&iter, &status);
> +
> +	switch(status) {
> +	case 0:
> +		DBG("SIM not inserted");
> +		break;
> +	case 1:
> +		DBG("SIM inserted");
> +		ofono_sim_inserted_notify(data->sim,TRUE);
> +		break;
> +	case 2:
> +		DBG("SIM inserted and PIN unlocked");
> +		break;
> +	case 3:
> +		DBG("SIM inserted and ready");
> +		break;
> +	default:
> +		return;
> +	}

I think you want to extract the actual SIM insert handling into a
separate function and just call it from telit_qss_cb and
telit_qss_notify.

> +}
> +
> +static void cfun_enable_cb(gboolean ok, GAtResult *result, gpointer user_data)
> +{
> +	struct ofono_modem *modem = user_data;
> +	struct telit_data *data = ofono_modem_get_data(modem);
> +
> +	DBG("");
> +
> +	if (ok)
> +		ofono_modem_set_powered(modem, TRUE);

If modem enabling fails, you should actually unreference the chat object
here and more important return here.

> +
> +	/* Enable sim state notification */
> +	g_at_chat_send(data->chat, "AT#QSS=1", none_prefix, NULL, NULL, NULL);
> +
> +	/* Follow sim state */
> +	g_at_chat_register(data->chat, "#QSS:", telit_qss_notify,
> +				FALSE, modem, NULL);
> +
> +	/* Query current sim state */
> +	g_at_chat_send(data->chat, "AT#QSS?", qss_prefix,
> +				telit_qss_cb, modem, NULL);
> +}
> +
> +static void cfun_disable_cb(gboolean ok, GAtResult *result, gpointer user_data)
> +{
> +	struct ofono_modem *modem = user_data;
> +	struct telit_data *data = ofono_modem_get_data(modem);
> +
> +	DBG("");
> +
> +	g_at_chat_unref(data->chat);
> +	data->chat = NULL;
> +
> +	if (ok)
> +		ofono_modem_set_powered(modem, FALSE);
> +}
> +
> +static GAtChat *open_device(struct ofono_modem *modem,
> +				const char *key, char *debug)
> +{
> +	const char *device;
> +	GAtSyntax *syntax;
> +	GIOChannel *channel;
> +	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_gsmv1();
> +	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, telit_debug, debug);
> +
> +	return chat;
> +}
> +
> +static int telit_enable(struct ofono_modem *modem)
> +{
> +	struct telit_data *data = ofono_modem_get_data(modem);
> +
> +	DBG("%p", modem);
> +
> +	data->chat = open_device(modem, "Modem", "Modem: ");
> +	if (data->chat == NULL)
> +		return -EINVAL;
> +
> +	/*
> +	 * Disable command echo and
> +	 * enable the Extended Error Result Codes
> +	 */
> +	g_at_chat_send(data->chat, "ATE0 +CMEE=1", none_prefix,
> +				NULL, NULL, NULL);

We should be using a none_prefix here, but not all plugins do this. So
if you wanna help us, then please send patches for fixing the other
modem plugins as well ;)

> +
> +	/* Set phone functionality */
> +	g_at_chat_send(data->chat, "AT+CFUN=4", none_prefix,
> +				cfun_enable_cb, modem, NULL);
> +
> +	return -EINPROGRESS;
> +}
> +
> +static int telit_disable(struct ofono_modem *modem)
> +{
> +	struct telit_data *data = ofono_modem_get_data(modem);
> +	DBG("%p", modem);
> +
> +	if (data->chat == NULL)
> +		return 0;
> +
> +	g_at_chat_cancel_all(data->chat);
> +	g_at_chat_unregister_all(data->chat);
> +
> +	/* Power down modem */
> +	g_at_chat_send(data->chat, "AT+CFUN=0", none_prefix,
> +				cfun_disable_cb, modem, NULL);
> +
> +	return -EINPROGRESS;
> +}
> +
> +static void set_online_cb(gboolean ok, GAtResult *result, gpointer user_data)
> +{
> +	struct cb_data *cbd = user_data;
> +	ofono_modem_online_cb_t cb = cbd->cb;
> +
> +	if (ok)
> +		CALLBACK_WITH_SUCCESS(cb, cbd->data);
> +	else
> +		CALLBACK_WITH_FAILURE(cb, cbd->data);
> +}
> +
> +static void telit_set_online(struct ofono_modem *modem, ofono_bool_t online,
> +				ofono_modem_online_cb_t cb, void *user_data)
> +{
> +	struct telit_data *data = ofono_modem_get_data(modem);
> +	struct cb_data *cbd = cb_data_new(cb, user_data);
> +	char const *command = online ? "AT+CFUN=1" : "AT+CFUN=4";
> +
> +	DBG("modem %p %s", modem, online ? "online" : "offline");
> +
> +	if (data->chat == NULL)
> +		goto error;

No need to check for data->chat here. If that happens, then something is
seriously broken ;)

> +	if (g_at_chat_send(data->chat, command, none_prefix,
> +			set_online_cb, cbd, g_free))
> +		return;
> +
> +error:
> +	g_free(cbd);
> +
> +	CALLBACK_WITH_FAILURE(cb, cbd->data);
> +}
> +
> +static void telit_pre_sim(struct ofono_modem *modem)
> +{
> +	struct telit_data *data = ofono_modem_get_data(modem);
> +
> +	DBG("%p", modem);
> +
> +	ofono_devinfo_create(modem, 0, "atmodem", data->chat);
> +	data->sim = ofono_sim_create(modem, 0, "atmodem", data->chat);
> +	ofono_voicecall_create(modem, 0, "atmodem", data->chat);
> +}
> +
> +static void telit_post_sim(struct ofono_modem *modem)
> +{
> +	struct telit_data *data = ofono_modem_get_data(modem);
> +
> +	DBG("%p", modem);
> +
> +	ofono_sms_create(modem, 0, "atmodem", data->chat);
> +}
> +
> +static void telit_post_online(struct ofono_modem *modem)
> +{
> +	struct telit_data *data = ofono_modem_get_data(modem);
> +	struct ofono_message_waiting *mw;
> +	struct ofono_gprs *gprs;
> +	struct ofono_gprs_context *gc;
> +
> +	DBG("%p", modem);
> +
> +	ofono_ussd_create(modem, 0, "atmodem", data->chat);
> +	ofono_call_forwarding_create(modem, 0, "atmodem", data->chat);
> +	ofono_call_settings_create(modem, 0, "atmodem", data->chat);
> +	ofono_netreg_create(modem, OFONO_VENDOR_TELIT, "atmodem", data->chat);

In general rule of style, please move netreg atom to the top. It will
not change functionality, but is just a good style.

> +	ofono_call_meter_create(modem, 0, "atmodem", data->chat);
> +	ofono_call_barring_create(modem, 0, "atmodem", data->chat);
> +
> +	gprs = ofono_gprs_create(modem, 0, "atmodem", data->chat);
> +	gc = ofono_gprs_context_create(modem, 0, "atmodem", data->chat);
> +
> +	if (gprs && gc)
> +		ofono_gprs_add_context(gprs, gc);
> +
> +	mw = ofono_message_waiting_create(modem);
> +	if (mw)
> +		ofono_message_waiting_register(mw);
> +}
> +
> +static struct ofono_modem_driver telit_driver = {
> +	.name			= "telit",
> +	.probe			= telit_probe,
> +	.remove			= telit_remove,
> +	.enable			= telit_enable,
> +	.disable		= telit_disable,
> +	.set_online		= telit_set_online,
> +	.pre_sim		= telit_pre_sim,
> +	.post_sim		= telit_post_sim,
> +	.post_online	= telit_post_online,
> +};
> +
> +static int telit_init(void)
> +{
> +	return ofono_modem_driver_register(&telit_driver);
> +}
> +
> +static void telit_exit(void)
> +{
> +	ofono_modem_driver_unregister(&telit_driver);
> +}
> +
> +OFONO_PLUGIN_DEFINE(telit, "telit driver", VERSION,
> +		OFONO_PLUGIN_PRIORITY_DEFAULT, telit_init, telit_exit)

Everything else looks all good.

Regards

Marcel