[PATCH 0/4] MAX17055 model configuration via DT

[PATCH 0/4] MAX17055 model configuration via DT

[Sebastian Krzyszkowiak]

Currently, there's no way to supply battery characteristics to max17042
driver on device tree platforms. This series changes that in a way that's
sufficient to configure MAX17055's m5 EZ algorithm, by using a standard
"monitored-battery" phandle.

Sebastian Krzyszkowiak (4):
  power: supply: max17042_battery: Add unit conversion macros
  power: supply: max17042_battery: use ModelCfg refresh on max17055
  dt-bindings: power: supply: max17042: Add monitored-battery phandle
  power: supply: max17042_battery: read battery properties from device
    tree

 .../bindings/power/supply/maxim,max17042.yaml |   4 +
 drivers/power/supply/max17042_battery.c       | 163 ++++++++++++------
 include/linux/power/max17042_battery.h        |   4 +
 3 files changed, 116 insertions(+), 55 deletions(-)

-- 
2.35.1

[PATCH 1/4] power: supply: max17042_battery: Add unit conversion macros

[Sebastian Krzyszkowiak]

Instead of sprinkling the code with magic numbers, put the unit
definitions used by the gauge into a set of macros. Macros are
used instead of simple defines in order to not require floating
point operations for divisions.

Signed-off-by: Sebastian Krzyszkowiak <sebastian.krzyszkowiak@puri.sm>
---
 drivers/power/supply/max17042_battery.c | 40 +++++++++++++++----------
 1 file changed, 24 insertions(+), 16 deletions(-)

diff --git a/drivers/power/supply/max17042_battery.c b/drivers/power/supply/max17042_battery.c
index ab031bbfbe78..c019d6c52363 100644
--- a/drivers/power/supply/max17042_battery.c
+++ b/drivers/power/supply/max17042_battery.c
@@ -51,6 +51,15 @@
 
 #define MAX17042_VMAX_TOLERANCE		50 /* 50 mV */
 
+#define MAX17042_CURRENT_LSB		1562500ll /* µV */
+#define MAX17042_CURRENT_RSENSE(x)	(x * MAX17042_CURRENT_LSB) /* µV */
+#define MAX17042_CAPACITY_LSB		5000000ll /* µVh */
+#define MAX17042_CAPACITY_RSENSE(x)	(x * MAX17042_CAPACITY_LSB) /* µVh */
+#define MAX17042_TIME(x)		(x * 5625 / 1000) /* s */
+#define MAX17042_VOLTAGE(x)		(x * 625 / 8) /* µV */
+#define MAX17042_RESISTANCE(x)		(x / 4096) /* Ω */
+#define MAX17042_TEMPERATURE(x)		(x / 256) /* °C */
+
 struct max17042_chip {
 	struct i2c_client *client;
 	struct regmap *regmap;
@@ -103,8 +112,7 @@ static int max17042_get_temperature(struct max17042_chip *chip, int *temp)
 
 	*temp = sign_extend32(data, 15);
 	/* The value is converted into deci-centigrade scale */
-	/* Units of LSB = 1 / 256 degree Celsius */
-	*temp = *temp * 10 / 256;
+	*temp = MAX17042_TEMPERATURE(*temp * 10);
 	return 0;
 }
 
@@ -161,7 +169,7 @@ static int max17042_get_status(struct max17042_chip *chip, int *status)
 		return ret;
 
 	avg_current = sign_extend32(data, 15);
-	avg_current *= 1562500 / chip->pdata->r_sns;
+	avg_current *= MAX17042_CURRENT_LSB / chip->pdata->r_sns;
 
 	if (avg_current > 0)
 		*status = POWER_SUPPLY_STATUS_CHARGING;
@@ -181,7 +189,7 @@ static int max17042_get_battery_health(struct max17042_chip *chip, int *health)
 		goto health_error;
 
 	/* bits [0-3] unused */
-	vavg = val * 625 / 8;
+	vavg = MAX17042_VOLTAGE(val);
 	/* Convert to millivolts */
 	vavg /= 1000;
 
@@ -190,7 +198,7 @@ static int max17042_get_battery_health(struct max17042_chip *chip, int *health)
 		goto health_error;
 
 	/* bits [0-3] unused */
-	vbatt = val * 625 / 8;
+	vbatt = MAX17042_VOLTAGE(val);
 	/* Convert to millivolts */
 	vbatt /= 1000;
 
@@ -297,21 +305,21 @@ static int max17042_get_property(struct power_supply *psy,
 		if (ret < 0)
 			return ret;
 
-		val->intval = data * 625 / 8;
+		val->intval = MAX17042_VOLTAGE(data);
 		break;
 	case POWER_SUPPLY_PROP_VOLTAGE_AVG:
 		ret = regmap_read(map, MAX17042_AvgVCELL, &data);
 		if (ret < 0)
 			return ret;
 
-		val->intval = data * 625 / 8;
+		val->intval = MAX17042_VOLTAGE(data);
 		break;
 	case POWER_SUPPLY_PROP_VOLTAGE_OCV:
 		ret = regmap_read(map, MAX17042_OCVInternal, &data);
 		if (ret < 0)
 			return ret;
 
-		val->intval = data * 625 / 8;
+		val->intval = MAX17042_VOLTAGE(data);
 		break;
 	case POWER_SUPPLY_PROP_CAPACITY:
 		if (chip->pdata->enable_current_sense)
@@ -328,7 +336,7 @@ static int max17042_get_property(struct power_supply *psy,
 		if (ret < 0)
 			return ret;
 
-		data64 = data * 5000000ll;
+		data64 = MAX17042_CAPACITY_RSENSE(data);
 		do_div(data64, chip->pdata->r_sns);
 		val->intval = data64;
 		break;
@@ -337,7 +345,7 @@ static int max17042_get_property(struct power_supply *psy,
 		if (ret < 0)
 			return ret;
 
-		data64 = data * 5000000ll;
+		data64 = MAX17042_CAPACITY_RSENSE(data);
 		do_div(data64, chip->pdata->r_sns);
 		val->intval = data64;
 		break;
@@ -346,7 +354,7 @@ static int max17042_get_property(struct power_supply *psy,
 		if (ret < 0)
 			return ret;
 
-		data64 = data * 5000000ll;
+		data64 = MAX17042_CAPACITY_RSENSE(data);
 		do_div(data64, chip->pdata->r_sns);
 		val->intval = data64;
 		break;
@@ -355,7 +363,7 @@ static int max17042_get_property(struct power_supply *psy,
 		if (ret < 0)
 			return ret;
 
-		data64 = sign_extend64(data, 15) * 5000000ll;
+		data64 = MAX17042_CAPACITY_RSENSE(sign_extend64(data, 15));
 		val->intval = div_s64(data64, chip->pdata->r_sns);
 		break;
 	case POWER_SUPPLY_PROP_TEMP:
@@ -397,7 +405,7 @@ static int max17042_get_property(struct power_supply *psy,
 			if (ret < 0)
 				return ret;
 
-			data64 = sign_extend64(data, 15) * 1562500ll;
+			data64 = MAX17042_CURRENT_RSENSE(sign_extend64(data, 15));
 			val->intval = div_s64(data64, chip->pdata->r_sns);
 		} else {
 			return -EINVAL;
@@ -409,7 +417,7 @@ static int max17042_get_property(struct power_supply *psy,
 			if (ret < 0)
 				return ret;
 
-			data64 = sign_extend64(data, 15) * 1562500ll;
+			data64 = MAX17042_CURRENT_RSENSE(sign_extend64(data, 15));
 			val->intval = div_s64(data64, chip->pdata->r_sns);
 		} else {
 			return -EINVAL;
@@ -420,7 +428,7 @@ static int max17042_get_property(struct power_supply *psy,
 		if (ret < 0)
 			return ret;
 
-		data64 = data * 1562500ll;
+		data64 = MAX17042_CURRENT_RSENSE(data);
 		val->intval = div_s64(data64, chip->pdata->r_sns);
 		break;
 	case POWER_SUPPLY_PROP_TIME_TO_EMPTY_NOW:
@@ -428,7 +436,7 @@ static int max17042_get_property(struct power_supply *psy,
 		if (ret < 0)
 			return ret;
 
-		val->intval = data * 5625 / 1000;
+		val->intval = MAX17042_TIME(data);
 		break;
 	default:
 		return -EINVAL;
-- 
2.35.1

[PATCH 2/4] power: supply: max17042_battery: use ModelCfg refresh on max17055

[Sebastian Krzyszkowiak]

Unlike other models, max17055 doesn't require cell characterization
data and operates on smaller amount of input variables (DesignCap,
VEmpty, IChgTerm and ModelCfg). Input data can already be filled in
by max17042_override_por_values, however model refresh bit has to be
set after adjusting input variables in order to make them apply.

Signed-off-by: Sebastian Krzyszkowiak <sebastian.krzyszkowiak@puri.sm>
---
 drivers/power/supply/max17042_battery.c | 73 +++++++++++++++----------
 include/linux/power/max17042_battery.h  |  3 +
 2 files changed, 48 insertions(+), 28 deletions(-)

diff --git a/drivers/power/supply/max17042_battery.c b/drivers/power/supply/max17042_battery.c
index c019d6c52363..c39250349a1d 100644
--- a/drivers/power/supply/max17042_battery.c
+++ b/drivers/power/supply/max17042_battery.c
@@ -806,6 +806,13 @@ static inline void max17042_override_por_values(struct max17042_chip *chip)
 	    (chip->chip_type == MAXIM_DEVICE_TYPE_MAX17055)) {
 		max17042_override_por(map, MAX17047_V_empty, config->vempty);
 	}
+
+	if (chip->chip_type == MAXIM_DEVICE_TYPE_MAX17055) {
+		max17042_override_por(map, MAX17055_ModelCfg, config->model_cfg);
+		// VChg is 1 by default, so allow it to be set to 0
+		regmap_update_bits(map, MAX17055_ModelCfg,
+				MAX17055_MODELCFG_VCHG_BIT, config->model_cfg);
+	}
 }
 
 static int max17042_init_chip(struct max17042_chip *chip)
@@ -814,44 +821,54 @@ static int max17042_init_chip(struct max17042_chip *chip)
 	int ret;
 
 	max17042_override_por_values(chip);
+
+	if (chip->chip_type == MAXIM_DEVICE_TYPE_MAX17055) {
+		regmap_write_bits(map, MAX17055_ModelCfg,
+				  MAX17055_MODELCFG_REFRESH_BIT, MAX17055_MODELCFG_REFRESH_BIT);
+	}
+
 	/* After Power up, the MAX17042 requires 500mS in order
 	 * to perform signal debouncing and initial SOC reporting
 	 */
 	msleep(500);
 
-	/* Initialize configuration */
-	max17042_write_config_regs(chip);
-
-	/* write cell characterization data */
-	ret = max17042_init_model(chip);
-	if (ret) {
-		dev_err(&chip->client->dev, "%s init failed\n",
-			__func__);
-		return -EIO;
-	}
+	if ((chip->chip_type == MAXIM_DEVICE_TYPE_MAX17042) ||
+	    (chip->chip_type == MAXIM_DEVICE_TYPE_MAX17047) ||
+	    (chip->chip_type == MAXIM_DEVICE_TYPE_MAX17050)) {
+		/* Initialize configuration */
+		max17042_write_config_regs(chip);
+
+		/* write cell characterization data */
+		ret = max17042_init_model(chip);
+		if (ret) {
+			dev_err(&chip->client->dev, "%s init failed\n",
+				__func__);
+			return -EIO;
+		}
 
-	ret = max17042_verify_model_lock(chip);
-	if (ret) {
-		dev_err(&chip->client->dev, "%s lock verify failed\n",
-			__func__);
-		return -EIO;
-	}
-	/* write custom parameters */
-	max17042_write_custom_regs(chip);
+		ret = max17042_verify_model_lock(chip);
+		if (ret) {
+			dev_err(&chip->client->dev, "%s lock verify failed\n",
+				__func__);
+			return -EIO;
+		}
+		/* write custom parameters */
+		max17042_write_custom_regs(chip);
 
-	/* update capacity params */
-	max17042_update_capacity_regs(chip);
+		/* update capacity params */
+		max17042_update_capacity_regs(chip);
 
-	/* delay must be atleast 350mS to allow VFSOC
-	 * to be calculated from the new configuration
-	 */
-	msleep(350);
+		/* delay must be at least 350mS to allow VFSOC
+		 * to be calculated from the new configuration
+		 */
+		msleep(350);
 
-	/* reset vfsoc0 reg */
-	max17042_reset_vfsoc0_reg(chip);
+		/* reset vfsoc0 reg */
+		max17042_reset_vfsoc0_reg(chip);
 
-	/* load new capacity params */
-	max17042_load_new_capacity_params(chip);
+		/* load new capacity params */
+		max17042_load_new_capacity_params(chip);
+	}
 
 	/* Init complete, Clear the POR bit */
 	regmap_update_bits(map, MAX17042_STATUS, STATUS_POR_BIT, 0x0);
diff --git a/include/linux/power/max17042_battery.h b/include/linux/power/max17042_battery.h
index c417abd2ab70..6943921cab5e 100644
--- a/include/linux/power/max17042_battery.h
+++ b/include/linux/power/max17042_battery.h
@@ -23,6 +23,8 @@
 
 #define MAX17042_CHARACTERIZATION_DATA_SIZE 48
 
+#define MAX17055_MODELCFG_REFRESH_BIT	BIT(15)
+
 enum max17042_register {
 	MAX17042_STATUS		= 0x00,
 	MAX17042_VALRT_Th	= 0x01,
@@ -208,6 +210,7 @@ struct max17042_config_data {
 	u16	full_soc_thresh;	/* 0x13 */
 	u16	design_cap;	/* 0x18 */
 	u16	ichgt_term;	/* 0x1E */
+	u16	model_cfg;	/* 0xDB */
 
 	/* MG3 config */
 	u16	at_rate;	/* 0x04 */
-- 
2.35.1

[PATCH 3/4] dt-bindings: power: supply: max17042: Add monitored-battery phandle

[Sebastian Krzyszkowiak]

In order to let the driver know about the characteristics of the monitored
battery, allow a standard "monitored-battery" property to be specified.

Cc: devicetree@vger.kernel.org
Signed-off-by: Sebastian Krzyszkowiak <sebastian.krzyszkowiak@puri.sm>
---
 .../devicetree/bindings/power/supply/maxim,max17042.yaml      | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/power/supply/maxim,max17042.yaml b/Documentation/devicetree/bindings/power/supply/maxim,max17042.yaml
index 971b53c58cc6..88c9f466f2c5 100644
--- a/Documentation/devicetree/bindings/power/supply/maxim,max17042.yaml
+++ b/Documentation/devicetree/bindings/power/supply/maxim,max17042.yaml
@@ -59,6 +59,10 @@ properties:
       Voltage threshold to report battery as over voltage (in mV).
       Default is not to report over-voltage events.
 
+  monitored-battery:
+    $ref: /schemas/types.yaml#/definitions/phandle
+    description: phandle to the battery node being monitored
+
 required:
   - compatible
   - reg
-- 
2.35.1

[PATCH 4/4] power: supply: max17042_battery: read battery properties from device tree

[Sebastian Krzyszkowiak]

So far configuring the gauge was only possible using platform data,
with no way to provide the configuration on device tree-based platforms.

Change that by looking up the configuration values from monitored-battery
property. This is especially useful on models implementing ModelGauge m5 EZ
algorithm, such as MAX17055, as all the required configuration can be
derived from a "simple-battery" DT node there.

In order to be able to access power supply framework in get_of_pdata,
move devm_power_supply_register earlier in max17042_probe.

Signed-off-by: Sebastian Krzyszkowiak <sebastian.krzyszkowiak@puri.sm>
---
 drivers/power/supply/max17042_battery.c | 50 +++++++++++++++++++------
 include/linux/power/max17042_battery.h  |  1 +
 2 files changed, 40 insertions(+), 11 deletions(-)

diff --git a/drivers/power/supply/max17042_battery.c b/drivers/power/supply/max17042_battery.c
index c39250349a1d..4c33565802d5 100644
--- a/drivers/power/supply/max17042_battery.c
+++ b/drivers/power/supply/max17042_battery.c
@@ -937,7 +937,9 @@ max17042_get_of_pdata(struct max17042_chip *chip)
 	struct device *dev = &chip->client->dev;
 	struct device_node *np = dev->of_node;
 	u32 prop;
+	u64 data64;
 	struct max17042_platform_data *pdata;
+	struct power_supply_battery_info *info;
 
 	pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
 	if (!pdata)
@@ -961,6 +963,32 @@ max17042_get_of_pdata(struct max17042_chip *chip)
 	if (of_property_read_s32(np, "maxim,over-volt", &pdata->vmax))
 		pdata->vmax = INT_MAX;
 
+	if (pdata->enable_current_sense &&
+	    power_supply_get_battery_info(chip->battery, &info) == 0) {
+		pdata->config_data = devm_kzalloc(dev, sizeof(*pdata->config_data), GFP_KERNEL);
+		if (!pdata->config_data)
+			return NULL;
+
+		if (info->charge_full_design_uah != -EINVAL) {
+			data64 = (u64)info->charge_full_design_uah * pdata->r_sns;
+			do_div(data64, MAX17042_CAPACITY_LSB);
+			pdata->config_data->design_cap = (u16)data64;
+			pdata->enable_por_init = true;
+		}
+		if (info->charge_term_current_ua != -EINVAL) {
+			data64 = (u64)info->charge_term_current_ua * pdata->r_sns;
+			do_div(data64, MAX17042_CURRENT_LSB);
+			pdata->config_data->ichgt_term = (u16)data64;
+			pdata->enable_por_init = true;
+		}
+		if (chip->chip_type == MAXIM_DEVICE_TYPE_MAX17055) {
+			if (info->voltage_max_design_uv > 4250000) {
+				pdata->config_data->model_cfg = MAX17055_MODELCFG_VCHG_BIT;
+				pdata->enable_por_init = true;
+			}
+		}
+	}
+
 	return pdata;
 }
 #endif
@@ -1092,16 +1120,23 @@ static int max17042_probe(struct i2c_client *client,
 		return -EINVAL;
 	}
 
+	i2c_set_clientdata(client, chip);
+	psy_cfg.drv_data = chip;
+	psy_cfg.of_node = dev->of_node;
+
+	chip->battery = devm_power_supply_register(&client->dev, max17042_desc,
+						   &psy_cfg);
+	if (IS_ERR(chip->battery)) {
+		dev_err(&client->dev, "failed: power supply register\n");
+		return PTR_ERR(chip->battery);
+	}
+
 	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);
-	psy_cfg.drv_data = chip;
-	psy_cfg.of_node = dev->of_node;
-
 	/* When current is not measured,
 	 * CURRENT_NOW and CURRENT_AVG properties should be invisible. */
 	if (!chip->pdata->enable_current_sense)
@@ -1122,13 +1157,6 @@ static int max17042_probe(struct i2c_client *client,
 		regmap_write(chip->regmap, MAX17042_LearnCFG, 0x0007);
 	}
 
-	chip->battery = devm_power_supply_register(&client->dev, max17042_desc,
-						   &psy_cfg);
-	if (IS_ERR(chip->battery)) {
-		dev_err(&client->dev, "failed: power supply register\n");
-		return PTR_ERR(chip->battery);
-	}
-
 	if (client->irq) {
 		unsigned int flags = IRQF_ONESHOT;
 
diff --git a/include/linux/power/max17042_battery.h b/include/linux/power/max17042_battery.h
index 6943921cab5e..367620800e7e 100644
--- a/include/linux/power/max17042_battery.h
+++ b/include/linux/power/max17042_battery.h
@@ -23,6 +23,7 @@
 
 #define MAX17042_CHARACTERIZATION_DATA_SIZE 48
 
+#define MAX17055_MODELCFG_VCHG_BIT	BIT(10)
 #define MAX17055_MODELCFG_REFRESH_BIT	BIT(15)
 
 enum max17042_register {
-- 
2.35.1

Re: [PATCH 1/4] power: supply: max17042_battery: Add unit conversion macros

[Krzysztof Kozlowski]

On 18/03/2022 01:10, Sebastian Krzyszkowiak wrote:
> Instead of sprinkling the code with magic numbers, put the unit
> definitions used by the gauge into a set of macros. Macros are
> used instead of simple defines in order to not require floating
> point operations for divisions.
> 
> Signed-off-by: Sebastian Krzyszkowiak <sebastian.krzyszkowiak@puri.sm>
> ---
>  drivers/power/supply/max17042_battery.c | 40 +++++++++++++++----------
>  1 file changed, 24 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/power/supply/max17042_battery.c b/drivers/power/supply/max17042_battery.c
> index ab031bbfbe78..c019d6c52363 100644
> --- a/drivers/power/supply/max17042_battery.c
> +++ b/drivers/power/supply/max17042_battery.c
> @@ -51,6 +51,15 @@
>  
>  #define MAX17042_VMAX_TOLERANCE		50 /* 50 mV */
>  
> +#define MAX17042_CURRENT_LSB		1562500ll /* µV */

Is this really long long? The usage in max17042_get_status() is with int
operand and result.

> +#define MAX17042_CURRENT_RSENSE(x)	(x * MAX17042_CURRENT_LSB) /* µV */
> +#define MAX17042_CAPACITY_LSB		5000000ll /* µVh */
> +#define MAX17042_CAPACITY_RSENSE(x)	(x * MAX17042_CAPACITY_LSB) /* µVh */
> +#define MAX17042_TIME(x)		(x * 5625 / 1000) /* s */
> +#define MAX17042_VOLTAGE(x)		(x * 625 / 8) /* µV */
> +#define MAX17042_RESISTANCE(x)		(x / 4096) /* Ω */
> +#define MAX17042_TEMPERATURE(x)		(x / 256) /* °C */

Please enclose the "x" in (), in each macro


Best regards,
Krzysztof

Re: [PATCH 2/4] power: supply: max17042_battery: use ModelCfg refresh on max17055

[Krzysztof Kozlowski]

On 18/03/2022 01:10, Sebastian Krzyszkowiak wrote:
> Unlike other models, max17055 doesn't require cell characterization
> data and operates on smaller amount of input variables (DesignCap,
> VEmpty, IChgTerm and ModelCfg). Input data can already be filled in
> by max17042_override_por_values, however model refresh bit has to be
> set after adjusting input variables in order to make them apply.
> 
> Signed-off-by: Sebastian Krzyszkowiak <sebastian.krzyszkowiak@puri.sm>
> ---
>  drivers/power/supply/max17042_battery.c | 73 +++++++++++++++----------
>  include/linux/power/max17042_battery.h  |  3 +
>  2 files changed, 48 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/power/supply/max17042_battery.c b/drivers/power/supply/max17042_battery.c
> index c019d6c52363..c39250349a1d 100644
> --- a/drivers/power/supply/max17042_battery.c
> +++ b/drivers/power/supply/max17042_battery.c
> @@ -806,6 +806,13 @@ static inline void max17042_override_por_values(struct max17042_chip *chip)
>  	    (chip->chip_type == MAXIM_DEVICE_TYPE_MAX17055)) {
>  		max17042_override_por(map, MAX17047_V_empty, config->vempty);
>  	}
> +
> +	if (chip->chip_type == MAXIM_DEVICE_TYPE_MAX17055) {
> +		max17042_override_por(map, MAX17055_ModelCfg, config->model_cfg);
> +		// VChg is 1 by default, so allow it to be set to 0

Consistent comment, so /* */

I actually do not understand fully the comment and the code. You write
entire model_cfg to MAX17055_ModelCfg and then immediately do it again,
but with smaller mask. Why?

> +		regmap_update_bits(map, MAX17055_ModelCfg,
> +				MAX17055_MODELCFG_VCHG_BIT, config->model_cfg);

Can you align the continued line with previous line? Same in other
places if it is not aligned.

> +	}
>  }
>  
>  static int max17042_init_chip(struct max17042_chip *chip)
> @@ -814,44 +821,54 @@ static int max17042_init_chip(struct max17042_chip *chip)
>  	int ret;
>  


Best regards,
Krzysztof

Re: [PATCH 3/4] dt-bindings: power: supply: max17042: Add monitored-battery phandle

[Krzysztof Kozlowski]

On 18/03/2022 01:10, Sebastian Krzyszkowiak wrote:
> In order to let the driver know about the characteristics of the monitored
> battery, allow a standard "monitored-battery" property to be specified.
> 
> Cc: devicetree@vger.kernel.org
> Signed-off-by: Sebastian Krzyszkowiak <sebastian.krzyszkowiak@puri.sm>
> ---
>  .../devicetree/bindings/power/supply/maxim,max17042.yaml      | 4 ++++
>  1 file changed, 4 insertions(+)
> 


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


Best regards,
Krzysztof

Re: [PATCH 4/4] power: supply: max17042_battery: read battery properties from device tree

[Krzysztof Kozlowski]

On 18/03/2022 01:10, Sebastian Krzyszkowiak wrote:
> So far configuring the gauge was only possible using platform data,
> with no way to provide the configuration on device tree-based platforms.
> 
> Change that by looking up the configuration values from monitored-battery
> property. This is especially useful on models implementing ModelGauge m5 EZ
> algorithm, such as MAX17055, as all the required configuration can be
> derived from a "simple-battery" DT node there.
> 
> In order to be able to access power supply framework in get_of_pdata,
> move devm_power_supply_register earlier in max17042_probe.
> 
> Signed-off-by: Sebastian Krzyszkowiak <sebastian.krzyszkowiak@puri.sm>
> ---
>  drivers/power/supply/max17042_battery.c | 50 +++++++++++++++++++------
>  include/linux/power/max17042_battery.h  |  1 +
>  2 files changed, 40 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/power/supply/max17042_battery.c b/drivers/power/supply/max17042_battery.c
> index c39250349a1d..4c33565802d5 100644
> --- a/drivers/power/supply/max17042_battery.c
> +++ b/drivers/power/supply/max17042_battery.c
> @@ -937,7 +937,9 @@ max17042_get_of_pdata(struct max17042_chip *chip)
>  	struct device *dev = &chip->client->dev;
>  	struct device_node *np = dev->of_node;
>  	u32 prop;
> +	u64 data64;
>  	struct max17042_platform_data *pdata;
> +	struct power_supply_battery_info *info;
>  
>  	pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
>  	if (!pdata)
> @@ -961,6 +963,32 @@ max17042_get_of_pdata(struct max17042_chip *chip)
>  	if (of_property_read_s32(np, "maxim,over-volt", &pdata->vmax))
>  		pdata->vmax = INT_MAX;
>  
> +	if (pdata->enable_current_sense &&
> +	    power_supply_get_battery_info(chip->battery, &info) == 0) {
> +		pdata->config_data = devm_kzalloc(dev, sizeof(*pdata->config_data), GFP_KERNEL);
> +		if (!pdata->config_data)
> +			return NULL;
> +
> +		if (info->charge_full_design_uah != -EINVAL) {
> +			data64 = (u64)info->charge_full_design_uah * pdata->r_sns;
> +			do_div(data64, MAX17042_CAPACITY_LSB);
> +			pdata->config_data->design_cap = (u16)data64;
> +			pdata->enable_por_init = true;
> +		}
> +		if (info->charge_term_current_ua != -EINVAL) {
> +			data64 = (u64)info->charge_term_current_ua * pdata->r_sns;
> +			do_div(data64, MAX17042_CURRENT_LSB);
> +			pdata->config_data->ichgt_term = (u16)data64;
> +			pdata->enable_por_init = true;
> +		}
> +		if (chip->chip_type == MAXIM_DEVICE_TYPE_MAX17055) {
> +			if (info->voltage_max_design_uv > 4250000) {
> +				pdata->config_data->model_cfg = MAX17055_MODELCFG_VCHG_BIT;
> +				pdata->enable_por_init = true;
> +			}
> +		}
> +	}
> +
>  	return pdata;
>  }
>  #endif
> @@ -1092,16 +1120,23 @@ static int max17042_probe(struct i2c_client *client,
>  		return -EINVAL;
>  	}
>  
> +	i2c_set_clientdata(client, chip);
> +	psy_cfg.drv_data = chip;
> +	psy_cfg.of_node = dev->of_node;
> +
> +	chip->battery = devm_power_supply_register(&client->dev, max17042_desc,
> +						   &psy_cfg);
> +	if (IS_ERR(chip->battery)) {
> +		dev_err(&client->dev, "failed: power supply register\n");
> +		return PTR_ERR(chip->battery);
> +	}

I don't think it is correct. You register power supply, thus making it
available for system, before configuring most of the data. For short
time the chip might report to the system bogus results and events.

Instead I think you should split it into two parts - init which happens
before registering power supply and after.


Best regards,
Krzysztof

Re: [PATCH 1/4] power: supply: max17042_battery: Add unit conversion macros

[Hans de Goede]

Hi,

On 3/18/22 01:10, Sebastian Krzyszkowiak wrote:
> Instead of sprinkling the code with magic numbers, put the unit
> definitions used by the gauge into a set of macros. Macros are
> used instead of simple defines in order to not require floating
> point operations for divisions.
> 
> Signed-off-by: Sebastian Krzyszkowiak <sebastian.krzyszkowiak@puri.sm>
> ---
>  drivers/power/supply/max17042_battery.c | 40 +++++++++++++++----------
>  1 file changed, 24 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/power/supply/max17042_battery.c b/drivers/power/supply/max17042_battery.c
> index ab031bbfbe78..c019d6c52363 100644
> --- a/drivers/power/supply/max17042_battery.c
> +++ b/drivers/power/supply/max17042_battery.c
> @@ -51,6 +51,15 @@
>  
>  #define MAX17042_VMAX_TOLERANCE		50 /* 50 mV */
>  
> +#define MAX17042_CURRENT_LSB		1562500ll /* µV */
> +#define MAX17042_CURRENT_RSENSE(x)	(x * MAX17042_CURRENT_LSB) /* µV */
> +#define MAX17042_CAPACITY_LSB		5000000ll /* µVh */
> +#define MAX17042_CAPACITY_RSENSE(x)	(x * MAX17042_CAPACITY_LSB) /* µVh */
> +#define MAX17042_TIME(x)		(x * 5625 / 1000) /* s */
> +#define MAX17042_VOLTAGE(x)		(x * 625 / 8) /* µV */
> +#define MAX17042_RESISTANCE(x)		(x / 4096) /* Ω */
> +#define MAX17042_TEMPERATURE(x)		(x / 256) /* °C */
> +
>  struct max17042_chip {
>  	struct i2c_client *client;
>  	struct regmap *regmap;
> @@ -103,8 +112,7 @@ static int max17042_get_temperature(struct max17042_chip *chip, int *temp)
>  
>  	*temp = sign_extend32(data, 15);
>  	/* The value is converted into deci-centigrade scale */
> -	/* Units of LSB = 1 / 256 degree Celsius */
> -	*temp = *temp * 10 / 256;
> +	*temp = MAX17042_TEMPERATURE(*temp * 10);

Shouldn't the "* 10"  be part of the macro?

Otherwise this looks good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans


>  	return 0;
>  }
>  
> @@ -161,7 +169,7 @@ static int max17042_get_status(struct max17042_chip *chip, int *status)
>  		return ret;
>  
>  	avg_current = sign_extend32(data, 15);
> -	avg_current *= 1562500 / chip->pdata->r_sns;
> +	avg_current *= MAX17042_CURRENT_LSB / chip->pdata->r_sns;
>  
>  	if (avg_current > 0)
>  		*status = POWER_SUPPLY_STATUS_CHARGING;
> @@ -181,7 +189,7 @@ static int max17042_get_battery_health(struct max17042_chip *chip, int *health)
>  		goto health_error;
>  
>  	/* bits [0-3] unused */
> -	vavg = val * 625 / 8;
> +	vavg = MAX17042_VOLTAGE(val);
>  	/* Convert to millivolts */
>  	vavg /= 1000;
>  
> @@ -190,7 +198,7 @@ static int max17042_get_battery_health(struct max17042_chip *chip, int *health)
>  		goto health_error;
>  
>  	/* bits [0-3] unused */
> -	vbatt = val * 625 / 8;
> +	vbatt = MAX17042_VOLTAGE(val);
>  	/* Convert to millivolts */
>  	vbatt /= 1000;
>  
> @@ -297,21 +305,21 @@ static int max17042_get_property(struct power_supply *psy,
>  		if (ret < 0)
>  			return ret;
>  
> -		val->intval = data * 625 / 8;
> +		val->intval = MAX17042_VOLTAGE(data);
>  		break;
>  	case POWER_SUPPLY_PROP_VOLTAGE_AVG:
>  		ret = regmap_read(map, MAX17042_AvgVCELL, &data);
>  		if (ret < 0)
>  			return ret;
>  
> -		val->intval = data * 625 / 8;
> +		val->intval = MAX17042_VOLTAGE(data);
>  		break;
>  	case POWER_SUPPLY_PROP_VOLTAGE_OCV:
>  		ret = regmap_read(map, MAX17042_OCVInternal, &data);
>  		if (ret < 0)
>  			return ret;
>  
> -		val->intval = data * 625 / 8;
> +		val->intval = MAX17042_VOLTAGE(data);
>  		break;
>  	case POWER_SUPPLY_PROP_CAPACITY:
>  		if (chip->pdata->enable_current_sense)
> @@ -328,7 +336,7 @@ static int max17042_get_property(struct power_supply *psy,
>  		if (ret < 0)
>  			return ret;
>  
> -		data64 = data * 5000000ll;
> +		data64 = MAX17042_CAPACITY_RSENSE(data);
>  		do_div(data64, chip->pdata->r_sns);
>  		val->intval = data64;
>  		break;
> @@ -337,7 +345,7 @@ static int max17042_get_property(struct power_supply *psy,
>  		if (ret < 0)
>  			return ret;
>  
> -		data64 = data * 5000000ll;
> +		data64 = MAX17042_CAPACITY_RSENSE(data);
>  		do_div(data64, chip->pdata->r_sns);
>  		val->intval = data64;
>  		break;
> @@ -346,7 +354,7 @@ static int max17042_get_property(struct power_supply *psy,
>  		if (ret < 0)
>  			return ret;
>  
> -		data64 = data * 5000000ll;
> +		data64 = MAX17042_CAPACITY_RSENSE(data);
>  		do_div(data64, chip->pdata->r_sns);
>  		val->intval = data64;
>  		break;
> @@ -355,7 +363,7 @@ static int max17042_get_property(struct power_supply *psy,
>  		if (ret < 0)
>  			return ret;
>  
> -		data64 = sign_extend64(data, 15) * 5000000ll;
> +		data64 = MAX17042_CAPACITY_RSENSE(sign_extend64(data, 15));
>  		val->intval = div_s64(data64, chip->pdata->r_sns);
>  		break;
>  	case POWER_SUPPLY_PROP_TEMP:
> @@ -397,7 +405,7 @@ static int max17042_get_property(struct power_supply *psy,
>  			if (ret < 0)
>  				return ret;
>  
> -			data64 = sign_extend64(data, 15) * 1562500ll;
> +			data64 = MAX17042_CURRENT_RSENSE(sign_extend64(data, 15));
>  			val->intval = div_s64(data64, chip->pdata->r_sns);
>  		} else {
>  			return -EINVAL;
> @@ -409,7 +417,7 @@ static int max17042_get_property(struct power_supply *psy,
>  			if (ret < 0)
>  				return ret;
>  
> -			data64 = sign_extend64(data, 15) * 1562500ll;
> +			data64 = MAX17042_CURRENT_RSENSE(sign_extend64(data, 15));
>  			val->intval = div_s64(data64, chip->pdata->r_sns);
>  		} else {
>  			return -EINVAL;
> @@ -420,7 +428,7 @@ static int max17042_get_property(struct power_supply *psy,
>  		if (ret < 0)
>  			return ret;
>  
> -		data64 = data * 1562500ll;
> +		data64 = MAX17042_CURRENT_RSENSE(data);
>  		val->intval = div_s64(data64, chip->pdata->r_sns);
>  		break;
>  	case POWER_SUPPLY_PROP_TIME_TO_EMPTY_NOW:
> @@ -428,7 +436,7 @@ static int max17042_get_property(struct power_supply *psy,
>  		if (ret < 0)
>  			return ret;
>  
> -		val->intval = data * 5625 / 1000;
> +		val->intval = MAX17042_TIME(data);
>  		break;
>  	default:
>  		return -EINVAL;

Re: [PATCH 1/4] power: supply: max17042_battery: Add unit conversion macros

[Hans de Goede]

Hi,

On 3/18/22 09:16, Krzysztof Kozlowski wrote:
> On 18/03/2022 01:10, Sebastian Krzyszkowiak wrote:
>> Instead of sprinkling the code with magic numbers, put the unit
>> definitions used by the gauge into a set of macros. Macros are
>> used instead of simple defines in order to not require floating
>> point operations for divisions.
>>
>> Signed-off-by: Sebastian Krzyszkowiak <sebastian.krzyszkowiak@puri.sm>
>> ---
>>  drivers/power/supply/max17042_battery.c | 40 +++++++++++++++----------
>>  1 file changed, 24 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/power/supply/max17042_battery.c b/drivers/power/supply/max17042_battery.c
>> index ab031bbfbe78..c019d6c52363 100644
>> --- a/drivers/power/supply/max17042_battery.c
>> +++ b/drivers/power/supply/max17042_battery.c
>> @@ -51,6 +51,15 @@
>>  
>>  #define MAX17042_VMAX_TOLERANCE		50 /* 50 mV */
>>  
>> +#define MAX17042_CURRENT_LSB		1562500ll /* µV */
> 
> Is this really long long? The usage in max17042_get_status() is with int
> operand and result.

The "ll" is part of the original code which these macros replace,
dropping the "ll" is IMHO out of scope for this patch, it would
clearly break the only change 1 thing per patch/commit rule.

>> +#define MAX17042_CURRENT_RSENSE(x)	(x * MAX17042_CURRENT_LSB) /* µV */
>> +#define MAX17042_CAPACITY_LSB		5000000ll /* µVh */
>> +#define MAX17042_CAPACITY_RSENSE(x)	(x * MAX17042_CAPACITY_LSB) /* µVh */
>> +#define MAX17042_TIME(x)		(x * 5625 / 1000) /* s */
>> +#define MAX17042_VOLTAGE(x)		(x * 625 / 8) /* µV */
>> +#define MAX17042_RESISTANCE(x)		(x / 4096) /* Ω */
>> +#define MAX17042_TEMPERATURE(x)		(x / 256) /* °C */
> 
> Please enclose the "x" in (), in each macro

Ack, right I should have spotted that in my own review.

Regards,

Hans

Re: [PATCH 2/4] power: supply: max17042_battery: use ModelCfg refresh on max17055

[Hans de Goede]

Hi,

On 3/18/22 01:10, Sebastian Krzyszkowiak wrote:
> Unlike other models, max17055 doesn't require cell characterization
> data and operates on smaller amount of input variables (DesignCap,
> VEmpty, IChgTerm and ModelCfg). Input data can already be filled in
> by max17042_override_por_values, however model refresh bit has to be
> set after adjusting input variables in order to make them apply.
> 
> Signed-off-by: Sebastian Krzyszkowiak <sebastian.krzyszkowiak@puri.sm>
> ---
>  drivers/power/supply/max17042_battery.c | 73 +++++++++++++++----------
>  include/linux/power/max17042_battery.h  |  3 +
>  2 files changed, 48 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/power/supply/max17042_battery.c b/drivers/power/supply/max17042_battery.c
> index c019d6c52363..c39250349a1d 100644
> --- a/drivers/power/supply/max17042_battery.c
> +++ b/drivers/power/supply/max17042_battery.c
> @@ -806,6 +806,13 @@ static inline void max17042_override_por_values(struct max17042_chip *chip)
>  	    (chip->chip_type == MAXIM_DEVICE_TYPE_MAX17055)) {
>  		max17042_override_por(map, MAX17047_V_empty, config->vempty);
>  	}
> +
> +	if (chip->chip_type == MAXIM_DEVICE_TYPE_MAX17055) {
> +		max17042_override_por(map, MAX17055_ModelCfg, config->model_cfg);
> +		// VChg is 1 by default, so allow it to be set to 0
> +		regmap_update_bits(map, MAX17055_ModelCfg,
> +				MAX17055_MODELCFG_VCHG_BIT, config->model_cfg);
> +	}
>  }
>  
>  static int max17042_init_chip(struct max17042_chip *chip)
> @@ -814,44 +821,54 @@ static int max17042_init_chip(struct max17042_chip *chip)
>  	int ret;
>  
>  	max17042_override_por_values(chip);
> +
> +	if (chip->chip_type == MAXIM_DEVICE_TYPE_MAX17055) {
> +		regmap_write_bits(map, MAX17055_ModelCfg,
> +				  MAX17055_MODELCFG_REFRESH_BIT, MAX17055_MODELCFG_REFRESH_BIT);
> +	}
> +

This can be folded into the if (chip->chip_type == MAXIM_DEVICE_TYPE_MAX17055) {}
which you add to max17042_override_por_values() above.


>  	/* After Power up, the MAX17042 requires 500mS in order
>  	 * to perform signal debouncing and initial SOC reporting
>  	 */
>  	msleep(500);
>  
> -	/* Initialize configuration */
> -	max17042_write_config_regs(chip);
> -
> -	/* write cell characterization data */
> -	ret = max17042_init_model(chip);
> -	if (ret) {
> -		dev_err(&chip->client->dev, "%s init failed\n",
> -			__func__);
> -		return -EIO;
> -	}
> +	if ((chip->chip_type == MAXIM_DEVICE_TYPE_MAX17042) ||
> +	    (chip->chip_type == MAXIM_DEVICE_TYPE_MAX17047) ||
> +	    (chip->chip_type == MAXIM_DEVICE_TYPE_MAX17050)) {
> +		/* Initialize configuration */
> +		max17042_write_config_regs(chip);
> +
> +		/* write cell characterization data */
> +		ret = max17042_init_model(chip);
> +		if (ret) {
> +			dev_err(&chip->client->dev, "%s init failed\n",
> +				__func__);
> +			return -EIO;
> +		}
>  
> -	ret = max17042_verify_model_lock(chip);
> -	if (ret) {
> -		dev_err(&chip->client->dev, "%s lock verify failed\n",
> -			__func__);
> -		return -EIO;
> -	}
> -	/* write custom parameters */
> -	max17042_write_custom_regs(chip);
> +		ret = max17042_verify_model_lock(chip);
> +		if (ret) {
> +			dev_err(&chip->client->dev, "%s lock verify failed\n",
> +				__func__);
> +			return -EIO;
> +		}
> +		/* write custom parameters */
> +		max17042_write_custom_regs(chip);
>  
> -	/* update capacity params */
> -	max17042_update_capacity_regs(chip);
> +		/* update capacity params */
> +		max17042_update_capacity_regs(chip);
>  
> -	/* delay must be atleast 350mS to allow VFSOC
> -	 * to be calculated from the new configuration
> -	 */
> -	msleep(350);
> +		/* delay must be at least 350mS to allow VFSOC
> +		 * to be calculated from the new configuration
> +		 */
> +		msleep(350);
>  
> -	/* reset vfsoc0 reg */
> -	max17042_reset_vfsoc0_reg(chip);
> +		/* reset vfsoc0 reg */
> +		max17042_reset_vfsoc0_reg(chip);
>  
> -	/* load new capacity params */
> -	max17042_load_new_capacity_params(chip);
> +		/* load new capacity params */
> +		max17042_load_new_capacity_params(chip);
> +	}
>  
>  	/* Init complete, Clear the POR bit */
>  	regmap_update_bits(map, MAX17042_STATUS, STATUS_POR_BIT, 0x0);
> diff --git a/include/linux/power/max17042_battery.h b/include/linux/power/max17042_battery.h
> index c417abd2ab70..6943921cab5e 100644
> --- a/include/linux/power/max17042_battery.h
> +++ b/include/linux/power/max17042_battery.h
> @@ -23,6 +23,8 @@
>  
>  #define MAX17042_CHARACTERIZATION_DATA_SIZE 48
>  
> +#define MAX17055_MODELCFG_REFRESH_BIT	BIT(15)
> +
>  enum max17042_register {
>  	MAX17042_STATUS		= 0x00,
>  	MAX17042_VALRT_Th	= 0x01,
> @@ -208,6 +210,7 @@ struct max17042_config_data {
>  	u16	full_soc_thresh;	/* 0x13 */
>  	u16	design_cap;	/* 0x18 */
>  	u16	ichgt_term;	/* 0x1E */
> +	u16	model_cfg;	/* 0xDB */
>  
>  	/* MG3 config */
>  	u16	at_rate;	/* 0x04 */

Regards,

Hans

Re: [PATCH 1/4] power: supply: max17042_battery: Add unit conversion macros

[Krzysztof Kozlowski]

On 18/03/2022 10:00, Hans de Goede wrote:
> Hi,
> 
> On 3/18/22 09:16, Krzysztof Kozlowski wrote:
>> On 18/03/2022 01:10, Sebastian Krzyszkowiak wrote:
>>> Instead of sprinkling the code with magic numbers, put the unit
>>> definitions used by the gauge into a set of macros. Macros are
>>> used instead of simple defines in order to not require floating
>>> point operations for divisions.
>>>
>>> Signed-off-by: Sebastian Krzyszkowiak <sebastian.krzyszkowiak@puri.sm>
>>> ---
>>>  drivers/power/supply/max17042_battery.c | 40 +++++++++++++++----------
>>>  1 file changed, 24 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/drivers/power/supply/max17042_battery.c b/drivers/power/supply/max17042_battery.c
>>> index ab031bbfbe78..c019d6c52363 100644
>>> --- a/drivers/power/supply/max17042_battery.c
>>> +++ b/drivers/power/supply/max17042_battery.c
>>> @@ -51,6 +51,15 @@
>>>  
>>>  #define MAX17042_VMAX_TOLERANCE		50 /* 50 mV */
>>>  
>>> +#define MAX17042_CURRENT_LSB		1562500ll /* µV */
>>
>> Is this really long long? The usage in max17042_get_status() is with int
>> operand and result.
> 
> The "ll" is part of the original code which these macros replace,
> dropping the "ll" is IMHO out of scope for this patch, it would
> clearly break the only change 1 thing per patch/commit rule.

Not in max17042_get_status(). The usage there is without ll. Three other
places use it in 64-bit context (result is 64-bit), so there indeed. But
in max17042_get_status() this is now different.


Best regards,
Krzysztof

Re: [PATCH 1/4] power: supply: max17042_battery: Add unit conversion macros

[Hans de Goede]

Hi,

On 3/18/22 10:06, Krzysztof Kozlowski wrote:
> On 18/03/2022 10:00, Hans de Goede wrote:
>> Hi,
>>
>> On 3/18/22 09:16, Krzysztof Kozlowski wrote:
>>> On 18/03/2022 01:10, Sebastian Krzyszkowiak wrote:
>>>> Instead of sprinkling the code with magic numbers, put the unit
>>>> definitions used by the gauge into a set of macros. Macros are
>>>> used instead of simple defines in order to not require floating
>>>> point operations for divisions.
>>>>
>>>> Signed-off-by: Sebastian Krzyszkowiak <sebastian.krzyszkowiak@puri.sm>
>>>> ---
>>>>  drivers/power/supply/max17042_battery.c | 40 +++++++++++++++----------
>>>>  1 file changed, 24 insertions(+), 16 deletions(-)
>>>>
>>>> diff --git a/drivers/power/supply/max17042_battery.c b/drivers/power/supply/max17042_battery.c
>>>> index ab031bbfbe78..c019d6c52363 100644
>>>> --- a/drivers/power/supply/max17042_battery.c
>>>> +++ b/drivers/power/supply/max17042_battery.c
>>>> @@ -51,6 +51,15 @@
>>>>  
>>>>  #define MAX17042_VMAX_TOLERANCE		50 /* 50 mV */
>>>>  
>>>> +#define MAX17042_CURRENT_LSB		1562500ll /* µV */
>>>
>>> Is this really long long? The usage in max17042_get_status() is with int
>>> operand and result.
>>
>> The "ll" is part of the original code which these macros replace,
>> dropping the "ll" is IMHO out of scope for this patch, it would
>> clearly break the only change 1 thing per patch/commit rule.
> 
> Not in max17042_get_status(). The usage there is without ll. Three other
> places use it in 64-bit context (result is 64-bit), so there indeed. But
> in max17042_get_status() this is now different.

Ah, good catch and there is a reason why it is not a ll there, a divide
is done on it, which is now a 64 bit divide which will break on 32 bit
builds...

Note that e.g. this existing block:

        case POWER_SUPPLY_PROP_CURRENT_NOW:
                if (chip->pdata->enable_current_sense) {
                        ret = regmap_read(map, MAX17042_Current, &data);
                        if (ret < 0)
                                return ret;

                        data64 = sign_extend64(data, 15) * 1562500ll;
                        val->intval = div_s64(data64, chip->pdata->r_sns);
                } else {
                        return -EINVAL;
                }
                break;

Solves this by using the div_s64 helper. So the code in max17042_get_status()
needs to be fixed to do the same.

The "ll" is necessary because 32768 * 1562500 = 51200000000 which does not
fit in a 32 bit integer.

So fixing max17042_get_status() to use sign_extend64 + div_s64 fixes
a potential bug there and as such that really should be done in
a separate preparation patch with a Cc stable.

Regards,

Hans

Re: [PATCH 2/4] power: supply: max17042_battery: use ModelCfg refresh on max17055

[Sebastian Krzyszkowiak]

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

On piątek, 18 marca 2022 09:22:16 CET Krzysztof Kozlowski wrote:
> On 18/03/2022 01:10, Sebastian Krzyszkowiak wrote:
> > Unlike other models, max17055 doesn't require cell characterization
> > data and operates on smaller amount of input variables (DesignCap,
> > VEmpty, IChgTerm and ModelCfg). Input data can already be filled in
> > by max17042_override_por_values, however model refresh bit has to be
> > set after adjusting input variables in order to make them apply.
> > 
> > Signed-off-by: Sebastian Krzyszkowiak <sebastian.krzyszkowiak@puri.sm>
> > ---
> > 
> >  drivers/power/supply/max17042_battery.c | 73 +++++++++++++++----------
> >  include/linux/power/max17042_battery.h  |  3 +
> >  2 files changed, 48 insertions(+), 28 deletions(-)
> > 
> > diff --git a/drivers/power/supply/max17042_battery.c
> > b/drivers/power/supply/max17042_battery.c index
> > c019d6c52363..c39250349a1d 100644
> > --- a/drivers/power/supply/max17042_battery.c
> > +++ b/drivers/power/supply/max17042_battery.c
> > @@ -806,6 +806,13 @@ static inline void
> > max17042_override_por_values(struct max17042_chip *chip)> 
> >  	    (chip->chip_type == MAXIM_DEVICE_TYPE_MAX17055)) {
> >  		
> >  		max17042_override_por(map, MAX17047_V_empty, config-
>vempty);
> >  	
> >  	}
> > 
> > +
> > +	if (chip->chip_type == MAXIM_DEVICE_TYPE_MAX17055) {
> > +		max17042_override_por(map, MAX17055_ModelCfg, config-
>model_cfg);
> > +		// VChg is 1 by default, so allow it to be set to 0
> 
> Consistent comment, so /* */
> 
> I actually do not understand fully the comment and the code. You write
> entire model_cfg to MAX17055_ModelCfg and then immediately do it again,
> but with smaller mask. Why?

That's because VChg is 1 on POR, and max17042_override_por doesn't do anything 
when value equals 0 - which means that if the whole config->model_cfg is 0, 
VChg won't get unset (which is needed for 4.2V batteries).

This could actually be replaced with a single regmap_write.

> > +		regmap_update_bits(map, MAX17055_ModelCfg,
> > +				MAX17055_MODELCFG_VCHG_BIT, 
config->model_cfg);
> 
> Can you align the continued line with previous line? Same in other
> places if it is not aligned.
> 
> > +	}
> > 
> >  }
> >  
> >  static int max17042_init_chip(struct max17042_chip *chip)
> > 
> > @@ -814,44 +821,54 @@ static int max17042_init_chip(struct max17042_chip
> > *chip)> 
> >  	int ret;
> 
> Best regards,
> Krzysztof


[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/4] power: supply: max17042_battery: Add unit conversion macros

[Sebastian Krzyszkowiak]

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

Hi Krzysztof, hi Hans,

thanks for the review!

On piątek, 18 marca 2022 10:51:26 CET Hans de Goede wrote:
> Hi,
> 
> On 3/18/22 10:06, Krzysztof Kozlowski wrote:
> > On 18/03/2022 10:00, Hans de Goede wrote:
> >> Hi,
> >> 
> >> On 3/18/22 09:16, Krzysztof Kozlowski wrote:
> >>> On 18/03/2022 01:10, Sebastian Krzyszkowiak wrote:
> >>>> Instead of sprinkling the code with magic numbers, put the unit
> >>>> definitions used by the gauge into a set of macros. Macros are
> >>>> used instead of simple defines in order to not require floating
> >>>> point operations for divisions.
> >>>> 
> >>>> Signed-off-by: Sebastian Krzyszkowiak <sebastian.krzyszkowiak@puri.sm>
> >>>> ---
> >>>> 
> >>>>  drivers/power/supply/max17042_battery.c | 40 +++++++++++++++----------
> >>>>  1 file changed, 24 insertions(+), 16 deletions(-)
> >>>> 
> >>>> diff --git a/drivers/power/supply/max17042_battery.c
> >>>> b/drivers/power/supply/max17042_battery.c index
> >>>> ab031bbfbe78..c019d6c52363 100644
> >>>> --- a/drivers/power/supply/max17042_battery.c
> >>>> +++ b/drivers/power/supply/max17042_battery.c
> >>>> @@ -51,6 +51,15 @@
> >>>> 
> >>>>  #define MAX17042_VMAX_TOLERANCE		50 /* 50 mV */
> >>>> 
> >>>> +#define MAX17042_CURRENT_LSB		1562500ll /* µV */
> >>> 
> >>> Is this really long long? The usage in max17042_get_status() is with int
> >>> operand and result.
> >> 
> >> The "ll" is part of the original code which these macros replace,
> >> dropping the "ll" is IMHO out of scope for this patch, it would
> >> clearly break the only change 1 thing per patch/commit rule.
> > 
> > Not in max17042_get_status(). The usage there is without ll. Three other
> > places use it in 64-bit context (result is 64-bit), so there indeed. But
> > in max17042_get_status() this is now different.
> 
> Ah, good catch and there is a reason why it is not a ll there, a divide
> is done on it, which is now a 64 bit divide which will break on 32 bit
> builds...
> 
> Note that e.g. this existing block:
> 
>         case POWER_SUPPLY_PROP_CURRENT_NOW:
>                 if (chip->pdata->enable_current_sense) {
>                         ret = regmap_read(map, MAX17042_Current, &data);
>                         if (ret < 0)
>                                 return ret;
> 
>                         data64 = sign_extend64(data, 15) * 1562500ll;
>                         val->intval = div_s64(data64, chip->pdata->r_sns);
>                 } else {
>                         return -EINVAL;
>                 }
>                 break;
> 
> Solves this by using the div_s64 helper. So the code in
> max17042_get_status() needs to be fixed to do the same.
> 
> The "ll" is necessary because 32768 * 1562500 = 51200000000 which does not
> fit in a 32 bit integer.
> 
> So fixing max17042_get_status() to use sign_extend64 + div_s64 fixes
> a potential bug there and as such that really should be done in
> a separate preparation patch with a Cc stable.
> 
> Regards,
> 
> Hans

Yes, I've already noticed that max17042_get_status was broken, but it managed 
to slip out of my mind before sending the series - although I haven't caught 
that I'm introducing a yet another breakage there :) I've actually thought 
about removing the unit conversion from this place whatsoever, because this 
function only ever cares about the sign of what's in MAX17042_Current, so it 
doesn't really need to do any division at all.

Best regards,
Sebastian

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/4] power: supply: max17042_battery: Add unit conversion macros

[Hans de Goede]

Hi,

On 3/18/22 21:06, Sebastian Krzyszkowiak wrote:
> Hi Krzysztof, hi Hans,
> 
> thanks for the review!
> 
> On piątek, 18 marca 2022 10:51:26 CET Hans de Goede wrote:
>> Hi,
>>
>> On 3/18/22 10:06, Krzysztof Kozlowski wrote:
>>> On 18/03/2022 10:00, Hans de Goede wrote:
>>>> Hi,
>>>>
>>>> On 3/18/22 09:16, Krzysztof Kozlowski wrote:
>>>>> On 18/03/2022 01:10, Sebastian Krzyszkowiak wrote:
>>>>>> Instead of sprinkling the code with magic numbers, put the unit
>>>>>> definitions used by the gauge into a set of macros. Macros are
>>>>>> used instead of simple defines in order to not require floating
>>>>>> point operations for divisions.
>>>>>>
>>>>>> Signed-off-by: Sebastian Krzyszkowiak <sebastian.krzyszkowiak@puri.sm>
>>>>>> ---
>>>>>>
>>>>>>  drivers/power/supply/max17042_battery.c | 40 +++++++++++++++----------
>>>>>>  1 file changed, 24 insertions(+), 16 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/power/supply/max17042_battery.c
>>>>>> b/drivers/power/supply/max17042_battery.c index
>>>>>> ab031bbfbe78..c019d6c52363 100644
>>>>>> --- a/drivers/power/supply/max17042_battery.c
>>>>>> +++ b/drivers/power/supply/max17042_battery.c
>>>>>> @@ -51,6 +51,15 @@
>>>>>>
>>>>>>  #define MAX17042_VMAX_TOLERANCE		50 /* 50 mV */
>>>>>>
>>>>>> +#define MAX17042_CURRENT_LSB		1562500ll /* µV */
>>>>>
>>>>> Is this really long long? The usage in max17042_get_status() is with int
>>>>> operand and result.
>>>>
>>>> The "ll" is part of the original code which these macros replace,
>>>> dropping the "ll" is IMHO out of scope for this patch, it would
>>>> clearly break the only change 1 thing per patch/commit rule.
>>>
>>> Not in max17042_get_status(). The usage there is without ll. Three other
>>> places use it in 64-bit context (result is 64-bit), so there indeed. But
>>> in max17042_get_status() this is now different.
>>
>> Ah, good catch and there is a reason why it is not a ll there, a divide
>> is done on it, which is now a 64 bit divide which will break on 32 bit
>> builds...
>>
>> Note that e.g. this existing block:
>>
>>         case POWER_SUPPLY_PROP_CURRENT_NOW:
>>                 if (chip->pdata->enable_current_sense) {
>>                         ret = regmap_read(map, MAX17042_Current, &data);
>>                         if (ret < 0)
>>                                 return ret;
>>
>>                         data64 = sign_extend64(data, 15) * 1562500ll;
>>                         val->intval = div_s64(data64, chip->pdata->r_sns);
>>                 } else {
>>                         return -EINVAL;
>>                 }
>>                 break;
>>
>> Solves this by using the div_s64 helper. So the code in
>> max17042_get_status() needs to be fixed to do the same.
>>
>> The "ll" is necessary because 32768 * 1562500 = 51200000000 which does not
>> fit in a 32 bit integer.
>>
>> So fixing max17042_get_status() to use sign_extend64 + div_s64 fixes
>> a potential bug there and as such that really should be done in
>> a separate preparation patch with a Cc stable.
>>
>> Regards,
>>
>> Hans
> 
> Yes, I've already noticed that max17042_get_status was broken, but it managed 
> to slip out of my mind before sending the series - although I haven't caught 
> that I'm introducing a yet another breakage there :) I've actually thought 
> about removing the unit conversion from this place whatsoever, because this 
> function only ever cares about the sign of what's in MAX17042_Current, so it 
> doesn't really need to do any division at all.

Removing the value conversion (in a separate patch) would be a good
solution too.

Regards,

Hans

Re: [PATCH 2/4] power: supply: max17042_battery: use ModelCfg refresh on max17055

[Krzysztof Kozlowski]

On 18/03/2022 20:58, Sebastian Krzyszkowiak wrote:
> On piątek, 18 marca 2022 09:22:16 CET Krzysztof Kozlowski wrote:
>> On 18/03/2022 01:10, Sebastian Krzyszkowiak wrote:
>>> Unlike other models, max17055 doesn't require cell characterization
>>> data and operates on smaller amount of input variables (DesignCap,
>>> VEmpty, IChgTerm and ModelCfg). Input data can already be filled in
>>> by max17042_override_por_values, however model refresh bit has to be
>>> set after adjusting input variables in order to make them apply.
>>>
>>> Signed-off-by: Sebastian Krzyszkowiak <sebastian.krzyszkowiak@puri.sm>
>>> ---
>>>
>>>  drivers/power/supply/max17042_battery.c | 73 +++++++++++++++----------
>>>  include/linux/power/max17042_battery.h  |  3 +
>>>  2 files changed, 48 insertions(+), 28 deletions(-)
>>>
>>> diff --git a/drivers/power/supply/max17042_battery.c
>>> b/drivers/power/supply/max17042_battery.c index
>>> c019d6c52363..c39250349a1d 100644
>>> --- a/drivers/power/supply/max17042_battery.c
>>> +++ b/drivers/power/supply/max17042_battery.c
>>> @@ -806,6 +806,13 @@ static inline void
>>> max17042_override_por_values(struct max17042_chip *chip)> 
>>>  	    (chip->chip_type == MAXIM_DEVICE_TYPE_MAX17055)) {
>>>  		
>>>  		max17042_override_por(map, MAX17047_V_empty, config-
>> vempty);
>>>  	
>>>  	}
>>>
>>> +
>>> +	if (chip->chip_type == MAXIM_DEVICE_TYPE_MAX17055) {
>>> +		max17042_override_por(map, MAX17055_ModelCfg, config-
>> model_cfg);
>>> +		// VChg is 1 by default, so allow it to be set to 0
>>
>> Consistent comment, so /* */
>>
>> I actually do not understand fully the comment and the code. You write
>> entire model_cfg to MAX17055_ModelCfg and then immediately do it again,
>> but with smaller mask. Why?
> 
> That's because VChg is 1 on POR, and max17042_override_por doesn't do anything 
> when value equals 0 - which means that if the whole config->model_cfg is 0, 
> VChg won't get unset (which is needed for 4.2V batteries).
> 
> This could actually be replaced with a single regmap_write.
> 

I got it now. But if config->model_cfg is 0, should VChg be unset?


Best regards,
Krzysztof

Re: [PATCH 2/4] power: supply: max17042_battery: use ModelCfg refresh on max17055

[Sebastian Krzyszkowiak]

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

On niedziela, 20 marca 2022 13:18:49 CET Krzysztof Kozlowski wrote:
> On 18/03/2022 20:58, Sebastian Krzyszkowiak wrote:
> > On piątek, 18 marca 2022 09:22:16 CET Krzysztof Kozlowski wrote:
> >> On 18/03/2022 01:10, Sebastian Krzyszkowiak wrote:
> >>> Unlike other models, max17055 doesn't require cell characterization
> >>> data and operates on smaller amount of input variables (DesignCap,
> >>> VEmpty, IChgTerm and ModelCfg). Input data can already be filled in
> >>> by max17042_override_por_values, however model refresh bit has to be
> >>> set after adjusting input variables in order to make them apply.
> >>> 
> >>> Signed-off-by: Sebastian Krzyszkowiak <sebastian.krzyszkowiak@puri.sm>
> >>> ---
> >>> 
> >>>  drivers/power/supply/max17042_battery.c | 73 +++++++++++++++----------
> >>>  include/linux/power/max17042_battery.h  |  3 +
> >>>  2 files changed, 48 insertions(+), 28 deletions(-)
> >>> 
> >>> diff --git a/drivers/power/supply/max17042_battery.c
> >>> b/drivers/power/supply/max17042_battery.c index
> >>> c019d6c52363..c39250349a1d 100644
> >>> --- a/drivers/power/supply/max17042_battery.c
> >>> +++ b/drivers/power/supply/max17042_battery.c
> >>> @@ -806,6 +806,13 @@ static inline void
> >>> max17042_override_por_values(struct max17042_chip *chip)>
> >>> 
> >>>  	    (chip->chip_type == MAXIM_DEVICE_TYPE_MAX17055)) {
> >>>  		
> >>>  		max17042_override_por(map, MAX17047_V_empty, config-
> >> 
> >> vempty);
> >> 
> >>>  	}
> >>> 
> >>> +
> >>> +	if (chip->chip_type == MAXIM_DEVICE_TYPE_MAX17055) {
> >>> +		max17042_override_por(map, MAX17055_ModelCfg, config-
> >> 
> >> model_cfg);
> >> 
> >>> +		// VChg is 1 by default, so allow it to be set to 0
> >> 
> >> Consistent comment, so /* */
> >> 
> >> I actually do not understand fully the comment and the code. You write
> >> entire model_cfg to MAX17055_ModelCfg and then immediately do it again,
> >> but with smaller mask. Why?
> > 
> > That's because VChg is 1 on POR, and max17042_override_por doesn't do
> > anything when value equals 0 - which means that if the whole
> > config->model_cfg is 0, VChg won't get unset (which is needed for 4.2V
> > batteries).
> > 
> > This could actually be replaced with a single regmap_write.
> 
> I got it now. But if config->model_cfg is 0, should VChg be unset?

That's a good question.

max17042_override_por doesn't override the register value when the given value 
equals zero in order to not override POR defaults with unset platform data. 
This way one can set only the registers that they want to change in `config` 
and the rest are untouched. This, however, only works if we assume that zero 
means "don't touch", which isn't the case for ModelCfg.

On the Librem 5, we need to unset VChg bit because our battery is only being 
charged up to 4.2V. Allowing to unset this bit only without having to touch 
the rest of the register was the motivation behind the current version of this 
patch, however, thinking about it now I can see that it fails to do that in 
the opposite case - when the DT contains a simple-battery with maximum voltage 
higher than 4.25V, VChg will be set in config->model_cfg causing the whole 
register to be overwritten.

So, I see two possible solutions:

1) move VChg handling to a separate variable in struct max17042_config_data. 
This way model_cfg can stay zero when there's no need to touch the rest of the 
register. This minimizes changes over current code.

2) remove max17042_override_por_values in its current shape altogether and 
make it only deal with the values that are actually being set by the driver 
(and only extend it in the future as it gains more ability). Currently most of 
this function is only usable with platform data - is there actually any user 
of max17042 that would need to configure the gauge without DT in the mainline 
kernel? My quick search didn't find any. Do we need or want to keep platform 
data support at all?

I'm leaning towards option 2, as it seems to me that currently this driver is 
being cluttered quite a lot by what's essentially a dead code. Adding new 
parameters to read from DT for POR initialization (which is necessary for 
other models than MAX17055) should be rather easy, but trying to fit them into 
current platform_data-oriented code may be not.

Regards,
Sebastian

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 4/4] power: supply: max17042_battery: read battery properties from device tree

[Sebastian Krzyszkowiak]

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

On piątek, 18 marca 2022 09:40:36 CET Krzysztof Kozlowski wrote:
> On 18/03/2022 01:10, Sebastian Krzyszkowiak wrote:
> > So far configuring the gauge was only possible using platform data,
> > with no way to provide the configuration on device tree-based platforms.
> > 
> > Change that by looking up the configuration values from monitored-battery
> > property. This is especially useful on models implementing ModelGauge m5
> > EZ
> > algorithm, such as MAX17055, as all the required configuration can be
> > derived from a "simple-battery" DT node there.
> > 
> > In order to be able to access power supply framework in get_of_pdata,
> > move devm_power_supply_register earlier in max17042_probe.
> > 
> > Signed-off-by: Sebastian Krzyszkowiak <sebastian.krzyszkowiak@puri.sm>
> > ---
> > 
> >  drivers/power/supply/max17042_battery.c | 50 +++++++++++++++++++------
> >  include/linux/power/max17042_battery.h  |  1 +
> >  2 files changed, 40 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/power/supply/max17042_battery.c
> > b/drivers/power/supply/max17042_battery.c index
> > c39250349a1d..4c33565802d5 100644
> > --- a/drivers/power/supply/max17042_battery.c
> > +++ b/drivers/power/supply/max17042_battery.c
> > @@ -937,7 +937,9 @@ max17042_get_of_pdata(struct max17042_chip *chip)
> > 
> >  	struct device *dev = &chip->client->dev;
> >  	struct device_node *np = dev->of_node;
> >  	u32 prop;
> > 
> > +	u64 data64;
> > 
> >  	struct max17042_platform_data *pdata;
> > 
> > +	struct power_supply_battery_info *info;
> > 
> >  	pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
> >  	if (!pdata)
> > 
> > @@ -961,6 +963,32 @@ max17042_get_of_pdata(struct max17042_chip *chip)
> > 
> >  	if (of_property_read_s32(np, "maxim,over-volt", &pdata->vmax))
> >  	
> >  		pdata->vmax = INT_MAX;
> > 
> > +	if (pdata->enable_current_sense &&
> > +	    power_supply_get_battery_info(chip->battery, &info) == 0) {
> > +		pdata->config_data = devm_kzalloc(dev, sizeof(*pdata-
>config_data),
> > GFP_KERNEL); +		if (!pdata->config_data)
> > +			return NULL;
> > +
> > +		if (info->charge_full_design_uah != -EINVAL) {
> > +			data64 = (u64)info->charge_full_design_uah * 
pdata->r_sns;
> > +			do_div(data64, MAX17042_CAPACITY_LSB);
> > +			pdata->config_data->design_cap = (u16)data64;
> > +			pdata->enable_por_init = true;
> > +		}
> > +		if (info->charge_term_current_ua != -EINVAL) {
> > +			data64 = (u64)info->charge_term_current_ua * 
pdata->r_sns;
> > +			do_div(data64, MAX17042_CURRENT_LSB);
> > +			pdata->config_data->ichgt_term = (u16)data64;
> > +			pdata->enable_por_init = true;
> > +		}
> > +		if (chip->chip_type == MAXIM_DEVICE_TYPE_MAX17055) {
> > +			if (info->voltage_max_design_uv > 4250000) {
> > +				pdata->config_data->model_cfg = 
MAX17055_MODELCFG_VCHG_BIT;
> > +				pdata->enable_por_init = true;
> > +			}
> > +		}
> > +	}
> > +
> > 
> >  	return pdata;
> >  
> >  }
> >  #endif
> > 
> > @@ -1092,16 +1120,23 @@ static int max17042_probe(struct i2c_client
> > *client,> 
> >  		return -EINVAL;
> >  	
> >  	}
> > 
> > +	i2c_set_clientdata(client, chip);
> > +	psy_cfg.drv_data = chip;
> > +	psy_cfg.of_node = dev->of_node;
> > +
> > +	chip->battery = devm_power_supply_register(&client->dev, 
max17042_desc,
> > +						   
&psy_cfg);
> > +	if (IS_ERR(chip->battery)) {
> > +		dev_err(&client->dev, "failed: power supply 
register\n");
> > +		return PTR_ERR(chip->battery);
> > +	}
> 
> I don't think it is correct. You register power supply, thus making it
> available for system, before configuring most of the data. For short
> time the chip might report to the system bogus results and events.
> 
> Instead I think you should split it into two parts - init which happens
> before registering power supply and after.

Simply splitting initialization into two parts won't really help. If you set 
capacity, current, Vchg and refresh the model after registering power supply, 
you will still end up having a short time window with bogus results. Looking 
at other drivers, they seem to deal with it in the same way - they register 
the power supply early, before the driver can fully configure the device.

To actually fix the problem with bogus data on init, it seems like we would 
either need some support from the power supply framework to notify it when can 
it actually start expecting correct data, or have a way to access the battery 
information without having to register power supply beforehand.

Since power_supply_get_battery_info doesn't actually seem to depend on 
power_supply device at all - it uses psy->dev for devm functions and psy-
>of_node to read the data from - I wonder if it could be split into a function 
that only takes an of_node?

Cheers,
Sebastian

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/4] power: supply: max17042_battery: use ModelCfg refresh on max17055

[Krzysztof Kozlowski]

On 20/03/2022 21:44, Sebastian Krzyszkowiak wrote:
> On niedziela, 20 marca 2022 13:18:49 CET Krzysztof Kozlowski wrote:
>> On 18/03/2022 20:58, Sebastian Krzyszkowiak wrote:
>>> On piątek, 18 marca 2022 09:22:16 CET Krzysztof Kozlowski wrote:
>>>> On 18/03/2022 01:10, Sebastian Krzyszkowiak wrote:
>>>>> Unlike other models, max17055 doesn't require cell characterization
>>>>> data and operates on smaller amount of input variables (DesignCap,
>>>>> VEmpty, IChgTerm and ModelCfg). Input data can already be filled in
>>>>> by max17042_override_por_values, however model refresh bit has to be
>>>>> set after adjusting input variables in order to make them apply.
>>>>>
>>>>> Signed-off-by: Sebastian Krzyszkowiak <sebastian.krzyszkowiak@puri.sm>
>>>>> ---
>>>>>
>>>>>  drivers/power/supply/max17042_battery.c | 73 +++++++++++++++----------
>>>>>  include/linux/power/max17042_battery.h  |  3 +
>>>>>  2 files changed, 48 insertions(+), 28 deletions(-)
>>>>>
>>>>> diff --git a/drivers/power/supply/max17042_battery.c
>>>>> b/drivers/power/supply/max17042_battery.c index
>>>>> c019d6c52363..c39250349a1d 100644
>>>>> --- a/drivers/power/supply/max17042_battery.c
>>>>> +++ b/drivers/power/supply/max17042_battery.c
>>>>> @@ -806,6 +806,13 @@ static inline void
>>>>> max17042_override_por_values(struct max17042_chip *chip)>
>>>>>
>>>>>  	    (chip->chip_type == MAXIM_DEVICE_TYPE_MAX17055)) {
>>>>>  		
>>>>>  		max17042_override_por(map, MAX17047_V_empty, config-
>>>>
>>>> vempty);
>>>>
>>>>>  	}
>>>>>
>>>>> +
>>>>> +	if (chip->chip_type == MAXIM_DEVICE_TYPE_MAX17055) {
>>>>> +		max17042_override_por(map, MAX17055_ModelCfg, config-
>>>>
>>>> model_cfg);
>>>>
>>>>> +		// VChg is 1 by default, so allow it to be set to 0
>>>>
>>>> Consistent comment, so /* */
>>>>
>>>> I actually do not understand fully the comment and the code. You write
>>>> entire model_cfg to MAX17055_ModelCfg and then immediately do it again,
>>>> but with smaller mask. Why?
>>>
>>> That's because VChg is 1 on POR, and max17042_override_por doesn't do
>>> anything when value equals 0 - which means that if the whole
>>> config->model_cfg is 0, VChg won't get unset (which is needed for 4.2V
>>> batteries).
>>>
>>> This could actually be replaced with a single regmap_write.
>>
>> I got it now. But if config->model_cfg is 0, should VChg be unset?
> 
> That's a good question.
> 
> max17042_override_por doesn't override the register value when the given value 
> equals zero in order to not override POR defaults with unset platform data. 
> This way one can set only the registers that they want to change in `config` 
> and the rest are untouched. This, however, only works if we assume that zero 
> means "don't touch", which isn't the case for ModelCfg.
> 
> On the Librem 5, we need to unset VChg bit because our battery is only being 
> charged up to 4.2V. Allowing to unset this bit only without having to touch 
> the rest of the register was the motivation behind the current version of this 
> patch, however, thinking about it now I can see that it fails to do that in 
> the opposite case - when the DT contains a simple-battery with maximum voltage 
> higher than 4.25V, VChg will be set in config->model_cfg causing the whole 
> register to be overwritten.

This is actually nice description which could be put into a comment there.

> 
> So, I see two possible solutions:
> 
> 1) move VChg handling to a separate variable in struct max17042_config_data. 
> This way model_cfg can stay zero when there's no need to touch the rest of the 
> register. This minimizes changes over current code.
> 
> 2) remove max17042_override_por_values in its current shape altogether and 
> make it only deal with the values that are actually being set by the driver 
> (and only extend it in the future as it gains more ability). Currently most of 
> this function is only usable with platform data - is there actually any user 
> of max17042 that would need to configure the gauge without DT in the mainline 
> kernel? My quick search didn't find any. Do we need or want to keep platform 
> data support at all?
> 
> I'm leaning towards option 2, as it seems to me that currently this driver is 
> being cluttered quite a lot by what's essentially a dead code. Adding new 
> parameters to read from DT for POR initialization (which is necessary for 
> other models than MAX17055) should be rather easy, but trying to fit them into 
> current platform_data-oriented code may be not.

I am in for removal of platform data.

Best regards,
Krzysztof

Re: [PATCH 4/4] power: supply: max17042_battery: read battery properties from device tree

[Krzysztof Kozlowski]

On 20/03/2022 22:24, Sebastian Krzyszkowiak wrote:
> On piątek, 18 marca 2022 09:40:36 CET Krzysztof Kozlowski wrote:
>> On 18/03/2022 01:10, Sebastian Krzyszkowiak wrote:
>>> So far configuring the gauge was only possible using platform data,
>>> with no way to provide the configuration on device tree-based platforms.
>>>
>>> Change that by looking up the configuration values from monitored-battery
>>> property. This is especially useful on models implementing ModelGauge m5
>>> EZ
>>> algorithm, such as MAX17055, as all the required configuration can be
>>> derived from a "simple-battery" DT node there.
>>>
>>> In order to be able to access power supply framework in get_of_pdata,
>>> move devm_power_supply_register earlier in max17042_probe.
>>>
>>> Signed-off-by: Sebastian Krzyszkowiak <sebastian.krzyszkowiak@puri.sm>
>>> ---
>>>
>>>  drivers/power/supply/max17042_battery.c | 50 +++++++++++++++++++------
>>>  include/linux/power/max17042_battery.h  |  1 +
>>>  2 files changed, 40 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/drivers/power/supply/max17042_battery.c
>>> b/drivers/power/supply/max17042_battery.c index
>>> c39250349a1d..4c33565802d5 100644
>>> --- a/drivers/power/supply/max17042_battery.c
>>> +++ b/drivers/power/supply/max17042_battery.c
>>> @@ -937,7 +937,9 @@ max17042_get_of_pdata(struct max17042_chip *chip)
>>>
>>>  	struct device *dev = &chip->client->dev;
>>>  	struct device_node *np = dev->of_node;
>>>  	u32 prop;
>>>
>>> +	u64 data64;
>>>
>>>  	struct max17042_platform_data *pdata;
>>>
>>> +	struct power_supply_battery_info *info;
>>>
>>>  	pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
>>>  	if (!pdata)
>>>
>>> @@ -961,6 +963,32 @@ max17042_get_of_pdata(struct max17042_chip *chip)
>>>
>>>  	if (of_property_read_s32(np, "maxim,over-volt", &pdata->vmax))
>>>  	
>>>  		pdata->vmax = INT_MAX;
>>>
>>> +	if (pdata->enable_current_sense &&
>>> +	    power_supply_get_battery_info(chip->battery, &info) == 0) {
>>> +		pdata->config_data = devm_kzalloc(dev, sizeof(*pdata-
>> config_data),
>>> GFP_KERNEL); +		if (!pdata->config_data)
>>> +			return NULL;
>>> +
>>> +		if (info->charge_full_design_uah != -EINVAL) {
>>> +			data64 = (u64)info->charge_full_design_uah * 
> pdata->r_sns;
>>> +			do_div(data64, MAX17042_CAPACITY_LSB);
>>> +			pdata->config_data->design_cap = (u16)data64;
>>> +			pdata->enable_por_init = true;
>>> +		}
>>> +		if (info->charge_term_current_ua != -EINVAL) {
>>> +			data64 = (u64)info->charge_term_current_ua * 
> pdata->r_sns;
>>> +			do_div(data64, MAX17042_CURRENT_LSB);
>>> +			pdata->config_data->ichgt_term = (u16)data64;
>>> +			pdata->enable_por_init = true;
>>> +		}
>>> +		if (chip->chip_type == MAXIM_DEVICE_TYPE_MAX17055) {
>>> +			if (info->voltage_max_design_uv > 4250000) {
>>> +				pdata->config_data->model_cfg = 
> MAX17055_MODELCFG_VCHG_BIT;
>>> +				pdata->enable_por_init = true;
>>> +			}
>>> +		}
>>> +	}
>>> +
>>>
>>>  	return pdata;
>>>  
>>>  }
>>>  #endif
>>>
>>> @@ -1092,16 +1120,23 @@ static int max17042_probe(struct i2c_client
>>> *client,> 
>>>  		return -EINVAL;
>>>  	
>>>  	}
>>>
>>> +	i2c_set_clientdata(client, chip);
>>> +	psy_cfg.drv_data = chip;
>>> +	psy_cfg.of_node = dev->of_node;
>>> +
>>> +	chip->battery = devm_power_supply_register(&client->dev, 
> max17042_desc,
>>> +						   
> &psy_cfg);
>>> +	if (IS_ERR(chip->battery)) {
>>> +		dev_err(&client->dev, "failed: power supply 
> register\n");
>>> +		return PTR_ERR(chip->battery);
>>> +	}
>>
>> I don't think it is correct. You register power supply, thus making it
>> available for system, before configuring most of the data. For short
>> time the chip might report to the system bogus results and events.
>>
>> Instead I think you should split it into two parts - init which happens
>> before registering power supply and after.
> 
> Simply splitting initialization into two parts won't really help. If you set 
> capacity, current, Vchg and refresh the model after registering power supply, 
> you will still end up having a short time window with bogus results. Looking 
> at other drivers, they seem to deal with it in the same way - they register 
> the power supply early, before the driver can fully configure the device.
> 
> To actually fix the problem with bogus data on init, it seems like we would 
> either need some support from the power supply framework to notify it when can 
> it actually start expecting correct data, or have a way to access the battery 
> information without having to register power supply beforehand.

Indeed I spotted similar pattern in other drivers, so this might be a
common issue.

> 
> Since power_supply_get_battery_info doesn't actually seem to depend on 
> power_supply device at all - it uses psy->dev for devm functions and psy-
> of_node to read the data from - I wonder if it could be split into a function 
> that only takes an of_node?

That might be the best approach.


Best regards,
Krzysztof