All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/3] sim800: add support for sim800 modem.
@ 2018-11-07 10:14 Clement Viel
  2018-11-07 10:14 ` [PATCH 2/3] sim800: add documentation Clement Viel
                   ` (2 more replies)
  0 siblings, 3 replies; 13+ messages in thread
From: Clement Viel @ 2018-11-07 10:14 UTC (permalink / raw)
  To: ofono

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

---
 plugins/sim900.c | 108 +++++++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 98 insertions(+), 10 deletions(-)

diff --git a/plugins/sim900.c b/plugins/sim900.c
index a7728cd..c12576d 100644
--- a/plugins/sim900.c
+++ b/plugins/sim900.c
@@ -24,6 +24,7 @@
 #endif
 
 #include <errno.h>
+#include <string.h>
 #include <stdlib.h>
 #include <glib.h>
 #include <gatchat.h>
@@ -60,13 +61,73 @@ static char *dlc_prefixes[NUM_DLC] = { "Voice: ", "Net: ", "SMS: ",
 
 static const char *none_prefix[] = { NULL };
 
+typedef enum type {
+	SIM800,
+	SIM900,
+	SIM_UNKNOWN,
+} type_t;
+
 struct sim900_data {
 	GIOChannel *device;
 	GAtMux *mux;
 	GAtChat * dlcs[NUM_DLC];
 	guint frame_size;
+	type_t modem_type;
 };
 
+static void set_modem_type (const char *type, struct ofono_modem *modem)
+{
+	struct sim900_data *data = ofono_modem_get_data(modem);
+
+	if (strcmp(type, "sim800") == 0)
+		data->modem_type = SIM800;
+	else if (strcmp(type, "sim900") == 0)
+		data->modem_type = SIM900;
+	else
+		data->modem_type = SIM_UNKNOWN;
+	DBG("modem type is %d",data->modem_type);
+}
+
+static void mux_ready_notify(GAtResult *result, gpointer user_data)
+{
+	struct ofono_modem *modem = user_data;
+	struct sim900_data *data = ofono_modem_get_data(modem);
+	struct ofono_gprs *gprs = NULL;
+	struct ofono_gprs_context *gc;
+
+	ofono_sms_create(modem, OFONO_VENDOR_SIMCOM, "atmodem",
+				data->dlcs[SMS_DLC]);
+
+	gprs = ofono_gprs_create(modem, 0, "atmodem", data->dlcs[GPRS_DLC]);
+	if (gprs == NULL)
+		return;
+
+	gc = ofono_gprs_context_create(modem, OFONO_VENDOR_SIMCOM,
+					"atmodem", data->dlcs[GPRS_DLC]);
+	if (gc)
+		ofono_gprs_add_context(gprs, gc);
+
+}
+
+static void check_model(gboolean ok, GAtResult *result, gpointer user_data)
+{
+	struct ofono_modem *modem = user_data;
+	GAtResultIter iter;
+	char const *model;
+
+	g_at_result_iter_init(&iter, result);
+
+	while (g_at_result_iter_next(&iter, NULL)) {
+		if (!g_at_result_iter_next_unquoted_string(&iter, &model))
+			continue;
+
+		if (strstr(model, "SIM800"))
+			set_modem_type("sim800", modem);
+		else if (strstr(model, "SIM900"))
+			set_modem_type("sim900", modem);
+	}
+}
+
 static int sim900_probe(struct ofono_modem *modem)
 {
 	struct sim900_data *data;
@@ -78,7 +139,6 @@ static int sim900_probe(struct ofono_modem *modem)
 		return -ENOMEM;
 
 	ofono_modem_set_data(modem, data);
-
 	return 0;
 }
 
@@ -111,6 +171,7 @@ static GAtChat *open_device(struct ofono_modem *modem,
 	GHashTable *options;
 
 	device = ofono_modem_get_string(modem, key);
+
 	if (device == NULL)
 		return NULL;
 
@@ -232,6 +293,11 @@ static void setup_internal_mux(struct ofono_modem *modem)
 			goto error;
 		}
 	}
+	if (data->modem_type == SIM800) {
+		for (i = 0; i<NUM_DLC; i++) {
+			g_at_chat_register(data->dlcs[i], "SMS Ready", mux_ready_notify, FALSE, modem, NULL);
+		}
+	}
 
 	ofono_modem_set_powered(modem, TRUE);
 
@@ -294,6 +360,7 @@ static int sim900_enable(struct ofono_modem *modem)
 		return -EINVAL;
 
 	g_at_chat_send(data->dlcs[SETUP_DLC], "ATE0", NULL, NULL, NULL, NULL);
+	g_at_chat_send(data->dlcs[SETUP_DLC], "AT+CGMM", NULL,check_model, modem, NULL);
 
 	/* For obtain correct sms service number */
 	g_at_chat_send(data->dlcs[SETUP_DLC], "AT+CSCS=\"GSM\"", NULL,
@@ -353,18 +420,20 @@ static void sim900_post_sim(struct ofono_modem *modem)
 
 	DBG("%p", modem);
 
-	ofono_phonebook_create(modem, 0, "atmodem", data->dlcs[VOICE_DLC]);
-	ofono_sms_create(modem, OFONO_VENDOR_SIMCOM, "atmodem",
+	if (data->modem_type != SIM800) {
+		ofono_phonebook_create(modem, 0, "atmodem", data->dlcs[VOICE_DLC]);
+		ofono_sms_create(modem, OFONO_VENDOR_SIMCOM, "atmodem",
 						data->dlcs[SMS_DLC]);
 
-	gprs = ofono_gprs_create(modem, 0, "atmodem", data->dlcs[GPRS_DLC]);
-	if (gprs == NULL)
-		return;
+		gprs = ofono_gprs_create(modem, 0, "atmodem", data->dlcs[GPRS_DLC]);
+		if (gprs == NULL)
+			return;
 
-	gc = ofono_gprs_context_create(modem, OFONO_VENDOR_SIMCOM_SIM900,
+		gc = ofono_gprs_context_create(modem, OFONO_VENDOR_SIMCOM_SIM900,
 					"atmodem", data->dlcs[GPRS_DLC]);
-	if (gc)
-		ofono_gprs_add_context(gprs, gc);
+		if (gc)
+			ofono_gprs_add_context(gprs, gc);
+	}
 }
 
 static void sim900_post_online(struct ofono_modem *modem)
@@ -391,9 +460,28 @@ static struct ofono_modem_driver sim900_driver = {
 	.post_online	= sim900_post_online,
 };
 
+
+static struct ofono_modem_driver sim800_driver = {
+	.name		= "sim800",
+	.probe		= sim900_probe,
+	.remove		= sim900_remove,
+	.enable		= sim900_enable,
+	.disable	= sim900_disable,
+	.pre_sim	= sim900_pre_sim,
+	.post_sim	= sim900_post_sim,
+	.post_online	= sim900_post_online,
+};
+
 static int sim900_init(void)
 {
-	return ofono_modem_driver_register(&sim900_driver);
+	int ret = 0;
+	ret = ofono_modem_driver_register(&sim800_driver);
+	if (!ret)
+		return ret;
+	ret = ofono_modem_driver_register(&sim900_driver);
+	if (!ret)
+		return ret;
+	return ret;
 }
 
 static void sim900_exit(void)
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 2/3] sim800: add documentation.
  2018-11-07 10:14 [PATCH 1/3] sim800: add support for sim800 modem Clement Viel
@ 2018-11-07 10:14 ` Clement Viel
  2018-11-07 13:50   ` Jonas Bonn
  2018-11-07 10:14 ` [PATCH 3/3] sim800: add udev detection Clement Viel
  2018-11-07 13:47 ` [PATCH 1/3] sim800: add support for sim800 modem Jonas Bonn
  2 siblings, 1 reply; 13+ messages in thread
From: Clement Viel @ 2018-11-07 10:14 UTC (permalink / raw)
  To: ofono

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

---
 doc/sim800-modem.txt | 7 +++++++
 1 file changed, 7 insertions(+)
 create mode 100644 doc/sim800-modem.txt

diff --git a/doc/sim800-modem.txt b/doc/sim800-modem.txt
new file mode 100644
index 0000000..a299e97
--- /dev/null
+++ b/doc/sim800-modem.txt
@@ -0,0 +1,7 @@
+SIM800 modem usage
+===================
+
+To enable SIM800 module support you need to put the following
+udev rule into appropriate file in /{etc,lib}/udev/rules.d:
+
+KERNEL=="ttyS0", ENV{OFONO_DRIVER}="sim800"
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 3/3] sim800: add udev detection.
  2018-11-07 10:14 [PATCH 1/3] sim800: add support for sim800 modem Clement Viel
  2018-11-07 10:14 ` [PATCH 2/3] sim800: add documentation Clement Viel
@ 2018-11-07 10:14 ` Clement Viel
  2018-11-07 13:30   ` Jonas Bonn
  2018-11-07 13:47 ` [PATCH 1/3] sim800: add support for sim800 modem Jonas Bonn
  2 siblings, 1 reply; 13+ messages in thread
From: Clement Viel @ 2018-11-07 10:14 UTC (permalink / raw)
  To: ofono

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

---
 plugins/udevng.c | 92 +++++++++++++++++++++++++++++++++++++++++---------------
 1 file changed, 68 insertions(+), 24 deletions(-)

diff --git a/plugins/udevng.c b/plugins/udevng.c
index ff6e1fc..ee6f9e8 100644
--- a/plugins/udevng.c
+++ b/plugins/udevng.c
@@ -711,8 +711,56 @@ static gboolean setup_telitqmi(struct modem_info *modem)
 	return TRUE;
 }
 
-/* TODO: Not used as we have no simcom driver */
-static gboolean setup_simcom(struct modem_info *modem)
+
+static gboolean setup_sim800(struct modem_info *modem)
+{
+	const char *mdm = NULL, *aux = NULL, *gps = NULL, *diag = NULL;
+	GSList *list;
+
+	DBG("%s", modem->syspath);
+
+	for (list = modem->devices; list; list = list->next) {
+		struct device_info *info = list->data;
+
+		DBG("%s %s %s %s", info->devnode, info->interface,
+						info->number, info->label);
+
+		if (g_strcmp0(info->label, "aux") == 0) {
+			DBG("setting aux as info->devnode");
+			aux = info->devnode;
+			if (mdm != NULL)
+				break;
+		} else if (g_strcmp0(info->label, "modem") == 0) {
+			mdm = info->devnode;
+			if (aux != NULL)
+				break;
+		} else if (g_strcmp0(info->interface, "255/0/0") == 0) {
+			if (g_strcmp0(info->number, "00") == 0)
+				mdm = info->devnode;
+			else if (g_strcmp0(info->number, "01") == 0)
+				gps = info->devnode;
+			else if (g_strcmp0(info->number, "02") == 0)
+				aux = info->devnode;
+			else if (g_strcmp0(info->number, "03") == 0)
+				mdm = info->devnode;
+		}
+	}
+	DBG("modem=%s aux=%s gps=%s diag=%s", mdm, aux, gps, diag);
+
+	if (mdm == NULL) {
+		DBG("modem did not set up");
+		return FALSE;
+	}
+
+	ofono_modem_set_string(modem->modem, "Device", mdm);
+	ofono_modem_set_string(modem->modem, "Modem", mdm);
+
+	return TRUE;
+}
+
+
+
+static gboolean setup_sim900(struct modem_info *modem)
 {
 	const char *mdm = NULL, *aux = NULL, *gps = NULL, *diag = NULL;
 	GSList *list;
@@ -962,6 +1010,8 @@ static gboolean setup_mbim(struct modem_info *modem)
 	ofono_modem_set_string(modem->modem, "Device", ctl);
 	ofono_modem_set_string(modem->modem, "NetworkInterface", net);
 	ofono_modem_set_string(modem->modem, "DescriptorFile", descriptors);
+	ofono_modem_set_string(modem->modem, "Vendor", modem->vendor);
+	ofono_modem_set_string(modem->modem, "Model", modem->model);
 
 	return TRUE;
 }
@@ -1193,22 +1243,12 @@ static gboolean setup_xmm7xxx(struct modem_info *modem)
 				info->interface, info->number, info->label,
 				info->sysattr, info->subsystem);
 
-		if (g_strcmp0(modem->model,"095a") == 0) {
-			if (g_strcmp0(info->subsystem, "tty") == 0) {
-				if (g_strcmp0(info->number, "00") == 0)
-					mdm = info->devnode;
-			} else if (g_strcmp0(info->subsystem, "net") == 0) {
-				if (g_strcmp0(info->number, "06") == 0)
-					net = info->devnode;
-			}
-		} else {
-			if (g_strcmp0(info->subsystem, "tty") == 0) {
-				if (g_strcmp0(info->number, "02") == 0)
-					mdm = info->devnode;
-			} else if (g_strcmp0(info->subsystem, "net") == 0) {
-				if (g_strcmp0(info->number, "00") == 0)
-					net = info->devnode;
-			}
+		if (g_strcmp0(info->subsystem, "tty") == 0) {
+			if (g_strcmp0(info->number, "02") == 0)
+				mdm = info->devnode;
+		} else if (g_strcmp0(info->subsystem, "net") == 0) {
+			if (g_strcmp0(info->number, "00") == 0)
+				net = info->devnode;
 		}
 	}
 
@@ -1290,7 +1330,8 @@ static struct {
 	{ "nokia",	setup_nokia	},
 	{ "telit",	setup_telit,	"device/interface"	},
 	{ "telitqmi",	setup_telitqmi	},
-	{ "simcom",	setup_simcom	},
+	{ "sim800",	setup_sim800	},
+	{ "sim900",	setup_sim900	},
 	{ "sim7100",	setup_sim7100	},
 	{ "zte",	setup_zte	},
 	{ "icera",	setup_icera	},
@@ -1308,7 +1349,9 @@ static struct {
 	{ "calypso",	setup_serial_modem	},
 	{ "cinterion",	setup_serial_modem	},
 	{ "nokiacdma",	setup_serial_modem	},
+	{ "sim800",	setup_serial_modem	},
 	{ "sim900",	setup_serial_modem	},
+	{ "sim800",	setup_serial_modem	},
 	{ "wavecom",	setup_wavecom		},
 	{ "tc65",	setup_tc65		},
 	{ "ehs6",	setup_ehs6		},
@@ -1471,7 +1514,7 @@ static void add_serial_device(struct udev_device *dev)
 	const char *subsystem;
 	struct udev_device* mdev;
 	const char* driver;
-
+	DBG("adding %s interface", udev_device_get_devpath(dev));
 	mdev = get_serial_modem_device(dev);
 	if (!mdev) {
 		DBG("Device is missing required OFONO_DRIVER property");
@@ -1663,7 +1706,8 @@ static struct {
 	{ "alcatel",	"option",	"1bbb", "0017"	},
 	{ "novatel",	"option",	"1410"		},
 	{ "zte",	"option",	"19d2"		},
-	{ "simcom",	"option",	"05c6", "9000"	},
+	{ "sim800",	"option",	"05c6", "9000"	},
+	{ "sim900",	"option",	"05c6", "9000"	},
 	{ "sim7100",	"option",	"1e0e", "9001"	},
 	{ "telit",	"usbserial",	"1bc7"		},
 	{ "telit",	"option",	"1bc7"		},
@@ -1693,7 +1737,6 @@ static struct {
 	{ "xmm7xxx",	"cdc_ncm",	"8087"		},
 	{ }
 };
-
 static void check_usb_device(struct udev_device *device)
 {
 	struct udev_device *usb_device;
@@ -1722,9 +1765,10 @@ static void check_usb_device(struct udev_device *device)
 			udev_device_get_parent_with_subsystem_devtype(
 				device, "usb", "usb_interface");
 
-		if (usb_interface)
+		if (usb_interface) {
 			driver = udev_device_get_property_value(
 					usb_interface, "OFONO_DRIVER");
+		}
 	}
 
 	if (driver == NULL) {
@@ -1801,7 +1845,7 @@ static gboolean create_modem(gpointer key, gpointer value, gpointer user_data)
 	struct modem_info *modem = value;
 	const char *syspath = key;
 	unsigned int i;
-
+	DBG("");
 	if (modem->modem != NULL)
 		return FALSE;
 
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH 3/3] sim800: add udev detection.
  2018-11-07 10:14 ` [PATCH 3/3] sim800: add udev detection Clement Viel
@ 2018-11-07 13:30   ` Jonas Bonn
  2018-11-07 13:58     ` Clement Viel
  0 siblings, 1 reply; 13+ messages in thread
From: Jonas Bonn @ 2018-11-07 13:30 UTC (permalink / raw)
  To: ofono

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

Hi Clement,

On 07/11/2018 11:14, Clement Viel wrote:
> ---
>   plugins/udevng.c | 92 +++++++++++++++++++++++++++++++++++++++++---------------
>   1 file changed, 68 insertions(+), 24 deletions(-)
> 
> diff --git a/plugins/udevng.c b/plugins/udevng.c
> index ff6e1fc..ee6f9e8 100644
> --- a/plugins/udevng.c
> +++ b/plugins/udevng.c
> @@ -711,8 +711,56 @@ static gboolean setup_telitqmi(struct modem_info *modem)
>   	return TRUE;
>   }
>   
> -/* TODO: Not used as we have no simcom driver */
> -static gboolean setup_simcom(struct modem_info *modem)
> +
> +static gboolean setup_sim800(struct modem_info *modem)
> +{
> +	const char *mdm = NULL, *aux = NULL, *gps = NULL, *diag = NULL;
> +	GSList *list;
> +
> +	DBG("%s", modem->syspath);
> +
> +	for (list = modem->devices; list; list = list->next) {
> +		struct device_info *info = list->data;
> +
> +		DBG("%s %s %s %s", info->devnode, info->interface,
> +						info->number, info->label);
> +
> +		if (g_strcmp0(info->label, "aux") == 0) {
> +			DBG("setting aux as info->devnode");
> +			aux = info->devnode;
> +			if (mdm != NULL)
> +				break;
> +		} else if (g_strcmp0(info->label, "modem") == 0) {
> +			mdm = info->devnode;
> +			if (aux != NULL)
> +				break;
> +		} else if (g_strcmp0(info->interface, "255/0/0") == 0) {
> +			if (g_strcmp0(info->number, "00") == 0)
> +				mdm = info->devnode;
> +			else if (g_strcmp0(info->number, "01") == 0)
> +				gps = info->devnode;
> +			else if (g_strcmp0(info->number, "02") == 0)
> +				aux = info->devnode;
> +			else if (g_strcmp0(info->number, "03") == 0)
> +				mdm = info->devnode;
> +		}
> +	}
> +	DBG("modem=%s aux=%s gps=%s diag=%s", mdm, aux, gps, diag);
> +

gps is unused.


> +	if (mdm == NULL) {
> +		DBG("modem did not set up");
> +		return FALSE;
> +	}
> +
> +	ofono_modem_set_string(modem->modem, "Device", mdm);

You probably mean 'aux' here.

> +	ofono_modem_set_string(modem->modem, "Modem", mdm);
> +
> +	return TRUE;
> +}
> +
> +
> +
> +static gboolean setup_sim900(struct modem_info *modem)
>   {
>   	const char *mdm = NULL, *aux = NULL, *gps = NULL, *diag = NULL;
>   	GSList *list;
> @@ -962,6 +1010,8 @@ static gboolean setup_mbim(struct modem_info *modem)
>   	ofono_modem_set_string(modem->modem, "Device", ctl);
>   	ofono_modem_set_string(modem->modem, "NetworkInterface", net);
>   	ofono_modem_set_string(modem->modem, "DescriptorFile", descriptors);
> +	ofono_modem_set_string(modem->modem, "Vendor", modem->vendor);
> +	ofono_modem_set_string(modem->modem, "Model", modem->model);

I don't see that these strings are being used anywhere in the modem driver.

>   
>   	return TRUE;
>   }
> @@ -1193,22 +1243,12 @@ static gboolean setup_xmm7xxx(struct modem_info *modem)
>   				info->interface, info->number, info->label,
>   				info->sysattr, info->subsystem);
>   
> -		if (g_strcmp0(modem->model,"095a") == 0) {
> -			if (g_strcmp0(info->subsystem, "tty") == 0) {
> -				if (g_strcmp0(info->number, "00") == 0)
> -					mdm = info->devnode;
> -			} else if (g_strcmp0(info->subsystem, "net") == 0) {
> -				if (g_strcmp0(info->number, "06") == 0)
> -					net = info->devnode;
> -			}
> -		} else {
> -			if (g_strcmp0(info->subsystem, "tty") == 0) {
> -				if (g_strcmp0(info->number, "02") == 0)
> -					mdm = info->devnode;
> -			} else if (g_strcmp0(info->subsystem, "net") == 0) {
> -				if (g_strcmp0(info->number, "00") == 0)
> -					net = info->devnode;
> -			}
> +		if (g_strcmp0(info->subsystem, "tty") == 0) {
> +			if (g_strcmp0(info->number, "02") == 0)
> +				mdm = info->devnode;
> +		} else if (g_strcmp0(info->subsystem, "net") == 0) {
> +			if (g_strcmp0(info->number, "00") == 0)
> +				net = info->devnode;
>   		}
>   	}
>  

This change doesn't look like it belongs to this patch.


> @@ -1290,7 +1330,8 @@ static struct {
>   	{ "nokia",	setup_nokia	},
>   	{ "telit",	setup_telit,	"device/interface"	},
>   	{ "telitqmi",	setup_telitqmi	},
> -	{ "simcom",	setup_simcom	},
> +	{ "sim800",	setup_sim800	},
> +	{ "sim900",	setup_sim900	},

Is the sim900 not a serial modem?  These setup routines are really only 
for USB devices.

>   	{ "sim7100",	setup_sim7100	},
>   	{ "zte",	setup_zte	},
>   	{ "icera",	setup_icera	},
> @@ -1308,7 +1349,9 @@ static struct {
>   	{ "calypso",	setup_serial_modem	},
>   	{ "cinterion",	setup_serial_modem	},
>   	{ "nokiacdma",	setup_serial_modem	},
> +	{ "sim800",	setup_serial_modem	},
>   	{ "sim900",	setup_serial_modem	},
> +	{ "sim800",	setup_serial_modem	},

You've added the same line twice.  Furthermore, the sim800 is a USB 
modem (???) so it should not be set up in this way.

>   	{ "wavecom",	setup_wavecom		},
>   	{ "tc65",	setup_tc65		},
>   	{ "ehs6",	setup_ehs6		},
> @@ -1471,7 +1514,7 @@ static void add_serial_device(struct udev_device *dev)
>   	const char *subsystem;
>   	struct udev_device* mdev;
>   	const char* driver;
> -
> +	DBG("adding %s interface", udev_device_get_devpath(dev));
>   	mdev = get_serial_modem_device(dev);
>   	if (!mdev) {
>   		DBG("Device is missing required OFONO_DRIVER property");
> @@ -1663,7 +1706,8 @@ static struct {
>   	{ "alcatel",	"option",	"1bbb", "0017"	},
>   	{ "novatel",	"option",	"1410"		},
>   	{ "zte",	"option",	"19d2"		},
> -	{ "simcom",	"option",	"05c6", "9000"	},
> +	{ "sim800",	"option",	"05c6", "9000"	}
> +	{ "sim900",	"option",	"05c6", "9000"	},


The sim900 is a serial modem... there is no vendor id nor product id.


>   	{ "sim7100",	"option",	"1e0e", "9001"	},
>   	{ "telit",	"usbserial",	"1bc7"		},
>   	{ "telit",	"option",	"1bc7"		},
> @@ -1693,7 +1737,6 @@ static struct {
>   	{ "xmm7xxx",	"cdc_ncm",	"8087"		},
>   	{ }
>   };
> -
>   static void check_usb_device(struct udev_device *device)
>   {
>   	struct udev_device *usb_device;
> @@ -1722,9 +1765,10 @@ static void check_usb_device(struct udev_device *device)
>   			udev_device_get_parent_with_subsystem_devtype(
>   				device, "usb", "usb_interface");
>   
> -		if (usb_interface)
> +		if (usb_interface) {
>   			driver = udev_device_get_property_value(
>   					usb_interface, "OFONO_DRIVER");
> +		}
>   	}

No.

>   
>   	if (driver == NULL) {
> @@ -1801,7 +1845,7 @@ static gboolean create_modem(gpointer key, gpointer value, gpointer user_data)
>   	struct modem_info *modem = value;
>   	const char *syspath = key;
>   	unsigned int i;
> -
> +	DBG("");
>   	if (modem->modem != NULL)
>   		return FALSE;
>   

Doesn't belong to this patch, if at all.

> 

/Jonas

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/3] sim800: add support for sim800 modem.
  2018-11-07 10:14 [PATCH 1/3] sim800: add support for sim800 modem Clement Viel
  2018-11-07 10:14 ` [PATCH 2/3] sim800: add documentation Clement Viel
  2018-11-07 10:14 ` [PATCH 3/3] sim800: add udev detection Clement Viel
@ 2018-11-07 13:47 ` Jonas Bonn
  2 siblings, 0 replies; 13+ messages in thread
From: Jonas Bonn @ 2018-11-07 13:47 UTC (permalink / raw)
  To: ofono

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

Hi Clement,

On 07/11/2018 11:14, Clement Viel wrote:
> ---
>   plugins/sim900.c | 108 +++++++++++++++++++++++++++++++++++++++++++++++++------
>   1 file changed, 98 insertions(+), 10 deletions(-)
> 
> diff --git a/plugins/sim900.c b/plugins/sim900.c
> index a7728cd..c12576d 100644
> --- a/plugins/sim900.c
> +++ b/plugins/sim900.c
> @@ -24,6 +24,7 @@
>   #endif
>   
>   #include <errno.h>
> +#include <string.h>
>   #include <stdlib.h>
>   #include <glib.h>
>   #include <gatchat.h>
> @@ -60,13 +61,73 @@ static char *dlc_prefixes[NUM_DLC] = { "Voice: ", "Net: ", "SMS: ",
>   
>   static const char *none_prefix[] = { NULL };
>   
> +typedef enum type {
> +	SIM800,
> +	SIM900,
> +	SIM_UNKNOWN,
> +} type_t;
> +

type_t is not a great name... furthermore, I don't think ofono wants 
typedef'd enums.  Just use:  enum simcom_model {...} or similar.

>   struct sim900_data {
>   	GIOChannel *device;
>   	GAtMux *mux;
>   	GAtChat * dlcs[NUM_DLC];
>   	guint frame_size;
> +	type_t modem_type;
>   };
>   
> +static void set_modem_type (const char *type, struct ofono_modem *modem)
> +{
> +	struct sim900_data *data = ofono_modem_get_data(modem);
> +
> +	if (strcmp(type, "sim800") == 0)
> +		data->modem_type = SIM800;
> +	else if (strcmp(type, "sim900") == 0)
> +		data->modem_type = SIM900;
> +	else
> +		data->modem_type = SIM_UNKNOWN;
> +	DBG("modem type is %d",data->modem_type);
> +}
> +

This function is unnecessary... see below.

> +static void mux_ready_notify(GAtResult *result, gpointer user_data)
> +{
> +	struct ofono_modem *modem = user_data;
> +	struct sim900_data *data = ofono_modem_get_data(modem);
> +	struct ofono_gprs *gprs = NULL;
> +	struct ofono_gprs_context *gc;
> +
> +	ofono_sms_create(modem, OFONO_VENDOR_SIMCOM, "atmodem",
> +				data->dlcs[SMS_DLC]);
> +
> +	gprs = ofono_gprs_create(modem, 0, "atmodem", data->dlcs[GPRS_DLC]);
> +	if (gprs == NULL)
> +		return;
> +
> +	gc = ofono_gprs_context_create(modem, OFONO_VENDOR_SIMCOM,
> +					"atmodem", data->dlcs[GPRS_DLC]);
> +	if (gc)
> +		ofono_gprs_add_context(gprs, gc);
> +
> +}
> +
> +static void check_model(gboolean ok, GAtResult *result, gpointer user_data)
> +{
> +	struct ofono_modem *modem = user_data;
> +	GAtResultIter iter;
> +	char const *model;
> +
> +	g_at_result_iter_init(&iter, result);
> +
> +	while (g_at_result_iter_next(&iter, NULL)) {
> +		if (!g_at_result_iter_next_unquoted_string(&iter, &model))
> +			continue;
> +
> +		if (strstr(model, "SIM800"))
> +			set_modem_type("sim800", modem);
> +		else if (strstr(model, "SIM900"))
> +			set_modem_type("sim900", modem);

Just set the type directly here... no need to pass an arbitrary string 
through a secondary function to accomplish that.

Furthermore, the result is exactly "SIM800" or "SIM900", right?  No need 
for strstr, use strcmp.

> +	}
> +}
> +
>   static int sim900_probe(struct ofono_modem *modem)
>   {
>   	struct sim900_data *data;
> @@ -78,7 +139,6 @@ static int sim900_probe(struct ofono_modem *modem)
>   		return -ENOMEM;
>   
>   	ofono_modem_set_data(modem, data);
> -
>   	return 0;
>   }
>   
> @@ -111,6 +171,7 @@ static GAtChat *open_device(struct ofono_modem *modem,
>   	GHashTable *options;
>   
>   	device = ofono_modem_get_string(modem, key);
> +
>   	if (device == NULL)
>   		return NULL;
>   
> @@ -232,6 +293,11 @@ static void setup_internal_mux(struct ofono_modem *modem)
>   			goto error;
>   		}
>   	}
> +	if (data->modem_type == SIM800) {
> +		for (i = 0; i<NUM_DLC; i++) {
> +			g_at_chat_register(data->dlcs[i], "SMS Ready", mux_ready_notify, FALSE, modem, NULL);
> +		}
> +	}
>   
>   	ofono_modem_set_powered(modem, TRUE);
>   
> @@ -294,6 +360,7 @@ static int sim900_enable(struct ofono_modem *modem)
>   		return -EINVAL;
>   
>   	g_at_chat_send(data->dlcs[SETUP_DLC], "ATE0", NULL, NULL, NULL, NULL);
> +	g_at_chat_send(data->dlcs[SETUP_DLC], "AT+CGMM", NULL,check_model, modem, NULL);
>   
>   	/* For obtain correct sms service number */
>   	g_at_chat_send(data->dlcs[SETUP_DLC], "AT+CSCS=\"GSM\"", NULL,
> @@ -353,18 +420,20 @@ static void sim900_post_sim(struct ofono_modem *modem)
>   
>   	DBG("%p", modem);
>   
> -	ofono_phonebook_create(modem, 0, "atmodem", data->dlcs[VOICE_DLC]);
> -	ofono_sms_create(modem, OFONO_VENDOR_SIMCOM, "atmodem",
> +	if (data->modem_type != SIM800) {

How about "== SIM900" because that's what this is for.

> +		ofono_phonebook_create(modem, 0, "atmodem", data->dlcs[VOICE_DLC]);
> +		ofono_sms_create(modem, OFONO_VENDOR_SIMCOM, "atmodem",
>   						data->dlcs[SMS_DLC]);
>   
> -	gprs = ofono_gprs_create(modem, 0, "atmodem", data->dlcs[GPRS_DLC]);
> -	if (gprs == NULL)
> -		return;
> +		gprs = ofono_gprs_create(modem, 0, "atmodem", data->dlcs[GPRS_DLC]);
> +		if (gprs == NULL)
> +			return;
>   
> -	gc = ofono_gprs_context_create(modem, OFONO_VENDOR_SIMCOM_SIM900,
> +		gc = ofono_gprs_context_create(modem, OFONO_VENDOR_SIMCOM_SIM900,
>   					"atmodem", data->dlcs[GPRS_DLC]);
> -	if (gc)
> -		ofono_gprs_add_context(gprs, gc);
> +		if (gc)
> +			ofono_gprs_add_context(gprs, gc);
> +	}
>   }
>   
>   static void sim900_post_online(struct ofono_modem *modem)
> @@ -391,9 +460,28 @@ static struct ofono_modem_driver sim900_driver = {
>   	.post_online	= sim900_post_online,
>   };
>   
> +
> +static struct ofono_modem_driver sim800_driver = {
> +	.name		= "sim800",
> +	.probe		= sim900_probe,
> +	.remove		= sim900_remove,
> +	.enable		= sim900_enable,
> +	.disable	= sim900_disable,
> +	.pre_sim	= sim900_pre_sim,
> +	.post_sim	= sim900_post_sim,
> +	.post_online	= sim900_post_online,
> +};

This is identical to the sim900_driver.

Your driver is already taking into account the modem model and adjusting 
functionality accordingly.  You don't need to specify a new driver for 
the sim800 where the only thing that changes is the name.

> +
>   static int sim900_init(void)
>   {
> -	return ofono_modem_driver_register(&sim900_driver);
> +	int ret = 0;
> +	ret = ofono_modem_driver_register(&sim800_driver);
> +	if (!ret)
> +		return ret;
> +	ret = ofono_modem_driver_register(&sim900_driver);
> +	if (!ret)
> +		return ret;
> +	return ret;
>   }
>   
>   static void sim900_exit(void)
> 

/Jonas

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/3] sim800: add documentation.
  2018-11-07 10:14 ` [PATCH 2/3] sim800: add documentation Clement Viel
@ 2018-11-07 13:50   ` Jonas Bonn
  2018-11-07 13:53     ` =?unknown-8bit?q?Pi=C4=8Dugins?= Arsenijs
  0 siblings, 1 reply; 13+ messages in thread
From: Jonas Bonn @ 2018-11-07 13:50 UTC (permalink / raw)
  To: ofono

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

Hi Clement,

On 07/11/2018 11:14, Clement Viel wrote:
> ---
>   doc/sim800-modem.txt | 7 +++++++
>   1 file changed, 7 insertions(+)
>   create mode 100644 doc/sim800-modem.txt
> 
> diff --git a/doc/sim800-modem.txt b/doc/sim800-modem.txt
> new file mode 100644
> index 0000000..a299e97
> --- /dev/null
> +++ b/doc/sim800-modem.txt
> @@ -0,0 +1,7 @@
> +SIM800 modem usage
> +===================
> +
> +To enable SIM800 module support you need to put the following
> +udev rule into appropriate file in /{etc,lib}/udev/rules.d:
> +
> +KERNEL=="ttyS0", ENV{OFONO_DRIVER}="sim800"
> 

Confused by this.  Isn't the sim900 the serial modem and sim800 the USB 
model?  The above rule is for a strictly "serial" modem.

/Jonas

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/3] sim800: add documentation.
  2018-11-07 13:50   ` Jonas Bonn
@ 2018-11-07 13:53     ` =?unknown-8bit?q?Pi=C4=8Dugins?= Arsenijs
  0 siblings, 0 replies; 13+ messages in thread
From: =?unknown-8bit?q?Pi=C4=8Dugins?= Arsenijs @ 2018-11-07 13:53 UTC (permalink / raw)
  To: ofono

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

> Confused by this. Isn't the sim900 the serial modem and sim800 the USB
> model? The above rule is for a strictly "serial" modem.

Nope, the SIM800 is a serial modem - it's, as far as I can tell, the next generation
after SIM900 or something along those lines. It also has a USB port, IIRC,
but I never had a chance to use that.

-Arsenijs

07.11.2018, 15:50, "Jonas Bonn" <jonas@norrbonn.se>:
> Hi Clement,
>
> On 07/11/2018 11:14, Clement Viel wrote:
>>  ---
>>    doc/sim800-modem.txt | 7 +++++++
>>    1 file changed, 7 insertions(+)
>>    create mode 100644 doc/sim800-modem.txt
>>
>>  diff --git a/doc/sim800-modem.txt b/doc/sim800-modem.txt
>>  new file mode 100644
>>  index 0000000..a299e97
>>  --- /dev/null
>>  +++ b/doc/sim800-modem.txt
>>  @@ -0,0 +1,7 @@
>>  +SIM800 modem usage
>>  +===================
>>  +
>>  +To enable SIM800 module support you need to put the following
>>  +udev rule into appropriate file in /{etc,lib}/udev/rules.d:
>>  +
>>  +KERNEL=="ttyS0", ENV{OFONO_DRIVER}="sim800"
>
> Confused by this. Isn't the sim900 the serial modem and sim800 the USB
> model? The above rule is for a strictly "serial" modem.
>
> /Jonas
> _______________________________________________
> ofono mailing list
> ofono(a)ofono.org
> https://lists.ofono.org/mailman/listinfo/ofono

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 3/3] sim800: add udev detection.
  2018-11-07 13:30   ` Jonas Bonn
@ 2018-11-07 13:58     ` Clement Viel
  0 siblings, 0 replies; 13+ messages in thread
From: Clement Viel @ 2018-11-07 13:58 UTC (permalink / raw)
  To: ofono

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

On Wed, Nov 07, 2018 at 02:30:43PM +0100, Jonas Bonn wrote:
> Hi Clement,
> 
> On 07/11/2018 11:14, Clement Viel wrote:
> >---
> >  plugins/udevng.c | 92 +++++++++++++++++++++++++++++++++++++++++---------------
> >  1 file changed, 68 insertions(+), 24 deletions(-)
> >
> >diff --git a/plugins/udevng.c b/plugins/udevng.c
> >index ff6e1fc..ee6f9e8 100644
> >--- a/plugins/udevng.c
> >+++ b/plugins/udevng.c
> >@@ -711,8 +711,56 @@ static gboolean setup_telitqmi(struct modem_info *modem)
> >  	return TRUE;
> >  }
> >-/* TODO: Not used as we have no simcom driver */
> >-static gboolean setup_simcom(struct modem_info *modem)
> >+
> >+static gboolean setup_sim800(struct modem_info *modem)
> >+{
> >+	const char *mdm = NULL, *aux = NULL, *gps = NULL, *diag = NULL;
> >+	GSList *list;
> >+
> >+	DBG("%s", modem->syspath);
> >+
> >+	for (list = modem->devices; list; list = list->next) {
> >+		struct device_info *info = list->data;
> >+
> >+		DBG("%s %s %s %s", info->devnode, info->interface,
> >+						info->number, info->label);
> >+
> >+		if (g_strcmp0(info->label, "aux") == 0) {
> >+			DBG("setting aux as info->devnode");
> >+			aux = info->devnode;
> >+			if (mdm != NULL)
> >+				break;
> >+		} else if (g_strcmp0(info->label, "modem") == 0) {
> >+			mdm = info->devnode;
> >+			if (aux != NULL)
> >+				break;
> >+		} else if (g_strcmp0(info->interface, "255/0/0") == 0) {
> >+			if (g_strcmp0(info->number, "00") == 0)
> >+				mdm = info->devnode;
> >+			else if (g_strcmp0(info->number, "01") == 0)
> >+				gps = info->devnode;
> >+			else if (g_strcmp0(info->number, "02") == 0)
> >+				aux = info->devnode;
> >+			else if (g_strcmp0(info->number, "03") == 0)
> >+				mdm = info->devnode;
> >+		}
> >+	}
> >+	DBG("modem=%s aux=%s gps=%s diag=%s", mdm, aux, gps, diag);
> >+
> 
> gps is unused.
> 
> 
> >+	if (mdm == NULL) {
> >+		DBG("modem did not set up");
> >+		return FALSE;
> >+	}
> >+
> >+	ofono_modem_set_string(modem->modem, "Device", mdm);
> 
> You probably mean 'aux' here.
> 
> >+	ofono_modem_set_string(modem->modem, "Modem", mdm);
> >+
> >+	return TRUE;
> >+}
> >+
> >+
> >+
> >+static gboolean setup_sim900(struct modem_info *modem)
> >  {
> >  	const char *mdm = NULL, *aux = NULL, *gps = NULL, *diag = NULL;
> >  	GSList *list;
> >@@ -962,6 +1010,8 @@ static gboolean setup_mbim(struct modem_info *modem)
> >  	ofono_modem_set_string(modem->modem, "Device", ctl);
> >  	ofono_modem_set_string(modem->modem, "NetworkInterface", net);
> >  	ofono_modem_set_string(modem->modem, "DescriptorFile", descriptors);
> >+	ofono_modem_set_string(modem->modem, "Vendor", modem->vendor);
> >+	ofono_modem_set_string(modem->modem, "Model", modem->model);
> 
> I don't see that these strings are being used anywhere in the modem driver.
> 
> >  	return TRUE;
> >  }
> >@@ -1193,22 +1243,12 @@ static gboolean setup_xmm7xxx(struct modem_info *modem)
> >  				info->interface, info->number, info->label,
> >  				info->sysattr, info->subsystem);
> >-		if (g_strcmp0(modem->model,"095a") == 0) {
> >-			if (g_strcmp0(info->subsystem, "tty") == 0) {
> >-				if (g_strcmp0(info->number, "00") == 0)
> >-					mdm = info->devnode;
> >-			} else if (g_strcmp0(info->subsystem, "net") == 0) {
> >-				if (g_strcmp0(info->number, "06") == 0)
> >-					net = info->devnode;
> >-			}
> >-		} else {
> >-			if (g_strcmp0(info->subsystem, "tty") == 0) {
> >-				if (g_strcmp0(info->number, "02") == 0)
> >-					mdm = info->devnode;
> >-			} else if (g_strcmp0(info->subsystem, "net") == 0) {
> >-				if (g_strcmp0(info->number, "00") == 0)
> >-					net = info->devnode;
> >-			}
> >+		if (g_strcmp0(info->subsystem, "tty") == 0) {
> >+			if (g_strcmp0(info->number, "02") == 0)
> >+				mdm = info->devnode;
> >+		} else if (g_strcmp0(info->subsystem, "net") == 0) {
> >+			if (g_strcmp0(info->number, "00") == 0)
> >+				net = info->devnode;
> >  		}
> >  	}
> 
> This change doesn't look like it belongs to this patch.
> 
> 
> >@@ -1290,7 +1330,8 @@ static struct {
> >  	{ "nokia",	setup_nokia	},
> >  	{ "telit",	setup_telit,	"device/interface"	},
> >  	{ "telitqmi",	setup_telitqmi	},
> >-	{ "simcom",	setup_simcom	},
> >+	{ "sim800",	setup_sim800	},
> >+	{ "sim900",	setup_sim900	},
> 
> Is the sim900 not a serial modem?  These setup routines are really only for
> USB devices.
> 
> >  	{ "sim7100",	setup_sim7100	},
> >  	{ "zte",	setup_zte	},
> >  	{ "icera",	setup_icera	},
> >@@ -1308,7 +1349,9 @@ static struct {
> >  	{ "calypso",	setup_serial_modem	},
> >  	{ "cinterion",	setup_serial_modem	},
> >  	{ "nokiacdma",	setup_serial_modem	},
> >+	{ "sim800",	setup_serial_modem	},
> >  	{ "sim900",	setup_serial_modem	},
> >+	{ "sim800",	setup_serial_modem	},
> 
> You've added the same line twice.  Furthermore, the sim800 is a USB modem
> (???) so it should not be set up in this way.
> 
> >  	{ "wavecom",	setup_wavecom		},
> >  	{ "tc65",	setup_tc65		},
> >  	{ "ehs6",	setup_ehs6		},
> >@@ -1471,7 +1514,7 @@ static void add_serial_device(struct udev_device *dev)
> >  	const char *subsystem;
> >  	struct udev_device* mdev;
> >  	const char* driver;
> >-
> >+	DBG("adding %s interface", udev_device_get_devpath(dev));
> >  	mdev = get_serial_modem_device(dev);
> >  	if (!mdev) {
> >  		DBG("Device is missing required OFONO_DRIVER property");
> >@@ -1663,7 +1706,8 @@ static struct {
> >  	{ "alcatel",	"option",	"1bbb", "0017"	},
> >  	{ "novatel",	"option",	"1410"		},
> >  	{ "zte",	"option",	"19d2"		},
> >-	{ "simcom",	"option",	"05c6", "9000"	},
> >+	{ "sim800",	"option",	"05c6", "9000"	}
> >+	{ "sim900",	"option",	"05c6", "9000"	},
> 
> 
> The sim900 is a serial modem... there is no vendor id nor product id.
> 
> 
> >  	{ "sim7100",	"option",	"1e0e", "9001"	},
> >  	{ "telit",	"usbserial",	"1bc7"		},
> >  	{ "telit",	"option",	"1bc7"		},
> >@@ -1693,7 +1737,6 @@ static struct {
> >  	{ "xmm7xxx",	"cdc_ncm",	"8087"		},
> >  	{ }
> >  };
> >-
> >  static void check_usb_device(struct udev_device *device)
> >  {
> >  	struct udev_device *usb_device;
> >@@ -1722,9 +1765,10 @@ static void check_usb_device(struct udev_device *device)
> >  			udev_device_get_parent_with_subsystem_devtype(
> >  				device, "usb", "usb_interface");
> >-		if (usb_interface)
> >+		if (usb_interface) {
> >  			driver = udev_device_get_property_value(
> >  					usb_interface, "OFONO_DRIVER");
> >+		}
> >  	}
> 
> No.
> 
> >  	if (driver == NULL) {
> >@@ -1801,7 +1845,7 @@ static gboolean create_modem(gpointer key, gpointer value, gpointer user_data)
> >  	struct modem_info *modem = value;
> >  	const char *syspath = key;
> >  	unsigned int i;
> >-
> >+	DBG("");
> >  	if (modem->modem != NULL)
> >  		return FALSE;
> 
> Doesn't belong to this patch, if at all.
> 
> >
> 
> /Jonas

Hi Jonas,

Thanks for the review, this patch is all wrong because it's not the good one...litlle bit tired, i'm sorry.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/3] sim800: add support for sim800 modem.
  2018-11-01 14:53 ` Denis Kenzior
@ 2018-11-02 23:31   ` Clement Viel
  0 siblings, 0 replies; 13+ messages in thread
From: Clement Viel @ 2018-11-02 23:31 UTC (permalink / raw)
  To: ofono

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

Hi Denis,

Thanks for the quick review !

On Thu, Nov 01, 2018 at 09:53:58AM -0500, Denis Kenzior wrote:
> Hi Clement,
> 
> On 10/15/2018 12:27 PM, Clement Viel wrote:
> >---
> >  plugins/sim900.c | 89 ++++++++++++++++++++++++++++++++++++++++++++++++++------
> >  1 file changed, 80 insertions(+), 9 deletions(-)
> >
> >diff --git a/plugins/sim900.c b/plugins/sim900.c
> >index a7728cd..9f3f334 100644
> >--- a/plugins/sim900.c
> >+++ b/plugins/sim900.c
> >@@ -24,6 +24,7 @@
> >  #endif
> >  #include <errno.h>
> >+#include <string.h>
> >  #include <stdlib.h>
> >  #include <glib.h>
> >  #include <gatchat.h>
> >@@ -60,13 +61,59 @@ static char *dlc_prefixes[NUM_DLC] = { "Voice: ", "Net: ", "SMS: ",
> >  static const char *none_prefix[] = { NULL };
> >+typedef enum type {
> >+	SIM800,
> >+	SIM900,
> >+	SIM_UNKNOWN,
> >+} type_t;
> >+
> >  struct sim900_data {
> >  	GIOChannel *device;
> >  	GAtMux *mux;
> >  	GAtChat * dlcs[NUM_DLC];
> >  	guint frame_size;
> >+	type_t modem_type;
> >  };
> >+static void set_modem_type (struct ofono_modem *modem)
> >+{
> >+	struct sim900_data *data = ofono_modem_get_data(modem);
> >+	const char *path = ofono_modem_get_path(modem);
> >+
> >+	if (strstr(path, "sim800"))
> >+		data->modem_type = SIM800;
> >+	else if (strstr(path, "sim900"))
> >+		data->modem_type = SIM900;
> >+	else
> >+		data->modem_type = SIM_UNKNOWN;
> >+
> 
> You can't really rely on the modem path for this.  Either query the model
> directly or set the type based on the driver selected.
> 
> And no empty lines at the end of the function please

I'll go with the CGMM way.

> 
> >+}
> >+
> >+static void mux_ready_notify(GAtResult *result, gpointer user_data)
> >+{
> >+	struct ofono_modem *modem = user_data;
> >+	struct sim900_data *data = ofono_modem_get_data(modem);
> >+	struct ofono_gprs *gprs = NULL;
> >+	struct ofono_gprs_context *gc;
> >+	static int notified = 0;
> >+
> >+	if (!notified) {
> >+	ofono_sms_create(modem, OFONO_VENDOR_SIMCOM, "atmodem",
> >+						data->dlcs[SMS_DLC]);
> >+
> >+
> >+	gprs = ofono_gprs_create(modem, 0, "atmodem", data->dlcs[GPRS_DLC]);
> >+	if (gprs == NULL)
> >+		return;
> >+
> >+	gc = ofono_gprs_context_create(modem, OFONO_VENDOR_SIMCOM,
> >+					"atmodem", data->dlcs[GPRS_DLC]);
> >+	if (gc)
> >+		ofono_gprs_add_context(gprs, gc);
> >+	}
> >+
> 
> The indentation and stule is all wrong here.
> 
> >+}
> >+
> >  static int sim900_probe(struct ofono_modem *modem)
> >  {
> >  	struct sim900_data *data;
> >@@ -79,6 +126,7 @@ static int sim900_probe(struct ofono_modem *modem)
> >  	ofono_modem_set_data(modem, data);
> >+	set_modem_type(modem);
> >  	return 0;
> >  }
> >@@ -111,6 +159,7 @@ static GAtChat *open_device(struct ofono_modem *modem,
> >  	GHashTable *options;
> >  	device = ofono_modem_get_string(modem, key);
> >+
> 
> This change is spurious and should not be part of this commit
> 
> >  	if (device == NULL)
> >  		return NULL;
> >@@ -232,6 +281,11 @@ static void setup_internal_mux(struct ofono_modem *modem)
> >  			goto error;
> >  		}
> >  	}
> >+	if (data->modem_type == SIM800) {
> >+		for (i = 0; i<NUM_DLC; i++) {
> >+			g_at_chat_register(data->dlcs[i], "SMS Ready", mux_ready_notify, FALSE, modem, NULL);
> >+		}
> >+	}
> 
> Why do you want this notification on all ports?  Isn't one enough?

These URC's are sent on all channels so I am catching them all to ensure all channels are now ready.

> >  	ofono_modem_set_powered(modem, TRUE);
> >@@ -353,18 +407,20 @@ static void sim900_post_sim(struct ofono_modem *modem)
> >  	DBG("%p", modem);
> >-	ofono_phonebook_create(modem, 0, "atmodem", data->dlcs[VOICE_DLC]);
> >-	ofono_sms_create(modem, OFONO_VENDOR_SIMCOM, "atmodem",
> >+	if (data->modem_type != SIM800) {
> >+		ofono_phonebook_create(modem, 0, "atmodem", data->dlcs[VOICE_DLC]);
> >+		ofono_sms_create(modem, OFONO_VENDOR_SIMCOM, "atmodem",
> >  						data->dlcs[SMS_DLC]);
> >-	gprs = ofono_gprs_create(modem, 0, "atmodem", data->dlcs[GPRS_DLC]);
> >-	if (gprs == NULL)
> >-		return;
> >+		gprs = ofono_gprs_create(modem, 0, "atmodem", data->dlcs[GPRS_DLC]);
> >+		if (gprs == NULL)
> >+			return;
> >-	gc = ofono_gprs_context_create(modem, OFONO_VENDOR_SIMCOM_SIM900,
> >+		gc = ofono_gprs_context_create(modem, OFONO_VENDOR_SIMCOM_SIM900,
> >  					"atmodem", data->dlcs[GPRS_DLC]);
> >-	if (gc)
> >-		ofono_gprs_add_context(gprs, gc);
> >+		if (gc)
> >+			ofono_gprs_add_context(gprs, gc);
> >+	}
> 
> Som SIM800 doesn't get any of these atoms?

SIM800 get these atoms by antoher way : the function setup_internal_mux registers the callback for SMS_READY URC. This is the callback that will call these atoms.
The Sim800 must respect this order when using these atoms else all the interfaces won't be up even if the AT commands returned OK.

> 
> >  }
> >  static void sim900_post_online(struct ofono_modem *modem)
> >@@ -391,9 +447,24 @@ static struct ofono_modem_driver sim900_driver = {
> >  	.post_online	= sim900_post_online,
> >  };
> >+
> >+static struct ofono_modem_driver sim800_driver = {
> >+	.name		= "sim800",
> >+	.probe		= sim900_probe,
> >+	.remove		= sim900_remove,
> >+	.enable		= sim900_enable,
> >+	.disable	= sim900_disable,
> >+	.pre_sim	= sim900_pre_sim,
> >+	.post_sim	= sim900_post_sim,
> >+	.post_online	= sim900_post_online,
> >+};
> >+
> >  static int sim900_init(void)
> >  {
> >-	return ofono_modem_driver_register(&sim900_driver);
> >+	int ret = 0;
> >+	ret = ofono_modem_driver_register(&sim800_driver);
> >+	ret += ofono_modem_driver_register(&sim900_driver);
> >+	return ret;
> 
> This doesn't really work this way.  The init function should either register
> all drivers successfully or fail.
> 
> >  }
> >  static void sim900_exit(void)
> >
> 
> Regards,
> -Denis

I'll add those remarks on the new patchset ( and i hope this one will be the one)!

Regards
Clem

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/3] sim800: add support for sim800 modem.
  2018-10-15 17:27 Clement Viel
  2018-10-31 15:16 ` =?unknown-8bit?q?Cl=C3=A9ment?= VIEL
@ 2018-11-01 14:53 ` Denis Kenzior
  2018-11-02 23:31   ` Clement Viel
  1 sibling, 1 reply; 13+ messages in thread
From: Denis Kenzior @ 2018-11-01 14:53 UTC (permalink / raw)
  To: ofono

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

Hi Clement,

On 10/15/2018 12:27 PM, Clement Viel wrote:
> ---
>   plugins/sim900.c | 89 ++++++++++++++++++++++++++++++++++++++++++++++++++------
>   1 file changed, 80 insertions(+), 9 deletions(-)
> 
> diff --git a/plugins/sim900.c b/plugins/sim900.c
> index a7728cd..9f3f334 100644
> --- a/plugins/sim900.c
> +++ b/plugins/sim900.c
> @@ -24,6 +24,7 @@
>   #endif
>   
>   #include <errno.h>
> +#include <string.h>
>   #include <stdlib.h>
>   #include <glib.h>
>   #include <gatchat.h>
> @@ -60,13 +61,59 @@ static char *dlc_prefixes[NUM_DLC] = { "Voice: ", "Net: ", "SMS: ",
>   
>   static const char *none_prefix[] = { NULL };
>   
> +typedef enum type {
> +	SIM800,
> +	SIM900,
> +	SIM_UNKNOWN,
> +} type_t;
> +
>   struct sim900_data {
>   	GIOChannel *device;
>   	GAtMux *mux;
>   	GAtChat * dlcs[NUM_DLC];
>   	guint frame_size;
> +	type_t modem_type;
>   };
>   
> +static void set_modem_type (struct ofono_modem *modem)
> +{
> +	struct sim900_data *data = ofono_modem_get_data(modem);
> +	const char *path = ofono_modem_get_path(modem);
> +
> +	if (strstr(path, "sim800"))
> +		data->modem_type = SIM800;
> +	else if (strstr(path, "sim900"))
> +		data->modem_type = SIM900;
> +	else
> +		data->modem_type = SIM_UNKNOWN;
> +

You can't really rely on the modem path for this.  Either query the 
model directly or set the type based on the driver selected.

And no empty lines at the end of the function please

> +}
> +
> +static void mux_ready_notify(GAtResult *result, gpointer user_data)
> +{
> +	struct ofono_modem *modem = user_data;
> +	struct sim900_data *data = ofono_modem_get_data(modem);
> +	struct ofono_gprs *gprs = NULL;
> +	struct ofono_gprs_context *gc;
> +	static int notified = 0;
> +
> +	if (!notified) {
> +	ofono_sms_create(modem, OFONO_VENDOR_SIMCOM, "atmodem",
> +						data->dlcs[SMS_DLC]);
> +
> +
> +	gprs = ofono_gprs_create(modem, 0, "atmodem", data->dlcs[GPRS_DLC]);
> +	if (gprs == NULL)
> +		return;
> +
> +	gc = ofono_gprs_context_create(modem, OFONO_VENDOR_SIMCOM,
> +					"atmodem", data->dlcs[GPRS_DLC]);
> +	if (gc)
> +		ofono_gprs_add_context(gprs, gc);
> +	}
> +

The indentation and stule is all wrong here.

> +}
> +
>   static int sim900_probe(struct ofono_modem *modem)
>   {
>   	struct sim900_data *data;
> @@ -79,6 +126,7 @@ static int sim900_probe(struct ofono_modem *modem)
>   
>   	ofono_modem_set_data(modem, data);
>   
> +	set_modem_type(modem);
>   	return 0;
>   }
>   
> @@ -111,6 +159,7 @@ static GAtChat *open_device(struct ofono_modem *modem,
>   	GHashTable *options;
>   
>   	device = ofono_modem_get_string(modem, key);
> +

This change is spurious and should not be part of this commit

>   	if (device == NULL)
>   		return NULL;
>   
> @@ -232,6 +281,11 @@ static void setup_internal_mux(struct ofono_modem *modem)
>   			goto error;
>   		}
>   	}
> +	if (data->modem_type == SIM800) {
> +		for (i = 0; i<NUM_DLC; i++) {
> +			g_at_chat_register(data->dlcs[i], "SMS Ready", mux_ready_notify, FALSE, modem, NULL);
> +		}
> +	}

Why do you want this notification on all ports?  Isn't one enough?

>   
>   	ofono_modem_set_powered(modem, TRUE);
>   
> @@ -353,18 +407,20 @@ static void sim900_post_sim(struct ofono_modem *modem)
>   
>   	DBG("%p", modem);
>   
> -	ofono_phonebook_create(modem, 0, "atmodem", data->dlcs[VOICE_DLC]);
> -	ofono_sms_create(modem, OFONO_VENDOR_SIMCOM, "atmodem",
> +	if (data->modem_type != SIM800) {
> +		ofono_phonebook_create(modem, 0, "atmodem", data->dlcs[VOICE_DLC]);
> +		ofono_sms_create(modem, OFONO_VENDOR_SIMCOM, "atmodem",
>   						data->dlcs[SMS_DLC]);
>   
> -	gprs = ofono_gprs_create(modem, 0, "atmodem", data->dlcs[GPRS_DLC]);
> -	if (gprs == NULL)
> -		return;
> +		gprs = ofono_gprs_create(modem, 0, "atmodem", data->dlcs[GPRS_DLC]);
> +		if (gprs == NULL)
> +			return;
>   
> -	gc = ofono_gprs_context_create(modem, OFONO_VENDOR_SIMCOM_SIM900,
> +		gc = ofono_gprs_context_create(modem, OFONO_VENDOR_SIMCOM_SIM900,
>   					"atmodem", data->dlcs[GPRS_DLC]);
> -	if (gc)
> -		ofono_gprs_add_context(gprs, gc);
> +		if (gc)
> +			ofono_gprs_add_context(gprs, gc);
> +	}

Som SIM800 doesn't get any of these atoms?

>   }
>   
>   static void sim900_post_online(struct ofono_modem *modem)
> @@ -391,9 +447,24 @@ static struct ofono_modem_driver sim900_driver = {
>   	.post_online	= sim900_post_online,
>   };
>   
> +
> +static struct ofono_modem_driver sim800_driver = {
> +	.name		= "sim800",
> +	.probe		= sim900_probe,
> +	.remove		= sim900_remove,
> +	.enable		= sim900_enable,
> +	.disable	= sim900_disable,
> +	.pre_sim	= sim900_pre_sim,
> +	.post_sim	= sim900_post_sim,
> +	.post_online	= sim900_post_online,
> +};
> +
>   static int sim900_init(void)
>   {
> -	return ofono_modem_driver_register(&sim900_driver);
> +	int ret = 0;
> +	ret = ofono_modem_driver_register(&sim800_driver);
> +	ret += ofono_modem_driver_register(&sim900_driver);
> +	return ret;

This doesn't really work this way.  The init function should either 
register all drivers successfully or fail.

>   }
>   
>   static void sim900_exit(void)
> 

Regards,
-Denis

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/3] sim800: add support for sim800 modem.
  2018-10-15 17:27 Clement Viel
@ 2018-10-31 15:16 ` =?unknown-8bit?q?Cl=C3=A9ment?= VIEL
  2018-11-01 14:53 ` Denis Kenzior
  1 sibling, 0 replies; 13+ messages in thread
From: =?unknown-8bit?q?Cl=C3=A9ment?= VIEL @ 2018-10-31 15:16 UTC (permalink / raw)
  To: ofono

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

Hi all,

I sent these patches 2 weeks ago and did not receive any feedback.
It's not urgent at all, I was just wandering whether they'll be
reviewed so I can move on :-)

Regards

Le lun. 15 oct. 2018 à 19:27, Clement Viel <vielclement@gmail.com> a écrit :
>
> ---
>  plugins/sim900.c | 89 ++++++++++++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 80 insertions(+), 9 deletions(-)
>
> diff --git a/plugins/sim900.c b/plugins/sim900.c
> index a7728cd..9f3f334 100644
> --- a/plugins/sim900.c
> +++ b/plugins/sim900.c
> @@ -24,6 +24,7 @@
>  #endif
>
>  #include <errno.h>
> +#include <string.h>
>  #include <stdlib.h>
>  #include <glib.h>
>  #include <gatchat.h>
> @@ -60,13 +61,59 @@ static char *dlc_prefixes[NUM_DLC] = { "Voice: ", "Net: ", "SMS: ",
>
>  static const char *none_prefix[] = { NULL };
>
> +typedef enum type {
> +       SIM800,
> +       SIM900,
> +       SIM_UNKNOWN,
> +} type_t;
> +
>  struct sim900_data {
>         GIOChannel *device;
>         GAtMux *mux;
>         GAtChat * dlcs[NUM_DLC];
>         guint frame_size;
> +       type_t modem_type;
>  };
>
> +static void set_modem_type (struct ofono_modem *modem)
> +{
> +       struct sim900_data *data = ofono_modem_get_data(modem);
> +       const char *path = ofono_modem_get_path(modem);
> +
> +       if (strstr(path, "sim800"))
> +               data->modem_type = SIM800;
> +       else if (strstr(path, "sim900"))
> +               data->modem_type = SIM900;
> +       else
> +               data->modem_type = SIM_UNKNOWN;
> +
> +}
> +
> +static void mux_ready_notify(GAtResult *result, gpointer user_data)
> +{
> +       struct ofono_modem *modem = user_data;
> +       struct sim900_data *data = ofono_modem_get_data(modem);
> +       struct ofono_gprs *gprs = NULL;
> +       struct ofono_gprs_context *gc;
> +       static int notified = 0;
> +
> +       if (!notified) {
> +       ofono_sms_create(modem, OFONO_VENDOR_SIMCOM, "atmodem",
> +                                               data->dlcs[SMS_DLC]);
> +
> +
> +       gprs = ofono_gprs_create(modem, 0, "atmodem", data->dlcs[GPRS_DLC]);
> +       if (gprs == NULL)
> +               return;
> +
> +       gc = ofono_gprs_context_create(modem, OFONO_VENDOR_SIMCOM,
> +                                       "atmodem", data->dlcs[GPRS_DLC]);
> +       if (gc)
> +               ofono_gprs_add_context(gprs, gc);
> +       }
> +
> +}
> +
>  static int sim900_probe(struct ofono_modem *modem)
>  {
>         struct sim900_data *data;
> @@ -79,6 +126,7 @@ static int sim900_probe(struct ofono_modem *modem)
>
>         ofono_modem_set_data(modem, data);
>
> +       set_modem_type(modem);
>         return 0;
>  }
>
> @@ -111,6 +159,7 @@ static GAtChat *open_device(struct ofono_modem *modem,
>         GHashTable *options;
>
>         device = ofono_modem_get_string(modem, key);
> +
>         if (device == NULL)
>                 return NULL;
>
> @@ -232,6 +281,11 @@ static void setup_internal_mux(struct ofono_modem *modem)
>                         goto error;
>                 }
>         }
> +       if (data->modem_type == SIM800) {
> +               for (i = 0; i<NUM_DLC; i++) {
> +                       g_at_chat_register(data->dlcs[i], "SMS Ready", mux_ready_notify, FALSE, modem, NULL);
> +               }
> +       }
>
>         ofono_modem_set_powered(modem, TRUE);
>
> @@ -353,18 +407,20 @@ static void sim900_post_sim(struct ofono_modem *modem)
>
>         DBG("%p", modem);
>
> -       ofono_phonebook_create(modem, 0, "atmodem", data->dlcs[VOICE_DLC]);
> -       ofono_sms_create(modem, OFONO_VENDOR_SIMCOM, "atmodem",
> +       if (data->modem_type != SIM800) {
> +               ofono_phonebook_create(modem, 0, "atmodem", data->dlcs[VOICE_DLC]);
> +               ofono_sms_create(modem, OFONO_VENDOR_SIMCOM, "atmodem",
>                                                 data->dlcs[SMS_DLC]);
>
> -       gprs = ofono_gprs_create(modem, 0, "atmodem", data->dlcs[GPRS_DLC]);
> -       if (gprs == NULL)
> -               return;
> +               gprs = ofono_gprs_create(modem, 0, "atmodem", data->dlcs[GPRS_DLC]);
> +               if (gprs == NULL)
> +                       return;
>
> -       gc = ofono_gprs_context_create(modem, OFONO_VENDOR_SIMCOM_SIM900,
> +               gc = ofono_gprs_context_create(modem, OFONO_VENDOR_SIMCOM_SIM900,
>                                         "atmodem", data->dlcs[GPRS_DLC]);
> -       if (gc)
> -               ofono_gprs_add_context(gprs, gc);
> +               if (gc)
> +                       ofono_gprs_add_context(gprs, gc);
> +       }
>  }
>
>  static void sim900_post_online(struct ofono_modem *modem)
> @@ -391,9 +447,24 @@ static struct ofono_modem_driver sim900_driver = {
>         .post_online    = sim900_post_online,
>  };
>
> +
> +static struct ofono_modem_driver sim800_driver = {
> +       .name           = "sim800",
> +       .probe          = sim900_probe,
> +       .remove         = sim900_remove,
> +       .enable         = sim900_enable,
> +       .disable        = sim900_disable,
> +       .pre_sim        = sim900_pre_sim,
> +       .post_sim       = sim900_post_sim,
> +       .post_online    = sim900_post_online,
> +};
> +
>  static int sim900_init(void)
>  {
> -       return ofono_modem_driver_register(&sim900_driver);
> +       int ret = 0;
> +       ret = ofono_modem_driver_register(&sim800_driver);
> +       ret += ofono_modem_driver_register(&sim900_driver);
> +       return ret;
>  }
>
>  static void sim900_exit(void)
> --
> 2.7.4
>


-- 
----
Clement Viel
Tel : +33688431961
-----

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 1/3] sim800: add support for sim800 modem.
@ 2018-10-15 17:27 Clement Viel
  2018-10-31 15:16 ` =?unknown-8bit?q?Cl=C3=A9ment?= VIEL
  2018-11-01 14:53 ` Denis Kenzior
  0 siblings, 2 replies; 13+ messages in thread
From: Clement Viel @ 2018-10-15 17:27 UTC (permalink / raw)
  To: ofono

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

---
 plugins/sim900.c | 89 ++++++++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 80 insertions(+), 9 deletions(-)

diff --git a/plugins/sim900.c b/plugins/sim900.c
index a7728cd..9f3f334 100644
--- a/plugins/sim900.c
+++ b/plugins/sim900.c
@@ -24,6 +24,7 @@
 #endif
 
 #include <errno.h>
+#include <string.h>
 #include <stdlib.h>
 #include <glib.h>
 #include <gatchat.h>
@@ -60,13 +61,59 @@ static char *dlc_prefixes[NUM_DLC] = { "Voice: ", "Net: ", "SMS: ",
 
 static const char *none_prefix[] = { NULL };
 
+typedef enum type {
+	SIM800,
+	SIM900,
+	SIM_UNKNOWN,
+} type_t;
+
 struct sim900_data {
 	GIOChannel *device;
 	GAtMux *mux;
 	GAtChat * dlcs[NUM_DLC];
 	guint frame_size;
+	type_t modem_type;
 };
 
+static void set_modem_type (struct ofono_modem *modem)
+{
+	struct sim900_data *data = ofono_modem_get_data(modem);
+	const char *path = ofono_modem_get_path(modem);
+
+	if (strstr(path, "sim800"))
+		data->modem_type = SIM800;
+	else if (strstr(path, "sim900"))
+		data->modem_type = SIM900;
+	else
+		data->modem_type = SIM_UNKNOWN;
+
+}
+
+static void mux_ready_notify(GAtResult *result, gpointer user_data)
+{
+	struct ofono_modem *modem = user_data;
+	struct sim900_data *data = ofono_modem_get_data(modem);
+	struct ofono_gprs *gprs = NULL;
+	struct ofono_gprs_context *gc;
+	static int notified = 0;
+
+	if (!notified) {
+	ofono_sms_create(modem, OFONO_VENDOR_SIMCOM, "atmodem",
+						data->dlcs[SMS_DLC]);
+
+
+	gprs = ofono_gprs_create(modem, 0, "atmodem", data->dlcs[GPRS_DLC]);
+	if (gprs == NULL)
+		return;
+
+	gc = ofono_gprs_context_create(modem, OFONO_VENDOR_SIMCOM,
+					"atmodem", data->dlcs[GPRS_DLC]);
+	if (gc)
+		ofono_gprs_add_context(gprs, gc);
+	}
+
+}
+
 static int sim900_probe(struct ofono_modem *modem)
 {
 	struct sim900_data *data;
@@ -79,6 +126,7 @@ static int sim900_probe(struct ofono_modem *modem)
 
 	ofono_modem_set_data(modem, data);
 
+	set_modem_type(modem);
 	return 0;
 }
 
@@ -111,6 +159,7 @@ static GAtChat *open_device(struct ofono_modem *modem,
 	GHashTable *options;
 
 	device = ofono_modem_get_string(modem, key);
+
 	if (device == NULL)
 		return NULL;
 
@@ -232,6 +281,11 @@ static void setup_internal_mux(struct ofono_modem *modem)
 			goto error;
 		}
 	}
+	if (data->modem_type == SIM800) {
+		for (i = 0; i<NUM_DLC; i++) {
+			g_at_chat_register(data->dlcs[i], "SMS Ready", mux_ready_notify, FALSE, modem, NULL);
+		}
+	}
 
 	ofono_modem_set_powered(modem, TRUE);
 
@@ -353,18 +407,20 @@ static void sim900_post_sim(struct ofono_modem *modem)
 
 	DBG("%p", modem);
 
-	ofono_phonebook_create(modem, 0, "atmodem", data->dlcs[VOICE_DLC]);
-	ofono_sms_create(modem, OFONO_VENDOR_SIMCOM, "atmodem",
+	if (data->modem_type != SIM800) {
+		ofono_phonebook_create(modem, 0, "atmodem", data->dlcs[VOICE_DLC]);
+		ofono_sms_create(modem, OFONO_VENDOR_SIMCOM, "atmodem",
 						data->dlcs[SMS_DLC]);
 
-	gprs = ofono_gprs_create(modem, 0, "atmodem", data->dlcs[GPRS_DLC]);
-	if (gprs == NULL)
-		return;
+		gprs = ofono_gprs_create(modem, 0, "atmodem", data->dlcs[GPRS_DLC]);
+		if (gprs == NULL)
+			return;
 
-	gc = ofono_gprs_context_create(modem, OFONO_VENDOR_SIMCOM_SIM900,
+		gc = ofono_gprs_context_create(modem, OFONO_VENDOR_SIMCOM_SIM900,
 					"atmodem", data->dlcs[GPRS_DLC]);
-	if (gc)
-		ofono_gprs_add_context(gprs, gc);
+		if (gc)
+			ofono_gprs_add_context(gprs, gc);
+	}
 }
 
 static void sim900_post_online(struct ofono_modem *modem)
@@ -391,9 +447,24 @@ static struct ofono_modem_driver sim900_driver = {
 	.post_online	= sim900_post_online,
 };
 
+
+static struct ofono_modem_driver sim800_driver = {
+	.name		= "sim800",
+	.probe		= sim900_probe,
+	.remove		= sim900_remove,
+	.enable		= sim900_enable,
+	.disable	= sim900_disable,
+	.pre_sim	= sim900_pre_sim,
+	.post_sim	= sim900_post_sim,
+	.post_online	= sim900_post_online,
+};
+
 static int sim900_init(void)
 {
-	return ofono_modem_driver_register(&sim900_driver);
+	int ret = 0;
+	ret = ofono_modem_driver_register(&sim800_driver);
+	ret += ofono_modem_driver_register(&sim900_driver);
+	return ret;
 }
 
 static void sim900_exit(void)
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 1/3] sim800: add support for sim800 modem.
@ 2018-10-03 13:28 Clement Viel
  0 siblings, 0 replies; 13+ messages in thread
From: Clement Viel @ 2018-10-03 13:28 UTC (permalink / raw)
  To: ofono

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

---
 plugins/sim900.c | 89 ++++++++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 80 insertions(+), 9 deletions(-)

diff --git a/plugins/sim900.c b/plugins/sim900.c
index a7728cd..9f3f334 100644
--- a/plugins/sim900.c
+++ b/plugins/sim900.c
@@ -24,6 +24,7 @@
 #endif
 
 #include <errno.h>
+#include <string.h>
 #include <stdlib.h>
 #include <glib.h>
 #include <gatchat.h>
@@ -60,13 +61,59 @@ static char *dlc_prefixes[NUM_DLC] = { "Voice: ", "Net: ", "SMS: ",
 
 static const char *none_prefix[] = { NULL };
 
+typedef enum type {
+	SIM800,
+	SIM900,
+	SIM_UNKNOWN,
+} type_t;
+
 struct sim900_data {
 	GIOChannel *device;
 	GAtMux *mux;
 	GAtChat * dlcs[NUM_DLC];
 	guint frame_size;
+	type_t modem_type;
 };
 
+static void set_modem_type (struct ofono_modem *modem)
+{
+	struct sim900_data *data = ofono_modem_get_data(modem);
+	const char *path = ofono_modem_get_path(modem);
+
+	if (strstr(path, "sim800"))
+		data->modem_type = SIM800;
+	else if (strstr(path, "sim900"))
+		data->modem_type = SIM900;
+	else
+		data->modem_type = SIM_UNKNOWN;
+
+}
+
+static void mux_ready_notify(GAtResult *result, gpointer user_data)
+{
+	struct ofono_modem *modem = user_data;
+	struct sim900_data *data = ofono_modem_get_data(modem);
+	struct ofono_gprs *gprs = NULL;
+	struct ofono_gprs_context *gc;
+	static int notified = 0;
+
+	if (!notified) {
+	ofono_sms_create(modem, OFONO_VENDOR_SIMCOM, "atmodem",
+						data->dlcs[SMS_DLC]);
+
+
+	gprs = ofono_gprs_create(modem, 0, "atmodem", data->dlcs[GPRS_DLC]);
+	if (gprs == NULL)
+		return;
+
+	gc = ofono_gprs_context_create(modem, OFONO_VENDOR_SIMCOM,
+					"atmodem", data->dlcs[GPRS_DLC]);
+	if (gc)
+		ofono_gprs_add_context(gprs, gc);
+	}
+
+}
+
 static int sim900_probe(struct ofono_modem *modem)
 {
 	struct sim900_data *data;
@@ -79,6 +126,7 @@ static int sim900_probe(struct ofono_modem *modem)
 
 	ofono_modem_set_data(modem, data);
 
+	set_modem_type(modem);
 	return 0;
 }
 
@@ -111,6 +159,7 @@ static GAtChat *open_device(struct ofono_modem *modem,
 	GHashTable *options;
 
 	device = ofono_modem_get_string(modem, key);
+
 	if (device == NULL)
 		return NULL;
 
@@ -232,6 +281,11 @@ static void setup_internal_mux(struct ofono_modem *modem)
 			goto error;
 		}
 	}
+	if (data->modem_type == SIM800) {
+		for (i = 0; i<NUM_DLC; i++) {
+			g_at_chat_register(data->dlcs[i], "SMS Ready", mux_ready_notify, FALSE, modem, NULL);
+		}
+	}
 
 	ofono_modem_set_powered(modem, TRUE);
 
@@ -353,18 +407,20 @@ static void sim900_post_sim(struct ofono_modem *modem)
 
 	DBG("%p", modem);
 
-	ofono_phonebook_create(modem, 0, "atmodem", data->dlcs[VOICE_DLC]);
-	ofono_sms_create(modem, OFONO_VENDOR_SIMCOM, "atmodem",
+	if (data->modem_type != SIM800) {
+		ofono_phonebook_create(modem, 0, "atmodem", data->dlcs[VOICE_DLC]);
+		ofono_sms_create(modem, OFONO_VENDOR_SIMCOM, "atmodem",
 						data->dlcs[SMS_DLC]);
 
-	gprs = ofono_gprs_create(modem, 0, "atmodem", data->dlcs[GPRS_DLC]);
-	if (gprs == NULL)
-		return;
+		gprs = ofono_gprs_create(modem, 0, "atmodem", data->dlcs[GPRS_DLC]);
+		if (gprs == NULL)
+			return;
 
-	gc = ofono_gprs_context_create(modem, OFONO_VENDOR_SIMCOM_SIM900,
+		gc = ofono_gprs_context_create(modem, OFONO_VENDOR_SIMCOM_SIM900,
 					"atmodem", data->dlcs[GPRS_DLC]);
-	if (gc)
-		ofono_gprs_add_context(gprs, gc);
+		if (gc)
+			ofono_gprs_add_context(gprs, gc);
+	}
 }
 
 static void sim900_post_online(struct ofono_modem *modem)
@@ -391,9 +447,24 @@ static struct ofono_modem_driver sim900_driver = {
 	.post_online	= sim900_post_online,
 };
 
+
+static struct ofono_modem_driver sim800_driver = {
+	.name		= "sim800",
+	.probe		= sim900_probe,
+	.remove		= sim900_remove,
+	.enable		= sim900_enable,
+	.disable	= sim900_disable,
+	.pre_sim	= sim900_pre_sim,
+	.post_sim	= sim900_post_sim,
+	.post_online	= sim900_post_online,
+};
+
 static int sim900_init(void)
 {
-	return ofono_modem_driver_register(&sim900_driver);
+	int ret = 0;
+	ret = ofono_modem_driver_register(&sim800_driver);
+	ret += ofono_modem_driver_register(&sim900_driver);
+	return ret;
 }
 
 static void sim900_exit(void)
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2018-11-07 13:58 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-11-07 10:14 [PATCH 1/3] sim800: add support for sim800 modem Clement Viel
2018-11-07 10:14 ` [PATCH 2/3] sim800: add documentation Clement Viel
2018-11-07 13:50   ` Jonas Bonn
2018-11-07 13:53     ` =?unknown-8bit?q?Pi=C4=8Dugins?= Arsenijs
2018-11-07 10:14 ` [PATCH 3/3] sim800: add udev detection Clement Viel
2018-11-07 13:30   ` Jonas Bonn
2018-11-07 13:58     ` Clement Viel
2018-11-07 13:47 ` [PATCH 1/3] sim800: add support for sim800 modem Jonas Bonn
  -- strict thread matches above, loose matches on Subject: below --
2018-10-15 17:27 Clement Viel
2018-10-31 15:16 ` =?unknown-8bit?q?Cl=C3=A9ment?= VIEL
2018-11-01 14:53 ` Denis Kenzior
2018-11-02 23:31   ` Clement Viel
2018-10-03 13:28 Clement Viel

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.