The macro mipi_dbi_command() does not report errors unless you wrap it in another macro to do the error reporting.
Report a rate-limited error so we know what is going on.
Drop the only user in DRM using mipi_dbi_command() and actually checking the error explicitly, let it use mipi_dbi_command_buf() directly instead.
After this any code wishing to send command arrays can rely on mipi_dbi_command() providing an appropriate error message if something goes wrong.
Suggested-by: Noralf Trønnes noralf@tronnes.org Suggested-by: Douglas Anderson dianders@chromium.org Signed-off-by: Linus Walleij linus.walleij@linaro.org --- ChangeLog v1->v2: - Fish out the struct device * from the DBI SPI client and use that to print the errors associated with the SPI device. --- drivers/gpu/drm/drm_mipi_dbi.c | 2 +- include/drm/drm_mipi_dbi.h | 6 +++++- 2 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_mipi_dbi.c b/drivers/gpu/drm/drm_mipi_dbi.c index 3854fb9798e9..c7c1b75df190 100644 --- a/drivers/gpu/drm/drm_mipi_dbi.c +++ b/drivers/gpu/drm/drm_mipi_dbi.c @@ -645,7 +645,7 @@ static int mipi_dbi_poweron_reset_conditional(struct mipi_dbi_dev *dbidev, bool return 1;
mipi_dbi_hw_reset(dbi); - ret = mipi_dbi_command(dbi, MIPI_DCS_SOFT_RESET); + ret = mipi_dbi_command_buf(dbi, MIPI_DCS_SOFT_RESET, NULL, 0); if (ret) { DRM_DEV_ERROR(dev, "Failed to send reset command (%d)\n", ret); if (dbidev->regulator) diff --git a/include/drm/drm_mipi_dbi.h b/include/drm/drm_mipi_dbi.h index f543d6e3e822..f00cb9690cf2 100644 --- a/include/drm/drm_mipi_dbi.h +++ b/include/drm/drm_mipi_dbi.h @@ -183,7 +183,11 @@ int mipi_dbi_buf_copy(void *dst, struct drm_framebuffer *fb, #define mipi_dbi_command(dbi, cmd, seq...) \ ({ \ const u8 d[] = { seq }; \ - mipi_dbi_command_stackbuf(dbi, cmd, d, ARRAY_SIZE(d)); \ + struct device *dev = &dbi->spi->dev; \ + int ret; \ + ret = mipi_dbi_command_stackbuf(dbi, cmd, d, ARRAY_SIZE(d)); \ + if (ret) \ + dev_err_ratelimited(dev, "error %d when sending command\n", ret); \ })
#ifdef CONFIG_DEBUG_FS
On Fri, Jul 02, 2021 at 12:04:55PM +0200, Linus Walleij wrote:
The macro mipi_dbi_command() does not report errors unless you wrap it in another macro to do the error reporting.
Report a rate-limited error so we know what is going on.
Drop the only user in DRM using mipi_dbi_command() and actually checking the error explicitly, let it use mipi_dbi_command_buf() directly instead.
After this any code wishing to send command arrays can rely on mipi_dbi_command() providing an appropriate error message if something goes wrong.
Suggested-by: Noralf Trønnes noralf@tronnes.org Suggested-by: Douglas Anderson dianders@chromium.org Signed-off-by: Linus Walleij linus.walleij@linaro.org
Signed-off-by: Sam Ravnborg sam@ravnborg.org
On Fri, Jul 02, 2021 at 12:27:42PM +0200, Sam Ravnborg wrote:
On Fri, Jul 02, 2021 at 12:04:55PM +0200, Linus Walleij wrote:
The macro mipi_dbi_command() does not report errors unless you wrap it in another macro to do the error reporting.
Report a rate-limited error so we know what is going on.
Drop the only user in DRM using mipi_dbi_command() and actually checking the error explicitly, let it use mipi_dbi_command_buf() directly instead.
After this any code wishing to send command arrays can rely on mipi_dbi_command() providing an appropriate error message if something goes wrong.
Suggested-by: Noralf Trønnes noralf@tronnes.org Suggested-by: Douglas Anderson dianders@chromium.org Signed-off-by: Linus Walleij linus.walleij@linaro.org
Signed-off-by: Sam Ravnborg sam@ravnborg.org
Too fast send, this should read: Reviewed-by: Sam Ravnborg sam@ravnborg.org
Den 02.07.2021 12.04, skrev Linus Walleij:
The macro mipi_dbi_command() does not report errors unless you wrap it in another macro to do the error reporting.
Report a rate-limited error so we know what is going on.
Drop the only user in DRM using mipi_dbi_command() and actually checking the error explicitly, let it use mipi_dbi_command_buf() directly instead.
After this any code wishing to send command arrays can rely on mipi_dbi_command() providing an appropriate error message if something goes wrong.
Suggested-by: Noralf Trønnes noralf@tronnes.org Suggested-by: Douglas Anderson dianders@chromium.org Signed-off-by: Linus Walleij linus.walleij@linaro.org
ChangeLog v1->v2:
- Fish out the struct device * from the DBI SPI client and use that to print the errors associated with the SPI device.
drivers/gpu/drm/drm_mipi_dbi.c | 2 +- include/drm/drm_mipi_dbi.h | 6 +++++- 2 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_mipi_dbi.c b/drivers/gpu/drm/drm_mipi_dbi.c index 3854fb9798e9..c7c1b75df190 100644 --- a/drivers/gpu/drm/drm_mipi_dbi.c +++ b/drivers/gpu/drm/drm_mipi_dbi.c @@ -645,7 +645,7 @@ static int mipi_dbi_poweron_reset_conditional(struct mipi_dbi_dev *dbidev, bool return 1;
mipi_dbi_hw_reset(dbi);
- ret = mipi_dbi_command(dbi, MIPI_DCS_SOFT_RESET);
- ret = mipi_dbi_command_buf(dbi, MIPI_DCS_SOFT_RESET, NULL, 0); if (ret) { DRM_DEV_ERROR(dev, "Failed to send reset command (%d)\n", ret); if (dbidev->regulator)
diff --git a/include/drm/drm_mipi_dbi.h b/include/drm/drm_mipi_dbi.h index f543d6e3e822..f00cb9690cf2 100644 --- a/include/drm/drm_mipi_dbi.h +++ b/include/drm/drm_mipi_dbi.h @@ -183,7 +183,11 @@ int mipi_dbi_buf_copy(void *dst, struct drm_framebuffer *fb, #define mipi_dbi_command(dbi, cmd, seq...) \ ({ \ const u8 d[] = { seq }; \
- mipi_dbi_command_stackbuf(dbi, cmd, d, ARRAY_SIZE(d)); \
- struct device *dev = &dbi->spi->dev; \
- int ret; \
- ret = mipi_dbi_command_stackbuf(dbi, cmd, d, ARRAY_SIZE(d)); \
- if (ret) \
dev_err_ratelimited(dev, "error %d when sending command\n", ret); \
Nit: Printing the failing command would have been useful, like you did in the driver macro.
})
I would have preferred if mipi_dbi_command could have returned the error code. This indicates that it should be possible: https://stackoverflow.com/questions/3532621/using-and-returning-output-in-c-...
But I can live with this, but if drivers want to start checking the error code we might have to rethink this.
But this works as things are now:
Reviewed-by: Noralf Trønnes noralf@tronnes.org
Hi Linus,
I love your patch! Yet something to improve:
[auto build test ERROR on linus/master] [also build test ERROR on v5.13 next-20210701] [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/Linus-Walleij/drm-dbi-Print-errors-... base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 3dbdb38e286903ec220aaf1fb29a8d94297da246 config: m68k-allmodconfig (attached as .config) compiler: m68k-linux-gcc (GCC) 9.3.0 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/42d93a52e398adbb1fe2dfbc895c649cc8d4... git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Linus-Walleij/drm-dbi-Print-errors-for-mipi_dbi_command/20210702-180745 git checkout 42d93a52e398adbb1fe2dfbc895c649cc8d42780 # save the attached .config to linux build tree mkdir build_dir COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross O=build_dir ARCH=m68k SHELL=/bin/bash drivers/gpu/drm/tiny/
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 >>):
In file included from drivers/gpu/drm/tiny/st7586.c:25: drivers/gpu/drm/tiny/st7586.c: In function 'st7586_pipe_disable':
include/drm/drm_mipi_dbi.h:186:27: error: invalid type argument of '->' (have 'struct mipi_dbi')
186 | struct device *dev = &dbi->spi->dev; \ | ^~ drivers/gpu/drm/tiny/st7586.c:260:2: note: in expansion of macro 'mipi_dbi_command' 260 | mipi_dbi_command(&dbidev->dbi, MIPI_DCS_SET_DISPLAY_OFF); | ^~~~~~~~~~~~~~~~
vim +186 include/drm/drm_mipi_dbi.h
160 161 u32 mipi_dbi_spi_cmd_max_speed(struct spi_device *spi, size_t len); 162 int mipi_dbi_spi_transfer(struct spi_device *spi, u32 speed_hz, 163 u8 bpw, const void *buf, size_t len); 164 165 int mipi_dbi_command_read(struct mipi_dbi *dbi, u8 cmd, u8 *val); 166 int mipi_dbi_command_buf(struct mipi_dbi *dbi, u8 cmd, u8 *data, size_t len); 167 int mipi_dbi_command_stackbuf(struct mipi_dbi *dbi, u8 cmd, const u8 *data, 168 size_t len); 169 int mipi_dbi_buf_copy(void *dst, struct drm_framebuffer *fb, 170 struct drm_rect *clip, bool swap); 171 /** 172 * mipi_dbi_command - MIPI DCS command with optional parameter(s) 173 * @dbi: MIPI DBI structure 174 * @cmd: Command 175 * @seq: Optional parameter(s) 176 * 177 * Send MIPI DCS command to the controller. Use mipi_dbi_command_read() for 178 * get/read. 179 * 180 * Returns: 181 * Zero on success, negative error code on failure. 182 */ 183 #define mipi_dbi_command(dbi, cmd, seq...) \ 184 ({ \ 185 const u8 d[] = { seq }; \
186 struct device *dev = &dbi->spi->dev; \
187 int ret; \ 188 ret = mipi_dbi_command_stackbuf(dbi, cmd, d, ARRAY_SIZE(d)); \ 189 if (ret) \ 190 dev_err_ratelimited(dev, "error %d when sending command\n", ret); \ 191 }) 192
--- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Hi Linus,
I love your patch! Yet something to improve:
[auto build test ERROR on linus/master] [also build test ERROR on v5.13 next-20210701] [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/Linus-Walleij/drm-dbi-Print-errors-... base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 3dbdb38e286903ec220aaf1fb29a8d94297da246 config: arm64-randconfig-r001-20210702 (attached as .config) compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project 9eb613b2de3163686b1a4bd1160f15ac56a4b083) 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 arm64 cross compiling tool for clang build # apt-get install binutils-aarch64-linux-gnu # https://github.com/0day-ci/linux/commit/42d93a52e398adbb1fe2dfbc895c649cc8d4... git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Linus-Walleij/drm-dbi-Print-errors-for-mipi_dbi_command/20210702-180745 git checkout 42d93a52e398adbb1fe2dfbc895c649cc8d42780 # save the attached .config to linux build tree mkdir build_dir COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross O=build_dir ARCH=arm64 SHELL=/bin/bash arch/arm64/kvm/ drivers/gpu/drm/tiny/
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/tiny/st7586.c:260:2: error: member reference type 'struct mipi_dbi' is not a pointer; did you mean to use '.'?
mipi_dbi_command(&dbidev->dbi, MIPI_DCS_SET_DISPLAY_OFF); ^ ~~~~~~~~~~~ include/drm/drm_mipi_dbi.h:186:27: note: expanded from macro 'mipi_dbi_command' struct device *dev = &dbi->spi->dev; \ ~~~^
drivers/gpu/drm/tiny/st7586.c:260:2: error: cannot take the address of an rvalue of type 'struct device *'
mipi_dbi_command(&dbidev->dbi, MIPI_DCS_SET_DISPLAY_OFF); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ include/drm/drm_mipi_dbi.h:186:23: note: expanded from macro 'mipi_dbi_command' struct device *dev = &dbi->spi->dev; \ ^~~~~~~~~~~~~~ 2 errors generated.
vim +260 drivers/gpu/drm/tiny/st7586.c
eac99d4a2013d9 drivers/gpu/drm/tinydrm/st7586.c David Lechner 2017-08-07 246 eac99d4a2013d9 drivers/gpu/drm/tinydrm/st7586.c David Lechner 2017-08-07 247 static void st7586_pipe_disable(struct drm_simple_display_pipe *pipe) eac99d4a2013d9 drivers/gpu/drm/tinydrm/st7586.c David Lechner 2017-08-07 248 { 84137b866e834a drivers/gpu/drm/tinydrm/st7586.c Noralf Trønnes 2019-07-22 249 struct mipi_dbi_dev *dbidev = drm_to_mipi_dbi_dev(pipe->crtc.dev); eac99d4a2013d9 drivers/gpu/drm/tinydrm/st7586.c David Lechner 2017-08-07 250 9d5645ad1b979c drivers/gpu/drm/tinydrm/st7586.c Noralf Trønnes 2019-02-25 251 /* 9d5645ad1b979c drivers/gpu/drm/tinydrm/st7586.c Noralf Trønnes 2019-02-25 252 * This callback is not protected by drm_dev_enter/exit since we want to 9d5645ad1b979c drivers/gpu/drm/tinydrm/st7586.c Noralf Trønnes 2019-02-25 253 * turn off the display on regular driver unload. It's highly unlikely 9d5645ad1b979c drivers/gpu/drm/tinydrm/st7586.c Noralf Trønnes 2019-02-25 254 * that the underlying SPI controller is gone should this be called after 9d5645ad1b979c drivers/gpu/drm/tinydrm/st7586.c Noralf Trønnes 2019-02-25 255 * unplug. 9d5645ad1b979c drivers/gpu/drm/tinydrm/st7586.c Noralf Trønnes 2019-02-25 256 */ 9d5645ad1b979c drivers/gpu/drm/tinydrm/st7586.c Noralf Trønnes 2019-02-25 257 eac99d4a2013d9 drivers/gpu/drm/tinydrm/st7586.c David Lechner 2017-08-07 258 DRM_DEBUG_KMS("\n"); eac99d4a2013d9 drivers/gpu/drm/tinydrm/st7586.c David Lechner 2017-08-07 259 84137b866e834a drivers/gpu/drm/tinydrm/st7586.c Noralf Trønnes 2019-07-22 @260 mipi_dbi_command(&dbidev->dbi, MIPI_DCS_SET_DISPLAY_OFF); eac99d4a2013d9 drivers/gpu/drm/tinydrm/st7586.c David Lechner 2017-08-07 261 } eac99d4a2013d9 drivers/gpu/drm/tinydrm/st7586.c David Lechner 2017-08-07 262
--- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
dri-devel@lists.freedesktop.org
-
kernel test robot -
Linus Walleij -
Noralf Trønnes -
Sam Ravnborg