[PATCH 0/2] Add ifxmodem support for enable/disable tty

[PATCH 0/2] Add ifxmodem support for enable/disable tty

[Jeevaka Badrappan]

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

Hi,

 Following patch adds the ifxmodem support for enabling/disabling tty
mode.

Regards,
Jeevaka

Jeevaka Badrappan (2):
  ctm: add ofono_ctm_get_modem interface
  ifxmodem: add enable/disable ctm support

 Makefile.am                 |    3 +-
 drivers/ifxmodem/ctm.c      |  243 +++++++++++++++++++++++++++++++++++++++++++
 drivers/ifxmodem/ifxmodem.c |    2 +
 drivers/ifxmodem/ifxmodem.h |    3 +
 include/ctm.h               |    2 +
 src/ctm.c                   |    5 +
 6 files changed, 257 insertions(+), 1 deletions(-)
 create mode 100644 drivers/ifxmodem/ctm.c

[PATCH 1/2] ctm: add ofono_ctm_get_modem interface

[Jeevaka Badrappan]

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

---
 include/ctm.h |    2 ++
 src/ctm.c     |    5 +++++
 2 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/include/ctm.h b/include/ctm.h
index 5305469..71e3d61 100644
--- a/include/ctm.h
+++ b/include/ctm.h
@@ -59,6 +59,8 @@ void ofono_ctm_remove(struct ofono_ctm *ctm);
 void ofono_ctm_set_data(struct ofono_ctm *ctm, void *data);
 void *ofono_ctm_get_data(struct ofono_ctm *ctm);
 
+struct ofono_modem *ofono_ctm_get_modem(struct ofono_ctm *ctm);
+
 #ifdef __cplusplus
 }
 #endif
diff --git a/src/ctm.c b/src/ctm.c
index 1df34c2..389bb8e 100644
--- a/src/ctm.c
+++ b/src/ctm.c
@@ -330,3 +330,8 @@ void *ofono_ctm_get_data(struct ofono_ctm *ctm)
 {
 	return ctm->driver_data;
 }
+
+struct ofono_modem *ofono_ctm_get_modem(struct ofono_ctm *ctm)
+{
+	return __ofono_atom_get_modem(ctm->atom);
+}
\ No newline at end of file
-- 
1.7.0.4

[PATCH 2/2] ifxmodem: add enable/disable ctm support

[Jeevaka Badrappan]

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

---
 Makefile.am                 |    3 +-
 drivers/ifxmodem/ctm.c      |  243 +++++++++++++++++++++++++++++++++++++++++++
 drivers/ifxmodem/ifxmodem.c |    2 +
 drivers/ifxmodem/ifxmodem.h |    3 +
 4 files changed, 250 insertions(+), 1 deletions(-)
 create mode 100644 drivers/ifxmodem/ctm.c

diff --git a/Makefile.am b/Makefile.am
index 8ad01cd..e6494b1 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -222,7 +222,8 @@ builtin_sources += drivers/atmodem/atutil.h \
 			drivers/ifxmodem/audio-settings.c \
 			drivers/ifxmodem/radio-settings.c \
 			drivers/ifxmodem/gprs-context.c \
-			drivers/ifxmodem/stk.c
+			drivers/ifxmodem/stk.c \
+			drivers/ifxmodem/ctm.c
 
 builtin_modules += stemodem
 builtin_sources += drivers/atmodem/atutil.h \
diff --git a/drivers/ifxmodem/ctm.c b/drivers/ifxmodem/ctm.c
new file mode 100644
index 0000000..17c7f5c
--- /dev/null
+++ b/drivers/ifxmodem/ctm.c
@@ -0,0 +1,243 @@
+/*
+ *
+ *  oFono - Open Source Telephony
+ *
+ *  Copyright (C) 2008-2010  Intel Corporation. All rights reserved.
+ *
+ *  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
+ *  published by the Free Software Foundation.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; if not, write to the Free Software
+ *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+ *
+ */
+
+#ifdef HAVE_CONFIG_H
+#include <config.h>
+#endif
+
+#define _GNU_SOURCE
+#include <string.h>
+#include <stdlib.h>
+#include <stdio.h>
+#include <errno.h>
+
+#include <glib.h>
+
+#include <ofono/log.h>
+#include <ofono/modem.h>
+#include <ofono/ctm.h>
+
+#include "gatchat.h"
+#include "gatresult.h"
+
+#include "ifxmodem.h"
+
+static const char *none_prefix[] = { NULL };
+static const char *xctms_prefix[] = { "XCTMS:", NULL };
+static const char *xdrv_prefix[] = { "XDRV:", NULL };
+
+struct ctm_data {
+	GAtChat *chat;
+	const char * audio_setting;
+	ofono_bool_t enable;
+};
+
+static void xctms_query_cb(gboolean ok, GAtResult *result, gpointer user_data)
+{
+	struct cb_data *cbd = user_data;
+	ofono_ctm_query_cb_t cb = cbd->cb;
+	struct ofono_error error;
+	GAtResultIter iter;
+	int value;
+	ofono_bool_t enable;
+
+	decode_at_error(&error, g_at_result_final_response(result));
+
+	if (!ok) {
+		cb(&error, -1, cbd->data);
+		return;
+	}
+
+	g_at_result_iter_init(&iter, result);
+
+	if (g_at_result_iter_next(&iter, "XCTMS:") == FALSE)
+		goto error;
+
+	if (g_at_result_iter_next_number(&iter, &value) == FALSE)
+		goto error;
+
+	/* FULL TTY mode status only sent to oFono */
+	enable = (value == 1) ? TRUE : FALSE;
+
+	cb(&error, enable, cbd->data);
+
+	return;
+
+error:
+	CALLBACK_WITH_FAILURE(cb, -1, cbd->data);
+}
+
+static void ifx_query_tty(struct ofono_ctm *ctm, ofono_ctm_query_cb_t cb,
+				void *data)
+{
+	struct ctm_data *ctmd = ofono_ctm_get_data(ctm);
+	struct cb_data *cbd = cb_data_new(cb, data);
+
+	if (cbd == NULL)
+		goto error;
+
+	if (g_at_chat_send(ctmd->chat, "AT+XCTMS?", xctms_prefix,
+				xctms_query_cb, cbd, g_free) > 0)
+		return;
+
+error:
+	g_free(cbd);
+
+	CALLBACK_WITH_FAILURE(cb, -1, data);
+}
+
+static void xctms_modify_cb(gboolean ok, GAtResult *result, gpointer user_data)
+{
+	struct cb_data *cbd = user_data;
+	ofono_ctm_set_cb_t cb = cbd->cb;
+	struct ofono_error error;
+	const char *setting = NULL;
+	struct ofono_ctm *ctm = cbd->user;
+	struct ctm_data *ctmd = ofono_ctm_get_data(ctm);
+	ofono_bool_t enable = ctmd->enable;
+
+	decode_at_error(&error, g_at_result_final_response(result));
+
+	if (!ok) {
+		cb(&error, cbd->data);
+		return;
+	}
+
+	if (g_strcmp0(ctmd->audio_setting, "FULL_DUPLEX") == 0)
+		setting = "0,0,0,0,0,0,0";
+	else if (g_strcmp0(ctmd->audio_setting, "BURSTMODE_48KHZ") == 0)
+		setting = "0,0,8,0,2,0,0";
+	else if (g_strcmp0(ctmd->audio_setting, "BURSTMODE_96KHZ") == 0)
+		setting = "0,0,9,0,2,0,0";
+
+	if (setting) {
+		char xdrv_buf[64];
+
+		/* configure source */
+		snprintf(xdrv_buf, sizeof(xdrv_buf), "AT+XDRV=40,4,%d,%d,%s,%s",
+						4,
+						0,
+						setting,
+						enable ? "2,5" : "0,0");
+		g_at_chat_send(ctmd->chat, xdrv_buf, xdrv_prefix, NULL, NULL,
+				NULL);
+
+		/* configure destination */
+		snprintf(xdrv_buf, sizeof(xdrv_buf), "AT+XDRV=40,5,%d,%d,%s,%s",
+						3,
+						0,
+						setting,
+						enable ? "2,6" : "0,0");
+
+		g_at_chat_send(ctmd->chat, xdrv_buf, xdrv_prefix, NULL, NULL,
+				NULL);
+	}
+
+	cb(&error, cbd->data);
+}
+
+static void ifx_set_tty(struct ofono_ctm *ctm, ofono_bool_t enable,
+				ofono_ctm_set_cb_t cb, void *data)
+{
+	struct ctm_data *ctmd = ofono_ctm_get_data(ctm);
+	struct cb_data *cbd = cb_data_new(cb, data);
+	char buf[20];
+
+	if (cbd == NULL)
+		goto error;
+
+	/* Only FULL TTY mode enabled/disabled */
+	snprintf(buf, sizeof(buf), "AT+XCTMS=%i", enable ? 1 : 0);
+
+	if (g_at_chat_send(ctmd->chat, buf, none_prefix,
+				xctms_modify_cb, cbd, g_free) > 0) {
+		ctmd->enable = enable;
+		ofono_ctm_set_data(ctm, ctmd);
+		cbd->user = ctm;
+	return;
+	}
+
+error:
+	g_free(cbd);
+
+	CALLBACK_WITH_FAILURE(cb, data);
+}
+
+static void xctms_support_cb(gboolean ok, GAtResult *result, gpointer user_data)
+{
+	struct ofono_ctm *ctm = user_data;
+
+	if (!ok)
+		ofono_ctm_remove(ctm);
+	else
+		ofono_ctm_register(ctm);
+}
+
+static int ifx_ctm_probe(struct ofono_ctm *ctm,
+				unsigned int vendor, void *data)
+{
+	struct ofono_modem *modem;
+	GAtChat *chat = data;
+	struct ctm_data *ctmd;
+
+	ctmd = g_try_new0(struct ctm_data, 1);
+	if (ctmd == NULL)
+		return -ENOMEM;
+
+	modem = ofono_ctm_get_modem(ctm);
+	ctmd->audio_setting = ofono_modem_get_string(modem, "AudioSetting");
+	ctmd->chat = g_at_chat_clone(chat);
+
+	ofono_ctm_set_data(ctm, ctmd);
+
+	g_at_chat_send(ctmd->chat, "AT+XCTMS=?", xctms_prefix,
+			xctms_support_cb, ctm, NULL);
+
+	return 0;
+}
+
+static void ifx_ctm_remove(struct ofono_ctm *ctm)
+{
+	struct ctm_data *ctmd = ofono_ctm_get_data(ctm);
+
+	ofono_ctm_set_data(ctm, NULL);
+
+	g_at_chat_unref(ctmd->chat);
+	g_free(ctmd);
+}
+
+static struct ofono_ctm_driver driver = {
+	.name           = "ifxmodem",
+	.probe          = ifx_ctm_probe,
+	.remove         = ifx_ctm_remove,
+	.query_tty      = ifx_query_tty,
+	.set_tty        = ifx_set_tty,
+};
+
+void ifx_ctm_init()
+{
+	ofono_ctm_driver_register(&driver);
+}
+
+void ifx_ctm_exit()
+{
+	ofono_ctm_driver_unregister(&driver);
+}
diff --git a/drivers/ifxmodem/ifxmodem.c b/drivers/ifxmodem/ifxmodem.c
index 8a9ac8f..fecb221 100644
--- a/drivers/ifxmodem/ifxmodem.c
+++ b/drivers/ifxmodem/ifxmodem.c
@@ -39,6 +39,7 @@ static int ifxmodem_init(void)
 	ifx_radio_settings_init();
 	ifx_gprs_context_init();
 	ifx_stk_init();
+	ifx_ctm_init();
 
 	return 0;
 }
@@ -50,6 +51,7 @@ static void ifxmodem_exit(void)
 	ifx_radio_settings_exit();
 	ifx_audio_settings_exit();
 	ifx_voicecall_exit();
+	ifx_ctm_exit();
 }
 
 OFONO_PLUGIN_DEFINE(ifxmodem, "Infineon modem driver", VERSION,
diff --git a/drivers/ifxmodem/ifxmodem.h b/drivers/ifxmodem/ifxmodem.h
index 8ea52e5..1bd58e8 100644
--- a/drivers/ifxmodem/ifxmodem.h
+++ b/drivers/ifxmodem/ifxmodem.h
@@ -35,3 +35,6 @@ extern void ifx_gprs_context_exit();
 
 extern void ifx_stk_init();
 extern void ifx_stk_exit();
+
+extern void ifx_ctm_init();
+extern void ifx_ctm_exit();
-- 
1.7.0.4

Re: [PATCH 2/2] ifxmodem: add enable/disable ctm support

[Marcel Holtmann]

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

Hi Jeevaka,

>  Makefile.am                 |    3 +-
>  drivers/ifxmodem/ctm.c      |  243 +++++++++++++++++++++++++++++++++++++++++++
>  drivers/ifxmodem/ifxmodem.c |    2 +
>  drivers/ifxmodem/ifxmodem.h |    3 +
>  4 files changed, 250 insertions(+), 1 deletions(-)
>  create mode 100644 drivers/ifxmodem/ctm.c
> 
> diff --git a/Makefile.am b/Makefile.am
> index 8ad01cd..e6494b1 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -222,7 +222,8 @@ builtin_sources += drivers/atmodem/atutil.h \
>  			drivers/ifxmodem/audio-settings.c \
>  			drivers/ifxmodem/radio-settings.c \
>  			drivers/ifxmodem/gprs-context.c \
> -			drivers/ifxmodem/stk.c
> +			drivers/ifxmodem/stk.c \
> +			drivers/ifxmodem/ctm.c
>  
>  builtin_modules += stemodem
>  builtin_sources += drivers/atmodem/atutil.h \
> diff --git a/drivers/ifxmodem/ctm.c b/drivers/ifxmodem/ctm.c
> new file mode 100644
> index 0000000..17c7f5c
> --- /dev/null
> +++ b/drivers/ifxmodem/ctm.c
> @@ -0,0 +1,243 @@
> +/*
> + *
> + *  oFono - Open Source Telephony
> + *
> + *  Copyright (C) 2008-2010  Intel Corporation. All rights reserved.
> + *
> + *  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
> + *  published by the Free Software Foundation.
> + *
> + *  This program is distributed in the hope that it will be useful,
> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + *  GNU General Public License for more details.
> + *
> + *  You should have received a copy of the GNU General Public License
> + *  along with this program; if not, write to the Free Software
> + *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
> + *
> + */
> +
> +#ifdef HAVE_CONFIG_H
> +#include <config.h>
> +#endif
> +
> +#define _GNU_SOURCE
> +#include <string.h>
> +#include <stdlib.h>
> +#include <stdio.h>
> +#include <errno.h>
> +
> +#include <glib.h>
> +
> +#include <ofono/log.h>
> +#include <ofono/modem.h>
> +#include <ofono/ctm.h>
> +
> +#include "gatchat.h"
> +#include "gatresult.h"
> +
> +#include "ifxmodem.h"
> +
> +static const char *none_prefix[] = { NULL };
> +static const char *xctms_prefix[] = { "XCTMS:", NULL };
> +static const char *xdrv_prefix[] = { "XDRV:", NULL };
> +
> +struct ctm_data {
> +	GAtChat *chat;
> +	const char * audio_setting;

you have an extra space between * and audio_setting. Please remove that.

> +	ofono_bool_t enable;
> +};
> +
> +static void xctms_query_cb(gboolean ok, GAtResult *result, gpointer user_data)
> +{
> +	struct cb_data *cbd = user_data;
> +	ofono_ctm_query_cb_t cb = cbd->cb;
> +	struct ofono_error error;
> +	GAtResultIter iter;
> +	int value;
> +	ofono_bool_t enable;
> +
> +	decode_at_error(&error, g_at_result_final_response(result));
> +
> +	if (!ok) {
> +		cb(&error, -1, cbd->data);
> +		return;
> +	}
> +
> +	g_at_result_iter_init(&iter, result);
> +
> +	if (g_at_result_iter_next(&iter, "XCTMS:") == FALSE)
> +		goto error;
> +
> +	if (g_at_result_iter_next_number(&iter, &value) == FALSE)
> +		goto error;
> +
> +	/* FULL TTY mode status only sent to oFono */
> +	enable = (value == 1) ? TRUE : FALSE;
> +
> +	cb(&error, enable, cbd->data);
> +
> +	return;
> +
> +error:
> +	CALLBACK_WITH_FAILURE(cb, -1, cbd->data);
> +}
> +
> +static void ifx_query_tty(struct ofono_ctm *ctm, ofono_ctm_query_cb_t cb,
> +				void *data)
> +{
> +	struct ctm_data *ctmd = ofono_ctm_get_data(ctm);
> +	struct cb_data *cbd = cb_data_new(cb, data);
> +
> +	if (cbd == NULL)
> +		goto error;
> +
> +	if (g_at_chat_send(ctmd->chat, "AT+XCTMS?", xctms_prefix,
> +				xctms_query_cb, cbd, g_free) > 0)
> +		return;
> +
> +error:
> +	g_free(cbd);
> +
> +	CALLBACK_WITH_FAILURE(cb, -1, data);
> +}
> +
> +static void xctms_modify_cb(gboolean ok, GAtResult *result, gpointer user_data)
> +{
> +	struct cb_data *cbd = user_data;
> +	ofono_ctm_set_cb_t cb = cbd->cb;
> +	struct ofono_error error;
> +	const char *setting = NULL;
> +	struct ofono_ctm *ctm = cbd->user;
> +	struct ctm_data *ctmd = ofono_ctm_get_data(ctm);
> +	ofono_bool_t enable = ctmd->enable;
> +
> +	decode_at_error(&error, g_at_result_final_response(result));
> +
> +	if (!ok) {
> +		cb(&error, cbd->data);
> +		return;
> +	}
> +
> +	if (g_strcmp0(ctmd->audio_setting, "FULL_DUPLEX") == 0)
> +		setting = "0,0,0,0,0,0,0";
> +	else if (g_strcmp0(ctmd->audio_setting, "BURSTMODE_48KHZ") == 0)
> +		setting = "0,0,8,0,2,0,0";
> +	else if (g_strcmp0(ctmd->audio_setting, "BURSTMODE_96KHZ") == 0)
> +		setting = "0,0,9,0,2,0,0";
> +
> +	if (setting) {
> +		char xdrv_buf[64];
> +
> +		/* configure source */
> +		snprintf(xdrv_buf, sizeof(xdrv_buf), "AT+XDRV=40,4,%d,%d,%s,%s",
> +						4,
> +						0,
> +						setting,
> +						enable ? "2,5" : "0,0");
> +		g_at_chat_send(ctmd->chat, xdrv_buf, xdrv_prefix, NULL, NULL,
> +				NULL);
> +
> +		/* configure destination */
> +		snprintf(xdrv_buf, sizeof(xdrv_buf), "AT+XDRV=40,5,%d,%d,%s,%s",
> +						3,
> +						0,
> +						setting,
> +						enable ? "2,6" : "0,0");
> +
> +		g_at_chat_send(ctmd->chat, xdrv_buf, xdrv_prefix, NULL, NULL,
> +				NULL);
> +	}

Now this is something I don't like at all. It is copied code from the
modem plugin.

The initial discussion was that we need to configure XDRV only once
during init and never have to touch it again. That seems to be not true
anymore. So what is the deal here?

Also if this is required, we might need to figure out a complete
different way of handling this. We can't have this in two places since
that means a full disconnect. Maybe putting this into the audio settings
atom might be better. However before we can do anything, I have to
understand the semantics behind XDRV, normal voice calls and TTY calls.

> +
> +	cb(&error, cbd->data);
> +}
> +
> +static void ifx_set_tty(struct ofono_ctm *ctm, ofono_bool_t enable,
> +				ofono_ctm_set_cb_t cb, void *data)
> +{
> +	struct ctm_data *ctmd = ofono_ctm_get_data(ctm);
> +	struct cb_data *cbd = cb_data_new(cb, data);
> +	char buf[20];
> +
> +	if (cbd == NULL)
> +		goto error;
> +
> +	/* Only FULL TTY mode enabled/disabled */
> +	snprintf(buf, sizeof(buf), "AT+XCTMS=%i", enable ? 1 : 0);
> +
> +	if (g_at_chat_send(ctmd->chat, buf, none_prefix,
> +				xctms_modify_cb, cbd, g_free) > 0) {
> +		ctmd->enable = enable;
> +		ofono_ctm_set_data(ctm, ctmd);

What is this ctm_set_data doing here. It seems wrong.

> +		cbd->user = ctm;
> +	return;

This return is a bit out of order.

> +	}
> +
> +error:
> +	g_free(cbd);
> +
> +	CALLBACK_WITH_FAILURE(cb, data);
> +}
> +
> +static void xctms_support_cb(gboolean ok, GAtResult *result, gpointer user_data)
> +{
> +	struct ofono_ctm *ctm = user_data;
> +
> +	if (!ok)
> +		ofono_ctm_remove(ctm);
> +	else
> +		ofono_ctm_register(ctm);

Don't bother with the remove here. We have not been doing that. So just
registering in success case is enough.

> +}
> +
> +static int ifx_ctm_probe(struct ofono_ctm *ctm,
> +				unsigned int vendor, void *data)
> +{
> +	struct ofono_modem *modem;
> +	GAtChat *chat = data;
> +	struct ctm_data *ctmd;
> +
> +	ctmd = g_try_new0(struct ctm_data, 1);
> +	if (ctmd == NULL)
> +		return -ENOMEM;
> +
> +	modem = ofono_ctm_get_modem(ctm);
> +	ctmd->audio_setting = ofono_modem_get_string(modem, "AudioSetting");
> +	ctmd->chat = g_at_chat_clone(chat);
> +
> +	ofono_ctm_set_data(ctm, ctmd);
> +
> +	g_at_chat_send(ctmd->chat, "AT+XCTMS=?", xctms_prefix,
> +			xctms_support_cb, ctm, NULL);
> +
> +	return 0;
> +}
> +
> +static void ifx_ctm_remove(struct ofono_ctm *ctm)
> +{
> +	struct ctm_data *ctmd = ofono_ctm_get_data(ctm);
> +
> +	ofono_ctm_set_data(ctm, NULL);
> +
> +	g_at_chat_unref(ctmd->chat);
> +	g_free(ctmd);
> +}
> +
> +static struct ofono_ctm_driver driver = {
> +	.name           = "ifxmodem",
> +	.probe          = ifx_ctm_probe,
> +	.remove         = ifx_ctm_remove,
> +	.query_tty      = ifx_query_tty,
> +	.set_tty        = ifx_set_tty,
> +};
> +
> +void ifx_ctm_init()
> +{

We wanna be consistent, so pleace do ifx_ctm_init(void) here.

> +	ofono_ctm_driver_register(&driver);
> +}
> +
> +void ifx_ctm_exit()
> +{

Same here. And for extra bonus points, I accept patches that fixes this
inside the whole oFono tree ;)

It is coding style rule M15 now.

Regards

Marcel

Re: [PATCH 1/2] ctm: add ofono_ctm_get_modem interface

[Marcel Holtmann]

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

Hi Jeevaka,

>  include/ctm.h |    2 ++
>  src/ctm.c     |    5 +++++
>  2 files changed, 7 insertions(+), 0 deletions(-)
> 
> diff --git a/include/ctm.h b/include/ctm.h
> index 5305469..71e3d61 100644
> --- a/include/ctm.h
> +++ b/include/ctm.h
> @@ -59,6 +59,8 @@ void ofono_ctm_remove(struct ofono_ctm *ctm);
>  void ofono_ctm_set_data(struct ofono_ctm *ctm, void *data);
>  void *ofono_ctm_get_data(struct ofono_ctm *ctm);
>  
> +struct ofono_modem *ofono_ctm_get_modem(struct ofono_ctm *ctm);
> +

in principle this is fine, but I wait for the result of the audio
discussion to see if we really need this.

Regards

Marcel

RE: [PATCH 2/2] ifxmodem: add enable/disable ctm support

[Jeevaka.Badrappan]

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

Hi Marcel,

ofono-bounces(a)ofono.org wrote:
>> +static void xctms_modify_cb(gboolean ok, GAtResult *result,
>> gpointer +user_data) { +	struct cb_data *cbd = user_data;
>> +	ofono_ctm_set_cb_t cb = cbd->cb;
>> +	struct ofono_error error;
>> +	const char *setting = NULL;
>> +	struct ofono_ctm *ctm = cbd->user;
>> +	struct ctm_data *ctmd = ofono_ctm_get_data(ctm);
>> +	ofono_bool_t enable = ctmd->enable;
>> +
>> +	decode_at_error(&error, g_at_result_final_response(result)); +
>> +	if (!ok) {
>> +		cb(&error, cbd->data);
>> +		return;
>> +	}
>> +
>> +	if (g_strcmp0(ctmd->audio_setting, "FULL_DUPLEX") == 0) +
setting
>> = "0,0,0,0,0,0,0"; +	else if (g_strcmp0(ctmd->audio_setting,
>> "BURSTMODE_48KHZ") == 0) +		setting = "0,0,8,0,2,0,0"; +
else if
>> (g_strcmp0(ctmd->audio_setting, "BURSTMODE_96KHZ") == 0) +
setting
>> = "0,0,9,0,2,0,0"; + +	if (setting) {
>> +		char xdrv_buf[64];
>> +
>> +		/* configure source */
>> +		snprintf(xdrv_buf, sizeof(xdrv_buf),
"AT+XDRV=40,4,%d,%d,%s,%s",
>> +						4, +
0,
>> +						setting,
>> +						enable ? "2,5" : "0,0");
>> +		g_at_chat_send(ctmd->chat, xdrv_buf, xdrv_prefix, NULL,
NULL,
>> +				NULL); +
>> +		/* configure destination */
>> +		snprintf(xdrv_buf, sizeof(xdrv_buf),
"AT+XDRV=40,5,%d,%d,%s,%s",
>> +						3, +
0,
>> +						setting,
>> +						enable ? "2,6" : "0,0");
>> +
>> +		g_at_chat_send(ctmd->chat, xdrv_buf, xdrv_prefix, NULL,
NULL,
>> +				NULL); +	}
> 
> Now this is something I don't like at all. It is copied code from the
> modem plugin. 

Its the same audio configuration code except that there is a new
parameter added
at the end of the parameter list for TTY case.

> 
> The initial discussion was that we need to configure XDRV
> only once during init and never have to touch it again. That
> seems to be not true anymore. So what is the deal here?
> 

Audio source/destination parameter includes configuration and transducer
mode

TRANSDUCER is the difference. Incase of voice call it is set to
default(0) whereas
for TTY call it is set to TRANSDUCER TTY( 5 for source and 6 for
destination).

TTY call - Last 2 parameters are 2,5 and 2,6 for the source(uplink) and
destination(downlink) respectively.
Voice call - Last 2 parameters are 0,0.

> Also if this is required, we might need to figure out a
> complete different way of handling this. We can't have this
> in two places since that means a full disconnect. Maybe
> putting this into the audio settings atom might be better.
> However before we can do anything, I have to understand the
> semantics behind XDRV, normal voice calls and TTY calls.
> 

Correct me if I'm wrong. If we move this to the audio settings atom,
then I'm
afraid that it will end up in used by only ifx modem. 

> 
> We wanna be consistent, so pleace do ifx_ctm_init(void) here.
> 
>> +	ofono_ctm_driver_register(&driver);
>> +}
>> +
>> +void ifx_ctm_exit()
>> +{
> 
> Same here. And for extra bonus points, I accept patches that
> fixes this inside the whole oFono tree ;)
> 
> It is coding style rule M15 now.
> 

Separate set of patches sent for the M15 coding style rule fix for the
whole oFono tree.

Regards,
Jeevaka

RE: [PATCH 2/2] ifxmodem: add enable/disable ctm support

[Marcel Holtmann]

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

Hi Jeevaka,

> > Now this is something I don't like at all. It is copied code from the
> > modem plugin. 
> 
> Its the same audio configuration code except that there is a new
> parameter added
> at the end of the parameter list for TTY case.

I really hate duplicating magic numbers in two places. Can we do this
without +XDRV for now and put a /* TODO mark */ in the code. I do need
to think about this audio settings handling a bit more.

So you might have to keep the +XDRV local in your code for testing, but
I'd rather get the other TTY logic in place and worry about the audio
stuff in a second round of patches.

> > Also if this is required, we might need to figure out a
> > complete different way of handling this. We can't have this
> > in two places since that means a full disconnect. Maybe
> > putting this into the audio settings atom might be better.
> > However before we can do anything, I have to understand the
> > semantics behind XDRV, normal voice calls and TTY calls.
> > 
> 
> Correct me if I'm wrong. If we move this to the audio settings atom,
> then I'm
> afraid that it will end up in used by only ifx modem. 

I am not following. This whole stuff is IFX specific. So yes, it will
only be used by IFX. All other vendors have to do their own stuff.

Regards

Marcel