* [PATCH v3 1/5] dt-bindings: mfd: maxim,max77759: reference power-supply schema and add regulator property
2025-12-27 0:04 [PATCH v3 0/5] Introduce MAX77759 charger driver Amit Sunil Dhamne via B4 Relay
@ 2025-12-27 0:04 ` Amit Sunil Dhamne via B4 Relay
2025-12-29 8:50 ` Krzysztof Kozlowski
2026-01-05 16:35 ` André Draszik
2025-12-27 0:04 ` [PATCH v3 2/5] dt-bindings: usb: maxim,max33359: Add supply property for vbus Amit Sunil Dhamne via B4 Relay
` (3 subsequent siblings)
4 siblings, 2 replies; 25+ messages in thread
From: Amit Sunil Dhamne via B4 Relay @ 2025-12-27 0:04 UTC (permalink / raw)
To: Sebastian Reichel, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
André Draszik, Lee Jones, Greg Kroah-Hartman,
Badhri Jagan Sridharan, Heikki Krogerus, Peter Griffin,
Tudor Ambarus, Alim Akhtar
Cc: linux-kernel, linux-pm, devicetree, linux-usb, linux-arm-kernel,
linux-samsung-soc, RD Babiera, Kyle Tso, Amit Sunil Dhamne
From: Amit Sunil Dhamne <amitsd@google.com>
Extend the max77759 binding to reference power-supply schema, so that
PMIC node can reference its supplier. Also, add regulator property to
control CHGIN (OTG) voltage.
Signed-off-by: Amit Sunil Dhamne <amitsd@google.com>
---
.../devicetree/bindings/mfd/maxim,max77759.yaml | 16 +++++++++++++++-
1 file changed, 15 insertions(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/mfd/maxim,max77759.yaml b/Documentation/devicetree/bindings/mfd/maxim,max77759.yaml
index 525de9ab3c2b..42e4a84d5204 100644
--- a/Documentation/devicetree/bindings/mfd/maxim,max77759.yaml
+++ b/Documentation/devicetree/bindings/mfd/maxim,max77759.yaml
@@ -16,6 +16,9 @@ description: |
The MAX77759 includes Battery Charger, Fuel Gauge, temperature sensors, USB
Type-C Port Controller (TCPC), NVMEM, and a GPIO expander.
+allOf:
+ - $ref: /schemas/power/supply/power-supply.yaml#
+
properties:
compatible:
const: maxim,max77759
@@ -37,12 +40,18 @@ properties:
nvmem-0:
$ref: /schemas/nvmem/maxim,max77759-nvmem.yaml
+ chgin-otg-regulator:
+ type: object
+ description: Provides Boost for sourcing VBUS.
+ $ref: /schemas/regulator/regulator.yaml#
+ unevaluatedProperties: false
+
required:
- compatible
- interrupts
- reg
-additionalProperties: false
+unevaluatedProperties: false
examples:
- |
@@ -59,6 +68,11 @@ examples:
interrupt-controller;
#interrupt-cells = <2>;
+ power-supplies = <&maxtcpci>;
+
+ chgin-otg-regulator {
+ regulator-name = "chgin-otg";
+ };
gpio {
compatible = "maxim,max77759-gpio";
--
2.52.0.351.gbe84eed79e-goog
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [PATCH v3 1/5] dt-bindings: mfd: maxim,max77759: reference power-supply schema and add regulator property
2025-12-27 0:04 ` [PATCH v3 1/5] dt-bindings: mfd: maxim,max77759: reference power-supply schema and add regulator property Amit Sunil Dhamne via B4 Relay
@ 2025-12-29 8:50 ` Krzysztof Kozlowski
2026-01-05 16:35 ` André Draszik
1 sibling, 0 replies; 25+ messages in thread
From: Krzysztof Kozlowski @ 2025-12-29 8:50 UTC (permalink / raw)
To: Amit Sunil Dhamne
Cc: Sebastian Reichel, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
André Draszik, Lee Jones, Greg Kroah-Hartman,
Badhri Jagan Sridharan, Heikki Krogerus, Peter Griffin,
Tudor Ambarus, Alim Akhtar, linux-kernel, linux-pm, devicetree,
linux-usb, linux-arm-kernel, linux-samsung-soc, RD Babiera,
Kyle Tso
On Sat, Dec 27, 2025 at 12:04:21AM +0000, Amit Sunil Dhamne wrote:
> Extend the max77759 binding to reference power-supply schema, so that
> PMIC node can reference its supplier. Also, add regulator property to
> control CHGIN (OTG) voltage.
>
> Signed-off-by: Amit Sunil Dhamne <amitsd@google.com>
> ---
> .../devicetree/bindings/mfd/maxim,max77759.yaml | 16 +++++++++++++++-
> 1 file changed, 15 insertions(+), 1 deletion(-)
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v3 1/5] dt-bindings: mfd: maxim,max77759: reference power-supply schema and add regulator property
2025-12-27 0:04 ` [PATCH v3 1/5] dt-bindings: mfd: maxim,max77759: reference power-supply schema and add regulator property Amit Sunil Dhamne via B4 Relay
2025-12-29 8:50 ` Krzysztof Kozlowski
@ 2026-01-05 16:35 ` André Draszik
1 sibling, 0 replies; 25+ messages in thread
From: André Draszik @ 2026-01-05 16:35 UTC (permalink / raw)
To: amitsd, Sebastian Reichel, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Lee Jones, Greg Kroah-Hartman,
Badhri Jagan Sridharan, Heikki Krogerus, Peter Griffin,
Tudor Ambarus, Alim Akhtar
Cc: linux-kernel, linux-pm, devicetree, linux-usb, linux-arm-kernel,
linux-samsung-soc, RD Babiera, Kyle Tso
On Sat, 2025-12-27 at 00:04 +0000, Amit Sunil Dhamne via B4 Relay wrote:
> From: Amit Sunil Dhamne <amitsd@google.com>
>
> Extend the max77759 binding to reference power-supply schema, so that
> PMIC node can reference its supplier. Also, add regulator property to
> control CHGIN (OTG) voltage.
>
> Signed-off-by: Amit Sunil Dhamne <amitsd@google.com>
> ---
> .../devicetree/bindings/mfd/maxim,max77759.yaml | 16 +++++++++++++++-
> 1 file changed, 15 insertions(+), 1 deletion(-)
Reviewed-by: André Draszik <andre.draszik@linaro.org>
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH v3 2/5] dt-bindings: usb: maxim,max33359: Add supply property for vbus
2025-12-27 0:04 [PATCH v3 0/5] Introduce MAX77759 charger driver Amit Sunil Dhamne via B4 Relay
2025-12-27 0:04 ` [PATCH v3 1/5] dt-bindings: mfd: maxim,max77759: reference power-supply schema and add regulator property Amit Sunil Dhamne via B4 Relay
@ 2025-12-27 0:04 ` Amit Sunil Dhamne via B4 Relay
2025-12-27 0:04 ` [PATCH v3 3/5] mfd: max77759: add register bitmasks and modify irq configs for charger Amit Sunil Dhamne via B4 Relay
` (2 subsequent siblings)
4 siblings, 0 replies; 25+ messages in thread
From: Amit Sunil Dhamne via B4 Relay @ 2025-12-27 0:04 UTC (permalink / raw)
To: Sebastian Reichel, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
André Draszik, Lee Jones, Greg Kroah-Hartman,
Badhri Jagan Sridharan, Heikki Krogerus, Peter Griffin,
Tudor Ambarus, Alim Akhtar
Cc: linux-kernel, linux-pm, devicetree, linux-usb, linux-arm-kernel,
linux-samsung-soc, RD Babiera, Kyle Tso, Amit Sunil Dhamne,
Krzysztof Kozlowski
From: Amit Sunil Dhamne <amitsd@google.com>
Add a regulator supply property for vbus. This notifies the regulator
provider to source vbus when Type-C operates in Source power mode,
while turn off sourcing vbus when operating in Sink mode or
disconnected.
Signed-off-by: Amit Sunil Dhamne <amitsd@google.com>
Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>
---
Documentation/devicetree/bindings/usb/maxim,max33359.yaml | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/Documentation/devicetree/bindings/usb/maxim,max33359.yaml b/Documentation/devicetree/bindings/usb/maxim,max33359.yaml
index 3de4dc40b791..e652a24902ea 100644
--- a/Documentation/devicetree/bindings/usb/maxim,max33359.yaml
+++ b/Documentation/devicetree/bindings/usb/maxim,max33359.yaml
@@ -32,6 +32,9 @@ properties:
description:
Properties for usb c connector.
+ vbus-supply:
+ description: Regulator to control sourcing Vbus.
+
required:
- compatible
- reg
@@ -53,6 +56,7 @@ examples:
reg = <0x25>;
interrupt-parent = <&gpa8>;
interrupts = <2 IRQ_TYPE_LEVEL_LOW>;
+ vbus-supply = <&chgin_otg_reg>;
connector {
compatible = "usb-c-connector";
--
2.52.0.351.gbe84eed79e-goog
^ permalink raw reply related [flat|nested] 25+ messages in thread* [PATCH v3 3/5] mfd: max77759: add register bitmasks and modify irq configs for charger
2025-12-27 0:04 [PATCH v3 0/5] Introduce MAX77759 charger driver Amit Sunil Dhamne via B4 Relay
2025-12-27 0:04 ` [PATCH v3 1/5] dt-bindings: mfd: maxim,max77759: reference power-supply schema and add regulator property Amit Sunil Dhamne via B4 Relay
2025-12-27 0:04 ` [PATCH v3 2/5] dt-bindings: usb: maxim,max33359: Add supply property for vbus Amit Sunil Dhamne via B4 Relay
@ 2025-12-27 0:04 ` Amit Sunil Dhamne via B4 Relay
2026-01-05 16:45 ` André Draszik
2025-12-27 0:04 ` [PATCH v3 4/5] power: supply: max77759: add charger driver Amit Sunil Dhamne via B4 Relay
2025-12-27 0:04 ` [PATCH v3 5/5] usb: typec: tcpm/tcpci_maxim: deprecate WAR for setting charger mode Amit Sunil Dhamne via B4 Relay
4 siblings, 1 reply; 25+ messages in thread
From: Amit Sunil Dhamne via B4 Relay @ 2025-12-27 0:04 UTC (permalink / raw)
To: Sebastian Reichel, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
André Draszik, Lee Jones, Greg Kroah-Hartman,
Badhri Jagan Sridharan, Heikki Krogerus, Peter Griffin,
Tudor Ambarus, Alim Akhtar
Cc: linux-kernel, linux-pm, devicetree, linux-usb, linux-arm-kernel,
linux-samsung-soc, RD Babiera, Kyle Tso, Amit Sunil Dhamne
From: Amit Sunil Dhamne <amitsd@google.com>
Add register bitmasks for charger function.
In addition split the charger IRQs further such that each bit represents
an IRQ downstream of charger regmap irq chip. In addition populate the
ack_base to offload irq ack to the regmap irq chip framework.
Signed-off-by: Amit Sunil Dhamne <amitsd@google.com>
---
drivers/mfd/max77759.c | 91 +++++++++++++++++--
include/linux/mfd/max77759.h | 202 ++++++++++++++++++++++++++++++++++++-------
2 files changed, 256 insertions(+), 37 deletions(-)
diff --git a/drivers/mfd/max77759.c b/drivers/mfd/max77759.c
index 6cf6306c4a3b..a5f7da003edd 100644
--- a/drivers/mfd/max77759.c
+++ b/drivers/mfd/max77759.c
@@ -201,8 +201,24 @@ static const struct regmap_config max77759_regmap_config_charger = {
* - SYSUVLO_INT
* - FSHIP_NOT_RD
* - CHGR_INT: charger
- * - CHG_INT
- * - CHG_INT2
+ * - INT1
+ * - AICL
+ * - CHGIN
+ * - WCIN
+ * - CHG
+ * - BAT
+ * - INLIM
+ * - THM2
+ * - BYP
+ * - INT2
+ * - INSEL
+ * - SYS_UVLO1
+ * - SYS_UVLO2
+ * - BAT_OILO
+ * - CHG_STA_CC
+ * - CHG_STA_CV
+ * - CHG_STA_TO
+ * - CHG_STA_DONE
*/
enum {
MAX77759_INT_MAXQ,
@@ -228,8 +244,22 @@ enum {
};
enum {
- MAX77759_CHARGER_INT_1,
- MAX77759_CHARGER_INT_2,
+ MAX77759_CHGR_INT1_AICL,
+ MAX77759_CHGR_INT1_CHGIN,
+ MAX77759_CHGR_INT1_WCIN,
+ MAX77759_CHGR_INT1_CHG,
+ MAX77759_CHGR_INT1_BAT,
+ MAX77759_CHGR_INT1_INLIM,
+ MAX77759_CHGR_INT1_THM2,
+ MAX77759_CHGR_INT1_BYP,
+ MAX77759_CHGR_INT2_INSEL,
+ MAX77759_CHGR_INT2_SYS_UVLO1,
+ MAX77759_CHGR_INT2_SYS_UVLO2,
+ MAX77759_CHGR_INT2_BAT_OILO,
+ MAX77759_CHGR_INT2_CHG_STA_CC,
+ MAX77759_CHGR_INT2_CHG_STA_CV,
+ MAX77759_CHGR_INT2_CHG_STA_TO,
+ MAX77759_CHGR_INT2_CHG_STA_DONE,
};
static const struct regmap_irq max77759_pmic_irqs[] = {
@@ -256,8 +286,38 @@ static const struct regmap_irq max77759_topsys_irqs[] = {
};
static const struct regmap_irq max77759_chgr_irqs[] = {
- REGMAP_IRQ_REG(MAX77759_CHARGER_INT_1, 0, GENMASK(7, 0)),
- REGMAP_IRQ_REG(MAX77759_CHARGER_INT_2, 1, GENMASK(7, 0)),
+ REGMAP_IRQ_REG(MAX77759_CHGR_INT1_AICL, 0,
+ MAX77759_CHGR_REG_CHG_INT_AICL),
+ REGMAP_IRQ_REG(MAX77759_CHGR_INT1_CHGIN, 0,
+ MAX77759_CHGR_REG_CHG_INT_CHGIN),
+ REGMAP_IRQ_REG(MAX77759_CHGR_INT1_WCIN, 0,
+ MAX77759_CHGR_REG_CHG_INT_WCIN),
+ REGMAP_IRQ_REG(MAX77759_CHGR_INT1_CHG, 0,
+ MAX77759_CHGR_REG_CHG_INT_CHG),
+ REGMAP_IRQ_REG(MAX77759_CHGR_INT1_BAT, 0,
+ MAX77759_CHGR_REG_CHG_INT_BAT),
+ REGMAP_IRQ_REG(MAX77759_CHGR_INT1_INLIM, 0,
+ MAX77759_CHGR_REG_CHG_INT_INLIM),
+ REGMAP_IRQ_REG(MAX77759_CHGR_INT1_THM2, 0,
+ MAX77759_CHGR_REG_CHG_INT_THM2),
+ REGMAP_IRQ_REG(MAX77759_CHGR_INT1_BYP, 0,
+ MAX77759_CHGR_REG_CHG_INT_BYP),
+ REGMAP_IRQ_REG(MAX77759_CHGR_INT2_INSEL, 1,
+ MAX77759_CHGR_REG_CHG_INT2_INSEL),
+ REGMAP_IRQ_REG(MAX77759_CHGR_INT2_SYS_UVLO1, 1,
+ MAX77759_CHGR_REG_CHG_INT2_SYS_UVLO1),
+ REGMAP_IRQ_REG(MAX77759_CHGR_INT2_SYS_UVLO2, 1,
+ MAX77759_CHGR_REG_CHG_INT2_SYS_UVLO2),
+ REGMAP_IRQ_REG(MAX77759_CHGR_INT2_BAT_OILO, 1,
+ MAX77759_CHGR_REG_CHG_INT2_BAT_OILO),
+ REGMAP_IRQ_REG(MAX77759_CHGR_INT2_CHG_STA_CC, 1,
+ MAX77759_CHGR_REG_CHG_INT2_CHG_STA_CC),
+ REGMAP_IRQ_REG(MAX77759_CHGR_INT2_CHG_STA_CV, 1,
+ MAX77759_CHGR_REG_CHG_INT2_CHG_STA_CV),
+ REGMAP_IRQ_REG(MAX77759_CHGR_INT2_CHG_STA_TO, 1,
+ MAX77759_CHGR_REG_CHG_INT2_CHG_STA_TO),
+ REGMAP_IRQ_REG(MAX77759_CHGR_INT2_CHG_STA_DONE, 1,
+ MAX77759_CHGR_REG_CHG_INT2_CHG_STA_DONE),
};
static const struct regmap_irq_chip max77759_pmic_irq_chip = {
@@ -302,6 +362,7 @@ static const struct regmap_irq_chip max77759_chrg_irq_chip = {
.domain_suffix = "CHGR",
.status_base = MAX77759_CHGR_REG_CHG_INT,
.mask_base = MAX77759_CHGR_REG_CHG_INT_MASK,
+ .ack_base = MAX77759_CHGR_REG_CHG_INT,
.num_regs = 2,
.irqs = max77759_chgr_irqs,
.num_irqs = ARRAY_SIZE(max77759_chgr_irqs),
@@ -325,8 +386,22 @@ static const struct resource max77759_gpio_resources[] = {
};
static const struct resource max77759_charger_resources[] = {
- DEFINE_RES_IRQ_NAMED(MAX77759_CHARGER_INT_1, "INT1"),
- DEFINE_RES_IRQ_NAMED(MAX77759_CHARGER_INT_2, "INT2"),
+ DEFINE_RES_IRQ_NAMED(MAX77759_CHGR_INT1_AICL, "AICL"),
+ DEFINE_RES_IRQ_NAMED(MAX77759_CHGR_INT1_CHGIN, "CHGIN"),
+ DEFINE_RES_IRQ_NAMED(MAX77759_CHGR_INT1_WCIN, "WCIN"),
+ DEFINE_RES_IRQ_NAMED(MAX77759_CHGR_INT1_CHG, "CHG"),
+ DEFINE_RES_IRQ_NAMED(MAX77759_CHGR_INT1_BAT, "BAT"),
+ DEFINE_RES_IRQ_NAMED(MAX77759_CHGR_INT1_INLIM, "INLIM"),
+ DEFINE_RES_IRQ_NAMED(MAX77759_CHGR_INT1_THM2, "THM2"),
+ DEFINE_RES_IRQ_NAMED(MAX77759_CHGR_INT1_BYP, "BYP"),
+ DEFINE_RES_IRQ_NAMED(MAX77759_CHGR_INT2_INSEL, "INSEL"),
+ DEFINE_RES_IRQ_NAMED(MAX77759_CHGR_INT2_SYS_UVLO1, "SYS_UVLO1"),
+ DEFINE_RES_IRQ_NAMED(MAX77759_CHGR_INT2_SYS_UVLO2, "SYS_UVLO2"),
+ DEFINE_RES_IRQ_NAMED(MAX77759_CHGR_INT2_BAT_OILO, "BAT_OILO"),
+ DEFINE_RES_IRQ_NAMED(MAX77759_CHGR_INT2_CHG_STA_CC, "CHG_STA_CC"),
+ DEFINE_RES_IRQ_NAMED(MAX77759_CHGR_INT2_CHG_STA_CV, "CHG_STA_CV"),
+ DEFINE_RES_IRQ_NAMED(MAX77759_CHGR_INT2_CHG_STA_TO, "CHG_STA_TO"),
+ DEFINE_RES_IRQ_NAMED(MAX77759_CHGR_INT2_CHG_STA_DONE, "CHG_STA_DONE"),
};
static const struct mfd_cell max77759_cells[] = {
diff --git a/include/linux/mfd/max77759.h b/include/linux/mfd/max77759.h
index c6face34e385..e674a519e782 100644
--- a/include/linux/mfd/max77759.h
+++ b/include/linux/mfd/max77759.h
@@ -59,35 +59,65 @@
#define MAX77759_MAXQ_REG_AP_DATAIN0 0xb1
#define MAX77759_MAXQ_REG_UIC_SWRST 0xe0
-#define MAX77759_CHGR_REG_CHG_INT 0xb0
-#define MAX77759_CHGR_REG_CHG_INT2 0xb1
-#define MAX77759_CHGR_REG_CHG_INT_MASK 0xb2
-#define MAX77759_CHGR_REG_CHG_INT2_MASK 0xb3
-#define MAX77759_CHGR_REG_CHG_INT_OK 0xb4
-#define MAX77759_CHGR_REG_CHG_DETAILS_00 0xb5
-#define MAX77759_CHGR_REG_CHG_DETAILS_01 0xb6
-#define MAX77759_CHGR_REG_CHG_DETAILS_02 0xb7
-#define MAX77759_CHGR_REG_CHG_DETAILS_03 0xb8
-#define MAX77759_CHGR_REG_CHG_CNFG_00 0xb9
-#define MAX77759_CHGR_REG_CHG_CNFG_01 0xba
-#define MAX77759_CHGR_REG_CHG_CNFG_02 0xbb
-#define MAX77759_CHGR_REG_CHG_CNFG_03 0xbc
-#define MAX77759_CHGR_REG_CHG_CNFG_04 0xbd
-#define MAX77759_CHGR_REG_CHG_CNFG_05 0xbe
-#define MAX77759_CHGR_REG_CHG_CNFG_06 0xbf
-#define MAX77759_CHGR_REG_CHG_CNFG_07 0xc0
-#define MAX77759_CHGR_REG_CHG_CNFG_08 0xc1
-#define MAX77759_CHGR_REG_CHG_CNFG_09 0xc2
-#define MAX77759_CHGR_REG_CHG_CNFG_10 0xc3
-#define MAX77759_CHGR_REG_CHG_CNFG_11 0xc4
-#define MAX77759_CHGR_REG_CHG_CNFG_12 0xc5
-#define MAX77759_CHGR_REG_CHG_CNFG_13 0xc6
-#define MAX77759_CHGR_REG_CHG_CNFG_14 0xc7
-#define MAX77759_CHGR_REG_CHG_CNFG_15 0xc8
-#define MAX77759_CHGR_REG_CHG_CNFG_16 0xc9
-#define MAX77759_CHGR_REG_CHG_CNFG_17 0xca
-#define MAX77759_CHGR_REG_CHG_CNFG_18 0xcb
-#define MAX77759_CHGR_REG_CHG_CNFG_19 0xcc
+#define MAX77759_CHGR_REG_CHG_INT 0xb0
+#define MAX77759_CHGR_REG_CHG_INT_AICL BIT(7)
+#define MAX77759_CHGR_REG_CHG_INT_CHGIN BIT(6)
+#define MAX77759_CHGR_REG_CHG_INT_WCIN BIT(5)
+#define MAX77759_CHGR_REG_CHG_INT_CHG BIT(4)
+#define MAX77759_CHGR_REG_CHG_INT_BAT BIT(3)
+#define MAX77759_CHGR_REG_CHG_INT_INLIM BIT(2)
+#define MAX77759_CHGR_REG_CHG_INT_THM2 BIT(1)
+#define MAX77759_CHGR_REG_CHG_INT_BYP BIT(0)
+#define MAX77759_CHGR_REG_CHG_INT2 0xb1
+#define MAX77759_CHGR_REG_CHG_INT2_INSEL BIT(7)
+#define MAX77759_CHGR_REG_CHG_INT2_SYS_UVLO1 BIT(6)
+#define MAX77759_CHGR_REG_CHG_INT2_SYS_UVLO2 BIT(5)
+#define MAX77759_CHGR_REG_CHG_INT2_BAT_OILO BIT(4)
+#define MAX77759_CHGR_REG_CHG_INT2_CHG_STA_CC BIT(3)
+#define MAX77759_CHGR_REG_CHG_INT2_CHG_STA_CV BIT(2)
+#define MAX77759_CHGR_REG_CHG_INT2_CHG_STA_TO BIT(1)
+#define MAX77759_CHGR_REG_CHG_INT2_CHG_STA_DONE BIT(0)
+#define MAX77759_CHGR_REG_CHG_INT_MASK 0xb2
+#define MAX77759_CHGR_REG_CHG_INT2_MASK 0xb3
+#define MAX77759_CHGR_REG_CHG_INT_OK 0xb4
+#define MAX77759_CHGR_REG_CHG_DETAILS_00 0xb5
+#define MAX77759_CHGR_REG_CHG_DETAILS_OO_CHGIN_DTLS GENMASK(6, 5)
+#define MAX77759_CHGR_REG_CHG_DETAILS_01 0xb6
+#define MAX77759_CHGR_REG_CHG_DETAILS_01_BAT_DTLS GENMASK(6, 4)
+#define MAX77759_CHGR_REG_CHG_DETAILS_01_CHG_DTLS GENMASK(3, 0)
+#define MAX77759_CHGR_REG_CHG_DETAILS_02 0xb7
+#define MAX77759_CHGR_REG_CHG_DETAILS_02_CHGIN_STS BIT(5)
+#define MAX77759_CHGR_REG_CHG_DETAILS_03 0xb8
+#define MAX77759_CHGR_REG_CHG_CNFG_00 0xb9
+#define MAX77759_CHGR_REG_CHG_CNFG_00_MODE GENMASK(3, 0)
+#define MAX77759_CHGR_REG_CHG_CNFG_01 0xba
+#define MAX77759_CHGR_REG_CHG_CNFG_02 0xbb
+#define MAX77759_CHGR_REG_CHG_CNFG_02_CHGCC GENMASK(5, 0)
+#define MAX77759_CHGR_REG_CHG_CNFG_03 0xbc
+#define MAX77759_CHGR_REG_CHG_CNFG_04 0xbd
+#define MAX77759_CHGR_REG_CHG_CNFG_04_CHG_CV_PRM GENMASK(5, 0)
+#define MAX77759_CHGR_REG_CHG_CNFG_05 0xbe
+#define MAX77759_CHGR_REG_CHG_CNFG_06 0xbf
+#define MAX77759_CHGR_REG_CHG_CNFG_06_CHGPROT GENMASK(3, 2)
+#define MAX77759_CHGR_REG_CHG_CNFG_07 0xc0
+#define MAX77759_CHGR_REG_CHG_CNFG_08 0xc1
+#define MAX77759_CHGR_REG_CHG_CNFG_09 0xc2
+#define MAX77759_CHGR_REG_CHG_CNFG_09_CHGIN_ILIM GENMASK(6, 0)
+#define MAX77759_CHGR_REG_CHG_CNFG_10 0xc3
+#define MAX77759_CHGR_REG_CHG_CNFG_11 0xc4
+#define MAX77759_CHGR_REG_CHG_CNFG_12 0xc5
+/* Wireless Charging input channel select */
+#define MAX77759_CHGR_REG_CHG_CNFG_12_WCINSEL BIT(6)
+/* CHGIN/USB input channel select */
+#define MAX77759_CHGR_REG_CHG_CNFG_12_CHGINSEL BIT(5)
+#define MAX77759_CHGR_REG_CHG_CNFG_13 0xc6
+#define MAX77759_CHGR_REG_CHG_CNFG_14 0xc7
+#define MAX77759_CHGR_REG_CHG_CNFG_15 0xc8
+#define MAX77759_CHGR_REG_CHG_CNFG_16 0xc9
+#define MAX77759_CHGR_REG_CHG_CNFG_17 0xca
+#define MAX77759_CHGR_REG_CHG_CNFG_18 0xcb
+#define MAX77759_CHGR_REG_CHG_CNFG_18_WDTEN BIT(0)
+#define MAX77759_CHGR_REG_CHG_CNFG_19 0xcc
/* MaxQ opcodes for max77759_maxq_command() */
#define MAX77759_MAXQ_OPCODE_MAXLENGTH (MAX77759_MAXQ_REG_AP_DATAOUT32 - \
@@ -101,6 +131,120 @@
#define MAX77759_MAXQ_OPCODE_USER_SPACE_READ 0x81
#define MAX77759_MAXQ_OPCODE_USER_SPACE_WRITE 0x82
+/*
+ * Charger Input Status
+ * @MAX77759_CHGR_CHGIN_DTLS_VBUS_UNDERVOLTAGE:
+ * Charger input voltage (Vchgin) < Under Voltage Threshold (Vuvlo)
+ * @MAX77759_CHGR_CHGIN_DTLS_VBUS_MARGINAL_VOLTAGE: Vchgin > Vuvlo and
+ * Vchgin < (Battery Voltage (Vbatt) + system voltage (Vsys))
+ * @MAX77759_CHGR_CHGIN_DTLS_VBUS_OVERVOLTAGE:
+ * Vchgin > Over Voltage threshold (Vovlo)
+ * @MAX77759_CHGR_CHGIN_DTLS_VBUS_VALID:
+ * Vchgin > Vuvlo, Vchgin < Vovlo and Vchgin > (Vsys + Vbatt)
+ */
+enum max77759_chgr_chgin_dtls_status {
+ MAX77759_CHGR_CHGIN_DTLS_VBUS_UNDERVOLTAGE,
+ MAX77759_CHGR_CHGIN_DTLS_VBUS_MARGINAL_VOLTAGE,
+ MAX77759_CHGR_CHGIN_DTLS_VBUS_OVERVOLTAGE,
+ MAX77759_CHGR_CHGIN_DTLS_VBUS_VALID,
+};
+
+/*
+ * Battery Details
+ * @MAX77759_CHGR_BAT_DTLS_NO_BATT_CHG_SUSP:
+ * No battery and the charger suspended
+ * @MAX77759_CHGR_BAT_DTLS_DEAD_BATTERY: Vbatt < Vtrickle
+ * @MAX77759_CHGR_BAT_DTLS_BAT_CHG_TIMER_FAULT:
+ * Charging suspended due to timer fault
+ * @MAX77759_CHGR_BAT_DTLS_BAT_OKAY:
+ * Battery okay and Vbatt > Min Sys Voltage (Vsysmin)
+ * @MAX77759_CHGR_BAT_DTLS_BAT_UNDERVOLTAGE:
+ * Battery is okay. Vtrickle < Vbatt < Vsysmin
+ * @MAX77759_CHGR_BAT_DTLS_BAT_OVERVOLTAGE:
+ * Battery voltage > Overvoltage threshold
+ * @MAX77759_CHGR_BAT_DTLS_BAT_OVERCURRENT:
+ * Battery current exceeds overcurrent threshold
+ * @MAX77759_CHGR_BAT_DTLS_BAT_ONLY_MODE:
+ * Battery only mode and battery level not available
+ */
+enum max77759_chgr_bat_dtls_states {
+ MAX77759_CHGR_BAT_DTLS_NO_BATT_CHG_SUSP,
+ MAX77759_CHGR_BAT_DTLS_DEAD_BATTERY,
+ MAX77759_CHGR_BAT_DTLS_BAT_CHG_TIMER_FAULT,
+ MAX77759_CHGR_BAT_DTLS_BAT_OKAY,
+ MAX77759_CHGR_BAT_DTLS_BAT_UNDERVOLTAGE,
+ MAX77759_CHGR_BAT_DTLS_BAT_OVERVOLTAGE,
+ MAX77759_CHGR_BAT_DTLS_BAT_OVERCURRENT,
+ MAX77759_CHGR_BAT_DTLS_BAT_ONLY_MODE,
+};
+
+/*
+ * Charger Details
+ * @MAX77759_CHGR_CHG_DTLS_PREQUAL: Charger in prequalification mode
+ * @MAX77759_CHGR_CHG_DTLS_CC: Charger in fast charge const curr mode
+ * @MAX77759_CHGR_CHG_DTLS_CV: Charger in fast charge const voltage mode
+ * @MAX77759_CHGR_CHG_DTLS_TO: Charger is in top off mode
+ * @MAX77759_CHGR_CHG_DTLS_DONE: Charger is done
+ * @MAX77759_CHGR_CHG_DTLS_RSVD_1: Reserved
+ * @MAX77759_CHGR_CHG_DTLS_TIMER_FAULT: Charger is in timer fault mode
+ * @MAX77759_CHGR_CHG_DTLS_SUSP_BATT_THM:
+ * Charger is suspended as bettery removal detected
+ * @MAX77759_CHGR_CHG_DTLS_OFF:
+ * Charger is off. Input invalid or charger disabled
+ * @MAX77759_CHGR_CHG_DTLS_RSVD_2: Reserved
+ * @MAX77759_CHGR_CHG_DTLS_RSVD_3: Reserved
+ * @MAX77759_CHGR_CHG_DTLS_OFF_WDOG_TIMER:
+ * Charger is off as watchdog timer expired
+ * @MAX77759_CHGR_CHG_DTLS_SUSP_JEITA: Charger is in JEITA control mode
+ */
+enum max77759_chgr_chg_dtls_states {
+ MAX77759_CHGR_CHG_DTLS_PREQUAL,
+ MAX77759_CHGR_CHG_DTLS_CC,
+ MAX77759_CHGR_CHG_DTLS_CV,
+ MAX77759_CHGR_CHG_DTLS_TO,
+ MAX77759_CHGR_CHG_DTLS_DONE,
+ MAX77759_CHGR_CHG_DTLS_RSVD_1,
+ MAX77759_CHGR_CHG_DTLS_TIMER_FAULT,
+ MAX77759_CHGR_CHG_DTLS_SUSP_BATT_THM,
+ MAX77759_CHGR_CHG_DTLS_OFF,
+ MAX77759_CHGR_CHG_DTLS_RSVD_2,
+ MAX77759_CHGR_CHG_DTLS_RSVD_3,
+ MAX77759_CHGR_CHG_DTLS_OFF_WDOG_TIMER,
+ MAX77759_CHGR_CHG_DTLS_SUSP_JEITA,
+};
+
+enum max77759_chgr_mode {
+ MAX77759_CHGR_MODE_OFF,
+ MAX77759_CHGR_MODE_CHG_BUCK_ON = 0x5,
+ MAX77759_CHGR_MODE_OTG_BOOST_ON = 0xA,
+};
+
+/* Fast charge current limits */
+#define MAX77759_CHGR_CHGCC_MIN_UA 133330
+#define MAX77759_CHGR_CHGCC_MAX_UA 4000000
+#define MAX77759_CHGR_CHGCC_STEP_UA 66670
+#define MAX77759_CHGR_CHGCC_REG_OFFSET 0x2
+
+/* Charge Termination Voltage Limits (in 2 ranges) */
+/* [3.8, 3.9] V range */
+#define MAX77759_CHGR_CHG_CV_PRM_LO_MIN_MV 3800
+#define MAX77759_CHGR_CHG_CV_PRM_LO_MAX_MV 3900
+#define MAX77759_CHGR_CHG_CV_PRM_LO_STEP_MV 100
+#define MAX77759_CHGR_CHG_CV_PRM_LO_MIN_REG 0x38
+#define MAX77759_CHGR_CHG_CV_PRM_LO_MAX_REG 0x39
+/* [4, 4.5] V range */
+#define MAX77759_CHGR_CHG_CV_PRM_HI_MIN_MV 4000
+#define MAX77759_CHGR_CHG_CV_PRM_HI_MAX_MV 4500
+#define MAX77759_CHGR_CHG_CV_PRM_HI_STEP_MV 10
+#define MAX77759_CHGR_CHG_CV_PRM_HI_MIN_REG 0x0
+#define MAX77759_CHGR_CHG_CV_PRM_HI_MAX_REG 0x32
+
+/* USB input current limits */
+#define MAX77759_CHGR_CHGIN_ILIM_MIN_UA 100000
+#define MAX77759_CHGR_CHGIN_ILIM_MAX_UA 3200000
+#define MAX77759_CHGR_CHGIN_ILIM_STEP_UA 25000
+#define MAX77759_CHGR_CHGIN_ILIM_REG_OFFSET 0x3
+
/**
* struct max77759 - core max77759 internal data structure
*
--
2.52.0.351.gbe84eed79e-goog
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [PATCH v3 3/5] mfd: max77759: add register bitmasks and modify irq configs for charger
2025-12-27 0:04 ` [PATCH v3 3/5] mfd: max77759: add register bitmasks and modify irq configs for charger Amit Sunil Dhamne via B4 Relay
@ 2026-01-05 16:45 ` André Draszik
2026-01-05 19:58 ` Amit Sunil Dhamne
0 siblings, 1 reply; 25+ messages in thread
From: André Draszik @ 2026-01-05 16:45 UTC (permalink / raw)
To: amitsd, Sebastian Reichel, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Lee Jones, Greg Kroah-Hartman,
Badhri Jagan Sridharan, Heikki Krogerus, Peter Griffin,
Tudor Ambarus, Alim Akhtar
Cc: linux-kernel, linux-pm, devicetree, linux-usb, linux-arm-kernel,
linux-samsung-soc, RD Babiera, Kyle Tso
On Sat, 2025-12-27 at 00:04 +0000, Amit Sunil Dhamne via B4 Relay wrote:
> From: Amit Sunil Dhamne <amitsd@google.com>
>
> Add register bitmasks for charger function.
> In addition split the charger IRQs further such that each bit represents
> an IRQ downstream of charger regmap irq chip. In addition populate the
> ack_base to offload irq ack to the regmap irq chip framework.
>
> Signed-off-by: Amit Sunil Dhamne <amitsd@google.com>
> ---
> drivers/mfd/max77759.c | 91 +++++++++++++++++--
> include/linux/mfd/max77759.h | 202 ++++++++++++++++++++++++++++++++++++-------
> 2 files changed, 256 insertions(+), 37 deletions(-)
>
> [...]
>
> diff --git a/include/linux/mfd/max77759.h b/include/linux/mfd/max77759.h
> index c6face34e385..e674a519e782 100644
> --- a/include/linux/mfd/max77759.h
> +++ b/include/linux/mfd/max77759.h
> @@ -59,35 +59,65 @@
> #define MAX77759_MAXQ_REG_AP_DATAIN0 0xb1
> #define MAX77759_MAXQ_REG_UIC_SWRST 0xe0
>
> -#define MAX77759_CHGR_REG_CHG_INT 0xb0
> -#define MAX77759_CHGR_REG_CHG_INT2 0xb1
> -#define MAX77759_CHGR_REG_CHG_INT_MASK 0xb2
> -#define MAX77759_CHGR_REG_CHG_INT2_MASK 0xb3
> -#define MAX77759_CHGR_REG_CHG_INT_OK 0xb4
> -#define MAX77759_CHGR_REG_CHG_DETAILS_00 0xb5
> -#define MAX77759_CHGR_REG_CHG_DETAILS_01 0xb6
> -#define MAX77759_CHGR_REG_CHG_DETAILS_02 0xb7
> -#define MAX77759_CHGR_REG_CHG_DETAILS_03 0xb8
> -#define MAX77759_CHGR_REG_CHG_CNFG_00 0xb9
> -#define MAX77759_CHGR_REG_CHG_CNFG_01 0xba
> -#define MAX77759_CHGR_REG_CHG_CNFG_02 0xbb
> -#define MAX77759_CHGR_REG_CHG_CNFG_03 0xbc
> -#define MAX77759_CHGR_REG_CHG_CNFG_04 0xbd
> -#define MAX77759_CHGR_REG_CHG_CNFG_05 0xbe
> -#define MAX77759_CHGR_REG_CHG_CNFG_06 0xbf
> -#define MAX77759_CHGR_REG_CHG_CNFG_07 0xc0
> -#define MAX77759_CHGR_REG_CHG_CNFG_08 0xc1
> -#define MAX77759_CHGR_REG_CHG_CNFG_09 0xc2
> -#define MAX77759_CHGR_REG_CHG_CNFG_10 0xc3
> -#define MAX77759_CHGR_REG_CHG_CNFG_11 0xc4
> -#define MAX77759_CHGR_REG_CHG_CNFG_12 0xc5
> -#define MAX77759_CHGR_REG_CHG_CNFG_13 0xc6
> -#define MAX77759_CHGR_REG_CHG_CNFG_14 0xc7
> -#define MAX77759_CHGR_REG_CHG_CNFG_15 0xc8
> -#define MAX77759_CHGR_REG_CHG_CNFG_16 0xc9
> -#define MAX77759_CHGR_REG_CHG_CNFG_17 0xca
> -#define MAX77759_CHGR_REG_CHG_CNFG_18 0xcb
> -#define MAX77759_CHGR_REG_CHG_CNFG_19 0xcc
> +#define MAX77759_CHGR_REG_CHG_INT 0xb0
> +#define MAX77759_CHGR_REG_CHG_INT_AICL BIT(7)
> +#define MAX77759_CHGR_REG_CHG_INT_CHGIN BIT(6)
> +#define MAX77759_CHGR_REG_CHG_INT_WCIN BIT(5)
> +#define MAX77759_CHGR_REG_CHG_INT_CHG BIT(4)
> +#define MAX77759_CHGR_REG_CHG_INT_BAT BIT(3)
> +#define MAX77759_CHGR_REG_CHG_INT_INLIM BIT(2)
> +#define MAX77759_CHGR_REG_CHG_INT_THM2 BIT(1)
> +#define MAX77759_CHGR_REG_CHG_INT_BYP BIT(0)
> +#define MAX77759_CHGR_REG_CHG_INT2 0xb1
> +#define MAX77759_CHGR_REG_CHG_INT2_INSEL BIT(7)
> +#define MAX77759_CHGR_REG_CHG_INT2_SYS_UVLO1 BIT(6)
> +#define MAX77759_CHGR_REG_CHG_INT2_SYS_UVLO2 BIT(5)
> +#define MAX77759_CHGR_REG_CHG_INT2_BAT_OILO BIT(4)
> +#define MAX77759_CHGR_REG_CHG_INT2_CHG_STA_CC BIT(3)
> +#define MAX77759_CHGR_REG_CHG_INT2_CHG_STA_CV BIT(2)
> +#define MAX77759_CHGR_REG_CHG_INT2_CHG_STA_TO BIT(1)
> +#define MAX77759_CHGR_REG_CHG_INT2_CHG_STA_DONE BIT(0)
> +#define MAX77759_CHGR_REG_CHG_INT_MASK 0xb2
> +#define MAX77759_CHGR_REG_CHG_INT2_MASK 0xb3
> +#define MAX77759_CHGR_REG_CHG_INT_OK 0xb4
> +#define MAX77759_CHGR_REG_CHG_DETAILS_00 0xb5
> +#define MAX77759_CHGR_REG_CHG_DETAILS_OO_CHGIN_DTLS GENMASK(6, 5)
> +#define MAX77759_CHGR_REG_CHG_DETAILS_01 0xb6
> +#define MAX77759_CHGR_REG_CHG_DETAILS_01_BAT_DTLS GENMASK(6, 4)
> +#define MAX77759_CHGR_REG_CHG_DETAILS_01_CHG_DTLS GENMASK(3, 0)
> +#define MAX77759_CHGR_REG_CHG_DETAILS_02 0xb7
> +#define MAX77759_CHGR_REG_CHG_DETAILS_02_CHGIN_STS BIT(5)
> +#define MAX77759_CHGR_REG_CHG_DETAILS_03 0xb8
> +#define MAX77759_CHGR_REG_CHG_CNFG_00 0xb9
> +#define MAX77759_CHGR_REG_CHG_CNFG_00_MODE GENMASK(3, 0)
> +#define MAX77759_CHGR_REG_CHG_CNFG_01 0xba
> +#define MAX77759_CHGR_REG_CHG_CNFG_02 0xbb
> +#define MAX77759_CHGR_REG_CHG_CNFG_02_CHGCC GENMASK(5, 0)
Small nit - there seems to be a stray TAB in this line.
Other than that:
Reviewed-by: André Draszik <andre.draszik@linaro.org>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v3 3/5] mfd: max77759: add register bitmasks and modify irq configs for charger
2026-01-05 16:45 ` André Draszik
@ 2026-01-05 19:58 ` Amit Sunil Dhamne
0 siblings, 0 replies; 25+ messages in thread
From: Amit Sunil Dhamne @ 2026-01-05 19:58 UTC (permalink / raw)
To: André Draszik, Sebastian Reichel, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Lee Jones, Greg Kroah-Hartman,
Badhri Jagan Sridharan, Heikki Krogerus, Peter Griffin,
Tudor Ambarus, Alim Akhtar
Cc: linux-kernel, linux-pm, devicetree, linux-usb, linux-arm-kernel,
linux-samsung-soc, RD Babiera, Kyle Tso
On 1/5/26 8:45 AM, André Draszik wrote:
> On Sat, 2025-12-27 at 00:04 +0000, Amit Sunil Dhamne via B4 Relay wrote:
>> From: Amit Sunil Dhamne <amitsd@google.com>
>>
>> Add register bitmasks for charger function.
>> In addition split the charger IRQs further such that each bit represents
>> an IRQ downstream of charger regmap irq chip. In addition populate the
>> ack_base to offload irq ack to the regmap irq chip framework.
>>
>> Signed-off-by: Amit Sunil Dhamne <amitsd@google.com>
>> ---
>> drivers/mfd/max77759.c | 91 +++++++++++++++++--
>> include/linux/mfd/max77759.h | 202 ++++++++++++++++++++++++++++++++++++-------
>> 2 files changed, 256 insertions(+), 37 deletions(-)
>>
>> [...]
>>
>> diff --git a/include/linux/mfd/max77759.h b/include/linux/mfd/max77759.h
>> index c6face34e385..e674a519e782 100644
>> --- a/include/linux/mfd/max77759.h
>> +++ b/include/linux/mfd/max77759.h
>> @@ -59,35 +59,65 @@
>> #define MAX77759_MAXQ_REG_AP_DATAIN0 0xb1
>> #define MAX77759_MAXQ_REG_UIC_SWRST 0xe0
>>
>> -#define MAX77759_CHGR_REG_CHG_INT 0xb0
>> -#define MAX77759_CHGR_REG_CHG_INT2 0xb1
>> -#define MAX77759_CHGR_REG_CHG_INT_MASK 0xb2
>> -#define MAX77759_CHGR_REG_CHG_INT2_MASK 0xb3
>> -#define MAX77759_CHGR_REG_CHG_INT_OK 0xb4
>> -#define MAX77759_CHGR_REG_CHG_DETAILS_00 0xb5
>> -#define MAX77759_CHGR_REG_CHG_DETAILS_01 0xb6
>> -#define MAX77759_CHGR_REG_CHG_DETAILS_02 0xb7
>> -#define MAX77759_CHGR_REG_CHG_DETAILS_03 0xb8
>> -#define MAX77759_CHGR_REG_CHG_CNFG_00 0xb9
>> -#define MAX77759_CHGR_REG_CHG_CNFG_01 0xba
>> -#define MAX77759_CHGR_REG_CHG_CNFG_02 0xbb
>> -#define MAX77759_CHGR_REG_CHG_CNFG_03 0xbc
>> -#define MAX77759_CHGR_REG_CHG_CNFG_04 0xbd
>> -#define MAX77759_CHGR_REG_CHG_CNFG_05 0xbe
>> -#define MAX77759_CHGR_REG_CHG_CNFG_06 0xbf
>> -#define MAX77759_CHGR_REG_CHG_CNFG_07 0xc0
>> -#define MAX77759_CHGR_REG_CHG_CNFG_08 0xc1
>> -#define MAX77759_CHGR_REG_CHG_CNFG_09 0xc2
>> -#define MAX77759_CHGR_REG_CHG_CNFG_10 0xc3
>> -#define MAX77759_CHGR_REG_CHG_CNFG_11 0xc4
>> -#define MAX77759_CHGR_REG_CHG_CNFG_12 0xc5
>> -#define MAX77759_CHGR_REG_CHG_CNFG_13 0xc6
>> -#define MAX77759_CHGR_REG_CHG_CNFG_14 0xc7
>> -#define MAX77759_CHGR_REG_CHG_CNFG_15 0xc8
>> -#define MAX77759_CHGR_REG_CHG_CNFG_16 0xc9
>> -#define MAX77759_CHGR_REG_CHG_CNFG_17 0xca
>> -#define MAX77759_CHGR_REG_CHG_CNFG_18 0xcb
>> -#define MAX77759_CHGR_REG_CHG_CNFG_19 0xcc
>> +#define MAX77759_CHGR_REG_CHG_INT 0xb0
>> +#define MAX77759_CHGR_REG_CHG_INT_AICL BIT(7)
>> +#define MAX77759_CHGR_REG_CHG_INT_CHGIN BIT(6)
>> +#define MAX77759_CHGR_REG_CHG_INT_WCIN BIT(5)
>> +#define MAX77759_CHGR_REG_CHG_INT_CHG BIT(4)
>> +#define MAX77759_CHGR_REG_CHG_INT_BAT BIT(3)
>> +#define MAX77759_CHGR_REG_CHG_INT_INLIM BIT(2)
>> +#define MAX77759_CHGR_REG_CHG_INT_THM2 BIT(1)
>> +#define MAX77759_CHGR_REG_CHG_INT_BYP BIT(0)
>> +#define MAX77759_CHGR_REG_CHG_INT2 0xb1
>> +#define MAX77759_CHGR_REG_CHG_INT2_INSEL BIT(7)
>> +#define MAX77759_CHGR_REG_CHG_INT2_SYS_UVLO1 BIT(6)
>> +#define MAX77759_CHGR_REG_CHG_INT2_SYS_UVLO2 BIT(5)
>> +#define MAX77759_CHGR_REG_CHG_INT2_BAT_OILO BIT(4)
>> +#define MAX77759_CHGR_REG_CHG_INT2_CHG_STA_CC BIT(3)
>> +#define MAX77759_CHGR_REG_CHG_INT2_CHG_STA_CV BIT(2)
>> +#define MAX77759_CHGR_REG_CHG_INT2_CHG_STA_TO BIT(1)
>> +#define MAX77759_CHGR_REG_CHG_INT2_CHG_STA_DONE BIT(0)
>> +#define MAX77759_CHGR_REG_CHG_INT_MASK 0xb2
>> +#define MAX77759_CHGR_REG_CHG_INT2_MASK 0xb3
>> +#define MAX77759_CHGR_REG_CHG_INT_OK 0xb4
>> +#define MAX77759_CHGR_REG_CHG_DETAILS_00 0xb5
>> +#define MAX77759_CHGR_REG_CHG_DETAILS_OO_CHGIN_DTLS GENMASK(6, 5)
>> +#define MAX77759_CHGR_REG_CHG_DETAILS_01 0xb6
>> +#define MAX77759_CHGR_REG_CHG_DETAILS_01_BAT_DTLS GENMASK(6, 4)
>> +#define MAX77759_CHGR_REG_CHG_DETAILS_01_CHG_DTLS GENMASK(3, 0)
>> +#define MAX77759_CHGR_REG_CHG_DETAILS_02 0xb7
>> +#define MAX77759_CHGR_REG_CHG_DETAILS_02_CHGIN_STS BIT(5)
>> +#define MAX77759_CHGR_REG_CHG_DETAILS_03 0xb8
>> +#define MAX77759_CHGR_REG_CHG_CNFG_00 0xb9
>> +#define MAX77759_CHGR_REG_CHG_CNFG_00_MODE GENMASK(3, 0)
>> +#define MAX77759_CHGR_REG_CHG_CNFG_01 0xba
>> +#define MAX77759_CHGR_REG_CHG_CNFG_02 0xbb
>> +#define MAX77759_CHGR_REG_CHG_CNFG_02_CHGCC GENMASK(5, 0)
> Small nit - there seems to be a stray TAB in this line.
Will fix it in the next revision.
BR,
Amit
>
> Other than that:
> Reviewed-by: André Draszik <andre.draszik@linaro.org>
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH v3 4/5] power: supply: max77759: add charger driver
2025-12-27 0:04 [PATCH v3 0/5] Introduce MAX77759 charger driver Amit Sunil Dhamne via B4 Relay
` (2 preceding siblings ...)
2025-12-27 0:04 ` [PATCH v3 3/5] mfd: max77759: add register bitmasks and modify irq configs for charger Amit Sunil Dhamne via B4 Relay
@ 2025-12-27 0:04 ` Amit Sunil Dhamne via B4 Relay
2026-01-05 17:32 ` André Draszik
2025-12-27 0:04 ` [PATCH v3 5/5] usb: typec: tcpm/tcpci_maxim: deprecate WAR for setting charger mode Amit Sunil Dhamne via B4 Relay
4 siblings, 1 reply; 25+ messages in thread
From: Amit Sunil Dhamne via B4 Relay @ 2025-12-27 0:04 UTC (permalink / raw)
To: Sebastian Reichel, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
André Draszik, Lee Jones, Greg Kroah-Hartman,
Badhri Jagan Sridharan, Heikki Krogerus, Peter Griffin,
Tudor Ambarus, Alim Akhtar
Cc: linux-kernel, linux-pm, devicetree, linux-usb, linux-arm-kernel,
linux-samsung-soc, RD Babiera, Kyle Tso, Amit Sunil Dhamne
From: Amit Sunil Dhamne <amitsd@google.com>
Add support for MAX77759 battery charger driver. This is a 4A 1-Cell
Li+/LiPoly dual input switch mode charger. While the device can support
USB & wireless charger inputs, this implementation only supports USB
input. This implementation supports both buck and boost modes.
Signed-off-by: Amit Sunil Dhamne <amitsd@google.com>
---
MAINTAINERS | 6 +
drivers/power/supply/Kconfig | 11 +
drivers/power/supply/Makefile | 1 +
drivers/power/supply/max77759_charger.c | 764 ++++++++++++++++++++++++++++++++
4 files changed, 782 insertions(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index dc731d37c8fe..26a9654ab75e 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -15539,6 +15539,12 @@ F: drivers/mfd/max77759.c
F: drivers/nvmem/max77759-nvmem.c
F: include/linux/mfd/max77759.h
+MAXIM MAX77759 BATTERY CHARGER DRIVER
+M: Amit Sunil Dhamne <amitsd@google.com>
+L: linux-kernel@vger.kernel.org
+S: Maintained
+F: drivers/power/supply/max77759_charger.c
+
MAXIM MAX77802 PMIC REGULATOR DEVICE DRIVER
M: Javier Martinez Canillas <javier@dowhile0.org>
L: linux-kernel@vger.kernel.org
diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig
index 92f9f7aae92f..e172fd980fde 100644
--- a/drivers/power/supply/Kconfig
+++ b/drivers/power/supply/Kconfig
@@ -1132,4 +1132,15 @@ config FUEL_GAUGE_MM8013
the state of charge, temperature, cycle count, actual and design
capacity, etc.
+config CHARGER_MAX77759
+ tristate "MAX77759 Charger Driver"
+ depends on MFD_MAX77759 && REGULATOR
+ default MFD_MAX77759
+ help
+ Say M or Y here to enable the MAX77759 Charger Driver. MAX77759
+ charger is a function of the MAX77759 PMIC. This is a dual input
+ switch-mode charger. This driver supports buck and OTG boost modes.
+
+ If built as a module, it will be called max77759_charger.
+
endif # POWER_SUPPLY
diff --git a/drivers/power/supply/Makefile b/drivers/power/supply/Makefile
index 4b79d5abc49a..6af905875ad5 100644
--- a/drivers/power/supply/Makefile
+++ b/drivers/power/supply/Makefile
@@ -128,3 +128,4 @@ obj-$(CONFIG_CHARGER_SURFACE) += surface_charger.o
obj-$(CONFIG_BATTERY_UG3105) += ug3105_battery.o
obj-$(CONFIG_CHARGER_QCOM_SMB2) += qcom_smbx.o
obj-$(CONFIG_FUEL_GAUGE_MM8013) += mm8013.o
+obj-$(CONFIG_CHARGER_MAX77759) += max77759_charger.o
diff --git a/drivers/power/supply/max77759_charger.c b/drivers/power/supply/max77759_charger.c
new file mode 100644
index 000000000000..3d255b069fb9
--- /dev/null
+++ b/drivers/power/supply/max77759_charger.c
@@ -0,0 +1,764 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * max77759_charger.c - Battery charger driver for MAX77759 charger device.
+ *
+ * Copyright 2025 Google LLC.
+ */
+
+#include <linux/bitfield.h>
+#include <linux/cleanup.h>
+#include <linux/device.h>
+#include <linux/devm-helpers.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
+#include <linux/math64.h>
+#include <linux/mfd/max77759.h>
+#include <linux/module.h>
+#include <linux/mod_devicetable.h>
+#include <linux/mutex.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/power_supply.h>
+#include <linux/regmap.h>
+#include <linux/regulator/driver.h>
+#include <linux/string_choices.h>
+
+/* Default values for Fast Charge Current & Float Voltage */
+#define CHG_CC_DEFAULT_UA 2266770
+#define CHG_FV_DEFAULT_MV 4300
+
+#define FOREACH_IRQ(S) \
+ S(AICL), \
+ S(CHGIN), \
+ S(CHG), \
+ S(INLIM), \
+ S(BAT_OILO), \
+ S(CHG_STA_CC), \
+ S(CHG_STA_CV), \
+ S(CHG_STA_TO), \
+ S(CHG_STA_DONE)
+
+#define GENERATE_ENUM(e) e
+#define GENERATE_STRING(s) #s
+
+enum {
+ FOREACH_IRQ(GENERATE_ENUM)
+};
+
+static const char *const chgr_irqs_str[] = {
+ FOREACH_IRQ(GENERATE_STRING)
+};
+
+static int irqs[ARRAY_SIZE(chgr_irqs_str)];
+
+struct max77759_charger {
+ struct device *dev;
+ struct regmap *regmap;
+ struct power_supply *psy;
+ struct regulator_dev *chgin_otg_rdev;
+ struct notifier_block nb;
+ struct power_supply *tcpm_psy;
+ struct work_struct psy_work;
+ struct mutex lock; /* protects the state below */
+ enum max77759_chgr_mode mode;
+};
+
+static inline int regval_to_val(int reg, int reg_offset, int step, int minval)
+{
+ return ((reg - reg_offset) * step) + minval;
+}
+
+static inline int val_to_regval(int val, int minval, int step, int reg_offset)
+{
+ s64 dividend;
+
+ if (unlikely(step == 0))
+ return reg_offset;
+
+ dividend = (s64)val - minval;
+ return DIV_S64_ROUND_CLOSEST(dividend, step) + reg_offset;
+}
+
+static inline int unlock_prot_regs(struct max77759_charger *chg, bool unlock)
+{
+ return regmap_update_bits(chg->regmap, MAX77759_CHGR_REG_CHG_CNFG_06,
+ MAX77759_CHGR_REG_CHG_CNFG_06_CHGPROT, unlock
+ ? MAX77759_CHGR_REG_CHG_CNFG_06_CHGPROT : 0);
+}
+
+static int charger_input_valid(struct max77759_charger *chg)
+{
+ u32 val;
+ int ret;
+
+ ret = regmap_read(chg->regmap, MAX77759_CHGR_REG_CHG_INT_OK, &val);
+ if (ret)
+ return ret;
+
+ return (val & MAX77759_CHGR_REG_CHG_INT_CHG) &&
+ (val & MAX77759_CHGR_REG_CHG_INT_CHGIN);
+}
+
+static int get_online(struct max77759_charger *chg)
+{
+ u32 val;
+ int ret;
+
+ ret = charger_input_valid(chg);
+ if (ret <= 0)
+ return ret;
+
+ ret = regmap_read(chg->regmap, MAX77759_CHGR_REG_CHG_DETAILS_02, &val);
+ if (ret)
+ return ret;
+
+ guard(mutex)(&chg->lock);
+ return (val & MAX77759_CHGR_REG_CHG_DETAILS_02_CHGIN_STS) &&
+ (chg->mode == MAX77759_CHGR_MODE_CHG_BUCK_ON);
+}
+
+static int get_status(struct max77759_charger *chg)
+{
+ u32 val;
+ int ret;
+
+ ret = regmap_read(chg->regmap, MAX77759_CHGR_REG_CHG_DETAILS_01, &val);
+ if (ret)
+ return ret;
+
+ switch (FIELD_GET(MAX77759_CHGR_REG_CHG_DETAILS_01_CHG_DTLS, val)) {
+ case MAX77759_CHGR_CHG_DTLS_PREQUAL:
+ case MAX77759_CHGR_CHG_DTLS_CC:
+ case MAX77759_CHGR_CHG_DTLS_CV:
+ case MAX77759_CHGR_CHG_DTLS_TO:
+ return POWER_SUPPLY_STATUS_CHARGING;
+ case MAX77759_CHGR_CHG_DTLS_DONE:
+ return POWER_SUPPLY_STATUS_FULL;
+ case MAX77759_CHGR_CHG_DTLS_TIMER_FAULT:
+ case MAX77759_CHGR_CHG_DTLS_SUSP_BATT_THM:
+ case MAX77759_CHGR_CHG_DTLS_OFF_WDOG_TIMER:
+ case MAX77759_CHGR_CHG_DTLS_SUSP_JEITA:
+ return POWER_SUPPLY_STATUS_NOT_CHARGING;
+ case MAX77759_CHGR_CHG_DTLS_OFF:
+ return POWER_SUPPLY_STATUS_DISCHARGING;
+ default:
+ break;
+ }
+
+ return POWER_SUPPLY_STATUS_UNKNOWN;
+}
+
+static int get_charge_type(struct max77759_charger *chg)
+{
+ u32 val;
+ int ret;
+
+ ret = regmap_read(chg->regmap, MAX77759_CHGR_REG_CHG_DETAILS_01, &val);
+ if (ret)
+ return ret;
+
+ switch (FIELD_GET(MAX77759_CHGR_REG_CHG_DETAILS_01_CHG_DTLS, val)) {
+ case MAX77759_CHGR_CHG_DTLS_PREQUAL:
+ return POWER_SUPPLY_CHARGE_TYPE_TRICKLE;
+ case MAX77759_CHGR_CHG_DTLS_CC:
+ case MAX77759_CHGR_CHG_DTLS_CV:
+ return POWER_SUPPLY_CHARGE_TYPE_FAST;
+ case MAX77759_CHGR_CHG_DTLS_TO:
+ return POWER_SUPPLY_CHARGE_TYPE_STANDARD;
+ case MAX77759_CHGR_CHG_DTLS_DONE:
+ case MAX77759_CHGR_CHG_DTLS_TIMER_FAULT:
+ case MAX77759_CHGR_CHG_DTLS_SUSP_BATT_THM:
+ case MAX77759_CHGR_CHG_DTLS_OFF_WDOG_TIMER:
+ case MAX77759_CHGR_CHG_DTLS_SUSP_JEITA:
+ case MAX77759_CHGR_CHG_DTLS_OFF:
+ return POWER_SUPPLY_CHARGE_TYPE_NONE;
+ default:
+ break;
+ }
+
+ return POWER_SUPPLY_CHARGE_TYPE_UNKNOWN;
+}
+
+static int get_chg_health(struct max77759_charger *chg)
+{
+ u32 val;
+ int ret;
+
+ ret = regmap_read(chg->regmap, MAX77759_CHGR_REG_CHG_DETAILS_00, &val);
+ if (ret)
+ return ret;
+
+ switch (FIELD_GET(MAX77759_CHGR_REG_CHG_DETAILS_OO_CHGIN_DTLS, val)) {
+ case MAX77759_CHGR_CHGIN_DTLS_VBUS_UNDERVOLTAGE:
+ case MAX77759_CHGR_CHGIN_DTLS_VBUS_MARGINAL_VOLTAGE:
+ return POWER_SUPPLY_HEALTH_UNDERVOLTAGE;
+ case MAX77759_CHGR_CHGIN_DTLS_VBUS_OVERVOLTAGE:
+ return POWER_SUPPLY_HEALTH_OVERVOLTAGE;
+ case MAX77759_CHGR_CHGIN_DTLS_VBUS_VALID:
+ return POWER_SUPPLY_HEALTH_GOOD;
+ default:
+ break;
+ }
+
+ return POWER_SUPPLY_HEALTH_UNKNOWN;
+}
+
+static int get_batt_health(struct max77759_charger *chg)
+{
+ u32 val;
+ int ret;
+
+ ret = regmap_read(chg->regmap, MAX77759_CHGR_REG_CHG_DETAILS_01, &val);
+ if (ret)
+ return ret;
+
+ switch (FIELD_GET(MAX77759_CHGR_REG_CHG_DETAILS_01_BAT_DTLS, val)) {
+ case MAX77759_CHGR_BAT_DTLS_NO_BATT_CHG_SUSP:
+ return POWER_SUPPLY_HEALTH_NO_BATTERY;
+ case MAX77759_CHGR_BAT_DTLS_DEAD_BATTERY:
+ return POWER_SUPPLY_HEALTH_DEAD;
+ case MAX77759_CHGR_BAT_DTLS_BAT_CHG_TIMER_FAULT:
+ return POWER_SUPPLY_HEALTH_SAFETY_TIMER_EXPIRE;
+ case MAX77759_CHGR_BAT_DTLS_BAT_OKAY:
+ case MAX77759_CHGR_BAT_DTLS_BAT_ONLY_MODE:
+ return POWER_SUPPLY_HEALTH_GOOD;
+ case MAX77759_CHGR_BAT_DTLS_BAT_UNDERVOLTAGE:
+ return POWER_SUPPLY_HEALTH_UNDERVOLTAGE;
+ case MAX77759_CHGR_BAT_DTLS_BAT_OVERVOLTAGE:
+ return POWER_SUPPLY_HEALTH_OVERVOLTAGE;
+ case MAX77759_CHGR_BAT_DTLS_BAT_OVERCURRENT:
+ return POWER_SUPPLY_HEALTH_OVERCURRENT;
+ default:
+ break;
+ }
+
+ return POWER_SUPPLY_HEALTH_UNKNOWN;
+}
+
+static int get_health(struct max77759_charger *chg)
+{
+ int ret;
+
+ ret = get_online(chg);
+ if (ret < 0)
+ return ret;
+
+ if (ret) {
+ ret = get_chg_health(chg);
+ if (ret < 0 || ret != POWER_SUPPLY_HEALTH_GOOD)
+ return ret;
+ }
+
+ return get_batt_health(chg);
+}
+
+static int get_fast_charge_current(struct max77759_charger *chg)
+{
+ u32 regval;
+ int ret;
+
+ ret = regmap_read(chg->regmap, MAX77759_CHGR_REG_CHG_CNFG_02, ®val);
+ if (ret)
+ return ret;
+
+ ret = FIELD_GET(MAX77759_CHGR_REG_CHG_CNFG_02_CHGCC, regval);
+ if (ret <= MAX77759_CHGR_CHGCC_REG_OFFSET)
+ return MAX77759_CHGR_CHGCC_MIN_UA;
+
+ return regval_to_val(ret, MAX77759_CHGR_CHGCC_REG_OFFSET,
+ MAX77759_CHGR_CHGCC_STEP_UA,
+ MAX77759_CHGR_CHGCC_MIN_UA);
+}
+
+static int set_fast_charge_current_limit(struct max77759_charger *chg,
+ u32 cc_max_ua)
+{
+ u32 val;
+
+ if (cc_max_ua < MAX77759_CHGR_CHGCC_MIN_UA ||
+ cc_max_ua > MAX77759_CHGR_CHGCC_MAX_UA)
+ return -EINVAL;
+
+ val = val_to_regval(cc_max_ua, MAX77759_CHGR_CHGCC_MIN_UA,
+ MAX77759_CHGR_CHGCC_STEP_UA,
+ MAX77759_CHGR_CHGCC_REG_OFFSET);
+ return regmap_update_bits(chg->regmap, MAX77759_CHGR_REG_CHG_CNFG_02,
+ MAX77759_CHGR_REG_CHG_CNFG_02_CHGCC, val);
+}
+
+static int get_float_voltage(struct max77759_charger *chg)
+{
+ u32 regval;
+ int ret;
+
+ ret = regmap_read(chg->regmap, MAX77759_CHGR_REG_CHG_CNFG_04, ®val);
+ if (ret)
+ return ret;
+
+ ret = FIELD_GET(MAX77759_CHGR_REG_CHG_CNFG_04_CHG_CV_PRM, regval);
+ switch (ret) {
+ case MAX77759_CHGR_CHG_CV_PRM_HI_MIN_REG ... MAX77759_CHGR_CHG_CV_PRM_HI_MAX_REG:
+ return regval_to_val(ret, MAX77759_CHGR_CHG_CV_PRM_HI_MIN_REG,
+ MAX77759_CHGR_CHG_CV_PRM_HI_STEP_MV,
+ MAX77759_CHGR_CHG_CV_PRM_HI_MIN_MV);
+ case MAX77759_CHGR_CHG_CV_PRM_LO_MIN_REG ... MAX77759_CHGR_CHG_CV_PRM_LO_MAX_REG:
+ return regval_to_val(ret, MAX77759_CHGR_CHG_CV_PRM_LO_MIN_REG,
+ MAX77759_CHGR_CHG_CV_PRM_LO_STEP_MV,
+ MAX77759_CHGR_CHG_CV_PRM_LO_MIN_MV);
+ default:
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+static int set_float_voltage_limit(struct max77759_charger *chg, u32 fv_mv)
+{
+ u32 regval;
+
+ if (fv_mv >= MAX77759_CHGR_CHG_CV_PRM_LO_MIN_MV &&
+ fv_mv <= MAX77759_CHGR_CHG_CV_PRM_LO_MAX_MV) {
+ regval = val_to_regval(fv_mv,
+ MAX77759_CHGR_CHG_CV_PRM_LO_MIN_MV,
+ MAX77759_CHGR_CHG_CV_PRM_LO_STEP_MV,
+ MAX77759_CHGR_CHG_CV_PRM_LO_MIN_REG);
+ } else if (fv_mv >= MAX77759_CHGR_CHG_CV_PRM_HI_MIN_MV &&
+ fv_mv <= MAX77759_CHGR_CHG_CV_PRM_HI_MAX_MV) {
+ regval = val_to_regval(fv_mv,
+ MAX77759_CHGR_CHG_CV_PRM_HI_MIN_MV,
+ MAX77759_CHGR_CHG_CV_PRM_HI_STEP_MV,
+ MAX77759_CHGR_CHG_CV_PRM_HI_MIN_REG);
+ } else {
+ return -EINVAL;
+ }
+
+ return regmap_update_bits(chg->regmap, MAX77759_CHGR_REG_CHG_CNFG_04,
+ MAX77759_CHGR_REG_CHG_CNFG_04_CHG_CV_PRM,
+ regval);
+}
+
+static int get_input_current_limit(struct max77759_charger *chg)
+{
+ u32 regval;
+ int ret;
+
+ ret = regmap_read(chg->regmap, MAX77759_CHGR_REG_CHG_CNFG_09, ®val);
+ if (ret)
+ return ret;
+
+ ret = FIELD_GET(MAX77759_CHGR_REG_CHG_CNFG_09_CHGIN_ILIM, regval);
+ if (ret <= MAX77759_CHGR_CHGIN_ILIM_REG_OFFSET)
+ return MAX77759_CHGR_CHGIN_ILIM_MIN_UA;
+
+ return regval_to_val(ret, MAX77759_CHGR_CHGIN_ILIM_REG_OFFSET,
+ MAX77759_CHGR_CHGIN_ILIM_STEP_UA,
+ MAX77759_CHGR_CHGIN_ILIM_MIN_UA);
+}
+
+static int set_input_current_limit(struct max77759_charger *chg, int ilim_ua)
+{
+ u32 regval;
+
+ if (ilim_ua < 0)
+ return -EINVAL;
+
+ if (ilim_ua == 0)
+ ilim_ua = MAX77759_CHGR_CHGIN_ILIM_MIN_UA;
+ else if (ilim_ua > MAX77759_CHGR_CHGIN_ILIM_MAX_UA)
+ ilim_ua = MAX77759_CHGR_CHGIN_ILIM_MAX_UA;
+
+ regval = val_to_regval(ilim_ua, MAX77759_CHGR_CHGIN_ILIM_MIN_UA,
+ MAX77759_CHGR_CHGIN_ILIM_STEP_UA,
+ MAX77759_CHGR_CHGIN_ILIM_REG_OFFSET);
+ return regmap_update_bits(chg->regmap, MAX77759_CHGR_REG_CHG_CNFG_09,
+ MAX77759_CHGR_REG_CHG_CNFG_09_CHGIN_ILIM,
+ regval);
+}
+
+static const enum power_supply_property max77759_charger_props[] = {
+ POWER_SUPPLY_PROP_ONLINE,
+ POWER_SUPPLY_PROP_PRESENT,
+ POWER_SUPPLY_PROP_STATUS,
+ POWER_SUPPLY_PROP_CHARGE_TYPE,
+ POWER_SUPPLY_PROP_HEALTH,
+ POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX,
+ POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE_MAX,
+ POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT,
+};
+
+static int max77759_charger_get_property(struct power_supply *psy,
+ enum power_supply_property psp,
+ union power_supply_propval *pval)
+{
+ struct max77759_charger *chg = power_supply_get_drvdata(psy);
+ int ret;
+
+ switch (psp) {
+ case POWER_SUPPLY_PROP_ONLINE:
+ ret = get_online(chg);
+ break;
+ case POWER_SUPPLY_PROP_PRESENT:
+ ret = charger_input_valid(chg);
+ break;
+ case POWER_SUPPLY_PROP_STATUS:
+ ret = get_status(chg);
+ break;
+ case POWER_SUPPLY_PROP_CHARGE_TYPE:
+ ret = get_charge_type(chg);
+ break;
+ case POWER_SUPPLY_PROP_HEALTH:
+ ret = get_health(chg);
+ break;
+ case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX:
+ ret = get_fast_charge_current(chg);
+ break;
+ case POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE_MAX:
+ ret = get_float_voltage(chg);
+ break;
+ case POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT:
+ ret = get_input_current_limit(chg);
+ break;
+ default:
+ ret = -EINVAL;
+ }
+
+ pval->intval = ret;
+ return ret < 0 ? ret : 0;
+}
+
+static const struct power_supply_desc max77759_charger_desc = {
+ .name = "max77759-charger",
+ .type = POWER_SUPPLY_TYPE_USB,
+ .properties = max77759_charger_props,
+ .num_properties = ARRAY_SIZE(max77759_charger_props),
+ .get_property = max77759_charger_get_property,
+};
+
+static int charger_set_mode(struct max77759_charger *chg,
+ enum max77759_chgr_mode mode)
+{
+ int ret;
+
+ guard(mutex)(&chg->lock);
+
+ if (chg->mode == mode)
+ return 0;
+
+ if ((mode == MAX77759_CHGR_MODE_CHG_BUCK_ON ||
+ mode == MAX77759_CHGR_MODE_OTG_BOOST_ON) &&
+ chg->mode != MAX77759_CHGR_MODE_OFF) {
+ dev_err(chg->dev, "Invalid mode transition from %d to %d",
+ chg->mode, mode);
+ return -EINVAL;
+ }
+
+ ret = regmap_update_bits(chg->regmap, MAX77759_CHGR_REG_CHG_CNFG_00,
+ MAX77759_CHGR_REG_CHG_CNFG_00_MODE, mode);
+ if (ret)
+ return ret;
+
+ chg->mode = mode;
+ return 0;
+}
+
+static int enable_chgin_otg(struct regulator_dev *rdev)
+{
+ struct max77759_charger *chg = rdev_get_drvdata(rdev);
+
+ return charger_set_mode(chg, MAX77759_CHGR_MODE_OTG_BOOST_ON);
+}
+
+static int disable_chgin_otg(struct regulator_dev *rdev)
+{
+ struct max77759_charger *chg = rdev_get_drvdata(rdev);
+
+ return charger_set_mode(chg, MAX77759_CHGR_MODE_OFF);
+}
+
+static int chgin_otg_status(struct regulator_dev *rdev)
+{
+ struct max77759_charger *chg = rdev_get_drvdata(rdev);
+
+ guard(mutex)(&chg->lock);
+ return chg->mode == MAX77759_CHGR_MODE_OTG_BOOST_ON;
+}
+
+static const struct regulator_ops chgin_otg_reg_ops = {
+ .enable = enable_chgin_otg,
+ .disable = disable_chgin_otg,
+ .is_enabled = chgin_otg_status,
+};
+
+static const struct regulator_desc chgin_otg_reg_desc = {
+ .name = "chgin-otg",
+ .of_match = of_match_ptr("chgin-otg-regulator"),
+ .owner = THIS_MODULE,
+ .ops = &chgin_otg_reg_ops,
+ .fixed_uV = 5000000,
+ .n_voltages = 1,
+};
+
+static irqreturn_t irq_handler(int irq, void *data)
+{
+ struct max77759_charger *chg = data;
+ struct device *dev = chg->dev;
+ u32 chgint_ok;
+ int i;
+
+ regmap_read(chg->regmap, MAX77759_CHGR_REG_CHG_INT_OK, &chgint_ok);
+
+ for (i = 0; i < ARRAY_SIZE(irqs); i++) {
+ if (irqs[i] == irq)
+ break;
+ }
+
+ switch (i) {
+ case AICL:
+ dev_dbg(dev, "AICL mode: %s",
+ str_no_yes(chgint_ok & MAX77759_CHGR_REG_CHG_INT_AICL));
+ break;
+ case CHGIN:
+ dev_dbg(dev, "CHGIN input valid: %s",
+ str_yes_no(chgint_ok & MAX77759_CHGR_REG_CHG_INT_CHGIN));
+ break;
+ case CHG:
+ dev_dbg(dev, "CHG status okay/off: %s",
+ str_yes_no(chgint_ok & MAX77759_CHGR_REG_CHG_INT_CHG));
+ break;
+ case INLIM:
+ dev_dbg(dev, "Current Limit reached: %s",
+ str_no_yes(chgint_ok & MAX77759_CHGR_REG_CHG_INT_INLIM));
+ break;
+ case BAT_OILO:
+ dev_dbg(dev, "Battery over-current threshold crossed");
+ break;
+ case CHG_STA_CC:
+ dev_dbg(dev, "Charger reached CC stage");
+ break;
+ case CHG_STA_CV:
+ dev_dbg(dev, "Charger reached CV stage");
+ break;
+ case CHG_STA_TO:
+ dev_dbg(dev, "Charger reached TO stage");
+ break;
+ case CHG_STA_DONE:
+ dev_dbg(dev, "Charger reached TO stage");
+ break;
+ default:
+ dev_err(dev, "Unrecognized irq: %d", i);
+ return IRQ_HANDLED;
+ }
+
+ power_supply_changed(chg->psy);
+ return IRQ_HANDLED;
+}
+
+static int max77759_init_irqhandler(struct max77759_charger *chg)
+{
+ struct device *dev = chg->dev;
+ unsigned long irq_flags;
+ struct irq_data *irqd;
+ int i, ret;
+
+ for (i = 0; i < ARRAY_SIZE(chgr_irqs_str); i++) {
+ ret = platform_get_irq_byname(to_platform_device(dev),
+ chgr_irqs_str[i]);
+ if (ret < 0) {
+ dev_err(dev,
+ "Failed to get irq resource for %s, ret=%d",
+ chgr_irqs_str[i], ret);
+ return ret;
+ }
+
+ irqs[i] = ret;
+ irq_flags = IRQF_ONESHOT;
+ irqd = irq_get_irq_data(irqs[i]);
+ if (irqd)
+ irq_flags |= irqd_get_trigger_type(irqd);
+
+ ret = devm_request_threaded_irq(dev, irqs[i], NULL, irq_handler,
+ irq_flags, dev_name(dev), chg);
+ if (ret) {
+ dev_err(dev,
+ "Unable to register irq handler for %s, ret=%d",
+ chgr_irqs_str[i], ret);
+ return ret;
+ }
+ }
+
+ return 0;
+}
+
+static int max77759_charger_init(struct max77759_charger *chg)
+{
+ struct power_supply_battery_info *info;
+ u32 regval, fast_chg_curr, fv;
+ int ret;
+
+ regmap_read(chg->regmap, MAX77759_CHGR_REG_CHG_CNFG_00, ®val);
+ chg->mode = FIELD_GET(MAX77759_CHGR_REG_CHG_CNFG_00_MODE, regval);
+ ret = charger_set_mode(chg, MAX77759_CHGR_MODE_OFF);
+ if (ret)
+ return ret;
+
+ if (power_supply_get_battery_info(chg->psy, &info)) {
+ fv = CHG_FV_DEFAULT_MV;
+ fast_chg_curr = CHG_CC_DEFAULT_UA;
+ } else {
+ fv = info->constant_charge_voltage_max_uv / 1000;
+ fast_chg_curr = info->constant_charge_current_max_ua;
+ }
+
+ ret = set_fast_charge_current_limit(chg, fast_chg_curr);
+ if (ret)
+ return ret;
+
+ ret = set_float_voltage_limit(chg, fv);
+ if (ret)
+ return ret;
+
+ ret = unlock_prot_regs(chg, true);
+ if (ret)
+ return ret;
+
+ /* Disable wireless charging input */
+ regmap_update_bits(chg->regmap, MAX77759_CHGR_REG_CHG_CNFG_12,
+ MAX77759_CHGR_REG_CHG_CNFG_12_WCINSEL, 0);
+
+ regmap_update_bits(chg->regmap, MAX77759_CHGR_REG_CHG_CNFG_18,
+ MAX77759_CHGR_REG_CHG_CNFG_18_WDTEN, 0);
+
+ return unlock_prot_regs(chg, false);
+}
+
+static void psy_work_item(struct work_struct *work)
+{
+ struct max77759_charger *chg =
+ container_of(work, struct max77759_charger, psy_work);
+ union power_supply_propval current_limit = { 0 }, online = { 0 };
+ int ret;
+
+ power_supply_get_property(chg->tcpm_psy, POWER_SUPPLY_PROP_CURRENT_MAX,
+ ¤t_limit);
+ power_supply_get_property(chg->tcpm_psy, POWER_SUPPLY_PROP_ONLINE,
+ &online);
+
+ if (online.intval && current_limit.intval) {
+ ret = set_input_current_limit(chg, current_limit.intval);
+ if (ret)
+ dev_err(chg->dev,
+ "Unable to set current limit, ret=%d", ret);
+
+ charger_set_mode(chg, MAX77759_CHGR_MODE_CHG_BUCK_ON);
+ } else {
+ charger_set_mode(chg, MAX77759_CHGR_MODE_OFF);
+ }
+}
+
+static int psy_changed(struct notifier_block *nb, unsigned long evt, void *data)
+{
+ struct max77759_charger *chg = container_of(nb, struct max77759_charger,
+ nb);
+ const char *psy_name = "tcpm-source";
+ struct power_supply *psy = data;
+
+ if (!strnstr(psy->desc->name, psy_name, strlen(psy_name)) ||
+ evt != PSY_EVENT_PROP_CHANGED)
+ return NOTIFY_OK;
+
+ chg->tcpm_psy = psy;
+ schedule_work(&chg->psy_work);
+ return NOTIFY_OK;
+}
+
+static void max_tcpci_unregister_psy_notifier(void *nb)
+{
+ power_supply_unreg_notifier(nb);
+}
+
+static int max77759_charger_probe(struct platform_device *pdev)
+{
+ struct regulator_config chgin_otg_reg_cfg;
+ struct power_supply_config psy_cfg;
+ struct device *dev = &pdev->dev;
+ struct max77759_charger *chg;
+ int ret;
+
+ device_set_of_node_from_dev(dev, dev->parent);
+ chg = devm_kzalloc(dev, sizeof(*chg), GFP_KERNEL);
+ if (!chg)
+ return -ENOMEM;
+
+ platform_set_drvdata(pdev, chg);
+ chg->dev = dev;
+ chg->regmap = dev_get_regmap(dev->parent, "charger");
+ if (!chg->regmap)
+ return dev_err_probe(dev, -ENODEV, "Missing regmap");
+
+ ret = devm_mutex_init(dev, &chg->lock);
+ if (ret)
+ return dev_err_probe(dev, ret, "Failed to initialize lock");
+
+ psy_cfg.fwnode = dev_fwnode(dev);
+ psy_cfg.drv_data = chg;
+ chg->psy = devm_power_supply_register(dev, &max77759_charger_desc,
+ &psy_cfg);
+ if (IS_ERR(chg->psy))
+ return dev_err_probe(dev, -EPROBE_DEFER,
+ "Failed to register psy, ret=%ld",
+ PTR_ERR(chg->psy));
+
+ ret = max77759_charger_init(chg);
+ if (ret)
+ return dev_err_probe(dev, ret,
+ "Failed to initialize max77759 charger");
+
+ chgin_otg_reg_cfg.dev = dev;
+ chgin_otg_reg_cfg.driver_data = chg;
+ chgin_otg_reg_cfg.of_node = dev_of_node(dev);
+ chg->chgin_otg_rdev = devm_regulator_register(dev, &chgin_otg_reg_desc,
+ &chgin_otg_reg_cfg);
+ if (IS_ERR(chg->chgin_otg_rdev))
+ return dev_err_probe(dev, PTR_ERR(chg->chgin_otg_rdev),
+ "Failed to register chgin otg regulator");
+
+ ret = devm_work_autocancel(dev, &chg->psy_work, psy_work_item);
+ if (ret)
+ return dev_err_probe(dev, ret, "Failed to initialize psy work");
+
+ chg->nb.notifier_call = psy_changed;
+ ret = power_supply_reg_notifier(&chg->nb);
+ if (ret)
+ return dev_err_probe(dev, ret,
+ "Unable to register psy notifier");
+
+ ret = devm_add_action_or_reset(dev, max_tcpci_unregister_psy_notifier,
+ &chg->nb);
+ if (ret)
+ return ret;
+
+ ret = max77759_init_irqhandler(chg);
+ if (ret)
+ return dev_err_probe(dev, ret,
+ "Unable to initialize irq handler");
+ return 0;
+}
+
+static const struct platform_device_id max77759_charger_id[] = {
+ {"max77759-charger",},
+ { }
+};
+MODULE_DEVICE_TABLE(platform, max77759_charger_id);
+
+static struct platform_driver max77759_charger_driver = {
+ .driver = {
+ .name = "max77759-charger",
+ },
+ .probe = max77759_charger_probe,
+ .id_table = max77759_charger_id,
+};
+module_platform_driver(max77759_charger_driver);
+
+MODULE_AUTHOR("Amit Sunil Dhamne <amitsd@google.com>");
+MODULE_DESCRIPTION("Maxim MAX77759 charger driver");
+MODULE_LICENSE("GPL");
--
2.52.0.351.gbe84eed79e-goog
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [PATCH v3 4/5] power: supply: max77759: add charger driver
2025-12-27 0:04 ` [PATCH v3 4/5] power: supply: max77759: add charger driver Amit Sunil Dhamne via B4 Relay
@ 2026-01-05 17:32 ` André Draszik
2026-01-06 23:41 ` Amit Sunil Dhamne
0 siblings, 1 reply; 25+ messages in thread
From: André Draszik @ 2026-01-05 17:32 UTC (permalink / raw)
To: amitsd, Sebastian Reichel, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Lee Jones, Greg Kroah-Hartman,
Badhri Jagan Sridharan, Heikki Krogerus, Peter Griffin,
Tudor Ambarus, Alim Akhtar
Cc: linux-kernel, linux-pm, devicetree, linux-usb, linux-arm-kernel,
linux-samsung-soc, RD Babiera, Kyle Tso
Hi Amit,
I haven't done a full review, but a few things caught my eye.
On Sat, 2025-12-27 at 00:04 +0000, Amit Sunil Dhamne via B4 Relay wrote:
> From: Amit Sunil Dhamne <amitsd@google.com>
>
> Add support for MAX77759 battery charger driver. This is a 4A 1-Cell
> Li+/LiPoly dual input switch mode charger. While the device can support
> USB & wireless charger inputs, this implementation only supports USB
> input. This implementation supports both buck and boost modes.
>
> Signed-off-by: Amit Sunil Dhamne <amitsd@google.com>
> ---
> MAINTAINERS | 6 +
> drivers/power/supply/Kconfig | 11 +
> drivers/power/supply/Makefile | 1 +
> drivers/power/supply/max77759_charger.c | 764 ++++++++++++++++++++++++++++++++
> 4 files changed, 782 insertions(+)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index dc731d37c8fe..26a9654ab75e 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -15539,6 +15539,12 @@ F: drivers/mfd/max77759.c
> F: drivers/nvmem/max77759-nvmem.c
> F: include/linux/mfd/max77759.h
>
> +MAXIM MAX77759 BATTERY CHARGER DRIVER
> +M: Amit Sunil Dhamne <amitsd@google.com>
> +L: linux-kernel@vger.kernel.org
> +S: Maintained
> +F: drivers/power/supply/max77759_charger.c
> +
> MAXIM MAX77802 PMIC REGULATOR DEVICE DRIVER
> M: Javier Martinez Canillas <javier@dowhile0.org>
> L: linux-kernel@vger.kernel.org
> diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig
> index 92f9f7aae92f..e172fd980fde 100644
> --- a/drivers/power/supply/Kconfig
> +++ b/drivers/power/supply/Kconfig
> @@ -1132,4 +1132,15 @@ config FUEL_GAUGE_MM8013
> the state of charge, temperature, cycle count, actual and design
> capacity, etc.
>
> +config CHARGER_MAX77759
> + tristate "MAX77759 Charger Driver"
> + depends on MFD_MAX77759 && REGULATOR
> + default MFD_MAX77759
> + help
> + Say M or Y here to enable the MAX77759 Charger Driver. MAX77759
> + charger is a function of the MAX77759 PMIC. This is a dual input
> + switch-mode charger. This driver supports buck and OTG boost modes.
> +
> + If built as a module, it will be called max77759_charger.
> +
It might make sense to add this block near the existing MAX77... charger drivers,
while updating the tristate string and keeping alphabetical order of entries.
> endif # POWER_SUPPLY
> diff --git a/drivers/power/supply/Makefile b/drivers/power/supply/Makefile
> index 4b79d5abc49a..6af905875ad5 100644
> --- a/drivers/power/supply/Makefile
> +++ b/drivers/power/supply/Makefile
> @@ -128,3 +128,4 @@ obj-$(CONFIG_CHARGER_SURFACE) += surface_charger.o
> obj-$(CONFIG_BATTERY_UG3105) += ug3105_battery.o
> obj-$(CONFIG_CHARGER_QCOM_SMB2) += qcom_smbx.o
> obj-$(CONFIG_FUEL_GAUGE_MM8013) += mm8013.o
> +obj-$(CONFIG_CHARGER_MAX77759) += max77759_charger.o
> diff --git a/drivers/power/supply/max77759_charger.c b/drivers/power/supply/max77759_charger.c
> new file mode 100644
> index 000000000000..3d255b069fb9
> --- /dev/null
> +++ b/drivers/power/supply/max77759_charger.c
> @@ -0,0 +1,764 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * max77759_charger.c - Battery charger driver for MAX77759 charger device.
> + *
> + * Copyright 2025 Google LLC.
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/cleanup.h>
> +#include <linux/device.h>
> +#include <linux/devm-helpers.h>
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
> +#include <linux/math64.h>
> +#include <linux/mfd/max77759.h>
> +#include <linux/module.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/mutex.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/power_supply.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/driver.h>
> +#include <linux/string_choices.h>
> +
> +/* Default values for Fast Charge Current & Float Voltage */
> +#define CHG_CC_DEFAULT_UA 2266770
> +#define CHG_FV_DEFAULT_MV 4300
> +
> +#define FOREACH_IRQ(S) \
> + S(AICL), \
> + S(CHGIN), \
> + S(CHG), \
> + S(INLIM), \
> + S(BAT_OILO), \
> + S(CHG_STA_CC), \
> + S(CHG_STA_CV), \
> + S(CHG_STA_TO), \
> + S(CHG_STA_DONE)
> +
> +#define GENERATE_ENUM(e) e
> +#define GENERATE_STRING(s) #s
> +
> +enum {
> + FOREACH_IRQ(GENERATE_ENUM)
> +};
> +
> +static const char *const chgr_irqs_str[] = {
> + FOREACH_IRQ(GENERATE_STRING)
> +};
> +
> +static int irqs[ARRAY_SIZE(chgr_irqs_str)];
No global variables please, this is not a singleton.
> [...]
>
> +static int set_input_current_limit(struct max77759_charger *chg, int ilim_ua)
> +{
> + u32 regval;
> +
> + if (ilim_ua < 0)
> + return -EINVAL;
> +
> + if (ilim_ua == 0)
> + ilim_ua = MAX77759_CHGR_CHGIN_ILIM_MIN_UA;
> + else if (ilim_ua > MAX77759_CHGR_CHGIN_ILIM_MAX_UA)
> + ilim_ua = MAX77759_CHGR_CHGIN_ILIM_MAX_UA;
What if ilim_ua == 1 (or any other value < min_uA)? You could use clamp()
instead of open-coding.
> +
> + regval = val_to_regval(ilim_ua, MAX77759_CHGR_CHGIN_ILIM_MIN_UA,
> + MAX77759_CHGR_CHGIN_ILIM_STEP_UA,
> + MAX77759_CHGR_CHGIN_ILIM_REG_OFFSET);
> + return regmap_update_bits(chg->regmap, MAX77759_CHGR_REG_CHG_CNFG_09,
> + MAX77759_CHGR_REG_CHG_CNFG_09_CHGIN_ILIM,
> + regval);
> +}
> +
> +static const enum power_supply_property max77759_charger_props[] = {
> + POWER_SUPPLY_PROP_ONLINE,
> + POWER_SUPPLY_PROP_PRESENT,
> + POWER_SUPPLY_PROP_STATUS,
> + POWER_SUPPLY_PROP_CHARGE_TYPE,
> + POWER_SUPPLY_PROP_HEALTH,
> + POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX,
> + POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE_MAX,
> + POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT,
> +};
> +
> +static int max77759_charger_get_property(struct power_supply *psy,
> + enum power_supply_property psp,
> + union power_supply_propval *pval)
> +{
> + struct max77759_charger *chg = power_supply_get_drvdata(psy);
> + int ret;
> +
> + switch (psp) {
> + case POWER_SUPPLY_PROP_ONLINE:
> + ret = get_online(chg);
> + break;
> + case POWER_SUPPLY_PROP_PRESENT:
> + ret = charger_input_valid(chg);
> + break;
> + case POWER_SUPPLY_PROP_STATUS:
> + ret = get_status(chg);
> + break;
> + case POWER_SUPPLY_PROP_CHARGE_TYPE:
> + ret = get_charge_type(chg);
> + break;
> + case POWER_SUPPLY_PROP_HEALTH:
> + ret = get_health(chg);
> + break;
> + case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX:
> + ret = get_fast_charge_current(chg);
> + break;
> + case POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE_MAX:
> + ret = get_float_voltage(chg);
> + break;
> + case POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT:
> + ret = get_input_current_limit(chg);
> + break;
> + default:
> + ret = -EINVAL;
> + }
> +
> + pval->intval = ret;
> + return ret < 0 ? ret : 0;
> +}
> +
> +static const struct power_supply_desc max77759_charger_desc = {
> + .name = "max77759-charger",
> + .type = POWER_SUPPLY_TYPE_USB,
> + .properties = max77759_charger_props,
> + .num_properties = ARRAY_SIZE(max77759_charger_props),
> + .get_property = max77759_charger_get_property,
> +};
> +
> +static int charger_set_mode(struct max77759_charger *chg,
> + enum max77759_chgr_mode mode)
> +{
> + int ret;
> +
> + guard(mutex)(&chg->lock);
> +
> + if (chg->mode == mode)
> + return 0;
> +
> + if ((mode == MAX77759_CHGR_MODE_CHG_BUCK_ON ||
> + mode == MAX77759_CHGR_MODE_OTG_BOOST_ON) &&
> + chg->mode != MAX77759_CHGR_MODE_OFF) {
> + dev_err(chg->dev, "Invalid mode transition from %d to %d",
> + chg->mode, mode);
> + return -EINVAL;
> + }
> +
> + ret = regmap_update_bits(chg->regmap, MAX77759_CHGR_REG_CHG_CNFG_00,
> + MAX77759_CHGR_REG_CHG_CNFG_00_MODE, mode);
> + if (ret)
> + return ret;
> +
> + chg->mode = mode;
> + return 0;
> +}
> +
> +static int enable_chgin_otg(struct regulator_dev *rdev)
> +{
> + struct max77759_charger *chg = rdev_get_drvdata(rdev);
> +
> + return charger_set_mode(chg, MAX77759_CHGR_MODE_OTG_BOOST_ON);
> +}
> +
> +static int disable_chgin_otg(struct regulator_dev *rdev)
> +{
> + struct max77759_charger *chg = rdev_get_drvdata(rdev);
> +
> + return charger_set_mode(chg, MAX77759_CHGR_MODE_OFF);
> +}
> +
> +static int chgin_otg_status(struct regulator_dev *rdev)
> +{
> + struct max77759_charger *chg = rdev_get_drvdata(rdev);
> +
> + guard(mutex)(&chg->lock);
> + return chg->mode == MAX77759_CHGR_MODE_OTG_BOOST_ON;
> +}
> +
> +static const struct regulator_ops chgin_otg_reg_ops = {
> + .enable = enable_chgin_otg,
> + .disable = disable_chgin_otg,
> + .is_enabled = chgin_otg_status,
> +};
> +
> +static const struct regulator_desc chgin_otg_reg_desc = {
> + .name = "chgin-otg",
> + .of_match = of_match_ptr("chgin-otg-regulator"),
> + .owner = THIS_MODULE,
> + .ops = &chgin_otg_reg_ops,
> + .fixed_uV = 5000000,
> + .n_voltages = 1,
> +};
> +
> +static irqreturn_t irq_handler(int irq, void *data)
> +{
> + struct max77759_charger *chg = data;
> + struct device *dev = chg->dev;
> + u32 chgint_ok;
> + int i;
> +
> + regmap_read(chg->regmap, MAX77759_CHGR_REG_CHG_INT_OK, &chgint_ok);
You might want to check the return value and return IRQ_NONE if it didn't
work?
> +
> + for (i = 0; i < ARRAY_SIZE(irqs); i++) {
> + if (irqs[i] == irq)
> + break;
> + }
> +
> + switch (i) {
> + case AICL:
> + dev_dbg(dev, "AICL mode: %s",
> + str_no_yes(chgint_ok & MAX77759_CHGR_REG_CHG_INT_AICL));
> + break;
> + case CHGIN:
> + dev_dbg(dev, "CHGIN input valid: %s",
> + str_yes_no(chgint_ok & MAX77759_CHGR_REG_CHG_INT_CHGIN));
> + break;
> + case CHG:
> + dev_dbg(dev, "CHG status okay/off: %s",
> + str_yes_no(chgint_ok & MAX77759_CHGR_REG_CHG_INT_CHG));
> + break;
> + case INLIM:
> + dev_dbg(dev, "Current Limit reached: %s",
> + str_no_yes(chgint_ok & MAX77759_CHGR_REG_CHG_INT_INLIM));
> + break;
> + case BAT_OILO:
> + dev_dbg(dev, "Battery over-current threshold crossed");
> + break;
> + case CHG_STA_CC:
> + dev_dbg(dev, "Charger reached CC stage");
> + break;
> + case CHG_STA_CV:
> + dev_dbg(dev, "Charger reached CV stage");
> + break;
> + case CHG_STA_TO:
> + dev_dbg(dev, "Charger reached TO stage");
> + break;
> + case CHG_STA_DONE:
> + dev_dbg(dev, "Charger reached TO stage");
> + break;
Are the above debug messages really all needed?
> + default:
> + dev_err(dev, "Unrecognized irq: %d", i);
> + return IRQ_HANDLED;
I'm not sure it should return IRQ_HANDLED in this case.
> + }
> +
> + power_supply_changed(chg->psy);
> + return IRQ_HANDLED;
> +}
> +
> +static int max77759_init_irqhandler(struct max77759_charger *chg)
> +{
> + struct device *dev = chg->dev;
> + unsigned long irq_flags;
> + struct irq_data *irqd;
> + int i, ret;
> +
> + for (i = 0; i < ARRAY_SIZE(chgr_irqs_str); i++) {
> + ret = platform_get_irq_byname(to_platform_device(dev),
> + chgr_irqs_str[i]);
> + if (ret < 0) {
> + dev_err(dev,
> + "Failed to get irq resource for %s, ret=%d",
> + chgr_irqs_str[i], ret);
> + return ret;
> + }
You should use return dev_err_probe() here, and drop the additional dev_err_probe()
in max77759_charger_probe().
> +
> + irqs[i] = ret;
> + irq_flags = IRQF_ONESHOT;
> + irqd = irq_get_irq_data(irqs[i]);
> + if (irqd)
> + irq_flags |= irqd_get_trigger_type(irqd);
The above three lines are not needed, and then you can also drop irq_flags and
use its value in the below call directly.
> +
> + ret = devm_request_threaded_irq(dev, irqs[i], NULL, irq_handler,
> + irq_flags, dev_name(dev), chg);
> + if (ret) {
> + dev_err(dev,
> + "Unable to register irq handler for %s, ret=%d",
> + chgr_irqs_str[i], ret);
> + return ret;
dev_err_probe() please.
> + }
> + }
> +
> + return 0;
> +}
> +
> +static int max77759_charger_init(struct max77759_charger *chg)
> +{
> + struct power_supply_battery_info *info;
> + u32 regval, fast_chg_curr, fv;
> + int ret;
> +
> + regmap_read(chg->regmap, MAX77759_CHGR_REG_CHG_CNFG_00, ®val);
> + chg->mode = FIELD_GET(MAX77759_CHGR_REG_CHG_CNFG_00_MODE, regval);
> + ret = charger_set_mode(chg, MAX77759_CHGR_MODE_OFF);
> + if (ret)
> + return ret;
> +
> + if (power_supply_get_battery_info(chg->psy, &info)) {
> + fv = CHG_FV_DEFAULT_MV;
> + fast_chg_curr = CHG_CC_DEFAULT_UA;
> + } else {
> + fv = info->constant_charge_voltage_max_uv / 1000;
> + fast_chg_curr = info->constant_charge_current_max_ua;
> + }
> +
> + ret = set_fast_charge_current_limit(chg, fast_chg_curr);
> + if (ret)
> + return ret;
> +
> + ret = set_float_voltage_limit(chg, fv);
> + if (ret)
> + return ret;
> +
> + ret = unlock_prot_regs(chg, true);
> + if (ret)
> + return ret;
> +
> + /* Disable wireless charging input */
> + regmap_update_bits(chg->regmap, MAX77759_CHGR_REG_CHG_CNFG_12,
> + MAX77759_CHGR_REG_CHG_CNFG_12_WCINSEL, 0);
> +
> + regmap_update_bits(chg->regmap, MAX77759_CHGR_REG_CHG_CNFG_18,
> + MAX77759_CHGR_REG_CHG_CNFG_18_WDTEN, 0);
I think it's good practice to check return values.
> +
> + return unlock_prot_regs(chg, false);
> +}
> +
> +static void psy_work_item(struct work_struct *work)
> +{
> + struct max77759_charger *chg =
> + container_of(work, struct max77759_charger, psy_work);
> + union power_supply_propval current_limit = { 0 }, online = { 0 };
> + int ret;
> +
> + power_supply_get_property(chg->tcpm_psy, POWER_SUPPLY_PROP_CURRENT_MAX,
> + ¤t_limit);
> + power_supply_get_property(chg->tcpm_psy, POWER_SUPPLY_PROP_ONLINE,
> + &online);
Would it make sense to rework this and check the return values? Then you can also
drop the greedy init at function entry.
> +
> + if (online.intval && current_limit.intval) {
> + ret = set_input_current_limit(chg, current_limit.intval);
> + if (ret)
> + dev_err(chg->dev,
> + "Unable to set current limit, ret=%d", ret);
> +
> + charger_set_mode(chg, MAX77759_CHGR_MODE_CHG_BUCK_ON);
> + } else {
> + charger_set_mode(chg, MAX77759_CHGR_MODE_OFF);
> + }
> +}
> +
> +static int psy_changed(struct notifier_block *nb, unsigned long evt, void *data)
> +{
> + struct max77759_charger *chg = container_of(nb, struct max77759_charger,
> + nb);
> + const char *psy_name = "tcpm-source";
> + struct power_supply *psy = data;
> +
> + if (!strnstr(psy->desc->name, psy_name, strlen(psy_name)) ||
> + evt != PSY_EVENT_PROP_CHANGED)
> + return NOTIFY_OK;
> +
> + chg->tcpm_psy = psy;
> + schedule_work(&chg->psy_work);
Maybe add a newline here.
> + return NOTIFY_OK;
> +}
> +
> +static void max_tcpci_unregister_psy_notifier(void *nb)
> +{
> + power_supply_unreg_notifier(nb);
> +}
> +
> +static int max77759_charger_probe(struct platform_device *pdev)
> +{
> + struct regulator_config chgin_otg_reg_cfg;
> + struct power_supply_config psy_cfg;
> + struct device *dev = &pdev->dev;
> + struct max77759_charger *chg;
> + int ret;
> +
> + device_set_of_node_from_dev(dev, dev->parent);
> + chg = devm_kzalloc(dev, sizeof(*chg), GFP_KERNEL);
> + if (!chg)
> + return -ENOMEM;
> +
> + platform_set_drvdata(pdev, chg);
> + chg->dev = dev;
> + chg->regmap = dev_get_regmap(dev->parent, "charger");
> + if (!chg->regmap)
> + return dev_err_probe(dev, -ENODEV, "Missing regmap");
> +
> + ret = devm_mutex_init(dev, &chg->lock);
> + if (ret)
> + return dev_err_probe(dev, ret, "Failed to initialize lock");
> +
> + psy_cfg.fwnode = dev_fwnode(dev);
> + psy_cfg.drv_data = chg;
> + chg->psy = devm_power_supply_register(dev, &max77759_charger_desc,
> + &psy_cfg);
> + if (IS_ERR(chg->psy))
> + return dev_err_probe(dev, -EPROBE_DEFER,
> + "Failed to register psy, ret=%ld",
> + PTR_ERR(chg->psy));
> +
> + ret = max77759_charger_init(chg);
> + if (ret)
> + return dev_err_probe(dev, ret,
> + "Failed to initialize max77759 charger");
> +
> + chgin_otg_reg_cfg.dev = dev;
> + chgin_otg_reg_cfg.driver_data = chg;
> + chgin_otg_reg_cfg.of_node = dev_of_node(dev);
> + chg->chgin_otg_rdev = devm_regulator_register(dev, &chgin_otg_reg_desc,
> + &chgin_otg_reg_cfg);
> + if (IS_ERR(chg->chgin_otg_rdev))
> + return dev_err_probe(dev, PTR_ERR(chg->chgin_otg_rdev),
> + "Failed to register chgin otg regulator");
> +
> + ret = devm_work_autocancel(dev, &chg->psy_work, psy_work_item);
> + if (ret)
> + return dev_err_probe(dev, ret, "Failed to initialize psy work");
> +
> + chg->nb.notifier_call = psy_changed;
> + ret = power_supply_reg_notifier(&chg->nb);
> + if (ret)
> + return dev_err_probe(dev, ret,
> + "Unable to register psy notifier");
> +
> + ret = devm_add_action_or_reset(dev, max_tcpci_unregister_psy_notifier,
> + &chg->nb);
> + if (ret)
> + return ret;
You could print a message here as well.
Cheers,
Andre'
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH v3 4/5] power: supply: max77759: add charger driver
2026-01-05 17:32 ` André Draszik
@ 2026-01-06 23:41 ` Amit Sunil Dhamne
2026-01-07 1:14 ` Amit Sunil Dhamne
0 siblings, 1 reply; 25+ messages in thread
From: Amit Sunil Dhamne @ 2026-01-06 23:41 UTC (permalink / raw)
To: André Draszik, Sebastian Reichel, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Lee Jones, Greg Kroah-Hartman,
Badhri Jagan Sridharan, Heikki Krogerus, Peter Griffin,
Tudor Ambarus, Alim Akhtar
Cc: linux-kernel, linux-pm, devicetree, linux-usb, linux-arm-kernel,
linux-samsung-soc, RD Babiera, Kyle Tso
Hi Andre',
On 1/5/26 9:32 AM, André Draszik wrote:
> Hi Amit,
>
> I haven't done a full review, but a few things caught my eye.
>
> On Sat, 2025-12-27 at 00:04 +0000, Amit Sunil Dhamne via B4 Relay wrote:
>> From: Amit Sunil Dhamne <amitsd@google.com>
>>
>> Add support for MAX77759 battery charger driver. This is a 4A 1-Cell
>> Li+/LiPoly dual input switch mode charger. While the device can support
>> USB & wireless charger inputs, this implementation only supports USB
>> input. This implementation supports both buck and boost modes.
>>
>> Signed-off-by: Amit Sunil Dhamne <amitsd@google.com>
>> ---
>> MAINTAINERS | 6 +
>> drivers/power/supply/Kconfig | 11 +
>> drivers/power/supply/Makefile | 1 +
>> drivers/power/supply/max77759_charger.c | 764 ++++++++++++++++++++++++++++++++
>> 4 files changed, 782 insertions(+)
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index dc731d37c8fe..26a9654ab75e 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -15539,6 +15539,12 @@ F: drivers/mfd/max77759.c
>> F: drivers/nvmem/max77759-nvmem.c
>> F: include/linux/mfd/max77759.h
>>
>> +MAXIM MAX77759 BATTERY CHARGER DRIVER
>> +M: Amit Sunil Dhamne <amitsd@google.com>
>> +L: linux-kernel@vger.kernel.org
>> +S: Maintained
>> +F: drivers/power/supply/max77759_charger.c
>> +
>> MAXIM MAX77802 PMIC REGULATOR DEVICE DRIVER
>> M: Javier Martinez Canillas <javier@dowhile0.org>
>> L: linux-kernel@vger.kernel.org
>> diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig
>> index 92f9f7aae92f..e172fd980fde 100644
>> --- a/drivers/power/supply/Kconfig
>> +++ b/drivers/power/supply/Kconfig
>> @@ -1132,4 +1132,15 @@ config FUEL_GAUGE_MM8013
>> the state of charge, temperature, cycle count, actual and design
>> capacity, etc.
>>
>> +config CHARGER_MAX77759
>> + tristate "MAX77759 Charger Driver"
>> + depends on MFD_MAX77759 && REGULATOR
>> + default MFD_MAX77759
>> + help
>> + Say M or Y here to enable the MAX77759 Charger Driver. MAX77759
>> + charger is a function of the MAX77759 PMIC. This is a dual input
>> + switch-mode charger. This driver supports buck and OTG boost modes.
>> +
>> + If built as a module, it will be called max77759_charger.
>> +
> It might make sense to add this block near the existing MAX77... charger drivers,
> while updating the tristate string and keeping alphabetical order of entries.
>
>> endif # POWER_SUPPLY
>> diff --git a/drivers/power/supply/Makefile b/drivers/power/supply/Makefile
>> index 4b79d5abc49a..6af905875ad5 100644
>> --- a/drivers/power/supply/Makefile
>> +++ b/drivers/power/supply/Makefile
>> @@ -128,3 +128,4 @@ obj-$(CONFIG_CHARGER_SURFACE) += surface_charger.o
>> obj-$(CONFIG_BATTERY_UG3105) += ug3105_battery.o
>> obj-$(CONFIG_CHARGER_QCOM_SMB2) += qcom_smbx.o
>> obj-$(CONFIG_FUEL_GAUGE_MM8013) += mm8013.o
>> +obj-$(CONFIG_CHARGER_MAX77759) += max77759_charger.o
>> diff --git a/drivers/power/supply/max77759_charger.c b/drivers/power/supply/max77759_charger.c
>> new file mode 100644
>> index 000000000000..3d255b069fb9
>> --- /dev/null
>> +++ b/drivers/power/supply/max77759_charger.c
>> @@ -0,0 +1,764 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * max77759_charger.c - Battery charger driver for MAX77759 charger device.
>> + *
>> + * Copyright 2025 Google LLC.
>> + */
>> +
>> +#include <linux/bitfield.h>
>> +#include <linux/cleanup.h>
>> +#include <linux/device.h>
>> +#include <linux/devm-helpers.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/irq.h>
>> +#include <linux/math64.h>
>> +#include <linux/mfd/max77759.h>
>> +#include <linux/module.h>
>> +#include <linux/mod_devicetable.h>
>> +#include <linux/mutex.h>
>> +#include <linux/of.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/power_supply.h>
>> +#include <linux/regmap.h>
>> +#include <linux/regulator/driver.h>
>> +#include <linux/string_choices.h>
>> +
>> +/* Default values for Fast Charge Current & Float Voltage */
>> +#define CHG_CC_DEFAULT_UA 2266770
>> +#define CHG_FV_DEFAULT_MV 4300
>> +
>> +#define FOREACH_IRQ(S) \
>> + S(AICL), \
>> + S(CHGIN), \
>> + S(CHG), \
>> + S(INLIM), \
>> + S(BAT_OILO), \
>> + S(CHG_STA_CC), \
>> + S(CHG_STA_CV), \
>> + S(CHG_STA_TO), \
>> + S(CHG_STA_DONE)
>> +
>> +#define GENERATE_ENUM(e) e
>> +#define GENERATE_STRING(s) #s
>> +
>> +enum {
>> + FOREACH_IRQ(GENERATE_ENUM)
>> +};
>> +
>> +static const char *const chgr_irqs_str[] = {
>> + FOREACH_IRQ(GENERATE_STRING)
>> +};
>> +
>> +static int irqs[ARRAY_SIZE(chgr_irqs_str)];
> No global variables please, this is not a singleton.
>
>> [...]
>>
>> +static int set_input_current_limit(struct max77759_charger *chg, int ilim_ua)
>> +{
>> + u32 regval;
>> +
>> + if (ilim_ua < 0)
>> + return -EINVAL;
>> +
>> + if (ilim_ua == 0)
>> + ilim_ua = MAX77759_CHGR_CHGIN_ILIM_MIN_UA;
>> + else if (ilim_ua > MAX77759_CHGR_CHGIN_ILIM_MAX_UA)
>> + ilim_ua = MAX77759_CHGR_CHGIN_ILIM_MAX_UA;
> What if ilim_ua == 1 (or any other value < min_uA)? You could use clamp()
> instead of open-coding.
>
>> +
>> + regval = val_to_regval(ilim_ua, MAX77759_CHGR_CHGIN_ILIM_MIN_UA,
>> + MAX77759_CHGR_CHGIN_ILIM_STEP_UA,
>> + MAX77759_CHGR_CHGIN_ILIM_REG_OFFSET);
>> + return regmap_update_bits(chg->regmap, MAX77759_CHGR_REG_CHG_CNFG_09,
>> + MAX77759_CHGR_REG_CHG_CNFG_09_CHGIN_ILIM,
>> + regval);
>> +}
>> +
>> +static const enum power_supply_property max77759_charger_props[] = {
>> + POWER_SUPPLY_PROP_ONLINE,
>> + POWER_SUPPLY_PROP_PRESENT,
>> + POWER_SUPPLY_PROP_STATUS,
>> + POWER_SUPPLY_PROP_CHARGE_TYPE,
>> + POWER_SUPPLY_PROP_HEALTH,
>> + POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX,
>> + POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE_MAX,
>> + POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT,
>> +};
>> +
>> +static int max77759_charger_get_property(struct power_supply *psy,
>> + enum power_supply_property psp,
>> + union power_supply_propval *pval)
>> +{
>> + struct max77759_charger *chg = power_supply_get_drvdata(psy);
>> + int ret;
>> +
>> + switch (psp) {
>> + case POWER_SUPPLY_PROP_ONLINE:
>> + ret = get_online(chg);
>> + break;
>> + case POWER_SUPPLY_PROP_PRESENT:
>> + ret = charger_input_valid(chg);
>> + break;
>> + case POWER_SUPPLY_PROP_STATUS:
>> + ret = get_status(chg);
>> + break;
>> + case POWER_SUPPLY_PROP_CHARGE_TYPE:
>> + ret = get_charge_type(chg);
>> + break;
>> + case POWER_SUPPLY_PROP_HEALTH:
>> + ret = get_health(chg);
>> + break;
>> + case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX:
>> + ret = get_fast_charge_current(chg);
>> + break;
>> + case POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE_MAX:
>> + ret = get_float_voltage(chg);
>> + break;
>> + case POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT:
>> + ret = get_input_current_limit(chg);
>> + break;
>> + default:
>> + ret = -EINVAL;
>> + }
>> +
>> + pval->intval = ret;
>> + return ret < 0 ? ret : 0;
>> +}
>> +
>> +static const struct power_supply_desc max77759_charger_desc = {
>> + .name = "max77759-charger",
>> + .type = POWER_SUPPLY_TYPE_USB,
>> + .properties = max77759_charger_props,
>> + .num_properties = ARRAY_SIZE(max77759_charger_props),
>> + .get_property = max77759_charger_get_property,
>> +};
>> +
>> +static int charger_set_mode(struct max77759_charger *chg,
>> + enum max77759_chgr_mode mode)
>> +{
>> + int ret;
>> +
>> + guard(mutex)(&chg->lock);
>> +
>> + if (chg->mode == mode)
>> + return 0;
>> +
>> + if ((mode == MAX77759_CHGR_MODE_CHG_BUCK_ON ||
>> + mode == MAX77759_CHGR_MODE_OTG_BOOST_ON) &&
>> + chg->mode != MAX77759_CHGR_MODE_OFF) {
>> + dev_err(chg->dev, "Invalid mode transition from %d to %d",
>> + chg->mode, mode);
>> + return -EINVAL;
>> + }
>> +
>> + ret = regmap_update_bits(chg->regmap, MAX77759_CHGR_REG_CHG_CNFG_00,
>> + MAX77759_CHGR_REG_CHG_CNFG_00_MODE, mode);
>> + if (ret)
>> + return ret;
>> +
>> + chg->mode = mode;
>> + return 0;
>> +}
>> +
>> +static int enable_chgin_otg(struct regulator_dev *rdev)
>> +{
>> + struct max77759_charger *chg = rdev_get_drvdata(rdev);
>> +
>> + return charger_set_mode(chg, MAX77759_CHGR_MODE_OTG_BOOST_ON);
>> +}
>> +
>> +static int disable_chgin_otg(struct regulator_dev *rdev)
>> +{
>> + struct max77759_charger *chg = rdev_get_drvdata(rdev);
>> +
>> + return charger_set_mode(chg, MAX77759_CHGR_MODE_OFF);
>> +}
>> +
>> +static int chgin_otg_status(struct regulator_dev *rdev)
>> +{
>> + struct max77759_charger *chg = rdev_get_drvdata(rdev);
>> +
>> + guard(mutex)(&chg->lock);
>> + return chg->mode == MAX77759_CHGR_MODE_OTG_BOOST_ON;
>> +}
>> +
>> +static const struct regulator_ops chgin_otg_reg_ops = {
>> + .enable = enable_chgin_otg,
>> + .disable = disable_chgin_otg,
>> + .is_enabled = chgin_otg_status,
>> +};
>> +
>> +static const struct regulator_desc chgin_otg_reg_desc = {
>> + .name = "chgin-otg",
>> + .of_match = of_match_ptr("chgin-otg-regulator"),
>> + .owner = THIS_MODULE,
>> + .ops = &chgin_otg_reg_ops,
>> + .fixed_uV = 5000000,
>> + .n_voltages = 1,
>> +};
>> +
>> +static irqreturn_t irq_handler(int irq, void *data)
>> +{
>> + struct max77759_charger *chg = data;
>> + struct device *dev = chg->dev;
>> + u32 chgint_ok;
>> + int i;
>> +
>> + regmap_read(chg->regmap, MAX77759_CHGR_REG_CHG_INT_OK, &chgint_ok);
> You might want to check the return value and return IRQ_NONE if it didn't
> work?
>
>> +
>> + for (i = 0; i < ARRAY_SIZE(irqs); i++) {
>> + if (irqs[i] == irq)
>> + break;
>> + }
>> +
>> + switch (i) {
>> + case AICL:
>> + dev_dbg(dev, "AICL mode: %s",
>> + str_no_yes(chgint_ok & MAX77759_CHGR_REG_CHG_INT_AICL));
>> + break;
>> + case CHGIN:
>> + dev_dbg(dev, "CHGIN input valid: %s",
>> + str_yes_no(chgint_ok & MAX77759_CHGR_REG_CHG_INT_CHGIN));
>> + break;
>> + case CHG:
>> + dev_dbg(dev, "CHG status okay/off: %s",
>> + str_yes_no(chgint_ok & MAX77759_CHGR_REG_CHG_INT_CHG));
>> + break;
>> + case INLIM:
>> + dev_dbg(dev, "Current Limit reached: %s",
>> + str_no_yes(chgint_ok & MAX77759_CHGR_REG_CHG_INT_INLIM));
>> + break;
>> + case BAT_OILO:
>> + dev_dbg(dev, "Battery over-current threshold crossed");
>> + break;
>> + case CHG_STA_CC:
>> + dev_dbg(dev, "Charger reached CC stage");
>> + break;
>> + case CHG_STA_CV:
>> + dev_dbg(dev, "Charger reached CV stage");
>> + break;
>> + case CHG_STA_TO:
>> + dev_dbg(dev, "Charger reached TO stage");
>> + break;
>> + case CHG_STA_DONE:
>> + dev_dbg(dev, "Charger reached TO stage");
>> + break;
> Are the above debug messages really all needed?
>
>> + default:
>> + dev_err(dev, "Unrecognized irq: %d", i);
>> + return IRQ_HANDLED;
> I'm not sure it should return IRQ_HANDLED in this case.
>
>> + }
>> +
>> + power_supply_changed(chg->psy);
>> + return IRQ_HANDLED;
>> +}
>> +
>> +static int max77759_init_irqhandler(struct max77759_charger *chg)
>> +{
>> + struct device *dev = chg->dev;
>> + unsigned long irq_flags;
>> + struct irq_data *irqd;
>> + int i, ret;
>> +
>> + for (i = 0; i < ARRAY_SIZE(chgr_irqs_str); i++) {
>> + ret = platform_get_irq_byname(to_platform_device(dev),
>> + chgr_irqs_str[i]);
>> + if (ret < 0) {
>> + dev_err(dev,
>> + "Failed to get irq resource for %s, ret=%d",
>> + chgr_irqs_str[i], ret);
>> + return ret;
>> + }
> You should use return dev_err_probe() here, and drop the additional dev_err_probe()
> in max77759_charger_probe().
>
>> +
>> + irqs[i] = ret;
>> + irq_flags = IRQF_ONESHOT;
>> + irqd = irq_get_irq_data(irqs[i]);
>> + if (irqd)
>> + irq_flags |= irqd_get_trigger_type(irqd);
> The above three lines are not needed, and then you can also drop irq_flags and
> use its value in the below call directly.
>
>> +
>> + ret = devm_request_threaded_irq(dev, irqs[i], NULL, irq_handler,
>> + irq_flags, dev_name(dev), chg);
>> + if (ret) {
>> + dev_err(dev,
>> + "Unable to register irq handler for %s, ret=%d",
>> + chgr_irqs_str[i], ret);
>> + return ret;
> dev_err_probe() please.
>
>> + }
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int max77759_charger_init(struct max77759_charger *chg)
>> +{
>> + struct power_supply_battery_info *info;
>> + u32 regval, fast_chg_curr, fv;
>> + int ret;
>> +
>> + regmap_read(chg->regmap, MAX77759_CHGR_REG_CHG_CNFG_00, ®val);
>> + chg->mode = FIELD_GET(MAX77759_CHGR_REG_CHG_CNFG_00_MODE, regval);
>> + ret = charger_set_mode(chg, MAX77759_CHGR_MODE_OFF);
>> + if (ret)
>> + return ret;
>> +
>> + if (power_supply_get_battery_info(chg->psy, &info)) {
>> + fv = CHG_FV_DEFAULT_MV;
>> + fast_chg_curr = CHG_CC_DEFAULT_UA;
>> + } else {
>> + fv = info->constant_charge_voltage_max_uv / 1000;
>> + fast_chg_curr = info->constant_charge_current_max_ua;
>> + }
>> +
>> + ret = set_fast_charge_current_limit(chg, fast_chg_curr);
>> + if (ret)
>> + return ret;
>> +
>> + ret = set_float_voltage_limit(chg, fv);
>> + if (ret)
>> + return ret;
>> +
>> + ret = unlock_prot_regs(chg, true);
>> + if (ret)
>> + return ret;
>> +
>> + /* Disable wireless charging input */
>> + regmap_update_bits(chg->regmap, MAX77759_CHGR_REG_CHG_CNFG_12,
>> + MAX77759_CHGR_REG_CHG_CNFG_12_WCINSEL, 0);
>> +
>> + regmap_update_bits(chg->regmap, MAX77759_CHGR_REG_CHG_CNFG_18,
>> + MAX77759_CHGR_REG_CHG_CNFG_18_WDTEN, 0);
> I think it's good practice to check return values.
>
>> +
>> + return unlock_prot_regs(chg, false);
>> +}
>> +
>> +static void psy_work_item(struct work_struct *work)
>> +{
>> + struct max77759_charger *chg =
>> + container_of(work, struct max77759_charger, psy_work);
>> + union power_supply_propval current_limit = { 0 }, online = { 0 };
>> + int ret;
>> +
>> + power_supply_get_property(chg->tcpm_psy, POWER_SUPPLY_PROP_CURRENT_MAX,
>> + ¤t_limit);
>> + power_supply_get_property(chg->tcpm_psy, POWER_SUPPLY_PROP_ONLINE,
>> + &online);
> Would it make sense to rework this and check the return values? Then you can also
> drop the greedy init at function entry.
>
>> +
>> + if (online.intval && current_limit.intval) {
>> + ret = set_input_current_limit(chg, current_limit.intval);
>> + if (ret)
>> + dev_err(chg->dev,
>> + "Unable to set current limit, ret=%d", ret);
>> +
>> + charger_set_mode(chg, MAX77759_CHGR_MODE_CHG_BUCK_ON);
>> + } else {
>> + charger_set_mode(chg, MAX77759_CHGR_MODE_OFF);
>> + }
>> +}
>> +
>> +static int psy_changed(struct notifier_block *nb, unsigned long evt, void *data)
>> +{
>> + struct max77759_charger *chg = container_of(nb, struct max77759_charger,
>> + nb);
>> + const char *psy_name = "tcpm-source";
>> + struct power_supply *psy = data;
>> +
>> + if (!strnstr(psy->desc->name, psy_name, strlen(psy_name)) ||
>> + evt != PSY_EVENT_PROP_CHANGED)
>> + return NOTIFY_OK;
>> +
>> + chg->tcpm_psy = psy;
>> + schedule_work(&chg->psy_work);
> Maybe add a newline here.
>
>> + return NOTIFY_OK;
>> +}
>> +
>> +static void max_tcpci_unregister_psy_notifier(void *nb)
>> +{
>> + power_supply_unreg_notifier(nb);
>> +}
>> +
>> +static int max77759_charger_probe(struct platform_device *pdev)
>> +{
>> + struct regulator_config chgin_otg_reg_cfg;
>> + struct power_supply_config psy_cfg;
>> + struct device *dev = &pdev->dev;
>> + struct max77759_charger *chg;
>> + int ret;
>> +
>> + device_set_of_node_from_dev(dev, dev->parent);
>> + chg = devm_kzalloc(dev, sizeof(*chg), GFP_KERNEL);
>> + if (!chg)
>> + return -ENOMEM;
>> +
>> + platform_set_drvdata(pdev, chg);
>> + chg->dev = dev;
>> + chg->regmap = dev_get_regmap(dev->parent, "charger");
>> + if (!chg->regmap)
>> + return dev_err_probe(dev, -ENODEV, "Missing regmap");
>> +
>> + ret = devm_mutex_init(dev, &chg->lock);
>> + if (ret)
>> + return dev_err_probe(dev, ret, "Failed to initialize lock");
>> +
>> + psy_cfg.fwnode = dev_fwnode(dev);
>> + psy_cfg.drv_data = chg;
>> + chg->psy = devm_power_supply_register(dev, &max77759_charger_desc,
>> + &psy_cfg);
>> + if (IS_ERR(chg->psy))
>> + return dev_err_probe(dev, -EPROBE_DEFER,
>> + "Failed to register psy, ret=%ld",
>> + PTR_ERR(chg->psy));
>> +
>> + ret = max77759_charger_init(chg);
>> + if (ret)
>> + return dev_err_probe(dev, ret,
>> + "Failed to initialize max77759 charger");
>> +
>> + chgin_otg_reg_cfg.dev = dev;
>> + chgin_otg_reg_cfg.driver_data = chg;
>> + chgin_otg_reg_cfg.of_node = dev_of_node(dev);
>> + chg->chgin_otg_rdev = devm_regulator_register(dev, &chgin_otg_reg_desc,
>> + &chgin_otg_reg_cfg);
>> + if (IS_ERR(chg->chgin_otg_rdev))
>> + return dev_err_probe(dev, PTR_ERR(chg->chgin_otg_rdev),
>> + "Failed to register chgin otg regulator");
>> +
>> + ret = devm_work_autocancel(dev, &chg->psy_work, psy_work_item);
>> + if (ret)
>> + return dev_err_probe(dev, ret, "Failed to initialize psy work");
>> +
>> + chg->nb.notifier_call = psy_changed;
>> + ret = power_supply_reg_notifier(&chg->nb);
>> + if (ret)
>> + return dev_err_probe(dev, ret,
>> + "Unable to register psy notifier");
>> +
>> + ret = devm_add_action_or_reset(dev, max_tcpci_unregister_psy_notifier,
>> + &chg->nb);
>> + if (ret)
>> + return ret;
> You could print a message here as well.
Thanks for the detailed review! I will fix all the flagged issues in the
next rev.
BR,
Amit
>
> Cheers,
> Andre'
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH v3 4/5] power: supply: max77759: add charger driver
2026-01-06 23:41 ` Amit Sunil Dhamne
@ 2026-01-07 1:14 ` Amit Sunil Dhamne
2026-01-12 13:47 ` André Draszik
0 siblings, 1 reply; 25+ messages in thread
From: Amit Sunil Dhamne @ 2026-01-07 1:14 UTC (permalink / raw)
To: André Draszik, Sebastian Reichel, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Lee Jones, Greg Kroah-Hartman,
Badhri Jagan Sridharan, Heikki Krogerus, Peter Griffin,
Tudor Ambarus, Alim Akhtar
Cc: linux-kernel, linux-pm, devicetree, linux-usb, linux-arm-kernel,
linux-samsung-soc, RD Babiera, Kyle Tso
On 1/6/26 3:41 PM, Amit Sunil Dhamne wrote:
> Hi Andre',
>
> On 1/5/26 9:32 AM, André Draszik wrote:
>> Hi Amit,
>>
>> I haven't done a full review, but a few things caught my eye.
>>
>> On Sat, 2025-12-27 at 00:04 +0000, Amit Sunil Dhamne via B4 Relay wrote:
>>> From: Amit Sunil Dhamne <amitsd@google.com>
>>>
>>> Add support for MAX77759 battery charger driver. This is a 4A 1-Cell
>>> Li+/LiPoly dual input switch mode charger. While the device can support
>>> USB & wireless charger inputs, this implementation only supports USB
>>> input. This implementation supports both buck and boost modes.
>>>
>>> Signed-off-by: Amit Sunil Dhamne <amitsd@google.com>
>>> ---
>>> MAINTAINERS | 6 +
>>> drivers/power/supply/Kconfig | 11 +
>>> drivers/power/supply/Makefile | 1 +
>>> drivers/power/supply/max77759_charger.c | 764
>>> ++++++++++++++++++++++++++++++++
>>> 4 files changed, 782 insertions(+)
>>>
>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>> index dc731d37c8fe..26a9654ab75e 100644
>>> --- a/MAINTAINERS
>>> +++ b/MAINTAINERS
>>> @@ -15539,6 +15539,12 @@ F: drivers/mfd/max77759.c
>>> F: drivers/nvmem/max77759-nvmem.c
>>> F: include/linux/mfd/max77759.h
>>> +MAXIM MAX77759 BATTERY CHARGER DRIVER
>>> +M: Amit Sunil Dhamne <amitsd@google.com>
>>> +L: linux-kernel@vger.kernel.org
>>> +S: Maintained
>>> +F: drivers/power/supply/max77759_charger.c
>>> +
>>> MAXIM MAX77802 PMIC REGULATOR DEVICE DRIVER
>>> M: Javier Martinez Canillas <javier@dowhile0.org>
>>> L: linux-kernel@vger.kernel.org
>>> diff --git a/drivers/power/supply/Kconfig
>>> b/drivers/power/supply/Kconfig
>>> index 92f9f7aae92f..e172fd980fde 100644
>>> --- a/drivers/power/supply/Kconfig
>>> +++ b/drivers/power/supply/Kconfig
>>> @@ -1132,4 +1132,15 @@ config FUEL_GAUGE_MM8013
>>> the state of charge, temperature, cycle count, actual and
>>> design
>>> capacity, etc.
>>> +config CHARGER_MAX77759
>>> + tristate "MAX77759 Charger Driver"
>>> + depends on MFD_MAX77759 && REGULATOR
>>> + default MFD_MAX77759
>>> + help
>>> + Say M or Y here to enable the MAX77759 Charger Driver. MAX77759
>>> + charger is a function of the MAX77759 PMIC. This is a dual input
>>> + switch-mode charger. This driver supports buck and OTG boost
>>> modes.
>>> +
>>> + If built as a module, it will be called max77759_charger.
>>> +
>> It might make sense to add this block near the existing MAX77...
>> charger drivers,
>> while updating the tristate string and keeping alphabetical order of
>> entries.
>>
>>> endif # POWER_SUPPLY
>>> diff --git a/drivers/power/supply/Makefile
>>> b/drivers/power/supply/Makefile
>>> index 4b79d5abc49a..6af905875ad5 100644
>>> --- a/drivers/power/supply/Makefile
>>> +++ b/drivers/power/supply/Makefile
>>> @@ -128,3 +128,4 @@ obj-$(CONFIG_CHARGER_SURFACE) +=
>>> surface_charger.o
>>> obj-$(CONFIG_BATTERY_UG3105) += ug3105_battery.o
>>> obj-$(CONFIG_CHARGER_QCOM_SMB2) += qcom_smbx.o
>>> obj-$(CONFIG_FUEL_GAUGE_MM8013) += mm8013.o
>>> +obj-$(CONFIG_CHARGER_MAX77759) += max77759_charger.o
>>> diff --git a/drivers/power/supply/max77759_charger.c
>>> b/drivers/power/supply/max77759_charger.c
>>> new file mode 100644
>>> index 000000000000..3d255b069fb9
>>> --- /dev/null
>>> +++ b/drivers/power/supply/max77759_charger.c
>>> @@ -0,0 +1,764 @@
>>> +// SPDX-License-Identifier: GPL-2.0-only
>>> +/*
>>> + * max77759_charger.c - Battery charger driver for MAX77759 charger
>>> device.
>>> + *
>>> + * Copyright 2025 Google LLC.
>>> + */
>>> +
>>> +#include <linux/bitfield.h>
>>> +#include <linux/cleanup.h>
>>> +#include <linux/device.h>
>>> +#include <linux/devm-helpers.h>
>>> +#include <linux/interrupt.h>
>>> +#include <linux/irq.h>
>>> +#include <linux/math64.h>
>>> +#include <linux/mfd/max77759.h>
>>> +#include <linux/module.h>
>>> +#include <linux/mod_devicetable.h>
>>> +#include <linux/mutex.h>
>>> +#include <linux/of.h>
>>> +#include <linux/platform_device.h>
>>> +#include <linux/power_supply.h>
>>> +#include <linux/regmap.h>
>>> +#include <linux/regulator/driver.h>
>>> +#include <linux/string_choices.h>
>>> +
>>> +/* Default values for Fast Charge Current & Float Voltage */
>>> +#define CHG_CC_DEFAULT_UA 2266770
>>> +#define CHG_FV_DEFAULT_MV 4300
>>> +
>>> +#define FOREACH_IRQ(S) \
>>> + S(AICL), \
>>> + S(CHGIN), \
>>> + S(CHG), \
>>> + S(INLIM), \
>>> + S(BAT_OILO), \
>>> + S(CHG_STA_CC), \
>>> + S(CHG_STA_CV), \
>>> + S(CHG_STA_TO), \
>>> + S(CHG_STA_DONE)
>>> +
>>> +#define GENERATE_ENUM(e) e
>>> +#define GENERATE_STRING(s) #s
>>> +
>>> +enum {
>>> + FOREACH_IRQ(GENERATE_ENUM)
>>> +};
>>> +
>>> +static const char *const chgr_irqs_str[] = {
>>> + FOREACH_IRQ(GENERATE_STRING)
>>> +};
>>> +
>>> +static int irqs[ARRAY_SIZE(chgr_irqs_str)];
>> No global variables please, this is not a singleton.
>>
>>> [...]
>>>
>>> +static int set_input_current_limit(struct max77759_charger *chg,
>>> int ilim_ua)
>>> +{
>>> + u32 regval;
>>> +
>>> + if (ilim_ua < 0)
>>> + return -EINVAL;
>>> +
>>> + if (ilim_ua == 0)
>>> + ilim_ua = MAX77759_CHGR_CHGIN_ILIM_MIN_UA;
>>> + else if (ilim_ua > MAX77759_CHGR_CHGIN_ILIM_MAX_UA)
>>> + ilim_ua = MAX77759_CHGR_CHGIN_ILIM_MAX_UA;
>> What if ilim_ua == 1 (or any other value < min_uA)? You could use
>> clamp()
>> instead of open-coding.
>>
>>> +
>>> + regval = val_to_regval(ilim_ua, MAX77759_CHGR_CHGIN_ILIM_MIN_UA,
>>> + MAX77759_CHGR_CHGIN_ILIM_STEP_UA,
>>> + MAX77759_CHGR_CHGIN_ILIM_REG_OFFSET);
>>> + return regmap_update_bits(chg->regmap,
>>> MAX77759_CHGR_REG_CHG_CNFG_09,
>>> + MAX77759_CHGR_REG_CHG_CNFG_09_CHGIN_ILIM,
>>> + regval);
>>> +}
>>> +
>>> +static const enum power_supply_property max77759_charger_props[] = {
>>> + POWER_SUPPLY_PROP_ONLINE,
>>> + POWER_SUPPLY_PROP_PRESENT,
>>> + POWER_SUPPLY_PROP_STATUS,
>>> + POWER_SUPPLY_PROP_CHARGE_TYPE,
>>> + POWER_SUPPLY_PROP_HEALTH,
>>> + POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX,
>>> + POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE_MAX,
>>> + POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT,
>>> +};
>>> +
>>> +static int max77759_charger_get_property(struct power_supply *psy,
>>> + enum power_supply_property psp,
>>> + union power_supply_propval *pval)
>>> +{
>>> + struct max77759_charger *chg = power_supply_get_drvdata(psy);
>>> + int ret;
>>> +
>>> + switch (psp) {
>>> + case POWER_SUPPLY_PROP_ONLINE:
>>> + ret = get_online(chg);
>>> + break;
>>> + case POWER_SUPPLY_PROP_PRESENT:
>>> + ret = charger_input_valid(chg);
>>> + break;
>>> + case POWER_SUPPLY_PROP_STATUS:
>>> + ret = get_status(chg);
>>> + break;
>>> + case POWER_SUPPLY_PROP_CHARGE_TYPE:
>>> + ret = get_charge_type(chg);
>>> + break;
>>> + case POWER_SUPPLY_PROP_HEALTH:
>>> + ret = get_health(chg);
>>> + break;
>>> + case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX:
>>> + ret = get_fast_charge_current(chg);
>>> + break;
>>> + case POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE_MAX:
>>> + ret = get_float_voltage(chg);
>>> + break;
>>> + case POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT:
>>> + ret = get_input_current_limit(chg);
>>> + break;
>>> + default:
>>> + ret = -EINVAL;
>>> + }
>>> +
>>> + pval->intval = ret;
>>> + return ret < 0 ? ret : 0;
>>> +}
>>> +
>>> +static const struct power_supply_desc max77759_charger_desc = {
>>> + .name = "max77759-charger",
>>> + .type = POWER_SUPPLY_TYPE_USB,
>>> + .properties = max77759_charger_props,
>>> + .num_properties = ARRAY_SIZE(max77759_charger_props),
>>> + .get_property = max77759_charger_get_property,
>>> +};
>>> +
>>> +static int charger_set_mode(struct max77759_charger *chg,
>>> + enum max77759_chgr_mode mode)
>>> +{
>>> + int ret;
>>> +
>>> + guard(mutex)(&chg->lock);
>>> +
>>> + if (chg->mode == mode)
>>> + return 0;
>>> +
>>> + if ((mode == MAX77759_CHGR_MODE_CHG_BUCK_ON ||
>>> + mode == MAX77759_CHGR_MODE_OTG_BOOST_ON) &&
>>> + chg->mode != MAX77759_CHGR_MODE_OFF) {
>>> + dev_err(chg->dev, "Invalid mode transition from %d to %d",
>>> + chg->mode, mode);
>>> + return -EINVAL;
>>> + }
>>> +
>>> + ret = regmap_update_bits(chg->regmap,
>>> MAX77759_CHGR_REG_CHG_CNFG_00,
>>> + MAX77759_CHGR_REG_CHG_CNFG_00_MODE, mode);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + chg->mode = mode;
>>> + return 0;
>>> +}
>>> +
>>> +static int enable_chgin_otg(struct regulator_dev *rdev)
>>> +{
>>> + struct max77759_charger *chg = rdev_get_drvdata(rdev);
>>> +
>>> + return charger_set_mode(chg, MAX77759_CHGR_MODE_OTG_BOOST_ON);
>>> +}
>>> +
>>> +static int disable_chgin_otg(struct regulator_dev *rdev)
>>> +{
>>> + struct max77759_charger *chg = rdev_get_drvdata(rdev);
>>> +
>>> + return charger_set_mode(chg, MAX77759_CHGR_MODE_OFF);
>>> +}
>>> +
>>> +static int chgin_otg_status(struct regulator_dev *rdev)
>>> +{
>>> + struct max77759_charger *chg = rdev_get_drvdata(rdev);
>>> +
>>> + guard(mutex)(&chg->lock);
>>> + return chg->mode == MAX77759_CHGR_MODE_OTG_BOOST_ON;
>>> +}
>>> +
>>> +static const struct regulator_ops chgin_otg_reg_ops = {
>>> + .enable = enable_chgin_otg,
>>> + .disable = disable_chgin_otg,
>>> + .is_enabled = chgin_otg_status,
>>> +};
>>> +
>>> +static const struct regulator_desc chgin_otg_reg_desc = {
>>> + .name = "chgin-otg",
>>> + .of_match = of_match_ptr("chgin-otg-regulator"),
>>> + .owner = THIS_MODULE,
>>> + .ops = &chgin_otg_reg_ops,
>>> + .fixed_uV = 5000000,
>>> + .n_voltages = 1,
>>> +};
>>> +
>>> +static irqreturn_t irq_handler(int irq, void *data)
>>> +{
>>> + struct max77759_charger *chg = data;
>>> + struct device *dev = chg->dev;
>>> + u32 chgint_ok;
>>> + int i;
>>> +
>>> + regmap_read(chg->regmap, MAX77759_CHGR_REG_CHG_INT_OK,
>>> &chgint_ok);
>> You might want to check the return value and return IRQ_NONE if it
>> didn't
>> work?
>>
>>> +
>>> + for (i = 0; i < ARRAY_SIZE(irqs); i++) {
>>> + if (irqs[i] == irq)
>>> + break;
>>> + }
>>> +
>>> + switch (i) {
>>> + case AICL:
>>> + dev_dbg(dev, "AICL mode: %s",
>>> + str_no_yes(chgint_ok & MAX77759_CHGR_REG_CHG_INT_AICL));
>>> + break;
>>> + case CHGIN:
>>> + dev_dbg(dev, "CHGIN input valid: %s",
>>> + str_yes_no(chgint_ok & MAX77759_CHGR_REG_CHG_INT_CHGIN));
>>> + break;
>>> + case CHG:
>>> + dev_dbg(dev, "CHG status okay/off: %s",
>>> + str_yes_no(chgint_ok & MAX77759_CHGR_REG_CHG_INT_CHG));
>>> + break;
>>> + case INLIM:
>>> + dev_dbg(dev, "Current Limit reached: %s",
>>> + str_no_yes(chgint_ok & MAX77759_CHGR_REG_CHG_INT_INLIM));
>>> + break;
>>> + case BAT_OILO:
>>> + dev_dbg(dev, "Battery over-current threshold crossed");
>>> + break;
>>> + case CHG_STA_CC:
>>> + dev_dbg(dev, "Charger reached CC stage");
>>> + break;
>>> + case CHG_STA_CV:
>>> + dev_dbg(dev, "Charger reached CV stage");
>>> + break;
>>> + case CHG_STA_TO:
>>> + dev_dbg(dev, "Charger reached TO stage");
>>> + break;
>>> + case CHG_STA_DONE:
>>> + dev_dbg(dev, "Charger reached TO stage");
>>> + break;
>> Are the above debug messages really all needed?
I forgot to respond to this comment in my previous email.
I think we can keep AICL, BAT_OILO, INLIM. They're either special
conditions (AICL) or faulty conditions (like BAT_OILO) and we can in
fact keep them at dev_info level. Rest can be removed and a
power_supply_changed() is sufficient.
Let me know what you think?
BR,
Amit
>>
>>> + default:
>>> + dev_err(dev, "Unrecognized irq: %d", i);
>>> + return IRQ_HANDLED;
>> I'm not sure it should return IRQ_HANDLED in this case.
>>
>>> + }
>>> +
>>> + power_supply_changed(chg->psy);
>>> + return IRQ_HANDLED;
>>> +}
>>> +
>>> +static int max77759_init_irqhandler(struct max77759_charger *chg)
>>> +{
>>> + struct device *dev = chg->dev;
>>> + unsigned long irq_flags;
>>> + struct irq_data *irqd;
>>> + int i, ret;
>>> +
>>> + for (i = 0; i < ARRAY_SIZE(chgr_irqs_str); i++) {
>>> + ret = platform_get_irq_byname(to_platform_device(dev),
>>> + chgr_irqs_str[i]);
>>> + if (ret < 0) {
>>> + dev_err(dev,
>>> + "Failed to get irq resource for %s, ret=%d",
>>> + chgr_irqs_str[i], ret);
>>> + return ret;
>>> + }
>> You should use return dev_err_probe() here, and drop the additional
>> dev_err_probe()
>> in max77759_charger_probe().
>>
>>> +
>>> + irqs[i] = ret;
>>> + irq_flags = IRQF_ONESHOT;
>>> + irqd = irq_get_irq_data(irqs[i]);
>>> + if (irqd)
>>> + irq_flags |= irqd_get_trigger_type(irqd);
>> The above three lines are not needed, and then you can also drop
>> irq_flags and
>> use its value in the below call directly.
>>
>>> +
>>> + ret = devm_request_threaded_irq(dev, irqs[i], NULL,
>>> irq_handler,
>>> + irq_flags, dev_name(dev), chg);
>>> + if (ret) {
>>> + dev_err(dev,
>>> + "Unable to register irq handler for %s, ret=%d",
>>> + chgr_irqs_str[i], ret);
>>> + return ret;
>> dev_err_probe() please.
>>
>>> + }
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int max77759_charger_init(struct max77759_charger *chg)
>>> +{
>>> + struct power_supply_battery_info *info;
>>> + u32 regval, fast_chg_curr, fv;
>>> + int ret;
>>> +
>>> + regmap_read(chg->regmap, MAX77759_CHGR_REG_CHG_CNFG_00, ®val);
>>> + chg->mode = FIELD_GET(MAX77759_CHGR_REG_CHG_CNFG_00_MODE, regval);
>>> + ret = charger_set_mode(chg, MAX77759_CHGR_MODE_OFF);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + if (power_supply_get_battery_info(chg->psy, &info)) {
>>> + fv = CHG_FV_DEFAULT_MV;
>>> + fast_chg_curr = CHG_CC_DEFAULT_UA;
>>> + } else {
>>> + fv = info->constant_charge_voltage_max_uv / 1000;
>>> + fast_chg_curr = info->constant_charge_current_max_ua;
>>> + }
>>> +
>>> + ret = set_fast_charge_current_limit(chg, fast_chg_curr);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + ret = set_float_voltage_limit(chg, fv);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + ret = unlock_prot_regs(chg, true);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + /* Disable wireless charging input */
>>> + regmap_update_bits(chg->regmap, MAX77759_CHGR_REG_CHG_CNFG_12,
>>> + MAX77759_CHGR_REG_CHG_CNFG_12_WCINSEL, 0);
>>> +
>>> + regmap_update_bits(chg->regmap, MAX77759_CHGR_REG_CHG_CNFG_18,
>>> + MAX77759_CHGR_REG_CHG_CNFG_18_WDTEN, 0);
>> I think it's good practice to check return values.
>>
>>> +
>>> + return unlock_prot_regs(chg, false);
>>> +}
>>> +
>>> +static void psy_work_item(struct work_struct *work)
>>> +{
>>> + struct max77759_charger *chg =
>>> + container_of(work, struct max77759_charger, psy_work);
>>> + union power_supply_propval current_limit = { 0 }, online = { 0 };
>>> + int ret;
>>> +
>>> + power_supply_get_property(chg->tcpm_psy,
>>> POWER_SUPPLY_PROP_CURRENT_MAX,
>>> + ¤t_limit);
>>> + power_supply_get_property(chg->tcpm_psy, POWER_SUPPLY_PROP_ONLINE,
>>> + &online);
>> Would it make sense to rework this and check the return values? Then
>> you can also
>> drop the greedy init at function entry.
>>
>>> +
>>> + if (online.intval && current_limit.intval) {
>>> + ret = set_input_current_limit(chg, current_limit.intval);
>>> + if (ret)
>>> + dev_err(chg->dev,
>>> + "Unable to set current limit, ret=%d", ret);
>>> +
>>> + charger_set_mode(chg, MAX77759_CHGR_MODE_CHG_BUCK_ON);
>>> + } else {
>>> + charger_set_mode(chg, MAX77759_CHGR_MODE_OFF);
>>> + }
>>> +}
>>> +
>>> +static int psy_changed(struct notifier_block *nb, unsigned long
>>> evt, void *data)
>>> +{
>>> + struct max77759_charger *chg = container_of(nb, struct
>>> max77759_charger,
>>> + nb);
>>> + const char *psy_name = "tcpm-source";
>>> + struct power_supply *psy = data;
>>> +
>>> + if (!strnstr(psy->desc->name, psy_name, strlen(psy_name)) ||
>>> + evt != PSY_EVENT_PROP_CHANGED)
>>> + return NOTIFY_OK;
>>> +
>>> + chg->tcpm_psy = psy;
>>> + schedule_work(&chg->psy_work);
>> Maybe add a newline here.
>>
>>> + return NOTIFY_OK;
>>> +}
>>> +
>>> +static void max_tcpci_unregister_psy_notifier(void *nb)
>>> +{
>>> + power_supply_unreg_notifier(nb);
>>> +}
>>> +
>>> +static int max77759_charger_probe(struct platform_device *pdev)
>>> +{
>>> + struct regulator_config chgin_otg_reg_cfg;
>>> + struct power_supply_config psy_cfg;
>>> + struct device *dev = &pdev->dev;
>>> + struct max77759_charger *chg;
>>> + int ret;
>>> +
>>> + device_set_of_node_from_dev(dev, dev->parent);
>>> + chg = devm_kzalloc(dev, sizeof(*chg), GFP_KERNEL);
>>> + if (!chg)
>>> + return -ENOMEM;
>>> +
>>> + platform_set_drvdata(pdev, chg);
>>> + chg->dev = dev;
>>> + chg->regmap = dev_get_regmap(dev->parent, "charger");
>>> + if (!chg->regmap)
>>> + return dev_err_probe(dev, -ENODEV, "Missing regmap");
>>> +
>>> + ret = devm_mutex_init(dev, &chg->lock);
>>> + if (ret)
>>> + return dev_err_probe(dev, ret, "Failed to initialize lock");
>>> +
>>> + psy_cfg.fwnode = dev_fwnode(dev);
>>> + psy_cfg.drv_data = chg;
>>> + chg->psy = devm_power_supply_register(dev, &max77759_charger_desc,
>>> + &psy_cfg);
>>> + if (IS_ERR(chg->psy))
>>> + return dev_err_probe(dev, -EPROBE_DEFER,
>>> + "Failed to register psy, ret=%ld",
>>> + PTR_ERR(chg->psy));
>>> +
>>> + ret = max77759_charger_init(chg);
>>> + if (ret)
>>> + return dev_err_probe(dev, ret,
>>> + "Failed to initialize max77759 charger");
>>> +
>>> + chgin_otg_reg_cfg.dev = dev;
>>> + chgin_otg_reg_cfg.driver_data = chg;
>>> + chgin_otg_reg_cfg.of_node = dev_of_node(dev);
>>> + chg->chgin_otg_rdev = devm_regulator_register(dev,
>>> &chgin_otg_reg_desc,
>>> + &chgin_otg_reg_cfg);
>>> + if (IS_ERR(chg->chgin_otg_rdev))
>>> + return dev_err_probe(dev, PTR_ERR(chg->chgin_otg_rdev),
>>> + "Failed to register chgin otg regulator");
>>> +
>>> + ret = devm_work_autocancel(dev, &chg->psy_work, psy_work_item);
>>> + if (ret)
>>> + return dev_err_probe(dev, ret, "Failed to initialize psy
>>> work");
>>> +
>>> + chg->nb.notifier_call = psy_changed;
>>> + ret = power_supply_reg_notifier(&chg->nb);
>>> + if (ret)
>>> + return dev_err_probe(dev, ret,
>>> + "Unable to register psy notifier");
>>> +
>>> + ret = devm_add_action_or_reset(dev,
>>> max_tcpci_unregister_psy_notifier,
>>> + &chg->nb);
>>> + if (ret)
>>> + return ret;
>> You could print a message here as well.
>
> Thanks for the detailed review! I will fix all the flagged issues in
> the next rev.
>
>
> BR,
>
> Amit
>
>>
>> Cheers,
>> Andre'
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH v3 4/5] power: supply: max77759: add charger driver
2026-01-07 1:14 ` Amit Sunil Dhamne
@ 2026-01-12 13:47 ` André Draszik
2026-01-12 19:37 ` Amit Sunil Dhamne
0 siblings, 1 reply; 25+ messages in thread
From: André Draszik @ 2026-01-12 13:47 UTC (permalink / raw)
To: Amit Sunil Dhamne, Sebastian Reichel, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Lee Jones, Greg Kroah-Hartman,
Badhri Jagan Sridharan, Heikki Krogerus, Peter Griffin,
Tudor Ambarus, Alim Akhtar
Cc: linux-kernel, linux-pm, devicetree, linux-usb, linux-arm-kernel,
linux-samsung-soc, RD Babiera, Kyle Tso
Hi Amit,
On Tue, 2026-01-06 at 17:14 -0800, Amit Sunil Dhamne wrote:
>
> On 1/6/26 3:41 PM, Amit Sunil Dhamne wrote:
> > Hi Andre',
> >
> > On 1/5/26 9:32 AM, André Draszik wrote:
> > > Hi Amit,
> > >
> > > I haven't done a full review, but a few things caught my eye.
> > >
> > > On Sat, 2025-12-27 at 00:04 +0000, Amit Sunil Dhamne via B4 Relay wrote:
> > > >
> > > > diff --git a/drivers/power/supply/Makefile
> > > > b/drivers/power/supply/Makefile
> > > > index 4b79d5abc49a..6af905875ad5 100644
> > > > --- a/drivers/power/supply/Makefile
> > > > +++ b/drivers/power/supply/Makefile
> > > > [...]
> > > > +
> > > > +static irqreturn_t irq_handler(int irq, void *data)
> > > > +{
> > > > + struct max77759_charger *chg = data;
> > > > + struct device *dev = chg->dev;
> > > > + u32 chgint_ok;
> > > > + int i;
> > > > +
> > > > + regmap_read(chg->regmap, MAX77759_CHGR_REG_CHG_INT_OK,
> > > > &chgint_ok);
> > > You might want to check the return value and return IRQ_NONE if it
> > > didn't
> > > work?
> > >
> > > > +
> > > > + for (i = 0; i < ARRAY_SIZE(irqs); i++) {
> > > > + if (irqs[i] == irq)
> > > > + break;
> > > > + }
> > > > +
> > > > + switch (i) {
> > > > + case AICL:
> > > > + dev_dbg(dev, "AICL mode: %s",
> > > > + str_no_yes(chgint_ok & MAX77759_CHGR_REG_CHG_INT_AICL));
> > > > + break;
> > > > + case CHGIN:
> > > > + dev_dbg(dev, "CHGIN input valid: %s",
> > > > + str_yes_no(chgint_ok & MAX77759_CHGR_REG_CHG_INT_CHGIN));
> > > > + break;
> > > > + case CHG:
> > > > + dev_dbg(dev, "CHG status okay/off: %s",
> > > > + str_yes_no(chgint_ok & MAX77759_CHGR_REG_CHG_INT_CHG));
> > > > + break;
> > > > + case INLIM:
> > > > + dev_dbg(dev, "Current Limit reached: %s",
> > > > + str_no_yes(chgint_ok & MAX77759_CHGR_REG_CHG_INT_INLIM));
> > > > + break;
> > > > + case BAT_OILO:
> > > > + dev_dbg(dev, "Battery over-current threshold crossed");
> > > > + break;
> > > > + case CHG_STA_CC:
> > > > + dev_dbg(dev, "Charger reached CC stage");
> > > > + break;
> > > > + case CHG_STA_CV:
> > > > + dev_dbg(dev, "Charger reached CV stage");
> > > > + break;
> > > > + case CHG_STA_TO:
> > > > + dev_dbg(dev, "Charger reached TO stage");
> > > > + break;
> > > > + case CHG_STA_DONE:
> > > > + dev_dbg(dev, "Charger reached TO stage");
> > > > + break;
> > > Are the above debug messages really all needed?
>
> I forgot to respond to this comment in my previous email.
>
> I think we can keep AICL, BAT_OILO, INLIM. They're either special
> conditions (AICL) or faulty conditions (like BAT_OILO) and we can in
> fact keep them at dev_info level. Rest can be removed and a
> power_supply_changed() is sufficient.
>
> Let me know what you think?
I don't think dev_info() in an interrupt handler is appropriate. At
least it should be ratelimited.
If it's something special / unexpected that needs attention, having
a dev_dbg() message only will usually not be visible to anybody.
Also will the call to power_supply_changed() down below handle the
special conditions (e.g. convey to upper levels)? If not, can it be
made to do so?
Cheers,
Andre
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH v3 4/5] power: supply: max77759: add charger driver
2026-01-12 13:47 ` André Draszik
@ 2026-01-12 19:37 ` Amit Sunil Dhamne
2026-01-13 10:02 ` André Draszik
0 siblings, 1 reply; 25+ messages in thread
From: Amit Sunil Dhamne @ 2026-01-12 19:37 UTC (permalink / raw)
To: André Draszik, Sebastian Reichel, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Lee Jones, Greg Kroah-Hartman,
Badhri Jagan Sridharan, Heikki Krogerus, Peter Griffin,
Tudor Ambarus, Alim Akhtar
Cc: linux-kernel, linux-pm, devicetree, linux-usb, linux-arm-kernel,
linux-samsung-soc, RD Babiera, Kyle Tso
Hi Andre',
On 1/12/26 5:47 AM, André Draszik wrote:
> Hi Amit,
>
> On Tue, 2026-01-06 at 17:14 -0800, Amit Sunil Dhamne wrote:
>> On 1/6/26 3:41 PM, Amit Sunil Dhamne wrote:
>>> Hi Andre',
>>>
>>> On 1/5/26 9:32 AM, André Draszik wrote:
>>>> Hi Amit,
>>>>
>>>> I haven't done a full review, but a few things caught my eye.
>>>>
>>>> On Sat, 2025-12-27 at 00:04 +0000, Amit Sunil Dhamne via B4 Relay wrote:
>>>>> diff --git a/drivers/power/supply/Makefile
>>>>> b/drivers/power/supply/Makefile
>>>>> index 4b79d5abc49a..6af905875ad5 100644
>>>>> --- a/drivers/power/supply/Makefile
>>>>> +++ b/drivers/power/supply/Makefile
>>>>> [...]
>>>>> +
>>>>> +static irqreturn_t irq_handler(int irq, void *data)
>>>>> +{
>>>>> + struct max77759_charger *chg = data;
>>>>> + struct device *dev = chg->dev;
>>>>> + u32 chgint_ok;
>>>>> + int i;
>>>>> +
>>>>> + regmap_read(chg->regmap, MAX77759_CHGR_REG_CHG_INT_OK,
>>>>> &chgint_ok);
>>>> You might want to check the return value and return IRQ_NONE if it
>>>> didn't
>>>> work?
>>>>
>>>>> +
>>>>> + for (i = 0; i < ARRAY_SIZE(irqs); i++) {
>>>>> + if (irqs[i] == irq)
>>>>> + break;
>>>>> + }
>>>>> +
>>>>> + switch (i) {
>>>>> + case AICL:
>>>>> + dev_dbg(dev, "AICL mode: %s",
>>>>> + str_no_yes(chgint_ok & MAX77759_CHGR_REG_CHG_INT_AICL));
>>>>> + break;
>>>>> + case CHGIN:
>>>>> + dev_dbg(dev, "CHGIN input valid: %s",
>>>>> + str_yes_no(chgint_ok & MAX77759_CHGR_REG_CHG_INT_CHGIN));
>>>>> + break;
>>>>> + case CHG:
>>>>> + dev_dbg(dev, "CHG status okay/off: %s",
>>>>> + str_yes_no(chgint_ok & MAX77759_CHGR_REG_CHG_INT_CHG));
>>>>> + break;
>>>>> + case INLIM:
>>>>> + dev_dbg(dev, "Current Limit reached: %s",
>>>>> + str_no_yes(chgint_ok & MAX77759_CHGR_REG_CHG_INT_INLIM));
>>>>> + break;
>>>>> + case BAT_OILO:
>>>>> + dev_dbg(dev, "Battery over-current threshold crossed");
>>>>> + break;
>>>>> + case CHG_STA_CC:
>>>>> + dev_dbg(dev, "Charger reached CC stage");
>>>>> + break;
>>>>> + case CHG_STA_CV:
>>>>> + dev_dbg(dev, "Charger reached CV stage");
>>>>> + break;
>>>>> + case CHG_STA_TO:
>>>>> + dev_dbg(dev, "Charger reached TO stage");
>>>>> + break;
>>>>> + case CHG_STA_DONE:
>>>>> + dev_dbg(dev, "Charger reached TO stage");
>>>>> + break;
>>>> Are the above debug messages really all needed?
>> I forgot to respond to this comment in my previous email.
>>
>> I think we can keep AICL, BAT_OILO, INLIM. They're either special
>> conditions (AICL) or faulty conditions (like BAT_OILO) and we can in
>> fact keep them at dev_info level. Rest can be removed and a
>> power_supply_changed() is sufficient.
>>
>> Let me know what you think?
> I don't think dev_info() in an interrupt handler is appropriate. At
> least it should be ratelimited.
>
> If it's something special / unexpected that needs attention, having
> a dev_dbg() message only will usually not be visible to anybody.
I agree. I can change the prints to dev_info_ratelimited for the stuff
we care about.
>
> Also will the call to power_supply_changed() down below handle the
> special conditions (e.g. convey to upper levels)? If not, can it be
> made to do so?
Yes it does, as I can see a call to kobject_uevent() inside
power_supply_changed_work(). Also, power_supply_changed() also notifies
other subsystems that have registered their notifiers downstream of this
power_supply object. So I believe we're good there.
If all the above sounds good, I will proceed with sending the next
revision including the fixes :).
BR,
Amit
>
> Cheers,
> Andre
>
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH v3 4/5] power: supply: max77759: add charger driver
2026-01-12 19:37 ` Amit Sunil Dhamne
@ 2026-01-13 10:02 ` André Draszik
2026-01-14 0:47 ` Amit Sunil Dhamne
0 siblings, 1 reply; 25+ messages in thread
From: André Draszik @ 2026-01-13 10:02 UTC (permalink / raw)
To: Amit Sunil Dhamne, Sebastian Reichel, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Lee Jones, Greg Kroah-Hartman,
Badhri Jagan Sridharan, Heikki Krogerus, Peter Griffin,
Tudor Ambarus, Alim Akhtar
Cc: linux-kernel, linux-pm, devicetree, linux-usb, linux-arm-kernel,
linux-samsung-soc, RD Babiera, Kyle Tso
Hi Amit,
On Mon, 2026-01-12 at 11:37 -0800, Amit Sunil Dhamne wrote:
> Hi Andre',
>
> On 1/12/26 5:47 AM, André Draszik wrote:
> > Hi Amit,
> >
> > On Tue, 2026-01-06 at 17:14 -0800, Amit Sunil Dhamne wrote:
> > > On 1/6/26 3:41 PM, Amit Sunil Dhamne wrote:
> > > > Hi Andre',
> > > >
> > > > On 1/5/26 9:32 AM, André Draszik wrote:
> > > > > Hi Amit,
> > > > >
> > > > > I haven't done a full review, but a few things caught my eye.
> > > > >
> > > > > On Sat, 2025-12-27 at 00:04 +0000, Amit Sunil Dhamne via B4 Relay wrote:
> > > > > > diff --git a/drivers/power/supply/Makefile
> > > > > > b/drivers/power/supply/Makefile
> > > > > > index 4b79d5abc49a..6af905875ad5 100644
> > > > > > --- a/drivers/power/supply/Makefile
> > > > > > +++ b/drivers/power/supply/Makefile
> > > > > > [...]
> > > > > > +
> > > > > > +static irqreturn_t irq_handler(int irq, void *data)
> > > > > > +{
> > > > > > + struct max77759_charger *chg = data;
> > > > > > + struct device *dev = chg->dev;
> > > > > > + u32 chgint_ok;
> > > > > > + int i;
> > > > > > +
> > > > > > + regmap_read(chg->regmap, MAX77759_CHGR_REG_CHG_INT_OK,
> > > > > > &chgint_ok);
> > > > > You might want to check the return value and return IRQ_NONE if it
> > > > > didn't
> > > > > work?
> > > > >
> > > > > > +
> > > > > > + for (i = 0; i < ARRAY_SIZE(irqs); i++) {
> > > > > > + if (irqs[i] == irq)
> > > > > > + break;
> > > > > > + }
> > > > > > +
> > > > > > + switch (i) {
> > > > > > + case AICL:
> > > > > > + dev_dbg(dev, "AICL mode: %s",
> > > > > > + str_no_yes(chgint_ok & MAX77759_CHGR_REG_CHG_INT_AICL));
> > > > > > + break;
> > > > > > + case CHGIN:
> > > > > > + dev_dbg(dev, "CHGIN input valid: %s",
> > > > > > + str_yes_no(chgint_ok & MAX77759_CHGR_REG_CHG_INT_CHGIN));
> > > > > > + break;
> > > > > > + case CHG:
> > > > > > + dev_dbg(dev, "CHG status okay/off: %s",
> > > > > > + str_yes_no(chgint_ok & MAX77759_CHGR_REG_CHG_INT_CHG));
> > > > > > + break;
> > > > > > + case INLIM:
> > > > > > + dev_dbg(dev, "Current Limit reached: %s",
> > > > > > + str_no_yes(chgint_ok & MAX77759_CHGR_REG_CHG_INT_INLIM));
> > > > > > + break;
> > > > > > + case BAT_OILO:
> > > > > > + dev_dbg(dev, "Battery over-current threshold crossed");
> > > > > > + break;
> > > > > > + case CHG_STA_CC:
> > > > > > + dev_dbg(dev, "Charger reached CC stage");
> > > > > > + break;
> > > > > > + case CHG_STA_CV:
> > > > > > + dev_dbg(dev, "Charger reached CV stage");
> > > > > > + break;
> > > > > > + case CHG_STA_TO:
> > > > > > + dev_dbg(dev, "Charger reached TO stage");
> > > > > > + break;
> > > > > > + case CHG_STA_DONE:
> > > > > > + dev_dbg(dev, "Charger reached TO stage");
> > > > > > + break;
> > > > > Are the above debug messages really all needed?
> > > I forgot to respond to this comment in my previous email.
> > >
> > > I think we can keep AICL, BAT_OILO, INLIM. They're either special
> > > conditions (AICL) or faulty conditions (like BAT_OILO) and we can in
> > > fact keep them at dev_info level. Rest can be removed and a
> > > power_supply_changed() is sufficient.
> > >
> > > Let me know what you think?
> > I don't think dev_info() in an interrupt handler is appropriate. At
> > least it should be ratelimited.
> >
> > If it's something special / unexpected that needs attention, having
> > a dev_dbg() message only will usually not be visible to anybody.
>
> I agree. I can change the prints to dev_info_ratelimited for the stuff
> we care about.
If it's an erroneous condition, maybe warn or even err are more appropriate?
But then, what is the expectation upon the user observing these messages?
What can or should they do? Who is going to look at these and can do
something sensible based on them?
> >
> > Also will the call to power_supply_changed() down below handle the
> > special conditions (e.g. convey to upper levels)? If not, can it be
> > made to do so?
>
> Yes it does, as I can see a call to kobject_uevent() inside
> power_supply_changed_work(). Also, power_supply_changed() also notifies
> other subsystems that have registered their notifiers downstream of this
> power_supply object. So I believe we're good there.
If erroneous conditions are handled by other / upper layers, why print a
message in this interrupt handler in the first place?
Also, I just noticed there is a max77705 charger driver. It seems quite
similar to this one, maybe it can be leveraged / extended?
Cheers,
Andre'
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH v3 4/5] power: supply: max77759: add charger driver
2026-01-13 10:02 ` André Draszik
@ 2026-01-14 0:47 ` Amit Sunil Dhamne
2026-01-14 10:20 ` André Draszik
0 siblings, 1 reply; 25+ messages in thread
From: Amit Sunil Dhamne @ 2026-01-14 0:47 UTC (permalink / raw)
To: André Draszik, Sebastian Reichel, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Lee Jones, Greg Kroah-Hartman,
Badhri Jagan Sridharan, Heikki Krogerus, Peter Griffin,
Tudor Ambarus, Alim Akhtar
Cc: linux-kernel, linux-pm, devicetree, linux-usb, linux-arm-kernel,
linux-samsung-soc, RD Babiera, Kyle Tso
Hi Andre',
On 1/13/26 2:02 AM, André Draszik wrote:
> Hi Amit,
>
> On Mon, 2026-01-12 at 11:37 -0800, Amit Sunil Dhamne wrote:
>> Hi Andre',
>>
>> On 1/12/26 5:47 AM, André Draszik wrote:
>>> Hi Amit,
>>>
>>> On Tue, 2026-01-06 at 17:14 -0800, Amit Sunil Dhamne wrote:
>>>> On 1/6/26 3:41 PM, Amit Sunil Dhamne wrote:
>>>>> Hi Andre',
>>>>>
>>>>> On 1/5/26 9:32 AM, André Draszik wrote:
>>>>>> Hi Amit,
>>>>>>
>>>>>> I haven't done a full review, but a few things caught my eye.
>>>>>>
>>>>>> On Sat, 2025-12-27 at 00:04 +0000, Amit Sunil Dhamne via B4 Relay wrote:
>>>>>>> diff --git a/drivers/power/supply/Makefile
>>>>>>> b/drivers/power/supply/Makefile
>>>>>>> index 4b79d5abc49a..6af905875ad5 100644
>>>>>>> --- a/drivers/power/supply/Makefile
>>>>>>> +++ b/drivers/power/supply/Makefile
>>>>>>> [...]
>>>>>>> +
>>>>>>> +static irqreturn_t irq_handler(int irq, void *data)
>>>>>>> +{
>>>>>>> + struct max77759_charger *chg = data;
>>>>>>> + struct device *dev = chg->dev;
>>>>>>> + u32 chgint_ok;
>>>>>>> + int i;
>>>>>>> +
>>>>>>> + regmap_read(chg->regmap, MAX77759_CHGR_REG_CHG_INT_OK,
>>>>>>> &chgint_ok);
>>>>>> You might want to check the return value and return IRQ_NONE if it
>>>>>> didn't
>>>>>> work?
>>>>>>
>>>>>>> +
>>>>>>> + for (i = 0; i < ARRAY_SIZE(irqs); i++) {
>>>>>>> + if (irqs[i] == irq)
>>>>>>> + break;
>>>>>>> + }
>>>>>>> +
>>>>>>> + switch (i) {
>>>>>>> + case AICL:
>>>>>>> + dev_dbg(dev, "AICL mode: %s",
>>>>>>> + str_no_yes(chgint_ok & MAX77759_CHGR_REG_CHG_INT_AICL));
>>>>>>> + break;
>>>>>>> + case CHGIN:
>>>>>>> + dev_dbg(dev, "CHGIN input valid: %s",
>>>>>>> + str_yes_no(chgint_ok & MAX77759_CHGR_REG_CHG_INT_CHGIN));
>>>>>>> + break;
>>>>>>> + case CHG:
>>>>>>> + dev_dbg(dev, "CHG status okay/off: %s",
>>>>>>> + str_yes_no(chgint_ok & MAX77759_CHGR_REG_CHG_INT_CHG));
>>>>>>> + break;
>>>>>>> + case INLIM:
>>>>>>> + dev_dbg(dev, "Current Limit reached: %s",
>>>>>>> + str_no_yes(chgint_ok & MAX77759_CHGR_REG_CHG_INT_INLIM));
>>>>>>> + break;
>>>>>>> + case BAT_OILO:
>>>>>>> + dev_dbg(dev, "Battery over-current threshold crossed");
>>>>>>> + break;
>>>>>>> + case CHG_STA_CC:
>>>>>>> + dev_dbg(dev, "Charger reached CC stage");
>>>>>>> + break;
>>>>>>> + case CHG_STA_CV:
>>>>>>> + dev_dbg(dev, "Charger reached CV stage");
>>>>>>> + break;
>>>>>>> + case CHG_STA_TO:
>>>>>>> + dev_dbg(dev, "Charger reached TO stage");
>>>>>>> + break;
>>>>>>> + case CHG_STA_DONE:
>>>>>>> + dev_dbg(dev, "Charger reached TO stage");
>>>>>>> + break;
>>>>>> Are the above debug messages really all needed?
>>>> I forgot to respond to this comment in my previous email.
>>>>
>>>> I think we can keep AICL, BAT_OILO, INLIM. They're either special
>>>> conditions (AICL) or faulty conditions (like BAT_OILO) and we can in
>>>> fact keep them at dev_info level. Rest can be removed and a
>>>> power_supply_changed() is sufficient.
>>>>
>>>> Let me know what you think?
>>> I don't think dev_info() in an interrupt handler is appropriate. At
>>> least it should be ratelimited.
>>>
>>> If it's something special / unexpected that needs attention, having
>>> a dev_dbg() message only will usually not be visible to anybody.
>> I agree. I can change the prints to dev_info_ratelimited for the stuff
>> we care about.
> If it's an erroneous condition, maybe warn or even err are more appropriate?
>
> But then, what is the expectation upon the user observing these messages?
> What can or should they do? Who is going to look at these and can do
> something sensible based on them?
The logging will help in postmortem analysis which may or may not
possible with just publishing uevents to userspace hoping that they log
the psy properties. Illustrating a situation:
1. Over current situation happened where the Battery to System current
exceeds the BAT_OILO threshold. This would also generate an interrupt.
2. The MAX77759 takes protective measures if the condition lasts for a
certain specified time and reset. Resetting will cause Vsys to collapse
to 0 if the system is only battery powered.
3. It'd be better that the BAT_OILO interrupt is logged in dmesg,
instead of just delegating it to user space as user can debug this
condition by looking at last_kmsg or pstore.
4. This signal can help the user debug conditions such as moisture (this
signal + contaminant detection) or indicative of a mechanical failure.
I do agree though that this is a hypothetical or very rare situation and
if you have a strong opinion against this I am okay with removing the
prints completely.
>
>>> Also will the call to power_supply_changed() down below handle the
>>> special conditions (e.g. convey to upper levels)? If not, can it be
>>> made to do so?
>> Yes it does, as I can see a call to kobject_uevent() inside
>> power_supply_changed_work(). Also, power_supply_changed() also notifies
>> other subsystems that have registered their notifiers downstream of this
>> power_supply object. So I believe we're good there.
> If erroneous conditions are handled by other / upper layers, why print a
> message in this interrupt handler in the first place?
I tried illustrating an example above.
>
> Also, I just noticed there is a max77705 charger driver. It seems quite
> similar to this one, maybe it can be leveraged / extended?
Thanks for the feedback. I reviewed the max77705 charger driver. .
Here is a breakdown of why I believe a separate driver may be a better
approach:
Similarities:
1. Helper Functions: We could potentially leverage common logic for
get_charge_type, get_status, get_health, and get_input_current.
2. Register Access: MAX77705 uses regfield abstractions to handle
register operations which can also be potentially leveraged.
3. Initialization: Some hardware initialization steps appear similar,
though about 60% of the max77705 initialization (e.g., switching
frequency, WCIN regulation voltage, top-off time) is irrelevant for the
max77759 configuration I need.
Differences:
1. OTG Support: The max77759 driver supports OTG boost mode, which is a
key requirement for my use case. While the max77705 hardware might
support OTG based on its registers, the current driver implementation
does not support it.
2. TCPCI/TCPM Integration: The max77759 driver is explicitly architected
to work with a TCPCI/TCPM-compliant Type-C controller to set input
current limits dynamically. It is ambiguous whether the max77705 device
uses a standard TCPCI/TCPM model or a proprietary one.
3. Register Incompatibility: There are distinct register differences.
For example, the max77705 driver relies on BATP and BATP_DTLS registers,
which do not exist in the max77759. Conversely, the max77759 has a
dedicated second interrupt register (CHG_INT2) that reports critical
signals like BAT_OILO, SYS_UVLO, and charging stages, which appear
absent or handled differently in the max77705. Additionally, MAX77759
has input selection (wireless, usb) and uses it in the driver but it's
not evident from register definitions whether max77705 has it.
4. Parameter Calculations: The formulas for calculating parameters like
Fast Charge Current (CHGCC) and Float Voltage are different between the
two chips. Merging the drivers would require separate, chip-specific
getter/setter functions for these core properties.
5. Device-Specific Workarounds: The max77705 driver includes a
workaround in max77705_aicl_irq that is not relevant to the max77759.
There may also be future workarounds which may not be applicable to one
or the other.
Logistical Constraints: I don't have access to max77705 hardware or its
full datasheet. This makes it impossible for me to test any shared code
changes to ensure I haven't introduced regressions on the max77705. IMO,
given these constraints and technical divergences, maintaining separate
drivers maybe a better choice. Please let me know wdyt?
BR,
Amit
>
>
> Cheers,
> Andre'
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH v3 4/5] power: supply: max77759: add charger driver
2026-01-14 0:47 ` Amit Sunil Dhamne
@ 2026-01-14 10:20 ` André Draszik
2026-01-15 2:52 ` Amit Sunil Dhamne
0 siblings, 1 reply; 25+ messages in thread
From: André Draszik @ 2026-01-14 10:20 UTC (permalink / raw)
To: Amit Sunil Dhamne, Sebastian Reichel, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Lee Jones, Greg Kroah-Hartman,
Badhri Jagan Sridharan, Heikki Krogerus, Peter Griffin,
Tudor Ambarus, Alim Akhtar
Cc: linux-kernel, linux-pm, devicetree, linux-usb, linux-arm-kernel,
linux-samsung-soc, RD Babiera, Kyle Tso
On Tue, 2026-01-13 at 16:47 -0800, Amit Sunil Dhamne wrote:
> Hi Andre',
>
> On 1/13/26 2:02 AM, André Draszik wrote:
> > Hi Amit,
> >
> > On Mon, 2026-01-12 at 11:37 -0800, Amit Sunil Dhamne wrote:
> > > Hi Andre',
> > >
> > > On 1/12/26 5:47 AM, André Draszik wrote:
> > > > Hi Amit,
> > > >
> > > > On Tue, 2026-01-06 at 17:14 -0800, Amit Sunil Dhamne wrote:
> > > > > On 1/6/26 3:41 PM, Amit Sunil Dhamne wrote:
> > > > > > Hi Andre',
> > > > > >
> > > > > > On 1/5/26 9:32 AM, André Draszik wrote:
> > > > > > > Hi Amit,
> > > > > > >
> > > > > > > I haven't done a full review, but a few things caught my eye.
> > > > > > >
> > > > > > > On Sat, 2025-12-27 at 00:04 +0000, Amit Sunil Dhamne via B4 Relay wrote:
> > > > > > > > diff --git a/drivers/power/supply/Makefile
> > > > > > > > b/drivers/power/supply/Makefile
> > > > > > > > index 4b79d5abc49a..6af905875ad5 100644
> > > > > > > > --- a/drivers/power/supply/Makefile
> > > > > > > > +++ b/drivers/power/supply/Makefile
> > > > > > > > [...]
> > > > > > > > +
> > > > > > > > +static irqreturn_t irq_handler(int irq, void *data)
> > > > > > > > +{
> > > > > > > > + struct max77759_charger *chg = data;
> > > > > > > > + struct device *dev = chg->dev;
> > > > > > > > + u32 chgint_ok;
> > > > > > > > + int i;
> > > > > > > > +
> > > > > > > > + regmap_read(chg->regmap, MAX77759_CHGR_REG_CHG_INT_OK,
> > > > > > > > &chgint_ok);
> > > > > > > You might want to check the return value and return IRQ_NONE if it
> > > > > > > didn't
> > > > > > > work?
> > > > > > >
> > > > > > > > +
> > > > > > > > + for (i = 0; i < ARRAY_SIZE(irqs); i++) {
> > > > > > > > + if (irqs[i] == irq)
> > > > > > > > + break;
> > > > > > > > + }
> > > > > > > > +
> > > > > > > > + switch (i) {
> > > > > > > > + case AICL:
> > > > > > > > + dev_dbg(dev, "AICL mode: %s",
> > > > > > > > + str_no_yes(chgint_ok & MAX77759_CHGR_REG_CHG_INT_AICL));
> > > > > > > > + break;
> > > > > > > > + case CHGIN:
> > > > > > > > + dev_dbg(dev, "CHGIN input valid: %s",
> > > > > > > > + str_yes_no(chgint_ok & MAX77759_CHGR_REG_CHG_INT_CHGIN));
> > > > > > > > + break;
> > > > > > > > + case CHG:
> > > > > > > > + dev_dbg(dev, "CHG status okay/off: %s",
> > > > > > > > + str_yes_no(chgint_ok & MAX77759_CHGR_REG_CHG_INT_CHG));
> > > > > > > > + break;
> > > > > > > > + case INLIM:
> > > > > > > > + dev_dbg(dev, "Current Limit reached: %s",
> > > > > > > > + str_no_yes(chgint_ok & MAX77759_CHGR_REG_CHG_INT_INLIM));
> > > > > > > > + break;
> > > > > > > > + case BAT_OILO:
> > > > > > > > + dev_dbg(dev, "Battery over-current threshold crossed");
> > > > > > > > + break;
> > > > > > > > + case CHG_STA_CC:
> > > > > > > > + dev_dbg(dev, "Charger reached CC stage");
> > > > > > > > + break;
> > > > > > > > + case CHG_STA_CV:
> > > > > > > > + dev_dbg(dev, "Charger reached CV stage");
> > > > > > > > + break;
> > > > > > > > + case CHG_STA_TO:
> > > > > > > > + dev_dbg(dev, "Charger reached TO stage");
> > > > > > > > + break;
> > > > > > > > + case CHG_STA_DONE:
> > > > > > > > + dev_dbg(dev, "Charger reached TO stage");
> > > > > > > > + break;
> > > > > > > Are the above debug messages really all needed?
> > > > > I forgot to respond to this comment in my previous email.
> > > > >
> > > > > I think we can keep AICL, BAT_OILO, INLIM. They're either special
> > > > > conditions (AICL) or faulty conditions (like BAT_OILO) and we can in
> > > > > fact keep them at dev_info level. Rest can be removed and a
> > > > > power_supply_changed() is sufficient.
> > > > >
> > > > > Let me know what you think?
> > > > I don't think dev_info() in an interrupt handler is appropriate. At
> > > > least it should be ratelimited.
> > > >
> > > > If it's something special / unexpected that needs attention, having
> > > > a dev_dbg() message only will usually not be visible to anybody.
> > > I agree. I can change the prints to dev_info_ratelimited for the stuff
> > > we care about.
> > If it's an erroneous condition, maybe warn or even err are more appropriate?
> >
> > But then, what is the expectation upon the user observing these messages?
> > What can or should they do? Who is going to look at these and can do
> > something sensible based on them?
>
> The logging will help in postmortem analysis which may or may not
> possible with just publishing uevents to userspace hoping that they log
> the psy properties. Illustrating a situation:
>
> 1. Over current situation happened where the Battery to System current
> exceeds the BAT_OILO threshold. This would also generate an interrupt.
>
> 2. The MAX77759 takes protective measures if the condition lasts for a
> certain specified time and reset. Resetting will cause Vsys to collapse
> to 0 if the system is only battery powered.
>
> 3. It'd be better that the BAT_OILO interrupt is logged in dmesg,
> instead of just delegating it to user space as user can debug this
> condition by looking at last_kmsg or pstore.
>
> 4. This signal can help the user debug conditions such as moisture (this
> signal + contaminant detection) or indicative of a mechanical failure.
>
> I do agree though that this is a hypothetical or very rare situation and
> if you have a strong opinion against this I am okay with removing the
> prints completely.
Thanks for details. OK, they sound useful in this case, but should still
be warn, not dbg.
> > >
>
>
> >
> > Also, I just noticed there is a max77705 charger driver. It seems quite
> > similar to this one, maybe it can be leveraged / extended?
>
> Thanks for the feedback. I reviewed the max77705 charger driver. .
>
> Here is a breakdown of why I believe a separate driver may be a better
> approach:
[...]
Thanks for the analysis, I agree with your conclusion. Mainly I noticed that
as part of AICL interrupt handling that driver does a bit of work, while here
we don't. I am wondering if that is applicable here is well.
Cheers,
Andre'
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH v3 4/5] power: supply: max77759: add charger driver
2026-01-14 10:20 ` André Draszik
@ 2026-01-15 2:52 ` Amit Sunil Dhamne
0 siblings, 0 replies; 25+ messages in thread
From: Amit Sunil Dhamne @ 2026-01-15 2:52 UTC (permalink / raw)
To: André Draszik, Sebastian Reichel, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Lee Jones, Greg Kroah-Hartman,
Badhri Jagan Sridharan, Heikki Krogerus, Peter Griffin,
Tudor Ambarus, Alim Akhtar
Cc: linux-kernel, linux-pm, devicetree, linux-usb, linux-arm-kernel,
linux-samsung-soc, RD Babiera, Kyle Tso
On 1/14/26 2:20 AM, André Draszik wrote:
> On Tue, 2026-01-13 at 16:47 -0800, Amit Sunil Dhamne wrote:
>> Hi Andre',
>>
>> On 1/13/26 2:02 AM, André Draszik wrote:
>>> Hi Amit,
>>>
>>> On Mon, 2026-01-12 at 11:37 -0800, Amit Sunil Dhamne wrote:
>>>> Hi Andre',
>>>>
>>>> On 1/12/26 5:47 AM, André Draszik wrote:
>>>>> Hi Amit,
>>>>>
>>>>> On Tue, 2026-01-06 at 17:14 -0800, Amit Sunil Dhamne wrote:
>>>>>> On 1/6/26 3:41 PM, Amit Sunil Dhamne wrote:
>>>>>>> Hi Andre',
>>>>>>>
>>>>>>> On 1/5/26 9:32 AM, André Draszik wrote:
>>>>>>>> Hi Amit,
>>>>>>>>
>>>>>>>> I haven't done a full review, but a few things caught my eye.
>>>>>>>>
>>>>>>>> On Sat, 2025-12-27 at 00:04 +0000, Amit Sunil Dhamne via B4 Relay wrote:
>>>>>>>>> diff --git a/drivers/power/supply/Makefile
>>>>>>>>> b/drivers/power/supply/Makefile
>>>>>>>>> index 4b79d5abc49a..6af905875ad5 100644
>>>>>>>>> --- a/drivers/power/supply/Makefile
>>>>>>>>> +++ b/drivers/power/supply/Makefile
>>>>>>>>> [...]
>>>>>>>>> +
>>>>>>>>> +static irqreturn_t irq_handler(int irq, void *data)
>>>>>>>>> +{
>>>>>>>>> + struct max77759_charger *chg = data;
>>>>>>>>> + struct device *dev = chg->dev;
>>>>>>>>> + u32 chgint_ok;
>>>>>>>>> + int i;
>>>>>>>>> +
>>>>>>>>> + regmap_read(chg->regmap, MAX77759_CHGR_REG_CHG_INT_OK,
>>>>>>>>> &chgint_ok);
>>>>>>>> You might want to check the return value and return IRQ_NONE if it
>>>>>>>> didn't
>>>>>>>> work?
>>>>>>>>
>>>>>>>>> +
>>>>>>>>> + for (i = 0; i < ARRAY_SIZE(irqs); i++) {
>>>>>>>>> + if (irqs[i] == irq)
>>>>>>>>> + break;
>>>>>>>>> + }
>>>>>>>>> +
>>>>>>>>> + switch (i) {
>>>>>>>>> + case AICL:
>>>>>>>>> + dev_dbg(dev, "AICL mode: %s",
>>>>>>>>> + str_no_yes(chgint_ok & MAX77759_CHGR_REG_CHG_INT_AICL));
>>>>>>>>> + break;
>>>>>>>>> + case CHGIN:
>>>>>>>>> + dev_dbg(dev, "CHGIN input valid: %s",
>>>>>>>>> + str_yes_no(chgint_ok & MAX77759_CHGR_REG_CHG_INT_CHGIN));
>>>>>>>>> + break;
>>>>>>>>> + case CHG:
>>>>>>>>> + dev_dbg(dev, "CHG status okay/off: %s",
>>>>>>>>> + str_yes_no(chgint_ok & MAX77759_CHGR_REG_CHG_INT_CHG));
>>>>>>>>> + break;
>>>>>>>>> + case INLIM:
>>>>>>>>> + dev_dbg(dev, "Current Limit reached: %s",
>>>>>>>>> + str_no_yes(chgint_ok & MAX77759_CHGR_REG_CHG_INT_INLIM));
>>>>>>>>> + break;
>>>>>>>>> + case BAT_OILO:
>>>>>>>>> + dev_dbg(dev, "Battery over-current threshold crossed");
>>>>>>>>> + break;
>>>>>>>>> + case CHG_STA_CC:
>>>>>>>>> + dev_dbg(dev, "Charger reached CC stage");
>>>>>>>>> + break;
>>>>>>>>> + case CHG_STA_CV:
>>>>>>>>> + dev_dbg(dev, "Charger reached CV stage");
>>>>>>>>> + break;
>>>>>>>>> + case CHG_STA_TO:
>>>>>>>>> + dev_dbg(dev, "Charger reached TO stage");
>>>>>>>>> + break;
>>>>>>>>> + case CHG_STA_DONE:
>>>>>>>>> + dev_dbg(dev, "Charger reached TO stage");
>>>>>>>>> + break;
>>>>>>>> Are the above debug messages really all needed?
>>>>>> I forgot to respond to this comment in my previous email.
>>>>>>
>>>>>> I think we can keep AICL, BAT_OILO, INLIM. They're either special
>>>>>> conditions (AICL) or faulty conditions (like BAT_OILO) and we can in
>>>>>> fact keep them at dev_info level. Rest can be removed and a
>>>>>> power_supply_changed() is sufficient.
>>>>>>
>>>>>> Let me know what you think?
>>>>> I don't think dev_info() in an interrupt handler is appropriate. At
>>>>> least it should be ratelimited.
>>>>>
>>>>> If it's something special / unexpected that needs attention, having
>>>>> a dev_dbg() message only will usually not be visible to anybody.
>>>> I agree. I can change the prints to dev_info_ratelimited for the stuff
>>>> we care about.
>>> If it's an erroneous condition, maybe warn or even err are more appropriate?
>>>
>>> But then, what is the expectation upon the user observing these messages?
>>> What can or should they do? Who is going to look at these and can do
>>> something sensible based on them?
>> The logging will help in postmortem analysis which may or may not
>> possible with just publishing uevents to userspace hoping that they log
>> the psy properties. Illustrating a situation:
>>
>> 1. Over current situation happened where the Battery to System current
>> exceeds the BAT_OILO threshold. This would also generate an interrupt.
>>
>> 2. The MAX77759 takes protective measures if the condition lasts for a
>> certain specified time and reset. Resetting will cause Vsys to collapse
>> to 0 if the system is only battery powered.
>>
>> 3. It'd be better that the BAT_OILO interrupt is logged in dmesg,
>> instead of just delegating it to user space as user can debug this
>> condition by looking at last_kmsg or pstore.
>>
>> 4. This signal can help the user debug conditions such as moisture (this
>> signal + contaminant detection) or indicative of a mechanical failure.
>>
>> I do agree though that this is a hypothetical or very rare situation and
>> if you have a strong opinion against this I am okay with removing the
>> prints completely.
> Thanks for details. OK, they sound useful in this case, but should still
> be warn, not dbg.
Sounds good, will fix.
>>
>>> Also, I just noticed there is a max77705 charger driver. It seems quite
>>> similar to this one, maybe it can be leveraged / extended?
>> Thanks for the feedback. I reviewed the max77705 charger driver. .
>>
>> Here is a breakdown of why I believe a separate driver may be a better
>> approach:
> [...]
>
> Thanks for the analysis, I agree with your conclusion. Mainly I noticed that
> as part of AICL interrupt handling that driver does a bit of work, while here
> we don't. I am wondering if that is applicable here is well.
I double-checked the downstream drivers and datasheet. There exists no
issue or WAR for max77759. Also, in case of max77759, the current
limiting will be driven by the hardware and there's no need for software
intervention. In case of max77705, the driver is explicitly reducing the
current limit (in max77705_aicl_irq()), implying that hardware is just
notifying the software to limit current due to (say) poor quality power
source.
BR,
Amit
>
> Cheers,
> Andre'
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH v3 5/5] usb: typec: tcpm/tcpci_maxim: deprecate WAR for setting charger mode
2025-12-27 0:04 [PATCH v3 0/5] Introduce MAX77759 charger driver Amit Sunil Dhamne via B4 Relay
` (3 preceding siblings ...)
2025-12-27 0:04 ` [PATCH v3 4/5] power: supply: max77759: add charger driver Amit Sunil Dhamne via B4 Relay
@ 2025-12-27 0:04 ` Amit Sunil Dhamne via B4 Relay
2026-01-05 16:10 ` Heikki Krogerus
` (2 more replies)
4 siblings, 3 replies; 25+ messages in thread
From: Amit Sunil Dhamne via B4 Relay @ 2025-12-27 0:04 UTC (permalink / raw)
To: Sebastian Reichel, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
André Draszik, Lee Jones, Greg Kroah-Hartman,
Badhri Jagan Sridharan, Heikki Krogerus, Peter Griffin,
Tudor Ambarus, Alim Akhtar
Cc: linux-kernel, linux-pm, devicetree, linux-usb, linux-arm-kernel,
linux-samsung-soc, RD Babiera, Kyle Tso, Amit Sunil Dhamne
From: Amit Sunil Dhamne <amitsd@google.com>
TCPCI maxim driver directly writes to the charger's register space to
set charger mode depending on the power role. As MAX77759 chg driver
exists, this WAR is not required.
Instead, use a regulator interface to source vbus when typec is in
source power mode. In other power modes, this regulator will be turned
off if active.
Signed-off-by: Amit Sunil Dhamne <amitsd@google.com>
---
drivers/usb/typec/tcpm/tcpci_maxim.h | 1 +
drivers/usb/typec/tcpm/tcpci_maxim_core.c | 54 +++++++++++++++++++------------
2 files changed, 34 insertions(+), 21 deletions(-)
diff --git a/drivers/usb/typec/tcpm/tcpci_maxim.h b/drivers/usb/typec/tcpm/tcpci_maxim.h
index b33540a42a95..b314606eb0f6 100644
--- a/drivers/usb/typec/tcpm/tcpci_maxim.h
+++ b/drivers/usb/typec/tcpm/tcpci_maxim.h
@@ -60,6 +60,7 @@ struct max_tcpci_chip {
struct tcpm_port *port;
enum contamiant_state contaminant_state;
bool veto_vconn_swap;
+ struct regulator *vbus_reg;
};
static inline int max_tcpci_read16(struct max_tcpci_chip *chip, unsigned int reg, u16 *val)
diff --git a/drivers/usb/typec/tcpm/tcpci_maxim_core.c b/drivers/usb/typec/tcpm/tcpci_maxim_core.c
index 19f638650796..e9e2405c5ca0 100644
--- a/drivers/usb/typec/tcpm/tcpci_maxim_core.c
+++ b/drivers/usb/typec/tcpm/tcpci_maxim_core.c
@@ -10,6 +10,7 @@
#include <linux/kernel.h>
#include <linux/module.h>
#include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
#include <linux/usb/pd.h>
#include <linux/usb/tcpci.h>
#include <linux/usb/tcpm.h>
@@ -35,12 +36,6 @@
*/
#define TCPC_RECEIVE_BUFFER_LEN 32
-#define MAX_BUCK_BOOST_SID 0x69
-#define MAX_BUCK_BOOST_OP 0xb9
-#define MAX_BUCK_BOOST_OFF 0
-#define MAX_BUCK_BOOST_SOURCE 0xa
-#define MAX_BUCK_BOOST_SINK 0x5
-
static const struct regmap_range max_tcpci_tcpci_range[] = {
regmap_reg_range(0x00, 0x95)
};
@@ -202,32 +197,49 @@ static void process_rx(struct max_tcpci_chip *chip, u16 status)
tcpm_pd_receive(chip->port, &msg, rx_type);
}
+static int get_vbus_regulator_handle(struct max_tcpci_chip *chip)
+{
+ if (IS_ERR_OR_NULL(chip->vbus_reg)) {
+ chip->vbus_reg = devm_regulator_get_exclusive(chip->dev,
+ "vbus");
+ if (IS_ERR_OR_NULL(chip->vbus_reg)) {
+ dev_err(chip->dev,
+ "Failed to get vbus regulator handle");
+ return -ENODEV;
+ }
+ }
+
+ return 0;
+}
+
static int max_tcpci_set_vbus(struct tcpci *tcpci, struct tcpci_data *tdata, bool source, bool sink)
{
struct max_tcpci_chip *chip = tdata_to_max_tcpci(tdata);
- u8 buffer_source[2] = {MAX_BUCK_BOOST_OP, MAX_BUCK_BOOST_SOURCE};
- u8 buffer_sink[2] = {MAX_BUCK_BOOST_OP, MAX_BUCK_BOOST_SINK};
- u8 buffer_none[2] = {MAX_BUCK_BOOST_OP, MAX_BUCK_BOOST_OFF};
- struct i2c_client *i2c = chip->client;
int ret;
- struct i2c_msg msgs[] = {
- {
- .addr = MAX_BUCK_BOOST_SID,
- .flags = i2c->flags & I2C_M_TEN,
- .len = 2,
- .buf = source ? buffer_source : sink ? buffer_sink : buffer_none,
- },
- };
-
if (source && sink) {
dev_err(chip->dev, "Both source and sink set\n");
return -EINVAL;
}
- ret = i2c_transfer(i2c->adapter, msgs, 1);
+ ret = get_vbus_regulator_handle(chip);
+ if (ret) {
+ /*
+ * Regulator is not necessary for sink only applications. Return
+ * success in cases where sink mode is being modified.
+ */
+ return source ? ret : 1;
+ }
+
+ if (source) {
+ if (!regulator_is_enabled(chip->vbus_reg))
+ ret = regulator_enable(chip->vbus_reg);
+ } else {
+ if (regulator_is_enabled(chip->vbus_reg))
+ ret = regulator_disable(chip->vbus_reg);
+ }
- return ret < 0 ? ret : 1;
+ return ret < 0 ? ret : 1;
}
static void process_power_status(struct max_tcpci_chip *chip)
--
2.52.0.351.gbe84eed79e-goog
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [PATCH v3 5/5] usb: typec: tcpm/tcpci_maxim: deprecate WAR for setting charger mode
2025-12-27 0:04 ` [PATCH v3 5/5] usb: typec: tcpm/tcpci_maxim: deprecate WAR for setting charger mode Amit Sunil Dhamne via B4 Relay
@ 2026-01-05 16:10 ` Heikki Krogerus
2026-01-05 16:47 ` André Draszik
2026-01-09 13:14 ` Heikki Krogerus
2 siblings, 0 replies; 25+ messages in thread
From: Heikki Krogerus @ 2026-01-05 16:10 UTC (permalink / raw)
To: amitsd
Cc: Sebastian Reichel, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
André Draszik, Lee Jones, Greg Kroah-Hartman,
Badhri Jagan Sridharan, Peter Griffin, Tudor Ambarus, Alim Akhtar,
linux-kernel, linux-pm, devicetree, linux-usb, linux-arm-kernel,
linux-samsung-soc, RD Babiera, Kyle Tso
Sat, Dec 27, 2025 at 12:04:25AM +0000, Amit Sunil Dhamne via B4 Relay kirjoitti:
> From: Amit Sunil Dhamne <amitsd@google.com>
>
> TCPCI maxim driver directly writes to the charger's register space to
> set charger mode depending on the power role. As MAX77759 chg driver
> exists, this WAR is not required.
>
> Instead, use a regulator interface to source vbus when typec is in
> source power mode. In other power modes, this regulator will be turned
> off if active.
>
> Signed-off-by: Amit Sunil Dhamne <amitsd@google.com>
Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> ---
> drivers/usb/typec/tcpm/tcpci_maxim.h | 1 +
> drivers/usb/typec/tcpm/tcpci_maxim_core.c | 54 +++++++++++++++++++------------
> 2 files changed, 34 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/usb/typec/tcpm/tcpci_maxim.h b/drivers/usb/typec/tcpm/tcpci_maxim.h
> index b33540a42a95..b314606eb0f6 100644
> --- a/drivers/usb/typec/tcpm/tcpci_maxim.h
> +++ b/drivers/usb/typec/tcpm/tcpci_maxim.h
> @@ -60,6 +60,7 @@ struct max_tcpci_chip {
> struct tcpm_port *port;
> enum contamiant_state contaminant_state;
> bool veto_vconn_swap;
> + struct regulator *vbus_reg;
> };
>
> static inline int max_tcpci_read16(struct max_tcpci_chip *chip, unsigned int reg, u16 *val)
> diff --git a/drivers/usb/typec/tcpm/tcpci_maxim_core.c b/drivers/usb/typec/tcpm/tcpci_maxim_core.c
> index 19f638650796..e9e2405c5ca0 100644
> --- a/drivers/usb/typec/tcpm/tcpci_maxim_core.c
> +++ b/drivers/usb/typec/tcpm/tcpci_maxim_core.c
> @@ -10,6 +10,7 @@
> #include <linux/kernel.h>
> #include <linux/module.h>
> #include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>
> #include <linux/usb/pd.h>
> #include <linux/usb/tcpci.h>
> #include <linux/usb/tcpm.h>
> @@ -35,12 +36,6 @@
> */
> #define TCPC_RECEIVE_BUFFER_LEN 32
>
> -#define MAX_BUCK_BOOST_SID 0x69
> -#define MAX_BUCK_BOOST_OP 0xb9
> -#define MAX_BUCK_BOOST_OFF 0
> -#define MAX_BUCK_BOOST_SOURCE 0xa
> -#define MAX_BUCK_BOOST_SINK 0x5
> -
> static const struct regmap_range max_tcpci_tcpci_range[] = {
> regmap_reg_range(0x00, 0x95)
> };
> @@ -202,32 +197,49 @@ static void process_rx(struct max_tcpci_chip *chip, u16 status)
> tcpm_pd_receive(chip->port, &msg, rx_type);
> }
>
> +static int get_vbus_regulator_handle(struct max_tcpci_chip *chip)
> +{
> + if (IS_ERR_OR_NULL(chip->vbus_reg)) {
> + chip->vbus_reg = devm_regulator_get_exclusive(chip->dev,
> + "vbus");
> + if (IS_ERR_OR_NULL(chip->vbus_reg)) {
> + dev_err(chip->dev,
> + "Failed to get vbus regulator handle");
> + return -ENODEV;
> + }
> + }
> +
> + return 0;
> +}
> +
> static int max_tcpci_set_vbus(struct tcpci *tcpci, struct tcpci_data *tdata, bool source, bool sink)
> {
> struct max_tcpci_chip *chip = tdata_to_max_tcpci(tdata);
> - u8 buffer_source[2] = {MAX_BUCK_BOOST_OP, MAX_BUCK_BOOST_SOURCE};
> - u8 buffer_sink[2] = {MAX_BUCK_BOOST_OP, MAX_BUCK_BOOST_SINK};
> - u8 buffer_none[2] = {MAX_BUCK_BOOST_OP, MAX_BUCK_BOOST_OFF};
> - struct i2c_client *i2c = chip->client;
> int ret;
>
> - struct i2c_msg msgs[] = {
> - {
> - .addr = MAX_BUCK_BOOST_SID,
> - .flags = i2c->flags & I2C_M_TEN,
> - .len = 2,
> - .buf = source ? buffer_source : sink ? buffer_sink : buffer_none,
> - },
> - };
> -
> if (source && sink) {
> dev_err(chip->dev, "Both source and sink set\n");
> return -EINVAL;
> }
>
> - ret = i2c_transfer(i2c->adapter, msgs, 1);
> + ret = get_vbus_regulator_handle(chip);
> + if (ret) {
> + /*
> + * Regulator is not necessary for sink only applications. Return
> + * success in cases where sink mode is being modified.
> + */
> + return source ? ret : 1;
> + }
> +
> + if (source) {
> + if (!regulator_is_enabled(chip->vbus_reg))
> + ret = regulator_enable(chip->vbus_reg);
> + } else {
> + if (regulator_is_enabled(chip->vbus_reg))
> + ret = regulator_disable(chip->vbus_reg);
> + }
>
> - return ret < 0 ? ret : 1;
> + return ret < 0 ? ret : 1;
> }
>
> static void process_power_status(struct max_tcpci_chip *chip)
>
> --
> 2.52.0.351.gbe84eed79e-goog
>
--
heikki
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH v3 5/5] usb: typec: tcpm/tcpci_maxim: deprecate WAR for setting charger mode
2025-12-27 0:04 ` [PATCH v3 5/5] usb: typec: tcpm/tcpci_maxim: deprecate WAR for setting charger mode Amit Sunil Dhamne via B4 Relay
2026-01-05 16:10 ` Heikki Krogerus
@ 2026-01-05 16:47 ` André Draszik
2026-01-09 13:14 ` Heikki Krogerus
2 siblings, 0 replies; 25+ messages in thread
From: André Draszik @ 2026-01-05 16:47 UTC (permalink / raw)
To: amitsd, Sebastian Reichel, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Lee Jones, Greg Kroah-Hartman,
Badhri Jagan Sridharan, Heikki Krogerus, Peter Griffin,
Tudor Ambarus, Alim Akhtar
Cc: linux-kernel, linux-pm, devicetree, linux-usb, linux-arm-kernel,
linux-samsung-soc, RD Babiera, Kyle Tso
On Sat, 2025-12-27 at 00:04 +0000, Amit Sunil Dhamne via B4 Relay wrote:
> From: Amit Sunil Dhamne <amitsd@google.com>
>
> TCPCI maxim driver directly writes to the charger's register space to
> set charger mode depending on the power role. As MAX77759 chg driver
> exists, this WAR is not required.
>
> Instead, use a regulator interface to source vbus when typec is in
> source power mode. In other power modes, this regulator will be turned
> off if active.
>
> Signed-off-by: Amit Sunil Dhamne <amitsd@google.com>
> ---
> drivers/usb/typec/tcpm/tcpci_maxim.h | 1 +
> drivers/usb/typec/tcpm/tcpci_maxim_core.c | 54 +++++++++++++++++++------------
> 2 files changed, 34 insertions(+), 21 deletions(-)
Reviewed-by: André Draszik <andre.draszik@linaro.org>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v3 5/5] usb: typec: tcpm/tcpci_maxim: deprecate WAR for setting charger mode
2025-12-27 0:04 ` [PATCH v3 5/5] usb: typec: tcpm/tcpci_maxim: deprecate WAR for setting charger mode Amit Sunil Dhamne via B4 Relay
2026-01-05 16:10 ` Heikki Krogerus
2026-01-05 16:47 ` André Draszik
@ 2026-01-09 13:14 ` Heikki Krogerus
2026-01-10 2:16 ` Amit Sunil Dhamne
2 siblings, 1 reply; 25+ messages in thread
From: Heikki Krogerus @ 2026-01-09 13:14 UTC (permalink / raw)
To: amitsd
Cc: Sebastian Reichel, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
André Draszik, Lee Jones, Greg Kroah-Hartman,
Badhri Jagan Sridharan, Peter Griffin, Tudor Ambarus, Alim Akhtar,
linux-kernel, linux-pm, devicetree, linux-usb, linux-arm-kernel,
linux-samsung-soc, RD Babiera, Kyle Tso
Hi,
> + if (source) {
> + if (!regulator_is_enabled(chip->vbus_reg))
> + ret = regulator_enable(chip->vbus_reg);
> + } else {
> + if (regulator_is_enabled(chip->vbus_reg))
> + ret = regulator_disable(chip->vbus_reg);
> + }
It looks like you have to do one more round, so can drop the
regulator_is_enabled() checks and just always enable/disable it
unconditionally.
if (source)
ret = regulator_enable(chip->vbus_reg);
else
ret = regulator_disable(chip->vbus_reg);
I don't think you need the check in any case, but if I've understood
this correctly, you should not use that check when the regulator does
not support that check because then the API claims it's always
enabled. So I guess in that case "if (!regulator_is_enabled())" may
not work as expected, and you may actually be left with a disabled
regulator. This may not be a problem on current platforms, but who
knows what happens in the future.
thanks,
--
heikki
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH v3 5/5] usb: typec: tcpm/tcpci_maxim: deprecate WAR for setting charger mode
2026-01-09 13:14 ` Heikki Krogerus
@ 2026-01-10 2:16 ` Amit Sunil Dhamne
2026-01-12 13:20 ` Heikki Krogerus
0 siblings, 1 reply; 25+ messages in thread
From: Amit Sunil Dhamne @ 2026-01-10 2:16 UTC (permalink / raw)
To: Heikki Krogerus
Cc: Sebastian Reichel, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
André Draszik, Lee Jones, Greg Kroah-Hartman,
Badhri Jagan Sridharan, Peter Griffin, Tudor Ambarus, Alim Akhtar,
linux-kernel, linux-pm, devicetree, linux-usb, linux-arm-kernel,
linux-samsung-soc, RD Babiera, Kyle Tso
Hi Heikki,
Thanks for the review!
On 1/9/26 5:14 AM, Heikki Krogerus wrote:
> Hi,
>
>> + if (source) {
>> + if (!regulator_is_enabled(chip->vbus_reg))
>> + ret = regulator_enable(chip->vbus_reg);
>> + } else {
>> + if (regulator_is_enabled(chip->vbus_reg))
>> + ret = regulator_disable(chip->vbus_reg);
>> + }
> It looks like you have to do one more round, so can drop the
> regulator_is_enabled() checks and just always enable/disable it
> unconditionally.
>
> if (source)
> ret = regulator_enable(chip->vbus_reg);
> else
> ret = regulator_disable(chip->vbus_reg);
The regulator framework uses refcounting on the number of enables. If
the number of times regulator is disabled > enabled, a warning will be
thrown. Also, I don't want to call regulator_enable more than once for
the same refcounting reason (will have to call disable those many number
of times to actually disable).
> I don't think you need the check in any case, but if I've understood
> this correctly, you should not use that check when the regulator does
> not support that check because then the API claims it's always
> enabled. So I guess in that case "if (!regulator_is_enabled())" may
> not work as expected, and you may actually be left with a disabled
> regulator. This may not be a problem on current platforms, but who
> knows what happens in the future.
I don't think this should be an issue in the future as this driver is
specifically meant for max77759_tcpci device and should only be used
with max77759 charger (they both exist only in the same package). And
that the max77759_charger driver does implement the callback. However,
if you think that regulator_is_enabled() is unreliable, I could track
the state within the tcpci driver instead of calling
regulator_is_enabled() and call enable/disable regulator accordingly.
Let me know wdyt and I'll update the next revision accordingly.
BR,
Amit
>
> thanks,
>
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH v3 5/5] usb: typec: tcpm/tcpci_maxim: deprecate WAR for setting charger mode
2026-01-10 2:16 ` Amit Sunil Dhamne
@ 2026-01-12 13:20 ` Heikki Krogerus
2026-01-15 2:58 ` Amit Sunil Dhamne
0 siblings, 1 reply; 25+ messages in thread
From: Heikki Krogerus @ 2026-01-12 13:20 UTC (permalink / raw)
To: Amit Sunil Dhamne
Cc: Sebastian Reichel, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
André Draszik, Lee Jones, Greg Kroah-Hartman,
Badhri Jagan Sridharan, Peter Griffin, Tudor Ambarus, Alim Akhtar,
linux-kernel, linux-pm, devicetree, linux-usb, linux-arm-kernel,
linux-samsung-soc, RD Babiera, Kyle Tso
Fri, Jan 09, 2026 at 06:16:57PM -0800, Amit Sunil Dhamne kirjoitti:
> Hi Heikki,
>
> Thanks for the review!
>
> On 1/9/26 5:14 AM, Heikki Krogerus wrote:
> > Hi,
> >
> >> + if (source) {
> >> + if (!regulator_is_enabled(chip->vbus_reg))
> >> + ret = regulator_enable(chip->vbus_reg);
> >> + } else {
> >> + if (regulator_is_enabled(chip->vbus_reg))
> >> + ret = regulator_disable(chip->vbus_reg);
> >> + }
> > It looks like you have to do one more round, so can drop the
> > regulator_is_enabled() checks and just always enable/disable it
> > unconditionally.
> >
> > if (source)
> > ret = regulator_enable(chip->vbus_reg);
> > else
> > ret = regulator_disable(chip->vbus_reg);
>
> The regulator framework uses refcounting on the number of enables. If
> the number of times regulator is disabled > enabled, a warning will be
> thrown. Also, I don't want to call regulator_enable more than once for
> the same refcounting reason (will have to call disable those many number
> of times to actually disable).
>
> > I don't think you need the check in any case, but if I've understood
> > this correctly, you should not use that check when the regulator does
> > not support that check because then the API claims it's always
> > enabled. So I guess in that case "if (!regulator_is_enabled())" may
> > not work as expected, and you may actually be left with a disabled
> > regulator. This may not be a problem on current platforms, but who
> > knows what happens in the future.
>
> I don't think this should be an issue in the future as this driver is
> specifically meant for max77759_tcpci device and should only be used
> with max77759 charger (they both exist only in the same package). And
> that the max77759_charger driver does implement the callback. However,
> if you think that regulator_is_enabled() is unreliable, I could track
> the state within the tcpci driver instead of calling
> regulator_is_enabled() and call enable/disable regulator accordingly.
>
> Let me know wdyt and I'll update the next revision accordingly.
Let's go with this then as is.
thanks,
--
heikki
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH v3 5/5] usb: typec: tcpm/tcpci_maxim: deprecate WAR for setting charger mode
2026-01-12 13:20 ` Heikki Krogerus
@ 2026-01-15 2:58 ` Amit Sunil Dhamne
0 siblings, 0 replies; 25+ messages in thread
From: Amit Sunil Dhamne @ 2026-01-15 2:58 UTC (permalink / raw)
To: Heikki Krogerus
Cc: Sebastian Reichel, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
André Draszik, Lee Jones, Greg Kroah-Hartman,
Badhri Jagan Sridharan, Peter Griffin, Tudor Ambarus, Alim Akhtar,
linux-kernel, linux-pm, devicetree, linux-usb, linux-arm-kernel,
linux-samsung-soc, RD Babiera, Kyle Tso
On 1/12/26 5:20 AM, Heikki Krogerus wrote:
> Fri, Jan 09, 2026 at 06:16:57PM -0800, Amit Sunil Dhamne kirjoitti:
>> Hi Heikki,
>>
>> Thanks for the review!
>>
>> On 1/9/26 5:14 AM, Heikki Krogerus wrote:
>>> Hi,
>>>
>>>> + if (source) {
>>>> + if (!regulator_is_enabled(chip->vbus_reg))
>>>> + ret = regulator_enable(chip->vbus_reg);
>>>> + } else {
>>>> + if (regulator_is_enabled(chip->vbus_reg))
>>>> + ret = regulator_disable(chip->vbus_reg);
>>>> + }
>>> It looks like you have to do one more round, so can drop the
>>> regulator_is_enabled() checks and just always enable/disable it
>>> unconditionally.
>>>
>>> if (source)
>>> ret = regulator_enable(chip->vbus_reg);
>>> else
>>> ret = regulator_disable(chip->vbus_reg);
>> The regulator framework uses refcounting on the number of enables. If
>> the number of times regulator is disabled > enabled, a warning will be
>> thrown. Also, I don't want to call regulator_enable more than once for
>> the same refcounting reason (will have to call disable those many number
>> of times to actually disable).
>>
>>> I don't think you need the check in any case, but if I've understood
>>> this correctly, you should not use that check when the regulator does
>>> not support that check because then the API claims it's always
>>> enabled. So I guess in that case "if (!regulator_is_enabled())" may
>>> not work as expected, and you may actually be left with a disabled
>>> regulator. This may not be a problem on current platforms, but who
>>> knows what happens in the future.
>> I don't think this should be an issue in the future as this driver is
>> specifically meant for max77759_tcpci device and should only be used
>> with max77759 charger (they both exist only in the same package). And
>> that the max77759_charger driver does implement the callback. However,
>> if you think that regulator_is_enabled() is unreliable, I could track
>> the state within the tcpci driver instead of calling
>> regulator_is_enabled() and call enable/disable regulator accordingly.
>>
>> Let me know wdyt and I'll update the next revision accordingly.
> Let's go with this then as is.
Sounds good. Thanks!
>
> thanks,
>
^ permalink raw reply [flat|nested] 25+ messages in thread