Re: [PATCH 2/3] charger-manager : Add default battery temperature checking funtion.

[jonghwa3.lee@samsung.com]

On 2013년 10월 26일 08:44, Anton Vorontsov wrote:

> On Fri, Oct 25, 2013 at 11:47:32AM +0900, Jonghwa Lee wrote:
>> During the charger manager driver's probing time, it can't succeed
>> if there's no pre-defined .temperature_out_of_range callback function.
>> But if fuel gauge supports battery temperature measurement, we
>> can use it directly. That's what cm_default_get_temp() function does.
>>
>> With flag measure_batter_temp ON, we normally use cm_default_get_temp()
>> for .temperature_out_of_range callback funtion.
>> The TEMP_AMBIENT property is only used for pre-defined one.
>>
>> Signed-off-by: Jonghwa Lee <jonghwa3.lee@samsung.com>
>> Signed-off-by: Myungjoo Ham <myungjoo.ham@samsung.com>
>> ---
> ...
>> +static int cm_default_get_temp(struct charger_manager *cm)
>> +{
>> +	struct charger_desc *desc = cm->desc;
>> +	union power_supply_propval val;
>> +	static int temp_alert_min = 0;
> 
> No. Never.


Can I assume that you worried about initialization of a variable which would be
used to hold minimum value to zero not INT_MAX or something big?
Anyway, come to think of it, it looks ugly for me either. I'll fix it.

> 
>> +	static int temp_alert_max = 0;
>> +	static int temp_alert_diff = 0;
>> +	static int last_temp_status = 0;
>> +	int ret;
>> +
>> +	if (!temp_alert_min && !temp_alert_max) {
>> +		/* Initialize minimum temperature for alert */
>> +		ret = cm->fuel_gauge->get_property(cm->fuel_gauge,
>> +					POWER_SUPPLY_PROP_TEMP_ALERT_MIN,
>> +					&val);
>> +		temp_alert_min = ret ? desc->temp_alert_min :
>> +					min(desc->temp_alert_min, val.intval);
> 
> Is it a minimum temperature of a bettery below which we should alert?
> Should it be max() then?


Yes, you're right. I'll fix it.

> 
>> +		if (!temp_alert_min)
>> +			temp_alert_min = CM_DEFAULT_TEMP_ALERT_MIN;
> 
> The whole function is ugly as hell.  Please tidy it up, split it into two,
> three or whatever amount needed to make it readable and comprehendable.
> Use temporary variables for things like
> cm->fuel_gauge->get_property(cm->fuel_gauge, ....).
> 


Okay, I'll re-post it.

Thanks for comments
Jonghwa.

>> +
>> +		/* Initialize maximum temperature for alert */
>> +		ret = cm->fuel_gauge->get_property(cm->fuel_gauge,
>> +					POWER_SUPPLY_PROP_TEMP_ALERT_MAX,
>> +					&val);
>> +		temp_alert_max = ret ? desc->temp_alert_max :
>> +					min(desc->temp_alert_max, val.intval);
> 
> Thanks,
> 
> Anton
>