[PATCH] Add call-volume interface to allow user adjust speaker and mic volume

[PATCH] Add call-volume interface to allow user adjust speaker and mic volume

[Zhang, Zhenhua]

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

Hi,

Attached is the patch to add call-volume interface for Ofono. It exposes two
properties: 'SpeakerVolume' and 'MicrophoneVolume' to allow user could control
phone volume through lower level driver. The ofono_call_volume_notify() is
used to report call volume property change notification to the upper 
application.

The default volume could be stored in the modem.conf or query from remote.
Each driver could report value in percentage to call-volume through
ofono_call_volume_notify(). And then the call-volume will sync volume with remote
side right after probe().

Please review it.

Regards,
Zhenhua
 

[-- Attachment #2: 0001-Add-call-volume-interface-to-allow-user-to-adjust-sp.patch --]
[-- Type: application/octet-stream, Size: 13932 bytes --]

From 87c8c86df23ef9f5705ba260201ba85b05db7b6f Mon Sep 17 00:00:00 2001
From: Zhenhua Zhang <zhenhua.zhang@intel.com>
Date: Fri, 25 Sep 2009 00:47:22 +0800
Subject: [PATCH] Add call-volume interface to allow user to adjust speaker and microphone volume.

---
 Makefile.am           |    4 +-
 include/call-volume.h |   67 +++++++++
 src/call-volume.c     |  374 +++++++++++++++++++++++++++++++++++++++++++++++++
 src/ofono.h           |    1 +
 4 files changed, 444 insertions(+), 2 deletions(-)
 create mode 100644 include/call-volume.h
 create mode 100644 src/call-volume.c

diff --git a/Makefile.am b/Makefile.am
index 02f8835..522c1b5 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -10,7 +10,7 @@ include_HEADERS = include/log.h include/plugin.h include/history.h \
 			include/phonebook.h include/ssn.h include/ussd.h \
 			include/sms.h include/sim.h include/message-waiting.h \
 			include/netreg.h include/voicecall.h include/devinfo.h \
-			include/cbs.h
+			include/cbs.h include/call-volume.h
 
 nodist_include_HEADERS = include/version.h
 
@@ -163,7 +163,7 @@ src_ofonod_SOURCES = $(gdbus_sources) $(builtin_sources) \
 			src/ssn.c src/call-barring.c src/sim.c \
 			src/phonebook.c src/history.c src/message-waiting.c \
 			src/simutil.h src/simutil.c src/storage.h \
-			src/storage.c src/cbs.c src/watch.c
+			src/storage.c src/cbs.c src/watch.c src/call-volume.c
 
 src_ofonod_LDADD = $(builtin_libadd) \
 			@GLIB_LIBS@ @GTHREAD_LIBS@ @DBUS_LIBS@ -ldl
diff --git a/include/call-volume.h b/include/call-volume.h
new file mode 100644
index 0000000..f36d2f4
--- /dev/null
+++ b/include/call-volume.h
@@ -0,0 +1,67 @@
+/*
+ *
+ *  oFono - Open Source Telephony
+ *
+ *  Copyright (C) 2008-2009  Intel Corporation. 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_CALL_VOLUME_H
+#define __OFONO_CALL_VOLUME_H
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+#include <ofono/types.h>
+#include <ofono/dbus.h>
+
+struct ofono_call_volume;
+
+typedef void (*ofono_call_volume_cb_t)(const struct ofono_error *error,
+					void *data);
+
+struct ofono_call_volume_driver {
+	const char *name;
+	int (*probe)(struct ofono_call_volume *cv, unsigned int vendor,
+			void *data);
+	void (*remove)(struct ofono_call_volume *cv);
+	void (*speaker_volume)(struct ofono_call_volume *cv, unsigned int percent,
+			ofono_call_volume_cb_t cb, void *data);
+	void (*microphone_volume)(struct ofono_call_volume *cv, unsigned int percent,
+			ofono_call_volume_cb_t cb, void *data);
+};
+
+void ofono_call_volume_notify(struct ofono_call_volume *cv,
+					const char *property, unsigned int percent);
+
+int ofono_call_volume_driver_register(const struct ofono_call_volume_driver *d);
+void ofono_call_volume_driver_unregister(const struct ofono_call_volume_driver *d);
+
+struct ofono_call_volume *ofono_call_volume_create(struct ofono_modem *modem,
+					unsigned int vendor, const char *driver, void *data);
+
+void ofono_call_volume_register(struct ofono_call_volume *cv);
+void ofono_call_volume_remove(struct ofono_call_volume *cv);
+
+void ofono_call_volume_set_data(struct ofono_call_volume *cv, void *data);
+void *ofono_call_volume_get_data(struct ofono_call_volume *cv);
+
+#ifdef __cplusplus
+}
+#endif
+
+#endif /* __OFONO_CALL_VOLUME_H */
diff --git a/src/call-volume.c b/src/call-volume.c
new file mode 100644
index 0000000..aa740c0
--- /dev/null
+++ b/src/call-volume.c
@@ -0,0 +1,374 @@
+/*
+ *
+ *  oFono - Open Source Telephony
+ *
+ *  Copyright (C) 2008-2009  Intel Corporation. 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 <string.h>
+#include <stdlib.h>
+#include <stdio.h>
+#include <errno.h>
+#include <unistd.h>
+
+#include <glib.h>
+
+#include <ofono/log.h>
+#include <ofono/modem.h>
+#include <ofono/call-volume.h>
+
+#include <gdbus.h>
+#include "ofono.h"
+#include "common.h"
+
+#define CALL_VOLUME_INTERFACE "org.ofono.CallVolume"
+
+#define CALL_VOLUME_FLAG_PENDING 0x1
+
+static GSList *g_drivers = NULL;
+
+struct ofono_call_volume {
+	DBusMessage *pending;
+	const struct ofono_call_volume_driver *driver;
+	int flags;
+	void *driver_data;
+	struct ofono_atom *atom;
+
+	unsigned int speaker_volume;
+	unsigned int microphone_volume;
+
+	unsigned int temp_volume; /* temp volume before it is accepted by remote */
+};
+
+static void call_volume_unregister(struct ofono_atom *atom);
+
+void ofono_call_volume_notify(struct ofono_call_volume *cv,
+						const char *property,
+						unsigned int percent)
+{
+	DBusConnection *conn = ofono_dbus_get_connection();
+	const char *path = __ofono_atom_get_path(cv->atom);
+
+	if(!strcmp(property, "SpeakerVolume")) {
+		cv->speaker_volume = percent;
+	}
+
+	if (!strcmp(property, "MicrophoneVolume")) {
+		cv->microphone_volume = percent;
+	}
+
+	ofono_dbus_signal_property_changed(conn, path, CALL_VOLUME_INTERFACE,
+						property, DBUS_TYPE_UINT32,
+						&percent);
+}
+
+static DBusMessage *cv_get_properties(DBusConnection *conn,
+						DBusMessage *msg, void *data)
+{
+	struct ofono_call_volume *cv = data;
+	DBusMessage *reply;
+	DBusMessageIter iter, dict;
+	unsigned int speaker_volume = cv->speaker_volume;
+	unsigned int microphone_volume = cv->microphone_volume;
+
+	if (cv->pending)
+		return __ofono_error_busy(msg);
+
+	cv->pending = dbus_message_ref(msg);
+
+	reply = dbus_message_new_method_return(cv->pending);
+
+	if (!reply)
+		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, "SpeakerVolume", DBUS_TYPE_UINT32,
+					&speaker_volume);
+
+	ofono_dbus_dict_append(&dict, "MicrophoneVolume",
+					DBUS_TYPE_UINT32, &microphone_volume);
+
+	dbus_message_iter_close_container(&iter, &dict);
+
+	__ofono_dbus_pending_reply(&cv->pending, reply);
+
+	return NULL;
+}
+
+static void generic_callback(const struct ofono_error *error, void *data)
+{
+	struct ofono_call_volume *cv = data;
+	DBusConnection *conn = ofono_dbus_get_connection();
+	DBusMessage *reply;
+
+	cv->flags &= ~CALL_VOLUME_FLAG_PENDING;
+
+	if (!cv->pending)
+		return;
+
+	if (error->type == OFONO_ERROR_TYPE_NO_ERROR)
+		reply = dbus_message_new_method_return(cv->pending);
+	else
+		reply = __ofono_error_failed(cv->pending);
+
+	g_dbus_send_message(conn, reply);
+
+	dbus_message_unref(cv->pending);
+	cv->pending = NULL;
+}
+
+static void sv_set_callback(const struct ofono_error *error, void *data)
+{
+	struct ofono_call_volume *cv = data;
+
+	if (error->type != OFONO_ERROR_TYPE_NO_ERROR) {
+		ofono_debug("Error occurred during Speaker Volume set: %s",
+						telephony_error_to_str(error));
+	} else {
+		cv->speaker_volume = cv->temp_volume;
+	}
+
+	generic_callback(error, data);
+}
+
+static void mv_set_callback(const struct ofono_error *error, void *data)
+{
+	struct ofono_call_volume *cv = data;
+
+	if (error->type != OFONO_ERROR_TYPE_NO_ERROR) {
+		ofono_debug("Error occurred during Microphone Volume set: %s",
+						telephony_error_to_str(error));
+	} else {
+		cv->microphone_volume = cv->temp_volume;
+	}
+
+	generic_callback(error, data);
+}
+
+static DBusMessage *cv_set_call_volume(DBusMessage *msg, struct ofono_call_volume *cv,
+					 const char *property, unsigned int percent)
+{
+	if (percent > 100)
+		return __ofono_error_invalid_format(msg);
+
+	DBG("set %s volume to %d percent", property, percent);
+
+	cv->flags |= CALL_VOLUME_FLAG_PENDING;
+	cv->temp_volume = percent;
+
+	if (msg)
+		cv->pending = dbus_message_ref(msg);
+
+	if(!strcmp(property, "SpeakerVolume")) {
+		if (!cv->driver->speaker_volume) {
+			return (msg != NULL)?__ofono_error_not_implemented(msg):NULL;
+		}
+		cv->driver->speaker_volume(cv, percent, sv_set_callback, cv);
+	}
+
+	if (!strcmp(property, "MicrophoneVolume")) {
+		if (!cv->driver->microphone_volume)
+			return (msg != NULL)?__ofono_error_not_implemented(msg):NULL;
+		cv->driver->microphone_volume(cv, percent, mv_set_callback, cv);
+	}
+
+	return NULL;
+}
+
+static DBusMessage *cv_set_property(DBusConnection *conn, DBusMessage *msg,
+					void *data)
+{
+	struct ofono_call_volume *cv = data;
+	DBusMessageIter iter;
+	DBusMessageIter var;
+	const char *property;
+	unsigned int percent;
+
+	if (cv->pending)
+		return __ofono_error_busy(msg);
+
+	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 (dbus_message_iter_get_arg_type(&var) != DBUS_TYPE_UINT32)
+		return __ofono_error_invalid_args(msg);
+
+	dbus_message_iter_get_basic(&var, &percent);
+
+	return cv_set_call_volume(msg, cv, property, percent);
+}
+
+static GDBusMethodTable cv_methods[] = {
+	{ "GetProperties",	"",	"a{sv}",	cv_get_properties,
+							G_DBUS_METHOD_FLAG_ASYNC},
+	{ "SetProperty",	"sv",	"",		cv_set_property,
+							G_DBUS_METHOD_FLAG_ASYNC },
+	{ }
+};
+
+static GDBusSignalTable cv_signals[] = {
+	{ "PropertyChanged",	"sv" },
+	{ }
+};
+
+static void call_volume_remove(struct ofono_atom *atom)
+{
+	struct ofono_call_volume *cv = __ofono_atom_get_data(atom);
+
+	DBG("atom: %p", atom);
+
+	if (cv== NULL)
+		return;
+
+	if (cv->driver && cv->driver->remove)
+		cv->driver->remove(cv);
+
+	g_free(cv);
+}
+
+struct ofono_call_volume *ofono_call_volume_create(struct ofono_modem *modem,
+					unsigned int vendor,
+					const char *driver,
+					void *data)
+{
+	struct ofono_call_volume *cv;
+	GSList *l;
+
+	if (driver == NULL)
+		return NULL;
+
+	cv = g_try_new0(struct ofono_call_volume, 1);
+	cv->speaker_volume = 100;
+	cv->microphone_volume = 100;
+
+	if (cv == NULL)
+		return NULL;
+
+	cv->atom = __ofono_modem_add_atom(modem,
+						OFONO_ATOM_TYPES_CALL_VOLUME,
+						call_volume_remove, cv);
+
+	for (l = g_drivers; l; l = l->next) {
+		const struct ofono_call_volume_driver *drv = l->data;
+
+		if (g_strcmp0(drv->name, driver))
+			continue;
+
+		if (drv->probe(cv, vendor, data) < 0)
+			continue;
+
+		cv->driver = drv;
+		break;
+	}
+
+	if (cv->driver) {
+		cv_set_call_volume(NULL, cv, "SpeakerVolume", cv->speaker_volume);
+		cv_set_call_volume(NULL, cv, "MicrophoneVolume", cv->microphone_volume);
+	}
+
+	return cv;
+}
+
+void ofono_call_volume_register(struct ofono_call_volume *cv)
+{
+	DBusConnection *conn = ofono_dbus_get_connection();
+	struct ofono_modem *modem = __ofono_atom_get_modem(cv->atom);
+	const char *path = __ofono_atom_get_path(cv->atom);
+
+	if (!g_dbus_register_interface(conn, path,
+					CALL_VOLUME_INTERFACE,
+					cv_methods, cv_signals, NULL,
+					cv, NULL)) {
+		ofono_error("Could not create %s interface",
+				CALL_VOLUME_INTERFACE);
+
+		return;
+	}
+
+	ofono_modem_add_interface(modem, CALL_VOLUME_INTERFACE);
+
+	__ofono_atom_register(cv->atom, call_volume_unregister);
+}
+
+void call_volume_unregister(struct ofono_atom *atom)
+{
+	DBusConnection *conn = ofono_dbus_get_connection();
+	struct ofono_call_volume *cv = __ofono_atom_get_data(atom);
+	struct ofono_modem *modem = __ofono_atom_get_modem(atom);
+	const char *path = __ofono_atom_get_path(atom);
+	GSList *l;
+
+	ofono_modem_remove_interface(modem, CALL_VOLUME_INTERFACE);
+	g_dbus_unregister_interface(conn, path,
+					CALL_VOLUME_INTERFACE);
+}
+
+int ofono_call_volume_driver_register(const struct ofono_call_volume_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_call_volume_driver_unregister(const struct ofono_call_volume_driver *d)
+{
+	DBG("driver: %p, name: %s", d, d->name);
+
+	g_drivers = g_slist_remove(g_drivers, (void *)d);
+}
+
+void ofono_call_volume_remove(struct ofono_call_volume *cv)
+{
+	__ofono_atom_free(cv->atom);
+}
+
+void ofono_call_volume_set_data(struct ofono_call_volume *cv, void *data)
+{
+	cv->driver_data = data;
+}
+
+void *ofono_call_volume_get_data(struct ofono_call_volume *cv)
+{
+	return cv->driver_data;
+}
+
diff --git a/src/ofono.h b/src/ofono.h
index 0659989..409a9e2 100644
--- a/src/ofono.h
+++ b/src/ofono.h
@@ -105,6 +105,7 @@ enum ofono_atom_type {
 	OFONO_ATOM_TYPE_SSN = 12,
 	OFONO_ATOM_TYPE_MESSAGE_WAITING = 13,
 	OFONO_ATOM_TYPE_CBS = 14,
+	OFONO_ATOM_TYPES_CALL_VOLUME = 15,
 };
 
 enum ofono_atom_watch_condition {
-- 
1.6.1.3

Re: [PATCH] Add call-volume interface to allow user adjust speaker and mic volume

[Marcel Holtmann]

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

Hi Zhenhua,

>  Makefile.am           |    4 +-
>  include/call-volume.h |   67 +++++++++
>  src/call-volume.c     |  374 +++++++++++++++++++++++++++++++++++++++++++++++++
>  src/ofono.h           |    1 +
>  4 files changed, 444 insertions(+), 2 deletions(-)
>  create mode 100644 include/call-volume.h
>  create mode 100644 src/call-volume.c
> 
> diff --git a/Makefile.am b/Makefile.am
> index 02f8835..522c1b5 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -10,7 +10,7 @@ include_HEADERS = include/log.h include/plugin.h include/history.h \
>                         include/phonebook.h include/ssn.h include/ussd.h \
>                         include/sms.h include/sim.h include/message-waiting.h \
>                         include/netreg.h include/voicecall.h include/devinfo.h \
> -                       include/cbs.h
> +                       include/cbs.h include/call-volume.h
>  
>  nodist_include_HEADERS = include/version.h
>  
> @@ -163,7 +163,7 @@ src_ofonod_SOURCES = $(gdbus_sources) $(builtin_sources) \
>                         src/ssn.c src/call-barring.c src/sim.c \
>                         src/phonebook.c src/history.c src/message-waiting.c \
>                         src/simutil.h src/simutil.c src/storage.h \
> -                       src/storage.c src/cbs.c src/watch.c
> +                       src/storage.c src/cbs.c src/watch.c src/call-volume.c
>  
>  src_ofonod_LDADD = $(builtin_libadd) \
>                         @GLIB_LIBS@ @GTHREAD_LIBS@ @DBUS_LIBS@ -ldl
> diff --git a/include/call-volume.h b/include/call-volume.h
> new file mode 100644
> index 0000000..f36d2f4
> --- /dev/null
> +++ b/include/call-volume.h
> @@ -0,0 +1,67 @@
> +/*
> + *
> + *  oFono - Open Source Telephony
> + *
> + *  Copyright (C) 2008-2009  Intel Corporation. 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_CALL_VOLUME_H
> +#define __OFONO_CALL_VOLUME_H
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
> +
> +#include <ofono/types.h>
> +#include <ofono/dbus.h>
> +
> +struct ofono_call_volume;
> +
> +typedef void (*ofono_call_volume_cb_t)(const struct ofono_error *error,
> +                                       void *data);
> +
> +struct ofono_call_volume_driver {
> +       const char *name;
> +       int (*probe)(struct ofono_call_volume *cv, unsigned int vendor,
> +                       void *data);
> +       void (*remove)(struct ofono_call_volume *cv);
> +       void (*speaker_volume)(struct ofono_call_volume *cv, unsigned int percent,
> +                       ofono_call_volume_cb_t cb, void *data);
> +       void (*microphone_volume)(struct ofono_call_volume *cv, unsigned int percent,
> +                       ofono_call_volume_cb_t cb, void *data);
> +};
> +
> +void ofono_call_volume_notify(struct ofono_call_volume *cv,
> +                                       const char *property, unsigned int percent);
> +
> +int ofono_call_volume_driver_register(const struct ofono_call_volume_driver *d);
> +void ofono_call_volume_driver_unregister(const struct ofono_call_volume_driver *d);
> +
> +struct ofono_call_volume *ofono_call_volume_create(struct ofono_modem *modem,
> +                                       unsigned int vendor, const char *driver, void *data);
> +
> +void ofono_call_volume_register(struct ofono_call_volume *cv);
> +void ofono_call_volume_remove(struct ofono_call_volume *cv);
> +
> +void ofono_call_volume_set_data(struct ofono_call_volume *cv, void *data);
> +void *ofono_call_volume_get_data(struct ofono_call_volume *cv);
> +
> +#ifdef __cplusplus
> +}
> +#endif
> +
> +#endif /* __OFONO_CALL_VOLUME_H */
> diff --git a/src/call-volume.c b/src/call-volume.c
> new file mode 100644
> index 0000000..aa740c0
> --- /dev/null
> +++ b/src/call-volume.c
> @@ -0,0 +1,374 @@
> +/*
> + *
> + *  oFono - Open Source Telephony
> + *
> + *  Copyright (C) 2008-2009  Intel Corporation. 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 <string.h>
> +#include <stdlib.h>
> +#include <stdio.h>
> +#include <errno.h>
> +#include <unistd.h>
> +
> +#include <glib.h>
> +
> +#include <ofono/log.h>
> +#include <ofono/modem.h>
> +#include <ofono/call-volume.h>
> +
> +#include <gdbus.h>
> +#include "ofono.h"
> +#include "common.h"
> +
> +#define CALL_VOLUME_INTERFACE "org.ofono.CallVolume"

we have to stop doing this. Start using this:

#define FOO_INTERFACE  OFONO_SERVICE ".FooInterface"

> +#define CALL_VOLUME_FLAG_PENDING 0x1
> +
> +static GSList *g_drivers = NULL;
> +
> +struct ofono_call_volume {
> +       DBusMessage *pending;
> +       const struct ofono_call_volume_driver *driver;
> +       int flags;
> +       void *driver_data;
> +       struct ofono_atom *atom;
> +
> +       unsigned int speaker_volume;
> +       unsigned int microphone_volume;
> +
> +       unsigned int temp_volume; /* temp volume before it is accepted by remote */
> +};
> +
> +static void call_volume_unregister(struct ofono_atom *atom);

That forward declaration seems pointless unless I actually missed
something. No forward declaration unless they are really needed. Just
shuffle the code around.

> +void ofono_call_volume_notify(struct ofono_call_volume *cv,
> +                                               const char *property,
> +                                               unsigned int percent)
> +{
> +       DBusConnection *conn = ofono_dbus_get_connection();
> +       const char *path = __ofono_atom_get_path(cv->atom);
> +
> +       if(!strcmp(property, "SpeakerVolume")) {
> +               cv->speaker_volume = percent;
> +       }

Coding style. Actually twice. No space behind if and single if
statements need no { }.

> +
> +       if (!strcmp(property, "MicrophoneVolume")) {
> +               cv->microphone_volume = percent;
> +       }

No  { } needed.

> +       ofono_dbus_signal_property_changed(conn, path, CALL_VOLUME_INTERFACE,
> +                                               property, DBUS_TYPE_UINT32,
> +                                               &percent);
> +}
> +
> +static DBusMessage *cv_get_properties(DBusConnection *conn,
> +                                               DBusMessage *msg, void *data)
> +{
> +       struct ofono_call_volume *cv = data;
> +       DBusMessage *reply;
> +       DBusMessageIter iter, dict;
> +       unsigned int speaker_volume = cv->speaker_volume;
> +       unsigned int microphone_volume = cv->microphone_volume;

What is this variable non-sense for. You can use cv->speaker_volume
directly from D-Bus message append functions. Only for certain strings
you need this.

> +
> +       if (cv->pending)
> +               return __ofono_error_busy(msg);
> +
> +       cv->pending = dbus_message_ref(msg);
> +
> +       reply = dbus_message_new_method_return(cv->pending);
> +
> +       if (!reply)
> +               return NULL;

What is this empty line for? Just don't do that. The error check
logically belongs to the function creating reply.

Also who unrefs cv->pending in this case?

> +       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, "SpeakerVolume", DBUS_TYPE_UINT32,
> +                                       &speaker_volume);
> +
> +       ofono_dbus_dict_append(&dict, "MicrophoneVolume",
> +                                       DBUS_TYPE_UINT32, &microphone_volume);

Why do we use UINT32 for percentage values? A BYTE would be clearly
enough.

> +
> +       dbus_message_iter_close_container(&iter, &dict);
> +
> +       __ofono_dbus_pending_reply(&cv->pending, reply);
> +
> +       return NULL;
> +}
> +
> +static void generic_callback(const struct ofono_error *error, void *data)
> +{
> +       struct ofono_call_volume *cv = data;
> +       DBusConnection *conn = ofono_dbus_get_connection();
> +       DBusMessage *reply;
> +
> +       cv->flags &= ~CALL_VOLUME_FLAG_PENDING;
> +
> +       if (!cv->pending)
> +               return;
> +
> +       if (error->type == OFONO_ERROR_TYPE_NO_ERROR)
> +               reply = dbus_message_new_method_return(cv->pending);
> +       else
> +               reply = __ofono_error_failed(cv->pending);
> +
> +       g_dbus_send_message(conn, reply);
> +
> +       dbus_message_unref(cv->pending);
> +       cv->pending = NULL;
> +}
> +
> +static void sv_set_callback(const struct ofono_error *error, void *data)
> +{
> +       struct ofono_call_volume *cv = data;
> +
> +       if (error->type != OFONO_ERROR_TYPE_NO_ERROR) {
> +               ofono_debug("Error occurred during Speaker Volume set: %s",
> +                                               telephony_error_to_str(error));
> +       } else {
> +               cv->speaker_volume = cv->temp_volume;
> +       }
> +
> +       generic_callback(error, data);
> +}
> +
> +static void mv_set_callback(const struct ofono_error *error, void *data)
> +{
> +       struct ofono_call_volume *cv = data;
> +
> +       if (error->type != OFONO_ERROR_TYPE_NO_ERROR) {
> +               ofono_debug("Error occurred during Microphone Volume set: %s",
> +                                               telephony_error_to_str(error));
> +       } else {
> +               cv->microphone_volume = cv->temp_volume;
> +       }
> +
> +       generic_callback(error, data);
> +}

So personally I think the ofono_debug() call makes these two function
hard to read and more complicated than needed. If we really want to
display this error then either use ofono_error() or just don't bother at
all with any kind of output.

> +static DBusMessage *cv_set_call_volume(DBusMessage *msg, struct ofono_call_volume *cv,
> +                                        const char *property, unsigned int percent)
> +{
> +       if (percent > 100)
> +               return __ofono_error_invalid_format(msg);
> +
> +       DBG("set %s volume to %d percent", property, percent);
> +
> +       cv->flags |= CALL_VOLUME_FLAG_PENDING;
> +       cv->temp_volume = percent;
> +
> +       if (msg)
> +               cv->pending = dbus_message_ref(msg);
> +
> +       if(!strcmp(property, "SpeakerVolume")) {
> +               if (!cv->driver->speaker_volume) {
> +                       return (msg != NULL)?__ofono_error_not_implemented(msg):NULL;
> +               }
> +               cv->driver->speaker_volume(cv, percent, sv_set_callback, cv);
> +       }

So the space after if is missing again. And this return statement is not
readable at all. It violates coding style and makes my brain hurt. Redo
this it becomes readable. Cramping everything in one line is just no
helpful.

> +
> +       if (!strcmp(property, "MicrophoneVolume")) {
> +               if (!cv->driver->microphone_volume)
> +                       return (msg != NULL)?__ofono_error_not_implemented(msg):NULL;
> +               cv->driver->microphone_volume(cv, percent, mv_set_callback, cv);
> +       }

See comment above.

> +
> +       return NULL;
> +}
> +
> +static DBusMessage *cv_set_property(DBusConnection *conn, DBusMessage *msg,
> +                                       void *data)
> +{
> +       struct ofono_call_volume *cv = data;
> +       DBusMessageIter iter;
> +       DBusMessageIter var;
> +       const char *property;
> +       unsigned int percent;
> +
> +       if (cv->pending)
> +               return __ofono_error_busy(msg);
> +
> +       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 (dbus_message_iter_get_arg_type(&var) != DBUS_TYPE_UINT32)
> +               return __ofono_error_invalid_args(msg);
> +
> +       dbus_message_iter_get_basic(&var, &percent);
> +
> +       return cv_set_call_volume(msg, cv, property, percent);
> +}
> +
> +static GDBusMethodTable cv_methods[] = {
> +       { "GetProperties",      "",     "a{sv}",        cv_get_properties,
> +                                                       G_DBUS_METHOD_FLAG_ASYNC},
> +       { "SetProperty",        "sv",   "",             cv_set_property,
> +                                                       G_DBUS_METHOD_FLAG_ASYNC },
> +       { }
> +};

I am missing the point why GetProperties has to be done async. That
looks like too much overhead for something just returning stored values.

> +
> +static GDBusSignalTable cv_signals[] = {
> +       { "PropertyChanged",    "sv" },
> +       { }
> +};
> +
> +static void call_volume_remove(struct ofono_atom *atom)
> +{
> +       struct ofono_call_volume *cv = __ofono_atom_get_data(atom);
> +
> +       DBG("atom: %p", atom);
> +
> +       if (cv== NULL)
> +               return;
> +
> +       if (cv->driver && cv->driver->remove)
> +               cv->driver->remove(cv);
> +
> +       g_free(cv);
> +}
> +
> +struct ofono_call_volume *ofono_call_volume_create(struct ofono_modem *modem,
> +                                       unsigned int vendor,
> +                                       const char *driver,
> +                                       void *data)
> +{
> +       struct ofono_call_volume *cv;
> +       GSList *l;
> +
> +       if (driver == NULL)
> +               return NULL;
> +
> +       cv = g_try_new0(struct ofono_call_volume, 1);
> +       cv->speaker_volume = 100;
> +       cv->microphone_volume = 100;
> +
> +       if (cv == NULL)
> +               return NULL;

How do you expect this to work actually? If cv == NULL, then your
assignment are NULL pointer dereferences and the daemon will crash. Your
error handling is not protecting it.

> +
> +       cv->atom = __ofono_modem_add_atom(modem,
> +                                               OFONO_ATOM_TYPES_CALL_VOLUME,
> +                                               call_volume_remove, cv);
> +
> +       for (l = g_drivers; l; l = l->next) {
> +               const struct ofono_call_volume_driver *drv = l->data;
> +
> +               if (g_strcmp0(drv->name, driver))
> +                       continue;
> +
> +               if (drv->probe(cv, vendor, data) < 0)
> +                       continue;
> +
> +               cv->driver = drv;
> +               break;
> +       }
> +
> +       if (cv->driver) {
> +               cv_set_call_volume(NULL, cv, "SpeakerVolume", cv->speaker_volume);
> +               cv_set_call_volume(NULL, cv, "MicrophoneVolume", cv->microphone_volume);
> +       }
> +
> +       return cv;
> +}
> +
> +void ofono_call_volume_register(struct ofono_call_volume *cv)
> +{
> +       DBusConnection *conn = ofono_dbus_get_connection();
> +       struct ofono_modem *modem = __ofono_atom_get_modem(cv->atom);
> +       const char *path = __ofono_atom_get_path(cv->atom);
> +
> +       if (!g_dbus_register_interface(conn, path,
> +                                       CALL_VOLUME_INTERFACE,
> +                                       cv_methods, cv_signals, NULL,
> +                                       cv, NULL)) {
> +               ofono_error("Could not create %s interface",
> +                               CALL_VOLUME_INTERFACE);
> +
> +               return;
> +       }
> +
> +       ofono_modem_add_interface(modem, CALL_VOLUME_INTERFACE);
> +
> +       __ofono_atom_register(cv->atom, call_volume_unregister);
> +}
> +
> +void call_volume_unregister(struct ofono_atom *atom)
> +{
> +       DBusConnection *conn = ofono_dbus_get_connection();
> +       struct ofono_call_volume *cv = __ofono_atom_get_data(atom);
> +       struct ofono_modem *modem = __ofono_atom_get_modem(atom);
> +       const char *path = __ofono_atom_get_path(atom);
> +       GSList *l;
> +
> +       ofono_modem_remove_interface(modem, CALL_VOLUME_INTERFACE);
> +       g_dbus_unregister_interface(conn, path,
> +                                       CALL_VOLUME_INTERFACE);
> +}
> +
> +int ofono_call_volume_driver_register(const struct ofono_call_volume_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);

I really hate these casts only because of const driver declaration.

Personally I am against it, but I see some value behind having this
const. However there has to be a space between the cast and the
variable.

> +
> +       return 0;
> +}
> +
> +void ofono_call_volume_driver_unregister(const struct ofono_call_volume_driver *d)
> +{
> +       DBG("driver: %p, name: %s", d, d->name);
> +
> +       g_drivers = g_slist_remove(g_drivers, (void *)d);
> +}

See comment above.

> +
> +void ofono_call_volume_remove(struct ofono_call_volume *cv)
> +{
> +       __ofono_atom_free(cv->atom);
> +}
> +
> +void ofono_call_volume_set_data(struct ofono_call_volume *cv, void *data)
> +{
> +       cv->driver_data = data;
> +}
> +
> +void *ofono_call_volume_get_data(struct ofono_call_volume *cv)
> +{
> +       return cv->driver_data;
> +}
> +
> diff --git a/src/ofono.h b/src/ofono.h
> index 0659989..409a9e2 100644
> --- a/src/ofono.h
> +++ b/src/ofono.h
> @@ -105,6 +105,7 @@ enum ofono_atom_type {
>         OFONO_ATOM_TYPE_SSN = 12,
>         OFONO_ATOM_TYPE_MESSAGE_WAITING = 13,
>         OFONO_ATOM_TYPE_CBS = 14,
> +       OFONO_ATOM_TYPES_CALL_VOLUME = 15,
>  };
>  
>  enum ofono_atom_watch_condition {

Regards

Marcel

RE: [PATCH] Add call-volume interface to allow user adjust speaker and mic volume

[Zhang, Zhenhua]

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

Hi Marcel,

Marcel Holtmann wrote:
> Hi Zhenhua,
> 
>> +#define CALL_VOLUME_INTERFACE "org.ofono.CallVolume"
> 
> we have to stop doing this. Start using this:
> 
> #define FOO_INTERFACE  OFONO_SERVICE ".FooInterface"
> 
>> +static void call_volume_unregister(struct ofono_atom *atom);
> 
> That forward declaration seems pointless unless I actually missed
> something. No forward declaration unless they are really needed. Just
> shuffle the code around.
> 
> Coding style. Actually twice. No space behind if and single if
> statements need no { }.
> 
>> +
>> +       if (!strcmp(property, "MicrophoneVolume")) {
>> +               cv->microphone_volume = percent;
>> +       }
> 
> No  { } needed.
> 
> What is this variable non-sense for. You can use cv->speaker_volume
> directly from D-Bus message append functions. Only for certain strings
> you need this.
> 
>> +
>> +       if (cv->pending)
>> +               return __ofono_error_busy(msg);
>> +
>> +       cv->pending = dbus_message_ref(msg);
>> +
>> +       reply = dbus_message_new_method_return(cv->pending); +
>> +       if (!reply)
>> +               return NULL;
> 
> What is this empty line for? Just don't do that. The error check
> logically belongs to the function creating reply.
> 
> Also who unrefs cv->pending in this case?
> 
>> +       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, "SpeakerVolume",
>> DBUS_TYPE_UINT32, +                                      
>> &speaker_volume); + +       ofono_dbus_dict_append(&dict,
>> "MicrophoneVolume", +                                      
>> DBUS_TYPE_UINT32, &microphone_volume); 
> 
> Why do we use UINT32 for percentage values? A BYTE would be clearly
> enough.
> 
>> +static void mv_set_callback(const struct ofono_error *error, void
>> *data) +{ +       struct ofono_call_volume *cv = data;
>> +
>> +       if (error->type != OFONO_ERROR_TYPE_NO_ERROR) {
>> +               ofono_debug("Error occurred during Microphone Volume
>> set: %s", +                                              
>> telephony_error_to_str(error)); +       } else { +              
>> cv->microphone_volume = cv->temp_volume; +       } +
>> +       generic_callback(error, data);
>> +}
> 
> So personally I think the ofono_debug() call makes these two function
> hard to read and more complicated than needed. If we really want to
> display this error then either use ofono_error() or just don't bother
> at all with any kind of output.
> 
>> +static DBusMessage *cv_set_call_volume(DBusMessage *msg, struct
>> ofono_call_volume *cv, +                                       
>> const char *property, unsigned int percent) +{ +       if (percent >
>> 100) +               return __ofono_error_invalid_format(msg); +
>> +       DBG("set %s volume to %d percent", property, percent); +
>> +       cv->flags |= CALL_VOLUME_FLAG_PENDING;
>> +       cv->temp_volume = percent;
>> +
>> +       if (msg)
>> +               cv->pending = dbus_message_ref(msg); +
>> +       if(!strcmp(property, "SpeakerVolume")) {
>> +               if (!cv->driver->speaker_volume) {
>> +                       return (msg !=
>> NULL)?__ofono_error_not_implemented(msg):NULL; +               } +  
>> cv->driver->speaker_volume(cv, percent, sv_set_callback, cv); +     
>> } 
> 
> So the space after if is missing again. And this return statement is
> not readable at all. It violates coding style and makes my brain
> hurt. Redo this it becomes readable. Cramping everything in one line
> is just no helpful.
> 
>> +static GDBusMethodTable cv_methods[] = {
>> +       { "GetProperties",      "",     "a{sv}",       
>> cv_get_properties, +                                                
>> G_DBUS_METHOD_FLAG_ASYNC}, +       { "SetProperty",        "sv",  
>> "",             cv_set_property, +                                  
>> G_DBUS_METHOD_FLAG_ASYNC }, +       { } +};
> 
> I am missing the point why GetProperties has to be done async. That
> looks like too much overhead for something just returning stored
> values. 
> 
>> +
>> +       if (driver == NULL)
>> +               return NULL;
>> +
>> +       cv = g_try_new0(struct ofono_call_volume, 1);
>> +       cv->speaker_volume = 100;
>> +       cv->microphone_volume = 100;
>> +
>> +       if (cv == NULL)
>> +               return NULL;
> 
> How do you expect this to work actually? If cv == NULL, then your
> assignment are NULL pointer dereferences and the daemon will crash.
> Your error handling is not protecting it.
> 
>> +int ofono_call_volume_driver_register(const struct
>> ofono_call_volume_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);
> 
> I really hate these casts only because of const driver declaration.
> 
> Personally I am against it, but I see some value behind having this
> const. However there has to be a space between the cast and the
> variable.
> 
>> +
>> +       return 0;
>> +}
>> +
>> +void ofono_call_volume_driver_unregister(const struct
>> ofono_call_volume_driver *d) +{ +       DBG("driver: %p, name: %s",
>> d, d->name); +
>> +       g_drivers = g_slist_remove(g_drivers, (void *)d);
>> +}
> 
> See comment above.
> 

Thank you for your valuable comments. The new patch fixed the code style
problem and I want to highlight some points here:

1. SpeakerVolume and MicrophoneVolume changes from DBUS_TYPE_UINT32 to
DBUS_TYPE_BYTE. The internal presentation is 'unsigned char' now.

2. In ofono_call_volume_create(), the initial speaker_volume and 
microphone_volume set to 50% instead of 100%. Each driver is responsible to 
probe modem and get the volume from mobile. Then it should use 
ofono_call_volume_notify() to overwrite default values.

3. However, in HFP spec v1.5, it's alway use HF side volume value to sync with
AG volume by using +VGS=, +VGM=. I don't see the AT command to query mobile
volume from the HF side. I expose ofono_set_call_volume() to lower driver to allow 
HFP driver sync volume with AG in the probe stage.

Please review it.

> 
> Regards
> 
> Marcel
> 
> 
> _______________________________________________
> ofono mailing list
> ofono(a)ofono.org
> http://lists.ofono.org/listinfo/ofono

Regards,
Zhenhua

[-- Attachment #2: 0002-Add-call-volume-interface-to-adjust-speaker-and-mic.patch --]
[-- Type: application/octet-stream, Size: 13661 bytes --]

From 790818b1470c8f18d39203fcc211905e63367c22 Mon Sep 17 00:00:00 2001
From: Zhenhua Zhang <zhenhua.zhang@intel.com>
Date: Sat, 26 Sep 2009 23:27:25 +0800
Subject: [PATCH] Add call volume interface to adjust speaker and mic volume

---
 Makefile.am           |    4 +-
 include/call-volume.h |   74 ++++++++++
 src/call-volume.c     |  366 +++++++++++++++++++++++++++++++++++++++++++++++++
 src/ofono.h           |    1 +
 4 files changed, 443 insertions(+), 2 deletions(-)
 create mode 100644 include/call-volume.h
 create mode 100644 src/call-volume.c

diff --git a/Makefile.am b/Makefile.am
index 55cedd8..ab6c3ba 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -10,7 +10,7 @@ include_HEADERS = include/log.h include/plugin.h include/history.h \
 			include/phonebook.h include/ssn.h include/ussd.h \
 			include/sms.h include/sim.h include/message-waiting.h \
 			include/netreg.h include/voicecall.h include/devinfo.h \
-			include/cbs.h
+			include/cbs.h include/call-volume.h
 
 nodist_include_HEADERS = include/version.h
 
@@ -163,7 +163,7 @@ src_ofonod_SOURCES = $(gdbus_sources) $(builtin_sources) \
 			src/ssn.c src/call-barring.c src/sim.c \
 			src/phonebook.c src/history.c src/message-waiting.c \
 			src/simutil.h src/simutil.c src/storage.h \
-			src/storage.c src/cbs.c src/watch.c
+			src/storage.c src/cbs.c src/watch.c src/call-volume.c
 
 src_ofonod_LDADD = $(builtin_libadd) \
 			@GLIB_LIBS@ @GTHREAD_LIBS@ @DBUS_LIBS@ -ldl
diff --git a/include/call-volume.h b/include/call-volume.h
new file mode 100644
index 0000000..ba2758f
--- /dev/null
+++ b/include/call-volume.h
@@ -0,0 +1,74 @@
+/*
+ *
+ *  oFono - Open Source Telephony
+ *
+ *  Copyright (C) 2008-2009  Intel Corporation. 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_CALL_VOLUME_H
+#define __OFONO_CALL_VOLUME_H
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+#include <ofono/types.h>
+#include <ofono/dbus.h>
+
+struct ofono_call_volume;
+
+typedef void (*ofono_call_volume_cb_t)(const struct ofono_error *error,
+					void *data);
+
+struct ofono_call_volume_driver {
+	const char *name;
+	int (*probe)(struct ofono_call_volume *cv, unsigned int vendor,
+			void *data);
+	void (*remove)(struct ofono_call_volume *cv);
+	void (*speaker_volume)(struct ofono_call_volume *cv,
+			unsigned char percent,
+			ofono_call_volume_cb_t cb, void *data);
+	void (*microphone_volume)(struct ofono_call_volume *cv,
+			unsigned char percent,
+			ofono_call_volume_cb_t cb, void *data);
+};
+
+void ofono_call_volume_notify(struct ofono_call_volume *cv,
+			const char *property, unsigned char percent);
+DBusMessage *ofono_set_call_volume(DBusMessage *msg,
+			struct ofono_call_volume *cv,
+			const char *property,
+			unsigned char percent);
+
+int ofono_call_volume_driver_register(const struct ofono_call_volume_driver *d);
+void ofono_call_volume_driver_unregister(
+			const struct ofono_call_volume_driver *d);
+
+struct ofono_call_volume *ofono_call_volume_create(struct ofono_modem *modem,
+			unsigned int vendor, const char *driver, void *data);
+
+void ofono_call_volume_register(struct ofono_call_volume *cv);
+void ofono_call_volume_remove(struct ofono_call_volume *cv);
+
+void ofono_call_volume_set_data(struct ofono_call_volume *cv, void *data);
+void *ofono_call_volume_get_data(struct ofono_call_volume *cv);
+
+#ifdef __cplusplus
+}
+#endif
+
+#endif /* __OFONO_CALL_VOLUME_H */
diff --git a/src/call-volume.c b/src/call-volume.c
new file mode 100644
index 0000000..9acee2b
--- /dev/null
+++ b/src/call-volume.c
@@ -0,0 +1,366 @@
+/*
+ *
+ *  oFono - Open Source Telephony
+ *
+ *  Copyright (C) 2008-2009  Intel Corporation. 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 <string.h>
+#include <stdlib.h>
+#include <stdio.h>
+#include <errno.h>
+#include <unistd.h>
+
+#include <glib.h>
+
+#include <ofono/log.h>
+#include <ofono/modem.h>
+#include <ofono/call-volume.h>
+
+#include <gdbus.h>
+#include "ofono.h"
+#include "common.h"
+
+#define CALL_VOLUME_INTERFACE OFONO_SERVICE ".CallVolume"
+
+#define CALL_VOLUME_FLAG_PENDING 0x1
+
+static GSList *g_drivers = NULL;
+
+struct ofono_call_volume {
+	DBusMessage *pending;
+	const struct ofono_call_volume_driver *driver;
+	int flags;
+	void *driver_data;
+	struct ofono_atom *atom;
+
+	unsigned char speaker_volume;
+	unsigned char microphone_volume;
+
+	/* temp volume before it is accepted by remote */
+	unsigned char temp_volume;
+};
+
+void ofono_call_volume_notify(struct ofono_call_volume *cv,
+						const char *property,
+						unsigned char percent)
+{
+	DBusConnection *conn = ofono_dbus_get_connection();
+	const char *path = __ofono_atom_get_path(cv->atom);
+
+	if (!strcmp(property, "SpeakerVolume"))
+		cv->speaker_volume = percent;
+
+	if (!strcmp(property, "MicrophoneVolume"))
+		cv->microphone_volume = percent;
+
+	ofono_dbus_signal_property_changed(conn, path, CALL_VOLUME_INTERFACE,
+					property, DBUS_TYPE_BYTE,
+					&percent);
+}
+
+static DBusMessage *cv_get_properties(DBusConnection *conn,
+						DBusMessage *msg, void *data)
+{
+	struct ofono_call_volume *cv = data;
+	DBusMessage *reply;
+	DBusMessageIter iter, dict;
+
+	reply = dbus_message_new_method_return(msg);
+
+	if (!reply)
+		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, "SpeakerVolume", DBUS_TYPE_BYTE,
+					&cv->speaker_volume);
+
+	ofono_dbus_dict_append(&dict, "MicrophoneVolume",
+					DBUS_TYPE_BYTE, &cv->microphone_volume);
+
+	dbus_message_iter_close_container(&iter, &dict);
+
+	return reply;
+}
+
+static void generic_callback(const struct ofono_error *error, void *data)
+{
+	struct ofono_call_volume *cv = data;
+	DBusConnection *conn = ofono_dbus_get_connection();
+	DBusMessage *reply;
+
+	cv->flags &= ~CALL_VOLUME_FLAG_PENDING;
+
+	if (!cv->pending)
+		return;
+
+	if (error->type == OFONO_ERROR_TYPE_NO_ERROR)
+		reply = dbus_message_new_method_return(cv->pending);
+	else
+		reply = __ofono_error_failed(cv->pending);
+
+	g_dbus_send_message(conn, reply);
+
+	dbus_message_unref(cv->pending);
+	cv->pending = NULL;
+}
+
+static void sv_set_callback(const struct ofono_error *error, void *data)
+{
+	struct ofono_call_volume *cv = data;
+
+	if (error->type != OFONO_ERROR_TYPE_NO_ERROR) {
+		ofono_error("Error occurred during Speaker Volume set: %s",
+					telephony_error_to_str(error));
+	} else {
+		cv->speaker_volume = cv->temp_volume;
+	}
+
+	generic_callback(error, data);
+}
+
+static void mv_set_callback(const struct ofono_error *error, void *data)
+{
+	struct ofono_call_volume *cv = data;
+
+	if (error->type != OFONO_ERROR_TYPE_NO_ERROR) {
+		ofono_error("Error occurred during Microphone Volume set: %s",
+					telephony_error_to_str(error));
+	} else {
+		cv->microphone_volume = cv->temp_volume;
+	}
+
+	generic_callback(error, data);
+}
+
+DBusMessage *ofono_set_call_volume(DBusMessage *msg,
+					struct ofono_call_volume *cv,
+					const char *property,
+					unsigned char percent)
+{
+	if (percent > 100)
+		return __ofono_error_invalid_format(msg);
+
+	DBG("set %s volume to %d percent", property, percent);
+
+	cv->flags |= CALL_VOLUME_FLAG_PENDING;
+	cv->temp_volume = percent;
+
+	if (msg)
+		cv->pending = dbus_message_ref(msg);
+
+	if (!strcmp(property, "SpeakerVolume")) {
+		if (!cv->driver->speaker_volume) {
+			if (msg != NULL)
+				return __ofono_error_not_implemented(msg);
+			else
+				return NULL;
+		}
+		cv->driver->speaker_volume(cv, percent, sv_set_callback, cv);
+	}
+
+	if (!strcmp(property, "MicrophoneVolume")) {
+		if (!cv->driver->microphone_volume) {
+			if (msg != NULL)
+				return __ofono_error_not_implemented(msg);
+			else
+				return NULL;
+		}
+		cv->driver->microphone_volume(cv, percent, mv_set_callback, cv);
+	}
+
+	return NULL;
+}
+
+static DBusMessage *cv_set_property(DBusConnection *conn, DBusMessage *msg,
+					void *data)
+{
+	struct ofono_call_volume *cv = data;
+	DBusMessageIter iter;
+	DBusMessageIter var;
+	const char *property;
+	unsigned char percent;
+
+	if (cv->pending)
+		return __ofono_error_busy(msg);
+
+	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 (dbus_message_iter_get_arg_type(&var) != DBUS_TYPE_BYTE)
+		return __ofono_error_invalid_args(msg);
+
+	dbus_message_iter_get_basic(&var, &percent);
+
+	return ofono_set_call_volume(msg, cv, property, percent);
+}
+
+static GDBusMethodTable cv_methods[] = {
+	{ "GetProperties",	"",	"a{sv}",	cv_get_properties },
+	{ "SetProperty",	"sv",	"",		cv_set_property,
+					G_DBUS_METHOD_FLAG_ASYNC },
+	{ }
+};
+
+static GDBusSignalTable cv_signals[] = {
+	{ "PropertyChanged",	"sv" },
+	{ }
+};
+
+static void call_volume_remove(struct ofono_atom *atom)
+{
+	struct ofono_call_volume *cv = __ofono_atom_get_data(atom);
+
+	DBG("atom: %p", atom);
+
+	if (cv == NULL)
+		return;
+
+	if (cv->driver && cv->driver->remove)
+		cv->driver->remove(cv);
+
+	g_free(cv);
+}
+
+struct ofono_call_volume *ofono_call_volume_create(struct ofono_modem *modem,
+					unsigned int vendor,
+					const char *driver,
+					void *data)
+{
+	struct ofono_call_volume *cv;
+	GSList *l;
+
+	if (driver == NULL)
+		return NULL;
+
+	cv = g_try_new0(struct ofono_call_volume, 1);
+	if (cv == NULL)
+		return NULL;
+
+	/* assume volume as half of maxium volume */
+	cv->speaker_volume = 50;
+	cv->microphone_volume = 50;
+	cv->atom = __ofono_modem_add_atom(modem,
+					OFONO_ATOM_TYPES_CALL_VOLUME,
+					call_volume_remove, cv);
+
+	for (l = g_drivers; l; l = l->next) {
+		const struct ofono_call_volume_driver *drv = l->data;
+
+		if (g_strcmp0(drv->name, driver))
+			continue;
+
+		if (drv->probe(cv, vendor, data) < 0)
+			continue;
+
+		cv->driver = drv;
+		break;
+	}
+
+	return cv;
+}
+
+static void call_volume_unregister(struct ofono_atom *atom)
+{
+	DBusConnection *conn = ofono_dbus_get_connection();
+	struct ofono_call_volume *cv = __ofono_atom_get_data(atom);
+	struct ofono_modem *modem = __ofono_atom_get_modem(atom);
+	const char *path = __ofono_atom_get_path(atom);
+	GSList *l;
+
+	ofono_modem_remove_interface(modem, CALL_VOLUME_INTERFACE);
+	g_dbus_unregister_interface(conn, path,
+					CALL_VOLUME_INTERFACE);
+}
+
+void ofono_call_volume_register(struct ofono_call_volume *cv)
+{
+	DBusConnection *conn = ofono_dbus_get_connection();
+	struct ofono_modem *modem = __ofono_atom_get_modem(cv->atom);
+	const char *path = __ofono_atom_get_path(cv->atom);
+
+	if (!g_dbus_register_interface(conn, path,
+					CALL_VOLUME_INTERFACE,
+					cv_methods, cv_signals, NULL,
+					cv, NULL)) {
+		ofono_error("Could not create %s interface",
+					CALL_VOLUME_INTERFACE);
+
+		return;
+	}
+
+	ofono_modem_add_interface(modem, CALL_VOLUME_INTERFACE);
+
+	__ofono_atom_register(cv->atom, call_volume_unregister);
+}
+
+int ofono_call_volume_driver_register(const struct ofono_call_volume_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_call_volume_driver_unregister(
+				const struct ofono_call_volume_driver *d)
+{
+	DBG("driver: %p, name: %s", d, d->name);
+
+	g_drivers = g_slist_remove(g_drivers, (void *) d);
+}
+
+void ofono_call_volume_remove(struct ofono_call_volume *cv)
+{
+	__ofono_atom_free(cv->atom);
+}
+
+void ofono_call_volume_set_data(struct ofono_call_volume *cv, void *data)
+{
+	cv->driver_data = data;
+}
+
+void *ofono_call_volume_get_data(struct ofono_call_volume *cv)
+{
+	return cv->driver_data;
+}
+
diff --git a/src/ofono.h b/src/ofono.h
index 0659989..409a9e2 100644
--- a/src/ofono.h
+++ b/src/ofono.h
@@ -105,6 +105,7 @@ enum ofono_atom_type {
 	OFONO_ATOM_TYPE_SSN = 12,
 	OFONO_ATOM_TYPE_MESSAGE_WAITING = 13,
 	OFONO_ATOM_TYPE_CBS = 14,
+	OFONO_ATOM_TYPES_CALL_VOLUME = 15,
 };
 
 enum ofono_atom_watch_condition {
-- 
1.6.1.3

Re: [PATCH] Add call-volume interface to allow user adjust speaker and mic volume

[Denis Kenzior]

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

Hi Zhenhua,

Your patch has been applied.  I refactored much of it afterwards :)  Let me 
know if I have broken something.

Regards,
-Denis