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

["<Vishal Badole>" <badolevishal1116@gmail.com>]

On Thu, Jun 23, 2022 at 06:05:48PM -0700, Stephen Boyd wrote:
> Quoting <Vishal Badole> (2022-06-22 10:02:20)
> > 
> > From f2e9d78bd0f135206fbfbf2e0178a5782b972939 Mon Sep 17 00:00:00 2001
> > From: Vishal Badole <badolevishal1116@gmail.com>
> > Date: Tue, 21 Jun 2022 09:55:51 +0530
> > Subject: [PATCH] Common clock: To list active consumers of clocks
> 
> That patch is still malformed. Please try again. Also, stop sending it
> as a reply-to the previous patch. Thanks!
>
We have applied and checked the patch on top of the mainline and not
able to see that it is malformed. We will share revised patch using git
send mail.
> > 
> > This feature lists the name of clocks and their consumer devices.
> > Using this feature user can easily check which device is using a
> > perticular clock. Consumers without dev_id are listed as no_dev_id.
> > 
> > Co-developed-by: Chinmoy Ghosh <chinmoyghosh2001@gmail.com>
> > Signed-off-by: Chinmoy Ghosh <chinmoyghosh2001@gmail.com>
> > Co-developed-by: Mintu Patel <mintupatel89@gmail.com>
> > Signed-off-by: Mintu Patel <mintupatel89@gmail.com>
> > Acked-by: Vimal Kumar <vimal.kumar32@gmail.com>
> 
> The acked-by tag is largely for maintainer use. Please remove it. See
> Documentation/process/5.Posting.rst for more info.
> 
Agreed, We will update this in the revised patch.

> > Signed-off-by: Vishal Badole <badolevishal1116@gmail.com>
> > ---
> >  drivers/clk/clk.c | 59 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 59 insertions(+)
> > 
> > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > index ed11918..b191009 100644
> > --- a/drivers/clk/clk.c
> > +++ b/drivers/clk/clk.c
> > @@ -3018,6 +3018,63 @@ static int clk_summary_show(struct seq_file *s, void *data)
> >  }
> >  DEFINE_SHOW_ATTRIBUTE(clk_summary);
> >  
> > +static void clk_consumer_show_one(struct seq_file *s, struct clk_core *core, int level)
> > +{
> > +       struct clk *clk_user;
> > +       const char *consumer;
> > +
> > +       hlist_for_each_entry(clk_user, &core->clks, clks_node) {
> > +               if (!clk_user->dev_id)
> > +                       consumer = "no_dev_id";
> > +               else
> > +                       consumer = clk_user->dev_id;
> 
> We can just pass NULL if there isn't a dev_id and get a nice "(NULL)"
> print instead of "no_dev_id".
> 
Agreed, we will replace "no_dev_id" with "deviceless" in the revised
patch.
> > +
> > +               seq_printf(s, "%*s%-*s %30s %5d %7d ",
> > +                          level * 3 + 1, "",
> > +                          30 - level * 3, clk_user->core->name, consumer,
> > +                          clk_user->core->enable_count, clk_user->core->prepare_count);
> 
> It would be great to not print the core enable count here and instead
> have two levels of enable accounting so we can print the per-user count.
> Basically, one in the 'struct clk_core' and one in the 'struct clk'. If
> that isn't done then this print is going to duplicate the count for
> every 'struct clk' and be meaningless.
>
We will add enable count member to struct clock to represent per user
count and will print that one along with clock and consumer name
> > +
> > +               if (clk_user->core->ops->is_enabled)
> > +                       seq_printf(s, " %8c\n", clk_core_is_enabled(clk_user->core) ? 'Y' : 'N');
> > +               else if (!clk_user->core->ops->enable)
> > +                       seq_printf(s, " %8c\n", 'Y');
> > +               else
> > +                       seq_printf(s, " %8c\n", '?');
> 
> I don't think we need any of these prints. They're already covered in
> the summary. And the summary should be able to print the users. See
> regulator_summary_show_subtree() for inspiration. It looks like they
> print "deviceless" for the "no_dev_id" case so maybe just use that
> instead of NULL print.
> 
We will remove above prints in the revised patch. We are facing indentation issue whle printing consumer in summary
as given below
                                 enable  prepare  protect                            duty  hardware            per-user
  clock                          count    count    count        rateccuracy phase  cycle    enable  consumer   count
  clk_mcasp0_fixed                   0        0        0           24576000     0  50000         Y   
  deviceless        0

In this case it will be better to have a separate debugfs entry as
clK_consumer to print clock, consumer and per-user count.
> > +       }
> > +}
> > +
> > +static void clk_consumer_show_subtree(struct seq_file *s, struct clk_core *c, int level)
> > +{
> > +       struct clk_core *child;
> > +
> > +       clk_consumer_show_one(s, c, level);
> > +
> > +       hlist_for_each_entry(child, &c->children, child_node)
> > +               clk_consumer_show_subtree(s, child, level + 1);
> > +}
> > +
> > +static int clk_consumer_show(struct seq_file *s, void *data)
> > +{
> > +       struct clk_core *c;
> > +       struct hlist_head **lists = (struct hlist_head **)s->private;
> > +
> > +       seq_puts(s, "                                                              enable   prepare   hardware\n");
> > +       seq_puts(s, "   clock                                       consumer        count     count     enable\n");
> > +       seq_puts(s, "-----------------------------------------------------------------------------------------\n");
> > +       clk_prepare_lock();
> > +
> > +       /*Traversing Linked List to print clock consumer*/
> 
> Please run scripts/checkpatch.pl, as this comment needs space after /*
> and before */
> 
We will update this in revised patch.
> > +
> > +       for (; *lists; lists++)
> > +               hlist_for_each_entry(c, *lists, child_node)
> > +                       clk_consumer_show_subtree(s, c, 0);
> > +
> > +       clk_prepare_unlock();
> > +
> > +       return 0;
> > +}
> > +DEFINE_SHOW_ATTRIBUTE(clk_consumer);
> > +
> >  static void clk_dump_one(struct seq_file *s, struct clk_core *c, int level)
> >  {
> >         int phase;