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