Hi Sam,
On Wed, Jul 10, 2019 at 09:48:55AM +0200, Sam Ravnborg wrote:
> Hi Laurent.
>
> This driver looks very good.
>
> On Sun, Jul 07, 2019 at 09:19:00PM +0300, Laurent Pinchart wrote:
> > This panel is used on the OpenMoko Neo FreeRunner and Neo 1973.
>
> Add info in Kconfig help entry?
>
> > +config DRM_PANEL_TPO_TD028TTEC1
> > + tristate "TPO TD028TTEC1 panel driver"
>
> Maybe spell out TPO like "TPO (Topology) TD028..."
>
> > + depends on OF && SPI
> > + depends on BACKLIGHT_CLASS_DEVICE
> > + help
> > + Say Y here if you want to enable support for TPO TD028TTEC1 480x640
> > + 2.8" panel.
> > +
> > config DRM_PANEL_TPO_TPG110
> > tristate "TPO TPG 800x400 panel"
> > depends on OF && SPI && GPIOLIB
> > obj-$(CONFIG_DRM_PANEL_TRULY_NT35597_WQXGA) += panel-truly-nt35597.o
> > diff --git a/drivers/gpu/drm/panel/panel-tpo-td028ttec1.c b/drivers/gpu/drm/panel/panel-tpo-td028ttec1.c
> > new file mode 100644
> > index 000000000000..05af9ea6339c
> > --- /dev/null
> > +++ b/drivers/gpu/drm/panel/panel-tpo-td028ttec1.c
> > +
> > +static int jbt_ret_write_0(struct td028ttec1_device *lcd, u8 reg, int *err)
> > +{
> > + struct spi_device *spi = lcd->spi;
> > + u16 tx_buf = JBT_COMMAND | reg;
> > + int ret;
> > +
> > + if (err && *err)
> > + return *err;
> > +
> > + ret = spi_write(spi, (u8 *)&tx_buf, sizeof(tx_buf));
> > + if (ret < 0) {
> > + dev_err(&spi->dev, "%s: SPI write failed: %d\n", __func__, ret);
> > + if (err)
> > + *err = ret;
> > + }
> > +
> > + return ret;
> > +}
>
> I like the way *err is used here.
> So if one call fails, the remaining calls are ignored.
I think it's a construct that could be used in more drivers, when
dealing with large sequences of writes.
> The way the code is written above it will only work on a little endian
> box, as the values are stored in an u16 that is later seen as an array of
> bytes.
> This is also true for the remaing similar functions and may be OK.
> We do not see any real demands for big endian anyway.
This could be fixed if desired, but I would do so on top of this patch
to minimise the changes compared to the omapdrm-specific panel driver.
> > +static int td028ttec1_enable(struct drm_panel *panel)
> > +{
> > + struct td028ttec1_device *lcd = to_td028ttec1_device(panel);
> > + unsigned int i;
> > + int ret = 0;
> > +
> > + /* Three times command zero */
> > + for (i = 0; i < 3; ++i) {
> > + jbt_ret_write_0(lcd, 0x00, &ret);
> > + usleep_range(1000, 2000);
> > + }
> > +
> > + if (ret)
> > + return ret;
>
> This if (ret) is really not needed.
> It somehow short-circuit the principle used in the rest of the function
> here. All jbt_reg_write() will be nop if ret != 0.
I'll drop it.
> > +
> > + /* deep standby out */
> > + jbt_reg_write_1(lcd, JBT_REG_POWER_ON_OFF, 0x17, &ret);
> > +
> > + /* RGB I/F on, RAM write off, QVGA through, SIGCON enable */
> > + jbt_reg_write_1(lcd, JBT_REG_DISPLAY_MODE, 0x80, &ret);
> > +
> > + /* Quad mode off */
> > + jbt_reg_write_1(lcd, JBT_REG_QUAD_RATE, 0x00, &ret);
> > +
> > + /* AVDD on, XVDD on */
> > + jbt_reg_write_1(lcd, JBT_REG_POWER_ON_OFF, 0x16, &ret);
> > +
> > + /* Output control */
> > + jbt_reg_write_2(lcd, JBT_REG_OUTPUT_CONTROL, 0xfff9, &ret);
> > +
> > + /* Sleep mode off */
> > + jbt_ret_write_0(lcd, JBT_REG_SLEEP_OUT, &ret);
> > +
> > + /* at this point we have like 50% grey */
> > +
> > + /* initialize register set */
> > + jbt_reg_write_1(lcd, JBT_REG_DISPLAY_MODE1, 0x01, &ret);
> > + jbt_reg_write_1(lcd, JBT_REG_DISPLAY_MODE2, 0x00, &ret);
> > + jbt_reg_write_1(lcd, JBT_REG_RGB_FORMAT, 0x60, &ret);
> > + jbt_reg_write_1(lcd, JBT_REG_DRIVE_SYSTEM, 0x10, &ret);
> > + jbt_reg_write_1(lcd, JBT_REG_BOOSTER_OP, 0x56, &ret);
> > + jbt_reg_write_1(lcd, JBT_REG_BOOSTER_MODE, 0x33, &ret);
> > + jbt_reg_write_1(lcd, JBT_REG_BOOSTER_FREQ, 0x11, &ret);
> > + jbt_reg_write_1(lcd, JBT_REG_BOOSTER_FREQ, 0x11, &ret);
> > + jbt_reg_write_1(lcd, JBT_REG_OPAMP_SYSCLK, 0x02, &ret);
> > + jbt_reg_write_1(lcd, JBT_REG_VSC_VOLTAGE, 0x2b, &ret);
> > + jbt_reg_write_1(lcd, JBT_REG_VCOM_VOLTAGE, 0x40, &ret);
> > + jbt_reg_write_1(lcd, JBT_REG_EXT_DISPL, 0x03, &ret);
> > + jbt_reg_write_1(lcd, JBT_REG_DCCLK_DCEV, 0x04, &ret);
> > + /*
> > + * default of 0x02 in JBT_REG_ASW_SLEW responsible for 72Hz requirement
> > + * to avoid red / blue flicker
> > + */
> > + jbt_reg_write_1(lcd, JBT_REG_ASW_SLEW, 0x04, &ret);
> > + jbt_reg_write_1(lcd, JBT_REG_DUMMY_DISPLAY, 0x00, &ret);
> > +
> > + jbt_reg_write_1(lcd, JBT_REG_SLEEP_OUT_FR_A, 0x11, &ret);
> > + jbt_reg_write_1(lcd, JBT_REG_SLEEP_OUT_FR_B, 0x11, &ret);
> > + jbt_reg_write_1(lcd, JBT_REG_SLEEP_OUT_FR_C, 0x11, &ret);
> > + jbt_reg_write_2(lcd, JBT_REG_SLEEP_IN_LCCNT_D, 0x2040, &ret);
> > + jbt_reg_write_2(lcd, JBT_REG_SLEEP_IN_LCCNT_E, 0x60c0, &ret);
> > + jbt_reg_write_2(lcd, JBT_REG_SLEEP_IN_LCCNT_F, 0x1020, &ret);
> > + jbt_reg_write_2(lcd, JBT_REG_SLEEP_IN_LCCNT_G, 0x60c0, &ret);
> > +
> > + jbt_reg_write_2(lcd, JBT_REG_GAMMA1_FINE_1, 0x5533, &ret);
> > + jbt_reg_write_1(lcd, JBT_REG_GAMMA1_FINE_2, 0x00, &ret);
> > + jbt_reg_write_1(lcd, JBT_REG_GAMMA1_INCLINATION, 0x00, &ret);
> > + jbt_reg_write_1(lcd, JBT_REG_GAMMA1_BLUE_OFFSET, 0x00, &ret);
> > +
> > + jbt_reg_write_2(lcd, JBT_REG_HCLOCK_VGA, 0x1f0, &ret);
> > + jbt_reg_write_1(lcd, JBT_REG_BLANK_CONTROL, 0x02, &ret);
> > + jbt_reg_write_2(lcd, JBT_REG_BLANK_TH_TV, 0x0804, &ret);
> > +
> > + jbt_reg_write_1(lcd, JBT_REG_CKV_ON_OFF, 0x01, &ret);
> > + jbt_reg_write_2(lcd, JBT_REG_CKV_1_2, 0x0000, &ret);
> > +
> > + jbt_reg_write_2(lcd, JBT_REG_OEV_TIMING, 0x0d0e, &ret);
> > + jbt_reg_write_2(lcd, JBT_REG_ASW_TIMING_1, 0x11a4, &ret);
> > + jbt_reg_write_1(lcd, JBT_REG_ASW_TIMING_2, 0x0e, &ret);
> > +
> > + jbt_ret_write_0(lcd, JBT_REG_DISPLAY_ON, &ret);
> > +
> > + if (ret)
> > + return ret;
> > +
> > + backlight_enable(lcd->backlight);
> > +
> > + return 0;
> > +}
> > +
> > +static const struct drm_display_mode td028ttec1_mode = {
> > + .clock = 22153,
> > + .hdisplay = 480,
> > + .hsync_start = 480 + 24,
> > + .hsync_end = 480 + 24 + 8,
> > + .htotal = 480 + 24 + 8 + 8,
> > + .vdisplay = 640,
> > + .vsync_start = 640 + 4,
> > + .vsync_end = 640 + 4 + 2,
> > + .vtotal = 640 + 4 + 2 + 2,
> > + .vrefresh = 66,
> > + .type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED,
> > + .flags = DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC,
> > +};
>
> Add width_mm + height_mm.
>
> > +static int td028ttec1_remove(struct spi_device *spi)
> > +{
> > + struct td028ttec1_device *lcd = spi_get_drvdata(spi);
> > +
> > + drm_panel_remove(&lcd->panel);
> > + td028ttec1_disable(&lcd->panel);
>
> Use drm_panel_disable();
>
> > +
> > + return 0;
> > +}
> > +
> > +static const struct of_device_id td028ttec1_of_match[] = {
> > + { .compatible = "tpo,td028ttec1", },
> > + /* DT backward compatibility. */
> > + { .compatible = "toppoly,td028ttec1", },
> > + {},
>
> { /* sentinel */ },
>
> With the above nits fixed/considered:
> Reviewed-by: Sam Ravnborg <sam@xxxxxxxxxxxx>
>
> Sam
--
Regards,
Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
