Re: [PATCH] usb: dwc3: fix build error when POWER_SUPPLY is not enabled

[Sebastian Reichel <sre@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 2858 bytes --]

Hi,

On Mon, Mar 08, 2021 at 09:31:46PM +0800, Ray Chi wrote:
> Fix build error when CONFIG_POWER_SUPPLY is not enabled.
> 
> The build error occurs in mips (cavium_octeon_defconfig).
> 
> mips-linux-gnu-ld: drivers/usb/dwc3/core.o: in function `dwc3_remove':
> drivers/usb/dwc3/core.c:1657: undefined reference to `power_supply_put'
> mips-linux-gnu-ld: drivers/usb/dwc3/core.o: in function `dwc3_get_properties':
> drivers/usb/dwc3/core.c:1270: undefined reference to `power_supply_get_by_name'
> mips-linux-gnu-ld: drivers/usb/dwc3/core.o: in function `dwc3_probe':
> drivers/usb/dwc3/core.c:1632: undefined reference to `power_supply_put'
> 
> Fixes: 59fa3def35de ("usb: dwc3: add a power supply for current control")
> Reported-by: Naresh Kamboju <naresh.kamboju@linaro.org>
> Signed-off-by: Ray Chi <raychi@google.com>
> ---

While I'm fine with merging this after fixing up the subject, the
original patch for dwc3 [0] looks completly incorrect to me.

First of all it uses wrong scale (power-supply uses uA, not mA),
so you are charging 1000x slower than expected. Then the patchset
introduces a new DT property to get the power-supply device, but
does not update the DT binding documentation and does not Cc the
DT binding maintainer. Next the property itself looks not very
smart to me. Usually one would use a device reference, not the
Linux device name.

Finally all existing devices solve this by registering a usb
notifier from the charger, so why are you going the other way
around? This is going to break once you want to use one of the
existing chargers with dwc3.

I suggest to drop/revert the whole patchset.

[0] https://lore.kernel.org/linux-usb/20210222115149.3606776-1-raychi@google.com/

-- Sebastian

>  include/linux/power_supply.h | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
> index 81a55e974feb..6e776be5bfa0 100644
> --- a/include/linux/power_supply.h
> +++ b/include/linux/power_supply.h
> @@ -381,8 +381,14 @@ struct power_supply_battery_info {
>  extern struct atomic_notifier_head power_supply_notifier;
>  extern int power_supply_reg_notifier(struct notifier_block *nb);
>  extern void power_supply_unreg_notifier(struct notifier_block *nb);
> +#if IS_ENABLED(CONFIG_POWER_SUPPLY)
>  extern struct power_supply *power_supply_get_by_name(const char *name);
>  extern void power_supply_put(struct power_supply *psy);
> +#else
> +static inline void power_supply_put(struct power_supply *psy) {}
> +static inline struct power_supply *power_supply_get_by_name(const char *name)
> +{ return NULL; }
> +#endif
>  #ifdef CONFIG_OF
>  extern struct power_supply *power_supply_get_by_phandle(struct device_node *np,
>  							const char *property);
> -- 
> 2.30.1.766.gb4fecdf3b7-goog
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]