diff mbox series

power: supply: disable faulty cooling logic

Message ID 20230121111621.2821558-1-andreas@kemnade.info (mailing list archive)
State Handled Elsewhere, archived
Headers show
Series power: supply: disable faulty cooling logic | expand

Commit Message

Andreas Kemnade Jan. 21, 2023, 11:16 a.m. UTC 
The rn5t618 power driver fails to register
a cooling device because POWER_SUPPLY_PROP_CHARGE_CONTROL_LIMIT_MAX
is missing but availability is not checked before registering
cooling device. After improved error checking in the thermal
code, the registration of the power supply fails entirely.

Checking for availability of _MAX before registering cooling device
fixes the rn5t618 problem. But the whole logic feels questionable.

First, the logic is inverted here:
the code tells: max_current = max_cooling but
0 = max_cooling, so there needs to be some inversion
in the code which cannot be found. Comparing with other
cooling devices, it can be found that value for fan speed is not
inverted, value for cpufreq cooling is inverted (similar situation
as here lowest frequency = max cooling)

Second, analyzing usage of _MAX: it is seems that maximum capabilities
of charging controller are specified and not of the battery. Probably
there is not too much mismatch in the drivers actually implementing
that. So nothing has exploded yet.  So there is no easy and safe way
to specifify a max cooling value now.

Conclusion for now (as a regression fix) just remove the cooling device
registration and do it properly later on.

Fixes: e49a1e1ee078 ("thermal/core: fix error code in __thermal_cooling_device_register()")
Fixes: 952aeeb3ee28 ("power_supply: Register power supply for thermal cooling device")
Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
---
 drivers/power/supply/power_supply_core.c | 93 ------------------------
 1 file changed, 93 deletions(-)

Comments

Sebastian Reichel Feb. 3, 2023, 12:28 p.m. UTC | #1 
Hi,

On Sat, Jan 21, 2023 at 12:16:21PM +0100, Andreas Kemnade wrote:
> The rn5t618 power driver fails to register
> a cooling device because POWER_SUPPLY_PROP_CHARGE_CONTROL_LIMIT_MAX
> is missing but availability is not checked before registering
> cooling device. After improved error checking in the thermal
> code, the registration of the power supply fails entirely.
> 
> Checking for availability of _MAX before registering cooling device
> fixes the rn5t618 problem. But the whole logic feels questionable.
> 
> First, the logic is inverted here:
> the code tells: max_current = max_cooling but
> 0 = max_cooling, so there needs to be some inversion
> in the code which cannot be found. Comparing with other
> cooling devices, it can be found that value for fan speed is not
> inverted, value for cpufreq cooling is inverted (similar situation
> as here lowest frequency = max cooling)
> 
> Second, analyzing usage of _MAX: it is seems that maximum capabilities
> of charging controller are specified and not of the battery. Probably
> there is not too much mismatch in the drivers actually implementing
> that. So nothing has exploded yet.  So there is no easy and safe way
> to specifify a max cooling value now.
> 
> Conclusion for now (as a regression fix) just remove the cooling device
> registration and do it properly later on.
> 
> Fixes: e49a1e1ee078 ("thermal/core: fix error code in __thermal_cooling_device_register()")
> Fixes: 952aeeb3ee28 ("power_supply: Register power supply for thermal cooling device")
> Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
> ---

Thanks, queued to for-next.

-- Sebastian

>  drivers/power/supply/power_supply_core.c | 93 ------------------------
>  1 file changed, 93 deletions(-)
> 
> diff --git a/drivers/power/supply/power_supply_core.c b/drivers/power/supply/power_supply_core.c
> index 7c790c41e2fe..cc5b2e22b42a 100644
> --- a/drivers/power/supply/power_supply_core.c
> +++ b/drivers/power/supply/power_supply_core.c
> @@ -1186,83 +1186,6 @@ static void psy_unregister_thermal(struct power_supply *psy)
>  	thermal_zone_device_unregister(psy->tzd);
>  }
>  
> -/* thermal cooling device callbacks */
> -static int ps_get_max_charge_cntl_limit(struct thermal_cooling_device *tcd,
> -					unsigned long *state)
> -{
> -	struct power_supply *psy;
> -	union power_supply_propval val;
> -	int ret;
> -
> -	psy = tcd->devdata;
> -	ret = power_supply_get_property(psy,
> -			POWER_SUPPLY_PROP_CHARGE_CONTROL_LIMIT_MAX, &val);
> -	if (ret)
> -		return ret;
> -
> -	*state = val.intval;
> -
> -	return ret;
> -}
> -
> -static int ps_get_cur_charge_cntl_limit(struct thermal_cooling_device *tcd,
> -					unsigned long *state)
> -{
> -	struct power_supply *psy;
> -	union power_supply_propval val;
> -	int ret;
> -
> -	psy = tcd->devdata;
> -	ret = power_supply_get_property(psy,
> -			POWER_SUPPLY_PROP_CHARGE_CONTROL_LIMIT, &val);
> -	if (ret)
> -		return ret;
> -
> -	*state = val.intval;
> -
> -	return ret;
> -}
> -
> -static int ps_set_cur_charge_cntl_limit(struct thermal_cooling_device *tcd,
> -					unsigned long state)
> -{
> -	struct power_supply *psy;
> -	union power_supply_propval val;
> -	int ret;
> -
> -	psy = tcd->devdata;
> -	val.intval = state;
> -	ret = psy->desc->set_property(psy,
> -		POWER_SUPPLY_PROP_CHARGE_CONTROL_LIMIT, &val);
> -
> -	return ret;
> -}
> -
> -static const struct thermal_cooling_device_ops psy_tcd_ops = {
> -	.get_max_state = ps_get_max_charge_cntl_limit,
> -	.get_cur_state = ps_get_cur_charge_cntl_limit,
> -	.set_cur_state = ps_set_cur_charge_cntl_limit,
> -};
> -
> -static int psy_register_cooler(struct power_supply *psy)
> -{
> -	/* Register for cooling device if psy can control charging */
> -	if (psy_has_property(psy->desc, POWER_SUPPLY_PROP_CHARGE_CONTROL_LIMIT)) {
> -		psy->tcd = thermal_cooling_device_register(
> -			(char *)psy->desc->name,
> -			psy, &psy_tcd_ops);
> -		return PTR_ERR_OR_ZERO(psy->tcd);
> -	}
> -
> -	return 0;
> -}
> -
> -static void psy_unregister_cooler(struct power_supply *psy)
> -{
> -	if (IS_ERR_OR_NULL(psy->tcd))
> -		return;
> -	thermal_cooling_device_unregister(psy->tcd);
> -}
>  #else
>  static int psy_register_thermal(struct power_supply *psy)
>  {
> @@ -1272,15 +1195,6 @@ static int psy_register_thermal(struct power_supply *psy)
>  static void psy_unregister_thermal(struct power_supply *psy)
>  {
>  }
> -
> -static int psy_register_cooler(struct power_supply *psy)
> -{
> -	return 0;
> -}
> -
> -static void psy_unregister_cooler(struct power_supply *psy)
> -{
> -}
>  #endif
>  
>  static struct power_supply *__must_check
> @@ -1354,10 +1268,6 @@ __power_supply_register(struct device *parent,
>  	if (rc)
>  		goto register_thermal_failed;
>  
> -	rc = psy_register_cooler(psy);
> -	if (rc)
> -		goto register_cooler_failed;
> -
>  	rc = power_supply_create_triggers(psy);
>  	if (rc)
>  		goto create_triggers_failed;
> @@ -1387,8 +1297,6 @@ __power_supply_register(struct device *parent,
>  add_hwmon_sysfs_failed:
>  	power_supply_remove_triggers(psy);
>  create_triggers_failed:
> -	psy_unregister_cooler(psy);
> -register_cooler_failed:
>  	psy_unregister_thermal(psy);
>  register_thermal_failed:
>  wakeup_init_failed:
> @@ -1540,7 +1448,6 @@ void power_supply_unregister(struct power_supply *psy)
>  	sysfs_remove_link(&psy->dev.kobj, "powers");
>  	power_supply_remove_hwmon_sysfs(psy);
>  	power_supply_remove_triggers(psy);
> -	psy_unregister_cooler(psy);
>  	psy_unregister_thermal(psy);
>  	device_init_wakeup(&psy->dev, false);
>  	device_unregister(&psy->dev);
> -- 
> 2.30.2
>
diff mbox series

Patch

diff --git a/drivers/power/supply/power_supply_core.c b/drivers/power/supply/power_supply_core.c
index 7c790c41e2fe..cc5b2e22b42a 100644
--- a/drivers/power/supply/power_supply_core.c
+++ b/drivers/power/supply/power_supply_core.c
@@ -1186,83 +1186,6 @@  static void psy_unregister_thermal(struct power_supply *psy)
 	thermal_zone_device_unregister(psy->tzd);
 }
 
-/* thermal cooling device callbacks */
-static int ps_get_max_charge_cntl_limit(struct thermal_cooling_device *tcd,
-					unsigned long *state)
-{
-	struct power_supply *psy;
-	union power_supply_propval val;
-	int ret;
-
-	psy = tcd->devdata;
-	ret = power_supply_get_property(psy,
-			POWER_SUPPLY_PROP_CHARGE_CONTROL_LIMIT_MAX, &val);
-	if (ret)
-		return ret;
-
-	*state = val.intval;
-
-	return ret;
-}
-
-static int ps_get_cur_charge_cntl_limit(struct thermal_cooling_device *tcd,
-					unsigned long *state)
-{
-	struct power_supply *psy;
-	union power_supply_propval val;
-	int ret;
-
-	psy = tcd->devdata;
-	ret = power_supply_get_property(psy,
-			POWER_SUPPLY_PROP_CHARGE_CONTROL_LIMIT, &val);
-	if (ret)
-		return ret;
-
-	*state = val.intval;
-
-	return ret;
-}
-
-static int ps_set_cur_charge_cntl_limit(struct thermal_cooling_device *tcd,
-					unsigned long state)
-{
-	struct power_supply *psy;
-	union power_supply_propval val;
-	int ret;
-
-	psy = tcd->devdata;
-	val.intval = state;
-	ret = psy->desc->set_property(psy,
-		POWER_SUPPLY_PROP_CHARGE_CONTROL_LIMIT, &val);
-
-	return ret;
-}
-
-static const struct thermal_cooling_device_ops psy_tcd_ops = {
-	.get_max_state = ps_get_max_charge_cntl_limit,
-	.get_cur_state = ps_get_cur_charge_cntl_limit,
-	.set_cur_state = ps_set_cur_charge_cntl_limit,
-};
-
-static int psy_register_cooler(struct power_supply *psy)
-{
-	/* Register for cooling device if psy can control charging */
-	if (psy_has_property(psy->desc, POWER_SUPPLY_PROP_CHARGE_CONTROL_LIMIT)) {
-		psy->tcd = thermal_cooling_device_register(
-			(char *)psy->desc->name,
-			psy, &psy_tcd_ops);
-		return PTR_ERR_OR_ZERO(psy->tcd);
-	}
-
-	return 0;
-}
-
-static void psy_unregister_cooler(struct power_supply *psy)
-{
-	if (IS_ERR_OR_NULL(psy->tcd))
-		return;
-	thermal_cooling_device_unregister(psy->tcd);
-}
 #else
 static int psy_register_thermal(struct power_supply *psy)
 {
@@ -1272,15 +1195,6 @@  static int psy_register_thermal(struct power_supply *psy)
 static void psy_unregister_thermal(struct power_supply *psy)
 {
 }
-
-static int psy_register_cooler(struct power_supply *psy)
-{
-	return 0;
-}
-
-static void psy_unregister_cooler(struct power_supply *psy)
-{
-}
 #endif
 
 static struct power_supply *__must_check
@@ -1354,10 +1268,6 @@  __power_supply_register(struct device *parent,
 	if (rc)
 		goto register_thermal_failed;
 
-	rc = psy_register_cooler(psy);
-	if (rc)
-		goto register_cooler_failed;
-
 	rc = power_supply_create_triggers(psy);
 	if (rc)
 		goto create_triggers_failed;
@@ -1387,8 +1297,6 @@  __power_supply_register(struct device *parent,
 add_hwmon_sysfs_failed:
 	power_supply_remove_triggers(psy);
 create_triggers_failed:
-	psy_unregister_cooler(psy);
-register_cooler_failed:
 	psy_unregister_thermal(psy);
 register_thermal_failed:
 wakeup_init_failed:
@@ -1540,7 +1448,6 @@  void power_supply_unregister(struct power_supply *psy)
 	sysfs_remove_link(&psy->dev.kobj, "powers");
 	power_supply_remove_hwmon_sysfs(psy);
 	power_supply_remove_triggers(psy);
-	psy_unregister_cooler(psy);
 	psy_unregister_thermal(psy);
 	device_init_wakeup(&psy->dev, false);
 	device_unregister(&psy->dev);