On Wed, Oct 05 2016, Felipe Balbi wrote: > Hi Baolin, > > Baolin Wang <baolin.wang@xxxxxxxxxx> writes: >>>> But you do! >>>> The mA number from the USB configuration is passed to usb_gadget_vbus_draw. >>>> Your patch passes that to usb_charger_set_cur_limit_by_type() >>>> which calls __usb_charger_set_cur_limit_by_type() which will set the >>>> cur_limit for whichever type uchger->type currently is. >>>> >>>> So when it is not relevant, your code *does* set some current limit. >>> >>> Suppose the charger type is DCP(it is not relevant to the mA number >>> from the USB configuration ), it will not do the USB enumeration, then >>> no USB configuration from host to set current. >> >> From the talking, there are some issues (thanks for Neil's comments) >> need to be fixed as below: >> 1. Need to add the method getting charger type from extcon subsystem. >> 2. Need to remove the method getting charger type from power supply. >> 3. There are still some different views about reporting the maximum >> current or minimum current to power driver. >> >> Now the current v16 patchset can work well on my Spreadtrum platform >> and Jun's NXP platform, if you like to apply this patchset then I can I'm really curious how much testing this has had. Have you actually plugged in different cable types (SDP DCP DCP ACA) and has each one been detected correctly? Because I cannot see how that could happen with the code you have posted. >> send out new patches to fix above issues. If you don't like that, I >> can send out new version patchset to fix above issues. Could you give >> me some suggestions what should I do next step? Thanks. > > Merge window just opened, nothing will happen for about 2 weeks. How > about you send a new version after merge window closes and we go from > there? Fixing 1 and 2 is needed. 3 we need to consider more > carefully. Perhaps report both minimum and maximum somehow? > > Neil, comments? This probably seems a bit harsh, but I really think the current patchset should be discarded and the the project started again with a clear vision of what is required. What we currently have is too confused. To respond to the points: >> 1. Need to add the method getting charger type from extcon subsystem. Yes. This should be the only way to get the charger type. >> 2. Need to remove the method getting charger type from power supply. Also need to remove the ->get_charger_type() method as there is no credible use-case for this. >> 3. There are still some different views about reporting the maximum >> current or minimum current to power driver. I think those were resolved. There was some confusion over whether a particular power manager wanted to be told the maximum or the minimum, but I think both have a clear use case in different hardware. Also: We don't want another notifier_chain. The usb_notifier combined with the extcon notifier are sufficient. Possibly it would be sensible to replace the usb notifier with a new new notifier chain, but don't add something without first cleaning up what is there. Also: resolve the question of whether it could ever make sense to have more than one "usb_charger" in a system. If it doesn't, make it an obvious singleton. If it does, make it clear how the correct usb_charger is chosen. Also: think very carefully before exposing any details through sysfs. Some of the details are already visible, either in sys/class/extcon or sys/class/power_supply. Don't duplicate without good reason. NeilBrown > > -- > balbi
Attachment:
signature.asc
Description: PGP signature
