Re: [PATCH 2/7] clk: clocking-wizard: use newer clk_hw API

[Harry Austen]

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]);
> >         }