diff mbox series

firmware: arm_scmi: fix i.MX build dependency

Message ID 20241115230555.2435004-1-arnd@kernel.org (mailing list archive)
State New
Headers show
Series firmware: arm_scmi: fix i.MX build dependency | expand

Commit Message

Arnd Bergmann Nov. 15, 2024, 11:05 p.m. UTC
From: Arnd Bergmann <arnd@arndb.de>

The newly added SCMI vendor driver references functions in the
protocol driver but needs a Kconfig dependency to ensure it can link,
essentially the Kconfig dependency needs to be reversed to match the
link time dependency:

arm-linux-gnueabi-ld: sound/soc/fsl/fsl_mqs.o: in function `fsl_mqs_sm_write':
fsl_mqs.c:(.text+0x1aa): undefined reference to `scmi_imx_misc_ctrl_set'
arm-linux-gnueabi-ld: sound/soc/fsl/fsl_mqs.o: in function `fsl_mqs_sm_read':
fsl_mqs.c:(.text+0x1ee): undefined reference to `scmi_imx_misc_ctrl_get'

This however only works after changing the dependency in the SND_SOC_FSL_MQS
driver as well, which uses 'select IMX_SCMI_MISC_DRV' to turn on a
driver it depends on. This is generally a bad idea, so the best solution
is to change that into a dependency.

To allow the ASoC driver to keep building with the SCMI support, this
needs to be an optional dependency that enforces the link-time
dependency if IMX_SCMI_MISC_DRV is a loadable module but not
depend on it if that is disabled.

Fixes: 61c9f03e22fc ("firmware: arm_scmi: Add initial support for i.MX MISC protocol")
Fixes: 101c9023594a ("ASoC: fsl_mqs: Support accessing registers by scmi interface")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
I'm not sure this is the version we want to go with in the end, without
having read the code in detail, but the version in linux-next is clearly
wrong in multiple ways and this is the best I could come up with so far.

If we can agree on this approach, it needs to go through the ASoC tree
as that contains the 101c9023594a commit at the moment.
---
 drivers/firmware/arm_scmi/vendors/imx/Kconfig | 1 +
 drivers/firmware/imx/Kconfig                  | 1 -
 sound/soc/fsl/Kconfig                         | 2 +-
 3 files changed, 2 insertions(+), 2 deletions(-)

Comments

Shawn Guo Nov. 16, 2024, 3:30 a.m. UTC | #1
On Sat, Nov 16, 2024 at 12:05:18AM +0100, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> The newly added SCMI vendor driver references functions in the
> protocol driver but needs a Kconfig dependency to ensure it can link,
> essentially the Kconfig dependency needs to be reversed to match the
> link time dependency:
> 
> arm-linux-gnueabi-ld: sound/soc/fsl/fsl_mqs.o: in function `fsl_mqs_sm_write':
> fsl_mqs.c:(.text+0x1aa): undefined reference to `scmi_imx_misc_ctrl_set'
> arm-linux-gnueabi-ld: sound/soc/fsl/fsl_mqs.o: in function `fsl_mqs_sm_read':
> fsl_mqs.c:(.text+0x1ee): undefined reference to `scmi_imx_misc_ctrl_get'
> 
> This however only works after changing the dependency in the SND_SOC_FSL_MQS
> driver as well, which uses 'select IMX_SCMI_MISC_DRV' to turn on a
> driver it depends on. This is generally a bad idea, so the best solution
> is to change that into a dependency.
> 
> To allow the ASoC driver to keep building with the SCMI support, this
> needs to be an optional dependency that enforces the link-time
> dependency if IMX_SCMI_MISC_DRV is a loadable module but not
> depend on it if that is disabled.
> 
> Fixes: 61c9f03e22fc ("firmware: arm_scmi: Add initial support for i.MX MISC protocol")
> Fixes: 101c9023594a ("ASoC: fsl_mqs: Support accessing registers by scmi interface")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> I'm not sure this is the version we want to go with in the end, without
> having read the code in detail, but the version in linux-next is clearly
> wrong in multiple ways and this is the best I could come up with so far.
> 
> If we can agree on this approach, it needs to go through the ASoC tree
> as that contains the 101c9023594a commit at the moment.

Thanks for spotting and fixing, Arnd!

Unless Peng has objection to this change, I'm good with it.

Shawn
Cristian Marussi Nov. 17, 2024, 10:03 a.m. UTC | #2
On Sat, Nov 16, 2024 at 12:05:18AM +0100, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> 

Hi Arnd,

> The newly added SCMI vendor driver references functions in the
> protocol driver but needs a Kconfig dependency to ensure it can link,
> essentially the Kconfig dependency needs to be reversed to match the
> link time dependency:
> 
> arm-linux-gnueabi-ld: sound/soc/fsl/fsl_mqs.o: in function `fsl_mqs_sm_write':
> fsl_mqs.c:(.text+0x1aa): undefined reference to `scmi_imx_misc_ctrl_set'
> arm-linux-gnueabi-ld: sound/soc/fsl/fsl_mqs.o: in function `fsl_mqs_sm_read':
> fsl_mqs.c:(.text+0x1ee): undefined reference to `scmi_imx_misc_ctrl_get'
> 

The SCMI drivers, like the newly added IMX_SCMI_MISC_DRV, generally make
ue of the related vendor protocol like IMX_SCMI_MISC_EXT, BUT the SCMI
stack is designed in a way that NO symbols are needed to be exported by
the protocol layer (to avoid a huge and growing number of symbols
exports)...so usually the current DRV-->PROTO dependency is fine.

In this case, AFAIU, it is the SCMI driver that in turn exports a few
helpers that are used by another driver fsl_mqs, which in turn could be
compiled and work with or without the SCMI stack, so with this patch we
are artificially reversing the DRV<--PROTO dependency to solve this
scenario in all the compillation scenarios...

....BUT given that the IMX_SCMI_MISC_DRV is the one that should export
the missing symbols could NOT this solved in a cleaner way, without
adding the fake reverse dependency, by instead modifying the header of
the driver with something like the classic:

--->8-----
diff --git a/include/linux/firmware/imx/sm.h b/include/linux/firmware/imx/sm.h
index 9b85a3f028d1..3a7a3ec367c5 100644
--- a/include/linux/firmware/imx/sm.h
+++ b/include/linux/firmware/imx/sm.h
@@ -17,7 +17,19 @@
 #define SCMI_IMX_CTRL_SAI4_MCLK                4       /* WAKE SAI4 MCLK */
 #define SCMI_IMX_CTRL_SAI5_MCLK                5       /* WAKE SAI5 MCLK */
 
+#ifdef IMX_SCMI_MISC_DRV
 int scmi_imx_misc_ctrl_get(u32 id, u32 *num, u32 *val);
 int scmi_imx_misc_ctrl_set(u32 id, u32 val);
+#else
+static inline int scmi_imx_misc_ctrl_get(u32 id, u32 *num, u32 *val)
+{
+       return 0;
+}
+
+static inline int scmi_imx_misc_ctrl_set(u32 id, u32 val)
+{
+       return 0;
+}
+#endif
 
 #endif
----->8-----------

....to just support compilation in all the scenarios.

> This however only works after changing the dependency in the SND_SOC_FSL_MQS
> driver as well, which uses 'select IMX_SCMI_MISC_DRV' to turn on a
> driver it depends on. This is generally a bad idea, so the best solution
> is to change that into a dependency.
> 
> To allow the ASoC driver to keep building with the SCMI support, this
> needs to be an optional dependency that enforces the link-time
> dependency if IMX_SCMI_MISC_DRV is a loadable module but not
> depend on it if that is disabled.
> 

...and maybe with the above additions you could avoid also these other
dep changes...

...not sure if I am missing something and I have definitely not tested
any of my babbling above...

Thanks,
Cristian
Arnd Bergmann Nov. 17, 2024, 10:16 a.m. UTC | #3
On Sun, Nov 17, 2024, at 11:03, Cristian Marussi wrote:
> On Sat, Nov 16, 2024 at 12:05:18AM +0100, Arnd Bergmann wrote:
>> 
>> arm-linux-gnueabi-ld: sound/soc/fsl/fsl_mqs.o: in function `fsl_mqs_sm_write':
>> fsl_mqs.c:(.text+0x1aa): undefined reference to `scmi_imx_misc_ctrl_set'
>> arm-linux-gnueabi-ld: sound/soc/fsl/fsl_mqs.o: in function `fsl_mqs_sm_read':
>> fsl_mqs.c:(.text+0x1ee): undefined reference to `scmi_imx_misc_ctrl_get'
>> 
>
> The SCMI drivers, like the newly added IMX_SCMI_MISC_DRV, generally make
> ue of the related vendor protocol like IMX_SCMI_MISC_EXT, BUT the SCMI
> stack is designed in a way that NO symbols are needed to be exported by
> the protocol layer (to avoid a huge and growing number of symbols
> exports)...so usually the current DRV-->PROTO dependency is fine.
>
> In this case, AFAIU, it is the SCMI driver that in turn exports a few
> helpers that are used by another driver fsl_mqs, which in turn could be
> compiled and work with or without the SCMI stack, so with this patch we
> are artificially reversing the DRV<--PROTO dependency to solve this
> scenario in all the compillation scenarios...
>
> ....BUT given that the IMX_SCMI_MISC_DRV is the one that should export
> the missing symbols could NOT this solved in a cleaner way, without
> adding the fake reverse dependency, by instead modifying the header of
> the driver with something like the classic:

> --->8-----
> diff --git a/include/linux/firmware/imx/sm.h b/include/linux/firmware/imx/sm.h
> index 9b85a3f028d1..3a7a3ec367c5 100644
> --- a/include/linux/firmware/imx/sm.h
> +++ b/include/linux/firmware/imx/sm.h
> @@ -17,7 +17,19 @@
>  #define SCMI_IMX_CTRL_SAI4_MCLK                4       /* WAKE SAI4 MCLK */
>  #define SCMI_IMX_CTRL_SAI5_MCLK                5       /* WAKE SAI5 MCLK */
> 
> +#ifdef IMX_SCMI_MISC_DRV
>  int scmi_imx_misc_ctrl_get(u32 id, u32 *num, u32 *val);
>  int scmi_imx_misc_ctrl_set(u32 id, u32 val);
> +#else
> +static inline int scmi_imx_misc_ctrl_get(u32 id, u32 *num, u32 *val)
> +{
> +       return 0;
> +}
> +
> +static inline int scmi_imx_misc_ctrl_set(u32 id, u32 val)
> +{
> +       return 0;
> +}
> +#endif

This usually doesn't work if the provider of these interfaces
can be in a loadable module. The #ifdef above means this won't
be usable when CONFIG_IMX_SCMI_MISC_DRV=m, while changing it to
IS_ENABLED(CONFIG_IMX_SCMI_MISC_DRV) still produces a link error
when the consumer is built-in. Changing it to IS_REACHABLE()
in turn is even worse because it avoids the link failure but
makes it silently do the wrong thing in some configurations.

>  #endif
> ----->8-----------
>
> ....to just support compilation in all the scenarios.
>
>> This however only works after changing the dependency in the SND_SOC_FSL_MQS
>> driver as well, which uses 'select IMX_SCMI_MISC_DRV' to turn on a
>> driver it depends on. This is generally a bad idea, so the best solution
>> is to change that into a dependency.
>> 
>> To allow the ASoC driver to keep building with the SCMI support, this
>> needs to be an optional dependency that enforces the link-time
>> dependency if IMX_SCMI_MISC_DRV is a loadable module but not
>> depend on it if that is disabled.
>> 
>
> ...and maybe with the above additions you could avoid also these other
> dep changes...
>
> ...not sure if I am missing something and I have definitely not tested
> any of my babbling above...

In my experience, there is no way to avoid reflecting the dependencies
correctly in Kconfig: if one driver has an EXPORT_SYMBOL that
gets picked up by another driver, you need a matching 'depends on'.

       Arnd
Peng Fan Nov. 17, 2024, 11:03 a.m. UTC | #4
> Subject: Re: [PATCH] firmware: arm_scmi: fix i.MX build dependency
> 
> On Sat, Nov 16, 2024 at 12:05:18AM +0100, Arnd Bergmann wrote:
> > From: Arnd Bergmann <arnd@arndb.de>
> >
> 
> Hi Arnd,
> 
> > The newly added SCMI vendor driver references functions in the
> > protocol driver but needs a Kconfig dependency to ensure it can link,
> > essentially the Kconfig dependency needs to be reversed to match
> the
> > link time dependency:
> >
> > arm-linux-gnueabi-ld: sound/soc/fsl/fsl_mqs.o: in function
> `fsl_mqs_sm_write':
> > fsl_mqs.c:(.text+0x1aa): undefined reference to
> `scmi_imx_misc_ctrl_set'
> > arm-linux-gnueabi-ld: sound/soc/fsl/fsl_mqs.o: in function
> `fsl_mqs_sm_read':
> > fsl_mqs.c:(.text+0x1ee): undefined reference to
> `scmi_imx_misc_ctrl_get'
> >
> 
> The SCMI drivers, like the newly added IMX_SCMI_MISC_DRV,
> generally make ue of the related vendor protocol like
> IMX_SCMI_MISC_EXT, BUT the SCMI stack is designed in a way that NO
> symbols are needed to be exported by the protocol layer (to avoid a
> huge and growing number of symbols exports)...so usually the current
> DRV-->PROTO dependency is fine.
> 
> In this case, AFAIU, it is the SCMI driver that in turn exports a few
> helpers that are used by another driver fsl_mqs, which in turn could be
> compiled and work with or without the SCMI stack, so with this patch
> we are artificially reversing the DRV<--PROTO dependency to solve this
> scenario in all the compillation scenarios...
> 
> ....BUT given that the IMX_SCMI_MISC_DRV is the one that should
> export the missing symbols could NOT this solved in a cleaner way,
> without adding the fake reverse dependency, by instead modifying the
> header of the driver with something like the classic:
> 
> --->8-----
> diff --git a/include/linux/firmware/imx/sm.h
> b/include/linux/firmware/imx/sm.h index
> 9b85a3f028d1..3a7a3ec367c5 100644
> --- a/include/linux/firmware/imx/sm.h
> +++ b/include/linux/firmware/imx/sm.h
> @@ -17,7 +17,19 @@
>  #define SCMI_IMX_CTRL_SAI4_MCLK                4       /* WAKE SAI4
> MCLK */
>  #define SCMI_IMX_CTRL_SAI5_MCLK                5       /* WAKE SAI5
> MCLK */
> 
> +#ifdef IMX_SCMI_MISC_DRV
>  int scmi_imx_misc_ctrl_get(u32 id, u32 *num, u32 *val);  int
> scmi_imx_misc_ctrl_set(u32 id, u32 val);
> +#else
> +static inline int scmi_imx_misc_ctrl_get(u32 id, u32 *num, u32 *val) {
> +       return 0;
> +}
> +
> +static inline int scmi_imx_misc_ctrl_set(u32 id, u32 val) {
> +       return 0;
> +}
> +#endif
> 

This change is good to me. With just a difference that in downstream
we use -EOPNOTSUPP

https://github.com/nxp-imx/linux-imx/blob/lf-6.6.y/include/linux/firmware/imx/sm.h#L19

Thanks,
Peng.

>  #endif
> ----->8-----------
> 
> ....to just support compilation in all the scenarios.
> 
> > This however only works after changing the dependency in the
> > SND_SOC_FSL_MQS driver as well, which uses 'select
> IMX_SCMI_MISC_DRV'
> > to turn on a driver it depends on. This is generally a bad idea, so
> > the best solution is to change that into a dependency.
> >
> > To allow the ASoC driver to keep building with the SCMI support, this
> > needs to be an optional dependency that enforces the link-time
> > dependency if IMX_SCMI_MISC_DRV is a loadable module but not
> depend on
> > it if that is disabled.
> >
> 
> ...and maybe with the above additions you could avoid also these other
> dep changes...
> 
> ...not sure if I am missing something and I have definitely not tested
> any of my babbling above...
> 
> Thanks,
> Cristian
Peng Fan Nov. 17, 2024, 11:05 a.m. UTC | #5
> Subject: Re: [PATCH] firmware: arm_scmi: fix i.MX build dependency
> 
> On Sun, Nov 17, 2024, at 11:03, Cristian Marussi wrote:
> > On Sat, Nov 16, 2024 at 12:05:18AM +0100, Arnd Bergmann wrote:
> >>
> >> arm-linux-gnueabi-ld: sound/soc/fsl/fsl_mqs.o: in function
> `fsl_mqs_sm_write':
> >> fsl_mqs.c:(.text+0x1aa): undefined reference to
> `scmi_imx_misc_ctrl_set'
> >> arm-linux-gnueabi-ld: sound/soc/fsl/fsl_mqs.o: in function
> `fsl_mqs_sm_read':
> >> fsl_mqs.c:(.text+0x1ee): undefined reference to
> `scmi_imx_misc_ctrl_get'
> >>
> >
> > The SCMI drivers, like the newly added IMX_SCMI_MISC_DRV,
> generally
> > make ue of the related vendor protocol like IMX_SCMI_MISC_EXT,
> BUT the
> > SCMI stack is designed in a way that NO symbols are needed to be
> > exported by the protocol layer (to avoid a huge and growing number
> of
> > symbols exports)...so usually the current DRV-->PROTO dependency is
> fine.
> >
> > In this case, AFAIU, it is the SCMI driver that in turn exports a few
> > helpers that are used by another driver fsl_mqs, which in turn could
> > be compiled and work with or without the SCMI stack, so with this
> > patch we are artificially reversing the DRV<--PROTO dependency to
> > solve this scenario in all the compillation scenarios...
> >
> > ....BUT given that the IMX_SCMI_MISC_DRV is the one that should
> export
> > the missing symbols could NOT this solved in a cleaner way, without
> > adding the fake reverse dependency, by instead modifying the header
> of
> > the driver with something like the classic:
> 
> > --->8-----
> > diff --git a/include/linux/firmware/imx/sm.h
> > b/include/linux/firmware/imx/sm.h index
> 9b85a3f028d1..3a7a3ec367c5
> > 100644
> > --- a/include/linux/firmware/imx/sm.h
> > +++ b/include/linux/firmware/imx/sm.h
> > @@ -17,7 +17,19 @@
> >  #define SCMI_IMX_CTRL_SAI4_MCLK                4       /* WAKE SAI4
> MCLK */
> >  #define SCMI_IMX_CTRL_SAI5_MCLK                5       /* WAKE SAI5
> MCLK */
> >
> > +#ifdef IMX_SCMI_MISC_DRV
> >  int scmi_imx_misc_ctrl_get(u32 id, u32 *num, u32 *val);  int
> > scmi_imx_misc_ctrl_set(u32 id, u32 val);
> > +#else
> > +static inline int scmi_imx_misc_ctrl_get(u32 id, u32 *num, u32 *val)
> > +{
> > +       return 0;
> > +}
> > +
> > +static inline int scmi_imx_misc_ctrl_set(u32 id, u32 val) {
> > +       return 0;
> > +}
> > +#endif
> 
> This usually doesn't work if the provider of these interfaces can be in a
> loadable module. The #ifdef above means this won't be usable when
> CONFIG_IMX_SCMI_MISC_DRV=m, while changing it to
> IS_ENABLED(CONFIG_IMX_SCMI_MISC_DRV) still produces a link error
> when the consumer is built-in. Changing it to IS_REACHABLE() in turn is
> even worse because it avoids the link failure but makes it silently do
> the wrong thing in some configurations.
> 
> >  #endif
> > ----->8-----------
> >
> > ....to just support compilation in all the scenarios.
> >
> >> This however only works after changing the dependency in the
> >> SND_SOC_FSL_MQS driver as well, which uses 'select
> IMX_SCMI_MISC_DRV'
> >> to turn on a driver it depends on. This is generally a bad idea, so
> >> the best solution is to change that into a dependency.
> >>
> >> To allow the ASoC driver to keep building with the SCMI support,
> this
> >> needs to be an optional dependency that enforces the link-time
> >> dependency if IMX_SCMI_MISC_DRV is a loadable module but not
> depend
> >> on it if that is disabled.
> >>
> >
> > ...and maybe with the above additions you could avoid also these
> other
> > dep changes...
> >
> > ...not sure if I am missing something and I have definitely not tested
> > any of my babbling above...
> 
> In my experience, there is no way to avoid reflecting the dependencies
> correctly in Kconfig: if one driver has an EXPORT_SYMBOL that gets
> picked up by another driver, you need a matching 'depends on'.

Oh. Thanks for sharing the knowledge. I am ok with your change.

Thanks,
Peng.

> 
>        Arnd
Mark Brown Nov. 18, 2024, 10:25 a.m. UTC | #6
On Sat, Nov 16, 2024 at 12:05:18AM +0100, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> The newly added SCMI vendor driver references functions in the
> protocol driver but needs a Kconfig dependency to ensure it can link,
> essentially the Kconfig dependency needs to be reversed to match the
> link time dependency:

Acked-by: Mark Brown <broonie@kernel.org>
Shengjiu Wang Nov. 20, 2024, 3:45 a.m. UTC | #7
On Sat, Nov 16, 2024 at 7:08 AM Arnd Bergmann <arnd@kernel.org> wrote:
>
> From: Arnd Bergmann <arnd@arndb.de>
>
> The newly added SCMI vendor driver references functions in the
> protocol driver but needs a Kconfig dependency to ensure it can link,
> essentially the Kconfig dependency needs to be reversed to match the
> link time dependency:
>
> arm-linux-gnueabi-ld: sound/soc/fsl/fsl_mqs.o: in function `fsl_mqs_sm_write':
> fsl_mqs.c:(.text+0x1aa): undefined reference to `scmi_imx_misc_ctrl_set'
> arm-linux-gnueabi-ld: sound/soc/fsl/fsl_mqs.o: in function `fsl_mqs_sm_read':
> fsl_mqs.c:(.text+0x1ee): undefined reference to `scmi_imx_misc_ctrl_get'
>
> This however only works after changing the dependency in the SND_SOC_FSL_MQS
> driver as well, which uses 'select IMX_SCMI_MISC_DRV' to turn on a
> driver it depends on. This is generally a bad idea, so the best solution
> is to change that into a dependency.
>
> To allow the ASoC driver to keep building with the SCMI support, this
> needs to be an optional dependency that enforces the link-time
> dependency if IMX_SCMI_MISC_DRV is a loadable module but not
> depend on it if that is disabled.
>
> Fixes: 61c9f03e22fc ("firmware: arm_scmi: Add initial support for i.MX MISC protocol")
> Fixes: 101c9023594a ("ASoC: fsl_mqs: Support accessing registers by scmi interface")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

Acked-by: Shengjiu Wang <shengjiu.wang@gmail.com>

Best regards
Shengjiu Wang
> ---
> I'm not sure this is the version we want to go with in the end, without
> having read the code in detail, but the version in linux-next is clearly
> wrong in multiple ways and this is the best I could come up with so far.
>
> If we can agree on this approach, it needs to go through the ASoC tree
> as that contains the 101c9023594a commit at the moment.
> ---
>  drivers/firmware/arm_scmi/vendors/imx/Kconfig | 1 +
>  drivers/firmware/imx/Kconfig                  | 1 -
>  sound/soc/fsl/Kconfig                         | 2 +-
>  3 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/firmware/arm_scmi/vendors/imx/Kconfig b/drivers/firmware/arm_scmi/vendors/imx/Kconfig
> index 2883ed24a84d..a01bf5e47301 100644
> --- a/drivers/firmware/arm_scmi/vendors/imx/Kconfig
> +++ b/drivers/firmware/arm_scmi/vendors/imx/Kconfig
> @@ -15,6 +15,7 @@ config IMX_SCMI_BBM_EXT
>  config IMX_SCMI_MISC_EXT
>         tristate "i.MX SCMI MISC EXTENSION"
>         depends on ARM_SCMI_PROTOCOL || (COMPILE_TEST && OF)
> +       depends on IMX_SCMI_MISC_DRV
>         default y if ARCH_MXC
>         help
>           This enables i.MX System MISC control logic such as gpio expander
> diff --git a/drivers/firmware/imx/Kconfig b/drivers/firmware/imx/Kconfig
> index 477d3f32d99a..907cd149c40a 100644
> --- a/drivers/firmware/imx/Kconfig
> +++ b/drivers/firmware/imx/Kconfig
> @@ -25,7 +25,6 @@ config IMX_SCU
>
>  config IMX_SCMI_MISC_DRV
>         tristate "IMX SCMI MISC Protocol driver"
> -       depends on IMX_SCMI_MISC_EXT || COMPILE_TEST
>         default y if ARCH_MXC
>         help
>           The System Controller Management Interface firmware (SCMI FW) is
> diff --git a/sound/soc/fsl/Kconfig b/sound/soc/fsl/Kconfig
> index 8e88830e8e57..678540b78280 100644
> --- a/sound/soc/fsl/Kconfig
> +++ b/sound/soc/fsl/Kconfig
> @@ -29,8 +29,8 @@ config SND_SOC_FSL_SAI
>  config SND_SOC_FSL_MQS
>         tristate "Medium Quality Sound (MQS) module support"
>         depends on SND_SOC_FSL_SAI
> +       depends on IMX_SCMI_MISC_DRV || !IMX_SCMI_MISC_DRV
>         select REGMAP_MMIO
> -       select IMX_SCMI_MISC_DRV if IMX_SCMI_MISC_EXT !=n
>         help
>           Say Y if you want to add Medium Quality Sound (MQS)
>           support for the Freescale CPUs.
> --
> 2.39.5
>
>
Sudeep Holla Nov. 29, 2024, 10:38 a.m. UTC | #8
On Sat, Nov 16, 2024 at 12:05:18AM +0100, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
>
> The newly added SCMI vendor driver references functions in the
> protocol driver but needs a Kconfig dependency to ensure it can link,
> essentially the Kconfig dependency needs to be reversed to match the
> link time dependency:
>
> arm-linux-gnueabi-ld: sound/soc/fsl/fsl_mqs.o: in function `fsl_mqs_sm_write':
> fsl_mqs.c:(.text+0x1aa): undefined reference to `scmi_imx_misc_ctrl_set'
> arm-linux-gnueabi-ld: sound/soc/fsl/fsl_mqs.o: in function `fsl_mqs_sm_read':
> fsl_mqs.c:(.text+0x1ee): undefined reference to `scmi_imx_misc_ctrl_get'
>
> This however only works after changing the dependency in the SND_SOC_FSL_MQS
> driver as well, which uses 'select IMX_SCMI_MISC_DRV' to turn on a
> driver it depends on. This is generally a bad idea, so the best solution
> is to change that into a dependency.
>
> To allow the ASoC driver to keep building with the SCMI support, this
> needs to be an optional dependency that enforces the link-time
> dependency if IMX_SCMI_MISC_DRV is a loadable module but not
> depend on it if that is disabled.
>
> Fixes: 61c9f03e22fc ("firmware: arm_scmi: Add initial support for i.MX MISC protocol")
> Fixes: 101c9023594a ("ASoC: fsl_mqs: Support accessing registers by scmi interface")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> I'm not sure this is the version we want to go with in the end, without
> having read the code in detail, but the version in linux-next is clearly
> wrong in multiple ways and this is the best I could come up with so far.
>
> If we can agree on this approach, it needs to go through the ASoC tree
> as that contains the 101c9023594a commit at the moment.

Sorry for the late response, was away for a while. I am fine with the
change, but bit confused as Mark Brown has acked the change instead by
which I assume he is not taking it through his tree.

I can can take it at -rc1 or if Mark B is happy to pull it, here is my ack.

Acked-by: Sudeep Holla <sudeep.holla@arm.com>

--
Regards,
Sudeep
diff mbox series

Patch

diff --git a/drivers/firmware/arm_scmi/vendors/imx/Kconfig b/drivers/firmware/arm_scmi/vendors/imx/Kconfig
index 2883ed24a84d..a01bf5e47301 100644
--- a/drivers/firmware/arm_scmi/vendors/imx/Kconfig
+++ b/drivers/firmware/arm_scmi/vendors/imx/Kconfig
@@ -15,6 +15,7 @@  config IMX_SCMI_BBM_EXT
 config IMX_SCMI_MISC_EXT
 	tristate "i.MX SCMI MISC EXTENSION"
 	depends on ARM_SCMI_PROTOCOL || (COMPILE_TEST && OF)
+	depends on IMX_SCMI_MISC_DRV
 	default y if ARCH_MXC
 	help
 	  This enables i.MX System MISC control logic such as gpio expander
diff --git a/drivers/firmware/imx/Kconfig b/drivers/firmware/imx/Kconfig
index 477d3f32d99a..907cd149c40a 100644
--- a/drivers/firmware/imx/Kconfig
+++ b/drivers/firmware/imx/Kconfig
@@ -25,7 +25,6 @@  config IMX_SCU
 
 config IMX_SCMI_MISC_DRV
 	tristate "IMX SCMI MISC Protocol driver"
-	depends on IMX_SCMI_MISC_EXT || COMPILE_TEST
 	default y if ARCH_MXC
 	help
 	  The System Controller Management Interface firmware (SCMI FW) is
diff --git a/sound/soc/fsl/Kconfig b/sound/soc/fsl/Kconfig
index 8e88830e8e57..678540b78280 100644
--- a/sound/soc/fsl/Kconfig
+++ b/sound/soc/fsl/Kconfig
@@ -29,8 +29,8 @@  config SND_SOC_FSL_SAI
 config SND_SOC_FSL_MQS
 	tristate "Medium Quality Sound (MQS) module support"
 	depends on SND_SOC_FSL_SAI
+	depends on IMX_SCMI_MISC_DRV || !IMX_SCMI_MISC_DRV
 	select REGMAP_MMIO
-	select IMX_SCMI_MISC_DRV if IMX_SCMI_MISC_EXT !=n
 	help
 	  Say Y if you want to add Medium Quality Sound (MQS)
 	  support for the Freescale CPUs.