Hi Mark Brown,
Thanks for the feedback.
> -----Original Message-----
> From: Mark Brown <broonie@xxxxxxxxxx>
> 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
