[RFC PATCH 0/7] Devm helpers for regulator get and enable

[RFC PATCH 0/7] Devm helpers for regulator get and enable

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 4833 bytes --]

Devm helpers for regulator get and enable

First patch in the series is actually just a simple documentation fix
which could be taken in as it is now (even though the rest of the series
is a RFC).

A few* drivers seem to pattern demonstrated by pseudocode:

- devm_regulator_get()
- regulator_enable()
- devm_add_action_or_reset(regulator_disable())

(*) A rough idea what 'a few' means in this context can be get by issuing:
"git grep -In -A10 devm_regulator_get |grep -B5 -A5 add_action |less"
and then further checking some of the reported drivers. This is what I did
when I realized I needed to enable a regulator for accelerometer and
thought I'd go with devm-action...

Introducing devm helpers for this pattern would remove bunch of code from
drivers. Typically at least following:

- replace 3 calls (devm_regulator_get[_optional](), regulator_enable(),
  devm_add_action_or_reset()) with just one
  (devm_regulator_get_enable[_optional]()).
- drop disable callback.

I believe this simplifies things by removing some dublicated code.

The other RFC aspect besides the question if this actually is useful, is
whether the devm_regulator_get_enable[_optional]() should return a pointer
to the obtained regulator or not. This RFC version does not return the
pointer for user because any call to regulator_disable() may lead to
regulator enable count imbalance upon device detach. (Eg, if someone calls
regulator_disable() and the device is then detached before user has
re-enabled the regulator). Not returning the pointer to obtained regulator
to caller is a good hint that the enable/disable should not be manually
handled when this API is used.

OTOH, not returning the pointer reduces the use-cases by not allowing
the consumers to perform other regulator actions. For example request the
voltages. A few drivers which used the "get, enable,
devm_action_to_disable" did also query the voltages. The suggested form of
the API does not suit needs of such users. The new API in its current form
really allows to only cover the very dummy cases where regulator is only
enabled for a lifetime of the driver. I am unsure if this is really
beneficial (well, there seems to be bunch of drivers doing just this) - or
if we should go with a version returning the struct regulator *

Some drivers did also manually disable the regulator (even though they had
registered the devm-action for disable) for PM functionality. I am unsure
if such use for suspend is actually safe(?) I didn't check if we can
guarantee that the driver is not detached after the PM suspend has disabled
the regulator(?)

This RFC converts only few a drivers to demonstrate benefits. This makes it
easier to rework the series if people thinks returning the pointer to
struct regulator should be done. I can't promise I'll convert all drivers
so, there is still plenty of fish in the sea for people who like to improve
the code (or count the beans ;]).

Finally - most of the converted drivers have not been tested (other than
compile-tested) due to lack of HW. All reviews and testing is _highly_
appreciated (as always!). I have the driver changes in individual patches
to make reviewing easier. I will squash the driver changes into one patch /
subsystem when I'll drop the "RFC" from the series.

Patch 1:
	Fix docmentation (devres API list) for regulator APIs
Patch 2:
	The devm helpers.
Patch 3:
	Add new devm-helper APIs to docs.
Patches 4 ... 7:
	Example drivers.

---

Matti Vaittinen (7):
  docs: devres: regulator: Add missing devm_* functions to devres.rst
  regulator: Add devm helpers for get and enable
  docs: devres: regulator: Add new get_enable functions to devres.rst
  clk: cdce925: simplify using devm_regulator_get_enable()
  gpu: drm: meson: simplify using devm_regulator_get_enable_optional()
  hwmon: lm90: simplify using devm_regulator_get_enable()
  adc: ad7192: simplify using devm_regulator_get_enable()

 .../driver-api/driver-model/devres.rst        |  9 +++
 drivers/clk/clk-cdce925.c                     | 21 ++-----
 drivers/gpu/drm/meson/meson_dw_hdmi.c         | 23 +-------
 drivers/hwmon/lm90.c                          | 21 +------
 drivers/iio/adc/ad7192.c                      | 15 +----
 drivers/regulator/devres.c                    | 59 +++++++++++++++++++
 include/linux/regulator/consumer.h            | 13 ++++
 7 files changed, 92 insertions(+), 69 deletions(-)

-- 
2.37.1


-- 
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND

~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
Simon says - in Latin please.
~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
Thanks to Simon Glass for the translation =] 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[RFC PATCH 0/7] Devm helpers for regulator get and enable

[Matti Vaittinen]

[-- Attachment #1.1: Type: text/plain, Size: 4833 bytes --]

Devm helpers for regulator get and enable

First patch in the series is actually just a simple documentation fix
which could be taken in as it is now (even though the rest of the series
is a RFC).

A few* drivers seem to pattern demonstrated by pseudocode:

- devm_regulator_get()
- regulator_enable()
- devm_add_action_or_reset(regulator_disable())

(*) A rough idea what 'a few' means in this context can be get by issuing:
"git grep -In -A10 devm_regulator_get |grep -B5 -A5 add_action |less"
and then further checking some of the reported drivers. This is what I did
when I realized I needed to enable a regulator for accelerometer and
thought I'd go with devm-action...

Introducing devm helpers for this pattern would remove bunch of code from
drivers. Typically at least following:

- replace 3 calls (devm_regulator_get[_optional](), regulator_enable(),
  devm_add_action_or_reset()) with just one
  (devm_regulator_get_enable[_optional]()).
- drop disable callback.

I believe this simplifies things by removing some dublicated code.

The other RFC aspect besides the question if this actually is useful, is
whether the devm_regulator_get_enable[_optional]() should return a pointer
to the obtained regulator or not. This RFC version does not return the
pointer for user because any call to regulator_disable() may lead to
regulator enable count imbalance upon device detach. (Eg, if someone calls
regulator_disable() and the device is then detached before user has
re-enabled the regulator). Not returning the pointer to obtained regulator
to caller is a good hint that the enable/disable should not be manually
handled when this API is used.

OTOH, not returning the pointer reduces the use-cases by not allowing
the consumers to perform other regulator actions. For example request the
voltages. A few drivers which used the "get, enable,
devm_action_to_disable" did also query the voltages. The suggested form of
the API does not suit needs of such users. The new API in its current form
really allows to only cover the very dummy cases where regulator is only
enabled for a lifetime of the driver. I am unsure if this is really
beneficial (well, there seems to be bunch of drivers doing just this) - or
if we should go with a version returning the struct regulator *

Some drivers did also manually disable the regulator (even though they had
registered the devm-action for disable) for PM functionality. I am unsure
if such use for suspend is actually safe(?) I didn't check if we can
guarantee that the driver is not detached after the PM suspend has disabled
the regulator(?)

This RFC converts only few a drivers to demonstrate benefits. This makes it
easier to rework the series if people thinks returning the pointer to
struct regulator should be done. I can't promise I'll convert all drivers
so, there is still plenty of fish in the sea for people who like to improve
the code (or count the beans ;]).

Finally - most of the converted drivers have not been tested (other than
compile-tested) due to lack of HW. All reviews and testing is _highly_
appreciated (as always!). I have the driver changes in individual patches
to make reviewing easier. I will squash the driver changes into one patch /
subsystem when I'll drop the "RFC" from the series.

Patch 1:
	Fix docmentation (devres API list) for regulator APIs
Patch 2:
	The devm helpers.
Patch 3:
	Add new devm-helper APIs to docs.
Patches 4 ... 7:
	Example drivers.

---

Matti Vaittinen (7):
  docs: devres: regulator: Add missing devm_* functions to devres.rst
  regulator: Add devm helpers for get and enable
  docs: devres: regulator: Add new get_enable functions to devres.rst
  clk: cdce925: simplify using devm_regulator_get_enable()
  gpu: drm: meson: simplify using devm_regulator_get_enable_optional()
  hwmon: lm90: simplify using devm_regulator_get_enable()
  adc: ad7192: simplify using devm_regulator_get_enable()

 .../driver-api/driver-model/devres.rst        |  9 +++
 drivers/clk/clk-cdce925.c                     | 21 ++-----
 drivers/gpu/drm/meson/meson_dw_hdmi.c         | 23 +-------
 drivers/hwmon/lm90.c                          | 21 +------
 drivers/iio/adc/ad7192.c                      | 15 +----
 drivers/regulator/devres.c                    | 59 +++++++++++++++++++
 include/linux/regulator/consumer.h            | 13 ++++
 7 files changed, 92 insertions(+), 69 deletions(-)

-- 
2.37.1


-- 
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND

~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
Simon says - in Latin please.
~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
Thanks to Simon Glass for the translation =] 

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 167 bytes --]

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

[RFC PATCH 0/7] Devm helpers for regulator get and enable

[Matti Vaittinen]

[-- Attachment #1.1: Type: text/plain, Size: 4833 bytes --]

Devm helpers for regulator get and enable

First patch in the series is actually just a simple documentation fix
which could be taken in as it is now (even though the rest of the series
is a RFC).

A few* drivers seem to pattern demonstrated by pseudocode:

- devm_regulator_get()
- regulator_enable()
- devm_add_action_or_reset(regulator_disable())

(*) A rough idea what 'a few' means in this context can be get by issuing:
"git grep -In -A10 devm_regulator_get |grep -B5 -A5 add_action |less"
and then further checking some of the reported drivers. This is what I did
when I realized I needed to enable a regulator for accelerometer and
thought I'd go with devm-action...

Introducing devm helpers for this pattern would remove bunch of code from
drivers. Typically at least following:

- replace 3 calls (devm_regulator_get[_optional](), regulator_enable(),
  devm_add_action_or_reset()) with just one
  (devm_regulator_get_enable[_optional]()).
- drop disable callback.

I believe this simplifies things by removing some dublicated code.

The other RFC aspect besides the question if this actually is useful, is
whether the devm_regulator_get_enable[_optional]() should return a pointer
to the obtained regulator or not. This RFC version does not return the
pointer for user because any call to regulator_disable() may lead to
regulator enable count imbalance upon device detach. (Eg, if someone calls
regulator_disable() and the device is then detached before user has
re-enabled the regulator). Not returning the pointer to obtained regulator
to caller is a good hint that the enable/disable should not be manually
handled when this API is used.

OTOH, not returning the pointer reduces the use-cases by not allowing
the consumers to perform other regulator actions. For example request the
voltages. A few drivers which used the "get, enable,
devm_action_to_disable" did also query the voltages. The suggested form of
the API does not suit needs of such users. The new API in its current form
really allows to only cover the very dummy cases where regulator is only
enabled for a lifetime of the driver. I am unsure if this is really
beneficial (well, there seems to be bunch of drivers doing just this) - or
if we should go with a version returning the struct regulator *

Some drivers did also manually disable the regulator (even though they had
registered the devm-action for disable) for PM functionality. I am unsure
if such use for suspend is actually safe(?) I didn't check if we can
guarantee that the driver is not detached after the PM suspend has disabled
the regulator(?)

This RFC converts only few a drivers to demonstrate benefits. This makes it
easier to rework the series if people thinks returning the pointer to
struct regulator should be done. I can't promise I'll convert all drivers
so, there is still plenty of fish in the sea for people who like to improve
the code (or count the beans ;]).

Finally - most of the converted drivers have not been tested (other than
compile-tested) due to lack of HW. All reviews and testing is _highly_
appreciated (as always!). I have the driver changes in individual patches
to make reviewing easier. I will squash the driver changes into one patch /
subsystem when I'll drop the "RFC" from the series.

Patch 1:
	Fix docmentation (devres API list) for regulator APIs
Patch 2:
	The devm helpers.
Patch 3:
	Add new devm-helper APIs to docs.
Patches 4 ... 7:
	Example drivers.

---

Matti Vaittinen (7):
  docs: devres: regulator: Add missing devm_* functions to devres.rst
  regulator: Add devm helpers for get and enable
  docs: devres: regulator: Add new get_enable functions to devres.rst
  clk: cdce925: simplify using devm_regulator_get_enable()
  gpu: drm: meson: simplify using devm_regulator_get_enable_optional()
  hwmon: lm90: simplify using devm_regulator_get_enable()
  adc: ad7192: simplify using devm_regulator_get_enable()

 .../driver-api/driver-model/devres.rst        |  9 +++
 drivers/clk/clk-cdce925.c                     | 21 ++-----
 drivers/gpu/drm/meson/meson_dw_hdmi.c         | 23 +-------
 drivers/hwmon/lm90.c                          | 21 +------
 drivers/iio/adc/ad7192.c                      | 15 +----
 drivers/regulator/devres.c                    | 59 +++++++++++++++++++
 include/linux/regulator/consumer.h            | 13 ++++
 7 files changed, 92 insertions(+), 69 deletions(-)

-- 
2.37.1


-- 
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND

~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
Simon says - in Latin please.
~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
Thanks to Simon Glass for the translation =] 

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[RFC PATCH 0/7] Devm helpers for regulator get and enable

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 4833 bytes --]

Devm helpers for regulator get and enable

First patch in the series is actually just a simple documentation fix
which could be taken in as it is now (even though the rest of the series
is a RFC).

A few* drivers seem to pattern demonstrated by pseudocode:

- devm_regulator_get()
- regulator_enable()
- devm_add_action_or_reset(regulator_disable())

(*) A rough idea what 'a few' means in this context can be get by issuing:
"git grep -In -A10 devm_regulator_get |grep -B5 -A5 add_action |less"
and then further checking some of the reported drivers. This is what I did
when I realized I needed to enable a regulator for accelerometer and
thought I'd go with devm-action...

Introducing devm helpers for this pattern would remove bunch of code from
drivers. Typically at least following:

- replace 3 calls (devm_regulator_get[_optional](), regulator_enable(),
  devm_add_action_or_reset()) with just one
  (devm_regulator_get_enable[_optional]()).
- drop disable callback.

I believe this simplifies things by removing some dublicated code.

The other RFC aspect besides the question if this actually is useful, is
whether the devm_regulator_get_enable[_optional]() should return a pointer
to the obtained regulator or not. This RFC version does not return the
pointer for user because any call to regulator_disable() may lead to
regulator enable count imbalance upon device detach. (Eg, if someone calls
regulator_disable() and the device is then detached before user has
re-enabled the regulator). Not returning the pointer to obtained regulator
to caller is a good hint that the enable/disable should not be manually
handled when this API is used.

OTOH, not returning the pointer reduces the use-cases by not allowing
the consumers to perform other regulator actions. For example request the
voltages. A few drivers which used the "get, enable,
devm_action_to_disable" did also query the voltages. The suggested form of
the API does not suit needs of such users. The new API in its current form
really allows to only cover the very dummy cases where regulator is only
enabled for a lifetime of the driver. I am unsure if this is really
beneficial (well, there seems to be bunch of drivers doing just this) - or
if we should go with a version returning the struct regulator *

Some drivers did also manually disable the regulator (even though they had
registered the devm-action for disable) for PM functionality. I am unsure
if such use for suspend is actually safe(?) I didn't check if we can
guarantee that the driver is not detached after the PM suspend has disabled
the regulator(?)

This RFC converts only few a drivers to demonstrate benefits. This makes it
easier to rework the series if people thinks returning the pointer to
struct regulator should be done. I can't promise I'll convert all drivers
so, there is still plenty of fish in the sea for people who like to improve
the code (or count the beans ;]).

Finally - most of the converted drivers have not been tested (other than
compile-tested) due to lack of HW. All reviews and testing is _highly_
appreciated (as always!). I have the driver changes in individual patches
to make reviewing easier. I will squash the driver changes into one patch /
subsystem when I'll drop the "RFC" from the series.

Patch 1:
	Fix docmentation (devres API list) for regulator APIs
Patch 2:
	The devm helpers.
Patch 3:
	Add new devm-helper APIs to docs.
Patches 4 ... 7:
	Example drivers.

---

Matti Vaittinen (7):
  docs: devres: regulator: Add missing devm_* functions to devres.rst
  regulator: Add devm helpers for get and enable
  docs: devres: regulator: Add new get_enable functions to devres.rst
  clk: cdce925: simplify using devm_regulator_get_enable()
  gpu: drm: meson: simplify using devm_regulator_get_enable_optional()
  hwmon: lm90: simplify using devm_regulator_get_enable()
  adc: ad7192: simplify using devm_regulator_get_enable()

 .../driver-api/driver-model/devres.rst        |  9 +++
 drivers/clk/clk-cdce925.c                     | 21 ++-----
 drivers/gpu/drm/meson/meson_dw_hdmi.c         | 23 +-------
 drivers/hwmon/lm90.c                          | 21 +------
 drivers/iio/adc/ad7192.c                      | 15 +----
 drivers/regulator/devres.c                    | 59 +++++++++++++++++++
 include/linux/regulator/consumer.h            | 13 ++++
 7 files changed, 92 insertions(+), 69 deletions(-)

-- 
2.37.1


-- 
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND

~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
Simon says - in Latin please.
~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
Thanks to Simon Glass for the translation =] 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[RFC PATCH 1/7] docs: devres: regulator: Add missing devm_* functions to devres.rst

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 1568 bytes --]

A few managed regulator functions were missing from the API list.

Add missing functions.

Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
---
This one is actually a documentation fix which adds existing APIs to the
list. I guess this patch is good for being merged independently from the
rest of the series (which is just an RFC atm).

 Documentation/driver-api/driver-model/devres.rst | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/Documentation/driver-api/driver-model/devres.rst b/Documentation/driver-api/driver-model/devres.rst
index 2d39967bafcc..271d1eb2234b 100644
--- a/Documentation/driver-api/driver-model/devres.rst
+++ b/Documentation/driver-api/driver-model/devres.rst
@@ -406,10 +406,17 @@ PWM
   devm_fwnode_pwm_get()
 
 REGULATOR
+  devm_regulator_bulk_register_supply_alias()
   devm_regulator_bulk_get()
   devm_regulator_get()
+  devm_regulator_get_exclusive()
+  devm_regulator_get_optional()
+  devm_regulator_irq_helper()
   devm_regulator_put()
   devm_regulator_register()
+  devm_regulator_register_notifier()
+  devm_regulator_register_supply_alias()
+  devm_regulator_unregister_notifier()
 
 RESET
   devm_reset_control_get()
-- 
2.37.1


-- 
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND

~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
Simon says - in Latin please.
~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
Thanks to Simon Glass for the translation =] 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[RFC PATCH 2/7] regulator: Add devm helpers for get and enable

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 4278 bytes --]

A few regulator consumer drivers seem to be just getting a regulator,
enabling it and registering a devm-action to disable the regulator at
the driver detach and then forget about it.

We can simplify this a bit by adding a devm-helper for this pattern.
Add devm_regulator_get_enable() and devm_regulator_get_enable_optional()

Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
---
 drivers/regulator/devres.c         | 59 ++++++++++++++++++++++++++++++
 include/linux/regulator/consumer.h | 13 +++++++
 2 files changed, 72 insertions(+)

diff --git a/drivers/regulator/devres.c b/drivers/regulator/devres.c
index 9113233f41cd..61b0facc975b 100644
--- a/drivers/regulator/devres.c
+++ b/drivers/regulator/devres.c
@@ -70,6 +70,65 @@ struct regulator *devm_regulator_get_exclusive(struct device *dev,
 }
 EXPORT_SYMBOL_GPL(devm_regulator_get_exclusive);
 
+static void regulator_action_disable(void *d)
+{
+	struct regulator *r = (struct regulator *)d;
+
+	regulator_disable(r);
+}
+
+static int _devm_regulator_get_enable(struct device *dev, const char *id,
+				      int get_type)
+{
+	struct regulator *r;
+	int ret;
+
+	r = _devm_regulator_get(dev, id, get_type);
+	if (IS_ERR(r))
+		return PTR_ERR(r);
+
+	ret = regulator_enable(r);
+	if (!ret)
+		ret = devm_add_action_or_reset(dev, &regulator_action_disable, r);
+
+	if (ret)
+		devm_regulator_put(r);
+
+	return ret;
+}
+
+/**
+ * devm_regulator_get_enable_optional - Resource managed regulator get and enable
+ * @dev: device to supply
+ * @id:  supply name or regulator ID.
+ *
+ * Get and enable regulator for duration of the device life-time.
+ * regulator_disable() and regulator_put() are automatically called on driver
+ * detach. See regulator_get_optional() and regulator_enable() for more
+ * information.
+ */
+int devm_regulator_get_enable_optional(struct device *dev, const char *id)
+{
+	return _devm_regulator_get_enable(dev, id, OPTIONAL_GET);
+}
+EXPORT_SYMBOL_GPL(devm_regulator_get_enable_optional);
+
+/**
+ * devm_regulator_get_enable - Resource managed regulator get and enable
+ * @dev: device to supply
+ * @id:  supply name or regulator ID.
+ *
+ * Get and enable regulator for duration of the device life-time.
+ * regulator_disable() and regulator_put() are automatically called on driver
+ * detach. See regulator_get() and regulator_enable() for more
+ * information.
+ */
+int devm_regulator_get_enable(struct device *dev, const char *id)
+{
+	return _devm_regulator_get_enable(dev, id, NORMAL_GET);
+}
+EXPORT_SYMBOL_GPL(devm_regulator_get_enable);
+
 /**
  * devm_regulator_get_optional - Resource managed regulator_get_optional()
  * @dev: device to supply
diff --git a/include/linux/regulator/consumer.h b/include/linux/regulator/consumer.h
index bbf6590a6dec..58e3dbcddb12 100644
--- a/include/linux/regulator/consumer.h
+++ b/include/linux/regulator/consumer.h
@@ -203,6 +203,8 @@ struct regulator *__must_check regulator_get_optional(struct device *dev,
 						      const char *id);
 struct regulator *__must_check devm_regulator_get_optional(struct device *dev,
 							   const char *id);
+int devm_regulator_get_enable(struct device *dev, const char *id);
+int devm_regulator_get_enable_optional(struct device *dev, const char *id);
 void regulator_put(struct regulator *regulator);
 void devm_regulator_put(struct regulator *regulator);
 
@@ -346,6 +348,17 @@ devm_regulator_get_exclusive(struct device *dev, const char *id)
 	return ERR_PTR(-ENODEV);
 }
 
+static inline int devm_regulator_get_enable(struct device *dev, const char *id)
+{
+	return -ENODEV;
+}
+
+static inline int devm_regulator_get_enable_optional(struct device *dev,
+						     const char *id)
+{
+	return -ENODEV;
+}
+
 static inline struct regulator *__must_check
 regulator_get_optional(struct device *dev, const char *id)
 {
-- 
2.37.1


-- 
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND

~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
Simon says - in Latin please.
~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
Thanks to Simon Glass for the translation =] 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[RFC PATCH 3/7] docs: devres: regulator: Add new get_enable functions to devres.rst

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 1187 bytes --]

Add the new devm_regulator_get_enable() and
devm_regulator_get_enable_optional() to devres.rst

Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
---
 Documentation/driver-api/driver-model/devres.rst | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/driver-api/driver-model/devres.rst b/Documentation/driver-api/driver-model/devres.rst
index 271d1eb2234b..6ee8e9ab2478 100644
--- a/Documentation/driver-api/driver-model/devres.rst
+++ b/Documentation/driver-api/driver-model/devres.rst
@@ -409,6 +409,8 @@ REGULATOR
   devm_regulator_bulk_register_supply_alias()
   devm_regulator_bulk_get()
   devm_regulator_get()
+  devm_regulator_get_enable()
+  devm_regulator_get_enable_optional()
   devm_regulator_get_exclusive()
   devm_regulator_get_optional()
   devm_regulator_irq_helper()
-- 
2.37.1


-- 
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND

~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
Simon says - in Latin please.
~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
Thanks to Simon Glass for the translation =] 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[RFC PATCH 4/7] clk: cdce925: simplify using devm_regulator_get_enable()

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 1889 bytes --]

Simplify the driver using devm_regulator_get_enable() instead of
open-coding the devm_add_action_or_reset().

A (minor?) functional change is that we don't print an error in case of a
deferred probe. Now we also print the error no matter which of the
involved calls caused the failure.

Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
---
 drivers/clk/clk-cdce925.c | 21 ++++-----------------
 1 file changed, 4 insertions(+), 17 deletions(-)

diff --git a/drivers/clk/clk-cdce925.c b/drivers/clk/clk-cdce925.c
index ef9a2d44e40c..6350682f7e6d 100644
--- a/drivers/clk/clk-cdce925.c
+++ b/drivers/clk/clk-cdce925.c
@@ -603,28 +603,15 @@ of_clk_cdce925_get(struct of_phandle_args *clkspec, void *_data)
 	return &data->clk[idx].hw;
 }
 
-static void cdce925_regulator_disable(void *regulator)
-{
-	regulator_disable(regulator);
-}
-
 static int cdce925_regulator_enable(struct device *dev, const char *name)
 {
-	struct regulator *regulator;
 	int err;
 
-	regulator = devm_regulator_get(dev, name);
-	if (IS_ERR(regulator))
-		return PTR_ERR(regulator);
-
-	err = regulator_enable(regulator);
-	if (err) {
-		dev_err(dev, "Failed to enable %s: %d\n", name, err);
-		return err;
-	}
+	err = devm_regulator_get_enable(dev, name);
+	if (err)
+		dev_err_probe(dev, err, "Failed to enable %s:\n", name);
 
-	return devm_add_action_or_reset(dev, cdce925_regulator_disable,
-					regulator);
+	return err;
 }
 
 /* The CDCE925 uses a funky way to read/write registers. Bulk mode is
-- 
2.37.1


-- 
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND

~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
Simon says - in Latin please.
~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
Thanks to Simon Glass for the translation =] 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[RFC PATCH 5/7] gpu: drm: meson: simplify using devm_regulator_get_enable_optional()

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 2377 bytes --]

Drop open-coded pattern: 'devm_regulator_get(), regulator_enable(),
add_action_or_reset(regulator_disable)' and use the
devm_regulator_get_enable_optional(). Also drop the seemingly unused
struct member 'hdmi_supply'.

Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
---
 drivers/gpu/drm/meson/meson_dw_hdmi.c | 23 +++--------------------
 1 file changed, 3 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/meson/meson_dw_hdmi.c b/drivers/gpu/drm/meson/meson_dw_hdmi.c
index 5cd2b2ebbbd3..7642f740272b 100644
--- a/drivers/gpu/drm/meson/meson_dw_hdmi.c
+++ b/drivers/gpu/drm/meson/meson_dw_hdmi.c
@@ -140,7 +140,6 @@ struct meson_dw_hdmi {
 	struct reset_control *hdmitx_apb;
 	struct reset_control *hdmitx_ctrl;
 	struct reset_control *hdmitx_phy;
-	struct regulator *hdmi_supply;
 	u32 irq_stat;
 	struct dw_hdmi *hdmi;
 	struct drm_bridge *bridge;
@@ -665,11 +664,6 @@ static void meson_dw_hdmi_init(struct meson_dw_hdmi *meson_dw_hdmi)
 
 }
 
-static void meson_disable_regulator(void *data)
-{
-	regulator_disable(data);
-}
-
 static void meson_disable_clk(void *data)
 {
 	clk_disable_unprepare(data);
@@ -723,20 +717,9 @@ static int meson_dw_hdmi_bind(struct device *dev, struct device *master,
 	meson_dw_hdmi->data = match;
 	dw_plat_data = &meson_dw_hdmi->dw_plat_data;
 
-	meson_dw_hdmi->hdmi_supply = devm_regulator_get_optional(dev, "hdmi");
-	if (IS_ERR(meson_dw_hdmi->hdmi_supply)) {
-		if (PTR_ERR(meson_dw_hdmi->hdmi_supply) == -EPROBE_DEFER)
-			return -EPROBE_DEFER;
-		meson_dw_hdmi->hdmi_supply = NULL;
-	} else {
-		ret = regulator_enable(meson_dw_hdmi->hdmi_supply);
-		if (ret)
-			return ret;
-		ret = devm_add_action_or_reset(dev, meson_disable_regulator,
-					       meson_dw_hdmi->hdmi_supply);
-		if (ret)
-			return ret;
-	}
+	ret = devm_regulator_get_enable_optional(dev, "hdmi");
+	if (ret != -ENODEV)
+		return ret;
 
 	meson_dw_hdmi->hdmitx_apb = devm_reset_control_get_exclusive(dev,
 						"hdmitx_apb");
-- 
2.37.1


-- 
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND

~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
Simon says - in Latin please.
~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
Thanks to Simon Glass for the translation =] 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[RFC PATCH 5/7] gpu: drm: meson: simplify using devm_regulator_get_enable_optional()

[Matti Vaittinen]

[-- Attachment #1.1: Type: text/plain, Size: 2377 bytes --]

Drop open-coded pattern: 'devm_regulator_get(), regulator_enable(),
add_action_or_reset(regulator_disable)' and use the
devm_regulator_get_enable_optional(). Also drop the seemingly unused
struct member 'hdmi_supply'.

Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
---
 drivers/gpu/drm/meson/meson_dw_hdmi.c | 23 +++--------------------
 1 file changed, 3 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/meson/meson_dw_hdmi.c b/drivers/gpu/drm/meson/meson_dw_hdmi.c
index 5cd2b2ebbbd3..7642f740272b 100644
--- a/drivers/gpu/drm/meson/meson_dw_hdmi.c
+++ b/drivers/gpu/drm/meson/meson_dw_hdmi.c
@@ -140,7 +140,6 @@ struct meson_dw_hdmi {
 	struct reset_control *hdmitx_apb;
 	struct reset_control *hdmitx_ctrl;
 	struct reset_control *hdmitx_phy;
-	struct regulator *hdmi_supply;
 	u32 irq_stat;
 	struct dw_hdmi *hdmi;
 	struct drm_bridge *bridge;
@@ -665,11 +664,6 @@ static void meson_dw_hdmi_init(struct meson_dw_hdmi *meson_dw_hdmi)
 
 }
 
-static void meson_disable_regulator(void *data)
-{
-	regulator_disable(data);
-}
-
 static void meson_disable_clk(void *data)
 {
 	clk_disable_unprepare(data);
@@ -723,20 +717,9 @@ static int meson_dw_hdmi_bind(struct device *dev, struct device *master,
 	meson_dw_hdmi->data = match;
 	dw_plat_data = &meson_dw_hdmi->dw_plat_data;
 
-	meson_dw_hdmi->hdmi_supply = devm_regulator_get_optional(dev, "hdmi");
-	if (IS_ERR(meson_dw_hdmi->hdmi_supply)) {
-		if (PTR_ERR(meson_dw_hdmi->hdmi_supply) == -EPROBE_DEFER)
-			return -EPROBE_DEFER;
-		meson_dw_hdmi->hdmi_supply = NULL;
-	} else {
-		ret = regulator_enable(meson_dw_hdmi->hdmi_supply);
-		if (ret)
-			return ret;
-		ret = devm_add_action_or_reset(dev, meson_disable_regulator,
-					       meson_dw_hdmi->hdmi_supply);
-		if (ret)
-			return ret;
-	}
+	ret = devm_regulator_get_enable_optional(dev, "hdmi");
+	if (ret != -ENODEV)
+		return ret;
 
 	meson_dw_hdmi->hdmitx_apb = devm_reset_control_get_exclusive(dev,
 						"hdmitx_apb");
-- 
2.37.1


-- 
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND

~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
Simon says - in Latin please.
~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
Thanks to Simon Glass for the translation =] 

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 167 bytes --]

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

[RFC PATCH 5/7] gpu: drm: meson: simplify using devm_regulator_get_enable_optional()

[Matti Vaittinen]

[-- Attachment #1.1: Type: text/plain, Size: 2377 bytes --]

Drop open-coded pattern: 'devm_regulator_get(), regulator_enable(),
add_action_or_reset(regulator_disable)' and use the
devm_regulator_get_enable_optional(). Also drop the seemingly unused
struct member 'hdmi_supply'.

Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
---
 drivers/gpu/drm/meson/meson_dw_hdmi.c | 23 +++--------------------
 1 file changed, 3 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/meson/meson_dw_hdmi.c b/drivers/gpu/drm/meson/meson_dw_hdmi.c
index 5cd2b2ebbbd3..7642f740272b 100644
--- a/drivers/gpu/drm/meson/meson_dw_hdmi.c
+++ b/drivers/gpu/drm/meson/meson_dw_hdmi.c
@@ -140,7 +140,6 @@ struct meson_dw_hdmi {
 	struct reset_control *hdmitx_apb;
 	struct reset_control *hdmitx_ctrl;
 	struct reset_control *hdmitx_phy;
-	struct regulator *hdmi_supply;
 	u32 irq_stat;
 	struct dw_hdmi *hdmi;
 	struct drm_bridge *bridge;
@@ -665,11 +664,6 @@ static void meson_dw_hdmi_init(struct meson_dw_hdmi *meson_dw_hdmi)
 
 }
 
-static void meson_disable_regulator(void *data)
-{
-	regulator_disable(data);
-}
-
 static void meson_disable_clk(void *data)
 {
 	clk_disable_unprepare(data);
@@ -723,20 +717,9 @@ static int meson_dw_hdmi_bind(struct device *dev, struct device *master,
 	meson_dw_hdmi->data = match;
 	dw_plat_data = &meson_dw_hdmi->dw_plat_data;
 
-	meson_dw_hdmi->hdmi_supply = devm_regulator_get_optional(dev, "hdmi");
-	if (IS_ERR(meson_dw_hdmi->hdmi_supply)) {
-		if (PTR_ERR(meson_dw_hdmi->hdmi_supply) == -EPROBE_DEFER)
-			return -EPROBE_DEFER;
-		meson_dw_hdmi->hdmi_supply = NULL;
-	} else {
-		ret = regulator_enable(meson_dw_hdmi->hdmi_supply);
-		if (ret)
-			return ret;
-		ret = devm_add_action_or_reset(dev, meson_disable_regulator,
-					       meson_dw_hdmi->hdmi_supply);
-		if (ret)
-			return ret;
-	}
+	ret = devm_regulator_get_enable_optional(dev, "hdmi");
+	if (ret != -ENODEV)
+		return ret;
 
 	meson_dw_hdmi->hdmitx_apb = devm_reset_control_get_exclusive(dev,
 						"hdmitx_apb");
-- 
2.37.1


-- 
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND

~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
Simon says - in Latin please.
~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
Thanks to Simon Glass for the translation =] 

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[RFC PATCH 5/7] gpu: drm: meson: simplify using devm_regulator_get_enable_optional()

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 2377 bytes --]

Drop open-coded pattern: 'devm_regulator_get(), regulator_enable(),
add_action_or_reset(regulator_disable)' and use the
devm_regulator_get_enable_optional(). Also drop the seemingly unused
struct member 'hdmi_supply'.

Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
---
 drivers/gpu/drm/meson/meson_dw_hdmi.c | 23 +++--------------------
 1 file changed, 3 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/meson/meson_dw_hdmi.c b/drivers/gpu/drm/meson/meson_dw_hdmi.c
index 5cd2b2ebbbd3..7642f740272b 100644
--- a/drivers/gpu/drm/meson/meson_dw_hdmi.c
+++ b/drivers/gpu/drm/meson/meson_dw_hdmi.c
@@ -140,7 +140,6 @@ struct meson_dw_hdmi {
 	struct reset_control *hdmitx_apb;
 	struct reset_control *hdmitx_ctrl;
 	struct reset_control *hdmitx_phy;
-	struct regulator *hdmi_supply;
 	u32 irq_stat;
 	struct dw_hdmi *hdmi;
 	struct drm_bridge *bridge;
@@ -665,11 +664,6 @@ static void meson_dw_hdmi_init(struct meson_dw_hdmi *meson_dw_hdmi)
 
 }
 
-static void meson_disable_regulator(void *data)
-{
-	regulator_disable(data);
-}
-
 static void meson_disable_clk(void *data)
 {
 	clk_disable_unprepare(data);
@@ -723,20 +717,9 @@ static int meson_dw_hdmi_bind(struct device *dev, struct device *master,
 	meson_dw_hdmi->data = match;
 	dw_plat_data = &meson_dw_hdmi->dw_plat_data;
 
-	meson_dw_hdmi->hdmi_supply = devm_regulator_get_optional(dev, "hdmi");
-	if (IS_ERR(meson_dw_hdmi->hdmi_supply)) {
-		if (PTR_ERR(meson_dw_hdmi->hdmi_supply) == -EPROBE_DEFER)
-			return -EPROBE_DEFER;
-		meson_dw_hdmi->hdmi_supply = NULL;
-	} else {
-		ret = regulator_enable(meson_dw_hdmi->hdmi_supply);
-		if (ret)
-			return ret;
-		ret = devm_add_action_or_reset(dev, meson_disable_regulator,
-					       meson_dw_hdmi->hdmi_supply);
-		if (ret)
-			return ret;
-	}
+	ret = devm_regulator_get_enable_optional(dev, "hdmi");
+	if (ret != -ENODEV)
+		return ret;
 
 	meson_dw_hdmi->hdmitx_apb = devm_reset_control_get_exclusive(dev,
 						"hdmitx_apb");
-- 
2.37.1


-- 
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND

~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
Simon says - in Latin please.
~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
Thanks to Simon Glass for the translation =] 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[RFC PATCH 6/7] hwmon: lm90: simplify using devm_regulator_get_enable()

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 1996 bytes --]

Drop open-coded pattern: 'devm_regulator_get(), regulator_enable(),
add_action_or_reset(regulator_disable)' and use the
devm_regulator_get_enable().

Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
---
 drivers/hwmon/lm90.c | 21 ++-------------------
 1 file changed, 2 insertions(+), 19 deletions(-)

diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
index 3820f0e61510..2ab561ec367c 100644
--- a/drivers/hwmon/lm90.c
+++ b/drivers/hwmon/lm90.c
@@ -1848,12 +1848,6 @@ static void lm90_remove_pec(void *dev)
 	device_remove_file(dev, &dev_attr_pec);
 }
 
-static void lm90_regulator_disable(void *regulator)
-{
-	regulator_disable(regulator);
-}
-
-
 static const struct hwmon_ops lm90_ops = {
 	.is_visible = lm90_is_visible,
 	.read = lm90_read,
@@ -1865,24 +1859,13 @@ static int lm90_probe(struct i2c_client *client)
 	struct device *dev = &client->dev;
 	struct i2c_adapter *adapter = client->adapter;
 	struct hwmon_channel_info *info;
-	struct regulator *regulator;
 	struct device *hwmon_dev;
 	struct lm90_data *data;
 	int err;
 
-	regulator = devm_regulator_get(dev, "vcc");
-	if (IS_ERR(regulator))
-		return PTR_ERR(regulator);
-
-	err = regulator_enable(regulator);
-	if (err < 0) {
-		dev_err(dev, "Failed to enable regulator: %d\n", err);
-		return err;
-	}
-
-	err = devm_add_action_or_reset(dev, lm90_regulator_disable, regulator);
+	err = devm_regulator_get_enable(dev, "vcc");
 	if (err)
-		return err;
+		return dev_err_probe(dev, err, "Failed to enable regulator\n");
 
 	data = devm_kzalloc(dev, sizeof(struct lm90_data), GFP_KERNEL);
 	if (!data)
-- 
2.37.1


-- 
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND

~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
Simon says - in Latin please.
~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
Thanks to Simon Glass for the translation =] 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[RFC PATCH 7/7] adc: ad7192: simplify using devm_regulator_get_enable()

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 1807 bytes --]

Drop open-coded pattern: 'devm_regulator_get(), regulator_enable(),
add_action_or_reset(regulator_disable)' and use the
devm_regulator_get_enable_optional(). Also drop the seemingly unused
struct member 'dvdd'.

Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
---
 drivers/iio/adc/ad7192.c | 15 ++-------------
 1 file changed, 2 insertions(+), 13 deletions(-)

diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c
index d71977be7d22..8a52c0dec3f9 100644
--- a/drivers/iio/adc/ad7192.c
+++ b/drivers/iio/adc/ad7192.c
@@ -177,7 +177,6 @@ struct ad7192_chip_info {
 struct ad7192_state {
 	const struct ad7192_chip_info	*chip_info;
 	struct regulator		*avdd;
-	struct regulator		*dvdd;
 	struct clk			*mclk;
 	u16				int_vref_mv;
 	u32				fclk;
@@ -1015,19 +1014,9 @@ static int ad7192_probe(struct spi_device *spi)
 	if (ret)
 		return ret;
 
-	st->dvdd = devm_regulator_get(&spi->dev, "dvdd");
-	if (IS_ERR(st->dvdd))
-		return PTR_ERR(st->dvdd);
-
-	ret = regulator_enable(st->dvdd);
-	if (ret) {
-		dev_err(&spi->dev, "Failed to enable specified DVdd supply\n");
-		return ret;
-	}
-
-	ret = devm_add_action_or_reset(&spi->dev, ad7192_reg_disable, st->dvdd);
+	ret = devm_regulator_get_enable(&spi->dev, "dvdd");
 	if (ret)
-		return ret;
+		return dev_err_probe(&spi->dev, ret, "Failed to enable specified DVdd supply\n");
 
 	ret = regulator_get_voltage(st->avdd);
 	if (ret < 0) {
-- 
2.37.1


-- 
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND

~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
Simon says - in Latin please.
~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
Thanks to Simon Glass for the translation =] 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC PATCH 2/7] regulator: Add devm helpers for get and enable

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 548 bytes --]

On Wed, Aug 10, 2022 at 02:29:55PM +0300, Matti Vaittinen wrote:
> A few regulator consumer drivers seem to be just getting a regulator,
> enabling it and registering a devm-action to disable the regulator at
> the driver detach and then forget about it.
> 
> We can simplify this a bit by adding a devm-helper for this pattern.
> Add devm_regulator_get_enable() and devm_regulator_get_enable_optional()

I'm really not keen on the idea of a devm managed enable, it's too prone
to bugs when someone gets round to implementing runtime PM.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC PATCH 2/7] regulator: Add devm helpers for get and enable

[Matti Vaittinen]

Hi deee Ho Mark,

Long time no chat. Glad that you had the time to check this series :)

On 8/10/22 14:56, Mark Brown wrote:
> On Wed, Aug 10, 2022 at 02:29:55PM +0300, Matti Vaittinen wrote:
>> A few regulator consumer drivers seem to be just getting a regulator,
>> enabling it and registering a devm-action to disable the regulator at
>> the driver detach and then forget about it.
>>
>> We can simplify this a bit by adding a devm-helper for this pattern.
>> Add devm_regulator_get_enable() and devm_regulator_get_enable_optional()
> 
> I'm really not keen on the idea of a devm managed enable, it's too prone
> to bugs when someone gets round to implementing runtime PM.

I see. And I agree the devm-based regulator disable can cause problems 
when combined with manual disable/enable.

In order to tackle the issue the suggested API does not return handle to 
the regulators - it really just provides the "get'n enable, then forget" 
solution. The consumers who use the suggested API to "devm get'n enable" 
will have had time manually controlling the regulator afterwards as they 
will not get the handle. I would almost claim that the pattern we 
nowadays see (devm_get, enable, add_action_or_reset(disable())) is more 
error prone as users seem to in many case be storing the regulator 
handle w/o any comment about the automated disable at detach.

-- 
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~

Discuss - Estimate - Plan - Report and finally accomplish this:
void do_work(int time) __attribute__ ((const));

Re: [RFC PATCH 6/7] hwmon: lm90: simplify using devm_regulator_get_enable()

[Guenter Roeck]

On 8/10/22 04:32, Matti Vaittinen wrote:
> Drop open-coded pattern: 'devm_regulator_get(), regulator_enable(),
> add_action_or_reset(regulator_disable)' and use the
> devm_regulator_get_enable().
> 
> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>

Acked-by: Guenter Roeck <linux@roeck-us.net>

> ---
>   drivers/hwmon/lm90.c | 21 ++-------------------
>   1 file changed, 2 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
> index 3820f0e61510..2ab561ec367c 100644
> --- a/drivers/hwmon/lm90.c
> +++ b/drivers/hwmon/lm90.c
> @@ -1848,12 +1848,6 @@ static void lm90_remove_pec(void *dev)
>   	device_remove_file(dev, &dev_attr_pec);
>   }
>   
> -static void lm90_regulator_disable(void *regulator)
> -{
> -	regulator_disable(regulator);
> -}
> -
> -
>   static const struct hwmon_ops lm90_ops = {
>   	.is_visible = lm90_is_visible,
>   	.read = lm90_read,
> @@ -1865,24 +1859,13 @@ static int lm90_probe(struct i2c_client *client)
>   	struct device *dev = &client->dev;
>   	struct i2c_adapter *adapter = client->adapter;
>   	struct hwmon_channel_info *info;
> -	struct regulator *regulator;
>   	struct device *hwmon_dev;
>   	struct lm90_data *data;
>   	int err;
>   
> -	regulator = devm_regulator_get(dev, "vcc");
> -	if (IS_ERR(regulator))
> -		return PTR_ERR(regulator);
> -
> -	err = regulator_enable(regulator);
> -	if (err < 0) {
> -		dev_err(dev, "Failed to enable regulator: %d\n", err);
> -		return err;
> -	}
> -
> -	err = devm_add_action_or_reset(dev, lm90_regulator_disable, regulator);
> +	err = devm_regulator_get_enable(dev, "vcc");
>   	if (err)
> -		return err;
> +		return dev_err_probe(dev, err, "Failed to enable regulator\n");
>   
>   	data = devm_kzalloc(dev, sizeof(struct lm90_data), GFP_KERNEL);
>   	if (!data)

Re: [RFC PATCH 2/7] regulator: Add devm helpers for get and enable

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 773 bytes --]

On Wed, Aug 10, 2022 at 03:19:05PM +0300, Matti Vaittinen wrote:

> In order to tackle the issue the suggested API does not return handle to the
> regulators - it really just provides the "get'n enable, then forget"
> solution. The consumers who use the suggested API to "devm get'n enable"
> will have had time manually controlling the regulator afterwards as they
> will not get the handle. I would almost claim that the pattern we nowadays
> see (devm_get, enable, add_action_or_reset(disable())) is more error prone
> as users seem to in many case be storing the regulator handle w/o any
> comment about the automated disable at detach.

Hrm, right - that does help with that case.  However we do need a bulk 
version since that's an obvious problem case.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC PATCH 2/7] regulator: Add devm helpers for get and enable

[Matti Vaittinen]

On 8/10/22 18:16, Mark Brown wrote:
> On Wed, Aug 10, 2022 at 03:19:05PM +0300, Matti Vaittinen wrote:
> 
>> In order to tackle the issue the suggested API does not return handle to the
>> regulators - it really just provides the "get'n enable, then forget"
>> solution. The consumers who use the suggested API to "devm get'n enable"
>> will have had time manually controlling the regulator afterwards as they
>> will not get the handle. I would almost claim that the pattern we nowadays
>> see (devm_get, enable, add_action_or_reset(disable())) is more error prone
>> as users seem to in many case be storing the regulator handle w/o any
>> comment about the automated disable at detach.
> 
> Hrm, right - that does help with that case.  However we do need a bulk
> version since that's an obvious problem case.
I'll take a look at the bulk APIs and add them if they're not too 
complex. I'm a bit short on time as I was told I should be doing 
something we can show to a customer ;)

As a result of this discussion - not returning the handle to struct 
regulator * sounds like a safest option here. I'll drop the RFC when I 
respin this (hopefully with the bulk-APIs and a few more converted drivers)

Thanks for the input!

Best Regards
	Matti Vaittinen

-- 
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~

Discuss - Estimate - Plan - Report and finally accomplish this:
void do_work(int time) __attribute__ ((const));