Re: [PATCH 2/4] charger-manager: Use IIO subsystem to read battery temperature instead of legacy method

[Lars-Peter Clausen <lars@metafoo.de>]

On 10/22/2013 02:51 PM, Chanwoo Choi wrote:
> This patch support charger-manager use IIO(Industrial I/O) subsystem to read
> current battery temperature instead of legacy methor about callback function.

How does this look in hardware? Do you have a general purpose ADC connected
to a temperature sensor that has a temperature dependent voltage output? And
at some point during the board design you measure the raw value of the ADC
both for the lower and the upper threshold temperatures?

> 
> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> Signed-off-by: Myungjoo Ham <myungjoo.ham@samsung.com>
> ---
>  drivers/power/Kconfig                 |  1 +
>  drivers/power/charger-manager.c       | 88 +++++++++++++++++++++++++++++++++--
>  include/linux/power/charger-manager.h | 13 ++++++
>  3 files changed, 97 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/power/Kconfig b/drivers/power/Kconfig
> index e6f92b4..6700191 100644
> --- a/drivers/power/Kconfig
> +++ b/drivers/power/Kconfig
> @@ -309,6 +309,7 @@ config CHARGER_MANAGER
>  	bool "Battery charger manager for multiple chargers"
>  	depends on REGULATOR && RTC_CLASS
>  	select EXTCON
> +	select IIO

Are you sure you want to force select the IIO framework? It looks like we do
not stub out iio_channel_get() and friends yet if CONFIG_IIO is not
selected. But I think that will the better solution here.

>  	help
>            Say Y to enable charger-manager support, which allows multiple
>            chargers attached to a battery and multiple batteries attached to a
> diff --git a/drivers/power/charger-manager.c b/drivers/power/charger-manager.c
> index cc720f9..02a395c 100644
> --- a/drivers/power/charger-manager.c
> +++ b/drivers/power/charger-manager.c
> @@ -26,6 +26,7 @@
>  #include <linux/regulator/consumer.h>
>  #include <linux/sysfs.h>
>  #include <linux/of.h>
> +#include <linux/iio/consumer.h>
>  
>  static const char * const default_event_names[] = {
>  	[CM_EVENT_UNKNOWN] = "Unknown",
> @@ -542,6 +543,50 @@ static int check_charging_duration(struct charger_manager *cm)
>  }
>  
>  /**
> + * read_battery_temperature - Read current battery temperature
> + * @cm: the Charger Manager representing the battery.
> + * @last_temp_mC: store current battery temperature
> + *
> + * Returns current state of temperature by using IIO or legacy method
> + * - CM_TEMP_NORMAL
> + * - CM_TEMP_OVERHEAT
> + * - CM_TEMP_COLD
> + */
> +static int read_battery_temperature(struct charger_manager *cm,
> +				    int *last_temp_mC)
> +{
> +	struct charger_desc *desc = cm->desc;
> +	int temp;
> +
> +	if (desc->channel) {
> +		temp = iio_read_channel_raw(desc->channel, last_temp_mC);
> +
> +		/*
> +		 * The charger-manager use IIO subsystem to read ADC raw data
> +		 * from IIO ADC device drvier. The each device driver has
> +		 * own non-standard ADC table. If user of charger-manager
> +		 * would like to get correct temperature value, have to convert
> +		 * 'last_temp_mC' variable according to proper calculation
> +		 * method and own ADC table.
> +		 */
> +
> +		if (*last_temp_mC >= desc->iio_adc_overheat)
> +			temp = CM_TEMP_NORMAL;	/* Overheat */

temp = CM_TEMP_OVERHEAT ?

> +		else if (*last_temp_mC <= desc->iio_adc_cold)
> +			temp = CM_TEMP_COLD;	/* Cold */
> +		else
> +			temp = CM_TEMP_NORMAL;	/* Normal */
> +
> +	} else if (desc->temperature_out_of_range) {
> +		temp = desc->temperature_out_of_range(last_temp_mC);
> +	} else {
> +		temp = INT_MAX;
> +	}
> +
> +	return temp;
> +}
> +
> +/**
>   * _cm_monitor - Monitor the temperature and return true for exceptions.
>   * @cm: the Charger Manager representing the battery.
>   *
> @@ -551,7 +596,7 @@ static int check_charging_duration(struct charger_manager *cm)
>  static bool _cm_monitor(struct charger_manager *cm)
>  {
>  	struct charger_desc *desc = cm->desc;
> -	int temp = desc->temperature_out_of_range(&cm->last_temp_mC);
> +	int temp = read_battery_temperature(cm, &cm->last_temp_mC);
>  
>  	dev_dbg(cm->dev, "monitoring (%2.2d.%3.3dC)\n",
>  		cm->last_temp_mC / 1000, cm->last_temp_mC % 1000);
> @@ -805,7 +850,7 @@ static int charger_get_property(struct power_supply *psy,
>  	case POWER_SUPPLY_PROP_TEMP:
>  		/* in thenth of centigrade */
>  		if (cm->last_temp_mC == INT_MIN)
> -			desc->temperature_out_of_range(&cm->last_temp_mC);
> +			read_battery_temperature(cm, &cm->last_temp_mC);

So this will now return the raw ADC value to userspace on a property that is
supposed to indicate milli-degree Celsius. That doesn't seem to be right.

>  		val->intval = cm->last_temp_mC / 100;
>  		if (!desc->meaure_battery_temp)
>  			ret = -ENODEV;
> @@ -813,7 +858,7 @@ static int charger_get_property(struct power_supply *psy,
>  	case POWER_SUPPLY_PROP_TEMP_AMBIENT:
>  		/* in thenth of centigrade */
>  		if (cm->last_temp_mC == INT_MIN)
> -			desc->temperature_out_of_range(&cm->last_temp_mC);
> +			read_battery_temperature(cm, &cm->last_temp_mC);
>  		val->intval = cm->last_temp_mC / 100;
>  		if (desc->measure_battery_temp)
>  			ret = -ENODEV;
> @@ -1586,6 +1631,32 @@ static int charger_manager_dt_parse_regulator(struct device *dev,
>  	return 0;
>  }
>  
> +static int charger_manager_dt_parse_iio(struct device *dev,
> +					      struct charger_desc *desc)
> +{
> +	struct device_node *np = dev->of_node;
> +
> +	if (of_property_read_u32(np, "iio-adc-overheat",
> +				&desc->iio_adc_overheat)) {
> +		dev_warn(dev, "cannot get standard value for hot temperature\n");
> +		return -EINVAL;
> +	}
> +
> +	if (of_property_read_u32(np, "iio-adc-cold",
> +				&desc->iio_adc_cold)) {
> +		dev_warn(dev, "cannot get standard value for cold temperature\n");
> +		return -EINVAL;
> +	}

iio-adc-overheat and iio-adc-cold are definitely not good names for
devicetree properties. The term IIO is really Linux specific and should not
appear in the devicetree. 'temperature-lower-threshold' and
'temperature-upper-threshold' might be better names.

> +
> +	desc->channel = iio_channel_get(dev, NULL);

You need to free the channel on the error paths in your probe function.

> +	if (IS_ERR(desc->channel)) {
> +		dev_err(dev, "cannot get iio channel\n");
> +		return PTR_ERR(desc->channel);
> +	}
> +
> +	return 0;
> +}