| Message ID | 20210308152935.2263935-1-arnd@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | power: supply: max8997_charger: make EXTCON dependency unconditional | expand |
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
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)
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
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
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
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
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
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
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
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.