[PATCH 1/3] drm/bridge: parade-ps8640: Don't try to enable VDO if poweron fails

[PATCH 1/3] drm/bridge: parade-ps8640: Don't try to enable VDO if poweron fails

[AngeloGioacchino Del Regno]

If the bridge cannot get powered on, there's no reason to try to
communicate with it: change the ps8640_bridge_poweron function to
return an error value to the caller, so that we can avoid calling
ps8640_bridge_vdo_control() in ps8640_pre_enable() if the poweron
sequence fails.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/gpu/drm/bridge/parade-ps8640.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c
index 4b36e4dc78f1..8c5402947b3c 100644
--- a/drivers/gpu/drm/bridge/parade-ps8640.c
+++ b/drivers/gpu/drm/bridge/parade-ps8640.c
@@ -293,19 +293,19 @@ static int ps8640_bridge_vdo_control(struct ps8640 *ps_bridge,
 	return 0;
 }
 
-static void ps8640_bridge_poweron(struct ps8640 *ps_bridge)
+static int ps8640_bridge_poweron(struct ps8640 *ps_bridge)
 {
 	struct regmap *map = ps_bridge->regmap[PAGE2_TOP_CNTL];
 	int ret, status;
 
 	if (ps_bridge->powered)
-		return;
+		return 0;
 
 	ret = regulator_bulk_enable(ARRAY_SIZE(ps_bridge->supplies),
 				    ps_bridge->supplies);
 	if (ret < 0) {
 		DRM_ERROR("cannot enable regulators %d\n", ret);
-		return;
+		return ret;
 	}
 
 	gpiod_set_value(ps_bridge->gpio_powerdown, 0);
@@ -352,11 +352,13 @@ static void ps8640_bridge_poweron(struct ps8640 *ps_bridge)
 
 	ps_bridge->powered = true;
 
-	return;
+	return 0;
 
 err_regulators_disable:
 	regulator_bulk_disable(ARRAY_SIZE(ps_bridge->supplies),
 			       ps_bridge->supplies);
+
+	return ret;
 }
 
 static void ps8640_bridge_poweroff(struct ps8640 *ps_bridge)
@@ -381,7 +383,9 @@ static void ps8640_pre_enable(struct drm_bridge *bridge)
 	struct ps8640 *ps_bridge = bridge_to_ps8640(bridge);
 	int ret;
 
-	ps8640_bridge_poweron(ps_bridge);
+	ret = ps8640_bridge_poweron(ps_bridge);
+	if (ret)
+		return;
 
 	ret = ps8640_bridge_vdo_control(ps_bridge, ENABLE);
 	if (ret < 0)
-- 
2.33.0

[PATCH 2/3] drm/bridge: parade-ps8640: Move real poweroff action to new function

[AngeloGioacchino Del Regno]

In preparation for varying the poweron error handling in function
ps8640_bridge_poweron(), move function ps8640_bridge_poweroff() up
and also move the actual logic to power off the chip to a new
__ps8640_bridge_poweroff() function.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/gpu/drm/bridge/parade-ps8640.c | 39 +++++++++++++++-----------
 1 file changed, 22 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c
index 8c5402947b3c..9334217d02c9 100644
--- a/drivers/gpu/drm/bridge/parade-ps8640.c
+++ b/drivers/gpu/drm/bridge/parade-ps8640.c
@@ -293,6 +293,28 @@ static int ps8640_bridge_vdo_control(struct ps8640 *ps_bridge,
 	return 0;
 }
 
+static void __ps8640_bridge_poweroff(struct ps8640 *ps_bridge)
+{
+	int ret;
+
+	gpiod_set_value(ps_bridge->gpio_reset, 1);
+	gpiod_set_value(ps_bridge->gpio_powerdown, 1);
+	if (regulator_bulk_disable(ARRAY_SIZE(ps_bridge->supplies),
+				   ps_bridge->supplies)) {
+		DRM_ERROR("cannot disable regulators %d\n", ret);
+	}
+}
+
+static void ps8640_bridge_poweroff(struct ps8640 *ps_bridge)
+{
+	if (!ps_bridge->powered)
+		return;
+
+	__ps8640_bridge_poweroff(ps_bridge);
+
+	ps_bridge->powered = false;
+}
+
 static int ps8640_bridge_poweron(struct ps8640 *ps_bridge)
 {
 	struct regmap *map = ps_bridge->regmap[PAGE2_TOP_CNTL];
@@ -361,23 +383,6 @@ static int ps8640_bridge_poweron(struct ps8640 *ps_bridge)
 	return ret;
 }
 
-static void ps8640_bridge_poweroff(struct ps8640 *ps_bridge)
-{
-	int ret;
-
-	if (!ps_bridge->powered)
-		return;
-
-	gpiod_set_value(ps_bridge->gpio_reset, 1);
-	gpiod_set_value(ps_bridge->gpio_powerdown, 1);
-	ret = regulator_bulk_disable(ARRAY_SIZE(ps_bridge->supplies),
-				     ps_bridge->supplies);
-	if (ret < 0)
-		DRM_ERROR("cannot disable regulators %d\n", ret);
-
-	ps_bridge->powered = false;
-}
-
 static void ps8640_pre_enable(struct drm_bridge *bridge)
 {
 	struct ps8640 *ps_bridge = bridge_to_ps8640(bridge);
-- 
2.33.0

Re: [PATCH 2/3] drm/bridge: parade-ps8640: Move real poweroff action to new function

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 2880 bytes --]

Hi AngeloGioacchino,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on drm/drm-next]
[also build test WARNING on v5.15-rc7 next-20211029]
[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/AngeloGioacchino-Del-Regno/drm-bridge-parade-ps8640-Don-t-try-to-enable-VDO-if-poweron-fails/20211029-212903
base:   git://anongit.freedesktop.org/drm/drm drm-next
config: arm-randconfig-r012-20211029 (attached as .config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project 5db7568a6a1fcb408eb8988abdaff2a225a8eb72)
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 arm cross compiling tool for clang build
        # apt-get install binutils-arm-linux-gnueabi
        # https://github.com/0day-ci/linux/commit/a04870beda522c60c8604006667038a096f1cbd4
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review AngeloGioacchino-Del-Regno/drm-bridge-parade-ps8640-Don-t-try-to-enable-VDO-if-poweron-fails/20211029-212903
        git checkout a04870beda522c60c8604006667038a096f1cbd4
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 ARCH=arm 

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/bridge/parade-ps8640.c:304:47: warning: variable 'ret' is uninitialized when used here [-Wuninitialized]
                   DRM_ERROR("cannot disable regulators %d\n", ret);
                                                               ^~~
   include/drm/drm_print.h:518:19: note: expanded from macro 'DRM_ERROR'
           __drm_err(fmt, ##__VA_ARGS__)
                            ^~~~~~~~~~~
   drivers/gpu/drm/bridge/parade-ps8640.c:298:9: note: initialize the variable 'ret' to silence this warning
           int ret;
                  ^
                   = 0
   1 warning generated.


vim +/ret +304 drivers/gpu/drm/bridge/parade-ps8640.c

   295	
   296	static void __ps8640_bridge_poweroff(struct ps8640 *ps_bridge)
   297	{
   298		int ret;
   299	
   300		gpiod_set_value(ps_bridge->gpio_reset, 1);
   301		gpiod_set_value(ps_bridge->gpio_powerdown, 1);
   302		if (regulator_bulk_disable(ARRAY_SIZE(ps_bridge->supplies),
   303					   ps_bridge->supplies)) {
 > 304			DRM_ERROR("cannot disable regulators %d\n", ret);
   305		}
   306	}
   307	

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

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 37617 bytes --]

Re: [PATCH 2/3] drm/bridge: parade-ps8640: Move real poweroff action to new function

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 2945 bytes --]

Hi AngeloGioacchino,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on drm/drm-next]
[also build test WARNING on v5.15-rc7 next-20211029]
[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/AngeloGioacchino-Del-Regno/drm-bridge-parade-ps8640-Don-t-try-to-enable-VDO-if-poweron-fails/20211029-212903
base:   git://anongit.freedesktop.org/drm/drm drm-next
config: arm-randconfig-r012-20211029 (attached as .config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project 5db7568a6a1fcb408eb8988abdaff2a225a8eb72)
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 arm cross compiling tool for clang build
        # apt-get install binutils-arm-linux-gnueabi
        # https://github.com/0day-ci/linux/commit/a04870beda522c60c8604006667038a096f1cbd4
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review AngeloGioacchino-Del-Regno/drm-bridge-parade-ps8640-Don-t-try-to-enable-VDO-if-poweron-fails/20211029-212903
        git checkout a04870beda522c60c8604006667038a096f1cbd4
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 ARCH=arm 

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/bridge/parade-ps8640.c:304:47: warning: variable 'ret' is uninitialized when used here [-Wuninitialized]
                   DRM_ERROR("cannot disable regulators %d\n", ret);
                                                               ^~~
   include/drm/drm_print.h:518:19: note: expanded from macro 'DRM_ERROR'
           __drm_err(fmt, ##__VA_ARGS__)
                            ^~~~~~~~~~~
   drivers/gpu/drm/bridge/parade-ps8640.c:298:9: note: initialize the variable 'ret' to silence this warning
           int ret;
                  ^
                   = 0
   1 warning generated.


vim +/ret +304 drivers/gpu/drm/bridge/parade-ps8640.c

   295	
   296	static void __ps8640_bridge_poweroff(struct ps8640 *ps_bridge)
   297	{
   298		int ret;
   299	
   300		gpiod_set_value(ps_bridge->gpio_reset, 1);
   301		gpiod_set_value(ps_bridge->gpio_powerdown, 1);
   302		if (regulator_bulk_disable(ARRAY_SIZE(ps_bridge->supplies),
   303					   ps_bridge->supplies)) {
 > 304			DRM_ERROR("cannot disable regulators %d\n", ret);
   305		}
   306	}
   307	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 37617 bytes --]

Re: [PATCH 2/3] drm/bridge: parade-ps8640: Move real poweroff action to new function

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 2780 bytes --]

Hi AngeloGioacchino,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on drm/drm-next]
[also build test ERROR on v5.15-rc7 next-20211029]
[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/AngeloGioacchino-Del-Regno/drm-bridge-parade-ps8640-Don-t-try-to-enable-VDO-if-poweron-fails/20211029-212903
base:   git://anongit.freedesktop.org/drm/drm drm-next
config: hexagon-buildonly-randconfig-r006-20211028 (attached as .config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project 5db7568a6a1fcb408eb8988abdaff2a225a8eb72)
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
        # https://github.com/0day-ci/linux/commit/a04870beda522c60c8604006667038a096f1cbd4
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review AngeloGioacchino-Del-Regno/drm-bridge-parade-ps8640-Don-t-try-to-enable-VDO-if-poweron-fails/20211029-212903
        git checkout a04870beda522c60c8604006667038a096f1cbd4
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 ARCH=hexagon 

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

All errors (new ones prefixed by >>):

>> drivers/gpu/drm/bridge/parade-ps8640.c:304:47: error: variable 'ret' is uninitialized when used here [-Werror,-Wuninitialized]
                   DRM_ERROR("cannot disable regulators %d\n", ret);
                                                               ^~~
   include/drm/drm_print.h:518:19: note: expanded from macro 'DRM_ERROR'
           __drm_err(fmt, ##__VA_ARGS__)
                            ^~~~~~~~~~~
   drivers/gpu/drm/bridge/parade-ps8640.c:298:9: note: initialize the variable 'ret' to silence this warning
           int ret;
                  ^
                   = 0
   1 error generated.


vim +/ret +304 drivers/gpu/drm/bridge/parade-ps8640.c

   295	
   296	static void __ps8640_bridge_poweroff(struct ps8640 *ps_bridge)
   297	{
   298		int ret;
   299	
   300		gpiod_set_value(ps_bridge->gpio_reset, 1);
   301		gpiod_set_value(ps_bridge->gpio_powerdown, 1);
   302		if (regulator_bulk_disable(ARRAY_SIZE(ps_bridge->supplies),
   303					   ps_bridge->supplies)) {
 > 304			DRM_ERROR("cannot disable regulators %d\n", ret);
   305		}
   306	}
   307	

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

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 39337 bytes --]

Re: [PATCH 2/3] drm/bridge: parade-ps8640: Move real poweroff action to new function

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 2843 bytes --]

Hi AngeloGioacchino,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on drm/drm-next]
[also build test ERROR on v5.15-rc7 next-20211029]
[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/AngeloGioacchino-Del-Regno/drm-bridge-parade-ps8640-Don-t-try-to-enable-VDO-if-poweron-fails/20211029-212903
base:   git://anongit.freedesktop.org/drm/drm drm-next
config: hexagon-buildonly-randconfig-r006-20211028 (attached as .config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project 5db7568a6a1fcb408eb8988abdaff2a225a8eb72)
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
        # https://github.com/0day-ci/linux/commit/a04870beda522c60c8604006667038a096f1cbd4
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review AngeloGioacchino-Del-Regno/drm-bridge-parade-ps8640-Don-t-try-to-enable-VDO-if-poweron-fails/20211029-212903
        git checkout a04870beda522c60c8604006667038a096f1cbd4
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 ARCH=hexagon 

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

All errors (new ones prefixed by >>):

>> drivers/gpu/drm/bridge/parade-ps8640.c:304:47: error: variable 'ret' is uninitialized when used here [-Werror,-Wuninitialized]
                   DRM_ERROR("cannot disable regulators %d\n", ret);
                                                               ^~~
   include/drm/drm_print.h:518:19: note: expanded from macro 'DRM_ERROR'
           __drm_err(fmt, ##__VA_ARGS__)
                            ^~~~~~~~~~~
   drivers/gpu/drm/bridge/parade-ps8640.c:298:9: note: initialize the variable 'ret' to silence this warning
           int ret;
                  ^
                   = 0
   1 error generated.


vim +/ret +304 drivers/gpu/drm/bridge/parade-ps8640.c

   295	
   296	static void __ps8640_bridge_poweroff(struct ps8640 *ps_bridge)
   297	{
   298		int ret;
   299	
   300		gpiod_set_value(ps_bridge->gpio_reset, 1);
   301		gpiod_set_value(ps_bridge->gpio_powerdown, 1);
   302		if (regulator_bulk_disable(ARRAY_SIZE(ps_bridge->supplies),
   303					   ps_bridge->supplies)) {
 > 304			DRM_ERROR("cannot disable regulators %d\n", ret);
   305		}
   306	}
   307	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 39337 bytes --]

[PATCH 3/3] drm/bridge: parade-ps8640: Perform full poweroff if poweron fails

[AngeloGioacchino Del Regno]

In function ps8640_bridge_poweron(), in case of a failure not relative
to the regulators enablement, the code was disabling the regulators but
the gpio changes that happened during the poweron sequence were not
being reverted back to a clean poweroff state.

Since it is expected that, when we enter ps8640_bridge_poweron(), the
powerdown and reset GPIOs are both in active state exactly as they were
left in the poweroff function before, we can simply call function
__ps8640_bridge_poweroff() in the failure case, reverting every change
that was done during the power on sequence.

Of course it was chosen to call the poweroff function instead of adding
code to revert the GPIO changes to the poweron one to avoid duplicating
code, as we would be doing exactly what the poweroff function does.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/gpu/drm/bridge/parade-ps8640.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c
index 9334217d02c9..8b54b515828a 100644
--- a/drivers/gpu/drm/bridge/parade-ps8640.c
+++ b/drivers/gpu/drm/bridge/parade-ps8640.c
@@ -346,7 +346,7 @@ static int ps8640_bridge_poweron(struct ps8640 *ps_bridge)
 
 	if (ret < 0) {
 		DRM_ERROR("failed read PAGE2_GPIO_H: %d\n", ret);
-		goto err_regulators_disable;
+		goto err_poweroff;
 	}
 
 	msleep(50);
@@ -362,23 +362,22 @@ static int ps8640_bridge_poweron(struct ps8640 *ps_bridge)
 	ret = regmap_update_bits(map, PAGE2_MCS_EN, MCS_EN, 0);
 	if (ret < 0) {
 		DRM_ERROR("failed write PAGE2_MCS_EN: %d\n", ret);
-		goto err_regulators_disable;
+		goto err_poweroff;
 	}
 
 	/* Switch access edp panel's edid through i2c */
 	ret = regmap_write(map, PAGE2_I2C_BYPASS, I2C_BYPASS_EN);
 	if (ret < 0) {
 		DRM_ERROR("failed write PAGE2_I2C_BYPASS: %d\n", ret);
-		goto err_regulators_disable;
+		goto err_poweroff;
 	}
 
 	ps_bridge->powered = true;
 
 	return 0;
 
-err_regulators_disable:
-	regulator_bulk_disable(ARRAY_SIZE(ps_bridge->supplies),
-			       ps_bridge->supplies);
+err_poweroff:
+	__ps8640_bridge_poweroff(ps_bridge);
 
 	return ret;
 }
-- 
2.33.0