Re: [PATCH] rtc: Kconfig: select REGMAP for RTC_DRV_DS1307

Hi Benjamin,

On Thu, Jul 6, 2023 at 8:14 AM Benjamin Gray [off-list ref] wrote:

On Thu, 2023-07-06 at 05:13 +0000, Christophe Leroy wrote:

quoted

Le 05/07/2023 à 02:30, Benjamin Gray a écrit :

quoted

The drivers/rtc/rtc-ds1307.c driver has a direct dependency on
struct regmap_config, which is guarded behind CONFIG_REGMAP.

Commit 70a640c0efa7 ("regmap: REGMAP_KUNIT should not select
REGMAP")
exposed this by disabling the default pick unless KUNIT_ALL_TESTS
is
set, causing the ppc64be allnoconfig build to fail.

Signed-off-by: Benjamin Gray <redacted>
---
  drivers/rtc/Kconfig | 1 +
  1 file changed, 1 insertion(+)

diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
index ffca9a8bb878..7455ebd189fe 100644
--- a/drivers/rtc/Kconfig
+++ b/drivers/rtc/Kconfig

@@ -246,6 +246,7 @@ config RTC_DRV_AS3722

  config RTC_DRV_DS1307
        tristate "Dallas/Maxim DS1307/37/38/39/40/41, ST M41T00,

EPSON RX-8025, ISL12057"
+       select REGMAP

As far as I can see, REGMAP defaults to Y when REGMAP_I2C is
selected.
Can you explain more in details why you have to select it explicitely
?
If there is something wrong with the logic, then the logic should be
fixed instead of just adding a selection of REGMAP for that
particular
RTC_DRV_DS1307. Because others like RTC_DRV_ABB5ZES3 or
RTC_DRV_ABEOZ9
might have the exact same problem.

Right, yeah, I don't want to assert this patch is the correct solution,
sending it was more to offer a fix and allow discussion if it should be
resolved some other way (so thanks for replying, I appreciate it).

In terms of why I made this patch, the way I see it, if a config option
requires another config option be set, then "selects" is the natural
way of phrasing this dependency. "default" on the REGMAP side seems
weird. If it's a default, does that mean it can be overridden? But
RTC_DRV_DS1307 *requires* REGMAP; it's not just a "would be nice". The
build breaks without it.

But maybe KConfig works differently to my assumptions. Maybe the
referenced patch that causes the build failure is actually incorrect
(CC Geert). I spoke with Joel Stanley (CC) and he indicated you're not
supposed to depend on REGMAP like KUnit does?

Thanks for CCing me!

Looks like I made a really silly mistake here: my patch not only allows
the user to enable REGMAP manually (for the test), but also to disable
it manually, regardless if there are users or not :-(

I think the proper fix is to replace the "default y if ..." by
"select REGMAP" for all users.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds