Hi Neil, On 3 November 2016 at 09:25, NeilBrown <neilb@xxxxxxxx> wrote: > On Tue, Nov 01 2016, Baolin Wang wrote: > > >>> So I won't be responding on this topic any further until I see a genuine >>> attempt to understand and resolve the inconsistencies with >>> usb_register_notifier(). >> >> Any better solution? > > I'm not sure exactly what you are asking, so I'll assume you are asking > the question I want to answer :-) > > 1/ Liase with the extcon developers to resolve the inconsistencies > with USB connector types. > e.g. current there is both "EXTCON_USB" and "EXTCON_CHG_USB_SDP" > which both seem to suggest a standard downstream port. There is no > documentation describing how these relate, and no consistent practice > to copy. > I suspect the intention is that > EXTCON_USB and EXTCON_USB_HOST indicated that data capabilities of > the cable, while EXTCON_CHG_USB* indicate the power capabilities of > the cable. > So EXTCON_CHG_USB_SDP should always appear together with EXTCON_USB > while EXTCON_CHG_USB_DCP would not, and EXTCON_CHG_USB_ACA > would normally appear with EXTCON_USB_HOST (I think). > Some drivers follow this model, particularly extcon-max14577.c > but it is not consistent. > > This policy should be well documented and possibly existing drivers > should be updated to follow it. > > At the same time it would make sense to resolve EXTCON_CHG_USB_SLOW > and EXTCON_CHG_USB_FAST. These names don't mean much. > They were recently removed from drivers/power/axp288_charger.c > which is good, but are still used in drivers/extcon/extcon-max* > Possibly they should be changed to names from the standard, or > possibly they should be renamed to identify the current they are > expected to provide. e.g. EXTCON_CHG_USB_500MA and EXTCON_CHG_USB_1A Now I am creating the new patchset with fixing and converting exist drivers. I did some investigation about EXTCON subsystem. From your suggestion: 1. EXTCON_CHG_USB_SDP should always appear together with EXTCON_USB. ---- After checking, now all extcon drivers were following this rule. 2. EXTCON_CHG_USB_ACA would normally appear with EXTCON_USB_HOST. ---- Now no extcon drivers used EXTCON_CHG_USB_ACA, then no need to change. 3. Change EXTCON_CHG_USB_SLOW/FAST to EXTCON_CHG_USB_500MA/1A. ---- There are no model that shows the slow charger should be 500mA and fast charger is 1A. (In extcon-max77693.c file, the fast charger can be drawn 2A), so changing to EXTCON_CHG_USB_500MA/1A is not useful I think. >From above investigation, I did not find anything need to be changed now. What do you think? Thanks. > > 2/ Change all usb phys to register an extcon and to send appropriate > notifications. Many already do, but I don't think it is universal. > It is probable that the extcon should be registered using common code > instead of each phy driver having its own > extcon_get_edev_by_phandle() > or whatever. > If the usb phy driver needs to look at battery charger registers to > know what sort of cable was connected (which I believe is the case > for the chips you are interested in), then it should do that. > > 3/ Currently some USB controllers discover that a cable was connected by > listening on an extcon, and some by registering for a usb_notifier > (described below) ... though there seem to only be 2 left which do that. > Now that all USB phys send connection information via extcon (see 2), > the USB controllers should be changed to all find out about the cable > using extcon. > > 4/ struct usb_phy contains: > /* for notification of usb_phy_events */ > struct atomic_notifier_head notifier; > > This is used inconsistently. Sometimes the argument passed > is NULL, sometimes it is a pointer to 'vbus_draw' - the current > limited negotiated via USB, sometimes it is a pointer the the gadget > though as far as I can tell, that last one is never used. > > This should be changed to be consistent. This notifier is no longer > needed to tell the USB controller that a cable was connected (extcon > now does that, see 3) so it is only used to communicate the > 'vbus_draw' information. > So it should be changed to *only* send a notification when vbus_draw > is known, and it should carry that information. > This should probably be done in common code, and removed > from individual drivers. > > 5/ Now that all cable connection notifications are sent over extcon and > all vbus_draw notifications are sent over the usb_phy notifier, write > some support code that a power supply client can use to be told what > power is available. > e.g. a battery charger driver would call: > register_power_client(.....) > or similar, providing a phandle (or similar) for the usb phy and a > function to call back when the available current changes (or maybe a > work_struct containing the function pointer) > > register_power_client() would then register with extcon and separately > with the usb_phy notifier. When the different events arrive it > calculates what ranges of currents are expected and calls the > call-back function with those details. > > 6/ Any battery charger that needs to know the available current can now > call register_power_client() and get the information delivered. > > NeilBrown -- Baolin.wang Best Regards -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
