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

[Sebastian Reichel]

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

Attachment: signature.asc
Description: PGP signature