Hi > -----Original Message----- > From: Felipe Balbi [mailto:ba...@kernel.org] > Sent: Wednesday, April 06, 2016 8:56 PM > To: Jun Li <jun...@nxp.com>; Baolin Wang <baolin.w...@linaro.org>; Peter > Chen <hzpeterc...@gmail.com> > Cc: Greg KH <gre...@linuxfoundation.org>; Sebastian Reichel > <s...@kernel.org>; Dmitry Eremin-Solenikov <dbarysh...@gmail.com>; David > Woodhouse <dw...@infradead.org>; Peter Chen <peter.c...@freescale.com>; > Alan Stern <st...@rowland.harvard.edu>; r.bald...@samsung.com; Yoshihiro > Shimoda <yoshihiro.shimoda...@renesas.com>; Lee Jones > <lee.jo...@linaro.org>; Mark Brown <broo...@kernel.org>; Charles Keepax > <ckee...@opensource.wolfsonmicro.com>; patc...@opensource.wolfsonmicro.com; > Linux PM list <linux...@vger.kernel.org>; USB <linux-...@vger.kernel.org>; > device-mainlin...@lists.linuxfoundation.org; LKML <linux- > ker...@vger.kernel.org> > Subject: RE: [PATCH v9 2/4] gadget: Support for the usb charger framework > > > Hi, > > Jun Li <jun...@nxp.com> writes: > >> >> On 6 April 2016 at 15:19, Peter Chen <hzpeterc...@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
