[PATCH v4 1/7] extcon: max8997: Add CHGINS and CHGRM interrupt handling

[PATCH v4 1/7] extcon: max8997: Add CHGINS and CHGRM interrupt handling

[Timon Baetz]

This allows the MAX8997 charger to set the current limit depending on
the detected extcon charger type.

Signed-off-by: Timon Baetz <timon.baetz@protonmail.com>
---
 drivers/extcon/extcon-max8997.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/extcon/extcon-max8997.c b/drivers/extcon/extcon-max8997.c
index 337b0eea4e62..e1408075ef7d 100644
--- a/drivers/extcon/extcon-max8997.c
+++ b/drivers/extcon/extcon-max8997.c
@@ -44,6 +44,8 @@ static struct max8997_muic_irq muic_irqs[] = {
 	{ MAX8997_MUICIRQ_ChgDetRun,	"muic-CHGDETRUN" },
 	{ MAX8997_MUICIRQ_ChgTyp,	"muic-CHGTYP" },
 	{ MAX8997_MUICIRQ_OVP,		"muic-OVP" },
+	{ MAX8997_PMICIRQ_CHGINS,	"pmic-CHGINS" },
+	{ MAX8997_PMICIRQ_CHGRM,	"pmic-CHGRM" },
 };
 
 /* Define supported cable type */
@@ -538,6 +540,8 @@ static void max8997_muic_irq_work(struct work_struct *work)
 	case MAX8997_MUICIRQ_DCDTmr:
 	case MAX8997_MUICIRQ_ChgDetRun:
 	case MAX8997_MUICIRQ_ChgTyp:
+	case MAX8997_PMICIRQ_CHGINS:
+	case MAX8997_PMICIRQ_CHGRM:
 		/* Handle charger cable */
 		ret = max8997_muic_chg_handler(info);
 		break;
-- 
2.25.1

[PATCH v4 1/7] extcon: max8997: Add CHGINS and CHGRM interrupt handling

[Timon Baetz]

This allows the MAX8997 charger to set the current limit depending on
the detected extcon charger type.

Signed-off-by: Timon Baetz <timon.baetz@protonmail.com>
---
 drivers/extcon/extcon-max8997.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/extcon/extcon-max8997.c b/drivers/extcon/extcon-max8997.c
index 337b0eea4e62..e1408075ef7d 100644
--- a/drivers/extcon/extcon-max8997.c
+++ b/drivers/extcon/extcon-max8997.c
@@ -44,6 +44,8 @@ static struct max8997_muic_irq muic_irqs[] = {
 	{ MAX8997_MUICIRQ_ChgDetRun,	"muic-CHGDETRUN" },
 	{ MAX8997_MUICIRQ_ChgTyp,	"muic-CHGTYP" },
 	{ MAX8997_MUICIRQ_OVP,		"muic-OVP" },
+	{ MAX8997_PMICIRQ_CHGINS,	"pmic-CHGINS" },
+	{ MAX8997_PMICIRQ_CHGRM,	"pmic-CHGRM" },
 };
 
 /* Define supported cable type */
@@ -538,6 +540,8 @@ static void max8997_muic_irq_work(struct work_struct *work)
 	case MAX8997_MUICIRQ_DCDTmr:
 	case MAX8997_MUICIRQ_ChgDetRun:
 	case MAX8997_MUICIRQ_ChgTyp:
+	case MAX8997_PMICIRQ_CHGINS:
+	case MAX8997_PMICIRQ_CHGRM:
 		/* Handle charger cable */
 		ret = max8997_muic_chg_handler(info);
 		break;
-- 
2.25.1



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 2/7] regulator: dt-bindings: Document max8997-pmic nodes

[Timon Baetz]

Add maxim,max8997-battery and maxim,max8997-muic optional nodes.

Signed-off-by: Timon Baetz <timon.baetz@protonmail.com>
---
 .../bindings/regulator/max8997-regulator.txt         | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/Documentation/devicetree/bindings/regulator/max8997-regulator.txt b/Documentation/devicetree/bindings/regulator/max8997-regulator.txt
index 6fe825b8ac1b..faaf2bbf0272 100644
--- a/Documentation/devicetree/bindings/regulator/max8997-regulator.txt
+++ b/Documentation/devicetree/bindings/regulator/max8997-regulator.txt
@@ -53,6 +53,18 @@ Additional properties required if either of the optional properties are used:
 - max8997,pmic-buck125-dvs-gpios: GPIO specifiers for three host gpio's used
   for dvs. The format of the gpio specifier depends in the gpio controller.
 
+Optional nodes:
+- charger: Node for configuring the charger driver.
+  Required properties:
+  - compatible: "maxim,max8997-battery"
+  Optional properties:
+  - extcon: extcon specifier for charging events
+  - charger-supply: regulator node for charging current
+
+- muic: Node used only by extcon consumers.
+  Required properties:
+  - compatible: "maxim,max8997-muic"
+
 Regulators: The regulators of max8997 that have to be instantiated should be
 included in a sub-node named 'regulators'. Regulator nodes included in this
 sub-node should be of the format as listed below.
-- 
2.25.1

[PATCH v4 2/7] regulator: dt-bindings: Document max8997-pmic nodes

[Timon Baetz]

Add maxim,max8997-battery and maxim,max8997-muic optional nodes.

Signed-off-by: Timon Baetz <timon.baetz@protonmail.com>
---
 .../bindings/regulator/max8997-regulator.txt         | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/Documentation/devicetree/bindings/regulator/max8997-regulator.txt b/Documentation/devicetree/bindings/regulator/max8997-regulator.txt
index 6fe825b8ac1b..faaf2bbf0272 100644
--- a/Documentation/devicetree/bindings/regulator/max8997-regulator.txt
+++ b/Documentation/devicetree/bindings/regulator/max8997-regulator.txt
@@ -53,6 +53,18 @@ Additional properties required if either of the optional properties are used:
 - max8997,pmic-buck125-dvs-gpios: GPIO specifiers for three host gpio's used
   for dvs. The format of the gpio specifier depends in the gpio controller.
 
+Optional nodes:
+- charger: Node for configuring the charger driver.
+  Required properties:
+  - compatible: "maxim,max8997-battery"
+  Optional properties:
+  - extcon: extcon specifier for charging events
+  - charger-supply: regulator node for charging current
+
+- muic: Node used only by extcon consumers.
+  Required properties:
+  - compatible: "maxim,max8997-muic"
+
 Regulators: The regulators of max8997 that have to be instantiated should be
 included in a sub-node named 'regulators'. Regulator nodes included in this
 sub-node should be of the format as listed below.
-- 
2.25.1



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 3/7] mfd: max8997: Add of_compatible to extcon and charger mfd_cell

[Timon Baetz]

Add of_compatible ("maxim,max8997-muic") to the mfd_cell to have a
of_node set in the extcon driver.

Add of_compatible ("maxim,max8997-battery") to the mfd_cell to configure
the charger driver.

Signed-off-by: Timon Baetz <timon.baetz@protonmail.com>
---
 drivers/mfd/max8997.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/mfd/max8997.c b/drivers/mfd/max8997.c
index 68d8f2b95287..55d3a6f97783 100644
--- a/drivers/mfd/max8997.c
+++ b/drivers/mfd/max8997.c
@@ -29,9 +29,9 @@
 static const struct mfd_cell max8997_devs[] = {
 	{ .name = "max8997-pmic", },
 	{ .name = "max8997-rtc", },
-	{ .name = "max8997-battery", },
+	{ .name = "max8997-battery", .of_compatible = "maxim,max8997-battery", },
 	{ .name = "max8997-haptic", },
-	{ .name = "max8997-muic", },
+	{ .name = "max8997-muic", .of_compatible = "maxim,max8997-muic", },
 	{ .name = "max8997-led", .id = 1 },
 	{ .name = "max8997-led", .id = 2 },
 };
-- 
2.25.1

[PATCH v4 3/7] mfd: max8997: Add of_compatible to extcon and charger mfd_cell

[Timon Baetz]

Add of_compatible ("maxim,max8997-muic") to the mfd_cell to have a
of_node set in the extcon driver.

Add of_compatible ("maxim,max8997-battery") to the mfd_cell to configure
the charger driver.

Signed-off-by: Timon Baetz <timon.baetz@protonmail.com>
---
 drivers/mfd/max8997.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/mfd/max8997.c b/drivers/mfd/max8997.c
index 68d8f2b95287..55d3a6f97783 100644
--- a/drivers/mfd/max8997.c
+++ b/drivers/mfd/max8997.c
@@ -29,9 +29,9 @@
 static const struct mfd_cell max8997_devs[] = {
 	{ .name = "max8997-pmic", },
 	{ .name = "max8997-rtc", },
-	{ .name = "max8997-battery", },
+	{ .name = "max8997-battery", .of_compatible = "maxim,max8997-battery", },
 	{ .name = "max8997-haptic", },
-	{ .name = "max8997-muic", },
+	{ .name = "max8997-muic", .of_compatible = "maxim,max8997-muic", },
 	{ .name = "max8997-led", .id = 1 },
 	{ .name = "max8997-led", .id = 2 },
 };
-- 
2.25.1



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 4/7] power: supply: max8997_charger: Set CHARGER current limit

[Timon Baetz]

Register for extcon notification and set charging current depending on
the detected cable type. Current values are taken from vendor kernel,
where most charger types end up setting 650mA [0].

Also enable and disable the CHARGER regulator based on extcon events.

[0] https://github.com/krzk/linux-vendor-backup/blob/samsung/galaxy-s2-epic-4g-touch-sph-d710-exynos4210-dump/drivers/misc/max8997-muic.c#L1675-L1678

Signed-off-by: Timon Baetz <timon.baetz@protonmail.com>
---
 drivers/power/supply/max8997_charger.c | 89 ++++++++++++++++++++++++++
 1 file changed, 89 insertions(+)

diff --git a/drivers/power/supply/max8997_charger.c b/drivers/power/supply/max8997_charger.c
index 1947af25879a..e8532e2af451 100644
--- a/drivers/power/supply/max8997_charger.c
+++ b/drivers/power/supply/max8997_charger.c
@@ -6,12 +6,14 @@
 //  MyungJoo Ham <myungjoo.ham@samsung.com>
 
 #include <linux/err.h>
+#include <linux/extcon.h>
 #include <linux/module.h>
 #include <linux/slab.h>
 #include <linux/platform_device.h>
 #include <linux/power_supply.h>
 #include <linux/mfd/max8997.h>
 #include <linux/mfd/max8997-private.h>
+#include <linux/regulator/consumer.h>
 
 /* MAX8997_REG_STATUS4 */
 #define DCINOK_SHIFT		1
@@ -31,6 +33,10 @@ struct charger_data {
 	struct device *dev;
 	struct max8997_dev *iodev;
 	struct power_supply *battery;
+	struct regulator *reg;
+	struct extcon_dev *edev;
+	struct notifier_block extcon_nb;
+	struct work_struct extcon_work;
 };
 
 static enum power_supply_property max8997_battery_props[] = {
@@ -88,6 +94,67 @@ static int max8997_battery_get_property(struct power_supply *psy,
 	return 0;
 }
 
+static void max8997_battery_extcon_evt_stop_work(void *data)
+{
+	struct charger_data *charger = data;
+
+	cancel_work_sync(&charger->extcon_work);
+}
+
+static void max8997_battery_extcon_evt_worker(struct work_struct *work)
+{
+	struct charger_data *charger =
+	    container_of(work, struct charger_data, extcon_work);
+	struct extcon_dev *edev = charger->edev;
+	int current_limit;
+
+	if (extcon_get_state(edev, EXTCON_CHG_USB_SDP) > 0) {
+		dev_dbg(charger->dev, "USB SDP charger is connected\n");
+		current_limit = 450000;
+	} else if (extcon_get_state(edev, EXTCON_CHG_USB_DCP) > 0) {
+		dev_dbg(charger->dev, "USB DCP charger is connected\n");
+		current_limit = 650000;
+	} else if (extcon_get_state(edev, EXTCON_CHG_USB_FAST) > 0) {
+		dev_dbg(charger->dev, "USB FAST charger is connected\n");
+		current_limit = 650000;
+	} else if (extcon_get_state(edev, EXTCON_CHG_USB_SLOW) > 0) {
+		dev_dbg(charger->dev, "USB SLOW charger is connected\n");
+		current_limit = 650000;
+	} else if (extcon_get_state(edev, EXTCON_CHG_USB_CDP) > 0) {
+		dev_dbg(charger->dev, "USB CDP charger is connected\n");
+		current_limit = 650000;
+	} else {
+		dev_dbg(charger->dev, "USB charger is diconnected\n");
+		current_limit = -1;
+	}
+
+	if (current_limit > 0) {
+		int ret = regulator_set_current_limit(charger->reg, current_limit, current_limit);
+
+		if (ret) {
+			dev_err(charger->dev, "failed to set current limit: %d\n", ret);
+			return;
+		}
+		ret = regulator_enable(charger->reg);
+		if (ret)
+			dev_err(charger->dev, "failed to enable regulator: %d\n", ret);
+	} else {
+		int ret  = regulator_disable(charger->reg);
+
+		if (ret)
+			dev_err(charger->dev, "failed to disable regulator: %d\n", ret);
+	}
+}
+
+static int max8997_battery_extcon_evt(struct notifier_block *nb,
+				unsigned long event, void *param)
+{
+	struct charger_data *charger =
+		container_of(nb, struct charger_data, extcon_nb);
+	schedule_work(&charger->extcon_work);
+	return NOTIFY_OK;
+}
+
 static const struct power_supply_desc max8997_battery_desc = {
 	.name		= "max8997_pmic",
 	.type		= POWER_SUPPLY_TYPE_BATTERY,
@@ -170,6 +237,28 @@ static int max8997_battery_probe(struct platform_device *pdev)
 		return PTR_ERR(charger->battery);
 	}
 
+	charger->reg = devm_regulator_get(&pdev->dev, "charger");
+	charger->edev = extcon_get_edev_by_phandle(&pdev->dev, 0);
+	if (PTR_ERR(charger->reg) == -EPROBE_DEFER ||
+	    PTR_ERR(charger->edev) == -EPROBE_DEFER)
+		return -EPROBE_DEFER;
+
+	if (!IS_ERR(charger->reg) && !IS_ERR(charger->edev)) {
+		INIT_WORK(&charger->extcon_work, max8997_battery_extcon_evt_worker);
+		ret = devm_add_action(&pdev->dev, max8997_battery_extcon_evt_stop_work, charger);
+		if (ret) {
+			dev_err(&pdev->dev, "failed to add extcon evt stop action: %d\n", ret);
+			return ret;
+		}
+		charger->extcon_nb.notifier_call = max8997_battery_extcon_evt;
+		ret = devm_extcon_register_notifier_all(&pdev->dev, charger->edev,
+							&charger->extcon_nb);
+		if (ret) {
+			dev_err(&pdev->dev, "failed to register extcon notifier\n");
+			return ret;
+		};
+	}
+
 	return 0;
 }
 
-- 
2.25.1

[PATCH v4 4/7] power: supply: max8997_charger: Set CHARGER current limit

[Timon Baetz]

Register for extcon notification and set charging current depending on
the detected cable type. Current values are taken from vendor kernel,
where most charger types end up setting 650mA [0].

Also enable and disable the CHARGER regulator based on extcon events.

[0] https://github.com/krzk/linux-vendor-backup/blob/samsung/galaxy-s2-epic-4g-touch-sph-d710-exynos4210-dump/drivers/misc/max8997-muic.c#L1675-L1678

Signed-off-by: Timon Baetz <timon.baetz@protonmail.com>
---
 drivers/power/supply/max8997_charger.c | 89 ++++++++++++++++++++++++++
 1 file changed, 89 insertions(+)

diff --git a/drivers/power/supply/max8997_charger.c b/drivers/power/supply/max8997_charger.c
index 1947af25879a..e8532e2af451 100644
--- a/drivers/power/supply/max8997_charger.c
+++ b/drivers/power/supply/max8997_charger.c
@@ -6,12 +6,14 @@
 //  MyungJoo Ham <myungjoo.ham@samsung.com>
 
 #include <linux/err.h>
+#include <linux/extcon.h>
 #include <linux/module.h>
 #include <linux/slab.h>
 #include <linux/platform_device.h>
 #include <linux/power_supply.h>
 #include <linux/mfd/max8997.h>
 #include <linux/mfd/max8997-private.h>
+#include <linux/regulator/consumer.h>
 
 /* MAX8997_REG_STATUS4 */
 #define DCINOK_SHIFT		1
@@ -31,6 +33,10 @@ struct charger_data {
 	struct device *dev;
 	struct max8997_dev *iodev;
 	struct power_supply *battery;
+	struct regulator *reg;
+	struct extcon_dev *edev;
+	struct notifier_block extcon_nb;
+	struct work_struct extcon_work;
 };
 
 static enum power_supply_property max8997_battery_props[] = {
@@ -88,6 +94,67 @@ static int max8997_battery_get_property(struct power_supply *psy,
 	return 0;
 }
 
+static void max8997_battery_extcon_evt_stop_work(void *data)
+{
+	struct charger_data *charger = data;
+
+	cancel_work_sync(&charger->extcon_work);
+}
+
+static void max8997_battery_extcon_evt_worker(struct work_struct *work)
+{
+	struct charger_data *charger =
+	    container_of(work, struct charger_data, extcon_work);
+	struct extcon_dev *edev = charger->edev;
+	int current_limit;
+
+	if (extcon_get_state(edev, EXTCON_CHG_USB_SDP) > 0) {
+		dev_dbg(charger->dev, "USB SDP charger is connected\n");
+		current_limit = 450000;
+	} else if (extcon_get_state(edev, EXTCON_CHG_USB_DCP) > 0) {
+		dev_dbg(charger->dev, "USB DCP charger is connected\n");
+		current_limit = 650000;
+	} else if (extcon_get_state(edev, EXTCON_CHG_USB_FAST) > 0) {
+		dev_dbg(charger->dev, "USB FAST charger is connected\n");
+		current_limit = 650000;
+	} else if (extcon_get_state(edev, EXTCON_CHG_USB_SLOW) > 0) {
+		dev_dbg(charger->dev, "USB SLOW charger is connected\n");
+		current_limit = 650000;
+	} else if (extcon_get_state(edev, EXTCON_CHG_USB_CDP) > 0) {
+		dev_dbg(charger->dev, "USB CDP charger is connected\n");
+		current_limit = 650000;
+	} else {
+		dev_dbg(charger->dev, "USB charger is diconnected\n");
+		current_limit = -1;
+	}
+
+	if (current_limit > 0) {
+		int ret = regulator_set_current_limit(charger->reg, current_limit, current_limit);
+
+		if (ret) {
+			dev_err(charger->dev, "failed to set current limit: %d\n", ret);
+			return;
+		}
+		ret = regulator_enable(charger->reg);
+		if (ret)
+			dev_err(charger->dev, "failed to enable regulator: %d\n", ret);
+	} else {
+		int ret  = regulator_disable(charger->reg);
+
+		if (ret)
+			dev_err(charger->dev, "failed to disable regulator: %d\n", ret);
+	}
+}
+
+static int max8997_battery_extcon_evt(struct notifier_block *nb,
+				unsigned long event, void *param)
+{
+	struct charger_data *charger =
+		container_of(nb, struct charger_data, extcon_nb);
+	schedule_work(&charger->extcon_work);
+	return NOTIFY_OK;
+}
+
 static const struct power_supply_desc max8997_battery_desc = {
 	.name		= "max8997_pmic",
 	.type		= POWER_SUPPLY_TYPE_BATTERY,
@@ -170,6 +237,28 @@ static int max8997_battery_probe(struct platform_device *pdev)
 		return PTR_ERR(charger->battery);
 	}
 
+	charger->reg = devm_regulator_get(&pdev->dev, "charger");
+	charger->edev = extcon_get_edev_by_phandle(&pdev->dev, 0);
+	if (PTR_ERR(charger->reg) == -EPROBE_DEFER ||
+	    PTR_ERR(charger->edev) == -EPROBE_DEFER)
+		return -EPROBE_DEFER;
+
+	if (!IS_ERR(charger->reg) && !IS_ERR(charger->edev)) {
+		INIT_WORK(&charger->extcon_work, max8997_battery_extcon_evt_worker);
+		ret = devm_add_action(&pdev->dev, max8997_battery_extcon_evt_stop_work, charger);
+		if (ret) {
+			dev_err(&pdev->dev, "failed to add extcon evt stop action: %d\n", ret);
+			return ret;
+		}
+		charger->extcon_nb.notifier_call = max8997_battery_extcon_evt;
+		ret = devm_extcon_register_notifier_all(&pdev->dev, charger->edev,
+							&charger->extcon_nb);
+		if (ret) {
+			dev_err(&pdev->dev, "failed to register extcon notifier\n");
+			return ret;
+		};
+	}
+
 	return 0;
 }
 
-- 
2.25.1



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 5/7] ARM: dts: exynos: Added muic and charger nodes for i9100

[Timon Baetz]

muic node is only used for extcon consumers.
charger node is used to specify muic and regulator.

Signed-off-by: Timon Baetz <timon.baetz@protonmail.com>
---
 arch/arm/boot/dts/exynos4210-i9100.dts | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/arch/arm/boot/dts/exynos4210-i9100.dts b/arch/arm/boot/dts/exynos4210-i9100.dts
index 5370ee477186..8fa704babd5e 100644
--- a/arch/arm/boot/dts/exynos4210-i9100.dts
+++ b/arch/arm/boot/dts/exynos4210-i9100.dts
@@ -584,6 +584,16 @@ EN32KHZ_CP {
 				regulator-always-on;
 			};
 		};
+
+		muic: max8997-muic {
+			compatible = "maxim,max8997-muic";
+		};
+
+		charger {
+			compatible = "maxim,max8997-battery";
+			charger-supply = <&charger_reg>;
+			extcon = <&muic>;
+		};
 	};
 };
 
-- 
2.25.1

[PATCH v4 5/7] ARM: dts: exynos: Added muic and charger nodes for i9100

[Timon Baetz]

muic node is only used for extcon consumers.
charger node is used to specify muic and regulator.

Signed-off-by: Timon Baetz <timon.baetz@protonmail.com>
---
 arch/arm/boot/dts/exynos4210-i9100.dts | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/arch/arm/boot/dts/exynos4210-i9100.dts b/arch/arm/boot/dts/exynos4210-i9100.dts
index 5370ee477186..8fa704babd5e 100644
--- a/arch/arm/boot/dts/exynos4210-i9100.dts
+++ b/arch/arm/boot/dts/exynos4210-i9100.dts
@@ -584,6 +584,16 @@ EN32KHZ_CP {
 				regulator-always-on;
 			};
 		};
+
+		muic: max8997-muic {
+			compatible = "maxim,max8997-muic";
+		};
+
+		charger {
+			compatible = "maxim,max8997-battery";
+			charger-supply = <&charger_reg>;
+			extcon = <&muic>;
+		};
 	};
 };
 
-- 
2.25.1



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 6/7] ARM: dts: exynos: Fix charging regulator voltage and current for i9100

[Timon Baetz]

Set CHARGER current and CHARGER_CV voltage according to Galaxy S2 Epic
4G Touch SPH-D710 Android vendor sources [0,1].

Remove regulator-always-on. The regulator can be enabled and disabled
based on extcon events.

[0] https://github.com/krzk/linux-vendor-backup/blob/samsung/galaxy-s2-epic-4g-touch-sph-d710-exynos4210-dump/drivers/power/max8997_charger_u1.c#L169-L170
[1] https://github.com/krzk/linux-vendor-backup/blob/samsung/galaxy-s2-epic-4g-touch-sph-d710-exynos4210-dump/drivers/power/max8997_charger_u1.c#L390-L391

Signed-off-by: Timon Baetz <timon.baetz@protonmail.com>
---
 arch/arm/boot/dts/exynos4210-i9100.dts | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/arch/arm/boot/dts/exynos4210-i9100.dts b/arch/arm/boot/dts/exynos4210-i9100.dts
index 8fa704babd5e..586d801af0b5 100644
--- a/arch/arm/boot/dts/exynos4210-i9100.dts
+++ b/arch/arm/boot/dts/exynos4210-i9100.dts
@@ -562,15 +562,14 @@ safe2_sreg: ESAFEOUT2 {
 
 			charger_reg: CHARGER {
 				regulator-name = "CHARGER";
-				regulator-min-microamp = <60000>;
-				regulator-max-microamp = <2580000>;
-				regulator-always-on;
+				regulator-min-microamp = <200000>;
+				regulator-max-microamp = <950000>;
 			};
 
 			chargercv_reg: CHARGER_CV {
 				regulator-name = "CHARGER_CV";
-				regulator-min-microvolt = <3800000>;
-				regulator-max-microvolt = <4100000>;
+				regulator-min-microvolt = <4200000>;
+				regulator-max-microvolt = <4200000>;
 				regulator-always-on;
 			};
 
-- 
2.25.1

[PATCH v4 6/7] ARM: dts: exynos: Fix charging regulator voltage and current for i9100

[Timon Baetz]

Set CHARGER current and CHARGER_CV voltage according to Galaxy S2 Epic
4G Touch SPH-D710 Android vendor sources [0,1].

Remove regulator-always-on. The regulator can be enabled and disabled
based on extcon events.

[0] https://github.com/krzk/linux-vendor-backup/blob/samsung/galaxy-s2-epic-4g-touch-sph-d710-exynos4210-dump/drivers/power/max8997_charger_u1.c#L169-L170
[1] https://github.com/krzk/linux-vendor-backup/blob/samsung/galaxy-s2-epic-4g-touch-sph-d710-exynos4210-dump/drivers/power/max8997_charger_u1.c#L390-L391

Signed-off-by: Timon Baetz <timon.baetz@protonmail.com>
---
 arch/arm/boot/dts/exynos4210-i9100.dts | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/arch/arm/boot/dts/exynos4210-i9100.dts b/arch/arm/boot/dts/exynos4210-i9100.dts
index 8fa704babd5e..586d801af0b5 100644
--- a/arch/arm/boot/dts/exynos4210-i9100.dts
+++ b/arch/arm/boot/dts/exynos4210-i9100.dts
@@ -562,15 +562,14 @@ safe2_sreg: ESAFEOUT2 {
 
 			charger_reg: CHARGER {
 				regulator-name = "CHARGER";
-				regulator-min-microamp = <60000>;
-				regulator-max-microamp = <2580000>;
-				regulator-always-on;
+				regulator-min-microamp = <200000>;
+				regulator-max-microamp = <950000>;
 			};
 
 			chargercv_reg: CHARGER_CV {
 				regulator-name = "CHARGER_CV";
-				regulator-min-microvolt = <3800000>;
-				regulator-max-microvolt = <4100000>;
+				regulator-min-microvolt = <4200000>;
+				regulator-max-microvolt = <4200000>;
 				regulator-always-on;
 			};
 
-- 
2.25.1



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 7/7] ARM: dts: exynos: Add top-off charging regulator node for i9100

[Timon Baetz]

Value taken from Galaxy S2 Epic 4G Touch SPH-D710 Android vendor
kernel [0] which always sets 200mA.

Also rearrange regulators based on definition in max8997.h.

[0] https://github.com/krzk/linux-vendor-backup/blob/samsung/galaxy-s2-epic-4g-touch-sph-d710-exynos4210-dump/drivers/power/sec_battery_u1.c#L1525

Signed-off-by: Timon Baetz <timon.baetz@protonmail.com>
---
 arch/arm/boot/dts/exynos4210-i9100.dts | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/arch/arm/boot/dts/exynos4210-i9100.dts b/arch/arm/boot/dts/exynos4210-i9100.dts
index 586d801af0b5..e702adb69670 100644
--- a/arch/arm/boot/dts/exynos4210-i9100.dts
+++ b/arch/arm/boot/dts/exynos4210-i9100.dts
@@ -560,6 +560,16 @@ safe2_sreg: ESAFEOUT2 {
 				regulator-boot-on;
 			};
 
+			EN32KHZ_AP {
+				regulator-name = "EN32KHZ_AP";
+				regulator-always-on;
+			};
+
+			EN32KHZ_CP {
+				regulator-name = "EN32KHZ_CP";
+				regulator-always-on;
+			};
+
 			charger_reg: CHARGER {
 				regulator-name = "CHARGER";
 				regulator-min-microamp = <200000>;
@@ -573,13 +583,10 @@ chargercv_reg: CHARGER_CV {
 				regulator-always-on;
 			};
 
-			EN32KHZ_AP {
-				regulator-name = "EN32KHZ_AP";
-				regulator-always-on;
-			};
-
-			EN32KHZ_CP {
-				regulator-name = "EN32KHZ_CP";
+			CHARGER_TOPOFF {
+				regulator-name = "CHARGER_TOPOFF";
+				regulator-min-microamp = <200000>;
+				regulator-max-microamp = <200000>;
 				regulator-always-on;
 			};
 		};
-- 
2.25.1

[PATCH v4 7/7] ARM: dts: exynos: Add top-off charging regulator node for i9100

[Timon Baetz]

Value taken from Galaxy S2 Epic 4G Touch SPH-D710 Android vendor
kernel [0] which always sets 200mA.

Also rearrange regulators based on definition in max8997.h.

[0] https://github.com/krzk/linux-vendor-backup/blob/samsung/galaxy-s2-epic-4g-touch-sph-d710-exynos4210-dump/drivers/power/sec_battery_u1.c#L1525

Signed-off-by: Timon Baetz <timon.baetz@protonmail.com>
---
 arch/arm/boot/dts/exynos4210-i9100.dts | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/arch/arm/boot/dts/exynos4210-i9100.dts b/arch/arm/boot/dts/exynos4210-i9100.dts
index 586d801af0b5..e702adb69670 100644
--- a/arch/arm/boot/dts/exynos4210-i9100.dts
+++ b/arch/arm/boot/dts/exynos4210-i9100.dts
@@ -560,6 +560,16 @@ safe2_sreg: ESAFEOUT2 {
 				regulator-boot-on;
 			};
 
+			EN32KHZ_AP {
+				regulator-name = "EN32KHZ_AP";
+				regulator-always-on;
+			};
+
+			EN32KHZ_CP {
+				regulator-name = "EN32KHZ_CP";
+				regulator-always-on;
+			};
+
 			charger_reg: CHARGER {
 				regulator-name = "CHARGER";
 				regulator-min-microamp = <200000>;
@@ -573,13 +583,10 @@ chargercv_reg: CHARGER_CV {
 				regulator-always-on;
 			};
 
-			EN32KHZ_AP {
-				regulator-name = "EN32KHZ_AP";
-				regulator-always-on;
-			};
-
-			EN32KHZ_CP {
-				regulator-name = "EN32KHZ_CP";
+			CHARGER_TOPOFF {
+				regulator-name = "CHARGER_TOPOFF";
+				regulator-min-microamp = <200000>;
+				regulator-max-microamp = <200000>;
 				regulator-always-on;
 			};
 		};
-- 
2.25.1



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 3/7] mfd: max8997: Add of_compatible to extcon and charger mfd_cell

[Lee Jones]

On Wed, 23 Dec 2020, Timon Baetz wrote:

> Add of_compatible ("maxim,max8997-muic") to the mfd_cell to have a
> of_node set in the extcon driver.
> 
> Add of_compatible ("maxim,max8997-battery") to the mfd_cell to configure
> the charger driver.
> 
> Signed-off-by: Timon Baetz <timon.baetz@protonmail.com>
> ---
>  drivers/mfd/max8997.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Why have you resent this?  It's already applied.

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

Re: [PATCH v4 3/7] mfd: max8997: Add of_compatible to extcon and charger mfd_cell

[Lee Jones]

On Wed, 23 Dec 2020, Timon Baetz wrote:

> Add of_compatible ("maxim,max8997-muic") to the mfd_cell to have a
> of_node set in the extcon driver.
> 
> Add of_compatible ("maxim,max8997-battery") to the mfd_cell to configure
> the charger driver.
> 
> Signed-off-by: Timon Baetz <timon.baetz@protonmail.com>
> ---
>  drivers/mfd/max8997.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Why have you resent this?  It's already applied.

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 2/7] regulator: dt-bindings: Document max8997-pmic nodes

[Krzysztof Kozlowski]

On Wed, Dec 23, 2020 at 01:42:43PM +0000, Timon Baetz wrote:
> Add maxim,max8997-battery and maxim,max8997-muic optional nodes.
> 
> Signed-off-by: Timon Baetz <timon.baetz@protonmail.com>

I already acked this, why did you skip my tag?

Best regards,
Krzysztof


> ---
>  .../bindings/regulator/max8997-regulator.txt         | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/regulator/max8997-regulator.txt b/Documentation/devicetree/bindings/regulator/max8997-regulator.txt
> index 6fe825b8ac1b..faaf2bbf0272 100644
> --- a/Documentation/devicetree/bindings/regulator/max8997-regulator.txt
> +++ b/Documentation/devicetree/bindings/regulator/max8997-regulator.txt
> @@ -53,6 +53,18 @@ Additional properties required if either of the optional properties are used:
>  - max8997,pmic-buck125-dvs-gpios: GPIO specifiers for three host gpio's used
>    for dvs. The format of the gpio specifier depends in the gpio controller.
>  
> +Optional nodes:
> +- charger: Node for configuring the charger driver.
> +  Required properties:
> +  - compatible: "maxim,max8997-battery"
> +  Optional properties:
> +  - extcon: extcon specifier for charging events
> +  - charger-supply: regulator node for charging current
> +
> +- muic: Node used only by extcon consumers.
> +  Required properties:
> +  - compatible: "maxim,max8997-muic"
> +
>  Regulators: The regulators of max8997 that have to be instantiated should be
>  included in a sub-node named 'regulators'. Regulator nodes included in this
>  sub-node should be of the format as listed below.
> -- 
> 2.25.1
> 
> 

Re: [PATCH v4 2/7] regulator: dt-bindings: Document max8997-pmic nodes

[Krzysztof Kozlowski]

On Wed, Dec 23, 2020 at 01:42:43PM +0000, Timon Baetz wrote:
> Add maxim,max8997-battery and maxim,max8997-muic optional nodes.
> 
> Signed-off-by: Timon Baetz <timon.baetz@protonmail.com>

I already acked this, why did you skip my tag?

Best regards,
Krzysztof


> ---
>  .../bindings/regulator/max8997-regulator.txt         | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/regulator/max8997-regulator.txt b/Documentation/devicetree/bindings/regulator/max8997-regulator.txt
> index 6fe825b8ac1b..faaf2bbf0272 100644
> --- a/Documentation/devicetree/bindings/regulator/max8997-regulator.txt
> +++ b/Documentation/devicetree/bindings/regulator/max8997-regulator.txt
> @@ -53,6 +53,18 @@ Additional properties required if either of the optional properties are used:
>  - max8997,pmic-buck125-dvs-gpios: GPIO specifiers for three host gpio's used
>    for dvs. The format of the gpio specifier depends in the gpio controller.
>  
> +Optional nodes:
> +- charger: Node for configuring the charger driver.
> +  Required properties:
> +  - compatible: "maxim,max8997-battery"
> +  Optional properties:
> +  - extcon: extcon specifier for charging events
> +  - charger-supply: regulator node for charging current
> +
> +- muic: Node used only by extcon consumers.
> +  Required properties:
> +  - compatible: "maxim,max8997-muic"
> +
>  Regulators: The regulators of max8997 that have to be instantiated should be
>  included in a sub-node named 'regulators'. Regulator nodes included in this
>  sub-node should be of the format as listed below.
> -- 
> 2.25.1
> 
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 4/7] power: supply: max8997_charger: Set CHARGER current limit

[Krzysztof Kozlowski]

On Wed, Dec 23, 2020 at 01:43:05PM +0000, Timon Baetz wrote:
> Register for extcon notification and set charging current depending on
> the detected cable type. Current values are taken from vendor kernel,
> where most charger types end up setting 650mA [0].
> 
> Also enable and disable the CHARGER regulator based on extcon events.
> 
> [0] https://github.com/krzk/linux-vendor-backup/blob/samsung/galaxy-s2-epic-4g-touch-sph-d710-exynos4210-dump/drivers/misc/max8997-muic.c#L1675-L1678
> 
> Signed-off-by: Timon Baetz <timon.baetz@protonmail.com>
> ---
>  drivers/power/supply/max8997_charger.c | 89 ++++++++++++++++++++++++++
>  1 file changed, 89 insertions(+)
> 
> diff --git a/drivers/power/supply/max8997_charger.c b/drivers/power/supply/max8997_charger.c
> index 1947af25879a..e8532e2af451 100644
> --- a/drivers/power/supply/max8997_charger.c
> +++ b/drivers/power/supply/max8997_charger.c
> @@ -6,12 +6,14 @@
>  //  MyungJoo Ham <myungjoo.ham@samsung.com>
>  
>  #include <linux/err.h>
> +#include <linux/extcon.h>
>  #include <linux/module.h>
>  #include <linux/slab.h>
>  #include <linux/platform_device.h>
>  #include <linux/power_supply.h>
>  #include <linux/mfd/max8997.h>
>  #include <linux/mfd/max8997-private.h>
> +#include <linux/regulator/consumer.h>
>  
>  /* MAX8997_REG_STATUS4 */
>  #define DCINOK_SHIFT		1
> @@ -31,6 +33,10 @@ struct charger_data {
>  	struct device *dev;
>  	struct max8997_dev *iodev;
>  	struct power_supply *battery;
> +	struct regulator *reg;
> +	struct extcon_dev *edev;
> +	struct notifier_block extcon_nb;
> +	struct work_struct extcon_work;
>  };
>  
>  static enum power_supply_property max8997_battery_props[] = {
> @@ -88,6 +94,67 @@ static int max8997_battery_get_property(struct power_supply *psy,
>  	return 0;
>  }
>  
> +static void max8997_battery_extcon_evt_stop_work(void *data)
> +{
> +	struct charger_data *charger = data;
> +
> +	cancel_work_sync(&charger->extcon_work);
> +}
> +
> +static void max8997_battery_extcon_evt_worker(struct work_struct *work)
> +{
> +	struct charger_data *charger =
> +	    container_of(work, struct charger_data, extcon_work);
> +	struct extcon_dev *edev = charger->edev;
> +	int current_limit;
> +
> +	if (extcon_get_state(edev, EXTCON_CHG_USB_SDP) > 0) {
> +		dev_dbg(charger->dev, "USB SDP charger is connected\n");
> +		current_limit = 450000;
> +	} else if (extcon_get_state(edev, EXTCON_CHG_USB_DCP) > 0) {
> +		dev_dbg(charger->dev, "USB DCP charger is connected\n");
> +		current_limit = 650000;
> +	} else if (extcon_get_state(edev, EXTCON_CHG_USB_FAST) > 0) {
> +		dev_dbg(charger->dev, "USB FAST charger is connected\n");
> +		current_limit = 650000;
> +	} else if (extcon_get_state(edev, EXTCON_CHG_USB_SLOW) > 0) {
> +		dev_dbg(charger->dev, "USB SLOW charger is connected\n");
> +		current_limit = 650000;
> +	} else if (extcon_get_state(edev, EXTCON_CHG_USB_CDP) > 0) {
> +		dev_dbg(charger->dev, "USB CDP charger is connected\n");
> +		current_limit = 650000;
> +	} else {
> +		dev_dbg(charger->dev, "USB charger is diconnected\n");
> +		current_limit = -1;
> +	}
> +
> +	if (current_limit > 0) {
> +		int ret = regulator_set_current_limit(charger->reg, current_limit, current_limit);
> +
> +		if (ret) {
> +			dev_err(charger->dev, "failed to set current limit: %d\n", ret);
> +			return;
> +		}
> +		ret = regulator_enable(charger->reg);
> +		if (ret)
> +			dev_err(charger->dev, "failed to enable regulator: %d\n", ret);
> +	} else {
> +		int ret  = regulator_disable(charger->reg);
> +
> +		if (ret)
> +			dev_err(charger->dev, "failed to disable regulator: %d\n", ret);
> +	}
> +}
> +
> +static int max8997_battery_extcon_evt(struct notifier_block *nb,
> +				unsigned long event, void *param)
> +{
> +	struct charger_data *charger =
> +		container_of(nb, struct charger_data, extcon_nb);
> +	schedule_work(&charger->extcon_work);
> +	return NOTIFY_OK;
> +}
> +
>  static const struct power_supply_desc max8997_battery_desc = {
>  	.name		= "max8997_pmic",
>  	.type		= POWER_SUPPLY_TYPE_BATTERY,
> @@ -170,6 +237,28 @@ static int max8997_battery_probe(struct platform_device *pdev)
>  		return PTR_ERR(charger->battery);
>  	}
>  
> +	charger->reg = devm_regulator_get(&pdev->dev, "charger");

Since you do not use get_optional, you will always get a dummy
regulator. In case of error, you should either print it or entirely fail
the probe. Silently continuing makes it difficult to spot errors.

Since the driver could operate in case of extcon/regulator error, just
dev_err() so failure will be spotted with dmesg.

It will complain on older DTBs because you are introducing incompatible
change, but that's expected. Just correct all other in-tree DTS.

Best regards,
Krzysztof


> +	charger->edev = extcon_get_edev_by_phandle(&pdev->dev, 0);
> +	if (PTR_ERR(charger->reg) == -EPROBE_DEFER ||
> +	    PTR_ERR(charger->edev) == -EPROBE_DEFER)
> +		return -EPROBE_DEFER;
> +
> +	if (!IS_ERR(charger->reg) && !IS_ERR(charger->edev)) {
> +		INIT_WORK(&charger->extcon_work, max8997_battery_extcon_evt_worker);
> +		ret = devm_add_action(&pdev->dev, max8997_battery_extcon_evt_stop_work, charger);
> +		if (ret) {
> +			dev_err(&pdev->dev, "failed to add extcon evt stop action: %d\n", ret);
> +			return ret;
> +		}
> +		charger->extcon_nb.notifier_call = max8997_battery_extcon_evt;
> +		ret = devm_extcon_register_notifier_all(&pdev->dev, charger->edev,
> +							&charger->extcon_nb);
> +		if (ret) {
> +			dev_err(&pdev->dev, "failed to register extcon notifier\n");
> +			return ret;
> +		};
> +	}
> +
>  	return 0;
>  }
>  
> -- 
> 2.25.1
> 
> 

Re: [PATCH v4 4/7] power: supply: max8997_charger: Set CHARGER current limit

[Krzysztof Kozlowski]

On Wed, Dec 23, 2020 at 01:43:05PM +0000, Timon Baetz wrote:
> Register for extcon notification and set charging current depending on
> the detected cable type. Current values are taken from vendor kernel,
> where most charger types end up setting 650mA [0].
> 
> Also enable and disable the CHARGER regulator based on extcon events.
> 
> [0] https://github.com/krzk/linux-vendor-backup/blob/samsung/galaxy-s2-epic-4g-touch-sph-d710-exynos4210-dump/drivers/misc/max8997-muic.c#L1675-L1678
> 
> Signed-off-by: Timon Baetz <timon.baetz@protonmail.com>
> ---
>  drivers/power/supply/max8997_charger.c | 89 ++++++++++++++++++++++++++
>  1 file changed, 89 insertions(+)
> 
> diff --git a/drivers/power/supply/max8997_charger.c b/drivers/power/supply/max8997_charger.c
> index 1947af25879a..e8532e2af451 100644
> --- a/drivers/power/supply/max8997_charger.c
> +++ b/drivers/power/supply/max8997_charger.c
> @@ -6,12 +6,14 @@
>  //  MyungJoo Ham <myungjoo.ham@samsung.com>
>  
>  #include <linux/err.h>
> +#include <linux/extcon.h>
>  #include <linux/module.h>
>  #include <linux/slab.h>
>  #include <linux/platform_device.h>
>  #include <linux/power_supply.h>
>  #include <linux/mfd/max8997.h>
>  #include <linux/mfd/max8997-private.h>
> +#include <linux/regulator/consumer.h>
>  
>  /* MAX8997_REG_STATUS4 */
>  #define DCINOK_SHIFT		1
> @@ -31,6 +33,10 @@ struct charger_data {
>  	struct device *dev;
>  	struct max8997_dev *iodev;
>  	struct power_supply *battery;
> +	struct regulator *reg;
> +	struct extcon_dev *edev;
> +	struct notifier_block extcon_nb;
> +	struct work_struct extcon_work;
>  };
>  
>  static enum power_supply_property max8997_battery_props[] = {
> @@ -88,6 +94,67 @@ static int max8997_battery_get_property(struct power_supply *psy,
>  	return 0;
>  }
>  
> +static void max8997_battery_extcon_evt_stop_work(void *data)
> +{
> +	struct charger_data *charger = data;
> +
> +	cancel_work_sync(&charger->extcon_work);
> +}
> +
> +static void max8997_battery_extcon_evt_worker(struct work_struct *work)
> +{
> +	struct charger_data *charger =
> +	    container_of(work, struct charger_data, extcon_work);
> +	struct extcon_dev *edev = charger->edev;
> +	int current_limit;
> +
> +	if (extcon_get_state(edev, EXTCON_CHG_USB_SDP) > 0) {
> +		dev_dbg(charger->dev, "USB SDP charger is connected\n");
> +		current_limit = 450000;
> +	} else if (extcon_get_state(edev, EXTCON_CHG_USB_DCP) > 0) {
> +		dev_dbg(charger->dev, "USB DCP charger is connected\n");
> +		current_limit = 650000;
> +	} else if (extcon_get_state(edev, EXTCON_CHG_USB_FAST) > 0) {
> +		dev_dbg(charger->dev, "USB FAST charger is connected\n");
> +		current_limit = 650000;
> +	} else if (extcon_get_state(edev, EXTCON_CHG_USB_SLOW) > 0) {
> +		dev_dbg(charger->dev, "USB SLOW charger is connected\n");
> +		current_limit = 650000;
> +	} else if (extcon_get_state(edev, EXTCON_CHG_USB_CDP) > 0) {
> +		dev_dbg(charger->dev, "USB CDP charger is connected\n");
> +		current_limit = 650000;
> +	} else {
> +		dev_dbg(charger->dev, "USB charger is diconnected\n");
> +		current_limit = -1;
> +	}
> +
> +	if (current_limit > 0) {
> +		int ret = regulator_set_current_limit(charger->reg, current_limit, current_limit);
> +
> +		if (ret) {
> +			dev_err(charger->dev, "failed to set current limit: %d\n", ret);
> +			return;
> +		}
> +		ret = regulator_enable(charger->reg);
> +		if (ret)
> +			dev_err(charger->dev, "failed to enable regulator: %d\n", ret);
> +	} else {
> +		int ret  = regulator_disable(charger->reg);
> +
> +		if (ret)
> +			dev_err(charger->dev, "failed to disable regulator: %d\n", ret);
> +	}
> +}
> +
> +static int max8997_battery_extcon_evt(struct notifier_block *nb,
> +				unsigned long event, void *param)
> +{
> +	struct charger_data *charger =
> +		container_of(nb, struct charger_data, extcon_nb);
> +	schedule_work(&charger->extcon_work);
> +	return NOTIFY_OK;
> +}
> +
>  static const struct power_supply_desc max8997_battery_desc = {
>  	.name		= "max8997_pmic",
>  	.type		= POWER_SUPPLY_TYPE_BATTERY,
> @@ -170,6 +237,28 @@ static int max8997_battery_probe(struct platform_device *pdev)
>  		return PTR_ERR(charger->battery);
>  	}
>  
> +	charger->reg = devm_regulator_get(&pdev->dev, "charger");

Since you do not use get_optional, you will always get a dummy
regulator. In case of error, you should either print it or entirely fail
the probe. Silently continuing makes it difficult to spot errors.

Since the driver could operate in case of extcon/regulator error, just
dev_err() so failure will be spotted with dmesg.

It will complain on older DTBs because you are introducing incompatible
change, but that's expected. Just correct all other in-tree DTS.

Best regards,
Krzysztof


> +	charger->edev = extcon_get_edev_by_phandle(&pdev->dev, 0);
> +	if (PTR_ERR(charger->reg) == -EPROBE_DEFER ||
> +	    PTR_ERR(charger->edev) == -EPROBE_DEFER)
> +		return -EPROBE_DEFER;
> +
> +	if (!IS_ERR(charger->reg) && !IS_ERR(charger->edev)) {
> +		INIT_WORK(&charger->extcon_work, max8997_battery_extcon_evt_worker);
> +		ret = devm_add_action(&pdev->dev, max8997_battery_extcon_evt_stop_work, charger);
> +		if (ret) {
> +			dev_err(&pdev->dev, "failed to add extcon evt stop action: %d\n", ret);
> +			return ret;
> +		}
> +		charger->extcon_nb.notifier_call = max8997_battery_extcon_evt;
> +		ret = devm_extcon_register_notifier_all(&pdev->dev, charger->edev,
> +							&charger->extcon_nb);
> +		if (ret) {
> +			dev_err(&pdev->dev, "failed to register extcon notifier\n");
> +			return ret;
> +		};
> +	}
> +
>  	return 0;
>  }
>  
> -- 
> 2.25.1
> 
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 3/7] mfd: max8997: Add of_compatible to extcon and charger mfd_cell

[Timon Baetz]

On Wed, 23 Dec 2020 15:32:07 +0000, Lee Jones wrote:
> On Wed, 23 Dec 2020, Timon Baetz wrote:
> 
> > Add of_compatible ("maxim,max8997-muic") to the mfd_cell to have a
> > of_node set in the extcon driver.
> >
> > Add of_compatible ("maxim,max8997-battery") to the mfd_cell to configure
> > the charger driver.
> >
> > Signed-off-by: Timon Baetz <timon.baetz@protonmail.com>
> > ---
> >  drivers/mfd/max8997.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)  
> 
> Why have you resent this?  It's already applied.
> 

I made a change to an other patch in this series and wasn't sure if I
had to resubmit everything. 

Thanks and sorry for the spam.
Timon

Re: [PATCH v4 3/7] mfd: max8997: Add of_compatible to extcon and charger mfd_cell

[Timon Baetz]

On Wed, 23 Dec 2020 15:32:07 +0000, Lee Jones wrote:
> On Wed, 23 Dec 2020, Timon Baetz wrote:
> 
> > Add of_compatible ("maxim,max8997-muic") to the mfd_cell to have a
> > of_node set in the extcon driver.
> >
> > Add of_compatible ("maxim,max8997-battery") to the mfd_cell to configure
> > the charger driver.
> >
> > Signed-off-by: Timon Baetz <timon.baetz@protonmail.com>
> > ---
> >  drivers/mfd/max8997.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)  
> 
> Why have you resent this?  It's already applied.
> 

I made a change to an other patch in this series and wasn't sure if I
had to resubmit everything. 

Thanks and sorry for the spam.
Timon


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 3/7] mfd: max8997: Add of_compatible to extcon and charger mfd_cell

[Lee Jones]

On Thu, 24 Dec 2020, Timon Baetz wrote:

> On Wed, 23 Dec 2020 15:32:07 +0000, Lee Jones wrote:
> > On Wed, 23 Dec 2020, Timon Baetz wrote:
> > 
> > > Add of_compatible ("maxim,max8997-muic") to the mfd_cell to have a
> > > of_node set in the extcon driver.
> > >
> > > Add of_compatible ("maxim,max8997-battery") to the mfd_cell to configure
> > > the charger driver.
> > >
> > > Signed-off-by: Timon Baetz <timon.baetz@protonmail.com>
> > > ---
> > >  drivers/mfd/max8997.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)  
> > 
> > Why have you resent this?  It's already applied.
> > 
> 
> I made a change to an other patch in this series and wasn't sure if I
> had to resubmit everything. 

No need to send patches that have been applied.

You can safely drop it from the set.

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

Re: [PATCH v4 3/7] mfd: max8997: Add of_compatible to extcon and charger mfd_cell

[Lee Jones]

On Thu, 24 Dec 2020, Timon Baetz wrote:

> On Wed, 23 Dec 2020 15:32:07 +0000, Lee Jones wrote:
> > On Wed, 23 Dec 2020, Timon Baetz wrote:
> > 
> > > Add of_compatible ("maxim,max8997-muic") to the mfd_cell to have a
> > > of_node set in the extcon driver.
> > >
> > > Add of_compatible ("maxim,max8997-battery") to the mfd_cell to configure
> > > the charger driver.
> > >
> > > Signed-off-by: Timon Baetz <timon.baetz@protonmail.com>
> > > ---
> > >  drivers/mfd/max8997.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)  
> > 
> > Why have you resent this?  It's already applied.
> > 
> 
> I made a change to an other patch in this series and wasn't sure if I
> had to resubmit everything. 

No need to send patches that have been applied.

You can safely drop it from the set.

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 4/7] power: supply: max8997_charger: Set CHARGER current limit

[Timon Baetz]

On Thu, 24 Dec 2020 10:55:59 +0100, Krzysztof Kozlowski wrote:
> On Wed, Dec 23, 2020 at 01:43:05PM +0000, Timon Baetz wrote:
> > Register for extcon notification and set charging current depending on
> > the detected cable type. Current values are taken from vendor kernel,
> > where most charger types end up setting 650mA [0].
> >
> > Also enable and disable the CHARGER regulator based on extcon events.
> >
> > [0] https://github.com/krzk/linux-vendor-backup/blob/samsung/galaxy-s2-epic-4g-touch-sph-d710-exynos4210-dump/drivers/misc/max8997-muic.c#L1675-L1678
> >
> > Signed-off-by: Timon Baetz <timon.baetz@protonmail.com>
> > ---
> >  drivers/power/supply/max8997_charger.c | 89 ++++++++++++++++++++++++++
> >  1 file changed, 89 insertions(+)
> >
> > diff --git a/drivers/power/supply/max8997_charger.c b/drivers/power/supply/max8997_charger.c
> > index 1947af25879a..e8532e2af451 100644
> > --- a/drivers/power/supply/max8997_charger.c
> > +++ b/drivers/power/supply/max8997_charger.c
> > @@ -6,12 +6,14 @@
> >  //  MyungJoo Ham <myungjoo.ham@samsung.com>
> >
> >  #include <linux/err.h>
> > +#include <linux/extcon.h>
> >  #include <linux/module.h>
> >  #include <linux/slab.h>
> >  #include <linux/platform_device.h>
> >  #include <linux/power_supply.h>
> >  #include <linux/mfd/max8997.h>
> >  #include <linux/mfd/max8997-private.h>
> > +#include <linux/regulator/consumer.h>
> >
> >  /* MAX8997_REG_STATUS4 */
> >  #define DCINOK_SHIFT		1
> > @@ -31,6 +33,10 @@ struct charger_data {
> >  	struct device *dev;
> >  	struct max8997_dev *iodev;
> >  	struct power_supply *battery;
> > +	struct regulator *reg;
> > +	struct extcon_dev *edev;
> > +	struct notifier_block extcon_nb;
> > +	struct work_struct extcon_work;
> >  };
> >
> >  static enum power_supply_property max8997_battery_props[] = {
> > @@ -88,6 +94,67 @@ static int max8997_battery_get_property(struct power_supply *psy,
> >  	return 0;
> >  }
> >
> > +static void max8997_battery_extcon_evt_stop_work(void *data)
> > +{
> > +	struct charger_data *charger = data;
> > +
> > +	cancel_work_sync(&charger->extcon_work);
> > +}
> > +
> > +static void max8997_battery_extcon_evt_worker(struct work_struct *work)
> > +{
> > +	struct charger_data *charger =
> > +	    container_of(work, struct charger_data, extcon_work);
> > +	struct extcon_dev *edev = charger->edev;
> > +	int current_limit;
> > +
> > +	if (extcon_get_state(edev, EXTCON_CHG_USB_SDP) > 0) {
> > +		dev_dbg(charger->dev, "USB SDP charger is connected\n");
> > +		current_limit = 450000;
> > +	} else if (extcon_get_state(edev, EXTCON_CHG_USB_DCP) > 0) {
> > +		dev_dbg(charger->dev, "USB DCP charger is connected\n");
> > +		current_limit = 650000;
> > +	} else if (extcon_get_state(edev, EXTCON_CHG_USB_FAST) > 0) {
> > +		dev_dbg(charger->dev, "USB FAST charger is connected\n");
> > +		current_limit = 650000;
> > +	} else if (extcon_get_state(edev, EXTCON_CHG_USB_SLOW) > 0) {
> > +		dev_dbg(charger->dev, "USB SLOW charger is connected\n");
> > +		current_limit = 650000;
> > +	} else if (extcon_get_state(edev, EXTCON_CHG_USB_CDP) > 0) {
> > +		dev_dbg(charger->dev, "USB CDP charger is connected\n");
> > +		current_limit = 650000;
> > +	} else {
> > +		dev_dbg(charger->dev, "USB charger is diconnected\n");
> > +		current_limit = -1;
> > +	}
> > +
> > +	if (current_limit > 0) {
> > +		int ret = regulator_set_current_limit(charger->reg, current_limit, current_limit);
> > +
> > +		if (ret) {
> > +			dev_err(charger->dev, "failed to set current limit: %d\n", ret);
> > +			return;
> > +		}
> > +		ret = regulator_enable(charger->reg);
> > +		if (ret)
> > +			dev_err(charger->dev, "failed to enable regulator: %d\n", ret);
> > +	} else {
> > +		int ret  = regulator_disable(charger->reg);
> > +
> > +		if (ret)
> > +			dev_err(charger->dev, "failed to disable regulator: %d\n", ret);
> > +	}
> > +}
> > +
> > +static int max8997_battery_extcon_evt(struct notifier_block *nb,
> > +				unsigned long event, void *param)
> > +{
> > +	struct charger_data *charger =
> > +		container_of(nb, struct charger_data, extcon_nb);
> > +	schedule_work(&charger->extcon_work);
> > +	return NOTIFY_OK;
> > +}
> > +
> >  static const struct power_supply_desc max8997_battery_desc = {
> >  	.name		= "max8997_pmic",
> >  	.type		= POWER_SUPPLY_TYPE_BATTERY,
> > @@ -170,6 +237,28 @@ static int max8997_battery_probe(struct platform_device *pdev)
> >  		return PTR_ERR(charger->battery);
> >  	}
> >
> > +	charger->reg = devm_regulator_get(&pdev->dev, "charger");  
> 
> Since you do not use get_optional, you will always get a dummy
> regulator. In case of error, you should either print it or entirely fail
> the probe. Silently continuing makes it difficult to spot errors.
> 
> Since the driver could operate in case of extcon/regulator error, just
> dev_err() so failure will be spotted with dmesg.

I will switch to devm_regulator_get_optional() and print an error on 
failure, thanks.

> It will complain on older DTBs because you are introducing incompatible
> change, but that's expected. Just correct all other in-tree DTS.

The other 2 in-tree DTS don't have CHARGER regulators. Not sure
how to correct those. Should I add muic and charger nodes without a
charger-supply? It will still complain in that case.

Thanks for guiding me through this,
Timon

> > +	charger->edev = extcon_get_edev_by_phandle(&pdev->dev, 0);
> > +	if (PTR_ERR(charger->reg) == -EPROBE_DEFER ||
> > +	    PTR_ERR(charger->edev) == -EPROBE_DEFER)
> > +		return -EPROBE_DEFER;
> > +
> > +	if (!IS_ERR(charger->reg) && !IS_ERR(charger->edev)) {
> > +		INIT_WORK(&charger->extcon_work, max8997_battery_extcon_evt_worker);
> > +		ret = devm_add_action(&pdev->dev, max8997_battery_extcon_evt_stop_work, charger);
> > +		if (ret) {
> > +			dev_err(&pdev->dev, "failed to add extcon evt stop action: %d\n", ret);
> > +			return ret;
> > +		}
> > +		charger->extcon_nb.notifier_call = max8997_battery_extcon_evt;
> > +		ret = devm_extcon_register_notifier_all(&pdev->dev, charger->edev,
> > +							&charger->extcon_nb);
> > +		if (ret) {
> > +			dev_err(&pdev->dev, "failed to register extcon notifier\n");
> > +			return ret;
> > +		};
> > +	}
> > +
> >  	return 0;
> >  }
> >
> > --
> > 2.25.1
> >
> >  

Re: [PATCH v4 4/7] power: supply: max8997_charger: Set CHARGER current limit

[Timon Baetz]

On Thu, 24 Dec 2020 10:55:59 +0100, Krzysztof Kozlowski wrote:
> On Wed, Dec 23, 2020 at 01:43:05PM +0000, Timon Baetz wrote:
> > Register for extcon notification and set charging current depending on
> > the detected cable type. Current values are taken from vendor kernel,
> > where most charger types end up setting 650mA [0].
> >
> > Also enable and disable the CHARGER regulator based on extcon events.
> >
> > [0] https://github.com/krzk/linux-vendor-backup/blob/samsung/galaxy-s2-epic-4g-touch-sph-d710-exynos4210-dump/drivers/misc/max8997-muic.c#L1675-L1678
> >
> > Signed-off-by: Timon Baetz <timon.baetz@protonmail.com>
> > ---
> >  drivers/power/supply/max8997_charger.c | 89 ++++++++++++++++++++++++++
> >  1 file changed, 89 insertions(+)
> >
> > diff --git a/drivers/power/supply/max8997_charger.c b/drivers/power/supply/max8997_charger.c
> > index 1947af25879a..e8532e2af451 100644
> > --- a/drivers/power/supply/max8997_charger.c
> > +++ b/drivers/power/supply/max8997_charger.c
> > @@ -6,12 +6,14 @@
> >  //  MyungJoo Ham <myungjoo.ham@samsung.com>
> >
> >  #include <linux/err.h>
> > +#include <linux/extcon.h>
> >  #include <linux/module.h>
> >  #include <linux/slab.h>
> >  #include <linux/platform_device.h>
> >  #include <linux/power_supply.h>
> >  #include <linux/mfd/max8997.h>
> >  #include <linux/mfd/max8997-private.h>
> > +#include <linux/regulator/consumer.h>
> >
> >  /* MAX8997_REG_STATUS4 */
> >  #define DCINOK_SHIFT		1
> > @@ -31,6 +33,10 @@ struct charger_data {
> >  	struct device *dev;
> >  	struct max8997_dev *iodev;
> >  	struct power_supply *battery;
> > +	struct regulator *reg;
> > +	struct extcon_dev *edev;
> > +	struct notifier_block extcon_nb;
> > +	struct work_struct extcon_work;
> >  };
> >
> >  static enum power_supply_property max8997_battery_props[] = {
> > @@ -88,6 +94,67 @@ static int max8997_battery_get_property(struct power_supply *psy,
> >  	return 0;
> >  }
> >
> > +static void max8997_battery_extcon_evt_stop_work(void *data)
> > +{
> > +	struct charger_data *charger = data;
> > +
> > +	cancel_work_sync(&charger->extcon_work);
> > +}
> > +
> > +static void max8997_battery_extcon_evt_worker(struct work_struct *work)
> > +{
> > +	struct charger_data *charger =
> > +	    container_of(work, struct charger_data, extcon_work);
> > +	struct extcon_dev *edev = charger->edev;
> > +	int current_limit;
> > +
> > +	if (extcon_get_state(edev, EXTCON_CHG_USB_SDP) > 0) {
> > +		dev_dbg(charger->dev, "USB SDP charger is connected\n");
> > +		current_limit = 450000;
> > +	} else if (extcon_get_state(edev, EXTCON_CHG_USB_DCP) > 0) {
> > +		dev_dbg(charger->dev, "USB DCP charger is connected\n");
> > +		current_limit = 650000;
> > +	} else if (extcon_get_state(edev, EXTCON_CHG_USB_FAST) > 0) {
> > +		dev_dbg(charger->dev, "USB FAST charger is connected\n");
> > +		current_limit = 650000;
> > +	} else if (extcon_get_state(edev, EXTCON_CHG_USB_SLOW) > 0) {
> > +		dev_dbg(charger->dev, "USB SLOW charger is connected\n");
> > +		current_limit = 650000;
> > +	} else if (extcon_get_state(edev, EXTCON_CHG_USB_CDP) > 0) {
> > +		dev_dbg(charger->dev, "USB CDP charger is connected\n");
> > +		current_limit = 650000;
> > +	} else {
> > +		dev_dbg(charger->dev, "USB charger is diconnected\n");
> > +		current_limit = -1;
> > +	}
> > +
> > +	if (current_limit > 0) {
> > +		int ret = regulator_set_current_limit(charger->reg, current_limit, current_limit);
> > +
> > +		if (ret) {
> > +			dev_err(charger->dev, "failed to set current limit: %d\n", ret);
> > +			return;
> > +		}
> > +		ret = regulator_enable(charger->reg);
> > +		if (ret)
> > +			dev_err(charger->dev, "failed to enable regulator: %d\n", ret);
> > +	} else {
> > +		int ret  = regulator_disable(charger->reg);
> > +
> > +		if (ret)
> > +			dev_err(charger->dev, "failed to disable regulator: %d\n", ret);
> > +	}
> > +}
> > +
> > +static int max8997_battery_extcon_evt(struct notifier_block *nb,
> > +				unsigned long event, void *param)
> > +{
> > +	struct charger_data *charger =
> > +		container_of(nb, struct charger_data, extcon_nb);
> > +	schedule_work(&charger->extcon_work);
> > +	return NOTIFY_OK;
> > +}
> > +
> >  static const struct power_supply_desc max8997_battery_desc = {
> >  	.name		= "max8997_pmic",
> >  	.type		= POWER_SUPPLY_TYPE_BATTERY,
> > @@ -170,6 +237,28 @@ static int max8997_battery_probe(struct platform_device *pdev)
> >  		return PTR_ERR(charger->battery);
> >  	}
> >
> > +	charger->reg = devm_regulator_get(&pdev->dev, "charger");  
> 
> Since you do not use get_optional, you will always get a dummy
> regulator. In case of error, you should either print it or entirely fail
> the probe. Silently continuing makes it difficult to spot errors.
> 
> Since the driver could operate in case of extcon/regulator error, just
> dev_err() so failure will be spotted with dmesg.

I will switch to devm_regulator_get_optional() and print an error on 
failure, thanks.

> It will complain on older DTBs because you are introducing incompatible
> change, but that's expected. Just correct all other in-tree DTS.

The other 2 in-tree DTS don't have CHARGER regulators. Not sure
how to correct those. Should I add muic and charger nodes without a
charger-supply? It will still complain in that case.

Thanks for guiding me through this,
Timon

> > +	charger->edev = extcon_get_edev_by_phandle(&pdev->dev, 0);
> > +	if (PTR_ERR(charger->reg) == -EPROBE_DEFER ||
> > +	    PTR_ERR(charger->edev) == -EPROBE_DEFER)
> > +		return -EPROBE_DEFER;
> > +
> > +	if (!IS_ERR(charger->reg) && !IS_ERR(charger->edev)) {
> > +		INIT_WORK(&charger->extcon_work, max8997_battery_extcon_evt_worker);
> > +		ret = devm_add_action(&pdev->dev, max8997_battery_extcon_evt_stop_work, charger);
> > +		if (ret) {
> > +			dev_err(&pdev->dev, "failed to add extcon evt stop action: %d\n", ret);
> > +			return ret;
> > +		}
> > +		charger->extcon_nb.notifier_call = max8997_battery_extcon_evt;
> > +		ret = devm_extcon_register_notifier_all(&pdev->dev, charger->edev,
> > +							&charger->extcon_nb);
> > +		if (ret) {
> > +			dev_err(&pdev->dev, "failed to register extcon notifier\n");
> > +			return ret;
> > +		};
> > +	}
> > +
> >  	return 0;
> >  }
> >
> > --
> > 2.25.1
> >
> >  




_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 4/7] power: supply: max8997_charger: Set CHARGER current limit

[Krzysztof Kozlowski]

On Thu, Dec 24, 2020 at 01:13:02PM +0000, Timon Baetz wrote:
> On Thu, 24 Dec 2020 10:55:59 +0100, Krzysztof Kozlowski wrote:
> > On Wed, Dec 23, 2020 at 01:43:05PM +0000, Timon Baetz wrote:
> > > Register for extcon notification and set charging current depending on
> > > the detected cable type. Current values are taken from vendor kernel,
> > > where most charger types end up setting 650mA [0].
> > >
> > > Also enable and disable the CHARGER regulator based on extcon events.
> > >
> > > [0] https://github.com/krzk/linux-vendor-backup/blob/samsung/galaxy-s2-epic-4g-touch-sph-d710-exynos4210-dump/drivers/misc/max8997-muic.c#L1675-L1678
> > >
> > > Signed-off-by: Timon Baetz <timon.baetz@protonmail.com>
> > > ---
> > >  drivers/power/supply/max8997_charger.c | 89 ++++++++++++++++++++++++++
> > >  1 file changed, 89 insertions(+)
> > >
> > > diff --git a/drivers/power/supply/max8997_charger.c b/drivers/power/supply/max8997_charger.c
> > > index 1947af25879a..e8532e2af451 100644
> > > --- a/drivers/power/supply/max8997_charger.c
> > > +++ b/drivers/power/supply/max8997_charger.c
> > > @@ -6,12 +6,14 @@
> > >  //  MyungJoo Ham <myungjoo.ham@samsung.com>
> > >
> > >  #include <linux/err.h>
> > > +#include <linux/extcon.h>
> > >  #include <linux/module.h>
> > >  #include <linux/slab.h>
> > >  #include <linux/platform_device.h>
> > >  #include <linux/power_supply.h>
> > >  #include <linux/mfd/max8997.h>
> > >  #include <linux/mfd/max8997-private.h>
> > > +#include <linux/regulator/consumer.h>
> > >
> > >  /* MAX8997_REG_STATUS4 */
> > >  #define DCINOK_SHIFT		1
> > > @@ -31,6 +33,10 @@ struct charger_data {
> > >  	struct device *dev;
> > >  	struct max8997_dev *iodev;
> > >  	struct power_supply *battery;
> > > +	struct regulator *reg;
> > > +	struct extcon_dev *edev;
> > > +	struct notifier_block extcon_nb;
> > > +	struct work_struct extcon_work;
> > >  };
> > >
> > >  static enum power_supply_property max8997_battery_props[] = {
> > > @@ -88,6 +94,67 @@ static int max8997_battery_get_property(struct power_supply *psy,
> > >  	return 0;
> > >  }
> > >
> > > +static void max8997_battery_extcon_evt_stop_work(void *data)
> > > +{
> > > +	struct charger_data *charger = data;
> > > +
> > > +	cancel_work_sync(&charger->extcon_work);
> > > +}
> > > +
> > > +static void max8997_battery_extcon_evt_worker(struct work_struct *work)
> > > +{
> > > +	struct charger_data *charger =
> > > +	    container_of(work, struct charger_data, extcon_work);
> > > +	struct extcon_dev *edev = charger->edev;
> > > +	int current_limit;
> > > +
> > > +	if (extcon_get_state(edev, EXTCON_CHG_USB_SDP) > 0) {
> > > +		dev_dbg(charger->dev, "USB SDP charger is connected\n");
> > > +		current_limit = 450000;
> > > +	} else if (extcon_get_state(edev, EXTCON_CHG_USB_DCP) > 0) {
> > > +		dev_dbg(charger->dev, "USB DCP charger is connected\n");
> > > +		current_limit = 650000;
> > > +	} else if (extcon_get_state(edev, EXTCON_CHG_USB_FAST) > 0) {
> > > +		dev_dbg(charger->dev, "USB FAST charger is connected\n");
> > > +		current_limit = 650000;
> > > +	} else if (extcon_get_state(edev, EXTCON_CHG_USB_SLOW) > 0) {
> > > +		dev_dbg(charger->dev, "USB SLOW charger is connected\n");
> > > +		current_limit = 650000;
> > > +	} else if (extcon_get_state(edev, EXTCON_CHG_USB_CDP) > 0) {
> > > +		dev_dbg(charger->dev, "USB CDP charger is connected\n");
> > > +		current_limit = 650000;
> > > +	} else {
> > > +		dev_dbg(charger->dev, "USB charger is diconnected\n");
> > > +		current_limit = -1;
> > > +	}
> > > +
> > > +	if (current_limit > 0) {
> > > +		int ret = regulator_set_current_limit(charger->reg, current_limit, current_limit);
> > > +
> > > +		if (ret) {
> > > +			dev_err(charger->dev, "failed to set current limit: %d\n", ret);
> > > +			return;
> > > +		}
> > > +		ret = regulator_enable(charger->reg);
> > > +		if (ret)
> > > +			dev_err(charger->dev, "failed to enable regulator: %d\n", ret);
> > > +	} else {
> > > +		int ret  = regulator_disable(charger->reg);
> > > +
> > > +		if (ret)
> > > +			dev_err(charger->dev, "failed to disable regulator: %d\n", ret);
> > > +	}
> > > +}
> > > +
> > > +static int max8997_battery_extcon_evt(struct notifier_block *nb,
> > > +				unsigned long event, void *param)
> > > +{
> > > +	struct charger_data *charger =
> > > +		container_of(nb, struct charger_data, extcon_nb);
> > > +	schedule_work(&charger->extcon_work);
> > > +	return NOTIFY_OK;
> > > +}
> > > +
> > >  static const struct power_supply_desc max8997_battery_desc = {
> > >  	.name		= "max8997_pmic",
> > >  	.type		= POWER_SUPPLY_TYPE_BATTERY,
> > > @@ -170,6 +237,28 @@ static int max8997_battery_probe(struct platform_device *pdev)
> > >  		return PTR_ERR(charger->battery);
> > >  	}
> > >
> > > +	charger->reg = devm_regulator_get(&pdev->dev, "charger");  
> > 
> > Since you do not use get_optional, you will always get a dummy
> > regulator. In case of error, you should either print it or entirely fail
> > the probe. Silently continuing makes it difficult to spot errors.
> > 
> > Since the driver could operate in case of extcon/regulator error, just
> > dev_err() so failure will be spotted with dmesg.
> 
> I will switch to devm_regulator_get_optional() and print an error on 
> failure, thanks.
> 
> > It will complain on older DTBs because you are introducing incompatible
> > change, but that's expected. Just correct all other in-tree DTS.
> 
> The other 2 in-tree DTS don't have CHARGER regulators. Not sure
> how to correct those. Should I add muic and charger nodes without a
> charger-supply? It will still complain in that case.

+Cc Marek,

This is why leaving the code as is - devm_regulator_get(), not optional
- makes sense. Core would provide dummy regulator, so you only have to
provide MUIC node.

If you change the code to devm_regulator_get_optional(), you need to add
everything: the charger regulator, the charger node and MUIC node.

For Trats, the configuration should be similar as i9100, although I
don't know the exact values of chargign voltage.

For Origen, there is no battery, so the power supply should not bind.
Maybe this could be achieved with "status disabled" for charger node? It
depends whether MFD will respect such field... If it disables the
charger, you're done.
The max8997 regulator driver is weird... AFAIU, it registers only
regulators present in DT. That's not how regulator driver should work.
Maybe you could confirm it? If it's true, then the value of charger
depends on bootloader settings and max8997 default values.

Best regards,
Krzysztof

Re: [PATCH v4 4/7] power: supply: max8997_charger: Set CHARGER current limit

[Krzysztof Kozlowski]

On Thu, Dec 24, 2020 at 01:13:02PM +0000, Timon Baetz wrote:
> On Thu, 24 Dec 2020 10:55:59 +0100, Krzysztof Kozlowski wrote:
> > On Wed, Dec 23, 2020 at 01:43:05PM +0000, Timon Baetz wrote:
> > > Register for extcon notification and set charging current depending on
> > > the detected cable type. Current values are taken from vendor kernel,
> > > where most charger types end up setting 650mA [0].
> > >
> > > Also enable and disable the CHARGER regulator based on extcon events.
> > >
> > > [0] https://github.com/krzk/linux-vendor-backup/blob/samsung/galaxy-s2-epic-4g-touch-sph-d710-exynos4210-dump/drivers/misc/max8997-muic.c#L1675-L1678
> > >
> > > Signed-off-by: Timon Baetz <timon.baetz@protonmail.com>
> > > ---
> > >  drivers/power/supply/max8997_charger.c | 89 ++++++++++++++++++++++++++
> > >  1 file changed, 89 insertions(+)
> > >
> > > diff --git a/drivers/power/supply/max8997_charger.c b/drivers/power/supply/max8997_charger.c
> > > index 1947af25879a..e8532e2af451 100644
> > > --- a/drivers/power/supply/max8997_charger.c
> > > +++ b/drivers/power/supply/max8997_charger.c
> > > @@ -6,12 +6,14 @@
> > >  //  MyungJoo Ham <myungjoo.ham@samsung.com>
> > >
> > >  #include <linux/err.h>
> > > +#include <linux/extcon.h>
> > >  #include <linux/module.h>
> > >  #include <linux/slab.h>
> > >  #include <linux/platform_device.h>
> > >  #include <linux/power_supply.h>
> > >  #include <linux/mfd/max8997.h>
> > >  #include <linux/mfd/max8997-private.h>
> > > +#include <linux/regulator/consumer.h>
> > >
> > >  /* MAX8997_REG_STATUS4 */
> > >  #define DCINOK_SHIFT		1
> > > @@ -31,6 +33,10 @@ struct charger_data {
> > >  	struct device *dev;
> > >  	struct max8997_dev *iodev;
> > >  	struct power_supply *battery;
> > > +	struct regulator *reg;
> > > +	struct extcon_dev *edev;
> > > +	struct notifier_block extcon_nb;
> > > +	struct work_struct extcon_work;
> > >  };
> > >
> > >  static enum power_supply_property max8997_battery_props[] = {
> > > @@ -88,6 +94,67 @@ static int max8997_battery_get_property(struct power_supply *psy,
> > >  	return 0;
> > >  }
> > >
> > > +static void max8997_battery_extcon_evt_stop_work(void *data)
> > > +{
> > > +	struct charger_data *charger = data;
> > > +
> > > +	cancel_work_sync(&charger->extcon_work);
> > > +}
> > > +
> > > +static void max8997_battery_extcon_evt_worker(struct work_struct *work)
> > > +{
> > > +	struct charger_data *charger =
> > > +	    container_of(work, struct charger_data, extcon_work);
> > > +	struct extcon_dev *edev = charger->edev;
> > > +	int current_limit;
> > > +
> > > +	if (extcon_get_state(edev, EXTCON_CHG_USB_SDP) > 0) {
> > > +		dev_dbg(charger->dev, "USB SDP charger is connected\n");
> > > +		current_limit = 450000;
> > > +	} else if (extcon_get_state(edev, EXTCON_CHG_USB_DCP) > 0) {
> > > +		dev_dbg(charger->dev, "USB DCP charger is connected\n");
> > > +		current_limit = 650000;
> > > +	} else if (extcon_get_state(edev, EXTCON_CHG_USB_FAST) > 0) {
> > > +		dev_dbg(charger->dev, "USB FAST charger is connected\n");
> > > +		current_limit = 650000;
> > > +	} else if (extcon_get_state(edev, EXTCON_CHG_USB_SLOW) > 0) {
> > > +		dev_dbg(charger->dev, "USB SLOW charger is connected\n");
> > > +		current_limit = 650000;
> > > +	} else if (extcon_get_state(edev, EXTCON_CHG_USB_CDP) > 0) {
> > > +		dev_dbg(charger->dev, "USB CDP charger is connected\n");
> > > +		current_limit = 650000;
> > > +	} else {
> > > +		dev_dbg(charger->dev, "USB charger is diconnected\n");
> > > +		current_limit = -1;
> > > +	}
> > > +
> > > +	if (current_limit > 0) {
> > > +		int ret = regulator_set_current_limit(charger->reg, current_limit, current_limit);
> > > +
> > > +		if (ret) {
> > > +			dev_err(charger->dev, "failed to set current limit: %d\n", ret);
> > > +			return;
> > > +		}
> > > +		ret = regulator_enable(charger->reg);
> > > +		if (ret)
> > > +			dev_err(charger->dev, "failed to enable regulator: %d\n", ret);
> > > +	} else {
> > > +		int ret  = regulator_disable(charger->reg);
> > > +
> > > +		if (ret)
> > > +			dev_err(charger->dev, "failed to disable regulator: %d\n", ret);
> > > +	}
> > > +}
> > > +
> > > +static int max8997_battery_extcon_evt(struct notifier_block *nb,
> > > +				unsigned long event, void *param)
> > > +{
> > > +	struct charger_data *charger =
> > > +		container_of(nb, struct charger_data, extcon_nb);
> > > +	schedule_work(&charger->extcon_work);
> > > +	return NOTIFY_OK;
> > > +}
> > > +
> > >  static const struct power_supply_desc max8997_battery_desc = {
> > >  	.name		= "max8997_pmic",
> > >  	.type		= POWER_SUPPLY_TYPE_BATTERY,
> > > @@ -170,6 +237,28 @@ static int max8997_battery_probe(struct platform_device *pdev)
> > >  		return PTR_ERR(charger->battery);
> > >  	}
> > >
> > > +	charger->reg = devm_regulator_get(&pdev->dev, "charger");  
> > 
> > Since you do not use get_optional, you will always get a dummy
> > regulator. In case of error, you should either print it or entirely fail
> > the probe. Silently continuing makes it difficult to spot errors.
> > 
> > Since the driver could operate in case of extcon/regulator error, just
> > dev_err() so failure will be spotted with dmesg.
> 
> I will switch to devm_regulator_get_optional() and print an error on 
> failure, thanks.
> 
> > It will complain on older DTBs because you are introducing incompatible
> > change, but that's expected. Just correct all other in-tree DTS.
> 
> The other 2 in-tree DTS don't have CHARGER regulators. Not sure
> how to correct those. Should I add muic and charger nodes without a
> charger-supply? It will still complain in that case.

+Cc Marek,

This is why leaving the code as is - devm_regulator_get(), not optional
- makes sense. Core would provide dummy regulator, so you only have to
provide MUIC node.

If you change the code to devm_regulator_get_optional(), you need to add
everything: the charger regulator, the charger node and MUIC node.

For Trats, the configuration should be similar as i9100, although I
don't know the exact values of chargign voltage.

For Origen, there is no battery, so the power supply should not bind.
Maybe this could be achieved with "status disabled" for charger node? It
depends whether MFD will respect such field... If it disables the
charger, you're done.
The max8997 regulator driver is weird... AFAIU, it registers only
regulators present in DT. That's not how regulator driver should work.
Maybe you could confirm it? If it's true, then the value of charger
depends on bootloader settings and max8997 default values.

Best regards,
Krzysztof

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 4/7] power: supply: max8997_charger: Set CHARGER current limit

[Krzysztof Kozlowski]

On Thu, Dec 24, 2020 at 02:37:06PM +0100, Krzysztof Kozlowski wrote:
> On Thu, Dec 24, 2020 at 01:13:02PM +0000, Timon Baetz wrote:
> > On Thu, 24 Dec 2020 10:55:59 +0100, Krzysztof Kozlowski wrote:
> > > > @@ -170,6 +237,28 @@ static int max8997_battery_probe(struct platform_device *pdev)
> > > >  		return PTR_ERR(charger->battery);
> > > >  	}
> > > >
> > > > +	charger->reg = devm_regulator_get(&pdev->dev, "charger");  
> > > 
> > > Since you do not use get_optional, you will always get a dummy
> > > regulator. In case of error, you should either print it or entirely fail
> > > the probe. Silently continuing makes it difficult to spot errors.
> > > 
> > > Since the driver could operate in case of extcon/regulator error, just
> > > dev_err() so failure will be spotted with dmesg.
> > 
> > I will switch to devm_regulator_get_optional() and print an error on 
> > failure, thanks.
> > 
> > > It will complain on older DTBs because you are introducing incompatible
> > > change, but that's expected. Just correct all other in-tree DTS.
> > 
> > The other 2 in-tree DTS don't have CHARGER regulators. Not sure
> > how to correct those. Should I add muic and charger nodes without a
> > charger-supply? It will still complain in that case.
> 
> +Cc Marek,
> 
> This is why leaving the code as is - devm_regulator_get(), not optional
> - makes sense. Core would provide dummy regulator, so you only have to
> provide MUIC node.
> 
> If you change the code to devm_regulator_get_optional(), you need to add
> everything: the charger regulator, the charger node and MUIC node.
> 
> For Trats, the configuration should be similar as i9100, although I
> don't know the exact values of chargign voltage.
> 
> For Origen, there is no battery, so the power supply should not bind.
> Maybe this could be achieved with "status disabled" for charger node? It
> depends whether MFD will respect such field... If it disables the
> charger, you're done.

I just looked at the MFD code and tested it - it nicely skips disabled
devices. Therefore, for Origen I propose to add disabled nodes for
charger and MUIC because these pins are not connected. No need to add
regulators in such case.

Best regards,
Krzysztof

Re: [PATCH v4 4/7] power: supply: max8997_charger: Set CHARGER current limit

[Krzysztof Kozlowski]

On Thu, Dec 24, 2020 at 02:37:06PM +0100, Krzysztof Kozlowski wrote:
> On Thu, Dec 24, 2020 at 01:13:02PM +0000, Timon Baetz wrote:
> > On Thu, 24 Dec 2020 10:55:59 +0100, Krzysztof Kozlowski wrote:
> > > > @@ -170,6 +237,28 @@ static int max8997_battery_probe(struct platform_device *pdev)
> > > >  		return PTR_ERR(charger->battery);
> > > >  	}
> > > >
> > > > +	charger->reg = devm_regulator_get(&pdev->dev, "charger");  
> > > 
> > > Since you do not use get_optional, you will always get a dummy
> > > regulator. In case of error, you should either print it or entirely fail
> > > the probe. Silently continuing makes it difficult to spot errors.
> > > 
> > > Since the driver could operate in case of extcon/regulator error, just
> > > dev_err() so failure will be spotted with dmesg.
> > 
> > I will switch to devm_regulator_get_optional() and print an error on 
> > failure, thanks.
> > 
> > > It will complain on older DTBs because you are introducing incompatible
> > > change, but that's expected. Just correct all other in-tree DTS.
> > 
> > The other 2 in-tree DTS don't have CHARGER regulators. Not sure
> > how to correct those. Should I add muic and charger nodes without a
> > charger-supply? It will still complain in that case.
> 
> +Cc Marek,
> 
> This is why leaving the code as is - devm_regulator_get(), not optional
> - makes sense. Core would provide dummy regulator, so you only have to
> provide MUIC node.
> 
> If you change the code to devm_regulator_get_optional(), you need to add
> everything: the charger regulator, the charger node and MUIC node.
> 
> For Trats, the configuration should be similar as i9100, although I
> don't know the exact values of chargign voltage.
> 
> For Origen, there is no battery, so the power supply should not bind.
> Maybe this could be achieved with "status disabled" for charger node? It
> depends whether MFD will respect such field... If it disables the
> charger, you're done.

I just looked at the MFD code and tested it - it nicely skips disabled
devices. Therefore, for Origen I propose to add disabled nodes for
charger and MUIC because these pins are not connected. No need to add
regulators in such case.

Best regards,
Krzysztof

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 4/7] power: supply: max8997_charger: Set CHARGER current limit

[Timon Baetz]

On Thu, 24 Dec 2020 15:00:38 +0100, Krzysztof Kozlowski wrote:
> On Thu, Dec 24, 2020 at 02:37:06PM +0100, Krzysztof Kozlowski wrote:
> > On Thu, Dec 24, 2020 at 01:13:02PM +0000, Timon Baetz wrote:  
> > > On Thu, 24 Dec 2020 10:55:59 +0100, Krzysztof Kozlowski wrote:  
> > > > > @@ -170,6 +237,28 @@ static int max8997_battery_probe(struct platform_device *pdev)
> > > > >  		return PTR_ERR(charger->battery);
> > > > >  	}
> > > > >
> > > > > +	charger->reg = devm_regulator_get(&pdev->dev, "charger");  
> > > >
> > > > Since you do not use get_optional, you will always get a dummy
> > > > regulator. In case of error, you should either print it or entirely fail
> > > > the probe. Silently continuing makes it difficult to spot errors.
> > > >
> > > > Since the driver could operate in case of extcon/regulator error, just
> > > > dev_err() so failure will be spotted with dmesg.  
> > >
> > > I will switch to devm_regulator_get_optional() and print an error on
> > > failure, thanks.
> > >  
> > > > It will complain on older DTBs because you are introducing incompatible
> > > > change, but that's expected. Just correct all other in-tree DTS.  
> > >
> > > The other 2 in-tree DTS don't have CHARGER regulators. Not sure
> > > how to correct those. Should I add muic and charger nodes without a
> > > charger-supply? It will still complain in that case.  
> >
> > +Cc Marek,
> >
> > This is why leaving the code as is - devm_regulator_get(), not optional
> > - makes sense. Core would provide dummy regulator, so you only have to
> > provide MUIC node.
> >
> > If you change the code to devm_regulator_get_optional(), you need to add
> > everything: the charger regulator, the charger node and MUIC node.
> >
> > For Trats, the configuration should be similar as i9100, although I
> > don't know the exact values of chargign voltage.
> >
> > For Origen, there is no battery, so the power supply should not bind.
> > Maybe this could be achieved with "status disabled" for charger node? It
> > depends whether MFD will respect such field... If it disables the
> > charger, you're done.  
> 
> I just looked at the MFD code and tested it - it nicely skips disabled
> devices. Therefore, for Origen I propose to add disabled nodes for
> charger and MUIC because these pins are not connected. No need to add
> regulators in such case.

With a dummy regulator regulator_set_current_limit() fails with -EINVAL.
Isn't it better to just skip charging control (and dev_info()) when there 
is no extcon or regulator? The charger driver would still probe
without those 2 properties and work as before.

Adding disabled nodes for Origen would probably still makes sense.

I also noticed that adding nodes for those MFD cells prints "DMA mask
not set" which seems to be related to https://lkml.org/lkml/2020/4/23/873.
Any suggestions on how to handle that?

Thanks,
Timon

Re: [PATCH v4 4/7] power: supply: max8997_charger: Set CHARGER current limit

[Timon Baetz]

On Thu, 24 Dec 2020 15:00:38 +0100, Krzysztof Kozlowski wrote:
> On Thu, Dec 24, 2020 at 02:37:06PM +0100, Krzysztof Kozlowski wrote:
> > On Thu, Dec 24, 2020 at 01:13:02PM +0000, Timon Baetz wrote:  
> > > On Thu, 24 Dec 2020 10:55:59 +0100, Krzysztof Kozlowski wrote:  
> > > > > @@ -170,6 +237,28 @@ static int max8997_battery_probe(struct platform_device *pdev)
> > > > >  		return PTR_ERR(charger->battery);
> > > > >  	}
> > > > >
> > > > > +	charger->reg = devm_regulator_get(&pdev->dev, "charger");  
> > > >
> > > > Since you do not use get_optional, you will always get a dummy
> > > > regulator. In case of error, you should either print it or entirely fail
> > > > the probe. Silently continuing makes it difficult to spot errors.
> > > >
> > > > Since the driver could operate in case of extcon/regulator error, just
> > > > dev_err() so failure will be spotted with dmesg.  
> > >
> > > I will switch to devm_regulator_get_optional() and print an error on
> > > failure, thanks.
> > >  
> > > > It will complain on older DTBs because you are introducing incompatible
> > > > change, but that's expected. Just correct all other in-tree DTS.  
> > >
> > > The other 2 in-tree DTS don't have CHARGER regulators. Not sure
> > > how to correct those. Should I add muic and charger nodes without a
> > > charger-supply? It will still complain in that case.  
> >
> > +Cc Marek,
> >
> > This is why leaving the code as is - devm_regulator_get(), not optional
> > - makes sense. Core would provide dummy regulator, so you only have to
> > provide MUIC node.
> >
> > If you change the code to devm_regulator_get_optional(), you need to add
> > everything: the charger regulator, the charger node and MUIC node.
> >
> > For Trats, the configuration should be similar as i9100, although I
> > don't know the exact values of chargign voltage.
> >
> > For Origen, there is no battery, so the power supply should not bind.
> > Maybe this could be achieved with "status disabled" for charger node? It
> > depends whether MFD will respect such field... If it disables the
> > charger, you're done.  
> 
> I just looked at the MFD code and tested it - it nicely skips disabled
> devices. Therefore, for Origen I propose to add disabled nodes for
> charger and MUIC because these pins are not connected. No need to add
> regulators in such case.

With a dummy regulator regulator_set_current_limit() fails with -EINVAL.
Isn't it better to just skip charging control (and dev_info()) when there 
is no extcon or regulator? The charger driver would still probe
without those 2 properties and work as before.

Adding disabled nodes for Origen would probably still makes sense.

I also noticed that adding nodes for those MFD cells prints "DMA mask
not set" which seems to be related to https://lkml.org/lkml/2020/4/23/873.
Any suggestions on how to handle that?

Thanks,
Timon


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 4/7] power: supply: max8997_charger: Set CHARGER current limit

[Krzysztof Kozlowski]

On Fri, Dec 25, 2020 at 11:33:21AM +0000, Timon Baetz wrote:
> On Thu, 24 Dec 2020 15:00:38 +0100, Krzysztof Kozlowski wrote:
> > On Thu, Dec 24, 2020 at 02:37:06PM +0100, Krzysztof Kozlowski wrote:
> > > On Thu, Dec 24, 2020 at 01:13:02PM +0000, Timon Baetz wrote:  
> > > > On Thu, 24 Dec 2020 10:55:59 +0100, Krzysztof Kozlowski wrote:  
> > > > > > @@ -170,6 +237,28 @@ static int max8997_battery_probe(struct platform_device *pdev)
> > > > > >  		return PTR_ERR(charger->battery);
> > > > > >  	}
> > > > > >
> > > > > > +	charger->reg = devm_regulator_get(&pdev->dev, "charger");  
> > > > >
> > > > > Since you do not use get_optional, you will always get a dummy
> > > > > regulator. In case of error, you should either print it or entirely fail
> > > > > the probe. Silently continuing makes it difficult to spot errors.
> > > > >
> > > > > Since the driver could operate in case of extcon/regulator error, just
> > > > > dev_err() so failure will be spotted with dmesg.  
> > > >
> > > > I will switch to devm_regulator_get_optional() and print an error on
> > > > failure, thanks.
> > > >  
> > > > > It will complain on older DTBs because you are introducing incompatible
> > > > > change, but that's expected. Just correct all other in-tree DTS.  
> > > >
> > > > The other 2 in-tree DTS don't have CHARGER regulators. Not sure
> > > > how to correct those. Should I add muic and charger nodes without a
> > > > charger-supply? It will still complain in that case.  
> > >
> > > +Cc Marek,
> > >
> > > This is why leaving the code as is - devm_regulator_get(), not optional
> > > - makes sense. Core would provide dummy regulator, so you only have to
> > > provide MUIC node.
> > >
> > > If you change the code to devm_regulator_get_optional(), you need to add
> > > everything: the charger regulator, the charger node and MUIC node.
> > >
> > > For Trats, the configuration should be similar as i9100, although I
> > > don't know the exact values of chargign voltage.
> > >
> > > For Origen, there is no battery, so the power supply should not bind.
> > > Maybe this could be achieved with "status disabled" for charger node? It
> > > depends whether MFD will respect such field... If it disables the
> > > charger, you're done.  
> > 
> > I just looked at the MFD code and tested it - it nicely skips disabled
> > devices. Therefore, for Origen I propose to add disabled nodes for
> > charger and MUIC because these pins are not connected. No need to add
> > regulators in such case.
> 
> With a dummy regulator regulator_set_current_limit() fails with -EINVAL.

Good point.

> Isn't it better to just skip charging control (and dev_info()) when there 
> is no extcon or regulator? The charger driver would still probe
> without those 2 properties and work as before.

Yes, makes sense.

> 
> Adding disabled nodes for Origen would probably still makes sense.
> 
> I also noticed that adding nodes for those MFD cells prints "DMA mask
> not set" which seems to be related to https://lkml.org/lkml/2020/4/23/873.
> Any suggestions on how to handle that?

I don't think it is your problem to solve. It affects other MFD devices
as well.

Best regards,
Krzysztof

Re: [PATCH v4 4/7] power: supply: max8997_charger: Set CHARGER current limit

[Krzysztof Kozlowski]

On Fri, Dec 25, 2020 at 11:33:21AM +0000, Timon Baetz wrote:
> On Thu, 24 Dec 2020 15:00:38 +0100, Krzysztof Kozlowski wrote:
> > On Thu, Dec 24, 2020 at 02:37:06PM +0100, Krzysztof Kozlowski wrote:
> > > On Thu, Dec 24, 2020 at 01:13:02PM +0000, Timon Baetz wrote:  
> > > > On Thu, 24 Dec 2020 10:55:59 +0100, Krzysztof Kozlowski wrote:  
> > > > > > @@ -170,6 +237,28 @@ static int max8997_battery_probe(struct platform_device *pdev)
> > > > > >  		return PTR_ERR(charger->battery);
> > > > > >  	}
> > > > > >
> > > > > > +	charger->reg = devm_regulator_get(&pdev->dev, "charger");  
> > > > >
> > > > > Since you do not use get_optional, you will always get a dummy
> > > > > regulator. In case of error, you should either print it or entirely fail
> > > > > the probe. Silently continuing makes it difficult to spot errors.
> > > > >
> > > > > Since the driver could operate in case of extcon/regulator error, just
> > > > > dev_err() so failure will be spotted with dmesg.  
> > > >
> > > > I will switch to devm_regulator_get_optional() and print an error on
> > > > failure, thanks.
> > > >  
> > > > > It will complain on older DTBs because you are introducing incompatible
> > > > > change, but that's expected. Just correct all other in-tree DTS.  
> > > >
> > > > The other 2 in-tree DTS don't have CHARGER regulators. Not sure
> > > > how to correct those. Should I add muic and charger nodes without a
> > > > charger-supply? It will still complain in that case.  
> > >
> > > +Cc Marek,
> > >
> > > This is why leaving the code as is - devm_regulator_get(), not optional
> > > - makes sense. Core would provide dummy regulator, so you only have to
> > > provide MUIC node.
> > >
> > > If you change the code to devm_regulator_get_optional(), you need to add
> > > everything: the charger regulator, the charger node and MUIC node.
> > >
> > > For Trats, the configuration should be similar as i9100, although I
> > > don't know the exact values of chargign voltage.
> > >
> > > For Origen, there is no battery, so the power supply should not bind.
> > > Maybe this could be achieved with "status disabled" for charger node? It
> > > depends whether MFD will respect such field... If it disables the
> > > charger, you're done.  
> > 
> > I just looked at the MFD code and tested it - it nicely skips disabled
> > devices. Therefore, for Origen I propose to add disabled nodes for
> > charger and MUIC because these pins are not connected. No need to add
> > regulators in such case.
> 
> With a dummy regulator regulator_set_current_limit() fails with -EINVAL.

Good point.

> Isn't it better to just skip charging control (and dev_info()) when there 
> is no extcon or regulator? The charger driver would still probe
> without those 2 properties and work as before.

Yes, makes sense.

> 
> Adding disabled nodes for Origen would probably still makes sense.
> 
> I also noticed that adding nodes for those MFD cells prints "DMA mask
> not set" which seems to be related to https://lkml.org/lkml/2020/4/23/873.
> Any suggestions on how to handle that?

I don't think it is your problem to solve. It affects other MFD devices
as well.

Best regards,
Krzysztof

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel