Re: [PATCH] plugins: droid: migrate to QMI from AT

Hi Denis,

On 25.07.25 г. 1:25 ч., Denis Kenzior wrote:
> Hi Ivaylo,
> 
> On 7/24/25 12:32 AM, Ivaylo Dimitrov wrote:
>> Drop GAtChat usage and move to qmimodem
>> ---
>>   plugins/droid.c  | 487 +++++++++++++++++++++++++++++++++++++++++------
>>   plugins/udevng.c |  30 ++-
>>   2 files changed, 450 insertions(+), 67 deletions(-)
>>
> 
> So the first question that comes to mind is: Why can't the gobi plugin 
> drive this device?  

Now you asked, I am wondering too :). For sure gobi was not working in 
an year or two when I tried it back then.

One thing I see that is different is that this
> plugin uses QMI_DMS_OPER_MODE_PERSIST_LOW_POWER instead of 
> QMI_DMS_OPER_MODE_LOW_POWER.  You also skip / hard code a few things, 
> but that can be special cased by introducing a new attribute or two, 
> similar to 'AlwaysOnline'.
> 

I wonder if QMI_DMS_OPER_MODE_PERSIST_LOW_POWER is not the proper state 
for all QMI modems when disabled. After all we don't want modem to auto 
wake-up. Besides droid, I have one quectel here that is supported by 
gobi, will test with it.

> On the upside, you should might get multi-context support and MTU 
> handling for free.
> 

nice.

Ok, will try to make it work with gobi, if it does not, will re-send the 
current one with the requested fixes, otherwise will send a patch for gobi.


Thanks,
Ivo

> <snip>
> 
>> +static void get_oper_mode_cb(struct qmi_result *result, void *user_data)
>>   {
>>       struct ofono_modem *modem = user_data;
>> +    struct droid_data *data = ofono_modem_get_data(modem);
>> +    struct qmi_param *param;
>> +    uint8_t mode;
>>       DBG("");
>> -    if (ok)
>> +    if (qmi_result_set_error(result, NULL)) {
>> +        shutdown_device(modem);
>> +        return;
>> +    }
>> +
>> +    if (!qmi_result_get_uint8(result, QMI_DMS_RESULT_OPER_MODE, 
>> &mode)) {
>> +        shutdown_device(modem);
>> +        return;
>> +    }
> 
> These shutdown_device(); return; lines might be better served by a 
> goto/label.
> 
>> +
>> +    data->oper_mode = mode;
>> +
>> +    switch (data->oper_mode) {
>> +    case QMI_DMS_OPER_MODE_ONLINE:
>> +        param = qmi_param_new_uint8(QMI_DMS_PARAM_OPER_MODE,
>> +                    QMI_DMS_OPER_MODE_PERSIST_LOW_POWER);
>> +        if (!param) {
>> +            shutdown_device(modem);
>> +            return;
>> +        }
> 
> ditto
> 
>> +
>> +        if (qmi_service_send(data->dms, QMI_DMS_SET_OPER_MODE, param,
>> +                    power_reset_cb, modem, NULL) > 0)
>> +            return;
>> +
>> +        shutdown_device(modem);
>> +        break;
> 
> ditto
> 
>> +    default:
>>           ofono_modem_set_powered(modem, TRUE);
>> +        break;
>> +    }
>>   }
> 
> <snip>
> 
>> -static int droid_enable(struct ofono_modem *modem)
>> +static void get_caps_cb(struct qmi_result *result, void *user_data)
>>   {
>> -    GAtChat *chat;
>> +    struct ofono_modem *modem = user_data;
>> +    struct droid_data *data = ofono_modem_get_data(modem);
>> +    const struct qmi_dms_device_caps *caps;
>> +    uint16_t len;
>> +    uint8_t i;
>>       DBG("");
>> -    chat = at_util_open_device(modem, "Device", droid_debug, "", NULL);
>> -    ofono_modem_set_data(modem, chat);
>> +    if (qmi_result_set_error(result, NULL))
>> +        goto error;
>> -    /* ensure modem is in a known state; verbose on, echo/quiet off */
>> -    g_at_chat_send(chat, "ATE0Q0V1", NULL, NULL, NULL, NULL);
>> +    caps = qmi_result_get(result, QMI_DMS_RESULT_DEVICE_CAPS, &len);
>> +    if (!caps)
>> +        goto error;
>> -    /* power up modem */
>> -    g_at_chat_send(chat, "AT+CFUN=1", NULL, cfun_set_on_cb, modem, 
>> NULL);
>> +    DBG("service capabilities %d", caps->data_capa);
>> +    DBG("sim supported %d", caps->sim_supported);
>> -    return 0;
>> +    for (i = 0; i < caps->radio_if_count; i++)
>> +        DBG("radio = %d", caps->radio_if[i]);
>> +
>> +    if (qmi_service_send(data->dms, QMI_DMS_GET_OPER_MODE, NULL,
>> +                get_oper_mode_cb, modem, NULL) > 0)
>> +        return;
>> +
>> +error:
>> +    shutdown_device(modem);
> 
> Like you do here :)
> 
>>   }
> 
> <snip>
> 
>> -static int droid_disable(struct ofono_modem *modem)
>> +static void create_dms_cb(struct qmi_service *service, void *user_data)
>> +{
>> +    struct ofono_modem *modem = user_data;
>> +    struct droid_data *data = ofono_modem_get_data(modem);
>> +    struct service_request *request = NULL;
>> +
>> +    DBG("");
>> +
>> +    if (!service)
>> +        goto error;
>> +
>> +    data->dms = service;
>> +    request = l_queue_peek_head(data->service_requests);
>> +
>> +    if (qmi_qmux_device_create_client(data->qmux, request->service_type,
>> +                        create_service_cb, modem, NULL))
>> +        return;
>> +
>> +error:
>> +    shutdown_device(modem);
>> +}
> 
> How is this function different from create_service_cb()?  Could you just 
> use that one instead by queuing up a DMS request on the service_requests 
> queue?
> 
> <snip>
> 
>> +
>> +static int droid_disable(struct ofono_modem *modem)
>> +{
>> +    struct droid_data *data = ofono_modem_get_data(modem);
>> +    struct qmi_param *param;
>> +
>> +    DBG("%p", modem);
>> +
>> +    param = qmi_param_new_uint8(QMI_DMS_PARAM_OPER_MODE,
>> +                QMI_DMS_OPER_MODE_PERSIST_LOW_POWER);
>> +    if (!param)
>> +        return -ENOMEM;
>> +
>> +    if (qmi_service_send(data->dms, QMI_DMS_SET_OPER_MODE, param,
>> +                power_disable_cb, modem, NULL) > 0)
>> +        return -EINPROGRESS;
> 
> Leaking param here
> 
>> -    if (sim)
>> -        ofono_sim_inserted_notify(sim, TRUE);
>> +    shutdown_device(modem);
>> +
>> +    return -EINPROGRESS;
>> +}
> 
> <snip>
> 
>> +
>> +/* Only some QMI features are usable, voicecall and sms are custom */
> 
> Not quite sure what this comment means?  I don't see any vendor 
> customization for voicecalls or sms?  Maybe comment is stale?
> 
>> +static void droid_pre_sim(struct ofono_modem *modem)
>> +{
>> +    struct droid_data *data = ofono_modem_get_data(modem);
>> +
>> +    DBG("%p", modem);
>> +
>> +    ofono_devinfo_create(modem, 0, "qmimodem",
>> +                qmi_service_clone(data->dms));
>> +    ofono_sim_create(modem, 0, "qmimodem",
>> +                qmi_service_clone(data->dms),
>> +                qmi_service_clone(data->uim));
>> +    ofono_voicecall_create(modem, 0, "qmimodem",
>> +                qmi_service_clone(data->voice));
>> +    ofono_location_reporting_create(modem, 0, "qmimodem",
>> +                    l_steal_ptr(data->pds));
>>   }
>>   static void droid_post_sim(struct ofono_modem *modem)
>>   {
>> -    GAtChat *chat = ofono_modem_get_data(modem);
>> +    struct droid_data *data = ofono_modem_get_data(modem);
>>       struct ofono_message_waiting *mw;
>> +    struct ofono_gprs *gprs;
>> +    struct ofono_gprs_context *gc;
>> +    const char *interface;
>> -    DBG("");
>> +    DBG("%p", modem);
>> +
>> +/*    ofono_phonebook_create(modem, 0, "qmimodem", data->qmux);*/
> 
> Remove?
> 
>> +    ofono_radio_settings_create(modem, 0, "qmimodem",
>> +                    qmi_service_clone(data->dms),
>> +                    qmi_service_clone(data->nas));
>> -    ofono_ussd_create(modem, 0, "atmodem", chat);
>> -    ofono_call_forwarding_create(modem, 0, "atmodem", chat);
>> -    ofono_call_settings_create(modem, 0, "atmodem", chat);
>> -    ofono_netreg_create(modem, 0, "atmodem", chat);
>> -    /*
>> -     * Droid 4 modem has problems with AT+CPUC?, avoid call meter for 
>> now.
>> -     */
>> -    ofono_call_barring_create(modem, 0, "atmodem", chat);
>> -    ofono_sms_create(modem, OFONO_VENDOR_DROID, "atmodem", chat);
>> -    ofono_phonebook_create(modem, 0, "atmodem", chat);
>> +    ofono_sms_create(modem, 0, "qmimodem",
>> +                qmi_service_clone(data->wms));
>>       mw = ofono_message_waiting_create(modem);
>>       if (mw)
>>           ofono_message_waiting_register(mw);
>> +
>> +    gprs = ofono_gprs_create(modem, 0, "qmimodem",
>> +                    qmi_service_clone(data->wds),
>> +                    qmi_service_clone(data->nas));
>> +    if (!gprs) {
>> +        ofono_warn("Unable to create gprs for: %s",
>> +                ofono_modem_get_path(modem));
>> +        return;
>> +    }
>> +
>> +    gc = ofono_gprs_context_create(modem, 0, "qmimodem", -1,
>> +                    qmi_service_clone(data->wds_ip4),
>> +                    qmi_service_clone(data->wds_ip6));
>> +    if (!gc) {
>> +        ofono_warn("Unable to create gprs-context for: %s",
>> +                ofono_modem_get_path(modem));
>> +        return;
>> +    }
>> +
>> +    ofono_gprs_add_context(gprs, gc);
>> +    interface = ofono_modem_get_string(modem, "NetworkInterface");
>> +    ofono_gprs_context_set_interface(gc, interface);
>> +}
>> +
> 
> <snip>
> 
>> diff --git a/plugins/udevng.c b/plugins/udevng.c
>> index 320cd3a6..7777996d 100644
>> --- a/plugins/udevng.c
>> +++ b/plugins/udevng.c
>> @@ -870,7 +870,9 @@ static gboolean setup_telitqmi(struct modem_info 
>> *modem)
>>   static gboolean setup_droid(struct modem_info *modem)
>>   {
>> -    const char *at = NULL;
>> +    const struct device_info *qmi = NULL;
>> +    const struct device_info *net = NULL;
>> +
>>       GSList *list;
>>       DBG("%s", modem->syspath);
>> @@ -878,21 +880,29 @@ static gboolean setup_droid(struct modem_info 
>> *modem)
>>       for (list = modem->devices; list; list = list->next) {
>>           struct device_info *info = list->data;
>>           const char *subsystem =
>> -            udev_device_get_subsystem(info->udev_device);
>> -
>> -        DBG("%s %s %s %s %s", info->devnode, info->interface,
>> -                info->number, info->label, subsystem);
>> +                udev_device_get_subsystem(info->udev_device);
>> +        DBG("%s %s %s %s %s %s", info->devnode, info->interface,
>> +                        info->number, info->label,
>> +                        info->sysattr, subsystem);
>> -        if (g_strcmp0(info->interface, "255/255/255") == 0 &&
>> -                g_strcmp0(info->number, "04") == 0) {
>> -            at = info->devnode;
>> +        if (g_strcmp0(subsystem, "usbmisc") == 0) {
>> +            if (g_strcmp0(info->number, "05") == 0)
>> +                qmi = info;
>> +        } else if (g_strcmp0(subsystem, "net") == 0) {
>> +            if (g_strcmp0(info->number, "05") == 0)
>> +                net = info;
>>           }
>>       }
>> -    if (at == NULL)
>> +    if (qmi == NULL || net == NULL)
>> +        return FALSE;
>> +
>> +    DBG("qmi=%s net=%s", qmi->devnode, get_ifname(net));
>> +
>> +
> 
> No double empty lines please
> 
>> +    if (setup_qmi_qmux(modem, qmi, net) < 0)
>>           return FALSE;
>> -    ofono_modem_set_string(modem->modem, "Device", at);
>>       ofono_modem_set_driver(modem->modem, "droid");
>>       return TRUE;
> 
> Regards,
> -Denis