[PATCH v2] ifx: Adding modem selftest to Infineon modem

[PATCH v2] ifx: Adding modem selftest to Infineon modem

[Robertino Benis]

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

Hi all,

This is a second attempt to submit a patch that triggers Infineon
modem selftest during ofono boot. Patch addresses issues raised
by Marcel from the previous submission.

Thank you,
-- r.


---
 plugins/ifx.c |   87 +++++++++++++++++++++++++++++++++++++++++++++++++++++----
 1 files changed, 81 insertions(+), 6 deletions(-)

diff --git a/plugins/ifx.c b/plugins/ifx.c
index 2f4c65b..3647dbc 100644
--- a/plugins/ifx.c
+++ b/plugins/ifx.c
@@ -71,6 +71,8 @@
 #define GPRS3_DLC   4
 #define AUX_DLC     5
 
+#define IFX_SELF_TESTS_TIMEOUT	5
+
 static char *dlc_prefixes[NUM_DLC] = { "Voice: ", "Net: ", "GPRS1: ",
 					"GPRS2: ", "GPRS3: ", "Aux: " };
 
@@ -80,6 +82,18 @@ static const char *dlc_nodes[NUM_DLC] = { "/dev/ttyGSM1", "/dev/ttyGSM2",
 
 static const char *none_prefix[] = { NULL };
 static const char *xdrv_prefix[] = { "+XDRV:", NULL };
+static const char *empty_prefix[] = { "", NULL };
+
+struct ifx_self_tests {
+	char *test_desc;
+	char *at_cmd;
+};
+
+static const struct ifx_self_tests mst[] = {
+		{ "RTC GTI Test", "at(a)rtc:rtc_gti_test_verify_32khz()" },
+		{ "Device Version Test", "at(a)vers:device_version_id()" },
+		{ NULL, NULL }
+};
 
 struct ifx_data {
 	GIOChannel *device;
@@ -99,6 +113,8 @@ struct ifx_data {
 	int audio_loopback;
 	struct ofono_sim *sim;
 	gboolean have_sim;
+	guint self_test_timeout;
+	int self_test_idx;
 };
 
 static void ifx_debug(const char *str, void *user_data)
@@ -545,6 +561,64 @@ static gboolean mux_timeout_cb(gpointer user_data)
 	return FALSE;
 }
 
+static gboolean self_test_timeout_cb(gpointer user_data)
+{
+	struct ofono_modem *modem = user_data;
+	struct ifx_data *data = ofono_modem_get_data(modem);
+
+	ofono_error("Modem %s: TIMED OUT",
+			mst[data->self_test_idx].test_desc);
+
+	g_source_remove(data->self_test_timeout);
+	data->self_test_timeout = 0;
+
+	shutdown_device(data);
+	ofono_modem_set_powered(modem, FALSE);
+
+	return FALSE;
+}
+
+static void ifx_self_test_cb(gboolean ok, GAtResult *result,
+				gpointer user_data)
+{
+	struct ofono_modem *modem = user_data;
+	struct ifx_data *data = ofono_modem_get_data(modem);
+
+	if (data->self_test_timeout > 0) {
+		g_source_remove(data->self_test_timeout);
+		data->self_test_timeout = 0;
+	}
+
+	if (!ok) {
+		ofono_error("Modem %s: FAIL",
+			mst[data->self_test_idx].test_desc);
+		shutdown_device(data);
+		ofono_modem_set_powered(modem, FALSE);
+
+		return;
+	}
+
+	data->self_test_idx++;
+
+	if (mst[data->self_test_idx].at_cmd != NULL) {
+		g_at_chat_send(data->dlcs[AUX_DLC],
+			mst[data->self_test_idx].at_cmd,
+			empty_prefix, ifx_self_test_cb, modem, NULL);
+
+		data->self_test_timeout = g_timeout_add_seconds(
+			IFX_SELF_TESTS_TIMEOUT, self_test_timeout_cb, modem);
+
+	} else {	/* Enable  MUX Channels */
+		data->frame_size = 1509;
+		g_at_chat_send(data->dlcs[AUX_DLC],
+				"AT+CMUX=0,0,,1509,10,3,30,,", NULL,
+				mux_setup_cb, modem, NULL);
+
+		data->mux_init_timeout = g_timeout_add_seconds(5,
+				mux_timeout_cb,	modem);
+	}
+}
+
 static int ifx_enable(struct ofono_modem *modem)
 {
 	struct ifx_data *data = ofono_modem_get_data(modem);
@@ -598,15 +672,16 @@ static int ifx_enable(struct ofono_modem *modem)
 	g_at_chat_send(chat, "ATE0 +CMEE=1", NULL,
 					NULL, NULL, NULL);
 
-	data->frame_size = 1509;
+	/* Execute Modem Self tests */
 
-	g_at_chat_send(chat, "AT+CMUX=0,0,,1509,10,3,30,,", NULL,
-					mux_setup_cb, modem, NULL);
+	data->dlcs[AUX_DLC] = chat;
+	data->self_test_idx = 0;
 
-	data->mux_init_timeout = g_timeout_add_seconds(5, mux_timeout_cb,
-								modem);
+	g_at_chat_send(data->dlcs[AUX_DLC], mst[data->self_test_idx].at_cmd,
+		empty_prefix, ifx_self_test_cb, modem, NULL);
 
-	data->dlcs[AUX_DLC] = chat;
+	data->self_test_timeout = g_timeout_add_seconds(IFX_SELF_TESTS_TIMEOUT,
+				self_test_timeout_cb,	modem);
 
 	return -EINPROGRESS;
 }
-- 
1.7.0.4

Re: [PATCH v2] ifx: Adding modem selftest to Infineon modem

[Marcel Holtmann]

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

Hi Robertino,

> diff --git a/plugins/ifx.c b/plugins/ifx.c
> index 2f4c65b..3647dbc 100644
> --- a/plugins/ifx.c
> +++ b/plugins/ifx.c
> @@ -71,6 +71,8 @@
>  #define GPRS3_DLC   4
>  #define AUX_DLC     5
>  
> +#define IFX_SELF_TESTS_TIMEOUT	5
> +

this is not what I meant actually. Adding a comment for the
g_timeout_add_seconds is just fine.

What I wanted to know is where the actual value came from. Is 5 seconds
enough? How do we know the time a test takes?

>  static char *dlc_prefixes[NUM_DLC] = { "Voice: ", "Net: ", "GPRS1: ",
>  					"GPRS2: ", "GPRS3: ", "Aux: " };
>  
> @@ -80,6 +82,18 @@ static const char *dlc_nodes[NUM_DLC] = { "/dev/ttyGSM1", "/dev/ttyGSM2",
>  
>  static const char *none_prefix[] = { NULL };
>  static const char *xdrv_prefix[] = { "+XDRV:", NULL };
> +static const char *empty_prefix[] = { "", NULL };

This is still wrong. See my comment from the original email. I am not
sure you really know what { "", NULL } means.

> +struct ifx_self_tests {
> +	char *test_desc;
> +	char *at_cmd;
> +};
> +
> +static const struct ifx_self_tests mst[] = {
> +		{ "RTC GTI Test", "at(a)rtc:rtc_gti_test_verify_32khz()" },
> +		{ "Device Version Test", "at(a)vers:device_version_id()" },
> +		{ NULL, NULL }
> +};

No need to declare the struct before assigning it. Just do it all in
once. And fix up the indentation for the values. Just one tab please.
 
>  struct ifx_data {
>  	GIOChannel *device;
> @@ -99,6 +113,8 @@ struct ifx_data {
>  	int audio_loopback;
>  	struct ofono_sim *sim;
>  	gboolean have_sim;
> +	guint self_test_timeout;
> +	int self_test_idx;
>  };
>  
>  static void ifx_debug(const char *str, void *user_data)
> @@ -545,6 +561,64 @@ static gboolean mux_timeout_cb(gpointer user_data)
>  	return FALSE;
>  }
>  
> +static gboolean self_test_timeout_cb(gpointer user_data)
> +{
> +	struct ofono_modem *modem = user_data;
> +	struct ifx_data *data = ofono_modem_get_data(modem);
> +
> +	ofono_error("Modem %s: TIMED OUT",
> +			mst[data->self_test_idx].test_desc);
> +
> +	g_source_remove(data->self_test_timeout);
> +	data->self_test_timeout = 0;
> +
> +	shutdown_device(data);
> +	ofono_modem_set_powered(modem, FALSE);
> +
> +	return FALSE;
> +}

I am stopping right here now actually. Please fix my review comments
from the initial review as well. This function is still broken.

Regards

Marcel

RE: [PATCH v2] ifx: Adding modem selftest to Infineon modem

[Bastian, Waldo]

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

> > @@ -80,6 +82,18 @@ static const char *dlc_nodes[NUM_DLC] = { "/dev/ttyGSM1", "/dev/ttyGSM2",
> >  
> >  static const char *none_prefix[] = { NULL };
> >  static const char *xdrv_prefix[] = { "+XDRV:", NULL };
> > +static const char *empty_prefix[] = { "", NULL };
> 
> This is still wrong. See my comment from the original email. I am not
> sure you really know what { "", NULL } means.

The response doesn't really have a proper prefix (see example below), this way the entire response ends up in the callback. Is there a better way? Given that there aren't any URC registered yet, it will not interfere with any other response. It's obviously not a good idea to use it once the modem is powered up.

at(a)vers:device_version_id()
CHIP ID = XXX
FLASH TYPE = YYY
FLASH ID = 0x898881
SmartiUE2 = 37664
RF PMU = 10548
PA PMU = 40244
RF ASM = 0
FEM PA = 21812
OK

Cheers,
Waldo

Re: [PATCH v2] ifx: Adding modem selftest to Infineon modem

[Denis Kenzior]

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

Hi Waldo,

On 12/18/2010 01:58 PM, Bastian, Waldo wrote:
>>> @@ -80,6 +82,18 @@ static const char *dlc_nodes[NUM_DLC] = { "/dev/ttyGSM1", "/dev/ttyGSM2",
>>>  
>>>  static const char *none_prefix[] = { NULL };
>>>  static const char *xdrv_prefix[] = { "+XDRV:", NULL };
>>> +static const char *empty_prefix[] = { "", NULL };
>>
>> This is still wrong. See my comment from the original email. I am not
>> sure you really know what { "", NULL } means.
> 
> The response doesn't really have a proper prefix (see example below), this way the entire response ends up in the callback. Is there a better way? Given that there aren't any URC registered yet, it will not interfere with any other response. It's obviously not a good idea to use it once the modem is powered up.
> 

Then simply passing NULL instead of empty_prefix is sufficient.

Regards,
-Denis