Re: [PATCH] clk: mediatek: mt8188: probe vpp with mtk_clk_simple_probe()

[Yu-chang Lee (李禹璋)]

On Fri, 2023-10-27 at 09:49 +0200, AngeloGioacchino Del Regno wrote:
> Il 27/10/23 09:42, Yu-chang Lee (李禹璋) ha scritto:
> > On Thu, 2023-10-26 at 13:45 +0200, AngeloGioacchino Del Regno
> > wrote:
> > > Il 26/10/23 13:38, yu-chang.lee ha scritto:
> > > > switch to the common mtk_clk_simple_probe() function for all of
> > > > the
> > > > clock drivers that are registering as platform drivers.
> > > > 
> > > 
> > > So VPPSYS0 and VPPSYS1 aren't dependant on MMSYS anymore?
> > > 
> > > Like this, it doesn't look like this will ever work fine, so if
> > > you
> > > want
> > > that to happen, you must provide a good explanation, and then,
> > > since
> > > MT8188
> > > and MT8195's VPPSYS are practically the same, you should also
> > > convert
> > > MT8195
> > > to do the same, and make sure that everything works as expected
> > > before sending
> > > a commit upstream.
> > > 
> > > Please, explain.
> > > 
> > > Thanks,
> > > Angelo
> > > 
> > 
> > Hi Angelo,
> > 
> > Thanks for your time and timely feedback. I don't find mt8188-vpp1
> > and
> > mt8188-vpp0 in mtk-mmsys.c. and thought probed them with
> > mtk_simple_probe(), refer to your comment on in this patch
> > "clk:
> > mediatek: Switch to mtk_clk_simple_probe() where possible", will be
> > a
> 
> Just to clarify: mtk_clk_pdev_probe() is mtk_clk_simple_probe() for
> pdev, nothing more and nothing less :-)
> 
> > good idea. On the other hand mt8195 does have dependency on MMSYS,
> > so I
> > don't think the same change work on mt8195.
> > 
> 
> Ok, then that's simply because MT8188 MDP3 is not upstream yet:
> that's going to
> happen, meaning that MT8188 will have the same mmsys dependency as
> MT8195 soon.
> 
> Sorry, this commit is not valid. NACK.
> 
> Thanks!
> Angelo
> 
> > Best Regards,
> > YuChang
> > 

Thanks for your time and feedback, then I think the appropriate change
here should be adding the vppsys' dependency in mt8188 mmsys.

I submitted another commit titled "soc: mediatek: mmsys: Add support
for MT8188 VPPSYS" according to your feedback. 

Best Regards,
Tony Lee

> > > > Signed-off-by: yu-chang.lee <yu-chang.lee@xxxxxxxxxxxx>
> > > > ---
> > > >    drivers/clk/mediatek/clk-mt8188-vpp0.c | 14 +++++++++++---
> > > >    drivers/clk/mediatek/clk-mt8188-vpp1.c | 14 +++++++++++---
> > > >    2 files changed, 22 insertions(+), 6 deletions(-)
> > > > 
> > > > diff --git a/drivers/clk/mediatek/clk-mt8188-vpp0.c
> > > > b/drivers/clk/mediatek/clk-mt8188-vpp0.c
> > > > index e7b02b26fefb..18fffa191ee1 100644
> > > > --- a/drivers/clk/mediatek/clk-mt8188-vpp0.c
> > > > +++ b/drivers/clk/mediatek/clk-mt8188-vpp0.c
> > > > @@ -96,6 +96,15 @@ static const struct mtk_clk_desc vpp0_desc =
> > > > {
> > > >    	.num_clks = ARRAY_SIZE(vpp0_clks),
> > > >    };
> > > >    
> > > > +static const struct of_device_id of_match_clk_mt8188_vpp0[] =
> > > > {
> > > > +	{
> > > > +		.compatible = "mediatek,mt8188-vppsys0",
> > > > +		.data = &vpp0_desc,
> > > > +	}, {
> > > > +		/* sentinel */
> > > > +	}
> > > > +};
> > > > +
> > > >    static const struct platform_device_id
> > > > clk_mt8188_vpp0_id_table[]
> > > > = {
> > > >    	{ .name = "clk-mt8188-vpp0", .driver_data =
> > > > (kernel_ulong_t)&vpp0_desc },
> > > >    	{ /* sentinel */ }
> > > > @@ -103,12 +112,11 @@ static const struct platform_device_id
> > > > clk_mt8188_vpp0_id_table[] = {
> > > >    MODULE_DEVICE_TABLE(platform, clk_mt8188_vpp0_id_table);
> > > >    
> > > >    static struct platform_driver clk_mt8188_vpp0_drv = {
> > > > -	.probe = mtk_clk_pdev_probe,
> > > > -	.remove_new = mtk_clk_pdev_remove,
> > > > +	.probe = mtk_clk_simple_probe,
> > > >    	.driver = {
> > > >    		.name = "clk-mt8188-vpp0",
> > > > +		.of_match_table = of_match_clk_mt8188_vpp0,
> > > >    	},
> > > > -	.id_table = clk_mt8188_vpp0_id_table,
> > > >    };
> > > >    module_platform_driver(clk_mt8188_vpp0_drv);
> > > >    MODULE_LICENSE("GPL");
> > > > diff --git a/drivers/clk/mediatek/clk-mt8188-vpp1.c
> > > > b/drivers/clk/mediatek/clk-mt8188-vpp1.c
> > > > index e8f0f7eca097..f4b35336d427 100644
> > > > --- a/drivers/clk/mediatek/clk-mt8188-vpp1.c
> > > > +++ b/drivers/clk/mediatek/clk-mt8188-vpp1.c
> > > > @@ -91,6 +91,15 @@ static const struct mtk_clk_desc vpp1_desc =
> > > > {
> > > >    	.num_clks = ARRAY_SIZE(vpp1_clks),
> > > >    };
> > > >    
> > > > +static const struct of_device_id of_match_clk_mt8188_vpp1[] =
> > > > {
> > > > +	{
> > > > +		.compatible = "mediatek,mt8188-vppsys1",
> > > > +		.data = &vpp1_desc,
> > > > +	}, {
> > > > +		/* sentinel */
> > > > +	}
> > > > +};
> > > > +
> > > >    static const struct platform_device_id
> > > > clk_mt8188_vpp1_id_table[]
> > > > = {
> > > >    	{ .name = "clk-mt8188-vpp1", .driver_data =
> > > > (kernel_ulong_t)&vpp1_desc },
> > > >    	{ /* sentinel */ }
> > > > @@ -98,12 +107,11 @@ static const struct platform_device_id
> > > > clk_mt8188_vpp1_id_table[] = {
> > > >    MODULE_DEVICE_TABLE(platform, clk_mt8188_vpp1_id_table);
> > > >    
> > > >    static struct platform_driver clk_mt8188_vpp1_drv = {
> > > > -	.probe = mtk_clk_pdev_probe,
> > > > -	.remove_new = mtk_clk_pdev_remove,
> > > > +	.probe = mtk_clk_simple_probe,
> > > >    	.driver = {
> > > >    		.name = "clk-mt8188-vpp1",
> > > > +		.of_match_table = of_match_clk_mt8188_vpp1,
> > > >    	},
> > > > -	.id_table = clk_mt8188_vpp1_id_table,
> > > >    };
> > > >    module_platform_driver(clk_mt8188_vpp1_drv);
> > > >    MODULE_LICENSE("GPL");
> > > 
> > > 
> 
>