Hi Felipe,
>> 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
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.
>
> --
> Baolin.wang
> Best Regards
--
Baolin.wang
Best Regards
Hi Baolin,
Baolin Wang <[email protected]> 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
> 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?
--
balbi
Hi Felipe,
On 5 October 2016 at 15:47, Felipe Balbi <[email protected]> wrote:
>
> Hi Baolin,
>
> Baolin Wang <[email protected]> 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
>> 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
Sure. I will send out the new version with fixing these issues. Thanks.
> carefully. Perhaps report both minimum and maximum somehow?
>
> Neil, comments?
>
> --
> balbi
--
Baolin.wang
Best Regards
On Wed, Oct 05 2016, Felipe Balbi wrote:
> Hi Baolin,
>
> Baolin Wang <[email protected]> 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
Hi Neil,
On 5 October 2016 at 18:44, NeilBrown <[email protected]> wrote:
> On Wed, Oct 05 2016, Felipe Balbi wrote:
>
>> Hi Baolin,
>>
>> Baolin Wang <[email protected]> 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.
I transplanted the USB charger framework to our Spreadtrum platform
with implementing the 'get_charger_type' callback to get the charger
type in power driver. Cause we get the charger type from accessing the
PMIC registers not from USB PHY.
>
>>> 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.
Probably not. Now the USB charger framework tried to integrate all
different charger plugged/unplugged events, and all different charger
type getting methods, then noticed the plugged/unplugged events and
charger current to power driver, which I think that is what USB
charger should really do. Moreover, this patchset is reviewed and
helped by many people (thanks Felipe, Greg, Mark, Peter and Jun), I
really hope I can make it better to upstream.
>
> 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.
Not really. Like I said, some platform's charger detection is done by
hardware not USB PHY, thus we can get the charger type from PMIC
hardware registers.
>
>>> 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.
No. User can implement the get_charger_type() method to access the
PMIC registers to get the charger type, which is one very common
method.
>
>>> 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.
So, seems I should report both minimum and maximum.
>
> 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.
USB charger is one virtual device not one actual hardware device, we
should not mess it together with usb_notifier or extcon notifier.
>
> 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.
Usually only one USB charger in one system, I have not seen more than
one charger in a system.
>
> 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.
I think now the current/state/type attributes are enough, which are
USB chargger needed. Thanks.
--
Baolin.wang
Best Regards