[PATCH v3 00/14] Use devm helpers for regulator get and enable

[PATCH v3 00/14] Use devm helpers for regulator get and enable

[Matti Vaittinen]

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

Use devm helpers for regulator get and enable

NOTE: The series depends on commit
ee94aff2628b ("Devm helpers for regulator get and enable")
which currently sits in Mark's regulator/for-next

A few* drivers seem to pattern demonstrated by pseudocode:

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

devm helpers for this pattern were added to 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.

I believe this simplifies things by removing some dublicated code.

This series reowrks a few drivers. 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:

v3:
	- Drop already applied helper patches
	- Add a few more drivers

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:
	Add new devm-helper APIs to docs.
Patch 2:
	simplified CLK driver(s)
Patch 3:
	simplified GPU driver(s)
Patch 4 - 5:
	simplified hwmon driver(s)
Patch 6 - 14:
	simplified IIO driver(s)

---

Matti Vaittinen (14):
  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()
  hwmon: adm1177: simplify using devm_regulator_get_enable()
  iio: ad7192: Simplify using devm_regulator_get_enable()
  iio: ltc2688: Simplify using devm_regulator_*get_enable()
  iio: bmg160_core: Simplify using devm_regulator_*get_enable()
  iio: st_lsm6dsx: Simplify using devm_regulator_*get_enable()
  iio: ad7476: simplify using devm_regulator_get_enable()
  iio: ad7606: simplify using devm_regulator_get_enable()
  iio: max1241: simplify using devm_regulator_get_enable()
  iio: max1363: simplify using devm_regulator_get_enable()
  iio: hmc425a: simplify using devm_regulator_get_enable()

 .../driver-api/driver-model/devres.rst        |  4 +++
 drivers/clk/clk-cdce925.c                     | 21 +++----------
 drivers/gpu/drm/bridge/sii902x.c              | 22 ++------------
 drivers/gpu/drm/meson/meson_dw_hdmi.c         | 23 ++------------
 drivers/hwmon/adm1177.c                       | 27 ++---------------
 drivers/hwmon/lm90.c                          | 15 ++--------
 drivers/iio/adc/ad7192.c                      | 15 ++--------
 drivers/iio/adc/ad7476.c                      | 11 +------
 drivers/iio/adc/ad7606.c                      | 22 ++------------
 drivers/iio/adc/ad7606.h                      |  1 -
 drivers/iio/adc/max1241.c                     | 28 ++---------------
 drivers/iio/adc/max1363.c                     | 11 +------
 drivers/iio/amplifiers/hmc425a.c              | 17 +----------
 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 ++++---------------
 17 files changed, 41 insertions(+), 255 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 v3 01/14] docs: devres: regulator: Add new get_enable functions to devres.rst

[Matti Vaittinen]

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

---
v2 => v3
No changes

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 882b14089454..f667b33bbda5 100644
--- a/Documentation/driver-api/driver-model/devres.rst
+++ b/Documentation/driver-api/driver-model/devres.rst
@@ -407,7 +407,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 v3 02/14] clk: cdce925: simplify using devm_regulator_get_enable()

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 1929 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 => onwards:
- 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 --]

Re: [PATCH v3 02/14] clk: cdce925: simplify using devm_regulator_get_enable()

[Stephen Boyd]

Quoting Matti Vaittinen (2022-08-19 12:17:53)
> 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>
> 
> ---

Applied to clk-next

[PATCH v3 03/14] gpu: drm: simplify drivers using devm_regulator_*get_enable*()

[Matti Vaittinen]

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

---
v2 => v3:
No changes

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 7ab38d734ad6..162f9c87eeb2 100644
--- a/drivers/gpu/drm/bridge/sii902x.c
+++ b/drivers/gpu/drm/bridge/sii902x.c
@@ -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,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)
@@ -1152,8 +1138,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 --]

Re: [PATCH v3 03/14] gpu: drm: simplify drivers using devm_regulator_*get_enable*()

[Robert Foss]

Hi Matti,

On Fri, 19 Aug 2022 at 21:18, Matti Vaittinen <mazziesaccount@gmail.com> wrote:
>
> 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>
>
> ---
> v2 => v3:
> No changes
>
> 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 7ab38d734ad6..162f9c87eeb2 100644
> --- a/drivers/gpu/drm/bridge/sii902x.c
> +++ b/drivers/gpu/drm/bridge/sii902x.c
> @@ -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,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)
> @@ -1152,8 +1138,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;
>  }

Ideally this patch would be split into two parts here, due to
maintainership boundaries.

For the sii902x part, please add my r-b.

Reviewed-by: Robert Foss <robert.foss@linaro.org>

> 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

Re: [PATCH v3 03/14] gpu: drm: simplify drivers using devm_regulator_*get_enable*()

[Matti Vaittinen]

On 8/29/22 17:25, Robert Foss wrote:

Thanks for the review Robert.

> Hi Matti,
> 
> On Fri, 19 Aug 2022 at 21:18, Matti Vaittinen <mazziesaccount@gmail.com> wrote:
>>
>> 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>
>>
>> ---
>> v2 => v3:
>> No changes
>>
>> 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 7ab38d734ad6..162f9c87eeb2 100644
>> --- a/drivers/gpu/drm/bridge/sii902x.c
>> +++ b/drivers/gpu/drm/bridge/sii902x.c
>> @@ -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,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)
>> @@ -1152,8 +1138,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;
>>   }
> 
> Ideally this patch would be split into two parts here, due to
> maintainership boundaries.

Ok. I will in any case respin this series when the dependency patches 
from Mark's tree have been merged to the -rc1. I can split this to 
patch/driver if it is preferred. I just though I'll decrease amount of 
mails by squashing these almost trivial changes.

> 
> For the sii902x part, please add my r-b.
> 
> Reviewed-by: Robert Foss <robert.foss@linaro.org>
Thanks.

Best Regards
  -- Matti

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

[PATCH v3 04/14] hwmon: lm90: simplify using devm_regulator_get_enable()

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 1761 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 => onwards:
- No changes
---
 drivers/hwmon/lm90.c | 15 ++-------------
 1 file changed, 2 insertions(+), 13 deletions(-)

diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
index 221de01a327a..6356d92547e0 100644
--- a/drivers/hwmon/lm90.c
+++ b/drivers/hwmon/lm90.c
@@ -2749,24 +2749,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 v3 05/14] hwmon: adm1177: simplify using devm_regulator_get_enable()

[Matti Vaittinen]

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

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 <mazziesaccount@gmail.com>

---
v2 => v3:
New patch
---
 drivers/hwmon/adm1177.c | 27 +++------------------------
 1 file changed, 3 insertions(+), 24 deletions(-)

diff --git a/drivers/hwmon/adm1177.c b/drivers/hwmon/adm1177.c
index 0c5dbc5e33b4..be17a26a84f1 100644
--- a/drivers/hwmon/adm1177.c
+++ b/drivers/hwmon/adm1177.c
@@ -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))
-- 
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 v3 05/14] hwmon: adm1177: simplify using devm_regulator_get_enable()

[Guenter Roeck]

On Fri, Aug 19, 2022 at 10:18:46PM +0300, Matti Vaittinen wrote:
> Drop open-coded pattern: 'devm_regulator_get(), regulator_enable(),
> add_action_or_reset(regulator_disable)' and use the
> devm_regulator_get_enable() 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 <mazziesaccount@gmail.com>

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

> 
> ---
> v2 => v3:
> New patch
> ---
>  drivers/hwmon/adm1177.c | 27 +++------------------------
>  1 file changed, 3 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/hwmon/adm1177.c b/drivers/hwmon/adm1177.c
> index 0c5dbc5e33b4..be17a26a84f1 100644
> --- a/drivers/hwmon/adm1177.c
> +++ b/drivers/hwmon/adm1177.c
> @@ -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))
> -- 
> 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 =] 

[PATCH v3 06/14] iio: ad7192: Simplify using devm_regulator_get_enable()

[Matti Vaittinen]

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

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

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

---
v2 => v3
Split to own patch.
---
 drivers/iio/adc/ad7192.c | 15 ++-------------
 1 file changed, 2 insertions(+), 13 deletions(-)

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


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

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

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

Re: [PATCH v3 06/14] iio: ad7192: Simplify using devm_regulator_get_enable()

[Jonathan Cameron]

On Fri, 19 Aug 2022 22:19:01 +0300
Matti Vaittinen <mazziesaccount@gmail.com> wrote:

> 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'.
> 
> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
Now the required dependencies are upstream...

Applied to the togreg branch of iio.git.

Thanks,

Jonathan

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

[PATCH v3 07/14] iio: ltc2688: Simplify using devm_regulator_*get_enable()

[Matti Vaittinen]

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

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

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

---
v2 => v3
Split to own patch.
---
 drivers/iio/dac/ltc2688.c | 23 +++--------------------
 1 file changed, 3 insertions(+), 20 deletions(-)

diff --git a/drivers/iio/dac/ltc2688.c b/drivers/iio/dac/ltc2688.c
index 28bdde2d3088..fcad3efe62ea 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)
-- 
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 v3 07/14] iio: ltc2688: Simplify using devm_regulator_*get_enable()

[Jonathan Cameron]

On Fri, 19 Aug 2022 22:19:17 +0300
Matti Vaittinen <mazziesaccount@gmail.com> wrote:

> Use devm_regulator_bulk_get_enable() instead of open coded bulk-get,
> bulk-enable, add-action-to-disable-at-detach - pattern.
> 
> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
> 
> ---
> v2 => v3
> Split to own patch.
> ---
>  drivers/iio/dac/ltc2688.c | 23 +++--------------------
>  1 file changed, 3 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/iio/dac/ltc2688.c b/drivers/iio/dac/ltc2688.c
> index 28bdde2d3088..fcad3efe62ea 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"};
trivial - slight preference for 
 { "vcc", "iovcc" };

This isn't as important as for numeric values as we get some readability
from the quotes but still nice to have.

For the whole static / vs non static. My personal preference is not
to have the static marking but I don't care that much.

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

Re: [PATCH v3 07/14] iio: ltc2688: Simplify using devm_regulator_*get_enable()

[Matti Vaittinen]

On 8/20/22 14:21, Jonathan Cameron wrote:
> On Fri, 19 Aug 2022 22:19:17 +0300
> Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> 
>> Use devm_regulator_bulk_get_enable() instead of open coded bulk-get,
>> bulk-enable, add-action-to-disable-at-detach - pattern.
>>
>> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
>>
>> ---
>> v2 => v3
>> Split to own patch.
>> ---
>>   drivers/iio/dac/ltc2688.c | 23 +++--------------------
>>   1 file changed, 3 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/iio/dac/ltc2688.c b/drivers/iio/dac/ltc2688.c
>> index 28bdde2d3088..fcad3efe62ea 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"};
> trivial - slight preference for
>   { "vcc", "iovcc" };
> 
> This isn't as important as for numeric values as we get some readability
> from the quotes but still nice to have.

Right. I'll fix it.

> For the whole static / vs non static. My personal preference is not
> to have the static marking but I don't care that much.
> 

I'd like to stick with the static here. I know this one particular array 
does not have much of a footprint - but I'd like to encourage the habit 
of considering the memory usage. This discussion serves as an example of 
how unknown the impact of making const data static is. I didn't know 
this myself until Sebastian educated me :)  Hence my strong preference 
on keeping this 'static' as an example for others who are as ignorant as 
I were ;) After all, having const data arrays static is quite an easy 
way of improving things - and it really does matter when there is many 
of arrays - or when they contain large data.

Yours
-- Matti

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

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

Re: [PATCH v3 07/14] iio: ltc2688: Simplify using devm_regulator_*get_enable()

[Andy Shevchenko]

On Sat, Aug 20, 2022 at 4:45 PM Matti Vaittinen
<mazziesaccount@gmail.com> wrote:
> On 8/20/22 14:21, Jonathan Cameron wrote:
> > On Fri, 19 Aug 2022 22:19:17 +0300
> > Matti Vaittinen <mazziesaccount@gmail.com> wrote:

...

> >> +    static const char * const regulators[] = {"vcc", "iovcc"};
> > trivial - slight preference for
> >   { "vcc", "iovcc" };
> >
> > This isn't as important as for numeric values as we get some readability
> > from the quotes but still nice to have.
>
> Right. I'll fix it.

And also make it a reversed xmas tree order.

> > For the whole static / vs non static. My personal preference is not
> > to have the static marking but I don't care that much.
>
> I'd like to stick with the static here. I know this one particular array
> does not have much of a footprint - but I'd like to encourage the habit
> of considering the memory usage. This discussion serves as an example of
> how unknown the impact of making const data static is. I didn't know
> this myself until Sebastian educated me :)  Hence my strong preference
> on keeping this 'static' as an example for others who are as ignorant as
> I were ;) After all, having const data arrays static is quite an easy
> way of improving things - and it really does matter when there is many
> of arrays - or when they contain large data.

But still the same comment about global scope of the variable is applied.

As I explained before, hiding global variables inside a function is a
bad code practice.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 07/14] iio: ltc2688: Simplify using devm_regulator_*get_enable()

[Matti Vaittinen]

On 8/20/22 19:09, Andy Shevchenko wrote:
> On Sat, Aug 20, 2022 at 4:45 PM Matti Vaittinen
> <mazziesaccount@gmail.com> wrote:
>> On 8/20/22 14:21, Jonathan Cameron wrote:
>>> On Fri, 19 Aug 2022 22:19:17 +0300
>>> Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> 
> ...
> 
>>>> +    static const char * const regulators[] = {"vcc", "iovcc"};
>>> trivial - slight preference for
>>>    { "vcc", "iovcc" };
>>>
>>> This isn't as important as for numeric values as we get some readability
>>> from the quotes but still nice to have.
>>
>> Right. I'll fix it.
> 
> And also make it a reversed xmas tree order.

can do.

> 
>>> For the whole static / vs non static. My personal preference is not
>>> to have the static marking but I don't care that much.
>>
>> I'd like to stick with the static here. I know this one particular array
>> does not have much of a footprint - but I'd like to encourage the habit
>> of considering the memory usage. This discussion serves as an example of
>> how unknown the impact of making const data static is. I didn't know
>> this myself until Sebastian educated me :)  Hence my strong preference
>> on keeping this 'static' as an example for others who are as ignorant as
>> I were ;) After all, having const data arrays static is quite an easy
>> way of improving things - and it really does matter when there is many
>> of arrays - or when they contain large data.
> 
> But still the same comment about global scope of the variable is applied.

I don't understand why you keep claiming the variable is global when it 
is not?

> As I explained before, hiding global variables inside a function is a
> bad code practice.

I don't really get what you mean here. And I definitely don't see any 
improvement if we would really use a global variable instead of a local one.

--Matti

-- 
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: [PATCH v3 07/14] iio: ltc2688: Simplify using devm_regulator_*get_enable()

[Andy Shevchenko]

On Sat, Aug 20, 2022 at 8:30 PM Matti Vaittinen
<mazziesaccount@gmail.com> wrote:
>
> On 8/20/22 19:09, Andy Shevchenko wrote:
> > On Sat, Aug 20, 2022 at 4:45 PM Matti Vaittinen
> > <mazziesaccount@gmail.com> wrote:
> >> On 8/20/22 14:21, Jonathan Cameron wrote:
> >>> On Fri, 19 Aug 2022 22:19:17 +0300
> >>> Matti Vaittinen <mazziesaccount@gmail.com> wrote:

...

> >>> For the whole static / vs non static. My personal preference is not
> >>> to have the static marking but I don't care that much.
> >>
> >> I'd like to stick with the static here. I know this one particular array
> >> does not have much of a footprint - but I'd like to encourage the habit
> >> of considering the memory usage. This discussion serves as an example of
> >> how unknown the impact of making const data static is. I didn't know
> >> this myself until Sebastian educated me :)  Hence my strong preference
> >> on keeping this 'static' as an example for others who are as ignorant as
> >> I were ;) After all, having const data arrays static is quite an easy
> >> way of improving things - and it really does matter when there is many
> >> of arrays - or when they contain large data.
> >
> > But still the same comment about global scope of the variable is applied.
>
> I don't understand why you keep claiming the variable is global when it
> is not?

It is. The static keyword makes it global, but putting the entire
definition into the function is asking for troubles.

I guess some C standard chapter describes that in non-understandable language.

> > As I explained before, hiding global variables inside a function is a
> > bad code practice.
>
> I don't really get what you mean here. And I definitely don't see any
> improvement if we would really use a global variable instead of a local one.

The improvement is avoid hiding the global variable to the local namespace.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 07/14] iio: ltc2688: Simplify using devm_regulator_*get_enable()

[Matti Vaittinen]

On 8/20/22 20:41, Andy Shevchenko wrote:
> On Sat, Aug 20, 2022 at 8:30 PM Matti Vaittinen
> <mazziesaccount@gmail.com> wrote:
>>
>> On 8/20/22 19:09, Andy Shevchenko wrote:
>>> On Sat, Aug 20, 2022 at 4:45 PM Matti Vaittinen
>>> <mazziesaccount@gmail.com> wrote:
>>>> On 8/20/22 14:21, Jonathan Cameron wrote:
>>>>> On Fri, 19 Aug 2022 22:19:17 +0300
>>>>> Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> 
> ...
> 
>>>>> For the whole static / vs non static. My personal preference is not
>>>>> to have the static marking but I don't care that much.
>>>>
>>>> I'd like to stick with the static here. I know this one particular array
>>>> does not have much of a footprint - but I'd like to encourage the habit
>>>> of considering the memory usage. This discussion serves as an example of
>>>> how unknown the impact of making const data static is. I didn't know
>>>> this myself until Sebastian educated me :)  Hence my strong preference
>>>> on keeping this 'static' as an example for others who are as ignorant as
>>>> I were ;) After all, having const data arrays static is quite an easy
>>>> way of improving things - and it really does matter when there is many
>>>> of arrays - or when they contain large data.
>>>
>>> But still the same comment about global scope of the variable is applied.
>>
>> I don't understand why you keep claiming the variable is global when it
>> is not?
> 
> It is. The static keyword makes it global, but putting the entire
> definition into the function is asking for troubles.
> 

Please, describe the trouble we can get with a local static const 
variable? A real concrete threat there is. I have explained the benefit. 
I have also explained the concrete possibility of name collision when we 
really do a global out of local.

> I guess some C standard chapter describes that in non-understandable language.
> 
>>> As I explained before, hiding global variables inside a function is a
>>> bad code practice.
>>
>> I don't really get what you mean here. And I definitely don't see any
>> improvement if we would really use a global variable instead of a local one.
> 
> The improvement is avoid hiding the global variable to the local namespace.

I guess you mean that you may miss the fact that a variable stays there 
even after execution exits the function, right? Ok, let's assume someone 
misses this point when reading the code. Now, please describe me the 
potential issues this can cause knowing our static is const and doesn't 
change the value.

Best Regards
	-- Matti

-- 
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: [PATCH v3 07/14] iio: ltc2688: Simplify using devm_regulator_*get_enable()

[Andy Shevchenko]

On Sat, Aug 20, 2022 at 10:00 PM Matti Vaittinen
<mazziesaccount@gmail.com> wrote:
> On 8/20/22 20:41, Andy Shevchenko wrote:
> > On Sat, Aug 20, 2022 at 8:30 PM Matti Vaittinen
> > <mazziesaccount@gmail.com> wrote:
> >> On 8/20/22 19:09, Andy Shevchenko wrote:
> >>> On Sat, Aug 20, 2022 at 4:45 PM Matti Vaittinen
> >>> <mazziesaccount@gmail.com> wrote:
> >>>> On 8/20/22 14:21, Jonathan Cameron wrote:
> >>>>> On Fri, 19 Aug 2022 22:19:17 +0300
> >>>>> Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> >
> > ...
> >
> >>>>> For the whole static / vs non static. My personal preference is not
> >>>>> to have the static marking but I don't care that much.
> >>>>
> >>>> I'd like to stick with the static here. I know this one particular array
> >>>> does not have much of a footprint - but I'd like to encourage the habit
> >>>> of considering the memory usage. This discussion serves as an example of
> >>>> how unknown the impact of making const data static is. I didn't know
> >>>> this myself until Sebastian educated me :)  Hence my strong preference
> >>>> on keeping this 'static' as an example for others who are as ignorant as
> >>>> I were ;) After all, having const data arrays static is quite an easy
> >>>> way of improving things - and it really does matter when there is many
> >>>> of arrays - or when they contain large data.
> >>>
> >>> But still the same comment about global scope of the variable is applied.
> >>
> >> I don't understand why you keep claiming the variable is global when it
> >> is not?
> >
> > It is. The static keyword makes it global, but putting the entire
> > definition into the function is asking for troubles.

> Please, describe the trouble we can get with a local static const
> variable? A real concrete threat there is. I have explained the benefit.
> I have also explained the concrete possibility of name collision when we
> really do a global out of local.

I told you, the benefit is not to play dirty tricks on developers,
maintainers and reviewers. It's simply harder to read the code and get
the usage of the variable that lifetime is out of scope of the
function.

> > I guess some C standard chapter describes that in non-understandable language.
> >
> >>> As I explained before, hiding global variables inside a function is a
> >>> bad code practice.
> >>
> >> I don't really get what you mean here. And I definitely don't see any
> >> improvement if we would really use a global variable instead of a local one.
> >
> > The improvement is avoid hiding the global variable to the local namespace.
>
> I guess you mean that you may miss the fact that a variable stays there
> even after execution exits the function, right? Ok, let's assume someone
> misses this point when reading the code. Now, please describe me the
> potential issues this can cause knowing our static is const and doesn't
> change the value.

When you hide the static variable inside the function, you simply
narrow visibility to the compiler, but the variable stays all the time
the module is in.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 07/14] iio: ltc2688: Simplify using devm_regulator_*get_enable()

[Vaittinen, Matti]

On 8/21/22 16:13, Andy Shevchenko wrote:
> On Sat, Aug 20, 2022 at 10:00 PM Matti Vaittinen
> <mazziesaccount@gmail.com> wrote:
>> On 8/20/22 20:41, Andy Shevchenko wrote:
>>> On Sat, Aug 20, 2022 at 8:30 PM Matti Vaittinen
>>> <mazziesaccount@gmail.com> wrote:
>>>> On 8/20/22 19:09, Andy Shevchenko wrote:
>>>>> On Sat, Aug 20, 2022 at 4:45 PM Matti Vaittinen
>>>>> <mazziesaccount@gmail.com> wrote:
>>>>>> On 8/20/22 14:21, Jonathan Cameron wrote:
>>>>>>> On Fri, 19 Aug 2022 22:19:17 +0300
>>>>>>> Matti Vaittinen <mazziesaccount@gmail.com> wrote:
>>>
>>> ...
>>>
>>>>>>> For the whole static / vs non static. My personal preference is not
>>>>>>> to have the static marking but I don't care that much.
>>>>>>
>>>>>> I'd like to stick with the static here. I know this one particular array
>>>>>> does not have much of a footprint - but I'd like to encourage the habit
>>>>>> of considering the memory usage. This discussion serves as an example of
>>>>>> how unknown the impact of making const data static is. I didn't know
>>>>>> this myself until Sebastian educated me :)  Hence my strong preference
>>>>>> on keeping this 'static' as an example for others who are as ignorant as
>>>>>> I were ;) After all, having const data arrays static is quite an easy
>>>>>> way of improving things - and it really does matter when there is many
>>>>>> of arrays - or when they contain large data.
>>>>>
>>>>> But still the same comment about global scope of the variable is applied.
>>>>
>>>> I don't understand why you keep claiming the variable is global when it
>>>> is not?
>>>
>>> It is. The static keyword makes it global, but putting the entire
>>> definition into the function is asking for troubles.
> 
>> Please, describe the trouble we can get with a local static const
>> variable? A real concrete threat there is. I have explained the benefit.
>> I have also explained the concrete possibility of name collision when we
>> really do a global out of local.
> 
> I told you, the benefit is not to play dirty tricks on developers,
> maintainers and reviewers.

I see nothing concrete in that statement.

> It's simply harder to read the code and get
> the usage of the variable that lifetime is out of scope of the
> function.

This still makes no sense to me. Lifetime is the same even if we put the 
variable out of the function as you suggested. It does not change. And a 
reviewer missing that fact for a const data does really not matter. 
Variable is there when function is entered, it has always the same value 
- it just is not in the stack. I agree that a variable which value may 
change between function calls is more difficult to track when it is 
static. This is not the case here.

For a reviewer or code reader it is actually _much simpler_ to see where 
the variable is used when we put it in the block where it is used. If I 
did as you suggested and pulled it out of the function then every code 
reader should grep the whole file to detect the use. Now readers only 
need to check if a pointer to the variable is returned out of the 
function. Oh, and this same check should be done for truly global 
variables too - as users can store the pointer to something which does 
not match the grep.

So, again. Putting the variables (also static ones) in the blocks do 
make code reading and reviewing _easier_.

> 
>>> I guess some C standard chapter describes that in non-understandable language.
>>>
>>>>> As I explained before, hiding global variables inside a function is a
>>>>> bad code practice.
>>>>
>>>> I don't really get what you mean here. And I definitely don't see any
>>>> improvement if we would really use a global variable instead of a local one.
>>>
>>> The improvement is avoid hiding the global variable to the local namespace.
>>
>> I guess you mean that you may miss the fact that a variable stays there
>> even after execution exits the function, right? Ok, let's assume someone
>> misses this point when reading the code. Now, please describe me the
>> potential issues this can cause knowing our static is const and doesn't
>> change the value.
> 
> When you hide the static variable inside the function, you simply
> narrow visibility to the compiler, but the variable stays all the time
> the module is in.

Yes. The constant, unchanging data stays there all the time. How does it 
make your reviewing harder? Why do you care whether the data stays in 
the same place or not? What you should be interested is where and how 
the data is accessed - and this is where having the variable local will 
actually help you.

More I think of this, less I can see the problem you see :(

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: [PATCH v3 07/14] iio: ltc2688: Simplify using devm_regulator_*get_enable()

[Jonathan Cameron]

Interesting to see the different mental models used when reading code.

Overall I'm fine with either static within function or static outside
function depending on author preference + for short cases like this
I'm also fine with them on the stack.

Obviously can only speak for what I'll take into IIO!

Jonathan

RE: [PATCH v3 07/14] iio: ltc2688: Simplify using devm_regulator_*get_enable()

[Sa, Nuno]

> From: Matti Vaittinen <mazziesaccount@gmail.com>
> Sent: Friday, August 19, 2022 9:19 PM
> To: Matti Vaittinen <mazziesaccount@gmail.com>; Matti Vaittinen
> <matti.vaittinen@fi.rohmeurope.com>
> Cc: Sa, Nuno <Nuno.Sa@analog.com>; Jonathan Cameron
> <jic23@kernel.org>; Lars-Peter Clausen <lars@metafoo.de>; linux-
> iio@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: [PATCH v3 07/14] iio: ltc2688: Simplify using
> devm_regulator_*get_enable()
> 
> [External]
> 
> Use devm_regulator_bulk_get_enable() instead of open coded bulk-
> get,
> bulk-enable, add-action-to-disable-at-detach - pattern.
> 
> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
> 

FWIW, I also don't care that much about the whole variable scope
discussion... Hence, with the change requested by Jonathan:

Reviewed-by: Nuno Sá <nuno.sa@analog.com>

Re: [PATCH v3 07/14] iio: ltc2688: Simplify using devm_regulator_*get_enable()

[Jonathan Cameron]

On Fri, 19 Aug 2022 22:19:17 +0300
Matti Vaittinen <mazziesaccount@gmail.com> wrote:

> Use devm_regulator_bulk_get_enable() instead of open coded bulk-get,
> bulk-enable, add-action-to-disable-at-detach - pattern.
> 
> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>

Applied with the ordering tweaked as per Andy's review.
Also tweaked the patch title to iio: dac: ltc2688: 
just to make it a bit easier to go from patch title to driver.

> 
> ---
> v2 => v3
> Split to own patch.
> ---
>  drivers/iio/dac/ltc2688.c | 23 +++--------------------
>  1 file changed, 3 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/iio/dac/ltc2688.c b/drivers/iio/dac/ltc2688.c
> index 28bdde2d3088..fcad3efe62ea 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)

[PATCH v3 08/14] iio: bmg160_core: Simplify using devm_regulator_*get_enable()

[Matti Vaittinen]

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

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

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

---
v2 => v3
Split to own patch.
---
 drivers/iio/gyro/bmg160_core.c | 24 +++---------------------
 1 file changed, 3 insertions(+), 21 deletions(-)

diff --git a/drivers/iio/gyro/bmg160_core.c b/drivers/iio/gyro/bmg160_core.c
index cedd9f02ea21..baa80980c99f 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;
-- 
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 v3 08/14] iio: bmg160_core: Simplify using devm_regulator_*get_enable()

[Andy Shevchenko]

On Fri, Aug 19, 2022 at 10:21 PM Matti Vaittinen
<mazziesaccount@gmail.com> wrote:
>
> Use devm_regulator_bulk_get_enable() instead of open coded bulk-get,
> bulk-enable, add-action-to-disable-at-detach - pattern.

...

>  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"};

Please, keep this following the "longest line first" rule. Note, in
this case you even can move it out of the function, so we will see
clearly that this is (not a hidden) global variable.

P.S. Same applies for the rest of the similar places in your series.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 08/14] iio: bmg160_core: Simplify using devm_regulator_*get_enable()

[Vaittinen, Matti]

Thanks for the review Andy

On 8/20/22 02:30, Andy Shevchenko wrote:
> On Fri, Aug 19, 2022 at 10:21 PM Matti Vaittinen
> <mazziesaccount@gmail.com> wrote:
>>
>> Use devm_regulator_bulk_get_enable() instead of open coded bulk-get,
>> bulk-enable, add-action-to-disable-at-detach - pattern.
> 
> ...
> 
>>   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"};
> 
> Please, keep this following the "longest line first" rule. Note, in

This was not following the (IMO slightly silly) rule even prior my 
patch. I can for sure move my line up - but that won't give you the 
"reverse X-mas tree".

I don't have any real objections on changing the styling though - I 
don't expect this to be merged before the dependency is in rc1 - so I 
guess I will anyways need to respin this for next cycle. I can do the 
styling then.

> this case you even can move it out of the function, so we will see
> clearly that this is (not a hidden) global variable.

Here I do disagree with you. Moving the array out of the function makes 
it _much_ less obvious it is not used outside this function. Reason for 
making is "static const" is to allow the data be placed in read-only 
area (thanks to Guenter who originally gave me this tip).

> P.S. Same applies for the rest of the similar places in your series.
> 

Br,
	-- 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: [PATCH v3 08/14] iio: bmg160_core: Simplify using devm_regulator_*get_enable()

[Andy Shevchenko]

On Sat, Aug 20, 2022 at 9:19 AM Vaittinen, Matti
<Matti.Vaittinen@fi.rohmeurope.com> wrote:
> On 8/20/22 02:30, Andy Shevchenko wrote:
> > On Fri, Aug 19, 2022 at 10:21 PM Matti Vaittinen
> > <mazziesaccount@gmail.com> wrote:

...

> >>          struct bmg160_data *data;
> >>          struct iio_dev *indio_dev;
> >>          int ret;
> >> +       static const char * const regulators[] = {"vdd", "vddio"};
> >
> > Please, keep this following the "longest line first" rule. Note, in
>
> This was not following the (IMO slightly silly) rule even prior my
> patch. I can for sure move my line up - but that won't give you the
> "reverse X-mas tree".

What do you mean by this? In the above case the rule does exactly give
you "reversed xmas tree order". What did I miss?

> I don't have any real objections on changing the styling though - I
> don't expect this to be merged before the dependency is in rc1 - so I
> guess I will anyways need to respin this for next cycle. I can do the
> styling then.

Fine with me.

> > this case you even can move it out of the function, so we will see
> > clearly that this is (not a hidden) global variable.
>
> Here I do disagree with you. Moving the array out of the function makes
> it _much_ less obvious it is not used outside this function. Reason for
> making is "static const" is to allow the data be placed in read-only
> area (thanks to Guenter who originally gave me this tip).

"static" in C language means two things (that's what come to my mind):
- for functions this tells that a function is not used outside of the module;
- for variables that it is a _global_ variable.

Hiding static inside functions is not a good coding practice since it
hides scope of the variable. And if you look into the kernel code, I
believe the use you are proposing is in minority.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 08/14] iio: bmg160_core: Simplify using devm_regulator_*get_enable()

[Vaittinen, Matti]

On 8/20/22 09:25, Andy Shevchenko wrote:
> On Sat, Aug 20, 2022 at 9:19 AM Vaittinen, Matti
> <Matti.Vaittinen@fi.rohmeurope.com> wrote:
>> On 8/20/22 02:30, Andy Shevchenko wrote:
>>> On Fri, Aug 19, 2022 at 10:21 PM Matti Vaittinen
>>> <mazziesaccount@gmail.com> wrote:
> 
> What did I miss?

 >>>>           struct bmg160_data *data;
 >>>>           struct iio_dev *indio_dev;

This does already violate the rule.

> 
>>> this case you even can move it out of the function, so we will see
>>> clearly that this is (not a hidden) global variable.
>>
>> Here I do disagree with you. Moving the array out of the function makes
>> it _much_ less obvious it is not used outside this function. Reason for
>> making is "static const" is to allow the data be placed in read-only
>> area (thanks to Guenter who originally gave me this tip).
> 
> "static" in C language means two things (that's what come to my mind):
> - for functions this tells that a function is not used outside of the module;
> - for variables that it is a _global_ variable.
> 
> Hiding static inside functions is not a good coding practice since it
> hides scope of the variable.

For const arrays the static in function does make sense. Being able to 
place the data in read-only areas do help with the memory on limited 
systems.

> And if you look into the kernel code, I
> believe the use you are proposing is in minority.

I don't know about the statistics. What I know is that we do have a 
technical benefits when we use static const arrays instead of non static 
ones in the functions. I do also believe placing the variables in blocks 
is a good practice.

I tend to agree with you that using local, non const statics has 
pitfalls - but the pitfalls do not really apply with const ones. You 
know the value and have no races. Benefit is that just by seeing that no 
pointer is returned you can be sure that no "sane code" uses the data 
outside the function it resides.

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: [PATCH v3 08/14] iio: bmg160_core: Simplify using devm_regulator_*get_enable()

[Andy Shevchenko]

On Sat, Aug 20, 2022 at 9:48 AM Vaittinen, Matti
<Matti.Vaittinen@fi.rohmeurope.com> wrote:
> On 8/20/22 09:25, Andy Shevchenko wrote:
> > On Sat, Aug 20, 2022 at 9:19 AM Vaittinen, Matti
> > <Matti.Vaittinen@fi.rohmeurope.com> wrote:
> >> On 8/20/22 02:30, Andy Shevchenko wrote:
> >>> On Fri, Aug 19, 2022 at 10:21 PM Matti Vaittinen
> >>> <mazziesaccount@gmail.com> wrote:
> >
> > What did I miss?
>
>  >>>>           struct bmg160_data *data;
>  >>>>           struct iio_dev *indio_dev;
>
> This does already violate the rule.

Indeed, I am reading this with an MTA that has True Type fonts, and I
can't see it at the first glance. But this breaks that rule slightly
while your added line breaks it significantly.

> >>> this case you even can move it out of the function, so we will see
> >>> clearly that this is (not a hidden) global variable.
> >>
> >> Here I do disagree with you. Moving the array out of the function makes
> >> it _much_ less obvious it is not used outside this function. Reason for
> >> making is "static const" is to allow the data be placed in read-only
> >> area (thanks to Guenter who originally gave me this tip).
> >
> > "static" in C language means two things (that's what come to my mind):
> > - for functions this tells that a function is not used outside of the module;
> > - for variables that it is a _global_ variable.
> >
> > Hiding static inside functions is not a good coding practice since it
> > hides scope of the variable.
>
> For const arrays the static in function does make sense. Being able to
> place the data in read-only areas do help with the memory on limited
> systems.

I'm not sure we are on the same page. I do not object to the "const"
part and we are _not_ talking about that.

> > And if you look into the kernel code, I
> > believe the use you are proposing is in minority.
>
> I don't know about the statistics. What I know is that we do have a
> technical benefits when we use static const arrays instead of non static
> ones in the functions. I do also believe placing the variables in blocks
> is a good practice.

Yes, and global variables are better to be seen as global variables.

> I tend to agree with you that using local, non const statics has
> pitfalls - but the pitfalls do not really apply with const ones. You
> know the value and have no races. Benefit is that just by seeing that no
> pointer is returned you can be sure that no "sane code" uses the data
> outside the function it resides.

Putting a global variable (const or non-const) to the function will
hide its scope and it is prone to getting two variables with the same
or very similar names with quite different semantics. That's why it's
really not good practice. I would rather see it outside of the
function _esp_ because it's static const.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 08/14] iio: bmg160_core: Simplify using devm_regulator_*get_enable()

[Matti Vaittinen]

On 8/20/22 10:18, Andy Shevchenko wrote:
> On Sat, Aug 20, 2022 at 9:48 AM Vaittinen, Matti
> <Matti.Vaittinen@fi.rohmeurope.com> wrote:
>> On 8/20/22 09:25, Andy Shevchenko wrote:
>>> On Sat, Aug 20, 2022 at 9:19 AM Vaittinen, Matti
>>> <Matti.Vaittinen@fi.rohmeurope.com> wrote:
>>>> On 8/20/22 02:30, Andy Shevchenko wrote:
>>>>> On Fri, Aug 19, 2022 at 10:21 PM Matti Vaittinen
>>>>> <mazziesaccount@gmail.com> wrote:
>>>
>>> What did I miss?
>>
>>   >>>>           struct bmg160_data *data;
>>   >>>>           struct iio_dev *indio_dev;
>>
>> This does already violate the rule.
> 
> Indeed, I am reading this with an MTA that has True Type fonts, and I
> can't see it at the first glance. But this breaks that rule slightly
> while your added line breaks it significantly.

Yes. As I said, I think the reverse xmas tree rule is not too well 
justified. Bunch of the subsystems do not really follow it, nor did this 
function. Yet, as I said, I can move the array to the first line in the 
function when I respin the series..

>>>>> this case you even can move it out of the function, so we will see
>>>>> clearly that this is (not a hidden) global variable.
>>>>
>>>> Here I do disagree with you. Moving the array out of the function makes
>>>> it _much_ less obvious it is not used outside this function. Reason for
>>>> making is "static const" is to allow the data be placed in read-only
>>>> area (thanks to Guenter who originally gave me this tip).

Just wanted to correct - it was Sebastian Reichel, not Guenter who 
explained me why doing local static const arrays is better than plain const.

>>>
>>> "static" in C language means two things (that's what come to my mind):
>>> - for functions this tells that a function is not used outside of the module;
>>> - for variables that it is a _global_ variable.
>>>
>>> Hiding static inside functions is not a good coding practice since it
>>> hides scope of the variable.
>>
>> For const arrays the static in function does make sense. Being able to
>> place the data in read-only areas do help with the memory on limited
>> systems.
> 
> I'm not sure we are on the same page. I do not object to the "const"
> part and we are _not_ talking about that.
> 

Maybe the explanation by Sebastian here can put us on the same page:
https://lore.kernel.org/all/20190502073539.GB7864@localhost.localdomain/
https://lore.kernel.org/all/322fa765ddd72972aba931c706657661ca685afa.camel@fi.rohmeurope.com/

>>> And if you look into the kernel code, I
>>> believe the use you are proposing is in minority.
>>
>> I don't know about the statistics. What I know is that we do have a
>> technical benefits when we use static const arrays instead of non static
>> ones in the functions. I do also believe placing the variables in blocks
>> is a good practice.
> 
> Yes, and global variables are better to be seen as global variables.
> 
>> I tend to agree with you that using local, non const statics has
>> pitfalls - but the pitfalls do not really apply with const ones. You
>> know the value and have no races. Benefit is that just by seeing that no
>> pointer is returned you can be sure that no "sane code" uses the data
>> outside the function it resides.
> 
> Putting a global variable (const or non-const) to the function will
> hide its scope and it is prone to getting two variables with the same
> or very similar names with quite different semantics.

I don't see how moving something from a local block to a global scope 
does make conflicts less of an issue? On the contrary, it makes things 
worse as then the moved variable will collide with any other variable in 
any of the functions in the whole file. Having the array as function 
local static makes the naming collisions to be issue only if another 
global variable has the same name. And if that happened - the chances 
are code would still be correct as the function here is clearly intended 
to use the local one. If someone really later adds a global with the 
same name - and uses the global in this function - then he should have 
noted we have local variable with same name. Additionally - such user 
would be using terribly bad name for a global variable.

Please note that scope of the function local static variable is limited 
to function even if the life-time is not just the life-time of a function.

> That's why it's
> really not good practice. I would rather see it outside of the
> function _esp_ because it's static const.

Sorry, I really don't agree with your reasoning here. :(


-- 
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: [PATCH v3 08/14] iio: bmg160_core: Simplify using devm_regulator_*get_enable()

[Andy Shevchenko]

On Sat, Aug 20, 2022 at 1:05 PM Matti Vaittinen
<mazziesaccount@gmail.com> wrote:
> On 8/20/22 10:18, Andy Shevchenko wrote:
> > On Sat, Aug 20, 2022 at 9:48 AM Vaittinen, Matti
> > <Matti.Vaittinen@fi.rohmeurope.com> wrote:
> >> On 8/20/22 09:25, Andy Shevchenko wrote:
> >>> On Sat, Aug 20, 2022 at 9:19 AM Vaittinen, Matti
> >>> <Matti.Vaittinen@fi.rohmeurope.com> wrote:
> >>>> On 8/20/22 02:30, Andy Shevchenko wrote:
> >>>>> On Fri, Aug 19, 2022 at 10:21 PM Matti Vaittinen
> >>>>> <mazziesaccount@gmail.com> wrote:
> >>>
> >>> What did I miss?
> >>
> >>   >>>>           struct bmg160_data *data;
> >>   >>>>           struct iio_dev *indio_dev;
> >>
> >> This does already violate the rule.
> >
> > Indeed, I am reading this with an MTA that has True Type fonts, and I
> > can't see it at the first glance. But this breaks that rule slightly
> > while your added line breaks it significantly.
>
> Yes. As I said, I think the reverse xmas tree rule is not too well
> justified. Bunch of the subsystems do not really follow it, nor did this
> function. Yet, as I said, I can move the array to the first line in the
> function when I respin the series..

You still can do better in _your_ series, right?

> >>>>> this case you even can move it out of the function, so we will see
> >>>>> clearly that this is (not a hidden) global variable.
> >>>>
> >>>> Here I do disagree with you. Moving the array out of the function makes
> >>>> it _much_ less obvious it is not used outside this function. Reason for
> >>>> making is "static const" is to allow the data be placed in read-only
> >>>> area (thanks to Guenter who originally gave me this tip).
>
> Just wanted to correct - it was Sebastian Reichel, not Guenter who
> explained me why doing local static const arrays is better than plain const.

Did he suggest putting it inside the function?

> >>> "static" in C language means two things (that's what come to my mind):
> >>> - for functions this tells that a function is not used outside of the module;
> >>> - for variables that it is a _global_ variable.
> >>>
> >>> Hiding static inside functions is not a good coding practice since it
> >>> hides scope of the variable.
> >>
> >> For const arrays the static in function does make sense. Being able to
> >> place the data in read-only areas do help with the memory on limited
> >> systems.
> >
> > I'm not sure we are on the same page. I do not object to the "const"
> > part and we are _not_ talking about that.
>
> Maybe the explanation by Sebastian here can put us on the same page:
> https://lore.kernel.org/all/20190502073539.GB7864@localhost.localdomain/
> https://lore.kernel.org/all/322fa765ddd72972aba931c706657661ca685afa.camel@fi.rohmeurope.com/

Again, you are too focused on "const", I'm talking about "static". The
above doesn't clear a bit regarding why you hide the global variable
inside a function. I don't see either Sebastian's clear point on this.

> >>> And if you look into the kernel code, I
> >>> believe the use you are proposing is in minority.
> >>
> >> I don't know about the statistics. What I know is that we do have a
> >> technical benefits when we use static const arrays instead of non static
> >> ones in the functions. I do also believe placing the variables in blocks
> >> is a good practice.
> >
> > Yes, and global variables are better to be seen as global variables.
> >
> >> I tend to agree with you that using local, non const statics has
> >> pitfalls - but the pitfalls do not really apply with const ones. You
> >> know the value and have no races. Benefit is that just by seeing that no
> >> pointer is returned you can be sure that no "sane code" uses the data
> >> outside the function it resides.
> >
> > Putting a global variable (const or non-const) to the function will
> > hide its scope and it is prone to getting two variables with the same
> > or very similar names with quite different semantics.
>
> I don't see how moving something from a local block to a global scope
> does make conflicts less of an issue?

You may add a static variable inside each function in the same module
and name it "foo" and there will be no conflict, but when you read the
code your brain will be spoiled. This is simply _bad code practice_. I
don't know how else I can explain this to you.

> On the contrary, it makes things
> worse as then the moved variable will collide with any other variable in
> any of the functions in the whole file. Having the array as function
> local static makes the naming collisions to be issue only if another
> global variable has the same name.

Again, you missed my point. I'm talking about reading and analysing
the code. Otherwise (good) compiler should spill a lot of warnings in
case you have global vs. local naming collision.

> And if that happened - the chances
> are code would still be correct as the function here is clearly intended
> to use the local one. If someone really later adds a global with the
> same name - and uses the global in this function - then he should have
> noted we have local variable with same name. Additionally - such user
> would be using terribly bad name for a global variable.
>
> Please note that scope of the function local static variable is limited
> to function even if the life-time is not just the life-time of a function.

Nope. The RO section might be very well flashed into ROM, so...

> > That's why it's
> > really not good practice. I would rather see it outside of the
> > function _esp_ because it's static const.
>
> Sorry, I really don't agree with your reasoning here. :(

So, whom should we listen to here? Because bad code is bad code. And
this is code above.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 08/14] iio: bmg160_core: Simplify using devm_regulator_*get_enable()

[Matti Vaittinen]

On 8/20/22 19:21, Andy Shevchenko wrote:
> On Sat, Aug 20, 2022 at 1:05 PM Matti Vaittinen
> <mazziesaccount@gmail.com> wrote:
>> On 8/20/22 10:18, Andy Shevchenko wrote:
>>> On Sat, Aug 20, 2022 at 9:48 AM Vaittinen, Matti
>>> <Matti.Vaittinen@fi.rohmeurope.com> wrote:
>>>> On 8/20/22 09:25, Andy Shevchenko wrote:
>>>>> On Sat, Aug 20, 2022 at 9:19 AM Vaittinen, Matti
>>>>> <Matti.Vaittinen@fi.rohmeurope.com> wrote:
>>>>>> On 8/20/22 02:30, Andy Shevchenko wrote:
>>>>>>> On Fri, Aug 19, 2022 at 10:21 PM Matti Vaittinen
>>>>>>> <mazziesaccount@gmail.com> wrote:
>>>>>
>>>>> What did I miss?
>>>>
>>>>    >>>>           struct bmg160_data *data;
>>>>    >>>>           struct iio_dev *indio_dev;
>>>>
>>>> This does already violate the rule.
>>>
>>> Indeed, I am reading this with an MTA that has True Type fonts, and I
>>> can't see it at the first glance. But this breaks that rule slightly
>>> while your added line breaks it significantly.
>>
>> Yes. As I said, I think the reverse xmas tree rule is not too well
>> justified. Bunch of the subsystems do not really follow it, nor did this
>> function. Yet, as I said, I can move the array to the first line in the
>> function when I respin the series..
> 
> You still can do better in _your_ series, right?

I don't see the benefit of the reverse xmas tree. We have discussed this 
already in the past :) I definitely have no need to start using reverse 
xmas tree thingee somewhere it has not been previously used. It may be 
better in _your_ opinion.

>>>>>>> this case you even can move it out of the function, so we will see
>>>>>>> clearly that this is (not a hidden) global variable.
>>>>>>
>>>>>> Here I do disagree with you. Moving the array out of the function makes
>>>>>> it _much_ less obvious it is not used outside this function. Reason for
>>>>>> making is "static const" is to allow the data be placed in read-only
>>>>>> area (thanks to Guenter who originally gave me this tip).
>>
>> Just wanted to correct - it was Sebastian Reichel, not Guenter who
>> explained me why doing local static const arrays is better than plain const.
> 
> Did he suggest putting it inside the function?

He asked me to convert a local array to static const. I though like you 
do now that the local array should not be static but just const - and he 
corrected me in his reply. This can be seen in the discussion I linked 
below.

>>>>> "static" in C language means two things (that's what come to my mind):
>>>>> - for functions this tells that a function is not used outside of the module;
>>>>> - for variables that it is a _global_ variable.
>>>>>
>>>>> Hiding static inside functions is not a good coding practice since it
>>>>> hides scope of the variable.
>>>>
>>>> For const arrays the static in function does make sense. Being able to
>>>> place the data in read-only areas do help with the memory on limited
>>>> systems.
>>>
>>> I'm not sure we are on the same page. I do not object to the "const"
>>> part and we are _not_ talking about that.
>>
>> Maybe the explanation by Sebastian here can put us on the same page:
>> https://lore.kernel.org/all/20190502073539.GB7864@localhost.localdomain/
>> https://lore.kernel.org/all/322fa765ddd72972aba931c706657661ca685afa.camel@fi.rohmeurope.com/
> 
> Again, you are too focused on "const", I'm talking about "static". The
> above doesn't clear a bit regarding why you hide the global variable
> inside a function. I don't see either Sebastian's clear point on this.

I don't really see why you talk about "hiding a global variable in a 
function"? A static variable which is declared in function is not 
global. It is local. It causes no more name collisions than a regular 
local variable does so I really don't understand your reasoning.

>>>>> And if you look into the kernel code, I
>>>>> believe the use you are proposing is in minority.
>>>>
>>>> I don't know about the statistics. What I know is that we do have a
>>>> technical benefits when we use static const arrays instead of non static
>>>> ones in the functions. I do also believe placing the variables in blocks
>>>> is a good practice.
>>>
>>> Yes, and global variables are better to be seen as global variables.
>>>
>>>> I tend to agree with you that using local, non const statics has
>>>> pitfalls - but the pitfalls do not really apply with const ones. You
>>>> know the value and have no races. Benefit is that just by seeing that no
>>>> pointer is returned you can be sure that no "sane code" uses the data
>>>> outside the function it resides.
>>>
>>> Putting a global variable (const or non-const) to the function will
>>> hide its scope and it is prone to getting two variables with the same
>>> or very similar names with quite different semantics.
>>
>> I don't see how moving something from a local block to a global scope
>> does make conflicts less of an issue?
> 
> You may add a static variable inside each function in the same module
> and name it "foo" and there will be no conflict, but when you read the
> code your brain will be spoiled.

And how is it different from reading functions where the regular 
variables have identical names? I _really_ can't follow your reasoning.

> This is simply _bad code practice_. I
> don't know how else I can explain this to you.
> 
>> On the contrary, it makes things
>> worse as then the moved variable will collide with any other variable in
>> any of the functions in the whole file. Having the array as function
>> local static makes the naming collisions to be issue only if another
>> global variable has the same name.
> 
> Again, you missed my point. I'm talking about reading and analysing
> the code.

I _definitely_ miss your point here. I have zero problems reading code 
where static const variables are used in a function. I think it is 
pretty much as hard as seeing a #defined value - difference being that 
one can point to the variable.

I admit that static variables whose value is changed can be more of a 
problem especially when access to function is not serialized.

> Otherwise (good) compiler should spill a lot of warnings in
> case you have global vs. local naming collision.
> 
>> And if that happened - the chances
>> are code would still be correct as the function here is clearly intended
>> to use the local one. If someone really later adds a global with the
>> same name - and uses the global in this function - then he should have
>> noted we have local variable with same name. Additionally - such user
>> would be using terribly bad name for a global variable.
>>
>> Please note that scope of the function local static variable is limited
>> to function even if the life-time is not just the life-time of a function.
> 
> Nope. The RO section might be very well flashed into ROM, so...

...so?

>>> That's why it's
>>> really not good practice. I would rather see it outside of the
>>> function _esp_ because it's static const.
>>
>> Sorry, I really don't agree with your reasoning here. :(
> 
> So, whom should we listen to here? Because bad code is bad code. And
> this is code above.

Bad is a subjective concept. I'd say the code gets much worse if we make 
the local variable a global one.

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

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

Re: [PATCH v3 08/14] iio: bmg160_core: Simplify using devm_regulator_*get_enable()

[Andy Shevchenko]

On Sat, Aug 20, 2022 at 8:27 PM Matti Vaittinen
<mazziesaccount@gmail.com> wrote:
> On 8/20/22 19:21, Andy Shevchenko wrote:
> > On Sat, Aug 20, 2022 at 1:05 PM Matti Vaittinen
> > <mazziesaccount@gmail.com> wrote:
> >> On 8/20/22 10:18, Andy Shevchenko wrote:
> >>> On Sat, Aug 20, 2022 at 9:48 AM Vaittinen, Matti
> >>> <Matti.Vaittinen@fi.rohmeurope.com> wrote:
> >>>> On 8/20/22 09:25, Andy Shevchenko wrote:
> >>>>> On Sat, Aug 20, 2022 at 9:19 AM Vaittinen, Matti
> >>>>> <Matti.Vaittinen@fi.rohmeurope.com> wrote:
> >>>>>> On 8/20/22 02:30, Andy Shevchenko wrote:
> >>>>>>> On Fri, Aug 19, 2022 at 10:21 PM Matti Vaittinen
> >>>>>>> <mazziesaccount@gmail.com> wrote:
> >>>>>
> >>>>> What did I miss?
> >>>>
> >>>>    >>>>           struct bmg160_data *data;
> >>>>    >>>>           struct iio_dev *indio_dev;
> >>>>
> >>>> This does already violate the rule.
> >>>
> >>> Indeed, I am reading this with an MTA that has True Type fonts, and I
> >>> can't see it at the first glance. But this breaks that rule slightly
> >>> while your added line breaks it significantly.
> >>
> >> Yes. As I said, I think the reverse xmas tree rule is not too well
> >> justified. Bunch of the subsystems do not really follow it, nor did this
> >> function. Yet, as I said, I can move the array to the first line in the
> >> function when I respin the series..
> >
> > You still can do better in _your_ series, right?
>
> I don't see the benefit of the reverse xmas tree. We have discussed this
> already in the past :) I definitely have no need to start using reverse
> xmas tree thingee somewhere it has not been previously used. It may be
> better in _your_ opinion.

Yes, this is a style issue and not a real coding problem. As a
reviewer with some of a background I feel that reversed xmas tree
ordering is better to read, maintain, and review. So, as a reviewer I
have an opinion, as a maintainer I can speak for the IIO subsystem,
since it's not on my watch.

> >>>>>>> this case you even can move it out of the function, so we will see
> >>>>>>> clearly that this is (not a hidden) global variable.
> >>>>>>
> >>>>>> Here I do disagree with you. Moving the array out of the function makes
> >>>>>> it _much_ less obvious it is not used outside this function. Reason for
> >>>>>> making is "static const" is to allow the data be placed in read-only
> >>>>>> area (thanks to Guenter who originally gave me this tip).
> >>
> >> Just wanted to correct - it was Sebastian Reichel, not Guenter who
> >> explained me why doing local static const arrays is better than plain const.
> >
> > Did he suggest putting it inside the function?
>
> He asked me to convert a local array to static const. I though like you
> do now that the local array should not be static but just const - and he
> corrected me in his reply. This can be seen in the discussion I linked
> below.

Yes, and it's irrelevant to what I'm trying to tell you.

> >>>>> "static" in C language means two things (that's what come to my mind):
> >>>>> - for functions this tells that a function is not used outside of the module;
> >>>>> - for variables that it is a _global_ variable.
> >>>>>
> >>>>> Hiding static inside functions is not a good coding practice since it
> >>>>> hides scope of the variable.
> >>>>
> >>>> For const arrays the static in function does make sense. Being able to
> >>>> place the data in read-only areas do help with the memory on limited
> >>>> systems.
> >>>
> >>> I'm not sure we are on the same page. I do not object to the "const"
> >>> part and we are _not_ talking about that.
> >>
> >> Maybe the explanation by Sebastian here can put us on the same page:
> >> https://lore.kernel.org/all/20190502073539.GB7864@localhost.localdomain/
> >> https://lore.kernel.org/all/322fa765ddd72972aba931c706657661ca685afa.camel@fi.rohmeurope.com/
> >
> > Again, you are too focused on "const", I'm talking about "static". The
> > above doesn't clear a bit regarding why you hide the global variable
> > inside a function. I don't see either Sebastian's clear point on this.
>
> I don't really see why you talk about "hiding a global variable in a
> function"? A static variable which is declared in function is not
> global. It is local.

SInce it's static it's global by nature, but local by namespace.

> It causes no more name collisions than a regular
> local variable does so I really don't understand your reasoning.

And I have no other words to explain it to you. You are using a global
variable in the scope of function. This is not good for the
maintenance and development as it's prone to get an issue in the
future.

> >>>>> And if you look into the kernel code, I
> >>>>> believe the use you are proposing is in minority.
> >>>>
> >>>> I don't know about the statistics. What I know is that we do have a
> >>>> technical benefits when we use static const arrays instead of non static
> >>>> ones in the functions. I do also believe placing the variables in blocks
> >>>> is a good practice.
> >>>
> >>> Yes, and global variables are better to be seen as global variables.
> >>>
> >>>> I tend to agree with you that using local, non const statics has
> >>>> pitfalls - but the pitfalls do not really apply with const ones. You
> >>>> know the value and have no races. Benefit is that just by seeing that no
> >>>> pointer is returned you can be sure that no "sane code" uses the data
> >>>> outside the function it resides.
> >>>
> >>> Putting a global variable (const or non-const) to the function will
> >>> hide its scope and it is prone to getting two variables with the same
> >>> or very similar names with quite different semantics.
> >>
> >> I don't see how moving something from a local block to a global scope
> >> does make conflicts less of an issue?
> >
> > You may add a static variable inside each function in the same module
> > and name it "foo" and there will be no conflict, but when you read the
> > code your brain will be spoiled.
>
> And how is it different from reading functions where the regular
> variables have identical names? I _really_ can't follow your reasoning.

Because they are on the stack and not one per module. When you read
the code it's very easy to miss that the variable is static if you
have a lot of other variables defined.

> > This is simply _bad code practice_. I
> > don't know how else I can explain this to you.
> >
> >> On the contrary, it makes things
> >> worse as then the moved variable will collide with any other variable in
> >> any of the functions in the whole file. Having the array as function
> >> local static makes the naming collisions to be issue only if another
> >> global variable has the same name.
> >
> > Again, you missed my point. I'm talking about reading and analysing
> > the code.
>
> I _definitely_ miss your point here. I have zero problems reading code
> where static const variables are used in a function. I think it is
> pretty much as hard as seeing a #defined value - difference being that
> one can point to the variable.

Good for you.

> I admit that static variables whose value is changed can be more of a
> problem especially when access to function is not serialized.
>
> > Otherwise (good) compiler should spill a lot of warnings in
> > case you have global vs. local naming collision.
> >
> >> And if that happened - the chances
> >> are code would still be correct as the function here is clearly intended
> >> to use the local one. If someone really later adds a global with the
> >> same name - and uses the global in this function - then he should have
> >> noted we have local variable with same name. Additionally - such user
> >> would be using terribly bad name for a global variable.
> >>
> >> Please note that scope of the function local static variable is limited
> >> to function even if the life-time is not just the life-time of a function.
> >
> > Nope. The RO section might be very well flashed into ROM, so...
>
> ...so?

It won't be created by function, it's created by the compiler /
linker. It won't gone if function gone. e.g.

__init foo()
{
  static bar ...;
}

is nonsense. And it takes a ROM space.

> >>> That's why it's
> >>> really not good practice. I would rather see it outside of the
> >>> function _esp_ because it's static const.
> >>
> >> Sorry, I really don't agree with your reasoning here. :(
> >
> > So, whom should we listen to here? Because bad code is bad code. And
> > this is code above.
>
> Bad is a subjective concept. I'd say the code gets much worse if we make
> the local variable a global one.

...


To summarize, we have a huge disagreement on the placement of the
static variables. Not sure we ever get into compromize here, so I
leave it up to maintainers, but my opinion that it is simply a bad
code practice.

--
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 08/14] iio: bmg160_core: Simplify using devm_regulator_*get_enable()

[Vaittinen, Matti]

On 8/21/22 16:08, Andy Shevchenko wrote:
> On Sat, Aug 20, 2022 at 8:27 PM Matti Vaittinen
> <mazziesaccount@gmail.com> wrote:
>> On 8/20/22 19:21, Andy Shevchenko wrote:
>>> On Sat, Aug 20, 2022 at 1:05 PM Matti Vaittinen
>>> <mazziesaccount@gmail.com> wrote:
>>>> On 8/20/22 10:18, Andy Shevchenko wrote:
>>>>> On Sat, Aug 20, 2022 at 9:48 AM Vaittinen, Matti
>>>>> <Matti.Vaittinen@fi.rohmeurope.com> wrote:
>>>>>> On 8/20/22 09:25, Andy Shevchenko wrote:
>>>>>>> On Sat, Aug 20, 2022 at 9:19 AM Vaittinen, Matti
>>>>>>> <Matti.Vaittinen@fi.rohmeurope.com> wrote:
>>>>>>>> On 8/20/22 02:30, Andy Shevchenko wrote:
>>>>>>>>> On Fri, Aug 19, 2022 at 10:21 PM Matti Vaittinen
>>>>>>>>> <mazziesaccount@gmail.com> wrote:

//snip

> SInce it's static it's global by nature, but local by namespace.

Which is an _improvement_ over having it in global namespace?


>> It causes no more name collisions than a regular
>> local variable does so I really don't understand your reasoning.
> 
> And I have no other words to explain it to you. You are using a global
> variable in the scope of function. This is not good for the
> maintenance and development as it's prone to get an issue in the
> future.

If you foresee some issues, please describe them as I don't see one 
single problem with a local static const data. I have seen you telling 
me that "static const" variables are _harder_ for you to review. Could 
you please explain what are the potential mistake(s) a reviewer can do, 
and what is the 'issue' that mistake can cause?

>>> So, whom should we listen to here? Because bad code is bad code. And
>>> this is code above.
>>
>> Bad is a subjective concept. I'd say the code gets much worse if we make
>> the local variable a global one.
> 
> ...
> 
> 
> To summarize, we have a huge disagreement on the placement of the
> static variables. Not sure we ever get into compromize here, so I
> leave it up to maintainers, but my opinion that it is simply a bad
> code practice.

Bad and good are labels we can place on things. We however need to have 
the reason for those labels to be meaningful. I am sorry but I don't 
want to label the local _const_ static variables bad without reason. 
This discussion starts to remind me on statements like "using goto is 
always bad" or "one must never use macros in C".

Yeah - ultimately it is a maintainer decision.

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: [PATCH v3 08/14] iio: bmg160_core: Simplify using devm_regulator_*get_enable()

[Jonathan Cameron]

On Sat, 20 Aug 2022 06:19:00 +0000
"Vaittinen, Matti" <Matti.Vaittinen@fi.rohmeurope.com> wrote:

> Thanks for the review Andy
> 
> On 8/20/22 02:30, Andy Shevchenko wrote:
> > On Fri, Aug 19, 2022 at 10:21 PM Matti Vaittinen
> > <mazziesaccount@gmail.com> wrote:  
> >>
> >> Use devm_regulator_bulk_get_enable() instead of open coded bulk-get,
> >> bulk-enable, add-action-to-disable-at-detach - pattern.  
> > 
> > ...
> >   
> >>   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"};  
> > 
> > Please, keep this following the "longest line first" rule. Note, in  
> 
> This was not following the (IMO slightly silly) rule even prior my 
> patch. I can for sure move my line up - but that won't give you the 
> "reverse X-mas tree".
> 
> I don't have any real objections on changing the styling though - I 
> don't expect this to be merged before the dependency is in rc1 - so I 
> guess I will anyways need to respin this for next cycle. I can do the 
> styling then.
I was a bit surprised Mark didn't do an immutable branch for this, but
indeed looks like it's going to be a multiple cycle thing - so we'll
probably have a bunch of new cases introduced in the meantime that
we need to tidy up.  Ah well.

> 
> > this case you even can move it out of the function, so we will see
> > clearly that this is (not a hidden) global variable.  
> 
> Here I do disagree with you. Moving the array out of the function makes 
> it _much_ less obvious it is not used outside this function. Reason for 
> making is "static const" is to allow the data be placed in read-only 
> area (thanks to Guenter who originally gave me this tip).
> 
> > P.S. Same applies for the rest of the similar places in your series.
> >   
> 
> Br,
> 	-- Matti
> 

Re: [PATCH v3 08/14] iio: bmg160_core: Simplify using devm_regulator_*get_enable()

[Matti Vaittinen]

On 8/20/22 14:38, Jonathan Cameron wrote:
> On Sat, 20 Aug 2022 06:19:00 +0000
> "Vaittinen, Matti" <Matti.Vaittinen@fi.rohmeurope.com> wrote:
> 
>> Thanks for the review Andy
>>
>> On 8/20/22 02:30, Andy Shevchenko wrote:
>>> On Fri, Aug 19, 2022 at 10:21 PM Matti Vaittinen
>>> <mazziesaccount@gmail.com> wrote:
>> >>I
>> don't expect this to be merged before the dependency is in rc1 - so I
>> guess I will anyways need to respin this for next cycle. I can do the
>> styling then.
> I was a bit surprised Mark didn't do an immutable branch for this, but
> indeed looks like it's going to be a multiple cycle thing - so we'll
> probably have a bunch of new cases introduced in the meantime that
> we need to tidy up.  Ah well.
>

I guess we can ask Mark what he thinks of the immutable branch ;) I 
admit I am not too keen on rebasing this - especially if I find the time 
to go through more drivers.

-- 
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: [PATCH v3 08/14] iio: bmg160_core: Simplify using devm_regulator_*get_enable()

[Jonathan Cameron]

On Fri, 19 Aug 2022 22:19:31 +0300
Matti Vaittinen <mazziesaccount@gmail.com> wrote:

> Use devm_regulator_bulk_get_enable() instead of open coded bulk-get,
> bulk-enable, add-action-to-disable-at-detach - pattern.
> 
> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
> 
> ---
> v2 => v3
> Split to own patch.
> ---
>  drivers/iio/gyro/bmg160_core.c | 24 +++---------------------
>  1 file changed, 3 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/iio/gyro/bmg160_core.c b/drivers/iio/gyro/bmg160_core.c
> index cedd9f02ea21..baa80980c99f 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"};

As in previous, small preference for spaces after { and before }

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

Re: [PATCH v3 08/14] iio: bmg160_core: Simplify using devm_regulator_*get_enable()

[Matti Vaittinen]

On 8/20/22 14:22, Jonathan Cameron wrote:
> On Fri, 19 Aug 2022 22:19:31 +0300
> Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> 
>> Use devm_regulator_bulk_get_enable() instead of open coded bulk-get,
>> bulk-enable, add-action-to-disable-at-detach - pattern.
>>
>> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
>>
>> ---
>> v2 => v3
>> Split to own patch.
>> ---
>>   drivers/iio/gyro/bmg160_core.c | 24 +++---------------------
>>   1 file changed, 3 insertions(+), 21 deletions(-)
>>
>> diff --git a/drivers/iio/gyro/bmg160_core.c b/drivers/iio/gyro/bmg160_core.c
>> index cedd9f02ea21..baa80980c99f 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"};
> 
> As in previous, small preference for spaces after { and before }

Thanks. I'll fix it when I respin.


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

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

Re: [PATCH v3 08/14] iio: bmg160_core: Simplify using devm_regulator_*get_enable()

[Jonathan Cameron]

On Fri, 19 Aug 2022 22:19:31 +0300
Matti Vaittinen <mazziesaccount@gmail.com> wrote:

> Use devm_regulator_bulk_get_enable() instead of open coded bulk-get,
> bulk-enable, add-action-to-disable-at-detach - pattern.
> 
> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>

Applied with tweaks:
- patch title includes gyro:
- ordering as Andy suggested
- spaces after { and before }
> 
> ---
> v2 => v3
> Split to own patch.
> ---
>  drivers/iio/gyro/bmg160_core.c | 24 +++---------------------
>  1 file changed, 3 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/iio/gyro/bmg160_core.c b/drivers/iio/gyro/bmg160_core.c
> index cedd9f02ea21..baa80980c99f 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;

Re: [PATCH v3 08/14] iio: bmg160_core: Simplify using devm_regulator_*get_enable()

[Matti Vaittinen]

On 10/16/22 19:08, Jonathan Cameron wrote:
> On Fri, 19 Aug 2022 22:19:31 +0300
> Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> 
>> Use devm_regulator_bulk_get_enable() instead of open coded bulk-get,
>> bulk-enable, add-action-to-disable-at-detach - pattern.
>>
>> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
> 
> Applied with tweaks:
> - patch title includes gyro:
> - ordering as Andy suggested
> - spaces after { and before }

Thanks Jonathan for saving me from respin the series :) Much apprecited!

Yours
	-- Matti

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

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

[PATCH v3 09/14] iio: st_lsm6dsx: Simplify using devm_regulator_*get_enable()

[Matti Vaittinen]

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

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>

---
v2 => v3
Split to own patch.
---
 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h      |  2 --
 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 30 ++++----------------
 2 files changed, 5 insertions(+), 27 deletions(-)

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 b5e4a4113652..c328d1a36d34 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 v3 09/14] iio: st_lsm6dsx: Simplify using devm_regulator_*get_enable()

[Jonathan Cameron]

On Fri, 19 Aug 2022 22:19:46 +0300
Matti Vaittinen <mazziesaccount@gmail.com> wrote:

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

Applied with similar ordering, patch naming and spacing tweaks to earlier patches.

> 
> ---
> v2 => v3
> Split to own patch.
> ---
>  drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h      |  2 --
>  drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 30 ++++----------------
>  2 files changed, 5 insertions(+), 27 deletions(-)
> 
> 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 b5e4a4113652..c328d1a36d34 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;

[PATCH v3 10/14] iio: ad7476: simplify using devm_regulator_get_enable()

[Matti Vaittinen]

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

---
v2 => v3:
New patch
---
 drivers/iio/adc/ad7476.c | 11 +----------
 1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/drivers/iio/adc/ad7476.c b/drivers/iio/adc/ad7476.c
index 94776f696290..80aebed47d1f 100644
--- a/drivers/iio/adc/ad7476.c
+++ b/drivers/iio/adc/ad7476.c
@@ -368,16 +368,7 @@ static int ad7476_probe(struct spi_device *spi)
 	}
 
 	if (st->chip_info->has_vdrive) {
-		reg = devm_regulator_get(&spi->dev, "vdrive");
-		if (IS_ERR(reg))
-			return PTR_ERR(reg);
-
-		ret = regulator_enable(reg);
-		if (ret)
-			return ret;
-
-		ret = devm_add_action_or_reset(&spi->dev, ad7476_reg_disable,
-					       reg);
+		ret = devm_regulator_get_enable(&spi->dev, "vdrive");
 		if (ret)
 			return ret;
 	}
-- 
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 v3 10/14] iio: ad7476: simplify using devm_regulator_get_enable()

[Sa, Nuno]

> -----Original Message-----
> From: Matti Vaittinen <mazziesaccount@gmail.com>
> Sent: Friday, August 19, 2022 9:20 PM
> To: Matti Vaittinen <mazziesaccount@gmail.com>; Matti Vaittinen
> <matti.vaittinen@fi.rohmeurope.com>
> Cc: Lars-Peter Clausen <lars@metafoo.de>; Hennerich, Michael
> <Michael.Hennerich@analog.com>; Jonathan Cameron
> <jic23@kernel.org>; linux-iio@vger.kernel.org; linux-
> kernel@vger.kernel.org
> Subject: [PATCH v3 10/14] iio: ad7476: simplify using
> devm_regulator_get_enable()
> 
> [External]
> 
> 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: Nuno Sá <nuno.sa@analog.com>

Re: [PATCH v3 10/14] iio: ad7476: simplify using devm_regulator_get_enable()

[Jonathan Cameron]

On Tue, 30 Aug 2022 11:44:28 +0000
"Sa, Nuno" <Nuno.Sa@analog.com> wrote:

> > -----Original Message-----
> > From: Matti Vaittinen <mazziesaccount@gmail.com>
> > Sent: Friday, August 19, 2022 9:20 PM
> > To: Matti Vaittinen <mazziesaccount@gmail.com>; Matti Vaittinen
> > <matti.vaittinen@fi.rohmeurope.com>
> > Cc: Lars-Peter Clausen <lars@metafoo.de>; Hennerich, Michael
> > <Michael.Hennerich@analog.com>; Jonathan Cameron
> > <jic23@kernel.org>; linux-iio@vger.kernel.org; linux-
> > kernel@vger.kernel.org
> > Subject: [PATCH v3 10/14] iio: ad7476: simplify using
> > devm_regulator_get_enable()
> > 
> > [External]
> > 
> > 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: Nuno Sá <nuno.sa@analog.com>

Applied

[PATCH v3 11/14] iio: ad7606: simplify using devm_regulator_get_enable()

[Matti Vaittinen]

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

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 <mazziesaccount@gmail.com>

---
v2 => v3:
New patch
---
 drivers/iio/adc/ad7606.c | 22 +++-------------------
 drivers/iio/adc/ad7606.h |  1 -
 2 files changed, 3 insertions(+), 20 deletions(-)

diff --git a/drivers/iio/adc/ad7606.c b/drivers/iio/adc/ad7606.c
index ba24f99523e0..dd6b603f65ea 100644
--- a/drivers/iio/adc/ad7606.c
+++ b/drivers/iio/adc/ad7606.c
@@ -557,13 +557,6 @@ static const struct iio_trigger_ops ad7606_trigger_ops = {
 	.validate_device = iio_trigger_validate_own_device,
 };
 
-static void ad7606_regulator_disable(void *data)
-{
-	struct ad7606_state *st = data;
-
-	regulator_disable(st->reg);
-}
-
 int ad7606_probe(struct device *dev, int irq, void __iomem *base_address,
 		 const char *name, unsigned int id,
 		 const struct ad7606_bus_ops *bops)
@@ -589,19 +582,10 @@ int ad7606_probe(struct device *dev, int irq, void __iomem *base_address,
 	st->scale_avail = ad7606_scale_avail;
 	st->num_scales = ARRAY_SIZE(ad7606_scale_avail);
 
-	st->reg = devm_regulator_get(dev, "avcc");
-	if (IS_ERR(st->reg))
-		return PTR_ERR(st->reg);
-
-	ret = regulator_enable(st->reg);
-	if (ret) {
-		dev_err(dev, "Failed to enable specified AVcc supply\n");
-		return ret;
-	}
-
-	ret = devm_add_action_or_reset(dev, ad7606_regulator_disable, st);
+	ret = devm_regulator_get_enable(dev, "avcc");
 	if (ret)
-		return ret;
+		return dev_err_probe(dev, ret,
+				     "Failed to enable specified AVcc supply\n");
 
 	st->chip_info = &ad7606_chip_info_tbl[id];
 
diff --git a/drivers/iio/adc/ad7606.h b/drivers/iio/adc/ad7606.h
index 2dc4f599f9df..924a2d1a6d94 100644
--- a/drivers/iio/adc/ad7606.h
+++ b/drivers/iio/adc/ad7606.h
@@ -92,7 +92,6 @@ struct ad7606_chip_info {
 struct ad7606_state {
 	struct device			*dev;
 	const struct ad7606_chip_info	*chip_info;
-	struct regulator		*reg;
 	const struct ad7606_bus_ops	*bops;
 	unsigned int			range[16];
 	unsigned int			oversampling;
-- 
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 v3 11/14] iio: ad7606: simplify using devm_regulator_get_enable()

[Sa, Nuno]

> From: Matti Vaittinen <mazziesaccount@gmail.com>
> Sent: Friday, August 19, 2022 9:20 PM
> To: Matti Vaittinen <mazziesaccount@gmail.com>; Matti Vaittinen
> <matti.vaittinen@fi.rohmeurope.com>
> Cc: Lars-Peter Clausen <lars@metafoo.de>; Hennerich, Michael
> <Michael.Hennerich@analog.com>; Jonathan Cameron
> <jic23@kernel.org>; linux-iio@vger.kernel.org; linux-
> kernel@vger.kernel.org
> Subject: [PATCH v3 11/14] iio: ad7606: simplify using
> devm_regulator_get_enable()
> 
> [External]
> 
> 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 <mazziesaccount@gmail.com>
> 
> ---

The commit message could state that while doing the change, dev_err_probe()
was also introduced. Bah, anyways:

Acked-by: Nuno Sá <nuno.sa@analog.com>

Re: [PATCH v3 11/14] iio: ad7606: simplify using devm_regulator_get_enable()

[Matti Vaittinen]

Thanks for the review(s) Nuno!

On 8/30/22 14:46, Sa, Nuno wrote:
>> From: Matti Vaittinen <mazziesaccount@gmail.com>
>> Sent: Friday, August 19, 2022 9:20 PM
>> To: Matti Vaittinen <mazziesaccount@gmail.com>; Matti Vaittinen
>> <matti.vaittinen@fi.rohmeurope.com>
>> Cc: Lars-Peter Clausen <lars@metafoo.de>; Hennerich, Michael
>> <Michael.Hennerich@analog.com>; Jonathan Cameron
>> <jic23@kernel.org>; linux-iio@vger.kernel.org; linux-
>> kernel@vger.kernel.org
>> Subject: [PATCH v3 11/14] iio: ad7606: simplify using
>> devm_regulator_get_enable()
>>
>> [External]
>>
>> 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 <mazziesaccount@gmail.com>
>>
>> ---
> 
> The commit message could state that while doing the change, dev_err_probe()
> was also introduced. Bah, anyways:
> 
> Acked-by: Nuno Sá <nuno.sa@analog.com>

Good point. I have few other changes to the series pending - and I 
probably need to rebase/respin when -rc1 is out (and dependency patches 
are merged from Mark's tree) => I may as well alter the commit message.

Yours,
	-- Matti

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

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

Re: [PATCH v3 11/14] iio: ad7606: simplify using devm_regulator_get_enable()

[Jonathan Cameron]

On Tue, 30 Aug 2022 15:54:07 +0300
Matti Vaittinen <mazziesaccount@gmail.com> wrote:

> Thanks for the review(s) Nuno!
> 
> On 8/30/22 14:46, Sa, Nuno wrote:
> >> From: Matti Vaittinen <mazziesaccount@gmail.com>
> >> Sent: Friday, August 19, 2022 9:20 PM
> >> To: Matti Vaittinen <mazziesaccount@gmail.com>; Matti Vaittinen
> >> <matti.vaittinen@fi.rohmeurope.com>
> >> Cc: Lars-Peter Clausen <lars@metafoo.de>; Hennerich, Michael
> >> <Michael.Hennerich@analog.com>; Jonathan Cameron
> >> <jic23@kernel.org>; linux-iio@vger.kernel.org; linux-
> >> kernel@vger.kernel.org
> >> Subject: [PATCH v3 11/14] iio: ad7606: simplify using
> >> devm_regulator_get_enable()
> >>
> >> [External]
> >>
> >> 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 <mazziesaccount@gmail.com>
> >>
> >> ---  
> > 
> > The commit message could state that while doing the change, dev_err_probe()
> > was also introduced. Bah, anyways:
> > 
> > Acked-by: Nuno Sá <nuno.sa@analog.com>  
> 
> Good point. I have few other changes to the series pending - and I 
> probably need to rebase/respin when -rc1 is out (and dependency patches 
> are merged from Mark's tree) => I may as well alter the commit message.
> 
I tweaked it and applied.

Not I'm grabbing these early because I forgot you'd sent them and
found myself writing the same patches.  Memory of a goldfish :)

Jonathan

> Yours,
> 	-- Matti
> 

Re: [PATCH v3 11/14] iio: ad7606: simplify using devm_regulator_get_enable()

[Jonathan Cameron]

On Sun, 16 Oct 2022 17:15:29 +0100
Jonathan Cameron <jic23@kernel.org> wrote:

> On Tue, 30 Aug 2022 15:54:07 +0300
> Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> 
> > Thanks for the review(s) Nuno!
> > 
> > On 8/30/22 14:46, Sa, Nuno wrote:  
> > >> From: Matti Vaittinen <mazziesaccount@gmail.com>
> > >> Sent: Friday, August 19, 2022 9:20 PM
> > >> To: Matti Vaittinen <mazziesaccount@gmail.com>; Matti Vaittinen
> > >> <matti.vaittinen@fi.rohmeurope.com>
> > >> Cc: Lars-Peter Clausen <lars@metafoo.de>; Hennerich, Michael
> > >> <Michael.Hennerich@analog.com>; Jonathan Cameron
> > >> <jic23@kernel.org>; linux-iio@vger.kernel.org; linux-
> > >> kernel@vger.kernel.org
> > >> Subject: [PATCH v3 11/14] iio: ad7606: simplify using
> > >> devm_regulator_get_enable()
> > >>
> > >> [External]
> > >>
> > >> 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 <mazziesaccount@gmail.com>
> > >>
> > >> ---    
> > > 
> > > The commit message could state that while doing the change, dev_err_probe()
> > > was also introduced. Bah, anyways:
> > > 
> > > Acked-by: Nuno Sá <nuno.sa@analog.com>    
> > 
> > Good point. I have few other changes to the series pending - and I 
> > probably need to rebase/respin when -rc1 is out (and dependency patches 
> > are merged from Mark's tree) => I may as well alter the commit message.
> >   
> I tweaked it and applied.
> 
> Not I'm grabbing these early because I forgot you'd sent them and
> found myself writing the same patches.  Memory of a goldfish :)

Tweaked a little more - you missed that the struct in the header had
kernel-doc for the struct regulator *.  Dropped that.

> 
> Jonathan
> 
> > Yours,
> > 	-- Matti
> >   
> 

Re: [PATCH v3 11/14] iio: ad7606: simplify using devm_regulator_get_enable()

[Matti Vaittinen]

On 10/16/22 19:24, Jonathan Cameron wrote:
> On Sun, 16 Oct 2022 17:15:29 +0100
> Jonathan Cameron <jic23@kernel.org> wrote:
> 
>> On Tue, 30 Aug 2022 15:54:07 +0300
>> Matti Vaittinen <mazziesaccount@gmail.com> wrote:
>>
>>> Thanks for the review(s) Nuno!
>>>
>>> On 8/30/22 14:46, Sa, Nuno wrote:
>>>>> From: Matti Vaittinen <mazziesaccount@gmail.com>
>>>>> Sent: Friday, August 19, 2022 9:20 PM
>>>>> To: Matti Vaittinen <mazziesaccount@gmail.com>; Matti Vaittinen
>>>>> <matti.vaittinen@fi.rohmeurope.com>
>>>>> Cc: Lars-Peter Clausen <lars@metafoo.de>; Hennerich, Michael
>>>>> <Michael.Hennerich@analog.com>; Jonathan Cameron
>>>>> <jic23@kernel.org>; linux-iio@vger.kernel.org; linux-
>>>>> kernel@vger.kernel.org
>>>>> Subject: [PATCH v3 11/14] iio: ad7606: simplify using
>>>>> devm_regulator_get_enable()
>>>>>
>>>>> [External]
>>>>>
>>>>> 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 <mazziesaccount@gmail.com>
>>>>>
>>>>> ---
>>>>
>>>> The commit message could state that while doing the change, dev_err_probe()
>>>> was also introduced. Bah, anyways:
>>>>
>>>> Acked-by: Nuno Sá <nuno.sa@analog.com>
>>>
>>> Good point. I have few other changes to the series pending - and I
>>> probably need to rebase/respin when -rc1 is out (and dependency patches
>>> are merged from Mark's tree) => I may as well alter the commit message.
>>>    
>> I tweaked it and applied.
>>
>> Not I'm grabbing these early because I forgot you'd sent them and
>> found myself writing the same patches.  Memory of a goldfish :)
> 
> Tweaked a little more - you missed that the struct in the header had
> kernel-doc for the struct regulator *.  Dropped that.

Thanks a bunch!

Yours,
	Matti

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

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

[PATCH v3 12/14] iio: max1241: simplify using devm_regulator_get_enable()

[Matti Vaittinen]

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

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 <mazziesaccount@gmail.com>

---
v2 => v3:
New patch
---
 drivers/iio/adc/max1241.c | 28 +++-------------------------
 1 file changed, 3 insertions(+), 25 deletions(-)

diff --git a/drivers/iio/adc/max1241.c b/drivers/iio/adc/max1241.c
index a815ad1f6913..500bb09ab19b 100644
--- a/drivers/iio/adc/max1241.c
+++ b/drivers/iio/adc/max1241.c
@@ -22,7 +22,6 @@ enum max1241_id {
 struct max1241 {
 	struct spi_device *spi;
 	struct mutex lock;
-	struct regulator *vdd;
 	struct regulator *vref;
 	struct gpio_desc *shutdown;
 
@@ -110,17 +109,6 @@ static const struct iio_info max1241_info = {
 	.read_raw = max1241_read_raw,
 };
 
-static void max1241_disable_vdd_action(void *data)
-{
-	struct max1241 *adc = data;
-	struct device *dev = &adc->spi->dev;
-	int err;
-
-	err = regulator_disable(adc->vdd);
-	if (err)
-		dev_err(dev, "could not disable vdd regulator.\n");
-}
-
 static void max1241_disable_vref_action(void *data)
 {
 	struct max1241 *adc = data;
@@ -147,20 +135,10 @@ static int max1241_probe(struct spi_device *spi)
 	adc->spi = spi;
 	mutex_init(&adc->lock);
 
-	adc->vdd = devm_regulator_get(dev, "vdd");
-	if (IS_ERR(adc->vdd))
-		return dev_err_probe(dev, PTR_ERR(adc->vdd),
-				     "failed to get vdd regulator\n");
-
-	ret = regulator_enable(adc->vdd);
+	ret = devm_regulator_get_enable(dev, "vdd");
 	if (ret)
-		return ret;
-
-	ret = devm_add_action_or_reset(dev, max1241_disable_vdd_action, adc);
-	if (ret) {
-		dev_err(dev, "could not set up vdd regulator cleanup action\n");
-		return ret;
-	}
+		return dev_err_probe(dev, ret,
+				     "failed to get/enable vdd regulator\n");
 
 	adc->vref = devm_regulator_get(dev, "vref");
 	if (IS_ERR(adc->vref))
-- 
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 v3 12/14] iio: max1241: simplify using devm_regulator_get_enable()

[Alexandru Lazar]

Heya,

I don't have the hardware at hand (long story, lots of smoke), so all I 
can do for this patch at the moment is:

Acked-by: Alexandru Lazar <alazar@startmail.com>

That being said, IIRC the MAX1241 has no special requirements in this 
regard, so if other SPI devices work, this one ought to work as well. If 
anyone's concerned, I can test it, but probably not sooner than a week 
or so :-(.

FWIW I really like this, the straightforward regulator boilerplate 
always felt a tad wrong.

All the best,
Alex

On 8/19/2022 10:20 PM, Matti Vaittinen wrote:
> Drop open-coded pattern: 'devm_regulator_get(), regulator_enable(),
> add_action_or_reset(regulator_disable)' and use the
> devm_regulator_get_enable() 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 <mazziesaccount@gmail.com>
> 
> ---
> v2 => v3:
> New patch
> ---
>   drivers/iio/adc/max1241.c | 28 +++-------------------------
>   1 file changed, 3 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/iio/adc/max1241.c b/drivers/iio/adc/max1241.c
> index a815ad1f6913..500bb09ab19b 100644
> --- a/drivers/iio/adc/max1241.c
> +++ b/drivers/iio/adc/max1241.c
> @@ -22,7 +22,6 @@ enum max1241_id {
>   struct max1241 {
>   	struct spi_device *spi;
>   	struct mutex lock;
> -	struct regulator *vdd;
>   	struct regulator *vref;
>   	struct gpio_desc *shutdown;
>   
> @@ -110,17 +109,6 @@ static const struct iio_info max1241_info = {
>   	.read_raw = max1241_read_raw,
>   };
>   
> -static void max1241_disable_vdd_action(void *data)
> -{
> -	struct max1241 *adc = data;
> -	struct device *dev = &adc->spi->dev;
> -	int err;
> -
> -	err = regulator_disable(adc->vdd);
> -	if (err)
> -		dev_err(dev, "could not disable vdd regulator.\n");
> -}
> -
>   static void max1241_disable_vref_action(void *data)
>   {
>   	struct max1241 *adc = data;
> @@ -147,20 +135,10 @@ static int max1241_probe(struct spi_device *spi)
>   	adc->spi = spi;
>   	mutex_init(&adc->lock);
>   
> -	adc->vdd = devm_regulator_get(dev, "vdd");
> -	if (IS_ERR(adc->vdd))
> -		return dev_err_probe(dev, PTR_ERR(adc->vdd),
> -				     "failed to get vdd regulator\n");
> -
> -	ret = regulator_enable(adc->vdd);
> +	ret = devm_regulator_get_enable(dev, "vdd");
>   	if (ret)
> -		return ret;
> -
> -	ret = devm_add_action_or_reset(dev, max1241_disable_vdd_action, adc);
> -	if (ret) {
> -		dev_err(dev, "could not set up vdd regulator cleanup action\n");
> -		return ret;
> -	}
> +		return dev_err_probe(dev, ret,
> +				     "failed to get/enable vdd regulator\n");
>   
>   	adc->vref = devm_regulator_get(dev, "vref");
>   	if (IS_ERR(adc->vref))

Re: [PATCH v3 12/14] iio: max1241: simplify using devm_regulator_get_enable()

[Jonathan Cameron]

On Fri, 19 Aug 2022 22:58:45 +0300
Alexandru Lazar <alazar@startmail.com> wrote:

> Heya,
> 
> I don't have the hardware at hand (long story, lots of smoke), so all I 
> can do for this patch at the moment is:
> 
> Acked-by: Alexandru Lazar <alazar@startmail.com>
> 
> That being said, IIRC the MAX1241 has no special requirements in this 
> regard, so if other SPI devices work, this one ought to work as well. If 
> anyone's concerned, I can test it, but probably not sooner than a week 
> or so :-(.
> 
> FWIW I really like this, the straightforward regulator boilerplate 
> always felt a tad wrong.
> 
> All the best,
> Alex

Now the dependencies are upstream... Applied to the togreg branch of
iio.git and pushed out as testing so that 0-day can play with it and
because I plan to rebase that tree on rc1 sometime shortly.

Jonathan

> 
> On 8/19/2022 10:20 PM, Matti Vaittinen wrote:
> > Drop open-coded pattern: 'devm_regulator_get(), regulator_enable(),
> > add_action_or_reset(regulator_disable)' and use the
> > devm_regulator_get_enable() 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 <mazziesaccount@gmail.com>
> > 
> > ---
> > v2 => v3:
> > New patch
> > ---
> >   drivers/iio/adc/max1241.c | 28 +++-------------------------
> >   1 file changed, 3 insertions(+), 25 deletions(-)
> > 
> > diff --git a/drivers/iio/adc/max1241.c b/drivers/iio/adc/max1241.c
> > index a815ad1f6913..500bb09ab19b 100644
> > --- a/drivers/iio/adc/max1241.c
> > +++ b/drivers/iio/adc/max1241.c
> > @@ -22,7 +22,6 @@ enum max1241_id {
> >   struct max1241 {
> >   	struct spi_device *spi;
> >   	struct mutex lock;
> > -	struct regulator *vdd;
> >   	struct regulator *vref;
> >   	struct gpio_desc *shutdown;
> >   
> > @@ -110,17 +109,6 @@ static const struct iio_info max1241_info = {
> >   	.read_raw = max1241_read_raw,
> >   };
> >   
> > -static void max1241_disable_vdd_action(void *data)
> > -{
> > -	struct max1241 *adc = data;
> > -	struct device *dev = &adc->spi->dev;
> > -	int err;
> > -
> > -	err = regulator_disable(adc->vdd);
> > -	if (err)
> > -		dev_err(dev, "could not disable vdd regulator.\n");
> > -}
> > -
> >   static void max1241_disable_vref_action(void *data)
> >   {
> >   	struct max1241 *adc = data;
> > @@ -147,20 +135,10 @@ static int max1241_probe(struct spi_device *spi)
> >   	adc->spi = spi;
> >   	mutex_init(&adc->lock);
> >   
> > -	adc->vdd = devm_regulator_get(dev, "vdd");
> > -	if (IS_ERR(adc->vdd))
> > -		return dev_err_probe(dev, PTR_ERR(adc->vdd),
> > -				     "failed to get vdd regulator\n");
> > -
> > -	ret = regulator_enable(adc->vdd);
> > +	ret = devm_regulator_get_enable(dev, "vdd");
> >   	if (ret)
> > -		return ret;
> > -
> > -	ret = devm_add_action_or_reset(dev, max1241_disable_vdd_action, adc);
> > -	if (ret) {
> > -		dev_err(dev, "could not set up vdd regulator cleanup action\n");
> > -		return ret;
> > -	}
> > +		return dev_err_probe(dev, ret,
> > +				     "failed to get/enable vdd regulator\n");
> >   
> >   	adc->vref = devm_regulator_get(dev, "vref");
> >   	if (IS_ERR(adc->vref))  

[PATCH v3 13/14] iio: max1363: simplify using devm_regulator_get_enable()

[Matti Vaittinen]

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

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 <mazziesaccount@gmail.com>

---
v2 => v3:
New patch
---
 drivers/iio/adc/max1363.c | 11 +----------
 1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/drivers/iio/adc/max1363.c b/drivers/iio/adc/max1363.c
index eef55ed4814a..d900e29d6cb7 100644
--- a/drivers/iio/adc/max1363.c
+++ b/drivers/iio/adc/max1363.c
@@ -169,7 +169,6 @@ struct max1363_state {
 	const struct max1363_chip_info	*chip_info;
 	const struct max1363_mode	*current_mode;
 	u32				requestedmask;
-	struct regulator		*reg;
 	struct mutex			lock;
 
 	/* Using monitor modes and buffer at the same time is
@@ -1603,15 +1602,7 @@ static int max1363_probe(struct i2c_client *client,
 	st = iio_priv(indio_dev);
 
 	mutex_init(&st->lock);
-	st->reg = devm_regulator_get(&client->dev, "vcc");
-	if (IS_ERR(st->reg))
-		return PTR_ERR(st->reg);
-
-	ret = regulator_enable(st->reg);
-	if (ret)
-		return ret;
-
-	ret = devm_add_action_or_reset(&client->dev, max1363_reg_disable, st->reg);
+	ret = devm_regulator_get_enable(&client->dev, "vcc");
 	if (ret)
 		return ret;
 
-- 
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 v3 13/14] iio: max1363: simplify using devm_regulator_get_enable()

[Sa, Nuno]

> -----Original Message-----
> From: Matti Vaittinen <mazziesaccount@gmail.com>
> Sent: Friday, August 19, 2022 9:21 PM
> To: Matti Vaittinen <mazziesaccount@gmail.com>; Matti Vaittinen
> <matti.vaittinen@fi.rohmeurope.com>
> Cc: Jonathan Cameron <jic23@kernel.org>; Lars-Peter Clausen
> <lars@metafoo.de>; Matti Vaittinen <mazziesaccount@gmail.com>;
> Alexandru Ardelean <aardelean@deviqon.com>; linux-
> iio@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: [PATCH v3 13/14] iio: max1363: simplify using
> devm_regulator_get_enable()
> 
> [External]
> 
> 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 <mazziesaccount@gmail.com>
> 
> ---

Acked-by: Nuno Sá <nuno.sa@analog.com>

Re: [PATCH v3 13/14] iio: max1363: simplify using devm_regulator_get_enable()

[Jonathan Cameron]

On Tue, 30 Aug 2022 11:50:05 +0000
"Sa, Nuno" <Nuno.Sa@analog.com> wrote:

> > -----Original Message-----
> > From: Matti Vaittinen <mazziesaccount@gmail.com>
> > Sent: Friday, August 19, 2022 9:21 PM
> > To: Matti Vaittinen <mazziesaccount@gmail.com>; Matti Vaittinen
> > <matti.vaittinen@fi.rohmeurope.com>
> > Cc: Jonathan Cameron <jic23@kernel.org>; Lars-Peter Clausen
> > <lars@metafoo.de>; Matti Vaittinen <mazziesaccount@gmail.com>;
> > Alexandru Ardelean <aardelean@deviqon.com>; linux-
> > iio@vger.kernel.org; linux-kernel@vger.kernel.org
> > Subject: [PATCH v3 13/14] iio: max1363: simplify using
> > devm_regulator_get_enable()
> > 
> > [External]
> > 
> > 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 <mazziesaccount@gmail.com>
> > 
> > ---  
> 
> Acked-by: Nuno Sá <nuno.sa@analog.com>

Applied.

Re: [PATCH v3 13/14] iio: max1363: simplify using devm_regulator_get_enable()

[Jonathan Cameron]

On Sun, 16 Oct 2022 17:18:38 +0100
Jonathan Cameron <jic23@kernel.org> wrote:

> On Tue, 30 Aug 2022 11:50:05 +0000
> "Sa, Nuno" <Nuno.Sa@analog.com> wrote:
> 
> > > -----Original Message-----
> > > From: Matti Vaittinen <mazziesaccount@gmail.com>
> > > Sent: Friday, August 19, 2022 9:21 PM
> > > To: Matti Vaittinen <mazziesaccount@gmail.com>; Matti Vaittinen
> > > <matti.vaittinen@fi.rohmeurope.com>
> > > Cc: Jonathan Cameron <jic23@kernel.org>; Lars-Peter Clausen
> > > <lars@metafoo.de>; Matti Vaittinen <mazziesaccount@gmail.com>;
> > > Alexandru Ardelean <aardelean@deviqon.com>; linux-
> > > iio@vger.kernel.org; linux-kernel@vger.kernel.org
> > > Subject: [PATCH v3 13/14] iio: max1363: simplify using
> > > devm_regulator_get_enable()
> > > 
> > > [External]
> > > 
> > > 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 <mazziesaccount@gmail.com>
> > > 
> > > ---    
> > 
> > Acked-by: Nuno Sá <nuno.sa@analog.com>  
> 
> Applied.

This one was missing cleaning up the docs as well. fixed up.

[PATCH v3 14/14] iio: hmc425a: simplify using devm_regulator_get_enable()

[Matti Vaittinen]

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

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 <mazziesaccount@gmail.com>

---
v2 => v3:
New patch
---
 drivers/iio/amplifiers/hmc425a.c | 17 +----------------
 1 file changed, 1 insertion(+), 16 deletions(-)

diff --git a/drivers/iio/amplifiers/hmc425a.c b/drivers/iio/amplifiers/hmc425a.c
index ce80e0c916f4..108f0f1685ef 100644
--- a/drivers/iio/amplifiers/hmc425a.c
+++ b/drivers/iio/amplifiers/hmc425a.c
@@ -34,7 +34,6 @@ struct hmc425a_chip_info {
 };
 
 struct hmc425a_state {
-	struct	regulator *reg;
 	struct	mutex lock; /* protect sensor state */
 	struct	hmc425a_chip_info *chip_info;
 	struct	gpio_descs *gpios;
@@ -162,13 +161,6 @@ static const struct of_device_id hmc425a_of_match[] = {
 };
 MODULE_DEVICE_TABLE(of, hmc425a_of_match);
 
-static void hmc425a_reg_disable(void *data)
-{
-	struct hmc425a_state *st = data;
-
-	regulator_disable(st->reg);
-}
-
 static struct hmc425a_chip_info hmc425a_chip_info_tbl[] = {
 	[ID_HMC425A] = {
 		.name = "hmc425a",
@@ -211,14 +203,7 @@ static int hmc425a_probe(struct platform_device *pdev)
 		return -ENODEV;
 	}
 
-	st->reg = devm_regulator_get(&pdev->dev, "vcc-supply");
-	if (IS_ERR(st->reg))
-		return PTR_ERR(st->reg);
-
-	ret = regulator_enable(st->reg);
-	if (ret)
-		return ret;
-	ret = devm_add_action_or_reset(&pdev->dev, hmc425a_reg_disable, st);
+	ret = devm_regulator_get_enable(&pdev->dev, "vcc-supply");
 	if (ret)
 		return ret;
 
-- 
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 v3 14/14] iio: hmc425a: simplify using devm_regulator_get_enable()

[Sa, Nuno]

> From: Matti Vaittinen <mazziesaccount@gmail.com>
> Sent: Friday, August 19, 2022 9:21 PM
> To: Matti Vaittinen <mazziesaccount@gmail.com>; Matti Vaittinen
> <matti.vaittinen@fi.rohmeurope.com>
> Cc: Lars-Peter Clausen <lars@metafoo.de>; Hennerich, Michael
> <Michael.Hennerich@analog.com>; Jonathan Cameron
> <jic23@kernel.org>; linux-iio@vger.kernel.org; linux-
> kernel@vger.kernel.org
> Subject: [PATCH v3 14/14] iio: hmc425a: simplify using
> devm_regulator_get_enable()
> 
> [External]
> 
> 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 <mazziesaccount@gmail.com>
> 
> ---

Acked-by: Nuno Sá <nuno.sa@analog.com>

(I see that in this patch you are not using dev_err_probe(). Maybe some
consistency in the series and where applicable would be appropriate :))

Re: [PATCH v3 14/14] iio: hmc425a: simplify using devm_regulator_get_enable()

[Matti Vaittinen]

On 8/30/22 14:49, Sa, Nuno wrote:
> 
>> From: Matti Vaittinen <mazziesaccount@gmail.com>
>> Sent: Friday, August 19, 2022 9:21 PM
>> To: Matti Vaittinen <mazziesaccount@gmail.com>; Matti Vaittinen
>> <matti.vaittinen@fi.rohmeurope.com>
>> Cc: Lars-Peter Clausen <lars@metafoo.de>; Hennerich, Michael
>> <Michael.Hennerich@analog.com>; Jonathan Cameron
>> <jic23@kernel.org>; linux-iio@vger.kernel.org; linux-
>> kernel@vger.kernel.org
>> Subject: [PATCH v3 14/14] iio: hmc425a: simplify using
>> devm_regulator_get_enable()
>>
>> [External]
>>
>> 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 <mazziesaccount@gmail.com>
>>
>> ---
> 
> Acked-by: Nuno Sá <nuno.sa@analog.com>
> 
> (I see that in this patch you are not using dev_err_probe(). Maybe some
> consistency in the series and where applicable would be appropriate :))

I don't think the driver did originally print an error if regulator get 
or enable failed. I didn't want to add any new prints - just converted 
the existing ones to use dev_err_probe(). I believe adding new prints 
would've been somewhat unrelated change :)

Yours,
	-- Matti

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

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

RE: [PATCH v3 14/14] iio: hmc425a: simplify using devm_regulator_get_enable()

[Sa, Nuno]

> -----Original Message-----
> From: Matti Vaittinen <mazziesaccount@gmail.com>
> Sent: Tuesday, August 30, 2022 3:00 PM
> To: Sa, Nuno <Nuno.Sa@analog.com>; Matti Vaittinen
> <matti.vaittinen@fi.rohmeurope.com>
> Cc: Lars-Peter Clausen <lars@metafoo.de>; Hennerich, Michael
> <Michael.Hennerich@analog.com>; Jonathan Cameron
> <jic23@kernel.org>; linux-iio@vger.kernel.org; linux-
> kernel@vger.kernel.org
> Subject: Re: [PATCH v3 14/14] iio: hmc425a: simplify using
> devm_regulator_get_enable()
> 
> [External]
> 
> On 8/30/22 14:49, Sa, Nuno wrote:
> >
> >> From: Matti Vaittinen <mazziesaccount@gmail.com>
> >> Sent: Friday, August 19, 2022 9:21 PM
> >> To: Matti Vaittinen <mazziesaccount@gmail.com>; Matti Vaittinen
> >> <matti.vaittinen@fi.rohmeurope.com>
> >> Cc: Lars-Peter Clausen <lars@metafoo.de>; Hennerich, Michael
> >> <Michael.Hennerich@analog.com>; Jonathan Cameron
> >> <jic23@kernel.org>; linux-iio@vger.kernel.org; linux-
> >> kernel@vger.kernel.org
> >> Subject: [PATCH v3 14/14] iio: hmc425a: simplify using
> >> devm_regulator_get_enable()
> >>
> >> [External]
> >>
> >> 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 <mazziesaccount@gmail.com>
> >>
> >> ---
> >
> > Acked-by: Nuno Sá <nuno.sa@analog.com>
> >
> > (I see that in this patch you are not using dev_err_probe(). Maybe
> some
> > consistency in the series and where applicable would be appropriate
> :))
> 
> I don't think the driver did originally print an error if regulator get
> or enable failed. I didn't want to add any new prints - just converted
> the existing ones to use dev_err_probe(). I believe adding new prints
> would've been somewhat unrelated change :)
> 

Ahh that makes sense. I failed to see the print that you were replacing
in the ad7606 patch...

- Nuno Sá

Re: [PATCH v3 14/14] iio: hmc425a: simplify using devm_regulator_get_enable()

[Jonathan Cameron]

On Tue, 30 Aug 2022 13:55:38 +0000
"Sa, Nuno" <Nuno.Sa@analog.com> wrote:

> > -----Original Message-----
> > From: Matti Vaittinen <mazziesaccount@gmail.com>
> > Sent: Tuesday, August 30, 2022 3:00 PM
> > To: Sa, Nuno <Nuno.Sa@analog.com>; Matti Vaittinen
> > <matti.vaittinen@fi.rohmeurope.com>
> > Cc: Lars-Peter Clausen <lars@metafoo.de>; Hennerich, Michael
> > <Michael.Hennerich@analog.com>; Jonathan Cameron
> > <jic23@kernel.org>; linux-iio@vger.kernel.org; linux-
> > kernel@vger.kernel.org
> > Subject: Re: [PATCH v3 14/14] iio: hmc425a: simplify using
> > devm_regulator_get_enable()
> > 
> > [External]
> > 
> > On 8/30/22 14:49, Sa, Nuno wrote:  
> > >  
> > >> From: Matti Vaittinen <mazziesaccount@gmail.com>
> > >> Sent: Friday, August 19, 2022 9:21 PM
> > >> To: Matti Vaittinen <mazziesaccount@gmail.com>; Matti Vaittinen
> > >> <matti.vaittinen@fi.rohmeurope.com>
> > >> Cc: Lars-Peter Clausen <lars@metafoo.de>; Hennerich, Michael
> > >> <Michael.Hennerich@analog.com>; Jonathan Cameron
> > >> <jic23@kernel.org>; linux-iio@vger.kernel.org; linux-
> > >> kernel@vger.kernel.org
> > >> Subject: [PATCH v3 14/14] iio: hmc425a: simplify using
> > >> devm_regulator_get_enable()
> > >>
> > >> [External]
> > >>
> > >> 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 <mazziesaccount@gmail.com>
> > >>
> > >> ---  
> > >
> > > Acked-by: Nuno Sá <nuno.sa@analog.com>
> > >
> > > (I see that in this patch you are not using dev_err_probe(). Maybe  
> > some  
> > > consistency in the series and where applicable would be appropriate  
> > :))
> > 
> > I don't think the driver did originally print an error if regulator get
> > or enable failed. I didn't want to add any new prints - just converted
> > the existing ones to use dev_err_probe(). I believe adding new prints
> > would've been somewhat unrelated change :)
> >   
> 
> Ahh that makes sense. I failed to see the print that you were replacing
> in the ad7606 patch...
> 
Applied to the togreg branch of iio.git though for now that's just pushed
out as testing.

As I mentioned on an earlier patch I forgot these existed and was about
to duplicate some of the work..  Anyhow, we only overlapped on a few
patches so I'll send the rest of my set out shortly.

There are probably other cases still in IIO but they are fiddly to grep
for!

Jonathan
> - Nuno Sá

Re: [PATCH v3 00/14] Use devm helpers for regulator get and enable

[Andy Shevchenko]

On Fri, Aug 19, 2022 at 10:20 PM Matti Vaittinen
<mazziesaccount@gmail.com> wrote:
>
> Use devm helpers for regulator get and enable
>
> NOTE: The series depends on commit
> ee94aff2628b ("Devm helpers for regulator get and enable")
> which currently sits in Mark's regulator/for-next
>
> A few* drivers seem to pattern demonstrated by pseudocode:
>
> - devm_regulator_get()
> - regulator_enable()
> - devm_add_action_or_reset(regulator_disable())
>
> devm helpers for this pattern were added to remove bunch of code from

remove a bunch

> 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.
>
> I believe this simplifies things by removing some dublicated code.

duplicated

> This series reowrks a few drivers. There is still plenty of fish in the

reworks

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

...

>   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()
>   hwmon: adm1177: simplify using devm_regulator_get_enable()

hwmon uses a different pattern for the Subject line.

-- 
With Best Regards,
Andy Shevchenko