From mboxrd@z Thu Jan 1 00:00:00 1970 Content-Type: multipart/mixed; boundary="===============4204857012257120044==" MIME-Version: 1.0 From: Clement Viel Subject: Re: [PATCH 1/3] sim800: add support for sim800 modem. Date: Sat, 03 Nov 2018 00:31:41 +0100 Message-ID: <20181102233140.GA6943@turing> In-Reply-To: <412dac97-dea0-efa5-eda2-e79aa569b99e@gmail.com> List-Id: To: ofono@ofono.org --===============4204857012257120044== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable Hi Denis, Thanks for the quick review ! On Thu, Nov 01, 2018 at 09:53:58AM -0500, Denis Kenzior wrote: > Hi Clement, > = > On 10/15/2018 12:27 PM, Clement Viel wrote: > >--- > > plugins/sim900.c | 89 ++++++++++++++++++++++++++++++++++++++++++++++++= ++------ > > 1 file changed, 80 insertions(+), 9 deletions(-) > > > >diff --git a/plugins/sim900.c b/plugins/sim900.c > >index a7728cd..9f3f334 100644 > >--- a/plugins/sim900.c > >+++ b/plugins/sim900.c > >@@ -24,6 +24,7 @@ > > #endif > > #include > >+#include > > #include > > #include > > #include > >@@ -60,13 +61,59 @@ static char *dlc_prefixes[NUM_DLC] =3D { "Voice: ", = "Net: ", "SMS: ", > > static const char *none_prefix[] =3D { NULL }; > >+typedef enum type { > >+ SIM800, > >+ SIM900, > >+ SIM_UNKNOWN, > >+} type_t; > >+ > > struct sim900_data { > > GIOChannel *device; > > GAtMux *mux; > > GAtChat * dlcs[NUM_DLC]; > > guint frame_size; > >+ type_t modem_type; > > }; > >+static void set_modem_type (struct ofono_modem *modem) > >+{ > >+ struct sim900_data *data =3D ofono_modem_get_data(modem); > >+ const char *path =3D ofono_modem_get_path(modem); > >+ > >+ if (strstr(path, "sim800")) > >+ data->modem_type =3D SIM800; > >+ else if (strstr(path, "sim900")) > >+ data->modem_type =3D SIM900; > >+ else > >+ data->modem_type =3D SIM_UNKNOWN; > >+ > = > You can't really rely on the modem path for this. Either query the model > directly or set the type based on the driver selected. > = > And no empty lines at the end of the function please I'll go with the CGMM way. > = > >+} > >+ > >+static void mux_ready_notify(GAtResult *result, gpointer user_data) > >+{ > >+ struct ofono_modem *modem =3D user_data; > >+ struct sim900_data *data =3D ofono_modem_get_data(modem); > >+ struct ofono_gprs *gprs =3D NULL; > >+ struct ofono_gprs_context *gc; > >+ static int notified =3D 0; > >+ > >+ if (!notified) { > >+ ofono_sms_create(modem, OFONO_VENDOR_SIMCOM, "atmodem", > >+ 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); > >+ } > >+ > = > The indentation and stule is all wrong here. > = > >+} > >+ > > static int sim900_probe(struct ofono_modem *modem) > > { > > struct sim900_data *data; > >@@ -79,6 +126,7 @@ static int sim900_probe(struct ofono_modem *modem) > > ofono_modem_set_data(modem, data); > >+ set_modem_type(modem); > > return 0; > > } > >@@ -111,6 +159,7 @@ static GAtChat *open_device(struct ofono_modem *mode= m, > > GHashTable *options; > > device =3D ofono_modem_get_string(modem, key); > >+ > = > This change is spurious and should not be part of this commit > = > > if (device =3D=3D NULL) > > return NULL; > >@@ -232,6 +281,11 @@ static void setup_internal_mux(struct ofono_modem *= modem) > > goto error; > > } > > } > >+ if (data->modem_type =3D=3D SIM800) { > >+ for (i =3D 0; i >+ g_at_chat_register(data->dlcs[i], "SMS Ready", mux_ready_notify, FAL= SE, modem, NULL); > >+ } > >+ } > = > Why do you want this notification on all ports? Isn't one enough? These URC's are sent on all channels so I am catching them all to ensure al= l channels are now ready. > > ofono_modem_set_powered(modem, TRUE); > >@@ -353,18 +407,20 @@ static void sim900_post_sim(struct ofono_modem *mo= dem) > > DBG("%p", modem); > >- ofono_phonebook_create(modem, 0, "atmodem", data->dlcs[VOICE_DLC]); > >- ofono_sms_create(modem, OFONO_VENDOR_SIMCOM, "atmodem", > >+ if (data->modem_type !=3D SIM800) { > >+ ofono_phonebook_create(modem, 0, "atmodem", data->dlcs[VOICE_DLC]); > >+ ofono_sms_create(modem, OFONO_VENDOR_SIMCOM, "atmodem", > > data->dlcs[SMS_DLC]); > >- gprs =3D ofono_gprs_create(modem, 0, "atmodem", data->dlcs[GPRS_DLC]); > >- if (gprs =3D=3D NULL) > >- return; > >+ 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_SIM900, > >+ gc =3D ofono_gprs_context_create(modem, OFONO_VENDOR_SIMCOM_SIM900, > > "atmodem", data->dlcs[GPRS_DLC]); > >- if (gc) > >- ofono_gprs_add_context(gprs, gc); > >+ if (gc) > >+ ofono_gprs_add_context(gprs, gc); > >+ } > = > Som SIM800 doesn't get any of these atoms? SIM800 get these atoms by antoher way : the function setup_internal_mux reg= isters the callback for SMS_READY URC. This is the callback that will call = these atoms. The Sim800 must respect this order when using these atoms else all the inte= rfaces won't be up even if the AT commands returned OK. > = > > } > > static void sim900_post_online(struct ofono_modem *modem) > >@@ -391,9 +447,24 @@ static struct ofono_modem_driver sim900_driver =3D { > > .post_online =3D sim900_post_online, > > }; > >+ > >+static struct ofono_modem_driver sim800_driver =3D { > >+ .name =3D "sim800", > >+ .probe =3D sim900_probe, > >+ .remove =3D sim900_remove, > >+ .enable =3D sim900_enable, > >+ .disable =3D sim900_disable, > >+ .pre_sim =3D sim900_pre_sim, > >+ .post_sim =3D sim900_post_sim, > >+ .post_online =3D sim900_post_online, > >+}; > >+ > > static int sim900_init(void) > > { > >- return ofono_modem_driver_register(&sim900_driver); > >+ int ret =3D 0; > >+ ret =3D ofono_modem_driver_register(&sim800_driver); > >+ ret +=3D ofono_modem_driver_register(&sim900_driver); > >+ return ret; > = > This doesn't really work this way. The init function should either regis= ter > all drivers successfully or fail. > = > > } > > static void sim900_exit(void) > > > = > Regards, > -Denis I'll add those remarks on the new patchset ( and i hope this one will be th= e one)! Regards Clem --===============4204857012257120044==--