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@xxxxxxxxx> > > > --- > > > 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@xxxxxxxxx/ > > > - * 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. :) -- Lee Jones [李琼斯]
