linux-kernel - Re: [PATCH v1 2/2] drm/tinydrm: add driver for ST7735R panels

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <63ca5df1-3908-fcba-9391-ce9ef035086f@lechnology.com>
Date:   Fri, 8 Dec 2017 15:25:09 -0600
From:   David Lechner <david@...hnology.com>
To:     Noralf Trønnes <noralf@...nnes.org>,
        dri-devel@...ts.freedesktop.org, devicetree@...r.kernel.org
Cc:     limor@...yada.net, Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1 2/2] drm/tinydrm: add driver for ST7735R panels

On 12/06/2017 12:27 PM, Noralf Trønnes wrote:
> 
> Den 29.11.2017 04.01, skrev David Lechner:
>> This adds a new driver for Sitronix ST7735R display panels.
>>
>> This has been tested using an Adafruit 1.8" TFT.
>>
>> Signed-off-by: David Lechner <david@...hnology.com>
>> ---
>>   MAINTAINERS                       |   6 +
>>   drivers/gpu/drm/tinydrm/Kconfig   |  10 ++
>>   drivers/gpu/drm/tinydrm/Makefile  |   1 +
>>   drivers/gpu/drm/tinydrm/st7735r.c | 237 
>> ++++++++++++++++++++++++++++++++++++++
>>   4 files changed, 254 insertions(+)
>>   create mode 100644 drivers/gpu/drm/tinydrm/st7735r.c
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index a174632..9c7707e 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -4462,6 +4462,12 @@ S:    Maintained
>>   F:    drivers/gpu/drm/tinydrm/st7586.c
>>   F:    Documentation/devicetree/bindings/display/st7586.txt
>> +DRM DRIVER FOR SITRONIX ST7735R PANELS
>> +M:    David Lechner <david@...hnology.com>
> 
> I know we haven't done this in the other tinydrm drivers, but I think
> we should start adding which tree the development is happening in:
> 
> T:    git git://anongit.freedesktop.org/drm/drm-misc

This is inherited, just like L:, so get_maintainers.pl --scm returns git 
git://anongit.freedesktop.org/drm/drm-misc already. So there doesn't 
seem to be a need to add this line.

> 
>> +S:    Maintained
>> +F:    drivers/gpu/drm/tinydrm/st7735r.c
>> +F:    Documentation/devicetree/bindings/display/st7735r.txt

<snip>

>> +}
>> +
>> +static void st7735r_pipe_disable(struct drm_simple_display_pipe *pipe)
>> +{
>> +    struct tinydrm_device *tdev = pipe_to_tinydrm(pipe);
>> +    struct mipi_dbi *mipi = mipi_dbi_from_tinydrm(tdev);
>> +
> 
> Please use mipi_dbi_pipe_disable() here.
> 
>> +    DRM_DEBUG_KMS("\n");
>> +
>> +    if (!mipi->enabled)
>> +        return;
>> +
>> +    tinydrm_disable_backlight(mipi->backlight);
>> +
>> +    mipi_dbi_command(mipi, MIPI_DCS_SET_DISPLAY_OFF);
> 
> You turn off the panel, have you checked what it looks like if you don't
> turn off backlight (which is optional in this driver)?
> 
> On the displays I have tried this on, all pixels turn white when they're
> not driven, letting backlight through, giving an all white display.
> That's why I have that blanking code in mipi_dbi_pipe_disable() when we
> don't have backlight control and the reason I don't turn off the panel.
> The power savings of not driving the panel is negligible AFAICR.
> 
> If you don't need DISPLAY_OFF, you can just use mipi_dbi_pipe_disable()
> directly as the callback.
> 

I tested this and you are right, it causes the panel to go white when a 
backlight is not specified, so I will just use mipi_dbi_pipe_disable().