[PATCH v3,2/2] drm: mediatek: Adjust the dpi output format to MT8186

[xinlei.lee]

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 at mediatek.com
> wrote:
> > From: Xinlei Lee <xinlei.lee at mediatek.com>
> > 
> > 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 at mediatek.com>
> > Signed-off-by: Jitao Shi <jitao.shi at mediatek.com>
> > Signed-off-by: Xinlei Lee <xinlei.lee at mediatek.com>
> > 
> > ---
> 
> [..]
> > --- 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