Skip to contentSkip to conversation

Fix RG35XX Battery Charging Issues

PATCH Applied Chris Morgan 4 participants 12 messages Jan 31, 2025 – Feb 3, 2025

Subsystems (5)

v1 of 2 Applied Latest is v2 →

lore
Share & tools
lore
#0 lore raw

cover Fix RG35XX Battery Charging Issues

From: Chris Morgan <[email protected]>

The Anbernic RG35XX devices sometimes fail to charge when the register for the battery temperature sensor is set to the incorrect value either by user error or an incorrectly programmed efuse. Allow users to hard-code if a temperature sensor is not present (which is the case for all Anbernic RGxx series devices) to prevent this issue from causing problems. Additionally, a bug was identified with the handling of PMU faults while this fix was being tested.

Chris Morgan (5):
power: supply: axp20x_battery: Fix fault handling for AXP717
dt-bindings: power: supply: axp20x-battery: Add x-powers,no-thermistor
mfd: axp20x: AXP717: Add AXP717_TS_PIN_CFG to writeable regs
power: supply: axp20x_battery: Update temp sensor for AXP717 from
device tree
arm64: dts: allwinner: rg35xx: Add no-thermistor property for battery

 .../x-powers,axp20x-battery-power-supply.yaml | 22 ++++++--
 .../sun50i-h700-anbernic-rg35xx-2024.dts      |  1 +
 drivers/mfd/axp20x.c                          |  2 +-
 drivers/power/supply/axp20x_battery.c         | 50 +++++++++++++------
 include/linux/mfd/axp20x.h                    |  1 +
 5 files changed, 56 insertions(+), 20 deletions(-)
Signature · 1 line
2.43.0
#1 lore raw

PATCH 1/5 power: supply: axp20x_battery: Fix fault handling for AXP717

From: Chris Morgan <[email protected]>

Correct the fault handling for the AXP717 by changing the i2c write from regmap_update_bits() to regmap_write_bits(). The update bits function does not work properly on a RW1C register where we must write a 1 back to an existing register to clear it.

Additionally, as part of this testing I confirmed the behavior of errors reappearing, so remove comment about assumptions.

Fixes 6625767049c2 ("power: supply: axp20x_battery: add support for AXP717")
---
 drivers/power/supply/axp20x_battery.c | 31 +++++++++++++--------------
 1 file changed, 15 insertions(+), 16 deletions(-)
drivers/power/supply/axp20x_battery.c +15 -16
index fa27195f074e..3c3158f31a48 100644
@@ -466,10 +466,9 @@ static int axp717_battery_get_prop(struct power_supply *psy,
 
 	/*
 	 * If a fault is detected it must also be cleared; if the
-	 * condition persists it should reappear (This is an
-	 * assumption, it's actually not documented). A restart was
-	 * not sufficient to clear the bit in testing despite the
-	 * register listed as POR.
+	 * condition persists it should reappear. A restart was not
+	 * sufficient to clear the bit in testing despite the register
+	 * listed as POR.
 	 */
 	case POWER_SUPPLY_PROP_HEALTH:
 		ret = regmap_read(axp20x_batt->regmap, AXP717_PMU_FAULT,
@@ -480,26 +479,26 @@ static int axp717_battery_get_prop(struct power_supply *psy,
 		switch (reg & AXP717_BATT_PMU_FAULT_MASK) {
 		case AXP717_BATT_UVLO_2_5V:
 			val->intval = POWER_SUPPLY_HEALTH_DEAD;
-			regmap_update_bits(axp20x_batt->regmap,
-					   AXP717_PMU_FAULT,
-					   AXP717_BATT_UVLO_2_5V,
-					   AXP717_BATT_UVLO_2_5V);
+			regmap_write_bits(axp20x_batt->regmap,
+					  AXP717_PMU_FAULT,
+					  AXP717_BATT_UVLO_2_5V,
+					  AXP717_BATT_UVLO_2_5V);
 			return 0;
 
 		case AXP717_BATT_OVER_TEMP:
 			val->intval = POWER_SUPPLY_HEALTH_HOT;
-			regmap_update_bits(axp20x_batt->regmap,
-					   AXP717_PMU_FAULT,
-					   AXP717_BATT_OVER_TEMP,
-					   AXP717_BATT_OVER_TEMP);
+			regmap_write_bits(axp20x_batt->regmap,
+					  AXP717_PMU_FAULT,
+					  AXP717_BATT_OVER_TEMP,
+					  AXP717_BATT_OVER_TEMP);
 			return 0;
 
 		case AXP717_BATT_UNDER_TEMP:
 			val->intval = POWER_SUPPLY_HEALTH_COLD;
-			regmap_update_bits(axp20x_batt->regmap,
-					   AXP717_PMU_FAULT,
-					   AXP717_BATT_UNDER_TEMP,
-					   AXP717_BATT_UNDER_TEMP);
+			regmap_write_bits(axp20x_batt->regmap,
+					  AXP717_PMU_FAULT,
+					  AXP717_BATT_UNDER_TEMP,
+					  AXP717_BATT_UNDER_TEMP);
 			return 0;
 
 		default:
Signature · 1 line
2.43.0
Reviewed-by on v1 1/5
#2 lore raw

Re: [PATCH 1/5] power: supply: axp20x_battery: Fix fault handling for AXP717

Quoted 8 lines · Chris Morgan

From: Chris Morgan <[email protected]>

Correct the fault handling for the AXP717 by changing the i2c write from regmap_update_bits() to regmap_write_bits(). The update bits function does not work properly on a RW1C register where we must write a 1 back to an existing register to clear it.

Quoted 5 lines

Additionally, as part of this testing I confirmed the behavior of errors reappearing, so remove comment about assumptions.

Fixes 6625767049c2 ("power: supply: axp20x_battery: add support for AXP717")
#3 lore raw

PATCH 2/5 dt-bindings: power: supply: axp20x-battery: Add x-powers,no-thermistor

In reply to Chris Morgan

From: Chris Morgan <[email protected]>

Add the vendor specific boolean property of x-powers,no-thermistor. This property optionally describes hardware where no thermistor is present on the battery and is specific to the AXP717. In rare circumstances this value can be set incorrectly in the efuse of the PMIC, and if it is not hard-coded the device will fail to charge.

---
 .../x-powers,axp20x-battery-power-supply.yaml | 22 ++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)
Documentation/devicetree/bindings/power/supply/x-powers,axp20x-battery-power-supply.yaml +19 -3
index 5ccd375eb294..e0f134a13f7e 100644
@@ -14,9 +14,6 @@ maintainers:
   - Chen-Yu Tsai <[email protected]>
   - Sebastian Reichel <[email protected]>
 
-allOf:
-  - $ref: power-supply.yaml#
-
 properties:
   compatible:
     oneOf:
@@ -35,7 +32,26 @@ properties:
       this gauge.
     $ref: /schemas/types.yaml#/definitions/phandle
 
+  x-powers,no-thermistor:
+    type: boolean
+    description: Indicates that no thermistor is connected to the TS pin
+
 required:
   - compatible
 
+allOf:
+  - $ref: power-supply.yaml#
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - x-powers,axp717-battery-power-supply
+    then:
+      properties:
+        x-powers,no-thermistor: true
+    else:
+      properties:
+        x-powers,no-thermistor: false
+
 additionalProperties: false
Signature · 1 line
2.43.0
Reviewed-by on v1 2/5
#4 lore raw

Re: [PATCH 2/5] dt-bindings: power: supply: axp20x-battery: Add x-powers,no-thermistor

Quoted 31 lines · Chris Morgan

From: Chris Morgan <[email protected]>

Add the vendor specific boolean property of x-powers,no-thermistor. This property optionally describes hardware where no thermistor is present on the battery and is specific to the AXP717. In rare circumstances this value can be set incorrectly in the efuse of the PMIC, and if it is not hard-coded the device will fail to charge.

---
 .../x-powers,axp20x-battery-power-supply.yaml | 22 ++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)
Documentation/devicetree/bindings/power/supply/x-powers,axp20x-battery-power-supply.yaml +0 -3
index 5ccd375eb294..e0f134a13f7e 100644
@@ -14,9 +14,6 @@ maintainers:
   - Chen-Yu Tsai <[email protected]>
   - Sebastian Reichel <[email protected]>
 
-allOf:
-  - $ref: power-supply.yaml#
-
 properties:
   compatible:
     oneOf:
@@ -35,7 +32,26 @@ properties:
       this gauge.
     $ref: /schemas/types.yaml#/definitions/phandle
 

Documentation/devicetree/bindings/power/supply/x-powers,axp20x-battery-power-supply.yaml properties: · line 44

Quoted 10 lines
Documentation/devicetree/bindings/power/supply/x-powers,axp20x-battery-power-supply.yaml +7 -0
+  x-powers,no-thermistor:
+    type: boolean
+    description: Indicates that no thermistor is connected to the TS pin
+
 required:
   - compatible
 
+allOf:
+  - $ref: power-supply.yaml#
+  - if:

This could be simpler - use not: here.

Documentation/devicetree/bindings/power/supply/x-powers,axp20x-battery-power-supply.yaml properties: · line 49

Quoted 5 lines
Documentation/devicetree/bindings/power/supply/x-powers,axp20x-battery-power-supply.yaml +5 -0
+      properties:
+        compatible:
+          contains:
+            enum:
+              - x-powers,axp717-battery-power-supply

Anyway,

Best regards,
Krzysztof

#5 lore raw

PATCH 3/5 mfd: axp20x: AXP717: Add AXP717_TS_PIN_CFG to writeable regs

In reply to Chris Morgan

From: Chris Morgan <[email protected]>

Add AXP717_TS_PIN_CFG (register 0x50) to the table of writeable registers so that the temperature sensor can be configured by the battery driver.

---
 drivers/mfd/axp20x.c       | 2 +-
 include/linux/mfd/axp20x.h | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)
drivers/mfd/axp20x.c +1 -1
index cff56deba24f..baf51dea98b0 100644
@@ -222,7 +222,7 @@ static const struct regmap_range axp717_writeable_ranges[] = {
 	regmap_reg_range(AXP717_PMU_FAULT, AXP717_MODULE_EN_CONTROL_1),
 	regmap_reg_range(AXP717_MIN_SYS_V_CONTROL, AXP717_BOOST_CONTROL),
 	regmap_reg_range(AXP717_VSYS_V_POWEROFF, AXP717_VSYS_V_POWEROFF),
-	regmap_reg_range(AXP717_IRQ0_EN, AXP717_IRQ4_EN),
+	regmap_reg_range(AXP717_IRQ0_EN, AXP717_TS_PIN_CFG),
 	regmap_reg_range(AXP717_IRQ0_STATE, AXP717_IRQ4_STATE),
 	regmap_reg_range(AXP717_ICC_CHG_SET, AXP717_CV_CHG_SET),
 	regmap_reg_range(AXP717_DCDC_OUTPUT_CONTROL, AXP717_CPUSLDO_CONTROL),
include/linux/mfd/axp20x.h +1 -0
index c3df0e615fbf..3c5aecf1d4b5 100644
@@ -137,6 +137,7 @@ enum axp20x_variants {
 #define AXP717_IRQ2_STATE		0x4a
 #define AXP717_IRQ3_STATE		0x4b
 #define AXP717_IRQ4_STATE		0x4c
+#define AXP717_TS_PIN_CFG		0x50
 #define AXP717_ICC_CHG_SET		0x62
 #define AXP717_ITERM_CHG_SET		0x63
 #define AXP717_CV_CHG_SET		0x64
Signature · 1 line
2.43.0
#6 lore raw

Re: [PATCH 3/5] mfd: axp20x: AXP717: Add AXP717_TS_PIN_CFG to writeable regs

Quoted 18 lines · Chris Morgan

From: Chris Morgan <[email protected]>

Add AXP717_TS_PIN_CFG (register 0x50) to the table of writeable registers so that the temperature sensor can be configured by the battery driver.

---
 drivers/mfd/axp20x.c       | 2 +-
 include/linux/mfd/axp20x.h | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)
drivers/mfd/axp20x.c +0 -0
index cff56deba24f..baf51dea98b0 100644

drivers/mfd/axp20x.c static const struct regmap_range axp717_writeable_ranges[] = { · line 225

Quoted 5 lines
drivers/mfd/axp20x.c +1 -1
@@ -222,7 +222,7 @@ static const struct regmap_range axp717_writeable_ranges[] = {
        regmap_reg_range(AXP717_PMU_FAULT, AXP717_MODULE_EN_CONTROL_1),
        regmap_reg_range(AXP717_MIN_SYS_V_CONTROL, AXP717_BOOST_CONTROL),
        regmap_reg_range(AXP717_VSYS_V_POWEROFF, AXP717_VSYS_V_POWEROFF),
-       regmap_reg_range(AXP717_IRQ0_EN, AXP717_IRQ4_EN),
+       regmap_reg_range(AXP717_IRQ0_EN, AXP717_TS_PIN_CFG),

Please add a separate entry. As you can see immediately below, there is a separate entry for the IRQ status registers. We're not allowing writes to unknown registers.

ChenYu

Quoted 19 lines
drivers/mfd/axp20x.c +0 -0
        regmap_reg_range(AXP717_IRQ0_STATE, AXP717_IRQ4_STATE),
        regmap_reg_range(AXP717_ICC_CHG_SET, AXP717_CV_CHG_SET),
        regmap_reg_range(AXP717_DCDC_OUTPUT_CONTROL, AXP717_CPUSLDO_CONTROL),
include/linux/mfd/axp20x.h +1 -1
index c3df0e615fbf..3c5aecf1d4b5 100644
@@ -137,6 +137,7 @@ enum axp20x_variants {
 #define AXP717_IRQ2_STATE              0x4a
 #define AXP717_IRQ3_STATE              0x4b
 #define AXP717_IRQ4_STATE              0x4c
+#define AXP717_TS_PIN_CFG              0x50
 #define AXP717_ICC_CHG_SET             0x62
 #define AXP717_ITERM_CHG_SET           0x63
 #define AXP717_CV_CHG_SET              0x64
--

2.43.0

#7 lore raw

PATCH 4/5 power: supply: axp20x_battery: Update temp sensor for AXP717 from device tree

In reply to Chris Morgan

From: Chris Morgan <[email protected]>

Allow a boolean property of "x-powers,no-thermistor" to specify devices where the ts pin is not connected to anything. This works around an issue found with some devices where the efuse is not programmed correctly from the factory or when the register gets set erroneously.

---
 drivers/power/supply/axp20x_battery.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)
drivers/power/supply/axp20x_battery.c +19 -0
index 3c3158f31a48..345a1bbe50ad 100644
@@ -89,6 +89,8 @@
 #define AXP717_BAT_CC_MIN_UA		0
 #define AXP717_BAT_CC_MAX_UA		3008000
 
+#define AXP717_TS_PIN_DISABLE		BIT(4)
+
 struct axp20x_batt_ps;
 
 struct axp_data {
@@ -117,6 +119,7 @@ struct axp20x_batt_ps {
 	/* Maximum constant charge current */
 	unsigned int max_ccc;
 	const struct axp_data	*data;
+	bool ts_disable;
 };
 
 static int axp20x_battery_get_max_voltage(struct axp20x_batt_ps *axp20x_batt,
@@ -984,6 +987,22 @@ static void axp717_set_battery_info(struct platform_device *pdev,
 	int ccc = info->constant_charge_current_max_ua;
 	int val;
 
+	axp_batt->ts_disable = (device_property_read_bool(axp_batt->dev,
+							  "x-powers,no-thermistor"));
+
+	/*
+	 * Under rare conditions an incorrectly programmed efuse for
+	 * the temp sensor on the PMIC may trigger a fault condition.
+	 * Allow users to hard-code if the ts pin is not used to work
+	 * around this problem.
+	 */
+	if (axp_batt->ts_disable) {
+		regmap_update_bits(axp_batt->regmap,
+				   AXP717_TS_PIN_CFG,
+				   AXP717_TS_PIN_DISABLE,
+				   AXP717_TS_PIN_DISABLE);
+	}
+
 	if (vmin > 0 && axp717_set_voltage_min_design(axp_batt, vmin))
 		dev_err(&pdev->dev,
 			"couldn't set voltage_min_design\n");
Signature · 1 line
2.43.0
#8 lore raw

Re: [PATCH 4/5] power: supply: axp20x_battery: Update temp sensor for AXP717 from device tree

Quoted 35 lines · Chris Morgan

From: Chris Morgan <[email protected]>

Allow a boolean property of "x-powers,no-thermistor" to specify devices where the ts pin is not connected to anything. This works around an issue found with some devices where the efuse is not programmed correctly from the factory or when the register gets set erroneously.

---
 drivers/power/supply/axp20x_battery.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)
drivers/power/supply/axp20x_battery.c +3 -0
index 3c3158f31a48..345a1bbe50ad 100644
@@ -89,6 +89,8 @@
 #define AXP717_BAT_CC_MIN_UA           0
 #define AXP717_BAT_CC_MAX_UA           3008000
 
+#define AXP717_TS_PIN_DISABLE          BIT(4)
+
 struct axp20x_batt_ps;
 
 struct axp_data {
@@ -117,6 +119,7 @@ struct axp20x_batt_ps {
        /* Maximum constant charge current */
        unsigned int max_ccc;
        const struct axp_data   *data;
+       bool ts_disable;
 };
 
 static int axp20x_battery_get_max_voltage(struct axp20x_batt_ps *axp20x_batt,

drivers/power/supply/axp20x_battery.c static void axp717_set_battery_info(struct platform_device *pdev, · line 996

Quoted 10 lines
drivers/power/supply/axp20x_battery.c +8 -0
@@ -984,6 +987,22 @@ static void axp717_set_battery_info(struct platform_device *pdev,
        int ccc = info->constant_charge_current_max_ua;
        int val;
+       axp_batt->ts_disable = (device_property_read_bool(axp_batt->dev,
+                                                         "x-powers,no-thermistor"));
+
+       /*
+        * Under rare conditions an incorrectly programmed efuse for
+        * the temp sensor on the PMIC may trigger a fault condition.
+        * Allow users to hard-code if the ts pin is not used to work
+        * around this problem.

AFAICT this function won't get run if there is no "monitored-battery" property under the PMIC node. So if someone were to unfortunately have a dev board that has a PMIC with incorrect efuse values, this won't save them.

This should be exceedingly rare, so I only ask that you mention this in the comment so if someone does unfortunately run into it they will have some clue.

Thanks
ChenYu

Quoted 14 lines
drivers/power/supply/axp20x_battery.c +8 -1
+        */
+       if (axp_batt->ts_disable) {
+               regmap_update_bits(axp_batt->regmap,
+                                  AXP717_TS_PIN_CFG,
+                                  AXP717_TS_PIN_DISABLE,
+                                  AXP717_TS_PIN_DISABLE);
+       }
+
        if (vmin > 0 && axp717_set_voltage_min_design(axp_batt, vmin))
                dev_err(&pdev->dev,
                        "couldn't set voltage_min_design\n");
--

2.43.0

#9 lore raw

PATCH 5/5 arm64: dts: allwinner: rg35xx: Add no-thermistor property for battery

In reply to Chris Morgan

From: Chris Morgan <[email protected]>

Add the property of x-powers,no-thermistor for the battery of the Anbernic RG35XX series of H700 devices.

---
 .../boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-2024.dts      | 1 +
 1 file changed, 1 insertion(+)
arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-2024.dts +1 -0
index a231abf1684a..0c89ccc662a2 100644
@@ -237,6 +237,7 @@ axp_adc: adc {
 		battery_power: battery-power {
 			compatible = "x-powers,axp717-battery-power-supply";
 			monitored-battery = <&battery>;
+			x-powers,no-thermistor;
 		};
 
 		regulators {
Signature · 1 line
2.43.0
#10 lore raw
In reply to Chris Morgan
Quoted 11 lines · Chris Morgan

From: Chris Morgan <[email protected]>

The Anbernic RG35XX devices sometimes fail to charge when the register for the battery temperature sensor is set to the incorrect value either by user error or an incorrectly programmed efuse. Allow users to hard-code if a temperature sensor is not present (which is the case for all Anbernic RGxx series devices) to prevent this issue from causing problems. Additionally, a bug was identified with the handling of PMU faults while this fix was being tested.

Quoted 7 lines

Chris Morgan (5):
power: supply: axp20x_battery: Fix fault handling for AXP717
dt-bindings: power: supply: axp20x-battery: Add x-powers,no-thermistor
mfd: axp20x: AXP717: Add AXP717_TS_PIN_CFG to writeable regs

power: supply: axp20x_battery: Update temp sensor for AXP717 from
device tree

Lee, FYI this power supply patch has a compile time dependency on the mfd patch, due to the new register offset macro.

Chris, in the future, if you are aware of build time dependencies, please mention them in the cover letter. That would help maintainers sort out how to land things.

Thanks
ChenYu

Quoted 12 lines

arm64: dts: allwinner: rg35xx: Add no-thermistor property for battery

 .../x-powers,axp20x-battery-power-supply.yaml | 22 ++++++--
 .../sun50i-h700-anbernic-rg35xx-2024.dts      |  1 +
 drivers/mfd/axp20x.c                          |  2 +-
 drivers/power/supply/axp20x_battery.c         | 50 +++++++++++++------
 include/linux/mfd/axp20x.h                    |  1 +
 5 files changed, 56 insertions(+), 20 deletions(-)

--
2.43.0

#11 lore raw

Re: (subset) [PATCH 0/5] Fix RG35XX Battery Charging Issues

In reply to Chris Morgan
Quoted 11 lines · Chris Morgan

From: Chris Morgan <[email protected]>

The Anbernic RG35XX devices sometimes fail to charge when the register for the battery temperature sensor is set to the incorrect value either by user error or an incorrectly programmed efuse. Allow users to hard-code if a temperature sensor is not present (which is the case for all Anbernic RGxx series devices) to prevent this issue from causing problems. Additionally, a bug was identified with the handling of PMU faults while this fix was being tested.

[...]

Applied, thanks!

[1/5] power: supply: axp20x_battery: Fix fault handling for AXP717

      commit: 98380110bd48fbfd6a798ee11fffff893d36062c

Best regards,

Signature · 1 line
Sebastian Reichel <[email protected]>