Hi Sam, On 23/11/2020 22:05, Sam Ravnborg wrote: > Hi Neil. > > Looks good but a few comments in the following that needs some attention. > > Sam > > On Mon, Nov 23, 2020 at 03:33:54PM +0100, Neil Armstrong wrote: >> This add support for the Khadas TS050 1080x1920 5" LCD DSI panel designed to work >> with the Khadas Edge-V, Captain, VIM3 and VIM3L Single Board Computers. >> It provides a MIPI DSI interface to the host, a built-in LED backlight >> and touch controller. >> >> The init values was taken from the vendor source tree, comments were added to the >> know values but most of the init table is undocumented. >> >> Signed-off-by: Neil Armstrong <narmstrong@xxxxxxxxxxxx> >> --- >> drivers/gpu/drm/panel/Kconfig | 11 + >> drivers/gpu/drm/panel/Makefile | 1 + >> drivers/gpu/drm/panel/panel-khadas-ts050.c | 876 +++++++++++++++++++++ >> 3 files changed, 888 insertions(+) >> create mode 100644 drivers/gpu/drm/panel/panel-khadas-ts050.c >> >> diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig >> index b4e021ea30f9..8fec45b2ce02 100644 >> --- a/drivers/gpu/drm/panel/Kconfig >> +++ b/drivers/gpu/drm/panel/Kconfig >> @@ -145,6 +145,17 @@ config DRM_PANEL_JDI_LT070ME05000 >> The panel has a 1200(RGB)×1920 (WUXGA) resolution and uses >> 24 bit per pixel. >> >> +config DRM_PANEL_KHADAS_TS050 >> + tristate "Khadas TS050 panel" >> + depends on OF >> + depends on DRM_MIPI_DSI >> + depends on BACKLIGHT_CLASS_DEVICE >> + help >> + Say Y here if you want to enable support for Khadas TS050 TFT-LCD >> + panel module. The panel has a 1080x1920 resolution and uses >> + 24 bit RGB per pixel. It provides a MIPI DSI interface to >> + the host, a built-in LED backlight and touch controller. >> + >> config DRM_PANEL_KINGDISPLAY_KD097D04 >> tristate "Kingdisplay kd097d04 panel" >> depends on OF >> diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile >> index ebbf488c7eac..03496695e03f 100644 >> --- a/drivers/gpu/drm/panel/Makefile >> +++ b/drivers/gpu/drm/panel/Makefile >> @@ -13,6 +13,7 @@ obj-$(CONFIG_DRM_PANEL_ILITEK_IL9322) += panel-ilitek-ili9322.o >> obj-$(CONFIG_DRM_PANEL_ILITEK_ILI9881C) += panel-ilitek-ili9881c.o >> obj-$(CONFIG_DRM_PANEL_INNOLUX_P079ZCA) += panel-innolux-p079zca.o >> obj-$(CONFIG_DRM_PANEL_JDI_LT070ME05000) += panel-jdi-lt070me05000.o >> +obj-$(CONFIG_DRM_PANEL_KHADAS_TS050) += panel-khadas-ts050.o >> obj-$(CONFIG_DRM_PANEL_KINGDISPLAY_KD097D04) += panel-kingdisplay-kd097d04.o >> obj-$(CONFIG_DRM_PANEL_LEADTEK_LTK050H3146W) += panel-leadtek-ltk050h3146w.o >> obj-$(CONFIG_DRM_PANEL_LEADTEK_LTK500HD1829) += panel-leadtek-ltk500hd1829.o >> diff --git a/drivers/gpu/drm/panel/panel-khadas-ts050.c b/drivers/gpu/drm/panel/panel-khadas-ts050.c >> new file mode 100644 >> index 000000000000..856fcad69306 >> --- /dev/null >> +++ b/drivers/gpu/drm/panel/panel-khadas-ts050.c >> @@ -0,0 +1,876 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/* >> + * Copyright (C) 2020 BayLibre, SAS >> + * Author: Neil Armstrong <narmstrong@xxxxxxxxxxxx> >> + */ >> + >> +#include <linux/delay.h> >> +#include <linux/gpio/consumer.h> >> +#include <linux/module.h> >> +#include <linux/of.h> >> +#include <linux/regulator/consumer.h> >> + >> +#include <video/mipi_display.h> >> + >> +#include <drm/drm_crtc.h> >> +#include <drm/drm_device.h> >> +#include <drm/drm_mipi_dsi.h> >> +#include <drm/drm_modes.h> >> +#include <drm/drm_panel.h> >> +#include <drm/drm_print.h> > Panels do not use DRM_ or drm_ for logging, so this include is not > needed. Ok forgot to remove this > >> + >> +struct khadas_ts050_panel { >> + struct drm_panel base; >> + struct mipi_dsi_device *link; >> + >> + struct regulator *supply; >> + struct gpio_desc *reset_gpio; >> + struct gpio_desc *enable_gpio; >> + >> + bool prepared; >> + bool enabled; >> +}; >> + >> +struct khadas_ts050_panel_cmd { >> + u8 cmd; >> + u8 data; >> +}; >> + >> +/* Only the CMD1 User Command set is documented */ >> +static const struct khadas_ts050_panel_cmd init_code[] = { >> + /* Select Unknown CMD Page (Undocumented) */ >> + {0xff, 0xee}, > ... >> + {0xd4, 0x04}, /* RGBMIPICTRL: VSYNC front porch = 4 */ >> +}; >> + >> +static inline >> +struct khadas_ts050_panel *to_khadas_ts050_panel(struct drm_panel *panel) >> +{ >> + return container_of(panel, struct khadas_ts050_panel, base); >> +} >> + >> +static int khadas_ts050_panel_prepare(struct drm_panel *panel) >> +{ >> + struct khadas_ts050_panel *khadas_ts050 = to_khadas_ts050_panel(panel); >> + int err, regulator_err; >> + unsigned int i; >> + >> + if (khadas_ts050->prepared) >> + return 0; >> + >> + gpiod_set_value_cansleep(khadas_ts050->enable_gpio, 0); >> + >> + err = regulator_enable(khadas_ts050->supply); >> + if (err < 0) >> + return err; >> + >> + gpiod_set_value_cansleep(khadas_ts050->enable_gpio, 1); >> + >> + msleep(60); >> + >> + gpiod_set_value_cansleep(khadas_ts050->reset_gpio, 1); >> + >> + usleep_range(10000, 11000); >> + >> + gpiod_set_value_cansleep(khadas_ts050->reset_gpio, 0); >> + >> + /* Select CMD2 page 4 (Undocumented) */ >> + mipi_dsi_dcs_write(khadas_ts050->link, 0xff, (u8[]){ 0x05 }, 1); >> + >> + /* Reload CMD1: Don't reload default value to register */ >> + mipi_dsi_dcs_write(khadas_ts050->link, 0xfb, (u8[]){ 0x01 }, 1); >> + >> + mipi_dsi_dcs_write(khadas_ts050->link, 0xc5, (u8[]){ 0x01 }, 1); >> + >> + msleep(100); >> + >> + for (i = 0; i < ARRAY_SIZE(init_code); i++) { >> + err = mipi_dsi_dcs_write(khadas_ts050->link, >> + init_code[i].cmd, >> + &init_code[i].data, 1); >> + if (err < 0) { >> + dev_err(panel->dev, "failed write cmds: %d\n", err); >> + goto poweroff; >> + } >> + } >> + >> + err = mipi_dsi_dcs_exit_sleep_mode(khadas_ts050->link); >> + if (err < 0) { >> + dev_err(panel->dev, "failed to exit sleep mode: %d\n", err); >> + goto poweroff; >> + } >> + >> + msleep(120); >> + >> + /* Select CMD1 */ >> + mipi_dsi_dcs_write(khadas_ts050->link, 0xff, (u8[]){ 0x00 }, 1); >> + >> + err = mipi_dsi_dcs_set_tear_on(khadas_ts050->link, >> + MIPI_DSI_DCS_TEAR_MODE_VBLANK); >> + if (err < 0) { >> + dev_err(panel->dev, "failed to set tear on: %d\n", err); >> + goto poweroff; >> + } >> + >> + >> + err = mipi_dsi_dcs_set_display_on(khadas_ts050->link); >> + if (err < 0) { >> + dev_err(panel->dev, "failed to set display on: %d\n", err); >> + goto poweroff; >> + } >> + >> + usleep_range(10000, 11000); >> + >> + khadas_ts050->prepared = true; >> + >> + return 0; >> + >> +poweroff: >> + gpiod_set_value_cansleep(khadas_ts050->enable_gpio, 0); > > gpiod_set_value_cansleep(khadas_ts050->reset_gpio, 1); > ?? > So panel is left in reset mode. Indeed, forgot this one >> + >> + regulator_err = regulator_disable(khadas_ts050->supply); >> + if (regulator_err) >> + dev_err(panel->dev, "failed to disable regulator: %d\n", regulator_err); > This check is not really of any value and can be dropped. ok > >> + >> + return err; >> +} >> + >> +static int khadas_ts050_panel_unprepare(struct drm_panel *panel) >> +{ >> + struct khadas_ts050_panel *khadas_ts050 = to_khadas_ts050_panel(panel); >> + int err; >> + >> + if (!khadas_ts050->prepared) >> + return 0; >> + >> + err = mipi_dsi_dcs_enter_sleep_mode(khadas_ts050->link); >> + if (err < 0) { >> + dev_err(panel->dev, "failed to enter sleep mode: %d\n", err); >> + return err; > Hmm, even if we fail here or below the panel is no longer prepared. > I think it would be better to set prepared to false unconditionally. > > Many panel drivers get this wrong. ok > > >> + } >> + >> + msleep(150); >> + >> + gpiod_set_value_cansleep(khadas_ts050->enable_gpio, 0); >> + >> + err = regulator_disable(khadas_ts050->supply); >> + if (err < 0) >> + return err; >> + >> + khadas_ts050->prepared = false; >> + >> + return 0; >> +} >> + >> +static int khadas_ts050_panel_enable(struct drm_panel *panel) >> +{ >> + struct khadas_ts050_panel *khadas_ts050 = to_khadas_ts050_panel(panel); >> + >> + if (khadas_ts050->enabled) >> + return 0; > No need for the check - just set it to true. ok >> + >> + khadas_ts050->enabled = true; >> + >> + return 0; >> +} >> + >> +static int khadas_ts050_panel_disable(struct drm_panel *panel) >> +{ >> + struct khadas_ts050_panel *khadas_ts050 = to_khadas_ts050_panel(panel); >> + int err; >> + >> + if (!khadas_ts050->enabled) >> + return 0; >> + >> + err = mipi_dsi_dcs_set_display_off(khadas_ts050->link); >> + if (err < 0) >> + dev_err(panel->dev, "failed to set display off: %d\n", err); >> + >> + usleep_range(10000, 11000); >> + >> + khadas_ts050->enabled = false; >> + >> + return 0; >> +} >> + >> +static const struct drm_display_mode default_mode = { >> + .clock = 120000, >> + .hdisplay = 1088, >> + .hsync_start = 1088 + 104, >> + .hsync_end = 1088 + 104 + 4, >> + .htotal = 1088 + 104 + 4 + 127, >> + .vdisplay = 1920, >> + .vsync_start = 1920 + 4, >> + .vsync_end = 1920 + 4 + 2, >> + .vtotal = 1920 + 4 + 2 + 3, >> + .flags = DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC, >> +}; >> + >> +static int khadas_ts050_panel_get_modes(struct drm_panel *panel, >> + struct drm_connector *connector) >> +{ >> + struct drm_display_mode *mode; >> + >> + mode = drm_mode_duplicate(connector->dev, &default_mode); >> + if (!mode) { >> + dev_err(panel->dev, "failed to add mode %ux%u@%u\n", >> + default_mode.hdisplay, default_mode.vdisplay, >> + drm_mode_vrefresh(&default_mode)); >> + return -ENOMEM; >> + } >> + >> + drm_mode_set_name(mode); >> + >> + drm_mode_probed_add(connector, mode); >> + >> + connector->display_info.width_mm = 64; >> + connector->display_info.height_mm = 118; >> + connector->display_info.bpc = 8; >> + >> + return 1; >> +} >> + >> +static const struct drm_panel_funcs khadas_ts050_panel_funcs = { >> + .prepare = khadas_ts050_panel_prepare, >> + .unprepare = khadas_ts050_panel_unprepare, >> + .enable = khadas_ts050_panel_enable, >> + .disable = khadas_ts050_panel_disable, >> + .get_modes = khadas_ts050_panel_get_modes, >> +}; >> + >> +static const struct of_device_id khadas_ts050_of_match[] = { >> + { .compatible = "khadas,ts050", }, >> + { /* sentinel */ } >> +}; >> +MODULE_DEVICE_TABLE(of, khadas_ts050_of_match); >> + >> +static int khadas_ts050_panel_add(struct khadas_ts050_panel *khadas_ts050) >> +{ >> + struct device *dev = &khadas_ts050->link->dev; >> + int err; >> + >> + khadas_ts050->supply = devm_regulator_get(dev, "power"); >> + if (IS_ERR(khadas_ts050->supply)) >> + return PTR_ERR(khadas_ts050->supply); > Use return dev_err_probbe(dev, PTR_ERR(khadas_ts050->supply), "failed to get power supply"); ok > > >> + >> + khadas_ts050->reset_gpio = devm_gpiod_get(dev, "reset", >> + GPIOD_OUT_LOW); >> + if (IS_ERR(khadas_ts050->reset_gpio)) { >> + err = PTR_ERR(khadas_ts050->reset_gpio); >> + dev_dbg(dev, "failed to get reset gpio: %d\n", err); >> + return err; > Likewise - use return dev_err_probe(...); > >> + >> + khadas_ts050->enable_gpio = devm_gpiod_get(dev, "enable", >> + GPIOD_OUT_HIGH); >> + if (IS_ERR(khadas_ts050->enable_gpio)) { >> + err = PTR_ERR(khadas_ts050->enable_gpio); >> + dev_dbg(dev, "failed to get enable gpio: %d\n", err); >> + return err; >> + } > dev_err_probe() > >> + >> + drm_panel_init(&khadas_ts050->base, &khadas_ts050->link->dev, >> + &khadas_ts050_panel_funcs, DRM_MODE_CONNECTOR_DSI); >> + >> + err = drm_panel_of_backlight(&khadas_ts050->base); >> + if (err) >> + return err; >> + >> + drm_panel_add(&khadas_ts050->base); >> + >> + return 0; >> +} >> + >> +static int khadas_ts050_panel_probe(struct mipi_dsi_device *dsi) >> +{ >> + struct khadas_ts050_panel *khadas_ts050; >> + int err; >> + >> + dsi->lanes = 4; >> + dsi->format = MIPI_DSI_FMT_RGB888; >> + dsi->mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_BURST | >> + MIPI_DSI_MODE_LPM | MIPI_DSI_MODE_EOT_PACKET; >> + >> + khadas_ts050 = devm_kzalloc(&dsi->dev, sizeof(*khadas_ts050), >> + GFP_KERNEL); >> + if (!khadas_ts050) >> + return -ENOMEM; >> + >> + mipi_dsi_set_drvdata(dsi, khadas_ts050); >> + khadas_ts050->link = dsi; >> + >> + err = khadas_ts050_panel_add(khadas_ts050); >> + if (err < 0) >> + return err; >> + >> + return mipi_dsi_attach(dsi); >> +} >> + >> +static int khadas_ts050_panel_remove(struct mipi_dsi_device *dsi) >> +{ >> + struct khadas_ts050_panel *khadas_ts050 = mipi_dsi_get_drvdata(dsi); >> + int err; >> + >> + err = mipi_dsi_detach(dsi); >> + if (err < 0) >> + dev_err(&dsi->dev, "failed to detach from DSI host: %d\n", err); >> + >> + drm_panel_remove(&khadas_ts050->base); >> + drm_panel_disable(&khadas_ts050->base); >> + drm_panel_unprepare(&khadas_ts050->base); >> + >> + return 0; >> +} >> + >> +static void khadas_ts050_panel_shutdown(struct mipi_dsi_device *dsi) >> +{ >> + struct khadas_ts050_panel *khadas_ts050 = mipi_dsi_get_drvdata(dsi); >> + >> + drm_panel_disable(&khadas_ts050->base); >> + drm_panel_unprepare(&khadas_ts050->base); >> +} >> + >> +static struct mipi_dsi_driver khadas_ts050_panel_driver = { >> + .driver = { >> + .name = "panel-khadas-ts050", >> + .of_match_table = khadas_ts050_of_match, >> + }, >> + .probe = khadas_ts050_panel_probe, >> + .remove = khadas_ts050_panel_remove, >> + .shutdown = khadas_ts050_panel_shutdown, >> +}; >> +module_mipi_dsi_driver(khadas_ts050_panel_driver); >> + >> +MODULE_AUTHOR("Neil Armstrong <narmstrong@xxxxxxxxxxxx>"); >> +MODULE_DESCRIPTION("Khadas TS050 panel driver"); >> +MODULE_LICENSE("GPL v2"); >> -- >> 2.25.1 >> >> _______________________________________________ >> dri-devel mailing list >> dri-devel@xxxxxxxxxxxxxxxxxxxxx >> https://lists.freedesktop.org/mailman/listinfo/dri-devel Thanks for the review! Neil _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel