* [PATCH v3 0/2] Telit LE910 V1 QMI support @ 2017-03-24 14:51 Lukasz Nowak 2017-03-24 14:51 ` [PATCH v3 1/2] gobi: Do not use low-power modes for some modems Lukasz Nowak ` (2 more replies) 0 siblings, 3 replies; 10+ messages in thread From: Lukasz Nowak @ 2017-03-24 14:51 UTC (permalink / raw) To: ofono [-- 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v3 1/2] gobi: Do not use low-power modes for some modems 2017-03-24 14:51 [PATCH v3 0/2] Telit LE910 V1 QMI support Lukasz Nowak @ 2017-03-24 14:51 ` Lukasz Nowak 2017-03-24 15:08 ` Denis Kenzior 2017-03-24 14:51 ` [PATCH v3 2/2] udevng: add Telit LE910 V1 support Lukasz Nowak 2017-03-24 15:11 ` [PATCH v3 0/2] Telit LE910 V1 QMI support Denis Kenzior 2 siblings, 1 reply; 10+ messages in thread From: Lukasz Nowak @ 2017-03-24 14:51 UTC (permalink / raw) To: ofono [-- 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v3 1/2] gobi: Do not use low-power modes for some modems 2017-03-24 14:51 ` [PATCH v3 1/2] gobi: Do not use low-power modes for some modems Lukasz Nowak @ 2017-03-24 15:08 ` Denis Kenzior 0 siblings, 0 replies; 10+ messages in thread From: Denis Kenzior @ 2017-03-24 15:08 UTC (permalink / raw) To: ofono [-- 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v3 2/2] udevng: add Telit LE910 V1 support 2017-03-24 14:51 [PATCH v3 0/2] Telit LE910 V1 QMI support Lukasz Nowak 2017-03-24 14:51 ` [PATCH v3 1/2] gobi: Do not use low-power modes for some modems Lukasz Nowak @ 2017-03-24 14:51 ` Lukasz Nowak 2017-03-24 15:16 ` Denis Kenzior 2017-03-24 15:56 ` Jonas Bonn 2017-03-24 15:11 ` [PATCH v3 0/2] Telit LE910 V1 QMI support Denis Kenzior 2 siblings, 2 replies; 10+ messages in thread From: Lukasz Nowak @ 2017-03-24 14:51 UTC (permalink / raw) To: ofono [-- 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v3 2/2] udevng: add Telit LE910 V1 support 2017-03-24 14:51 ` [PATCH v3 2/2] udevng: add Telit LE910 V1 support Lukasz Nowak @ 2017-03-24 15:16 ` Denis Kenzior 2017-03-24 15:56 ` Jonas Bonn 1 sibling, 0 replies; 10+ messages in thread From: Denis Kenzior @ 2017-03-24 15:16 UTC (permalink / raw) To: ofono [-- 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3 2/2] udevng: add Telit LE910 V1 support 2017-03-24 14:51 ` [PATCH v3 2/2] udevng: add Telit LE910 V1 support Lukasz Nowak 2017-03-24 15:16 ` Denis Kenzior @ 2017-03-24 15:56 ` Jonas Bonn 2017-03-24 23:07 ` Lukasz Nowak 2017-03-25 20:59 ` Denis Kenzior 1 sibling, 2 replies; 10+ messages in thread From: Jonas Bonn @ 2017-03-24 15:56 UTC (permalink / raw) To: ofono [-- 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3 2/2] udevng: add Telit LE910 V1 support 2017-03-24 15:56 ` Jonas Bonn @ 2017-03-24 23:07 ` Lukasz Nowak 2017-03-25 11:15 ` Lukasz Nowak 2017-03-25 20:59 ` Denis Kenzior 1 sibling, 1 reply; 10+ messages in thread From: Lukasz Nowak @ 2017-03-24 23:07 UTC (permalink / raw) To: ofono [-- 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 > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3 2/2] udevng: add Telit LE910 V1 support 2017-03-24 23:07 ` Lukasz Nowak @ 2017-03-25 11:15 ` Lukasz Nowak 0 siblings, 0 replies; 10+ messages in thread From: Lukasz Nowak @ 2017-03-25 11:15 UTC (permalink / raw) To: ofono [-- 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 >> ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3 2/2] udevng: add Telit LE910 V1 support 2017-03-24 15:56 ` Jonas Bonn 2017-03-24 23:07 ` Lukasz Nowak @ 2017-03-25 20:59 ` Denis Kenzior 1 sibling, 0 replies; 10+ messages in thread From: Denis Kenzior @ 2017-03-25 20:59 UTC (permalink / raw) To: ofono [-- 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3 0/2] Telit LE910 V1 QMI support 2017-03-24 14:51 [PATCH v3 0/2] Telit LE910 V1 QMI support Lukasz Nowak 2017-03-24 14:51 ` [PATCH v3 1/2] gobi: Do not use low-power modes for some modems Lukasz Nowak 2017-03-24 14:51 ` [PATCH v3 2/2] udevng: add Telit LE910 V1 support Lukasz Nowak @ 2017-03-24 15:11 ` Denis Kenzior 2 siblings, 0 replies; 10+ messages in thread From: Denis Kenzior @ 2017-03-24 15:11 UTC (permalink / raw) To: ofono [-- 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2017-03-25 20:59 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-03-24 14:51 [PATCH v3 0/2] Telit LE910 V1 QMI support Lukasz Nowak 2017-03-24 14:51 ` [PATCH v3 1/2] gobi: Do not use low-power modes for some modems Lukasz Nowak 2017-03-24 15:08 ` Denis Kenzior 2017-03-24 14:51 ` [PATCH v3 2/2] udevng: add Telit LE910 V1 support Lukasz Nowak 2017-03-24 15:16 ` Denis Kenzior 2017-03-24 15:56 ` Jonas Bonn 2017-03-24 23:07 ` Lukasz Nowak 2017-03-25 11:15 ` Lukasz Nowak 2017-03-25 20:59 ` Denis Kenzior 2017-03-24 15:11 ` [PATCH v3 0/2] Telit LE910 V1 QMI support Denis Kenzior
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.