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