[v3,08/18] phy: ti: phy-j721e-wiz: split wiz_clock_init() function
Commit Message
The wiz_clock_init() function mixes probe and hardware configuration.
Rename the wiz_clock_init() to wiz_clock_probe() and move the hardware
configuration part in a new function named wiz_clock_init().
This hardware configuration sequence must be called during the resume
stage of the driver.
Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
---
drivers/phy/ti/phy-j721e-wiz.c | 65 ++++++++++++++++++++++++------------------
1 file changed, 37 insertions(+), 28 deletions(-)
Comments
On Thu, Feb 15, 2024 at 04:17:53PM +0100, Thomas Richard wrote:
> The wiz_clock_init() function mixes probe and hardware configuration.
> Rename the wiz_clock_init() to wiz_clock_probe() and move the hardware
> configuration part in a new function named wiz_clock_init().
>
> This hardware configuration sequence must be called during the resume
> stage of the driver.
..
(Side note, as this can be done later)
> if (rate >= 100000000)
> + if (rate >= 100000000)
> + if (rate >= 100000000)
I would make local definition and use it, we may get the global one as there
are users.
#define HZ_PER_GHZ 1000000000UL
On 15-02-24, 17:43, Andy Shevchenko wrote:
> On Thu, Feb 15, 2024 at 04:17:53PM +0100, Thomas Richard wrote:
> > The wiz_clock_init() function mixes probe and hardware configuration.
> > Rename the wiz_clock_init() to wiz_clock_probe() and move the hardware
> > configuration part in a new function named wiz_clock_init().
> >
> > This hardware configuration sequence must be called during the resume
> > stage of the driver.
>
> ...
>
> (Side note, as this can be done later)
>
> > if (rate >= 100000000)
>
> > + if (rate >= 100000000)
>
> > + if (rate >= 100000000)
>
> I would make local definition and use it, we may get the global one as there
> are users.
>
> #define HZ_PER_GHZ 1000000000UL
Better to define as:
#define HZ_PER_GHZ 1 * GIGA
On 24/02/16 11:32AM, Vinod Koul wrote:
> On 15-02-24, 17:43, Andy Shevchenko wrote:
> > On Thu, Feb 15, 2024 at 04:17:53PM +0100, Thomas Richard wrote:
> > > The wiz_clock_init() function mixes probe and hardware configuration.
> > > Rename the wiz_clock_init() to wiz_clock_probe() and move the hardware
> > > configuration part in a new function named wiz_clock_init().
> > >
> > > This hardware configuration sequence must be called during the resume
> > > stage of the driver.
> >
> > ...
> >
> > (Side note, as this can be done later)
> >
> > > if (rate >= 100000000)
> >
> > > + if (rate >= 100000000)
> >
> > > + if (rate >= 100000000)
> >
> > I would make local definition and use it, we may get the global one as there
> > are users.
> >
> > #define HZ_PER_GHZ 1000000000UL
>
> Better to define as:
> #define HZ_PER_GHZ 1 * GIGA
The variable "rate" is being compared against 100 MHz and not 1 GHz.
The driver already has the following macros defined:
#define REF_CLK_19_2MHZ 19200000
#define REF_CLK_25MHZ 25000000
#define REF_CLK_100MHZ 100000000
#define REF_CLK_156_25MHZ 156250000
So would it be acceptable to change it to:
if (rate >= REF_CLK_100MHZ)
instead?
Regards,
Siddharth.
On Fri, Feb 16, 2024 at 11:32:31AM +0530, Vinod Koul wrote:
> On 15-02-24, 17:43, Andy Shevchenko wrote:
> > On Thu, Feb 15, 2024 at 04:17:53PM +0100, Thomas Richard wrote:
..
> > (Side note, as this can be done later)
> >
> > > if (rate >= 100000000)
> >
> > > + if (rate >= 100000000)
> >
> > > + if (rate >= 100000000)
> >
> > I would make local definition and use it, we may get the global one as there
> > are users.
> >
> > #define HZ_PER_GHZ 1000000000UL
>
> Better to define as:
> #define HZ_PER_GHZ 1 * GIGA
(with parentheses)
Maybe here, but when it appears in units.h it will be defined as I wrote
to be aligned with the rest of definitions.
On Fri, Feb 16, 2024 at 02:34:39PM +0530, Siddharth Vadapalli wrote:
> On 24/02/16 11:32AM, Vinod Koul wrote:
> > On 15-02-24, 17:43, Andy Shevchenko wrote:
> > > On Thu, Feb 15, 2024 at 04:17:53PM +0100, Thomas Richard wrote:
..
> > > (Side note, as this can be done later)
> > >
> > > > if (rate >= 100000000)
> > >
> > > > + if (rate >= 100000000)
> > >
> > > > + if (rate >= 100000000)
> > >
> > > I would make local definition and use it, we may get the global one as there
> > > are users.
> > >
> > > #define HZ_PER_GHZ 1000000000UL
> >
> > Better to define as:
> > #define HZ_PER_GHZ 1 * GIGA
>
> The variable "rate" is being compared against 100 MHz and not 1 GHz.
Extremely good point why constant definitions are better (to avoid missing
or extra 0, etc)!
> The driver already has the following macros defined:
> #define REF_CLK_19_2MHZ 19200000
> #define REF_CLK_25MHZ 25000000
> #define REF_CLK_100MHZ 100000000
> #define REF_CLK_156_25MHZ 156250000
>
> So would it be acceptable to change it to:
> if (rate >= REF_CLK_100MHZ)
> instead?
Sounds like a good idea to me.
@@ -1076,25 +1076,11 @@ static int wiz_clock_register(struct wiz *wiz)
return ret;
}
-static int wiz_clock_init(struct wiz *wiz, struct device_node *node)
+static void wiz_clock_init(struct wiz *wiz)
{
- const struct wiz_clk_mux_sel *clk_mux_sel = wiz->clk_mux_sel;
- struct device *dev = wiz->dev;
- struct device_node *clk_node;
- const char *node_name;
unsigned long rate;
- struct clk *clk;
- int ret;
- int i;
-
- clk = devm_clk_get(dev, "core_ref_clk");
- if (IS_ERR(clk))
- return dev_err_probe(dev, PTR_ERR(clk),
- "core_ref_clk clock not found\n");
-
- wiz->input_clks[WIZ_CORE_REFCLK] = clk;
- rate = clk_get_rate(clk);
+ rate = clk_get_rate(wiz->input_clks[WIZ_CORE_REFCLK]);
if (rate >= 100000000)
regmap_field_write(wiz->pma_cmn_refclk_int_mode, 0x1);
else
@@ -1119,6 +1105,39 @@ static int wiz_clock_init(struct wiz *wiz, struct device_node *node)
break;
}
+ if (wiz->input_clks[WIZ_CORE_REFCLK1]) {
+ rate = clk_get_rate(wiz->input_clks[WIZ_CORE_REFCLK1]);
+ if (rate >= 100000000)
+ regmap_field_write(wiz->pma_cmn_refclk1_int_mode, 0x1);
+ else
+ regmap_field_write(wiz->pma_cmn_refclk1_int_mode, 0x3);
+
+ }
+
+ rate = clk_get_rate(wiz->input_clks[WIZ_EXT_REFCLK]);
+ if (rate >= 100000000)
+ regmap_field_write(wiz->pma_cmn_refclk_mode, 0x0);
+ else
+ regmap_field_write(wiz->pma_cmn_refclk_mode, 0x2);
+}
+
+static int wiz_clock_probe(struct wiz *wiz, struct device_node *node)
+{
+ const struct wiz_clk_mux_sel *clk_mux_sel = wiz->clk_mux_sel;
+ struct device *dev = wiz->dev;
+ struct device_node *clk_node;
+ const char *node_name;
+ struct clk *clk;
+ int ret;
+ int i;
+
+ clk = devm_clk_get(dev, "core_ref_clk");
+ if (IS_ERR(clk))
+ return dev_err_probe(dev, PTR_ERR(clk),
+ "core_ref_clk clock not found\n");
+
+ 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))
@@ -1126,12 +1145,6 @@ static int wiz_clock_init(struct wiz *wiz, struct device_node *node)
"core_ref1_clk clock not found\n");
wiz->input_clks[WIZ_CORE_REFCLK1] = clk;
-
- rate = clk_get_rate(clk);
- if (rate >= 100000000)
- regmap_field_write(wiz->pma_cmn_refclk1_int_mode, 0x1);
- else
- regmap_field_write(wiz->pma_cmn_refclk1_int_mode, 0x3);
}
clk = devm_clk_get(dev, "ext_ref_clk");
@@ -1141,11 +1154,7 @@ static int wiz_clock_init(struct wiz *wiz, struct device_node *node)
wiz->input_clks[WIZ_EXT_REFCLK] = clk;
- 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);
+ wiz_clock_init(wiz);
switch (wiz->type) {
case AM64_WIZ_10G:
@@ -1589,7 +1598,7 @@ static int wiz_probe(struct platform_device *pdev)
goto err_get_sync;
}
- ret = wiz_clock_init(wiz, node);
+ ret = wiz_clock_probe(wiz, node);
if (ret < 0) {
dev_warn(dev, "Failed to initialize clocks\n");
goto err_get_sync;