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

[jerome Neanne]

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.