Messages in this thread | | | Date | Tue, 30 May 2023 01:17:13 +0300 | Subject | Re: [PATCH RFC 03/10] drm/panel: Add LGD panel driver for Sony Xperia XZ3 | From | Dmitry Baryshkov <> |
| |
On 30/05/2023 00:11, Marijn Suijten wrote: > On 2023-05-22 04:16:20, Dmitry Baryshkov wrote: > <snip> >>> + if (ctx->dsi->dsc) { >> >> dsi->dsc is always set, thus this condition can be dropped. > > I want to leave room for possibly running the panel without DSC (at a > lower resolution/refresh rate, or at higher power consumption if there > is enough BW) by not assigning the pointer, if we get access to panel > documentation: probably one of the magic commands sent in this driver > controls it but we don't know which. > >>> + drm_dsc_pps_payload_pack(&pps, ctx->dsi->dsc); >>> + >>> + ret = mipi_dsi_picture_parameter_set(ctx->dsi, &pps); >>> + if (ret < 0) { >>> + dev_err(panel->dev, "failed to transmit PPS: %d\n", ret); >>> + goto fail; >>> + } >>> + ret = mipi_dsi_compression_mode(ctx->dsi, true); >>> + if (ret < 0) { >>> + dev_err(dev, "failed to enable compression mode: %d\n", ret); >>> + goto fail; >>> + } >>> + >>> + msleep(28); >>> + } >>> + >>> + ctx->prepared = true; >>> + return 0; >>> + >>> +fail: >>> + gpiod_set_value_cansleep(ctx->reset_gpio, 0); >>> + regulator_disable(ctx->vddio); >>> + return ret; >>> +} > <snip> >>> + /* This panel only supports DSC; unconditionally enable it */ > > On that note I should perhaps reword this. > >>> + dsi->dsc = dsc = devm_kzalloc(&dsi->dev, sizeof(*dsc), GFP_KERNEL); >> >> I think double assignments are frowned upon. > > Ack. > >> >>> + if (!dsc) >>> + return -ENOMEM; >>> + >>> + dsc->dsc_version_major = 1; >>> + dsc->dsc_version_minor = 1; >>> + >>> + dsc->slice_height = 32; >>> + dsc->slice_count = 2; >>> + // TODO: Get hdisplay from the mode >> >> Would you like to fix the TODO? > > I can't unless either migrating to drm_bridge (is that doable?) or > expand drm_panel. That's a larger task, but I don't think this driver > is the right place to track that desire. Should I drop the comment > entirely or reword it?
I'd say, reword it, so that it becomes more obvious why this TODO can not be fixed at this moment.
> >>> + WARN_ON(1440 % dsc->slice_count); >>> + dsc->slice_width = 1440 / dsc->slice_count; > > <snip> > > - Marijn
-- With best wishes Dmitry
| |