| Message ID | 13c2f4fb71958bf9a5527acbed8b8b60dc569656.1459494744.git.baolin.wang@linaro.org (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On Fri, Apr 01, 2016 at 03:21:50PM +0800, Baolin Wang wrote: > For supporting the usb charger, it adds the usb_charger_init() and > usb_charger_exit() functions for usb charger initialization and exit. > > It will report to the usb charger when the gadget state is changed, > then the usb charger can do the power things. > > Introduce a callback 'get_charger_type' which will implemented by > user for usb gadget operations to get the usb charger type. > > Signed-off-by: Baolin Wang <baolin.wang@linaro.org> > --- > drivers/usb/gadget/udc/udc-core.c | 11 +++++++++++ > include/linux/usb/gadget.h | 15 +++++++++++++++ > 2 files changed, 26 insertions(+) > > diff --git a/drivers/usb/gadget/udc/udc-core.c b/drivers/usb/gadget/udc/udc-core.c > index b86a6f0..8d98c6b 100644 > --- a/drivers/usb/gadget/udc/udc-core.c > +++ b/drivers/usb/gadget/udc/udc-core.c > @@ -28,6 +28,7 @@ > #include <linux/usb/ch9.h> > #include <linux/usb/gadget.h> > #include <linux/usb.h> > +#include <linux/usb/charger.h> > > /** > * struct usb_udc - describes one usb device controller > @@ -230,6 +231,9 @@ static void usb_gadget_state_work(struct work_struct *work) > struct usb_gadget *gadget = work_to_gadget(work); > struct usb_udc *udc = gadget->udc; > > + /* when the gadget state is changed, then report to USB charger */ > + usb_charger_plug_by_gadget(gadget, gadget->state); > + > if (udc) > sysfs_notify(&udc->dev.kobj, NULL, "state"); > } > @@ -423,8 +427,14 @@ int usb_add_gadget_udc_release(struct device *parent, struct usb_gadget *gadget, > > mutex_unlock(&udc_lock); > > + ret = usb_charger_init(gadget); > + if (ret) > + goto err5; > + > return 0; > > +err5: > + device_del(&udc->dev); > err4: > list_del(&udc->list); > mutex_unlock(&udc_lock); > @@ -503,6 +513,7 @@ void usb_del_gadget_udc(struct usb_gadget *gadget) > kobject_uevent(&udc->dev.kobj, KOBJ_REMOVE); > flush_work(&gadget->work); > device_unregister(&udc->dev); > + usb_charger_exit(gadget); > device_unregister(&gadget->dev); > } > EXPORT_SYMBOL_GPL(usb_del_gadget_udc); > diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h > index d82d006..054488a 100644 > --- a/include/linux/usb/gadget.h > +++ b/include/linux/usb/gadget.h > @@ -24,6 +24,7 @@ > #include <linux/types.h> > #include <linux/workqueue.h> > #include <linux/usb/ch9.h> > +#include <linux/usb/charger.h> > > struct usb_ep; > > @@ -563,6 +564,8 @@ struct usb_gadget_ops { > struct usb_ep *(*match_ep)(struct usb_gadget *, > struct usb_endpoint_descriptor *, > struct usb_ss_ep_comp_descriptor *); > + /* get the charger type */ > + enum usb_charger_type (*get_charger_type)(struct usb_gadget *); > }; Since we already have get_charger_type callback at usb_charger structure, why we still need this API at usb_gadget_ops? > > /** > @@ -635,6 +638,8 @@ struct usb_gadget { > unsigned out_epnum; > unsigned in_epnum; > struct usb_otg_caps *otg_caps; > + /* negotiate the power with the usb charger */ > + struct usb_charger *charger; > > unsigned sg_supported:1; > unsigned is_otg:1; > @@ -839,10 +844,20 @@ static inline int usb_gadget_vbus_connect(struct usb_gadget *gadget) > * reporting how much power the device may consume. For example, this > * could affect how quickly batteries are recharged. > * > + * It will also notify the USB charger how much power the device may > + * consume if there is a USB charger linking with the gadget. > + * > * Returns zero on success, else negative errno. > */ > static inline int usb_gadget_vbus_draw(struct usb_gadget *gadget, unsigned mA) > { > + enum usb_charger_type type; > + > + if (gadget->charger) { > + type = usb_charger_get_type(gadget->charger); > + usb_charger_set_cur_limit_by_type(gadget->charger, type, mA); > + } > + You may do something redundant. - Charger detection only occurs at connection period, at other periods, we only change the current limit, and notify charger IC. That is to say, we may only need to save charger type and current limit at usb_charger structure, we don't need to distinguish all chargers type from time to time. - The purpose of usb_gadget_vbus_draw design is notify charger IC too, so you can do set current limit and notify charger IC together at this API together, it has already covered all situations. We don't need to notify charger IC again when the gadget status has changed again. > if (!gadget->ops->vbus_draw) > return -EOPNOTSUPP; > return gadget->ops->vbus_draw(gadget, mA); > -- > 1.7.9.5 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-usb" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
On 6 April 2016 at 15:19, Peter Chen <hzpeterchen@gmail.com> wrote: > On Fri, Apr 01, 2016 at 03:21:50PM +0800, Baolin Wang wrote: >> >> @@ -563,6 +564,8 @@ struct usb_gadget_ops { >> struct usb_ep *(*match_ep)(struct usb_gadget *, >> struct usb_endpoint_descriptor *, >> struct usb_ss_ep_comp_descriptor *); >> + /* get the charger type */ >> + enum usb_charger_type (*get_charger_type)(struct usb_gadget *); >> }; > > Since we already have get_charger_type callback at usb_charger > structure, why we still need this API at usb_gadget_ops? In case some users want to get charger type at gadget level. > >> >> /** >> @@ -635,6 +638,8 @@ struct usb_gadget { >> unsigned out_epnum; >> unsigned in_epnum; >> struct usb_otg_caps *otg_caps; >> + /* negotiate the power with the usb charger */ >> + struct usb_charger *charger; >> >> unsigned sg_supported:1; >> unsigned is_otg:1; >> @@ -839,10 +844,20 @@ static inline int usb_gadget_vbus_connect(struct usb_gadget *gadget) >> * reporting how much power the device may consume. For example, this >> * could affect how quickly batteries are recharged. >> * >> + * It will also notify the USB charger how much power the device may >> + * consume if there is a USB charger linking with the gadget. >> + * >> * Returns zero on success, else negative errno. >> */ >> static inline int usb_gadget_vbus_draw(struct usb_gadget *gadget, unsigned mA) >> { >> + enum usb_charger_type type; >> + >> + if (gadget->charger) { >> + type = usb_charger_get_type(gadget->charger); >> + usb_charger_set_cur_limit_by_type(gadget->charger, type, mA); >> + } >> + > > You may do something redundant. > > - Charger detection only occurs at connection period, at other periods, > we only change the current limit, and notify charger IC. That is to > say, we may only need to save charger type and current limit at > usb_charger structure, we don't need to distinguish all chargers > type from time to time. That's right. I just want to get the charger type as one parameter to set current. The function is implemented as below: enum usb_charger_type usb_charger_get_type(struct usb_charger *uchger) { enum usb_charger_type type; mutex_lock(&uchger->lock); type = uchger->type; mutex_unlock(&uchger->lock); return type; } > > - The purpose of usb_gadget_vbus_draw design is notify charger IC too, > so you can do set current limit and notify charger IC together at this > API together, it has already covered all situations. We don't need to > notify charger IC again when the gadget status has changed again. It did not notify charger IC again. You are right, usb_gadget_vbus_draw design will notify charger IC, so we want to record the current in usb charger framework at the same time. > >> if (!gadget->ops->vbus_draw) >> return -EOPNOTSUPP; >> return gadget->ops->vbus_draw(gadget, mA); >> -- >> 1.7.9.5 >> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-usb" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html > > -- > > Best Regards, > Peter Chen
Hi > -----Original Message----- > From: linux-usb-owner@vger.kernel.org [mailto:linux-usb- > owner@vger.kernel.org] On Behalf Of Baolin Wang > Sent: Wednesday, April 06, 2016 6:47 PM > To: Peter Chen <hzpeterchen@gmail.com> > Cc: Felipe Balbi <balbi@kernel.org>; Greg KH <gregkh@linuxfoundation.org>; > Sebastian Reichel <sre@kernel.org>; Dmitry Eremin-Solenikov > <dbaryshkov@gmail.com>; David Woodhouse <dwmw2@infradead.org>; Peter Chen > <peter.chen@freescale.com>; Alan Stern <stern@rowland.harvard.edu>; > r.baldyga@samsung.com; Yoshihiro Shimoda > <yoshihiro.shimoda.uh@renesas.com>; Lee Jones <lee.jones@linaro.org>; Mark > Brown <broonie@kernel.org>; Charles Keepax > <ckeepax@opensource.wolfsonmicro.com>; patches@opensource.wolfsonmicro.com; > Linux PM list <linux-pm@vger.kernel.org>; USB <linux-usb@vger.kernel.org>; > device-mainlining@lists.linuxfoundation.org; LKML <linux- > kernel@vger.kernel.org> > Subject: Re: [PATCH v9 2/4] gadget: Support for the usb charger framework > > On 6 April 2016 at 15:19, Peter Chen <hzpeterchen@gmail.com> wrote: > > On Fri, Apr 01, 2016 at 03:21:50PM +0800, Baolin Wang wrote: > >> > >> @@ -563,6 +564,8 @@ struct usb_gadget_ops { > >> struct usb_ep *(*match_ep)(struct usb_gadget *, > >> struct usb_endpoint_descriptor *, > >> struct usb_ss_ep_comp_descriptor *); > >> + /* get the charger type */ > >> + enum usb_charger_type (*get_charger_type)(struct usb_gadget *); > >> }; > > > > Since we already have get_charger_type callback at usb_charger > > structure, why we still need this API at usb_gadget_ops? > > In case some users want to get charger type at gadget level. > Why gadget needs to know charger type? I also don't catch the intent of This api, as my understanding, gadget only need report gadget state changes. All information required for usb charger is charger type and gadget state. Li Jun
Hi, Jun Li <jun.li@nxp.com> writes: >> -----Original Message----- >> From: linux-usb-owner@vger.kernel.org [mailto:linux-usb- >> owner@vger.kernel.org] On Behalf Of Baolin Wang >> Sent: Wednesday, April 06, 2016 6:47 PM >> To: Peter Chen <hzpeterchen@gmail.com> >> Cc: Felipe Balbi <balbi@kernel.org>; Greg KH <gregkh@linuxfoundation.org>; >> Sebastian Reichel <sre@kernel.org>; Dmitry Eremin-Solenikov >> <dbaryshkov@gmail.com>; David Woodhouse <dwmw2@infradead.org>; Peter Chen >> <peter.chen@freescale.com>; Alan Stern <stern@rowland.harvard.edu>; >> r.baldyga@samsung.com; Yoshihiro Shimoda >> <yoshihiro.shimoda.uh@renesas.com>; Lee Jones <lee.jones@linaro.org>; Mark >> Brown <broonie@kernel.org>; Charles Keepax >> <ckeepax@opensource.wolfsonmicro.com>; patches@opensource.wolfsonmicro.com; >> Linux PM list <linux-pm@vger.kernel.org>; USB <linux-usb@vger.kernel.org>; >> device-mainlining@lists.linuxfoundation.org; LKML <linux- >> kernel@vger.kernel.org> >> Subject: Re: [PATCH v9 2/4] gadget: Support for the usb charger framework >> >> On 6 April 2016 at 15:19, Peter Chen <hzpeterchen@gmail.com> wrote: >> > On Fri, Apr 01, 2016 at 03:21:50PM +0800, Baolin Wang wrote: >> >> >> >> @@ -563,6 +564,8 @@ struct usb_gadget_ops { >> >> struct usb_ep *(*match_ep)(struct usb_gadget *, >> >> struct usb_endpoint_descriptor *, >> >> struct usb_ss_ep_comp_descriptor *); >> >> + /* get the charger type */ >> >> + enum usb_charger_type (*get_charger_type)(struct usb_gadget *); >> >> }; >> > >> > Since we already have get_charger_type callback at usb_charger >> > structure, why we still need this API at usb_gadget_ops? >> >> In case some users want to get charger type at gadget level. >> > Why gadget needs to know charger type? I also don't catch the intent of because some gadgets need to call usb_gadget_vbus_draw(), although for that they need power in mA rather. > This api, as my understanding, gadget only need report gadget state changes. > All information required for usb charger is charger type and gadget state. you're making an assumption about how the HW is laid out which might not be true.
> -----Original Message----- > From: Felipe Balbi [mailto:balbi@kernel.org] > Sent: Wednesday, April 06, 2016 8:22 PM > To: Jun Li <jun.li@nxp.com>; Baolin Wang <baolin.wang@linaro.org>; Peter > Chen <hzpeterchen@gmail.com> > Cc: Greg KH <gregkh@linuxfoundation.org>; Sebastian Reichel > <sre@kernel.org>; Dmitry Eremin-Solenikov <dbaryshkov@gmail.com>; David > Woodhouse <dwmw2@infradead.org>; Peter Chen <peter.chen@freescale.com>; > Alan Stern <stern@rowland.harvard.edu>; r.baldyga@samsung.com; Yoshihiro > Shimoda <yoshihiro.shimoda.uh@renesas.com>; Lee Jones > <lee.jones@linaro.org>; Mark Brown <broonie@kernel.org>; Charles Keepax > <ckeepax@opensource.wolfsonmicro.com>; patches@opensource.wolfsonmicro.com; > Linux PM list <linux-pm@vger.kernel.org>; USB <linux-usb@vger.kernel.org>; > device-mainlining@lists.linuxfoundation.org; LKML <linux- > kernel@vger.kernel.org> > Subject: RE: [PATCH v9 2/4] gadget: Support for the usb charger framework > > > Hi, > > Jun Li <jun.li@nxp.com> writes: > >> -----Original Message----- > >> From: linux-usb-owner@vger.kernel.org [mailto:linux-usb- > >> owner@vger.kernel.org] On Behalf Of Baolin Wang > >> Sent: Wednesday, April 06, 2016 6:47 PM > >> To: Peter Chen <hzpeterchen@gmail.com> > >> Cc: Felipe Balbi <balbi@kernel.org>; Greg KH > >> <gregkh@linuxfoundation.org>; Sebastian Reichel <sre@kernel.org>; > >> Dmitry Eremin-Solenikov <dbaryshkov@gmail.com>; David Woodhouse > >> <dwmw2@infradead.org>; Peter Chen <peter.chen@freescale.com>; Alan > >> Stern <stern@rowland.harvard.edu>; r.baldyga@samsung.com; Yoshihiro > >> Shimoda <yoshihiro.shimoda.uh@renesas.com>; Lee Jones > >> <lee.jones@linaro.org>; Mark Brown <broonie@kernel.org>; Charles > >> Keepax <ckeepax@opensource.wolfsonmicro.com>; > >> patches@opensource.wolfsonmicro.com; > >> Linux PM list <linux-pm@vger.kernel.org>; USB > >> <linux-usb@vger.kernel.org>; > >> device-mainlining@lists.linuxfoundation.org; LKML <linux- > >> kernel@vger.kernel.org> > >> Subject: Re: [PATCH v9 2/4] gadget: Support for the usb charger > >> framework > >> > >> On 6 April 2016 at 15:19, Peter Chen <hzpeterchen@gmail.com> wrote: > >> > On Fri, Apr 01, 2016 at 03:21:50PM +0800, Baolin Wang wrote: > >> >> > >> >> @@ -563,6 +564,8 @@ struct usb_gadget_ops { > >> >> struct usb_ep *(*match_ep)(struct usb_gadget *, > >> >> struct usb_endpoint_descriptor *, > >> >> struct usb_ss_ep_comp_descriptor *); > >> >> + /* get the charger type */ > >> >> + enum usb_charger_type (*get_charger_type)(struct usb_gadget > >> >> + *); > >> >> }; > >> > > >> > Since we already have get_charger_type callback at usb_charger > >> > structure, why we still need this API at usb_gadget_ops? > >> > >> In case some users want to get charger type at gadget level. > >> > > Why gadget needs to know charger type? I also don't catch the intent > > of > > because some gadgets need to call usb_gadget_vbus_draw(), although for > that they need power in mA rather. In below change of usb_gadget_vbus_draw(), already can get charger type via usb_charger_get_type(). static inline int usb_gadget_vbus_draw(struct usb_gadget *gadget, unsigned mA) { + enum usb_charger_type type; + + if (gadget->charger) { + type = usb_charger_get_type(gadget->charger); + usb_charger_set_cur_limit_by_type(gadget->charger, type, mA); + } + if (!gadget->ops->vbus_draw) return -EOPNOTSUPP; return gadget->ops->vbus_draw(gadget, mA); Could you detail in what situation gadget->ops-> get_charger_type() is used? > > > This api, as my understanding, gadget only need report gadget state > changes. > > All information required for usb charger is charger type and gadget > state. > > you're making an assumption about how the HW is laid out which might not > be true. > What other information you refer to here? Or what I am not aware of? Thanks. Li Jun > -- > balbi -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, Jun Li <jun.li@nxp.com> writes: >> >> On 6 April 2016 at 15:19, Peter Chen <hzpeterchen@gmail.com> wrote: >> >> > On Fri, Apr 01, 2016 at 03:21:50PM +0800, Baolin Wang wrote: >> >> >> >> >> >> @@ -563,6 +564,8 @@ struct usb_gadget_ops { >> >> >> struct usb_ep *(*match_ep)(struct usb_gadget *, >> >> >> struct usb_endpoint_descriptor *, >> >> >> struct usb_ss_ep_comp_descriptor *); >> >> >> + /* get the charger type */ >> >> >> + enum usb_charger_type (*get_charger_type)(struct usb_gadget >> >> >> + *); >> >> >> }; >> >> > >> >> > Since we already have get_charger_type callback at usb_charger >> >> > structure, why we still need this API at usb_gadget_ops? >> >> >> >> In case some users want to get charger type at gadget level. >> >> >> > Why gadget needs to know charger type? I also don't catch the intent >> > of >> >> because some gadgets need to call usb_gadget_vbus_draw(), although for >> that they need power in mA rather. > > In below change of usb_gadget_vbus_draw(), already can get charger type > via usb_charger_get_type(). > > static inline int usb_gadget_vbus_draw(struct usb_gadget *gadget, unsigned mA) > { > + enum usb_charger_type type; > + > + if (gadget->charger) { > + type = usb_charger_get_type(gadget->charger); > + usb_charger_set_cur_limit_by_type(gadget->charger, type, mA); > + } > + > if (!gadget->ops->vbus_draw) > return -EOPNOTSUPP; > return gadget->ops->vbus_draw(gadget, mA); > > Could you detail in what situation gadget->ops-> get_charger_type() is used? isn't it right there in the code ? Isn't usb_gadget_vbus_draw() calling it ? What did I miss here ? >> > This api, as my understanding, gadget only need report gadget state >> changes. >> > All information required for usb charger is charger type and gadget >> state. >> >> you're making an assumption about how the HW is laid out which might not >> be true. >> > > What other information you refer to here? Or what I am not aware of? what I'm trying to say is that you're assuming gadgets don't need to know anything other than charger type and gadget state (suspended, resume, enumerated, default state, addressed, etc), but that might not be true for all UDCs. You can't make that assumption that charger type and gadget state is enough. The real question is what do we need *now*, but still keep in mind that what we need *now* might be valid 2 years from now, so API needs to be a little flexible. cheers
Hi > -----Original Message----- > From: Felipe Balbi [mailto:balbi@kernel.org] > Sent: Wednesday, April 06, 2016 8:56 PM > To: Jun Li <jun.li@nxp.com>; Baolin Wang <baolin.wang@linaro.org>; Peter > Chen <hzpeterchen@gmail.com> > Cc: Greg KH <gregkh@linuxfoundation.org>; Sebastian Reichel > <sre@kernel.org>; Dmitry Eremin-Solenikov <dbaryshkov@gmail.com>; David > Woodhouse <dwmw2@infradead.org>; Peter Chen <peter.chen@freescale.com>; > Alan Stern <stern@rowland.harvard.edu>; r.baldyga@samsung.com; Yoshihiro > Shimoda <yoshihiro.shimoda.uh@renesas.com>; Lee Jones > <lee.jones@linaro.org>; Mark Brown <broonie@kernel.org>; Charles Keepax > <ckeepax@opensource.wolfsonmicro.com>; patches@opensource.wolfsonmicro.com; > Linux PM list <linux-pm@vger.kernel.org>; USB <linux-usb@vger.kernel.org>; > device-mainlining@lists.linuxfoundation.org; LKML <linux- > kernel@vger.kernel.org> > Subject: RE: [PATCH v9 2/4] gadget: Support for the usb charger framework > > > Hi, > > Jun Li <jun.li@nxp.com> writes: > >> >> On 6 April 2016 at 15:19, Peter Chen <hzpeterchen@gmail.com> wrote: > >> >> > On Fri, Apr 01, 2016 at 03:21:50PM +0800, Baolin Wang wrote: > >> >> >> > >> >> >> @@ -563,6 +564,8 @@ struct usb_gadget_ops { > >> >> >> struct usb_ep *(*match_ep)(struct usb_gadget *, > >> >> >> struct usb_endpoint_descriptor *, > >> >> >> struct usb_ss_ep_comp_descriptor *); > >> >> >> + /* get the charger type */ > >> >> >> + enum usb_charger_type (*get_charger_type)(struct > >> >> >> + usb_gadget *); > >> >> >> }; > >> >> > > >> >> > Since we already have get_charger_type callback at usb_charger > >> >> > structure, why we still need this API at usb_gadget_ops? > >> >> > >> >> In case some users want to get charger type at gadget level. > >> >> > >> > Why gadget needs to know charger type? I also don't catch the > >> > intent of > >> > >> because some gadgets need to call usb_gadget_vbus_draw(), although > >> for that they need power in mA rather. > > > > In below change of usb_gadget_vbus_draw(), already can get charger > > type via usb_charger_get_type(). > > > > static inline int usb_gadget_vbus_draw(struct usb_gadget *gadget, > > unsigned mA) { > > + enum usb_charger_type type; > > + > > + if (gadget->charger) { > > + type = usb_charger_get_type(gadget->charger); > > + usb_charger_set_cur_limit_by_type(gadget->charger, type, > mA); > > + } > > + > > if (!gadget->ops->vbus_draw) > > return -EOPNOTSUPP; > > return gadget->ops->vbus_draw(gadget, mA); > > > > Could you detail in what situation gadget->ops-> get_charger_type() is > used? > > isn't it right there in the code ? Isn't usb_gadget_vbus_draw() calling > it ? What did I miss here ? Well, that's true, so my real meaning is why gadget need get charger type via another new api gadget->ops->get_charger_type(). > > >> > This api, as my understanding, gadget only need report gadget state > >> changes. > >> > All information required for usb charger is charger type and gadget > >> state. > >> > >> you're making an assumption about how the HW is laid out which might > >> not be true. > >> > > > > What other information you refer to here? Or what I am not aware of? > > what I'm trying to say is that you're assuming gadgets don't need to know > anything other than charger type and gadget state (suspended, resume, > enumerated, default state, addressed, etc), but that might not be true for > all UDCs. You can't make that assumption that charger type and gadget > state is enough. The real question is what do we need *now*, but still > keep in mind that what we need *now* might be valid 2 years from now, so > API needs to be a little flexible. Get your point, flexible, I just thought create an api without any user for existing case/spec, wouldn't it be better to let the real user add it later when it's needed. > > cheers > > -- > balbi -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, Jun Li <jun.li@nxp.com> writes: >> >> >> > Since we already have get_charger_type callback at usb_charger >> >> >> > structure, why we still need this API at usb_gadget_ops? >> >> >> >> >> >> In case some users want to get charger type at gadget level. >> >> >> >> >> > Why gadget needs to know charger type? I also don't catch the >> >> > intent of >> >> >> >> because some gadgets need to call usb_gadget_vbus_draw(), although >> >> for that they need power in mA rather. >> > >> > In below change of usb_gadget_vbus_draw(), already can get charger >> > type via usb_charger_get_type(). >> > >> > static inline int usb_gadget_vbus_draw(struct usb_gadget *gadget, >> > unsigned mA) { >> > + enum usb_charger_type type; >> > + >> > + if (gadget->charger) { >> > + type = usb_charger_get_type(gadget->charger); >> > + usb_charger_set_cur_limit_by_type(gadget->charger, type, >> mA); >> > + } >> > + >> > if (!gadget->ops->vbus_draw) >> > return -EOPNOTSUPP; >> > return gadget->ops->vbus_draw(gadget, mA); >> > >> > Could you detail in what situation gadget->ops-> get_charger_type() is >> used? >> >> isn't it right there in the code ? Isn't usb_gadget_vbus_draw() calling >> it ? What did I miss here ? > > Well, that's true, so my real meaning is why gadget need get charger type > via another new api gadget->ops->get_charger_type(). because of semantics. usb_gadget_vbus_draw() is *only* supposed to connect a load across vbus and gnd so some battery can be charged. Also, we need to abstract away this ->get_charger_type() operation because it might be different for each UDC. $subject has a fragility, however: It's assuming that we should always call ->get_charger_type() before ->vbus_draw(), but that's a good default, I'd say. >> >> > This api, as my understanding, gadget only need report gadget state >> >> changes. >> >> > All information required for usb charger is charger type and gadget >> >> state. >> >> >> >> you're making an assumption about how the HW is laid out which might >> >> not be true. >> >> >> > >> > What other information you refer to here? Or what I am not aware of? >> >> what I'm trying to say is that you're assuming gadgets don't need to know >> anything other than charger type and gadget state (suspended, resume, >> enumerated, default state, addressed, etc), but that might not be true for >> all UDCs. You can't make that assumption that charger type and gadget >> state is enough. The real question is what do we need *now*, but still >> keep in mind that what we need *now* might be valid 2 years from now, so >> API needs to be a little flexible. > > Get your point, flexible, I just thought create an api without any user > for existing case/spec, wouldn't it be better to let the real user add it > later when it's needed. that sure is a fair point.
On Wed, Apr 06, 2016 at 04:58:03PM +0300, Felipe Balbi wrote: > > Hi, > > Jun Li <jun.li@nxp.com> writes: > >> >> >> > Since we already have get_charger_type callback at usb_charger > >> >> >> > structure, why we still need this API at usb_gadget_ops? > >> >> >> > >> >> >> In case some users want to get charger type at gadget level. > >> >> >> > >> >> > Why gadget needs to know charger type? I also don't catch the > >> >> > intent of > >> >> > >> >> because some gadgets need to call usb_gadget_vbus_draw(), although > >> >> for that they need power in mA rather. > >> > > >> > In below change of usb_gadget_vbus_draw(), already can get charger > >> > type via usb_charger_get_type(). > >> > > >> > static inline int usb_gadget_vbus_draw(struct usb_gadget *gadget, > >> > unsigned mA) { > >> > + enum usb_charger_type type; > >> > + > >> > + if (gadget->charger) { > >> > + type = usb_charger_get_type(gadget->charger); > >> > + usb_charger_set_cur_limit_by_type(gadget->charger, type, > >> mA); > >> > + } > >> > + > >> > if (!gadget->ops->vbus_draw) > >> > return -EOPNOTSUPP; > >> > return gadget->ops->vbus_draw(gadget, mA); > >> > > >> > Could you detail in what situation gadget->ops-> get_charger_type() is > >> used? > >> > >> isn't it right there in the code ? Isn't usb_gadget_vbus_draw() calling > >> it ? What did I miss here ? > > > > Well, that's true, so my real meaning is why gadget need get charger type > > via another new api gadget->ops->get_charger_type(). > > because of semantics. usb_gadget_vbus_draw() is *only* supposed to > connect a load across vbus and gnd so some battery can be charged. Also, > we need to abstract away this ->get_charger_type() operation because it > might be different for each UDC. In this patch set, there are two ->get_charger_type in below two structures, as my understanding, get_charger_type at struct usb_charger can be implemented at UDC drivers; But I don't see necessary we need to implement get_charger_type for usb_gadget_ops at UDC drivers again. What do you think? struct usb_gadget_ops { .... struct usb_ep *(*match_ep)(struct usb_gadget *, + /* get the charger type */ + enum usb_charger_type (*get_charger_type)(struct usb_gadget *); }; struct usb_charger { ... + /* user can get charger type by implementing this callback */ + enum usb_charger_type (*get_charger_type)(struct usb_charger *); } > > $subject has a fragility, however: It's assuming that we should always > call ->get_charger_type() before ->vbus_draw(), but that's a good > default, I'd say. >
Hi, Peter Chen <hzpeterchen@gmail.com> writes: >> >> > In below change of usb_gadget_vbus_draw(), already can get charger >> >> > type via usb_charger_get_type(). >> >> > >> >> > static inline int usb_gadget_vbus_draw(struct usb_gadget *gadget, >> >> > unsigned mA) { >> >> > + enum usb_charger_type type; >> >> > + >> >> > + if (gadget->charger) { >> >> > + type = usb_charger_get_type(gadget->charger); >> >> > + usb_charger_set_cur_limit_by_type(gadget->charger, type, >> >> mA); >> >> > + } >> >> > + >> >> > if (!gadget->ops->vbus_draw) >> >> > return -EOPNOTSUPP; >> >> > return gadget->ops->vbus_draw(gadget, mA); >> >> > >> >> > Could you detail in what situation gadget->ops-> get_charger_type() is >> >> used? >> >> >> >> isn't it right there in the code ? Isn't usb_gadget_vbus_draw() calling >> >> it ? What did I miss here ? >> > >> > Well, that's true, so my real meaning is why gadget need get charger type >> > via another new api gadget->ops->get_charger_type(). >> >> because of semantics. usb_gadget_vbus_draw() is *only* supposed to >> connect a load across vbus and gnd so some battery can be charged. Also, >> we need to abstract away this ->get_charger_type() operation because it >> might be different for each UDC. > > In this patch set, there are two ->get_charger_type in below two > structures, as my understanding, get_charger_type at struct usb_charger > can be implemented at UDC drivers; But I don't see necessary we > need to implement get_charger_type for usb_gadget_ops at UDC drivers > again. What do you think? I had missed that completely, nice catch. I agree with you, there should be one place where this is implemented and struct usb_charger sounds like it is that place.
diff --git a/drivers/usb/gadget/udc/udc-core.c b/drivers/usb/gadget/udc/udc-core.c index b86a6f0..8d98c6b 100644 --- a/drivers/usb/gadget/udc/udc-core.c +++ b/drivers/usb/gadget/udc/udc-core.c @@ -28,6 +28,7 @@ #include <linux/usb/ch9.h> #include <linux/usb/gadget.h> #include <linux/usb.h> +#include <linux/usb/charger.h> /** * struct usb_udc - describes one usb device controller @@ -230,6 +231,9 @@ static void usb_gadget_state_work(struct work_struct *work) struct usb_gadget *gadget = work_to_gadget(work); struct usb_udc *udc = gadget->udc; + /* when the gadget state is changed, then report to USB charger */ + usb_charger_plug_by_gadget(gadget, gadget->state); + if (udc) sysfs_notify(&udc->dev.kobj, NULL, "state"); } @@ -423,8 +427,14 @@ int usb_add_gadget_udc_release(struct device *parent, struct usb_gadget *gadget, mutex_unlock(&udc_lock); + ret = usb_charger_init(gadget); + if (ret) + goto err5; + return 0; +err5: + device_del(&udc->dev); err4: list_del(&udc->list); mutex_unlock(&udc_lock); @@ -503,6 +513,7 @@ void usb_del_gadget_udc(struct usb_gadget *gadget) kobject_uevent(&udc->dev.kobj, KOBJ_REMOVE); flush_work(&gadget->work); device_unregister(&udc->dev); + usb_charger_exit(gadget); device_unregister(&gadget->dev); } EXPORT_SYMBOL_GPL(usb_del_gadget_udc); diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h index d82d006..054488a 100644 --- a/include/linux/usb/gadget.h +++ b/include/linux/usb/gadget.h @@ -24,6 +24,7 @@ #include <linux/types.h> #include <linux/workqueue.h> #include <linux/usb/ch9.h> +#include <linux/usb/charger.h> struct usb_ep; @@ -563,6 +564,8 @@ struct usb_gadget_ops { struct usb_ep *(*match_ep)(struct usb_gadget *, struct usb_endpoint_descriptor *, struct usb_ss_ep_comp_descriptor *); + /* get the charger type */ + enum usb_charger_type (*get_charger_type)(struct usb_gadget *); }; /** @@ -635,6 +638,8 @@ struct usb_gadget { unsigned out_epnum; unsigned in_epnum; struct usb_otg_caps *otg_caps; + /* negotiate the power with the usb charger */ + struct usb_charger *charger; unsigned sg_supported:1; unsigned is_otg:1; @@ -839,10 +844,20 @@ static inline int usb_gadget_vbus_connect(struct usb_gadget *gadget) * reporting how much power the device may consume. For example, this * could affect how quickly batteries are recharged. * + * It will also notify the USB charger how much power the device may + * consume if there is a USB charger linking with the gadget. + * * Returns zero on success, else negative errno. */ static inline int usb_gadget_vbus_draw(struct usb_gadget *gadget, unsigned mA) { + enum usb_charger_type type; + + if (gadget->charger) { + type = usb_charger_get_type(gadget->charger); + usb_charger_set_cur_limit_by_type(gadget->charger, type, mA); + } + if (!gadget->ops->vbus_draw) return -EOPNOTSUPP; return gadget->ops->vbus_draw(gadget, mA);
For supporting the usb charger, it adds the usb_charger_init() and usb_charger_exit() functions for usb charger initialization and exit. It will report to the usb charger when the gadget state is changed, then the usb charger can do the power things. Introduce a callback 'get_charger_type' which will implemented by user for usb gadget operations to get the usb charger type. Signed-off-by: Baolin Wang <baolin.wang@linaro.org> --- drivers/usb/gadget/udc/udc-core.c | 11 +++++++++++ include/linux/usb/gadget.h | 15 +++++++++++++++ 2 files changed, 26 insertions(+)