[PATCH 01/13] power: Make power_supply_am_i_supplied return -ENODEV if there are no suppliers

[PATCH 01/13] power: Make power_supply_am_i_supplied return -ENODEV if there are no suppliers

[Hans de Goede]

Allow a caller of power_supply_am_i_supplied to differentiate between
there not being any suppliers, vs no suppliers being online by returning
-ENODEV if there are no suppliers matching supplied_to / supplied_from.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/power/supply/power_supply_core.c | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/drivers/power/supply/power_supply_core.c b/drivers/power/supply/power_supply_core.c
index 1e0960b..13a39da 100644
--- a/drivers/power/supply/power_supply_core.c
+++ b/drivers/power/supply/power_supply_core.c
@@ -280,13 +280,19 @@ static inline int power_supply_check_supplies(struct power_supply *psy)
 }
 #endif
 
-static int __power_supply_am_i_supplied(struct device *dev, void *data)
+struct am_i_supplied_data {
+	struct power_supply *psy;
+	unsigned int count;
+};
+
+static int __power_supply_am_i_supplied(struct device *dev, void *_data)
 {
 	union power_supply_propval ret = {0,};
-	struct power_supply *psy = data;
 	struct power_supply *epsy = dev_get_drvdata(dev);
+	struct am_i_supplied_data *data = _data;
 
-	if (__power_supply_is_supplied_by(epsy, psy))
+	data->count++;
+	if (__power_supply_is_supplied_by(epsy, data->psy))
 		if (!epsy->desc->get_property(epsy, POWER_SUPPLY_PROP_ONLINE,
 					&ret))
 			return ret.intval;
@@ -296,12 +302,16 @@ static int __power_supply_am_i_supplied(struct device *dev, void *data)
 
 int power_supply_am_i_supplied(struct power_supply *psy)
 {
+	struct am_i_supplied_data data = { psy, 0 };
 	int error;
 
-	error = class_for_each_device(power_supply_class, NULL, psy,
+	error = class_for_each_device(power_supply_class, NULL, &data,
 				      __power_supply_am_i_supplied);
 
-	dev_dbg(&psy->dev, "%s %d\n", __func__, error);
+	dev_dbg(&psy->dev, "%s count %u err %d\n", __func__, data.count, error);
+
+	if (data.count == 0)
+		return -ENODEV;
 
 	return error;
 }
-- 
2.9.3

[PATCH 02/13] power: max17042_battery: Use sign_extend32 instead of DIY code

[Hans de Goede]

Use sign_extend32 to sign-extend variables where necessary instead of
DIY code.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/power/supply/max17042_battery.c | 24 +++---------------------
 1 file changed, 3 insertions(+), 21 deletions(-)

diff --git a/drivers/power/supply/max17042_battery.c b/drivers/power/supply/max17042_battery.c
index da7a75f..790dfa9 100644
--- a/drivers/power/supply/max17042_battery.c
+++ b/drivers/power/supply/max17042_battery.c
@@ -106,13 +106,7 @@ static int max17042_get_temperature(struct max17042_chip *chip, int *temp)
 	if (ret < 0)
 		return ret;
 
-	*temp = data;
-	/* The value is signed. */
-	if (*temp & 0x8000) {
-		*temp = (0x7fff & ~*temp) + 1;
-		*temp *= -1;
-	}
-
+	*temp = sign_extend32(data, 15);
 	/* The value is converted into deci-centigrade scale */
 	/* Units of LSB = 1 / 256 degree Celsius */
 	*temp = *temp * 10 / 256;
@@ -302,13 +296,7 @@ static int max17042_get_property(struct power_supply *psy,
 			if (ret < 0)
 				return ret;
 
-			val->intval = data;
-			if (val->intval & 0x8000) {
-				/* Negative */
-				val->intval = ~val->intval & 0x7fff;
-				val->intval++;
-				val->intval *= -1;
-			}
+			val->intval = sign_extend32(data, 15);
 			val->intval *= 1562500 / chip->pdata->r_sns;
 		} else {
 			return -EINVAL;
@@ -320,13 +308,7 @@ static int max17042_get_property(struct power_supply *psy,
 			if (ret < 0)
 				return ret;
 
-			val->intval = data;
-			if (val->intval & 0x8000) {
-				/* Negative */
-				val->intval = ~val->intval & 0x7fff;
-				val->intval++;
-				val->intval *= -1;
-			}
+			val->intval = sign_extend32(data, 15);
 			val->intval *= 1562500 / chip->pdata->r_sns;
 		} else {
 			return -EINVAL;
-- 
2.9.3

[PATCH 03/13] power: max17047_battery: The temp alert values are 8-bit 2's complement

[Hans de Goede]

The temp alert values are 8-bit 2's complement, so sign-extend them
before reporting them back to the caller.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/power/supply/max17042_battery.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/power/supply/max17042_battery.c b/drivers/power/supply/max17042_battery.c
index 790dfa9..a51b296 100644
--- a/drivers/power/supply/max17042_battery.c
+++ b/drivers/power/supply/max17042_battery.c
@@ -270,14 +270,14 @@ static int max17042_get_property(struct power_supply *psy,
 		if (ret < 0)
 			return ret;
 		/* LSB is Alert Minimum. In deci-centigrade */
-		val->intval = (data & 0xff) * 10;
+		val->intval = sign_extend32(data & 0xff, 7) * 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;
+		val->intval = sign_extend32(data >> 8, 7) * 10;
 		break;
 	case POWER_SUPPLY_PROP_TEMP_MIN:
 		val->intval = chip->pdata->temp_min;
-- 
2.9.3

[PATCH 04/13] power: max17042_battery: Add default platform_data for x86 use

[Hans de Goede]

Some x86 machines use a max17047 fuel-gauge. X86 does not have board
files / dt to provide platform data, so add default platform_data
as fallback option so that the driver can work on x86.

Since not all boards have a thermistor hooked up, set temp_min to 0 and
change the health checks from temp <= temp_min to temp < temp_min to
not trigger on such boards (where temp reads 0).

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/power/supply/max17042_battery.c | 60 +++++++++++++++++++++++++++++----
 1 file changed, 53 insertions(+), 7 deletions(-)

diff --git a/drivers/power/supply/max17042_battery.c b/drivers/power/supply/max17042_battery.c
index a51b296..85a6bf3 100644
--- a/drivers/power/supply/max17042_battery.c
+++ b/drivers/power/supply/max17042_battery.c
@@ -150,12 +150,12 @@ static int max17042_get_battery_health(struct max17042_chip *chip, int *health)
 	if (ret < 0)
 		goto health_error;
 
-	if (temp <= chip->pdata->temp_min) {
+	if (temp < chip->pdata->temp_min) {
 		*health = POWER_SUPPLY_HEALTH_COLD;
 		goto out;
 	}
 
-	if (temp >= chip->pdata->temp_max) {
+	if (temp > chip->pdata->temp_max) {
 		*health = POWER_SUPPLY_HEALTH_OVERHEAT;
 		goto out;
 	}
@@ -772,8 +772,9 @@ static void max17042_init_worker(struct work_struct *work)
 
 #ifdef CONFIG_OF
 static struct max17042_platform_data *
-max17042_get_pdata(struct device *dev)
+max17042_get_pdata(struct max17042_chip *chip)
 {
+	struct device *dev = &chip->client->dev;
 	struct device_node *np = dev->of_node;
 	u32 prop;
 	struct max17042_platform_data *pdata;
@@ -806,10 +807,55 @@ max17042_get_pdata(struct device *dev)
 	return pdata;
 }
 #else
+static struct max17042_reg_data max17047_default_pdata_init_regs[] = {
+	/*
+	 * Some firmwares do not set FullSOCThr, Enable End-of-Charge Detection
+	 * when the voltage FG reports 95%, as recommend in the datasheet.
+	 */
+	{ MAX17047_FullSOCThr, 95 << 8 },
+};
+
 static struct max17042_platform_data *
-max17042_get_pdata(struct device *dev)
+max17042_get_pdata(struct max17042_chip *chip)
 {
-	return dev->platform_data;
+	struct device *dev = &chip->client->dev;
+	struct max17042_platform_data *pdata;
+	int ret, misc_cfg;
+
+	if (dev->platform_data)
+		return dev->platform_data;
+
+	/*
+	 * The MAX17047 gets used on x86 where we've no platform data, assume
+	 * the firmware will already have initialized the fuel-gauge and provide
+	 * default values for the non init bits to make things work.
+	 */
+	pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
+	if (!pdata)
+		return pdata;
+
+	if (chip->chip_type != MAXIM_DEVICE_TYPE_MAX17042) {
+		pdata->init_data = max17047_default_pdata_init_regs;
+		pdata->num_init_data =
+			ARRAY_SIZE(max17047_default_pdata_init_regs);
+	}
+
+	ret = regmap_read(chip->regmap, MAX17042_MiscCFG, &misc_cfg);
+	if (ret < 0)
+		return NULL;
+
+	/* If bits 0-1 are set to 3 then only Voltage readings are used */
+	if ((misc_cfg & 0x3) == 3)
+		pdata->enable_current_sense = false;
+	else
+		pdata->enable_current_sense = true;
+
+	pdata->vmin = 3000;
+	pdata->vmax = 4500;	/* Some devices use a LiHV cell */
+	pdata->temp_min = 0;	/* Some devices do not have a temp sensor */
+	pdata->temp_max = 700;	/* 70 degrees Celcius */
+
+	return pdata;
 }
 #endif
 
@@ -858,20 +904,20 @@ static int max17042_probe(struct i2c_client *client,
 		return -ENOMEM;
 
 	chip->client = client;
+	chip->chip_type = id->driver_data;
 	chip->regmap = devm_regmap_init_i2c(client, &max17042_regmap_config);
 	if (IS_ERR(chip->regmap)) {
 		dev_err(&client->dev, "Failed to initialize regmap\n");
 		return -EINVAL;
 	}
 
-	chip->pdata = max17042_get_pdata(&client->dev);
+	chip->pdata = max17042_get_pdata(chip);
 	if (!chip->pdata) {
 		dev_err(&client->dev, "no platform data provided\n");
 		return -EINVAL;
 	}
 
 	i2c_set_clientdata(client, chip);
-	chip->chip_type = id->driver_data;
 	psy_cfg.drv_data = chip;
 
 	/* When current is not measured,
-- 
2.9.3

[PATCH 05/13] power: max17042_battery: Change name in power_supply_desc to "main-battery"

[Hans de Goede]

The max17042 will typically be couple with some charger IC, almost all
charger drivers contain:

static char *xxx_charger_supplied_to[] = {
        "main-battery",
};

probe()
{
	struct power_supply_config cfg;

	cfg.supplied_to = xxx_charger_supplied_to;
	cfg.num_supplicants = ARRAY_SIZE(xxx_charger_supplied_to);
	xxx->charger = power_supply_register(dev, &x_charger_desc, &_cfg);
}

Change the name in max17042's power_supply_desc to "main-battery" to
match, so that power_supply_am_i_supplied() can be used to implement
POWER_SUPPLY_PROP_STATUS.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/power/supply/max17042_battery.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/power/supply/max17042_battery.c b/drivers/power/supply/max17042_battery.c
index 85a6bf3..f8a0384 100644
--- a/drivers/power/supply/max17042_battery.c
+++ b/drivers/power/supply/max17042_battery.c
@@ -866,7 +866,7 @@ static const struct regmap_config max17042_regmap_config = {
 };
 
 static const struct power_supply_desc max17042_psy_desc = {
-	.name		= "max170xx_battery",
+	.name		= "main-battery",
 	.type		= POWER_SUPPLY_TYPE_BATTERY,
 	.get_property	= max17042_get_property,
 	.set_property	= max17042_set_property,
@@ -876,7 +876,7 @@ static const struct power_supply_desc max17042_psy_desc = {
 };
 
 static const struct power_supply_desc max17042_no_current_sense_psy_desc = {
-	.name		= "max170xx_battery",
+	.name		= "main-battery",
 	.type		= POWER_SUPPLY_TYPE_BATTERY,
 	.get_property	= max17042_get_property,
 	.set_property	= max17042_set_property,
-- 
2.9.3

[PATCH 06/13] power: max17042_battery: Add support for the STATUS property

[Hans de Goede]

Userspace prefers the driver having a status property over having to guess
itself. Specifically this will properly make the GNOME3 UI (and likely
others) properly show discharging / charging / full status, instead
of always showing discharging as status.

Note that in the case there is no charger driver supplying the max17042,
then a status of unknown will get returned. At least upower treats
this the same as not having a status attribute, so in this case nothing
changes from a userspace pov.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/power/supply/max17042_battery.c | 49 +++++++++++++++++++++++++++++++++
 1 file changed, 49 insertions(+)

diff --git a/drivers/power/supply/max17042_battery.c b/drivers/power/supply/max17042_battery.c
index f8a0384..1ea8368 100644
--- a/drivers/power/supply/max17042_battery.c
+++ b/drivers/power/supply/max17042_battery.c
@@ -65,6 +65,9 @@
 
 #define MAX17042_VMAX_TOLERANCE		50 /* 50 mV */
 
+/* Consider RepCap which is less then 10 units below FullCAP full */
+#define MAX17042_FULL_THRESHOLD		10
+
 struct max17042_chip {
 	struct i2c_client *client;
 	struct regmap *regmap;
@@ -76,6 +79,7 @@ struct max17042_chip {
 };
 
 static enum power_supply_property max17042_battery_props[] = {
+	POWER_SUPPLY_PROP_STATUS,
 	POWER_SUPPLY_PROP_PRESENT,
 	POWER_SUPPLY_PROP_CYCLE_COUNT,
 	POWER_SUPPLY_PROP_VOLTAGE_MAX,
@@ -113,6 +117,46 @@ static int max17042_get_temperature(struct max17042_chip *chip, int *temp)
 	return 0;
 }
 
+static int max17042_get_status(struct max17042_chip *chip, int *status)
+{
+	int ret, charge_full, charge_now;
+
+	ret = power_supply_am_i_supplied(chip->battery);
+	if (ret < 0) {
+		*status = POWER_SUPPLY_STATUS_UNKNOWN;
+		return 0;
+	}
+	if (ret == 0) {
+		*status = POWER_SUPPLY_STATUS_DISCHARGING;
+		return 0;
+	}
+
+	/*
+	 * The MAX170xx has builtin end-of-charge detection and will update
+	 * FullCAP to match RepCap when it detects end of charging.
+	 *
+	 * When this cycle the battery gets charged to a higher (calculated)
+	 * capacity then the previous cycle then FullCAP will get updated
+	 * contineously once end-of-charge detection kicks in, so allow the
+	 * 2 to differ a bit.
+	 */
+
+	ret = regmap_read(chip->regmap, MAX17042_FullCAP, &charge_full);
+	if (ret < 0)
+		return ret;
+
+	ret = regmap_read(chip->regmap, MAX17042_RepCap, &charge_now);
+	if (ret < 0)
+		return ret;
+
+	if ((charge_full - charge_now) <= MAX17042_FULL_THRESHOLD)
+		*status = POWER_SUPPLY_STATUS_FULL;
+	else
+		*status = POWER_SUPPLY_STATUS_CHARGING;
+
+	return 0;
+}
+
 static int max17042_get_battery_health(struct max17042_chip *chip, int *health)
 {
 	int temp, vavg, vbatt, ret;
@@ -182,6 +226,11 @@ static int max17042_get_property(struct power_supply *psy,
 		return -EAGAIN;
 
 	switch (psp) {
+	case POWER_SUPPLY_PROP_STATUS:
+		ret = max17042_get_status(chip, &val->intval);
+		if (ret < 0)
+			return ret;
+		break;
 	case POWER_SUPPLY_PROP_PRESENT:
 		ret = regmap_read(map, MAX17042_STATUS, &data);
 		if (ret < 0)
-- 
2.9.3

[PATCH 07/13] power: max17042_battery: Add external_power_changed callback

[Hans de Goede]

If our supplier changes status, chances are we've changed status too,
let any listeners know about this.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/power/supply/max17042_battery.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/power/supply/max17042_battery.c b/drivers/power/supply/max17042_battery.c
index 1ea8368..e4288b5 100644
--- a/drivers/power/supply/max17042_battery.c
+++ b/drivers/power/supply/max17042_battery.c
@@ -432,6 +432,11 @@ static int max17042_property_is_writeable(struct power_supply *psy,
 	return ret;
 }
 
+static void max17042_external_power_changed(struct power_supply *psy)
+{
+	power_supply_changed(psy);
+}
+
 static int max17042_write_verify_reg(struct regmap *map, u8 reg, u32 value)
 {
 	int retries = 8;
@@ -920,6 +925,7 @@ static const struct power_supply_desc max17042_psy_desc = {
 	.get_property	= max17042_get_property,
 	.set_property	= max17042_set_property,
 	.property_is_writeable	= max17042_property_is_writeable,
+	.external_power_changed	= max17042_external_power_changed,
 	.properties	= max17042_battery_props,
 	.num_properties	= ARRAY_SIZE(max17042_battery_props),
 };
-- 
2.9.3

[PATCH 08/13] power: max17042_battery: Add support for the TECHNOLOGY attribute

[Hans de Goede]

The max17042 is intended for LiOn or LiPo batteries, add a TECHNOLOGY
attribute to reflect this. Note this is hardcoded to LiOn as there is
no way to tell the difference, this is done by many drivers and is
better then not providing any technology info at all.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/power/supply/max17042_battery.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/power/supply/max17042_battery.c b/drivers/power/supply/max17042_battery.c
index e4288b5..1b2ff16 100644
--- a/drivers/power/supply/max17042_battery.c
+++ b/drivers/power/supply/max17042_battery.c
@@ -81,6 +81,7 @@ struct max17042_chip {
 static enum power_supply_property max17042_battery_props[] = {
 	POWER_SUPPLY_PROP_STATUS,
 	POWER_SUPPLY_PROP_PRESENT,
+	POWER_SUPPLY_PROP_TECHNOLOGY,
 	POWER_SUPPLY_PROP_CYCLE_COUNT,
 	POWER_SUPPLY_PROP_VOLTAGE_MAX,
 	POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN,
@@ -241,6 +242,9 @@ static int max17042_get_property(struct power_supply *psy,
 		else
 			val->intval = 1;
 		break;
+	case POWER_SUPPLY_PROP_TECHNOLOGY:
+		val->intval = POWER_SUPPLY_TECHNOLOGY_LION;
+		break;
 	case POWER_SUPPLY_PROP_CYCLE_COUNT:
 		ret = regmap_read(map, MAX17042_Cycles, &data);
 		if (ret < 0)
-- 
2.9.3

[PATCH 09/13] power: max17042_battery: Add support for the VOLT_MIN property

[Hans de Goede]

The info is there, so lets export it, like we already do for VOLT_MAX.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/power/supply/max17042_battery.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/power/supply/max17042_battery.c b/drivers/power/supply/max17042_battery.c
index 1b2ff16..770e188 100644
--- a/drivers/power/supply/max17042_battery.c
+++ b/drivers/power/supply/max17042_battery.c
@@ -84,6 +84,7 @@ static enum power_supply_property max17042_battery_props[] = {
 	POWER_SUPPLY_PROP_TECHNOLOGY,
 	POWER_SUPPLY_PROP_CYCLE_COUNT,
 	POWER_SUPPLY_PROP_VOLTAGE_MAX,
+	POWER_SUPPLY_PROP_VOLTAGE_MIN,
 	POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN,
 	POWER_SUPPLY_PROP_VOLTAGE_NOW,
 	POWER_SUPPLY_PROP_VOLTAGE_AVG,
@@ -260,6 +261,13 @@ static int max17042_get_property(struct power_supply *psy,
 		val->intval = data >> 8;
 		val->intval *= 20000; /* Units of LSB = 20mV */
 		break;
+	case POWER_SUPPLY_PROP_VOLTAGE_MIN:
+		ret = regmap_read(map, MAX17042_MinMaxVolt, &data);
+		if (ret < 0)
+			return ret;
+
+		val->intval = (data & 0xff) * 20000; /* Units of 20mV */
+		break;
 	case POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN:
 		if (chip->chip_type == MAXIM_DEVICE_TYPE_MAX17042)
 			ret = regmap_read(map, MAX17042_V_empty, &data);
-- 
2.9.3

[PATCH 10/13] power: max17042_battery: Add support for the CHARGE_FULL_DESIGN property

[Hans de Goede]

The info is there, lets export it as a property.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/power/supply/max17042_battery.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/power/supply/max17042_battery.c b/drivers/power/supply/max17042_battery.c
index 770e188..56cfff7 100644
--- a/drivers/power/supply/max17042_battery.c
+++ b/drivers/power/supply/max17042_battery.c
@@ -90,6 +90,7 @@ static enum power_supply_property max17042_battery_props[] = {
 	POWER_SUPPLY_PROP_VOLTAGE_AVG,
 	POWER_SUPPLY_PROP_VOLTAGE_OCV,
 	POWER_SUPPLY_PROP_CAPACITY,
+	POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN,
 	POWER_SUPPLY_PROP_CHARGE_FULL,
 	POWER_SUPPLY_PROP_CHARGE_COUNTER,
 	POWER_SUPPLY_PROP_TEMP,
@@ -307,6 +308,13 @@ static int max17042_get_property(struct power_supply *psy,
 
 		val->intval = data >> 8;
 		break;
+	case POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN:
+		ret = regmap_read(map, MAX17042_DesignCap, &data);
+		if (ret < 0)
+			return ret;
+
+		val->intval = data * 1000 / 2;
+		break;
 	case POWER_SUPPLY_PROP_CHARGE_FULL:
 		ret = regmap_read(map, MAX17042_FullCAP, &data);
 		if (ret < 0)
-- 
2.9.3

[PATCH 11/13] power: max17042_battery: Add support for CHARGE_NOW property

[Hans de Goede]

Atleast upower prefers the more precise charge_now sysfs value over
capacity and the max17042 has the info, so lets export it.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/power/supply/max17042_battery.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/power/supply/max17042_battery.c b/drivers/power/supply/max17042_battery.c
index 56cfff7..fab4b95 100644
--- a/drivers/power/supply/max17042_battery.c
+++ b/drivers/power/supply/max17042_battery.c
@@ -92,6 +92,7 @@ static enum power_supply_property max17042_battery_props[] = {
 	POWER_SUPPLY_PROP_CAPACITY,
 	POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN,
 	POWER_SUPPLY_PROP_CHARGE_FULL,
+	POWER_SUPPLY_PROP_CHARGE_NOW,
 	POWER_SUPPLY_PROP_CHARGE_COUNTER,
 	POWER_SUPPLY_PROP_TEMP,
 	POWER_SUPPLY_PROP_TEMP_ALERT_MIN,
@@ -322,6 +323,13 @@ static int max17042_get_property(struct power_supply *psy,
 
 		val->intval = data * 1000 / 2;
 		break;
+	case POWER_SUPPLY_PROP_CHARGE_NOW:
+		ret = regmap_read(map, MAX17042_RepCap, &data);
+		if (ret < 0)
+			return ret;
+
+		val->intval = data * 1000 / 2;
+		break;
 	case POWER_SUPPLY_PROP_CHARGE_COUNTER:
 		ret = regmap_read(map, MAX17042_QH, &data);
 		if (ret < 0)
-- 
2.9.3

[PATCH 12/13] power: max17042_battery: Add support for SCOPE property

[Hans de Goede]

Add support for the SCOPE property, always return SCOPE_SYSTEM,
as the max170xx is used for the main battery on all known systems
with a max170xx.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/power/supply/max17042_battery.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/power/supply/max17042_battery.c b/drivers/power/supply/max17042_battery.c
index fab4b95..ce6f38c 100644
--- a/drivers/power/supply/max17042_battery.c
+++ b/drivers/power/supply/max17042_battery.c
@@ -100,6 +100,7 @@ static enum power_supply_property max17042_battery_props[] = {
 	POWER_SUPPLY_PROP_TEMP_MIN,
 	POWER_SUPPLY_PROP_TEMP_MAX,
 	POWER_SUPPLY_PROP_HEALTH,
+	POWER_SUPPLY_PROP_SCOPE,
 	POWER_SUPPLY_PROP_CURRENT_NOW,
 	POWER_SUPPLY_PROP_CURRENT_AVG,
 };
@@ -367,6 +368,9 @@ static int max17042_get_property(struct power_supply *psy,
 		if (ret < 0)
 			return ret;
 		break;
+	case POWER_SUPPLY_PROP_SCOPE:
+		val->intval = POWER_SUPPLY_SCOPE_SYSTEM;
+		break;
 	case POWER_SUPPLY_PROP_CURRENT_NOW:
 		if (chip->pdata->enable_current_sense) {
 			ret = regmap_read(map, MAX17042_Current, &data);
-- 
2.9.3

[PATCH 13/13] power: max77693_charger: Add supplied_to info to power_supply_config

[Hans de Goede]

On the exynos4412-trats2 the max77693 is used together with a
max17047 fuel-gauge. Add supplied_to info containing "main-battery",
so that the get_status code in the max17042_battery driver can use
power_supply_am_i_supplied and can properly let userspace know
if the battery is being charged or not.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/power/supply/max77693_charger.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/power/supply/max77693_charger.c b/drivers/power/supply/max77693_charger.c
index 6c78884..1eeed26 100644
--- a/drivers/power/supply/max77693_charger.c
+++ b/drivers/power/supply/max77693_charger.c
@@ -254,6 +254,10 @@ static int max77693_charger_get_property(struct power_supply *psy,
 	return ret;
 }
 
+static char *max77693_charger_supplied_to[] = {
+	"main-battery",
+};
+
 static const struct power_supply_desc max77693_charger_desc = {
 	.name		= MAX77693_CHARGER_NAME,
 	.type		= POWER_SUPPLY_TYPE_BATTERY,
@@ -697,6 +701,8 @@ static int max77693_charger_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
+	psy_cfg.supplied_to = max77693_charger_supplied_to;
+	psy_cfg.num_supplicants = ARRAY_SIZE(max77693_charger_supplied_to),
 	psy_cfg.drv_data = chg;
 
 	ret = device_create_file(&pdev->dev, &dev_attr_fast_charge_timer);
-- 
2.9.3

Re: [PATCH 01/13] power: Make power_supply_am_i_supplied return -ENODEV if there are no suppliers

[Krzysztof Kozlowski]

On Fri, Apr 14, 2017 at 02:59:07PM +0200, Hans de Goede wrote:
> Allow a caller of power_supply_am_i_supplied to differentiate between
> there not being any suppliers, vs no suppliers being online by returning
> -ENODEV if there are no suppliers matching supplied_to / supplied_from.
> 

This is missing important piece of information - why you need to
differentiate that? What is the difference for you between no supplies
at all and not-being-supplied?


> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/power/supply/power_supply_core.c | 20 +++++++++++++++-----
>  1 file changed, 15 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/power/supply/power_supply_core.c b/drivers/power/supply/power_supply_core.c
> index 1e0960b..13a39da 100644
> --- a/drivers/power/supply/power_supply_core.c
> +++ b/drivers/power/supply/power_supply_core.c
> @@ -280,13 +280,19 @@ static inline int power_supply_check_supplies(struct power_supply *psy)
>  }
>  #endif
>  
> -static int __power_supply_am_i_supplied(struct device *dev, void *data)
> +struct am_i_supplied_data {

How about a prefix, e.g.: "psy_am_i_supplied_data"?

Best regards,
Krzysztof

Re: [PATCH 01/13] power: Make power_supply_am_i_supplied return -ENODEV if there are no suppliers

[Hans de Goede]

Hi,

On 14-04-17 15:56, Krzysztof Kozlowski wrote:
> On Fri, Apr 14, 2017 at 02:59:07PM +0200, Hans de Goede wrote:
>> Allow a caller of power_supply_am_i_supplied to differentiate between
>> there not being any suppliers, vs no suppliers being online by returning
>> -ENODEV if there are no suppliers matching supplied_to / supplied_from.
>>
>
> This is missing important piece of information - why you need to
> differentiate that? What is the difference for you between no supplies
> at all and not-being-supplied?

Thank you for the quick response.

I think it is sensible to assume that the hardware actually always has a
way of charging the battery so where you say "no supplies at all" in
reality what would be the case is : "no power_supply-drivers
registered / bound for any supplies at all". At which point we can not
determine in many fuel-gauge drivers (which are the only user of
power_supply_am_i_supplied) if we're charging or discharging.

Here is the code for the STATUS property I'm adding to the max17042
driver in a later commit in the set:

static int max17042_get_status(struct max17042_chip *chip, int *status)
{
         int ret, charge_full, charge_now;

         ret = power_supply_am_i_supplied(chip->battery);
         if (ret < 0) {
                 *status = POWER_SUPPLY_STATUS_UNKNOWN;
                 return 0;
         }
         if (ret == 0) {
                 *status = POWER_SUPPLY_STATUS_DISCHARGING;
                 return 0;
         }

	...
}

This is where the -ENODEV comes in to make it properly return
POWER_SUPPLY_STATUS_UNKNOWN instead of always returning
POWER_SUPPLY_STATUS_DISCHARGING even if that may be untrue.

Note that I've since found out that the only in tree user of the
max17042 driver is arch/arm/boot/dts/exynos4412-trats2.dts and
the last patch in my makes the max77693 charger driver properly
set supplied_to so that power_supply_am_i_supplied() will work
correctly for the max17047 / max77693 combo on that board, which
sort of renders this patch unnecessary. I still think this patch
is a good idea, but it can be dropped if other people disagree.

Regards,

Hans




>
>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>  drivers/power/supply/power_supply_core.c | 20 +++++++++++++++-----
>>  1 file changed, 15 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/power/supply/power_supply_core.c b/drivers/power/supply/power_supply_core.c
>> index 1e0960b..13a39da 100644
>> --- a/drivers/power/supply/power_supply_core.c
>> +++ b/drivers/power/supply/power_supply_core.c
>> @@ -280,13 +280,19 @@ static inline int power_supply_check_supplies(struct power_supply *psy)
>>  }
>>  #endif
>>
>> -static int __power_supply_am_i_supplied(struct device *dev, void *data)
>> +struct am_i_supplied_data {
>
> How about a prefix, e.g.: "psy_am_i_supplied_data"?
>
> Best regards,
> Krzysztof
>

Re: [PATCH 01/13] power: Make power_supply_am_i_supplied return -ENODEV if there are no suppliers

[Krzysztof Kozlowski]

On Fri, Apr 14, 2017 at 04:07:22PM +0200, Hans de Goede wrote:
> Hi,
> 
> On 14-04-17 15:56, Krzysztof Kozlowski wrote:
> > On Fri, Apr 14, 2017 at 02:59:07PM +0200, Hans de Goede wrote:
> > > Allow a caller of power_supply_am_i_supplied to differentiate between
> > > there not being any suppliers, vs no suppliers being online by returning
> > > -ENODEV if there are no suppliers matching supplied_to / supplied_from.
> > > 
> > 
> > This is missing important piece of information - why you need to
> > differentiate that? What is the difference for you between no supplies
> > at all and not-being-supplied?
> 
> Thank you for the quick response.
> 
> I think it is sensible to assume that the hardware actually always has a
> way of charging the battery so where you say "no supplies at all" in
> reality what would be the case is : "no power_supply-drivers
> registered / bound for any supplies at all". At which point we can not
> determine in many fuel-gauge drivers (which are the only user of
> power_supply_am_i_supplied) if we're charging or discharging.
> 
> Here is the code for the STATUS property I'm adding to the max17042
> driver in a later commit in the set:
> 
> static int max17042_get_status(struct max17042_chip *chip, int *status)
> {
>         int ret, charge_full, charge_now;
> 
>         ret = power_supply_am_i_supplied(chip->battery);
>         if (ret < 0) {
>                 *status = POWER_SUPPLY_STATUS_UNKNOWN;
>                 return 0;
>         }
>         if (ret == 0) {
>                 *status = POWER_SUPPLY_STATUS_DISCHARGING;
>                 return 0;
>         }
> 
> 	...
> }

The explanation makes sense. It should be put in the commit msg as
rationale for this patch.


Best regards,
Krzysztof

> 
> This is where the -ENODEV comes in to make it properly return
> POWER_SUPPLY_STATUS_UNKNOWN instead of always returning
> POWER_SUPPLY_STATUS_DISCHARGING even if that may be untrue.
> 
> Note that I've since found out that the only in tree user of the
> max17042 driver is arch/arm/boot/dts/exynos4412-trats2.dts and
> the last patch in my makes the max77693 charger driver properly
> set supplied_to so that power_supply_am_i_supplied() will work
> correctly for the max17047 / max77693 combo on that board, which
> sort of renders this patch unnecessary. I still think this patch
> is a good idea, but it can be dropped if other people disagree.
> 
> Regards,
> 
> Hans
> 

Re: [PATCH 03/13] power: max17047_battery: The temp alert values are 8-bit 2's complement

[Krzysztof Kozlowski]

On Fri, Apr 14, 2017 at 02:59:09PM +0200, Hans de Goede wrote:
> The temp alert values are 8-bit 2's complement, so sign-extend them
> before reporting them back to the caller.

Are you sure that these are reported with sign bit? I couldn't find
confirmation of this in datasheet.

Best regards,
Krzysztof

> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/power/supply/max17042_battery.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/power/supply/max17042_battery.c b/drivers/power/supply/max17042_battery.c
> index 790dfa9..a51b296 100644
> --- a/drivers/power/supply/max17042_battery.c
> +++ b/drivers/power/supply/max17042_battery.c
> @@ -270,14 +270,14 @@ static int max17042_get_property(struct power_supply *psy,
>  		if (ret < 0)
>  			return ret;
>  		/* LSB is Alert Minimum. In deci-centigrade */
> -		val->intval = (data & 0xff) * 10;
> +		val->intval = sign_extend32(data & 0xff, 7) * 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;
> +		val->intval = sign_extend32(data >> 8, 7) * 10;
>  		break;
>  	case POWER_SUPPLY_PROP_TEMP_MIN:
>  		val->intval = chip->pdata->temp_min;
> -- 
> 2.9.3
> 

Re: [PATCH 03/13] power: max17047_battery: The temp alert values are 8-bit 2's complement

[Hans de Goede]

Hi,

On 14-04-17 17:09, Krzysztof Kozlowski wrote:
> On Fri, Apr 14, 2017 at 02:59:09PM +0200, Hans de Goede wrote:
>> The temp alert values are 8-bit 2's complement, so sign-extend them
>> before reporting them back to the caller.
>
> Are you sure that these are reported with sign bit? I couldn't find
> confirmation of this in datasheet.

From: MAX17047-MAX17050.pdf

"T ALRT Threshold Register (02h)
The T ALRT Threshold register sets upper and lower limits
that generate an ALRT pin interrupt if exceeded by the
Temperature register value. The upper 8 bits set the maxi-
mum value and the lower 8 bits set the minimum value.
Interrupt threshold limits are stored in two’s-complement
format"

And the reset default of 7F80h also hints at this,
as it is +127 for max -128 for min (aka temp based
alerts disabled).

Regards,

Hans


>
> Best regards,
> Krzysztof
>
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>  drivers/power/supply/max17042_battery.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/power/supply/max17042_battery.c b/drivers/power/supply/max17042_battery.c
>> index 790dfa9..a51b296 100644
>> --- a/drivers/power/supply/max17042_battery.c
>> +++ b/drivers/power/supply/max17042_battery.c
>> @@ -270,14 +270,14 @@ static int max17042_get_property(struct power_supply *psy,
>>  		if (ret < 0)
>>  			return ret;
>>  		/* LSB is Alert Minimum. In deci-centigrade */
>> -		val->intval = (data & 0xff) * 10;
>> +		val->intval = sign_extend32(data & 0xff, 7) * 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;
>> +		val->intval = sign_extend32(data >> 8, 7) * 10;
>>  		break;
>>  	case POWER_SUPPLY_PROP_TEMP_MIN:
>>  		val->intval = chip->pdata->temp_min;
>> --
>> 2.9.3
>>

Re: [PATCH 01/13] power: Make power_supply_am_i_supplied return -ENODEV if there are no suppliers

[Hans de Goede]

Hi,

On 14-04-17 16:31, Krzysztof Kozlowski wrote:
> On Fri, Apr 14, 2017 at 04:07:22PM +0200, Hans de Goede wrote:
>> Hi,
>>
>> On 14-04-17 15:56, Krzysztof Kozlowski wrote:
>>> On Fri, Apr 14, 2017 at 02:59:07PM +0200, Hans de Goede wrote:
>>>> Allow a caller of power_supply_am_i_supplied to differentiate between
>>>> there not being any suppliers, vs no suppliers being online by returning
>>>> -ENODEV if there are no suppliers matching supplied_to / supplied_from.
>>>>
>>>
>>> This is missing important piece of information - why you need to
>>> differentiate that? What is the difference for you between no supplies
>>> at all and not-being-supplied?
>>
>> Thank you for the quick response.
>>
>> I think it is sensible to assume that the hardware actually always has a
>> way of charging the battery so where you say "no supplies at all" in
>> reality what would be the case is : "no power_supply-drivers
>> registered / bound for any supplies at all". At which point we can not
>> determine in many fuel-gauge drivers (which are the only user of
>> power_supply_am_i_supplied) if we're charging or discharging.
>>
>> Here is the code for the STATUS property I'm adding to the max17042
>> driver in a later commit in the set:
>>
>> static int max17042_get_status(struct max17042_chip *chip, int *status)
>> {
>>         int ret, charge_full, charge_now;
>>
>>         ret = power_supply_am_i_supplied(chip->battery);
>>         if (ret < 0) {
>>                 *status = POWER_SUPPLY_STATUS_UNKNOWN;
>>                 return 0;
>>         }
>>         if (ret == 0) {
>>                 *status = POWER_SUPPLY_STATUS_DISCHARGING;
>>                 return 0;
>>         }
>>
>> 	...
>> }
>
> The explanation makes sense. It should be put in the commit msg as
> rationale for this patch.

Ok, I've added a similar text to the commit msg for v2, I will wait
until you've had a chance to respond to the rest of the set before
sending out a v2.

Regards,

Hans

Re: [PATCH 03/13] power: max17047_battery: The temp alert values are 8-bit 2's complement

[Krzysztof Kozlowski]

On Fri, Apr 14, 2017 at 05:16:32PM +0200, Hans de Goede wrote:
> Hi,
> 
> On 14-04-17 17:09, Krzysztof Kozlowski wrote:
> > On Fri, Apr 14, 2017 at 02:59:09PM +0200, Hans de Goede wrote:
> > > The temp alert values are 8-bit 2's complement, so sign-extend them
> > > before reporting them back to the caller.
> > 
> > Are you sure that these are reported with sign bit? I couldn't find
> > confirmation of this in datasheet.
> 
> From: MAX17047-MAX17050.pdf
> 
> "T ALRT Threshold Register (02h)
> The T ALRT Threshold register sets upper and lower limits
> that generate an ALRT pin interrupt if exceeded by the
> Temperature register value. The upper 8 bits set the maxi-
> mum value and the lower 8 bits set the minimum value.
> Interrupt threshold limits are stored in two’s-complement
> format"

That does not say it is signed. It might be still from 0 to 255.

> And the reset default of 7F80h also hints at this,
> as it is +127 for max -128 for min (aka temp based
> alerts disabled).

In this case looks correct... but max77693 fuel gauge datasheet says
about TALRT_Th (0x02):
Reset value: 0x00FF
"...At power up, the thresholds default to their maximum settings- 7F80h
(disabled)."
which as you see contains to different reset values...

Ehhh... Max17047max17050 datasheet looks a little bit more
chaotic/organized so I guess it should be used as a source.


Best regards,
Krzysztof

Re: [PATCH 02/13] power: max17042_battery: Use sign_extend32 instead of DIY code

[Krzysztof Kozlowski]

On Fri, Apr 14, 2017 at 02:59:08PM +0200, Hans de Goede wrote:
> Use sign_extend32 to sign-extend variables where necessary instead of
> DIY code.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/power/supply/max17042_battery.c | 24 +++---------------------
>  1 file changed, 3 insertions(+), 21 deletions(-)
> 

Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>

Best regards,
Krzysztof

Re: [PATCH 04/13] power: max17042_battery: Add default platform_data for x86 use

[Krzysztof Kozlowski]

On Fri, Apr 14, 2017 at 02:59:10PM +0200, Hans de Goede wrote:
> Some x86 machines use a max17047 fuel-gauge. X86 does not have board
> files / dt to provide platform data, so add default platform_data
> as fallback option so that the driver can work on x86.

I am not convinced by this explanation. AFAIR, the x86 boards which use
max17042-family are providing the platform data. See commit edd4ab055931
("power: max17042_battery: add HEALTH and TEMP_* properties support").

I am not sure what are you trying to fix here. What is your
development/testing platform?

Best regards,
Krzysztof

> 
> Since not all boards have a thermistor hooked up, set temp_min to 0 and
> change the health checks from temp <= temp_min to temp < temp_min to
> not trigger on such boards (where temp reads 0).
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/power/supply/max17042_battery.c | 60 +++++++++++++++++++++++++++++----
>  1 file changed, 53 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/power/supply/max17042_battery.c b/drivers/power/supply/max17042_battery.c
> index a51b296..85a6bf3 100644
> --- a/drivers/power/supply/max17042_battery.c
> +++ b/drivers/power/supply/max17042_battery.c
> @@ -150,12 +150,12 @@ static int max17042_get_battery_health(struct max17042_chip *chip, int *health)
>  	if (ret < 0)
>  		goto health_error;
>  
> -	if (temp <= chip->pdata->temp_min) {
> +	if (temp < chip->pdata->temp_min) {
>  		*health = POWER_SUPPLY_HEALTH_COLD;
>  		goto out;
>  	}
>  
> -	if (temp >= chip->pdata->temp_max) {
> +	if (temp > chip->pdata->temp_max) {
>  		*health = POWER_SUPPLY_HEALTH_OVERHEAT;
>  		goto out;
>  	}
> @@ -772,8 +772,9 @@ static void max17042_init_worker(struct work_struct *work)
>  
>  #ifdef CONFIG_OF
>  static struct max17042_platform_data *
> -max17042_get_pdata(struct device *dev)
> +max17042_get_pdata(struct max17042_chip *chip)
>  {
> +	struct device *dev = &chip->client->dev;
>  	struct device_node *np = dev->of_node;
>  	u32 prop;
>  	struct max17042_platform_data *pdata;
> @@ -806,10 +807,55 @@ max17042_get_pdata(struct device *dev)
>  	return pdata;
>  }
>  #else
> +static struct max17042_reg_data max17047_default_pdata_init_regs[] = {
> +	/*
> +	 * Some firmwares do not set FullSOCThr, Enable End-of-Charge Detection
> +	 * when the voltage FG reports 95%, as recommend in the datasheet.
> +	 */
> +	{ MAX17047_FullSOCThr, 95 << 8 },
> +};
> +
>  static struct max17042_platform_data *
> -max17042_get_pdata(struct device *dev)
> +max17042_get_pdata(struct max17042_chip *chip)
>  {
> -	return dev->platform_data;
> +	struct device *dev = &chip->client->dev;
> +	struct max17042_platform_data *pdata;
> +	int ret, misc_cfg;
> +
> +	if (dev->platform_data)
> +		return dev->platform_data;
> +
> +	/*
> +	 * The MAX17047 gets used on x86 where we've no platform data, assume
> +	 * the firmware will already have initialized the fuel-gauge and provide
> +	 * default values for the non init bits to make things work.
> +	 */
> +	pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
> +	if (!pdata)
> +		return pdata;
> +
> +	if (chip->chip_type != MAXIM_DEVICE_TYPE_MAX17042) {
> +		pdata->init_data = max17047_default_pdata_init_regs;
> +		pdata->num_init_data =
> +			ARRAY_SIZE(max17047_default_pdata_init_regs);
> +	}
> +
> +	ret = regmap_read(chip->regmap, MAX17042_MiscCFG, &misc_cfg);
> +	if (ret < 0)
> +		return NULL;
> +
> +	/* If bits 0-1 are set to 3 then only Voltage readings are used */
> +	if ((misc_cfg & 0x3) == 3)
> +		pdata->enable_current_sense = false;
> +	else
> +		pdata->enable_current_sense = true;
> +
> +	pdata->vmin = 3000;
> +	pdata->vmax = 4500;	/* Some devices use a LiHV cell */
> +	pdata->temp_min = 0;	/* Some devices do not have a temp sensor */
> +	pdata->temp_max = 700;	/* 70 degrees Celcius */
> +
> +	return pdata;
>  }
>  #endif
>  
> @@ -858,20 +904,20 @@ static int max17042_probe(struct i2c_client *client,
>  		return -ENOMEM;
>  
>  	chip->client = client;
> +	chip->chip_type = id->driver_data;
>  	chip->regmap = devm_regmap_init_i2c(client, &max17042_regmap_config);
>  	if (IS_ERR(chip->regmap)) {
>  		dev_err(&client->dev, "Failed to initialize regmap\n");
>  		return -EINVAL;
>  	}
>  
> -	chip->pdata = max17042_get_pdata(&client->dev);
> +	chip->pdata = max17042_get_pdata(chip);
>  	if (!chip->pdata) {
>  		dev_err(&client->dev, "no platform data provided\n");
>  		return -EINVAL;
>  	}
>  
>  	i2c_set_clientdata(client, chip);
> -	chip->chip_type = id->driver_data;
>  	psy_cfg.drv_data = chip;
>  
>  	/* When current is not measured,
> -- 
> 2.9.3
> 

Re: [PATCH 03/13] power: max17047_battery: The temp alert values are 8-bit 2's complement

[Hans de Goede]

Hi,

On 14-04-17 17:26, Krzysztof Kozlowski wrote:
> On Fri, Apr 14, 2017 at 05:16:32PM +0200, Hans de Goede wrote:
>> Hi,
>>
>> On 14-04-17 17:09, Krzysztof Kozlowski wrote:
>>> On Fri, Apr 14, 2017 at 02:59:09PM +0200, Hans de Goede wrote:
>>>> The temp alert values are 8-bit 2's complement, so sign-extend them
>>>> before reporting them back to the caller.
>>>
>>> Are you sure that these are reported with sign bit? I couldn't find
>>> confirmation of this in datasheet.
>>
>> From: MAX17047-MAX17050.pdf
>>
>> "T ALRT Threshold Register (02h)
>> The T ALRT Threshold register sets upper and lower limits
>> that generate an ALRT pin interrupt if exceeded by the
>> Temperature register value. The upper 8 bits set the maxi-
>> mum value and the lower 8 bits set the minimum value.
>> Interrupt threshold limits are stored in two’s-complement
>> format"
>
> That does not say it is signed. It might be still from 0 to 255.

Two's complement format is always signed that is its whole
purpose, from: https://en.wikipedia.org/wiki/Two's_complement

"Two's complement is a ... binary signed number representation"

>> And the reset default of 7F80h also hints at this,
>> as it is +127 for max -128 for min (aka temp based
>> alerts disabled).
>
> In this case looks correct... but max77693 fuel gauge datasheet says
> about TALRT_Th (0x02):
> Reset value: 0x00FF
> "...At power up, the thresholds default to their maximum settings- 7F80h
> (disabled)."
> which as you see contains to different reset values...

Notice that that datasheet is contradicting itself, it says:
"Reset value: 0x00FF" but also "at power up, the thresholds default
to their maximum settings- 7F80h" I can confirm on my max17047 that
the latter is correct.

> Ehhh... Max17047max17050 datasheet looks a little bit more
> chaotic/organized so I guess it should be used as a source.

Ack.

Regards,

Hans

Re: [PATCH 05/13] power: max17042_battery: Change name in power_supply_desc to "main-battery"

[Krzysztof Kozlowski]

On Fri, Apr 14, 2017 at 02:59:11PM +0200, Hans de Goede wrote:
> The max17042 will typically be couple with some charger IC, almost all

s/couple/coupled/


Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>

Best regards,
Krzysztof

> charger drivers contain:
> 
> static char *xxx_charger_supplied_to[] = {
>         "main-battery",
> };
> 
> probe()
> {
> 	struct power_supply_config cfg;
> 
> 	cfg.supplied_to = xxx_charger_supplied_to;
> 	cfg.num_supplicants = ARRAY_SIZE(xxx_charger_supplied_to);
> 	xxx->charger = power_supply_register(dev, &x_charger_desc, &_cfg);
> }
> 
> Change the name in max17042's power_supply_desc to "main-battery" to
> match, so that power_supply_am_i_supplied() can be used to implement
> POWER_SUPPLY_PROP_STATUS.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/power/supply/max17042_battery.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 

Re: [PATCH 03/13] power: max17047_battery: The temp alert values are 8-bit 2's complement

[Krzysztof Kozlowski]

On Fri, Apr 14, 2017 at 05:36:57PM +0200, Hans de Goede wrote:
> Hi,
> 
> On 14-04-17 17:26, Krzysztof Kozlowski wrote:
> > On Fri, Apr 14, 2017 at 05:16:32PM +0200, Hans de Goede wrote:
> > > Hi,
> > > 
> > > On 14-04-17 17:09, Krzysztof Kozlowski wrote:
> > > > On Fri, Apr 14, 2017 at 02:59:09PM +0200, Hans de Goede wrote:
> > > > > The temp alert values are 8-bit 2's complement, so sign-extend them
> > > > > before reporting them back to the caller.
> > > > 
> > > > Are you sure that these are reported with sign bit? I couldn't find
> > > > confirmation of this in datasheet.
> > > 
> > > From: MAX17047-MAX17050.pdf
> > > 
> > > "T ALRT Threshold Register (02h)
> > > The T ALRT Threshold register sets upper and lower limits
> > > that generate an ALRT pin interrupt if exceeded by the
> > > Temperature register value. The upper 8 bits set the maxi-
> > > mum value and the lower 8 bits set the minimum value.
> > > Interrupt threshold limits are stored in two’s-complement
> > > format"
> > 
> > That does not say it is signed. It might be still from 0 to 255.
> 
> Two's complement format is always signed that is its whole
> purpose, from: https://en.wikipedia.org/wiki/Two's_complement
>

Ahh, yes.


Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>

Best regards,
Krzysztof

Re: [PATCH 04/13] power: max17042_battery: Add default platform_data for x86 use

[Krzysztof Kozlowski]

On Fri, Apr 14, 2017 at 02:59:10PM +0200, Hans de Goede wrote:
> Some x86 machines use a max17047 fuel-gauge. X86 does not have board
> files / dt to provide platform data, so add default platform_data
> as fallback option so that the driver can work on x86.
> 
> Since not all boards have a thermistor hooked up, set temp_min to 0 and
> change the health checks from temp <= temp_min to temp < temp_min to
> not trigger on such boards (where temp reads 0).
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/power/supply/max17042_battery.c | 60 +++++++++++++++++++++++++++++----
>  1 file changed, 53 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/power/supply/max17042_battery.c b/drivers/power/supply/max17042_battery.c
> index a51b296..85a6bf3 100644
> --- a/drivers/power/supply/max17042_battery.c
> +++ b/drivers/power/supply/max17042_battery.c
> @@ -150,12 +150,12 @@ static int max17042_get_battery_health(struct max17042_chip *chip, int *health)
>  	if (ret < 0)
>  		goto health_error;
>  
> -	if (temp <= chip->pdata->temp_min) {
> +	if (temp < chip->pdata->temp_min) {
>  		*health = POWER_SUPPLY_HEALTH_COLD;
>  		goto out;
>  	}
>  
> -	if (temp >= chip->pdata->temp_max) {
> +	if (temp > chip->pdata->temp_max) {
>  		*health = POWER_SUPPLY_HEALTH_OVERHEAT;
>  		goto out;
>  	}
> @@ -772,8 +772,9 @@ static void max17042_init_worker(struct work_struct *work)
>  
>  #ifdef CONFIG_OF
>  static struct max17042_platform_data *
> -max17042_get_pdata(struct device *dev)
> +max17042_get_pdata(struct max17042_chip *chip)
>  {
> +	struct device *dev = &chip->client->dev;
>  	struct device_node *np = dev->of_node;
>  	u32 prop;
>  	struct max17042_platform_data *pdata;
> @@ -806,10 +807,55 @@ max17042_get_pdata(struct device *dev)
>  	return pdata;
>  }
>  #else
> +static struct max17042_reg_data max17047_default_pdata_init_regs[] = {
> +	/*
> +	 * Some firmwares do not set FullSOCThr, Enable End-of-Charge Detection
> +	 * when the voltage FG reports 95%, as recommend in the datasheet.
> +	 */
> +	{ MAX17047_FullSOCThr, 95 << 8 },

Maybe use MAX17042_BATTERY_FULL (and replace it to 95?). If not the
MAX17042_BATTERY_FULL can be removed in separate patch.

> +};
> +
>  static struct max17042_platform_data *
> -max17042_get_pdata(struct device *dev)
> +max17042_get_pdata(struct max17042_chip *chip)
>  {
> -	return dev->platform_data;
> +	struct device *dev = &chip->client->dev;
> +	struct max17042_platform_data *pdata;
> +	int ret, misc_cfg;
> +
> +	if (dev->platform_data)
> +		return dev->platform_data;
> +
> +	/*
> +	 * The MAX17047 gets used on x86 where we've no platform data, assume
> +	 * the firmware will already have initialized the fuel-gauge and provide
> +	 * default values for the non init bits to make things work.
> +	 */
> +	pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
> +	if (!pdata)
> +		return pdata;
> +
> +	if (chip->chip_type != MAXIM_DEVICE_TYPE_MAX17042) {
> +		pdata->init_data = max17047_default_pdata_init_regs;
> +		pdata->num_init_data =
> +			ARRAY_SIZE(max17047_default_pdata_init_regs);
> +	}
> +
> +	ret = regmap_read(chip->regmap, MAX17042_MiscCFG, &misc_cfg);
> +	if (ret < 0)
> +		return NULL;
> +
> +	/* If bits 0-1 are set to 3 then only Voltage readings are used */
> +	if ((misc_cfg & 0x3) == 3)

Mixing 0x3 and 3 looks suspicious. How about " == 0x3"?

> +		pdata->enable_current_sense = false;
> +	else
> +		pdata->enable_current_sense = true;

In that case you need to set the r_sns (unless
MAX17042_DEFAULT_SNS_RESISTOR is okay for you).
> +
> +	pdata->vmin = 3000;
> +	pdata->vmax = 4500;	/* Some devices use a LiHV cell */
> +	pdata->temp_min = 0;	/* Some devices do not have a temp sensor */
> +	pdata->temp_max = 700;	/* 70 degrees Celcius */

How about putting all of these hard-coded numbers next to
MAX17042_DEFAULT_SNS_RESISTOR?

Best regards,
Krzysztof

Re: [PATCH 06/13] power: max17042_battery: Add support for the STATUS property

[Krzysztof Kozlowski]

On Fri, Apr 14, 2017 at 02:59:12PM +0200, Hans de Goede wrote:
> Userspace prefers the driver having a status property over having to guess
> itself. Specifically this will properly make the GNOME3 UI (and likely
> others) properly show discharging / charging / full status, instead
> of always showing discharging as status.
> 
> Note that in the case there is no charger driver supplying the max17042,
> then a status of unknown will get returned. At least upower treats
> this the same as not having a status attribute, so in this case nothing
> changes from a userspace pov.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/power/supply/max17042_battery.c | 49 +++++++++++++++++++++++++++++++++
>  1 file changed, 49 insertions(+)
> 
> diff --git a/drivers/power/supply/max17042_battery.c b/drivers/power/supply/max17042_battery.c
> index f8a0384..1ea8368 100644
> --- a/drivers/power/supply/max17042_battery.c
> +++ b/drivers/power/supply/max17042_battery.c
> @@ -65,6 +65,9 @@
>  
>  #define MAX17042_VMAX_TOLERANCE		50 /* 50 mV */
>  
> +/* Consider RepCap which is less then 10 units below FullCAP full */
> +#define MAX17042_FULL_THRESHOLD		10

How about grouping all of defaults and configuration values in one
place? Some of them are already in
include/linux/power/max17042_battery.h.

Best regards,
Krzysztof

Re: [PATCH 07/13] power: max17042_battery: Add external_power_changed callback

[Krzysztof Kozlowski]

On Fri, Apr 14, 2017 at 02:59:13PM +0200, Hans de Goede wrote:
> If our supplier changes status, chances are we've changed status too,
> let any listeners know about this.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/power/supply/max17042_battery.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 

Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>

Best regards,
Krzysztof

Re: [PATCH 08/13] power: max17042_battery: Add support for the TECHNOLOGY attribute

[Krzysztof Kozlowski]

On Fri, Apr 14, 2017 at 02:59:14PM +0200, Hans de Goede wrote:
> The max17042 is intended for LiOn or LiPo batteries, add a TECHNOLOGY
> attribute to reflect this. Note this is hardcoded to LiOn as there is
> no way to tell the difference, this is done by many drivers and is
> better then not providing any technology info at all.

Same as Wolfgang's try:
https://patchwork.kernel.org/patch/9349945/
but with better explanation. :)

Yet still I am not convinced that providing a wrong information (Li-Ion
in case of LiPo battery) is better than not providing any info at all.

Best regards,
Krzysztof

Re: [PATCH 09/13] power: max17042_battery: Add support for the VOLT_MIN property

[Krzysztof Kozlowski]

On Fri, Apr 14, 2017 at 02:59:15PM +0200, Hans de Goede wrote:
> The info is there, so lets export it, like we already do for VOLT_MAX.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/power/supply/max17042_battery.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 

Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>

Best regards,
Krzysztof

Re: [PATCH 10/13] power: max17042_battery: Add support for the CHARGE_FULL_DESIGN property

[Krzysztof Kozlowski]

On Fri, Apr 14, 2017 at 02:59:16PM +0200, Hans de Goede wrote:
> The info is there, lets export it as a property.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/power/supply/max17042_battery.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/power/supply/max17042_battery.c b/drivers/power/supply/max17042_battery.c
> index 770e188..56cfff7 100644
> --- a/drivers/power/supply/max17042_battery.c
> +++ b/drivers/power/supply/max17042_battery.c
> @@ -90,6 +90,7 @@ static enum power_supply_property max17042_battery_props[] = {
>  	POWER_SUPPLY_PROP_VOLTAGE_AVG,
>  	POWER_SUPPLY_PROP_VOLTAGE_OCV,
>  	POWER_SUPPLY_PROP_CAPACITY,
> +	POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN,
>  	POWER_SUPPLY_PROP_CHARGE_FULL,
>  	POWER_SUPPLY_PROP_CHARGE_COUNTER,
>  	POWER_SUPPLY_PROP_TEMP,
> @@ -307,6 +308,13 @@ static int max17042_get_property(struct power_supply *psy,
>  
>  		val->intval = data >> 8;
>  		break;
> +	case POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN:
> +		ret = regmap_read(map, MAX17042_DesignCap, &data);
> +		if (ret < 0)
> +			return ret;
> +
> +		val->intval = data * 1000 / 2;

You need to include the rsns. The step 0.5 mAh is valid only for  10
mOhm resistor.

Best regards,
Krzysztof

> +		break;
>  	case POWER_SUPPLY_PROP_CHARGE_FULL:
>  		ret = regmap_read(map, MAX17042_FullCAP, &data);
>  		if (ret < 0)
> -- 
> 2.9.3
> 

Re: [PATCH 11/13] power: max17042_battery: Add support for CHARGE_NOW property

[Krzysztof Kozlowski]

On Fri, Apr 14, 2017 at 02:59:17PM +0200, Hans de Goede wrote:
> Atleast upower prefers the more precise charge_now sysfs value over
> capacity and the max17042 has the info, so lets export it.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/power/supply/max17042_battery.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/power/supply/max17042_battery.c b/drivers/power/supply/max17042_battery.c
> index 56cfff7..fab4b95 100644
> --- a/drivers/power/supply/max17042_battery.c
> +++ b/drivers/power/supply/max17042_battery.c
> @@ -92,6 +92,7 @@ static enum power_supply_property max17042_battery_props[] = {
>  	POWER_SUPPLY_PROP_CAPACITY,
>  	POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN,
>  	POWER_SUPPLY_PROP_CHARGE_FULL,
> +	POWER_SUPPLY_PROP_CHARGE_NOW,
>  	POWER_SUPPLY_PROP_CHARGE_COUNTER,
>  	POWER_SUPPLY_PROP_TEMP,
>  	POWER_SUPPLY_PROP_TEMP_ALERT_MIN,
> @@ -322,6 +323,13 @@ static int max17042_get_property(struct power_supply *psy,
>  
>  		val->intval = data * 1000 / 2;
>  		break;
> +	case POWER_SUPPLY_PROP_CHARGE_NOW:
> +		ret = regmap_read(map, MAX17042_RepCap, &data);
> +		if (ret < 0)
> +			return ret;
> +
> +		val->intval = data * 1000 / 2;

Same as in previous, missing sense resistor in calculation.

Best regards,
Krzysztof


> +		break;
>  	case POWER_SUPPLY_PROP_CHARGE_COUNTER:
>  		ret = regmap_read(map, MAX17042_QH, &data);
>  		if (ret < 0)
> -- 
> 2.9.3
> 

Re: [PATCH 12/13] power: max17042_battery: Add support for SCOPE property

[Krzysztof Kozlowski]

On Fri, Apr 14, 2017 at 02:59:18PM +0200, Hans de Goede wrote:
> Add support for the SCOPE property, always return SCOPE_SYSTEM,
> as the max170xx is used for the main battery on all known systems
> with a max170xx.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/power/supply/max17042_battery.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 

Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>

Best regards,
Krzysztof

Re: [PATCH 13/13] power: max77693_charger: Add supplied_to info to power_supply_config

[Krzysztof Kozlowski]

On Fri, Apr 14, 2017 at 02:59:19PM +0200, Hans de Goede wrote:
> On the exynos4412-trats2 the max77693 is used together with a
> max17047 fuel-gauge. Add supplied_to info containing "main-battery",
> so that the get_status code in the max17042_battery driver can use
> power_supply_am_i_supplied and can properly let userspace know
> if the battery is being charged or not.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/power/supply/max77693_charger.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 

Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>

Best regards,
Krzysztof

Re: [PATCH 04/13] power: max17042_battery: Add default platform_data for x86 use

[Hans de Goede]

Hi,

On 14-04-17 17:54, Krzysztof Kozlowski wrote:
 > On Fri, Apr 14, 2017 at 05:43:37PM +0200, Hans de Goede wrote:
 >> Hi,
 >>
 >> On 14-04-17 17:35, Krzysztof Kozlowski wrote:
 >>> On Fri, Apr 14, 2017 at 02:59:10PM +0200, Hans de Goede wrote:
 >>>> Some x86 machines use a max17047 fuel-gauge. X86 does not have board
 >>>> files / dt to provide platform data, so add default platform_data
 >>>> as fallback option so that the driver can work on x86.
 >>>
 >>> I am not convinced by this explanation. AFAIR, the x86 boards which use
 >>> max17042-family are providing the platform data. See commit edd4ab055931
 >>> ("power: max17042_battery: add HEALTH and TEMP_* properties support").
 >>
 >> Yes that commits adds the members to the struct max17042_platform_data,
 >> but if you grep through the mainline kernel there is no code what so
 >> ever ever filling such a struct.
 >
 > Yes, my assumption was they are coming out of tree - through Simple
 > Firmware Interface. At least that was how I understood Ramakrishna's
 > reply:
 > https://patchwork.kernel.org/patch/6326981/
 >
 > I think the commit message is a little bit non-precise here. How about
 > changing the initial sentence adding "might" ("86 might be missing platform_data if
 > not provided by SFI" or something similar).

Ok, will do for v2.

>> Since not all boards have a thermistor hooked up, set temp_min to 0 and
>> change the health checks from temp <= temp_min to temp < temp_min to
>> not trigger on such boards (where temp reads 0).
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>  drivers/power/supply/max17042_battery.c | 60 +++++++++++++++++++++++++++++----
>>  1 file changed, 53 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/power/supply/max17042_battery.c b/drivers/power/supply/max17042_battery.c
>> index a51b296..85a6bf3 100644
>> --- a/drivers/power/supply/max17042_battery.c
>> +++ b/drivers/power/supply/max17042_battery.c
>> @@ -150,12 +150,12 @@ static int max17042_get_battery_health(struct max17042_chip *chip, int *health)
>>  	if (ret < 0)
>>  		goto health_error;
>>
>> -	if (temp <= chip->pdata->temp_min) {
>> +	if (temp < chip->pdata->temp_min) {
>>  		*health = POWER_SUPPLY_HEALTH_COLD;
>>  		goto out;
>>  	}
>>
>> -	if (temp >= chip->pdata->temp_max) {
>> +	if (temp > chip->pdata->temp_max) {
>>  		*health = POWER_SUPPLY_HEALTH_OVERHEAT;
>>  		goto out;
>>  	}
>> @@ -772,8 +772,9 @@ static void max17042_init_worker(struct work_struct *work)
>>
>>  #ifdef CONFIG_OF
>>  static struct max17042_platform_data *
>> -max17042_get_pdata(struct device *dev)
>> +max17042_get_pdata(struct max17042_chip *chip)
>>  {
>> +	struct device *dev = &chip->client->dev;
>>  	struct device_node *np = dev->of_node;
>>  	u32 prop;
>>  	struct max17042_platform_data *pdata;
>> @@ -806,10 +807,55 @@ max17042_get_pdata(struct device *dev)
>>  	return pdata;
>>  }
>>  #else
>> +static struct max17042_reg_data max17047_default_pdata_init_regs[] = {
>> +	/*
>> +	 * Some firmwares do not set FullSOCThr, Enable End-of-Charge Detection
>> +	 * when the voltage FG reports 95%, as recommend in the datasheet.
>> +	 */
>> +	{ MAX17047_FullSOCThr, 95 << 8 },
>
> Maybe use MAX17042_BATTERY_FULL (and replace it to 95?). If not the
> MAX17042_BATTERY_FULL can be removed in separate patch.

Ok, will do for v2.

>> +};
>> +
>>  static struct max17042_platform_data *
>> -max17042_get_pdata(struct device *dev)
>> +max17042_get_pdata(struct max17042_chip *chip)
>>  {
>> -	return dev->platform_data;
>> +	struct device *dev = &chip->client->dev;
>> +	struct max17042_platform_data *pdata;
>> +	int ret, misc_cfg;
>> +
>> +	if (dev->platform_data)
>> +		return dev->platform_data;
>> +
>> +	/*
>> +	 * The MAX17047 gets used on x86 where we've no platform data, assume
>> +	 * the firmware will already have initialized the fuel-gauge and provide
>> +	 * default values for the non init bits to make things work.
>> +	 */
>> +	pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
>> +	if (!pdata)
>> +		return pdata;
>> +
>> +	if (chip->chip_type != MAXIM_DEVICE_TYPE_MAX17042) {
>> +		pdata->init_data = max17047_default_pdata_init_regs;
>> +		pdata->num_init_data =
>> +			ARRAY_SIZE(max17047_default_pdata_init_regs);
>> +	}
>> +
>> +	ret = regmap_read(chip->regmap, MAX17042_MiscCFG, &misc_cfg);
>> +	if (ret < 0)
>> +		return NULL;
>> +
>> +	/* If bits 0-1 are set to 3 then only Voltage readings are used */
>> +	if ((misc_cfg & 0x3) == 3)
>
> Mixing 0x3 and 3 looks suspicious. How about " == 0x3"?

Ok, will do for v2.

>> +		pdata->enable_current_sense = false;
>> +	else
>> +		pdata->enable_current_sense = true;
>
> In that case you need to set the r_sns (unless
> MAX17042_DEFAULT_SNS_RESISTOR is okay for you).

The default value is ok for me / my board.

>> +
>> +	pdata->vmin = 3000;
>> +	pdata->vmax = 4500;	/* Some devices use a LiHV cell */
>> +	pdata->temp_min = 0;	/* Some devices do not have a temp sensor */
>> +	pdata->temp_max = 700;	/* 70 degrees Celcius */
>
> How about putting all of these hard-coded numbers next to
> MAX17042_DEFAULT_SNS_RESISTOR?

Ok, will also do for v2 :)

Thank you for all the reviews.

Regards,

Hans

Re: [PATCH 06/13] power: max17042_battery: Add support for the STATUS property

[Hans de Goede]

Hi,

On 14-04-17 18:11, Krzysztof Kozlowski wrote:
> On Fri, Apr 14, 2017 at 02:59:12PM +0200, Hans de Goede wrote:
>> Userspace prefers the driver having a status property over having to guess
>> itself. Specifically this will properly make the GNOME3 UI (and likely
>> others) properly show discharging / charging / full status, instead
>> of always showing discharging as status.
>>
>> Note that in the case there is no charger driver supplying the max17042,
>> then a status of unknown will get returned. At least upower treats
>> this the same as not having a status attribute, so in this case nothing
>> changes from a userspace pov.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>  drivers/power/supply/max17042_battery.c | 49 +++++++++++++++++++++++++++++++++
>>  1 file changed, 49 insertions(+)
>>
>> diff --git a/drivers/power/supply/max17042_battery.c b/drivers/power/supply/max17042_battery.c
>> index f8a0384..1ea8368 100644
>> --- a/drivers/power/supply/max17042_battery.c
>> +++ b/drivers/power/supply/max17042_battery.c
>> @@ -65,6 +65,9 @@
>>
>>  #define MAX17042_VMAX_TOLERANCE		50 /* 50 mV */
>>
>> +/* Consider RepCap which is less then 10 units below FullCAP full */
>> +#define MAX17042_FULL_THRESHOLD		10
>
> How about grouping all of defaults and configuration values in one
> place? Some of them are already in
> include/linux/power/max17042_battery.h.

Ok, I will move this to include/linux/power/max17042_battery.h for v2.

Regards,

Hans

Re: [PATCH 08/13] power: max17042_battery: Add support for the TECHNOLOGY attribute

[Hans de Goede]

Hi,

On 14-04-17 18:26, Krzysztof Kozlowski wrote:
> On Fri, Apr 14, 2017 at 02:59:14PM +0200, Hans de Goede wrote:
>> The max17042 is intended for LiOn or LiPo batteries, add a TECHNOLOGY
>> attribute to reflect this. Note this is hardcoded to LiOn as there is
>> no way to tell the difference, this is done by many drivers and is
>> better then not providing any technology info at all.
>
> Same as Wolfgang's try:
> https://patchwork.kernel.org/patch/9349945/
> but with better explanation. :)

Hehe.

> Yet still I am not convinced that providing a wrong information (Li-Ion
> in case of LiPo battery) is better than not providing any info at all.

Well lithium-ion polymer batteries to use their full name are a subset
of the Li-Ion battery family so returning TECHNOLOGY_LION for them is
technically correct. The technology attribute gets read by for example
upower and then shown by for example mate-power-manager to the user:

https://github.com/mate-desktop/mate-power-manager/tree/master/src#L390

It would be nice to actually show something there, even if we're not
correctly identifying what sub-family of lithium-ion battery we are
actually dealing with.

Regards,

Hans

Re: [PATCH 10/13] power: max17042_battery: Add support for the CHARGE_FULL_DESIGN property

[Hans de Goede]

Hi,

On 14-04-17 18:34, Krzysztof Kozlowski wrote:
> On Fri, Apr 14, 2017 at 02:59:16PM +0200, Hans de Goede wrote:
>> The info is there, lets export it as a property.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>  drivers/power/supply/max17042_battery.c | 8 ++++++++
>>  1 file changed, 8 insertions(+)
>>
>> diff --git a/drivers/power/supply/max17042_battery.c b/drivers/power/supply/max17042_battery.c
>> index 770e188..56cfff7 100644
>> --- a/drivers/power/supply/max17042_battery.c
>> +++ b/drivers/power/supply/max17042_battery.c
>> @@ -90,6 +90,7 @@ static enum power_supply_property max17042_battery_props[] = {
>>  	POWER_SUPPLY_PROP_VOLTAGE_AVG,
>>  	POWER_SUPPLY_PROP_VOLTAGE_OCV,
>>  	POWER_SUPPLY_PROP_CAPACITY,
>> +	POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN,
>>  	POWER_SUPPLY_PROP_CHARGE_FULL,
>>  	POWER_SUPPLY_PROP_CHARGE_COUNTER,
>>  	POWER_SUPPLY_PROP_TEMP,
>> @@ -307,6 +308,13 @@ static int max17042_get_property(struct power_supply *psy,
>>
>>  		val->intval = data >> 8;
>>  		break;
>> +	case POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN:
>> +		ret = regmap_read(map, MAX17042_DesignCap, &data);
>> +		if (ret < 0)
>> +			return ret;
>> +
>> +		val->intval = data * 1000 / 2;
>
> You need to include the rsns. The step 0.5 mAh is valid only for  10
> mOhm resistor.

You're right, will fix. Note I copy pasted this from the original
code for CHARGE_FULL, so I will add a patch fixing that.

Regards,

Hans


>
> Best regards,
> Krzysztof
>
>> +		break;
>>  	case POWER_SUPPLY_PROP_CHARGE_FULL:
>>  		ret = regmap_read(map, MAX17042_FullCAP, &data);
>>  		if (ret < 0)
>> --
>> 2.9.3
>>

Re: [PATCH 08/13] power: max17042_battery: Add support for the TECHNOLOGY attribute

[Krzysztof Kozlowski]

On Fri, Apr 14, 2017 at 08:06:56PM +0200, Hans de Goede wrote:
> Hi,
> 
> On 14-04-17 18:26, Krzysztof Kozlowski wrote:
> > On Fri, Apr 14, 2017 at 02:59:14PM +0200, Hans de Goede wrote:
> > > The max17042 is intended for LiOn or LiPo batteries, add a TECHNOLOGY
> > > attribute to reflect this. Note this is hardcoded to LiOn as there is
> > > no way to tell the difference, this is done by many drivers and is
> > > better then not providing any technology info at all.
> > 
> > Same as Wolfgang's try:
> > https://patchwork.kernel.org/patch/9349945/
> > but with better explanation. :)
> 
> Hehe.
> 
> > Yet still I am not convinced that providing a wrong information (Li-Ion
> > in case of LiPo battery) is better than not providing any info at all.
> 
> Well lithium-ion polymer batteries to use their full name are a subset
> of the Li-Ion battery family so returning TECHNOLOGY_LION for them is
> technically correct. The technology attribute gets read by for example
> upower and then shown by for example mate-power-manager to the user:
> 
> https://github.com/mate-desktop/mate-power-manager/tree/master/src#L390

...which differentiates between Li-Ion and LiPo.

The same with kernel's power supply API - we have the
POWER_SUPPLY_TECHNOLOGY_LIPO for some reason.

> It would be nice to actually show something there, even if we're not
> correctly identifying what sub-family of lithium-ion battery we are
> actually dealing with.

You're right that it is a subfamily. I do not know what it is expected
from this interface, e.g. if stating LIPO as LION is okay or not. I see
some benefit of providing a fixed LION (especially that in mobile use
cases of max17042 these are Li-Ion). On the other hand, it might be
inaccurate. Probably harmless but inaccurate.

I do not oppose the patch.

You might consider giving some credits to Wolfgang's try with
Suggested-by.

Best regards,
Krzysztof

Re: [PATCH 08/13] power: max17042_battery: Add support for the TECHNOLOGY attribute

[Hans de Goede]

Hi,

On 14-04-17 20:20, Krzysztof Kozlowski wrote:
> On Fri, Apr 14, 2017 at 08:06:56PM +0200, Hans de Goede wrote:
>> Hi,
>>
>> On 14-04-17 18:26, Krzysztof Kozlowski wrote:
>>> On Fri, Apr 14, 2017 at 02:59:14PM +0200, Hans de Goede wrote:
>>>> The max17042 is intended for LiOn or LiPo batteries, add a TECHNOLOGY
>>>> attribute to reflect this. Note this is hardcoded to LiOn as there is
>>>> no way to tell the difference, this is done by many drivers and is
>>>> better then not providing any technology info at all.
>>>
>>> Same as Wolfgang's try:
>>> https://patchwork.kernel.org/patch/9349945/
>>> but with better explanation. :)
>>
>> Hehe.
>>
>>> Yet still I am not convinced that providing a wrong information (Li-Ion
>>> in case of LiPo battery) is better than not providing any info at all.
>>
>> Well lithium-ion polymer batteries to use their full name are a subset
>> of the Li-Ion battery family so returning TECHNOLOGY_LION for them is
>> technically correct. The technology attribute gets read by for example
>> upower and then shown by for example mate-power-manager to the user:
>>
>> https://github.com/mate-desktop/mate-power-manager/tree/master/src#L390
>
> ...which differentiates between Li-Ion and LiPo.
>
> The same with kernel's power supply API - we have the
> POWER_SUPPLY_TECHNOLOGY_LIPO for some reason.
>
>> It would be nice to actually show something there, even if we're not
>> correctly identifying what sub-family of lithium-ion battery we are
>> actually dealing with.
>
> You're right that it is a subfamily. I do not know what it is expected
> from this interface, e.g. if stating LIPO as LION is okay or not. I see
> some benefit of providing a fixed LION (especially that in mobile use
> cases of max17042 these are Li-Ion). On the other hand, it might be
> inaccurate. Probably harmless but inaccurate.
>
> I do not oppose the patch.
>
> You might consider giving some credits to Wolfgang's try with
> Suggested-by.

I've added the Suggested-by and improved the commit msg a bit for v2.

Regards,

Hans