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

[Jonas Bonn <jonas@southpole.se>]

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