[PATCH] cpufreq: s3c64xx: Remove pointless NULL check in s3c64xx_cpufreq_driver_init

[PATCH] cpufreq: s3c64xx: Remove pointless NULL check in s3c64xx_cpufreq_driver_init

[Nathan Chancellor]

When building with Clang + -Wtautological-pointer-compare:

drivers/cpufreq/s3c64xx-cpufreq.c:152:6: warning: comparison of array
's3c64xx_freq_table' equal to a null pointer is always false
[-Wtautological-pointer-compare]
        if (s3c64xx_freq_table == NULL) {
            ^~~~~~~~~~~~~~~~~~    ~~~~
1 warning generated.

The definition of s3c64xx_freq_table is surrounded by an ifdef
directive for CONFIG_CPU_S3C6410, which is always true for this driver
because it depends on it in drivers/cpufreq/Kconfig.arm (and if it
weren't, there would be a build error because s3c64xx_freq_table would
not be a defined symbol).

Resolve this warning by removing the unnecessary NULL check because it
is always false as Clang notes. While we are at it, remove the
unnecessary ifdef conditional because it is always true.

Fixes: b3748ddd8056 ("[ARM] S3C64XX: Initial support for DVFS")
Link: https://github.com/ClangBuiltLinux/linux/issues/748
Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
---
 drivers/cpufreq/s3c64xx-cpufreq.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/drivers/cpufreq/s3c64xx-cpufreq.c b/drivers/cpufreq/s3c64xx-cpufreq.c
index af0c00dabb22..c6bdfc308e99 100644
--- a/drivers/cpufreq/s3c64xx-cpufreq.c
+++ b/drivers/cpufreq/s3c64xx-cpufreq.c
@@ -19,7 +19,6 @@
 static struct regulator *vddarm;
 static unsigned long regulator_latency;
 
-#ifdef CONFIG_CPU_S3C6410
 struct s3c64xx_dvfs {
 	unsigned int vddarm_min;
 	unsigned int vddarm_max;
@@ -48,7 +47,6 @@ static struct cpufreq_frequency_table s3c64xx_freq_table[] = {
 	{ 0, 4, 800000 },
 	{ 0, 0, CPUFREQ_TABLE_END },
 };
-#endif
 
 static int s3c64xx_cpufreq_set_target(struct cpufreq_policy *policy,
 				      unsigned int index)
@@ -149,11 +147,6 @@ static int s3c64xx_cpufreq_driver_init(struct cpufreq_policy *policy)
 	if (policy->cpu != 0)
 		return -EINVAL;
 
-	if (s3c64xx_freq_table == NULL) {
-		pr_err("No frequency information for this CPU\n");
-		return -ENODEV;
-	}
-
 	policy->clk = clk_get(NULL, "armclk");
 	if (IS_ERR(policy->clk)) {
 		pr_err("Unable to obtain ARMCLK: %ld\n",
-- 
2.23.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] cpufreq: s3c64xx: Remove pointless NULL check in s3c64xx_cpufreq_driver_init

[Viresh Kumar]

On 22-10-19, 17:09, Nathan Chancellor wrote:
> When building with Clang + -Wtautological-pointer-compare:
> 
> drivers/cpufreq/s3c64xx-cpufreq.c:152:6: warning: comparison of array
> 's3c64xx_freq_table' equal to a null pointer is always false
> [-Wtautological-pointer-compare]
>         if (s3c64xx_freq_table == NULL) {
>             ^~~~~~~~~~~~~~~~~~    ~~~~
> 1 warning generated.
> 
> The definition of s3c64xx_freq_table is surrounded by an ifdef
> directive for CONFIG_CPU_S3C6410, which is always true for this driver
> because it depends on it in drivers/cpufreq/Kconfig.arm (and if it
> weren't, there would be a build error because s3c64xx_freq_table would
> not be a defined symbol).
> 
> Resolve this warning by removing the unnecessary NULL check because it
> is always false as Clang notes. While we are at it, remove the
> unnecessary ifdef conditional because it is always true.
> 
> Fixes: b3748ddd8056 ("[ARM] S3C64XX: Initial support for DVFS")

+broonie, who wrote this patch to see his views on why he kept it like
this.

> Link: https://github.com/ClangBuiltLinux/linux/issues/748
> Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
> ---
>  drivers/cpufreq/s3c64xx-cpufreq.c | 7 -------
>  1 file changed, 7 deletions(-)
> 
> diff --git a/drivers/cpufreq/s3c64xx-cpufreq.c b/drivers/cpufreq/s3c64xx-cpufreq.c
> index af0c00dabb22..c6bdfc308e99 100644
> --- a/drivers/cpufreq/s3c64xx-cpufreq.c
> +++ b/drivers/cpufreq/s3c64xx-cpufreq.c
> @@ -19,7 +19,6 @@
>  static struct regulator *vddarm;
>  static unsigned long regulator_latency;
>  
> -#ifdef CONFIG_CPU_S3C6410
>  struct s3c64xx_dvfs {
>  	unsigned int vddarm_min;
>  	unsigned int vddarm_max;
> @@ -48,7 +47,6 @@ static struct cpufreq_frequency_table s3c64xx_freq_table[] = {
>  	{ 0, 4, 800000 },
>  	{ 0, 0, CPUFREQ_TABLE_END },
>  };
> -#endif
>  
>  static int s3c64xx_cpufreq_set_target(struct cpufreq_policy *policy,
>  				      unsigned int index)
> @@ -149,11 +147,6 @@ static int s3c64xx_cpufreq_driver_init(struct cpufreq_policy *policy)
>  	if (policy->cpu != 0)
>  		return -EINVAL;
>  
> -	if (s3c64xx_freq_table == NULL) {
> -		pr_err("No frequency information for this CPU\n");
> -		return -ENODEV;
> -	}
> -
>  	policy->clk = clk_get(NULL, "armclk");
>  	if (IS_ERR(policy->clk)) {
>  		pr_err("Unable to obtain ARMCLK: %ld\n",
> -- 
> 2.23.0

-- 
viresh

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] cpufreq: s3c64xx: Remove pointless NULL check in s3c64xx_cpufreq_driver_init

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 949 bytes --]

On Wed, Oct 23, 2019 at 08:53:02AM +0530, Viresh Kumar wrote:
> On 22-10-19, 17:09, Nathan Chancellor wrote:
> > When building with Clang + -Wtautological-pointer-compare:
> > 
> > drivers/cpufreq/s3c64xx-cpufreq.c:152:6: warning: comparison of array
> > 's3c64xx_freq_table' equal to a null pointer is always false
> > [-Wtautological-pointer-compare]
> >         if (s3c64xx_freq_table == NULL) {
> >             ^~~~~~~~~~~~~~~~~~    ~~~~
> > 1 warning generated.
> > 
> > The definition of s3c64xx_freq_table is surrounded by an ifdef
> > directive for CONFIG_CPU_S3C6410, which is always true for this driver
> > because it depends on it in drivers/cpufreq/Kconfig.arm (and if it
> > weren't, there would be a build error because s3c64xx_freq_table would
> > not be a defined symbol).

> +broonie, who wrote this patch to see his views on why he kept it like
> this.

The driver should also have supported s3c6400 as well.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] cpufreq: s3c64xx: Remove pointless NULL check in s3c64xx_cpufreq_driver_init

[Nathan Chancellor]

On Wed, Oct 23, 2019 at 11:43:04AM +0100, Mark Brown wrote:
> On Wed, Oct 23, 2019 at 08:53:02AM +0530, Viresh Kumar wrote:
> > On 22-10-19, 17:09, Nathan Chancellor wrote:
> > > When building with Clang + -Wtautological-pointer-compare:
> > > 
> > > drivers/cpufreq/s3c64xx-cpufreq.c:152:6: warning: comparison of array
> > > 's3c64xx_freq_table' equal to a null pointer is always false
> > > [-Wtautological-pointer-compare]
> > >         if (s3c64xx_freq_table == NULL) {
> > >             ^~~~~~~~~~~~~~~~~~    ~~~~
> > > 1 warning generated.
> > > 
> > > The definition of s3c64xx_freq_table is surrounded by an ifdef
> > > directive for CONFIG_CPU_S3C6410, which is always true for this driver
> > > because it depends on it in drivers/cpufreq/Kconfig.arm (and if it
> > > weren't, there would be a build error because s3c64xx_freq_table would
> > > not be a defined symbol).
> 
> > +broonie, who wrote this patch to see his views on why he kept it like
> > this.
> 
> The driver should also have supported s3c6400 as well.

Kconfig says otherwise, unless I am missing something.

config ARM_S3C64XX_CPUFREQ
        bool "Samsung S3C64XX"
        depends on CPU_S3C6410
        default y
        help
          This adds the CPUFreq driver for Samsung S3C6410 SoC.

          If in doubt, say N.

Cheers,
Nathan

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] cpufreq: s3c64xx: Remove pointless NULL check in s3c64xx_cpufreq_driver_init

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 531 bytes --]

On Wed, Oct 23, 2019 at 09:26:28AM -0700, Nathan Chancellor wrote:
> On Wed, Oct 23, 2019 at 11:43:04AM +0100, Mark Brown wrote:

> > The driver should also have supported s3c6400 as well.

> Kconfig says otherwise, unless I am missing something.

> config ARM_S3C64XX_CPUFREQ
>         bool "Samsung S3C64XX"
>         depends on CPU_S3C6410
>         default y
>         help
>           This adds the CPUFreq driver for Samsung S3C6410 SoC.
> 
>           If in doubt, say N.

Note the XX in the config option.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] cpufreq: s3c64xx: Remove pointless NULL check in s3c64xx_cpufreq_driver_init

[Nathan Chancellor]

On Wed, Oct 23, 2019 at 05:36:56PM +0100, Mark Brown wrote:
> On Wed, Oct 23, 2019 at 09:26:28AM -0700, Nathan Chancellor wrote:
> > On Wed, Oct 23, 2019 at 11:43:04AM +0100, Mark Brown wrote:
> 
> > > The driver should also have supported s3c6400 as well.
> 
> > Kconfig says otherwise, unless I am missing something.
> 
> > config ARM_S3C64XX_CPUFREQ
> >         bool "Samsung S3C64XX"
> >         depends on CPU_S3C6410
> >         default y
> >         help
> >           This adds the CPUFreq driver for Samsung S3C6410 SoC.
> > 
> >           If in doubt, say N.
> 
> Note the XX in the config option.

But what about the depends and the help text?

If I just enable the following config options in multi_v7_defconfig and
remove that depends, the driver will not build because the {dvfs,freq}_table
definitions only get added to the final source file when CONFIG CPU_S3C6410 is
set...

CONFIG_ARCH_MULTI_V6=y
CONFIG_ARCH_S3C64XX=y
CONFIG_MACH_SMDK6400=y

  CC      drivers/cpufreq/s3c64xx-cpufreq.o
../drivers/cpufreq/s3c64xx-cpufreq.c:61:13: error: use of undeclared identifier 's3c64xx_freq_table'
        new_freq = s3c64xx_freq_table[index].frequency;
                   ^
../drivers/cpufreq/s3c64xx-cpufreq.c:62:29: error: use of undeclared identifier 's3c64xx_freq_table'
        dvfs = &s3c64xx_dvfs_table[s3c64xx_freq_table[index].driver_data];
                                   ^
../drivers/cpufreq/s3c64xx-cpufreq.c:62:10: error: use of undeclared identifier 's3c64xx_dvfs_table'
        dvfs = &s3c64xx_dvfs_table[s3c64xx_freq_table[index].driver_data];
                ^
...
14 errors generated.

So maybe it _should_ support s3c6400 but it does not appear to, which
is why there is this clang warning that my patch is trying to address.

If I am missing something critical, please let me know.

Cheers,
Nathan

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] cpufreq: s3c64xx: Remove pointless NULL check in s3c64xx_cpufreq_driver_init

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 644 bytes --]

On Wed, Oct 23, 2019 at 09:54:17AM -0700, Nathan Chancellor wrote:
> On Wed, Oct 23, 2019 at 05:36:56PM +0100, Mark Brown wrote:
> > On Wed, Oct 23, 2019 at 09:26:28AM -0700, Nathan Chancellor wrote:
> > > On Wed, Oct 23, 2019 at 11:43:04AM +0100, Mark Brown wrote:

> > > > The driver should also have supported s3c6400 as well.

> > > Kconfig says otherwise, unless I am missing something.

> > Note the XX in the config option.

> But what about the depends and the help text?

Viresh asked why the driver was written with s3c6410 support optional.
I explained that the reason that it was written this way was to
accomodate s3c6400 support.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] cpufreq: s3c64xx: Remove pointless NULL check in s3c64xx_cpufreq_driver_init

[Nathan Chancellor]

On Wed, Oct 23, 2019 at 05:59:23PM +0100, Mark Brown wrote:
> On Wed, Oct 23, 2019 at 09:54:17AM -0700, Nathan Chancellor wrote:
> > On Wed, Oct 23, 2019 at 05:36:56PM +0100, Mark Brown wrote:
> > > On Wed, Oct 23, 2019 at 09:26:28AM -0700, Nathan Chancellor wrote:
> > > > On Wed, Oct 23, 2019 at 11:43:04AM +0100, Mark Brown wrote:
> 
> > > > > The driver should also have supported s3c6400 as well.
> 
> > > > Kconfig says otherwise, unless I am missing something.
> 
> > > Note the XX in the config option.
> 
> > But what about the depends and the help text?
> 
> Viresh asked why the driver was written with s3c6410 support optional.
> I explained that the reason that it was written this way was to
> accomodate s3c6400 support.

Ah understood, thanks for the clarification and sorry for the
misunderstanding!

Cheers,
Nathan

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] cpufreq: s3c64xx: Remove pointless NULL check in s3c64xx_cpufreq_driver_init

[Viresh Kumar]

On 22-10-19, 17:09, Nathan Chancellor wrote:
> When building with Clang + -Wtautological-pointer-compare:
> 
> drivers/cpufreq/s3c64xx-cpufreq.c:152:6: warning: comparison of array
> 's3c64xx_freq_table' equal to a null pointer is always false
> [-Wtautological-pointer-compare]
>         if (s3c64xx_freq_table == NULL) {
>             ^~~~~~~~~~~~~~~~~~    ~~~~
> 1 warning generated.
> 
> The definition of s3c64xx_freq_table is surrounded by an ifdef
> directive for CONFIG_CPU_S3C6410, which is always true for this driver
> because it depends on it in drivers/cpufreq/Kconfig.arm (and if it
> weren't, there would be a build error because s3c64xx_freq_table would
> not be a defined symbol).
> 
> Resolve this warning by removing the unnecessary NULL check because it
> is always false as Clang notes. While we are at it, remove the
> unnecessary ifdef conditional because it is always true.
> 
> Fixes: b3748ddd8056 ("[ARM] S3C64XX: Initial support for DVFS")
> Link: https://github.com/ClangBuiltLinux/linux/issues/748
> Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
> ---
>  drivers/cpufreq/s3c64xx-cpufreq.c | 7 -------
>  1 file changed, 7 deletions(-)

Applied. Thanks.

-- 
viresh

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel