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

From: Rex-BC Chen
Date: Fri May 20 2022 - 01:57:04 EST

On Thu, 2022-05-19 at 17:47 +0400, Yassine Oudjana wrote:
> From: Yassine Oudjana <y.oudjana@xxxxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxx/__;!!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@xxxxxxxxxxxx/__;!!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@xxxxxxxxxxxxx/__;!!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@xxxxxxxxxxxx/

BRs,
Rex