[PATCH] pwm: sun4i: redundant assignment to variable pval

[PATCH] pwm: sun4i: redundant assignment to variable pval

[Colin King]

From: Colin Ian King <colin.king@canonical.com>

Variable pval is being assigned a value that is never read. The
assignment is redundant and hence can be removed.

Addresses-Coverity: ("Unused value")
Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
 drivers/pwm/pwm-sun4i.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
index 6f5840a1a82d..53970d4ba695 100644
--- a/drivers/pwm/pwm-sun4i.c
+++ b/drivers/pwm/pwm-sun4i.c
@@ -156,7 +156,6 @@ static int sun4i_pwm_calculate(struct sun4i_pwm_chip *sun4i_pwm,
 	if (sun4i_pwm->data->has_prescaler_bypass) {
 		/* First, test without any prescaler when available */
 		prescaler = PWM_PRESCAL_MASK;
-		pval = 1;
 		/*
 		 * When not using any prescaler, the clock period in nanoseconds
 		 * is not an integer so round it half up instead of
-- 
2.20.1


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

Re: [PATCH] pwm: sun4i: redundant assignment to variable pval

[Uwe Kleine-König]

On Wed, Oct 02, 2019 at 11:08:44AM +0100, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
> 
> Variable pval is being assigned a value that is never read. The
> assignment is redundant and hence can be removed.
> 
> Addresses-Coverity: ("Unused value")
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
>  drivers/pwm/pwm-sun4i.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
> index 6f5840a1a82d..53970d4ba695 100644
> --- a/drivers/pwm/pwm-sun4i.c
> +++ b/drivers/pwm/pwm-sun4i.c
> @@ -156,7 +156,6 @@ static int sun4i_pwm_calculate(struct sun4i_pwm_chip *sun4i_pwm,
>  	if (sun4i_pwm->data->has_prescaler_bypass) {
>  		/* First, test without any prescaler when available */
>  		prescaler = PWM_PRESCAL_MASK;
> -		pval = 1;
>  		/*
>  		 * When not using any prescaler, the clock period in nanoseconds
>  		 * is not an integer so round it half up instead of

Looks fine, the issue exists since
deb9c462f4e539cc7f8389b9855eb7a507c78e7e.

You can even make pval a local variable for the second for loop.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

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

[PATCH] pwm: sun4i: Narrow scope of local variable

[Uwe Kleine-König]

The variable pval is only used in a single block in the function
sun4i_pwm_calculate(). So declare it in a more local scope to simplify
the function for humans and compilers.

While the diffstat for this patch is negative for this patch I still
thing the advantage of having a narrower scope is beneficial.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
Hello,

for the patch that became

	1b98ad3b3be9 ("pwm: sun4i: Drop redundant assignment to variable pval")

(and which yielded the situation that pval is only used in this single
block) I suggested to do this change. This was ignored however by both
Colin and Thierry without comment. So I suggest the change here
separately.

Best regards
Uwe

 drivers/pwm/pwm-sun4i.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
index 581d23287333..8919e6ab7577 100644
--- a/drivers/pwm/pwm-sun4i.c
+++ b/drivers/pwm/pwm-sun4i.c
@@ -149,7 +149,7 @@ static int sun4i_pwm_calculate(struct sun4i_pwm_chip *sun4i_pwm,
 			       u32 *dty, u32 *prd, unsigned int *prsclr)
 {
 	u64 clk_rate, div = 0;
-	unsigned int pval, prescaler = 0;
+	unsigned int prescaler = 0;
 
 	clk_rate = clk_get_rate(sun4i_pwm->clk);
 
@@ -170,6 +170,8 @@ static int sun4i_pwm_calculate(struct sun4i_pwm_chip *sun4i_pwm,
 	if (prescaler == 0) {
 		/* Go up from the first divider */
 		for (prescaler = 0; prescaler < PWM_PRESCAL_MASK; prescaler++) {
+			unsigned int pval;
+
 			if (!prescaler_table[prescaler])
 				continue;
 			pval = prescaler_table[prescaler];
-- 
2.24.0


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

Re: [PATCH] pwm: sun4i: Narrow scope of local variable

[walter harms]

Am 05.12.2019 08:24, schrieb Uwe Kleine-König:
> The variable pval is only used in a single block in the function
> sun4i_pwm_calculate(). So declare it in a more local scope to simplify
> the function for humans and compilers.
> 
> While the diffstat for this patch is negative for this patch I still
> thing the advantage of having a narrower scope is beneficial.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
> Hello,
> 
> for the patch that became
> 
> 	1b98ad3b3be9 ("pwm: sun4i: Drop redundant assignment to variable pval")
> 
> (and which yielded the situation that pval is only used in this single
> block) I suggested to do this change. This was ignored however by both
> Colin and Thierry without comment. So I suggest the change here
> separately.
> 
> Best regards
> Uwe
> 
>  drivers/pwm/pwm-sun4i.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
> index 581d23287333..8919e6ab7577 100644
> --- a/drivers/pwm/pwm-sun4i.c
> +++ b/drivers/pwm/pwm-sun4i.c
> @@ -149,7 +149,7 @@ static int sun4i_pwm_calculate(struct sun4i_pwm_chip *sun4i_pwm,
>  			       u32 *dty, u32 *prd, unsigned int *prsclr)
>  {
>  	u64 clk_rate, div = 0;
> -	unsigned int pval, prescaler = 0;
> +	unsigned int prescaler = 0;
>  
>  	clk_rate = clk_get_rate(sun4i_pwm->clk);
>  
> @@ -170,6 +170,8 @@ static int sun4i_pwm_calculate(struct sun4i_pwm_chip *sun4i_pwm,
>  	if (prescaler == 0) {
>  		/* Go up from the first divider */
>  		for (prescaler = 0; prescaler < PWM_PRESCAL_MASK; prescaler++) {
> +			unsigned int pval;
> +
>  			if (!prescaler_table[prescaler])
>  				continue;
>  			pval = prescaler_table[prescaler];


nit picking:
Doing the assignment first would remove the only use
of prescaler_table[prescaler].

unsigned int pval = prescaler_table[prescaler];
if ( ! pval )
  continue;

if you feel adventures you could also replace the for() for a while()
since we know that prescaler == 0.

while ( prescaler < PWM_PRESCAL_MASK )
{
unsigned int pval = prescaler_table[prescaler++];
....


jm2c,

 wh

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

Re: [PATCH] pwm: sun4i: Narrow scope of local variable

[Uwe Kleine-König]

Hello Walter,

On Thu, Dec 05, 2019 at 09:37:55AM +0100, walter harms wrote:
> Am 05.12.2019 08:24, schrieb Uwe Kleine-König:
> > +			unsigned int pval;
> > +
> >  			if (!prescaler_table[prescaler])
> >  				continue;
> >  			pval = prescaler_table[prescaler];
> 
> 
> nit picking:
> Doing the assignment first would remove the only use
> of prescaler_table[prescaler].

nit picking: it would be reduced to a single use?!

> unsigned int pval = prescaler_table[prescaler];
> if ( ! pval )
>   continue;

Right, will send a v2 with that.

> if you feel adventures you could also replace the for() for a while()
> since we know that prescaler == 0.
> 
> while ( prescaler < PWM_PRESCAL_MASK )
> {
> unsigned int pval = prescaler_table[prescaler++];
> ....

That however has some side effects as prescaler is used after leaving
the loop.

Best regards
Uwe


-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

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

[PATCH v2] pwm: sun4i: Narrow scope of local variable

[Uwe Kleine-König]

The variable pval is only used in a single block in the function
sun4i_pwm_calculate(). So declare it in a more local scope to simplify
the function for humans and compilers.

While at it also simplify assignment to pval.

While the diffstat for this patch is negative for this patch I still
thing the advantage of having a narrower scope is beneficial.

In my compiler / .config setup (gcc 8.2.1, arm/imx_v6_v7_defconfig +
COMPILE_TEST + PWM_SUN4I) this change doesn't result in any binary
changes.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
Hello,

changes since (implicit) v1:

 - also simplify assignment to pval as suggested by Walter
 - verify the patch doesn't introduce binary changes

Best regards
Uwe

 drivers/pwm/pwm-sun4i.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
index 1fa2057419fb..4f77ebc8ae69 100644
--- a/drivers/pwm/pwm-sun4i.c
+++ b/drivers/pwm/pwm-sun4i.c
@@ -152,7 +152,7 @@ static int sun4i_pwm_calculate(struct sun4i_pwm_chip *sun4i_pwm,
 			       u32 *dty, u32 *prd, unsigned int *prsclr)
 {
 	u64 clk_rate, div = 0;
-	unsigned int pval, prescaler = 0;
+	unsigned int prescaler = 0;
 
 	clk_rate = clk_get_rate(sun4i_pwm->clk);
 
@@ -173,9 +173,11 @@ static int sun4i_pwm_calculate(struct sun4i_pwm_chip *sun4i_pwm,
 	if (prescaler == 0) {
 		/* Go up from the first divider */
 		for (prescaler = 0; prescaler < PWM_PRESCAL_MASK; prescaler++) {
+			unsigned int pval = prescaler_table[prescaler];
+
-			if (!prescaler_table[prescaler])
+			if (!pval)
 				continue;
-			pval = prescaler_table[prescaler];
+
 			div = clk_rate;
 			do_div(div, pval);
 			div = div * state->period;
-- 
2.24.0


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

Re: [PATCH] pwm: sun4i: redundant assignment to variable pval

[Thierry Reding]

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

On Wed, Oct 02, 2019 at 11:08:44AM +0100, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
> 
> Variable pval is being assigned a value that is never read. The
> assignment is redundant and hence can be removed.
> 
> Addresses-Coverity: ("Unused value")
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
>  drivers/pwm/pwm-sun4i.c | 1 -
>  1 file changed, 1 deletion(-)

Applied, thanks.

Thierry

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 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] pwm: sun4i: redundant assignment to variable pval

[Dan Carpenter]

On Wed, Oct 02, 2019 at 11:08:44AM +0100, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
> 
> Variable pval is being assigned a value that is never read. The
> assignment is redundant and hence can be removed.
> 
> Addresses-Coverity: ("Unused value")
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
>  drivers/pwm/pwm-sun4i.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
> index 6f5840a1a82d..53970d4ba695 100644
> --- a/drivers/pwm/pwm-sun4i.c
> +++ b/drivers/pwm/pwm-sun4i.c
> @@ -156,7 +156,6 @@ static int sun4i_pwm_calculate(struct sun4i_pwm_chip *sun4i_pwm,
>  	if (sun4i_pwm->data->has_prescaler_bypass) {
>  		/* First, test without any prescaler when available */
>  		prescaler = PWM_PRESCAL_MASK;
> -		pval = 1;
>  		/*
>  		 * When not using any prescaler, the clock period in nanoseconds
>  		 * is not an integer so round it half up instead of

Are you sure?  It looks used to me.

regards,
dan carpenter


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

Re: [PATCH] pwm: sun4i: redundant assignment to variable pval

[Colin Ian King]

On 02/10/2019 14:25, Dan Carpenter wrote:
> On Wed, Oct 02, 2019 at 11:08:44AM +0100, Colin King wrote:
>> From: Colin Ian King <colin.king@canonical.com>
>>
>> Variable pval is being assigned a value that is never read. The
>> assignment is redundant and hence can be removed.
>>
>> Addresses-Coverity: ("Unused value")
>> Signed-off-by: Colin Ian King <colin.king@canonical.com>
>> ---
>>  drivers/pwm/pwm-sun4i.c | 1 -
>>  1 file changed, 1 deletion(-)
>>
>> diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
>> index 6f5840a1a82d..53970d4ba695 100644
>> --- a/drivers/pwm/pwm-sun4i.c
>> +++ b/drivers/pwm/pwm-sun4i.c
>> @@ -156,7 +156,6 @@ static int sun4i_pwm_calculate(struct sun4i_pwm_chip *sun4i_pwm,
>>  	if (sun4i_pwm->data->has_prescaler_bypass) {
>>  		/* First, test without any prescaler when available */
>>  		prescaler = PWM_PRESCAL_MASK;
>> -		pval = 1;
>>  		/*
>>  		 * When not using any prescaler, the clock period in nanoseconds
>>  		 * is not an integer so round it half up instead of
> 
> Are you sure?  It looks used to me.

It's only read in a do_div() and before that it is being assigned:

                        pval = prescaler_table[prescaler];
                        div = clk_rate;
                        do_div(div, pval);

so the assigned value of pval = 1 is never read

Colin
> 
> regards,
> dan carpenter
> 


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

Re: [PATCH] pwm: sun4i: redundant assignment to variable pval

[Dan Carpenter]

On Wed, Oct 02, 2019 at 04:25:06PM +0300, Dan Carpenter wrote:
> On Wed, Oct 02, 2019 at 11:08:44AM +0100, Colin King wrote:
> > From: Colin Ian King <colin.king@canonical.com>
> > 
> > Variable pval is being assigned a value that is never read. The
> > assignment is redundant and hence can be removed.
> > 
> > Addresses-Coverity: ("Unused value")
> > Signed-off-by: Colin Ian King <colin.king@canonical.com>
> > ---
> >  drivers/pwm/pwm-sun4i.c | 1 -
> >  1 file changed, 1 deletion(-)
> > 
> > diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
> > index 6f5840a1a82d..53970d4ba695 100644
> > --- a/drivers/pwm/pwm-sun4i.c
> > +++ b/drivers/pwm/pwm-sun4i.c
> > @@ -156,7 +156,6 @@ static int sun4i_pwm_calculate(struct sun4i_pwm_chip *sun4i_pwm,
> >  	if (sun4i_pwm->data->has_prescaler_bypass) {
> >  		/* First, test without any prescaler when available */
> >  		prescaler = PWM_PRESCAL_MASK;
> > -		pval = 1;
> >  		/*
> >  		 * When not using any prescaler, the clock period in nanoseconds
> >  		 * is not an integer so round it half up instead of
> 
> Are you sure?  It looks used to me.

Ah.  Never mind.  My tree was out of date.

regards,
dan carpenter


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

Re: [PATCH] pwm: sun4i: redundant assignment to variable pval

[Colin Ian King]

On 02/10/2019 14:29, Dan Carpenter wrote:
> On Wed, Oct 02, 2019 at 04:25:06PM +0300, Dan Carpenter wrote:
>> On Wed, Oct 02, 2019 at 11:08:44AM +0100, Colin King wrote:
>>> From: Colin Ian King <colin.king@canonical.com>
>>>
>>> Variable pval is being assigned a value that is never read. The
>>> assignment is redundant and hence can be removed.
>>>
>>> Addresses-Coverity: ("Unused value")
>>> Signed-off-by: Colin Ian King <colin.king@canonical.com>
>>> ---
>>>  drivers/pwm/pwm-sun4i.c | 1 -
>>>  1 file changed, 1 deletion(-)
>>>
>>> diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
>>> index 6f5840a1a82d..53970d4ba695 100644
>>> --- a/drivers/pwm/pwm-sun4i.c
>>> +++ b/drivers/pwm/pwm-sun4i.c
>>> @@ -156,7 +156,6 @@ static int sun4i_pwm_calculate(struct sun4i_pwm_chip *sun4i_pwm,
>>>  	if (sun4i_pwm->data->has_prescaler_bypass) {
>>>  		/* First, test without any prescaler when available */
>>>  		prescaler = PWM_PRESCAL_MASK;
>>> -		pval = 1;
>>>  		/*
>>>  		 * When not using any prescaler, the clock period in nanoseconds
>>>  		 * is not an integer so round it half up instead of
>>
>> Are you sure?  It looks used to me.
> 
> Ah.  Never mind.  My tree was out of date.

No problem. I appreciated you eyeballing my fixes.
> 
> regards,
> dan carpenter
> 


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