| 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
| ||
|
Date: Sat, 26 Oct 2013 13:20:24 +0200
From: Lars-Peter Clausen <lars@...afoo.de>
To: Chanwoo Choi <cw00.choi@...sung.com>
CC: anton@...msg.org, linux-kernel@...r.kernel.org,
devicetree@...r.kernel.org, dwmw2@...radead.org,
grant.likely@...aro.org, rob.herring@...xeda.com,
myungjoo.ham@...sung.com, kyungmin.park@...sung.com,
"linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>
Subject: Re: [PATCH 2/4] charger-manager: Use IIO subsystem to read battery
temperature instead of legacy method
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@...sung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@...sung.com>
> Signed-off-by: Myungjoo Ham <myungjoo.ham@...sung.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;
> +}
--
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/
Powered by blists - more mailing lists