Re: [PATCH v9 2/4] gadget: Support for the usb charger framework

[Peter Chen]

On Wed, Apr 06, 2016 at 04:58:03PM +0300, Felipe Balbi wrote:
> 
> Hi,
> 
> Jun Li <jun.li@xxxxxxx> 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.
> 

-- 

Best Regards,
Peter Chen