On Tue, 2022-08-23 at 16:16 -0400, Nícolas F. R. A. Prado wrote: > On Tue, Aug 23, 2022 at 02:18:37PM +0800, xinlei.lee@xxxxxxxxxxxx > wrote: > > From: Xinlei Lee <xinlei.lee@xxxxxxxxxxxx> > > > > Dpi output needs to adjust the output format to dual edge for > > MT8186. > > Because MT8186 HW has been modified at that time, SW needs to > > cooperate. > > And the register (MMSYS) reserved for dpi will be used for output > > format control (dual_edge/single_edge). > > > > Co-developed-by: Jitao Shi <jitao.shi@xxxxxxxxxxxx> > > Signed-off-by: Jitao Shi <jitao.shi@xxxxxxxxxxxx> > > Signed-off-by: Xinlei Lee <xinlei.lee@xxxxxxxxxxxx> > > > > --- > > [..] > > --- a/drivers/gpu/drm/mediatek/mtk_dpi.c > > +++ b/drivers/gpu/drm/mediatek/mtk_dpi.c > > [..] > > * @yuv422_en_bit: Enable bit of yuv422. > > * @csc_enable_bit: Enable bit of CSC. > > * @pixels_per_iter: Quantity of transferred pixels per iteration. > > + * @rgb888_dual_enable: Control output format for mt8186. > > Let's not mention mt8186 in the description to keep the property > generic. Also, > this description should say what having 'rgb888_dual_enable = true' > indicates > about the hardware (in this case mt8186) and it currently doesn't. > > Let's take a step back. What does 'dual enable' mean in this context > and how > does it relate to 'dual edge' and the dpi output format? By answering > those > questions we can find a description (and maybe variable name) that > makes more > sense. > > > */ > > [..] > > @@ -449,6 +454,9 @@ 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->rgb888_dual_enable) > > + mtk_mmsys_ddp_dpi_fmt_config(dpi->mmsys_dev, > > DPI_RGB888_DDR_CON, > > + DPI_FORMAT_MASK, NULL); > > This if block should be further indented. > > > } else { > > mtk_dpi_mask(dpi, DPI_DDR_SETTING, DDR_EN | DDR_4PHASE, > > 0); > > } > > [..] > > --- a/drivers/gpu/drm/mediatek/mtk_dpi_regs.h > > +++ b/drivers/gpu/drm/mediatek/mtk_dpi_regs.h > > @@ -235,4 +235,8 @@ > > #define MATRIX_SEL_RGB_TO_JPEG 0 > > #define MATRIX_SEL_RGB_TO_BT601 2 > > > > +#define DPI_FORMAT_MASK 0x1 > > +#define DPI_RGB888_DDR_CON BIT(0) > > +#define DPI_RGB565_SDR_CON BIT(1) > > I'm not sure if it would make more sense to have these definitions in > the mmsys > header since they're configurations of a register in mmsys' > iospace... I think > we can keep them here but at least add a comment above: > > /* Values for DPI configuration in MMSYS address space */ > > Thanks, > Nícolas Hi Nícolas: Thanks for your careful review! I will modify the description of this member variable and add the hardware state corresponding to the software setting. (eg. rgb888_dual_enable = true the hardware output rgb888_dual_edge format data) Your suggestion is very necessary, maybe my name is not accurate enough, this flag is to enable RGB888_dual_edge format output. Would it be better for the variable to be called RGB888_dual_edge_enable then? Also I'll fix the code formatting issues and add descriptions to the mmsys registers in the mtk_dpi_regs.h file. Best Regards! xinlei