On Fri, 2022-10-21 at 11:39 -0400, Nícolas F. R. A. Prado wrote: > On Fri, Oct 21, 2022 at 08:18:25PM +0800, xinlei.lee wrote: > > On Thu, 2022-10-20 at 12:40 -0400, Nícolas F. R. A. Prado wrote: > > > On Wed, Oct 19, 2022 at 10:52:15AM +0800, xinlei.lee@xxxxxxxxxxxx > > > wrote: > > [..] > > > > @@ -448,8 +453,12 @@ static void mtk_dpi_dual_edge(struct > > > > mtk_dpi > > > > *dpi) > > > > mtk_dpi_mask(dpi, DPI_OUTPUT_SETTING, > > > > dpi->output_fmt == > > > > MEDIA_BUS_FMT_RGB888_2X12_LE ? > > > > EDGE_SEL : 0, EDGE_SEL); > > > > + if (dpi->conf->edge_cfg_in_mmsys) > > > > + mtk_mmsys_ddp_dpi_fmt_config(dpi- > > > > >mmsys_dev, > > > > MTK_DPI_RGB888_DDR_CON); > > > > } else { > > > > mtk_dpi_mask(dpi, DPI_DDR_SETTING, DDR_EN | > > > > DDR_4PHASE, > > > > 0); > > > > + if (dpi->conf->edge_cfg_in_mmsys) > > > > + mtk_mmsys_ddp_dpi_fmt_config(dpi- > > > > >mmsys_dev, > > > > MTK_DPI_RGB888_SDR_CON); > > > > > > I know this isn't one of the formats supported by MT8186, but > > > since > > > we're using > > > platform-agnostic formats now... This else branch in theory could > > > also run for a > > > format like MEDIA_BUS_FMT_YUYV8_1X16. Would it make sense to set > > > MTK_DPI_RGB888_SDR_CON in that case? > > > > > > Thanks, > > > Nícolas > > > > > > > } > > > > > > [..] > > > > Hi Nícolas: > > > > Thanks for your review! > > > > You are right, I understand you think this MTK_DPI_RGB888_SDR_CON > > format seems useless as it will not be set, I confirmed with the > > designer how the setting in mmsys affects the output format of the > > MT8186, this mmsys setting will not be used by other ICs. > > > > As mentioned earlier, the mmsys setting will make the MT8186dpi > > have > > four output formats, even though the MT8186 dpi may not use them > > all. > > > > So what needs to change here? > > We could check that the format in the else path is a single edge > RGB888 format > like MEDIA_BUS_FMT_RGB888_1X24 before setting the mmsys config, but > there are > also other formats possible, and I actually don't think it's worth it > to > complicate the logic further to protect from an edge-case that can't > be hit > yet... > > So just leave it as it is. We can worry about it when/if a non-RGB888 > single > edge format needs to be setup on mmsys. > > So, > > Reviewed-by: Nícolas F. R. A. Prado <nfraprado@xxxxxxxxxxxxx> > > Thanks, > Nícolas Hi Nícolas: Thanks for your review! Best Regards! xinlei