[PATCH 0/2] add more multi functions to streamline error handling

[PATCH 0/2] add more multi functions to streamline error handling

[Tejas Vipin]

This series adds more multi style functions and uses them in the 
startek-kd070fhfid015 panel effectively.

Tejas Vipin (2):
  drm/mipi-dsi: add more multi functions for better error handling
  drm/panel: startek-kd070fhfid015: transition to mipi_dsi wrapped
    functions

 drivers/gpu/drm/drm_mipi_dsi.c                | 164 ++++++++++++++++++
 .../drm/panel/panel-startek-kd070fhfid015.c   | 107 ++++--------
 include/drm/drm_mipi_dsi.h                    |  10 ++
 3 files changed, 209 insertions(+), 72 deletions(-)

-- 
2.45.2

[PATCH 1/2] drm/mipi-dsi: add more multi functions for better error handling

[Tejas Vipin]

Add more functions that can benefit from being multi style, which
reduces code size in panels where they appear.

Signed-off-by: Tejas Vipin <tejasvipin76@gmail.com>
---
 drivers/gpu/drm/drm_mipi_dsi.c | 164 +++++++++++++++++++++++++++++++++
 include/drm/drm_mipi_dsi.h     |  10 ++
 2 files changed, 174 insertions(+)

diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c
index a471c46f5ca6..3f7fe734b684 100644
--- a/drivers/gpu/drm/drm_mipi_dsi.c
+++ b/drivers/gpu/drm/drm_mipi_dsi.c
@@ -1639,6 +1639,170 @@ void mipi_dsi_dcs_set_tear_on_multi(struct mipi_dsi_multi_context *ctx,
 }
 EXPORT_SYMBOL(mipi_dsi_dcs_set_tear_on_multi);
 
+/**
+ * mipi_dsi_turn_on_peripheral_multi() - sends a Turn On Peripheral command
+ * @ctx: Context for multiple DSI transactions
+ *
+ * Like mipi_dsi_turn_on_peripheral() but deals with errors in a way that
+ * makes it convenient to make several calls in a row.
+ */
+void mipi_dsi_turn_on_peripheral_multi(struct mipi_dsi_multi_context *ctx)
+{
+	struct mipi_dsi_device *dsi = ctx->dsi;
+	struct device *dev = &dsi->dev;
+	ssize_t ret;
+
+	if (ctx->accum_err)
+		return;
+
+	ret = mipi_dsi_turn_on_peripheral(dsi);
+	if (ret < 0) {
+		ctx->accum_err = ret;
+		dev_err(dev, "Failed to turn on peripheral: %d\n",
+			ctx->accum_err);
+	}
+}
+EXPORT_SYMBOL(mipi_dsi_turn_on_peripheral_multi);
+
+/**
+ * mipi_dsi_dcs_soft_reset_multi() - perform a software reset of the display module
+ * @ctx: Context for multiple DSI transactions
+ *
+ * Like mipi_dsi_dcs_soft_reset() but deals with errors in a way that
+ * makes it convenient to make several calls in a row.
+ */
+void mipi_dsi_dcs_soft_reset_multi(struct mipi_dsi_multi_context *ctx)
+{
+	struct mipi_dsi_device *dsi = ctx->dsi;
+	struct device *dev = &dsi->dev;
+	ssize_t ret;
+
+	if (ctx->accum_err)
+		return;
+
+	ret = mipi_dsi_dcs_soft_reset(dsi);
+	if (ret < 0) {
+		ctx->accum_err = ret;
+		dev_err(dev, "Failed to mipi_dsi_dcs_soft_reset: %d\n",
+			ctx->accum_err);
+	}
+}
+EXPORT_SYMBOL(mipi_dsi_dcs_soft_reset_multi);
+
+/**
+ * mipi_dsi_dcs_set_display_brightness_multi() - sets the brightness value of
+ *	the display
+ * @ctx: Context for multiple DSI transactions
+ * @brightness: brightness value
+ *
+ * Like mipi_dsi_dcs_set_display_brightness() but deals with errors in a way that
+ * makes it convenient to make several calls in a row.
+ */
+void mipi_dsi_dcs_set_display_brightness_multi(struct mipi_dsi_multi_context *ctx,
+					       u16 brightness)
+{
+	struct mipi_dsi_device *dsi = ctx->dsi;
+	struct device *dev = &dsi->dev;
+	ssize_t ret;
+
+	if (ctx->accum_err)
+		return;
+
+	ret = mipi_dsi_dcs_set_display_brightness(dsi, brightness);
+	if (ret < 0) {
+		ctx->accum_err = ret;
+		dev_err(dev, "Failed to write display brightness: %d\n",
+			ctx->accum_err);
+	}
+}
+EXPORT_SYMBOL(mipi_dsi_dcs_set_display_brightness_multi);
+
+/**
+ * mipi_dsi_dcs_set_pixel_format_multi() - sets the pixel format for the RGB image
+ *	data used by the interface
+ * @ctx: Context for multiple DSI transactions
+ * @format: pixel format
+ *
+ * Like mipi_dsi_dcs_set_pixel_format() but deals with errors in a way that
+ * makes it convenient to make several calls in a row.
+ */
+void mipi_dsi_dcs_set_pixel_format_multi(struct mipi_dsi_multi_context *ctx,
+					 u8 format)
+{
+	struct mipi_dsi_device *dsi = ctx->dsi;
+	struct device *dev = &dsi->dev;
+	ssize_t ret;
+
+	if (ctx->accum_err)
+		return;
+
+	ret = mipi_dsi_dcs_set_pixel_format(dsi, format);
+	if (ret < 0) {
+		ctx->accum_err = ret;
+		dev_err(dev, "Failed to set pixel format: %d\n",
+			ctx->accum_err);
+	}
+}
+EXPORT_SYMBOL(mipi_dsi_dcs_set_pixel_format_multi);
+
+/**
+ * mipi_dsi_dcs_set_column_address_multi() - define the column extent of the
+ *	frame memory accessed by the host processor
+ * @ctx: Context for multiple DSI transactions
+ * @start: first column of frame memory
+ * @end: last column of frame memory
+ *
+ * Like mipi_dsi_dcs_set_column_address() but deals with errors in a way that
+ * makes it convenient to make several calls in a row.
+ */
+void mipi_dsi_dcs_set_column_address_multi(struct mipi_dsi_multi_context *ctx,
+					   u16 start, u16 end)
+{
+	struct mipi_dsi_device *dsi = ctx->dsi;
+	struct device *dev = &dsi->dev;
+	ssize_t ret;
+
+	if (ctx->accum_err)
+		return;
+
+	ret = mipi_dsi_dcs_set_column_address(dsi, start, end);
+	if (ret < 0) {
+		ctx->accum_err = ret;
+		dev_err(dev, "Failed to set column address: %d\n",
+			ctx->accum_err);
+	}
+}
+EXPORT_SYMBOL(mipi_dsi_dcs_set_column_address_multi);
+
+/**
+ * mipi_dsi_dcs_set_page_address_multi() - define the column extent of the
+ *	frame memory accessed by the host processor
+ * @ctx: Context for multiple DSI transactions
+ * @start: first column of frame memory
+ * @end: last column of frame memory
+ *
+ * Like mipi_dsi_dcs_set_page_address() but deals with errors in a way that
+ * makes it convenient to make several calls in a row.
+ */
+void mipi_dsi_dcs_set_page_address_multi(struct mipi_dsi_multi_context *ctx,
+					   u16 start, u16 end)
+{
+	struct mipi_dsi_device *dsi = ctx->dsi;
+	struct device *dev = &dsi->dev;
+	ssize_t ret;
+
+	if (ctx->accum_err)
+		return;
+
+	ret = mipi_dsi_dcs_set_page_address(dsi, start, end);
+	if (ret < 0) {
+		ctx->accum_err = ret;
+		dev_err(dev, "Failed to set page address: %d\n",
+			ctx->accum_err);
+	}
+}
+EXPORT_SYMBOL(mipi_dsi_dcs_set_page_address_multi);
+
 static int mipi_dsi_drv_probe(struct device *dev)
 {
 	struct mipi_dsi_driver *drv = to_mipi_dsi_driver(dev->driver);
diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h
index 71d121aeef24..203b5d53d58f 100644
--- a/include/drm/drm_mipi_dsi.h
+++ b/include/drm/drm_mipi_dsi.h
@@ -358,6 +358,16 @@ void mipi_dsi_dcs_set_display_off_multi(struct mipi_dsi_multi_context *ctx);
 void mipi_dsi_dcs_set_display_on_multi(struct mipi_dsi_multi_context *ctx);
 void mipi_dsi_dcs_set_tear_on_multi(struct mipi_dsi_multi_context *ctx,
 				    enum mipi_dsi_dcs_tear_mode mode);
+void mipi_dsi_turn_on_peripheral_multi(struct mipi_dsi_multi_context *ctx);
+void mipi_dsi_dcs_soft_reset_multi(struct mipi_dsi_multi_context *ctx);
+void mipi_dsi_dcs_set_display_brightness_multi(struct mipi_dsi_multi_context *ctx,
+					       u16 brightness);
+void mipi_dsi_dcs_set_pixel_format_multi(struct mipi_dsi_multi_context *ctx,
+					 u8 format);
+void mipi_dsi_dcs_set_column_address_multi(struct mipi_dsi_multi_context *ctx,
+					   u16 start, u16 end);
+void mipi_dsi_dcs_set_page_address_multi(struct mipi_dsi_multi_context *ctx,
+					   u16 start, u16 end);
 
 /**
  * mipi_dsi_generic_write_seq - transmit data using a generic write packet
-- 
2.45.2

[PATCH 2/2] drm/panel: startek-kd070fhfid015: transition to mipi_dsi wrapped functions

[Tejas Vipin]

Use multi style wrapped functions for mipi_dsi in the
startek-kd070fhfid015 panel.

Signed-off-by: Tejas Vipin <tejasvipin76@gmail.com>
---
 .../drm/panel/panel-startek-kd070fhfid015.c   | 107 ++++++------------
 1 file changed, 35 insertions(+), 72 deletions(-)

diff --git a/drivers/gpu/drm/panel/panel-startek-kd070fhfid015.c b/drivers/gpu/drm/panel/panel-startek-kd070fhfid015.c
index 0156689f41cd..d58c81c43724 100644
--- a/drivers/gpu/drm/panel/panel-startek-kd070fhfid015.c
+++ b/drivers/gpu/drm/panel/panel-startek-kd070fhfid015.c
@@ -52,92 +52,63 @@ static inline struct stk_panel *to_stk_panel(struct drm_panel *panel)
 static int stk_panel_init(struct stk_panel *stk)
 {
 	struct mipi_dsi_device *dsi = stk->dsi;
-	struct device *dev = &stk->dsi->dev;
-	int ret;
+	struct mipi_dsi_multi_context dsi_ctx = {.dsi = dsi};
 
-	ret = mipi_dsi_dcs_soft_reset(dsi);
-	if (ret < 0) {
-		dev_err(dev, "failed to mipi_dsi_dcs_soft_reset: %d\n", ret);
-		return ret;
-	}
-	mdelay(5);
+	mipi_dsi_dcs_soft_reset_multi(&dsi_ctx);
 
-	ret = mipi_dsi_dcs_exit_sleep_mode(dsi);
-	if (ret < 0) {
-		dev_err(dev, "failed to set exit sleep mode: %d\n", ret);
-		return ret;
-	}
-	msleep(120);
+	if (!dsi_ctx.accum_err)
+		mdelay(5);
+
+	mipi_dsi_dcs_exit_sleep_mode_multi(&dsi_ctx);
 
-	mipi_dsi_generic_write_seq(dsi, DSI_REG_MCAP, 0x04);
+	mipi_dsi_msleep(&dsi_ctx, 120);
+
+	mipi_dsi_generic_write_seq_multi(&dsi_ctx, DSI_REG_MCAP, 0x04);
 
 	/* Interface setting, video mode */
-	mipi_dsi_generic_write_seq(dsi, DSI_REG_IS, 0x14, 0x08, 0x00, 0x22, 0x00);
-	mipi_dsi_generic_write_seq(dsi, DSI_REG_IIS, 0x0C, 0x00);
-	mipi_dsi_generic_write_seq(dsi, DSI_REG_CTRL, 0x3A, 0xD3);
+	mipi_dsi_generic_write_seq_multi(&dsi_ctx, DSI_REG_IS, 0x14, 0x08, 0x00, 0x22, 0x00);
+	mipi_dsi_generic_write_seq_multi(&dsi_ctx, DSI_REG_IIS, 0x0C, 0x00);
+	mipi_dsi_generic_write_seq_multi(&dsi_ctx, DSI_REG_CTRL, 0x3A, 0xD3);
 
-	ret = mipi_dsi_dcs_set_display_brightness(dsi, 0x77);
-	if (ret < 0) {
-		dev_err(dev, "failed to write display brightness: %d\n", ret);
-		return ret;
-	}
+	mipi_dsi_dcs_set_display_brightness_multi(&dsi_ctx, 0x77);
 
-	mipi_dsi_dcs_write_seq(dsi, MIPI_DCS_WRITE_CONTROL_DISPLAY,
-			       MIPI_DCS_WRITE_MEMORY_START);
+	mipi_dsi_dcs_write_seq_multi(&dsi_ctx, MIPI_DCS_WRITE_CONTROL_DISPLAY,
+				     MIPI_DCS_WRITE_MEMORY_START);
 
-	ret = mipi_dsi_dcs_set_pixel_format(dsi, 0x77);
-	if (ret < 0) {
-		dev_err(dev, "failed to set pixel format: %d\n", ret);
-		return ret;
-	}
+	mipi_dsi_dcs_set_pixel_format_multi(&dsi_ctx, 0x77);
 
-	ret = mipi_dsi_dcs_set_column_address(dsi, 0, stk->mode->hdisplay - 1);
-	if (ret < 0) {
-		dev_err(dev, "failed to set column address: %d\n", ret);
-		return ret;
-	}
+	mipi_dsi_dcs_set_column_address_multi(&dsi_ctx, 0, stk->mode->hdisplay - 1);
 
-	ret = mipi_dsi_dcs_set_page_address(dsi, 0, stk->mode->vdisplay - 1);
-	if (ret < 0) {
-		dev_err(dev, "failed to set page address: %d\n", ret);
-		return ret;
-	}
+	mipi_dsi_dcs_set_page_address_multi(&dsi_ctx, 0, stk->mode->vdisplay - 1);
 
-	return 0;
+	return dsi_ctx.accum_err;
 }
 
 static int stk_panel_on(struct stk_panel *stk)
 {
 	struct mipi_dsi_device *dsi = stk->dsi;
-	struct device *dev = &stk->dsi->dev;
-	int ret;
+	struct mipi_dsi_multi_context dsi_ctx = {.dsi = dsi};
 
-	ret = mipi_dsi_dcs_set_display_on(dsi);
-	if (ret < 0)
-		dev_err(dev, "failed to set display on: %d\n", ret);
+	mipi_dsi_dcs_set_display_on_multi(&dsi_ctx);
 
-	mdelay(20);
+	if (!dsi_ctx.accum_err)
+		mdelay(20);
 
-	return ret;
+	return dsi_ctx.accum_err;
 }
 
 static void stk_panel_off(struct stk_panel *stk)
 {
 	struct mipi_dsi_device *dsi = stk->dsi;
-	struct device *dev = &stk->dsi->dev;
-	int ret;
+	struct mipi_dsi_multi_context dsi_ctx = {.dsi = dsi};
 
 	dsi->mode_flags &= ~MIPI_DSI_MODE_LPM;
 
-	ret = mipi_dsi_dcs_set_display_off(dsi);
-	if (ret < 0)
-		dev_err(dev, "failed to set display off: %d\n", 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);
+	mipi_dsi_dcs_enter_sleep_mode_multi(&dsi_ctx);
 
-	msleep(100);
+	mipi_dsi_msleep(&dsi_ctx, 100);
 }
 
 static int stk_panel_unprepare(struct drm_panel *panel)
@@ -155,7 +126,6 @@ static int stk_panel_unprepare(struct drm_panel *panel)
 static int stk_panel_prepare(struct drm_panel *panel)
 {
 	struct stk_panel *stk = to_stk_panel(panel);
-	struct device *dev = &stk->dsi->dev;
 	int ret;
 
 	gpiod_set_value(stk->reset_gpio, 0);
@@ -175,16 +145,12 @@ static int stk_panel_prepare(struct drm_panel *panel)
 	gpiod_set_value(stk->reset_gpio, 1);
 	mdelay(10);
 	ret = stk_panel_init(stk);
-	if (ret < 0) {
-		dev_err(dev, "failed to init panel: %d\n", ret);
+	if (ret < 0)
 		goto poweroff;
-	}
 
 	ret = stk_panel_on(stk);
-	if (ret < 0) {
-		dev_err(dev, "failed to set panel on: %d\n", ret);
+	if (ret < 0)
 		goto poweroff;
-	}
 
 	return 0;
 
@@ -250,18 +216,15 @@ static int dsi_dcs_bl_get_brightness(struct backlight_device *bl)
 static int dsi_dcs_bl_update_status(struct backlight_device *bl)
 {
 	struct mipi_dsi_device *dsi = bl_get_data(bl);
-	struct device *dev = &dsi->dev;
-	int ret;
+	struct mipi_dsi_multi_context dsi_ctx = {.dsi = dsi};
 
 	dsi->mode_flags &= ~MIPI_DSI_MODE_LPM;
-	ret = mipi_dsi_dcs_set_display_brightness(dsi, bl->props.brightness);
-	if (ret < 0) {
-		dev_err(dev, "failed to set DSI control: %d\n", ret);
-		return ret;
-	}
+	mipi_dsi_dcs_set_display_brightness_multi(&dsi_ctx, bl->props.brightness);
+	if (dsi_ctx.accum_err)
+		return dsi_ctx.accum_err;
 
 	dsi->mode_flags |= MIPI_DSI_MODE_LPM;
-	return 0;
+	return dsi_ctx.accum_err;
 }
 
 static const struct backlight_ops dsi_bl_ops = {
-- 
2.45.2

Re: [PATCH 1/2] drm/mipi-dsi: add more multi functions for better error handling

[Doug Anderson]

Hi,

On Fri, Jun 28, 2024 at 11:25 AM Tejas Vipin <tejasvipin76@gmail.com> wrote:
>
> +/**
> + * mipi_dsi_dcs_set_page_address_multi() - define the column extent of the
> + *     frame memory accessed by the host processor
> + * @ctx: Context for multiple DSI transactions
> + * @start: first column of frame memory
> + * @end: last column of frame memory

nit: "start" and "end" comments should say "first page" and "last
page", not "first column" and "last column". The previous function was
the one about columns.

> + *
> + * Like mipi_dsi_dcs_set_page_address() but deals with errors in a way that
> + * makes it convenient to make several calls in a row.
> + */
> +void mipi_dsi_dcs_set_page_address_multi(struct mipi_dsi_multi_context *ctx,
> +                                          u16 start, u16 end)

nit: indentation of the above line isn't _quite_ right.


Other than the two nits, this looks fine to me, but I'd prefer if
someone else provides an "Ack" in addition to me that they're OK
adding these extra "multi" functions. Both Dmitry and Linus W were
involved in the original "multi" functions, so maybe they'd be willing
to offer their opinions?

Re: [PATCH 2/2] drm/panel: startek-kd070fhfid015: transition to mipi_dsi wrapped functions

[Doug Anderson]

Hi,

On Fri, Jun 28, 2024 at 11:25 AM Tejas Vipin <tejasvipin76@gmail.com> wrote:
>
> @@ -52,92 +52,63 @@ static inline struct stk_panel *to_stk_panel(struct drm_panel *panel)
>  static int stk_panel_init(struct stk_panel *stk)
>  {
>         struct mipi_dsi_device *dsi = stk->dsi;
> -       struct device *dev = &stk->dsi->dev;
> -       int ret;
> +       struct mipi_dsi_multi_context dsi_ctx = {.dsi = dsi};
>
> -       ret = mipi_dsi_dcs_soft_reset(dsi);
> -       if (ret < 0) {
> -               dev_err(dev, "failed to mipi_dsi_dcs_soft_reset: %d\n", ret);
> -               return ret;
> -       }
> -       mdelay(5);
> +       mipi_dsi_dcs_soft_reset_multi(&dsi_ctx);
>
> -       ret = mipi_dsi_dcs_exit_sleep_mode(dsi);
> -       if (ret < 0) {
> -               dev_err(dev, "failed to set exit sleep mode: %d\n", ret);
> -               return ret;
> -       }
> -       msleep(120);
> +       if (!dsi_ctx.accum_err)
> +               mdelay(5);

I'm curious: why isn't the above just "mipi_dsi_msleep(5)" and get rid
of the "if" test?


> +       mipi_dsi_dcs_exit_sleep_mode_multi(&dsi_ctx);
>
> -       mipi_dsi_generic_write_seq(dsi, DSI_REG_MCAP, 0x04);
> +       mipi_dsi_msleep(&dsi_ctx, 120);
> +
> +       mipi_dsi_generic_write_seq_multi(&dsi_ctx, DSI_REG_MCAP, 0x04);
>
>         /* Interface setting, video mode */
> -       mipi_dsi_generic_write_seq(dsi, DSI_REG_IS, 0x14, 0x08, 0x00, 0x22, 0x00);
> -       mipi_dsi_generic_write_seq(dsi, DSI_REG_IIS, 0x0C, 0x00);
> -       mipi_dsi_generic_write_seq(dsi, DSI_REG_CTRL, 0x3A, 0xD3);
> +       mipi_dsi_generic_write_seq_multi(&dsi_ctx, DSI_REG_IS, 0x14, 0x08, 0x00, 0x22, 0x00);
> +       mipi_dsi_generic_write_seq_multi(&dsi_ctx, DSI_REG_IIS, 0x0C, 0x00);
> +       mipi_dsi_generic_write_seq_multi(&dsi_ctx, DSI_REG_CTRL, 0x3A, 0xD3);

nit: While touching these lines, it'd be nice to transition them to
lower case hex (3a vs 3A).


> -       ret = mipi_dsi_dcs_set_display_brightness(dsi, 0x77);
> -       if (ret < 0) {
> -               dev_err(dev, "failed to write display brightness: %d\n", ret);
> -               return ret;
> -       }
> +       mipi_dsi_dcs_set_display_brightness_multi(&dsi_ctx, 0x77);
>
> -       mipi_dsi_dcs_write_seq(dsi, MIPI_DCS_WRITE_CONTROL_DISPLAY,
> -                              MIPI_DCS_WRITE_MEMORY_START);
> +       mipi_dsi_dcs_write_seq_multi(&dsi_ctx, MIPI_DCS_WRITE_CONTROL_DISPLAY,
> +                                    MIPI_DCS_WRITE_MEMORY_START);
>
> -       ret = mipi_dsi_dcs_set_pixel_format(dsi, 0x77);
> -       if (ret < 0) {
> -               dev_err(dev, "failed to set pixel format: %d\n", ret);
> -               return ret;
> -       }
> +       mipi_dsi_dcs_set_pixel_format_multi(&dsi_ctx, 0x77);
>
> -       ret = mipi_dsi_dcs_set_column_address(dsi, 0, stk->mode->hdisplay - 1);
> -       if (ret < 0) {
> -               dev_err(dev, "failed to set column address: %d\n", ret);
> -               return ret;
> -       }
> +       mipi_dsi_dcs_set_column_address_multi(&dsi_ctx, 0, stk->mode->hdisplay - 1);
>
> -       ret = mipi_dsi_dcs_set_page_address(dsi, 0, stk->mode->vdisplay - 1);
> -       if (ret < 0) {
> -               dev_err(dev, "failed to set page address: %d\n", ret);
> -               return ret;
> -       }
> +       mipi_dsi_dcs_set_page_address_multi(&dsi_ctx, 0, stk->mode->vdisplay - 1);

Nice to get rid of all of these special case "if" tests and error handling! :-)

optional nit: it feels like there are two many blank lines separating
single line statements. Maybe get rid of a few of the blank lines?

>
> -       return 0;
> +       return dsi_ctx.accum_err;
>  }
>
>  static int stk_panel_on(struct stk_panel *stk)
>  {
>         struct mipi_dsi_device *dsi = stk->dsi;
> -       struct device *dev = &stk->dsi->dev;
> -       int ret;
> +       struct mipi_dsi_multi_context dsi_ctx = {.dsi = dsi};
>
> -       ret = mipi_dsi_dcs_set_display_on(dsi);
> -       if (ret < 0)
> -               dev_err(dev, "failed to set display on: %d\n", ret);
> +       mipi_dsi_dcs_set_display_on_multi(&dsi_ctx);
>
> -       mdelay(20);
> +       if (!dsi_ctx.accum_err)
> +               mdelay(20);

Like above, not sure why this isn't mipi_dsi_msleep(20).


-Doug

Re: [PATCH 1/2] drm/mipi-dsi: add more multi functions for better error handling

[Tejas Vipin]

On 7/9/24 5:50 AM, Doug Anderson wrote:
> Hi,
> 
> On Fri, Jun 28, 2024 at 11:25 AM Tejas Vipin <tejasvipin76@gmail.com> wrote:
>>
>> +/**
>> + * mipi_dsi_dcs_set_page_address_multi() - define the column extent of the
>> + *     frame memory accessed by the host processor
>> + * @ctx: Context for multiple DSI transactions
>> + * @start: first column of frame memory
>> + * @end: last column of frame memory
> 
> nit: "start" and "end" comments should say "first page" and "last
> page", not "first column" and "last column". The previous function was
> the one about columns.
> 
>> + *
>> + * Like mipi_dsi_dcs_set_page_address() but deals with errors in a way that
>> + * makes it convenient to make several calls in a row.
>> + */
>> +void mipi_dsi_dcs_set_page_address_multi(struct mipi_dsi_multi_context *ctx,
>> +                                          u16 start, u16 end)
> 
> nit: indentation of the above line isn't _quite_ right.
> 
> 
> Other than the two nits, this looks fine to me, but I'd prefer if
> someone else provides an "Ack" in addition to me that they're OK
> adding these extra "multi" functions. Both Dmitry and Linus W were
> involved in the original "multi" functions, so maybe they'd be willing
> to offer their opinions?

I think a better way to go forward with multi style functions is to
use macros. All the multi style functions are basically exactly the
same with the only difference being the function called internally
and the dev_err. This can be represented in the form of a macro, and
would save on a ton of otherwise redundant code, while also allowing 
us to "convert" any function to multi style as and when we please.

We would ideally have 2 macros, one for the main functions where we
desire to modify accum_err on an error arising, and another macro that
just checks accum_err to see if the function should be a no op.

If you guys think this is a good idea, I'll work on the macros and
do the multi conversions in this new way.

-- 
Tejas Vipin

Re: [PATCH 1/2] drm/mipi-dsi: add more multi functions for better error handling

[Doug Anderson]

Hi,

On Tue, Jul 9, 2024 at 4:18 AM Tejas Vipin <tejasvipin76@gmail.com> wrote:
>
> On 7/9/24 5:50 AM, Doug Anderson wrote:
> > Hi,
> >
> > On Fri, Jun 28, 2024 at 11:25 AM Tejas Vipin <tejasvipin76@gmail.com> wrote:
> >>
> >> +/**
> >> + * mipi_dsi_dcs_set_page_address_multi() - define the column extent of the
> >> + *     frame memory accessed by the host processor
> >> + * @ctx: Context for multiple DSI transactions
> >> + * @start: first column of frame memory
> >> + * @end: last column of frame memory
> >
> > nit: "start" and "end" comments should say "first page" and "last
> > page", not "first column" and "last column". The previous function was
> > the one about columns.
> >
> >> + *
> >> + * Like mipi_dsi_dcs_set_page_address() but deals with errors in a way that
> >> + * makes it convenient to make several calls in a row.
> >> + */
> >> +void mipi_dsi_dcs_set_page_address_multi(struct mipi_dsi_multi_context *ctx,
> >> +                                          u16 start, u16 end)
> >
> > nit: indentation of the above line isn't _quite_ right.
> >
> >
> > Other than the two nits, this looks fine to me, but I'd prefer if
> > someone else provides an "Ack" in addition to me that they're OK
> > adding these extra "multi" functions. Both Dmitry and Linus W were
> > involved in the original "multi" functions, so maybe they'd be willing
> > to offer their opinions?
>
> I think a better way to go forward with multi style functions is to
> use macros. All the multi style functions are basically exactly the
> same with the only difference being the function called internally
> and the dev_err. This can be represented in the form of a macro, and
> would save on a ton of otherwise redundant code, while also allowing
> us to "convert" any function to multi style as and when we please.
>
> We would ideally have 2 macros, one for the main functions where we
> desire to modify accum_err on an error arising, and another macro that
> just checks accum_err to see if the function should be a no op.
>
> If you guys think this is a good idea, I'll work on the macros and
> do the multi conversions in this new way.

I had a similar thought but I wasn't sure how easy it would be. If you
want to prototype it out and send out patches if they look good then
that'd be nice. We'd want to make sure that we actually generate
functions for the "multi" variants since we don't want all that inline
code on every caller, but generating those functions with a macro does
seem like it would work. I guess you'd also need to include some sort
of string for use in the error messages.

-Doug