Re: [PATCH v10 1/6] interconnect: icc-clk: Allow user to specify master/slave ids

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 29/04/2024 10:13, Varadarajan Narayanan wrote:
  	for (i = 0, j = 0; i < num_clocks; i++) {
  		qp->clocks[i].clk = data[i].clk;
- node = icc_node_create(first_id + j);
+		node = icc_node_create(first_id + data[i].master_id);

You have a few conditionals in the way down the end of the existing for() loop but then you hit this

        onecell->nodes[j++] = node;
    }

which means that this

    node = icc_node_create(first_id + data[i].master_id);

is not analogous to this

    node = icc_node_create(first_id + j);

So for any loop of this for() where j was incremented previously you would not _not_ have the same node ids after your change.

In other words dropping the j index will result in different node numbering.

Is that

a) intended
b) correct

Your commit log says "allow the drive rto provide the preferred master ids and slave ids" which it does but it _also_ changes the autogenerated ids.

So could you either a) fix that or b) justify it, in your commit log.

Also I think the 8996 specific change should be in its own patch.

TBH I'm not sure the autogen change is on-purpose or warranted and for certain the commit log is not elucidating on which is the intended case.

I think you should rewrite this patch in two ways

1. Fix the autogen case or
1. Justify the change for the autogen case.
2. Separate drivers/clk/qcom/clk-cbf-8996.c into its own patch that
   applies directly after changing the core

Perhaps you've already gone through this debate with other reviewers but then you haven't captured that in your cover letter or commit log so at a minimum, please do that.

---
bod




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [Linux for Sparc]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux