diff mbox series

mmc: renesas_sdhi: add regulator dependency

Message ID 20250329082123.2325267-1-arnd@kernel.org (mailing list archive)
State New
Headers show
Series mmc: renesas_sdhi: add regulator dependency | expand

Commit Message

Arnd Bergmann March 29, 2025, 8:20 a.m. UTC 
From: Arnd Bergmann <arnd@arndb.de>

The driver started using the regulator subsystem and fails to build without
that now:

ERROR: modpost: "rdev_get_drvdata" [drivers/mmc/host/renesas_sdhi_core.ko] undefined!
ERROR: modpost: "devm_regulator_register" [drivers/mmc/host/renesas_sdhi_core.ko] undefined!

Add a Kconfig dependency for this.

The 'select RESET_CONTROLLER' needs to either go away or get changed to a dependency
in order to avoid Kconfig dependency loops here. The 'select' is otherwise used by
drivers that register a reset controller, but normally not by those that just use one.

Fixes: fae80a99dc03 ("mmc: renesas_sdhi: Add support for RZ/G3E SoC")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/mmc/host/Kconfig | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Wolfram Sang March 29, 2025, 9:46 a.m. UTC | #1 
On Sat, Mar 29, 2025 at 09:20:52AM +0100, Arnd Bergmann wrote:

>  config MMC_SDHI
>  	tristate "Renesas SDHI SD/SDIO controller support"
> -	depends on SUPERH || ARCH_RENESAS || COMPILE_TEST
> +	depends on SUPERH || (ARCH_RENESAS && RESET_CONTROLLER) || COMPILE_TEST
> +	depends on REGULATOR

Hmm, this is too strict IMO. SuperH does not need REGULATOR. But adding
REGULATOR to ARCH_RENESAS like RESET_CONTROLLER will still fail for
COMPILE_TEST, right?

Maybe we need to maintain the dependency in Kconfig after all:

	depends on REGULATOR if ARCH_R8A<whichever needs it>
?
Arnd Bergmann March 29, 2025, 10:46 a.m. UTC | #2 
On Sat, Mar 29, 2025, at 10:46, Wolfram Sang wrote:
> On Sat, Mar 29, 2025 at 09:20:52AM +0100, Arnd Bergmann wrote:
>
>>  config MMC_SDHI
>>  	tristate "Renesas SDHI SD/SDIO controller support"
>> -	depends on SUPERH || ARCH_RENESAS || COMPILE_TEST
>> +	depends on SUPERH || (ARCH_RENESAS && RESET_CONTROLLER) || COMPILE_TEST
>> +	depends on REGULATOR
>
> Hmm, this is too strict IMO. SuperH does not need REGULATOR.

I haven't tried building on sh, but I don't see why it wouldn't
need the regulator dependency. The code that calls it is

        rcfg.of_node = of_get_child_by_name(dev->of_node, "vqmmc-regulator");
        if (!of_device_is_available(rcfg.of_node)) {
                of_node_put(rcfg.of_node);
                rcfg.of_node = NULL;
        }

        if (rcfg.of_node) {
                rcfg.driver_data = priv->host;
                rdev = devm_regulator_register(dev, &renesas_sdhi_vqmmc_regulator, &rcfg);
        ...

which sounds like regulators are always needed when
of_get_child_by_name() may return a non-NULL pointer, i.e.
when CONFIG_OF is enabled.

If this is correct, maybe this is the best variant:

config MMC_SDHI
  	tristate "Renesas SDHI SD/SDIO controller support"
	depends on SUPERH || ARCH_RENESAS || COMPILE_TEST
        depends on (REGULATOR && RESET_CONTROLLER) || !OF

CONFIG_ARCH_RENESAS is only set when CONFIG_OF is also set,
so both subsystem dependencies are covered by that, while
SUPERH doesn't currently enable OF, but will need the
regulator and reset controller if that patch is ever merged.

      Arnd
Wolfram Sang March 29, 2025, 11:46 a.m. UTC | #3 
On Sat, Mar 29, 2025 at 11:46:10AM +0100, Arnd Bergmann wrote:
> On Sat, Mar 29, 2025, at 10:46, Wolfram Sang wrote:
> > On Sat, Mar 29, 2025 at 09:20:52AM +0100, Arnd Bergmann wrote:
> >
> >>  config MMC_SDHI
> >>  	tristate "Renesas SDHI SD/SDIO controller support"
> >> -	depends on SUPERH || ARCH_RENESAS || COMPILE_TEST
> >> +	depends on SUPERH || (ARCH_RENESAS && RESET_CONTROLLER) || COMPILE_TEST
> >> +	depends on REGULATOR
> >
> > Hmm, this is too strict IMO. SuperH does not need REGULATOR.
> 
> I haven't tried building on sh, but I don't see why it wouldn't
> need the regulator dependency. The code that calls it is
> 
>         rcfg.of_node = of_get_child_by_name(dev->of_node, "vqmmc-regulator");
>         if (!of_device_is_available(rcfg.of_node)) {
>                 of_node_put(rcfg.of_node);
>                 rcfg.of_node = NULL;
>         }
> 
>         if (rcfg.of_node) {
>                 rcfg.driver_data = priv->host;
>                 rdev = devm_regulator_register(dev, &renesas_sdhi_vqmmc_regulator, &rcfg);
>         ...
> 
> which sounds like regulators are always needed when
> of_get_child_by_name() may return a non-NULL pointer, i.e.
> when CONFIG_OF is enabled.

rcfg.of_node will be NULLed when the regulator device is not available.
Which is only true for a few SoC variants. Most other SoCs use
'vmmc-supply' insteand of 'vqmmc-regulator'.

> If this is correct, maybe this is the best variant:
> 
> config MMC_SDHI
>   	tristate "Renesas SDHI SD/SDIO controller support"
> 	depends on SUPERH || ARCH_RENESAS || COMPILE_TEST
>         depends on (REGULATOR && RESET_CONTROLLER) || !OF
> 
> CONFIG_ARCH_RENESAS is only set when CONFIG_OF is also set,
> so both subsystem dependencies are covered by that, while
> SUPERH doesn't currently enable OF, but will need the
> regulator and reset controller if that patch is ever merged.

Even given the above, I like this approach and think it still works.
But Biju is the SDHI regulator expert here.
Biju Das March 29, 2025, 12:49 p.m. UTC | #4 
Hi All,

> -----Original Message-----
> From: Wolfram Sang <wsa+renesas@sang-engineering.com>
> Sent: 29 March 2025 11:47
> Subject: Re: [PATCH] mmc: renesas_sdhi: add regulator dependency
> 
> On Sat, Mar 29, 2025 at 11:46:10AM +0100, Arnd Bergmann wrote:
> > On Sat, Mar 29, 2025, at 10:46, Wolfram Sang wrote:
> > > On Sat, Mar 29, 2025 at 09:20:52AM +0100, Arnd Bergmann wrote:
> > >
> > >>  config MMC_SDHI
> > >>  	tristate "Renesas SDHI SD/SDIO controller support"
> > >> -	depends on SUPERH || ARCH_RENESAS || COMPILE_TEST
> > >> +	depends on SUPERH || (ARCH_RENESAS && RESET_CONTROLLER) || COMPILE_TEST
> > >> +	depends on REGULATOR
> > >
> > > Hmm, this is too strict IMO. SuperH does not need REGULATOR.
> >
> > I haven't tried building on sh, but I don't see why it wouldn't need
> > the regulator dependency. The code that calls it is
> >
> >         rcfg.of_node = of_get_child_by_name(dev->of_node, "vqmmc-regulator");
> >         if (!of_device_is_available(rcfg.of_node)) {
> >                 of_node_put(rcfg.of_node);
> >                 rcfg.of_node = NULL;
> >         }
> >
> >         if (rcfg.of_node) {
> >                 rcfg.driver_data = priv->host;
> >                 rdev = devm_regulator_register(dev, &renesas_sdhi_vqmmc_regulator, &rcfg);
> >         ...
> >
> > which sounds like regulators are always needed when
> > of_get_child_by_name() may return a non-NULL pointer, i.e.
> > when CONFIG_OF is enabled.
> 
> rcfg.of_node will be NULLed when the regulator device is not available.
> Which is only true for a few SoC variants. Most other SoCs use 'vmmc-supply' instean
d of 'vqmmc-
> regulator'.

Yes, Only few SoCs has internal regulator support and is based on
of_device_is_available().


> 
> > If this is correct, maybe this is the best variant:
> >
> > config MMC_SDHI
> >   	tristate "Renesas SDHI SD/SDIO controller support"
> > 	depends on SUPERH || ARCH_RENESAS || COMPILE_TEST
> >         depends on (REGULATOR && RESET_CONTROLLER) || !OF
> >
> > CONFIG_ARCH_RENESAS is only set when CONFIG_OF is also set, so both
> > subsystem dependencies are covered by that, while SUPERH doesn't
> > currently enable OF, but will need the regulator and reset controller
> > if that patch is ever merged.
> 
> Even given the above, I like this approach and think it still works.
> But Biju is the SDHI regulator expert here.

Tested-by: Biju Das <biju.das.jz@bp.renesas.com>

Cheers,
Biju
diff mbox series

Patch

diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
index d7c90a9630e9..c0f03367d765 100644
--- a/drivers/mmc/host/Kconfig
+++ b/drivers/mmc/host/Kconfig
@@ -692,9 +692,9 @@  config MMC_TMIO_CORE
 
 config MMC_SDHI
 	tristate "Renesas SDHI SD/SDIO controller support"
-	depends on SUPERH || ARCH_RENESAS || COMPILE_TEST
+	depends on SUPERH || (ARCH_RENESAS && RESET_CONTROLLER) || COMPILE_TEST
+	depends on REGULATOR
 	select MMC_TMIO_CORE
-	select RESET_CONTROLLER if ARCH_RENESAS
 	help
 	  This provides support for the SDHI SD/SDIO controller found in
 	  Renesas SuperH, ARM and ARM64 based SoCs