diff mbox series

drm/panel-notatek-nt35510: Fix enable/disable sequence

Message ID 20200805142808.670451-1-linus.walleij@linaro.org (mailing list archive)
State New, archived
Headers show
Series drm/panel-notatek-nt35510: Fix enable/disable sequence | expand

Commit Message

Linus Walleij Aug. 5, 2020, 2:28 p.m. UTC
The driver was relying on only prepare()/unprepare() to
enable/disable the display.

This does not work because prepare() will be called
before the DSI host/bridge is ready to send any DSI
commands and disable() will be called after the DSI
host/bridge is disabled and thus unable to send any
DSI commands.

Move all DCS command sending to the enable() and
disable() callbacks, as is proper.

The prepare() and unprepare() is now only used to
enable and disable the regulators() and
asserting/de-asserting the reset line so we inline the
regulator and reset code here.

While developing this it was discovered that during
powercycling the device looses its ability to read
the MTP ID:s. We were lucky before as the display
came up with MTP reading enabled, but as this makes
powercycling work, we also need to send two small
sequences that enable the reading of the MTP ID
after powering on the regulators.

This makes it possible to use the user space UI
stack Phosh on the Samsung GT-S7710 as Phosh
enables/disables the display when starting.

Rename the nt35510_power_on() function to
nt35510_power_on_sequence() to reflect the fact
that this is not really about regulators but a
DCS command sequence.

Cc: Stephan Gerhold <stephan@gerhold.net>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/gpu/drm/panel/panel-novatek-nt35510.c | 107 +++++++++++-------
 1 file changed, 68 insertions(+), 39 deletions(-)

Comments

Linus Walleij Aug. 8, 2020, 10:03 a.m. UTC | #1
On Wed, Aug 5, 2020 at 4:28 PM Linus Walleij <linus.walleij@linaro.org> wrote:

> The driver was relying on only prepare()/unprepare() to
> enable/disable the display.
>
> This does not work because prepare() will be called
> before the DSI host/bridge is ready to send any DSI
> commands and disable() will be called after the DSI
> host/bridge is disabled and thus unable to send any
> DSI commands.
>
> Move all DCS command sending to the enable() and
> disable() callbacks, as is proper.

Ahem this is against how all panel drivers are written.
Surely the DSI host must be up and running at
prepare() and unprepare() time.

I take this patch back to the drawing table.

Yours,
Linus Walleij
diff mbox series

Patch

diff --git a/drivers/gpu/drm/panel/panel-novatek-nt35510.c b/drivers/gpu/drm/panel/panel-novatek-nt35510.c
index 4a8fa908a2cf..39113601e944 100644
--- a/drivers/gpu/drm/panel/panel-novatek-nt35510.c
+++ b/drivers/gpu/drm/panel/panel-novatek-nt35510.c
@@ -376,6 +376,10 @@  struct nt35510 {
 };
 
 /* Manufacturer command has strictly this byte sequence */
+static const u8 nt35510_mauc_mtp_read_param[] = { 0xAA, 0x55, 0x25, 0x01 };
+static const u8 nt35510_mauc_mtp_read_setting[] = { 0x01, 0x02, 0x00, 0x20,
+						    0x33, 0x13, 0x00, 0x40,
+						    0x00, 0x00, 0x23, 0x02 };
 static const u8 nt35510_mauc_select_page_0[] = { 0x55, 0xAA, 0x52, 0x08, 0x00 };
 static const u8 nt35510_mauc_select_page_1[] = { 0x55, 0xAA, 0x52, 0x08, 0x01 };
 static const u8 nt35510_vgh_on[] = { 0x01 };
@@ -674,29 +678,22 @@  static const struct backlight_ops nt35510_bl_ops = {
 /*
  * This power-on sequence
  */
-static int nt35510_power_on(struct nt35510 *nt)
+static int nt35510_power_on_sequence(struct nt35510 *nt)
 {
 	struct mipi_dsi_device *dsi = to_mipi_dsi_device(nt->dev);
 	int ret;
 
-	ret = regulator_bulk_enable(ARRAY_SIZE(nt->supplies), nt->supplies);
-	if (ret < 0) {
-		dev_err(nt->dev, "unable to enable regulators\n");
+	ret = nt35510_send_long(nt, dsi, MCS_CMD_MTP_READ_PARAM,
+				ARRAY_SIZE(nt35510_mauc_mtp_read_param),
+				nt35510_mauc_mtp_read_param);
+	if (ret)
 		return ret;
-	}
 
-	/* Toggle RESET in accordance with datasheet page 370 */
-	if (nt->reset_gpio) {
-		gpiod_set_value(nt->reset_gpio, 1);
-		/* Active min 10 us according to datasheet, let's say 20 */
-		usleep_range(20, 1000);
-		gpiod_set_value(nt->reset_gpio, 0);
-		/*
-		 * 5 ms during sleep mode, 120 ms during sleep out mode
-		 * according to datasheet, let's use 120-140 ms.
-		 */
-		usleep_range(120000, 140000);
-	}
+	ret = nt35510_send_long(nt, dsi, MCS_CMD_MTP_READ_SETTING,
+				ARRAY_SIZE(nt35510_mauc_mtp_read_setting),
+				nt35510_mauc_mtp_read_setting);
+	if (ret)
+		return ret;
 
 	ret = nt35510_read_id(nt);
 	if (ret)
@@ -758,26 +755,16 @@  static int nt35510_power_on(struct nt35510 *nt)
 	return 0;
 }
 
-static int nt35510_power_off(struct nt35510 *nt)
-{
-	int ret;
-
-	ret = regulator_bulk_disable(ARRAY_SIZE(nt->supplies), nt->supplies);
-	if (ret)
-		return ret;
-
-	if (nt->reset_gpio)
-		gpiod_set_value(nt->reset_gpio, 1);
-
-	return 0;
-}
-
-static int nt35510_unprepare(struct drm_panel *panel)
+static int nt35510_disable(struct drm_panel *panel)
 {
 	struct nt35510 *nt = panel_to_nt35510(panel);
 	struct mipi_dsi_device *dsi = to_mipi_dsi_device(nt->dev);
 	int ret;
 
+	/*
+	 * The final DCS traffic must happen here, at unprepare() the
+	 * DSI host will not accept traffic any more.
+	 */
 	ret = mipi_dsi_dcs_set_display_off(dsi);
 	if (ret) {
 		DRM_DEV_ERROR(nt->dev, "failed to turn display off (%d)\n",
@@ -797,20 +784,36 @@  static int nt35510_unprepare(struct drm_panel *panel)
 	/* Wait 4 frames, how much is that 5ms in the vendor driver */
 	usleep_range(5000, 10000);
 
-	ret = nt35510_power_off(nt);
+	return 0;
+}
+
+static int nt35510_unprepare(struct drm_panel *panel)
+{
+	struct nt35510 *nt = panel_to_nt35510(panel);
+	int ret;
+
+	ret = regulator_bulk_disable(ARRAY_SIZE(nt->supplies), nt->supplies);
 	if (ret)
 		return ret;
 
+	if (nt->reset_gpio)
+		gpiod_set_value(nt->reset_gpio, 1);
+
+
 	return 0;
 }
 
-static int nt35510_prepare(struct drm_panel *panel)
+static int nt35510_enable(struct drm_panel *panel)
 {
 	struct nt35510 *nt = panel_to_nt35510(panel);
 	struct mipi_dsi_device *dsi = to_mipi_dsi_device(nt->dev);
 	int ret;
 
-	ret = nt35510_power_on(nt);
+	/*
+	 * At this point the DSI host is up and clocked and we can start
+	 * sending DCS commands.
+	 */
+	ret = nt35510_power_on_sequence(nt);
 	if (ret)
 		return ret;
 
@@ -836,6 +839,33 @@  static int nt35510_prepare(struct drm_panel *panel)
 	return 0;
 }
 
+static int nt35510_prepare(struct drm_panel *panel)
+{
+	struct nt35510 *nt = panel_to_nt35510(panel);
+	int ret;
+
+	ret = regulator_bulk_enable(ARRAY_SIZE(nt->supplies), nt->supplies);
+	if (ret < 0) {
+		dev_err(nt->dev, "unable to enable regulators\n");
+		return ret;
+	}
+
+	/* Toggle RESET in accordance with datasheet page 370 */
+	if (nt->reset_gpio) {
+		gpiod_set_value(nt->reset_gpio, 1);
+		/* Active min 10 us according to datasheet, let's say 20 */
+		usleep_range(20, 1000);
+		gpiod_set_value(nt->reset_gpio, 0);
+		/*
+		 * 5 ms during sleep mode, 120 ms during sleep out mode
+		 * according to datasheet, let's use 120-140 ms.
+		 */
+		usleep_range(120000, 140000);
+	}
+
+	return 0;
+}
+
 static int nt35510_get_modes(struct drm_panel *panel,
 			     struct drm_connector *connector)
 {
@@ -862,6 +892,8 @@  static int nt35510_get_modes(struct drm_panel *panel,
 }
 
 static const struct drm_panel_funcs nt35510_drm_funcs = {
+	.enable = nt35510_enable,
+	.disable = nt35510_disable,
 	.unprepare = nt35510_unprepare,
 	.prepare = nt35510_prepare,
 	.get_modes = nt35510_get_modes,
@@ -970,14 +1002,11 @@  static int nt35510_probe(struct mipi_dsi_device *dsi)
 static int nt35510_remove(struct mipi_dsi_device *dsi)
 {
 	struct nt35510 *nt = mipi_dsi_get_drvdata(dsi);
-	int ret;
 
 	mipi_dsi_detach(dsi);
-	/* Power off */
-	ret = nt35510_power_off(nt);
 	drm_panel_remove(&nt->panel);
 
-	return ret;
+	return 0;
 }
 
 /*