[PATCH 0/6] gemalto's ALS3 and PHS8P support

[PATCH 0/6] gemalto's ALS3 and PHS8P support

[Gabriel Lucas]

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

This patch serie bring further support to PHS8P modem
and the same level of support for ALS3 modem. Here are
the lists on enhancements:
- ALS3 is detected by Ofono and uses the gemalto plugin
- SIM removing and insertion is supported
- The optional Technology property on NetworkRegistration
is supported
- We ensure that the modem is ready to work before
initializing it

Some of the enhancements are brought by workarounds because
of AT command interface limitations

Gabriel Lucas (4):
  gemalto: add detection of ALS3 modem
  gemalto: support ALS3 in gemalto's plugin
  sim: give access to the driver
  gemalto: fix sim reinsertion issue

Mariem Cherif (2):
  gemalto: acquire the network technology
  gemalto: handle sim is inserted or removed URCs

 drivers/atmodem/network-registration.c |  45 +++++++++
 include/sim.h                          |   2 +
 plugins/gemalto.c                      | 169 +++++++++++++++++++++++++++++++--
 plugins/udevng.c                       |  18 ++++
 src/sim.c                              |   5 +
 5 files changed, 233 insertions(+), 6 deletions(-)

-- 
1.9.1

[PATCH 1/6] gemalto: add detection of ALS3 modem

[Gabriel Lucas]

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

The product ID is added to the list of
modems to be detected by Ofono.
The gemalto plugin is used to handle the
ALS3 modem.
---
 plugins/udevng.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/plugins/udevng.c b/plugins/udevng.c
index 3c7d99e..d398c6e 100644
--- a/plugins/udevng.c
+++ b/plugins/udevng.c
@@ -1132,6 +1132,7 @@ static gboolean setup_gemalto(struct modem_info* modem)
 		DBG("%s %s %s %s %s", info->devnode, info->interface,
 				info->number, info->label, info->subsystem);
 
+		// PHS8-P
 		if (g_strcmp0(info->interface, "255/255/255") == 0) {
 			if (g_strcmp0(info->number, "01") == 0)
 				gps = info->devnode;
@@ -1144,6 +1145,20 @@ static gboolean setup_gemalto(struct modem_info* modem)
 			else if (g_strcmp0(info->subsystem, "usbmisc") == 0)
 				qmi = info->devnode;
 		}
+
+		// ALS3
+		if (g_strcmp0(info->interface, "2/2/1") == 0) {
+			if (g_strcmp0(info->number, "00") == 0)
+				mdm = info->devnode;
+			else if (g_strcmp0(info->number, "02") == 0)
+				app = info->devnode;
+			else if (g_strcmp0(info->number, "04") == 0)
+				gps = info->devnode;
+		}
+		if (g_strcmp0(info->interface, "2/6/0") == 0) {
+			if (g_strcmp0(info->subsystem, "net") == 0)
+				net = info->devnode;
+		}
 	}
 
 	DBG("application=%s gps=%s modem=%s network=%s qmi=%s",
@@ -1156,6 +1171,7 @@ static gboolean setup_gemalto(struct modem_info* modem)
 	ofono_modem_set_string(modem->modem, "GPS", gps);
 	ofono_modem_set_string(modem->modem, "Modem", mdm);
 	ofono_modem_set_string(modem->modem, "Device", qmi);
+	ofono_modem_set_string(modem->modem, "Model", modem->model);
 	ofono_modem_set_string(modem->modem, "NetworkInterface", net);
 
 	return TRUE;
@@ -1601,6 +1617,8 @@ static struct {
 	{ "gemalto",	"option",	"1e2d",	"0053"	},
 	{ "gemalto",	"cdc_wdm",	"1e2d",	"0053"	},
 	{ "gemalto",	"qmi_wwan",	"1e2d",	"0053"	},
+	{ "gemalto",	"cdc_acm",	"1e2d",	"0061"	},
+	{ "gemalto",	"cdc_ether",	"1e2d",	"0061"	},
 	{ "telit",	"cdc_ncm",	"1bc7", "0036"	},
 	{ "telit",	"cdc_acm",	"1bc7", "0036"	},
 	{ "xmm7xxx",	"cdc_acm",	"8087", "0930"	},
-- 
1.9.1

Re: [PATCH 1/6] gemalto: add detection of ALS3 modem

[Denis Kenzior]

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

Hi Gabriel,

On 03/15/2018 07:49 AM, Gabriel Lucas wrote:
> The product ID is added to the list of
> modems to be detected by Ofono.
> The gemalto plugin is used to handle the
> ALS3 modem.
> ---
>   plugins/udevng.c | 18 ++++++++++++++++++
>   1 file changed, 18 insertions(+)
> 

Patch applied, thanks.

Regards,
-Denis

[PATCH 2/6] gemalto: support ALS3 in gemalto's plugin

[Gabriel Lucas]

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

Force serial port opening options
Wait for modem to be ready to start
initializing it
Handle LTE
---
 plugins/gemalto.c | 105 ++++++++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 99 insertions(+), 6 deletions(-)

diff --git a/plugins/gemalto.c b/plugins/gemalto.c
index 3739d7b..16ca463 100644
--- a/plugins/gemalto.c
+++ b/plugins/gemalto.c
@@ -53,6 +53,10 @@
 
 #define HARDWARE_MONITOR_INTERFACE OFONO_SERVICE ".cinterion.HardwareMonitor"
 
+// Supported gemalto's modem
+#define GEMALTO_MODEL_PHS8P 	"0053"
+#define GEMALTO_MODEL_ALS3 		"0061"
+
 static const char *none_prefix[] = { NULL };
 static const char *sctm_prefix[] = { "^SCTM:", NULL };
 static const char *sbv_prefix[] = { "^SBV:", NULL };
@@ -70,6 +74,8 @@ struct gemalto_data {
 	gboolean have_sim;
 	struct at_util_sim_state_query *sim_state_query;
 	struct gemalto_hardware_monitor *hm;
+	guint modem_ready_id;
+	guint trial_cmd_id;
 };
 
 static int gemalto_probe(struct ofono_modem *modem)
@@ -107,10 +113,24 @@ static GAtChat *open_device(const char *device)
 	GAtSyntax *syntax;
 	GIOChannel *channel;
 	GAtChat *chat;
+	GHashTable *options;
+
+	options = g_hash_table_new(g_str_hash, g_str_equal);
+	if (options == NULL)
+		return NULL;
+
+	g_hash_table_insert(options, "Baud", "115200");
+	g_hash_table_insert(options, "StopBits", "1");
+	g_hash_table_insert(options, "DataBits", "8");
+	g_hash_table_insert(options, "Parity", "none");
+	g_hash_table_insert(options, "XonXoff", "off");
+	g_hash_table_insert(options, "RtsCts", "on");
+	g_hash_table_insert(options, "Local", "on");
+	g_hash_table_insert(options, "Read", "on");
 
 	DBG("Opening device %s", device);
 
-	channel = g_at_tty_open(device, NULL);
+	channel = g_at_tty_open(device, options);
 	if (channel == NULL)
 		return NULL;
 
@@ -118,6 +138,7 @@ static GAtChat *open_device(const char *device)
 	chat = g_at_chat_new(channel, syntax);
 	g_at_syntax_unref(syntax);
 	g_io_channel_unref(channel);
+	g_hash_table_destroy(options);
 
 	if (chat == NULL)
 		return NULL;
@@ -300,29 +321,33 @@ static int gemalto_hardware_monitor_enable(struct ofono_modem *modem)
 	return 0;
 }
 
-static int gemalto_enable(struct ofono_modem *modem)
+static void gemalto_initialize(struct ofono_modem *modem)
 {
 	struct gemalto_data *data = ofono_modem_get_data(modem);
 	const char *app, *mdm;
 
-	DBG("%p", modem);
+	/* Close devices */
+	g_at_chat_unref(data->mdm);
+	g_at_chat_unref(data->app);
+
+	DBG("");
 
 	app = ofono_modem_get_string(modem, "Application");
 	mdm = ofono_modem_get_string(modem, "Modem");
 
 	if (app == NULL || mdm == NULL)
-		return -EINVAL;
+		return;
 
 	/* Open devices */
 	data->app = open_device(app);
 	if (data->app == NULL)
-		return -EINVAL;
+		return;
 
 	data->mdm = open_device(mdm);
 	if (data->mdm == NULL) {
 		g_at_chat_unref(data->app);
 		data->app = NULL;
-		return -EINVAL;
+		return;
 	}
 
 	if (getenv("OFONO_AT_DEBUG")) {
@@ -340,6 +365,67 @@ static int gemalto_enable(struct ofono_modem *modem)
 			cfun_enable, modem, NULL);
 
 	gemalto_hardware_monitor_enable(modem);
+}
+
+static void gemalto_modem_ready(GAtResult *result, gpointer user_data)
+{
+	struct ofono_modem *modem = user_data;
+	struct gemalto_data *data = ofono_modem_get_data(modem);
+
+	DBG("");
+
+	g_at_chat_unregister(data->app, data->modem_ready_id);
+	g_at_chat_cancel(data->app, data->trial_cmd_id);
+
+	gemalto_initialize(modem);
+}
+
+static void gemalto_at_cb(gboolean ok, GAtResult *result, gpointer user_data)
+{
+	struct ofono_modem *modem = user_data;
+	struct gemalto_data *data = ofono_modem_get_data(modem);
+
+	g_at_chat_unregister(data->app, data->modem_ready_id);
+	gemalto_initialize(modem);
+}
+
+static gboolean gemalto_at_timeout(gpointer user_data)
+{
+	struct ofono_modem *modem = user_data;
+	struct gemalto_data *data = ofono_modem_get_data(modem);
+
+	data->trial_cmd_id = g_at_chat_send(data->app, "AT+CFUN?", none_prefix, gemalto_at_cb, modem, NULL);
+
+	return FALSE;
+}
+
+static int gemalto_enable(struct ofono_modem *modem)
+{
+	struct gemalto_data *data = ofono_modem_get_data(modem);
+	const char *app, *mdm;
+
+	DBG("%p", modem);
+
+	app = ofono_modem_get_string(modem, "Application");
+	mdm = ofono_modem_get_string(modem, "Modem");
+
+	if (app == NULL || mdm == NULL)
+		return -EINVAL;
+
+	/* Open devices */
+	data->app = open_device(app);
+	if (data->app == NULL)
+		return -EINVAL;
+
+	data->mdm = open_device(mdm);
+	if (data->mdm == NULL) {
+		g_at_chat_unref(data->app);
+		data->app = NULL;
+		return -EINVAL;
+	}
+
+	data->modem_ready_id = g_at_chat_register(data->app, "^SYSSTART", gemalto_modem_ready, FALSE, modem, NULL);
+	data->trial_cmd_id = g_at_chat_send(data->app, "ATE0 AT", none_prefix, gemalto_at_cb, modem, NULL);
 
 	return -EINPROGRESS;
 }
@@ -449,10 +535,17 @@ static void gemalto_post_sim(struct ofono_modem *modem)
 static void gemalto_post_online(struct ofono_modem *modem)
 {
 	struct gemalto_data *data = ofono_modem_get_data(modem);
+	const char *model;
 
 	DBG("%p", modem);
 
 	ofono_netreg_create(modem, OFONO_VENDOR_CINTERION, "atmodem", data->app);
+
+	model = ofono_modem_get_string(modem, "Model");
+
+	if(!strncmp(model, GEMALTO_MODEL_ALS3, 4)) {
+		ofono_lte_create(modem, "atmodem", data->app);
+	}
 }
 
 static struct ofono_modem_driver gemalto_driver = {
-- 
1.9.1

Re: [PATCH 2/6] gemalto: support ALS3 in gemalto's plugin

[Denis Kenzior]

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

Hi Gabriel,

On 03/15/2018 07:49 AM, Gabriel Lucas wrote:
> Force serial port opening options
> Wait for modem to be ready to start
> initializing it
> Handle LTE
> ---
>   plugins/gemalto.c | 105 ++++++++++++++++++++++++++++++++++++++++++++++++++----
>   1 file changed, 99 insertions(+), 6 deletions(-)
> 
> diff --git a/plugins/gemalto.c b/plugins/gemalto.c
> index 3739d7b..16ca463 100644
> --- a/plugins/gemalto.c
> +++ b/plugins/gemalto.c
> @@ -53,6 +53,10 @@
>   
>   #define HARDWARE_MONITOR_INTERFACE OFONO_SERVICE ".cinterion.HardwareMonitor"
>   
> +// Supported gemalto's modem

No // please, use /* */.  This is C code ;)

> +#define GEMALTO_MODEL_PHS8P 	"0053"
> +#define GEMALTO_MODEL_ALS3 		"0061"
> +
>   static const char *none_prefix[] = { NULL };
>   static const char *sctm_prefix[] = { "^SCTM:", NULL };
>   static const char *sbv_prefix[] = { "^SBV:", NULL };
> @@ -70,6 +74,8 @@ struct gemalto_data {
>   	gboolean have_sim;
>   	struct at_util_sim_state_query *sim_state_query;
>   	struct gemalto_hardware_monitor *hm;
> +	guint modem_ready_id;
> +	guint trial_cmd_id;
>   };
>   
>   static int gemalto_probe(struct ofono_modem *modem)
> @@ -107,10 +113,24 @@ static GAtChat *open_device(const char *device)
>   	GAtSyntax *syntax;
>   	GIOChannel *channel;
>   	GAtChat *chat;
> +	GHashTable *options;
> +
> +	options = g_hash_table_new(g_str_hash, g_str_equal);
> +	if (options == NULL)
> +		return NULL;
> +
> +	g_hash_table_insert(options, "Baud", "115200");
> +	g_hash_table_insert(options, "StopBits", "1");
> +	g_hash_table_insert(options, "DataBits", "8");
> +	g_hash_table_insert(options, "Parity", "none");
> +	g_hash_table_insert(options, "XonXoff", "off");
> +	g_hash_table_insert(options, "RtsCts", "on");
> +	g_hash_table_insert(options, "Local", "on");
> +	g_hash_table_insert(options, "Read", "on");
>   
>   	DBG("Opening device %s", device);
>   
> -	channel = g_at_tty_open(device, NULL);
> +	channel = g_at_tty_open(device, options);
>   	if (channel == NULL)
>   		return NULL;

You leak options here in case channel == NULL.  The typical pattern is 
to call g_hash_table_destroy(options) right after g_at_tty_open.  See 
other plugins for details.

>   
> @@ -118,6 +138,7 @@ static GAtChat *open_device(const char *device)
>   	chat = g_at_chat_new(channel, syntax);
>   	g_at_syntax_unref(syntax);
>   	g_io_channel_unref(channel);
> +	g_hash_table_destroy(options);
>   
>   	if (chat == NULL)
>   		return NULL;
> @@ -300,29 +321,33 @@ static int gemalto_hardware_monitor_enable(struct ofono_modem *modem)
>   	return 0;
>   }
>   
> -static int gemalto_enable(struct ofono_modem *modem)
> +static void gemalto_initialize(struct ofono_modem *modem)
>   {
>   	struct gemalto_data *data = ofono_modem_get_data(modem);
>   	const char *app, *mdm;
>   
> -	DBG("%p", modem);
> +	/* Close devices */
> +	g_at_chat_unref(data->mdm);
> +	g_at_chat_unref(data->app);
> +

Why would you close both devices?  It is unnecessary, especially in the 
case that the modem is already up and responding to commands.

> +	DBG("");
>   
>   	app = ofono_modem_get_string(modem, "Application");
>   	mdm = ofono_modem_get_string(modem, "Modem");
>   
>   	if (app == NULL || mdm == NULL)
> -		return -EINVAL;
> +		return;
>   
>   	/* Open devices */
>   	data->app = open_device(app);
>   	if (data->app == NULL)
> -		return -EINVAL;
> +		return;
>   
>   	data->mdm = open_device(mdm);
>   	if (data->mdm == NULL) {
>   		g_at_chat_unref(data->app);
>   		data->app = NULL;
> -		return -EINVAL;
> +		return;
>   	}
>   
>   	if (getenv("OFONO_AT_DEBUG")) {
> @@ -340,6 +365,67 @@ static int gemalto_enable(struct ofono_modem *modem)
>   			cfun_enable, modem, NULL);
>   
>   	gemalto_hardware_monitor_enable(modem);
> +}
> +
> +static void gemalto_modem_ready(GAtResult *result, gpointer user_data)
> +{
> +	struct ofono_modem *modem = user_data;
> +	struct gemalto_data *data = ofono_modem_get_data(modem);
> +
> +	DBG("");
> +
> +	g_at_chat_unregister(data->app, data->modem_ready_id);
> +	g_at_chat_cancel(data->app, data->trial_cmd_id);
> +
> +	gemalto_initialize(modem);
> +}
> +
> +static void gemalto_at_cb(gboolean ok, GAtResult *result, gpointer user_data)
> +{
> +	struct ofono_modem *modem = user_data;
> +	struct gemalto_data *data = ofono_modem_get_data(modem);
> +
> +	g_at_chat_unregister(data->app, data->modem_ready_id);
> +	gemalto_initialize(modem);
> +}
> +
> +static gboolean gemalto_at_timeout(gpointer user_data)
> +{
> +	struct ofono_modem *modem = user_data;
> +	struct gemalto_data *data = ofono_modem_get_data(modem);
> +
> +	data->trial_cmd_id = g_at_chat_send(data->app, "AT+CFUN?", none_prefix, gemalto_at_cb, modem, NULL);

What does this do?

> +
> +	return FALSE;
> +}

Where is the timeout started?

> +
> +static int gemalto_enable(struct ofono_modem *modem)
> +{
> +	struct gemalto_data *data = ofono_modem_get_data(modem);
> +	const char *app, *mdm;
> +
> +	DBG("%p", modem);
> +
> +	app = ofono_modem_get_string(modem, "Application");
> +	mdm = ofono_modem_get_string(modem, "Modem");
> +
> +	if (app == NULL || mdm == NULL)
> +		return -EINVAL;
> +
> +	/* Open devices */
> +	data->app = open_device(app);
> +	if (data->app == NULL)
> +		return -EINVAL;
> +
> +	data->mdm = open_device(mdm);
> +	if (data->mdm == NULL) {
> +		g_at_chat_unref(data->app);
> +		data->app = NULL;
> +		return -EINVAL;
> +	}
> +
> +	data->modem_ready_id = g_at_chat_register(data->app, "^SYSSTART", gemalto_modem_ready, FALSE, modem, NULL);
> +	data->trial_cmd_id = g_at_chat_send(data->app, "ATE0 AT", none_prefix, gemalto_at_cb, modem, NULL);

We use the Linux Kernel coding style with house rules documented in 
doc/coding-style.txt.  No lines over 80 characters please.

Why do you open both ports only to close them in gemalto_initialize? 
One port here would be sufficient.

>   
>   	return -EINPROGRESS;
>   }
> @@ -449,10 +535,17 @@ static void gemalto_post_sim(struct ofono_modem *modem)
>   static void gemalto_post_online(struct ofono_modem *modem)
>   {
>   	struct gemalto_data *data = ofono_modem_get_data(modem);
> +	const char *model;
>   
>   	DBG("%p", modem);
>   
>   	ofono_netreg_create(modem, OFONO_VENDOR_CINTERION, "atmodem", data->app);
> +
> +	model = ofono_modem_get_string(modem, "Model");

You might as well perform this assignment at variable declaration.

> +
> +	if(!strncmp(model, GEMALTO_MODEL_ALS3, 4)) {
> +		ofono_lte_create(modem, "atmodem", data->app);
> +	}

Why strncmp?  No need for {} and there should be a space between if and '('

Also, you most likely need the LTE atom in post_sim.

You also now have to little arguments to ofono_lte_create.  Please git 
pull --rebase first.

>   }
>   
>   static struct ofono_modem_driver gemalto_driver = {
> 

Regards,
-Denis

Re: [PATCH 2/6] gemalto: support ALS3 in gemalto's plugin

[Gabriel Lucas]

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

Hi Denis,

On 15/03/2018 18:11, Denis Kenzior wrote:
> No // please, use /* */.  This is C code ;)
Fixed. I've just learned it was not ANSI C :).
>
> You leak options here in case channel == NULL.  The typical pattern is 
> to call g_hash_table_destroy(options) right after g_at_tty_open.  See 
> other plugins for details.
Fixed
>
>>   @@ -118,6 +138,7 @@ static GAtChat *open_device(const char *device)
>>       chat = g_at_chat_new(channel, syntax);
>>       g_at_syntax_unref(syntax);
>>       g_io_channel_unref(channel);
>> +    g_hash_table_destroy(options);
>>         if (chat == NULL)
>>           return NULL;
>> @@ -300,29 +321,33 @@ static int 
>> gemalto_hardware_monitor_enable(struct ofono_modem *modem)
>>       return 0;
>>   }
>>   -static int gemalto_enable(struct ofono_modem *modem)
>> +static void gemalto_initialize(struct ofono_modem *modem)
>>   {
>>       struct gemalto_data *data = ofono_modem_get_data(modem);
>>       const char *app, *mdm;
>>   -    DBG("%p", modem);
>> +    /* Close devices */
>> +    g_at_chat_unref(data->mdm);
>> +    g_at_chat_unref(data->app);
>> +
>
> Why would you close both devices?  It is unnecessary, especially in 
> the case that the modem is already up and responding to commands.
Because it seems that opening the device while it is not ready leads the 
driver to a weird state.
When the modem becomes ready, I am not able to send any AT command to 
the APP device.
Here is the output:

ofonod[12826]: src/modem.c:get_modem_property() modem 0x72b750 property 
Application
ofonod[12826]: src/modem.c:get_modem_property() modem 0x72b750 property 
Modem
ofonod[12826]: plugins/gemalto.c:open_device() Opening device /dev/ttyACM1
ofonod[12826]: plugins/gemalto.c:open_device() Opening device /dev/ttyACM0
ofonod[12826]: plugins/gemalto.c:gemalto_modem_ready()
ofonod[12826]: plugins/gemalto.c:gemalto_initialize()
ofonod[12826]: src/modem.c:get_modem_property() modem 0x72b750 property 
Application
ofonod[12826]: src/modem.c:get_modem_property() modem 0x72b750 property 
Modem
ofonod[12826]: plugins/gemalto.c:gemalto_hardware_monitor_enable()
ofonod[12826]: Mdm> ATE0\r
ofonod[12826]: Mdm< ATE0\r\r\nOK\r\n
ofonod[12826]: Mdm> AT&C0\r
ofonod[12826]: Mdm< \r\n+CME ERROR: operation not supported\r\n
ofonod[12826]: src/modem.c:set_powered_timeout() modem: 0x72b750

When it is re-opened, it works fine.
>
>> +    DBG("");
>>         app = ofono_modem_get_string(modem, "Application");
>>       mdm = ofono_modem_get_string(modem, "Modem");
>>         if (app == NULL || mdm == NULL)
>> -        return -EINVAL;
>> +        return;
>>         /* Open devices */
>>       data->app = open_device(app);
>>       if (data->app == NULL)
>> -        return -EINVAL;
>> +        return;
>>         data->mdm = open_device(mdm);
>>       if (data->mdm == NULL) {
>>           g_at_chat_unref(data->app);
>>           data->app = NULL;
>> -        return -EINVAL;
>> +        return;
>>       }
>>         if (getenv("OFONO_AT_DEBUG")) {
>> @@ -340,6 +365,67 @@ static int gemalto_enable(struct ofono_modem 
>> *modem)
>>               cfun_enable, modem, NULL);
>>         gemalto_hardware_monitor_enable(modem);
>> +}
>> +
>> +static void gemalto_modem_ready(GAtResult *result, gpointer user_data)
>> +{
>> +    struct ofono_modem *modem = user_data;
>> +    struct gemalto_data *data = ofono_modem_get_data(modem);
>> +
>> +    DBG("");
>> +
>> +    g_at_chat_unregister(data->app, data->modem_ready_id);
>> +    g_at_chat_cancel(data->app, data->trial_cmd_id);
>> +
>> +    gemalto_initialize(modem);
>> +}
>> +
>> +static void gemalto_at_cb(gboolean ok, GAtResult *result, gpointer 
>> user_data)
>> +{
>> +    struct ofono_modem *modem = user_data;
>> +    struct gemalto_data *data = ofono_modem_get_data(modem);
>> +
>> +    g_at_chat_unregister(data->app, data->modem_ready_id);
>> +    gemalto_initialize(modem);
>> +}
>> +
>> +static gboolean gemalto_at_timeout(gpointer user_data)
>> +{
>> +    struct ofono_modem *modem = user_data;
>> +    struct gemalto_data *data = ofono_modem_get_data(modem);
>> +
>> +    data->trial_cmd_id = g_at_chat_send(data->app, "AT+CFUN?", 
>> none_prefix, gemalto_at_cb, modem, NULL);
>
> What does this do?
Some gemalto's modems have their USB devices mounted before they are 
ready to handle AT command.
At modem's or oFono's startup, the modem is either ready or not. In 
gemalto_enable, we register to the "^SYSSTART" URC and send the "AT" 
command.
- If the AT command returns, then the modem is ready so we unregister 
the "^SYSSTART" URC and start initializing the modem.
- If the modem isn't ready, the AT command will never return so we have 
to wait for the "^SYSSTART" URC. When it is received, we cancel the AT
command because it will never get any answer and start initializing the 
modem.

I've no cleaner way to do that.
>
>> +
>> +    return FALSE;
>> +}
>
> Where is the timeout started?
Seems like I forgot to clean some parts of the code ;). There is no timeout.
>
> We use the Linux Kernel coding style with house rules documented in 
> doc/coding-style.txt.  No lines over 80 characters please.
Fixed
>
> Why do you open both ports only to close them in gemalto_initialize? 
> One port here would be sufficient.
You are right, I've changed the code such that it opens only app, then 
try the modem on it and reopen it
only if the modem wasn't ready.
>
> You might as well perform this assignment at variable declaration.
Fixed
>> +
>> +    if(!strncmp(model, GEMALTO_MODEL_ALS3, 4)) {
>> +        ofono_lte_create(modem, "atmodem", data->app);
>> +    }
>
> Why strncmp?  No need for {} and there should be a space between if 
> and '('
Because strncmp is more secured than strcmp. But if you prefer strcmp or 
anything else, I'll change it.
>
> Also, you most likely need the LTE atom in post_sim.
>
> You also now have to little arguments to ofono_lte_create.  Please git 
> pull --rebase first.
Fixed

I'm waiting for your answers before reposting the reviewed patch.
Thanks for the patch review.

Best Regards,
Gabriel

Re: [PATCH 2/6] gemalto: support ALS3 in gemalto's plugin

[Denis Kenzior]

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

Hi Gabriel,

>>>   { >>>       struct gemalto_data *data = ofono_modem_get_data(modem);
>>>       const char *app, *mdm;
>>>   -    DBG("%p", modem);
>>> +    /* Close devices */
>>> +    g_at_chat_unref(data->mdm);
>>> +    g_at_chat_unref(data->app);
>>> +
>>
>> Why would you close both devices?  It is unnecessary, especially in 
>> the case that the modem is already up and responding to commands.
> Because it seems that opening the device while it is not ready leads the 
> driver to a weird state.
> When the modem becomes ready, I am not able to send any AT command to 
> the APP device.
> Here is the output:
> 
> ofonod[12826]: src/modem.c:get_modem_property() modem 0x72b750 property 
> Application
> ofonod[12826]: src/modem.c:get_modem_property() modem 0x72b750 property 
> Modem
> ofonod[12826]: plugins/gemalto.c:open_device() Opening device /dev/ttyACM1
> ofonod[12826]: plugins/gemalto.c:open_device() Opening device /dev/ttyACM0
> ofonod[12826]: plugins/gemalto.c:gemalto_modem_ready()
> ofonod[12826]: plugins/gemalto.c:gemalto_initialize()
> ofonod[12826]: src/modem.c:get_modem_property() modem 0x72b750 property 
> Application
> ofonod[12826]: src/modem.c:get_modem_property() modem 0x72b750 property 
> Modem
> ofonod[12826]: plugins/gemalto.c:gemalto_hardware_monitor_enable()
> ofonod[12826]: Mdm> ATE0\r
> ofonod[12826]: Mdm< ATE0\r\r\nOK\r\n
> ofonod[12826]: Mdm> AT&C0\r
> ofonod[12826]: Mdm< \r\n+CME ERROR: operation not supported\r\n
> ofonod[12826]: src/modem.c:set_powered_timeout() modem: 0x72b750
> 
> When it is re-opened, it works fine.

So my question was two fold:

1. Why would you open both devices?  Wouldn't opening just one to figure 
out whether the modem is 'ready' or not be enough?
2. If the modem is ready, why would you close the opened device?  That 
just seems to be a waste of time.

If the device isn't ready then I could see how re-opening the port might 
be required.

>>> +
>>> +    if(!strncmp(model, GEMALTO_MODEL_ALS3, 4)) {
>>> +        ofono_lte_create(modem, "atmodem", data->app);
>>> +    }
>>
>> Why strncmp?  No need for {} and there should be a space between if 
>> and '('
> Because strncmp is more secured than strcmp. But if you prefer strcmp or 
> anything else, I'll change it.

In this case they're the same and its not like you have user input to 
worry about here...

Regards,
-Denis

[PATCH 2/6] gemalto: support ALS3 in gemalto's plugin

[Gabriel Lucas]

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

Force serial port opening options
Wait for modem to be ready to start
initializing it
Handle LTE
---
 plugins/gemalto.c | 106 +++++++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 94 insertions(+), 12 deletions(-)

diff --git a/plugins/gemalto.c b/plugins/gemalto.c
index 3739d7b..af50a9b 100644
--- a/plugins/gemalto.c
+++ b/plugins/gemalto.c
@@ -53,6 +53,10 @@
 
 #define HARDWARE_MONITOR_INTERFACE OFONO_SERVICE ".cinterion.HardwareMonitor"
 
+/* Supported gemalto's modem */
+#define GEMALTO_MODEL_PHS8P 	"0053"
+#define GEMALTO_MODEL_ALS3 		"0061"
+
 static const char *none_prefix[] = { NULL };
 static const char *sctm_prefix[] = { "^SCTM:", NULL };
 static const char *sbv_prefix[] = { "^SBV:", NULL };
@@ -70,6 +74,8 @@ struct gemalto_data {
 	gboolean have_sim;
 	struct at_util_sim_state_query *sim_state_query;
 	struct gemalto_hardware_monitor *hm;
+	guint modem_ready_id;
+	guint trial_cmd_id;
 };
 
 static int gemalto_probe(struct ofono_modem *modem)
@@ -107,10 +113,26 @@ static GAtChat *open_device(const char *device)
 	GAtSyntax *syntax;
 	GIOChannel *channel;
 	GAtChat *chat;
+	GHashTable *options;
+
+	options = g_hash_table_new(g_str_hash, g_str_equal);
+	if (options == NULL)
+		return NULL;
+
+	g_hash_table_insert(options, "Baud", "115200");
+	g_hash_table_insert(options, "StopBits", "1");
+	g_hash_table_insert(options, "DataBits", "8");
+	g_hash_table_insert(options, "Parity", "none");
+	g_hash_table_insert(options, "XonXoff", "off");
+	g_hash_table_insert(options, "RtsCts", "on");
+	g_hash_table_insert(options, "Local", "on");
+	g_hash_table_insert(options, "Read", "on");
 
 	DBG("Opening device %s", device);
 
-	channel = g_at_tty_open(device, NULL);
+	channel = g_at_tty_open(device, options);
+	g_hash_table_destroy(options);
+
 	if (channel == NULL)
 		return NULL;
 
@@ -300,29 +322,24 @@ static int gemalto_hardware_monitor_enable(struct ofono_modem *modem)
 	return 0;
 }
 
-static int gemalto_enable(struct ofono_modem *modem)
+static void gemalto_initialize(struct ofono_modem *modem)
 {
 	struct gemalto_data *data = ofono_modem_get_data(modem);
-	const char *app, *mdm;
+	const char *mdm;
 
-	DBG("%p", modem);
+	DBG("");
 
-	app = ofono_modem_get_string(modem, "Application");
 	mdm = ofono_modem_get_string(modem, "Modem");
 
-	if (app == NULL || mdm == NULL)
-		return -EINVAL;
+	if (mdm == NULL)
+		return;
 
 	/* Open devices */
-	data->app = open_device(app);
-	if (data->app == NULL)
-		return -EINVAL;
-
 	data->mdm = open_device(mdm);
 	if (data->mdm == NULL) {
 		g_at_chat_unref(data->app);
 		data->app = NULL;
-		return -EINVAL;
+		return;
 	}
 
 	if (getenv("OFONO_AT_DEBUG")) {
@@ -340,6 +357,67 @@ static int gemalto_enable(struct ofono_modem *modem)
 			cfun_enable, modem, NULL);
 
 	gemalto_hardware_monitor_enable(modem);
+}
+
+static void gemalto_modem_ready(GAtResult *result, gpointer user_data)
+{
+	struct ofono_modem *modem = user_data;
+	struct gemalto_data *data = ofono_modem_get_data(modem);
+	const char *app = ofono_modem_get_string(modem, "Application");
+
+	DBG("");
+
+	g_at_chat_unregister(data->app, data->modem_ready_id);
+	g_at_chat_cancel(data->app, data->trial_cmd_id);
+
+	/*!
+	 * As the modem wasn't ready to handle AT commands when we opened
+	 * it, we have to close and reopen the device app.
+	 */
+	g_at_chat_unref(data->app);
+
+	if(app == NULL)
+		return;
+
+	data->app = open_device(app);
+	if (data->app == NULL)
+		return;
+
+	gemalto_initialize(modem);
+}
+
+static void gemalto_at_cb(gboolean ok, GAtResult *result, gpointer user_data)
+{
+	struct ofono_modem *modem = user_data;
+	struct gemalto_data *data = ofono_modem_get_data(modem);
+
+	g_at_chat_unregister(data->app, data->modem_ready_id);
+	gemalto_initialize(modem);
+}
+
+static int gemalto_enable(struct ofono_modem *modem)
+{
+	struct gemalto_data *data = ofono_modem_get_data(modem);
+	const char *app, *mdm;
+
+	DBG("%p", modem);
+
+	app = ofono_modem_get_string(modem, "Application");
+	mdm = ofono_modem_get_string(modem, "Modem");
+
+	if (app == NULL || mdm == NULL)
+		return -EINVAL;
+
+	/* Open devices */
+	data->app = open_device(app);
+	if (data->app == NULL)
+		return -EINVAL;
+
+	/* Try the AT command. If it doesn't work, wait for ^SYSSTART */
+	data->modem_ready_id = g_at_chat_register(data->app, "^SYSSTART",
+								gemalto_modem_ready, FALSE, modem, NULL);
+	data->trial_cmd_id = g_at_chat_send(data->app, "ATE0 AT",
+								none_prefix, gemalto_at_cb, modem, NULL);
 
 	return -EINPROGRESS;
 }
@@ -432,6 +510,7 @@ static void gemalto_post_sim(struct ofono_modem *modem)
 	struct gemalto_data *data = ofono_modem_get_data(modem);
 	struct ofono_gprs *gprs;
 	struct ofono_gprs_context *gc;
+	const char *model = ofono_modem_get_string(modem, "Model");
 
 	DBG("%p", modem);
 
@@ -444,6 +523,9 @@ static void gemalto_post_sim(struct ofono_modem *modem)
 
 	if (gprs && gc)
 		ofono_gprs_add_context(gprs, gc);
+
+	if (!g_strcmp0(model, GEMALTO_MODEL_ALS3))
+		ofono_lte_create(modem, OFONO_VENDOR_CINTERION, "atmodem", data->app);
 }
 
 static void gemalto_post_online(struct ofono_modem *modem)
-- 
1.9.1

[PATCH 2/6] gemalto: support ALS3 in gemalto's plugin

[Gabriel Lucas]

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

Force serial port opening options
Wait for modem to be ready to start
initializing it
Handle LTE
---
 plugins/gemalto.c | 106 +++++++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 94 insertions(+), 12 deletions(-)

diff --git a/plugins/gemalto.c b/plugins/gemalto.c
index 3739d7b..af50a9b 100644
--- a/plugins/gemalto.c
+++ b/plugins/gemalto.c
@@ -53,6 +53,10 @@
 
 #define HARDWARE_MONITOR_INTERFACE OFONO_SERVICE ".cinterion.HardwareMonitor"
 
+/* Supported gemalto's modem */
+#define GEMALTO_MODEL_PHS8P 	"0053"
+#define GEMALTO_MODEL_ALS3 		"0061"
+
 static const char *none_prefix[] = { NULL };
 static const char *sctm_prefix[] = { "^SCTM:", NULL };
 static const char *sbv_prefix[] = { "^SBV:", NULL };
@@ -70,6 +74,8 @@ struct gemalto_data {
 	gboolean have_sim;
 	struct at_util_sim_state_query *sim_state_query;
 	struct gemalto_hardware_monitor *hm;
+	guint modem_ready_id;
+	guint trial_cmd_id;
 };
 
 static int gemalto_probe(struct ofono_modem *modem)
@@ -107,10 +113,26 @@ static GAtChat *open_device(const char *device)
 	GAtSyntax *syntax;
 	GIOChannel *channel;
 	GAtChat *chat;
+	GHashTable *options;
+
+	options = g_hash_table_new(g_str_hash, g_str_equal);
+	if (options == NULL)
+		return NULL;
+
+	g_hash_table_insert(options, "Baud", "115200");
+	g_hash_table_insert(options, "StopBits", "1");
+	g_hash_table_insert(options, "DataBits", "8");
+	g_hash_table_insert(options, "Parity", "none");
+	g_hash_table_insert(options, "XonXoff", "off");
+	g_hash_table_insert(options, "RtsCts", "on");
+	g_hash_table_insert(options, "Local", "on");
+	g_hash_table_insert(options, "Read", "on");
 
 	DBG("Opening device %s", device);
 
-	channel = g_at_tty_open(device, NULL);
+	channel = g_at_tty_open(device, options);
+	g_hash_table_destroy(options);
+
 	if (channel == NULL)
 		return NULL;
 
@@ -300,29 +322,24 @@ static int gemalto_hardware_monitor_enable(struct ofono_modem *modem)
 	return 0;
 }
 
-static int gemalto_enable(struct ofono_modem *modem)
+static void gemalto_initialize(struct ofono_modem *modem)
 {
 	struct gemalto_data *data = ofono_modem_get_data(modem);
-	const char *app, *mdm;
+	const char *mdm;
 
-	DBG("%p", modem);
+	DBG("");
 
-	app = ofono_modem_get_string(modem, "Application");
 	mdm = ofono_modem_get_string(modem, "Modem");
 
-	if (app == NULL || mdm == NULL)
-		return -EINVAL;
+	if (mdm == NULL)
+		return;
 
 	/* Open devices */
-	data->app = open_device(app);
-	if (data->app == NULL)
-		return -EINVAL;
-
 	data->mdm = open_device(mdm);
 	if (data->mdm == NULL) {
 		g_at_chat_unref(data->app);
 		data->app = NULL;
-		return -EINVAL;
+		return;
 	}
 
 	if (getenv("OFONO_AT_DEBUG")) {
@@ -340,6 +357,67 @@ static int gemalto_enable(struct ofono_modem *modem)
 			cfun_enable, modem, NULL);
 
 	gemalto_hardware_monitor_enable(modem);
+}
+
+static void gemalto_modem_ready(GAtResult *result, gpointer user_data)
+{
+	struct ofono_modem *modem = user_data;
+	struct gemalto_data *data = ofono_modem_get_data(modem);
+	const char *app = ofono_modem_get_string(modem, "Application");
+
+	DBG("");
+
+	g_at_chat_unregister(data->app, data->modem_ready_id);
+	g_at_chat_cancel(data->app, data->trial_cmd_id);
+
+	/*!
+	 * As the modem wasn't ready to handle AT commands when we opened
+	 * it, we have to close and reopen the device app.
+	 */
+	g_at_chat_unref(data->app);
+
+	if(app == NULL)
+		return;
+
+	data->app = open_device(app);
+	if (data->app == NULL)
+		return;
+
+	gemalto_initialize(modem);
+}
+
+static void gemalto_at_cb(gboolean ok, GAtResult *result, gpointer user_data)
+{
+	struct ofono_modem *modem = user_data;
+	struct gemalto_data *data = ofono_modem_get_data(modem);
+
+	g_at_chat_unregister(data->app, data->modem_ready_id);
+	gemalto_initialize(modem);
+}
+
+static int gemalto_enable(struct ofono_modem *modem)
+{
+	struct gemalto_data *data = ofono_modem_get_data(modem);
+	const char *app, *mdm;
+
+	DBG("%p", modem);
+
+	app = ofono_modem_get_string(modem, "Application");
+	mdm = ofono_modem_get_string(modem, "Modem");
+
+	if (app == NULL || mdm == NULL)
+		return -EINVAL;
+
+	/* Open devices */
+	data->app = open_device(app);
+	if (data->app == NULL)
+		return -EINVAL;
+
+	/* Try the AT command. If it doesn't work, wait for ^SYSSTART */
+	data->modem_ready_id = g_at_chat_register(data->app, "^SYSSTART",
+				gemalto_modem_ready, FALSE, modem, NULL);
+	data->trial_cmd_id = g_at_chat_send(data->app, "ATE0 AT",
+				none_prefix, gemalto_at_cb, modem, NULL);
 
 	return -EINPROGRESS;
 }
@@ -432,6 +510,7 @@ static void gemalto_post_sim(struct ofono_modem *modem)
 	struct gemalto_data *data = ofono_modem_get_data(modem);
 	struct ofono_gprs *gprs;
 	struct ofono_gprs_context *gc;
+	const char *model = ofono_modem_get_string(modem, "Model");
 
 	DBG("%p", modem);
 
@@ -444,6 +523,9 @@ static void gemalto_post_sim(struct ofono_modem *modem)
 
 	if (gprs && gc)
 		ofono_gprs_add_context(gprs, gc);
+
+	if (!g_strcmp0(model, GEMALTO_MODEL_ALS3))
+		ofono_lte_create(modem, OFONO_VENDOR_CINTERION, "atmodem", data->app);
 }
 
 static void gemalto_post_online(struct ofono_modem *modem)
-- 
1.9.1

[PATCH 2/6] gemalto: support ALS3 in gemalto's plugin

[Gabriel Lucas]

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

Force serial port opening options
Wait for modem to be ready to start
initializing it
Handle LTE
---
 plugins/gemalto.c | 107 ++++++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 95 insertions(+), 12 deletions(-)

diff --git a/plugins/gemalto.c b/plugins/gemalto.c
index 3739d7b..3fb2904 100644
--- a/plugins/gemalto.c
+++ b/plugins/gemalto.c
@@ -53,6 +53,10 @@
 
 #define HARDWARE_MONITOR_INTERFACE OFONO_SERVICE ".cinterion.HardwareMonitor"
 
+/* Supported gemalto's modem */
+#define GEMALTO_MODEL_PHS8P 	"0053"
+#define GEMALTO_MODEL_ALS3 		"0061"
+
 static const char *none_prefix[] = { NULL };
 static const char *sctm_prefix[] = { "^SCTM:", NULL };
 static const char *sbv_prefix[] = { "^SBV:", NULL };
@@ -70,6 +74,8 @@ struct gemalto_data {
 	gboolean have_sim;
 	struct at_util_sim_state_query *sim_state_query;
 	struct gemalto_hardware_monitor *hm;
+	guint modem_ready_id;
+	guint trial_cmd_id;
 };
 
 static int gemalto_probe(struct ofono_modem *modem)
@@ -107,10 +113,26 @@ static GAtChat *open_device(const char *device)
 	GAtSyntax *syntax;
 	GIOChannel *channel;
 	GAtChat *chat;
+	GHashTable *options;
+
+	options = g_hash_table_new(g_str_hash, g_str_equal);
+	if (options == NULL)
+		return NULL;
+
+	g_hash_table_insert(options, "Baud", "115200");
+	g_hash_table_insert(options, "StopBits", "1");
+	g_hash_table_insert(options, "DataBits", "8");
+	g_hash_table_insert(options, "Parity", "none");
+	g_hash_table_insert(options, "XonXoff", "off");
+	g_hash_table_insert(options, "RtsCts", "on");
+	g_hash_table_insert(options, "Local", "on");
+	g_hash_table_insert(options, "Read", "on");
 
 	DBG("Opening device %s", device);
 
-	channel = g_at_tty_open(device, NULL);
+	channel = g_at_tty_open(device, options);
+	g_hash_table_destroy(options);
+
 	if (channel == NULL)
 		return NULL;
 
@@ -300,29 +322,24 @@ static int gemalto_hardware_monitor_enable(struct ofono_modem *modem)
 	return 0;
 }
 
-static int gemalto_enable(struct ofono_modem *modem)
+static void gemalto_initialize(struct ofono_modem *modem)
 {
 	struct gemalto_data *data = ofono_modem_get_data(modem);
-	const char *app, *mdm;
+	const char *mdm;
 
-	DBG("%p", modem);
+	DBG("");
 
-	app = ofono_modem_get_string(modem, "Application");
 	mdm = ofono_modem_get_string(modem, "Modem");
 
-	if (app == NULL || mdm == NULL)
-		return -EINVAL;
+	if (mdm == NULL)
+		return;
 
 	/* Open devices */
-	data->app = open_device(app);
-	if (data->app == NULL)
-		return -EINVAL;
-
 	data->mdm = open_device(mdm);
 	if (data->mdm == NULL) {
 		g_at_chat_unref(data->app);
 		data->app = NULL;
-		return -EINVAL;
+		return;
 	}
 
 	if (getenv("OFONO_AT_DEBUG")) {
@@ -340,6 +357,67 @@ static int gemalto_enable(struct ofono_modem *modem)
 			cfun_enable, modem, NULL);
 
 	gemalto_hardware_monitor_enable(modem);
+}
+
+static void gemalto_modem_ready(GAtResult *result, gpointer user_data)
+{
+	struct ofono_modem *modem = user_data;
+	struct gemalto_data *data = ofono_modem_get_data(modem);
+	const char *app = ofono_modem_get_string(modem, "Application");
+
+	DBG("");
+
+	g_at_chat_unregister(data->app, data->modem_ready_id);
+	g_at_chat_cancel(data->app, data->trial_cmd_id);
+
+	/*!
+	 * As the modem wasn't ready to handle AT commands when we opened
+	 * it, we have to close and reopen the device app.
+	 */
+	g_at_chat_unref(data->app);
+
+	if(app == NULL)
+		return;
+
+	data->app = open_device(app);
+	if (data->app == NULL)
+		return;
+
+	gemalto_initialize(modem);
+}
+
+static void gemalto_at_cb(gboolean ok, GAtResult *result, gpointer user_data)
+{
+	struct ofono_modem *modem = user_data;
+	struct gemalto_data *data = ofono_modem_get_data(modem);
+
+	g_at_chat_unregister(data->app, data->modem_ready_id);
+	gemalto_initialize(modem);
+}
+
+static int gemalto_enable(struct ofono_modem *modem)
+{
+	struct gemalto_data *data = ofono_modem_get_data(modem);
+	const char *app, *mdm;
+
+	DBG("%p", modem);
+
+	app = ofono_modem_get_string(modem, "Application");
+	mdm = ofono_modem_get_string(modem, "Modem");
+
+	if (app == NULL || mdm == NULL)
+		return -EINVAL;
+
+	/* Open devices */
+	data->app = open_device(app);
+	if (data->app == NULL)
+		return -EINVAL;
+
+	/* Try the AT command. If it doesn't work, wait for ^SYSSTART */
+	data->modem_ready_id = g_at_chat_register(data->app, "^SYSSTART",
+				gemalto_modem_ready, FALSE, modem, NULL);
+	data->trial_cmd_id = g_at_chat_send(data->app, "ATE0 AT",
+				none_prefix, gemalto_at_cb, modem, NULL);
 
 	return -EINPROGRESS;
 }
@@ -432,6 +510,7 @@ static void gemalto_post_sim(struct ofono_modem *modem)
 	struct gemalto_data *data = ofono_modem_get_data(modem);
 	struct ofono_gprs *gprs;
 	struct ofono_gprs_context *gc;
+	const char *model = ofono_modem_get_string(modem, "Model");
 
 	DBG("%p", modem);
 
@@ -444,6 +523,10 @@ static void gemalto_post_sim(struct ofono_modem *modem)
 
 	if (gprs && gc)
 		ofono_gprs_add_context(gprs, gc);
+
+	if (!g_strcmp0(model, GEMALTO_MODEL_ALS3))
+		ofono_lte_create(modem, OFONO_VENDOR_CINTERION,
+						"atmodem", data->app);
 }
 
 static void gemalto_post_online(struct ofono_modem *modem)
-- 
1.9.1

Re: [PATCH 2/6] gemalto: support ALS3 in gemalto's plugin

[Denis Kenzior]

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

Hi Gabriel,
<snip>

> @@ -300,29 +322,24 @@ static int gemalto_hardware_monitor_enable(struct ofono_modem *modem)
>   	return 0;
>   }
>   
> -static int gemalto_enable(struct ofono_modem *modem)
> +static void gemalto_initialize(struct ofono_modem *modem)
>   {
>   	struct gemalto_data *data = ofono_modem_get_data(modem);
> -	const char *app, *mdm;
> +	const char *mdm;
>   
> -	DBG("%p", modem);
> +	DBG("");
>   
> -	app = ofono_modem_get_string(modem, "Application");
>   	mdm = ofono_modem_get_string(modem, "Modem");
>   
> -	if (app == NULL || mdm == NULL)
> -		return -EINVAL;
> +	if (mdm == NULL)
> +		return;
>   
>   	/* Open devices */
> -	data->app = open_device(app);
> -	if (data->app == NULL)
> -		return -EINVAL;
> -
>   	data->mdm = open_device(mdm);
>   	if (data->mdm == NULL) {
>   		g_at_chat_unref(data->app);
>   		data->app = NULL;
> -		return -EINVAL;
> +		return;
>   	}
>   
>   	if (getenv("OFONO_AT_DEBUG")) {
> @@ -340,6 +357,67 @@ static int gemalto_enable(struct ofono_modem *modem)
>   			cfun_enable, modem, NULL);
>   
>   	gemalto_hardware_monitor_enable(modem);
> +}
> +
> +static void gemalto_modem_ready(GAtResult *result, gpointer user_data)
> +{
> +	struct ofono_modem *modem = user_data;
> +	struct gemalto_data *data = ofono_modem_get_data(modem);
> +	const char *app = ofono_modem_get_string(modem, "Application");
> +
> +	DBG("");
> +
> +	g_at_chat_unregister(data->app, data->modem_ready_id);
> +	g_at_chat_cancel(data->app, data->trial_cmd_id);

Since you're unrefing data->app below, this isn't really necessary.  You 
might want to reset the ids to zero here though.

> +
> +	/*!
> +	 * As the modem wasn't ready to handle AT commands when we opened
> +	 * it, we have to close and reopen the device app.
> +	 */ 
> +	g_at_chat_unref(data->app);
> +
> +	if(app == NULL)
> +		return;

Not our coding style.  Also, the "Application" property should not 
disappear under you, so I would leave this check out.

> +
> +	data->app = open_device(app);
> +	if (data->app == NULL)
> +		return;

You might want to tell the core that the enable operation failed.  So 
ofono_modem_set_powered(modem, FALSE);

> +
> +	gemalto_initialize(modem);
> +}
> +
> +static void gemalto_at_cb(gboolean ok, GAtResult *result, gpointer user_data)
> +{
> +	struct ofono_modem *modem = user_data;
> +	struct gemalto_data *data = ofono_modem_get_data(modem);
> +
> +	g_at_chat_unregister(data->app, data->modem_ready_id);
> +	gemalto_initialize(modem);

Do you also want to reset modem_ready_id to 0 as well?  And trial_cmd_id

> +}
> +
> +static int gemalto_enable(struct ofono_modem *modem)
> +{
> +	struct gemalto_data *data = ofono_modem_get_data(modem);
> +	const char *app, *mdm;
> +
> +	DBG("%p", modem);
> +
> +	app = ofono_modem_get_string(modem, "Application");
> +	mdm = ofono_modem_get_string(modem, "Modem");
> +
> +	if (app == NULL || mdm == NULL)
> +		return -EINVAL;
> +
> +	/* Open devices */
> +	data->app = open_device(app);
> +	if (data->app == NULL)
> +		return -EINVAL;
> +
> +	/* Try the AT command. If it doesn't work, wait for ^SYSSTART */
> +	data->modem_ready_id = g_at_chat_register(data->app, "^SYSSTART",
> +				gemalto_modem_ready, FALSE, modem, NULL);
> +	data->trial_cmd_id = g_at_chat_send(data->app, "ATE0 AT",
> +				none_prefix, gemalto_at_cb, modem, NULL);
>   
>   	return -EINPROGRESS;
>   }
> @@ -432,6 +510,7 @@ static void gemalto_post_sim(struct ofono_modem *modem)
>   	struct gemalto_data *data = ofono_modem_get_data(modem);
>   	struct ofono_gprs *gprs;
>   	struct ofono_gprs_context *gc;
> +	const char *model = ofono_modem_get_string(modem, "Model");
>   
>   	DBG("%p", modem);
>   
> @@ -444,6 +523,9 @@ static void gemalto_post_sim(struct ofono_modem *modem)
>   
>   	if (gprs && gc)
>   		ofono_gprs_add_context(gprs, gc);
> +
> +	if (!g_strcmp0(model, GEMALTO_MODEL_ALS3))
> +		ofono_lte_create(modem, OFONO_VENDOR_CINTERION, "atmodem", data->app);

Over 80 characters here

>   }
>   
>   static void gemalto_post_online(struct ofono_modem *modem)
> 

Regards,
-Denis

Re: [PATCH 2/6] gemalto: support ALS3 in gemalto's plugin

[Gabriel Lucas]

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

Hi Denis,


On 16/03/2018 17:08, Denis Kenzior wrote:
>> +
>> +static void gemalto_modem_ready(GAtResult *result, gpointer user_data)
>> +{
>> +    struct ofono_modem *modem = user_data;
>> +    struct gemalto_data *data = ofono_modem_get_data(modem);
>> +    const char *app = ofono_modem_get_string(modem, "Application");
>> +
>> +    DBG("");
>> +
>> +    g_at_chat_unregister(data->app, data->modem_ready_id);
>> +    g_at_chat_cancel(data->app, data->trial_cmd_id);
>
> Since you're unrefing data->app below, this isn't really necessary.  
> You might want to reset the ids to zero here though.
Done
>
>> +
>> +    /*!
>> +     * As the modem wasn't ready to handle AT commands when we opened
>> +     * it, we have to close and reopen the device app.
>> +     */ +    g_at_chat_unref(data->app);
>> +
>> +    if(app == NULL)
>> +        return;
>
> Not our coding style.  Also, the "Application" property should not 
> disappear under you, so I would leave this check out.
Is it like this :
/* foo bar
  * bar
*/
or
/*
  * foo bar
  * bar
*/
?

I've removed the checking.
>
>> +
>> +    data->app = open_device(app);
>> +    if (data->app == NULL)
>> +        return;
>
> You might want to tell the core that the enable operation failed. So 
> ofono_modem_set_powered(modem, FALSE);
Done
>
>> +
>> +    gemalto_initialize(modem);
>> +}
>> +
>> +static void gemalto_at_cb(gboolean ok, GAtResult *result, gpointer 
>> user_data)
>> +{
>> +    struct ofono_modem *modem = user_data;
>> +    struct gemalto_data *data = ofono_modem_get_data(modem);
>> +
>> +    g_at_chat_unregister(data->app, data->modem_ready_id);
>> +    gemalto_initialize(modem);
>
> Do you also want to reset modem_ready_id to 0 as well?  And trial_cmd_id
Done
>
>> +}
>> +
>> +static int gemalto_enable(struct ofono_modem *modem)
>> +{
>> +    struct gemalto_data *data = ofono_modem_get_data(modem);
>> +    const char *app, *mdm;
>> +
>> +    DBG("%p", modem);
>> +
>> +    app = ofono_modem_get_string(modem, "Application");
>> +    mdm = ofono_modem_get_string(modem, "Modem");
Here, I've realized I'm getting the "Modem" property but I'm not
opening it. Should I still get it here such that I could check that it
isn't NULL ? Or do you prefer it to be handled in gemalto_initialize() ?
>> +
>> +    if (app == NULL || mdm == NULL)
>> +        return -EINVAL;
>> +
>> +    /* Open devices */
>> +    data->app = open_device(app);
>> +    if (data->app == NULL)
>> +        return -EINVAL;
>> +
>> +    /* Try the AT command. If it doesn't work, wait for ^SYSSTART */
>> +    data->modem_ready_id = g_at_chat_register(data->app, "^SYSSTART",
>> +                gemalto_modem_ready, FALSE, modem, NULL);
>> +    data->trial_cmd_id = g_at_chat_send(data->app, "ATE0 AT",
>> +                none_prefix, gemalto_at_cb, modem, NULL);
>>         return -EINPROGRESS;
>>   }
>> @@ -432,6 +510,7 @@ static void gemalto_post_sim(struct ofono_modem 
>> *modem)
>>       struct gemalto_data *data = ofono_modem_get_data(modem);
>>       struct ofono_gprs *gprs;
>>       struct ofono_gprs_context *gc;
>> +    const char *model = ofono_modem_get_string(modem, "Model");
>>         DBG("%p", modem);
>>   @@ -444,6 +523,9 @@ static void gemalto_post_sim(struct ofono_modem 
>> *modem)
>>         if (gprs && gc)
>>           ofono_gprs_add_context(gprs, gc);
>> +
>> +    if (!g_strcmp0(model, GEMALTO_MODEL_ALS3))
>> +        ofono_lte_create(modem, OFONO_VENDOR_CINTERION, "atmodem", 
>> data->app);
>
> Over 80 characters here
Done
>
>>   }
>>     static void gemalto_post_online(struct ofono_modem *modem)
>>
>
> Regards,
> -Denis
Best regards,
Gabriel

Re: [PATCH 2/6] gemalto: support ALS3 in gemalto's plugin

[Denis Kenzior]

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

Hi Gabriel,

>>> +    if(app == NULL)
>>> +        return;
>>
>> Not our coding style.  Also, the "Application" property should not 
>> disappear under you, so I would leave this check out.
> Is it like this :
> /* foo bar
>   * bar
> */
> or
> /*
>   * foo bar
>   * bar
> */
> ?

For multi-line comments it the latter. See doc/coding-style.txt item M2. 
  For single line comments:
/* foo bar */ is acceptable.

However, I meant there's no space between if and () I believe.

>>> +
>>> +    app = ofono_modem_get_string(modem, "Application");
>>> +    mdm = ofono_modem_get_string(modem, "Modem");
> Here, I've realized I'm getting the "Modem" property but I'm not
> opening it. Should I still get it here such that I could check that it
> isn't NULL ? Or do you prefer it to be handled in gemalto_initialize() ?

I would check it here since the error path is easier.

Regards,
-Denis

[PATCH 2/6] gemalto: support ALS3 in gemalto's plugin

[Gabriel Lucas]

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

Force serial port opening options
Wait for modem to be ready to start
initializing it
Handle LTE
---
 plugins/gemalto.c | 107 ++++++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 95 insertions(+), 12 deletions(-)

diff --git a/plugins/gemalto.c b/plugins/gemalto.c
index 3739d7b..6979683 100644
--- a/plugins/gemalto.c
+++ b/plugins/gemalto.c
@@ -53,6 +53,10 @@
 
 #define HARDWARE_MONITOR_INTERFACE OFONO_SERVICE ".cinterion.HardwareMonitor"
 
+/* Supported gemalto's modem */
+#define GEMALTO_MODEL_PHS8P 	"0053"
+#define GEMALTO_MODEL_ALS3 	"0061"
+
 static const char *none_prefix[] = { NULL };
 static const char *sctm_prefix[] = { "^SCTM:", NULL };
 static const char *sbv_prefix[] = { "^SBV:", NULL };
@@ -70,6 +74,8 @@ struct gemalto_data {
 	gboolean have_sim;
 	struct at_util_sim_state_query *sim_state_query;
 	struct gemalto_hardware_monitor *hm;
+	guint modem_ready_id;
+	guint trial_cmd_id;
 };
 
 static int gemalto_probe(struct ofono_modem *modem)
@@ -107,10 +113,26 @@ static GAtChat *open_device(const char *device)
 	GAtSyntax *syntax;
 	GIOChannel *channel;
 	GAtChat *chat;
+	GHashTable *options;
+
+	options = g_hash_table_new(g_str_hash, g_str_equal);
+	if (options == NULL)
+		return NULL;
+
+	g_hash_table_insert(options, "Baud", "115200");
+	g_hash_table_insert(options, "StopBits", "1");
+	g_hash_table_insert(options, "DataBits", "8");
+	g_hash_table_insert(options, "Parity", "none");
+	g_hash_table_insert(options, "XonXoff", "off");
+	g_hash_table_insert(options, "RtsCts", "on");
+	g_hash_table_insert(options, "Local", "on");
+	g_hash_table_insert(options, "Read", "on");
 
 	DBG("Opening device %s", device);
 
-	channel = g_at_tty_open(device, NULL);
+	channel = g_at_tty_open(device, options);
+	g_hash_table_destroy(options);
+
 	if (channel == NULL)
 		return NULL;
 
@@ -300,29 +322,24 @@ static int gemalto_hardware_monitor_enable(struct ofono_modem *modem)
 	return 0;
 }
 
-static int gemalto_enable(struct ofono_modem *modem)
+static void gemalto_initialize(struct ofono_modem *modem)
 {
 	struct gemalto_data *data = ofono_modem_get_data(modem);
-	const char *app, *mdm;
+	const char *mdm;
 
-	DBG("%p", modem);
+	DBG("");
 
-	app = ofono_modem_get_string(modem, "Application");
 	mdm = ofono_modem_get_string(modem, "Modem");
 
-	if (app == NULL || mdm == NULL)
-		return -EINVAL;
+	if (mdm == NULL)
+		return;
 
 	/* Open devices */
-	data->app = open_device(app);
-	if (data->app == NULL)
-		return -EINVAL;
-
 	data->mdm = open_device(mdm);
 	if (data->mdm == NULL) {
 		g_at_chat_unref(data->app);
 		data->app = NULL;
-		return -EINVAL;
+		return;
 	}
 
 	if (getenv("OFONO_AT_DEBUG")) {
@@ -340,6 +357,67 @@ static int gemalto_enable(struct ofono_modem *modem)
 			cfun_enable, modem, NULL);
 
 	gemalto_hardware_monitor_enable(modem);
+}
+
+static void gemalto_modem_ready(GAtResult *result, gpointer user_data)
+{
+	struct ofono_modem *modem = user_data;
+	struct gemalto_data *data = ofono_modem_get_data(modem);
+	const char *app = ofono_modem_get_string(modem, "Application");
+
+	DBG("");
+
+	/* 
+	 * As the modem wasn't ready to handle AT commands when we opened
+	 * it, we have to close and reopen the device app.
+	 */
+	data->modem_ready_id = 0;
+	data->trial_cmd_id = 0;
+
+	g_at_chat_unref(data->app);
+
+	data->app = open_device(app);
+	if (data->app == NULL) {
+		ofono_modem_set_powered(modem, FALSE);
+	} else {
+		gemalto_initialize(modem);
+	}
+}
+
+static void gemalto_at_cb(gboolean ok, GAtResult *result, gpointer user_data)
+{
+	struct ofono_modem *modem = user_data;
+	struct gemalto_data *data = ofono_modem_get_data(modem);
+
+	g_at_chat_unregister(data->app, data->modem_ready_id);
+	data->modem_ready_id = 0;
+
+	gemalto_initialize(modem);
+}
+
+static int gemalto_enable(struct ofono_modem *modem)
+{
+	struct gemalto_data *data = ofono_modem_get_data(modem);
+	const char *app, *mdm;
+
+	DBG("%p", modem);
+
+	app = ofono_modem_get_string(modem, "Application");
+	mdm = ofono_modem_get_string(modem, "Modem");
+
+	if (app == NULL || mdm == NULL)
+		return -EINVAL;
+
+	/* Open devices */
+	data->app = open_device(app);
+	if (data->app == NULL)
+		return -EINVAL;
+
+	/* Try the AT command. If it doesn't work, wait for ^SYSSTART */
+	data->modem_ready_id = g_at_chat_register(data->app, "^SYSSTART",
+				gemalto_modem_ready, FALSE, modem, NULL);
+	data->trial_cmd_id = g_at_chat_send(data->app, "ATE0 AT",
+				none_prefix, gemalto_at_cb, modem, NULL);
 
 	return -EINPROGRESS;
 }
@@ -432,6 +510,7 @@ static void gemalto_post_sim(struct ofono_modem *modem)
 	struct gemalto_data *data = ofono_modem_get_data(modem);
 	struct ofono_gprs *gprs;
 	struct ofono_gprs_context *gc;
+	const char *model = ofono_modem_get_string(modem, "Model");
 
 	DBG("%p", modem);
 
@@ -444,6 +523,10 @@ static void gemalto_post_sim(struct ofono_modem *modem)
 
 	if (gprs && gc)
 		ofono_gprs_add_context(gprs, gc);
+
+	if (!g_strcmp0(model, GEMALTO_MODEL_ALS3))
+		ofono_lte_create(modem, OFONO_VENDOR_CINTERION,
+						"atmodem", data->app);
 }
 
 static void gemalto_post_online(struct ofono_modem *modem)
-- 
1.9.1

Re: [PATCH 2/6] gemalto: support ALS3 in gemalto's plugin

[Denis Kenzior]

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

Hi Gabriel,

On 03/19/2018 10:15 AM, Gabriel Lucas wrote:
> Force serial port opening options
> Wait for modem to be ready to start
> initializing it
> Handle LTE
> ---
>   plugins/gemalto.c | 107 ++++++++++++++++++++++++++++++++++++++++++++++++------
>   1 file changed, 95 insertions(+), 12 deletions(-)
> 

Applied, thanks.

Regards,
-Denis

[PATCH 3/6] gemalto: acquire the network technology

[Gabriel Lucas]

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

From: Mariem Cherif <mariem.cherif@ardia.com.tn>

---
 drivers/atmodem/network-registration.c | 45 ++++++++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)

diff --git a/drivers/atmodem/network-registration.c b/drivers/atmodem/network-registration.c
index a5e2af3..aec9c2d 100644
--- a/drivers/atmodem/network-registration.c
+++ b/drivers/atmodem/network-registration.c
@@ -48,6 +48,7 @@ static const char *cops_prefix[] = { "+COPS:", NULL };
 static const char *csq_prefix[] = { "+CSQ:", NULL };
 static const char *cind_prefix[] = { "+CIND:", NULL };
 static const char *cmer_prefix[] = { "+CMER:", NULL };
+static const char *smoni_prefix[] = { "^SMONI:", "", NULL };
 static const char *zpas_prefix[] = { "+ZPAS:", NULL };
 static const char *option_tech_prefix[] = { "_OCTI:", "_OUWCTI:", NULL };
 
@@ -178,6 +179,32 @@ static int option_parse_tech(GAtResult *result)
 	return tech;
 }
 
+static int cinterion_parse_tech(GAtResult *result)
+{
+	int tech = -1;
+	GAtResultIter iter;
+	GSList *l;
+	g_at_result_iter_init(&iter, result);
+	l = result->lines;
+	if (strstr(l->data, "^SMONI: ") != NULL) {
+		gchar **body = g_strsplit(l->data, "^SMONI: ", 2);
+		if (*body != NULL) {
+			gchar **data = g_strsplit(body[1], ",", 20);
+			if (*data != NULL) {
+				if (g_strcmp0(data[0], "2G") == 0) {
+					tech = ACCESS_TECHNOLOGY_GSM;
+				} else if (g_strcmp0 (data[0], "3G") == 0) {
+					tech = ACCESS_TECHNOLOGY_UTRAN;
+				} else if (g_strcmp0 (data[0], "4G") == 0) {
+					tech = ACCESS_TECHNOLOGY_EUTRAN;
+				}
+			}
+			g_strfreev(body);
+		}
+	}
+	return tech;
+}
+
 static void at_creg_cb(gboolean ok, GAtResult *result, gpointer user_data)
 {
 	struct cb_data *cbd = user_data;
@@ -205,6 +232,18 @@ static void at_creg_cb(gboolean ok, GAtResult *result, gpointer user_data)
 	cb(&error, status, lac, ci, tech, cbd->data);
 }
 
+static void cinterion_query_tech_cb(gboolean ok, GAtResult *result,
+                                              gpointer user_data)
+{
+	struct tech_query *tq = user_data;
+	int tech;
+
+	tech = cinterion_parse_tech(result);
+
+	ofono_netreg_status_notify(tq->netreg,
+			tq->status, tq->lac, tq->ci, tech);
+}
+
 static void zte_tech_cb(gboolean ok, GAtResult *result, gpointer user_data)
 {
 	struct cb_data *cbd = user_data;
@@ -1518,6 +1557,12 @@ static void creg_notify(GAtResult *result, gpointer user_data)
 					option_query_tech_cb, tq, g_free) > 0)
 			return;
 		break;
+    case OFONO_VENDOR_CINTERION:
+              if (g_at_chat_send(nd->chat, "AT^SMONI",
+                                      smoni_prefix,
+                                      cinterion_query_tech_cb, tq, g_free) > 0)
+                      return;
+              break;
 	}
 
 	g_free(tq);
-- 
1.9.1

Re: [PATCH 3/6] gemalto: acquire the network technology

[Denis Kenzior]

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

Hi Gabriel,

On 03/15/2018 07:49 AM, Gabriel Lucas wrote:
> From: Mariem Cherif <mariem.cherif@ardia.com.tn>
> 
> ---
>   drivers/atmodem/network-registration.c | 45 ++++++++++++++++++++++++++++++++++
>   1 file changed, 45 insertions(+)
> 
> diff --git a/drivers/atmodem/network-registration.c b/drivers/atmodem/network-registration.c
> index a5e2af3..aec9c2d 100644
> --- a/drivers/atmodem/network-registration.c
> +++ b/drivers/atmodem/network-registration.c
> @@ -48,6 +48,7 @@ static const char *cops_prefix[] = { "+COPS:", NULL };
>   static const char *csq_prefix[] = { "+CSQ:", NULL };
>   static const char *cind_prefix[] = { "+CIND:", NULL };
>   static const char *cmer_prefix[] = { "+CMER:", NULL };
> +static const char *smoni_prefix[] = { "^SMONI:", "", NULL };

Is there a reason why "" is added?  Is the modem sending lines not 
prefixed by '^SMONI:' in the response?

>   static const char *zpas_prefix[] = { "+ZPAS:", NULL };
>   static const char *option_tech_prefix[] = { "_OCTI:", "_OUWCTI:", NULL };
>   
> @@ -178,6 +179,32 @@ static int option_parse_tech(GAtResult *result)
>   	return tech;
>   }
>   
> +static int cinterion_parse_tech(GAtResult *result)
> +{
> +	int tech = -1;
> +	GAtResultIter iter;
> +	GSList *l;

new line here please

> +	g_at_result_iter_init(&iter, result);
> +	l = result->lines;

What does the output look like?

> +	if (strstr(l->data, "^SMONI: ") != NULL) {

Generally we use g_at_result_iter_next("^SMONI:");

> +		gchar **body = g_strsplit(l->data, "^SMONI: ", 2);
> +		if (*body != NULL) {
> +			gchar **data = g_strsplit(body[1], ",", 20);
> +			if (*data != NULL) {
> +				if (g_strcmp0(data[0], "2G") == 0) {
> +					tech = ACCESS_TECHNOLOGY_GSM;
> +				} else if (g_strcmp0 (data[0], "3G") == 0) {
> +					tech = ACCESS_TECHNOLOGY_UTRAN;
> +				} else if (g_strcmp0 (data[0], "4G") == 0) {
> +					tech = ACCESS_TECHNOLOGY_EUTRAN;
> +				}
> +			}

Are you leaking data?  Have you run this through valgrind?

> +			g_strfreev(body);

It seems all of this can be accomplished with 
g_at_result_iter_next_unquoted_string.

> +		}
> +	}
> +	return tech;
> +}
> +
>   static void at_creg_cb(gboolean ok, GAtResult *result, gpointer user_data)
>   {
>   	struct cb_data *cbd = user_data;
> @@ -205,6 +232,18 @@ static void at_creg_cb(gboolean ok, GAtResult *result, gpointer user_data)
>   	cb(&error, status, lac, ci, tech, cbd->data);
>   }
>   
> +static void cinterion_query_tech_cb(gboolean ok, GAtResult *result,
> +                                              gpointer user_data)
> +{
> +	struct tech_query *tq = user_data;
> +	int tech;
> +
> +	tech = cinterion_parse_tech(result);
> +
> +	ofono_netreg_status_notify(tq->netreg,
> +			tq->status, tq->lac, tq->ci, tech);
> +}
> +
>   static void zte_tech_cb(gboolean ok, GAtResult *result, gpointer user_data)
>   {
>   	struct cb_data *cbd = user_data;
> @@ -1518,6 +1557,12 @@ static void creg_notify(GAtResult *result, gpointer user_data)
>   					option_query_tech_cb, tq, g_free) > 0)
>   			return;
>   		break;
> +    case OFONO_VENDOR_CINTERION:
> +              if (g_at_chat_send(nd->chat, "AT^SMONI",
> +                                      smoni_prefix,
> +                                      cinterion_query_tech_cb, tq, g_free) > 0)
> +                      return;
> +              break;
>   	}
>   
>   	g_free(tq);
> 

Regards,
-Denis

Re: [PATCH 3/6] gemalto: acquire the network technology

[Gabriel Lucas]

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

Hi Denis,

Really sorry, this patch is from Mariem and I haven't reviewed it.

the cinterion_parse_tech function really needs to be rework. I'll resend you
a cleaner version today.

On 15/03/2018 18:19, Denis Kenzior wrote:
> Hi Gabriel,
>
> On 03/15/2018 07:49 AM, Gabriel Lucas wrote:
>> From: Mariem Cherif <mariem.cherif@ardia.com.tn>
>>
>> ---
>>   drivers/atmodem/network-registration.c | 45 
>> ++++++++++++++++++++++++++++++++++
>>   1 file changed, 45 insertions(+)
>>
>> diff --git a/drivers/atmodem/network-registration.c 
>> b/drivers/atmodem/network-registration.c
>> index a5e2af3..aec9c2d 100644
>> --- a/drivers/atmodem/network-registration.c
>> +++ b/drivers/atmodem/network-registration.c
>> @@ -48,6 +48,7 @@ static const char *cops_prefix[] = { "+COPS:", NULL };
>>   static const char *csq_prefix[] = { "+CSQ:", NULL };
>>   static const char *cind_prefix[] = { "+CIND:", NULL };
>>   static const char *cmer_prefix[] = { "+CMER:", NULL };
>> +static const char *smoni_prefix[] = { "^SMONI:", "", NULL };
>
> Is there a reason why "" is added?  Is the modem sending lines not 
> prefixed by '^SMONI:' in the response?
>
>>   static const char *zpas_prefix[] = { "+ZPAS:", NULL };
>>   static const char *option_tech_prefix[] = { "_OCTI:", "_OUWCTI:", 
>> NULL };
>>   @@ -178,6 +179,32 @@ static int option_parse_tech(GAtResult *result)
>>       return tech;
>>   }
>>   +static int cinterion_parse_tech(GAtResult *result)
>> +{
>> +    int tech = -1;
>> +    GAtResultIter iter;
>> +    GSList *l;
>
> new line here please
>
>> +    g_at_result_iter_init(&iter, result);
>> +    l = result->lines;
>
> What does the output look like?
>
>> +    if (strstr(l->data, "^SMONI: ") != NULL) {
>
> Generally we use g_at_result_iter_next("^SMONI:");
>
>> +        gchar **body = g_strsplit(l->data, "^SMONI: ", 2);
>> +        if (*body != NULL) {
>> +            gchar **data = g_strsplit(body[1], ",", 20);
>> +            if (*data != NULL) {
>> +                if (g_strcmp0(data[0], "2G") == 0) {
>> +                    tech = ACCESS_TECHNOLOGY_GSM;
>> +                } else if (g_strcmp0 (data[0], "3G") == 0) {
>> +                    tech = ACCESS_TECHNOLOGY_UTRAN;
>> +                } else if (g_strcmp0 (data[0], "4G") == 0) {
>> +                    tech = ACCESS_TECHNOLOGY_EUTRAN;
>> +                }
>> +            }
>
> Are you leaking data?  Have you run this through valgrind?
>
>> +            g_strfreev(body);
>
> It seems all of this can be accomplished with 
> g_at_result_iter_next_unquoted_string.
>
> Regards,
> -Denis

[PATCH 3/6] gemalto: acquire the network technology

[Gabriel Lucas]

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

From: Mariem Cherif <mariem.cherif@ardia.com.tn>

---
 drivers/atmodem/network-registration.c | 45 ++++++++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)

diff --git a/drivers/atmodem/network-registration.c b/drivers/atmodem/network-registration.c
index a5e2af3..80f6291 100644
--- a/drivers/atmodem/network-registration.c
+++ b/drivers/atmodem/network-registration.c
@@ -48,6 +48,7 @@ static const char *cops_prefix[] = { "+COPS:", NULL };
 static const char *csq_prefix[] = { "+CSQ:", NULL };
 static const char *cind_prefix[] = { "+CIND:", NULL };
 static const char *cmer_prefix[] = { "+CMER:", NULL };
+static const char *smoni_prefix[] = { "^SMONI:", NULL };
 static const char *zpas_prefix[] = { "+ZPAS:", NULL };
 static const char *option_tech_prefix[] = { "_OCTI:", "_OUWCTI:", NULL };
 
@@ -178,6 +179,32 @@ static int option_parse_tech(GAtResult *result)
 	return tech;
 }
 
+static int cinterion_parse_tech(GAtResult *result)
+{
+	int tech = -1;
+	GAtResultIter iter;
+	GSList *l;
+	const char *technology;
+
+	g_at_result_iter_init(&iter, result);
+
+	if (!g_at_result_iter_next(&iter, "^SMONI: "))
+		return tech;
+
+	if (!g_at_result_iter_next_unquoted_string(&iter, &technology))
+		return tech;
+
+	if (strcmp(technology, "2G") == 0) {
+		tech = ACCESS_TECHNOLOGY_GSM_EGPRS;
+	} else if (strcmp(technology, "3G") == 0) {
+		tech = ACCESS_TECHNOLOGY_UTRAN;
+	} else if (strcmp(technology, "4G") == 0) {
+		tech = ACCESS_TECHNOLOGY_EUTRAN;
+	}
+
+	return tech;
+}
+
 static void at_creg_cb(gboolean ok, GAtResult *result, gpointer user_data)
 {
 	struct cb_data *cbd = user_data;
@@ -205,6 +232,18 @@ static void at_creg_cb(gboolean ok, GAtResult *result, gpointer user_data)
 	cb(&error, status, lac, ci, tech, cbd->data);
 }
 
+static void cinterion_query_tech_cb(gboolean ok, GAtResult *result,
+                                              gpointer user_data)
+{
+	struct tech_query *tq = user_data;
+	int tech;
+
+	tech = cinterion_parse_tech(result);
+
+	ofono_netreg_status_notify(tq->netreg,
+			tq->status, tq->lac, tq->ci, tech);
+}
+
 static void zte_tech_cb(gboolean ok, GAtResult *result, gpointer user_data)
 {
 	struct cb_data *cbd = user_data;
@@ -1518,6 +1557,12 @@ static void creg_notify(GAtResult *result, gpointer user_data)
 					option_query_tech_cb, tq, g_free) > 0)
 			return;
 		break;
+    case OFONO_VENDOR_CINTERION:
+              if (g_at_chat_send(nd->chat, "AT^SMONI",
+                                      smoni_prefix,
+                                      cinterion_query_tech_cb, tq, g_free) > 0)
+                      return;
+              break;
 	}
 
 	g_free(tq);
-- 
1.9.1

Re: [PATCH 3/6] gemalto: acquire the network technology

[Denis Kenzior]

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

Hi Gabriel, Mariem,

On 03/16/2018 07:59 AM, Gabriel Lucas wrote:
> From: Mariem Cherif <mariem.cherif@ardia.com.tn>
> 
> ---
>   drivers/atmodem/network-registration.c | 45 ++++++++++++++++++++++++++++++++++
>   1 file changed, 45 insertions(+)

When applying this patch I got:

   CC       drivers/atmodem/network-registration.o
drivers/atmodem/network-registration.c: In function ‘cinterion_parse_tech’:
drivers/atmodem/network-registration.c:186:10: error: unused variable 
‘l’ [-Werror=unused-variable]
   GSList *l;
           ^
drivers/atmodem/network-registration.c: At top level:
cc1: error: unrecognized command line option ‘-Wno-format-truncation’ 
[-Werror]
cc1: all warnings being treated as errors

Please make sure you test with --disable-debug --enable-optimization 
prior to submission so the compiler catches any unused variables.

I went ahead and removed the offending GSList *l declaration and applied 
this patch.  Thanks!

Regards,
-Denis

Re: [PATCH 3/6] gemalto: acquire the network technology

[Gabriel LUCAS]

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

Thanks Denis. My apologies for the unused variable.

On 2018-03-16 15:27, Denis Kenzior wrote:

> Hi Gabriel, Mariem,
> 
> On 03/16/2018 07:59 AM, Gabriel Lucas wrote:
> 
>> From: Mariem Cherif <mariem.cherif@ardia.com.tn> ---
>> drivers/atmodem/network-registration.c | 45
>> ++++++++++++++++++++++++++++++++++ 1 file changed, 45 insertions(+)
> 
> When applying this patch I got:
> 
> CC drivers/atmodem/network-registration.o
> drivers/atmodem/network-registration.c: In function
> 'cinterion_parse_tech':
> drivers/atmodem/network-registration.c:186:10: error: unused variable
> 'l' [-Werror=unused-variable]
> GSList *l;
> ^
> drivers/atmodem/network-registration.c: At top level:
> cc1: error: unrecognized command line option '-Wno-format-truncation'
> [-Werror]
> cc1: all warnings being treated as errors
> 
> Please make sure you test with --disable-debug --enable-optimization
> prior to submission so the compiler catches any unused variables.

It seems that my sdk's environment isn't configured to show all 
warnings.
I'm gonna fix that.

> I went ahead and removed the offending GSList *l declaration and 
> applied
> this patch. Thanks!
> 
> Regards,
> -Denis

Gabriel

[PATCH 4/6] gemalto: handle sim is inserted or removed URCs

[Gabriel Lucas]

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

From: Mariem Cherif <mariem.cherif@ardia.com.tn>

---
 plugins/gemalto.c | 37 +++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/plugins/gemalto.c b/plugins/gemalto.c
index 16ca463..c7fb783 100644
--- a/plugins/gemalto.c
+++ b/plugins/gemalto.c
@@ -497,6 +497,36 @@ static void gemalto_set_online(struct ofono_modem *modem, ofono_bool_t online,
 	g_free(cbd);
 }
 
+static void gemalto_ciev_notify(GAtResult *result, gpointer user_data)
+{
+	struct ofono_sim *sim = user_data;
+	const char *sim_status = "simstatus";
+	const char *ind_str;
+	int status;
+	GAtResultIter iter;
+
+	g_at_result_iter_init(&iter, result);
+
+	if (!g_at_result_iter_next(&iter, "+CIEV:"))
+		return;
+
+	if (!g_at_result_iter_next_unquoted_string(&iter, &ind_str))
+		return;
+
+	if (!g_str_equal(sim_status, ind_str))
+		return;
+
+	if (!g_at_result_iter_next_number(&iter, &status))
+		return;
+
+	DBG("sim status %d", status);
+	if (status == 0) {
+		ofono_sim_inserted_notify(sim, FALSE);
+	} else if(status == 1) {
+		ofono_sim_inserted_notify(sim, TRUE);
+	}
+}
+
 static void gemalto_pre_sim(struct ofono_modem *modem)
 {
 	struct gemalto_data *data = ofono_modem_get_data(modem);
@@ -509,6 +539,13 @@ static void gemalto_pre_sim(struct ofono_modem *modem)
 	sim = ofono_sim_create(modem, OFONO_VENDOR_CINTERION, "atmodem",
 						data->app);
 
+	/* Register for specific sim status reports */
+	g_at_chat_register(data->app, "+CIEV:",
+			gemalto_ciev_notify, FALSE, sim, NULL);
+
+	g_at_chat_send(data->app, "AT^SIND=\"simstatus\",1", none_prefix,
+			NULL, NULL, NULL);
+
 	if (sim && data->have_sim == TRUE)
 		ofono_sim_inserted_notify(sim, TRUE);
 }
-- 
1.9.1

Re: [PATCH 4/6] gemalto: handle sim is inserted or removed URCs

[Denis Kenzior]

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

Hi Gabriel,

On 03/15/2018 07:49 AM, Gabriel Lucas wrote:
> From: Mariem Cherif <mariem.cherif@ardia.com.tn>
> 
> ---
>   plugins/gemalto.c | 37 +++++++++++++++++++++++++++++++++++++
>   1 file changed, 37 insertions(+)
> 
> diff --git a/plugins/gemalto.c b/plugins/gemalto.c
> index 16ca463..c7fb783 100644
> --- a/plugins/gemalto.c
> +++ b/plugins/gemalto.c
> @@ -497,6 +497,36 @@ static void gemalto_set_online(struct ofono_modem *modem, ofono_bool_t online,
>   	g_free(cbd);
>   }
>   
> +static void gemalto_ciev_notify(GAtResult *result, gpointer user_data)
> +{
> +	struct ofono_sim *sim = user_data;
> +	const char *sim_status = "simstatus";
> +	const char *ind_str;
> +	int status;
> +	GAtResultIter iter;
> +
> +	g_at_result_iter_init(&iter, result);
> +
> +	if (!g_at_result_iter_next(&iter, "+CIEV:"))
> +		return;
> +

So generally +CIEV indication is an <index>,<value> syntax.

> +	if (!g_at_result_iter_next_unquoted_string(&iter, &ind_str))
> +		return;

Does Gemalto use some other syntax here?  If so, you might want to 
document a simple example above.

> +
> +	if (!g_str_equal(sim_status, ind_str))
> +		return;
> +
> +	if (!g_at_result_iter_next_number(&iter, &status))
> +		return;
> +
> +	DBG("sim status %d", status);
> +	if (status == 0) {
> +		ofono_sim_inserted_notify(sim, FALSE);
> +	} else if(status == 1) {
> +		ofono_sim_inserted_notify(sim, TRUE);
> +	}

Okay, but simpler written as ofono_sim_inserted_notify(sim, status);

> +}
> +
>   static void gemalto_pre_sim(struct ofono_modem *modem)
>   {
>   	struct gemalto_data *data = ofono_modem_get_data(modem);
> @@ -509,6 +539,13 @@ static void gemalto_pre_sim(struct ofono_modem *modem)
>   	sim = ofono_sim_create(modem, OFONO_VENDOR_CINTERION, "atmodem",
>   						data->app);
>   
> +	/* Register for specific sim status reports */
> +	g_at_chat_register(data->app, "+CIEV:",
> +			gemalto_ciev_notify, FALSE, sim, NULL);
> +
> +	g_at_chat_send(data->app, "AT^SIND=\"simstatus\",1", none_prefix,
> +			NULL, NULL, NULL);
> +
>   	if (sim && data->have_sim == TRUE)
>   		ofono_sim_inserted_notify(sim, TRUE);
>   }
> 

Regards,
-Denis

Re: [PATCH 4/6] gemalto: handle sim is inserted or removed URCs

[Gabriel Lucas]

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

Hi Denis,

On 15/03/2018 18:26, Denis Kenzior wrote:
> Hi Gabriel,
>
> On 03/15/2018 07:49 AM, Gabriel Lucas wrote:
>> From: Mariem Cherif <mariem.cherif@ardia.com.tn>
>>
>> +    g_at_result_iter_init(&iter, result);
>> +
>> +    if (!g_at_result_iter_next(&iter, "+CIEV:"))
>> +        return;
>> +
>
> So generally +CIEV indication is an <index>,<value> syntax.
We receive this kind of signal: +CIEV: simstatus,<status>.
In this code, Mariem is ensuring the index is simstatus.
Do you prefer us to use g_at_result_iter_next to handle the index ?
Or even to use "+CIEV: simstatus" as prefix in the register function ?
>> +    if (!g_at_result_iter_next_unquoted_string(&iter, &ind_str))
>> +        return;
>
> Does Gemalto use some other syntax here?  If so, you might want to 
> document a simple example above.
I've added a comment
>> +        ofono_sim_inserted_notify(sim, FALSE);
>> +    } else if(status == 1) {
>> +        ofono_sim_inserted_notify(sim, TRUE);
>> +    }
>
> Okay, but simpler written as ofono_sim_inserted_notify(sim, status);
The thing is we don't only receive the status 0 and 1. We also have 4 
(ALS3) and
5 (ALS3 + PHS8P). Only the first two should be used.

Best regards,
Gabriel

Re: [PATCH 4/6] gemalto: handle sim is inserted or removed URCs

[Denis Kenzior]

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

Hi Gabriel,

On 03/16/2018 08:28 AM, Gabriel Lucas wrote:
> Hi Denis,
> 
> On 15/03/2018 18:26, Denis Kenzior wrote:
>> Hi Gabriel,
>>
>> On 03/15/2018 07:49 AM, Gabriel Lucas wrote:
>>> From: Mariem Cherif <mariem.cherif@ardia.com.tn>
>>>
>>> +    g_at_result_iter_init(&iter, result);
>>> +
>>> +    if (!g_at_result_iter_next(&iter, "+CIEV:"))
>>> +        return;
>>> +
>>
>> So generally +CIEV indication is an <index>,<value> syntax.
> We receive this kind of signal: +CIEV: simstatus,<status>.
> In this code, Mariem is ensuring the index is simstatus.
> Do you prefer us to use g_at_result_iter_next to handle the index ?
> Or even to use "+CIEV: simstatus" as prefix in the register function ?

Okay, that makes sense now.  Gemalto should really fix their firmware as 
this syntax is not really correct, but nothing we can do about it now. 
I would stick with the current approach.

>>> +    if (!g_at_result_iter_next_unquoted_string(&iter, &ind_str))
>>> +        return;
>>
>> Does Gemalto use some other syntax here?  If so, you might want to 
>> document a simple example above.
> I've added a comment
>>> +        ofono_sim_inserted_notify(sim, FALSE);
>>> +    } else if(status == 1) {
>>> +        ofono_sim_inserted_notify(sim, TRUE);
>>> +    }
>>
>> Okay, but simpler written as ofono_sim_inserted_notify(sim, status);
> The thing is we don't only receive the status 0 and 1. We also have 4 
> (ALS3) and
> 5 (ALS3 + PHS8P). Only the first two should be used.

In that case I would use a switch/case statement, add a case label 
/comment for the other possibilities and what they mean and simply use a 
'break' statement for those.

Regards,
-Denis

[PATCH 4/6] gemalto: handle sim is inserted or removed URCs

[Gabriel Lucas]

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

From: Mariem Cherif <mariem.cherif@ardia.com.tn>

---
 plugins/gemalto.c | 70 +++++++++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 66 insertions(+), 4 deletions(-)

diff --git a/plugins/gemalto.c b/plugins/gemalto.c
index 5573606..13270d6 100644
--- a/plugins/gemalto.c
+++ b/plugins/gemalto.c
@@ -489,20 +489,82 @@ static void gemalto_set_online(struct ofono_modem *modem, ofono_bool_t online,
 	g_free(cbd);
 }
 
+static void sim_ready_cb(gboolean present, gpointer user_data)
+{
+	struct ofono_sim *sim = user_data;
+
+	DBG("sim present: %d", present);
+
+	ofono_sim_inserted_notify(sim, present);
+}
+
+static void gemalto_ciev_notify(GAtResult *result, gpointer user_data)
+{
+	struct ofono_modem *modem = user_data;
+	struct gemalto_data *data = ofono_modem_get_data(modem);
+	struct ofono_sim *sim = data->sim;
+
+	const char *sim_status = "simstatus";
+	const char *ind_str;
+	int status;
+	GAtResultIter iter;
+
+	g_at_result_iter_init(&iter, result);
+
+	/* Example: +CIEV: simstatus,<status> */
+	if (!g_at_result_iter_next(&iter, "+CIEV:"))
+		return;
+
+	if (!g_at_result_iter_next_unquoted_string(&iter, &ind_str))
+		return;
+
+	if (!g_str_equal(sim_status, ind_str))
+		return;
+
+	if (!g_at_result_iter_next_number(&iter, &status))
+		return;
+
+	DBG("sim status %d", status);
+
+	switch (status) {
+	/* SIM is removed from the holder */
+	case 0:
+		ofono_sim_inserted_notify(sim, FALSE);
+		break;
+
+	/* SIM is inserted inside the holder */
+	case 1:
+		/* The SIM won't be ready yet */
+		data->sim_state_query = at_util_sim_state_query_new(data->app,
+					1, 20, sim_ready_cb, sim,
+					NULL);
+		break;
+
+	default:
+		break;
+	}
+}
+
 static void gemalto_pre_sim(struct ofono_modem *modem)
 {
 	struct gemalto_data *data = ofono_modem_get_data(modem);
-	struct ofono_sim *sim;
 
 	DBG("%p", modem);
 
 	ofono_devinfo_create(modem, 0, "atmodem", data->app);
 	ofono_location_reporting_create(modem, 0, "gemaltomodem", data->app);
-	sim = ofono_sim_create(modem, OFONO_VENDOR_CINTERION, "atmodem",
+	data->sim = ofono_sim_create(modem, OFONO_VENDOR_CINTERION, "atmodem",
 						data->app);
 
-	if (sim && data->have_sim == TRUE)
-		ofono_sim_inserted_notify(sim, TRUE);
+	/* Register for specific sim status reports */
+	g_at_chat_register(data->app, "+CIEV:",
+			gemalto_ciev_notify, FALSE, modem, NULL);
+
+	g_at_chat_send(data->app, "AT^SIND=\"simstatus\",1", none_prefix,
+			NULL, NULL, NULL);
+
+	if (data->sim && data->have_sim == TRUE)
+		ofono_sim_inserted_notify(data->sim, TRUE);
 }
 
 static void gemalto_post_sim(struct ofono_modem *modem)
-- 
1.9.1

Re: [PATCH 4/6] gemalto: handle sim is inserted or removed URCs

[Denis Kenzior]

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

Hi Gabriel,

On 03/19/2018 10:57 AM, Gabriel Lucas wrote:
> From: Mariem Cherif <mariem.cherif@ardia.com.tn>
> 
> ---
>   plugins/gemalto.c | 70 +++++++++++++++++++++++++++++++++++++++++++++++++++----
>   1 file changed, 66 insertions(+), 4 deletions(-)
> 
> diff --git a/plugins/gemalto.c b/plugins/gemalto.c
> index 5573606..13270d6 100644
> --- a/plugins/gemalto.c
> +++ b/plugins/gemalto.c
> @@ -489,20 +489,82 @@ static void gemalto_set_online(struct ofono_modem *modem, ofono_bool_t online,
>   	g_free(cbd);
>   }
>   
> +static void sim_ready_cb(gboolean present, gpointer user_data)
> +{
> +	struct ofono_sim *sim = user_data;
> +
> +	DBG("sim present: %d", present);
> +
> +	ofono_sim_inserted_notify(sim, present);

You should free sim_state_query here and zero out the variable, 
otherwise you will leak memory on repeated sim insertion / reinsertion.

> +}
> +
> +static void gemalto_ciev_notify(GAtResult *result, gpointer user_data)

Okay, this part looks good now

>   static void gemalto_pre_sim(struct ofono_modem *modem)
>   {
>   	struct gemalto_data *data = ofono_modem_get_data(modem);
> -	struct ofono_sim *sim;
>   
>   	DBG("%p", modem);
>   
>   	ofono_devinfo_create(modem, 0, "atmodem", data->app);
>   	ofono_location_reporting_create(modem, 0, "gemaltomodem", data->app);
> -	sim = ofono_sim_create(modem, OFONO_VENDOR_CINTERION, "atmodem",
> +	data->sim = ofono_sim_create(modem, OFONO_VENDOR_CINTERION, "atmodem",
>   						data->app);
>   
> -	if (sim && data->have_sim == TRUE)
> -		ofono_sim_inserted_notify(sim, TRUE);
> +	/* Register for specific sim status reports */
> +	g_at_chat_register(data->app, "+CIEV:",
> +			gemalto_ciev_notify, FALSE, modem, NULL);
> +
> +	g_at_chat_send(data->app, "AT^SIND=\"simstatus\",1", none_prefix,
> +			NULL, NULL, NULL);
> +

As a general rule of thumb, there should be no AT commands being sent in 
pre_sim/post_sim/post_online callbacks.  Move this sequence as part of 
the initialization procedure, perhaps after calling 
ofono_modem_set_powered() in sim_state_cb()

> +	if (data->sim && data->have_sim == TRUE)
> +		ofono_sim_inserted_notify(data->sim, TRUE);
>   }
>   
>   static void gemalto_post_sim(struct ofono_modem *modem)
> 

Regards,
-Denis

[PATCH 4/6] gemalto: handle sim is inserted or removed URCs

[Gabriel Lucas]

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

From: Mariem Cherif <mariem.cherif@ardia.com.tn>

---
 plugins/gemalto.c | 78 ++++++++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 74 insertions(+), 4 deletions(-)

diff --git a/plugins/gemalto.c b/plugins/gemalto.c
index 5573606..029e09d 100644
--- a/plugins/gemalto.c
+++ b/plugins/gemalto.c
@@ -57,6 +57,8 @@
 #define GEMALTO_MODEL_PHS8P 	"0053"
 #define GEMALTO_MODEL_ALS3 	"0061"
 
+static void gemalto_ciev_notify(GAtResult *result, gpointer user_data);
+
 static const char *none_prefix[] = { NULL };
 static const char *sctm_prefix[] = { "^SCTM:", NULL };
 static const char *sbv_prefix[] = { "^SBV:", NULL };
@@ -157,6 +159,14 @@ static void sim_state_cb(gboolean present, gpointer user_data)
 
 	data->have_sim = present;
 	ofono_modem_set_powered(modem, TRUE);
+
+	/* Register for specific sim status reports */
+	g_at_chat_register(data->app, "+CIEV:",
+			gemalto_ciev_notify, FALSE, modem, NULL);
+
+	g_at_chat_send(data->app, "AT^SIND=\"simstatus\",1", none_prefix,
+			NULL, NULL, NULL);
+
 }
 
 static void cfun_enable(gboolean ok, GAtResult *result, gpointer user_data)
@@ -489,20 +499,80 @@ static void gemalto_set_online(struct ofono_modem *modem, ofono_bool_t online,
 	g_free(cbd);
 }
 
+static void sim_ready_cb(gboolean present, gpointer user_data)
+{
+	struct ofono_modem *modem = user_data;
+	struct gemalto_data *data = ofono_modem_get_data(modem);
+	struct ofono_sim *sim = data->sim;
+
+	at_util_sim_state_query_free(data->sim_state_query);
+	data->sim_state_query = NULL;
+
+	DBG("sim present: %d", present);
+
+	ofono_sim_inserted_notify(sim, present);
+}
+
+static void gemalto_ciev_notify(GAtResult *result, gpointer user_data)
+{
+	struct ofono_modem *modem = user_data;
+	struct gemalto_data *data = ofono_modem_get_data(modem);
+	struct ofono_sim *sim = data->sim;
+
+	const char *sim_status = "simstatus";
+	const char *ind_str;
+	int status;
+	GAtResultIter iter;
+
+	g_at_result_iter_init(&iter, result);
+
+	/* Example: +CIEV: simstatus,<status> */
+	if (!g_at_result_iter_next(&iter, "+CIEV:"))
+		return;
+
+	if (!g_at_result_iter_next_unquoted_string(&iter, &ind_str))
+		return;
+
+	if (!g_str_equal(sim_status, ind_str))
+		return;
+
+	if (!g_at_result_iter_next_number(&iter, &status))
+		return;
+
+	DBG("sim status %d", status);
+
+	switch (status) {
+	/* SIM is removed from the holder */
+	case 0:
+		ofono_sim_inserted_notify(sim, FALSE);
+		break;
+
+	/* SIM is inserted inside the holder */
+	case 1:
+		/* The SIM won't be ready yet */
+		data->sim_state_query = at_util_sim_state_query_new(data->app,
+					1, 20, sim_ready_cb, modem,
+					NULL);
+		break;
+
+	default:
+		break;
+	}
+}
+
 static void gemalto_pre_sim(struct ofono_modem *modem)
 {
 	struct gemalto_data *data = ofono_modem_get_data(modem);
-	struct ofono_sim *sim;
 
 	DBG("%p", modem);
 
 	ofono_devinfo_create(modem, 0, "atmodem", data->app);
 	ofono_location_reporting_create(modem, 0, "gemaltomodem", data->app);
-	sim = ofono_sim_create(modem, OFONO_VENDOR_CINTERION, "atmodem",
+	data->sim = ofono_sim_create(modem, OFONO_VENDOR_CINTERION, "atmodem",
 						data->app);
 
-	if (sim && data->have_sim == TRUE)
-		ofono_sim_inserted_notify(sim, TRUE);
+	if (data->sim && data->have_sim == TRUE)
+		ofono_sim_inserted_notify(data->sim, TRUE);
 }
 
 static void gemalto_post_sim(struct ofono_modem *modem)
-- 
1.9.1

Re: [PATCH 4/6] gemalto: handle sim is inserted or removed URCs

[Denis Kenzior]

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

Hi Gabriel, Mariem,

On 03/19/2018 11:26 AM, Gabriel Lucas wrote:
> From: Mariem Cherif <mariem.cherif@ardia.com.tn>
> 
> ---
>   plugins/gemalto.c | 78 ++++++++++++++++++++++++++++++++++++++++++++++++++++---
>   1 file changed, 74 insertions(+), 4 deletions(-)
> 
> diff --git a/plugins/gemalto.c b/plugins/gemalto.c
> index 5573606..029e09d 100644
> --- a/plugins/gemalto.c
> +++ b/plugins/gemalto.c
> @@ -57,6 +57,8 @@
>   #define GEMALTO_MODEL_PHS8P 	"0053"
>   #define GEMALTO_MODEL_ALS3 	"0061"
>   
> +static void gemalto_ciev_notify(GAtResult *result, gpointer user_data);
> +

Please note doc/coding-style.txt item M17.  I silently re-structured 
this patch to avoid the need for this forward-declaration.

Patch has been applied, thanks!

Regards,
-Denis

Re: [PATCH 4/6] gemalto: handle sim is inserted or removed URCs

[Gabriel Lucas]

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

Hi Denis,


On 19/03/2018 18:10, Denis Kenzior wrote:
> Hi Gabriel, Mariem,
>
> On 03/19/2018 11:26 AM, Gabriel Lucas wrote:
>> From: Mariem Cherif <mariem.cherif@ardia.com.tn>
>>
>> ---
>>   plugins/gemalto.c | 78 
>> ++++++++++++++++++++++++++++++++++++++++++++++++++++---
>>   1 file changed, 74 insertions(+), 4 deletions(-)
>>
>> diff --git a/plugins/gemalto.c b/plugins/gemalto.c
>> index 5573606..029e09d 100644
>> --- a/plugins/gemalto.c
>> +++ b/plugins/gemalto.c
>> @@ -57,6 +57,8 @@
>>   #define GEMALTO_MODEL_PHS8P     "0053"
>>   #define GEMALTO_MODEL_ALS3     "0061"
>>   +static void gemalto_ciev_notify(GAtResult *result, gpointer 
>> user_data);
>> +
>
> Please note doc/coding-style.txt item M17.  I silently re-structured 
> this patch to avoid the need for this forward-declaration.
>
> Patch has been applied, thanks!
>
> Regards,
> -Denis

Thank you, that's better that way

Best regards,
Gabriel

[PATCH 5/6] sim: give access to the driver

[Gabriel Lucas]

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

---
 include/sim.h | 2 ++
 src/sim.c     | 5 +++++
 2 files changed, 7 insertions(+)

diff --git a/include/sim.h b/include/sim.h
index fad4c0d..cf679db 100644
--- a/include/sim.h
+++ b/include/sim.h
@@ -217,6 +217,8 @@ struct ofono_sim *ofono_sim_create(struct ofono_modem *modem,
 void ofono_sim_register(struct ofono_sim *sim);
 void ofono_sim_remove(struct ofono_sim *sim);
 
+const struct ofono_sim_driver* ofono_sim_get_driver(struct ofono_sim *sim);
+
 void ofono_sim_set_data(struct ofono_sim *sim, void *data);
 void *ofono_sim_get_data(struct ofono_sim *sim);
 
diff --git a/src/sim.c b/src/sim.c
index eb9f56a..f56706f 100644
--- a/src/sim.c
+++ b/src/sim.c
@@ -3305,6 +3305,11 @@ void ofono_sim_remove(struct ofono_sim *sim)
 	__ofono_atom_free(sim->atom);
 }
 
+const struct ofono_sim_driver* ofono_sim_get_driver(struct ofono_sim *sim)
+{
+	return sim->driver;
+}
+
 void ofono_sim_set_data(struct ofono_sim *sim, void *data)
 {
 	sim->driver_data = data;
-- 
1.9.1

Re: [PATCH 5/6] sim: give access to the driver

[Denis Kenzior]

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

Hi Gabriel,

> +const struct ofono_sim_driver* ofono_sim_get_driver(struct ofono_sim *sim);
No, please don't do this.  The driver should just issue the relevant 
command directly.

Regards,
-Denis

[PATCH 6/6] gemalto: fix sim reinsertion issue

[Gabriel Lucas]

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

When the SIM card is reinserted in the holder,
the IRC +CIEV: simstatus,1 is emitted. The problem
is that the SIM isn't ready when it is received.
Hence, Ofono fails on CPIN? command and the modem
cannot be used. This patch make ofono retry the
CPIN? command until it succeeds.
---
 plugins/gemalto.c | 29 ++++++++++++++++++++++++++++-
 1 file changed, 28 insertions(+), 1 deletion(-)

diff --git a/plugins/gemalto.c b/plugins/gemalto.c
index c7fb783..caa5912 100644
--- a/plugins/gemalto.c
+++ b/plugins/gemalto.c
@@ -497,6 +497,33 @@ static void gemalto_set_online(struct ofono_modem *modem, ofono_bool_t online,
 	g_free(cbd);
 }
 
+static void sim_pin_query_cb(const struct ofono_error *error,
+				enum ofono_sim_password_type pin_type,
+				void *data);
+
+static void gemalto_wait_after_plug(struct ofono_sim *sim) {
+	ofono_sim_get_driver(sim)->query_passwd_state(sim, sim_pin_query_cb, sim);
+}
+
+static gboolean cpin_timeout(gpointer sim) {
+	gemalto_wait_after_plug(sim);
+
+	return FALSE;
+}
+
+static void sim_pin_query_cb(const struct ofono_error *error,
+				enum ofono_sim_password_type pin_type,
+				void *data) {
+	struct ofono_sim *sim = data;
+
+	if (error->type != OFONO_ERROR_TYPE_NO_ERROR) {
+		g_timeout_add(50, cpin_timeout, sim);
+	}
+	else {
+		ofono_sim_inserted_notify(sim, TRUE);
+	}
+}
+
 static void gemalto_ciev_notify(GAtResult *result, gpointer user_data)
 {
 	struct ofono_sim *sim = user_data;
@@ -523,7 +550,7 @@ static void gemalto_ciev_notify(GAtResult *result, gpointer user_data)
 	if (status == 0) {
 		ofono_sim_inserted_notify(sim, FALSE);
 	} else if(status == 1) {
-		ofono_sim_inserted_notify(sim, TRUE);
+		gemalto_wait_after_plug(sim);
 	}
 }
 
-- 
1.9.1

Re: [PATCH 6/6] gemalto: fix sim reinsertion issue

[Denis Kenzior]

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

Hi Gabriel,

On 03/15/2018 07:49 AM, Gabriel Lucas wrote:
> When the SIM card is reinserted in the holder,
> the IRC +CIEV: simstatus,1 is emitted. The problem
> is that the SIM isn't ready when it is received.
> Hence, Ofono fails on CPIN? command and the modem
> cannot be used. This patch make ofono retry the
> CPIN? command until it succeeds.
> ---
>   plugins/gemalto.c | 29 ++++++++++++++++++++++++++++-
>   1 file changed, 28 insertions(+), 1 deletion(-)
> 
> diff --git a/plugins/gemalto.c b/plugins/gemalto.c
> index c7fb783..caa5912 100644
> --- a/plugins/gemalto.c
> +++ b/plugins/gemalto.c
> @@ -497,6 +497,33 @@ static void gemalto_set_online(struct ofono_modem *modem, ofono_bool_t online,
>   	g_free(cbd);
>   }
>   
> +static void sim_pin_query_cb(const struct ofono_error *error,
> +				enum ofono_sim_password_type pin_type,
> +				void *data);
> +
> +static void gemalto_wait_after_plug(struct ofono_sim *sim) {
> +	ofono_sim_get_driver(sim)->query_passwd_state(sim, sim_pin_query_cb, sim);
> +}
> +
> +static gboolean cpin_timeout(gpointer sim) {
> +	gemalto_wait_after_plug(sim);
> +
> +	return FALSE;
> +}
> +
> +static void sim_pin_query_cb(const struct ofono_error *error,
> +				enum ofono_sim_password_type pin_type,
> +				void *data) {
> +	struct ofono_sim *sim = data;
> +
> +	if (error->type != OFONO_ERROR_TYPE_NO_ERROR) {
> +		g_timeout_add(50, cpin_timeout, sim);
> +	}
> +	else {
> +		ofono_sim_inserted_notify(sim, TRUE);
> +	}
> +}
> +
>   static void gemalto_ciev_notify(GAtResult *result, gpointer user_data)
>   {
>   	struct ofono_sim *sim = user_data;
> @@ -523,7 +550,7 @@ static void gemalto_ciev_notify(GAtResult *result, gpointer user_data)
>   	if (status == 0) {
>   		ofono_sim_inserted_notify(sim, FALSE);
>   	} else if(status == 1) {
> -		ofono_sim_inserted_notify(sim, TRUE);
> +		gemalto_wait_after_plug(sim);

Why don't you just use at_util_sim_state_query_new here?

>   	}
>   }
>   
> 

Regards,
-Denis

Re: [PATCH 6/6] gemalto: fix sim reinsertion issue

[Gabriel Lucas]

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

Hi Denis,

On 15/03/2018 18:29, Denis Kenzior wrote:
> Hi Gabriel,
>
> Why don't you just use at_util_sim_state_query_new here?

Well, because I didn't know it existed :).
Thanks, I going to use it.

Best regards,
Gabriel

Re: [PATCH 6/6] gemalto: fix sim reinsertion issue

[Gabriel Lucas]

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

Hi Denis,


On 15/03/2018 18:29, Denis Kenzior wrote:
> Hi Gabriel,
>
> Why don't you just use at_util_sim_state_query_new here?
>
> Regards,
> -Denis

This is now done directly in [PATCH 4/6] gemalto: handle sim is inserted 
or removed URCs.

Best regards,
Gabriel

[PATCH 0/6] gemalto's ALS3 and PHS8P support

[Gabriel Lucas]

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

This patch serie bring further support to PHS8P modem
and the same level of support for ALS3 modem. Here are
the lists on enhancements:
- ALS3 is detected by Ofono and uses the gemalto plugin
- SIM removing and insertion is supported
- The optional Technology property on NetworkRegistration
is supported
- We ensure that the modem is ready to work before
initializing it

Some of the enhancements are brought by workarounds because
of AT command interface limitations

Gabriel Lucas (4):
  gemalto: add detection of ALS3 modem
  gemalto: support ALS3 in gemalto's plugin
  sim: give access to the driver
  gemalto: fix sim reinsertion issue

Mariem Cherif (2):
  gemalto: acquire the network technology
  gemalto: handle sim is inserted or removed URCs

 drivers/atmodem/network-registration.c |  45 +++++++++
 include/sim.h                          |   2 +
 plugins/gemalto.c                      | 169 +++++++++++++++++++++++++++++++--
 plugins/udevng.c                       |  18 ++++
 src/sim.c                              |   5 +
 5 files changed, 233 insertions(+), 6 deletions(-)

-- 
1.9.1

[PATCH 2/6] gemalto: support ALS3 in gemalto's plugin

[Gabriel Lucas]

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

Force serial port opening options
Wait for modem to be ready to start
initializing it
Handle LTE
---
 plugins/gemalto.c | 105 ++++++++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 99 insertions(+), 6 deletions(-)

diff --git a/plugins/gemalto.c b/plugins/gemalto.c
index 3739d7b..16ca463 100644
--- a/plugins/gemalto.c
+++ b/plugins/gemalto.c
@@ -53,6 +53,10 @@
 
 #define HARDWARE_MONITOR_INTERFACE OFONO_SERVICE ".cinterion.HardwareMonitor"
 
+// Supported gemalto's modem
+#define GEMALTO_MODEL_PHS8P 	"0053"
+#define GEMALTO_MODEL_ALS3 		"0061"
+
 static const char *none_prefix[] = { NULL };
 static const char *sctm_prefix[] = { "^SCTM:", NULL };
 static const char *sbv_prefix[] = { "^SBV:", NULL };
@@ -70,6 +74,8 @@ struct gemalto_data {
 	gboolean have_sim;
 	struct at_util_sim_state_query *sim_state_query;
 	struct gemalto_hardware_monitor *hm;
+	guint modem_ready_id;
+	guint trial_cmd_id;
 };
 
 static int gemalto_probe(struct ofono_modem *modem)
@@ -107,10 +113,24 @@ static GAtChat *open_device(const char *device)
 	GAtSyntax *syntax;
 	GIOChannel *channel;
 	GAtChat *chat;
+	GHashTable *options;
+
+	options = g_hash_table_new(g_str_hash, g_str_equal);
+	if (options == NULL)
+		return NULL;
+
+	g_hash_table_insert(options, "Baud", "115200");
+	g_hash_table_insert(options, "StopBits", "1");
+	g_hash_table_insert(options, "DataBits", "8");
+	g_hash_table_insert(options, "Parity", "none");
+	g_hash_table_insert(options, "XonXoff", "off");
+	g_hash_table_insert(options, "RtsCts", "on");
+	g_hash_table_insert(options, "Local", "on");
+	g_hash_table_insert(options, "Read", "on");
 
 	DBG("Opening device %s", device);
 
-	channel = g_at_tty_open(device, NULL);
+	channel = g_at_tty_open(device, options);
 	if (channel == NULL)
 		return NULL;
 
@@ -118,6 +138,7 @@ static GAtChat *open_device(const char *device)
 	chat = g_at_chat_new(channel, syntax);
 	g_at_syntax_unref(syntax);
 	g_io_channel_unref(channel);
+	g_hash_table_unref(options);
 
 	if (chat == NULL)
 		return NULL;
@@ -300,29 +321,33 @@ static int gemalto_hardware_monitor_enable(struct ofono_modem *modem)
 	return 0;
 }
 
-static int gemalto_enable(struct ofono_modem *modem)
+static void gemalto_initialize(struct ofono_modem *modem)
 {
 	struct gemalto_data *data = ofono_modem_get_data(modem);
 	const char *app, *mdm;
 
-	DBG("%p", modem);
+	/* Close devices */
+	g_at_chat_unref(data->mdm);
+	g_at_chat_unref(data->app);
+
+	DBG("");
 
 	app = ofono_modem_get_string(modem, "Application");
 	mdm = ofono_modem_get_string(modem, "Modem");
 
 	if (app == NULL || mdm == NULL)
-		return -EINVAL;
+		return;
 
 	/* Open devices */
 	data->app = open_device(app);
 	if (data->app == NULL)
-		return -EINVAL;
+		return;
 
 	data->mdm = open_device(mdm);
 	if (data->mdm == NULL) {
 		g_at_chat_unref(data->app);
 		data->app = NULL;
-		return -EINVAL;
+		return;
 	}
 
 	if (getenv("OFONO_AT_DEBUG")) {
@@ -340,6 +365,67 @@ static int gemalto_enable(struct ofono_modem *modem)
 			cfun_enable, modem, NULL);
 
 	gemalto_hardware_monitor_enable(modem);
+}
+
+static void gemalto_modem_ready(GAtResult *result, gpointer user_data)
+{
+	struct ofono_modem *modem = user_data;
+	struct gemalto_data *data = ofono_modem_get_data(modem);
+
+	DBG("");
+
+	g_at_chat_unregister(data->app, data->modem_ready_id);
+	g_at_chat_cancel(data->app, data->trial_cmd_id);
+
+	gemalto_initialize(modem);
+}
+
+static void gemalto_at_cb(gboolean ok, GAtResult *result, gpointer user_data)
+{
+	struct ofono_modem *modem = user_data;
+	struct gemalto_data *data = ofono_modem_get_data(modem);
+
+	g_at_chat_unregister(data->app, data->modem_ready_id);
+	gemalto_initialize(modem);
+}
+
+static gboolean gemalto_at_timeout(gpointer user_data)
+{
+	struct ofono_modem *modem = user_data;
+	struct gemalto_data *data = ofono_modem_get_data(modem);
+
+	data->trial_cmd_id = g_at_chat_send(data->app, "AT+CFUN?", none_prefix, gemalto_at_cb, modem, NULL);
+
+	return FALSE;
+}
+
+static int gemalto_enable(struct ofono_modem *modem)
+{
+	struct gemalto_data *data = ofono_modem_get_data(modem);
+	const char *app, *mdm;
+
+	DBG("%p", modem);
+
+	app = ofono_modem_get_string(modem, "Application");
+	mdm = ofono_modem_get_string(modem, "Modem");
+
+	if (app == NULL || mdm == NULL)
+		return -EINVAL;
+
+	/* Open devices */
+	data->app = open_device(app);
+	if (data->app == NULL)
+		return -EINVAL;
+
+	data->mdm = open_device(mdm);
+	if (data->mdm == NULL) {
+		g_at_chat_unref(data->app);
+		data->app = NULL;
+		return -EINVAL;
+	}
+
+	data->modem_ready_id = g_at_chat_register(data->app, "^SYSSTART", gemalto_modem_ready, FALSE, modem, NULL);
+	data->trial_cmd_id = g_at_chat_send(data->app, "ATE0 AT", none_prefix, gemalto_at_cb, modem, NULL);
 
 	return -EINPROGRESS;
 }
@@ -449,10 +535,17 @@ static void gemalto_post_sim(struct ofono_modem *modem)
 static void gemalto_post_online(struct ofono_modem *modem)
 {
 	struct gemalto_data *data = ofono_modem_get_data(modem);
+	const char *model;
 
 	DBG("%p", modem);
 
 	ofono_netreg_create(modem, OFONO_VENDOR_CINTERION, "atmodem", data->app);
+
+	model = ofono_modem_get_string(modem, "Model");
+
+	if(!strncmp(model, GEMALTO_MODEL_ALS3, 4)) {
+		ofono_lte_create(modem, "atmodem", data->app);
+	}
 }
 
 static struct ofono_modem_driver gemalto_driver = {
-- 
1.9.1