All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/8] power: supply: Add driver for Qualcomm SMBCHG
@ 2022-08-08  7:34 Yassine Oudjana
  2022-08-08  7:34 ` [PATCH 1/8] dt-bindings: power: supply: Add DT schema " Yassine Oudjana
                   ` (7 more replies)
  0 siblings, 8 replies; 23+ messages in thread
From: Yassine Oudjana @ 2022-08-08  7:34 UTC (permalink / raw)
  To: Sebastian Reichel, Rob Herring, Krzysztof Kozlowski, Andy Gross,
	Bjorn Andersson
  Cc: Yassine Oudjana, Yassine Oudjana, Alejandro Tafalla,
	Konrad Dybcio, linux-pm, linux-arm-msm, devicetree, phone-devel,
	linux-kernel

From: Yassine Oudjana <y.oudjana@protonmail.com>

This series adds a driver for the switch-mode battery charger found on PMICs
such as PMI8994, and referred to in the vendor kernel[1] as smbcharger or
SMBCHG. More details on this block can be found in the last patch message.

This driver currently supports the charger blocks of PMI8994 and PMI8996.
PMI8950 was also to be supported, but it was dropped due to some last minute
issues, to be brought back at a later time once ready.

The OTG regulator remains unused on devices where the charger is enabled in
this series due to lack of a consumer. Applying a patch[2] adding vbus-supply
to DWC3 allows it to enable the OTG regulator making USB host without
external power possible.

[1] https://github.com/android-linux-stable/msm-3.18/blob/kernel.lnx.3.18.r34-rel/drivers/power/qpnp-smbcharger.c
[2] https://lore.kernel.org/linux-usb/20200805061744.20404-1-mike.looijmans@topic.nl/

Yassine Oudjana (8):
  dt-bindings: power: supply: Add DT schema for Qualcomm SMBCHG
  arm64: dts: qcom: pmi8994: Add SMBCHG
  arm64: dts: qcom: pmi8996: Add SMBCHG
  arm64: dts: qcom: msm8996-xiaomi-*: Add battery data
  arm64: dts: qcom: msm8996-xiaomi-*: Enable SMBCHG
  soc: qcom: Add PMIC secure register write helpers
  util_macros.h: Add macro to find closest smaller value in array
  power: supply: Add driver for Qualcomm SMBCHG

 .../bindings/power/supply/qcom,smbchg.yaml    |  205 ++
 MAINTAINERS                                   |   16 +
 .../boot/dts/qcom/msm8996-xiaomi-common.dtsi  |   15 +
 .../boot/dts/qcom/msm8996-xiaomi-gemini.dts   |    5 +
 .../boot/dts/qcom/msm8996-xiaomi-natrium.dts  |    5 +
 .../boot/dts/qcom/msm8996-xiaomi-scorpio.dts  |    5 +
 arch/arm64/boot/dts/qcom/pmi8994.dtsi         |   72 +
 arch/arm64/boot/dts/qcom/pmi8996.dtsi         |    4 +
 drivers/power/supply/Kconfig                  |   11 +
 drivers/power/supply/Makefile                 |    1 +
 drivers/power/supply/qcom-smbchg.c            | 1664 +++++++++++++++++
 drivers/power/supply/qcom-smbchg.h            |  428 +++++
 drivers/soc/qcom/Kconfig                      |    4 +
 drivers/soc/qcom/Makefile                     |    1 +
 drivers/soc/qcom/pmic-sec-write.c             |   82 +
 include/linux/util_macros.h                   |   22 +
 include/soc/qcom/pmic-sec-write.h             |    9 +
 17 files changed, 2549 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/power/supply/qcom,smbchg.yaml
 create mode 100644 drivers/power/supply/qcom-smbchg.c
 create mode 100644 drivers/power/supply/qcom-smbchg.h
 create mode 100644 drivers/soc/qcom/pmic-sec-write.c
 create mode 100644 include/soc/qcom/pmic-sec-write.h

-- 
2.37.1


^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH 1/8] dt-bindings: power: supply: Add DT schema for Qualcomm SMBCHG
  2022-08-08  7:34 [PATCH 0/8] power: supply: Add driver for Qualcomm SMBCHG Yassine Oudjana
@ 2022-08-08  7:34 ` Yassine Oudjana
  2022-08-08  8:42   ` Krzysztof Kozlowski
  2022-11-30 16:24   ` Krzysztof Kozlowski
  2022-08-08  7:34 ` [PATCH 2/8] arm64: dts: qcom: pmi8994: Add SMBCHG Yassine Oudjana
                   ` (6 subsequent siblings)
  7 siblings, 2 replies; 23+ messages in thread
From: Yassine Oudjana @ 2022-08-08  7:34 UTC (permalink / raw)
  To: Sebastian Reichel, Rob Herring, Krzysztof Kozlowski, Andy Gross,
	Bjorn Andersson
  Cc: Yassine Oudjana, Yassine Oudjana, Alejandro Tafalla,
	Konrad Dybcio, linux-pm, linux-arm-msm, devicetree, phone-devel,
	linux-kernel

From: Yassine Oudjana <y.oudjana@protonmail.com>

Add DT schema for the switch-mode battery charger found on Qualcomm
PMICs such as PMI8994. Due to lack of documentation, some interrupt
descriptions might be inaccurate.

Signed-off-by: Yassine Oudjana <y.oudjana@protonmail.com>
---
 .../bindings/power/supply/qcom,smbchg.yaml    | 205 ++++++++++++++++++
 MAINTAINERS                                   |   8 +
 2 files changed, 213 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/power/supply/qcom,smbchg.yaml

diff --git a/Documentation/devicetree/bindings/power/supply/qcom,smbchg.yaml b/Documentation/devicetree/bindings/power/supply/qcom,smbchg.yaml
new file mode 100644
index 000000000000..d825a9c10b3e
--- /dev/null
+++ b/Documentation/devicetree/bindings/power/supply/qcom,smbchg.yaml
@@ -0,0 +1,205 @@
+# SPDX-License-Identifier: (GPL-2.0-only or BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/power/supply/qcom,smbchg.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Qualcomm PMIC Switch-Mode Battery Charger
+
+maintainers:
+  - Yassine Oudjana <y.oudjana@protonmail.com>
+  - Alejandro Tafalla <atafalla@dnyon.com>
+
+properties:
+  compatible:
+    enum:
+      - qcom,pmi8994-smbchg
+      - qcom,pmi8996-smbchg
+
+  reg:
+    maxItems: 1
+
+  monitored-battery:
+    description: |
+      phandle of battery characteristics node.
+      The charger uses the following properties:
+      - charge-term-current-microamp
+      - constant-charge-current-max-microamp
+      - voltage-max-design-microvolt
+      The constant-charge-current-max-microamp and voltage-max-design-microvolt
+      properties must be set.
+      See Documentation/devicetree/bindings/power/supply/battery.yaml
+
+  interrupts:
+    items:
+      - description: Charger error
+      - description: Charger inhibited
+      - description: Charger precharge safety timer timeout
+      - description: Charger charge safety timer timeout
+      - description: Charger pre to fast charging switch threshold reached
+      - description: Charger recharge threshold reached
+      - description: Charger taper threshold reached
+      - description: Charger charge termination threshold reached
+      - description: Battery hot
+      - description: Battery warm
+      - description: Battery cold
+      - description: Battery cool
+      - description: Battery overvoltage
+      - description: Battery low
+      - description: Battery missing
+      - description: Battery thermistor missing # unconfirmed
+      - description: USB input undervolt
+      - description: USB input overvolt
+      - description: USB input source detected
+      - description: OTG regulator failure
+      - description: OTG regulator overcurrent
+      - description: Automatic input current limiting done
+      - description: USB ID pin changed
+      - description: DC input undervolt
+      - description: DC input overvolt
+      - description: Power OK
+      - description: Temperature shutdown
+      - description: Watchdog timeout
+      - description: Flash failure
+      - description: OTST2 # unknown
+      - description: OTST3 # unknown
+
+  interrupt-names:
+    items:
+      - const: chg-error
+      - const: chg-inhibit
+      - const: chg-prechg-sft
+      - const: chg-complete-chg-sft
+      - const: chg-p2f-thr
+      - const: chg-rechg-thr
+      - const: chg-taper-thr
+      - const: chg-tcc-thr
+      - const: batt-hot
+      - const: batt-warm
+      - const: batt-cold
+      - const: batt-cool
+      - const: batt-ov
+      - const: batt-low
+      - const: batt-missing
+      - const: batt-term-missing
+      - const: usbin-uv
+      - const: usbin-ov
+      - const: usbin-src-det
+      - const: otg-fail
+      - const: otg-oc
+      - const: aicl-done
+      - const: usbid-change
+      - const: dcin-uv
+      - const: dcin-ov
+      - const: power-ok
+      - const: temp-shutdown
+      - const: wdog-timeout
+      - const: flash-fail
+      - const: otst2
+      - const: otst3
+
+  otg-vbus:
+    type: object
+
+    description:
+      OTG regulator subnode.
+
+required:
+  - compatible
+  - reg
+  - monitored-battery
+  - interrupts
+  - interrupt-names
+  - otg-vbus
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+
+    battery: battery {
+        compatible = "simple-battery";
+
+        charge-full-design-microamp-hours = <4070000>;
+        charge-term-current-microamp = <100000>;
+        voltage-min-design-microvolt = <3400000>;
+        voltage-max-design-microvolt = <4400000>;
+    };
+
+    pmic {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        charger@1000 {
+            compatible = "qcom,pmi8996-smbchg";
+            reg = <0x1000>;
+
+            interrupts = <0x2 0x10 0x0 IRQ_TYPE_EDGE_BOTH>,
+                         <0x2 0x10 0x1 IRQ_TYPE_EDGE_BOTH>,
+                         <0x2 0x10 0x2 IRQ_TYPE_EDGE_BOTH>,
+                         <0x2 0x10 0x3 IRQ_TYPE_EDGE_BOTH>,
+                         <0x2 0x10 0x4 IRQ_TYPE_EDGE_BOTH>,
+                         <0x2 0x10 0x5 IRQ_TYPE_EDGE_BOTH>,
+                         <0x2 0x10 0x6 IRQ_TYPE_EDGE_RISING>,
+                         <0x2 0x10 0x7 IRQ_TYPE_EDGE_RISING>,
+                         <0x2 0x12 0x0 IRQ_TYPE_EDGE_BOTH>,
+                         <0x2 0x12 0x1 IRQ_TYPE_EDGE_BOTH>,
+                         <0x2 0x12 0x2 IRQ_TYPE_EDGE_BOTH>,
+                         <0x2 0x12 0x3 IRQ_TYPE_EDGE_BOTH>,
+                         <0x2 0x12 0x4 IRQ_TYPE_EDGE_BOTH>,
+                         <0x2 0x12 0x5 IRQ_TYPE_EDGE_BOTH>,
+                         <0x2 0x12 0x6 IRQ_TYPE_EDGE_BOTH>,
+                         <0x2 0x12 0x7 IRQ_TYPE_EDGE_BOTH>,
+                         <0x2 0x13 0x0 IRQ_TYPE_EDGE_BOTH>,
+                         <0x2 0x13 0x1 IRQ_TYPE_EDGE_BOTH>,
+                         <0x2 0x13 0x2 IRQ_TYPE_EDGE_BOTH>,
+                         <0x2 0x13 0x3 IRQ_TYPE_EDGE_BOTH>,
+                         <0x2 0x13 0x4 IRQ_TYPE_EDGE_RISING>,
+                         <0x2 0x13 0x5 IRQ_TYPE_EDGE_RISING>,
+                         <0x2 0x13 0x6 IRQ_TYPE_EDGE_FALLING>,
+                         <0x2 0x14 0x0 IRQ_TYPE_EDGE_BOTH>,
+                         <0x2 0x14 0x1 IRQ_TYPE_EDGE_BOTH>,
+                         <0x2 0x16 0x0 IRQ_TYPE_EDGE_BOTH>,
+                         <0x2 0x16 0x1 IRQ_TYPE_EDGE_BOTH>,
+                         <0x2 0x16 0x2 IRQ_TYPE_EDGE_BOTH>,
+                         <0x2 0x16 0x3 IRQ_TYPE_EDGE_BOTH>,
+                         <0x2 0x16 0x4 IRQ_TYPE_EDGE_BOTH>,
+                         <0x2 0x16 0x5 IRQ_TYPE_EDGE_BOTH>;
+            interrupt-names = "chg-error",
+                              "chg-inhibit",
+                              "chg-prechg-sft",
+                              "chg-complete-chg-sft",
+                              "chg-p2f-thr",
+                              "chg-rechg-thr",
+                              "chg-taper-thr",
+                              "chg-tcc-thr",
+                              "batt-hot",
+                              "batt-warm",
+                              "batt-cold",
+                              "batt-cool",
+                              "batt-ov",
+                              "batt-low",
+                              "batt-missing",
+                              "batt-term-missing",
+                              "usbin-uv",
+                              "usbin-ov",
+                              "usbin-src-det",
+                              "otg-fail",
+                              "otg-oc",
+                              "aicl-done",
+                              "usbid-change",
+                              "dcin-uv",
+                              "dcin-ov",
+                              "power-ok",
+                              "temp-shutdown",
+                              "wdog-timeout",
+                              "flash-fail",
+                              "otst2",
+                              "otst3";
+
+            monitored-battery = <&battery>;
+
+            otg-vbus { };
+        };
+    };
diff --git a/MAINTAINERS b/MAINTAINERS
index 3796fd700727..7e9f5893c0eb 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -16951,6 +16951,14 @@ F:	Documentation/networking/device_drivers/cellular/qualcomm/rmnet.rst
 F:	drivers/net/ethernet/qualcomm/rmnet/
 F:	include/linux/if_rmnet.h
 
+QUALCOMM SMBCHG DRIVER
+M:	Yassine Oudjana <y.oudjana@protonmail.com>
+M:	Alejandro Tafalla <atafalla@dnyon.com>
+L:	linux-pm@vger.kernel.org
+L:	linux-arm-msm@vger.kernel.org
+S:	Maintained
+F:	Documentation/devicetree/bindings/power/supply/qcom,smbchg.yaml
+
 QUALCOMM TSENS THERMAL DRIVER
 M:	Amit Kucheria <amitk@kernel.org>
 M:	Thara Gopinath <thara.gopinath@gmail.com>
-- 
2.37.1


^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [PATCH 2/8] arm64: dts: qcom: pmi8994: Add SMBCHG
  2022-08-08  7:34 [PATCH 0/8] power: supply: Add driver for Qualcomm SMBCHG Yassine Oudjana
  2022-08-08  7:34 ` [PATCH 1/8] dt-bindings: power: supply: Add DT schema " Yassine Oudjana
@ 2022-08-08  7:34 ` Yassine Oudjana
  2022-08-08  7:34 ` [PATCH 3/8] arm64: dts: qcom: pmi8996: " Yassine Oudjana
                   ` (5 subsequent siblings)
  7 siblings, 0 replies; 23+ messages in thread
From: Yassine Oudjana @ 2022-08-08  7:34 UTC (permalink / raw)
  To: Sebastian Reichel, Rob Herring, Krzysztof Kozlowski, Andy Gross,
	Bjorn Andersson
  Cc: Yassine Oudjana, Yassine Oudjana, Alejandro Tafalla,
	Konrad Dybcio, linux-pm, linux-arm-msm, devicetree, phone-devel,
	linux-kernel

From: Yassine Oudjana <y.oudjana@protonmail.com>

Add a node for the switch-mode battery charger available on
this PMIC.

Signed-off-by: Yassine Oudjana <y.oudjana@protonmail.com>
---
 arch/arm64/boot/dts/qcom/pmi8994.dtsi | 72 +++++++++++++++++++++++++++
 1 file changed, 72 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/pmi8994.dtsi b/arch/arm64/boot/dts/qcom/pmi8994.dtsi
index 84c44912ec93..3e9db97c9805 100644
--- a/arch/arm64/boot/dts/qcom/pmi8994.dtsi
+++ b/arch/arm64/boot/dts/qcom/pmi8994.dtsi
@@ -10,6 +10,78 @@ pmic@2 {
 		#address-cells = <1>;
 		#size-cells = <0>;
 
+		pmi8994_smbchg: charger@1000 {
+			compatible = "qcom,pmi8994-smbchg";
+			reg = <0x1000>;
+
+			interrupts = <0x2 0x10 0x0 IRQ_TYPE_EDGE_BOTH>,
+				     <0x2 0x10 0x1 IRQ_TYPE_EDGE_BOTH>,
+				     <0x2 0x10 0x2 IRQ_TYPE_EDGE_BOTH>,
+				     <0x2 0x10 0x3 IRQ_TYPE_EDGE_BOTH>,
+				     <0x2 0x10 0x4 IRQ_TYPE_EDGE_BOTH>,
+				     <0x2 0x10 0x5 IRQ_TYPE_EDGE_BOTH>,
+				     <0x2 0x10 0x6 IRQ_TYPE_EDGE_RISING>,
+				     <0x2 0x10 0x7 IRQ_TYPE_EDGE_RISING>,
+				     <0x2 0x12 0x0 IRQ_TYPE_EDGE_BOTH>,
+				     <0x2 0x12 0x1 IRQ_TYPE_EDGE_BOTH>,
+				     <0x2 0x12 0x2 IRQ_TYPE_EDGE_BOTH>,
+				     <0x2 0x12 0x3 IRQ_TYPE_EDGE_BOTH>,
+				     <0x2 0x12 0x4 IRQ_TYPE_EDGE_BOTH>,
+				     <0x2 0x12 0x5 IRQ_TYPE_EDGE_BOTH>,
+				     <0x2 0x12 0x6 IRQ_TYPE_EDGE_BOTH>,
+				     <0x2 0x12 0x7 IRQ_TYPE_EDGE_BOTH>,
+				     <0x2 0x13 0x0 IRQ_TYPE_EDGE_BOTH>,
+				     <0x2 0x13 0x1 IRQ_TYPE_EDGE_BOTH>,
+				     <0x2 0x13 0x2 IRQ_TYPE_EDGE_BOTH>,
+				     <0x2 0x13 0x3 IRQ_TYPE_EDGE_BOTH>,
+				     <0x2 0x13 0x4 IRQ_TYPE_EDGE_RISING>,
+				     <0x2 0x13 0x5 IRQ_TYPE_EDGE_RISING>,
+				     <0x2 0x13 0x6 IRQ_TYPE_EDGE_FALLING>,
+				     <0x2 0x14 0x0 IRQ_TYPE_EDGE_BOTH>,
+				     <0x2 0x14 0x1 IRQ_TYPE_EDGE_BOTH>,
+				     <0x2 0x16 0x0 IRQ_TYPE_EDGE_BOTH>,
+				     <0x2 0x16 0x1 IRQ_TYPE_EDGE_BOTH>,
+				     <0x2 0x16 0x2 IRQ_TYPE_EDGE_BOTH>,
+				     <0x2 0x16 0x3 IRQ_TYPE_EDGE_BOTH>,
+				     <0x2 0x16 0x4 IRQ_TYPE_EDGE_BOTH>,
+				     <0x2 0x16 0x5 IRQ_TYPE_EDGE_BOTH>;
+			interrupt-names = "chg-error",
+					  "chg-inhibit",
+					  "chg-prechg-sft",
+					  "chg-complete-chg-sft",
+					  "chg-p2f-thr",
+					  "chg-rechg-thr",
+					  "chg-taper-thr",
+					  "chg-tcc-thr",
+					  "batt-hot",
+					  "batt-warm",
+					  "batt-cold",
+					  "batt-cool",
+					  "batt-ov",
+					  "batt-low",
+					  "batt-missing",
+					  "batt-term-missing",
+					  "usbin-uv",
+					  "usbin-ov",
+					  "usbin-src-det",
+					  "otg-fail",
+					  "otg-oc",
+					  "aicl-done",
+					  "usbid-change",
+					  "dcin-uv",
+					  "dcin-ov",
+					  "power-ok",
+					  "temp-shutdown",
+					  "wdog-timeout",
+					  "flash-fail",
+					  "otst2",
+					  "otst3";
+
+			status = "disabled";
+
+			chg_otg: otg-vbus { };
+		};
+
 		pmi8994_gpios: gpios@c000 {
 			compatible = "qcom,pmi8994-gpio", "qcom,spmi-gpio";
 			reg = <0xc000>;
-- 
2.37.1


^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [PATCH 3/8] arm64: dts: qcom: pmi8996: Add SMBCHG
  2022-08-08  7:34 [PATCH 0/8] power: supply: Add driver for Qualcomm SMBCHG Yassine Oudjana
  2022-08-08  7:34 ` [PATCH 1/8] dt-bindings: power: supply: Add DT schema " Yassine Oudjana
  2022-08-08  7:34 ` [PATCH 2/8] arm64: dts: qcom: pmi8994: Add SMBCHG Yassine Oudjana
@ 2022-08-08  7:34 ` Yassine Oudjana
  2022-08-08  7:34 ` [PATCH 5/8] arm64: dts: qcom: msm8996-xiaomi-*: Enable SMBCHG Yassine Oudjana
                   ` (4 subsequent siblings)
  7 siblings, 0 replies; 23+ messages in thread
From: Yassine Oudjana @ 2022-08-08  7:34 UTC (permalink / raw)
  To: Sebastian Reichel, Rob Herring, Krzysztof Kozlowski, Andy Gross,
	Bjorn Andersson
  Cc: Yassine Oudjana, Yassine Oudjana, Alejandro Tafalla,
	Konrad Dybcio, linux-pm, linux-arm-msm, devicetree, phone-devel,
	linux-kernel

From: Yassine Oudjana <y.oudjana@protonmail.com>

The SMBCHG block on PMI8996 is slightly different from the one
on PMI8994, making a different compatible string required.
Change the label and compatible of pmi8994_smbchg to match PMI8996.

Signed-off-by: Yassine Oudjana <y.oudjana@protonmail.com>
---
 arch/arm64/boot/dts/qcom/pmi8996.dtsi | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/pmi8996.dtsi b/arch/arm64/boot/dts/qcom/pmi8996.dtsi
index 31b47209e261..ea8d86d12bb8 100644
--- a/arch/arm64/boot/dts/qcom/pmi8996.dtsi
+++ b/arch/arm64/boot/dts/qcom/pmi8996.dtsi
@@ -13,3 +13,7 @@
 / {
 	qcom,pmic-id = <0x20009 0x10013 0 0>;
 };
+
+pmi8996_smbchg: &pmi8994_smbchg {
+	compatible = "qcom,pmi8996-smbchg";
+};
-- 
2.37.1


^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [PATCH 5/8] arm64: dts: qcom: msm8996-xiaomi-*: Enable SMBCHG
  2022-08-08  7:34 [PATCH 0/8] power: supply: Add driver for Qualcomm SMBCHG Yassine Oudjana
                   ` (2 preceding siblings ...)
  2022-08-08  7:34 ` [PATCH 3/8] arm64: dts: qcom: pmi8996: " Yassine Oudjana
@ 2022-08-08  7:34 ` Yassine Oudjana
  2022-08-08  7:34 ` [PATCH 6/8] soc: qcom: Add PMIC secure register write helpers Yassine Oudjana
                   ` (3 subsequent siblings)
  7 siblings, 0 replies; 23+ messages in thread
From: Yassine Oudjana @ 2022-08-08  7:34 UTC (permalink / raw)
  To: Sebastian Reichel, Rob Herring, Krzysztof Kozlowski, Andy Gross,
	Bjorn Andersson
  Cc: Yassine Oudjana, Yassine Oudjana, Alejandro Tafalla,
	Konrad Dybcio, linux-pm, linux-arm-msm, devicetree, phone-devel,
	linux-kernel

From: Yassine Oudjana <y.oudjana@protonmail.com>

Enable pmi8994_smbchg, and set usb-psy-name in dwc3 to point
to its USB charge path supply, allowing it to configure
the input current limit when plugged into a SDP.

Signed-off-by: Yassine Oudjana <y.oudjana@protonmail.com>
---
 arch/arm64/boot/dts/qcom/msm8996-xiaomi-common.dtsi | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/msm8996-xiaomi-common.dtsi b/arch/arm64/boot/dts/qcom/msm8996-xiaomi-common.dtsi
index 022ecbcd965a..a18c9a9baacf 100644
--- a/arch/arm64/boot/dts/qcom/msm8996-xiaomi-common.dtsi
+++ b/arch/arm64/boot/dts/qcom/msm8996-xiaomi-common.dtsi
@@ -304,6 +304,12 @@ &pm8994_resin {
 	linux,code = <KEY_VOLUMEDOWN>;
 };
 
+&pmi8994_smbchg {
+	status = "okay";
+
+	monitored-battery = <&battery>;
+};
+
 &slpi_pil {
 	status = "okay";
 
@@ -324,6 +330,8 @@ &usb3_dwc3 {
 	phys = <&hsusb_phy1>;
 	phy-names = "usb2-phy";
 
+	usb-psy-name = "qcom-smbchg-usb";
+
 	maximum-speed = "high-speed";
 	snps,is-utmi-l1-suspend;
 	snps,usb2-gadget-lpm-disable;
-- 
2.37.1


^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [PATCH 6/8] soc: qcom: Add PMIC secure register write helpers
  2022-08-08  7:34 [PATCH 0/8] power: supply: Add driver for Qualcomm SMBCHG Yassine Oudjana
                   ` (3 preceding siblings ...)
  2022-08-08  7:34 ` [PATCH 5/8] arm64: dts: qcom: msm8996-xiaomi-*: Enable SMBCHG Yassine Oudjana
@ 2022-08-08  7:34 ` Yassine Oudjana
  2022-08-08  7:34 ` [PATCH 7/8] util_macros.h: Add macro to find closest smaller value in array Yassine Oudjana
                   ` (2 subsequent siblings)
  7 siblings, 0 replies; 23+ messages in thread
From: Yassine Oudjana @ 2022-08-08  7:34 UTC (permalink / raw)
  To: Sebastian Reichel, Rob Herring, Krzysztof Kozlowski, Andy Gross,
	Bjorn Andersson
  Cc: Yassine Oudjana, Yassine Oudjana, Alejandro Tafalla,
	Konrad Dybcio, linux-pm, linux-arm-msm, devicetree, phone-devel,
	linux-kernel

From: Yassine Oudjana <y.oudjana@protonmail.com>

Some peripheral blocks in Qualcomm PMICs such as some in the
switch-mode battery charger and fuel gauge of PMI8994 and later
have secure registers that need to be unlocked first before being
written to. Add some helper functions to do what is needed to
write to those registers.

Signed-off-by: Yassine Oudjana <y.oudjana@protonmail.com>
---
 MAINTAINERS                       |  6 +++
 drivers/soc/qcom/Kconfig          |  4 ++
 drivers/soc/qcom/Makefile         |  1 +
 drivers/soc/qcom/pmic-sec-write.c | 82 +++++++++++++++++++++++++++++++
 include/soc/qcom/pmic-sec-write.h |  9 ++++
 5 files changed, 102 insertions(+)
 create mode 100644 drivers/soc/qcom/pmic-sec-write.c
 create mode 100644 include/soc/qcom/pmic-sec-write.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 7e9f5893c0eb..f6cf3a27d132 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -16942,6 +16942,12 @@ S:	Maintained
 F:	Documentation/devicetree/bindings/mtd/qcom,nandc.yaml
 F:	drivers/mtd/nand/raw/qcom_nandc.c
 
+QUALCOMM PMIC SECURE WRITE HELPERS
+M:	Yassine Oudjana <y.oudjana@protonmail.com>
+L:	linux-arm-msm@vger.kernel.org
+S:	Maintained
+F:	drivers/soc/qcom/pmic-sec-write.c
+
 QUALCOMM RMNET DRIVER
 M:	Subash Abhinov Kasiviswanathan <quic_subashab@quicinc.com>
 M:	Sean Tranchetti <quic_stranche@quicinc.com>
diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
index e0d7a5459562..ecc3aeb75ba7 100644
--- a/drivers/soc/qcom/Kconfig
+++ b/drivers/soc/qcom/Kconfig
@@ -91,6 +91,10 @@ config QCOM_PDR_HELPERS
 	tristate
 	select QCOM_QMI_HELPERS
 
+config QCOM_PMIC_SEC_WRITE
+	tristate
+	depends on REGMAP
+
 config QCOM_QMI_HELPERS
 	tristate
 	depends on NET
diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
index d66604aff2b0..058f499820cd 100644
--- a/drivers/soc/qcom/Makefile
+++ b/drivers/soc/qcom/Makefile
@@ -29,3 +29,4 @@ obj-$(CONFIG_QCOM_RPMHPD) += rpmhpd.o
 obj-$(CONFIG_QCOM_RPMPD) += rpmpd.o
 obj-$(CONFIG_QCOM_KRYO_L2_ACCESSORS) +=	kryo-l2-accessors.o
 obj-$(CONFIG_QCOM_ICC_BWMON)	+= icc-bwmon.o
+obj-$(CONFIG_QCOM_PMIC_SEC_WRITE)	+= pmic-sec-write.o
diff --git a/drivers/soc/qcom/pmic-sec-write.c b/drivers/soc/qcom/pmic-sec-write.c
new file mode 100644
index 000000000000..8072c6115f2e
--- /dev/null
+++ b/drivers/soc/qcom/pmic-sec-write.c
@@ -0,0 +1,82 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Helper functions for secure register writes on Qualcomm PMICs
+ */
+
+#include <linux/export.h>
+#include <linux/module.h>
+#include <linux/regmap.h>
+#include <linux/spinlock.h>
+
+#define SEC_ACCESS_ADDR		0x00d0
+#define SEC_ACCESS_VALUE	0x00a5
+#define PERPH_BASE_MASK		0xff00
+
+static DEFINE_SPINLOCK(sec_access_lock);
+
+/**
+ * @brief qcom_pmic_sec_write() - Write to a secure register
+ *
+ * @param regmap Pointer to regmap
+ * @param addr   Address of register to write into
+ * @param val    Buffer to write bytes from
+ * @param len    Length of register in bytes
+ * @return 0 on success, -errno on failure
+ *
+ * @details: Some blocks have registers that need to be unlocked first before
+ * being written to. This function unlocks secure registers in the peripheral
+ * block of a given register then writes a given value to the register.
+ */
+int qcom_pmic_sec_write(struct regmap *regmap, u16 addr, u8 *val, int len)
+{
+	unsigned long flags;
+	unsigned int perph_base;
+	int ret;
+
+	spin_lock_irqsave(&sec_access_lock, flags);
+
+	/* Get peripheral base of given register */
+	perph_base = (addr & PERPH_BASE_MASK);
+
+	ret = regmap_write(regmap, perph_base + SEC_ACCESS_ADDR,
+			   SEC_ACCESS_VALUE);
+	if (ret)
+		goto out;
+
+	ret = regmap_bulk_write(regmap, addr, val, len);
+out:
+	spin_unlock_irqrestore(&sec_access_lock, flags);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(qcom_pmic_sec_write);
+
+/**
+ * @brief qcom_pmic_sec_masked_write() - Masked write to secure registers
+ *
+ * @param regmap Pointer to regmap
+ * @param addr   Address of register to write into
+ * @param mask   Mask to apply on value
+ * @param val    value to be written
+ * @return 0 on success, -errno on failure
+ *
+ * @details: Masked version of smbchg_sec_write().
+ */
+int qcom_pmic_sec_masked_write(struct regmap *regmap, u16 addr, u8 mask, u8 val)
+{
+	int reg;
+	int ret;
+
+	ret = regmap_read(regmap, addr, &reg);
+	if (ret)
+		return ret;
+
+	reg &= ~mask;
+	reg |= val & mask;
+
+	return qcom_pmic_sec_write(regmap, addr, (u8 *)&reg, 1);
+}
+EXPORT_SYMBOL_GPL(qcom_pmic_sec_masked_write);
+
+MODULE_AUTHOR("Yassine Oudjana <y.oudjana@protonmail.com>");
+MODULE_DESCRIPTION("Qualcomm PMIC secure write helpers");
+MODULE_LICENSE("GPL");
diff --git a/include/soc/qcom/pmic-sec-write.h b/include/soc/qcom/pmic-sec-write.h
new file mode 100644
index 000000000000..cc2229620ca4
--- /dev/null
+++ b/include/soc/qcom/pmic-sec-write.h
@@ -0,0 +1,9 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+#ifndef __SOC_ARCH_QCOM_PMIC_SEC_WRITE_H__
+#define __SOC_ARCH_QCOM_PMIC_SEC_WRITE_H__
+
+int qcom_pmic_sec_write(struct regmap *regmap, u16 addr, u8 *val, int len);
+int qcom_pmic_sec_masked_write(struct regmap *regmap, u16 addr, u8 mask, u8 val);
+
+#endif
-- 
2.37.1


^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [PATCH 7/8] util_macros.h: Add macro to find closest smaller value in array
  2022-08-08  7:34 [PATCH 0/8] power: supply: Add driver for Qualcomm SMBCHG Yassine Oudjana
                   ` (4 preceding siblings ...)
  2022-08-08  7:34 ` [PATCH 6/8] soc: qcom: Add PMIC secure register write helpers Yassine Oudjana
@ 2022-08-08  7:34 ` Yassine Oudjana
  2022-08-08  7:34 ` [PATCH 8/8] power: supply: Add driver for Qualcomm SMBCHG Yassine Oudjana
  2022-08-08  8:41 ` [PATCH 0/8] " Krzysztof Kozlowski
  7 siblings, 0 replies; 23+ messages in thread
From: Yassine Oudjana @ 2022-08-08  7:34 UTC (permalink / raw)
  To: Sebastian Reichel, Rob Herring, Krzysztof Kozlowski, Andy Gross,
	Bjorn Andersson
  Cc: Yassine Oudjana, Yassine Oudjana, Alejandro Tafalla,
	Konrad Dybcio, linux-pm, linux-arm-msm, devicetree, phone-devel,
	linux-kernel

From: Yassine Oudjana <y.oudjana@protonmail.com>

Add a macro to find the value closest to but smaller than a given
value in an array.

Signed-off-by: Yassine Oudjana <y.oudjana@protonmail.com>
---
 include/linux/util_macros.h | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/include/linux/util_macros.h b/include/linux/util_macros.h
index 72299f261b25..ad0020e7932b 100644
--- a/include/linux/util_macros.h
+++ b/include/linux/util_macros.h
@@ -38,4 +38,26 @@
  */
 #define find_closest_descending(x, a, as) __find_closest(x, a, as, >=)
 
+/**
+ * find_closest_smaller - locate the closest smaller element in a sorted array
+ * @x: The reference value.
+ * @a: The array in which to look for the closest smaller element. Must be
+ *  sorted in ascending order.
+ * @as: Size of 'a'.
+ *
+ * Returns the index of the element closest to and smaller than 'x', or -1
+ * if no element smaller than 'x' exists in the array.
+ */
+#define find_closest_smaller(x, a, as)					\
+({									\
+	typeof(as) __fcs_i;						\
+	typeof(x) __fcs_x = (x);					\
+	typeof(*a) const *__fcs_a = (a);				\
+	for (__fcs_i = 0; __fcs_i < (as); __fcs_i++) {			\
+		if (__fcs_x < __fcs_a[__fcs_i])				\
+			break;						\
+	}								\
+	(__fcs_i - 1);							\
+})
+
 #endif
-- 
2.37.1


^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [PATCH 8/8] power: supply: Add driver for Qualcomm SMBCHG
  2022-08-08  7:34 [PATCH 0/8] power: supply: Add driver for Qualcomm SMBCHG Yassine Oudjana
                   ` (5 preceding siblings ...)
  2022-08-08  7:34 ` [PATCH 7/8] util_macros.h: Add macro to find closest smaller value in array Yassine Oudjana
@ 2022-08-08  7:34 ` Yassine Oudjana
  2022-08-08  8:55   ` Krzysztof Kozlowski
  2022-08-08  8:41 ` [PATCH 0/8] " Krzysztof Kozlowski
  7 siblings, 1 reply; 23+ messages in thread
From: Yassine Oudjana @ 2022-08-08  7:34 UTC (permalink / raw)
  To: Sebastian Reichel, Rob Herring, Krzysztof Kozlowski, Andy Gross,
	Bjorn Andersson
  Cc: Yassine Oudjana, Yassine Oudjana, Alejandro Tafalla,
	Konrad Dybcio, linux-pm, linux-arm-msm, devicetree, phone-devel,
	linux-kernel

From: Yassine Oudjana <y.oudjana@protonmail.com>

Add a driver for the switch-mode battery charger found on
PMICs such as PMI8994. This block is referred to in the vendor
kernel[1] as smbcharger or SMBCHG. It has USB and DC inputs,
and can generate VBUS for USB OTG with a boost regulator.
It supports Qualcomm Quick Charge 2.0, and can operate along
with a parallel charger (SMB1357, or SMB1351 for added Quick
Charge 3.0 support) for improved efficiency at higher currents.

At the moment, this driver supports charging off of the USB input
at 5V with input current limit up to 3A. It also includes support
for operating the OTG boost regulator as well as extcon
functionality, reporting states of USB and USB_HOST with VBUS and
charge port types.

Co-developed-by: Alejandro Tafalla <atafalla@dnyon.com>
Signed-off-by: Alejandro Tafalla <atafalla@dnyon.com>
Signed-off-by: Yassine Oudjana <y.oudjana@protonmail.com>

[1] https://github.com/android-linux-stable/msm-3.18/blob/kernel.lnx.3.18.r34-rel/drivers/power/qpnp-smbcharger.c
---
 MAINTAINERS                        |    2 +
 drivers/power/supply/Kconfig       |   11 +
 drivers/power/supply/Makefile      |    1 +
 drivers/power/supply/qcom-smbchg.c | 1664 ++++++++++++++++++++++++++++
 drivers/power/supply/qcom-smbchg.h |  428 +++++++
 5 files changed, 2106 insertions(+)
 create mode 100644 drivers/power/supply/qcom-smbchg.c
 create mode 100644 drivers/power/supply/qcom-smbchg.h

diff --git a/MAINTAINERS b/MAINTAINERS
index f6cf3a27d132..9b8693050890 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -16964,6 +16964,8 @@ L:	linux-pm@vger.kernel.org
 L:	linux-arm-msm@vger.kernel.org
 S:	Maintained
 F:	Documentation/devicetree/bindings/power/supply/qcom,smbchg.yaml
+F:	drivers/power/supply/qcom-smbchg.c
+F:	drivers/power/supply/qcom-smbchg.h
 
 QUALCOMM TSENS THERMAL DRIVER
 M:	Amit Kucheria <amitk@kernel.org>
diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig
index 1aa8323ad9f6..246bfc118d9f 100644
--- a/drivers/power/supply/Kconfig
+++ b/drivers/power/supply/Kconfig
@@ -633,6 +633,17 @@ config CHARGER_QCOM_SMBB
 	  documentation for more detail.  The base name for this driver is
 	  'pm8941_charger'.
 
+config CHARGER_QCOM_SMBCHG
+	tristate "Qualcomm Switch-Mode Battery Charger"
+	depends on MFD_SPMI_PMIC || COMPILE_TEST
+	depends on OF
+	depends on EXTCON
+	depends on REGULATOR
+	select QCOM_PMIC_SEC_WRITE
+	help
+	  Say Y to include support for the Switch-Mode Battery Charger block
+	  found in Qualcomm PMICs such as PMI8994.
+
 config CHARGER_BQ2415X
 	tristate "TI BQ2415x battery charger driver"
 	depends on I2C
diff --git a/drivers/power/supply/Makefile b/drivers/power/supply/Makefile
index 7f02f36aea55..7c2c037cd8b1 100644
--- a/drivers/power/supply/Makefile
+++ b/drivers/power/supply/Makefile
@@ -83,6 +83,7 @@ obj-$(CONFIG_CHARGER_MAX8998)	+= max8998_charger.o
 obj-$(CONFIG_CHARGER_MP2629)	+= mp2629_charger.o
 obj-$(CONFIG_CHARGER_MT6360)	+= mt6360_charger.o
 obj-$(CONFIG_CHARGER_QCOM_SMBB)	+= qcom_smbb.o
+obj-$(CONFIG_CHARGER_QCOM_SMBCHG)	+= qcom-smbchg.o
 obj-$(CONFIG_CHARGER_BQ2415X)	+= bq2415x_charger.o
 obj-$(CONFIG_CHARGER_BQ24190)	+= bq24190_charger.o
 obj-$(CONFIG_CHARGER_BQ24257)	+= bq24257_charger.o
diff --git a/drivers/power/supply/qcom-smbchg.c b/drivers/power/supply/qcom-smbchg.c
new file mode 100644
index 000000000000..23a9667953c9
--- /dev/null
+++ b/drivers/power/supply/qcom-smbchg.c
@@ -0,0 +1,1664 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Power supply driver for Qualcomm PMIC switch-mode battery charger
+ *
+ * Copyright (c) 2021 Yassine Oudjana <y.oudjana@protonmail.com>
+ * Copyright (c) 2021 Alejandro Tafalla <atafalla@dnyon.com>
+ */
+
+#include <asm/unaligned.h>
+#include <linux/errno.h>
+#include <linux/extcon-provider.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/of_irq.h>
+#include <linux/interrupt.h>
+#include <linux/platform_device.h>
+#include <linux/power_supply.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
+#include <linux/reboot.h>
+#include <linux/regulator/driver.h>
+#include <linux/util_macros.h>
+#include <soc/qcom/pmic-sec-write.h>
+
+#include "qcom-smbchg.h"
+
+/**
+ * @brief smbchg_usb_is_present() - Check for USB presence
+ *
+ * @param chip Pointer to smbchg_chip.
+ * @return true if USB present, false otherwise
+ */
+static bool smbchg_usb_is_present(struct smbchg_chip *chip)
+{
+	u32 value;
+	int ret;
+
+	ret = regmap_read(chip->regmap, chip->base + SMBCHG_USB_CHGPTH_RT_STS,
+			  &value);
+	if (ret) {
+		dev_err(chip->dev,
+			"Failed to read USB charge path real-time status: %pe\n",
+			ERR_PTR(ret));
+		return false;
+	}
+
+	if (!(value & USBIN_SRC_DET_BIT) || (value & USBIN_OV_BIT))
+		return false;
+
+	ret = regmap_read(chip->regmap,
+			  chip->base + SMBCHG_USB_CHGPTH_INPUT_STS, &value);
+	if (ret) {
+		dev_err(chip->dev,
+			"Failed to read USB charge path input status: %pe\n",
+			ERR_PTR(ret));
+		return false;
+	}
+
+	return !!(value & (USBIN_9V | USBIN_UNREG | USBIN_LV));
+}
+
+/**
+ * @brief smbchg_usb_enable() - Enable/disable USB charge path
+ *
+ * @param chip   Pointer to smbchg_chip
+ * @param enable true to enable, false to disable
+ * @return 0 on success, -errno on failure
+ */
+static int smbchg_usb_enable(struct smbchg_chip *chip, bool enable)
+{
+	int ret;
+
+	dev_dbg(chip->dev, "%sabling USB charge path\n", enable ? "En" : "Dis");
+
+	ret = regmap_update_bits(chip->regmap,
+				 chip->base + SMBCHG_USB_CHGPTH_CMD_IL,
+				 USBIN_SUSPEND_BIT,
+				 enable ? 0 : USBIN_SUSPEND_BIT);
+	if (ret)
+		dev_err(chip->dev, "Failed to %sable USB charge path: %pe\n",
+			enable ? "en" : "dis", ERR_PTR(ret));
+
+	return ret;
+}
+
+/**
+ * @brief smbchg_usb_get_type() - Get USB port type
+ *
+ * @param chip Pointer to smbchg_chip
+ * @return enum power_supply_usb_type, the type of the connected USB port
+ */
+static enum power_supply_usb_type smbchg_usb_get_type(struct smbchg_chip *chip)
+{
+	u32 reg;
+	int ret;
+
+	ret = regmap_read(chip->regmap, chip->base + SMBCHG_MISC_IDEV_STS,
+			  &reg);
+	if (ret) {
+		dev_err(chip->dev, "Failed to read USB type: %pe\n",
+			ERR_PTR(ret));
+		return POWER_SUPPLY_USB_TYPE_UNKNOWN;
+	}
+
+	if (reg & USB_TYPE_SDP_BIT)
+		return POWER_SUPPLY_USB_TYPE_SDP;
+	else if (reg & USB_TYPE_OTHER_BIT || reg & USB_TYPE_DCP_BIT)
+		return POWER_SUPPLY_USB_TYPE_DCP;
+	else if (reg & USB_TYPE_CDP_BIT)
+		return POWER_SUPPLY_USB_TYPE_CDP;
+	else
+		return POWER_SUPPLY_USB_TYPE_UNKNOWN;
+}
+
+/**
+ * @brief smbchg_usb_get_ilim() - Get USB input current limit
+ *
+ * @param chip Pointer to smbchg_chip
+ * @return Current limit in microamperes on success, -errno on failure
+ *
+ * @details: Get currently configured input current limit on the USB
+ * charge path.
+ */
+static int smbchg_usb_get_ilim(struct smbchg_chip *chip)
+{
+	bool usb_3, full_current;
+	int value, ret;
+
+	ret = regmap_read(chip->regmap, chip->base + SMBCHG_USB_CHGPTH_CMD_IL,
+			  &value);
+	if (ret)
+		return ret;
+
+	/* Low current mode */
+	if (!(value & USBIN_MODE_HC_BIT)) {
+		ret = regmap_read(chip->regmap,
+				  chip->base + SMBCHG_USB_CHGPTH_CFG, &value);
+		if (ret)
+			return ret;
+
+		usb_3 = value & CFG_USB3P0_SEL_BIT;
+
+		ret = regmap_read(chip->regmap,
+				  chip->base + SMBCHG_USB_CHGPTH_CMD_IL,
+				  &value);
+		if (ret)
+			return ret;
+
+		full_current = value & USB51_MODE_BIT;
+
+		return smbchg_lc_ilim(usb_3, full_current);
+	}
+
+	/* High current mode */
+	if (value & ICL_OVERRIDE_BIT) {
+		/*
+		 * Read the ilim index set in the USB charge path input
+		 * limiting configuration register and look up the
+		 * corresponding current limit in the ilim table.
+		 */
+		ret = regmap_read(chip->regmap,
+				  chip->base + SMBCHG_USB_CHGPTH_IL_CFG,
+				  &value);
+		if (ret)
+			return ret;
+
+		return chip->data->ilim_table[value & USBIN_INPUT_MASK];
+	}
+
+	/* AICL */
+	ret = regmap_read(chip->regmap,
+			  chip->base + SMBCHG_USB_CHGPTH_ICL_STS_1, &value);
+	if (ret) {
+		dev_err(chip->dev, "Failed to read ICL status: %pe\n",
+			ERR_PTR(ret));
+		return ret;
+	}
+
+	return chip->data->ilim_table[value & ICL_STS_MASK];
+}
+
+/**
+ * @brief smbchg_usb_set_ilim_lc() - Set USB input current limit using
+ * low current mode
+ *
+ * @param chip       Pointer to smbchg_chip
+ * @param current_ua Target current limit in microamperes
+ * @return Actual current limit in microamperes on success, -errno on failure
+ *
+ * @details: Low current mode provides four options for USB input current
+ * limiting:
+ * - 100mA (USB 2.0 limited SDP current)
+ * - 150mA (USB 3.0 limited SDP current)
+ * - 500mA (USB 2.0 full SDP current)
+ * - 900mA (USB 3.0 full SDP current)
+ * This mode is most suitable for use with SDPs.
+ */
+static int smbchg_usb_set_ilim_lc(struct smbchg_chip *chip, int current_ua)
+{
+	bool usb_3;
+	bool full_current;
+	u8 ilim_mask;
+	int ret;
+
+	if (current_ua < smbchg_lc_ilim_options[0])
+		/* Target current limit too small */
+		return -EINVAL;
+
+	ilim_mask = find_closest_smaller(current_ua, smbchg_lc_ilim_options,
+					 ARRAY_SIZE(smbchg_lc_ilim_options));
+
+	usb_3 = ilim_mask & LC_ILIM_USB3_BIT;
+	full_current = ilim_mask & LC_ILIM_FULL_CURRENT_BIT;
+
+	/* Set USB version */
+	ret = qcom_pmic_sec_masked_write(chip->regmap,
+					 chip->base + SMBCHG_USB_CHGPTH_CFG,
+					 CFG_USB3P0_SEL_BIT,
+					 usb_3 ? USB_3P0_SEL : USB_2P0_SEL);
+	if (ret) {
+		dev_err(chip->dev, "Failed to set USB version: %pe\n",
+			ERR_PTR(ret));
+		return ret;
+	}
+
+	/* Set USB current level */
+	ret = regmap_update_bits(chip->regmap,
+				 chip->base + SMBCHG_USB_CHGPTH_CMD_IL,
+				 USB51_MODE_BIT,
+				 full_current ? USB51_MODE_BIT : 0);
+	if (ret) {
+		dev_err(chip->dev, "Failed to set USB current level: %pe\n",
+			ERR_PTR(ret));
+		return ret;
+	}
+
+	/* Disable high current mode */
+	ret = regmap_update_bits(chip->regmap,
+				 chip->base + SMBCHG_USB_CHGPTH_CMD_IL,
+				 USBIN_MODE_HC_BIT, 0);
+	if (ret) {
+		dev_err(chip->dev, "Failed to disable high current mode: %pe\n",
+			ERR_PTR(ret));
+		return ret;
+	}
+
+	dev_dbg(chip->dev,
+		"LC mode current limit set to %duA (%duA requested)\n",
+		smbchg_lc_ilim_options[ilim_mask], current_ua);
+
+	return smbchg_lc_ilim_options[ilim_mask];
+}
+
+/**
+ * @brief smbchg_usb_set_ilim_hc() - Set USB input current limit using
+ * high current mode
+ *
+ * @param chip       Pointer to smbchg_chip
+ * @param current_ua Target current limit in microamperes
+ * @return Actual current limit in microamperes on success, -errno on failure
+ *
+ * @details: High current mode provides a large range of input current limits
+ * with granular control, but does not reach limits as low as the low
+ * current mode does. This mode is most suitable for use with DCPs and CDPs.
+ * This will override and disable AICL.
+ */
+static int smbchg_usb_set_ilim_hc(struct smbchg_chip *chip, int current_ua)
+{
+	size_t ilim_index;
+	int ilim;
+	int ret;
+
+	if (current_ua < chip->data->ilim_table[0])
+		/* Target current limit too small */
+		return -EINVAL;
+
+	/*
+	 * Get index of closest current limit supported by the charger.
+	 * Always round down to avoid exceeding the target limit.
+	 */
+	ilim_index =
+		find_closest_smaller(current_ua, chip->data->ilim_table,
+				     (unsigned int)chip->data->ilim_table_len);
+	ilim = chip->data->ilim_table[ilim_index];
+
+	/* Set the current limit index */
+	ret = qcom_pmic_sec_masked_write(chip->regmap,
+					 chip->base + SMBCHG_USB_CHGPTH_IL_CFG,
+					 USBIN_INPUT_MASK, ilim_index);
+	if (ret) {
+		dev_err(chip->dev, "Failed to set current limit index: %pe\n",
+			ERR_PTR(ret));
+		return ret;
+	}
+
+	/*
+	 * Enable high current mode and override AICL to bring the current
+	 * limit into effect
+	 */
+	ret = regmap_update_bits(chip->regmap,
+				 chip->base + SMBCHG_USB_CHGPTH_CMD_IL,
+				 USBIN_MODE_HC_BIT | ICL_OVERRIDE_BIT,
+				 USBIN_MODE_HC_BIT | ICL_OVERRIDE_BIT);
+	if (ret) {
+		dev_err(chip->dev, "Failed to set high current mode: %pe\n",
+			ERR_PTR(ret));
+		return ret;
+	}
+
+	dev_dbg(chip->dev,
+		"HC mode current limit set to %duA (%duA requested)\n", ilim,
+		current_ua);
+
+	/* Disable AICL as it is no longer used */
+	ret = qcom_pmic_sec_masked_write(
+		chip->regmap, chip->base + SMBCHG_USB_CHGPTH_AICL_CFG,
+		USB_CHGPTH_AICL_EN, 0);
+	if (ret)
+		/*
+		 * Failing to disable AICL is not critical; it will continue
+		 * to run but will not affect the input current limit as it
+		 * has been overridden previously.
+		 */
+		dev_warn(chip->dev, "Failed to disable AICL: %pe\n",
+			 ERR_PTR(ret));
+
+	return ilim;
+}
+
+/**
+ * @brief smbchg_usb_set_ilim() - Set USB input current limit
+ *
+ * @param chip       Pointer to smbchg_chip
+ * @param current_ua Target current limit in microamperes
+ * @return Actual current limit in microamperes on success, -errno on failure
+ *
+ * @details: Find a suitable current limiting mode and use it to set the
+ * current limit.
+ */
+static int smbchg_usb_set_ilim(struct smbchg_chip *chip, int current_ua)
+{
+	size_t i;
+	int ret;
+
+	/*
+	 * Disable USB charge path if the requested current limit is
+	 * lower than the minimum supported limit.
+	 */
+	if (current_ua < smbchg_lc_ilim_options[0])
+		return smbchg_usb_enable(chip, false);
+
+	/*
+	 * Use LC mode if the requested current limit matches one of
+	 * its options. This would likely mean that the current limit
+	 * is meant for a SDP.
+	 */
+	for (i = 0; i < ARRAY_SIZE(smbchg_lc_ilim_options); ++i) {
+		if (current_ua == smbchg_lc_ilim_options[i]) {
+			ret = smbchg_usb_set_ilim_lc(chip, current_ua);
+			goto out;
+		}
+	}
+
+	/*
+	 * Use LC mode if the requested current limit mode is too low
+	 * for HC mode.
+	 */
+	if (current_ua < chip->data->ilim_table[0]) {
+		ret = smbchg_usb_set_ilim_lc(chip, current_ua);
+		goto out;
+	}
+
+	/* Use HC mode otherwise */
+	ret = smbchg_usb_set_ilim_hc(chip, current_ua);
+out:
+	/* Enable USB charge path if a valid current limit has been set */
+	if (ret > 0)
+		smbchg_usb_enable(chip, true);
+
+	return ret;
+}
+
+/**
+ * @brief smbchg_usb_aicl_enable() - Enable AICL on the USB charge path
+ *
+ * @param chip Pointer to smbchg_chip
+ * @return 0 on success, -errno on failure
+ *
+ * @details: Enable Automatic Input Current Limiting (AICL) on the USB
+ * charge path. This will automatically enforce a suitable input current
+ * limit. Set a limit with smbchg_usb_set_ilim_hc to disable.
+ */
+static int smbchg_usb_aicl_enable(struct smbchg_chip *chip)
+{
+	int ret;
+
+	/* Clear AICL override to make its input current limits effective */
+	ret = regmap_update_bits(chip->regmap,
+				 chip->base + SMBCHG_USB_CHGPTH_CMD_IL,
+				 ICL_OVERRIDE_BIT, 0);
+	if (ret) {
+		dev_err(chip->dev, "Failed to clear ICL override: %pe\n",
+			ERR_PTR(ret));
+		return ret;
+	}
+
+	/*
+	 * Enable HC mode to use the limit set by AICL. On at least PMI8950
+	 * this is also needed to for AICL to run at all.
+	 */
+	ret = regmap_update_bits(chip->regmap,
+				 chip->base + SMBCHG_USB_CHGPTH_CMD_IL,
+				 USBIN_MODE_HC_BIT, USBIN_MODE_HC_BIT);
+
+	if (ret) {
+		dev_err(chip->dev, "Failed to set high current mode: %pe\n",
+			ERR_PTR(ret));
+		return IRQ_NONE;
+	}
+
+	/*
+	 * AICL limits itself by the HC mode current limit index set in the
+	 * USB_CHGPTH_IL_CFG register. Set it to the maximum index to allow
+	 * AICL to get the highest possible input current limit.
+	 * Since AICL override was cleared, setting this index will not
+	 * affect the live input current limit.
+	 */
+	ret = qcom_pmic_sec_masked_write(chip->regmap,
+					 chip->base + SMBCHG_USB_CHGPTH_IL_CFG,
+					 USBIN_INPUT_MASK,
+					 chip->data->ilim_table_len - 1);
+	if (ret) {
+		dev_err(chip->dev, "Failed to set current limit index: %pe\n",
+			ERR_PTR(ret));
+		return ret;
+	}
+
+	/* Enable AICL */
+	return qcom_pmic_sec_masked_write(
+		chip->regmap, chip->base + SMBCHG_USB_CHGPTH_AICL_CFG,
+		USB_CHGPTH_AICL_EN, USB_CHGPTH_AICL_EN);
+}
+
+/**
+ * @brief smbchg_charging_enable() - Enable battery charging
+ *
+ * @param chip   Pointer to smbchg_chip
+ * @param enable true to enable, false to disable
+ * @return 0 on success, -errno on failure
+ */
+static int smbchg_charging_enable(struct smbchg_chip *chip, bool enable)
+{
+	int ret;
+
+	ret = regmap_update_bits(chip->regmap,
+				 chip->base + SMBCHG_BAT_IF_CMD_CHG, CHG_EN_BIT,
+				 enable ? 0 : CHG_EN_BIT);
+	if (ret)
+		dev_err(chip->dev, "Failed to enable battery charging: %pe\n",
+			ERR_PTR(ret));
+
+	return ret;
+}
+
+/**
+ * @brief smbchg_charging_set_vfloat() - Set float voltage
+ *
+ * @param chip       Pointer to smbchg_chip
+ * @param voltage_uv Float voltage in microvolts
+ * @return Actual float voltage on success, -errno on failure
+ */
+static int smbchg_charging_set_vfloat(struct smbchg_chip *chip, int voltage_uv)
+{
+	unsigned int fv_index;
+	int fv;
+	int ret;
+
+	if (voltage_uv < smbchg_fv_table[0] ||
+	    voltage_uv > smbchg_fv_table[ARRAY_SIZE(smbchg_fv_table) - 1])
+		/* Float voltage not supported */
+		return -EINVAL;
+
+	/*
+	 * Float voltage is set by writing the index of the desired voltage
+	 * in the float voltage table to the FV_CFG register. Indices start
+	 * at 5.
+	 */
+	fv_index = 5 + find_closest_smaller(voltage_uv, smbchg_fv_table,
+					    ARRAY_SIZE(smbchg_fv_table));
+
+	ret = qcom_pmic_sec_masked_write(chip->regmap,
+					 chip->base + SMBCHG_CHGR_FV_CFG,
+					 FV_MASK, fv_index);
+	if (ret) {
+		dev_err(chip->dev, "Failed to set float voltage: %pe\n",
+			ERR_PTR(ret));
+		return ret;
+	}
+
+	fv = smbchg_fv_table[fv_index - 5];
+
+	dev_dbg(chip->dev, "Float voltage set to %duV (%duV requested)", fv,
+		voltage_uv);
+
+	return fv;
+}
+
+/**
+ * @brief smbchg_charging_get_iterm() - Get charge termination current
+ *
+ * @param chip Pointer to smbchg_chip
+ * @return Charge termination current in microamperes on success,
+ * -errno on failure
+ */
+static int smbchg_charging_get_iterm(struct smbchg_chip *chip)
+{
+	unsigned int iterm_index;
+	int ret;
+
+	ret = regmap_read(chip->regmap, chip->base + SMBCHG_CHGR_TCC_CFG,
+			  &iterm_index);
+	if (ret)
+		return ret;
+
+	iterm_index &= CHG_ITERM_MASK;
+
+	return chip->data->iterm_table[iterm_index];
+}
+
+/**
+ * @brief smbchg_charging_set_iterm() - Set charge termination current
+ *
+ * @param chip       Pointer to smbchg_chip
+ * @param current_ua Termination current in microamperes
+ * @return Actual charge termination current in microamperes on success,
+ * -errno on failure
+ */
+static int smbchg_charging_set_iterm(struct smbchg_chip *chip, int current_ua)
+{
+	size_t iterm_count = chip->data->iterm_table_len;
+	unsigned int iterm_index;
+	int iterm;
+	int ret;
+
+	iterm_index =
+		find_closest(current_ua, chip->data->iterm_table, iterm_count);
+
+	ret = qcom_pmic_sec_masked_write(chip->regmap,
+					 chip->base + SMBCHG_CHGR_TCC_CFG,
+					 CHG_ITERM_MASK, iterm_index);
+	if (ret)
+		return ret;
+
+	iterm = chip->data->iterm_table[iterm_index];
+
+	dev_dbg(chip->dev,
+		"Termination current limit set to %duA (%duA requested)", iterm,
+		current_ua);
+
+	return iterm;
+}
+
+/**
+ * @brief smbchg_charging_get_ilim() - Get constant charge current limit
+ *
+ * @param chip       Pointer to smbchg_chip
+ * @return Constant charge current limit in microamperes on success, -errno
+ * on failure
+ */
+static int smbchg_charging_get_ilim(struct smbchg_chip *chip)
+{
+	u32 value;
+	int ret;
+
+	/*
+	 * Read the ilim index set in the FCC configuration register and
+	 * look up the corresponding current limit in the ilim table.
+	 */
+	ret = regmap_read(chip->regmap, chip->base + SMBCHG_CHGR_FCC_CFG,
+			  &value);
+	if (ret)
+		return ret;
+
+	return chip->data->ilim_table[value & FCC_MASK];
+}
+
+/**
+ * @brief smbchg_charging_set_ilim() - Set constant charge current limit
+ *
+ * @param chip       Pointer to smbchg_chip
+ * @return Constant charge current limit in microamperes on success, -errno
+ * on failure
+ */
+static int smbchg_charging_set_ilim(struct smbchg_chip *chip, int current_ua)
+{
+	size_t ilim_index;
+	int ilim;
+	int ret;
+
+	if (current_ua < chip->data->ilim_table[0])
+		/* Target current limit too small */
+		return -EINVAL;
+
+	/*
+	 * Get index of closest current limit supported by the charger.
+	 * Always round down to avoid exceeding the target limit.
+	 */
+	ilim_index =
+		find_closest_smaller(current_ua, chip->data->ilim_table,
+				     (unsigned int)chip->data->ilim_table_len);
+	ilim = chip->data->ilim_table[ilim_index];
+
+	/* Set the current limit index */
+	ret = qcom_pmic_sec_masked_write(chip->regmap,
+					 chip->base + SMBCHG_CHGR_FCC_CFG,
+					 FCC_MASK, ilim_index);
+	if (ret) {
+		dev_err(chip->dev,
+			"Failed to set constant charge current limit: %pe\n",
+			ERR_PTR(ret));
+		return ret;
+	}
+
+	dev_dbg(chip->dev,
+		"Constant charge current limit set to %duA (%duA requested)\n",
+		ilim, current_ua);
+
+	return ilim;
+}
+
+/**
+ * @brief smbchg_batt_is_present() - Check for battery presence
+ *
+ * @param chip Pointer to smbchg_chip
+ * @return true if battery present, false otherwise
+ */
+static bool smbchg_batt_is_present(struct smbchg_chip *chip)
+{
+	unsigned int value;
+	int ret;
+
+	ret = regmap_read(chip->regmap, chip->base + SMBCHG_BAT_IF_RT_STS,
+			  &value);
+	if (ret) {
+		dev_err(chip->dev,
+			"Failed to read battery real-time status: %pe\n",
+			ERR_PTR(ret));
+		return false;
+	}
+
+	return !(value & BAT_MISSING_BIT);
+}
+
+/**
+ * @brief smbchg_otg_is_present() - Check for OTG presence
+ *
+ * @param chip Pointer to smbchg_chip
+ * @return true if OTG present, false otherwise
+ */
+static bool smbchg_otg_is_present(struct smbchg_chip *chip)
+{
+	u32 value;
+	u16 usb_id;
+	int ret;
+
+	/* Check ID pin */
+	ret = regmap_bulk_read(chip->regmap,
+			       chip->base + SMBCHG_USB_CHGPTH_USBID_MSB, &value,
+			       2);
+	if (ret) {
+		dev_err(chip->dev, "Failed to read ID pin: %pe\n",
+			ERR_PTR(ret));
+		return false;
+	}
+
+	put_unaligned_be16(value, &usb_id);
+	dev_vdbg(chip->dev, "0x%04x read on ID pin\n", usb_id);
+	if (usb_id > USBID_GND_THRESHOLD)
+		return false;
+
+	ret = regmap_read(chip->regmap, chip->base + SMBCHG_USB_CHGPTH_RID_STS,
+			  &value);
+	if (ret) {
+		dev_err(chip->dev, "Failed to read ID resistance status: %pe\n",
+			ERR_PTR(ret));
+		return false;
+	}
+
+	return (value & RID_MASK) == 0;
+}
+
+/**
+ * @brief smbchg_otg_enable() - Enable OTG regulator
+ *
+ * @param rdev Pointer to regulator_dev
+ * @return 0 on success, -errno on failure
+ */
+static int smbchg_otg_enable(struct regulator_dev *rdev)
+{
+	struct smbchg_chip *chip = rdev_get_drvdata(rdev);
+	int ret;
+
+	dev_dbg(chip->dev, "Enabling OTG VBUS regulator");
+
+	ret = regmap_update_bits(chip->regmap,
+				 chip->base + SMBCHG_BAT_IF_CMD_CHG, OTG_EN_BIT,
+				 OTG_EN_BIT);
+	if (ret)
+		dev_err(chip->dev, "Failed to enable OTG regulator: %pe\n",
+			ERR_PTR(ret));
+
+	return ret;
+}
+
+/**
+ * @brief smbchg_otg_disable() - Disable OTG regulator
+ *
+ * @param rdev Pointer to regulator_dev
+ * @return 0 on success, -errno on failure
+ */
+static int smbchg_otg_disable(struct regulator_dev *rdev)
+{
+	struct smbchg_chip *chip = rdev_get_drvdata(rdev);
+	int ret;
+
+	dev_dbg(chip->dev, "Disabling OTG VBUS regulator");
+
+	ret = regmap_update_bits(chip->regmap,
+				 chip->base + SMBCHG_BAT_IF_CMD_CHG, OTG_EN_BIT,
+				 0);
+	if (ret) {
+		dev_err(chip->dev, "Failed to disable OTG regulator: %pe\n",
+			ERR_PTR(ret));
+		return ret;
+	}
+
+	return 0;
+}
+
+/**
+ * @brief smbchg_otg_is_enabled() - Check if OTG regulator is enabled
+ *
+ * @param rdev Pointer to regulator_dev
+ * @return int 1 if enabled, 0 if disabled
+ */
+static int smbchg_otg_is_enabled(struct regulator_dev *rdev)
+{
+	struct smbchg_chip *chip = rdev_get_drvdata(rdev);
+	u32 value = 0;
+	int ret;
+
+	ret = regmap_read(chip->regmap, chip->base + SMBCHG_BAT_IF_CMD_CHG,
+			  &value);
+	if (ret)
+		dev_err(chip->dev, "Failed to read OTG regulator status\n");
+
+	return !!(value & OTG_EN_BIT);
+}
+
+static const struct regulator_ops smbchg_otg_ops = {
+	.enable = smbchg_otg_enable,
+	.disable = smbchg_otg_disable,
+	.is_enabled = smbchg_otg_is_enabled,
+};
+
+static void smbchg_otg_reset_worker(struct work_struct *work)
+{
+	struct smbchg_chip *chip =
+		container_of(work, struct smbchg_chip, otg_reset_work);
+	int ret;
+
+	dev_dbg(chip->dev, "Resetting OTG VBUS regulator\n");
+
+	ret = regmap_update_bits(chip->regmap,
+				 chip->base + SMBCHG_BAT_IF_CMD_CHG, OTG_EN_BIT,
+				 0);
+	if (ret) {
+		dev_err(chip->dev,
+			"Failed to disable OTG regulator for reset: %pe\n",
+			ERR_PTR(ret));
+		return;
+	}
+
+	msleep(OTG_RESET_DELAY_MS);
+
+	/*
+	 * Only re-enable the OTG regulator if OTG is still present
+	 * after sleeping
+	 */
+	if (!smbchg_otg_is_present(chip))
+		return;
+
+	ret = regmap_update_bits(chip->regmap,
+				 chip->base + SMBCHG_BAT_IF_CMD_CHG, OTG_EN_BIT,
+				 OTG_EN_BIT);
+	if (ret)
+		dev_err(chip->dev,
+			"Failed to re-enable OTG regulator after reset: %pe\n",
+			ERR_PTR(ret));
+}
+
+/**
+ * @brief smbchg_extcon_update() - Update extcon properties and sync cables
+ *
+ * @param chip Pointer to smbchg_chip
+ */
+static void smbchg_extcon_update(struct smbchg_chip *chip)
+{
+	enum power_supply_usb_type usb_type = smbchg_usb_get_type(chip);
+	bool usb_present = smbchg_usb_is_present(chip);
+	bool otg_present = smbchg_otg_is_present(chip);
+	int otg_vbus_present = smbchg_otg_is_enabled(chip->otg_reg);
+
+	extcon_set_state(chip->edev, EXTCON_USB, usb_present);
+	extcon_set_state(chip->edev, EXTCON_USB_HOST, otg_present);
+	extcon_set_property(chip->edev, EXTCON_USB_HOST, EXTCON_PROP_USB_VBUS,
+			    (union extcon_property_value)otg_vbus_present);
+
+	if (usb_present) {
+		extcon_set_state(chip->edev, EXTCON_CHG_USB_SDP,
+				 usb_type == POWER_SUPPLY_USB_TYPE_SDP);
+		extcon_set_state(chip->edev, EXTCON_CHG_USB_DCP,
+				 usb_type == POWER_SUPPLY_USB_TYPE_DCP);
+		extcon_set_state(chip->edev, EXTCON_CHG_USB_CDP,
+				 usb_type == POWER_SUPPLY_USB_TYPE_CDP);
+		extcon_set_property(
+			chip->edev, EXTCON_USB, EXTCON_PROP_USB_VBUS,
+			(union extcon_property_value)(
+				usb_type != POWER_SUPPLY_USB_TYPE_UNKNOWN));
+	} else {
+		/*
+		 * Charging extcon cables and VBUS are unavailable when
+		 * USB is not present.
+		 */
+		extcon_set_state(chip->edev, EXTCON_CHG_USB_SDP, false);
+		extcon_set_state(chip->edev, EXTCON_CHG_USB_DCP, false);
+		extcon_set_state(chip->edev, EXTCON_CHG_USB_CDP, false);
+		extcon_set_property(chip->edev, EXTCON_USB,
+				    EXTCON_PROP_USB_VBUS,
+				    (union extcon_property_value) false);
+	}
+
+	/* Sync all extcon cables */
+	extcon_sync(chip->edev, EXTCON_USB);
+	extcon_sync(chip->edev, EXTCON_USB_HOST);
+	extcon_sync(chip->edev, EXTCON_CHG_USB_SDP);
+	extcon_sync(chip->edev, EXTCON_CHG_USB_DCP);
+	extcon_sync(chip->edev, EXTCON_CHG_USB_CDP);
+}
+
+static const unsigned int smbchg_extcon_cable[] = {
+	EXTCON_USB,	    EXTCON_USB_HOST,	EXTCON_CHG_USB_SDP,
+	EXTCON_CHG_USB_DCP, EXTCON_CHG_USB_CDP, EXTCON_NONE,
+};
+
+static irqreturn_t smbchg_handle_charger_error(int irq, void *data)
+{
+	struct smbchg_chip *chip = data;
+
+	dev_err(chip->dev, "Charger error");
+
+	power_supply_changed(chip->usb_psy);
+
+	return IRQ_HANDLED;
+}
+
+static irqreturn_t smbchg_handle_p2f(int irq, void *data)
+{
+	struct smbchg_chip *chip = data;
+
+	dev_dbg(chip->dev, "Fast charging threshold reached");
+
+	power_supply_changed(chip->usb_psy);
+
+	return IRQ_HANDLED;
+}
+
+static irqreturn_t smbchg_handle_rechg(int irq, void *data)
+{
+	struct smbchg_chip *chip = data;
+
+	dev_dbg(chip->dev, "Recharge threshold reached");
+
+	/* Auto-recharge is enabled, nothing to do here */
+	return IRQ_HANDLED;
+}
+
+static irqreturn_t smbchg_handle_taper(int irq, void *data)
+{
+	struct smbchg_chip *chip = data;
+
+	dev_dbg(chip->dev, "Taper charging threshold reached");
+
+	power_supply_changed(chip->usb_psy);
+
+	return IRQ_HANDLED;
+}
+
+static irqreturn_t smbchg_handle_tcc(int irq, void *data)
+{
+	struct smbchg_chip *chip = data;
+
+	dev_dbg(chip->dev, "Termination current reached");
+
+	power_supply_changed(chip->usb_psy);
+
+	return IRQ_HANDLED;
+}
+
+static irqreturn_t smbchg_handle_batt_temp(int irq, void *data)
+{
+	struct smbchg_chip *chip = data;
+
+	power_supply_changed(chip->usb_psy);
+
+	return IRQ_HANDLED;
+}
+
+static irqreturn_t smbchg_handle_batt_presence(int irq, void *data)
+{
+	struct smbchg_chip *chip = data;
+	bool batt_present = smbchg_batt_is_present(chip);
+
+	dev_dbg(chip->dev, "Battery %spresent\n", batt_present ? "" : "not ");
+
+	power_supply_changed(chip->usb_psy);
+
+	return IRQ_HANDLED;
+}
+
+static irqreturn_t smbchg_handle_usb_source_detect(int irq, void *data)
+{
+	struct smbchg_chip *chip = data;
+	bool usb_present;
+	enum power_supply_usb_type usb_type;
+	int ret;
+
+	usb_present = smbchg_usb_is_present(chip);
+	dev_dbg(chip->dev, "USB %spresent\n", usb_present ? "" : "not ");
+
+	usb_type = smbchg_usb_get_type(chip);
+
+	/*
+	 * Prepare for running AICL to find a suitable input current
+	 * limit if connected to a DCP or a CDP
+	 */
+	if (usb_present && (usb_type == POWER_SUPPLY_USB_TYPE_DCP ||
+			    usb_type == POWER_SUPPLY_USB_TYPE_CDP)) {
+		/*
+		 * Enable AICL to find a suitable current limit for the
+		 * newly plugged into port.
+		 */
+		ret = smbchg_usb_aicl_enable(chip);
+		if (ret) {
+			dev_err(chip->dev, "Failed to enable AICL: %pe\n",
+				ERR_PTR(ret));
+			return IRQ_NONE;
+		}
+
+		/* Enable USB charge path to make AICL run and start charging */
+		ret = smbchg_usb_enable(chip, true);
+		if (ret) {
+			dev_err(chip->dev,
+				"Failed to enable USB charge path: %pe\n",
+				ERR_PTR(ret));
+			return IRQ_NONE;
+		}
+	}
+
+	smbchg_extcon_update(chip);
+	power_supply_changed(chip->usb_psy);
+
+	return IRQ_HANDLED;
+}
+
+static irqreturn_t smbchg_handle_usbid_change(int irq, void *data)
+{
+	struct smbchg_chip *chip = data;
+	bool otg_present;
+
+	/*
+	 * ADC conversion for USB ID resistance in the fuel gauge can take
+	 * up to 15ms to finish after the USB ID change interrupt is fired.
+	 * Wait for it to finish before detecting OTG presence. Add an extra
+	 * 5ms for good measure.
+	 */
+	msleep(20);
+
+	otg_present = smbchg_otg_is_present(chip);
+	dev_dbg(chip->dev, "OTG %spresent\n", otg_present ? "" : "not ");
+
+	smbchg_extcon_update(chip);
+
+	return IRQ_HANDLED;
+}
+
+static irqreturn_t smbchg_handle_otg_fail(int irq, void *data)
+{
+	struct smbchg_chip *chip = data;
+
+	dev_err(chip->dev, "OTG regulator failure");
+
+	/* Report failure */
+	regulator_notifier_call_chain(chip->otg_reg, REGULATOR_EVENT_FAIL,
+				      NULL);
+
+	return IRQ_HANDLED;
+}
+
+static irqreturn_t smbchg_handle_otg_oc(int irq, void *data)
+{
+	struct smbchg_chip *chip = data;
+
+	/*
+	 * Inrush current of some devices can trip the OTG over-current
+	 * protection on the PMI8994 and PMI8996 chargers due to
+	 * a hardware bug.
+	 * Try resetting the OTG regulator a few times, and only report
+	 * over-current if it persists.
+	 */
+	if (chip->data->reset_otg_on_oc) {
+		if (chip->otg_resets < NUM_OTG_RESET_RETRIES) {
+			schedule_work(&chip->otg_reset_work);
+			chip->otg_resets++;
+			return IRQ_HANDLED;
+		}
+
+		chip->otg_resets = 0;
+	}
+
+	dev_warn(chip->dev, "OTG over-current");
+
+	/* Report over-current */
+	regulator_notifier_call_chain(chip->otg_reg,
+				      REGULATOR_EVENT_OVER_CURRENT, NULL);
+
+	/* Regulator is automatically disabled in hardware on over-current */
+	regulator_notifier_call_chain(chip->otg_reg, REGULATOR_EVENT_DISABLE,
+				      NULL);
+
+	return IRQ_HANDLED;
+}
+
+static irqreturn_t smbchg_handle_aicl_done(int irq, void *data)
+{
+	struct smbchg_chip *chip = data;
+	int ilim;
+	int ret;
+
+	dev_dbg(chip->dev, "AICL done");
+
+	ilim = smbchg_usb_get_ilim(chip);
+	if (ilim < 0)
+		dev_warn(chip->dev, "Failed to read AICL result: %pe\n",
+			 ERR_PTR(ret));
+	else
+		dev_dbg(chip->dev, "AICL result: %uuA", ilim);
+
+	power_supply_changed(chip->usb_psy);
+
+	return IRQ_HANDLED;
+}
+
+static irqreturn_t smbchg_handle_temp_shutdown(int irq, void *data)
+{
+	struct smbchg_chip *chip = data;
+
+	hw_protection_shutdown("Charger thermal emergency", 100);
+
+	smbchg_charging_enable(chip, false);
+	smbchg_usb_enable(chip, false);
+
+	power_supply_changed(chip->usb_psy);
+
+	return IRQ_HANDLED;
+}
+
+const struct smbchg_irq smbchg_irqs[] = {
+	{ "chg-error", smbchg_handle_charger_error },
+	{ "chg-inhibit", NULL },
+	{ "chg-prechg-sft", NULL },
+	{ "chg-complete-chg-sft", NULL },
+	{ "chg-p2f-thr", smbchg_handle_p2f },
+	{ "chg-rechg-thr", smbchg_handle_rechg },
+	{ "chg-taper-thr", smbchg_handle_taper },
+	{ "chg-tcc-thr", smbchg_handle_tcc },
+	{ "batt-hot", smbchg_handle_batt_temp },
+	{ "batt-warm", smbchg_handle_batt_temp },
+	{ "batt-cold", smbchg_handle_batt_temp },
+	{ "batt-cool", smbchg_handle_batt_temp },
+	{ "batt-ov", NULL },
+	{ "batt-low", NULL },
+	{ "batt-missing", smbchg_handle_batt_presence },
+	{ "batt-term-missing", NULL },
+	{ "usbin-uv", NULL },
+	{ "usbin-ov", NULL },
+	{ "usbin-src-det", smbchg_handle_usb_source_detect },
+	{ "usbid-change", smbchg_handle_usbid_change },
+	{ "otg-fail", smbchg_handle_otg_fail },
+	{ "otg-oc", smbchg_handle_otg_oc },
+	{ "aicl-done", smbchg_handle_aicl_done },
+	{ "dcin-uv", NULL },
+	{ "dcin-ov", NULL },
+	{ "power-ok", NULL },
+	{ "temp-shutdown", smbchg_handle_temp_shutdown },
+	{ "wdog-timeout", NULL },
+	{ "flash-fail", NULL },
+	{ "otst2", NULL },
+	{ "otst3", NULL },
+};
+
+/**
+ * @brief smbchg_get_charge_type() - Get charge type
+ *
+ * @param chip Pointer to smbchg_chip
+ * @return Charge type, as defined in <linux/power_supply.h>
+ */
+static int smbchg_get_charge_type(struct smbchg_chip *chip)
+{
+	int value, ret;
+
+	ret = regmap_read(chip->regmap, chip->base + SMBCHG_CHGR_STS, &value);
+	if (ret) {
+		dev_err(chip->dev, "Failed to read charger status: %pe\n",
+			ERR_PTR(ret));
+		return POWER_SUPPLY_CHARGE_TYPE_UNKNOWN;
+	}
+
+	value = (value & CHG_TYPE_MASK) >> CHG_TYPE_SHIFT;
+	dev_vdbg(chip->dev, "Charge type: 0x%x", value);
+	switch (value) {
+	case BATT_NOT_CHG_VAL:
+		return POWER_SUPPLY_CHARGE_TYPE_NONE;
+	case BATT_PRE_CHG_VAL:
+		/* Low current precharging */
+		return POWER_SUPPLY_CHARGE_TYPE_TRICKLE;
+	case BATT_FAST_CHG_VAL:
+		/* Constant current fast charging */
+	case BATT_TAPER_CHG_VAL:
+		/* Constant voltage fast charging */
+		return POWER_SUPPLY_CHARGE_TYPE_FAST;
+	default:
+		dev_err(chip->dev, "Invalid charge type value 0x%x read\n",
+			value);
+		return POWER_SUPPLY_CHARGE_TYPE_UNKNOWN;
+	}
+}
+
+/**
+ * @brief smbchg_get_health() - Get battery health
+ *
+ * @param chip Pointer to smbchg_chip
+ * @return Battery health, as defined in <linux/power_supply.h>
+ */
+static int smbchg_get_health(struct smbchg_chip *chip)
+{
+	int value, ret;
+	bool batt_present = smbchg_batt_is_present(chip);
+
+	if (!batt_present)
+		return POWER_SUPPLY_HEALTH_NO_BATTERY;
+
+	ret = regmap_read(chip->regmap, chip->base + SMBCHG_BAT_IF_RT_STS,
+			  &value);
+	if (ret) {
+		dev_err(chip->dev,
+			"Failed to read battery real-time status: %pe\n",
+			ERR_PTR(ret));
+		return POWER_SUPPLY_HEALTH_UNKNOWN;
+	}
+
+	if (value & HOT_BAT_HARD_BIT)
+		return POWER_SUPPLY_HEALTH_OVERHEAT;
+	else if (value & HOT_BAT_SOFT_BIT)
+		return POWER_SUPPLY_HEALTH_WARM;
+	else if (value & COLD_BAT_HARD_BIT)
+		return POWER_SUPPLY_HEALTH_COLD;
+	else if (value & COLD_BAT_SOFT_BIT)
+		return POWER_SUPPLY_HEALTH_COOL;
+
+	return POWER_SUPPLY_HEALTH_GOOD;
+}
+
+/**
+ * @brief smbchg_get_status() - Get battery status
+ *
+ * @param chip Pointer to smbchg_chip
+ * @return Battery status, as defined in <linux/power_supply.h>
+ */
+static int smbchg_get_status(struct smbchg_chip *chip)
+{
+	int value, ret, chg_type;
+
+	/* Check if power input is present */
+	/* TODO: Check if DC charge path is present once supported */
+	if (!smbchg_usb_is_present(chip))
+		return POWER_SUPPLY_STATUS_DISCHARGING;
+
+	ret = regmap_read(chip->regmap, chip->base + SMBCHG_CHGR_RT_STS,
+			  &value);
+	if (ret) {
+		dev_err(chip->dev,
+			"Failed to read charger real-time status: %pe\n",
+			ERR_PTR(ret));
+		return POWER_SUPPLY_STATUS_UNKNOWN;
+	}
+	dev_vdbg(chip->dev, "Charger real-time status: 0x%x", value);
+
+	/* Check if temination current is reached or if charging is inhibited */
+	if (value & BAT_TCC_REACHED_BIT || value & CHG_INHIBIT_BIT)
+		return POWER_SUPPLY_STATUS_FULL;
+
+	ret = regmap_read(chip->regmap, chip->base + SMBCHG_CHGR_STS, &value);
+	if (ret) {
+		dev_err(chip->dev, "Failed to read charger status: %pe\n",
+			ERR_PTR(ret));
+		return POWER_SUPPLY_STATUS_UNKNOWN;
+	}
+	dev_vdbg(chip->dev, "Charger status: 0x%x", value);
+
+	/* Check for charger hold-off */
+	if (value & CHG_HOLD_OFF_BIT)
+		return POWER_SUPPLY_STATUS_NOT_CHARGING;
+
+	chg_type = smbchg_get_charge_type(chip);
+	switch (chg_type) {
+	case POWER_SUPPLY_CHARGE_TYPE_UNKNOWN:
+		return POWER_SUPPLY_STATUS_UNKNOWN;
+	case POWER_SUPPLY_CHARGE_TYPE_NONE:
+		return POWER_SUPPLY_STATUS_DISCHARGING;
+	default:
+		return POWER_SUPPLY_STATUS_CHARGING;
+	}
+}
+
+static int smbchg_get_property(struct power_supply *psy,
+			       enum power_supply_property psp,
+			       union power_supply_propval *val)
+{
+	struct smbchg_chip *chip = power_supply_get_drvdata(psy);
+	int ret;
+
+	switch (psp) {
+	case POWER_SUPPLY_PROP_STATUS:
+		val->intval = smbchg_get_status(chip);
+		break;
+	case POWER_SUPPLY_PROP_CHARGE_TYPE:
+		val->intval = smbchg_get_charge_type(chip);
+		break;
+	case POWER_SUPPLY_PROP_HEALTH:
+		val->intval = smbchg_get_health(chip);
+		break;
+	case POWER_SUPPLY_PROP_PRESENT:
+	case POWER_SUPPLY_PROP_ONLINE:
+		val->intval = smbchg_usb_is_present(chip);
+		break;
+	case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT:
+		ret = smbchg_charging_get_ilim(chip);
+		if (ret < 0)
+			return ret;
+		val->intval = ret;
+		break;
+	case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX:
+		val->intval = chip->batt_info->constant_charge_current_max_ua;
+		break;
+	case POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT:
+		ret = smbchg_usb_get_ilim(chip);
+		if (ret < 0)
+			return ret;
+		val->intval = ret;
+		break;
+	case POWER_SUPPLY_PROP_USB_TYPE:
+		val->intval = smbchg_usb_get_type(chip);
+		break;
+	case POWER_SUPPLY_PROP_CHARGE_TERM_CURRENT:
+		ret = smbchg_charging_get_iterm(chip);
+		if (ret < 0)
+			return ret;
+		val->intval = ret;
+		break;
+	default:
+		dev_err(chip->dev, "Invalid property: %d\n", psp);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int smbchg_set_property(struct power_supply *psy,
+			       enum power_supply_property psp,
+			       const union power_supply_propval *val)
+{
+	struct smbchg_chip *chip = power_supply_get_drvdata(psy);
+	int ret;
+
+	switch (psp) {
+	case POWER_SUPPLY_PROP_ONLINE:
+		ret = smbchg_usb_enable(chip, val->intval);
+		break;
+	case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT:
+		/*
+		 * Prevent exceeding the maximum constant charge current
+		 * allowed by the battery
+		 */
+		if (val->intval >
+		    chip->batt_info->constant_charge_current_max_ua)
+			return -EINVAL;
+
+		ret = smbchg_charging_set_ilim(chip, val->intval);
+		break;
+	case POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT:
+		ret = smbchg_usb_set_ilim(chip, val->intval);
+		if (ret > 0)
+			ret = 0;
+		break;
+	default:
+		dev_err(chip->dev, "Invalid property: %d\n", psp);
+		return -EINVAL;
+	}
+
+	return ret;
+}
+
+static int smbchg_property_is_writeable(struct power_supply *psy,
+					enum power_supply_property psp)
+{
+	switch (psp) {
+	case POWER_SUPPLY_PROP_ONLINE:
+	case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT:
+	case POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT:
+		return true;
+	default:
+		return false;
+	}
+
+	return 0;
+}
+
+static enum power_supply_property smbchg_props[] = {
+	POWER_SUPPLY_PROP_STATUS,
+	POWER_SUPPLY_PROP_CHARGE_TYPE,
+	POWER_SUPPLY_PROP_HEALTH,
+	POWER_SUPPLY_PROP_PRESENT,
+	POWER_SUPPLY_PROP_ONLINE,
+	POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT,
+	POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX,
+	POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT,
+	POWER_SUPPLY_PROP_USB_TYPE,
+	POWER_SUPPLY_PROP_CHARGE_TERM_CURRENT
+};
+
+static enum power_supply_usb_type smbchg_usb_types[] = {
+	POWER_SUPPLY_USB_TYPE_UNKNOWN, POWER_SUPPLY_USB_TYPE_SDP,
+	POWER_SUPPLY_USB_TYPE_DCP, POWER_SUPPLY_USB_TYPE_CDP
+};
+
+static const struct power_supply_desc smbchg_usb_psy_desc = {
+	.name = "qcom-smbchg-usb",
+	.type = POWER_SUPPLY_TYPE_USB,
+	.usb_types = smbchg_usb_types,
+	.num_usb_types = ARRAY_SIZE(smbchg_usb_types),
+	.properties = smbchg_props,
+	.num_properties = ARRAY_SIZE(smbchg_props),
+	.get_property = smbchg_get_property,
+	.set_property = smbchg_set_property,
+	.property_is_writeable = smbchg_property_is_writeable
+};
+
+/**
+ * @brief smbchg_init() - Main initialization routine
+ *
+ * @param chip Pointer to smbchg_chip
+ * @return 0 on success, -errno on failure
+ *
+ * @details Initialize charger hardware for USB charging.
+ */
+static int smbchg_init(struct smbchg_chip *chip)
+{
+	int ret;
+
+	/*
+	 * Charger configuration, part 1:
+	 * - Set recharge voltage reading source to fuel gauge
+	 * - Set charge termination current reading source to fuel gauge
+	 */
+	ret = qcom_pmic_sec_masked_write(
+		chip->regmap, chip->base + SMBCHG_CHGR_CHGR_CFG1,
+		RECHG_THRESHOLD_SRC_BIT | TERM_I_SRC_BIT,
+		RCHG_SRC_FG | TERM_SRC_FG);
+	if (ret)
+		return ret;
+
+	/*
+	 * Charger configuration, part 2:
+	 * - Enable charger inhibition
+	 * - Enable automatic recharge
+	 * - Enable current termination
+	 * - Set pre to fast charging transition to automatic
+	 * - Set charge enable source to command
+	 */
+	ret = qcom_pmic_sec_masked_write(
+		chip->regmap, chip->base + SMBCHG_CHGR_CHGR_CFG2,
+		CHARGER_INHIBIT_BIT | AUTO_RECHG_BIT | I_TERM_BIT |
+			P2F_CHG_TRAN_BIT | CHG_EN_SRC_BIT,
+		CHG_INHIBIT_EN | AUTO_RCHG_EN | CURRENT_TERM_EN |
+			PRE_FAST_AUTO | CHG_EN_SRC_CMD);
+	if (ret)
+		return ret;
+
+	/* Set recharge threshold to 100mV */
+	ret = qcom_pmic_sec_masked_write(chip->regmap,
+					 chip->base + SMBCHG_CHGR_CFG,
+					 RCHG_LVL_BIT, RCHG_THRESH_100MV);
+	if (ret)
+		return ret;
+
+	/* Set termination current */
+	if (chip->batt_info->charge_term_current_ua != -EINVAL) {
+		ret = smbchg_charging_set_iterm(
+			chip, chip->batt_info->charge_term_current_ua);
+		if (ret < 0) {
+			dev_err(chip->dev,
+				"Failed to set termination current: %pe\n",
+				ERR_PTR(ret));
+			return ret;
+		}
+	}
+
+	/* Set constant charge current limit */
+	ret = smbchg_charging_set_ilim(
+		chip, chip->batt_info->constant_charge_current_max_ua);
+	if (ret < 0) {
+		dev_err(chip->dev,
+			"Failed to set constant charge current limit: %pe\n",
+			ERR_PTR(ret));
+		return ret;
+	}
+
+	/* Set float voltage */
+	ret = smbchg_charging_set_vfloat(
+		chip, chip->batt_info->voltage_max_design_uv);
+	if (ret < 0) {
+		dev_err(chip->dev, "Failed to set float voltage: %pe\n",
+			ERR_PTR(ret));
+		return ret;
+	}
+
+	/* Enable charging */
+	ret = smbchg_charging_enable(chip, true);
+	if (ret) {
+		dev_err(chip->dev, "Failed to enable charging: %pe\n",
+			ERR_PTR(ret));
+		return ret;
+	}
+
+	/*
+	 * USB charge path configuration:
+	 * - Set USB charge path control to command
+	 * - Set command polarity to full current mode (i.e. setting
+	 *   USB_CHGPTH_CMD_IL:USB51_MODE_BIT corresponds to full SDP current)
+	 */
+	ret = qcom_pmic_sec_masked_write(
+		chip->regmap, chip->base + SMBCHG_USB_CHGPTH_CFG,
+		USB51AC_CTRL | USB51_COMMAND_POL,
+		USB51_COMMAND_CONTROL | USB51AC_COMMAND1_500);
+	if (ret)
+		return ret;
+
+	/* Enable APSD */
+	ret = qcom_pmic_sec_masked_write(
+		chip->regmap, chip->base + SMBCHG_USB_CHGPTH_APSD_CFG,
+		USB_CHGPTH_APSD_EN, USB_CHGPTH_APSD_EN);
+	if (ret) {
+		dev_err(chip->dev, "Failed to enable APSD: %pe\n",
+			ERR_PTR(ret));
+		return ret;
+	}
+
+	/* Enable periodic AICL rerun on the USB charge path */
+	ret = qcom_pmic_sec_masked_write(chip->regmap,
+					 chip->base + SMBCHG_MISC_TRIM_OPT_15_8,
+					 AICL_RERUN_MASK, AICL_RERUN_USB_BIT);
+	if (ret) {
+		dev_err(chip->dev, "Failed to enable AICL rerun: %pe\n",
+			ERR_PTR(ret));
+		return ret;
+	}
+
+	/*
+	 * Call the USB source detect handler once to set USB current limit
+	 * and enable the charge path if USB is present.
+	 */
+	smbchg_handle_usb_source_detect(0, chip);
+
+	return 0;
+}
+
+static int smbchg_probe(struct platform_device *pdev)
+{
+	struct smbchg_chip *chip;
+	struct regulator_config config = {};
+	struct power_supply_config supply_config = {};
+	int i, irq, ret;
+
+	chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL);
+	if (!chip)
+		return -ENOMEM;
+
+	chip->dev = &pdev->dev;
+
+	chip->regmap = dev_get_regmap(chip->dev->parent, NULL);
+	if (!chip->regmap) {
+		dev_err(chip->dev, "Failed to get regmap\n");
+		return -ENODEV;
+	}
+
+	ret = of_property_read_u32(chip->dev->of_node, "reg", &chip->base);
+	if (ret) {
+		dev_err(chip->dev, "Failed to get base address: %pe\n",
+			ERR_PTR(ret));
+		return ret;
+	}
+
+	spin_lock_init(&chip->sec_access_lock);
+	INIT_WORK(&chip->otg_reset_work, smbchg_otg_reset_worker);
+
+	/* Initialize OTG regulator */
+	chip->otg_rdesc.id = -1;
+	chip->otg_rdesc.name = "otg-vbus";
+	chip->otg_rdesc.ops = &smbchg_otg_ops;
+	chip->otg_rdesc.owner = THIS_MODULE;
+	chip->otg_rdesc.type = REGULATOR_VOLTAGE;
+	chip->otg_rdesc.of_match = "otg-vbus";
+
+	config.dev = chip->dev;
+	config.driver_data = chip;
+
+	chip->otg_reg =
+		devm_regulator_register(chip->dev, &chip->otg_rdesc, &config);
+	if (IS_ERR(chip->otg_reg)) {
+		dev_err(chip->dev,
+			"Failed to register OTG VBUS regulator: %pe\n",
+			chip->otg_reg);
+		return PTR_ERR(chip->otg_reg);
+	}
+
+	chip->data = of_device_get_match_data(chip->dev);
+
+	supply_config.drv_data = chip;
+	supply_config.of_node = pdev->dev.of_node;
+	chip->usb_psy = devm_power_supply_register(
+		chip->dev, &smbchg_usb_psy_desc, &supply_config);
+	if (IS_ERR(chip->usb_psy)) {
+		dev_err(chip->dev, "Failed to register USB power supply: %pe\n",
+			chip->usb_psy);
+		return PTR_ERR(chip->usb_psy);
+	}
+
+	ret = power_supply_get_battery_info(chip->usb_psy, &chip->batt_info);
+	if (ret) {
+		dev_err(chip->dev, "Failed to get battery info: %pe\n",
+			ERR_PTR(ret));
+		return ret;
+	}
+
+	if (chip->batt_info->voltage_max_design_uv == -EINVAL) {
+		dev_err(chip->dev,
+			"Battery info missing maximum design voltage\n");
+		return -EINVAL;
+	}
+
+	if (chip->batt_info->constant_charge_current_max_ua == -EINVAL) {
+		dev_err(chip->dev,
+			"Battery info missing maximum constant charge current\n");
+		return -EINVAL;
+	}
+
+	/* Initialize extcon */
+	chip->edev = devm_extcon_dev_allocate(chip->dev, smbchg_extcon_cable);
+	if (IS_ERR(chip->edev)) {
+		dev_err(chip->dev, "Failed to allocate extcon device: %pe\n",
+			chip->edev);
+		return PTR_ERR(chip->edev);
+	}
+
+	ret = devm_extcon_dev_register(chip->dev, chip->edev);
+	if (ret) {
+		dev_err(chip->dev, "Failed to register extcon device: %pe\n",
+			ERR_PTR(ret));
+		return ret;
+	}
+
+	extcon_set_property_capability(chip->edev, EXTCON_USB,
+				       EXTCON_PROP_USB_VBUS);
+	extcon_set_property_capability(chip->edev, EXTCON_USB_HOST,
+				       EXTCON_PROP_USB_VBUS);
+
+	/* Initialize charger */
+	ret = smbchg_init(chip);
+	if (ret)
+		return ret;
+
+	/* Request IRQs */
+	for (i = 0; i < ARRAY_SIZE(smbchg_irqs); ++i) {
+		/* IRQ unused */
+		if (!smbchg_irqs[i].handler)
+			continue;
+
+		irq = of_irq_get_byname(pdev->dev.of_node, smbchg_irqs[i].name);
+		if (irq < 0) {
+			dev_err(chip->dev, "Failed to get %s IRQ: %pe\n",
+				smbchg_irqs[i].name, ERR_PTR(irq));
+			return irq;
+		}
+
+		ret = devm_request_threaded_irq(chip->dev, irq, NULL,
+						smbchg_irqs[i].handler,
+						IRQF_ONESHOT,
+						smbchg_irqs[i].name, chip);
+		if (ret) {
+			dev_err(chip->dev, "failed to request %s IRQ: %pe\n",
+				smbchg_irqs[i].name, ERR_PTR(irq));
+			return ret;
+		}
+	}
+
+	platform_set_drvdata(pdev, chip);
+
+	return 0;
+}
+
+static int smbchg_remove(struct platform_device *pdev)
+{
+	struct smbchg_chip *chip = platform_get_drvdata(pdev);
+
+	smbchg_usb_enable(chip, false);
+	smbchg_charging_enable(chip, false);
+
+	return 0;
+}
+
+static const struct of_device_id smbchg_id_table[] = {
+	{ .compatible = "qcom,pmi8994-smbchg", .data = &smbchg_pmi8994_data },
+	{ .compatible = "qcom,pmi8996-smbchg", .data = &smbchg_pmi8996_data },
+	{}
+};
+MODULE_DEVICE_TABLE(of, smbchg_id_table);
+
+static struct platform_driver smbchg_driver = {
+	.probe = smbchg_probe,
+	.remove = smbchg_remove,
+	.driver = {
+		.name = "qcom-smbchg",
+		.of_match_table = smbchg_id_table,
+	},
+};
+module_platform_driver(smbchg_driver);
+
+MODULE_AUTHOR("Yassine Oudjana <y.oudjana@protonmail.com>");
+MODULE_AUTHOR("Alejandro Tafalla <atafalla@dnyon.com>");
+MODULE_DESCRIPTION("Qualcomm PMIC switch-mode battery charger driver");
+MODULE_LICENSE("GPL");
diff --git a/drivers/power/supply/qcom-smbchg.h b/drivers/power/supply/qcom-smbchg.h
new file mode 100644
index 000000000000..06445e10b4db
--- /dev/null
+++ b/drivers/power/supply/qcom-smbchg.h
@@ -0,0 +1,428 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+/* Registers */
+/* CHGR */
+#define SMBCHG_CHGR_FV_STS			0x00c
+#define SMBCHG_CHGR_STS				0x00e
+#define SMBCHG_CHGR_RT_STS			0x010
+#define SMBCHG_CHGR_FCC_CFG			0x0f2
+#define SMBCHG_CHGR_FCC_CMP_CFG			0x0f3
+#define SMBCHG_CHGR_FV_CFG			0x0f4
+#define SMBCHG_CHGR_FV_CMP			0x0f5
+#define SMBCHG_CHGR_AFVC_CFG			0x0f6
+#define SMBCHG_CHGR_CHG_INHIB_CFG		0x0f7
+#define SMBCHG_CHGR_TCC_CFG			0x0f9
+#define SMBCHG_CHGR_CCMP_CFG			0x0fa
+#define SMBCHG_CHGR_CHGR_CFG1			0x0fb
+#define SMBCHG_CHGR_CHGR_CFG2			0x0fc
+#define SMBCHG_CHGR_SFT_CFG			0x0fd
+#define SMBCHG_CHGR_CFG				0x0ff
+
+/* OTG */
+#define SMBCHG_OTG_RT_STS			0x110
+#define SMBCHG_OTG_OTG_CFG			0x1f1
+#define SMBCHG_OTG_TRIM6			0x1f6
+#define SMBCHG_OTG_LOW_PWR_OPTIONS		0x1ff
+
+/* BAT-IF */
+#define SMBCHG_BAT_IF_BAT_PRES_STS		0x208
+#define SMBCHG_BAT_IF_RT_STS			0x210
+#define SMBCHG_BAT_IF_CMD_CHG			0x242
+#define SMBCHG_BAT_IF_CMD_CHG_LED		0x243
+#define SMBCHG_BAT_IF_BM_CFG			0x2f3
+#define SMBCHG_BAT_IF_BAT_IF_TRIM7		0x2f7
+#define SMBCHG_BAT_IF_BB_CLMP_SEL		0x2f8
+#define SMBCHG_BAT_IF_ZIN_ICL_PT		0x2fc
+#define SMBCHG_BAT_IF_ZIN_ICL_LV		0x2fd
+#define SMBCHG_BAT_IF_ZIN_ICL_HV		0x2fe
+
+/* USB-CHGPTH */
+#define SMBCHG_USB_CHGPTH_ICL_STS_1		0x307
+#define SMBCHG_USB_CHGPTH_PWR_PATH		0x308
+#define SMBCHG_USB_CHGPTH_ICL_STS_2		0x309
+#define SMBCHG_USB_CHGPTH_RID_STS		0x30b
+#define SMBCHG_USB_CHGPTH_USBIN_HVDCP_STS	0x30c
+#define SMBCHG_USB_CHGPTH_INPUT_STS		0x30d
+#define SMBCHG_USB_CHGPTH_USBID_MSB		0x30e
+#define SMBCHG_USB_CHGPTH_RT_STS		0x310
+#define SMBCHG_USB_CHGPTH_CMD_IL		0x340
+#define SMBCHG_USB_CHGPTH_CMD_APSD		0x341
+#define SMBCHG_USB_CHGPTH_CMD_HVDCP_1		0x342
+#define SMBCHG_USB_CHGPTH_CMD_HVDCP_2		0x343
+#define SMBCHG_USB_CHGPTH_USBIN_CHGR_CFG	0x3f1
+#define SMBCHG_USB_CHGPTH_IL_CFG		0x3f2
+#define SMBCHG_USB_CHGPTH_AICL_CFG		0x3f3
+#define SMBCHG_USB_CHGPTH_CFG			0x3f4
+#define SMBCHG_USB_CHGPTH_APSD_CFG		0x3f5
+#define SMBCHG_USB_CHGPTH_TR_RID		0x3fa
+#define SMBCHG_USB_CHGPTH_ICL_BUF_CONFIG	0x3fc
+#define SMBCHG_USB_CHGPTH_TR_8OR32B		0x3fe
+
+/* DC-CHGPTH */
+#define SMBCHG_DC_CHGPTH_RT_STS			0x410
+#define SMBCHG_DC_CHGPTH_IL_CFG			0x4f2
+#define SMBCHG_DC_CHGPTH_AICL_CFG		0x4f3
+#define SMBCHG_DC_CHGPTH_AICL_WL_SEL_CFG	0x4f5
+
+/* MISC */
+#define SMBCHG_MISC_REVISION1			0x600
+#define SMBCHG_MISC_IDEV_STS			0x608
+#define SMBCHG_MISC_RT_STS			0x610
+#define SMBCHG_MISC_TEST			0x6e2
+#define SMBCHG_MISC_NTC_VOUT_CFG		0x6f3
+#define SMBCHG_MISC_TRIM_OPT_15_8		0x6f5
+#define SMBCHG_MISC_TRIM_OPT_7_0		0x6f6
+#define SMBCHG_MISC_TRIM_14			0x6fe
+
+/* Bits */
+/* CHGR_STS bits */
+#define CHG_TYPE_MASK			GENMASK(2, 1)
+#define CHG_HOLD_OFF_BIT		BIT(3)\
+
+/* CHGR_FCC_CFG bits */
+#define FCC_MASK			GENMASK(4, 0)
+
+/* CHGR_RT_STS bits */
+#define CHG_INHIBIT_BIT			BIT(1)
+#define CHG_COMP_SFT_BIT		BIT(3)
+#define BAT_TCC_REACHED_BIT		BIT(7)
+
+/* CHGR_FV_CFG bits */
+#define FV_MASK				GENMASK(5, 0)
+
+/* CHGR_TCC_CFG bits */
+#define CHG_ITERM_MASK			GENMASK(2, 0)
+
+/* CHGR_CHGR_CFG1 bits */
+#define RECHG_THRESHOLD_SRC_BIT		BIT(1)
+#define TERM_I_SRC_BIT			BIT(2)
+
+/* CHGR_CHGR_CFG2 bits */
+#define CHARGER_INHIBIT_BIT		BIT(0)
+#define AUTO_RECHG_BIT			BIT(2)
+#define I_TERM_BIT			BIT(3)
+#define P2F_CHG_TRAN_BIT		BIT(5)
+#define CHG_EN_POLARITY_BIT		BIT(6)
+#define CHG_EN_SRC_BIT			BIT(7)
+
+/* CHGR_CFG bits */
+#define RCHG_LVL_BIT			BIT(0)
+
+/* BAT_IF_CMD_CHG bits */
+#define OTG_EN_BIT			BIT(0)
+#define CHG_EN_BIT			BIT(1)
+
+/* BAT_IF_RT_STS bits */
+#define HOT_BAT_HARD_BIT		BIT(0)
+#define HOT_BAT_SOFT_BIT		BIT(1)
+#define COLD_BAT_HARD_BIT		BIT(2)
+#define COLD_BAT_SOFT_BIT		BIT(3)
+#define BAT_OV_BIT			BIT(4)
+#define BAT_LOW_BIT			BIT(5)
+#define BAT_MISSING_BIT			BIT(6)
+#define BAT_TERM_MISSING_BIT		BIT(7)
+
+/* USB_CHGPTH_ICL_STS_1 bits */
+#define ICL_STS_MASK			GENMASK(4, 0)
+#define AICL_STS_BIT			BIT(5)
+#define AICL_SUSP_BIT			BIT(6)
+
+/* USB_CHGPTH_CFG bits */
+#define USB51AC_CTRL			BIT(1)
+#define USB51_COMMAND_POL		BIT(2)
+#define CFG_USB3P0_SEL_BIT		BIT(7)
+
+/* USB_CHGPTH_RT_STS bits */
+#define USBIN_OV_BIT			BIT(1)
+#define USBIN_SRC_DET_BIT		BIT(2)
+
+/* USB_CHGPTH_RID_STS bits */
+#define RID_MASK			GENMASK(3, 0)
+
+/* USB_CHGPTH_CMD_IL bits */
+#define USBIN_MODE_HC_BIT		BIT(0)
+#define USB51_MODE_BIT			BIT(1)
+#define ICL_OVERRIDE_BIT		BIT(2)
+#define USBIN_SUSPEND_BIT		BIT(4)
+
+/* USB_CHGPTH_IL_CFG bits */
+#define USBIN_INPUT_MASK		GENMASK(4, 0)
+
+/* USB_CHGPTH_AICL_CFG bits */
+#define USB_CHGPTH_AICL_EN		BIT(2)
+
+/* USB_CHGPTH_APSD_CFG bits */
+#define USB_CHGPTH_APSD_EN		BIT(0)
+
+/* MISC_IDEV_STS bits */
+#define FMB_STS_MASK			GENMASK(3, 0)
+
+/* MISC_TRIM_OPT_15_8 bits */
+#define AICL_RERUN_MASK			GENMASK(5, 4)
+#define AICL_RERUN_DC_BIT		BIT(4)
+#define AICL_RERUN_USB_BIT		BIT(5)
+
+/* Values */
+/* CHGR_STS values */
+#define CHG_TYPE_SHIFT			1
+#define BATT_NOT_CHG_VAL		0x0
+#define BATT_PRE_CHG_VAL		0x1
+#define BATT_FAST_CHG_VAL		0x2
+#define BATT_TAPER_CHG_VAL		0x3
+
+/* CHGR_CHGR_CFG1 values */
+#define	RCHG_SRC_ANA			0
+#define RCHG_SRC_FG			BIT(1)
+#define TERM_SRC_ANA			0
+#define TERM_SRC_FG			BIT(2)
+
+/* CHGR_CHGR_CFG2 values */
+#define CHG_INHIBIT_DIS			0
+#define CHG_INHIBIT_EN			BIT(0)
+#define AUTO_RCHG_EN			0
+#define AUTO_RCHG_DIS			BIT(2)
+#define CURRENT_TERM_EN			0
+#define CURRENT_TERM_DIS		BIT(3)
+#define PRE_FAST_AUTO			0
+#define PRE_FAST_CMD			BIT(5)
+#define CHG_EN_SRC_CMD			0
+#define CHG_EN_SRC_PIN			BIT(7)
+
+/* CHGR_CFG values */
+#define RCHG_THRESH_100MV		0
+#define RCHG_THRESH_200MV		BIT(0)
+
+/* USB_CHGPTH_INPUT_STS values */
+#define USBIN_9V			BIT(5)
+#define USBIN_UNREG			BIT(4)
+#define USBIN_LV			BIT(3)
+
+/* USB_CHGPTH_USBID_MSB values */
+#define USBID_GND_THRESHOLD		0x495
+
+/* USB_CHGPTH_CFG values */
+#define USB51_COMMAND_CONTROL		0
+#define USB51_PIN_CONTROL		BIT(1)
+#define USB51AC_COMMAND1_500		0
+#define USB51AC_COMMAND1_100		BIT(2)
+#define USB_2P0_SEL			0
+#define USB_3P0_SEL			BIT(7)
+
+/* MISC_IDEV_STS values */
+#define USB_TYPE_SDP_BIT		BIT(4)
+#define USB_TYPE_OTHER_BIT		BIT(5)
+#define USB_TYPE_DCP_BIT		BIT(6)
+#define USB_TYPE_CDP_BIT		BIT(7)
+
+/* Constant parameters */
+#define NUM_OTG_RESET_RETRIES		5
+#define OTG_RESET_DELAY_MS		20
+
+/*
+ * These values can be indexed using a bitmask:
+ * 0: USB 2.0/3.0
+ * 1: Limited/full current
+ */
+static const int smbchg_lc_ilim_options[] = {
+	[0b00] = 100000,
+	[0b01] = 150000,
+	[0b10] = 500000,
+	[0b11] = 900000
+};
+#define LC_ILIM_USB3_BIT		BIT(0)
+#define LC_ILIM_FULL_CURRENT_BIT	BIT(1)
+#define smbchg_lc_ilim(usb_3, full_current) smbchg_lc_ilim_options[usb_3 | full_current << 1]
+
+struct smbchg_chip {
+	unsigned int base;
+	struct device *dev;
+	struct regmap *regmap;
+
+	struct power_supply_battery_info *batt_info;
+	struct power_supply *usb_psy;
+
+	struct regulator_desc otg_rdesc;
+	struct regulator_dev *otg_reg;
+	int otg_resets;
+
+	struct extcon_dev *edev;
+
+	spinlock_t sec_access_lock;
+	struct work_struct otg_reset_work;
+
+	const struct smbchg_data *data;
+};
+
+struct smbchg_irq {
+	const char *name;
+	irq_handler_t handler;
+};
+
+struct smbchg_data {
+	const int *iterm_table;
+	unsigned int iterm_table_len;
+	const int *ilim_table;
+	unsigned int ilim_table_len;
+
+	bool reset_otg_on_oc;
+};
+
+static const int smbchg_fv_table[] = {
+	3600000,
+	3620000,
+	3640000,
+	3660000,
+	3680000,
+	3700000,
+	3720000,
+	3740000,
+	3760000,
+	3780000,
+	3800000,
+	3820000,
+	3840000,
+	3860000,
+	3880000,
+	3900000,
+	3920000,
+	3940000,
+	3960000,
+	3980000,
+	4000000,
+	4020000,
+	4040000,
+	4060000,
+	4080000,
+	4100000,
+	4120000,
+	4140000,
+	4160000,
+	4180000,
+	4200000,
+	4220000,
+	4240000,
+	4260000,
+	4280000,
+	4300000,
+	4320000,
+	4340000,
+	4350000,
+	4360000,
+	4380000,
+	4400000,
+	4420000,
+	4440000,
+	4460000,
+	4480000,
+	4500000
+};
+
+static const int smbchg_iterm_table_pmi8994[] = {
+	50000,
+	100000,
+	150000,
+	200000,
+	250000,
+	300000,
+	500000,
+	600000
+};
+
+static const int smbchg_ilim_table_pmi8994[] = {
+	300000,
+	400000,
+	450000,
+	475000,
+	500000,
+	550000,
+	600000,
+	650000,
+	700000,
+	900000,
+	950000,
+	1000000,
+	1100000,
+	1200000,
+	1400000,
+	1450000,
+	1500000,
+	1600000,
+	1800000,
+	1850000,
+	1880000,
+	1910000,
+	1930000,
+	1950000,
+	1970000,
+	2000000,
+	2050000,
+	2100000,
+	2300000,
+	2400000,
+	2500000,
+	3000000
+};
+
+static const int smbchg_iterm_table_pmi8996[] = {
+	50000,
+	100000,
+	150000,
+	200000,
+	250000,
+	300000,
+	400000,
+	500000
+};
+
+static const int smbchg_ilim_table_pmi8996[] = {
+	300000,
+	400000,
+	500000,
+	600000,
+	700000,
+	800000,
+	900000,
+	1000000,
+	1100000,
+	1200000,
+	1300000,
+	1400000,
+	1450000,
+	1500000,
+	1550000,
+	1600000,
+	1700000,
+	1800000,
+	1900000,
+	1950000,
+	2000000,
+	2050000,
+	2100000,
+	2200000,
+	2300000,
+	2400000,
+	2500000,
+	2600000,
+	2700000,
+	2800000,
+	2900000,
+	3000000
+};
+
+static const struct smbchg_data smbchg_pmi8994_data = {
+	.iterm_table = smbchg_iterm_table_pmi8994,
+	.iterm_table_len = ARRAY_SIZE(smbchg_iterm_table_pmi8994),
+	.ilim_table = smbchg_ilim_table_pmi8994,
+	.ilim_table_len = ARRAY_SIZE(smbchg_ilim_table_pmi8994),
+
+	.reset_otg_on_oc = true
+};
+
+static const struct smbchg_data smbchg_pmi8996_data = {
+	.iterm_table = smbchg_iterm_table_pmi8996,
+	.iterm_table_len = ARRAY_SIZE(smbchg_iterm_table_pmi8996),
+	.ilim_table = smbchg_ilim_table_pmi8996,
+	.ilim_table_len = ARRAY_SIZE(smbchg_ilim_table_pmi8996),
+
+	.reset_otg_on_oc = true
+};
-- 
2.37.1


^ permalink raw reply related	[flat|nested] 23+ messages in thread

* Re: [PATCH 0/8] power: supply: Add driver for Qualcomm SMBCHG
  2022-08-08  7:34 [PATCH 0/8] power: supply: Add driver for Qualcomm SMBCHG Yassine Oudjana
                   ` (6 preceding siblings ...)
  2022-08-08  7:34 ` [PATCH 8/8] power: supply: Add driver for Qualcomm SMBCHG Yassine Oudjana
@ 2022-08-08  8:41 ` Krzysztof Kozlowski
  2022-08-08  9:39   ` Yassine Oudjana
  7 siblings, 1 reply; 23+ messages in thread
From: Krzysztof Kozlowski @ 2022-08-08  8:41 UTC (permalink / raw)
  To: Yassine Oudjana, Sebastian Reichel, Rob Herring,
	Krzysztof Kozlowski, Andy Gross, Bjorn Andersson, Caleb Connolly
  Cc: Yassine Oudjana, Alejandro Tafalla, Konrad Dybcio, linux-pm,
	linux-arm-msm, devicetree, phone-devel, linux-kernel

On 08/08/2022 10:34, Yassine Oudjana wrote:
> From: Yassine Oudjana <y.oudjana@protonmail.com>
> 
> This series adds a driver for the switch-mode battery charger found on PMICs
> such as PMI8994, and referred to in the vendor kernel[1] as smbcharger or
> SMBCHG. More details on this block can be found in the last patch message.
> 
> This driver currently supports the charger blocks of PMI8994 and PMI8996.
> PMI8950 was also to be supported, but it was dropped due to some last minute
> issues, to be brought back at a later time once ready.
> 
> The OTG regulator remains unused on devices where the charger is enabled in
> this series due to lack of a consumer. Applying a patch[2] adding vbus-supply
> to DWC3 allows it to enable the OTG regulator making USB host without
> external power possible.
> 
> [1] https://github.com/android-linux-stable/msm-3.18/blob/kernel.lnx.3.18.r34-rel/drivers/power/qpnp-smbcharger.c
> [2] https://lore.kernel.org/linux-usb/20200805061744.20404-1-mike.looijmans@topic.nl/

How is it different from PMI8998? I expect not that much, so this should
be based on existing work:
https://lore.kernel.org/linux-arm-msm/20220706194125.1861256-1-caleb.connolly@linaro.org/

Unless they are different, but then please create common parts and
explain the differences.

Best regards,
Krzysztof

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 1/8] dt-bindings: power: supply: Add DT schema for Qualcomm SMBCHG
  2022-08-08  7:34 ` [PATCH 1/8] dt-bindings: power: supply: Add DT schema " Yassine Oudjana
@ 2022-08-08  8:42   ` Krzysztof Kozlowski
  2022-11-20 15:46     ` Yassine Oudjana
  2022-11-30 16:24   ` Krzysztof Kozlowski
  1 sibling, 1 reply; 23+ messages in thread
From: Krzysztof Kozlowski @ 2022-08-08  8:42 UTC (permalink / raw)
  To: Yassine Oudjana, Sebastian Reichel, Rob Herring,
	Krzysztof Kozlowski, Andy Gross, Bjorn Andersson
  Cc: Yassine Oudjana, Alejandro Tafalla, Konrad Dybcio, linux-pm,
	linux-arm-msm, devicetree, phone-devel, linux-kernel

On 08/08/2022 10:34, Yassine Oudjana wrote:
> From: Yassine Oudjana <y.oudjana@protonmail.com>
> 
> Add DT schema for the switch-mode battery charger found on Qualcomm
> PMICs such as PMI8994. Due to lack of documentation, some interrupt
> descriptions might be inaccurate.
> 
> Signed-off-by: Yassine Oudjana <y.oudjana@protonmail.com>
> ---
>  .../bindings/power/supply/qcom,smbchg.yaml    | 205 ++++++++++++++++++
>  MAINTAINERS                                   |   8 +
>  2 files changed, 213 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/power/supply/qcom,smbchg.yaml
> 
> diff --git a/Documentation/devicetree/bindings/power/supply/qcom,smbchg.yaml b/Documentation/devicetree/bindings/power/supply/qcom,smbchg.yaml
> new file mode 100644
> index 000000000000..d825a9c10b3e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/power/supply/qcom,smbchg.yaml
> @@ -0,0 +1,205 @@
> +# SPDX-License-Identifier: (GPL-2.0-only or BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/power/supply/qcom,smbchg.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Qualcomm PMIC Switch-Mode Battery Charger
> +
> +maintainers:
> +  - Yassine Oudjana <y.oudjana@protonmail.com>
> +  - Alejandro Tafalla <atafalla@dnyon.com>
> +
> +properties:
> +  compatible:
> +    enum:
> +      - qcom,pmi8994-smbchg
> +      - qcom,pmi8996-smbchg
> +
> +  reg:
> +    maxItems: 1
> +
> +  monitored-battery:
> +    description: |
> +      phandle of battery characteristics node.
> +      The charger uses the following properties:
> +      - charge-term-current-microamp
> +      - constant-charge-current-max-microamp
> +      - voltage-max-design-microvolt
> +      The constant-charge-current-max-microamp and voltage-max-design-microvolt
> +      properties must be set.
> +      See Documentation/devicetree/bindings/power/supply/battery.yaml
> +
> +  interrupts:
> +    items:
> +      - description: Charger error
> +      - description: Charger inhibited
> +      - description: Charger precharge safety timer timeout
> +      - description: Charger charge safety timer timeout
> +      - description: Charger pre to fast charging switch threshold reached
> +      - description: Charger recharge threshold reached
> +      - description: Charger taper threshold reached
> +      - description: Charger charge termination threshold reached
> +      - description: Battery hot
> +      - description: Battery warm
> +      - description: Battery cold
> +      - description: Battery cool
> +      - description: Battery overvoltage
> +      - description: Battery low
> +      - description: Battery missing
> +      - description: Battery thermistor missing # unconfirmed
> +      - description: USB input undervolt
> +      - description: USB input overvolt
> +      - description: USB input source detected
> +      - description: OTG regulator failure
> +      - description: OTG regulator overcurrent
> +      - description: Automatic input current limiting done
> +      - description: USB ID pin changed
> +      - description: DC input undervolt
> +      - description: DC input overvolt
> +      - description: Power OK
> +      - description: Temperature shutdown
> +      - description: Watchdog timeout
> +      - description: Flash failure
> +      - description: OTST2 # unknown
> +      - description: OTST3 # unknown

It seems you listed register interrupts, not physical pins. This should
be interrupt lines.

Best regards,
Krzysztof

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 8/8] power: supply: Add driver for Qualcomm SMBCHG
  2022-08-08  7:34 ` [PATCH 8/8] power: supply: Add driver for Qualcomm SMBCHG Yassine Oudjana
@ 2022-08-08  8:55   ` Krzysztof Kozlowski
  2022-08-08 10:05     ` Yassine Oudjana
  0 siblings, 1 reply; 23+ messages in thread
From: Krzysztof Kozlowski @ 2022-08-08  8:55 UTC (permalink / raw)
  To: Yassine Oudjana, Sebastian Reichel, Rob Herring,
	Krzysztof Kozlowski, Andy Gross, Bjorn Andersson
  Cc: Yassine Oudjana, Alejandro Tafalla, Konrad Dybcio, linux-pm,
	linux-arm-msm, devicetree, phone-devel, linux-kernel

On 08/08/2022 10:34, Yassine Oudjana wrote:
> From: Yassine Oudjana <y.oudjana@protonmail.com>
> 
> Add a driver for the switch-mode battery charger found on
> PMICs such as PMI8994. This block is referred to in the vendor
> kernel[1] as smbcharger or SMBCHG. It has USB and DC inputs,
> and can generate VBUS for USB OTG with a boost regulator.
> It supports Qualcomm Quick Charge 2.0, and can operate along
> with a parallel charger (SMB1357, or SMB1351 for added Quick
> Charge 3.0 support) for improved efficiency at higher currents.
> 
> At the moment, this driver supports charging off of the USB input
> at 5V with input current limit up to 3A. It also includes support
> for operating the OTG boost regulator as well as extcon
> functionality, reporting states of USB and USB_HOST with VBUS and
> charge port types.
> 
> Co-developed-by: Alejandro Tafalla <atafalla@dnyon.com>
> Signed-off-by: Alejandro Tafalla <atafalla@dnyon.com>
> Signed-off-by: Yassine Oudjana <y.oudjana@protonmail.com>
> 
> [1] https://github.com/android-linux-stable/msm-3.18/blob/kernel.lnx.3.18.r34-rel/drivers/power/qpnp-smbcharger.c
> ---
>  MAINTAINERS                        |    2 +
>  drivers/power/supply/Kconfig       |   11 +
>  drivers/power/supply/Makefile      |    1 +
>  drivers/power/supply/qcom-smbchg.c | 1664 ++++++++++++++++++++++++++++
>  drivers/power/supply/qcom-smbchg.h |  428 +++++++
>  5 files changed, 2106 insertions(+)
>  create mode 100644 drivers/power/supply/qcom-smbchg.c
>  create mode 100644 drivers/power/supply/qcom-smbchg.h
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index f6cf3a27d132..9b8693050890 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -16964,6 +16964,8 @@ L:	linux-pm@vger.kernel.org
>  L:	linux-arm-msm@vger.kernel.org
>  S:	Maintained
>  F:	Documentation/devicetree/bindings/power/supply/qcom,smbchg.yaml
> +F:	drivers/power/supply/qcom-smbchg.c
> +F:	drivers/power/supply/qcom-smbchg.h
>  
>  QUALCOMM TSENS THERMAL DRIVER
>  M:	Amit Kucheria <amitk@kernel.org>
> diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig
> index 1aa8323ad9f6..246bfc118d9f 100644
> --- a/drivers/power/supply/Kconfig
> +++ b/drivers/power/supply/Kconfig
> @@ -633,6 +633,17 @@ config CHARGER_QCOM_SMBB
>  	  documentation for more detail.  The base name for this driver is
>  	  'pm8941_charger'.
>  
> +config CHARGER_QCOM_SMBCHG
> +	tristate "Qualcomm Switch-Mode Battery Charger"

As I mentioned in cover letter, this should be either squashed into
Caleb's work, merged into some common part or kept separate but with
clear explaining why it cannot be merged.

Some incomplete review follows:

> +	depends on MFD_SPMI_PMIC || COMPILE_TEST
> +	depends on OF
> +	depends on EXTCON
> +	depends on REGULATOR
> +	select QCOM_PMIC_SEC_WRITE
> +	help
> +	  Say Y to include support for the Switch-Mode Battery Charger block
> +	  found in Qualcomm PMICs such as PMI8994.
> +
>  config CHARGER_BQ2415X
>  	tristate "TI BQ2415x battery charger driver"
>  	depends on I2C
> diff --git a/drivers/power/supply/Makefile b/drivers/power/supply/Makefile
> index 7f02f36aea55..7c2c037cd8b1 100644
> --- a/drivers/power/supply/Makefile
> +++ b/drivers/power/supply/Makefile
> @@ -83,6 +83,7 @@ obj-$(CONFIG_CHARGER_MAX8998)	+= max8998_charger.o
>  obj-$(CONFIG_CHARGER_MP2629)	+= mp2629_charger.o
>  obj-$(CONFIG_CHARGER_MT6360)	+= mt6360_charger.o
>  obj-$(CONFIG_CHARGER_QCOM_SMBB)	+= qcom_smbb.o
> +obj-$(CONFIG_CHARGER_QCOM_SMBCHG)	+= qcom-smbchg.o
>  obj-$(CONFIG_CHARGER_BQ2415X)	+= bq2415x_charger.o
>  obj-$(CONFIG_CHARGER_BQ24190)	+= bq24190_charger.o
>  obj-$(CONFIG_CHARGER_BQ24257)	+= bq24257_charger.o
> diff --git a/drivers/power/supply/qcom-smbchg.c b/drivers/power/supply/qcom-smbchg.c
> new file mode 100644
> index 000000000000..23a9667953c9
> --- /dev/null
> +++ b/drivers/power/supply/qcom-smbchg.c
> @@ -0,0 +1,1664 @@
> +// SPDX-License-Identifier: GPL-2.0-only

Several things look like based from original sources, so please retain
original copyright.

> +/*
> + * Power supply driver for Qualcomm PMIC switch-mode battery charger
> + *
> + * Copyright (c) 2021 Yassine Oudjana <y.oudjana@protonmail.com>
> + * Copyright (c) 2021 Alejandro Tafalla <atafalla@dnyon.com>
> + */
> +
> +#include <asm/unaligned.h>
> +#include <linux/errno.h>
> +#include <linux/extcon-provider.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/of_irq.h>
> +#include <linux/interrupt.h>
> +#include <linux/platform_device.h>
> +#include <linux/power_supply.h>
> +#include <linux/regmap.h>
> +#include <linux/slab.h>
> +#include <linux/reboot.h>
> +#include <linux/regulator/driver.h>
> +#include <linux/util_macros.h>
> +#include <soc/qcom/pmic-sec-write.h>
> +
> +#include "qcom-smbchg.h"
> +


(...)

> +/**
> + * @brief smbchg_otg_enable() - Enable OTG regulator
> + *
> + * @param rdev Pointer to regulator_dev
> + * @return 0 on success, -errno on failure
> + */
> +static int smbchg_otg_enable(struct regulator_dev *rdev)
> +{
> +	struct smbchg_chip *chip = rdev_get_drvdata(rdev);
> +	int ret;
> +
> +	dev_dbg(chip->dev, "Enabling OTG VBUS regulator");
> +
> +	ret = regmap_update_bits(chip->regmap,
> +				 chip->base + SMBCHG_BAT_IF_CMD_CHG, OTG_EN_BIT,
> +				 OTG_EN_BIT);
> +	if (ret)
> +		dev_err(chip->dev, "Failed to enable OTG regulator: %pe\n",
> +			ERR_PTR(ret));
> +
> +	return ret;
> +}
> +
> +/**
> + * @brief smbchg_otg_disable() - Disable OTG regulator
> + *
> + * @param rdev Pointer to regulator_dev
> + * @return 0 on success, -errno on failure
> + */
> +static int smbchg_otg_disable(struct regulator_dev *rdev)
> +{
> +	struct smbchg_chip *chip = rdev_get_drvdata(rdev);
> +	int ret;
> +
> +	dev_dbg(chip->dev, "Disabling OTG VBUS regulator");

Here and in other places - no dbg messages replacing tracing. We have
tracing for that.

> +
> +	ret = regmap_update_bits(chip->regmap,
> +				 chip->base + SMBCHG_BAT_IF_CMD_CHG, OTG_EN_BIT,
> +				 0);
> +	if (ret) {
> +		dev_err(chip->dev, "Failed to disable OTG regulator: %pe\n",
> +			ERR_PTR(ret));
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +

(...)

> +static irqreturn_t smbchg_handle_charger_error(int irq, void *data)
> +{
> +	struct smbchg_chip *chip = data;
> +
> +	dev_err(chip->dev, "Charger error");
> +
> +	power_supply_changed(chip->usb_psy);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static irqreturn_t smbchg_handle_p2f(int irq, void *data)
> +{
> +	struct smbchg_chip *chip = data;
> +
> +	dev_dbg(chip->dev, "Fast charging threshold reached");
> +
> +	power_supply_changed(chip->usb_psy);
> +
> +	return IRQ_HANDLED;

Several interrupts which are exactly the same... no, use one interrupt
handler for all such cases. Anyway I have some doubts about these 20 or
more separate interrupt lines...

> +}
> +
> +static irqreturn_t smbchg_handle_rechg(int irq, void *data)
> +{
> +	struct smbchg_chip *chip = data;
> +
> +	dev_dbg(chip->dev, "Recharge threshold reached");
> +
> +	/* Auto-recharge is enabled, nothing to do here */
> +	return IRQ_HANDLED;
> +}
> +
> +static irqreturn_t smbchg_handle_taper(int irq, void *data)
> +{
> +	struct smbchg_chip *chip = data;
> +
> +	dev_dbg(chip->dev, "Taper charging threshold reached");
> +
> +	power_supply_changed(chip->usb_psy);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static irqreturn_t smbchg_handle_tcc(int irq, void *data)
> +{
> +	struct smbchg_chip *chip = data;
> +
> +	dev_dbg(chip->dev, "Termination current reached");
> +
> +	power_supply_changed(chip->usb_psy);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static irqreturn_t smbchg_handle_batt_temp(int irq, void *data)
> +{
> +	struct smbchg_chip *chip = data;
> +
> +	power_supply_changed(chip->usb_psy);
> +
> +	return IRQ_HANDLED;
> +}

(...)

> +
> +const struct smbchg_irq smbchg_irqs[] = {

You mix everywhere functions with static variables. All variables -
except platform_driver related go to the beginning of the file.

> +	{ "chg-error", smbchg_handle_charger_error },
> +	{ "chg-inhibit", NULL },
> +	{ "chg-prechg-sft", NULL },
> +	{ "chg-complete-chg-sft", NULL },
> +	{ "chg-p2f-thr", smbchg_handle_p2f },
> +	{ "chg-rechg-thr", smbchg_handle_rechg },
> +	{ "chg-taper-thr", smbchg_handle_taper },
> +	{ "chg-tcc-thr", smbchg_handle_tcc },
> +	{ "batt-hot", smbchg_handle_batt_temp },
> +	{ "batt-warm", smbchg_handle_batt_temp },
> +	{ "batt-cold", smbchg_handle_batt_temp },
> +	{ "batt-cool", smbchg_handle_batt_temp },
> +	{ "batt-ov", NULL },
> +	{ "batt-low", NULL },
> +	{ "batt-missing", smbchg_handle_batt_presence },
> +	{ "batt-term-missing", NULL },
> +	{ "usbin-uv", NULL },
> +	{ "usbin-ov", NULL },
> +	{ "usbin-src-det", smbchg_handle_usb_source_detect },
> +	{ "usbid-change", smbchg_handle_usbid_change },
> +	{ "otg-fail", smbchg_handle_otg_fail },
> +	{ "otg-oc", smbchg_handle_otg_oc },
> +	{ "aicl-done", smbchg_handle_aicl_done },
> +	{ "dcin-uv", NULL },
> +	{ "dcin-ov", NULL },
> +	{ "power-ok", NULL },
> +	{ "temp-shutdown", smbchg_handle_temp_shutdown },
> +	{ "wdog-timeout", NULL },
> +	{ "flash-fail", NULL },
> +	{ "otst2", NULL },
> +	{ "otst3", NULL },
> +};
> +
> +/**

(...)

> +
> +static int smbchg_probe(struct platform_device *pdev)
> +{
> +	struct smbchg_chip *chip;
> +	struct regulator_config config = {};
> +	struct power_supply_config supply_config = {};
> +	int i, irq, ret;
> +
> +	chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL);
> +	if (!chip)
> +		return -ENOMEM;
> +
> +	chip->dev = &pdev->dev;
> +
> +	chip->regmap = dev_get_regmap(chip->dev->parent, NULL);
> +	if (!chip->regmap) {
> +		dev_err(chip->dev, "Failed to get regmap\n");
> +		return -ENODEV;
> +	}
> +
> +	ret = of_property_read_u32(chip->dev->of_node, "reg", &chip->base);

First: device_xxx
Second: what if address is bigger than u32? Shouldn't this be
of_read_number or something similar in device_xxx API?



> +	if (ret) {
> +		dev_err(chip->dev, "Failed to get base address: %pe\n",
> +			ERR_PTR(ret));
> +		return ret;
> +	}
> +
> +	spin_lock_init(&chip->sec_access_lock);
> +	INIT_WORK(&chip->otg_reset_work, smbchg_otg_reset_worker);
> +
> +	/* Initialize OTG regulator */
> +	chip->otg_rdesc.id = -1;
> +	chip->otg_rdesc.name = "otg-vbus";
> +	chip->otg_rdesc.ops = &smbchg_otg_ops;
> +	chip->otg_rdesc.owner = THIS_MODULE;
> +	chip->otg_rdesc.type = REGULATOR_VOLTAGE;
> +	chip->otg_rdesc.of_match = "otg-vbus";
> +
> +	config.dev = chip->dev;
> +	config.driver_data = chip;
> +
> +	chip->otg_reg =
> +		devm_regulator_register(chip->dev, &chip->otg_rdesc, &config);

This wrapping is difficult to read.

> +	if (IS_ERR(chip->otg_reg)) {
> +		dev_err(chip->dev,
> +			"Failed to register OTG VBUS regulator: %pe\n",
> +			chip->otg_reg);
> +		return PTR_ERR(chip->otg_reg);
> +	}
> +
> +	chip->data = of_device_get_match_data(chip->dev);
> +
> +	supply_config.drv_data = chip;
> +	supply_config.of_node = pdev->dev.of_node;
> +	chip->usb_psy = devm_power_supply_register(

This wrapping is difficult to read.

> +		chip->dev, &smbchg_usb_psy_desc, &supply_config);
> +	if (IS_ERR(chip->usb_psy)) {
> +		dev_err(chip->dev, "Failed to register USB power supply: %pe\n",
> +			chip->usb_psy);
> +		return PTR_ERR(chip->usb_psy);
> +	}
> +
> +	ret = power_supply_get_battery_info(chip->usb_psy, &chip->batt_info);
> +	if (ret) {
> +		dev_err(chip->dev, "Failed to get battery info: %pe\n",
> +			ERR_PTR(ret));
> +		return ret;
> +	}
> +
> +	if (chip->batt_info->voltage_max_design_uv == -EINVAL) {
> +		dev_err(chip->dev,
> +			"Battery info missing maximum design voltage\n");
> +		return -EINVAL;
> +	}
> +
> +	if (chip->batt_info->constant_charge_current_max_ua == -EINVAL) {
> +		dev_err(chip->dev,
> +			"Battery info missing maximum constant charge current\n");
> +		return -EINVAL;
> +	}
> +
> +	/* Initialize extcon */
> +	chip->edev = devm_extcon_dev_allocate(chip->dev, smbchg_extcon_cable);
> +	if (IS_ERR(chip->edev)) {
> +		dev_err(chip->dev, "Failed to allocate extcon device: %pe\n",
> +			chip->edev);
> +		return PTR_ERR(chip->edev);
> +	}
> +
> +	ret = devm_extcon_dev_register(chip->dev, chip->edev);
> +	if (ret) {
> +		dev_err(chip->dev, "Failed to register extcon device: %pe\n",
> +			ERR_PTR(ret));
> +		return ret;
> +	}
> +
> +	extcon_set_property_capability(chip->edev, EXTCON_USB,
> +				       EXTCON_PROP_USB_VBUS);
> +	extcon_set_property_capability(chip->edev, EXTCON_USB_HOST,
> +				       EXTCON_PROP_USB_VBUS);
> +
> +	/* Initialize charger */
> +	ret = smbchg_init(chip);
> +	if (ret)
> +		return ret;
> +
> +	/* Request IRQs */
> +	for (i = 0; i < ARRAY_SIZE(smbchg_irqs); ++i) {
> +		/* IRQ unused */
> +		if (!smbchg_irqs[i].handler)
> +			continue;
> +
> +		irq = of_irq_get_byname(pdev->dev.of_node, smbchg_irqs[i].name);
> +		if (irq < 0) {
> +			dev_err(chip->dev, "Failed to get %s IRQ: %pe\n",
> +				smbchg_irqs[i].name, ERR_PTR(irq));
> +			return irq;
> +		}
> +
> +		ret = devm_request_threaded_irq(chip->dev, irq, NULL,
> +						smbchg_irqs[i].handler,
> +						IRQF_ONESHOT,
> +						smbchg_irqs[i].name, chip);
> +		if (ret) {
> +			dev_err(chip->dev, "failed to request %s IRQ: %pe\n",
> +				smbchg_irqs[i].name, ERR_PTR(irq));
> +			return ret;
> +		}
> +	}
> +
> +	platform_set_drvdata(pdev, chip);
> +
> +	return 0;
> +}
> +
> +static int smbchg_remove(struct platform_device *pdev)
> +{
> +	struct smbchg_chip *chip = platform_get_drvdata(pdev);
> +
> +	smbchg_usb_enable(chip, false);
> +	smbchg_charging_enable(chip, false);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id smbchg_id_table[] = {
> +	{ .compatible = "qcom,pmi8994-smbchg", .data = &smbchg_pmi8994_data },
> +	{ .compatible = "qcom,pmi8996-smbchg", .data = &smbchg_pmi8996_data },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, smbchg_id_table);
> +
> +static struct platform_driver smbchg_driver = {
> +	.probe = smbchg_probe,
> +	.remove = smbchg_remove,
> +	.driver = {
> +		.name = "qcom-smbchg",
> +		.of_match_table = smbchg_id_table,
> +	},
> +};
> +module_platform_driver(smbchg_driver);
> +
> +MODULE_AUTHOR("Yassine Oudjana <y.oudjana@protonmail.com>");
> +MODULE_AUTHOR("Alejandro Tafalla <atafalla@dnyon.com>");
> +MODULE_DESCRIPTION("Qualcomm PMIC switch-mode battery charger driver");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/power/supply/qcom-smbchg.h b/drivers/power/supply/qcom-smbchg.h
> new file mode 100644
> index 000000000000..06445e10b4db
> --- /dev/null
> +++ b/drivers/power/supply/qcom-smbchg.h
> @@ -0,0 +1,428 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +
> +/* Registers */
> +/* CHGR */
> +#define SMBCHG_CHGR_FV_STS			0x00c
> +#define SMBCHG_CHGR_STS				0x00e
> +#define SMBCHG_CHGR_RT_STS			0x010
> +#define SMBCHG_CHGR_FCC_CFG			0x0f2
> +#define SMBCHG_CHGR_FCC_CMP_CFG			0x0f3
> +#define SMBCHG_CHGR_FV_CFG			0x0f4
> +#define SMBCHG_CHGR_FV_CMP			0x0f5
> +#define SMBCHG_CHGR_AFVC_CFG			0x0f6
> +#define SMBCHG_CHGR_CHG_INHIB_CFG		0x0f7
> +#define SMBCHG_CHGR_TCC_CFG			0x0f9
> +#define SMBCHG_CHGR_CCMP_CFG			0x0fa
> +#define SMBCHG_CHGR_CHGR_CFG1			0x0fb
> +#define SMBCHG_CHGR_CHGR_CFG2			0x0fc
> +#define SMBCHG_CHGR_SFT_CFG			0x0fd
> +#define SMBCHG_CHGR_CFG				0x0ff
> +
> +/* OTG */
> +#define SMBCHG_OTG_RT_STS			0x110
> +#define SMBCHG_OTG_OTG_CFG			0x1f1
> +#define SMBCHG_OTG_TRIM6			0x1f6
> +#define SMBCHG_OTG_LOW_PWR_OPTIONS		0x1ff
> +
> +/* BAT-IF */
> +#define SMBCHG_BAT_IF_BAT_PRES_STS		0x208
> +#define SMBCHG_BAT_IF_RT_STS			0x210
> +#define SMBCHG_BAT_IF_CMD_CHG			0x242
> +#define SMBCHG_BAT_IF_CMD_CHG_LED		0x243
> +#define SMBCHG_BAT_IF_BM_CFG			0x2f3
> +#define SMBCHG_BAT_IF_BAT_IF_TRIM7		0x2f7
> +#define SMBCHG_BAT_IF_BB_CLMP_SEL		0x2f8
> +#define SMBCHG_BAT_IF_ZIN_ICL_PT		0x2fc
> +#define SMBCHG_BAT_IF_ZIN_ICL_LV		0x2fd
> +#define SMBCHG_BAT_IF_ZIN_ICL_HV		0x2fe
> +
> +/* USB-CHGPTH */
> +#define SMBCHG_USB_CHGPTH_ICL_STS_1		0x307
> +#define SMBCHG_USB_CHGPTH_PWR_PATH		0x308
> +#define SMBCHG_USB_CHGPTH_ICL_STS_2		0x309
> +#define SMBCHG_USB_CHGPTH_RID_STS		0x30b
> +#define SMBCHG_USB_CHGPTH_USBIN_HVDCP_STS	0x30c
> +#define SMBCHG_USB_CHGPTH_INPUT_STS		0x30d
> +#define SMBCHG_USB_CHGPTH_USBID_MSB		0x30e
> +#define SMBCHG_USB_CHGPTH_RT_STS		0x310
> +#define SMBCHG_USB_CHGPTH_CMD_IL		0x340
> +#define SMBCHG_USB_CHGPTH_CMD_APSD		0x341
> +#define SMBCHG_USB_CHGPTH_CMD_HVDCP_1		0x342
> +#define SMBCHG_USB_CHGPTH_CMD_HVDCP_2		0x343
> +#define SMBCHG_USB_CHGPTH_USBIN_CHGR_CFG	0x3f1
> +#define SMBCHG_USB_CHGPTH_IL_CFG		0x3f2
> +#define SMBCHG_USB_CHGPTH_AICL_CFG		0x3f3
> +#define SMBCHG_USB_CHGPTH_CFG			0x3f4
> +#define SMBCHG_USB_CHGPTH_APSD_CFG		0x3f5
> +#define SMBCHG_USB_CHGPTH_TR_RID		0x3fa
> +#define SMBCHG_USB_CHGPTH_ICL_BUF_CONFIG	0x3fc
> +#define SMBCHG_USB_CHGPTH_TR_8OR32B		0x3fe
> +
> +/* DC-CHGPTH */
> +#define SMBCHG_DC_CHGPTH_RT_STS			0x410
> +#define SMBCHG_DC_CHGPTH_IL_CFG			0x4f2
> +#define SMBCHG_DC_CHGPTH_AICL_CFG		0x4f3
> +#define SMBCHG_DC_CHGPTH_AICL_WL_SEL_CFG	0x4f5
> +
> +/* MISC */
> +#define SMBCHG_MISC_REVISION1			0x600
> +#define SMBCHG_MISC_IDEV_STS			0x608
> +#define SMBCHG_MISC_RT_STS			0x610
> +#define SMBCHG_MISC_TEST			0x6e2
> +#define SMBCHG_MISC_NTC_VOUT_CFG		0x6f3
> +#define SMBCHG_MISC_TRIM_OPT_15_8		0x6f5
> +#define SMBCHG_MISC_TRIM_OPT_7_0		0x6f6
> +#define SMBCHG_MISC_TRIM_14			0x6fe
> +
> +/* Bits */
> +/* CHGR_STS bits */
> +#define CHG_TYPE_MASK			GENMASK(2, 1)
> +#define CHG_HOLD_OFF_BIT		BIT(3)\
> +
> +/* CHGR_FCC_CFG bits */
> +#define FCC_MASK			GENMASK(4, 0)
> +
> +/* CHGR_RT_STS bits */
> +#define CHG_INHIBIT_BIT			BIT(1)
> +#define CHG_COMP_SFT_BIT		BIT(3)
> +#define BAT_TCC_REACHED_BIT		BIT(7)
> +
> +/* CHGR_FV_CFG bits */
> +#define FV_MASK				GENMASK(5, 0)
> +
> +/* CHGR_TCC_CFG bits */
> +#define CHG_ITERM_MASK			GENMASK(2, 0)
> +
> +/* CHGR_CHGR_CFG1 bits */
> +#define RECHG_THRESHOLD_SRC_BIT		BIT(1)
> +#define TERM_I_SRC_BIT			BIT(2)
> +
> +/* CHGR_CHGR_CFG2 bits */
> +#define CHARGER_INHIBIT_BIT		BIT(0)
> +#define AUTO_RECHG_BIT			BIT(2)
> +#define I_TERM_BIT			BIT(3)
> +#define P2F_CHG_TRAN_BIT		BIT(5)
> +#define CHG_EN_POLARITY_BIT		BIT(6)
> +#define CHG_EN_SRC_BIT			BIT(7)
> +
> +/* CHGR_CFG bits */
> +#define RCHG_LVL_BIT			BIT(0)
> +
> +/* BAT_IF_CMD_CHG bits */
> +#define OTG_EN_BIT			BIT(0)
> +#define CHG_EN_BIT			BIT(1)
> +
> +/* BAT_IF_RT_STS bits */
> +#define HOT_BAT_HARD_BIT		BIT(0)
> +#define HOT_BAT_SOFT_BIT		BIT(1)
> +#define COLD_BAT_HARD_BIT		BIT(2)
> +#define COLD_BAT_SOFT_BIT		BIT(3)
> +#define BAT_OV_BIT			BIT(4)
> +#define BAT_LOW_BIT			BIT(5)
> +#define BAT_MISSING_BIT			BIT(6)
> +#define BAT_TERM_MISSING_BIT		BIT(7)
> +
> +/* USB_CHGPTH_ICL_STS_1 bits */
> +#define ICL_STS_MASK			GENMASK(4, 0)
> +#define AICL_STS_BIT			BIT(5)
> +#define AICL_SUSP_BIT			BIT(6)
> +
> +/* USB_CHGPTH_CFG bits */
> +#define USB51AC_CTRL			BIT(1)
> +#define USB51_COMMAND_POL		BIT(2)
> +#define CFG_USB3P0_SEL_BIT		BIT(7)
> +
> +/* USB_CHGPTH_RT_STS bits */
> +#define USBIN_OV_BIT			BIT(1)
> +#define USBIN_SRC_DET_BIT		BIT(2)
> +
> +/* USB_CHGPTH_RID_STS bits */
> +#define RID_MASK			GENMASK(3, 0)
> +
> +/* USB_CHGPTH_CMD_IL bits */
> +#define USBIN_MODE_HC_BIT		BIT(0)
> +#define USB51_MODE_BIT			BIT(1)
> +#define ICL_OVERRIDE_BIT		BIT(2)
> +#define USBIN_SUSPEND_BIT		BIT(4)
> +
> +/* USB_CHGPTH_IL_CFG bits */
> +#define USBIN_INPUT_MASK		GENMASK(4, 0)
> +
> +/* USB_CHGPTH_AICL_CFG bits */
> +#define USB_CHGPTH_AICL_EN		BIT(2)
> +
> +/* USB_CHGPTH_APSD_CFG bits */
> +#define USB_CHGPTH_APSD_EN		BIT(0)
> +
> +/* MISC_IDEV_STS bits */
> +#define FMB_STS_MASK			GENMASK(3, 0)
> +
> +/* MISC_TRIM_OPT_15_8 bits */
> +#define AICL_RERUN_MASK			GENMASK(5, 4)
> +#define AICL_RERUN_DC_BIT		BIT(4)
> +#define AICL_RERUN_USB_BIT		BIT(5)
> +
> +/* Values */
> +/* CHGR_STS values */
> +#define CHG_TYPE_SHIFT			1
> +#define BATT_NOT_CHG_VAL		0x0
> +#define BATT_PRE_CHG_VAL		0x1
> +#define BATT_FAST_CHG_VAL		0x2
> +#define BATT_TAPER_CHG_VAL		0x3
> +
> +/* CHGR_CHGR_CFG1 values */
> +#define	RCHG_SRC_ANA			0
> +#define RCHG_SRC_FG			BIT(1)
> +#define TERM_SRC_ANA			0
> +#define TERM_SRC_FG			BIT(2)
> +
> +/* CHGR_CHGR_CFG2 values */
> +#define CHG_INHIBIT_DIS			0
> +#define CHG_INHIBIT_EN			BIT(0)
> +#define AUTO_RCHG_EN			0
> +#define AUTO_RCHG_DIS			BIT(2)
> +#define CURRENT_TERM_EN			0
> +#define CURRENT_TERM_DIS		BIT(3)
> +#define PRE_FAST_AUTO			0
> +#define PRE_FAST_CMD			BIT(5)
> +#define CHG_EN_SRC_CMD			0
> +#define CHG_EN_SRC_PIN			BIT(7)
> +
> +/* CHGR_CFG values */
> +#define RCHG_THRESH_100MV		0
> +#define RCHG_THRESH_200MV		BIT(0)
> +
> +/* USB_CHGPTH_INPUT_STS values */
> +#define USBIN_9V			BIT(5)
> +#define USBIN_UNREG			BIT(4)
> +#define USBIN_LV			BIT(3)
> +
> +/* USB_CHGPTH_USBID_MSB values */
> +#define USBID_GND_THRESHOLD		0x495
> +
> +/* USB_CHGPTH_CFG values */
> +#define USB51_COMMAND_CONTROL		0
> +#define USB51_PIN_CONTROL		BIT(1)
> +#define USB51AC_COMMAND1_500		0
> +#define USB51AC_COMMAND1_100		BIT(2)
> +#define USB_2P0_SEL			0
> +#define USB_3P0_SEL			BIT(7)
> +
> +/* MISC_IDEV_STS values */
> +#define USB_TYPE_SDP_BIT		BIT(4)
> +#define USB_TYPE_OTHER_BIT		BIT(5)
> +#define USB_TYPE_DCP_BIT		BIT(6)
> +#define USB_TYPE_CDP_BIT		BIT(7)
> +
> +/* Constant parameters */
> +#define NUM_OTG_RESET_RETRIES		5
> +#define OTG_RESET_DELAY_MS		20
> +
> +/*
> + * These values can be indexed using a bitmask:
> + * 0: USB 2.0/3.0
> + * 1: Limited/full current
> + */
> +static const int smbchg_lc_ilim_options[] = {
> +	[0b00] = 100000,
> +	[0b01] = 150000,
> +	[0b10] = 500000,
> +	[0b11] = 900000
> +};
> +#define LC_ILIM_USB3_BIT		BIT(0)
> +#define LC_ILIM_FULL_CURRENT_BIT	BIT(1)
> +#define smbchg_lc_ilim(usb_3, full_current) smbchg_lc_ilim_options[usb_3 | full_current << 1]
> +
> +struct smbchg_chip {
> +	unsigned int base;
> +	struct device *dev;
> +	struct regmap *regmap;
> +
> +	struct power_supply_battery_info *batt_info;
> +	struct power_supply *usb_psy;
> +
> +	struct regulator_desc otg_rdesc;
> +	struct regulator_dev *otg_reg;
> +	int otg_resets;
> +
> +	struct extcon_dev *edev;
> +
> +	spinlock_t sec_access_lock;
> +	struct work_struct otg_reset_work;
> +
> +	const struct smbchg_data *data;
> +};
> +
> +struct smbchg_irq {
> +	const char *name;
> +	irq_handler_t handler;
> +};
> +
> +struct smbchg_data {
> +	const int *iterm_table;
> +	unsigned int iterm_table_len;
> +	const int *ilim_table;
> +	unsigned int ilim_table_len;
> +
> +	bool reset_otg_on_oc;
> +};
> +
> +static const int smbchg_fv_table[] = {

No static data in headers.

Best regards,
Krzysztof

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 0/8] power: supply: Add driver for Qualcomm SMBCHG
  2022-08-08  8:41 ` [PATCH 0/8] " Krzysztof Kozlowski
@ 2022-08-08  9:39   ` Yassine Oudjana
  2022-08-08 13:24     ` Caleb Connolly
  0 siblings, 1 reply; 23+ messages in thread
From: Yassine Oudjana @ 2022-08-08  9:39 UTC (permalink / raw)
  To: Krzysztof Kozlowski
  Cc: Sebastian Reichel, Rob Herring, Krzysztof Kozlowski, Andy Gross,
	Bjorn Andersson, Caleb Connolly, Yassine Oudjana,
	Alejandro Tafalla, Konrad Dybcio, linux-pm, linux-arm-msm,
	devicetree, phone-devel, linux-kernel


On Mon, Aug 8 2022 at 11:41:26 +03:00:00, Krzysztof Kozlowski 
<krzysztof.kozlowski@linaro.org> wrote:
> On 08/08/2022 10:34, Yassine Oudjana wrote:
>>  From: Yassine Oudjana <y.oudjana@protonmail.com>
>> 
>>  This series adds a driver for the switch-mode battery charger found 
>> on PMICs
>>  such as PMI8994, and referred to in the vendor kernel[1] as 
>> smbcharger or
>>  SMBCHG. More details on this block can be found in the last patch 
>> message.
>> 
>>  This driver currently supports the charger blocks of PMI8994 and 
>> PMI8996.
>>  PMI8950 was also to be supported, but it was dropped due to some 
>> last minute
>>  issues, to be brought back at a later time once ready.
>> 
>>  The OTG regulator remains unused on devices where the charger is 
>> enabled in
>>  this series due to lack of a consumer. Applying a patch[2] adding 
>> vbus-supply
>>  to DWC3 allows it to enable the OTG regulator making USB host 
>> without
>>  external power possible.
>> 
>>  [1] 
>> https://github.com/android-linux-stable/msm-3.18/blob/kernel.lnx.3.18.r34-rel/drivers/power/qpnp-smbcharger.c
>>  [2] 
>> https://lore.kernel.org/linux-usb/20200805061744.20404-1-mike.looijmans@topic.nl/
> 
> How is it different from PMI8998? I expect not that much, so this 
> should
> be based on existing work:
> https://lore.kernel.org/linux-arm-msm/20220706194125.1861256-1-caleb.connolly@linaro.org/
> 
> Unless they are different, but then please create common parts and
> explain the differences.
> 
> Best regards,
> Krzysztof

This driver has been in slow developement for a long time before that 
one existed, which was why no initial attempt at a common driver was 
made. With that said however, I've been watching its development even 
before it was sent for review, and It seems that the hardware is 
actually quite different. For example, the original charger entirely 
lacks the type-c functionality that exists on the second gen one. There 
are a couple of similar registers like CMD_APSD (same address and 
function) CHGR_CFG2 (same/similar function, different address), but 
other than that there don't seem to be any major similarities. While I 
guess it would technically be possible to force them into one driver 
with multiple register tables and separate functions for most tasks, I 
think it would just unnecessarily complicate things. One thing that is 
common however is the secure register unlock sequence, which I have 
separated in patch 6 to allow for its use in other drivers (the fuel 
gauge block has secure registers too so it will also be used in an 
upcoming fuel gauge driver).



^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 8/8] power: supply: Add driver for Qualcomm SMBCHG
  2022-08-08  8:55   ` Krzysztof Kozlowski
@ 2022-08-08 10:05     ` Yassine Oudjana
  2022-08-08 13:42       ` Krzysztof Kozlowski
  0 siblings, 1 reply; 23+ messages in thread
From: Yassine Oudjana @ 2022-08-08 10:05 UTC (permalink / raw)
  To: Krzysztof Kozlowski
  Cc: Sebastian Reichel, Rob Herring, Krzysztof Kozlowski, Andy Gross,
	Bjorn Andersson, Yassine Oudjana, Alejandro Tafalla,
	Konrad Dybcio, linux-pm, linux-arm-msm, devicetree, phone-devel,
	linux-kernel


On Mon, Aug 8 2022 at 11:55:02 +03:00:00, Krzysztof Kozlowski 
<krzysztof.kozlowski@linaro.org> wrote:
> On 08/08/2022 10:34, Yassine Oudjana wrote:
>>  From: Yassine Oudjana <y.oudjana@protonmail.com>
>> 
>>  Add a driver for the switch-mode battery charger found on
>>  PMICs such as PMI8994. This block is referred to in the vendor
>>  kernel[1] as smbcharger or SMBCHG. It has USB and DC inputs,
>>  and can generate VBUS for USB OTG with a boost regulator.
>>  It supports Qualcomm Quick Charge 2.0, and can operate along
>>  with a parallel charger (SMB1357, or SMB1351 for added Quick
>>  Charge 3.0 support) for improved efficiency at higher currents.
>> 
>>  At the moment, this driver supports charging off of the USB input
>>  at 5V with input current limit up to 3A. It also includes support
>>  for operating the OTG boost regulator as well as extcon
>>  functionality, reporting states of USB and USB_HOST with VBUS and
>>  charge port types.
>> 
>>  Co-developed-by: Alejandro Tafalla <atafalla@dnyon.com>
>>  Signed-off-by: Alejandro Tafalla <atafalla@dnyon.com>
>>  Signed-off-by: Yassine Oudjana <y.oudjana@protonmail.com>
>> 
>>  [1] 
>> https://github.com/android-linux-stable/msm-3.18/blob/kernel.lnx.3.18.r34-rel/drivers/power/qpnp-smbcharger.c
>>  ---
>>   MAINTAINERS                        |    2 +
>>   drivers/power/supply/Kconfig       |   11 +
>>   drivers/power/supply/Makefile      |    1 +
>>   drivers/power/supply/qcom-smbchg.c | 1664 
>> ++++++++++++++++++++++++++++
>>   drivers/power/supply/qcom-smbchg.h |  428 +++++++
>>   5 files changed, 2106 insertions(+)
>>   create mode 100644 drivers/power/supply/qcom-smbchg.c
>>   create mode 100644 drivers/power/supply/qcom-smbchg.h
>> 
>>  diff --git a/MAINTAINERS b/MAINTAINERS
>>  index f6cf3a27d132..9b8693050890 100644
>>  --- a/MAINTAINERS
>>  +++ b/MAINTAINERS
>>  @@ -16964,6 +16964,8 @@ L:	linux-pm@vger.kernel.org
>>   L:	linux-arm-msm@vger.kernel.org
>>   S:	Maintained
>>   F:	Documentation/devicetree/bindings/power/supply/qcom,smbchg.yaml
>>  +F:	drivers/power/supply/qcom-smbchg.c
>>  +F:	drivers/power/supply/qcom-smbchg.h
>> 
>>   QUALCOMM TSENS THERMAL DRIVER
>>   M:	Amit Kucheria <amitk@kernel.org>
>>  diff --git a/drivers/power/supply/Kconfig 
>> b/drivers/power/supply/Kconfig
>>  index 1aa8323ad9f6..246bfc118d9f 100644
>>  --- a/drivers/power/supply/Kconfig
>>  +++ b/drivers/power/supply/Kconfig
>>  @@ -633,6 +633,17 @@ config CHARGER_QCOM_SMBB
>>   	  documentation for more detail.  The base name for this driver is
>>   	  'pm8941_charger'.
>> 
>>  +config CHARGER_QCOM_SMBCHG
>>  +	tristate "Qualcomm Switch-Mode Battery Charger"
> 
> As I mentioned in cover letter, this should be either squashed into
> Caleb's work, merged into some common part or kept separate but with
> clear explaining why it cannot be merged.
> 
> Some incomplete review follows:
> 
>>  +	depends on MFD_SPMI_PMIC || COMPILE_TEST
>>  +	depends on OF
>>  +	depends on EXTCON
>>  +	depends on REGULATOR
>>  +	select QCOM_PMIC_SEC_WRITE
>>  +	help
>>  +	  Say Y to include support for the Switch-Mode Battery Charger 
>> block
>>  +	  found in Qualcomm PMICs such as PMI8994.
>>  +
>>   config CHARGER_BQ2415X
>>   	tristate "TI BQ2415x battery charger driver"
>>   	depends on I2C
>>  diff --git a/drivers/power/supply/Makefile 
>> b/drivers/power/supply/Makefile
>>  index 7f02f36aea55..7c2c037cd8b1 100644
>>  --- a/drivers/power/supply/Makefile
>>  +++ b/drivers/power/supply/Makefile
>>  @@ -83,6 +83,7 @@ obj-$(CONFIG_CHARGER_MAX8998)	+= max8998_charger.o
>>   obj-$(CONFIG_CHARGER_MP2629)	+= mp2629_charger.o
>>   obj-$(CONFIG_CHARGER_MT6360)	+= mt6360_charger.o
>>   obj-$(CONFIG_CHARGER_QCOM_SMBB)	+= qcom_smbb.o
>>  +obj-$(CONFIG_CHARGER_QCOM_SMBCHG)	+= qcom-smbchg.o
>>   obj-$(CONFIG_CHARGER_BQ2415X)	+= bq2415x_charger.o
>>   obj-$(CONFIG_CHARGER_BQ24190)	+= bq24190_charger.o
>>   obj-$(CONFIG_CHARGER_BQ24257)	+= bq24257_charger.o
>>  diff --git a/drivers/power/supply/qcom-smbchg.c 
>> b/drivers/power/supply/qcom-smbchg.c
>>  new file mode 100644
>>  index 000000000000..23a9667953c9
>>  --- /dev/null
>>  +++ b/drivers/power/supply/qcom-smbchg.c
>>  @@ -0,0 +1,1664 @@
>>  +// SPDX-License-Identifier: GPL-2.0-only
> 
> Several things look like based from original sources, so please retain
> original copyright.

Do I replace the existing copyright here with the original one, just 
add it, or mention that this driver is based on downstream sources 
(maybe putting a link as well) then add it?

> 
>>  +/*
>>  + * Power supply driver for Qualcomm PMIC switch-mode battery 
>> charger
>>  + *
>>  + * Copyright (c) 2021 Yassine Oudjana <y.oudjana@protonmail.com>
>>  + * Copyright (c) 2021 Alejandro Tafalla <atafalla@dnyon.com>
>>  + */
>>  +
>>  +#include <asm/unaligned.h>
>>  +#include <linux/errno.h>
>>  +#include <linux/extcon-provider.h>
>>  +#include <linux/kernel.h>
>>  +#include <linux/module.h>
>>  +#include <linux/of.h>
>>  +#include <linux/of_device.h>
>>  +#include <linux/of_irq.h>
>>  +#include <linux/interrupt.h>
>>  +#include <linux/platform_device.h>
>>  +#include <linux/power_supply.h>
>>  +#include <linux/regmap.h>
>>  +#include <linux/slab.h>
>>  +#include <linux/reboot.h>
>>  +#include <linux/regulator/driver.h>
>>  +#include <linux/util_macros.h>
>>  +#include <soc/qcom/pmic-sec-write.h>
>>  +
>>  +#include "qcom-smbchg.h"
>>  +
> 
> 
> (...)
> 
>>  +/**
>>  + * @brief smbchg_otg_enable() - Enable OTG regulator
>>  + *
>>  + * @param rdev Pointer to regulator_dev
>>  + * @return 0 on success, -errno on failure
>>  + */
>>  +static int smbchg_otg_enable(struct regulator_dev *rdev)
>>  +{
>>  +	struct smbchg_chip *chip = rdev_get_drvdata(rdev);
>>  +	int ret;
>>  +
>>  +	dev_dbg(chip->dev, "Enabling OTG VBUS regulator");
>>  +
>>  +	ret = regmap_update_bits(chip->regmap,
>>  +				 chip->base + SMBCHG_BAT_IF_CMD_CHG, OTG_EN_BIT,
>>  +				 OTG_EN_BIT);
>>  +	if (ret)
>>  +		dev_err(chip->dev, "Failed to enable OTG regulator: %pe\n",
>>  +			ERR_PTR(ret));
>>  +
>>  +	return ret;
>>  +}
>>  +
>>  +/**
>>  + * @brief smbchg_otg_disable() - Disable OTG regulator
>>  + *
>>  + * @param rdev Pointer to regulator_dev
>>  + * @return 0 on success, -errno on failure
>>  + */
>>  +static int smbchg_otg_disable(struct regulator_dev *rdev)
>>  +{
>>  +	struct smbchg_chip *chip = rdev_get_drvdata(rdev);
>>  +	int ret;
>>  +
>>  +	dev_dbg(chip->dev, "Disabling OTG VBUS regulator");
> 
> Here and in other places - no dbg messages replacing tracing. We have
> tracing for that.

Will look into it.

> 
>>  +
>>  +	ret = regmap_update_bits(chip->regmap,
>>  +				 chip->base + SMBCHG_BAT_IF_CMD_CHG, OTG_EN_BIT,
>>  +				 0);
>>  +	if (ret) {
>>  +		dev_err(chip->dev, "Failed to disable OTG regulator: %pe\n",
>>  +			ERR_PTR(ret));
>>  +		return ret;
>>  +	}
>>  +
>>  +	return 0;
>>  +}
>>  +
> 
> (...)
> 
>>  +static irqreturn_t smbchg_handle_charger_error(int irq, void *data)
>>  +{
>>  +	struct smbchg_chip *chip = data;
>>  +
>>  +	dev_err(chip->dev, "Charger error");
>>  +
>>  +	power_supply_changed(chip->usb_psy);
>>  +
>>  +	return IRQ_HANDLED;
>>  +}
>>  +
>>  +static irqreturn_t smbchg_handle_p2f(int irq, void *data)
>>  +{
>>  +	struct smbchg_chip *chip = data;
>>  +
>>  +	dev_dbg(chip->dev, "Fast charging threshold reached");
>>  +
>>  +	power_supply_changed(chip->usb_psy);
>>  +
>>  +	return IRQ_HANDLED;
> 
> Several interrupts which are exactly the same... no, use one interrupt
> handler for all such cases. Anyway I have some doubts about these 20 
> or
> more separate interrupt lines...

This was to allow for printing different debug messages. I believe 
there are interrupt status registers that can be used instead, so I 
might try to use them to find out the specific event in a single 
handler while adding tracing.

> 
>>  +}
>>  +
>>  +static irqreturn_t smbchg_handle_rechg(int irq, void *data)
>>  +{
>>  +	struct smbchg_chip *chip = data;
>>  +
>>  +	dev_dbg(chip->dev, "Recharge threshold reached");
>>  +
>>  +	/* Auto-recharge is enabled, nothing to do here */
>>  +	return IRQ_HANDLED;
>>  +}
>>  +
>>  +static irqreturn_t smbchg_handle_taper(int irq, void *data)
>>  +{
>>  +	struct smbchg_chip *chip = data;
>>  +
>>  +	dev_dbg(chip->dev, "Taper charging threshold reached");
>>  +
>>  +	power_supply_changed(chip->usb_psy);
>>  +
>>  +	return IRQ_HANDLED;
>>  +}
>>  +
>>  +static irqreturn_t smbchg_handle_tcc(int irq, void *data)
>>  +{
>>  +	struct smbchg_chip *chip = data;
>>  +
>>  +	dev_dbg(chip->dev, "Termination current reached");
>>  +
>>  +	power_supply_changed(chip->usb_psy);
>>  +
>>  +	return IRQ_HANDLED;
>>  +}
>>  +
>>  +static irqreturn_t smbchg_handle_batt_temp(int irq, void *data)
>>  +{
>>  +	struct smbchg_chip *chip = data;
>>  +
>>  +	power_supply_changed(chip->usb_psy);
>>  +
>>  +	return IRQ_HANDLED;
>>  +}
> 
> (...)
> 
>>  +
>>  +const struct smbchg_irq smbchg_irqs[] = {
> 
> You mix everywhere functions with static variables. All variables -
> except platform_driver related go to the beginning of the file.

They were grouped based on their topic (interrupt handling, power 
supply properties, ...). Will rearrange them.

> 
>>  +	{ "chg-error", smbchg_handle_charger_error },
>>  +	{ "chg-inhibit", NULL },
>>  +	{ "chg-prechg-sft", NULL },
>>  +	{ "chg-complete-chg-sft", NULL },
>>  +	{ "chg-p2f-thr", smbchg_handle_p2f },
>>  +	{ "chg-rechg-thr", smbchg_handle_rechg },
>>  +	{ "chg-taper-thr", smbchg_handle_taper },
>>  +	{ "chg-tcc-thr", smbchg_handle_tcc },
>>  +	{ "batt-hot", smbchg_handle_batt_temp },
>>  +	{ "batt-warm", smbchg_handle_batt_temp },
>>  +	{ "batt-cold", smbchg_handle_batt_temp },
>>  +	{ "batt-cool", smbchg_handle_batt_temp },
>>  +	{ "batt-ov", NULL },
>>  +	{ "batt-low", NULL },
>>  +	{ "batt-missing", smbchg_handle_batt_presence },
>>  +	{ "batt-term-missing", NULL },
>>  +	{ "usbin-uv", NULL },
>>  +	{ "usbin-ov", NULL },
>>  +	{ "usbin-src-det", smbchg_handle_usb_source_detect },
>>  +	{ "usbid-change", smbchg_handle_usbid_change },
>>  +	{ "otg-fail", smbchg_handle_otg_fail },
>>  +	{ "otg-oc", smbchg_handle_otg_oc },
>>  +	{ "aicl-done", smbchg_handle_aicl_done },
>>  +	{ "dcin-uv", NULL },
>>  +	{ "dcin-ov", NULL },
>>  +	{ "power-ok", NULL },
>>  +	{ "temp-shutdown", smbchg_handle_temp_shutdown },
>>  +	{ "wdog-timeout", NULL },
>>  +	{ "flash-fail", NULL },
>>  +	{ "otst2", NULL },
>>  +	{ "otst3", NULL },
>>  +};
>>  +
>>  +/**
> 
> (...)
> 
>>  +
>>  +static int smbchg_probe(struct platform_device *pdev)
>>  +{
>>  +	struct smbchg_chip *chip;
>>  +	struct regulator_config config = {};
>>  +	struct power_supply_config supply_config = {};
>>  +	int i, irq, ret;
>>  +
>>  +	chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL);
>>  +	if (!chip)
>>  +		return -ENOMEM;
>>  +
>>  +	chip->dev = &pdev->dev;
>>  +
>>  +	chip->regmap = dev_get_regmap(chip->dev->parent, NULL);
>>  +	if (!chip->regmap) {
>>  +		dev_err(chip->dev, "Failed to get regmap\n");
>>  +		return -ENODEV;
>>  +	}
>>  +
>>  +	ret = of_property_read_u32(chip->dev->of_node, "reg", 
>> &chip->base);
> 
> First: device_xxx

Okay.

> Second: what if address is bigger than u32? Shouldn't this be
> of_read_number or something similar in device_xxx API?

The address wouldn't exceed sizeof(u16). Actually now I think I 
should've used property_read_u16 instead. I couldn't find a device_* 
equivalent of of_read_number (or at least not a direct one), are you 
sure it exists?

> 
>>  +	if (ret) {
>>  +		dev_err(chip->dev, "Failed to get base address: %pe\n",
>>  +			ERR_PTR(ret));
>>  +		return ret;
>>  +	}
>>  +
>>  +	spin_lock_init(&chip->sec_access_lock);
>>  +	INIT_WORK(&chip->otg_reset_work, smbchg_otg_reset_worker);
>>  +
>>  +	/* Initialize OTG regulator */
>>  +	chip->otg_rdesc.id = -1;
>>  +	chip->otg_rdesc.name = "otg-vbus";
>>  +	chip->otg_rdesc.ops = &smbchg_otg_ops;
>>  +	chip->otg_rdesc.owner = THIS_MODULE;
>>  +	chip->otg_rdesc.type = REGULATOR_VOLTAGE;
>>  +	chip->otg_rdesc.of_match = "otg-vbus";
>>  +
>>  +	config.dev = chip->dev;
>>  +	config.driver_data = chip;
>>  +
>>  +	chip->otg_reg =
>>  +		devm_regulator_register(chip->dev, &chip->otg_rdesc, &config);
> 
> This wrapping is difficult to read.

This was done by clang-format. I don't mind changing it manually, but 
shouldn't it be conforming to the linux code style if it was done 
automatically following the .clang-format config file in the root of 
the kernel tree?

> 
>>  +	if (IS_ERR(chip->otg_reg)) {
>>  +		dev_err(chip->dev,
>>  +			"Failed to register OTG VBUS regulator: %pe\n",
>>  +			chip->otg_reg);
>>  +		return PTR_ERR(chip->otg_reg);
>>  +	}
>>  +
>>  +	chip->data = of_device_get_match_data(chip->dev);
>>  +
>>  +	supply_config.drv_data = chip;
>>  +	supply_config.of_node = pdev->dev.of_node;
>>  +	chip->usb_psy = devm_power_supply_register(
> 
> This wrapping is difficult to read.

Ditto.

> 
>>  +		chip->dev, &smbchg_usb_psy_desc, &supply_config);
>>  +	if (IS_ERR(chip->usb_psy)) {
>>  +		dev_err(chip->dev, "Failed to register USB power supply: %pe\n",
>>  +			chip->usb_psy);
>>  +		return PTR_ERR(chip->usb_psy);
>>  +	}
>>  +
>>  +	ret = power_supply_get_battery_info(chip->usb_psy, 
>> &chip->batt_info);
>>  +	if (ret) {
>>  +		dev_err(chip->dev, "Failed to get battery info: %pe\n",
>>  +			ERR_PTR(ret));
>>  +		return ret;
>>  +	}
>>  +
>>  +	if (chip->batt_info->voltage_max_design_uv == -EINVAL) {
>>  +		dev_err(chip->dev,
>>  +			"Battery info missing maximum design voltage\n");
>>  +		return -EINVAL;
>>  +	}
>>  +
>>  +	if (chip->batt_info->constant_charge_current_max_ua == -EINVAL) {
>>  +		dev_err(chip->dev,
>>  +			"Battery info missing maximum constant charge current\n");
>>  +		return -EINVAL;
>>  +	}
>>  +
>>  +	/* Initialize extcon */
>>  +	chip->edev = devm_extcon_dev_allocate(chip->dev, 
>> smbchg_extcon_cable);
>>  +	if (IS_ERR(chip->edev)) {
>>  +		dev_err(chip->dev, "Failed to allocate extcon device: %pe\n",
>>  +			chip->edev);
>>  +		return PTR_ERR(chip->edev);
>>  +	}
>>  +
>>  +	ret = devm_extcon_dev_register(chip->dev, chip->edev);
>>  +	if (ret) {
>>  +		dev_err(chip->dev, "Failed to register extcon device: %pe\n",
>>  +			ERR_PTR(ret));
>>  +		return ret;
>>  +	}
>>  +
>>  +	extcon_set_property_capability(chip->edev, EXTCON_USB,
>>  +				       EXTCON_PROP_USB_VBUS);
>>  +	extcon_set_property_capability(chip->edev, EXTCON_USB_HOST,
>>  +				       EXTCON_PROP_USB_VBUS);
>>  +
>>  +	/* Initialize charger */
>>  +	ret = smbchg_init(chip);
>>  +	if (ret)
>>  +		return ret;
>>  +
>>  +	/* Request IRQs */
>>  +	for (i = 0; i < ARRAY_SIZE(smbchg_irqs); ++i) {
>>  +		/* IRQ unused */
>>  +		if (!smbchg_irqs[i].handler)
>>  +			continue;
>>  +
>>  +		irq = of_irq_get_byname(pdev->dev.of_node, smbchg_irqs[i].name);
>>  +		if (irq < 0) {
>>  +			dev_err(chip->dev, "Failed to get %s IRQ: %pe\n",
>>  +				smbchg_irqs[i].name, ERR_PTR(irq));
>>  +			return irq;
>>  +		}
>>  +
>>  +		ret = devm_request_threaded_irq(chip->dev, irq, NULL,
>>  +						smbchg_irqs[i].handler,
>>  +						IRQF_ONESHOT,
>>  +						smbchg_irqs[i].name, chip);
>>  +		if (ret) {
>>  +			dev_err(chip->dev, "failed to request %s IRQ: %pe\n",
>>  +				smbchg_irqs[i].name, ERR_PTR(irq));
>>  +			return ret;
>>  +		}
>>  +	}
>>  +
>>  +	platform_set_drvdata(pdev, chip);
>>  +
>>  +	return 0;
>>  +}
>>  +
>>  +static int smbchg_remove(struct platform_device *pdev)
>>  +{
>>  +	struct smbchg_chip *chip = platform_get_drvdata(pdev);
>>  +
>>  +	smbchg_usb_enable(chip, false);
>>  +	smbchg_charging_enable(chip, false);
>>  +
>>  +	return 0;
>>  +}
>>  +
>>  +static const struct of_device_id smbchg_id_table[] = {
>>  +	{ .compatible = "qcom,pmi8994-smbchg", .data = 
>> &smbchg_pmi8994_data },
>>  +	{ .compatible = "qcom,pmi8996-smbchg", .data = 
>> &smbchg_pmi8996_data },
>>  +	{}
>>  +};
>>  +MODULE_DEVICE_TABLE(of, smbchg_id_table);
>>  +
>>  +static struct platform_driver smbchg_driver = {
>>  +	.probe = smbchg_probe,
>>  +	.remove = smbchg_remove,
>>  +	.driver = {
>>  +		.name = "qcom-smbchg",
>>  +		.of_match_table = smbchg_id_table,
>>  +	},
>>  +};
>>  +module_platform_driver(smbchg_driver);
>>  +
>>  +MODULE_AUTHOR("Yassine Oudjana <y.oudjana@protonmail.com>");
>>  +MODULE_AUTHOR("Alejandro Tafalla <atafalla@dnyon.com>");
>>  +MODULE_DESCRIPTION("Qualcomm PMIC switch-mode battery charger 
>> driver");
>>  +MODULE_LICENSE("GPL");
>>  diff --git a/drivers/power/supply/qcom-smbchg.h 
>> b/drivers/power/supply/qcom-smbchg.h
>>  new file mode 100644
>>  index 000000000000..06445e10b4db
>>  --- /dev/null
>>  +++ b/drivers/power/supply/qcom-smbchg.h
>>  @@ -0,0 +1,428 @@
>>  +/* SPDX-License-Identifier: GPL-2.0-only */
>>  +
>>  +/* Registers */
>>  +/* CHGR */
>>  +#define SMBCHG_CHGR_FV_STS			0x00c
>>  +#define SMBCHG_CHGR_STS				0x00e
>>  +#define SMBCHG_CHGR_RT_STS			0x010
>>  +#define SMBCHG_CHGR_FCC_CFG			0x0f2
>>  +#define SMBCHG_CHGR_FCC_CMP_CFG			0x0f3
>>  +#define SMBCHG_CHGR_FV_CFG			0x0f4
>>  +#define SMBCHG_CHGR_FV_CMP			0x0f5
>>  +#define SMBCHG_CHGR_AFVC_CFG			0x0f6
>>  +#define SMBCHG_CHGR_CHG_INHIB_CFG		0x0f7
>>  +#define SMBCHG_CHGR_TCC_CFG			0x0f9
>>  +#define SMBCHG_CHGR_CCMP_CFG			0x0fa
>>  +#define SMBCHG_CHGR_CHGR_CFG1			0x0fb
>>  +#define SMBCHG_CHGR_CHGR_CFG2			0x0fc
>>  +#define SMBCHG_CHGR_SFT_CFG			0x0fd
>>  +#define SMBCHG_CHGR_CFG				0x0ff
>>  +
>>  +/* OTG */
>>  +#define SMBCHG_OTG_RT_STS			0x110
>>  +#define SMBCHG_OTG_OTG_CFG			0x1f1
>>  +#define SMBCHG_OTG_TRIM6			0x1f6
>>  +#define SMBCHG_OTG_LOW_PWR_OPTIONS		0x1ff
>>  +
>>  +/* BAT-IF */
>>  +#define SMBCHG_BAT_IF_BAT_PRES_STS		0x208
>>  +#define SMBCHG_BAT_IF_RT_STS			0x210
>>  +#define SMBCHG_BAT_IF_CMD_CHG			0x242
>>  +#define SMBCHG_BAT_IF_CMD_CHG_LED		0x243
>>  +#define SMBCHG_BAT_IF_BM_CFG			0x2f3
>>  +#define SMBCHG_BAT_IF_BAT_IF_TRIM7		0x2f7
>>  +#define SMBCHG_BAT_IF_BB_CLMP_SEL		0x2f8
>>  +#define SMBCHG_BAT_IF_ZIN_ICL_PT		0x2fc
>>  +#define SMBCHG_BAT_IF_ZIN_ICL_LV		0x2fd
>>  +#define SMBCHG_BAT_IF_ZIN_ICL_HV		0x2fe
>>  +
>>  +/* USB-CHGPTH */
>>  +#define SMBCHG_USB_CHGPTH_ICL_STS_1		0x307
>>  +#define SMBCHG_USB_CHGPTH_PWR_PATH		0x308
>>  +#define SMBCHG_USB_CHGPTH_ICL_STS_2		0x309
>>  +#define SMBCHG_USB_CHGPTH_RID_STS		0x30b
>>  +#define SMBCHG_USB_CHGPTH_USBIN_HVDCP_STS	0x30c
>>  +#define SMBCHG_USB_CHGPTH_INPUT_STS		0x30d
>>  +#define SMBCHG_USB_CHGPTH_USBID_MSB		0x30e
>>  +#define SMBCHG_USB_CHGPTH_RT_STS		0x310
>>  +#define SMBCHG_USB_CHGPTH_CMD_IL		0x340
>>  +#define SMBCHG_USB_CHGPTH_CMD_APSD		0x341
>>  +#define SMBCHG_USB_CHGPTH_CMD_HVDCP_1		0x342
>>  +#define SMBCHG_USB_CHGPTH_CMD_HVDCP_2		0x343
>>  +#define SMBCHG_USB_CHGPTH_USBIN_CHGR_CFG	0x3f1
>>  +#define SMBCHG_USB_CHGPTH_IL_CFG		0x3f2
>>  +#define SMBCHG_USB_CHGPTH_AICL_CFG		0x3f3
>>  +#define SMBCHG_USB_CHGPTH_CFG			0x3f4
>>  +#define SMBCHG_USB_CHGPTH_APSD_CFG		0x3f5
>>  +#define SMBCHG_USB_CHGPTH_TR_RID		0x3fa
>>  +#define SMBCHG_USB_CHGPTH_ICL_BUF_CONFIG	0x3fc
>>  +#define SMBCHG_USB_CHGPTH_TR_8OR32B		0x3fe
>>  +
>>  +/* DC-CHGPTH */
>>  +#define SMBCHG_DC_CHGPTH_RT_STS			0x410
>>  +#define SMBCHG_DC_CHGPTH_IL_CFG			0x4f2
>>  +#define SMBCHG_DC_CHGPTH_AICL_CFG		0x4f3
>>  +#define SMBCHG_DC_CHGPTH_AICL_WL_SEL_CFG	0x4f5
>>  +
>>  +/* MISC */
>>  +#define SMBCHG_MISC_REVISION1			0x600
>>  +#define SMBCHG_MISC_IDEV_STS			0x608
>>  +#define SMBCHG_MISC_RT_STS			0x610
>>  +#define SMBCHG_MISC_TEST			0x6e2
>>  +#define SMBCHG_MISC_NTC_VOUT_CFG		0x6f3
>>  +#define SMBCHG_MISC_TRIM_OPT_15_8		0x6f5
>>  +#define SMBCHG_MISC_TRIM_OPT_7_0		0x6f6
>>  +#define SMBCHG_MISC_TRIM_14			0x6fe
>>  +
>>  +/* Bits */
>>  +/* CHGR_STS bits */
>>  +#define CHG_TYPE_MASK			GENMASK(2, 1)
>>  +#define CHG_HOLD_OFF_BIT		BIT(3)\
>>  +
>>  +/* CHGR_FCC_CFG bits */
>>  +#define FCC_MASK			GENMASK(4, 0)
>>  +
>>  +/* CHGR_RT_STS bits */
>>  +#define CHG_INHIBIT_BIT			BIT(1)
>>  +#define CHG_COMP_SFT_BIT		BIT(3)
>>  +#define BAT_TCC_REACHED_BIT		BIT(7)
>>  +
>>  +/* CHGR_FV_CFG bits */
>>  +#define FV_MASK				GENMASK(5, 0)
>>  +
>>  +/* CHGR_TCC_CFG bits */
>>  +#define CHG_ITERM_MASK			GENMASK(2, 0)
>>  +
>>  +/* CHGR_CHGR_CFG1 bits */
>>  +#define RECHG_THRESHOLD_SRC_BIT		BIT(1)
>>  +#define TERM_I_SRC_BIT			BIT(2)
>>  +
>>  +/* CHGR_CHGR_CFG2 bits */
>>  +#define CHARGER_INHIBIT_BIT		BIT(0)
>>  +#define AUTO_RECHG_BIT			BIT(2)
>>  +#define I_TERM_BIT			BIT(3)
>>  +#define P2F_CHG_TRAN_BIT		BIT(5)
>>  +#define CHG_EN_POLARITY_BIT		BIT(6)
>>  +#define CHG_EN_SRC_BIT			BIT(7)
>>  +
>>  +/* CHGR_CFG bits */
>>  +#define RCHG_LVL_BIT			BIT(0)
>>  +
>>  +/* BAT_IF_CMD_CHG bits */
>>  +#define OTG_EN_BIT			BIT(0)
>>  +#define CHG_EN_BIT			BIT(1)
>>  +
>>  +/* BAT_IF_RT_STS bits */
>>  +#define HOT_BAT_HARD_BIT		BIT(0)
>>  +#define HOT_BAT_SOFT_BIT		BIT(1)
>>  +#define COLD_BAT_HARD_BIT		BIT(2)
>>  +#define COLD_BAT_SOFT_BIT		BIT(3)
>>  +#define BAT_OV_BIT			BIT(4)
>>  +#define BAT_LOW_BIT			BIT(5)
>>  +#define BAT_MISSING_BIT			BIT(6)
>>  +#define BAT_TERM_MISSING_BIT		BIT(7)
>>  +
>>  +/* USB_CHGPTH_ICL_STS_1 bits */
>>  +#define ICL_STS_MASK			GENMASK(4, 0)
>>  +#define AICL_STS_BIT			BIT(5)
>>  +#define AICL_SUSP_BIT			BIT(6)
>>  +
>>  +/* USB_CHGPTH_CFG bits */
>>  +#define USB51AC_CTRL			BIT(1)
>>  +#define USB51_COMMAND_POL		BIT(2)
>>  +#define CFG_USB3P0_SEL_BIT		BIT(7)
>>  +
>>  +/* USB_CHGPTH_RT_STS bits */
>>  +#define USBIN_OV_BIT			BIT(1)
>>  +#define USBIN_SRC_DET_BIT		BIT(2)
>>  +
>>  +/* USB_CHGPTH_RID_STS bits */
>>  +#define RID_MASK			GENMASK(3, 0)
>>  +
>>  +/* USB_CHGPTH_CMD_IL bits */
>>  +#define USBIN_MODE_HC_BIT		BIT(0)
>>  +#define USB51_MODE_BIT			BIT(1)
>>  +#define ICL_OVERRIDE_BIT		BIT(2)
>>  +#define USBIN_SUSPEND_BIT		BIT(4)
>>  +
>>  +/* USB_CHGPTH_IL_CFG bits */
>>  +#define USBIN_INPUT_MASK		GENMASK(4, 0)
>>  +
>>  +/* USB_CHGPTH_AICL_CFG bits */
>>  +#define USB_CHGPTH_AICL_EN		BIT(2)
>>  +
>>  +/* USB_CHGPTH_APSD_CFG bits */
>>  +#define USB_CHGPTH_APSD_EN		BIT(0)
>>  +
>>  +/* MISC_IDEV_STS bits */
>>  +#define FMB_STS_MASK			GENMASK(3, 0)
>>  +
>>  +/* MISC_TRIM_OPT_15_8 bits */
>>  +#define AICL_RERUN_MASK			GENMASK(5, 4)
>>  +#define AICL_RERUN_DC_BIT		BIT(4)
>>  +#define AICL_RERUN_USB_BIT		BIT(5)
>>  +
>>  +/* Values */
>>  +/* CHGR_STS values */
>>  +#define CHG_TYPE_SHIFT			1
>>  +#define BATT_NOT_CHG_VAL		0x0
>>  +#define BATT_PRE_CHG_VAL		0x1
>>  +#define BATT_FAST_CHG_VAL		0x2
>>  +#define BATT_TAPER_CHG_VAL		0x3
>>  +
>>  +/* CHGR_CHGR_CFG1 values */
>>  +#define	RCHG_SRC_ANA			0
>>  +#define RCHG_SRC_FG			BIT(1)
>>  +#define TERM_SRC_ANA			0
>>  +#define TERM_SRC_FG			BIT(2)
>>  +
>>  +/* CHGR_CHGR_CFG2 values */
>>  +#define CHG_INHIBIT_DIS			0
>>  +#define CHG_INHIBIT_EN			BIT(0)
>>  +#define AUTO_RCHG_EN			0
>>  +#define AUTO_RCHG_DIS			BIT(2)
>>  +#define CURRENT_TERM_EN			0
>>  +#define CURRENT_TERM_DIS		BIT(3)
>>  +#define PRE_FAST_AUTO			0
>>  +#define PRE_FAST_CMD			BIT(5)
>>  +#define CHG_EN_SRC_CMD			0
>>  +#define CHG_EN_SRC_PIN			BIT(7)
>>  +
>>  +/* CHGR_CFG values */
>>  +#define RCHG_THRESH_100MV		0
>>  +#define RCHG_THRESH_200MV		BIT(0)
>>  +
>>  +/* USB_CHGPTH_INPUT_STS values */
>>  +#define USBIN_9V			BIT(5)
>>  +#define USBIN_UNREG			BIT(4)
>>  +#define USBIN_LV			BIT(3)
>>  +
>>  +/* USB_CHGPTH_USBID_MSB values */
>>  +#define USBID_GND_THRESHOLD		0x495
>>  +
>>  +/* USB_CHGPTH_CFG values */
>>  +#define USB51_COMMAND_CONTROL		0
>>  +#define USB51_PIN_CONTROL		BIT(1)
>>  +#define USB51AC_COMMAND1_500		0
>>  +#define USB51AC_COMMAND1_100		BIT(2)
>>  +#define USB_2P0_SEL			0
>>  +#define USB_3P0_SEL			BIT(7)
>>  +
>>  +/* MISC_IDEV_STS values */
>>  +#define USB_TYPE_SDP_BIT		BIT(4)
>>  +#define USB_TYPE_OTHER_BIT		BIT(5)
>>  +#define USB_TYPE_DCP_BIT		BIT(6)
>>  +#define USB_TYPE_CDP_BIT		BIT(7)
>>  +
>>  +/* Constant parameters */
>>  +#define NUM_OTG_RESET_RETRIES		5
>>  +#define OTG_RESET_DELAY_MS		20
>>  +
>>  +/*
>>  + * These values can be indexed using a bitmask:
>>  + * 0: USB 2.0/3.0
>>  + * 1: Limited/full current
>>  + */
>>  +static const int smbchg_lc_ilim_options[] = {
>>  +	[0b00] = 100000,
>>  +	[0b01] = 150000,
>>  +	[0b10] = 500000,
>>  +	[0b11] = 900000
>>  +};
>>  +#define LC_ILIM_USB3_BIT		BIT(0)
>>  +#define LC_ILIM_FULL_CURRENT_BIT	BIT(1)
>>  +#define smbchg_lc_ilim(usb_3, full_current) 
>> smbchg_lc_ilim_options[usb_3 | full_current << 1]
>>  +
>>  +struct smbchg_chip {
>>  +	unsigned int base;
>>  +	struct device *dev;
>>  +	struct regmap *regmap;
>>  +
>>  +	struct power_supply_battery_info *batt_info;
>>  +	struct power_supply *usb_psy;
>>  +
>>  +	struct regulator_desc otg_rdesc;
>>  +	struct regulator_dev *otg_reg;
>>  +	int otg_resets;
>>  +
>>  +	struct extcon_dev *edev;
>>  +
>>  +	spinlock_t sec_access_lock;
>>  +	struct work_struct otg_reset_work;
>>  +
>>  +	const struct smbchg_data *data;
>>  +};
>>  +
>>  +struct smbchg_irq {
>>  +	const char *name;
>>  +	irq_handler_t handler;
>>  +};
>>  +
>>  +struct smbchg_data {
>>  +	const int *iterm_table;
>>  +	unsigned int iterm_table_len;
>>  +	const int *ilim_table;
>>  +	unsigned int ilim_table_len;
>>  +
>>  +	bool reset_otg_on_oc;
>>  +};
>>  +
>>  +static const int smbchg_fv_table[] = {
> 
> No static data in headers.

Understood.

> 
> Best regards,
> Krzysztof



^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 0/8] power: supply: Add driver for Qualcomm SMBCHG
  2022-08-08  9:39   ` Yassine Oudjana
@ 2022-08-08 13:24     ` Caleb Connolly
  0 siblings, 0 replies; 23+ messages in thread
From: Caleb Connolly @ 2022-08-08 13:24 UTC (permalink / raw)
  To: Yassine Oudjana, Krzysztof Kozlowski
  Cc: Sebastian Reichel, Rob Herring, Krzysztof Kozlowski, Andy Gross,
	Bjorn Andersson, Yassine Oudjana, Alejandro Tafalla,
	Konrad Dybcio, linux-pm, linux-arm-msm, devicetree, phone-devel,
	linux-kernel



On 08/08/2022 10:39, Yassine Oudjana wrote:
> 
> On Mon, Aug 8 2022 at 11:41:26 +03:00:00, Krzysztof Kozlowski 
> <krzysztof.kozlowski@linaro.org> wrote:
>> On 08/08/2022 10:34, Yassine Oudjana wrote:
>>>  From: Yassine Oudjana <y.oudjana@protonmail.com>
>>>
>>>  This series adds a driver for the switch-mode battery charger found on PMICs
>>>  such as PMI8994, and referred to in the vendor kernel[1] as smbcharger or
>>>  SMBCHG. More details on this block can be found in the last patch message.
>>>
>>>  This driver currently supports the charger blocks of PMI8994 and PMI8996.
>>>  PMI8950 was also to be supported, but it was dropped due to some last minute
>>>  issues, to be brought back at a later time once ready.
>>>
>>>  The OTG regulator remains unused on devices where the charger is enabled in
>>>  this series due to lack of a consumer. Applying a patch[2] adding vbus-supply
>>>  to DWC3 allows it to enable the OTG regulator making USB host without
>>>  external power possible.
>>>
>>>  [1] 
>>> https://github.com/android-linux-stable/msm-3.18/blob/kernel.lnx.3.18.r34-rel/drivers/power/qpnp-smbcharger.c 
>>>
>>>  [2] 
>>> https://lore.kernel.org/linux-usb/20200805061744.20404-1-mike.looijmans@topic.nl/ 
>>>
>>
>> How is it different from PMI8998? I expect not that much, so this should
>> be based on existing work:
>> https://lore.kernel.org/linux-arm-msm/20220706194125.1861256-1-caleb.connolly@linaro.org/ 
>>
>>
>> Unless they are different, but then please create common parts and
>> explain the differences.
>>
>> Best regards,
>> Krzysztof
> 
> This driver has been in slow developement for a long time before that one 
> existed, which was why no initial attempt at a common driver was made. With that 
> said however, I've been watching its development even before it was sent for 
> review, and It seems that the hardware is actually quite different. For example, 
> the original charger entirely lacks the type-c functionality that exists on the 
> second gen one. There are a couple of similar registers like CMD_APSD (same 
> address and function) CHGR_CFG2 (same/similar function, different address), but 
> other than that there don't seem to be any major similarities. While I guess it 
> would technically be possible to force them into one driver with multiple 
> register tables and separate functions for most tasks, I think it would just 
> unnecessarily complicate things. One thing that is common however is the secure 
> register unlock sequence, which I have separated in patch 6 to allow for its use 
> in other drivers (the fuel gauge block has secure registers too so it will also 
> be used in an upcoming fuel gauge driver).

Yes, we took the shared approach for the still work in progress fuel gauge 
driver, and whilst there are more similarities in that block for basic 
functionality at least, more complicated components differ quite a lot as far as 
I'm aware.

Even for the fuel gauge, separate handlers are needed for a lot of things still:
https://gitlab.com/sdm845-mainline/linux/-/blob/sdm845/5.19-release/drivers/power/supply/qcom_fg.c#L792
So I don't think trying to create a common driver here is the right approach.

Perhaps some abstraction is possible for the overall similarities like handling 
the APSD, dealing with current limiting, cable detection etc, perhaps some of 
this common code could be pulled out into a shared "helper"?

Maybe this is something worth reconsidering as and when we look at adding 
support for some of the more complicated features this hardware supports.
> 
> 

-- 
Kind Regards,
Caleb (they/he)

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 8/8] power: supply: Add driver for Qualcomm SMBCHG
  2022-08-08 10:05     ` Yassine Oudjana
@ 2022-08-08 13:42       ` Krzysztof Kozlowski
  0 siblings, 0 replies; 23+ messages in thread
From: Krzysztof Kozlowski @ 2022-08-08 13:42 UTC (permalink / raw)
  To: Yassine Oudjana
  Cc: Sebastian Reichel, Rob Herring, Krzysztof Kozlowski, Andy Gross,
	Bjorn Andersson, Yassine Oudjana, Alejandro Tafalla,
	Konrad Dybcio, linux-pm, linux-arm-msm, devicetree, phone-devel,
	linux-kernel

On 08/08/2022 13:05, Yassine Oudjana wrote:
> 
> On Mon, Aug 8 2022 at 11:55:02 +03:00:00, Krzysztof Kozlowski 
> <krzysztof.kozlowski@linaro.org> wrote:
>> On 08/08/2022 10:34, Yassine Oudjana wrote:
>>>  From: Yassine Oudjana <y.oudjana@protonmail.com>
>>>
>>>  Add a driver for the switch-mode battery charger found on
>>>  PMICs such as PMI8994. This block is referred to in the vendor
>>>  kernel[1] as smbcharger or SMBCHG. It has USB and DC inputs,
>>>  and can generate VBUS for USB OTG with a boost regulator.
>>>  It supports Qualcomm Quick Charge 2.0, and can operate along
>>>  with a parallel charger (SMB1357, or SMB1351 for added Quick
>>>  Charge 3.0 support) for improved efficiency at higher currents.
>>>
>>>  At the moment, this driver supports charging off of the USB input
>>>  at 5V with input current limit up to 3A. It also includes support
>>>  for operating the OTG boost regulator as well as extcon
>>>  functionality, reporting states of USB and USB_HOST with VBUS and
>>>  charge port types.
>>>
>>>  Co-developed-by: Alejandro Tafalla <atafalla@dnyon.com>
>>>  Signed-off-by: Alejandro Tafalla <atafalla@dnyon.com>
>>>  Signed-off-by: Yassine Oudjana <y.oudjana@protonmail.com>
>>>
>>>  [1] 
>>> https://github.com/android-linux-stable/msm-3.18/blob/kernel.lnx.3.18.r34-rel/drivers/power/qpnp-smbcharger.c
>>>  ---
>>>   MAINTAINERS                        |    2 +
>>>   drivers/power/supply/Kconfig       |   11 +
>>>   drivers/power/supply/Makefile      |    1 +
>>>   drivers/power/supply/qcom-smbchg.c | 1664 
>>> ++++++++++++++++++++++++++++
>>>   drivers/power/supply/qcom-smbchg.h |  428 +++++++
>>>   5 files changed, 2106 insertions(+)
>>>   create mode 100644 drivers/power/supply/qcom-smbchg.c
>>>   create mode 100644 drivers/power/supply/qcom-smbchg.h
>>>
>>>  diff --git a/MAINTAINERS b/MAINTAINERS
>>>  index f6cf3a27d132..9b8693050890 100644
>>>  --- a/MAINTAINERS
>>>  +++ b/MAINTAINERS
>>>  @@ -16964,6 +16964,8 @@ L:	linux-pm@vger.kernel.org
>>>   L:	linux-arm-msm@vger.kernel.org
>>>   S:	Maintained
>>>   F:	Documentation/devicetree/bindings/power/supply/qcom,smbchg.yaml
>>>  +F:	drivers/power/supply/qcom-smbchg.c
>>>  +F:	drivers/power/supply/qcom-smbchg.h
>>>
>>>   QUALCOMM TSENS THERMAL DRIVER
>>>   M:	Amit Kucheria <amitk@kernel.org>
>>>  diff --git a/drivers/power/supply/Kconfig 
>>> b/drivers/power/supply/Kconfig
>>>  index 1aa8323ad9f6..246bfc118d9f 100644
>>>  --- a/drivers/power/supply/Kconfig
>>>  +++ b/drivers/power/supply/Kconfig
>>>  @@ -633,6 +633,17 @@ config CHARGER_QCOM_SMBB
>>>   	  documentation for more detail.  The base name for this driver is
>>>   	  'pm8941_charger'.
>>>
>>>  +config CHARGER_QCOM_SMBCHG
>>>  +	tristate "Qualcomm Switch-Mode Battery Charger"
>>
>> As I mentioned in cover letter, this should be either squashed into
>> Caleb's work, merged into some common part or kept separate but with
>> clear explaining why it cannot be merged.
>>
>> Some incomplete review follows:
>>
>>>  +	depends on MFD_SPMI_PMIC || COMPILE_TEST
>>>  +	depends on OF
>>>  +	depends on EXTCON
>>>  +	depends on REGULATOR
>>>  +	select QCOM_PMIC_SEC_WRITE
>>>  +	help
>>>  +	  Say Y to include support for the Switch-Mode Battery Charger 
>>> block
>>>  +	  found in Qualcomm PMICs such as PMI8994.
>>>  +
>>>   config CHARGER_BQ2415X
>>>   	tristate "TI BQ2415x battery charger driver"
>>>   	depends on I2C
>>>  diff --git a/drivers/power/supply/Makefile 
>>> b/drivers/power/supply/Makefile
>>>  index 7f02f36aea55..7c2c037cd8b1 100644
>>>  --- a/drivers/power/supply/Makefile
>>>  +++ b/drivers/power/supply/Makefile
>>>  @@ -83,6 +83,7 @@ obj-$(CONFIG_CHARGER_MAX8998)	+= max8998_charger.o
>>>   obj-$(CONFIG_CHARGER_MP2629)	+= mp2629_charger.o
>>>   obj-$(CONFIG_CHARGER_MT6360)	+= mt6360_charger.o
>>>   obj-$(CONFIG_CHARGER_QCOM_SMBB)	+= qcom_smbb.o
>>>  +obj-$(CONFIG_CHARGER_QCOM_SMBCHG)	+= qcom-smbchg.o
>>>   obj-$(CONFIG_CHARGER_BQ2415X)	+= bq2415x_charger.o
>>>   obj-$(CONFIG_CHARGER_BQ24190)	+= bq24190_charger.o
>>>   obj-$(CONFIG_CHARGER_BQ24257)	+= bq24257_charger.o
>>>  diff --git a/drivers/power/supply/qcom-smbchg.c 
>>> b/drivers/power/supply/qcom-smbchg.c
>>>  new file mode 100644
>>>  index 000000000000..23a9667953c9
>>>  --- /dev/null
>>>  +++ b/drivers/power/supply/qcom-smbchg.c
>>>  @@ -0,0 +1,1664 @@
>>>  +// SPDX-License-Identifier: GPL-2.0-only
>>
>> Several things look like based from original sources, so please retain
>> original copyright.
> 
> Do I replace the existing copyright here with the original one, just 
> add it, or mention that this driver is based on downstream sources 
> (maybe putting a link as well) then add it?

Add original copyright and optionally mention that it is based on
downstream source. Links are not needed.

>>
>>>  +
>>>  +static int smbchg_probe(struct platform_device *pdev)
>>>  +{
>>>  +	struct smbchg_chip *chip;
>>>  +	struct regulator_config config = {};
>>>  +	struct power_supply_config supply_config = {};
>>>  +	int i, irq, ret;
>>>  +
>>>  +	chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL);
>>>  +	if (!chip)
>>>  +		return -ENOMEM;
>>>  +
>>>  +	chip->dev = &pdev->dev;
>>>  +
>>>  +	chip->regmap = dev_get_regmap(chip->dev->parent, NULL);
>>>  +	if (!chip->regmap) {
>>>  +		dev_err(chip->dev, "Failed to get regmap\n");
>>>  +		return -ENODEV;
>>>  +	}
>>>  +
>>>  +	ret = of_property_read_u32(chip->dev->of_node, "reg", 
>>> &chip->base);
>>
>> First: device_xxx
> 
> Okay.
> 
>> Second: what if address is bigger than u32? Shouldn't this be
>> of_read_number or something similar in device_xxx API?
> 
> The address wouldn't exceed sizeof(u16). Actually now I think I 
> should've used property_read_u16 instead. I couldn't find a device_* 
> equivalent of of_read_number (or at least not a direct one), are you 
> sure it exists?

I think u16 would be confusing as reg size is minimum u32 (with
address-cells==1). Instead of of_read_number(), maybe this should be
of_get_address() (see pm8941-pwrkey.c), but there is no device_xxx()
equivalent. Still I think it would be the most appropriate to parse
actual address.



Best regards,
Krzysztof

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 1/8] dt-bindings: power: supply: Add DT schema for Qualcomm SMBCHG
  2022-08-08  8:42   ` Krzysztof Kozlowski
@ 2022-11-20 15:46     ` Yassine Oudjana
  2022-11-21  8:26       ` Krzysztof Kozlowski
  0 siblings, 1 reply; 23+ messages in thread
From: Yassine Oudjana @ 2022-11-20 15:46 UTC (permalink / raw)
  To: Krzysztof Kozlowski
  Cc: Sebastian Reichel, Rob Herring, Krzysztof Kozlowski, Andy Gross,
	Bjorn Andersson, Alejandro Tafalla, Konrad Dybcio, linux-pm,
	linux-arm-msm, devicetree, phone-devel, linux-kernel,
	Yassine Oudjana, Yassine Oudjana

On Mon, 8 Aug 2022 11:42:34 +0300, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
> On 08/08/2022 10:34, Yassine Oudjana wrote:
> > From: Yassine Oudjana <y.oudjana@protonmail.com>
> > 
> > Add DT schema for the switch-mode battery charger found on Qualcomm
> > PMICs such as PMI8994. Due to lack of documentation, some interrupt
> > descriptions might be inaccurate.
> > 
> > Signed-off-by: Yassine Oudjana <y.oudjana@protonmail.com>
> > ---
> >  .../bindings/power/supply/qcom,smbchg.yaml    | 205 ++++++++++++++++++
> >  MAINTAINERS                                   |   8 +
> >  2 files changed, 213 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/power/supply/qcom,smbchg.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/power/supply/qcom,smbchg.yaml b/Documentation/devicetree/bindings/power/supply/qcom,smbchg.yaml
> > new file mode 100644
> > index 000000000000..d825a9c10b3e
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/power/supply/qcom,smbchg.yaml
> > @@ -0,0 +1,205 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only or BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/power/supply/qcom,smbchg.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Qualcomm PMIC Switch-Mode Battery Charger
> > +
> > +maintainers:
> > +  - Yassine Oudjana <y.oudjana@protonmail.com>
> > +  - Alejandro Tafalla <atafalla@dnyon.com>
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - qcom,pmi8994-smbchg
> > +      - qcom,pmi8996-smbchg
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  monitored-battery:
> > +    description: |
> > +      phandle of battery characteristics node.
> > +      The charger uses the following properties:
> > +      - charge-term-current-microamp
> > +      - constant-charge-current-max-microamp
> > +      - voltage-max-design-microvolt
> > +      The constant-charge-current-max-microamp and voltage-max-design-microvolt
> > +      properties must be set.
> > +      See Documentation/devicetree/bindings/power/supply/battery.yaml
> > +
> > +  interrupts:
> > +    items:
> > +      - description: Charger error
> > +      - description: Charger inhibited
> > +      - description: Charger precharge safety timer timeout
> > +      - description: Charger charge safety timer timeout
> > +      - description: Charger pre to fast charging switch threshold reached
> > +      - description: Charger recharge threshold reached
> > +      - description: Charger taper threshold reached
> > +      - description: Charger charge termination threshold reached
> > +      - description: Battery hot
> > +      - description: Battery warm
> > +      - description: Battery cold
> > +      - description: Battery cool
> > +      - description: Battery overvoltage
> > +      - description: Battery low
> > +      - description: Battery missing
> > +      - description: Battery thermistor missing # unconfirmed
> > +      - description: USB input undervolt
> > +      - description: USB input overvolt
> > +      - description: USB input source detected
> > +      - description: OTG regulator failure
> > +      - description: OTG regulator overcurrent
> > +      - description: Automatic input current limiting done
> > +      - description: USB ID pin changed
> > +      - description: DC input undervolt
> > +      - description: DC input overvolt
> > +      - description: Power OK
> > +      - description: Temperature shutdown
> > +      - description: Watchdog timeout
> > +      - description: Flash failure
> > +      - description: OTST2 # unknown
> > +      - description: OTST3 # unknown
> 
> It seems you listed register interrupts, not physical pins. This should
> be interrupt lines.

I'm not sure what I'm supposed to do here. I couldn't find an interrupt-lines
property used anywhere so that's not what you meant, right?


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 1/8] dt-bindings: power: supply: Add DT schema for Qualcomm SMBCHG
  2022-11-20 15:46     ` Yassine Oudjana
@ 2022-11-21  8:26       ` Krzysztof Kozlowski
  2022-11-21 10:36         ` Yassine Oudjana
  0 siblings, 1 reply; 23+ messages in thread
From: Krzysztof Kozlowski @ 2022-11-21  8:26 UTC (permalink / raw)
  To: Yassine Oudjana
  Cc: Sebastian Reichel, Rob Herring, Krzysztof Kozlowski, Andy Gross,
	Bjorn Andersson, Alejandro Tafalla, Konrad Dybcio, linux-pm,
	linux-arm-msm, devicetree, phone-devel, linux-kernel,
	Yassine Oudjana

On 20/11/2022 16:46, Yassine Oudjana wrote:
>>> +  interrupts:
>>> +    items:
>>> +      - description: Charger error
>>> +      - description: Charger inhibited
>>> +      - description: Charger precharge safety timer timeout
>>> +      - description: Charger charge safety timer timeout
>>> +      - description: Charger pre to fast charging switch threshold reached
>>> +      - description: Charger recharge threshold reached
>>> +      - description: Charger taper threshold reached
>>> +      - description: Charger charge termination threshold reached
>>> +      - description: Battery hot
>>> +      - description: Battery warm
>>> +      - description: Battery cold
>>> +      - description: Battery cool
>>> +      - description: Battery overvoltage
>>> +      - description: Battery low
>>> +      - description: Battery missing
>>> +      - description: Battery thermistor missing # unconfirmed
>>> +      - description: USB input undervolt
>>> +      - description: USB input overvolt
>>> +      - description: USB input source detected
>>> +      - description: OTG regulator failure
>>> +      - description: OTG regulator overcurrent
>>> +      - description: Automatic input current limiting done
>>> +      - description: USB ID pin changed
>>> +      - description: DC input undervolt
>>> +      - description: DC input overvolt
>>> +      - description: Power OK
>>> +      - description: Temperature shutdown
>>> +      - description: Watchdog timeout
>>> +      - description: Flash failure
>>> +      - description: OTST2 # unknown
>>> +      - description: OTST3 # unknown
>>
>> It seems you listed register interrupts, not physical pins. This should
>> be interrupt lines.
> 
> I'm not sure what I'm supposed to do here. I couldn't find an interrupt-lines
> property used anywhere so that's not what you meant, right?

Are these physical interrupt lines this device has, register offsets or
virtual interrupts (e.g. passed via irq_chip)? Definitely not the first
and rather offsets for qpnpint_irq_domain_translate. Devicetree is not
for describing register layout of devices. IOW, register layout does not
change on different boards, because the device is exactly the same, so
there is no point to put it into DTS.

Best regards,
Krzysztof


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 1/8] dt-bindings: power: supply: Add DT schema for Qualcomm SMBCHG
  2022-11-21  8:26       ` Krzysztof Kozlowski
@ 2022-11-21 10:36         ` Yassine Oudjana
  2022-11-21 17:07           ` Krzysztof Kozlowski
  0 siblings, 1 reply; 23+ messages in thread
From: Yassine Oudjana @ 2022-11-21 10:36 UTC (permalink / raw)
  To: Krzysztof Kozlowski
  Cc: Sebastian Reichel, Rob Herring, Krzysztof Kozlowski, Andy Gross,
	Bjorn Andersson, Alejandro Tafalla, Konrad Dybcio, linux-pm,
	linux-arm-msm, devicetree, phone-devel, linux-kernel,
	Yassine Oudjana


On Mon, Nov 21 2022 at 09:26:59 +01:00:00, Krzysztof Kozlowski 
<krzysztof.kozlowski@linaro.org> wrote:
> On 20/11/2022 16:46, Yassine Oudjana wrote:
>>>>  +  interrupts:
>>>>  +    items:
>>>>  +      - description: Charger error
>>>>  +      - description: Charger inhibited
>>>>  +      - description: Charger precharge safety timer timeout
>>>>  +      - description: Charger charge safety timer timeout
>>>>  +      - description: Charger pre to fast charging switch 
>>>> threshold reached
>>>>  +      - description: Charger recharge threshold reached
>>>>  +      - description: Charger taper threshold reached
>>>>  +      - description: Charger charge termination threshold reached
>>>>  +      - description: Battery hot
>>>>  +      - description: Battery warm
>>>>  +      - description: Battery cold
>>>>  +      - description: Battery cool
>>>>  +      - description: Battery overvoltage
>>>>  +      - description: Battery low
>>>>  +      - description: Battery missing
>>>>  +      - description: Battery thermistor missing # unconfirmed
>>>>  +      - description: USB input undervolt
>>>>  +      - description: USB input overvolt
>>>>  +      - description: USB input source detected
>>>>  +      - description: OTG regulator failure
>>>>  +      - description: OTG regulator overcurrent
>>>>  +      - description: Automatic input current limiting done
>>>>  +      - description: USB ID pin changed
>>>>  +      - description: DC input undervolt
>>>>  +      - description: DC input overvolt
>>>>  +      - description: Power OK
>>>>  +      - description: Temperature shutdown
>>>>  +      - description: Watchdog timeout
>>>>  +      - description: Flash failure
>>>>  +      - description: OTST2 # unknown
>>>>  +      - description: OTST3 # unknown
>>> 
>>>  It seems you listed register interrupts, not physical pins. This 
>>> should
>>>  be interrupt lines.
>> 
>>  I'm not sure what I'm supposed to do here. I couldn't find an 
>> interrupt-lines
>>  property used anywhere so that's not what you meant, right?
> 
> Are these physical interrupt lines this device has, register offsets 
> or
> virtual interrupts (e.g. passed via irq_chip)? Definitely not the 
> first
> and rather offsets for qpnpint_irq_domain_translate. Devicetree is not
> for describing register layout of devices. IOW, register layout does 
> not
> change on different boards, because the device is exactly the same, so
> there is no point to put it into DTS.
> 

So how would I describe the interrupts then? Or if you are saying I 
shouldn't have these interrupts in DT at all, how would I get them and 
register handlers for them in the driver? the PMIC arbiter takes 4 
interrupt cells, 3 of which are these offsets specifying the peripheral 
and interrupt. All other PMIC peripherals currently described in DT 
(examples being qcom,pm8916-wcd-analog-codec, qcom,pm8941-pwrkey and 
qcom-wled) have their interrupts (if any) described this way, with the 
only exceptions perhaps being the GPIO and MPP controllers which are 
themselves interrupt controllers. Changing the way PMIC peripheral 
interrupts are described would require changing PMIC arbiter bindings 
and code which I believe is out of the scope of this patch series.



^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 1/8] dt-bindings: power: supply: Add DT schema for Qualcomm SMBCHG
  2022-11-21 10:36         ` Yassine Oudjana
@ 2022-11-21 17:07           ` Krzysztof Kozlowski
  2022-11-22 13:30             ` Dmitry Baryshkov
  0 siblings, 1 reply; 23+ messages in thread
From: Krzysztof Kozlowski @ 2022-11-21 17:07 UTC (permalink / raw)
  To: Yassine Oudjana
  Cc: Sebastian Reichel, Rob Herring, Krzysztof Kozlowski, Andy Gross,
	Bjorn Andersson, Alejandro Tafalla, Konrad Dybcio, linux-pm,
	linux-arm-msm, devicetree, phone-devel, linux-kernel,
	Yassine Oudjana

On 21/11/2022 11:36, Yassine Oudjana wrote:
> 
> On Mon, Nov 21 2022 at 09:26:59 +01:00:00, Krzysztof Kozlowski 
> <krzysztof.kozlowski@linaro.org> wrote:
>> On 20/11/2022 16:46, Yassine Oudjana wrote:
>>>>>  +  interrupts:
>>>>>  +    items:
>>>>>  +      - description: Charger error
>>>>>  +      - description: Charger inhibited
>>>>>  +      - description: Charger precharge safety timer timeout
>>>>>  +      - description: Charger charge safety timer timeout
>>>>>  +      - description: Charger pre to fast charging switch 
>>>>> threshold reached
>>>>>  +      - description: Charger recharge threshold reached
>>>>>  +      - description: Charger taper threshold reached
>>>>>  +      - description: Charger charge termination threshold reached
>>>>>  +      - description: Battery hot
>>>>>  +      - description: Battery warm
>>>>>  +      - description: Battery cold
>>>>>  +      - description: Battery cool
>>>>>  +      - description: Battery overvoltage
>>>>>  +      - description: Battery low
>>>>>  +      - description: Battery missing
>>>>>  +      - description: Battery thermistor missing # unconfirmed
>>>>>  +      - description: USB input undervolt
>>>>>  +      - description: USB input overvolt
>>>>>  +      - description: USB input source detected
>>>>>  +      - description: OTG regulator failure
>>>>>  +      - description: OTG regulator overcurrent
>>>>>  +      - description: Automatic input current limiting done
>>>>>  +      - description: USB ID pin changed
>>>>>  +      - description: DC input undervolt
>>>>>  +      - description: DC input overvolt
>>>>>  +      - description: Power OK
>>>>>  +      - description: Temperature shutdown
>>>>>  +      - description: Watchdog timeout
>>>>>  +      - description: Flash failure
>>>>>  +      - description: OTST2 # unknown
>>>>>  +      - description: OTST3 # unknown
>>>>
>>>>  It seems you listed register interrupts, not physical pins. This 
>>>> should
>>>>  be interrupt lines.
>>>
>>>  I'm not sure what I'm supposed to do here. I couldn't find an 
>>> interrupt-lines
>>>  property used anywhere so that's not what you meant, right?
>>
>> Are these physical interrupt lines this device has, register offsets 
>> or
>> virtual interrupts (e.g. passed via irq_chip)? Definitely not the 
>> first
>> and rather offsets for qpnpint_irq_domain_translate. Devicetree is not
>> for describing register layout of devices. IOW, register layout does 
>> not
>> change on different boards, because the device is exactly the same, so
>> there is no point to put it into DTS.
>>
> 
> So how would I describe the interrupts then? Or if you are saying I 
> shouldn't have these interrupts in DT at all, how would I get them and 
> register handlers for them in the driver? the PMIC arbiter takes 4 
> interrupt cells, 3 of which are these offsets specifying the peripheral 
> and interrupt. All other PMIC peripherals currently described in DT 
> (examples being qcom,pm8916-wcd-analog-codec, qcom,pm8941-pwrkey and 
> qcom-wled) have their interrupts (if any) described this way, with the 
> only exceptions perhaps being the GPIO and MPP controllers which are 
> themselves interrupt controllers. Changing the way PMIC peripheral 
> interrupts are described would require changing PMIC arbiter bindings 
> and code which I believe is out of the scope of this patch series.

I don't think this would touch PMIC arbiter bindings, rather the PMIC
itself. Usually complex devices (like PMICs) have one few physical
interrupt lines and many registers related to some specific interrupts.
For example:
1. One IRQ line,
2. Register with bits for overvoltage, undervoltage, vharger error etc.

Now how the MFD child device accesses them. Since this is strictly
related to hardware programming model, it's not something you put to
Devicetree. Instead parent device (PMIC) registers IRQ chip for its one
interrupt line with several Linux (or virtual) interrupts. The children
then just get a virtual IRQ from the parent (PMIC) and setup a
handler(s) for them.

You will find some examples for this, just grep for regmap_irq_get_virq,
for the drivers using regmap_irq_chip (or irq_create_mapping for
non-regmaps).

Since it is *one* device - the PMIC and its child like charger - the
register layout is fixed thus I think these virtual (or Linux)
interrupts are fixed as well.

I don't know why Qualcomm PMIC for SPMI is done differently.

Best regards,
Krzysztof


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 1/8] dt-bindings: power: supply: Add DT schema for Qualcomm SMBCHG
  2022-11-21 17:07           ` Krzysztof Kozlowski
@ 2022-11-22 13:30             ` Dmitry Baryshkov
  2022-11-28 11:39               ` Krzysztof Kozlowski
  0 siblings, 1 reply; 23+ messages in thread
From: Dmitry Baryshkov @ 2022-11-22 13:30 UTC (permalink / raw)
  To: Krzysztof Kozlowski
  Cc: Yassine Oudjana, Sebastian Reichel, Rob Herring,
	Krzysztof Kozlowski, Andy Gross, Bjorn Andersson,
	Alejandro Tafalla, Konrad Dybcio, linux-pm, linux-arm-msm,
	devicetree, phone-devel, linux-kernel, Yassine Oudjana

Hi,

On Mon, 21 Nov 2022 at 19:07, Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 21/11/2022 11:36, Yassine Oudjana wrote:
> >
> > On Mon, Nov 21 2022 at 09:26:59 +01:00:00, Krzysztof Kozlowski
> > <krzysztof.kozlowski@linaro.org> wrote:
> >> On 20/11/2022 16:46, Yassine Oudjana wrote:
> >>>>>  +  interrupts:
> >>>>>  +    items:
> >>>>>  +      - description: Charger error
> >>>>>  +      - description: Charger inhibited
> >>>>>  +      - description: Charger precharge safety timer timeout
> >>>>>  +      - description: Charger charge safety timer timeout
> >>>>>  +      - description: Charger pre to fast charging switch
> >>>>> threshold reached
> >>>>>  +      - description: Charger recharge threshold reached
> >>>>>  +      - description: Charger taper threshold reached
> >>>>>  +      - description: Charger charge termination threshold reached
> >>>>>  +      - description: Battery hot
> >>>>>  +      - description: Battery warm
> >>>>>  +      - description: Battery cold
> >>>>>  +      - description: Battery cool
> >>>>>  +      - description: Battery overvoltage
> >>>>>  +      - description: Battery low
> >>>>>  +      - description: Battery missing
> >>>>>  +      - description: Battery thermistor missing # unconfirmed
> >>>>>  +      - description: USB input undervolt
> >>>>>  +      - description: USB input overvolt
> >>>>>  +      - description: USB input source detected
> >>>>>  +      - description: OTG regulator failure
> >>>>>  +      - description: OTG regulator overcurrent
> >>>>>  +      - description: Automatic input current limiting done
> >>>>>  +      - description: USB ID pin changed
> >>>>>  +      - description: DC input undervolt
> >>>>>  +      - description: DC input overvolt
> >>>>>  +      - description: Power OK
> >>>>>  +      - description: Temperature shutdown
> >>>>>  +      - description: Watchdog timeout
> >>>>>  +      - description: Flash failure
> >>>>>  +      - description: OTST2 # unknown
> >>>>>  +      - description: OTST3 # unknown
> >>>>
> >>>>  It seems you listed register interrupts, not physical pins. This
> >>>> should
> >>>>  be interrupt lines.
> >>>
> >>>  I'm not sure what I'm supposed to do here. I couldn't find an
> >>> interrupt-lines
> >>>  property used anywhere so that's not what you meant, right?
> >>
> >> Are these physical interrupt lines this device has, register offsets
> >> or
> >> virtual interrupts (e.g. passed via irq_chip)? Definitely not the
> >> first
> >> and rather offsets for qpnpint_irq_domain_translate. Devicetree is not
> >> for describing register layout of devices. IOW, register layout does
> >> not
> >> change on different boards, because the device is exactly the same, so
> >> there is no point to put it into DTS.
> >>
> >
> > So how would I describe the interrupts then? Or if you are saying I
> > shouldn't have these interrupts in DT at all, how would I get them and
> > register handlers for them in the driver? the PMIC arbiter takes 4
> > interrupt cells, 3 of which are these offsets specifying the peripheral
> > and interrupt. All other PMIC peripherals currently described in DT
> > (examples being qcom,pm8916-wcd-analog-codec, qcom,pm8941-pwrkey and
> > qcom-wled) have their interrupts (if any) described this way, with the
> > only exceptions perhaps being the GPIO and MPP controllers which are
> > themselves interrupt controllers. Changing the way PMIC peripheral
> > interrupts are described would require changing PMIC arbiter bindings
> > and code which I believe is out of the scope of this patch series.
>
> I don't think this would touch PMIC arbiter bindings, rather the PMIC
> itself. Usually complex devices (like PMICs) have one few physical
> interrupt lines and many registers related to some specific interrupts.
> For example:
> 1. One IRQ line,
> 2. Register with bits for overvoltage, undervoltage, vharger error etc.
>
> Now how the MFD child device accesses them. Since this is strictly
> related to hardware programming model, it's not something you put to
> Devicetree. Instead parent device (PMIC) registers IRQ chip for its one
> interrupt line with several Linux (or virtual) interrupts. The children
> then just get a virtual IRQ from the parent (PMIC) and setup a
> handler(s) for them.

Unfortunately this is not how SPMI PMICs work (at least on the
Qualcomm platforms). Access to interrupt registers is handled via the
SPMI bus arbiter writes, not through the GPIO pin or typical spmi's
bus interface (in the other words, not through the PMIC's SPMI
regmap). I guess we can add an intermediate irq chip to automatically
handle the USID, etc. However I doubt that it will really bring a lot
in our case.

> You will find some examples for this, just grep for regmap_irq_get_virq,
> for the drivers using regmap_irq_chip (or irq_create_mapping for
> non-regmaps).
>
> Since it is *one* device - the PMIC and its child like charger - the
> register layout is fixed thus I think these virtual (or Linux)
> interrupts are fixed as well.
>
> I don't know why Qualcomm PMIC for SPMI is done differently.
>
> Best regards,
> Krzysztof
>


-- 
With best wishes
Dmitry

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 1/8] dt-bindings: power: supply: Add DT schema for Qualcomm SMBCHG
  2022-11-22 13:30             ` Dmitry Baryshkov
@ 2022-11-28 11:39               ` Krzysztof Kozlowski
  2022-11-28 11:52                 ` Dmitry Baryshkov
  0 siblings, 1 reply; 23+ messages in thread
From: Krzysztof Kozlowski @ 2022-11-28 11:39 UTC (permalink / raw)
  To: Dmitry Baryshkov
  Cc: Yassine Oudjana, Sebastian Reichel, Rob Herring,
	Krzysztof Kozlowski, Andy Gross, Bjorn Andersson,
	Alejandro Tafalla, Konrad Dybcio, linux-pm, linux-arm-msm,
	devicetree, phone-devel, linux-kernel, Yassine Oudjana

On 22/11/2022 14:30, Dmitry Baryshkov wrote:
> Hi,
> 
> On Mon, 21 Nov 2022 at 19:07, Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
>>
>> On 21/11/2022 11:36, Yassine Oudjana wrote:
>>>
>>> On Mon, Nov 21 2022 at 09:26:59 +01:00:00, Krzysztof Kozlowski
>>> <krzysztof.kozlowski@linaro.org> wrote:
>>>> On 20/11/2022 16:46, Yassine Oudjana wrote:
>>>>>>>  +  interrupts:
>>>>>>>  +    items:
>>>>>>>  +      - description: Charger error
>>>>>>>  +      - description: Charger inhibited
>>>>>>>  +      - description: Charger precharge safety timer timeout
>>>>>>>  +      - description: Charger charge safety timer timeout
>>>>>>>  +      - description: Charger pre to fast charging switch
>>>>>>> threshold reached
>>>>>>>  +      - description: Charger recharge threshold reached
>>>>>>>  +      - description: Charger taper threshold reached
>>>>>>>  +      - description: Charger charge termination threshold reached
>>>>>>>  +      - description: Battery hot
>>>>>>>  +      - description: Battery warm
>>>>>>>  +      - description: Battery cold
>>>>>>>  +      - description: Battery cool
>>>>>>>  +      - description: Battery overvoltage
>>>>>>>  +      - description: Battery low
>>>>>>>  +      - description: Battery missing
>>>>>>>  +      - description: Battery thermistor missing # unconfirmed
>>>>>>>  +      - description: USB input undervolt
>>>>>>>  +      - description: USB input overvolt
>>>>>>>  +      - description: USB input source detected
>>>>>>>  +      - description: OTG regulator failure
>>>>>>>  +      - description: OTG regulator overcurrent
>>>>>>>  +      - description: Automatic input current limiting done
>>>>>>>  +      - description: USB ID pin changed
>>>>>>>  +      - description: DC input undervolt
>>>>>>>  +      - description: DC input overvolt
>>>>>>>  +      - description: Power OK
>>>>>>>  +      - description: Temperature shutdown
>>>>>>>  +      - description: Watchdog timeout
>>>>>>>  +      - description: Flash failure
>>>>>>>  +      - description: OTST2 # unknown
>>>>>>>  +      - description: OTST3 # unknown
>>>>>>
>>>>>>  It seems you listed register interrupts, not physical pins. This
>>>>>> should
>>>>>>  be interrupt lines.
>>>>>
>>>>>  I'm not sure what I'm supposed to do here. I couldn't find an
>>>>> interrupt-lines
>>>>>  property used anywhere so that's not what you meant, right?
>>>>
>>>> Are these physical interrupt lines this device has, register offsets
>>>> or
>>>> virtual interrupts (e.g. passed via irq_chip)? Definitely not the
>>>> first
>>>> and rather offsets for qpnpint_irq_domain_translate. Devicetree is not
>>>> for describing register layout of devices. IOW, register layout does
>>>> not
>>>> change on different boards, because the device is exactly the same, so
>>>> there is no point to put it into DTS.
>>>>
>>>
>>> So how would I describe the interrupts then? Or if you are saying I
>>> shouldn't have these interrupts in DT at all, how would I get them and
>>> register handlers for them in the driver? the PMIC arbiter takes 4
>>> interrupt cells, 3 of which are these offsets specifying the peripheral
>>> and interrupt. All other PMIC peripherals currently described in DT
>>> (examples being qcom,pm8916-wcd-analog-codec, qcom,pm8941-pwrkey and
>>> qcom-wled) have their interrupts (if any) described this way, with the
>>> only exceptions perhaps being the GPIO and MPP controllers which are
>>> themselves interrupt controllers. Changing the way PMIC peripheral
>>> interrupts are described would require changing PMIC arbiter bindings
>>> and code which I believe is out of the scope of this patch series.
>>
>> I don't think this would touch PMIC arbiter bindings, rather the PMIC
>> itself. Usually complex devices (like PMICs) have one few physical
>> interrupt lines and many registers related to some specific interrupts.
>> For example:
>> 1. One IRQ line,
>> 2. Register with bits for overvoltage, undervoltage, vharger error etc.
>>
>> Now how the MFD child device accesses them. Since this is strictly
>> related to hardware programming model, it's not something you put to
>> Devicetree. Instead parent device (PMIC) registers IRQ chip for its one
>> interrupt line with several Linux (or virtual) interrupts. The children
>> then just get a virtual IRQ from the parent (PMIC) and setup a
>> handler(s) for them.
> 
> Unfortunately this is not how SPMI PMICs work (at least on the
> Qualcomm platforms). Access to interrupt registers is handled via the
> SPMI bus arbiter writes, not through the GPIO pin or typical spmi's
> bus interface (in the other words, not through the PMIC's SPMI
> regmap). 

I am not sure how this is related... Just because they do not use same
regmap/interface does not mean that child device should have register
bits as interrupt sources. Do you model I2C PMICs devices the same way?
No. They get the interrupts from the parent and how the parent handles
them (same or different regmap) is separate problem.

The charger node does not make SPMI bus as interrupt parent, so these
interrupts are going to the SPMI PMIC don't they? or is it mistake in
DTS - lack of interrupt-parent?


> I guess we can add an intermediate irq chip to automatically
> handle the USID, etc. However I doubt that it will really bring a lot
> in our case.

The charger node defines all interrupts with SID=2, which is also not
really correct. The parent device is SID=2. The child - does not matter.
DT is a tree for some reason...

Best regards,
Krzysztof


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 1/8] dt-bindings: power: supply: Add DT schema for Qualcomm SMBCHG
  2022-11-28 11:39               ` Krzysztof Kozlowski
@ 2022-11-28 11:52                 ` Dmitry Baryshkov
  0 siblings, 0 replies; 23+ messages in thread
From: Dmitry Baryshkov @ 2022-11-28 11:52 UTC (permalink / raw)
  To: Krzysztof Kozlowski
  Cc: Yassine Oudjana, Sebastian Reichel, Rob Herring,
	Krzysztof Kozlowski, Andy Gross, Bjorn Andersson,
	Alejandro Tafalla, Konrad Dybcio, linux-pm, linux-arm-msm,
	devicetree, phone-devel, linux-kernel, Yassine Oudjana

Hi,

On Mon, 28 Nov 2022 at 13:39, Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 22/11/2022 14:30, Dmitry Baryshkov wrote:
> > Hi,
> >
> > On Mon, 21 Nov 2022 at 19:07, Krzysztof Kozlowski
> > <krzysztof.kozlowski@linaro.org> wrote:
> >>
> >> On 21/11/2022 11:36, Yassine Oudjana wrote:
> >>>
> >>> On Mon, Nov 21 2022 at 09:26:59 +01:00:00, Krzysztof Kozlowski
> >>> <krzysztof.kozlowski@linaro.org> wrote:
> >>>> On 20/11/2022 16:46, Yassine Oudjana wrote:
> >>>>>>>  +  interrupts:
> >>>>>>>  +    items:
> >>>>>>>  +      - description: Charger error
> >>>>>>>  +      - description: Charger inhibited
> >>>>>>>  +      - description: Charger precharge safety timer timeout
> >>>>>>>  +      - description: Charger charge safety timer timeout
> >>>>>>>  +      - description: Charger pre to fast charging switch
> >>>>>>> threshold reached
> >>>>>>>  +      - description: Charger recharge threshold reached
> >>>>>>>  +      - description: Charger taper threshold reached
> >>>>>>>  +      - description: Charger charge termination threshold reached
> >>>>>>>  +      - description: Battery hot
> >>>>>>>  +      - description: Battery warm
> >>>>>>>  +      - description: Battery cold
> >>>>>>>  +      - description: Battery cool
> >>>>>>>  +      - description: Battery overvoltage
> >>>>>>>  +      - description: Battery low
> >>>>>>>  +      - description: Battery missing
> >>>>>>>  +      - description: Battery thermistor missing # unconfirmed
> >>>>>>>  +      - description: USB input undervolt
> >>>>>>>  +      - description: USB input overvolt
> >>>>>>>  +      - description: USB input source detected
> >>>>>>>  +      - description: OTG regulator failure
> >>>>>>>  +      - description: OTG regulator overcurrent
> >>>>>>>  +      - description: Automatic input current limiting done
> >>>>>>>  +      - description: USB ID pin changed
> >>>>>>>  +      - description: DC input undervolt
> >>>>>>>  +      - description: DC input overvolt
> >>>>>>>  +      - description: Power OK
> >>>>>>>  +      - description: Temperature shutdown
> >>>>>>>  +      - description: Watchdog timeout
> >>>>>>>  +      - description: Flash failure
> >>>>>>>  +      - description: OTST2 # unknown
> >>>>>>>  +      - description: OTST3 # unknown
> >>>>>>
> >>>>>>  It seems you listed register interrupts, not physical pins. This
> >>>>>> should
> >>>>>>  be interrupt lines.
> >>>>>
> >>>>>  I'm not sure what I'm supposed to do here. I couldn't find an
> >>>>> interrupt-lines
> >>>>>  property used anywhere so that's not what you meant, right?
> >>>>
> >>>> Are these physical interrupt lines this device has, register offsets
> >>>> or
> >>>> virtual interrupts (e.g. passed via irq_chip)? Definitely not the
> >>>> first
> >>>> and rather offsets for qpnpint_irq_domain_translate. Devicetree is not
> >>>> for describing register layout of devices. IOW, register layout does
> >>>> not
> >>>> change on different boards, because the device is exactly the same, so
> >>>> there is no point to put it into DTS.
> >>>>
> >>>
> >>> So how would I describe the interrupts then? Or if you are saying I
> >>> shouldn't have these interrupts in DT at all, how would I get them and
> >>> register handlers for them in the driver? the PMIC arbiter takes 4
> >>> interrupt cells, 3 of which are these offsets specifying the peripheral
> >>> and interrupt. All other PMIC peripherals currently described in DT
> >>> (examples being qcom,pm8916-wcd-analog-codec, qcom,pm8941-pwrkey and
> >>> qcom-wled) have their interrupts (if any) described this way, with the
> >>> only exceptions perhaps being the GPIO and MPP controllers which are
> >>> themselves interrupt controllers. Changing the way PMIC peripheral
> >>> interrupts are described would require changing PMIC arbiter bindings
> >>> and code which I believe is out of the scope of this patch series.
> >>
> >> I don't think this would touch PMIC arbiter bindings, rather the PMIC
> >> itself. Usually complex devices (like PMICs) have one few physical
> >> interrupt lines and many registers related to some specific interrupts.
> >> For example:
> >> 1. One IRQ line,
> >> 2. Register with bits for overvoltage, undervoltage, vharger error etc.
> >>
> >> Now how the MFD child device accesses them. Since this is strictly
> >> related to hardware programming model, it's not something you put to
> >> Devicetree. Instead parent device (PMIC) registers IRQ chip for its one
> >> interrupt line with several Linux (or virtual) interrupts. The children
> >> then just get a virtual IRQ from the parent (PMIC) and setup a
> >> handler(s) for them.
> >
> > Unfortunately this is not how SPMI PMICs work (at least on the
> > Qualcomm platforms). Access to interrupt registers is handled via the
> > SPMI bus arbiter writes, not through the GPIO pin or typical spmi's
> > bus interface (in the other words, not through the PMIC's SPMI
> > regmap).
>
> I am not sure how this is related... Just because they do not use same
> regmap/interface does not mean that child device should have register
> bits as interrupt sources. Do you model I2C PMICs devices the same way?
> No. They get the interrupts from the parent and how the parent handles
> them (same or different regmap) is separate problem.

For i2c PMICs it's typically not the case, since the interrupt is OOB.
It is an external GPIO pin, as you have described previously.

For Qcom SPMI PMIC the interrupts are really handled by the SPMI host
controller (arbieter). So the DT really represents the hardware.

>
> The charger node does not make SPMI bus as interrupt parent, so these
> interrupts are going to the SPMI PMIC don't they? or is it mistake in
> DTS - lack of interrupt-parent?

I thought that with the lack of the interrupt-parent, the OS will
traverse the tree up until it finds one. Is it so?

> > I guess we can add an intermediate irq chip to automatically
> > handle the USID, etc. However I doubt that it will really bring a lot
> > in our case.
>
> The charger node defines all interrupts with SID=2, which is also not
> really correct. The parent device is SID=2. The child - does not matter.
> DT is a tree for some reason...

Yes, I agree with you. The bindings do not look ideal here. I was
trying to point out that this is not a problem with the smbchg device
only. Other Qcom SPMI PMIC devices also use exactly the same approach
(except gpio and mpps).
So, we can either:
- Continue using the current approach
- Rework pmic driver to provide an intermediate IRQ domain. This would
require finding a way to ping SPMI ARB's IRQ registers from PMIC
controller (via callbacks, via extra interrupt translation, etc).

-- 
With best wishes
Dmitry

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 1/8] dt-bindings: power: supply: Add DT schema for Qualcomm SMBCHG
  2022-08-08  7:34 ` [PATCH 1/8] dt-bindings: power: supply: Add DT schema " Yassine Oudjana
  2022-08-08  8:42   ` Krzysztof Kozlowski
@ 2022-11-30 16:24   ` Krzysztof Kozlowski
  1 sibling, 0 replies; 23+ messages in thread
From: Krzysztof Kozlowski @ 2022-11-30 16:24 UTC (permalink / raw)
  To: Yassine Oudjana, Sebastian Reichel, Rob Herring,
	Krzysztof Kozlowski, Andy Gross, Bjorn Andersson
  Cc: Yassine Oudjana, Alejandro Tafalla, Konrad Dybcio, linux-pm,
	linux-arm-msm, devicetree, phone-devel, linux-kernel

On 08/08/2022 09:34, Yassine Oudjana wrote:
> From: Yassine Oudjana <y.oudjana@protonmail.com>
> 


> +      See Documentation/devicetree/bindings/power/supply/battery.yaml
> +
> +  interrupts:
> +    items:
> +      - description: Charger error
> +      - description: Charger inhibited
> +      - description: Charger precharge safety timer timeout
> +      - description: Charger charge safety timer timeout
> +      - description: Charger pre to fast charging switch threshold reached
> +      - description: Charger recharge threshold reached
> +      - description: Charger taper threshold reached

After discussing with Dmitry, it's current design of the PMIC (parent
device) so it's fine. We would need to make bigger refactoring of
drivers and bindings to change it to match other typical MFD PMICs. In
current state it's the only way to express device interrupts.

Two more comments below.

> +      - description: Charger charge termination threshold reached
> +      - description: Battery hot
> +      - description: Battery warm
> +      - description: Battery cold
> +      - description: Battery cool
> +      - description: Battery overvoltage
> +      - description: Battery low
> +      - description: Battery missing
> +      - description: Battery thermistor missing # unconfirmed
> +      - description: USB input undervolt
> +      - description: USB input overvolt
> +      - description: USB input source detected
> +      - description: OTG regulator failure
> +      - description: OTG regulator overcurrent
> +      - description: Automatic input current limiting done
> +      - description: USB ID pin changed
> +      - description: DC input undervolt
> +      - description: DC input overvolt
> +      - description: Power OK
> +      - description: Temperature shutdown
> +      - description: Watchdog timeout
> +      - description: Flash failure
> +      - description: OTST2 # unknown
> +      - description: OTST3 # unknown
> +
> +  interrupt-names:
> +    items:
> +      - const: chg-error
> +      - const: chg-inhibit
> +      - const: chg-prechg-sft
> +      - const: chg-complete-chg-sft
> +      - const: chg-p2f-thr
> +      - const: chg-rechg-thr
> +      - const: chg-taper-thr
> +      - const: chg-tcc-thr
> +      - const: batt-hot
> +      - const: batt-warm
> +      - const: batt-cold
> +      - const: batt-cool
> +      - const: batt-ov
> +      - const: batt-low
> +      - const: batt-missing
> +      - const: batt-term-missing
> +      - const: usbin-uv
> +      - const: usbin-ov
> +      - const: usbin-src-det
> +      - const: otg-fail
> +      - const: otg-oc
> +      - const: aicl-done
> +      - const: usbid-change
> +      - const: dcin-uv
> +      - const: dcin-ov
> +      - const: power-ok
> +      - const: temp-shutdown
> +      - const: wdog-timeout
> +      - const: flash-fail
> +      - const: otst2
> +      - const: otst3
> +
> +  otg-vbus:
> +    type: object

I think I did not comment about this one - this looks like regulator.yaml.

> +
> +    description:
> +      OTG regulator subnode.
> +
> +required:
> +  - compatible
> +  - reg
> +  - monitored-battery
> +  - interrupts
> +  - interrupt-names
> +  - otg-vbus
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +
> +    battery: battery {
> +        compatible = "simple-battery";
> +
> +        charge-full-design-microamp-hours = <4070000>;
> +        charge-term-current-microamp = <100000>;
> +        voltage-min-design-microvolt = <3400000>;
> +        voltage-max-design-microvolt = <4400000>;
> +    };
> +
> +    pmic {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        charger@1000 {
> +            compatible = "qcom,pmi8996-smbchg";
> +            reg = <0x1000>;
> +
> +            interrupts = <0x2 0x10 0x0 IRQ_TYPE_EDGE_BOTH>,
> +                         <0x2 0x10 0x1 IRQ_TYPE_EDGE_BOTH>,
> +                         <0x2 0x10 0x2 IRQ_TYPE_EDGE_BOTH>,
> +                         <0x2 0x10 0x3 IRQ_TYPE_EDGE_BOTH>,
> +                         <0x2 0x10 0x4 IRQ_TYPE_EDGE_BOTH>,
> +                         <0x2 0x10 0x5 IRQ_TYPE_EDGE_BOTH>,
> +                         <0x2 0x10 0x6 IRQ_TYPE_EDGE_RISING>,
> +                         <0x2 0x10 0x7 IRQ_TYPE_EDGE_RISING>,
> +                         <0x2 0x12 0x0 IRQ_TYPE_EDGE_BOTH>,
> +                         <0x2 0x12 0x1 IRQ_TYPE_EDGE_BOTH>,
> +                         <0x2 0x12 0x2 IRQ_TYPE_EDGE_BOTH>,
> +                         <0x2 0x12 0x3 IRQ_TYPE_EDGE_BOTH>,
> +                         <0x2 0x12 0x4 IRQ_TYPE_EDGE_BOTH>,
> +                         <0x2 0x12 0x5 IRQ_TYPE_EDGE_BOTH>,
> +                         <0x2 0x12 0x6 IRQ_TYPE_EDGE_BOTH>,
> +                         <0x2 0x12 0x7 IRQ_TYPE_EDGE_BOTH>,
> +                         <0x2 0x13 0x0 IRQ_TYPE_EDGE_BOTH>,
> +                         <0x2 0x13 0x1 IRQ_TYPE_EDGE_BOTH>,
> +                         <0x2 0x13 0x2 IRQ_TYPE_EDGE_BOTH>,
> +                         <0x2 0x13 0x3 IRQ_TYPE_EDGE_BOTH>,
> +                         <0x2 0x13 0x4 IRQ_TYPE_EDGE_RISING>,
> +                         <0x2 0x13 0x5 IRQ_TYPE_EDGE_RISING>,
> +                         <0x2 0x13 0x6 IRQ_TYPE_EDGE_FALLING>,
> +                         <0x2 0x14 0x0 IRQ_TYPE_EDGE_BOTH>,
> +                         <0x2 0x14 0x1 IRQ_TYPE_EDGE_BOTH>,
> +                         <0x2 0x16 0x0 IRQ_TYPE_EDGE_BOTH>,
> +                         <0x2 0x16 0x1 IRQ_TYPE_EDGE_BOTH>,
> +                         <0x2 0x16 0x2 IRQ_TYPE_EDGE_BOTH>,
> +                         <0x2 0x16 0x3 IRQ_TYPE_EDGE_BOTH>,
> +                         <0x2 0x16 0x4 IRQ_TYPE_EDGE_BOTH>,
> +                         <0x2 0x16 0x5 IRQ_TYPE_EDGE_BOTH>;
> +            interrupt-names = "chg-error",
> +                              "chg-inhibit",
> +                              "chg-prechg-sft",
> +                              "chg-complete-chg-sft",
> +                              "chg-p2f-thr",
> +                              "chg-rechg-thr",
> +                              "chg-taper-thr",
> +                              "chg-tcc-thr",
> +                              "batt-hot",
> +                              "batt-warm",
> +                              "batt-cold",
> +                              "batt-cool",
> +                              "batt-ov",
> +                              "batt-low",
> +                              "batt-missing",
> +                              "batt-term-missing",
> +                              "usbin-uv",
> +                              "usbin-ov",
> +                              "usbin-src-det",
> +                              "otg-fail",
> +                              "otg-oc",
> +                              "aicl-done",
> +                              "usbid-change",
> +                              "dcin-uv",
> +                              "dcin-ov",
> +                              "power-ok",
> +                              "temp-shutdown",
> +                              "wdog-timeout",
> +                              "flash-fail",
> +                              "otst2",
> +                              "otst3";
> +
> +            monitored-battery = <&battery>;
> +
> +            otg-vbus { };

Why empty?

Best regards,
Krzysztof


^ permalink raw reply	[flat|nested] 23+ messages in thread

end of thread, other threads:[~2022-11-30 16:24 UTC | newest]

Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-08-08  7:34 [PATCH 0/8] power: supply: Add driver for Qualcomm SMBCHG Yassine Oudjana
2022-08-08  7:34 ` [PATCH 1/8] dt-bindings: power: supply: Add DT schema " Yassine Oudjana
2022-08-08  8:42   ` Krzysztof Kozlowski
2022-11-20 15:46     ` Yassine Oudjana
2022-11-21  8:26       ` Krzysztof Kozlowski
2022-11-21 10:36         ` Yassine Oudjana
2022-11-21 17:07           ` Krzysztof Kozlowski
2022-11-22 13:30             ` Dmitry Baryshkov
2022-11-28 11:39               ` Krzysztof Kozlowski
2022-11-28 11:52                 ` Dmitry Baryshkov
2022-11-30 16:24   ` Krzysztof Kozlowski
2022-08-08  7:34 ` [PATCH 2/8] arm64: dts: qcom: pmi8994: Add SMBCHG Yassine Oudjana
2022-08-08  7:34 ` [PATCH 3/8] arm64: dts: qcom: pmi8996: " Yassine Oudjana
2022-08-08  7:34 ` [PATCH 5/8] arm64: dts: qcom: msm8996-xiaomi-*: Enable SMBCHG Yassine Oudjana
2022-08-08  7:34 ` [PATCH 6/8] soc: qcom: Add PMIC secure register write helpers Yassine Oudjana
2022-08-08  7:34 ` [PATCH 7/8] util_macros.h: Add macro to find closest smaller value in array Yassine Oudjana
2022-08-08  7:34 ` [PATCH 8/8] power: supply: Add driver for Qualcomm SMBCHG Yassine Oudjana
2022-08-08  8:55   ` Krzysztof Kozlowski
2022-08-08 10:05     ` Yassine Oudjana
2022-08-08 13:42       ` Krzysztof Kozlowski
2022-08-08  8:41 ` [PATCH 0/8] " Krzysztof Kozlowski
2022-08-08  9:39   ` Yassine Oudjana
2022-08-08 13:24     ` Caleb Connolly

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.