| Message ID | 20210421084256.57591-1-linruizhe@huawei.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] thermal: ti-soc-thermal: Remove unused variable 'val' | expand |
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 >
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
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
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
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);
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(-)