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

[Baolin Wang]

On 5 April 2016 at 17:43, Peter Chen <hzpeterchen@xxxxxxxxx> wrote:
> On Tue, Apr 05, 2016 at 05:34:02PM +0800, Baolin Wang wrote:
>> On 5 April 2016 at 16:12, Peter Chen <hzpeterchen@xxxxxxxxx> wrote:
>> > On Tue, Apr 05, 2016 at 03:54:31PM +0800, Baolin Wang wrote:
>> >> Hi Peter,
>> >>
>> >> On 5 April 2016 at 14:46, Peter Chen <hzpeterchen@xxxxxxxxx> wrote:
>> >> >
>> >> > We are thinking USB charger framework for Freescale i.mx SoC series,
>> >> > since our internal framework is not good enough.
>> >> > So I have more questions for your framework since there are many
>> >> > different USB charger designs, and I hope it is universal.
>> >>
>> >> Great, thanks for your attention and suggestions.
>> >>
>> >> >
>> >> > - I would like to see your all code to let the charger work, eg
>> >> > you have said the charger detection is done by PMIC automatically,
>> >> > but I did not find your PMIC code to read charger type.
>> >>
>> >> Yeah, this patchset did not give an example to read charger type from
>> >> PMIC registers. (Cause now the user 'wm831x_power' don't need this.)
>> >> But I think user can get it easily by implementing below callbacks.
>> >> (1) gadget->ops->get_charger_type();
>> >> (2) power_supply_get_property(uchger->psy, POWER_SUPPLY_PROP_CHARGE_TYPE, &val);
>> >> (3) uchger->get_charger_type();
>> >>
>> >
>> > I just would like if you can have this, then, we (you) can test it, eg,
>> > you can test if the wm831x can charge more than 1500mA for DCP.
>>
>> Mark, could you please address Peter's comments about if the the
>> wm831x can charge more than 1500mA for DCP? (I have no environment to
>> test wm831x) Thanks.
>>
>
> I don't want you or Mark to test at hardware, I just would like to see
> some code that how PMIC, wm831x, and USB gadget driver work together.

The whole code are in this patchset.

>
>> >> >
>> >> > Besides, how you can make sure the charger detection has finished
>> >> > before the framework handles USB_CHARGER_PRESENT event?
>> >>
>> >> I think we don't need to care about this situation. If the charger
>> >> type is 'UNKNOWN_TYPE' (maybe charger detection is not finished yet),
>> >> the charger framework will not set current (current is 0) for power
>> >> driver.
>> >
>> > Then, when we notify the charger IC for larger current, eg, for DCP.
>>
>> I suppose If the usb charger framework gets the charger type, then
>> notify the charger IC for larger current.
>
> It is no problem for software detection, but for hardware one, it must
> make sure it sends "USB_CHARGER_PRESENT" event after detection has
> finished, otherwise no one will notify charger framework for DCP.

Yeah, the hardware detection should guarantee charger detection will
be finished firstly.

>
>>
>> >
>> >>
>> >> >
>> >> > - I commented the current limit at different situations for USB
>> >> > charger last time, but I have not seen your further comments.
>> >> > I would like give it again. For DCP, you can notify charger IC
>> >> > once you get the charger type. But for CDP/SDP, you need to
>> >> > notify charger IC after set configuration has finished, since
>> >> > the host may still not be ready to give high current.
>> >>
>> >> As my understanding, if the usb charger framework get the charger
>> >> type, it means we can notify the power driver to set the current. If
>> >> you don't ready for setting current, please don't give the charger
>> >> type to usb charger framework.
>> >>
>> >> The framework does not want to focus on charger detection too much,
>> >> and just supplies one callback '->charger_detect()' for user to be
>> >> implemented if they ensure they need to do the SW charger detection
>> >> manually (Note: must at the right time to do the SW detection.). So
>> >> the usb charger just focus on dealing with the usb gadget power
>> >> negotiation, and it does not need to care much how to do charger
>> >> detection on your platform.
>> >
>> > No, this comment is common one, but only for SW detection. Eg, when
>> > the PMIC tells you it is a SDP, you can't notify to charger IC about
>> > 500mA at once, you need to do it after host allows you to do it.
>>
>> Well. Sounds reasonable. I just give an ugly example to implement the
>> 'gadget->ops->get_charger_type()' method to get the charger type.
>>
>> enum usb_charger_type get_charger_type(struct usb_gadget *gadget)
>> {
>>         if (host is allowed)
>>                 read charger type from PMIC;
>>         else
>>                 return UNKNOWN_TYPE;
>> }
>>
>> So that will makes usb charger do not need to care about too much to
>> make things more complicated. Or do you have any other good
>> suggestions? Thanks.
>>
>
> Since it is a USB charger, you had to be involved with USB stuffs:).

OK. Maybe I need to do some investigation about that. Thanks.

>
> --
>
> Best Regards,
> Peter Chen



-- 
Baolin.wang
Best Regards