Re: [PATCH v3 2/5] OPP: Add function to look up required OPP's for a given OPP

[Viresh Kumar]

On 24-07-19, 20:46, Saravana Kannan wrote:
> On Wed, Jul 24, 2019 at 7:58 PM Viresh Kumar <viresh.kumar@xxxxxxxxxx> 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.

-- 
viresh