[PATCH v2] power: max17042_battery: add HEALTH and TEMP_* properties support

[PATCH v2] power: max17042_battery: add HEALTH and TEMP_* properties support

[Ramakrishna Pallala]

This patch adds the support for following battery properties
to max17042 fuel gauge driver.

POWER_SUPPLY_PROP_TEMP_ALERT_MIN
POWER_SUPPLY_PROP_TEMP_ALERT_MAX
POWER_SUPPLY_PROP_TEMP_MIN
POWER_SUPPLY_PROP_TEMP_MAX
POWER_SUPPLY_PROP_HEALTH

Signed-off-by: Ramakrishna Pallala <ramakrishna.pallala@intel.com>
Reviewed-by: Krzysztof Kozlowski <k.kozlowski.k@gmail.com>
---
 drivers/power/max17042_battery.c       |  190 ++++++++++++++++++++++++++++++--
 include/linux/power/max17042_battery.h |    4 +
 2 files changed, 183 insertions(+), 11 deletions(-)

diff --git a/drivers/power/max17042_battery.c b/drivers/power/max17042_battery.c
index 6cc5e87..748d976 100644
--- a/drivers/power/max17042_battery.c
+++ b/drivers/power/max17042_battery.c
@@ -63,6 +63,8 @@
 #define dP_ACC_100	0x1900
 #define dP_ACC_200	0x3200
 
+#define MAX17042_VMAX_TOLERANCE		50 /* 50 mV */
+
 struct max17042_chip {
 	struct i2c_client *client;
 	struct regmap *regmap;
@@ -85,10 +87,94 @@ static enum power_supply_property max17042_battery_props[] = {
 	POWER_SUPPLY_PROP_CHARGE_FULL,
 	POWER_SUPPLY_PROP_CHARGE_COUNTER,
 	POWER_SUPPLY_PROP_TEMP,
+	POWER_SUPPLY_PROP_TEMP_ALERT_MIN,
+	POWER_SUPPLY_PROP_TEMP_ALERT_MAX,
+	POWER_SUPPLY_PROP_TEMP_MIN,
+	POWER_SUPPLY_PROP_TEMP_MAX,
+	POWER_SUPPLY_PROP_HEALTH,
 	POWER_SUPPLY_PROP_CURRENT_NOW,
 	POWER_SUPPLY_PROP_CURRENT_AVG,
 };
 
+static int max17042_get_temperature(struct max17042_chip *chip, int *temp)
+{
+	int ret;
+	u32 data;
+	struct regmap *map = chip->regmap;
+
+	ret = regmap_read(map, MAX17042_TEMP, &data);
+	if (ret < 0)
+		return ret;
+
+	*temp = data;
+	/* The value is signed. */
+	if (*temp & 0x8000) {
+		*temp = (0x7fff & ~*temp) + 1;
+		*temp *= -1;
+	}
+
+	/* The value is converted into deci-centigrade scale */
+	/* Units of LSB = 1 / 256 degree Celsius */
+	*temp = *temp * 10 / 256;
+	return 0;
+}
+
+static int max17042_get_battery_health(struct max17042_chip *chip, int *health)
+{
+	int temp, vavg, vbatt, ret;
+	u32 val;
+
+	ret = regmap_read(chip->regmap, MAX17042_AvgVCELL, &val);
+	if (ret < 0)
+		goto health_error;
+
+	/* bits [0-3] unused */
+	vavg = val * 625 / 8;
+	/* Convert to millivolts */
+	vavg /= 1000;
+
+	ret = regmap_read(chip->regmap, MAX17042_VCELL, &val);
+	if (ret < 0)
+		goto health_error;
+
+	/* bits [0-3] unused */
+	vbatt = val * 625 / 8;
+	/* Convert to millivolts */
+	vbatt /= 1000;
+
+	if (vavg < chip->pdata->vmin) {
+		*health = POWER_SUPPLY_HEALTH_DEAD;
+		goto out;
+	}
+
+	if (vbatt > chip->pdata->vmax + MAX17042_VMAX_TOLERANCE) {
+		*health = POWER_SUPPLY_HEALTH_OVERVOLTAGE;
+		goto out;
+	}
+
+	ret = max17042_get_temperature(chip, &temp);
+	if (ret < 0)
+		goto health_error;
+
+	if (temp <= chip->pdata->temp_min) {
+		*health = POWER_SUPPLY_HEALTH_COLD;
+		goto out;
+	}
+
+	if (temp >= chip->pdata->temp_max) {
+		*health = POWER_SUPPLY_HEALTH_OVERHEAT;
+		goto out;
+	}
+
+	*health = POWER_SUPPLY_HEALTH_GOOD;
+
+out:
+	return 0;
+
+health_error:
+	return ret;
+}
+
 static int max17042_get_property(struct power_supply *psy,
 			    enum power_supply_property psp,
 			    union power_supply_propval *val)
@@ -181,19 +267,34 @@ static int max17042_get_property(struct power_supply *psy,
 		val->intval = data * 1000 / 2;
 		break;
 	case POWER_SUPPLY_PROP_TEMP:
-		ret = regmap_read(map, MAX17042_TEMP, &data);
+		ret = max17042_get_temperature(chip, &val->intval);
+		if (ret < 0)
+			return ret;
+		break;
+	case POWER_SUPPLY_PROP_TEMP_ALERT_MIN:
+		ret = regmap_read(map, MAX17042_TALRT_Th, &data);
+		if (ret < 0)
+			return ret;
+		/* LSB is Alert Minimum. In deci-centigrade */
+		val->intval = (data & 0xff) * 10;
+		break;
+	case POWER_SUPPLY_PROP_TEMP_ALERT_MAX:
+		ret = regmap_read(map, MAX17042_TALRT_Th, &data);
+		if (ret < 0)
+			return ret;
+		/* MSB is Alert Maximum. In deci-centigrade */
+		val->intval = (data >> 8) * 10;
+		break;
+	case POWER_SUPPLY_PROP_TEMP_MIN:
+		val->intval = chip->pdata->temp_min;
+		break;
+	case POWER_SUPPLY_PROP_TEMP_MAX:
+		val->intval = chip->pdata->temp_max;
+		break;
+	case POWER_SUPPLY_PROP_HEALTH:
+		ret = max17042_get_battery_health(chip, &val->intval);
 		if (ret < 0)
 			return ret;
-
-		val->intval = data;
-		/* The value is signed. */
-		if (val->intval & 0x8000) {
-			val->intval = (0x7fff & ~val->intval) + 1;
-			val->intval *= -1;
-		}
-		/* The value is converted into deci-centigrade scale */
-		/* Units of LSB = 1 / 256 degree Celsius */
-		val->intval = val->intval * 10 / 256;
 		break;
 	case POWER_SUPPLY_PROP_CURRENT_NOW:
 		if (chip->pdata->enable_current_sense) {
@@ -237,6 +338,69 @@ static int max17042_get_property(struct power_supply *psy,
 	return 0;
 }
 
+static int max17042_set_property(struct power_supply *psy,
+			    enum power_supply_property psp,
+			    const union power_supply_propval *val)
+{
+	struct max17042_chip *chip = power_supply_get_drvdata(psy);
+	struct regmap *map = chip->regmap;
+	int ret = 0;
+	u32 data;
+	int8_t temp;
+
+	switch (psp) {
+	case POWER_SUPPLY_PROP_TEMP_ALERT_MIN:
+		ret = regmap_read(map, MAX17042_TALRT_Th, &data);
+		if (ret < 0)
+			return ret;
+
+		/* Input in deci-centigrade, convert to centigrade */
+		temp = val->intval / 10;
+		/* force min < max */
+		if (temp >= (int8_t)(data >> 8))
+			temp = (int8_t)(data >> 8) - 1;
+		/* Write both MAX and MIN ALERT */
+		data = (data & 0xff00) + temp;
+		ret = regmap_write(map, MAX17042_TALRT_Th, data);
+		break;
+	case POWER_SUPPLY_PROP_TEMP_ALERT_MAX:
+		ret = regmap_read(map, MAX17042_TALRT_Th, &data);
+		if (ret < 0)
+			return ret;
+
+		/* Input in Deci-Centigrade, convert to centigrade */
+		temp = val->intval / 10;
+		/* force max > min */
+		if (temp <= (int8_t)(data & 0xff))
+			temp = (int8_t)(data & 0xff) + 1;
+		/* Write both MAX and MIN ALERT */
+		data = (data & 0xff) + (temp << 8);
+		ret = regmap_write(map, MAX17042_TALRT_Th, data);
+		break;
+	default:
+		ret = -EINVAL;
+	}
+
+	return ret;
+}
+
+static int max17042_property_is_writeable(struct power_supply *psy,
+		enum power_supply_property psp)
+{
+	int ret;
+
+	switch (psp) {
+	case POWER_SUPPLY_PROP_TEMP_ALERT_MIN:
+	case POWER_SUPPLY_PROP_TEMP_ALERT_MAX:
+		ret = 1;
+		break;
+	default:
+		ret = 0;
+	}
+
+	return ret;
+}
+
 static int max17042_write_verify_reg(struct regmap *map, u8 reg, u32 value)
 {
 	int retries = 8;
@@ -665,6 +829,8 @@ static const struct power_supply_desc max17042_psy_desc = {
 	.name		= "max170xx_battery",
 	.type		= POWER_SUPPLY_TYPE_BATTERY,
 	.get_property	= max17042_get_property,
+	.set_property	= max17042_set_property,
+	.property_is_writeable	= max17042_property_is_writeable,
 	.properties	= max17042_battery_props,
 	.num_properties	= ARRAY_SIZE(max17042_battery_props),
 };
@@ -673,6 +839,8 @@ static const struct power_supply_desc max17042_no_current_sense_psy_desc = {
 	.name		= "max170xx_battery",
 	.type		= POWER_SUPPLY_TYPE_BATTERY,
 	.get_property	= max17042_get_property,
+	.set_property	= max17042_set_property,
+	.property_is_writeable	= max17042_property_is_writeable,
 	.properties	= max17042_battery_props,
 	.num_properties	= ARRAY_SIZE(max17042_battery_props) - 2,
 };
diff --git a/include/linux/power/max17042_battery.h b/include/linux/power/max17042_battery.h
index cf112b4..522757a 100644
--- a/include/linux/power/max17042_battery.h
+++ b/include/linux/power/max17042_battery.h
@@ -215,6 +215,10 @@ struct max17042_platform_data {
 	 * the datasheet although it can be changed by board designers.
 	 */
 	unsigned int r_sns;
+	int         vmin;	/* in millivolts */
+	int         vmax;	/* in millivolts */
+	int         temp_min;	/* in tenths of degree Celsius */
+	int         temp_max;	/* in tenths of degree Celsius */
 };
 
 #endif /* __MAX17042_BATTERY_H_ */
-- 
1.7.9.5

Re: [PATCH v2] power: max17042_battery: add HEALTH and TEMP_* properties support

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 374 bytes --]

Hi,

On Sun, May 24, 2015 at 09:11:58AM +0530, Ramakrishna Pallala wrote:
> This patch adds the support for following battery properties
> to max17042 fuel gauge driver.
> 
> POWER_SUPPLY_PROP_TEMP_ALERT_MIN
> POWER_SUPPLY_PROP_TEMP_ALERT_MAX
> POWER_SUPPLY_PROP_TEMP_MIN
> POWER_SUPPLY_PROP_TEMP_MAX
> POWER_SUPPLY_PROP_HEALTH

Thanks, queued.

-- Sebastian

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v2] power: max17042_battery: add HEALTH and TEMP_* properties support

[Krzysztof Kozlowski]

2015-05-24 12:41 GMT+09:00 Ramakrishna Pallala <ramakrishna.pallala@intel.com>:
> This patch adds the support for following battery properties
> to max17042 fuel gauge driver.
>
> POWER_SUPPLY_PROP_TEMP_ALERT_MIN
> POWER_SUPPLY_PROP_TEMP_ALERT_MAX
> POWER_SUPPLY_PROP_TEMP_MIN
> POWER_SUPPLY_PROP_TEMP_MAX
> POWER_SUPPLY_PROP_HEALTH

I wonder, have you tested the patch? After booting on Trats2 device
(max77693 which identifies itself as 17047-like) the values are:
POWER_SUPPLY_TEMP_ALERT_MIN=1280
POWER_SUPPLY_TEMP_ALERT_MAX=1270
POWER_SUPPLY_TEMP=257
This is okay, datasheet says that register after booting will have
value of 0x7f80.

However setting them to some value which should trigger interrupts
(like 300 for MIN or 200 for MAX) does not trigger the interrupt. I
added a printk in max17042_thread_handler().

Is the temperature alert feature working?

Best regards,
Krzysztof

Re: [PATCH v2] power: max17042_battery: add HEALTH and TEMP_* properties support

[Krzysztof Kozlowski]

2015-06-08 10:22 GMT+09:00 Krzysztof Kozlowski <k.kozlowski@samsung.com>:
> 2015-05-24 12:41 GMT+09:00 Ramakrishna Pallala <ramakrishna.pallala@intel.com>:
>> This patch adds the support for following battery properties
>> to max17042 fuel gauge driver.
>>
>> POWER_SUPPLY_PROP_TEMP_ALERT_MIN
>> POWER_SUPPLY_PROP_TEMP_ALERT_MAX
>> POWER_SUPPLY_PROP_TEMP_MIN
>> POWER_SUPPLY_PROP_TEMP_MAX
>> POWER_SUPPLY_PROP_HEALTH
>
> I wonder, have you tested the patch? After booting on Trats2 device
> (max77693 which identifies itself as 17047-like) the values are:
> POWER_SUPPLY_TEMP_ALERT_MIN=1280
> POWER_SUPPLY_TEMP_ALERT_MAX=1270
> POWER_SUPPLY_TEMP=257
> This is okay, datasheet says that register after booting will have
> value of 0x7f80.
>
> However setting them to some value which should trigger interrupts
> (like 300 for MIN or 200 for MAX) does not trigger the interrupt. I
> added a printk in max17042_thread_handler().
>
> Is the temperature alert feature working?

Dear Ramakrishna Pallala,

Can you reply to my question above?
If this feature is not working, then it should be removed.

Best regards,
Krzysztof
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in

Re: [PATCH v2] power: max17042_battery: add HEALTH and TEMP_* properties support

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 1393 bytes --]

Hi,

On Tue, Jun 23, 2015 at 09:58:41AM +0900, Krzysztof Kozlowski wrote:
> 2015-06-08 10:22 GMT+09:00 Krzysztof Kozlowski <k.kozlowski@samsung.com>:
> > 2015-05-24 12:41 GMT+09:00 Ramakrishna Pallala <ramakrishna.pallala@intel.com>:
> >> This patch adds the support for following battery properties
> >> to max17042 fuel gauge driver.
> >>
> >> POWER_SUPPLY_PROP_TEMP_ALERT_MIN
> >> POWER_SUPPLY_PROP_TEMP_ALERT_MAX
> >> POWER_SUPPLY_PROP_TEMP_MIN
> >> POWER_SUPPLY_PROP_TEMP_MAX
> >> POWER_SUPPLY_PROP_HEALTH
> >
> > I wonder, have you tested the patch? After booting on Trats2 device
> > (max77693 which identifies itself as 17047-like) the values are:
> > POWER_SUPPLY_TEMP_ALERT_MIN=1280
> > POWER_SUPPLY_TEMP_ALERT_MAX=1270
> > POWER_SUPPLY_TEMP=257
> > This is okay, datasheet says that register after booting will have
> > value of 0x7f80.
> >
> > However setting them to some value which should trigger interrupts
> > (like 300 for MIN or 200 for MAX) does not trigger the interrupt. I
> > added a printk in max17042_thread_handler().
> >
> > Is the temperature alert feature working?
> 
> Can you reply to my question above?
> If this feature is not working, then it should be removed.

What's the status of this? I cannot test the feature, since
I don't have the hardware. I agree, that this should be
removed, if it's not working.

-- Sebastian

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

RE: [PATCH v2] power: max17042_battery: add HEALTH and TEMP_* properties support

[Pallala, Ramakrishna]

Hi Krzysztof Kozlowski,

>On Tue, Jun 23, 2015 at 09:58:41AM +0900, Krzysztof Kozlowski wrote:
>> 2015-06-08 10:22 GMT+09:00 Krzysztof Kozlowski <k.kozlowski@samsung.com>:
>> > 2015-05-24 12:41 GMT+09:00 Ramakrishna Pallala <ramakrishna.pallala@intel.com>:
>> >> This patch adds the support for following battery properties to 
> >> max17042 fuel gauge driver.
>> >>
>> >> POWER_SUPPLY_PROP_TEMP_ALERT_MIN
> >> POWER_SUPPLY_PROP_TEMP_ALERT_MAX
>> >> POWER_SUPPLY_PROP_TEMP_MIN
>> >> POWER_SUPPLY_PROP_TEMP_MAX
>> >> POWER_SUPPLY_PROP_HEALTH
>> >
>> > I wonder, have you tested the patch? After booting on Trats2 device
>> > (max77693 which identifies itself as 17047-like) the values are:
>> > POWER_SUPPLY_TEMP_ALERT_MIN=1280
>> > POWER_SUPPLY_TEMP_ALERT_MAX=1270
>> > POWER_SUPPLY_TEMP=257
>> > This is okay, datasheet says that register after booting will have 
>> > value of 0x7f80.
>> >
>> > However setting them to some value which should trigger interrupts 
>> > (like 300 for MIN or 200 for MAX) does not trigger the interrupt. I 
>> > added a printk in max17042_thread_handler().
>> >
>> > Is the temperature alert feature working?
>> 
>> Can you reply to my question above?
>> If this feature is not working, then it should be removed.

>What's the status of this? I cannot test the feature, since I don't have the hardware. I agree, that this should be removed, if it's not working.

I missed this email (may be overlooked it). To have the interrupts enabled we need the config registers(0x1Dh) bit's BIT(9), BIT(4) and BIT92) should be 1 and  BIT(8) should be 0.

Can you dump the status(00h), Talrt(02H) Temp(08h) and config(1Dh) registers values and share?

Thanks,
Ram

Re: [PATCH v2] power: max17042_battery: add HEALTH and TEMP_* properties support

[Krzysztof Kozlowski]

On 25.07.2015 22:23, Pallala, Ramakrishna wrote:
> Hi Krzysztof Kozlowski,
> 
>> On Tue, Jun 23, 2015 at 09:58:41AM +0900, Krzysztof Kozlowski wrote:
>>> 2015-06-08 10:22 GMT+09:00 Krzysztof Kozlowski <k.kozlowski@samsung.com>:
>>>> 2015-05-24 12:41 GMT+09:00 Ramakrishna Pallala <ramakrishna.pallala@intel.com>:
>>>>> This patch adds the support for following battery properties to 
>>>> max17042 fuel gauge driver.
>>>>>
>>>>> POWER_SUPPLY_PROP_TEMP_ALERT_MIN
>>>> POWER_SUPPLY_PROP_TEMP_ALERT_MAX
>>>>> POWER_SUPPLY_PROP_TEMP_MIN
>>>>> POWER_SUPPLY_PROP_TEMP_MAX
>>>>> POWER_SUPPLY_PROP_HEALTH
>>>>
>>>> I wonder, have you tested the patch? After booting on Trats2 device
>>>> (max77693 which identifies itself as 17047-like) the values are:
>>>> POWER_SUPPLY_TEMP_ALERT_MIN=1280
>>>> POWER_SUPPLY_TEMP_ALERT_MAX=1270
>>>> POWER_SUPPLY_TEMP=257
>>>> This is okay, datasheet says that register after booting will have 
>>>> value of 0x7f80.
>>>>
>>>> However setting them to some value which should trigger interrupts 
>>>> (like 300 for MIN or 200 for MAX) does not trigger the interrupt. I 
>>>> added a printk in max17042_thread_handler().
>>>>
>>>> Is the temperature alert feature working?
>>>
>>> Can you reply to my question above?
>>> If this feature is not working, then it should be removed.
> 
>> What's the status of this? I cannot test the feature, since I don't have the hardware. I agree, that this should be removed, if it's not working.
> 
> I missed this email (may be overlooked it). To have the interrupts enabled we need the config registers(0x1Dh) bit's BIT(9), BIT(4) and BIT92) should be 1 and  BIT(8) should be 0.
> 
> Can you dump the status(00h), Talrt(02H) Temp(08h) and config(1Dh) registers values and share?

Thanks for responding. The issue was in BIT(8) which was set to default
value of 0x1. This would mean to use external sensor but the board does
not have it.

This is a DT platform and there is no initial config data so all
registers are set to default values.

Anyway everything seems to work as expected, thanks for explanation.

Best regards,
Krzysztof

Re: [PATCH v2] power: max17042_battery: add HEALTH and TEMP_* properties support

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 905 bytes --]

Hi,

On Mon, Jul 27, 2015 at 09:34:13AM +0900, Krzysztof Kozlowski wrote:
>>> [...]
>>
>> I missed this email (may be overlooked it). To have the
>> interrupts enabled we need the config registers(0x1Dh) bit's
>> BIT(9), BIT(4) and BIT92) should be 1 and  BIT(8) should be 0.
>> 
>> Can you dump the status(00h), Talrt(02H) Temp(08h) and
>> config(1Dh) registers values and share?
> 
> Thanks for responding. The issue was in BIT(8) which was set to default
> value of 0x1. This would mean to use external sensor but the board does
> not have it.
> 
> This is a DT platform and there is no initial config data so all
> registers are set to default values.
> 
> Anyway everything seems to work as expected, thanks for explanation.

So I guess the bit should be set to 0 during probe. Maybe with a
boolean DT property "maxim,has-external-sensor" for setting it
to 1.

-- Sebastian

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v2] power: max17042_battery: add HEALTH and TEMP_* properties support

[Krzysztof Kozlowski]

On 27.07.2015 10:22, Sebastian Reichel wrote:
> Hi,
> 
> On Mon, Jul 27, 2015 at 09:34:13AM +0900, Krzysztof Kozlowski wrote:
>>>> [...]
>>>
>>> I missed this email (may be overlooked it). To have the
>>> interrupts enabled we need the config registers(0x1Dh) bit's
>>> BIT(9), BIT(4) and BIT92) should be 1 and  BIT(8) should be 0.
>>>
>>> Can you dump the status(00h), Talrt(02H) Temp(08h) and
>>> config(1Dh) registers values and share?
>>
>> Thanks for responding. The issue was in BIT(8) which was set to default
>> value of 0x1. This would mean to use external sensor but the board does
>> not have it.
>>
>> This is a DT platform and there is no initial config data so all
>> registers are set to default values.
>>
>> Anyway everything seems to work as expected, thanks for explanation.
> 
> So I guess the bit should be set to 0 during probe. Maybe with a
> boolean DT property "maxim,has-external-sensor" for setting it
> to 1.

Yes, something like this but it may be not sufficient. Other
configuration register may also require tuning.

This tuning happens when platform data is provided. The driver overrides
sets whole configuration (the config register and even some more
settings). Since on ARM we moved to DT-only this functionality is
crippled for us but works for Intel.

I put it on my todo list (fixing the sensor or entire configuration)...
but if anyone would like to fix this, then go ahead. I can provide a
tested-by.

Best regards,
Krzysztof