| Message ID | 20210404184126.974098-1-petr.vorel@gmail.com |
|---|---|
| State | Rejected |
| Headers | show |
| Series | [1/1] package/modem-manager: enable qcom-soc if libqrtr-glib enabled | expand |
On Sun, 4 Apr 2021 20:41:26 +0200 Petr Vorel <petr.vorel@gmail.com> wrote: > This plugin makes sense only for Qualcomm MSM phone. > Although there is currently no such device defconfig in Buildroot yet, > it makes sense to prepare modem-manager for it. > > Signed-off-by: Petr Vorel <petr.vorel@gmail.com> > --- > package/modem-manager/modem-manager.mk | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/package/modem-manager/modem-manager.mk b/package/modem-manager/modem-manager.mk > index 2cc45d266b..d62d13cf45 100644 > --- a/package/modem-manager/modem-manager.mk > +++ b/package/modem-manager/modem-manager.mk > @@ -34,6 +34,12 @@ else > MODEM_MANAGER_CONF_OPTS += --without-mbim > endif > > +ifeq ($(BR2_PACKAGE_LIBQRTR_GLIB),y) > +MODEM_MANAGER_CONF_OPTS += --enable-plugin-qcom-soc Isn't that missing a MODEM_MANAGER_DEPENDENCIES += libqrtr-glib line ? Or is it really a runtime dependency ? Thomas
> On Sun, 4 Apr 2021 20:41:26 +0200 > Petr Vorel <petr.vorel@gmail.com> wrote: > > This plugin makes sense only for Qualcomm MSM phone. > > Although there is currently no such device defconfig in Buildroot yet, > > it makes sense to prepare modem-manager for it. > > Signed-off-by: Petr Vorel <petr.vorel@gmail.com> > > --- > > package/modem-manager/modem-manager.mk | 6 ++++++ > > 1 file changed, 6 insertions(+) > > diff --git a/package/modem-manager/modem-manager.mk b/package/modem-manager/modem-manager.mk > > index 2cc45d266b..d62d13cf45 100644 > > --- a/package/modem-manager/modem-manager.mk > > +++ b/package/modem-manager/modem-manager.mk > > @@ -34,6 +34,12 @@ else > > MODEM_MANAGER_CONF_OPTS += --without-mbim > > endif > > +ifeq ($(BR2_PACKAGE_LIBQRTR_GLIB),y) > > +MODEM_MANAGER_CONF_OPTS += --enable-plugin-qcom-soc > Isn't that missing a MODEM_MANAGER_DEPENDENCIES += libqrtr-glib line ? > Or is it really a runtime dependency ? error: Couldn't find libqmi-glib >= 1.29.3 when building locally. => you're right it probably requires MODEM_MANAGER_DEPENDENCIES += libqrtr-glib I'll test it and probably sent v2. Thanks for catching error! Kind regards, Petr > Thomas
> On Sun, 4 Apr 2021 20:41:26 +0200 > Petr Vorel <petr.vorel@gmail.com> wrote: > > This plugin makes sense only for Qualcomm MSM phone. > > Although there is currently no such device defconfig in Buildroot yet, > > it makes sense to prepare modem-manager for it. > > Signed-off-by: Petr Vorel <petr.vorel@gmail.com> > > --- > > package/modem-manager/modem-manager.mk | 6 ++++++ > > 1 file changed, 6 insertions(+) > > diff --git a/package/modem-manager/modem-manager.mk b/package/modem-manager/modem-manager.mk > > index 2cc45d266b..d62d13cf45 100644 > > --- a/package/modem-manager/modem-manager.mk > > +++ b/package/modem-manager/modem-manager.mk > > @@ -34,6 +34,12 @@ else > > MODEM_MANAGER_CONF_OPTS += --without-mbim > > endif > > +ifeq ($(BR2_PACKAGE_LIBQRTR_GLIB),y) > > +MODEM_MANAGER_CONF_OPTS += --enable-plugin-qcom-soc > Isn't that missing a MODEM_MANAGER_DEPENDENCIES += libqrtr-glib line ? > Or is it really a runtime dependency ? Actually verified locally on buildroot, it works as is. (no need for MODEM_MANAGER_DEPENDENCIES += libqrtr-glib). Thus I've updated patchwork status to NEW. Kind regards, Petr > Thomas
On Mon, 5 Apr 2021 15:44:13 +0200 Petr Vorel <petr.vorel@gmail.com> wrote: > > > +ifeq ($(BR2_PACKAGE_LIBQRTR_GLIB),y) > > > +MODEM_MANAGER_CONF_OPTS += --enable-plugin-qcom-soc > > > Isn't that missing a MODEM_MANAGER_DEPENDENCIES += libqrtr-glib line ? > > Or is it really a runtime dependency ? > Actually verified locally on buildroot, it works as is. > (no need for MODEM_MANAGER_DEPENDENCIES += libqrtr-glib). > > Thus I've updated patchwork status to NEW. How can this work? Does it dlopen() the libqrtr-glib library at runtime? This deserves some explanation as we would normally imagine that to be a build-time dependency. Thomas
On Mon, 5 Apr 2021, 16:36 Thomas Petazzoni, <thomas.petazzoni@bootlin.com> wrote: > On Mon, 5 Apr 2021 15:44:13 +0200 > Petr Vorel <petr.vorel@gmail.com> wrote: > > > > > +ifeq ($(BR2_PACKAGE_LIBQRTR_GLIB),y) > > > > +MODEM_MANAGER_CONF_OPTS += --enable-plugin-qcom-soc > > > > > Isn't that missing a MODEM_MANAGER_DEPENDENCIES += libqrtr-glib line ? > > > Or is it really a runtime dependency ? > > Actually verified locally on buildroot, it works as is. > > (no need for MODEM_MANAGER_DEPENDENCIES += libqrtr-glib). > > > > Thus I've updated patchwork status to NEW. > > How can this work? Does it dlopen() the libqrtr-glib library at > runtime? This deserves some explanation as we would normally imagine > that to be a build-time dependency. > It's probably too soon to add libqrtr-glib as a dependency of MM, the QRTR support is not even in MM git master yet (likely this week). libqrtr-glib is right now only a dependency of libqmi, I think we should focus on that. Let's add the MM build-time dependency once there is a MM release with QRTR support. >
Hi Thomas, Aleksander, > On Mon, 5 Apr 2021, 16:36 Thomas Petazzoni, <thomas.petazzoni@bootlin.com> > wrote: > > On Mon, 5 Apr 2021 15:44:13 +0200 > > Petr Vorel <petr.vorel@gmail.com> wrote: > > > > > +ifeq ($(BR2_PACKAGE_LIBQRTR_GLIB),y) > > > > > +MODEM_MANAGER_CONF_OPTS += --enable-plugin-qcom-soc > > > > Isn't that missing a MODEM_MANAGER_DEPENDENCIES += libqrtr-glib line ? > > > > Or is it really a runtime dependency ? > > > Actually verified locally on buildroot, it works as is. > > > (no need for MODEM_MANAGER_DEPENDENCIES += libqrtr-glib). > > > Thus I've updated patchwork status to NEW. > > How can this work? Does it dlopen() the libqrtr-glib library at > > runtime? This deserves some explanation as we would normally imagine > > that to be a build-time dependency. > It's probably too soon to add libqrtr-glib as a dependency of MM, the QRTR > support is not even in MM git master yet (likely this week). > libqrtr-glib is right now only a dependency of libqmi, I think we should > focus on that. Let's add the MM build-time dependency once there is a MM > release with QRTR support. Thomas, sorry for confusion. Aleksander, please correct me if I'm wrong. Looking at ModemManager sources (8fc60754 "qcom-soc: new plugin for Qualcomm SoCs") it currently does not depend on libqrtr-glib at all (not sure if you want use libqrtr-glib dependency later only for qcom-soc for for something else in ModemManager). I based decision for --enable-plugin-qcom-soc on BR2_PACKAGE_LIBQRTR_GLIB, because if one wants Qualcomm IPC Router protocol helper library, he probably wants also qcom-soc plugin. And even nobody is now using Buildroot for Qualcomm phones, it can change in the future. Kind regards, Petr
Hey, > > > > > > +ifeq ($(BR2_PACKAGE_LIBQRTR_GLIB),y) > > > > > > +MODEM_MANAGER_CONF_OPTS += --enable-plugin-qcom-soc > > > > > > Isn't that missing a MODEM_MANAGER_DEPENDENCIES += libqrtr-glib line ? > > > > > Or is it really a runtime dependency ? > > > > Actually verified locally on buildroot, it works as is. > > > > (no need for MODEM_MANAGER_DEPENDENCIES += libqrtr-glib). > > > > > Thus I've updated patchwork status to NEW. > > > > How can this work? Does it dlopen() the libqrtr-glib library at > > > runtime? This deserves some explanation as we would normally imagine > > > that to be a build-time dependency. > > > > > It's probably too soon to add libqrtr-glib as a dependency of MM, the QRTR > > support is not even in MM git master yet (likely this week). > > > libqrtr-glib is right now only a dependency of libqmi, I think we should > > focus on that. Let's add the MM build-time dependency once there is a MM > > release with QRTR support. > > Thomas, sorry for confusion. Aleksander, please correct me if I'm wrong. Looking > at ModemManager sources (8fc60754 "qcom-soc: new plugin for Qualcomm SoCs") it > currently does not depend on libqrtr-glib at all (not sure if you want use > libqrtr-glib dependency later only for qcom-soc for for something else in > ModemManager). > Yes, that's exactly what will happen. Once the QRTR support is integrated, it will only be used in the qcom-soc plugin. If you don't plan to build the qcom-soc plugin, you could build ModemManager configuring --without-qrtr and/or disabling the QRTR support also in libqmi. > I based decision for --enable-plugin-qcom-soc on BR2_PACKAGE_LIBQRTR_GLIB, > because if one wants Qualcomm IPC Router protocol helper library, he probably > wants also qcom-soc plugin. And even nobody is now using Buildroot for Qualcomm > phones, it can change in the future. It isn't strictly necessary though, you could use the qcom-soc plugin without QRTR, using RPMSG instead (as it is the case right now in MM 1.16). It really depends on the SoC you're targeting to use.
On Sun, 4 Apr 2021 20:41:26 +0200 Petr Vorel <petr.vorel@gmail.com> wrote: > This plugin makes sense only for Qualcomm MSM phone. > Although there is currently no such device defconfig in Buildroot yet, > it makes sense to prepare modem-manager for it. > > Signed-off-by: Petr Vorel <petr.vorel@gmail.com> > --- > package/modem-manager/modem-manager.mk | 6 ++++++ > 1 file changed, 6 insertions(+) Peter, Alexander, based on your feedback, I have marked this patch as Rejected. If that is an incorrect assessment of the situation from me, do not hesitate to let me know. Thanks! Thomas
Hi Thomas, > On Sun, 4 Apr 2021 20:41:26 +0200 > Petr Vorel <petr.vorel@gmail.com> wrote: > > This plugin makes sense only for Qualcomm MSM phone. > > Although there is currently no such device defconfig in Buildroot yet, > > it makes sense to prepare modem-manager for it. > > Signed-off-by: Petr Vorel <petr.vorel@gmail.com> > > --- > > package/modem-manager/modem-manager.mk | 6 ++++++ > > 1 file changed, 6 insertions(+) > Peter, Alexander, based on your feedback, I have marked this patch as > Rejected. If that is an incorrect assessment of the situation from me, > do not hesitate to let me know. OK for me, I should have done it before myself. I expect package/libqrtr-glib/ will be used in future for more than just this driver (now is this package IMHO useless, but that should change in the future). Kind regards, Petr > Thanks! > Thomas
diff --git a/package/modem-manager/modem-manager.mk b/package/modem-manager/modem-manager.mk index 2cc45d266b..d62d13cf45 100644 --- a/package/modem-manager/modem-manager.mk +++ b/package/modem-manager/modem-manager.mk @@ -34,6 +34,12 @@ else MODEM_MANAGER_CONF_OPTS += --without-mbim endif +ifeq ($(BR2_PACKAGE_LIBQRTR_GLIB),y) +MODEM_MANAGER_CONF_OPTS += --enable-plugin-qcom-soc +else +MODEM_MANAGER_CONF_OPTS += --disable-plugin-qcom-soc +endif + define MODEM_MANAGER_INSTALL_INIT_SYSV $(INSTALL) -m 0755 -D package/modem-manager/S44modem-manager \ $(TARGET_DIR)/etc/init.d/S44modem-manager
This plugin makes sense only for Qualcomm MSM phone. Although there is currently no such device defconfig in Buildroot yet, it makes sense to prepare modem-manager for it. Signed-off-by: Petr Vorel <petr.vorel@gmail.com> --- package/modem-manager/modem-manager.mk | 6 ++++++ 1 file changed, 6 insertions(+)