On Wed, Oct 05 2016, Felipe Balbi wrote:

Hi Baolin,

Baolin Wang [off-list ref] writes:

quoted

quoted

quoted

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.

quoted

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:

quoted

1. Need to add the method getting charger type from extcon subsystem.

Yes.  This should be the only way to get the charger type.

quoted

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.

quoted

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