Re: [PATCH 2/5] speedup: Check for supported modem capabilities first

[Denis Kenzior <denkenz@gmail.com>]

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

Hi Guillaume,

On 01/09/2012 07:10 AM, Guillaume Zajac wrote:
> ---
>  plugins/speedup.c |   40 ++++++++++++++++++++++++++++++++++++++--
>  1 files changed, 38 insertions(+), 2 deletions(-)
> 
> diff --git a/plugins/speedup.c b/plugins/speedup.c
> index a90dfe3..f03f8f3 100644
> --- a/plugins/speedup.c
> +++ b/plugins/speedup.c
> @@ -25,6 +25,7 @@
>  
>  #include <errno.h>
>  #include <stdlib.h>
> +#include <string.h>
>  
>  #include <glib.h>
>  #include <gatchat.h>
> @@ -47,11 +48,15 @@
>  #include <drivers/atmodem/atutil.h>
>  #include <drivers/atmodem/vendor.h>
>  
> +static const char *gcap_prefix[] = { "+GCAP:", NULL };
> +
>  struct speedup_data {
>  	GAtChat *modem;
>  	GAtChat *aux;
>  	gboolean have_sim;
>  	struct at_util_sim_state_query *sim_state_query;
> +	gboolean have_gsm;
> +	gboolean have_cdma;

It might be a good idea to use a single gboolean, or better yet an enum
here instead.  There's no point to waste 8 bytes when a single byte can do.

>  };
>  
>  static int speedup_probe(struct ofono_modem *modem)
> @@ -165,6 +170,36 @@ static void cfun_enable(gboolean ok, GAtResult *result, gpointer user_data)
>  						2, 20, sim_state_cb, modem);
>  }
>  
> +static void gcap_support(gboolean ok, GAtResult *result, gpointer user_data)
> +{
> +	struct ofono_modem *modem = user_data;
> +	struct speedup_data *data = ofono_modem_get_data(modem);
> +	GAtResultIter iter;
> +	const char *gcap;
> +
> +	if (!ok)
> +		goto done;
> +
> +	g_at_result_iter_init(&iter, result);
> +
> +	if (!g_at_result_iter_next(&iter, "+GCAP:"))
> +		goto done;
> +
> +	while (g_at_result_iter_next_unquoted_string(&iter, &gcap)) {
> +		if (*gcap == '\0')
> +			break;
> +
> +		if (!strcmp(gcap, "+CGSM"))
> +			data->have_gsm = TRUE;
> +		else if (!strcmp(gcap, "+CIS707-A"))
> +			data->have_cdma = TRUE;
> +	}

First of all, this really belongs in atutil as a convenience function.

Second, why don't we fail on the obvious condition that neither CDMA nor
GSM is detected?

> +
> +done:
> +	g_at_chat_send(data->aux, "AT+CFUN=1", NULL,
> +					cfun_enable, modem, NULL);
> +}
> +
>  static int speedup_enable(struct ofono_modem *modem)
>  {
>  	struct speedup_data *data = ofono_modem_get_data(modem);
> @@ -185,8 +220,9 @@ static int speedup_enable(struct ofono_modem *modem)
>  	g_at_chat_send(data->modem, "ATE0 +CMEE=1", NULL, NULL, NULL, NULL);
>  	g_at_chat_send(data->aux, "ATE0 +CMEE=1", NULL, NULL, NULL, NULL);
>  
> -	g_at_chat_send(data->aux, "AT+CFUN=1", NULL,
> -					cfun_enable, modem, NULL);
> +	/* Check for GSM/CDMA capabilities */
> +	g_at_chat_send(data->aux, "ATI", gcap_prefix,
> +					gcap_support, modem, NULL);
>  
>  	return -EINPROGRESS;
>  }

Regards,
-Denis