Devm helpers for regulator get and enable

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

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

Add missing functions.

---
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(+)

index 2d39967bafcc..271d1eb2234b 100644
@@ -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 =] 

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

---
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(+)

index 9113233f41cd..ca3d702b0634 100644
@@ -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);

index bbf6590a6dec..24cf5f099eef 100644
@@ -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 =] 

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

---
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(+)

index 271d1eb2234b..89f87ae613f0 100644
@@ -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 =] 

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.

---
RFCv1 => v2:
- No changes

---
 drivers/clk/clk-cdce925.c | 21 ++++-----------------
 1 file changed, 4 insertions(+), 17 deletions(-)

index ef9a2d44e40c..6350682f7e6d 100644
@@ -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 =] 

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

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

index 65549fbfdc87..4bf572b7ca77 100644
@@ -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;
 }

index 5cd2b2ebbbd3..7642f740272b 100644
@@ -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 =] 

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

---
RFCv1 => v2:
- No changes

---
 drivers/hwmon/lm90.c | 21 ++-------------------
 1 file changed, 2 insertions(+), 19 deletions(-)

index 3820f0e61510..2ab561ec367c 100644
@@ -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 =] 

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.

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

index d71977be7d22..8a52c0dec3f9 100644
@@ -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) {

index 937b0d25a11c..1abc88cd99f9 100644
@@ -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)

index 81a6d09788bd..acfabac645b6 100644
@@ -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;

index a86dd29a4738..03238c64c777 100644
@@ -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;

index 910397716833..7b40f6b58834 100644
@@ -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 =] 

Morning Andy,

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

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)

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.

Thanks,
Mark

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.

Regards,

Laurent Pinchart

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.

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

Hi Mark,

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.

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

Quoting Laurent Pinchart (2022-08-15 11:52:36)

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.

This API is implemented now.

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.

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.

Hi Stephan,

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.

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.

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

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.

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

Regards,

Laurent Pinchart

_______________________________________________
linux-amlogic mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-amlogic

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.

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.

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.

_______________________________________________
linux-amlogic mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Hi dee Ho Mark, Laurent, Stephen, all

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.

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

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?

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 :])

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

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.

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

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.

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 ~~
_______________________________________________
linux-amlogic mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-amlogic

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.

...

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

With Best Regards,
Andy Shevchenko

_______________________________________________
linux-amlogic mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-amlogic

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.

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.

Quoting Mark Brown (2022-08-15 15:07:35)

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

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.

...

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

With Best Regards,
Andy Shevchenko

_______________________________________________
linux-amlogic mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Hi Mark,

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! ~~

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

Applied to

https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git for-next

Thanks!

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

      commit: da279e6965b3838e99e5c0ab8f76b87bf86b31a5

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.

Thanks,
Mark

_______________________________________________
linux-amlogic mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-amlogic