diff mbox series

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

Message ID 20240611110402.58104-4-biju.das.jz@bp.renesas.com (mailing list archive)
State Superseded
Delegated to: Geert Uytterhoeven
Headers show
Series Add support for USB VBUS control for RZ/G2L SoCs | expand

Commit Message

Biju Das June 11, 2024, 11:03 a.m. UTC
Add a helper function that allow regulator consumers to allow low-level
enable register access, in order to enable/disable regulator in atomic
context.

The use-case for RZ/G2L SoC is to enable VBUS selection register based
on vbus detection that happens in interrupt context.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
v3:
 * New patch.
---
 Documentation/power/regulator/consumer.rst |  5 ++++
 drivers/regulator/core.c                   | 32 ++++++++++++++++++++++
 include/linux/regulator/consumer.h         |  8 ++++++
 3 files changed, 45 insertions(+)

Comments

Mark Brown June 11, 2024, 2:59 p.m. UTC | #1
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.

Also it's not sure about that name, if we were doing this it should be
more describing the effect on the regulator rather than this happening
to be done via a register write (this should also work for GPIOs...).
Biju Das June 11, 2024, 4:28 p.m. UTC | #2
Hi Mark Brown,

Thanks for the feedback.

> -----Original Message-----
> From: Mark Brown <broonie@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
Mark Brown June 12, 2024, 3:50 p.m. UTC | #3
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.

> > 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.
Biju Das June 14, 2024, 11:43 a.m. UTC | #4
Hi Mark Brown,

Thanks for the feedback.

> -----Original Message-----
> From: Mark Brown <broonie@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
Mark Brown June 14, 2024, 2:31 p.m. UTC | #5
On Fri, Jun 14, 2024 at 11:43:39AM +0000, Biju Das wrote:

> 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);

We should also enforce this.

> 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.

Sure.
Biju Das June 14, 2024, 3:52 p.m. UTC | #6
Hi Mark Brown,

Thanks for the feedback.

> -----Original Message-----
> From: Mark Brown <broonie@kernel.org>
> Sent: Friday, June 14, 2024 3:32 PM
> Subject: Re: [PATCH v3 3/6] regulator: core: Add helper for allow access to enable register
> 
> On Fri, Jun 14, 2024 at 11:43:39AM +0000, Biju Das wrote:
> 
> > 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);
> 
> We should also enforce this.

OK, will enforce and return error

	int ret = -EOPNOTSUPP;

	if (!ops || !ops->enable || !ops->disable || !regulator->exclusive)
		return ret;

Cheers,
Biju
diff mbox series

Patch

diff --git a/Documentation/power/regulator/consumer.rst b/Documentation/power/regulator/consumer.rst
index 85c2bf5ac07e..b5502f4ffe46 100644
--- a/Documentation/power/regulator/consumer.rst
+++ b/Documentation/power/regulator/consumer.rst
@@ -227,3 +227,8 @@  directly written to the voltage selector register, use::
 
 	int regulator_list_hardware_vsel(struct regulator *regulator,
 					 unsigned selector);
+
+To access the hardware register for enabling/disabling the regulator, use::
+
+	int regulator_set_hardware_enable_register(struct regulator *regulator,
+						   bool enable);
diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 5794f4e9dd52..19df42868bbd 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -3407,6 +3407,38 @@  int regulator_list_hardware_vsel(struct regulator *regulator,
 }
 EXPORT_SYMBOL_GPL(regulator_list_hardware_vsel);
 
+/**
+ * regulator_set_hardware_enable_register - set the HW enable register
+ * @regulator: regulator source
+ * @enable: true for enable, false for disable
+ *
+ * Request that the regulator be enabled/disabled with the regulator output at
+ * the predefined voltage or current value.
+ *
+ * On success 0 is returned, otherwise a negative errno is returned.
+ */
+int regulator_set_hardware_enable_register(struct regulator *regulator,
+					   bool enable)
+{
+	struct regulator_dev *rdev = regulator->rdev;
+	const struct regulator_ops *ops = rdev->desc->ops;
+	int ret = -EOPNOTSUPP;
+
+	if (!ops)
+		return ret;
+
+	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);
+	}
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(regulator_set_hardware_enable_register);
+
 /**
  * regulator_get_linear_step - return the voltage step size between VSEL values
  * @regulator: regulator source
diff --git a/include/linux/regulator/consumer.h b/include/linux/regulator/consumer.h
index e6f81fc1fb17..4aa5c57de052 100644
--- a/include/linux/regulator/consumer.h
+++ b/include/linux/regulator/consumer.h
@@ -250,6 +250,8 @@  int regulator_get_hardware_vsel_register(struct regulator *regulator,
 					 unsigned *vsel_mask);
 int regulator_list_hardware_vsel(struct regulator *regulator,
 				 unsigned selector);
+int regulator_set_hardware_enable_register(struct regulator *regulator,
+					   bool enable);
 
 /* regulator notifier block */
 int regulator_register_notifier(struct regulator *regulator,
@@ -571,6 +573,12 @@  static inline int regulator_list_hardware_vsel(struct regulator *regulator,
 	return -EOPNOTSUPP;
 }
 
+static inline int regulator_set_hardware_enable_register(struct regulator *regulator,
+							 bool enable)
+{
+	return -EOPNOTSUPP;
+}
+
 static inline int regulator_register_notifier(struct regulator *regulator,
 			      struct notifier_block *nb)
 {