| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <8812d17a-7b8f-d21b-3473-dd6989d2fdca@baylibre.com>
Date: Tue, 24 Nov 2020 10:08:10 +0100
From: Neil Armstrong <narmstrong@...libre.com>
To: Sam Ravnborg <sam@...nborg.org>
Cc: devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
dri-devel@...ts.freedesktop.org
Subject: Re: [PATCH 2/2] drm: panel: add Khadas TS050 panel driver
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@...libre.com>
>> ---
>> 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@...libre.com>
>> + */
>> +
>> +#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@...libre.com>");
>> +MODULE_DESCRIPTION("Khadas TS050 panel driver");
>> +MODULE_LICENSE("GPL v2");
>> --
>> 2.25.1
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@...ts.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
Thanks for the review!
Neil
Powered by blists - more mailing lists