Messages in this thread | | | Date | Thu, 23 Mar 2023 10:12:21 +0100 | Subject | Re: [PATCH INTERNAL v1 3/3] regulator: tps6594-regulator: Add driver for TI TPS6594 regulators | From | jerome Neanne <> |
| |
>> +static int tps6594_get_rdev_by_name(const char *regulator_name, >> + struct regulator_dev *rdevbucktbl[BUCK_NB], >> + struct regulator_dev *rdevldotbl[LDO_NB], >> + struct regulator_dev *dev) >> +{ >> + int i; >> + >> + for (i = 0; i <= BUCK_NB; i++) { >> + if (strcmp(regulator_name, buck_regs[i].name) == 0) { >> + dev = rdevbucktbl[i]; >> + return 0; >> + } >> + } >> + >> + for (i = 0; i < ARRAY_SIZE(ldo_regs); i++) { >> + if (strcmp(regulator_name, ldo_regs[i].name) == 0) { >> + dev = rdevldotbl[i]; >> + return 0; >> + } >> + } >> + return -EINVAL; >> +} > >> + for (i = 0; i < ARRAY_SIZE(tps6594_regulator_irq_types); ++i) { >> + irq_type = &tps6594_regulator_irq_types[i]; >> + >> + irq = platform_get_irq_byname(pdev, irq_type->irq_name); >> + if (irq < 0) >> + return -EINVAL; >> + >> + irq_data[i].dev = tps->dev; >> + irq_data[i].type = irq_type; >> + >> + tps6594_get_rdev_by_name(irq_type->regulator_name, rdevbucktbl, >> + rdevldotbl, rdev); > > This would be simpler and you wouldn't need this lookup function if the > regulator descriptions included their IRQ names, then you could just > request the interrupts while registering the regulators. I changed the code to follow your recommendations then now in case of a multiphase buck, only one set of interrupt is requested. ex: multiphase buck1234 single buck5 cat /proc/interrupts: 563: 0 0 tps6594-0-0x4c 0 Edge buck1_ov 564: 0 0 tps6594-0-0x4c 1 Edge buck1_uv 565: 0 0 tps6594-0-0x4c 2 Edge buck1_sc 566: 0 0 tps6594-0-0x4c 3 Edge buck1_ilim 579: 0 0 tps6594-0-0x4c 16 Edge buck5_ov 580: 0 0 tps6594-0-0x4c 17 Edge buck5_uv 581: 0 0 tps6594-0-0x4c 18 Edge buck5_sc 582: 0 0 tps6594-0-0x4c 19 Edge buck5_ilim
buck2, buck3, buck4 are not associated to a regulator device because buck1 registers control all the multiphase bucks (only one logic regulator). Consequently the mapping for the associated interrupts does not occur. I'm not sure it's the right option. Do you suggest to keep it like that for multiphase? Is it better to request all the interrupts anyway and map it to the same rdev?
> >> + error = devm_request_threaded_irq(tps->dev, irq, NULL, >> + tps6594_regulator_irq_handler, >> + IRQF_ONESHOT, >> + irq_type->irq_name, >> + &irq_data[i]); >> + if (error) { >> + dev_err(tps->dev, "failed to request %s IRQ %d: %d\n", >> + irq_type->irq_name, irq, error); >> + return error; >> + } > > This leaks all previously requested interrupts. I'm not sure to understand this sentence correctly. You mean all the interrupts already requested are still allocated after the error occurs?
| |