Re: [PATCH 5/6] drm/panel: simple: Innolux TV123WAM is actually P120ZDG-BF1

[Doug Anderson]

Hi,

On Thu, Oct 25, 2018 at 11:13 AM Sean Paul <sean@xxxxxxxxxx> wrote:
>
> On Mon, Oct 22, 2018 at 01:46:38PM -0700, Douglas Anderson wrote:
> > As far as I can tell the panel that was added in commit da50bd4258db
> > ("drm/panel: simple: Add Innolux TV123WAM panel driver support")
> > wasn't actually an Innolux TV123WAM but was actually an Innolux
> > P120ZDG-BF1.
> >
> > As far as I can tell the Innolux TV123WAM isn't a real panel and but
> > it's a mosh between the TI TV123WAM and the Innolux P120ZDG-BF1.
> > Let's unmosh.
> >
> > Here's my evidence:
> >
> > * Searching for TV123WAM on the Internet turns up a TI panel.  While
> >   it's possible that an Innolux panel has the same model number as the
> >   TI Panel, it seems a little doubtful.  Looking up the datasheet from
> >   the TI Panel shows that it's 1920 x 1280 and 259.2 mm x 172.8 mm.
> >
> > * As far as I know, the patch adding the Innolux Panel was supposed to
> >   be for the board that's sitting in front of me as I type this
> >   (support for that board is not yet upstream).  On the back of that
> >   panel I see Innolux P120ZDZ-EZ1 rev B1.
> >
> > * Someone pointed me at a datasheet that's supposed to be for the
> >   panel in front of me (sorry, I can't share the datasheet).  That
> >   datasheet has the string "p120zdg-bf1"
> >
> > * If I search for "P120ZDG-BF1" on the Internet I get hits for panels
> >   that are 2160x1440.  They don't have datasheets, but the fact that
> >   the resolution matches is a good sign.
> >
> > In any case, let's update the name and also the physical size to match
> > the correct panel.
> >
> > Fixes: da50bd4258db ("drm/panel: simple: Add Innolux TV123WAM panel driver support")
> > Signed-off-by: Douglas Anderson <dianders@xxxxxxxxxxxx>
> > Cc: Sandeep Panda <spanda@xxxxxxxxxxxxxx>
> > ---
> >
> >  drivers/gpu/drm/panel/panel-simple.c | 14 +++++++-------
> >  1 file changed, 7 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
> > index 937e97490c30..7ee1abc5d81b 100644
> > --- a/drivers/gpu/drm/panel/panel-simple.c
> > +++ b/drivers/gpu/drm/panel/panel-simple.c
> > @@ -1370,7 +1370,7 @@ static const struct panel_desc innolux_n156bge_l21 = {
> >       },
> >  };
> >
> > -static const struct drm_display_mode innolux_tv123wam_mode = {
> > +static const struct drm_display_mode innolux_p120zdg_bf1_mode = {
> >       .clock = 206016,
> >       .hdisplay = 2160,
> >       .hsync_start = 2160 + 48,
> > @@ -1384,13 +1384,13 @@ static const struct drm_display_mode innolux_tv123wam_mode = {
> >       .flags = DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC,
> >  };
> >
> > -static const struct panel_desc innolux_tv123wam = {
> > -     .modes = &innolux_tv123wam_mode,
> > +static const struct panel_desc innolux_p120zdg_bf1 = {
> > +     .modes = &innolux_p120zdg_bf1_mode,
> >       .num_modes = 1,
> >       .bpc = 8,
> >       .size = {
> > -             .width = 259,
> > -             .height = 173,
> > +             .width = 254,
> > +             .height = 169,
> >       },
> >       .delay = {
> >               .prepare = 200,
> > @@ -2454,8 +2454,8 @@ static const struct of_device_id platform_of_match[] = {
> >               .compatible = "innolux,n156bge-l21",
> >               .data = &innolux_n156bge_l21,
> >       }, {
> > -             .compatible = "innolux,tv123wam",
>
> I think we should update the struct, but we might want to keep this around.
> Given the tv123wam panel is TI, we're likely not going to have a collision on
> innolux,...
>
> That said, I'll defer to robh on this one, I'm not sure if changing names is
> cool once the bindings have hit mainline.

Whoops, I missed Rob on this patch--just had him on the bindings one.

...generally I believe Rob seems to be OK with wiping out backward
compatibility for things like this when the previous binding is super
new and there's no evidence that anyone ever used it (like if it was
added for a specific board and that board doesn't have a fully
functional DT anyway).

In this particular case I'm 99.9999% certain nobody is using the
existing binding.  If someone crawls out of the woodwork and says this
patch broke them, it would be trivially easy to add the backward
compatible string later.

Obviously Rob can feel free to correct me if I'm wrong.

I purposely put this patch at the end of the series so we can land the
earlier ones and we can sit on this one for a little while if desired.

-Doug