by Jun Li

[permalink] [raw]

Subject: RE: [PATCH v10 1/4] gadget: Introduce the usb charger framework

Hi

> -----Original Message-----
> From: Baolin Wang [mailto:[email protected]]
> Sent: Monday, April 11, 2016 7:15 PM
> To: Jun Li <[email protected]>
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected]; device-
> [email protected]; [email protected]
> Subject: Re: [PATCH v10 1/4] gadget: Introduce the usb charger framework
>
> Hi Jun,
>
> Sorry for late reply.
>
> >> >> >> >> +/*
> >> >> >> >> + * usb_charger_detect_type() - detect the charger type
> manually.
> >> >> >> >> + * @uchger - usb charger device
> >> >> >> >> + *
> >> >> >> >> + * Note: You should ensure you need to detect the charger
> >> >> >> >> +type manually on your
> >> >> >> >> + * platform.
> >> >> >> >> + * You should call it at the right gadget state to avoid
> >> >> >> >> +affecting gadget
> >> >> >> >> + * enumeration.
> >> >> >> >> + */
> >> >> >> >> +int usb_charger_detect_type(struct usb_charger *uchger) {
> >> >> >> >> + enum usb_charger_type type;
> >> >> >> >> +
> >> >> >> >> + if (WARN(!uchger->charger_detect,
> >> >> >> >> + "charger detection method should not be NULL"))
> >> >> >> >> + return -EINVAL;
> >> >> >> >> +
> >> >> >> >> + type = uchger->charger_detect(uchger);
> >> >> >> >> +
> >> >> >> >> + mutex_lock(&uchger->lock);
> >> >> >> >> + uchger->type = type;
> >> >> >> >> + mutex_unlock(&uchger->lock);
> >> >> >> >> +
> >> >> >> >> + return 0;
> >> >> >> >> +}
> >> >> >> >> +EXPORT_SYMBOL_GPL(usb_charger_detect_type);
> >> >> >> >
> >> >> >> > I still recommend have a central place to call
> >> >> >> > usb_charger_detect_type() to cover real charger detection
> >> >> >> > instead of leaving this question open to diff users. This can
> >> >> >> > be done after vbus-on is detected and before do gadget
> >> >> >> > connect. I don't think this
> >> >> >> will make your framework complicated.
> >> >> >>
> >> >> >> This API is used for detecting the charger type manually
> >> >> >> (software charger detection), so if user's udc driver is needed
> >> >> >> to do this, then they can issue it at their right place to make
> it more flexible.
> >> >> >> But let us see other people's suggestion. Thanks.
> >> >> >
> >> >> > Ok, actually this API can be used for what ever charger
> >> >> > detection type, user can implement any method(manual detection,
> >> >> > directly read result from some HW...what ever) in
> >> >> > uchger->charger_detect(), target is
> >> >>
> >> >> But reading result from some HW dose not means *detect*, actually
> >> >> is
> >> *get*.
> >> >> We can not mix them together with the different semantics.
> >> >
> >> > It doesn’t matter here, you already define the _get_ API for just
> >> > read the charger type from the saved value(uchger->type), so we can
> >> > think the API to make uchger->type from UNKNOW to ONE KNOWN type is
> >> > *detect*, because we don't need 2 separate API one for *get* type
> >> > from HW and another one for *detect* from HW, only one API involve
> >> > HW access
> >> is enough.
> >>
> >> But another issue is some users may need to get the charger type from
> >> power supply by "power_supply_get_property()" function, we need to
> >> integrate with the power supply things in the usb charger framework,
> >> not user to implement that.
> >
> > Why this user(your case) can't put the charger type get by
> > "power_supply_get_property()" in its uchger->charger_detect(), any
> > special reason prevent you doing it? I am not sure if this case is
> > very common, even if it is, you also can put it in
> > usb_charger_detect_type()
> >
> > usb_charger_detect_type()
> > {
> > If can get from power_supply_get_property()
> > ...
> > else if uchger->charger_detect
> > uchger->charger_detect();
> > ...
> > }
>
> If user want to implement above method, they need to get the
> 'power_supply' structure to do this action, which maybe can not get in
> some contexts. So we need to integrate with the power supply things to
> avoid this situation. IIRC, Felipe suggested me to flesh out the charger
> getting method.
>

Okay, then I agree with you to let user do that with more flexibility,
I tried to enable usb charger detection on one NXP i.mx platform based on
this framework, works fine, thanks!

Li Jun
> Hi Felipe, what do you think of Jun's suggestion? Thanks.
>
> >
> > I don't know if most design of usb charger detection now days is as
> > easy as your PMIC for software(HW does the whole detection process and
> > prevent the vbus generation until detection has completed), anyway if
> > your framework can guarantee the detection process finished before
> > gadget connect, then each user don't have to worry about the HW
> > detection process details and seek a proper place to do it.
> >
> > Li Jun
> >>
> >> >
> >> > - Detect: (write to and) read from HW to get the charger type,
> >> > save to uchger->type;
> >> > - Get: read the charger type from uchger->type.
> >> >
> >> >>
> >> >> > to have the charger type and set uchger->type, then you no need
> >> >> > to add the comments/description to limit it usage. Also I do see
> >> >> > there is
> >> >> possible central place to do this.
> >> >> >
> >> >>
> >> >> --
> >> >> Baolin.wang
> >> >> Best Regards
> >>
> >>
> >>
> >> --
> >> Baolin.wang
> >> Best Regards
>
>
>
> --
> Baolin.wang
> Best Regards

2016-04-18 08:57:27

by Baolin Wang

[permalink] [raw]

Subject: Re: [PATCH v10 1/4] gadget: Introduce the usb charger framework

Hi Jun,

On 18 April 2016 at 16:27, Jun Li <[email protected]> wrote:
>> >>
>> >> But another issue is some users may need to get the charger type from
>> >> power supply by "power_supply_get_property()" function, we need to
>> >> integrate with the power supply things in the usb charger framework,
>> >> not user to implement that.
>> >
>> > Why this user(your case) can't put the charger type get by
>> > "power_supply_get_property()" in its uchger->charger_detect(), any
>> > special reason prevent you doing it? I am not sure if this case is
>> > very common, even if it is, you also can put it in
>> > usb_charger_detect_type()
>> >
>> > usb_charger_detect_type()
>> > {
>> > If can get from power_supply_get_property()
>> > ...
>> > else if uchger->charger_detect
>> > uchger->charger_detect();
>> > ...
>> > }
>>
>> If user want to implement above method, they need to get the
>> 'power_supply' structure to do this action, which maybe can not get in
>> some contexts. So we need to integrate with the power supply things to
>> avoid this situation. IIRC, Felipe suggested me to flesh out the charger
>> getting method.
>>
>
> Okay, then I agree with you to let user do that with more flexibility,
> I tried to enable usb charger detection on one NXP i.mx platform based on
> this framework, works fine, thanks!

That's great. Thanks.

--
Baolin.wang
Best Regards