[PATCH 0/2] regulator: max77826: Constify and simplify

[PATCH 0/2] regulator: max77826: Constify and simplify

[Christophe JAILLET]

Patch 1 is just a constification path. It should be straighforward.


Patch 2 is COMPLETELY SPECULATIVE. Some code and memory allocation
*LOOK* useless. So review with care!
Looking are lore does not give any information about this code to me.
v1: https://lore.kernel.org/all/20200413164440.1138178-1-iskren.chernev@gmail.com/
v2: https://lore.kernel.org/all/20200414172250.2363235-1-iskren.chernev@gmail.com/

It *looks* like code written to release some resources in a .remove()
function, maybe used before switchwing to devm_ function.

Christophe JAILLET (2):
  regulator: max77826: Constify struct regulator_desc
  regulator: max77826: Simplify max77826_i2c_probe()

 drivers/regulator/max77826-regulator.c | 20 +-------------------
 1 file changed, 1 insertion(+), 19 deletions(-)

-- 
2.46.0

[PATCH 1/2] regulator: max77826: Constify struct regulator_desc

[Christophe JAILLET]

'struct regulator_desc' is not modified in this driver.

Constifying this structure moves some data to a read-only section, so
increase overall security, especially when the structure holds some
function pointers.

On a x86_64, with allmodconfig:
Before:
======
   text	   data	    bss	    dec	    hex	filename
   3906	   5808	     16	   9730	   2602	drivers/regulator/max77826-regulator.o

After:
=====
   text	   data	    bss	    dec	    hex	filename
   9218	    496	     16	   9730	   2602	drivers/regulator/max77826-regulator.o

Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
--
Compile tested only
---
 drivers/regulator/max77826-regulator.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/regulator/max77826-regulator.c b/drivers/regulator/max77826-regulator.c
index 5590cdf615b7..376e3110c695 100644
--- a/drivers/regulator/max77826-regulator.c
+++ b/drivers/regulator/max77826-regulator.c
@@ -153,7 +153,7 @@ enum max77826_regulators {
 
 struct max77826_regulator_info {
 	struct regmap *regmap;
-	struct regulator_desc *rdesc;
+	const struct regulator_desc *rdesc;
 };
 
 static const struct regmap_config max77826_regmap_config = {
@@ -187,7 +187,7 @@ static const struct regulator_ops max77826_buck_ops = {
 	.set_voltage_time_sel	= max77826_set_voltage_time_sel,
 };
 
-static struct regulator_desc max77826_regulators_desc[] = {
+static const struct regulator_desc max77826_regulators_desc[] = {
 	MAX77826_LDO(1, NMOS),
 	MAX77826_LDO(2, NMOS),
 	MAX77826_LDO(3, NMOS),
-- 
2.46.0

[PATCH 2/2] regulator: max77826: Simplify max77826_i2c_probe()

[Christophe JAILLET]

'struct max77826_regulator_info' is unused and can be removed.

There is no i2c_get_clientdata().
Resources are managed, so there is no need to keep references unless
explicitly needed.

Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
--
Compile tested only.

This patch IS SPECULATIVE, review with care!
---
 drivers/regulator/max77826-regulator.c | 18 ------------------
 1 file changed, 18 deletions(-)

diff --git a/drivers/regulator/max77826-regulator.c b/drivers/regulator/max77826-regulator.c
index 376e3110c695..3b12ad361222 100644
--- a/drivers/regulator/max77826-regulator.c
+++ b/drivers/regulator/max77826-regulator.c
@@ -149,13 +149,6 @@ enum max77826_regulators {
 		.owner = THIS_MODULE,					\
 	}
 
-
-
-struct max77826_regulator_info {
-	struct regmap *regmap;
-	const struct regulator_desc *rdesc;
-};
-
 static const struct regmap_config max77826_regmap_config = {
 	.reg_bits = 8,
 	.val_bits = 8,
@@ -235,30 +228,19 @@ static int max77826_read_device_id(struct regmap *regmap, struct device *dev)
 static int max77826_i2c_probe(struct i2c_client *client)
 {
 	struct device *dev = &client->dev;
-	struct max77826_regulator_info *info;
 	struct regulator_config config = {};
 	struct regulator_dev *rdev;
 	struct regmap *regmap;
 	int i;
 
-	info = devm_kzalloc(dev, sizeof(struct max77826_regulator_info),
-				GFP_KERNEL);
-	if (!info)
-		return -ENOMEM;
-
-	info->rdesc = max77826_regulators_desc;
 	regmap = devm_regmap_init_i2c(client, &max77826_regmap_config);
 	if (IS_ERR(regmap)) {
 		dev_err(dev, "Failed to allocate regmap!\n");
 		return PTR_ERR(regmap);
 	}
 
-	info->regmap = regmap;
-	i2c_set_clientdata(client, info);
-
 	config.dev = dev;
 	config.regmap = regmap;
-	config.driver_data = info;
 
 	for (i = 0; i < MAX77826_MAX_REGULATORS; i++) {
 		rdev = devm_regulator_register(dev,
-- 
2.46.0

Re: [PATCH 1/2] regulator: max77826: Constify struct regulator_desc

[Iskren Chernev]

Reviewed-by: Iskren Chernev <iskren.chernev@gmail.com>

On Sun, Sep 8, 2024 at 2:41 PM Christophe JAILLET
<christophe.jaillet@wanadoo.fr> wrote:
>
> 'struct regulator_desc' is not modified in this driver.
>
> Constifying this structure moves some data to a read-only section, so
> increase overall security, especially when the structure holds some
> function pointers.
>
> On a x86_64, with allmodconfig:
> Before:
> ======
>    text    data     bss     dec     hex filename
>    3906    5808      16    9730    2602 drivers/regulator/max77826-regulator.o
>
> After:
> =====
>    text    data     bss     dec     hex filename
>    9218     496      16    9730    2602 drivers/regulator/max77826-regulator.o
>
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> --
> Compile tested only
> ---
>  drivers/regulator/max77826-regulator.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/regulator/max77826-regulator.c b/drivers/regulator/max77826-regulator.c
> index 5590cdf615b7..376e3110c695 100644
> --- a/drivers/regulator/max77826-regulator.c
> +++ b/drivers/regulator/max77826-regulator.c
> @@ -153,7 +153,7 @@ enum max77826_regulators {
>
>  struct max77826_regulator_info {
>         struct regmap *regmap;
> -       struct regulator_desc *rdesc;
> +       const struct regulator_desc *rdesc;
>  };
>
>  static const struct regmap_config max77826_regmap_config = {
> @@ -187,7 +187,7 @@ static const struct regulator_ops max77826_buck_ops = {
>         .set_voltage_time_sel   = max77826_set_voltage_time_sel,
>  };
>
> -static struct regulator_desc max77826_regulators_desc[] = {
> +static const struct regulator_desc max77826_regulators_desc[] = {
>         MAX77826_LDO(1, NMOS),
>         MAX77826_LDO(2, NMOS),
>         MAX77826_LDO(3, NMOS),
> --
> 2.46.0
>

Re: [PATCH 2/2] regulator: max77826: Simplify max77826_i2c_probe()

[Iskren Chernev]

This was my first kernel patch, I didn't understand most of the details.
I guess I left it because of the i2d_set_clientdata, but nothing is accessing
it, so I guess it's safe to drop.

Reviewed-by: Iskren Chernev <iskren.chernev@gmail.com>

On Sun, Sep 8, 2024 at 2:41 PM Christophe JAILLET
<christophe.jaillet@wanadoo.fr> wrote:
>
> 'struct max77826_regulator_info' is unused and can be removed.
>
> There is no i2c_get_clientdata().
> Resources are managed, so there is no need to keep references unless
> explicitly needed.
>
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> --
> Compile tested only.
>
> This patch IS SPECULATIVE, review with care!
> ---
>  drivers/regulator/max77826-regulator.c | 18 ------------------
>  1 file changed, 18 deletions(-)
>
> diff --git a/drivers/regulator/max77826-regulator.c b/drivers/regulator/max77826-regulator.c
> index 376e3110c695..3b12ad361222 100644
> --- a/drivers/regulator/max77826-regulator.c
> +++ b/drivers/regulator/max77826-regulator.c
> @@ -149,13 +149,6 @@ enum max77826_regulators {
>                 .owner = THIS_MODULE,                                   \
>         }
>
> -
> -
> -struct max77826_regulator_info {
> -       struct regmap *regmap;
> -       const struct regulator_desc *rdesc;
> -};
> -
>  static const struct regmap_config max77826_regmap_config = {
>         .reg_bits = 8,
>         .val_bits = 8,
> @@ -235,30 +228,19 @@ static int max77826_read_device_id(struct regmap *regmap, struct device *dev)
>  static int max77826_i2c_probe(struct i2c_client *client)
>  {
>         struct device *dev = &client->dev;
> -       struct max77826_regulator_info *info;
>         struct regulator_config config = {};
>         struct regulator_dev *rdev;
>         struct regmap *regmap;
>         int i;
>
> -       info = devm_kzalloc(dev, sizeof(struct max77826_regulator_info),
> -                               GFP_KERNEL);
> -       if (!info)
> -               return -ENOMEM;
> -
> -       info->rdesc = max77826_regulators_desc;
>         regmap = devm_regmap_init_i2c(client, &max77826_regmap_config);
>         if (IS_ERR(regmap)) {
>                 dev_err(dev, "Failed to allocate regmap!\n");
>                 return PTR_ERR(regmap);
>         }
>
> -       info->regmap = regmap;
> -       i2c_set_clientdata(client, info);
> -
>         config.dev = dev;
>         config.regmap = regmap;
> -       config.driver_data = info;
>
>         for (i = 0; i < MAX77826_MAX_REGULATORS; i++) {
>                 rdev = devm_regulator_register(dev,
> --
> 2.46.0
>