Re: [PATCH v3 2/2] cpufreq: tegra: Re-model Tegra cpufreq driver

[bilhuang <bilhuang@nvidia.com>]

On 12/10/2013 01:32 AM, Stephen Warren wrote:
> On 12/09/2013 01:44 AM, bilhuang wrote:
>> On 12/06/2013 07:04 AM, Stephen Warren wrote:
>>> On 12/05/2013 12:44 AM, Bill Huang wrote:
>>>> Re-model Tegra cpufreq driver to support all Tegra series of SoCs.
>>>>
>>>> * Make tegra-cpufreq.c a generic Tegra cpufreq driver.
>>>> * Move Tegra20 specific codes into tegra20-cpufreq.c.
>>>> * Bind Tegra cpufreq dirver with a fake device so defer probe would work
>>>>     when we're going to get regulator in the driver to support voltage
>>>>     scaling (DVFS).
>>>
>>>> diff --git a/drivers/cpufreq/tegra-cpufreq.c
>>>> b/drivers/cpufreq/tegra-cpufreq.c
>>>
>>>> @@ -91,14 +40,10 @@ static int tegra_update_cpu_speed(struct
>>>> cpufreq_policy *policy,
>>> ...
>>>> +    if (soc_config->vote_emc_on_cpu_rate)
>>>> +        soc_config->vote_emc_on_cpu_rate(rate);
>>>> +
>>>> +    ret = soc_config->cpu_clk_set_rate(rate * 1000);
>>>>        if (ret)
>>>>            pr_err("cpu-tegra: Failed to set cpu frequency to %lu kHz\n",
>>>>                rate);
>>>
>>> Is there any/much shared code left in this file after this patch? It
>>> seems like all this file does now is make each cpufreq callback function
>>> call soc_config->the_same_function_name(). If so, wouldn't it be better
>>> to simply implement completely separate tegar20-cpufreq and
>>> tegra30-cpufreq drivers, and register them each directly with the
>>> cpufreq core, to avoid this file doing all the indirection?
>>
>> I think this file is needed since we can shared the registration and
>> probe logic for different SoCs.
>
> But there's basically nothing in probe() already, and if we have a
> separate driver for each SoC, then there's even less code; just a call
> to devm_kzalloc() for the device-specific data (which will be
> SoC-specific in size anyway), and a call to cpufreq_register_driver(). I
> don't think it's worth sharing that if it means that every other
> function needs to be an indirect function call.
OK that makes sense.
>
>>>> -int __init tegra_cpufreq_init(void)
>>>> +static struct {
>>>> +    char *compat;
>>>> +    int (*init)(struct tegra_cpufreq_data *,
>>>> +            const struct tegra_cpufreq_config **);
>>>> +} tegra_init_funcs[] = {
>>>> +    { "nvidia,tegra20", tegra20_cpufreq_init },
>>>> +};
>>>> +
>>>> +static int tegra_cpufreq_probe(struct platform_device *pdev)
>>> ...
>>>> +    for (i = 0; i < ARRAY_SIZE(tegra_init_funcs); i++) {
>>>> +        if (of_machine_is_compatible(tegra_init_funcs[i].compat)) {
>>>> +            ret = tegra_init_funcs[i].init(tegra_data, &soc_config);
>>>> +            if (!ret)
>>>> +                break;
>>>> +            else
>>>> +                goto out;
>>>> +        }
>>>>        }
>>>> +    if (i == ARRAY_SIZE(tegra_init_funcs))
>>>> +        goto out;
>>>
>>> I think there are better ways of doing this than open-coding it. Perhaps
>>> of_match_device() or the platform-driver equivalent could be made to
>>> work?
>>
>> Open coding is everywhere in OF helper functions actually. I doubt if we
>> can use of_match_device() if we're not adding node in DT.
>> If we're matching the platform device then we might need open coding, no?
>
> For platform devices, you can set up the id_table of struct
> platform_driver, and then simply call platform_get_device_id(pdev)
> inside probe() to find the matching entry. drivers/i2c/busses/i2c-at91.c
> is an example of how this works (just some random driver I found using
> grep).
If we're going to have separate driver for each SoC, then we don't need 
platform_get_device_id(pdev) stuffs...

What I would like to do is creating platform cpufreq device with name 
"${root_compatible}-cpufreq" then each SoC cpufreq driver can bind to 
it, but the question is, which file is the best place to do this? Create 
a new file for this or use existing file like arch/arm/mach-tegra/tegra.c?
>
>>>> +int __init tegra_cpufreq_init(void)
>>>> +{
>>>> +    struct platform_device_info devinfo = { .name = "tegra-cpufreq", };
>>>> +
>>>> +    platform_device_register_full(&devinfo);
>>>> +
>>>> +    return 0;
>>>>    }
>>>>    EXPORT_SYMBOL(tegra_cpufreq_init);
>>>
>>> Perhaps instead of hard-coding the name "tegra-cpufreq" here, you could
>>> dynamically construct the device name based on the DT's root compatible
>>> value, register "${root_compatible}-cpufreq", e.g.
>>> "nvidia,tegra20-cpufreq" or "nvidia,tegra30-cpufreq". That would allow
>>> the kernel's standard device/driver matching mechanism to pick the
>>> correct driver to instantiate. Perhaps you could even dynamically
>>> register an OF device so that you can use of_match_device() in probe, if
>>
>> I guess what you meant dynamically register an OF device is registering
>> an fake OF device by calling of_device_add(), no? If yes then what
>> of_node should we give?
>
> Yes. Good question about which node. I guess the root node would be the
> only one that made any sense at all, and admittedly it doesn't make a
> huge amount of sense. Perhaps registers a platform device rather than an
> OF device would make more sense. See platform_device_register() I think.
>