[PATCH] power: supply: max8997_charger: make EXTCON dependency unconditional

[PATCH] power: supply: max8997_charger: make EXTCON dependency unconditional

[Arnd Bergmann]

From: Arnd Bergmann <arnd@arndb.de>

Some of the extcon interfaces have a fallback implementation that can
be used when EXTCON is disabled, but some others do not, causing a
build failure:

drivers/power/supply/max8997_charger.c:261:9: error: implicit declaration of function 'devm_extcon_register_notifier_all' [-Werror,-Wimplicit-function-declaration]
                ret = devm_extcon_register_notifier_all(&pdev->dev, charger->edev,
                      ^
drivers/power/supply/max8997_charger.c:261:9: note: did you mean 'devm_extcon_register_notifier'?
include/linux/extcon.h:263:19: note: 'devm_extcon_register_notifier' declared here
static inline int devm_extcon_register_notifier(struct device *dev,

I assume there is no reason to actually build this driver without extcon
support, so a hard dependency is the easiest fix. Alternatively the
header file could be extended to provide additional inline stubs.

Fixes: f384989e88d4 ("power: supply: max8997_charger: Set CHARGER current limit")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/power/supply/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig
index 006b95eca673..6cce17e1d47a 100644
--- a/drivers/power/supply/Kconfig
+++ b/drivers/power/supply/Kconfig
@@ -555,7 +555,7 @@ config CHARGER_MAX77693
 config CHARGER_MAX8997
 	tristate "Maxim MAX8997/MAX8966 PMIC battery charger driver"
 	depends on MFD_MAX8997 && REGULATOR_MAX8997
-	depends on EXTCON || !EXTCON
+	depends on EXTCON
 	help
 	  Say Y to enable support for the battery charger control sysfs and
 	  platform data of MAX8997/LP3974 PMICs.
-- 
2.29.2

Re: [PATCH] power: supply: max8997_charger: make EXTCON dependency unconditional

[Krzysztof Kozlowski]

On 08/03/2021 16:29, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> Some of the extcon interfaces have a fallback implementation that can
> be used when EXTCON is disabled, but some others do not, causing a
> build failure:
> 
> drivers/power/supply/max8997_charger.c:261:9: error: implicit declaration of function 'devm_extcon_register_notifier_all' [-Werror,-Wimplicit-function-declaration]
>                 ret = devm_extcon_register_notifier_all(&pdev->dev, charger->edev,
>                       ^
> drivers/power/supply/max8997_charger.c:261:9: note: did you mean 'devm_extcon_register_notifier'?
> include/linux/extcon.h:263:19: note: 'devm_extcon_register_notifier' declared here
> static inline int devm_extcon_register_notifier(struct device *dev,
> 
> I assume there is no reason to actually build this driver without extcon
> support, so a hard dependency is the easiest fix. Alternatively the
> header file could be extended to provide additional inline stubs.

Hi Arnd,

Thanks for the patch but I think I got it covered with:
https://lore.kernel.org/lkml/20210215100610.19911-2-cw00.choi@samsung.com/
(sent via extcon tree).

Did you experience a new/different issue?

Best regards,
Krzysztof

Re: [PATCH] power: supply: max8997_charger: make EXTCON dependency unconditional

[Arnd Bergmann]

On Mon, Mar 8, 2021 at 4:33 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On 08/03/2021 16:29, Arnd Bergmann wrote:
> > From: Arnd Bergmann <arnd@arndb.de>
> >
> > Some of the extcon interfaces have a fallback implementation that can
> > be used when EXTCON is disabled, but some others do not, causing a
> > build failure:
> >
> > drivers/power/supply/max8997_charger.c:261:9: error: implicit declaration of function 'devm_extcon_register_notifier_all' [-Werror,-Wimplicit-function-declaration]
> >                 ret = devm_extcon_register_notifier_all(&pdev->dev, charger->edev,
> >                       ^
> > drivers/power/supply/max8997_charger.c:261:9: note: did you mean 'devm_extcon_register_notifier'?
> > include/linux/extcon.h:263:19: note: 'devm_extcon_register_notifier' declared here
> > static inline int devm_extcon_register_notifier(struct device *dev,
> >
> > I assume there is no reason to actually build this driver without extcon
> > support, so a hard dependency is the easiest fix. Alternatively the
> > header file could be extended to provide additional inline stubs.
>
> Hi Arnd,
>
> Thanks for the patch but I think I got it covered with:
> https://lore.kernel.org/lkml/20210215100610.19911-2-cw00.choi@samsung.com/
> (sent via extcon tree).
>
> Did you experience a new/different issue?

The patch should be fine and address the problem, I just didn't see it was
already fixed in linux-next as I'm still testing on mainline (rc2 at
the moment).

I assume the fix will make it into a future -rc then.

      Arnd

Re: [PATCH] power: supply: max8997_charger: make EXTCON dependency unconditional

[Krzysztof Kozlowski]

On Mon, 8 Mar 2021 at 17:02, Arnd Bergmann <arnd@kernel.org> wrote:
>
> On Mon, Mar 8, 2021 at 4:33 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> >
> > On 08/03/2021 16:29, Arnd Bergmann wrote:
> > > From: Arnd Bergmann <arnd@arndb.de>
> > >
> > > Some of the extcon interfaces have a fallback implementation that can
> > > be used when EXTCON is disabled, but some others do not, causing a
> > > build failure:
> > >
> > > drivers/power/supply/max8997_charger.c:261:9: error: implicit declaration of function 'devm_extcon_register_notifier_all' [-Werror,-Wimplicit-function-declaration]
> > >                 ret = devm_extcon_register_notifier_all(&pdev->dev, charger->edev,
> > >                       ^
> > > drivers/power/supply/max8997_charger.c:261:9: note: did you mean 'devm_extcon_register_notifier'?
> > > include/linux/extcon.h:263:19: note: 'devm_extcon_register_notifier' declared here
> > > static inline int devm_extcon_register_notifier(struct device *dev,
> > >
> > > I assume there is no reason to actually build this driver without extcon
> > > support, so a hard dependency is the easiest fix. Alternatively the
> > > header file could be extended to provide additional inline stubs.
> >
> > Hi Arnd,
> >
> > Thanks for the patch but I think I got it covered with:
> > https://lore.kernel.org/lkml/20210215100610.19911-2-cw00.choi@samsung.com/
> > (sent via extcon tree).
> >
> > Did you experience a new/different issue?
>
> The patch should be fine and address the problem, I just didn't see it was
> already fixed in linux-next as I'm still testing on mainline (rc2 at
> the moment).
>
> I assume the fix will make it into a future -rc then.

It's still only in linux-next via extcon tree, so it seems Greg did
not take it yet.

Chanwoo,
You might need to follow up on this, so your pull request won't get lost.

Best regards,
Krzysztof

Re: [PATCH] power: supply: max8997_charger: make EXTCON dependency unconditional

[Chanwoo Choi]

Hi Krzysztof,

On 3/9/21 1:11 AM, Krzysztof Kozlowski wrote:
> On Mon, 8 Mar 2021 at 17:02, Arnd Bergmann <arnd@kernel.org> wrote:
>>
>> On Mon, Mar 8, 2021 at 4:33 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>>>
>>> On 08/03/2021 16:29, Arnd Bergmann wrote:
>>>> From: Arnd Bergmann <arnd@arndb.de>
>>>>
>>>> Some of the extcon interfaces have a fallback implementation that can
>>>> be used when EXTCON is disabled, but some others do not, causing a
>>>> build failure:
>>>>
>>>> drivers/power/supply/max8997_charger.c:261:9: error: implicit declaration of function 'devm_extcon_register_notifier_all' [-Werror,-Wimplicit-function-declaration]
>>>>                 ret = devm_extcon_register_notifier_all(&pdev->dev, charger->edev,
>>>>                       ^
>>>> drivers/power/supply/max8997_charger.c:261:9: note: did you mean 'devm_extcon_register_notifier'?
>>>> include/linux/extcon.h:263:19: note: 'devm_extcon_register_notifier' declared here
>>>> static inline int devm_extcon_register_notifier(struct device *dev,
>>>>
>>>> I assume there is no reason to actually build this driver without extcon
>>>> support, so a hard dependency is the easiest fix. Alternatively the
>>>> header file could be extended to provide additional inline stubs.
>>>
>>> Hi Arnd,
>>>
>>> Thanks for the patch but I think I got it covered with:
>>> https://lore.kernel.org/lkml/20210215100610.19911-2-cw00.choi@samsung.com/
>>> (sent via extcon tree).
>>>
>>> Did you experience a new/different issue?
>>
>> The patch should be fine and address the problem, I just didn't see it was
>> already fixed in linux-next as I'm still testing on mainline (rc2 at
>> the moment).
>>
>> I assume the fix will make it into a future -rc then.
> 
> It's still only in linux-next via extcon tree, so it seems Greg did
> not take it yet.
> 
> Chanwoo,
> You might need to follow up on this, so your pull request won't get lost.

I'm sorry. Because of my fault, the previous pull request was not merged to v5.12-rc1.
To fix this issue, I'll send the pull request for rc3 to Greg.

Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH] power: supply: max8997_charger: make EXTCON dependency unconditional

[Matti Vaittinen]

Hello Arnd,

On Mon, 2021-03-08 at 16:29 +0100, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> I assume there is no reason to actually build this driver without
> extcon
> support, so a hard dependency is the easiest fix. Alternatively the
> header file could be extended to provide additional inline stubs.

I am absolutely not insisting this but it would be better if there was
no hard dependency. I've tried couple of times to do changes to bunch
of drivers (added some devm-functionality or generic definitions or -
you name it) and I always end up at least compile-testing changes to
multiple drivers. I always repeat following:

1. Manually hack the Makefiles to compile changed drivers as modules

2. Try CONFIG_COMPLILE_TEST
 - unfortunately not too widely supported

3. Manually hack more to get drivers with 'hard dependencies' compiled
- occasionally ending up to commenting out the calls with dependencies.

So, if adding the stub is straightforward I'd vote for it.

But I guess you know this quite well so I am just giving my 10 cents -
decision can be yours :)

Best Regards
	Matti Vaittinen

--
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland
SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND

~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~

Simon says - in Latin please.
"non cogito me" dixit Rene Descarte, deinde evanescavit

(Thanks for the translation Simon)

Re: [PATCH] power: supply: max8997_charger: make EXTCON dependency unconditional

[Arnd Bergmann]

On Mon, Mar 8, 2021 at 4:52 PM Matti Vaittinen
<matti.vaittinen@fi.rohmeurope.com> wrote:
>
> Hello Arnd,
>
> On Mon, 2021-03-08 at 16:29 +0100, Arnd Bergmann wrote:
> > From: Arnd Bergmann <arnd@arndb.de>
> >
> > I assume there is no reason to actually build this driver without
> > extcon
> > support, so a hard dependency is the easiest fix. Alternatively the
> > header file could be extended to provide additional inline stubs.
>
> I am absolutely not insisting this but it would be better if there was
> no hard dependency. I've tried couple of times to do changes to bunch
> of drivers (added some devm-functionality or generic definitions or -
> you name it) and I always end up at least compile-testing changes to
> multiple drivers. I always repeat following:
>
> 1. Manually hack the Makefiles to compile changed drivers as modules
>
> 2. Try CONFIG_COMPLILE_TEST
>  - unfortunately not too widely supported
>
> 3. Manually hack more to get drivers with 'hard dependencies' compiled
> - occasionally ending up to commenting out the calls with dependencies.
>
> So, if adding the stub is straightforward I'd vote for it.
>
> But I guess you know this quite well so I am just giving my 10 cents -
> decision can be yours :)

As Krzysztof said, he's already fixed this in linux-next the way you
prefer. Both approaches are common, and subsystem maintainers
have different preferences. I more or less picked one at random
here.

The main downside of the 'depends on FOO || !FOO' dependency is
that new developers tend to be confused by the syntax. It also
means you don't easily catch the problem with building the driver as
built-in if the dependency is missing, these can go unnoticed for a
long time until someone runs into the wrong randconfig build.

      Arnd

Re: [PATCH] power: supply: max8997_charger: make EXTCON dependency unconditional

[Andy Shevchenko]

On Mon, Mar 8, 2021 at 5:29 PM Arnd Bergmann <arnd@kernel.org> wrote:

> -       depends on EXTCON || !EXTCON

I stumbled over this.
What is the point of having this line at all?
What magic trick does it serve for?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] power: supply: max8997_charger: make EXTCON dependency unconditional

[Andy Shevchenko]

On Mon, Mar 8, 2021 at 6:06 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Mon, Mar 8, 2021 at 5:29 PM Arnd Bergmann <arnd@kernel.org> wrote:
>
> > -       depends on EXTCON || !EXTCON
>
> I stumbled over this.
> What is the point of having this line at all?
> What magic trick does it serve for?

Okay, it seems I can answer my question: it requires extcon to be
built-in when the driver is built-in. I often saw similar written
slightly differently.



-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] power: supply: max8997_charger: make EXTCON dependency unconditional

[Vaittinen, Matti]

On Mon, 2021-03-08 at 18:06 +0200, Andy Shevchenko wrote:
> On Mon, Mar 8, 2021 at 5:29 PM Arnd Bergmann <arnd@kernel.org> wrote:
> 
> > -       depends on EXTCON || !EXTCON
> 
> I stumbled over this.
> What is the point of having this line at all?
> What magic trick does it serve for?

The logic was somewhat hairy. I used it once for BD70528 watchdog which
provides lock functions for RTC - or stubs if WDG is not used. Problem
which that solved was that when RTC was built-in and WDG was a module -
these functions were missing. It might've been Guenter or A. Belloni
who suggested it.

I don't remember 100% sure but I think it might require EXTCON to be
build as a module. I guess the discussion I had regarding BD70528 can
be found from lore.kernel.org - but it is probably buried deep... Maybe
someone will give better and more definite answer.

Best Regards
	Matti Vaittinen