lkml.org 
[lkml]   [2013]   [Dec]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v4 1/1] cpufreq: tegra: Re-model Tegra20 cpufreq driver
On 12/14/2013 07:21 AM, Stephen Warren wrote:
> On 12/12/2013 02:33 AM, Bill Huang wrote:
>> Re-model Tegra20 cpufreq driver as below.
>>
>> * Rename tegra-cpufreq.c to tegra20-cpufreq.c since this file supports
>> only Tegra20.
>> * Add probe function so defer probe can be used when we're going to
>> support DVFS.
>> * Create a fake cpufreq platform device with its name being
>> "${root_compatible}-cpufreq" so SoC cpufreq driver can bind to it
>> accordingly.
>
>> diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
>
>> +config ARM_TEGRA20_CPUFREQ
>> + bool "NVIDIA TEGRA20"
>> + depends on ARM_TEGRA_CPUFREQ && ARCH_TEGRA_2x_SOC
>> + default y
>> + help
>> + This enables Tegra20 cpufreq functionality, it adds
>> + Tegra20 CPU frequency ladder and the call back functions
>> + to set CPU rate. All the non-SoC dependant codes are
>> + controlled by the config ARM_TEGRA20_CPUFREQ.
>
> I think that last sentence is no longer true in this patch version. Or,
> did you mean to write ARM_TEGRA_CPUFREQ rather than ARM_TEGRA20_CPUFREQ?

Right, should be ARM_TEGRA_CPUFREQ, thanks for catching this.
>
>> diff --git a/drivers/cpufreq/tegra-cpufreq.c b/drivers/cpufreq/tegra-cpufreq.c
>
>> +static const char * const tegra_soc_compat[] = {
>> + "nvidia,tegra124",
>> + "nvidia,tegra114",
>> + "nvidia,tegra30",
>> + "nvidia,tegra20",
>> + NULL
>> };
>
> That table will need editing for each chip. I wonder if you can do
> something like always use the very last entry in /compatible. That would
> assume a particular ordering of the compatible entries, but they should
> be in the order $board, $soc anyway...

How do we get subset of a string and making sure it is the last? There
must be some assumptions here (though they will possibly be true) I
guess, for example, they should be in the order $board, $soc... and the
last "nvidia" should be the start of the last compatible id we would
like to get.
>
>> +int __init tegra_cpufreq_init(void)
>> + int i;
>> +
>> + for (i = 0; i < ARRAY_SIZE(tegra_soc_compat); i++) {
>> + if (of_machine_is_compatible(tegra_soc_compat[i])) {
>> + struct platform_device_info devinfo;
>> + char buf[40];
>> +
>> + memset(&devinfo, 0, sizeof(devinfo));
>> + strcpy(buf, tegra_soc_compat[i]);
>> + strcat(buf, "-cpufreq");
>
> kasprintf() might be simpler, and would avoid the arbitrary 39-character
> string limit and possibility of overflow.

Ah yeah, thanks.
>
>> + devinfo.name = buf;
>> + platform_device_register_full(&devinfo);
>
> Does the devinfo struct need to stick around, i.e. does
> platform_device_register_full keep the pointer, or take a copy of the
> struct? If it keeps the pointer, it'd be best to make devinfo a static
> global variable.

devinfo is used to provide dev info to create platform device structure,
so I think it is OK here.
>
>> diff --git a/drivers/cpufreq/tegra20-cpufreq.c b/drivers/cpufreq/tegra20-cpufreq.c
>
> Please pass the "-C" option to "git format-patch"; I assume that almost
> all the code in this file is simply cut/paste verbatim from
> tegra-cpufreq.c where it was deleted.

OK, thanks.
>
>> + * Copyright (C) 2010 Google, Inc.
>
> It's worth adding NV (c) here too.

OK.
>
>> +static int tegra20_cpufreq_remove(struct platform_device *pdev)
>> +{
>> + cpufreq_unregister_driver(&tegra20_cpufreq_driver);
>> + return 0;
>> +}
>
> That leaks all the clk_get_sys() calls. Does building this as a module
> work OK?

I should add back those clk_put here.
>
>> +MODULE_LICENSE("GPL");
>
> That should be "GPL v2".

OK.
>
>> diff --git a/include/linux/tegra-cpufreq.h b/include/linux/tegra-cpufreq.h
>
>> +#ifdef CONFIG_ARM_TEGRA_CPUFREQ
>> +int tegra_cpufreq_init(void);
>> +#else
>> +static inline int tegra_cpufreq_init(void)
>> +{ return; }
>> +#endif
>
> If you're going to wrap the { } onto one line, then I think it'd be best
> to wrap the whole thing (prototype and body) onto one line. Otherwise,
> write:
>
> {
> return;
> }
>
> Oh, and you need "return 0" not just "return".

Thanks for catching.
>



\
 
 \ /
  Last update: 2013-12-16 12:41    [W:0.081 / U:0.608 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site