[PATCH 0/4] drm/panel: Support for OSD101T2045-53TS and OSD101T2587-53TS

[PATCH 0/4] drm/panel: Support for OSD101T2045-53TS and OSD101T2587-53TS

[Peter Ujfalusi]

Hi,

Add support for OSD101T2045-53TS and OSD101T2587-53TS from One Stop Displays.

The two panel is similar with one big difference: OSD101T2587-53TS requires the
MIPI_DSI_TURN_ON_PERIPHERAL message, thus can not be handled by panel-simple.

Regards,
Peter
---
Peter Ujfalusi (4):
  dt-bindings: display: Add bindings for OSD101T2045-53TS
  drm/panel: simple: Add support for OSD101T2045-53TS
  dt-bindings: display: Add bindings for OSD101T2587-53TS panel
  drm/panel: Add OSD101T2587-53TS driver

 .../display/panel/osd,osd101t2045-53ts.txt    |  11 +
 .../display/panel/osd,osd101t2587-53ts.txt    |  14 +
 drivers/gpu/drm/panel/Kconfig                 |   6 +
 drivers/gpu/drm/panel/Makefile                |   1 +
 .../drm/panel/panel-osd-osd101t2587-53ts.c    | 284 ++++++++++++++++++
 drivers/gpu/drm/panel/panel-simple.c          |  34 +++
 6 files changed, 350 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/panel/osd,osd101t2045-53ts.txt
 create mode 100644 Documentation/devicetree/bindings/display/panel/osd,osd101t2587-53ts.txt
 create mode 100644 drivers/gpu/drm/panel/panel-osd-osd101t2587-53ts.c

-- 
Peter

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

[PATCH 1/4] dt-bindings: display: Add bindings for OSD101T2045-53TS

[Peter Ujfalusi]

This adds the device-tree bindings for the OSD101T2045-53TS 10.1"
1920x1200 panel from One Stop Displays.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
 .../bindings/display/panel/osd,osd101t2045-53ts.txt   | 11 +++++++++++
 1 file changed, 11 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/panel/osd,osd101t2045-53ts.txt

diff --git a/Documentation/devicetree/bindings/display/panel/osd,osd101t2045-53ts.txt b/Documentation/devicetree/bindings/display/panel/osd,osd101t2045-53ts.txt
new file mode 100644
index 000000000000..b3f6df59f7c1
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/panel/osd,osd101t2045-53ts.txt
@@ -0,0 +1,11 @@
+One Stop Displays OSD101T2045-53TS 10.1" 1920x1200 panel
+
+Required properties:
+- compatible: should be "osd,osd101t2045-53ts"
+- power-supply: as specified in the base binding
+
+Optional properties:
+- backlight: as specified in the base binding
+
+This binding is compatible with the simple-panel binding, which is specified
+in simple-panel.txt in this directory.
-- 
Peter

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

[PATCH 2/4] drm/panel: simple: Add support for OSD101T2045-53TS

[Peter Ujfalusi]

Add support for the OSD101T2045-53TS 10.1" 1920x1200 panel from One Stop
Displays to the panel-simple driver

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
 drivers/gpu/drm/panel/panel-simple.c | 34 ++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
index 9e8218f6a3f2..0337583ffa49 100644
--- a/drivers/gpu/drm/panel/panel-simple.c
+++ b/drivers/gpu/drm/panel/panel-simple.c
@@ -2996,6 +2996,37 @@ static const struct panel_desc_dsi panasonic_vvx10f004b00 = {
 	.lanes = 4,
 };
 
+static const struct drm_display_mode osd101t2045_53ts_mode = {
+	.clock = 154500,
+	.hdisplay = 1920,
+	.hsync_start = 1920 + 112,
+	.hsync_end = 1920 + 112 + 16,
+	.htotal = 1920 + 112 + 16 + 32,
+	.vdisplay = 1200,
+	.vsync_start = 1200 + 16,
+	.vsync_end = 1200 + 16 + 2,
+	.vtotal = 1200 + 16 + 2 + 16,
+	.vrefresh = 60,
+	.flags = DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC,
+};
+
+static const struct panel_desc_dsi osd101t2045_53ts = {
+	.desc = {
+		.modes = &osd101t2045_53ts_mode,
+		.num_modes = 1,
+		.bpc = 8,
+		.size = {
+			.width = 217,
+			.height = 136,
+		},
+	},
+	.flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_BURST |
+		 MIPI_DSI_MODE_VIDEO_SYNC_PULSE |
+		 MIPI_DSI_MODE_EOT_PACKET,
+	.format = MIPI_DSI_FMT_RGB888,
+	.lanes = 4,
+};
+
 static const struct of_device_id dsi_of_match[] = {
 	{
 		.compatible = "auo,b080uan01",
@@ -3012,6 +3043,9 @@ static const struct of_device_id dsi_of_match[] = {
 	}, {
 		.compatible = "panasonic,vvx10f004b00",
 		.data = &panasonic_vvx10f004b00
+	}, {
+		.compatible = "osd,osd101t2045-53ts",
+		.data = &osd101t2045_53ts
 	}, {
 		/* sentinel */
 	}
-- 
Peter

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

[PATCH 3/4] dt-bindings: display: Add bindings for OSD101T2587-53TS panel

[Peter Ujfalusi]

This adds the device-tree bindings for the OSD101T2587-53TS 10.1"
1920x1200 panel from One Stop Displays.

Note: the panel is similar to OSD101T2045-53TS, but it needs additional
MIPI_DSI_TURN_ON_PERIPHERAL message from the host.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
 .../display/panel/osd,osd101t2587-53ts.txt         | 14 ++++++++++++++
 1 file changed, 14 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/panel/osd,osd101t2587-53ts.txt

diff --git a/Documentation/devicetree/bindings/display/panel/osd,osd101t2587-53ts.txt b/Documentation/devicetree/bindings/display/panel/osd,osd101t2587-53ts.txt
new file mode 100644
index 000000000000..2082cae1a0e3
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/panel/osd,osd101t2587-53ts.txt
@@ -0,0 +1,14 @@
+One Stop Displays OSD101T2587-53TS 10.1" 1920x1200 panel
+
+The panel is similar to OSD101T2045-53TS, but it needs additional
+MIPI_DSI_TURN_ON_PERIPHERAL message from the host.
+
+Required properties:
+- compatible: should be "osd,osd101t2587-53ts"
+- power-supply: as specified in the base binding
+
+Optional properties:
+- backlight: as specified in the base binding
+
+This binding is compatible with the simple-panel binding, which is specified
+in simple-panel.txt in this directory.
-- 
Peter

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

[PATCH 4/4] drm/panel: Add OSD101T2587-53TS driver

[Peter Ujfalusi]

The panel is similar to OSD101T2045-53TS (which is handled by panel-simple)
with one big difference: osd101t2587-53ts needs MIPI_DSI_TURN_ON_PERIPHERAL
message to be sent from the host to be operational and thus can not be
handled by panel-simple.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
 drivers/gpu/drm/panel/Kconfig                 |   6 +
 drivers/gpu/drm/panel/Makefile                |   1 +
 .../drm/panel/panel-osd-osd101t2587-53ts.c    | 284 ++++++++++++++++++
 3 files changed, 291 insertions(+)
 create mode 100644 drivers/gpu/drm/panel/panel-osd-osd101t2587-53ts.c

diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
index 3e070153ef21..351661920838 100644
--- a/drivers/gpu/drm/panel/Kconfig
+++ b/drivers/gpu/drm/panel/Kconfig
@@ -122,6 +122,12 @@ config DRM_PANEL_ORISETECH_OTM8009A
 	  Say Y here if you want to enable support for Orise Technology
 	  otm8009a 480x800 dsi 2dl panel.
 
+config DRM_PANEL_OSD_OSD101T2587_53TS
+	tristate "OSD OSD101T2587-53TS DSI 1920x1200 video mode panel"
+	depends on OF
+	depends on DRM_MIPI_DSI
+	depends on BACKLIGHT_CLASS_DEVICE
+
 config DRM_PANEL_PANASONIC_VVX10F034N00
 	tristate "Panasonic VVX10F034N00 1920x1200 video mode panel"
 	depends on OF
diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile
index e7ab71968bbf..d9d99956db0c 100644
--- a/drivers/gpu/drm/panel/Makefile
+++ b/drivers/gpu/drm/panel/Makefile
@@ -10,6 +10,7 @@ obj-$(CONFIG_DRM_PANEL_KINGDISPLAY_KD097D04) += panel-kingdisplay-kd097d04.o
 obj-$(CONFIG_DRM_PANEL_LG_LG4573) += panel-lg-lg4573.o
 obj-$(CONFIG_DRM_PANEL_OLIMEX_LCD_OLINUXINO) += panel-olimex-lcd-olinuxino.o
 obj-$(CONFIG_DRM_PANEL_ORISETECH_OTM8009A) += panel-orisetech-otm8009a.o
+obj-$(CONFIG_DRM_PANEL_OSD_OSD101T2587_53TS) += panel-osd-osd101t2587-53ts.o
 obj-$(CONFIG_DRM_PANEL_PANASONIC_VVX10F034N00) += panel-panasonic-vvx10f034n00.o
 obj-$(CONFIG_DRM_PANEL_RASPBERRYPI_TOUCHSCREEN) += panel-raspberrypi-touchscreen.o
 obj-$(CONFIG_DRM_PANEL_RAYDIUM_RM68200) += panel-raydium-rm68200.o
diff --git a/drivers/gpu/drm/panel/panel-osd-osd101t2587-53ts.c b/drivers/gpu/drm/panel/panel-osd-osd101t2587-53ts.c
new file mode 100644
index 000000000000..6dbdd5b27bfb
--- /dev/null
+++ b/drivers/gpu/drm/panel/panel-osd-osd101t2587-53ts.c
@@ -0,0 +1,284 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ *  Copyright (C) 2018 Texas Instruments Incorporated - http://www.ti.com
+ *  Author: Peter Ujfalusi <peter.ujfalusi@ti.com>
+ */
+
+#include <linux/backlight.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/regulator/consumer.h>
+
+#include <drm/drmP.h>
+#include <drm/drm_crtc.h>
+#include <drm/drm_mipi_dsi.h>
+#include <drm/drm_panel.h>
+
+#include <video/mipi_display.h>
+
+struct osd101t2587_panel {
+	struct drm_panel base;
+	struct mipi_dsi_device *dsi;
+
+	struct backlight_device *backlight;
+	struct regulator *supply;
+
+	bool prepared;
+	bool enabled;
+
+	ktime_t earliest_wake;
+
+	const struct drm_display_mode *default_mode;
+	const struct drm_display_mode *mode;
+};
+
+static inline struct osd101t2587_panel *to_osd101t2587_panel(struct drm_panel *panel)
+{
+	return container_of(panel, struct osd101t2587_panel, base);
+}
+
+static int osd101t2587_panel_disable(struct drm_panel *panel)
+{
+	struct osd101t2587_panel *osd101t2587 = to_osd101t2587_panel(panel);
+	int ret;
+
+	if (!osd101t2587->enabled)
+		return 0;
+
+	if (osd101t2587->backlight) {
+		osd101t2587->backlight->props.power = FB_BLANK_POWERDOWN;
+		osd101t2587->backlight->props.state |= BL_CORE_FBBLANK;
+		backlight_update_status(osd101t2587->backlight);
+	}
+
+	ret = mipi_dsi_shutdown_peripheral(osd101t2587->dsi);
+
+	osd101t2587->enabled = false;
+
+	return ret;
+}
+
+static int osd101t2587_panel_unprepare(struct drm_panel *panel)
+{
+	struct osd101t2587_panel *osd101t2587 = to_osd101t2587_panel(panel);
+
+	if (!osd101t2587->prepared)
+		return 0;
+
+	regulator_disable(osd101t2587->supply);
+	osd101t2587->prepared = false;
+
+	return 0;
+}
+
+static int osd101t2587_panel_prepare(struct drm_panel *panel)
+{
+	struct osd101t2587_panel *osd101t2587 = to_osd101t2587_panel(panel);
+	int ret;
+
+	if (osd101t2587->prepared)
+		return 0;
+
+	ret = regulator_enable(osd101t2587->supply);
+	if (!ret)
+		osd101t2587->prepared = true;
+
+	return ret;
+}
+
+static int osd101t2587_panel_enable(struct drm_panel *panel)
+{
+	struct osd101t2587_panel *osd101t2587 = to_osd101t2587_panel(panel);
+	int ret;
+
+	if (osd101t2587->enabled)
+		return 0;
+
+	ret = mipi_dsi_turn_on_peripheral(osd101t2587->dsi);
+	if (ret)
+		return ret;
+
+	if (osd101t2587->backlight) {
+		osd101t2587->backlight->props.power = FB_BLANK_UNBLANK;
+		osd101t2587->backlight->props.state &= ~BL_CORE_FBBLANK;
+		backlight_update_status(osd101t2587->backlight);
+	}
+
+	osd101t2587->enabled = true;
+
+	return ret;
+}
+
+static const struct drm_display_mode default_mode_osd101t2587 = {
+	.clock = 164400,
+	.hdisplay = 1920,
+	.hsync_start = 1920 + 152,
+	.hsync_end = 1920 + 152 + 52,
+	.htotal = 1920 + 152 + 52 + 20,
+	.vdisplay = 1200,
+	.vsync_start = 1200 + 24,
+	.vsync_end = 1200 + 24 + 6,
+	.vtotal = 1200 + 24 + 6 + 48,
+	.vrefresh = 60,
+	.flags = DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC,
+};
+
+static int osd101t2587_panel_get_modes(struct drm_panel *panel)
+{
+	struct osd101t2587_panel *osd101t2587 = to_osd101t2587_panel(panel);
+	struct drm_display_mode *mode;
+
+	mode = drm_mode_duplicate(panel->drm, osd101t2587->default_mode);
+	if (!mode) {
+		dev_err(panel->drm->dev, "failed to add mode %ux%ux@%u\n",
+			osd101t2587->default_mode->hdisplay,
+			osd101t2587->default_mode->vdisplay,
+			osd101t2587->default_mode->vrefresh);
+		return -ENOMEM;
+	}
+
+	drm_mode_set_name(mode);
+
+	drm_mode_probed_add(panel->connector, mode);
+
+	panel->connector->display_info.width_mm = 217;
+	panel->connector->display_info.height_mm = 136;
+
+	return 1;
+}
+
+static const struct drm_panel_funcs osd101t2587_panel_funcs = {
+	.disable = osd101t2587_panel_disable,
+	.unprepare = osd101t2587_panel_unprepare,
+	.prepare = osd101t2587_panel_prepare,
+	.enable = osd101t2587_panel_enable,
+	.get_modes = osd101t2587_panel_get_modes,
+};
+
+static const struct of_device_id osd101t2587_of_match[] = {
+	{
+		.compatible = "osd,osd101t2587-53ts",
+		.data = &default_mode_osd101t2587,
+	}, { }
+};
+MODULE_DEVICE_TABLE(of, osd101t2587_of_match);
+
+static int osd101t2587_panel_add(struct osd101t2587_panel *osd101t2587)
+{
+	struct device *dev = &osd101t2587->dsi->dev;
+	struct device_node *np;
+	int ret;
+
+	osd101t2587->mode = osd101t2587->default_mode;
+
+	osd101t2587->supply = devm_regulator_get(dev, "power");
+	if (IS_ERR(osd101t2587->supply))
+		return PTR_ERR(osd101t2587->supply);
+
+	np = of_parse_phandle(dev->of_node, "backlight", 0);
+	if (np) {
+		osd101t2587->backlight = of_find_backlight_by_node(np);
+		of_node_put(np);
+
+		if (!osd101t2587->backlight)
+			return -EPROBE_DEFER;
+	}
+
+	drm_panel_init(&osd101t2587->base);
+	osd101t2587->base.funcs = &osd101t2587_panel_funcs;
+	osd101t2587->base.dev = &osd101t2587->dsi->dev;
+
+	ret = drm_panel_add(&osd101t2587->base);
+	if (ret < 0)
+		goto put_backlight;
+
+	return 0;
+
+put_backlight:
+	if (osd101t2587->backlight)
+		put_device(&osd101t2587->backlight->dev);
+
+	return ret;
+}
+
+static void osd101t2587_panel_del(struct osd101t2587_panel *osd101t2587)
+{
+	if (osd101t2587->base.dev)
+		drm_panel_remove(&osd101t2587->base);
+
+	if (osd101t2587->backlight)
+		put_device(&osd101t2587->backlight->dev);
+}
+
+static int osd101t2587_panel_probe(struct mipi_dsi_device *dsi)
+{
+	struct osd101t2587_panel *osd101t2587;
+	const struct of_device_id *id;
+	int ret;
+
+	id = of_match_node(osd101t2587_of_match, dsi->dev.of_node);
+	if (!id)
+		return -ENODEV;
+
+	dsi->lanes = 4;
+	dsi->format = MIPI_DSI_FMT_RGB888;
+	dsi->mode_flags = MIPI_DSI_MODE_VIDEO |
+			  MIPI_DSI_MODE_VIDEO_BURST |
+			  MIPI_DSI_MODE_VIDEO_SYNC_PULSE |
+			  MIPI_DSI_MODE_EOT_PACKET;
+
+	osd101t2587 = devm_kzalloc(&dsi->dev, sizeof(*osd101t2587), GFP_KERNEL);
+	if (!osd101t2587)
+		return -ENOMEM;
+
+	mipi_dsi_set_drvdata(dsi, osd101t2587);
+
+	osd101t2587->dsi = dsi;
+	osd101t2587->default_mode = id->data;
+
+	ret = osd101t2587_panel_add(osd101t2587);
+	if (ret < 0)
+		return ret;
+
+	return mipi_dsi_attach(dsi);
+}
+
+static int osd101t2587_panel_remove(struct mipi_dsi_device *dsi)
+{
+	struct osd101t2587_panel *osd101t2587 = mipi_dsi_get_drvdata(dsi);
+	int ret;
+
+	ret = osd101t2587_panel_disable(&osd101t2587->base);
+	if (ret < 0)
+		dev_err(&dsi->dev, "failed to disable panel: %d\n", ret);
+
+	ret = mipi_dsi_detach(dsi);
+	if (ret < 0)
+		dev_err(&dsi->dev, "failed to detach from DSI host: %d\n", ret);
+
+	osd101t2587_panel_del(osd101t2587);
+
+	return 0;
+}
+
+static void osd101t2587_panel_shutdown(struct mipi_dsi_device *dsi)
+{
+	struct osd101t2587_panel *osd101t2587 = mipi_dsi_get_drvdata(dsi);
+
+	osd101t2587_panel_disable(&osd101t2587->base);
+}
+
+static struct mipi_dsi_driver osd101t2587_panel_driver = {
+	.driver = {
+		.name = "panel-osd-osd101t2587-53ts",
+		.of_match_table = osd101t2587_of_match,
+	},
+	.probe = osd101t2587_panel_probe,
+	.remove = osd101t2587_panel_remove,
+	.shutdown = osd101t2587_panel_shutdown,
+};
+module_mipi_dsi_driver(osd101t2587_panel_driver);
+
+MODULE_AUTHOR("Peter Ujfalusi <peter.ujfalusi@ti.com>");
+MODULE_DESCRIPTION("OSD101T2587-53TS DSI panel");
+MODULE_LICENSE("GPL v2");
-- 
Peter

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [PATCH 4/4] drm/panel: Add OSD101T2587-53TS driver

[Sam Ravnborg]

Hi Peter.

Good with more panel drivers.
Some comments in the following, please do not blindly follow them but
check that this is OK.

	Sam

On Fri, Feb 15, 2019 at 04:03:15PM +0200, Peter Ujfalusi via dri-devel wrote:
> The panel is similar to OSD101T2045-53TS (which is handled by panel-simple)
> with one big difference: osd101t2587-53ts needs MIPI_DSI_TURN_ON_PERIPHERAL
> message to be sent from the host to be operational and thus can not be
> handled by panel-simple.
> 
> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
> ---
> +++ b/drivers/gpu/drm/panel/panel-osd-osd101t2587-53ts.c
> @@ -0,0 +1,284 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + *  Copyright (C) 2018 Texas Instruments Incorporated - http://www.ti.com
> + *  Author: Peter Ujfalusi <peter.ujfalusi@ti.com>
> + */
> +
> +#include <linux/backlight.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/regulator/consumer.h>
> +
> +#include <drm/drmP.h>
Please do not use drmP.h in new drivers - we try to get rid of this file.

> +#include <drm/drm_crtc.h>
> +#include <drm/drm_mipi_dsi.h>
> +#include <drm/drm_panel.h>
> +
> +#include <video/mipi_display.h>
> +
> +struct osd101t2587_panel {
> +	struct drm_panel base;
> +	struct mipi_dsi_device *dsi;
> +
> +	struct backlight_device *backlight;
> +	struct regulator *supply;
> +
> +	bool prepared;
> +	bool enabled;
> +
> +	ktime_t earliest_wake;
earliest_wake is not used, and can be deleted.

> +
> +	const struct drm_display_mode *default_mode;
> +	const struct drm_display_mode *mode;
Assigned but never used. Drop the assignment and the variable?

> +};
> +
> +static inline struct osd101t2587_panel *to_osd101t2587_panel(struct drm_panel *panel)
> +{
> +	return container_of(panel, struct osd101t2587_panel, base);
> +}
> +
> +static int osd101t2587_panel_disable(struct drm_panel *panel)
> +{
> +	struct osd101t2587_panel *osd101t2587 = to_osd101t2587_panel(panel);
> +	int ret;
> +
> +	if (!osd101t2587->enabled)
> +		return 0;
> +
> +	if (osd101t2587->backlight) {
> +		osd101t2587->backlight->props.power = FB_BLANK_POWERDOWN;
> +		osd101t2587->backlight->props.state |= BL_CORE_FBBLANK;
> +		backlight_update_status(osd101t2587->backlight);
> +	}
backlight_disable(osd101t2587->backlight);


> +
> +	ret = mipi_dsi_shutdown_peripheral(osd101t2587->dsi);
> +
> +	osd101t2587->enabled = false;
> +
> +	return ret;
> +}
> +
> +static int osd101t2587_panel_unprepare(struct drm_panel *panel)
> +{
> +	struct osd101t2587_panel *osd101t2587 = to_osd101t2587_panel(panel);
> +
> +	if (!osd101t2587->prepared)
> +		return 0;
> +
> +	regulator_disable(osd101t2587->supply);
> +	osd101t2587->prepared = false;
> +
> +	return 0;
> +}
> +
> +static int osd101t2587_panel_prepare(struct drm_panel *panel)
> +{
> +	struct osd101t2587_panel *osd101t2587 = to_osd101t2587_panel(panel);
> +	int ret;
> +
> +	if (osd101t2587->prepared)
> +		return 0;
> +
> +	ret = regulator_enable(osd101t2587->supply);
> +	if (!ret)
> +		osd101t2587->prepared = true;
> +
> +	return ret;
> +}
> +
> +static int osd101t2587_panel_enable(struct drm_panel *panel)
> +{
> +	struct osd101t2587_panel *osd101t2587 = to_osd101t2587_panel(panel);
> +	int ret;
> +
> +	if (osd101t2587->enabled)
> +		return 0;
> +
> +	ret = mipi_dsi_turn_on_peripheral(osd101t2587->dsi);
> +	if (ret)
> +		return ret;
> +
> +	if (osd101t2587->backlight) {
> +		osd101t2587->backlight->props.power = FB_BLANK_UNBLANK;
> +		osd101t2587->backlight->props.state &= ~BL_CORE_FBBLANK;
> +		backlight_update_status(osd101t2587->backlight);
> +	}
backlight_enable(osd101t2587->backlight);
can be used here.

> +
> +	osd101t2587->enabled = true;
> +
> +	return ret;
> +}
> +
> +static const struct drm_display_mode default_mode_osd101t2587 = {
> +	.clock = 164400,
> +	.hdisplay = 1920,
> +	.hsync_start = 1920 + 152,
> +	.hsync_end = 1920 + 152 + 52,
> +	.htotal = 1920 + 152 + 52 + 20,
> +	.vdisplay = 1200,
> +	.vsync_start = 1200 + 24,
> +	.vsync_end = 1200 + 24 + 6,
> +	.vtotal = 1200 + 24 + 6 + 48,
> +	.vrefresh = 60,
> +	.flags = DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC,
> +};
> +
> +static int osd101t2587_panel_get_modes(struct drm_panel *panel)
> +{
> +	struct osd101t2587_panel *osd101t2587 = to_osd101t2587_panel(panel);
> +	struct drm_display_mode *mode;
> +
> +	mode = drm_mode_duplicate(panel->drm, osd101t2587->default_mode);
> +	if (!mode) {
> +		dev_err(panel->drm->dev, "failed to add mode %ux%ux@%u\n",
> +			osd101t2587->default_mode->hdisplay,
> +			osd101t2587->default_mode->vdisplay,
> +			osd101t2587->default_mode->vrefresh);
> +		return -ENOMEM;
> +	}
> +
> +	drm_mode_set_name(mode);
> +
> +	drm_mode_probed_add(panel->connector, mode);
> +
> +	panel->connector->display_info.width_mm = 217;
> +	panel->connector->display_info.height_mm = 136;
> +
> +	return 1;
> +}
> +
> +static const struct drm_panel_funcs osd101t2587_panel_funcs = {
> +	.disable = osd101t2587_panel_disable,
> +	.unprepare = osd101t2587_panel_unprepare,
> +	.prepare = osd101t2587_panel_prepare,
> +	.enable = osd101t2587_panel_enable,
> +	.get_modes = osd101t2587_panel_get_modes,
> +};
> +
> +static const struct of_device_id osd101t2587_of_match[] = {
> +	{
> +		.compatible = "osd,osd101t2587-53ts",
> +		.data = &default_mode_osd101t2587,
> +	}, { }
Move { } to a separate line and use { /* sentinel */ } to make it look like other drivers.
 

> +};
> +MODULE_DEVICE_TABLE(of, osd101t2587_of_match);
> +
> +static int osd101t2587_panel_add(struct osd101t2587_panel *osd101t2587)
> +{
> +	struct device *dev = &osd101t2587->dsi->dev;
> +	struct device_node *np;
> +	int ret;
> +
> +	osd101t2587->mode = osd101t2587->default_mode;
> +
> +	osd101t2587->supply = devm_regulator_get(dev, "power");
> +	if (IS_ERR(osd101t2587->supply))
> +		return PTR_ERR(osd101t2587->supply);
> +
> +	np = of_parse_phandle(dev->of_node, "backlight", 0);
> +	if (np) {
> +		osd101t2587->backlight = of_find_backlight_by_node(np);
> +		of_node_put(np);
> +
> +		if (!osd101t2587->backlight)
> +			return -EPROBE_DEFER;
> +	}
Use devm_of_find_backlight() - this results in simpler code.

> +
> +	drm_panel_init(&osd101t2587->base);
> +	osd101t2587->base.funcs = &osd101t2587_panel_funcs;
> +	osd101t2587->base.dev = &osd101t2587->dsi->dev;
> +
> +	ret = drm_panel_add(&osd101t2587->base);
> +	if (ret < 0)
> +		goto put_backlight;
> +
> +	return 0;
> +
> +put_backlight:
> +	if (osd101t2587->backlight)
> +		put_device(&osd101t2587->backlight->dev);
When using devm_of_find_backlight() this is not needed.

> +
> +	return ret;
> +}
> +
> +static void osd101t2587_panel_del(struct osd101t2587_panel *osd101t2587)
> +{
> +	if (osd101t2587->base.dev)
> +		drm_panel_remove(&osd101t2587->base);
> +
> +	if (osd101t2587->backlight)
> +		put_device(&osd101t2587->backlight->dev);
When you use devm_of_find_backlight() then this is done for you, so the above can be deleted.

> +}
> +
> +static int osd101t2587_panel_probe(struct mipi_dsi_device *dsi)
> +{
> +	struct osd101t2587_panel *osd101t2587;
> +	const struct of_device_id *id;
> +	int ret;
> +
> +	id = of_match_node(osd101t2587_of_match, dsi->dev.of_node);
> +	if (!id)
> +		return -ENODEV;
> +
> +	dsi->lanes = 4;
> +	dsi->format = MIPI_DSI_FMT_RGB888;
> +	dsi->mode_flags = MIPI_DSI_MODE_VIDEO |
> +			  MIPI_DSI_MODE_VIDEO_BURST |
> +			  MIPI_DSI_MODE_VIDEO_SYNC_PULSE |
> +			  MIPI_DSI_MODE_EOT_PACKET;
> +
> +	osd101t2587 = devm_kzalloc(&dsi->dev, sizeof(*osd101t2587), GFP_KERNEL);
> +	if (!osd101t2587)
> +		return -ENOMEM;
> +
> +	mipi_dsi_set_drvdata(dsi, osd101t2587);
> +
> +	osd101t2587->dsi = dsi;
> +	osd101t2587->default_mode = id->data;
> +
> +	ret = osd101t2587_panel_add(osd101t2587);
> +	if (ret < 0)
> +		return ret;
> +
> +	return mipi_dsi_attach(dsi);
> +}
> +
> +static int osd101t2587_panel_remove(struct mipi_dsi_device *dsi)
> +{
> +	struct osd101t2587_panel *osd101t2587 = mipi_dsi_get_drvdata(dsi);
> +	int ret;
> +
> +	ret = osd101t2587_panel_disable(&osd101t2587->base);
> +	if (ret < 0)
> +		dev_err(&dsi->dev, "failed to disable panel: %d\n", ret);
> +
To trun off power supply call osd101t2587_panel_unprepare() here?

> +	ret = mipi_dsi_detach(dsi);
> +	if (ret < 0)
> +		dev_err(&dsi->dev, "failed to detach from DSI host: %d\n", ret);
> +
> +	osd101t2587_panel_del(osd101t2587);
Replace this with
	drm_panel_remove(osd101t2587->base);
This should be fine and no need to disable backlight, this is done in osd101t2587_panel_disable()

> +
> +	return 0;
> +}
> +
> +static void osd101t2587_panel_shutdown(struct mipi_dsi_device *dsi)
> +{
> +	struct osd101t2587_panel *osd101t2587 = mipi_dsi_get_drvdata(dsi);
> +
Maybe call osd101t2587_panel_unprepare() here to turn off power supply?

> +	osd101t2587_panel_disable(&osd101t2587->base);
> +}
> +
> +static struct mipi_dsi_driver osd101t2587_panel_driver = {
> +	.driver = {
> +		.name = "panel-osd-osd101t2587-53ts",
> +		.of_match_table = osd101t2587_of_match,
> +	},
> +	.probe = osd101t2587_panel_probe,
> +	.remove = osd101t2587_panel_remove,
> +	.shutdown = osd101t2587_panel_shutdown,
> +};
> +module_mipi_dsi_driver(osd101t2587_panel_driver);
> +
> +MODULE_AUTHOR("Peter Ujfalusi <peter.ujfalusi@ti.com>");
> +MODULE_DESCRIPTION("OSD101T2587-53TS DSI panel");
> +MODULE_LICENSE("GPL v2");
> -- 
> Peter
> 
> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/4] drm/panel: Add OSD101T2587-53TS driver

[Peter Ujfalusi]

Hi Sam,

On 15/02/2019 20.07, Sam Ravnborg wrote:
> Hi Peter.
> 
> Good with more panel drivers.
> Some comments in the following, please do not blindly follow them but
> check that this is OK.

First of all, thank you for the review and sorry for the delay!

> 
> 	Sam
> 
> On Fri, Feb 15, 2019 at 04:03:15PM +0200, Peter Ujfalusi via dri-devel wrote:
>> The panel is similar to OSD101T2045-53TS (which is handled by panel-simple)
>> with one big difference: osd101t2587-53ts needs MIPI_DSI_TURN_ON_PERIPHERAL
>> message to be sent from the host to be operational and thus can not be
>> handled by panel-simple.
>>
>> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
>> ---
>> +++ b/drivers/gpu/drm/panel/panel-osd-osd101t2587-53ts.c
>> @@ -0,0 +1,284 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + *  Copyright (C) 2018 Texas Instruments Incorporated - http://www.ti.com
>> + *  Author: Peter Ujfalusi <peter.ujfalusi@ti.com>
>> + */
>> +
>> +#include <linux/backlight.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/regulator/consumer.h>
>> +
>> +#include <drm/drmP.h>
> Please do not use drmP.h in new drivers - we try to get rid of this file.

OK.

> 
>> +#include <drm/drm_crtc.h>
>> +#include <drm/drm_mipi_dsi.h>
>> +#include <drm/drm_panel.h>
>> +
>> +#include <video/mipi_display.h>
>> +
>> +struct osd101t2587_panel {
>> +	struct drm_panel base;
>> +	struct mipi_dsi_device *dsi;
>> +
>> +	struct backlight_device *backlight;
>> +	struct regulator *supply;
>> +
>> +	bool prepared;
>> +	bool enabled;
>> +
>> +	ktime_t earliest_wake;
> earliest_wake is not used, and can be deleted.

Yes, it is leftover.

>> +
>> +	const struct drm_display_mode *default_mode;
>> +	const struct drm_display_mode *mode;
> Assigned but never used. Drop the assignment and the variable?

Hrm, you are right.

> 
>> +};
>> +
>> +static inline struct osd101t2587_panel *to_osd101t2587_panel(struct drm_panel *panel)
>> +{
>> +	return container_of(panel, struct osd101t2587_panel, base);
>> +}
>> +
>> +static int osd101t2587_panel_disable(struct drm_panel *panel)
>> +{
>> +	struct osd101t2587_panel *osd101t2587 = to_osd101t2587_panel(panel);
>> +	int ret;
>> +
>> +	if (!osd101t2587->enabled)
>> +		return 0;
>> +
>> +	if (osd101t2587->backlight) {
>> +		osd101t2587->backlight->props.power = FB_BLANK_POWERDOWN;
>> +		osd101t2587->backlight->props.state |= BL_CORE_FBBLANK;
>> +		backlight_update_status(osd101t2587->backlight);
>> +	}
> backlight_disable(osd101t2587->backlight);

OK

>> +
>> +	ret = mipi_dsi_shutdown_peripheral(osd101t2587->dsi);
>> +
>> +	osd101t2587->enabled = false;
>> +
>> +	return ret;
>> +}
>> +
>> +static int osd101t2587_panel_unprepare(struct drm_panel *panel)
>> +{
>> +	struct osd101t2587_panel *osd101t2587 = to_osd101t2587_panel(panel);
>> +
>> +	if (!osd101t2587->prepared)
>> +		return 0;
>> +
>> +	regulator_disable(osd101t2587->supply);
>> +	osd101t2587->prepared = false;
>> +
>> +	return 0;
>> +}
>> +
>> +static int osd101t2587_panel_prepare(struct drm_panel *panel)
>> +{
>> +	struct osd101t2587_panel *osd101t2587 = to_osd101t2587_panel(panel);
>> +	int ret;
>> +
>> +	if (osd101t2587->prepared)
>> +		return 0;
>> +
>> +	ret = regulator_enable(osd101t2587->supply);
>> +	if (!ret)
>> +		osd101t2587->prepared = true;
>> +
>> +	return ret;
>> +}
>> +
>> +static int osd101t2587_panel_enable(struct drm_panel *panel)
>> +{
>> +	struct osd101t2587_panel *osd101t2587 = to_osd101t2587_panel(panel);
>> +	int ret;
>> +
>> +	if (osd101t2587->enabled)
>> +		return 0;
>> +
>> +	ret = mipi_dsi_turn_on_peripheral(osd101t2587->dsi);
>> +	if (ret)
>> +		return ret;
>> +
>> +	if (osd101t2587->backlight) {
>> +		osd101t2587->backlight->props.power = FB_BLANK_UNBLANK;
>> +		osd101t2587->backlight->props.state &= ~BL_CORE_FBBLANK;
>> +		backlight_update_status(osd101t2587->backlight);
>> +	}
> backlight_enable(osd101t2587->backlight);
> can be used here.

Yes, it can be used.

> 
>> +
>> +	osd101t2587->enabled = true;
>> +
>> +	return ret;
>> +}
>> +
>> +static const struct drm_display_mode default_mode_osd101t2587 = {
>> +	.clock = 164400,
>> +	.hdisplay = 1920,
>> +	.hsync_start = 1920 + 152,
>> +	.hsync_end = 1920 + 152 + 52,
>> +	.htotal = 1920 + 152 + 52 + 20,
>> +	.vdisplay = 1200,
>> +	.vsync_start = 1200 + 24,
>> +	.vsync_end = 1200 + 24 + 6,
>> +	.vtotal = 1200 + 24 + 6 + 48,
>> +	.vrefresh = 60,
>> +	.flags = DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC,
>> +};
>> +
>> +static int osd101t2587_panel_get_modes(struct drm_panel *panel)
>> +{
>> +	struct osd101t2587_panel *osd101t2587 = to_osd101t2587_panel(panel);
>> +	struct drm_display_mode *mode;
>> +
>> +	mode = drm_mode_duplicate(panel->drm, osd101t2587->default_mode);
>> +	if (!mode) {
>> +		dev_err(panel->drm->dev, "failed to add mode %ux%ux@%u\n",
>> +			osd101t2587->default_mode->hdisplay,
>> +			osd101t2587->default_mode->vdisplay,
>> +			osd101t2587->default_mode->vrefresh);
>> +		return -ENOMEM;
>> +	}
>> +
>> +	drm_mode_set_name(mode);
>> +
>> +	drm_mode_probed_add(panel->connector, mode);
>> +
>> +	panel->connector->display_info.width_mm = 217;
>> +	panel->connector->display_info.height_mm = 136;
>> +
>> +	return 1;
>> +}
>> +
>> +static const struct drm_panel_funcs osd101t2587_panel_funcs = {
>> +	.disable = osd101t2587_panel_disable,
>> +	.unprepare = osd101t2587_panel_unprepare,
>> +	.prepare = osd101t2587_panel_prepare,
>> +	.enable = osd101t2587_panel_enable,
>> +	.get_modes = osd101t2587_panel_get_modes,
>> +};
>> +
>> +static const struct of_device_id osd101t2587_of_match[] = {
>> +	{
>> +		.compatible = "osd,osd101t2587-53ts",
>> +		.data = &default_mode_osd101t2587,
>> +	}, { }
> Move { } to a separate line and use { /* sentinel */ } to make it look like other drivers.
>  
> 
>> +};
>> +MODULE_DEVICE_TABLE(of, osd101t2587_of_match);
>> +
>> +static int osd101t2587_panel_add(struct osd101t2587_panel *osd101t2587)
>> +{
>> +	struct device *dev = &osd101t2587->dsi->dev;
>> +	struct device_node *np;
>> +	int ret;
>> +
>> +	osd101t2587->mode = osd101t2587->default_mode;
>> +
>> +	osd101t2587->supply = devm_regulator_get(dev, "power");
>> +	if (IS_ERR(osd101t2587->supply))
>> +		return PTR_ERR(osd101t2587->supply);
>> +
>> +	np = of_parse_phandle(dev->of_node, "backlight", 0);
>> +	if (np) {
>> +		osd101t2587->backlight = of_find_backlight_by_node(np);
>> +		of_node_put(np);
>> +
>> +		if (!osd101t2587->backlight)
>> +			return -EPROBE_DEFER;
>> +	}
> Use devm_of_find_backlight() - this results in simpler code.

Yes, it certainly makes the code cleaner.

> 
>> +
>> +	drm_panel_init(&osd101t2587->base);
>> +	osd101t2587->base.funcs = &osd101t2587_panel_funcs;
>> +	osd101t2587->base.dev = &osd101t2587->dsi->dev;
>> +
>> +	ret = drm_panel_add(&osd101t2587->base);
>> +	if (ret < 0)
>> +		goto put_backlight;
>> +
>> +	return 0;
>> +
>> +put_backlight:
>> +	if (osd101t2587->backlight)
>> +		put_device(&osd101t2587->backlight->dev);
> When using devm_of_find_backlight() this is not needed.

OK.

> 
>> +
>> +	return ret;
>> +}
>> +
>> +static void osd101t2587_panel_del(struct osd101t2587_panel *osd101t2587)
>> +{
>> +	if (osd101t2587->base.dev)
>> +		drm_panel_remove(&osd101t2587->base);
>> +
>> +	if (osd101t2587->backlight)
>> +		put_device(&osd101t2587->backlight->dev);
> When you use devm_of_find_backlight() then this is done for you, so the above can be deleted.

OK.

>> +}
>> +
>> +static int osd101t2587_panel_probe(struct mipi_dsi_device *dsi)
>> +{
>> +	struct osd101t2587_panel *osd101t2587;
>> +	const struct of_device_id *id;
>> +	int ret;
>> +
>> +	id = of_match_node(osd101t2587_of_match, dsi->dev.of_node);
>> +	if (!id)
>> +		return -ENODEV;
>> +
>> +	dsi->lanes = 4;
>> +	dsi->format = MIPI_DSI_FMT_RGB888;
>> +	dsi->mode_flags = MIPI_DSI_MODE_VIDEO |
>> +			  MIPI_DSI_MODE_VIDEO_BURST |
>> +			  MIPI_DSI_MODE_VIDEO_SYNC_PULSE |
>> +			  MIPI_DSI_MODE_EOT_PACKET;
>> +
>> +	osd101t2587 = devm_kzalloc(&dsi->dev, sizeof(*osd101t2587), GFP_KERNEL);
>> +	if (!osd101t2587)
>> +		return -ENOMEM;
>> +
>> +	mipi_dsi_set_drvdata(dsi, osd101t2587);
>> +
>> +	osd101t2587->dsi = dsi;
>> +	osd101t2587->default_mode = id->data;
>> +
>> +	ret = osd101t2587_panel_add(osd101t2587);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	return mipi_dsi_attach(dsi);
>> +}
>> +
>> +static int osd101t2587_panel_remove(struct mipi_dsi_device *dsi)
>> +{
>> +	struct osd101t2587_panel *osd101t2587 = mipi_dsi_get_drvdata(dsi);
>> +	int ret;
>> +
>> +	ret = osd101t2587_panel_disable(&osd101t2587->base);
>> +	if (ret < 0)
>> +		dev_err(&dsi->dev, "failed to disable panel: %d\n", ret);
>> +
> To trun off power supply call osd101t2587_panel_unprepare() here?
> 
>> +	ret = mipi_dsi_detach(dsi);
>> +	if (ret < 0)
>> +		dev_err(&dsi->dev, "failed to detach from DSI host: %d\n", ret);
>> +
>> +	osd101t2587_panel_del(osd101t2587);
> Replace this with
> 	drm_panel_remove(osd101t2587->base);
> This should be fine and no need to disable backlight, this is done in osd101t2587_panel_disable()

OK.

>> +
>> +	return 0;
>> +}
>> +
>> +static void osd101t2587_panel_shutdown(struct mipi_dsi_device *dsi)
>> +{
>> +	struct osd101t2587_panel *osd101t2587 = mipi_dsi_get_drvdata(dsi);
>> +
> Maybe call osd101t2587_panel_unprepare() here to turn off power supply?

Make sense, in this order:
	osd101t2587_panel_disable(&osd101t2587->base);
	osd101t2587_panel_unprepare(&osd101t2587->base);

But should the osd101t2587_panel_remove() do the same thing? or the
osd101t2587_panel_disable() is redundant in the osd101t2587_panel_remove()?

Regards,
- Péter

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [PATCH 4/4] drm/panel: Add OSD101T2587-53TS driver

[Peter Ujfalusi]

Hi Sam,

On 15/02/2019 20.07, Sam Ravnborg wrote:
>> +#include <linux/backlight.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/regulator/consumer.h>
>> +
>> +#include <drm/drmP.h>
> Please do not use drmP.h in new drivers - we try to get rid of this file.

...

>> +static int osd101t2587_panel_get_modes(struct drm_panel *panel)
>> +{
>> +	struct osd101t2587_panel *osd101t2587 = to_osd101t2587_panel(panel);
>> +	struct drm_display_mode *mode;
>> +
>> +	mode = drm_mode_duplicate(panel->drm, osd101t2587->default_mode);
>> +	if (!mode) {
>> +		dev_err(panel->drm->dev, "failed to add mode %ux%ux@%u\n",

drm/drmP.h is needed for this dev_err.

Regards,
- Péter

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [PATCH 4/4] drm/panel: Add OSD101T2587-53TS driver

[Sam Ravnborg]

Hi Peter.

On Wed, Feb 20, 2019 at 12:39:11PM +0200, Peter Ujfalusi wrote:
> Hi Sam,
> 
> On 15/02/2019 20.07, Sam Ravnborg wrote:
> >> +#include <linux/backlight.h>
> >> +#include <linux/module.h>
> >> +#include <linux/of.h>
> >> +#include <linux/regulator/consumer.h>
> >> +
> >> +#include <drm/drmP.h>
> > Please do not use drmP.h in new drivers - we try to get rid of this file.
> 
> ...
> 
> >> +static int osd101t2587_panel_get_modes(struct drm_panel *panel)
> >> +{
> >> +	struct osd101t2587_panel *osd101t2587 = to_osd101t2587_panel(panel);
> >> +	struct drm_display_mode *mode;
> >> +
> >> +	mode = drm_mode_duplicate(panel->drm, osd101t2587->default_mode);
> >> +	if (!mode) {
> >> +		dev_err(panel->drm->dev, "failed to add mode %ux%ux@%u\n",
> 
> drm/drmP.h is needed for this dev_err.
drmP.h is only a set of include files and forwards today.
So you need to figure out what to replace it with.

Often removing drmP.h requires you to add more than one extra include file.

	Sam

Re: [PATCH 4/4] drm/panel: Add OSD101T2587-53TS driver

[Peter Ujfalusi]

Hi Sam,

On 20/02/2019 13.52, Sam Ravnborg wrote:
> Hi Peter.
> 
> On Wed, Feb 20, 2019 at 12:39:11PM +0200, Peter Ujfalusi wrote:
>> Hi Sam,
>>
>> On 15/02/2019 20.07, Sam Ravnborg wrote:
>>>> +#include <linux/backlight.h>
>>>> +#include <linux/module.h>
>>>> +#include <linux/of.h>
>>>> +#include <linux/regulator/consumer.h>
>>>> +
>>>> +#include <drm/drmP.h>
>>> Please do not use drmP.h in new drivers - we try to get rid of this file.
>>
>> ...
>>
>>>> +static int osd101t2587_panel_get_modes(struct drm_panel *panel)
>>>> +{
>>>> +	struct osd101t2587_panel *osd101t2587 = to_osd101t2587_panel(panel);
>>>> +	struct drm_display_mode *mode;
>>>> +
>>>> +	mode = drm_mode_duplicate(panel->drm, osd101t2587->default_mode);
>>>> +	if (!mode) {
>>>> +		dev_err(panel->drm->dev, "failed to add mode %ux%ux@%u\n",
>>
>> drm/drmP.h is needed for this dev_err.
> drmP.h is only a set of include files and forwards today.
> So you need to figure out what to replace it with.
> 
> Often removing drmP.h requires you to add more than one extra include file.

Replacing it with
#include <drm/drm_device.h>

works perfectly.

Thanks,
- Péter

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [PATCH 4/4] drm/panel: Add OSD101T2587-53TS driver

[Sam Ravnborg]

Hi Peter.

Always good to see that feedback input is used.
 
> OK.
> 
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +static void osd101t2587_panel_shutdown(struct mipi_dsi_device *dsi)
> >> +{
> >> +	struct osd101t2587_panel *osd101t2587 = mipi_dsi_get_drvdata(dsi);
> >> +
> > Maybe call osd101t2587_panel_unprepare() here to turn off power supply?
> 
> Make sense, in this order:
> 	osd101t2587_panel_disable(&osd101t2587->base);
> 	osd101t2587_panel_unprepare(&osd101t2587->base);
> 
> But should the osd101t2587_panel_remove() do the same thing? or the
> osd101t2587_panel_disable() is redundant in the osd101t2587_panel_remove()?

I do not know the details to answer this.
In other words - I do not know if we can rely on that panel->disbale is always
called when a driver is removed.
Try to read the descriptions and maybe test it.

Other drivers do as far as I recall use disable in the remove function.

	Sam