Hi Marcel, I am resending this email as MS outlook was fast before I could format the message. Sorry for that.
-----Original Message----- From: Ramakrishna, AnandX Sent: Wednesday, February 02, 2011 10:08 AM To: [email protected] Subject: RE: [PATCH v8] ifx: Adding modem selftest for Infineon modem 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
