Re: [PATCH] drm/panel-edp: Add timings for BOE NV133WUM-N63

[Clayton Craft <clayton@craftyguy.net>]

[-- Attachment #1: Type: text/plain, Size: 1824 bytes --]

On Mon, 30 Oct 2023 13:58:31 -0700 Jessica Zhang <quic_jesszhan@quicinc.com> wrote:
> 
> 
> On 10/27/2023 7:19 PM, Clayton Craft wrote:
> > This panel is found on laptops e.g., variants of the Thinkpad X13s.
> > Configuration was collected from the panel's EDID.
> 
> Hi Clayton,
> 
> Thanks for the patch -- it looks good to me aside from one minor comment 
> below.
> 
> > 
> > Signed-off-by: Clayton Craft <clayton@craftyguy.net>
> > ---
> >   drivers/gpu/drm/panel/panel-edp.c | 27 +++++++++++++++++++++++++++
> >   1 file changed, 27 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/panel/panel-edp.c b/drivers/gpu/drm/panel/panel-edp.c
> > index 95c8472d878a..5db283f014f3 100644
> > --- a/drivers/gpu/drm/panel/panel-edp.c
> > +++ b/drivers/gpu/drm/panel/panel-edp.c
> > @@ -1204,6 +1204,29 @@ static const struct panel_desc boe_nv133fhm_n61 = {
> >   	},
> >   };
> >   
> > +static const struct drm_display_mode boe_nv133wum_n63_modes = {
> 
> Will other modes be added to this struct in the future? If so, I think 
> we can probably turn this into an array to make it easier to extend.

To be honest, I have no idea. I saw a kernel backtrace in dmesg that was related
to this driver not having config for my panel, so I added it after looking at
similar examples here.

Personally I'd prefer not to get too fancy with arrays and such until there's
actually a need for it, e.g., someone comes along with more modes to add. But I
can try to do that if my "keep it simple until it needs to be extended" approach
is unacceptable :)

> Otherwise, can you change the name to "*_mode"? Seems to me that almost 
> all other panels with a single mode have the name as "*_mode" with the 
> only exception being a carry-over from older panel-simple.c code.

Ok I'll rename to *_mode in V2. Thanks for reviewing!

-Clayton

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]