| Message ID | 20210701222518.3895552-1-linus.walleij@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/dbi: Print errors for mipi_dbi_command() | expand |
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
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
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
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
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(-)