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

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?  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'.

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

<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