On Mon, 30 Oct 2023 13:58:31 -0700 Jessica Zhang <[email protected]> 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 <[email protected]> > > --- > > 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
signature.asc
Description: PGP signature
