Re: [PATCH v1] drm/panel-edp: Add panels with conservative timings

[Terry Hsiao <terry_hsiao@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxx>]

Hi Doug,

Thank you for your reply.
The patch has been modified and will be sent to you shortly.

The timings are set based on the panel datasheets in IssueTracker
B116XTN02.3: B116XTN02.3(HW 9A)_HP_ Functional Spec_0617Y24.pdf
B116XAN06.1: B116XAN06.1_7A_HP_ Final Functional Spec 0617Y24.pdf
B116XAT04.1: B116XAT04.1 HW 0 A(HH)_ Pre Functional Spec_HP_ 0425Y24.pdf
NV116WHM-A4D: NV116WHM-A4D V8.0 Teacake  Product Specification-20240416.pdf
N116BCA-EA2: Approval Specification N116BCA-EA2_C3_20231212.pdf

N116BCP-EA2: TFT-LCD Tentative N116BCP-EA2 C2 for HP Ver 0.2-240502.pdf

On page 24 of the N116BCP-EA2 datasheet, the value for t9 as disable is "null". 

If I have misunderstood what you mean, please correct me.

Thank you,

On Wed, Jul 17, 2024 at 10:58 PM Doug Anderson <dianders@xxxxxxxxxxxx> wrote:
>
> Hi,
>
> On Wed, Jul 17, 2024 at 2:39 AM Terry Hsiao
> <terry_hsiao@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxx> wrote:
> >
> > The 6 panels are used on Mediatek MT8186 Chromebooks
> > - B116XAT04.1  (06AF/B4C4)
> > - NV116WHM-A4D (09E5/FA0C)
> > - N116BCP-EA2  (0DAE/6111)
> > - B116XTN02.3  (06AF/AA73)
> > - B116XAN06.1  (06AF/99A1)
> > - N116BCA-EA2  (0DAE/5D11)
> >
> > Signed-off-by: Terry Hsiao <terry_hsiao@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxx>
> > ---
> >  drivers/gpu/drm/panel/panel-edp.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
>
> Please resend with a better patch subject, like "drm/panel-edp: Add 6
> panels used by MT8186 Chromebooks".
>
> Also: are you adding timings based on the datasheets, or are you just
> guessing here? The previous patches that added "conservative" timings
> were because the Chromebooks involved were really old and couldn't be
> tracked down and folks couldn't find the relevant datasheets. In the
> case of MT8188 I'd expect you to be adding timings based on the
> datasheets. Please confirm that you are.
>
> If possible, it's really nice to have the raw EDIDs for the panels in
> the commit message in case someone needs it later.
>
>
> > diff --git a/drivers/gpu/drm/panel/panel-edp.c b/drivers/gpu/drm/panel/panel-edp.c
> > index f85a6404ba58..ac280607998f 100644
> > --- a/drivers/gpu/drm/panel/panel-edp.c
> > +++ b/drivers/gpu/drm/panel/panel-edp.c
> > @@ -1845,8 +1845,11 @@ static const struct edp_panel_entry edp_panels[] = {
> >         EDP_PANEL_ENTRY('A', 'U', 'O', 0x635c, &delay_200_500_e50, "B116XAN06.3"),
> >         EDP_PANEL_ENTRY('A', 'U', 'O', 0x639c, &delay_200_500_e50, "B140HAK02.7"),
> >         EDP_PANEL_ENTRY('A', 'U', 'O', 0x723c, &delay_200_500_e50, "B140XTN07.2"),
> > +       EDP_PANEL_ENTRY('A', 'U', 'O', 0x73aa, &delay_200_500_e50, "B116XTN02.3"),
> >         EDP_PANEL_ENTRY('A', 'U', 'O', 0x8594, &delay_200_500_e50, "B133UAN01.0"),
> >         EDP_PANEL_ENTRY('A', 'U', 'O', 0xd497, &delay_200_500_e50, "B120XAN01.0"),
> > +       EDP_PANEL_ENTRY('A', 'U', 'O', 0xa199, &delay_200_500_e50, "B116XAN06.1"),
>
> Please keep this sorted. For instance, 0xa199 should come _before_
> 0xd497, right?
>
>
> > +       EDP_PANEL_ENTRY('A', 'U', 'O', 0xc4b4, &delay_200_500_e50, "B116XAT04.1"),
> >         EDP_PANEL_ENTRY('A', 'U', 'O', 0xf390, &delay_200_500_e50, "B140XTN07.7"),
> >
> >         EDP_PANEL_ENTRY('B', 'O', 'E', 0x0607, &delay_200_500_e200, "Unknown"),
> > @@ -1901,6 +1904,7 @@ static const struct edp_panel_entry edp_panels[] = {
> >         EDP_PANEL_ENTRY('B', 'O', 'E', 0x0b56, &delay_200_500_e80, "NT140FHM-N47"),
> >         EDP_PANEL_ENTRY('B', 'O', 'E', 0x0c20, &delay_200_500_e80, "NT140FHM-N47"),
> >         EDP_PANEL_ENTRY('B', 'O', 'E', 0x0cb6, &delay_200_500_e200, "NT116WHM-N44"),
> > +       EDP_PANEL_ENTRY('B', 'O', 'E', 0x0cfa, &delay_200_500_e50, "NV116WHM-A4D"),
> >
> >         EDP_PANEL_ENTRY('C', 'M', 'N', 0x1130, &delay_200_500_e50, "N116BGE-EB2"),
> >         EDP_PANEL_ENTRY('C', 'M', 'N', 0x1132, &delay_200_500_e80_d50, "N116BGE-EA2"),
> > @@ -1916,8 +1920,10 @@ static const struct edp_panel_entry edp_panels[] = {
> >         EDP_PANEL_ENTRY('C', 'M', 'N', 0x1156, &delay_200_500_e80_d50, "Unknown"),
> >         EDP_PANEL_ENTRY('C', 'M', 'N', 0x1157, &delay_200_500_e80_d50, "N116BGE-EA2"),
> >         EDP_PANEL_ENTRY('C', 'M', 'N', 0x115b, &delay_200_500_e80_d50, "N116BCN-EB1"),
> > +       EDP_PANEL_ENTRY('C', 'M', 'N', 0x115d, &delay_200_500_e80_d50, "N116BCA-EA2"),
> >         EDP_PANEL_ENTRY('C', 'M', 'N', 0x115e, &delay_200_500_e80_d50, "N116BCA-EA1"),
> >         EDP_PANEL_ENTRY('C', 'M', 'N', 0x1160, &delay_200_500_e80_d50, "N116BCJ-EAK"),
> > +       EDP_PANEL_ENTRY('C', 'M', 'N', 0x1161, &delay_200_500_e80, "N116BCP-EA2"),
>
> It looks suspicious that all the panels around this one need 50 ms for
> disable but yours doesn't. While it's certainly possible that things
> changed for this panel, it's worth double-checking.
>
> -Doug