[PATCH v12,1/3] soc: mediatek: Add all settings to mtk_mmsys_ddp_dpi_fmt_config func

[xinlei.lee]

On Thu, 2022-10-20 at 12:33 -0400, Nícolas F. R. A. Prado wrote:
> Hi,
> 
> On Wed, Oct 19, 2022 at 10:52:14AM +0800, xinlei.lee at mediatek.com
> wrote:
> > From: Xinlei Lee <xinlei.lee at mediatek.com>
> > 
> > The difference between MT8186 and other ICs is that when modifying
> > the
> > output format, we need to modify the mmsys_base+0x400 register to
> > take
> > effect.
> > So when setting the dpi output format, we need to call mmsys_func
> > to set
> 
> mmsys_func isn't something that exists in the code. Instead mention
> the actual
> function name: mtk_mmsys_ddp_dpi_fmt_config.
> 
> > it to MT8186 synchronously.
> 
> 
> Here, before saying that the commit adds all the settings for dpi,
> you could
> have mentioned that the previous commit lacked those, to make it
> clearer:
> 
> Commit a071e52f75d1 ("soc: mediatek: Add mmsys func to adapt to dpi
> output for MT8186")
> lacked some of the possible output formats and also had a wrong
> bitmask.
> 
> 
> > Adding mmsys all the settings that need to be modified with dpi are
> > for
> > mt8186.
> 
> This sentence I would change to the following one:
> 
> Add the missing output formats and fix the bitmask.
> 
> 
> Finally, you're also making the function more HW-agnostic (although
> in my
> opinion this could've been a future separate commit), so it's worth
> mentioning
> it here:
> 
> While at it, also update mtk_mmsys_ddp_dpi_fmt_config() to use
> generic formats,
> so that it is slightly easier to extend for other platforms.
> 
> > 
> > Fixes: a071e52f75d1 ("soc: mediatek: Add mmsys func to adapt to dpi
> > output for MT8186")
> 
> The fixes tag should be kept in a single line, without wrapping.
> 
> > 
> > Signed-off-by: Xinlei Lee <xinlei.lee at mediatek.com>
> > Reviewed-by: AngeloGioacchino Del Regno <
> > angelogioacchino.delregno at collabora.com>
> > Reviewed-by: CK Hu <ck.hu at mediatek.com>
> > ---
> >  drivers/soc/mediatek/mt8186-mmsys.h    |  8 +++++---
> >  drivers/soc/mediatek/mtk-mmsys.c       | 27 ++++++++++++++++++++
> > ------
> >  include/linux/soc/mediatek/mtk-mmsys.h |  7 +++++++
> >  3 files changed, 33 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/soc/mediatek/mt8186-mmsys.h
> > b/drivers/soc/mediatek/mt8186-mmsys.h
> > index 09b1ccbc0093..035aec1eb616 100644
> > --- a/drivers/soc/mediatek/mt8186-mmsys.h
> > +++ b/drivers/soc/mediatek/mt8186-mmsys.h
> > @@ -5,9 +5,11 @@
> >  
> >  /* Values for DPI configuration in MMSYS address space */
> >  #define MT8186_MMSYS_DPI_OUTPUT_FORMAT		0x400
> > -#define DPI_FORMAT_MASK					0x1
> > -#define DPI_RGB888_DDR_CON				BIT(0)
> > -#define DPI_RGB565_SDR_CON				BIT(1)
> > +#define DPI_FORMAT_MASK					GENMASK
> > (1, 0)
> > +#define DPI_RGB888_SDR_CON				0
> > +#define DPI_RGB888_DDR_CON				1
> > +#define DPI_RGB565_SDR_CON				2
> > +#define DPI_RGB565_DDR_CON				3
> 
> These defines should all have a MT8186_ prefix. This will avoid
> confusions now
> that mtk_mmsys_ddp_dpi_fmt_config() is being made more platform-
> agnostic.
> 
> >  
> >  #define MT8186_MMSYS_OVL_CON			0xF04
> >  #define MT8186_MMSYS_OVL0_CON_MASK			0x3
> > diff --git a/drivers/soc/mediatek/mtk-mmsys.c
> > b/drivers/soc/mediatek/mtk-mmsys.c
> > index d2c7a87aab87..205f6de45851 100644
> > --- a/drivers/soc/mediatek/mtk-mmsys.c
> > +++ b/drivers/soc/mediatek/mtk-mmsys.c
> > @@ -238,12 +238,27 @@ static void mtk_mmsys_update_bits(struct
> > mtk_mmsys *mmsys, u32 offset, u32 mask,
> >  
> >  void mtk_mmsys_ddp_dpi_fmt_config(struct device *dev, u32 val)
> >  {
> > -	if (val)
> > -		mtk_mmsys_update_bits(dev_get_drvdata(dev),
> > MT8186_MMSYS_DPI_OUTPUT_FORMAT,
> > -				      DPI_RGB888_DDR_CON,
> > DPI_FORMAT_MASK);
> > -	else
> > -		mtk_mmsys_update_bits(dev_get_drvdata(dev),
> > MT8186_MMSYS_DPI_OUTPUT_FORMAT,
> > -				      DPI_RGB565_SDR_CON,
> > DPI_FORMAT_MASK);
> > +	struct mtk_mmsys *mmsys = dev_get_drvdata(dev);
> > +
> > +	switch (val) {
> > +	case MTK_DPI_RGB888_SDR_CON:
> > +		mtk_mmsys_update_bits(mmsys,
> > MT8186_MMSYS_DPI_OUTPUT_FORMAT,
> > +				      DPI_FORMAT_MASK,
> > DPI_RGB888_SDR_CON);
> > +		break;
> > +	case MTK_DPI_RGB565_SDR_CON:
> > +		mtk_mmsys_update_bits(mmsys,
> > MT8186_MMSYS_DPI_OUTPUT_FORMAT,
> > +				      DPI_FORMAT_MASK,
> > DPI_RGB565_SDR_CON);
> > +		break;
> > +	case MTK_DPI_RGB565_DDR_CON:
> > +		mtk_mmsys_update_bits(mmsys,
> > MT8186_MMSYS_DPI_OUTPUT_FORMAT,
> > +				      DPI_FORMAT_MASK,
> > DPI_RGB565_DDR_CON);
> > +		break;
> > +	case MTK_DPI_RGB888_DDR_CON:
> > +	default:
> > +		mtk_mmsys_update_bits(mmsys,
> > MT8186_MMSYS_DPI_OUTPUT_FORMAT,
> > +				      DPI_FORMAT_MASK,
> > DPI_RGB888_DDR_CON);
> > +		break;
> > +	}
> 
> To be honest I don't really see the point of making the function
> slightly more
> platform-agnostic like this. With a single platform making use of it
> it's just
> an unneeded extra abstraction, and it could easily be done when a
> second
> platform starts requiring this as well...
> 
> In any case,
> 
> Reviewed-by: Nícolas F. R. A. Prado <nfraprado at collabora.com>
> 
> Thanks,
> Nícolas
> 
> >  }
> 
> [..]

Hi Nícolas:

Thanks for your detailed reply and correction.
Before sending out the next edition, I have two questions I would like 
to confirm with you in response to your responses:
1.While at it, also update mtk_mmsys_ddp_dpi_fmt_config() to use 
generic formats, so that it is slightly easier to extend for other 
platforms.
=> This is to make this mtk_mmsys_ddp_dpi_fmt_config() func more 
general? 
This function may only be used by MT8186, because only MT8186
has 
corresponding modifications on HW, and enables the registers reserved 
in mmsys for dpi use to control the output format. Because this 
register is not defined for other ic, I added control to this function 
call in mtk_dpi.c. If you think there are other ways to make it look 
more generic, where should I correct it?

2. These definitions should all have a MT8186_ prefix. This will avoid 
confusion as mtk_mmsys_ddp_dpi_fmt_config() becomes more platform 
independent.

Honestly, I don't really see the point of making the feature platform-
agnostic like this. Using it on a single platform is just an extra 
abstraction that isn't needed, when a second platform starts needing 
it too, it can be done easily...

=> My understanding here is that prefixing variables with labels is 
more conducive to making functions generic, and can be reused if there 
is such a situation in the future. I understand the importance of 
keeping the function platform agnostic, but as mentioned, it may only 
be used by the MT8186 if there are special cases where other ICs may 
rely on mtk_mmsys_update_bits to create new functions.

The above content is only my understanding. If you have any questions 
or suggestions, we will communicate again.

Looking forward to your reply.

Best Regards!
xinlei