On Thu, May 21, 2026 at 5:37 PM Doug Anderson <[email protected]> wrote:
> Hi, > > On Sun, May 17, 2026 at 8:43 PM Chintan Patel <[email protected]> > wrote: > > > > @@ -55,13 +51,9 @@ struct nt36672a_panel_desc { > > enum mipi_dsi_pixel_format format; > > unsigned int lanes; > > > > - unsigned int num_on_cmds_1; > > - const struct nt36672a_panel_cmd *on_cmds_1; > > - unsigned int num_on_cmds_2; > > - const struct nt36672a_panel_cmd *on_cmds_2; > > - > > - unsigned int num_off_cmds; > > - const struct nt36672a_panel_cmd *off_cmds; > > + void (*on_init_1)(struct mipi_dsi_multi_context *dsi_ctx); > > + void (*on_init_2)(struct mipi_dsi_multi_context *dsi_ctx); > > + void (*off_init)(struct mipi_dsi_multi_context *dsi_ctx); > > nit: The name "off_init" sounds a little strange to me. Maybe the > three functions could be: > > send_init_cmds_1 > send_init_cmds_2 > send_deinit_cmds > Good point, agreed that off_init is a bit awkward naming-wise. I'll rename these to something along the lines of: send_init_cmds_1 send_init_cmds_2 send_deinit_cmds in the next revision. > ...or something like that? > > > + mipi_dsi_dcs_write_seq_multi(dsi_ctx, 0xFF, 0x22); > > nit: I wouldn't block landing the patch for it, but since we're > touching all this code it would be nice to make things lowercase hex. > 0xff instead of 0xFF. That's the kernel standard, even if there are > some drivers that violate it. > > Make sense - will do in v3. > > > + for (reg = 0x02; reg <= 0x10; reg++) > > + mipi_dsi_dcs_write_var_seq_multi(dsi_ctx, reg, 0x40); > So I was curious about if the "for" loops helped out. They seem to. > > Overall, your change increased the size of the driver by quite a bit, > but I guess that's the price we pay since (when we talked about this > in the past) people really didn't like the table-based approach. Old > driver vs. with your changes: > > $ ./scripts/bloat-o-meter panel-novatek-nt36672a-old.ko > panel-novatek-nt36672a-new.ko > add/remove: 9/3 grow/shrink: 2/5 up/down: 10430/-1712 (8718) > Function old new delta > .Ltmp1 1292 4652 +3360 > $x 1392 4752 +3360 > tianma_fhd_video_on_init_1 - 3148 +3148 > tianma_fhd_video_on_init_1.d - 252 +252 > tianma_fhd_video_on_init_2 - 144 +144 > tianma_fhd_video_off_init - 124 +124 > tianma_fhd_video_on_init_2.d - 10 +10 > tianma_fhd_video_off_init.d - 8 +8 > .Ltmp8 - 8 +8 > .Ltmp7 - 8 +8 > .Ltmp6 - 8 +8 > tianma_fhd_video_off_cmds 8 - -8 > tianma_fhd_video_on_cmds_2 10 - -10 > tianma_fhd_video_panel_desc 88 64 -24 > nt36672a_panel_unprepare 244 212 -32 > nt36672a_panel_prepare 340 284 -56 > $d 3555 3331 -224 > tianma_fhd_video_on_cmds_1 562 - -562 > .Ltmp3 804 8 -796 > Total: Before=11206, After=19924, chg +77.80% > > ...but if we hadn't done the "for" loops, it would have been much worse: > > dianders@dianders:v6.6 ((8953acf077cf...))$ ./scripts/bloat-o-meter > panel-novatek-nt36672a-old.ko panel-novatek-nt36672a-new-noloops.ko > add/remove: 9/3 grow/shrink: 4/4 up/down: 18380/-1488 (16892) > Function old new delta > .Ltmp1 1292 7164 +5872 > $x 1392 7264 +5872 > tianma_fhd_video_on_init_1 - 5664 +5664 > tianma_fhd_video_on_init_1.d - 562 +562 > tianma_fhd_video_on_init_2 - 144 +144 > tianma_fhd_video_off_init - 124 +124 > $d 3555 3649 +94 > tianma_fhd_video_on_init_2.d - 10 +10 > tianma_fhd_video_off_init.d - 8 +8 > .Ltmp8 - 8 +8 > .Ltmp7 - 8 +8 > .Ltmp6 - 8 +8 > __UNIQUE_ID_modinfo_411 79 85 +6 > tianma_fhd_video_off_cmds 8 - -8 > tianma_fhd_video_on_cmds_2 10 - -10 > tianma_fhd_video_panel_desc 88 64 -24 > nt36672a_panel_unprepare 244 212 -32 > nt36672a_panel_prepare 340 284 -56 > tianma_fhd_video_on_cmds_1 562 - -562 > .Ltmp3 804 8 -796 > Total: Before=11206, After=28098, chg +150.74% > > ...and even the shortest "for" loop you added made a difference. ;-) > > So I'm good with it, though I'd prefer that you send a new version (if > possible) since the "off_init" really bothers me for some reason. ;-P > > I'll also note that I did some hacky testing to confirm that your > "for" loops do indeed create the same set of commands as we had before > your patch. > Thanks a lot for the detailed review and for running the bloat-o-meter comparisons. Yeah, the code size growth is unfortunately larger than I'd hoped, but the loop-based cleanup at least seems to keep it within something more reasonable. Thanks also for verifying that the generated command sequences still match the original tables. > In any case: > > Reviewed-by: Douglas Anderson <[email protected]> >
