Re: [RFC 1/3] STE-plugin: Add vendor STE
Hi Sjur,
This patch add STE as vendor, and a few adjustments needed in atmodem
for ST-Ericsson modem.
---
drivers/atmodem/gprs.c | 15 +--
drivers/atmodem/vendor.h |5 +
plugins/modemconf.c |5 +
3 files changed, 23 insertions(+), 2 deletions(-)
diff --git a/drivers/atmodem/gprs.c b/drivers/atmodem/gprs.c
index 76085d9..305f22f 100644
--- a/drivers/atmodem/gprs.c
+++ b/drivers/atmodem/gprs.c
@@ -17,6 +17,10 @@
* along with this program; if not, write to the Free Software
* Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301
USA
*
+ * Copyright (C) 2010 ST-Ericsson AB.
+ * Author: Marit Henriksen, marit.xx.henrik...@stericsson.com.
+ * STE specific implementation.
+ *
*/
please don't do that. Add the proper copyright to the header if the
changes you are making are major. We track authorship via the GIT
itself.
#ifdef HAVE_CONFIG_H
@@ -38,6 +42,7 @@
#include gatresult.h
#include atmodem.h
+#include vendor.h
static const char *cgreg_prefix[] = { +CGREG:, NULL };
static const char *cgdcont_prefix[] = { +CGDCONT:, NULL };
@@ -45,6 +50,7 @@ static const char *none_prefix[] = { NULL };
struct gprs_data {
GAtChat *chat;
+ unsigned int vendor;
};
static void at_cgatt_cb(gboolean ok, GAtResult *result, gpointer user_data)
@@ -216,7 +222,12 @@ static void at_cgreg_test_cb(gboolean ok, GAtResult
*result,
g_at_chat_send(gd-chat, cmd, none_prefix, NULL, NULL, NULL);
g_at_chat_send(gd-chat, AT+CGAUTO=0, none_prefix, NULL, NULL, NULL);
- g_at_chat_send(gd-chat, AT+CGEREP=2,1, none_prefix,
+
+ if (gd-vendor == OFONO_VENDOR_STE)
+ g_at_chat_send(gd-chat, AT+CGEREP=1,0, none_prefix,
+ gprs_initialized, gprs, NULL);
+ else
+ g_at_chat_send(gd-chat, AT+CGEREP=2,1, none_prefix,
gprs_initialized, gprs, NULL);
return;
Can you add some extra comment here explaining why this is needed.
Otherwise it looks like some black magic.
@@ -289,7 +300,7 @@ static int at_gprs_probe(struct ofono_gprs *gprs,
gd = g_new0(struct gprs_data, 1);
gd-chat = chat;
-
+ gd-vendor = vendor;
ofono_gprs_set_data(gprs, gd);
g_at_chat_send(chat, AT+CGDCONT=?, cgdcont_prefix,
diff --git a/drivers/atmodem/vendor.h b/drivers/atmodem/vendor.h
index 8d9ed47..d7b5210 100644
--- a/drivers/atmodem/vendor.h
+++ b/drivers/atmodem/vendor.h
@@ -17,11 +17,16 @@
* along with this program; if not, write to the Free Software
* Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301
USA
*
+ * Copyright (C) 2010 ST-Ericsson AB.
+ * Author: Marit Henriksen, marit.xx.henrik...@stericsson.com.
+ * STE specific implementation.
+ *
*/
To be honest, just adding a new entry in enum is not really a reason to
add a copyright statement here.
enum ofono_vendor {
OFONO_VENDOR_GENERIC = 0,
OFONO_VENDOR_CALYPSO,
+ OFONO_VENDOR_STE,
OFONO_VENDOR_QUALCOMM_MSM,
OFONO_VENDOR_OPTION_HSO,
};
diff --git a/plugins/modemconf.c b/plugins/modemconf.c
index c8c261f..41c7428 100644
--- a/plugins/modemconf.c
+++ b/plugins/modemconf.c
@@ -17,6 +17,10 @@
* along with this program; if not, write to the Free Software
* Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301
USA
*
+ * Copyright (C) 2010 ST-Ericsson AB.
+ * Author: Marit Henriksen, marit.xx.henrik...@stericsson.com.
+ * STE specific implementation.
+ *
*/
#ifdef HAVE_CONFIG_H
@@ -128,6 +132,7 @@ static struct ofono_modem *create_modem(GKeyFile
*keyfile, const char *group)
set_address(modem, keyfile, group);
if (!g_strcmp0(driver, atgen) || !g_strcmp0(driver, g1) ||
+ !g_strcmp0(driver, ste) ||
!g_strcmp0(driver, calypso) ||
!g_strcmp0(driver, hfp) ||
!g_strcmp0(driver, palmpre))
I am failing to see the reason for this. I makes no sense to me. The
CAIF kernel code (as far as I understand it) exports AT command sockets
and they are configured differently. So this seems like a change without
any functionality.
Regards
Marcel
___
ofono mailing list
ofono@ofono.org
http://lists.ofono.org/listinfo/ofono
Re: [RFC 3/3] STE-plugin: Adding STE plugin
Hi Sjur,
Added implementation for STE modem; STE modem driver, and STE specific
drivers for gprs, network registration and voice call.
This patch uses CAIF sockets. CAIF patch for net-next-2.6 will be
contributed on net...@vger.kernel.org soon.
can you please split these into smaller patches so they are easier to
review. I am thinking of something the like this; one per network
registration, one per voice call, one per GPRS.
You are making some core changes and we need to have a close look at
them and what the potential impact would be.
This is not a complete review, but I making some comments on obvious
things.
index 000..1d5d8db
--- /dev/null
+++ b/drivers/stemodem/gprs-context.c
@@ -0,0 +1,612 @@
+/*
+ *
+ * oFono - Open Source Telephony
+ *
+ * Copyright (C) 2008-2009 Intel Corporation. All rights reserved.
+ * Copyright (C) 2010 ST-Ericsson AB.
+ * Author: Marit Henriksen, marit.xx.henrik...@stericsson.com.
As mentioned before, we track authorship via GIT. So ensure that the GIT
commits are properly done.
+#include linux/types.h
+#include net/if.h
+#include sys/ioctl.h
+#include arpa/inet.h
+#include linux/caif/caif_socket.h
+#include linux/caif/if_caif.h
This is dangerous until the CAIF subsystem is actually merged and
present everywhere. Please consider adding an option to enable stemodem
driver (like we do for atmodem and isimodem).
It might make sense to have a local copy of the required structure and
constants to allow an easier complication. Of course this depends on
having CAIF at least in net-next-2.6 tree.
+/* TODO: should parse_xml function to be moved to e.g. atutil? */
First question. Why not use the XML parse that comes with GLib.
+static char *parse_xml(char * xml, char* tag)
+{
Please use consistent coding style. It is char *xml. And it is always
like this. No extra space after * or char*.
+char *begin;
+char *end;
+int len;
+char *res = NULL;
+char *start = (char *)g_malloc(strlen(tag)+3);
+char *stop = (char *)g_malloc(strlen(tag)+4);
No casting of malloc function. It is not needed. And extra spaces
between operation. Meaning malloc(strlen(tag) + 3).
This applies to all code.
+ if (create) {
+ if (ioctl(s, SIOCCAIFNETNEW, ifr) 0) {
+ ofono_debug(Failed to create IP interface for CAIF);
+ goto error;
+ }
+
+ s = socket(PF_INET, SOCK_DGRAM, 0);
+ if (s 0) {
+ ofono_debug(Failed to create socket.);
+ goto error;
+ }
+
+ /* Set IP address */
+ memset(sin, 0, sizeof(struct sockaddr));
+ sin.sin_family = AF_INET;
+
+ if (inet_pton(AF_INET, ip, sin.sin_addr) = 0) {
+ ofono_debug(inet_pton failed, will not be
+ able to set the IP address);
+ goto error;
+ }
+ memcpy(ifr.ifr_addr, sin, sizeof(struct sockaddr));
+
+ if (ioctl(s, SIOCSIFADDR, ifr) 0) {
+ ofono_debug(Failed to set IP address for
+ interface: %s, ifr.ifr_name);
+ goto error;
+ }
oFono is never setting the IP address, netmask or any other
configuration option. The only thing that oFono does is bringing the
interface up. Systems like ConnMan do the IP configuration.
Please see the comments in the documentation on how we expose IP
interfaces. Check with ConnMan and how we configure them.
+ if (ioctl(s, SIOCSIFMTU, ifr) 0)
+ ofono_debug(Failed to set MTU for interface: %s,
+ ifr.ifr_name);
+ } else {
+ if (ioctl(s, SIOCGIFINDEX, ifr) == 0) {
+ if (ioctl(s, SIOCCAIFNETREMOVE, ifr) 0) {
+ ofono_debug(Failed to remove IP interface
+ for CAIF);
+ goto error;
+ }
+ } else {
+ ofono_debug(Did not find interface (%s) to remove,
+ interface);
+ goto error;
+ }
+ }
Having create and remove in the same function seems hard to read. Why
not have one for creating the interface and one for removing it. From
what I see so far, it is not much more code. And makes it a lot easier
to read and understand for other people.
+ g_at_result_iter_init(iter, result);
+ for (i = 0; i g_at_result_num_response_lines(result); i++) {
+ g_at_result_iter_next(iter, NULL);
+ res_string = strdup(g_at_result_iter_raw_line(iter));
+
+ if (strstr(res_string, ip_address)) {
+ ip = g_strdup(parse_xml(res_string,
+ ip_address));
+ } else if
