On Wed Jul 24, 2024 at 12:14 AM BST, Stephen Boyd wrote:
> General comment: do one thing in one patch, i.e. use clk_hw API and
> don't also migrate to devm in one patch.
Fair point. Will split into two patches in v2.
>
> Quoting Harry Austen (2024-07-20 05:01:36)
> > diff --git a/drivers/clk/xilinx/clk-xlnx-clock-wizard.c b/drivers/clk/xilinx/clk-xlnx-clock-wizard.c
> > index 0ca045849ea3e..30c5cc9dcd7e9 100644
> > --- a/drivers/clk/xilinx/clk-xlnx-clock-wizard.c
> > +++ b/drivers/clk/xilinx/clk-xlnx-clock-wizard.c
> > @@ -17,6 +17,7 @@
> > #include <linux/of.h>
> > #include <linux/math64.h>
> > #include <linux/module.h>
> > +#include <linux/overflow.h>
>
> What is this include for? __counted_by()?
For struct_size()
>
> > #include <linux/err.h>
> > #include <linux/iopoll.h>
> >
> > @@ -121,24 +122,22 @@ enum clk_wzrd_int_clks {
> > /**
> > * struct clk_wzrd - Clock wizard private data structure
> > *
> > - * @clk_data: Clock data
> > + * @clk_data: Output clock data
> > * @nb: Notifier block
> > * @base: Memory base
> > * @clk_in1: Handle to input clock 'clk_in1'
> > * @axi_clk: Handle to input clock 's_axi_aclk'
> > * @clks_internal: Internal clocks
> > - * @clkout: Output clocks
> > * @speed_grade: Speed grade of the device
> > * @suspended: Flag indicating power state of the device
> > */
> > struct clk_wzrd {
> > - struct clk_onecell_data clk_data;
> > + struct clk_hw_onecell_data *clk_data;
>
> It could be placed at the end and then one allocation could be used
> instead of two.
Ah good point. Will move in v2.
>
> > struct notifier_block nb;
> > void __iomem *base;
> > struct clk *clk_in1;
> > struct clk *axi_clk;
> > - struct clk *clks_internal[wzrd_clk_int_max];
> > - struct clk *clkout[WZRD_NUM_OUTPUTS];
> > + struct clk_hw *clks_internal[wzrd_clk_int_max];
> > unsigned int speed_grade;
> > bool suspended;
> > };
> > @@ -1108,35 +1110,32 @@ static int clk_wzrd_probe(struct platform_device *pdev)
> > if (!div)
> > div = 1;
> >
> > - clk_mul_name = __clk_get_name(clk_wzrd->clks_internal[wzrd_clk_mul]);
> > + clk_mul_name = clk_hw_get_name(clk_wzrd->clks_internal[wzrd_clk_mul]);
> > clk_wzrd->clks_internal[wzrd_clk_mul_div] =
> > - clk_register_fixed_factor(&pdev->dev, clk_name,
> > - clk_mul_name, 0, 1, div);
> > + clk_hw_register_fixed_factor(&pdev->dev, clk_name,
> > + clk_mul_name, 0, 1, div);
> > } else {
> > ctrl_reg = clk_wzrd->base + WZRD_CLK_CFG_REG(is_versal, 0);
> > - clk_wzrd->clks_internal[wzrd_clk_mul_div] = clk_register_divider
> > + clk_wzrd->clks_internal[wzrd_clk_mul_div] = clk_hw_register_divider
>
> Are these going to be using devm so that on failure they get
> unregistered?
I appear to have missed that entirely. Yes, that was the intention. Will fix in v2.
>
> > (&pdev->dev, clk_name,
> > - __clk_get_name(clk_wzrd->clks_internal[wzrd_clk_mul]),
> > + clk_hw_get_name(clk_wzrd->clks_internal[wzrd_clk_mul]),
> > flags, ctrl_reg, 0, 8, CLK_DIVIDER_ONE_BASED |
> > CLK_DIVIDER_ALLOW_ZERO, &clkwzrd_lock);
> > }
> > if (IS_ERR(clk_wzrd->clks_internal[wzrd_clk_mul_div])) {
> > dev_err(&pdev->dev, "unable to register divider clock\n");
> > - ret = PTR_ERR(clk_wzrd->clks_internal[wzrd_clk_mul_div]);
> > - goto err_rm_int_clk;
> > + return PTR_ERR(clk_wzrd->clks_internal[wzrd_clk_mul_div]);
> > }
