Re: [PATCH v19 0/4] Introduce usb charger framework to deal with the usb gadget power negotation

[Baolin Wang]

On 10 March 2017 at 16:27, Jun Li <jun.li@xxxxxxx> wrote:
> Hi
>
>> -----Original Message-----
>> From: Baolin Wang [mailto:baolin.wang@xxxxxxxxxx]
>> Sent: Friday, March 10, 2017 3:15 PM
>> To: Jun Li <jun.li@xxxxxxx>
>> Cc: NeilBrown <neilb@xxxxxxxx>; Felipe Balbi <balbi@xxxxxxxxxx>; Greg KH
>> <gregkh@xxxxxxxxxxxxxxxxxxx>; Sebastian Reichel <sre@xxxxxxxxxx>; Dmitry
>> Eremin-Solenikov <dbaryshkov@xxxxxxxxx>; David Woodhouse
>> <dwmw2@xxxxxxxxxxxxx>; robh@xxxxxxxxxx; Marek Szyprowski
>> <m.szyprowski@xxxxxxxxxxx>; Ruslan Bilovol <ruslan.bilovol@xxxxxxxxx>;
>> Peter Chen <peter.chen@xxxxxxxxxxxxx>; Alan Stern
>> <stern@xxxxxxxxxxxxxxxxxxx>; grygorii.strashko@xxxxxx; Yoshihiro Shimoda
>> <yoshihiro.shimoda.uh@xxxxxxxxxxx>; Lee Jones <lee.jones@xxxxxxxxxx>;
>> Mark Brown <broonie@xxxxxxxxxx>; John Stultz <john.stultz@xxxxxxxxxx>;
>> Charles Keepax <ckeepax@xxxxxxxxxxxxxxxxxxxxxxxxxxx>;
>> patches@xxxxxxxxxxxxxxxxxxxxxxxxxxx; Linux PM list <linux-
>> pm@xxxxxxxxxxxxxxx>; USB <linux-usb@xxxxxxxxxxxxxxx>; device-
>> mainlining@xxxxxxxxxxxxxxxxxxxxxxxxx; LKML <linux-kernel@xxxxxxxxxxxxxxx>
>> Subject: Re: [PATCH v19 0/4] Introduce usb charger framework to deal with
>> the usb gadget power negotation
>>
>> On 10 March 2017 at 14:30, Jun Li <jun.li@xxxxxxx> wrote:
>> >> >> >
>> >> >> > Will generic phy need add extcon as well?
>> >> >>
>> >> >> Yes, will add a 'struct extcon_dev*' in 'struct usb_phy', which
>> >> >> will be common code.
>> >> >>
>> >> >
>> >> > I mean the common code need add 'struct extcon_dev' into both
>> >> > 'struct phy' and 'struct usb_phy', right? as some/new usb phy use
>> >> > that generic phy
>> >> driver.
>> >>
>> >> Ah, you remind me. Seems we need also add one 'struct extcon_dev'
>> >> into 'struct phy'.
>> >>
>> >> >> >
>> >> >> >> >
>> >> >> >> >> Secondly, I also agreed with Peter's comments: Not only USB
>> >> >> >> >> PHY to register an extcon, but also for the drivers which
>> >> >> >> >> can detect USB charger type, it may be USB controller
>> >> >> >> >> driver, USB type-c driver, pmic driver, and these drivers
>> >> >> >> >> may not have an extcon device since the internal part can finish
>> the vbus detect.
>> >> >> >> >
>> >> >> >> > Whichever part can detect vbus, the driver for that part must
>> >> >> >> > be able to find the extcon and trigger a notification.
>> >> >> >> > Maybe one part can detect VBUS, another can measure the
>> >> >> >> > resistance on ID and a third can work through the state
>> >> >> >> > machine to determine if D+ and D- are shorted together.
>> >> >> >> > Somehow these three need to work together to determine what
>> >> >> >> > is
>> >> >> >> plugged
>> >> >> >> > in to the external connection port.  Somewhere there much an
>> >> 'extcon'
>> >> >> >> > device which represents that port and which the three devices
>> >> >> >> > can find and can interact with.
>> >> >> >> > I think it makes sense for the usb_phy to be the connection point.
>> >> >> >> > Each of the devices can get to the phy, and the phy can get
>> >> >> >> > to the
>> >> >> extcon.
>> >> >> >> > It doesn't matter very much if the usb phy driver creates the
>> >> >> >> > extcon, or if something else creates the extcon and the phy
>> >> >> >> > driver performs a lookup to find it (e.g. based on devicetree info).
>> >> >> >> >
>> >> >> >> > The point is that there is obviously an external physical
>> >> >> >> > connection, and so there should be an 'extcon' device that
>> >> represents it.
>> >> >> >>
>> >> >> >> Peter & Jun, is it OK for you every phy has one extcon device
>> >> >> >> to receive VBUS notification, especially for detecting the
>> >> >> >> charger type by
>> >> >> software?
>> >> >> >>
>> >> >> >
>> >> >> > My understanding is phy/usb_phy as the connection point, will
>> >> >> > send the notification to PMIC driver which actually control the
>> >> >> > charge current, also this will be done in your common framework,
>> right?
>> >> >>
>> >> >> Not in USB charger framework. If we are all agree every usb_phy
>> >> >> can register one extcon device, can get correct charger type and
>> >> >> send out correct vbus_draw information, then we don't need USB
>> >> >> charger framework as Neil suggested. So this will be okay for your
>> >> >> case (especially for detecting the charger type by software) ?
>> >> >
>> >> > In my case, charger detection is done by controller driver and I
>> >> > need do charger type detection internally, and only pass the
>> >> > current draw info via phy which will send out, this seems ok for
>> >> > me, but I think it will be good if you or someone can show us an
>> >> > example user based on the
>> >> design Neil suggested.
>> >> > Will you work out that common code if this USB charger framework is
>> >> > not
>> >> needed?
>> >>
>> >> I will add  a 'struct extcon_dev*' in 'struct usb_phy' and struct
>> >> phyâ. Others are already ready if everyone has no complain about
>> >> current design, except
>> >
>> > Only adding extcon_dev into usb_phy/phy  and all others are ready?
>> > My understanding you will also do:
>> > - We need find a central place to send the notification(phy common part).
>>
>> That will include these implementation when adding extcon_dev.
>>
>
> OK, thanks.
>
>> > - If the extcon_dev is directly added in usb_phy/phy, PMIC needs some
>> API to findup it.
>>
>> PMIC can find extcon device by phandle.
>
> extcon_dev(not a reference pointer) is directly added in usb_phy/phy, not via devicetree,
> how PMIC find it by phandle?

>From my understanding, here we should add one pointer (extcon_dev *),
since usb phy is not one external connect device.

>
>>
>> >
>> >> my one concern. (I am afraid if it is enough to send out vbus draw
>> >> information from USB phy driver, for example you will miss super
>> >> speed (900mA), which need get the speed information from gadget
>> >> driver.)
>> >>
>> >
>> > Can we handle this in USB(so has super speed information) and just
>> > send out 900mA directly?
>>
>> From Neil's suggestion, we only have one place to send out current
>> information from usb phy, so I have this concern and doubt if we still need
>> the USB charger framework.
>
> So if put it in phy/usb_phy, this is a problem, that only one place should have
> the infor of both speed and usb state, how about put it at usb_gadget, then,
> e.g. send the notification in usb_gadget_vbus_connect()?

That is same what USB charger did, from this point, we need USB
charger to send out vbus draw information according to speed and usb
state. But I should listen to other guys suggestion. Peter and Felipe,
what do you think?

-- 
Baolin.wang
Best Regards