linux-kernel - Re: [PATCH 3/5] charger-manager: Add support sysfs entry for charger

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] [day] [month] [year] [list]

Message-id: <505C1673.50603@samsung.com>
Date:	Fri, 21 Sep 2012 16:25:39 +0900
From:	Chanwoo Choi <cw00.choi@...sung.com>
To:	Anton Vorontsov <anton.vorontsov@...aro.org>
Cc:	jenny.tc@...el.com, ramakrishna.pallala@...el.com,
	myungjoo.ham@...sung.com, kyungmin.park@...sung.com,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/5] charger-manager: Add support sysfs entry for charger

On 09/21/2012 02:12 PM, Anton Vorontsov wrote:

> On Tue, Aug 21, 2012 at 05:06:49PM +0900, Chanwoo Choi wrote:
>> This patch add support sysfs entry for each charger(regulator).
>> Charger-manager use one or more chargers for charging battery but
>> some charger isn't necessary on specific scenario. So, if some charger
>> isn't needed, can disable specific charger through 'externally_control'
>> entry while system is on state and confirm the information(name, state)
>> of charger.
>>
>> the list of added sysfs entry
>> - /sys/class/power_supply/battery/chargers/charger.[index]/name
>> : show name of charger(regulator)
>> - /sys/class/power_supply/battery/chargers/charger.[index]/state
>> : show either enabled or disabled state of charger
>> - /sys/class/power_supply/battery/chargers/charger.[index]/externally_control
> 
> The API looks sane.
> 
> For the future, you might want to get rid of the 'name', and instead
> just point to a regulator device (via a sysfs symlink). I.e.
> 
> /sys/class/power_supply/battery/chargers/charger.[index]/device
> would be a symlink to the regulator device.
> 
> But for the time being, I guess it's OK as is (although I wouldn't
> mind if it'll use the symlink from the start. :-)


OK, as your said, I will modify sysfs entry for charger(regulator) by
linking sysfs
of regulator device.

> 
> [...]
>>  		for (j = 0 ; j < charger->num_cables ; j++) {
>>  			struct charger_cable *cable = &charger->cables[j];
>> @@ -1287,6 +1386,71 @@ static int charger_manager_probe(struct platform_device *pdev)
>>  			cable->charger = charger;
>>  			cable->cm = cm;
>>  		}
>> +
> [...]
>> +		charger->attr_g.attrs = charger->attrs;
>> +
>> +		sysfs_attr_init(&cable->attr_name.attr);
> 
> Notice that 'cable' is declared in the 'for' loop above,
> so this doesn't compile for me:
> 
>   CHECK   drivers/power/charger-manager.c
> drivers/power/charger-manager.c:1559:17: error: undefined identifier 'cable'
> drivers/power/charger-manager.c:1564:17: error: undefined identifier 'cable'
> drivers/power/charger-manager.c:1569:17: error: undefined identifier 'cable'
>   CC      drivers/power/charger-manager.o
> drivers/power/charger-manager.c: In function ‘charger_manager_probe’:
> drivers/power/charger-manager.c:1559:3: error: ‘cable’ undeclared (first use in this function)
> drivers/power/charger-manager.c:1559:3: note: each undeclared identifier is reported only once for each function it appears in
> make[1]: *** [drivers/power/charger-manager.o] Error 1
> 

I will fix it.

> Also:
> - Please adhere to the codingstyle, there should be no spaces
>   before ';' in the for loop statement.
> - If possible, please consider splitting _probe routine, it is more
>   than 300 lines long nowadays.
> 


I resend soon patch which fix build break. As you said, I will modify
sysfs entry
for charger(regulator) by linking sysfs of regulator device and split _probe
routine.

Thanks for your comment.

Best Regards,
Chanwoo Choi





--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/