Re: [PATCH] drm/panel: novatek-nt36523: transition to mipi_dsi wrapped functions

[neil . armstrong]

On 06/03/2025 18:38, Tejas Vipin wrote:
> On 3/6/25 10:58 PM, Doug Anderson wrote:
> > Hi,
> >
> > On Thu, Mar 6, 2025 at 6:05 AM <neil.armstrong@xxxxxxxxxx> wrote:
> > > On 06/03/2025 14:43, Tejas Vipin wrote:
> > > > Changes the novatek-nt36523 panel to use multi style functions for
> > > > improved error handling.
> > > >
> > > > Signed-off-by: Tejas Vipin <tejasvipin76@xxxxxxxxx>
> > > > ---
> > > >    drivers/gpu/drm/panel/panel-novatek-nt36523.c | 1683 ++++++++---------
> > > >    1 file changed, 823 insertions(+), 860 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/panel/panel-novatek-nt36523.c b/drivers/gpu/drm/panel/panel-novatek-nt36523.c
> > > > index 04f1d2676c78..922a225f6258 100644
> > > > --- a/drivers/gpu/drm/panel/panel-novatek-nt36523.c
> > > > +++ b/drivers/gpu/drm/panel/panel-novatek-nt36523.c
> > > > @@ -23,10 +23,12 @@
> > > >
> > > >    #define DSI_NUM_MIN 1
> > > >
> > > > -#define mipi_dsi_dual_dcs_write_seq(dsi0, dsi1, cmd, seq...)        \
> > > > -             do {                                                 \
> > > > -                     mipi_dsi_dcs_write_seq(dsi0, cmd, seq);      \
> > > > -                     mipi_dsi_dcs_write_seq(dsi1, cmd, seq);      \
> > > > +#define mipi_dsi_dual_dcs_write_seq_multi(dsi_ctx0, dsi_ctx1, cmd, seq...)      \
> > > > +             do {                                                            \
> > > > +                     mipi_dsi_dcs_write_seq_multi(&dsi_ctx0, cmd, seq);      \
> > > > +                     dsi_ctx1.accum_err = dsi_ctx0.accum_err;                \
> > > > +                     mipi_dsi_dcs_write_seq_multi(&dsi_ctx1, cmd, seq);      \
> > > > +                     dsi_ctx0.accum_err = dsi_ctx1.accum_err;                \
> > >
> > > Just thinking out loud, but can't we do :
> > >
> > > struct mipi_dsi_multi_context dsi_ctx = { .dsi = NULL };
> > >
> > > #define mipi_dsi_dual_dcs_write_seq_multi(dsi_ctx, dsi0, dsi1, cmd, seq...)      \
> > >                  do {
> > >                          dsi_ctx.dsi = dsi0;                                     \
> > >                          mipi_dsi_dcs_write_seq_multi(&dsi_ctx, cmd, seq);       \
> > >                          dsi_ctx.dsi = dsi1;                                     \
> > >                          mipi_dsi_dcs_write_seq_multi(&dsi_ctx, cmd, seq);       \
> > >
> > > ?
> > >
> > > So we have a single accum_err.
> >
> > Even though the code you used was what I suggested in IRC, I like
> > Neil's suggestion better here. What do you think?
>
> I like Dmitry's suggestion [1]. If we went ahead with this we'd also
> only need to equate the accum_err for the few msleep calls. Since it
> does change the behavior, I'd like to hear another opinion on it before
> I go ahead with it.

As noted by Douglas, my solution doesn't change the current behaviour,
and I don't feel it's fine to leave one of the 2 DSI links fully configure
while stopping on error on the other one, it will lead to incomplete situations
and the debug purpose is not really acceptable, either it works or it fails entirely.
Leaving an half configured panel can damage it on some OLED like technologies.

Neil

> [1]: https://lore.kernel.org/all/p2esqngynwfrshz5rqfnmx6qgwf4dclpkb3mphwg2vavx2jbcg@clqoy5tjh7bb/
>
> > Other than that, it looks good to me.
> >
> > -Doug