Re: [PATCH RFC] drm/panel-edp: add fat warning against adding new panel compatibles

[Dmitry Baryshkov]

On Thu, May 23, 2024 at 08:36:39AM -0700, Doug Anderson wrote:
> Hi,
> 
> On Wed, May 22, 2024 at 3:07 PM Dmitry Baryshkov
> <dmitry.baryshkov@xxxxxxxxxx> wrote:
> >
> > Add a fat warning against adding new panel compatibles to the panel-edp
> > driver. All new users of the eDP panels are supposed to use the generic
> > "edp-panel" compatible device on the AUX bus. The remaining compatibles
> > are either used by the existing DT or were used previously and are
> > retained for backwards compatibility.
> >
> > Suggested-by: Doug Anderson <dianders@xxxxxxxxxxxx>
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
> > ---
> > The following compatibles were never used by the devices supported by
> > the upstream kernel and are a subject to possible removal:
> >
> > - auo,b133han05
> 
> Ack. Looks like this was added by Bjorn but by the time the dts for
> the board (from context, sc8180x-primus) using it made it upstream it
> was using "edp-panel".
> 
> 
> > - auo,b140han06
> 
> Ack. Same as above, but this time the board was "sc8180x-lenovo-flex-5g".
> 
> 
> > - ivo,m133nwf4-r0
> 
> Ack. Also added by Bjorn but not easy to tell from context which board
> it was from. "m133nwf4" never shows up in the history of arm64 qcom
> dts files.
> 
> 
> > - lg,lp097qx1-spa1
> 
> Maybe. Added by Yakir at Rockchip. I don't think this was ChromeOS
> related so I don't have any inside knowledge. Presumably this is for
> some other hardware they were using. Probably worth CCing Rockchip
> mailing list. It's entirely possible that they have downstream dts
> files using this and there's no requirement to upstream dts files that
> I'm aware of.

I see. Adding him to the CC list.

> 
> 
> > - lg,lp129qe
> 
> NAK. See "arch/arm/boot/dts/nvidia/tegra124-venice2.dts"

Yes. I even had a comment next to it. And then blindly posted it here.

> 
> 
> > - samsung,lsn122dl01-c01
> 
> Maybe. Also by Yakir at Rockchip and also doesn't appear to be
> ChromeOS, so you should ask them.
> 
> 
> > - samsung,ltn140at29-301
> 
> NAK. arch/arm/boot/dts/nvidia/tegra124-nyan-blaze.dts
> 
> 
> > - sharp,ld-d5116z01b
> 
> Added by Jeffrey Hugo. Maybe Cc him to make sure it's OK to delete?

I pinged him on IRC.

> 
> 
> > - sharp,lq140m1jw46
> 
> Ack. Feel free to delete. This was used in the sc7280 reference board
> (hoglin/zoglin) and by the time the dts made it upstream it was
> already using generic edp-panel.
> 
> 
> > - starry,kr122ea0sra
> 
> Ack. From my previous notes: "starry,kr122ea0sra - rk3399-gru-gru
> (reference board, not upstream)". Nobody needs this.
> 
> 
> > I'm considering dropping them, unless there is a good reason not to do
> > so.
> > ---
> >  drivers/gpu/drm/panel/panel-edp.c | 18 +++++++++++++++++-
> >  1 file changed, 17 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/panel/panel-edp.c b/drivers/gpu/drm/panel/panel-edp.c
> > index 6db277efcbb7..95b25ec67168 100644
> > --- a/drivers/gpu/drm/panel/panel-edp.c
> > +++ b/drivers/gpu/drm/panel/panel-edp.c
> > @@ -1776,7 +1776,23 @@ static const struct of_device_id platform_of_match[] = {
> >         {
> >                 /* Must be first */
> >                 .compatible = "edp-panel",
> > -       }, {
> > +       },
> > +       /*
> > +        * Do not add panels to the list below unless they cannot be handled by
> > +        * the generic edp-panel compatible.
> > +        *
> > +        * The only two valid reasons are:
> > +        * - because of the panel issues (e.g. broken EDID or broken
> > +        *   identification),
> > +        * - because the platform which uses the panel didn't wire up the AUX
> > +        *   bus properly.
> 
> You mean that they physically didn't wire up the AUX bus? I don't
> think that's actually possible. I don't believe you can use an eDP
> panel without this working. Conceivably a reason to add here is if
> there's some old platform that hasn't coded up support for AUX bus.
> ...but it would be much preferred to update the platform driver to
> support it.

I was thinking about the DT/driver side of the story. Let me rephrase it
in a cleaner way.

-- 
With best wishes
Dmitry