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 <[email protected]>
Signed-off-by: Ray Chi <[email protected]>
---
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
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 <[email protected]>
> Signed-off-by: Ray Chi <[email protected]>
> ---
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/[email protected]/
-- 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
>
Hi Sebastian,
Sorry for the late reply.
On Wed, Mar 10, 2021 at 2:58 AM Sebastian Reichel <[email protected]> wrote:
>
> 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 <[email protected]>
> > Signed-off-by: Ray Chi <[email protected]>
> > ---
>
> 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.
Yes, it should use uA and send this information, and I will update a
patch to fix it and add the DT binding documentation.
> 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.
Only the USB controller will know USB state/speed so that I think it is
better to send this information from the USB side.
For example:
For USB high speed, charging current should be limited to 500 mA in
configured state.
For USB super speed, charging current should be limited to 900 mA in
configured state.
>
> I suggest to drop/revert the whole patchset.
>
> [0] https://lore.kernel.org/linux-usb/[email protected]/
>
> -- 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
> >
Hi,
On Fri, Mar 12, 2021 at 09:57:56PM +0800, Ray Chi wrote:
> > 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.
>
> Yes, it should use uA and send this information, and I will update a
> patch to fix it and add the DT binding documentation.
Considering your programming is off by a factor 1000 I wonder how
this patchset has been tested.
> > 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.
>
> Only the USB controller will know USB state/speed so that I think
> it is better to send this information from the USB side. For
> example: For USB high speed, charging current should be limited to
> 500 mA in configured state. For USB super speed, charging current
> should be limited to 900 mA in configured state.
usb_register_notifier registers a callback to receive information
from the USB subsystem. Then power-supply drivers can query specific
info, e.g. call usb_phy_get_charger_current(). This is already being
done by some power-supply drivers, so using one of those
power-supply charger drivers in combination with dwc3 will now result
in potentially racy behaviour as far as I can see.
> > I suggest to drop/revert the whole patchset.
-- Sebastian
Hi,
On Mon, Mar 15, 2021 at 6:35 AM Sebastian Reichel <[email protected]> wrote:
>
> Hi,
>
> On Fri, Mar 12, 2021 at 09:57:56PM +0800, Ray Chi wrote:
> > > 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.
> >
> > Yes, it should use uA and send this information, and I will update a
> > patch to fix it and add the DT binding documentation.
>
> Considering your programming is off by a factor 1000 I wonder how
> this patchset has been tested.
Since our corresponding charging driver also uses mA as the unit, I
don't find this problem.
>
> > > 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.
> >
> > Only the USB controller will know USB state/speed so that I think
> > it is better to send this information from the USB side. For
> > example: For USB high speed, charging current should be limited to
> > 500 mA in configured state. For USB super speed, charging current
> > should be limited to 900 mA in configured state.
>
> usb_register_notifier registers a callback to receive information
> from the USB subsystem. Then power-supply drivers can query specific
> info, e.g. call usb_phy_get_charger_current(). This is already being
> done by some power-supply drivers, so using one of those
> power-supply charger drivers in combination with dwc3 will now result
> in potentially racy behaviour as far as I can see.
Since these functions are defined in driver/usb/phy/phy.c, only the
devices which support usb_phy can use them.
If the device supports generic PHY drivers, it needs an additional way
to provide the information.
BTW, when there are two or more ways to provide the information, I
think it is fine to return the result directly if one of the ways is
executed successfully.
>
> > > I suggest to drop/revert the whole patchset.
>
> -- Sebastian
On Fri, Mar 12, 2021 at 09:57:56PM +0800, Ray Chi wrote:
> Hi Sebastian,
>
> Sorry for the late reply.
>
> On Wed, Mar 10, 2021 at 2:58 AM Sebastian Reichel <[email protected]> wrote:
> >
> > 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 <[email protected]>
> > > Signed-off-by: Ray Chi <[email protected]>
> > > ---
> >
> > 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.
>
> Yes, it should use uA and send this information, and I will update a
> patch to fix it and add the DT binding documentation.
So should I revert what we currently have in my usb-next tree, or do
you have a fix for this?
thanks,
greg k-h
Hi Greg,
I will upload fixes for power supply usage in dwc3 and dt-binding
documentation for the new device tree this week.
Thanks,
Ray
On Tue, Mar 23, 2021 at 9:47 PM Greg KH <[email protected]> wrote:
>
> On Fri, Mar 12, 2021 at 09:57:56PM +0800, Ray Chi wrote:
> > Hi Sebastian,
> >
> > Sorry for the late reply.
> >
> > On Wed, Mar 10, 2021 at 2:58 AM Sebastian Reichel <[email protected]> wrote:
> > >
> > > 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 <[email protected]>
> > > > Signed-off-by: Ray Chi <[email protected]>
> > > > ---
> > >
> > > 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.
> >
> > Yes, it should use uA and send this information, and I will update a
> > patch to fix it and add the DT binding documentation.
>
> So should I revert what we currently have in my usb-next tree, or do
> you have a fix for this?
>
> thanks,
>
> greg k-h