max77693: USB event listener for charger

This prepares for an updated regulator and charger driver. The defines are needed to set the maximum input current and the fast charge current.

Signed-off-by Wolfgang Wiedmeyer <[email protected]>

---
 include/linux/mfd/max77693-private.h | 7 +++++++
 1 file changed, 7 insertions(+)

include/linux/mfd/max77693-private.h +7 -0

index 3c7a63b..ca18344 100644
@@ -222,6 +222,9 @@ enum max77693_charger_battery_state {
 #define CHG_CNFG_00_CHG_MASK		0x1
 #define CHG_CNFG_00_BUCK_MASK		0x4
 
+/* MAX77693 CHG_CNFG_02 register */
+#define CHG_CNFG_02_CC_MASK		0x3F
+
 /* MAX77693_CHG_REG_CHG_CNFG_01 register */
 #define CHG_CNFG_01_FCHGTIME_SHIFT	0
 #define CHG_CNFG_01_CHGRSTRT_SHIFT	4

@@ -258,6 +261,10 @@ enum max77693_charger_battery_state {
 
 /* MAX77693 CHG_CNFG_09 Register */
 #define CHG_CNFG_09_CHGIN_ILIM_MASK	0x7F
+#define CHG_CNFG_09_CHGIN_ILIM_500_MAX	500000
+#define CHG_CNFG_09_CHGIN_ILIM_500_MIN	470000
+#define CHG_CNFG_09_CHGIN_ILIM_0_MAX	60000
+#define CHG_CNFG_09_CHGIN_ILIM_0_MIN	0
 
 /* MAX77693 CHG_CTRL Register */
 #define SAFEOUT_CTRL_SAFEOUT1_MASK	0x3

Signature · 1 line

2.8.0.rc3

Quoted 5 lines · Wolfgang Wiedmeyer

This prepares for an updated regulator and charger driver. The defines are needed to set the maximum input current and the fast charge current.

Signed-off-by Wolfgang Wiedmeyer <[email protected]>

This shouldn't be separate patch, because:
1. You are touching later power and regulator so this patch should be applied to one and then pulled by other... a lot of unneeded work.
2. It is difficult to find which new code is needed for which driver.

Simpler approach might work - when changing the regulator or psy driver, just change the header as needed. Unless there are conflicts and both of them need the same?

Best regards,
Krzysztof

Quoted 32 lines

---
 include/linux/mfd/max77693-private.h | 7 +++++++
 1 file changed, 7 insertions(+)

include/linux/mfd/max77693-private.h +7 -0

index 3c7a63b..ca18344 100644
@@ -222,6 +222,9 @@ enum max77693_charger_battery_state {
 #define CHG_CNFG_00_CHG_MASK		0x1
 #define CHG_CNFG_00_BUCK_MASK		0x4
 
+/* MAX77693 CHG_CNFG_02 register */
+#define CHG_CNFG_02_CC_MASK		0x3F
+
 /* MAX77693_CHG_REG_CHG_CNFG_01 register */
 #define CHG_CNFG_01_FCHGTIME_SHIFT	0
 #define CHG_CNFG_01_CHGRSTRT_SHIFT	4

@@ -258,6 +261,10 @@ enum max77693_charger_battery_state {
 
 /* MAX77693 CHG_CNFG_09 Register */
 #define CHG_CNFG_09_CHGIN_ILIM_MASK	0x7F
+#define CHG_CNFG_09_CHGIN_ILIM_500_MAX	500000
+#define CHG_CNFG_09_CHGIN_ILIM_500_MIN	470000
+#define CHG_CNFG_09_CHGIN_ILIM_0_MAX	60000
+#define CHG_CNFG_09_CHGIN_ILIM_0_MIN	0
 
 /* MAX77693 CHG_CTRL Register */
 #define SAFEOUT_CTRL_SAFEOUT1_MASK	0x3

Signature · 1 line

2.8.0.rc3

For MAX77693, the fast charge current also needs to be manipulated for proper charging. The fast charge current is only set in the case of the MAX77693 type, as the MAX77843 properly manipulates the fast charge current.

The fast charge current is set to the next possible value below the maximum input current.

Signed-off-by Wolfgang Wiedmeyer <[email protected]>

---
 drivers/regulator/max77693-regulator.c | 45 +++++++++++++++++++++++++++++++---
 1 file changed, 42 insertions(+), 3 deletions(-)

drivers/regulator/max77693-regulator.c +42 -3

index cfbb951..e2f7584 100644
@@ -54,14 +54,19 @@ struct chg_reg_data {
 	unsigned int linear_mask;
 	unsigned int uA_step;
 	unsigned int min_sel;
+
+	bool set_fast;
+	unsigned int fast_reg;
+	unsigned int fast_mask;
 };
 
 /*
  * MAX77693 CHARGER regulator - Min : 20mA, Max : 2580mA, step : 20mA
  * 0x00, 0x01, 0x2, 0x03	= 60 mA
  * 0x04 ~ 0x7E			= (60 + (X - 3) * 20) mA
- * Actually for MAX77693 the driver manipulates the maximum input current,
- * not the fast charge current (output). This should be fixed.
+ * Actually for MAX77693 the driver manipulates the maximum input current
+ * and the fast charge current (output) because the fast charge current
+ * is not set.
  *
  * On MAX77843 the calculation formula is the same (except values).
  * Fortunately it properly manipulates the fast charge current.

@@ -100,6 +105,8 @@ static int max77693_chg_set_current_limit(struct regulator_dev *rdev,
 	const struct chg_reg_data *reg_data = rdev_get_drvdata(rdev);
 	unsigned int chg_min_uA = rdev->constraints->min_uA;
 	int sel = 0;
+	unsigned int data;
+	int ret;
 
 	while (chg_min_uA + reg_data->uA_step * sel < min_uA)
 		sel++;

@@ -110,7 +117,35 @@ static int max77693_chg_set_current_limit(struct regulator_dev *rdev,
 	/* the first four codes for charger current are all 60mA */
 	sel += reg_data->min_sel;
 
-	return regmap_write(rdev->regmap, reg_data->linear_reg, sel);
+	ret = regmap_write(rdev->regmap, reg_data->linear_reg, sel);
+	if (ret < 0)
+		return ret;
+
+	if (reg_data->set_fast) {
+		/* disable fast charge if minimum value */
+		if (sel == reg_data->min_sel)
+			data = 0;
+		else {
+			/*
+			 * set the fast charge current to the closest value
+			 * below the input current
+			 */
+			ret = regmap_read(rdev->regmap, reg_data->fast_reg,
+					  &data);
+			if (ret < 0)
+				return ret;
+
+			sel *= reg_data->uA_step / 1000; /* convert to mA */
+			data &= ~reg_data->fast_mask;
+			data |= sel * 10 / 333; /* 0.1A/3 steps */
+		}
+
+		ret = regmap_write(rdev->regmap, reg_data->fast_reg, data);
+		if (ret < 0)
+			return ret;
+	}
+
+	return 0;
 }
 /* end of CHARGER regulator ops */
 

@@ -197,6 +232,9 @@ static const struct chg_reg_data max77693_chg_reg_data = {
 	.linear_mask	= CHG_CNFG_09_CHGIN_ILIM_MASK,
 	.uA_step	= 20000,
 	.min_sel	= 3,
+	.set_fast	= true,
+	.fast_reg	= MAX77693_CHG_REG_CHG_CNFG_02,
+	.fast_mask	= CHG_CNFG_02_CC_MASK,
 };
 
 #define	max77843_regulator_desc_esafeout(num)	{			\

@@ -237,6 +275,7 @@ static const struct chg_reg_data max77843_chg_reg_data = {
 	.linear_mask	= MAX77843_CHG_FAST_CHG_CURRENT_MASK,
 	.uA_step	= MAX77843_CHG_FAST_CHG_CURRENT_STEP,
 	.min_sel	= 2,
+	.set_fast	= false,
 };
 
 static int max77693_pmic_probe(struct platform_device *pdev)

Signature · 1 line

2.8.0.rc3

Quoted 4 lines · Wolfgang Wiedmeyer

For MAX77693, the fast charge current also needs to be manipulated for proper charging. The fast charge current is only set in the case of the MAX77693 type, as the MAX77843 properly manipulates the fast charge current.

Are you sure it has to be manipulated? Some time I didn't dig into this. Now I looked at the datasheet and it says that usually there is no need for changing the charge current during the operation. Maxim recommends to setting it to a maximum safe value for the battery. The device will manage charge current on its own.

However I agree that the charge current should be set... maybe once, to a maximum value appropriate for battery. Probably this should be done by max77693_charger in max77693_dt_init().

Best regards,
Krzysztof

Quoted 103 lines

The fast charge current is set to the next possible value below the maximum input current.

Signed-off-by Wolfgang Wiedmeyer <[email protected]>

---
 drivers/regulator/max77693-regulator.c | 45 +++++++++++++++++++++++++++++++---
 1 file changed, 42 insertions(+), 3 deletions(-)

drivers/regulator/max77693-regulator.c +42 -3

index cfbb951..e2f7584 100644
@@ -54,14 +54,19 @@ struct chg_reg_data {
 	unsigned int linear_mask;
 	unsigned int uA_step;
 	unsigned int min_sel;
+
+	bool set_fast;
+	unsigned int fast_reg;
+	unsigned int fast_mask;
 };
 
 /*
  * MAX77693 CHARGER regulator - Min : 20mA, Max : 2580mA, step : 20mA
  * 0x00, 0x01, 0x2, 0x03	= 60 mA
  * 0x04 ~ 0x7E			= (60 + (X - 3) * 20) mA
- * Actually for MAX77693 the driver manipulates the maximum input current,
- * not the fast charge current (output). This should be fixed.
+ * Actually for MAX77693 the driver manipulates the maximum input current
+ * and the fast charge current (output) because the fast charge current
+ * is not set.
  *
  * On MAX77843 the calculation formula is the same (except values).
  * Fortunately it properly manipulates the fast charge current.

@@ -100,6 +105,8 @@ static int max77693_chg_set_current_limit(struct regulator_dev *rdev,
 	const struct chg_reg_data *reg_data = rdev_get_drvdata(rdev);
 	unsigned int chg_min_uA = rdev->constraints->min_uA;
 	int sel = 0;
+	unsigned int data;
+	int ret;
 
 	while (chg_min_uA + reg_data->uA_step * sel < min_uA)
 		sel++;

@@ -110,7 +117,35 @@ static int max77693_chg_set_current_limit(struct regulator_dev *rdev,
 	/* the first four codes for charger current are all 60mA */
 	sel += reg_data->min_sel;
 
-	return regmap_write(rdev->regmap, reg_data->linear_reg, sel);
+	ret = regmap_write(rdev->regmap, reg_data->linear_reg, sel);
+	if (ret < 0)
+		return ret;
+
+	if (reg_data->set_fast) {
+		/* disable fast charge if minimum value */
+		if (sel == reg_data->min_sel)
+			data = 0;
+		else {
+			/*
+			 * set the fast charge current to the closest value
+			 * below the input current
+			 */
+			ret = regmap_read(rdev->regmap, reg_data->fast_reg,
+					  &data);
+			if (ret < 0)
+				return ret;
+
+			sel *= reg_data->uA_step / 1000; /* convert to mA */
+			data &= ~reg_data->fast_mask;
+			data |= sel * 10 / 333; /* 0.1A/3 steps */
+		}
+
+		ret = regmap_write(rdev->regmap, reg_data->fast_reg, data);
+		if (ret < 0)
+			return ret;
+	}
+
+	return 0;
 }
 /* end of CHARGER regulator ops */
 

@@ -197,6 +232,9 @@ static const struct chg_reg_data max77693_chg_reg_data = {
 	.linear_mask	= CHG_CNFG_09_CHGIN_ILIM_MASK,
 	.uA_step	= 20000,
 	.min_sel	= 3,
+	.set_fast	= true,
+	.fast_reg	= MAX77693_CHG_REG_CHG_CNFG_02,
+	.fast_mask	= CHG_CNFG_02_CC_MASK,
 };
 
 #define	max77843_regulator_desc_esafeout(num)	{			\

@@ -237,6 +275,7 @@ static const struct chg_reg_data max77843_chg_reg_data = {
 	.linear_mask	= MAX77843_CHG_FAST_CHG_CURRENT_MASK,
 	.uA_step	= MAX77843_CHG_FAST_CHG_CURRENT_STEP,
 	.min_sel	= 2,
+	.set_fast	= false,
 };
 
 static int max77693_pmic_probe(struct platform_device *pdev)

Signature · 1 line

2.8.0.rc3

When charging is disabled (e.g. by removing the USB cable) the charge current is not reset to zero. So if I expose the current by the CURRENT_NOW property, it incorrectly reports the current that was set when charging was enabled, although there is no charging going on anymore. So I felt the need to update the charge current every time the charger gets enabled or disabled.
Initially, the charge current is set to zero, so I think it needs to be set at least at the beginning to enable charging.

Quoted 8 lines · Wolfgang Wiedmeyer

When charging is disabled (e.g. by removing the USB cable) the charge current is not reset to zero. So if I expose the current by the CURRENT_NOW property, it incorrectly reports the current that was set when charging was enabled, although there is no charging going on anymore. So I felt the need to update the charge current every time the charger gets enabled or disabled.
Initially, the charge current is set to zero, so I think it needs to be set at least at the beginning to enable charging.

Are you sure that the register value you're looking at is the actual charge current right now and not just the maximum that the charger will try to use depending on the conditions (supply available, battery state...)? It seems like you're acting as though it's the latter but that's not what the chip is doing.

Quoted 11 lines · Mark Brown

Quoted 8 lines · Wolfgang Wiedmeyer

When charging is disabled (e.g. by removing the USB cable) the charge current is not reset to zero. So if I expose the current by the CURRENT_NOW property, it incorrectly reports the current that was set when charging was enabled, although there is no charging going on anymore. So I felt the need to update the charge current every time the charger gets enabled or disabled.
Initially, the charge current is set to zero, so I think it needs to be set at least at the beginning to enable charging.

Quoted 5 lines

Are you sure that the register value you're looking at is the actual charge current right now and not just the maximum that the charger will try to use depending on the conditions (supply available, battery state...)? It seems like you're acting as though it's the latter but that's not what the chip is doing.

I was looking at the vendor code that was released for the Galaxy S3 and there the same register gets accessed for getting the current for the CURRENT_NOW property [1] and for setting the current [2]. So is this probably the wrong use of the CURRENT_NOW property because not the actual charge current is read but the maximum value that was set?

This patch adds a listener for extcon cable events and enables charging if an USB cable is connected. It recognizes SDP and DCP cable types and treats them the same (same input current and fast charge current). The maximum input current is set before the charger is enabled and before the charger gets disabled, the maximum input current is set to zero. The listener is inspired by the listener implementation that was used for the AXP288 Charger driver.

The patch also adds support for the CURRENT_NOW property. It reads the fast charge current that gets set before the charger is enabled or disabled.

Signed-off-by Wolfgang Wiedmeyer <[email protected]>

Quoted 8 lines · Wolfgang Wiedmeyer

This patch adds a listener for extcon cable events and enables charging if an USB cable is connected. It recognizes SDP and DCP cable types and treats them the same (same input current and fast charge current). The maximum input current is set before the charger is enabled and before the charger gets disabled, the maximum input current is set to zero. The listener is inspired by the listener implementation that was used for the AXP288 Charger driver.

Quoted 5 lines

The patch also adds support for the CURRENT_NOW property. It reads the fast charge current that gets set before the charger is enabled or disabled.

Signed-off-by Wolfgang Wiedmeyer <[email protected]>

No. This power supply driver should not manage regulators. It is not a regulator consumer. For that specific need, there is a charger-manager driver.

I agree that you might configure here the charger. You might even expose some writeable properties through power supply class. However the purpose of this driver is to expose the battery charger to user-space, not to replace the user-space with its work.

So... NACK.

If you would like to play with charger-manager, here is my old DTS for Trats2 (might need updates):

index 595ad4ba6977..b4361b4a9de7 100644

arch/arm/boot/dts/exynos4412-trats2.dts +38 -0

@@ -856,6 +856,44 @@
                };
        };
 
+       charger-manager@0 {
+               compatible = "charger-manager";
+               status = "okay";
+               chg-reg-supply = <&charger_reg>;
+
+               cm-name = "battery";
+               /* Polling only for external power source */
+               cm-poll-mode = <2>;
+               cm-poll-interval = <30000>;
+
+               cm-fullbatt-vchkdrop-ms = <30000>;
+               cm-fullbatt-vchkdrop-volt = <150000>;
+               cm-fullbatt-soc = <100>;
+
+               cm-battery-stat = <0>;
+               cm-fuel-gauge = "max170xx_battery";
+
+               /* Allow charging for 5hr */
+               cm-charging-max = <18000000>;
+               /* Allow discharging for 2hr */
+               cm-discharging-max = <7200000>;
+
+               cm-num-chargers = <1>;
+               cm-chargers = "max77693-charger";
+
+               charger@0 {
+                       cm-regulator-name = "chg-reg";
+                       cable@0 {
+                               cm-cable-name = "USB";
+                               cm-cable-extcon = "max77693-muic";
+                       };
+                       cable@1 {
+                               cm-cable-name = "TA";
+                               cm-cable-extcon = "max77693-muic";
+                       };
+               };
+       };
+
        exynos-usbphy@125B0000 {
                status = "okay";
        };

Signed-off-by Krzysztof Kozlowski <[email protected]>

Best regards,
Krzysztof

Quoted 9 lines · Krzysztof Kozlowski

Quoted 8 lines · Wolfgang Wiedmeyer

This patch adds a listener for extcon cable events and enables charging if an USB cable is connected. It recognizes SDP and DCP cable types and treats them the same (same input current and fast charge current). The maximum input current is set before the charger is enabled and before the charger gets disabled, the maximum input current is set to zero. The listener is inspired by the listener implementation that was used for the AXP288 Charger driver.

Quoted 8 lines

Quoted 5 lines

The patch also adds support for the CURRENT_NOW property. It reads the fast charge current that gets set before the charger is enabled or disabled.

Signed-off-by Wolfgang Wiedmeyer <[email protected]>

No. This power supply driver should not manage regulators. It is not a regulator consumer. For that specific need, there is a charger-manager driver.

When I was in the middle of implementing this, I noticed that the charger manager does everything that is needed. But it took me quite some time to configure the DTS correctly until I realized that the charger manager used a deprecated function (extcon_register_interest()) and thus couldn't work. And as I didn't see the charger-manager in any other device's DTS, I thought that this might not be right way.
But Chanwoo Choi has a fix: https://patchwork.kernel.org/patch/8898541/
So I will try to get it working with this patch.

Quoted 6 lines

I agree that you might configure here the charger. You might even expose some writeable properties through power supply class. However the purpose of this driver is to expose the battery charger to user-space, not to replace the user-space with its work.

So... NACK.

Ok, then I will try to reduce the patch to the CURRENT_NOW property support.

Quoted 2 lines

If you would like to play with charger-manager, here is my old DTS for Trats2 (might need updates):

Is there a reason that this patch is not in the kernel? It would have been very helpful for me :)