Re: [RFC PATCH 17/20] atmodem: add lte specific functions

[Denis Kenzior <denkenz@gmail.com>]

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

On 04/11/2011 05:20 AM, Vijay Nayani wrote:
> Changes include: Registration of CEREG events,
> registration status query based on registered
> technology, handling of ME PDN ACT CGEV event,
> reading of default context params and notification
> of default context params to the core.
> ---
>  drivers/atmodem/packet-service.c |  238 ++++++++++++++++++++++++++++++++++++++
>  1 files changed, 238 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/atmodem/packet-service.c b/drivers/atmodem/packet-service.c
> index 9999d9d..b766a3b 100644
> --- a/drivers/atmodem/packet-service.c
> +++ b/drivers/atmodem/packet-service.c
> @@ -4,6 +4,7 @@
>   *
>   *  Copyright (C) 2008-2010  Intel Corporation. All rights reserved.
>   *  Copyright (C) 2010 ST-Ericsson AB.
> + *  Copyright 2011 EB(Elektrobit).
>   *
>   *  This program is free software; you can redistribute it and/or modify
>   *  it under the terms of the GNU General Public License version 2 as
> @@ -40,17 +41,23 @@
>  #include "gatchat.h"
>  #include "gatresult.h"
>  
> +#include  "common.h"
> +
>  #include "atmodem.h"
>  #include "vendor.h"
>  
>  static const char *cgreg_prefix[] = { "+CGREG:", NULL };
>  static const char *cgdcont_prefix[] = { "+CGDCONT:", NULL };
> +static const char *cereg_prefix[] = { "+CEREG:", NULL };
> +static const char *cgcontrdp_prefix[] = { "+CGCONTRDP:", NULL };
>  static const char *none_prefix[] = { NULL };
>  
>  struct packet_service_data {
>  	GAtChat *chat;
>  	int default_cid;
>  	int default_context_type;
> +	int cid_min;
> +	int cid_max;
>  	unsigned int vendor;
>  };
>  
> @@ -109,6 +116,134 @@ static void at_cgreg_cb(gboolean ok, GAtResult *result, gpointer user_data)
>  	cb(&error, status, cbd->data);
>  }
>  
> +static void at_cereg_cb(gboolean ok, GAtResult *result, gpointer user_data)
> +{
> +	struct cb_data *cbd = user_data;
> +	ofono_packet_service_status_cb_t cb = cbd->cb;
> +	struct ofono_error error;
> +	int status;
> +	struct packet_service_data *psd = cbd->user;
> +
> +	decode_at_error(&error, g_at_result_final_response(result));
> +
> +	if (!ok) {
> +		cb(&error, -1, cbd->data);
> +		return;
> +	}
> +
> +	if (at_util_parse_reg(result, "+CEREG:", NULL, &status,
> +				NULL, NULL, NULL, psd->vendor) == FALSE) {
> +		CALLBACK_WITH_FAILURE(cb, -1, cbd->data);
> +		return;
> +	}
> +
> +	cb(&error, status, cbd->data);
> +}
> +
> +static void at_cgcontrdp_cb(gboolean ok, GAtResult *result, gpointer user_data)
> +{
> +	struct ofono_packet_service *ps = user_data;
> +	struct packet_service_data *psd = ofono_packet_service_get_data(ps);
> +	GAtResultIter iter;
> +	int cid, bearer_id;
> +	const char *apn, *address, *gateway, *dns1, *dns2;
> +	const char *dns[3];
> +	struct ofono_default_context_param *param;
> +
> +	if (!ok)
> +		return;
> +
> +	g_at_result_iter_init(&iter, result);
> +
> +	if (g_at_result_iter_next(&iter, "+CGCONTRDP:") == FALSE)
> +		return;
> +
> +	if (g_at_result_iter_next_number(&iter, &cid) == FALSE)
> +		return;
> +
> +	if (g_at_result_iter_next_number(&iter, &bearer_id) == FALSE)
> +		return;
> +
> +	if (g_at_result_iter_next_string(&iter, &apn) == FALSE)
> +		return;
> +
> +	if (g_at_result_iter_next_string(&iter, &address) == FALSE)
> +		goto out;
> +
> +	if (g_at_result_iter_next_string(&iter, &gateway) == FALSE)
> +		goto out;
> +
> +	if (g_at_result_iter_next_string(&iter, &dns1) == FALSE)
> +		goto out;
> +
> +	if (g_at_result_iter_next_string(&iter, &dns2) == FALSE)
> +		goto out;
> +
> +	dns[0] = dns1;
> +	dns[1] = dns2;
> +	dns[2] = 0;
> +
> +out:
> +	param = g_new0(struct ofono_default_context_param, 1);
> +	if (param == NULL)
> +		return;
> +
> +	param->cid = cid;
> +	param->proto = psd->default_context_type;
> +	param->apn = apn;
> +	param->address = address;
> +	param->gateway = gateway;
> +	param->dns = dns;
> +
> +	ofono_packet_service_default_context_notify(ps, param);

This is really not working out, since you still need to get the
interface for this context.  The fact that a context is active does not
necessarily mean that all the resources have been properly allocated.
Namely a gprs-context allocated and all the implications (e.g. AT
channel dedicated to the gprs-context is taken, ppp / high speed data
connection established, etc.)

Only the core can do the context allocation, so you still need to let
the core decide what to do.

> +
> +	g_free(param);
> +	param = NULL;
> +}
> +
> +static void at_cgdcont_cb(gboolean ok, GAtResult *result, gpointer user_data)
> +{
> +	struct ofono_packet_service *ps = user_data;
> +	struct packet_service_data *psd = ofono_packet_service_get_data(ps);
> +	GAtResultIter iter;
> +	int cid;
> +	const char *pdp_type;
> +	char buf[64];
> +	gboolean found = FALSE;
> +
> +	if (!ok)
> +		return;
> +
> +	g_at_result_iter_init(&iter, result);
> +
> +	while (!found && g_at_result_iter_next(&iter, "+CGDCONT:")) {
> +		if (g_at_result_iter_next_number(&iter, &cid) == FALSE)
> +			return;
> +
> +		if (psd->default_cid != cid)
> +			continue;
> +
> +		g_at_result_iter_next_string(&iter, &pdp_type);
> +
> +		found = TRUE;
> +	}
> +
> +	if (found == FALSE)
> +		return;
> +
> +	if (g_str_equal(pdp_type, "IP"))
> +		psd->default_context_type = OFONO_PACKET_PROTO_IP;
> +	else if (g_str_equal(pdp_type, "IPV6"))
> +		psd->default_context_type = OFONO_PACKET_PROTO_IPV6;
> +	else if (g_str_equal(pdp_type, "IPV4V6"))
> +		psd->default_context_type = OFONO_PACKET_PROTO_IPV4V6;
> +
> +	snprintf(buf, sizeof(buf), "AT+CGCONTRDP=%i", psd->default_cid);
> +
> +	g_at_chat_send(psd->chat, buf, cgcontrdp_prefix, at_cgcontrdp_cb,
> +								ps, NULL);
> +}
> +
>  static void at_packet_service_registration_status(
>  					struct ofono_packet_service *ps,
>  					ofono_packet_service_status_cb_t cb,
> @@ -138,6 +273,13 @@ static void at_packet_service_registration_status(
>  		break;
>  	}
>  
> +	if (ofono_packet_service_get_technology(ps) ==
> +						ACCESS_TECHNOLOGY_EUTRAN) {
> +		if (g_at_chat_send(psd->chat, "AT+CEREG?", cereg_prefix,
> +					at_cereg_cb, cbd, g_free) > 0)
> +			return;
> +	}
> +
>  	if (g_at_chat_send(psd->chat, "AT+CGREG?", cgreg_prefix,
>  				at_cgreg_cb, cbd, g_free) > 0)
>  		return;

This is really just a bad idea.  The core needs to understand the
concept of CEREG properly, not try to hide this in the driver.

Regards,
-Denis