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.
