Re: [PATCH v2] ifx: Adding modem selftest to Infineon modem
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, a...@rtc:rtc_gti_test_verify_32khz() },
+ { Device Version Test, 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
___
ofono mailing list
ofono@ofono.org
http://lists.ofono.org/listinfo/ofono
Re: [PATCH v2 8/9] udev: Add cdmagen
Hi Dara, struct ofono_modem *modem; @@ -533,6 +552,8 @@ done: add_isi(modem, udev_device); else if (g_strcmp0(driver, n900) == 0) add_isi(modem, udev_device); + else if (g_strcmp0(driver, cdmagen) == 0) + add_cdmagen(modem, udev_device); } this is actually wrong. The generic drivers running against phonesim should not be done via udev. We did remove atgen for a reason actually since it was not really useful. Even the isigen driver needs to be renamed into isiusb or usbpn in the long term. Regards Marcel ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
Re: [PATCH v2 7/9] ofono-rules: Add cdmagen device
Hi Dara,
plugins/ofono.rules |4
1 files changed, 4 insertions(+), 0 deletions(-)
diff --git a/plugins/ofono.rules b/plugins/ofono.rules
index da8a8ef..34698f0 100644
--- a/plugins/ofono.rules
+++ b/plugins/ofono.rules
@@ -429,6 +429,10 @@ ATTRS{idVendor}==0930, ATTRS{idProduct}==1311,
ENV{OFONO_DRIVER}=mbm
# Nokia Internet Stick CS-10
ATTRS{idVendor}==0421, ATTRS{idProduct}==060e, ENV{OFONO_DRIVER}=nokia
+# Nokia CDMA Device
+ATTRS{idVendor}==0421, ATTRS{idProduct}==023e,
ENV{OFONO_DRIVER}=cdmagen
+ATTRS{idVendor}==0421, ATTRS{idProduct}==00b6,
ENV{OFONO_DRIVER}=cdmagen
+
since this is Nokia specific device, then using nokiacdma would be a
better name. Or using OFONO_DRIVER=nokia NOKIA_MODE=cdma and sharing the
driver might be useful.
It really all depends how much they have in common. So what kind of
hardware is this actually? A phone or a real USB dongle?
Regards
Marcel
___
ofono mailing list
ofono@ofono.org
http://lists.ofono.org/listinfo/ofono
RE: [PATCH v2] ifx: Adding modem selftest to Infineon modem
@@ -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.
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
___
ofono mailing list
ofono@ofono.org
http://lists.ofono.org/listinfo/ofono
Re: [PATCH v2] ifx: Adding modem selftest to Infineon modem
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
___
ofono mailing list
ofono@ofono.org
http://lists.ofono.org/listinfo/ofono
