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)
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);
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-brid... 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/a04870beda522c60c8604006667038a096f1... 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
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-brid... 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/a04870beda522c60c8604006667038a096f1... 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
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; }
dri-devel@lists.freedesktop.org
-
AngeloGioacchino Del Regno -
kernel test robot