Use devm helpers for regulator get and enable

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 <[email protected]>

---

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 <[email protected]>

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

drivers/gpu/drm/bridge/sii902x.c +3 -17

index 7ab38d734ad6..162f9c87eeb2 100644
@@ -171,7 +171,6 @@ struct sii902x {
        struct drm_connector connector;
        struct gpio_desc *reset_gpio;
        struct i2c_mux_core *i2cmux;
-       struct regulator_bulk_data supplies[2];
        bool sink_is_hdmi;
        /*
         * Mutex protects audio and video functions from interfering

@@ -1072,6 +1071,7 @@ static int sii902x_probe(struct i2c_client *client,
        struct device *dev = &client->dev;
        struct device_node *endpoint;
        struct sii902x *sii902x;
+       static const char * const supplies[] = {"iovcc", "cvcc12"};
        int ret;
 
        ret = i2c_check_functionality(client->adapter,

@@ -1122,27 +1122,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)

drivers/gpu/drm/meson/meson_dw_hdmi.c +3 -21

index 5cd2b2ebbbd3..7642f740272b 100644
@@ -140,7 +140,6 @@ struct meson_dw_hdmi {
        struct reset_control *hdmitx_apb;
        struct reset_control *hdmitx_ctrl;
        struct reset_control *hdmitx_phy;
-       struct regulator *hdmi_supply;
        u32 irq_stat;
        struct dw_hdmi *hdmi;
        struct drm_bridge *bridge;

@@ -665,11 +664,6 @@ static void meson_dw_hdmi_init(struct meson_dw_hdmi *meson_dw_hdmi)
 
 }
 
-static void meson_disable_regulator(void *data)
-{
-       regulator_disable(data);
-}
-
 static void meson_disable_clk(void *data)
 {
        clk_disable_unprepare(data);

@@ -723,20 +717,9 @@ static int meson_dw_hdmi_bind(struct device *dev, struct device *master,
        meson_dw_hdmi->data = match;
        dw_plat_data = &meson_dw_hdmi->dw_plat_data;
 
-       meson_dw_hdmi->hdmi_supply = devm_regulator_get_optional(dev, "hdmi");
-       if (IS_ERR(meson_dw_hdmi->hdmi_supply)) {
-               if (PTR_ERR(meson_dw_hdmi->hdmi_supply) == -EPROBE_DEFER)
-                       return -EPROBE_DEFER;
-               meson_dw_hdmi->hdmi_supply = NULL;
-       } else {
-               ret = regulator_enable(meson_dw_hdmi->hdmi_supply);
-               if (ret)
-                       return ret;
-               ret = devm_add_action_or_reset(dev, meson_disable_regulator,
-                                              meson_dw_hdmi->hdmi_supply);
-               if (ret)
-                       return ret;
-       }
+       ret = devm_regulator_get_enable_optional(dev, "hdmi");
+       if (ret != -ENODEV)
+               return ret;
 
        meson_dw_hdmi->hdmitx_apb = devm_reset_control_get_exclusive(dev,
                                                "hdmitx_apb");
--

2.37.1

Hi Matti,

Quoted 83 lines · Matti Vaittinen

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 <[email protected]>

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

drivers/gpu/drm/bridge/sii902x.c +3 -19

index 7ab38d734ad6..162f9c87eeb2 100644
@@ -171,7 +171,6 @@ struct sii902x {
         struct drm_connector connector;
         struct gpio_desc *reset_gpio;
         struct i2c_mux_core *i2cmux;
-       struct regulator_bulk_data supplies[2];
         bool sink_is_hdmi;
         /*
          * Mutex protects audio and video functions from interfering

@@ -1072,6 +1071,7 @@ static int sii902x_probe(struct i2c_client *client,
         struct device *dev = &client->dev;
         struct device_node *endpoint;
         struct sii902x *sii902x;
+       static const char * const supplies[] = {"iovcc", "cvcc12"};
         int ret;
 
         ret = i2c_check_functionality(client->adapter,

@@ -1122,27 +1122,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);
 

drivers/gpu/drm/bridge/sii902x.c static int sii902x_remove(struct i2c_client *client) · line 1143

Quoted 2 lines

drivers/gpu/drm/bridge/sii902x.c +0 -0

         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 <[email protected]>

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

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

---
v2 => v3:
New patch

---
 drivers/hwmon/adm1177.c | 27 +++------------------------
 1 file changed, 3 insertions(+), 24 deletions(-)

drivers/hwmon/adm1177.c +3 -24

index 0c5dbc5e33b4..be17a26a84f1 100644
@@ -26,14 +26,12 @@
 /**
  * struct adm1177_state - driver instance specific data
  * @client:		pointer to i2c client
- * @reg:		regulator info for the power supply of the device
  * @r_sense_uohm:	current sense resistor value
  * @alert_threshold_ua:	current limit for shutdown
  * @vrange_high:	internal voltage divider
  */
 struct adm1177_state {
 	struct i2c_client	*client;
-	struct regulator	*reg;
 	u32			r_sense_uohm;
 	u32			alert_threshold_ua;
 	bool			vrange_high;

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

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

Signature · 14 lines

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

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 <[email protected]>

---
v2 => v3
Split to own patch.

---
 drivers/iio/adc/ad7192.c | 15 ++-------------
 1 file changed, 2 insertions(+), 13 deletions(-)

drivers/iio/adc/ad7192.c +2 -13

index d71977be7d22..8a52c0dec3f9 100644
@@ -177,7 +177,6 @@ struct ad7192_chip_info {
 struct ad7192_state {
 	const struct ad7192_chip_info	*chip_info;
 	struct regulator		*avdd;
-	struct regulator		*dvdd;
 	struct clk			*mclk;
 	u16				int_vref_mv;
 	u32				fclk;

@@ -1015,19 +1014,9 @@ static int ad7192_probe(struct spi_device *spi)
 	if (ret)
 		return ret;
 
-	st->dvdd = devm_regulator_get(&spi->dev, "dvdd");
-	if (IS_ERR(st->dvdd))
-		return PTR_ERR(st->dvdd);
-
-	ret = regulator_enable(st->dvdd);
-	if (ret) {
-		dev_err(&spi->dev, "Failed to enable specified DVdd supply\n");
-		return ret;
-	}
-
-	ret = devm_add_action_or_reset(&spi->dev, ad7192_reg_disable, st->dvdd);
+	ret = devm_regulator_get_enable(&spi->dev, "dvdd");
 	if (ret)
-		return ret;
+		return dev_err_probe(&spi->dev, ret, "Failed to enable specified DVdd supply\n");
 
 	ret = regulator_get_voltage(st->avdd);
 	if (ret < 0) {

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 <[email protected]>

---
v2 => v3
Split to own patch.

---
 drivers/iio/dac/ltc2688.c | 23 +++--------------------
 1 file changed, 3 insertions(+), 20 deletions(-)

drivers/iio/dac/ltc2688.c +0 -8

index 28bdde2d3088..fcad3efe62ea 100644
@@ -84,7 +84,6 @@ struct ltc2688_chan {
 struct ltc2688_state {
 	struct spi_device *spi;
 	struct regmap *regmap;
-	struct regulator_bulk_data regulators[2];
 	struct ltc2688_chan channels[LTC2688_DAC_CHANNELS];
 	struct iio_chan_spec *iio_chan;
 	/* lock to protect against multiple access to the device and shared data */

@@ -902,13 +901,6 @@ static int ltc2688_setup(struct ltc2688_state *st, struct regulator *vref)
 			       LTC2688_CONFIG_EXT_REF);
 }
 
-static void ltc2688_disable_regulators(void *data)
-{
-	struct ltc2688_state *st = data;
-
-	regulator_bulk_disable(ARRAY_SIZE(st->regulators), st->regulators);
-}
-
 static void ltc2688_disable_regulator(void *regulator)
 {
 	regulator_disable(regulator);

drivers/iio/dac/ltc2688.c +2 -12

 	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)

Quoted 39 lines · On Fri, 19 Aug 2022 22:19:17 +0300 Matti Vaittinen <[email protected]>…

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 <[email protected]>

---
v2 => v3
Split to own patch.

---
  drivers/iio/dac/ltc2688.c | 23 +++--------------------
  1 file changed, 3 insertions(+), 20 deletions(-)

drivers/iio/dac/ltc2688.c +0 -8

index 28bdde2d3088..fcad3efe62ea 100644
@@ -84,7 +84,6 @@ struct ltc2688_chan {
  struct ltc2688_state {
  	struct spi_device *spi;
  	struct regmap *regmap;
-	struct regulator_bulk_data regulators[2];
  	struct ltc2688_chan channels[LTC2688_DAC_CHANNELS];
  	struct iio_chan_spec *iio_chan;
  	/* lock to protect against multiple access to the device and shared data */

@@ -902,13 +901,6 @@ static int ltc2688_setup(struct ltc2688_state *st, struct regulator *vref)
  			       LTC2688_CONFIG_EXT_REF);
  }
 
-static void ltc2688_disable_regulators(void *data)
-{
-	struct ltc2688_state *st = data;
-
-	regulator_bulk_disable(ARRAY_SIZE(st->regulators), st->regulators);
-}
-
  static void ltc2688_disable_regulator(void *regulator)
  {
  	regulator_disable(regulator);

drivers/iio/dac/ltc2688.c static int ltc2688_probe(struct spi_device *spi) · line 965

Quoted 4 lines

drivers/iio/dac/ltc2688.c +1 -0

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

Quoted 2 lines · Jonathan Cameron

On Fri, 19 Aug 2022 22:19:17 +0300
Matti Vaittinen <[email protected]> wrote:

Quoted 6 lines

Quoted 1 line

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

Quoted 2 lines

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.

Quoted 3 lines · Matti Vaittinen

Quoted 2 lines · Jonathan Cameron

On Fri, 19 Aug 2022 22:19:17 +0300
Matti Vaittinen <[email protected]> wrote:

...

Quoted 4 lines

Quoted 4 lines

Quoted 1 line

+ static const char * const regulators[] = {"vcc", "iovcc"};

trivial - slight preference for
{ "vcc", "iovcc" };

Quoted 4 lines

Quoted 2 lines

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.

Quoted 4 lines

Quoted 3 lines

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

Quoted 9 lines

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.

Quoted 5 lines · Andy Shevchenko

Quoted 3 lines · Matti Vaittinen

Quoted 2 lines · Jonathan Cameron

On Fri, 19 Aug 2022 22:19:17 +0300
Matti Vaittinen <[email protected]> wrote:

Quoted 13 lines

Quoted 12 lines

Quoted 2 lines

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.

Quoted 1 line

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?

Quoted 2 lines

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.

Quoted 7 lines · Matti Vaittinen

Quoted 5 lines · Andy Shevchenko

Quoted 3 lines · Matti Vaittinen

Quoted 2 lines · Jonathan Cameron

On Fri, 19 Aug 2022 22:19:17 +0300
Matti Vaittinen <[email protected]> wrote:

...

Quoted 13 lines

Quoted 13 lines

Quoted 12 lines

Quoted 2 lines

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.

Quoted 4 lines

Quoted 1 line

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.

Quoted 5 lines

Quoted 2 lines

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.

Quoted 29 lines · Andy Shevchenko

Quoted 6 lines · Matti Vaittinen

Quoted 5 lines · Andy Shevchenko

Quoted 3 lines · Matti Vaittinen

Quoted 2 lines · Jonathan Cameron

On Fri, 19 Aug 2022 22:19:17 +0300
Matti Vaittinen <[email protected]> wrote:

...

Quoted 17 lines

Quoted 13 lines

Quoted 12 lines

Quoted 2 lines

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.

Quoted 1 line

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?

Quoted 2 lines

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.

Quoted 8 lines

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

Quoted 5 lines

Quoted 2 lines

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.

Quoted 1 line

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.

Quoted 32 lines · Matti Vaittinen

Quoted 31 lines · Andy Shevchenko

Quoted 6 lines · Matti Vaittinen

Quoted 5 lines · Andy Shevchenko

Quoted 3 lines · Matti Vaittinen

Quoted 2 lines · Jonathan Cameron

On Fri, 19 Aug 2022 22:19:17 +0300
Matti Vaittinen <[email protected]> wrote:

...

Quoted 13 lines

Quoted 13 lines

Quoted 12 lines

Quoted 2 lines

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.

Quoted 4 lines

Quoted 1 line

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.

Quoted 4 lines

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.

Quoted 9 lines

Quoted 9 lines

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

Quoted 5 lines

Quoted 2 lines

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.

Quoted 7 lines

Quoted 1 line

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.

From: Matti Vaittinen <[email protected]>
Sent: Friday, August 19, 2022 9:19 PM
To: Matti Vaittinen <[email protected]>; Matti Vaittinen <[email protected]>
Cc: Sa, Nuno <[email protected]>; Jonathan Cameron <[email protected]>; Lars-Peter Clausen <[email protected]>; linux- [email protected]; [email protected]
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 <[email protected]>

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 <[email protected]>

---
v2 => v3
Split to own patch.

---
 drivers/iio/dac/ltc2688.c | 23 +++--------------------
 1 file changed, 3 insertions(+), 20 deletions(-)

drivers/iio/dac/ltc2688.c +3 -20

index 28bdde2d3088..fcad3efe62ea 100644
@@ -84,7 +84,6 @@ struct ltc2688_chan {
 struct ltc2688_state {
 	struct spi_device *spi;
 	struct regmap *regmap;
-	struct regulator_bulk_data regulators[2];
 	struct ltc2688_chan channels[LTC2688_DAC_CHANNELS];
 	struct iio_chan_spec *iio_chan;
 	/* lock to protect against multiple access to the device and shared data */

@@ -902,13 +901,6 @@ static int ltc2688_setup(struct ltc2688_state *st, struct regulator *vref)
 			       LTC2688_CONFIG_EXT_REF);
 }
 
-static void ltc2688_disable_regulators(void *data)
-{
-	struct ltc2688_state *st = data;
-
-	regulator_bulk_disable(ARRAY_SIZE(st->regulators), st->regulators);
-}
-
 static void ltc2688_disable_regulator(void *regulator)
 {
 	regulator_disable(regulator);

@@ -970,6 +962,7 @@ static int ltc2688_probe(struct spi_device *spi)
 	struct regulator *vref_reg;
 	struct device *dev = &spi->dev;
 	int ret;
+	static const char * const regulators[] = {"vcc", "iovcc"};
 
 	indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
 	if (!indio_dev)

@@ -988,21 +981,11 @@ static int ltc2688_probe(struct spi_device *spi)
 		return dev_err_probe(dev, PTR_ERR(st->regmap),
 				     "Failed to init regmap");
 
-	st->regulators[0].supply = "vcc";
-	st->regulators[1].supply = "iovcc";
-	ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(st->regulators),
-				      st->regulators);
-	if (ret)
-		return dev_err_probe(dev, ret, "Failed to get regulators\n");
-
-	ret = regulator_bulk_enable(ARRAY_SIZE(st->regulators), st->regulators);
+	ret = devm_regulator_bulk_get_enable(dev, ARRAY_SIZE(regulators),
+					     regulators);
 	if (ret)
 		return dev_err_probe(dev, ret, "Failed to enable regulators\n");
 
-	ret = devm_add_action_or_reset(dev, ltc2688_disable_regulators, st);
-	if (ret)
-		return ret;
-
 	vref_reg = devm_regulator_get_optional(dev, "vref");
 	if (IS_ERR(vref_reg)) {
 		if (PTR_ERR(vref_reg) != -ENODEV)

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

Quoted 3 lines · Matti Vaittinen

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

...

Quoted 7 lines

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.

Quoted 2 lines · Andy Shevchenko

On Fri, Aug 19, 2022 at 10:21 PM Matti Vaittinen
<[email protected]> wrote:

Quoted 6 lines

Quoted 4 lines

         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.

Quoted 2 lines

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

Quoted 3 lines · Matti

Quoted 2 lines · Andy Shevchenko

On Fri, Aug 19, 2022 at 10:21 PM Matti Vaittinen
<[email protected]> wrote:

What did I miss?

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

          struct bmg160_data *data;
          struct iio_dev *indio_dev;

Quoted 8 lines

Quoted 3 lines

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.

Quoted 6 lines · Andy Shevchenko

Quoted 3 lines · Matti

Quoted 2 lines · Andy Shevchenko

On Fri, Aug 19, 2022 at 10:21 PM Matti Vaittinen
<[email protected]> wrote:

Quoted 4 lines

What did I miss?

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

          struct bmg160_data *data;
          struct iio_dev *indio_dev;

This does already violate the rule.

Quoted 12 lines

Quoted 7 lines

Quoted 2 lines

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.

Quoted 2 lines

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.

Quoted 2 lines

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.

Quoted 9 lines · Matti

Quoted 7 lines · Andy Shevchenko

Quoted 3 lines · Matti

Quoted 2 lines · Andy Shevchenko

On Fri, Aug 19, 2022 at 10:21 PM Matti Vaittinen
<[email protected]> wrote:

What did I miss?

Quoted 4 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

          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.

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 2 lines

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

Quoted 8 lines

Quoted 7 lines

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

Quoted 3 lines

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.

Quoted 7 lines

Quoted 2 lines

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.

Quoted 5 lines

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.

Quoted 16 lines · Andy Shevchenko

Quoted 13 lines · Matti

Quoted 10 lines · Andy Shevchenko

Quoted 3 lines · Matti

Quoted 2 lines · Andy Shevchenko

On Fri, Aug 19, 2022 at 10:21 PM Matti Vaittinen
<[email protected]> wrote:

What did I miss?

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

          struct bmg160_data *data;
          struct iio_dev *indio_dev;

This does already violate the rule.

Quoted 3 lines

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

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 2 lines

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.

Quoted 11 lines

Quoted 10 lines

Quoted 6 lines

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

Quoted 2 lines

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/[email protected]/
https://lore.kernel.org/all/[email protected]/

Quoted 16 lines

Quoted 7 lines

Quoted 2 lines

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.

Quoted 5 lines

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.

Quoted 3 lines

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.

Quoted 3 lines

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

Quoted 21 lines · Matti Vaittinen

Quoted 19 lines · Andy Shevchenko

Quoted 7 lines · Matti

Quoted 6 lines · Andy Shevchenko

Quoted 3 lines · Matti

Quoted 2 lines · Andy Shevchenko

On Fri, Aug 19, 2022 at 10:21 PM Matti Vaittinen
<[email protected]> wrote:

Quoted 6 lines

Quoted 4 lines

What did I miss?

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

          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.

Quoted 4 lines

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?

Quoted 8 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 2 lines

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

Quoted 2 lines

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?

Quoted 14 lines

Quoted 13 lines

Quoted 10 lines

Quoted 6 lines

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

Quoted 3 lines

Maybe the explanation by Sebastian here can put us on the same page:
https://lore.kernel.org/all/[email protected]/
https://lore.kernel.org/all/[email protected]/

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.

Quoted 20 lines

Quoted 19 lines

Quoted 7 lines

Quoted 2 lines

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.

Quoted 5 lines

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.

Quoted 2 lines

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.

Quoted 5 lines

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.

Quoted 7 lines

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.

Quoted 2 lines

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

Quoted 5 lines

Quoted 3 lines

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.

Quoted 28 lines · Andy Shevchenko

Quoted 25 lines · Matti Vaittinen

Quoted 11 lines · Andy Shevchenko

Quoted 9 lines · Matti

Quoted 7 lines · Andy Shevchenko

Quoted 3 lines · Matti

Quoted 2 lines · Andy Shevchenko

On Fri, Aug 19, 2022 at 10:21 PM Matti Vaittinen
<[email protected]> wrote:

What did I miss?

Quoted 8 lines

Quoted 4 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

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

Quoted 1 line

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.

Quoted 11 lines

Quoted 10 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 2 lines

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.

Quoted 1 line

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.

Quoted 18 lines

Quoted 17 lines

Quoted 7 lines

Quoted 7 lines

Quoted 6 lines

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

Quoted 6 lines

Quoted 3 lines

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/[email protected]/
https://lore.kernel.org/all/[email protected]/

Quoted 3 lines

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.

Quoted 23 lines

Quoted 22 lines

Quoted 16 lines

Quoted 7 lines

Quoted 2 lines

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.

Quoted 5 lines

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.

Quoted 3 lines

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?

Quoted 3 lines

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.

Quoted 9 lines

This is simply _bad code practice_. I
don't know how else I can explain this to you.

Quoted 5 lines

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.

Quoted 2 lines

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.

Quoted 10 lines

Otherwise (good) compiler should spill a lot of warnings in case you have global vs. local naming collision.

Quoted 7 lines

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.

Quoted 4 lines

Quoted 2 lines

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?

Quoted 8 lines

Quoted 5 lines

Quoted 3 lines

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.

Quoted 4 lines · Matti Vaittinen

Quoted 3 lines · Andy Shevchenko

Quoted 1 line · Matti Vaittinen

On 8/20/22 10:18, Andy Shevchenko wrote:

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 6 lines · Matti

Quoted 5 lines · Andy Shevchenko

Quoted 3 lines · Matti

Quoted 2 lines · Andy Shevchenko

On Fri, Aug 19, 2022 at 10:21 PM Matti Vaittinen
<[email protected]> wrote:

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

Quoted 2 lines

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.

Quoted 5 lines

Quoted 2 lines

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.

Thanks for the review Andy

Quoted 16 lines · Andy Shevchenko

Quoted 3 lines · Matti Vaittinen

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

...

Quoted 7 lines

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

Quoted 1 line

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.

Quoted 3 lines

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

Quoted 2 lines

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

Br,
-- Matti

Quoted 5 lines · Matti

Thanks for the review Andy

Quoted 2 lines · Andy Shevchenko

On Fri, Aug 19, 2022 at 10:21 PM Matti Vaittinen
<[email protected]> wrote:

Quoted 4 lines

Quoted 1 line

Quoted 1 line

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.

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 <[email protected]>

---
v2 => v3
Split to own patch.

---
 drivers/iio/gyro/bmg160_core.c | 24 +++---------------------
 1 file changed, 3 insertions(+), 21 deletions(-)

drivers/iio/gyro/bmg160_core.c +0 -5

index cedd9f02ea21..baa80980c99f 100644
@@ -93,7 +93,6 @@
 
 struct bmg160_data {
 	struct regmap *regmap;
-	struct regulator_bulk_data regulators[2];
 	struct iio_trigger *dready_trig;
 	struct iio_trigger *motion_trig;
 	struct iio_mount_matrix orientation;

@@ -1067,19 +1066,13 @@ static const char *bmg160_match_acpi_device(struct device *dev)
 	return dev_name(dev);
 }
 
-static void bmg160_disable_regulators(void *d)
-{
-	struct bmg160_data *data = d;
-

drivers/iio/gyro/bmg160_core.c +2 -13

 	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;

Quoted 28 lines · On Fri, 19 Aug 2022 22:19:31 +0300 Matti Vaittinen <[email protected]>…

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 <[email protected]>

---
v2 => v3
Split to own patch.

---
  drivers/iio/gyro/bmg160_core.c | 24 +++---------------------
  1 file changed, 3 insertions(+), 21 deletions(-)

drivers/iio/gyro/bmg160_core.c +0 -1

index cedd9f02ea21..baa80980c99f 100644
@@ -93,7 +93,6 @@
 
  struct bmg160_data {
  	struct regmap *regmap;
-	struct regulator_bulk_data regulators[2];
  	struct iio_trigger *dready_trig;
  	struct iio_trigger *motion_trig;
  	struct iio_mount_matrix orientation;

@@ -1067,19 +1066,13 @@ static const char *bmg160_match_acpi_device(struct device *dev)
  	return dev_name(dev);
  }
 

Quoted 4 lines

drivers/iio/gyro/bmg160_core.c +0 -4

-static void bmg160_disable_regulators(void *d)
-{
-	struct bmg160_data *data = d;
-

drivers/iio/gyro/bmg160_core.c static const char *bmg160_match_acpi_device(struct device *dev) · line 1075

Quoted 10 lines

drivers/iio/gyro/bmg160_core.c +1 -3

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

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 <[email protected]>

---
v2 => v3
Split to own patch.

---
 drivers/iio/gyro/bmg160_core.c | 24 +++---------------------
 1 file changed, 3 insertions(+), 21 deletions(-)

drivers/iio/gyro/bmg160_core.c +3 -21

index cedd9f02ea21..baa80980c99f 100644
@@ -93,7 +93,6 @@
 
 struct bmg160_data {
 	struct regmap *regmap;
-	struct regulator_bulk_data regulators[2];
 	struct iio_trigger *dready_trig;
 	struct iio_trigger *motion_trig;
 	struct iio_mount_matrix orientation;

@@ -1067,19 +1066,13 @@ static const char *bmg160_match_acpi_device(struct device *dev)
 	return dev_name(dev);
 }
 
-static void bmg160_disable_regulators(void *d)
-{
-	struct bmg160_data *data = d;
-
-	regulator_bulk_disable(ARRAY_SIZE(data->regulators), data->regulators);
-}
-
 int bmg160_core_probe(struct device *dev, struct regmap *regmap, int irq,
 		      const char *name)
 {
 	struct bmg160_data *data;
 	struct iio_dev *indio_dev;
 	int ret;
+	static const char * const regulators[] = {"vdd", "vddio"};
 
 	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
 	if (!indio_dev)

@@ -1090,22 +1083,11 @@ int bmg160_core_probe(struct device *dev, struct regmap *regmap, int irq,
 	data->irq = irq;
 	data->regmap = regmap;
 
-	data->regulators[0].supply = "vdd";
-	data->regulators[1].supply = "vddio";
-	ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(data->regulators),
-				      data->regulators);
+	ret = devm_regulator_bulk_get_enable(dev, ARRAY_SIZE(regulators),
+					     regulators);
 	if (ret)
 		return dev_err_probe(dev, ret, "Failed to get regulators\n");
 
-	ret = regulator_bulk_enable(ARRAY_SIZE(data->regulators),
-				    data->regulators);
-	if (ret)
-		return ret;
-
-	ret = devm_add_action_or_reset(dev, bmg160_disable_regulators, data);
-	if (ret)
-		return ret;
-
 	ret = iio_read_mount_matrix(dev, &data->orientation);
 	if (ret)
 		return ret;

Quoted 3 lines · On Fri, 19 Aug 2022 22:19:31 +0300 Matti Vaittinen <[email protected]>…

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

Quoted 1 line

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

Applied with tweaks:
- patch title includes gyro:
- ordering as Andy suggested
- spaces after { and before }

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 <[email protected]>

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

drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h +0 -2

index a86dd29a4738..03238c64c777 100644
@@ -372,7 +372,6 @@ struct st_lsm6dsx_sensor {
  * struct st_lsm6dsx_hw - ST IMU MEMS hw instance
  * @dev: Pointer to instance of struct device (I2C or SPI).
  * @regmap: Register map of the device.
- * @regulators: VDD/VDDIO voltage regulators.
  * @irq: Device interrupt line (I2C or SPI).
  * @fifo_lock: Mutex to prevent concurrent access to the hw FIFO.
  * @conf_lock: Mutex to prevent concurrent FIFO configuration update.

@@ -395,7 +394,6 @@ struct st_lsm6dsx_sensor {
 struct st_lsm6dsx_hw {
 	struct device *dev;
 	struct regmap *regmap;
-	struct regulator_bulk_data regulators[2];
 	int irq;
 
 	struct mutex fifo_lock;

drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c +5 -25

index b5e4a4113652..c328d1a36d34 100644
@@ -2172,36 +2172,20 @@ static int st_lsm6dsx_irq_setup(struct st_lsm6dsx_hw *hw)
 
 static int st_lsm6dsx_init_regulators(struct device *dev)
 {
-	struct st_lsm6dsx_hw *hw = dev_get_drvdata(dev);
 	int err;
-
 	/* vdd-vddio power regulators */
-	hw->regulators[0].supply = "vdd";
-	hw->regulators[1].supply = "vddio";
-	err = devm_regulator_bulk_get(dev, ARRAY_SIZE(hw->regulators),
-				      hw->regulators);
-	if (err)
-		return dev_err_probe(dev, err, "failed to get regulators\n");
+	static const char * const regulators[] = {"vdd", "vddio"};
 
-	err = regulator_bulk_enable(ARRAY_SIZE(hw->regulators),
-				    hw->regulators);
-	if (err) {
-		dev_err(dev, "failed to enable regulators: %d\n", err);
-		return err;
-	}
+	err = devm_regulator_bulk_get_enable(dev, ARRAY_SIZE(regulators),
+					     regulators);
+	if (err)
+		return dev_err_probe(dev, err, "failed to enable regulators\n");
 
 	msleep(50);
 
 	return 0;
 }
 
-static void st_lsm6dsx_chip_uninit(void *data)
-{
-	struct st_lsm6dsx_hw *hw = data;
-
-	regulator_bulk_disable(ARRAY_SIZE(hw->regulators), hw->regulators);
-}
-
 int st_lsm6dsx_probe(struct device *dev, int irq, int hw_id,
 		     struct regmap *regmap)
 {

@@ -2225,10 +2209,6 @@ int st_lsm6dsx_probe(struct device *dev, int irq, int hw_id,
 	if (err)
 		return err;
 
-	err = devm_add_action_or_reset(dev, st_lsm6dsx_chip_uninit, hw);
-	if (err)
-		return err;
-
 	hw->buff = devm_kzalloc(dev, ST_LSM6DSX_BUFF_SIZE, GFP_KERNEL);
 	if (!hw->buff)
 		return -ENOMEM;

-----Original Message-----
From: Matti Vaittinen <[email protected]>
Sent: Friday, August 19, 2022 9:20 PM
To: Matti Vaittinen <[email protected]>; Matti Vaittinen <[email protected]>
Cc: Lars-Peter Clausen <[email protected]>; Hennerich, Michael <[email protected]>; Jonathan Cameron <[email protected]>; [email protected]; linux- [email protected]
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 <[email protected]>

---

Quoted 18 lines

-----Original Message-----
From: Matti Vaittinen <[email protected]>
Sent: Friday, August 19, 2022 9:20 PM
To: Matti Vaittinen <[email protected]>; Matti Vaittinen <[email protected]>
Cc: Lars-Peter Clausen <[email protected]>; Hennerich, Michael <[email protected]>; Jonathan Cameron <[email protected]>; [email protected]; linux- [email protected]
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()

Quoted 3 lines

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

---

Acked-by Nuno Sá <[email protected]>

From: Matti Vaittinen <[email protected]>
Sent: Friday, August 19, 2022 9:20 PM
To: Matti Vaittinen <[email protected]>; Matti Vaittinen <[email protected]>
Cc: Lars-Peter Clausen <[email protected]>; Hennerich, Michael <[email protected]>; Jonathan Cameron <[email protected]>; [email protected]; linux- [email protected]
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 <[email protected]>

---

Quoted 19 lines

From: Matti Vaittinen <[email protected]>
Sent: Friday, August 19, 2022 9:20 PM
To: Matti Vaittinen <[email protected]>; Matti Vaittinen <[email protected]>
Cc: Lars-Peter Clausen <[email protected]>; Hennerich, Michael <[email protected]>; Jonathan Cameron <[email protected]>; [email protected]; linux- [email protected]
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.

Quoted 3 lines

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

---

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

Acked-by Nuno Sá <[email protected]>

Thanks for the review(s) Nuno!

Quoted 23 lines · Nuno

Quoted 22 lines

From: Matti Vaittinen <[email protected]>
Sent: Friday, August 19, 2022 9:20 PM
To: Matti Vaittinen <[email protected]>; Matti Vaittinen <[email protected]>
Cc: Lars-Peter Clausen <[email protected]>; Hennerich, Michael <[email protected]>; Jonathan Cameron <[email protected]>; [email protected]; linux- [email protected]
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 <[email protected]>

---

Quoted 4 lines

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

Acked-by Nuno Sá <[email protected]>

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

Quoted 31 lines · On Tue, 30 Aug 2022 15:54:07 +0300 Matti Vaittinen <[email protected]>…

Thanks for the review(s) Nuno!

Quoted 27 lines · Nuno

Quoted 19 lines

From: Matti Vaittinen <[email protected]>
Sent: Friday, August 19, 2022 9:20 PM
To: Matti Vaittinen <[email protected]>; Matti Vaittinen <[email protected]>
Cc: Lars-Peter Clausen <[email protected]>; Hennerich, Michael <[email protected]>; Jonathan Cameron <[email protected]>; [email protected]; linux- [email protected]
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.

Quoted 3 lines

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

---

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

Acked-by Nuno Sá <[email protected]>

Quoted 4 lines

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

Quoted 3 lines

Yours,
-- Matti

Quoted 38 lines · On Sun, 16 Oct 2022 17:15:29 +0100 Jonathan Cameron <[email protected]> wrote:

Quoted 35 lines · On Tue, 30 Aug 2022 15:54:07 +0300 Matti Vaittinen <[email protected]>…

Thanks for the review(s) Nuno!

Quoted 19 lines · Nuno

Quoted 19 lines

From: Matti Vaittinen <[email protected]>
Sent: Friday, August 19, 2022 9:20 PM
To: Matti Vaittinen <[email protected]>; Matti Vaittinen <[email protected]>
Cc: Lars-Peter Clausen <[email protected]>; Hennerich, Michael <[email protected]>; Jonathan Cameron <[email protected]>; [email protected]; linux- [email protected]
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.

Quoted 8 lines

Quoted 3 lines

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

---

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

Acked-by Nuno Sá <[email protected]>

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.

Quoted 4 lines

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.

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

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

---
v2 => v3:
New patch

---
  drivers/iio/adc/max1241.c | 28 +++-------------------------
  1 file changed, 3 insertions(+), 25 deletions(-)

drivers/iio/adc/max1241.c +3 -25

index a815ad1f6913..500bb09ab19b 100644
@@ -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))

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 <[email protected]>

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

Quoted 69 lines

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

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

---
v2 => v3:
New patch

---
  drivers/iio/adc/max1241.c | 28 +++-------------------------
  1 file changed, 3 insertions(+), 25 deletions(-)

drivers/iio/adc/max1241.c +3 -25

index a815ad1f6913..500bb09ab19b 100644
@@ -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))  

-----Original Message-----
From: Matti Vaittinen <[email protected]>
Sent: Friday, August 19, 2022 9:21 PM
To: Matti Vaittinen <[email protected]>; Matti Vaittinen <[email protected]>
Cc: Jonathan Cameron <[email protected]>; Lars-Peter Clausen <[email protected]>; Matti Vaittinen <[email protected]>; Alexandru Ardelean <[email protected]>; linux- [email protected]; [email protected]
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 <[email protected]>

---

Quoted 20 lines

-----Original Message-----
From: Matti Vaittinen <[email protected]>
Sent: Friday, August 19, 2022 9:21 PM
To: Matti Vaittinen <[email protected]>; Matti Vaittinen <[email protected]>
Cc: Jonathan Cameron <[email protected]>; Lars-Peter Clausen <[email protected]>; Matti Vaittinen <[email protected]>; Alexandru Ardelean <[email protected]>; linux- [email protected]; [email protected]
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.

Quoted 3 lines

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

---

Acked-by Nuno Sá <[email protected]>

Quoted 20 lines · Nuno

Quoted 20 lines

-----Original Message-----
From: Matti Vaittinen <[email protected]>
Sent: Friday, August 19, 2022 9:21 PM
To: Matti Vaittinen <[email protected]>; Matti Vaittinen <[email protected]>
Cc: Jonathan Cameron <[email protected]>; Lars-Peter Clausen <[email protected]>; Matti Vaittinen <[email protected]>; Alexandru Ardelean <[email protected]>; linux- [email protected]; [email protected]
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.

Quoted 5 lines

Quoted 3 lines

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

---

Acked-by Nuno Sá <[email protected]>

Applied.

From: Matti Vaittinen <[email protected]>
Sent: Friday, August 19, 2022 9:21 PM
To: Matti Vaittinen <[email protected]>; Matti Vaittinen <[email protected]>
Cc: Lars-Peter Clausen <[email protected]>; Hennerich, Michael <[email protected]>; Jonathan Cameron <[email protected]>; [email protected]; linux- [email protected]
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 <[email protected]>

---

Quoted 20 lines

From: Matti Vaittinen <[email protected]>
Sent: Friday, August 19, 2022 9:21 PM
To: Matti Vaittinen <[email protected]>; Matti Vaittinen <[email protected]>
Cc: Lars-Peter Clausen <[email protected]>; Hennerich, Michael <[email protected]>; Jonathan Cameron <[email protected]>; [email protected]; linux- [email protected]
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.

Quoted 3 lines

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

---

Acked-by Nuno Sá <[email protected]>

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

-----Original Message-----
From: Matti Vaittinen <[email protected]>
Sent: Tuesday, August 30, 2022 3:00 PM
To: Sa, Nuno <[email protected]>; Matti Vaittinen <[email protected]>
Cc: Lars-Peter Clausen <[email protected]>; Hennerich, Michael <[email protected]>; Jonathan Cameron <[email protected]>; [email protected]; linux- [email protected]
Subject: Re: [PATCH v3 14/14] iio: hmc425a: simplify using devm_regulator_get_enable()

[External]

Quoted 12 lines · Nuno

Quoted 12 lines

From: Matti Vaittinen <[email protected]>
Sent: Friday, August 19, 2022 9:21 PM
To: Matti Vaittinen <[email protected]>; Matti Vaittinen <[email protected]>
Cc: Lars-Peter Clausen <[email protected]>; Hennerich, Michael <[email protected]>; Jonathan Cameron <[email protected]>; [email protected]; linux- [email protected]
Subject: [PATCH v3 14/14] iio: hmc425a: simplify using devm_regulator_get_enable()

Quoted 3 lines

Quoted 3 lines

[External]

Drop open-coded pattern: 'devm_regulator_get(),

regulator_enable(),

Quoted 2 lines

Quoted 2 lines

add_action_or_reset(regulator_disable)' and use the devm_regulator_get_enable() and drop the pointer to the

regulator.

Quoted 1 line

Quoted 1 line

This simplifies code and makes it less tempting to add manual

control

Quoted 9 lines

Quoted 5 lines

for the regulator which is also controlled by devm.

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

---

Acked-by Nuno Sá <[email protected]>

(I see that in this patch you are not using dev_err_probe(). Maybe

some

Quoted 1 line

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

Quoted 49 lines

-----Original Message-----
From: Matti Vaittinen <[email protected]>
Sent: Tuesday, August 30, 2022 3:00 PM
To: Sa, Nuno <[email protected]>; Matti Vaittinen <[email protected]>
Cc: Lars-Peter Clausen <[email protected]>; Hennerich, Michael <[email protected]>; Jonathan Cameron <[email protected]>; [email protected]; linux- [email protected]
Subject: Re: [PATCH v3 14/14] iio: hmc425a: simplify using devm_regulator_get_enable()

[External]

Quoted 12 lines · Nuno

Quoted 12 lines

From: Matti Vaittinen <[email protected]>
Sent: Friday, August 19, 2022 9:21 PM
To: Matti Vaittinen <[email protected]>; Matti Vaittinen <[email protected]>
Cc: Lars-Peter Clausen <[email protected]>; Hennerich, Michael <[email protected]>; Jonathan Cameron <[email protected]>; [email protected]; linux- [email protected]
Subject: [PATCH v3 14/14] iio: hmc425a: simplify using devm_regulator_get_enable()

Quoted 3 lines

Quoted 3 lines

[External]

Drop open-coded pattern: 'devm_regulator_get(),

regulator_enable(),

Quoted 2 lines

Quoted 2 lines

add_action_or_reset(regulator_disable)' and use the devm_regulator_get_enable() and drop the pointer to the

regulator.

Quoted 1 line

Quoted 1 line

This simplifies code and makes it less tempting to add manual

control

Quoted 9 lines

Quoted 5 lines

for the regulator which is also controlled by devm.

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

---

Acked-by Nuno Sá <[email protected]>

(I see that in this patch you are not using dev_err_probe(). Maybe

some

Quoted 1 line

consistency in the series and where applicable would be appropriate

:))

Quoted 5 lines

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á

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

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

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:

[...]