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

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

[AngeloGioacchino Del Regno]

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

> Signed-off-by: yu-chang.lee <yu-chang.lee@mediatek.com>
> ---
>   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");

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

[Yu-chang Lee (李禹璋)]

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
good idea. On the other hand mt8195 does have dependency on MMSYS, so I
don't think the same change work on mt8195.

Best Regards,
YuChang

> > Signed-off-by: yu-chang.lee <yu-chang.lee@mediatek.com>
> > ---
> >   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");
> 
> 

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

[AngeloGioacchino Del Regno]

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
> 
>>> Signed-off-by: yu-chang.lee <yu-chang.lee@mediatek.com>
>>> ---
>>>    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");
>>
>>

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@mediatek.com>
> > > > ---
> > > >    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");
> > > 
> > > 
> 
>