Hi, On Fri, Aug 27, 2021 at 1:24 AM yangcong <yangcong5@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxx> wrote: > > Add driver for BOE tv110c9m-ll3 and Inx hj110iz-01a panel > both of those are 10.95" 1200x2000 panel. Your commit message would be a good place to note design choices you made in your patch. Maybe you might say: Support for these two panels fits in nicely with the existing panel-boe-tv101wum-nl6 driver as suggested by Sam [1]. The main things we needed to handle were: a) These panels need slightly longer delays in two places. Since these new delays aren't much longer, let's just unconditionally increase them for the driver. b) One of these two panels doesn't support DSI HS mode so this patch adds a flag for a panel to disable that. [1] https://lore.kernel.org/r/YSPAseE6WD8dDRuz@xxxxxxxxxxxx/ If you send a new version, maybe you could include prose similar to that? > + _INIT_DCS_CMD(0x4D, 0x21), > + _INIT_DCS_CMD(0x4E, 0x43), > + _INIT_DCS_CMD(0x51, 0x12), > + _INIT_DCS_CMD(0x52, 0x34), > + _INIT_DCS_CMD(0x55, 0x82, 0x02), > + _INIT_DCS_CMD(0x56, 0x04), > + _INIT_DCS_CMD(0x58, 0x21), > + _INIT_DCS_CMD(0x59, 0x30), > + _INIT_DCS_CMD(0x5A, 0xBA), //9A nit: the "//9A" above seems like it's leftover from something. Remove? > + _INIT_DCS_CMD(0x1F, 0xBA),//9A > + _INIT_DCS_CMD(0x20, 0xA0), > + > + _INIT_DCS_CMD(0x26, 0xBA),//9A > + _INIT_DCS_CMD(0x27, 0xA0), > + > + _INIT_DCS_CMD(0x33, 0xBA),//9A > + _INIT_DCS_CMD(0x34, 0xA0), > + > + _INIT_DCS_CMD(0x3F, 0xE0), > + > + _INIT_DCS_CMD(0x40, 0x00), > + > + _INIT_DCS_CMD(0x44, 0x00), > + _INIT_DCS_CMD(0x45, 0x40), > + > + _INIT_DCS_CMD(0x48, 0xBA),//9A > + _INIT_DCS_CMD(0x49, 0xA0), > + > + _INIT_DCS_CMD(0x5B, 0x00), > + _INIT_DCS_CMD(0x5C, 0x00), > + _INIT_DCS_CMD(0x5D, 0x00), > + _INIT_DCS_CMD(0x5E, 0xD0), > + > + _INIT_DCS_CMD(0x61, 0xBA),//9A > + _INIT_DCS_CMD(0x62, 0xA0), More random //9A to remove above? > @@ -515,7 +1363,7 @@ static int boe_panel_unprepare(struct drm_panel *panel) > regulator_disable(boe->pp3300); > } else { > gpiod_set_value(boe->enable_gpio, 0); > - usleep_range(500, 1000); > + usleep_range(1000, 2000); > regulator_disable(boe->avee); > regulator_disable(boe->avdd); > usleep_range(5000, 7000); > @@ -556,7 +1404,7 @@ static int boe_panel_prepare(struct drm_panel *panel) > if (ret < 0) > goto poweroffavdd; > > - usleep_range(5000, 10000); > + usleep_range(10000, 15000); nit: how about using the range 10000, 11000? Last I looked at usleep_range() it almost always ended up at the longer of the two times, so that will shave 4 ms off and get us nearly to where we were without your change. The whole point of the range is to make the system more power efficient for frequent operations (wakeup combining), but that really doesn't matter for something as infrequent as turning on a LCD. Other than nits this looks fine to me and I'd be happy to add my Reviewed-by to a version with nits fixed. I'm not really an expert on MIPI panels but the convention of a big stream of binary commands seems to match what other panels in this driver do, even if their table of binary data isn't quite as long as yours (are all of yours actually needed?). I'm happy to land this in drm-misc-next with Sam or Thierry's Ack, too. -Doug
