[RFC PATCH 0/6] Support ROHM BD96801 scalable PMIC

[RFC PATCH 0/6] Support ROHM BD96801 scalable PMIC

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 3578 bytes --]

Support ROHM BD96801 "scalable" PMIC.

The ROHM BD96801 is automotive grade PMIC, intended to be usable in
multiple solutions. The BD96801 can be used as a stand-alone, or together
with separate 'companion PMICs'. This modular approach aims to make this
PMIC suitable for various use-cases.

This RFC series is a product of a _long_ process. The product has been
re-designed a few times and this series has been sitting in my git
forgotten for long periods of time, then been re-worked when new design
has been available, after which it has again been forgotten. Last week I
finally received the word that this product should've been stabilized,
digged up my last set of patches (from Nov 2021 - cover letter
reminding me the driver development had been done during 3 years...)

I think this is valid information for reviewers as it's good to keep an
eye on obsoleted practices - even though I tried updating this series.

This is sent as an RFC because of the regulator features which can be
configured only when the PMIC is in STBY state. This is described more
detailed in the regulator patch.

Another "oddity" is that the PMIC has two physical IRQ lines. When I
last wrote this patch in 2021 I had some naming collison in debugfs for
the IRQ domains. Back then I used:
irq_domain_update_bus_token(intb_domain, DOMAIN_BUS_WIRED);
to work-around the issue. Now, when rebasing to v6.9-rc1 the naming
collision was gone and things seemed to work. However, it'd be great if
the IRQ code in MFD driver was reviewed by greater minds :)

Rest of the series ought to be business as usual.

Matti Vaittinen (6):
  dt-bindings: ROHM BD96801 PMIC regulators
  dt-bindings: mfd: bd96801 PMIC core
  mfd: support ROHM BD96801 PMIC core
  regulator: bd96801: ROHM BD96801 PMIC regulators
  watchdog: ROHM BD96801 PMIC WDG driver
  MAINTAINERS: Add ROHM BD96801 'scalable PMIC' entries

 .../bindings/mfd/rohm,bd96801-pmic.yaml       |  155 ++
 .../regulator/rohm,bd96801-regulator.yaml     |   69 +
 MAINTAINERS                                   |    4 +
 drivers/mfd/Kconfig                           |   13 +
 drivers/mfd/Makefile                          |    1 +
 drivers/mfd/rohm-bd96801.c                    |  454 ++++
 drivers/regulator/Kconfig                     |   12 +
 drivers/regulator/Makefile                    |    2 +
 drivers/regulator/bd96801-regulator.c         | 2109 +++++++++++++++++
 drivers/watchdog/Kconfig                      |   13 +
 drivers/watchdog/Makefile                     |    1 +
 drivers/watchdog/bd96801_wdt.c                |  375 +++
 include/linux/mfd/rohm-bd96801.h              |  212 ++
 include/linux/mfd/rohm-generic.h              |    1 +
 14 files changed, 3421 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mfd/rohm,bd96801-pmic.yaml
 create mode 100644 Documentation/devicetree/bindings/regulator/rohm,bd96801-regulator.yaml
 create mode 100644 drivers/mfd/rohm-bd96801.c
 create mode 100644 drivers/regulator/bd96801-regulator.c
 create mode 100644 drivers/watchdog/bd96801_wdt.c
 create mode 100644 include/linux/mfd/rohm-bd96801.h


base-commit: 4cece764965020c22cff7665b18a012006359095
-- 
2.43.2


-- 
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND

~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
Simon says - in Latin please.
~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
Thanks to Simon Glass for the translation =] 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[RFC PATCH 1/6] dt-bindings: ROHM BD96801 PMIC regulators

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 3313 bytes --]

ROHM BD96801 is a highly configurable automotive grade PMIC. Introduce
DT bindings for the BD96801 regulators.

Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
---
 .../regulator/rohm,bd96801-regulator.yaml     | 69 +++++++++++++++++++
 1 file changed, 69 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/regulator/rohm,bd96801-regulator.yaml

diff --git a/Documentation/devicetree/bindings/regulator/rohm,bd96801-regulator.yaml b/Documentation/devicetree/bindings/regulator/rohm,bd96801-regulator.yaml
new file mode 100644
index 000000000000..4015802a3d84
--- /dev/null
+++ b/Documentation/devicetree/bindings/regulator/rohm,bd96801-regulator.yaml
@@ -0,0 +1,69 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/regulator/rohm,bd96801-regulator.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: ROHM BD96801 Power Management Integrated Circuit regulators
+
+maintainers:
+  - Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
+
+description: |
+  This module is part of the ROHM BD96801 MFD device. For more details
+  see Documentation/devicetree/bindings/mfd/rohm,bd96801-pmic.yaml.
+
+  The regulator controller is represented as a sub-node of the PMIC node
+  on the device tree.
+
+  Regulator nodes should be named to BUCK_<number> and LDO_<number>.
+  The valid names for BD96801 regulator nodes are
+  BUCK1, BUCK2, BUCK3, BUCK4, LDO5, LDO6, LDO7
+
+patternProperties:
+  "^LDO[5-7]$":
+    type: object
+    description:
+      Properties for single LDO regulator.
+    $ref: regulator.yaml#
+
+    properties:
+      regulator-name:
+        pattern: "^ldo[5-7]$"
+        description:
+          Name of the regulator. Should be "ldo5", ..., "ldo7"
+      rohm,initial-voltage-microvolt:
+        description:
+          Initial voltage for regulator. Voltage can be tuned +/-150 mV from
+          this value. NOTE, This can be modified via I2C only when PMIC is in
+          STBY state.
+        minimum: 300000
+        maximum: 3300000
+
+  "^BUCK[1-4]$":
+    type: object
+    description:
+      Properties for single BUCK regulator.
+    $ref: regulator.yaml#
+
+    properties:
+      regulator-name:
+        pattern: "^buck[1-4]$"
+        description:
+          should be "buck1", ..., "buck4"
+      rohm,initial-voltage-microvolt:
+        description:
+          Initial voltage for regulator. Voltage can be tuned +/-150 mV from
+          this value. NOTE, This can be modified via I2C only when PMIC is in
+          STBY state.
+        minimum: 500000
+        maximum: 3300000
+      rohm,keep-on-stby:
+        description:
+          Keep the regulator powered when PMIC transitions to STBY state.
+        type: boolean
+
+    required:
+      - regulator-name
+  additionalProperties: false
+additionalProperties: false
-- 
2.43.2


-- 
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND

~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
Simon says - in Latin please.
~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
Thanks to Simon Glass for the translation =] 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[RFC PATCH 2/6] dt-bindings: mfd: bd96801 PMIC core

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 7075 bytes --]

ROHM BD96801 is a highly configurable automotive grade PMIC. Introduce
DT bindings for the BD96801 core.

Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
---
 .../bindings/mfd/rohm,bd96801-pmic.yaml       | 155 ++++++++++++++++++
 1 file changed, 155 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mfd/rohm,bd96801-pmic.yaml

diff --git a/Documentation/devicetree/bindings/mfd/rohm,bd96801-pmic.yaml b/Documentation/devicetree/bindings/mfd/rohm,bd96801-pmic.yaml
new file mode 100644
index 000000000000..0d512fe19081
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/rohm,bd96801-pmic.yaml
@@ -0,0 +1,155 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/mfd/rohm,bd96801-pmic.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: ROHM BD96801 Scalable Power Management Integrated Circuit
+
+maintainers:
+  - Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
+
+description: |
+  BD96801 is an automotive grade single-chip power management IC.
+  It integrates 4 buck converters and 3 LDOs with safety features like
+  over-/under voltage and over current detection and a watchdog.
+
+properties:
+  compatible:
+    const: rohm,bd96801
+
+  reg:
+    description:
+      I2C slave address.
+    maxItems: 1
+
+  interrupts:
+    description:
+      The PMIC provides intb and errb IRQ lines. The errb IRQ line is used
+      for fatal IRQs which will cause the PMIC to shut down power outputs.
+      In many systems this will shut down the SoC contolling the PMIC and
+      connecting/handling the errb can be omitted. However, there are cases
+      where the SoC is not powered by the PMIC. In that case it may be
+      useful to connect the errb and handle errb events.
+    minItems: 1
+    maxItems: 2
+
+  interrupt-names:
+    minItems: 1
+    items:
+      - enum: [intb, errb]
+      - const: errb
+
+  regulators:
+    $ref: ../regulator/rohm,bd96801-regulator.yaml
+    description:
+      List of child nodes that specify the regulators.
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - interrupt-names
+  - regulators
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+    #include <dt-bindings/leds/common.h>
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+        pmic: pmic@60 {
+            reg = <0x60>;
+            compatible = "rohm,bd96801";
+            interrupt-parent = <&gpio1>;
+            interrupts = <29 IRQ_TYPE_LEVEL_LOW>, <6 IRQ_TYPE_LEVEL_LOW>;
+            interrupt-names = "intb", "errb";
+
+            regulators {
+                buck1: BUCK1 {
+                    regulator-name = "buck1";
+                    regulator-ramp-delay = <1250>;
+                    /* 0.5V min INITIAL - 150 mV tune */
+                    regulator-min-microvolt = <350000>;
+                    /* 3.3V + 150mV tune */
+                    regulator-max-microvolt = <3450000>;
+
+                    /* These can be set only when PMIC is in STBY */
+                    rohm,initial-voltage-microvolt = <500000>;
+                    regulator-ov-error-microvolt = <230000>;
+                    regulator-uv-error-microvolt = <230000>;
+                    regulator-temp-protection-kelvin = <1>;
+                    regulator-temp-warn-kelvin = <0>;
+                };
+                buck2: BUCK2 {
+                    regulator-name = "buck2";
+                    regulator-min-microvolt = <350000>;
+                    regulator-max-microvolt = <3450000>;
+
+                    rohm,initial-voltage-microvolt = <3000000>;
+                    regulator-ov-error-microvolt = <18000>;
+                    regulator-uv-error-microvolt = <18000>;
+                    regulator-temp-protection-kelvin = <1>;
+                    regulator-temp-warn-kelvin = <1>;
+                };
+                buck3: BUCK3 {
+                    regulator-name = "buck3";
+                    regulator-min-microvolt = <350000>;
+                    regulator-max-microvolt = <3450000>;
+
+                    rohm,initial-voltage-microvolt = <600000>;
+                    regulator-ov-warn-microvolt = <18000>;
+                    regulator-uv-warn-microvolt = <18000>;
+                    regulator-temp-protection-kelvin = <1>;
+                    regulator-temp-error-kelvin = <0>;
+                };
+                buck4: BUCK4 {
+                    regulator-name = "buck4";
+                    regulator-min-microvolt = <350000>;
+                    regulator-max-microvolt = <3450000>;
+
+                    rohm,initial-voltage-microvolt = <600000>;
+                    regulator-ov-warn-microvolt = <18000>;
+                    regulator-uv-warn-microvolt = <18000>;
+                    regulator-temp-protection-kelvin = <1>;
+                    regulator-temp-error-kelvin = <0>;
+                };
+                ldo5: LDO5 {
+                    regulator-name = "ldo5";
+                    regulator-min-microvolt = <300000>;
+                    regulator-max-microvolt = <3300000>;
+
+                    rohm,initial-voltage-microvolt = <500000>;
+                    regulator-ov-error-microvolt = <36000>;
+                    regulator-uv-error-microvolt = <34000>;
+                    regulator-temp-protection-kelvin = <1>;
+                    regulator-temp-warn-kelvin = <0>;
+                };
+                ldo6: LDO6 {
+                    regulator-name = "ldo6";
+                    regulator-min-microvolt = <300000>;
+                    regulator-max-microvolt = <3300000>;
+
+                    rohm,initial-voltage-microvolt = <300000>;
+                    regulator-ov-error-microvolt = <36000>;
+                    regulator-uv-error-microvolt = <34000>;
+                    regulator-temp-protection-kelvin = <1>;
+                    regulator-temp-warn-kelvin = <0>;
+                };
+                ldo7: LDO7 {
+                    regulator-name = "ldo7";
+                    regulator-min-microvolt = <300000>;
+                    regulator-max-microvolt = <3300000>;
+
+                    rohm,initial-voltage-microvolt = <500000>;
+                    regulator-ov-error-microvolt = <36000>;
+                    regulator-uv-error-microvolt = <34000>;
+                    regulator-temp-protection-kelvin = <1>;
+                    regulator-temp-warn-kelvin = <0>;
+                };
+            };
+        };
+    };
-- 
2.43.2


-- 
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND

~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
Simon says - in Latin please.
~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
Thanks to Simon Glass for the translation =] 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[RFC PATCH 3/6] mfd: support ROHM BD96801 PMIC core

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 30339 bytes --]

The ROHM BD96801 PMIC is highly customizable automotive grade PMIC
which integrates regulator and watchdog funtionalities.

Provide IRQ and register accesses for regulator/watchdog drivers.

Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
---
 drivers/mfd/Kconfig              |  13 +
 drivers/mfd/Makefile             |   1 +
 drivers/mfd/rohm-bd96801.c       | 454 +++++++++++++++++++++++++++++++
 include/linux/mfd/rohm-bd96801.h | 212 +++++++++++++++
 include/linux/mfd/rohm-generic.h |   1 +
 5 files changed, 681 insertions(+)
 create mode 100644 drivers/mfd/rohm-bd96801.c
 create mode 100644 include/linux/mfd/rohm-bd96801.h

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 4b023ee229cf..947045eb3a8e 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -2089,6 +2089,19 @@ config MFD_ROHM_BD957XMUF
 	  BD9573MUF Power Management ICs. BD9576 and BD9573 are primarily
 	  designed to be used to power R-Car series processors.
 
+config MFD_ROHM_BD96801
+	tristate "ROHM BD96801 Power Management IC"
+	depends on I2C=y
+	depends on OF
+	select REGMAP_I2C
+	select REGMAP_IRQ
+	select MFD_CORE
+	help
+	  Select this option to get support for the ROHM BD96801 Power
+	  Management IC. The ROHM BD96801 is a highly scalable power management
+	  IC for industrial and automotive use. The BD96801 can be used as a
+	  master PMIC in a chained PMIC solutions with suitable companion PMICs.
+
 config MFD_STM32_LPTIMER
 	tristate "Support for STM32 Low-Power Timer"
 	depends on (ARCH_STM32 && OF) || COMPILE_TEST
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index c66f07edcd0e..e792892d4a8b 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -264,6 +264,7 @@ obj-$(CONFIG_RAVE_SP_CORE)	+= rave-sp.o
 obj-$(CONFIG_MFD_ROHM_BD71828)	+= rohm-bd71828.o
 obj-$(CONFIG_MFD_ROHM_BD718XX)	+= rohm-bd718x7.o
 obj-$(CONFIG_MFD_ROHM_BD957XMUF)	+= rohm-bd9576.o
+obj-$(CONFIG_MFD_ROHM_BD96801)	+= rohm-bd96801.o
 obj-$(CONFIG_MFD_STMFX) 	+= stmfx.o
 obj-$(CONFIG_MFD_KHADAS_MCU) 	+= khadas-mcu.o
 obj-$(CONFIG_MFD_ACER_A500_EC)	+= acer-ec-a500.o
diff --git a/drivers/mfd/rohm-bd96801.c b/drivers/mfd/rohm-bd96801.c
new file mode 100644
index 000000000000..7610d0114653
--- /dev/null
+++ b/drivers/mfd/rohm-bd96801.c
@@ -0,0 +1,454 @@
+// SPDX-License-Identifier: GPL-2.0-only
+//
+// Copyright (C) 2022 ROHM Semiconductors
+//
+// ROHM BD96801 PMIC driver
+
+#include <linux/i2c.h>
+#include <linux/interrupt.h>
+#include <linux/mfd/core.h>
+#include <linux/module.h>
+#include <linux/property.h>
+#include <linux/regmap.h>
+#include <linux/types.h>
+
+#include <linux/mfd/rohm-bd96801.h>
+#include <linux/mfd/rohm-generic.h>
+
+static const struct resource regulator_errb_irqs[] = {
+	DEFINE_RES_IRQ_NAMED(BD96801_OTP_ERR_STAT, "bd96801-otp-err"),
+	DEFINE_RES_IRQ_NAMED(BD96801_DBIST_ERR_STAT, "bd96801-dbist-err"),
+	DEFINE_RES_IRQ_NAMED(BD96801_EEP_ERR_STAT, "bd96801-eep-err"),
+	DEFINE_RES_IRQ_NAMED(BD96801_ABIST_ERR_STAT, "bd96801-abist-err"),
+	DEFINE_RES_IRQ_NAMED(BD96801_PRSTB_ERR_STAT, "bd96801-prstb-err"),
+	DEFINE_RES_IRQ_NAMED(BD96801_DRMOS1_ERR_STAT, "bd96801-drmoserr1"),
+	DEFINE_RES_IRQ_NAMED(BD96801_DRMOS2_ERR_STAT, "bd96801-drmoserr2"),
+	DEFINE_RES_IRQ_NAMED(BD96801_SLAVE_ERR_STAT, "bd96801-slave-err"),
+	DEFINE_RES_IRQ_NAMED(BD96801_VREF_ERR_STAT, "bd96801-vref-err"),
+	DEFINE_RES_IRQ_NAMED(BD96801_TSD_ERR_STAT, "bd96801-tsd"),
+	DEFINE_RES_IRQ_NAMED(BD96801_UVLO_ERR_STAT, "bd96801-uvlo-err"),
+	DEFINE_RES_IRQ_NAMED(BD96801_OVLO_ERR_STAT, "bd96801-ovlo-err"),
+	DEFINE_RES_IRQ_NAMED(BD96801_OSC_ERR_STAT, "bd96801-osc-err"),
+	DEFINE_RES_IRQ_NAMED(BD96801_PON_ERR_STAT, "bd96801-pon-err"),
+	DEFINE_RES_IRQ_NAMED(BD96801_POFF_ERR_STAT, "bd96801-poff-err"),
+	DEFINE_RES_IRQ_NAMED(BD96801_CMD_SHDN_ERR_STAT, "bd96801-cmd-shdn-err"),
+
+	DEFINE_RES_IRQ_NAMED(BD96801_INT_PRSTB_WDT_ERR, "bd96801-prstb-wdt-err"),
+	DEFINE_RES_IRQ_NAMED(BD96801_INT_CHIP_IF_ERR, "bd96801-chip-if-err"),
+	DEFINE_RES_IRQ_NAMED(BD96801_INT_SHDN_ERR_STAT, "bd96801-int-shdn-err"),
+
+	DEFINE_RES_IRQ_NAMED(BD96801_BUCK1_PVIN_ERR_STAT, "bd96801-buck1-pvin-err"),
+	DEFINE_RES_IRQ_NAMED(BD96801_BUCK1_OVP_ERR_STAT, "bd96801-buck1-ovp-err"),
+	DEFINE_RES_IRQ_NAMED(BD96801_BUCK1_UVP_ERR_STAT, "bd96801-buck1-uvp-err"),
+	DEFINE_RES_IRQ_NAMED(BD96801_BUCK1_SHDN_ERR_STAT, "bd96801-buck1-shdn-err"),
+
+	DEFINE_RES_IRQ_NAMED(BD96801_BUCK2_PVIN_ERR_STAT, "bd96801-buck2-pvin-err"),
+	DEFINE_RES_IRQ_NAMED(BD96801_BUCK2_OVP_ERR_STAT, "bd96801-buck2-ovp-err"),
+	DEFINE_RES_IRQ_NAMED(BD96801_BUCK2_UVP_ERR_STAT, "bd96801-buck2-uvp-err"),
+	DEFINE_RES_IRQ_NAMED(BD96801_BUCK2_SHDN_ERR_STAT, "bd96801-buck2-shdn-err"),
+
+	DEFINE_RES_IRQ_NAMED(BD96801_BUCK3_PVIN_ERR_STAT, "bd96801-buck3-pvin-err"),
+	DEFINE_RES_IRQ_NAMED(BD96801_BUCK3_OVP_ERR_STAT, "bd96801-buck3-ovp-err"),
+	DEFINE_RES_IRQ_NAMED(BD96801_BUCK3_UVP_ERR_STAT, "bd96801-buck3-uvp-err"),
+	DEFINE_RES_IRQ_NAMED(BD96801_BUCK3_SHDN_ERR_STAT, "bd96801-buck3-shdn-err"),
+
+	DEFINE_RES_IRQ_NAMED(BD96801_BUCK4_PVIN_ERR_STAT, "bd96801-buck4-pvin-err"),
+	DEFINE_RES_IRQ_NAMED(BD96801_BUCK4_OVP_ERR_STAT, "bd96801-buck4-ovp-err"),
+	DEFINE_RES_IRQ_NAMED(BD96801_BUCK4_UVP_ERR_STAT, "bd96801-buck4-uvp-err"),
+	DEFINE_RES_IRQ_NAMED(BD96801_BUCK4_SHDN_ERR_STAT, "bd96801-buck4-shdn-err"),
+
+	DEFINE_RES_IRQ_NAMED(BD96801_LDO5_PVIN_ERR_STAT, "bd96801-ldo5-pvin-err"),
+	DEFINE_RES_IRQ_NAMED(BD96801_LDO5_OVP_ERR_STAT, "bd96801-ldo5-ovp-err"),
+	DEFINE_RES_IRQ_NAMED(BD96801_LDO5_UVP_ERR_STAT, "bd96801-ldo5-uvp-err"),
+	DEFINE_RES_IRQ_NAMED(BD96801_LDO5_SHDN_ERR_STAT, "bd96801-ldo5-shdn-err"),
+
+	DEFINE_RES_IRQ_NAMED(BD96801_LDO6_PVIN_ERR_STAT, "bd96801-ldo6-pvin-err"),
+	DEFINE_RES_IRQ_NAMED(BD96801_LDO6_OVP_ERR_STAT, "bd96801-ldo6-ovp-err"),
+	DEFINE_RES_IRQ_NAMED(BD96801_LDO6_UVP_ERR_STAT, "bd96801-ldo6-uvp-err"),
+	DEFINE_RES_IRQ_NAMED(BD96801_LDO6_SHDN_ERR_STAT, "bd96801-ldo6-shdn-err"),
+
+	DEFINE_RES_IRQ_NAMED(BD96801_LDO7_PVIN_ERR_STAT, "bd96801-ldo7-pvin-err"),
+	DEFINE_RES_IRQ_NAMED(BD96801_LDO7_OVP_ERR_STAT, "bd96801-ldo7-ovp-err"),
+	DEFINE_RES_IRQ_NAMED(BD96801_LDO7_UVP_ERR_STAT, "bd96801-ldo7-uvp-err"),
+	DEFINE_RES_IRQ_NAMED(BD96801_LDO7_SHDN_ERR_STAT, "bd96801-ldo7-shdn-err"),
+};
+
+static const struct resource regulator_intb_irqs[] = {
+	DEFINE_RES_IRQ_NAMED(BD96801_TW_STAT, "bd96801-core-thermal"),
+
+	DEFINE_RES_IRQ_NAMED(BD96801_BUCK1_OCPH_STAT, "bd96801-buck1-overcurr-h"),
+	DEFINE_RES_IRQ_NAMED(BD96801_BUCK1_OCPL_STAT, "bd96801-buck1-overcurr-l"),
+	DEFINE_RES_IRQ_NAMED(BD96801_BUCK1_OCPN_STAT, "bd96801-buck1-overcurr-n"),
+	DEFINE_RES_IRQ_NAMED(BD96801_BUCK1_OVD_STAT, "bd96801-buck1-overvolt"),
+	DEFINE_RES_IRQ_NAMED(BD96801_BUCK1_UVD_STAT, "bd96801-buck1-undervolt"),
+	DEFINE_RES_IRQ_NAMED(BD96801_BUCK1_TW_CH_STAT, "bd96801-buck1-thermal"),
+
+	DEFINE_RES_IRQ_NAMED(BD96801_BUCK2_OCPH_STAT, "bd96801-buck2-overcurr-h"),
+	DEFINE_RES_IRQ_NAMED(BD96801_BUCK2_OCPL_STAT, "bd96801-buck2-overcurr-l"),
+	DEFINE_RES_IRQ_NAMED(BD96801_BUCK2_OCPN_STAT, "bd96801-buck2-overcurr-n"),
+	DEFINE_RES_IRQ_NAMED(BD96801_BUCK2_OVD_STAT, "bd96801-buck2-overvolt"),
+	DEFINE_RES_IRQ_NAMED(BD96801_BUCK2_UVD_STAT, "bd96801-buck2-undervolt"),
+	DEFINE_RES_IRQ_NAMED(BD96801_BUCK2_TW_CH_STAT, "bd96801-buck2-thermal"),
+
+	DEFINE_RES_IRQ_NAMED(BD96801_BUCK3_OCPH_STAT, "bd96801-buck3-overcurr-h"),
+	DEFINE_RES_IRQ_NAMED(BD96801_BUCK3_OCPL_STAT, "bd96801-buck3-overcurr-l"),
+	DEFINE_RES_IRQ_NAMED(BD96801_BUCK3_OCPN_STAT, "bd96801-buck3-overcurr-n"),
+	DEFINE_RES_IRQ_NAMED(BD96801_BUCK3_OVD_STAT, "bd96801-buck3-overvolt"),
+	DEFINE_RES_IRQ_NAMED(BD96801_BUCK3_UVD_STAT, "bd96801-buck3-undervolt"),
+	DEFINE_RES_IRQ_NAMED(BD96801_BUCK3_TW_CH_STAT, "bd96801-buck3-thermal"),
+
+	DEFINE_RES_IRQ_NAMED(BD96801_BUCK4_OCPH_STAT, "bd96801-buck4-overcurr-h"),
+	DEFINE_RES_IRQ_NAMED(BD96801_BUCK4_OCPL_STAT, "bd96801-buck4-overcurr-l"),
+	DEFINE_RES_IRQ_NAMED(BD96801_BUCK4_OCPN_STAT, "bd96801-buck4-overcurr-n"),
+	DEFINE_RES_IRQ_NAMED(BD96801_BUCK4_OVD_STAT, "bd96801-buck4-overvolt"),
+	DEFINE_RES_IRQ_NAMED(BD96801_BUCK4_UVD_STAT, "bd96801-buck4-undervolt"),
+	DEFINE_RES_IRQ_NAMED(BD96801_BUCK4_TW_CH_STAT, "bd96801-buck4-thermal"),
+
+	DEFINE_RES_IRQ_NAMED(BD96801_LDO5_OCPH_STAT, "bd96801-ldo5-overcurr"),
+	DEFINE_RES_IRQ_NAMED(BD96801_LDO5_OVD_STAT, "bd96801-ldo5-overvolt"),
+	DEFINE_RES_IRQ_NAMED(BD96801_LDO5_UVD_STAT, "bd96801-ldo5-undervolt"),
+
+	DEFINE_RES_IRQ_NAMED(BD96801_LDO6_OCPH_STAT, "bd96801-ldo6-overcurr"),
+	DEFINE_RES_IRQ_NAMED(BD96801_LDO6_OVD_STAT, "bd96801-ldo6-overvolt"),
+	DEFINE_RES_IRQ_NAMED(BD96801_LDO6_UVD_STAT, "bd96801-ldo6-undervolt"),
+
+	DEFINE_RES_IRQ_NAMED(BD96801_LDO7_OCPH_STAT, "bd96801-ldo7-overcurr"),
+	DEFINE_RES_IRQ_NAMED(BD96801_LDO7_OVD_STAT, "bd96801-ldo7-overvolt"),
+	DEFINE_RES_IRQ_NAMED(BD96801_LDO7_UVD_STAT, "bd96801-ldo7-undervolt"),
+};
+
+enum {
+	WDG_CELL = 0,
+	REGULATOR_CELL,
+};
+
+static struct mfd_cell bd96801_mfd_cells[] = {
+	[WDG_CELL] = { .name = "bd96801-wdt", },
+	[REGULATOR_CELL] = { .name = "bd96801-pmic", },
+};
+
+static const struct regmap_range bd96801_volatile_ranges[] = {
+	/* Status regs */
+	regmap_reg_range(BD96801_REG_WD_FEED, BD96801_REG_WD_FAILCOUNT),
+	regmap_reg_range(BD96801_REG_WD_ASK, BD96801_REG_WD_ASK),
+	regmap_reg_range(BD96801_REG_WD_STATUS, BD96801_REG_WD_STATUS),
+	regmap_reg_range(BD96801_REG_PMIC_STATE, BD96801_REG_INT_LDO7_INTB),
+	/* Registers which do not update value unless PMIC is in STBY */
+	regmap_reg_range(BD96801_REG_SSCG_CTRL, BD96801_REG_SHD_INTB),
+	regmap_reg_range(BD96801_REG_BUCK_OVP, BD96801_REG_BOOT_OVERTIME),
+	/*
+	 * LDO control registers have single bit (LDO MODE) which does not
+	 * change when we write it unless PMIC is in STBY. It's safer to not
+	 * cache it.
+	 */
+	regmap_reg_range(BD96801_LDO5_VOL_LVL_REG, BD96801_LDO7_VOL_LVL_REG),
+};
+
+static const struct regmap_access_table volatile_regs = {
+	.yes_ranges = bd96801_volatile_ranges,
+	.n_yes_ranges = ARRAY_SIZE(bd96801_volatile_ranges),
+};
+
+/*
+ * For ERRB we need main register bit mapping as bit(0) indicates active IRQ
+ * in one of the first 3 sub IRQ registers, For INTB we can use default 1 to 1
+ * mapping.
+ */
+static unsigned int bit0_offsets[] = {0, 1, 2};	/* System stat, 3 registers */
+static unsigned int bit1_offsets[] = {3};	/* Buck 1 stat */
+static unsigned int bit2_offsets[] = {4};	/* Buck 2 stat */
+static unsigned int bit3_offsets[] = {5};	/* Buck 3 stat */
+static unsigned int bit4_offsets[] = {6};	/* Buck 4 stat */
+static unsigned int bit5_offsets[] = {7};	/* LDO 5 stat */
+static unsigned int bit6_offsets[] = {8};	/* LDO 6 stat */
+static unsigned int bit7_offsets[] = {9};	/* LDO 7 stat */
+
+static struct regmap_irq_sub_irq_map errb_sub_irq_offsets[] = {
+	REGMAP_IRQ_MAIN_REG_OFFSET(bit0_offsets),
+	REGMAP_IRQ_MAIN_REG_OFFSET(bit1_offsets),
+	REGMAP_IRQ_MAIN_REG_OFFSET(bit2_offsets),
+	REGMAP_IRQ_MAIN_REG_OFFSET(bit3_offsets),
+	REGMAP_IRQ_MAIN_REG_OFFSET(bit4_offsets),
+	REGMAP_IRQ_MAIN_REG_OFFSET(bit5_offsets),
+	REGMAP_IRQ_MAIN_REG_OFFSET(bit6_offsets),
+	REGMAP_IRQ_MAIN_REG_OFFSET(bit7_offsets),
+};
+
+static const struct regmap_irq bd96801_errb_irqs[] = {
+	/* Reg 0x52 Fatal ERRB1 */
+	REGMAP_IRQ_REG(BD96801_OTP_ERR_STAT, 0, BD96801_OTP_ERR_MASK),
+	REGMAP_IRQ_REG(BD96801_DBIST_ERR_STAT, 0, BD96801_DBIST_ERR_MASK),
+	REGMAP_IRQ_REG(BD96801_EEP_ERR_STAT, 0, BD96801_EEP_ERR_MASK),
+	REGMAP_IRQ_REG(BD96801_ABIST_ERR_STAT, 0, BD96801_ABIST_ERR_MASK),
+	REGMAP_IRQ_REG(BD96801_PRSTB_ERR_STAT, 0, BD96801_PRSTB_ERR_MASK),
+	REGMAP_IRQ_REG(BD96801_DRMOS1_ERR_STAT, 0, BD96801_DRMOS1_ERR_MASK),
+	REGMAP_IRQ_REG(BD96801_DRMOS2_ERR_STAT, 0, BD96801_DRMOS2_ERR_MASK),
+	REGMAP_IRQ_REG(BD96801_SLAVE_ERR_STAT, 0, BD96801_SLAVE_ERR_MASK),
+	/* 0x53 Fatal ERRB2 */
+	REGMAP_IRQ_REG(BD96801_VREF_ERR_STAT, 1, BD96801_VREF_ERR_MASK),
+	REGMAP_IRQ_REG(BD96801_TSD_ERR_STAT, 1, BD96801_TSD_ERR_MASK),
+	REGMAP_IRQ_REG(BD96801_UVLO_ERR_STAT, 1, BD96801_UVLO_ERR_MASK),
+	REGMAP_IRQ_REG(BD96801_OVLO_ERR_STAT, 1, BD96801_OVLO_ERR_MASK),
+	REGMAP_IRQ_REG(BD96801_OSC_ERR_STAT, 1, BD96801_OSC_ERR_MASK),
+	REGMAP_IRQ_REG(BD96801_PON_ERR_STAT, 1, BD96801_PON_ERR_MASK),
+	REGMAP_IRQ_REG(BD96801_POFF_ERR_STAT, 1, BD96801_POFF_ERR_MASK),
+	REGMAP_IRQ_REG(BD96801_CMD_SHDN_ERR_STAT, 1, BD96801_CMD_SHDN_ERR_MASK),
+	/* 0x54 Fatal INTB shadowed to ERRB */
+	REGMAP_IRQ_REG(BD96801_INT_PRSTB_WDT_ERR, 2, BD96801_INT_PRSTB_WDT_ERR_MASK),
+	REGMAP_IRQ_REG(BD96801_INT_CHIP_IF_ERR, 2, BD96801_INT_CHIP_IF_ERR_MASK),
+	REGMAP_IRQ_REG(BD96801_INT_SHDN_ERR_STAT, 2, BD96801_INT_SHDN_ERR_MASK),
+	/* Reg 0x55 BUCK1 ERR IRQs */
+	REGMAP_IRQ_REG(BD96801_BUCK1_PVIN_ERR_STAT, 3, BD96801_OUT_PVIN_ERR_MASK),
+	REGMAP_IRQ_REG(BD96801_BUCK1_OVP_ERR_STAT, 3, BD96801_OUT_OVP_ERR_MASK),
+	REGMAP_IRQ_REG(BD96801_BUCK1_UVP_ERR_STAT, 3, BD96801_OUT_UVP_ERR_MASK),
+	REGMAP_IRQ_REG(BD96801_BUCK1_SHDN_ERR_STAT, 3, BD96801_OUT_SHDN_ERR_MASK),
+	/* Reg 0x56 BUCK2 ERR IRQs */
+	REGMAP_IRQ_REG(BD96801_BUCK2_PVIN_ERR_STAT, 4, BD96801_OUT_PVIN_ERR_MASK),
+	REGMAP_IRQ_REG(BD96801_BUCK2_OVP_ERR_STAT, 4, BD96801_OUT_OVP_ERR_MASK),
+	REGMAP_IRQ_REG(BD96801_BUCK2_UVP_ERR_STAT, 4, BD96801_OUT_UVP_ERR_MASK),
+	REGMAP_IRQ_REG(BD96801_BUCK2_SHDN_ERR_STAT, 4, BD96801_OUT_SHDN_ERR_MASK),
+	/* Reg 0x57 BUCK3 ERR IRQs */
+	REGMAP_IRQ_REG(BD96801_BUCK3_PVIN_ERR_STAT, 5, BD96801_OUT_PVIN_ERR_MASK),
+	REGMAP_IRQ_REG(BD96801_BUCK3_OVP_ERR_STAT, 5, BD96801_OUT_OVP_ERR_MASK),
+	REGMAP_IRQ_REG(BD96801_BUCK3_UVP_ERR_STAT, 5, BD96801_OUT_UVP_ERR_MASK),
+	REGMAP_IRQ_REG(BD96801_BUCK3_SHDN_ERR_STAT, 5, BD96801_OUT_SHDN_ERR_MASK),
+	/* Reg 0x58 BUCK4 ERR IRQs */
+	REGMAP_IRQ_REG(BD96801_BUCK4_PVIN_ERR_STAT, 6, BD96801_OUT_PVIN_ERR_MASK),
+	REGMAP_IRQ_REG(BD96801_BUCK4_OVP_ERR_STAT, 6, BD96801_OUT_OVP_ERR_MASK),
+	REGMAP_IRQ_REG(BD96801_BUCK4_UVP_ERR_STAT, 6, BD96801_OUT_UVP_ERR_MASK),
+	REGMAP_IRQ_REG(BD96801_BUCK4_SHDN_ERR_STAT, 6, BD96801_OUT_SHDN_ERR_MASK),
+	/* Reg 0x59 LDO5 ERR IRQs */
+	REGMAP_IRQ_REG(BD96801_LDO5_PVIN_ERR_STAT, 7, BD96801_OUT_PVIN_ERR_MASK),
+	REGMAP_IRQ_REG(BD96801_LDO5_OVP_ERR_STAT, 7, BD96801_OUT_OVP_ERR_MASK),
+	REGMAP_IRQ_REG(BD96801_LDO5_UVP_ERR_STAT, 7, BD96801_OUT_UVP_ERR_MASK),
+	REGMAP_IRQ_REG(BD96801_LDO5_SHDN_ERR_STAT, 7, BD96801_OUT_SHDN_ERR_MASK),
+	/* Reg 0x5a LDO6 ERR IRQs */
+	REGMAP_IRQ_REG(BD96801_LDO6_PVIN_ERR_STAT, 8, BD96801_OUT_PVIN_ERR_MASK),
+	REGMAP_IRQ_REG(BD96801_LDO6_OVP_ERR_STAT, 8, BD96801_OUT_OVP_ERR_MASK),
+	REGMAP_IRQ_REG(BD96801_LDO6_UVP_ERR_STAT, 8, BD96801_OUT_UVP_ERR_MASK),
+	REGMAP_IRQ_REG(BD96801_LDO6_SHDN_ERR_STAT, 8, BD96801_OUT_SHDN_ERR_MASK),
+	/* Reg 0x5b LDO7 ERR IRQs */
+	REGMAP_IRQ_REG(BD96801_LDO7_PVIN_ERR_STAT, 9, BD96801_OUT_PVIN_ERR_MASK),
+	REGMAP_IRQ_REG(BD96801_LDO7_OVP_ERR_STAT, 9, BD96801_OUT_OVP_ERR_MASK),
+	REGMAP_IRQ_REG(BD96801_LDO7_UVP_ERR_STAT, 9, BD96801_OUT_UVP_ERR_MASK),
+	REGMAP_IRQ_REG(BD96801_LDO7_SHDN_ERR_STAT, 9, BD96801_OUT_SHDN_ERR_MASK),
+};
+
+static const struct regmap_irq bd96801_intb_irqs[] = {
+	/* STATUS SYSTEM INTB */
+	REGMAP_IRQ_REG(BD96801_TW_STAT, 0, BD96801_TW_STAT_MASK),
+	REGMAP_IRQ_REG(BD96801_WDT_ERR_STAT, 0, BD96801_WDT_ERR_STAT_MASK),
+	REGMAP_IRQ_REG(BD96801_I2C_ERR_STAT, 0, BD96801_I2C_ERR_STAT_MASK),
+	REGMAP_IRQ_REG(BD96801_CHIP_IF_ERR_STAT, 0, BD96801_CHIP_IF_ERR_STAT_MASK),
+	/* STATUS BUCK1 INTB */
+	REGMAP_IRQ_REG(BD96801_BUCK1_OCPH_STAT, 1, BD96801_BUCK_OCPH_STAT_MASK),
+	REGMAP_IRQ_REG(BD96801_BUCK1_OCPL_STAT, 1, BD96801_BUCK_OCPL_STAT_MASK),
+	REGMAP_IRQ_REG(BD96801_BUCK1_OCPN_STAT, 1, BD96801_BUCK_OCPN_STAT_MASK),
+	REGMAP_IRQ_REG(BD96801_BUCK1_OVD_STAT, 1, BD96801_BUCK_OVD_STAT_MASK),
+	REGMAP_IRQ_REG(BD96801_BUCK1_UVD_STAT, 1, BD96801_BUCK_UVD_STAT_MASK),
+	REGMAP_IRQ_REG(BD96801_BUCK1_TW_CH_STAT, 1, BD96801_BUCK_TW_CH_STAT_MASK),
+	/* BUCK 2 INTB */
+	REGMAP_IRQ_REG(BD96801_BUCK2_OCPH_STAT, 2, BD96801_BUCK_OCPH_STAT_MASK),
+	REGMAP_IRQ_REG(BD96801_BUCK2_OCPL_STAT, 2, BD96801_BUCK_OCPL_STAT_MASK),
+	REGMAP_IRQ_REG(BD96801_BUCK2_OCPN_STAT, 2, BD96801_BUCK_OCPN_STAT_MASK),
+	REGMAP_IRQ_REG(BD96801_BUCK2_OVD_STAT, 2, BD96801_BUCK_OVD_STAT_MASK),
+	REGMAP_IRQ_REG(BD96801_BUCK2_UVD_STAT, 2, BD96801_BUCK_UVD_STAT_MASK),
+	REGMAP_IRQ_REG(BD96801_BUCK2_TW_CH_STAT, 2, BD96801_BUCK_TW_CH_STAT_MASK),
+	/* BUCK 3 INTB */
+	REGMAP_IRQ_REG(BD96801_BUCK3_OCPH_STAT, 3, BD96801_BUCK_OCPH_STAT_MASK),
+	REGMAP_IRQ_REG(BD96801_BUCK3_OCPL_STAT, 3, BD96801_BUCK_OCPL_STAT_MASK),
+	REGMAP_IRQ_REG(BD96801_BUCK3_OCPN_STAT, 3, BD96801_BUCK_OCPN_STAT_MASK),
+	REGMAP_IRQ_REG(BD96801_BUCK3_OVD_STAT, 3, BD96801_BUCK_OVD_STAT_MASK),
+	REGMAP_IRQ_REG(BD96801_BUCK3_UVD_STAT, 3, BD96801_BUCK_UVD_STAT_MASK),
+	REGMAP_IRQ_REG(BD96801_BUCK3_TW_CH_STAT, 3, BD96801_BUCK_TW_CH_STAT_MASK),
+	/* BUCK 4 INTB */
+	REGMAP_IRQ_REG(BD96801_BUCK4_OCPH_STAT, 4, BD96801_BUCK_OCPH_STAT_MASK),
+	REGMAP_IRQ_REG(BD96801_BUCK4_OCPL_STAT, 4, BD96801_BUCK_OCPL_STAT_MASK),
+	REGMAP_IRQ_REG(BD96801_BUCK4_OCPN_STAT, 4, BD96801_BUCK_OCPN_STAT_MASK),
+	REGMAP_IRQ_REG(BD96801_BUCK4_OVD_STAT, 4, BD96801_BUCK_OVD_STAT_MASK),
+	REGMAP_IRQ_REG(BD96801_BUCK4_UVD_STAT, 4, BD96801_BUCK_UVD_STAT_MASK),
+	REGMAP_IRQ_REG(BD96801_BUCK4_TW_CH_STAT, 4, BD96801_BUCK_TW_CH_STAT_MASK),
+	/* LDO5 INTB */
+	REGMAP_IRQ_REG(BD96801_LDO5_OCPH_STAT, 5, BD96801_LDO_OCPH_STAT_MASK),
+	REGMAP_IRQ_REG(BD96801_LDO5_OVD_STAT, 5, BD96801_LDO_OVD_STAT_MASK),
+	REGMAP_IRQ_REG(BD96801_LDO5_UVD_STAT, 5, BD96801_LDO_UVD_STAT_MASK),
+	/* LDO6 INTB */
+	REGMAP_IRQ_REG(BD96801_LDO6_OCPH_STAT, 6, BD96801_LDO_OCPH_STAT_MASK),
+	REGMAP_IRQ_REG(BD96801_LDO6_OVD_STAT, 6, BD96801_LDO_OVD_STAT_MASK),
+	REGMAP_IRQ_REG(BD96801_LDO6_UVD_STAT, 6, BD96801_LDO_UVD_STAT_MASK),
+	/* LDO7 INTB */
+	REGMAP_IRQ_REG(BD96801_LDO7_OCPH_STAT, 7, BD96801_LDO_OCPH_STAT_MASK),
+	REGMAP_IRQ_REG(BD96801_LDO7_OVD_STAT, 7, BD96801_LDO_OVD_STAT_MASK),
+	REGMAP_IRQ_REG(BD96801_LDO7_UVD_STAT, 7, BD96801_LDO_UVD_STAT_MASK),
+};
+
+static struct regmap_irq_chip bd96801_irq_chip_errb = {
+	.name = "bd96801-irq-errb",
+	.main_status = BD96801_REG_INT_MAIN,
+	.num_main_regs = 1,
+	.irqs = &bd96801_errb_irqs[0],
+	.num_irqs = ARRAY_SIZE(bd96801_errb_irqs),
+	.status_base = BD96801_REG_INT_SYS_ERRB1,
+	.mask_base = BD96801_REG_MASK_SYS_ERRB,
+	.ack_base = BD96801_REG_INT_SYS_ERRB1,
+	.init_ack_masked = true,
+	.num_regs = 10,
+	.irq_reg_stride = 1,
+	.sub_reg_offsets = &errb_sub_irq_offsets[0],
+};
+
+static struct regmap_irq_chip bd96801_irq_chip_intb = {
+	.name = "bd96801-irq-intb",
+	.main_status = BD96801_REG_INT_MAIN,
+	.num_main_regs = 1,
+	.irqs = &bd96801_intb_irqs[0],
+	.num_irqs = ARRAY_SIZE(bd96801_intb_irqs),
+	.status_base = BD96801_REG_INT_SYS_INTB,
+	.mask_base = BD96801_REG_MASK_SYS_INTB,
+	.ack_base = BD96801_REG_INT_SYS_INTB,
+	.init_ack_masked = true,
+	.num_regs = 8,
+	.irq_reg_stride = 1,
+};
+
+static const struct regmap_config bd96801_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+	.volatile_table = &volatile_regs,
+	.cache_type = REGCACHE_RBTREE,
+};
+
+static int bd96801_i2c_probe(struct i2c_client *i2c)
+{
+	int i, ret, intb_irq, errb_irq, num_regu_irqs, num_intb, num_errb = 0;
+	struct regmap_irq_chip_data *intb_irq_data, *errb_irq_data;
+	struct irq_domain *intb_domain, *errb_domain;
+	const struct fwnode_handle *fwnode;
+	struct resource *regulator_res;
+	struct regmap *regmap;
+
+	fwnode = dev_fwnode(&i2c->dev);
+	if (!fwnode) {
+		dev_err(&i2c->dev, "no fwnode\n");
+		return -EINVAL;
+	}
+
+	intb_irq = fwnode_irq_get_byname(fwnode, "intb");
+	if (intb_irq < 0)
+		return dev_err_probe(&i2c->dev, intb_irq,
+				     "No INTB IRQ configured\n");
+
+	num_intb =  ARRAY_SIZE(regulator_intb_irqs);
+
+	/* ERRB may be omitted if processor is powered by the PMIC */
+	errb_irq = fwnode_irq_get_byname(fwnode, "errb");
+	if (errb_irq < 0)
+		errb_irq = 0;
+
+	if (errb_irq)
+		num_errb = ARRAY_SIZE(regulator_errb_irqs);
+
+	num_regu_irqs = num_intb + num_errb;
+
+	regulator_res = kcalloc(num_regu_irqs, sizeof(*regulator_res),
+				GFP_KERNEL);
+	if (!regulator_res)
+		return -ENOMEM;
+
+	regmap = devm_regmap_init_i2c(i2c, &bd96801_regmap_config);
+	if (IS_ERR(regmap)) {
+		ret = dev_err_probe(&i2c->dev, PTR_ERR(regmap),
+				    "regmap initialization failed\n");
+		goto free_out;
+	}
+	ret = devm_regmap_add_irq_chip(&i2c->dev, regmap, intb_irq,
+				       IRQF_ONESHOT, 0, &bd96801_irq_chip_intb,
+				       &intb_irq_data);
+	if (ret) {
+		dev_err_probe(&i2c->dev, ret, "Failed to add INTB irq_chip\n");
+		goto free_out;
+	}
+
+	/*
+	 * MFD core code is built to handle only one IRQ domain. BD96801
+	 * has two domains so we do IRQ mapping here and provide the
+	 * already mapped IRQ numbers to sub-devices.
+	 */
+	intb_domain = regmap_irq_get_domain(intb_irq_data);
+
+	for (i = 0; i < num_intb; i++) {
+		struct resource *res = &regulator_res[i];
+
+		*res = regulator_intb_irqs[i];
+		res->start = res->end = irq_create_mapping(intb_domain,
+							    res->start);
+	}
+
+	if (num_errb) {
+		ret = devm_regmap_add_irq_chip(&i2c->dev, regmap, errb_irq,
+					       IRQF_ONESHOT, 0,
+					       &bd96801_irq_chip_errb,
+					       &errb_irq_data);
+		if (ret) {
+			dev_err_probe(&i2c->dev, ret,
+				      "Failed to add ERRB (%d) irq_chip\n",
+				      errb_irq);
+			goto free_out;
+		}
+		errb_domain = regmap_irq_get_domain(errb_irq_data);
+
+		for (i = 0; i < num_errb; i++) {
+			struct resource *res = &regulator_res[num_intb + i];
+
+			*res = regulator_errb_irqs[i];
+			res->start = res->end = irq_create_mapping(errb_domain,
+								   res->start);
+		}
+	}
+
+	bd96801_mfd_cells[REGULATOR_CELL].resources = regulator_res;
+	bd96801_mfd_cells[REGULATOR_CELL].num_resources = num_regu_irqs;
+	ret = devm_mfd_add_devices(&i2c->dev, PLATFORM_DEVID_AUTO,
+				   bd96801_mfd_cells,
+				   ARRAY_SIZE(bd96801_mfd_cells), NULL, 0, NULL);
+	if (ret)
+		dev_err_probe(&i2c->dev, ret, "Failed to create subdevices\n");
+
+free_out:
+	kfree(regulator_res);
+
+	return ret;
+}
+
+static const struct of_device_id bd96801_of_match[] = {
+	{
+		.compatible = "rohm,bd96801",
+	},
+	{ }
+};
+MODULE_DEVICE_TABLE(of, bd96801_of_match);
+
+static struct i2c_driver bd96801_i2c_driver = {
+	.driver = {
+		.name = "rohm-bd96801",
+		.of_match_table = bd96801_of_match,
+	},
+	.probe = bd96801_i2c_probe,
+};
+
+static int __init bd96801_i2c_init(void)
+{
+	return i2c_add_driver(&bd96801_i2c_driver);
+}
+/* Initialise early so consumer devices can complete system boot */
+subsys_initcall(bd96801_i2c_init);
+
+static void __exit bd96801_i2c_exit(void)
+{
+	i2c_del_driver(&bd96801_i2c_driver);
+}
+module_exit(bd96801_i2c_exit);
+
+MODULE_AUTHOR("Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>");
+MODULE_DESCRIPTION("ROHM BD96801 Power Management IC driver");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/mfd/rohm-bd96801.h b/include/linux/mfd/rohm-bd96801.h
new file mode 100644
index 000000000000..47b07171dcb2
--- /dev/null
+++ b/include/linux/mfd/rohm-bd96801.h
@@ -0,0 +1,212 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/* Copyright (C) 2020 ROHM Semiconductors */
+
+#ifndef __LINUX_MFD_BD96801_H__
+#define __LINUX_MFD_BD96801_H__
+
+#define BD96801_REG_SSCG_CTRL		0x09
+#define BD96801_REG_SHD_INTB            0x20
+#define BD96801_LDO5_VOL_LVL_REG	0x2c
+#define BD96801_LDO6_VOL_LVL_REG	0x2d
+#define BD96801_LDO7_VOL_LVL_REG	0x2e
+#define BD96801_REG_BUCK_OVP		0x30
+#define BD96801_REG_BUCK_OVD		0x35
+#define BD96801_REG_LDO_OVP		0x31
+#define BD96801_REG_LDO_OVD		0x36
+#define BD96801_REG_BOOT_OVERTIME	0x3a
+#define BD96801_REG_WD_TMO		0x40
+#define BD96801_REG_WD_CONF		0x41
+#define BD96801_REG_WD_FEED		0x42
+#define BD96801_REG_WD_FAILCOUNT	0x43
+#define BD96801_REG_WD_ASK		0x46
+#define BD96801_REG_WD_STATUS		0x4a
+#define BD96801_REG_PMIC_STATE		0x4f
+#define BD96801_REG_EXT_STATE		0x50
+
+#define BD96801_STATE_STBY		0x09
+
+/* IRQ register area */
+#define BD96801_REG_INT_MAIN		0x51
+
+/*
+ * The BD96801 has two physical IRQ lines, INTB and ERRB.
+ * For now we just handle the INTB.
+ *
+ * The 'main status register' is located at 0x51.
+ * The ERRB status registers are located at 0x52 ... 0x5B
+ * INTB status registers are at range 0x5c ... 0x63
+ */
+#define BD96801_REG_INT_SYS_ERRB1	0x52
+#define BD96801_REG_INT_SYS_INTB	0x5c
+#define BD96801_REG_INT_LDO7_INTB	0x63
+
+/* MASK registers */
+#define BD96801_REG_MASK_SYS_INTB	0x73
+#define BD96801_REG_MASK_SYS_ERRB	0x69
+
+#define BD96801_MAX_REGISTER		0x7a
+
+#define BD96801_OTP_ERR_MASK		BIT(0)
+#define BD96801_DBIST_ERR_MASK		BIT(1)
+#define BD96801_EEP_ERR_MASK		BIT(2)
+#define BD96801_ABIST_ERR_MASK		BIT(3)
+#define BD96801_PRSTB_ERR_MASK		BIT(4)
+#define BD96801_DRMOS1_ERR_MASK		BIT(5)
+#define BD96801_DRMOS2_ERR_MASK		BIT(6)
+#define BD96801_SLAVE_ERR_MASK		BIT(7)
+#define BD96801_VREF_ERR_MASK		BIT(0)
+#define BD96801_TSD_ERR_MASK		BIT(1)
+#define BD96801_UVLO_ERR_MASK		BIT(2)
+#define BD96801_OVLO_ERR_MASK		BIT(3)
+#define BD96801_OSC_ERR_MASK		BIT(4)
+#define BD96801_PON_ERR_MASK		BIT(5)
+#define BD96801_POFF_ERR_MASK		BIT(6)
+#define BD96801_CMD_SHDN_ERR_MASK	BIT(7)
+#define BD96801_INT_PRSTB_WDT_ERR_MASK	BIT(0)
+#define BD96801_INT_CHIP_IF_ERR_MASK	BIT(3)
+#define BD96801_INT_SHDN_ERR_MASK	BIT(7)
+#define BD96801_OUT_PVIN_ERR_MASK	BIT(0)
+#define BD96801_OUT_OVP_ERR_MASK	BIT(1)
+#define BD96801_OUT_UVP_ERR_MASK	BIT(2)
+#define BD96801_OUT_SHDN_ERR_MASK	BIT(7)
+
+/* ERRB IRQs */
+enum {
+	/* Reg 0x52, 0x53, 0x54 - ERRB system IRQs */
+	BD96801_OTP_ERR_STAT,
+	BD96801_DBIST_ERR_STAT,
+	BD96801_EEP_ERR_STAT,
+	BD96801_ABIST_ERR_STAT,
+	BD96801_PRSTB_ERR_STAT,
+	BD96801_DRMOS1_ERR_STAT,
+	BD96801_DRMOS2_ERR_STAT,
+	BD96801_SLAVE_ERR_STAT,
+	BD96801_VREF_ERR_STAT,
+	BD96801_TSD_ERR_STAT,
+	BD96801_UVLO_ERR_STAT,
+	BD96801_OVLO_ERR_STAT,
+	BD96801_OSC_ERR_STAT,
+	BD96801_PON_ERR_STAT,
+	BD96801_POFF_ERR_STAT,
+	BD96801_CMD_SHDN_ERR_STAT,
+	BD96801_INT_PRSTB_WDT_ERR,
+	BD96801_INT_CHIP_IF_ERR,
+	BD96801_INT_SHDN_ERR_STAT,
+
+	/* Reg 0x55 BUCK1 ERR IRQs */
+	BD96801_BUCK1_PVIN_ERR_STAT,
+	BD96801_BUCK1_OVP_ERR_STAT,
+	BD96801_BUCK1_UVP_ERR_STAT,
+	BD96801_BUCK1_SHDN_ERR_STAT,
+
+	/* Reg 0x56 BUCK2 ERR IRQs */
+	BD96801_BUCK2_PVIN_ERR_STAT,
+	BD96801_BUCK2_OVP_ERR_STAT,
+	BD96801_BUCK2_UVP_ERR_STAT,
+	BD96801_BUCK2_SHDN_ERR_STAT,
+
+	/* Reg 0x57 BUCK3 ERR IRQs */
+	BD96801_BUCK3_PVIN_ERR_STAT,
+	BD96801_BUCK3_OVP_ERR_STAT,
+	BD96801_BUCK3_UVP_ERR_STAT,
+	BD96801_BUCK3_SHDN_ERR_STAT,
+
+	/* Reg 0x58 BUCK4 ERR IRQs */
+	BD96801_BUCK4_PVIN_ERR_STAT,
+	BD96801_BUCK4_OVP_ERR_STAT,
+	BD96801_BUCK4_UVP_ERR_STAT,
+	BD96801_BUCK4_SHDN_ERR_STAT,
+
+	/* Reg 0x59 LDO5 ERR IRQs */
+	BD96801_LDO5_PVIN_ERR_STAT,
+	BD96801_LDO5_OVP_ERR_STAT,
+	BD96801_LDO5_UVP_ERR_STAT,
+	BD96801_LDO5_SHDN_ERR_STAT,
+
+	/* Reg 0x5a LDO6 ERR IRQs */
+	BD96801_LDO6_PVIN_ERR_STAT,
+	BD96801_LDO6_OVP_ERR_STAT,
+	BD96801_LDO6_UVP_ERR_STAT,
+	BD96801_LDO6_SHDN_ERR_STAT,
+
+	/* Reg 0x5b LDO7 ERR IRQs */
+	BD96801_LDO7_PVIN_ERR_STAT,
+	BD96801_LDO7_OVP_ERR_STAT,
+	BD96801_LDO7_UVP_ERR_STAT,
+	BD96801_LDO7_SHDN_ERR_STAT,
+};
+
+/* INTB IRQs */
+enum {
+	/* Reg 0x5c (System INTB) */
+	BD96801_TW_STAT,
+	BD96801_WDT_ERR_STAT,
+	BD96801_I2C_ERR_STAT,
+	BD96801_CHIP_IF_ERR_STAT,
+
+	/* Reg 0x5d (BUCK1 INTB) */
+	BD96801_BUCK1_OCPH_STAT,
+	BD96801_BUCK1_OCPL_STAT,
+	BD96801_BUCK1_OCPN_STAT,
+	BD96801_BUCK1_OVD_STAT,
+	BD96801_BUCK1_UVD_STAT,
+	BD96801_BUCK1_TW_CH_STAT,
+
+	/* Reg 0x5e (BUCK2 INTB) */
+	BD96801_BUCK2_OCPH_STAT,
+	BD96801_BUCK2_OCPL_STAT,
+	BD96801_BUCK2_OCPN_STAT,
+	BD96801_BUCK2_OVD_STAT,
+	BD96801_BUCK2_UVD_STAT,
+	BD96801_BUCK2_TW_CH_STAT,
+
+	/* Reg 0x5f (BUCK3 INTB)*/
+	BD96801_BUCK3_OCPH_STAT,
+	BD96801_BUCK3_OCPL_STAT,
+	BD96801_BUCK3_OCPN_STAT,
+	BD96801_BUCK3_OVD_STAT,
+	BD96801_BUCK3_UVD_STAT,
+	BD96801_BUCK3_TW_CH_STAT,
+
+	/* Reg 0x60 (BUCK4 INTB)*/
+	BD96801_BUCK4_OCPH_STAT,
+	BD96801_BUCK4_OCPL_STAT,
+	BD96801_BUCK4_OCPN_STAT,
+	BD96801_BUCK4_OVD_STAT,
+	BD96801_BUCK4_UVD_STAT,
+	BD96801_BUCK4_TW_CH_STAT,
+
+	/* Reg 0x61 (LDO5 INTB) */
+	BD96801_LDO5_OCPH_STAT, //bit [0]
+	BD96801_LDO5_OVD_STAT,	//bit [3]
+	BD96801_LDO5_UVD_STAT,  //bit [4]
+
+	/* Reg 0x62 (LDO6 INTB) */
+	BD96801_LDO6_OCPH_STAT, //bit [0]
+	BD96801_LDO6_OVD_STAT,	//bit [3]
+	BD96801_LDO6_UVD_STAT,  //bit [4]
+
+	/* Reg 0x63 (LDO7 INTB) */
+	BD96801_LDO7_OCPH_STAT, //bit [0]
+	BD96801_LDO7_OVD_STAT,	//bit [3]
+	BD96801_LDO7_UVD_STAT,  //bit [4]
+};
+
+/* IRQ MASKs */
+#define BD96801_TW_STAT_MASK		BIT(0)
+#define BD96801_WDT_ERR_STAT_MASK	BIT(1)
+#define BD96801_I2C_ERR_STAT_MASK	BIT(2)
+#define BD96801_CHIP_IF_ERR_STAT_MASK	BIT(3)
+
+#define BD96801_BUCK_OCPH_STAT_MASK	BIT(0)
+#define BD96801_BUCK_OCPL_STAT_MASK	BIT(1)
+#define BD96801_BUCK_OCPN_STAT_MASK	BIT(2)
+#define BD96801_BUCK_OVD_STAT_MASK	BIT(3)
+#define BD96801_BUCK_UVD_STAT_MASK	BIT(4)
+#define BD96801_BUCK_TW_CH_STAT_MASK	BIT(5)
+
+#define BD96801_LDO_OCPH_STAT_MASK	BIT(0)
+#define BD96801_LDO_OVD_STAT_MASK	BIT(3)
+#define BD96801_LDO_UVD_STAT_MASK	BIT(4)
+
+#endif
diff --git a/include/linux/mfd/rohm-generic.h b/include/linux/mfd/rohm-generic.h
index 4eeb22876bad..e7d4e6afe388 100644
--- a/include/linux/mfd/rohm-generic.h
+++ b/include/linux/mfd/rohm-generic.h
@@ -16,6 +16,7 @@ enum rohm_chip_type {
 	ROHM_CHIP_TYPE_BD71828,
 	ROHM_CHIP_TYPE_BD71837,
 	ROHM_CHIP_TYPE_BD71847,
+	ROHM_CHIP_TYPE_BD96801,
 	ROHM_CHIP_TYPE_AMOUNT
 };
 
-- 
2.43.2


-- 
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND

~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
Simon says - in Latin please.
~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
Thanks to Simon Glass for the translation =] 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[RFC PATCH 5/6] watchdog: ROHM BD96801 PMIC WDG driver

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 12479 bytes --]

Introduce driver for WDG block on ROHM BD96801 scalable PMIC.

This driver only supports watchdog with I2C feeding and delayed
response detection. Whether the watchdog toggles PRSTB pin or
just causes an interrupt can be configured via device-tree.

The BD96801 PMIC HW supports also window watchdog (too early
feeding detection) and Q&A mode. These are not supported by
this driver.

Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
---
 drivers/watchdog/Kconfig       |  13 ++
 drivers/watchdog/Makefile      |   1 +
 drivers/watchdog/bd96801_wdt.c | 375 +++++++++++++++++++++++++++++++++
 3 files changed, 389 insertions(+)
 create mode 100644 drivers/watchdog/bd96801_wdt.c

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 6bee137cfbe0..d97e735e1faa 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -181,6 +181,19 @@ config BD957XMUF_WATCHDOG
 	  watchdog. Alternatively say M to compile the driver as a module,
 	  which will be called bd9576_wdt.
 
+config BD96801_WATCHDOG
+	tristate "ROHM BD96801 PMIC Watchdog"
+	depends on MFD_ROHM_BD96801
+	select WATCHDOG_CORE
+	help
+	  Support for the watchdog in the ROHM BD96801 PMIC. Watchdog can be
+	  configured to only generate IRQ or to trigger system reset via reset
+	  pin.
+
+	  Say Y here to include support for the ROHM BD96801 watchdog.
+	  Alternatively say M to compile the driver as a module,
+	  which will be called bd96801_wdt.
+
 config CROS_EC_WATCHDOG
 	tristate "ChromeOS EC-based watchdog"
 	select WATCHDOG_CORE
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index 3710c218f05e..31bc94436c81 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -217,6 +217,7 @@ obj-$(CONFIG_XEN_WDT) += xen_wdt.o
 
 # Architecture Independent
 obj-$(CONFIG_BD957XMUF_WATCHDOG) += bd9576_wdt.o
+obj-$(CONFIG_BD96801_WATCHDOG) += bd96801_wdt.o
 obj-$(CONFIG_CROS_EC_WATCHDOG) += cros_ec_wdt.o
 obj-$(CONFIG_DA9052_WATCHDOG) += da9052_wdt.o
 obj-$(CONFIG_DA9055_WATCHDOG) += da9055_wdt.o
diff --git a/drivers/watchdog/bd96801_wdt.c b/drivers/watchdog/bd96801_wdt.c
new file mode 100644
index 000000000000..cb2b526ecc21
--- /dev/null
+++ b/drivers/watchdog/bd96801_wdt.c
@@ -0,0 +1,375 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) 2022 ROHM Semiconductors
+ *
+ * ROHM BD96801 watchdog driver
+ */
+
+#include <linux/kernel.h>
+#include <linux/mfd/rohm-bd96801.h>
+#include <linux/mfd/rohm-generic.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/watchdog.h>
+
+static bool nowayout;
+module_param(nowayout, bool, 0);
+MODULE_PARM_DESC(nowayout,
+		"Watchdog cannot be stopped once started (default=\"false\")");
+
+#define BD96801_WD_TMO_SHORT_MASK	0x70
+#define BD96801_WD_RATIO_MASK		0x3
+#define BD96801_WD_TYPE_MASK		0x4
+#define BD96801_WD_TYPE_SLOW		0x4
+#define BD96801_WD_TYPE_WIN		0x0
+
+#define BD96801_WD_EN_MASK		0x3
+#define BD96801_WD_IF_EN		0x1
+#define BD96801_WD_QA_EN		0x2
+#define BD96801_WD_DISABLE		0x0
+
+#define BD96801_WD_ASSERT_MASK		0x8
+#define BD96801_WD_ASSERT_RST		0x8
+#define BD96801_WD_ASSERT_IRQ		0x0
+
+#define BD96801_WD_FEED_MASK		0x1
+#define BD96801_WD_FEED			0x1
+
+/* units in uS */
+#define FASTNG_MIN			3370
+#define BD96801_WDT_DEFAULT_MARGIN	6905120
+/* Unit is seconds */
+#define DEFAULT_TIMEOUT 30
+
+/*
+ * BD96801 WDG supports window mode so the TMO consists of SHORT and LONG
+ * timeout values. SHORT time is meaningfull only in window mode where feeding
+ * period shorter than SHORT would be an error. LONG time is used to detect if
+ * feeding is not occurring within given time limit (SoC SW hangs). The LONG
+ * timeout time is a multiple of (2, 4, 8 0r 16 times) the SHORT timeout.
+ */
+
+struct wdtbd96801 {
+	struct device		*dev;
+	struct regmap		*regmap;
+	bool			always_running;
+	struct watchdog_device	wdt;
+};
+
+static int bd96801_wdt_ping(struct watchdog_device *wdt)
+{
+	struct wdtbd96801 *w = watchdog_get_drvdata(wdt);
+
+	return regmap_update_bits(w->regmap, BD96801_REG_WD_FEED,
+				 BD96801_WD_FEED_MASK, BD96801_WD_FEED);
+}
+
+static int bd96801_wdt_start(struct watchdog_device *wdt)
+{
+	struct wdtbd96801 *w = watchdog_get_drvdata(wdt);
+	int ret;
+
+	ret = regmap_update_bits(w->regmap, BD96801_REG_WD_CONF,
+				 BD96801_WD_EN_MASK, BD96801_WD_IF_EN);
+
+	return ret;
+}
+
+static int bd96801_wdt_stop(struct watchdog_device *wdt)
+{
+	struct wdtbd96801 *w = watchdog_get_drvdata(wdt);
+
+	if (!w->always_running)
+		return regmap_update_bits(w->regmap, BD96801_REG_WD_CONF,
+				 BD96801_WD_EN_MASK, BD96801_WD_DISABLE);
+	set_bit(WDOG_HW_RUNNING, &wdt->status);
+
+	return 0;
+}
+
+static const struct watchdog_info bd96801_wdt_info = {
+	.options	= WDIOF_MAGICCLOSE | WDIOF_KEEPALIVEPING |
+			  WDIOF_SETTIMEOUT,
+	.identity	= "BD96801 Watchdog",
+};
+
+static const struct watchdog_ops bd96801_wdt_ops = {
+	.start		= bd96801_wdt_start,
+	.stop		= bd96801_wdt_stop,
+	.ping		= bd96801_wdt_ping,
+};
+
+static int find_closest_fast(int target, int *sel, int *val)
+{
+	int i;
+	int window = FASTNG_MIN;
+
+	for (i = 0; i < 8 && window < target; i++)
+		window <<= 1;
+
+	*val = window;
+	*sel = i;
+
+	if (i == 8)
+		return -EINVAL;
+
+	return 0;
+}
+
+static int find_closest_slow_by_fast(int fast_val, int *target, int *slowsel)
+{
+	int sel;
+	static const int multipliers[] = {2, 4, 8, 16};
+
+	for (sel = 0; sel < ARRAY_SIZE(multipliers) &&
+	     multipliers[sel] * fast_val < *target; sel++)
+		;
+
+	if (sel == ARRAY_SIZE(multipliers))
+		return -EINVAL;
+
+	*slowsel = sel;
+	*target = multipliers[sel] * fast_val;
+
+	return 0;
+}
+
+static int find_closest_slow(int *target, int *slow_sel, int *fast_sel)
+{
+	static const int multipliers[] = {2, 4, 8, 16};
+	int i, j;
+	int val = 0;
+	int window = FASTNG_MIN;
+
+	for (i = 0; i < 8; i++) {
+		for (j = 0; j < ARRAY_SIZE(multipliers); j++) {
+			int slow;
+
+			slow = window * multipliers[j];
+			if (slow >= *target && (!val || slow < val)) {
+				val = slow;
+				*fast_sel = i;
+				*slow_sel = j;
+			}
+		}
+		window <<= 1;
+	}
+	if (!val)
+		return -EINVAL;
+
+	*target = val;
+
+	return 0;
+}
+
+static int bd96801_set_wdt_mode(struct wdtbd96801 *w, int hw_margin,
+			       int hw_margin_min)
+{
+	int ret, fastng, slowng, type, reg, mask;
+	struct device *dev = w->dev;
+
+	/* convert to uS */
+	hw_margin *= 1000;
+	hw_margin_min *= 1000;
+	if (hw_margin_min) {
+		int min;
+
+		type = BD96801_WD_TYPE_WIN;
+		dev_dbg(dev, "Setting type WINDOW 0x%x\n", type);
+		ret = find_closest_fast(hw_margin_min, &fastng, &min);
+		if (ret) {
+			dev_err(dev, "bad WDT window for fast timeout\n");
+			return ret;
+		}
+
+		ret = find_closest_slow_by_fast(min, &hw_margin, &slowng);
+		if (ret) {
+			dev_err(dev, "bad WDT window\n");
+			return ret;
+		}
+		w->wdt.min_hw_heartbeat_ms = min / 1000;
+	} else {
+		type = BD96801_WD_TYPE_SLOW;
+		dev_dbg(dev, "Setting type SLOW 0x%x\n", type);
+		ret = find_closest_slow(&hw_margin, &slowng, &fastng);
+		if (ret) {
+			dev_err(dev, "bad WDT window\n");
+			return ret;
+		}
+	}
+
+	w->wdt.max_hw_heartbeat_ms = hw_margin / 1000;
+
+	fastng <<= ffs(BD96801_WD_TMO_SHORT_MASK) - 1;
+
+	reg = slowng | fastng;
+	mask = BD96801_WD_RATIO_MASK | BD96801_WD_TMO_SHORT_MASK;
+	ret = regmap_update_bits(w->regmap, BD96801_REG_WD_TMO,
+				 mask, reg);
+	if (ret)
+		return ret;
+
+	ret = regmap_update_bits(w->regmap, BD96801_REG_WD_CONF,
+				 BD96801_WD_TYPE_MASK, type);
+
+	return ret;
+}
+
+static int bd96801_set_heartbeat_from_hw(struct wdtbd96801 *w,
+					 unsigned int conf_reg)
+{
+	int ret;
+	unsigned int val, sel, fast;
+
+	/*
+	 * The BD96801 supports a somewhat peculiar QA-mode, which we do not
+	 * support in this driver. If the QA-mode is enabled then we just
+	 * warn and bail-out.
+	 */
+	if ((conf_reg & BD96801_WD_EN_MASK) != BD96801_WD_IF_EN) {
+		dev_warn(w->dev, "watchdog set to Q&A mode - exiting\n");
+		return -EINVAL;
+	}
+
+	ret = regmap_read(w->regmap, BD96801_REG_WD_TMO, &val);
+	if (ret)
+		return ret;
+
+	sel = val & BD96801_WD_TMO_SHORT_MASK;
+	sel >>= ffs(BD96801_WD_TMO_SHORT_MASK) - 1;
+	fast = FASTNG_MIN << sel;
+
+	sel = (val & BD96801_WD_RATIO_MASK) + 1;
+	w->wdt.max_hw_heartbeat_ms = (fast << sel) / USEC_PER_MSEC;
+
+	if ((conf_reg & BD96801_WD_TYPE_MASK) == BD96801_WD_TYPE_WIN)
+		w->wdt.min_hw_heartbeat_ms = fast / USEC_PER_MSEC;
+
+	return 0;
+}
+
+static int init_wdg_hw(struct wdtbd96801 *w)
+{
+	u32 hw_margin[2];
+	int count, ret;
+	u32 hw_margin_max = BD96801_WDT_DEFAULT_MARGIN, hw_margin_min = 0;
+
+	count = device_property_count_u32(w->dev->parent, "rohm,hw-timeout-ms");
+	if (count < 0 && count != -EINVAL)
+		return count;
+
+	if (count > 0) {
+		if (count > ARRAY_SIZE(hw_margin))
+			return -EINVAL;
+
+		ret = device_property_read_u32_array(w->dev->parent,
+						     "rohm,hw-timeout-ms",
+						     &hw_margin[0], count);
+		if (ret < 0)
+			return ret;
+
+		if (count == 1)
+			hw_margin_max = hw_margin[0];
+
+		if (count == 2) {
+			hw_margin_max = hw_margin[1];
+			hw_margin_min = hw_margin[0];
+		}
+	}
+
+	ret = bd96801_set_wdt_mode(w, hw_margin_max, hw_margin_min);
+	if (ret)
+		return ret;
+
+	ret = device_property_match_string(w->dev->parent, "rohm,wdg-action",
+					   "prstb");
+	if (ret >= 0) {
+		ret = regmap_update_bits(w->regmap, BD96801_REG_WD_CONF,
+				 BD96801_WD_ASSERT_MASK,
+				 BD96801_WD_ASSERT_RST);
+		return ret;
+	}
+
+	ret = device_property_match_string(w->dev->parent, "rohm,wdg-action",
+					   "intb-only");
+	if (ret >= 0) {
+		ret = regmap_update_bits(w->regmap, BD96801_REG_WD_CONF,
+				 BD96801_WD_ASSERT_MASK,
+				 BD96801_WD_ASSERT_IRQ);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int bd96801_wdt_probe(struct platform_device *pdev)
+{
+	struct wdtbd96801 *w;
+	int ret;
+	unsigned int val;
+
+	w = devm_kzalloc(&pdev->dev, sizeof(*w), GFP_KERNEL);
+	if (!w)
+		return -ENOMEM;
+
+	w->regmap = dev_get_regmap(pdev->dev.parent, NULL);
+	w->dev = &pdev->dev;
+
+	w->wdt.info = &bd96801_wdt_info;
+	w->wdt.ops =  &bd96801_wdt_ops;
+	w->wdt.parent = pdev->dev.parent;
+	w->wdt.timeout = DEFAULT_TIMEOUT;
+	watchdog_set_drvdata(&w->wdt, w);
+
+	w->always_running = device_property_read_bool(pdev->dev.parent,
+						      "always-running");
+
+	ret = regmap_read(w->regmap, BD96801_REG_WD_CONF, &val);
+	if (ret)
+		return dev_err_probe(&pdev->dev, ret,
+				     "Failed to get the watchdog state\n");
+
+	/*
+	 * If the WDG is already enabled we assume it is configured by boot.
+	 * In this case we just update the hw-timeout based on values set to
+	 * the timeout / mode registers and leave the hardware configs
+	 * untouched.
+	 */
+	if ((val & BD96801_WD_EN_MASK) != BD96801_WD_DISABLE) {
+		dev_dbg(&pdev->dev, "watchdog was running during probe\n");
+		ret = bd96801_set_heartbeat_from_hw(w, val);
+		if (ret)
+			return ret;
+
+		set_bit(WDOG_HW_RUNNING, &w->wdt.status);
+	} else {
+		/* If WDG is not running so we will initializate it */
+		ret = init_wdg_hw(w);
+		if (ret)
+			return ret;
+	}
+
+	watchdog_init_timeout(&w->wdt, 0, pdev->dev.parent);
+	watchdog_set_nowayout(&w->wdt, nowayout);
+	watchdog_stop_on_reboot(&w->wdt);
+
+	if (w->always_running)
+		bd96801_wdt_start(&w->wdt);
+
+	return devm_watchdog_register_device(&pdev->dev, &w->wdt);
+}
+
+static struct platform_driver bd96801_wdt = {
+	.driver = {
+		.name = "bd96801-wdt"
+	},
+	.probe = bd96801_wdt_probe,
+};
+module_platform_driver(bd96801_wdt);
+
+MODULE_AUTHOR("Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>");
+MODULE_DESCRIPTION("BD96801 watchdog driver");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:bd96801-wdt");
-- 
2.43.2


-- 
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND

~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
Simon says - in Latin please.
~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
Thanks to Simon Glass for the translation =] 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[RFC PATCH 6/6] MAINTAINERS: Add ROHM BD96801 'scalable PMIC' entries

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 1470 bytes --]

Add maintainer entries for ROHM BD96801 a.k.a 'scalable PMIC'

Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
---
 MAINTAINERS | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index aa3b947fb080..da68144d51ae 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -19111,17 +19111,21 @@ F:	drivers/gpio/gpio-bd71828.c
 F:	drivers/mfd/rohm-bd71828.c
 F:	drivers/mfd/rohm-bd718x7.c
 F:	drivers/mfd/rohm-bd9576.c
+F:	drivers/mfd/rohm-bd96801.c
 F:	drivers/regulator/bd71815-regulator.c
 F:	drivers/regulator/bd71828-regulator.c
 F:	drivers/regulator/bd718x7-regulator.c
 F:	drivers/regulator/bd9576-regulator.c
+F:	drivers/regulator/bd96801-regulator.c
 F:	drivers/regulator/rohm-regulator.c
 F:	drivers/rtc/rtc-bd70528.c
 F:	drivers/watchdog/bd9576_wdt.c
+F:	drivers/watchdog/bd96801_wdt.c
 F:	include/linux/mfd/rohm-bd71815.h
 F:	include/linux/mfd/rohm-bd71828.h
 F:	include/linux/mfd/rohm-bd718x7.h
 F:	include/linux/mfd/rohm-bd957x.h
+F:	include/linux/mfd/rohm-bd96801.h
 F:	include/linux/mfd/rohm-generic.h
 F:	include/linux/mfd/rohm-shared.h
 
-- 
2.43.2


-- 
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND

~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
Simon says - in Latin please.
~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
Thanks to Simon Glass for the translation =] 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC PATCH 5/6] watchdog: ROHM BD96801 PMIC WDG driver

[Krzysztof Kozlowski]

On 02/04/2024 15:11, Matti Vaittinen wrote:
> Introduce driver for WDG block on ROHM BD96801 scalable PMIC.
> 
> This driver only supports watchdog with I2C feeding and delayed
> response detection. Whether the watchdog toggles PRSTB pin or
> just causes an interrupt can be configured via device-tree.
> 
> The BD96801 PMIC HW supports also window watchdog (too early

...

> +
> +static struct platform_driver bd96801_wdt = {
> +	.driver = {
> +		.name = "bd96801-wdt"
> +	},
> +	.probe = bd96801_wdt_probe,
> +};
> +module_platform_driver(bd96801_wdt);
> +
> +MODULE_AUTHOR("Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>");
> +MODULE_DESCRIPTION("BD96801 watchdog driver");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:bd96801-wdt");

Same comment on alias. Please use ID table.

Best regards,
Krzysztof

Re: [RFC PATCH 5/6] watchdog: ROHM BD96801 PMIC WDG driver

[Guenter Roeck]

On Tue, Apr 02, 2024 at 04:11:41PM +0300, Matti Vaittinen wrote:
> Introduce driver for WDG block on ROHM BD96801 scalable PMIC.
> 
> This driver only supports watchdog with I2C feeding and delayed
> response detection. Whether the watchdog toggles PRSTB pin or
> just causes an interrupt can be configured via device-tree.
> 
> The BD96801 PMIC HW supports also window watchdog (too early
> feeding detection) and Q&A mode. These are not supported by
> this driver.
> 
> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
> ---
>  drivers/watchdog/Kconfig       |  13 ++
>  drivers/watchdog/Makefile      |   1 +
>  drivers/watchdog/bd96801_wdt.c | 375 +++++++++++++++++++++++++++++++++
>  3 files changed, 389 insertions(+)
>  create mode 100644 drivers/watchdog/bd96801_wdt.c
> 
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index 6bee137cfbe0..d97e735e1faa 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -181,6 +181,19 @@ config BD957XMUF_WATCHDOG
>  	  watchdog. Alternatively say M to compile the driver as a module,
>  	  which will be called bd9576_wdt.
>  
> +config BD96801_WATCHDOG
> +	tristate "ROHM BD96801 PMIC Watchdog"
> +	depends on MFD_ROHM_BD96801
> +	select WATCHDOG_CORE
> +	help
> +	  Support for the watchdog in the ROHM BD96801 PMIC. Watchdog can be
> +	  configured to only generate IRQ or to trigger system reset via reset
> +	  pin.
> +
> +	  Say Y here to include support for the ROHM BD96801 watchdog.
> +	  Alternatively say M to compile the driver as a module,
> +	  which will be called bd96801_wdt.
> +
>  config CROS_EC_WATCHDOG
>  	tristate "ChromeOS EC-based watchdog"
>  	select WATCHDOG_CORE
> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> index 3710c218f05e..31bc94436c81 100644
> --- a/drivers/watchdog/Makefile
> +++ b/drivers/watchdog/Makefile
> @@ -217,6 +217,7 @@ obj-$(CONFIG_XEN_WDT) += xen_wdt.o
>  
>  # Architecture Independent
>  obj-$(CONFIG_BD957XMUF_WATCHDOG) += bd9576_wdt.o
> +obj-$(CONFIG_BD96801_WATCHDOG) += bd96801_wdt.o
>  obj-$(CONFIG_CROS_EC_WATCHDOG) += cros_ec_wdt.o
>  obj-$(CONFIG_DA9052_WATCHDOG) += da9052_wdt.o
>  obj-$(CONFIG_DA9055_WATCHDOG) += da9055_wdt.o
> diff --git a/drivers/watchdog/bd96801_wdt.c b/drivers/watchdog/bd96801_wdt.c
> new file mode 100644
> index 000000000000..cb2b526ecc21
> --- /dev/null
> +++ b/drivers/watchdog/bd96801_wdt.c
> @@ -0,0 +1,375 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) 2022 ROHM Semiconductors
> + *
> + * ROHM BD96801 watchdog driver
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/mfd/rohm-bd96801.h>
> +#include <linux/mfd/rohm-generic.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/watchdog.h>
> +
> +static bool nowayout;
> +module_param(nowayout, bool, 0);
> +MODULE_PARM_DESC(nowayout,
> +		"Watchdog cannot be stopped once started (default=\"false\")");
> +
> +#define BD96801_WD_TMO_SHORT_MASK	0x70
> +#define BD96801_WD_RATIO_MASK		0x3
> +#define BD96801_WD_TYPE_MASK		0x4
> +#define BD96801_WD_TYPE_SLOW		0x4
> +#define BD96801_WD_TYPE_WIN		0x0
> +
> +#define BD96801_WD_EN_MASK		0x3
> +#define BD96801_WD_IF_EN		0x1
> +#define BD96801_WD_QA_EN		0x2
> +#define BD96801_WD_DISABLE		0x0
> +
> +#define BD96801_WD_ASSERT_MASK		0x8
> +#define BD96801_WD_ASSERT_RST		0x8
> +#define BD96801_WD_ASSERT_IRQ		0x0
> +
> +#define BD96801_WD_FEED_MASK		0x1
> +#define BD96801_WD_FEED			0x1
> +
> +/* units in uS */
> +#define FASTNG_MIN			3370
> +#define BD96801_WDT_DEFAULT_MARGIN	6905120
> +/* Unit is seconds */
> +#define DEFAULT_TIMEOUT 30
> +
> +/*
> + * BD96801 WDG supports window mode so the TMO consists of SHORT and LONG
> + * timeout values. SHORT time is meaningfull only in window mode where feeding
> + * period shorter than SHORT would be an error. LONG time is used to detect if
> + * feeding is not occurring within given time limit (SoC SW hangs). The LONG
> + * timeout time is a multiple of (2, 4, 8 0r 16 times) the SHORT timeout.
> + */
> +
> +struct wdtbd96801 {
> +	struct device		*dev;
> +	struct regmap		*regmap;
> +	bool			always_running;
> +	struct watchdog_device	wdt;
> +};
> +
> +static int bd96801_wdt_ping(struct watchdog_device *wdt)
> +{
> +	struct wdtbd96801 *w = watchdog_get_drvdata(wdt);
> +
> +	return regmap_update_bits(w->regmap, BD96801_REG_WD_FEED,
> +				 BD96801_WD_FEED_MASK, BD96801_WD_FEED);
> +}
> +
> +static int bd96801_wdt_start(struct watchdog_device *wdt)
> +{
> +	struct wdtbd96801 *w = watchdog_get_drvdata(wdt);
> +	int ret;
> +
> +	ret = regmap_update_bits(w->regmap, BD96801_REG_WD_CONF,
> +				 BD96801_WD_EN_MASK, BD96801_WD_IF_EN);
> +
> +	return ret;
> +}
> +
> +static int bd96801_wdt_stop(struct watchdog_device *wdt)
> +{
> +	struct wdtbd96801 *w = watchdog_get_drvdata(wdt);
> +
> +	if (!w->always_running)
> +		return regmap_update_bits(w->regmap, BD96801_REG_WD_CONF,
> +				 BD96801_WD_EN_MASK, BD96801_WD_DISABLE);
> +	set_bit(WDOG_HW_RUNNING, &wdt->status);
> +
> +	return 0;
> +}
> +
> +static const struct watchdog_info bd96801_wdt_info = {
> +	.options	= WDIOF_MAGICCLOSE | WDIOF_KEEPALIVEPING |
> +			  WDIOF_SETTIMEOUT,
> +	.identity	= "BD96801 Watchdog",
> +};
> +
> +static const struct watchdog_ops bd96801_wdt_ops = {
> +	.start		= bd96801_wdt_start,
> +	.stop		= bd96801_wdt_stop,
> +	.ping		= bd96801_wdt_ping,
> +};
> +
> +static int find_closest_fast(int target, int *sel, int *val)
> +{
> +	int i;
> +	int window = FASTNG_MIN;
> +
> +	for (i = 0; i < 8 && window < target; i++)
> +		window <<= 1;
> +
> +	*val = window;
> +	*sel = i;
> +
> +	if (i == 8)
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
> +static int find_closest_slow_by_fast(int fast_val, int *target, int *slowsel)
> +{
> +	int sel;
> +	static const int multipliers[] = {2, 4, 8, 16};
> +
> +	for (sel = 0; sel < ARRAY_SIZE(multipliers) &&
> +	     multipliers[sel] * fast_val < *target; sel++)
> +		;
> +
> +	if (sel == ARRAY_SIZE(multipliers))
> +		return -EINVAL;
> +
> +	*slowsel = sel;
> +	*target = multipliers[sel] * fast_val;
> +
> +	return 0;
> +}
> +
> +static int find_closest_slow(int *target, int *slow_sel, int *fast_sel)
> +{
> +	static const int multipliers[] = {2, 4, 8, 16};
> +	int i, j;
> +	int val = 0;
> +	int window = FASTNG_MIN;
> +
> +	for (i = 0; i < 8; i++) {
> +		for (j = 0; j < ARRAY_SIZE(multipliers); j++) {
> +			int slow;
> +
> +			slow = window * multipliers[j];
> +			if (slow >= *target && (!val || slow < val)) {
> +				val = slow;
> +				*fast_sel = i;
> +				*slow_sel = j;
> +			}
> +		}
> +		window <<= 1;
> +	}
> +	if (!val)
> +		return -EINVAL;
> +
> +	*target = val;
> +
> +	return 0;
> +}
> +
> +static int bd96801_set_wdt_mode(struct wdtbd96801 *w, int hw_margin,
> +			       int hw_margin_min)
> +{
> +	int ret, fastng, slowng, type, reg, mask;
> +	struct device *dev = w->dev;
> +
> +	/* convert to uS */
> +	hw_margin *= 1000;
> +	hw_margin_min *= 1000;
> +	if (hw_margin_min) {
> +		int min;
> +
> +		type = BD96801_WD_TYPE_WIN;
> +		dev_dbg(dev, "Setting type WINDOW 0x%x\n", type);
> +		ret = find_closest_fast(hw_margin_min, &fastng, &min);
> +		if (ret) {
> +			dev_err(dev, "bad WDT window for fast timeout\n");
> +			return ret;
> +		}
> +
> +		ret = find_closest_slow_by_fast(min, &hw_margin, &slowng);
> +		if (ret) {
> +			dev_err(dev, "bad WDT window\n");
> +			return ret;
> +		}
> +		w->wdt.min_hw_heartbeat_ms = min / 1000;
> +	} else {
> +		type = BD96801_WD_TYPE_SLOW;
> +		dev_dbg(dev, "Setting type SLOW 0x%x\n", type);
> +		ret = find_closest_slow(&hw_margin, &slowng, &fastng);
> +		if (ret) {
> +			dev_err(dev, "bad WDT window\n");
> +			return ret;
> +		}
> +	}
> +
> +	w->wdt.max_hw_heartbeat_ms = hw_margin / 1000;
> +
> +	fastng <<= ffs(BD96801_WD_TMO_SHORT_MASK) - 1;
> +
> +	reg = slowng | fastng;
> +	mask = BD96801_WD_RATIO_MASK | BD96801_WD_TMO_SHORT_MASK;
> +	ret = regmap_update_bits(w->regmap, BD96801_REG_WD_TMO,
> +				 mask, reg);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_update_bits(w->regmap, BD96801_REG_WD_CONF,
> +				 BD96801_WD_TYPE_MASK, type);
> +
> +	return ret;
> +}
> +
> +static int bd96801_set_heartbeat_from_hw(struct wdtbd96801 *w,
> +					 unsigned int conf_reg)
> +{
> +	int ret;
> +	unsigned int val, sel, fast;
> +
> +	/*
> +	 * The BD96801 supports a somewhat peculiar QA-mode, which we do not
> +	 * support in this driver. If the QA-mode is enabled then we just
> +	 * warn and bail-out.
> +	 */
> +	if ((conf_reg & BD96801_WD_EN_MASK) != BD96801_WD_IF_EN) {
> +		dev_warn(w->dev, "watchdog set to Q&A mode - exiting\n");
> +		return -EINVAL;
> +	}
> +
> +	ret = regmap_read(w->regmap, BD96801_REG_WD_TMO, &val);
> +	if (ret)
> +		return ret;
> +
> +	sel = val & BD96801_WD_TMO_SHORT_MASK;
> +	sel >>= ffs(BD96801_WD_TMO_SHORT_MASK) - 1;
> +	fast = FASTNG_MIN << sel;
> +
> +	sel = (val & BD96801_WD_RATIO_MASK) + 1;
> +	w->wdt.max_hw_heartbeat_ms = (fast << sel) / USEC_PER_MSEC;
> +
> +	if ((conf_reg & BD96801_WD_TYPE_MASK) == BD96801_WD_TYPE_WIN)
> +		w->wdt.min_hw_heartbeat_ms = fast / USEC_PER_MSEC;
> +
> +	return 0;
> +}
> +
> +static int init_wdg_hw(struct wdtbd96801 *w)
> +{
> +	u32 hw_margin[2];
> +	int count, ret;
> +	u32 hw_margin_max = BD96801_WDT_DEFAULT_MARGIN, hw_margin_min = 0;
> +
> +	count = device_property_count_u32(w->dev->parent, "rohm,hw-timeout-ms");
> +	if (count < 0 && count != -EINVAL)
> +		return count;
> +
> +	if (count > 0) {
> +		if (count > ARRAY_SIZE(hw_margin))
> +			return -EINVAL;
> +
> +		ret = device_property_read_u32_array(w->dev->parent,
> +						     "rohm,hw-timeout-ms",
> +						     &hw_margin[0], count);
> +		if (ret < 0)
> +			return ret;
> +
> +		if (count == 1)
> +			hw_margin_max = hw_margin[0];
> +
> +		if (count == 2) {
> +			hw_margin_max = hw_margin[1];
> +			hw_margin_min = hw_margin[0];
> +		}
> +	}
> +
> +	ret = bd96801_set_wdt_mode(w, hw_margin_max, hw_margin_min);
> +	if (ret)
> +		return ret;
> +
> +	ret = device_property_match_string(w->dev->parent, "rohm,wdg-action",
> +					   "prstb");
> +	if (ret >= 0) {
> +		ret = regmap_update_bits(w->regmap, BD96801_REG_WD_CONF,
> +				 BD96801_WD_ASSERT_MASK,
> +				 BD96801_WD_ASSERT_RST);
> +		return ret;
> +	}
> +
> +	ret = device_property_match_string(w->dev->parent, "rohm,wdg-action",
> +					   "intb-only");
> +	if (ret >= 0) {
> +		ret = regmap_update_bits(w->regmap, BD96801_REG_WD_CONF,
> +				 BD96801_WD_ASSERT_MASK,
> +				 BD96801_WD_ASSERT_IRQ);
> +		return ret;
> +	}

I don't see the devicetree bindings documented in the series.

I am also a bit surprised that the interrupt isn't handled in the driver.
Please explain.

> +
> +	return 0;
> +}
> +
> +static int bd96801_wdt_probe(struct platform_device *pdev)
> +{
> +	struct wdtbd96801 *w;
> +	int ret;
> +	unsigned int val;
> +
> +	w = devm_kzalloc(&pdev->dev, sizeof(*w), GFP_KERNEL);
> +	if (!w)
> +		return -ENOMEM;
> +
> +	w->regmap = dev_get_regmap(pdev->dev.parent, NULL);

dev_get_regmap() can return NULL.

> +	w->dev = &pdev->dev;
> +
> +	w->wdt.info = &bd96801_wdt_info;
> +	w->wdt.ops =  &bd96801_wdt_ops;
> +	w->wdt.parent = pdev->dev.parent;
> +	w->wdt.timeout = DEFAULT_TIMEOUT;
> +	watchdog_set_drvdata(&w->wdt, w);
> +
> +	w->always_running = device_property_read_bool(pdev->dev.parent,
> +						      "always-running");
> +
Without documentation, it looks like the always-running (from
linux,wdt-gpio.yaml) may be abused. Its defined meaning is
"the watchdog is always running and can not be stopped". Its
use here seems to be "start watchdog when loading the module and
prevent it from being stopped". 

Oh well, looks like the abuse was introduced with bd9576_wdt. That
doesn't make it better. At the very least it needs to be documented
that the property does not have the intended (documented) meaning.

> +	ret = regmap_read(w->regmap, BD96801_REG_WD_CONF, &val);
> +	if (ret)
> +		return dev_err_probe(&pdev->dev, ret,
> +				     "Failed to get the watchdog state\n");
> +
> +	/*
> +	 * If the WDG is already enabled we assume it is configured by boot.
> +	 * In this case we just update the hw-timeout based on values set to
> +	 * the timeout / mode registers and leave the hardware configs
> +	 * untouched.
> +	 */
> +	if ((val & BD96801_WD_EN_MASK) != BD96801_WD_DISABLE) {
> +		dev_dbg(&pdev->dev, "watchdog was running during probe\n");
> +		ret = bd96801_set_heartbeat_from_hw(w, val);
> +		if (ret)
> +			return ret;
> +
> +		set_bit(WDOG_HW_RUNNING, &w->wdt.status);
> +	} else {
> +		/* If WDG is not running so we will initializate it */
> +		ret = init_wdg_hw(w);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	watchdog_init_timeout(&w->wdt, 0, pdev->dev.parent);
> +	watchdog_set_nowayout(&w->wdt, nowayout);
> +	watchdog_stop_on_reboot(&w->wdt);
> +
> +	if (w->always_running)
> +		bd96801_wdt_start(&w->wdt);

I think this needs to set WDOG_HW_RUNNING or the watchdog will trigger
a reboot if the watchdog device is not opened and the watchdog wasn't
already running when the module was loaded.

That makes me wonder what happens if the property is set and the
watchdog daemon isn't started in the bd9576_wdt driver.

> +
> +	return devm_watchdog_register_device(&pdev->dev, &w->wdt);
> +}
> +
> +static struct platform_driver bd96801_wdt = {
> +	.driver = {
> +		.name = "bd96801-wdt"
> +	},
> +	.probe = bd96801_wdt_probe,
> +};
> +module_platform_driver(bd96801_wdt);
> +
> +MODULE_AUTHOR("Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>");
> +MODULE_DESCRIPTION("BD96801 watchdog driver");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:bd96801-wdt");
> -- 
> 2.43.2
> 
> 
> -- 
> Matti Vaittinen, Linux device drivers
> ROHM Semiconductors, Finland SWDC
> Kiviharjunlenkki 1E
> 90220 OULU
> FINLAND
> 
> ~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
> Simon says - in Latin please.
> ~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
> Thanks to Simon Glass for the translation =] 

Re: [RFC PATCH 5/6] watchdog: ROHM BD96801 PMIC WDG driver

[Matti Vaittinen]

Hi Guenter,

First of all, thanks for the review. It was quick! Especially when we 
speak of a RFC series. Very much appreciated.

On 4/2/24 20:11, Guenter Roeck wrote:
> On Tue, Apr 02, 2024 at 04:11:41PM +0300, Matti Vaittinen wrote >> +static int init_wdg_hw(struct wdtbd96801 *w)
>> +{
>> +	u32 hw_margin[2];
>> +	int count, ret;
>> +	u32 hw_margin_max = BD96801_WDT_DEFAULT_MARGIN, hw_margin_min = 0;
>> +
>> +	count = device_property_count_u32(w->dev->parent, "rohm,hw-timeout-ms");
>> +	if (count < 0 && count != -EINVAL)
>> +		return count;
>> +
>> +	if (count > 0) {
>> +		if (count > ARRAY_SIZE(hw_margin))
>> +			return -EINVAL;
>> +
>> +		ret = device_property_read_u32_array(w->dev->parent,
>> +						     "rohm,hw-timeout-ms",
>> +						     &hw_margin[0], count);
>> +		if (ret < 0)
>> +			return ret;
>> +
>> +		if (count == 1)
>> +			hw_margin_max = hw_margin[0];
>> +
>> +		if (count == 2) {
>> +			hw_margin_max = hw_margin[1];
>> +			hw_margin_min = hw_margin[0];
>> +		}
>> +	}
>> +
>> +	ret = bd96801_set_wdt_mode(w, hw_margin_max, hw_margin_min);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = device_property_match_string(w->dev->parent, "rohm,wdg-action",
>> +					   "prstb");
>> +	if (ret >= 0) {
>> +		ret = regmap_update_bits(w->regmap, BD96801_REG_WD_CONF,
>> +				 BD96801_WD_ASSERT_MASK,
>> +				 BD96801_WD_ASSERT_RST);
>> +		return ret;
>> +	}
>> +
>> +	ret = device_property_match_string(w->dev->parent, "rohm,wdg-action",
>> +					   "intb-only");
>> +	if (ret >= 0) {
>> +		ret = regmap_update_bits(w->regmap, BD96801_REG_WD_CONF,
>> +				 BD96801_WD_ASSERT_MASK,
>> +				 BD96801_WD_ASSERT_IRQ);
>> +		return ret;
>> +	}
> 
> I don't see the devicetree bindings documented in the series.

Seems like I have missed this WDG binding. But after reading your 
comment below, I am wondering if I should just drop the binding and 
default to "prstb" (shutdown should the feeding be skipped) - and leave 
the "intb-only" case for one who actually needs such.

> I am also a bit surprised that the interrupt isn't handled in the driver.
> Please explain.

Basically, I just had no idea what the IRQ should do in the generic 
case. If we get an interrupt, it means the WDG feeding has failed. My 
thinking is that, what should happen is forced reset. I don't see how 
that can be done in reliably manner from an IRQ handler.

When the "prstb WDG action" is set (please, see the above DT binding 
handling), the PMIC shall shut down power outputs. This should get the 
watchdog's job done.

With the "intb-only"-option, PMIC will not turn off the power. I'd 
expect there to be some external HW connection which handles the reset 
by HW.

After all this being said, I wonder if I should just unconditionally 
configure the PMIC to always turn off the power (prstb option) should 
the feeding fail? Or do someone have some suggestion what the IRQ 
handler should do (except maybe print an error msg)?

> 
>> +
>> +	return 0;
>> +}
>> +
>> +static int bd96801_wdt_probe(struct platform_device *pdev)
>> +{
>> +	struct wdtbd96801 *w;
>> +	int ret;
>> +	unsigned int val;
>> +
>> +	w = devm_kzalloc(&pdev->dev, sizeof(*w), GFP_KERNEL);
>> +	if (!w)
>> +		return -ENOMEM;
>> +
>> +	w->regmap = dev_get_regmap(pdev->dev.parent, NULL);
> 
> dev_get_regmap() can return NULL.
> 
>> +	w->dev = &pdev->dev;
>> +
>> +	w->wdt.info = &bd96801_wdt_info;
>> +	w->wdt.ops =  &bd96801_wdt_ops;
>> +	w->wdt.parent = pdev->dev.parent;
>> +	w->wdt.timeout = DEFAULT_TIMEOUT;
>> +	watchdog_set_drvdata(&w->wdt, w);
>> +
>> +	w->always_running = device_property_read_bool(pdev->dev.parent,
>> +						      "always-running");
>> +
> Without documentation, it looks like the always-running (from
> linux,wdt-gpio.yaml) may be abused. Its defined meaning is
> "the watchdog is always running and can not be stopped". Its
> use here seems to be "start watchdog when loading the module and
> prevent it from being stopped".

Yes. You're right.

> Oh well, looks like the abuse was introduced with bd9576_wdt. That
> doesn't make it better. At the very least it needs to be documented
> that the property does not have the intended (documented) meaning.

I can raise my hand for a sign of an error here. I've been misreading 
the intended meaning of the always-running. Not sure if I've picked it 
from another driver (maybe GPIO watchdog), or if I've just 
misinterpreted the binding docs.

Do you suggest me to add a note in the BD9576 binding doc (there is no 
BD9576 specific binding doc for watchdog. I wonder whether this warrants 
adding one under watchdog or if the note can be added under 
Documentation/devicetree/bindings/mfd/rohm,bd9576...).

>> +	ret = regmap_read(w->regmap, BD96801_REG_WD_CONF, &val);
>> +	if (ret)
>> +		return dev_err_probe(&pdev->dev, ret,
>> +				     "Failed to get the watchdog state\n");
>> +
>> +	/*
>> +	 * If the WDG is already enabled we assume it is configured by boot.
>> +	 * In this case we just update the hw-timeout based on values set to
>> +	 * the timeout / mode registers and leave the hardware configs
>> +	 * untouched.
>> +	 */
>> +	if ((val & BD96801_WD_EN_MASK) != BD96801_WD_DISABLE) {
>> +		dev_dbg(&pdev->dev, "watchdog was running during probe\n");
>> +		ret = bd96801_set_heartbeat_from_hw(w, val);
>> +		if (ret)
>> +			return ret;
>> +
>> +		set_bit(WDOG_HW_RUNNING, &w->wdt.status);
>> +	} else {
>> +		/* If WDG is not running so we will initializate it */
>> +		ret = init_wdg_hw(w);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>> +	watchdog_init_timeout(&w->wdt, 0, pdev->dev.parent);
>> +	watchdog_set_nowayout(&w->wdt, nowayout);
>> +	watchdog_stop_on_reboot(&w->wdt);
>> +
>> +	if (w->always_running)
>> +		bd96801_wdt_start(&w->wdt);
> 
> I think this needs to set WDOG_HW_RUNNING or the watchdog will trigger
> a reboot if the watchdog device is not opened and the watchdog wasn't
> already running when the module was loaded.

I believe you're right. Seems I haven't properly tested this path.

> That makes me wonder what happens if the property is set and the
> watchdog daemon isn't started in the bd9576_wdt driver.

My assumption is the dog starts barking. I'll see if I find the BD9576 
break-out board from one of my boxes to wire it up and test this. If the 
always-running is not working it might be justified to just drop it from 
both drivers. I believe it'd be indication that no-one is really using 
the always-running with the upstream driver.

Thanks a ton!

Yours,
	-- Matti

-- 
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~

Re: [RFC PATCH 5/6] watchdog: ROHM BD96801 PMIC WDG driver

[Guenter Roeck]

On Wed, Apr 03, 2024 at 09:34:35AM +0300, Matti Vaittinen wrote:
> Hi Guenter,
> 
> First of all, thanks for the review. It was quick! Especially when we speak
> of a RFC series. Very much appreciated.
> 
> On 4/2/24 20:11, Guenter Roeck wrote:
> > On Tue, Apr 02, 2024 at 04:11:41PM +0300, Matti Vaittinen wrote >> +static int init_wdg_hw(struct wdtbd96801 *w)
> > > +{
> > > +	u32 hw_margin[2];
> > > +	int count, ret;
> > > +	u32 hw_margin_max = BD96801_WDT_DEFAULT_MARGIN, hw_margin_min = 0;
> > > +
> > > +	count = device_property_count_u32(w->dev->parent, "rohm,hw-timeout-ms");
> > > +	if (count < 0 && count != -EINVAL)
> > > +		return count;
> > > +
> > > +	if (count > 0) {
> > > +		if (count > ARRAY_SIZE(hw_margin))
> > > +			return -EINVAL;
> > > +
> > > +		ret = device_property_read_u32_array(w->dev->parent,
> > > +						     "rohm,hw-timeout-ms",
> > > +						     &hw_margin[0], count);
> > > +		if (ret < 0)
> > > +			return ret;
> > > +
> > > +		if (count == 1)
> > > +			hw_margin_max = hw_margin[0];
> > > +
> > > +		if (count == 2) {
> > > +			hw_margin_max = hw_margin[1];
> > > +			hw_margin_min = hw_margin[0];
> > > +		}
> > > +	}
> > > +
> > > +	ret = bd96801_set_wdt_mode(w, hw_margin_max, hw_margin_min);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	ret = device_property_match_string(w->dev->parent, "rohm,wdg-action",
> > > +					   "prstb");
> > > +	if (ret >= 0) {
> > > +		ret = regmap_update_bits(w->regmap, BD96801_REG_WD_CONF,
> > > +				 BD96801_WD_ASSERT_MASK,
> > > +				 BD96801_WD_ASSERT_RST);
> > > +		return ret;
> > > +	}
> > > +
> > > +	ret = device_property_match_string(w->dev->parent, "rohm,wdg-action",
> > > +					   "intb-only");
> > > +	if (ret >= 0) {
> > > +		ret = regmap_update_bits(w->regmap, BD96801_REG_WD_CONF,
> > > +				 BD96801_WD_ASSERT_MASK,
> > > +				 BD96801_WD_ASSERT_IRQ);
> > > +		return ret;
> > > +	}
> > 
> > I don't see the devicetree bindings documented in the series.
> 
> Seems like I have missed this WDG binding. But after reading your comment
> below, I am wondering if I should just drop the binding and default to
> "prstb" (shutdown should the feeding be skipped) - and leave the "intb-only"
> case for one who actually needs such.
> 
> > I am also a bit surprised that the interrupt isn't handled in the driver.
> > Please explain.
> 
> Basically, I just had no idea what the IRQ should do in the generic case. If
> we get an interrupt, it means the WDG feeding has failed. My thinking is
> that, what should happen is forced reset. I don't see how that can be done
> in reliably manner from an IRQ handler.
> 
> When the "prstb WDG action" is set (please, see the above DT binding
> handling), the PMIC shall shut down power outputs. This should get the
> watchdog's job done.
> 
> With the "intb-only"-option, PMIC will not turn off the power. I'd expect
> there to be some external HW connection which handles the reset by HW.
> 
> After all this being said, I wonder if I should just unconditionally
> configure the PMIC to always turn off the power (prstb option) should the
> feeding fail? Or do someone have some suggestion what the IRQ handler should
> do (except maybe print an error msg)?
> 

Other watchdog drivers call emergency_restart() if the watchdog times out
and triggers an interrupt. Are you saying this won't work for this system ?
If so, please explain.

Thanks,
Guenter

Re: [RFC PATCH 5/6] watchdog: ROHM BD96801 PMIC WDG driver

[Matti Vaittinen]

On 4/3/24 15:41, Guenter Roeck wrote:
> On Wed, Apr 03, 2024 at 09:34:35AM +0300, Matti Vaittinen wrote:
>> Hi Guenter,
>>
>> First of all, thanks for the review. It was quick! Especially when we speak
>> of a RFC series. Very much appreciated.
>>
>> On 4/2/24 20:11, Guenter Roeck wrote:
>>> On Tue, Apr 02, 2024 at 04:11:41PM +0300, Matti Vaittinen wrote >> +static int init_wdg_hw(struct wdtbd96801 *w)
>>>> +{
>>>> +	u32 hw_margin[2];
>>>> +	int count, ret;
>>>> +	u32 hw_margin_max = BD96801_WDT_DEFAULT_MARGIN, hw_margin_min = 0;
>>>> +
>>>> +	count = device_property_count_u32(w->dev->parent, "rohm,hw-timeout-ms");
>>>> +	if (count < 0 && count != -EINVAL)
>>>> +		return count;
>>>> +
>>>> +	if (count > 0) {
>>>> +		if (count > ARRAY_SIZE(hw_margin))
>>>> +			return -EINVAL;
>>>> +
>>>> +		ret = device_property_read_u32_array(w->dev->parent,
>>>> +						     "rohm,hw-timeout-ms",
>>>> +						     &hw_margin[0], count);
>>>> +		if (ret < 0)
>>>> +			return ret;
>>>> +
>>>> +		if (count == 1)
>>>> +			hw_margin_max = hw_margin[0];
>>>> +
>>>> +		if (count == 2) {
>>>> +			hw_margin_max = hw_margin[1];
>>>> +			hw_margin_min = hw_margin[0];
>>>> +		}
>>>> +	}
>>>> +
>>>> +	ret = bd96801_set_wdt_mode(w, hw_margin_max, hw_margin_min);
>>>> +	if (ret)
>>>> +		return ret;
>>>> +
>>>> +	ret = device_property_match_string(w->dev->parent, "rohm,wdg-action",
>>>> +					   "prstb");
>>>> +	if (ret >= 0) {
>>>> +		ret = regmap_update_bits(w->regmap, BD96801_REG_WD_CONF,
>>>> +				 BD96801_WD_ASSERT_MASK,
>>>> +				 BD96801_WD_ASSERT_RST);
>>>> +		return ret;
>>>> +	}
>>>> +
>>>> +	ret = device_property_match_string(w->dev->parent, "rohm,wdg-action",
>>>> +					   "intb-only");
>>>> +	if (ret >= 0) {
>>>> +		ret = regmap_update_bits(w->regmap, BD96801_REG_WD_CONF,
>>>> +				 BD96801_WD_ASSERT_MASK,
>>>> +				 BD96801_WD_ASSERT_IRQ);
>>>> +		return ret;
>>>> +	}
>>>
>>> I don't see the devicetree bindings documented in the series.
>>
>> Seems like I have missed this WDG binding. But after reading your comment
>> below, I am wondering if I should just drop the binding and default to
>> "prstb" (shutdown should the feeding be skipped) - and leave the "intb-only"
>> case for one who actually needs such.
>>
>>> I am also a bit surprised that the interrupt isn't handled in the driver.
>>> Please explain.
>>
>> Basically, I just had no idea what the IRQ should do in the generic case. If
>> we get an interrupt, it means the WDG feeding has failed. My thinking is
>> that, what should happen is forced reset. I don't see how that can be done
>> in reliably manner from an IRQ handler.
>>
>> When the "prstb WDG action" is set (please, see the above DT binding
>> handling), the PMIC shall shut down power outputs. This should get the
>> watchdog's job done.
>>
>> With the "intb-only"-option, PMIC will not turn off the power. I'd expect
>> there to be some external HW connection which handles the reset by HW.
>>
>> After all this being said, I wonder if I should just unconditionally
>> configure the PMIC to always turn off the power (prstb option) should the
>> feeding fail? Or do someone have some suggestion what the IRQ handler should
>> do (except maybe print an error msg)?
>>
> 
> Other watchdog drivers call emergency_restart() if the watchdog times out
> and triggers an interrupt. Are you saying this won't work for this system ?
> If so, please explain.
> 

Thanks Guenter. If it works with systems using other devices, then it 
should work (to the same extent) with systems using this PMIC. Thanks.

I'll add the IRQ handling to next version - but it may take a while as 
I'm currently having some problems with the IRQs in general, and because 
I'll wait for feedback from Mark to the regulator part.

Yours,
	-- Matti

-- 
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~

Re: [RFC PATCH 5/6] watchdog: ROHM BD96801 PMIC WDG driver

[Guenter Roeck]

On Wed, Apr 03, 2024 at 03:47:12PM +0300, Matti Vaittinen wrote:
> > 
> > Other watchdog drivers call emergency_restart() if the watchdog times out
> > and triggers an interrupt. Are you saying this won't work for this system ?
> > If so, please explain.
> > 
> 
> Thanks Guenter. If it works with systems using other devices, then it should
> work (to the same extent) with systems using this PMIC. Thanks.
> 

You might also consider to just call panic(). What is what we do if the
pretimeout panic governor is enabled.

That makes me wonder if it would make sense to introduce watchdog timeout
governors, similar to the existing pretimeout governors. Maybe if I ever
find the time to do it ...

Guenter

> I'll add the IRQ handling to next version - but it may take a while as I'm
> currently having some problems with the IRQs in general, and because I'll
> wait for feedback from Mark to the regulator part.
> 
> Yours,
> 	-- Matti
> 
> -- 
> Matti Vaittinen
> Linux kernel developer at ROHM Semiconductors
> Oulu Finland
> 
> ~~ When things go utterly wrong vim users can always type :help! ~~
> 

Re: [RFC PATCH 0/6] Support ROHM BD96801 scalable PMIC

[Matti Vaittinen]

On 4/2/24 16:07, Matti Vaittinen wrote:
> Another "oddity" is that the PMIC has two physical IRQ lines. When I
> last wrote this patch in 2021 I had some naming collison in debugfs for
> the IRQ domains. Back then I used:
> irq_domain_update_bus_token(intb_domain, DOMAIN_BUS_WIRED);
> to work-around the issue. Now, when rebasing to v6.9-rc1 the naming
> collision was gone and things seemed to work. However, it'd be great if
> the IRQ code in MFD driver was reviewed by greater minds :)

It appears my statement "things seemed to work" is a bit too optimistic. 
I am afraid my approach of having two separate IRQ domains for one 
device (and DT-node) is just somehow fundamentally wrong. It'd be great 
to learn what's the correct "ideology" here.

It appears the naming collision is still there. My config just had the 
CONFIG_GENERIC_IRQ_DEBUGFS disabled. Enabling it shows the same naming 
collison:
debugfs: File 
':ocp:interconnect@48000000:segment@100000:target-module@9c000:i2c@0:pmic@60' 
in directory 'domains' already present!

If I'm not mistaken the debugfs file name is generated from the 
device-tree node path+name. This is a subtle hint that it is not 
expected there are more than 1 IRQ-domain / device. I guess this kind of 
makes sense if we can have more than 1 HWIRQ handled by a single domain 
(I don't recall having to ever write such domain/IRQ-controller before, 
but I think it should be possible).

I have now 3 new questions =)

1. Should we be able to have more than 1 IRQ domain / device?
2. Should regmap_irq support having more than 1 HWIRQ
3. If answer to 1 is "no" - should we protect against this somehow? (see 
why below).

When CONFIG_GENERIC_IRQ_DEBUGFS is disabled, adding the two IRQ 
controllers with own IRQ domains (intb and errb here) to a single device 
is seemingly successful. I see no complaints / errors. Also, most of the 
IRQs seem to work - but not all. In my case trying to issue:

cat /proc/interrupts

will oops. Also, looking in the /sys/kernel/irq/ lists folders for all 
the "intb" and "errb" IRQs - but reading the files contained in these 
directories will cause an oops for all "errb" interrupts except for the 
first 16.

Finally, if I use the
irq_domain_update_bus_token(intb_domain, DOMAIN_BUS_WIRED);

to add "-1" at the end of the "intb" - domain name resulting domains:

:ocp:interconnect@48000000:segment@100000:target-module@9c000:i2c@0:pmic@60
:ocp:interconnect@48000000:segment@100000:target-module@9c000:i2c@0:pmic@60-1

then it seems that reading the IRQ information from the /proc/interrupts 
works as expected. Here I am making a wild guess that the name of the 
domain is used as a key for some data-lookups, and having two domains 
with a same name will either overwrite something or cause wrong domain 
data to be fetched. (This is just guessing for now).

Any tips, hints or thoughts on this?

Yours,
	-- Matti

-- 
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~

Re: [RFC PATCH 0/6] Support ROHM BD96801 scalable PMIC

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 702 bytes --]

On Thu, Apr 04, 2024 at 10:26:34AM +0300, Matti Vaittinen wrote:

> 1. Should we be able to have more than 1 IRQ domain / device?
> 2. Should regmap_irq support having more than 1 HWIRQ

I would expect each parent interrupt to show up as a separate remap_irq.

> then it seems that reading the IRQ information from the /proc/interrupts
> works as expected. Here I am making a wild guess that the name of the domain
> is used as a key for some data-lookups, and having two domains with a same
> name will either overwrite something or cause wrong domain data to be
> fetched. (This is just guessing for now).

So if we arrange to supply a name when we register multiple domains
things should work fine?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC PATCH 0/6] Support ROHM BD96801 scalable PMIC

[Matti Vaittinen]

Hi Mark,

On 4/4/24 15:09, Mark Brown wrote:
> On Thu, Apr 04, 2024 at 10:26:34AM +0300, Matti Vaittinen wrote:
> 
>> 1. Should we be able to have more than 1 IRQ domain / device?
>> 2. Should regmap_irq support having more than 1 HWIRQ
> 
> I would expect each parent interrupt to show up as a separate remap_irq.
> 
>> then it seems that reading the IRQ information from the /proc/interrupts
>> works as expected. Here I am making a wild guess that the name of the domain
>> is used as a key for some data-lookups, and having two domains with a same
>> name will either overwrite something or cause wrong domain data to be
>> fetched. (This is just guessing for now).
> 
> So if we arrange to supply a name when we register multiple domains
> things should work fine?

Thanks for taking the time to look at my questions :)
I have been debugging this thing whole day today, without getting too 
far :) It seems there is something beyond the name collision though.

After I tried adding '-1' to the end of the other domain name to avoid 
the debugfs name collision I managed to do couple of successful runs - 
after which I reported here that problem seems to be just the naming. 
Soon after sending that mail I hit the oops again even though the naming 
was fixed.

Further debugging shows that the desc->action->name for the last 28 
'errb' IRQs get corrupted. This might point more to the IRQ requester 
side - so I need to further study the BD96801 driver side as well as the 
regulator_irq_helper. I'm having the creeping feeling that at the end of 
the day I need to find the guilty one from the mirror :)

But yes, creating 2 regmap-IRQ controllers for one device seems to 
generate naming conflict in the debugfs - so unless I'm mistaken, with 
the current regmap-IRQ we can't have more than 1 regmap-IRQ entity for a 
single device.

Just please give me some more time to see if I find the cause of the 
corruption and I hope I can write more concrete description. For now it 
was enough for me to hear having more than 1 IRQ domain / device is not 
something on the "DON'T DO THIS" -list.

Yours,
	-- Matti

-- 
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~

Re: [RFC PATCH 0/6] Support ROHM BD96801 scalable PMIC

[Matti Vaittinen]

On 4/4/24 16:15, Matti Vaittinen wrote:
> Hi Mark,
> 
> On 4/4/24 15:09, Mark Brown wrote:
>> On Thu, Apr 04, 2024 at 10:26:34AM +0300, Matti Vaittinen wrote:
>>
>>> 1. Should we be able to have more than 1 IRQ domain / device?
>>> 2. Should regmap_irq support having more than 1 HWIRQ
>>
>> I would expect each parent interrupt to show up as a separate remap_irq.
>>
>>> then it seems that reading the IRQ information from the /proc/interrupts
>>> works as expected. Here I am making a wild guess that the name of the 
>>> domain
>>> is used as a key for some data-lookups, and having two domains with a 
>>> same
>>> name will either overwrite something or cause wrong domain data to be
>>> fetched. (This is just guessing for now).

This was wrong guessing.

>> So if we arrange to supply a name when we register multiple domains
>> things should work fine?

After my latest findings, yes, I think so. How to do this correctly is 
beyond me though. The __irq_domain_create() seems to me that the name is 
meant to be the dt-node name when the controller is backed by a real 
dt-node. Naming of the irq_domain_alloc_named_fwnode() sounds to me like 
it is only intended to be used when there is no real fwnode. All 
suggestions appreciated. Using the:
irq_domain_update_bus_token(intb_domain, DOMAIN_BUS_WIRED);
feels like a dirty hack, and won't scale if there is more HWIRQs.

> Thanks for taking the time to look at my questions :)
> I have been debugging this thing whole day today, without getting too 
> far :) It seems there is something beyond the name collision though.
> 
> After I tried adding '-1' to the end of the other domain name to avoid 
> the debugfs name collision I managed to do couple of successful runs - 
> after which I reported here that problem seems to be just the naming. 
> Soon after sending that mail I hit the oops again even though the naming 
> was fixed.
> 
> Further debugging shows that the desc->action->name for the last 28 
> 'errb' IRQs get corrupted. This might point more to the IRQ requester 
> side - so I need to further study the BD96801 driver side as well as the 
> regulator_irq_helper. I'm having the creeping feeling that at the end of 
> the day I need to find the guilty one from the mirror :)

I was not wrong on this one. The regulator_irq_helper() duplicates 
memory for the data given in  const struct regulator_irq_desc *d - but 
it does not duplicate the irq name pointed from the given 
regulator_irq_desc. Nor does the request_threaded_irq(). I passed some 
of the IRQ names from the stack in the BD96801 driver ... a bug I 
should've caught earlier.

Well, good thing is that now I can fix the regulator_irq_helper() to do:

--- a/drivers/regulator/irq_helpers.c
+++ b/drivers/regulator/irq_helpers.c
@@ -352,6 +352,9 @@ void *regulator_irq_helper(struct device *dev,

         h->irq = irq;
         h->desc = *d;
+       h->desc.name = devm_kstrdup(dev, d->name, GFP_KERNEL);
+       if (!h->desc.name)
+               return ERR_PTR(-ENOMEM);

         ret = init_rdev_state(dev, h, rdev, common_errs, per_rdev_errs,
                               rdev_amount);

I'll send a patch if this sounds like a correct thing to do.



-- 
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~

Re: [RFC PATCH 0/6] Support ROHM BD96801 scalable PMIC

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 695 bytes --]

On Fri, Apr 05, 2024 at 12:19:40PM +0300, Matti Vaittinen wrote:

> Well, good thing is that now I can fix the regulator_irq_helper() to do:

> --- a/drivers/regulator/irq_helpers.c
> +++ b/drivers/regulator/irq_helpers.c
> @@ -352,6 +352,9 @@ void *regulator_irq_helper(struct device *dev,
> 
>         h->irq = irq;
>         h->desc = *d;
> +       h->desc.name = devm_kstrdup(dev, d->name, GFP_KERNEL);
> +       if (!h->desc.name)
> +               return ERR_PTR(-ENOMEM);
> 
>         ret = init_rdev_state(dev, h, rdev, common_errs, per_rdev_errs,
>                               rdev_amount);
> 
> I'll send a patch if this sounds like a correct thing to do.

Sure.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC PATCH 3/6] mfd: support ROHM BD96801 PMIC core

[Lee Jones]

On Tue, 02 Apr 2024, Matti Vaittinen wrote:

> The ROHM BD96801 PMIC is highly customizable automotive grade PMIC
> which integrates regulator and watchdog funtionalities.
> 
> Provide IRQ and register accesses for regulator/watchdog drivers.
> 
> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
> ---
>  drivers/mfd/Kconfig              |  13 +
>  drivers/mfd/Makefile             |   1 +
>  drivers/mfd/rohm-bd96801.c       | 454 +++++++++++++++++++++++++++++++
>  include/linux/mfd/rohm-bd96801.h | 212 +++++++++++++++
>  include/linux/mfd/rohm-generic.h |   1 +
>  5 files changed, 681 insertions(+)
>  create mode 100644 drivers/mfd/rohm-bd96801.c
>  create mode 100644 include/linux/mfd/rohm-bd96801.h
> 
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 4b023ee229cf..947045eb3a8e 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -2089,6 +2089,19 @@ config MFD_ROHM_BD957XMUF
>  	  BD9573MUF Power Management ICs. BD9576 and BD9573 are primarily
>  	  designed to be used to power R-Car series processors.
>  
> +config MFD_ROHM_BD96801
> +	tristate "ROHM BD96801 Power Management IC"
> +	depends on I2C=y
> +	depends on OF
> +	select REGMAP_I2C
> +	select REGMAP_IRQ
> +	select MFD_CORE
> +	help
> +	  Select this option to get support for the ROHM BD96801 Power
> +	  Management IC. The ROHM BD96801 is a highly scalable power management

Power Management

> +	  IC for industrial and automotive use. The BD96801 can be used as a
> +	  master PMIC in a chained PMIC solutions with suitable companion PMICs.

solution

> +
>  config MFD_STM32_LPTIMER
>  	tristate "Support for STM32 Low-Power Timer"
>  	depends on (ARCH_STM32 && OF) || COMPILE_TEST
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index c66f07edcd0e..e792892d4a8b 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -264,6 +264,7 @@ obj-$(CONFIG_RAVE_SP_CORE)	+= rave-sp.o
>  obj-$(CONFIG_MFD_ROHM_BD71828)	+= rohm-bd71828.o
>  obj-$(CONFIG_MFD_ROHM_BD718XX)	+= rohm-bd718x7.o
>  obj-$(CONFIG_MFD_ROHM_BD957XMUF)	+= rohm-bd9576.o
> +obj-$(CONFIG_MFD_ROHM_BD96801)	+= rohm-bd96801.o
>  obj-$(CONFIG_MFD_STMFX) 	+= stmfx.o
>  obj-$(CONFIG_MFD_KHADAS_MCU) 	+= khadas-mcu.o
>  obj-$(CONFIG_MFD_ACER_A500_EC)	+= acer-ec-a500.o
> diff --git a/drivers/mfd/rohm-bd96801.c b/drivers/mfd/rohm-bd96801.c
> new file mode 100644
> index 000000000000..7610d0114653
> --- /dev/null
> +++ b/drivers/mfd/rohm-bd96801.c
> @@ -0,0 +1,454 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +//
> +// Copyright (C) 2022 ROHM Semiconductors

No updates for 2 years?

> +//
> +// ROHM BD96801 PMIC driver

Only the SPDX should have C++ style comments.

> +#include <linux/i2c.h>
> +#include <linux/interrupt.h>
> +#include <linux/mfd/core.h>
> +#include <linux/module.h>
> +#include <linux/property.h>
> +#include <linux/regmap.h>
> +#include <linux/types.h>
> +
> +#include <linux/mfd/rohm-bd96801.h>
> +#include <linux/mfd/rohm-generic.h>
> +
> +static const struct resource regulator_errb_irqs[] = {
> +	DEFINE_RES_IRQ_NAMED(BD96801_OTP_ERR_STAT, "bd96801-otp-err"),
> +	DEFINE_RES_IRQ_NAMED(BD96801_DBIST_ERR_STAT, "bd96801-dbist-err"),
> +	DEFINE_RES_IRQ_NAMED(BD96801_EEP_ERR_STAT, "bd96801-eep-err"),
> +	DEFINE_RES_IRQ_NAMED(BD96801_ABIST_ERR_STAT, "bd96801-abist-err"),
> +	DEFINE_RES_IRQ_NAMED(BD96801_PRSTB_ERR_STAT, "bd96801-prstb-err"),
> +	DEFINE_RES_IRQ_NAMED(BD96801_DRMOS1_ERR_STAT, "bd96801-drmoserr1"),
> +	DEFINE_RES_IRQ_NAMED(BD96801_DRMOS2_ERR_STAT, "bd96801-drmoserr2"),
> +	DEFINE_RES_IRQ_NAMED(BD96801_SLAVE_ERR_STAT, "bd96801-slave-err"),
> +	DEFINE_RES_IRQ_NAMED(BD96801_VREF_ERR_STAT, "bd96801-vref-err"),
> +	DEFINE_RES_IRQ_NAMED(BD96801_TSD_ERR_STAT, "bd96801-tsd"),
> +	DEFINE_RES_IRQ_NAMED(BD96801_UVLO_ERR_STAT, "bd96801-uvlo-err"),
> +	DEFINE_RES_IRQ_NAMED(BD96801_OVLO_ERR_STAT, "bd96801-ovlo-err"),
> +	DEFINE_RES_IRQ_NAMED(BD96801_OSC_ERR_STAT, "bd96801-osc-err"),
> +	DEFINE_RES_IRQ_NAMED(BD96801_PON_ERR_STAT, "bd96801-pon-err"),
> +	DEFINE_RES_IRQ_NAMED(BD96801_POFF_ERR_STAT, "bd96801-poff-err"),
> +	DEFINE_RES_IRQ_NAMED(BD96801_CMD_SHDN_ERR_STAT, "bd96801-cmd-shdn-err"),
> +
> +	DEFINE_RES_IRQ_NAMED(BD96801_INT_PRSTB_WDT_ERR, "bd96801-prstb-wdt-err"),
> +	DEFINE_RES_IRQ_NAMED(BD96801_INT_CHIP_IF_ERR, "bd96801-chip-if-err"),
> +	DEFINE_RES_IRQ_NAMED(BD96801_INT_SHDN_ERR_STAT, "bd96801-int-shdn-err"),
> +
> +	DEFINE_RES_IRQ_NAMED(BD96801_BUCK1_PVIN_ERR_STAT, "bd96801-buck1-pvin-err"),
> +	DEFINE_RES_IRQ_NAMED(BD96801_BUCK1_OVP_ERR_STAT, "bd96801-buck1-ovp-err"),
> +	DEFINE_RES_IRQ_NAMED(BD96801_BUCK1_UVP_ERR_STAT, "bd96801-buck1-uvp-err"),
> +	DEFINE_RES_IRQ_NAMED(BD96801_BUCK1_SHDN_ERR_STAT, "bd96801-buck1-shdn-err"),
> +
> +	DEFINE_RES_IRQ_NAMED(BD96801_BUCK2_PVIN_ERR_STAT, "bd96801-buck2-pvin-err"),
> +	DEFINE_RES_IRQ_NAMED(BD96801_BUCK2_OVP_ERR_STAT, "bd96801-buck2-ovp-err"),
> +	DEFINE_RES_IRQ_NAMED(BD96801_BUCK2_UVP_ERR_STAT, "bd96801-buck2-uvp-err"),
> +	DEFINE_RES_IRQ_NAMED(BD96801_BUCK2_SHDN_ERR_STAT, "bd96801-buck2-shdn-err"),
> +
> +	DEFINE_RES_IRQ_NAMED(BD96801_BUCK3_PVIN_ERR_STAT, "bd96801-buck3-pvin-err"),
> +	DEFINE_RES_IRQ_NAMED(BD96801_BUCK3_OVP_ERR_STAT, "bd96801-buck3-ovp-err"),
> +	DEFINE_RES_IRQ_NAMED(BD96801_BUCK3_UVP_ERR_STAT, "bd96801-buck3-uvp-err"),
> +	DEFINE_RES_IRQ_NAMED(BD96801_BUCK3_SHDN_ERR_STAT, "bd96801-buck3-shdn-err"),
> +
> +	DEFINE_RES_IRQ_NAMED(BD96801_BUCK4_PVIN_ERR_STAT, "bd96801-buck4-pvin-err"),
> +	DEFINE_RES_IRQ_NAMED(BD96801_BUCK4_OVP_ERR_STAT, "bd96801-buck4-ovp-err"),
> +	DEFINE_RES_IRQ_NAMED(BD96801_BUCK4_UVP_ERR_STAT, "bd96801-buck4-uvp-err"),
> +	DEFINE_RES_IRQ_NAMED(BD96801_BUCK4_SHDN_ERR_STAT, "bd96801-buck4-shdn-err"),
> +
> +	DEFINE_RES_IRQ_NAMED(BD96801_LDO5_PVIN_ERR_STAT, "bd96801-ldo5-pvin-err"),
> +	DEFINE_RES_IRQ_NAMED(BD96801_LDO5_OVP_ERR_STAT, "bd96801-ldo5-ovp-err"),
> +	DEFINE_RES_IRQ_NAMED(BD96801_LDO5_UVP_ERR_STAT, "bd96801-ldo5-uvp-err"),
> +	DEFINE_RES_IRQ_NAMED(BD96801_LDO5_SHDN_ERR_STAT, "bd96801-ldo5-shdn-err"),
> +
> +	DEFINE_RES_IRQ_NAMED(BD96801_LDO6_PVIN_ERR_STAT, "bd96801-ldo6-pvin-err"),
> +	DEFINE_RES_IRQ_NAMED(BD96801_LDO6_OVP_ERR_STAT, "bd96801-ldo6-ovp-err"),
> +	DEFINE_RES_IRQ_NAMED(BD96801_LDO6_UVP_ERR_STAT, "bd96801-ldo6-uvp-err"),
> +	DEFINE_RES_IRQ_NAMED(BD96801_LDO6_SHDN_ERR_STAT, "bd96801-ldo6-shdn-err"),
> +
> +	DEFINE_RES_IRQ_NAMED(BD96801_LDO7_PVIN_ERR_STAT, "bd96801-ldo7-pvin-err"),
> +	DEFINE_RES_IRQ_NAMED(BD96801_LDO7_OVP_ERR_STAT, "bd96801-ldo7-ovp-err"),
> +	DEFINE_RES_IRQ_NAMED(BD96801_LDO7_UVP_ERR_STAT, "bd96801-ldo7-uvp-err"),
> +	DEFINE_RES_IRQ_NAMED(BD96801_LDO7_SHDN_ERR_STAT, "bd96801-ldo7-shdn-err"),
> +};
> +
> +static const struct resource regulator_intb_irqs[] = {
> +	DEFINE_RES_IRQ_NAMED(BD96801_TW_STAT, "bd96801-core-thermal"),
> +
> +	DEFINE_RES_IRQ_NAMED(BD96801_BUCK1_OCPH_STAT, "bd96801-buck1-overcurr-h"),
> +	DEFINE_RES_IRQ_NAMED(BD96801_BUCK1_OCPL_STAT, "bd96801-buck1-overcurr-l"),
> +	DEFINE_RES_IRQ_NAMED(BD96801_BUCK1_OCPN_STAT, "bd96801-buck1-overcurr-n"),
> +	DEFINE_RES_IRQ_NAMED(BD96801_BUCK1_OVD_STAT, "bd96801-buck1-overvolt"),
> +	DEFINE_RES_IRQ_NAMED(BD96801_BUCK1_UVD_STAT, "bd96801-buck1-undervolt"),
> +	DEFINE_RES_IRQ_NAMED(BD96801_BUCK1_TW_CH_STAT, "bd96801-buck1-thermal"),
> +
> +	DEFINE_RES_IRQ_NAMED(BD96801_BUCK2_OCPH_STAT, "bd96801-buck2-overcurr-h"),
> +	DEFINE_RES_IRQ_NAMED(BD96801_BUCK2_OCPL_STAT, "bd96801-buck2-overcurr-l"),
> +	DEFINE_RES_IRQ_NAMED(BD96801_BUCK2_OCPN_STAT, "bd96801-buck2-overcurr-n"),
> +	DEFINE_RES_IRQ_NAMED(BD96801_BUCK2_OVD_STAT, "bd96801-buck2-overvolt"),
> +	DEFINE_RES_IRQ_NAMED(BD96801_BUCK2_UVD_STAT, "bd96801-buck2-undervolt"),
> +	DEFINE_RES_IRQ_NAMED(BD96801_BUCK2_TW_CH_STAT, "bd96801-buck2-thermal"),
> +
> +	DEFINE_RES_IRQ_NAMED(BD96801_BUCK3_OCPH_STAT, "bd96801-buck3-overcurr-h"),
> +	DEFINE_RES_IRQ_NAMED(BD96801_BUCK3_OCPL_STAT, "bd96801-buck3-overcurr-l"),
> +	DEFINE_RES_IRQ_NAMED(BD96801_BUCK3_OCPN_STAT, "bd96801-buck3-overcurr-n"),
> +	DEFINE_RES_IRQ_NAMED(BD96801_BUCK3_OVD_STAT, "bd96801-buck3-overvolt"),
> +	DEFINE_RES_IRQ_NAMED(BD96801_BUCK3_UVD_STAT, "bd96801-buck3-undervolt"),
> +	DEFINE_RES_IRQ_NAMED(BD96801_BUCK3_TW_CH_STAT, "bd96801-buck3-thermal"),
> +
> +	DEFINE_RES_IRQ_NAMED(BD96801_BUCK4_OCPH_STAT, "bd96801-buck4-overcurr-h"),
> +	DEFINE_RES_IRQ_NAMED(BD96801_BUCK4_OCPL_STAT, "bd96801-buck4-overcurr-l"),
> +	DEFINE_RES_IRQ_NAMED(BD96801_BUCK4_OCPN_STAT, "bd96801-buck4-overcurr-n"),
> +	DEFINE_RES_IRQ_NAMED(BD96801_BUCK4_OVD_STAT, "bd96801-buck4-overvolt"),
> +	DEFINE_RES_IRQ_NAMED(BD96801_BUCK4_UVD_STAT, "bd96801-buck4-undervolt"),
> +	DEFINE_RES_IRQ_NAMED(BD96801_BUCK4_TW_CH_STAT, "bd96801-buck4-thermal"),
> +
> +	DEFINE_RES_IRQ_NAMED(BD96801_LDO5_OCPH_STAT, "bd96801-ldo5-overcurr"),
> +	DEFINE_RES_IRQ_NAMED(BD96801_LDO5_OVD_STAT, "bd96801-ldo5-overvolt"),
> +	DEFINE_RES_IRQ_NAMED(BD96801_LDO5_UVD_STAT, "bd96801-ldo5-undervolt"),
> +
> +	DEFINE_RES_IRQ_NAMED(BD96801_LDO6_OCPH_STAT, "bd96801-ldo6-overcurr"),
> +	DEFINE_RES_IRQ_NAMED(BD96801_LDO6_OVD_STAT, "bd96801-ldo6-overvolt"),
> +	DEFINE_RES_IRQ_NAMED(BD96801_LDO6_UVD_STAT, "bd96801-ldo6-undervolt"),
> +
> +	DEFINE_RES_IRQ_NAMED(BD96801_LDO7_OCPH_STAT, "bd96801-ldo7-overcurr"),
> +	DEFINE_RES_IRQ_NAMED(BD96801_LDO7_OVD_STAT, "bd96801-ldo7-overvolt"),
> +	DEFINE_RES_IRQ_NAMED(BD96801_LDO7_UVD_STAT, "bd96801-ldo7-undervolt"),
> +};
> +
> +enum {
> +	WDG_CELL = 0,
> +	REGULATOR_CELL,
> +};
> +
> +static struct mfd_cell bd96801_mfd_cells[] = {
> +	[WDG_CELL] = { .name = "bd96801-wdt", },
> +	[REGULATOR_CELL] = { .name = "bd96801-pmic", },
> +};
> +
> +static const struct regmap_range bd96801_volatile_ranges[] = {
> +	/* Status regs */
> +	regmap_reg_range(BD96801_REG_WD_FEED, BD96801_REG_WD_FAILCOUNT),
> +	regmap_reg_range(BD96801_REG_WD_ASK, BD96801_REG_WD_ASK),
> +	regmap_reg_range(BD96801_REG_WD_STATUS, BD96801_REG_WD_STATUS),
> +	regmap_reg_range(BD96801_REG_PMIC_STATE, BD96801_REG_INT_LDO7_INTB),
> +	/* Registers which do not update value unless PMIC is in STBY */
> +	regmap_reg_range(BD96801_REG_SSCG_CTRL, BD96801_REG_SHD_INTB),
> +	regmap_reg_range(BD96801_REG_BUCK_OVP, BD96801_REG_BOOT_OVERTIME),
> +	/*
> +	 * LDO control registers have single bit (LDO MODE) which does not
> +	 * change when we write it unless PMIC is in STBY. It's safer to not
> +	 * cache it.
> +	 */
> +	regmap_reg_range(BD96801_LDO5_VOL_LVL_REG, BD96801_LDO7_VOL_LVL_REG),
> +};
> +
> +static const struct regmap_access_table volatile_regs = {
> +	.yes_ranges = bd96801_volatile_ranges,
> +	.n_yes_ranges = ARRAY_SIZE(bd96801_volatile_ranges),
> +};
> +
> +/*
> + * For ERRB we need main register bit mapping as bit(0) indicates active IRQ
> + * in one of the first 3 sub IRQ registers, For INTB we can use default 1 to 1
> + * mapping.
> + */
> +static unsigned int bit0_offsets[] = {0, 1, 2};	/* System stat, 3 registers */
> +static unsigned int bit1_offsets[] = {3};	/* Buck 1 stat */
> +static unsigned int bit2_offsets[] = {4};	/* Buck 2 stat */
> +static unsigned int bit3_offsets[] = {5};	/* Buck 3 stat */
> +static unsigned int bit4_offsets[] = {6};	/* Buck 4 stat */
> +static unsigned int bit5_offsets[] = {7};	/* LDO 5 stat */
> +static unsigned int bit6_offsets[] = {8};	/* LDO 6 stat */
> +static unsigned int bit7_offsets[] = {9};	/* LDO 7 stat */
> +
> +static struct regmap_irq_sub_irq_map errb_sub_irq_offsets[] = {
> +	REGMAP_IRQ_MAIN_REG_OFFSET(bit0_offsets),
> +	REGMAP_IRQ_MAIN_REG_OFFSET(bit1_offsets),
> +	REGMAP_IRQ_MAIN_REG_OFFSET(bit2_offsets),
> +	REGMAP_IRQ_MAIN_REG_OFFSET(bit3_offsets),
> +	REGMAP_IRQ_MAIN_REG_OFFSET(bit4_offsets),
> +	REGMAP_IRQ_MAIN_REG_OFFSET(bit5_offsets),
> +	REGMAP_IRQ_MAIN_REG_OFFSET(bit6_offsets),
> +	REGMAP_IRQ_MAIN_REG_OFFSET(bit7_offsets),
> +};
> +
> +static const struct regmap_irq bd96801_errb_irqs[] = {
> +	/* Reg 0x52 Fatal ERRB1 */
> +	REGMAP_IRQ_REG(BD96801_OTP_ERR_STAT, 0, BD96801_OTP_ERR_MASK),
> +	REGMAP_IRQ_REG(BD96801_DBIST_ERR_STAT, 0, BD96801_DBIST_ERR_MASK),
> +	REGMAP_IRQ_REG(BD96801_EEP_ERR_STAT, 0, BD96801_EEP_ERR_MASK),
> +	REGMAP_IRQ_REG(BD96801_ABIST_ERR_STAT, 0, BD96801_ABIST_ERR_MASK),
> +	REGMAP_IRQ_REG(BD96801_PRSTB_ERR_STAT, 0, BD96801_PRSTB_ERR_MASK),
> +	REGMAP_IRQ_REG(BD96801_DRMOS1_ERR_STAT, 0, BD96801_DRMOS1_ERR_MASK),
> +	REGMAP_IRQ_REG(BD96801_DRMOS2_ERR_STAT, 0, BD96801_DRMOS2_ERR_MASK),
> +	REGMAP_IRQ_REG(BD96801_SLAVE_ERR_STAT, 0, BD96801_SLAVE_ERR_MASK),
> +	/* 0x53 Fatal ERRB2 */
> +	REGMAP_IRQ_REG(BD96801_VREF_ERR_STAT, 1, BD96801_VREF_ERR_MASK),
> +	REGMAP_IRQ_REG(BD96801_TSD_ERR_STAT, 1, BD96801_TSD_ERR_MASK),
> +	REGMAP_IRQ_REG(BD96801_UVLO_ERR_STAT, 1, BD96801_UVLO_ERR_MASK),
> +	REGMAP_IRQ_REG(BD96801_OVLO_ERR_STAT, 1, BD96801_OVLO_ERR_MASK),
> +	REGMAP_IRQ_REG(BD96801_OSC_ERR_STAT, 1, BD96801_OSC_ERR_MASK),
> +	REGMAP_IRQ_REG(BD96801_PON_ERR_STAT, 1, BD96801_PON_ERR_MASK),
> +	REGMAP_IRQ_REG(BD96801_POFF_ERR_STAT, 1, BD96801_POFF_ERR_MASK),
> +	REGMAP_IRQ_REG(BD96801_CMD_SHDN_ERR_STAT, 1, BD96801_CMD_SHDN_ERR_MASK),
> +	/* 0x54 Fatal INTB shadowed to ERRB */
> +	REGMAP_IRQ_REG(BD96801_INT_PRSTB_WDT_ERR, 2, BD96801_INT_PRSTB_WDT_ERR_MASK),
> +	REGMAP_IRQ_REG(BD96801_INT_CHIP_IF_ERR, 2, BD96801_INT_CHIP_IF_ERR_MASK),
> +	REGMAP_IRQ_REG(BD96801_INT_SHDN_ERR_STAT, 2, BD96801_INT_SHDN_ERR_MASK),
> +	/* Reg 0x55 BUCK1 ERR IRQs */
> +	REGMAP_IRQ_REG(BD96801_BUCK1_PVIN_ERR_STAT, 3, BD96801_OUT_PVIN_ERR_MASK),
> +	REGMAP_IRQ_REG(BD96801_BUCK1_OVP_ERR_STAT, 3, BD96801_OUT_OVP_ERR_MASK),
> +	REGMAP_IRQ_REG(BD96801_BUCK1_UVP_ERR_STAT, 3, BD96801_OUT_UVP_ERR_MASK),
> +	REGMAP_IRQ_REG(BD96801_BUCK1_SHDN_ERR_STAT, 3, BD96801_OUT_SHDN_ERR_MASK),
> +	/* Reg 0x56 BUCK2 ERR IRQs */
> +	REGMAP_IRQ_REG(BD96801_BUCK2_PVIN_ERR_STAT, 4, BD96801_OUT_PVIN_ERR_MASK),
> +	REGMAP_IRQ_REG(BD96801_BUCK2_OVP_ERR_STAT, 4, BD96801_OUT_OVP_ERR_MASK),
> +	REGMAP_IRQ_REG(BD96801_BUCK2_UVP_ERR_STAT, 4, BD96801_OUT_UVP_ERR_MASK),
> +	REGMAP_IRQ_REG(BD96801_BUCK2_SHDN_ERR_STAT, 4, BD96801_OUT_SHDN_ERR_MASK),
> +	/* Reg 0x57 BUCK3 ERR IRQs */
> +	REGMAP_IRQ_REG(BD96801_BUCK3_PVIN_ERR_STAT, 5, BD96801_OUT_PVIN_ERR_MASK),
> +	REGMAP_IRQ_REG(BD96801_BUCK3_OVP_ERR_STAT, 5, BD96801_OUT_OVP_ERR_MASK),
> +	REGMAP_IRQ_REG(BD96801_BUCK3_UVP_ERR_STAT, 5, BD96801_OUT_UVP_ERR_MASK),
> +	REGMAP_IRQ_REG(BD96801_BUCK3_SHDN_ERR_STAT, 5, BD96801_OUT_SHDN_ERR_MASK),
> +	/* Reg 0x58 BUCK4 ERR IRQs */
> +	REGMAP_IRQ_REG(BD96801_BUCK4_PVIN_ERR_STAT, 6, BD96801_OUT_PVIN_ERR_MASK),
> +	REGMAP_IRQ_REG(BD96801_BUCK4_OVP_ERR_STAT, 6, BD96801_OUT_OVP_ERR_MASK),
> +	REGMAP_IRQ_REG(BD96801_BUCK4_UVP_ERR_STAT, 6, BD96801_OUT_UVP_ERR_MASK),
> +	REGMAP_IRQ_REG(BD96801_BUCK4_SHDN_ERR_STAT, 6, BD96801_OUT_SHDN_ERR_MASK),
> +	/* Reg 0x59 LDO5 ERR IRQs */
> +	REGMAP_IRQ_REG(BD96801_LDO5_PVIN_ERR_STAT, 7, BD96801_OUT_PVIN_ERR_MASK),
> +	REGMAP_IRQ_REG(BD96801_LDO5_OVP_ERR_STAT, 7, BD96801_OUT_OVP_ERR_MASK),
> +	REGMAP_IRQ_REG(BD96801_LDO5_UVP_ERR_STAT, 7, BD96801_OUT_UVP_ERR_MASK),
> +	REGMAP_IRQ_REG(BD96801_LDO5_SHDN_ERR_STAT, 7, BD96801_OUT_SHDN_ERR_MASK),
> +	/* Reg 0x5a LDO6 ERR IRQs */
> +	REGMAP_IRQ_REG(BD96801_LDO6_PVIN_ERR_STAT, 8, BD96801_OUT_PVIN_ERR_MASK),
> +	REGMAP_IRQ_REG(BD96801_LDO6_OVP_ERR_STAT, 8, BD96801_OUT_OVP_ERR_MASK),
> +	REGMAP_IRQ_REG(BD96801_LDO6_UVP_ERR_STAT, 8, BD96801_OUT_UVP_ERR_MASK),
> +	REGMAP_IRQ_REG(BD96801_LDO6_SHDN_ERR_STAT, 8, BD96801_OUT_SHDN_ERR_MASK),
> +	/* Reg 0x5b LDO7 ERR IRQs */
> +	REGMAP_IRQ_REG(BD96801_LDO7_PVIN_ERR_STAT, 9, BD96801_OUT_PVIN_ERR_MASK),
> +	REGMAP_IRQ_REG(BD96801_LDO7_OVP_ERR_STAT, 9, BD96801_OUT_OVP_ERR_MASK),
> +	REGMAP_IRQ_REG(BD96801_LDO7_UVP_ERR_STAT, 9, BD96801_OUT_UVP_ERR_MASK),
> +	REGMAP_IRQ_REG(BD96801_LDO7_SHDN_ERR_STAT, 9, BD96801_OUT_SHDN_ERR_MASK),
> +};
> +
> +static const struct regmap_irq bd96801_intb_irqs[] = {
> +	/* STATUS SYSTEM INTB */
> +	REGMAP_IRQ_REG(BD96801_TW_STAT, 0, BD96801_TW_STAT_MASK),
> +	REGMAP_IRQ_REG(BD96801_WDT_ERR_STAT, 0, BD96801_WDT_ERR_STAT_MASK),
> +	REGMAP_IRQ_REG(BD96801_I2C_ERR_STAT, 0, BD96801_I2C_ERR_STAT_MASK),
> +	REGMAP_IRQ_REG(BD96801_CHIP_IF_ERR_STAT, 0, BD96801_CHIP_IF_ERR_STAT_MASK),
> +	/* STATUS BUCK1 INTB */
> +	REGMAP_IRQ_REG(BD96801_BUCK1_OCPH_STAT, 1, BD96801_BUCK_OCPH_STAT_MASK),
> +	REGMAP_IRQ_REG(BD96801_BUCK1_OCPL_STAT, 1, BD96801_BUCK_OCPL_STAT_MASK),
> +	REGMAP_IRQ_REG(BD96801_BUCK1_OCPN_STAT, 1, BD96801_BUCK_OCPN_STAT_MASK),
> +	REGMAP_IRQ_REG(BD96801_BUCK1_OVD_STAT, 1, BD96801_BUCK_OVD_STAT_MASK),
> +	REGMAP_IRQ_REG(BD96801_BUCK1_UVD_STAT, 1, BD96801_BUCK_UVD_STAT_MASK),
> +	REGMAP_IRQ_REG(BD96801_BUCK1_TW_CH_STAT, 1, BD96801_BUCK_TW_CH_STAT_MASK),
> +	/* BUCK 2 INTB */
> +	REGMAP_IRQ_REG(BD96801_BUCK2_OCPH_STAT, 2, BD96801_BUCK_OCPH_STAT_MASK),
> +	REGMAP_IRQ_REG(BD96801_BUCK2_OCPL_STAT, 2, BD96801_BUCK_OCPL_STAT_MASK),
> +	REGMAP_IRQ_REG(BD96801_BUCK2_OCPN_STAT, 2, BD96801_BUCK_OCPN_STAT_MASK),
> +	REGMAP_IRQ_REG(BD96801_BUCK2_OVD_STAT, 2, BD96801_BUCK_OVD_STAT_MASK),
> +	REGMAP_IRQ_REG(BD96801_BUCK2_UVD_STAT, 2, BD96801_BUCK_UVD_STAT_MASK),
> +	REGMAP_IRQ_REG(BD96801_BUCK2_TW_CH_STAT, 2, BD96801_BUCK_TW_CH_STAT_MASK),
> +	/* BUCK 3 INTB */
> +	REGMAP_IRQ_REG(BD96801_BUCK3_OCPH_STAT, 3, BD96801_BUCK_OCPH_STAT_MASK),
> +	REGMAP_IRQ_REG(BD96801_BUCK3_OCPL_STAT, 3, BD96801_BUCK_OCPL_STAT_MASK),
> +	REGMAP_IRQ_REG(BD96801_BUCK3_OCPN_STAT, 3, BD96801_BUCK_OCPN_STAT_MASK),
> +	REGMAP_IRQ_REG(BD96801_BUCK3_OVD_STAT, 3, BD96801_BUCK_OVD_STAT_MASK),
> +	REGMAP_IRQ_REG(BD96801_BUCK3_UVD_STAT, 3, BD96801_BUCK_UVD_STAT_MASK),
> +	REGMAP_IRQ_REG(BD96801_BUCK3_TW_CH_STAT, 3, BD96801_BUCK_TW_CH_STAT_MASK),
> +	/* BUCK 4 INTB */
> +	REGMAP_IRQ_REG(BD96801_BUCK4_OCPH_STAT, 4, BD96801_BUCK_OCPH_STAT_MASK),
> +	REGMAP_IRQ_REG(BD96801_BUCK4_OCPL_STAT, 4, BD96801_BUCK_OCPL_STAT_MASK),
> +	REGMAP_IRQ_REG(BD96801_BUCK4_OCPN_STAT, 4, BD96801_BUCK_OCPN_STAT_MASK),
> +	REGMAP_IRQ_REG(BD96801_BUCK4_OVD_STAT, 4, BD96801_BUCK_OVD_STAT_MASK),
> +	REGMAP_IRQ_REG(BD96801_BUCK4_UVD_STAT, 4, BD96801_BUCK_UVD_STAT_MASK),
> +	REGMAP_IRQ_REG(BD96801_BUCK4_TW_CH_STAT, 4, BD96801_BUCK_TW_CH_STAT_MASK),
> +	/* LDO5 INTB */
> +	REGMAP_IRQ_REG(BD96801_LDO5_OCPH_STAT, 5, BD96801_LDO_OCPH_STAT_MASK),
> +	REGMAP_IRQ_REG(BD96801_LDO5_OVD_STAT, 5, BD96801_LDO_OVD_STAT_MASK),
> +	REGMAP_IRQ_REG(BD96801_LDO5_UVD_STAT, 5, BD96801_LDO_UVD_STAT_MASK),
> +	/* LDO6 INTB */
> +	REGMAP_IRQ_REG(BD96801_LDO6_OCPH_STAT, 6, BD96801_LDO_OCPH_STAT_MASK),
> +	REGMAP_IRQ_REG(BD96801_LDO6_OVD_STAT, 6, BD96801_LDO_OVD_STAT_MASK),
> +	REGMAP_IRQ_REG(BD96801_LDO6_UVD_STAT, 6, BD96801_LDO_UVD_STAT_MASK),
> +	/* LDO7 INTB */
> +	REGMAP_IRQ_REG(BD96801_LDO7_OCPH_STAT, 7, BD96801_LDO_OCPH_STAT_MASK),
> +	REGMAP_IRQ_REG(BD96801_LDO7_OVD_STAT, 7, BD96801_LDO_OVD_STAT_MASK),
> +	REGMAP_IRQ_REG(BD96801_LDO7_UVD_STAT, 7, BD96801_LDO_UVD_STAT_MASK),
> +};
> +
> +static struct regmap_irq_chip bd96801_irq_chip_errb = {
> +	.name = "bd96801-irq-errb",
> +	.main_status = BD96801_REG_INT_MAIN,
> +	.num_main_regs = 1,
> +	.irqs = &bd96801_errb_irqs[0],
> +	.num_irqs = ARRAY_SIZE(bd96801_errb_irqs),
> +	.status_base = BD96801_REG_INT_SYS_ERRB1,
> +	.mask_base = BD96801_REG_MASK_SYS_ERRB,
> +	.ack_base = BD96801_REG_INT_SYS_ERRB1,
> +	.init_ack_masked = true,
> +	.num_regs = 10,
> +	.irq_reg_stride = 1,
> +	.sub_reg_offsets = &errb_sub_irq_offsets[0],
> +};
> +
> +static struct regmap_irq_chip bd96801_irq_chip_intb = {
> +	.name = "bd96801-irq-intb",
> +	.main_status = BD96801_REG_INT_MAIN,
> +	.num_main_regs = 1,
> +	.irqs = &bd96801_intb_irqs[0],
> +	.num_irqs = ARRAY_SIZE(bd96801_intb_irqs),
> +	.status_base = BD96801_REG_INT_SYS_INTB,
> +	.mask_base = BD96801_REG_MASK_SYS_INTB,
> +	.ack_base = BD96801_REG_INT_SYS_INTB,
> +	.init_ack_masked = true,
> +	.num_regs = 8,
> +	.irq_reg_stride = 1,
> +};
> +
> +static const struct regmap_config bd96801_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +	.volatile_table = &volatile_regs,
> +	.cache_type = REGCACHE_RBTREE,
> +};
> +
> +static int bd96801_i2c_probe(struct i2c_client *i2c)
> +{
> +	int i, ret, intb_irq, errb_irq, num_regu_irqs, num_intb, num_errb = 0;
> +	struct regmap_irq_chip_data *intb_irq_data, *errb_irq_data;
> +	struct irq_domain *intb_domain, *errb_domain;
> +	const struct fwnode_handle *fwnode;
> +	struct resource *regulator_res;
> +	struct regmap *regmap;
> +
> +	fwnode = dev_fwnode(&i2c->dev);
> +	if (!fwnode) {
> +		dev_err(&i2c->dev, "no fwnode\n");
> +		return -EINVAL;

Why not dev_err_probe() here for uniformity?

> +	}
> +
> +	intb_irq = fwnode_irq_get_byname(fwnode, "intb");
> +	if (intb_irq < 0)
> +		return dev_err_probe(&i2c->dev, intb_irq,
> +				     "No INTB IRQ configured\n");

This function would look nicer if you expanded to 100-chars.

> +	num_intb =  ARRAY_SIZE(regulator_intb_irqs);
> +
> +	/* ERRB may be omitted if processor is powered by the PMIC */
> +	errb_irq = fwnode_irq_get_byname(fwnode, "errb");
> +	if (errb_irq < 0)
> +		errb_irq = 0;
> +
> +	if (errb_irq)
> +		num_errb = ARRAY_SIZE(regulator_errb_irqs);
> +
> +	num_regu_irqs = num_intb + num_errb;
> +
> +	regulator_res = kcalloc(num_regu_irqs, sizeof(*regulator_res),
> +				GFP_KERNEL);
> +	if (!regulator_res)
> +		return -ENOMEM;
> +
> +	regmap = devm_regmap_init_i2c(i2c, &bd96801_regmap_config);
> +	if (IS_ERR(regmap)) {
> +		ret = dev_err_probe(&i2c->dev, PTR_ERR(regmap),
> +				    "regmap initialization failed\n");
> +		goto free_out;
> +	}

No need to clump these together - '\n' here please.

> +	ret = devm_regmap_add_irq_chip(&i2c->dev, regmap, intb_irq,
> +				       IRQF_ONESHOT, 0, &bd96801_irq_chip_intb,
> +				       &intb_irq_data);
> +	if (ret) {
> +		dev_err_probe(&i2c->dev, ret, "Failed to add INTB irq_chip\n");
> +		goto free_out;
> +	}
> +
> +	/*
> +	 * MFD core code is built to handle only one IRQ domain. BD96801
> +	 * has two domains so we do IRQ mapping here and provide the
> +	 * already mapped IRQ numbers to sub-devices.
> +	 */
> +	intb_domain = regmap_irq_get_domain(intb_irq_data);
> +
> +	for (i = 0; i < num_intb; i++) {
> +		struct resource *res = &regulator_res[i];
> +
> +		*res = regulator_intb_irqs[i];
> +		res->start = res->end = irq_create_mapping(intb_domain,
> +							    res->start);
> +	}
> +
> +	if (num_errb) {
> +		ret = devm_regmap_add_irq_chip(&i2c->dev, regmap, errb_irq,
> +					       IRQF_ONESHOT, 0,
> +					       &bd96801_irq_chip_errb,
> +					       &errb_irq_data);
> +		if (ret) {
> +			dev_err_probe(&i2c->dev, ret,
> +				      "Failed to add ERRB (%d) irq_chip\n",
> +				      errb_irq);
> +			goto free_out;
> +		}
> +		errb_domain = regmap_irq_get_domain(errb_irq_data);
> +
> +		for (i = 0; i < num_errb; i++) {
> +			struct resource *res = &regulator_res[num_intb + i];
> +
> +			*res = regulator_errb_irqs[i];
> +			res->start = res->end = irq_create_mapping(errb_domain,
> +								   res->start);
> +		}
> +	}
> +
> +	bd96801_mfd_cells[REGULATOR_CELL].resources = regulator_res;
> +	bd96801_mfd_cells[REGULATOR_CELL].num_resources = num_regu_irqs;

'\n' here.

> +	ret = devm_mfd_add_devices(&i2c->dev, PLATFORM_DEVID_AUTO,
> +				   bd96801_mfd_cells,
> +				   ARRAY_SIZE(bd96801_mfd_cells), NULL, 0, NULL);
> +	if (ret)
> +		dev_err_probe(&i2c->dev, ret, "Failed to create subdevices\n");
> +
> +free_out:
> +	kfree(regulator_res);
> +
> +	return ret;
> +}
> +
> +static const struct of_device_id bd96801_of_match[] = {
> +	{
> +		.compatible = "rohm,bd96801",
> +	},

Should be on a single line.

> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, bd96801_of_match);
> +
> +static struct i2c_driver bd96801_i2c_driver = {
> +	.driver = {
> +		.name = "rohm-bd96801",
> +		.of_match_table = bd96801_of_match,
> +	},
> +	.probe = bd96801_i2c_probe,
> +};
> +
> +static int __init bd96801_i2c_init(void)
> +{
> +	return i2c_add_driver(&bd96801_i2c_driver);
> +}

'\n'

> +/* Initialise early so consumer devices can complete system boot */
> +subsys_initcall(bd96801_i2c_init);
> +
> +static void __exit bd96801_i2c_exit(void)
> +{
> +	i2c_del_driver(&bd96801_i2c_driver);
> +}
> +module_exit(bd96801_i2c_exit);
> +
> +MODULE_AUTHOR("Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>");
> +MODULE_DESCRIPTION("ROHM BD96801 Power Management IC driver");
> +MODULE_LICENSE("GPL");
> diff --git a/include/linux/mfd/rohm-bd96801.h b/include/linux/mfd/rohm-bd96801.h
> new file mode 100644
> index 000000000000..47b07171dcb2
> --- /dev/null
> +++ b/include/linux/mfd/rohm-bd96801.h
> @@ -0,0 +1,212 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/* Copyright (C) 2020 ROHM Semiconductors */
> +
> +#ifndef __LINUX_MFD_BD96801_H__
> +#define __LINUX_MFD_BD96801_H__

Drop the LINUX_ part.

> +#define BD96801_REG_SSCG_CTRL		0x09
> +#define BD96801_REG_SHD_INTB            0x20
> +#define BD96801_LDO5_VOL_LVL_REG	0x2c
> +#define BD96801_LDO6_VOL_LVL_REG	0x2d
> +#define BD96801_LDO7_VOL_LVL_REG	0x2e
> +#define BD96801_REG_BUCK_OVP		0x30
> +#define BD96801_REG_BUCK_OVD		0x35
> +#define BD96801_REG_LDO_OVP		0x31
> +#define BD96801_REG_LDO_OVD		0x36
> +#define BD96801_REG_BOOT_OVERTIME	0x3a
> +#define BD96801_REG_WD_TMO		0x40
> +#define BD96801_REG_WD_CONF		0x41
> +#define BD96801_REG_WD_FEED		0x42
> +#define BD96801_REG_WD_FAILCOUNT	0x43
> +#define BD96801_REG_WD_ASK		0x46
> +#define BD96801_REG_WD_STATUS		0x4a
> +#define BD96801_REG_PMIC_STATE		0x4f
> +#define BD96801_REG_EXT_STATE		0x50
> +
> +#define BD96801_STATE_STBY		0x09
> +
> +/* IRQ register area */
> +#define BD96801_REG_INT_MAIN		0x51
> +
> +/*
> + * The BD96801 has two physical IRQ lines, INTB and ERRB.
> + * For now we just handle the INTB.
> + *
> + * The 'main status register' is located at 0x51.
> + * The ERRB status registers are located at 0x52 ... 0x5B
> + * INTB status registers are at range 0x5c ... 0x63
> + */
> +#define BD96801_REG_INT_SYS_ERRB1	0x52
> +#define BD96801_REG_INT_SYS_INTB	0x5c
> +#define BD96801_REG_INT_LDO7_INTB	0x63
> +
> +/* MASK registers */
> +#define BD96801_REG_MASK_SYS_INTB	0x73
> +#define BD96801_REG_MASK_SYS_ERRB	0x69
> +
> +#define BD96801_MAX_REGISTER		0x7a
> +
> +#define BD96801_OTP_ERR_MASK		BIT(0)
> +#define BD96801_DBIST_ERR_MASK		BIT(1)
> +#define BD96801_EEP_ERR_MASK		BIT(2)
> +#define BD96801_ABIST_ERR_MASK		BIT(3)
> +#define BD96801_PRSTB_ERR_MASK		BIT(4)
> +#define BD96801_DRMOS1_ERR_MASK		BIT(5)
> +#define BD96801_DRMOS2_ERR_MASK		BIT(6)
> +#define BD96801_SLAVE_ERR_MASK		BIT(7)
> +#define BD96801_VREF_ERR_MASK		BIT(0)
> +#define BD96801_TSD_ERR_MASK		BIT(1)
> +#define BD96801_UVLO_ERR_MASK		BIT(2)
> +#define BD96801_OVLO_ERR_MASK		BIT(3)
> +#define BD96801_OSC_ERR_MASK		BIT(4)
> +#define BD96801_PON_ERR_MASK		BIT(5)
> +#define BD96801_POFF_ERR_MASK		BIT(6)
> +#define BD96801_CMD_SHDN_ERR_MASK	BIT(7)
> +#define BD96801_INT_PRSTB_WDT_ERR_MASK	BIT(0)
> +#define BD96801_INT_CHIP_IF_ERR_MASK	BIT(3)
> +#define BD96801_INT_SHDN_ERR_MASK	BIT(7)
> +#define BD96801_OUT_PVIN_ERR_MASK	BIT(0)
> +#define BD96801_OUT_OVP_ERR_MASK	BIT(1)
> +#define BD96801_OUT_UVP_ERR_MASK	BIT(2)
> +#define BD96801_OUT_SHDN_ERR_MASK	BIT(7)
> +
> +/* ERRB IRQs */
> +enum {
> +	/* Reg 0x52, 0x53, 0x54 - ERRB system IRQs */
> +	BD96801_OTP_ERR_STAT,
> +	BD96801_DBIST_ERR_STAT,
> +	BD96801_EEP_ERR_STAT,
> +	BD96801_ABIST_ERR_STAT,
> +	BD96801_PRSTB_ERR_STAT,
> +	BD96801_DRMOS1_ERR_STAT,
> +	BD96801_DRMOS2_ERR_STAT,
> +	BD96801_SLAVE_ERR_STAT,
> +	BD96801_VREF_ERR_STAT,
> +	BD96801_TSD_ERR_STAT,
> +	BD96801_UVLO_ERR_STAT,
> +	BD96801_OVLO_ERR_STAT,
> +	BD96801_OSC_ERR_STAT,
> +	BD96801_PON_ERR_STAT,
> +	BD96801_POFF_ERR_STAT,
> +	BD96801_CMD_SHDN_ERR_STAT,
> +	BD96801_INT_PRSTB_WDT_ERR,
> +	BD96801_INT_CHIP_IF_ERR,
> +	BD96801_INT_SHDN_ERR_STAT,
> +
> +	/* Reg 0x55 BUCK1 ERR IRQs */
> +	BD96801_BUCK1_PVIN_ERR_STAT,
> +	BD96801_BUCK1_OVP_ERR_STAT,
> +	BD96801_BUCK1_UVP_ERR_STAT,
> +	BD96801_BUCK1_SHDN_ERR_STAT,
> +
> +	/* Reg 0x56 BUCK2 ERR IRQs */
> +	BD96801_BUCK2_PVIN_ERR_STAT,
> +	BD96801_BUCK2_OVP_ERR_STAT,
> +	BD96801_BUCK2_UVP_ERR_STAT,
> +	BD96801_BUCK2_SHDN_ERR_STAT,
> +
> +	/* Reg 0x57 BUCK3 ERR IRQs */
> +	BD96801_BUCK3_PVIN_ERR_STAT,
> +	BD96801_BUCK3_OVP_ERR_STAT,
> +	BD96801_BUCK3_UVP_ERR_STAT,
> +	BD96801_BUCK3_SHDN_ERR_STAT,
> +
> +	/* Reg 0x58 BUCK4 ERR IRQs */
> +	BD96801_BUCK4_PVIN_ERR_STAT,
> +	BD96801_BUCK4_OVP_ERR_STAT,
> +	BD96801_BUCK4_UVP_ERR_STAT,
> +	BD96801_BUCK4_SHDN_ERR_STAT,
> +
> +	/* Reg 0x59 LDO5 ERR IRQs */
> +	BD96801_LDO5_PVIN_ERR_STAT,
> +	BD96801_LDO5_OVP_ERR_STAT,
> +	BD96801_LDO5_UVP_ERR_STAT,
> +	BD96801_LDO5_SHDN_ERR_STAT,
> +
> +	/* Reg 0x5a LDO6 ERR IRQs */
> +	BD96801_LDO6_PVIN_ERR_STAT,
> +	BD96801_LDO6_OVP_ERR_STAT,
> +	BD96801_LDO6_UVP_ERR_STAT,
> +	BD96801_LDO6_SHDN_ERR_STAT,
> +
> +	/* Reg 0x5b LDO7 ERR IRQs */
> +	BD96801_LDO7_PVIN_ERR_STAT,
> +	BD96801_LDO7_OVP_ERR_STAT,
> +	BD96801_LDO7_UVP_ERR_STAT,
> +	BD96801_LDO7_SHDN_ERR_STAT,
> +};
> +
> +/* INTB IRQs */
> +enum {
> +	/* Reg 0x5c (System INTB) */
> +	BD96801_TW_STAT,
> +	BD96801_WDT_ERR_STAT,
> +	BD96801_I2C_ERR_STAT,
> +	BD96801_CHIP_IF_ERR_STAT,
> +
> +	/* Reg 0x5d (BUCK1 INTB) */
> +	BD96801_BUCK1_OCPH_STAT,
> +	BD96801_BUCK1_OCPL_STAT,
> +	BD96801_BUCK1_OCPN_STAT,
> +	BD96801_BUCK1_OVD_STAT,
> +	BD96801_BUCK1_UVD_STAT,
> +	BD96801_BUCK1_TW_CH_STAT,
> +
> +	/* Reg 0x5e (BUCK2 INTB) */
> +	BD96801_BUCK2_OCPH_STAT,
> +	BD96801_BUCK2_OCPL_STAT,
> +	BD96801_BUCK2_OCPN_STAT,
> +	BD96801_BUCK2_OVD_STAT,
> +	BD96801_BUCK2_UVD_STAT,
> +	BD96801_BUCK2_TW_CH_STAT,
> +
> +	/* Reg 0x5f (BUCK3 INTB)*/
> +	BD96801_BUCK3_OCPH_STAT,
> +	BD96801_BUCK3_OCPL_STAT,
> +	BD96801_BUCK3_OCPN_STAT,
> +	BD96801_BUCK3_OVD_STAT,
> +	BD96801_BUCK3_UVD_STAT,
> +	BD96801_BUCK3_TW_CH_STAT,
> +
> +	/* Reg 0x60 (BUCK4 INTB)*/
> +	BD96801_BUCK4_OCPH_STAT,
> +	BD96801_BUCK4_OCPL_STAT,
> +	BD96801_BUCK4_OCPN_STAT,
> +	BD96801_BUCK4_OVD_STAT,
> +	BD96801_BUCK4_UVD_STAT,
> +	BD96801_BUCK4_TW_CH_STAT,
> +
> +	/* Reg 0x61 (LDO5 INTB) */
> +	BD96801_LDO5_OCPH_STAT, //bit [0]
> +	BD96801_LDO5_OVD_STAT,	//bit [3]
> +	BD96801_LDO5_UVD_STAT,  //bit [4]
> +
> +	/* Reg 0x62 (LDO6 INTB) */
> +	BD96801_LDO6_OCPH_STAT, //bit [0]
> +	BD96801_LDO6_OVD_STAT,	//bit [3]
> +	BD96801_LDO6_UVD_STAT,  //bit [4]
> +
> +	/* Reg 0x63 (LDO7 INTB) */
> +	BD96801_LDO7_OCPH_STAT, //bit [0]
> +	BD96801_LDO7_OVD_STAT,	//bit [3]
> +	BD96801_LDO7_UVD_STAT,  //bit [4]
> +};
> +
> +/* IRQ MASKs */
> +#define BD96801_TW_STAT_MASK		BIT(0)
> +#define BD96801_WDT_ERR_STAT_MASK	BIT(1)
> +#define BD96801_I2C_ERR_STAT_MASK	BIT(2)
> +#define BD96801_CHIP_IF_ERR_STAT_MASK	BIT(3)
> +
> +#define BD96801_BUCK_OCPH_STAT_MASK	BIT(0)
> +#define BD96801_BUCK_OCPL_STAT_MASK	BIT(1)
> +#define BD96801_BUCK_OCPN_STAT_MASK	BIT(2)
> +#define BD96801_BUCK_OVD_STAT_MASK	BIT(3)
> +#define BD96801_BUCK_UVD_STAT_MASK	BIT(4)
> +#define BD96801_BUCK_TW_CH_STAT_MASK	BIT(5)
> +
> +#define BD96801_LDO_OCPH_STAT_MASK	BIT(0)
> +#define BD96801_LDO_OVD_STAT_MASK	BIT(3)
> +#define BD96801_LDO_UVD_STAT_MASK	BIT(4)
> +
> +#endif
> diff --git a/include/linux/mfd/rohm-generic.h b/include/linux/mfd/rohm-generic.h
> index 4eeb22876bad..e7d4e6afe388 100644
> --- a/include/linux/mfd/rohm-generic.h
> +++ b/include/linux/mfd/rohm-generic.h
> @@ -16,6 +16,7 @@ enum rohm_chip_type {
>  	ROHM_CHIP_TYPE_BD71828,
>  	ROHM_CHIP_TYPE_BD71837,
>  	ROHM_CHIP_TYPE_BD71847,
> +	ROHM_CHIP_TYPE_BD96801,
>  	ROHM_CHIP_TYPE_AMOUNT
>  };
>  
> -- 
> 2.43.2
> 
> 
> -- 
> Matti Vaittinen, Linux device drivers
> ROHM Semiconductors, Finland SWDC
> Kiviharjunlenkki 1E
> 90220 OULU
> FINLAND
> 
> ~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
> Simon says - in Latin please.
> ~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
> Thanks to Simon Glass for the translation =] 



-- 
Lee Jones [李琼斯]

Re: [RFC PATCH 3/6] mfd: support ROHM BD96801 PMIC core

[Matti Vaittinen]

Hi deee Ho Lee!

Thanks a ton for taking a look at this :) I already sent the V2 
yesterday, briefly before receiving your comments. I think all of the 
comments are relevant for the V2 as well, I will fix them for the V3 
when I get to that. If you find the time to take a look at V2, then the 
major things are addition of a watchdog IRQ + a work-around for the 
debugFS name collision for IRQ domains.

On 4/11/24 17:38, Lee Jones wrote:
> On Tue, 02 Apr 2024, Matti Vaittinen wrote:
> 
>> The ROHM BD96801 PMIC is highly customizable automotive grade PMIC
>> which integrates regulator and watchdog funtionalities.
>>
>> Provide IRQ and register accesses for regulator/watchdog drivers.
>>
>> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
>> ---
>>   drivers/mfd/Kconfig              |  13 +
>>   drivers/mfd/Makefile             |   1 +
>>   drivers/mfd/rohm-bd96801.c       | 454 +++++++++++++++++++++++++++++++
>>   include/linux/mfd/rohm-bd96801.h | 212 +++++++++++++++
>>   include/linux/mfd/rohm-generic.h |   1 +
>>   5 files changed, 681 insertions(+)
>>   create mode 100644 drivers/mfd/rohm-bd96801.c
>>   create mode 100644 include/linux/mfd/rohm-bd96801.h
>>
>> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
>> index 4b023ee229cf..947045eb3a8e 100644
>> --- a/drivers/mfd/Kconfig
>> +++ b/drivers/mfd/Kconfig
>> @@ -2089,6 +2089,19 @@ config MFD_ROHM_BD957XMUF
>>   	  BD9573MUF Power Management ICs. BD9576 and BD9573 are primarily
>>   	  designed to be used to power R-Car series processors.
>>   
>> +config MFD_ROHM_BD96801
>> +	tristate "ROHM BD96801 Power Management IC"
>> +	depends on I2C=y
>> +	depends on OF
>> +	select REGMAP_I2C
>> +	select REGMAP_IRQ
>> +	select MFD_CORE
>> +	help
>> +	  Select this option to get support for the ROHM BD96801 Power
>> +	  Management IC. The ROHM BD96801 is a highly scalable power management
> 
> Power Management

Out of the curiosity, why is the "Power Management IC" written with 
capitals, when speaking of a class of devices instead of a model? (I am 
100% fine with the change, just curious).

> 
>> +	  IC for industrial and automotive use. The BD96801 can be used as a
>> +	  master PMIC in a chained PMIC solutions with suitable companion PMICs
...

>> +// SPDX-License-Identifier: GPL-2.0-only
>> +//
>> +// Copyright (C) 2022 ROHM Semiconductors
> 
> No updates for 2 years?

The year should be updated - thanks. But, now that you asked...  Almost 
no updates. The patches have rotten in my outbox, waiting for the 
permisson to be sent out... But yeah, I've sure added some changes 
before sending the series - I'll update the copyright :)

>> +
>> +static int bd96801_i2c_probe(struct i2c_client *i2c)
>> +{
>> +	int i, ret, intb_irq, errb_irq, num_regu_irqs, num_intb, num_errb = 0;
>> +	struct regmap_irq_chip_data *intb_irq_data, *errb_irq_data;
>> +	struct irq_domain *intb_domain, *errb_domain;
>> +	const struct fwnode_handle *fwnode;
>> +	struct resource *regulator_res;
>> +	struct regmap *regmap;
>> +
>> +	fwnode = dev_fwnode(&i2c->dev);
>> +	if (!fwnode) {
>> +		dev_err(&i2c->dev, "no fwnode\n");
>> +		return -EINVAL;
> 
> Why not dev_err_probe() here for uniformity?

I can change it to dev_err_probe() if it's strongly preferred. It just 
feels silly to use dev_err_probe() when the return value is hardcoded. 
Intentionally writing code like

err = -EINVAL;
if (err == ...)

just makes me feel a bit sick.

>> +	}
>> +
>> +	intb_irq = fwnode_irq_get_byname(fwnode, "intb");
>> +	if (intb_irq < 0)
>> +		return dev_err_probe(&i2c->dev, intb_irq,
>> +				     "No INTB IRQ configured\n");
> 
> This function would look nicer if you expanded to 100-chars.

The reason why I still prefer the good old 80-chars for files I work 
with, is that I am often having 3 terminal windows parallel on my laptop 
screen. (Or, when I have my wide mofnitor connected it is 3 editor 
windows + minicom). I need to keep the terminals small enough. 
Besides... I hate to admit this, but the time is finally taking it's 
toll. My eyes aren't quite the same they were 2 years ago...

So, same old story, I can change this if it is important enough for 
others, but personally I rather work with the short lines.

...

>> diff --git a/include/linux/mfd/rohm-bd96801.h b/include/linux/mfd/rohm-bd96801.h
>> new file mode 100644
>> index 000000000000..47b07171dcb2
>> --- /dev/null
>> +++ b/include/linux/mfd/rohm-bd96801.h
>> @@ -0,0 +1,212 @@
>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>> +/* Copyright (C) 2020 ROHM Semiconductors */
>> +

...

>> +/* IRQ register area */
>> +#define BD96801_REG_INT_MAIN		0x51
>> +
>> +/*
>> + * The BD96801 has two physical IRQ lines, INTB and ERRB.
>> + * For now we just handle the INTB.

Note to self, this comment is no longer true.

Thanks for the review!

Yours,
	-- Matti


-- 
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~

Re: [RFC PATCH 3/6] mfd: support ROHM BD96801 PMIC core

[Matti Vaittinen]

On 4/12/24 08:40, Matti Vaittinen wrote:
> Hi deee Ho Lee!
> 
> Thanks a ton for taking a look at this :) I already sent the V2 
> yesterday, briefly before receiving your comments.

It's not completely unusual I don't know what I am doing. BUT, it's not 
that common I don't know what I did... Seems like the V2 is still in my 
outbox, so I will add the fixes to your comments prior sending it! :)

Sorry for the added confusion!

Yours,
	-- Matti

-- 
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~

Re: [RFC PATCH 3/6] mfd: support ROHM BD96801 PMIC core

[Lee Jones]

On Fri, 12 Apr 2024, Matti Vaittinen wrote:

> Hi deee Ho Lee!
> 
> Thanks a ton for taking a look at this :) I already sent the V2 yesterday,
> briefly before receiving your comments. I think all of the comments are
> relevant for the V2 as well, I will fix them for the V3 when I get to that.
> If you find the time to take a look at V2, then the major things are
> addition of a watchdog IRQ + a work-around for the debugFS name collision
> for IRQ domains.
> 
> On 4/11/24 17:38, Lee Jones wrote:
> > On Tue, 02 Apr 2024, Matti Vaittinen wrote:
> > 
> > > The ROHM BD96801 PMIC is highly customizable automotive grade PMIC
> > > which integrates regulator and watchdog funtionalities.
> > > 
> > > Provide IRQ and register accesses for regulator/watchdog drivers.
> > > 
> > > Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
> > > ---
> > >   drivers/mfd/Kconfig              |  13 +
> > >   drivers/mfd/Makefile             |   1 +
> > >   drivers/mfd/rohm-bd96801.c       | 454 +++++++++++++++++++++++++++++++
> > >   include/linux/mfd/rohm-bd96801.h | 212 +++++++++++++++
> > >   include/linux/mfd/rohm-generic.h |   1 +
> > >   5 files changed, 681 insertions(+)
> > >   create mode 100644 drivers/mfd/rohm-bd96801.c
> > >   create mode 100644 include/linux/mfd/rohm-bd96801.h
> > > 
> > > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> > > index 4b023ee229cf..947045eb3a8e 100644
> > > --- a/drivers/mfd/Kconfig
> > > +++ b/drivers/mfd/Kconfig
> > > @@ -2089,6 +2089,19 @@ config MFD_ROHM_BD957XMUF
> > >   	  BD9573MUF Power Management ICs. BD9576 and BD9573 are primarily
> > >   	  designed to be used to power R-Car series processors.
> > > +config MFD_ROHM_BD96801
> > > +	tristate "ROHM BD96801 Power Management IC"
> > > +	depends on I2C=y
> > > +	depends on OF
> > > +	select REGMAP_I2C
> > > +	select REGMAP_IRQ
> > > +	select MFD_CORE
> > > +	help
> > > +	  Select this option to get support for the ROHM BD96801 Power
> > > +	  Management IC. The ROHM BD96801 is a highly scalable power management
> > 
> > Power Management
> 
> Out of the curiosity, why is the "Power Management IC" written with
> capitals, when speaking of a class of devices instead of a model? (I am 100%
> fine with the change, just curious).

It's no different to how its expressed in the tristate section above.

Power Management IC or PMIC.

  "provides power management capabilities" describes its function?

  "is a scalable Power Management IC", describes the device?

But actually, it just looks odd when both are used in the same section.

/me likes uniformity and consistency.

> > > +	  IC for industrial and automotive use. The BD96801 can be used as a
> > > +	  master PMIC in a chained PMIC solutions with suitable companion PMICs
> ...
> 
> > > +// SPDX-License-Identifier: GPL-2.0-only
> > > +//
> > > +// Copyright (C) 2022 ROHM Semiconductors
> > 
> > No updates for 2 years?
> 
> The year should be updated - thanks. But, now that you asked...  Almost no
> updates. The patches have rotten in my outbox, waiting for the permisson to
> be sent out... But yeah, I've sure added some changes before sending the
> series - I'll update the copyright :)
> 
> > > +
> > > +static int bd96801_i2c_probe(struct i2c_client *i2c)
> > > +{
> > > +	int i, ret, intb_irq, errb_irq, num_regu_irqs, num_intb, num_errb = 0;
> > > +	struct regmap_irq_chip_data *intb_irq_data, *errb_irq_data;
> > > +	struct irq_domain *intb_domain, *errb_domain;
> > > +	const struct fwnode_handle *fwnode;
> > > +	struct resource *regulator_res;
> > > +	struct regmap *regmap;
> > > +
> > > +	fwnode = dev_fwnode(&i2c->dev);
> > > +	if (!fwnode) {
> > > +		dev_err(&i2c->dev, "no fwnode\n");
> > > +		return -EINVAL;
> > 
> > Why not dev_err_probe() here for uniformity?
> 
> I can change it to dev_err_probe() if it's strongly preferred. It just feels
> silly to use dev_err_probe() when the return value is hardcoded.

Not at all:

git grep dev_err_probe | grep "\-[A-Z]"

> Intentionally writing code like
> 
> err = -EINVAL;
> if (err == ...)
> 
> just makes me feel a bit sick.

Why would you want to do that?

> > > +	}
> > > +
> > > +	intb_irq = fwnode_irq_get_byname(fwnode, "intb");
> > > +	if (intb_irq < 0)
> > > +		return dev_err_probe(&i2c->dev, intb_irq,
> > > +				     "No INTB IRQ configured\n");
> > 
> > This function would look nicer if you expanded to 100-chars.
> 
> The reason why I still prefer the good old 80-chars for files I work with,
> is that I am often having 3 terminal windows parallel on my laptop screen.
> (Or, when I have my wide mofnitor connected it is 3 editor windows +
> minicom). I need to keep the terminals small enough. Besides... I hate to
> admit this, but the time is finally taking it's toll. My eyes aren't quite
> the same they were 2 years ago...

Upgrade your 14" CRT monitor to something more modern. :)

I have a 32" 4k monitor with a good sized font and each of my 3
terminals (per i3 workspace) are ~150 chars wide.

> So, same old story, I can change this if it is important enough for others,
> but personally I rather work with the short lines.

It's not a showstopper.

-- 
Lee Jones [李琼斯]

Re: [RFC PATCH 3/6] mfd: support ROHM BD96801 PMIC core

[Matti Vaittinen]

On 4/12/24 10:23, Lee Jones wrote:
> On Fri, 12 Apr 2024, Matti Vaittinen wrote:
> 
>> Hi deee Ho Lee!
>>
>> Thanks a ton for taking a look at this :) I already sent the V2 yesterday,
>> briefly before receiving your comments. I think all of the comments are
>> relevant for the V2 as well, I will fix them for the V3 when I get to that.
>> If you find the time to take a look at V2, then the major things are
>> addition of a watchdog IRQ + a work-around for the debugFS name collision
>> for IRQ domains.
>>
>> On 4/11/24 17:38, Lee Jones wrote:
>>> On Tue, 02 Apr 2024, Matti Vaittinen wrote:
>>>
>>>> The ROHM BD96801 PMIC is highly customizable automotive grade PMIC
>>>> which integrates regulator and watchdog funtionalities.
>>>>
>>>> Provide IRQ and register accesses for regulator/watchdog drivers.
>>>>
>>>> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
>>>> ---
>>>>    drivers/mfd/Kconfig              |  13 +
>>>>    drivers/mfd/Makefile             |   1 +
>>>>    drivers/mfd/rohm-bd96801.c       | 454 +++++++++++++++++++++++++++++++
>>>>    include/linux/mfd/rohm-bd96801.h | 212 +++++++++++++++
>>>>    include/linux/mfd/rohm-generic.h |   1 +
>>>>    5 files changed, 681 insertions(+)
>>>>    create mode 100644 drivers/mfd/rohm-bd96801.c
>>>>    create mode 100644 include/linux/mfd/rohm-bd96801.h
>>>>
>>>> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
>>>> index 4b023ee229cf..947045eb3a8e 100644
>>>> --- a/drivers/mfd/Kconfig
>>>> +++ b/drivers/mfd/Kconfig
>>>> @@ -2089,6 +2089,19 @@ config MFD_ROHM_BD957XMUF
>>>>    	  BD9573MUF Power Management ICs. BD9576 and BD9573 are primarily
>>>>    	  designed to be used to power R-Car series processors.
>>>> +config MFD_ROHM_BD96801
>>>> +	tristate "ROHM BD96801 Power Management IC"
>>>> +	depends on I2C=y
>>>> +	depends on OF
>>>> +	select REGMAP_I2C
>>>> +	select REGMAP_IRQ
>>>> +	select MFD_CORE
>>>> +	help
>>>> +	  Select this option to get support for the ROHM BD96801 Power
>>>> +	  Management IC. The ROHM BD96801 is a highly scalable power management
>>>
>>> Power Management
>>
>> Out of the curiosity, why is the "Power Management IC" written with
>> capitals, when speaking of a class of devices instead of a model? (I am 100%
>> fine with the change, just curious).
> 
> It's no different to how its expressed in the tristate section above.
> 
> Power Management IC or PMIC.
> 
>    "provides power management capabilities" describes its function?
> 
>    "is a scalable Power Management IC", describes the device?
> 
> But actually, it just looks odd when both are used in the same section.
> 
> /me likes uniformity and consistency.

It's okay, thanks for the explanation :)

>>>> +	  IC for industrial and automotive use. The BD96801 can be used as a
>>>> +	  master PMIC in a chained PMIC solutions with suitable companion PMICs
>> ...
>>
>>>> +static int bd96801_i2c_probe(struct i2c_client *i2c)
>>>> +{
>>>> +	int i, ret, intb_irq, errb_irq, num_regu_irqs, num_intb, num_errb = 0;
>>>> +	struct regmap_irq_chip_data *intb_irq_data, *errb_irq_data;
>>>> +	struct irq_domain *intb_domain, *errb_domain;
>>>> +	const struct fwnode_handle *fwnode;
>>>> +	struct resource *regulator_res;
>>>> +	struct regmap *regmap;
>>>> +
>>>> +	fwnode = dev_fwnode(&i2c->dev);
>>>> +	if (!fwnode) {
>>>> +		dev_err(&i2c->dev, "no fwnode\n");
>>>> +		return -EINVAL;
>>>
>>> Why not dev_err_probe() here for uniformity?
>>
>> I can change it to dev_err_probe() if it's strongly preferred. It just feels
>> silly to use dev_err_probe() when the return value is hardcoded.
> 
> Not at all:
> 
> git grep dev_err_probe | grep "\-[A-Z]" 

Yes, I know people do use the dev_err_probe() with hardcoded errors but 
it does not make me feel any better about it :)

>> Intentionally writing code like
>>
>> err = -EINVAL;
>> if (err == ...)
>>
>> just makes me feel a bit sick.
> 
> Why would you want to do that?

This is what the dev_err_probe() with a hardcoded err does, right?

int dev_err_probe(const struct device *dev, int err, const char *fmt, ...)
{
	...
	if (err != -EPROBE_DEFER) {
		dev_err(dev, "error %pe: %pV", ERR_PTR(err), &vaf);
	} else {
		device_set_deferred_probe_reason(dev, &vaf);
		dev_dbg(dev, "error %pe: %pV", ERR_PTR(err), &vaf);
	}
	...
}

> 
>>>> +	}
>>>> +
>>>> +	intb_irq = fwnode_irq_get_byname(fwnode, "intb");
>>>> +	if (intb_irq < 0)
>>>> +		return dev_err_probe(&i2c->dev, intb_irq,
>>>> +				     "No INTB IRQ configured\n");
>>>
>>> This function would look nicer if you expanded to 100-chars.
>>
>> The reason why I still prefer the good old 80-chars for files I work with,
>> is that I am often having 3 terminal windows parallel on my laptop screen.
>> (Or, when I have my wide mofnitor connected it is 3 editor windows +
>> minicom). I need to keep the terminals small enough. Besides... I hate to
>> admit this, but the time is finally taking it's toll. My eyes aren't quite
>> the same they were 2 years ago...
> 
> Upgrade your 14" CRT monitor to something more modern. :)

But those things were built to last! And throwing away perfectly working 
stuff... :)

> 
> I have a 32" 4k monitor with a good sized font and each of my 3
> terminals (per i3 workspace) are ~150 chars wide.
> 
>> So, same old story, I can change this if it is important enough for others,
>> but personally I rather work with the short lines.
> 
> It's not a showstopper.

I'll revise the line lengths for next version. I think this one still 
won't go much over 80 chars, which may still fit on my terminals. I'll 
change it if it fits, keep it if it wont. Thanks for pointing it out :)

Yours,
	-- Matti

-- 
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~

Re: [RFC PATCH 3/6] mfd: support ROHM BD96801 PMIC core

[Lee Jones]

On Fri, 12 Apr 2024, Matti Vaittinen wrote:

> On 4/12/24 10:23, Lee Jones wrote:
> > On Fri, 12 Apr 2024, Matti Vaittinen wrote:
> > 
> > > Hi deee Ho Lee!
> > > 
> > > Thanks a ton for taking a look at this :) I already sent the V2 yesterday,
> > > briefly before receiving your comments. I think all of the comments are
> > > relevant for the V2 as well, I will fix them for the V3 when I get to that.
> > > If you find the time to take a look at V2, then the major things are
> > > addition of a watchdog IRQ + a work-around for the debugFS name collision
> > > for IRQ domains.
> > > 
> > > On 4/11/24 17:38, Lee Jones wrote:
> > > > On Tue, 02 Apr 2024, Matti Vaittinen wrote:
> > > > 
> > > > > The ROHM BD96801 PMIC is highly customizable automotive grade PMIC
> > > > > which integrates regulator and watchdog funtionalities.
> > > > > 
> > > > > Provide IRQ and register accesses for regulator/watchdog drivers.
> > > > > 
> > > > > Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
> > > > > ---
> > > > >    drivers/mfd/Kconfig              |  13 +
> > > > >    drivers/mfd/Makefile             |   1 +
> > > > >    drivers/mfd/rohm-bd96801.c       | 454 +++++++++++++++++++++++++++++++
> > > > >    include/linux/mfd/rohm-bd96801.h | 212 +++++++++++++++
> > > > >    include/linux/mfd/rohm-generic.h |   1 +
> > > > >    5 files changed, 681 insertions(+)
> > > > >    create mode 100644 drivers/mfd/rohm-bd96801.c
> > > > >    create mode 100644 include/linux/mfd/rohm-bd96801.h
> > > > > 
> > > > > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> > > > > index 4b023ee229cf..947045eb3a8e 100644
> > > > > --- a/drivers/mfd/Kconfig
> > > > > +++ b/drivers/mfd/Kconfig
> > > > > @@ -2089,6 +2089,19 @@ config MFD_ROHM_BD957XMUF
> > > > >    	  BD9573MUF Power Management ICs. BD9576 and BD9573 are primarily
> > > > >    	  designed to be used to power R-Car series processors.
> > > > > +config MFD_ROHM_BD96801
> > > > > +	tristate "ROHM BD96801 Power Management IC"
> > > > > +	depends on I2C=y
> > > > > +	depends on OF
> > > > > +	select REGMAP_I2C
> > > > > +	select REGMAP_IRQ
> > > > > +	select MFD_CORE
> > > > > +	help
> > > > > +	  Select this option to get support for the ROHM BD96801 Power
> > > > > +	  Management IC. The ROHM BD96801 is a highly scalable power management
> > > > 
> > > > Power Management
> > > 
> > > Out of the curiosity, why is the "Power Management IC" written with
> > > capitals, when speaking of a class of devices instead of a model? (I am 100%
> > > fine with the change, just curious).
> > 
> > It's no different to how its expressed in the tristate section above.
> > 
> > Power Management IC or PMIC.
> > 
> >    "provides power management capabilities" describes its function?
> > 
> >    "is a scalable Power Management IC", describes the device?
> > 
> > But actually, it just looks odd when both are used in the same section.
> > 
> > /me likes uniformity and consistency.
> 
> It's okay, thanks for the explanation :)
> 
> > > > > +	  IC for industrial and automotive use. The BD96801 can be used as a
> > > > > +	  master PMIC in a chained PMIC solutions with suitable companion PMICs
> > > ...
> > > 
> > > > > +static int bd96801_i2c_probe(struct i2c_client *i2c)
> > > > > +{
> > > > > +	int i, ret, intb_irq, errb_irq, num_regu_irqs, num_intb, num_errb = 0;
> > > > > +	struct regmap_irq_chip_data *intb_irq_data, *errb_irq_data;
> > > > > +	struct irq_domain *intb_domain, *errb_domain;
> > > > > +	const struct fwnode_handle *fwnode;
> > > > > +	struct resource *regulator_res;
> > > > > +	struct regmap *regmap;
> > > > > +
> > > > > +	fwnode = dev_fwnode(&i2c->dev);
> > > > > +	if (!fwnode) {
> > > > > +		dev_err(&i2c->dev, "no fwnode\n");
> > > > > +		return -EINVAL;
> > > > 
> > > > Why not dev_err_probe() here for uniformity?
> > > 
> > > I can change it to dev_err_probe() if it's strongly preferred. It just feels
> > > silly to use dev_err_probe() when the return value is hardcoded.
> > 
> > Not at all:
> > 
> > git grep dev_err_probe | grep "\-[A-Z]"
> 
> Yes, I know people do use the dev_err_probe() with hardcoded errors but it
> does not make me feel any better about it :)

<look into my swirling eyes> Uniformity within the function!

> > > Intentionally writing code like
> > > 
> > > err = -EINVAL;
> > > if (err == ...)
> > > 
> > > just makes me feel a bit sick.
> > 
> > Why would you want to do that?
> 
> This is what the dev_err_probe() with a hardcoded err does, right?
> 
> int dev_err_probe(const struct device *dev, int err, const char *fmt, ...)
> {
> 	...
> 	if (err != -EPROBE_DEFER) {
> 		dev_err(dev, "error %pe: %pV", ERR_PTR(err), &vaf);
> 	} else {
> 		device_set_deferred_probe_reason(dev, &vaf);
> 		dev_dbg(dev, "error %pe: %pV", ERR_PTR(err), &vaf);
> 	}
> 	...
> }

Attempt to purge this info from you brain!

> > > > > +	}
> > > > > +
> > > > > +	intb_irq = fwnode_irq_get_byname(fwnode, "intb");
> > > > > +	if (intb_irq < 0)
> > > > > +		return dev_err_probe(&i2c->dev, intb_irq,
> > > > > +				     "No INTB IRQ configured\n");
> > > > 
> > > > This function would look nicer if you expanded to 100-chars.
> > > 
> > > The reason why I still prefer the good old 80-chars for files I work with,
> > > is that I am often having 3 terminal windows parallel on my laptop screen.
> > > (Or, when I have my wide mofnitor connected it is 3 editor windows +
> > > minicom). I need to keep the terminals small enough. Besides... I hate to
> > > admit this, but the time is finally taking it's toll. My eyes aren't quite
> > > the same they were 2 years ago...
> > 
> > Upgrade your 14" CRT monitor to something more modern. :)
> 
> But those things were built to last! And throwing away perfectly working
> stuff... :)

Can't argue with that!  Maybe put 2 side-by-side or 4 in a matrix!

-- 
Lee Jones [李琼斯]

Re: [RFC PATCH 0/6] Support ROHM BD96801 scalable PMIC

[Matti Vaittinen]

Hi dee Ho peeps,

On 4/5/24 12:19, Matti Vaittinen wrote:
> On 4/4/24 16:15, Matti Vaittinen wrote:
>> Hi Mark,
>>
>> On 4/4/24 15:09, Mark Brown wrote:
>>> On Thu, Apr 04, 2024 at 10:26:34AM +0300, Matti Vaittinen wrote:
>>>
>>>> 1. Should we be able to have more than 1 IRQ domain / device?
>>>> 2. Should regmap_irq support having more than 1 HWIRQ
>>>
>>> I would expect each parent interrupt to show up as a separate remap_irq.

...
>>> So if we arrange to supply a name when we register multiple domains
>>> things should work fine?
> 
> After my latest findings, yes, I think so. How to do this correctly is 
> beyond me though. The __irq_domain_create() seems to me that the name is 
> meant to be the dt-node name when the controller is backed by a real 
> dt-node. Naming of the irq_domain_alloc_named_fwnode() sounds to me like 
> it is only intended to be used when there is no real fwnode. All 
> suggestions appreciated. Using the:
> irq_domain_update_bus_token(intb_domain, DOMAIN_BUS_WIRED);
> feels like a dirty hack, and won't scale if there is more HWIRQs.

I tried taking a look at this again.

If we wanted to support multiple HWIRQs / regmap-IRQ controller, it 
would require us to duplicate almost everything in the struct 
regmap_irq_chip for every new parent IRQ. The status/mask register 
information, IRQ type, etc. Naturally, it would require also duplicating 
lot of the data contained in the struct regmap_irq_chip_data. I am not 
sure if this could be done so the change is not reflected in the 
existing IRQ data initialization macros etc. Furthermore, some API 
changes would be required like changes to regmap_irq_get_domain().

I am a bit afraid this change, if implemented in regmap-IRQ, would be 
very intrusive and potentially impact large amount of callers. But more 
importantly, looking the amount of data that should be duplicated per 
new HWIRQ makes me think that an IRQ controller is really a product of a 
parent IRQ, not product of the device. Hence, assuming there is only one 
IRQ controller instance / device does not feel any more correct than 
assuming there is an IRQ controller instance / parent IRQ. Same thinking 
applies to IRQ domains.

Thus, forcing the regmap-IRQ to support multiple parents instead of 
having own regmap-IRQ instance / parent IRQ feels like fitting square 
item to a round hole. I am sure fixing all the bugs I caused would give 
donate a lot of EXP-points though :rolleyes:

Question is, should I still try?

Another option I see, is trying to think if irq-domain name could be 
changed. (This is what the RFC v3 does, [ab]using the 
irq_domain_update_bus_token()). I was a bit put off by the idea of 
'instantiating' multiple domains (or regmap-IRQ controllers) from a 
single node, but more I think of this, more I lean towards it. Besides, 
this is not something completely odd, I think MFD devices do this 
anyways. (Instantiate multiple [sub]devices from single DT-node). I 
would love to get an opinion from someone who knows the 'fundamentals' 
of the IRQ domains, and possibly some pointer for the right approach.

Finally we might also consider adding own sub-node in DT for each parent 
IRQ - but this feels very wrong to me.

All in all, I am having very hard time trying to think how to proceed. 
The last option for me is to skip support for the ERRB IRQ from the 
BD96801 driver, which would leave this problem to the next person 
working with a device providing multiple physical IRQs.

Yours,
	-- Matti

-- 
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~

Re: [RFC PATCH 0/6] Support ROHM BD96801 scalable PMIC

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 2517 bytes --]

On Mon, Apr 22, 2024 at 01:52:27PM +0300, Matti Vaittinen wrote:
> On 4/5/24 12:19, Matti Vaittinen wrote:
> > On 4/4/24 16:15, Matti Vaittinen wrote:

> > > > I would expect each parent interrupt to show up as a separate remap_irq.

> > > > So if we arrange to supply a name when we register multiple domains
> > > > things should work fine?

> > After my latest findings, yes, I think so. How to do this correctly is
> > beyond me though. The __irq_domain_create() seems to me that the name is
> > meant to be the dt-node name when the controller is backed by a real
> > dt-node. Naming of the irq_domain_alloc_named_fwnode() sounds to me like

...

> If we wanted to support multiple HWIRQs / regmap-IRQ controller, it would
> require us to duplicate almost everything in the struct regmap_irq_chip for
> every new parent IRQ. The status/mask register information, IRQ type, etc.
> Naturally, it would require also duplicating lot of the data contained in
> the struct regmap_irq_chip_data. I am not sure if this could be done so the
> change is not reflected in the existing IRQ data initialization macros etc.
> Furthermore, some API changes would be required like changes to
> regmap_irq_get_domain().

I don't understand what the difficulty is here - we're creating multiple
interrupt controllers so I'd expect to have to have full definitions of
each, and since everything is referenced by name from the root
regmap_irq_chip which gets registered it's just a case of supplying
different names and all the helpers should be fine?

> Thus, forcing the regmap-IRQ to support multiple parents instead of having
> own regmap-IRQ instance / parent IRQ feels like fitting square item to a
> round hole. I am sure fixing all the bugs I caused would give donate a lot
> of EXP-points though :rolleyes:

Right, my suggestion is to register multiple regmap_irq instrances - one
per parent - and supply a name that allows all the display/debugfs stuff
that currently uses the dev_name() to deduplicate.  You'd end up
sticking -primary, -secondary or whatever name was supplied onto the
names we currently use.

> Another option I see, is trying to think if irq-domain name could be
> changed. (This is what the RFC v3 does, [ab]using the
> irq_domain_update_bus_token()). I was a bit put off by the idea of
> 'instantiating' multiple domains (or regmap-IRQ controllers) from a single
> node, but more I think of this, more I lean towards it. Besides, this is not

Yes, register mutliple controllers with different names.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC PATCH 0/6] Support ROHM BD96801 scalable PMIC

[Matti Vaittinen]

On 5/9/24 08:08, Mark Brown wrote:
> On Mon, Apr 22, 2024 at 01:52:27PM +0300, Matti Vaittinen wrote:
>> On 4/5/24 12:19, Matti Vaittinen wrote:
>>> On 4/4/24 16:15, Matti Vaittinen wrote:
> 
>>>>> I would expect each parent interrupt to show up as a separate remap_irq.
> 
>>>>> So if we arrange to supply a name when we register multiple domains
>>>>> things should work fine?
> 
>>> After my latest findings, yes, I think so. How to do this correctly is
>>> beyond me though. The __irq_domain_create() seems to me that the name is
>>> meant to be the dt-node name when the controller is backed by a real
>>> dt-node. Naming of the irq_domain_alloc_named_fwnode() sounds to me like
> 
> ...
> 
>> If we wanted to support multiple HWIRQs / regmap-IRQ controller, it would
>> require us to duplicate almost everything in the struct regmap_irq_chip for
>> every new parent IRQ. The status/mask register information, IRQ type, etc.
>> Naturally, it would require also duplicating lot of the data contained in
>> the struct regmap_irq_chip_data. I am not sure if this could be done so the
>> change is not reflected in the existing IRQ data initialization macros etc.
>> Furthermore, some API changes would be required like changes to
>> regmap_irq_get_domain().
> 
> I don't understand what the difficulty is here - we're creating multiple
> interrupt controllers so I'd expect to have to have full definitions of
> each, and since everything is referenced by name from the root
> regmap_irq_chip which gets registered it's just a case of supplying
> different names and all the helpers should be fine?
> 
>> Thus, forcing the regmap-IRQ to support multiple parents instead of having
>> own regmap-IRQ instance / parent IRQ feels like fitting square item to a
>> round hole. I am sure fixing all the bugs I caused would give donate a lot
>> of EXP-points though :rolleyes:
> 
> Right, my suggestion is to register multiple regmap_irq instrances - one
> per parent - and supply a name that allows all the display/debugfs stuff
> that currently uses the dev_name() to deduplicate.  You'd end up
> sticking -primary, -secondary or whatever name was supplied onto the
> names we currently use.
> 
>> Another option I see, is trying to think if irq-domain name could be
>> changed. (This is what the RFC v3 does, [ab]using the
>> irq_domain_update_bus_token()). I was a bit put off by the idea of
>> 'instantiating' multiple domains (or regmap-IRQ controllers) from a single
>> node, but more I think of this, more I lean towards it. Besides, this is not
> 
> Yes, register mutliple controllers with different names.

Thanks for the guidance Mark. The controller name is not a problem. 
Problem is that I don't see a (proper) way to supply a name for the IRQ 
domain which gets registered by regmap-IRQ. IRQ domain code picks the 
name for the domain by the device-tree node. Both of our IRQ controllers 
would be instantiated from same node => the IRQ domain will get same 
name => debugfs will conflict.

My "solution" was simply dropping the ERRB IRQ from the driver (for now 
at least). I did send that as a series without 'RFC' - but made a 
mistake and restarted the versioning from v1. I am currently working 
with 2 other PMICs, one of them does also provide similar setup of two 
IRQ lines. Thus, I think being able to provide a name (suffix?) for IRQ 
domain when registering it instead of just using the name of the DT node 
is something I should look into. It's just nice to know someone else 
thinks it is valid approach.

Thanks for the input!

Yours,
	-- Matti



-- 
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~

Re: [RFC PATCH 0/6] Support ROHM BD96801 scalable PMIC

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1241 bytes --]

On Thu, May 09, 2024 at 10:03:43AM +0300, Matti Vaittinen wrote:
> On 5/9/24 08:08, Mark Brown wrote:

> > > > > > So if we arrange to supply a name when we register multiple domains
> > > > > > things should work fine?

> Thanks for the guidance Mark. The controller name is not a problem. Problem
> is that I don't see a (proper) way to supply a name for the IRQ domain which
> gets registered by regmap-IRQ. IRQ domain code picks the name for the domain
> by the device-tree node. Both of our IRQ controllers would be instantiated
> from same node => the IRQ domain will get same name => debugfs will
> conflict.

That's why I'm suggesting to add something - just put a name field in
the struct.

> My "solution" was simply dropping the ERRB IRQ from the driver (for now at
> least). I did send that as a series without 'RFC' - but made a mistake and
> restarted the versioning from v1. I am currently working with 2 other PMICs,
> one of them does also provide similar setup of two IRQ lines. Thus, I think
> being able to provide a name (suffix?) for IRQ domain when registering it
> instead of just using the name of the DT node is something I should look
> into. It's just nice to know someone else thinks it is valid approach.

Yes.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]