Re: [PATCH 2/8] telit: add additional port for data connection

[Denis Kenzior <denkenz@gmail.com>]

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

Hi Christopher,

CC-ing Gustavo since he wrote much of the SAP bits in this driver...

On 08/13/2012 08:17 AM, Christopher Vogl wrote:
> Use MDM port for data service and AUX for the AT chat.
> Disable DCD so that the modem does not hangup after a data connection.
> Add vendor for sim and gprs.
>
> udevng: rename aux port from Data to Aux for telit.
>
> Telit software user guide says:
> USB AUX doesn’t support any flow control method. Therefore, this port
> isn’t suitable for DATA service port.
> We recommend this port should be used only for AT command
> and URC processing.
> ---
>   plugins/telit.c  |   76 +++++++++++++++++++++++++++++++++++++----------------
>   plugins/udevng.c |    2 +-
>   2 files changed, 54 insertions(+), 24 deletions(-)
>
> diff --git a/plugins/telit.c b/plugins/telit.c
> index 853fd44..8446a22 100644
> --- a/plugins/telit.c
> +++ b/plugins/telit.c
> @@ -65,8 +65,8 @@ static const char *qss_prefix[] = { "#QSS:", NULL };
>   static const char *rsen_prefix[]= { "#RSEN:", NULL };
>
>   struct telit_data {
> -	GAtChat *chat;
> -	GAtChat *aux;
> +	GAtChat *chat;		/* AT chat */
> +	GAtChat *modem;		/* Data port */
>   	struct ofono_sim *sim;
>   	struct ofono_modem *sap_modem;
>   	GIOChannel *bt_io;
> @@ -295,6 +295,13 @@ static void cfun_enable_cb(gboolean ok, GAtResult *result, gpointer user_data)
>   		return;
>   	}
>
> +	/*
> +	 * Switch data carrier detect signal off.
> +	 * When the DCD is disabled the modem does not hangup anymore
> +	 * after the data connection.
> +	 */
> +	g_at_chat_send(data->chat, "AT&C0", NULL, NULL, NULL, NULL);
> +
>   	ofono_modem_set_powered(m, TRUE);
>
>   	/* Enable sim state notification */
> @@ -311,10 +318,19 @@ static int telit_enable(struct ofono_modem *modem)
>
>   	DBG("%p", modem);
>
> -	data->chat = open_device(modem, "Modem", "Modem: ");
> -	if (data->chat == NULL)
> +	data->modem = open_device(modem, "Modem", "Modem: ");
> +	if (data->modem == NULL)
>   		return -EINVAL;
>
> +	data->chat = open_device(modem, "Aux", "Aux: ");
> +	if (data->chat == NULL) {
> +		g_at_chat_unref(data->modem);
> +		data->modem = NULL;
> +		return -EIO;
> +	}
> +
> +	g_at_chat_set_slave(data->modem, data->chat);
> +

Gustavo, are you OK with these changes? I do not recall whether it made 
a difference on which port the #RSEN was being sent.

>   	/*
>   	 * Disable command echo and
>   	 * enable the Extended Error Result Codes
> @@ -362,8 +378,6 @@ static void rsen_enable_cb(gboolean ok, GAtResult *result, gpointer user_data)
>   	DBG("%p", modem);
>
>   	if (!ok) {
> -		g_at_chat_unref(data->aux);
> -		data->aux = NULL;
>   		ofono_modem_set_powered(data->sap_modem, FALSE);
>   		sap_close_io(modem);
>   		return;
> @@ -380,6 +394,21 @@ static void cfun_disable_cb(gboolean ok, GAtResult *result, gpointer user_data)
>
>   	DBG("%p", modem);
>
> +	GIOChannel *channel = g_at_chat_get_channel(data->modem);
> +	if(channel) {
> +		g_io_channel_unref(channel);
> +		channel = NULL;
> +	}

Why do you need this part?  What's the reason to manually unref the channel?

> +
> +	g_at_chat_unref(data->modem);
> +	data->modem = NULL;
> +
> +	channel = g_at_chat_get_channel(data->chat);
> +	if(channel) {
> +		g_io_channel_unref(channel);
> +		channel = NULL;
> +	}
> +

And here?

>   	g_at_chat_unref(data->chat);
>   	data->chat = NULL;
>
> @@ -394,6 +423,9 @@ static int telit_disable(struct ofono_modem *modem)
>   	struct telit_data *data = ofono_modem_get_data(modem);
>   	DBG("%p", modem);
>
> +	g_at_chat_cancel_all(data->modem);
> +	g_at_chat_unregister_all(data->modem);
> +
>   	g_at_chat_cancel_all(data->chat);
>   	g_at_chat_unregister_all(data->chat);
>
> @@ -411,9 +443,6 @@ static void rsen_disable_cb(gboolean ok, GAtResult *result, gpointer user_data)
>
>   	DBG("%p", modem);
>
> -	g_at_chat_unref(data->aux);
> -	data->aux = NULL;
> -
>   	sap_close_io(modem);
>
>   	telit_disable(modem);
> @@ -469,10 +498,6 @@ static int telit_sap_enable(struct ofono_modem *modem,
>   	g_io_channel_set_buffered(data->hw_io, FALSE);
>   	g_io_channel_set_close_on_unref(data->hw_io, TRUE);
>
> -	data->aux = open_device(modem, "Data", "Aux: ");
> -	if (data->aux == NULL)
> -		goto error;
> -
>   	data->bt_io = g_io_channel_unix_new(bt_fd);
>   	if (data->bt_io == NULL)
>   		goto error;
> @@ -491,13 +516,13 @@ static int telit_sap_enable(struct ofono_modem *modem,
>
>   	data->sap_modem = sap_modem;
>
> -	g_at_chat_register(data->aux, "#RSEN:", telit_rsen_notify,
> +	g_at_chat_register(data->chat, "#RSEN:", telit_rsen_notify,
>   				FALSE, modem, NULL);
>
> -	g_at_chat_send(data->aux, "AT#NOPT=0", NULL, NULL, NULL, NULL);
> +	g_at_chat_send(data->chat, "AT#NOPT=0", NULL, NULL, NULL, NULL);
>
>   	/* Set SAP functionality */
> -	g_at_chat_send(data->aux, "AT#RSEN=1,1,0,2,0", rsen_prefix,
> +	g_at_chat_send(data->chat, "AT#RSEN=1,1,0,2,0", rsen_prefix,
>   				rsen_enable_cb, modem, NULL);
>
>   	return -EINPROGRESS;
> @@ -516,10 +541,7 @@ static int telit_sap_disable(struct ofono_modem *modem)
>
>   	DBG("%p", modem);
>
> -	g_at_chat_cancel_all(data->aux);
> -	g_at_chat_unregister_all(data->aux);
> -

Removing this part might not be correct, Gustavo?

> -	g_at_chat_send(data->aux, "AT#RSEN=0", rsen_prefix,
> +	g_at_chat_send(data->chat, "AT#RSEN=0", rsen_prefix,
>   				rsen_disable_cb, modem, NULL);
>
>   	return -EINPROGRESS;
> @@ -535,7 +557,7 @@ static void telit_pre_sim(struct ofono_modem *modem)
>   	DBG("%p", modem);
>
>   	ofono_devinfo_create(modem, 0, "atmodem", data->chat);
> -	data->sim = ofono_sim_create(modem, 0, "atmodem", data->chat);
> +	data->sim = ofono_sim_create(modem, OFONO_VENDOR_TELIT, "atmodem", data->chat);
>   	ofono_voicecall_create(modem, 0, "atmodem", data->chat);
>   }
>
> @@ -593,8 +615,8 @@ static void telit_post_online(struct ofono_modem *modem)
>   	ofono_call_meter_create(modem, 0, "atmodem", data->chat);
>   	ofono_call_barring_create(modem, 0, "atmodem", data->chat);
>
> -	gprs = ofono_gprs_create(modem, 0, "atmodem", data->chat);
> -	gc = ofono_gprs_context_create(modem, 0, "atmodem", data->chat);
> +	gprs = ofono_gprs_create(modem, OFONO_VENDOR_TELIT, "atmodem", data->chat);
> +	gc = ofono_gprs_context_create(modem, 0, "atmodem", data->modem);
>
>   	if (gprs&&  gc)
>   		ofono_gprs_add_context(gprs, gc);
> @@ -633,11 +655,19 @@ static int telit_probe(struct ofono_modem *modem)
>
>   static void telit_remove(struct ofono_modem *modem)
>   {
> +	struct telit_data *data = ofono_modem_get_data(modem);
> +	
>   	DBG("%p", modem);
>
>   	bluetooth_sap_client_unregister(modem);
>
>   	ofono_modem_set_data(modem, NULL);
> +	
> +	/* Cleanup after hot-unplug */
> +	g_at_chat_unref(data->chat);
> +	g_at_chat_unref(data->modem);
> +	
> +	g_free(data);
>   }
>
>   static struct ofono_modem_driver telit_driver = {
> diff --git a/plugins/udevng.c b/plugins/udevng.c
> index 872039a..bd5e5e0 100644
> --- a/plugins/udevng.c
> +++ b/plugins/udevng.c
> @@ -615,7 +615,7 @@ static gboolean setup_telit(struct modem_info *modem)
>   	DBG("modem=%s aux=%s gps=%s diag=%s", mdm, aux, gps, diag);
>
>   	ofono_modem_set_string(modem->modem, "Modem", mdm);
> -	ofono_modem_set_string(modem->modem, "Data", aux);
> +	ofono_modem_set_string(modem->modem, "Aux", aux);
>   	ofono_modem_set_string(modem->modem, "GPS", gps);
>
>   	return TRUE;

This part belongs in a separate patch.

Regards,
-Denis