Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210308133146.3168995-1-raychi@google.com> <20210309185807.ka4iljasq5cmpmil@earth.universe>
 <CAPBYUsCJ3ftC4ur412rFZGeeM_kDHrCh=BVci3=8SE2eFdPcQg@mail.gmail.com> <20210314223513.tznvhuq7phxxvjfo@earth.universe>
In-Reply-To: <20210314223513.tznvhuq7phxxvjfo@earth.universe>
From:   Ray Chi <raychi@google.com>
Date:   Mon, 15 Mar 2021 20:51:44 +0800
Message-ID: <CAPBYUsAb5Jghok-8d2ypdEBaqoahVPUAqYt0F8H0VocDMDW7Qg@mail.gmail.com>
Subject: Re: [PATCH] usb: dwc3: fix build error when POWER_SUPPLY is not enabled
To:     Sebastian Reichel <sre@kernel.org>
Cc:     Greg KH <gregkh@linuxfoundation.org>,
        Naresh Kamboju <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>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi,

On Mon, Mar 15, 2021 at 6:35 AM Sebastian Reichel <sre@kernel.org> 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