Messages in this thread | | | From | Saravana Kannan <> | Date | Thu, 25 Jul 2019 18:41:44 -0700 | Subject | Re: [PATCH v3 2/5] OPP: Add function to look up required OPP's for a given OPP |
| |
On Wed, Jul 24, 2019 at 10:38 PM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > On 24-07-19, 20:46, Saravana Kannan wrote: > > On Wed, Jul 24, 2019 at 7:58 PM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > > On 23-07-19, 17:23, Saravana Kannan wrote: > > > > > I almost said "not sure. Let me just compare pointers". > > > > I think (not sure) it has to do with the same OPP table being used to > > > > create multiple OPP table copies if the "shared OPP table" flag isn't > > > > set? > > > > Can you confirm if this makes sense? If so, I can add a comment patch > > > > that adds comments to the existing code and then copies it into this > > > > function in this patch. > > > > > > Right, that was the reason but we also need to fix ... > > > > I know I gave that explanation but I'm still a bit confused by the > > existing logic. If the same DT OPP table is used to create multiple in > > memory OPP tables, how do you device which in memory OPP table is the > > right one to point to? > > This is a bit broken actually, we don't see any problems right now but > may eventually have to fix it someday. > > We pick the first in-memory OPP table that was created using the DT > OPP table. This is done because the DT doesn't provide any explicit > linking to the required-opp device right now. > > Right now the required-opps is only used for power domains and so it > is working fine. It may work fine for your case as well. But once we > have a case we want to use required-opps in a single OPP table for > both power-domains and master/slave thing you are proposing, we may > see more problems. > > > > > > > + break; > > > > > > + } > > > > > > + > > > > > > + if (unlikely(i == src_table->required_opp_count)) { > > > > > > + pr_err("%s: Couldn't find matching OPP table (%p: %p)\n", > > > > > > + __func__, src_table, dst_table); > > > > > > + return NULL; > > > > > > + } > > > > > > + > > > > > > + mutex_lock(&src_table->lock); > > > > > > + > > > > > > + list_for_each_entry(opp, &src_table->opp_list, node) { > > > > > > + if (opp == src_opp) { > > > > > > ... this as well. We must be comparing node pointers here as well. > > > > Not really, if an in memory OPP entry is not part of an in memory OPP > > table list, I don't think it should be considered part of the OPP > > table just because the node pointer is the same. I think that's > > explicitly wrong and the above code is correct as is. > > I understand what you are saying, but because we match the very first > OPP table that was there in the list we need to match the DT node here > as well. > > Or somehow we make sure to have the correct in-memory OPP table being > pointed by the required-opp-table array. Then we don't need the node > pointer anywhere here.
Ah, right. I'll fix this.
-Saravana
| |