| Message ID | 20180427140358.30839-1-elder@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Andy Gross |
| Headers | show |
Was there something wrong with this patch? I sent it a long time ago but it still applies cleanly. I can re-send if you like. -Alex On 04/27/2018 09:03 AM, Alex Elder wrote: > Add a prompt to the "tristate" attribute in the Kconfig file in > which QCOM_QMI_HELPERS is defined; I find it doesn't get selected > without it. > > Signed-off-by: Alex Elder <elder@linaro.org> > --- > drivers/soc/qcom/Kconfig | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig > index 5c4535b545cc..81a501765cd9 100644 > --- a/drivers/soc/qcom/Kconfig > +++ b/drivers/soc/qcom/Kconfig > @@ -36,7 +36,7 @@ config QCOM_PM > low power modes. > > config QCOM_QMI_HELPERS > - tristate > + tristate "Qualcomm QMI Helpers" > depends on ARCH_QCOM && NET > help > Helper library for handling QMI encoded messages. QMI encoded >
On Fri, Sep 28, 2018 at 06:41:11AM -0500, Alex Elder wrote: > Was there something wrong with this patch? I sent it a long time > ago but it still applies cleanly. I can re-send if you like. Hello Alex, This patch does not apply on top of https://git.kernel.org/pub/scm/linux/kernel/git/agross/linux.git/log/?h=for-next When you don't specify a string for a Kconfig, then you won't be able to select it via menuconfig, only through other Kconfigs, e.g.: $ git grep QCOM_QMI_HELPERS drivers/remoteproc/Kconfig: select QCOM_QMI_HELPERS What Kconfig is it that needs QCOM_QMI_HELPERS? Why do you need to make is selectable via menuconfig? Isn't it sufficient to add a "select QCOM_QMI_HELPERS" to that Kconfig? Kind regards, Niklas > > -Alex > > On 04/27/2018 09:03 AM, Alex Elder wrote: > > Add a prompt to the "tristate" attribute in the Kconfig file in > > which QCOM_QMI_HELPERS is defined; I find it doesn't get selected > > without it. > > > > Signed-off-by: Alex Elder <elder@linaro.org> > > --- > > drivers/soc/qcom/Kconfig | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig > > index 5c4535b545cc..81a501765cd9 100644 > > --- a/drivers/soc/qcom/Kconfig > > +++ b/drivers/soc/qcom/Kconfig > > @@ -36,7 +36,7 @@ config QCOM_PM > > low power modes. > > > > config QCOM_QMI_HELPERS > > - tristate > > + tristate "Qualcomm QMI Helpers" > > depends on ARCH_QCOM && NET > > help > > Helper library for handling QMI encoded messages. QMI encoded > > >
On 09/28/2018 07:26 AM, Niklas Cassel wrote: > On Fri, Sep 28, 2018 at 06:41:11AM -0500, Alex Elder wrote: >> Was there something wrong with this patch? I sent it a long time >> ago but it still applies cleanly. I can re-send if you like. > > Hello Alex, > > This patch does not apply on top of > https://git.kernel.org/pub/scm/linux/kernel/git/agross/linux.git/log/?h=for-next Sorry, I sent this before I tracked down where Andy's for-next tree was. It applied OK to linus/master. > When you don't specify a string for a Kconfig, then you won't be able to > select it via menuconfig, only through other Kconfigs, e.g.: > $ git grep QCOM_QMI_HELPERS > drivers/remoteproc/Kconfig: select QCOM_QMI_HELPERS > > What Kconfig is it that needs QCOM_QMI_HELPERS? IPA, which is not yet merged. The entry in "drivers/soc/qcom/Kconfig" for this option includes help text, so I presume it was an oversight not to include a string (prompt). It is the only non-trivial entry in that file that does not include one. Why should this entry not be listed in menus? > Why do you need to make is selectable via menuconfig? > Isn't it sufficient to add a "select QCOM_QMI_HELPERS" to that Kconfig? I have CONFIG_IPA dependent on CONFIG_QCOM_QMI_HELPERS. I could change that to select it instead but I still think this entry should be changed. I'll mention that SLIM_QCOM_NGD_CTRL in drivers/slimbus/Kconfig also depends on--rather than selects--QCOM_QMI_HELPERS. -Alex > > Kind regards, > Niklas > >> >> -Alex >> >> On 04/27/2018 09:03 AM, Alex Elder wrote: >>> Add a prompt to the "tristate" attribute in the Kconfig file in >>> which QCOM_QMI_HELPERS is defined; I find it doesn't get selected >>> without it. >>> >>> Signed-off-by: Alex Elder <elder@linaro.org> >>> --- >>> drivers/soc/qcom/Kconfig | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig >>> index 5c4535b545cc..81a501765cd9 100644 >>> --- a/drivers/soc/qcom/Kconfig >>> +++ b/drivers/soc/qcom/Kconfig >>> @@ -36,7 +36,7 @@ config QCOM_PM >>> low power modes. >>> >>> config QCOM_QMI_HELPERS >>> - tristate >>> + tristate "Qualcomm QMI Helpers" >>> depends on ARCH_QCOM && NET >>> help >>> Helper library for handling QMI encoded messages. QMI encoded >>> >>
On Fri, Sep 28, 2018 at 08:23:03AM -0500, Alex Elder wrote: > On 09/28/2018 07:26 AM, Niklas Cassel wrote: > > On Fri, Sep 28, 2018 at 06:41:11AM -0500, Alex Elder wrote: > >> Was there something wrong with this patch? I sent it a long time > >> ago but it still applies cleanly. I can re-send if you like. > > > > Hello Alex, > > > > This patch does not apply on top of > > https://git.kernel.org/pub/scm/linux/kernel/git/agross/linux.git/log/?h=for-next > > Sorry, I sent this before I tracked down where Andy's for-next > tree was. It applied OK to linus/master. > > > When you don't specify a string for a Kconfig, then you won't be able to > > select it via menuconfig, only through other Kconfigs, e.g.: > > $ git grep QCOM_QMI_HELPERS > > drivers/remoteproc/Kconfig: select QCOM_QMI_HELPERS > > > > What Kconfig is it that needs QCOM_QMI_HELPERS? > > IPA, which is not yet merged. > > The entry in "drivers/soc/qcom/Kconfig" for this option includes > help text, so I presume it was an oversight not to include a > string (prompt). It is the only non-trivial entry in that file > that does not include one. Why should this entry not be listed > in menus? I see. Indeed it is a bit confusing that it has a help text. > > > Why do you need to make is selectable via menuconfig? > > Isn't it sufficient to add a "select QCOM_QMI_HELPERS" to that Kconfig? > > I have CONFIG_IPA dependent on CONFIG_QCOM_QMI_HELPERS. I could > change that to select it instead but I still think this entry > should be changed. > > I'll mention that SLIM_QCOM_NGD_CTRL in drivers/slimbus/Kconfig > also depends on--rather than selects--QCOM_QMI_HELPERS. I would have expected SLIM_QCOM_NGD_CTRL to select rather than depend. But then it is also weird that QCOM_QMI_HELPERS has a help text. I guess Bjorn should answer how this Kconfig was intended to be used, because right now it is kind of ambiguous. Kind regards, Niklas > > -Alex > > > > > Kind regards, > > Niklas > > > >> > >> -Alex > >> > >> On 04/27/2018 09:03 AM, Alex Elder wrote: > >>> Add a prompt to the "tristate" attribute in the Kconfig file in > >>> which QCOM_QMI_HELPERS is defined; I find it doesn't get selected > >>> without it. > >>> > >>> Signed-off-by: Alex Elder <elder@linaro.org> > >>> --- > >>> drivers/soc/qcom/Kconfig | 2 +- > >>> 1 file changed, 1 insertion(+), 1 deletion(-) > >>> > >>> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig > >>> index 5c4535b545cc..81a501765cd9 100644 > >>> --- a/drivers/soc/qcom/Kconfig > >>> +++ b/drivers/soc/qcom/Kconfig > >>> @@ -36,7 +36,7 @@ config QCOM_PM > >>> low power modes. > >>> > >>> config QCOM_QMI_HELPERS > >>> - tristate > >>> + tristate "Qualcomm QMI Helpers" > >>> depends on ARCH_QCOM && NET > >>> help > >>> Helper library for handling QMI encoded messages. QMI encoded > >>> > >> >
On Fri 28 Sep 06:23 PDT 2018, Alex Elder wrote: > On 09/28/2018 07:26 AM, Niklas Cassel wrote: > > On Fri, Sep 28, 2018 at 06:41:11AM -0500, Alex Elder wrote: > >> Was there something wrong with this patch? I sent it a long time > >> ago but it still applies cleanly. I can re-send if you like. > > > > Hello Alex, > > > > This patch does not apply on top of > > https://git.kernel.org/pub/scm/linux/kernel/git/agross/linux.git/log/?h=for-next > > Sorry, I sent this before I tracked down where Andy's for-next > tree was. It applied OK to linus/master. > > > When you don't specify a string for a Kconfig, then you won't be able to > > select it via menuconfig, only through other Kconfigs, e.g.: > > $ git grep QCOM_QMI_HELPERS > > drivers/remoteproc/Kconfig: select QCOM_QMI_HELPERS > > > > What Kconfig is it that needs QCOM_QMI_HELPERS? > > IPA, which is not yet merged. > > The entry in "drivers/soc/qcom/Kconfig" for this option includes > help text, so I presume it was an oversight not to include a > string (prompt). It is the only non-trivial entry in that file > that does not include one. Why should this entry not be listed > in menus? > > > Why do you need to make is selectable via menuconfig? I made it this way because I don't see a reason for anyone to ever select this config option if there isn't a client available - and clients that "depends on" this simply forces the developer to find the QMI helpers in menuconfig and enable it manually. > > Isn't it sufficient to add a "select QCOM_QMI_HELPERS" to that Kconfig? > > I have CONFIG_IPA dependent on CONFIG_QCOM_QMI_HELPERS. I could > change that to select it instead but I still think this entry > should be changed. > Why do you say it needs to change? I tested this by just adding a "select QCOM_QMI_HELPERS" in my client drivers. > I'll mention that SLIM_QCOM_NGD_CTRL in drivers/slimbus/Kconfig > also depends on--rather than selects--QCOM_QMI_HELPERS. > That's wrong. Regards, Bjorn > -Alex > > > > > Kind regards, > > Niklas > > > >> > >> -Alex > >> > >> On 04/27/2018 09:03 AM, Alex Elder wrote: > >>> Add a prompt to the "tristate" attribute in the Kconfig file in > >>> which QCOM_QMI_HELPERS is defined; I find it doesn't get selected > >>> without it. > >>> > >>> Signed-off-by: Alex Elder <elder@linaro.org> > >>> --- > >>> drivers/soc/qcom/Kconfig | 2 +- > >>> 1 file changed, 1 insertion(+), 1 deletion(-) > >>> > >>> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig > >>> index 5c4535b545cc..81a501765cd9 100644 > >>> --- a/drivers/soc/qcom/Kconfig > >>> +++ b/drivers/soc/qcom/Kconfig > >>> @@ -36,7 +36,7 @@ config QCOM_PM > >>> low power modes. > >>> > >>> config QCOM_QMI_HELPERS > >>> - tristate > >>> + tristate "Qualcomm QMI Helpers" > >>> depends on ARCH_QCOM && NET > >>> help > >>> Helper library for handling QMI encoded messages. QMI encoded > >>> > >> >
On 09/28/2018 12:21 PM, Bjorn Andersson wrote: >> I have CONFIG_IPA dependent on CONFIG_QCOM_QMI_HELPERS. I could >> change that to select it instead but I still think this entry >> should be changed. I was thinking one of two changes: - Add the prompt string (which I suggested) - Don't add the prompt, but remove the help and/or add a comment to indicate that the lack of prompt is intentional (because it really looked like an oversight to me) I don't really care though; I'll change my code to select it rather than depend on it. > Why do you say it needs to change? I tested this by just adding a > "select QCOM_QMI_HELPERS" in my client drivers. > >> I'll mention that SLIM_QCOM_NGD_CTRL in drivers/slimbus/Kconfig >> also depends on--rather than selects--QCOM_QMI_HELPERS. >> > That's wrong. Maybe someone else should fix this, but I can do it if you like (and if that's the case, say so). -Alex
diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig index 5c4535b545cc..81a501765cd9 100644 --- a/drivers/soc/qcom/Kconfig +++ b/drivers/soc/qcom/Kconfig @@ -36,7 +36,7 @@ config QCOM_PM low power modes. config QCOM_QMI_HELPERS - tristate + tristate "Qualcomm QMI Helpers" depends on ARCH_QCOM && NET help Helper library for handling QMI encoded messages. QMI encoded
Add a prompt to the "tristate" attribute in the Kconfig file in which QCOM_QMI_HELPERS is defined; I find it doesn't get selected without it. Signed-off-by: Alex Elder <elder@linaro.org> --- drivers/soc/qcom/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)