[PATCH 0/2] s2mps11 code refactoring

[PATCH 0/2] s2mps11 code refactoring

[Andi Shyti]

Hi,

the two patches are related to some refactoring of the s2mps11
device driver.

The first patch is quite trivial, but it removes a variable which
I consider redundant and doesn't improve any readability.

The second patch is removing some structures which will be never
used in the kernel. The structure removed are merged in a single
more generic structure.

Thanks,
Andi

Andi Shyti (2):
  clk: s2mps11: remove redundant variable
  clk: s2mps11: allocate only one structure for clock init

 drivers/clk/clk-s2mps11.c | 82 +++++++++++++----------------------------------
 1 file changed, 23 insertions(+), 59 deletions(-)

-- 
2.6.4

[PATCH 1/2] clk: s2mps11: remove redundant variable

[Andi Shyti]

In the probe function the s2mps11_clk pointer is used only to
iterate through the s2mps11_clks. The naming itself brings
confusion and the readability is not improved.

Remove it, save some memory and headaches due to name similarity.

Signed-off-by: Andi Shyti <andi.shyti@samsung.com>
---
 drivers/clk/clk-s2mps11.c | 36 +++++++++++++++++-------------------
 1 file changed, 17 insertions(+), 19 deletions(-)

diff --git a/drivers/clk/clk-s2mps11.c b/drivers/clk/clk-s2mps11.c
index d266299..ccb12f1 100644
--- a/drivers/clk/clk-s2mps11.c
+++ b/drivers/clk/clk-s2mps11.c
@@ -28,7 +28,7 @@
 #include <linux/mfd/samsung/s5m8767.h>
 #include <linux/mfd/samsung/core.h>
 
-#define s2mps11_name(a) (a->hw.init->name)
+#define s2mps11_name(a) (a.hw.init->name)
 
 static struct clk **clk_table;
 static struct clk_onecell_data clk_data;
@@ -177,18 +177,16 @@ static struct device_node *s2mps11_clk_parse_dt(struct platform_device *pdev,
 static int s2mps11_clk_probe(struct platform_device *pdev)
 {
 	struct sec_pmic_dev *iodev = dev_get_drvdata(pdev->dev.parent);
-	struct s2mps11_clk *s2mps11_clks, *s2mps11_clk;
+	struct s2mps11_clk *s2mps11_clks;
 	unsigned int s2mps11_reg;
 	struct clk_init_data *clks_init;
 	int i, ret = 0;
 
 	s2mps11_clks = devm_kcalloc(&pdev->dev, S2MPS11_CLKS_NUM,
-				sizeof(*s2mps11_clk), GFP_KERNEL);
+				sizeof(*s2mps11_clks), GFP_KERNEL);
 	if (!s2mps11_clks)
 		return -ENOMEM;
 
-	s2mps11_clk = s2mps11_clks;
-
 	clk_table = devm_kcalloc(&pdev->dev, S2MPS11_CLKS_NUM,
 				sizeof(struct clk *), GFP_KERNEL);
 	if (!clk_table)
@@ -221,26 +219,26 @@ static int s2mps11_clk_probe(struct platform_device *pdev)
 	if (IS_ERR(s2mps11_clks->clk_np))
 		return PTR_ERR(s2mps11_clks->clk_np);
 
-	for (i = 0; i < S2MPS11_CLKS_NUM; i++, s2mps11_clk++) {
+	for (i = 0; i < S2MPS11_CLKS_NUM; i++) {
 		if (!clks_init[i].name)
 			continue; /* Skip clocks not present in some devices */
-		s2mps11_clk->iodev = iodev;
-		s2mps11_clk->hw.init = &clks_init[i];
-		s2mps11_clk->mask = 1 << i;
-		s2mps11_clk->reg = s2mps11_reg;
-
-		s2mps11_clk->clk = devm_clk_register(&pdev->dev,
-							&s2mps11_clk->hw);
-		if (IS_ERR(s2mps11_clk->clk)) {
+		s2mps11_clks[i].iodev = iodev;
+		s2mps11_clks[i].hw.init = &clks_init[i];
+		s2mps11_clks[i].mask = 1 << i;
+		s2mps11_clks[i].reg = s2mps11_reg;
+
+		s2mps11_clks[i].clk = devm_clk_register(&pdev->dev,
+							&s2mps11_clks[i].hw);
+		if (IS_ERR(s2mps11_clks[i].clk)) {
 			dev_err(&pdev->dev, "Fail to register : %s\n",
-						s2mps11_name(s2mps11_clk));
-			ret = PTR_ERR(s2mps11_clk->clk);
+						s2mps11_name(s2mps11_clks[i]));
+			ret = PTR_ERR(s2mps11_clks[i].clk);
 			goto err_reg;
 		}
 
-		s2mps11_clk->lookup = clkdev_create(s2mps11_clk->clk,
-					s2mps11_name(s2mps11_clk), NULL);
-		if (!s2mps11_clk->lookup) {
+		s2mps11_clks[i].lookup = clkdev_create(s2mps11_clks[i].clk,
+					s2mps11_name(s2mps11_clks[i]), NULL);
+		if (!s2mps11_clks[i].lookup) {
 			ret = -ENOMEM;
 			goto err_reg;
 		}
-- 
2.6.4

Re: [PATCH 1/2] clk: s2mps11: remove redundant variable

[Krzysztof Kozlowski]

On 18.01.2016 17:25, Andi Shyti wrote:
> In the probe function the s2mps11_clk pointer is used only to
> iterate through the s2mps11_clks. The naming itself brings
> confusion and the readability is not improved.
> 
> Remove it, save some memory and headaches due to name similarity.

Moving the declaration of 's2mps11_clk' into the for-loop could be also
a way of improving the readability (narrow scope of variable) but your
approach also looks fine.

Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>

Best regards,
Krzysztof

> 
> Signed-off-by: Andi Shyti <andi.shyti@samsung.com>
> ---
>  drivers/clk/clk-s2mps11.c | 36 +++++++++++++++++-------------------
>  1 file changed, 17 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/clk/clk-s2mps11.c b/drivers/clk/clk-s2mps11.c
> index d266299..ccb12f1 100644
> --- a/drivers/clk/clk-s2mps11.c
> +++ b/drivers/clk/clk-s2mps11.c
> @@ -28,7 +28,7 @@
>  #include <linux/mfd/samsung/s5m8767.h>
>  #include <linux/mfd/samsung/core.h>
>  
> -#define s2mps11_name(a) (a->hw.init->name)
> +#define s2mps11_name(a) (a.hw.init->name)
>  
>  static struct clk **clk_table;
>  static struct clk_onecell_data clk_data;
> @@ -177,18 +177,16 @@ static struct device_node *s2mps11_clk_parse_dt(struct platform_device *pdev,
>  static int s2mps11_clk_probe(struct platform_device *pdev)
>  {
>  	struct sec_pmic_dev *iodev = dev_get_drvdata(pdev->dev.parent);
> -	struct s2mps11_clk *s2mps11_clks, *s2mps11_clk;
> +	struct s2mps11_clk *s2mps11_clks;
>  	unsigned int s2mps11_reg;
>  	struct clk_init_data *clks_init;
>  	int i, ret = 0;
>  
>  	s2mps11_clks = devm_kcalloc(&pdev->dev, S2MPS11_CLKS_NUM,
> -				sizeof(*s2mps11_clk), GFP_KERNEL);
> +				sizeof(*s2mps11_clks), GFP_KERNEL);
>  	if (!s2mps11_clks)
>  		return -ENOMEM;
>  
> -	s2mps11_clk = s2mps11_clks;
> -
>  	clk_table = devm_kcalloc(&pdev->dev, S2MPS11_CLKS_NUM,
>  				sizeof(struct clk *), GFP_KERNEL);
>  	if (!clk_table)
> @@ -221,26 +219,26 @@ static int s2mps11_clk_probe(struct platform_device *pdev)
>  	if (IS_ERR(s2mps11_clks->clk_np))
>  		return PTR_ERR(s2mps11_clks->clk_np);
>  
> -	for (i = 0; i < S2MPS11_CLKS_NUM; i++, s2mps11_clk++) {
> +	for (i = 0; i < S2MPS11_CLKS_NUM; i++) {
>  		if (!clks_init[i].name)
>  			continue; /* Skip clocks not present in some devices */
> -		s2mps11_clk->iodev = iodev;
> -		s2mps11_clk->hw.init = &clks_init[i];
> -		s2mps11_clk->mask = 1 << i;
> -		s2mps11_clk->reg = s2mps11_reg;
> -
> -		s2mps11_clk->clk = devm_clk_register(&pdev->dev,
> -							&s2mps11_clk->hw);
> -		if (IS_ERR(s2mps11_clk->clk)) {
> +		s2mps11_clks[i].iodev = iodev;
> +		s2mps11_clks[i].hw.init = &clks_init[i];
> +		s2mps11_clks[i].mask = 1 << i;
> +		s2mps11_clks[i].reg = s2mps11_reg;
> +
> +		s2mps11_clks[i].clk = devm_clk_register(&pdev->dev,
> +							&s2mps11_clks[i].hw);
> +		if (IS_ERR(s2mps11_clks[i].clk)) {
>  			dev_err(&pdev->dev, "Fail to register : %s\n",
> -						s2mps11_name(s2mps11_clk));
> -			ret = PTR_ERR(s2mps11_clk->clk);
> +						s2mps11_name(s2mps11_clks[i]));
> +			ret = PTR_ERR(s2mps11_clks[i].clk);
>  			goto err_reg;
>  		}
>  
> -		s2mps11_clk->lookup = clkdev_create(s2mps11_clk->clk,
> -					s2mps11_name(s2mps11_clk), NULL);
> -		if (!s2mps11_clk->lookup) {
> +		s2mps11_clks[i].lookup = clkdev_create(s2mps11_clks[i].clk,
> +					s2mps11_name(s2mps11_clks[i]), NULL);
> +		if (!s2mps11_clks[i].lookup) {
>  			ret = -ENOMEM;
>  			goto err_reg;
>  		}
> 

[PATCH 2/2] clk: s2mps11: allocate only one structure for clock init

[Andi Shyti]

The driver allocates three structures for three different clock
types. They are quite similar and in the clock init data they
differ only by the name. Only one of these structure is used,
while the others lie unused in the memory.

The clock's name, though, is not such a meaningful information
and by assigning the same name to the initial data we can avoid
over allocation. The common name chosen will be s2mps11,
coherently with the device driver name, instead of the clock
device.

Therefore, remove the structures associated to s2mps13 and
s2mps14 and use only the one referred to s2mps11 for all kind of
clocks.

Signed-off-by: Andi Shyti <andi.shyti@samsung.com>
Suggested-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
---
 drivers/clk/clk-s2mps11.c | 48 +++++++----------------------------------------
 1 file changed, 7 insertions(+), 41 deletions(-)

diff --git a/drivers/clk/clk-s2mps11.c b/drivers/clk/clk-s2mps11.c
index ccb12f1..dce9753 100644
--- a/drivers/clk/clk-s2mps11.c
+++ b/drivers/clk/clk-s2mps11.c
@@ -99,6 +99,7 @@ static struct clk_ops s2mps11_clk_ops = {
 	.recalc_rate	= s2mps11_clk_recalc_rate,
 };
 
+/* This ss2mps11_clks_init tructure is common to s2mps11, s2mps13 and s2mps14 */
 static struct clk_init_data s2mps11_clks_init[S2MPS11_CLKS_NUM] = {
 	[S2MPS11_CLK_AP] = {
 		.name = "s2mps11_ap",
@@ -117,37 +118,6 @@ static struct clk_init_data s2mps11_clks_init[S2MPS11_CLKS_NUM] = {
 	},
 };
 
-static struct clk_init_data s2mps13_clks_init[S2MPS11_CLKS_NUM] = {
-	[S2MPS11_CLK_AP] = {
-		.name = "s2mps13_ap",
-		.ops = &s2mps11_clk_ops,
-		.flags = CLK_IS_ROOT,
-	},
-	[S2MPS11_CLK_CP] = {
-		.name = "s2mps13_cp",
-		.ops = &s2mps11_clk_ops,
-		.flags = CLK_IS_ROOT,
-	},
-	[S2MPS11_CLK_BT] = {
-		.name = "s2mps13_bt",
-		.ops = &s2mps11_clk_ops,
-		.flags = CLK_IS_ROOT,
-	},
-};
-
-static struct clk_init_data s2mps14_clks_init[S2MPS11_CLKS_NUM] = {
-	[S2MPS11_CLK_AP] = {
-		.name = "s2mps14_ap",
-		.ops = &s2mps11_clk_ops,
-		.flags = CLK_IS_ROOT,
-	},
-	[S2MPS11_CLK_BT] = {
-		.name = "s2mps14_bt",
-		.ops = &s2mps11_clk_ops,
-		.flags = CLK_IS_ROOT,
-	},
-};
-
 static struct device_node *s2mps11_clk_parse_dt(struct platform_device *pdev,
 		struct clk_init_data *clks_init)
 {
@@ -179,8 +149,8 @@ static int s2mps11_clk_probe(struct platform_device *pdev)
 	struct sec_pmic_dev *iodev = dev_get_drvdata(pdev->dev.parent);
 	struct s2mps11_clk *s2mps11_clks;
 	unsigned int s2mps11_reg;
-	struct clk_init_data *clks_init;
 	int i, ret = 0;
+	enum sec_device_type hwid = platform_get_device_id(pdev)->driver_data;
 
 	s2mps11_clks = devm_kcalloc(&pdev->dev, S2MPS11_CLKS_NUM,
 				sizeof(*s2mps11_clks), GFP_KERNEL);
@@ -192,22 +162,18 @@ static int s2mps11_clk_probe(struct platform_device *pdev)
 	if (!clk_table)
 		return -ENOMEM;
 
-	switch(platform_get_device_id(pdev)->driver_data) {
+	switch (hwid) {
 	case S2MPS11X:
 		s2mps11_reg = S2MPS11_REG_RTC_CTRL;
-		clks_init = s2mps11_clks_init;
 		break;
 	case S2MPS13X:
 		s2mps11_reg = S2MPS13_REG_RTCCTRL;
-		clks_init = s2mps13_clks_init;
 		break;
 	case S2MPS14X:
 		s2mps11_reg = S2MPS14_REG_RTCCTRL;
-		clks_init = s2mps14_clks_init;
 		break;
 	case S5M8767X:
 		s2mps11_reg = S5M8767_REG_CTRL1;
-		clks_init = s2mps11_clks_init;
 		break;
 	default:
 		dev_err(&pdev->dev, "Invalid device type\n");
@@ -215,15 +181,15 @@ static int s2mps11_clk_probe(struct platform_device *pdev)
 	}
 
 	/* Store clocks of_node in first element of s2mps11_clks array */
-	s2mps11_clks->clk_np = s2mps11_clk_parse_dt(pdev, clks_init);
+	s2mps11_clks->clk_np = s2mps11_clk_parse_dt(pdev, s2mps11_clks_init);
 	if (IS_ERR(s2mps11_clks->clk_np))
 		return PTR_ERR(s2mps11_clks->clk_np);
 
 	for (i = 0; i < S2MPS11_CLKS_NUM; i++) {
-		if (!clks_init[i].name)
+		if (i == S2MPS11_CLK_CP && hwid == S2MPS14X)
 			continue; /* Skip clocks not present in some devices */
 		s2mps11_clks[i].iodev = iodev;
-		s2mps11_clks[i].hw.init = &clks_init[i];
+		s2mps11_clks[i].hw.init = &s2mps11_clks_init[i];
 		s2mps11_clks[i].mask = 1 << i;
 		s2mps11_clks[i].reg = s2mps11_reg;
 
@@ -246,7 +212,7 @@ static int s2mps11_clk_probe(struct platform_device *pdev)
 
 	for (i = 0; i < S2MPS11_CLKS_NUM; i++) {
 		/* Skip clocks not present on S2MPS14 */
-		if (!clks_init[i].name)
+		if (i == S2MPS11_CLK_CP && hwid == S2MPS14X)
 			continue;
 		clk_table[i] = s2mps11_clks[i].clk;
 	}
-- 
2.6.4

Re: [PATCH 2/2] clk: s2mps11: allocate only one structure for clock init

[Krzysztof Kozlowski]

On 18.01.2016 17:25, Andi Shyti wrote:
> The driver allocates three structures for three different clock
> types. They are quite similar and in the clock init data they
> differ only by the name. Only one of these structure is used,

s/structure/structures/

> while the others lie unused in the memory.
> 
> The clock's name, though, is not such a meaningful information
> and by assigning the same name to the initial data we can avoid
> over allocation. The common name chosen will be s2mps11,
> coherently with the device driver name, instead of the clock
> device.
> 
> Therefore, remove the structures associated to s2mps13 and
> s2mps14 and use only the one referred to s2mps11 for all kind of
> clocks.
> 
> Signed-off-by: Andi Shyti <andi.shyti@samsung.com>
> Suggested-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> ---
>  drivers/clk/clk-s2mps11.c | 48 +++++++----------------------------------------
>  1 file changed, 7 insertions(+), 41 deletions(-)
> 
> diff --git a/drivers/clk/clk-s2mps11.c b/drivers/clk/clk-s2mps11.c
> index ccb12f1..dce9753 100644
> --- a/drivers/clk/clk-s2mps11.c
> +++ b/drivers/clk/clk-s2mps11.c
> @@ -99,6 +99,7 @@ static struct clk_ops s2mps11_clk_ops = {
>  	.recalc_rate	= s2mps11_clk_recalc_rate,
>  };
>  
> +/* This ss2mps11_clks_init tructure is common to s2mps11, s2mps13 and s2mps14 */

s/ss2mps11_clks_init/s2mps11_clks_init/

>  static struct clk_init_data s2mps11_clks_init[S2MPS11_CLKS_NUM] = {
>  	[S2MPS11_CLK_AP] = {
>  		.name = "s2mps11_ap",
> @@ -117,37 +118,6 @@ static struct clk_init_data s2mps11_clks_init[S2MPS11_CLKS_NUM] = {
>  	},
>  };
>  
> -static struct clk_init_data s2mps13_clks_init[S2MPS11_CLKS_NUM] = {
> -	[S2MPS11_CLK_AP] = {
> -		.name = "s2mps13_ap",
> -		.ops = &s2mps11_clk_ops,
> -		.flags = CLK_IS_ROOT,
> -	},
> -	[S2MPS11_CLK_CP] = {
> -		.name = "s2mps13_cp",
> -		.ops = &s2mps11_clk_ops,
> -		.flags = CLK_IS_ROOT,
> -	},
> -	[S2MPS11_CLK_BT] = {
> -		.name = "s2mps13_bt",
> -		.ops = &s2mps11_clk_ops,
> -		.flags = CLK_IS_ROOT,
> -	},
> -};
> -
> -static struct clk_init_data s2mps14_clks_init[S2MPS11_CLKS_NUM] = {
> -	[S2MPS11_CLK_AP] = {
> -		.name = "s2mps14_ap",
> -		.ops = &s2mps11_clk_ops,
> -		.flags = CLK_IS_ROOT,
> -	},
> -	[S2MPS11_CLK_BT] = {
> -		.name = "s2mps14_bt",
> -		.ops = &s2mps11_clk_ops,
> -		.flags = CLK_IS_ROOT,
> -	},
> -};
> -
>  static struct device_node *s2mps11_clk_parse_dt(struct platform_device *pdev,
>  		struct clk_init_data *clks_init)
>  {

The check if (!clks_init[i].name) in this function is meaningless now.
How will device with S2MPS14 behave?

Best regards,
Krzysztof


> @@ -179,8 +149,8 @@ static int s2mps11_clk_probe(struct platform_device *pdev)
>  	struct sec_pmic_dev *iodev = dev_get_drvdata(pdev->dev.parent);
>  	struct s2mps11_clk *s2mps11_clks;
>  	unsigned int s2mps11_reg;
> -	struct clk_init_data *clks_init;
>  	int i, ret = 0;
> +	enum sec_device_type hwid = platform_get_device_id(pdev)->driver_data;
>  
>  	s2mps11_clks = devm_kcalloc(&pdev->dev, S2MPS11_CLKS_NUM,
>  				sizeof(*s2mps11_clks), GFP_KERNEL);
> @@ -192,22 +162,18 @@ static int s2mps11_clk_probe(struct platform_device *pdev)
>  	if (!clk_table)
>  		return -ENOMEM;
>  
> -	switch(platform_get_device_id(pdev)->driver_data) {
> +	switch (hwid) {
>  	case S2MPS11X:
>  		s2mps11_reg = S2MPS11_REG_RTC_CTRL;
> -		clks_init = s2mps11_clks_init;
>  		break;
>  	case S2MPS13X:
>  		s2mps11_reg = S2MPS13_REG_RTCCTRL;
> -		clks_init = s2mps13_clks_init;
>  		break;
>  	case S2MPS14X:
>  		s2mps11_reg = S2MPS14_REG_RTCCTRL;
> -		clks_init = s2mps14_clks_init;
>  		break;
>  	case S5M8767X:
>  		s2mps11_reg = S5M8767_REG_CTRL1;
> -		clks_init = s2mps11_clks_init;
>  		break;
>  	default:
>  		dev_err(&pdev->dev, "Invalid device type\n");
> @@ -215,15 +181,15 @@ static int s2mps11_clk_probe(struct platform_device *pdev)
>  	}
>  
>  	/* Store clocks of_node in first element of s2mps11_clks array */
> -	s2mps11_clks->clk_np = s2mps11_clk_parse_dt(pdev, clks_init);
> +	s2mps11_clks->clk_np = s2mps11_clk_parse_dt(pdev, s2mps11_clks_init);
>  	if (IS_ERR(s2mps11_clks->clk_np))
>  		return PTR_ERR(s2mps11_clks->clk_np);
>  
>  	for (i = 0; i < S2MPS11_CLKS_NUM; i++) {
> -		if (!clks_init[i].name)
> +		if (i == S2MPS11_CLK_CP && hwid == S2MPS14X)
>  			continue; /* Skip clocks not present in some devices */
>  		s2mps11_clks[i].iodev = iodev;
> -		s2mps11_clks[i].hw.init = &clks_init[i];
> +		s2mps11_clks[i].hw.init = &s2mps11_clks_init[i];
>  		s2mps11_clks[i].mask = 1 << i;
>  		s2mps11_clks[i].reg = s2mps11_reg;
>  
> @@ -246,7 +212,7 @@ static int s2mps11_clk_probe(struct platform_device *pdev)
>  
>  	for (i = 0; i < S2MPS11_CLKS_NUM; i++) {
>  		/* Skip clocks not present on S2MPS14 */
> -		if (!clks_init[i].name)
> +		if (i == S2MPS11_CLK_CP && hwid == S2MPS14X)
>  			continue;
>  		clk_table[i] = s2mps11_clks[i].clk;
>  	}
> 

Re: [PATCH 2/2] clk: s2mps11: allocate only one structure for clock init

[Yadwinder Singh Brar]

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

Hello Andi Shyti,

On Mon, Jan 18, 2016 at 12:25 AM, Andi Shyti <andi.shyti@samsung.com> wrote:

> The driver allocates three structures for three different clock
> types. They are quite similar and in the clock init data they
> differ only by the name. Only one of these structure is used,
> while the others lie unused in the memory.
>

If you are worried about memory, they can be made __initdata by
creating a copy during probe.


>
> The clock's name, though, is not such a meaningful information
>

I think it can be meaningful in debugging.


> and by assigning the same name to the initial data we can avoid
> over allocation. The common name chosen will be s2mps11,
> coherently with the device driver name, instead of the clock
> device.
>
> Therefore, remove the structures associated to s2mps13 and
> s2mps14 and use only the one referred to s2mps11 for all kind of
> clocks.
>

IMHO, with all these modifications, it will leave driver with some extra
checks and reduced readability, perhaps will make it complex to add
support for similar clocks but with different clk_ops, if next version or
 any similar mfd chip comes up in future.


Regards,
Yadwinder


>
> Signed-off-by: Andi Shyti <andi.shyti@samsung.com>
> Suggested-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> ---
>  drivers/clk/clk-s2mps11.c | 48
> +++++++----------------------------------------
>  1 file changed, 7 insertions(+), 41 deletions(-)
>
> diff --git a/drivers/clk/clk-s2mps11.c b/drivers/clk/clk-s2mps11.c
> index ccb12f1..dce9753 100644
> --- a/drivers/clk/clk-s2mps11.c
> +++ b/drivers/clk/clk-s2mps11.c
> @@ -99,6 +99,7 @@ static struct clk_ops s2mps11_clk_ops = {
>         .recalc_rate    = s2mps11_clk_recalc_rate,
>  };
>
> +/* This ss2mps11_clks_init tructure is common to s2mps11, s2mps13 and
> s2mps14 */
>  static struct clk_init_data s2mps11_clks_init[S2MPS11_CLKS_NUM] = {
>         [S2MPS11_CLK_AP] = {
>                 .name = "s2mps11_ap",
> @@ -117,37 +118,6 @@ static struct clk_init_data
> s2mps11_clks_init[S2MPS11_CLKS_NUM] = {
>         },
>  };
>
> -static struct clk_init_data s2mps13_clks_init[S2MPS11_CLKS_NUM] = {
> -       [S2MPS11_CLK_AP] = {
> -               .name = "s2mps13_ap",
> -               .ops = &s2mps11_clk_ops,
> -               .flags = CLK_IS_ROOT,
> -       },
> -       [S2MPS11_CLK_CP] = {
> -               .name = "s2mps13_cp",
> -               .ops = &s2mps11_clk_ops,
> -               .flags = CLK_IS_ROOT,
> -       },
> -       [S2MPS11_CLK_BT] = {
> -               .name = "s2mps13_bt",
> -               .ops = &s2mps11_clk_ops,
> -               .flags = CLK_IS_ROOT,
> -       },
> -};
> -
> -static struct clk_init_data s2mps14_clks_init[S2MPS11_CLKS_NUM] = {
> -       [S2MPS11_CLK_AP] = {
> -               .name = "s2mps14_ap",
> -               .ops = &s2mps11_clk_ops,
> -               .flags = CLK_IS_ROOT,
> -       },
> -       [S2MPS11_CLK_BT] = {
> -               .name = "s2mps14_bt",
> -               .ops = &s2mps11_clk_ops,
> -               .flags = CLK_IS_ROOT,
> -       },
> -};
> -
>  static struct device_node *s2mps11_clk_parse_dt(struct platform_device
> *pdev,
>                 struct clk_init_data *clks_init)
>  {
> @@ -179,8 +149,8 @@ static int s2mps11_clk_probe(struct platform_device
> *pdev)
>         struct sec_pmic_dev *iodev = dev_get_drvdata(pdev->dev.parent);
>         struct s2mps11_clk *s2mps11_clks;
>         unsigned int s2mps11_reg;
> -       struct clk_init_data *clks_init;
>         int i, ret = 0;
> +       enum sec_device_type hwid =
> platform_get_device_id(pdev)->driver_data;
>
>         s2mps11_clks = devm_kcalloc(&pdev->dev, S2MPS11_CLKS_NUM,
>                                 sizeof(*s2mps11_clks), GFP_KERNEL);
> @@ -192,22 +162,18 @@ static int s2mps11_clk_probe(struct platform_device
> *pdev)
>         if (!clk_table)
>                 return -ENOMEM;
>
> -       switch(platform_get_device_id(pdev)->driver_data) {
> +       switch (hwid) {
>         case S2MPS11X:
>                 s2mps11_reg = S2MPS11_REG_RTC_CTRL;
> -               clks_init = s2mps11_clks_init;
>                 break;
>         case S2MPS13X:
>                 s2mps11_reg = S2MPS13_REG_RTCCTRL;
> -               clks_init = s2mps13_clks_init;
>                 break;
>         case S2MPS14X:
>                 s2mps11_reg = S2MPS14_REG_RTCCTRL;
> -               clks_init = s2mps14_clks_init;
>                 break;
>         case S5M8767X:
>                 s2mps11_reg = S5M8767_REG_CTRL1;
> -               clks_init = s2mps11_clks_init;
>                 break;
>         default:
>                 dev_err(&pdev->dev, "Invalid device type\n");
> @@ -215,15 +181,15 @@ static int s2mps11_clk_probe(struct platform_device
> *pdev)
>         }
>
>         /* Store clocks of_node in first element of s2mps11_clks array */
> -       s2mps11_clks->clk_np = s2mps11_clk_parse_dt(pdev, clks_init);
> +       s2mps11_clks->clk_np = s2mps11_clk_parse_dt(pdev,
> s2mps11_clks_init);
>         if (IS_ERR(s2mps11_clks->clk_np))
>                 return PTR_ERR(s2mps11_clks->clk_np);
>
>         for (i = 0; i < S2MPS11_CLKS_NUM; i++) {
> -               if (!clks_init[i].name)
> +               if (i == S2MPS11_CLK_CP && hwid == S2MPS14X)
>                         continue; /* Skip clocks not present in some
> devices */
>                 s2mps11_clks[i].iodev = iodev;
> -               s2mps11_clks[i].hw.init = &clks_init[i];
> +               s2mps11_clks[i].hw.init = &s2mps11_clks_init[i];
>                 s2mps11_clks[i].mask = 1 << i;
>                 s2mps11_clks[i].reg = s2mps11_reg;
>
> @@ -246,7 +212,7 @@ static int s2mps11_clk_probe(struct platform_device
> *pdev)
>
>         for (i = 0; i < S2MPS11_CLKS_NUM; i++) {
>                 /* Skip clocks not present on S2MPS14 */
> -               if (!clks_init[i].name)
> +               if (i == S2MPS11_CLK_CP && hwid == S2MPS14X)
>                         continue;
>                 clk_table[i] = s2mps11_clks[i].clk;
>         }
> --
> 2.6.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe
> linux-samsung-soc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

[-- Attachment #2: Type: text/html, Size: 8485 bytes --]

Re: [PATCH 2/2] clk: s2mps11: allocate only one structure for clock init

[Krzysztof Kozlowski]

On 19.01.2016 09:42, Yadwinder Singh Brar wrote:
> Hello Andi Shyti,
> 
> On Mon, Jan 18, 2016 at 12:25 AM, Andi Shyti <andi.shyti@samsung.com
> <mailto:andi.shyti@samsung.com>> wrote:
> 
>     The driver allocates three structures for three different clock
>     types. They are quite similar and in the clock init data they
>     differ only by the name. Only one of these structure is used,
>     while the others lie unused in the memory.
> 
> 
> If you are worried about memory, they can be made __initdata by
> creating a copy during probe.

Which would add small delay (need to allocate memory and copy the data)
during the boot.

>  
> 
> 
>     The clock's name, though, is not such a meaningful information
> 
> 
> I think it can be meaningful in debugging.

What kind of meaning? The name is still present... It is easy to find
the type of device (e.g. compatible).

>  
> 
>     and by assigning the same name to the initial data we can avoid
>     over allocation. The common name chosen will be s2mps11,
>     coherently with the device driver name, instead of the clock
>     device.
> 
>     Therefore, remove the structures associated to s2mps13 and
>     s2mps14 and use only the one referred to s2mps11 for all kind of
>     clocks.
> 
> 
> IMHO, with all these modifications, it will leave driver with some extra
> checks and reduced readability, perhaps will make it complex to add
> support for similar clocks but with different clk_ops, if next version or
>  any similar mfd chip comes up in future.

There is no evidence that some future chip will require clk_ops. All of
them are so far exactly the same. Even the newly added S2MPS15 is the same.

This really should be simplified.

Best regards,
Krzysztof


> 
> 
> Regards,
> Yadwinder
>  
> 
> 
>     Signed-off-by: Andi Shyti <andi.shyti@samsung.com
>     <mailto:andi.shyti@samsung.com>>
>     Suggested-by: Krzysztof Kozlowski <k.kozlowski@samsung.com
>     <mailto:k.kozlowski@samsung.com>>
>     ---
>      drivers/clk/clk-s2mps11.c | 48
>     +++++++----------------------------------------
>      1 file changed, 7 insertions(+), 41 deletions(-)
> 
>     diff --git a/drivers/clk/clk-s2mps11.c b/drivers/clk/clk-s2mps11.c
>     index ccb12f1..dce9753 100644
>     --- a/drivers/clk/clk-s2mps11.c
>     +++ b/drivers/clk/clk-s2mps11.c
>     @@ -99,6 +99,7 @@ static struct clk_ops s2mps11_clk_ops = {
>             .recalc_rate    = s2mps11_clk_recalc_rate,
>      };
> 
>     +/* This ss2mps11_clks_init tructure is common to s2mps11, s2mps13
>     and s2mps14 */
>      static struct clk_init_data s2mps11_clks_init[S2MPS11_CLKS_NUM] = {
>             [S2MPS11_CLK_AP] = {
>                     .name = "s2mps11_ap",
>     @@ -117,37 +118,6 @@ static struct clk_init_data
>     s2mps11_clks_init[S2MPS11_CLKS_NUM] = {
>             },
>      };
> 
>     -static struct clk_init_data s2mps13_clks_init[S2MPS11_CLKS_NUM] = {
>     -       [S2MPS11_CLK_AP] = {
>     -               .name = "s2mps13_ap",
>     -               .ops = &s2mps11_clk_ops,
>     -               .flags = CLK_IS_ROOT,
>     -       },
>     -       [S2MPS11_CLK_CP] = {
>     -               .name = "s2mps13_cp",
>     -               .ops = &s2mps11_clk_ops,
>     -               .flags = CLK_IS_ROOT,
>     -       },
>     -       [S2MPS11_CLK_BT] = {
>     -               .name = "s2mps13_bt",
>     -               .ops = &s2mps11_clk_ops,
>     -               .flags = CLK_IS_ROOT,
>     -       },
>     -};
>     -
>     -static struct clk_init_data s2mps14_clks_init[S2MPS11_CLKS_NUM] = {
>     -       [S2MPS11_CLK_AP] = {
>     -               .name = "s2mps14_ap",
>     -               .ops = &s2mps11_clk_ops,
>     -               .flags = CLK_IS_ROOT,
>     -       },
>     -       [S2MPS11_CLK_BT] = {
>     -               .name = "s2mps14_bt",
>     -               .ops = &s2mps11_clk_ops,
>     -               .flags = CLK_IS_ROOT,
>     -       },
>     -};
>     -
>      static struct device_node *s2mps11_clk_parse_dt(struct
>     platform_device *pdev,
>                     struct clk_init_data *clks_init)
>      {
>     @@ -179,8 +149,8 @@ static int s2mps11_clk_probe(struct
>     platform_device *pdev)
>             struct sec_pmic_dev *iodev = dev_get_drvdata(pdev->dev.parent);
>             struct s2mps11_clk *s2mps11_clks;
>             unsigned int s2mps11_reg;
>     -       struct clk_init_data *clks_init;
>             int i, ret = 0;
>     +       enum sec_device_type hwid =
>     platform_get_device_id(pdev)->driver_data;
> 
>             s2mps11_clks = devm_kcalloc(&pdev->dev, S2MPS11_CLKS_NUM,
>                                     sizeof(*s2mps11_clks), GFP_KERNEL);
>     @@ -192,22 +162,18 @@ static int s2mps11_clk_probe(struct
>     platform_device *pdev)
>             if (!clk_table)
>                     return -ENOMEM;
> 
>     -       switch(platform_get_device_id(pdev)->driver_data) {
>     +       switch (hwid) {
>             case S2MPS11X:
>                     s2mps11_reg = S2MPS11_REG_RTC_CTRL;
>     -               clks_init = s2mps11_clks_init;
>                     break;
>             case S2MPS13X:
>                     s2mps11_reg = S2MPS13_REG_RTCCTRL;
>     -               clks_init = s2mps13_clks_init;
>                     break;
>             case S2MPS14X:
>                     s2mps11_reg = S2MPS14_REG_RTCCTRL;
>     -               clks_init = s2mps14_clks_init;
>                     break;
>             case S5M8767X:
>                     s2mps11_reg = S5M8767_REG_CTRL1;
>     -               clks_init = s2mps11_clks_init;
>                     break;
>             default:
>                     dev_err(&pdev->dev, "Invalid device type\n");
>     @@ -215,15 +181,15 @@ static int s2mps11_clk_probe(struct
>     platform_device *pdev)
>             }
> 
>             /* Store clocks of_node in first element of s2mps11_clks
>     array */
>     -       s2mps11_clks->clk_np = s2mps11_clk_parse_dt(pdev, clks_init);
>     +       s2mps11_clks->clk_np = s2mps11_clk_parse_dt(pdev,
>     s2mps11_clks_init);
>             if (IS_ERR(s2mps11_clks->clk_np))
>                     return PTR_ERR(s2mps11_clks->clk_np);
> 
>             for (i = 0; i < S2MPS11_CLKS_NUM; i++) {
>     -               if (!clks_init[i].name)
>     +               if (i == S2MPS11_CLK_CP && hwid == S2MPS14X)
>                             continue; /* Skip clocks not present in some
>     devices */
>                     s2mps11_clks[i].iodev = iodev;
>     -               s2mps11_clks[i].hw.init = &clks_init[i];
>     +               s2mps11_clks[i].hw.init = &s2mps11_clks_init[i];
>                     s2mps11_clks[i].mask = 1 << i;
>                     s2mps11_clks[i].reg = s2mps11_reg;
> 
>     @@ -246,7 +212,7 @@ static int s2mps11_clk_probe(struct
>     platform_device *pdev)
> 
>             for (i = 0; i < S2MPS11_CLKS_NUM; i++) {
>                     /* Skip clocks not present on S2MPS14 */
>     -               if (!clks_init[i].name)
>     +               if (i == S2MPS11_CLK_CP && hwid == S2MPS14X)
>                             continue;
>                     clk_table[i] = s2mps11_clks[i].clk;
>             }
>     --
>     2.6.4
> 
>     --
>     To unsubscribe from this list: send the line "unsubscribe
>     linux-samsung-soc" in
>     the body of a message to majordomo@vger.kernel.org
>     <mailto:majordomo@vger.kernel.org>
>     More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 

Re: [PATCH 2/2] clk: s2mps11: allocate only one structure for clock init

[Yadwinder Singh Brar]

>>
>> IMHO, with all these modifications, it will leave driver with some extra
>> checks and reduced readability, perhaps will make it complex to add
>> support for similar clocks but with different clk_ops, if next version or
>>  any similar mfd chip comes up in future.
>
> There is no evidence that some future chip will require clk_ops. All of
> them are so far exactly the same. Even the newly added S2MPS15 is the same.
>

Can we guaranty about future chips?
Even we didn't expect there will be one less clock in case of s2mps14.

> This really should be simplified.
>

IMO, it is just reduction of LoC.

Regards,
Yadwinder

Re: [PATCH 2/2] clk: s2mps11: allocate only one structure for clock init

[Andi Shyti]

Hi Yadwinder,

>     The driver allocates three structures for three different clock
>     types. They are quite similar and in the clock init data they
>     differ only by the name. Only one of these structure is used,
>     while the others lie unused in the memory.
> 
> 
> If you are worried about memory, they can be made __initdata by
> creating a copy during probe.

mmmhhh... allocating in boot time as much as we want and then copy
what we need? It doesn't look that pretty to me.

>     The clock's name, though, is not such a meaningful information
> 
> 
> I think it can be meaningful in debugging.

Can you explain what's the use of the naming other than
debugging?

>     and by assigning the same name to the initial data we can avoid
>     over allocation. The common name chosen will be s2mps11,
>     coherently with the device driver name, instead of the clock
>     device.
> 
>     Therefore, remove the structures associated to s2mps13 and
>     s2mps14 and use only the one referred to s2mps11 for all kind of
>     clocks.
> 
> 
> IMHO, with all these modifications, it will leave driver with some extra
> checks and reduced readability, perhaps will make it complex to add
> support for similar clocks but with different clk_ops, if next version or
>  any similar mfd chip comes up in future.

In that case, when the new chip will come, we would need to
figure out something, but for sure I don't see it as a good idea
to leave allocated unused structures.

Thanks,
Andi

Re: [PATCH 2/2] clk: s2mps11: allocate only one structure for clock init

[Yadwinder Singh Brar]

On Mon, Jan 18, 2016 at 6:43 PM, Andi Shyti <andi.shyti@samsung.com> wrote:
> Hi Yadwinder,
>
>>     The driver allocates three structures for three different clock
>>     types. They are quite similar and in the clock init data they
>>     differ only by the name. Only one of these structure is used,
>>     while the others lie unused in the memory.
>>
>>
>> If you are worried about memory, they can be made __initdata by
>> creating a copy during probe.
>
> mmmhhh... allocating in boot time as much as we want and then copy
> what we need? It doesn't look that pretty to me.
>

I think its not a new practice and I don't see any issue with it.

>>     The clock's name, though, is not such a meaningful information
>>
>>
>> I think it can be meaningful in debugging.
>
> Can you explain what's the use of the naming other than
> debugging?
>

Isn't debugging important enough ?  :)

I had misunderstood your below statement.
Looking at code, it seems its still using different names for different clocks.

>>     and by assigning the same name to the initial data we can avoid
>>     over allocation. The common name chosen will be s2mps11,
>>     coherently with the device driver name, instead of the clock
>>     device.
>>
>>     Therefore, remove the structures associated to s2mps13 and
>>     s2mps14 and use only the one referred to s2mps11 for all kind of
>>     clocks.
>>
>>
>> IMHO, with all these modifications, it will leave driver with some extra
>> checks and reduced readability, perhaps will make it complex to add
>> support for similar clocks but with different clk_ops, if next version or
>>  any similar mfd chip comes up in future.
>
> In that case, when the new chip will come, we would need to
> figure out something,

Different structures were introduced to handle such cases and keep
driver simple and clean by keeping keep no. of if() checks as limited
as possible.

> but for sure I don't see it as a good idea
> to leave allocated unused structures.
>

Even a single unused structure isn't a good idea, in case where this driver
doesn't get probed. :)

Regards,
Yadwinder

> Thanks,
> Andi
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html