Re: [PATCH v3] Common clock: To list active consumers of clocks

[Stephen Boyd <sboyd@kernel.org>]

Quoting Vishal Badole (2022-08-02 11:09:47)
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index f00d4c1..c96079f 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -102,6 +102,7 @@ struct clk {
>         unsigned long min_rate;
>         unsigned long max_rate;
>         unsigned int exclusive_count;
> +       unsigned int enable_count;
>         struct hlist_node clks_node;
>  };
>  
> @@ -1008,6 +1009,10 @@ void clk_disable(struct clk *clk)
>                 return;
>  
>         clk_core_disable_lock(clk->core);
> +
> +       if (clk->enable_count > 0)
> +               clk->enable_count--;
> +
>  }
>  EXPORT_SYMBOL_GPL(clk_disable);
>  
> @@ -1169,10 +1174,16 @@ EXPORT_SYMBOL_GPL(clk_restore_context);
>   */
>  int clk_enable(struct clk *clk)
>  {
> +       int ret;
> +
>         if (!clk)
>                 return 0;
>  
> -       return clk_core_enable_lock(clk->core);
> +       ret = clk_core_enable_lock(clk->core);
> +       if (!ret)
> +               clk->enable_count++;
> +
> +       return ret;
>  }
>  EXPORT_SYMBOL_GPL(clk_enable);

We'll want the above three hunks to be a different patch so we can
discuss the merits of tracking per user enable counts. Do you have a
usecase for this or is it "just for fun"? By adding a count we have more
code, and we waste more memory to track this stat. I really would rather
not bloat just because, so please elaborate on your use case.

>  
> @@ -2953,28 +2964,41 @@ static void clk_summary_show_one(struct seq_file *s, struct clk_core *c,
>                                  int level)
>  {
>         int phase;
> +       struct clk *clk_user;
> +       int multi_node = 0;
>  
> -       seq_printf(s, "%*s%-*s %7d %8d %8d %11lu %10lu ",
> +       seq_printf(s, "%*s%-*s %-7d %-8d %-8d %-11lu %-10lu ",
>                    level * 3 + 1, "",
> -                  30 - level * 3, c->name,
> +                  35 - level * 3, c->name,
>                    c->enable_count, c->prepare_count, c->protect_count,
>                    clk_core_get_rate_recalc(c),
>                    clk_core_get_accuracy_recalc(c));
>  
>         phase = clk_core_get_phase(c);
>         if (phase >= 0)
> -               seq_printf(s, "%5d", phase);
> +               seq_printf(s, "%-5d", phase);
>         else
>                 seq_puts(s, "-----");
>  
> -       seq_printf(s, " %6d", clk_core_get_scaled_duty_cycle(c, 100000));
> +       seq_printf(s, " %-6d", clk_core_get_scaled_duty_cycle(c, 100000));
>  
>         if (c->ops->is_enabled)
> -               seq_printf(s, " %9c\n", clk_core_is_enabled(c) ? 'Y' : 'N');
> +               seq_printf(s, " %5c ", clk_core_is_enabled(c) ? 'Y' : 'N');
>         else if (!c->ops->enable)
> -               seq_printf(s, " %9c\n", 'Y');
> +               seq_printf(s, " %5c ", 'Y');
>         else
> -               seq_printf(s, " %9c\n", '?');
> +               seq_printf(s, " %5c ", '?');
> +
> +       hlist_for_each_entry(clk_user, &c->clks, clks_node) {
> +               seq_printf(s, "%*s%-*s  %-4d\n",
> +                          level * 3 + 2 + 105 * multi_node, "",
> +                          30,
> +                          clk_user->dev_id ? clk_user->dev_id : "deviceless",
> +                          clk_user->enable_count);
> +
> +               multi_node = 1;

This part that prints the dev_id might be useful and can be the first
patch in the series. In that same patch, please print the con_id so we
know which clk it is for the device. We should also improve of_clk_get()
so that the index is visible to the 'struct clk::con_id' somehow. Maybe
we can convert the integer index into a string and assign that to con_id
in that case as well.