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

[Neil Armstrong]

On 6/18/26 19:00, Doug Anderson wrote:
> 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...

Yeah I kept all the signoff and names as in the the source files,
it wasn't an issue for other patches, so I left it....

> > +/*
> > + * 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]);

Right, indeed will switch

> > +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.

Yep you're right, there's no sense to return the error here.

> > +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?

I did follow the DDIC spec here, and yeah Sashiko already
pointed it to me but reset needs to be switched to low _after_
the vdd supplies goes low. Perhaps indeed the vcc one should be
turned off after the reset. I'll do that.

> > +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.

Yep I'll move vcc disable after the reset to match the spec more closely.

> > +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")

No need, I'll drop it

> > +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.

You need a delay between enabling vcc and vdd according to the spec,
this is why I did a split.

> > +       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()?

Yeah sure

> 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.

Yep I fixed the real issues, the remaining issues are about the init
sequence and some impossible init sequence between drm and dsi.

Thanks,
Neil

> -Doug