Re: [PATCH v19 03/10] soc: mediatek: add mtk-mmsys support for mt8195 vdosys0

Re: [PATCH v19 03/10] soc: mediatek: add mtk-mmsys support for mt8195 vdosys0

[AngeloGioacchino Del Regno]

Il 15/04/22 10:39, jason-jh.lin ha scritto:
> 1. Add mt8195 mmsys compatible for 2 vdosys.
> 2. Add io_start into each driver data of mt8195 vdosys.
> 3. Add get match data function to identify mmsys by io_start.
> 4. Add mt8195 routing table settings of vdosys0.
> 
> Signed-off-by: jason-jh.lin <jason-jh.lin@mediatek.com>

Unless anyone wants this commit to be split in two, one for the match data
and one for the mt8195 addition, it looks almost good to me, apart one small
change that has to be done, check below:


> --- >   drivers/soc/mediatek/mt8195-mmsys.h    | 370 +++++++++++++++++++++++++
>   drivers/soc/mediatek/mtk-mmsys.c       | 152 +++++++++-
>   drivers/soc/mediatek/mtk-mmsys.h       |   6 +
>   include/linux/soc/mediatek/mtk-mmsys.h |  11 +
>   4 files changed, 528 insertions(+), 11 deletions(-)
>   create mode 100644 drivers/soc/mediatek/mt8195-mmsys.h
> 

..snip..

> diff --git a/drivers/soc/mediatek/mtk-mmsys.h b/drivers/soc/mediatek/mtk-mmsys.h
> index 77f37f8c715b..21cf300ba864 100644
> --- a/drivers/soc/mediatek/mtk-mmsys.h
> +++ b/drivers/soc/mediatek/mtk-mmsys.h
> @@ -87,12 +87,18 @@ struct mtk_mmsys_routes {
>   };
>   
>   struct mtk_mmsys_driver_data {
> +	const u32 io_start;

I agree with you that this iostart is in 32bits boundaries, and that this will
practically never change... and I was in doubt whether this would be acceptable
or not, because of saving some memory.

Even though I would really love to have the savings, in order to avoid having any
"surprises" in the future (any future breakage for "reasons"), we shall comply
with the kernel types, so, this has to be...

	const resource_size_t io_start;

...and this is the same for both this file and mtk_drm_drv.h, which you modify in
patch 07/10.

After fixing that:

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>


Cheers,
Angelo

Re: [PATCH v19 07/10] drm/mediatek: add mediatek-drm of vdosys0 support for mt8195

[AngeloGioacchino Del Regno]

Il 15/04/22 10:39, jason-jh.lin ha scritto:
> 1. Add driver data of mt8195 vdosys0 to mediatek-drm and the sub driver.
> 2. Add get driver data function to identify which vdosys by io_start.
> 
> Signed-off-by: jason-jh.lin <jason-jh.lin@mediatek.com>
> ---
>   drivers/gpu/drm/mediatek/mtk_disp_rdma.c |   6 +
>   drivers/gpu/drm/mediatek/mtk_drm_drv.c   | 141 +++++++++++++++++++++--
>   drivers/gpu/drm/mediatek/mtk_drm_drv.h   |   6 +
>   3 files changed, 145 insertions(+), 8 deletions(-)
> 

..snip..

> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.h b/drivers/gpu/drm/mediatek/mtk_drm_drv.h
> index a58cebd01d35..e1a37ca091f3 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_drv.h
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.h
> @@ -21,6 +21,7 @@ struct drm_property;
>   struct regmap;
>   
>   struct mtk_mmsys_driver_data {
> +	const unsigned int io_start;

As mentioned in patch 03/10, this has to be

	const resource_size_t io_start;

Thanks!

Re: [PATCH v19 10/10] soc: mediatek: remove DDP_DOMPONENT_DITHER enum

[AngeloGioacchino Del Regno]

Il 15/04/22 10:39, jason-jh.lin ha scritto:
> After mmsys and drm change DITHER enum to DDP_COMPONENT_DITHER0,
> mmsys header can remove the useless DDP_COMPONENT_DITHER enum.
> 
> Signed-off-by: jason-jh.lin <jason-jh.lin@mediatek.com>

Can you please fix the commit title with:
soc: mediatek: remove DDP_DOMPONENT_DITHER from enum

that "from" is important, as you're not removing *an enum*, but *from an enum*.

After that:

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

Thanks!
Angelo

Re: [PATCH v19 09/10] drm/mediatek: add postfix 0 to DDP_COMPONENT_DITHER for mt8195 vdosys0

[AngeloGioacchino Del Regno]

Il 15/04/22 10:39, jason-jh.lin ha scritto:
> Because mt8195 vdosys0 has 2 DITHER components,
> so the postfix 0 need to be added to DDP_COMPONENT_DITHER.
> 
> Then DITHER enum will become:
> DDP_COMPONENT_DITHER0 and DDP_COMPONENT_DITHER1.
> 
> Signed-off-by: jason-jh.lin <jason-jh.lin@mediatek.com>

I think that "postfix" should be "suffix" instead :)))

In any case:

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

Re: [PATCH v19 08/10] soc: mediatek: add DDP_DOMPONENT_DITHER0 enum for mt8195 vdosys0

[AngeloGioacchino Del Regno]

Il 15/04/22 10:39, jason-jh.lin ha scritto:
> The mmsys routing table of mt8195 vdosys0 has 2 DITHER components,
> so mmsys need to add DDP_COMPONENT_DITHER1 and change all usages of
> DITHER enum form DDP_COMPONENT_DITHER to DDP_COMPONENT_DITHER0.
> 
> But its header need to keep DDP_COMPONENT_DITHER enum
> until drm/mediatek also changed it.
> 
> Signed-off-by: jason-jh.lin <jason-jh.lin@mediatek.com>

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

Re: [PATCH v19 03/10] soc: mediatek: add mtk-mmsys support for mt8195 vdosys0

[Jason-JH Lin]

Hi Angelo,

Thanks for the reviews.

On Fri, 2022-04-15 at 12:18 +0200, AngeloGioacchino Del Regno wrote:
> Il 15/04/22 10:39, jason-jh.lin ha scritto:
> > 1. Add mt8195 mmsys compatible for 2 vdosys.
> > 2. Add io_start into each driver data of mt8195 vdosys.
> > 3. Add get match data function to identify mmsys by io_start.
> > 4. Add mt8195 routing table settings of vdosys0.
> > 
> > Signed-off-by: jason-jh.lin <jason-jh.lin@mediatek.com>
> 
> Unless anyone wants this commit to be split in two, one for the match
> data
> and one for the mt8195 addition, it looks almost good to me, apart
> one small
> change that has to be done, check below:
> 
> 
> > --- >   drivers/soc/mediatek/mt8195-mmsys.h    | 370
> > +++++++++++++++++++++++++
> >   drivers/soc/mediatek/mtk-mmsys.c       | 152 +++++++++-
> >   drivers/soc/mediatek/mtk-mmsys.h       |   6 +
> >   include/linux/soc/mediatek/mtk-mmsys.h |  11 +
> >   4 files changed, 528 insertions(+), 11 deletions(-)
> >   create mode 100644 drivers/soc/mediatek/mt8195-mmsys.h
> > 
> 
> ..snip..
> 
> > diff --git a/drivers/soc/mediatek/mtk-mmsys.h
> > b/drivers/soc/mediatek/mtk-mmsys.h
> > index 77f37f8c715b..21cf300ba864 100644
> > --- a/drivers/soc/mediatek/mtk-mmsys.h
> > +++ b/drivers/soc/mediatek/mtk-mmsys.h
> > @@ -87,12 +87,18 @@ struct mtk_mmsys_routes {
> >   };
> >   
> >   struct mtk_mmsys_driver_data {
> > +	const u32 io_start;
> 
> I agree with you that this iostart is in 32bits boundaries, and that
> this will
> practically never change... and I was in doubt whether this would be
> acceptable
> or not, because of saving some memory.
> 
> Even though I would really love to have the savings, in order to
> avoid having any
> "surprises" in the future (any future breakage for "reasons"), we
> shall comply
> with the kernel types, so, this has to be...
> 
> 	const resource_size_t io_start;
> 
> ...and this is the same for both this file and mtk_drm_drv.h, which
> you modify in
> patch 07/10.
> 
> After fixing that:
> 
> Reviewed-by: AngeloGioacchino Del Regno <
> angelogioacchino.delregno@collabora.com>
> 
> 
> Cheers,
> Angelo

OK, I'll fix them in the next version.
Thanks.

Regards,
Jason-JH.Lin
-- 
Jason-JH Lin <jason-jh.lin@mediatek.com>

Re: [PATCH v19 09/10] drm/mediatek: add postfix 0 to DDP_COMPONENT_DITHER for mt8195 vdosys0

[Jason-JH Lin]

Hi Angelo,

Thanks for the reviews.

On Fri, 2022-04-15 at 12:32 +0200, AngeloGioacchino Del Regno wrote:
> Il 15/04/22 10:39, jason-jh.lin ha scritto:
> > Because mt8195 vdosys0 has 2 DITHER components,
> > so the postfix 0 need to be added to DDP_COMPONENT_DITHER.
> > 
> > Then DITHER enum will become:
> > DDP_COMPONENT_DITHER0 and DDP_COMPONENT_DITHER1.
> > 
> > Signed-off-by: jason-jh.lin <jason-jh.lin@mediatek.com>
> 
> I think that "postfix" should be "suffix" instead :)))
> 
OK, I'll fix it.
Thanks.

Regards,
Jason-JH.Lin

> In any case:
> 
> Reviewed-by: AngeloGioacchino Del Regno <
> angelogioacchino.delregno@collabora.com>
> 
> 
> _______________________________________________
> Linux-mediatek mailing list
> Linux-mediatek@lists.infradead.org
> 
https://urldefense.com/v3/__http://lists.infradead.org/mailman/listinfo/linux-mediatek__;!!CTRNKA9wMg0ARbw!0VHqcu_Bx0oyxup8Fj2rgqHuGYhj6UYv4YNNbEr3K8CBkZNPEQxjbrjcw6OccXWGif6P$
>  
-- 
Jason-JH Lin <jason-jh.lin@mediatek.com>

Re: [PATCH v19 10/10] soc: mediatek: remove DDP_DOMPONENT_DITHER enum

[Jason-JH Lin]

Hi Angelo,

Thanks for the reviews.

On Fri, 2022-04-15 at 12:32 +0200, AngeloGioacchino Del Regno wrote:
> Il 15/04/22 10:39, jason-jh.lin ha scritto:
> > After mmsys and drm change DITHER enum to DDP_COMPONENT_DITHER0,
> > mmsys header can remove the useless DDP_COMPONENT_DITHER enum.
> > 
> > Signed-off-by: jason-jh.lin <jason-jh.lin@mediatek.com>
> 
> Can you please fix the commit title with:
> soc: mediatek: remove DDP_DOMPONENT_DITHER from enum
> 
> that "from" is important, as you're not removing *an enum*, but *from
> an enum*.
> 
Yes, I agree with you.
I'll fix it in the next version.
Thanks.

Regards,
Jason-JH.Lin

> After that:
> 
> Reviewed-by: AngeloGioacchino Del Regno <
> angelogioacchino.delregno@collabora.com>
> 
> Thanks!
> Angelo
-- 
Jason-JH Lin <jason-jh.lin@mediatek.com>