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

From: "Noralf Trønnes" <noralf@tronnes.org>
To: Sam Ravnborg <sam@ravnborg.org>
Cc: robh+dt@kernel.org, thierry.reding@gmail.com, maxime@cerno.tech,
	dave.stevenson@raspberrypi.com, david@lechnology.com,
	devicetree@vger.kernel.org, dri-devel@lists.freedesktop.org
Subject: Re: [PATCH v2 3/3] drm/panel: Add MIPI DBI compatible SPI driver
Date: Thu, 27 Jan 2022 21:26:29 +0100	[thread overview]
Message-ID: <d7bc7ab2-29bc-12bc-db9c-466592addfc1@tronnes.org> (raw)
In-Reply-To: <YfL5ptT3Kw1ohC/1@ravnborg.org>

Den 27.01.2022 20.59, skrev Sam Ravnborg:
> 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!
> 

Yes I really hope we can find a way to get this accepted.

>> +
>> +	/*
>> +	 * 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.
> 

Parsing text files in the kernel sounds very scary, not something that I
would like to try.

I will make a script that generates and parses the binary representation
(which is big endian so it's somewhat readable with xxd or the like).
There's a wiki link in the MAINTAINERS entry that will have info about
the format including the script. It will also serve as a place to share
config snippets/script incantations for displays.

I will make the script when the file format is decided upon. Here's the
hack I currently use:
https://gist.github.com/notro/3ca61c48e7dcc4a0ef34dbadbc30bfa5

Noralf.