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>
---
 drivers/gpu/drm/panel/panel-raydium-rm692e5.c | 227 ++++++++----------
 1 file changed, 98 insertions(+), 129 deletions(-)

diff --git a/drivers/gpu/drm/panel/panel-raydium-rm692e5.c b/drivers/gpu/drm/panel/panel-raydium-rm692e5.c
index 21d97f6b8a2f..3ddbedeac077 100644
--- a/drivers/gpu/drm/panel/panel-raydium-rm692e5.c
+++ b/drivers/gpu/drm/panel/panel-raydium-rm692e5.c
@@ -43,124 +43,103 @@ static void rm692e5_reset(struct rm692e5_panel *ctx)
 static int rm692e5_on(struct rm692e5_panel *ctx)
 {
 	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, 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;
+	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);
+
+	return dsi_ctx.accum_err;
 }
 
 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)
@@ -168,48 +147,38 @@ 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) {
+		dev_err(dev, "Failed to enable regulators: %d\n", 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);
+	dsi_ctx.accum_err = rm692e5_on(ctx);
+	if (dsi_ctx.accum_err) {
+		dev_err(dev, "Failed to initialize panel: %d\n", dsi_ctx.accum_err);
 		gpiod_set_value_cansleep(ctx->reset_gpio, 1);
 		regulator_bulk_disable(ARRAY_SIZE(ctx->supplies), ctx->supplies);
-		return ret;
+		return dsi_ctx.accum_err;
 	}
 
 	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;
-	}
+	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);
 
-	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_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;
+	return dsi_ctx.accum_err;
 }
 
 static int rm692e5_unprepare(struct drm_panel *panel)
-- 
2.45.2

Hi,

On Sat, Jun 15, 2024 at 2:40 AM Tejas Vipin <tejasvipin76@gmail.com> wrote:
>
> @@ -168,48 +147,38 @@ 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) {
> +               dev_err(dev, "Failed to enable regulators: %d\n", dsi_ctx.accum_err);
> +               return dsi_ctx.accum_err;
>         }

It would be my preference to get rid of the error print here since
regulator_bulk_enable() already prints an error message.


>         rm692e5_reset(ctx);
>
> -       ret = rm692e5_on(ctx);
> -       if (ret < 0) {
> -               dev_err(dev, "Failed to initialize panel: %d\n", ret);
> +       dsi_ctx.accum_err = rm692e5_on(ctx);
> +       if (dsi_ctx.accum_err) {
> +               dev_err(dev, "Failed to initialize panel: %d\n", dsi_ctx.accum_err);

I'd probably change rm692e5_on() to take the "dsi_ctx" as a parameter
and then you don't need to declare a new one there.

...also, you don't need to add an error message since rm692e5_on()
will have already printed one (since the "multi" style functions
always print error messages for you).



>                 gpiod_set_value_cansleep(ctx->reset_gpio, 1);
>                 regulator_bulk_disable(ARRAY_SIZE(ctx->supplies), ctx->supplies);
> -               return ret;
> +               return dsi_ctx.accum_err;

Not new for your patch, but it seems odd that we don't do this error
handling (re-assert reset and disable the regulator) for errors later
in the function. Shouldn't it do that? It feels like the error
handling should be in an "err" label and we should end up doing that
any time we return an error code... What do you think?


-Doug

On 6/18/24 1:36 AM, Doug Anderson wrote:
> Hi,
> 
> On Sat, Jun 15, 2024 at 2:40 AM Tejas Vipin <tejasvipin76@gmail.com> wrote:
>>
>> @@ -168,48 +147,38 @@ 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) {
>> +               dev_err(dev, "Failed to enable regulators: %d\n", dsi_ctx.accum_err);
>> +               return dsi_ctx.accum_err;
>>         }
> 
> It would be my preference to get rid of the error print here since
> regulator_bulk_enable() already prints an error message.
> 
> 
>>         rm692e5_reset(ctx);
>>
>> -       ret = rm692e5_on(ctx);
>> -       if (ret < 0) {
>> -               dev_err(dev, "Failed to initialize panel: %d\n", ret);
>> +       dsi_ctx.accum_err = rm692e5_on(ctx);
>> +       if (dsi_ctx.accum_err) {
>> +               dev_err(dev, "Failed to initialize panel: %d\n", dsi_ctx.accum_err);
> 
> I'd probably change rm692e5_on() to take the "dsi_ctx" as a parameter
> and then you don't need to declare a new one there.
> 
> ...also, you don't need to add an error message since rm692e5_on()
> will have already printed one (since the "multi" style functions
> always print error messages for you).

I'm guessing that the change about regulator_bulk_enable and 
rm692e5 should also be applied to all the other panels where
similar behavior occurs?

> 
> 
> 
>>                 gpiod_set_value_cansleep(ctx->reset_gpio, 1);
>>                 regulator_bulk_disable(ARRAY_SIZE(ctx->supplies), ctx->supplies);
>> -               return ret;
>> +               return dsi_ctx.accum_err;
> 
> Not new for your patch, but it seems odd that we don't do this error
> handling (re-assert reset and disable the regulator) for errors later
> in the function. Shouldn't it do that? It feels like the error
> handling should be in an "err" label and we should end up doing that
> any time we return an error code... What do you think?

Personally I don't think this is necessary because imo labels
only get useful when there's a couple of them and/or they're 
jumped to multiple times. I don't think either would happen in
this particular function. But I guess if you have some convention
in mind, then it could be done?

> 
> 
> -Doug

Hi,

On Tue, Jun 18, 2024 at 5:25 AM Tejas Vipin <tejasvipin76@gmail.com> wrote:
>
> >>         rm692e5_reset(ctx);
> >>
> >> -       ret = rm692e5_on(ctx);
> >> -       if (ret < 0) {
> >> -               dev_err(dev, "Failed to initialize panel: %d\n", ret);
> >> +       dsi_ctx.accum_err = rm692e5_on(ctx);
> >> +       if (dsi_ctx.accum_err) {
> >> +               dev_err(dev, "Failed to initialize panel: %d\n", dsi_ctx.accum_err);
> >
> > I'd probably change rm692e5_on() to take the "dsi_ctx" as a parameter
> > and then you don't need to declare a new one there.
> >
> > ...also, you don't need to add an error message since rm692e5_on()
> > will have already printed one (since the "multi" style functions
> > always print error messages for you).
>
> I'm guessing that the change about regulator_bulk_enable and
> rm692e5 should also be applied to all the other panels where
> similar behavior occurs?

Yeah, I'd say so.


> >>                 gpiod_set_value_cansleep(ctx->reset_gpio, 1);
> >>                 regulator_bulk_disable(ARRAY_SIZE(ctx->supplies), ctx->supplies);
> >> -               return ret;
> >> +               return dsi_ctx.accum_err;
> >
> > Not new for your patch, but it seems odd that we don't do this error
> > handling (re-assert reset and disable the regulator) for errors later
> > in the function. Shouldn't it do that? It feels like the error
> > handling should be in an "err" label and we should end up doing that
> > any time we return an error code... What do you think?
>
> Personally I don't think this is necessary because imo labels
> only get useful when there's a couple of them and/or they're
> jumped to multiple times. I don't think either would happen in
> this particular function. But I guess if you have some convention
> in mind, then it could be done?

I think mostly my suggestion was just that we should also do the
gpiod_set_value_cansleep() and regulator_bulk_disable() at the end of
rm692e5_prepare() if `dsi_ctx.accum_err` is non-zero. Then you've got
two places doing the same thing: here and at the end of the function.

...oh, but everything below here is already a no-op if the error is
set. ...so I guess looking at it closer, my suggestion wouldn't be a
"goto" but would instead be to just move the gpio/regulator call to
the end. What do you think?

-Doug

On 6/18/24 7:06 PM, Doug Anderson wrote:
> Hi,
> 
> On Tue, Jun 18, 2024 at 5:25 AM Tejas Vipin <tejasvipin76@gmail.com> wrote:
>>
>>>>         rm692e5_reset(ctx);
>>>>
>>>> -       ret = rm692e5_on(ctx);
>>>> -       if (ret < 0) {
>>>> -               dev_err(dev, "Failed to initialize panel: %d\n", ret);
>>>> +       dsi_ctx.accum_err = rm692e5_on(ctx);
>>>> +       if (dsi_ctx.accum_err) {
>>>> +               dev_err(dev, "Failed to initialize panel: %d\n", dsi_ctx.accum_err);
>>>
>>> I'd probably change rm692e5_on() to take the "dsi_ctx" as a parameter
>>> and then you don't need to declare a new one there.
>>>
>>> ...also, you don't need to add an error message since rm692e5_on()
>>> will have already printed one (since the "multi" style functions
>>> always print error messages for you).
>>
>> I'm guessing that the change about regulator_bulk_enable and
>> rm692e5 should also be applied to all the other panels where
>> similar behavior occurs?
> 
> Yeah, I'd say so.
> 
> 
>>>>                 gpiod_set_value_cansleep(ctx->reset_gpio, 1);
>>>>                 regulator_bulk_disable(ARRAY_SIZE(ctx->supplies), ctx->supplies);
>>>> -               return ret;
>>>> +               return dsi_ctx.accum_err;
>>>
>>> Not new for your patch, but it seems odd that we don't do this error
>>> handling (re-assert reset and disable the regulator) for errors later
>>> in the function. Shouldn't it do that? It feels like the error
>>> handling should be in an "err" label and we should end up doing that
>>> any time we return an error code... What do you think?
>>
>> Personally I don't think this is necessary because imo labels
>> only get useful when there's a couple of them and/or they're
>> jumped to multiple times. I don't think either would happen in
>> this particular function. But I guess if you have some convention
>> in mind, then it could be done?
> 
> I think mostly my suggestion was just that we should also do the
> gpiod_set_value_cansleep() and regulator_bulk_disable() at the end of
> rm692e5_prepare() if `dsi_ctx.accum_err` is non-zero. Then you've got
> two places doing the same thing: here and at the end of the function.
> 
> ...oh, but everything below here is already a no-op if the error is
> set. ...so I guess looking at it closer, my suggestion wouldn't be a
> "goto" but would instead be to just move the gpio/regulator call to
> the end. What do you think?

Yeah, sounds good. I'll be doing that.

> 
> -Doug