Re: [PATCH] power_supply: Register battery as a thermal zone

[Ryan Mallon <rmallon@gmail.com>]

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