Hi Lasse,

On 03/02/2011 03:33 AM, Lasse Kunnasluoto wrote:
> ---
>  plugins/ste.c |  138 ++++++++++++++++++++++++++++++++++++---------------------
>  1 files changed, 87 insertions(+), 51 deletions(-)
> 
> diff --git a/plugins/ste.c b/plugins/ste.c
> index b786571..9a1d3d3 100644
> --- a/plugins/ste.c
> +++ b/plugins/ste.c
> @@ -65,13 +65,19 @@
>  #include <drivers/stemodem/caif_socket.h>
>  #include <drivers/stemodem/if_caif.h>
>  
> -#define NUM_CHAT	1
> +#define NUM_CHAT	5
> +#define AT_DEFAULT	0
> +#define AT_NET	1
> +#define AT_VOICE	2
> +#define AT_GPRS	3
> +#define AT_SIM	4

Can you cleanup the indentation of the channel numbers here to be more
consistent?  And I'd actually like to have an empty line between these
and the MAX_PDP_CONTEXTS define.

>  #define MAX_PDP_CONTEXTS	4
>  
> -static char *chat_prefixes[NUM_CHAT] = { "Default: " };
> +static char *chat_prefixes[NUM_CHAT] = { "Default: ", "Net: ", "Voice: ",
> +					"GPRS: ", "SIM: " };
>  
>  struct ste_data {
> -	GAtChat *chat;
> +	GAtChat *chat[NUM_CHAT];
>  	gboolean have_sim;
>  	struct ofono_sim *sim;
>  };
> @@ -105,12 +111,14 @@ static int ste_probe(struct ofono_modem *modem)
>  static void ste_remove(struct ofono_modem *modem)
>  {
>  	struct ste_data *data = ofono_modem_get_data(modem);
> +	int i;
>  
>  	DBG("%p", modem);
>  
>  	ofono_modem_set_data(modem, NULL);
>  
> -	g_at_chat_unref(data->chat);
> +	for (i = 0; i < NUM_CHAT; i++)
> +		g_at_chat_unref(data->chat[i]);
>  
>  	g_free(data);
>  }
> @@ -185,7 +193,7 @@ static gboolean init_sim_reporting(gpointer user_data)
>  	struct ofono_modem *modem = user_data;
>  	struct ste_data *data = ofono_modem_get_data(modem);
>  
> -	g_at_chat_send(data->chat, "AT*ESIMSR=1;*ESIMSR?", NULL,
> +	g_at_chat_send(data->chat[AT_SIM], "AT*ESIMSR=1;*ESIMSR?", NULL,
>  			handle_sim_state,
>  			modem, NULL);
>  
> @@ -195,11 +203,21 @@ static gboolean init_sim_reporting(gpointer user_data)
>  static void cfun_enable(gboolean ok, GAtResult *result, gpointer user_data)
>  {
>  	struct ofono_modem *modem = user_data;
> +	struct ste_data *data = ofono_modem_get_data(modem);
> +	int i;
>  
>  	DBG("");
>  
>  	if (!ok) {
>  		ofono_modem_set_powered(modem, FALSE);
> +
> +		for (i = 0; i < NUM_CHAT; i++) {
> +			g_at_chat_cancel_all(data->chat[i]);
> +			g_at_chat_unregister_all(data->chat[i]);
> +			g_at_chat_unref(data->chat[i]);
> +			data->chat[i] = NULL;
> +		}
> +
>  		return;
>  	}
>  
> @@ -295,35 +313,47 @@ static void esimsr_notify(GAtResult *result, gpointer user_data)
>  static int ste_enable(struct ofono_modem *modem)
>  {
>  	struct ste_data *data = ofono_modem_get_data(modem);
> -	GIOChannel *channel;
>  	GAtSyntax *syntax;
> +	int i;
>  
>  	syntax = g_at_syntax_new_gsm_permissive();
>  

Sharing a syntax between all channels isn't really going to work out how
you think ;)  Each syntax has its own state, so you have to create a
syntax for every channel separately.

> -	channel = ste_create_channel(modem);
> -	if (!channel)
> -		return -EIO;
> +	for (i = 0; i < NUM_CHAT; i++) {
> +		GIOChannel *channel = ste_create_channel(modem);
>  
> -	data->chat = g_at_chat_new_blocking(channel, syntax);
> -	g_at_chat_send(data->chat, "AT&F E0 V1 X4 &C1 +CMEE=1",
> -			NULL, NULL, NULL, NULL);
> +		data->chat[i] = g_at_chat_new_blocking(channel, syntax);
> +		if (data->chat[i] == NULL)
> +			return -EIO;

You might want to handle these errors properly.  If creation of one chat
fails, then you should unref all of them.

>  
> -	/* All STE modems support UTF-8 */
> -	g_at_chat_send(data->chat, "AT+CSCS=\"UTF-8\"",
> -			NULL, NULL, NULL, NULL);
> +		g_at_chat_send(data->chat[i], "AT&F E0 V1 X4 &C1 +CMEE=1",
> +				NULL, NULL, NULL, NULL);
>  
> -	g_io_channel_unref(channel);
> -	g_at_syntax_unref(syntax);
> +		/* All STE modems support UTF-8 */
> +		g_at_chat_send(data->chat[i], "AT+CSCS=\"UTF-8\"",
> +				NULL, NULL, NULL, NULL);
>  
> -	if (data->chat == NULL)
> -		return -ENOMEM;
> +		g_io_channel_unref(channel);
> +	}
>  
> -	if (getenv("OFONO_AT_DEBUG"))
> -		g_at_chat_set_debug(data->chat, ste_debug, chat_prefixes[0]);
> +	g_at_syntax_unref(syntax);
>  
> -	g_at_chat_send(data->chat, "AT+CFUN=4", NULL, cfun_enable, modem, NULL);
> +	if (getenv("OFONO_AT_DEBUG")) {
> +		g_at_chat_set_debug(data->chat[AT_DEFAULT], ste_debug,
> +					chat_prefixes[0]);
> +		g_at_chat_set_debug(data->chat[AT_NET], ste_debug,
> +					chat_prefixes[1]);
> +		g_at_chat_set_debug(data->chat[AT_VOICE], ste_debug,
> +					chat_prefixes[2]);
> +		g_at_chat_set_debug(data->chat[AT_GPRS], ste_debug,
> +					chat_prefixes[3]);
> +		g_at_chat_set_debug(data->chat[AT_SIM], ste_debug,
> +					chat_prefixes[4]);
> +	}

Why is this being done here and not in a loop above?

>  
> -	g_at_chat_register(data->chat, "*ESIMSR:", esimsr_notify,
> +	g_at_chat_send(data->chat[AT_DEFAULT], "AT+CFUN=4", NULL, cfun_enable,
> +				modem, NULL);
> +
> +	g_at_chat_register(data->chat[AT_SIM], "*ESIMSR:", esimsr_notify,
>  				FALSE, modem, NULL);
>  
>  	return -EINPROGRESS;
> @@ -333,11 +363,14 @@ static void cfun_disable(gboolean ok, GAtResult *result, gpointer user_data)
>  {
>  	struct ofono_modem *modem = user_data;
>  	struct ste_data *data = ofono_modem_get_data(modem);
> +	int i;
>  
>  	DBG("");
>  
> -	g_at_chat_unref(data->chat);
> -	data->chat = NULL;
> +	for (i = 0; i < NUM_CHAT; i++) {
> +		g_at_chat_unref(data->chat[i]);
> +		data->chat[i] = NULL;
> +	}
>  
>  	if (ok)
>  		ofono_modem_set_powered(modem, FALSE);
> @@ -346,15 +379,15 @@ static void cfun_disable(gboolean ok, GAtResult *result, gpointer user_data)
>  static int ste_disable(struct ofono_modem *modem)
>  {
>  	struct ste_data *data = ofono_modem_get_data(modem);
> +	int i;
>  
>  	DBG("%p", modem);
>  
> -	if (data->chat == NULL)
> -		return 0;
> -
> -	g_at_chat_cancel_all(data->chat);
> -	g_at_chat_unregister_all(data->chat);
> -	g_at_chat_send(data->chat, "AT+CFUN=4", NULL,
> +	for (i = 0; i < NUM_CHAT; i++) {
> +		g_at_chat_cancel_all(data->chat[i]);
> +		g_at_chat_unregister_all(data->chat[i]);
> +	}

Coding style M1

> +	g_at_chat_send(data->chat[AT_DEFAULT], "AT+CFUN=4", NULL,
>  					cfun_disable, modem, NULL);
>  
>  	return -EINPROGRESS;
> @@ -375,7 +408,7 @@ static void ste_set_online(struct ofono_modem *modem, ofono_bool_t online,
>  				ofono_modem_online_cb_t cb, void *user_data)
>  {
>  	struct ste_data *data = ofono_modem_get_data(modem);
> -	GAtChat *chat = data->chat;
> +	GAtChat *chat = data->chat[AT_DEFAULT];
>  	struct cb_data *cbd = cb_data_new(cb, user_data);
>  	char const *command = online ? "AT+CFUN=1" : "AT+CFUN=4";
>  
> @@ -395,10 +428,10 @@ static void ste_pre_sim(struct ofono_modem *modem)
>  
>  	DBG("%p", modem);
>  
> -	ofono_devinfo_create(modem, 0, "atmodem", data->chat);
> +	ofono_devinfo_create(modem, 0, "atmodem", data->chat[AT_DEFAULT]);
>  	data->sim = ofono_sim_create(modem, OFONO_VENDOR_MBM, "atmodem",
> -					data->chat);
> -	ofono_voicecall_create(modem, 0, "stemodem", data->chat);
> +					data->chat[AT_SIM]);
> +	ofono_voicecall_create(modem, 0, "stemodem", data->chat[AT_VOICE]);
>  }
>  
>  static void ste_post_sim(struct ofono_modem *modem)
> @@ -407,11 +440,11 @@ static void ste_post_sim(struct ofono_modem *modem)
>  
>  	DBG("%p", modem);
>  
> -	ofono_stk_create(modem, 0, "mbmmodem", data->chat);
> -	ofono_phonebook_create(modem, 0, "atmodem", data->chat);
> -	ofono_radio_settings_create(modem, 0, "stemodem", data->chat);
> +	ofono_stk_create(modem, 0, "mbmmodem", data->chat[AT_SIM]);
> +	ofono_phonebook_create(modem, 0, "atmodem", data->chat[AT_SIM]);
> +	ofono_radio_settings_create(modem, 0, "stemodem", data->chat[AT_NET]);
>  
> -	ofono_sms_create(modem, 0, "atmodem", data->chat);
> +	ofono_sms_create(modem, 0, "atmodem", data->chat[AT_DEFAULT]);
>  }
>  
>  static void ste_post_online(struct ofono_modem *modem)
> @@ -424,22 +457,25 @@ static void ste_post_online(struct ofono_modem *modem)
>  
>  	DBG("%p", modem);
>  
> -	ofono_ussd_create(modem, 0, "atmodem", data->chat);
> -	ofono_call_forwarding_create(modem, 0, "atmodem", data->chat);
> -	ofono_call_settings_create(modem, 0, "atmodem", data->chat);
> -	ofono_netreg_create(modem, OFONO_VENDOR_MBM, "atmodem", data->chat);
> -	ofono_call_meter_create(modem, 0, "atmodem", data->chat);
> -	ofono_call_barring_create(modem, 0, "atmodem", data->chat);
> -	ofono_ssn_create(modem, 0, "atmodem", data->chat);
> -	ofono_call_volume_create(modem, 0, "atmodem", data->chat);
> -	ofono_cbs_create(modem, 0, "atmodem", data->chat);
> +	ofono_ussd_create(modem, 0, "atmodem", data->chat[AT_DEFAULT]);
> +	ofono_call_forwarding_create(modem, 0,
> +					"atmodem", data->chat[AT_DEFAULT]);
> +	ofono_call_settings_create(modem, 0, "atmodem", data->chat[AT_DEFAULT]);
> +	ofono_netreg_create(modem, OFONO_VENDOR_MBM,
> +					"atmodem", data->chat[AT_NET]);
> +	ofono_call_meter_create(modem, 0, "atmodem", data->chat[AT_DEFAULT]);
> +	ofono_call_barring_create(modem, 0, "atmodem", data->chat[AT_DEFAULT]);
> +	ofono_ssn_create(modem, 0, "atmodem", data->chat[AT_DEFAULT]);
> +	ofono_call_volume_create(modem, 0, "atmodem", data->chat[AT_DEFAULT]);
> +	ofono_cbs_create(modem, 0, "atmodem", data->chat[AT_DEFAULT]);
>  
>  	gprs = ofono_gprs_create(modem, OFONO_VENDOR_MBM,
> -					"atmodem", data->chat);
> +					"atmodem", data->chat[AT_GPRS]);
> +
>  	if (gprs) {
>  		for (i = 0; i < MAX_PDP_CONTEXTS; i++) {
> -			gc = ofono_gprs_context_create(
> -					modem, 0, "stemodem", data->chat);
> +			gc = ofono_gprs_context_create(modem, 0, "stemodem",
> +					 data->chat[AT_GPRS]);

You have a whitespace violation here, mixing tabs and spaces.

>  			if (gc == NULL)
>  				break;
>  

Regards,
-Denis