Re: [PATCH v2 1/4] [qmimodem][voicecall] Implement call dialing

[Denis Kenzior <denkenz@gmail.com>]

Hi Adam,

On 3/26/24 05:19, Adam Pigg wrote:
> Add voicecall dialling to the qmimodem driver
> Includes required infratructure and setup of the QMI services
> 
> Call State Handling
> ===================
> On initialisation, register the all_call_status_ind callback to be
> called for QMI_VOICE_IND_ALL_STATUS.  This will handle notificatiof of
> call status to the rest of the system
> 
> Dial Handling
> =============
> The dial function sets up the parameters for the QMI_VOICE_DIAL_CALL
> service.  The parameters are the number to be called and the call type,
> which is currently hard coded to be QMI_VOICE_CALL_TYPE_VOICE.  The
> dial_cb callback wi then be called and will receive the call-id.
> ---
>   drivers/qmimodem/voice.h     |  51 ++++
>   drivers/qmimodem/voicecall.c | 503 ++++++++++++++++++++++++++++++++++-
>   2 files changed, 553 insertions(+), 1 deletion(-)

Looking better! I have a bunch of feedback, but it is not exhaustive, so just be 
prepared for more rounds in the future :)

First minor thing is the commit naming conventions, oFono usually uses the 
following:
<modem driver>:<atom>: <description> or
<modem driver>: <description>

So in your case:

qmimodem: voicecall: Implement call dialing ... or
qmi: voice: Implement call dialing ... or
qmimodem: Implement call dialing

I find all three acceptable.

Another thing, please fix your editor not to indent to opening brace or mix tabs 
and spaces.  We only use tabs for indentation.  I'll point out a few examples later.

Your submission doesn't compile without warnings, so that's another thing to 
address for future submissions:

     drivers/qmimodem/voicecall.c:135:5: error: no previous declaration for 
'ofono_call_compare' [-Werror=missing-declarations]
       135 | int ofono_call_compare(const void *a, const void *b, void *data)
           |     ^~~~~~~~~~~~~~~~~~
     drivers/qmimodem/voicecall.c:149:6: error: no previous declaration for 
'ofono_call_compare_by_status' [-Werror=missing-declarations]
       149 | bool ofono_call_compare_by_status(const void *a, const void *b)
           |      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
     drivers/qmimodem/voicecall.c:157:6: error: no previous declaration for 
'ofono_call_compare_by_id' [-Werror=missing-declarations]
       157 | bool ofono_call_compare_by_id(const void *a, const void *b)
           |      ^~~~~~~~~~~~~~~~~~~~~~~~
     drivers/qmimodem/voicecall.c:165:6: error: no previous declaration for 
'ofono_call_list_dial_callback' [-Werror=missing-declarations]
       165 | void ofono_call_list_dial_callback(struct ofono_voicecall *vc,
           |      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
     drivers/qmimodem/voicecall.c:193:6: error: no previous declaration for 
'ofono_call_list_notify' [-Werror=missing-declarations]
       193 | void ofono_call_list_notify(struct ofono_voicecall *vc,
           |      ^~~~~~~~~~~~~~~~~~~~~~
     drivers/qmimodem/voicecall.c:252:13: error: no previous declaration for 
'qmi_voice_call_state_name' [-Werror=missing-declarations]
       252 | const char *qmi_voice_call_state_name(enum qmi_voice_call_state value)
           |             ^~~~~~~~~~~~~~~~~~~~~~~~~
     drivers/qmimodem/voicecall.c:270:5: error: no previous declaration for 
'qmi_to_ofono_status' [-Werror=missing-declarations]
       270 | int qmi_to_ofono_status(uint8_t status, int *ret)
           |     ^~~~~~~~~~~~~~~~~~~
     drivers/qmimodem/voicecall.c:308:9: error: no previous declaration for 
'ofono_to_qmi_direction' [-Werror=missing-declarations]
       308 | uint8_t ofono_to_qmi_direction(enum call_direction ofono_direction)
           |         ^~~~~~~~~~~~~~~~~~~~~~
     drivers/qmimodem/voicecall.c:312:21: error: no previous declaration for 
'qmi_to_ofono_direction' [-Werror=missing-declarations]
       312 | enum call_direction qmi_to_ofono_direction(uint8_t qmi_direction)
           |                     ^~~~~~~~~~~~~~~~~~~~~~
     drivers/qmimodem/voicecall.c:472:1: error: no previous declaration for 
'qmi_voice_dial_call_parse' [-Werror=missing-declarations]
       472 | qmi_voice_dial_call_parse(struct qmi_result *qmi_result,
           | ^~~~~~~~~~~~~~~~~~~~~~~~~
     drivers/qmimodem/voicecall.c:571:18: error: no previous declaration for 
'qmi_voice_answer_call_parse' [-Werror=missing-declarations]
       571 | enum parse_error qmi_voice_answer_call_parse(
           |                  ^~~~~~~~~~~~~~~~~~~~~~~~~~~
     drivers/qmimodem/voicecall.c:660:1: error: no previous declaration for 
'qmi_voice_end_call_parse' [-Werror=missing-declarations]
       660 | qmi_voice_end_call_parse(struct qmi_result *qmi_result,
           | ^~~~~~~~~~~~~~~~~~~~~~~~
     drivers/qmimodem/voicecall.c: In function 'hangup_active':
     drivers/qmimodem/voicecall.c:743:23: error: comparison of integer 
expressions of different signedness: 'int' and 'long unsigned int' 
[-Werror=sign-compare]
       743 |         for (i = 0; i < L_ARRAY_SIZE(active); i++) {
           |                       ^

CI also reported some issues:

0001-Implement-call-dialing.patch:154: WARNING: externs should be avoided in .c 
files
0001-Implement-call-dialing.patch:165: WARNING: 'als' may be misspelled - 
perhaps 'also'?
0001-Implement-call-dialing.patch:238: WARNING: braces {} are not necessary for 
single statement blocks
0001-Implement-call-dialing.patch:292: WARNING: braces {} are not necessary for 
single statement blocks
0001-Implement-call-dialing.patch:310: WARNING: Macros with flow control 
statements should be avoided
0001-Implement-call-dialing.patch:437: WARNING: braces {} are not necessary for 
any arm of this statement
0001-Implement-call-dialing.patch:482: WARNING: 'informations' may be misspelled 
- perhaps 'information'?

Now, lets get to the review:

> 
> diff --git a/drivers/qmimodem/voice.h b/drivers/qmimodem/voice.h
> index 917e72f7..1a584f10 100644
> --- a/drivers/qmimodem/voice.h
> +++ b/drivers/qmimodem/voice.h
> @@ -15,6 +15,8 @@
>    *
>    */
>   
> +#include <stdint.h>
> +

This probably belongs in drivers/qmimodem/voicecall.c itself.  Rule of thumb is 
that internal headers such as wds.h, wms.h, voice.h etc should not have any 
system includes.

>   #define QMI_VOICE_PARAM_USS_DATA 0x01
>   
>   #define QMI_VOICE_PARAM_ASYNC_USSD_ERROR 0x10
> @@ -25,6 +27,9 @@
>   #define QMI_VOICE_PARAM_USSD_IND_DATA 0x10
>   #define QMI_VOICE_PARAM_USSD_IND_UCS2 0x11
>   
> +#define QMI_VOICE_IND_ALL_STATUS 0x2e
> +#define QMI_VOICE_GET_ALL_STATUS 0x2f

Probably belongs in voice_commands enum below.

> +
>   /* according to GSM TS 23.038 section 5
>    * coding group 1111, No message class, 8 bit data
>    */

<snip>

> +
> +enum qmi_voice_call_dial_param {
> +	QMI_VOICE_DIAL_CALL_NUMBER = 0x01,
> +	QMI_VOICE_DIAL_CALL_TYPE = 0x10

The other files inside qmimodem/ use #defines for this.  For example:

#define QMI_DMS_PARAM_REPORT_PIN_STATUS         0x12    /* bool */

Similarly, all RESULT TLVs have '_RESULT_' in the #define name.

Sticking to that convention would be more preferred.

An even more preferred approach would be to avoid definiting enumerations in the 
header file completely.  Typically the enumeration is only used in a single 
function, i.e. the parser or builder for the reqesult/request.  In that case, 
use a static const variable in the function itself.  See
drivers/qmimodem/network-registration.c, set_event_report_cb() for an example. 
The advantage of the above approach is that you have all relevant definitions 
very close to the actual use location and are not tripping over sentence sized 
enumeration names.

> +};
> +
> +enum qmi_voice_call_dial_return {
> +	QMI_VOICE_DIAL_RETURN_CALL_ID = 0x10
> +};
> +
> +enum qmi_voice_all_call_status_commands {

This enum is for the TLV types included in the result, no?  If so, see above 
about naming.

> +	QMI_VOICE_ALL_CALL_STATUS_CALL_INFORMATION = 0x01,
> +	QMI_VOICE_ALL_CALL_STATUS_REMOTE_NUMBER = 0x10
> +};
> +
> +enum qmi_voice_all_call_info_commands {
> +	QMI_VOICE_ALL_CALL_INFO_CALL_INFORMATION = 0x10,
> +	QMI_VOICE_ALL_CALL_INFO_REMOTE_NUMBER = 0x11
> +};
> +
> +enum qmi_voice_call_type {
> +	QMI_VOICE_CALL_TYPE_VOICE = 0x0,
> +	QMI_VOICE_CALL_TYPE_VOICE_FORCE,
> +};
> +
> +enum parse_error {
> +	NONE = 0,
> +	MISSING_MANDATORY = 1,
> +	INVALID_LENGTH = 2,
> +};
> +

Use errno.h for this:
err = 0 -> ok
err = -EINVAL or -EBADMSG -> missing mandatory
err = -EMSGSIZE -> invalid length

>   struct qmi_ussd_data {
>   	uint8_t dcs;
>   	uint8_t length;
> @@ -98,3 +148,4 @@ enum qmi_ss_reason {
>   	QMI_VOICE_SS_RSN_CLIP =			0x10,
>   	QMI_VOICE_SS_RSN_CLIR =			0x11
>   };
> +

No empty lines at EOF please, my git settings refuse to apply it.

<snip>

> @@ -35,8 +40,499 @@ struct voicecall_data {
>   	struct qmi_service *voice;
>   	uint16_t major;
>   	uint16_t minor;
> +	struct l_queue *call_list;
> +	struct voicecall_static *vs;

This member isn't used?

> +	struct ofono_phone_number dialed;
> +};
> +
> +struct qmi_voice_all_call_status_ind {
> +	bool call_information_set;
> +	const struct qmi_voice_call_information *call_information;
> +	bool remote_party_number_set;
> +	uint8_t remote_party_number_size;
> +	const struct qmi_voice_remote_party_number_instance
> +		*remote_party_number[16];
> +};
> +
> +enum parse_error
> +qmi_voice_call_status(struct qmi_result *qmi_result,
> +		      struct qmi_voice_all_call_status_ind *result);

This function isn't exported, so this declaration isn't necessary.

> +
> +struct qmi_voice_call_information_instance {
> +	uint8_t id;
> +	uint8_t state;
> +	uint8_t type;
> +	uint8_t direction;
> +	uint8_t mode;
> +	uint8_t multipart_indicator;
> +	uint8_t als;
> +} __attribute__((__packed__));
> +
> +struct qmi_voice_call_information {
> +	uint8_t size;
> +	struct qmi_voice_call_information_instance instance[0];
> +} __attribute__((__packed__));
> +
> +struct qmi_voice_remote_party_number_instance {
> +	uint8_t call_id;
> +	uint8_t presentation_indicator;
> +	uint8_t number_size;
> +	char number[0];
> +} __attribute__((__packed__));
> +
> +struct qmi_voice_remote_party_number {
> +	uint8_t size;
> +	struct qmi_voice_remote_party_number_instance instance[0];
> +} __attribute__((__packed__));
> +
> +struct qmi_voice_dial_call_arg {
> +	bool calling_number_set;
> +	const char *calling_number;
> +	bool call_type_set;
> +	uint8_t call_type;
> +};

This structure is not needed, get rid of it.

> +
> +struct qmi_voice_dial_call_result {
> +	bool call_id_set;
> +	uint8_t call_id;
>   };
>   
> +int ofono_call_compare(const void *a, const void *b, void *data)

static?

> +{
> +	const struct ofono_call *ca = a;
> +	const struct ofono_call *cb = b;
> +
> +	if (ca->id < cb->id)
> +		return -1;
> +
> +	if (ca->id > cb->id)
> +		return 1;
> +
> +	return 0;
> +}
> +
> +bool ofono_call_compare_by_status(const void *a, const void *b)

static ?

> +{
> +	const struct ofono_call *call = a;
> +	int status = L_PTR_TO_INT(b);
> +
> +	return status == call->status;
> +}
> +
> +bool ofono_call_compare_by_id(const void *a, const void *b)

static?

> +{
> +	const struct ofono_call *call = a;
> +	unsigned int id = L_PTR_TO_UINT(b);
> +
> +	return (call->id == id);
> +}
> +
> +void ofono_call_list_dial_callback(struct ofono_voicecall *vc,

static void..

> +				   struct l_queue **call_list,

A single pointer is enough...

> +				   const struct ofono_phone_number *ph,
> +				   int call_id)
> +{
> +	struct ofono_call *call;
> +
> +	/* check if call_id already present */
> +	call = l_queue_find(*call_list, ofono_call_compare_by_id,
> +			    L_UINT_TO_PTR(call_id));
> +
> +	if (call) {
> +		return;
> +	}

See the CI complaining about this.  Single statement blocks don't need {}

> +
> +	call = l_new(struct ofono_call, 1);
> +	call->id = call_id;
> +
> +	memcpy(&call->called_number, ph, sizeof(*ph));
> +	call->direction = CALL_DIRECTION_MOBILE_ORIGINATED;
> +	call->status = CALL_STATUS_DIALING;
> +	call->type = 0; /* voice */
> +
> +	l_queue_insert(*call_list, call, ofono_call_compare, NULL);
> +
> +	ofono_voicecall_notify(vc, call);
> +}
> +
> +void ofono_call_list_notify(struct ofono_voicecall *vc,
> +			    struct l_queue **call_list, struct l_queue *calls) > +{
> +	struct l_queue *old_calls = *call_list;
> +	struct l_queue *new_calls = calls;
> +	struct ofono_call *new_call, *old_call;
> +	const struct l_queue_entry *old_entry, *new_entry;
> +
> +	uint loop_length =
> +		MAX(l_queue_length(old_calls), l_queue_length(new_calls));
> +
> +	old_entry = l_queue_get_entries(old_calls);
> +	new_entry = l_queue_get_entries(new_calls);
> +
> +	for (uint i = 0; i < loop_length; ++i) {
> +		old_call = old_entry ? old_entry->data : NULL;
> +		new_call = new_entry ? new_entry->data : NULL;
> +
> +		if (new_call && new_call->status == CALL_STATUS_DISCONNECTED) {
> +			ofono_voicecall_disconnected(
> +				vc, new_call->id,
> +				OFONO_DISCONNECT_REASON_REMOTE_HANGUP, NULL);
> +
> +			l_queue_remove(calls, new_call);
> +			l_free(new_call);
> +			continue;
> +		}
> +
> +		if (old_call &&
> +		    (new_call == NULL || (new_call->id > old_call->id))) {
> +			ofono_voicecall_disconnected(
> +				vc, old_call->id,
> +				OFONO_DISCONNECT_REASON_REMOTE_HANGUP, NULL);
> +		} else if (new_call && (old_call == NULL ||
> +					(new_call->id < old_call->id))) {
> +			DBG("Notify new call %d", new_call->id);
> +			/* new call, signal it */
> +			if (new_call->type == 0) {
> +				ofono_voicecall_notify(vc, new_call);
> +			}

No need for {}

> +		} else {
> +			if (memcmp(new_call, old_call, sizeof(*new_call)) &&
> +			    new_call->type == 0)

Please fix your editor, oFono uses tabs for indentation

> +				ofono_voicecall_notify(vc, new_call);
> +		}
> +		if (old_entry)
> +			old_entry = old_entry->next;
> +		if (new_entry)
> +			new_entry = new_entry->next;
> +	}
> +
> +	l_queue_destroy(*call_list, l_free);
> +	*call_list = calls;
> +}
> +

<snip>

> +int qmi_to_ofono_status(uint8_t status, int *ret)

There are two acceptable ways to return success/error status from functions:

bool -> false on error, true on success
int -> 0 is success, -ERRNO (-EINVAL) on error.

I suspect using bool here would be better

> +{
> +	int err = 0;
> +
> +	switch (status) {
> +	case QMI_VOICE_CALL_STATE_IDLE:
> +	case QMI_VOICE_CALL_STATE_END:
> +	case QMI_VOICE_CALL_STATE_DISCONNECTING:
> +		*ret = CALL_STATUS_DISCONNECTED;
> +		break;
> +	case QMI_VOICE_CALL_STATE_HOLD:
> +		*ret = CALL_STATUS_HELD;
> +		break;
> +	case QMI_VOICE_CALL_STATE_WAITING:
> +		*ret = CALL_STATUS_WAITING;
> +		break;
> +	case QMI_VOICE_CALL_STATE_ORIG:
> +		*ret = CALL_STATUS_DIALING;
> +		break;
> +	case QMI_VOICE_CALL_STATE_SETUP:
> +	case QMI_VOICE_CALL_STATE_INCOMING:
> +		*ret = CALL_STATUS_INCOMING;
> +		break;
> +	case QMI_VOICE_CALL_STATE_CONV:
> +		*ret = CALL_STATUS_ACTIVE;
> +		break;
> +	case QMI_VOICE_CALL_STATE_CC_IN_PROG:
> +		*ret = CALL_STATUS_DIALING;
> +		break;
> +	case QMI_VOICE_CALL_STATE_ALERTING:
> +		*ret = CALL_STATUS_ALERTING;
> +		break;
> +	default:
> +		err = 1;
> +	}
> +	return err;
> +}
> +

<snip>

> +enum parse_error
> +qmi_voice_call_status(struct qmi_result *qmi_result,
> +		      struct qmi_voice_all_call_status_ind *result)

Similar to the above comment, use an int return here

> +{
> +	int err = NONE;
> +	int offset;
> +	uint16_t len;
> +	bool status = true;
> +	const struct qmi_voice_remote_party_number *remote_party_number;
> +	const struct qmi_voice_call_information *call_information;
> +
> +	/* mandatory */
> +	call_information = qmi_result_get(
> +		qmi_result, QMI_VOICE_ALL_CALL_STATUS_CALL_INFORMATION, &len);
> +
> +	/* This is so ugly! but TLV for indicator and response is different */
> +	if (!call_information) {
> +		call_information = qmi_result_get(
> +			qmi_result, QMI_VOICE_ALL_CALL_INFO_CALL_INFORMATION,
> +			&len);
> +		status = false;
> +	}
> +
> +	if (call_information) {
> +		/* verify the length */
> +		if (len < sizeof(call_information->size))
> +			return INVALID_LENGTH;
> +
> +		if (len != call_information->size *
> +			sizeof(struct qmi_voice_call_information_instance) +
> +			sizeof(call_information->size)) {
> +			return INVALID_LENGTH;
> +		}
> +		result->call_information_set = 1;
> +		result->call_information = call_information;
> +	} else
> +		return MISSING_MANDATORY;
> +
> +	/* mandatory */
> +	remote_party_number = qmi_result_get(
> +		qmi_result,
> +		status ? QMI_VOICE_ALL_CALL_STATUS_REMOTE_NUMBER :
> +			 QMI_VOICE_ALL_CALL_INFO_REMOTE_NUMBER,
> +		&len);
> +
> +	if (remote_party_number) {
> +		const struct qmi_voice_remote_party_number_instance *instance;
> +		int instance_size =
> +			sizeof(struct qmi_voice_remote_party_number_instance);
> +		int i;
> +
> +		/* verify the length */
> +		if (len < sizeof(remote_party_number->size))
> +			return INVALID_LENGTH;
> +
> +		for (i = 0, offset = sizeof(remote_party_number->size);
> +		     offset <= len && i < 16 && i < remote_party_number->size;
> +		     i++) {
> +			if (offset == len) {
> +				break;
> +			} else if (offset + instance_size > len) {
> +				return INVALID_LENGTH;
> +			}
> +
> +			instance = (void *)remote_party_number + offset;
> +			result->remote_party_number[i] = instance;
> +			offset +=
> +				sizeof(struct qmi_voice_remote_party_number_instance) +
> +				instance->number_size;
> +		}
> +		result->remote_party_number_set = 1;
> +		result->remote_party_number_size = remote_party_number->size;
> +	} else
> +		return MISSING_MANDATORY;
> +
> +	return err;
> +}
> +
> +static void all_call_status_ind(struct qmi_result *result, void *user_data)
> +{
> +	struct ofono_voicecall *vc = user_data;
> +	struct voicecall_data *vd = ofono_voicecall_get_data(vc);
> +	struct l_queue *calls = NULL;
> +	int i;
> +	int size = 0;
> +	struct qmi_voice_all_call_status_ind status_ind;
> +
> +	calls = l_queue_new();

Might as well move initialization up to the definition.

> +
> +	DBG("");
> +	if (qmi_voice_call_status(result, &status_ind) != NONE) {
> +		DBG("Parsing of all call status indication failed");
> +		return;

You're leaking memory allocated to calls here.  If the queue contents are to be 
freed with l_free, you can do something like:

static void queue_destroy_free(void *p)
{
	struct l_queue *q = *(void **) p;
	l_queue_destroy(q, l_free);
}

_auto_(queue_desroy_free) struct l_queue *calls = l_queue_new() instead.

> +	}
> +
> +	if (!status_ind.remote_party_number_set ||
> +	    !status_ind.call_information_set) {

Another case of your editor indenting incorrectly

> +		DBG("Some required fields are not set");
> +		return;
> +	}
> +
> +	size = status_ind.call_information->size;
> +	if (!size) {
> +		DBG("No call informations received!");
> +		return;
> +	}
> +
> +	/* expect we have valid fields for every call */
> +	if (size != status_ind.remote_party_number_size) {
> +		DBG("Not all fields have the same size");
> +		return;
> +	}
> +
> +	for (i = 0; i < size; i++) {
> +		struct qmi_voice_call_information_instance call_info;
> +		struct ofono_call *call;
> +		const struct qmi_voice_remote_party_number_instance
> +			*remote_party = status_ind.remote_party_number[i];
> +		int number_size;
> +
> +		call_info = status_ind.call_information->instance[i];
> +		call = l_new(struct ofono_call, 1);
> +		call->id = call_info.id;
> +		call->direction = qmi_to_ofono_direction(call_info.direction);
> +
> +		if (qmi_to_ofono_status(call_info.state, &call->status)) {
> +			DBG("Ignore call id %d, because can not convert QMI state 0x%x to ofono.",
> +			    call_info.id, call_info.state);
> +			continue;

Leaking call here?

> +		}
> +		DBG("Call %d in state %s(%d)", call_info.id,
> +		    qmi_voice_call_state_name(call_info.state),
> +		    call_info.state);
> +
> +		call->type = 0; /* always voice */
> +		number_size = remote_party->number_size;
> +		if (number_size > OFONO_MAX_PHONE_NUMBER_LENGTH)
> +			number_size = OFONO_MAX_PHONE_NUMBER_LENGTH;
> +		strncpy(call->phone_number.number, remote_party->number,
> +			number_size);

Use l_strlcpy instead of strncpy.  Otherwise you must take care of null 
terminating.  Refer to man strncpy to understand why.

> +		/* FIXME: set phone_number_type */
> +
> +		if (strlen(call->phone_number.number) > 0)
> +			call->clip_validity = 0;
> +		else
> +			call->clip_validity = 2;
> +
> +		l_queue_push_tail(calls, call);
> +		DBG("%d", l_queue_length(calls));
> +	}
> +
> +	ofono_call_list_notify(vc, &vd->call_list, calls);

Leaking calls here even on the success path...

> +}
> +
> +enum parse_errorstatic int qmi_voice_dial..
> +qmi_voice_dial_call_parse(struct qmi_result *qmi_result,
> +			  struct qmi_voice_dial_call_result *result)
This structure and the corresponding definition seem useless if they're only 
used between dial_cb and this parser helper.  Inlining the contents inside 
dial_cb would seem to be cleaner.
> +{
> +	int err = NONE;
> +
> +	/* mandatory */
> +	if (qmi_result_get_uint8(qmi_result, QMI_VOICE_DIAL_RETURN_CALL_ID,
> +				 &result->call_id))
> +		result->call_id_set = 1;
> +	else
> +		err = MISSING_MANDATORY;
> +
> +	return err;
> +}
> +
> +static void dial_cb(struct qmi_result *result, void *user_data)
> +{
> +	struct cb_data *cbd = user_data;
> +	struct ofono_voicecall *vc = cbd->user;
> +	struct voicecall_data *vd = ofono_voicecall_get_data(vc);
> +	ofono_voicecall_cb_t cb = cbd->cb;
> +	uint16_t error;
> +	struct qmi_voice_dial_call_result dial_result;
> +
> +	DBG("");
> +
> +	if (qmi_result_set_error(result, &error)) {
> +		DBG("QMI Error %d", error);
> +		CALLBACK_WITH_FAILURE(cb, cbd->data);
> +		return;
> +	}
> +
> +	if (qmi_voice_dial_call_parse(result, &dial_result) != NONE) {
> +		DBG("Received invalid Result");
> +		CALLBACK_WITH_FAILURE(cb, cbd->data);
> +		return;
> +	}
> +
> +	if (!dial_result.call_id_set) {
> +		DBG("Didn't receive a call id");
> +		CALLBACK_WITH_FAILURE(cb, cbd->data);
> +		return;
> +	}
> +
> +	DBG("New call QMI id %d", dial_result.call_id);
> +	ofono_call_list_dial_callback(vc, &vd->call_list, &vd->dialed,

You're passing vc to this function, it can easily obtain the call list via 
ofono_voicecall_get_data(vc);

> +				      dial_result.call_id); > +
> +	/* FIXME: create a timeout on this call_id */

What's this about?

> +	CALLBACK_WITH_SUCCESS(cb, cbd->data);
> +}
> +
> +static void dial(struct ofono_voicecall *vc,
> +		 const struct ofono_phone_number *ph,
> +		 enum ofono_clir_option clir, ofono_voicecall_cb_t cb,
> +		 void *data)
> +{
> +	struct voicecall_data *vd = ofono_voicecall_get_data(vc);
> +	struct cb_data *cbd = cb_data_new(cb, data);
> +	struct qmi_voice_dial_call_arg arg;

This structure doesn't seem to serve any purpose?  You can fill out the 
qmi_param information based solely on the passed in arguments.

> +	struct qmi_param *param = NULL;

Don't initialize variables unnecessarily.  The compiler will warn if you're 
using an uninitialized variable.

> +
> +	DBG("");
> +
> +	cbd->user = vc;
> +	arg.calling_number_set = true;
> +	arg.calling_number = phone_number_to_string(ph);
> +	memcpy(&vd->dialed, ph, sizeof(*ph));
> +
> +	arg.call_type_set = true;
> +	arg.call_type = QMI_VOICE_CALL_TYPE_VOICE;
> +
> +	param = qmi_param_new();
> +	if (!param)
> +		goto error;
> +
> +	if (arg.calling_number_set) {
> +		if (!qmi_param_append(param, QMI_VOICE_DIAL_CALL_NUMBER,
> +				      strlen(arg.calling_number),
> +				      arg.calling_number)) {
> +			goto error;
> +		}
> +	}
> +
> +	if (arg.call_type_set)
> +		qmi_param_append_uint8(param, QMI_VOICE_DIAL_CALL_TYPE,
> +				       arg.call_type);
> +
> +	if (qmi_service_send(vd->voice, QMI_VOICE_DIAL_CALL, param, dial_cb,
> +			     cbd, l_free) > 0) {
> +		return;
> +	}

No need for {}

> +
> +error:
> +	CALLBACK_WITH_FAILURE(cb, data);
> +	l_free(cbd);
> +	l_free(param);
> +}
> +
>   static void create_voice_cb(struct qmi_service *service, void *user_data)
>   {
>   	struct ofono_voicecall *vc = user_data;
> @@ -58,6 +554,9 @@ static void create_voice_cb(struct qmi_service *service, void *user_data)
>   
>   	data->voice = qmi_service_ref(service);
>   
> +	qmi_service_register(data->voice, QMI_VOICE_IND_ALL_STATUS,
> +			     all_call_status_ind, vc, NULL);
> +
>   	ofono_voicecall_register(vc);
>   }
>   
> @@ -70,6 +569,7 @@ static int qmi_voicecall_probe(struct ofono_voicecall *vc,
>   	DBG("");
>   
>   	data = l_new(struct voicecall_data, 1);
> +	data->call_list = l_queue_new();
>   
>   	ofono_voicecall_set_data(vc, data);
>   
> @@ -77,7 +577,6 @@ static int qmi_voicecall_probe(struct ofono_voicecall *vc,
>   					create_voice_cb, vc, NULL);
>   
>   	return 0;
> -
>   }
>   
>   static void qmi_voicecall_remove(struct ofono_voicecall *vc)
> @@ -98,7 +597,9 @@ static void qmi_voicecall_remove(struct ofono_voicecall *vc)
>   static const struct ofono_voicecall_driver driver = {
>   	.probe		= qmi_voicecall_probe,
>   	.remove		= qmi_voicecall_remove,
> +	.dial		= dial,
>   };
>   
>   OFONO_ATOM_DRIVER_BUILTIN(voicecall, qmimodem, &driver)
>   
> +

No empty lines at EOF please.