* [PATCH 0/6] Driver registration rework
@ 2018-10-26 9:54 Jonas Bonn
2018-10-26 9:54 ` [PATCH 1/6] include: add macros for declaring drivers Jonas Bonn
` (6 more replies)
0 siblings, 7 replies; 10+ messages in thread
From: Jonas Bonn @ 2018-10-26 9:54 UTC (permalink / raw)
To: ofono
[-- Attachment #1: Type: text/plain, Size: 3385 bytes --]
Hi Denis,
Since I don't know if you'd even be interested in a series like this,
here's a draft that touches just a few places to give you an idea of
where this would take us... take a look and let me know what you think.
/Jonas
******
ofono has historically had a system in place whereby modems are
encapsulated as 'plugins', the idea being that these could be loosely
coupled to the core and added/removed flexibly as requirements changed.
Theoretically, the plugin would register a set of new drivers (atoms)
with the core and then call ofono_modem_register(...) after having
configured the modem object appropriately (device node, etc). In
practice, however, this does not work well and the modem plugins don't
really follow this model.
- drivers (atoms) use a lot of core functionality and cannot be built
externally using only the exported ofono headers. As such, the
drivers really need to be part of core ofono, irregardless of whether
they are packed into separate modules (plugins) or linked into one
monolithici mage
- ofono no longer provides the ability to built the plugins in the
source tree as external libs; they are all linked into one monolithic
image by default with no option to change this
- the modem 'plugins' are 99% modem driver and 1% plugin; the plugin bit
just takes care of registering the modem driver with the core and does
nothing else
- udevng takes care of setting up the modem objects and binding them
with their drivers
This patch series tries to addres this in the following way:
i) Add driver registration macros that lift registration with the core
into init() (constructor) functions; this allows the drivers to
automatically be registered at startup or on module load if they happen
be part of a plugin
ii) Move the 'modem driver' part of the modem plugins into the drivers/
directory.
iii) If the plugins are doing nothing other than registering the modem
driver, drop it altogether.
Jonas Bonn (6):
include: add macros for declaring drivers
qmimodem: simplify driver registration
gobi: modem driver cannot be a plugin
atmodem: add missing headers to atutil.h
telitmodem: simplify driver registration
telitmodem: modem driver is not a plugin
Makefile.am | 12 ++----
drivers/atmodem/atutil.h | 3 ++
{plugins => drivers/qmimodem}/gobi.c | 13 +------
drivers/qmimodem/gprs-context.c | 10 +----
drivers/qmimodem/location-reporting.c | 10 +----
drivers/qmimodem/qmimodem.c | 4 --
drivers/qmimodem/qmimodem.h | 6 ---
drivers/telitmodem/gprs-context-ncm.c | 15 +-------
drivers/telitmodem/location-reporting.c | 16 ++------
{plugins => drivers/telitmodem}/telit.c | 15 +-------
drivers/telitmodem/telitmodem.c | 51 -------------------------
drivers/telitmodem/telitmodem.h | 27 -------------
include/gprs-context.h | 11 ++++++
include/location-reporting.h | 10 +++++
include/modem.h | 11 ++++++
15 files changed, 47 insertions(+), 167 deletions(-)
rename {plugins => drivers/qmimodem}/gobi.c (97%)
rename {plugins => drivers/telitmodem}/telit.c (97%)
delete mode 100644 drivers/telitmodem/telitmodem.c
delete mode 100644 drivers/telitmodem/telitmodem.h
--
2.17.1
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 1/6] include: add macros for declaring drivers
2018-10-26 9:54 [PATCH 0/6] Driver registration rework Jonas Bonn
@ 2018-10-26 9:54 ` Jonas Bonn
2018-10-26 9:54 ` [PATCH 2/6] qmimodem: simplify driver registration Jonas Bonn
` (5 subsequent siblings)
6 siblings, 0 replies; 10+ messages in thread
From: Jonas Bonn @ 2018-10-26 9:54 UTC (permalink / raw)
To: ofono
[-- Attachment #1: Type: text/plain, Size: 2236 bytes --]
---
include/gprs-context.h | 11 +++++++++++
include/location-reporting.h | 10 ++++++++++
include/modem.h | 11 +++++++++++
3 files changed, 32 insertions(+)
diff --git a/include/gprs-context.h b/include/gprs-context.h
index 965cefc2..d7576bf5 100644
--- a/include/gprs-context.h
+++ b/include/gprs-context.h
@@ -115,6 +115,17 @@ void ofono_gprs_context_set_ipv6_gateway(struct ofono_gprs_context *gc,
const char *gateway);
void ofono_gprs_context_set_ipv6_dns_servers(struct ofono_gprs_context *gc,
const char **dns);
+
+#define OFONO_GPRS_CONTEXT_DRIVER(driver) \
+static void __attribute((constructor(102))) init() \
+{ \
+ ofono_gprs_context_driver_register(driver); \
+} \
+static void __attribute((destructor(102))) fini() \
+{ \
+ ofono_gprs_context_driver_unregister(driver); \
+}
+
#ifdef __cplusplus
}
#endif
diff --git a/include/location-reporting.h b/include/location-reporting.h
index 0717f710..a639a2b4 100644
--- a/include/location-reporting.h
+++ b/include/location-reporting.h
@@ -74,6 +74,16 @@ void *ofono_location_reporting_get_data(struct ofono_location_reporting *lr);
struct ofono_modem *ofono_location_reporting_get_modem(
struct ofono_location_reporting *lr);
+#define OFONO_LOCATION_REPORTING_DRIVER(driver) \
+static void __attribute((constructor(102))) init() \
+{ \
+ ofono_location_reporting_driver_register(driver); \
+} \
+static void __attribute((destructor(102))) fini() \
+{ \
+ ofono_location_reporting_driver_unregister(driver); \
+}
+
#ifdef __cplusplus
}
#endif
diff --git a/include/modem.h b/include/modem.h
index 9b12cfad..decd1297 100644
--- a/include/modem.h
+++ b/include/modem.h
@@ -130,6 +130,17 @@ struct ofono_modem *ofono_modem_find(ofono_modem_compare_cb_t func,
void ofono_modem_set_powered_timeout_hint(struct ofono_modem *modem,
unsigned int seconds);
+#define OFONO_MODEM_DRIVER(driver) \
+static void __attribute((constructor(102))) init() \
+{ \
+ ofono_modem_driver_register(driver); \
+} \
+static void __attribute((destructor(102))) fini() \
+{ \
+ ofono_modem_driver_unregister(driver); \
+}
+
+
#ifdef __cplusplus
}
#endif
--
2.17.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 2/6] qmimodem: simplify driver registration
2018-10-26 9:54 [PATCH 0/6] Driver registration rework Jonas Bonn
2018-10-26 9:54 ` [PATCH 1/6] include: add macros for declaring drivers Jonas Bonn
@ 2018-10-26 9:54 ` Jonas Bonn
2018-10-26 9:54 ` [PATCH 3/6] gobi: modem driver cannot be a plugin Jonas Bonn
` (4 subsequent siblings)
6 siblings, 0 replies; 10+ messages in thread
From: Jonas Bonn @ 2018-10-26 9:54 UTC (permalink / raw)
To: ofono
[-- Attachment #1: Type: text/plain, Size: 2891 bytes --]
---
drivers/qmimodem/gprs-context.c | 10 +---------
drivers/qmimodem/location-reporting.c | 10 +---------
drivers/qmimodem/qmimodem.c | 4 ----
drivers/qmimodem/qmimodem.h | 6 ------
4 files changed, 2 insertions(+), 28 deletions(-)
diff --git a/drivers/qmimodem/gprs-context.c b/drivers/qmimodem/gprs-context.c
index dcdf8ae1..0dc33b0f 100644
--- a/drivers/qmimodem/gprs-context.c
+++ b/drivers/qmimodem/gprs-context.c
@@ -501,12 +501,4 @@ static const struct ofono_gprs_context_driver driver = {
.detach_shutdown = qmi_gprs_context_detach_shutdown,
};
-void qmi_gprs_context_init(void)
-{
- ofono_gprs_context_driver_register(&driver);
-}
-
-void qmi_gprs_context_exit(void)
-{
- ofono_gprs_context_driver_unregister(&driver);
-}
+OFONO_GPRS_CONTEXT_DRIVER(&driver)
diff --git a/drivers/qmimodem/location-reporting.c b/drivers/qmimodem/location-reporting.c
index 90d57db7..2052b460 100644
--- a/drivers/qmimodem/location-reporting.c
+++ b/drivers/qmimodem/location-reporting.c
@@ -287,12 +287,4 @@ static const struct ofono_location_reporting_driver driver = {
.disable = qmi_location_reporting_disable,
};
-void qmi_location_reporting_init()
-{
- ofono_location_reporting_driver_register(&driver);
-}
-
-void qmi_location_reporting_exit()
-{
- ofono_location_reporting_driver_unregister(&driver);
-}
+OFONO_LOCATION_REPORTING_DRIVER(&driver)
diff --git a/drivers/qmimodem/qmimodem.c b/drivers/qmimodem/qmimodem.c
index 11e68f2e..5f1a267e 100644
--- a/drivers/qmimodem/qmimodem.c
+++ b/drivers/qmimodem/qmimodem.c
@@ -38,10 +38,8 @@ static int qmimodem_init(void)
qmi_sms_init();
qmi_ussd_init();
qmi_gprs_init();
- qmi_gprs_context_init();
qmi_lte_init();
qmi_radio_settings_init();
- qmi_location_reporting_init();
qmi_netmon_init();
return 0;
@@ -50,10 +48,8 @@ static int qmimodem_init(void)
static void qmimodem_exit(void)
{
qmi_netmon_exit();
- qmi_location_reporting_exit();
qmi_radio_settings_exit();
qmi_lte_exit();
- qmi_gprs_context_exit();
qmi_gprs_exit();
qmi_ussd_exit();
qmi_sms_exit();
diff --git a/drivers/qmimodem/qmimodem.h b/drivers/qmimodem/qmimodem.h
index eeb1375a..72e5417d 100644
--- a/drivers/qmimodem/qmimodem.h
+++ b/drivers/qmimodem/qmimodem.h
@@ -45,17 +45,11 @@ extern void qmi_ussd_exit(void);
extern void qmi_gprs_init(void);
extern void qmi_gprs_exit(void);
-extern void qmi_gprs_context_init(void);
-extern void qmi_gprs_context_exit(void);
-
extern void qmi_lte_init(void);
extern void qmi_lte_exit(void);
extern void qmi_radio_settings_init(void);
extern void qmi_radio_settings_exit(void);
-extern void qmi_location_reporting_init(void);
-extern void qmi_location_reporting_exit(void);
-
extern void qmi_netmon_init(void);
extern void qmi_netmon_exit(void);
--
2.17.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 3/6] gobi: modem driver cannot be a plugin
2018-10-26 9:54 [PATCH 0/6] Driver registration rework Jonas Bonn
2018-10-26 9:54 ` [PATCH 1/6] include: add macros for declaring drivers Jonas Bonn
2018-10-26 9:54 ` [PATCH 2/6] qmimodem: simplify driver registration Jonas Bonn
@ 2018-10-26 9:54 ` Jonas Bonn
2018-10-26 9:54 ` [PATCH 4/6] atmodem: add missing headers to atutil.h Jonas Bonn
` (3 subsequent siblings)
6 siblings, 0 replies; 10+ messages in thread
From: Jonas Bonn @ 2018-10-26 9:54 UTC (permalink / raw)
To: ofono
[-- Attachment #1: Type: text/plain, Size: 1762 bytes --]
This patch converts the gobi driver from being a 'plugin' to being a
regular 'modem driver'.
Modem drivers, in general, use a lot of internal ofono functionality and
cannot be built standalone against just the exported ofono headers. As
such, these cannot be built as plugins and therefore should netiher be part
of the plugins directory nor declared as plugins.
---
Makefile.am | 5 ++---
{plugins => drivers/qmimodem}/gobi.c | 13 +------------
2 files changed, 3 insertions(+), 15 deletions(-)
rename {plugins => drivers/qmimodem}/gobi.c (97%)
diff --git a/Makefile.am b/Makefile.am
index e8e4ed95..f7bcb4cd 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -237,10 +237,9 @@ builtin_sources += $(qmi_sources) \
drivers/qmimodem/lte.c \
drivers/qmimodem/radio-settings.c \
drivers/qmimodem/location-reporting.c \
- drivers/qmimodem/netmon.c
+ drivers/qmimodem/netmon.c \
+ drivers/qmimodem/gobi.c
-builtin_modules += gobi
-builtin_sources += plugins/gobi.c
endif
if ATMODEM
diff --git a/plugins/gobi.c b/drivers/qmimodem/gobi.c
similarity index 97%
rename from plugins/gobi.c
rename to drivers/qmimodem/gobi.c
index d5a5965e..69a682a8 100644
--- a/plugins/gobi.c
+++ b/drivers/qmimodem/gobi.c
@@ -529,15 +529,4 @@ static const struct ofono_modem_driver gobi_driver = {
.post_online = gobi_post_online,
};
-static int gobi_init(void)
-{
- return ofono_modem_driver_register(&gobi_driver);
-}
-
-static void gobi_exit(void)
-{
- ofono_modem_driver_unregister(&gobi_driver);
-}
-
-OFONO_PLUGIN_DEFINE(gobi, "Qualcomm Gobi modem driver", VERSION,
- OFONO_PLUGIN_PRIORITY_DEFAULT, gobi_init, gobi_exit)
+OFONO_MODEM_DRIVER(&gobi_driver)
--
2.17.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 4/6] atmodem: add missing headers to atutil.h
2018-10-26 9:54 [PATCH 0/6] Driver registration rework Jonas Bonn
` (2 preceding siblings ...)
2018-10-26 9:54 ` [PATCH 3/6] gobi: modem driver cannot be a plugin Jonas Bonn
@ 2018-10-26 9:54 ` Jonas Bonn
2018-10-26 9:54 ` [PATCH 5/6] telitmodem: simplify driver registration Jonas Bonn
` (2 subsequent siblings)
6 siblings, 0 replies; 10+ messages in thread
From: Jonas Bonn @ 2018-10-26 9:54 UTC (permalink / raw)
To: ofono
[-- Attachment #1: Type: text/plain, Size: 423 bytes --]
---
drivers/atmodem/atutil.h | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/atmodem/atutil.h b/drivers/atmodem/atutil.h
index f1389a94..d74c33d2 100644
--- a/drivers/atmodem/atutil.h
+++ b/drivers/atmodem/atutil.h
@@ -20,6 +20,9 @@
*
*/
+#include "gatchat.h"
+#include "gatresult.h"
+
enum at_util_sms_store {
AT_UTIL_SMS_STORE_SM = 0,
AT_UTIL_SMS_STORE_ME = 1,
--
2.17.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 5/6] telitmodem: simplify driver registration
2018-10-26 9:54 [PATCH 0/6] Driver registration rework Jonas Bonn
` (3 preceding siblings ...)
2018-10-26 9:54 ` [PATCH 4/6] atmodem: add missing headers to atutil.h Jonas Bonn
@ 2018-10-26 9:54 ` Jonas Bonn
2018-10-26 9:54 ` [PATCH 6/6] telitmodem: modem driver is not a plugin Jonas Bonn
2018-10-29 20:26 ` [PATCH 0/6] Driver registration rework Denis Kenzior
6 siblings, 0 replies; 10+ messages in thread
From: Jonas Bonn @ 2018-10-26 9:54 UTC (permalink / raw)
To: ofono
[-- Attachment #1: Type: text/plain, Size: 5873 bytes --]
Using the driver registration macros saves us having to declare a plugin
just to have the drivers registered.
---
Makefile.am | 3 --
drivers/telitmodem/gprs-context-ncm.c | 15 +-------
drivers/telitmodem/location-reporting.c | 16 ++------
drivers/telitmodem/telitmodem.c | 51 -------------------------
drivers/telitmodem/telitmodem.h | 27 -------------
5 files changed, 5 insertions(+), 107 deletions(-)
delete mode 100644 drivers/telitmodem/telitmodem.c
delete mode 100644 drivers/telitmodem/telitmodem.h
diff --git a/Makefile.am b/Makefile.am
index f7bcb4cd..49b6cbb7 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -320,10 +320,7 @@ builtin_sources += drivers/atmodem/atutil.h \
drivers/mbmmodem/stk.c \
drivers/mbmmodem/location-reporting.c
-builtin_modules += telitmodem
builtin_sources += drivers/atmodem/atutil.h \
- drivers/telitmodem/telitmodem.h \
- drivers/telitmodem/telitmodem.c \
drivers/telitmodem/location-reporting.c \
drivers/telitmodem/gprs-context-ncm.c
diff --git a/drivers/telitmodem/gprs-context-ncm.c b/drivers/telitmodem/gprs-context-ncm.c
index c4e60e21..63146605 100644
--- a/drivers/telitmodem/gprs-context-ncm.c
+++ b/drivers/telitmodem/gprs-context-ncm.c
@@ -31,10 +31,7 @@
#include <ofono/modem.h>
#include <ofono/gprs-context.h>
-#include "gatchat.h"
-#include "gatresult.h"
-
-#include "telitmodem.h"
+#include <drivers/atmodem/atutil.h>
static const char *none_prefix[] = { NULL };
static const char *cgpaddr_prefix[] = { "+CGPADDR:", NULL };
@@ -476,12 +473,4 @@ static const struct ofono_gprs_context_driver driver = {
.deactivate_primary = telitncm_gprs_deactivate_primary,
};
-void telitncm_gprs_context_init(void)
-{
- ofono_gprs_context_driver_register(&driver);
-}
-
-void telitncm_gprs_context_exit(void)
-{
- ofono_gprs_context_driver_unregister(&driver);
-}
+OFONO_GPRS_CONTEXT_DRIVER(&driver)
diff --git a/drivers/telitmodem/location-reporting.c b/drivers/telitmodem/location-reporting.c
index 245c29c2..638a8760 100644
--- a/drivers/telitmodem/location-reporting.c
+++ b/drivers/telitmodem/location-reporting.c
@@ -35,11 +35,9 @@
#include <ofono/modem.h>
#include <ofono/location-reporting.h>
-#include "gatchat.h"
-#include "gatresult.h"
-#include "gattty.h"
+#include <drivers/atmodem/atutil.h>
-#include "telitmodem.h"
+#include "gattty.h"
static const char *none_prefix[] = { NULL };
static const char *portcfg_prefix[] = { "#PORTCFG:", NULL };
@@ -303,12 +301,4 @@ static const struct ofono_location_reporting_driver driver = {
.disable = telit_location_reporting_disable,
};
-void telit_location_reporting_init()
-{
- ofono_location_reporting_driver_register(&driver);
-}
-
-void telit_location_reporting_exit()
-{
- ofono_location_reporting_driver_unregister(&driver);
-}
+OFONO_LOCATION_REPORTING_DRIVER(&driver)
diff --git a/drivers/telitmodem/telitmodem.c b/drivers/telitmodem/telitmodem.c
deleted file mode 100644
index 4aa2c444..00000000
--- a/drivers/telitmodem/telitmodem.c
+++ /dev/null
@@ -1,51 +0,0 @@
-/*
- *
- * oFono - Open Source Telephony
- *
- * Copyright (C) 2008-2011 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
-
-#include <glib.h>
-#include <gatchat.h>
-
-#define OFONO_API_SUBJECT_TO_CHANGE
-#include <ofono/plugin.h>
-#include <ofono/types.h>
-
-#include "telitmodem.h"
-
-static int telitmodem_init(void)
-{
- telit_location_reporting_init();
- telitncm_gprs_context_init();
-
- return 0;
-}
-
-static void telitmodem_exit(void)
-{
- telit_location_reporting_exit();
- telitncm_gprs_context_exit();
-}
-
-OFONO_PLUGIN_DEFINE(telitmodem, "Telit modem driver", VERSION,
- OFONO_PLUGIN_PRIORITY_DEFAULT,
- telitmodem_init, telitmodem_exit)
diff --git a/drivers/telitmodem/telitmodem.h b/drivers/telitmodem/telitmodem.h
deleted file mode 100644
index 8a14595a..00000000
--- a/drivers/telitmodem/telitmodem.h
+++ /dev/null
@@ -1,27 +0,0 @@
-/*
- *
- * oFono - Open Source Telephony
- *
- * Copyright (C) 2008-2011 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
- *
- */
-
-#include <drivers/atmodem/atutil.h>
-
-extern void telit_location_reporting_init();
-extern void telit_location_reporting_exit();
-extern void telitncm_gprs_context_init();
-extern void telitncm_gprs_context_exit();
--
2.17.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 6/6] telitmodem: modem driver is not a plugin
2018-10-26 9:54 [PATCH 0/6] Driver registration rework Jonas Bonn
` (4 preceding siblings ...)
2018-10-26 9:54 ` [PATCH 5/6] telitmodem: simplify driver registration Jonas Bonn
@ 2018-10-26 9:54 ` Jonas Bonn
2018-10-29 20:26 ` [PATCH 0/6] Driver registration rework Denis Kenzior
6 siblings, 0 replies; 10+ messages in thread
From: Jonas Bonn @ 2018-10-26 9:54 UTC (permalink / raw)
To: ofono
[-- Attachment #1: Type: text/plain, Size: 2094 bytes --]
The 'modem driver' part of this file uses a lot of internal ofono
functionality and therefore belongs in drivers/, not plugins/.
By using the driver registration macros, the driver is registered on
load, whether it be by way of a plugin or by nature of being part of a
monolithic ofono biuld. As such, the plugin doesn't need to explicity
register the driver; that leaves this plugin with nothing to do so the
plugin declaration can be dropped altogether.
---
Makefile.am | 4 +---
{plugins => drivers/telitmodem}/telit.c | 15 +--------------
2 files changed, 2 insertions(+), 17 deletions(-)
rename {plugins => drivers/telitmodem}/telit.c (97%)
diff --git a/Makefile.am b/Makefile.am
index 49b6cbb7..76d1769a 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -321,6 +321,7 @@ builtin_sources += drivers/atmodem/atutil.h \
drivers/mbmmodem/location-reporting.c
builtin_sources += drivers/atmodem/atutil.h \
+ drivers/telitmodem/telit.c \
drivers/telitmodem/location-reporting.c \
drivers/telitmodem/gprs-context-ncm.c
@@ -503,9 +504,6 @@ builtin_sources += plugins/sim7100.c
builtin_modules += connman
builtin_sources += plugins/connman.c
-builtin_modules += telit
-builtin_sources += plugins/telit.c
-
builtin_modules += quectel
builtin_sources += plugins/quectel.c
diff --git a/plugins/telit.c b/drivers/telitmodem/telit.c
similarity index 97%
rename from plugins/telit.c
rename to drivers/telitmodem/telit.c
index 5b17aee5..0a394bdf 100644
--- a/plugins/telit.c
+++ b/drivers/telitmodem/telit.c
@@ -549,17 +549,4 @@ static const struct ofono_modem_driver telit_driver = {
.post_online = telit_post_online,
};
-static int telit_init(void)
-{
- DBG("");
-
- return ofono_modem_driver_register(&telit_driver);
-}
-
-static void telit_exit(void)
-{
- ofono_modem_driver_unregister(&telit_driver);
-}
-
-OFONO_PLUGIN_DEFINE(telit, "Telit driver", VERSION,
- OFONO_PLUGIN_PRIORITY_DEFAULT, telit_init, telit_exit)
+OFONO_MODEM_DRIVER(&telit_driver)
--
2.17.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 0/6] Driver registration rework
2018-10-26 9:54 [PATCH 0/6] Driver registration rework Jonas Bonn
` (5 preceding siblings ...)
2018-10-26 9:54 ` [PATCH 6/6] telitmodem: modem driver is not a plugin Jonas Bonn
@ 2018-10-29 20:26 ` Denis Kenzior
2018-10-30 13:07 ` Jonas Bonn
6 siblings, 1 reply; 10+ messages in thread
From: Denis Kenzior @ 2018-10-29 20:26 UTC (permalink / raw)
To: ofono
[-- Attachment #1: Type: text/plain, Size: 4875 bytes --]
Hi Jonas,
On 10/26/2018 04:54 AM, Jonas Bonn wrote:
> Hi Denis,
>
> Since I don't know if you'd even be interested in a series like this,
> here's a draft that touches just a few places to give you an idea of
> where this would take us... take a look and let me know what you think.
>
> /Jonas
>
> ******
>
> ofono has historically had a system in place whereby modems are
> encapsulated as 'plugins', the idea being that these could be loosely
> coupled to the core and added/removed flexibly as requirements changed.
> Theoretically, the plugin would register a set of new drivers (atoms)
> with the core and then call ofono_modem_register(...) after having
> configured the modem object appropriately (device node, etc). In
> practice, however, this does not work well and the modem plugins don't
> really follow this model.
Yeah it is a bit of a historical weirdness. We put the atom drivers
into drivers/* and the modem drivers went into plugins/ along with
actual plugins. Maybe we should fix that...
>
> - drivers (atoms) use a lot of core functionality and cannot be built
> externally using only the exported ofono headers. As such, the
> drivers really need to be part of core ofono, irregardless of whether
> they are packed into separate modules (plugins) or linked into one
> monolithici mage
Sort of. They're still drivers and can be removed at will from the
core. The only thing that having them in tree helps with is access to
some private headers. And even these are not really that numerous.
>
> - ofono no longer provides the ability to built the plugins in the
> source tree as external libs; they are all linked into one monolithic
> image by default with no option to change this
You can in theory do this, but not for in-tree plugins/drivers. What
would be the point anyway?
>
> - the modem 'plugins' are 99% modem driver and 1% plugin; the plugin bit
> just takes care of registering the modem driver with the core and does
> nothing else
True. As I said, the modem drivers should probably be in a separate
place. I was never fully happy with how this got setup, but then this
isn't that big of an issue in my mind.
>
> - udevng takes care of setting up the modem objects and binding them
> with their drivers
>
> This patch series tries to addres this in the following way:
>
> i) Add driver registration macros that lift registration with the core
> into init() (constructor) functions; this allows the drivers to
> automatically be registered at startup or on module load if they happen
> be part of a plugin
Except this breaks our plugin whitelist / blacklist logic. And also
makes it impossible to control the init / exit sequence which might be
useful to have.
If you really want to do this, then maybe we need to use the 'section()'
magic for this. Look into e.g. how iwd registers eap methods.
Still a question of how we handle atom driver registration...
>
> ii) Move the 'modem driver' part of the modem plugins into the drivers/
> directory.
I'm not sure drivers/ is really the right place. At least it is weird
to me that gobi would live in drivers/qmimodem/. Perhaps we need a
place for modem drivers only.
>
> iii) If the plugins are doing nothing other than registering the modem
> driver, drop it altogether.
>
> Jonas Bonn (6):
> include: add macros for declaring drivers
> qmimodem: simplify driver registration
> gobi: modem driver cannot be a plugin
> atmodem: add missing headers to atutil.h
> telitmodem: simplify driver registration
> telitmodem: modem driver is not a plugin
>
> Makefile.am | 12 ++----
> drivers/atmodem/atutil.h | 3 ++
> {plugins => drivers/qmimodem}/gobi.c | 13 +------
> drivers/qmimodem/gprs-context.c | 10 +----
> drivers/qmimodem/location-reporting.c | 10 +----
> drivers/qmimodem/qmimodem.c | 4 --
> drivers/qmimodem/qmimodem.h | 6 ---
> drivers/telitmodem/gprs-context-ncm.c | 15 +-------
> drivers/telitmodem/location-reporting.c | 16 ++------
> {plugins => drivers/telitmodem}/telit.c | 15 +-------
> drivers/telitmodem/telitmodem.c | 51 -------------------------
> drivers/telitmodem/telitmodem.h | 27 -------------
> include/gprs-context.h | 11 ++++++
> include/location-reporting.h | 10 +++++
> include/modem.h | 11 ++++++
> 15 files changed, 47 insertions(+), 167 deletions(-)
> rename {plugins => drivers/qmimodem}/gobi.c (97%)
> rename {plugins => drivers/telitmodem}/telit.c (97%)
> delete mode 100644 drivers/telitmodem/telitmodem.c
> delete mode 100644 drivers/telitmodem/telitmodem.h
>
Regards,
-Denis
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 0/6] Driver registration rework
2018-10-29 20:26 ` [PATCH 0/6] Driver registration rework Denis Kenzior
@ 2018-10-30 13:07 ` Jonas Bonn
2018-10-30 14:42 ` Denis Kenzior
0 siblings, 1 reply; 10+ messages in thread
From: Jonas Bonn @ 2018-10-30 13:07 UTC (permalink / raw)
To: ofono
[-- Attachment #1: Type: text/plain, Size: 6823 bytes --]
Hi Denis,
On 29/10/2018 21:26, Denis Kenzior wrote:
> Hi Jonas,
>
> On 10/26/2018 04:54 AM, Jonas Bonn wrote:
>> Hi Denis,
>>
>> Since I don't know if you'd even be interested in a series like this,
>> here's a draft that touches just a few places to give you an idea of
>> where this would take us... take a look and let me know what you think.
>>
>> /Jonas
>>
>> ******
>>
>> ofono has historically had a system in place whereby modems are
>> encapsulated as 'plugins', the idea being that these could be loosely
>> coupled to the core and added/removed flexibly as requirements changed.
>> Theoretically, the plugin would register a set of new drivers (atoms)
>> with the core and then call ofono_modem_register(...) after having
>> configured the modem object appropriately (device node, etc). In
>> practice, however, this does not work well and the modem plugins don't
>> really follow this model.
>
> Yeah it is a bit of a historical weirdness. We put the atom drivers
> into drivers/* and the modem drivers went into plugins/ along with
> actual plugins. Maybe we should fix that...
>
>>
>> - drivers (atoms) use a lot of core functionality and cannot be built
>> externally using only the exported ofono headers. As such, the
>> drivers really need to be part of core ofono, irregardless of whether
>> they are packed into separate modules (plugins) or linked into one
>> monolithici mage
>
> Sort of. They're still drivers and can be removed at will from the
> core. The only thing that having them in tree helps with is access to
> some private headers. And even these are not really that numerous.
>
Essentially all the drivers send AT commands, QMI commands, etc. which
involves using core code. These drivers cannot be built externally from
ofono. Or am I misunderstanding what's being exported from ofono somehow?
>>
>> - ofono no longer provides the ability to built the plugins in the
>> source tree as external libs; they are all linked into one monolithic
>> image by default with no option to change this
>
> You can in theory do this, but not for in-tree plugins/drivers. What
> would be the point anyway?
Right. I was just stating a fact... ;)
>
>>
>> - the modem 'plugins' are 99% modem driver and 1% plugin; the plugin bit
>> just takes care of registering the modem driver with the core and does
>> nothing else
>
> True. As I said, the modem drivers should probably be in a separate
> place. I was never fully happy with how this got setup, but then this
> isn't that big of an issue in my mind.
>
>>
>> - udevng takes care of setting up the modem objects and binding them
>> with their drivers
>>
>> This patch series tries to addres this in the following way:
>>
>> i) Add driver registration macros that lift registration with the core
>> into init() (constructor) functions; this allows the drivers to
>> automatically be registered at startup or on module load if they happen
>> be part of a plugin
>
> Except this breaks our plugin whitelist / blacklist logic. And also
> makes it impossible to control the init / exit sequence which might be
> useful to have.
The drivers are independent from each other, as things currently stand,
so the init/exit sequence isn't important (as far as I can tell). Were
it to become important, that's manageable... for the attribute solution,
the priority can be tweaked if ordering is necessary.
As for the blacklisting: the plugins currently only register the
drivers and do nothing else. The cost of registering a driver is low so
there's really no gain in blacklisting these plugins. (Note that this
concerns these 'modem driver' plugins only... there are some "real"
plugins, too... mbpi, udev, etc. that belong in plugins/ and where
blacklisting might make sense).
>
> If you really want to do this, then maybe we need to use the 'section()'
> magic for this. Look into e.g. how iwd registers eap methods.
Yes, that was the next step. I didn't want to rush there, though, given
the (sometimes) conservative nature of the project. :)
>
> Still a question of how we handle atom driver registration...
Not sure what you are saying here. Are you referring to the section
magic here? If so, then the sections get mapped to symbols identifying
the start and end of the driver array and this replaces the current
g_driver_list mechanism currently in use.
>
>>
>> ii) Move the 'modem driver' part of the modem plugins into the drivers/
>> directory.
>
> I'm not sure drivers/ is really the right place. At least it is weird
> to me that gobi would live in drivers/qmimodem/. Perhaps we need a
> place for modem drivers only.
drivers/ feels right, though I agree drivers/qmimodem might not be quite
right. Point is, it's not a plugin so to at least get it out of plugins/.
Is it worth my continuing with this?
/Jonas
>
>>
>> iii) If the plugins are doing nothing other than registering the modem
>> driver, drop it altogether.
>>
>> Jonas Bonn (6):
>> include: add macros for declaring drivers
>> qmimodem: simplify driver registration
>> gobi: modem driver cannot be a plugin
>> atmodem: add missing headers to atutil.h
>> telitmodem: simplify driver registration
>> telitmodem: modem driver is not a plugin
>>
>> Makefile.am | 12 ++----
>> drivers/atmodem/atutil.h | 3 ++
>> {plugins => drivers/qmimodem}/gobi.c | 13 +------
>> drivers/qmimodem/gprs-context.c | 10 +----
>> drivers/qmimodem/location-reporting.c | 10 +----
>> drivers/qmimodem/qmimodem.c | 4 --
>> drivers/qmimodem/qmimodem.h | 6 ---
>> drivers/telitmodem/gprs-context-ncm.c | 15 +-------
>> drivers/telitmodem/location-reporting.c | 16 ++------
>> {plugins => drivers/telitmodem}/telit.c | 15 +-------
>> drivers/telitmodem/telitmodem.c | 51 -------------------------
>> drivers/telitmodem/telitmodem.h | 27 -------------
>> include/gprs-context.h | 11 ++++++
>> include/location-reporting.h | 10 +++++
>> include/modem.h | 11 ++++++
>> 15 files changed, 47 insertions(+), 167 deletions(-)
>> rename {plugins => drivers/qmimodem}/gobi.c (97%)
>> rename {plugins => drivers/telitmodem}/telit.c (97%)
>> delete mode 100644 drivers/telitmodem/telitmodem.c
>> delete mode 100644 drivers/telitmodem/telitmodem.h
>>
>
> Regards,
> -Denis
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 0/6] Driver registration rework
2018-10-30 13:07 ` Jonas Bonn
@ 2018-10-30 14:42 ` Denis Kenzior
0 siblings, 0 replies; 10+ messages in thread
From: Denis Kenzior @ 2018-10-30 14:42 UTC (permalink / raw)
To: ofono
[-- Attachment #1: Type: text/plain, Size: 4742 bytes --]
Hi Jonas,
>> Sort of. They're still drivers and can be removed at will from the
>> core. The only thing that having them in tree helps with is access to
>> some private headers. And even these are not really that numerous.
>>
>
> Essentially all the drivers send AT commands, QMI commands, etc. which
> involves using core code. These drivers cannot be built externally from
> ofono. Or am I misunderstanding what's being exported from ofono somehow?
>
I'm not sure I'm understanding you correctly. So from my Point of View
(POV):
- Core is the core. You can't take anything away from it. The driver
can be removed at configure time or even run time and nobody would know
or care if their hardware was not reliant on that driver.
- There is no fundamental difference between an out-of-tree driver and a
builtin. There really isn't. There are some conveniences that the
builtin drivers enjoy, but these are easily replaced if you live out of
tree. So if you really cared, yes you could take the qmi driver and
build it out of tree no problem. They're builtins for ease of maintenance.
- Drivers are not actually using any core code. They use GAtChat, QMI,
MBIM, etc. But I don't consider that core code, its just helpers.
So to sum it up: I still don't know what you're saying above, but there
is a clear demarcation line between what is core code and what is a
driver.
>>> i) Add driver registration macros that lift registration with the core
>>> into init() (constructor) functions; this allows the drivers to
>>> automatically be registered at startup or on module load if they happen
>>> be part of a plugin
>>
>> Except this breaks our plugin whitelist / blacklist logic. And also
>> makes it impossible to control the init / exit sequence which might be
>> useful to have.
>
> The drivers are independent from each other, as things currently stand,
> so the init/exit sequence isn't important (as far as I can tell). Were
> it to become important, that's manageable... for the attribute solution,
> the priority can be tweaked if ordering is necessary.
Well it does allow you to turn off the atmodem and all related drivers
completely for example. Whether that is useful for atom drivers is
somewhat arguable. I don't remember using this capability, so if no one
else objects I'm fine with converting atom drivers into a new world
order. But then this only buys you about 7-8 lines of savings per atom
driver, so not really a big deal.
>
> As for the blacklisting: the plugins currently only register the
> drivers and do nothing else. The cost of registering a driver is low so
> there's really no gain in blacklisting these plugins. (Note that this
> concerns these 'modem driver' plugins only... there are some "real"
> plugins, too... mbpi, udev, etc. that belong in plugins/ and where
> blacklisting might make sense).
This is where I disagree. We blacklist / whitelist individual modem
drivers all the time for debug purposes. So at the very least you need
to preserve that ability. The section() magic I mentioned earlier would
allow you to do that.
>
>>
>> If you really want to do this, then maybe we need to use the
>> 'section()' magic for this. Look into e.g. how iwd registers eap
>> methods.
>
> Yes, that was the next step. I didn't want to rush there, though, given
> the (sometimes) conservative nature of the project. :)
>
So I'm okay with the general idea, I might object to some implementation
details though.
>>
>> Still a question of how we handle atom driver registration...
>
> Not sure what you are saying here. Are you referring to the section
> magic here? If so, then the sections get mapped to symbols identifying
> the start and end of the driver array and this replaces the current
> g_driver_list mechanism currently in use.
>
No, I meant that one could blacklist the atmodem plugin for example and
effectively disable all atmodem atom drivers. So this ability would be
lost with this approach. But as stated above, I'm okay losing that if
nobody else complains.
>>
>>>
>>> ii) Move the 'modem driver' part of the modem plugins into the drivers/
>>> directory.
>>
>> I'm not sure drivers/ is really the right place. At least it is weird
>> to me that gobi would live in drivers/qmimodem/. Perhaps we need a
>> place for modem drivers only.
>
> drivers/ feels right, though I agree drivers/qmimodem might not be quite
> right. Point is, it's not a plugin so to at least get it out of plugins/.
>
> Is it worth my continuing with this?
>
I would say yes.
Regards,
-Denis
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2018-10-30 14:42 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-10-26 9:54 [PATCH 0/6] Driver registration rework Jonas Bonn
2018-10-26 9:54 ` [PATCH 1/6] include: add macros for declaring drivers Jonas Bonn
2018-10-26 9:54 ` [PATCH 2/6] qmimodem: simplify driver registration Jonas Bonn
2018-10-26 9:54 ` [PATCH 3/6] gobi: modem driver cannot be a plugin Jonas Bonn
2018-10-26 9:54 ` [PATCH 4/6] atmodem: add missing headers to atutil.h Jonas Bonn
2018-10-26 9:54 ` [PATCH 5/6] telitmodem: simplify driver registration Jonas Bonn
2018-10-26 9:54 ` [PATCH 6/6] telitmodem: modem driver is not a plugin Jonas Bonn
2018-10-29 20:26 ` [PATCH 0/6] Driver registration rework Denis Kenzior
2018-10-30 13:07 ` Jonas Bonn
2018-10-30 14:42 ` Denis Kenzior
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.