Re: [PATCHv2] isi: fix handling of incoming/waiting calls

[Denis Kenzior <denkenz@gmail.com>]

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

Hi Pekka,

On 11/08/2010 01:49 PM, Pekka.Pessi(a)nokia.com wrote:
> From: Pekka Pessi <Pekka.Pessi@nokia.com>
> 
> Isimodem driver reports the mobile-terminated call as soon as possible,
> however, it is not possible to answer a call before it is in
> "MT-ALERTING" or "WAITING" state.
> 
> Also, report the state of an early mobile-terminated call to the core as
> "waiting" or "incoming", depending on the state of the other calls.
> ---
>  drivers/isimodem/voicecall.c |  121 +++++++++++++++++++++++++++++++++++++-----
>  1 files changed, 108 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/isimodem/voicecall.c b/drivers/isimodem/voicecall.c
> index c3365f6..511c38e 100644
> --- a/drivers/isimodem/voicecall.c
> +++ b/drivers/isimodem/voicecall.c
> @@ -65,8 +65,10 @@ struct isi_voicecall {
>  static void isi_call_notify(struct ofono_voicecall *ovc,
>  				struct isi_call *call);
>  static void isi_call_release(struct ofono_voicecall *, struct isi_call *);
> -static struct ofono_call isi_call_as_ofono_call(struct isi_call const *);
> -static int isi_call_status_to_clcc(struct isi_call const *call);
> +static struct ofono_call isi_call_as_ofono_call(struct isi_voicecall const *,
> +						struct isi_call const *);
> +static int isi_call_status_to_clcc(struct isi_voicecall const *,
> +					struct isi_call const *);

Is there really a need for all these forward declarations?  Try to avoid
these whenever possible.  Also, the general convention so far has been
to use const struct foo *, not struct foo const *.  They are equivalent,
but people with non-C++ background are typically more used to the first one.

>  static struct isi_call *isi_call_set_idle(struct isi_call *call);
>  
>  typedef void GIsiIndication(GIsiClient *client,
> @@ -97,12 +99,30 @@ typedef void isi_call_req_step(struct isi_call_req_context *, int reason);
>  struct isi_call_req_context {
>  	struct isi_call_req_context *next, **prev;
>  	isi_call_req_step *step;
> +	int id;
>  	struct ofono_voicecall *ovc;
>  	ofono_voicecall_cb_t cb;
>  	void *data;
>  };
>  
>  static struct isi_call_req_context *
> +isi_call_req_context_new(struct ofono_voicecall *ovc,
> +				ofono_voicecall_cb_t cb, void *data)
> +{
> +	struct isi_call_req_context *irc;
> +
> +	irc = g_try_new0(struct isi_call_req_context, 1);
> +
> +	if (irc) {
> +		irc->ovc = ovc;
> +		irc->cb = cb;
> +		irc->data = data;
> +	}
> +
> +	return irc;
> +}
> +
> +static struct isi_call_req_context *
>  isi_call_req(struct ofono_voicecall *ovc,
>  		void const *restrict req,
>  		size_t len,
> @@ -135,7 +155,8 @@ isi_call_req(struct ofono_voicecall *ovc,
>  }
>  
>  static void isi_ctx_queue(struct isi_call_req_context *irc,
> -				isi_call_req_step *next)
> +				isi_call_req_step *next,
> +				int id)
>  {
>  	if (irc->prev == NULL) {
>  		struct isi_voicecall *ivc = ofono_voicecall_get_data(irc->ovc);
> @@ -149,6 +170,7 @@ static void isi_ctx_queue(struct isi_call_req_context *irc,
>  	}
>  
>  	irc->step = next;
> +	irc->id = id;
>  }
>  
>  static void isi_ctx_remove(struct isi_call_req_context *irc)
> @@ -503,18 +525,65 @@ static void isi_call_status_ind_cb(GIsiClient *client,
>  	isi_call_notify(ovc, call);
>  }
>  
> +static void isi_wait_incoming(struct isi_call_req_context *irc, int event);
> +

And another forward declaration of a static function...

>  static struct isi_call_req_context *
>  isi_call_answer_req(struct ofono_voicecall *ovc,
>  			uint8_t call_id, ofono_voicecall_cb_t cb, void *data)
>  {
> +	struct isi_voicecall *ivc = ofono_voicecall_get_data(ovc);
> +
>  	uint8_t const req[] = {
>  		CALL_ANSWER_REQ, call_id, 0
>  	};
>  	size_t rlen = sizeof req;
>  
> +	int id = 0;

why do you bother initializing this variable here? You're
re-initializing it again 2 lines down.  Please see doc/coding-style.txt
item M7.

> +
> +	for (id = 1; id <= 7; id++) {
> +		if ((ivc->calls[id].status == CALL_STATUS_PROCEEDING ||
> +				ivc->calls[id].status == CALL_STATUS_COMING) &&
> +			(ivc->calls[id].mode_info & CALL_MODE_ORIGINATOR))
> +			break;

Please see coding-style.txt item M4.

> +	}
> +
> +	if (id <= 7) {
> +		struct isi_call_req_context *irc;
> +		irc = isi_call_req_context_new(ovc, cb, data);
> +		if (irc) {
> +			isi_ctx_queue(irc, isi_wait_incoming, id);
> +			return irc;
> +		}
> +	}
> +

This code might be easier to follow if you move the if condition into
the for loop above and use a continue statement as needed.

>  	return isi_call_req(ovc, req, rlen, isi_call_answer_resp, cb, data);
>  }
>  
> +static void isi_wait_incoming(struct isi_call_req_context *irc,
> +				int event)
> +{
> +	struct isi_voicecall *ivc;
> +
> +	DBG("irc=%p event=%u", (void *)irc, event);
> +
> +	switch (event) {
> +	case CALL_STATUS_MT_ALERTING:
> +	case CALL_STATUS_WAITING:
> +		isi_call_answer_req(irc->ovc, CALL_ID_ALL, irc->cb, irc->data);
> +		isi_ctx_free(irc);
> +		return;
> +	}
> +
> +	ivc = ofono_voicecall_get_data(irc->ovc);
> +	switch (ivc->calls[irc->id].status) {

Rule M2 applies here as well.

> +	case CALL_STATUS_MO_RELEASE:
> +	case CALL_STATUS_MT_RELEASE:
> +	case CALL_STATUS_TERMINATED:
> +	case CALL_STATUS_IDLE:
> +		isi_ctx_return_failure(irc);
> +	}
> +}
> +
>  static gboolean isi_call_answer_resp(GIsiClient *client,
>  					void const *restrict data,
>  					size_t len,
> @@ -830,11 +899,11 @@ static void isi_call_notify(struct ofono_voicecall *ovc,
>  		return;
>  	}
>  
> -	ocall = isi_call_as_ofono_call(call);
> +	ocall = isi_call_as_ofono_call(ivc, call);
>  
> -	DBG("id=%u,%s,%u,\"%s\",%u,%u",
> +	DBG("id=%u,\"%s\",%u,\"%s\",%u,%u",
>  		ocall.id,
> -		ocall.direction ? "terminated" : "originated",
> +		ocall.direction ? "mt" : "mo",
>  		ocall.status,
>  		ocall.phone_number.number,
>  		ocall.phone_number.type,
> @@ -875,14 +944,15 @@ static void isi_call_release(struct ofono_voicecall *ovc,
>  		isi_call_set_idle(call);
>  }
>  
> -static struct ofono_call isi_call_as_ofono_call(struct isi_call const *call)
> +static struct ofono_call isi_call_as_ofono_call(struct isi_voicecall const *ivc,
> +						struct isi_call const *call)
>  {
>  	struct ofono_call ocall = { call->id };
>  	struct ofono_phone_number *number = &ocall.phone_number;
>  
>  	ocall.type = 0;	/* Voice call */
>  	ocall.direction = call->mode_info & CALL_MODE_ORIGINATOR;
> -	ocall.status = isi_call_status_to_clcc(call);
> +	ocall.status = isi_call_status_to_clcc(ivc, call);
>  	memcpy(number->number, call->address, sizeof number->number);
>  	number->type = 0x80 | call->addr_type;
>  	ocall.clip_validity = call->presentation & 3;
> @@ -892,17 +962,41 @@ static struct ofono_call isi_call_as_ofono_call(struct isi_call const *call)
>  	return ocall;
>  }
>  
> +static int isi_call_waiting_or_incoming(struct isi_voicecall const *ivc,
> +					struct isi_call const *call)
> +{
> +	int id = 0;

And again, rule M7 being broken

> +
> +	for (id = 1; id <= 7; id++) {
> +		switch (ivc->calls[id].status) {
> +		case CALL_STATUS_ANSWERED:
> +		case CALL_STATUS_ACTIVE:
> +		case CALL_STATUS_MO_RELEASE:
> +		case CALL_STATUS_MT_RELEASE:
> +		case CALL_STATUS_HOLD_INITIATED:
> +		case CALL_STATUS_HOLD:
> +		case CALL_STATUS_RETRIEVE_INITIATED:
> +		case CALL_STATUS_RECONNECT_PENDING:
> +		case CALL_STATUS_SWAP_INITIATED:
> +			return 5; /* waiting */
> +		}
> +	}
> +

You don't use variable call at all here...

> +	return 4;		/* incoming */
> +}
> +
>  /** Get +CLCC status */
> -static int isi_call_status_to_clcc(struct isi_call const *call)
> +static int isi_call_status_to_clcc(struct isi_voicecall const *ivc,
> +					struct isi_call const *call)

You add a new parameter to this function to pass into the target
function, but end up not using it in the target.  Hmm... ;)

>  {
>  	switch (call->status) {
>  	case CALL_STATUS_CREATE:
>  		return 2;
>  	case CALL_STATUS_COMING:
> -		return 4;
> +		return isi_call_waiting_or_incoming(ivc, call);
>  	case CALL_STATUS_PROCEEDING:
>  		if ((call->mode_info & CALL_MODE_ORIGINATOR))
> -			return 4; /* MT */
> +			return isi_call_waiting_or_incoming(ivc, call); /* MT */
>  		else
>  			return 2; /* MO */
>  	case CALL_STATUS_MO_ALERTING:
> @@ -1072,9 +1166,9 @@ static void isi_release_all_active(struct ofono_voicecall *ovc,
>  		if (irc == NULL)
>  			;
>  		else if (waiting)
> -			isi_ctx_queue(irc, isi_wait_and_answer);
> +			isi_ctx_queue(irc, isi_wait_and_answer, 0);
>  		else if (hold)
> -			isi_ctx_queue(irc, isi_wait_and_retrieve);
> +			isi_ctx_queue(irc, isi_wait_and_retrieve, 0);
>  	} else
>  		CALLBACK_WITH_FAILURE(cb, data);
>  }
> @@ -1149,6 +1243,7 @@ static void isi_release_specific(struct ofono_voicecall *ovc, int id,
>  		uint8_t cause = CALL_CAUSE_RELEASE_BY_USER;
>  
>  		switch (status->status) {
> +		case CALL_STATUS_COMING:
>  		case CALL_STATUS_MT_ALERTING:
>  		case CALL_STATUS_WAITING:
>  			cause = CALL_CAUSE_BUSY_USER_REQUEST;

Regards,
-Denis