[PATCH 3/3] sim800: add udev detection

[PATCH 3/3] sim800: add udev detection

[Clement Viel]

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

---
 plugins/udevng.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/plugins/udevng.c b/plugins/udevng.c
index 3c39e68..202519e 100644
--- a/plugins/udevng.c
+++ b/plugins/udevng.c
@@ -1300,6 +1300,7 @@ static struct {
 	{ "calypso",	setup_serial_modem	},
 	{ "cinterion",	setup_serial_modem	},
 	{ "nokiacdma",	setup_serial_modem	},
+	{ "sim800",	setup_serial_modem	},
 	{ "sim900",	setup_serial_modem	},
 	{ "wavecom",	setup_wavecom		},
 	{ "tc65",	setup_tc65		},
-- 
2.7.4

[PATCH 3/3] sim800: add udev detection

[Clement Viel]

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

---
 plugins/udevng.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/plugins/udevng.c b/plugins/udevng.c
index 3c39e68..202519e 100644
--- a/plugins/udevng.c
+++ b/plugins/udevng.c
@@ -1300,6 +1300,7 @@ static struct {
 	{ "calypso",	setup_serial_modem	},
 	{ "cinterion",	setup_serial_modem	},
 	{ "nokiacdma",	setup_serial_modem	},
+	{ "sim800",	setup_serial_modem	},
 	{ "sim900",	setup_serial_modem	},
 	{ "wavecom",	setup_wavecom		},
 	{ "tc65",	setup_tc65		},
-- 
2.7.4

[PATCH 1/3] sim800: add support for sim800 modem.

[Clement Viel]

[-- 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

[PATCH 2/3] sim800: add documentation.

[Clement Viel]

[-- 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

[PATCH 3/3] sim800: add udev detection.

[Clement Viel]

[-- 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

Re: [PATCH 3/3] sim800: add udev detection.

[Jonas Bonn]

[-- 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

Re: [PATCH 1/3] sim800: add support for sim800 modem.

[Jonas Bonn]

[-- 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

Re: [PATCH 2/3] sim800: add documentation.

[Jonas Bonn]

[-- 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

Re: [PATCH 2/3] sim800: add documentation.

[=?unknown-8bit?q?Pi=C4=8Dugins?= Arsenijs]

[-- 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

Re: [PATCH 3/3] sim800: add udev detection.

[Clement Viel]

[-- 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.

[PATCH 3/3] sim800: add udev detection.

[Clement Viel]

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

---
 plugins/udevng.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/plugins/udevng.c b/plugins/udevng.c
index ff6e1fc..d0a849d 100644
--- a/plugins/udevng.c
+++ b/plugins/udevng.c
@@ -1308,6 +1308,7 @@ static struct {
 	{ "calypso",	setup_serial_modem	},
 	{ "cinterion",	setup_serial_modem	},
 	{ "nokiacdma",	setup_serial_modem	},
+	{ "sim800",	setup_serial_modem	},
 	{ "sim900",	setup_serial_modem	},
 	{ "wavecom",	setup_wavecom		},
 	{ "tc65",	setup_tc65		},
-- 
2.7.4

Re: [PATCH 3/3] sim800: add udev detection.

[Jonas Bonn]

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



On 07/11/2018 15:00, Clement Viel wrote:
> ---
>   plugins/udevng.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/plugins/udevng.c b/plugins/udevng.c
> index ff6e1fc..d0a849d 100644
> --- a/plugins/udevng.c
> +++ b/plugins/udevng.c
> @@ -1308,6 +1308,7 @@ static struct {
>   	{ "calypso",	setup_serial_modem	},
>   	{ "cinterion",	setup_serial_modem	},
>   	{ "nokiacdma",	setup_serial_modem	},
> +	{ "sim800",	setup_serial_modem	},

Right... that's much simpler.  And given that you are extending the 
sim900 driver to handle the sim800 model, you don't need the above line 
either.

/Jonas

>   	{ "sim900",	setup_serial_modem	},
>   	{ "wavecom",	setup_wavecom		},
>   	{ "tc65",	setup_tc65		},
> 

Re: [PATCH 3/3] sim800: add udev detection.

[Clement Viel]

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

On Wed, Nov 07, 2018 at 03:02:54PM +0100, Jonas Bonn wrote:
> 
> 
> On 07/11/2018 15:00, Clement Viel wrote:
> >---
> >  plugins/udevng.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> >diff --git a/plugins/udevng.c b/plugins/udevng.c
> >index ff6e1fc..d0a849d 100644
> >--- a/plugins/udevng.c
> >+++ b/plugins/udevng.c
> >@@ -1308,6 +1308,7 @@ static struct {
> >  	{ "calypso",	setup_serial_modem	},
> >  	{ "cinterion",	setup_serial_modem	},
> >  	{ "nokiacdma",	setup_serial_modem	},
> >+	{ "sim800",	setup_serial_modem	},
> 
> Right... that's much simpler.  And given that you are extending the sim900
> driver to handle the sim800 model, you don't need the above line either.
> 
> /Jonas
> 
> >  	{ "sim900",	setup_serial_modem	},
> >  	{ "wavecom",	setup_wavecom		},
> >  	{ "tc65",	setup_tc65		},
> >
I think I must keep this line because the udev rule added by the user will specify that modem is "OFONO_DRIVER=sim800" so udevng.c must know what to do with that.

Re: [PATCH 3/3] sim800: add udev detection.

[Denis Kenzior]

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

Hi Clement,
>>> +	{ "sim800",	setup_serial_modem	}, > I think I must keep this line because the udev rule added by the user 
will specify that modem is "OFONO_DRIVER=sim800" so udevng.c must know 
what to do with that.
This is kind of weird that we have a driver line for a modem driver that 
doesn't exist.  We never do things this way.  Why don't you just modify 
the driver instructions to have OFONO_DRIVER=sim900 instead?

Also, we could simply rename the sim900 driver into sim_x00 or 
simcom_serial or something too.  That could make things a bit more clear.

Regards,
-Denis

Re: [PATCH 3/3] sim800: add udev detection.

[Clement Viel]

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

On Thu, Nov 08, 2018 at 10:59:30AM -0600, Denis Kenzior wrote:

Hi Denis,

> Hi Clement,
> >>>+	{ "sim800",	setup_serial_modem	}, > I think I must keep this line
> >>>because the udev rule added by the user
> will specify that modem is "OFONO_DRIVER=sim800" so udevng.c must know what
> to do with that.
> This is kind of weird that we have a driver line for a modem driver that
> doesn't exist.  We never do things this way.  Why don't you just modify the
> driver instructions to have OFONO_DRIVER=sim900 instead?
> 
I agree with you, I was thinking the sim800 must be mentioned to avoid confusing
doc/sim900.txt to mention that sim800 is supported by this driver. Then, keep in
the users.

> Also, we could simply rename the sim900 driver into sim_x00 or simcom_serial
> or something too.  That could make things a bit more clear.

I think we can keep that in mind for further development.

> 
> Regards,
> -Denis

Just tell me whether I should add mention of sim800 in sim900.txt or rename sim900.txt
in simcom.txt to mention the two drivers. Then I'll upload the patchset with doc in it.

Regards
Clement

Re: [PATCH 3/3] sim800: add udev detection.

[Clement Viel]

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

On Thu, Nov 08, 2018 at 10:59:30AM -0600, Denis Kenzior wrote:
> Hi Clement,
> >>>+	{ "sim800",	setup_serial_modem	}, > I think I must keep this line
> >>>because the udev rule added by the user
> will specify that modem is "OFONO_DRIVER=sim800" so udevng.c must know what
> to do with that.
> This is kind of weird that we have a driver line for a modem driver that
> doesn't exist.  We never do things this way.  Why don't you just modify the
> driver instructions to have OFONO_DRIVER=sim900 instead?
> 
> Also, we could simply rename the sim900 driver into sim_x00 or simcom_serial
> or something too.  That could make things a bit more clear.
> 
> Regards,
> -Denis


Moreover if we go for the OFONO_DRIVER=sim900 solution, plugins/udevng.c will stay 
the same.

Re: [PATCH 3/3] sim800: add udev detection.

[Denis Kenzior]

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

Hi Clement,

> Just tell me whether I should add mention of sim800 in sim900.txt or rename sim900.txt
> in simcom.txt to mention the two drivers. Then I'll upload the patchset with doc in it.

I don't feel strongly either way, but if I had to pick, I say lets 
rename sim900.txt into simcom.txt and maintain all driver instructions 
there.

Regards,
-Denis