Skip to contentSkip to conversation

Use devm helpers for regulator get and enable

PATCH Applied Matti Vaittinen 6 participants 15 messages Oct 21, 2022 – Nov 16, 2022

Subsystems (8)
DRM drivers DRM misc Arm/Amlogic Meson SoC support Hardware monitoring +4 more subsystems
lore
Share & tools
lore

Matti Vaittinen

author
#0 lore raw

v4 cover Use devm helpers for regulator get and enable

Simplify couple of drivers by using the new devm_regulator_*get_enable*()

These patches were previously part of the series:
https://lore.kernel.org/lkml/[email protected]/
"Devm helpers for regulator get and enable". I did keep the patch series versioning even though I changed the series name (subject of this mail) to "Use devm helpers for regulator get and enable". Name was changed because the devm helpers are already in 6.1-rc1.

Also, most of the patches in the series are already merged to subsystem trees so this series now contains only the patches that have not yet been merged. I hope they can be now directly taken sirectly into respective subsystem trees as the dependencies should be in v6.1-rc1.

Please note that these changes are only compile-tested as I don't have the HW to do proper testing. Thus, reviewing / testing is highly appreciated.

Revision history:

v3 => v4:
- Drop applied patches
- rewrite cover-letter
- rebase on v6.1-rc1
- split meson and sii902x into own patches as requested.
- slightly modify dev_err_probe() return in sii902x.

---

Matti Vaittinen (4):
gpu: drm: meson: Use devm_regulator_*get_enable*()
gpu: drm: sii902x: Use devm_regulator_bulk_get_enable()
hwmon: lm90: simplify using devm_regulator_get_enable()
hwmon: adm1177: simplify using devm_regulator_get_enable()

 drivers/gpu/drm/bridge/sii902x.c      | 26 ++++----------------------
 drivers/gpu/drm/meson/meson_dw_hdmi.c | 23 +++--------------------
 drivers/hwmon/adm1177.c               | 27 +++------------------------
 drivers/hwmon/lm90.c                  | 20 ++------------------
 4 files changed, 12 insertions(+), 84 deletions(-)

base-commit: 9abf2313adc1ca1b6180c508c25f22f9395cc780

Signature · 14 lines
2.37.3


-- 
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 =]
author
#1 lore raw

PATCH v4 1/4 gpu: drm: meson: Use devm_regulator_*get_enable*()

Simplify using the devm_regulator_get_enable_optional(). Also drop the seemingly unused struct member 'hdmi_supply'.

Signed-off-by Matti Vaittinen <[email protected]>

---
v3 => v4:
- split meson part to own patch

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/meson/meson_dw_hdmi.c | 23 +++--------------------
 1 file changed, 3 insertions(+), 20 deletions(-)
drivers/gpu/drm/meson/meson_dw_hdmi.c +3 -20
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");
Signature · 14 lines
2.37.3


-- 
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 =]
maintainer Reviewed-by on v4 1/4
#2 lore raw

Re: [PATCH v4 1/4] gpu: drm: meson: Use devm_regulator_*get_enable*()

Quoted 56 lines · Matti Vaittinen

Simplify using the devm_regulator_get_enable_optional(). Also drop the seemingly unused struct member 'hdmi_supply'.

Signed-off-by Matti Vaittinen <[email protected]>

---
v3 => v4:
- split meson part to own patch

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/meson/meson_dw_hdmi.c | 23 +++--------------------
  1 file changed, 3 insertions(+), 20 deletions(-)
drivers/gpu/drm/meson/meson_dw_hdmi.c +0 -16
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,

drivers/gpu/drm/meson/meson_dw_hdmi.c static int meson_dw_hdmi_bind(struct device *dev, struct device *master, · line 725

Quoted 10 lines
drivers/gpu/drm/meson/meson_dw_hdmi.c +3 -4
-					       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");
Reviewed-by Neil Armstrong <[email protected]>
reviewer
#3 lore raw

Re: [PATCH v4 1/4] gpu: drm: meson: Use devm_regulator_*get_enable*()

In reply to Matti Vaittinen

Hi Matti,

Quoted 53 lines · Matti Vaittinen

Simplify using the devm_regulator_get_enable_optional(). Also drop the seemingly unused struct member 'hdmi_supply'.

Signed-off-by Matti Vaittinen <[email protected]>

---
v3 => v4:
- split meson part to own patch

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/meson/meson_dw_hdmi.c | 23 +++--------------------
 1 file changed, 3 insertions(+), 20 deletions(-)
drivers/gpu/drm/meson/meson_dw_hdmi.c +0 -13
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);

drivers/gpu/drm/meson/meson_dw_hdmi.c static int meson_dw_hdmi_bind(struct device *dev, struct device *master, · line 722

Quoted 10 lines
drivers/gpu/drm/meson/meson_dw_hdmi.c +3 -7
-		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;

As noted in the review of the series that introduced devm_regulator_get_enable_optional(), the right thing to do is to implement runtime PM in this driver to avoid wasting power.

Quoted 3 lines
drivers/gpu/drm/meson/meson_dw_hdmi.c +0 -0
 	meson_dw_hdmi->hdmitx_apb = devm_reset_control_get_exclusive(dev,
 						"hdmitx_apb");
Signature · 3 lines
Regards,

Laurent Pinchart
maintainer
#4 lore raw

Re: [PATCH v4 1/4] gpu: drm: meson: Use devm_regulator_*get_enable*()

Hi,

Quoted 62 lines · Laurent Pinchart

Hi Matti,

Quoted 59 lines · Matti Vaittinen

Simplify using the devm_regulator_get_enable_optional(). Also drop the seemingly unused struct member 'hdmi_supply'.

Signed-off-by Matti Vaittinen <[email protected]>

---
v3 => v4:
- split meson part to own patch

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/meson/meson_dw_hdmi.c | 23 +++--------------------
  1 file changed, 3 insertions(+), 20 deletions(-)
drivers/gpu/drm/meson/meson_dw_hdmi.c +0 -19
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;
Quoted 8 lines

drivers/gpu/drm/meson/meson_dw_hdmi.c static int meson_dw_hdmi_bind(struct device *dev, struct device *master, · line 722

Quoted 4 lines
drivers/gpu/drm/meson/meson_dw_hdmi.c +3 -1
-	}
+	ret = devm_regulator_get_enable_optional(dev, "hdmi");
+	if (ret != -ENODEV)
+		return ret;

As noted in the review of the series that introduced devm_regulator_get_enable_optional(), the right thing to do is to implement runtime PM in this driver to avoid wasting power.

While I agree, it's not really the same level of effort as this patch should be functionally equivalent.

Quoted 5 lines

drivers/gpu/drm/meson/meson_dw_hdmi.c static int meson_dw_hdmi_bind(struct device *dev, struct device *master, · line 725

Quoted 4 lines
drivers/gpu/drm/meson/meson_dw_hdmi.c +0 -0
  	meson_dw_hdmi->hdmitx_apb = devm_reset_control_get_exclusive(dev,
  						"hdmitx_apb");

Neil

#5 lore raw

Re: [PATCH v4 1/4] gpu: drm: meson: Use devm_regulator_*get_enable*()

Quoted 21 lines · Neil Armstrong

Hi,

Quoted 18 lines · Laurent Pinchart

Hi Matti,

Quoted 15 lines · Matti Vaittinen

Simplify using the devm_regulator_get_enable_optional(). Also drop the seemingly unused struct member 'hdmi_supply'.

Signed-off-by Matti Vaittinen <[email protected]>

---
v3 => v4:
- split meson part to own patch

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

//Snip

Quoted 8 lines
Quoted 4 lines

As noted in the review of the series that introduced devm_regulator_get_enable_optional(), the right thing to do is to implement runtime PM in this driver to avoid wasting power.

While I agree, it's not really the same level of effort as this patch should be functionally equivalent.

Exactly. As I wrote, I do not have the HW to test this change. This intends to bring no functional changes. It is just a minor simplification while also making it harder to create a bug with the regulator control. (Registering the devm_action and leaving the handle to the regulator is more fragile than using this new API which does not give user the handle).

I am in no way against someone further improving the functionality here (by adding runtime PM) but this someone is not me. Yet, I don't see how this patch prevents runtime PM being implemented? The first thing that needs to be done is removing the devm-action assuming someone did implement power-saving by disabling the regulator(s) at runtime. After this patch is applied, the action removal is automatically a necessity in order to get the handle for regulator control.

I know this helper is not preferred by all but it is still safer than the current code which registers the action while allowing the regulator control.

Yours
-- Matti

Signature · 5 lines
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~
#6 lore raw

Re: [PATCH v4 1/4] gpu: drm: meson: Use devm_regulator_*get_enable*()

In reply to Neil Armstrong

Hi dee Ho Laurent & All,

Quoted 75 lines · Neil Armstrong

Hi,

Quoted 63 lines · Laurent Pinchart

Hi Matti,

Quoted 60 lines · Matti Vaittinen

Simplify using the devm_regulator_get_enable_optional(). Also drop the seemingly unused struct member 'hdmi_supply'.

Signed-off-by Matti Vaittinen <[email protected]>

---
v3 => v4:
- split meson part to own patch

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/meson/meson_dw_hdmi.c | 23 +++--------------------

  1 file changed, 3 insertions(+), 20 deletions(-)
drivers/gpu/drm/meson/meson_dw_hdmi.c +0 -19
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;
Quoted 8 lines

drivers/gpu/drm/meson/meson_dw_hdmi.c static int meson_dw_hdmi_bind(struct device *dev, struct device *master, · line 722

Quoted 4 lines
drivers/gpu/drm/meson/meson_dw_hdmi.c +3 -1
-    }
+    ret = devm_regulator_get_enable_optional(dev, "hdmi");
+    if (ret != -ENODEV)
+        return ret;

As noted in the review of the series that introduced devm_regulator_get_enable_optional(), the right thing to do is to implement runtime PM in this driver to avoid wasting power.

Quoted 2 lines

While I agree, it's not really the same level of effort as this patch should be functionally equivalent.

While we all agree that power savings gained by runtime PM would be great - I don't think we should stop minor improvements while waiting for that to magically happen ;)

I like the saying I first heard from Jonathan Cameron - "Don't let the perfect be an enemy of good".

After that being said, should I re-spin this or just drop it? (I am cleaning up my local git from old stuff, and these are about to vanish from my books).

Yours,
-- Matti

Signature · 5 lines
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~
maintainer
#7 lore raw

Re: [PATCH v4 1/4] gpu: drm: meson: Use devm_regulator_*get_enable*()

Quoted 85 lines · Matti

Hi dee Ho Laurent & All,

Quoted 75 lines · Neil Armstrong

Hi,

Quoted 63 lines · Laurent Pinchart

Hi Matti,

Quoted 60 lines · Matti Vaittinen

Simplify using the devm_regulator_get_enable_optional(). Also drop the seemingly unused struct member 'hdmi_supply'.

Signed-off-by Matti Vaittinen <[email protected]>

---
v3 => v4:
- split meson part to own patch

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/meson/meson_dw_hdmi.c | 23 +++--------------------
   1 file changed, 3 insertions(+), 20 deletions(-)
drivers/gpu/drm/meson/meson_dw_hdmi.c +0 -19
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;
Quoted 8 lines

drivers/gpu/drm/meson/meson_dw_hdmi.c static int meson_dw_hdmi_bind(struct device *dev, struct device *master, · line 722

Quoted 4 lines
drivers/gpu/drm/meson/meson_dw_hdmi.c +3 -1
-    }
+    ret = devm_regulator_get_enable_optional(dev, "hdmi");
+    if (ret != -ENODEV)
+        return ret;

As noted in the review of the series that introduced devm_regulator_get_enable_optional(), the right thing to do is to implement runtime PM in this driver to avoid wasting power.

Quoted 2 lines

While I agree, it's not really the same level of effort as this patch should be functionally equivalent.

While we all agree that power savings gained by runtime PM would be great - I don't think we should stop minor improvements while waiting for that to magically happen ;)

Quoted 6 lines

I like the saying I first heard from Jonathan Cameron - "Don't let the perfect be an enemy of good".

After that being said, should I re-spin this or just drop it? (I am cleaning up my local git from old stuff, and these are about to vanish from my books).

I'm ok with you, please re-spin it.

Neil

Quoted 4 lines

Yours,
-- Matti

author
#8 lore raw

PATCH v4 2/4 gpu: drm: sii902x: Use devm_regulator_bulk_get_enable()

In reply to Matti Vaittinen

Simplify using devm_regulator_bulk_get_enable()

Signed-off-by Matti Vaittinen <[email protected]>
Reviewed-by Robert Foss <[email protected]>

---
v3 => v4:
- split to own patch.
- return directly the value returned by the dev_err_probe()

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 | 26 ++++----------------------
 1 file changed, 4 insertions(+), 22 deletions(-)
drivers/gpu/drm/bridge/sii902x.c +4 -22
index 878fb7d3732b..f6e8b401069b 100644
@@ -171,7 +171,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

@@ -1072,6 +1071,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,

@@ -1122,27 +1122,11 @@ 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);
+	ret = devm_regulator_bulk_get_enable(dev, ARRAY_SIZE(supplies), supplies);
 	if (ret < 0)
-		return ret;
-
-	ret = regulator_bulk_enable(ARRAY_SIZE(sii902x->supplies),
-				    sii902x->supplies);
-	if (ret < 0) {
-		dev_err_probe(dev, ret, "Failed to enable supplies");
-		return ret;
-	}
+		return dev_err_probe(dev, ret, "Failed to enable supplies");
 
-	ret = sii902x_init(sii902x);
-	if (ret < 0) {
-		regulator_bulk_disable(ARRAY_SIZE(sii902x->supplies),
-				       sii902x->supplies);
-	}
-
-	return ret;
+	return sii902x_init(sii902x);
 }
 
 static void sii902x_remove(struct i2c_client *client)

@@ -1152,8 +1136,6 @@ static void 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);
 }
 
 static const struct of_device_id sii902x_dt_ids[] = {
Signature · 14 lines
2.37.3


-- 
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 =]
author
#9 lore raw

Re: [PATCH v4 2/4] gpu: drm: sii902x: Use devm_regulator_bulk_get_enable()

Quoted 4 lines · Matti Vaittinen

Simplify using devm_regulator_bulk_get_enable()

Signed-off-by Matti Vaittinen <[email protected]>
Reviewed-by Robert Foss <[email protected]>

Robert, I did slightly modify the return from probe when using the dev_err_probe(). I still decided to keep your RBT - please let me know if you disagree.

Eg, this:

drivers/gpu/drm/bridge/sii902x.c static int sii902x_probe(struct i2c_client *client, · line 1137

Quoted 4 lines
drivers/gpu/drm/bridge/sii902x.c +0 -4
-	if (ret < 0) {
-		dev_err_probe(dev, ret, "Failed to enable supplies");
-		return ret;
-	}

was converted to:

drivers/gpu/drm/bridge/sii902x.c static int sii902x_probe(struct i2c_client *client, · line 1127

Quoted 2 lines

if (ret < 0)

drivers/gpu/drm/bridge/sii902x.c +1 -0
+		return dev_err_probe(dev, ret, "Failed to enable supplies");

Yours
-- Matti

Signature · 5 lines
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~
Acked-by
#10 lore raw

Re: [PATCH v4 2/4] gpu: drm: sii902x: Use devm_regulator_bulk_get_enable()

Quoted 10 lines · Matti Vaittinen
Quoted 4 lines · Matti Vaittinen

Simplify using devm_regulator_bulk_get_enable()

Signed-off-by Matti Vaittinen <[email protected]>
Reviewed-by Robert Foss <[email protected]>

Robert, I did slightly modify the return from probe when using the dev_err_probe(). I still decided to keep your RBT - please let me know if you disagree.

Not a problem. In fact, feel free to upgrade it to an acked-by, to simplify merging this series.

Acked-by Robert Foss <[email protected]>
Quoted 21 lines

Eg, this:

drivers/gpu/drm/bridge/sii902x.c static int sii902x_probe(struct i2c_client *client, · line 1137

Quoted 4 lines
drivers/gpu/drm/bridge/sii902x.c +0 -4
-     if (ret < 0) {
-             dev_err_probe(dev, ret, "Failed to enable supplies");
-             return ret;
-     }

was converted to:

Quoted 2 lines
     if (ret < 0)

+ return dev_err_probe(dev, ret, "Failed to enable supplies");

Yours
-- Matti

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

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

author
#11 lore raw

PATCH v4 3/4 hwmon: lm90: simplify using devm_regulator_get_enable()

In reply to Matti Vaittinen

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 <[email protected]>
Acked-by Guenter Roeck <[email protected]>

---
RFCv1 => v2:
- No changes

---
 drivers/hwmon/lm90.c | 20 ++------------------
 1 file changed, 2 insertions(+), 18 deletions(-)
drivers/hwmon/lm90.c +2 -18
index db595f7d01f8..a3f95ba00dbf 100644
@@ -2663,11 +2663,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 int lm90_probe_channel_from_dt(struct i2c_client *client,
 				      struct device_node *child,
 				      struct lm90_data *data)

@@ -2749,24 +2744,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)
Signature · 14 lines
2.37.3


-- 
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 =]
author
#13 lore raw

PATCH v4 4/4 hwmon: adm1177: simplify using devm_regulator_get_enable()

In reply to Matti Vaittinen

Drop open-coded pattern: 'devm_regulator_get(), regulator_enable(), add_action_or_reset(regulator_disable)' and use the devm_regulator_get_enable() and drop the pointer to the regulator. This simplifies code and makes it less tempting to add manual control for the regulator which is also controlled by devm.

Signed-off-by Matti Vaittinen <[email protected]>
Acked-by Guenter Roeck <[email protected]>

---
v2 => v3:
New patch

---
 drivers/hwmon/adm1177.c | 27 +++------------------------
 1 file changed, 3 insertions(+), 24 deletions(-)
drivers/hwmon/adm1177.c +3 -24
index 0c5dbc5e33b4..be17a26a84f1 100644
@@ -26,14 +26,12 @@
 /**
  * struct adm1177_state - driver instance specific data
  * @client:		pointer to i2c client
- * @reg:		regulator info for the power supply of the device
  * @r_sense_uohm:	current sense resistor value
  * @alert_threshold_ua:	current limit for shutdown
  * @vrange_high:	internal voltage divider
  */
 struct adm1177_state {
 	struct i2c_client	*client;
-	struct regulator	*reg;
 	u32			r_sense_uohm;
 	u32			alert_threshold_ua;
 	bool			vrange_high;

@@ -189,13 +187,6 @@ static const struct hwmon_chip_info adm1177_chip_info = {
 	.info = adm1177_info,
 };
 
-static void adm1177_remove(void *data)
-{
-	struct adm1177_state *st = data;
-
-	regulator_disable(st->reg);
-}
-
 static int adm1177_probe(struct i2c_client *client)
 {
 	struct device *dev = &client->dev;

@@ -210,21 +201,9 @@ static int adm1177_probe(struct i2c_client *client)
 
 	st->client = client;
 
-	st->reg = devm_regulator_get_optional(&client->dev, "vref");
-	if (IS_ERR(st->reg)) {
-		if (PTR_ERR(st->reg) == -EPROBE_DEFER)
-			return -EPROBE_DEFER;
-
-		st->reg = NULL;
-	} else {
-		ret = regulator_enable(st->reg);
-		if (ret)
-			return ret;
-		ret = devm_add_action_or_reset(&client->dev, adm1177_remove,
-					       st);
-		if (ret)
-			return ret;
-	}
+	ret = devm_regulator_get_enable_optional(&client->dev, "vref");
+	if (ret == -EPROBE_DEFER)
+		return -EPROBE_DEFER;
 
 	if (device_property_read_u32(dev, "shunt-resistor-micro-ohms",
 				     &st->r_sense_uohm))
Signature · 14 lines
2.37.3


-- 
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 =]
maintainer Applied
#14 lore raw

Re: [PATCH v4 4/4] hwmon: adm1177: simplify using devm_regulator_get_enable()

Quoted 8 lines · Matti Vaittinen

Drop open-coded pattern: 'devm_regulator_get(), regulator_enable(), add_action_or_reset(regulator_disable)' and use the devm_regulator_get_enable() and drop the pointer to the regulator. This simplifies code and makes it less tempting to add manual control for the regulator which is also controlled by devm.

Signed-off-by Matti Vaittinen <[email protected]>
Acked-by Guenter Roeck <[email protected]>

Applied to hwmon-next.

Thanks,
Guenter