diff mbox series

[v2] drm/dbi: Print errors for mipi_dbi_command()

Message ID 20210702100455.3928920-1-linus.walleij@linaro.org (mailing list archive)
State New, archived
Headers show
Series [v2] drm/dbi: Print errors for mipi_dbi_command() | expand

Commit Message

Linus Walleij July 2, 2021, 10:04 a.m. UTC 
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(-)

Comments

Sam Ravnborg July 2, 2021, 10:27 a.m. UTC | #1 
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>
Sam Ravnborg July 2, 2021, 10:28 a.m. UTC | #2 
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>
Noralf Trønnes July 2, 2021, 11:03 a.m. UTC | #3 
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-macro

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>
kernel test robot July 2, 2021, 11:42 a.m. UTC | #4 
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-for-mipi_dbi_command/20210702-180745
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/42d93a52e398adbb1fe2dfbc895c649cc8d42780
        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
kernel test robot July 2, 2021, 1:57 p.m. UTC | #5 
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-for-mipi_dbi_command/20210702-180745
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/42d93a52e398adbb1fe2dfbc895c649cc8d42780
        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
diff mbox series

Patch

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