* Re: [PATCH] power_supply: Register battery as a thermal zone
2012-05-09 11:18 [PATCH] power_supply: Register battery as a thermal zone Jenny TC
@ 2012-05-09 6:16 ` Ryan Mallon
2012-05-09 9:33 ` Tc, Jenny
0 siblings, 1 reply; 3+ messages in thread
From: Ryan Mallon @ 2012-05-09 6:16 UTC (permalink / raw)
To: Jenny TC; +Cc: cbouatmailru, anton.vorontsov, linux-kernel, durgadoss.r
On 09/05/12 21:18, Jenny TC wrote:
> Battery and charger contribute to Thermals in most of the embedded
> devices. So, it makes sense to identify them as Thermal zones in a
> particular platform.
>
> This patch registers a thermal zone if the power supply is reporting
> a temperature property. The thermal zone will be used by platform's
> thermal management solution.
>
> Signed-off-by: Jenny TC <jenny.tc@intel.com>
> ---
> +#ifdef CONFIG_THERMAL
> +static int power_supply_read_temp(struct thermal_zone_device *tzd,
> + unsigned long *temp)
> +{
> + struct power_supply *psy;
> + union power_supply_propval val;
> + int ret;
> +
> + WARN_ON(tzd == NULL);
> + psy = tzd->devdata;
> + WARN_ON(psy == NULL);
These WARN_ONs seem unnecessary since you will oops if either of them
are NULL anyway.
> + ret = psy->get_property(psy,
> + POWER_SUPPLY_PROP_TEMP, &val);
> + if (!ret)
> + *temp = val.intval * 100;
> + return ret;
> +}
<snip>
> int power_supply_register(struct device *parent, struct power_supply *psy)
> {
> struct device *dev;
> int rc;
> +#ifdef CONFIG_THERMAL
> + int i;
> +#endif
>
> dev = kzalloc(sizeof(*dev), GFP_KERNEL);
> if (!dev)
> @@ -196,7 +223,21 @@ int power_supply_register(struct device *parent, struct power_supply *psy)
> rc = device_add(dev);
> if (rc)
> goto device_add_failed;
> -
> +#ifdef CONFIG_THERMAL
> + /* Register battery zone device psy reports temperature */
> + for (i = 0; i < psy->num_properties; i++) {
> + if (psy->properties[i] == POWER_SUPPLY_PROP_TEMP) {
> + psy->tzd = thermal_zone_device_register(
> + (char *)psy->name, 0, psy,
> + &psy_tzd_ops, 0, 0, 0, 0);
> + if (IS_ERR(psy->tzd)) {
> + rc = PTR_ERR(psy->tzd);
> + goto therm_zone_reg_failed;
> + }
> + break;
> + }
> + }
> +#endif
This would be better moved into its own function, so you can minimise
the amount of ifdefs needed. Having extra ifdefs for variable
declarations and exit paths is especially ugly :-/. Ideally you can have
a single #ifdef CONFIG_THERMAL block, and define empty functions for the
!CONFIG_THERMAL case.
~Ryan
^ permalink raw reply [flat|nested] 3+ messages in thread
* RE: [PATCH] power_supply: Register battery as a thermal zone
2012-05-09 6:16 ` Ryan Mallon
@ 2012-05-09 9:33 ` Tc, Jenny
0 siblings, 0 replies; 3+ messages in thread
From: Tc, Jenny @ 2012-05-09 9:33 UTC (permalink / raw)
To: Ryan Mallon
Cc: cbouatmailru@gmail.com, anton.vorontsov@linaro.org,
linux-kernel@vger.kernel.org, R, Durgadoss
>
> > Battery and charger contribute to Thermals in most of the embedded
> > devices. So, it makes sense to identify them as Thermal zones in a
> > particular platform.
> >
> > This patch registers a thermal zone if the power supply is reporting a
> > temperature property. The thermal zone will be used by platform's
> > thermal management solution.
> >
> > Signed-off-by: Jenny TC <jenny.tc@intel.com>
> > ---
>
> > +#ifdef CONFIG_THERMAL
> > +static int power_supply_read_temp(struct thermal_zone_device *tzd,
> > + unsigned long *temp)
> > +{
> > + struct power_supply *psy;
> > + union power_supply_propval val;
> > + int ret;
> > +
> > + WARN_ON(tzd == NULL);
> > + psy = tzd->devdata;
> > + WARN_ON(psy == NULL);
>
>
> These WARN_ONs seem unnecessary since you will oops if either of them
> are NULL anyway.
>
Agreed.
> > + ret = psy->get_property(psy,
> > + POWER_SUPPLY_PROP_TEMP, &val);
> > + if (!ret)
> > + *temp = val.intval * 100;
> > + return ret;
> > +}
>
>
> <snip>
>
> > int power_supply_register(struct device *parent, struct power_supply
> > *psy) {
> > struct device *dev;
> > int rc;
> > +#ifdef CONFIG_THERMAL
> > + int i;
> > +#endif
> >
> > dev = kzalloc(sizeof(*dev), GFP_KERNEL);
> > if (!dev)
> > @@ -196,7 +223,21 @@ int power_supply_register(struct device *parent,
> struct power_supply *psy)
> > rc = device_add(dev);
> > if (rc)
> > goto device_add_failed;
> > -
> > +#ifdef CONFIG_THERMAL
> > + /* Register battery zone device psy reports temperature */
> > + for (i = 0; i < psy->num_properties; i++) {
> > + if (psy->properties[i] == POWER_SUPPLY_PROP_TEMP) {
> > + psy->tzd = thermal_zone_device_register(
> > + (char *)psy->name, 0, psy,
> > + &psy_tzd_ops, 0, 0, 0, 0);
> > + if (IS_ERR(psy->tzd)) {
> > + rc = PTR_ERR(psy->tzd);
> > + goto therm_zone_reg_failed;
> > + }
> > + break;
> > + }
> > + }
> > +#endif
>
>
> This would be better moved into its own function, so you can minimise the
> amount of ifdefs needed. Having extra ifdefs for variable declarations and
> exit paths is especially ugly :-/. Ideally you can have a single #ifdef
> CONFIG_THERMAL block, and define empty functions for the
> !CONFIG_THERMAL case.
>
Agreed. I will submit another patch in a few minutes.
> ~Ryan
^ permalink raw reply [flat|nested] 3+ messages in thread
* [PATCH] power_supply: Register battery as a thermal zone
@ 2012-05-09 11:18 Jenny TC
2012-05-09 6:16 ` Ryan Mallon
0 siblings, 1 reply; 3+ messages in thread
From: Jenny TC @ 2012-05-09 11:18 UTC (permalink / raw)
To: cbouatmailru, anton.vorontsov; +Cc: linux-kernel, durgadoss.r, Jenny TC
Battery and charger contribute to Thermals in most of the embedded
devices. So, it makes sense to identify them as Thermal zones in a
particular platform.
This patch registers a thermal zone if the power supply is reporting
a temperature property. The thermal zone will be used by platform's
thermal management solution.
Signed-off-by: Jenny TC <jenny.tc@intel.com>
---
drivers/power/power_supply_core.c | 52 ++++++++++++++++++++++++++++++++++++-
include/linux/power_supply.h | 3 ++
2 files changed, 54 insertions(+), 1 deletions(-)
diff --git a/drivers/power/power_supply_core.c b/drivers/power/power_supply_core.c
index 6ad6127..ea690ba 100644
--- a/drivers/power/power_supply_core.c
+++ b/drivers/power/power_supply_core.c
@@ -17,6 +17,7 @@
#include <linux/device.h>
#include <linux/err.h>
#include <linux/power_supply.h>
+#include <linux/thermal.h>
#include "power_supply.h"
/* exported for the APM Power driver, APM emulation */
@@ -169,10 +170,36 @@ static void power_supply_dev_release(struct device *dev)
kfree(dev);
}
+#ifdef CONFIG_THERMAL
+static int power_supply_read_temp(struct thermal_zone_device *tzd,
+ unsigned long *temp)
+{
+ struct power_supply *psy;
+ union power_supply_propval val;
+ int ret;
+
+ WARN_ON(tzd == NULL);
+ psy = tzd->devdata;
+ WARN_ON(psy == NULL);
+ ret = psy->get_property(psy,
+ POWER_SUPPLY_PROP_TEMP, &val);
+ if (!ret)
+ *temp = val.intval * 100;
+ return ret;
+}
+
+static struct thermal_zone_device_ops psy_tzd_ops = {
+ .get_temp = power_supply_read_temp,
+};
+#endif
+
int power_supply_register(struct device *parent, struct power_supply *psy)
{
struct device *dev;
int rc;
+#ifdef CONFIG_THERMAL
+ int i;
+#endif
dev = kzalloc(sizeof(*dev), GFP_KERNEL);
if (!dev)
@@ -196,7 +223,21 @@ int power_supply_register(struct device *parent, struct power_supply *psy)
rc = device_add(dev);
if (rc)
goto device_add_failed;
-
+#ifdef CONFIG_THERMAL
+ /* Register battery zone device psy reports temperature */
+ for (i = 0; i < psy->num_properties; i++) {
+ if (psy->properties[i] == POWER_SUPPLY_PROP_TEMP) {
+ psy->tzd = thermal_zone_device_register(
+ (char *)psy->name, 0, psy,
+ &psy_tzd_ops, 0, 0, 0, 0);
+ if (IS_ERR(psy->tzd)) {
+ rc = PTR_ERR(psy->tzd);
+ goto therm_zone_reg_failed;
+ }
+ break;
+ }
+ }
+#endif
rc = power_supply_create_triggers(psy);
if (rc)
goto create_triggers_failed;
@@ -206,6 +247,11 @@ int power_supply_register(struct device *parent, struct power_supply *psy)
goto success;
create_triggers_failed:
+#ifdef CONFIG_THERMAL
+therm_zone_reg_failed:
+ if (!IS_ERR(psy->tzd))
+ thermal_zone_device_unregister(psy->tzd);
+#endif
device_del(dev);
kobject_set_name_failed:
device_add_failed:
@@ -220,6 +266,10 @@ void power_supply_unregister(struct power_supply *psy)
cancel_work_sync(&psy->changed_work);
sysfs_remove_link(&psy->dev->kobj, "powers");
power_supply_remove_triggers(psy);
+#ifdef CONFIG_THERMAL
+ if (psy->tzd)
+ thermal_zone_device_unregister(psy->tzd);
+#endif
device_unregister(psy->dev);
}
EXPORT_SYMBOL_GPL(power_supply_unregister);
diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
index c38c13d..1f58435 100644
--- a/include/linux/power_supply.h
+++ b/include/linux/power_supply.h
@@ -172,6 +172,9 @@ struct power_supply {
/* private */
struct device *dev;
struct work_struct changed_work;
+#ifdef CONFIG_THERMAL
+ struct thermal_zone_device *tzd;
+#endif
#ifdef CONFIG_LEDS_TRIGGERS
struct led_trigger *charging_full_trig;
--
1.7.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
end of thread, other threads:[~2012-05-09 9:34 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-05-09 11:18 [PATCH] power_supply: Register battery as a thermal zone Jenny TC
2012-05-09 6:16 ` Ryan Mallon
2012-05-09 9:33 ` Tc, Jenny
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox