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

[David Lechner]

On 12/11/2017 03:18 PM, Noralf TrÃnnes wrote:
> Den 10.12.2017 23.10, 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>
> > ---
>
> > +static void st7735r_pipe_enable(struct drm_simple_display_pipe *pipe,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ struct drm_crtc_state *crtc_state)
> > +{
> > +ÂÂÂ struct tinydrm_device *tdev = pipe_to_tinydrm(pipe);
> > +ÂÂÂ struct mipi_dbi *mipi = mipi_dbi_from_tinydrm(tdev);
> > +ÂÂÂ struct device *dev = tdev->drm->dev;
> > +ÂÂÂ int ret;
> > +ÂÂÂ u8 addr_mode;
> > +
> > +ÂÂÂ DRM_DEBUG_KMS("\n");
> > +
> > +ÂÂÂ mipi_dbi_hw_reset(mipi);
> > +
> > +ÂÂÂ ret = mipi_dbi_command(mipi, MIPI_DCS_SOFT_RESET);
> > +ÂÂÂ if (ret) {
> > +ÂÂÂÂÂÂÂ DRM_DEV_ERROR(dev, "Error sending command %d\n", ret);
> > +ÂÂÂÂÂÂÂ return;
> > +ÂÂÂ }
> > +
> > +ÂÂÂ msleep(150);
> > +
> > +ÂÂÂ mipi_dbi_command(mipi, MIPI_DCS_EXIT_SLEEP_MODE);
> > +ÂÂÂ msleep(500);
> > +
> > +ÂÂÂ mipi_dbi_command(mipi, ST7735R_FRMCTR1, 0x01, 0x2c, 0x2d);
> > +ÂÂÂ mipi_dbi_command(mipi, ST7735R_FRMCTR2, 0x01, 0x2c, 0x2d);
> > +ÂÂÂ mipi_dbi_command(mipi, ST7735R_FRMCTR3, 0x01, 0x2c, 0x2d, 0x01, 
> > 0x2c,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂ 0x2d);
> > +ÂÂÂ mipi_dbi_command(mipi, ST7735R_INVCTR, 0x07);
> > +ÂÂÂ mipi_dbi_command(mipi, ST7735R_PWCTR1, 0xa2, 0x02, 0x84);
> > +ÂÂÂ mipi_dbi_command(mipi, ST7735R_PWCTR2, 0xc5);
> > +ÂÂÂ mipi_dbi_command(mipi, ST7735R_PWCTR3, 0x0a, 0x00);
> > +ÂÂÂ mipi_dbi_command(mipi, ST7735R_PWCTR4, 0x8a, 0x2a);
> > +ÂÂÂ mipi_dbi_command(mipi, ST7735R_PWCTR5, 0x8a, 0xee);
> > +ÂÂÂ mipi_dbi_command(mipi, ST7735R_VMCTR1, 0x0e);
> > +ÂÂÂ mipi_dbi_command(mipi, MIPI_DCS_EXIT_INVERT_MODE);
> > +ÂÂÂ switch (mipi->rotation) {
> > +ÂÂÂ default:
> > +ÂÂÂÂÂÂÂ addr_mode = ST7735R_MX | ST7735R_MY;
> > +ÂÂÂÂÂÂÂ break;
> > +ÂÂÂ case 90:
> > +ÂÂÂÂÂÂÂ addr_mode = ST7735R_MX | ST7735R_MV;
> > +ÂÂÂÂÂÂÂ break;
> > +ÂÂÂ case 180:
> > +ÂÂÂÂÂÂÂ addr_mode = 0;
> > +ÂÂÂÂÂÂÂ break;
> > +ÂÂÂ case 270:
> > +ÂÂÂÂÂÂÂ addr_mode = ST7735R_MY | ST7735R_MV;
> > +ÂÂÂÂÂÂÂ break;
> > +ÂÂÂ }
> > +ÂÂÂ mipi_dbi_command(mipi, MIPI_DCS_SET_ADDRESS_MODE, addr_mode);
> > +ÂÂÂ mipi_dbi_command(mipi, MIPI_DCS_SET_PIXEL_FORMAT,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂ MIPI_DCS_PIXEL_FMT_16BIT);
> > +ÂÂÂ mipi_dbi_command(mipi, ST7735R_GAMCTRP1, 0x0f, 0x1a, 0x0f, 0x18, 
> > 0x2f,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂ 0x28, 0x20, 0x22, 0x1f, 0x1b, 0x23, 0x37, 0x00, 0x07,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂ 0x02, 0x10);
> > +ÂÂÂ mipi_dbi_command(mipi, ST7735R_GAMCTRN1, 0x0f, 0x1b, 0x0f, 0x17, 
> > 0x33,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂ 0x2c, 0x29, 0x2e, 0x30, 0x30, 0x39, 0x3f, 0x00, 0x07,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂ 0x03, 0x10);
> > +ÂÂÂ mipi_dbi_command(mipi, MIPI_DCS_SET_DISPLAY_ON);
> > +
> > +ÂÂÂ msleep(100);
> > +
> > +ÂÂÂ mipi_dbi_command(mipi, MIPI_DCS_ENTER_NORMAL_MODE);
> > +
> > +ÂÂÂ msleep(20);
> > +
> > +ÂÂÂ mipi_dbi_pipe_enable(pipe, crtc_state);
> > +}
> > +
> > +static const struct drm_simple_display_pipe_funcs st7735r_pipe_funcs = {
> > +ÂÂÂ .enableÂÂÂÂÂÂÂ = st7735r_pipe_enable,
> > +ÂÂÂ .disableÂÂÂ = mipi_dbi_pipe_disable,
> > +ÂÂÂ .updateÂÂÂÂÂÂÂ = tinydrm_display_pipe_update,
> > +ÂÂÂ .prepare_fbÂÂÂ = tinydrm_display_pipe_prepare_fb,
> > +};
> > +
> > +static const struct drm_display_mode st7735r_mode = {
> > +ÂÂÂ TINYDRM_MODE(128, 160, 28, 35),
> > +};
>
> This naming talk has made me realise that these should be named after
> the panel since they're not generic controller functions.
> Maybe the mode can be generic, not sure if the physical size can/will
> differ, the resolution is very likely to be the same for all.

Adafruit has a 1.44" 128x128px display [1] with the same controller, so 
the answer is no, the mode cannot be generic.

[1]: https://www.adafruit.com/product/2088
> What's your view on my descision to split the MIPI DBI compatible
> controllers into per controller drivers instead of having one driver
> that supports them all?

My first impression was that I didn't like it so much. But now, I 
actually think that it is a good idea. I think it is a good compromise 
between some boilerplate code in every driver and a driver with so many 
quirks that it is very difficult to work on. It may make sense to 
combine some very similar controllers, but as a rule of thumb, I think 
one driver per controller will work nicely.

> I've been afraid that it will be a very big file with macro definitions
> per controller and enable functions per panel.
>
> This driver has 15 macros and ~80 panel specific lines.
> With one panel supported, half the driver is boilerplate.
> The mi0283qt panel (MIPI DBI compatible) is in the same ballpark.
>
> Adding helpers for panel reset/exit_sleep, for MIPI_DCS_SET_ADDRESS_MODE
> and for display_on/enter_normal/flush could cut it down some more if we
> made one driver to rule them all. Maybe the enable function per panel
> could be cut in half that way.
>
> I'm starting to wonder if one driver isn't such a bad solution after all...

I think as we add more drivers, the parts that get duplicated will 
become obvious candidates for helper functions to refactor, but I don't 
think trying to cram everything all into one driver is such a good idea.