[PATCH v3] thermal: ti-soc-thermal: Remove unused variable 'val'

[PATCH v3] thermal: ti-soc-thermal: Remove unused variable 'val'

[Lin Ruizhe]

The variable 'val'in function ti_bandgap_restore_ctxt is
the register value of read bandgap registers. This function is to
restore the context. But there is no operation on the return value
of this register, so this block is redundant. Hulk robot scans this
warning.This commit remove the dead code.

Reported-by: Hulk Robot <hulkci@huawei.com>
Fixes: b87ea759a4cc ("staging: omap-thermal: fix context restore function")
Signed-off-by: Lin Ruizhe <linruizhe@huawei.com>
---
v3:
-Add Fixes tag and more accurate commit message in this patch.
v2:
-As suggest remove the whole unuesed block in fuction
 ti_bandgap_restore_ctxt

 drivers/thermal/ti-soc-thermal/ti-bandgap.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/thermal/ti-soc-thermal/ti-bandgap.c b/drivers/thermal/ti-soc-thermal/ti-bandgap.c
index d81af89166d2..684ffb645aa9 100644
--- a/drivers/thermal/ti-soc-thermal/ti-bandgap.c
+++ b/drivers/thermal/ti-soc-thermal/ti-bandgap.c
@@ -1142,14 +1142,10 @@ static int ti_bandgap_restore_ctxt(struct ti_bandgap *bgp)
 	for (i = 0; i < bgp->conf->sensor_count; i++) {
 		struct temp_sensor_registers *tsr;
 		struct temp_sensor_regval *rval;
-		u32 val = 0;
 
 		rval = &bgp->regval[i];
 		tsr = bgp->conf->sensors[i].registers;
 
-		if (TI_BANDGAP_HAS(bgp, COUNTER))
-			val = ti_bandgap_readl(bgp, tsr->bgap_counter);
-
 		if (TI_BANDGAP_HAS(bgp, TSHUT_CONFIG))
 			ti_bandgap_writel(bgp, rval->tshut_threshold,
 					  tsr->tshut_threshold);
-- 
2.22.0

Re: [PATCH v3] thermal: ti-soc-thermal: Remove unused variable 'val'

[Greg KH]

On Wed, Apr 21, 2021 at 04:42:56PM +0800, Lin Ruizhe wrote:
> The variable 'val'in function ti_bandgap_restore_ctxt is
> the register value of read bandgap registers. This function is to
> restore the context. But there is no operation on the return value
> of this register, so this block is redundant. Hulk robot scans this
> warning.This commit remove the dead code.
> 
> Reported-by: Hulk Robot <hulkci@huawei.com>
> Fixes: b87ea759a4cc ("staging: omap-thermal: fix context restore function")
> Signed-off-by: Lin Ruizhe <linruizhe@huawei.com>
> ---
> v3:
> -Add Fixes tag and more accurate commit message in this patch.
> v2:
> -As suggest remove the whole unuesed block in fuction
>  ti_bandgap_restore_ctxt
> 
>  drivers/thermal/ti-soc-thermal/ti-bandgap.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/drivers/thermal/ti-soc-thermal/ti-bandgap.c b/drivers/thermal/ti-soc-thermal/ti-bandgap.c
> index d81af89166d2..684ffb645aa9 100644
> --- a/drivers/thermal/ti-soc-thermal/ti-bandgap.c
> +++ b/drivers/thermal/ti-soc-thermal/ti-bandgap.c
> @@ -1142,14 +1142,10 @@ static int ti_bandgap_restore_ctxt(struct ti_bandgap *bgp)
>  	for (i = 0; i < bgp->conf->sensor_count; i++) {
>  		struct temp_sensor_registers *tsr;
>  		struct temp_sensor_regval *rval;
> -		u32 val = 0;
>  
>  		rval = &bgp->regval[i];
>  		tsr = bgp->conf->sensors[i].registers;
>  
> -		if (TI_BANDGAP_HAS(bgp, COUNTER))
> -			val = ti_bandgap_readl(bgp, tsr->bgap_counter);

Are you sure that this hardware does not require this read to happen in
order for it to work properly?

Lots of hardware does need this, have you tested this?

thanks,

greg k-h

Re: [PATCH v3] thermal: ti-soc-thermal: Remove unused variable 'val'

[Daniel Lezcano]

On 21/04/2021 10:49, Greg KH wrote:
> On Wed, Apr 21, 2021 at 04:42:56PM +0800, Lin Ruizhe wrote:
>> The variable 'val'in function ti_bandgap_restore_ctxt is
>> the register value of read bandgap registers. This function is to
>> restore the context. But there is no operation on the return value
>> of this register, so this block is redundant. Hulk robot scans this
>> warning.This commit remove the dead code.
>>
>> Reported-by: Hulk Robot <hulkci@huawei.com>
>> Fixes: b87ea759a4cc ("staging: omap-thermal: fix context restore function")
>> Signed-off-by: Lin Ruizhe <linruizhe@huawei.com>
>> ---
>> v3:
>> -Add Fixes tag and more accurate commit message in this patch.
>> v2:
>> -As suggest remove the whole unuesed block in fuction
>>  ti_bandgap_restore_ctxt
>>
>>  drivers/thermal/ti-soc-thermal/ti-bandgap.c | 4 ----
>>  1 file changed, 4 deletions(-)
>>
>> diff --git a/drivers/thermal/ti-soc-thermal/ti-bandgap.c b/drivers/thermal/ti-soc-thermal/ti-bandgap.c
>> index d81af89166d2..684ffb645aa9 100644
>> --- a/drivers/thermal/ti-soc-thermal/ti-bandgap.c
>> +++ b/drivers/thermal/ti-soc-thermal/ti-bandgap.c
>> @@ -1142,14 +1142,10 @@ static int ti_bandgap_restore_ctxt(struct ti_bandgap *bgp)
>>  	for (i = 0; i < bgp->conf->sensor_count; i++) {
>>  		struct temp_sensor_registers *tsr;
>>  		struct temp_sensor_regval *rval;
>> -		u32 val = 0;
>>  
>>  		rval = &bgp->regval[i];
>>  		tsr = bgp->conf->sensors[i].registers;
>>  
>> -		if (TI_BANDGAP_HAS(bgp, COUNTER))
>> -			val = ti_bandgap_readl(bgp, tsr->bgap_counter);
> 
> Are you sure that this hardware does not require this read to happen in
> order for it to work properly?

Yes, initially we had the same concern but we were unable to find
anything specific in the history. The commit mentioned above removed the
user of the 'val' code but without removing this block of code.

When looking at the current code, it really looks like an oversight.

There is nothing in the commit's changelog referring to a need of
reading the counter register but perhaps I'm wrong because I'm not sure
to understand correctly the changelog.

> Lots of hardware does need this, have you tested this?
> 
> thanks,
> 
> greg k-h
> 


-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH v3] thermal: ti-soc-thermal: Remove unused variable 'val'

[Tony Lindgren]

* Daniel Lezcano <daniel.lezcano@linaro.org> [210421 09:07]:
> On 21/04/2021 10:49, Greg KH wrote:
> > On Wed, Apr 21, 2021 at 04:42:56PM +0800, Lin Ruizhe wrote:
> >> The variable 'val'in function ti_bandgap_restore_ctxt is
> >> the register value of read bandgap registers. This function is to
> >> restore the context. But there is no operation on the return value
> >> of this register, so this block is redundant. Hulk robot scans this
> >> warning.This commit remove the dead code.
> >>
> >> Reported-by: Hulk Robot <hulkci@huawei.com>
> >> Fixes: b87ea759a4cc ("staging: omap-thermal: fix context restore function")
> >> Signed-off-by: Lin Ruizhe <linruizhe@huawei.com>
> >> ---
> >> v3:
> >> -Add Fixes tag and more accurate commit message in this patch.
> >> v2:
> >> -As suggest remove the whole unuesed block in fuction
> >>  ti_bandgap_restore_ctxt
> >>
> >>  drivers/thermal/ti-soc-thermal/ti-bandgap.c | 4 ----
> >>  1 file changed, 4 deletions(-)
> >>
> >> diff --git a/drivers/thermal/ti-soc-thermal/ti-bandgap.c b/drivers/thermal/ti-soc-thermal/ti-bandgap.c
> >> index d81af89166d2..684ffb645aa9 100644
> >> --- a/drivers/thermal/ti-soc-thermal/ti-bandgap.c
> >> +++ b/drivers/thermal/ti-soc-thermal/ti-bandgap.c
> >> @@ -1142,14 +1142,10 @@ static int ti_bandgap_restore_ctxt(struct ti_bandgap *bgp)
> >>  	for (i = 0; i < bgp->conf->sensor_count; i++) {
> >>  		struct temp_sensor_registers *tsr;
> >>  		struct temp_sensor_regval *rval;
> >> -		u32 val = 0;
> >>  
> >>  		rval = &bgp->regval[i];
> >>  		tsr = bgp->conf->sensors[i].registers;
> >>  
> >> -		if (TI_BANDGAP_HAS(bgp, COUNTER))
> >> -			val = ti_bandgap_readl(bgp, tsr->bgap_counter);
> > 
> > Are you sure that this hardware does not require this read to happen in
> > order for it to work properly?
> 
> Yes, initially we had the same concern but we were unable to find
> anything specific in the history. The commit mentioned above removed the
> user of the 'val' code but without removing this block of code.
> 
> When looking at the current code, it really looks like an oversight.

Yes so it seems.

> There is nothing in the commit's changelog referring to a need of
> reading the counter register but perhaps I'm wrong because I'm not sure
> to understand correctly the changelog.
> 
> > Lots of hardware does need this, have you tested this?

I just tested this on omap3 logicpd torpedo devkit that can do off during
idle and reading /sys/class/thermal/thermal_zone0/temp works. So feel
free to add:

Reviewed-by: Tony Lindgren <tony@atomide.com>
Tested-by: Tony Lindgren <tony@atomide.com>

Re: [PATCH v3] thermal: ti-soc-thermal: Remove unused variable 'val'

[Daniel Lezcano]

Hi Tony,

thanks for testing

  -- Daniel

On 21/04/2021 11:37, Tony Lindgren wrote:
> * Daniel Lezcano <daniel.lezcano@linaro.org> [210421 09:07]:
>> On 21/04/2021 10:49, Greg KH wrote:
>>> On Wed, Apr 21, 2021 at 04:42:56PM +0800, Lin Ruizhe wrote:
>>>> The variable 'val'in function ti_bandgap_restore_ctxt is
>>>> the register value of read bandgap registers. This function is to
>>>> restore the context. But there is no operation on the return value
>>>> of this register, so this block is redundant. Hulk robot scans this
>>>> warning.This commit remove the dead code.
>>>>
>>>> Reported-by: Hulk Robot <hulkci@huawei.com>
>>>> Fixes: b87ea759a4cc ("staging: omap-thermal: fix context restore function")
>>>> Signed-off-by: Lin Ruizhe <linruizhe@huawei.com>
>>>> ---
>>>> v3:
>>>> -Add Fixes tag and more accurate commit message in this patch.
>>>> v2:
>>>> -As suggest remove the whole unuesed block in fuction
>>>>  ti_bandgap_restore_ctxt
>>>>
>>>>  drivers/thermal/ti-soc-thermal/ti-bandgap.c | 4 ----
>>>>  1 file changed, 4 deletions(-)
>>>>
>>>> diff --git a/drivers/thermal/ti-soc-thermal/ti-bandgap.c b/drivers/thermal/ti-soc-thermal/ti-bandgap.c
>>>> index d81af89166d2..684ffb645aa9 100644
>>>> --- a/drivers/thermal/ti-soc-thermal/ti-bandgap.c
>>>> +++ b/drivers/thermal/ti-soc-thermal/ti-bandgap.c
>>>> @@ -1142,14 +1142,10 @@ static int ti_bandgap_restore_ctxt(struct ti_bandgap *bgp)
>>>>  	for (i = 0; i < bgp->conf->sensor_count; i++) {
>>>>  		struct temp_sensor_registers *tsr;
>>>>  		struct temp_sensor_regval *rval;
>>>> -		u32 val = 0;
>>>>  
>>>>  		rval = &bgp->regval[i];
>>>>  		tsr = bgp->conf->sensors[i].registers;
>>>>  
>>>> -		if (TI_BANDGAP_HAS(bgp, COUNTER))
>>>> -			val = ti_bandgap_readl(bgp, tsr->bgap_counter);
>>>
>>> Are you sure that this hardware does not require this read to happen in
>>> order for it to work properly?
>>
>> Yes, initially we had the same concern but we were unable to find
>> anything specific in the history. The commit mentioned above removed the
>> user of the 'val' code but without removing this block of code.
>>
>> When looking at the current code, it really looks like an oversight.
> 
> Yes so it seems.
> 
>> There is nothing in the commit's changelog referring to a need of
>> reading the counter register but perhaps I'm wrong because I'm not sure
>> to understand correctly the changelog.
>>
>>> Lots of hardware does need this, have you tested this?
> 
> I just tested this on omap3 logicpd torpedo devkit that can do off during
> idle and reading /sys/class/thermal/thermal_zone0/temp works. So feel
> free to add:
> 
> Reviewed-by: Tony Lindgren <tony@atomide.com>
> Tested-by: Tony Lindgren <tony@atomide.com>

Thanks for testing


-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

答复: [PATCH v3] thermal: ti-soc-thermal: Remove unused variable 'val'

[linruizhe]

Hi Daniel,

Do I need to make more changes to this patch?

Thanks
 
  -Lin Ruizhe

-----邮件原件-----
发件人: Daniel Lezcano [mailto:daniel.lezcano@linaro.org] 
发送时间: 2021年4月21日 19:37
收件人: Tony Lindgren <tony@atomide.com>
抄送: Greg KH <gregkh@linuxfoundation.org>; linruizhe <linruizhe@huawei.com>; rui.zhang@intel.com; edubezval@gmail.com; j-keerthy@ti.com; amitk@kernel.org; eballetbo@gmail.com; linux-pm@vger.kernel.org; linux-omap@vger.kernel.org; linux-kernel@vger.kernel.org
主题: Re: [PATCH v3] thermal: ti-soc-thermal: Remove unused variable 'val'


Hi Tony,

thanks for testing

  -- Daniel

On 21/04/2021 11:37, Tony Lindgren wrote:
> * Daniel Lezcano <daniel.lezcano@linaro.org> [210421 09:07]:
>> On 21/04/2021 10:49, Greg KH wrote:
>>> On Wed, Apr 21, 2021 at 04:42:56PM +0800, Lin Ruizhe wrote:
>>>> The variable 'val'in function ti_bandgap_restore_ctxt is the 
>>>> register value of read bandgap registers. This function is to 
>>>> restore the context. But there is no operation on the return value 
>>>> of this register, so this block is redundant. Hulk robot scans this 
>>>> warning.This commit remove the dead code.
>>>>
>>>> Reported-by: Hulk Robot <hulkci@huawei.com>
>>>> Fixes: b87ea759a4cc ("staging: omap-thermal: fix context restore 
>>>> function")
>>>> Signed-off-by: Lin Ruizhe <linruizhe@huawei.com>
>>>> ---
>>>> v3:
>>>> -Add Fixes tag and more accurate commit message in this patch.
>>>> v2:
>>>> -As suggest remove the whole unuesed block in fuction  
>>>> ti_bandgap_restore_ctxt
>>>>
>>>>  drivers/thermal/ti-soc-thermal/ti-bandgap.c | 4 ----
>>>>  1 file changed, 4 deletions(-)
>>>>
>>>> diff --git a/drivers/thermal/ti-soc-thermal/ti-bandgap.c 
>>>> b/drivers/thermal/ti-soc-thermal/ti-bandgap.c
>>>> index d81af89166d2..684ffb645aa9 100644
>>>> --- a/drivers/thermal/ti-soc-thermal/ti-bandgap.c
>>>> +++ b/drivers/thermal/ti-soc-thermal/ti-bandgap.c
>>>> @@ -1142,14 +1142,10 @@ static int ti_bandgap_restore_ctxt(struct ti_bandgap *bgp)
>>>>  	for (i = 0; i < bgp->conf->sensor_count; i++) {
>>>>  		struct temp_sensor_registers *tsr;
>>>>  		struct temp_sensor_regval *rval;
>>>> -		u32 val = 0;
>>>>  
>>>>  		rval = &bgp->regval[i];
>>>>  		tsr = bgp->conf->sensors[i].registers;
>>>>  
>>>> -		if (TI_BANDGAP_HAS(bgp, COUNTER))
>>>> -			val = ti_bandgap_readl(bgp, tsr->bgap_counter);
>>>
>>> Are you sure that this hardware does not require this read to happen 
>>> in order for it to work properly?
>>
>> Yes, initially we had the same concern but we were unable to find 
>> anything specific in the history. The commit mentioned above removed 
>> the user of the 'val' code but without removing this block of code.
>>
>> When looking at the current code, it really looks like an oversight.
> 
> Yes so it seems.
> 
>> There is nothing in the commit's changelog referring to a need of 
>> reading the counter register but perhaps I'm wrong because I'm not 
>> sure to understand correctly the changelog.
>>
>>> Lots of hardware does need this, have you tested this?
> 
> I just tested this on omap3 logicpd torpedo devkit that can do off 
> during idle and reading /sys/class/thermal/thermal_zone0/temp works. 
> So feel free to add:
> 
> Reviewed-by: Tony Lindgren <tony@atomide.com>
> Tested-by: Tony Lindgren <tony@atomide.com>

Thanks for testing


--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook | <http://twitter.com/#!/linaroorg> Twitter | <http://www.linaro.org/linaro-blog/> Blog

Re: 答复: [PATCH v3] thermal: ti-soc-thermal: Remove unused variable 'val'

[Daniel Lezcano]

Hi Lin,


On 22/04/2021 14:39, linruizhe wrote:
> Hi Daniel,
> 
> Do I need to make more changes to this patch?

No, it is fine. I've applied it.

Thanks

  -- Daniel


> Thanks
>  
>   -Lin Ruizhe
> 
> -----邮件原件-----
> 发件人: Daniel Lezcano [mailto:daniel.lezcano@linaro.org] 
> 发送时间: 2021年4月21日 19:37
> 收件人: Tony Lindgren <tony@atomide.com>
> 抄送: Greg KH <gregkh@linuxfoundation.org>; linruizhe <linruizhe@huawei.com>; rui.zhang@intel.com; edubezval@gmail.com; j-keerthy@ti.com; amitk@kernel.org; eballetbo@gmail.com; linux-pm@vger.kernel.org; linux-omap@vger.kernel.org; linux-kernel@vger.kernel.org
> 主题: Re: [PATCH v3] thermal: ti-soc-thermal: Remove unused variable 'val'
> 
> 
> Hi Tony,
> 
> thanks for testing
> 
>   -- Daniel
> 
> On 21/04/2021 11:37, Tony Lindgren wrote:
>> * Daniel Lezcano <daniel.lezcano@linaro.org> [210421 09:07]:
>>> On 21/04/2021 10:49, Greg KH wrote:
>>>> On Wed, Apr 21, 2021 at 04:42:56PM +0800, Lin Ruizhe wrote:
>>>>> The variable 'val'in function ti_bandgap_restore_ctxt is the 
>>>>> register value of read bandgap registers. This function is to 
>>>>> restore the context. But there is no operation on the return value 
>>>>> of this register, so this block is redundant. Hulk robot scans this 
>>>>> warning.This commit remove the dead code.
>>>>>
>>>>> Reported-by: Hulk Robot <hulkci@huawei.com>
>>>>> Fixes: b87ea759a4cc ("staging: omap-thermal: fix context restore 
>>>>> function")
>>>>> Signed-off-by: Lin Ruizhe <linruizhe@huawei.com>
>>>>> ---
>>>>> v3:
>>>>> -Add Fixes tag and more accurate commit message in this patch.
>>>>> v2:
>>>>> -As suggest remove the whole unuesed block in fuction  
>>>>> ti_bandgap_restore_ctxt
>>>>>
>>>>>  drivers/thermal/ti-soc-thermal/ti-bandgap.c | 4 ----
>>>>>  1 file changed, 4 deletions(-)
>>>>>
>>>>> diff --git a/drivers/thermal/ti-soc-thermal/ti-bandgap.c 
>>>>> b/drivers/thermal/ti-soc-thermal/ti-bandgap.c
>>>>> index d81af89166d2..684ffb645aa9 100644
>>>>> --- a/drivers/thermal/ti-soc-thermal/ti-bandgap.c
>>>>> +++ b/drivers/thermal/ti-soc-thermal/ti-bandgap.c
>>>>> @@ -1142,14 +1142,10 @@ static int ti_bandgap_restore_ctxt(struct ti_bandgap *bgp)
>>>>>  	for (i = 0; i < bgp->conf->sensor_count; i++) {
>>>>>  		struct temp_sensor_registers *tsr;
>>>>>  		struct temp_sensor_regval *rval;
>>>>> -		u32 val = 0;
>>>>>  
>>>>>  		rval = &bgp->regval[i];
>>>>>  		tsr = bgp->conf->sensors[i].registers;
>>>>>  
>>>>> -		if (TI_BANDGAP_HAS(bgp, COUNTER))
>>>>> -			val = ti_bandgap_readl(bgp, tsr->bgap_counter);
>>>>
>>>> Are you sure that this hardware does not require this read to happen 
>>>> in order for it to work properly?
>>>
>>> Yes, initially we had the same concern but we were unable to find 
>>> anything specific in the history. The commit mentioned above removed 
>>> the user of the 'val' code but without removing this block of code.
>>>
>>> When looking at the current code, it really looks like an oversight.
>>
>> Yes so it seems.
>>
>>> There is nothing in the commit's changelog referring to a need of 
>>> reading the counter register but perhaps I'm wrong because I'm not 
>>> sure to understand correctly the changelog.
>>>
>>>> Lots of hardware does need this, have you tested this?
>>
>> I just tested this on omap3 logicpd torpedo devkit that can do off 
>> during idle and reading /sys/class/thermal/thermal_zone0/temp works. 
>> So feel free to add:
>>
>> Reviewed-by: Tony Lindgren <tony@atomide.com>
>> Tested-by: Tony Lindgren <tony@atomide.com>
> 
> Thanks for testing
> 
> 
> --
> <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
> 
> Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook | <http://twitter.com/#!/linaroorg> Twitter | <http://www.linaro.org/linaro-blog/> Blog
> 


-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog