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

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

[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>
---
 drivers/gpu/drm/drm_mipi_dbi.c | 2 +-
 include/drm/drm_mipi_dbi.h     | 5 ++++-
 2 files changed, 5 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..2057ad32760c 100644
--- a/include/drm/drm_mipi_dbi.h
+++ b/include/drm/drm_mipi_dbi.h
@@ -183,7 +183,10 @@ 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)); \
+	int ret; \
+	ret = mipi_dbi_command_stackbuf(dbi, cmd, d, ARRAY_SIZE(d)); \
+	if (ret) \
+		pr_err_ratelimited("MIPI DBI: error %d when sending command\n", ret); \
 })
 
 #ifdef CONFIG_DEBUG_FS
-- 
2.31.1

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

[Sam Ravnborg]

Hi Linus,

On Fri, Jul 02, 2021 at 12:25:18AM +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>
> ---
>  drivers/gpu/drm/drm_mipi_dbi.c | 2 +-
>  include/drm/drm_mipi_dbi.h     | 5 ++++-
>  2 files changed, 5 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)
I do not see the value in this change??
There are many other mipi_dbi_command() users and the error return
continues to be checked?!??!


> diff --git a/include/drm/drm_mipi_dbi.h b/include/drm/drm_mipi_dbi.h
> index f543d6e3e822..2057ad32760c 100644
> --- a/include/drm/drm_mipi_dbi.h
> +++ b/include/drm/drm_mipi_dbi.h
> @@ -183,7 +183,10 @@ 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)); \
> +	int ret; \
> +	ret = mipi_dbi_command_stackbuf(dbi, cmd, d, ARRAY_SIZE(d)); \
> +	if (ret) \
> +		pr_err_ratelimited("MIPI DBI: error %d when sending command\n", ret); \
>  })
Coud this be more informative if the spi device was printed, it is
available? Maybe in 99% of the cases there is only one user anyway so it
will not help?

	Sam

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

[Linus Walleij]

On Fri, Jul 2, 2021 at 7:45 AM Sam Ravnborg <sam@ravnborg.org> wrote:

> > -     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)
>
> I do not see the value in this change??
> There are many other mipi_dbi_command() users and the error return
> continues to be checked?!??!

When we change the mipi_dbi_command() macro to check the error
value we can no longer retrieve the return value from the macro
so we need to use the call the command buf directly so that we
can obtain the return code.

I do not know any other way. Suggestions welcome!

> >  #define mipi_dbi_command(dbi, cmd, seq...) \
> >  ({ \
> >       const u8 d[] = { seq }; \
> > -     mipi_dbi_command_stackbuf(dbi, cmd, d, ARRAY_SIZE(d)); \
> > +     int ret; \
> > +     ret = mipi_dbi_command_stackbuf(dbi, cmd, d, ARRAY_SIZE(d)); \
> > +     if (ret) \
> > +             pr_err_ratelimited("MIPI DBI: error %d when sending command\n", ret); \
> >  })
>
> Coud this be more informative if the spi device was printed, it is
> available? Maybe in 99% of the cases there is only one user anyway so it
> will not help?

Hm I can get to the SPI device, lemme try this.

Yours,
Linus Walleij

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

[Sam Ravnborg]

Hi Linus,

On Fri, Jul 02, 2021 at 11:58:52AM +0200, Linus Walleij wrote:
> On Fri, Jul 2, 2021 at 7:45 AM Sam Ravnborg <sam@ravnborg.org> wrote:
> 
> > > -     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)
> >
> > I do not see the value in this change??
> > There are many other mipi_dbi_command() users and the error return
> > continues to be checked?!??!
> 
> When we change the mipi_dbi_command() macro to check the error
> value we can no longer retrieve the return value from the macro
> so we need to use the call the command buf directly so that we
> can obtain the return code.
Thanks, got it now.

	Sam