Re: [PATCH v7 2/2] drm: panel: add support for the Renesas R63419 based dual-DSI video mode Display Panels

[Doug Anderson]

Hi,

On Fri, Jun 5, 2026 at 7:51 AM Neil Armstrong <neil.armstrong@xxxxxxxxxx> wrote:
>
> From: KancyJoe <kancy2333@xxxxxxxxxxx>
>
> Implement support for the Renesas 63419 based dual-DSI video mode
> Display Panels found in the Ayaneo gaming handled devices.
>
> Signed-off-by: KancyJoe <kancy2333@xxxxxxxxxxx>

I notice "Kancy Joe" has a space in the source files, but not in the
signoff. I guess Signed-off-by isn't necessarily required to be real
names these days, but still seems odd...


> +/*
> + * Helper to switch between DSI links, so we share a single dsi_ctx
> + * for both links, so in case of an error all writes & sleep for
> + * both links are ignored.
> + */
> +static inline void dsi_link_switch(struct renesas_r63419_panel *ctx,
> +                                  struct mipi_dsi_multi_context *dsi_ctx,
> +                                  unsigned int link)
> +{
> +       dsi_ctx->dsi = ctx->dsi[link];
> +}
> +
> +static int renesas_r63419_on(struct renesas_r63419_panel *ctx)
> +{
> +       struct mipi_dsi_multi_context dsi_ctx = { 0 };
> +
> +       /* Panel registers are loaded from DDIC Non Volatile Memory */
> +
> +       dsi_link_switch(ctx, &dsi_ctx, 0);
> +       mipi_dsi_dcs_exit_sleep_mode_multi(&dsi_ctx);
> +       dsi_link_switch(ctx, &dsi_ctx, 1);
> +       mipi_dsi_dcs_exit_sleep_mode_multi(&dsi_ctx);

Instead of dsi_link_switch(), can't you use the mipi_dsi_dual()
function? I think it would be:

mipi_dsi_dual(mipi_dsi_dcs_exit_sleep_mode_multi, dsi_ctx,
ctx->dsi[0], ctx->dsi[1]);


> +static int renesas_r63419_disable(struct drm_panel *panel)
> +{
> +       struct renesas_r63419_panel *ctx = to_renesas_r63419_panel(panel);
> +       struct mipi_dsi_multi_context dsi_ctx = { 0 };
> +
> +       dsi_link_switch(ctx, &dsi_ctx, 0);
> +       mipi_dsi_dcs_set_display_off_multi(&dsi_ctx);
> +       dsi_link_switch(ctx, &dsi_ctx, 1);
> +       mipi_dsi_dcs_set_display_off_multi(&dsi_ctx);
> +       mipi_dsi_msleep(&dsi_ctx, 50);
> +
> +       dsi_link_switch(ctx, &dsi_ctx, 0);
> +       mipi_dsi_dcs_enter_sleep_mode_multi(&dsi_ctx);
> +       dsi_link_switch(ctx, &dsi_ctx, 1);
> +       mipi_dsi_dcs_enter_sleep_mode_multi(&dsi_ctx);
> +       mipi_dsi_msleep(&dsi_ctx, 120);
> +
> +       return dsi_ctx.accum_err;

I'm not sure we've been terribly consistent, but should the above be
"return 0"? I'm not actually sure there's any benefit to a panel's
disable() function returning an error to begin with.
drm_panel_disable() doesn't return an error, so all this does is skip
setting "panel->enabled" to false and make it harder for the system to
recover.


> +static int renesas_r63419_prepare(struct drm_panel *panel)
> +{
> +       struct renesas_r63419_panel *ctx = to_renesas_r63419_panel(panel);
> +       int ret;
> +
> +       ret = regulator_bulk_enable(ARRAY_SIZE(renesas_r63419_vdd_supplies),
> +                                   ctx->vdd_supplies);
> +       if (ret < 0)
> +               return ret;
> +
> +       usleep_range(1000, 2000);
> +
> +       ret = regulator_bulk_enable(ARRAY_SIZE(renesas_r63419_vcc_supplies),
> +                                   ctx->vcc_supplies);
> +       if (ret < 0) {
> +               regulator_bulk_disable(ARRAY_SIZE(renesas_r63419_vdd_supplies),
> +                                      ctx->vdd_supplies);
> +               return ret;
> +       }
> +
> +       usleep_range(1000, 2000);
> +
> +       gpiod_set_value_cansleep(ctx->reset_gpio, 0);
> +
> +       usleep_range(3000, 4000);
> +
> +       ret = renesas_r63419_on(ctx);
> +       if (ret < 0) {
> +               dev_err(panel->dev, "Failed to initialize panel: %d\n", ret);
> +
> +               /* Power off sequence from the r63419 datasheet */
> +               regulator_bulk_disable(ARRAY_SIZE(renesas_r63419_vcc_supplies),
> +                                      ctx->vcc_supplies);
> +               regulator_bulk_disable(ARRAY_SIZE(renesas_r63419_vdd_supplies),
> +                                      ctx->vdd_supplies);
> +
> +               gpiod_set_value_cansleep(ctx->reset_gpio, 1);

To make de-init opposite to init, shouldn't the reset come before you
turn the regulators off? Depending on the design of the panel, I'd
imagine this could prevent back-powering some logic?

I'd also expect vdd supplies to be turned off first?


> +static int renesas_r63419_unprepare(struct drm_panel *panel)
> +{
> +       struct renesas_r63419_panel *ctx = to_renesas_r63419_panel(panel);
> +
> +       /* Power off sequence from the r63419 datasheet */
> +       regulator_bulk_disable(ARRAY_SIZE(renesas_r63419_vcc_supplies), ctx->vcc_supplies);
> +       regulator_bulk_disable(ARRAY_SIZE(renesas_r63419_vdd_supplies), ctx->vdd_supplies);
> +
> +       gpiod_set_value_cansleep(ctx->reset_gpio, 1);

Similar: shouldn't the reset come before the regulators to make
power-off the opposite of init.


> +static int renesas_r63419_get_modes(struct drm_panel *panel,
> +                                   struct drm_connector *connector)
> +{
> +       struct renesas_r63419_panel *ctx = to_renesas_r63419_panel(panel);
> +       const struct drm_display_mode *mode = ctx->desc->mode;
> +
> +       drm_connector_set_panel_orientation(connector, ctx->orientation);

IIRC, the above was a workaround that caused a warning splat. Is your
panel used on a system that actually needs it? Could your DRM driver
be fixed rather than persisting this hack? For context, see commit
47bef230225b ("drm/panel: panel-edp: Implement .get_orientation
callback")


> +static int renesas_r63419_probe(struct mipi_dsi_device *dsi)
> +{
> +       struct mipi_dsi_device_info info = { };
> +       struct device *dev = &dsi->dev;
> +       struct renesas_r63419_panel *ctx;
> +       struct device_node *dsi1_node;
> +       struct mipi_dsi_host *dsi1_host;
> +       int ret, i;
> +
> +       ctx = devm_drm_panel_alloc(dev, struct renesas_r63419_panel, panel,
> +                                  &renesas_r63419_panel_funcs, DRM_MODE_CONNECTOR_DSI);
> +       if (IS_ERR(ctx))
> +               return PTR_ERR(ctx);
> +
> +       ctx->desc = of_device_get_match_data(dev);
> +       if (!ctx->desc)
> +               return dev_err_probe(dev, -ENODEV,
> +                                    "Failed to get panel description\n");
> +
> +       ret = devm_regulator_bulk_get_const(&dsi->dev,
> +                                           ARRAY_SIZE(renesas_r63419_vdd_supplies),
> +                                           renesas_r63419_vdd_supplies, &ctx->vdd_supplies);
> +       if (ret < 0)
> +               return ret;
> +
> +       ret = devm_regulator_bulk_get_const(&dsi->dev,
> +                                           ARRAY_SIZE(renesas_r63419_vcc_supplies),
> +                                           renesas_r63419_vcc_supplies, &ctx->vcc_supplies);
> +       if (ret < 0)
> +               return ret;

It seems like both sets of supplies are always enabled / disabled
together with no delay between them. Do you truly need two lists, or
can this be combined to one list of regulators. That would simplify a
bunch of logic.


> +       ctx->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
> +       if (IS_ERR(ctx->reset_gpio))
> +               return dev_err_probe(dev, PTR_ERR(ctx->reset_gpio),
> +                                    "Failed to get reset gpio\n");
> +
> +       /* Get second DSI host */
> +       dsi1_node = of_graph_get_remote_node(dsi->dev.of_node, 1, -1);
> +       if (!dsi1_node)
> +               return dev_err_probe(dev, -ENODEV,
> +                                    "Failed to get remote node for second DSI\n");
> +
> +       dsi1_host = of_find_mipi_dsi_host_by_node(dsi1_node);
> +       of_node_put(dsi1_node);
> +       if (!dsi1_host)
> +               return dev_err_probe(dev, -EPROBE_DEFER,
> +                                    "Failed to find second DSI host\n");
> +
> +       /* Copy current DSI info, do not provide OF node since no driver needs to be attached */
> +       strscpy(info.type, dsi->name, sizeof(info.type));

Can't you use the two-argument form of strscpy()?

FWIW, I also notice that the Sashiko AI bot had some comments. Did you
already look all of those over and decide they don't need fixing? I
have a vague recollection that there's no need to worry about someone
calling disable() and then enable() without going through the
unprepare() / prepare(). If my memory is correct, I guess that would
be nice to document... I didn't analyze some of the other claims that
the AI bot had.


-Doug