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

[Maxime Ripard <maxime@cerno.tech>]

[-- Attachment #1: Type: text/plain, Size: 3571 bytes --]

On Thu, Jan 27, 2022 at 06:53:48PM +0100, Noralf Trønnes wrote:
> >> +struct panel_mipi_dbi_config {
> >> +	/* Magic string: panel_mipi_dbi_magic */
> >> +	u8 magic[15];
> >> +
> >> +	/* Config file format version */
> >> +	u8 file_format_version;
> >> +
> >> +	/* Width in pixels */
> >> +	__be16 width;
> >> +	/* Height in pixels */
> >> +	__be16 height;
> >> +
> >> +	/* Width in millimeters (optional) */
> >> +	__be16 width_mm;
> >> +	/* Height in millimeters (optional) */
> >> +	__be16 height_mm;
> >> +
> >> +	/* X-axis panel offset */
> >> +	__be16 x_offset;
> >> +	/* Y-axis panel offset */
> >> +	__be16 y_offset;
> >> +
> >> +	/* 4 pad bytes, must be zero */
> >> +	u8 pad[4];
> >> +
> >> +	/*
> >> +	 * 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
> >> +	 */
> >> +	u8 commands[];
> >> +};
> > 
> > I'm not really a fan of parsing raw data in the kernel. I guess we can't
> > really avoid the introduction of a special case to sleep, but we already
> > have dt properties for all of the other properties (but X and Y offset,
> > maybe?)
> > 
> > Maybe we should use those instead?
>
> I don't understand your reluctance to parsing data, lots of ioctls do
> it.

The reluctance comes from the parsing itself: you need to have input
validation, and it's hard to get right. The less we have, the easier it
gets.

> And this data can only be loaded by root. What I like about having
> these properties in the config file is that the binding becomes a
> fallback binding that can actually be made to work without changing the
> Device Tree.
> 
> For arguments sake let's say tiny/st7735r.c was not built and we had
> this node:
> 
> display@0{
> 	compatible = "jianda,jd-t18003-t01", "sitronix,st7735r",
> "panel-mipi-dbi-spi";
> };
> 
> It will still be possible to use this display without changing the
> Device Tree. Just add a firmware/config file.
> 
> Having the properties in DT it would have to look like this for the
> fallback to work:
> 
> display@0{
> 	compatible = "jianda,jd-t18003-t01", "sitronix,st7735r",
> "panel-mipi-dbi-spi";
> 	panel-timing = {
> 		hactive = <128>;
> 		vactive = <128>;
> 	};
> 	width-mm = <25>;
> 	height-mm = <26>;
> 	x-offset = <2>;
> 	y-offset = <3>;
> };
> 
> Is this important, I'm not sure. What do you think?

Parts of it is ergonomics I guess. We're used to having all those
properties either in the DT or the driver, but here we introduce a new
way that isn't done anywhere else.

And I don't see any real downside to putting it in the DT? It's going to
be in an overlay, under the user's control anyway, right?

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]