[PATCH v3 3/6] regulator: core: Add helper for allow access to enable register

[Biju Das]

Hi Mark Brown,

Thanks for the feedback.

> -----Original Message-----
> From: Mark Brown <broonie at kernel.org>
> Sent: Tuesday, June 11, 2024 4:00 PM
> Subject: Re: [PATCH v3 3/6] regulator: core: Add helper for allow access to enable register
> 
> On Tue, Jun 11, 2024 at 12:03:59PM +0100, Biju Das wrote:
> > Add a helper function that allow regulator consumers to allow
> > low-level enable register access, in order to enable/disable regulator
> > in atomic context.
> 
> > +To access the hardware register for enabling/disabling the regulator, use::
> > +
> > +	int regulator_set_hardware_enable_register(struct regulator *regulator,
> > +						   bool enable);
> 
> So, it'll doubtless not be a surprise that I'm not thrilled with this - it's basically just
> punching a hole straight through all the locking and reference counting in a way that's just
> begging for abuse.  At the very least we should have a check for exclusive access in there.

Do you mean exclusive access by means of spinlock to avoid race between enable/disable()?
If that is the case

Option1:
    Add a spin_lock in struct regulator_dev and add locking in regulator_xxx_hardware_enable_xxx()

Option 2:
   Core calls the callback for enable()/disable() and driver handles the exclusive access by
spin lock

or
Please share, if you have any other options?

> 
> Also it's not sure about that name, if we were doing this it should be more describing the effect

What about the name regulator_hardware_enable() to make it generic??

> on the regulator rather than this happening to be done via a register write (this should also work
> for GPIOs...).

Do you mean to make it generic, so that it works for both regmap based enable/disable() as well as
gpio based enable()/disable()??

Cheers,
Biju