Re: [PATCH 2/2] drm/panel: Add Samsung S6E3FA2 DSI panel driver

[Linus Walleij <linus.walleij@linaro.org>]

 On Sun, Jul 25, 2021 at 5:01 PM Sam Ravnborg <sam@ravnborg.org> wrote:

> This driver supports two different panels:
>
>         S6E3FA2
>         EA8064G
>
> They differ on a lot of the tables and requires different init.
> In other words there is only a little boiler plate code that is in
> common.
>
> I think it would be much cleaner with individual drivers for each panel.

Sometimes Samsung have different *physical* panels connected
to the same display controller, but I don't know what is the case
here. This looks like it could actually be two different display
controllers. (I don't like these opaque binary drops from Samsung,
datasheets would be nice...)

What I think is most intuitive is to have one driver per display controller.
If the two drivers are writing some very similar registers with very
similar values they are probably the same display controller.

If they are not then they are not...

If they are obviously the same display controller I think parameterizing
a display controller driver along the line of panel-novatek-nt35510.c
is the best. If different display controllers, we need different drivers.

> Which brings me to next topic - this is two different panels and the DT
> are supports to describe the HW - so the DT tree should have different
> entries depending on the actual panel. As it is now you try to hide the
> fact that one compatible describes two different panels.

(...)
> > +     ret = s6e3fa2_dsi_dcs_read1(dsi, MCS_READ_ID1, &id1);
> > +     if (ret < 0)
> > +             return ret;
> > +     ret = s6e3fa2_dsi_dcs_read1(dsi, MCS_READ_ID2, &id2);
> > +     if (ret < 0)
> > +             return ret;
> > +     ret = s6e3fa2_dsi_dcs_read1(dsi, MCS_READ_ID3, &id3);
> > +     if (ret < 0)
> > +             return ret;
> > +
> > +     lcd_id = id1 << 16 | id2 << 8 | id3;
> > +
> > +     switch (lcd_id) {
> > +     case LCD_ID_S6E3FA2:
> > +             dev_info(&dsi->dev, "detected S6E3FA2 panel (ID: 0x%x)\n",
> > +                      lcd_id);
> > +             ctx->subtype = PANEL_S6E3FA2;
> > +             ctx->seq_data = &seqdata_s6e3fa2;
> > +             break;
> > +     case LCD_ID_EA8064G:
> > +             dev_info(&dsi->dev, "detected EA8064G panel (ID: 0x%x)\n",
> > +                      lcd_id);
> > +             ctx->subtype = PANEL_EA8064G;
> > +             ctx->seq_data = &seqdata_ea8064g;
> > +             break;
> > +     default:
> > +             dev_warn(&dsi->dev, "unsupported panel ID: 0x%x\n", lcd_id);
> > +             ctx->subtype = PANEL_UNKNOWN;

This does look like two different panels, I'd like to know the MTP
IDs printed (also wrote in different mail). The MTP print I think
should be kept.

Yours,
Linus Walleij