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

[David Lechner]

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@xxxxxxxxxxxxxx>
> > ---
> > Â 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@xxxxxxxxxxxxxx>
>
> 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().