* [PATCH] rtc: Kconfig: select REGMAP for RTC_DRV_DS1307
@ 2023-07-05 0:30 Benjamin Gray
2023-07-06 5:13 ` Christophe Leroy
0 siblings, 1 reply; 6+ messages in thread
From: Benjamin Gray @ 2023-07-05 0:30 UTC (permalink / raw)
To: linux-rtc; +Cc: linuxppc-dev, Benjamin Gray
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
select REGMAP_I2C
select WATCHDOG_CORE if WATCHDOG
help
--
2.41.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] rtc: Kconfig: select REGMAP for RTC_DRV_DS1307
2023-07-05 0:30 [PATCH] rtc: Kconfig: select REGMAP for RTC_DRV_DS1307 Benjamin Gray
@ 2023-07-06 5:13 ` Christophe Leroy
2023-07-06 6:14 ` Benjamin Gray
0 siblings, 1 reply; 6+ messages in thread
From: Christophe Leroy @ 2023-07-06 5:13 UTC (permalink / raw)
To: Benjamin Gray, linux-rtc@vger.kernel.org; +Cc: linuxppc-dev@lists.ozlabs.org
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.
> select REGMAP_I2C
> select WATCHDOG_CORE if WATCHDOG
> help
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] rtc: Kconfig: select REGMAP for RTC_DRV_DS1307
2023-07-06 5:13 ` Christophe Leroy
@ 2023-07-06 6:14 ` Benjamin Gray
2023-07-06 6:35 ` Christophe Leroy
2023-07-06 7:50 ` Geert Uytterhoeven
0 siblings, 2 replies; 6+ messages in thread
From: Benjamin Gray @ 2023-07-06 6:14 UTC (permalink / raw)
To: Christophe Leroy, linux-rtc@vger.kernel.org
Cc: Geert Uytterhoeven, linuxppc-dev@lists.ozlabs.org, Joel Stanley
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?
As for other drivers having the same problem, quite possibly, yes. But
the PPC configs don't seem to build those drivers, so I'd prefer to
leave it to anyone who does build them to report the error. I wasn't
planning to audit all the drivers, I was just trying to fix the PPC
configs I build and test.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] rtc: Kconfig: select REGMAP for RTC_DRV_DS1307
2023-07-06 6:14 ` Benjamin Gray
@ 2023-07-06 6:35 ` Christophe Leroy
2023-07-06 7:50 ` Geert Uytterhoeven
1 sibling, 0 replies; 6+ messages in thread
From: Christophe Leroy @ 2023-07-06 6:35 UTC (permalink / raw)
To: Benjamin Gray, linux-rtc@vger.kernel.org
Cc: Geert Uytterhoeven, linuxppc-dev@lists.ozlabs.org, Joel Stanley
Le 06/07/2023 à 08:14, Benjamin Gray a écrit :
> 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.
I mean't "why doesn't it work as is", and/or "why didn't you fix what
doesn't work".
Well, until commit 70a640c0efa7 ("regmap: REGMAP_KUNIT should not select
REGMAP") it was not user-selectable so I couldn't be overridden.
After commit 70a640c0efa7 ("regmap: REGMAP_KUNIT should not select
REGMAP") it is overridable so we have an additional problem.
Does RTC_DRV_DS1307 require REGMAP or does RTC_DRV_DS1307 require
REGMAP_I2C and then REGMAP_I2C requires REGMAP ?
I think that huge default like for REGMAP should be replaced by
individual selection of REGMAP from each of those config items. For
exemple REGMAP_I2C should select REGMAP then I guess it would also solve
your issue wouldn't 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?
>
> As for other drivers having the same problem, quite possibly, yes. But
> the PPC configs don't seem to build those drivers, so I'd prefer to
> leave it to anyone who does build them to report the error. I wasn't
> planning to audit all the drivers, I was just trying to fix the PPC
> configs I build and test.
Well ok but that's probably the indication of a deeper problem which
deserves a more generic fix.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] rtc: Kconfig: select REGMAP for RTC_DRV_DS1307
2023-07-06 6:14 ` Benjamin Gray
2023-07-06 6:35 ` Christophe Leroy
@ 2023-07-06 7:50 ` Geert Uytterhoeven
2023-07-06 11:43 ` Geert Uytterhoeven
1 sibling, 1 reply; 6+ messages in thread
From: Geert Uytterhoeven @ 2023-07-06 7:50 UTC (permalink / raw)
To: Benjamin Gray
Cc: linux-rtc@vger.kernel.org, Arnd Bergmann,
linuxppc-dev@lists.ozlabs.org, Joel Stanley
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] rtc: Kconfig: select REGMAP for RTC_DRV_DS1307
2023-07-06 7:50 ` Geert Uytterhoeven
@ 2023-07-06 11:43 ` Geert Uytterhoeven
0 siblings, 0 replies; 6+ messages in thread
From: Geert Uytterhoeven @ 2023-07-06 11:43 UTC (permalink / raw)
To: Benjamin Gray
Cc: linux-rtc@vger.kernel.org, Arnd Bergmann,
linuxppc-dev@lists.ozlabs.org, Joel Stanley
On Thu, Jul 6, 2023 at 9:50 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> 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.
I have sent a patch to do so, followed by a few related fixes
https://lore.kernel.org/r/525c37a568b10623ffb2d108850afd7e37f9350e.1688643442.git.geert@linux-m68k.org
Thanks!
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-07-06 11:44 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-07-05 0:30 [PATCH] rtc: Kconfig: select REGMAP for RTC_DRV_DS1307 Benjamin Gray
2023-07-06 5:13 ` Christophe Leroy
2023-07-06 6:14 ` Benjamin Gray
2023-07-06 6:35 ` Christophe Leroy
2023-07-06 7:50 ` Geert Uytterhoeven
2023-07-06 11:43 ` Geert Uytterhoeven
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).