| Message ID | 332a2d2429e2ba3c96afd28c1ccc18efc38e1fd3.1717486682.git.mazziesaccount@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | Support ROHM BD96801 Scalable PMIC | expand |
On Tue, 04 Jun 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: > v2 =>: > - No changes > v1 => v2: > - New patch > --- > drivers/mfd/rohm-bd96801.c | 291 ++++++++++++++++++++++++++++++++----- > 1 file changed, 253 insertions(+), 38 deletions(-) > > diff --git a/drivers/mfd/rohm-bd96801.c b/drivers/mfd/rohm-bd96801.c > index 1c2a9591be7b..b7f073318873 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. Why bother adding all that blurb in the first place? > + * 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> > @@ -45,6 +39,65 @@ > > #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"), > > @@ -89,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_mfd_cells[] = { > - { > - .name = "bd96801-wdt", > - .resources = wdg_intb_irqs, > - .num_resources = ARRAY_SIZE(wdg_intb_irqs), > - }, { > - .name = "bd96801-pmic", > - .resources = regulator_intb_irqs, > - .num_resources = ARRAY_SIZE(regulator_intb_irqs), > - }, > + [WDG_CELL] = { .name = "bd96801-wdt", }, > + [REGULATOR_CELL] = { .name = "bd96801-pmic", }, > }; > > static const struct regmap_range bd96801_volatile_ranges[] = { > @@ -127,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 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), > @@ -175,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_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 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], > @@ -198,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) > @@ -212,10 +364,28 @@ 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 = kcalloc(num_regu_irqs, sizeof(*regulator_res), GFP_KERNEL); Why not devm_* and omit the kfree()? > + 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), > + if (IS_ERR(regmap)) { > + ret = dev_err_probe(&i2c->dev, PTR_ERR(regmap), > "Regmap initialization failed\n"); > + goto free_out; > + } > > ret = regmap_write(regmap, BD96801_LOCK_REG, BD96801_UNLOCK); > if (ret) > @@ -224,18 +394,63 @@ static int bd96801_i2c_probe(struct i2c_client *i2c) > ret = devm_regmap_add_irq_chip(&i2c->dev, regmap, intb_irq, > 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"); > + if (ret) { > + dev_err_probe(&i2c->dev, ret, "Failed to add INTB irq_chip\n"); > + goto free_out; > + } > > intb_domain = regmap_irq_get_domain(intb_irq_data); > > - ret = devm_mfd_add_devices(&i2c->dev, PLATFORM_DEVID_AUTO, > - bd96801_mfd_cells, > - ARRAY_SIZE(bd96801_mfd_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 = ®ulator_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_mfd_cells[WDG_CELL].resources = &wdg_irq; > + bd96801_mfd_cells[WDG_CELL].num_resources = 1; > + > + if (num_errb) { 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) { > + 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 = ®ulator_res[num_intb + i]; > + > + *res = regulator_errb_irqs[i]; > + res->start = res->end = irq_create_mapping(errb_domain, > + res->start); > + } > + } skip_errb: > + 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(&i2c->dev, "Failed to create subdevices\n"); > + dev_err_probe(&i2c->dev, ret, "Failed to create subdevices\n"); > + > +free_out: > + kfree(regulator_res); > > return ret; > } > -- > 2.45.1 > > > -- > 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 =]
On 6/13/24 19:32, Lee Jones wrote: > On Tue, 04 Jun 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: >> v2 =>: >> - No changes >> v1 => v2: >> - New patch >> --- >> drivers/mfd/rohm-bd96801.c | 291 ++++++++++++++++++++++++++++++++----- >> 1 file changed, 253 insertions(+), 38 deletions(-) >> >> diff --git a/drivers/mfd/rohm-bd96801.c b/drivers/mfd/rohm-bd96801.c >> index 1c2a9591be7b..b7f073318873 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. > > Why bother adding all that blurb in the first place? Because, I assume there are users who would like to have the ERRB in use. The main purpose of this comment is that any such users could a) see this version does not support ERRB. b) can find the original RFC with ERRB supportn and a workaround. c) know why this version does not work with ERRB and thus fix this It seems this ERRB support may be missing from upstream for a while, hence I think having this note is worthy until (if) this ERRB patch lands in upstream. > >> + * 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 ;) >> */ ... >> 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) >> @@ -212,10 +364,28 @@ 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 = kcalloc(num_regu_irqs, sizeof(*regulator_res), GFP_KERNEL); > > Why not devm_* and omit the kfree()? I used kcalloc() because this memory is only temporarily needed. It is not needed after devm_mfd_add_devices() returns. Sure the devm_* would simplify the error paths... Thanks! > >> + 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), >> + if (IS_ERR(regmap)) { >> + ret = dev_err_probe(&i2c->dev, PTR_ERR(regmap), >> "Regmap initialization failed\n"); >> + goto free_out; >> + } >> >> ret = regmap_write(regmap, BD96801_LOCK_REG, BD96801_UNLOCK); >> if (ret) >> @@ -224,18 +394,63 @@ static int bd96801_i2c_probe(struct i2c_client *i2c) >> ret = devm_regmap_add_irq_chip(&i2c->dev, regmap, intb_irq, >> 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"); >> + if (ret) { >> + dev_err_probe(&i2c->dev, ret, "Failed to add INTB irq_chip\n"); >> + goto free_out; >> + } >> >> intb_domain = regmap_irq_get_domain(intb_irq_data); >> >> - ret = devm_mfd_add_devices(&i2c->dev, PLATFORM_DEVID_AUTO, >> - bd96801_mfd_cells, >> - ARRAY_SIZE(bd96801_mfd_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 = ®ulator_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_mfd_cells[WDG_CELL].resources = &wdg_irq; >> + bd96801_mfd_cells[WDG_CELL].num_resources = 1; >> + >> + if (num_errb) { > > if (!num_errb) > goto skip_errb; Ok, can do. > >> + 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 = ®ulator_res[num_intb + i]; >> + >> + *res = regulator_errb_irqs[i]; >> + res->start = res->end = irq_create_mapping(errb_domain, >> + res->start); >> + } >> + } > > skip_errb: ... Thanks for comments Lee. Reworking this will have to wait for the irqdomain name suffix, which I will continue after Hervé has done his part of the irqdomain changes. I will omit this patch from the next re-spin of the series. Yours, -- Matti
On Fri, 14 Jun 2024, Matti Vaittinen wrote: > On 6/13/24 19:32, Lee Jones wrote: > > On Tue, 04 Jun 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: > > > v2 =>: > > > - No changes > > > v1 => v2: > > > - New patch > > > --- > > > drivers/mfd/rohm-bd96801.c | 291 ++++++++++++++++++++++++++++++++----- > > > 1 file changed, 253 insertions(+), 38 deletions(-) > > > > > > diff --git a/drivers/mfd/rohm-bd96801.c b/drivers/mfd/rohm-bd96801.c > > > index 1c2a9591be7b..b7f073318873 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. > > > > Why bother adding all that blurb in the first place? > > Because, I assume there are users who would like to have the ERRB in use. > The main purpose of this comment is that any such users could > a) see this version does not support ERRB. > b) can find the original RFC with ERRB supportn and a workaround. > c) know why this version does not work with ERRB and thus fix this > > It seems this ERRB support may be missing from upstream for a while, hence I > think having this note is worthy until (if) this ERRB patch lands in > upstream. What I mean is - you're adding all of these extra lines in patch 3 and removing them in patch 9. > > > + * 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 ;) > > > */ > > ... > > > > 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) > > > @@ -212,10 +364,28 @@ 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 = kcalloc(num_regu_irqs, sizeof(*regulator_res), GFP_KERNEL); > > > > Why not devm_* and omit the kfree()? > > I used kcalloc() because this memory is only temporarily needed. It is not > needed after devm_mfd_add_devices() returns. > > Sure the devm_* would simplify the error paths... Thanks! > > > > > > + 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), > > > + if (IS_ERR(regmap)) { > > > + ret = dev_err_probe(&i2c->dev, PTR_ERR(regmap), > > > "Regmap initialization failed\n"); > > > + goto free_out; > > > + } > > > ret = regmap_write(regmap, BD96801_LOCK_REG, BD96801_UNLOCK); > > > if (ret) > > > @@ -224,18 +394,63 @@ static int bd96801_i2c_probe(struct i2c_client *i2c) > > > ret = devm_regmap_add_irq_chip(&i2c->dev, regmap, intb_irq, > > > 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"); > > > + if (ret) { > > > + dev_err_probe(&i2c->dev, ret, "Failed to add INTB irq_chip\n"); > > > + goto free_out; > > > + } > > > intb_domain = regmap_irq_get_domain(intb_irq_data); > > > - ret = devm_mfd_add_devices(&i2c->dev, PLATFORM_DEVID_AUTO, > > > - bd96801_mfd_cells, > > > - ARRAY_SIZE(bd96801_mfd_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 = ®ulator_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_mfd_cells[WDG_CELL].resources = &wdg_irq; > > > + bd96801_mfd_cells[WDG_CELL].num_resources = 1; > > > + > > > + if (num_errb) { > > > > if (!num_errb) > > goto skip_errb; > > Ok, can do. > > > > > > + 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 = ®ulator_res[num_intb + i]; > > > + > > > + *res = regulator_errb_irqs[i]; > > > + res->start = res->end = irq_create_mapping(errb_domain, > > > + res->start); > > > + } > > > + } > > > > skip_errb: > > ... > > Thanks for comments Lee. Reworking this will have to wait for the irqdomain > name suffix, which I will continue after Hervé has done his part of the > irqdomain changes. I will omit this patch from the next re-spin of the > series. I'm in no rush. :)
On 6/14/24 10:50, Lee Jones wrote: > On Fri, 14 Jun 2024, Matti Vaittinen wrote: > >> On 6/13/24 19:32, Lee Jones wrote: >>> On Tue, 04 Jun 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: >>>> v2 =>: >>>> - No changes >>>> v1 => v2: >>>> - New patch >>>> --- >>>> drivers/mfd/rohm-bd96801.c | 291 ++++++++++++++++++++++++++++++++----- >>>> 1 file changed, 253 insertions(+), 38 deletions(-) >>>> >>>> diff --git a/drivers/mfd/rohm-bd96801.c b/drivers/mfd/rohm-bd96801.c >>>> index 1c2a9591be7b..b7f073318873 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. >>> >>> Why bother adding all that blurb in the first place? >> >> Because, I assume there are users who would like to have the ERRB in use. >> The main purpose of this comment is that any such users could >> a) see this version does not support ERRB. >> b) can find the original RFC with ERRB supportn and a workaround. >> c) know why this version does not work with ERRB and thus fix this >> >> It seems this ERRB support may be missing from upstream for a while, hence I >> think having this note is worthy until (if) this ERRB patch lands in >> upstream. > > What I mean is - you're adding all of these extra lines in patch 3 and > removing them in patch 9. > True. This is because I had a feeling the irqdomain changes might not get merged that fast as it seemed like something that is not completely trivial. This comment is useful if patches 7-10 aren't merged together with 1-6 - which I now also hope is the case XD >>>> + * 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 ;) >>>> */ >> >> ... >> >> Thanks for comments Lee. Reworking this will have to wait for the irqdomain >> name suffix, which I will continue after Hervé has done his part of the >> irqdomain changes. I will omit this patch from the next re-spin of the >> series. > > I'm in no rush. :) Well, glad to hear ;) I still usually try to avoid delaying sending the follow-up patches. I am under impression it is easier to review the new revision if the previous revision was not reviewed too long ago... ;) I feel it is polite to tell the reviewers there will be some delay when I know it. Yours, -- Matti
On Fri, 14 Jun 2024, Matti Vaittinen wrote: > On 6/14/24 10:50, Lee Jones wrote: > > On Fri, 14 Jun 2024, Matti Vaittinen wrote: > > > > > On 6/13/24 19:32, Lee Jones wrote: > > > > On Tue, 04 Jun 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: > > > > > v2 =>: > > > > > - No changes > > > > > v1 => v2: > > > > > - New patch > > > > > --- > > > > > drivers/mfd/rohm-bd96801.c | 291 ++++++++++++++++++++++++++++++++----- > > > > > 1 file changed, 253 insertions(+), 38 deletions(-) > > > > > > > > > > diff --git a/drivers/mfd/rohm-bd96801.c b/drivers/mfd/rohm-bd96801.c > > > > > index 1c2a9591be7b..b7f073318873 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. > > > > > > > > Why bother adding all that blurb in the first place? > > > > > > Because, I assume there are users who would like to have the ERRB in use. > > > The main purpose of this comment is that any such users could > > > a) see this version does not support ERRB. > > > b) can find the original RFC with ERRB supportn and a workaround. > > > c) know why this version does not work with ERRB and thus fix this > > > > > > It seems this ERRB support may be missing from upstream for a while, hence I > > > think having this note is worthy until (if) this ERRB patch lands in > > > upstream. > > > > What I mean is - you're adding all of these extra lines in patch 3 and > > removing them in patch 9. > > > > True. This is because I had a feeling the irqdomain changes might not get > merged that fast as it seemed like something that is not completely trivial. > This comment is useful if patches 7-10 aren't merged together with 1-6 - > which I now also hope is the case XD > > > > > > + * 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 ;) > > > > > */ > > > > > > ... > > > > > > Thanks for comments Lee. Reworking this will have to wait for the irqdomain > > > name suffix, which I will continue after Hervé has done his part of the > > > irqdomain changes. I will omit this patch from the next re-spin of the > > > series. > > > > I'm in no rush. :) > > Well, glad to hear ;) I still usually try to avoid delaying sending the > follow-up patches. I am under impression it is easier to review the new > revision if the previous revision was not reviewed too long ago... ;) I'm used to it. Old reviews are cached locally. > I feel it is polite to tell the reviewers there will be some delay when I > know it. I wouldn't lose any sleep over it. :)
diff --git a/drivers/mfd/rohm-bd96801.c b/drivers/mfd/rohm-bd96801.c index 1c2a9591be7b..b7f073318873 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> @@ -45,6 +39,65 @@ #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"), @@ -89,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_mfd_cells[] = { - { - .name = "bd96801-wdt", - .resources = wdg_intb_irqs, - .num_resources = ARRAY_SIZE(wdg_intb_irqs), - }, { - .name = "bd96801-pmic", - .resources = regulator_intb_irqs, - .num_resources = ARRAY_SIZE(regulator_intb_irqs), - }, + [WDG_CELL] = { .name = "bd96801-wdt", }, + [REGULATOR_CELL] = { .name = "bd96801-pmic", }, }; static const struct regmap_range bd96801_volatile_ranges[] = { @@ -127,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 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), @@ -175,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_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 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], @@ -198,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) @@ -212,10 +364,28 @@ 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 = 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)) - return dev_err_probe(&i2c->dev, PTR_ERR(regmap), + if (IS_ERR(regmap)) { + ret = dev_err_probe(&i2c->dev, PTR_ERR(regmap), "Regmap initialization failed\n"); + goto free_out; + } ret = regmap_write(regmap, BD96801_LOCK_REG, BD96801_UNLOCK); if (ret) @@ -224,18 +394,63 @@ static int bd96801_i2c_probe(struct i2c_client *i2c) ret = devm_regmap_add_irq_chip(&i2c->dev, regmap, intb_irq, 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"); + if (ret) { + dev_err_probe(&i2c->dev, ret, "Failed to add INTB irq_chip\n"); + goto free_out; + } intb_domain = regmap_irq_get_domain(intb_irq_data); - ret = devm_mfd_add_devices(&i2c->dev, PLATFORM_DEVID_AUTO, - bd96801_mfd_cells, - ARRAY_SIZE(bd96801_mfd_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 = ®ulator_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_mfd_cells[WDG_CELL].resources = &wdg_irq; + bd96801_mfd_cells[WDG_CELL].num_resources = 1; + + 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 = ®ulator_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(&i2c->dev, "Failed to create subdevices\n"); + dev_err_probe(&i2c->dev, ret, "Failed to create subdevices\n"); + +free_out: + kfree(regulator_res); return ret; }
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: v2 =>: - No changes v1 => v2: - New patch --- drivers/mfd/rohm-bd96801.c | 291 ++++++++++++++++++++++++++++++++----- 1 file changed, 253 insertions(+), 38 deletions(-)