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] [thread-next>] [day] [month] [year] [list]

Message-ID: <4FAA0BA1.5080004@gmail.com>
Date:	Wed, 09 May 2012 16:16:01 +1000
From:	Ryan Mallon <rmallon@...il.com>
To:	Jenny TC <jenny.tc@...el.com>
CC:	cbouatmailru@...il.com, anton.vorontsov@...aro.org,
	linux-kernel@...r.kernel.org, durgadoss.r@...el.com
Subject: Re: [PATCH] power_supply: Register battery as a thermal zone

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

> +	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
--
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/