Re: [PATCH 2/2] drm: panel: add Khadas TS050 panel driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux