[PATCH V4 3/3] drm/panel: Add NewVision NV3051D MIPI-DSI LCD panel

[Linus Walleij]

Hi Chris,

thanks for your patch!

On Fri, Oct 28, 2022 at 10:50 PM Chris Morgan <macroalpha82 at gmail.com> wrote:

> From: Chris Morgan <macromorgan at hotmail.com>
>
> Support NewVision NV3051D panels as found on the Anbernic RG353P and
> RG353V. The underlying LCD part number for the RG353x devices is
> unknown, so the device name and a fallback for the driver IC is
> used instead.
>
> Signed-off-by: Chris Morgan <macromorgan at hotmail.com>

(...)
> +struct panel_nv3051d {
> +       struct device *dev;
> +       struct drm_panel panel;
> +       struct gpio_desc *reset_gpio;
> +       const struct nv3051d_panel_info *panel_info;
> +       struct regulator *vdd;
> +       bool prepared;

I think you want to get rid of prepared. The framework keeps track of state.

> +#define dsi_dcs_write_seq(dsi, cmd, seq...) do {                       \
> +               static const u8 b[] = { cmd, seq };                     \
> +               int ret;                                                \
> +               ret = mipi_dsi_dcs_write_buffer(dsi, b, ARRAY_SIZE(b)); \
> +               if (ret < 0)                                            \
> +                       return ret;                                     \
> +       } while (0)

This is a reimplementation of mipi_dsi_dcs_write_seq() so use that
instead. (Found in include/drm/drm_mipi_dsi.h)

> +       /*
> +        * Init sequence was supplied by device vendor with no
> +        * documentation.
> +        */

Grrrr but not your fault. What *can* work is to look in related
datasheets and infer some of the magic variables from there.
Anyways best effort is best effort and as long as it's working
that's OK.

An experiment you can do if you have time is to see if you can read out
MTP ID from the panel (3 bytes, check how other drivers do it)
and use that to figure out who manufactured the actual display
controller. It's a bit of sleuthing.

> +       ctx->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
> +       if (IS_ERR(ctx->reset_gpio)) {
> +               dev_err(dev, "cannot get reset gpio\n");
> +               return PTR_ERR(ctx->reset_gpio);
> +       }

I usually get it with GPIOD_OUT_HIGH so that reset i asserted at probe().

You do have active low flagged in your DTS do you not?

Yours,
Linus Walleij