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

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

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 4094 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.

A few* drivers seem to use pattern demonstrated by pseudocode:

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

Introducing devm helpers for this pattern would remove bunch of code from
drivers. Typically 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.
- remove stored pointer to struct regulator - which can lead to problem
  when an devm action for regulator_disable is used.

I believe this simplifies things by removing some dublicated code.

The suggested managed 'get_enable' APIs do not return the pointer to
regulators for user because any call to regulator_disable()
(or regulator_enable()) may easily 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 these APIs are 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 API does not suit
needs of such users.

This series reowrks only a few drivers as I am short of time. 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!).

Revision history:

RFCv1 => v2:
	- Add devm_regulator_bulk_get_enable() and
	  devm_regulator_bulk_put()
	- Convert a couple of drivers to use the new
	  devm_regulator_bulk_get_enable().
	- Squash all IIO patches into one.

Patch 1:
	Fix docmentation (devres API list) for regulator APIs
Patch 2:
	The new devm helpers.
Patch 3:
	Add new devm-helper APIs to docs.
Patch 4:
	simplified CLK driver(s)
Patch 5:
	simplified GPU driver(s)
Patch 6:
	simplified hwmon driver(s)
Patch 7:
	simplified IIO driver(s)

---

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: simplify drivers using devm_regulator_*get_enable*()
  hwmon: lm90: simplify using devm_regulator_get_enable()
  iio: Simplify drivers using devm_regulator_*get_enable()

 .../driver-api/driver-model/devres.rst        |  11 ++
 drivers/clk/clk-cdce925.c                     |  21 +--
 drivers/gpu/drm/bridge/sii902x.c              |  22 +--
 drivers/gpu/drm/meson/meson_dw_hdmi.c         |  23 +--
 drivers/hwmon/lm90.c                          |  21 +--
 drivers/iio/adc/ad7192.c                      |  15 +-
 drivers/iio/dac/ltc2688.c                     |  23 +--
 drivers/iio/gyro/bmg160_core.c                |  24 +--
 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h       |   2 -
 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c  |  30 +---
 drivers/regulator/devres.c                    | 164 ++++++++++++++++++
 include/linux/regulator/consumer.h            |  27 +++
 12 files changed, 227 insertions(+), 156 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 --]

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

[Matti Vaittinen]

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

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

Add missing functions.

Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>

---
RFCv1 => v2:
- No changes

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

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

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 9186 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>

---
RFCv1 => v2
 - Add devm_regulator_bulk_put() and devm_regulator_bulk_get_enable()
---
 drivers/regulator/devres.c         | 164 +++++++++++++++++++++++++++++
 include/linux/regulator/consumer.h |  27 +++++
 2 files changed, 191 insertions(+)

diff --git a/drivers/regulator/devres.c b/drivers/regulator/devres.c
index 9113233f41cd..ca3d702b0634 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
@@ -166,6 +225,111 @@ int devm_regulator_bulk_get(struct device *dev, int num_consumers,
 }
 EXPORT_SYMBOL_GPL(devm_regulator_bulk_get);
 
+static int devm_regulator_bulk_match(struct device *dev, void *res,
+				     void *data)
+{
+	struct regulator_bulk_devres *match = res;
+	struct regulator_bulk_data *target = data;
+
+	/*
+	 * We check the put uses same consumer list as the get did.
+	 * We _could_ scan all entries in consumer array and check the
+	 * regulators match but ATM I don't see the need. We can change this
+	 * later if needed.
+	 */
+	return match->consumers == target;
+}
+
+/**
+ * devm_regulator_bulk_put - Resource managed regulator_bulk_put()
+ * @consumers: consumers to free
+ *
+ * Deallocate regulators allocated with devm_regulator_bulk_get(). Normally
+ * this function will not need to be called and the resource management
+ * code will ensure that the resource is freed.
+ */
+void devm_regulator_bulk_put(struct regulator_bulk_data *consumers)
+{
+	int rc;
+	struct regulator *regulator = consumers[0].consumer;
+
+	rc = devres_release(regulator->dev, devm_regulator_bulk_release,
+			    devm_regulator_bulk_match, consumers);
+	if (rc != 0)
+		WARN_ON(rc);
+}
+EXPORT_SYMBOL_GPL(devm_regulator_bulk_put);
+
+static void devm_regulator_bulk_disable(void *res)
+{
+	struct regulator_bulk_devres *devres = res;
+	int i;
+
+	for (i = 0; i < devres->num_consumers; i++)
+		regulator_disable(devres->consumers[i].consumer);
+}
+
+/**
+ * devm_regulator_bulk_get_enable - managed get'n enable multiple regulators
+ *
+ * @dev:           device to supply
+ * @num_consumers: number of consumers to register
+ * @id:            list of supply names or regulator IDs
+ *
+ * @return 0 on success, an errno on failure.
+ *
+ * This helper function allows drivers to get several regulator
+ * consumers in one operation with management, the regulators will
+ * automatically be freed when the device is unbound.  If any of the
+ * regulators cannot be acquired then any regulators that were
+ * allocated will be freed before returning to the caller.
+ */
+int devm_regulator_bulk_get_enable(struct device *dev, int num_consumers,
+				   const char * const *id)
+{
+	struct regulator_bulk_devres *devres;
+	struct regulator_bulk_data *consumers;
+	int i, ret;
+
+	devres = devm_kmalloc(dev, sizeof(*devres), GFP_KERNEL);
+	if (!devres)
+		return -ENOMEM;
+
+	devres->consumers = devm_kcalloc(dev, num_consumers, sizeof(*consumers),
+					 GFP_KERNEL);
+	consumers = devres->consumers;
+	if (!consumers)
+		return -ENOMEM;
+
+	devres->num_consumers = num_consumers;
+
+	for (i = 0; i < num_consumers; i++)
+		consumers[i].supply = id[i];
+
+	ret = devm_regulator_bulk_get(dev, num_consumers, consumers);
+	if (ret)
+		return ret;
+
+	for (i = 0; i < num_consumers; i++) {
+		ret = regulator_enable(consumers[i].consumer);
+		if (ret)
+			goto unwind;
+	}
+
+	ret = devm_add_action(dev, devm_regulator_bulk_disable, devres);
+	if (!ret)
+		return 0;
+
+unwind:
+	while (--i >= 0)
+		regulator_disable(consumers[i].consumer);
+
+	devm_regulator_bulk_put(consumers);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(devm_regulator_bulk_get_enable);
+
 static void devm_rdev_release(struct device *dev, void *res)
 {
 	regulator_unregister(*(struct regulator_dev **)res);
diff --git a/include/linux/regulator/consumer.h b/include/linux/regulator/consumer.h
index bbf6590a6dec..24cf5f099eef 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);
 
@@ -240,8 +242,11 @@ int __must_check regulator_bulk_get(struct device *dev, int num_consumers,
 				    struct regulator_bulk_data *consumers);
 int __must_check devm_regulator_bulk_get(struct device *dev, int num_consumers,
 					 struct regulator_bulk_data *consumers);
+void devm_regulator_bulk_put(struct regulator_bulk_data *consumers);
 int __must_check regulator_bulk_enable(int num_consumers,
 				       struct regulator_bulk_data *consumers);
+int devm_regulator_bulk_get_enable(struct device *dev, int num_consumers,
+				   const char * const *id);
 int regulator_bulk_disable(int num_consumers,
 			   struct regulator_bulk_data *consumers);
 int regulator_bulk_force_disable(int num_consumers,
@@ -346,6 +351,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)
 {
@@ -367,6 +383,10 @@ static inline void devm_regulator_put(struct regulator *regulator)
 {
 }
 
+static inline void devm_regulator_bulk_put(struct regulator_bulk_data *consumers)
+{
+}
+
 static inline int regulator_register_supply_alias(struct device *dev,
 						  const char *id,
 						  struct device *alias_dev,
@@ -457,6 +477,13 @@ static inline int regulator_bulk_enable(int num_consumers,
 	return 0;
 }
 
+static inline int devm_regulator_bulk_get_enable(struct device *dev,
+						 int num_consumers,
+						 const char * const *id)
+{
+	return 0;
+}
+
 static inline int regulator_bulk_disable(int num_consumers,
 					 struct regulator_bulk_data *consumers)
 {
-- 
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 --]

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

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 1355 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>

---
RFCv1 => v2:
 - Add devm_regulator_bulk_put() and devm_regulator_bulk_get_enable()
---
 Documentation/driver-api/driver-model/devres.rst | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/driver-api/driver-model/devres.rst b/Documentation/driver-api/driver-model/devres.rst
index 271d1eb2234b..89f87ae613f0 100644
--- a/Documentation/driver-api/driver-model/devres.rst
+++ b/Documentation/driver-api/driver-model/devres.rst
@@ -408,7 +408,11 @@ PWM
 REGULATOR
   devm_regulator_bulk_register_supply_alias()
   devm_regulator_bulk_get()
+  devm_regulator_bulk_get_enable()
+  devm_regulator_bulk_put()
   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 --]

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

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 1924 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>

---
RFCv1 => v2:
- No changes
---
 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 --]

[PATCH v2 5/7] gpu: drm: simplify drivers using devm_regulator_*get_enable*()

[Matti Vaittinen]

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

Simplify drivers using managed "regulator get and enable".

meson:
Use the devm_regulator_get_enable_optional(). Also drop the seemingly
unused struct member 'hdmi_supply'.

sii902x:
Simplify using devm_regulator_bulk_get_enable()

Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>

---
RFCv1 => v2:
- Change also sii902x to use devm_regulator_bulk_get_enable()

Please note - this is only compile-tested due to the lack of HW. Careful
review and testing is _highly_ appreciated.
---
 drivers/gpu/drm/bridge/sii902x.c      | 22 +++-------------------
 drivers/gpu/drm/meson/meson_dw_hdmi.c | 23 +++--------------------
 2 files changed, 6 insertions(+), 39 deletions(-)

diff --git a/drivers/gpu/drm/bridge/sii902x.c b/drivers/gpu/drm/bridge/sii902x.c
index 65549fbfdc87..4bf572b7ca77 100644
--- a/drivers/gpu/drm/bridge/sii902x.c
+++ b/drivers/gpu/drm/bridge/sii902x.c
@@ -170,7 +170,6 @@ struct sii902x {
 	struct drm_connector connector;
 	struct gpio_desc *reset_gpio;
 	struct i2c_mux_core *i2cmux;
-	struct regulator_bulk_data supplies[2];
 	bool sink_is_hdmi;
 	/*
 	 * Mutex protects audio and video functions from interfering
@@ -1070,6 +1069,7 @@ static int sii902x_probe(struct i2c_client *client,
 	struct device *dev = &client->dev;
 	struct device_node *endpoint;
 	struct sii902x *sii902x;
+	static const char * const supplies[] = {"iovcc", "cvcc12"};
 	int ret;
 
 	ret = i2c_check_functionality(client->adapter,
@@ -1120,27 +1120,13 @@ static int sii902x_probe(struct i2c_client *client,
 
 	mutex_init(&sii902x->mutex);
 
-	sii902x->supplies[0].supply = "iovcc";
-	sii902x->supplies[1].supply = "cvcc12";
-	ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(sii902x->supplies),
-				      sii902x->supplies);
-	if (ret < 0)
-		return ret;
-
-	ret = regulator_bulk_enable(ARRAY_SIZE(sii902x->supplies),
-				    sii902x->supplies);
+	ret = devm_regulator_bulk_get_enable(dev, ARRAY_SIZE(supplies), supplies);
 	if (ret < 0) {
 		dev_err_probe(dev, ret, "Failed to enable supplies");
 		return ret;
 	}
 
-	ret = sii902x_init(sii902x);
-	if (ret < 0) {
-		regulator_bulk_disable(ARRAY_SIZE(sii902x->supplies),
-				       sii902x->supplies);
-	}
-
-	return ret;
+	return sii902x_init(sii902x);
 }
 
 static int sii902x_remove(struct i2c_client *client)
@@ -1150,8 +1136,6 @@ static int sii902x_remove(struct i2c_client *client)
 
 	i2c_mux_del_adapters(sii902x->i2cmux);
 	drm_bridge_remove(&sii902x->bridge);
-	regulator_bulk_disable(ARRAY_SIZE(sii902x->supplies),
-			       sii902x->supplies);
 
 	return 0;
 }
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 --]

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

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 2077 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>
Acked-by: Guenter Roeck <linux@roeck-us.net>

---
RFCv1 => v2:
- No changes
---
 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 --]

[PATCH v2 7/7] iio: Simplify drivers using devm_regulator_*get_enable()

[Matti Vaittinen]

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

adc/ad7192:
Use devm_regulator_get_enable() instead of open coded get, enable,
add-action-to-disable-at-detach - pattern. Also drop the seemingly unused
struct member 'dvdd'.

dac/ltc2688:
gyro/bmg160_core:
Use devm_regulator_bulk_get_enable() instead of open coded bulk-get,
bulk-enable, add-action-to-disable-at-detach - pattern.

imu/st_lsm6dsx:
Use devm_regulator_bulk_get_enable() instead of open coded bulk-get,
bulk-enable, add-action-to-disable-at-detach - pattern.

A functional change (which seems like a bugfix) is that if
regulator_bulk_get fails, the enable is not attempted.

Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>

---
RFCv1 => v2:
Squashed all IIO changes to one patch. Added use of
devm_regulator_bulk_get_enable().

Please note - this is only compile-tested due to the lack of HW. Careful
review and testing is _highly_ appreciated.
---
 drivers/iio/adc/ad7192.c                     | 15 ++--------
 drivers/iio/dac/ltc2688.c                    | 23 ++-------------
 drivers/iio/gyro/bmg160_core.c               | 24 ++--------------
 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h      |  2 --
 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 30 ++++----------------
 5 files changed, 13 insertions(+), 81 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) {
diff --git a/drivers/iio/dac/ltc2688.c b/drivers/iio/dac/ltc2688.c
index 937b0d25a11c..1abc88cd99f9 100644
--- a/drivers/iio/dac/ltc2688.c
+++ b/drivers/iio/dac/ltc2688.c
@@ -84,7 +84,6 @@ struct ltc2688_chan {
 struct ltc2688_state {
 	struct spi_device *spi;
 	struct regmap *regmap;
-	struct regulator_bulk_data regulators[2];
 	struct ltc2688_chan channels[LTC2688_DAC_CHANNELS];
 	struct iio_chan_spec *iio_chan;
 	/* lock to protect against multiple access to the device and shared data */
@@ -902,13 +901,6 @@ static int ltc2688_setup(struct ltc2688_state *st, struct regulator *vref)
 			       LTC2688_CONFIG_EXT_REF);
 }
 
-static void ltc2688_disable_regulators(void *data)
-{
-	struct ltc2688_state *st = data;
-
-	regulator_bulk_disable(ARRAY_SIZE(st->regulators), st->regulators);
-}
-
 static void ltc2688_disable_regulator(void *regulator)
 {
 	regulator_disable(regulator);
@@ -970,6 +962,7 @@ static int ltc2688_probe(struct spi_device *spi)
 	struct regulator *vref_reg;
 	struct device *dev = &spi->dev;
 	int ret;
+	static const char * const regulators[] = {"vcc", "iovcc"};
 
 	indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
 	if (!indio_dev)
@@ -988,21 +981,11 @@ static int ltc2688_probe(struct spi_device *spi)
 		return dev_err_probe(dev, PTR_ERR(st->regmap),
 				     "Failed to init regmap");
 
-	st->regulators[0].supply = "vcc";
-	st->regulators[1].supply = "iovcc";
-	ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(st->regulators),
-				      st->regulators);
-	if (ret)
-		return dev_err_probe(dev, ret, "Failed to get regulators\n");
-
-	ret = regulator_bulk_enable(ARRAY_SIZE(st->regulators), st->regulators);
+	ret = devm_regulator_bulk_get_enable(dev, ARRAY_SIZE(regulators),
+					     regulators);
 	if (ret)
 		return dev_err_probe(dev, ret, "Failed to enable regulators\n");
 
-	ret = devm_add_action_or_reset(dev, ltc2688_disable_regulators, st);
-	if (ret)
-		return ret;
-
 	vref_reg = devm_regulator_get_optional(dev, "vref");
 	if (IS_ERR(vref_reg)) {
 		if (PTR_ERR(vref_reg) != -ENODEV)
diff --git a/drivers/iio/gyro/bmg160_core.c b/drivers/iio/gyro/bmg160_core.c
index 81a6d09788bd..acfabac645b6 100644
--- a/drivers/iio/gyro/bmg160_core.c
+++ b/drivers/iio/gyro/bmg160_core.c
@@ -93,7 +93,6 @@
 
 struct bmg160_data {
 	struct regmap *regmap;
-	struct regulator_bulk_data regulators[2];
 	struct iio_trigger *dready_trig;
 	struct iio_trigger *motion_trig;
 	struct iio_mount_matrix orientation;
@@ -1067,19 +1066,13 @@ static const char *bmg160_match_acpi_device(struct device *dev)
 	return dev_name(dev);
 }
 
-static void bmg160_disable_regulators(void *d)
-{
-	struct bmg160_data *data = d;
-
-	regulator_bulk_disable(ARRAY_SIZE(data->regulators), data->regulators);
-}
-
 int bmg160_core_probe(struct device *dev, struct regmap *regmap, int irq,
 		      const char *name)
 {
 	struct bmg160_data *data;
 	struct iio_dev *indio_dev;
 	int ret;
+	static const char * const regulators[] = {"vdd", "vddio"};
 
 	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
 	if (!indio_dev)
@@ -1090,22 +1083,11 @@ int bmg160_core_probe(struct device *dev, struct regmap *regmap, int irq,
 	data->irq = irq;
 	data->regmap = regmap;
 
-	data->regulators[0].supply = "vdd";
-	data->regulators[1].supply = "vddio";
-	ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(data->regulators),
-				      data->regulators);
+	ret = devm_regulator_bulk_get_enable(dev, ARRAY_SIZE(regulators),
+					     regulators);
 	if (ret)
 		return dev_err_probe(dev, ret, "Failed to get regulators\n");
 
-	ret = regulator_bulk_enable(ARRAY_SIZE(data->regulators),
-				    data->regulators);
-	if (ret)
-		return ret;
-
-	ret = devm_add_action_or_reset(dev, bmg160_disable_regulators, data);
-	if (ret)
-		return ret;
-
 	ret = iio_read_mount_matrix(dev, &data->orientation);
 	if (ret)
 		return ret;
diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
index a86dd29a4738..03238c64c777 100644
--- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
+++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
@@ -372,7 +372,6 @@ struct st_lsm6dsx_sensor {
  * struct st_lsm6dsx_hw - ST IMU MEMS hw instance
  * @dev: Pointer to instance of struct device (I2C or SPI).
  * @regmap: Register map of the device.
- * @regulators: VDD/VDDIO voltage regulators.
  * @irq: Device interrupt line (I2C or SPI).
  * @fifo_lock: Mutex to prevent concurrent access to the hw FIFO.
  * @conf_lock: Mutex to prevent concurrent FIFO configuration update.
@@ -395,7 +394,6 @@ struct st_lsm6dsx_sensor {
 struct st_lsm6dsx_hw {
 	struct device *dev;
 	struct regmap *regmap;
-	struct regulator_bulk_data regulators[2];
 	int irq;
 
 	struct mutex fifo_lock;
diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
index 910397716833..7b40f6b58834 100644
--- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
+++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
@@ -2172,36 +2172,20 @@ static int st_lsm6dsx_irq_setup(struct st_lsm6dsx_hw *hw)
 
 static int st_lsm6dsx_init_regulators(struct device *dev)
 {
-	struct st_lsm6dsx_hw *hw = dev_get_drvdata(dev);
 	int err;
-
 	/* vdd-vddio power regulators */
-	hw->regulators[0].supply = "vdd";
-	hw->regulators[1].supply = "vddio";
-	err = devm_regulator_bulk_get(dev, ARRAY_SIZE(hw->regulators),
-				      hw->regulators);
-	if (err)
-		return dev_err_probe(dev, err, "failed to get regulators\n");
+	static const char * const regulators[] = {"vdd", "vddio"};
 
-	err = regulator_bulk_enable(ARRAY_SIZE(hw->regulators),
-				    hw->regulators);
-	if (err) {
-		dev_err(dev, "failed to enable regulators: %d\n", err);
-		return err;
-	}
+	err = devm_regulator_bulk_get_enable(dev, ARRAY_SIZE(regulators),
+					     regulators);
+	if (err)
+		return dev_err_probe(dev, err, "failed to enable regulators\n");
 
 	msleep(50);
 
 	return 0;
 }
 
-static void st_lsm6dsx_chip_uninit(void *data)
-{
-	struct st_lsm6dsx_hw *hw = data;
-
-	regulator_bulk_disable(ARRAY_SIZE(hw->regulators), hw->regulators);
-}
-
 int st_lsm6dsx_probe(struct device *dev, int irq, int hw_id,
 		     struct regmap *regmap)
 {
@@ -2225,10 +2209,6 @@ int st_lsm6dsx_probe(struct device *dev, int irq, int hw_id,
 	if (err)
 		return err;
 
-	err = devm_add_action_or_reset(dev, st_lsm6dsx_chip_uninit, hw);
-	if (err)
-		return err;
-
 	hw->buff = devm_kzalloc(dev, ST_LSM6DSX_BUFF_SIZE, GFP_KERNEL);
 	if (!hw->buff)
 		return -ENOMEM;
-- 
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: [PATCH v2 7/7] iio: Simplify drivers using devm_regulator_*get_enable()

[Matti Vaittinen]

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

Morning Andy,

On Fri, Aug 12, 2022 at 09:05:52PM +0200, Andy Shevchenko wrote:
> On Friday, August 12, 2022, Matti Vaittinen <mazziesaccount@gmail.com>
> wrote:
> 
> > adc/ad7192:
> > Use devm_regulator_get_enable() instead of open coded get, enable,
> > add-action-to-disable-at-detach - pattern. Also drop the seemingly unused
> > struct member 'dvdd'.
> 
> In IIO we expect to have one patch per driver. Please split.

Fine with me. I didn't go through too many drivers anyways. I'll split
for the v3. I'll just wait for a while for other feedback.

Best Regards
  Matti

-- 
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: (subset) [PATCH v2 0/7] Devm helpers for regulator get and enable

[Laurent Pinchart]

On Mon, Aug 15, 2022 at 04:44:44PM +0100, Mark Brown wrote:
> On Fri, 12 Aug 2022 13:08:17 +0300, Matti Vaittinen wrote:
> > 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.
> > 
> > A few* drivers seem to use pattern demonstrated by pseudocode:
> > 
> > [...]
> 
> Applied to
> 
>    https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git for-next
> 
> Thanks!
> 
> [1/7] docs: devres: regulator: Add missing devm_* functions to devres.rst
>       commit: 9b6744f60b6b47bc0757a1955adb4d2c3ab22e13
> [2/7] regulator: Add devm helpers for get and enable
>       (no commit info)

I didn't have time to reply to the series yet, but I think this isn't a
great idea. There are two issues:

- With devres, you don't have full control over the order in which
  resources will be released, which means that you can't control the
  power off sequence, in particular if it needs to be sequenced with
  GPIOs and clocks. That's not a concern for all drivers, but this API
  will creep in in places where it shouldn't be used, driver authours
  should really pay attention to power management and not live with the
  false impression that everything will be handled automatically for
  them. In the worst cases, an incorrect power off sequence could lead
  to hardware damage.

- Powering regulators on at probe time and leaving them on is a very bad
  practice from a power management point of view, and should really be
  discouraged. Adding convenience helpers to make this easy is the wrong
  message, we should instead push driver authors to implement proper
  runtime PM.

> All being well this means that it will be integrated into the linux-next
> tree (usually sometime in the next 24 hours) and sent to Linus during
> the next merge window (or sooner if it is a bug fix), however if
> problems are discovered then the patch may be dropped or reverted.
> 
> You may get further e-mails resulting from automated or manual testing
> and review of the tree, please engage with people reporting problems and
> send followup patches addressing any issues that are reported if needed.
> 
> If any updates are required or you are submitting further changes they
> should be sent as incremental updates against current git, existing
> patches will not be replaced.
> 
> Please add any relevant lists and maintainers to the CCs when replying
> to this mail.

-- 
Regards,

Laurent Pinchart

Re: (subset) [PATCH v2 0/7] Devm helpers for regulator get and enable

[Mark Brown]

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

On Mon, Aug 15, 2022 at 06:54:45PM +0300, Laurent Pinchart wrote:

> - With devres, you don't have full control over the order in which
>   resources will be released, which means that you can't control the
>   power off sequence, in particular if it needs to be sequenced with
>   GPIOs and clocks. That's not a concern for all drivers, but this API
>   will creep in in places where it shouldn't be used, driver authours
>   should really pay attention to power management and not live with the
>   false impression that everything will be handled automatically for
>   them. In the worst cases, an incorrect power off sequence could lead
>   to hardware damage.

I basically agree with these concerns which is why I was only happy with
this API when Matti suggested doing it in a way that meant that the
callers are unable to access the regulator at runtime, this means that
if anyone wants to do any kind of management of the power state outside
of probe and remove they are forced to convert to the full fat APIs.
The general ordering concern with devm is that the free happens too late
but for the most part this isn't such a concern with regulators, they
might have delayed power off anyway due to sharing - it's no worse than
memory allocation AFAICT.  Given all the other APIs using devm it's
probably going to end up fixing some bugs.

For sequencing I'm not convinced it's much worse than the bulk API is
anyway, and practically speaking I expect most devices that have
problems here will also need more control over power anyway - it's
certainly the common case that hardware has pretty basic requirements
and is fairly tolerant.

> - Powering regulators on at probe time and leaving them on is a very bad
>   practice from a power management point of view, and should really be
>   discouraged. Adding convenience helpers to make this easy is the wrong
>   message, we should instead push driver authors to implement proper
>   runtime PM.

The stick simply isn't working here as far as I can see.

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

Re: (subset) [PATCH v2 0/7] Devm helpers for regulator get and enable

[Laurent Pinchart]

Hi Mark,

On Mon, Aug 15, 2022 at 05:33:06PM +0100, Mark Brown wrote:
> On Mon, Aug 15, 2022 at 06:54:45PM +0300, Laurent Pinchart wrote:
> 
> > - With devres, you don't have full control over the order in which
> >   resources will be released, which means that you can't control the
> >   power off sequence, in particular if it needs to be sequenced with
> >   GPIOs and clocks. That's not a concern for all drivers, but this API
> >   will creep in in places where it shouldn't be used, driver authours
> >   should really pay attention to power management and not live with the
> >   false impression that everything will be handled automatically for
> >   them. In the worst cases, an incorrect power off sequence could lead
> >   to hardware damage.
> 
> I basically agree with these concerns which is why I was only happy with
> this API when Matti suggested doing it in a way that meant that the
> callers are unable to access the regulator at runtime, this means that
> if anyone wants to do any kind of management of the power state outside
> of probe and remove they are forced to convert to the full fat APIs.
> The general ordering concern with devm is that the free happens too late
> but for the most part this isn't such a concern with regulators, they
> might have delayed power off anyway due to sharing - it's no worse than
> memory allocation AFAICT.  Given all the other APIs using devm it's
> probably going to end up fixing some bugs.
> 
> For sequencing I'm not convinced it's much worse than the bulk API is
> anyway, and practically speaking I expect most devices that have
> problems here will also need more control over power anyway - it's
> certainly the common case that hardware has pretty basic requirements
> and is fairly tolerant.

I'm not extremely concerned here at the moment, as power should be the
last thing to be turned off, after clocks and reset signals. As clocks
and GPIOs will still be controlled manually in the driver .remove()
function, it means that power will go last, which should be fine.
However, should a devm_clk_get_enable() or similar function be
implemented, we'll run into trouble. Supplying active high input signals
to a device that is not powered can lead to latch-up, which tends to
only manifest after a statistically significant number of occurrences of
the condition, and can slowly damage the hardware over time. This is a
real concern as it will typically not be caught during early
development. I think we would still be better off with requiring drivers
to manually handle powering off the device until we provide a mechanism
that can do so safely in an automated way.

> > - Powering regulators on at probe time and leaving them on is a very bad
> >   practice from a power management point of view, and should really be
> >   discouraged. Adding convenience helpers to make this easy is the wrong
> >   message, we should instead push driver authors to implement proper
> >   runtime PM.
> 
> The stick simply isn't working here as far as I can see.

Do you think there's no way we can get it to work, instead of giving up
and adding an API that goes in the wrong direction ? :-( I'll give a
talk about the dangers of devm_* at the kernel summit, this is something
I can mention to raise awareness of the issue among maintainers,
hopefully leading to improvements through better reviews.

-- 
Regards,

Laurent Pinchart

Re: (subset) [PATCH v2 0/7] Devm helpers for regulator get and enable

[Stephen Boyd]

Quoting Laurent Pinchart (2022-08-15 11:52:36)
> Hi Mark,
> 
> On Mon, Aug 15, 2022 at 05:33:06PM +0100, Mark Brown wrote:
> > On Mon, Aug 15, 2022 at 06:54:45PM +0300, Laurent Pinchart wrote:
> > 
> > > - With devres, you don't have full control over the order in which
> > >   resources will be released, which means that you can't control the
> > >   power off sequence, in particular if it needs to be sequenced with
> > >   GPIOs and clocks. That's not a concern for all drivers, but this API
> > >   will creep in in places where it shouldn't be used, driver authours
> > >   should really pay attention to power management and not live with the
> > >   false impression that everything will be handled automatically for
> > >   them. In the worst cases, an incorrect power off sequence could lead
> > >   to hardware damage.

I think the main issue is that platform drivers are being asked to do
too much. We've put the burden on platform driver authors to intimately
understand how their devices are integrated, and as we all know they're
not very interested in these details because they already have a hard
time to write a driver just to make their latest gizmo whir. Throw in
power management and you get these wrappers that try to compartmentalize
power management logic away from the main part of the driver that's
plugging into the driver subsystem because the SoC integration logic is
constantly changing but the device core isn't.

We need to enhance the platform bus layer to make it SoC aware when the
platform device is inside an SoC, or "board" aware when the device lives
outside of an SoC, i.e. it's a discrete IC. The bus layer should manage
power state transitions for the platform devices, and the platform
drivers should only be able to request runtime power/performance state
changes through device PM APIs (dev_pm_*). If this can all be done
through genpds then it sounds great. We may need to write some generic
code for discrete ICs that enables regulators and then clks before
muxing out pins or something like that. Obviously, I don't have all the
details figured out.

The basic idea is that drivers should be focused on what they're
driving, not navigating the (sometimes) complex integration that's
taking place around them. When a device driver probe function is called
the device should already be powered on. When the driver is
removed/unbound, the power should be removed after the driver's remove
function is called. We're only going to be able to solve the power
sequencing and ordering problem by taking away power control and
sequencing from drivers.

> > 
> > I basically agree with these concerns which is why I was only happy with
> > this API when Matti suggested doing it in a way that meant that the
> > callers are unable to access the regulator at runtime, this means that
> > if anyone wants to do any kind of management of the power state outside
> > of probe and remove they are forced to convert to the full fat APIs.
> > The general ordering concern with devm is that the free happens too late
> > but for the most part this isn't such a concern with regulators, they
> > might have delayed power off anyway due to sharing - it's no worse than
> > memory allocation AFAICT.  Given all the other APIs using devm it's
> > probably going to end up fixing some bugs.
> > 
> > For sequencing I'm not convinced it's much worse than the bulk API is
> > anyway, and practically speaking I expect most devices that have
> > problems here will also need more control over power anyway - it's
> > certainly the common case that hardware has pretty basic requirements
> > and is fairly tolerant.
> 
> I'm not extremely concerned here at the moment, as power should be the
> last thing to be turned off, after clocks and reset signals. As clocks
> and GPIOs will still be controlled manually in the driver .remove()
> function, it means that power will go last, which should be fine.
> However, should a devm_clk_get_enable() or similar function be

This API is implemented now.

> implemented, we'll run into trouble. Supplying active high input signals
> to a device that is not powered can lead to latch-up, which tends to
> only manifest after a statistically significant number of occurrences of
> the condition, and can slowly damage the hardware over time. This is a
> real concern as it will typically not be caught during early
> development. I think we would still be better off with requiring drivers
> to manually handle powering off the device until we provide a mechanism
> that can do so safely in an automated way.

Can you describe the error scenario further? I think it's driver author
error that would lead to getting and enabling the regulator after
getting and enabling a clk that drives out a clock signal on some pins
that aren't powered yet. I'm not sure that's all that much easier to do
with these sorts of devm APIs, but if it is then I'm concerned.

> 
> > > - Powering regulators on at probe time and leaving them on is a very bad
> > >   practice from a power management point of view, and should really be
> > >   discouraged. Adding convenience helpers to make this easy is the wrong
> > >   message, we should instead push driver authors to implement proper
> > >   runtime PM.
> > 
> > The stick simply isn't working here as far as I can see.
> 
> Do you think there's no way we can get it to work, instead of giving up
> and adding an API that goes in the wrong direction ? :-( I'll give a
> talk about the dangers of devm_* at the kernel summit, this is something
> I can mention to raise awareness of the issue among maintainers,
> hopefully leading to improvements through better reviews.
> 

I agree with Mark, the stick isn't working. We discussed these exact
same issues for years with the devm clk get APIs. Search the archives.

Re: (subset) [PATCH v2 0/7] Devm helpers for regulator get and enable

[Laurent Pinchart]

Hi Stephan,

On Mon, Aug 15, 2022 at 01:58:55PM -0700, Stephen Boyd wrote:
> Quoting Laurent Pinchart (2022-08-15 11:52:36)
> > On Mon, Aug 15, 2022 at 05:33:06PM +0100, Mark Brown wrote:
> > > On Mon, Aug 15, 2022 at 06:54:45PM +0300, Laurent Pinchart wrote:
> > > 
> > > > - With devres, you don't have full control over the order in which
> > > >   resources will be released, which means that you can't control the
> > > >   power off sequence, in particular if it needs to be sequenced with
> > > >   GPIOs and clocks. That's not a concern for all drivers, but this API
> > > >   will creep in in places where it shouldn't be used, driver authours
> > > >   should really pay attention to power management and not live with the
> > > >   false impression that everything will be handled automatically for
> > > >   them. In the worst cases, an incorrect power off sequence could lead
> > > >   to hardware damage.
> 
> I think the main issue is that platform drivers are being asked to do
> too much. We've put the burden on platform driver authors to intimately
> understand how their devices are integrated, and as we all know they're
> not very interested in these details because they already have a hard
> time to write a driver just to make their latest gizmo whir. Throw in
> power management and you get these wrappers that try to compartmentalize
> power management logic away from the main part of the driver that's
> plugging into the driver subsystem because the SoC integration logic is
> constantly changing but the device core isn't.
> 
> We need to enhance the platform bus layer to make it SoC aware when the
> platform device is inside an SoC, or "board" aware when the device lives
> outside of an SoC, i.e. it's a discrete IC. The bus layer should manage
> power state transitions for the platform devices, and the platform
> drivers should only be able to request runtime power/performance state
> changes through device PM APIs (dev_pm_*). If this can all be done
> through genpds then it sounds great. We may need to write some generic
> code for discrete ICs that enables regulators and then clks before
> muxing out pins or something like that. Obviously, I don't have all the
> details figured out.
> 
> The basic idea is that drivers should be focused on what they're
> driving, not navigating the (sometimes) complex integration that's
> taking place around them. When a device driver probe function is called
> the device should already be powered on.

No. ACPI does that in many cases, and that's a real bad idea. There are
devices that you do *not* want to power up on probe. I'm thinking, for
example, about camera sensors that have a privacy LED that will light up
when the sensor is powered up. You don't want it to flash on boot. There
are also other use cases related to fault tolerance where you want
drivers to initialize properly and only access the device later.

> When the driver is
> removed/unbound, the power should be removed after the driver's remove
> function is called. We're only going to be able to solve the power
> sequencing and ordering problem by taking away power control and
> sequencing from drivers.

For SoC devices we may be able to achieve this to some extent, at least
for simple devices that don't have exotic requirements (by exotic I mean
requiring more than one clock for instance, so it's not *that* exotic).
For on-board devices that's impossible by nature, the power up/down
constraints are specific to the device, it's the job of the driver to
handle them, the same way it has to handle everything else that is
device-specific.

The right way to handle this in my opinion is to push for RPM support in
drivers. The regulator, reset, clock & co. handling then happens in the
RPM suspend and resume handlers, they're self-contained, well ordered,
and much easier to review. The rest of the driver then only uses the RPM
API. It's easy(er) and clean (at least when you don't throw ACPI to the
mix, with its "peculiar" idea of calling probe with the device powered
on *BUT* in the RPM suspended state - due to historical reasons I
believe - but I think that should be fixable on the ACPI side, albeit
perhaps not without substantial effort), and you get simple review
rules: if the driver doesn't implement RPM, or handles resource
enable/disable outside of the RPM suspend/resume handlers, it's most
likely getting it wrong.

These devres helpers go in the exact opposite direction of what we
should be doing, by telling driver authors it's totally fine to not
implement power management. Why don't we just drop error handling and go
back to the big kernel lock in that case ? That was much easier to
program too.

> > > I basically agree with these concerns which is why I was only happy with
> > > this API when Matti suggested doing it in a way that meant that the
> > > callers are unable to access the regulator at runtime, this means that
> > > if anyone wants to do any kind of management of the power state outside
> > > of probe and remove they are forced to convert to the full fat APIs.
> > > The general ordering concern with devm is that the free happens too late
> > > but for the most part this isn't such a concern with regulators, they
> > > might have delayed power off anyway due to sharing - it's no worse than
> > > memory allocation AFAICT.  Given all the other APIs using devm it's
> > > probably going to end up fixing some bugs.
> > > 
> > > For sequencing I'm not convinced it's much worse than the bulk API is
> > > anyway, and practically speaking I expect most devices that have
> > > problems here will also need more control over power anyway - it's
> > > certainly the common case that hardware has pretty basic requirements
> > > and is fairly tolerant.
> > 
> > I'm not extremely concerned here at the moment, as power should be the
> > last thing to be turned off, after clocks and reset signals. As clocks
> > and GPIOs will still be controlled manually in the driver .remove()
> > function, it means that power will go last, which should be fine.
> > However, should a devm_clk_get_enable() or similar function be
> 
> This API is implemented now.

:-( We're really going straight into the wall.

> > implemented, we'll run into trouble. Supplying active high input signals
> > to a device that is not powered can lead to latch-up, which tends to
> > only manifest after a statistically significant number of occurrences of
> > the condition, and can slowly damage the hardware over time. This is a
> > real concern as it will typically not be caught during early
> > development. I think we would still be better off with requiring drivers
> > to manually handle powering off the device until we provide a mechanism
> > that can do so safely in an automated way.
> 
> Can you describe the error scenario further? I think it's driver author
> error that would lead to getting and enabling the regulator after
> getting and enabling a clk that drives out a clock signal on some pins
> that aren't powered yet. I'm not sure that's all that much easier to do
> with these sorts of devm APIs, but if it is then I'm concerned.

You will very quickly see drivers doing this (either directly or
indirectly):

probe()
{
	devm_clk_get_enabled();
	devm_regulator_get_enable();
}

Without a devres-based get+enable API drivers can get the resources they
need in any order, possibly moving some of those resource acquisition
operations to different functions, and then have a clear block of code
that enables the resources in the right order. These devres helpers give
a false sense of security to driver authors and they will end up
introducing problems, the same way that devm_kzalloc() makes it
outrageously easy to crash the kernel by disconnecting a device that is
in use.

> > > > - Powering regulators on at probe time and leaving them on is a very bad
> > > >   practice from a power management point of view, and should really be
> > > >   discouraged. Adding convenience helpers to make this easy is the wrong
> > > >   message, we should instead push driver authors to implement proper
> > > >   runtime PM.
> > > 
> > > The stick simply isn't working here as far as I can see.
> > 
> > Do you think there's no way we can get it to work, instead of giving up
> > and adding an API that goes in the wrong direction ? :-( I'll give a
> > talk about the dangers of devm_* at the kernel summit, this is something
> > I can mention to raise awareness of the issue among maintainers,
> > hopefully leading to improvements through better reviews.
> 
> I agree with Mark, the stick isn't working. We discussed these exact
> same issues for years with the devm clk get APIs. Search the archives.

And the conclusion was to just give up and do nothing ? :-(

-- 
Regards,

Laurent Pinchart

Re: (subset) [PATCH v2 0/7] Devm helpers for regulator get and enable

[Mark Brown]

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

On Mon, Aug 15, 2022 at 01:58:55PM -0700, Stephen Boyd wrote:
> Quoting Laurent Pinchart (2022-08-15 11:52:36)
> > On Mon, Aug 15, 2022 at 05:33:06PM +0100, Mark Brown wrote:
> > > On Mon, Aug 15, 2022 at 06:54:45PM +0300, Laurent Pinchart wrote:

> > > > - With devres, you don't have full control over the order in which
> > > >   resources will be released, which means that you can't control the
> > > >   power off sequence, in particular if it needs to be sequenced with
> > > >   GPIOs and clocks. That's not a concern for all drivers, but this API
> > > >   will creep in in places where it shouldn't be used, driver authours
> > > >   should really pay attention to power management and not live with the
> > > >   false impression that everything will be handled automatically for
> > > >   them. In the worst cases, an incorrect power off sequence could lead
> > > >   to hardware damage.

> I think the main issue is that platform drivers are being asked to do
> too much. We've put the burden on platform driver authors to intimately
> understand how their devices are integrated, and as we all know they're

This is for the regulator API, it's mainly for off SoC devices so it's
not a question of understanding the integration of a device into a piece
of silicon, it's a question of understanding the integration of a chip
into a board which seems reasonably in scope for a chip driver and is
certainly the sort of thing that you'd be talking to your customers
about as a silicon vendor.

> The basic idea is that drivers should be focused on what they're
> driving, not navigating the (sometimes) complex integration that's
> taking place around them. When a device driver probe function is called
> the device should already be powered on. When the driver is
> removed/unbound, the power should be removed after the driver's remove
> function is called. We're only going to be able to solve the power
> sequencing and ordering problem by taking away power control and
> sequencing from drivers.

That is a sensible approach for most on SoC things but for something
shipped as a separate driver there's little point in separating the
power and clocking domain driver from the device since there's typically
a 1:1 mapping.  Usually either it's extremely simple (eg, turn
everything on and remove reset) but some devices really need to manage
things.  There's obviously some edge cases in SoC integration as well
(eg, the need to manage card supplies for SD controllers, or knowing
exact clock rates for things like audio controllers) so you need some
flex.

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

Re: (subset) [PATCH v2 0/7] Devm helpers for regulator get and enable

[Mark Brown]

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

On Tue, Aug 16, 2022 at 12:17:17AM +0300, Laurent Pinchart wrote:
> On Mon, Aug 15, 2022 at 01:58:55PM -0700, Stephen Boyd wrote:

> > The basic idea is that drivers should be focused on what they're
> > driving, not navigating the (sometimes) complex integration that's
> > taking place around them. When a device driver probe function is called
> > the device should already be powered on.

> No. ACPI does that in many cases, and that's a real bad idea. There are
> devices that you do *not* want to power up on probe. I'm thinking, for
> example, about camera sensors that have a privacy LED that will light up
> when the sensor is powered up. You don't want it to flash on boot. There
> are also other use cases related to fault tolerance where you want
> drivers to initialize properly and only access the device later.

I don't think it's an either/or thing in terms of approach here - we
need a range of options to choose from.  ACPI is totally fine and solves
real problems for the systems it targets, the problems we see with it
are mainly that it has a very strong system abstraction and doesn't cope
well when things go outside that coupled with the fact that Windows long
ago decided that board files were totally fine for papering over any
problems so people haven't worked on standardisation where they should.
Some SoCs like to do similar things with their power controller cores.

Conversely for example with many (but not all) SoC IPs the mechanics of
the system integration and range of options available are such that
dealing with them is kind of out of scope of the driver, but they're
often very repetitive over any given SoC so there is a benefit in
pushing things into power domains rather than having the driver for the
IP manage everything.  We need to be able to be flexible so we can find
the best idioms to represent the different systems in front of us rather
than trying to force all systems into a single idiom.

> These devres helpers go in the exact opposite direction of what we
> should be doing, by telling driver authors it's totally fine to not
> implement power management. Why don't we just drop error handling and go
> back to the big kernel lock in that case ? That was much easier to
> program too.

Sometimes it's totally fine to not worry, at least at a first pass.
Perhaps you're more concerned with real time, perhaps your system
doesn't provide control for the relevant resources.  Sometimes the
savings are so negligable that it's questionable if doing the power
manageement is an overall power saving.

> You will very quickly see drivers doing this (either directly or
> indirectly):

> probe()
> {
> 	devm_clk_get_enabled();
> 	devm_regulator_get_enable();
> }

> Without a devres-based get+enable API drivers can get the resources they
> need in any order, possibly moving some of those resource acquisition
> operations to different functions, and then have a clear block of code
> that enables the resources in the right order. These devres helpers give
> a false sense of security to driver authors and they will end up
> introducing problems, the same way that devm_kzalloc() makes it
> outrageously easy to crash the kernel by disconnecting a device that is
> in use.

TBH I think the problem you have here is with devm not with this
particular function.  That's a different conversation, and a totally
valid one especially when you start looking at things like implementing
userspace APIs which need to cope with hardware going away while still
visible to userspace.  It's *probably* more of a subsystem conversation
than a driver one though, or at least I think subsystems should try to
arrange to make it so.

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

Re: (subset) [PATCH v2 0/7] Devm helpers for regulator get and enable

[Matti Vaittinen]

Hi dee Ho Mark, Laurent, Stephen, all

On 8/16/22 01:55, Mark Brown wrote:
> On Tue, Aug 16, 2022 at 12:17:17AM +0300, Laurent Pinchart wrote:
>> On Mon, Aug 15, 2022 at 01:58:55PM -0700, Stephen Boyd wrote:
> 
>> You will very quickly see drivers doing this (either directly or
>> indirectly):
> 
>> probe()
>> {
>> 	devm_clk_get_enabled();
>> 	devm_regulator_get_enable();
>> }
> 
>> Without a devres-based get+enable API drivers can get the resources they
>> need in any order, possibly moving some of those resource acquisition
>> operations to different functions, and then have a clear block of code
>> that enables the resources in the right order.

I agree. And I think that drivers which do that should stick with it. 
Still, as you know the devm-unwinding is also done in well defined 
order. I believe that instead of fighting against the devm we should try 
educate people to pay attention in the order of unwinding (also when not 
handled by the devm. Driver writers occasionally break things also w/o 
devm for example by freeing resources needed by IRQ handlers prior 
freeing the IRQ.)

If "purging" must not be done in reverse order compared to the 
aquisition - then one should not use devm. I know people have done 
errors with devm - OTOH, I've seen devm also fixing bunch of errors.

>> These devres helpers give
>> a false sense of security to driver authors and they will end up
>> introducing problems, the same way that devm_kzalloc() makes it
>> outrageously easy to crash the kernel by disconnecting a device that is
>> in use.

I think this is going a bit "off-topic" but I'd like to understand what 
is behind this statement? From device-writer's perspective - I don't 
know much better alternatives to free up the memory. I don't see how 
freeing stuff at .remove would be any better? As far as I understand - 
if someone is using driver's resources after the device has gone and the 
driver is detached, then there is not much the driver could do to 
free-up the stuff be it devm or not? This sounds like fundamentally 
different problem (to me).

> TBH I think the problem you have here is with devm not with this
> particular function.

I must say I kind of agree with Mark. If we stop for a second to think 
what would the Laurent's example look like if there were no 
devm_regulator_get_enable() provided. I bet the poor driver author could 
have used devm_clk_get_enabled() - and then implemented a .remove for 
disabling the regulator. That would be even worse, right?

> That's a different conversation, and a totally
> valid one especially when you start looking at things like implementing
> userspace APIs which need to cope with hardware going away while still
> visible to userspace.

This is interesting. It's not easy for me to spot how devm changes 
things here? If we consider some removable device - then I guess also 
the .remove() is ran only after HW has already gone? Yes, devm might 
make the time window when userspace can see hardware that has gone 
longer but does it bring any new problem there? It seems to me devm can 
make hitting the spot more likely - but I don't think it brings 
completely new issues? (Well, I may be wrong here - wouldn't be the 
first time :])

> It's *probably* more of a subsystem conversation
> than a driver one though, or at least I think subsystems should try to
> arrange to make it so.


-- 
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: (subset) [PATCH v2 0/7] Devm helpers for regulator get and enable

[Andy Shevchenko]

On Mon, Aug 15, 2022 at 11:20 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> On Mon, Aug 15, 2022 at 05:33:06PM +0100, Mark Brown wrote:

...

> However, should a devm_clk_get_enable() or similar function be
> implemented, we'll run into trouble.

And in 5.19 we have devm_clk_get_enable(), are we already in trouble?

-- 
With Best Regards,
Andy Shevchenko

Re: (subset) [PATCH v2 0/7] Devm helpers for regulator get and enable

[Andy Shevchenko]

On Tue, Aug 16, 2022 at 8:37 AM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> On Mon, Aug 15, 2022 at 01:58:55PM -0700, Stephen Boyd wrote:
> > Quoting Laurent Pinchart (2022-08-15 11:52:36)
> > > On Mon, Aug 15, 2022 at 05:33:06PM +0100, Mark Brown wrote:

...

> > > we'll run into trouble. Supplying active high input signals
> > > to a device that is not powered can lead to latch-up, which tends to
> > > only manifest after a statistically significant number of occurrences of
> > > the condition, and can slowly damage the hardware over time. This is a
> > > real concern as it will typically not be caught during early
> > > development. I think we would still be better off with requiring drivers
> > > to manually handle powering off the device until we provide a mechanism
> > > that can do so safely in an automated way.
> >
> > Can you describe the error scenario further? I think it's driver author
> > error that would lead to getting and enabling the regulator after
> > getting and enabling a clk that drives out a clock signal on some pins
> > that aren't powered yet. I'm not sure that's all that much easier to do
> > with these sorts of devm APIs, but if it is then I'm concerned.
>
> You will very quickly see drivers doing this (either directly or
> indirectly):
>
> probe()
> {
>         devm_clk_get_enabled();
>         devm_regulator_get_enable();
> }

And how is it devm specific? If the author puts the same without devm
the ordering would be wrong, correct? devm allows us to focus on
ordering in a *single* place, which is a win. You seem to be proposing
to make a high burden on a driver's author to focus on ordering in the
*three* places. I disagree with that. Yet the driver author has to
understand many issues with any tool they use. So the root cause of
your whining is rather on the edge of documentation and education.
(Yes, I have heard about issues with object lifetime in v4l2
subdevices regarding to devm, but it seems irrelevant to devm
mechanism itself.)

> Without a devres-based get+enable API drivers can get the resources they
> need in any order, possibly moving some of those resource acquisition
> operations to different functions, and then have a clear block of code
> that enables the resources in the right order. These devres helpers give
> a false sense of security to driver authors and they will end up
> introducing problems, the same way that devm_kzalloc() makes it
> outrageously easy to crash the kernel by disconnecting a device that is
> in use.



-- 
With Best Regards,
Andy Shevchenko

Re: (subset) [PATCH v2 0/7] Devm helpers for regulator get and enable

[Mark Brown]

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

On Tue, Aug 16, 2022 at 07:56:06AM +0300, Matti Vaittinen wrote:
> On 8/16/22 01:55, Mark Brown wrote:
> > On Tue, Aug 16, 2022 at 12:17:17AM +0300, Laurent Pinchart wrote:

> > > These devres helpers give
> > > a false sense of security to driver authors and they will end up
> > > introducing problems, the same way that devm_kzalloc() makes it
> > > outrageously easy to crash the kernel by disconnecting a device that is
> > > in use.

> I think this is going a bit "off-topic" but I'd like to understand what is
> behind this statement? From device-writer's perspective - I don't know much
> better alternatives to free up the memory. I don't see how freeing stuff at
> .remove would be any better? As far as I understand - if someone is using
> driver's resources after the device has gone and the driver is detached,
> then there is not much the driver could do to free-up the stuff be it devm
> or not? This sounds like fundamentally different problem (to me).

If a driver has done something like create a file that's accessible to
userspace then that file may be held open by userspace even after the
device goes away, the driver that created the file needs to ensure that
any storage that's used by the file remains allocated until the file is
also freed (typically this is data specific to the file rather than the
full device data).  Similar situations can exist elsewhere, that's just
the common case.  There'll be a deletion callback on whatever other
object causes the problem, the allocation needs to be reference counted
against both the the device and whatever other users there might be.

> > That's a different conversation, and a totally
> > valid one especially when you start looking at things like implementing
> > userspace APIs which need to cope with hardware going away while still
> > visible to userspace.

> This is interesting. It's not easy for me to spot how devm changes things
> here? If we consider some removable device - then I guess also the .remove()
> is ran only after HW has already gone? Yes, devm might make the time window
> when userspace can see hardware that has gone longer but does it bring any
> new problem there? It seems to me devm can make hitting the spot more likely
> - but I don't think it brings completely new issues? (Well, I may be wrong
> here - wouldn't be the first time :])

See above, something can still know the device was there even after it's
gone.

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

Re: (subset) [PATCH v2 0/7] Devm helpers for regulator get and enable

[Vaittinen, Matti]

On 8/16/22 13:36, Mark Brown wrote:
> On Tue, Aug 16, 2022 at 07:56:06AM +0300, Matti Vaittinen wrote:
>> On 8/16/22 01:55, Mark Brown wrote:
>>> On Tue, Aug 16, 2022 at 12:17:17AM +0300, Laurent Pinchart wrote:
> 
>>>> These devres helpers give
>>>> a false sense of security to driver authors and they will end up
>>>> introducing problems, the same way that devm_kzalloc() makes it
>>>> outrageously easy to crash the kernel by disconnecting a device that is
>>>> in use.
> 
>> I think this is going a bit "off-topic" but I'd like to understand what is
>> behind this statement? From device-writer's perspective - I don't know much
>> better alternatives to free up the memory. I don't see how freeing stuff at
>> .remove would be any better? As far as I understand - if someone is using
>> driver's resources after the device has gone and the driver is detached,
>> then there is not much the driver could do to free-up the stuff be it devm
>> or not? This sounds like fundamentally different problem (to me).
> 
> If a driver has done something like create a file that's accessible to
> userspace then that file may be held open by userspace even after the
> device goes away, the driver that created the file needs to ensure that
> any storage that's used by the file remains allocated until the file is
> also freed (typically this is data specific to the file rather than the
> full device data).  Similar situations can exist elsewhere, that's just
> the common case.  There'll be a deletion callback on whatever other
> object causes the problem, the allocation needs to be reference counted
> against both the the device and whatever other users there might be.

Oh right. Thanks. So we're discussing (a corner?) case where the freeing 
is done by a callback that was registered by a driver. Callback being 
called for example when the refcount for a resource gets down, 
potentially long after the driver was gone.

I see the problem of releasing something with devm in such case. Still, 
I don't think it is something we should avoid by banning the use of devm 
- which is useful in many other cases. It'd be equally wrong release the 
resource in .remove() or doing any other "double freeing". To me this 
boils down to educating people about the life-times.

I wonder if writing such 'release callbacks' is compulsory? I mean, if I 
was writing a driver to some new (to me) subsystem and was required to 
write an explicit release-callback for a resource - then it'd surely 
rang a bell about potentially double freeing stuff with devm. Especially 
if the doc stated the callback can be called after the driver has been 
detached.

> 
>>> That's a different conversation, and a totally
>>> valid one especially when you start looking at things like implementing
>>> userspace APIs which need to cope with hardware going away while still
>>> visible to userspace.
> 
>> This is interesting. It's not easy for me to spot how devm changes things
>> here? If we consider some removable device - then I guess also the .remove()
>> is ran only after HW has already gone? Yes, devm might make the time window
>> when userspace can see hardware that has gone longer but does it bring any
>> new problem there? It seems to me devm can make hitting the spot more likely
>> - but I don't think it brings completely new issues? (Well, I may be wrong
>> here - wouldn't be the first time :])
> 
> See above, something can still know the device was there even after it's
> gone.

Yes. Thanks for the education.
I'm still not sure I understand how devm changes the picture. I'd guess 
in the case you described above, the user-space would still see the 
device until it closes the file and the call-back cleans-up. I guess 
freeing the stuff (that is used until user-space drops the handle) 
anywhere except the clean-up call-back is wrong - be the mechanism devm 
or driver's .remove() or any other. To me the key is still teaching 
people to know what bits may be used after the driver has been detached.

Thanks for the explanation guys - this has been insightful to me :)

Best Regards
-- Matti

-- 
The Linux Kernel guy at ROHM Semiconductors

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

~~ this year is the year of a signature writers block ~~

Re: (subset) [PATCH v2 0/7] Devm helpers for regulator get and enable

[Mark Brown]

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

On Tue, Aug 16, 2022 at 11:06:21AM +0000, Vaittinen, Matti wrote:

> I wonder if writing such 'release callbacks' is compulsory? I mean, if I 
> was writing a driver to some new (to me) subsystem and was required to 
> write an explicit release-callback for a resource - then it'd surely 
> rang a bell about potentially double freeing stuff with devm. Especially 
> if the doc stated the callback can be called after the driver has been 
> detached.

Generally yes, thoguh people can and do leave them blank and it's easy
enough to do some cleanup in there that assumes that the device is still
present and not think the device might've gone away especially if the
hardware isn't practically hotpluggable.

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

Re: (subset) [PATCH v2 0/7] Devm helpers for regulator get and enable

[Matti Vaittinen]

Hi Mark,

On 8/15/22 18:44, Mark Brown wrote:
> On Fri, 12 Aug 2022 13:08:17 +0300, Matti Vaittinen wrote:
>> 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.
>>
>> A few* drivers seem to use pattern demonstrated by pseudocode:
>>
>> [...]
> 
> Applied to
> 
>     https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git for-next
> 
> Thanks!
> 
> [1/7] docs: devres: regulator: Add missing devm_* functions to devres.rst
>        commit: 9b6744f60b6b47bc0757a1955adb4d2c3ab22e13
> [2/7] regulator: Add devm helpers for get and enable
>        (no commit info)
> 

I was planning to send out the v3 (where IIO patches are no longer 
squashed into one). I didn't spot the above mentioned patch 2/7 from 
regulator/for-next. I'd just like to get confirmation the 2/7 was not 
merged even though it's mentioned in this mail before re-spinning the 
series with it.

Best Regards
   --Matti

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

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

Re: (subset) [PATCH v2 0/7] Devm helpers for regulator get and enable

[Mark Brown]

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

On Thu, Aug 18, 2022 at 02:33:53PM +0300, Matti Vaittinen wrote:
> On 8/15/22 18:44, Mark Brown wrote:

> > [2/7] regulator: Add devm helpers for get and enable
> >        (no commit info)

> I was planning to send out the v3 (where IIO patches are no longer squashed
> into one). I didn't spot the above mentioned patch 2/7 from
> regulator/for-next. I'd just like to get confirmation the 2/7 was not merged
> even though it's mentioned in this mail before re-spinning the series with
> it.

It's not there yet (that's the "no commit info"), but it is queued for
today.

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

Re: (subset) [PATCH v2 0/7] Devm helpers for regulator get and enable

[Vaittinen, Matti]

On 8/18/22 14:54, Mark Brown wrote:
> On Thu, Aug 18, 2022 at 02:33:53PM +0300, Matti Vaittinen wrote:
>> On 8/15/22 18:44, Mark Brown wrote:
> 
>>> [2/7] regulator: Add devm helpers for get and enable
>>>         (no commit info)
> 
>> I was planning to send out the v3 (where IIO patches are no longer squashed
>> into one). I didn't spot the above mentioned patch 2/7 from
>> regulator/for-next. I'd just like to get confirmation the 2/7 was not merged
>> even though it's mentioned in this mail before re-spinning the series with
>> it.
> 
> It's not there yet (that's the "no commit info"), but it is queued for
> today.

Understood. Thanks! I'll rebase the next version on top of the 
regulator/for-next when it's out then.

--Matti

Re: (subset) [PATCH v2 0/7] Devm helpers for regulator get and enable

[Stephen Boyd]

Quoting Mark Brown (2022-08-15 15:07:35)
> On Mon, Aug 15, 2022 at 01:58:55PM -0700, Stephen Boyd wrote:
> 
> > I think the main issue is that platform drivers are being asked to do
> > too much. We've put the burden on platform driver authors to intimately
> > understand how their devices are integrated, and as we all know they're
> 
> This is for the regulator API, it's mainly for off SoC devices so it's
> not a question of understanding the integration of a device into a piece
> of silicon, it's a question of understanding the integration of a chip
> into a board which seems reasonably in scope for a chip driver and is
> certainly the sort of thing that you'd be talking to your customers
> about as a silicon vendor.

Right. I'm coming from the devm_clk_get_*() APIs angle when saying that
platform drivers don't want to know everything.

> 
> > The basic idea is that drivers should be focused on what they're
> > driving, not navigating the (sometimes) complex integration that's
> > taking place around them. When a device driver probe function is called
> > the device should already be powered on. When the driver is
> > removed/unbound, the power should be removed after the driver's remove
> > function is called. We're only going to be able to solve the power
> > sequencing and ordering problem by taking away power control and
> > sequencing from drivers.
> 
> That is a sensible approach for most on SoC things but for something
> shipped as a separate driver there's little point in separating the
> power and clocking domain driver from the device since there's typically
> a 1:1 mapping.  Usually either it's extremely simple (eg, turn
> everything on and remove reset) but some devices really need to manage
> things.  There's obviously some edge cases in SoC integration as well
> (eg, the need to manage card supplies for SD controllers, or knowing
> exact clock rates for things like audio controllers) so you need some
> flex.

I think we're on the same page. The clk API bridges both on SoC and off
SoC devices, but leans more towards on SoC devices so I'm coming from
that angle. 

I agree it doesn't make sense to rip out and move power management logic
for off SoC devices (your chip driver), because then you get a driver
that is split to two places. The hardware engineer for those types of
devices has designed the chip to be more aware of the system integration
and how their chip is powered, so that it can be easily integrated into
various designs without their involvement. This allows it to be used on
numerous boards and that's partly the reason why Linux doesn't have
board files or board "drivers" because the combinatorial explosion is
unmanageable, hence DTS and driver subsystems. The boundary of the
combinations ends at the chip which is 1:1 with the platform driver.

For on SoC devices, the hardware engineer typically isn't involved in
the system integration at all. Instead they hand that task off to the
SoC integrator who has to wire everything up (clks, power, resets) and
layout the SoC. The combinatorial explosion isn't possible here, because
only so many SoCs are ever created and customers can't rewire the
internals of the SoC to change which clks go there (although I guess
with FPGAs this may be possible). The boundary where the combinations
exist is at the device level, not the SoC level, but we've encoded the
SoC details into the compatible strings and the drivers to the point
that the boundary is pushed to the SoC level.

For these on SoC devices, we should extract and consolidate the power
management logic away from the drivers, because we're spreading the SoC
integration knowledge all around the drivers/ directory for every device
class that exists in that SoC. I continue to see drivers that get
another clk in the next SoC generation because there was some change to
split a clk domain or they get another regulator because they split a
power domain. The driver doesn't care, but it has to match a new
compatible string and then get the proper list of clks or regulators
even though it just wants to turn the thing on and get it running. This
gunk needs to go. Runtime PM is a solution to part of the problem, but I
think RPM ops should be about poking device registers for these on SoC
devices, not about controlling yet another clk or regulator that got
wired up this SoC generation.

Probably we need to get away from having platform driver probe for on
SoC devices get resources like clks, regulators, and interconnects at
all. Instead, those should be managed by some "SoC" driver that knows
the integration of the SoC and can make sure the proper power sequencing
is followed. Hopefully we can do this largely via genpd and RPM, with a
little bit of help from some SoC driver that registers genpds for
devices under /soc. Of course there are exact clk frequencies required
sometimes (audio rates, display link rates, serial baud rates, etc.) but
those sorts of things could use a higher level of abstraction so that we
don't get bogged down in the details of which clk needs to be used to
set the clk frequency when this compatible is present. Maybe
dev_pm_opp_set_rate() is the answer there, but then we have some drivers
that call clk APIs like clk_round_rate() that we'll need to figure out
how to manage.

Long story short, there's a need for both approaches so that we can
manage the combinatorial complexity at the place where it is. I hope the
devm APIs are going to help us find that place so we can come up with a
solution to the "drivers don't want to know" problem when we see that
XYZ driver can use a genpd that turns on the power along with RPM to
turn it off during suspend.