Re: [PATCH v5] drm/panel: db7430: Add driver for Samsung DB7430

[Doug Anderson <dianders@chromium.org>]

Hi,

On Thu, Jun 10, 2021 at 3:07 PM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> @@ -0,0 +1,347 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Panel driver for the Samsung LMS397KF04 480x800 DPI RGB panel.
> + * According to the data sheet the display controller is called DB7430.
> + * Found in the Samsung Galaxy Beam GT-I8350 mobile phone.
> + * Linus Walleij <linus.walleij@linaro.org>
> + */
> +#include <drm/drm_modes.h>
> +#include <drm/drm_mipi_dbi.h>

nit that "mipi" sorts before "modes"


> +static int db7430_power_on(struct db7430 *db)
> +{
> +       struct mipi_dbi *dbi = &db->dbi;
> +       int ret;
> +
> +       /* Power up */
> +       ret = regulator_bulk_enable(ARRAY_SIZE(db->regulators),
> +                                   db->regulators);
> +       if (ret) {
> +               dev_err(db->dev, "failed to enable regulators: %d\n", ret);
> +               return ret;
> +       }
> +       msleep(50);
> +
> +       /* Assert reset >=1 ms */
> +       gpiod_set_value_cansleep(db->reset, 1);
> +       usleep_range(1000, 5000);
> +       /* De-assert reset */
> +       gpiod_set_value_cansleep(db->reset, 0);
> +       /* Wait >= 10 ms */
> +       msleep(10);
> +       dev_dbg(db->dev, "de-asserted RESET\n");
> +
> +       /*
> +        * This is set to 0x0a (RGB/BGR order + horizontal flip) in order
> +        * to make the display behave normally. If this is not set the displays
> +        * normal output behaviour is horizontally flipped and BGR ordered. Do
> +        * it twice because the first message doesn't always "take".
> +        */
> +       mipi_dbi_command(dbi, MIPI_DCS_SET_ADDRESS_MODE, 0x0a);

I would still prefer it if there was some type of error checking since
SPI commands can fail and could potentially fail silently. What about
at least this (untested):

#define db7430_dbi_cmd(_db, _cmd, _seq...) \
  do {
    int _ret = mipi_dbi_command(_db->dbi, _cmd, _seq);
    if (_ret)
      dev_warn(_db->dev, "DBI cmd %d failed (%d)\n", _cmd, _ret);
  } while (0)

Then at least you know _something_ will show up in the logs if there's
a transfer failure instead of silence?

If you truly don't want the error checking then I guess I won't
insist, but it feels like the kind of thing that will bite someone
eventually... In any case, I'm happy to add this now (especially since
the DBI stuff is Acked now).

Reviewed-by: Douglas Anderson <dianders@chromium.org>

I presume that you'd commit it to drm-misc yourself?

-Doug