Hi,
On Fri, Aug 9, 2024 at 9:55 PM Tejas Vipin <tejasvipin76@xxxxxxxxx> wrote:
>
> @@ -41,79 +41,41 @@ static void jdi_fhd_r63452_reset(struct jdi_fhd_r63452 *ctx)
> static int jdi_fhd_r63452_on(struct jdi_fhd_r63452 *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, 0xb0, 0x00);
> - mipi_dsi_generic_write_seq(dsi, 0xd6, 0x01);
> - mipi_dsi_generic_write_seq(dsi, 0xec,
> - 0x64, 0xdc, 0xec, 0x3b, 0x52, 0x00, 0x0b, 0x0b,
> - 0x13, 0x15, 0x68, 0x0b, 0xb5);
> - mipi_dsi_generic_write_seq(dsi, 0xb0, 0x03);
> + mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0xb0, 0x00);
> + mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0xd6, 0x01);
> + mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0xec,
> + 0x64, 0xdc, 0xec, 0x3b, 0x52, 0x00, 0x0b, 0x0b,
> + 0x13, 0x15, 0x68, 0x0b, 0xb5);
> + mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0xb0, 0x03);
>
> - ret = mipi_dsi_dcs_set_tear_on(dsi, MIPI_DSI_DCS_TEAR_MODE_VBLANK);
> - if (ret < 0) {
> - dev_err(dev, "Failed to set tear on: %d\n", ret);
> - return ret;
> - }
> + mipi_dsi_dcs_set_tear_on_multi(&dsi_ctx, MIPI_DSI_DCS_TEAR_MODE_VBLANK);
>
> - mipi_dsi_dcs_write_seq(dsi, MIPI_DCS_SET_ADDRESS_MODE, 0x00);
> + mipi_dsi_dcs_write_seq_multi(&dsi_ctx, MIPI_DCS_SET_ADDRESS_MODE, 0x00);
>
> - 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);
> + mipi_dsi_dcs_set_column_address_multi(&dsi_ctx, 0x0000, 0x0437);
> + mipi_dsi_dcs_set_page_address_multi(&dsi_ctx, 0x0000, 0x077f);
> + mipi_dsi_dcs_set_tear_scanline_multi(&dsi_ctx, 0x0000);
> + mipi_dsi_dcs_set_display_brightness_multi(&dsi_ctx, 0x00ff);
>
> - ret = mipi_dsi_dcs_set_column_address(dsi, 0x0000, 0x0437);
> - if (ret < 0) {
> - dev_err(dev, "Failed to set column address: %d\n", ret);
> - return ret;
> - }
> + mipi_dsi_dcs_write_seq_multi(&dsi_ctx, MIPI_DCS_WRITE_CONTROL_DISPLAY, 0x24);
> + mipi_dsi_dcs_write_seq_multi(&dsi_ctx, MIPI_DCS_WRITE_POWER_SAVE, 0x00);
> + mipi_dsi_dcs_write_seq_multi(&dsi_ctx, MIPI_DCS_SET_CABC_MIN_BRIGHTNESS, 0x00);
> + mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0x84, 0x00);
>
> - ret = mipi_dsi_dcs_set_page_address(dsi, 0x0000, 0x077f);
> - if (ret < 0) {
> - dev_err(dev, "Failed to set page address: %d\n", ret);
> - return ret;
> - }
> -
> - ret = mipi_dsi_dcs_set_tear_scanline(dsi, 0x0000);
> - if (ret < 0) {
> - dev_err(dev, "Failed to set tear scanline: %d\n", ret);
> - return ret;
> - }
> + mipi_dsi_dcs_set_display_on_multi(&dsi_ctx);
> + mipi_dsi_msleep(&dsi_ctx, 20);
> + mipi_dsi_dcs_exit_sleep_mode_multi(&dsi_ctx);
> + mipi_dsi_msleep(&dsi_ctx, 80);
>
> - ret = mipi_dsi_dcs_set_display_brightness(dsi, 0x00ff);
> - if (ret < 0) {
> - dev_err(dev, "Failed to set display brightness: %d\n", ret);
> - return ret;
> - }
> -
> - mipi_dsi_dcs_write_seq(dsi, MIPI_DCS_WRITE_CONTROL_DISPLAY, 0x24);
> - mipi_dsi_dcs_write_seq(dsi, MIPI_DCS_WRITE_POWER_SAVE, 0x00);
> - mipi_dsi_dcs_write_seq(dsi, MIPI_DCS_SET_CABC_MIN_BRIGHTNESS, 0x00);
> - mipi_dsi_dcs_write_seq(dsi, 0x84, 0x00);
> -
> - ret = mipi_dsi_dcs_set_display_on(dsi);
> - if (ret < 0) {
> - dev_err(dev, "Failed to set display on: %d\n", ret);
> - return ret;
> - }
> - msleep(20);
> -
> - 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(80);
> -
> - mipi_dsi_generic_write_seq(dsi, 0xb0, 0x04);
> - mipi_dsi_dcs_write_seq(dsi, 0x84, 0x00);
> - mipi_dsi_generic_write_seq(dsi, 0xc8, 0x11);
> - mipi_dsi_generic_write_seq(dsi, 0xb0, 0x03);
> + mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0xb0, 0x04);
> + mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0x84, 0x00);
> + mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0xc8, 0x11);
> + mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0xb0, 0x03);
>
> return 0;
Whoops! Not "return 0". "return dsi_ctx.accum_err".
> @@ -121,31 +83,22 @@ static int jdi_fhd_r63452_on(struct jdi_fhd_r63452 *ctx)
> static int jdi_fhd_r63452_off(struct jdi_fhd_r63452 *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, 0xb0, 0x00);
> - mipi_dsi_generic_write_seq(dsi, 0xd6, 0x01);
> - mipi_dsi_generic_write_seq(dsi, 0xec,
> - 0x64, 0xdc, 0xec, 0x3b, 0x52, 0x00, 0x0b, 0x0b,
> - 0x13, 0x15, 0x68, 0x0b, 0x95);
> - mipi_dsi_generic_write_seq(dsi, 0xb0, 0x03);
> -
> - ret = mipi_dsi_dcs_set_display_off(dsi);
> - if (ret < 0) {
> - dev_err(dev, "Failed to set display off: %d\n", ret);
> - return ret;
> - }
> - usleep_range(2000, 3000);
> -
> - 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(120);
> + mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0xb0, 0x00);
> + mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0xd6, 0x01);
> + mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0xec,
> + 0x64, 0xdc, 0xec, 0x3b, 0x52, 0x00, 0x0b, 0x0b,
> + 0x13, 0x15, 0x68, 0x0b, 0x95);
> + mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0xb0, 0x03);
> +
> + mipi_dsi_dcs_set_display_off_multi(&dsi_ctx);
> + if (!dsi_ctx.accum_err)
> + usleep_range(2000, 3000);
> + mipi_dsi_dcs_enter_sleep_mode_multi(&dsi_ctx);
> + mipi_dsi_msleep(&dsi_ctx, 120);
>
> return 0;
Whoops! Not "return 0". "return dsi_ctx.accum_err".
Aside from that, this looks really nice to me. The code is much more
succinct and I bet much smaller.
FWIW, I won't insist, but I wouldn't object to this patch also fixing
the callers of jdi_fhd_r63452_on() and jdi_fhd_r63452_off() so that
they no longer print error messages since the _multi functions are
always chatty and thus they're just extra double-prints. If you do
this, jdi_fhd_r63452_off() could actually be a function that returned
"void". ...then you might want to add a comment saying why
jdi_fhd_r63452_unprepare() doesn't pass on any errors. That would be
something like "NOTE: even if sending one of the poweroff commands
failed, we won't return an error here. While the panel won't have been
cleanly turned off at least we've asserted the reset signal so it
should be safe to power it back on again later". ...or something like
that.
