diff mbox series

[v4,68/80] drm/panel: panel-dsi-cm: remove extra 'if'

Message ID 20201124124538.660710-69-tomi.valkeinen@ti.com (mailing list archive)
State New, archived
Headers show
Series Convert DSI code to use drm_mipi_dsi and drm_panel | expand

Commit Message

Tomi Valkeinen Nov. 24, 2020, 12:45 p.m. UTC 
We have a useless 'if' in the dsicm_bl_update_status(), a left over from
the conversion to DRM model. Drop the if.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/gpu/drm/panel/panel-dsi-cm.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

Comments

Sam Ravnborg Nov. 24, 2020, 5:34 p.m. UTC | #1 
Hi Tomi,

On Tue, Nov 24, 2020 at 02:45:26PM +0200, Tomi Valkeinen wrote:
> We have a useless 'if' in the dsicm_bl_update_status(), a left over from
> the conversion to DRM model. Drop the if.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
Reviewed-by: Sam Ravnborg <sam@ravnborg.org>

But with a few comments.

> ---
>  drivers/gpu/drm/panel/panel-dsi-cm.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panel/panel-dsi-cm.c b/drivers/gpu/drm/panel/panel-dsi-cm.c
> index 1e7f73340736..c17ed728c695 100644
> --- a/drivers/gpu/drm/panel/panel-dsi-cm.c
> +++ b/drivers/gpu/drm/panel/panel-dsi-cm.c
> @@ -194,7 +194,7 @@ static int dsicm_set_update_window(struct panel_drv_data *ddata)
>  static int dsicm_bl_update_status(struct backlight_device *dev)
>  {
>  	struct panel_drv_data *ddata = dev_get_drvdata(&dev->dev);
> -	int r = 0;
> +	int r;
>  	int level;
>  
>  	if (dev->props.fb_blank == FB_BLANK_UNBLANK &&

Unrelated to this patch - but here it would be better to use
level = backlight_get_brightness(dev);

level is 0 is backlight is off.

	Sam


> @@ -207,11 +207,9 @@ static int dsicm_bl_update_status(struct backlight_device *dev)
>  
>  	mutex_lock(&ddata->lock);
>  
> -	if (ddata->enabled) {
> -		if (!r)
> -			r = dsicm_dcs_write_1(
> -				ddata, MIPI_DCS_SET_DISPLAY_BRIGHTNESS, level);
> -	}
> +	if (ddata->enabled)
> +		r = dsicm_dcs_write_1(ddata, MIPI_DCS_SET_DISPLAY_BRIGHTNESS,
> +				      level);
>  
>  	mutex_unlock(&ddata->lock);
>  
> -- 
> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
Laurent Pinchart Dec. 1, 2020, 12:22 a.m. UTC | #2 
Hi Tomi,

Thank you for the patch.

On Tue, Nov 24, 2020 at 02:45:26PM +0200, Tomi Valkeinen wrote:
> We have a useless 'if' in the dsicm_bl_update_status(), a left over from
> the conversion to DRM model. Drop the if.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
>  drivers/gpu/drm/panel/panel-dsi-cm.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panel/panel-dsi-cm.c b/drivers/gpu/drm/panel/panel-dsi-cm.c
> index 1e7f73340736..c17ed728c695 100644
> --- a/drivers/gpu/drm/panel/panel-dsi-cm.c
> +++ b/drivers/gpu/drm/panel/panel-dsi-cm.c
> @@ -194,7 +194,7 @@ static int dsicm_set_update_window(struct panel_drv_data *ddata)
>  static int dsicm_bl_update_status(struct backlight_device *dev)
>  {
>  	struct panel_drv_data *ddata = dev_get_drvdata(&dev->dev);
> -	int r = 0;
> +	int r;
>  	int level;
>  
>  	if (dev->props.fb_blank == FB_BLANK_UNBLANK &&
> @@ -207,11 +207,9 @@ static int dsicm_bl_update_status(struct backlight_device *dev)
>  
>  	mutex_lock(&ddata->lock);
>  
> -	if (ddata->enabled) {
> -		if (!r)
> -			r = dsicm_dcs_write_1(
> -				ddata, MIPI_DCS_SET_DISPLAY_BRIGHTNESS, level);
> -	}
> +	if (ddata->enabled)
> +		r = dsicm_dcs_write_1(ddata, MIPI_DCS_SET_DISPLAY_BRIGHTNESS,
> +				      level);

If !ddata->enabled, won't r be uninitialized ?

>  
>  	mutex_unlock(&ddata->lock);
>
Tomi Valkeinen Dec. 1, 2020, 11:26 a.m. UTC | #3 
On 01/12/2020 02:22, Laurent Pinchart wrote:
> Hi Tomi,
> 
> Thank you for the patch.
> 
> On Tue, Nov 24, 2020 at 02:45:26PM +0200, Tomi Valkeinen wrote:
>> We have a useless 'if' in the dsicm_bl_update_status(), a left over from
>> the conversion to DRM model. Drop the if.
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
>> ---
>>  drivers/gpu/drm/panel/panel-dsi-cm.c | 10 ++++------
>>  1 file changed, 4 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/panel/panel-dsi-cm.c b/drivers/gpu/drm/panel/panel-dsi-cm.c
>> index 1e7f73340736..c17ed728c695 100644
>> --- a/drivers/gpu/drm/panel/panel-dsi-cm.c
>> +++ b/drivers/gpu/drm/panel/panel-dsi-cm.c
>> @@ -194,7 +194,7 @@ static int dsicm_set_update_window(struct panel_drv_data *ddata)
>>  static int dsicm_bl_update_status(struct backlight_device *dev)
>>  {
>>  	struct panel_drv_data *ddata = dev_get_drvdata(&dev->dev);
>> -	int r = 0;
>> +	int r;
>>  	int level;
>>  
>>  	if (dev->props.fb_blank == FB_BLANK_UNBLANK &&
>> @@ -207,11 +207,9 @@ static int dsicm_bl_update_status(struct backlight_device *dev)
>>  
>>  	mutex_lock(&ddata->lock);
>>  
>> -	if (ddata->enabled) {
>> -		if (!r)
>> -			r = dsicm_dcs_write_1(
>> -				ddata, MIPI_DCS_SET_DISPLAY_BRIGHTNESS, level);
>> -	}
>> +	if (ddata->enabled)
>> +		r = dsicm_dcs_write_1(ddata, MIPI_DCS_SET_DISPLAY_BRIGHTNESS,
>> +				      level);
> 
> If !ddata->enabled, won't r be uninitialized ?

Yes. I wonder how I missed that... I'll keep the "r = 0" initialization.

 Tomi
kernel test robot Dec. 2, 2020, 8:08 p.m. UTC | #4 
Hi Tomi,

I love your patch! Perhaps something to improve:

[auto build test WARNING on omap/for-next]
[also build test WARNING on robh/for-next balbi-usb/testing/next linus/master v5.10-rc6]
[cannot apply to next-20201201]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Tomi-Valkeinen/Convert-DSI-code-to-use-drm_mipi_dsi-and-drm_panel/20201124-205129
base:   https://git.kernel.org/pub/scm/linux/kernel/git/tmlind/linux-omap.git for-next
config: riscv-randconfig-r016-20201202 (attached as .config)
compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project 2671fccf0381769276ca8246ec0499adcb9b0355)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install riscv cross compiling tool for clang build
        # apt-get install binutils-riscv64-linux-gnu
        # https://github.com/0day-ci/linux/commit/09d304dc23e60a46580ec8a3d7db7210138fc9db
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Tomi-Valkeinen/Convert-DSI-code-to-use-drm_mipi_dsi-and-drm_panel/20201124-205129
        git checkout 09d304dc23e60a46580ec8a3d7db7210138fc9db
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=riscv 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> drivers/gpu/drm/panel/panel-dsi-cm.c:210:6: warning: variable 'r' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
           if (ddata->enabled)
               ^~~~~~~~~~~~~~
   drivers/gpu/drm/panel/panel-dsi-cm.c:216:9: note: uninitialized use occurs here
           return r;
                  ^
   drivers/gpu/drm/panel/panel-dsi-cm.c:210:2: note: remove the 'if' if its condition is always true
           if (ddata->enabled)
           ^~~~~~~~~~~~~~~~~~~
   drivers/gpu/drm/panel/panel-dsi-cm.c:197:7: note: initialize the variable 'r' to silence this warning
           int r;
                ^
                 = 0
   1 warning generated.

vim +210 drivers/gpu/drm/panel/panel-dsi-cm.c

   193	
   194	static int dsicm_bl_update_status(struct backlight_device *dev)
   195	{
   196		struct panel_drv_data *ddata = dev_get_drvdata(&dev->dev);
   197		int r;
   198		int level;
   199	
   200		if (dev->props.fb_blank == FB_BLANK_UNBLANK &&
   201				dev->props.power == FB_BLANK_UNBLANK)
   202			level = dev->props.brightness;
   203		else
   204			level = 0;
   205	
   206		dev_dbg(&ddata->dsi->dev, "update brightness to %d\n", level);
   207	
   208		mutex_lock(&ddata->lock);
   209	
 > 210		if (ddata->enabled)
   211			r = dsicm_dcs_write_1(ddata, MIPI_DCS_SET_DISPLAY_BRIGHTNESS,
   212					      level);
   213	
   214		mutex_unlock(&ddata->lock);
   215	
   216		return r;
   217	}
   218	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
diff mbox series

Patch

diff --git a/drivers/gpu/drm/panel/panel-dsi-cm.c b/drivers/gpu/drm/panel/panel-dsi-cm.c
index 1e7f73340736..c17ed728c695 100644
--- a/drivers/gpu/drm/panel/panel-dsi-cm.c
+++ b/drivers/gpu/drm/panel/panel-dsi-cm.c
@@ -194,7 +194,7 @@  static int dsicm_set_update_window(struct panel_drv_data *ddata)
 static int dsicm_bl_update_status(struct backlight_device *dev)
 {
 	struct panel_drv_data *ddata = dev_get_drvdata(&dev->dev);
-	int r = 0;
+	int r;
 	int level;
 
 	if (dev->props.fb_blank == FB_BLANK_UNBLANK &&
@@ -207,11 +207,9 @@  static int dsicm_bl_update_status(struct backlight_device *dev)
 
 	mutex_lock(&ddata->lock);
 
-	if (ddata->enabled) {
-		if (!r)
-			r = dsicm_dcs_write_1(
-				ddata, MIPI_DCS_SET_DISPLAY_BRIGHTNESS, level);
-	}
+	if (ddata->enabled)
+		r = dsicm_dcs_write_1(ddata, MIPI_DCS_SET_DISPLAY_BRIGHTNESS,
+				      level);
 
 	mutex_unlock(&ddata->lock);