From mboxrd@z Thu Jan 1 00:00:00 1970 Content-Type: multipart/mixed; boundary="===============6139908203090280492==" MIME-Version: 1.0 From: Denis Kenzior Subject: Re: [PATCH] Add GPRS, mux, volume control support in SIMCOM sim900 modem module Date: Thu, 28 Feb 2013 10:42:57 -0600 Message-ID: <512F8911.3030100@gmail.com> In-Reply-To: <1362066215-27452-1-git-send-email-r.r.zaripov@gmail.com> List-Id: To: ofono@ofono.org --===============6139908203090280492== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable Hi Renat, On 02/28/2013 09:43 AM, r.r.zaripov(a)gmail.com wrote: > From: Renat Zaripov > > --- > plugins/sim900.c | 162 +++++++++++++++++++++++++++++++++++++++++++++--= ------- > 1 file changed, 137 insertions(+), 25 deletions(-) > Watch those trailing whitespaces: Applying: Add GPRS, mux, volume control support in SIMCOM sim900 modem = module /home/denkenz/ofono-master/.git/rebase-apply/patch:118: trailing whitespace. if(!g_at_mux_start(data->mux)) /home/denkenz/ofono-master/.git/rebase-apply/patch:153: trailing whitespace. = /home/denkenz/ofono-master/.git/rebase-apply/patch:155: trailing whitespace. = /home/denkenz/ofono-master/.git/rebase-apply/patch:186: trailing whitespace. if (data->dlcs[SETUP_DLC] =3D=3D NULL) /home/denkenz/ofono-master/.git/rebase-apply/patch:202: trailing whitespace. g_at_chat_send(data->dlcs[SETUP_DLC], "AT+CMUX=3D0,0,5,128,10,3,30,10,2", warning: squelched 1 whitespace error fatal: 6 lines add whitespace errors. > diff --git a/plugins/sim900.c b/plugins/sim900.c > index 496faa6..f228c0e 100644 > --- a/plugins/sim900.c > +++ b/plugins/sim900.c > @@ -29,6 +29,7 @@ > #include > #include > #include > +#include > > #define OFONO_API_SUBJECT_TO_CHANGE > #include > @@ -44,13 +45,28 @@ > #include > #include > #include > +#include > > #include > > +#define NUM_DLC 5 > + > +#define SETUP_DLC 0 > +#define VOICE_DLC 1 > +#define NETREG_DLC 2 > +#define SMS_DLC 3 > +#define GPRS_DLC 4 > + > + No double empty lines please > +static char *dlc_prefixes[NUM_DLC] =3D { "Voice: ", "Net: ", "SMS: ", "G= PRS: " , "Setup: "}; This line is over 80 characters > + > static const char *none_prefix[] =3D { NULL }; > > struct sim900_data { > - GAtChat *modem; > + GIOChannel *device; > + GAtMux *mux; > + GAtChat *dlcs[NUM_DLC]; > + guint frame_size; > }; > > static int sim900_probe(struct ofono_modem *modem) > @@ -76,7 +92,7 @@ static void sim900_remove(struct ofono_modem *modem) > > ofono_modem_set_data(modem, NULL); > > - g_at_chat_unref(data->modem); > + g_at_chat_unref(data->dlcs[SETUP_DLC]); Why do you only take care of 1 DLC here? You need to clean up all of = them and the mux as well. > > g_free(data); > } > @@ -91,6 +107,7 @@ static void sim900_debug(const char *str, void *user_d= ata) > static GAtChat *open_device(struct ofono_modem *modem, > const char *key, char *debug) > { > + struct sim900_data *data =3D ofono_modem_get_data(modem); > const char *device; > GAtSyntax *syntax; > GIOChannel *channel; > @@ -119,10 +136,33 @@ static GAtChat *open_device(struct ofono_modem *mod= em, > if (channel =3D=3D NULL) > return NULL; > > + data->device =3D channel; > syntax =3D g_at_syntax_new_gsm_permissive(); > chat =3D g_at_chat_new(channel, syntax); > g_at_syntax_unref(syntax); > > + if (chat =3D=3D NULL) > + return NULL; > + > + if (getenv("OFONO_AT_DEBUG")) > + g_at_chat_set_debug(chat, sim900_debug, debug); > + > + return chat; > +} > + > + Again please no double-empty lines > +static GAtChat *create_chat(GIOChannel *channel, struct ofono_modem *mod= em, > + char *debug) > +{ > + GAtSyntax *syntax; > + GAtChat *chat; > + > + if (channel =3D=3D NULL) > + return NULL; > + > + syntax =3D g_at_syntax_new_gsmv1(); > + chat =3D g_at_chat_new(channel, syntax); > + g_at_syntax_unref(syntax); > g_io_channel_unref(channel); > > if (chat =3D=3D NULL) > @@ -134,6 +174,65 @@ static GAtChat *open_device(struct ofono_modem *mode= m, > return chat; > } > > +static void setup_internal_mux(struct ofono_modem *modem) > +{ > + struct sim900_data *data =3D ofono_modem_get_data(modem); > + int i; > + > + DBG(""); > + > + data->frame_size =3D 128; > + > + data->mux =3D g_at_mux_new_gsm0710_basic(data->device, data->frame_size= ); > + if (data->mux =3D=3D NULL) > + goto error; > + > + if (getenv("OFONO_MUX_DEBUG")) > + g_at_mux_set_debug(data->mux, sim900_debug, "MUX: "); > + > + if(!g_at_mux_start(data->mux)) You're not cleaning up the mux here > + goto error; > + > + for (i =3D 0; i< NUM_DLC; i++) { > + GIOChannel *channel =3D g_at_mux_create_channel(data->mux); > + > + data->dlcs[i] =3D create_chat(channel, modem, dlc_prefixes[i]); > + if (data->dlcs[i] =3D=3D NULL) { > + ofono_error("Failed to create channel"); > + goto error; You are not cleaning up channels or the mux here > + } > + } > + > + ofono_modem_set_powered(modem, TRUE); > + > + return; > + > +error: > + ofono_modem_set_powered(modem, FALSE); > +} > + > +static void mux_setup_cb(gboolean ok, GAtResult *result, gpointer user_d= ata) > +{ > + struct ofono_modem *modem =3D user_data; > + struct sim900_data *data =3D ofono_modem_get_data(modem); > + > + DBG(""); > + > + g_at_chat_unref(data->dlcs[SETUP_DLC]); > + data->dlcs[SETUP_DLC] =3D NULL; > + > + if (!ok) > + goto error; > + > + setup_internal_mux(modem); > + = > + return; > + = > +error: > + ofono_modem_set_powered(modem, FALSE); You also need to close the device IO channel. > +} > + > + And again, double empty lines :) > static void cfun_enable(gboolean ok, GAtResult *result, gpointer user_d= ata) > { > struct ofono_modem *modem =3D user_data; > @@ -142,11 +241,9 @@ static void cfun_enable(gboolean ok, GAtResult *resu= lt, gpointer user_data) > DBG(""); > > if (!ok) { > - g_at_chat_unref(data->modem); > - data->modem =3D NULL; > + g_at_chat_unref(data->dlcs[SETUP_DLC]); > + data->dlcs[SETUP_DLC] =3D NULL; > } > - > - ofono_modem_set_powered(modem, ok); > } > > static int sim900_enable(struct ofono_modem *modem) > @@ -155,21 +252,22 @@ static int sim900_enable(struct ofono_modem *modem) > > DBG("%p", modem); > > - data->modem =3D open_device(modem, "Device", "Device: "); > - if (data->modem =3D=3D NULL) { > - DBG("return -EINVAL"); > + data->dlcs[SETUP_DLC] =3D open_device(modem, "Device", "Setup: "); > + if (data->dlcs[SETUP_DLC] =3D=3D NULL) > return -EINVAL; > - } > > - g_at_chat_send(data->modem, "ATE0", NULL, NULL, NULL, NULL); > + g_at_chat_send(data->dlcs[SETUP_DLC], "ATE0", NULL, NULL, NULL, NULL); > > /* For obtain correct sms service number */ > - g_at_chat_send(data->modem, "AT+CSCS=3D\"GSM\"", NULL, > + g_at_chat_send(data->dlcs[SETUP_DLC], "AT+CSCS=3D\"GSM\"", NULL, > NULL, NULL, NULL); > > - g_at_chat_send(data->modem, "AT+CFUN=3D1", none_prefix, > + g_at_chat_send(data->dlcs[SETUP_DLC], "AT+CFUN=3D1", none_prefix, > cfun_enable, modem, NULL); > > + g_at_chat_send(data->dlcs[SETUP_DLC], "AT+CMUX=3D0,0,5,128,10,3,30,10,2= ", > + NULL, mux_setup_cb, modem, NULL); Maybe you should do this from cfun_enable, and call = ofono_modem_set_powered once the mux is setup, not before. > + > return -EINPROGRESS; > } > > @@ -181,8 +279,8 @@ static void cfun_disable(gboolean ok, GAtResult *resu= lt, gpointer user_data) > > DBG(""); > > - g_at_chat_unref(data->modem); > - data->modem =3D NULL; > + g_at_chat_unref(data->dlcs[SETUP_DLC]); > + data->dlcs[SETUP_DLC] =3D NULL; You need to clean up properly here, including all chats, muxer, and = device channel. > > if (ok) > ofono_modem_set_powered(modem, FALSE); > @@ -194,10 +292,10 @@ static int sim900_disable(struct ofono_modem *modem) > > DBG("%p", modem); > > - g_at_chat_cancel_all(data->modem); > - g_at_chat_unregister_all(data->modem); > + g_at_chat_cancel_all(data->dlcs[SETUP_DLC]); > + g_at_chat_unregister_all(data->dlcs[SETUP_DLC]); > > - g_at_chat_send(data->modem, "AT+CFUN=3D4", none_prefix, > + g_at_chat_send(data->dlcs[SETUP_DLC], "AT+CFUN=3D4", none_prefix, > cfun_disable, modem, NULL); > > return -EINPROGRESS; > @@ -210,9 +308,9 @@ static void sim900_pre_sim(struct ofono_modem *modem) > > DBG("%p", modem); > > - ofono_devinfo_create(modem, 0, "atmodem", data->modem); > + ofono_devinfo_create(modem, 0, "atmodem", data->dlcs[VOICE_DLC]); > sim =3D ofono_sim_create(modem, OFONO_VENDOR_SIMCOM, "atmodem", > - data->modem); > + data->dlcs[VOICE_DLC]); > > if (sim) > ofono_sim_inserted_notify(sim, TRUE); > @@ -221,12 +319,25 @@ static void sim900_pre_sim(struct ofono_modem *mode= m) > static void sim900_post_sim(struct ofono_modem *modem) > { > struct sim900_data *data =3D ofono_modem_get_data(modem); > + struct ofono_gprs *gprs; > + struct ofono_gprs_context *gc; > > DBG("%p", modem); > > - ofono_phonebook_create(modem, 0, "atmodem", data->modem); > + ofono_phonebook_create(modem, 0, "atmodem", data->dlcs[VOICE_DLC]); > ofono_sms_create(modem, OFONO_VENDOR_SIMCOM, "atmodem", > - data->modem); > + data->dlcs[SMS_DLC]); > + > + gprs =3D ofono_gprs_create(modem, 0, = > + "atmodem", data->dlcs[GPRS_DLC]); > + if (gprs =3D=3D NULL) > + return; > + > + gc =3D ofono_gprs_context_create(modem, OFONO_VENDOR_SIMCOM, > + "atmodem", data->dlcs[GPRS_DLC]); > + if (gc) > + ofono_gprs_add_context(gprs, gc); > + > } > > static void sim900_post_online(struct ofono_modem *modem) > @@ -235,9 +346,10 @@ static void sim900_post_online(struct ofono_modem *m= odem) > > DBG("%p", modem); > > - ofono_netreg_create(modem, OFONO_VENDOR_SIMCOM, "atmodem", data->modem); > - ofono_ussd_create(modem, 0, "atmodem", data->modem); > - ofono_voicecall_create(modem, 0, "atmodem", data->modem); > + ofono_netreg_create(modem, OFONO_VENDOR_SIMCOM, "atmodem", data->dlcs[V= OICE_DLC]); > + ofono_ussd_create(modem, 0, "atmodem", data->dlcs[VOICE_DLC]); > + ofono_voicecall_create(modem, 0, "atmodem", data->dlcs[VOICE_DLC]); > + ofono_call_volume_create(modem, 0, "atmodem", data->dlcs[VOICE_DLC]); > } > > static struct ofono_modem_driver sim900_driver =3D { Regards, -Denis --===============6139908203090280492==--