[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: Wednesday, June 12, 2024 4:50 PM
> Subject: Re: [PATCH v3 3/6] regulator: core: Add helper for allow access to enable register
> 
> On Tue, Jun 11, 2024 at 04:28:37PM +0000, Biju Das wrote:
> 
> > > 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
> 
> No, I mean regulator_get_exclusive(), this clearly can't work if there's more than one consumer.

OK, I will document like below

To access the hardware register for enabling/disabling the regulator,
consumers must use regulator_get_exclusive(), as it can't work if there's
more than one consumer. To enable/disable regulator use::
 
       int regulator_hardware_enable(struct regulator *regulator, bool enable);

> 
> > > 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??
> 
> Possibly.
> 
> > > 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()??
> 
> That too, I was mainly thinking about the name here though.

OK, will remove the restriction

-       if (enable) {
-               if (ops->enable == regulator_enable_regmap)
-                       ret = ops->enable(rdev);
-       } else {
-               if (ops->disable == regulator_disable_regmap)
-                       ret = rdev->desc->ops->disable(rdev);
-       }
+       if (enable)
+               ret = ops->enable(rdev);
+       else
+               ret = ops->disable(rdev);


Please let me know if anything wrong.

Cheers,
Biju