Hi Marcel, I am replying to this email behalf of Robertino.
I appreciate if you can review my comments/proposed solution before I submit the next patch. -----Original Message----- From: [email protected] [mailto:[email protected]] On Behalf Of Marcel Holtmann Sent: Tuesday, February 01, 2011 11:30 PM To: [email protected] Subject: Re: [PATCH v8] ifx: Adding modem selftest for Infineon modem Hi Robertino, > Infineon modem selftest, during ifx_enable(). > Two steps trigger, with timeout. In case one > fails, modem will not power up. > > --- > plugins/ifx.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++------ > 1 files changed, 51 insertions(+), 7 deletions(-) > > diff --git a/plugins/ifx.c b/plugins/ifx.c > index 411c012..0250f64 100644 > --- a/plugins/ifx.c > +++ b/plugins/ifx.c > @@ -524,21 +524,57 @@ static gboolean mux_timeout_cb(gpointer user_data) > struct ofono_modem *modem = user_data; > struct ifx_data *data = ofono_modem_get_data(modem); > > - ofono_error("Timeout with multiplexer setup"); > + ofono_error("Timeout with modem and multiplexer setup"); > > data->mux_init_timeout = 0; > > - g_at_chat_unref(data->dlcs[AUX_DLC]); > - data->dlcs[AUX_DLC] = NULL; > + if(data->dlcs[AUX_DLC]) { > + g_at_chat_unref(data->dlcs[AUX_DLC]); > + data->dlcs[AUX_DLC] = NULL; > + } > > - g_io_channel_unref(data->device); > - data->device = NULL; > + if(data->device) { > + g_io_channel_unref(data->device); > + data->device = NULL; > + } what are these suppose to be doing? This is in the timeout handler and the timeout will only ever be triggered if data->dlcs[AUX_DLC] is actually valid. If it gets triggered otherwise then you forgot to disable the timer. So these should not be needed. Proposed solution in the selftest callback: static void rtc_gti_selftest_cb(gboolean ok, GAtResult *result, gpointer user_data) { struct ofono_modem *modem = user_data; struct ifx_data *data = ofono_modem_get_data(modem); if (ok) return; ofono_error("at@rtc:rtc_gti_test_verify_32khz()-FAILED"); g_at_chat_cancel_all(data->dlcs[AUX_DLC]); if (data->mux_init_timeout > 0) { g_source_remove(data->mux_init_timeout); data->mux_init_timeout = 0; } if(data->dlcs[AUX_DLC]) { g_at_chat_unref(data->dlcs[AUX_DLC]); data->dlcs[AUX_DLC] = NULL; } if(data->device) { g_io_channel_unref(data->device); data->device = NULL; } ofono_modem_set_powered(modem, FALSE); } However, please follow our coding style. It is ... if<space>( ... Will take care > ofono_modem_set_powered(modem, FALSE); > > return FALSE; > } > > +static void dev_ver_selftest_cb(gboolean ok, GAtResult *result, > + gpointer user_data) > +{ > + > + struct ofono_modem *modem = user_data; > + struct ifx_data *data = ofono_modem_get_data(modem); > + > + if (ok) > + return; > + > + ofono_error("ERROR:IFX Selftest at@vers:device_version_id()" > + "-FAILED"); > + g_at_chat_cancel_all(data->dlcs[AUX_DLC]); > + shutdown_device(data); You do need to watch out for such really simple issues with the coding style. Please get them fixed before submitting a patch. Will take care. Now to the fact of using shutdown_device() here. That is not really a good idea. That function can only be used after you have the MUX up and running since it takes care of taking down the MUX or restoring the line discipline. However you have not done that yet. Will not use shutdown_device(), will use proposed solution as mentioned above. > +} > + > +static void rtc_gti_selftest_cb(gboolean ok, GAtResult *result, > + gpointer user_data) > +{ > + struct ofono_modem *modem = user_data; > + struct ifx_data *data = ofono_modem_get_data(modem); > + > + if (ok) > + return; > + > + ofono_error("ERROR:IFX Selftest" > + "at@rtc:rtc_gti_test_verify_32khz()-FAILED"); And I would prefer if you just leave the "ERROR:IFX Selftest" part out here. It is clear that it is an error since ofono_error() says so and also tells syslog about it. Will code as ofono_error("at@rtc:rtc_gti_test_verify_32khz()-FAILED"); > + > + g_at_chat_cancel_all(data->dlcs[AUX_DLC]); > + shutdown_device(data); > +} > + > static int ifx_enable(struct ofono_modem *modem) > { > struct ifx_data *data = ofono_modem_get_data(modem); > @@ -592,13 +628,21 @@ static int ifx_enable(struct ofono_modem *modem) > g_at_chat_send(chat, "ATE0 +CMEE=1", NULL, > NULL, NULL, NULL); > > + /* Execute Modem Self tests */ > + g_at_chat_send(chat, "at@rtc:rtc_gti_test_verify_32khz()", > + NULL, rtc_gti_selftest_cb, modem, NULL); > + > + g_at_chat_send(chat, "at@vers:device_version_id()", > + NULL, dev_ver_selftest_cb, modem, NULL); > + > + /* Enable MUX Channels */ > data->frame_size = 1509; > > g_at_chat_send(chat, "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); > + data->mux_init_timeout = g_timeout_add_seconds( > + 10, mux_timeout_cb, modem); Please keep the original style with 10, mux_timeout_cb in the original line. Do you mean? data->mux_init_timeout = g_timeout_add_seconds(10, mux_timeout_cb, modem); And you might wanna still add the comment about expected execution time of the selftest commands. Will add comments as "total self test execution takes less than 2 secs, used 10 secs to cover both selftest and mux setup time" Regards Marcel Regards, Anand _______________________________________________ ofono mailing list [email protected] http://lists.ofono.org/listinfo/ofono _______________________________________________ ofono mailing list [email protected] http://lists.ofono.org/listinfo/ofono
