Re: [PATCH v2 3/3] drm/panel: Add MIPI DBI compatible SPI driver

[Sam Ravnborg <sam@ravnborg.org>]

Hi Noralf,

On Tue, Jan 25, 2022 at 06:57:00PM +0100, Noralf Trønnes wrote:
> Add a driver that will work with most MIPI DBI compatible SPI panels.
> This avoids adding a driver for every new MIPI DBI compatible controller
> that is to be used by Linux. The 'compatible' Device Tree property with
> a '.bin' suffix will be used to load a firmware file that contains the
> controller configuration.

Loading a configuration from a firmware file is very
elegant - I like.
This will be very useful in a million cases with a lot of small panels!

> +
> +	/*
> +	 * Optional MIPI commands to execute when the display pipeline is enabled.
> +	 * This can be used to configure the display controller.
> +	 *
> +	 * The commands are stored in a byte array with the format:
> +	 *     command, num_parameters, [ parameter, ...], command, ...
> +	 *
> +	 * Some commands require a pause before the next command can be received.
> +	 * Inserting a delay in the command sequence is done by using the NOP command with one
> +	 * parameter: delay in miliseconds (the No Operation command is part of the MIPI Display
> +	 * Command Set where it has no parameters).
> +	 *
> +	 * Example:
> +	 *     command 0x11
> +	 *     sleep 120ms
> +	 *     command 0xb1 parameters 0x01, 0x2c, 0x2d
> +	 *     command 0x29
> +	 *
> +	 * Byte sequence:
> +	 *     0x11 0x00
> +	 *     0x00 0x01 0x78
> +	 *     0xb1 0x03 0x01 0x2c 0x2d
> +	 *     0x29 0x00
> +	 */
Using a binary file that is unreadable when it is first created is less
elegant.
I am sure you have considered a txt file - and I know parsing a txt file
in prone for more errros than parsing a binary file.


But if the text file looked like:
"
	# The is a comment
	cmd 0x11 0x00

	# We need to sleep
	sleepms 120

	# Do something more
	cmd 0xb1 0x03 0x01 0x2c 0x2d
	cmd 0x29 0x00
"

The file is easier to comment (document) and easier to read and
modify.
The suffix could be ".panel" to tell this is something specific for
a panel.
Using lib/parser could make the code somewhat simple but I did not try
to code it myself.

The code you included below for the binary file is very simple,
but you shift the burden to the people who have to create binary files.
And people using obscure displays are not always so good at this stuff.

Sorry that I do not include code to do the above, but let me know if
this would help to convince you.

	Sam - who has been absent due to house renovation and such