diff mbox series

[3/6] clk: mediatek: reset: Return reset data pointer on register

Message ID 20220519134728.456643-4-y.oudjana@protonmail.com (mailing list archive)
State New, archived
Headers show
Series clk: mediatek: Improvements to simple probe/remove and reset controller unregistration | expand

Commit Message

Yassine Oudjana May 19, 2022, 1:47 p.m. UTC
From: Yassine Oudjana <y.oudjana@protonmail.com>

Return a struct mtk_clk_rst_data * when registering a reset
controller in preparation for adding an unregister helper
that will take it as an argument. Make the necessary changes
in drivers that do not currently discard the return value
of register functions.

Signed-off-by: Yassine Oudjana <y.oudjana@protonmail.com>
---
Dependencies:
- clk: mediatek: Move to struct clk_hw provider APIs (series)
  https://patchwork.kernel.org/project/linux-mediatek/cover/20220510104804.544597-1-wenst@chromium.org/ 
- Cleanup MediaTek clk reset drivers and support MT8192/MT8195 (series)
  https://patchwork.kernel.org/project/linux-mediatek/cover/20220503093856.22250-1-rex-bc.chen@mediatek.com/
- Export required symbols to compile clk drivers as module (single patch)
  https://patchwork.kernel.org/project/linux-mediatek/patch/20220518111652.223727-7-angelogioacchino.delregno@collabora.com/

 drivers/clk/mediatek/clk-mt8192.c |  7 +++++--
 drivers/clk/mediatek/clk-mtk.c    |  9 +++++---
 drivers/clk/mediatek/reset.c      | 34 ++++++++++++++++---------------
 drivers/clk/mediatek/reset.h      | 14 +++++++------
 4 files changed, 37 insertions(+), 27 deletions(-)

Comments

Rex-BC Chen (陳柏辰) May 20, 2022, 5:56 a.m. UTC | #1
On Thu, 2022-05-19 at 17:47 +0400, Yassine Oudjana wrote:
> From: Yassine Oudjana <y.oudjana@protonmail.com>
> 
> Return a struct mtk_clk_rst_data * when registering a reset
> controller in preparation for adding an unregister helper
> that will take it as an argument. Make the necessary changes
> in drivers that do not currently discard the return value
> of register functions.
> 
> Signed-off-by: Yassine Oudjana <y.oudjana@protonmail.com>
> ---
> Dependencies:
> - clk: mediatek: Move to struct clk_hw provider APIs (series)
>   
> https://urldefense.com/v3/__https://patchwork.kernel.org/project/linux-mediatek/cover/20220510104804.544597-1-wenst@chromium.org/__;!!CTRNKA9wMg0ARbw!1TS-6hbS7UPn08ETCuNFzymINPNyp_PlQ22cQbJVNp6vDRjgREzDVlLjvsmyN1YkE77G$ 
>  
> - Cleanup MediaTek clk reset drivers and support MT8192/MT8195
> (series)
>   
> https://urldefense.com/v3/__https://patchwork.kernel.org/project/linux-mediatek/cover/20220503093856.22250-1-rex-bc.chen@mediatek.com/__;!!CTRNKA9wMg0ARbw!1TS-6hbS7UPn08ETCuNFzymINPNyp_PlQ22cQbJVNp6vDRjgREzDVlLjvsmyNwSqs3wS$
>  
> - Export required symbols to compile clk drivers as module (single
> patch)
>   
> https://urldefense.com/v3/__https://patchwork.kernel.org/project/linux-mediatek/patch/20220518111652.223727-7-angelogioacchino.delregno@collabora.com/__;!!CTRNKA9wMg0ARbw!1TS-6hbS7UPn08ETCuNFzymINPNyp_PlQ22cQbJVNp6vDRjgREzDVlLjvsmyNwGDWf68$
>  
> 
>  drivers/clk/mediatek/clk-mt8192.c |  7 +++++--
>  drivers/clk/mediatek/clk-mtk.c    |  9 +++++---
>  drivers/clk/mediatek/reset.c      | 34 ++++++++++++++++-------------
> --
>  drivers/clk/mediatek/reset.h      | 14 +++++++------
>  4 files changed, 37 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/clk/mediatek/clk-mt8192.c
> b/drivers/clk/mediatek/clk-mt8192.c
> index ebbd2798d9a3..a658a74644de 100644
> --- a/drivers/clk/mediatek/clk-mt8192.c
> +++ b/drivers/clk/mediatek/clk-mt8192.c
> @@ -1255,6 +1255,7 @@ static int clk_mt8192_infra_probe(struct
> platform_device *pdev)
>  {
>  	struct clk_hw_onecell_data *clk_data;
>  	struct device_node *node = pdev->dev.of_node;
> +	struct mtk_clk_rst_data *rst_data;
>  	int r;
>  
>  	clk_data = mtk_alloc_clk_data(CLK_INFRA_NR_CLK);
> @@ -1265,9 +1266,11 @@ static int clk_mt8192_infra_probe(struct
> platform_device *pdev)
>  	if (r)
>  		goto free_clk_data;
>  
> -	r = mtk_register_reset_controller_with_dev(&pdev->dev,
> &clk_rst_desc);
> -	if (r)
> +	rst_data = mtk_register_reset_controller_with_dev(&pdev->dev,
> &clk_rst_desc);
> +	if (IS_ERR(rst_data)) {
> +		r = PTR_ERR(rst_data);
>  		goto free_clk_data;
> +	}
>  
>  	r = of_clk_add_hw_provider(node, of_clk_hw_onecell_get,
> clk_data);
>  	if (r)
> diff --git a/drivers/clk/mediatek/clk-mtk.c
> b/drivers/clk/mediatek/clk-mtk.c
> index 3a8875b6c37f..1b5591733e2b 100644
> --- a/drivers/clk/mediatek/clk-mtk.c
> +++ b/drivers/clk/mediatek/clk-mtk.c
> @@ -424,6 +424,7 @@ int mtk_clk_simple_probe(struct platform_device
> *pdev)
>  	const struct mtk_clk_desc *mcd;
>  	struct clk_hw_onecell_data *clk_data;
>  	struct device_node *node = pdev->dev.of_node;
> +	struct mtk_clk_rst_data *rst_data;
>  	int r;
>  
>  	mcd = of_device_get_match_data(&pdev->dev);
> @@ -446,10 +447,12 @@ int mtk_clk_simple_probe(struct platform_device
> *pdev)
>  	platform_set_drvdata(pdev, clk_data);
>  
>  	if (mcd->rst_desc) {
> -		r = mtk_register_reset_controller_with_dev(&pdev->dev,
> -							   mcd-
> >rst_desc);
> -		if (r)
> +		rst_data =
> mtk_register_reset_controller_with_dev(&pdev->dev,
> +							   	  mcd
> ->rst_desc);
> +		if (IS_ERR(rst_data)) {
> +			r = PTR_ERR(rst_data);
>  			goto unregister_clks;
> +		}
>  	}
>  
>  	return r;
> diff --git a/drivers/clk/mediatek/reset.c
> b/drivers/clk/mediatek/reset.c
> index 290ceda84ce4..09862baf1d57 100644
> --- a/drivers/clk/mediatek/reset.c
> +++ b/drivers/clk/mediatek/reset.c
> @@ -110,8 +110,9 @@ static int reset_xlate(struct
> reset_controller_dev *rcdev,
>  	return data->desc->rst_idx_map[reset_spec->args[0]];
>  }
>  
> -int mtk_register_reset_controller(struct device_node *np,
> -				  const struct mtk_clk_rst_desc *desc)
> +struct mtk_clk_rst_data
> +*mtk_register_reset_controller(struct device_node *np,
> +			       const struct mtk_clk_rst_desc *desc)
>  {
>  	struct regmap *regmap;
>  	const struct reset_control_ops *rcops = NULL;
> @@ -120,7 +121,7 @@ int mtk_register_reset_controller(struct
> device_node *np,
>  
>  	if (!desc) {
>  		pr_err("mtk clock reset desc is NULL\n");
> -		return -EINVAL;
> +		return ERR_PTR(-EINVAL);
>  	}
>  
>  	switch (desc->version) {
> @@ -132,18 +133,18 @@ int mtk_register_reset_controller(struct
> device_node *np,
>  		break;
>  	default:
>  		pr_err("Unknown reset version %d\n", desc->version);
> -		return -EINVAL;
> +		return ERR_PTR(-EINVAL);
>  	}
>  
>  	regmap = device_node_to_regmap(np);
>  	if (IS_ERR(regmap)) {
>  		pr_err("Cannot find regmap for %pOF: %pe\n", np,
> regmap);
> -		return -EINVAL;
> +		return ERR_PTR(-EINVAL);
>  	}
>  
>  	data = kzalloc(sizeof(*data), GFP_KERNEL);
>  	if (!data)
> -		return -ENOMEM;
> +		return ERR_PTR(-ENOMEM);
>  
>  	data->desc = desc;
>  	data->regmap = regmap;
> @@ -163,14 +164,15 @@ int mtk_register_reset_controller(struct
> device_node *np,
>  	if (ret) {
>  		pr_err("could not register reset controller: %d\n",
> ret);
>  		kfree(data);
> -		return ret;
> +		return ERR_PTR(ret);
>  	}
>  
> -	return 0;
> +	return data;
>  }
>  
> -int mtk_register_reset_controller_with_dev(struct device *dev,
> -					   const struct
> mtk_clk_rst_desc *desc)
> +struct mtk_clk_rst_data
> +*mtk_register_reset_controller_with_dev(struct device *dev,
> +					const struct mtk_clk_rst_desc
> *desc)
>  {
>  	struct device_node *np = dev->of_node;
>  	struct regmap *regmap;
> @@ -180,7 +182,7 @@ int mtk_register_reset_controller_with_dev(struct
> device *dev,
>  
>  	if (!desc) {
>  		dev_err(dev, "mtk clock reset desc is NULL\n");
> -		return -EINVAL;
> +		return ERR_PTR(-EINVAL);
>  	}
>  
>  	switch (desc->version) {
> @@ -192,18 +194,18 @@ int
> mtk_register_reset_controller_with_dev(struct device *dev,
>  		break;
>  	default:
>  		dev_err(dev, "Unknown reset version %d\n", desc-
> >version);
> -		return -EINVAL;
> +		return ERR_PTR(-EINVAL);
>  	}
>  
>  	regmap = device_node_to_regmap(np);
>  	if (IS_ERR(regmap)) {
>  		dev_err(dev, "Cannot find regmap %pe\n", regmap);
> -		return -EINVAL;
> +		return ERR_PTR(-EINVAL);
>  	}
>  
>  	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
>  	if (!data)
> -		return -ENOMEM;
> +		return ERR_PTR(-ENOMEM);
>  
>  	data->desc = desc;
>  	data->regmap = regmap;
> @@ -223,10 +225,10 @@ int
> mtk_register_reset_controller_with_dev(struct device *dev,
>  	ret = devm_reset_controller_register(dev, &data->rcdev);
>  	if (ret) {
>  		dev_err(dev, "could not register reset controller:
> %d\n", ret);
> -		return ret;
> +		return ERR_PTR(ret);
>  	}
>  
> -	return 0;
> +	return data;
>  }
>  EXPORT_SYMBOL_GPL(mtk_register_reset_controller_with_dev);
>  
> diff --git a/drivers/clk/mediatek/reset.h
> b/drivers/clk/mediatek/reset.h
> index 913fe676cba7..7418dd0d046f 100644
> --- a/drivers/clk/mediatek/reset.h
> +++ b/drivers/clk/mediatek/reset.h
> @@ -64,19 +64,21 @@ struct mtk_clk_rst_data {
>   * @np: Pointer to device node.
>   * @desc: Constant pointer to description of clock reset.
>   *
> - * Return: 0 on success and errorno otherwise.
> + * Return: Pointer to struct mtk_clk_rst_data on success and error
> pointer otherwise.
>   */
> -int mtk_register_reset_controller(struct device_node *np,
> -				  const struct mtk_clk_rst_desc *desc);
> +struct mtk_clk_rst_data
> +*mtk_register_reset_controller(struct device_node *np,
> +			       const struct mtk_clk_rst_desc *desc);
>  
>  /**
>   * mtk_register_reset_controller - Register mediatek clock reset
> controller with device
>   * @np: Pointer to device.
>   * @desc: Constant pointer to description of clock reset.
>   *
> - * Return: 0 on success and errorno otherwise.
> + * Return: Pointer to struct mtk_clk_rst_data on success and error
> pointer otherwise.
>   */
> -int mtk_register_reset_controller_with_dev(struct device *dev,
> -					   const struct
> mtk_clk_rst_desc *desc);
> +struct mtk_clk_rst_data
> +*mtk_register_reset_controller_with_dev(struct device *dev,
> +					const struct mtk_clk_rst_desc
> *desc);
>  
>  #endif /* __DRV_CLK_MTK_RESET_H */
> -- 
> 2.36.1
> 

Hello,

Stephen wants me to use  "auxiliary bus" in [1].
I am not sure why it didn't appear in lore, so I add the message.
I said I will find some time to do this after my reset cleanup series.
If so, I think we don't need to modify this in this time?

-----
Quoting Rex-BC Chen (2022-05-08 22:35:55)
> 
> The drivers of this series are reviewed.
> The binding of this series are also acked.
> Could you spare some time and give us some suggestion?

Have you considered using the auxiliary bus to split the Mediatek clk
and reset device up into a clk device and a reset device? The idea
would be to move the reset related code into drivers/reset and have the
clk code in drivers/clk. It's purely an organizational thing and it can
certainly be done later but it may be a good idea to do this to clearly
split out the two different functionalities.
-----

[1]:
https://lore.kernel.org/all/20220503093856.22250-1-rex-bc.chen@mediatek.com/

BRs,
Rex
AngeloGioacchino Del Regno May 20, 2022, 8:42 a.m. UTC | #2
Il 19/05/22 15:47, Yassine Oudjana ha scritto:
> From: Yassine Oudjana <y.oudjana@protonmail.com>
> 
> Return a struct mtk_clk_rst_data * when registering a reset
> controller in preparation for adding an unregister helper
> that will take it as an argument. Make the necessary changes
> in drivers that do not currently discard the return value
> of register functions.
> 
> Signed-off-by: Yassine Oudjana <y.oudjana@protonmail.com>

Hello Yassine,

Thanks for your efforts on helping to make the MediaTek clocks better - I agree
(and I'm not the only one..) that there's a lot of work to do on this side.

Though... I don't think that this is the right direction: you're right about
properly unregistering (in patch 4/6) the reset controllers on rmmod/failure
but I'm not sure that this kind of noise brings any benefit.

Explaining:
You definitely saw that there's a new register _with_dev, which uses devm ops
and that's going to automatically cleanup in case of removal/failure.
This is what we should do.

Hence, my proposal is to drop patch 3/6, 4/6, 5/6 and (slowly, steadily) migrate
all of the MediaTek clocks from CLK_OF_DECLARE() to platform drivers (which also
means that we can eventually change them to tristate!), so that we slowly remove
all users of all functions that are not "_with_dev", and that we finally remove
all of these then-unused functions as well.

Making sure that I don't get misunderstood:
      I'm not implying that this huge migration work is on your shoulders!

P.S.: Chen-Yu, Miles: do you also agree? :-)

Cheers,
Angelo
Chen-Yu Tsai May 20, 2022, 9:02 a.m. UTC | #3
On Fri, May 20, 2022 at 4:42 PM AngeloGioacchino Del Regno
<angelogioacchino.delregno@collabora.com> wrote:
>
> Il 19/05/22 15:47, Yassine Oudjana ha scritto:
> > From: Yassine Oudjana <y.oudjana@protonmail.com>
> >
> > Return a struct mtk_clk_rst_data * when registering a reset
> > controller in preparation for adding an unregister helper
> > that will take it as an argument. Make the necessary changes
> > in drivers that do not currently discard the return value
> > of register functions.
> >
> > Signed-off-by: Yassine Oudjana <y.oudjana@protonmail.com>
>
> Hello Yassine,
>
> Thanks for your efforts on helping to make the MediaTek clocks better - I agree
> (and I'm not the only one..) that there's a lot of work to do on this side.
>
> Though... I don't think that this is the right direction: you're right about
> properly unregistering (in patch 4/6) the reset controllers on rmmod/failure
> but I'm not sure that this kind of noise brings any benefit.

Since Rex is working on cleaning up the reset bits, maybe also coordinate
with him?

> Explaining:
> You definitely saw that there's a new register _with_dev, which uses devm ops
> and that's going to automatically cleanup in case of removal/failure.

I don't think they use devres at the moment. They should, but I haven't
gotten that far with my improvements. *wink*

> This is what we should do.
>
> Hence, my proposal is to drop patch 3/6, 4/6, 5/6 and (slowly, steadily) migrate
> all of the MediaTek clocks from CLK_OF_DECLARE() to platform drivers (which also
> means that we can eventually change them to tristate!), so that we slowly remove
> all users of all functions that are not "_with_dev", and that we finally remove
> all of these then-unused functions as well.

I agree with moving all the drivers off of CLK_OF_DECLARE() if possible.
There are definitely going to be a few ARM32 ones that can't be converted,
likely because they don't have the ARM arch timer, and need a clock
registered before the timers. Been there.

And there's a whole lot of work to be done for all the old drivers so that
they clean up after themselves.

My desire is to clean up the API (the naming and the parameters) so that
they more closely match up with the underlying CCF APIs they call. We can
then add devres variants (and of_ ones for the unfortunate platforms that
need them).

> Making sure that I don't get misunderstood:
>       I'm not implying that this huge migration work is on your shoulders!

Yeah. I'll likely take up quite a bit of it after all my currently planned
cleanup work is done. Unless of course someone beats me to it.

> P.S.: Chen-Yu, Miles: do you also agree? :-)

Yes.


Regards
ChenYu
Miles Chen May 20, 2022, 9:08 a.m. UTC | #4
> Il 19/05/22 15:47, Yassine Oudjana ha scritto:
> > From: Yassine Oudjana <y.oudjana@protonmail.com>
> > 
> > Return a struct mtk_clk_rst_data * when registering a reset
> > controller in preparation for adding an unregister helper
> > that will take it as an argument. Make the necessary changes
> > in drivers that do not currently discard the return value
> > of register functions.
> > 
> > Signed-off-by: Yassine Oudjana <y.oudjana@protonmail.com>
> 
> Hello Yassine,
> 
> Thanks for your efforts on helping to make the MediaTek clocks better - I agree
> (and I'm not the only one..) that there's a lot of work to do on this side.
> 
> Though... I don't think that this is the right direction: you're right about
> properly unregistering (in patch 4/6) the reset controllers on rmmod/failure
> but I'm not sure that this kind of noise brings any benefit.
> 
> Explaining:
> You definitely saw that there's a new register _with_dev, which uses devm ops
> and that's going to automatically cleanup in case of removal/failure.
> This is what we should do.
> 
> Hence, my proposal is to drop patch 3/6, 4/6, 5/6 and (slowly, steadily) migrate
> all of the MediaTek clocks from CLK_OF_DECLARE() to platform drivers (which also
> means that we can eventually change them to tristate!), so that we slowly remove
> all users of all functions that are not "_with_dev", and that we finally remove
> all of these then-unused functions as well.
> 
> Making sure that I don't get misunderstood:
>       I'm not implying that this huge migration work is on your shoulders!
> 
> P.S.: Chen-Yu, Miles: do you also agree? :-)

Agreed. 3/6, 4/6, 5/6 introduce mtk_simple_clk_controller, but we just got
ChenYu's patch merged to clk-next to switch to clk_hw_onecell_data, and
hoplefully we can test them in v5.18 or v5.19.
So I think we should wait for ChenYu's cleanup.

-	struct clk_hw_onecell_data *clk_data;
+	struct mtk_simple_clk_controller *clk_ctrl;

thanks,
Miles

> 
> Cheers,
> Angelo
> 
>
Yassine Oudjana May 20, 2022, 9:41 a.m. UTC | #5
On Fri, May 20 2022 at 10:42:40 +0200, AngeloGioacchino Del Regno 
<angelogioacchino.delregno@collabora.com> wrote:
> Il 19/05/22 15:47, Yassine Oudjana ha scritto:
>> From: Yassine Oudjana <y.oudjana@protonmail.com>
>> 
>> Return a struct mtk_clk_rst_data * when registering a reset
>> controller in preparation for adding an unregister helper
>> that will take it as an argument. Make the necessary changes
>> in drivers that do not currently discard the return value
>> of register functions.
>> 
>> Signed-off-by: Yassine Oudjana <y.oudjana@protonmail.com>
> 
> Hello Yassine,
> 
> Thanks for your efforts on helping to make the MediaTek clocks better 
> - I agree
> (and I'm not the only one..) that there's a lot of work to do on this 
> side.
> 
> Though... I don't think that this is the right direction: you're 
> right about
> properly unregistering (in patch 4/6) the reset controllers on 
> rmmod/failure
> but I'm not sure that this kind of noise brings any benefit.
> 
> Explaining:
> You definitely saw that there's a new register _with_dev, which uses 
> devm ops
> and that's going to automatically cleanup in case of removal/failure.
> This is what we should do.
> 
> Hence, my proposal is to drop patch 3/6, 4/6, 5/6 and (slowly, 
> steadily) migrate
> all of the MediaTek clocks from CLK_OF_DECLARE() to platform drivers 
> (which also
> means that we can eventually change them to tristate!), so that we 
> slowly remove
> all users of all functions that are not "_with_dev", and that we 
> finally remove
> all of these then-unused functions as well.

I've tried to make small (but hopefully not too small) steps with
little improvements. Originally in MT6735 clock drivers v1, I only
added reset controller unregister, and while rebasing on Rex-BC's
reset cleanup series I found mtk_clk_simple_probe/remove while
looking for references to mtk_register_reset_controller, so
I thought of using it for my drivers resulting in this series
adding support for the extra 4 clock types. I started finding
other things that could be improved such as the other clock types
not having register_*_with_dev(), but I had to avoid adding
anything else since that would only make me find more things to
improve and this series would've never been finished and sent.

With that said, if these patches could become an obstacle for
later more complete reworks, then by all means drop them.

> 
> Making sure that I don't get misunderstood:
>      I'm not implying that this huge migration work is on your 
> shoulders!
> 

Of course. I would never be able to handle such a large task.
Everyone currently helping with modernizing this common clock
framework has my full respect. You are doing amazing work.

Thanks,
Yassine
diff mbox series

Patch

diff --git a/drivers/clk/mediatek/clk-mt8192.c b/drivers/clk/mediatek/clk-mt8192.c
index ebbd2798d9a3..a658a74644de 100644
--- a/drivers/clk/mediatek/clk-mt8192.c
+++ b/drivers/clk/mediatek/clk-mt8192.c
@@ -1255,6 +1255,7 @@  static int clk_mt8192_infra_probe(struct platform_device *pdev)
 {
 	struct clk_hw_onecell_data *clk_data;
 	struct device_node *node = pdev->dev.of_node;
+	struct mtk_clk_rst_data *rst_data;
 	int r;
 
 	clk_data = mtk_alloc_clk_data(CLK_INFRA_NR_CLK);
@@ -1265,9 +1266,11 @@  static int clk_mt8192_infra_probe(struct platform_device *pdev)
 	if (r)
 		goto free_clk_data;
 
-	r = mtk_register_reset_controller_with_dev(&pdev->dev, &clk_rst_desc);
-	if (r)
+	rst_data = mtk_register_reset_controller_with_dev(&pdev->dev, &clk_rst_desc);
+	if (IS_ERR(rst_data)) {
+		r = PTR_ERR(rst_data);
 		goto free_clk_data;
+	}
 
 	r = of_clk_add_hw_provider(node, of_clk_hw_onecell_get, clk_data);
 	if (r)
diff --git a/drivers/clk/mediatek/clk-mtk.c b/drivers/clk/mediatek/clk-mtk.c
index 3a8875b6c37f..1b5591733e2b 100644
--- a/drivers/clk/mediatek/clk-mtk.c
+++ b/drivers/clk/mediatek/clk-mtk.c
@@ -424,6 +424,7 @@  int mtk_clk_simple_probe(struct platform_device *pdev)
 	const struct mtk_clk_desc *mcd;
 	struct clk_hw_onecell_data *clk_data;
 	struct device_node *node = pdev->dev.of_node;
+	struct mtk_clk_rst_data *rst_data;
 	int r;
 
 	mcd = of_device_get_match_data(&pdev->dev);
@@ -446,10 +447,12 @@  int mtk_clk_simple_probe(struct platform_device *pdev)
 	platform_set_drvdata(pdev, clk_data);
 
 	if (mcd->rst_desc) {
-		r = mtk_register_reset_controller_with_dev(&pdev->dev,
-							   mcd->rst_desc);
-		if (r)
+		rst_data = mtk_register_reset_controller_with_dev(&pdev->dev,
+							   	  mcd->rst_desc);
+		if (IS_ERR(rst_data)) {
+			r = PTR_ERR(rst_data);
 			goto unregister_clks;
+		}
 	}
 
 	return r;
diff --git a/drivers/clk/mediatek/reset.c b/drivers/clk/mediatek/reset.c
index 290ceda84ce4..09862baf1d57 100644
--- a/drivers/clk/mediatek/reset.c
+++ b/drivers/clk/mediatek/reset.c
@@ -110,8 +110,9 @@  static int reset_xlate(struct reset_controller_dev *rcdev,
 	return data->desc->rst_idx_map[reset_spec->args[0]];
 }
 
-int mtk_register_reset_controller(struct device_node *np,
-				  const struct mtk_clk_rst_desc *desc)
+struct mtk_clk_rst_data
+*mtk_register_reset_controller(struct device_node *np,
+			       const struct mtk_clk_rst_desc *desc)
 {
 	struct regmap *regmap;
 	const struct reset_control_ops *rcops = NULL;
@@ -120,7 +121,7 @@  int mtk_register_reset_controller(struct device_node *np,
 
 	if (!desc) {
 		pr_err("mtk clock reset desc is NULL\n");
-		return -EINVAL;
+		return ERR_PTR(-EINVAL);
 	}
 
 	switch (desc->version) {
@@ -132,18 +133,18 @@  int mtk_register_reset_controller(struct device_node *np,
 		break;
 	default:
 		pr_err("Unknown reset version %d\n", desc->version);
-		return -EINVAL;
+		return ERR_PTR(-EINVAL);
 	}
 
 	regmap = device_node_to_regmap(np);
 	if (IS_ERR(regmap)) {
 		pr_err("Cannot find regmap for %pOF: %pe\n", np, regmap);
-		return -EINVAL;
+		return ERR_PTR(-EINVAL);
 	}
 
 	data = kzalloc(sizeof(*data), GFP_KERNEL);
 	if (!data)
-		return -ENOMEM;
+		return ERR_PTR(-ENOMEM);
 
 	data->desc = desc;
 	data->regmap = regmap;
@@ -163,14 +164,15 @@  int mtk_register_reset_controller(struct device_node *np,
 	if (ret) {
 		pr_err("could not register reset controller: %d\n", ret);
 		kfree(data);
-		return ret;
+		return ERR_PTR(ret);
 	}
 
-	return 0;
+	return data;
 }
 
-int mtk_register_reset_controller_with_dev(struct device *dev,
-					   const struct mtk_clk_rst_desc *desc)
+struct mtk_clk_rst_data
+*mtk_register_reset_controller_with_dev(struct device *dev,
+					const struct mtk_clk_rst_desc *desc)
 {
 	struct device_node *np = dev->of_node;
 	struct regmap *regmap;
@@ -180,7 +182,7 @@  int mtk_register_reset_controller_with_dev(struct device *dev,
 
 	if (!desc) {
 		dev_err(dev, "mtk clock reset desc is NULL\n");
-		return -EINVAL;
+		return ERR_PTR(-EINVAL);
 	}
 
 	switch (desc->version) {
@@ -192,18 +194,18 @@  int mtk_register_reset_controller_with_dev(struct device *dev,
 		break;
 	default:
 		dev_err(dev, "Unknown reset version %d\n", desc->version);
-		return -EINVAL;
+		return ERR_PTR(-EINVAL);
 	}
 
 	regmap = device_node_to_regmap(np);
 	if (IS_ERR(regmap)) {
 		dev_err(dev, "Cannot find regmap %pe\n", regmap);
-		return -EINVAL;
+		return ERR_PTR(-EINVAL);
 	}
 
 	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
 	if (!data)
-		return -ENOMEM;
+		return ERR_PTR(-ENOMEM);
 
 	data->desc = desc;
 	data->regmap = regmap;
@@ -223,10 +225,10 @@  int mtk_register_reset_controller_with_dev(struct device *dev,
 	ret = devm_reset_controller_register(dev, &data->rcdev);
 	if (ret) {
 		dev_err(dev, "could not register reset controller: %d\n", ret);
-		return ret;
+		return ERR_PTR(ret);
 	}
 
-	return 0;
+	return data;
 }
 EXPORT_SYMBOL_GPL(mtk_register_reset_controller_with_dev);
 
diff --git a/drivers/clk/mediatek/reset.h b/drivers/clk/mediatek/reset.h
index 913fe676cba7..7418dd0d046f 100644
--- a/drivers/clk/mediatek/reset.h
+++ b/drivers/clk/mediatek/reset.h
@@ -64,19 +64,21 @@  struct mtk_clk_rst_data {
  * @np: Pointer to device node.
  * @desc: Constant pointer to description of clock reset.
  *
- * Return: 0 on success and errorno otherwise.
+ * Return: Pointer to struct mtk_clk_rst_data on success and error pointer otherwise.
  */
-int mtk_register_reset_controller(struct device_node *np,
-				  const struct mtk_clk_rst_desc *desc);
+struct mtk_clk_rst_data
+*mtk_register_reset_controller(struct device_node *np,
+			       const struct mtk_clk_rst_desc *desc);
 
 /**
  * mtk_register_reset_controller - Register mediatek clock reset controller with device
  * @np: Pointer to device.
  * @desc: Constant pointer to description of clock reset.
  *
- * Return: 0 on success and errorno otherwise.
+ * Return: Pointer to struct mtk_clk_rst_data on success and error pointer otherwise.
  */
-int mtk_register_reset_controller_with_dev(struct device *dev,
-					   const struct mtk_clk_rst_desc *desc);
+struct mtk_clk_rst_data
+*mtk_register_reset_controller_with_dev(struct device *dev,
+					const struct mtk_clk_rst_desc *desc);
 
 #endif /* __DRV_CLK_MTK_RESET_H */