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

[Geert Uytterhoeven <geert@linux-m68k.org>]

Hi Benjamin,

On Thu, Jul 6, 2023 at 8:14 AM Benjamin Gray <bgray@linux.ibm.com> wrote:
> On Thu, 2023-07-06 at 05:13 +0000, Christophe Leroy wrote:
> > Le 05/07/2023 à 02:30, Benjamin Gray a écrit :
> > > 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 <bgray@linux.ibm.com>
> > > ---
> > >   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