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

Hi Jean,

Jean Pihet [off-list ref] writes:

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

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

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

Hi Kevin,

On Wed, Oct 3, 2012 at 12:21 AM, Kevin Hilman
[off-list ref] wrote:

Hi Jean,

Jean Pihet [off-list ref] writes:

quoted

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

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

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

diff --git a/arch/arm/mach-omap2/sr_device.c b/arch/arm/mach-omap2/sr_device.c
index d033a65..2885a77 100644
--- a/arch/arm/mach-omap2/sr_device.c
+++ b/arch/arm/mach-omap2/sr_device.c

@@ -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",

diff --git a/drivers/power/avs/smartreflex.c b/drivers/power/avs/smartreflex.c
index 92f6728..7c03c90 100644
--- a/drivers/power/avs/smartreflex.c
+++ b/drivers/power/avs/smartreflex.c

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

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.

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.

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.

Kevin

Thanks for reviewing,
Jean