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

From: Sebastian Reichel <sre@kernel.org>
To: Ray Chi <raychi@google.com>
Cc: Greg KH <gregkh@linuxfoundation.org>,
	naresh.kamboju@linaro.org, Kyle Tso <kyletso@google.com>,
	linux-pm@vger.kernel.org, linux-kernel@vger.kernel.org,
	Badhri Jagan Sridharan <badhri@google.com>
Subject: Re: [PATCH] usb: dwc3: fix build error when POWER_SUPPLY is not enabled
Date: Sun, 14 Mar 2021 23:35:13 +0100	[thread overview]
Message-ID: <20210314223513.tznvhuq7phxxvjfo@earth.universe> (raw)
In-Reply-To: <CAPBYUsCJ3ftC4ur412rFZGeeM_kDHrCh=BVci3=8SE2eFdPcQg@mail.gmail.com>

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

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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]