Re: [PATCH v1 10/11] cpufreq: tegra20: Wrap cpufreq into platform driver

2018-05-18 Thread Dmitry Osipenko 
On 18.05.2018 12:07, Thierry Reding wrote:
> On Thu, May 17, 2018 at 09:00:55PM +0300, Dmitry Osipenko wrote:
>> Currently tegra20-cpufreq kernel module isn't getting autoloaded because
>> there is no device associated with the module, this is one of two patches
>> that resolves the module autoloading. This patch adds a module alias that
>> will associate the tegra20-cpufreq kernel module with the platform device,
>> other patch will instantiate the actual platform device. And now it makes
>> sense to wrap cpufreq driver into a platform driver for consistency.
>>
>> Signed-off-by: Dmitry Osipenko 
>> ---
>>  drivers/cpufreq/tegra20-cpufreq.c | 116 +++---
>>  1 file changed, 73 insertions(+), 43 deletions(-)
>>
>> diff --git a/drivers/cpufreq/tegra20-cpufreq.c 
>> b/drivers/cpufreq/tegra20-cpufreq.c
>> index c0a7b5a78aa6..f9d02a28df9e 100644
>> --- a/drivers/cpufreq/tegra20-cpufreq.c
>> +++ b/drivers/cpufreq/tegra20-cpufreq.c
>> @@ -19,7 +19,7 @@
>>  #include 
>>  #include 
>>  #include 
>> -#include 
>> +#include 
>>  
>>  static struct cpufreq_frequency_table freq_table[] = {
>>  { .frequency = 216000 },
>> @@ -33,15 +33,19 @@ static struct cpufreq_frequency_table freq_table[] = {
>>  { .frequency = CPUFREQ_TABLE_END },
>>  };
>>  
>> -static struct clk *cpu_clk;
>> -static struct clk *pll_x_clk;
>> -static struct clk *pll_p_clk;
>> -static bool pll_x_prepared;
>> +struct tegra20_cpufreq_data {
> 
> Nit: I'm not a big fan of _data suffixes because they are completely
> redundant. Any data structure by definition hosts data, so I'd just drop
> that.

Okay, I'll drop it in v2.

> [...]
>> @@ -152,55 +161,76 @@ static struct cpufreq_driver tegra_cpufreq_driver = {
>>  .suspend= cpufreq_generic_suspend,
>>  };
>>  
>> -static int __init tegra_cpufreq_init(void)
>> +static int tegra20_cpufreq_probe(struct platform_device *pdev)
>>  {
>> +struct tegra20_cpufreq_data *data;
>>  int err;
>>  
>> -if (!of_machine_is_compatible("nvidia,tegra20"))
>> -return -ENODEV;
>> +data = devm_kzalloc(>dev, sizeof(*data), GFP_KERNEL);
>> +if (!data)
>> +return -ENOMEM;
>>  
>> -cpu_clk = clk_get_sys(NULL, "cclk");
>> -if (IS_ERR(cpu_clk))
>> -return PTR_ERR(cpu_clk);
>> +data->cpu_clk = clk_get_sys(NULL, "cclk");
>> +if (IS_ERR(data->cpu_clk))
>> +return PTR_ERR(data->cpu_clk);
>>  
>> -pll_x_clk = clk_get_sys(NULL, "pll_x");
>> -if (IS_ERR(pll_x_clk)) {
>> -err = PTR_ERR(pll_x_clk);
>> +data->pll_x_clk = clk_get_sys(NULL, "pll_x");
>> +if (IS_ERR(data->pll_x_clk)) {
>> +err = PTR_ERR(data->pll_x_clk);
>>  goto put_cpu;
>>  }
>>  
>> -pll_p_clk = clk_get_sys(NULL, "pll_p");
>> -if (IS_ERR(pll_p_clk)) {
>> -err = PTR_ERR(pll_p_clk);
>> +data->pll_p_clk = clk_get_sys(NULL, "pll_p");
>> +if (IS_ERR(data->pll_p_clk)) {
>> +err = PTR_ERR(data->pll_p_clk);
>>  goto put_pll_x;
>>  }
>>  
>> +data->dev = >dev;
>> +
>> +tegra_cpufreq_driver.driver_data = data;
> 
> Couldn't this be embedded into struct tegra20_cpufreq_data? Moving
> everything but this into a per-device data structure seems half-baked.

That's a good suggestions, thank you.

Re: [PATCH v1 10/11] cpufreq: tegra20: Wrap cpufreq into platform driver

2018-05-18 Thread Dmitry Osipenko 
On 18.05.2018 12:07, Thierry Reding wrote:
> On Thu, May 17, 2018 at 09:00:55PM +0300, Dmitry Osipenko wrote:
>> Currently tegra20-cpufreq kernel module isn't getting autoloaded because
>> there is no device associated with the module, this is one of two patches
>> that resolves the module autoloading. This patch adds a module alias that
>> will associate the tegra20-cpufreq kernel module with the platform device,
>> other patch will instantiate the actual platform device. And now it makes
>> sense to wrap cpufreq driver into a platform driver for consistency.
>>
>> Signed-off-by: Dmitry Osipenko 
>> ---
>>  drivers/cpufreq/tegra20-cpufreq.c | 116 +++---
>>  1 file changed, 73 insertions(+), 43 deletions(-)
>>
>> diff --git a/drivers/cpufreq/tegra20-cpufreq.c 
>> b/drivers/cpufreq/tegra20-cpufreq.c
>> index c0a7b5a78aa6..f9d02a28df9e 100644
>> --- a/drivers/cpufreq/tegra20-cpufreq.c
>> +++ b/drivers/cpufreq/tegra20-cpufreq.c
>> @@ -19,7 +19,7 @@
>>  #include 
>>  #include 
>>  #include 
>> -#include 
>> +#include 
>>  
>>  static struct cpufreq_frequency_table freq_table[] = {
>>  { .frequency = 216000 },
>> @@ -33,15 +33,19 @@ static struct cpufreq_frequency_table freq_table[] = {
>>  { .frequency = CPUFREQ_TABLE_END },
>>  };
>>  
>> -static struct clk *cpu_clk;
>> -static struct clk *pll_x_clk;
>> -static struct clk *pll_p_clk;
>> -static bool pll_x_prepared;
>> +struct tegra20_cpufreq_data {
> 
> Nit: I'm not a big fan of _data suffixes because they are completely
> redundant. Any data structure by definition hosts data, so I'd just drop
> that.

Okay, I'll drop it in v2.

> [...]
>> @@ -152,55 +161,76 @@ static struct cpufreq_driver tegra_cpufreq_driver = {
>>  .suspend= cpufreq_generic_suspend,
>>  };
>>  
>> -static int __init tegra_cpufreq_init(void)
>> +static int tegra20_cpufreq_probe(struct platform_device *pdev)
>>  {
>> +struct tegra20_cpufreq_data *data;
>>  int err;
>>  
>> -if (!of_machine_is_compatible("nvidia,tegra20"))
>> -return -ENODEV;
>> +data = devm_kzalloc(>dev, sizeof(*data), GFP_KERNEL);
>> +if (!data)
>> +return -ENOMEM;
>>  
>> -cpu_clk = clk_get_sys(NULL, "cclk");
>> -if (IS_ERR(cpu_clk))
>> -return PTR_ERR(cpu_clk);
>> +data->cpu_clk = clk_get_sys(NULL, "cclk");
>> +if (IS_ERR(data->cpu_clk))
>> +return PTR_ERR(data->cpu_clk);
>>  
>> -pll_x_clk = clk_get_sys(NULL, "pll_x");
>> -if (IS_ERR(pll_x_clk)) {
>> -err = PTR_ERR(pll_x_clk);
>> +data->pll_x_clk = clk_get_sys(NULL, "pll_x");
>> +if (IS_ERR(data->pll_x_clk)) {
>> +err = PTR_ERR(data->pll_x_clk);
>>  goto put_cpu;
>>  }
>>  
>> -pll_p_clk = clk_get_sys(NULL, "pll_p");
>> -if (IS_ERR(pll_p_clk)) {
>> -err = PTR_ERR(pll_p_clk);
>> +data->pll_p_clk = clk_get_sys(NULL, "pll_p");
>> +if (IS_ERR(data->pll_p_clk)) {
>> +err = PTR_ERR(data->pll_p_clk);
>>  goto put_pll_x;
>>  }
>>  
>> +data->dev = >dev;
>> +
>> +tegra_cpufreq_driver.driver_data = data;
> 
> Couldn't this be embedded into struct tegra20_cpufreq_data? Moving
> everything but this into a per-device data structure seems half-baked.

That's a good suggestions, thank you.

Re: [PATCH v1 10/11] cpufreq: tegra20: Wrap cpufreq into platform driver

2018-05-18 Thread Thierry Reding 
On Thu, May 17, 2018 at 09:00:55PM +0300, Dmitry Osipenko wrote:
> Currently tegra20-cpufreq kernel module isn't getting autoloaded because
> there is no device associated with the module, this is one of two patches
> that resolves the module autoloading. This patch adds a module alias that
> will associate the tegra20-cpufreq kernel module with the platform device,
> other patch will instantiate the actual platform device. And now it makes
> sense to wrap cpufreq driver into a platform driver for consistency.
> 
> Signed-off-by: Dmitry Osipenko 
> ---
>  drivers/cpufreq/tegra20-cpufreq.c | 116 +++---
>  1 file changed, 73 insertions(+), 43 deletions(-)
> 
> diff --git a/drivers/cpufreq/tegra20-cpufreq.c 
> b/drivers/cpufreq/tegra20-cpufreq.c
> index c0a7b5a78aa6..f9d02a28df9e 100644
> --- a/drivers/cpufreq/tegra20-cpufreq.c
> +++ b/drivers/cpufreq/tegra20-cpufreq.c
> @@ -19,7 +19,7 @@
>  #include 
>  #include 
>  #include 
> -#include 
> +#include 
>  
>  static struct cpufreq_frequency_table freq_table[] = {
>   { .frequency = 216000 },
> @@ -33,15 +33,19 @@ static struct cpufreq_frequency_table freq_table[] = {
>   { .frequency = CPUFREQ_TABLE_END },
>  };
>  
> -static struct clk *cpu_clk;
> -static struct clk *pll_x_clk;
> -static struct clk *pll_p_clk;
> -static bool pll_x_prepared;
> +struct tegra20_cpufreq_data {

Nit: I'm not a big fan of _data suffixes because they are completely
redundant. Any data structure by definition hosts data, so I'd just drop
that.

[...]
> @@ -152,55 +161,76 @@ static struct cpufreq_driver tegra_cpufreq_driver = {
>   .suspend= cpufreq_generic_suspend,
>  };
>  
> -static int __init tegra_cpufreq_init(void)
> +static int tegra20_cpufreq_probe(struct platform_device *pdev)
>  {
> + struct tegra20_cpufreq_data *data;
>   int err;
>  
> - if (!of_machine_is_compatible("nvidia,tegra20"))
> - return -ENODEV;
> + data = devm_kzalloc(>dev, sizeof(*data), GFP_KERNEL);
> + if (!data)
> + return -ENOMEM;
>  
> - cpu_clk = clk_get_sys(NULL, "cclk");
> - if (IS_ERR(cpu_clk))
> - return PTR_ERR(cpu_clk);
> + data->cpu_clk = clk_get_sys(NULL, "cclk");
> + if (IS_ERR(data->cpu_clk))
> + return PTR_ERR(data->cpu_clk);
>  
> - pll_x_clk = clk_get_sys(NULL, "pll_x");
> - if (IS_ERR(pll_x_clk)) {
> - err = PTR_ERR(pll_x_clk);
> + data->pll_x_clk = clk_get_sys(NULL, "pll_x");
> + if (IS_ERR(data->pll_x_clk)) {
> + err = PTR_ERR(data->pll_x_clk);
>   goto put_cpu;
>   }
>  
> - pll_p_clk = clk_get_sys(NULL, "pll_p");
> - if (IS_ERR(pll_p_clk)) {
> - err = PTR_ERR(pll_p_clk);
> + data->pll_p_clk = clk_get_sys(NULL, "pll_p");
> + if (IS_ERR(data->pll_p_clk)) {
> + err = PTR_ERR(data->pll_p_clk);
>   goto put_pll_x;
>   }
>  
> + data->dev = >dev;
> +
> + tegra_cpufreq_driver.driver_data = data;

Couldn't this be embedded into struct tegra20_cpufreq_data? Moving
everything but this into a per-device data structure seems half-baked.

Thierry


signature.asc
Description: PGP signature

Re: [PATCH v1 10/11] cpufreq: tegra20: Wrap cpufreq into platform driver

2018-05-18 Thread Thierry Reding 
On Thu, May 17, 2018 at 09:00:55PM +0300, Dmitry Osipenko wrote:
> Currently tegra20-cpufreq kernel module isn't getting autoloaded because
> there is no device associated with the module, this is one of two patches
> that resolves the module autoloading. This patch adds a module alias that
> will associate the tegra20-cpufreq kernel module with the platform device,
> other patch will instantiate the actual platform device. And now it makes
> sense to wrap cpufreq driver into a platform driver for consistency.
> 
> Signed-off-by: Dmitry Osipenko 
> ---
>  drivers/cpufreq/tegra20-cpufreq.c | 116 +++---
>  1 file changed, 73 insertions(+), 43 deletions(-)
> 
> diff --git a/drivers/cpufreq/tegra20-cpufreq.c 
> b/drivers/cpufreq/tegra20-cpufreq.c
> index c0a7b5a78aa6..f9d02a28df9e 100644
> --- a/drivers/cpufreq/tegra20-cpufreq.c
> +++ b/drivers/cpufreq/tegra20-cpufreq.c
> @@ -19,7 +19,7 @@
>  #include 
>  #include 
>  #include 
> -#include 
> +#include 
>  
>  static struct cpufreq_frequency_table freq_table[] = {
>   { .frequency = 216000 },
> @@ -33,15 +33,19 @@ static struct cpufreq_frequency_table freq_table[] = {
>   { .frequency = CPUFREQ_TABLE_END },
>  };
>  
> -static struct clk *cpu_clk;
> -static struct clk *pll_x_clk;
> -static struct clk *pll_p_clk;
> -static bool pll_x_prepared;
> +struct tegra20_cpufreq_data {

Nit: I'm not a big fan of _data suffixes because they are completely
redundant. Any data structure by definition hosts data, so I'd just drop
that.

[...]
> @@ -152,55 +161,76 @@ static struct cpufreq_driver tegra_cpufreq_driver = {
>   .suspend= cpufreq_generic_suspend,
>  };
>  
> -static int __init tegra_cpufreq_init(void)
> +static int tegra20_cpufreq_probe(struct platform_device *pdev)
>  {
> + struct tegra20_cpufreq_data *data;
>   int err;
>  
> - if (!of_machine_is_compatible("nvidia,tegra20"))
> - return -ENODEV;
> + data = devm_kzalloc(>dev, sizeof(*data), GFP_KERNEL);
> + if (!data)
> + return -ENOMEM;
>  
> - cpu_clk = clk_get_sys(NULL, "cclk");
> - if (IS_ERR(cpu_clk))
> - return PTR_ERR(cpu_clk);
> + data->cpu_clk = clk_get_sys(NULL, "cclk");
> + if (IS_ERR(data->cpu_clk))
> + return PTR_ERR(data->cpu_clk);
>  
> - pll_x_clk = clk_get_sys(NULL, "pll_x");
> - if (IS_ERR(pll_x_clk)) {
> - err = PTR_ERR(pll_x_clk);
> + data->pll_x_clk = clk_get_sys(NULL, "pll_x");
> + if (IS_ERR(data->pll_x_clk)) {
> + err = PTR_ERR(data->pll_x_clk);
>   goto put_cpu;
>   }
>  
> - pll_p_clk = clk_get_sys(NULL, "pll_p");
> - if (IS_ERR(pll_p_clk)) {
> - err = PTR_ERR(pll_p_clk);
> + data->pll_p_clk = clk_get_sys(NULL, "pll_p");
> + if (IS_ERR(data->pll_p_clk)) {
> + err = PTR_ERR(data->pll_p_clk);
>   goto put_pll_x;
>   }
>  
> + data->dev = >dev;
> +
> + tegra_cpufreq_driver.driver_data = data;

Couldn't this be embedded into struct tegra20_cpufreq_data? Moving
everything but this into a per-device data structure seems half-baked.

Thierry


signature.asc
Description: PGP signature

Re: [PATCH v1 10/11] cpufreq: tegra20: Wrap cpufreq into platform driver

2018-05-18 Thread Viresh Kumar 
On 18-05-18, 11:09, Dmitry Osipenko wrote:
> On 18.05.2018 05:07, Viresh Kumar wrote:
> > On 17-05-18, 21:00, Dmitry Osipenko wrote:
> >> -static int __init tegra_cpufreq_init(void)
> >> +static int tegra20_cpufreq_probe(struct platform_device *pdev)
> >>  {
> >> +  struct tegra20_cpufreq_data *data;
> >>int err;
> >>  
> >> -  if (!of_machine_is_compatible("nvidia,tegra20"))
> >> -  return -ENODEV;
> > 
> > So this stuff wasn't really required as you are getting rid of that in
> > the same series. Should we really add it then ? Maybe ..
> > 
> 
> It's not strictly needed, but I'd prefer to keep that stuff for clarity as it
> kinda shows the way that led to the final result.

Okay.

-- 
viresh

Re: [PATCH v1 10/11] cpufreq: tegra20: Wrap cpufreq into platform driver

2018-05-18 Thread Viresh Kumar 
On 18-05-18, 11:09, Dmitry Osipenko wrote:
> On 18.05.2018 05:07, Viresh Kumar wrote:
> > On 17-05-18, 21:00, Dmitry Osipenko wrote:
> >> -static int __init tegra_cpufreq_init(void)
> >> +static int tegra20_cpufreq_probe(struct platform_device *pdev)
> >>  {
> >> +  struct tegra20_cpufreq_data *data;
> >>int err;
> >>  
> >> -  if (!of_machine_is_compatible("nvidia,tegra20"))
> >> -  return -ENODEV;
> > 
> > So this stuff wasn't really required as you are getting rid of that in
> > the same series. Should we really add it then ? Maybe ..
> > 
> 
> It's not strictly needed, but I'd prefer to keep that stuff for clarity as it
> kinda shows the way that led to the final result.

Okay.

-- 
viresh

Re: [PATCH v1 10/11] cpufreq: tegra20: Wrap cpufreq into platform driver

2018-05-18 Thread Dmitry Osipenko 
On 18.05.2018 05:07, Viresh Kumar wrote:
> On 17-05-18, 21:00, Dmitry Osipenko wrote:
>> -static int __init tegra_cpufreq_init(void)
>> +static int tegra20_cpufreq_probe(struct platform_device *pdev)
>>  {
>> +struct tegra20_cpufreq_data *data;
>>  int err;
>>  
>> -if (!of_machine_is_compatible("nvidia,tegra20"))
>> -return -ENODEV;
> 
> So this stuff wasn't really required as you are getting rid of that in
> the same series. Should we really add it then ? Maybe ..
> 

It's not strictly needed, but I'd prefer to keep that stuff for clarity as it
kinda shows the way that led to the final result.

[snip]

Re: [PATCH v1 10/11] cpufreq: tegra20: Wrap cpufreq into platform driver

2018-05-18 Thread Dmitry Osipenko 
On 18.05.2018 05:07, Viresh Kumar wrote:
> On 17-05-18, 21:00, Dmitry Osipenko wrote:
>> -static int __init tegra_cpufreq_init(void)
>> +static int tegra20_cpufreq_probe(struct platform_device *pdev)
>>  {
>> +struct tegra20_cpufreq_data *data;
>>  int err;
>>  
>> -if (!of_machine_is_compatible("nvidia,tegra20"))
>> -return -ENODEV;
> 
> So this stuff wasn't really required as you are getting rid of that in
> the same series. Should we really add it then ? Maybe ..
> 

It's not strictly needed, but I'd prefer to keep that stuff for clarity as it
kinda shows the way that led to the final result.

[snip]

Re: [PATCH v1 10/11] cpufreq: tegra20: Wrap cpufreq into platform driver

2018-05-17 Thread Viresh Kumar 
On 17-05-18, 21:00, Dmitry Osipenko wrote:
> -static int __init tegra_cpufreq_init(void)
> +static int tegra20_cpufreq_probe(struct platform_device *pdev)
>  {
> + struct tegra20_cpufreq_data *data;
>   int err;
>  
> - if (!of_machine_is_compatible("nvidia,tegra20"))
> - return -ENODEV;

So this stuff wasn't really required as you are getting rid of that in
the same series. Should we really add it then ? Maybe ..

> + data = devm_kzalloc(>dev, sizeof(*data), GFP_KERNEL);
> + if (!data)
> + return -ENOMEM;
>  
> - cpu_clk = clk_get_sys(NULL, "cclk");
> - if (IS_ERR(cpu_clk))
> - return PTR_ERR(cpu_clk);
> + data->cpu_clk = clk_get_sys(NULL, "cclk");
> + if (IS_ERR(data->cpu_clk))
> + return PTR_ERR(data->cpu_clk);
>  
> - pll_x_clk = clk_get_sys(NULL, "pll_x");
> - if (IS_ERR(pll_x_clk)) {
> - err = PTR_ERR(pll_x_clk);
> + data->pll_x_clk = clk_get_sys(NULL, "pll_x");
> + if (IS_ERR(data->pll_x_clk)) {
> + err = PTR_ERR(data->pll_x_clk);
>   goto put_cpu;
>   }
>  
> - pll_p_clk = clk_get_sys(NULL, "pll_p");
> - if (IS_ERR(pll_p_clk)) {
> - err = PTR_ERR(pll_p_clk);
> + data->pll_p_clk = clk_get_sys(NULL, "pll_p");
> + if (IS_ERR(data->pll_p_clk)) {
> + err = PTR_ERR(data->pll_p_clk);
>   goto put_pll_x;
>   }
>  
> + data->dev = >dev;
> +
> + tegra_cpufreq_driver.driver_data = data;
> +
>   err = cpufreq_register_driver(_cpufreq_driver);
>   if (err)
>   goto put_pll_p;
>  
> + platform_set_drvdata(pdev, data);
> +
>   return 0;
>  
>  put_pll_p:
> - clk_put(pll_p_clk);
> + clk_put(data->pll_p_clk);
>  put_pll_x:
> - clk_put(pll_x_clk);
> + clk_put(data->pll_x_clk);
>  put_cpu:
> - clk_put(cpu_clk);
> + clk_put(data->cpu_clk);
>  
>   return err;
>  }
>  
> -static void __exit tegra_cpufreq_exit(void)
> +static int tegra20_cpufreq_remove(struct platform_device *pdev)
>  {
> + struct tegra20_cpufreq_data *data = platform_get_drvdata(pdev);
> +
>   cpufreq_unregister_driver(_cpufreq_driver);
> - clk_put(pll_p_clk);
> - clk_put(pll_x_clk);
> - clk_put(cpu_clk);
> +
> + clk_put(data->pll_p_clk);
> + clk_put(data->pll_x_clk);
> + clk_put(data->cpu_clk);
> +
> + return 0;
>  }
>  
> +static struct platform_driver tegra20_cpufreq_driver = {
> + .probe  = tegra20_cpufreq_probe,
> + .remove = tegra20_cpufreq_remove,
> + .driver = {
> + .name   = "tegra20-cpufreq",
> + },
> +};
> +module_platform_driver(tegra20_cpufreq_driver);
> +
>  MODULE_AUTHOR("Colin Cross ");
> +MODULE_ALIAS("platform:tegra20-cpufreq");
>  MODULE_DESCRIPTION("NVIDIA Tegra20 cpufreq driver");
>  MODULE_LICENSE("GPL");
> -module_init(tegra_cpufreq_init);
> -module_exit(tegra_cpufreq_exit);

Acked-by: Viresh Kumar 

-- 
viresh

Re: [PATCH v1 10/11] cpufreq: tegra20: Wrap cpufreq into platform driver

2018-05-17 Thread Viresh Kumar 
On 17-05-18, 21:00, Dmitry Osipenko wrote:
> -static int __init tegra_cpufreq_init(void)
> +static int tegra20_cpufreq_probe(struct platform_device *pdev)
>  {
> + struct tegra20_cpufreq_data *data;
>   int err;
>  
> - if (!of_machine_is_compatible("nvidia,tegra20"))
> - return -ENODEV;

So this stuff wasn't really required as you are getting rid of that in
the same series. Should we really add it then ? Maybe ..

> + data = devm_kzalloc(>dev, sizeof(*data), GFP_KERNEL);
> + if (!data)
> + return -ENOMEM;
>  
> - cpu_clk = clk_get_sys(NULL, "cclk");
> - if (IS_ERR(cpu_clk))
> - return PTR_ERR(cpu_clk);
> + data->cpu_clk = clk_get_sys(NULL, "cclk");
> + if (IS_ERR(data->cpu_clk))
> + return PTR_ERR(data->cpu_clk);
>  
> - pll_x_clk = clk_get_sys(NULL, "pll_x");
> - if (IS_ERR(pll_x_clk)) {
> - err = PTR_ERR(pll_x_clk);
> + data->pll_x_clk = clk_get_sys(NULL, "pll_x");
> + if (IS_ERR(data->pll_x_clk)) {
> + err = PTR_ERR(data->pll_x_clk);
>   goto put_cpu;
>   }
>  
> - pll_p_clk = clk_get_sys(NULL, "pll_p");
> - if (IS_ERR(pll_p_clk)) {
> - err = PTR_ERR(pll_p_clk);
> + data->pll_p_clk = clk_get_sys(NULL, "pll_p");
> + if (IS_ERR(data->pll_p_clk)) {
> + err = PTR_ERR(data->pll_p_clk);
>   goto put_pll_x;
>   }
>  
> + data->dev = >dev;
> +
> + tegra_cpufreq_driver.driver_data = data;
> +
>   err = cpufreq_register_driver(_cpufreq_driver);
>   if (err)
>   goto put_pll_p;
>  
> + platform_set_drvdata(pdev, data);
> +
>   return 0;
>  
>  put_pll_p:
> - clk_put(pll_p_clk);
> + clk_put(data->pll_p_clk);
>  put_pll_x:
> - clk_put(pll_x_clk);
> + clk_put(data->pll_x_clk);
>  put_cpu:
> - clk_put(cpu_clk);
> + clk_put(data->cpu_clk);
>  
>   return err;
>  }
>  
> -static void __exit tegra_cpufreq_exit(void)
> +static int tegra20_cpufreq_remove(struct platform_device *pdev)
>  {
> + struct tegra20_cpufreq_data *data = platform_get_drvdata(pdev);
> +
>   cpufreq_unregister_driver(_cpufreq_driver);
> - clk_put(pll_p_clk);
> - clk_put(pll_x_clk);
> - clk_put(cpu_clk);
> +
> + clk_put(data->pll_p_clk);
> + clk_put(data->pll_x_clk);
> + clk_put(data->cpu_clk);
> +
> + return 0;
>  }
>  
> +static struct platform_driver tegra20_cpufreq_driver = {
> + .probe  = tegra20_cpufreq_probe,
> + .remove = tegra20_cpufreq_remove,
> + .driver = {
> + .name   = "tegra20-cpufreq",
> + },
> +};
> +module_platform_driver(tegra20_cpufreq_driver);
> +
>  MODULE_AUTHOR("Colin Cross ");
> +MODULE_ALIAS("platform:tegra20-cpufreq");
>  MODULE_DESCRIPTION("NVIDIA Tegra20 cpufreq driver");
>  MODULE_LICENSE("GPL");
> -module_init(tegra_cpufreq_init);
> -module_exit(tegra_cpufreq_exit);

Acked-by: Viresh Kumar 

-- 
viresh

[PATCH v1 10/11] cpufreq: tegra20: Wrap cpufreq into platform driver

2018-05-17 Thread Dmitry Osipenko 
Currently tegra20-cpufreq kernel module isn't getting autoloaded because
there is no device associated with the module, this is one of two patches
that resolves the module autoloading. This patch adds a module alias that
will associate the tegra20-cpufreq kernel module with the platform device,
other patch will instantiate the actual platform device. And now it makes
sense to wrap cpufreq driver into a platform driver for consistency.

Signed-off-by: Dmitry Osipenko 
---
 drivers/cpufreq/tegra20-cpufreq.c | 116 +++---
 1 file changed, 73 insertions(+), 43 deletions(-)

diff --git a/drivers/cpufreq/tegra20-cpufreq.c 
b/drivers/cpufreq/tegra20-cpufreq.c
index c0a7b5a78aa6..f9d02a28df9e 100644
--- a/drivers/cpufreq/tegra20-cpufreq.c
+++ b/drivers/cpufreq/tegra20-cpufreq.c
@@ -19,7 +19,7 @@
 #include 
 #include 
 #include 
-#include 
+#include 
 
 static struct cpufreq_frequency_table freq_table[] = {
{ .frequency = 216000 },
@@ -33,15 +33,19 @@ static struct cpufreq_frequency_table freq_table[] = {
{ .frequency = CPUFREQ_TABLE_END },
 };
 
-static struct clk *cpu_clk;
-static struct clk *pll_x_clk;
-static struct clk *pll_p_clk;
-static bool pll_x_prepared;
+struct tegra20_cpufreq_data {
+   struct device *dev;
+   struct clk *cpu_clk;
+   struct clk *pll_x_clk;
+   struct clk *pll_p_clk;
+   bool pll_x_prepared;
+};
 
 static unsigned int tegra_get_intermediate(struct cpufreq_policy *policy,
   unsigned int index)
 {
-   unsigned int ifreq = clk_get_rate(pll_p_clk) / 1000;
+   struct tegra20_cpufreq_data *data = cpufreq_get_driver_data();
+   unsigned int ifreq = clk_get_rate(data->pll_p_clk) / 1000;
 
/*
 * Don't switch to intermediate freq if:
@@ -57,6 +61,7 @@ static unsigned int tegra_get_intermediate(struct 
cpufreq_policy *policy,
 static int tegra_target_intermediate(struct cpufreq_policy *policy,
 unsigned int index)
 {
+   struct tegra20_cpufreq_data *data = cpufreq_get_driver_data();
int ret;
 
/*
@@ -69,21 +74,22 @@ static int tegra_target_intermediate(struct cpufreq_policy 
*policy,
 * Also, we wouldn't be using pll_x anymore and must not take extra
 * reference to it, as it can be disabled now to save some power.
 */
-   clk_prepare_enable(pll_x_clk);
+   clk_prepare_enable(data->pll_x_clk);
 
-   ret = clk_set_parent(cpu_clk, pll_p_clk);
+   ret = clk_set_parent(data->cpu_clk, data->pll_p_clk);
if (ret)
-   clk_disable_unprepare(pll_x_clk);
+   clk_disable_unprepare(data->pll_x_clk);
else
-   pll_x_prepared = true;
+   data->pll_x_prepared = true;
 
return ret;
 }
 
 static int tegra_target(struct cpufreq_policy *policy, unsigned int index)
 {
+   struct tegra20_cpufreq_data *data = cpufreq_get_driver_data();
unsigned long rate = freq_table[index].frequency;
-   unsigned int ifreq = clk_get_rate(pll_p_clk) / 1000;
+   unsigned int ifreq = clk_get_rate(data->pll_p_clk) / 1000;
int ret;
 
/*
@@ -91,14 +97,14 @@ static int tegra_target(struct cpufreq_policy *policy, 
unsigned int index)
 * as it isn't used anymore.
 */
if (rate == ifreq)
-   return clk_set_parent(cpu_clk, pll_p_clk);
+   return clk_set_parent(data->cpu_clk, data->pll_p_clk);
 
-   ret = clk_set_rate(pll_x_clk, rate * 1000);
+   ret = clk_set_rate(data->pll_x_clk, rate * 1000);
/* Restore to earlier frequency on error, i.e. pll_x */
if (ret)
-   pr_err("Failed to change pll_x to %lu\n", rate);
+   dev_err(data->dev, "Failed to change pll_x to %lu\n", rate);
 
-   ret = clk_set_parent(cpu_clk, pll_x_clk);
+   ret = clk_set_parent(data->cpu_clk, data->pll_x_clk);
/* This shouldn't fail while changing or restoring */
WARN_ON(ret);
 
@@ -106,9 +112,9 @@ static int tegra_target(struct cpufreq_policy *policy, 
unsigned int index)
 * Drop count to pll_x clock only if we switched to intermediate freq
 * earlier while transitioning to a target frequency.
 */
-   if (pll_x_prepared) {
-   clk_disable_unprepare(pll_x_clk);
-   pll_x_prepared = false;
+   if (data->pll_x_prepared) {
+   clk_disable_unprepare(data->pll_x_clk);
+   data->pll_x_prepared = false;
}
 
return ret;
@@ -116,25 +122,28 @@ static int tegra_target(struct cpufreq_policy *policy, 
unsigned int index)
 
 static int tegra_cpu_init(struct cpufreq_policy *policy)
 {
+   struct tegra20_cpufreq_data *data = cpufreq_get_driver_data();
int ret;
 
-   clk_prepare_enable(cpu_clk);
+   clk_prepare_enable(data->cpu_clk);
 
/* FIXME: what's the actual transition time? */
ret =

[PATCH v1 10/11] cpufreq: tegra20: Wrap cpufreq into platform driver

2018-05-17 Thread Dmitry Osipenko 
Currently tegra20-cpufreq kernel module isn't getting autoloaded because
there is no device associated with the module, this is one of two patches
that resolves the module autoloading. This patch adds a module alias that
will associate the tegra20-cpufreq kernel module with the platform device,
other patch will instantiate the actual platform device. And now it makes
sense to wrap cpufreq driver into a platform driver for consistency.

Signed-off-by: Dmitry Osipenko 
---
 drivers/cpufreq/tegra20-cpufreq.c | 116 +++---
 1 file changed, 73 insertions(+), 43 deletions(-)

diff --git a/drivers/cpufreq/tegra20-cpufreq.c 
b/drivers/cpufreq/tegra20-cpufreq.c
index c0a7b5a78aa6..f9d02a28df9e 100644
--- a/drivers/cpufreq/tegra20-cpufreq.c
+++ b/drivers/cpufreq/tegra20-cpufreq.c
@@ -19,7 +19,7 @@
 #include 
 #include 
 #include 
-#include 
+#include 
 
 static struct cpufreq_frequency_table freq_table[] = {
{ .frequency = 216000 },
@@ -33,15 +33,19 @@ static struct cpufreq_frequency_table freq_table[] = {
{ .frequency = CPUFREQ_TABLE_END },
 };
 
-static struct clk *cpu_clk;
-static struct clk *pll_x_clk;
-static struct clk *pll_p_clk;
-static bool pll_x_prepared;
+struct tegra20_cpufreq_data {
+   struct device *dev;
+   struct clk *cpu_clk;
+   struct clk *pll_x_clk;
+   struct clk *pll_p_clk;
+   bool pll_x_prepared;
+};
 
 static unsigned int tegra_get_intermediate(struct cpufreq_policy *policy,
   unsigned int index)
 {
-   unsigned int ifreq = clk_get_rate(pll_p_clk) / 1000;
+   struct tegra20_cpufreq_data *data = cpufreq_get_driver_data();
+   unsigned int ifreq = clk_get_rate(data->pll_p_clk) / 1000;
 
/*
 * Don't switch to intermediate freq if:
@@ -57,6 +61,7 @@ static unsigned int tegra_get_intermediate(struct 
cpufreq_policy *policy,
 static int tegra_target_intermediate(struct cpufreq_policy *policy,
 unsigned int index)
 {
+   struct tegra20_cpufreq_data *data = cpufreq_get_driver_data();
int ret;
 
/*
@@ -69,21 +74,22 @@ static int tegra_target_intermediate(struct cpufreq_policy 
*policy,
 * Also, we wouldn't be using pll_x anymore and must not take extra
 * reference to it, as it can be disabled now to save some power.
 */
-   clk_prepare_enable(pll_x_clk);
+   clk_prepare_enable(data->pll_x_clk);
 
-   ret = clk_set_parent(cpu_clk, pll_p_clk);
+   ret = clk_set_parent(data->cpu_clk, data->pll_p_clk);
if (ret)
-   clk_disable_unprepare(pll_x_clk);
+   clk_disable_unprepare(data->pll_x_clk);
else
-   pll_x_prepared = true;
+   data->pll_x_prepared = true;
 
return ret;
 }
 
 static int tegra_target(struct cpufreq_policy *policy, unsigned int index)
 {
+   struct tegra20_cpufreq_data *data = cpufreq_get_driver_data();
unsigned long rate = freq_table[index].frequency;
-   unsigned int ifreq = clk_get_rate(pll_p_clk) / 1000;
+   unsigned int ifreq = clk_get_rate(data->pll_p_clk) / 1000;
int ret;
 
/*
@@ -91,14 +97,14 @@ static int tegra_target(struct cpufreq_policy *policy, 
unsigned int index)
 * as it isn't used anymore.
 */
if (rate == ifreq)
-   return clk_set_parent(cpu_clk, pll_p_clk);
+   return clk_set_parent(data->cpu_clk, data->pll_p_clk);
 
-   ret = clk_set_rate(pll_x_clk, rate * 1000);
+   ret = clk_set_rate(data->pll_x_clk, rate * 1000);
/* Restore to earlier frequency on error, i.e. pll_x */
if (ret)
-   pr_err("Failed to change pll_x to %lu\n", rate);
+   dev_err(data->dev, "Failed to change pll_x to %lu\n", rate);
 
-   ret = clk_set_parent(cpu_clk, pll_x_clk);
+   ret = clk_set_parent(data->cpu_clk, data->pll_x_clk);
/* This shouldn't fail while changing or restoring */
WARN_ON(ret);
 
@@ -106,9 +112,9 @@ static int tegra_target(struct cpufreq_policy *policy, 
unsigned int index)
 * Drop count to pll_x clock only if we switched to intermediate freq
 * earlier while transitioning to a target frequency.
 */
-   if (pll_x_prepared) {
-   clk_disable_unprepare(pll_x_clk);
-   pll_x_prepared = false;
+   if (data->pll_x_prepared) {
+   clk_disable_unprepare(data->pll_x_clk);
+   data->pll_x_prepared = false;
}
 
return ret;
@@ -116,25 +122,28 @@ static int tegra_target(struct cpufreq_policy *policy, 
unsigned int index)
 
 static int tegra_cpu_init(struct cpufreq_policy *policy)
 {
+   struct tegra20_cpufreq_data *data = cpufreq_get_driver_data();
int ret;
 
-   clk_prepare_enable(cpu_clk);
+   clk_prepare_enable(data->cpu_clk);
 
/* FIXME: what's the actual transition time? */
ret = cpufreq_generic_init(policy,