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

Krzysztof Kozłowski
2014-10-30T13:11:42Z

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@samsung.com>
> ---
> 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