lkml.org 
[lkml]   [2012]   [May]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRE: [PATCH] power_supply: Register battery as a thermal zone
    Date
    > 
    > > 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
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2012-05-09 12:02    [W:0.031 / U:0.496 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site