Hi > -----Original Message----- > From: Baolin Wang [mailto:baolin.wang@xxxxxxxxxx] > Sent: Monday, April 11, 2016 7:15 PM > To: Jun Li <jun.li@xxxxxxx> > Cc: balbi@xxxxxxxxxx; gregkh@xxxxxxxxxxxxxxxxxxx; sre@xxxxxxxxxx; > dbaryshkov@xxxxxxxxx; dwmw2@xxxxxxxxxxxxx; peter.chen@xxxxxxxxxxxxx; > stern@xxxxxxxxxxxxxxxxxxx; r.baldyga@xxxxxxxxxxx; > yoshihiro.shimoda.uh@xxxxxxxxxxx; lee.jones@xxxxxxxxxx; broonie@xxxxxxxxxx; > ckeepax@xxxxxxxxxxxxxxxxxxxxxxxxxxx; patches@xxxxxxxxxxxxxxxxxxxxxxxxxxx; > linux-pm@xxxxxxxxxxxxxxx; linux-usb@xxxxxxxxxxxxxxx; device- > mainlining@xxxxxxxxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx > 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 ��.n��������+%������w��{.n�����{���)�� jg��� �����ݢj����G���� ���j:+v���w�m������w�������h�����٥
