Re: [PATCH 01/10] power: charger-manager: Use thermal subsystem interface only to get temperature.

[Krzysztof KozÅowski]

On 30.10.2014 13:43, Jonghwa Lee wrote:
> It drops the way of using power_supply interface to reference battery's
> temperature. Then it tries to use thermal subsystem's only. This makes driver
> more simple and also can remove ifdeferies.
> 
> Signed-off-by: Jonghwa Lee <jonghwa3.lee@xxxxxxxxxxx>
> ---
>  drivers/power/Kconfig                 |    1 +
>  drivers/power/charger-manager.c       |  113 ++++++++-------------------------
>  include/linux/power/charger-manager.h |    3 +-
>  3 files changed, 28 insertions(+), 89 deletions(-)
> 
> diff --git a/drivers/power/Kconfig b/drivers/power/Kconfig
> index 8ff2511..115d153 100644
> --- a/drivers/power/Kconfig
> +++ b/drivers/power/Kconfig
> @@ -317,6 +317,7 @@ config CHARGER_MANAGER
>  	bool "Battery charger manager for multiple chargers"
>  	depends on REGULATOR
>  	select EXTCON
> +	select THERMAL

I think both of "select" here could be dangerous. Select should rather
be used for non-visible errors. Just use "depends on".

>  	help
>            Say Y to enable charger-manager support, which allows multiple
>            chargers attached to a battery and multiple batteries attached to a
> diff --git a/drivers/power/charger-manager.c b/drivers/power/charger-manager.c
> index 22246b9..b4b101c 100644
> --- a/drivers/power/charger-manager.c
> +++ b/drivers/power/charger-manager.c
> @@ -28,13 +28,6 @@
>  #include <linux/of.h>
>  #include <linux/thermal.h>
>  
> -/*
> - * Default termperature threshold for charging.
> - * Every temperature units are in tenth of centigrade.
> - */
> -#define CM_DEFAULT_RECHARGE_TEMP_DIFF	50
> -#define CM_DEFAULT_CHARGE_TEMP_MAX	500
> -
>  static const char * const default_event_names[] = {
>  	[CM_EVENT_UNKNOWN] = "Unknown",
>  	[CM_EVENT_BATT_FULL] = "Battery Full",
> @@ -572,40 +565,18 @@ static int check_charging_duration(struct charger_manager *cm)
>  	return ret;
>  }
>  
> -static int cm_get_battery_temperature_by_psy(struct charger_manager *cm,
> -					int *temp)
> -{
> -	struct power_supply *fuel_gauge;
> -
> -	fuel_gauge = power_supply_get_by_name(cm->desc->psy_fuel_gauge);
> -	if (!fuel_gauge)
> -		return -ENODEV;
> -
> -	return fuel_gauge->get_property(fuel_gauge,
> -				POWER_SUPPLY_PROP_TEMP,
> -				(union power_supply_propval *)temp);
> -}
> -
>  static int cm_get_battery_temperature(struct charger_manager *cm,
>  					int *temp)
>  {
>  	int ret;
>  
> -	if (!cm->desc->measure_battery_temp)
> +	if (!cm->tzd_batt)
>  		return -ENODEV;
>  
> -#ifdef CONFIG_THERMAL
> -	if (cm->tzd_batt) {
> -		ret = thermal_zone_get_temp(cm->tzd_batt, (unsigned long *)temp);
> -		if (!ret)
> -			/* Calibrate temperature unit */
> -			*temp /= 100;
> -	} else
> -#endif
> -	{
> -		/* if-else continued from CONFIG_THERMAL */
> -		ret = cm_get_battery_temperature_by_psy(cm, temp);
> -	}
> +	ret = thermal_zone_get_temp(cm->tzd_batt, (unsigned long *)temp);
> +	if (!ret)
> +		/* Calibrate temperature unit */
> +		*temp /= 100;
>  
>  	return ret;
>  }
> @@ -623,7 +594,7 @@ static int cm_check_thermal_status(struct charger_manager *cm)
>  		 * occur hazadous result. We have to handle it
>  		 * depending on battery type.
>  		 */
> -		dev_err(cm->dev, "Failed to get battery temperature\n");
> +		dev_dbg(cm->dev, "Failed to get battery temperature\n");

A valuable change but not strictly related to the commit. Additionally
that is a user-visible change. Could you split it to separate patch?

Best regards,
Krzysztof

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