| Message ID | 20240611110402.58104-4-biju.das.jz@bp.renesas.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | Add support for USB VBUS control for RZ/G2L SoCs | expand |
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...).
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
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.
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
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.
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 --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) {
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(+)