1. Patchew
  2. linux
  3. View series

[PATCH v2] drm/panel: raydium-rm692e5: transition to mipi_dsi wrapped functions

Tejas Vipin posted 1 patch 1 year, 6 months ago
There is a newer version of this series
drivers/gpu/drm/panel/panel-raydium-rm692e5.c | 237 ++++++++----------
1 file changed, 99 insertions(+), 138 deletions(-)
[PATCH v2] drm/panel: raydium-rm692e5: transition to mipi_dsi wrapped functions
Posted by Tejas Vipin 1 year, 6 months ago 
Use functions introduced in commit 966e397e4f60 ("drm/mipi-dsi: Introduce
mipi_dsi_*_write_seq_multi()") and commit f79d6d28d8fe
("drm/mipi-dsi: wrap more functions for streamline handling") for the
raydium rm692e5 panel.

Signed-off-by: Tejas Vipin <tejasvipin76@gmail.com>
---
Changes in v2:
    - Change rm692e5_on to return void and take mipi_dsi_multi_context
      as an argument.
    - Remove unnecessary warnings.
    - More efficient error handling in rm692e5_prepare

v1: https://lore.kernel.org/all/20240615093758.65431-1-tejasvipin76@gmail.com/
---
 drivers/gpu/drm/panel/panel-raydium-rm692e5.c | 237 ++++++++----------
 1 file changed, 99 insertions(+), 138 deletions(-)

diff --git a/drivers/gpu/drm/panel/panel-raydium-rm692e5.c b/drivers/gpu/drm/panel/panel-raydium-rm692e5.c
index 21d97f6b8a2f..9936bda61af2 100644
--- a/drivers/gpu/drm/panel/panel-raydium-rm692e5.c
+++ b/drivers/gpu/drm/panel/panel-raydium-rm692e5.c
@@ -40,176 +40,137 @@ static void rm692e5_reset(struct rm692e5_panel *ctx)
 	usleep_range(10000, 11000);
 }
 
-static int rm692e5_on(struct rm692e5_panel *ctx)
+static void rm692e5_on(struct mipi_dsi_multi_context *dsi_ctx)
 {
-	struct mipi_dsi_device *dsi = ctx->dsi;
-	struct device *dev = &dsi->dev;
-	int ret;
+	dsi_ctx->dsi->mode_flags |= MIPI_DSI_MODE_LPM;
+
+	mipi_dsi_generic_write_seq_multi(dsi_ctx, 0xfe, 0x41);
+	mipi_dsi_generic_write_seq_multi(dsi_ctx, 0xd6, 0x00);
+	mipi_dsi_generic_write_seq_multi(dsi_ctx, 0xfe, 0x16);
+	mipi_dsi_generic_write_seq_multi(dsi_ctx, 0x8a, 0x87);
+	mipi_dsi_generic_write_seq_multi(dsi_ctx, 0xfe, 0x71);
+	mipi_dsi_generic_write_seq_multi(dsi_ctx, 0x82, 0x01);
+	mipi_dsi_generic_write_seq_multi(dsi_ctx, 0xc6, 0x00);
+	mipi_dsi_generic_write_seq_multi(dsi_ctx, 0xc7, 0x2c);
+	mipi_dsi_generic_write_seq_multi(dsi_ctx, 0xc8, 0x64);
+	mipi_dsi_generic_write_seq_multi(dsi_ctx, 0xc9, 0x3c);
+	mipi_dsi_generic_write_seq_multi(dsi_ctx, 0xca, 0x80);
+	mipi_dsi_generic_write_seq_multi(dsi_ctx, 0xcb, 0x02);
+	mipi_dsi_generic_write_seq_multi(dsi_ctx, 0xcc, 0x02);
+	mipi_dsi_generic_write_seq_multi(dsi_ctx, 0xfe, 0x38);
+	mipi_dsi_generic_write_seq_multi(dsi_ctx, 0x18, 0x13);
+	mipi_dsi_generic_write_seq_multi(dsi_ctx, 0xfe, 0xf4);
+	mipi_dsi_generic_write_seq_multi(dsi_ctx, 0x00, 0xff);
+	mipi_dsi_generic_write_seq_multi(dsi_ctx, 0x01, 0xff);
+	mipi_dsi_generic_write_seq_multi(dsi_ctx, 0x02, 0xcf);
+	mipi_dsi_generic_write_seq_multi(dsi_ctx, 0x03, 0xbc);
+	mipi_dsi_generic_write_seq_multi(dsi_ctx, 0x04, 0xb9);
+	mipi_dsi_generic_write_seq_multi(dsi_ctx, 0x05, 0x99);
+	mipi_dsi_generic_write_seq_multi(dsi_ctx, 0x06, 0x02);
+	mipi_dsi_generic_write_seq_multi(dsi_ctx, 0x07, 0x0a);
+	mipi_dsi_generic_write_seq_multi(dsi_ctx, 0x08, 0xe0);
+	mipi_dsi_generic_write_seq_multi(dsi_ctx, 0x09, 0x4c);
+	mipi_dsi_generic_write_seq_multi(dsi_ctx, 0x0a, 0xeb);
+	mipi_dsi_generic_write_seq_multi(dsi_ctx, 0x0b, 0xe8);
+	mipi_dsi_generic_write_seq_multi(dsi_ctx, 0x0c, 0x32);
+	mipi_dsi_generic_write_seq_multi(dsi_ctx, 0x0d, 0x07);
+	mipi_dsi_generic_write_seq_multi(dsi_ctx, 0xfe, 0xf4);
+	mipi_dsi_generic_write_seq_multi(dsi_ctx, 0x0d, 0xc0);
+	mipi_dsi_generic_write_seq_multi(dsi_ctx, 0x0e, 0xff);
+	mipi_dsi_generic_write_seq_multi(dsi_ctx, 0x0f, 0xff);
+	mipi_dsi_generic_write_seq_multi(dsi_ctx, 0x10, 0x33);
+	mipi_dsi_generic_write_seq_multi(dsi_ctx, 0x11, 0x6f);
+	mipi_dsi_generic_write_seq_multi(dsi_ctx, 0x12, 0x6e);
+	mipi_dsi_generic_write_seq_multi(dsi_ctx, 0x13, 0xa6);
+	mipi_dsi_generic_write_seq_multi(dsi_ctx, 0x14, 0x80);
+	mipi_dsi_generic_write_seq_multi(dsi_ctx, 0x15, 0x02);
+	mipi_dsi_generic_write_seq_multi(dsi_ctx, 0x16, 0x38);
+	mipi_dsi_generic_write_seq_multi(dsi_ctx, 0x17, 0xd3);
+	mipi_dsi_generic_write_seq_multi(dsi_ctx, 0x18, 0x3a);
+	mipi_dsi_generic_write_seq_multi(dsi_ctx, 0x19, 0xba);
+	mipi_dsi_generic_write_seq_multi(dsi_ctx, 0x1a, 0xcc);
+	mipi_dsi_generic_write_seq_multi(dsi_ctx, 0x1b, 0x01);
+
+	mipi_dsi_dcs_nop_multi(dsi_ctx);
+
+	mipi_dsi_msleep(dsi_ctx, 32);
+
+	mipi_dsi_generic_write_seq_multi(dsi_ctx, 0xfe, 0x38);
+	mipi_dsi_generic_write_seq_multi(dsi_ctx, 0x18, 0x13);
+	mipi_dsi_generic_write_seq_multi(dsi_ctx, 0xfe, 0xd1);
+	mipi_dsi_generic_write_seq_multi(dsi_ctx, 0xd3, 0x00);
+	mipi_dsi_generic_write_seq_multi(dsi_ctx, 0xd0, 0x00);
+	mipi_dsi_generic_write_seq_multi(dsi_ctx, 0xd2, 0x00);
+	mipi_dsi_generic_write_seq_multi(dsi_ctx, 0xd4, 0x00);
+	mipi_dsi_generic_write_seq_multi(dsi_ctx, 0xb4, 0x01);
+	mipi_dsi_generic_write_seq_multi(dsi_ctx, 0xfe, 0xf9);
+	mipi_dsi_generic_write_seq_multi(dsi_ctx, 0x00, 0xaf);
+	mipi_dsi_generic_write_seq_multi(dsi_ctx, 0x1d, 0x37);
+	mipi_dsi_generic_write_seq_multi(dsi_ctx, 0x44, 0x0a, 0x7b);
+	mipi_dsi_generic_write_seq_multi(dsi_ctx, 0xfe, 0x00);
+	mipi_dsi_generic_write_seq_multi(dsi_ctx, 0xfa, 0x01);
+	mipi_dsi_generic_write_seq_multi(dsi_ctx, 0xc2, 0x08);
+	mipi_dsi_generic_write_seq_multi(dsi_ctx, 0x35, 0x00);
+	mipi_dsi_generic_write_seq_multi(dsi_ctx, 0x51, 0x05, 0x42);
+
+	mipi_dsi_dcs_exit_sleep_mode_multi(dsi_ctx);
+	mipi_dsi_msleep(dsi_ctx, 100);
+	mipi_dsi_dcs_set_display_on_multi(dsi_ctx);
 
-	dsi->mode_flags |= MIPI_DSI_MODE_LPM;
-
-	mipi_dsi_generic_write_seq(dsi, 0xfe, 0x41);
-	mipi_dsi_generic_write_seq(dsi, 0xd6, 0x00);
-	mipi_dsi_generic_write_seq(dsi, 0xfe, 0x16);
-	mipi_dsi_generic_write_seq(dsi, 0x8a, 0x87);
-	mipi_dsi_generic_write_seq(dsi, 0xfe, 0x71);
-	mipi_dsi_generic_write_seq(dsi, 0x82, 0x01);
-	mipi_dsi_generic_write_seq(dsi, 0xc6, 0x00);
-	mipi_dsi_generic_write_seq(dsi, 0xc7, 0x2c);
-	mipi_dsi_generic_write_seq(dsi, 0xc8, 0x64);
-	mipi_dsi_generic_write_seq(dsi, 0xc9, 0x3c);
-	mipi_dsi_generic_write_seq(dsi, 0xca, 0x80);
-	mipi_dsi_generic_write_seq(dsi, 0xcb, 0x02);
-	mipi_dsi_generic_write_seq(dsi, 0xcc, 0x02);
-	mipi_dsi_generic_write_seq(dsi, 0xfe, 0x38);
-	mipi_dsi_generic_write_seq(dsi, 0x18, 0x13);
-	mipi_dsi_generic_write_seq(dsi, 0xfe, 0xf4);
-	mipi_dsi_generic_write_seq(dsi, 0x00, 0xff);
-	mipi_dsi_generic_write_seq(dsi, 0x01, 0xff);
-	mipi_dsi_generic_write_seq(dsi, 0x02, 0xcf);
-	mipi_dsi_generic_write_seq(dsi, 0x03, 0xbc);
-	mipi_dsi_generic_write_seq(dsi, 0x04, 0xb9);
-	mipi_dsi_generic_write_seq(dsi, 0x05, 0x99);
-	mipi_dsi_generic_write_seq(dsi, 0x06, 0x02);
-	mipi_dsi_generic_write_seq(dsi, 0x07, 0x0a);
-	mipi_dsi_generic_write_seq(dsi, 0x08, 0xe0);
-	mipi_dsi_generic_write_seq(dsi, 0x09, 0x4c);
-	mipi_dsi_generic_write_seq(dsi, 0x0a, 0xeb);
-	mipi_dsi_generic_write_seq(dsi, 0x0b, 0xe8);
-	mipi_dsi_generic_write_seq(dsi, 0x0c, 0x32);
-	mipi_dsi_generic_write_seq(dsi, 0x0d, 0x07);
-	mipi_dsi_generic_write_seq(dsi, 0xfe, 0xf4);
-	mipi_dsi_generic_write_seq(dsi, 0x0d, 0xc0);
-	mipi_dsi_generic_write_seq(dsi, 0x0e, 0xff);
-	mipi_dsi_generic_write_seq(dsi, 0x0f, 0xff);
-	mipi_dsi_generic_write_seq(dsi, 0x10, 0x33);
-	mipi_dsi_generic_write_seq(dsi, 0x11, 0x6f);
-	mipi_dsi_generic_write_seq(dsi, 0x12, 0x6e);
-	mipi_dsi_generic_write_seq(dsi, 0x13, 0xa6);
-	mipi_dsi_generic_write_seq(dsi, 0x14, 0x80);
-	mipi_dsi_generic_write_seq(dsi, 0x15, 0x02);
-	mipi_dsi_generic_write_seq(dsi, 0x16, 0x38);
-	mipi_dsi_generic_write_seq(dsi, 0x17, 0xd3);
-	mipi_dsi_generic_write_seq(dsi, 0x18, 0x3a);
-	mipi_dsi_generic_write_seq(dsi, 0x19, 0xba);
-	mipi_dsi_generic_write_seq(dsi, 0x1a, 0xcc);
-	mipi_dsi_generic_write_seq(dsi, 0x1b, 0x01);
-
-	ret = mipi_dsi_dcs_nop(dsi);
-	if (ret < 0) {
-		dev_err(dev, "Failed to nop: %d\n", ret);
-		return ret;
-	}
-	msleep(32);
-
-	mipi_dsi_generic_write_seq(dsi, 0xfe, 0x38);
-	mipi_dsi_generic_write_seq(dsi, 0x18, 0x13);
-	mipi_dsi_generic_write_seq(dsi, 0xfe, 0xd1);
-	mipi_dsi_generic_write_seq(dsi, 0xd3, 0x00);
-	mipi_dsi_generic_write_seq(dsi, 0xd0, 0x00);
-	mipi_dsi_generic_write_seq(dsi, 0xd2, 0x00);
-	mipi_dsi_generic_write_seq(dsi, 0xd4, 0x00);
-	mipi_dsi_generic_write_seq(dsi, 0xb4, 0x01);
-	mipi_dsi_generic_write_seq(dsi, 0xfe, 0xf9);
-	mipi_dsi_generic_write_seq(dsi, 0x00, 0xaf);
-	mipi_dsi_generic_write_seq(dsi, 0x1d, 0x37);
-	mipi_dsi_generic_write_seq(dsi, 0x44, 0x0a, 0x7b);
-	mipi_dsi_generic_write_seq(dsi, 0xfe, 0x00);
-	mipi_dsi_generic_write_seq(dsi, 0xfa, 0x01);
-	mipi_dsi_generic_write_seq(dsi, 0xc2, 0x08);
-	mipi_dsi_generic_write_seq(dsi, 0x35, 0x00);
-	mipi_dsi_generic_write_seq(dsi, 0x51, 0x05, 0x42);
-
-	ret = mipi_dsi_dcs_exit_sleep_mode(dsi);
-	if (ret < 0) {
-		dev_err(dev, "Failed to exit sleep mode: %d\n", ret);
-		return ret;
-	}
-	msleep(100);
-
-	ret = mipi_dsi_dcs_set_display_on(dsi);
-	if (ret < 0) {
-		dev_err(dev, "Failed to set display on: %d\n", ret);
-		return ret;
-	}
-
-	return 0;
 }
 
 static int rm692e5_disable(struct drm_panel *panel)
 {
 	struct rm692e5_panel *ctx = to_rm692e5_panel(panel);
 	struct mipi_dsi_device *dsi = ctx->dsi;
-	struct device *dev = &dsi->dev;
-	int ret;
+	struct mipi_dsi_multi_context dsi_ctx = { .dsi = dsi };
 
 	dsi->mode_flags &= ~MIPI_DSI_MODE_LPM;
 
-	mipi_dsi_generic_write_seq(dsi, 0xfe, 0x00);
+	mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0xfe, 0x00);
 
-	ret = mipi_dsi_dcs_set_display_off(dsi);
-	if (ret < 0) {
-		dev_err(dev, "Failed to set display off: %d\n", ret);
-		return ret;
-	}
+	mipi_dsi_dcs_set_display_off_multi(&dsi_ctx);
 
-	ret = mipi_dsi_dcs_enter_sleep_mode(dsi);
-	if (ret < 0) {
-		dev_err(dev, "Failed to enter sleep mode: %d\n", ret);
-		return ret;
-	}
-	msleep(100);
+	mipi_dsi_dcs_enter_sleep_mode_multi(&dsi_ctx);
 
-	return 0;
+	mipi_dsi_msleep(&dsi_ctx, 100);
+
+	return dsi_ctx.accum_err;
 }
 
 static int rm692e5_prepare(struct drm_panel *panel)
 {
 	struct rm692e5_panel *ctx = to_rm692e5_panel(panel);
 	struct drm_dsc_picture_parameter_set pps;
-	struct device *dev = &ctx->dsi->dev;
-	int ret;
+	struct mipi_dsi_multi_context dsi_ctx = { .dsi = ctx->dsi };
 
-	ret = regulator_bulk_enable(ARRAY_SIZE(ctx->supplies), ctx->supplies);
-	if (ret < 0) {
-		dev_err(dev, "Failed to enable regulators: %d\n", ret);
-		return ret;
-	}
+	dsi_ctx.accum_err = regulator_bulk_enable(ARRAY_SIZE(ctx->supplies), ctx->supplies);
+	if (dsi_ctx.accum_err)
+		return dsi_ctx.accum_err;
 
 	rm692e5_reset(ctx);
 
-	ret = rm692e5_on(ctx);
-	if (ret < 0) {
-		dev_err(dev, "Failed to initialize panel: %d\n", ret);
-		gpiod_set_value_cansleep(ctx->reset_gpio, 1);
-		regulator_bulk_disable(ARRAY_SIZE(ctx->supplies), ctx->supplies);
-		return ret;
-	}
+	rm692e5_on(&dsi_ctx);
 
 	drm_dsc_pps_payload_pack(&pps, &ctx->dsc);
 
-	ret = mipi_dsi_picture_parameter_set(ctx->dsi, &pps);
-	if (ret < 0) {
-		dev_err(panel->dev, "failed to transmit PPS: %d\n", ret);
-		return ret;
-	}
-
-	ret = mipi_dsi_compression_mode(ctx->dsi, true);
-	if (ret < 0) {
-		dev_err(dev, "failed to enable compression mode: %d\n", ret);
-		return ret;
-	}
-
-	msleep(28);
+	mipi_dsi_picture_parameter_set_multi(&dsi_ctx, &pps);
+	mipi_dsi_compression_mode_ext_multi(&dsi_ctx, true, MIPI_DSI_COMPRESSION_DSC, 0);
+	mipi_dsi_msleep(&dsi_ctx, 28);
 
-	mipi_dsi_generic_write_seq(ctx->dsi, 0xfe, 0x40);
+	mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0xfe, 0x40);
 
 	/* 0x05 -> 90Hz, 0x00 -> 60Hz */
-	mipi_dsi_generic_write_seq(ctx->dsi, 0xbd, 0x05);
+	mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0xbd, 0x05);
 
-	mipi_dsi_generic_write_seq(ctx->dsi, 0xfe, 0x00);
+	mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0xfe, 0x00);
 
-	return 0;
+	if (dsi_ctx.accum_err) {
+		gpiod_set_value_cansleep(ctx->reset_gpio, 1);
+		regulator_bulk_disable(ARRAY_SIZE(ctx->supplies), ctx->supplies);
+	}
+
+	return dsi_ctx.accum_err;
 }
 
 static int rm692e5_unprepare(struct drm_panel *panel)
-- 
2.45.2
Re: [PATCH v2] drm/panel: raydium-rm692e5: transition to mipi_dsi wrapped functions
Posted by Doug Anderson 1 year, 6 months ago 
Hi,

On Tue, Jun 18, 2024 at 8:34 PM Tejas Vipin <tejasvipin76@gmail.com> wrote:
>
> Use functions introduced in commit 966e397e4f60 ("drm/mipi-dsi: Introduce
> mipi_dsi_*_write_seq_multi()") and commit f79d6d28d8fe
> ("drm/mipi-dsi: wrap more functions for streamline handling") for the
> raydium rm692e5 panel.

It should be noted in the patch notes that as part of this patch there
is a small functionality change which is arguably a bugfix.
Specifically if some of the initialization commands in
rm692e5_prepare() fail we'll now properly power the panel back off.

IMO it's not a big enough deal to add a "Fixes" line since it's
unlikely anyone is actually hitting this. If you want to add a Fixes
tag (or someone else feels strongly that there should be one), the
right way would probably to make this a 2-patch series and have _just_
the bugfix first and then have the conversion be a no-op.


> Signed-off-by: Tejas Vipin <tejasvipin76@gmail.com>
> ---
> Changes in v2:
>     - Change rm692e5_on to return void and take mipi_dsi_multi_context
>       as an argument.
>     - Remove unnecessary warnings.
>     - More efficient error handling in rm692e5_prepare
>
> v1: https://lore.kernel.org/all/20240615093758.65431-1-tejasvipin76@gmail.com/
> ---
>  drivers/gpu/drm/panel/panel-raydium-rm692e5.c | 237 ++++++++----------
>  1 file changed, 99 insertions(+), 138 deletions(-)
>
> diff --git a/drivers/gpu/drm/panel/panel-raydium-rm692e5.c b/drivers/gpu/drm/panel/panel-raydium-rm692e5.c
> index 21d97f6b8a2f..9936bda61af2 100644
> --- a/drivers/gpu/drm/panel/panel-raydium-rm692e5.c
> +++ b/drivers/gpu/drm/panel/panel-raydium-rm692e5.c
> @@ -40,176 +40,137 @@ static void rm692e5_reset(struct rm692e5_panel *ctx)
>         usleep_range(10000, 11000);
>  }
>
> -static int rm692e5_on(struct rm692e5_panel *ctx)
> +static void rm692e5_on(struct mipi_dsi_multi_context *dsi_ctx)
>  {
> -       struct mipi_dsi_device *dsi = ctx->dsi;
> -       struct device *dev = &dsi->dev;
> -       int ret;
> +       dsi_ctx->dsi->mode_flags |= MIPI_DSI_MODE_LPM;
> +
> +       mipi_dsi_generic_write_seq_multi(dsi_ctx, 0xfe, 0x41);
> +       mipi_dsi_generic_write_seq_multi(dsi_ctx, 0xd6, 0x00);
> +       mipi_dsi_generic_write_seq_multi(dsi_ctx, 0xfe, 0x16);
> +       mipi_dsi_generic_write_seq_multi(dsi_ctx, 0x8a, 0x87);
> +       mipi_dsi_generic_write_seq_multi(dsi_ctx, 0xfe, 0x71);
> +       mipi_dsi_generic_write_seq_multi(dsi_ctx, 0x82, 0x01);
> +       mipi_dsi_generic_write_seq_multi(dsi_ctx, 0xc6, 0x00);
> +       mipi_dsi_generic_write_seq_multi(dsi_ctx, 0xc7, 0x2c);
> +       mipi_dsi_generic_write_seq_multi(dsi_ctx, 0xc8, 0x64);
> +       mipi_dsi_generic_write_seq_multi(dsi_ctx, 0xc9, 0x3c);
> +       mipi_dsi_generic_write_seq_multi(dsi_ctx, 0xca, 0x80);
> +       mipi_dsi_generic_write_seq_multi(dsi_ctx, 0xcb, 0x02);
> +       mipi_dsi_generic_write_seq_multi(dsi_ctx, 0xcc, 0x02);
> +       mipi_dsi_generic_write_seq_multi(dsi_ctx, 0xfe, 0x38);
> +       mipi_dsi_generic_write_seq_multi(dsi_ctx, 0x18, 0x13);
> +       mipi_dsi_generic_write_seq_multi(dsi_ctx, 0xfe, 0xf4);
> +       mipi_dsi_generic_write_seq_multi(dsi_ctx, 0x00, 0xff);
> +       mipi_dsi_generic_write_seq_multi(dsi_ctx, 0x01, 0xff);
> +       mipi_dsi_generic_write_seq_multi(dsi_ctx, 0x02, 0xcf);
> +       mipi_dsi_generic_write_seq_multi(dsi_ctx, 0x03, 0xbc);
> +       mipi_dsi_generic_write_seq_multi(dsi_ctx, 0x04, 0xb9);
> +       mipi_dsi_generic_write_seq_multi(dsi_ctx, 0x05, 0x99);
> +       mipi_dsi_generic_write_seq_multi(dsi_ctx, 0x06, 0x02);
> +       mipi_dsi_generic_write_seq_multi(dsi_ctx, 0x07, 0x0a);
> +       mipi_dsi_generic_write_seq_multi(dsi_ctx, 0x08, 0xe0);
> +       mipi_dsi_generic_write_seq_multi(dsi_ctx, 0x09, 0x4c);
> +       mipi_dsi_generic_write_seq_multi(dsi_ctx, 0x0a, 0xeb);
> +       mipi_dsi_generic_write_seq_multi(dsi_ctx, 0x0b, 0xe8);
> +       mipi_dsi_generic_write_seq_multi(dsi_ctx, 0x0c, 0x32);
> +       mipi_dsi_generic_write_seq_multi(dsi_ctx, 0x0d, 0x07);
> +       mipi_dsi_generic_write_seq_multi(dsi_ctx, 0xfe, 0xf4);
> +       mipi_dsi_generic_write_seq_multi(dsi_ctx, 0x0d, 0xc0);
> +       mipi_dsi_generic_write_seq_multi(dsi_ctx, 0x0e, 0xff);
> +       mipi_dsi_generic_write_seq_multi(dsi_ctx, 0x0f, 0xff);
> +       mipi_dsi_generic_write_seq_multi(dsi_ctx, 0x10, 0x33);
> +       mipi_dsi_generic_write_seq_multi(dsi_ctx, 0x11, 0x6f);
> +       mipi_dsi_generic_write_seq_multi(dsi_ctx, 0x12, 0x6e);
> +       mipi_dsi_generic_write_seq_multi(dsi_ctx, 0x13, 0xa6);
> +       mipi_dsi_generic_write_seq_multi(dsi_ctx, 0x14, 0x80);
> +       mipi_dsi_generic_write_seq_multi(dsi_ctx, 0x15, 0x02);
> +       mipi_dsi_generic_write_seq_multi(dsi_ctx, 0x16, 0x38);
> +       mipi_dsi_generic_write_seq_multi(dsi_ctx, 0x17, 0xd3);
> +       mipi_dsi_generic_write_seq_multi(dsi_ctx, 0x18, 0x3a);
> +       mipi_dsi_generic_write_seq_multi(dsi_ctx, 0x19, 0xba);
> +       mipi_dsi_generic_write_seq_multi(dsi_ctx, 0x1a, 0xcc);
> +       mipi_dsi_generic_write_seq_multi(dsi_ctx, 0x1b, 0x01);
> +
> +       mipi_dsi_dcs_nop_multi(dsi_ctx);
> +
> +       mipi_dsi_msleep(dsi_ctx, 32);
> +
> +       mipi_dsi_generic_write_seq_multi(dsi_ctx, 0xfe, 0x38);
> +       mipi_dsi_generic_write_seq_multi(dsi_ctx, 0x18, 0x13);
> +       mipi_dsi_generic_write_seq_multi(dsi_ctx, 0xfe, 0xd1);
> +       mipi_dsi_generic_write_seq_multi(dsi_ctx, 0xd3, 0x00);
> +       mipi_dsi_generic_write_seq_multi(dsi_ctx, 0xd0, 0x00);
> +       mipi_dsi_generic_write_seq_multi(dsi_ctx, 0xd2, 0x00);
> +       mipi_dsi_generic_write_seq_multi(dsi_ctx, 0xd4, 0x00);
> +       mipi_dsi_generic_write_seq_multi(dsi_ctx, 0xb4, 0x01);
> +       mipi_dsi_generic_write_seq_multi(dsi_ctx, 0xfe, 0xf9);
> +       mipi_dsi_generic_write_seq_multi(dsi_ctx, 0x00, 0xaf);
> +       mipi_dsi_generic_write_seq_multi(dsi_ctx, 0x1d, 0x37);
> +       mipi_dsi_generic_write_seq_multi(dsi_ctx, 0x44, 0x0a, 0x7b);
> +       mipi_dsi_generic_write_seq_multi(dsi_ctx, 0xfe, 0x00);
> +       mipi_dsi_generic_write_seq_multi(dsi_ctx, 0xfa, 0x01);
> +       mipi_dsi_generic_write_seq_multi(dsi_ctx, 0xc2, 0x08);
> +       mipi_dsi_generic_write_seq_multi(dsi_ctx, 0x35, 0x00);
> +       mipi_dsi_generic_write_seq_multi(dsi_ctx, 0x51, 0x05, 0x42);
> +
> +       mipi_dsi_dcs_exit_sleep_mode_multi(dsi_ctx);
> +       mipi_dsi_msleep(dsi_ctx, 100);
> +       mipi_dsi_dcs_set_display_on_multi(dsi_ctx);
>

nit: Delete the extra blank line above so there's not a blank like
before the closing brace of the function.

Hopefully you can post a v3 with the blank line removed and adjust the
commit message. Then feel free to add:

Reviewed-by: Douglas Anderson <dianders@chromium.org>
Re: [PATCH v2] drm/panel: raydium-rm692e5: transition to mipi_dsi wrapped functions
Posted by Dmitry Baryshkov 1 year, 6 months ago 
On Wed, Jun 19, 2024 at 09:03:49AM GMT, Tejas Vipin wrote:
> Use functions introduced in commit 966e397e4f60 ("drm/mipi-dsi: Introduce
> mipi_dsi_*_write_seq_multi()") and commit f79d6d28d8fe
> ("drm/mipi-dsi: wrap more functions for streamline handling") for the
> raydium rm692e5 panel.
> 
> Signed-off-by: Tejas Vipin <tejasvipin76@gmail.com>
> ---
> Changes in v2:
>     - Change rm692e5_on to return void and take mipi_dsi_multi_context
>       as an argument.
>     - Remove unnecessary warnings.
>     - More efficient error handling in rm692e5_prepare
> 
> v1: https://lore.kernel.org/all/20240615093758.65431-1-tejasvipin76@gmail.com/
> ---
>  drivers/gpu/drm/panel/panel-raydium-rm692e5.c | 237 ++++++++----------
>  1 file changed, 99 insertions(+), 138 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panel/panel-raydium-rm692e5.c b/drivers/gpu/drm/panel/panel-raydium-rm692e5.c
> index 21d97f6b8a2f..9936bda61af2 100644
> --- a/drivers/gpu/drm/panel/panel-raydium-rm692e5.c
> +++ b/drivers/gpu/drm/panel/panel-raydium-rm692e5.c

>  static int rm692e5_prepare(struct drm_panel *panel)
>  {
>  	struct rm692e5_panel *ctx = to_rm692e5_panel(panel);
>  	struct drm_dsc_picture_parameter_set pps;
> -	struct device *dev = &ctx->dsi->dev;
> -	int ret;
> +	struct mipi_dsi_multi_context dsi_ctx = { .dsi = ctx->dsi };
>  
> -	ret = regulator_bulk_enable(ARRAY_SIZE(ctx->supplies), ctx->supplies);
> -	if (ret < 0) {
> -		dev_err(dev, "Failed to enable regulators: %d\n", ret);
> -		return ret;
> -	}
> +	dsi_ctx.accum_err = regulator_bulk_enable(ARRAY_SIZE(ctx->supplies), ctx->supplies);
> +	if (dsi_ctx.accum_err)
> +		return dsi_ctx.accum_err;

int ret, please. Let's leave dsi_ctx.accum_err for DSI errors only.
LGTM otherwise.

>  
>  	rm692e5_reset(ctx);
>  
> -	ret = rm692e5_on(ctx);
> -	if (ret < 0) {
> -		dev_err(dev, "Failed to initialize panel: %d\n", ret);
> -		gpiod_set_value_cansleep(ctx->reset_gpio, 1);
> -		regulator_bulk_disable(ARRAY_SIZE(ctx->supplies), ctx->supplies);
> -		return ret;
> -	}
> +	rm692e5_on(&dsi_ctx);
>  


-- 
With best wishes
Dmitry
Re: [PATCH v2] drm/panel: raydium-rm692e5: transition to mipi_dsi wrapped functions
Posted by Tejas Vipin 1 year, 6 months ago 

On 6/19/24 12:06 PM, Dmitry Baryshkov wrote:
> On Wed, Jun 19, 2024 at 09:03:49AM GMT, Tejas Vipin wrote:
>> Use functions introduced in commit 966e397e4f60 ("drm/mipi-dsi: Introduce
>> mipi_dsi_*_write_seq_multi()") and commit f79d6d28d8fe
>> ("drm/mipi-dsi: wrap more functions for streamline handling") for the
>> raydium rm692e5 panel.
>>
>> Signed-off-by: Tejas Vipin <tejasvipin76@gmail.com>
>> ---
>> Changes in v2:
>>     - Change rm692e5_on to return void and take mipi_dsi_multi_context
>>       as an argument.
>>     - Remove unnecessary warnings.
>>     - More efficient error handling in rm692e5_prepare
>>
>> v1: https://lore.kernel.org/all/20240615093758.65431-1-tejasvipin76@gmail.com/
>> ---
>>  drivers/gpu/drm/panel/panel-raydium-rm692e5.c | 237 ++++++++----------
>>  1 file changed, 99 insertions(+), 138 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/panel/panel-raydium-rm692e5.c b/drivers/gpu/drm/panel/panel-raydium-rm692e5.c
>> index 21d97f6b8a2f..9936bda61af2 100644
>> --- a/drivers/gpu/drm/panel/panel-raydium-rm692e5.c
>> +++ b/drivers/gpu/drm/panel/panel-raydium-rm692e5.c
> 
>>  static int rm692e5_prepare(struct drm_panel *panel)
>>  {
>>  	struct rm692e5_panel *ctx = to_rm692e5_panel(panel);
>>  	struct drm_dsc_picture_parameter_set pps;
>> -	struct device *dev = &ctx->dsi->dev;
>> -	int ret;
>> +	struct mipi_dsi_multi_context dsi_ctx = { .dsi = ctx->dsi };
>>  
>> -	ret = regulator_bulk_enable(ARRAY_SIZE(ctx->supplies), ctx->supplies);
>> -	if (ret < 0) {
>> -		dev_err(dev, "Failed to enable regulators: %d\n", ret);
>> -		return ret;
>> -	}
>> +	dsi_ctx.accum_err = regulator_bulk_enable(ARRAY_SIZE(ctx->supplies), ctx->supplies);
>> +	if (dsi_ctx.accum_err)
>> +		return dsi_ctx.accum_err;
> 
> int ret, please. Let's leave dsi_ctx.accum_err for DSI errors only.
> LGTM otherwise.

Is this really necessary seeing how regulator_bulk_enable returns
0 on success anyways? It saves creating a new variable for a single
check. In case you do think its necessary, should it be changed in
himax_hx83102 too?

> 
>>  
>>  	rm692e5_reset(ctx);
>>  
>> -	ret = rm692e5_on(ctx);
>> -	if (ret < 0) {
>> -		dev_err(dev, "Failed to initialize panel: %d\n", ret);
>> -		gpiod_set_value_cansleep(ctx->reset_gpio, 1);
>> -		regulator_bulk_disable(ARRAY_SIZE(ctx->supplies), ctx->supplies);
>> -		return ret;
>> -	}
>> +	rm692e5_on(&dsi_ctx);
>>  
> 
> 

-- 
---
Tejas Vipin
Re: [PATCH v2] drm/panel: raydium-rm692e5: transition to mipi_dsi wrapped functions
Posted by Doug Anderson 1 year, 6 months ago 
Hi,

On Wed, Jun 19, 2024 at 12:23 AM Tejas Vipin <tejasvipin76@gmail.com> wrote:
>
>
>
> On 6/19/24 12:06 PM, Dmitry Baryshkov wrote:
> > On Wed, Jun 19, 2024 at 09:03:49AM GMT, Tejas Vipin wrote:
> >> Use functions introduced in commit 966e397e4f60 ("drm/mipi-dsi: Introduce
> >> mipi_dsi_*_write_seq_multi()") and commit f79d6d28d8fe
> >> ("drm/mipi-dsi: wrap more functions for streamline handling") for the
> >> raydium rm692e5 panel.
> >>
> >> Signed-off-by: Tejas Vipin <tejasvipin76@gmail.com>
> >> ---
> >> Changes in v2:
> >>     - Change rm692e5_on to return void and take mipi_dsi_multi_context
> >>       as an argument.
> >>     - Remove unnecessary warnings.
> >>     - More efficient error handling in rm692e5_prepare
> >>
> >> v1: https://lore.kernel.org/all/20240615093758.65431-1-tejasvipin76@gmail.com/
> >> ---
> >>  drivers/gpu/drm/panel/panel-raydium-rm692e5.c | 237 ++++++++----------
> >>  1 file changed, 99 insertions(+), 138 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/panel/panel-raydium-rm692e5.c b/drivers/gpu/drm/panel/panel-raydium-rm692e5.c
> >> index 21d97f6b8a2f..9936bda61af2 100644
> >> --- a/drivers/gpu/drm/panel/panel-raydium-rm692e5.c
> >> +++ b/drivers/gpu/drm/panel/panel-raydium-rm692e5.c
> >
> >>  static int rm692e5_prepare(struct drm_panel *panel)
> >>  {
> >>      struct rm692e5_panel *ctx = to_rm692e5_panel(panel);
> >>      struct drm_dsc_picture_parameter_set pps;
> >> -    struct device *dev = &ctx->dsi->dev;
> >> -    int ret;
> >> +    struct mipi_dsi_multi_context dsi_ctx = { .dsi = ctx->dsi };
> >>
> >> -    ret = regulator_bulk_enable(ARRAY_SIZE(ctx->supplies), ctx->supplies);
> >> -    if (ret < 0) {
> >> -            dev_err(dev, "Failed to enable regulators: %d\n", ret);
> >> -            return ret;
> >> -    }
> >> +    dsi_ctx.accum_err = regulator_bulk_enable(ARRAY_SIZE(ctx->supplies), ctx->supplies);
> >> +    if (dsi_ctx.accum_err)
> >> +            return dsi_ctx.accum_err;
> >
> > int ret, please. Let's leave dsi_ctx.accum_err for DSI errors only.
> > LGTM otherwise.
>
> Is this really necessary seeing how regulator_bulk_enable returns
> 0 on success anyways? It saves creating a new variable for a single
> check. In case you do think its necessary, should it be changed in
> himax_hx83102 too?

Right. I made the same choice as Tejas did when I wrote commit
a2ab7cb169da ("drm/panel: himax-hx83102: use wrapped MIPI DCS
functions"). In that commit message, I wrote:

It can also be noted that hx83102_prepare() has a mix of things that
can take advantage of _multi calls and things that can't. The cleanest
seemed to be to use the multi_ctx still but consistently use the
"accum_err" variable for error returns, though that's definitely a
style decision with pros and cons.

In my mind trying to juggle half the cases having the error in "ret"
and half in the DSI context was a recipe for getting mixed up and
returning the wrong error. On the other hand, it felt awkward using
the "dsi_ctx.accum_err". In the end I felt that the extra awkwardness
was worth it if it meant that I was less likely to "return ret" when
the error code was actually in "dsi_ctx.accum_err"...


-Doug
Re: [PATCH v2] drm/panel: raydium-rm692e5: transition to mipi_dsi wrapped functions
Posted by Dmitry Baryshkov 1 year, 6 months ago 
On Thu, 20 Jun 2024 at 17:42, Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Wed, Jun 19, 2024 at 12:23 AM Tejas Vipin <tejasvipin76@gmail.com> wrote:
> >
> >
> >
> > On 6/19/24 12:06 PM, Dmitry Baryshkov wrote:
> > > On Wed, Jun 19, 2024 at 09:03:49AM GMT, Tejas Vipin wrote:
> > >> Use functions introduced in commit 966e397e4f60 ("drm/mipi-dsi: Introduce
> > >> mipi_dsi_*_write_seq_multi()") and commit f79d6d28d8fe
> > >> ("drm/mipi-dsi: wrap more functions for streamline handling") for the
> > >> raydium rm692e5 panel.
> > >>
> > >> Signed-off-by: Tejas Vipin <tejasvipin76@gmail.com>
> > >> ---
> > >> Changes in v2:
> > >>     - Change rm692e5_on to return void and take mipi_dsi_multi_context
> > >>       as an argument.
> > >>     - Remove unnecessary warnings.
> > >>     - More efficient error handling in rm692e5_prepare
> > >>
> > >> v1: https://lore.kernel.org/all/20240615093758.65431-1-tejasvipin76@gmail.com/
> > >> ---
> > >>  drivers/gpu/drm/panel/panel-raydium-rm692e5.c | 237 ++++++++----------
> > >>  1 file changed, 99 insertions(+), 138 deletions(-)
> > >>
> > >> diff --git a/drivers/gpu/drm/panel/panel-raydium-rm692e5.c b/drivers/gpu/drm/panel/panel-raydium-rm692e5.c
> > >> index 21d97f6b8a2f..9936bda61af2 100644
> > >> --- a/drivers/gpu/drm/panel/panel-raydium-rm692e5.c
> > >> +++ b/drivers/gpu/drm/panel/panel-raydium-rm692e5.c
> > >
> > >>  static int rm692e5_prepare(struct drm_panel *panel)
> > >>  {
> > >>      struct rm692e5_panel *ctx = to_rm692e5_panel(panel);
> > >>      struct drm_dsc_picture_parameter_set pps;
> > >> -    struct device *dev = &ctx->dsi->dev;
> > >> -    int ret;
> > >> +    struct mipi_dsi_multi_context dsi_ctx = { .dsi = ctx->dsi };
> > >>
> > >> -    ret = regulator_bulk_enable(ARRAY_SIZE(ctx->supplies), ctx->supplies);
> > >> -    if (ret < 0) {
> > >> -            dev_err(dev, "Failed to enable regulators: %d\n", ret);
> > >> -            return ret;
> > >> -    }
> > >> +    dsi_ctx.accum_err = regulator_bulk_enable(ARRAY_SIZE(ctx->supplies), ctx->supplies);
> > >> +    if (dsi_ctx.accum_err)
> > >> +            return dsi_ctx.accum_err;
> > >
> > > int ret, please. Let's leave dsi_ctx.accum_err for DSI errors only.
> > > LGTM otherwise.
> >
> > Is this really necessary seeing how regulator_bulk_enable returns
> > 0 on success anyways? It saves creating a new variable for a single
> > check. In case you do think its necessary, should it be changed in
> > himax_hx83102 too?
>
> Right. I made the same choice as Tejas did when I wrote commit
> a2ab7cb169da ("drm/panel: himax-hx83102: use wrapped MIPI DCS
> functions"). In that commit message, I wrote:
>
> It can also be noted that hx83102_prepare() has a mix of things that
> can take advantage of _multi calls and things that can't. The cleanest
> seemed to be to use the multi_ctx still but consistently use the
> "accum_err" variable for error returns, though that's definitely a
> style decision with pros and cons.
>
> In my mind trying to juggle half the cases having the error in "ret"
> and half in the DSI context was a recipe for getting mixed up and
> returning the wrong error. On the other hand, it felt awkward using
> the "dsi_ctx.accum_err". In the end I felt that the extra awkwardness
> was worth it if it meant that I was less likely to "return ret" when
> the error code was actually in "dsi_ctx.accum_err"...

Fair point.

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>


-- 
With best wishes
Dmitry

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.