Re: [PATCH v3, Part1, 4/5] cdmamodem: Add CDMA voicecall driver support for MO Call

[Denis Kenzior <denkenz@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 4323 bytes --]

Hi Dara,

<snip>

> +
> +struct voicecall_driver {
> +	struct ofono_cdma_voicecall *vc;
> +	unsigned int local_release;

What is the use of these two variables?

> +	GAtChat *chat;
> +	unsigned int vendor;
> +};
> +
> +struct change_state_req {
> +	struct ofono_cdma_voicecall *vc;
> +	ofono_cdma_voicecall_cb_t cb;
> +	void *data;
> +};

is cb_data not good enough for some reason? :)

> +
> +static void at_dial_cb(gboolean ok, GAtResult *result, gpointer user_data)
> +{
> +	struct cb_data *cbd = user_data;
> +	ofono_cdma_voicecall_cb_t cb = cbd->cb;
> +	struct ofono_error error;
> +
> +	decode_at_error(&error, g_at_result_final_response(result));
> +
> +	if (!ok)
> +		goto out;

This if statement serves no purpose

> +
> +out:

And neither does the label

> +	cb(&error, cbd->data);
> +}
> +
> +static void at_dial(struct ofono_cdma_voicecall *vc,
> +				const struct ofono_cdma_phone_number *ph,
> +				ofono_cdma_voicecall_cb_t cb, void *data)
> +{
> +	struct voicecall_driver *vd = ofono_cdma_voicecall_get_data(vc);
> +	struct cb_data *cbd = cb_data_new(cb, data);
> +	char buf[256];
> +
> +	if (cbd == NULL)
> +		goto error;
> +
> +	cbd->user = vc;

Why are you assigning cbd->user and never making use of it?

> +
> +	snprintf(buf, sizeof(buf), "AT+CDV=%s", ph->number);
> +
> +	if (g_at_chat_send(vd->chat, buf, none_prefix,
> +				at_dial_cb, cbd, g_free) > 0)
> +		return;
> +
> +error:
> +	g_free(cbd);
> +
> +	CALLBACK_WITH_FAILURE(cb, data);
> +}
> +
> +static void at_template(const char *cmd,
> +				struct ofono_cdma_voicecall *vc,
> +				GAtResultFunc result_cb,
> +				ofono_cdma_voicecall_cb_t cb, void *data)
> +{
> +	struct voicecall_driver *vd = ofono_cdma_voicecall_get_data(vc);
> +	struct change_state_req *req = g_try_new0(struct change_state_req, 1);
> +
> +	if (req == NULL)
> +		goto error;
> +
> +	req->vc = vc;
> +	req->cb = cb;
> +	req->data = data;
> +
> +	if (g_at_chat_send(vd->chat, cmd, none_prefix,
> +				result_cb, req, g_free) > 0)
> +		return;
> +
> +error:
> +	g_free(req);
> +
> +	CALLBACK_WITH_FAILURE(cb, data);
> +}
> +
> +static void at_hangup_cb(gboolean ok, GAtResult *result, gpointer user_data)
> +{
> +	struct change_state_req *req = user_data;
> +
> +	if (!ok) {
> +		ofono_error("hangup failed.");
> +		return;

Please don't do this.  If the hangup fails you never return from the
operation and will cause the cdma-voicecall atom to stall forever.
decoding the error and calling the callback is good enough.

> +	}
> +
> +	ofono_cdma_voicecall_disconnected(req->vc,
> +				OFONO_DISCONNECT_REASON_LOCAL_HANGUP, NULL);

Fair enough, but this should really come via a modem unsolicited
notification.

> +		CALLBACK_WITH_SUCCESS(req->cb, req->data);

This is not necessary if you call the callback up above.  And why the
different indentation?

> +}
> +
> +static void at_hangup(struct ofono_cdma_voicecall *vc,
> +			ofono_cdma_voicecall_cb_t cb, void *data)
> +{
> +	/* Hangup active call */
> +	at_template("AT+CHV", vc, at_hangup_cb, cb, data);
> +}
> +
> +static int at_voicecall_probe(struct ofono_cdma_voicecall *vc,
> +		unsigned int vendor, void *data)
> +{
> +	GAtChat *chat = data;
> +	struct voicecall_driver *vd;
> +
> +	vd = g_try_new0(struct voicecall_driver, 1);
> +	if (vd == NULL)
> +		return -ENOMEM;
> +
> +	vd->chat = g_at_chat_clone(chat);
> +	vd->vendor = vendor;
> +
> +	ofono_cdma_voicecall_set_data(vc, vd);
> +
> +	ofono_cdma_voicecall_register(vc);
> +
> +	return 0;
> +}
> +
> +static void at_voicecall_remove(struct ofono_cdma_voicecall *vc)
> +{
> +	struct voicecall_driver *vd = ofono_cdma_voicecall_get_data(vc);
> +
> +	ofono_cdma_voicecall_set_data(vc, NULL);
> +
> +	g_at_chat_unref(vd->chat);
> +	g_free(vd);
> +}
> +
> +static struct ofono_cdma_voicecall_driver driver = {
> +	.name			= "cdmamodem",
> +	.probe			= at_voicecall_probe,
> +	.remove			= at_voicecall_remove,
> +	.dial			= at_dial,
> +	.hangup			= at_hangup,
> +};
> +
> +void cdma_at_voicecall_init()
> +{
> +	ofono_cdma_voicecall_driver_register(&driver);
> +}
> +
> +void cdma_at_voicecall_exit()
> +{
> +	ofono_cdma_voicecall_driver_unregister(&driver);
> +}

Regards,
-Denis