[PATCH v3 0/2] Telit LE910 V1 QMI support

[PATCH v3 0/2] Telit LE910 V1 QMI support

[Lukasz Nowak]

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

From: Lukasz Nowak <lnowak@tycoint.com>

Changes from V2:
- made AlwaysOnline flow cleaner in gobi.c
- it turns out that AlwaysOnline is already a part of core logic
  in src/modem.c which have the same effect as if the driver had
  no set_online() function at all. Hence removed changes to
  gobi_set_online() as it is never called.


Changes from V1:
- split into multiple smaller commits
- added comments in code, documenting issues with the Telit QMI firmware
- disabled non-working low-power operating modes in additional cases
- use mccmnc string instead of truncating an invalid operator name

Lukasz Nowak (2):
  gobi: Do not use low-power modes for some modems
  udevng: add Telit LE910 V1 support

 plugins/gobi.c   | 19 +++++++++++++++++++
 plugins/udevng.c | 24 ++++++++++++++++++++----
 2 files changed, 39 insertions(+), 4 deletions(-)

-- 
2.7.4

[PATCH v3 1/2] gobi: Do not use low-power modes for some modems

[Lukasz Nowak]

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

From: Lukasz Nowak <lnowak@tycoint.com>

Telit QMI modems have a problem with the low-power operating modes.
After entering and leaving such a state, UIM service does not return.
The sim card is still marked as powered-down. The QMI interface does
not have a way to power it back on.

To avoid this, keep modems with the "AlwaysOnline" flag online
in the disable-modem and offline-modem procedures.
---
 plugins/gobi.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/plugins/gobi.c b/plugins/gobi.c
index 6a78941..06f906d 100644
--- a/plugins/gobi.c
+++ b/plugins/gobi.c
@@ -168,6 +168,16 @@ static void get_oper_mode_cb(struct qmi_result *result, void *user_data)
 
 	data->oper_mode = mode;
 
+	/*
+	 * Telit QMI LTE modem must remain online. If powered down, it also
+	 * powers down the sim card, and QMI interface has no way to bring
+	 * it back alive.
+	 */
+	if (ofono_modem_get_boolean(modem, "AlwaysOnline")) {
+		ofono_modem_set_powered(modem, TRUE);
+		return;
+	}
+
 	switch (data->oper_mode) {
 	case QMI_DMS_OPER_MODE_ONLINE:
 		param = qmi_param_new_uint8(QMI_DMS_PARAM_OPER_MODE,
@@ -353,6 +363,14 @@ static int gobi_disable(struct ofono_modem *modem)
 	qmi_service_cancel_all(data->dms);
 	qmi_service_unregister_all(data->dms);
 
+	/*
+	 * Telit QMI modem must remain online. If powered down, it also
+	 * powers down the sim card, and QMI interface has no way to bring
+	 * it back alive.
+	 */
+	if (ofono_modem_get_boolean(modem, "AlwaysOnline"))
+		goto out;
+
 	param = qmi_param_new_uint8(QMI_DMS_PARAM_OPER_MODE,
 					QMI_DMS_OPER_MODE_PERSIST_LOW_POWER);
 	if (!param)
@@ -362,6 +380,7 @@ static int gobi_disable(struct ofono_modem *modem)
 					power_disable_cb, modem, NULL) > 0)
 		return -EINPROGRESS;
 
+out:
 	shutdown_device(modem);
 
 	return -EINPROGRESS;
-- 
2.7.4

[PATCH v3 2/2] udevng: add Telit LE910 V1 support

[Lukasz Nowak]

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

From: Lukasz Nowak <lnowak@tycoint.com>

Tested with LE910-SVG and Verizon.
---
 plugins/udevng.c | 24 ++++++++++++++++++++----
 1 file changed, 20 insertions(+), 4 deletions(-)

diff --git a/plugins/udevng.c b/plugins/udevng.c
index 2279bbe..3b15064 100644
--- a/plugins/udevng.c
+++ b/plugins/udevng.c
@@ -182,14 +182,15 @@ static gboolean setup_gobi(struct modem_info *modem)
 	const char *qmi = NULL, *mdm = NULL, *net = NULL;
 	const char *gps = NULL, *diag = NULL;
 	GSList *list;
+	gboolean telit = FALSE;
 
 	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);
+		DBG("%s %s %s %s %s", info->devnode, info->interface,
+					info->number, info->label, info->subsystem);
 
 		if (g_strcmp0(info->interface, "255/255/255") == 0) {
 			if (info->number == NULL)
@@ -199,10 +200,18 @@ static gboolean setup_gobi(struct modem_info *modem)
 			else if (g_strcmp0(info->number, "01") == 0)
 				diag = info->devnode;
 			else if (g_strcmp0(info->number, "02") == 0)
-				mdm = info->devnode;
+				if (g_strcmp0(info->subsystem, "net") == 0)
+					net = info->devnode;
+				else if (g_strcmp0(info->subsystem, "usbmisc") == 0) {
+					telit = TRUE;
+					qmi = info->devnode;
+				} else
+					mdm = info->devnode;
 			else if (g_strcmp0(info->number, "03") == 0)
 				gps = info->devnode;
-		}
+		} else if (g_strcmp0(info->interface, "255/0/0") == 0)
+			if (g_strcmp0(info->number, "04") == 0)
+				mdm = info->devnode;
 	}
 
 	if (qmi == NULL || mdm == NULL || net == NULL)
@@ -215,6 +224,12 @@ static gboolean setup_gobi(struct modem_info *modem)
 	ofono_modem_set_string(modem->modem, "Diag", diag);
 	ofono_modem_set_string(modem->modem, "NetworkInterface", net);
 
+	if (telit) {
+		/* Telit LE910 V1 modems */
+		ofono_modem_set_boolean(modem->modem, "ForceSimLegacy", TRUE);
+		ofono_modem_set_boolean(modem->modem, "AlwaysOnline", TRUE);
+	}
+
 	return TRUE;
 }
 
@@ -1168,6 +1183,7 @@ static struct {
 	{ "mbm",	"cdc_ether",	"0930"		},
 	{ "mbm",	"cdc_ncm",	"0930"		},
 	{ "hso",	"hso"				},
+	{ "gobi",	"option",	"1bc7", "1201"	},
 	{ "gobi",	"qmi_wwan"			},
 	{ "gobi",	"qcserial"			},
 	{ "sierra",	"qmi_wwan",	"1199"		},
-- 
2.7.4

Re: [PATCH v3 1/2] gobi: Do not use low-power modes for some modems

[Denis Kenzior]

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

Hi Lukasz,

On 03/24/2017 09:51 AM, Lukasz Nowak wrote:
> From: Lukasz Nowak <lnowak@tycoint.com>
>
> Telit QMI modems have a problem with the low-power operating modes.
> After entering and leaving such a state, UIM service does not return.
> The sim card is still marked as powered-down. The QMI interface does
> not have a way to power it back on.
>
> To avoid this, keep modems with the "AlwaysOnline" flag online
> in the disable-modem and offline-modem procedures.
> ---
>  plugins/gobi.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
>

Applied, thanks.

Regards,
-Denis

Re: [PATCH v3 0/2] Telit LE910 V1 QMI support

[Denis Kenzior]

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

Hi Lukasz,

On 03/24/2017 09:51 AM, Lukasz Nowak wrote:
> From: Lukasz Nowak <lnowak@tycoint.com>
>
> Changes from V2:
> - made AlwaysOnline flow cleaner in gobi.c
> - it turns out that AlwaysOnline is already a part of core logic
>   in src/modem.c which have the same effect as if the driver had
>   no set_online() function at all. Hence removed changes to
>   gobi_set_online() as it is never called.
>

Ah yes, I completely forgot that someone added the AlwaysOnline override 
so you didn't need to implement a brand new driver without .set_online 
implemented.  Good that you discovered that, sorry for the confusion.

Regards,
-Denis

Re: [PATCH v3 2/2] udevng: add Telit LE910 V1 support

[Denis Kenzior]

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

Hi Lukasz,

On 03/24/2017 09:51 AM, Lukasz Nowak wrote:
> From: Lukasz Nowak <lnowak@tycoint.com>
>
> Tested with LE910-SVG and Verizon.
> ---
>  plugins/udevng.c | 24 ++++++++++++++++++++----
>  1 file changed, 20 insertions(+), 4 deletions(-)
>
> diff --git a/plugins/udevng.c b/plugins/udevng.c
> index 2279bbe..3b15064 100644
> --- a/plugins/udevng.c
> +++ b/plugins/udevng.c
> @@ -182,14 +182,15 @@ static gboolean setup_gobi(struct modem_info *modem)
>  	const char *qmi = NULL, *mdm = NULL, *net = NULL;
>  	const char *gps = NULL, *diag = NULL;
>  	GSList *list;
> +	gboolean telit = FALSE;
>
>  	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);
> +		DBG("%s %s %s %s %s", info->devnode, info->interface,
> +					info->number, info->label, info->subsystem);
>
>  		if (g_strcmp0(info->interface, "255/255/255") == 0) {
>  			if (info->number == NULL)
> @@ -199,10 +200,18 @@ static gboolean setup_gobi(struct modem_info *modem)
>  			else if (g_strcmp0(info->number, "01") == 0)
>  				diag = info->devnode;
>  			else if (g_strcmp0(info->number, "02") == 0)
> -				mdm = info->devnode;
> +				if (g_strcmp0(info->subsystem, "net") == 0)
> +					net = info->devnode;
> +				else if (g_strcmp0(info->subsystem, "usbmisc") == 0) {

just a small nit:  this line is over 80 characters

> +					telit = TRUE;

Would it be safer to set this value based on usb major/minor number instead?

> +					qmi = info->devnode;
> +				} else
> +					mdm = info->devnode;
>  			else if (g_strcmp0(info->number, "03") == 0)
>  				gps = info->devnode;
> -		}
> +		} else if (g_strcmp0(info->interface, "255/0/0") == 0)
> +			if (g_strcmp0(info->number, "04") == 0)
> +				mdm = info->devnode;
>  	}
>
>  	if (qmi == NULL || mdm == NULL || net == NULL)
> @@ -215,6 +224,12 @@ static gboolean setup_gobi(struct modem_info *modem)
>  	ofono_modem_set_string(modem->modem, "Diag", diag);
>  	ofono_modem_set_string(modem->modem, "NetworkInterface", net);
>
> +	if (telit) {
> +		/* Telit LE910 V1 modems */
> +		ofono_modem_set_boolean(modem->modem, "ForceSimLegacy", TRUE);
> +		ofono_modem_set_boolean(modem->modem, "AlwaysOnline", TRUE);
> +	}
> +
>  	return TRUE;
>  }
>
> @@ -1168,6 +1183,7 @@ static struct {
>  	{ "mbm",	"cdc_ether",	"0930"		},
>  	{ "mbm",	"cdc_ncm",	"0930"		},
>  	{ "hso",	"hso"				},
> +	{ "gobi",	"option",	"1bc7", "1201"	},
>  	{ "gobi",	"qmi_wwan"			},
>  	{ "gobi",	"qcserial"			},
>  	{ "sierra",	"qmi_wwan",	"1199"		},
>

Regards,
-Denis

Re: [PATCH v3 2/2] udevng: add Telit LE910 V1 support

[Jonas Bonn]

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

Hi Lukasz,

I'm working on support for the Quectel UC21 and have some similar 
patches to what you have below.  Based on that experience, here are some 
comments:

On 03/24/2017 03:51 PM, Lukasz Nowak wrote:
> From: Lukasz Nowak <lnowak@tycoint.com>
>
> Tested with LE910-SVG and Verizon.
> ---
>   plugins/udevng.c | 24 ++++++++++++++++++++----
>   1 file changed, 20 insertions(+), 4 deletions(-)
>
> diff --git a/plugins/udevng.c b/plugins/udevng.c
> index 2279bbe..3b15064 100644
> --- a/plugins/udevng.c
> +++ b/plugins/udevng.c
> @@ -182,14 +182,15 @@ static gboolean setup_gobi(struct modem_info *modem)
>   	const char *qmi = NULL, *mdm = NULL, *net = NULL;
>   	const char *gps = NULL, *diag = NULL;
>   	GSList *list;
> +	gboolean telit = FALSE;
>   
>   	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);
> +		DBG("%s %s %s %s %s", info->devnode, info->interface,
> +					info->number, info->label, info->subsystem);

Put this DBG message change in a separate patch.  It's a good idea and I 
did the same thing in my tree, but it's unrelated to the telit support.

>   
>   		if (g_strcmp0(info->interface, "255/255/255") == 0) {
>   			if (info->number == NULL)
> @@ -199,10 +200,18 @@ static gboolean setup_gobi(struct modem_info *modem)
>   			else if (g_strcmp0(info->number, "01") == 0)
>   				diag = info->devnode;
>   			else if (g_strcmp0(info->number, "02") == 0)
> -				mdm = info->devnode;
> +				if (g_strcmp0(info->subsystem, "net") == 0)
> +					net = info->devnode;
> +				else if (g_strcmp0(info->subsystem, "usbmisc") == 0) {
> +					telit = TRUE;
> +					qmi = info->devnode;
> +				} else
> +					mdm = info->devnode;
>   			else if (g_strcmp0(info->number, "03") == 0)
>   				gps = info->devnode;
> -		}
> +		} else if (g_strcmp0(info->interface, "255/0/0") == 0)
> +			if (g_strcmp0(info->number, "04") == 0)
> +				mdm = info->devnode;
>   	}

I need almost exactly the same code for the UC21, but the endpoints are 
different.  What we'll end up with if we do this is an enormous tree of 
cases that all span multiple interfaces, endpoints, and subsystems.  
Because of that, I'm not sure that setup_gobi is the right place to be 
putting this.

The Sierra 7xxx modem does something similar, but uses setup_sierra to 
attach the endpoints and then defers to the gobi driver with:

ofono_modem_set_driver(modem->mode, "gobi"); /* around line 270... in 
setup_sierra */

I suspect that that's a better approach.  I think using setup_gobi makes 
little sense because it doesn't provide a clear association to the 
hardware vendor and the devices from different vendors don't have a 
common configuration.

setup_telit makes some assumptions about endpoint 2 that you might not 
want to muck about with... in my opinion, doing a setup_telit_le910() is 
not horrid, and better than making a mess of setup_gobi.

>   
>   	if (qmi == NULL || mdm == NULL || net == NULL)
> @@ -215,6 +224,12 @@ static gboolean setup_gobi(struct modem_info *modem)
>   	ofono_modem_set_string(modem->modem, "Diag", diag);
>   	ofono_modem_set_string(modem->modem, "NetworkInterface", net);
>   
> +	if (telit) {
> +		/* Telit LE910 V1 modems */
> +		ofono_modem_set_boolean(modem->modem, "ForceSimLegacy", TRUE);
> +		ofono_modem_set_boolean(modem->modem, "AlwaysOnline", TRUE);
> +	}
> +
>   	return TRUE;
>   }
>   
> @@ -1168,6 +1183,7 @@ static struct {
>   	{ "mbm",	"cdc_ether",	"0930"		},
>   	{ "mbm",	"cdc_ncm",	"0930"		},
>   	{ "hso",	"hso"				},
> +	{ "gobi",	"option",	"1bc7", "1201"	},
I don't understand why you are setting "option" here... does the device 
not use qmi_wwan?

/Jonas

Re: [PATCH v3 2/2] udevng: add Telit LE910 V1 support

[Lukasz Nowak]

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

Hi Jonas,

I think you will be happy with PATCH v4 :)

On 24/03/17 15:56, Jonas Bonn wrote:
> Hi Lukasz,
> 
> I'm working on support for the Quectel UC21 and have some similar 
> patches to what you have below.  Based on that experience, here are some 
> comments:
> 
> On 03/24/2017 03:51 PM, Lukasz Nowak wrote:
>> From: Lukasz Nowak <lnowak@tycoint.com>
>>
>> Tested with LE910-SVG and Verizon.
>> ---
>>   plugins/udevng.c | 24 ++++++++++++++++++++----
>>   1 file changed, 20 insertions(+), 4 deletions(-)
>>
>> diff --git a/plugins/udevng.c b/plugins/udevng.c
>> index 2279bbe..3b15064 100644
>> --- a/plugins/udevng.c
>> +++ b/plugins/udevng.c
>> @@ -182,14 +182,15 @@ static gboolean setup_gobi(struct modem_info *modem)
>>   	const char *qmi = NULL, *mdm = NULL, *net = NULL;
>>   	const char *gps = NULL, *diag = NULL;
>>   	GSList *list;
>> +	gboolean telit = FALSE;
>>   
>>   	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);
>> +		DBG("%s %s %s %s %s", info->devnode, info->interface,
>> +					info->number, info->label, info->subsystem);
> 
> Put this DBG message change in a separate patch.  It's a good idea and I 
> did the same thing in my tree, but it's unrelated to the telit support.
> 
>>   
>>   		if (g_strcmp0(info->interface, "255/255/255") == 0) {
>>   			if (info->number == NULL)
>> @@ -199,10 +200,18 @@ static gboolean setup_gobi(struct modem_info *modem)
>>   			else if (g_strcmp0(info->number, "01") == 0)
>>   				diag = info->devnode;
>>   			else if (g_strcmp0(info->number, "02") == 0)
>> -				mdm = info->devnode;
>> +				if (g_strcmp0(info->subsystem, "net") == 0)
>> +					net = info->devnode;
>> +				else if (g_strcmp0(info->subsystem, "usbmisc") == 0) {
>> +					telit = TRUE;
>> +					qmi = info->devnode;
>> +				} else
>> +					mdm = info->devnode;
>>   			else if (g_strcmp0(info->number, "03") == 0)
>>   				gps = info->devnode;
>> -		}
>> +		} else if (g_strcmp0(info->interface, "255/0/0") == 0)
>> +			if (g_strcmp0(info->number, "04") == 0)
>> +				mdm = info->devnode;
>>   	}
> 
> I need almost exactly the same code for the UC21, but the endpoints are 
> different.  What we'll end up with if we do this is an enormous tree of 
> cases that all span multiple interfaces, endpoints, and subsystems.  
> Because of that, I'm not sure that setup_gobi is the right place to be 
> putting this.
> 
> The Sierra 7xxx modem does something similar, but uses setup_sierra to 
> attach the endpoints and then defers to the gobi driver with:
> 
> ofono_modem_set_driver(modem->mode, "gobi"); /* around line 270... in 
> setup_sierra */
> 
> I suspect that that's a better approach.  I think using setup_gobi makes 
> little sense because it doesn't provide a clear association to the 
> hardware vendor and the devices from different vendors don't have a 
> common configuration.
> 
> setup_telit makes some assumptions about endpoint 2 that you might not 
> want to muck about with... in my opinion, doing a setup_telit_le910() is 
> not horrid, and better than making a mess of setup_gobi.

The reason for using setup_gobi() was a problem with udevng, where
cdc-wdm0 would not have vendor_id:product_id. There was no way to separate
setup functions in vendor_list without vid:pid.
This is fixed in v4 - setup_telitqmi() was added.

> 
>>   
>>   	if (qmi == NULL || mdm == NULL || net == NULL)
>> @@ -215,6 +224,12 @@ static gboolean setup_gobi(struct modem_info *modem)
>>   	ofono_modem_set_string(modem->modem, "Diag", diag);
>>   	ofono_modem_set_string(modem->modem, "NetworkInterface", net);
>>   
>> +	if (telit) {
>> +		/* Telit LE910 V1 modems */
>> +		ofono_modem_set_boolean(modem->modem, "ForceSimLegacy", TRUE);
>> +		ofono_modem_set_boolean(modem->modem, "AlwaysOnline", TRUE);
>> +	}
>> +
>>   	return TRUE;
>>   }
>>   
>> @@ -1168,6 +1183,7 @@ static struct {
>>   	{ "mbm",	"cdc_ether",	"0930"		},
>>   	{ "mbm",	"cdc_ncm",	"0930"		},
>>   	{ "hso",	"hso"				},
>> +	{ "gobi",	"option",	"1bc7", "1201"	},
> I don't understand why you are setting "option" here... does the device 
> not use qmi_wwan?

Right, thanks for reminding me about this. setup_gobi() requires a modem device.
I didn't want to touch it as i have not other qmi device to test with.
Telit modem has a parallel AT interface using the option driver, so I just
added it there early on.

But I think it is not used anywhere in gobi, so I will try to remove it from
setup_telitqmi on Monday.

Lukasz


> 
> /Jonas
> 
> _______________________________________________
> ofono mailing list
> ofono(a)ofono.org
> https://lists.ofono.org/mailman/listinfo/ofono
> 

Re: [PATCH v3 2/2] udevng: add Telit LE910 V1 support

[Lukasz Nowak]

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

Hi Jonas,

On 24/03/17 23:07, Lukasz Nowak wrote:
> Hi Jonas,
> 
> I think you will be happy with PATCH v4 :)
> 
> On 24/03/17 15:56, Jonas Bonn wrote:
>> Hi Lukasz,
>>
>> I'm working on support for the Quectel UC21 and have some similar 
>> patches to what you have below.  Based on that experience, here are some 
>> comments:
>>
>> On 03/24/2017 03:51 PM, Lukasz Nowak wrote:
>>> From: Lukasz Nowak <lnowak@tycoint.com>
>>>
>>> Tested with LE910-SVG and Verizon.
>>> ---
>>>   plugins/udevng.c | 24 ++++++++++++++++++++----
>>>   1 file changed, 20 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/plugins/udevng.c b/plugins/udevng.c
>>> index 2279bbe..3b15064 100644
>>> --- a/plugins/udevng.c
>>> +++ b/plugins/udevng.c
>>> @@ -182,14 +182,15 @@ static gboolean setup_gobi(struct modem_info *modem)
>>>   	const char *qmi = NULL, *mdm = NULL, *net = NULL;
>>>   	const char *gps = NULL, *diag = NULL;
>>>   	GSList *list;
>>> +	gboolean telit = FALSE;
>>>   
>>>   	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);
>>> +		DBG("%s %s %s %s %s", info->devnode, info->interface,
>>> +					info->number, info->label, info->subsystem);
>>
>> Put this DBG message change in a separate patch.  It's a good idea and I 
>> did the same thing in my tree, but it's unrelated to the telit support.
>>
>>>   
>>>   		if (g_strcmp0(info->interface, "255/255/255") == 0) {
>>>   			if (info->number == NULL)
>>> @@ -199,10 +200,18 @@ static gboolean setup_gobi(struct modem_info *modem)
>>>   			else if (g_strcmp0(info->number, "01") == 0)
>>>   				diag = info->devnode;
>>>   			else if (g_strcmp0(info->number, "02") == 0)
>>> -				mdm = info->devnode;
>>> +				if (g_strcmp0(info->subsystem, "net") == 0)
>>> +					net = info->devnode;
>>> +				else if (g_strcmp0(info->subsystem, "usbmisc") == 0) {
>>> +					telit = TRUE;
>>> +					qmi = info->devnode;
>>> +				} else
>>> +					mdm = info->devnode;
>>>   			else if (g_strcmp0(info->number, "03") == 0)
>>>   				gps = info->devnode;
>>> -		}
>>> +		} else if (g_strcmp0(info->interface, "255/0/0") == 0)
>>> +			if (g_strcmp0(info->number, "04") == 0)
>>> +				mdm = info->devnode;
>>>   	}
>>
>> I need almost exactly the same code for the UC21, but the endpoints are 
>> different.  What we'll end up with if we do this is an enormous tree of 
>> cases that all span multiple interfaces, endpoints, and subsystems.  
>> Because of that, I'm not sure that setup_gobi is the right place to be 
>> putting this.
>>
>> The Sierra 7xxx modem does something similar, but uses setup_sierra to 
>> attach the endpoints and then defers to the gobi driver with:
>>
>> ofono_modem_set_driver(modem->mode, "gobi"); /* around line 270... in 
>> setup_sierra */
>>
>> I suspect that that's a better approach.  I think using setup_gobi makes 
>> little sense because it doesn't provide a clear association to the 
>> hardware vendor and the devices from different vendors don't have a 
>> common configuration.
>>
>> setup_telit makes some assumptions about endpoint 2 that you might not 
>> want to muck about with... in my opinion, doing a setup_telit_le910() is 
>> not horrid, and better than making a mess of setup_gobi.
> 
> The reason for using setup_gobi() was a problem with udevng, where
> cdc-wdm0 would not have vendor_id:product_id. There was no way to separate
> setup functions in vendor_list without vid:pid.
> This is fixed in v4 - setup_telitqmi() was added.
> 
>>
>>>   
>>>   	if (qmi == NULL || mdm == NULL || net == NULL)
>>> @@ -215,6 +224,12 @@ static gboolean setup_gobi(struct modem_info *modem)
>>>   	ofono_modem_set_string(modem->modem, "Diag", diag);
>>>   	ofono_modem_set_string(modem->modem, "NetworkInterface", net);
>>>   
>>> +	if (telit) {
>>> +		/* Telit LE910 V1 modems */
>>> +		ofono_modem_set_boolean(modem->modem, "ForceSimLegacy", TRUE);
>>> +		ofono_modem_set_boolean(modem->modem, "AlwaysOnline", TRUE);
>>> +	}
>>> +
>>>   	return TRUE;
>>>   }
>>>   
>>> @@ -1168,6 +1183,7 @@ static struct {
>>>   	{ "mbm",	"cdc_ether",	"0930"		},
>>>   	{ "mbm",	"cdc_ncm",	"0930"		},
>>>   	{ "hso",	"hso"				},
>>> +	{ "gobi",	"option",	"1bc7", "1201"	},
>> I don't understand why you are setting "option" here... does the device 
>> not use qmi_wwan?
> 
> Right, thanks for reminding me about this. setup_gobi() requires a modem device.
> I didn't want to touch it as i have not other qmi device to test with.
> Telit modem has a parallel AT interface using the option driver, so I just
> added it there early on.

I remember now why this is required. The Telit modem always creates those option devices alongside qmi_wwan/cdc_wdm, and if I don't list them explicitly mapped as:
{ "telitqmi",	"option",	"1bc7", "1201"	}

then the driver selection will fall back to this:
{ "telit",	"option",	"1bc7"		}

which we don't want.

So the entry in vendor_list[] must stay. I will only remove this:

		} else if (g_strcmp0(info->interface, "255/0/0") == 0)
			if (g_strcmp0(info->number, "04") == 0)
				mdm = info->devnode;
[...]
	ofono_modem_set_string(modem->modem, "Modem", mdm);

as it is not doing anything.

Lukasz

> 
> But I think it is not used anywhere in gobi, so I will try to remove it from
> setup_telitqmi on Monday.
> 
> Lukasz
> 
> 
>>
>> /Jonas
>>
>> _______________________________________________
>> ofono mailing list
>> ofono(a)ofono.org
>> https://lists.ofono.org/mailman/listinfo/ofono
>>

Re: [PATCH v3 2/2] udevng: add Telit LE910 V1 support

[Denis Kenzior]

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

Hi Jonas,

>
> I need almost exactly the same code for the UC21, but the endpoints are
> different.  What we'll end up with if we do this is an enormous tree of
> cases that all span multiple interfaces, endpoints, and subsystems.
> Because of that, I'm not sure that setup_gobi is the right place to be
> putting this.
>
> The Sierra 7xxx modem does something similar, but uses setup_sierra to
> attach the endpoints and then defers to the gobi driver with:
>
> ofono_modem_set_driver(modem->mode, "gobi"); /* around line 270... in
> setup_sierra */
>
> I suspect that that's a better approach.  I think using setup_gobi makes
> little sense because it doesn't provide a clear association to the
> hardware vendor and the devices from different vendors don't have a
> common configuration.

Good that you're taking a second look, the QMI stuff is not my 
specialty.  Marcel was the one who added QMI support, so he can correct 
me if I'm wrong.  But from what I remember the gobi driver was first 
written to support gobi 3000 cards from Dell.  They were not branded so 
the driver was named gobi.  Later, Sierra and Huawei came out with 
almost the same card with slight differences.  Hence setup_sierra and 
setup_huawei use a gobi driver for certain families.  setup_gobi itself 
is probably only relevant to that original dell card.

>
> setup_telit makes some assumptions about endpoint 2 that you might not
> want to muck about with... in my opinion, doing a setup_telit_le910() is
> not horrid, and better than making a mess of setup_gobi.
>

+1 on not using setup_gobi.  How hard would it be to modify setup_telit 
to handle this?

Regards,
-Denis