Re: [PATCH v3 9/9] drm/panel: innolux-p079zca: Don't use a table for initting panels

[Linus Walleij]

On Wed, May 1, 2024 at 5:43 PM Douglas Anderson  wrote:

> Consensus on the mailing lists is that panels shouldn't use a table of
> init commands but should instead use init functions. We'll use the
> same concepts as the recently introduced
> mipi_dsi_generic_write_seq_multi() to make this clean/easy and also
> not bloat the driver too much. Measuring before/after this change:
>
> $ scripts/bloat-o-meter \
>   .../before/panel-innolux-p079zca.ko \
>   .../after/panel-innolux-p079zca.ko
> add/remove: 3/2 grow/shrink: 0/1 up/down: 2356/-1944 (412)
> Function old new   delta
> innolux_p097pfg_init   -1772   +1772
> innolux_p097pfg_init.d - 480+480
> innolux_panel_write_multi  - 104+104
> innolux_panel_prepare412 308-104
> .compoundliteral 480   --480
> innolux_p097pfg_init_cmds   1360   -   -1360
> Total: Before=5802, After=6214, chg +7.10%
>
> Note that, unlike some other drivers, we actually make this panel
> driver _bigger_ by using the new functions. This is because the
> innolux-p079zca panel driver didn't have as complex of a table and
> thus the old table was more efficient than the code. The bloat is
> still not giant (only 412 bytes).
>
> Also note that we can't direclty use
> mipi_dsi_generic_write_seq_multi() here because we need to deal with
> the crazy "nop" that this driver sends after all commands. This means
> that we have to write code that is "inspired" by the new macros.
>
> Since we're touching all the tables, let's also convert hex numbers to
> lower case as per kernel conventions.
>
> Signed-off-by: Douglas Anderson 

With the mentioned bugfix:
Reviewed-by: Linus Walleij 

Yours,
Linus Walleij

Re: [PATCH v3 9/9] drm/panel: innolux-p079zca: Don't use a table for initting panels

[Doug Anderson]

Hi,

On Wed, May 1, 2024 at 8:43 AM Douglas Anderson  wrote:
>
> @@ -132,33 +125,9 @@ static int innolux_panel_prepare(struct drm_panel *panel)
> /* p079zca: t4, p097pfg: t5 */
> usleep_range(2, 21000);
>
> -   if (innolux->desc->init_cmds) {
> -   const struct panel_init_cmd *cmds =
> -   innolux->desc->init_cmds;
> -   unsigned int i;
> -
> -   for (i = 0; cmds[i].len != 0; i++) {
> -   const struct panel_init_cmd *cmd = &cmds[i];
> -
> -   err = mipi_dsi_generic_write(innolux->link, cmd->data,
> -cmd->len);
> -   if (err < 0) {
> -   dev_err(panel->dev, "failed to write command 
> %u\n", i);
> -   goto poweroff;
> -   }
> -
> -   /*
> -* Included by random guessing, because without this
> -* (or at least, some delay), the panel sometimes
> -* didn't appear to pick up the command sequence.
> -*/
> -   err = mipi_dsi_dcs_nop(innolux->link);
> -   if (err < 0) {
> -   dev_err(panel->dev, "failed to send DCS nop: 
> %d\n", err);
> -   goto poweroff;
> -   }
> -   }
> -   }
> +   err = innolux->desc->init(innolux);
> +   if (err < 0)
> +   goto poweroff;

FWIW, I happened to notice a bug in the above by code inspection. The
old code checked "if (innolux->desc->init_cmds)" and thus handled
init_cmds being NULL. The new code doesn't handle the init function
being NULL. One of the two panels in this file (which seems to have no
users in mainline) doesn't specify an init sequence.

I'll spin this next week with the extra "if" test.

-Doug