Add max77620 charging & low battery support

PATCH Superseded Mark Zhang 4 participants 15 messages Jan 29, 2019 – Feb 12, 2019

Subsystems (3)

Device tree bindings Tegra architecture support Multifunction devices (MFD)

lore

Share & tools

lore

Mark Zhang

author

2019-01-29 08:55 UTC #0 lore raw

v2 cover Add max77620 charging & low battery support

This patch set adds support for max77620 backup battery charging and low battery monitoring.

Changes in v2:
- Add devicetree binding documentation

Mark Zhang (4):
mfd: max77620: Add backup battery charger support
mfd: max77620: add documentation for backup battery charging
mfd: max77620: Add low battery monitor support
mfd: max77620: add documentation for low battery monitoring

 .../devicetree/bindings/mfd/max77620.txt      |  34 +++++
 drivers/mfd/max77620.c                        | 137 +++++++++++++++++-
 2 files changed, 170 insertions(+), 1 deletion(-)

Signature · 1 line

2.19.2

Mark Zhang

author

2019-01-29 08:55 UTC #1 lore raw

PATCH v2 1/4 mfd: max77620: Add backup battery charger support

Add PMIC configurations for backup battery charger, which is a constant voltage and constant current style charger with a series output resistance.

The max77620 register CNFGBBC(addr: 0x04) defines the parameters of backup battery charger. This patch adds support for it.

Signed-off-by Laxman Dewangan <[email protected]>

Signed-off-by Venkat Reddy Talla <[email protected]>

Signed-off-by Mark Zhang <[email protected]>

---
 drivers/mfd/max77620.c | 80 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 80 insertions(+)

drivers/mfd/max77620.c +80 -0

index d8ddd1a6f304..f58143103185 100644
@@ -398,6 +398,82 @@ static int max77620_initialise_fps(struct max77620_chip *chip)
 	return 0;
 }
 
+static int max77620_init_backup_battery_charging(struct max77620_chip *chip)
+{
+	struct device *dev = chip->dev;
+	struct device_node *np;
+	u32 pval;
+	u8 config;
+	int charging_current;
+	int charging_voltage;
+	int resistor;
+	int ret;
+
+	np = of_get_child_by_name(dev->of_node, "backup-battery");
+	if (!np) {
+		dev_info(dev, "Backup battery charging support disabled\n");
+		ret = regmap_update_bits(chip->rmap, MAX77620_REG_CNFGBBC,
+					 MAX77620_CNFGBBC_ENABLE, 0);
+		if (ret < 0)
+			dev_err(dev, "Failed to update CNFGBBC: %d\n", ret);
+		return ret;
+	}
+
+	ret = of_property_read_u32(np,
+			"maxim,backup-battery-charging-current", &pval);
+	charging_current = (!ret) ? pval : 50;
+
+	ret = of_property_read_u32(np,
+			"maxim,backup-battery-charging-voltage", &pval);
+	charging_voltage = (!ret) ? pval : 2500000;
+	charging_voltage /= 1000;
+
+	ret = of_property_read_u32(np,
+			"maxim,backup-battery-output-resister", &pval);
+	resistor = (!ret) ? pval : 1000;
+
+	config = MAX77620_CNFGBBC_ENABLE;
+	if (charging_current <= 50)
+		config |= 0 << MAX77620_CNFGBBC_CURRENT_SHIFT;
+	else if (charging_current <= 100)
+		config |= 3 << MAX77620_CNFGBBC_CURRENT_SHIFT;
+	else if (charging_current <= 200)
+		config |= 0 << MAX77620_CNFGBBC_CURRENT_SHIFT;
+	else if (charging_current <= 400)
+		config |= 3 << MAX77620_CNFGBBC_CURRENT_SHIFT;
+	else if (charging_current <= 600)
+		config |= 1 << MAX77620_CNFGBBC_CURRENT_SHIFT;
+	else
+		config |= 2 << MAX77620_CNFGBBC_CURRENT_SHIFT;
+
+	if (charging_current > 100)
+		config |= MAX77620_CNFGBBC_LOW_CURRENT_DISABLE;
+
+	if (charging_voltage <= 2500)
+		config |= 0 << MAX77620_CNFGBBC_VOLTAGE_SHIFT;
+	else if (charging_voltage <= 3000)
+		config |= 1 << MAX77620_CNFGBBC_VOLTAGE_SHIFT;
+	else if (charging_voltage <= 3300)
+		config |= 2 << MAX77620_CNFGBBC_VOLTAGE_SHIFT;
+	else
+		config |= 3 << MAX77620_CNFGBBC_VOLTAGE_SHIFT;
+
+	if (resistor <= 100)
+		config |= 0 << MAX77620_CNFGBBC_RESISTOR_SHIFT;
+	else if (resistor <= 1000)
+		config |= 1 << MAX77620_CNFGBBC_RESISTOR_SHIFT;
+	else if (resistor <= 3000)
+		config |= 2 << MAX77620_CNFGBBC_RESISTOR_SHIFT;
+	else if (resistor <= 6000)
+		config |= 3 << MAX77620_CNFGBBC_RESISTOR_SHIFT;
+
+	ret = regmap_write(chip->rmap, MAX77620_REG_CNFGBBC, config);
+	if (ret < 0)
+		dev_err(dev, "Reg 0x%02x write failed, %d\n",
+				MAX77620_REG_CNFGBBC, ret);
+	return ret;
+}
+
 static int max77620_read_es_version(struct max77620_chip *chip)
 {
 	unsigned int val;

@@ -483,6 +559,10 @@ static int max77620_probe(struct i2c_client *client,
 	if (ret < 0)
 		return ret;
 
+	ret = max77620_init_backup_battery_charging(chip);
+	if (ret < 0)
+		return ret;
+
 	ret =  devm_mfd_add_devices(chip->dev, PLATFORM_DEVID_NONE,
 				    mfd_cells, n_mfd_cells, NULL, 0,
 				    regmap_irq_get_domain(chip->top_irq_data));

Signature · 1 line

2.19.2

Mark Zhang

author

2019-01-29 08:55 UTC #2 lore raw

PATCH v2 2/4 mfd: max77620: add documentation for backup battery charging

In reply to Mark Zhang

Adding documentation for 3 new backup battery charging dts properties:
- maxim,backup-battery-charging-current
- maxim,backup-battery-charging-voltage
- maxim,backup-battery-output-resister

Signed-off-by Mark Zhang <[email protected]>

---
 .../devicetree/bindings/mfd/max77620.txt      | 20 +++++++++++++++++++
 1 file changed, 20 insertions(+)

Documentation/devicetree/bindings/mfd/max77620.txt +20 -0

index 9c16d51cc15b..484b17e4fba5 100644
@@ -122,6 +122,26 @@ For DT binding details of different sub modules like GPIO, pincontrol,
 regulator, power, please refer respective device-tree binding document
 under their respective sub-system directories.
 
+Backup Battery:
+==============
+This sub-node configure charging backup battery of the device. Device has
+support of charging the backup battery. The subnode name is "backup-battery".
+The property for backup-battery child nodes as:
+Presence of this child node will enable the backup battery charging.
+
+Optional properties:
+	-maxim,backup-battery-charging-current: Charging current setting.
+			The device supports 50/100/200/400/600/800uA.
+			If this property is unavailable then it will
+			charge with 50uA.
+	-maxim,backup-battery-charging-voltage: Charging Voltage Limit Setting.
+			Device supports 2500000/3000000/3300000/350000uV.
+			Default will be set to 2500mV. The voltage will be roundoff
+			to nearest lower side if other than above is configured.
+	-maxim,backup-battery-output-resister: Output resistor on Ohm.
+			Device supports 100/1000/3000/6000 Ohms.
+			Default will be set to 1000 Ohm.
+
 Example:
 --------
 #include <dt-bindings/mfd/max77620.h>

Signature · 1 line

2.19.2

Rob Herring

maintainer

2019-01-30 19:51 UTC #3 lore raw

Re: [PATCH v2 2/4] mfd: max77620: add documentation for backup battery charging

Quoted 28 lines · Mark Zhang

Adding documentation for 3 new backup battery charging dts properties:
- maxim,backup-battery-charging-current
- maxim,backup-battery-charging-voltage
- maxim,backup-battery-output-resister

Signed-off-by Mark Zhang <[email protected]>

---
 .../devicetree/bindings/mfd/max77620.txt      | 20 +++++++++++++++++++
 1 file changed, 20 insertions(+)

Documentation/devicetree/bindings/mfd/max77620.txt +9 -0

index 9c16d51cc15b..484b17e4fba5 100644
@@ -122,6 +122,26 @@ For DT binding details of different sub modules like GPIO, pincontrol,
 regulator, power, please refer respective device-tree binding document
 under their respective sub-system directories.
 
+Backup Battery:
+==============
+This sub-node configure charging backup battery of the device. Device has
+support of charging the backup battery. The subnode name is "backup-battery".
+The property for backup-battery child nodes as:
+Presence of this child node will enable the backup battery charging.
+
+Optional properties:
+	-maxim,backup-battery-charging-current: Charging current setting.

Documentation/devicetree/bindings/mfd/max77620.txt For DT binding details of different sub modules like GPIO, pincontrol, · line 143

Quoted 10 lines

Documentation/devicetree/bindings/mfd/max77620.txt +10 -0

+			The device supports 50/100/200/400/600/800uA.
+			If this property is unavailable then it will
+			charge with 50uA.
+	-maxim,backup-battery-charging-voltage: Charging Voltage Limit Setting.
+			Device supports 2500000/3000000/3300000/350000uV.
+			Default will be set to 2500mV. The voltage will be roundoff
+			to nearest lower side if other than above is configured.
+	-maxim,backup-battery-output-resister: Output resistor on Ohm.
+			Device supports 100/1000/3000/6000 Ohms.
+			Default will be set to 1000 Ohm.

These need property unit suffixes as defined in property-units.txt. The names are kind of long, so perhaps shorten them a bit.

Quoted 7 lines

Documentation/devicetree/bindings/mfd/max77620.txt +1 -0
+
 Example:
 --------
 #include <dt-bindings/mfd/max77620.h>
Signature · 1 line
2.19.2

Mark Zhang

author

2019-01-31 02:26 UTC #4 lore raw

Re: [PATCH v2 2/4] mfd: max77620: add documentation for backup battery charging

Quoted 34 lines · Rob Herring

Quoted 33 lines · Mark Zhang

Adding documentation for 3 new backup battery charging dts properties:
- maxim,backup-battery-charging-current
- maxim,backup-battery-charging-voltage
- maxim,backup-battery-output-resister

Signed-off-by Mark Zhang <[email protected]>

---
 .../devicetree/bindings/mfd/max77620.txt      | 20 +++++++++++++++++++
 1 file changed, 20 insertions(+)

Documentation/devicetree/bindings/mfd/max77620.txt +14 -0

index 9c16d51cc15b..484b17e4fba5 100644
@@ -122,6 +122,26 @@ For DT binding details of different sub modules like GPIO, pincontrol,
 regulator, power, please refer respective device-tree binding document
 under their respective sub-system directories.
 
+Backup Battery:
+==============
+This sub-node configure charging backup battery of the device. Device has
+support of charging the backup battery. The subnode name is "backup-battery".
+The property for backup-battery child nodes as:
+Presence of this child node will enable the backup battery charging.
+
+Optional properties:
+	-maxim,backup-battery-charging-current: Charging current setting.
+			The device supports 50/100/200/400/600/800uA.
+			If this property is unavailable then it will
+			charge with 50uA.
+	-maxim,backup-battery-charging-voltage: Charging Voltage Limit Setting.
+			Device supports 2500000/3000000/3300000/350000uV.

Quoted 9 lines

Documentation/devicetree/bindings/mfd/max77620.txt For DT binding details of different sub modules like GPIO, pincontrol, · line 143
Quoted 5 lines
Documentation/devicetree/bindings/mfd/max77620.txt +5 -0
+			Default will be set to 2500mV. The voltage will be roundoff
+			to nearest lower side if other than above is configured.
+	-maxim,backup-battery-output-resister: Output resistor on Ohm.
+			Device supports 100/1000/3000/6000 Ohms.
+			Default will be set to 1000 Ohm.
These need property unit suffixes as defined in property-units.txt. The names are kind of long, so perhaps shorten them a bit.

Thanks Rob. Given these properties will be under "backup-battery" section, so how about changing them to:
- maxim,charging-current-microamp
- maxim,charging-voltage-microvolt
- maxim,output-resister-ohms

Mark

Quoted 7 lines

Quoted 7 lines
Documentation/devicetree/bindings/mfd/max77620.txt +1 -0
+
 Example:
 --------
 #include <dt-bindings/mfd/max77620.h>
Signature · 1 line
2.19.2

Mark Zhang

author

2019-01-29 08:55 UTC #5 lore raw

PATCH v2 3/4 mfd: max77620: Add low battery monitor support

In reply to Mark Zhang

This patch adds PMIC configurations for low-battery monitoring by handling max77620 register CNFGGLBL1.

Signed-off-by Laxman Dewangan <[email protected]>

Signed-off-by Venkat Reddy Talla <[email protected]>

Signed-off-by Mark Zhang <[email protected]>

---
 drivers/mfd/max77620.c | 57 +++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 56 insertions(+), 1 deletion(-)

drivers/mfd/max77620.c +56 -1

index f58143103185..9e50d145afd8 100644
@@ -474,6 +474,57 @@ static int max77620_init_backup_battery_charging(struct max77620_chip *chip)
 	return ret;
 }
 
+static int max77620_init_low_battery_monitor(struct max77620_chip *chip)
+{
+	struct device *dev = chip->dev;
+	struct device_node *np;
+	bool pval;
+	u8 mask = 0;
+	u8 val = 0;
+	int ret;
+
+	np = of_get_child_by_name(dev->of_node, "low-battery-monitor");
+	if (!np) {
+		dev_info(dev, "Low battery monitoring support disabled\n");
+		return 0;
+	}
+
+	pval = of_property_read_bool(np, "maxim,low-battery-dac-enable");
+	if (pval) {
+		mask |= MAX77620_CNFGGLBL1_LBDAC_EN;
+		val |= MAX77620_CNFGGLBL1_LBDAC_EN;
+	}
+
+	pval = of_property_read_bool(np, "maxim,low-battery-dac-disable");
+	if (pval)
+		mask |= MAX77620_CNFGGLBL1_LBDAC_EN;
+
+	pval = of_property_read_bool(np, "maxim,low-battery-shutdown-enable");
+	if (pval) {
+		mask |= MAX77620_CNFGGLBL1_MPPLD;
+		val |= MAX77620_CNFGGLBL1_MPPLD;
+	}
+
+	pval = of_property_read_bool(np, "maxim,low-battery-shutdown-disable");
+	if (pval)
+		mask |= MAX77620_CNFGGLBL1_MPPLD;
+
+	pval = of_property_read_bool(np, "maxim,low-battery-reset-enable");
+	if (pval) {
+		mask |= MAX77620_CNFGGLBL1_LBRSTEN;
+		val |= MAX77620_CNFGGLBL1_LBRSTEN;
+	}
+
+	pval = of_property_read_bool(np, "maxim,low-battery-reset-disable");
+	if (pval)
+		mask |= MAX77620_CNFGGLBL1_LBRSTEN;
+
+	ret = regmap_update_bits(chip->rmap, MAX77620_REG_CNFGGLBL1, mask, val);
+	if (ret < 0)
+		dev_err(dev, "Reg CNFGGLBL1 update failed: %d\n", ret);
+	return ret;
+}
+
 static int max77620_read_es_version(struct max77620_chip *chip)
 {
 	unsigned int val;

@@ -563,7 +614,11 @@ static int max77620_probe(struct i2c_client *client,
 	if (ret < 0)
 		return ret;
 
-	ret =  devm_mfd_add_devices(chip->dev, PLATFORM_DEVID_NONE,
+	ret = max77620_init_low_battery_monitor(chip);
+	if (ret < 0)
+		return ret;
+
+	ret = devm_mfd_add_devices(chip->dev, PLATFORM_DEVID_NONE,
 				    mfd_cells, n_mfd_cells, NULL, 0,
 				    regmap_irq_get_domain(chip->top_irq_data));
 	if (ret < 0) {

Signature · 1 line

2.19.2

Mark Zhang

author

2019-01-29 08:55 UTC #6 lore raw

PATCH v2 4/4 mfd: max77620: add documentation for low battery monitoring

In reply to Mark Zhang

Adding documentation for low battery monitor properties:
- maxim,low-battery-dac-enable
- maxim,low-battery-dac-disable
- maxim,low-battery-shutdown-enable
- maxim,low-battery-shutdown-disable
- maxim,low-battery-reset-enable
- maxim,low-battery-reset-disable

Signed-off-by Mark Zhang <[email protected]>

---
 Documentation/devicetree/bindings/mfd/max77620.txt | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

Documentation/devicetree/bindings/mfd/max77620.txt +14 -0

index 484b17e4fba5..5fed0a463b80 100644
@@ -142,6 +142,20 @@ Optional properties:
 			Device supports 100/1000/3000/6000 Ohms.
 			Default will be set to 1000 Ohm.
 
+Low-Battery Monitor:
+==================
+This sub-node configure low battery monitor configuration registers. Device has
+support for low-battery monitor configuration through child DT node
+"low-battery-monitor".
+
+Optional properties:
+	- maxim,low-battery-dac-enable: Enable low battery DAC.
+	- maxim,low-battery-dac-disable: Disable low battery DAC.
+	- maxim,low-battery-shutdown-enable: Enable low battery shutdown.
+	- maxim,low-battery-shutdown-disable: Disable low battery shutdown.
+	- maxim,low-battery-reset-enable: Enable low battery reset.
+	- maxim,low-battery-reset-disable: Disable low battery reset.
+
 Example:
 --------
 #include <dt-bindings/mfd/max77620.h>

Signature · 1 line

2.19.2

Rob Herring

maintainer

2019-01-30 19:53 UTC #7 lore raw

Re: [PATCH v2 4/4] mfd: max77620: add documentation for low battery monitoring

Quoted 24 lines · Mark Zhang

Adding documentation for low battery monitor properties:
- maxim,low-battery-dac-enable
- maxim,low-battery-dac-disable
- maxim,low-battery-shutdown-enable
- maxim,low-battery-shutdown-disable
- maxim,low-battery-reset-enable
- maxim,low-battery-reset-disable
Signed-off-by Mark Zhang <[email protected]>
---
 Documentation/devicetree/bindings/mfd/max77620.txt | 14 ++++++++++++++
 1 file changed, 14 insertions(+)
Documentation/devicetree/bindings/mfd/max77620.txt +3 -0
index 484b17e4fba5..5fed0a463b80 100644
@@ -142,6 +142,20 @@ Optional properties:
 			Device supports 100/1000/3000/6000 Ohms.
 			Default will be set to 1000 Ohm.
 
+Low-Battery Monitor:
+==================
+This sub-node configure low battery monitor configuration registers. Device has

Documentation/devicetree/bindings/mfd/max77620.txt Optional properties: · line 157

Quoted 10 lines

Documentation/devicetree/bindings/mfd/max77620.txt +10 -0

+support for low-battery monitor configuration through child DT node
+"low-battery-monitor".
+
+Optional properties:
+	- maxim,low-battery-dac-enable: Enable low battery DAC.
+	- maxim,low-battery-dac-disable: Disable low battery DAC.
+	- maxim,low-battery-shutdown-enable: Enable low battery shutdown.
+	- maxim,low-battery-shutdown-disable: Disable low battery shutdown.
+	- maxim,low-battery-reset-enable: Enable low battery reset.
+	- maxim,low-battery-reset-disable: Disable low battery reset.

Do you really need 3 states with the 3rd being prop not present.

Rob

Mark Zhang

author

2019-01-31 02:29 UTC #8 lore raw

Re: [PATCH v2 4/4] mfd: max77620: add documentation for low battery monitoring

Quoted 22 lines · Rob Herring

Quoted 21 lines · Mark Zhang
Adding documentation for low battery monitor properties:
- maxim,low-battery-dac-enable
- maxim,low-battery-dac-disable
- maxim,low-battery-shutdown-enable
- maxim,low-battery-shutdown-disable
- maxim,low-battery-reset-enable
- maxim,low-battery-reset-disable
Signed-off-by Mark Zhang <[email protected]>
---
 Documentation/devicetree/bindings/mfd/max77620.txt | 14 ++++++++++++++
 1 file changed, 14 insertions(+)
Documentation/devicetree/bindings/mfd/max77620.txt +0 -0
index 484b17e4fba5..5fed0a463b80 100644
@@ -142,6 +142,20 @@ Optional properties:
 			Device supports 100/1000/3000/6000 Ohms.
 			Default will be set to 1000 Ohm.
 

Quoted 15 lines

Documentation/devicetree/bindings/mfd/max77620.txt Optional properties: · line 157

Quoted 13 lines

Documentation/devicetree/bindings/mfd/max77620.txt +13 -0

+Low-Battery Monitor:
+==================
+This sub-node configure low battery monitor configuration registers. Device has
+support for low-battery monitor configuration through child DT node
+"low-battery-monitor".
+
+Optional properties:
+	- maxim,low-battery-dac-enable: Enable low battery DAC.
+	- maxim,low-battery-dac-disable: Disable low battery DAC.
+	- maxim,low-battery-shutdown-enable: Enable low battery shutdown.
+	- maxim,low-battery-shutdown-disable: Disable low battery shutdown.
+	- maxim,low-battery-reset-enable: Enable low battery reset.
+	- maxim,low-battery-reset-disable: Disable low battery reset.

Do you really need 3 states with the 3rd being prop not present.

Yeah, so I think we can just keep 3 of them and shorten the names a little bit (lbm stands for "low battery monitoring"):
- maxim,lbm-dac-enable
- maxim,lbm-shutdown-enable
- maxim,lbm-reset-enable

Does this look good to you?
Mark

Quoted 3 lines

Rob

Rob Herring

maintainer

2019-01-31 21:05 UTC #9 lore raw

Re: [PATCH v2 4/4] mfd: max77620: add documentation for low battery monitoring

Quoted 24 lines · Mark Zhang

Quoted 22 lines · Rob Herring
Quoted 21 lines · Mark Zhang
Adding documentation for low battery monitor properties:
- maxim,low-battery-dac-enable
- maxim,low-battery-dac-disable
- maxim,low-battery-shutdown-enable
- maxim,low-battery-shutdown-disable
- maxim,low-battery-reset-enable
- maxim,low-battery-reset-disable
Signed-off-by Mark Zhang <[email protected]>
---
 Documentation/devicetree/bindings/mfd/max77620.txt | 14 ++++++++++++++
 1 file changed, 14 insertions(+)
Documentation/devicetree/bindings/mfd/max77620.txt +0 -0
index 484b17e4fba5..5fed0a463b80 100644
@@ -142,6 +142,20 @@ Optional properties:
                     Device supports 100/1000/3000/6000 Ohms.
                     Default will be set to 1000 Ohm.
 

Quoted 5 lines

Quoted 5 lines
Documentation/devicetree/bindings/mfd/max77620.txt Optional properties: · line 149
Quoted 5 lines
Documentation/devicetree/bindings/mfd/max77620.txt +5 -0
+Low-Battery Monitor:
+==================
+This sub-node configure low battery monitor configuration registers. Device has
+support for low-battery monitor configuration through child DT node
+"low-battery-monitor".

Missed this the first time, but there's not really any reason for these to be in a child node.

Quoted 11 lines

Quoted 10 lines
Documentation/devicetree/bindings/mfd/max77620.txt Optional properties: · line 157
Quoted 8 lines
Documentation/devicetree/bindings/mfd/max77620.txt +8 -0
+
+Optional properties:
+    - maxim,low-battery-dac-enable: Enable low battery DAC.
+    - maxim,low-battery-dac-disable: Disable low battery DAC.
+    - maxim,low-battery-shutdown-enable: Enable low battery shutdown.
+    - maxim,low-battery-shutdown-disable: Disable low battery shutdown.
+    - maxim,low-battery-reset-enable: Enable low battery reset.
+    - maxim,low-battery-reset-disable: Disable low battery reset.
Do you really need 3 states with the 3rd being prop not present.

Quoted 7 lines

Yeah, so I think we can just keep 3 of them and shorten the names a little bit (lbm stands for "low battery monitoring"):
- maxim,lbm-dac-enable
- maxim,lbm-shutdown-enable
- maxim,lbm-reset-enable
Does this look good to you?

Yes. However, are these 3 mutually exclusive? If so, then perhaps 'maxim,low-battery-mode = "<dac|shutdown|reset>"'?

Rob

Billy laws

2019-01-31 21:37 UTC #10 lore raw

Re: [PATCH v2 4/4] mfd: max77620: add documentation for low battery monitoring

Quoted 58 lines · Rob Herring

Quoted 11 lines · Mark Zhang
Quoted 9 lines · Rob Herring
Quoted 8 lines · Mark Zhang
Adding documentation for low battery monitor properties:
- maxim,low-battery-dac-enable
- maxim,low-battery-dac-disable
- maxim,low-battery-shutdown-enable
- maxim,low-battery-shutdown-disable
- maxim,low-battery-reset-enable
- maxim,low-battery-reset-disable
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Signed-off-by Mark Zhang <[email protected]>
---
 Documentation/devicetree/bindings/mfd/max77620.txt | 14
++++++++++++++
Quoted 3 lines
Quoted 3 lines
Review
Quoted 3 lines
 1 file changed, 14 insertions(+)
(quoted diff) +0 -0
b/Documentation/devicetree/bindings/mfd/max77620.txt
Quoted 10 lines
Quoted 10 lines
Documentation/devicetree/bindings/mfd/max77620.txt Optional properties: · line 147
Quoted 10 lines
index 484b17e4fba5..5fed0a463b80 100644
Documentation/devicetree/bindings/mfd/max77620.txt +3 -0
@@ -142,6 +142,20 @@ Optional properties:
                     Device supports 100/1000/3000/6000 Ohms.
                     Default will be set to 1000 Ohm.
 
+Low-Battery Monitor:
+==================
+This sub-node configure low battery monitor configuration
registers. Device has
Quoted 1 line
Quoted 1 line
Documentation/devicetree/bindings/mfd/max77620.txt Optional properties: · line 148
Quoted 1 line
Documentation/devicetree/bindings/mfd/max77620.txt +1 -0
+support for low-battery monitor configuration through child DT
node
Quoted 1 line
Quoted 1 line
Documentation/devicetree/bindings/mfd/max77620.txt Optional properties: · line 149
Quoted 1 line
Documentation/devicetree/bindings/mfd/max77620.txt +1 -0
+"low-battery-monitor".
Missed this the first time, but there's not really any reason for these to be in a child node.
Quoted 5 lines
Quoted 5 lines
Documentation/devicetree/bindings/mfd/max77620.txt Optional properties:
Quoted 5 lines
Documentation/devicetree/bindings/mfd/max77620.txt +5 -0
+
+Optional properties:
+    - maxim,low-battery-dac-enable: Enable low battery DAC.
+    - maxim,low-battery-dac-disable: Disable low battery DAC.
+    - maxim,low-battery-shutdown-enable: Enable low battery
shutdown.
Quoted 1 line
Quoted 1 line
Documentation/devicetree/bindings/mfd/max77620.txt Optional properties: · line 155
Quoted 1 line
Documentation/devicetree/bindings/mfd/max77620.txt +1 -0
+    - maxim,low-battery-shutdown-disable: Disable low battery
shutdown.
Quoted 6 lines
Quoted 4 lines
Documentation/devicetree/bindings/mfd/max77620.txt Optional properties: · line 157
Quoted 2 lines
Documentation/devicetree/bindings/mfd/max77620.txt +2 -0
+    - maxim,low-battery-reset-enable: Enable low battery reset.
+    - maxim,low-battery-reset-disable: Disable low battery reset.
Do you really need 3 states with the 3rd being prop not present.
Yeah, so I think we can just keep 3 of them and shorten the names a
little bit
Quoted 5 lines
(lbm stands for "low battery monitoring"):
- maxim,lbm-dac-enable
- maxim,lbm-shutdown-enable
- maxim,lbm-reset-enable

Quoted 4 lines

Quoted 1 line
Does this look good to you?
Yes. However, are these 3 mutually exclusive? If so, then perhaps 'maxim,low-battery-mode = "<dac|shutdown|reset>"'?

I agree, would also allow the addition of the voltage cut off point configuration by just adding more defines.(current forces 3.something v cutoff)

Quoted 2 lines

Rob

Lee Jones

2019-02-07 08:48 UTC #11 lore raw

In reply to Mark Zhang

Quoted 6 lines · Mark Zhang

This patch set adds support for max77620 backup battery charging and low battery monitoring.
Changes in v2:
- Add devicetree binding documentation

Quoted 8 lines

Mark Zhang (4):
mfd: max77620: Add backup battery charger support
mfd: max77620: add documentation for backup battery charging
mfd: max77620: Add low battery monitor support
mfd: max77620: add documentation for low battery monitoring
 .../devicetree/bindings/mfd/max77620.txt      |  34 +++++
 drivers/mfd/max77620.c                        | 137 +++++++++++++++++-

All of this needs moving out to the correct subsystem.

drivers/power/supply/max77620-battery.c looks right.

Quoted 1 line

 2 files changed, 170 insertions(+), 1 deletion(-)

Signature · 4 lines

Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Mark Zhang

author

2019-02-12 04:38 UTC #12 lore raw

Quoted 14 lines · Lee Jones

Quoted 12 lines · Mark Zhang
This patch set adds support for max77620 backup battery charging and low battery monitoring.
Changes in v2:
- Add devicetree binding documentation
Mark Zhang (4):
mfd: max77620: Add backup battery charger support
mfd: max77620: add documentation for backup battery charging
mfd: max77620: Add low battery monitor support
mfd: max77620: add documentation for low battery monitoring

Quoted 6 lines

Quoted 2 lines
 .../devicetree/bindings/mfd/max77620.txt      |  34 +++++
 drivers/mfd/max77620.c                        | 137 +++++++++++++++++-
All of this needs moving out to the correct subsystem.
drivers/power/supply/max77620-battery.c looks right.

Actually max77620 is not a power supply device. This patch set adds 2 features:
- Backup battery charger. The RTC in max77620 is supplied from a backup battery and consumes 2.0uA when no other power sources are available. So basically this is not a system battery charger, it's just for RTC.
- Low battery monitoring. This is for monitoring the system main battery voltage, so max77620 can shutdown or reset the SoC accordingly. But I think this doesn't conform the idea of "power supply" driver as well.

Mark

Quoted 3 lines

Quoted 2 lines

 2 files changed, 170 insertions(+), 1 deletion(-)

Lee Jones

2019-02-12 08:04 UTC #13 lore raw

Quoted 22 lines · Mark Zhang

Quoted 14 lines · Lee Jones
Quoted 12 lines · Mark Zhang
This patch set adds support for max77620 backup battery charging and low battery monitoring.
Changes in v2:
- Add devicetree binding documentation
Mark Zhang (4):
mfd: max77620: Add backup battery charger support
mfd: max77620: add documentation for backup battery charging
mfd: max77620: Add low battery monitor support
mfd: max77620: add documentation for low battery monitoring
Quoted 6 lines
Quoted 2 lines
 .../devicetree/bindings/mfd/max77620.txt      |  34 +++++
 drivers/mfd/max77620.c                        | 137 +++++++++++++++++-
All of this needs moving out to the correct subsystem.
drivers/power/supply/max77620-battery.c looks right.

Quoted 8 lines

Actually max77620 is not a power supply device. This patch set adds 2 features:
- Backup battery charger. The RTC in max77620 is supplied from a backup battery and consumes 2.0uA when no other power sources are available. So basically this is not a system battery charger, it's just for RTC.
- Low battery monitoring. This is for monitoring the system main battery voltage, so max77620 can shutdown or reset the SoC accordingly. But I think this doesn't conform the idea of "power supply" driver as well.

Most other battery handling seems to happen in drivers/power/supply/*.battery*. If that's not the right location, then you need to find a place for it to go. MFDs do not provide useful functionality per say - that is the role of the child devices.

Signature · 4 lines

Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Mark Zhang

author

2019-02-12 08:29 UTC #14 lore raw

Quoted 33 lines · Lee Jones

Quoted 22 lines · Mark Zhang
Quoted 14 lines · Lee Jones
Quoted 12 lines · Mark Zhang
This patch set adds support for max77620 backup battery charging and low battery monitoring.
Changes in v2:
- Add devicetree binding documentation
Mark Zhang (4):
mfd: max77620: Add backup battery charger support
mfd: max77620: add documentation for backup battery charging
mfd: max77620: Add low battery monitor support
mfd: max77620: add documentation for low battery monitoring
Quoted 6 lines
Quoted 2 lines
 .../devicetree/bindings/mfd/max77620.txt      |  34 +++++
 drivers/mfd/max77620.c                        | 137 +++++++++++++++++-
All of this needs moving out to the correct subsystem.
drivers/power/supply/max77620-battery.c looks right.
Quoted 8 lines
Actually max77620 is not a power supply device. This patch set adds 2 features:
- Backup battery charger. The RTC in max77620 is supplied from a backup battery and consumes 2.0uA when no other power sources are available. So basically this is not a system battery charger, it's just for RTC.
- Low battery monitoring. This is for monitoring the system main battery voltage, so max77620 can shutdown or reset the SoC accordingly. But I think this doesn't conform the idea of "power supply" driver as well.

Quoted 5 lines

Most other battery handling seems to happen in drivers/power/supply/*.battery*. If that's not the right location, then you need to find a place for it to go. MFDs do not provide useful functionality per say - that is the role of the child devices.

Understood. Looking at "enum power_supply_property", battery handling drivers are able to get battery capacity/current voltage/temperature/status/... and etc, which is making sense. But my patch set doesn't do this kind of things at all so I think the power supply framework doesn't fit here. Let me check other similar device drivers... hope to get inspired. Thanks Lee.

Mark