[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <grxi7y5l52fyyd7w5kmgvdzgbld2g2tjl4keudmykbuaxk6abk@ramj3rfa5lai>
Date: Tue, 30 Jan 2024 00:02:51 +0100
From: Andi Shyti <andi.shyti@...nel.org>
To: Thomas Richard <thomas.richard@...tlin.com>
Cc: Linus Walleij <linus.walleij@...aro.org>,
Bartosz Golaszewski <brgl@...ev.pl>, Andy Shevchenko <andy@...nel.org>,
Tony Lindgren <tony@...mide.com>, Haojian Zhuang <haojian.zhuang@...aro.org>,
Vignesh R <vigneshr@...com>, Aaro Koskinen <aaro.koskinen@....fi>,
Janusz Krzysztofik <jmkrzyszt@...il.com>, Peter Rosin <peda@...ntia.se>, Vinod Koul <vkoul@...nel.org>,
Kishon Vijay Abraham I <kishon@...nel.org>, Philipp Zabel <p.zabel@...gutronix.de>,
Tom Joseph <tjoseph@...ence.com>, Lorenzo Pieralisi <lpieralisi@...nel.org>,
Krzysztof Wilczyński <kw@...ux.com>, Rob Herring <robh@...nel.org>,
Bjorn Helgaas <bhelgaas@...gle.com>, linux-gpio@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org, linux-omap@...r.kernel.org, linux-i2c@...r.kernel.org,
linux-phy@...ts.infradead.org, linux-pci@...r.kernel.org, gregory.clement@...tlin.com,
theo.lebrun@...tlin.com, thomas.petazzoni@...tlin.com, u-kumar1@...com
Subject: Re: [PATCH v2 06/15] phy: ti: phy-j721e-wiz: make wiz_clock_init
callable multiple times
Hi Thomas,
On Fri, Jan 26, 2024 at 03:36:48PM +0100, Thomas Richard wrote:
> For suspend and resume support, wiz_clock_init needs to be called multiple
> times.
>
> Add a parameter to wiz_clock_init to be able to skip clocks registration.
>
> Based on the work of Théo Lebrun <theo.lebrun@...tlin.com>
>
> Signed-off-by: Thomas Richard <thomas.richard@...tlin.com>
> ---
> drivers/phy/ti/phy-j721e-wiz.c | 60 +++++++++++++++++++++++++-----------------
> 1 file changed, 36 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/phy/ti/phy-j721e-wiz.c b/drivers/phy/ti/phy-j721e-wiz.c
> index fc3cd98c60ff..09f7edf16562 100644
> --- a/drivers/phy/ti/phy-j721e-wiz.c
> +++ b/drivers/phy/ti/phy-j721e-wiz.c
> @@ -1076,7 +1076,7 @@ static int wiz_clock_register(struct wiz *wiz)
> return ret;
> }
>
> -static int wiz_clock_init(struct wiz *wiz, struct device_node *node)
> +static int wiz_clock_init(struct wiz *wiz, struct device_node *node, bool probe)
the 'bool probe' is ugly. When someone reads something like
"ret = wiz_clock_init(wiz, node, true);" wonders what the
true/false is and needs to follow through the function to
understand.
Please, split this function (and below) into two parts:
wiz_clock_probe() and wiz_clock_init(). The former calls the
latter. Later on you can just call wiz_clock_init().
Andi
> {
> const struct wiz_clk_mux_sel *clk_mux_sel = wiz->clk_mux_sel;
> struct device *dev = wiz->dev;
> @@ -1087,14 +1087,36 @@ static int wiz_clock_init(struct wiz *wiz, struct device_node *node)
> int ret;
> int i;
>
> - clk = devm_clk_get(dev, "core_ref_clk");
> - if (IS_ERR(clk)) {
> - dev_err(dev, "core_ref_clk clock not found\n");
> - ret = PTR_ERR(clk);
> - return ret;
> + if (probe) {
> + clk = devm_clk_get(dev, "core_ref_clk");
> + if (IS_ERR(clk)) {
> + dev_err(dev, "core_ref_clk clock not found\n");
> + ret = PTR_ERR(clk);
> + return ret;
> + }
> + wiz->input_clks[WIZ_CORE_REFCLK] = clk;
> +
> + if (wiz->data->pma_cmn_refclk1_int_mode) {
> + clk = devm_clk_get(dev, "core_ref1_clk");
> + if (IS_ERR(clk)) {
> + dev_err(dev, "core_ref1_clk clock not found\n");
> + ret = PTR_ERR(clk);
> + return ret;
> + }
> + wiz->input_clks[WIZ_CORE_REFCLK1] = clk;
> + }
> +
> + clk = devm_clk_get(dev, "ext_ref_clk");
> + if (IS_ERR(clk)) {
> + dev_err(dev, "ext_ref_clk clock not found\n");
> + ret = PTR_ERR(clk);
> + return ret;
> + }
> + wiz->input_clks[WIZ_EXT_REFCLK] = clk;
> }
> - wiz->input_clks[WIZ_CORE_REFCLK] = clk;
>
> +
> + clk = wiz->input_clks[WIZ_CORE_REFCLK];
> rate = clk_get_rate(clk);
> if (rate >= 100000000)
> regmap_field_write(wiz->pma_cmn_refclk_int_mode, 0x1);
> @@ -1121,14 +1143,7 @@ static int wiz_clock_init(struct wiz *wiz, struct device_node *node)
> }
>
> if (wiz->data->pma_cmn_refclk1_int_mode) {
> - clk = devm_clk_get(dev, "core_ref1_clk");
> - if (IS_ERR(clk)) {
> - dev_err(dev, "core_ref1_clk clock not found\n");
> - ret = PTR_ERR(clk);
> - return ret;
> - }
> - wiz->input_clks[WIZ_CORE_REFCLK1] = clk;
> -
> + clk = wiz->input_clks[WIZ_CORE_REFCLK1];
> rate = clk_get_rate(clk);
> if (rate >= 100000000)
> regmap_field_write(wiz->pma_cmn_refclk1_int_mode, 0x1);
> @@ -1136,20 +1151,17 @@ static int wiz_clock_init(struct wiz *wiz, struct device_node *node)
> regmap_field_write(wiz->pma_cmn_refclk1_int_mode, 0x3);
> }
>
> - clk = devm_clk_get(dev, "ext_ref_clk");
> - if (IS_ERR(clk)) {
> - dev_err(dev, "ext_ref_clk clock not found\n");
> - ret = PTR_ERR(clk);
> - return ret;
> - }
> - wiz->input_clks[WIZ_EXT_REFCLK] = clk;
> -
> + clk = wiz->input_clks[WIZ_EXT_REFCLK];
> rate = clk_get_rate(clk);
> if (rate >= 100000000)
> regmap_field_write(wiz->pma_cmn_refclk_mode, 0x0);
> else
> regmap_field_write(wiz->pma_cmn_refclk_mode, 0x2);
>
> + /* What follows is about registering clocks. */
> + if (!probe)
> + return 0;
> +
> switch (wiz->type) {
> case AM64_WIZ_10G:
> case J7200_WIZ_10G:
> @@ -1592,7 +1604,7 @@ static int wiz_probe(struct platform_device *pdev)
> goto err_get_sync;
> }
>
> - ret = wiz_clock_init(wiz, node);
> + ret = wiz_clock_init(wiz, node, true);
> if (ret < 0) {
> dev_warn(dev, "Failed to initialize clocks\n");
> goto err_get_sync;
>
> --
> 2.39.2
>
Powered by blists - more mailing lists