| Message ID | 20210702135601.3952726-1-linus.walleij@linaro.org |
|---|---|
| State | Accepted |
| Commit | 3f5aa5ac0b0f9704f0c60f5fbbbcdc8c043d6eb6 |
| Headers | show |
| Series | [v3] drm/dbi: Print errors for mipi_dbi_command() | expand |
Den 02.07.2021 15.56, 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. You forgot to remove this section. With that fixed: Reviewed-by: Noralf Trønnes <noralf@tronnes.org> > > 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 v2->v3: > - Make the macro actually return the error value if need be, by > putting a single ret; at the end of the macro. (Neat trick from > StackOverflow!) > - Switch the site where I switched mipi_dbi_command() to > mipi_dbi_command_buf() back to what it was. > - Print the failed command in the error message. > - Put the dbi in (parens) since drivers/gpu/drm/tiny/st7586.c was > passing &dbidev->dbi as parameter to mipi_dbi_command() > and this would expand to > struct device *dev = &&dbidev->dbi->spi->dev > which can't be parsed but > struct device *dev = &(&dbidev->dbi)->spi-dev; > should work. I hope. > 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. > --- > include/drm/drm_mipi_dbi.h | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/include/drm/drm_mipi_dbi.h b/include/drm/drm_mipi_dbi.h > index f543d6e3e822..05e194958265 100644 > --- a/include/drm/drm_mipi_dbi.h > +++ b/include/drm/drm_mipi_dbi.h > @@ -183,7 +183,12 @@ 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 %#02x\n", ret, cmd); \ > + ret; \ > }) > > #ifdef CONFIG_DEBUG_FS >
Hi, On Fri, Jul 2, 2021 at 6:58 AM Linus Walleij <linus.walleij@linaro.org> 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> > --- > ChangeLog v2->v3: > - Make the macro actually return the error value if need be, by > putting a single ret; at the end of the macro. (Neat trick from > StackOverflow!) > - Switch the site where I switched mipi_dbi_command() to > mipi_dbi_command_buf() back to what it was. > - Print the failed command in the error message. > - Put the dbi in (parens) since drivers/gpu/drm/tiny/st7586.c was > passing &dbidev->dbi as parameter to mipi_dbi_command() > and this would expand to > struct device *dev = &&dbidev->dbi->spi->dev > which can't be parsed but > struct device *dev = &(&dbidev->dbi)->spi-dev; > should work. I hope. > 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. > --- > include/drm/drm_mipi_dbi.h | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) This seems reasonable to me. There's really no reasonable case I can think of where someone would want to handle the error without it spitting something in the logs. If someone ever comes up with a need for that then we can always add a variant that avoids the logging. ;-) With the commit message fixed as per Noralf: Reviewed-by: Douglas Anderson <dianders@chromium.org>
diff --git a/include/drm/drm_mipi_dbi.h b/include/drm/drm_mipi_dbi.h index f543d6e3e822..05e194958265 100644 --- a/include/drm/drm_mipi_dbi.h +++ b/include/drm/drm_mipi_dbi.h @@ -183,7 +183,12 @@ 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 %#02x\n", ret, cmd); \ + 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> --- ChangeLog v2->v3: - Make the macro actually return the error value if need be, by putting a single ret; at the end of the macro. (Neat trick from StackOverflow!) - Switch the site where I switched mipi_dbi_command() to mipi_dbi_command_buf() back to what it was. - Print the failed command in the error message. - Put the dbi in (parens) since drivers/gpu/drm/tiny/st7586.c was passing &dbidev->dbi as parameter to mipi_dbi_command() and this would expand to struct device *dev = &&dbidev->dbi->spi->dev which can't be parsed but struct device *dev = &(&dbidev->dbi)->spi-dev; should work. I hope. 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. --- include/drm/drm_mipi_dbi.h | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) -- 2.31.1