Skip to contentSkip to conversation

ARM: OMAP: SmartReflex: pass device dependent data via platform data

PATCH New Jean Pihet 3 participants 16 messages Sep 20, 2012 – Oct 3, 2012

Subsystems (5)
Arm (32-bit) Arm sub-architectures OMAP2+ support Power supply +1 more subsystems
lore
Share & tools
lore

Jean Pihet

author
#0 lore raw

cover ARM: OMAP: SmartReflex: pass device dependent data via platform data

Remove the device dependent settings (cpu_is_xxx(), IP fck etc.) from the driver code and pass them instead via the platform data.
This allows a clean separation of the driver code and the platform code, as required by the recent re-org of the platform data header files.

The patch also includes a fix of the error handling path in omap_sr_probe to correctly de-allocate resources in case of problems. $SUBJECT requires this change.

Jean Pihet (2):
ARM: OMAP: SmartReflex: fix error path in init function
ARM: OMAP: SmartReflex: pass device dependent data via platform data

 arch/arm/mach-omap2/sr_device.c   |   14 +++++++++
 drivers/power/avs/smartreflex.c   |   57 +++++++++++++++---------------------
 include/linux/power/smartreflex.h |   17 ++++++++++-
 3 files changed, 53 insertions(+), 35 deletions(-)
Signature · 1 line
1.7.7.6
author
#1 lore raw

PATCH 1/2 ARM: OMAP: SmartReflex: fix error path in init function

Fix the error handling path in omap_sr_probe to correctly de-allocate resources in case of problems.

Signed-off-by Jean Pihet <[email protected]>
---
 drivers/power/avs/smartreflex.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)
drivers/power/avs/smartreflex.c +4 -4
index 44efc6e..92f6728 100644
@@ -928,7 +928,7 @@ static int __init omap_sr_probe(struct platform_device *pdev)
 	if (!sr_info->base) {
 		dev_err(&pdev->dev, "%s: ioremap fail\n", __func__);
 		ret = -ENOMEM;
-		goto err_release_region;
+		goto err_free_name;
 	}
 
 	if (irq)

@@ -967,7 +967,7 @@ static int __init omap_sr_probe(struct platform_device *pdev)
 		dev_err(&pdev->dev, "%s: Unable to create debugfs directory\n",
 			__func__);
 		ret = PTR_ERR(sr_info->dbg_dir);
-		goto err_free_name;
+		goto err_debugfs;
 	}
 
 	(void) debugfs_create_file("autocomp", S_IRUGO | S_IWUSR,

@@ -1011,11 +1011,11 @@ static int __init omap_sr_probe(struct platform_device *pdev)
 
 err_debugfs:
 	debugfs_remove_recursive(sr_info->dbg_dir);
-err_free_name:
-	kfree(sr_info->name);
 err_iounmap:
 	list_del(&sr_info->node);
 	iounmap(sr_info->base);
+err_free_name:
+	kfree(sr_info->name);
 err_release_region:
 	release_mem_region(mem->start, resource_size(mem));
 err_free_devinfo:
Signature · 1 line
1.7.7.6
author
#2 lore raw

PATCH 2/2 ARM: OMAP: SmartReflex: pass device dependent data via platform data

In reply to Jean Pihet

Remove the device dependent settings (cpu_is_xxx(), IP fck etc.) from the driver code and pass them instead via the platform data.
This allows a clean separation of the driver code and the platform code.

Signed-off-by Jean Pihet <[email protected]>
---
 arch/arm/mach-omap2/sr_device.c   |   14 ++++++++++
 drivers/power/avs/smartreflex.c   |   51 +++++++++++++++---------------------
 include/linux/power/smartreflex.h |   17 +++++++++++-
 3 files changed, 50 insertions(+), 32 deletions(-)
arch/arm/mach-omap2/sr_device.c +14 -0
index d033a65..12d95c8 100644
@@ -118,10 +118,24 @@ static int __init sr_dev_init(struct omap_hwmod *oh, void *user)
 	}
 
 	sr_data->name = oh->name;
+	sr_data->fck_name = oh->main_clk;
 	sr_data->ip_type = oh->class->rev;
 	sr_data->senn_mod = 0x1;
 	sr_data->senp_mod = 0x1;
 
+	if (cpu_is_omap34xx() || cpu_is_omap44xx()) {
+		sr_data->err_weight = OMAP3430_SR_ERRWEIGHT;
+		sr_data->err_maxlimit = OMAP3430_SR_ERRMAXLIMIT;
+		sr_data->accum_data = OMAP3430_SR_ACCUMDATA;
+		if (!(strcmp(sr_data->name, "smartreflex_mpu_iva"))) {
+			sr_data->senn_avgweight = OMAP3430_SR1_SENNAVGWEIGHT;
+			sr_data->senp_avgweight = OMAP3430_SR1_SENPAVGWEIGHT;
+		} else {
+			sr_data->senn_avgweight = OMAP3430_SR2_SENNAVGWEIGHT;
+			sr_data->senp_avgweight = OMAP3430_SR2_SENPAVGWEIGHT;
+		}
+	}
+
 	sr_data->voltdm = voltdm_lookup(sr_dev_attr->sensor_voltdm_name);
 	if (IS_ERR(sr_data->voltdm)) {
 		pr_err("%s: Unable to get voltage domain pointer for VDD %s\n",
drivers/power/avs/smartreflex.c +21 -30

101 lines not on this page · raw message

include/linux/power/smartreflex.h +15 -2
index 3101e62..7b16c3c 100644
@@ -145,6 +145,7 @@
 
 struct omap_sr {
 	char				*name;
+	char				*fck_name;
 	struct list_head		node;
 	struct platform_device		*pdev;
 	struct omap_sr_nvalue_table	*nvalue_table;

@@ -259,9 +260,15 @@ struct omap_sr_nvalue_table {
  * struct omap_sr_data - Smartreflex platform data.
  *
  * @name:		instance name
+ * @fck_name:		IP block fck name
  * @ip_type:		Smartreflex IP type.
- * @senp_mod:		SENPENABLE value for the sr
- * @senn_mod:		SENNENABLE value for sr
+ * @senp_mod:		SENPENABLE value of the sr CONFIG register
+ * @senn_mod:		SENNENABLE value for sr CONFIG register
+ * @err_weight		ERRWEIGHT value of the sr ERRCONFIG register
+ * @err_maxlimit	ERRMAXLIMIT value of the sr ERRCONFIG register
+ * @accum_data		ACCUMDATA value of the sr CONFIG register
+ * @senn_avgweight	SENNAVGWEIGHT value of the sr AVGWEIGHT register
+ * @senp_avgweight	SENPAVGWEIGHT value of the sr AVGWEIGHT register
  * @nvalue_count:	Number of distinct nvalues in the nvalue table
  * @enable_on_init:	whether this sr module needs to enabled at
  *			boot up or not.

@@ -271,9 +278,15 @@ struct omap_sr_nvalue_table {
  */
 struct omap_sr_data {
 	const char			*name;
+	const char			*fck_name;
 	int				ip_type;
 	u32				senp_mod;
 	u32				senn_mod;
+	u32				err_weight;
+	u32				err_maxlimit;
+	u32				accum_data;
+	u32				senn_avgweight;
+	u32				senp_avgweight;
 	int				nvalue_count;
 	bool				enable_on_init;
 	struct omap_sr_nvalue_table	*nvalue_table;
Signature · 1 line
1.7.7.6
maintainer
#3 lore raw

PATCH 2/2 ARM: OMAP: SmartReflex: pass device dependent data via platform data

Hi,

Quoted 10 lines · Jean Pihet
drivers/power/avs/smartreflex.c +0 -4
@@ -131,14 +131,11 @@ static void sr_set_clk_length(struct omap_sr *sr)
 	struct clk *sys_ck;
 	u32 sys_clk_speed;
 
-	if (cpu_is_omap34xx())
-		sys_ck = clk_get(NULL, "sys_ck");
-	else
-		sys_ck = clk_get(NULL, "sys_clkin_ck");

drivers/power/avs/smartreflex.c static void sr_set_clk_length(struct omap_sr *sr) · line 141

Quoted 10 lines
drivers/power/avs/smartreflex.c +3 -2
+	sys_ck = clk_get(NULL, sr->fck_name);
 
 	if (IS_ERR(sys_ck)) {
-		dev_err(&sr->pdev->dev, "%s: unable to get sys clk\n",
-			__func__);
+		dev_err(&sr->pdev->dev, "%s: unable to get smartreflex fck %s\n",
+			__func__, sr->fck_name);
 		return;
 	}

You should be able to make this even simpler and not have to pass the clock name around at all. Just do:

syc_ck = clk_get(NULL, "fck);
...

In the driver, and add the necessary entries to the clock alias table. That way it's up to the SoC to set up the necessary clocks and the driver stays generic.

drivers/power/avs/smartreflex.c static int __devexit omap_sr_remove(struct platform_device *pdev) · line 1045

Quoted 8 lines
drivers/power/avs/smartreflex.c +1 -0
@@ -1049,6 +1039,7 @@ static int __devexit omap_sr_remove(struct platform_device *pdev)
 
 	list_del(&sr_info->node);
 	iounmap(sr_info->base);
+	kfree(sr_info->fck_name);
 	kfree(sr_info->name);
 	kfree(sr_info);
 	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);

Then there's no need for the kfree of the fck_name either.

There's an example of a similar patch done for twl-core.c as commit defa6be1 (mfd: Fix compile for twl-core.c by removing cpu_is_omap usage) in current linux next, except with smartreflex you probably don't need to do any of the platform_device_alloc trickery like twl-core.c neded to get around using the i2c numbers as names.

Regards,

Tony

author
#4 lore raw

PATCH 2/2 ARM: OMAP: SmartReflex: pass device dependent data via platform data

Hi Tony,

Quoted 27 lines · Tony Lindgren

Hi,

Quoted 10 lines · Jean Pihet
drivers/power/avs/smartreflex.c +0 -4
@@ -131,14 +131,11 @@ static void sr_set_clk_length(struct omap_sr *sr)
      struct clk *sys_ck;
      u32 sys_clk_speed;
 
-     if (cpu_is_omap34xx())
-             sys_ck = clk_get(NULL, "sys_ck");
-     else
-             sys_ck = clk_get(NULL, "sys_clkin_ck");

drivers/power/avs/smartreflex.c static void sr_set_clk_length(struct omap_sr *sr) · line 141

Quoted 10 lines
drivers/power/avs/smartreflex.c +3 -2
+     sys_ck = clk_get(NULL, sr->fck_name);
 
      if (IS_ERR(sys_ck)) {
-             dev_err(&sr->pdev->dev, "%s: unable to get sys clk\n",
-                     __func__);
+             dev_err(&sr->pdev->dev, "%s: unable to get smartreflex fck %s\n",
+                     __func__, sr->fck_name);
              return;
      }

You should be able to make this even simpler and not have to pass the clock name around at all. Just do:

Quoted 2 lines

syc_ck = clk_get(NULL, "fck);
...

The problem is that the system has multiple instances of the smartreflex module, each having its own fck. On OMAP3/4 the fck's are derived from sys_clk via muxes and latches.
The proposed code uses the fck's as defined in the .main_clk field of the hwmod entries, so that it takes the muxes and latches into account and also has a consistent clock naming.

Quoted 3 lines

In the driver, and add the necessary entries to the clock alias table. That way it's up to the SoC to set up the necessary clocks and the driver stays generic.

Got it. The only solution would be to use an unique fck for all smartreflex modules in all configurations. Is that acceptable?

Quoted 13 lines

drivers/power/avs/smartreflex.c static int __devexit omap_sr_remove(struct platform_device *pdev) · line 1045

Quoted 9 lines
drivers/power/avs/smartreflex.c +1 -0
@@ -1049,6 +1039,7 @@ static int __devexit omap_sr_remove(struct platform_device *pdev)
 
      list_del(&sr_info->node);
      iounmap(sr_info->base);
+     kfree(sr_info->fck_name);
      kfree(sr_info->name);
      kfree(sr_info);
      mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);

Then there's no need for the kfree of the fck_name either.

Quoted 5 lines

There's an example of a similar patch done for twl-core.c as commit defa6be1 (mfd: Fix compile for twl-core.c by removing cpu_is_omap usage) in current linux next, except with smartreflex you probably don't need to do any of the platform_device_alloc trickery like twl-core.c neded to get around using the i2c numbers as names.

Thanks!

Regards,
Jean

Quoted 4 lines

Regards,

Tony

maintainer
#5 lore raw

PATCH 2/2 ARM: OMAP: SmartReflex: pass device dependent data via platform data

Quoted 5 lines · Jean Pihet
Quoted 4 lines · Tony Lindgren

You should be able to make this even simpler and not have to pass the clock name around at all. Just do:

Quoted 8 lines
Quoted 2 lines

syc_ck = clk_get(NULL, "fck);
...

The problem is that the system has multiple instances of the smartreflex module, each having its own fck. On OMAP3/4 the fck's are derived from sys_clk via muxes and latches.
The proposed code uses the fck's as defined in the .main_clk field of the hwmod entries, so that it takes the muxes and latches into account and also has a consistent clock naming.

If the same system has multiple clocks, then you could have them matched by the smartreflex driver instance number.

Or if you mean different source clocks for various omaps, then you just need to set multiple aliases for those clocks.

Quoted 5 lines
Quoted 3 lines

In the driver, and add the necessary entries to the clock alias table. That way it's up to the SoC to set up the necessary clocks and the driver stays generic.

Got it. The only solution would be to use an unique fck for all smartreflex modules in all configurations. Is that acceptable?

Hmm maybe I don't follow you, but sounds like you just need to use the driver instance like we do for timers:

$ grep omap_timer arch/arm/mach-omap2/clock*data*.c
arch/arm/mach-omap2/clock44xx_data.c:   CLK("omap_timer.1",     "timer_sys_ck", &sys_clkin_ck,  CK_443X),
arch/arm/mach-omap2/clock44xx_data.c:   CLK("omap_timer.2",     "timer_sys_ck", &sys_clkin_ck,  CK_443X),
arch/arm/mach-omap2/clock44xx_data.c:   CLK("omap_timer.3",     "timer_sys_ck", &sys_clkin_ck,  CK_443X),
...

If you need multiple clocks for a driver instance, then they typically are just "fck" and "ick".

Regards,

Tony

author
#6 lore raw

PATCH 2/2 ARM: OMAP: SmartReflex: pass device dependent data via platform data

Hi Tony,

Quoted 27 lines · Tony Lindgren
Quoted 5 lines · Jean Pihet
Quoted 4 lines · Tony Lindgren

You should be able to make this even simpler and not have to pass the clock name around at all. Just do:

Quoted 8 lines
Quoted 2 lines

syc_ck = clk_get(NULL, "fck);
...

The problem is that the system has multiple instances of the smartreflex module, each having its own fck. On OMAP3/4 the fck's are derived from sys_clk via muxes and latches.
The proposed code uses the fck's as defined in the .main_clk field of the hwmod entries, so that it takes the muxes and latches into account and also has a consistent clock naming.

If the same system has multiple clocks, then you could have them matched by the smartreflex driver instance number.

Or if you mean different source clocks for various omaps, then you just need to set multiple aliases for those clocks.

Quoted 5 lines
Quoted 3 lines

In the driver, and add the necessary entries to the clock alias table. That way it's up to the SoC to set up the necessary clocks and the driver stays generic.

Got it. The only solution would be to use an unique fck for all smartreflex modules in all configurations. Is that acceptable?

Quoted 8 lines

Hmm maybe I don't follow you, but sounds like you just need to use the driver instance like we do for timers:

$ grep omap_timer arch/arm/mach-omap2/clock*data*.c
arch/arm/mach-omap2/clock44xx_data.c:   CLK("omap_timer.1",     "timer_sys_ck", &sys_clkin_ck,  CK_443X),
arch/arm/mach-omap2/clock44xx_data.c:   CLK("omap_timer.2",     "timer_sys_ck", &sys_clkin_ck,  CK_443X),
arch/arm/mach-omap2/clock44xx_data.c:   CLK("omap_timer.3",     "timer_sys_ck", &sys_clkin_ck,  CK_443X),
...

Ok.

I have a new version that implements this, re-submitting in a bit.

Quoted 7 lines

If you need multiple clocks for a driver instance, then they typically are just "fck" and "ick".

Regards,

Tony

Thanks,
Jean

author
#7 lore raw

cover ARM: OMAP: SmartReflex: pass device dependent data via platform data

In reply to Jean Pihet

Remove the device dependent settings (cpu_is_xxx(), IP fck etc.) from the driver code and pass them instead via the platform data.
This allows a clean separation of the driver code and the platform code, as required by the move of the platform header files to include/linux/platform_data.

The patch also includes a fix of the error handling path in omap_sr_probe to correctly de-allocate resources in case of problems. $SUBJECT requires this change.

Jean Pihet (2):
ARM: OMAP: SmartReflex: fix error path in init function
ARM: OMAP: SmartReflex: pass device dependent data via platform data

 arch/arm/mach-omap2/sr_device.c   |   13 ++++++++++
 drivers/power/avs/smartreflex.c   |   48 +++++++++++-------------------------
 include/linux/power/smartreflex.h |   14 +++++++++-
 3 files changed, 40 insertions(+), 35 deletions(-)
Signature · 1 line
1.7.7.6
author
#8 lore raw

PATCH 1/2 ARM: OMAP: SmartReflex: fix error path in init function

Fix the error handling path in omap_sr_probe to correctly de-allocate resources in case of problems.

Signed-off-by Jean Pihet <[email protected]>
---
 drivers/power/avs/smartreflex.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)
drivers/power/avs/smartreflex.c +4 -4
index 44efc6e..92f6728 100644
@@ -928,7 +928,7 @@ static int __init omap_sr_probe(struct platform_device *pdev)
 	if (!sr_info->base) {
 		dev_err(&pdev->dev, "%s: ioremap fail\n", __func__);
 		ret = -ENOMEM;
-		goto err_release_region;
+		goto err_free_name;
 	}
 
 	if (irq)

@@ -967,7 +967,7 @@ static int __init omap_sr_probe(struct platform_device *pdev)
 		dev_err(&pdev->dev, "%s: Unable to create debugfs directory\n",
 			__func__);
 		ret = PTR_ERR(sr_info->dbg_dir);
-		goto err_free_name;
+		goto err_debugfs;
 	}
 
 	(void) debugfs_create_file("autocomp", S_IRUGO | S_IWUSR,

@@ -1011,11 +1011,11 @@ static int __init omap_sr_probe(struct platform_device *pdev)
 
 err_debugfs:
 	debugfs_remove_recursive(sr_info->dbg_dir);
-err_free_name:
-	kfree(sr_info->name);
 err_iounmap:
 	list_del(&sr_info->node);
 	iounmap(sr_info->base);
+err_free_name:
+	kfree(sr_info->name);
 err_release_region:
 	release_mem_region(mem->start, resource_size(mem));
 err_free_devinfo:
Signature · 1 line
1.7.7.6
#9 lore raw

PATCH 1/2 ARM: OMAP: SmartReflex: fix error path in init function

Quoted 4 lines · Jean Pihet

Fix the error handling path in omap_sr_probe to correctly de-allocate resources in case of problems.

Signed-off-by Jean Pihet <[email protected]>

Thanks, queuing this one as a fix for v3.7-rc

Kevin

author
#10 lore raw

PATCH 2/2 ARM: OMAP: SmartReflex: pass device dependent data via platform data

In reply to Jean Pihet

Remove the device dependent settings (cpu_is_xxx(), IP fck etc.) from the driver code and pass them instead via the platform data.
This allows a clean separation of the driver code and the platform code, as required by the move of the platform header files to include/linux/platform_data.

Signed-off-by Jean Pihet <[email protected]>
---
 arch/arm/mach-omap2/sr_device.c   |   13 ++++++++++++
 drivers/power/avs/smartreflex.c   |   40 ++++++++++--------------------------
 include/linux/power/smartreflex.h |   14 +++++++++++-
 3 files changed, 36 insertions(+), 31 deletions(-)
arch/arm/mach-omap2/sr_device.c +13 -0
index d033a65..2885a77 100644
@@ -122,6 +122,19 @@ static int __init sr_dev_init(struct omap_hwmod *oh, void *user)
 	sr_data->senn_mod = 0x1;
 	sr_data->senp_mod = 0x1;
 
+	if (cpu_is_omap34xx() || cpu_is_omap44xx()) {
+		sr_data->err_weight = OMAP3430_SR_ERRWEIGHT;
+		sr_data->err_maxlimit = OMAP3430_SR_ERRMAXLIMIT;
+		sr_data->accum_data = OMAP3430_SR_ACCUMDATA;
+		if (!(strcmp(sr_data->name, "smartreflex_mpu"))) {
+			sr_data->senn_avgweight = OMAP3430_SR1_SENNAVGWEIGHT;
+			sr_data->senp_avgweight = OMAP3430_SR1_SENPAVGWEIGHT;
+		} else {
+			sr_data->senn_avgweight = OMAP3430_SR2_SENNAVGWEIGHT;
+			sr_data->senp_avgweight = OMAP3430_SR2_SENPAVGWEIGHT;
+		}
+	}
+
 	sr_data->voltdm = voltdm_lookup(sr_dev_attr->sensor_voltdm_name);
 	if (IS_ERR(sr_data->voltdm)) {
 		pr_err("%s: Unable to get voltage domain pointer for VDD %s\n",
drivers/power/avs/smartreflex.c +11 -29
index 92f6728..7c03c90 100644
@@ -128,17 +128,16 @@ static irqreturn_t sr_interrupt(int irq, void *data)
 
 static void sr_set_clk_length(struct omap_sr *sr)
 {
+	char fck_name[16]; /* "smartreflex.0" fits in 16 chars */
 	struct clk *sys_ck;
 	u32 sys_clk_speed;
 
-	if (cpu_is_omap34xx())
-		sys_ck = clk_get(NULL, "sys_ck");
-	else
-		sys_ck = clk_get(NULL, "sys_clkin_ck");
+	sprintf(fck_name, "smartreflex.%d", sr->srid);
+	sys_ck = clk_get(NULL, fck_name);
 
 	if (IS_ERR(sys_ck)) {
-		dev_err(&sr->pdev->dev, "%s: unable to get sys clk\n",
-			__func__);
+		dev_err(&sr->pdev->dev, "%s: unable to get smartreflex fck %s\n",
+			__func__, fck_name);
 		return;
 	}
 

@@ -168,28 +167,6 @@ static void sr_set_clk_length(struct omap_sr *sr)
 	}
 }
 
-static void sr_set_regfields(struct omap_sr *sr)
-{
-	/*
-	 * For time being these values are defined in smartreflex.h
-	 * and populated during init. May be they can be moved to board
-	 * file or pmic specific data structure. In that case these structure
-	 * fields will have to be populated using the pdata or pmic structure.
-	 */
-	if (cpu_is_omap34xx() || cpu_is_omap44xx()) {
-		sr->err_weight = OMAP3430_SR_ERRWEIGHT;
-		sr->err_maxlimit = OMAP3430_SR_ERRMAXLIMIT;
-		sr->accum_data = OMAP3430_SR_ACCUMDATA;
-		if (!(strcmp(sr->name, "smartreflex_mpu_iva"))) {
-			sr->senn_avgweight = OMAP3430_SR1_SENNAVGWEIGHT;
-			sr->senp_avgweight = OMAP3430_SR1_SENPAVGWEIGHT;
-		} else {
-			sr->senn_avgweight = OMAP3430_SR2_SENNAVGWEIGHT;
-			sr->senp_avgweight = OMAP3430_SR2_SENPAVGWEIGHT;
-		}
-	}
-}
-
 static void sr_start_vddautocomp(struct omap_sr *sr)
 {
 	if (!sr_class || !(sr_class->enable) || !(sr_class->configure)) {

@@ -922,8 +899,14 @@ static int __init omap_sr_probe(struct platform_device *pdev)
 	sr_info->nvalue_count = pdata->nvalue_count;
 	sr_info->senn_mod = pdata->senn_mod;
 	sr_info->senp_mod = pdata->senp_mod;
+	sr_info->err_weight = pdata->err_weight;
+	sr_info->err_maxlimit = pdata->err_maxlimit;
+	sr_info->accum_data = pdata->accum_data;
+	sr_info->senn_avgweight = pdata->senn_avgweight;
+	sr_info->senp_avgweight = pdata->senp_avgweight;
 	sr_info->autocomp_active = false;
 	sr_info->ip_type = pdata->ip_type;
+
 	sr_info->base = ioremap(mem->start, resource_size(mem));
 	if (!sr_info->base) {
 		dev_err(&pdev->dev, "%s: ioremap fail\n", __func__);

@@ -935,7 +918,6 @@ static int __init omap_sr_probe(struct platform_device *pdev)
 		sr_info->irq = irq->start;
 
 	sr_set_clk_length(sr_info);
-	sr_set_regfields(sr_info);
 
 	list_add(&sr_info->node, &sr_list);
include/linux/power/smartreflex.h +12 -2
index 3101e62..09d5efc 100644
@@ -260,8 +260,13 @@ struct omap_sr_nvalue_table {
  *
  * @name:		instance name
  * @ip_type:		Smartreflex IP type.
- * @senp_mod:		SENPENABLE value for the sr
- * @senn_mod:		SENNENABLE value for sr
+ * @senp_mod:		SENPENABLE value of the sr CONFIG register
+ * @senn_mod:		SENNENABLE value for sr CONFIG register
+ * @err_weight		ERRWEIGHT value of the sr ERRCONFIG register
+ * @err_maxlimit	ERRMAXLIMIT value of the sr ERRCONFIG register
+ * @accum_data		ACCUMDATA value of the sr CONFIG register
+ * @senn_avgweight	SENNAVGWEIGHT value of the sr AVGWEIGHT register
+ * @senp_avgweight	SENPAVGWEIGHT value of the sr AVGWEIGHT register
  * @nvalue_count:	Number of distinct nvalues in the nvalue table
  * @enable_on_init:	whether this sr module needs to enabled at
  *			boot up or not.

@@ -274,6 +279,11 @@ struct omap_sr_data {
 	int				ip_type;
 	u32				senp_mod;
 	u32				senn_mod;
+	u32				err_weight;
+	u32				err_maxlimit;
+	u32				accum_data;
+	u32				senn_avgweight;
+	u32				senp_avgweight;
 	int				nvalue_count;
 	bool				enable_on_init;
 	struct omap_sr_nvalue_table	*nvalue_table;
Signature · 1 line
1.7.7.6
#11 lore raw

PATCH 2/2 ARM: OMAP: SmartReflex: pass device dependent data via platform data

Hi Jean,

Quoted 8 lines · Jean Pihet

Remove the device dependent settings (cpu_is_xxx(), IP fck etc.) from the driver code and pass them instead via the platform data.
This allows a clean separation of the driver code and the platform code, as required by the move of the platform header files to include/linux/platform_data.

Signed-off-by Jean Pihet <[email protected]>

Could you make pdata change and the clock change should be two different patches? Also, your previous patch to align SR clock names should be combined with the changes made here.

Some comments on the clock change below...

Quoted 37 lines
---
 arch/arm/mach-omap2/sr_device.c   |   13 ++++++++++++
 drivers/power/avs/smartreflex.c   |   40 ++++++++++--------------------------
 include/linux/power/smartreflex.h |   14 +++++++++++-
 3 files changed, 36 insertions(+), 31 deletions(-)
arch/arm/mach-omap2/sr_device.c +13 -0
index d033a65..2885a77 100644
@@ -122,6 +122,19 @@ static int __init sr_dev_init(struct omap_hwmod *oh, void *user)
 	sr_data->senn_mod = 0x1;
 	sr_data->senp_mod = 0x1;
 
+	if (cpu_is_omap34xx() || cpu_is_omap44xx()) {
+		sr_data->err_weight = OMAP3430_SR_ERRWEIGHT;
+		sr_data->err_maxlimit = OMAP3430_SR_ERRMAXLIMIT;
+		sr_data->accum_data = OMAP3430_SR_ACCUMDATA;
+		if (!(strcmp(sr_data->name, "smartreflex_mpu"))) {
+			sr_data->senn_avgweight = OMAP3430_SR1_SENNAVGWEIGHT;
+			sr_data->senp_avgweight = OMAP3430_SR1_SENPAVGWEIGHT;
+		} else {
+			sr_data->senn_avgweight = OMAP3430_SR2_SENNAVGWEIGHT;
+			sr_data->senp_avgweight = OMAP3430_SR2_SENPAVGWEIGHT;
+		}
+	}
+
 	sr_data->voltdm = voltdm_lookup(sr_dev_attr->sensor_voltdm_name);
 	if (IS_ERR(sr_data->voltdm)) {
 		pr_err("%s: Unable to get voltage domain pointer for VDD %s\n",
drivers/power/avs/smartreflex.c +0 -0
index 92f6728..7c03c90 100644
@@ -128,17 +128,16 @@ static irqreturn_t sr_interrupt(int irq, void *data)
 
 static void sr_set_clk_length(struct omap_sr *sr)

drivers/power/avs/smartreflex.c static irqreturn_t sr_interrupt(int irq, void *data) · line 135

Quoted 10 lines
drivers/power/avs/smartreflex.c +2 -4
 {
+	char fck_name[16]; /* "smartreflex.0" fits in 16 chars */
 	struct clk *sys_ck;
 	u32 sys_clk_speed;
 
-	if (cpu_is_omap34xx())
-		sys_ck = clk_get(NULL, "sys_ck");
-	else
-		sys_ck = clk_get(NULL, "sys_clkin_ck");
+	sprintf(fck_name, "smartreflex.%d", sr->srid);

hmm, isn't this the same as dev_name(&sr->pdev.dev) ?

Combined with your earlier patch to align clock names, this should just be:

        sys_ck = clk_get(&sr->pdev.dev, "fck");

Also note that you've changed this from sys_clk to the SR functional clock, which seems to be the same clock on 34xx and 44xx, but that change should be clearly documented in the changelog.

Kevin

author
#12 lore raw

PATCH 2/2 ARM: OMAP: SmartReflex: pass device dependent data via platform data

Hi Kevin,

Quoted 61 lines · Kevin Hilman

Hi Jean,

Quoted 8 lines · Jean Pihet

Remove the device dependent settings (cpu_is_xxx(), IP fck etc.) from the driver code and pass them instead via the platform data.
This allows a clean separation of the driver code and the platform code, as required by the move of the platform header files to include/linux/platform_data.

Signed-off-by Jean Pihet <[email protected]>

Could you make pdata change and the clock change should be two different patches? Also, your previous patch to align SR clock names should be combined with the changes made here.

Some comments on the clock change below...

Quoted 42 lines
---
 arch/arm/mach-omap2/sr_device.c   |   13 ++++++++++++
 drivers/power/avs/smartreflex.c   |   40 ++++++++++--------------------------
 include/linux/power/smartreflex.h |   14 +++++++++++-
 3 files changed, 36 insertions(+), 31 deletions(-)
arch/arm/mach-omap2/sr_device.c +13 -0
index d033a65..2885a77 100644
@@ -122,6 +122,19 @@ static int __init sr_dev_init(struct omap_hwmod *oh, void *user)
      sr_data->senn_mod = 0x1;
      sr_data->senp_mod = 0x1;
 
+     if (cpu_is_omap34xx() || cpu_is_omap44xx()) {
+             sr_data->err_weight = OMAP3430_SR_ERRWEIGHT;
+             sr_data->err_maxlimit = OMAP3430_SR_ERRMAXLIMIT;
+             sr_data->accum_data = OMAP3430_SR_ACCUMDATA;
+             if (!(strcmp(sr_data->name, "smartreflex_mpu"))) {
+                     sr_data->senn_avgweight = OMAP3430_SR1_SENNAVGWEIGHT;
+                     sr_data->senp_avgweight = OMAP3430_SR1_SENPAVGWEIGHT;
+             } else {
+                     sr_data->senn_avgweight = OMAP3430_SR2_SENNAVGWEIGHT;
+                     sr_data->senp_avgweight = OMAP3430_SR2_SENPAVGWEIGHT;
+             }
+     }
+
      sr_data->voltdm = voltdm_lookup(sr_dev_attr->sensor_voltdm_name);
      if (IS_ERR(sr_data->voltdm)) {
              pr_err("%s: Unable to get voltage domain pointer for VDD %s\n",
drivers/power/avs/smartreflex.c +1 -0
index 92f6728..7c03c90 100644
@@ -128,17 +128,16 @@ static irqreturn_t sr_interrupt(int irq, void *data)
 
 static void sr_set_clk_length(struct omap_sr *sr)
 {
+     char fck_name[16]; /* "smartreflex.0" fits in 16 chars */
      struct clk *sys_ck;
      u32 sys_clk_speed;
Quoted 7 lines

drivers/power/avs/smartreflex.c static irqreturn_t sr_interrupt(int irq, void *data) · line 135

Quoted 5 lines
drivers/power/avs/smartreflex.c +1 -4
-     if (cpu_is_omap34xx())
-             sys_ck = clk_get(NULL, "sys_ck");
-     else
-             sys_ck = clk_get(NULL, "sys_clkin_ck");
+     sprintf(fck_name, "smartreflex.%d", sr->srid);

hmm, isn't this the same as dev_name(&sr->pdev.dev) ?

Yes. Atfer the previous patch "ARM: OMAP: hwmod: align the SmartReflex fck names" there is a direct mapping between the device name and the IP functional clock. Note: the mapping is based on the order of the hwmod entries and so it is important not to move them around.

Quoted 4 lines

Combined with your earlier patch to align clock names, this should just be:

        sys_ck = clk_get(&sr->pdev.dev, "fck");

Great! That works great and the code is much more elegant.

Quoted 3 lines

Also note that you've changed this from sys_clk to the SR functional clock, which seems to be the same clock on 34xx and 44xx, but that change should be clearly documented in the changelog.

Ok.

Updated patch in a bit.

Quoted 2 lines

Kevin

Thanks for reviewing,
Jean

author
#13 lore raw

PATCH 2/2 ARM: OMAP: SmartReflex: pass device dependent data via platform data

In reply to Jean Pihet

From: Jean Pihet <[email protected]>

Remove the device dependent code (ex. cpu_is_xxx()) and settings from the driver code and instead pass them via the platform data. This allows a clean separation of the driver code and the platform code, as required by the move of the platform header files to include/linux/platform_data.

Note about the smartreflex functional clocks: the smartreflex fclks are derived from sys_clk and are named "smartreflex.%d". Since the smartreflex device names and the functional clock names are identical the device driver code uses them to control the functional clocks.

Signed-off-by Jean Pihet <[email protected]>
---
 arch/arm/mach-omap2/sr_device.c   |   13 +++++++++++++
 drivers/power/avs/smartreflex.c   |   38 +++++++++----------------------------
 include/linux/power/smartreflex.h |   14 ++++++++++++--
 3 files changed, 34 insertions(+), 31 deletions(-)
arch/arm/mach-omap2/sr_device.c +13 -0
index cbeae56..06de443 100644
@@ -121,6 +121,19 @@ static int __init sr_dev_init(struct omap_hwmod *oh, void *user)
 	sr_data->senn_mod = 0x1;
 	sr_data->senp_mod = 0x1;
 
+	if (cpu_is_omap34xx() || cpu_is_omap44xx()) {
+		sr_data->err_weight = OMAP3430_SR_ERRWEIGHT;
+		sr_data->err_maxlimit = OMAP3430_SR_ERRMAXLIMIT;
+		sr_data->accum_data = OMAP3430_SR_ACCUMDATA;
+		if (!(strcmp(sr_data->name, "smartreflex_mpu"))) {
+			sr_data->senn_avgweight = OMAP3430_SR1_SENNAVGWEIGHT;
+			sr_data->senp_avgweight = OMAP3430_SR1_SENPAVGWEIGHT;
+		} else {
+			sr_data->senn_avgweight = OMAP3430_SR2_SENNAVGWEIGHT;
+			sr_data->senp_avgweight = OMAP3430_SR2_SENPAVGWEIGHT;
+		}
+	}
+
 	sr_data->voltdm = voltdm_lookup(sr_dev_attr->sensor_voltdm_name);
 	if (IS_ERR(sr_data->voltdm)) {
 		pr_err("%s: Unable to get voltage domain pointer for VDD %s\n",
drivers/power/avs/smartreflex.c +9 -29
index 24768a2..829467f 100644
@@ -133,14 +133,11 @@ static void sr_set_clk_length(struct omap_sr *sr)
 	struct clk *sys_ck;
 	u32 sys_clk_speed;
 
-	if (cpu_is_omap34xx())
-		sys_ck = clk_get(NULL, "sys_ck");
-	else
-		sys_ck = clk_get(NULL, "sys_clkin_ck");
+	sys_ck = clk_get(&sr->pdev->dev, "fck");
 
 	if (IS_ERR(sys_ck)) {
-		dev_err(&sr->pdev->dev, "%s: unable to get sys clk\n",
-			__func__);
+		dev_err(&sr->pdev->dev, "%s: unable to get smartreflex fck %s\n",
+				__func__, dev_name(&sr->pdev->dev));
 		return;
 	}
 

@@ -170,28 +167,6 @@ static void sr_set_clk_length(struct omap_sr *sr)
 	}
 }
 
-static void sr_set_regfields(struct omap_sr *sr)
-{
-	/*
-	 * For time being these values are defined in smartreflex.h
-	 * and populated during init. May be they can be moved to board
-	 * file or pmic specific data structure. In that case these structure
-	 * fields will have to be populated using the pdata or pmic structure.
-	 */
-	if (cpu_is_omap34xx() || cpu_is_omap44xx()) {
-		sr->err_weight = OMAP3430_SR_ERRWEIGHT;
-		sr->err_maxlimit = OMAP3430_SR_ERRMAXLIMIT;
-		sr->accum_data = OMAP3430_SR_ACCUMDATA;
-		if (!(strcmp(sr->name, "smartreflex_mpu_iva"))) {
-			sr->senn_avgweight = OMAP3430_SR1_SENNAVGWEIGHT;
-			sr->senp_avgweight = OMAP3430_SR1_SENPAVGWEIGHT;
-		} else {
-			sr->senn_avgweight = OMAP3430_SR2_SENNAVGWEIGHT;
-			sr->senp_avgweight = OMAP3430_SR2_SENPAVGWEIGHT;
-		}
-	}
-}
-
 static void sr_start_vddautocomp(struct omap_sr *sr)
 {
 	if (!sr_class || !(sr_class->enable) || !(sr_class->configure)) {

@@ -924,8 +899,14 @@ static int __init omap_sr_probe(struct platform_device *pdev)
 	sr_info->nvalue_count = pdata->nvalue_count;
 	sr_info->senn_mod = pdata->senn_mod;
 	sr_info->senp_mod = pdata->senp_mod;
+	sr_info->err_weight = pdata->err_weight;
+	sr_info->err_maxlimit = pdata->err_maxlimit;
+	sr_info->accum_data = pdata->accum_data;
+	sr_info->senn_avgweight = pdata->senn_avgweight;
+	sr_info->senp_avgweight = pdata->senp_avgweight;
 	sr_info->autocomp_active = false;
 	sr_info->ip_type = pdata->ip_type;
+
 	sr_info->base = ioremap(mem->start, resource_size(mem));
 	if (!sr_info->base) {
 		dev_err(&pdev->dev, "%s: ioremap fail\n", __func__);

@@ -937,7 +918,6 @@ static int __init omap_sr_probe(struct platform_device *pdev)
 		sr_info->irq = irq->start;
 
 	sr_set_clk_length(sr_info);
-	sr_set_regfields(sr_info);
 
 	list_add(&sr_info->node, &sr_list);
include/linux/power/smartreflex.h +12 -2
index 4a496eb..c0f44c2 100644
@@ -260,8 +260,13 @@ struct omap_sr_nvalue_table {
  *
  * @name:		instance name
  * @ip_type:		Smartreflex IP type.
- * @senp_mod:		SENPENABLE value for the sr
- * @senn_mod:		SENNENABLE value for sr
+ * @senp_mod:		SENPENABLE value of the sr CONFIG register
+ * @senn_mod:		SENNENABLE value for sr CONFIG register
+ * @err_weight		ERRWEIGHT value of the sr ERRCONFIG register
+ * @err_maxlimit	ERRMAXLIMIT value of the sr ERRCONFIG register
+ * @accum_data		ACCUMDATA value of the sr CONFIG register
+ * @senn_avgweight	SENNAVGWEIGHT value of the sr AVGWEIGHT register
+ * @senp_avgweight	SENPAVGWEIGHT value of the sr AVGWEIGHT register
  * @nvalue_count:	Number of distinct nvalues in the nvalue table
  * @enable_on_init:	whether this sr module needs to enabled at
  *			boot up or not.

@@ -274,6 +279,11 @@ struct omap_sr_data {
 	int				ip_type;
 	u32				senp_mod;
 	u32				senn_mod;
+	u32				err_weight;
+	u32				err_maxlimit;
+	u32				accum_data;
+	u32				senn_avgweight;
+	u32				senp_avgweight;
 	int				nvalue_count;
 	bool				enable_on_init;
 	struct omap_sr_nvalue_table	*nvalue_table;
Signature · 1 line
1.7.10.4
#14 lore raw

PATCH 2/2 ARM: OMAP: SmartReflex: pass device dependent data via platform data

Quoted 8 lines · jean.pihet at newoldbits.com

From: Jean Pihet <[email protected]>

Remove the device dependent code (ex. cpu_is_xxx()) and settings from the driver code and instead pass them via the platform data. This allows a clean separation of the driver code and the platform code, as required by the move of the platform header files to include/linux/platform_data.

Quoted 4 lines

Note about the smartreflex functional clocks: the smartreflex fclks are derived from sys_clk and are named "smartreflex.%d". Since the smartreflex device names and the functional clock names are identical the device driver code uses them to control the functional clocks.

Thanks for adding this part.

One more nit below, then please resend this patch as a combined series with the "align fclk names" patch. (note: The previous patch 1 from this series I've queued separately as a fix for v3.7-rc. )

Quoted 36 lines
Signed-off-by Jean Pihet <[email protected]>
---
 arch/arm/mach-omap2/sr_device.c   |   13 +++++++++++++
 drivers/power/avs/smartreflex.c   |   38 +++++++++----------------------------
 include/linux/power/smartreflex.h |   14 ++++++++++++--
 3 files changed, 34 insertions(+), 31 deletions(-)
arch/arm/mach-omap2/sr_device.c +13 -0
index cbeae56..06de443 100644
@@ -121,6 +121,19 @@ static int __init sr_dev_init(struct omap_hwmod *oh, void *user)
 	sr_data->senn_mod = 0x1;
 	sr_data->senp_mod = 0x1;
 
+	if (cpu_is_omap34xx() || cpu_is_omap44xx()) {
+		sr_data->err_weight = OMAP3430_SR_ERRWEIGHT;
+		sr_data->err_maxlimit = OMAP3430_SR_ERRMAXLIMIT;
+		sr_data->accum_data = OMAP3430_SR_ACCUMDATA;
+		if (!(strcmp(sr_data->name, "smartreflex_mpu"))) {
+			sr_data->senn_avgweight = OMAP3430_SR1_SENNAVGWEIGHT;
+			sr_data->senp_avgweight = OMAP3430_SR1_SENPAVGWEIGHT;
+		} else {
+			sr_data->senn_avgweight = OMAP3430_SR2_SENNAVGWEIGHT;
+			sr_data->senp_avgweight = OMAP3430_SR2_SENPAVGWEIGHT;
+		}
+	}
+
 	sr_data->voltdm = voltdm_lookup(sr_dev_attr->sensor_voltdm_name);
 	if (IS_ERR(sr_data->voltdm)) {
 		pr_err("%s: Unable to get voltage domain pointer for VDD %s\n",
drivers/power/avs/smartreflex.c +0 -0
index 24768a2..829467f 100644

drivers/power/avs/smartreflex.c static void sr_set_clk_length(struct omap_sr *sr) · line 136

Quoted 8 lines
drivers/power/avs/smartreflex.c +1 -4
@@ -133,14 +133,11 @@ static void sr_set_clk_length(struct omap_sr *sr)
 	struct clk *sys_ck;
 	u32 sys_clk_speed;
 
-	if (cpu_is_omap34xx())
-		sys_ck = clk_get(NULL, "sys_ck");
-	else
-		sys_ck = clk_get(NULL, "sys_clkin_ck");
+	sys_ck = clk_get(&sr->pdev->dev, "fck");

nit: since this isn't the sys_clk anymore, could you s/sys_ck/fck/ ?

Thanks,

Kevin

author
#15 lore raw

PATCH 2/2 ARM: OMAP: SmartReflex: pass device dependent data via platform data

Kevin,

Quoted 15 lines · Kevin Hilman
Quoted 8 lines · jean.pihet at newoldbits.com

From: Jean Pihet <[email protected]>

Remove the device dependent code (ex. cpu_is_xxx()) and settings from the driver code and instead pass them via the platform data. This allows a clean separation of the driver code and the platform code, as required by the move of the platform header files to include/linux/platform_data.

Quoted 4 lines

Note about the smartreflex functional clocks: the smartreflex fclks are derived from sys_clk and are named "smartreflex.%d". Since the smartreflex device names and the functional clock names are identical the device driver code uses them to control the functional clocks.

Quoted 4 lines

Thanks for adding this part.

One more nit below, then please resend this patch as a combined series with the "align fclk names" patch.

Just re-sent the new series.

Quoted 2 lines

(note: The previous patch 1 from this
series I've queued separately as a fix for v3.7-rc. )

Thanks! The new series is based on mainline 3.6.0 with this patch applied.

...

Quoted 8 lines
Quoted 8 lines
drivers/power/avs/smartreflex.c +0 -0
index 24768a2..829467f 100644
@@ -133,14 +133,11 @@ static void sr_set_clk_length(struct omap_sr *sr)
      struct clk *sys_ck;
      u32 sys_clk_speed;
Quoted 7 lines

drivers/power/avs/smartreflex.c static void sr_set_clk_length(struct omap_sr *sr) · line 136

Quoted 5 lines
drivers/power/avs/smartreflex.c +1 -4
-     if (cpu_is_omap34xx())
-             sys_ck = clk_get(NULL, "sys_ck");
-     else
-             sys_ck = clk_get(NULL, "sys_clkin_ck");
+     sys_ck = clk_get(&sr->pdev->dev, "fck");

nit: since this isn't the sys_clk anymore, could you s/sys_ck/fck/ ?

Done!

Quoted 5 lines

Thanks,

Kevin

Thanks,
Jean