Linux-PM Archive on lore.kernel.org
 help / color / Atom feed
From: Saravana Kannan <saravanak@google.com>
To: Stephen Boyd <sboyd@kernel.org>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>,
	Mark Rutland <mark.rutland@arm.com>, Nishanth Menon <nm@ti.com>,
	Rob Herring <robh+dt@kernel.org>,
	Viresh Kumar <vireshk@kernel.org>,
	Georgi Djakov <georgi.djakov@linaro.org>,
	Vincent Guittot <vincent.guittot@linaro.org>,
	"Sweeney, Sean" <seansw@qti.qualcomm.com>,
	David Dai <daidavid1@codeaurora.org>,
	adharmap@codeaurora.org, Rajendra Nayak <rnayak@codeaurora.org>,
	Sibi Sankar <sibis@codeaurora.org>,
	Bjorn Andersson <bjorn.andersson@linaro.org>,
	Evan Green <evgreen@chromium.org>,
	Android Kernel Team <kernel-team@android.com>,
	Linux PM <linux-pm@vger.kernel.org>,
	"open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
	<devicetree@vger.kernel.org>, LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v5 2/3] OPP: Add support for bandwidth OPP tables
Date: Tue, 20 Aug 2019 15:34:07 -0700
Message-ID: <CAGETcx8h2v054eS6pZ3iEsM2AGfN45_fqJGsVt+1UhbrK_dbdQ@mail.gmail.com> (raw)
In-Reply-To: <20190816182131.A692E206C1@mail.kernel.org>

On Fri, Aug 16, 2019 at 11:21 AM Stephen Boyd <sboyd@kernel.org> wrote:
>
> Quoting Saravana Kannan (2019-08-07 15:31:10)
> > diff --git a/drivers/opp/of.c b/drivers/opp/of.c
> > index 1813f5ad5fa2..e1750033fef9 100644
> > --- a/drivers/opp/of.c
> > +++ b/drivers/opp/of.c
> > @@ -523,6 +523,35 @@ void dev_pm_opp_of_remove_table(struct device *dev)
> >  }
> >  EXPORT_SYMBOL_GPL(dev_pm_opp_of_remove_table);
> >
> > +static int _read_opp_key(struct dev_pm_opp *new_opp, struct device_node *np)
> > +{
> > +       int ret;
> > +       u64 rate;
> > +       u32 bw;
> > +
> > +       ret = of_property_read_u64(np, "opp-hz", &rate);
> > +       if (!ret) {
> > +               /*
> > +                * Rate is defined as an unsigned long in clk API, and so
> > +                * casting explicitly to its type. Must be fixed once rate is 64
> > +                * bit guaranteed in clk API.
> > +                */
> > +               new_opp->rate = (unsigned long)rate;
> > +               return 0;
> > +       }
> > +
> > +       ret = of_property_read_u32(np, "opp-peak-kBps", &bw);
> > +       if (ret)
> > +               return ret;
> > +       new_opp->rate = (unsigned long) bw;
> > +
> > +       ret = of_property_read_u32(np, "opp-avg-kBps", &bw);
> > +       if (!ret)
>
> I would write this as
>
>         if (!of_property_read_u32(np, "opp-avg-kBps", &bw))
>                 new_opp->avg_bw = (unsigned long) bw;
>
> because you don't care about the return value.

Sure, will do.

>
> > +               new_opp->avg_bw = (unsigned long) bw;
> > +
> > +       return 0;
> > +}
> > +
> >  /**
> >   * _opp_add_static_v2() - Allocate static OPPs (As per 'v2' DT bindings)
> >   * @opp_table: OPP table
> > diff --git a/drivers/opp/opp.h b/drivers/opp/opp.h
> > index 01a500e2c40a..6bb238af9cac 100644
> > --- a/drivers/opp/opp.h
> > +++ b/drivers/opp/opp.h
> > @@ -56,7 +56,8 @@ extern struct list_head opp_tables;
> >   * @turbo:     true if turbo (boost) OPP
> >   * @suspend:   true if suspend OPP
> >   * @pstate: Device's power domain's performance state.
> > - * @rate:      Frequency in hertz
> > + * @rate:      Frequency in hertz OR Peak bandwidth in kilobytes per second
>
> Why is Peak capitalized?

Because it's another Key? :)

Just kidding. I'll fix it.

> > + * @avg_bw:    Average bandwidth in kilobytes per second
> >   * @level:     Performance level
> >   * @supplies:  Power supplies voltage/current values
> >   * @clock_latency_ns: Latency (in nanoseconds) of switching to this OPP's
> > @@ -78,6 +79,7 @@ struct dev_pm_opp {
> >         bool suspend;
> >         unsigned int pstate;
> >         unsigned long rate;
>
> If you're trying to save space why not make an anonymous union here of
> 'rate' and 'bandwidth'? Then the code doesn't read all weird.

It's not about saving space. It's about having to rewrite all the
helper functions (see subsequent patch in this series) for different
"keys" with zero difference in the actual comparisons/logic. I'm
proposing I rename this to "key" in another email.


-Saravana

  reply index

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-08-07 22:31 [PATCH v5 0/3] Introduce Bandwidth OPPs for interconnects Saravana Kannan
2019-08-07 22:31 ` [PATCH v5 1/3] dt-bindings: opp: Introduce opp-peak-kBps and opp-avg-kBps bindings Saravana Kannan
2019-08-21 20:33   ` Rob Herring
2019-08-26 20:26     ` Saravana Kannan
2019-08-07 22:31 ` [PATCH v5 2/3] OPP: Add support for bandwidth OPP tables Saravana Kannan
2019-08-16 18:21   ` Stephen Boyd
2019-08-20 22:34     ` Saravana Kannan [this message]
2019-08-20  6:13   ` Viresh Kumar
2019-08-20 22:27     ` Saravana Kannan
2019-08-20 22:36       ` Saravana Kannan
2019-08-21  5:24         ` Viresh Kumar
2019-08-21  5:26         ` Viresh Kumar
2019-08-21  5:23       ` Viresh Kumar
2019-08-07 22:31 ` [PATCH v5 3/3] OPP: Add helper function " Saravana Kannan
2019-08-16 18:22   ` Stephen Boyd
2019-08-15 16:19 ` [PATCH v5 0/3] Introduce Bandwidth OPPs for interconnects Georgi Djakov
2019-08-16  1:54   ` Saravana Kannan

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAGETcx8h2v054eS6pZ3iEsM2AGfN45_fqJGsVt+1UhbrK_dbdQ@mail.gmail.com \
    --to=saravanak@google.com \
    --cc=adharmap@codeaurora.org \
    --cc=bjorn.andersson@linaro.org \
    --cc=daidavid1@codeaurora.org \
    --cc=devicetree@vger.kernel.org \
    --cc=evgreen@chromium.org \
    --cc=georgi.djakov@linaro.org \
    --cc=kernel-team@android.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pm@vger.kernel.org \
    --cc=mark.rutland@arm.com \
    --cc=nm@ti.com \
    --cc=rjw@rjwysocki.net \
    --cc=rnayak@codeaurora.org \
    --cc=robh+dt@kernel.org \
    --cc=sboyd@kernel.org \
    --cc=seansw@qti.qualcomm.com \
    --cc=sibis@codeaurora.org \
    --cc=vincent.guittot@linaro.org \
    --cc=vireshk@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-PM Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-pm/0 linux-pm/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-pm linux-pm/ https://lore.kernel.org/linux-pm \
		linux-pm@vger.kernel.org
	public-inbox-index linux-pm

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-pm


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git