[PATCH 0/2] ROHM BD96801 Support ERRB IRQ

[PATCH 0/2] ROHM BD96801 Support ERRB IRQ

[Matti Vaittinen]

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

Add ERRB IRQ support to ROHM BD96801 PMIC driver.

The BD96801 has two IRQ pins, ERRB and INTB. ERRB is used to notify
'fatal' events so handling it has not been mandatory. However, there
are use-cases where handling the ERRB interrupts may be beneficial.

---

Supporting more than one IRQ pin in one device, using the regmap-IRQ,
caused an IRQ domain naming collision in the debugFS when device-tree
was used.

In order to avoid the collision, support for adding a name suffix to
IRQ domain was merged in regmap tree at
commit c69bb91c47e8 ("regmap IRQ support for devices with multiple IRQs")
This is required for the MFD driver to compile. Hence this series is
based on the regmap/for-6.12.

Furthermore, there has been a few (trivial) fixes to the BD96801 driver
in the regulator tree, and thus merge conflicts are possible. I can
re-spin the regulator driver based on the regulator tree when there is
a branch with the required IRQ domain and regmap commits as well as the
MFD driver. The MFD driver with ERRB support is needed so that the
regulator probe won't break for the users with the "errb" in
device-tree.

The ERRB patches were originally part of this series:
https://lore.kernel.org/all/cover.1717486682.git.mazziesaccount@gmail.com/

Revision history in the patches refers to the old versions in that
series.

Matti Vaittinen (2):
  mfd: bd96801: Add ERRB IRQ
  regulator: bd96801: Add ERRB IRQ

 drivers/mfd/rohm-bd96801.c            | 276 ++++++++++++++++++++++----
 drivers/regulator/bd96801-regulator.c | 130 ++++++++++--
 2 files changed, 356 insertions(+), 50 deletions(-)

-- 
2.45.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 --]

[PATCH 1/2] mfd: bd96801: Add ERRB IRQ

[Matti Vaittinen]

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

The ROHM BD96801 "scalable PMIC" provides two physical IRQs. The ERRB
handling can in many cases be omitted because it is used to inform fatal
IRQs, which usually kill the power from the SOC.

There may however be use-cases where the SOC has a 'back-up' emergency
power source which allows some very short time of operation to try to
gracefully shut down sensitive hardware. Furthermore, it is possible the
processor controlling the PMIC is not powered by the PMIC. In such cases
handling the ERRB IRQs may be beneficial.

Add support for ERRB IRQs.

Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
---
Revision history:
New series (only ERRB addition)
v1:
	- use devm allocation for regulator_res
	- use goto skip_errb instead of an if (errb)
	- constify immutable structs

Old series (All BD96801 functionality + irqdomain and regmap changes)
v2 => v3:
	- No changes
v1 => v2:
	- New patch

mfd: constify structs
---
 drivers/mfd/rohm-bd96801.c | 276 ++++++++++++++++++++++++++++++++-----
 1 file changed, 241 insertions(+), 35 deletions(-)

diff --git a/drivers/mfd/rohm-bd96801.c b/drivers/mfd/rohm-bd96801.c
index 714f08ed544b..fff764ddbf2f 100644
--- a/drivers/mfd/rohm-bd96801.c
+++ b/drivers/mfd/rohm-bd96801.c
@@ -5,13 +5,9 @@
  * ROHM BD96801 PMIC driver
  *
  * This version of the "BD86801 scalable PMIC"'s driver supports only very
- * basic set of the PMIC features. Most notably, there is no support for
- * the ERRB interrupt and the configurations which should be done when the
- * PMIC is in STBY mode.
- *
- * Supporting the ERRB interrupt would require dropping the regmap-IRQ
- * usage or working around (or accepting a presense of) a naming conflict
- * in debugFS IRQs.
+ * basic set of the PMIC features.
+ * Most notably, there is no support for the configurations which should
+ * be done when the PMIC is in STBY mode.
  *
  * Being able to reliably do the configurations like changing the
  * regulator safety limits (like limits for the over/under -voltages, over
@@ -23,16 +19,14 @@
  * be the need to configure these safety limits. Hence it's not simple to
  * come up with a generic solution.
  *
- * Users who require the ERRB handling and STBY state configurations can
- * have a look at the original RFC:
+ * Users who require the STBY state configurations can  have a look at the
+ * original RFC:
  * https://lore.kernel.org/all/cover.1712920132.git.mazziesaccount@gmail.com/
- * which implements a workaround to debugFS naming conflict and some of
- * the safety limit configurations - but leaves the state change handling
- * and synchronization to be implemented.
+ * which implements some of the safety limit configurations - but leaves the
+ * state change handling and synchronization to be implemented.
  *
  * It would be great to hear (and receive a patch!) if you implement the
- * STBY configuration support or a proper fix to the debugFS naming
- * conflict in your downstream driver ;)
+ * STBY configuration support or a proper fix in your downstream driver ;)
  */
 
 #include <linux/i2c.h>
@@ -46,6 +40,64 @@
 #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"),
 
@@ -90,20 +142,14 @@ static const struct resource regulator_intb_irqs[] = {
 	DEFINE_RES_IRQ_NAMED(BD96801_LDO7_UVD_STAT, "bd96801-ldo7-undervolt"),
 };
 
-static const struct resource wdg_intb_irqs[] = {
-	DEFINE_RES_IRQ_NAMED(BD96801_WDT_ERR_STAT, "bd96801-wdg"),
+enum {
+	WDG_CELL = 0,
+	REGULATOR_CELL,
 };
 
 static struct mfd_cell bd96801_cells[] = {
-	{
-		.name = "bd96801-wdt",
-		.resources = wdg_intb_irqs,
-		.num_resources = ARRAY_SIZE(wdg_intb_irqs),
-	}, {
-		.name = "bd96801-regulator",
-		.resources = regulator_intb_irqs,
-		.num_resources = ARRAY_SIZE(regulator_intb_irqs),
-	},
+	[WDG_CELL] = { .name = "bd96801-wdt", },
+	[REGULATOR_CELL] = { .name = "bd96801-regulator", },
 };
 
 static const struct regmap_range bd96801_volatile_ranges[] = {
@@ -128,6 +174,91 @@ static const struct regmap_access_table volatile_regs = {
 	.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 const 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),
@@ -176,8 +307,25 @@ static const struct regmap_irq bd96801_intb_irqs[] = {
 	REGMAP_IRQ_REG(BD96801_LDO7_UVD_STAT, 7, BD96801_LDO_UVD_STAT_MASK),
 };
 
-static struct regmap_irq_chip bd96801_irq_chip_intb = {
+static const struct regmap_irq_chip bd96801_irq_chip_errb = {
+	.name = "bd96801-irq-errb",
+	.domain_suffix = "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 const struct regmap_irq_chip bd96801_irq_chip_intb = {
 	.name = "bd96801-irq-intb",
+	.domain_suffix = "intb",
 	.main_status = BD96801_REG_INT_MAIN,
 	.num_main_regs = 1,
 	.irqs = &bd96801_intb_irqs[0],
@@ -199,11 +347,14 @@ static const struct regmap_config bd96801_regmap_config = {
 
 static int bd96801_i2c_probe(struct i2c_client *i2c)
 {
-	struct regmap_irq_chip_data *intb_irq_data;
+	int i, ret, intb_irq, errb_irq, num_regu_irqs, num_intb, num_errb = 0;
+	int wdg_irq_no;
+	struct regmap_irq_chip_data *intb_irq_data, *errb_irq_data;
+	struct irq_domain *intb_domain, *errb_domain;
+	struct resource wdg_irq;
 	const struct fwnode_handle *fwnode;
-	struct irq_domain *intb_domain;
+	struct resource *regulator_res;
 	struct regmap *regmap;
-	int ret, intb_irq;
 
 	fwnode = dev_fwnode(&i2c->dev);
 	if (!fwnode)
@@ -213,6 +364,23 @@ static int bd96801_i2c_probe(struct i2c_client *i2c)
 	if (intb_irq < 0)
 		return dev_err_probe(&i2c->dev, intb_irq, "INTB IRQ not 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 = devm_kcalloc(&i2c->dev, 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))
 		return dev_err_probe(&i2c->dev, PTR_ERR(regmap),
@@ -226,16 +394,54 @@ static int bd96801_i2c_probe(struct i2c_client *i2c)
 				       IRQF_ONESHOT, 0, &bd96801_irq_chip_intb,
 				       &intb_irq_data);
 	if (ret)
-		return dev_err_probe(&i2c->dev, ret, "Failed to add INTB IRQ chip\n");
+		return dev_err_probe(&i2c->dev, ret, "Failed to add INTB irq_chip\n");
 
 	intb_domain = regmap_irq_get_domain(intb_irq_data);
 
-	ret = devm_mfd_add_devices(&i2c->dev, PLATFORM_DEVID_AUTO,
-				   bd96801_cells,
-				   ARRAY_SIZE(bd96801_cells), NULL, 0,
-				   intb_domain);
+	/*
+	 * 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.
+	 */
+	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);
+	}
+
+	wdg_irq_no = irq_create_mapping(intb_domain, BD96801_WDT_ERR_STAT);
+	wdg_irq = DEFINE_RES_IRQ_NAMED(wdg_irq_no, "bd96801-wdg");
+	bd96801_cells[WDG_CELL].resources = &wdg_irq;
+	bd96801_cells[WDG_CELL].num_resources = 1;
+
+	if (!num_errb)
+		goto skip_errb;
+
+	ret = devm_regmap_add_irq_chip(&i2c->dev, regmap, errb_irq, IRQF_ONESHOT,
+				       0, &bd96801_irq_chip_errb, &errb_irq_data);
+	if (ret)
+		return dev_err_probe(&i2c->dev, ret,
+				     "Failed to add ERRB (%d) irq_chip\n", errb_irq);
+
+	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);
+	}
+
+skip_errb:
+	bd96801_cells[REGULATOR_CELL].resources = regulator_res;
+	bd96801_cells[REGULATOR_CELL].num_resources = num_regu_irqs;
+
+	ret = devm_mfd_add_devices(&i2c->dev, PLATFORM_DEVID_AUTO, bd96801_cells,
+				   ARRAY_SIZE(bd96801_cells), NULL, 0, NULL);
 	if (ret)
-		dev_err(&i2c->dev, "Failed to create subdevices\n");
+		dev_err_probe(&i2c->dev, ret, "Failed to create subdevices\n");
 
 	return ret;
 }
-- 
2.45.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: [PATCH 1/2] mfd: bd96801: Add ERRB IRQ

[Lee Jones]

On Mon, 26 Aug 2024, Matti Vaittinen wrote:

> The ROHM BD96801 "scalable PMIC" provides two physical IRQs. The ERRB
> handling can in many cases be omitted because it is used to inform fatal
> IRQs, which usually kill the power from the SOC.
> 
> There may however be use-cases where the SOC has a 'back-up' emergency
> power source which allows some very short time of operation to try to
> gracefully shut down sensitive hardware. Furthermore, it is possible the
> processor controlling the PMIC is not powered by the PMIC. In such cases
> handling the ERRB IRQs may be beneficial.
> 
> Add support for ERRB IRQs.
> 
> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
> ---
> Revision history:
> New series (only ERRB addition)
> v1:
> 	- use devm allocation for regulator_res
> 	- use goto skip_errb instead of an if (errb)
> 	- constify immutable structs
> 
> Old series (All BD96801 functionality + irqdomain and regmap changes)
> v2 => v3:
> 	- No changes
> v1 => v2:
> 	- New patch
> 
> mfd: constify structs
> ---
>  drivers/mfd/rohm-bd96801.c | 276 ++++++++++++++++++++++++++++++++-----
>  1 file changed, 241 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/mfd/rohm-bd96801.c b/drivers/mfd/rohm-bd96801.c
> index 714f08ed544b..fff764ddbf2f 100644
> --- a/drivers/mfd/rohm-bd96801.c
> +++ b/drivers/mfd/rohm-bd96801.c
> @@ -5,13 +5,9 @@
>   * ROHM BD96801 PMIC driver
>   *
>   * This version of the "BD86801 scalable PMIC"'s driver supports only very
> - * basic set of the PMIC features. Most notably, there is no support for
> - * the ERRB interrupt and the configurations which should be done when the
> - * PMIC is in STBY mode.
> - *
> - * Supporting the ERRB interrupt would require dropping the regmap-IRQ
> - * usage or working around (or accepting a presense of) a naming conflict
> - * in debugFS IRQs.
> + * basic set of the PMIC features.
> + * Most notably, there is no support for the configurations which should
> + * be done when the PMIC is in STBY mode.
>   *
>   * Being able to reliably do the configurations like changing the
>   * regulator safety limits (like limits for the over/under -voltages, over
> @@ -23,16 +19,14 @@
>   * be the need to configure these safety limits. Hence it's not simple to
>   * come up with a generic solution.
>   *
> - * Users who require the ERRB handling and STBY state configurations can
> - * have a look at the original RFC:
> + * Users who require the STBY state configurations can  have a look at the
> + * original RFC:
>   * https://lore.kernel.org/all/cover.1712920132.git.mazziesaccount@gmail.com/
> - * which implements a workaround to debugFS naming conflict and some of
> - * the safety limit configurations - but leaves the state change handling
> - * and synchronization to be implemented.
> + * which implements some of the safety limit configurations - but leaves the
> + * state change handling and synchronization to be implemented.
>   *
>   * It would be great to hear (and receive a patch!) if you implement the
> - * STBY configuration support or a proper fix to the debugFS naming
> - * conflict in your downstream driver ;)
> + * STBY configuration support or a proper fix in your downstream driver ;)
>   */
>  
>  #include <linux/i2c.h>
> @@ -46,6 +40,64 @@
>  #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"),
>  
> @@ -90,20 +142,14 @@ static const struct resource regulator_intb_irqs[] = {
>  	DEFINE_RES_IRQ_NAMED(BD96801_LDO7_UVD_STAT, "bd96801-ldo7-undervolt"),
>  };
>  
> -static const struct resource wdg_intb_irqs[] = {
> -	DEFINE_RES_IRQ_NAMED(BD96801_WDT_ERR_STAT, "bd96801-wdg"),
> +enum {
> +	WDG_CELL = 0,
> +	REGULATOR_CELL,
>  };
>  
>  static struct mfd_cell bd96801_cells[] = {
> -	{
> -		.name = "bd96801-wdt",
> -		.resources = wdg_intb_irqs,
> -		.num_resources = ARRAY_SIZE(wdg_intb_irqs),
> -	}, {
> -		.name = "bd96801-regulator",
> -		.resources = regulator_intb_irqs,
> -		.num_resources = ARRAY_SIZE(regulator_intb_irqs),
> -	},
> +	[WDG_CELL] = { .name = "bd96801-wdt", },
> +	[REGULATOR_CELL] = { .name = "bd96801-regulator", },
>  };
>  
>  static const struct regmap_range bd96801_volatile_ranges[] = {
> @@ -128,6 +174,91 @@ static const struct regmap_access_table volatile_regs = {
>  	.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 const 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),
> @@ -176,8 +307,25 @@ static const struct regmap_irq bd96801_intb_irqs[] = {
>  	REGMAP_IRQ_REG(BD96801_LDO7_UVD_STAT, 7, BD96801_LDO_UVD_STAT_MASK),
>  };
>  
> -static struct regmap_irq_chip bd96801_irq_chip_intb = {
> +static const struct regmap_irq_chip bd96801_irq_chip_errb = {
> +	.name = "bd96801-irq-errb",
> +	.domain_suffix = "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 const struct regmap_irq_chip bd96801_irq_chip_intb = {
>  	.name = "bd96801-irq-intb",
> +	.domain_suffix = "intb",
>  	.main_status = BD96801_REG_INT_MAIN,
>  	.num_main_regs = 1,
>  	.irqs = &bd96801_intb_irqs[0],
> @@ -199,11 +347,14 @@ static const struct regmap_config bd96801_regmap_config = {
>  
>  static int bd96801_i2c_probe(struct i2c_client *i2c)
>  {
> -	struct regmap_irq_chip_data *intb_irq_data;
> +	int i, ret, intb_irq, errb_irq, num_regu_irqs, num_intb, num_errb = 0;
> +	int wdg_irq_no;

Nit: Not sure why the smaller data elements have been placed at the top.

They were better down where they were.

> +	struct regmap_irq_chip_data *intb_irq_data, *errb_irq_data;
> +	struct irq_domain *intb_domain, *errb_domain;
> +	struct resource wdg_irq;
>  	const struct fwnode_handle *fwnode;
> -	struct irq_domain *intb_domain;
> +	struct resource *regulator_res;
>  	struct regmap *regmap;
> -	int ret, intb_irq;
>  
>  	fwnode = dev_fwnode(&i2c->dev);
>  	if (!fwnode)
> @@ -213,6 +364,23 @@ static int bd96801_i2c_probe(struct i2c_client *i2c)
>  	if (intb_irq < 0)
>  		return dev_err_probe(&i2c->dev, intb_irq, "INTB IRQ not 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 = devm_kcalloc(&i2c->dev, 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))
>  		return dev_err_probe(&i2c->dev, PTR_ERR(regmap),
> @@ -226,16 +394,54 @@ static int bd96801_i2c_probe(struct i2c_client *i2c)
>  				       IRQF_ONESHOT, 0, &bd96801_irq_chip_intb,
>  				       &intb_irq_data);
>  	if (ret)
> -		return dev_err_probe(&i2c->dev, ret, "Failed to add INTB IRQ chip\n");
> +		return dev_err_probe(&i2c->dev, ret, "Failed to add INTB irq_chip\n");
>  
>  	intb_domain = regmap_irq_get_domain(intb_irq_data);
>  
> -	ret = devm_mfd_add_devices(&i2c->dev, PLATFORM_DEVID_AUTO,
> -				   bd96801_cells,
> -				   ARRAY_SIZE(bd96801_cells), NULL, 0,
> -				   intb_domain);
> +	/*
> +	 * 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.
> +	 */

Do one or more of the subdevices consume both domains?

If not, why not call devm_mfd_add_devices() twice?

> +	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);
> +	}
> +
> +	wdg_irq_no = irq_create_mapping(intb_domain, BD96801_WDT_ERR_STAT);
> +	wdg_irq = DEFINE_RES_IRQ_NAMED(wdg_irq_no, "bd96801-wdg");
> +	bd96801_cells[WDG_CELL].resources = &wdg_irq;
> +	bd96801_cells[WDG_CELL].num_resources = 1;
> +
> +	if (!num_errb)
> +		goto skip_errb;
> +
> +	ret = devm_regmap_add_irq_chip(&i2c->dev, regmap, errb_irq, IRQF_ONESHOT,
> +				       0, &bd96801_irq_chip_errb, &errb_irq_data);
> +	if (ret)
> +		return dev_err_probe(&i2c->dev, ret,
> +				     "Failed to add ERRB (%d) irq_chip\n", errb_irq);
> +
> +	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);
> +	}
> +
> +skip_errb:
> +	bd96801_cells[REGULATOR_CELL].resources = regulator_res;
> +	bd96801_cells[REGULATOR_CELL].num_resources = num_regu_irqs;
> +
> +	ret = devm_mfd_add_devices(&i2c->dev, PLATFORM_DEVID_AUTO, bd96801_cells,
> +				   ARRAY_SIZE(bd96801_cells), NULL, 0, NULL);
>  	if (ret)
> -		dev_err(&i2c->dev, "Failed to create subdevices\n");
> +		dev_err_probe(&i2c->dev, ret, "Failed to create subdevices\n");
>  
>  	return ret;
>  }
> -- 
> 2.45.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: [PATCH 1/2] mfd: bd96801: Add ERRB IRQ

[Matti Vaittinen]

On 8/30/24 10:28, Lee Jones wrote:
> On Mon, 26 Aug 2024, Matti Vaittinen wrote:
> 
>> The ROHM BD96801 "scalable PMIC" provides two physical IRQs. The ERRB
>> handling can in many cases be omitted because it is used to inform fatal
>> IRQs, which usually kill the power from the SOC.
>>
>> There may however be use-cases where the SOC has a 'back-up' emergency
>> power source which allows some very short time of operation to try to
>> gracefully shut down sensitive hardware. Furthermore, it is possible the
>> processor controlling the PMIC is not powered by the PMIC. In such cases
>> handling the ERRB IRQs may be beneficial.
>>
>> Add support for ERRB IRQs.

Thanks for the review Lee! :)

>>
>> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
>> ---
>> Revision history:
>> New series (only ERRB addition)
>> v1:
>> 	- use devm allocation for regulator_res
>> 	- use goto skip_errb instead of an if (errb)
>> 	- constify immutable structs
>>
>> Old series (All BD96801 functionality + irqdomain and regmap changes)
>> v2 => v3:
>> 	- No changes
>> v1 => v2:
>> 	- New patch
>>
>> mfd: constify structs
>> ---
>>   
>>   static int bd96801_i2c_probe(struct i2c_client *i2c)
>>   {
>> -	struct regmap_irq_chip_data *intb_irq_data;
>> +	int i, ret, intb_irq, errb_irq, num_regu_irqs, num_intb, num_errb = 0;
>> +	int wdg_irq_no;
> 
> Nit: Not sure why the smaller data elements have been placed at the top.

Because some people have told me it's easier for them to read the local 
variable declarations when the code is formatted to "reverse xmas-tree" 
-style. I suppose I've tried to follow that here.

> They were better down where they were.

My old personal preference has just been to have 'simple' integer types 
first, then structs, and the pointers last. I don't think having 
xmas-tree (reversed or not) plays a role in my code-reading ability...

I won't re-spin the series just for this, if this is just a 'nit'. I 
will try to remember the comment if I need to rebase / respin this later 
though :)

>> +	struct regmap_irq_chip_data *intb_irq_data, *errb_irq_data;
>> +	struct irq_domain *intb_domain, *errb_domain;
>> +	struct resource wdg_irq;
>>   	const struct fwnode_handle *fwnode;
>> -	struct irq_domain *intb_domain;
>> +	struct resource *regulator_res;
>>   	struct regmap *regmap;
>> -	int ret, intb_irq;
>>   
>>   	fwnode = dev_fwnode(&i2c->dev);
>>   	if (!fwnode)
>> @@ -213,6 +364,23 @@ static int bd96801_i2c_probe(struct i2c_client *i2c)
>>   	if (intb_irq < 0)
>>   		return dev_err_probe(&i2c->dev, intb_irq, "INTB IRQ not 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 = devm_kcalloc(&i2c->dev, 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))
>>   		return dev_err_probe(&i2c->dev, PTR_ERR(regmap),
>> @@ -226,16 +394,54 @@ static int bd96801_i2c_probe(struct i2c_client *i2c)
>>   				       IRQF_ONESHOT, 0, &bd96801_irq_chip_intb,
>>   				       &intb_irq_data);
>>   	if (ret)
>> -		return dev_err_probe(&i2c->dev, ret, "Failed to add INTB IRQ chip\n");
>> +		return dev_err_probe(&i2c->dev, ret, "Failed to add INTB irq_chip\n");
>>   
>>   	intb_domain = regmap_irq_get_domain(intb_irq_data);
>>   
>> -	ret = devm_mfd_add_devices(&i2c->dev, PLATFORM_DEVID_AUTO,
>> -				   bd96801_cells,
>> -				   ARRAY_SIZE(bd96801_cells), NULL, 0,
>> -				   intb_domain);
>> +	/*
>> +	 * 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.
>> +	 */
> 
> Do one or more of the subdevices consume both domains?

I believe the regulators consume both.

> If not, why not call devm_mfd_add_devices() twice?

Thanks for this suggestion :) It didn't occur to me I could do that. 
Well, here I need both domains for regulators so it probably wouldn't 
work - but maybe I will remember this is a viable option for future 
designs! Thanks!

> 
>> +	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);
>> +	}
>> +
>> +	wdg_irq_no = irq_create_mapping(intb_domain, BD96801_WDT_ERR_STAT);
>> +	wdg_irq = DEFINE_RES_IRQ_NAMED(wdg_irq_no, "bd96801-wdg");
>> +	bd96801_cells[WDG_CELL].resources = &wdg_irq;
>> +	bd96801_cells[WDG_CELL].num_resources = 1;
>> +
>> +	if (!num_errb)
>> +		goto skip_errb;
>> +
>> +	ret = devm_regmap_add_irq_chip(&i2c->dev, regmap, errb_irq, IRQF_ONESHOT,
>> +				       0, &bd96801_irq_chip_errb, &errb_irq_data);
>> +	if (ret)
>> +		return dev_err_probe(&i2c->dev, ret,
>> +				     "Failed to add ERRB (%d) irq_chip\n", errb_irq);
>> +
>> +	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);
>> +	}
>> +
>> +skip_errb:
>> +	bd96801_cells[REGULATOR_CELL].resources = regulator_res;
>> +	bd96801_cells[REGULATOR_CELL].num_resources = num_regu_irqs;
>> +
>> +	ret = devm_mfd_add_devices(&i2c->dev, PLATFORM_DEVID_AUTO, bd96801_cells,
>> +				   ARRAY_SIZE(bd96801_cells), NULL, 0, NULL);
>>   	if (ret)
>> -		dev_err(&i2c->dev, "Failed to create subdevices\n");
>> +		dev_err_probe(&i2c->dev, ret, "Failed to create subdevices\n");
>>   
>>   	return ret;
>>   }

Yours,
	-- Matti

Re: [PATCH 1/2] mfd: bd96801: Add ERRB IRQ

[Lee Jones]

On Fri, 30 Aug 2024, Matti Vaittinen wrote:

> On 8/30/24 10:28, Lee Jones wrote:
> > On Mon, 26 Aug 2024, Matti Vaittinen wrote:
> > 
> > > The ROHM BD96801 "scalable PMIC" provides two physical IRQs. The ERRB
> > > handling can in many cases be omitted because it is used to inform fatal
> > > IRQs, which usually kill the power from the SOC.
> > > 
> > > There may however be use-cases where the SOC has a 'back-up' emergency
> > > power source which allows some very short time of operation to try to
> > > gracefully shut down sensitive hardware. Furthermore, it is possible the
> > > processor controlling the PMIC is not powered by the PMIC. In such cases
> > > handling the ERRB IRQs may be beneficial.
> > > 
> > > Add support for ERRB IRQs.
> 
> Thanks for the review Lee! :)
> 
> > > 
> > > Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
> > > ---
> > > Revision history:
> > > New series (only ERRB addition)
> > > v1:
> > > 	- use devm allocation for regulator_res
> > > 	- use goto skip_errb instead of an if (errb)
> > > 	- constify immutable structs
> > > 
> > > Old series (All BD96801 functionality + irqdomain and regmap changes)
> > > v2 => v3:
> > > 	- No changes
> > > v1 => v2:
> > > 	- New patch
> > > 
> > > mfd: constify structs
> > > ---
> > >   static int bd96801_i2c_probe(struct i2c_client *i2c)
> > >   {
> > > -	struct regmap_irq_chip_data *intb_irq_data;
> > > +	int i, ret, intb_irq, errb_irq, num_regu_irqs, num_intb, num_errb = 0;
> > > +	int wdg_irq_no;
> > 
> > Nit: Not sure why the smaller data elements have been placed at the top.
> 
> Because some people have told me it's easier for them to read the local
> variable declarations when the code is formatted to "reverse xmas-tree"
> -style. I suppose I've tried to follow that here.
> 
> > They were better down where they were.
> 
> My old personal preference has just been to have 'simple' integer types
> first, then structs, and the pointers last. I don't think having xmas-tree
> (reversed or not) plays a role in my code-reading ability...
> 
> I won't re-spin the series just for this, if this is just a 'nit'. I will
> try to remember the comment if I need to rebase / respin this later though
> :)

Please leave them were they are.

> > > +	struct regmap_irq_chip_data *intb_irq_data, *errb_irq_data;
> > > +	struct irq_domain *intb_domain, *errb_domain;
> > > +	struct resource wdg_irq;
> > >   	const struct fwnode_handle *fwnode;
> > > -	struct irq_domain *intb_domain;
> > > +	struct resource *regulator_res;
> > >   	struct regmap *regmap;
> > > -	int ret, intb_irq;
> > >   	fwnode = dev_fwnode(&i2c->dev);
> > >   	if (!fwnode)
> > > @@ -213,6 +364,23 @@ static int bd96801_i2c_probe(struct i2c_client *i2c)
> > >   	if (intb_irq < 0)
> > >   		return dev_err_probe(&i2c->dev, intb_irq, "INTB IRQ not 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 = devm_kcalloc(&i2c->dev, 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))
> > >   		return dev_err_probe(&i2c->dev, PTR_ERR(regmap),
> > > @@ -226,16 +394,54 @@ static int bd96801_i2c_probe(struct i2c_client *i2c)
> > >   				       IRQF_ONESHOT, 0, &bd96801_irq_chip_intb,
> > >   				       &intb_irq_data);
> > >   	if (ret)
> > > -		return dev_err_probe(&i2c->dev, ret, "Failed to add INTB IRQ chip\n");
> > > +		return dev_err_probe(&i2c->dev, ret, "Failed to add INTB irq_chip\n");

Kernel logs are user facing.  The previous message was better.

Please drop this change.

> > >   	intb_domain = regmap_irq_get_domain(intb_irq_data);
> > > -	ret = devm_mfd_add_devices(&i2c->dev, PLATFORM_DEVID_AUTO,
> > > -				   bd96801_cells,
> > > -				   ARRAY_SIZE(bd96801_cells), NULL, 0,
> > > -				   intb_domain);
> > > +	/*
> > > +	 * 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.
> > > +	 */
> > 
> > Do one or more of the subdevices consume both domains?
> 
> I believe the regulators consume both.
> 
> > If not, why not call devm_mfd_add_devices() twice?
> 
> Thanks for this suggestion :) It didn't occur to me I could do that. Well,
> here I need both domains for regulators so it probably wouldn't work - but
> maybe I will remember this is a viable option for future designs! Thanks!

That's a shame.  This all looks quite messy.

-- 
Lee Jones [李琼斯]

Re: [PATCH 1/2] mfd: bd96801: Add ERRB IRQ

[Matti Vaittinen]

On 9/3/24 17:37, Lee Jones wrote:
> On Fri, 30 Aug 2024, Matti Vaittinen wrote:
> 
>> On 8/30/24 10:28, Lee Jones wrote:
>>> On Mon, 26 Aug 2024, Matti Vaittinen wrote:
>>>
>>>> The ROHM BD96801 "scalable PMIC" provides two physical IRQs. The ERRB
>>>> handling can in many cases be omitted because it is used to inform fatal
>>>> IRQs, which usually kill the power from the SOC.
>>>>
>>>> There may however be use-cases where the SOC has a 'back-up' emergency
>>>> power source which allows some very short time of operation to try to
>>>> gracefully shut down sensitive hardware. Furthermore, it is possible the
>>>> processor controlling the PMIC is not powered by the PMIC. In such cases
>>>> handling the ERRB IRQs may be beneficial.
>>>>
>>>> Add support for ERRB IRQs.
>>
>> Thanks for the review Lee! :)
>>
>>>>
>>>> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
>>>> ---
>>>> Revision history:
>>>> New series (only ERRB addition)
>>>> v1:
>>>> 	- use devm allocation for regulator_res
>>>> 	- use goto skip_errb instead of an if (errb)
>>>> 	- constify immutable structs
>>>>
>>>> Old series (All BD96801 functionality + irqdomain and regmap changes)
>>>> v2 => v3:
>>>> 	- No changes
>>>> v1 => v2:
>>>> 	- New patch
>>>>
>>>> mfd: constify structs
>>>> ---
>>>>    static int bd96801_i2c_probe(struct i2c_client *i2c)
>>>>    {
>>>> -	struct regmap_irq_chip_data *intb_irq_data;
>>>> +	int i, ret, intb_irq, errb_irq, num_regu_irqs, num_intb, num_errb = 0;
>>>> +	int wdg_irq_no;
>>>
>>> Nit: Not sure why the smaller data elements have been placed at the top.
>>
>> Because some people have told me it's easier for them to read the local
>> variable declarations when the code is formatted to "reverse xmas-tree"
>> -style. I suppose I've tried to follow that here.
>>
>>> They were better down where they were.
>>
>> My old personal preference has just been to have 'simple' integer types
>> first, then structs, and the pointers last. I don't think having xmas-tree
>> (reversed or not) plays a role in my code-reading ability...
>>
>> I won't re-spin the series just for this, if this is just a 'nit'. I will
>> try to remember the comment if I need to rebase / respin this later though
>> :)
> 
> Please leave them were they are.

I suppose this is a request to leave them where they are in-tree code 
now - not to leave them where they are in this patch :) I'll re-spin 
this anyways for the print below. So, I might as well revert the local 
variable formatting here :)

>>>> +	struct regmap_irq_chip_data *intb_irq_data, *errb_irq_data;
>>>> +	struct irq_domain *intb_domain, *errb_domain;
>>>> +	struct resource wdg_irq;
>>>>    	const struct fwnode_handle *fwnode;
>>>> -	struct irq_domain *intb_domain;
>>>> +	struct resource *regulator_res;
>>>>    	struct regmap *regmap;
>>>> -	int ret, intb_irq;
>>>>    	fwnode = dev_fwnode(&i2c->dev);
>>>>    	if (!fwnode)
>>>> @@ -213,6 +364,23 @@ static int bd96801_i2c_probe(struct i2c_client *i2c)
>>>>    	if (intb_irq < 0)
>>>>    		return dev_err_probe(&i2c->dev, intb_irq, "INTB IRQ not 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 = devm_kcalloc(&i2c->dev, 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))
>>>>    		return dev_err_probe(&i2c->dev, PTR_ERR(regmap),
>>>> @@ -226,16 +394,54 @@ static int bd96801_i2c_probe(struct i2c_client *i2c)
>>>>    				       IRQF_ONESHOT, 0, &bd96801_irq_chip_intb,
>>>>    				       &intb_irq_data);
>>>>    	if (ret)
>>>> -		return dev_err_probe(&i2c->dev, ret, "Failed to add INTB IRQ chip\n");
>>>> +		return dev_err_probe(&i2c->dev, ret, "Failed to add INTB irq_chip\n");
> 
> Kernel logs are user facing.  The previous message was better.
> 
> Please drop this change.

Good catch. This has been an unintended change. Not sure where this 
originated from.


Yours,
	-- Matti

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

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

[PATCH 2/2] regulator: bd96801: Add ERRB IRQ

[Matti Vaittinen]

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

The ROHM BD96801 "scalable PMIC" provides two physical IRQs. The ERRB
handling can in many cases be omitted because it is used to inform fatal
IRQs, which usually kill the power from the SOC.

There may however be use-cases where the SOC has a 'back-up' emergency
power source which allows some very short time of operation to try to
gracefully shut down sensitive hardware. Furthermore, it is possible the
processor controlling the PMIC is not powered by the PMIC. In such cases
handling the ERRB IRQs may be beneficial.

Add support for ERRB IRQs.

Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
Reviewed-by: Mark Brown <broonie@kernel.org>

---
Revision history:
New series (only ERRB addition)
v1:
	- No changes

Old series (All BD96801 functionality + irqdomain and regmap changes)
v2 =>:
	- No changes
v1 => v2:
        - New patch
---
 drivers/regulator/bd96801-regulator.c | 130 +++++++++++++++++++++++---
 1 file changed, 115 insertions(+), 15 deletions(-)

diff --git a/drivers/regulator/bd96801-regulator.c b/drivers/regulator/bd96801-regulator.c
index 46ca81f18703..9443e386eaed 100644
--- a/drivers/regulator/bd96801-regulator.c
+++ b/drivers/regulator/bd96801-regulator.c
@@ -5,12 +5,7 @@
 /*
  * This version of the "BD86801 scalable PMIC"'s driver supports only very
  * basic set of the PMIC features. Most notably, there is no support for
- * the ERRB interrupt and the configurations which should be done when the
- * PMIC is in STBY mode.
- *
- * Supporting the ERRB interrupt would require dropping the regmap-IRQ
- * usage or working around (or accepting a presense of) a naming conflict
- * in debugFS IRQs.
+ * the configurations which should be done when the PMIC is in STBY mode.
  *
  * Being able to reliably do the configurations like changing the
  * regulator safety limits (like limits for the over/under -voltages, over
@@ -22,16 +17,14 @@
  * be the need to configure these safety limits. Hence it's not simple to
  * come up with a generic solution.
  *
- * Users who require the ERRB handling and STBY state configurations can
- * have a look at the original RFC:
+ * Users who require the STBY state configurations can have a look at the
+ * original RFC:
  * https://lore.kernel.org/all/cover.1712920132.git.mazziesaccount@gmail.com/
- * which implements a workaround to debugFS naming conflict and some of
- * the safety limit configurations - but leaves the state change handling
- * and synchronization to be implemented.
+ * which implements some of the safety limit configurations - but leaves the
+ * state change handling and synchronization to be implemented.
  *
  * It would be great to hear (and receive a patch!) if you implement the
- * STBY configuration support or a proper fix to the debugFS naming
- * conflict in your downstream driver ;)
+ * STBY configuration support in your downstream driver ;)
  */
 
 #include <linux/delay.h>
@@ -733,6 +726,95 @@ static int initialize_pmic_data(struct device *dev,
 	return 0;
 }
 
+static int bd96801_map_event_all(int irq, struct regulator_irq_data *rid,
+			  unsigned long *dev_mask)
+{
+	int i;
+
+	for (i = 0; i < rid->num_states; i++) {
+		rid->states[i].notifs = REGULATOR_EVENT_FAIL;
+		rid->states[i].errors = REGULATOR_ERROR_FAIL;
+		*dev_mask |= BIT(i);
+	}
+
+	return 0;
+}
+
+static int bd96801_rdev_errb_irqs(struct platform_device *pdev,
+				  struct regulator_dev *rdev)
+{
+	int i;
+	void *retp;
+	static const char * const single_out_errb_irqs[] = {
+		"bd96801-%s-pvin-err", "bd96801-%s-ovp-err",
+		"bd96801-%s-uvp-err", "bd96801-%s-shdn-err",
+	};
+
+	for (i = 0; i < ARRAY_SIZE(single_out_errb_irqs); i++) {
+		struct regulator_irq_desc id = {
+			.map_event = bd96801_map_event_all,
+			.irq_off_ms = 1000,
+		};
+		struct regulator_dev *rdev_arr[1];
+		char tmp[255];
+		int irq;
+
+		snprintf(tmp, 255, single_out_errb_irqs[i], rdev->desc->name);
+		tmp[254] = 0;
+		id.name = tmp;
+
+		irq = platform_get_irq_byname(pdev, tmp);
+		if (irq < 0)
+			continue;
+
+		rdev_arr[0] = rdev;
+		retp = devm_regulator_irq_helper(&pdev->dev, &id, irq, 0,
+						 REGULATOR_ERROR_FAIL, NULL,
+						 rdev_arr, 1);
+		if (IS_ERR(retp))
+			return PTR_ERR(retp);
+
+	}
+	return 0;
+}
+
+static int bd96801_global_errb_irqs(struct platform_device *pdev,
+				    struct regulator_dev **rdev, int num_rdev)
+{
+	int i, num_irqs;
+	void *retp;
+	static const char * const global_errb_irqs[] = {
+		"bd96801-otp-err", "bd96801-dbist-err", "bd96801-eep-err",
+		"bd96801-abist-err", "bd96801-prstb-err", "bd96801-drmoserr1",
+		"bd96801-drmoserr2", "bd96801-slave-err", "bd96801-vref-err",
+		"bd96801-tsd", "bd96801-uvlo-err", "bd96801-ovlo-err",
+		"bd96801-osc-err", "bd96801-pon-err", "bd96801-poff-err",
+		"bd96801-cmd-shdn-err", "bd96801-int-shdn-err"
+	};
+
+	num_irqs = ARRAY_SIZE(global_errb_irqs);
+	for (i = 0; i < num_irqs; i++) {
+		int irq;
+		struct regulator_irq_desc id = {
+			.name = global_errb_irqs[i],
+			.map_event = bd96801_map_event_all,
+			.irq_off_ms = 1000,
+		};
+
+		irq = platform_get_irq_byname(pdev, global_errb_irqs[i]);
+		if (irq < 0)
+			continue;
+
+		retp = devm_regulator_irq_helper(&pdev->dev, &id, irq, 0,
+						 REGULATOR_ERROR_FAIL, NULL,
+						  rdev, num_rdev);
+		if (IS_ERR(retp))
+			return PTR_ERR(retp);
+	}
+
+	return 0;
+}
+
 static int bd96801_rdev_intb_irqs(struct platform_device *pdev,
 				  struct bd96801_pmic_data *pdata,
 				  struct bd96801_irqinfo *iinfo,
@@ -788,11 +870,10 @@ static int bd96801_rdev_intb_irqs(struct platform_device *pdev,
 	return 0;
 }
 
-
-
 static int bd96801_probe(struct platform_device *pdev)
 {
 	struct regulator_dev *ldo_errs_rdev_arr[BD96801_NUM_LDOS];
+	struct regulator_dev *all_rdevs[BD96801_NUM_REGULATORS];
 	struct bd96801_regulator_data *rdesc;
 	struct regulator_config config = {};
 	int ldo_errs_arr[BD96801_NUM_LDOS];
@@ -800,6 +881,7 @@ static int bd96801_probe(struct platform_device *pdev)
 	int temp_notif_ldos = 0;
 	struct device *parent;
 	int i, ret;
+	bool use_errb;
 	void *retp;
 
 	parent = pdev->dev.parent;
@@ -824,6 +906,13 @@ static int bd96801_probe(struct platform_device *pdev)
 	config.regmap = pdata->regmap;
 	config.dev = parent;
 
+	ret = of_property_match_string(pdev->dev.parent->of_node,
+				       "interrupt-names", "errb");
+	if (ret < 0)
+		use_errb = false;
+	else
+		use_errb = true;
+
 	ret = bd96801_walk_regulator_dt(&pdev->dev, pdata->regmap, rdesc,
 					BD96801_NUM_REGULATORS);
 	if (ret)
@@ -842,6 +931,7 @@ static int bd96801_probe(struct platform_device *pdev)
 				rdesc[i].desc.name);
 			return PTR_ERR(rdev);
 		}
+		all_rdevs[i] = rdev;
 		/*
 		 * LDOs don't have own temperature monitoring. If temperature
 		 * notification was requested for this LDO from DT then we will
@@ -863,6 +953,12 @@ static int bd96801_probe(struct platform_device *pdev)
 			if (ret)
 				return ret;
 		}
+		/* Register per regulator ERRB notifiers */
+		if (use_errb) {
+			ret = bd96801_rdev_errb_irqs(pdev, rdev);
+			if (ret)
+				return ret;
+		}
 	}
 	if (temp_notif_ldos) {
 		int irq;
@@ -884,6 +980,10 @@ static int bd96801_probe(struct platform_device *pdev)
 			return PTR_ERR(retp);
 	}
 
+	if (use_errb)
+		return bd96801_global_errb_irqs(pdev, all_rdevs,
+						ARRAY_SIZE(all_rdevs));
+
 	return 0;
 }
 
-- 
2.45.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 --]