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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




Den 02.02.2022 11.09, skrev Maxime Ripard:
> 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?
> 

Ok, I'll spin a new version using DT properties.

Noralf.



[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux