linux-kernel - Re: [PATCH 1/3] power: charger-manager: Replace deprecatd API of extcon

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 31 Mar 2016 17:35:16 +0900
From:	Chanwoo Choi <cw00.choi@...sung.com>
To:	Rob Herring <robh@...nel.org>
Cc:	sre@...nel.org, dbaryshkov@...il.com, dwmw2@...radead.org,
	myungjoo.ham@...sung.com, linux-pm@...r.kernel.org,
	linux-kernel@...r.kernel.org, devicetree@...r.kernel.org
Subject: Re: [PATCH 1/3] power: charger-manager: Replace deprecatd API of extcon

Hi Rob,

On 2016년 03월 29일 06:07, Rob Herring wrote:
> On Mon, Mar 28, 2016 at 08:25:40AM +0900, Chanwoo Choi wrote:
>> This patch removes the deprecated notifier API of extcon framework and then use
>> the new extcon API[2] with the unique id[1] to indicate the each external
>> connector. Alter deprecated API as following:
>> - extcon_register_interest() -> extcon_register_notifier()
>> - extcon_unregister_interest() -> extcon_unregister_notifier()
>>
>> And, extcon alters the name of USB charger connector in patch[3] as following:
>> - EXTCON_CHG_USB_SDP /* Standard Downstream Port */
>> - EXTCON_CHG_USB_DCP /* Dedicated Charging Port */
>> - EXTCON_CHG_USB_CDP /* Charging Downstream Port */
>> - EXTCON_CHG_USB_ACA /* Accessory Charger Adapter */
>>
>> So, the name of external charger connector are changed as following:
>> - "USB" -> "SDP"
>> - "TA" -> "DCP"
>>
>> [1] Commit 2a9de9c0f08d61
>> - ("extcon: Use the unique id for external connector instead of string)
>> [2] Commit 046050f6e623e4
>> - ("extcon: Update the prototype of extcon_register_notifier() with enum extcon
>> [3] Commit 11eecf910bd81d
>> - ("extcon: Modify the id and name of external connector")
>>
>> Signed-off-by: Chanwoo Choi <cw00.choi@...sung.com>
>> ---
>>  .../bindings/power_supply/charger-manager.txt      |  4 +--
>>  drivers/power/charger-manager.c                    | 31 ++++++++++++++--------
>>  include/linux/power/charger-manager.h              |  4 +--
>>  3 files changed, 24 insertions(+), 15 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/power_supply/charger-manager.txt b/Documentation/devicetree/bindings/power_supply/charger-manager.txt
>> index ec4fe9de3137..73193e380dc2 100644
>> --- a/Documentation/devicetree/bindings/power_supply/charger-manager.txt
>> +++ b/Documentation/devicetree/bindings/power_supply/charger-manager.txt
>> @@ -65,13 +65,13 @@ Example :
>>  		regulator@0 {
>>  			cm-regulator-name = "chg-reg";
>>  			cable@0 {
>> -				cm-cable-name = "USB";
>> +				cm-cable-id = 5; /* EXTCON_CHG_USB_SDP */
> 
> Sorry, but delete the charger-manager binding and start over. It looks 
> designed around the needs of Linux, not what the h/w looks like. You're 

I agree. The charger-manager didn't express the h/w. IMHO, charging
framework may be included in the power_supply core such as fuel-gauge device.
Just it is my opinion.

For example, fuel-gauge device may include the connected device such as
charger device (battery charging), extcon device(detect the type of charger cable)
and rtc device (check the battery state periodically with sample rate) for battery charging.

I agree that we should handle the charger-manager binding.

But now,
this patch just removes the dependency of deprecated API of extcon.
So, I think that the issue of charger-manager binding should be handled
in other patches.

> changing the driver and having to change the DT in a non-compatible way 
> is the first clue.
> 
>>  				cm-cable-extcon = "extcon-dev.0";
> 
> What is this!? The linux device name?

Just the name of extcon device to detect the type of charger cable.
But, extcon framekwork has the extcon_get_edev_by_phandle() to get the
instance by using the phandle in Device Tree.

> 
>>  				cm-cable-min = <475000>;
>>  				cm-cable-max = <500000>;
> 
> This is set by the USB spec...

I agree. we need to modify it.

> 
>>  			};
>>  			cable@1 {
>> -				cm-cable-name = "TA";
>> +				cm-cable-id = 6; /* EXTCON_CHG_USB_DCP */
>>  				cm-cable-extcon = "extcon-dev.0";
>>  				cm-cable-min = <650000>;
>>  				cm-cable-max = <675000>;
> 
> 
> 


Best Regards,
Chanwoo Choi