Re: [PATCH INTERNAL v1 3/3] regulator: tps6594-regulator: Add driver for TI TPS6594 regulators

jerome Neanne
2023-03-24T08:00:12Z

On 23/03/2023 12:38, Mark Brown wrote:
> On Thu, Mar 23, 2023 at 10:12:21AM +0100, jerome Neanne wrote:
>
>>> 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.
>
>> 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?
>
> Do the other interrupts do anything useful for this configuration? With
> a lot of hardware the whole control interface gets merged into one which
> includes the interrupts.
>
Discussed the point with TI in //. In case of multiphase buck ex: buck12
All the control is delegated to buck1 registers but there is still a
possibility that an interrupt triggers on buck2 (overcurrent typically).
I slightly changed the logic so that all the interrupts are registered
even in multiphase mode. In that case interrupts for buck2 are attached
to rdev buck12.
>>>> + 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?
>
> Yes, I'd either not registered the devm or thought there was some other
> interrupt wasn't devm.
All the interrupts are requested with devm, then should be fine.