Hi Kevin,
On Wed, Oct 3, 2012 at 12:21 AM, Kevin Hilman
<khilman@xxxxxxxxxxxxxxxxxxx> wrote:
> Hi Jean,
>
> Jean Pihet <jean.pihet@xxxxxxxxxxxxxx> 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 <j-pihet@xxxxxx>
>
> 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...
>
>> ---
>> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
