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

[jerome Neanne <jneanne@baylibre.com>]

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.