[PATCH v2 0/9] add LTE atom

[PATCH v2 0/9] add LTE atom

[Dragos Tatulea]

[-- Attachment #1: Type: text/plain, Size: 1819 bytes --]

This patchset adds a simple LTE atom. It can only set the default
APN. But it's meant to be easily extensible in the future.

The atom exposes a per modem D-Bus interface. It also uses the
oFono persistent configuration store.

The patchset contains:
* lte atom implementation
* U-Blox Toby L2 lte driver implementation
* script for setting the lte properties

The last patches contain the implementation of the TobyL2 driver.
They might sit better in a different patchset maybe.

What is not here:
* Atom is always shown, regardles of technology. This should be
  filtered out.
* No provisioning support yet.

v2 contains fixes from previous v1 review.

Dragos Tatulea (9):
  include: add LTE dbus service define
  include: add header file for lte atom
  lte: add implementation for LTE atom
  build: add lte atom support
  test: add script for setting lte atom properties
  doc: add lte atom documentation
  ubloxmodem: add lte atom driver
  build: add support for ublox lte atom driver
  plugins: ublox: enable lte driver for tobyl2

 Makefile.am                     |   8 +-
 doc/lte-api.txt                 |  35 ++++
 drivers/ubloxmodem/lte.c        | 154 +++++++++++++++++
 drivers/ubloxmodem/ubloxmodem.c |   3 +
 drivers/ubloxmodem/ubloxmodem.h |   5 +
 include/dbus.h                  |   1 +
 include/lte.h                   |  68 ++++++++
 plugins/ublox.c                 |   3 +
 src/lte.c                       | 369 ++++++++++++++++++++++++++++++++++++++++
 test/set-lte-property           |  26 +++
 10 files changed, 669 insertions(+), 3 deletions(-)
 create mode 100644 doc/lte-api.txt
 create mode 100644 drivers/ubloxmodem/lte.c
 create mode 100644 include/lte.h
 create mode 100644 src/lte.c
 create mode 100755 test/set-lte-property

-- 
2.7.4

[PATCH 1/9] include: add LTE dbus service define

[Dragos Tatulea]

[-- Attachment #1: Type: text/plain, Size: 586 bytes --]

---
 include/dbus.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/dbus.h b/include/dbus.h
index 99d45d6..cf68ae5 100644
--- a/include/dbus.h
+++ b/include/dbus.h
@@ -61,6 +61,7 @@ extern "C" {
 #define OFONO_HANDSFREE_INTERFACE OFONO_SERVICE ".Handsfree"
 #define OFONO_SIRI_INTERFACE OFONO_SERVICE ".Siri"
 #define OFONO_NETMON_INTERFACE OFONO_SERVICE ".NetworkMonitor"
+#define OFONO_LTE_INTERFACE OFONO_SERVICE ".LongTermEvolution"
 
 /* CDMA Interfaces */
 #define OFONO_CDMA_VOICECALL_MANAGER_INTERFACE "org.ofono.cdma.VoiceCallManager"
-- 
2.7.4

Re: [PATCH 1/9] include: add LTE dbus service define

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 216 bytes --]

Hi Dragos,

On 11/10/2016 10:55 AM, Dragos Tatulea wrote:
> ---
>   include/dbus.h | 1 +
>   1 file changed, 1 insertion(+)
>

Applied, thanks.  I did push some minor fixes afterwards.

Regards,
-Denis

[PATCH 2/9] include: add header file for lte atom

[Dragos Tatulea]

[-- Attachment #1: Type: text/plain, Size: 2284 bytes --]

---
 include/lte.h | 68 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 68 insertions(+)
 create mode 100644 include/lte.h

diff --git a/include/lte.h b/include/lte.h
new file mode 100644
index 0000000..f437798
--- /dev/null
+++ b/include/lte.h
@@ -0,0 +1,68 @@
+/*
+ *
+ *  oFono - Open Source Telephony
+ *
+ *  Copyright (C) 2016  Endocode AG. All rights reserved.
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License version 2 as
+ *  published by the Free Software Foundation.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; if not, write to the Free Software
+ *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+ *
+ */
+
+#ifndef __OFONO_LTE_H
+#define __OFONO_LTE_H
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+#include <ofono/types.h>
+
+struct ofono_lte;
+
+struct ofono_lte_default_attach_info {
+	char apn[OFONO_GPRS_MAX_APN_LENGTH + 1];
+};
+
+typedef void (*ofono_lte_cb_t)(const struct ofono_error *error, void *data);
+
+struct ofono_lte_driver {
+	const char *name;
+	int (*probe)(struct ofono_lte *lte, void *data);
+	void (*remove)(struct ofono_lte *lte);
+	int (*set_default_attach_info)(const struct ofono_lte *lte,
+			struct ofono_lte_default_attach_info *info,
+			ofono_lte_cb_t cb, void *data);
+
+};
+
+int ofono_lte_driver_register(const struct ofono_lte_driver *d);
+
+void ofono_lte_driver_unregister(const struct ofono_lte_driver *d);
+
+struct ofono_lte *ofono_lte_create(struct ofono_modem *modem,
+					const char *driver, void *data);
+
+void ofono_lte_register(struct ofono_lte *lte);
+
+void ofono_lte_remove(struct ofono_lte *lte);
+
+void ofono_lte_set_data(struct ofono_lte *lte, void *data);
+
+void *ofono_lte_get_data(const struct ofono_lte *lte);
+
+#ifdef __cplusplus
+}
+#endif
+
+#endif
-- 
2.7.4

Re: [PATCH 2/9] include: add header file for lte atom

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 273 bytes --]

Hi Dragos,

On 11/10/2016 10:55 AM, Dragos Tatulea wrote:
> ---
>   include/lte.h | 68 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 68 insertions(+)
>   create mode 100644 include/lte.h
>

Applied, thanks.

Regards,
-Denis

[PATCH 3/9] lte: add implementation for LTE atom

[Dragos Tatulea]

[-- Attachment #1: Type: text/plain, Size: 10144 bytes --]

This implementation can only get/set the default APN setting. But
anything expected for this atom is there:
* D-Bus interface
* sync-ing settings to/from file
* interaction with driver
---
 src/lte.c | 369 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 369 insertions(+)
 create mode 100644 src/lte.c

diff --git a/src/lte.c b/src/lte.c
new file mode 100644
index 0000000..ba22dc2
--- /dev/null
+++ b/src/lte.c
@@ -0,0 +1,369 @@
+/*
+ *
+ *  oFono - Open Source Telephony
+ *
+ *  Copyright (C) 2016  Endocode AG. All rights reserved.
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License version 2 as
+ *  published by the Free Software Foundation.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; if not, write to the Free Software
+ *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+ *
+ */
+
+#ifdef HAVE_CONFIG_H
+#include <config.h>
+#endif
+
+#include <string.h>
+#include <errno.h>
+
+#include <glib.h>
+#include <gdbus.h>
+
+#include "ofono.h"
+
+#include "common.h"
+#include "log.h"
+#include "gprs-context.h"
+#include "storage.h"
+
+#include "lte.h"
+
+#define SETTINGS_STORE "lte"
+#define SETTINGS_GROUP "Settings"
+#define DEFAULT_APN_KEY "DefaultAccessPointName"
+
+struct ofono_lte {
+	const struct ofono_lte_driver *driver;
+	void *driver_data;
+	struct ofono_atom *atom;
+	char *imsi;
+	GKeyFile *settings;
+	DBusMessage *pending;
+	char old_apn[OFONO_GPRS_MAX_APN_LENGTH+1];
+	struct ofono_lte_default_attach_info info;
+};
+
+static GSList *g_drivers = NULL;
+
+static void lte_load_settings(struct ofono_lte *lte)
+{
+	char *apn;
+
+	if (lte->imsi == NULL)
+		return;
+
+	lte->settings = storage_open(lte->imsi, SETTINGS_STORE);
+
+	if (lte->settings == NULL) {
+		ofono_error("LTE: Can't open settings file, "
+				"changes won't be persistent");
+		return;
+	}
+
+	apn = g_key_file_get_string(lte->settings, SETTINGS_GROUP ,
+					DEFAULT_APN_KEY, NULL);
+	if (apn)
+		strcpy(lte->info.apn, apn);
+}
+
+static DBusMessage *lte_get_properties(DBusConnection *conn,
+					DBusMessage *msg, void *data)
+{
+	struct ofono_lte *lte = data;
+	const char *apn = lte->info.apn;
+	DBusMessage *reply;
+	DBusMessageIter iter;
+	DBusMessageIter dict;
+
+	reply = dbus_message_new_method_return(msg);
+	if (reply == NULL)
+		return NULL;
+
+	dbus_message_iter_init_append(reply, &iter);
+
+	dbus_message_iter_open_container(&iter, DBUS_TYPE_ARRAY,
+					OFONO_PROPERTIES_ARRAY_SIGNATURE,
+					&dict);
+	ofono_dbus_dict_append(&dict, DEFAULT_APN_KEY, DBUS_TYPE_STRING, &apn);
+	dbus_message_iter_close_container(&iter, &dict);
+
+	return reply;
+}
+
+static void lte_set_default_attach_info_cb(const struct ofono_error *error,
+						void *data)
+{
+	struct ofono_lte *lte = data;
+	const char *path = __ofono_atom_get_path(lte->atom);
+	DBusConnection *conn = ofono_dbus_get_connection();
+	DBusMessage *reply;
+	const char *apn = lte->info.apn;
+
+	DBG("%s error %d", path, error->type);
+
+	if (error->type != OFONO_ERROR_TYPE_NO_ERROR) {
+		g_strlcpy(lte->info.apn, lte->old_apn,
+				OFONO_GPRS_MAX_APN_LENGTH+1);
+
+		__ofono_dbus_pending_reply(&lte->pending,
+				__ofono_error_failed(lte->pending));
+		return;
+	}
+
+	if (lte->pending == NULL)
+		/* This can happen on first register: it's not a reply to
+		 * anything.
+		 */
+		return;
+
+	if (lte->settings) {
+		if (strlen(lte->info.apn) == 0)
+			/* Clear entry on empty APN. */
+			g_key_file_remove_key(lte->settings, SETTINGS_GROUP,
+						DEFAULT_APN_KEY, NULL);
+		else
+			g_key_file_set_string(lte->settings, SETTINGS_GROUP,
+						DEFAULT_APN_KEY, lte->info.apn);
+
+		storage_sync(lte->imsi, SETTINGS_STORE, lte->settings);
+	}
+
+	reply = dbus_message_new_method_return(lte->pending);
+	__ofono_dbus_pending_reply(&lte->pending, reply);
+	lte->pending = NULL;
+
+	ofono_dbus_signal_property_changed(conn, path,
+					OFONO_CONNECTION_CONTEXT_INTERFACE,
+					DEFAULT_APN_KEY,
+					DBUS_TYPE_STRING, &apn);
+}
+
+static DBusMessage *lte_set_default_apn(struct ofono_lte *lte,
+				DBusConnection *conn, DBusMessage *msg,
+				const char *apn)
+{
+	if (strlen(apn) > OFONO_GPRS_MAX_APN_LENGTH)
+		return __ofono_error_invalid_format(msg);
+
+	if (g_str_equal(apn, lte->info.apn))
+		return dbus_message_new_method_return(msg);
+
+	/* We do care about empty value: it can be used for reset. */
+	if (is_valid_apn(apn) == FALSE && apn[0] != '\0')
+		return __ofono_error_invalid_format(msg);
+
+	if (lte->pending) {
+		/* Can happen on fast succession of calls. */
+		dbus_message_unref(lte->pending);
+		lte->pending = NULL;
+	}
+
+	lte->pending = dbus_message_ref(msg);
+
+	g_strlcpy(lte->old_apn, lte->info.apn, OFONO_GPRS_MAX_APN_LENGTH+1);
+	g_strlcpy(lte->info.apn, apn, OFONO_GPRS_MAX_APN_LENGTH+1);
+
+	lte->driver->set_default_attach_info(lte, &lte->info,
+				lte_set_default_attach_info_cb, lte);
+
+	return NULL;
+}
+
+static DBusMessage *lte_set_property(DBusConnection *conn,
+					DBusMessage *msg, void *data)
+{
+	struct ofono_lte *lte = data;
+	DBusMessageIter iter;
+	DBusMessageIter var;
+	const char *property;
+	const char *str;
+
+	if (!dbus_message_iter_init(msg, &iter))
+		return __ofono_error_invalid_args(msg);
+
+	if (dbus_message_iter_get_arg_type(&iter) != DBUS_TYPE_STRING)
+		return __ofono_error_invalid_args(msg);
+
+	dbus_message_iter_get_basic(&iter, &property);
+	dbus_message_iter_next(&iter);
+
+	if (dbus_message_iter_get_arg_type(&iter) != DBUS_TYPE_VARIANT)
+		return __ofono_error_invalid_args(msg);
+
+	dbus_message_iter_recurse(&iter, &var);
+
+
+	if (!strcmp(property, DEFAULT_APN_KEY)) {
+		if (dbus_message_iter_get_arg_type(&var) != DBUS_TYPE_STRING)
+			return __ofono_error_invalid_args(msg);
+
+		dbus_message_iter_get_basic(&var, &str);
+
+		return lte_set_default_apn(lte, conn, msg, str);
+	}
+
+	return __ofono_error_invalid_args(msg);
+}
+
+static const GDBusMethodTable lte_methods[] = {
+	{ GDBUS_METHOD("GetProperties",
+			NULL, GDBUS_ARGS({ "properties", "a{sv}" }),
+			lte_get_properties) },
+	{ GDBUS_ASYNC_METHOD("SetProperty",
+			GDBUS_ARGS({ "property", "s" }, { "value", "v" }),
+			NULL, lte_set_property) },
+	{ }
+};
+
+static const GDBusSignalTable lte_signals[] = {
+	{ GDBUS_SIGNAL("PropertyChanged",
+			GDBUS_ARGS({ "name", "s" }, { "value", "v" })) },
+	{ }
+};
+
+static void lte_atom_remove(struct ofono_atom *atom)
+{
+	struct ofono_lte *lte = __ofono_atom_get_data(atom);
+
+	DBG("atom: %p", atom);
+
+	if (lte == NULL)
+		return;
+
+	if (lte->settings) {
+		storage_close(lte->imsi, SETTINGS_STORE, lte->settings, TRUE);
+
+		g_free(lte->imsi);
+		lte->imsi = NULL;
+		lte->settings = NULL;
+	}
+
+	if (lte->driver && lte->driver->remove)
+		lte->driver->remove(lte);
+
+	g_free(lte);
+}
+
+
+struct ofono_lte *ofono_lte_create(struct ofono_modem *modem,
+					const char *driver, void *data)
+{
+	struct ofono_lte *lte;
+	GSList *l;
+
+	if (driver == NULL)
+		return NULL;
+
+	lte = g_try_new0(struct ofono_lte, 1);
+
+	if (lte == NULL)
+		return NULL;
+
+	lte->atom = __ofono_modem_add_atom(modem, OFONO_ATOM_TYPE_LTE,
+						lte_atom_remove, lte);
+
+	for (l = g_drivers; l; l = l->next) {
+		const struct ofono_lte_driver *drv = l->data;
+
+		if (g_strcmp0(drv->name, driver))
+			continue;
+
+		if (drv->probe(lte, data) < 0)
+			continue;
+
+		lte->driver = drv;
+		break;
+	}
+
+	DBG("LTE atom created");
+
+	return lte;
+}
+
+int ofono_lte_driver_register(const struct ofono_lte_driver *d)
+{
+	DBG("driver: %p, name: %s", d, d->name);
+
+	if (d->probe == NULL)
+		return -EINVAL;
+
+	g_drivers = g_slist_prepend(g_drivers, (void *) d);
+
+	return 0;
+}
+
+void ofono_lte_driver_unregister(const struct ofono_lte_driver *d)
+{
+	DBG("driver: %p, name: %s", d, d->name);
+
+	g_drivers = g_slist_remove(g_drivers, (void *) d);
+}
+
+static void lte_atom_unregister(struct ofono_atom *atom)
+{
+	DBusConnection *conn = ofono_dbus_get_connection();
+	struct ofono_modem *modem = __ofono_atom_get_modem(atom);
+	const char *path = __ofono_atom_get_path(atom);
+
+	ofono_modem_remove_interface(modem, OFONO_LTE_INTERFACE);
+	g_dbus_unregister_interface(conn, path, OFONO_LTE_INTERFACE);
+}
+
+void ofono_lte_register(struct ofono_lte *lte)
+{
+	DBusConnection *conn = ofono_dbus_get_connection();
+	struct ofono_modem *modem = __ofono_atom_get_modem(lte->atom);
+	const char *path = __ofono_atom_get_path(lte->atom);
+	struct ofono_sim *sim = __ofono_atom_find(OFONO_ATOM_TYPE_SIM, modem);
+	const char *imsi = ofono_sim_get_imsi(sim);
+
+	if (imsi == NULL) {
+		ofono_error("No sim atom required for registering LTE atom.");	
+		return;
+	}
+
+	lte->imsi = g_strdup(imsi);
+
+	lte_load_settings(lte);
+
+	if (!g_dbus_register_interface(conn, path,
+				OFONO_LTE_INTERFACE,
+				lte_methods, lte_signals, NULL,
+				lte, NULL)) {
+		ofono_error("Could not create %s interface",
+				OFONO_LTE_INTERFACE);
+		return;
+	}
+
+	ofono_modem_add_interface(modem, OFONO_LTE_INTERFACE);
+
+	__ofono_atom_register(lte->atom, lte_atom_unregister);
+
+	lte->driver->set_default_attach_info(lte, &lte->info,
+					lte_set_default_attach_info_cb, lte);
+}
+
+void ofono_lte_remove(struct ofono_lte *lte)
+{
+	__ofono_atom_free(lte->atom);
+}
+
+void ofono_lte_set_data(struct ofono_lte *lte, void *data)
+{
+	lte->driver_data = data;
+}
+
+void *ofono_lte_get_data(const struct ofono_lte *lte)
+{
+	return lte->driver_data;
+}
-- 
2.7.4

Re: [PATCH 3/9] lte: add implementation for LTE atom

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 12563 bytes --]

Hi Dragos,

On 11/10/2016 10:55 AM, Dragos Tatulea wrote:
> This implementation can only get/set the default APN setting. But
> anything expected for this atom is there:
> * D-Bus interface
> * sync-ing settings to/from file
> * interaction with driver
> ---
>   src/lte.c | 369 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 369 insertions(+)
>   create mode 100644 src/lte.c
>
> diff --git a/src/lte.c b/src/lte.c
> new file mode 100644
> index 0000000..ba22dc2
> --- /dev/null
> +++ b/src/lte.c
> @@ -0,0 +1,369 @@
> +/*
> + *
> + *  oFono - Open Source Telephony
> + *
> + *  Copyright (C) 2016  Endocode AG. All rights reserved.
> + *
> + *  This program is free software; you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License version 2 as
> + *  published by the Free Software Foundation.
> + *
> + *  This program is distributed in the hope that it will be useful,
> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + *  GNU General Public License for more details.
> + *
> + *  You should have received a copy of the GNU General Public License
> + *  along with this program; if not, write to the Free Software
> + *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
> + *
> + */
> +
> +#ifdef HAVE_CONFIG_H
> +#include <config.h>
> +#endif
> +
> +#include <string.h>
> +#include <errno.h>
> +
> +#include <glib.h>
> +#include <gdbus.h>
> +
> +#include "ofono.h"
> +
> +#include "common.h"


> +#include "log.h"
> +#include "gprs-context.h"

The above two are already included as part of ofono.h.  No need for it here.

> +#include "storage.h"
> +
> +#include "lte.h"

This should be put into ofono.h.  Rule of thumb is that all "public 
plugin" includes (e.g. in include/ofono/*) should be in ofono.h.

The only includes you should be utilizing are the various utility 
headers in src/

> +
> +#define SETTINGS_STORE "lte"
> +#define SETTINGS_GROUP "Settings"
> +#define DEFAULT_APN_KEY "DefaultAccessPointName"
> +
> +struct ofono_lte {
> +	const struct ofono_lte_driver *driver;
> +	void *driver_data;
> +	struct ofono_atom *atom;
> +	char *imsi;
> +	GKeyFile *settings;
> +	DBusMessage *pending;
> +	char old_apn[OFONO_GPRS_MAX_APN_LENGTH+1];

doc/coding-style.txt item M3.  Generally we do it one of two ways:

1. by using pending_foo variable.
2. By re-parsing the pending message

I'd just use approach #2

> +	struct ofono_lte_default_attach_info info;
> +};
> +
> +static GSList *g_drivers = NULL;
> +
> +static void lte_load_settings(struct ofono_lte *lte)
> +{
> +	char *apn;
> +
> +	if (lte->imsi == NULL)
> +		return;
> +
> +	lte->settings = storage_open(lte->imsi, SETTINGS_STORE);
> +
> +	if (lte->settings == NULL) {
> +		ofono_error("LTE: Can't open settings file, "
> +				"changes won't be persistent");
> +		return;
> +	}
> +
> +	apn = g_key_file_get_string(lte->settings, SETTINGS_GROUP ,
> +					DEFAULT_APN_KEY, NULL);
> +	if (apn)
> +		strcpy(lte->info.apn, apn);
> +}
> +
> +static DBusMessage *lte_get_properties(DBusConnection *conn,
> +					DBusMessage *msg, void *data)
> +{
> +	struct ofono_lte *lte = data;
> +	const char *apn = lte->info.apn;
> +	DBusMessage *reply;
> +	DBusMessageIter iter;
> +	DBusMessageIter dict;
> +
> +	reply = dbus_message_new_method_return(msg);
> +	if (reply == NULL)
> +		return NULL;
> +
> +	dbus_message_iter_init_append(reply, &iter);
> +
> +	dbus_message_iter_open_container(&iter, DBUS_TYPE_ARRAY,
> +					OFONO_PROPERTIES_ARRAY_SIGNATURE,
> +					&dict);
> +	ofono_dbus_dict_append(&dict, DEFAULT_APN_KEY, DBUS_TYPE_STRING, &apn);
> +	dbus_message_iter_close_container(&iter, &dict);
> +
> +	return reply;
> +}
> +
> +static void lte_set_default_attach_info_cb(const struct ofono_error *error,
> +						void *data)
> +{
> +	struct ofono_lte *lte = data;
> +	const char *path = __ofono_atom_get_path(lte->atom);
> +	DBusConnection *conn = ofono_dbus_get_connection();
> +	DBusMessage *reply;
> +	const char *apn = lte->info.apn;
> +
> +	DBG("%s error %d", path, error->type);
> +
> +	if (error->type != OFONO_ERROR_TYPE_NO_ERROR) {
> +		g_strlcpy(lte->info.apn, lte->old_apn,
> +				OFONO_GPRS_MAX_APN_LENGTH+1);
> +
> +		__ofono_dbus_pending_reply(&lte->pending,
> +				__ofono_error_failed(lte->pending));
> +		return;
> +	}
> +
> +	if (lte->pending == NULL)
> +		/* This can happen on first register: it's not a reply to
> +		 * anything.
> +		 */
> +		return;

For the initial set case, lets just have a whole new callback.

So the easy way to get the new APN we just set is to parse the arguments 
from pending one more time.  Since it was already sanity checked prior, 
you can keep it real simple here.

> +
> +	if (lte->settings) {
> +		if (strlen(lte->info.apn) == 0)
> +			/* Clear entry on empty APN. */
> +			g_key_file_remove_key(lte->settings, SETTINGS_GROUP,
> +						DEFAULT_APN_KEY, NULL);
> +		else
> +			g_key_file_set_string(lte->settings, SETTINGS_GROUP,
> +						DEFAULT_APN_KEY, lte->info.apn);
> +
> +		storage_sync(lte->imsi, SETTINGS_STORE, lte->settings);
> +	}
> +
> +	reply = dbus_message_new_method_return(lte->pending);
> +	__ofono_dbus_pending_reply(&lte->pending, reply);
> +	lte->pending = NULL;
> +
> +	ofono_dbus_signal_property_changed(conn, path,
> +					OFONO_CONNECTION_CONTEXT_INTERFACE,
> +					DEFAULT_APN_KEY,
> +					DBUS_TYPE_STRING, &apn);
> +}
> +
> +static DBusMessage *lte_set_default_apn(struct ofono_lte *lte,
> +				DBusConnection *conn, DBusMessage *msg,
> +				const char *apn)
> +{
> +	if (strlen(apn) > OFONO_GPRS_MAX_APN_LENGTH)
> +		return __ofono_error_invalid_format(msg);
> +
> +	if (g_str_equal(apn, lte->info.apn))
> +		return dbus_message_new_method_return(msg);
> +
> +	/* We do care about empty value: it can be used for reset. */
> +	if (is_valid_apn(apn) == FALSE && apn[0] != '\0')
> +		return __ofono_error_invalid_format(msg);
> +
> +	if (lte->pending) {
> +		/* Can happen on fast succession of calls. */
> +		dbus_message_unref(lte->pending);
> +		lte->pending = NULL;

??  Generally we simply return a busy error.

> +	}
> +
> +	lte->pending = dbus_message_ref(msg);
> +
> +	g_strlcpy(lte->old_apn, lte->info.apn, OFONO_GPRS_MAX_APN_LENGTH+1);
> +	g_strlcpy(lte->info.apn, apn, OFONO_GPRS_MAX_APN_LENGTH+1);

doc/coding-style.txt item M3.

Also, lets not do it this way.  Since GetProperties() in between calling 
SetProperty() and it returning might potentially give erroneous results.

> +
> +	lte->driver->set_default_attach_info(lte, &lte->info,
> +				lte_set_default_attach_info_cb, lte);
> +
> +	return NULL;
> +}
> +
> +static DBusMessage *lte_set_property(DBusConnection *conn,
> +					DBusMessage *msg, void *data)
> +{
> +	struct ofono_lte *lte = data;
> +	DBusMessageIter iter;
> +	DBusMessageIter var;
> +	const char *property;
> +	const char *str;
> +
> +	if (!dbus_message_iter_init(msg, &iter))
> +		return __ofono_error_invalid_args(msg);
> +
> +	if (dbus_message_iter_get_arg_type(&iter) != DBUS_TYPE_STRING)
> +		return __ofono_error_invalid_args(msg);
> +
> +	dbus_message_iter_get_basic(&iter, &property);
> +	dbus_message_iter_next(&iter);
> +
> +	if (dbus_message_iter_get_arg_type(&iter) != DBUS_TYPE_VARIANT)
> +		return __ofono_error_invalid_args(msg);
> +
> +	dbus_message_iter_recurse(&iter, &var);
> +
> +

Double empty line

> +	if (!strcmp(property, DEFAULT_APN_KEY)) {
> +		if (dbus_message_iter_get_arg_type(&var) != DBUS_TYPE_STRING)
> +			return __ofono_error_invalid_args(msg);
> +
> +		dbus_message_iter_get_basic(&var, &str);
> +
> +		return lte_set_default_apn(lte, conn, msg, str);
> +	}
> +
> +	return __ofono_error_invalid_args(msg);
> +}
> +
> +static const GDBusMethodTable lte_methods[] = {
> +	{ GDBUS_METHOD("GetProperties",
> +			NULL, GDBUS_ARGS({ "properties", "a{sv}" }),
> +			lte_get_properties) },
> +	{ GDBUS_ASYNC_METHOD("SetProperty",
> +			GDBUS_ARGS({ "property", "s" }, { "value", "v" }),
> +			NULL, lte_set_property) },
> +	{ }
> +};
> +
> +static const GDBusSignalTable lte_signals[] = {
> +	{ GDBUS_SIGNAL("PropertyChanged",
> +			GDBUS_ARGS({ "name", "s" }, { "value", "v" })) },
> +	{ }
> +};
> +
> +static void lte_atom_remove(struct ofono_atom *atom)
> +{
> +	struct ofono_lte *lte = __ofono_atom_get_data(atom);
> +
> +	DBG("atom: %p", atom);
> +
> +	if (lte == NULL)
> +		return;
> +
> +	if (lte->settings) {
> +		storage_close(lte->imsi, SETTINGS_STORE, lte->settings, TRUE);
> +
> +		g_free(lte->imsi);
> +		lte->imsi = NULL;
> +		lte->settings = NULL;
> +	}
> +
> +	if (lte->driver && lte->driver->remove)
> +		lte->driver->remove(lte);
> +
> +	g_free(lte);
> +}
> +
> +

double empty line :)

> +struct ofono_lte *ofono_lte_create(struct ofono_modem *modem,
> +					const char *driver, void *data)
> +{
> +	struct ofono_lte *lte;
> +	GSList *l;
> +
> +	if (driver == NULL)
> +		return NULL;
> +
> +	lte = g_try_new0(struct ofono_lte, 1);
> +
> +	if (lte == NULL)
> +		return NULL;
> +
> +	lte->atom = __ofono_modem_add_atom(modem, OFONO_ATOM_TYPE_LTE,
> +						lte_atom_remove, lte);
> +
> +	for (l = g_drivers; l; l = l->next) {
> +		const struct ofono_lte_driver *drv = l->data;
> +
> +		if (g_strcmp0(drv->name, driver))
> +			continue;
> +
> +		if (drv->probe(lte, data) < 0)
> +			continue;
> +
> +		lte->driver = drv;
> +		break;
> +	}
> +
> +	DBG("LTE atom created");
> +
> +	return lte;
> +}
> +
> +int ofono_lte_driver_register(const struct ofono_lte_driver *d)
> +{
> +	DBG("driver: %p, name: %s", d, d->name);
> +
> +	if (d->probe == NULL)
> +		return -EINVAL;
> +
> +	g_drivers = g_slist_prepend(g_drivers, (void *) d);
> +
> +	return 0;
> +}
> +
> +void ofono_lte_driver_unregister(const struct ofono_lte_driver *d)
> +{
> +	DBG("driver: %p, name: %s", d, d->name);
> +
> +	g_drivers = g_slist_remove(g_drivers, (void *) d);
> +}
> +
> +static void lte_atom_unregister(struct ofono_atom *atom)
> +{
> +	DBusConnection *conn = ofono_dbus_get_connection();
> +	struct ofono_modem *modem = __ofono_atom_get_modem(atom);
> +	const char *path = __ofono_atom_get_path(atom);
> +
> +	ofono_modem_remove_interface(modem, OFONO_LTE_INTERFACE);
> +	g_dbus_unregister_interface(conn, path, OFONO_LTE_INTERFACE);
> +}
> +
> +void ofono_lte_register(struct ofono_lte *lte)
> +{
> +	DBusConnection *conn = ofono_dbus_get_connection();
> +	struct ofono_modem *modem = __ofono_atom_get_modem(lte->atom);
> +	const char *path = __ofono_atom_get_path(lte->atom);
> +	struct ofono_sim *sim = __ofono_atom_find(OFONO_ATOM_TYPE_SIM, modem);
> +	const char *imsi = ofono_sim_get_imsi(sim);
> +
> +	if (imsi == NULL) {
> +		ofono_error("No sim atom required for registering LTE atom.");	
> +		return;
> +	}
> +
> +	lte->imsi = g_strdup(imsi);
> +
> +	lte_load_settings(lte);
> +
> +	if (!g_dbus_register_interface(conn, path,
> +				OFONO_LTE_INTERFACE,
> +				lte_methods, lte_signals, NULL,
> +				lte, NULL)) {
> +		ofono_error("Could not create %s interface",
> +				OFONO_LTE_INTERFACE);
> +		return;
> +	}
> +
> +	ofono_modem_add_interface(modem, OFONO_LTE_INTERFACE);
> +
> +	__ofono_atom_register(lte->atom, lte_atom_unregister);
> +
> +	lte->driver->set_default_attach_info(lte, &lte->info,
> +					lte_set_default_attach_info_cb, lte);

Generally we should register the interface after we have loaded the 
settings and bootstrapped them into the driver.  This will be especially 
true if we have to run provisioning first.  So i would perform the 
registration steps in a specific callback.  e.g. something like:

if (lte->driver->set_default_attach_info) {
	lte->driver->set_default_attach_info(lte, &lte->info, 
init_default_attach_info_cb, lte);
	return;
}

/* otherwise, register the interface, probably by calling a common helper */
ofono_lte_finish_register(...);

> +}
> +
> +void ofono_lte_remove(struct ofono_lte *lte)
> +{
> +	__ofono_atom_free(lte->atom);
> +}
> +
> +void ofono_lte_set_data(struct ofono_lte *lte, void *data)
> +{
> +	lte->driver_data = data;
> +}
> +
> +void *ofono_lte_get_data(const struct ofono_lte *lte)
> +{
> +	return lte->driver_data;
> +}
>

Regards,
-Denis

[PATCH 4/9] build: add lte atom support

[Dragos Tatulea]

[-- Attachment #1: Type: text/plain, Size: 925 bytes --]

---
 Makefile.am | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Makefile.am b/Makefile.am
index acd5083..9d22365 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -22,7 +22,7 @@ pkginclude_HEADERS = include/log.h include/plugin.h include/history.h \
 			include/private-network.h include/cdma-netreg.h \
 			include/cdma-provision.h include/handsfree.h \
 			include/handsfree-audio.h include/siri.h \
-			include/netmon.h
+			include/netmon.h include/lte.h
 
 nodist_pkginclude_HEADERS = include/version.h
 
@@ -605,7 +605,7 @@ src_ofonod_SOURCES = $(builtin_sources) $(gatchat_sources) src/ofono.ver \
 			src/cdma-provision.c src/handsfree.c \
 			src/handsfree-audio.c src/bluetooth.h \
 			src/hfp.h src/siri.c \
-			src/netmon.c
+			src/netmon.c src/lte.c
 
 src_ofonod_LDADD = gdbus/libgdbus-internal.la $(builtin_libadd) \
 			@GLIB_LIBS@ @DBUS_LIBS@ -ldl
-- 
2.7.4

[PATCH 5/9] test: add script for setting lte atom properties

[Dragos Tatulea]

[-- Attachment #1: Type: text/plain, Size: 985 bytes --]

---
 test/set-lte-property | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)
 create mode 100755 test/set-lte-property

diff --git a/test/set-lte-property b/test/set-lte-property
new file mode 100755
index 0000000..a34e60a
--- /dev/null
+++ b/test/set-lte-property
@@ -0,0 +1,26 @@
+#!/usr/bin/python3
+
+import dbus
+import sys
+
+bus = dbus.SystemBus()
+
+if len(sys.argv) == 4:
+	path = sys.argv[1]
+	name = sys.argv[2]
+	value = sys.argv[3]
+elif len(sys.argv) == 3:
+	manager = dbus.Interface(bus.get_object('org.ofono', '/'),
+					'org.ofono.Manager')
+	modems = manager.GetModems()
+	path = modems[0][0]
+	name = sys.argv[1]
+	value = sys.argv[2]
+else:
+	print("%s [PATH] name value" % (sys.argv[0]))
+	sys.exit(0)
+
+print("Setting {} as {} for modem {}..." .format(name, value, path))
+lte = dbus.Interface(bus.get_object('org.ofono', path),
+			'org.ofono.LongTermEvolution')
+lte.SetProperty(name, value)
-- 
2.7.4

Re: [PATCH 5/9] test: add script for setting lte atom properties

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 256 bytes --]

Hi Dragos,

On 11/10/2016 10:55 AM, Dragos Tatulea wrote:
> ---
>   test/set-lte-property | 26 ++++++++++++++++++++++++++
>   1 file changed, 26 insertions(+)
>   create mode 100755 test/set-lte-property
>

Applied, thanks.

Regards,
-Denis

[PATCH 6/9] doc: add lte atom documentation

[Dragos Tatulea]

[-- Attachment #1: Type: text/plain, Size: 1354 bytes --]

---
 doc/lte-api.txt | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)
 create mode 100644 doc/lte-api.txt

diff --git a/doc/lte-api.txt b/doc/lte-api.txt
new file mode 100644
index 0000000..8a2a97d
--- /dev/null
+++ b/doc/lte-api.txt
@@ -0,0 +1,35 @@
+LongTermEvolution Hierarchy
+
+Service		org.ofono
+Interface	org.ofono.LongTermEvolution
+Object path	[variable prefix]/{modem0,modem1,...}
+
+
+Methods		dict GetProperties()
+
+			Returns all LongTermEvolution configuration properties.
+
+		void SetProperty(string property, variant value)
+
+			Changes the value of the specified property. Only
+			properties that are listed as readwrite are
+			changeable. On success a PropertyChanged signal
+			will be emitted.
+
+			Possible Errors: [service].Error.InProgress
+					 [service].Error.InvalidArguments
+					 [service].Error.Failed
+
+Signals		PropertyChanged(string property, variant value)
+
+			This signal indicates a changed value of the given
+			property.
+
+Properties	string DefaultAccessPointName [readwrite]
+
+			On LongTermEvolution, contexts activate automatically.
+			This property allows selection of an APN to be used on
+			next automatic activation.
+
+			Setting this property to an empty string clears the
+			default APN from the modem.
-- 
2.7.4

Re: [PATCH 6/9] doc: add lte atom documentation

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 253 bytes --]

Hi Dragos,

On 11/10/2016 10:55 AM, Dragos Tatulea wrote:
> ---
>   doc/lte-api.txt | 35 +++++++++++++++++++++++++++++++++++
>   1 file changed, 35 insertions(+)
>   create mode 100644 doc/lte-api.txt
>

Applied, thanks.

Regards,
-Denis

[PATCH 7/9] ubloxmodem: add lte atom driver

[Dragos Tatulea]

[-- Attachment #1: Type: text/plain, Size: 5561 bytes --]

Adds U-Blox Toby L2 driver for setting the default APN via the
+UCGDFLT command. Currently only IPv4 is supported. APN is
not stored to modem's non-volatile memory. oFono will manage this
default APN via it's config storage.

When receiving an empty default APN, the value is reset.
---
 drivers/ubloxmodem/lte.c        | 154 ++++++++++++++++++++++++++++++++++++++++
 drivers/ubloxmodem/ubloxmodem.c |   3 +
 drivers/ubloxmodem/ubloxmodem.h |   5 ++
 3 files changed, 162 insertions(+)
 create mode 100644 drivers/ubloxmodem/lte.c

diff --git a/drivers/ubloxmodem/lte.c b/drivers/ubloxmodem/lte.c
new file mode 100644
index 0000000..43c02d4
--- /dev/null
+++ b/drivers/ubloxmodem/lte.c
@@ -0,0 +1,154 @@
+/*
+ *
+ *  oFono - Open Source Telephony
+ *
+ *  Copyright (C) 2016  Endocode AG. All rights reserved.
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License version 2 as
+ *  published by the Free Software Foundation.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; if not, write to the Free Software
+ *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+ *
+ */
+#ifdef HAVE_CONFIG_H
+#include <config.h>
+#endif
+
+#define _GNU_SOURCE
+#include <stdlib.h>
+#include <stdio.h>
+#include <string.h>
+#include <errno.h>
+
+#include <glib.h>
+
+#include <ofono/modem.h>
+#include <ofono/gprs-context.h>
+#include <ofono/log.h>
+#include <ofono/lte.h>
+
+#include "gatchat.h"
+#include "gatresult.h"
+
+#include "ubloxmodem.h"
+
+static const char *ucgdflt_prefix[] = { "+UCGDFLT:", NULL };
+
+struct lte_driver_data {
+	GAtChat *chat;
+};
+
+static void ucgdflt_cb(gboolean ok, GAtResult *result, gpointer user_data)
+{
+	struct cb_data *cbd = user_data;
+	ofono_lte_cb_t cb = cbd->cb;
+
+	DBG("ok %d", ok);
+
+	if (!ok) {
+		struct ofono_error error;
+
+		decode_at_error(&error, g_at_result_final_response(result));
+		cb(&error, cbd->data);
+	} else {
+		CALLBACK_WITH_SUCCESS(cb, cbd->data);
+	}
+
+	g_free(cbd);
+
+	return;
+}
+
+static int ublox_lte_set_default_attach_info(const struct ofono_lte *lte,
+				struct ofono_lte_default_attach_info *info,
+				ofono_lte_cb_t cb, void *data)
+{
+	struct lte_driver_data *ldd = ofono_lte_get_data(lte);
+	char buf[32 + OFONO_GPRS_MAX_APN_LENGTH + 1];
+	struct cb_data *cbd = cb_data_new(cb, data);
+
+	DBG("LTE config with APN: %s", info->apn);
+
+	if (strlen(info->apn) > 0)
+		snprintf(buf, sizeof(buf), "AT+UCGDFLT=0,\"IP\",\"%s\"",
+				info->apn);
+	else
+		snprintf(buf, sizeof(buf), "AT+UCGDFLT=0");
+
+	/* We can't do much in case of failure so don't check response. */
+	if (g_at_chat_send(ldd->chat, buf, ucgdflt_prefix,
+				ucgdflt_cb, cbd, NULL) == 0) {
+		g_free(cbd);
+		CALLBACK_WITH_FAILURE(cb, data);
+	}
+
+	return 0;
+}
+
+static gboolean lte_delayed_register(gpointer user_data)
+{
+	struct ofono_lte *lte = user_data;
+
+	ofono_lte_register(lte);
+
+	return FALSE;
+}
+
+static int ublox_lte_probe(struct ofono_lte *lte, void *data)
+{
+	GAtChat *chat = data;
+	struct lte_driver_data *ldd;
+
+	DBG("ublox lte probe");
+
+	ldd = g_try_new0(struct lte_driver_data, 1);
+	if (!ldd) {
+		return -ENOMEM;
+	}
+
+	ldd->chat = g_at_chat_clone(chat);
+
+	ofono_lte_set_data(lte, ldd);
+
+	g_idle_add(lte_delayed_register, lte);
+
+	return 0;
+}
+
+static void ublox_lte_remove(struct ofono_lte *lte)
+{
+	struct lte_driver_data *ldd = ofono_lte_get_data(lte);
+
+	DBG("ublox lte remove");
+
+	g_at_chat_unref(ldd->chat);
+
+	ofono_lte_set_data(lte, NULL);
+
+	g_free(ldd);
+}
+
+static struct ofono_lte_driver driver = {
+	.name				= UBLOXMODEM,
+	.probe				= ublox_lte_probe,
+	.remove				= ublox_lte_remove,
+	.set_default_attach_info	= ublox_lte_set_default_attach_info,
+};
+
+void ublox_lte_init(void)
+{
+	ofono_lte_driver_register(&driver);
+}
+
+void ublox_lte_exit(void)
+{
+	ofono_lte_driver_unregister(&driver);
+}
diff --git a/drivers/ubloxmodem/ubloxmodem.c b/drivers/ubloxmodem/ubloxmodem.c
index 7fc671e..93cb928 100644
--- a/drivers/ubloxmodem/ubloxmodem.c
+++ b/drivers/ubloxmodem/ubloxmodem.c
@@ -29,12 +29,14 @@
 #define OFONO_API_SUBJECT_TO_CHANGE
 #include <ofono/plugin.h>
 #include <ofono/types.h>
+#include <ofono/modem.h>
 
 #include "ubloxmodem.h"
 
 static int ubloxmodem_init(void)
 {
 	ublox_gprs_context_init();
+	ublox_lte_init();
 
 	return 0;
 }
@@ -42,6 +44,7 @@ static int ubloxmodem_init(void)
 static void ubloxmodem_exit(void)
 {
 	ublox_gprs_context_exit();
+	ublox_lte_exit();
 }
 
 OFONO_PLUGIN_DEFINE(ubloxmodem, "U-Blox Toby L2 high speed modem driver",
diff --git a/drivers/ubloxmodem/ubloxmodem.h b/drivers/ubloxmodem/ubloxmodem.h
index 0c8a621..cf66412 100644
--- a/drivers/ubloxmodem/ubloxmodem.h
+++ b/drivers/ubloxmodem/ubloxmodem.h
@@ -21,5 +21,10 @@
 
 #include <drivers/atmodem/atutil.h>
 
+#define UBLOXMODEM "ubloxmodem"
+
 extern void ublox_gprs_context_init(void);
 extern void ublox_gprs_context_exit(void);
+
+extern void ublox_lte_init(void);
+extern void ublox_lte_exit(void);
-- 
2.7.4

Re: [PATCH 7/9] ubloxmodem: add lte atom driver

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 6602 bytes --]

Hi Dragos,

On 11/10/2016 10:55 AM, Dragos Tatulea wrote:
> Adds U-Blox Toby L2 driver for setting the default APN via the
> +UCGDFLT command. Currently only IPv4 is supported. APN is
> not stored to modem's non-volatile memory. oFono will manage this
> default APN via it's config storage.
>
> When receiving an empty default APN, the value is reset.
> ---
>   drivers/ubloxmodem/lte.c        | 154 ++++++++++++++++++++++++++++++++++++++++
>   drivers/ubloxmodem/ubloxmodem.c |   3 +
>   drivers/ubloxmodem/ubloxmodem.h |   5 ++
>   3 files changed, 162 insertions(+)
>   create mode 100644 drivers/ubloxmodem/lte.c
>
> diff --git a/drivers/ubloxmodem/lte.c b/drivers/ubloxmodem/lte.c
> new file mode 100644
> index 0000000..43c02d4
> --- /dev/null
> +++ b/drivers/ubloxmodem/lte.c
> @@ -0,0 +1,154 @@
> +/*
> + *
> + *  oFono - Open Source Telephony
> + *
> + *  Copyright (C) 2016  Endocode AG. All rights reserved.
> + *
> + *  This program is free software; you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License version 2 as
> + *  published by the Free Software Foundation.
> + *
> + *  This program is distributed in the hope that it will be useful,
> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + *  GNU General Public License for more details.
> + *
> + *  You should have received a copy of the GNU General Public License
> + *  along with this program; if not, write to the Free Software
> + *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
> + *
> + */
> +#ifdef HAVE_CONFIG_H
> +#include <config.h>
> +#endif
> +
> +#define _GNU_SOURCE
> +#include <stdlib.h>
> +#include <stdio.h>
> +#include <string.h>
> +#include <errno.h>
> +
> +#include <glib.h>
> +
> +#include <ofono/modem.h>
> +#include <ofono/gprs-context.h>
> +#include <ofono/log.h>
> +#include <ofono/lte.h>
> +
> +#include "gatchat.h"
> +#include "gatresult.h"
> +
> +#include "ubloxmodem.h"
> +
> +static const char *ucgdflt_prefix[] = { "+UCGDFLT:", NULL };
> +
> +struct lte_driver_data {
> +	GAtChat *chat;
> +};
> +
> +static void ucgdflt_cb(gboolean ok, GAtResult *result, gpointer user_data)
> +{
> +	struct cb_data *cbd = user_data;
> +	ofono_lte_cb_t cb = cbd->cb;
> +
> +	DBG("ok %d", ok);
> +
> +	if (!ok) {
> +		struct ofono_error error;
> +
> +		decode_at_error(&error, g_at_result_final_response(result));
> +		cb(&error, cbd->data);

decode_at_error works for OK responses as well, so there's no need for 
this if-else statement.

> +	} else {
> +		CALLBACK_WITH_SUCCESS(cb, cbd->data);
> +	}
> +
> +	g_free(cbd);

This should not be necessary either.  See below.

> +
> +	return;

?? this return is pointless

> +}
> +
> +static int ublox_lte_set_default_attach_info(const struct ofono_lte *lte,
> +				struct ofono_lte_default_attach_info *info,
> +				ofono_lte_cb_t cb, void *data)
> +{
> +	struct lte_driver_data *ldd = ofono_lte_get_data(lte);
> +	char buf[32 + OFONO_GPRS_MAX_APN_LENGTH + 1];
> +	struct cb_data *cbd = cb_data_new(cb, data);
> +
> +	DBG("LTE config with APN: %s", info->apn);
> +
> +	if (strlen(info->apn) > 0)
> +		snprintf(buf, sizeof(buf), "AT+UCGDFLT=0,\"IP\",\"%s\"",
> +				info->apn);
> +	else
> +		snprintf(buf, sizeof(buf), "AT+UCGDFLT=0");
> +
> +	/* We can't do much in case of failure so don't check response. */
> +	if (g_at_chat_send(ldd->chat, buf, ucgdflt_prefix,
> +				ucgdflt_cb, cbd, NULL) == 0) {

You should be setting the last argument (e.g. GDestroyNotify) as g_free 
to avoid memory leaks.

> +		g_free(cbd);
> +		CALLBACK_WITH_FAILURE(cb, data);
> +	}

This is far easier to write as:

if (g_at_chat_send(....) > 0)
	return;

CALLBACK_WITH_FAILURE();

> +
> +	return 0;

Only probe() can return error values.  All other methods should return 
void.  I pushed a fix to include/lte.h to reflect this.

> +}
> +
> +static gboolean lte_delayed_register(gpointer user_data)
> +{
> +	struct ofono_lte *lte = user_data;
> +
> +	ofono_lte_register(lte);
> +
> +	return FALSE;
> +}
> +
> +static int ublox_lte_probe(struct ofono_lte *lte, void *data)
> +{
> +	GAtChat *chat = data;
> +	struct lte_driver_data *ldd;
> +
> +	DBG("ublox lte probe");
> +
> +	ldd = g_try_new0(struct lte_driver_data, 1);
> +	if (!ldd) {
> +		return -ENOMEM;
> +	}
> +
> +	ldd->chat = g_at_chat_clone(chat);
> +
> +	ofono_lte_set_data(lte, ldd);
> +
> +	g_idle_add(lte_delayed_register, lte);
> +
> +	return 0;
> +}
> +
> +static void ublox_lte_remove(struct ofono_lte *lte)
> +{
> +	struct lte_driver_data *ldd = ofono_lte_get_data(lte);
> +
> +	DBG("ublox lte remove");
> +
> +	g_at_chat_unref(ldd->chat);
> +
> +	ofono_lte_set_data(lte, NULL);
> +
> +	g_free(ldd);
> +}
> +
> +static struct ofono_lte_driver driver = {
> +	.name				= UBLOXMODEM,
> +	.probe				= ublox_lte_probe,
> +	.remove				= ublox_lte_remove,
> +	.set_default_attach_info	= ublox_lte_set_default_attach_info,
> +};
> +
> +void ublox_lte_init(void)
> +{
> +	ofono_lte_driver_register(&driver);
> +}
> +
> +void ublox_lte_exit(void)
> +{
> +	ofono_lte_driver_unregister(&driver);
> +}
> diff --git a/drivers/ubloxmodem/ubloxmodem.c b/drivers/ubloxmodem/ubloxmodem.c
> index 7fc671e..93cb928 100644
> --- a/drivers/ubloxmodem/ubloxmodem.c
> +++ b/drivers/ubloxmodem/ubloxmodem.c
> @@ -29,12 +29,14 @@
>   #define OFONO_API_SUBJECT_TO_CHANGE
>   #include <ofono/plugin.h>
>   #include <ofono/types.h>
> +#include <ofono/modem.h>
>
>   #include "ubloxmodem.h"
>
>   static int ubloxmodem_init(void)
>   {
>   	ublox_gprs_context_init();
> +	ublox_lte_init();
>
>   	return 0;
>   }
> @@ -42,6 +44,7 @@ static int ubloxmodem_init(void)
>   static void ubloxmodem_exit(void)
>   {
>   	ublox_gprs_context_exit();
> +	ublox_lte_exit();
>   }
>
>   OFONO_PLUGIN_DEFINE(ubloxmodem, "U-Blox Toby L2 high speed modem driver",
> diff --git a/drivers/ubloxmodem/ubloxmodem.h b/drivers/ubloxmodem/ubloxmodem.h
> index 0c8a621..cf66412 100644
> --- a/drivers/ubloxmodem/ubloxmodem.h
> +++ b/drivers/ubloxmodem/ubloxmodem.h
> @@ -21,5 +21,10 @@
>
>   #include <drivers/atmodem/atutil.h>
>
> +#define UBLOXMODEM "ubloxmodem"
> +
>   extern void ublox_gprs_context_init(void);
>   extern void ublox_gprs_context_exit(void);
> +
> +extern void ublox_lte_init(void);
> +extern void ublox_lte_exit(void);
>

Regards,
-Denis

[PATCH 8/9] build: add support for ublox lte atom driver

[Dragos Tatulea]

[-- Attachment #1: Type: text/plain, Size: 512 bytes --]

---
 Makefile.am | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/Makefile.am b/Makefile.am
index 9d22365..08c7395 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -382,7 +382,9 @@ builtin_modules += ubloxmodem
 builtin_sources += drivers/atmodem/atutil.h \
 			drivers/ubloxmodem/ubloxmodem.h \
 			drivers/ubloxmodem/ubloxmodem.c \
-			drivers/ubloxmodem/gprs-context.c
+			drivers/ubloxmodem/gprs-context.c \
+			drivers/ubloxmodem/lte.c
+
 
 
 if PHONESIM
-- 
2.7.4

[PATCH 9/9] plugins: ublox: enable lte driver for tobyl2

[Dragos Tatulea]

[-- Attachment #1: Type: text/plain, Size: 636 bytes --]

---
 plugins/ublox.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/plugins/ublox.c b/plugins/ublox.c
index 092ba64..6d77df8 100644
--- a/plugins/ublox.c
+++ b/plugins/ublox.c
@@ -38,6 +38,7 @@
 #include <ofono/sim.h>
 #include <ofono/gprs.h>
 #include <ofono/gprs-context.h>
+#include <ofono/lte.h>
 
 #include <drivers/atmodem/atutil.h>
 #include <drivers/atmodem/vendor.h>
@@ -313,6 +314,8 @@ static void ublox_post_sim(struct ofono_modem *modem)
 
 		--ncontexts;
 	}
+
+	ofono_lte_create(modem, "ubloxmodem", data->aux);
 }
 
 static void ublox_post_online(struct ofono_modem *modem)
-- 
2.7.4

[PATCH 0/9] RFC: add LTE atom

[Dragos Tatulea]

[-- Attachment #1: Type: text/plain, Size: 1769 bytes --]

This patchset adds a simple LTE atom. It can only set the default
APN. But it's meant to be easily extensible in the future.

The atom exposes a per modem D-Bus interface. It also uses the
oFono persistent configuration store.

The patchset contains:
* lte atom implementation
* U-Blox Toby L2 lte driver implementation
* script for setting the lte properties

The last patches contain the implementation of the TobyL2 driver.
They might sit better in a different patchset maybe.

What is not here:
* Atom is always shown, regardles of technology. This should be
  filtered out.
* No provisioning support yet.

Dragos Tatulea (9):
  include: add header file for lte atom
  src: ofono.h: add LTE atom define
  lte: add implementation for LTE atom
  build: add lte atom support
  test: add script for setting lte atom properties
  doc: add lte atom documentation
  ubloxmodem: add lte atom driver
  build: add support for ublox lte atom driver
  plugins: ublox: enable lte driver for tobyl2

 Makefile.am                     |   8 +-
 doc/lte-api.txt                 |  35 +++++
 drivers/ubloxmodem/lte.c        | 125 +++++++++++++++
 drivers/ubloxmodem/ubloxmodem.c |   3 +
 drivers/ubloxmodem/ubloxmodem.h |   5 +
 include/lte.h                   |  69 ++++++++
 plugins/ublox.c                 |   3 +
 src/lte.c                       | 341 ++++++++++++++++++++++++++++++++++++++++
 src/ofono.h                     |   1 +
 test/set-lte-property           |  25 +++
 10 files changed, 612 insertions(+), 3 deletions(-)
 create mode 100644 doc/lte-api.txt
 create mode 100644 drivers/ubloxmodem/lte.c
 create mode 100644 include/lte.h
 create mode 100644 src/lte.c
 create mode 100755 test/set-lte-property

-- 
2.7.4

[PATCH 3/9] lte: add implementation for LTE atom

[Dragos Tatulea]

[-- Attachment #1: Type: text/plain, Size: 9067 bytes --]

This implementation can only get/set the default APN setting. But
anything expected for this atom is there:
* D-Bus interface
* sync-ing settings to/from file
* interaction with driver
---
 src/lte.c | 341 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 341 insertions(+)
 create mode 100644 src/lte.c

diff --git a/src/lte.c b/src/lte.c
new file mode 100644
index 0000000..cde0496
--- /dev/null
+++ b/src/lte.c
@@ -0,0 +1,341 @@
+/*
+ *
+ *  oFono - Open Source Telephony
+ *
+ *  Copyright (C) 2016  Endocode AG. All rights reserved.
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License version 2 as
+ *  published by the Free Software Foundation.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; if not, write to the Free Software
+ *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+ *
+ */
+
+#ifdef HAVE_CONFIG_H
+#include <config.h>
+#endif
+
+#include <string.h>
+#include <errno.h>
+
+#include <glib.h>
+#include <gdbus.h>
+
+#include "ofono.h"
+
+#include "common.h"
+#include "log.h"
+#include "gprs-context.h"
+#include "storage.h"
+
+#include "lte.h"
+
+#define SETTINGS_STORE "lte"
+#define SETTINGS_GROUP "Settings"
+
+struct ofono_lte {
+	const struct ofono_lte_driver *driver;
+	void *driver_data;
+	struct ofono_atom *atom;
+	char *imsi;
+	GKeyFile *settings;
+	struct ofono_lte_context_config config;
+};
+
+static GSList *g_drivers = NULL;
+
+static void lte_load_settings(struct ofono_lte *lte)
+{
+	char *apn;
+
+	if (lte->imsi == NULL)
+		return;
+
+	lte->settings = storage_open(lte->imsi, SETTINGS_STORE);
+
+	if (lte->settings == NULL) {
+		ofono_error("LTE: Can't open settings file, "
+				"changes won't be persistent");
+		return;
+	}
+
+	apn = g_key_file_get_string(lte->settings, SETTINGS_GROUP ,
+					"DefaultAPN", NULL);
+	if (apn) {
+		strcpy(lte->config.apn, apn);
+
+		if (lte->driver)
+			lte->driver->config(lte, &lte->config, NULL, NULL);
+	}
+}
+
+static void append_lte_properties(struct ofono_lte *lte,
+					DBusMessageIter *dict)
+{
+	const char *apn = lte->config.apn;
+
+	ofono_dbus_dict_append(dict, "DefaultAPN", DBUS_TYPE_STRING, &apn);
+}
+
+static DBusMessage *lte_get_properties(DBusConnection *conn,
+					DBusMessage *msg, void *data)
+{
+	struct ofono_lte *lte = data;
+	DBusMessage *reply;
+	DBusMessageIter iter;
+	DBusMessageIter dict;
+
+	reply = dbus_message_new_method_return(msg);
+	if (reply == NULL)
+		return NULL;
+
+	dbus_message_iter_init_append(reply, &iter);
+
+	dbus_message_iter_open_container(&iter, DBUS_TYPE_ARRAY,
+					OFONO_PROPERTIES_ARRAY_SIGNATURE,
+					&dict);
+	append_lte_properties(lte, &dict);
+	dbus_message_iter_close_container(&iter, &dict);
+
+	return reply;
+}
+
+static DBusMessage *lte_set_default_apn(struct ofono_lte *lte,
+				DBusConnection *conn, DBusMessage *msg,
+				const char *apn)
+{
+	const char *path = __ofono_atom_get_path(lte->atom);
+
+	if (strlen(apn) > OFONO_GPRS_MAX_APN_LENGTH)
+		return __ofono_error_invalid_format(msg);
+
+	if (g_str_equal(apn, lte->config.apn))
+		return dbus_message_new_method_return(msg);
+
+	/* We do care about empty value: it can be used for reset. */
+	if (is_valid_apn(apn) == FALSE && apn[0] != '\0')
+		return __ofono_error_invalid_format(msg);
+
+	strcpy(lte->config.apn, apn);
+
+	if (lte->settings) {
+		if (strlen(apn) == 0)
+			/* Clear entry on empty APN. */
+			g_key_file_remove_key(lte->settings, SETTINGS_GROUP,
+						"DefaultAPN", NULL);
+		else
+			g_key_file_set_string(lte->settings, SETTINGS_GROUP,
+						"DefaultAPN", lte->config.apn);
+
+		storage_sync(lte->imsi, SETTINGS_STORE, lte->settings);
+	}
+
+	if (lte->driver)
+		lte->driver->config(lte, &lte->config, NULL, NULL);
+
+	if (msg)
+		g_dbus_send_reply(conn, msg, DBUS_TYPE_INVALID);
+
+	ofono_dbus_signal_property_changed(conn, path,
+					OFONO_CONNECTION_CONTEXT_INTERFACE,
+					"DefaultAPN",
+					DBUS_TYPE_STRING, &apn);
+
+	return NULL;
+
+}
+
+static DBusMessage *lte_set_property(DBusConnection *conn,
+					DBusMessage *msg, void *data)
+{
+	struct ofono_lte *lte = data;
+	DBusMessageIter iter;
+	DBusMessageIter var;
+	const char *property;
+	const char *str;
+
+	if (!dbus_message_iter_init(msg, &iter))
+		return __ofono_error_invalid_args(msg);
+
+	if (dbus_message_iter_get_arg_type(&iter) != DBUS_TYPE_STRING)
+		return __ofono_error_invalid_args(msg);
+
+	dbus_message_iter_get_basic(&iter, &property);
+	dbus_message_iter_next(&iter);
+
+	if (dbus_message_iter_get_arg_type(&iter) != DBUS_TYPE_VARIANT)
+		return __ofono_error_invalid_args(msg);
+
+	dbus_message_iter_recurse(&iter, &var);
+
+
+	if (!strcmp(property, "DefaultAPN")) {
+		if (dbus_message_iter_get_arg_type(&var) != DBUS_TYPE_STRING)
+			return __ofono_error_invalid_args(msg);
+
+		dbus_message_iter_get_basic(&var, &str);
+
+		return lte_set_default_apn(lte, conn, msg, str);
+	}
+
+
+	return __ofono_error_invalid_args(msg);
+}
+
+static const GDBusMethodTable lte_methods[] = {
+	{ GDBUS_METHOD("GetProperties",
+			NULL, GDBUS_ARGS({ "properties", "a{sv}" }),
+			lte_get_properties) },
+	{ GDBUS_ASYNC_METHOD("SetProperty",
+			GDBUS_ARGS({ "property", "s" }, { "value", "v" }),
+			NULL, lte_set_property) },
+	{ }
+};
+
+static const GDBusSignalTable lte_signals[] = {
+	{ GDBUS_SIGNAL("PropertyChanged",
+			GDBUS_ARGS({ "name", "s" }, { "value", "v" })) },
+	{ }
+};
+
+static void lte_atom_remove(struct ofono_atom *atom)
+{
+	struct ofono_lte *lte = __ofono_atom_get_data(atom);
+
+	DBG("atom: %p", atom);
+
+	if (lte == NULL)
+		return;
+
+	if (lte->settings) {
+		storage_close(lte->imsi, SETTINGS_STORE, lte->settings, TRUE);
+
+		g_free(lte->imsi);
+		lte->imsi = NULL;
+		lte->settings = NULL;
+	}
+
+	if (lte->driver && lte->driver->remove)
+		lte->driver->remove(lte);
+
+	g_free(lte);
+}
+
+
+struct ofono_lte *ofono_lte_create(struct ofono_modem *modem,
+					const char *driver, void *data)
+{
+	struct ofono_lte *lte;
+	GSList *l;
+
+	if (driver == NULL)
+		return NULL;
+
+	lte = g_try_new0(struct ofono_lte, 1);
+
+	if (lte == NULL)
+		return NULL;
+
+	lte->atom = __ofono_modem_add_atom(modem, OFONO_ATOM_TYPE_LTE,
+						lte_atom_remove, lte);
+
+	for (l = g_drivers; l; l = l->next) {
+		const struct ofono_lte_driver *drv = l->data;
+
+		if (g_strcmp0(drv->name, driver))
+			continue;
+
+		if (drv->probe(lte, data) < 0)
+			continue;
+
+		lte->driver = drv;
+		break;
+	}
+
+	DBG("LTE atom created");
+
+	return lte;
+}
+
+int ofono_lte_driver_register(const struct ofono_lte_driver *d)
+{
+	DBG("driver: %p, name: %s", d, d->name);
+
+	if (d->probe == NULL)
+		return -EINVAL;
+
+	g_drivers = g_slist_prepend(g_drivers, (void *) d);
+
+	return 0;
+}
+
+void ofono_lte_driver_unregister(const struct ofono_lte_driver *d)
+{
+	DBG("driver: %p, name: %s", d, d->name);
+
+	g_drivers = g_slist_remove(g_drivers, (void *) d);
+}
+
+static void lte_atom_unregister(struct ofono_atom *atom)
+{
+	DBusConnection *conn = ofono_dbus_get_connection();
+	struct ofono_modem *modem = __ofono_atom_get_modem(atom);
+	const char *path = __ofono_atom_get_path(atom);
+
+	ofono_modem_remove_interface(modem, OFONO_LTE_INTERFACE);
+	g_dbus_unregister_interface(conn, path, OFONO_LTE_INTERFACE);
+}
+
+void ofono_lte_register(struct ofono_lte *lte)
+{
+	DBusConnection *conn = ofono_dbus_get_connection();
+	struct ofono_modem *modem = __ofono_atom_get_modem(lte->atom);
+	const char *path = __ofono_atom_get_path(lte->atom);
+	struct ofono_sim *sim = __ofono_atom_find(OFONO_ATOM_TYPE_SIM, modem);
+	const char *imsi = ofono_sim_get_imsi(sim);
+
+	if (imsi == NULL) {
+		ofono_error("No sim atom required for registering LTE atom.");	
+		return;
+	}
+
+	lte->imsi = g_strdup(imsi);
+
+	lte_load_settings(lte);
+
+	if (!g_dbus_register_interface(conn, path,
+				OFONO_LTE_INTERFACE,
+				lte_methods, lte_signals, NULL,
+				lte, NULL)) {
+		ofono_error("Could not create %s interface",
+				OFONO_LTE_INTERFACE);
+		return;
+	}
+
+	ofono_modem_add_interface(modem, OFONO_LTE_INTERFACE);
+
+	__ofono_atom_register(lte->atom, lte_atom_unregister);
+}
+
+void ofono_lte_remove(struct ofono_lte *lte)
+{
+	__ofono_atom_free(lte->atom);
+}
+
+void ofono_lte_set_data(struct ofono_lte *lte, void *data)
+{
+	lte->driver_data = data;
+}
+
+void *ofono_lte_get_data(struct ofono_lte *lte)
+{
+	return lte->driver_data;
+}
-- 
2.7.4

Re: [PATCH 3/9] lte: add implementation for LTE atom

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 10325 bytes --]

Hi Dragos,

On 11/09/2016 11:43 AM, Dragos Tatulea wrote:
> This implementation can only get/set the default APN setting. But
> anything expected for this atom is there:
> * D-Bus interface
> * sync-ing settings to/from file
> * interaction with driver
> ---
>   src/lte.c | 341 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 341 insertions(+)
>   create mode 100644 src/lte.c
>
> diff --git a/src/lte.c b/src/lte.c
> new file mode 100644
> index 0000000..cde0496
> --- /dev/null
> +++ b/src/lte.c
> @@ -0,0 +1,341 @@
> +/*
> + *
> + *  oFono - Open Source Telephony
> + *
> + *  Copyright (C) 2016  Endocode AG. All rights reserved.
> + *
> + *  This program is free software; you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License version 2 as
> + *  published by the Free Software Foundation.
> + *
> + *  This program is distributed in the hope that it will be useful,
> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + *  GNU General Public License for more details.
> + *
> + *  You should have received a copy of the GNU General Public License
> + *  along with this program; if not, write to the Free Software
> + *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
> + *
> + */
> +
> +#ifdef HAVE_CONFIG_H
> +#include <config.h>
> +#endif
> +
> +#include <string.h>
> +#include <errno.h>
> +
> +#include <glib.h>
> +#include <gdbus.h>
> +
> +#include "ofono.h"
> +
> +#include "common.h"
> +#include "log.h"
> +#include "gprs-context.h"
> +#include "storage.h"
> +
> +#include "lte.h"
> +
> +#define SETTINGS_STORE "lte"
> +#define SETTINGS_GROUP "Settings"
> +
> +struct ofono_lte {
> +	const struct ofono_lte_driver *driver;
> +	void *driver_data;
> +	struct ofono_atom *atom;
> +	char *imsi;
> +	GKeyFile *settings;
> +	struct ofono_lte_context_config config;
> +};
> +
> +static GSList *g_drivers = NULL;
> +
> +static void lte_load_settings(struct ofono_lte *lte)
> +{
> +	char *apn;
> +
> +	if (lte->imsi == NULL)
> +		return;
> +
> +	lte->settings = storage_open(lte->imsi, SETTINGS_STORE);
> +
> +	if (lte->settings == NULL) {
> +		ofono_error("LTE: Can't open settings file, "
> +				"changes won't be persistent");
> +		return;
> +	}
> +
> +	apn = g_key_file_get_string(lte->settings, SETTINGS_GROUP ,
> +					"DefaultAPN", NULL);
> +	if (apn) {
> +		strcpy(lte->config.apn, apn);
> +
> +		if (lte->driver)
> +			lte->driver->config(lte, &lte->config, NULL, NULL);

We should only register the atom after restoring the settings succeeds. 
  radio-settings.c is a good example of this.

> +	}
> +}
> +
> +static void append_lte_properties(struct ofono_lte *lte,
> +					DBusMessageIter *dict)
> +{
> +	const char *apn = lte->config.apn;
> +
> +	ofono_dbus_dict_append(dict, "DefaultAPN", DBUS_TYPE_STRING, &apn);
> +}
> +
> +static DBusMessage *lte_get_properties(DBusConnection *conn,
> +					DBusMessage *msg, void *data)
> +{
> +	struct ofono_lte *lte = data;
> +	DBusMessage *reply;
> +	DBusMessageIter iter;
> +	DBusMessageIter dict;
> +
> +	reply = dbus_message_new_method_return(msg);
> +	if (reply == NULL)
> +		return NULL;
> +
> +	dbus_message_iter_init_append(reply, &iter);
> +
> +	dbus_message_iter_open_container(&iter, DBUS_TYPE_ARRAY,
> +					OFONO_PROPERTIES_ARRAY_SIGNATURE,
> +					&dict);
> +	append_lte_properties(lte, &dict);

Why bother with this function?  Can't we just append the properties here?

> +	dbus_message_iter_close_container(&iter, &dict);
> +
> +	return reply;
> +}
> +
> +static DBusMessage *lte_set_default_apn(struct ofono_lte *lte,
> +				DBusConnection *conn, DBusMessage *msg,
> +				const char *apn)
> +{
> +	const char *path = __ofono_atom_get_path(lte->atom);
> +
> +	if (strlen(apn) > OFONO_GPRS_MAX_APN_LENGTH)
> +		return __ofono_error_invalid_format(msg);
> +
> +	if (g_str_equal(apn, lte->config.apn))
> +		return dbus_message_new_method_return(msg);
> +
> +	/* We do care about empty value: it can be used for reset. */
> +	if (is_valid_apn(apn) == FALSE && apn[0] != '\0')
> +		return __ofono_error_invalid_format(msg);
> +
> +	strcpy(lte->config.apn, apn);
> +
> +	if (lte->settings) {
> +		if (strlen(apn) == 0)
> +			/* Clear entry on empty APN. */
> +			g_key_file_remove_key(lte->settings, SETTINGS_GROUP,
> +						"DefaultAPN", NULL);
> +		else
> +			g_key_file_set_string(lte->settings, SETTINGS_GROUP,
> +						"DefaultAPN", lte->config.apn);
> +
> +		storage_sync(lte->imsi, SETTINGS_STORE, lte->settings);
> +	}
> +
> +	if (lte->driver)
> +		lte->driver->config(lte, &lte->config, NULL, NULL);
> +
> +	if (msg)
> +		g_dbus_send_reply(conn, msg, DBUS_TYPE_INVALID);

This setup is a bit unusual from the rest of oFono design where we reply 
to the message & update settings from within the driver callback.  e.g. 
look at cbs.c and how it sets "Topics".

> +
> +	ofono_dbus_signal_property_changed(conn, path,
> +					OFONO_CONNECTION_CONTEXT_INTERFACE,
> +					"DefaultAPN",
> +					DBUS_TYPE_STRING, &apn);
> +
> +	return NULL;
> +
> +}
> +
> +static DBusMessage *lte_set_property(DBusConnection *conn,
> +					DBusMessage *msg, void *data)
> +{
> +	struct ofono_lte *lte = data;
> +	DBusMessageIter iter;
> +	DBusMessageIter var;
> +	const char *property;
> +	const char *str;
> +
> +	if (!dbus_message_iter_init(msg, &iter))
> +		return __ofono_error_invalid_args(msg);
> +
> +	if (dbus_message_iter_get_arg_type(&iter) != DBUS_TYPE_STRING)
> +		return __ofono_error_invalid_args(msg);
> +
> +	dbus_message_iter_get_basic(&iter, &property);
> +	dbus_message_iter_next(&iter);
> +
> +	if (dbus_message_iter_get_arg_type(&iter) != DBUS_TYPE_VARIANT)
> +		return __ofono_error_invalid_args(msg);
> +
> +	dbus_message_iter_recurse(&iter, &var);
> +
> +

no double empty lines please

> +	if (!strcmp(property, "DefaultAPN")) {
> +		if (dbus_message_iter_get_arg_type(&var) != DBUS_TYPE_STRING)
> +			return __ofono_error_invalid_args(msg);
> +
> +		dbus_message_iter_get_basic(&var, &str);
> +
> +		return lte_set_default_apn(lte, conn, msg, str);
> +	}
> +
> +

as above

> +	return __ofono_error_invalid_args(msg);
> +}
> +
> +static const GDBusMethodTable lte_methods[] = {
> +	{ GDBUS_METHOD("GetProperties",
> +			NULL, GDBUS_ARGS({ "properties", "a{sv}" }),
> +			lte_get_properties) },
> +	{ GDBUS_ASYNC_METHOD("SetProperty",
> +			GDBUS_ARGS({ "property", "s" }, { "value", "v" }),
> +			NULL, lte_set_property) },
> +	{ }
> +};
> +
> +static const GDBusSignalTable lte_signals[] = {
> +	{ GDBUS_SIGNAL("PropertyChanged",
> +			GDBUS_ARGS({ "name", "s" }, { "value", "v" })) },
> +	{ }
> +};
> +
> +static void lte_atom_remove(struct ofono_atom *atom)
> +{
> +	struct ofono_lte *lte = __ofono_atom_get_data(atom);
> +
> +	DBG("atom: %p", atom);
> +
> +	if (lte == NULL)
> +		return;
> +
> +	if (lte->settings) {
> +		storage_close(lte->imsi, SETTINGS_STORE, lte->settings, TRUE);
> +
> +		g_free(lte->imsi);
> +		lte->imsi = NULL;
> +		lte->settings = NULL;
> +	}
> +
> +	if (lte->driver && lte->driver->remove)
> +		lte->driver->remove(lte);
> +
> +	g_free(lte);
> +}
> +
> +

Double empty line ;)

> +struct ofono_lte *ofono_lte_create(struct ofono_modem *modem,
> +					const char *driver, void *data)
> +{
> +	struct ofono_lte *lte;
> +	GSList *l;
> +
> +	if (driver == NULL)
> +		return NULL;
> +
> +	lte = g_try_new0(struct ofono_lte, 1);
> +
> +	if (lte == NULL)
> +		return NULL;
> +
> +	lte->atom = __ofono_modem_add_atom(modem, OFONO_ATOM_TYPE_LTE,
> +						lte_atom_remove, lte);
> +
> +	for (l = g_drivers; l; l = l->next) {
> +		const struct ofono_lte_driver *drv = l->data;
> +
> +		if (g_strcmp0(drv->name, driver))
> +			continue;
> +
> +		if (drv->probe(lte, data) < 0)
> +			continue;
> +
> +		lte->driver = drv;
> +		break;
> +	}
> +
> +	DBG("LTE atom created");
> +
> +	return lte;
> +}
> +
> +int ofono_lte_driver_register(const struct ofono_lte_driver *d)
> +{
> +	DBG("driver: %p, name: %s", d, d->name);
> +
> +	if (d->probe == NULL)
> +		return -EINVAL;
> +
> +	g_drivers = g_slist_prepend(g_drivers, (void *) d);
> +
> +	return 0;
> +}
> +
> +void ofono_lte_driver_unregister(const struct ofono_lte_driver *d)
> +{
> +	DBG("driver: %p, name: %s", d, d->name);
> +
> +	g_drivers = g_slist_remove(g_drivers, (void *) d);
> +}
> +
> +static void lte_atom_unregister(struct ofono_atom *atom)
> +{
> +	DBusConnection *conn = ofono_dbus_get_connection();
> +	struct ofono_modem *modem = __ofono_atom_get_modem(atom);
> +	const char *path = __ofono_atom_get_path(atom);
> +
> +	ofono_modem_remove_interface(modem, OFONO_LTE_INTERFACE);
> +	g_dbus_unregister_interface(conn, path, OFONO_LTE_INTERFACE);
> +}
> +
> +void ofono_lte_register(struct ofono_lte *lte)
> +{
> +	DBusConnection *conn = ofono_dbus_get_connection();
> +	struct ofono_modem *modem = __ofono_atom_get_modem(lte->atom);
> +	const char *path = __ofono_atom_get_path(lte->atom);
> +	struct ofono_sim *sim = __ofono_atom_find(OFONO_ATOM_TYPE_SIM, modem);
> +	const char *imsi = ofono_sim_get_imsi(sim);
> +
> +	if (imsi == NULL) {
> +		ofono_error("No sim atom required for registering LTE atom.");	
> +		return;
> +	}
> +
> +	lte->imsi = g_strdup(imsi);
> +
> +	lte_load_settings(lte);
> +
> +	if (!g_dbus_register_interface(conn, path,
> +				OFONO_LTE_INTERFACE,
> +				lte_methods, lte_signals, NULL,
> +				lte, NULL)) {
> +		ofono_error("Could not create %s interface",
> +				OFONO_LTE_INTERFACE);
> +		return;
> +	}
> +
> +	ofono_modem_add_interface(modem, OFONO_LTE_INTERFACE);
> +
> +	__ofono_atom_register(lte->atom, lte_atom_unregister);
> +}
> +
> +void ofono_lte_remove(struct ofono_lte *lte)
> +{
> +	__ofono_atom_free(lte->atom);
> +}
> +
> +void ofono_lte_set_data(struct ofono_lte *lte, void *data)
> +{
> +	lte->driver_data = data;
> +}
> +
> +void *ofono_lte_get_data(struct ofono_lte *lte)
> +{
> +	return lte->driver_data;
> +}
>

Regards,
-Denis