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@xxxxxxxxxxxx
> wrote:
> > From: Xinlei Lee <xinlei.lee@xxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxx>
> > Reviewed-by: AngeloGioacchino Del Regno <
> > angelogioacchino.delregno@xxxxxxxxxxxxx>
> > Reviewed-by: CK Hu <ck.hu@xxxxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxxxxx>
>
> 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
