linux-kernel - RE: [PATCH] power_supply: Register battery as a thermal zone

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20ADAB092842284E95860F279283C56421D47A@BGSMSX101.gar.corp.intel.com>
Date:	Wed, 9 May 2012 09:33:57 +0000
From:	"Tc, Jenny" <jenny.tc@...el.com>
To:	Ryan Mallon <rmallon@...il.com>
CC:	"cbouatmailru@...il.com" <cbouatmailru@...il.com>,
	"anton.vorontsov@...aro.org" <anton.vorontsov@...aro.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"R, Durgadoss" <durgadoss.r@...el.com>
Subject: RE: [PATCH] power_supply: Register battery as a thermal zone

> 
> > 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@...el.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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/